Bug 165508

Summary: Add wildcard to Access-Control-Allow-Methods and Access-Control-Allow-Headers
Product: WebKit Reporter: Michael[tm] Smith <mike>
Component: Page LoadingAssignee: Rob Buis <rbuis>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, ap, beidson, cdumez, commit-queue, dbates, ews, fred.wang, japhet, lopezhector206, rbuis, rniwa, webkit-bug-importer, wilander, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews101 for mac-sierra
none
Archive of layout-test-results from ews107 for mac-sierra-wk2
none
Archive of layout-test-results from ews112 for mac-sierra
none
Archive of layout-test-results from ews121 for ios-simulator-wk2
none
Patch
none
Patch
none
Archive of layout-test-results from ews102 for mac-sierra
none
Archive of layout-test-results from ews104 for mac-sierra-wk2
none
Archive of layout-test-results from ews115 for mac-sierra
none
Patch
none
Archive of layout-test-results from ews103 for mac-sierra
none
Archive of layout-test-results from ews116 for mac-sierra
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews211 for win-future
none
Patch none

Description Michael[tm] Smith 2016-12-06 19:35:41 PST
May 2016 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.

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.

Blink bug: https://bugs.chromium.org/p/chromium/issues/detail?id=615313
Gecko bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1309358
Comment 1 Rob Buis 2018-11-25 13:09:10 PST
Created attachment 355603 [details]
Patch
Comment 2 Build Bot 2018-11-25 14:12:34 PST
Comment on attachment 355603 [details]
Patch

Attachment 355603 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10147623

New failing tests:
imported/w3c/web-platform-tests/fetch/api/cors/cors-preflight-star.any.worker.html
imported/w3c/web-platform-tests/fetch/api/cors/cors-preflight-star.any.html
Comment 3 Build Bot 2018-11-25 14:12:36 PST
Created attachment 355604 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 4 Build Bot 2018-11-25 14:23:24 PST
Comment on attachment 355603 [details]
Patch

Attachment 355603 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10147634

New failing tests:
imported/w3c/web-platform-tests/fetch/api/cors/cors-preflight-star.any.worker.html
imported/w3c/web-platform-tests/fetch/api/cors/cors-preflight-star.any.html
Comment 5 Build Bot 2018-11-25 14:23:26 PST
Created attachment 355605 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 6 Build Bot 2018-11-25 15:06:09 PST
Comment on attachment 355603 [details]
Patch

Attachment 355603 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10147672

New failing tests:
imported/w3c/web-platform-tests/fetch/api/cors/cors-preflight-star.any.worker.html
imported/w3c/web-platform-tests/fetch/api/cors/cors-preflight-star.any.html
Comment 7 Build Bot 2018-11-25 15:06:11 PST
Created attachment 355610 [details]
Archive of layout-test-results from ews112 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 8 Build Bot 2018-11-25 15:14:14 PST
Comment on attachment 355603 [details]
Patch

Attachment 355603 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10147700

New failing tests:
imported/w3c/web-platform-tests/fetch/api/cors/cors-preflight-star.any.worker.html
imported/w3c/web-platform-tests/fetch/api/cors/cors-preflight-star.any.html
Comment 9 Build Bot 2018-11-25 15:14:15 PST
Created attachment 355611 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6
Comment 10 Rob Buis 2018-11-26 07:20:45 PST
Created attachment 355638 [details]
Patch
Comment 11 Frédéric Wang (:fredw) 2018-11-26 10:53:57 PST
Comment on attachment 355638 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=355638&action=review

> Source/WebCore/ChangeLog:10
> +        add this to the check. Same for ccess-Control-Allow-Headers (step 6.7).

Access-Control-Allow-Headers

> Source/WebCore/loader/CrossOriginPreflightResultCache.cpp:86
> +    if (m_methods.contains(method) || (m_methods.contains("*") && storedCredentialsPolicy == StoredCredentialsPolicy::DoNotUse) || isOnAccessControlSimpleRequestMethodWhitelist(method))

I wonder if this and the statement below can be factor out in a separate helper function.

> Source/WebCore/loader/CrossOriginPreflightResultCache.cpp:98
> +        if (!m_headers.contains(header.key) && !(m_headers.contains("*") && storedCredentialsPolicy == StoredCredentialsPolicy::DoNotUse)) {

it seems this check is independent of header, so we can probably hence calculate it only once and take this out of the loop.
Comment 12 Rob Buis 2018-11-30 00:16:53 PST
Created attachment 356153 [details]
Patch
Comment 13 Build Bot 2018-11-30 01:25:02 PST
Comment on attachment 356153 [details]
Patch

Attachment 356153 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10211865

New failing tests:
imported/w3c/web-platform-tests/fetch/api/cors/cors-expose-star.sub.any.html
imported/w3c/web-platform-tests/fetch/api/cors/cors-expose-star.sub.any.worker.html
Comment 14 Build Bot 2018-11-30 01:25:04 PST
Created attachment 356162 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 15 Build Bot 2018-11-30 01:35:55 PST
Comment on attachment 356153 [details]
Patch

Attachment 356153 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10211880

New failing tests:
imported/w3c/web-platform-tests/service-workers/cache-storage/window/cache-match.https.html
imported/w3c/web-platform-tests/service-workers/cache-storage/worker/cache-match.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/fetch-cors-exposed-header-names.https.html
imported/w3c/web-platform-tests/service-workers/cache-storage/serviceworker/cache-match.https.html
Comment 16 Build Bot 2018-11-30 01:35:57 PST
Created attachment 356163 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 17 Build Bot 2018-11-30 02:16:09 PST
Comment on attachment 356153 [details]
Patch

Attachment 356153 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10212034

New failing tests:
imported/w3c/web-platform-tests/fetch/api/cors/cors-expose-star.sub.any.html
imported/w3c/web-platform-tests/fetch/api/cors/cors-expose-star.sub.any.worker.html
Comment 18 Build Bot 2018-11-30 02:16:11 PST
Created attachment 356165 [details]
Archive of layout-test-results from ews115 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 19 Rob Buis 2018-11-30 08:24:06 PST
Created attachment 356179 [details]
Patch
Comment 20 Build Bot 2018-11-30 09:32:29 PST
Comment on attachment 356179 [details]
Patch

Attachment 356179 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10215395

New failing tests:
imported/w3c/web-platform-tests/fetch/api/cors/cors-expose-star.sub.any.html
imported/w3c/web-platform-tests/fetch/api/cors/cors-expose-star.sub.any.worker.html
Comment 21 Build Bot 2018-11-30 09:32:31 PST
Created attachment 356187 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 22 Build Bot 2018-11-30 10:24:13 PST
Comment on attachment 356179 [details]
Patch

Attachment 356179 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10215582

New failing tests:
imported/w3c/web-platform-tests/fetch/api/cors/cors-expose-star.sub.any.html
imported/w3c/web-platform-tests/fetch/api/cors/cors-expose-star.sub.any.worker.html
Comment 23 Build Bot 2018-11-30 10:24:15 PST
Created attachment 356193 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 24 Rob Buis 2018-11-30 11:45:55 PST
Created attachment 356209 [details]
Patch
Comment 25 Rob Buis 2018-12-07 08:53:59 PST
Created attachment 356815 [details]
Patch
Comment 26 Rob Buis 2018-12-07 09:58:44 PST
I noticed https://bugs.webkit.org/show_bug.cgi?id=169194 already tracks Access-Control-Expose-Headers wildcard support, so this bug can be restricted to Access-Control-Allow-Methods and Access-Control-Allow-Headers.
Comment 27 Frédéric Wang (:fredw) 2018-12-18 00:18:31 PST
Comment on attachment 356815 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=356815&action=review

> Source/WebCore/ChangeLog:3
> +        Add wildcard to Access-Control-Expose-Headers, Access-Control-Allow-Methods, and Access-Control-Allow-Headers

I guess you can update the bug title then

> Source/WebCore/ChangeLog:10
> +        add this to the check. Same for ccess-Control-Allow-Headers (step 6.7).

Again, A is missing at ccess-Control-Allow-Headers

> Source/WebCore/loader/CrossOriginPreflightResultCache.cpp:86
> +    if (m_methods.contains(method) || (m_methods.contains("*") && storedCredentialsPolicy == StoredCredentialsPolicy::DoNotUse) || isOnAccessControlSimpleRequestMethodWhitelist(method))

So StoredCredentialsPolicy is the same as the spec's credentials mode (https://fetch.spec.whatwg.org/#concept-request-credentials-mode)? If so then probably we should use the same name conventions in the future. Also, here and below steps 6.5 and 6.7 say we should really check that the credentials mode is not "include" i.e. storedCredentialsPolicy != StoredCredentialsPolicy::Use so that it still works when we have more than two values.
Comment 28 Rob Buis 2018-12-18 06:30:05 PST
Created attachment 357565 [details]
Patch
Comment 29 Rob Buis 2019-06-08 10:38:45 PDT
Created attachment 371657 [details]
Patch
Comment 30 Build Bot 2019-06-08 14:30:41 PDT
Comment on attachment 371657 [details]
Patch

Attachment 371657 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12419994

New failing tests:
css3/filters/blur-various-radii.html
imported/blink/fast/canvas/bug382588.html
Comment 31 Build Bot 2019-06-08 14:30:44 PDT
Created attachment 371667 [details]
Archive of layout-test-results from ews211 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews211  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 32 Rob Buis 2019-06-09 02:30:11 PDT
Created attachment 371706 [details]
Patch
Comment 33 WebKit Commit Bot 2019-06-09 04:55:34 PDT
Comment on attachment 371706 [details]
Patch

Clearing flags on attachment: 371706

Committed r246238: <https://trac.webkit.org/changeset/246238>
Comment 34 WebKit Commit Bot 2019-06-09 04:55:36 PDT
All reviewed patches have been landed.  Closing bug.
Comment 35 Radar WebKit Bug Importer 2019-06-09 04:57:30 PDT
<rdar://problem/51560580>