Bug 99983 - XMLHttpRequest Content-Type should be taken from Blob type
: XMLHttpRequest Content-Type should be taken from Blob type
Status: RESOLVED FIXED
: WebKit
XML
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
: 100927 104352
: 11049
  Show dependency treegraph
 
Reported: 2012-10-22 05:29 PST by
Modified: 2012-12-07 01:07 PST (History)


Attachments
Patch 1 (4.22 KB, patch)
2012-10-22 06:00 PST, Alexander Shalamov
ap: review-
ap: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Patch 2 (8.75 KB, patch)
2012-10-26 04:01 PST, Alexander Shalamov
ap: review-
buildbot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Patch 3 (12.22 KB, patch)
2012-11-29 05:35 PST, Alexander Shalamov
no flags Review Patch | Details | Formatted Diff | Diff
Patch 4 (12.23 KB, patch)
2012-11-29 05:42 PST, Alexander Shalamov
ap: review+
buildbot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Patch 5 (13.45 KB, patch)
2012-12-03 06:20 PST, Alexander Shalamov
buildbot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Patch 6 (12.84 KB, patch)
2012-12-05 06:07 PST, Alexander Shalamov
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-10-22 05:29:46 PST
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
------- Comment #1 From 2012-10-22 06:00:06 PST -------
Created an attachment (id=169890) [details]
Patch 1

- Added layout test for XHR::send(Blob)
- Set Content-Type header for XHR if Blob object has type
------- Comment #2 From 2012-10-22 09:49:34 PST -------
(From update of attachment 169890 [details])
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).
------- Comment #3 From 2012-10-23 06:25:15 PST -------
(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)
------- Comment #4 From 2012-10-23 09:13:14 PST -------
> 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 #5 From 2012-10-26 04:01:15 PST -------
Created an attachment (id=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 #6 From 2012-10-26 06:11:15 PST -------
(From update of attachment 170880 [details])
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 #7 From 2012-10-26 16:06:13 PST -------
(From update of attachment 170880 [details])
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.
------- Comment #8 From 2012-11-29 05:35:12 PST -------
Created an attachment (id=176711) [details]
Patch 3

- rebased to master
- use isValidContentType to validate blob mime type
------- Comment #9 From 2012-11-29 05:39:46 PST -------
(From update of attachment 176711 [details])
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.
------- Comment #10 From 2012-11-29 05:42:35 PST -------
Created an attachment (id=176712) [details]
Patch 4

- added {}
------- Comment #11 From 2012-11-30 16:16:18 PST -------
(From update of attachment 176712 [details])
Attachment 176712 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15055592
------- Comment #12 From 2012-11-30 16:59:22 PST -------
(From update of attachment 176712 [details])
I'm not happy about the test, but OK.

You'll need to add a file that implements isValidContentType to the Windows project.
------- Comment #13 From 2012-12-03 06:20:49 PST -------
Created an attachment (id=177242) [details]
Patch 5

- added WebCore::isValidContentType(const WTF::String&) to symbols file.
------- Comment #14 From 2012-12-03 10:04:34 PST -------
(From update of attachment 177242 [details])
Attachment 177242 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15117438
------- Comment #15 From 2012-12-05 06:07:16 PST -------
Created an attachment (id=177736) [details]
Patch 6

- added ParsedContentType to visual studio project file
------- Comment #16 From 2012-12-06 14:33:43 PST -------
(From update of attachment 177736 [details])
Clearing flags on attachment: 177736

Committed r136893: <http://trac.webkit.org/changeset/136893>
------- Comment #17 From 2012-12-06 14:33:48 PST -------
All reviewed patches have been landed.  Closing bug.