Created attachment 295689 [details] Patch

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().

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()?

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)

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.

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.

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

(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

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

(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?