Closed
Bug 1340652
Opened 7 years ago
Closed 7 years ago
fetch/xhr referrer in redirected worker scripts is wrong
Categories
(Core :: DOM: Workers, defect)
Core
DOM: Workers
Tracking
()
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.
Assignee | ||
Comment 1•7 years ago
|
||
Looking at the code I think the referrer will be wrong if the main script load redirects.
Assignee | ||
Comment 2•7 years ago
|
||
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.
Assignee | ||
Comment 3•7 years ago
|
||
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.
Assignee | ||
Comment 4•7 years ago
|
||
[Tracking Requested - why for this release]: Functional regression.
status-firefox53:
--- → unaffected
status-firefox54:
--- → affected
tracking-firefox54:
--- → ?
Keywords: regression
Summary: fetch referrer in dedicated and shared workers may have regressed → fetch referrer in redirected worker scripts is wrong
Assignee | ||
Updated•7 years ago
|
Summary: fetch referrer in redirected worker scripts is wrong → fetch/xhr referrer in redirected worker scripts is wrong
Assignee | ||
Comment 6•7 years ago
|
||
Assignee | ||
Comment 7•7 years ago
|
||
Assignee | ||
Comment 8•7 years ago
|
||
Attachment #8838774 -
Attachment is obsolete: true
Assignee | ||
Comment 9•7 years ago
|
||
Attachment #8839203 -
Attachment is obsolete: true
Assignee | ||
Comment 10•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=52200de137c4acaf01f72bba3bfc2062109e3511
Assignee | ||
Comment 11•7 years ago
|
||
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
Assignee | ||
Comment 12•7 years ago
|
||
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
Assignee | ||
Comment 13•7 years ago
|
||
Still some orange to figure out here.
Assignee | ||
Comment 14•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9685c43a6fc05d7589ecd4ed0ef605795bf5db52
Attachment #8839264 -
Attachment is obsolete: true
Assignee | ||
Comment 15•7 years ago
|
||
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)
Assignee | ||
Comment 16•7 years ago
|
||
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)
Assignee | ||
Comment 17•7 years ago
|
||
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)
Assignee | ||
Comment 18•7 years ago
|
||
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 19•7 years ago
|
||
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+
Updated•7 years ago
|
Attachment #8839209 -
Flags: review?(amarchesini) → review+
Comment 20•7 years ago
|
||
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+
Updated•7 years ago
|
Attachment #8839195 -
Flags: review?(amarchesini) → review+
Assignee | ||
Comment 21•7 years ago
|
||
(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
Assignee | ||
Comment 22•7 years ago
|
||
Attachment #8839294 -
Attachment is obsolete: true
Attachment #8840429 -
Flags: review+
Assignee | ||
Comment 23•7 years ago
|
||
Attachment #8839190 -
Attachment is obsolete: true
Attachment #8840430 -
Flags: review+
Assignee | ||
Comment 24•7 years ago
|
||
Another try since I want to make sure that nsIURI Equals() is equivalent: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e13b1c4286591585296a2dd116e37fa84045b37
Comment 25•7 years ago
|
||
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
Comment 26•7 years ago
|
||
bugherder |
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
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Updated•7 years ago
|
status-firefox52:
--- → unaffected
Updated•7 years ago
|
Iteration: --- → 54.3 - Mar 6
Updated•7 years ago
|
Whiteboard: [e10s-multi:+]
You need to log in
before you can comment on or make changes to this bug.
Description
•