Bug 165178 – Require preflight for non-standard CORS-safelisted request headers Accept, Accept-Language, and Content-Language

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[John Wilander]

Fetch currently only restricts the header Content-Type for simple requests: https://fetch.spec.whatwg.org/#cors-safelisted-request-header

This means simple CORS requests can send unexpected characters in Accept, Accept-Language, and Content-Language header values.

RFC 7231 implies restrictions on these header values:
Accept https://tools.ietf.org/html/rfc7231#section-5.3.2
Accept-Language https://tools.ietf.org/html/rfc7231#section-5.3.5
Content-Language https://tools.ietf.org/html/rfc7231#section-3.1.3.2

As per discussions in the W3C WebAppSec group we should try to restrict these header values to help protect servers that do not expect simple CORS requests.

Non-standard header values should trigger a preflight and require the headers to be whitelisted in the response's Access-Control-Allow-Headers.

[John Wilander]

Created attachment 295689 [details]
Patch

[John Wilander]

rdar://problem/18792250

[Daniel Bates]

Comment on attachment 295689 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +

I know that you explain the motivation behind this change in comment 0. I suggest that we also repeat this reasoning in the ChangeLog description so as to avoid the need for a person to visit the bug to understand the motivation for this change.

> Source/WebCore/ChangeLog:13
> +            Now makes a call to WebCore:: isValidAcceptHeaderValue for Accept headers

"WebCore:: isValidAcceptHeaderValue" => "WebCore::isValidAcceptHeaderValue()"

> Source/WebCore/ChangeLog:14
> +            and WebCore:: isValidLanguageHeaderValue for Accept-Language and

"WebCore:: isValidLanguageHeaderValue" => "WebCore::isValidLanguageHeaderValue()"

> Source/WebCore/ChangeLog:28
> +            Now makes a call to WebCore:: isValidAcceptHeaderValue for Accept headers

"WebCore:: isValidAcceptHeaderValue" => "WebCore::isValidAcceptHeaderValue()"

> Source/WebCore/ChangeLog:29
> +            and WebCore:: isValidLanguageHeaderValue for Accept-Language and

"WebCore:: isValidLanguageHeaderValue" => "WebCore::isValidLanguageHeaderValue()"

> Source/WebCore/platform/Language.cpp:200
> +    UChar c;

No need to follow C89 semantics and declare all variables up-front and we are not returning this value. Please declare/define this variable where it is initialized on line 202.

> Source/WebCore/platform/Language.cpp:211
> +    for (unsigned i = 0; i < value.length(); ++i) {
> +        c = value[i];
> +        if (!isASCIIAlphanumeric(c)
> +            && c != ' '
> +            && c != '*'
> +            && c != '-'
> +            && c != '.'
> +            && c != ';'
> +            && c != '=')
> +            return false;
> +    }

It tends to be harder to reason about negation. I would write this as:

for (unsigned i = 0; i < value.length(); ++i) {
    UChar c = value[i];
    if (isASCIIAlphanumeric(c) || c == ' ' || c == '*' || c == '-' || c == '.' || c == ';' || c == '=')
        continue;
    return false;
}
return true;

> Source/WebCore/platform/network/HTTPParsers.cpp:133
> +    UChar c;

No need to follow C89 semantics and declare all variables up-front and we are not returning this value. Please declare/define this variable where it is initialized on line 135.

> Source/WebCore/platform/network/HTTPParsers.cpp:146
> +    for (unsigned i = 0; i < value.length(); ++i) {
> +        c = value[i];
> +        if (!isASCIIAlphanumeric(c)
> +            && c != ' '
> +            && c != '*'
> +            && c != '.'
> +            && c != '/'
> +            && c != ';'
> +            && c != '=')
> +            return false;
> +    }
> +    
> +    return true;

I would use a similar style as mentioned in comment in isValidLanguageHeaderValue().

[Daniel Bates]

Comment on attachment 295689 [details]
Patch

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

> Source/WebCore/platform/Language.cpp:193
> +bool isValidLanguageHeaderValue(const String& value)

This file, Language.cpp, does not seem like the appropriate place for such HTTP header validation logic. How did you come to the decision to put such logic here as opposed to HTTPParsers.cpp as you did for isValidAcceptHeaderValue()?

[Daniel Bates]

Comment on attachment 295689 [details]
Patch

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

> Source/WebCore/platform/Language.cpp:215
> +    return true;

i assume an empty string is valid? (I haven't read the referenced RFCs)

> Source/WebCore/platform/network/HTTPParsers.cpp:131
> +bool isValidAcceptHeaderValue(const String& value)

I assume an empty string is valid? (I haven't read the referenced RFCs)

[youenn fablet]

Comment on attachment 295689 [details]
Patch

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

>> Source/WebCore/ChangeLog:8
>> +
> 
> I know that you explain the motivation behind this change in comment 0. I suggest that we also repeat this reasoning in the ChangeLog description so as to avoid the need for a person to visit the bug to understand the motivation for this change.

AFAIK, this is not yet part of the fetch specification. There was some push back IIRC.
Would you be able to give a link to webapp-sec WG discussions?
Do you know the status on other browsers?

Fetch no-cors mode allows fetch API to send cross-origin requests without any preflight.
Even with that change, servers will need to protect themselves from browser requests containing those header values.

>> Source/WebCore/platform/network/HTTPParsers.cpp:146
>> +    return true;
> 
> I would use a similar style as mentioned in comment in isValidLanguageHeaderValue().

It is simpler to group isValidAcceptHeaderValue and isValidLanguageHeaderValue together.
They also are the same atm.

> LayoutTests/http/tests/xmlhttprequest/cors-non-standard-safelisted-headers-should-trigger-preflight.html:9
> +<!-- https://fetch.spec.whatwg.org/#cors-safelisted-request-header -->

When fetch spec will get updated, this link should be relevant to those tests but currently it is not.
These tests would be a good contribution to W3C web-platform-tests if they were testharness.js based.
That may also help adoption of that CORS change.

If you want to update the tests accordingly, you might want to consider promise_test and fetch API, which might be more handy as well.

[John Wilander]

Thanks for your reviews, Dan and Youenn! Comments inline below.

(In reply to comment #3)
> Comment on attachment 295689 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=295689&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +
> 
> I know that you explain the motivation behind this change in comment 0. I
> suggest that we also repeat this reasoning in the ChangeLog description so
> as to avoid the need for a person to visit the bug to understand the
> motivation for this change.

Agreed.

> > Source/WebCore/ChangeLog:13
> > +            Now makes a call to WebCore:: isValidAcceptHeaderValue for Accept headers
> 
> "WebCore:: isValidAcceptHeaderValue" => "WebCore::isValidAcceptHeaderValue()"
> 
> > Source/WebCore/ChangeLog:14
> > +            and WebCore:: isValidLanguageHeaderValue for Accept-Language and
> 
> "WebCore:: isValidLanguageHeaderValue" =>
> "WebCore::isValidLanguageHeaderValue()"
> 
> > Source/WebCore/ChangeLog:28
> > +            Now makes a call to WebCore:: isValidAcceptHeaderValue for Accept headers
> 
> "WebCore:: isValidAcceptHeaderValue" => "WebCore::isValidAcceptHeaderValue()"
> 
> > Source/WebCore/ChangeLog:29
> > +            and WebCore:: isValidLanguageHeaderValue for Accept-Language and
> 
> "WebCore:: isValidLanguageHeaderValue" =>
> "WebCore::isValidLanguageHeaderValue()"

Looks like my text editor is playing tricks on me. Will fix.

> > Source/WebCore/platform/Language.cpp:200
> > +    UChar c;
> 
> No need to follow C89 semantics and declare all variables up-front and we
> are not returning this value. Please declare/define this variable where it
> is initialized on line 202.
> 
> > Source/WebCore/platform/Language.cpp:211
> > +    for (unsigned i = 0; i < value.length(); ++i) {
> > +        c = value[i];
> > +        if (!isASCIIAlphanumeric(c)
> > +            && c != ' '
> > +            && c != '*'
> > +            && c != '-'
> > +            && c != '.'
> > +            && c != ';'
> > +            && c != '=')
> > +            return false;
> > +    }

Will fix.

> It tends to be harder to reason about negation. I would write this as:
> 
> for (unsigned i = 0; i < value.length(); ++i) {
>     UChar c = value[i];
>     if (isASCIIAlphanumeric(c) || c == ' ' || c == '*' || c == '-' || c ==
> '.' || c == ';' || c == '=')
>         continue;
>     return false;
> }
> return true;

Agreed.

> > Source/WebCore/platform/network/HTTPParsers.cpp:133
> > +    UChar c;
> 
> No need to follow C89 semantics and declare all variables up-front and we
> are not returning this value. Please declare/define this variable where it
> is initialized on line 135.

Will fix.

> > Source/WebCore/platform/network/HTTPParsers.cpp:146
> > +    for (unsigned i = 0; i < value.length(); ++i) {
> > +        c = value[i];
> > +        if (!isASCIIAlphanumeric(c)
> > +            && c != ' '
> > +            && c != '*'
> > +            && c != '.'
> > +            && c != '/'
> > +            && c != ';'
> > +            && c != '=')
> > +            return false;
> > +    }
> > +    
> > +    return true;
> 
> I would use a similar style as mentioned in comment in
> isValidLanguageHeaderValue().

Agreed.


(In reply to comment #4)
> Comment on attachment 295689 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=295689&action=review
> 
> > Source/WebCore/platform/Language.cpp:193
> > +bool isValidLanguageHeaderValue(const String& value)
> 
> This file, Language.cpp, does not seem like the appropriate place for such
> HTTP header validation logic. How did you come to the decision to put such
> logic here as opposed to HTTPParsers.cpp as you did for
> isValidAcceptHeaderValue()?

It is the file that deals with language codes. When working on this patch I came across https://bugs.webkit.org/show_bug.cgi?id=123926 and I have a follow-up patch for Language.cpp and VideoTrack.cpp that takes us closer to full validation of language codes/tags. But I can absolutely split language validation into a basic header check in HTTPParsers.cpp and deeper things in Language.cpp.


(In reply to comment #6)
> Comment on attachment 295689 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=295689&action=review
> 
> >> Source/WebCore/ChangeLog:8
> >> +
> > 
> > I know that you explain the motivation behind this change in comment 0. I suggest that we also repeat this reasoning in the ChangeLog description so as to avoid the need for a person to visit the bug to understand the motivation for this change.
> 
> AFAIK, this is not yet part of the fetch specification.

Fetch bug: https://github.com/whatwg/fetch/issues/382

> There was some push back IIRC.

The pushback was primarily from a former Mozilla engineer Jonas Sicking, in this thread: https://github.com/whatwg/fetch/issues/313

It was brought up again at the W3C WebAppSec phone meeting and since Sicking has left Mozilla they wanted to go through it again.

> Would you be able to give a link to webapp-sec WG discussions?

The notes from the phone meeting on 11/16 are not yet available:
https://www.w3.org/2011/webappsec/Minutes.html

> Do you know the status on other browsers?

At the 11/16 meeting Google was in favor of this change but we agreed we need ample beta testing. In the case of WebKit it will be through trunk and Safari Technology Preview.

It should be noted here that we're not blocking such request headers, merely making them trigger a preflight. And it is odd that three of the four safe-listed request headers for simple CORS do not have any restrictions beyond field-content token production.

> Fetch no-cors mode allows fetch API to send cross-origin requests without
> any preflight.
> Even with that change, servers will need to protect themselves from browser
> requests containing those header values.

Yes. But the danger here lies in cross-origin requests.

> >> Source/WebCore/platform/network/HTTPParsers.cpp:146
> >> +    return true;
> > 
> > I would use a similar style as mentioned in comment in isValidLanguageHeaderValue().
> 
> It is simpler to group isValidAcceptHeaderValue and
> isValidLanguageHeaderValue together.
> They also are the same atm.

Actually, they are not the same which is why they are separate functions. Accept is allowed to use '/' and Accept-Language and Content-Language are allowed to use '-'.

> > LayoutTests/http/tests/xmlhttprequest/cors-non-standard-safelisted-headers-should-trigger-preflight.html:9
> > +<!-- https://fetch.spec.whatwg.org/#cors-safelisted-request-header -->
> 
> When fetch spec will get updated, this link should be relevant to those
> tests but currently it is not.

I put it there to refer to which headers are safe-listed. But I can remove it.

> These tests would be a good contribution to W3C web-platform-tests if they
> were testharness.js based.
> That may also help adoption of that CORS change.

Agreed. Is it OK if I file a follow-up bug for that? I'd like to get field testing and a conversation with the other browsers going.

> If you want to update the tests accordingly, you might want to consider
> promise_test and fetch API, which might be more handy as well.

Thanks, I look at that when it's time to harmonize the tests with W3C.

[youenn fablet]

> > Fetch no-cors mode allows fetch API to send cross-origin requests without
> > any preflight.
> > Even with that change, servers will need to protect themselves from browser
> > requests containing those header values.
> 
> Yes. But the danger here lies in cross-origin requests.

That is not very clear to me.
Do you mean that the danger is in cross-origin requests in fetch cors mode and not in cross-origin requests in fetch no-cors mode?

> > > LayoutTests/http/tests/xmlhttprequest/cors-non-standard-safelisted-headers-should-trigger-preflight.html:9
> > > +<!-- https://fetch.spec.whatwg.org/#cors-safelisted-request-header -->
> > 
> > When fetch spec will get updated, this link should be relevant to those
> > tests but currently it is not.
> 
> I put it there to refer to which headers are safe-listed. But I can remove
> it.

I guess when fetch spec will get updated, the pointer will be the same.
Let's keep it then.
You could make it as <meta name="help" href="https://fetch.spec.whatwg.org/#cors-safelisted-request-header">

> > These tests would be a good contribution to W3C web-platform-tests if they
> > were testharness.js based.
> > That may also help adoption of that CORS change.
> 
> Agreed. Is it OK if I file a follow-up bug for that? I'd like to get field
> testing and a conversation with the other browsers going.

np

[John Wilander]

(In reply to comment #8)
> > > Fetch no-cors mode allows fetch API to send cross-origin requests without
> > > any preflight.
> > > Even with that change, servers will need to protect themselves from browser
> > > requests containing those header values.
> > 
> > Yes. But the danger here lies in cross-origin requests.
> 
> That is not very clear to me.
> Do you mean that the danger is in cross-origin requests in fetch cors mode
> and not in cross-origin requests in fetch no-cors mode?

Sorry, I got confused with regular, same-origin Fetch. You know the two CORS modes better than I do so please advice. :)

The isSimpleHeader() check is done for for no-cors mode too. FetchHeaders.cpp will deny the header write for no-cors:

static ExceptionOr<bool> canWriteHeader(const String& name, const String& value, FetchHeaders::Guard guard)
{
    ...
    if (guard == FetchHeaders::Guard::RequestNoCors && !isSimpleHeader(name, value))
        return false;

> > > > LayoutTests/http/tests/xmlhttprequest/cors-non-standard-safelisted-headers-should-trigger-preflight.html:9
> > > > +<!-- https://fetch.spec.whatwg.org/#cors-safelisted-request-header -->
> > > 
> > > When fetch spec will get updated, this link should be relevant to those
> > > tests but currently it is not.
> > 
> > I put it there to refer to which headers are safe-listed. But I can remove
> > it.
> 
> I guess when fetch spec will get updated, the pointer will be the same.
> Let's keep it then.
> You could make it as <meta name="help"
> href="https://fetch.spec.whatwg.org/#cors-safelisted-request-header">
> 
> > > These tests would be a good contribution to W3C web-platform-tests if they
> > > were testharness.js based.
> > > That may also help adoption of that CORS change.
> > 
> > Agreed. Is it OK if I file a follow-up bug for that? I'd like to get field
> > testing and a conversation with the other browsers going.
> 
> np

[youenn fablet]

> The isSimpleHeader() check is done for for no-cors mode too.
> FetchHeaders.cpp will deny the header write for no-cors:

We would then have requests emitted without Accept/Accept-Language/Content-Language headers.
We need tests on that before shipping that change.
We probably want some additional discussion on GitHub fetch as well.

[youenn fablet]

(In reply to comment #10)
> > The isSimpleHeader() check is done for for no-cors mode too.
> > FetchHeaders.cpp will deny the header write for no-cors:
> 
> We would then have requests emitted without
> Accept/Accept-Language/Content-Language headers.

Or with default values defined by lower network layers?

[John Wilander]

(In reply to comment #10)
> > The isSimpleHeader() check is done for for no-cors mode too.
> > FetchHeaders.cpp will deny the header write for no-cors:
> 
> We would then have requests emitted without
> Accept/Accept-Language/Content-Language headers.
> We need tests on that before shipping that change.
> We probably want some additional discussion on GitHub fetch as well.

Yes, I saw that in my manual testing.

Are you saying I should add Fetch no-cors tests now or we need to do it before shipping Fetch?

[John Wilander]

(In reply to comment #10)
> > The isSimpleHeader() check is done for for no-cors mode too.
> > FetchHeaders.cpp will deny the header write for no-cors:
> 
> We would then have requests emitted without
> Accept/Accept-Language/Content-Language headers.
> We need tests on that before shipping that change.

I looked into this today. There's no easy way for the server to signal whether it received a particular request header or not since the response is opaque.

I may be able to get it working through a conditional redirect on the server and a { mode: "no-cors", redirect: "error" } setting in the client. Does that sound reasonable to you? If not, how would you structure this test?

[John Wilander]

Created attachment 295863 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 295863 [details]
Patch

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

> LayoutTests/ChangeLog:10
> +        Fetch currently only restricts the header Content-Type for simple requests:
> +        https://fetch.spec.whatwg.org/#cors-safelisted-request-header

This patch seems to be adding an implementation that is parallel to one in XMLHttprequest (see e.g. isForbiddenRequestHeader and checks in XMLHttpRequest::setRequestHeader).

Is it right to make such changes at a different level? The effect seems very confusing - sometimes misuse results in an exception, other times it results in preflight, depending on the exact API used.

[John Wilander]

(In reply to comment #15)
> Comment on attachment 295863 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=295863&action=review
> 
> > LayoutTests/ChangeLog:10
> > +        Fetch currently only restricts the header Content-Type for simple requests:
> > +        https://fetch.spec.whatwg.org/#cors-safelisted-request-header
> 
> This patch seems to be adding an implementation that is parallel to one in
> XMLHttprequest (see e.g. isForbiddenRequestHeader and checks in
> XMLHttpRequest::setRequestHeader).
> 
> Is it right to make such changes at a different level? The effect seems very
> confusing - sometimes misuse results in an exception, other times it results
> in preflight, depending on the exact API used.

It is not a parallel to isForbiddenRequestHeader() or the checks in setRequestHeader(). These headers are explicitly allowed by spec and even "safe listed" for simple CORS requests. I did consider an alternate naming of the new functions but decided that this is the best way, i.e.:
1) The new functions check the header values according to RFC 7231, which is why I think their names are correct.
2) The new functions are (currently) called when we check if a cross-origin request should be handled as simple.
3) The decision in the case of CORS and Fetch in cors mode is to trigger a preflight. The decision in the case of Fetch in non-cors mode is to not allow the header write.
4) If the server responds with Access-Control-Allow-Headers and whitelists the header(s) in question, we will send it/them.

[youenn fablet]

(In reply to comment #13)
> (In reply to comment #10)
> > > The isSimpleHeader() check is done for for no-cors mode too.
> > > FetchHeaders.cpp will deny the header write for no-cors:
> > 
> > We would then have requests emitted without
> > Accept/Accept-Language/Content-Language headers.
> > We need tests on that before shipping that change.
> 
> I looked into this today. There's no easy way for the server to signal
> whether it received a particular request header or not since the response is
> opaque.
> 
> I may be able to get it working through a conditional redirect on the server
> and a { mode: "no-cors", redirect: "error" } setting in the client. Does
> that sound reasonable to you? If not, how would you structure this test?

One approach is to do keep this information in the server:
- a first request is the no-cors one and sets the state
- a second request will retrieve the state (doing a same origin or cors request)

The infrastructure is already available in web-platform-tests.
See for instance LayoutTests/imported/w3c/web-platform-tests/fetch/api/resources/preflight.py and LayoutTests/imported/w3c/web-platform-tests/fetch/api/cors/cors-redirect-preflight.js for examples.

[John Wilander]

(In reply to comment #17)
> (In reply to comment #13)
> > (In reply to comment #10)
> > > > The isSimpleHeader() check is done for for no-cors mode too.
> > > > FetchHeaders.cpp will deny the header write for no-cors:
> > > 
> > > We would then have requests emitted without
> > > Accept/Accept-Language/Content-Language headers.
> > > We need tests on that before shipping that change.
> > 
> > I looked into this today. There's no easy way for the server to signal
> > whether it received a particular request header or not since the response is
> > opaque.
> > 
> > I may be able to get it working through a conditional redirect on the server
> > and a { mode: "no-cors", redirect: "error" } setting in the client. Does
> > that sound reasonable to you? If not, how would you structure this test?
> 
> One approach is to do keep this information in the server:
> - a first request is the no-cors one and sets the state
> - a second request will retrieve the state (doing a same origin or cors
> request)
> 
> The infrastructure is already available in web-platform-tests.
> See for instance
> LayoutTests/imported/w3c/web-platform-tests/fetch/api/resources/preflight.py
> and
> LayoutTests/imported/w3c/web-platform-tests/fetch/api/cors/cors-redirect-
> preflight.js for examples.

I have a test stub but LayoutTests/http/tests/fetch/ is not configured to run a Python server yet. Can we get this patch landed without the Fetch no-cors test? We need to get it into Safari Technology Preview to assess whether the change is possible to ship at all.

[youenn fablet]

Comment on attachment 295863 [details]
Patch

Let's try it then.

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

> Source/WebCore/platform/network/HTTPParsers.cpp:856
>  {

This routine is the same as isOnAccessControlSimpleRequestHeaderWhitelist.
Would you be able when landing to remove it and replace it with isCrossOriginSafeRequestHeader?
isSimpleHeader is also sharing some functionality with it.
Can we unify all three when landing this patch?

>>> LayoutTests/ChangeLog:10
>>> +        https://fetch.spec.whatwg.org/#cors-safelisted-request-header
>> 
>> This patch seems to be adding an implementation that is parallel to one in XMLHttprequest (see e.g. isForbiddenRequestHeader and checks in XMLHttpRequest::setRequestHeader).
>> 
>> Is it right to make such changes at a different level? The effect seems very confusing - sometimes misuse results in an exception, other times it results in preflight, depending on the exact API used.
> 
> It is not a parallel to isForbiddenRequestHeader() or the checks in setRequestHeader(). These headers are explicitly allowed by spec and even "safe listed" for simple CORS requests. I did consider an alternate naming of the new functions but decided that this is the best way, i.e.:
> 1) The new functions check the header values according to RFC 7231, which is why I think their names are correct.
> 2) The new functions are (currently) called when we check if a cross-origin request should be handled as simple.
> 3) The decision in the case of CORS and Fetch in cors mode is to trigger a preflight. The decision in the case of Fetch in non-cors mode is to not allow the header write.
> 4) If the server responds with Access-Control-Allow-Headers and whitelists the header(s) in question, we will send it/them.

XHR should be using isForbiddenHeaderName as per the spec.
But there are some differences with isForbiddenRequestHeader.
We should work on unifying them in the future.

> LayoutTests/http/tests/xmlhttprequest/cors-non-standard-safelisted-headers-should-trigger-preflight.html:37
> +                testPassed(description);

indentation issue.

> LayoutTests/http/tests/xmlhttprequest/cors-non-standard-safelisted-headers-should-trigger-preflight.html:42
> +                } else {

one liners do not need {}

> LayoutTests/http/tests/xmlhttprequest/cors-non-standard-safelisted-headers-should-trigger-preflight.html:48
> +    var abnormalSimpleCorsHeaderValue = "() { :;};"

Can we have a test containing all allowed non-alphanumeric characters for each one of the three headers?

> LayoutTests/http/tests/xmlhttprequest/cors-non-standard-safelisted-headers-should-trigger-preflight.html:130
> +        if(invocation) {

space after the if.
Also why do we need that if test?
The "invocation"variable seems a bit strange, xhr would be more usual I guess.

[youenn fablet]

> I have a test stub but LayoutTests/http/tests/fetch/ is not configured to
> run a Python server yet. Can we get this patch landed without the Fetch
> no-cors test? We need to get it into Safari Technology Preview to assess
> whether the change is possible to ship at all.

Given we want interop here and work jointly with other browsers, it makes sense to me to upload those tests/create new tests to web-platform-tests.
They should also be run in LayoutTests/imported/w3c/web-platform-tests as all tests in that folders are run behind wpt python server

[John Wilander]

Created attachment 295966 [details]
Patch

[John Wilander]

Comment on attachment 295966 [details]
Patch

I have one more change to make.

[John Wilander]

Created attachment 295967 [details]
Patch

[John Wilander]

(In reply to comment #19)
> Comment on attachment 295863 [details]
> Patch
> 
> Let's try it then.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=295863&action=review
> 
> > Source/WebCore/platform/network/HTTPParsers.cpp:856
> >  {
> 
> This routine is the same as isOnAccessControlSimpleRequestHeaderWhitelist.
> Would you be able when landing to remove it and replace it with
> isCrossOriginSafeRequestHeader?
> isSimpleHeader is also sharing some functionality with it.
> Can we unify all three when landing this patch?

I agree. Done.

> >>> LayoutTests/ChangeLog:10
> >>> +        https://fetch.spec.whatwg.org/#cors-safelisted-request-header
> >> 
> >> This patch seems to be adding an implementation that is parallel to one in XMLHttprequest (see e.g. isForbiddenRequestHeader and checks in XMLHttpRequest::setRequestHeader).
> >> 
> >> Is it right to make such changes at a different level? The effect seems very confusing - sometimes misuse results in an exception, other times it results in preflight, depending on the exact API used.
> > 
> > It is not a parallel to isForbiddenRequestHeader() or the checks in setRequestHeader(). These headers are explicitly allowed by spec and even "safe listed" for simple CORS requests. I did consider an alternate naming of the new functions but decided that this is the best way, i.e.:
> > 1) The new functions check the header values according to RFC 7231, which is why I think their names are correct.
> > 2) The new functions are (currently) called when we check if a cross-origin request should be handled as simple.
> > 3) The decision in the case of CORS and Fetch in cors mode is to trigger a preflight. The decision in the case of Fetch in non-cors mode is to not allow the header write.
> > 4) If the server responds with Access-Control-Allow-Headers and whitelists the header(s) in question, we will send it/them.
> 
> XHR should be using isForbiddenHeaderName as per the spec.
> But there are some differences with isForbiddenRequestHeader.
> We should work on unifying them in the future.

Would be good, yes.

> > LayoutTests/http/tests/xmlhttprequest/cors-non-standard-safelisted-headers-should-trigger-preflight.html:37
> > +                testPassed(description);
> 
> indentation issue.

Fixed.

> > LayoutTests/http/tests/xmlhttprequest/cors-non-standard-safelisted-headers-should-trigger-preflight.html:42
> > +                } else {
> 
> one liners do not need {}

Fixed along with a set of others. I did not think the one-liner rule applied to JavaScript where I believe curly brackets are encouraged in general. Nevertheless, I removed them here.

> > LayoutTests/http/tests/xmlhttprequest/cors-non-standard-safelisted-headers-should-trigger-preflight.html:48
> > +    var abnormalSimpleCorsHeaderValue = "() { :;};"
> 
> Can we have a test containing all allowed non-alphanumeric characters for
> each one of the three headers?

Good idea. Fixed.

> > LayoutTests/http/tests/xmlhttprequest/cors-non-standard-safelisted-headers-should-trigger-preflight.html:130
> > +        if(invocation) {
> 
> space after the if.
> Also why do we need that if test?
> The "invocation"variable seems a bit strange, xhr would be more usual I
> guess.

Removed the if and renamed the variable.

Thanks for the review!

[WebKit Commit Bot]

Comment on attachment 295967 [details]
Patch

Clearing flags on attachment: 295967

Committed r209261: <http://trac.webkit.org/changeset/209261>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.