Bug 147445 – XHR.setRequestHeader should remove trailing and leading whitespaces from the header value

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Chris Mitchell]

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.

[youenn fablet]

(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?

[youenn fablet]

> Has CT: " " a special meaning for that server?
> What about using application/octet-stream instead?

Or not setting CT at all?

[Chris Mitchell]

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.

[Chris Mitchell]

We have no problem with other browsers.
They allow a SPACE in the header value.

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

> 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.

[Alexey Proskuryakov]

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.

[youenn fablet]

(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

[youenn fablet]

(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

[youenn fablet]

(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?

[Alexey Proskuryakov]

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.

[youenn fablet]

(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.

[Alexey Proskuryakov]

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.

[youenn fablet]

(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.

[Alexey Proskuryakov]

> 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.

[youenn fablet]

(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?

[Alexey Proskuryakov]

> 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.

[youenn fablet]

(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.

[youenn fablet]

(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.

[youenn fablet]

Created attachment 258364 [details]
Fix for combination

[youenn fablet]

(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.

[youenn fablet]

(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.

[youenn fablet]

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.

[Darin Adler]

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.

[youenn fablet]

Created attachment 258719 [details]
Patch

[youenn fablet]

(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?

[youenn fablet]

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)

[Darin Adler]

(In reply to comment #26)
> Btw, stripLeadingAndTrailingHTMLSpaces is not using String::stripWhiteSpace
> AFIAKT. Is it for performance reasons?

I believe so.

[Darin Adler]

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.

[youenn fablet]

(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.

[youenn fablet]

Created attachment 258818 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 258818 [details]
Patch for landing

Clearing flags on attachment: 258818

Committed r188333: <http://trac.webkit.org/changeset/188333>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.