1244249 - Implement structured cloning for push interfaces

Status: RESOLVED WONTFIX

(Core :: DOM: Push Subscriptions, enhancement, P3)

Description

[Lina Butler [:lina]]

Martin pointed out in bug 1191931 that we don't currently support this. I could see a case for sending subscriptions across frames via `postMessage`, if you've registered a push-only worker in an iframe and want to manage them from the parent page. You could also persist them...though `serviceWorkerRegistration.pushManager.getSubscription()` will always return the subscription, so there's no need to do that yourself.

Adding structured clone support for `PushMessageData` would be nice. That way, you could persist payloads directly to IndexedDB.

Comment 1

[Lina Butler [:lina]]

Martin, these would require additions to the spec, correct?

Comment 2

[Martin Thomson [:mt:]]

Yes, we need to explicitly define how to clone something for it to be usable with structured cloning.  I think that we should be careful to preserve the origin (i.e., principal) when we transfer the object so that calls to serialize and getKey() fail on other origins.  At least that way we stop little accidental leaks of keying material.

I think that we can save a principal property on the subscription and do caller.principal.subsumes(subscription.principal) when calling into getKey() and when serializing.  Both need to throw a SecurityError at that point.

So yes, this isn't that easy to get right.  But it will be useful to be able to (for example) store a subscription in indexedDB.

Comment 3

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1244249, Part 1 - Add JS_ReadString to match JS_WriteString.

Review commit: https://reviewboard.mozilla.org/r/32949/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32949/

Comment 4

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1244249, Part 2 - Implement structured cloning for push subscriptions.

Review commit: https://reviewboard.mozilla.org/r/32951/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32951/

Comment 5

[Lina Butler [:lina]]

Here's some progress I made on the train, although it doesn't handle the principals correctly.

This looks tricky to implement, too, because nsIPrincipal is main-thread only. Going from main thread to worker, I think we'd need to perform the security check in a WorkerMainThreadRunnable when we create the WorkerPushSubscription. We can get the principal from the WorkerPrivate and check if it subsumes the PushSubscription's principal.

Going the other way, I guess we'd also use a WorkerMainThreadRunnable to serialize the principal, then perform the check when we create the PushSubscription. I'm assuming it's not safe to use the worker thread's JSStructuredCloneWriter on the main thread...so we may need to call PrincipalToPrincipalInfo in the runnable, and pass the PrincipalInfo struct back to the worker thread. In that case, we should extract the serialization logic from nsJSPrincipals::write (and ReadKnownPrincipalType).

It seems like there should be an easier way to do this. AFAICT, though, the existing interfaces that support structured cloning don't do these security checks.

Comment 6

[Martin Thomson [:mt:]]

Hmm, thinking about this some more, I think that the security checks aren't going to do any good anyway.  It's not like we can stop someone from extracting keys and passing the raw values.  And I don't see any way we might *use* a subscription in another origin in a way that would let them *use* the public-private key pair.  I did really want to avoid accidental leaking of the authentication key, but I guess if it's hard and pointless we can just see if a simple clone is acceptable.

Comment 7

[Lina Butler [:lina]]

I'll be curious to hear what the others think. Will hold off on requesting review until we have a spec strawman.

Comment 8

[Lina Butler [:lina]]

Unassigning me until the spec is updated.

Comment 9

[Kagami Rosylight [:saschanaz] (they/them)]

The spec doesn't have this, btw: https://w3c.github.io/push-api/#dom-pushsubscription (No [Transferable])

Comment 10

[Kagami Rosylight [:saschanaz] (they/them)]

I don't think we should do this. Maybe the spec changed since the initial report?