1309358 - [CORS] Add wildcard to Access-Control-Expose-Headers, Access-Control-Allow-Methods, and Access-Control-Allow-Headers

Status: RESOLVED FIXED

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

Description

[Florian Scholz (Open Web Docs)]

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.

Comment 1

[Florian Scholz (Open Web Docs)]

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.

Comment 2

[Anne (:annevk)]

Basic tests for Access-Control-Expose-Headers: https://github.com/w3c/web-platform-tests/pull/5047.

Comment 3

[Anne (:annevk)]

Basic tests for Access-Control-Allow-Methods/Headers: * at https://github.com/w3c/web-platform-tests/pull/5050.

Comment 4

[Florian Scholz (Open Web Docs)]

There is an open spec issue here https://github.com/whatwg/fetch/issues/548

Comment 5

[Anne (:annevk)]

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.

Comment 6

[Shachar]

any update ?

Comment 7

[Valentin Gosu [:valentin] (he/him)]

Andrea, who should work on this bug?

Comment 8

[Andrea Marchesini [:baku]]

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

[Andrew Overholt [:overholt]]

Nhi, can someone from Necko take this?

Comment 10

[Nhi Nguyen (:nhi)]

in Necko's queue

Comment 11

[Nhi Nguyen (:nhi)]

:annevk, could you help us determine the priority for this? Is there a particular release we should target? Thanks!

Comment 12

[Anne (:annevk)]

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.

Comment 13

[Kershaw Chang [:kershaw]]

Attachment: Bug 1309358 - P1: Add wildcard to Access-Control-Expose-Headers

Comment 14

[Kershaw Chang [:kershaw]]

Attachment: Bug 1309358 - P2: Add wildcard to Access-Control-Allow-Method and Access-Control-Allow-Headers

Comment 15

[Thomas Wisniewski [:twisniewski]]

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

[Pulsebot]

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

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/c15f9113c30a
https://hg.mozilla.org/mozilla-central/rev/765be63207f3

Comment 18

[j.j.]

Shouldn't this land in 68 to fix bug 1559795 ?

Comment 19

[Thomas Wisniewski [:twisniewski]]

Yes, if this can be landed in 68 and 68 ESR, along with bug 1564175, that would likely be ideal.

Comment 20

[Kershaw Chang [:kershaw]]

(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

[j.j.]

[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

[Ryan VanderMeulen [:RyanVM]]

(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

[Kevin Cox [@kevincox:matrix.org]]

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.

Comment 24

[Kershaw Chang [:kershaw]]

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

Comment 25

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/80afdf70f4a3
https://hg.mozilla.org/releases/mozilla-beta/rev/82c9cdc700d8

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/4ed9a5ede7fb
https://hg.mozilla.org/releases/mozilla-esr68/rev/fc78fbb5899b

Comment 28

[Florian Scholz (Open Web Docs)]

Announced on Firefox 69 for developers:

• https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/69#HTTP

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

[Anne (:annevk)]

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!

Comment 30

[Florian Scholz (Open Web Docs)]

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.

1. 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?

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

[Anne (:annevk)]

Thanks!

1. Yes.
2. 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).

Comment 32

[Florian Scholz (Open Web Docs)]

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.