CORS preflights are not sent with the original request's referrer and referrer policy

RESOLVED FIXED in Firefox 64

Status

()

enhancement
P2
normal
RESOLVED FIXED
11 months ago
10 months ago

People

(Reporter: twisniewski, Assigned: twisniewski)

Tracking

(Blocks 1 bug)

Trunk
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

(Whiteboard: [necko-triaged][wptsync upstream])

Attachments

(1 attachment)

Per spec step 1, the OPTIONS request must inherit the original request's referrer and referrer policy: https://fetch.spec.whatwg.org/#cors-preflight-fetch

We are not doing that, causing us to fail most of these WPTs: http://w3c-test.org/fetch/api/cors/cors-preflight-referrer.any.html (and the related worker tests)

It looks like we should be able to just copy the relevant values from the original request in nsCORSListenerProxy::StartCORSPreflight to match the spec.
A try-run seems fine with that approach, so I'll post a patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8bb9aaf13cc62be891b5c6b9116fdaeb3f64299
OPTIONS preflights inherit the original request's referrer and referrer policy
Assignee: nobody → twisniewski
Priority: -- → P2
QA Contact: jduell.mcbugs
Whiteboard: [necko-triaged]
Anne, could you please take a look and sign off we are doing the right thing here. From a technical perspective this is correct. Please see my comment in phabricator.
Flags: needinfo?(annevk)
I went through blame and I couldn't find a version where this wasn't supposed to have a referrer for at least six years. Before that it wasn't really stated one way or another and we might generally not have defined Referer that well.

(Four tests of http://w3c-test.org/fetch/api/cors/cors-preflight-referrer.any.html fail in Chrome and Safari too as well by the way and should also continue to fail in Firefox when run over port 80, as the default port really needs to be elided. It seems the test (like many others) doesn't take into account that it can be run over port 80.)
Flags: needinfo?(annevk)
QA Contact: jduell.mcbugs
Bizarre. The results on wpt.fyi shows Chrome and Safari as passing all of the tests when they actually don't.

But yes, this patch does also fail those four tests when run over port 80 as Chrome/Safari do.

If it helps I could modify testing/web-platform/tests/common/get_host_info.sub.js as part of this patch, so that it does not add ":80" to the HTTP_ORIGIN/etc variables (just adding the port to the URL if it is not 80)?
Flags: needinfo?(annevk)
I think it's fine to leave it not working on port 80 since there are many tests with this problem (but it's also fine to fix it). I suspect that wpt.fyi doesn't use port 80 to obtain the results, but something like port 8000 as is also the default when you run the tests locally, perhaps in part to not run into this.
Flags: needinfo?(annevk)
Sure, Anne. If you're fine with the WPT change I made in the patch at this try-run, then I don't mind landing it with the rest of the patch: https://hg.mozilla.org/try/rev/4e77783600214d0df0025d69e3ffa9b3054b4c0e

But if that seems like the wrong fix, then I'll leave it out.
Flags: needinfo?(annevk)
The HTTPS port can only be elided if it's 443, not 80 (see https://url.spec.whatwg.org/#default-port). If you change that I think it's okay and probably a change worth making.
Flags: needinfo?(annevk)
>The HTTPS port can only be elided if it's 443

Um, right. Duh.

>it's okay and probably a change worth making.

Okay then, I've updated the patch to make the WPT change (with the :443 fix in comment 8).



Chris, do you have any remaining doubts/concerns, or should we go ahead and land this one?
Flags: needinfo?(ckerschb)
(In reply to Thomas Wisniewski [:twisniewski] from comment #9)
> Chris, do you have any remaining doubts/concerns, or should we go ahead and
> land this one?

Nope, please go ahead and land. Thanks for fixing actually!
Flags: needinfo?(ckerschb)
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4c60ba7cca76
have OPTIONS preflights inherit the original request's referrer and referrer policy; r=ckerschb
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4c60ba7cca76
Status: NEW → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/13498 for changes under testing/web-platform/tests
Whiteboard: [necko-triaged] → [necko-triaged][wptsync upstream]
Whiteboard: [necko-triaged][wptsync upstream] → [necko-triaged][wptsync upstream error]
Whiteboard: [necko-triaged][wptsync upstream error] → [necko-triaged][wptsync upstream]
Upstream PR merged
You need to log in before you can comment on or make changes to this bug.