XMLHttpRequest::setRequestHeader currently validates header names and values. This validation could be tighten for header values as illustrated by http://w3c-test.org/web-platform-tests/master/XMLHttpRequest/setrequestheader-bogus-value.htm According RFC2616, header values should be encoded as ISO-8859-1 and should not contain any control character except whitespaces (CR, LF, SPACE and TAB)
Created attachment 223839 [details] Patch
Comment on attachment 223839 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=223839&action=review > Source/WebCore/platform/network/HTTPParsers.cpp:105 > +bool isValidHTTPHeaderValue(const String& characters) I don’t think “characters” is a good name for a string. Maybe “string” or “value”. Maybe make this take a StringView instead of a const String&. > Source/WebCore/platform/network/HTTPParsers.cpp:116 > + // FIXME: Check all constraints in one loop > + if (!characters.containsOnlyLatin1() || characters.contains('\r') || characters.contains('\n')) > + return false; > + // FIXME: Add validation of encoding rules defined by RFC 2047. > + for (unsigned i = 0; i < characters.length(); ++i) { > + UChar c = characters[i]; > + // Check that c is not a control character except if whitespace (reduced to 0x09 in that context) > + if (c <= 0x1F && c != 0x09) > + return false; > + } This seems like a strange way to write this. Why have four different loops here? I would write: unsigned length = value.length(); for (unsigned i = 0; i < length; ++i) { UChar character = value[i]; if (character != '\t' && (character < 0x20 || character > 0x7F)) return false; } return true; If you took a string view it could be: for (UChar character : value.codeunits()) { if (character != '\t' && (character < 0x20 || character > 0x7F)) return false; } No need for containsOnlyLatin1, or a special check for \r or \n.
(In reply to comment #2) > (From update of attachment 223839 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=223839&action=review > > > Source/WebCore/platform/network/HTTPParsers.cpp:105 > > +bool isValidHTTPHeaderValue(const String& characters) > > I don’t think “characters” is a good name for a string. Maybe “string” or “value”. Maybe make this take a StringView instead of a const String&. > > > Source/WebCore/platform/network/HTTPParsers.cpp:116 > > + // FIXME: Check all constraints in one loop > > + if (!characters.containsOnlyLatin1() || characters.contains('\r') || characters.contains('\n')) > > + return false; > > + // FIXME: Add validation of encoding rules defined by RFC 2047. > > + for (unsigned i = 0; i < characters.length(); ++i) { > > + UChar c = characters[i]; > > + // Check that c is not a control character except if whitespace (reduced to 0x09 in that context) > > + if (c <= 0x1F && c != 0x09) > > + return false; > > + } > > This seems like a strange way to write this. Why have four different loops here? I would write: > > unsigned length = value.length(); > for (unsigned i = 0; i < length; ++i) { > UChar character = value[i]; > if (character != '\t' && (character < 0x20 || character > 0x7F)) > return false; > } > return true; > > If you took a string view it could be: > > for (UChar character : value.codeunits()) { > if (character != '\t' && (character < 0x20 || character > 0x7F)) > return false; > } > > No need for containsOnlyLatin1, or a special check for \r or \n. Thanks for the review. IIRC, I wrote it this way to easily align with the spec rules. Reading it again, I agree with your suggestion. Also I just saw RFC2616 was recently obsolete by RFC7230, which may have an impact on this patch.
Created attachment 239942 [details] Updated according comments and RFC7230
Comment on attachment 239942 [details] Updated according comments and RFC7230 View in context: https://bugs.webkit.org/attachment.cgi?id=239942&action=review Looks OK, but there is a bug here and the test case has insufficient coverage. > Source/WebCore/platform/network/HTTPParsers.cpp:111 > + c = value[value.length()]; This will always return 0, since it’s trying to fetch a character past the end of the string. > LayoutTests/http/tests/xmlhttprequest/set-bad-headervalue.html:24 > + try_value("t\rt", true) Coverage of this test is insufficient. I don’t see any cases checking the rules about the first and last characters not being spaces or tabs. If we had cases checking for that we would have noticed the bug I mentioned above.
Created attachment 240150 [details] fixing bug and improving test coverage
Comment on attachment 240150 [details] fixing bug and improving test coverage Clearing flags on attachment: 240150 Committed r174920: <http://trac.webkit.org/changeset/174920>
All reviewed patches have been landed. Closing bug.
I can see that the changes for this have tightened the header value validation more than what RFC 7230 suggests. If you set the value to a single SPACE character it is reported as an invalid header value. RFC 7320 does not suggest this check should be done. You can check this by adding try_value(" ") to the test harness. You will see that this test passes (i.e. it throws an exception) but accordinging to RFC 7320 it shouldn't since a single SPACE character is a valid value. I am reporting this since we utilites the ablility to set the "Content-Type" to a space character and these changes have broken our site. Should I raise a new bug?
(In reply to comment #9) > I can see that the changes for this have tightened the header value > validation more than what RFC 7230 suggests. If you set the value to a > single SPACE character it is reported as an invalid header value. RFC 7320 > does not suggest this check should be done. You can check this by adding > try_value(" ") to the test harness. You will see that this test passes (i.e. > it throws an exception) but accordinging to RFC 7320 it shouldn't since a > single SPACE character is a valid value. > > I am reporting this since we utilites the ablility to set the "Content-Type" > to a space character and these changes have broken our site. > > Should I raise a new bug? Hi. Yes, please file a new bug. Can you CC me, as well as ap and darin? Out of curiosity, can you explain why setting Content-Type to " "?
Hi, We are uploading files to a service which requires the content-type to be set. Any value will work. We are not 100% of their MimeType so we set it to a space character and the system we are uploading to doesn't seem to care about the validity but does allow the upload to happen. I will file the bug and CC those you mentioned. Thanks
(In reply to comment #11) > Hi, > > We are uploading files to a service which requires the content-type to be > set. Any value will work. We are not 100% of their MimeType so we set it to > a space character and the system we are uploading to doesn't seem to care > about the validity but does allow the upload to happen. > > I will file the bug and CC those you mentioned. > > Thanks Thanks for filing bug 147445