Bug 1496577 - have OPTIONS preflights inherit the original request's referrer and referrer policy; r?ckerschb
46 bytes, text/x-phabricator-request
|Details | Review|
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
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.
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.)
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)?
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.
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.
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.
>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?
(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!
Sure. A fresh try-run still seems fine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=981f3c293d1e41d6236c93923c68857c7a2a45fe Requesting check-in.
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/4c60ba7cca76 have OPTIONS preflights inherit the original request's referrer and referrer policy; r=ckerschb
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]
Can't merge web-platform-tests PR due to failing upstream checks: Github PR https://github.com/web-platform-tests/wpt/pull/13498 * continuous-integration/travis-ci/pr (https://travis-ci.org/web-platform-tests/wpt/builds/440963760?utm_source=github_status&utm_medium=notification)
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.