Closed Bug 1340652 Opened 3 years ago Closed 3 years ago

fetch/xhr referrer in redirected worker scripts is wrong

Categories

(Core :: DOM: Workers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Iteration:
54.3 - Mar 6
Tracking Status
firefox52 --- unaffected
firefox53 --- unaffected
firefox54 + fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

()

Details

(Keywords: regression, Whiteboard: [e10s-multi:+] )

Attachments

(4 files, 7 obsolete files)

4.82 KB, patch
baku
: review+
Details | Diff | Splinter Review
2.62 KB, patch
baku
: review+
Details | Diff | Splinter Review
4.89 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
3.79 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
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.
Looking at the code I think the referrer will be wrong if the main script load redirects.
See Also: → 1340694
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.
Attached patch worker-referrer-fix.patch (obsolete) — Splinter Review
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.
[Tracking Requested - why for this release]:

Functional regression.
Keywords: regression
Summary: fetch referrer in dedicated and shared workers may have regressed → fetch referrer in redirected worker scripts is wrong
Tracking 54+ for this based on Comment 4.
Summary: fetch referrer in redirected worker scripts is wrong → fetch/xhr referrer in redirected worker scripts is wrong
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
Attachment #8839207 - Attachment is obsolete: true
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
Attachment #8839251 - Attachment is obsolete: true
Still some orange to figure out here.
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.
Attachment #8839294 - Flags: review?(amarchesini)
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.
Attachment #8839209 - Flags: review?(amarchesini)
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.
Attachment #8839190 - Flags: review?(amarchesini)
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.
Attachment #8839195 - Flags: review?(amarchesini)
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);
Attachment #8839294 - Flags: review?(amarchesini) → review+
Attachment #8839209 - Flags: review?(amarchesini) → review+
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.
Attachment #8839190 - Flags: review?(amarchesini) → review+
Attachment #8839195 - Flags: review?(amarchesini) → review+
(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
Another try since I want to make sure that nsIURI Equals() is equivalent:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e13b1c4286591585296a2dd116e37fa84045b37
Pushed by bkelly@mozilla.com:
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
Iteration: --- → 54.3 - Mar 6
Whiteboard: [e10s-multi:+]
Depends on: 1406547
You need to log in before you can comment on or make changes to this bug.