[webkit-reviews] review denied: [Bug 99983] [XMLHttpRequest] Content-Type and encoding is set incorrectly for Blob objects : [Attachment 169890] Patch 1

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Alexander Shalamov
<alexander.shalamov at gmail.com>'s request for review:
Bug 99983: [XMLHttpRequest] Content-Type and encoding is set incorrectly for
Blob objects
https://bugs.webkit.org/show_bug.cgi?id=99983

Attachment 169890: Patch 1
https://bugs.webkit.org/attachment.cgi?id=169890&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
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).