Closed Bug 1495880 Opened 6 years ago Closed 5 years ago

Failure in xhr/access-control-basic-cors-safelisted-request-headers.htm

Categories

(Core :: DOM: Networking, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox64 --- wontfix
firefox68 --- fixed

People

(Reporter: twisniewski, Assigned: twisniewski)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file)

This WPT is failing because our XHR code's list of CORS-safelisted request headers is out of sync with the spec: https://fetch.spec.whatwg.org/#cors-safelisted-request-header

That is, we're incorrectly allowing last-event-id, while not allowing any of: dpr, downlink, save-data, viewport-width, width
Our fetch code is also out-of-sync with the spec, and isn't safelisting dpr, downlink, save-data, viewport-width or width.
Component: DOM → DOM: Networking
I have a patch ready on try, seems fine so far: https://treeherder.mozilla.org/#/jobs?repo=try&revision=34329895722a647e399d4cd8ddf48773da18ec35

Properly passing the related WPTs required implementing the bits of the spec which validate not just the header names, but also their values (which included finding the relevant Client-Hints specs' definitions of what "valid values" actually were for those headers).
Assignee: nobody → twisniewski
update our cors/no-cors header safelisting to match the Fetch spec
(feel free to tweak the importance, but since this is already assigned and has a patch, I figured it might be a P1)
Priority: -- → P1
QA Contact: jduell.mcbugs
Whiteboard: [necko-triaged]
QA Contact: jduell.mcbugs
Jonathan is doing some work on this over in Bug 1483815. Let's just get Bug 1483815 landed and then use this bug to do the remaining bits the other bug is not addressing.
See Also: → 1483815

Alright, I've updated the patch on phabricator to add the bits that Jonathan's work didn't add in bug 1483815, and also address review feedback from the last round. A fresh try run seems fine to me: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a648f2f3cc58e554abfd534f540e29b2675b8326

For now the new patch only addresses XHRs, not Fetch. If it lands and doesn't show any signs of problems, I'll file a follow-up bug to update Fetch accordingly.

It appears that the WPT here is out-of-sync with the Fetch spec, and in fact the intent is to remove the Client Hints headers from the safelisted request headers. I'll wait to confirm all of that before updating my patch.

For the Client Hints thing Thomas, see https://github.com/whatwg/fetch/issues/867 and https://github.com/whatwg/fetch/pull/773. It's not entirely clear what the approach will end up being and I'm also not sure to what extent we're committed to any particular outcome.

I see. Thanks, Anne. It still appears that (for now) the spec will drop the Client Hints bits entirely, until a proper outcome is made. So it seems that this bug/patch should just focus on aligning Firefox with the the non-CH bits for now.

Pushed by twisniewski@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/aaedb3306c2e
update our cors/no-cors header safelisting to match the Fetch spec; r=hsivonen,ckerschb
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Backout by csabou@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/ce51e7ec806c
Backed out changeset aaedb3306c2e in attempt to fix the wpt failures on fetch.any.serviceworker.html. a=merge
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/3ebf6f2a8138
update our cors/no-cors header safelisting to match the Fetch spec; r=hsivonen,ckerschb a=reland
Whiteboard: [necko-triaged] → [necko-triaged][wptsync upstream error]
Whiteboard: [necko-triaged][wptsync upstream error] → [necko-triaged][wptsync upstream]
Whiteboard: [necko-triaged][wptsync upstream] → [necko-triaged][wptsync upstream error]
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/16663 for changes under testing/web-platform/tests
Whiteboard: [necko-triaged][wptsync upstream error] → [necko-triaged][wptsync upstream]
Whiteboard: [necko-triaged][wptsync upstream] → [necko-triaged][wptsync upstream error]
Whiteboard: [necko-triaged][wptsync upstream error] → [necko-triaged][wptsync upstream]
Whiteboard: [necko-triaged][wptsync upstream] → [necko-triaged][wptsync upstream error]
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/17071 for changes under testing/web-platform/tests
Whiteboard: [necko-triaged][wptsync upstream error] → [necko-triaged][wptsync upstream]
Upstream PR was closed without merging
Regressions: 1559795
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: