Bug 99983

Created attachment 169890 [details] Patch 1 - Added layout test for XHR::send(Blob) - Set Content-Type header for XHR if Blob object has type

Comment on attachment 169890 [details] Patch 1 View in context: https://bugs.webkit.org/attachment.cgi?id=169890&action=review Thank you for splitting the work into smaller chunks! This one looks pretty non-controversial, and should be good to go soon. > LayoutTests/http/tests/xmlhttprequest/post-blob-content-type.html:12 > + var blob = new Blob(["Test content"], {type: "text/plain;charset=UTF-8"}); Can you please add some test cases that don't look like default charset? With text/plain, one can't help but wonder if the result here is real, or just a fallback to some default. Also, please add test cases where Blob Content-Type contains newlines of various kinds, or is otherwise invalid. You are adding a new HTTP header injection attack vector in this patch, so you should test it very carefully. > LayoutTests/http/tests/xmlhttprequest/post-blob-content-type.html:14 > + xhr.open("POST", "print-content-type.cgi", false); It would be nice to test both sync and async XHR. If you are only testing one, make it async, because that's the common case. > Source/WebCore/ChangeLog:3 > + [XMLHttpRequest] Content-Type and encoding is set incorrectly for Blob objects [] prefixes are primarily meant for platform specific bug that people working on cross-platform code can ignore. I think that some reviewers may be filtering out e-mails about patches where title starts with "[". That's not appropriate for XMLHTtpRequest, because all ports care about it. > Source/WebCore/ChangeLog:9 > + Fix XMLHttpRequest::send(Blob*) method, so that the Content-Type is set according to W3C specification. > + http://www.w3.org/TR/XMLHttpRequest/#the-send-method Do any browsers agree with the specification? > Source/WebCore/xml/XMLHttpRequest.cpp:619 > + String contentType = getRequestHeader("Content-Type"); > + const String& blobType = body->type(); This looks curious. Why is one variable a String, and another a reference? That's not dictated by function return type. > Source/WebCore/xml/XMLHttpRequest.cpp:621 > + if (!blobType.isEmpty() && contentType.isEmpty()) > + setRequestHeaderInternal("Content-Type", blobType); What does the spec say about cross-origin XHR? One presumably needs CORS headers to send POST requests with arbitrary content types cross origin. I can't tell it from memory if our code will handle this case correctly. In any case, please add a test (we usually do 127.0.0.1 vs localhost to test cross origin requests).

(In reply to comment #2) > Also, please add test cases where Blob Content-Type contains newlines of various kinds, or is otherwise invalid. You are adding a new HTTP header injection attack vector in this patch, so you should test it very carefully. I checked ports that use libsoup (gtk, efl) and invalid header value is handled in libsoup. Briefly checked chromium stack and I couldn't find any place where they check if header value is valid. Is there http header name / value sanity checking code in WebCore that I can reuse? I couldn't find anything. Also checked how mozilla and opera handle header values with newline characters: Mozilla - doesn't send request Opera - doesn't set invalid header (Behaves like like gtk / elf pors) > > LayoutTests/http/tests/xmlhttprequest/post-blob-content-type.html:14 > > + xhr.open("POST", "print-content-type.cgi", false); > > It would be nice to test both sync and async XHR. If you are only testing one, make it async, because that's the common case. I will modify test to use both. > > Source/WebCore/ChangeLog:9 > > + Fix XMLHttpRequest::send(Blob*) method, so that the Content-Type is set according to W3C specification. > > + http://www.w3.org/TR/XMLHttpRequest/#the-send-method > > Do any browsers agree with the specification? Mozilla sets content-type according to the spec. Opera doesn't set anything and they added Blob support recently in 12.10 beta. IE10 should have support, but I can't test it, since I don't have Win8 environment. > > Source/WebCore/xml/XMLHttpRequest.cpp:619 > > + String contentType = getRequestHeader("Content-Type"); > > + const String& blobType = body->type(); > > This looks curious. Why is one variable a String, and another a reference? > That's not dictated by function return type. Will make contentType const String& to avoid one copy. > > Source/WebCore/xml/XMLHttpRequest.cpp:621 > > + if (!blobType.isEmpty() && contentType.isEmpty()) > > + setRequestHeaderInternal("Content-Type", blobType); > > What does the spec say about cross-origin XHR? One presumably needs CORS headers to send POST requests with arbitrary content types cross origin. I can't tell it from memory if our code will handle this case correctly. > > In any case, please add a test (we usually do 127.0.0.1 vs localhost to test cross origin requests). I will add test for cross-origin XHR and double check spec for FileAPI (http://dev.w3.org/2006/webapi/FileAPI/#use-cases-scheme)

> Is there http header name / value sanity checking code in WebCore that I can reuse? I couldn't find anything. You could take a look at XMLHttpRequest's setRequestHeader implementation. Also, perhaps Blob itself performs the validation?

Created attachment 170880 [details] Patch 2 - Added sync and async tests - Added test for unicode characters in blob mime type (Specification doesn't say if exception should be thrown, Opera and WebKit throw exception. Mozilla doesn't.) - Added test for newline characters inside blob MIME type string - Added cross-origin request tests - Added validation check for mime type value

Comment on attachment 170880 [details] Patch 2 Attachment 170880 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14606095 New failing tests: http/tests/xmlhttprequest/post-blob-content-type.html

Comment on attachment 170880 [details] Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=170880&action=review Looks good, r- for failing test. > LayoutTests/http/tests/xmlhttprequest/post-blob-content-type-expected.txt:9 > +PASS Sync test: Blob Content-Type: "invalid/type\r\ncharset;=koi8-r". Sent Content-type: "" I'd have added separate tests for CR, LF, CRLF, and for Unicode newlines. Also, for a boundary - unexpected things may happen if the request becomes multipart. Also, quote marks. Please consider any tests you can think of - as mentioned before, we're adding a new attack vector, and should be super vigilant. > LayoutTests/http/tests/xmlhttprequest/post-blob-content-type-expected.txt:18 > +PASS Async test: Should fail. So is it a PASS or a FAIL then? :) I would prefer the test split into multiple files. That way, long-term maintenance is much easier. A series of shouldBe is best when we are checking a long list of very similar cases, but these are quite different. You could make test machinery simpler and easier to understand. Filename of a separate case is a very prominent place for a brief description, so if a test fails, you already have an idea of what went wrong simply by looking at its name. It's much easier to get rid of these "pass must fail" mysteries, too. > LayoutTests/http/tests/xmlhttprequest/post-blob-content-type.html:4 > + <script src="../../js-test-resources/js-test-pre.js"></script> No need for the relative path here - as this is an HTTP test, you can start form the root. Not a big deal - will just make it easier to move the test into a different directory if needed, for example. > LayoutTests/http/tests/xmlhttprequest/post-blob-content-type.html:116 > + if (async) xhr.onloadend = reportResult; I usually don't object against non-WebKit coding style in tests, but this one has logic complicated enough that I feel tempted to.

Created attachment 176711 [details] Patch 3 - rebased to master - use isValidContentType to validate blob mime type

Comment on attachment 176711 [details] Patch 3 View in context: https://bugs.webkit.org/attachment.cgi?id=176711&action=review > Source/WebCore/xml/XMLHttpRequest.cpp:652 > + else WebKit Coding style says to use { } for this case.

Created attachment 176712 [details] Patch 4 - added {}

Comment on attachment 176712 [details] Patch 4 Attachment 176712 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/15055592

Comment on attachment 176712 [details] Patch 4 I'm not happy about the test, but OK. You'll need to add a file that implements isValidContentType to the Windows project.

Created attachment 177242 [details] Patch 5 - added WebCore::isValidContentType(const WTF::String&) to symbols file.

Comment on attachment 177242 [details] Patch 5 Attachment 177242 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/15117438

Created attachment 177736 [details] Patch 6 - added ParsedContentType to visual studio project file

Comment on attachment 177736 [details] Patch 6 Clearing flags on attachment: 177736 Committed r136893: <http://trac.webkit.org/changeset/136893>

All reviewed patches have been landed. Closing bug.