Service Workers should intercept redirects for "manual" requests

RESOLVED FIXED in Firefox 55

Status

()

defect
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jugglinmike, Assigned: bkelly)

Tracking

(Blocks 1 bug, {site-compat})

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr45 disabled, firefox-esr52 disabled, firefox54- wontfix, firefox55 fixed, firefox56 fixed)

Details

Attachments

(5 attachments, 5 obsolete attachments)

2.18 KB, patch
dragana
: review+
Details | Diff | Splinter Review
2.37 KB, patch
dragana
: review+
Details | Diff | Splinter Review
1.05 KB, patch
dragana
: review+
Details | Diff | Splinter Review
805 bytes, patch
dragana
: review+
Details | Diff | Splinter Review
4.95 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
(Reporter)

Description

2 years ago
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
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.
Priority: -- → P1
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".
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Andrew, you probably care about whatever I do here because it will impact your parent side intercept stuff.
(Assignee)

Updated

2 years ago
Blocks: 1293277
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.
Attachment #8876852 - Flags: review?(dd.mozilla)
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.
Attachment #8877368 - Flags: review?(dd.mozilla)
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.
Attachment #8877369 - Flags: review?(dd.mozilla)
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.
Attachment #8877370 - Flags: review?(dd.mozilla)
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.
Attachment #8877371 - Flags: review?(dd.mozilla)
Attachment #8877371 - Flags: review?(dd.mozilla) → review+
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.
Attachment #8876852 - Flags: review?(dd.mozilla) → review+
Attachment #8877368 - Flags: review?(dd.mozilla) → review+
Attachment #8877369 - Flags: review?(dd.mozilla) → review+
Attachment #8877370 - Flags: review?(dd.mozilla) → review+
Updated with review feedback.
Attachment #8876852 - Attachment is obsolete: true
Attachment #8878042 - Flags: review+

Comment 22

2 years ago
Pushed by bkelly@mozilla.com:
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
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 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
Attachment #8878042 - Flags: approval-mozilla-beta?
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.
Attachment #8877368 - Flags: approval-mozilla-beta?
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.
Attachment #8877369 - Flags: approval-mozilla-beta?
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.
Attachment #8877370 - Flags: approval-mozilla-beta?
Comment on attachment 8877371 [details] [diff] [review]
P5 Update WPT test expectations. r=dragana

See comment 25.
Attachment #8877371 - Flags: approval-mozilla-beta?
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.
Attachment #8878042 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8877368 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8877370 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8877371 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8877369 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
[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)
Is this a regression in 54?
(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.
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
You need to log in before you can comment on or make changes to this bug.