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?

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.