1363848 - Service Workers should intercept redirects for "manual" requests

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect, P1)

Description

[Mike Pennisi [:jugglinmike]]

Steps to reproduce:

1. Register a service worker and wait for it to become active
2. Insert an iframe into the document whose `src` is in-scope of the service
   worker and produces an HTTP 302 redirect response.

Expected: the active service worker should intercept the initial request and
the subsequent redirected request.

Actual: the active service worker intercepts the initial request only.

Background:

This behavior reflects the letter of the Fetch specification prior to the
commit at [1]. That commit limits the application of the "skip-service-worker
flag" to requests whose "redirect mode" is "follow", as noted in the commit
message:

> [...]
> * Only when redirects are automatically followed should we set the
> skip-service-worker flag, otherwise we negatively affect navigations.
> [...]

Although the request's "skip-service-worker flag" has since been re-implemented
as the "service-worker mode" [2], the standard still specifies the behavior
described above.

Bug 1229369 seems to describe the same problem, and it has been resolved as
"FIXED". That said, the referenced change to the specification occurred *after*
the Bugzilla ticket was closed, so this may reflect some churn in the
specification itself. The Web Platform Tests project currently includes an
invalid test for the erroneous behavior [3], and Firefox passes that test. I
will be correcting the test shortly.

[1] https://github.com/whatwg/fetch/commit/ec6f5ef5f99cb6b0dd6c701b49791810fb380b04
[2] https://github.com/whatwg/fetch/commit/d41c2380dc828e7a23c6196a344b42b2d0e9beec
[3] https://github.com/w3c/web-platform-tests/blob/6fe674bcceee914ca912f870f32b0f85f9fd7483/service-workers/service-worker/navigation-redirect.https.html

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

This is an important compat issue where we differ from chrome.  We should probably fix this ASAP.  Redirects on navigate are heavily used by web authentication work flows.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

This is impacting my work in bug 1293277 since those patches effect how network requests are controlled during redirect.

I think the trickiest part of this bug will be *not intercepting* for internal redirects, but intercepting for other redirects.  I don't think we can just add the BYPASS_SERVICE_WORKER flag for internal redirects because it would then get propagated to a later real redirect.  I'll need to add some new flag that says "don't intercept on this channel, but if you are redirected go ahead and intercept".

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Andrew, you probably care about whatever I do here because it will impact your parent side intercept stuff.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Record the last redirect flags on the http channel. r=dragana

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Do not intercept internal redirects regardless of LOAD_BYPASS_SERVICE_WORKER flag. r=dragana

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Don't set LOAD_BYPASS_SERVICE_WORKER flag on internal redirects in nsHttpChannel. r=dragana

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Only set LOAD_BYPASS_SERVICE_WORKER when resetting the child channel if we're not in "manual" redirect mode. r=dragana

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Update WPT test expectations. r=dragana

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b14b2f0adc958446277e896e1cf0d7e1431fa20e

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=947e2bb65562e9a3048e686a4f18c1fe03e697da

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Do not intercept internal redirects regardless of LOAD_BYPASS_SERVICE_WORKER flag. r=dragana

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Don't set LOAD_BYPASS_SERVICE_WORKER flag on internal redirects in nsHttpChannel. r=dragana

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Only set LOAD_BYPASS_SERVICE_WORKER when resetting the child channel if we're not in "manual" redirect mode. r=dragana

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Update WPT test expectations. r=dragana

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=08356985dc7e76707f77fa6af4126cc6d6ed02e4

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8876852 [details] [diff] [review]
P1 Record the last redirect flags on the http channel. r=dragana

Review of attachment 8876852 [details] [diff] [review]:
-----------------------------------------------------------------

Dragana, this patch adds a new member to HttpBaseChannel to remember the redirect flags used to create the channel.  I want to do this so I can inspect if the channel was created as the result of an internal redirect in HttpBaseChannel::ShouldIntercept() in the next patch.

Previously we avoided intercepting internal redirects by setting the LOAD_BYPASS_SERVICE_WORKER flag.  This is problematic, though, because that is a persistent flag that we persist across both internal and real redirects.  We set it for many other reasons.  The tests described in comment 0 show that we incorrectly bypass the service worker on further real redirects after an internal redirect.  This is happening because we are trying to use the persistent load flag here.

An alternative solution would be to create another load flag just for this purpose.  Something like "LOAD_BYPASS_SERVICE_WORKER_ONLY_ONCE" and then clear it on the next redirect.  I chose not to go with this route for a couple reasons:

1. I think we might be running low on load flags bits.
2. Adding a load flag and then immediately clearing it would be somewhat unique and not like other load flags (AFAIK).

I'm open to other solutions if this one is not necko'y enough.

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8877368 [details] [diff] [review]
P2 Do not intercept internal redirects regardless of LOAD_BYPASS_SERVICE_WORKER flag. r=dragana

Review of attachment 8877368 [details] [diff] [review]:
-----------------------------------------------------------------

This patch uses the new mLastRedirectFlags member to automatically avoid service worker interception on internal redirects.  I tried to write a detailed comment here, so hopefully the patch is somewhat self-explanatory.

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8877369 [details] [diff] [review]
P3 Don't set LOAD_BYPASS_SERVICE_WORKER flag on internal redirects in nsHttpChannel. r=dragana

Review of attachment 8877369 [details] [diff] [review]:
-----------------------------------------------------------------

This patch removes the place where we used to set the LOAD_BYPASS_SERVICE_WORKER flag on internal redirects previously.  The old logic was a bit convoluted, but it effectively only applied to INTERNAL_REDIRECT.

Note, the existing bit here about setting LOAD_BYPASS_SERVICE_WORKER if the mRedirectMode is not manual is basically step 4.2 here:

https://fetch.spec.whatwg.org/#http-fetch

The spec phrases this as:

  If request’s redirect mode is "follow", then set request’s service-workers mode to "foreign".

We are using the "not manual" check as a more defensive version of "mode is follow".  We don't have foreign fetch service workers yet, so we effectively use service-workers mode "none" by setting LOAD_BYPASS_SERVICE_WORKER.

Note, if you would like to defer any part of the review to someone on the service worker team :asuth has been looking at this and could probably field any of the service worker specific bits.

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8877370 [details] [diff] [review]
P4 Only set LOAD_BYPASS_SERVICE_WORKER when resetting the child channel if we're not in "manual" redirect mode. r=dragana

Review of attachment 8877370 [details] [diff] [review]:
-----------------------------------------------------------------

This patch implements the conditional in step 4.2 from the spec on the e10s child side:

https://fetch.spec.whatwg.org/#http-fetch

We were incorrectly disabling further interception for all types of requests when in e10s mode.  This is necessary for the fallback test cases discussed in comment 0 to work in e10s mode.

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8877371 [details] [diff] [review]
P5 Update WPT test expectations. r=dragana

Review of attachment 8877371 [details] [diff] [review]:
-----------------------------------------------------------------

This patch simply updates the WPT test expectations now that we are passing the tests per spec.

Comment 20

[Dragana Damjanovic [:dragana]]

Comment on attachment 8876852 [details] [diff] [review]
P1 Record the last redirect flags on the http channel. r=dragana

Review of attachment 8876852 [details] [diff] [review]:
-----------------------------------------------------------------

I think this solution is just fine.

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +4041,5 @@
>    return NS_OK;
>  }
>  
> +NS_IMETHODIMP
> +HttpBaseChannel::GetLastRedirectFlags(uint32_t* aValue)

can you change this to uint32_t *aValue

::: netwerk/protocol/http/HttpBaseChannel.h
@@ +259,5 @@
>    virtual void SetCorsPreflightParameters(const nsTArray<nsCString>& unsafeHeaders) override;
>    NS_IMETHOD GetConnectionInfoHashKey(nsACString& aConnectionInfoHashKey) override;
>    NS_IMETHOD GetIntegrityMetadata(nsAString& aIntegrityMetadata) override;
>    NS_IMETHOD SetIntegrityMetadata(const nsAString& aIntegrityMetadata) override;
> +  NS_IMETHOD GetLastRedirectFlags(uint32_t* aValue) override;

here as well.

@@ +631,5 @@
>    bool mIsTrackingResource;
>  
>    uint64_t mChannelId;
>  
> +  uint32_t mLastRedirectFlags;

can you add here the same comment as in the idl (we have mRedirectMode and mLastRedirecFlags at first look that can be confusing...) Thanks.

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Record the last redirect flags on the http channel. r=dragana

Updated with review feedback.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6eae0eebdda8
P1 Record the last redirect flags on the http channel. r=dragana
https://hg.mozilla.org/integration/mozilla-inbound/rev/32cf4cab7660
P2 Do not intercept internal redirects regardless of LOAD_BYPASS_SERVICE_WORKER flag. r=dragana
https://hg.mozilla.org/integration/mozilla-inbound/rev/48ddbce7a837
P3 Don't set LOAD_BYPASS_SERVICE_WORKER flag on internal redirects in nsHttpChannel. r=dragana
https://hg.mozilla.org/integration/mozilla-inbound/rev/04e557c1a8c8
P4 Only set LOAD_BYPASS_SERVICE_WORKER when resetting the child channel if we're not in "manual" redirect mode. r=dragana
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ba0efde7212
P5 Update WPT test expectations. r=dragana

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6eae0eebdda8
https://hg.mozilla.org/mozilla-central/rev/32cf4cab7660
https://hg.mozilla.org/mozilla-central/rev/48ddbce7a837
https://hg.mozilla.org/mozilla-central/rev/04e557c1a8c8
https://hg.mozilla.org/mozilla-central/rev/0ba0efde7212

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

There is a large site that is hitting this problem in their login workflow.  I think we should try to uplift to at least FF55 beta if we can.

Comment 25

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8878042 [details] [diff] [review]
P1 Record the last redirect flags on the http channel. r=dragana

Approval Request Comment
[Feature/Bug causing the regression]: Service Workers
[User impact if declined]: This is a web compat issue.  In particular it can break redirect-based authentication paths (oauth/etc) when service workers are enabled.  We have had one large site report the problem to us.
[Is this code covered by automated tests?]: There is a WPT that covers this behavior.
[Has the fix been verified in Nightly?]: Yes.  I verified that this bug fixes the problem for the large site running into the compat issue.
[Needs manual test from QE? If yes, steps to reproduce]: None
[List of other uplifts needed for the feature/fix]: None.  The WPT expectations file might need to be rebased on uplift.
[Is the change risky?]: Low risk.
[Why is the change risky/not risky?]: The changes here are relatively small and should only impact service workers.  Also, we are still in the first week of the beta release cycle.
[String changes made/needed]: None

Comment 26

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8877368 [details] [diff] [review]
P2 Do not intercept internal redirects regardless of LOAD_BYPASS_SERVICE_WORKER flag. r=dragana

See comment 25.

Comment 27

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8877369 [details] [diff] [review]
P3 Don't set LOAD_BYPASS_SERVICE_WORKER flag on internal redirects in nsHttpChannel. r=dragana

See comment 25.

Comment 28

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8877370 [details] [diff] [review]
P4 Only set LOAD_BYPASS_SERVICE_WORKER when resetting the child channel if we're not in "manual" redirect mode. r=dragana

See comment 25.

Comment 29

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8877371 [details] [diff] [review]
P5 Update WPT test expectations. r=dragana

See comment 25.

Comment 30

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8878042 [details] [diff] [review]
P1 Record the last redirect flags on the http channel. r=dragana

Let's land this work for the beta 3 build on Monday as discussed over email. We have some automated test coverage and we should be able to test against the broken site.

Comment 31

[Ben Kelly [:bkelly, not reviewing]]

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/2bb8cf5019229b773d15c65a537f414e99376431
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/a2ec6f92b8df102c0849762cdc77c406c9e88503
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/74875a638dcd11ab0fc2bb329cb72914712a6475
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/574dac67fc5bdd63f96a70e88f5d85ef611e046e
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/b08a305464d127e25fab19f358fac15fceb52954

Comment 32

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

[Tracking Requested - why for this release]: This is a service worker compat problem.  We got an email today from Microsoft telling us Outlook Web authentication is breaking because of it. (from Ben's email to r-d ML)

Comment 33

[Kohei Yoshino [:kohei]]

Is this a regression in 54?

Comment 34

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Kohei Yoshino [:kohei] from comment #33)
> Is this a regression in 54?

No.  We've had this behavior for a long time.

Comment 35

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

From Ben's latest email, this is a wontfix for 54. MS is planning to disable SWs on Firefox until Fx55 goes live. They are not using push so users will not see any degraded features