Adding more bogus header values

[youennf]

WebKit tightened a bit header values checking based on RFC 7230 rules.
Related bug entry is https://bugs.webkit.org/show_bug.cgi?id=128593.
The " " header value specific case is discussed in https://bugs.webkit.org/show_bug.cgi?id=147445 as some sites actually use it.

[hoppipolla-critic-bot]

Critic review: https://critic.hoppipolla.co.uk/r/5681

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

[hallvors]

I think disallowing setting the value to a single space (or any other ws character) is OK - but are we really sure it's web compatible to disallow leading and trailing spaces? I'd rather tell the RFC author to relax that requirement..

[hallvors]

@annevk thoughts?

[hallvors]

@Ms2ger points out https://www.rfc-editor.org/errata_search.php?rfc=7230&eid=4189%3E - thanks!

Also, I note that RFC7230 says

A field value might be preceded and/or followed by optional whitespace (OWS); a single SP preceding the field-value is preferred for consistent readability by humans. The field value does not include any leading or trailing whitespace: OWS occurring before the first non-whitespace octet of the field value or after the last non-whitespace octet of the field value ought to be excluded by parsers when extracting the field value from a header field.

(See 3.2.4. Field Parsing)

So, since the RFC doesn't forbid sending those spaces over the wire the question is whether an author saying setRequestHeader('X-test', ' foo ') expects those spaces to show up as part of the value on the backend, and whether our API should throw to warn him/her it doesn't work like that.

[youennf]

Agreed, the question is more at XHR level than at RFC level.
It seems to me that either setRequestHeader('X-test', ' foo ') should throw or it should lead to the exact same underlying request as setRequestHeader('X-test', 'foo') would do.
Note also that the XHR spec is currently referring to RFC2616, not RFC7230.

[annevk]

See also https://www.w3.org/Bugs/Public/show_bug.cgi?id=27797.

The discussion in the WebKit bugs is a bit unclear to me. ap is referring to a specification change, but this time WebKit actually changed ahead of any specification changes.

We could make setRequestHeader() and the equivalent API in fetch() strip leading and trailing ASCII whitespace before validation. It's not entirely clear to me what would be best here.

[hallvors]

My first impression was that it's too risky to start throwing exceptions for input nobody was throwing exceptions for until now. I've done some more testing of the current state of implementations, and we have an interesting range of behaviours:

1. Firefox and Opera (Presto): remove preceeding whitespace from header values, but not whitespace after the value. A whitespace-only value is sent as empty (but the header is still sent)
2. Chrome: trims whitespace before and after value, sends empty header for whitespace-only
3. IE: trims whitespace before and after value, and doesn't send any header at all for the whitespace-only one.

So setting a header to whitespace only is sort of a hack solution for "removeRequestHeader()" being missing, and I guess a few sites have discovered this and use it as such. It's also extremely likely that there are some typos and minor bugs out there adding ws before or after values - harmless stuff that would break if we start throwing.

Ergo: I think we should not start mandating exceptions for whitespace in the header value. I think either IE's or Chrome's behaviour can be standardised, although perhaps Chrome's is a tad less surprising to a web developer.

@annevk agree?

[annevk]

We should have a way to have a header with an empty value so 1/2 make the most sense. Stripping on both sides is probably somewhat better?

It's a bit unclear to me if this should affect the Headers API too.

[hallvors]

IMHO stripping on only one side just looks odd, inconsistent :-p. So let's align with Chrome. @youennf - could you change the PR? It would be nice to have a new test based on XMLHttpRequest/setrequestheader-allow-empty-value.htm to verify that ws is removed instead of the changes we have now.

[youennf]

Sounds good to me.
I will update the PR and add a specific test for " ".

[youennf]

It may also be worth investigating how the XHR header combine rule works with empty strings.
Skipping them when combining might make sense.

[annevk]

Why, isn't ,secondValue valid?

[youennf]

Digging a little bit more the issue, setting a "Content-Type" with the empty value is not in a very good shape.
WebKit and Chrome seem to combine the empty value with the default "Content-Type" value.
Looking at the code, Firefox seems to use the default "Content-Type" value.
Don't know about IE.

As can be read in https://bugs.webkit.org/show_bug.cgi?id=147445, one web app is currently using " " as "Content-Type" value. They are migrating to application/octet-stream which makes more sense but are still using it currently.

I think the browser should try to honor what the web app wants (or maybe throw an exception). Changing the value behind the scene seems strange, especialy the combined value.

[annevk]

Combine semantics only take effect if the developer sets a header twice. The user agent is only supposed to add Content-Type if none was added by the developer per the XMLHttpRequest Standard.

[youennf]

Agreed.
A patch is under review in WebKit (https://bugs.webkit.org/show_bug.cgi?id=147784) to fix that.

[hallvors]

great, thanks @youennf

[annevk]

@youennf would you like to be added as Youenn Fablet to the acknowledgments section of the Fetch and XMLHttpRequest Standards? Or rather as youennf?

[youennf]

Thanks for asking.
Youenn Fablet is fine.