[webkit-reviews] review denied: [Bug 111380] Handle CRLF in MIME types of Blobs appended to multipart FormData : [Attachment 191509] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Victor Costan
<costan at gmail.com>'s request for review:
Bug 111380: Handle CRLF in MIME types of Blobs appended to multipart FormData
https://bugs.webkit.org/show_bug.cgi?id=111380

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

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=191509&action=review


I do not see anything in relevant specs that says how to handle invalid types
for serialization. What I suggest doing is to check what Firefox does, probably
match its behavior, and likely file a bug against XMLHttpRequest specification.


> Source/WebCore/platform/network/FormDataBuilder.cpp:82
> +static void appendStringWithoutCrLf(Vector<char>& buffer, const CString&
string)

WebKit style would be "appendStringReplacingCRLF".

> Source/WebCore/platform/network/FormDataBuilder.cpp:84
> +    unsigned length = string.length();

CString::length() returns a size_t, not an unsigned.

> Source/WebCore/platform/network/FormDataBuilder.cpp:85
> +    for (unsigned i = 0; i < length; ++i) {

So index should be size_t too.

> Source/WebCore/platform/network/FormDataBuilder.cpp:86
> +	   unsigned char c = string.data()[i];

CString::data() returns a char, not an unsigned char.

> Source/WebCore/platform/network/FormDataBuilder.cpp:87
> +	   if (c == 0x0d && i + 1 < length && string.data()[i + 1] == 0x0a) {

It is not sufficient to replace CRLF sequences. Receivers are likely to treat
isolated CR and LF bytes as line separators (they certainly do that in HTTP
headers).

> Source/WebCore/platform/network/FormDataBuilder.cpp:88
> +	       // HTTP receivers may replace any LWS with a single SP.

I don't think that HTTP rules apply here, as these are MIME headers.

Generation of MIME headers is specified by HTML5 "multipart/form-data encoding
algorithm", which quickly defers to RFC 2388. Said RFC doesn't consider the
possibility of invalid type, just saying that Content-Type defaults to
text/plain if not known.

>
LayoutTests/http/tests/local/formdata/send-form-data-filename-escaping-expected
.txt:7
>
+file1=before-hax.txt:text/plain:1234567890&file2=more%22hax%0D%0A.txt%0D%0A:ap
plication/hax content-length: 20
:1234567890&file3=after-hax.txt:text/plain:1234567890

My previous comment still stands - the test does not have clear pass/fail
criteria. If something changes in the output five years later, how will we know
if the test really fails, or it's a benign change?

I suggest staring with splitting the test into multiple subtests, each with its
own output and pass criteria.

>
LayoutTests/http/tests/local/formdata/send-form-data-filename-escaping.html:42
> +    testFailed("This test is not interactive, please run using
DumpRenderTree");

Is this actually true? What prevents the test from passing?

For Content-Type, we don't need to have a real file, we can build the blob
dynamically. If some subtests can be run interactively and others can not,
please split this into multiple .html tests.