WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
99983
XMLHttpRequest Content-Type should be taken from Blob type
https://bugs.webkit.org/show_bug.cgi?id=99983
Summary
XMLHttpRequest Content-Type should be taken from Blob type
Alexander Shalamov
Reported
2012-10-22 05:29:46 PDT
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
Attachments
Patch 1
(4.22 KB, patch)
2012-10-22 06:00 PDT
,
Alexander Shalamov
ap
: review-
ap
: commit-queue-
Details
Formatted Diff
Diff
Patch 2
(8.75 KB, patch)
2012-10-26 04:01 PDT
,
Alexander Shalamov
ap
: review-
buildbot
: commit-queue-
Details
Formatted Diff
Diff
Patch 3
(12.22 KB, patch)
2012-11-29 05:35 PST
,
Alexander Shalamov
no flags
Details
Formatted Diff
Diff
Patch 4
(12.23 KB, patch)
2012-11-29 05:42 PST
,
Alexander Shalamov
ap
: review+
buildbot
: commit-queue-
Details
Formatted Diff
Diff
Patch 5
(13.45 KB, patch)
2012-12-03 06:20 PST
,
Alexander Shalamov
buildbot
: commit-queue-
Details
Formatted Diff
Diff
Patch 6
(12.84 KB, patch)
2012-12-05 06:07 PST
,
Alexander Shalamov
no flags
Details
Formatted Diff
Diff
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
Alexander Shalamov
Comment 1
2012-10-22 06:00:06 PDT
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 2
2012-10-22 09:49:34 PDT
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
Comment 3
2012-10-23 06:25:15 PDT
(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
Comment 4
2012-10-23 09:13:14 PDT
> 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
Comment 5
2012-10-26 04:01:15 PDT
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 6
2012-10-26 06:11:15 PDT
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 7
2012-10-26 16:06:13 PDT
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
Comment 8
2012-11-29 05:35:12 PST
Created
attachment 176711
[details]
Patch 3 - rebased to master - use isValidContentType to validate blob mime type
Chris Dumez
Comment 9
2012-11-29 05:39:46 PST
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
Comment 10
2012-11-29 05:42:35 PST
Created
attachment 176712
[details]
Patch 4 - added {}
Build Bot
Comment 11
2012-11-30 16:16:18 PST
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 12
2012-11-30 16:59:22 PST
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
Comment 13
2012-12-03 06:20:49 PST
Created
attachment 177242
[details]
Patch 5 - added WebCore::isValidContentType(const WTF::String&) to symbols file.
Build Bot
Comment 14
2012-12-03 10:04:34 PST
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
Comment 15
2012-12-05 06:07:16 PST
Created
attachment 177736
[details]
Patch 6 - added ParsedContentType to visual studio project file
WebKit Review Bot
Comment 16
2012-12-06 14:33:43 PST
Comment on
attachment 177736
[details]
Patch 6 Clearing flags on attachment: 177736 Committed
r136893
: <
http://trac.webkit.org/changeset/136893
>
WebKit Review Bot
Comment 17
2012-12-06 14:33:48 PST
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug