[CORS] Add wildcard to Access-Control-Expose-Headers, Access-Control-Allow-Methods, and Access-Control-Allow-Headers
Categories
(Core :: DOM: Networking, enhancement, P2)
Tracking
()
People
(Reporter: fs, Assigned: kershaw)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete, Whiteboard: [domsecurity-backlog2] spec change [necko-triaged])
Attachments
(2 files)
47 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
RyanVM
:
approval-mozilla-esr68+
|
Details | Review |
47 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
RyanVM
:
approval-mozilla-esr68+
|
Details | Review |
A recent change in the Fetch spec: https://github.com/whatwg/fetch/commit/cdbb13c08650b10c9ebfc54d046bec0639e7ba7c > Enable Access-Control-Expose-Headers, Access-Control-Allow-Methods, > and Access-Control-Allow-Headers to use a wildcard, with the same > restriction as placed upon wildcards in Access-Control-Allow-Origin. > Namely, it can only be used for requests where the credentials mode is "omit". > The Authorization header still needs to be explicitly listed by > Access-Control-Allow-Headers even with the wildcard. > This also makes the CORS cache wildcard-aware and updates some of the > terminology around CORS caches to share more concepts.
Reporter | ||
Comment 1•8 years ago
|
||
The new syntax: Access-Control-Expose-Headers = #field-name / wildcard Access-Control-Allow-Methods = #method / wildcard Access-Control-Allow-Headers = #field-name-or-wildcard The difference between the Access-Control-Expose-Headers and Access-Control-Allow-Headers production is that the latter needs to be able to handle `*, Authorization` as header value whereas the former does not.
Updated•8 years ago
|
Comment 2•8 years ago
|
||
Basic tests for Access-Control-Expose-Headers: https://github.com/w3c/web-platform-tests/pull/5047.
Comment 3•8 years ago
|
||
Basic tests for Access-Control-Allow-Methods/Headers: * at https://github.com/w3c/web-platform-tests/pull/5050.
Reporter | ||
Comment 4•7 years ago
|
||
There is an open spec issue here https://github.com/whatwg/fetch/issues/548
Comment 5•7 years ago
|
||
That's now resolved via https://github.com/whatwg/fetch/pull/592. The semantics ended up being tweaked slightly and adjusted tests are at https://github.com/w3c/web-platform-tests/pull/7223 (will land soon). There's nothing blocking this now that I'm aware of.
Updated•6 years ago
|
Comment 8•5 years ago
|
||
I don't know exactly who works on Fetch in these days. I suspect just me (but I hope I'm wrong)... Andrew, can you help assigning this bug to someone? I'm happy to mentor/help/review.
The entry point of this bug is InternalHeaders::CORSHeaders():
https://searchfox.org/mozilla-central/rev/6c9f60f8cc064a1005cd8141ecd526578ae9da7a/dom/fetch/InternalHeaders.cpp#512
Comment 9•5 years ago
|
||
Nhi, can someone from Necko take this?
Comment 10•5 years ago
|
||
in Necko's queue
Comment 11•5 years ago
|
||
:annevk, could you help us determine the priority for this? Is there a particular release we should target? Thanks!
Comment 12•5 years ago
|
||
Chrome ships this and developers want to use this to make configuration of their server setups easier (and more cache-friendly). Implementation should not be too much work (basically carefully tweaking existing logic to account for *
in non-credential scenarios). Hard to assign a priority without knowing all the work the team is responsible for, but I think it'd be good to get this in sooner rather than later as we've let it wait for quite a while already.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 13•5 years ago
|
||
Assignee | ||
Comment 14•5 years ago
|
||
This may be worth bumping up in priority, since we've seen some related webcompat fallout now that we've updated our CORS-handling to better match the spec (with CesiumJS in bug 1559795). For now, Cesium has worked around that known fallout, but it seems worth fixing this as soon as possible in case more appears (so we can potentially request uplift to at least the 68 ESR).
Comment 16•5 years ago
|
||
Pushed by kjang@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c15f9113c30a P1: Add wildcard to Access-Control-Expose-Headers r=baku https://hg.mozilla.org/integration/autoland/rev/765be63207f3 P2: Add wildcard to Access-Control-Allow-Method and Access-Control-Allow-Headers r=baku,mayhemer
Comment 17•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c15f9113c30a
https://hg.mozilla.org/mozilla-central/rev/765be63207f3
Updated•5 years ago
|
Comment 18•5 years ago
|
||
Shouldn't this land in 68 to fix bug 1559795 ?
Yes, if this can be landed in 68 and 68 ESR, along with bug 1564175, that would likely be ideal.
Assignee | ||
Comment 20•5 years ago
|
||
(In reply to Thomas Wisniewski [:twisniewski] from comment #19)
Yes, if this can be landed in 68 and 68 ESR, along with bug 1564175, that would likely be ideal.
Ryan, is it too late to request the uplift to 68?
Comment 21•5 years ago
|
||
[Tracking Requested - why for this release]:
Bug 1559795 had this tracking flags +.
They were removed to make the decision here instead (bug 1559795 comment 31)
Comment 22•5 years ago
|
||
(In reply to Kershaw Chang [:kershaw] from comment #20)
Ryan, is it too late to request the uplift to 68?
Too late for 68, but feel free to request for ESR68 if you feel strongly that it should be considered. Presumably you'd also want to request for Beta as well if you think this needs uplift.
Comment 23•5 years ago
|
||
FWIW I have seen this in the wild so especially if this ships in stable Firefox there will probably be some sites that are broken in ESR. Seeing as it is already in use today I would recomend uplifting at least to ESR it to keep everything working smoothly in firefox.
Assignee | ||
Comment 24•5 years ago
|
||
Comment on attachment 9075409 [details]
Bug 1309358 - P1: Add wildcard to Access-Control-Expose-Headers
ESR Uplift Approval Request
- If this is not a sec:{high,crit} bug, please state case for ESR consideration: This patch helps to fix some webcompat issues and also makes firefox match the fetch spec better.
- User impact if declined: Some websites might work not well.
- Fix Landed on Version: 70
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): The patch is quite simple. It should be not risky.
- String or UUID changes made by this patch: none
Beta/Release Uplift Approval Request
- User impact if declined: Some websites might work not well.
- Is this code covered by automated tests?: Yes
- Has the fix been verified in Nightly?: Yes
- Needs manual test from QE?: No
- If yes, steps to reproduce:
- List of other uplifts needed: Bug 1564175
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): The patch is quite simple and we also have wpt tests for this.
It should be not risky. - String changes made/needed: none
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Updated•5 years ago
|
Comment 25•5 years ago
|
||
Comment on attachment 9075409 [details]
Bug 1309358 - P1: Add wildcard to Access-Control-Expose-Headers
Fixes some spec compliance issues which have caused real-world site bustage in Fx68. Covered by web-platform-tests. Approved for 69.0b9 and 68.1esr.
Updated•5 years ago
|
Comment 26•5 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/80afdf70f4a3
https://hg.mozilla.org/releases/mozilla-beta/rev/82c9cdc700d8
Comment 27•5 years ago
|
||
bugherder uplift |
Reporter | ||
Comment 28•5 years ago
|
||
Announced on Firefox 69 for developers:
Reference pages update (under "directives"):
- https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Expose-Headers
- https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Methods
- https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Headers
:annevk, can you review if the wording is correct? (what I had in my original description here isn't what got implemented, so what I wrote on the MDN pages now is how I understood the current text at https://fetch.spec.whatwg.org/#http-new-header-syntax)
Also, should this get an extra row in the MDN compat table or is it rather a minor detail?
Comment 29•5 years ago
|
||
It's a minor addition, but probably worth noting separately due to the huge gap in time since the initial feature.
Feedback on the documentation:
- In general, looks great.
- An aspect not documented there is that the Authorization header cannot be wildcarded.
- Now that we also impose restrictions on the values of Accept, Accept-Language, and Content-Language (see bug 1564175), there might be legitimate reasons to specify them in Access-Control-Allow-Headers. This was always true for Content-Type and although it is somewhat suggested by the text that could be a little clearer.
- It might be nice to do some cleanup on terminology and align with the Fetch Standard (e.g., simple headers is not a thing anymore).
Hope that helps and thanks for writing it!
Reporter | ||
Comment 30•5 years ago
|
||
Thanks for your excellent feedback!
I've made some changes:
- The compat table will get a row labeled "Wildcard (
*
)" with https://github.com/mdn/browser-compat-data/pull/4577 - I've given the wildcard info a bit more space in the text, so it can be seen as an additional feature.
- I've added the fact that the Authorization header cannot be wildcarded in Access-Control-Expose-Headers and Access-Control-Allow-Headers.
- I've updated the "simple (response) header" terms to the Fetch terms CORS-safelisted request header and CORS-safelisted response header. This might not be perfect throughout the MDN wiki pages yet, but I tried :)
I have two more questions:
Now that we also impose restrictions on the values of Accept, Accept-Language, and Content-Language (see bug 1564175), there might be legitimate reasons to specify them in Access-Control-Allow-Headers. This was always true for Content-Type and although it is somewhat suggested by the text that could be a little clearer.
-
Are you saying that if I put the Accept, Accept-language, Content-Language or Content-Type header names in a Access-Control-Allow-Headers header that these restrictions no longer apply and thus it would sometimes make sense to list them in Access-Control-Allow-Headers?
-
I'm not 100% sure I understand the back-and-forth restrictions introduced in bug 1564175, but I think we should document the ones explained here on the CORS-safelisted request header page, right? Are there more restrictions now or are those explained by :jkt the current ones?
Comment 31•5 years ago
|
||
Thanks!
- Yes.
- We also restrict the length of the value. It should match https://fetch.spec.whatwg.org/#cors-safelisted-request-header (there's corresponding tests in web-platform-tests too).
Reporter | ||
Comment 32•5 years ago
|
||
Awesome, thank you!
I've updated https://developer.mozilla.org/en-US/docs/Glossary/CORS-safelisted_request_header and https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Headers again to take this into account.
I think we're done here.
Description
•