Fetch requests issued with service worker change referer

RESOLVED FIXED in Firefox 49

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: info, Assigned: bkelly)

Tracking

(Blocks 1 bug)

49 Branch
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 wontfix, firefox48 wontfix, firefox49+ fixed, firefox-esr45 unaffected, firefox50+ fixed)

Details

Attachments

(6 attachments)

Reporter

Description

3 years ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/50.0.2661.102 YaBrowser/16.6.0.8125 Safari/537.36

Steps to reproduce:

1. Create a service worker file
2. Listen to 'fetch' events inside of it
3. Issue fetch requests with evt.request as an argument


Actual results:

evt.request.referrer is "about:client" or any URL, but server shows that "Referer" header is "/service-worker.js" file. I also saved a video of that: https://www.youtube.com/watch?v=shOvjBVKSFI&feature=youtu.be


Expected results:

if evt.request.referrer is "about:client" server shouldn't have received "Referer" header at all. If evt.request.referrer is URL, server should see it inside "Referer" header, but not /sevice-worker.js file
Reporter

Comment 1

3 years ago
I also forgot to mention that this behaviour is different in Chrome: issued request's referrer doesn't get changed there.
Reporter

Comment 2

3 years ago
And here are the files I use to reproduce the issue: https://gist.github.com/1999/868a999af31a7d25dd2811ab824cbea0
Component: Untriaged → DOM: Service Workers
Product: Firefox → Core
Flags: needinfo?(bkelly)
Reproduced.  I think I see the problem as well.
Assignee: nobody → bkelly
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(bkelly)
This is a bug in how we implement the Request constructor step 8:

https://fetch.spec.whatwg.org/#dom-request

We don't copy the referrer like we are supposed to.  That one line change fixes the test case for me.

I'll write a wpt test next.
Attachment #8769701 - Flags: review?(bugmail)
[Tracking Requested - why for this release]:

I think this should have been implemented as part of bug 1251872 which landed in FF47.  We should uplift this web compat issue to beta 48 if we can.
This test fails without the P1 patch and succeeds with it applied.
Attachment #8769714 - Flags: review?(bugmail)
Comment on attachment 8769701 [details] [diff] [review]
P1 Make InternalRequest::GetRequestConstructorCopy() copy the referrer per the spec. r=asuth

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

Confirming/restating: InternalRequest::GetRequestConstructorCopy exclusively corresponds to step 8.  The implied intent of the erroneous comment would presumably be covered by the "explicit InternalRequest(const nsACString& aURL)" signature used in the !aInput.IsRequest() case or the aInit.IsAnyMemberPresent() referrer{policy} clobberin' corresponding to step 13 that follows.
Attachment #8769701 - Flags: review?(bugmail) → review+
Comment on attachment 8769714 [details] [diff] [review]
P2 Add wpt verifying fetch() respects a Request object's referrer. r=asuth

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

Test exercises the code path via Fetch method explicit use of Request constructor.  And although Gecko has separate document and worker code-paths for parsing/processing the referrer when passed, they don't meaningfully diverge when passing "about:client".  (And WPT/tests/fetch/api/referrer-origin.js covers testing the diverging failure paths from both document and worker contexts, so it already has coverage.)
Attachment #8769714 - Flags: review?(bugmail) → review+
Looks like this affects another test as well:

TEST-UNEXPECTED-FAIL | /service-workers/service-worker/referer.https.html | Verify the referer - assert_equals: expected "finish" but got "failure:Referer for request-headers.py must be https://web-platform.test:8443/service-workers/service-worker/resources/fetch-rewrite-worker.js but got https://web-platform.test:8443/service-workers/service-worker/resources/referer-iframe.html"
The service worker wpt tests actually exercise the exact path from comment 0.  They trigger a fetch(event.request) in the fetch-rewrite-worker.js service worker script.  So fixing this code requires updating that test expectation.
Attachment #8769765 - Flags: review?(bugmail)
Attachment #8769765 - Attachment description: Fix service worker wpt test to expect fetch(event.request) to use original referrer. r=asuth → P3 Fix service worker wpt test to expect fetch(event.request) to use original referrer. r=asuth
Comment on attachment 8769765 [details] [diff] [review]
P3 Fix service worker wpt test to expect fetch(event.request) to use original referrer. r=asuth

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

Confirming: of the three checks, with the second one modified:
- First skips SW and uses original fetch, referrer is correctly the iframe.
- Second uses SW which reuses the existing event.request verbatim, the comment 0 situation, and so the right answer is instead the iframe, not the worker.
- Third uses SW and creates an explicit new Request for the URL, and its RequestInit dict is populated but without referrer, so the worker is still the right referrer in this case.
Attachment #8769765 - Flags: review?(bugmail) → review+

Comment 14

3 years ago
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e86a99a37d83
P1 Make InternalRequest::GetRequestConstructorCopy() copy the referrer per the spec. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/5746260b3644
P2 Add wpt verifying fetch() respects a Request object's referrer. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/9224d883dbb1
P3 Fix service worker wpt test to expect fetch(event.request) to use original referrer. r=asuth
Comment on attachment 8769701 [details] [diff] [review]
P1 Make InternalRequest::GetRequestConstructorCopy() copy the referrer per the spec. r=asuth

Approval Request Comment
[Feature/regressing bug #]: Service workers and fetch.  Should have been implemented as part of bug 1251872.
[User impact if declined]: Web compatibility issues with chrome.
[Describe test coverage new/current, TreeHerder]: New wpt test included in this bug.  Previous test coverage verifies referer policy is still enforced.
[Risks and why]: Minimal due to test coverage.  Only affects service workers and fetch().
[String/UUID change made/needed]: None.
Attachment #8769701 - Flags: approval-mozilla-beta?
Attachment #8769701 - Flags: approval-mozilla-aurora?
Comment on attachment 8769714 [details] [diff] [review]
P2 Add wpt verifying fetch() respects a Request object's referrer. r=asuth

See comment 15.
Attachment #8769714 - Flags: approval-mozilla-beta?
Attachment #8769714 - Flags: approval-mozilla-aurora?
Comment on attachment 8769765 [details] [diff] [review]
P3 Fix service worker wpt test to expect fetch(event.request) to use original referrer. r=asuth

See comment 15.
Attachment #8769765 - Flags: approval-mozilla-beta?
Attachment #8769765 - Flags: approval-mozilla-aurora?
Rebase of P1 for beta/aurora.
Rebase of P2 for aurora.
Posted patch Beta P2 patchSplinter Review
Rebase of P2 for beta.
I don't think this needs to be uplifted to 45ESR because bug 1251872 only landed in FF47.

Comment 22

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e86a99a37d83
https://hg.mozilla.org/mozilla-central/rev/5746260b3644
https://hg.mozilla.org/mozilla-central/rev/9224d883dbb1
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Hi Ben,
If we let this ride the train on 49/50 only and won't fix in 48, will this be a big impact for user for 48?
Flags: needinfo?(bkelly)
(In reply to Gerry Chang [:gchang] from comment #23)
> If we let this ride the train on 49/50 only and won't fix in 48, will this
> be a big impact for user for 48?

It would be nice to get in to minimize compat issues for developers, but if FF48 is considered under risk for some reason I understand not uplifting it.
Flags: needinfo?(bkelly)
Comment on attachment 8769701 [details] [diff] [review]
P1 Make InternalRequest::GetRequestConstructorCopy() copy the referrer per the spec. r=asuth

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

Hi Ben,
Thanks for understanding. Let's let it ride the train on 49 and 50.
Attachment #8769701 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Comment on attachment 8769714 [details] [diff] [review]
P2 Add wpt verifying fetch() respects a Request object's referrer. r=asuth

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

Per comment #24.
Attachment #8769714 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Comment on attachment 8769765 [details] [diff] [review]
P3 Fix service worker wpt test to expect fetch(event.request) to use original referrer. r=asuth

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

Per comment #24.
Attachment #8769765 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Un-track and won't fix in 48.
Tracking 50+ for this web compat issue.
Comment on attachment 8769701 [details] [diff] [review]
P1 Make InternalRequest::GetRequestConstructorCopy() copy the referrer per the spec. r=asuth

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

Per comment #24, the patch fixes the web compatibility issues. Take it in 49 aurora.
Attachment #8769701 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8769714 [details] [diff] [review]
P2 Add wpt verifying fetch() respects a Request object's referrer. r=asuth

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

As mentioned in comment #30, take it in 49 aurora.
Attachment #8769714 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8769765 [details] [diff] [review]
P3 Fix service worker wpt test to expect fetch(event.request) to use original referrer. r=asuth

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

As mentioned in comment #30, take it in 49 aurora.
Attachment #8769765 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Track 49 for this web compat issue.
You need to log in before you can comment on or make changes to this bug.