[Webkit-unassigned] [Bug 111380] Handle CRLF in MIME types of Blobs appended to multipart FormData

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=111380


Alexey Proskuryakov <ap at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #191509|review?                     |review-
               Flag|                            |




--- Comment #18 from Alexey Proskuryakov <ap at webkit.org>  2013-03-05 22:45:38 PST ---
(From update of attachment 191509)
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:application/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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.