Bug 99983 – XMLHttpRequest Content-Type should be taken from Blob type

• Component: XML
• Status: RESOLVED FIXED

[Alexander Shalamov]

When Blob object is sent using XMLHttpRequest::send method, Content-Type should be the type of the Blob object.
http://www.w3.org/TR/XMLHttpRequest/#the-send-method

[Alexander Shalamov]

Created attachment 169890 [details]
Patch 1

- Added layout test for XHR::send(Blob)
- Set Content-Type header for XHR if Blob object has type

[Alexey Proskuryakov]

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

[Alexander Shalamov]

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

[Alexey Proskuryakov]

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

[Alexander Shalamov]

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

[Build Bot]

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

[Alexey Proskuryakov]

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.

[Alexander Shalamov]

Created attachment 176711 [details]
Patch 3

- rebased to master
- use isValidContentType to validate blob mime type

[Chris Dumez]

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.

[Alexander Shalamov]

Created attachment 176712 [details]
Patch 4

- added {}

[Build Bot]

Comment on attachment 176712 [details]
Patch 4

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

[Alexey Proskuryakov]

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.

[Alexander Shalamov]

Created attachment 177242 [details]
Patch 5

- added WebCore::isValidContentType(const WTF::String&) to symbols file.

[Build Bot]

Comment on attachment 177242 [details]
Patch 5

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

[Alexander Shalamov]

Created attachment 177736 [details]
Patch 6

- added ParsedContentType to visual studio project file

[WebKit Review Bot]

Comment on attachment 177736 [details]
Patch 6

Clearing flags on attachment: 177736

Committed r136893: <http://trac.webkit.org/changeset/136893>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.