1340652 - fetch/xhr referrer in redirected worker scripts is wrong

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

It turns out fetch() in workers depends on the URL of the WorkerPrivate principal to be the script URL in order to set the referrer header correctly.  Bug 1337522 may have regressed this in dedicated and shared workers.  I'll write a test to check.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Looking at the code I think the referrer will be wrong if the main script load redirects.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

I wrote a simple test here:

  https://people-mozilla.org/~bkelly/worker-script-redirect/index.html

It spawns a worker at redir-script.js that redirects to script.js  The script then does a fetch() to index.html.

In FF53 the referrer on the fetch is script.js, which is correct.

In FF54 the referrer on the fetch is redir-script.js, which is incorrect.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: worker-referrer-fix.patch

Work-in-progress.  Not sure I understand the mResolvedScriptURI here.  I thought it should include redirects, but perhaps it doesn't.

Anyway, this clearly needs to get fixed.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

[Tracking Requested - why for this release]:

Functional regression.

Comment 5

[Marcia Knous [:marcia]]

Tracking 54+ for this based on Comment 4.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Test XHR referer header from redirected worker script. r=baku

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Test fetch referer header in worker and redirected worker scripts. r=baku

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Assert principal URL matches final worker script URL. r=baku

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Assert principal URL matches final worker script URL. r=baku

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Override worker principal after channel load completes to get correct principal URL. r=baku

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

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Assert principal URL matches final worker script URL. r=baku

We need to exempt blob: URLs from the principal URL check.  This likely means we send the wrong refer blob URL workers, but lets allow it for now.

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

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Assert principal URL matches final worker script URL. r=baku

We only want to assert the principal if the load succeeds.  Lots of orange on worker loads that were blocked for various reasons.

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

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Still some orange to figure out here.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Assert principal URL matches final worker script URL. r=baku

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

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8839294 [details] [diff] [review]
P1 Assert principal URL matches final worker script URL. r=baku

This patch adds assertions to clarify our expectations for the final worker principal.  Our fetch and xhr code depend on the principal to have a URL matching the worker script URL in order to set the referer correctly.  Personally I think this is incredibly error prone (bug 1340694), but this at least should help us avoid regressing this invariant again in the future.

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8839209 [details] [diff] [review]
P2 Override worker principal after channel load completes to get correct principal URL. r=baku

Add back the code that sets the final channel principal on the worker private.  While the origin has not changed we need to use the channel principal in order to have the correct principal URL for setting referer headers.

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8839190 [details] [diff] [review]
P3 Test XHR referer header from redirected worker script. r=baku

This adds a WPT test that verifies referer is set correctly from a redirected worker script.

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8839195 [details] [diff] [review]
P4 Test fetch referer header in worker and redirected worker scripts. r=baku

This adds a WPT that verifies fetch() referer headers are correct when set from a redirected worker script.

Comment 19

[Andrea Marchesini [:baku]]

Comment on attachment 8839294 [details] [diff] [review]
P1 Assert principal URL matches final worker script URL. r=baku

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

::: dom/workers/WorkerPrivate.cpp
@@ +1996,5 @@
> +  NS_ENSURE_SUCCESS(rv, false);
> +
> +  // A system principal must either be a blob URL or a resource JSM.
> +  if (mPrincipal->GetIsSystemPrincipal()) {
> +    if (scheme == NS_LITERAL_CSTRING("blob")) {

Do we use Blob with systemPrincipal? We should not.

@@ +2030,5 @@
> +  NS_ENSURE_SUCCESS(rv, false);
> +  NS_ENSURE_TRUE(principalURI, false);
> +
> +  nsAutoCString principalSpec;
> +  rv = principalURI->GetSpec(principalSpec);

return principalURI->Equals(mBaseURI);

Comment 20

[Andrea Marchesini [:baku]]

Comment on attachment 8839190 [details] [diff] [review]
P3 Test XHR referer header from redirected worker script. r=baku

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

::: testing/web-platform/tests/XMLHttpRequest/open-url-redirected-worker-origin.htm
@@ +41,5 @@
> +
> +    </script>
> +</body>
> +</html>
> +

no extra line here.

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Andrea Marchesini [:baku] from comment #19)
> > +  // A system principal must either be a blob URL or a resource JSM.
> > +  if (mPrincipal->GetIsSystemPrincipal()) {
> > +    if (scheme == NS_LITERAL_CSTRING("blob")) {
> 
> Do we use Blob with systemPrincipal? We should not.

Our tests do!  See:

https://dxr.mozilla.org/mozilla-central/source/dom/workers/test/browser_bug1104623.js#32

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Assert principal URL matches final worker script URL. r=baku

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Test XHR referer header from redirected worker script. r=baku

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

Another try since I want to make sure that nsIURI Equals() is equivalent:

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

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/513ff147aa6c
P1 Assert principal URL matches final worker script URL. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/55bc3b41baab
P2 Override worker principal after channel load completes to get correct principal URL. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b3d067a2d74
P3 Test XHR referer header from redirected worker script. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/6417e23dc2ad
P4 Test fetch referer header in worker and redirected worker scripts. r=baku

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/513ff147aa6c
https://hg.mozilla.org/mozilla-central/rev/55bc3b41baab
https://hg.mozilla.org/mozilla-central/rev/9b3d067a2d74
https://hg.mozilla.org/mozilla-central/rev/6417e23dc2ad