Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

reserved Client objects and redirected navigations #1031

Closed
wanderview opened this issue Dec 16, 2016 · 24 comments
Closed

reserved Client objects and redirected navigations #1031

wanderview opened this issue Dec 16, 2016 · 24 comments

Comments

@wanderview
Copy link
Member

I'm looking at implementing the new Client semantics including "reserved Client" for navigations. I'm curious, though, if the current spec works as intended for redirects.

Consider:

  1. Register service workers at https://foo.com/ and https://bar.com/
  2. Browser navigates a new tab to https://foo.com/a
  3. https://foo.com/a is intercepted by the foo.com SW and passed through to the network
  4. https://foo.com/a redirects to https://foo.com/b
  5. https://foo.com/b is intercepted by the foo.com SW and passed through to the network
  6. https://foo.com/b redirects to https://bar.com
  7. https://bar.com is intercepted by the bar.com SW

Per the current spec:

  • The FetchEvents in steps (2), (4), and (6) will all have the same reservedClientId.
  • In steps (2), (4), and (6) the reserved Client.url will be https://foo.com/a (the original URL)
  • The foo.com SW in steps (2) and (4) can postMessage() to the Client which is ultimately in the bar.com origin

The postMessage() behavior seems like a clear SOP violation and should be fixed. We could do this by either:

  1. When a redirect crosses origin boundaries clear any pending postMessage() events on the reserved Client.
  2. When a redirect crosses origin boundaries create a new reserved Client.
  3. Create a new reserved Client on every redirect.

I personally lean towards creating a new reserved Client on every redirect. This would also ensure the Client.url matches the actual final URL loaded in the window. It also seems conceptually consistent with an algorithm that uses manual redirection and re-enters the service worker each time. It also simplifies edge cases for the service worker code. It doesn't have to worry about possibly having a reserved Client postMessage()'d by two different service workers or twice by the same service worker by accident.

Thoughts? @jungkees @jakearchibald

@wanderview
Copy link
Member Author

For now I am going to work towards implementing a new reserved Client on every redirect. I can adjust if we consensus reaches a different conclusion.

@wanderview
Copy link
Member Author

Also consider that the worker environment creation URL is set based on the worker global scope's URL which reflects redirects. It seems we should probably reflect redirects here as well.

Of course, the worker creation URL is only set after following all redirects and not at each redirect step. It also doesn't give us the opportunity to create a new Client on each redirect since fetch spec handles the redirections.

The fact that workers are same-origin avoids issues with cross-origin postMessage(), though.

I guess there is an open question here about how close we need to keep worker and window Client behavior to each other. Closer seems better, though.

@jungkees
Copy link
Collaborator

jungkees commented Dec 19, 2016

Thanks for spotting this!

I personally lean towards creating a new reserved Client on every redirect. This would also ensure the Client.url matches the actual final URL loaded in the window. It also seems conceptually consistent with an algorithm that uses manual redirection and re-enters the service worker each time. It also simplifies edge cases for the service worker code. It doesn't have to worry about possibly having a reserved Client postMessage()'d by two different service workers or twice by the same service worker by accident.

Agreed to all those points. Redirect mode "manual" and the fact that every redirect going through registration match again seem like good reasons we have new reserved clients for them.

I guess there is an open question here about how close we need to keep worker and window Client behavior to each other. Closer seems better, though.

Basically, I think we can think of it as we defer the behavior to what the request expects to fetch. Here, the worker script fetch "follows" redirects, so a single reserved client makes sense to me. Note that, though, in #1034 (comment), I suggested we initialize the reserved client's url to the request url in the worker case, too.

@jungkees jungkees added this to the Version 1 milestone Dec 19, 2016
@jakearchibald
Copy link
Contributor

One of the aims of reserved clients is to be able to postMessage and have those buffered until the page is created. I agree that we can't allow these to happen cross-origin, but it feels like we're losing one of the features behind the creation of reserved clients.

A way around this is to provide a way to get a genuine client from a reserved client - a promise that would resolve with a client or undefined if the client isn't created or goes cross-origin.

@jungkees
Copy link
Collaborator

A way around this is to provide a way to get a genuine client from a reserved client - a promise that would resolve with a client or undefined if the client isn't created or goes cross-origin.

@jakearchibald, your idea of getting a genuine client is under the assumption of using a single reserved client throughout the redirects of a navigation or multiple reserved clients for each redirect? Hope the API isn't get too much complex to cover a corner case but not sure if this case is rather common in practice.

@wanderview
Copy link
Member Author

I have thought about this a bit more over the weekend.

Given the fact that we are making Client.url always empty string while reserved, the only issues to resolve here is the cross-origin postMessage(). As Jake said, its desirable to have that work for same-origin.

So I now think it would be a bit better to keep the same reserved Client when a redirect is same-origin. If a redirect cross origin boundaries then a new reserved Client would need to be created.

This solves the immediate problem and also make the window reserved Client consistent with the worker reserved Client during redirects.

@wanderview
Copy link
Member Author

A way around this is to provide a way to get a genuine client from a reserved client - a promise that would resolve with a client or undefined if the client isn't created or goes cross-origin.

A Client.ready promise seems reasonable to me in practice, but it might be a bit clunky given that Client is a snapshot. The promise would resolve, but Client.reserved would still return true.

Personally I'd be happy to make Client a live object in the future, but I think there were implementation concerns around that in blink. (Or perhaps I am thinking of the issue that drove us to have FetchEvent.clientID instead of the Client itself on the event.)

@wanderview
Copy link
Member Author

wanderview commented Dec 19, 2016

Another reason to create a new Client when following a redirect across origins:

In general Caches.matchAll() and Caches.get() are same-origin. You can't see cross-origin Client objects. I don't think we should ever allow a single Client ID to be exposed under different origins. Creating a new Client and associated ID is the best way to achieve this.

Edit: In other words, the origin of a Client should never be allowed to change. Even though there is no URL exposed on Client while its reserved it still has a pinned origin.

@wanderview
Copy link
Member Author

We should also make it clear that if a Window navigation is redirected the final Client object has the final redirected Response URL as well.

@jungkees
Copy link
Collaborator

the only issues to resolve here is the cross-origin postMessage().

So I now think it would be a bit better to keep the same reserved Client when a redirect is same-origin. If a redirect cross origin boundaries then a new reserved Client would need to be created.

I like this reasoning but see a corner case with the same-origin redirect scenario:

Browser has two registrations for an origin: foo.com/a/ and foo.com/b/.

  1. Browser navigates a new tab to https://foo.com/a/resource.
  2. https://foo.com/a/resource is intercepted by the foo.com/a/ SW and passed through to the network.
  3. https://foo.com/a/resource redirects to https://foo.com/b/resource.
  4. https://foo.com/b/resource is intercepted by the foo.com/b/ SW.

In this case, if step 2 and step 4 called postMessage() to their reserved clients, they will be queued for the same client. I think client matching needs a new client.

@wanderview
Copy link
Member Author

I can go either way on this issue. @jakearchibald, what do you think?

@jungkees
Copy link
Collaborator

jungkees commented Dec 21, 2016

To summarize, we have three options here:

  1. Use new clients for each redirect (and so each registration matching)
  • Seems no corner case
  • Can't retain all the messages targeted to the intermediary clients; only the last environment can hold the queued messagesAll the clients can retain only their own messages
  1. Use a single client throughout redirects
  • Override environment's creation URL to request's url for each entry to main fetch
  • Basically queue messages to the environment, but to avoid SOP violation, when crossing an origin boundary, the messages queued in the environment should be flushed before invoking main fetch
  • Depending on the location urls, multiple SWs with different scopes can postMessage() to the same client. This means devs may have to control the received messages in the application level.
  1. Create a new client when crossing origin boundaries
  • Best of both worlds (or worst of?)

1 seems the simplest. 3 seems good in the sense that a single reserved client represents what becomes the actual client at the end regardless of which location urls are encountered. For 2 and 3, the fact that client:scope can be 1:N bothers me.

@annevk
Copy link
Member

annevk commented Dec 22, 2016

  1. A SW's scope does not matter, right?
  2. Same-origin -> cross -> same, needs to be three clients I think. Seems easy to open up a side channel otherwise.

@jungkees
Copy link
Collaborator

  1. A SW's scope does not matter, right?

In the option 1, the scope apparently doesn't matter as only the final client will survive and the other clients created for redirects won't get the actual global object. But in the option 2 and 3, the same client is reused for redirects. (e.g. the case in #1031 (comment).) So, the messages sent from multiple (same-origin) SWs with different scopes will be delivered to the same ServiceWorkerContainer object. Not sure if it's an expected behavior. I didn't think it'd be but not really sure.

  1. Same-origin -> cross -> same, needs to be three clients I think. Seems easy to open up a side channel otherwise.

This opens up two possibilities. It can creates three clients as you suggested, and the final client will survive, being associated with the actual global object. Or it can use two clients for different origins, in which case we might retain the message from the same-origin SW before it redirected to cross-origin resource. (For this, we'll need to track the redirects to queue only the messages from the same-origin SWs to the original request's url.)

@wanderview
Copy link
Member Author

As an implementation note, I likely will have to do something like:

  1. Create the reserved Client object before the initial network request.
  2. On redirect mutate the existing Client object to the new origin/URL/ID and wipe any pending messages.
  3. Freeze the origin/URL/ID once the Client is marked as execution ready.

I don't like this, but due to a number of implementation details its very very racy to handle redirects for worker threads any other way. The redirects happen on a different thread altogether.

@jakearchibald
Copy link
Contributor

Once an origin is crossed as part of a redirect, the reserved client must be discarded and a new one created for the other origin.

In terms of clients that cross service worker scopes, I think that's fine, even if multiple service workers queue messages for that client.

@wanderview

A Client.ready promise seems reasonable to me in practice, but it might be a bit clunky given that Client is a snapshot. The promise would resolve, but Client.reserved would still return true.

I was thinking the promise would resolve with the real (not-reserved) client.

@wanderview
Copy link
Member Author

As an implementation note, I likely will have to do something like:

  • Create the reserved Client object before the initial network request.
  • On redirect mutate the existing Client object to the new origin/URL/ID and wipe any pending messages.
  • Freeze the origin/URL/ID once the Client is marked as execution ready.

I don't like this, but due to a number of implementation details its very very racy to handle redirects for worker threads any other way. The redirects happen on a different thread altogether.

FWIW, turns out I don't need to do this any more. I was able to prove to myself that we don't allow cross-origin redirects for worker scripts even with our non-standard data URL handling in gecko.

So I will create a new Client where appropriate in firefox instead of mutating the existing one.

@wanderview
Copy link
Member Author

For added fun, @bzbarsky posed this question while implementing this:

If a sandboxed iframe is loads foo.com and it redirects to another foo.com URL, is that considered same-origin or cross-origin?

My initial implementation in gecko is going to treat it as cross-origin since sandboxed iframes use opaque origins. What do people think?

@jungkees
Copy link
Collaborator

jungkees commented Feb 9, 2017

Yeah, the newly created document for another foo.com will have a unique origin. So, treating it as cross-origin makes sense. I don't see any reason we shouldn't follow it.

  1. Create a new client when crossing origin boundaries

Has this been the consensus? If so, "origin1 to origin2 to origin1 to origin2" will have two clients or four clients?

@wanderview
Copy link
Member Author

Has this been the consensus? If so, "origin1 to origin2 to origin1 to origin2" will have two clients or four clients?

I plan to do four clients here. I don't think its worth the complexity to try to match redirects back to old clients on the same network request in previous redirects. That would be complicated and this is a corner case.

@jungkees
Copy link
Collaborator

Agreed.

@jakearchibald
Copy link
Contributor

F2F: 4 clients in the #1031 (comment) sounds good - new client at every change of origin.

Reuse reserved client for same origin navigations.

@annevk
Copy link
Member

annevk commented Apr 3, 2017

(As I noted before, I'd be very suspicious about reusing clients in the "corner case" scenario (A -> B -> A) (which is not that corner case really, with lots of login flows using it). That might very well lead to subtle attacks. So this solution seems good.)

@jakearchibald
Copy link
Contributor

We aren't going to expose reserved clients, so this is no longer an issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants