whatwg/xhr

Sort headers as if ASCII-uppercased

hsivonen opened this issue · 12 comments

hsivonen commented

Sorting headers lexicographically after ASCII-lowercasing them has the property that application-defined headers that begin with underscores sort to the beginning of the return string of getAllResponseHeaders. This breaks when JS accidentally assumes that the header of interest cannot be the first one.

I suggest doing the sorting as if the the header names had been ASCII-uppercased, to avoid sorting headers starting with an underscore first.

annevk commented

Do we want to do this specifically for getAllResponseHeaders() in isolation or should this affect Headers as well?

cc @youennf @yutakahirano

youennf commented

I have a slight preference for getAllResponseHeaders() only since this is mainly for backward compatibility and we are moving to a world where lowercased headers is the norm.

  • 👍2
hsivonen commented

AFAICT, the two are implemented separately in Gecko, so doing this only for XHR and not Fetch seems appropriate.

annevk commented

whatwg/fetch#906 has some initial refactoring of Fetch.

I think what I'm going to do is that XHR takes the output of "sort and combine" and sorts it again using a sorting algorithm I'll define as "legacy-uppercased-byte less than". That way Fetch doesn't contain the hack and it's unlikely it'll spread further, assuming Chrome fixes their bugs...

yutakahirano commented

Sorry, which bug(s) are you referring to?

annevk commented

@yutakahirano Chrome doesn't sort headers it seems, at least as far as getAllResponseHeaders() is concerned (did not check the Headers object). I mentioned this at https://bugs.chromium.org/p/chromium/issues/detail?id=651750#c15 as I couldn't find a dedicated issue (perhaps this change was made before filing such issues).

yutakahirano commented

Thanks!

annevk commented

Okay, I have fixes up for Fetch, Infra, and XMLHttpRequest. Are you adding WPT tests @hsivonen?

hsivonen commented

I'm modifying an existing test to add an underscore header.

annevk commented

Okay, https://phabricator.services.mozilla.com/D31786 is sufficient for me. If there are no more comments I plan on landing this somewhere next week (it takes some time for all the cross-specification references to be indexed, which is also why the current patch fails).