Bug 147445 - XHR.setRequestHeader should remove trailing and leading whitespaces from the header value
Summary: XHR.setRequestHeader should remove trailing and leading whitespaces from the ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac All
: P2 Major
Assignee: youenn fablet
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-07-30 07:58 PDT by Chris Mitchell
Modified: 2015-08-12 03:12 PDT (History)
5 users (show)

See Also:


Attachments
Fix for combination (5.80 KB, patch)
2015-08-06 07:36 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (10.32 KB, patch)
2015-08-11 09:47 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (10.29 KB, patch)
2015-08-12 02:17 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Mitchell 2015-07-30 07:58:40 PDT
The changes for Bug 128593 were made to make the validation of the HTTP header tighter as per suggestions RFC 7230.
Looking at the changes made, extra validation has been implement namely classing a single SPACE character as invalid.
RFC 7230 doesn't not specify that this is an invalid value.

I am reporting this since we utilites the ablility to set the "Content-Type" to a space character and these changes have broken our site.
Comment 1 youenn fablet 2015-07-30 08:53:36 PDT
(In reply to comment #0)
> The changes for Bug 128593 were made to make the validation of the HTTP
> header tighter as per suggestions RFC 7230.
> Looking at the changes made, extra validation has been implement namely
> classing a single SPACE character as invalid.
> RFC 7230 doesn't not specify that this is an invalid value.

Looking at https://tools.ietf.org/html/rfc2616#section-4.2, it seems RFC 2616 is fine with a " " header value.
Looking at https://tools.ietf.org/html/rfc7230#section-3.2, field-value is expected to start with a character, not a whitespace.

That said, IIRC, XHR spec is referencing RFC 2616.

Do you know what other browsers are doing?

> I am reporting this since we utilites the ablility to set the "Content-Type"
> to a space character and these changes have broken our site.

Has CT: " " a special meaning for that server?
What about using application/octet-stream instead?
Comment 2 youenn fablet 2015-07-30 08:55:13 PDT
> Has CT: " " a special meaning for that server?
> What about using application/octet-stream instead?

Or not setting CT at all?
Comment 3 Chris Mitchell 2015-07-30 08:59:08 PDT
If you don't set CT the brower sets it for you based on the Mime Type of data being uploaded (POST/PUT). This is what I was trying to get around in the first place.

We have made changes to our system to set the CT to "application/octet-stream" but we have current installations which don't have this change.
Comment 4 Chris Mitchell 2015-07-30 09:00:10 PDT
We have no problem with other browsers.
They allow a SPACE in the header value.
Comment 5 Alexey Proskuryakov 2015-07-30 09:01:33 PDT
A field value cannot have a leading space per RFC7230, because field-content production requires a field-vchar before any spaces. Furthermore, the space would be just ignored as part of optional whitespace before the value. In other words, "Content-Type: foo" is equivalent to "Content-Type:     foo", so the extra spaces are just ignored.

Out of curiosity, what did the site achieve by setting Content-Type to space? This is not a valid MIME type either, so even if HTTP allowed that, it wouldn't have done anything meaningful I think.
Comment 6 Alexey Proskuryakov 2015-07-30 09:02:33 PDT
> Out of curiosity, what did the site achieve by setting Content-Type to space?

I see that you answered that already. Yes, using a valid content type is the correct solution.
Comment 7 Alexey Proskuryakov 2015-07-30 09:23:07 PDT
Understood, what this means is that those browsers adhere to an obsolete version of the HTTP spec here.

Calling xhr.setRequestHeader(" ") is wrong on many levels, so I'm not willing to ask for a reversal of this spec change unless there is evidence that a sizable proportion of the internet is broken because of the change.
Comment 8 youenn fablet 2015-07-31 00:46:17 PDT
(In reply to comment #7)
> Understood, what this means is that those browsers adhere to an obsolete
> version of the HTTP spec here.
> 
> Calling xhr.setRequestHeader(" ") is wrong on many levels, so I'm not
> willing to ask for a reversal of this spec change unless there is evidence
> that a sizable proportion of the internet is broken because of the change.

As part of syncing WebKit and WPT tests, let's check this with W3C folks.
I filed https://critic.hoppipolla.co.uk/r/5681
Comment 9 youenn fablet 2015-07-31 05:05:09 PDT
(In reply to comment #8)
> (In reply to comment #7)
> > Understood, what this means is that those browsers adhere to an obsolete
> > version of the HTTP spec here.
> > 
> > Calling xhr.setRequestHeader(" ") is wrong on many levels, so I'm not
> > willing to ask for a reversal of this spec change unless there is evidence
> > that a sizable proportion of the internet is broken because of the change.
> 
> As part of syncing WebKit and WPT tests, let's check this with W3C folks.
> I filed https://critic.hoppipolla.co.uk/r/5681

Discussion link is: https://github.com/w3c/web-platform-tests/pull/2045
Comment 10 youenn fablet 2015-08-04 06:37:26 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > Understood, what this means is that those browsers adhere to an obsolete
> > > version of the HTTP spec here.
> > > 
> > > Calling xhr.setRequestHeader(" ") is wrong on many levels, so I'm not
> > > willing to ask for a reversal of this spec change unless there is evidence
> > > that a sizable proportion of the internet is broken because of the change.
> > 
> > As part of syncing WebKit and WPT tests, let's check this with W3C folks.
> > I filed https://critic.hoppipolla.co.uk/r/5681
> 
> Discussion link is: https://github.com/w3c/web-platform-tests/pull/2045

Proposal seems to converge in trimming whitespace at XHR level.
That would mean sending an empty header value when passed value is " ".

IMHO, it makes sense when reading the RFC and is probably safer than throwing an exception, based on Chris feedback and what other browsers are doing...

ap, do you agree with that direction?
Comment 11 Alexey Proskuryakov 2015-08-04 09:09:54 PDT
Trimming spaces at XHR level is probably OK, however how would that fix this particular case? Also, not sending a Content-Type is just wrong.
Comment 12 youenn fablet 2015-08-04 10:05:22 PDT
(In reply to comment #11)
> Trimming spaces at XHR level is probably OK, however how would that fix this
> particular case? Also, not sending a Content-Type is just wrong.

This particular case would be solved as the Content-Type would be sent with an empty string as value.
Comment 13 Alexey Proskuryakov 2015-08-04 10:29:34 PDT
I think that it's a misunderstanding. Setting an empty Content-Type results in a malformed header field value with CFNetwork (tested on OS X 10.9.5):

Content-Type: , application/xml

So, trimming spaces wouldn't help.

And sorry for sounding like a broken record, but removing Content-Type is a bad idea, I don't see any reason to make that possible through XMLHttpRequest.
Comment 14 youenn fablet 2015-08-04 11:24:46 PDT
(In reply to comment #13)
> I think that it's a misunderstanding. Setting an empty Content-Type results
> in a malformed header field value with CFNetwork (tested on OS X 10.9.5):
> 
> Content-Type: , application/xml
> 
> So, trimming spaces wouldn't help.

Ah I see...
HTTP allows empty header values AFAIK. HPack supports it also.
Is it considered as a CFNetwork bug? Is it envisioned to update CFNetwork?

If we think CFNetwork will have support for it in the future, we should keep the idea of trimming whitespaces at XHR level and try to find a temporary solution.

> And sorry for sounding like a broken record, but removing Content-Type is a
> bad idea, I don't see any reason to make that possible through
> XMLHttpRequest.

Agreed.
Comment 15 Alexey Proskuryakov 2015-08-04 12:17:23 PDT
> Is it considered as a CFNetwork bug?

That's actually in WebKit, as calling HTTPHeaderMap::add twice combines the values. I don't know why the same didn't use to happen with a space, that's probably just another bug somewhere down the line.
Comment 16 youenn fablet 2015-08-05 06:48:47 PDT
(In reply to comment #15)
> > Is it considered as a CFNetwork bug?
> 
> That's actually in WebKit, as calling HTTPHeaderMap::add twice combines the
> values. I don't know why the same didn't use to happen with a space, that's
> probably just another bug somewhere down the line.

I read your previous message too fast, sorry about that.

It seems strange that the combine rule applies to "Content-Type".
That seems like a different bug in the XHR implementation.
Either the web app sets it and the browser uses it as is.
Or the web app does not set it and the browser will set it instead.

If we split the sub-issues, that would give:
1. Add support for trimming whitespaces in XHR.setRequestHeader
2. Ensure XHR can set a header with an empty value
3. Ensure XHR can set "Content-Type" with an empty value
4. Decide what to do about empty values in the context of XHR combine rule.

Are we converging?
Comment 17 Alexey Proskuryakov 2015-08-05 09:08:09 PDT
> If we split the sub-issues, that would give:
> 1. Add support for trimming whitespaces in XHR.setRequestHeader

I'm not sure if that would be an observable change in behavior given the below...

> 2. Ensure XHR can set a header with an empty value

I think that this should raise an exception.

> 3. Ensure XHR can set "Content-Type" with an empty value

Ditto.

> 4. Decide what to do about empty values in the context of XHR combine rule.

I remember that there was some discussion about which header field values to combine, and which to replace. We may even have a bugzilla bug.

This doesn't seem too important, as it would only affect authors who actively tried to shoot themselves in the foot by setting the same header multiple times, and expecting that to override the value.
Comment 18 youenn fablet 2015-08-05 10:50:37 PDT
(In reply to comment #17)
> > If we split the sub-issues, that would give:
> > 1. Add support for trimming whitespaces in XHR.setRequestHeader
> 
> I'm not sure if that would be an observable change in behavior given the
> below...
> 
> > 2. Ensure XHR can set a header with an empty value
> 
> I think that this should raise an exception.

Why should it be?

It is a valid value per the HTTP spec.
The semantics of that value is defined for some standardized headers, probably for application specific headers as well.

Firefox, Chrome and IE are not throwing.

> > 3. Ensure XHR can set "Content-Type" with an empty value
> 
> Ditto.

This case is really less appealing.
But again, what is the benefit of throwing here?

The same throw/notthrow rule should apply to all settable headers.
Since there are defaulting rules defined for this header, the processing of a Content-Type empty header value might be defined differently though if that proves to be useful.
Comment 19 youenn fablet 2015-08-06 07:33:47 PDT
(In reply to comment #15)
> > Is it considered as a CFNetwork bug?
> 
> That's actually in WebKit, as calling HTTPHeaderMap::add twice combines the
> values. I don't know why the same didn't use to happen with a space, that's
> probably just another bug somewhere down the line.

I had a look at the XHR code.
As suspected, the issue is that we check whether "Content-Type" is set by checking if its value is empty.
Fixing this should solve the underlying problem Chris uncovered.
Comment 20 youenn fablet 2015-08-06 07:36:59 PDT
Created attachment 258364 [details]
Fix for combination
Comment 21 youenn fablet 2015-08-06 07:38:55 PDT
(In reply to comment #20)
> Created attachment 258364 [details]
> Fix for combination

Patch illustrates the underlying bug with corresponding test and fix.
Let's see what will say mac bots about that.
Comment 22 youenn fablet 2015-08-07 08:06:45 PDT
(In reply to comment #21)
> (In reply to comment #20)
> > Created attachment 258364 [details]
> > Fix for combination
> 
> Patch illustrates the underlying bug with corresponding test and fix.
> Let's see what will say mac bots about that.

Let's fix that one as a separate bug.
I filed bug 147784 for that purpose.
Comment 23 youenn fablet 2015-08-10 10:24:49 PDT
As per https://fetch.spec.whatwg.org/#concept-header-value-normalize, trailing and leading whitespaces should be removed from setRequestHeader header values.
This should be done prior from checking from header value validity.

Let's focus on this issue here.
Comment 24 Darin Adler 2015-08-10 22:35:12 PDT
The function we probably want to use for this is stripLeadingAndTrailingHTMLSpaces. We should resist the temptation to use stripWhiteSpace, which will likely strip characters that we should not be stripping.
Comment 25 youenn fablet 2015-08-11 09:47:18 PDT
Created attachment 258719 [details]
Patch
Comment 26 youenn fablet 2015-08-11 09:51:27 PDT
(In reply to comment #24)
> The function we probably want to use for this is
> stripLeadingAndTrailingHTMLSpaces. We should resist the temptation to use
> stripWhiteSpace, which will likely strip characters that we should not be
> stripping.

stripLeadingAndTrailingHTMLSpaces is almost perfect, except that it is stripping form feed characters.
The proposed patch creates a new stripLeadingAndTrailingHTTPSpaces based on String::stripWhiteSpace(IsHTTPSpace).

Btw, stripLeadingAndTrailingHTMLSpaces is not using String::stripWhiteSpace AFIAKT. Is it for performance reasons?
Comment 27 youenn fablet 2015-08-11 09:53:35 PDT
Comment on attachment 258719 [details]
Patch

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

> Source/WebCore/platform/network/HTTPParsers.h:95
> +// Strip leading and trailing whitespace as defined by the HTML specification. 

Will update this comment in a subsequent patch to point to the right spec (https://fetch.spec.whatwg.org/#concept-header-value-normalize)
Comment 28 Darin Adler 2015-08-11 10:39:17 PDT
(In reply to comment #26)
> Btw, stripLeadingAndTrailingHTMLSpaces is not using String::stripWhiteSpace
> AFIAKT. Is it for performance reasons?

I believe so.
Comment 29 Darin Adler 2015-08-11 12:06:38 PDT
Comment on attachment 258719 [details]
Patch

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

> Source/WebCore/xml/XMLHttpRequest.cpp:958
> +    String normalizedValue = stripLeadingAndTrailingHTTPSpaces(value);
> +    if (!isValidHTTPHeaderValue(normalizedValue)) {
> +        ec = SYNTAX_ERR;
> +        return;
> +    }

This changes the order of checking as well as changing the check. Why?

Also there is no test coverage for this change in order.
Comment 30 youenn fablet 2015-08-12 02:14:59 PDT
(In reply to comment #29)
> Comment on attachment 258719 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=258719&action=review
> 
> > Source/WebCore/xml/XMLHttpRequest.cpp:958
> > +    String normalizedValue = stripLeadingAndTrailingHTTPSpaces(value);
> > +    if (!isValidHTTPHeaderValue(normalizedValue)) {
> > +        ec = SYNTAX_ERR;
> > +        return;
> > +    }
> 
> This changes the order of checking as well as changing the check. Why?
> 
> Also there is no test coverage for this change in order.

We check after normalization, following the spec.
I will revert the order change.
Comment 31 youenn fablet 2015-08-12 02:17:01 PDT
Created attachment 258818 [details]
Patch for landing
Comment 32 WebKit Commit Bot 2015-08-12 03:12:50 PDT
Comment on attachment 258818 [details]
Patch for landing

Clearing flags on attachment: 258818

Committed r188333: <http://trac.webkit.org/changeset/188333>
Comment 33 WebKit Commit Bot 2015-08-12 03:12:55 PDT
All reviewed patches have been landed.  Closing bug.