Bug 8644 – XMLHttpRequest removes spaces from content-types before processing

• Status: RESOLVED
• Resolution: FIXED
• Component: XML

[Eric Seidel (no email)]

static inline String getMIMEType(const String& contentTypeString)
in xmlhttprequest.cpp has a line which skips spaces.

I'm not sure this is right.  According to RFC 2045 " " is an invalid char for MIME types, we're just ignoring it, instead of rejeccting the content type as I would have expected.

[Alexey Proskuryakov]

I think skipping spaces accounts for the following (allowed by RFC 2616, if I understand it correctly):

Content-Type: text/html  ;  charset=windows-1251

The existing code certainly seems to be too forgiving. I'm marking this as a blocker for bug 5954 (Cleanup Content-Type parsing).

[Dave Hyatt]

What WinIE does is all that matters.  We may be too forgiving because they are.  Make sure to test with WinIE.

[Darin Adler]

*** Bug 8997 has been marked as a duplicate of this bug. ***

[Julian Reschke]

WFIW, the equivalent bug in Chromium currently is being fixed: https://bugs.chromium.org/p/chromium/issues/detail?id=642346#c11

[Rob Buis]

Created attachment 356913 [details]
Patch

[EWS Watchlist]

Comment on attachment 356913 [details]
Patch

Attachment 356913 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10325909

New failing tests:
imported/w3c/web-platform-tests/mimesniff/mime-types/parsing.any.html
imported/w3c/web-platform-tests/mimesniff/mime-types/parsing.any.worker.html

[EWS Watchlist]

Created attachment 356914 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 356913 [details]
Patch

Attachment 356913 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10325922

New failing tests:
imported/w3c/web-platform-tests/mimesniff/mime-types/parsing.any.html
imported/w3c/web-platform-tests/mimesniff/mime-types/parsing.any.worker.html

[EWS Watchlist]

Created attachment 356915 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 356913 [details]
Patch

Attachment 356913 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10325980

New failing tests:
imported/w3c/web-platform-tests/mimesniff/mime-types/parsing.any.html
imported/w3c/web-platform-tests/mimesniff/mime-types/parsing.any.worker.html

[EWS Watchlist]

Created attachment 356919 [details]
Archive of layout-test-results from ews115 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 356913 [details]
Patch

Attachment 356913 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10325979

New failing tests:
imported/w3c/web-platform-tests/mimesniff/mime-types/parsing.any.html
imported/w3c/web-platform-tests/mimesniff/mime-types/parsing.any.worker.html

[EWS Watchlist]

Created attachment 356920 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Rob Buis]

Created attachment 356921 [details]
Patch

[Darin Adler]

Comment on attachment 356921 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=356921&action=review

I’m not sure this does the right thing. I’d like to see perhaps some unit tests for this function. There are some strange edge cases in the function. For example, if the string contains only spaces and tabs then we return all those spaces and tabs, rather than returning the empty string, and FetchResponse::create might want it to return an empty string instead.

> Source/WebCore/platform/network/HTTPParsers.cpp:304
> +    unsigned pos = 0;

If writing new code it would be better to use a word rather than a word fragment. I think "position" would be OK.

> Source/WebCore/platform/network/HTTPParsers.cpp:307
> +    while (pos < length) {

Seems like this can be written as a for loop.

> Source/WebCore/platform/network/HTTPParsers.cpp:320
> +    while (pos < length) {

Seems like this can be written as a for loop.

[Rob Buis]

Created attachment 356949 [details]
Patch

[Chris Dumez]

Comment on attachment 356949 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=356949&action=review

> Source/WebCore/ChangeLog:10
> +        adheres to LWS definition from RFC 2616, i.e. space or HT.

RFC 2616 is dead, we should stop referring to it. Here, I think we may want https://tools.ietf.org/html/rfc7231#section-3.1.1.1 instead.

[Chris Dumez]

Comment on attachment 356949 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=356949&action=review

r=me with comments.

> Source/WebCore/platform/network/HTTPParsers.cpp:328
> +        if (c == ',' || c == ';')

Why is this in the comma section?  ';' is not about allow multiple values. I think we should move the ';'...

> Source/WebCore/platform/network/HTTPParsers.cpp:331
> +        if (c != '\t' && c != ' ')

... to here.

[Rob Buis]

(In reply to Chris Dumez from comment #18)
> Comment on attachment 356949 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=356949&action=review
> 
> r=me with comments.
> 
> > Source/WebCore/platform/network/HTTPParsers.cpp:328
> > +        if (c == ',' || c == ';')
> 
> Why is this in the comma section?  ';' is not about allow multiple values. I
> think we should move the ';'...
> 
> > Source/WebCore/platform/network/HTTPParsers.cpp:331
> > +        if (c != '\t' && c != ' ')
> 
> ... to here.

Are you sure about that? The old behavior is to break as soon as ';' is seen (like in chromium). I don't want to change too much except no longer ignoring whitespace, since that may break stuff.

[Chris Dumez]

Comment on attachment 356949 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=356949&action=review

>>> Source/WebCore/platform/network/HTTPParsers.cpp:328
>>> +        if (c == ',' || c == ';')
>> 
>> Why is this in the comma section?  ';' is not about allow multiple values. I think we should move the ';'...
> 
> Are you sure about that? The old behavior is to break as soon as ';' is seen (like in chromium). I don't want to change too much except no longer ignoring whitespace, since that may break stuff.

Oh, I guess I am confused about why we're not also breaking as soon as we fine an optional white space (OWS), below.

[Chris Dumez]

Comment on attachment 356949 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=356949&action=review

>> Source/WebCore/platform/network/HTTPParsers.cpp:331
>> +        if (c != '\t' && c != ' ')
> 
> ... to here.

I.e. 
if (c == '\t' || c == ' ' || c == ';')
    break;

typeEnd = position + 1;

Am I missing something?

[Rob Buis]

(In reply to Chris Dumez from comment #21)
> Comment on attachment 356949 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=356949&action=review
> 
> >> Source/WebCore/platform/network/HTTPParsers.cpp:331
> >> +        if (c != '\t' && c != ' ')
> > 
> > ... to here.
> 
> I.e. 
> if (c == '\t' || c == ' ' || c == ';')
>     break;
> 
> typeEnd = position + 1;
> 
> Am I missing something?

Ah, if that is what you meant I misinterpreted your original suggestion. That does seem  to be valid and more efficient, I'll make a patch.

[Rob Buis]

Created attachment 356971 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 356971 [details]
Patch

Clearing flags on attachment: 356971

Committed r239040: <https://trac.webkit.org/changeset/239040>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/46604633>