setRequestHeader behavior doesn't seem to match implementations

[bzbarsky]

If you look at http://w3c-test.org/XMLHttpRequest/setrequestheader-case-insensitive.htm it fails in all of Safari, Chrome, Firefox, and Edge. The test is doing this:

    var client = new XMLHttpRequest()
    client.open("POST", "resources/inspect-headers.py?filter_value=t1,t2,t3", false)
    client.setRequestHeader("x-test", "t1")
    client.setRequestHeader("X-TEST", "t2")
    client.setRequestHeader("X-teST", "t3")
    client.send(null)
    assert_equals(client.responseText, "x-test,")

which asserts that the value of the "x-test" header that was sent to the server is "t1,t2,t3". In Safari, Chrome, Firefox, and Edge the value is "t1, t2, t3" (with spaces after the commas).

This sort of thing has come up before (e.g. whatwg/fetch#422)...

@annevk I don't know whether you want to fix this in XHR or in fetch or whether you think all browsers should change here or what...

[bzbarsky]

Oh, and of course if the spec changes here the XMLHttpRequest/setrequestheader-case-insensitive.htm web platform test needs to get updated.

[bzbarsky]

I expect XMLHttpRequest/setrequestheader-header-allowed.htm has the same issue....

[bzbarsky]

One other note. At least in the case of Gecko, this is all handled in the underlying networking library, not XHR itself. Which means the "separate with comma and space" is general behavior for headers, and applies to fetch at the moment, and to any network requests that Gecko makes, including ones we don't have ported to be on top of fetch yet.

I would not be surprised if the situation in other engines is similar.

So it might make the most sense to change this in fetch after all...

[annevk]

I think when I last looked at this Necko wasn't consistent in how it used spaces for various headers. And other browsers weren't either. Might have been Accept-Encoding?

[bzbarsky]

It all depends on how the headers are set.

If the header is set all at once to some string with commas and no spaces in it, then that's the string that will get sent. That's the Accept-Encoding situation: it's set all at once.

But if the header is set multiple times with the same header name and multiple values and the coalescing into a single comma-separated list happens inside the networking library, then the spaces will be there.

Just like if you do (in the current spec):

xhr.setRequestHeader("foo", "a,    b,     c");

you get a different value sent than if you do:

xhr.setRequestHeader("foo", "a");
xhr.setRequestHeader("foo", "b");
xhr.setRequestHeader("foo", "c");

[annevk]

Yeah, but they're semantically equivalent.

We did discuss this at some point and decided that changing it would probably be okay.

@youennf @tyoshino thoughts?

[youennf]

The coalescing in WebKit is not consistent either, the underlying libraries usually implementing " ".
It would be just simpler to align fetch here.
It would be even better to just punt on RFC 7230.
IIRC, RFC7230 should be made more precise about that.

[youennf]

s/implementing " "/implementing ", "/

[bzbarsky]

Yeah, but they're semantically equivalent.

I understand that. And in theory. :( In practice, lots of places seem to have custom processing of all sorts for XHR-sent headers. :(

[annevk]

@youennf punting to RFC 7230 would be great, if it defined operations for this that everyone aligned on. However, that's not really their style. They're okay with A and B being semantically equivalent and not requiring a particular serialization.

In any event, I guess I'll change this in Fetch to include the space.

[annevk]

Okay, so we basically want to revert whatwg/fetch#207.

[annevk]

So Fetch mentions , three times:

1. "combine"; used by XMLHttpRequest and WebSocket and brought up here.
2. "combined value", used by Headers. Unclear to me what we want to do there.
3. Population algorithm of the Access-Control-Request-Headers header. Unclear to me what we want to do there (I suspect nothing, but probably warrants a note, or maybe it should be refactored in terms of "combine").

So in part I'd like to know how much inconsistency we want here. We can go with what the majority does I suppose, but we'll have to add some notes here and there to explain that is what we're doing, since casual onlookers will find it weird.

[bzbarsky]

It looks to me like Firefox uses "," without space for items 2 and 3 of your list (presumably because those pieces were more or less implemented after the current fetch spec was written) and ", " (with space) for item 1.

What do other UAs do for Access-Control-Request-Headers? Changing behavior here could lead to compat issues, obviously, so if they all agree on "," we should probably leave it that way (and in particular, depending on what happens with item 1 that might mean that it can't be defined in terms of "combine").

Yes, the inconsistencies are a bit annoying, I agree.

[annevk]

Chrome and Safari also do ACRH without space.

[annevk]

And per fetch/api/headers/headers-combine.html on WPT those browsers also agree on 2. Notes it is I guess.

[annevk]

I just realized getResponseHeader() and getAllResponseHeader() are defined in terms of Fetch... I guess those might need to be different too then?

[annevk]

Of course, no tests exist for those.

[bzbarsky]

I'm pretty sure that getResponseHeader in Gecko separates with ", " (with space). Same for getAllResponseHeaders...

[annevk]

Seems that XMLHttpRequest/getresponseheader-case-insensitive.htm tests that and was never updated to test what Fetch required.

[annevk]

Current thinking, assuming the goal is maximum compatibility with deployed rather browsers, rather than a coherent whole:

• Add a space to combine's ,. Perhaps rename it to legacy combine to indicate this is only for old stuff.
• Add a legacy space flag to sort and combine and combined value, that does exactly what you think it might do. (Potentially we do that for combine as well, so we can use it for Access-Control-Request-Headers and others.)

Updating tests first...

[annevk]

So I created this response to test getAllResponseHeaders() and browsers are all over the map.

HTTP/1.1 200 YAYAYAYA
foo-TEST: 1
FOO-test: 2
ALSO-here: Mr. PB
ewok: lego

Safari ends up with FOO-test, Chrome and Firefox with foo-TEST. Sorting is different. Most other details are the same, including ending in \r\n which goes against the current standard.

[annevk]

What's tricky about this test is that if we don't lowercase header names and sort them, all kinds of underlying HTTP library stuff gets exposed. Given that browsers are different, it seems reasonable to still try to change that?

[annevk]

If you want to view my test and test changes: web-platform-tests/wpt#4641.

[bzbarsky]

Safari ends up with FOO-test, Chrome and Firefox with foo-TEST

I'm not sure what you mean. Don't both include both headers?

I just went through the Firefox code for this stuff and at first glance we don't do any case-folding anywhere in there, but I might just be missing something.....

[bzbarsky]

Oh, I see, actually. We do kinda do case-folding but it's rather bizarre. When we first see a header name, we intern it; the key is the header string and the interned value represents that string. Later if we need to intern a header name, we look it up in the interning hashtable and this check does a case-insensitive match on the key, but returns whatever the value is.

So afaict once we see a "foo-TEST" header we will treat all header names that are just different casings of it as "foo-TEST" or so.

[annevk]

@youennf so in whatwg/fetch#189 you asked for header ordering to be defined as lexicographical. However, what do you suggest we do for getAllResponseHeaders()? What WebKit implements at the moment doesn't seem like an option.

[youennf]

Would it make sense to separate the issue between request headers and response headers?
Considering response headers as always lowercased seems to not cause any issue.

[annevk]

I'm fine with separating them, I filed #109 on the response headers, but I'd like to solve them all together.

[annevk]

@bzbarsky it seems you did not test Safari TP. Safari TP does follow the standard for setRequestHeader(), although @youennf seems to want to change that back?

In any event, provided we go ahead with ", " we still need some decisions in #109.

[bzbarsky]

@bzbarsky it seems you did not test Safari TP

Safari TP doesn't run on my OS, so yes... I should have tested WebKit nightly, though....

[annevk]

Note to self: update web-platform-tests/wpt#4956 once I figure out what to do here.

[annevk]

Steps I've taken thus far:

1. Landed changes to tests to align with implementations.
2. Posted a PR for Fetch and one for XMLHttpRequest to align with the tests for setRequestHeader() and add the space there.

[annevk]

(I'm still assuming that Safari TP wants to change back here.)

[annevk]

Filed https://bugs.webkit.org/show_bug.cgi?id=169285 on WebKit for this. Will likely land all the standard changes tomorrow.

[annevk]

So after discussion with the WebKit folks the plan has become to try and reconcile XMLHttpRequest and Fetch, aligning both on what XMLHttpRequest already did.

New bugs filed: https://bugzilla.mozilla.org/show_bug.cgi?id=1346313 and https://bugs.chromium.org/p/chromium/issues/detail?id=700434.