1285502 - Fetch requests issued with service worker change referer

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Dmitry Sorin]

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

Comment 1

[Dmitry Sorin]

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

Comment 2

[Dmitry Sorin]

And here are the files I use to reproduce the issue: https://gist.github.com/1999/868a999af31a7d25dd2811ab824cbea0

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Reproduced.  I think I see the problem as well.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Make InternalRequest::GetRequestConstructorCopy() copy the referrer per the spec. r=asuth

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.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

[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.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add wpt verifying fetch() respects a Request object's referrer. r=asuth

This test fails without the P1 patch and succeeds with it applied.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 8

[Andrew Sutherland [:asuth] (he/him)]

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.

Comment 9

[Andrew Sutherland [:asuth] (he/him)]

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.)

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

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"

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Fix service worker wpt test to expect fetch(event.request) to use original referrer. r=asuth

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.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 13

[Andrew Sutherland [:asuth] (he/him)]

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.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
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 15

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8769714 [details] [diff] [review]
P2 Add wpt verifying fetch() respects a Request object's referrer. r=asuth

See comment 15.

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Beta/Aurora P1 patch

Rebase of P1 for beta/aurora.

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Aurora P2 patch

Rebase of P2 for aurora.

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Beta P2 patch

Rebase of P2 for beta.

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

I don't think this needs to be uplifted to 45ESR because bug 1251872 only landed in FF47.

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e86a99a37d83
https://hg.mozilla.org/mozilla-central/rev/5746260b3644
https://hg.mozilla.org/mozilla-central/rev/9224d883dbb1

Comment 23

[Gerry Chang [:gchang]]

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?

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

(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.

Comment 25

[Gerry Chang [:gchang]]

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.

Comment 26

[Gerry Chang [:gchang]]

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.

Comment 27

[Gerry Chang [:gchang]]

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.

Comment 28

[Gerry Chang [:gchang]]

Un-track and won't fix in 48.

Comment 29

[Marcia Knous [:marcia]]

Tracking 50+ for this web compat issue.

Comment 30

[Gerry Chang [:gchang]]

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.

Comment 31

[Gerry Chang [:gchang]]

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.

Comment 32

[Gerry Chang [:gchang]]

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.

Comment 33

[Gerry Chang [:gchang]]

Track 49 for this web compat issue.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4e0eadf7b6a1
https://hg.mozilla.org/releases/mozilla-aurora/rev/9baf67cba254
https://hg.mozilla.org/releases/mozilla-aurora/rev/1ba7f271fd89