Switch to a blacklist model for restricted Accept headers in simple CORS requests

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[John Wilander]

The whitelist model in https://bugs.webkit.org/show_bug.cgi?id=165178 (and follow-up https://bugs.webkit.org/show_bug.cgi?id=165566) was too strict for Accept headers. We should move to a blacklist of delimiter character instead.

[John Wilander]

Created attachment 297616 [details]
Patch

[WebKit Commit Bot]

Attachment 297616 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/network/HTTPParsers.cpp:145:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[John Wilander]

(In reply to comment #2)
> Attachment 297616 [details] did not pass style-queue:
> 
> 
> ERROR: Source/WebCore/platform/network/HTTPParsers.cpp:145:  An else if
> statement should be written as an if statement when the prior "if" concludes
> with a return, break, continue or goto statement. 
> [readability/control_flow] [4]
> Total errors found: 1 in 5 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

This is a deliberate violation of the style rules. Please assess in review.

[John Wilander]

rdar://problem/29635131

[Alex Christensen]

Comment on attachment 297616 [details]
Patch

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

> LayoutTests/http/tests/xmlhttprequest/cors-non-standard-safelisted-headers-should-trigger-preflight.html:116
> +            headersToAdd: [{ name : "Accept", value: allDisallowedDelimiterCharactersForAcceptHeader }],

A better test would be to test that each one of the disallowed characters caused preflight.  With this test method, if you had an allowed character in the set of disallowed characters, the test would still pass.

[Alex Christensen]

Comment on attachment 297616 [details]
Patch

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

> Source/WebCore/platform/network/HTTPParsers.cpp:147
> +        else if (isDelimiterCharacter(c))

Yep, this else isn't necessary.

[John Wilander]

(In reply to comment #5)
> Comment on attachment 297616 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=297616&action=review
> 
> > LayoutTests/http/tests/xmlhttprequest/cors-non-standard-safelisted-headers-should-trigger-preflight.html:116
> > +            headersToAdd: [{ name : "Accept", value: allDisallowedDelimiterCharactersForAcceptHeader }],
> 
> A better test would be to test that each one of the disallowed characters
> caused preflight.  With this test method, if you had an allowed character in
> the set of disallowed characters, the test would still pass.

Got it. This was an update for a test Youenn requested last time over. I'll update with your suggestion.

[John Wilander]

(In reply to comment #6)
> Comment on attachment 297616 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=297616&action=review
> 
> > Source/WebCore/platform/network/HTTPParsers.cpp:147
> > +        else if (isDelimiterCharacter(c))
> 
> Yep, this else isn't necessary.

Oh, now I see what the style complaint was about. Will fix.

[John Wilander]

Created attachment 297617 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 297617 [details]
Patch

Clearing flags on attachment: 297617

Committed r210077: <http://trac.webkit.org/changeset/210077>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Anne van Kesteren]

John, I would appreciate if you had communicated this in the Fetch repository. It seems rather strange for only WebKit to enforce this and no other browser.

[Chris Dumez]

(In reply to comment #12)
> John, I would appreciate if you had communicated this in the Fetch
> repository. It seems rather strange for only WebKit to enforce this and no
> other browser.

@John, isn't this the change that's breaking Web sites too?

[John Wilander]

(In reply to comment #12)
> John, I would appreciate if you had communicated this in the Fetch
> repository. It seems rather strange for only WebKit to enforce this and no
> other browser.

Sure. First, this is not shipping to regular users in Safari. It is in WebKit Nightly, Safari Technology Preview, and of course any other user of WebKit that decides to pick up the patch.
   Our intention is to get real world feedback before we decide whether to ship in Safari or not. We've seen a couple of broken sites which is why the patch has evolved over three commits. Currently there's just one site reported to have problems and it's because it uses '_' in a language tag. We consider it reasonable to require a preflight for characters outside the header spec.

This issue was originally filed under embargo with all major browser engines fall 2014. Since the behavior is by specification and also not a security bug in the browsers themselves it was hard for us to get coordination on a change. We tried.
   When we joined W3C WebAppSec spring 2016 we brought this up at the first face-to-face meeting we attended. Minutes here: https://www.w3.org/2011/webappsec/minutes/2016-05-17-webappsec-minutes.html#item08 .
   We were asked to file a Fetch bug which we did: https://github.com/whatwg/fetch/issues/382 . The discussion got moved to https://github.com/whatwg/fetch/issues/313 where it tapered off after my comment on September 9. Jonas Sicking had left Mozilla by then which might explain why he left it hanging.
   The discussion instead moved to https://github.com/w3c/webappsec-csp/issues/115 and was brought up on the phone meeting November 16. The minutes are not yet available (https://www.w3.org/2011/webappsec/Minutes.html) but my personal notes said "Google expressed a favorable stance on this proposal. Mozilla needs to re-assess it since the engineer who made comments previously has left the company."

With all of the above and the long wait to do something about this we decided to implement and see if there is significant breakage. We will report on our findings to both Fetch and WebAppSec.

What would you like us to do meanwhile? Thanks!

[Anne van Kesteren]

John, it seems my comment was somewhat premature. I didn't realize this was only an experiment for now. I thought it was meant to ship. I also didn't know that Google might be on board. Thanks for the update.