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

Status: RESOLVED FIXED

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

Description

[Thomas Wisniewski [:twisniewski]]

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

Comment 1

[Thomas Wisniewski [:twisniewski]]

Our fetch code is also out-of-sync with the spec, and isn't safelisting dpr, downlink, save-data, viewport-width or width.

Comment 2

[Thomas Wisniewski [:twisniewski]]

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).

Comment 3

[Thomas Wisniewski [:twisniewski]]

Attachment: Bug 1495880 - update our cors/no-cors header safelisting to match the Fetch spec; r?hsivonen,r?ckerschb

update our cors/no-cors header safelisting to match the Fetch spec

Comment 4

[Daniel Stenberg [:bagder]]

(feel free to tweak the importance, but since this is already assigned and has a patch, I figured it might be a P1)

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

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.

Comment 6

[Thomas Wisniewski [:twisniewski]]

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.

Comment 7

[Thomas Wisniewski [:twisniewski]]

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.

Comment 8

[Anne (:annevk)]

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.

Comment 9

[Thomas Wisniewski [:twisniewski]]

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.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aaedb3306c2e
update our cors/no-cors header safelisting to match the Fetch spec; r=hsivonen,ckerschb

Comment 11

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/aaedb3306c2e

Comment 12

[Pulsebot]

Backout by [e-mail]:
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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
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

Comment 14

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/16663 for changes under testing/web-platform/tests

Comment 15

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/17071 for changes under testing/web-platform/tests

Comment 16

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging