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

navigations that are not intercepted should still allow interception on further redirects #793

Closed
wanderview opened this issue Dec 1, 2015 · 21 comments

Comments

@wanderview
Copy link
Member

After talking with @annevk, it seems the intention of the spec is that every redirect of a navigation should check for service worker interception again. Currently the spec is quite vague about this.

Currently step 4.2 of HTTP Fetch unconditionally set skip-service-worker on the Request if a service worker does not intercept in step 3. In the case of a navigation that redirects, this results in a Request with skip-service-worker being set returning an opaque redirect back to the navigation spec logic. The http spec navigation logic is somewhat vague about if a new Request is created or not. The important question here is "does the skip-service-worker" flag propagate across redirects for the navigation?

To sidestep the http spec vagueness, @annevk has suggested we simply change HTTP Fetch step 4.2 to only set skip-service-worker if the RequestRedirect value is "follow".

@wanderview
Copy link
Member Author

Gecko bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1229369

Our current behavior is to check for interception again when a service worker returns a 30x response from Handle Fetch, but not re-check for interception after a service worker does not intercept.

@annevk annevk added the fetch label Dec 15, 2015
@jungkees
Copy link
Collaborator

jungkees commented Jan 7, 2016

@annevk has suggested we simply change HTTP Fetch step 4.2 to only set skip-service-worker if the RequestRedirect value is "follow".

Seems like it has not been addressed yet? Also need to check what Blink does. /cc @mattto

@jakearchibald jakearchibald added this to the Version 1 milestone Apr 10, 2016
@jakearchibald
Copy link
Contributor

Related whatwg/html#461

@jakearchibald
Copy link
Contributor

F2F: unhandled navigation requests that result in a redirect should go back through the SW

@mfalken
Copy link
Member

mfalken commented Apr 12, 2016

Just want to confirm. Chrome's navigation-redirect.html test asserts that an unhandled navigation goes back through the SW ('SW-fallbacked redirect to same-origin same-scope' test), but it looks like Mozilla and web-platform-tests asserts the opposite in that test. But Mozilla's bug "service workers should always re-intercept on navigation redirects" is marked fixed. Which test asserts the desired behavior?

Edit: fixed links

@wanderview
Copy link
Member Author

@mattto The tests we upstreamed match the current spec. We can fix the spec and firefox behavior now that we have agreement to fix the spec.

@wanderview
Copy link
Member Author

Turns out we have already implemented this and it will hit release channel next week.

@jakearchibald
Copy link
Contributor

Pre F2F notes: doesn't look like there's anything to discuss

@jungkees
Copy link
Collaborator

jungkees commented Aug 9, 2016

@annevk, I think now this issue can be closed with addressing #793 (comment). Please let me know if anything else is left.

@annevk
Copy link
Member

annevk commented Aug 9, 2016

So now that we have Foreign Fetch, the solution I proposed is no longer ideal. A same-origin to cross-origin redirect should probably have the cross-origin URL intercepted through foreign fetch, if there is such a worker there.

I'll apply the solution I proposed and leave fixing Foreign Fetch to @mkruisselbrink. That probably involves changing the "skip-service-worker flag" into something a little more sophisticated.

@annevk
Copy link
Member

annevk commented Aug 9, 2016

So actually, while trying to fix this I noted that the other problem is authentication dialogs. What if you get one of those while redirect mode is manual?

@annevk
Copy link
Member

annevk commented Aug 9, 2016

I think the solution we want is something where we skip the (same-origin) service worker until fetch has returned to its caller. HTML can then invoke HTTP-redirect fetch at a later point and not be affected. Does that make more sense?

@jungkees
Copy link
Collaborator

jungkees commented Aug 9, 2016

I noted that the other problem is authentication dialogs. What if you get one of those while redirect mode is manual?

How about:

In HTTP fetch:
Step 5.2: If request's skip-service-worker flag is unset and request's redirect mode is "follow", set skip-service-worker flag.
Step 6 > redirect status > Substep 5 > "manual": Add the step "Unset request's skip-service-worker flag."
Step 6 > 401/407: Add the step "Set request's skip-service-worker flag." before invoking subsequent HTTP fetch.

@jungkees
Copy link
Collaborator

jungkees commented Aug 9, 2016

I think the solution we want is something where we skip the (same-origin) service worker until fetch has returned to its caller.

I think we don't want to skip the service worker when the redirect mode is "manual" in the first place?

@annevk
Copy link
Member

annevk commented Aug 9, 2016

I think we don't want to skip the service worker when the redirect mode is "manual" in the first place?

That wouldn't happen in that model. But if we at some point offer a feature to skip the service worker, I don't see why that shouldn't work for the "manual" redirect mode.

From your proposal:

Step 5.2: If request's skip-service-worker flag is unset and request's redirect mode is "follow", set skip-service-worker flag.

Why the check if the flag is unset? Seems redundant.

Step 6 > redirect status > Substep 5 > "manual": Add the step "Unset request's skip-service-worker flag."

I don't think we want to do this per above. Although something close to this is needed if the 401/407 is followed by a redirect. Which is why I think a variable that we pass along rather than a flag stored on request is probably the way to go here.

@jungkees
Copy link
Collaborator

jungkees commented Aug 9, 2016

Why the check if the flag is unset? Seems redundant.

Sorry. Coming back from other context, I can't remember exactly. Seems redundant though I thought there was a reason.

Although something close to this is needed if the 401/407 is followed by a redirect.

Yes, this is the scenario I thought unsetting the flag is needed.

Which is why I think a variable that we pass along rather than a flag stored on request is probably the way to go here.

I'm fine with the way you think of it. But I think I'm not quite clear with the entire flow in detail. I'll give a feedback when you provide an outline or changes to the spec.

@annevk
Copy link
Member

annevk commented Aug 9, 2016

In the scenario I was thinking about, fetch(url, { skipSW: true, redirect: "manual" }), I guess it'll never matter whether what happens to the request. So maybe your solution is okay, although foreign fetch remains unaddressed...

@mkruisselbrink
Copy link
Collaborator

So now that we have Foreign Fetch, the solution I proposed is no longer ideal. A same-origin to cross-origin redirect should probably have the cross-origin URL intercepted through foreign fetch, if there is such a worker there.

I'm not entirely following what this issue is about. Is what you're proposing going to change non-navigation fetches? From the title of this issue I assumed this was only about navigations, which foreign fetch doesn't intercept anyway.

Ah, maybe you're saying that the same issue that with redirected navigations also applies to foreign fetch; some kind of fetch goes to the network, gets redirected to something cross origin with a foreign fetch handler. Now should that redirected request be handled by foreign fetch or not. That's a good question. Not sure what the current chrome implementation does, probably it just doesn't allow these redirects to be intercepted.

I'll apply the solution I proposed and leave fixing Foreign Fetch to @mkruisselbrink. That probably involves changing the "skip-service-worker flag" into something a little more sophisticated.

FWIW internally in chrome/blink the "skip-service-worker flag" is already more complicated than a simple boolean because of some annoying implementation details/the same flag being used for other things. But yeah, applying whatever solution and leaving fixing Foreign Fetch to me sounds good.

@annevk
Copy link
Member

annevk commented Aug 10, 2016

Not sure what the current chrome implementation does, probably it just doesn't allow these redirects to be intercepted.

Yeah, that seems wrongish. Since it means that if you end up at a service through a redirect, the service cannot work offline.

@annevk
Copy link
Member

annevk commented Aug 10, 2016

I filed whatwg/fetch#362 for the foreign fetch issue. Going to fix this per @jungkees' proposal.

@annevk
Copy link
Member

annevk commented Aug 10, 2016

So, thinking about this a bit more, HTTP authentication should probably be handled at the network layer. It doesn't make much sense for service workers to return 401/407...

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

7 participants