search

whatwg / xhr

XMLHttpRequest Standard
https://xhr.spec.whatwg.org/
Other
314 stars 131 forks source link

Sort headers as if ASCII-uppercased #248

Closed hsivonen closed 5 years ago

hsivonen commented 5 years ago

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 5 years ago

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

cc @youennf @yutakahirano

youennf commented 5 years ago

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.

hsivonen commented 5 years ago

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

annevk commented 5 years ago

https://github.com/whatwg/fetch/pull/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 5 years ago

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

annevk commented 5 years ago

@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 5 years ago

Thanks!

annevk commented 5 years ago

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

hsivonen commented 5 years ago

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

annevk commented 5 years ago

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

annevk commented 5 years ago

Firefox is fixed. I filed https://bugs.webkit.org/show_bug.cgi?id=200565 against Safari. Chrome still has https://bugs.chromium.org/p/chromium/issues/detail?id=651750.

yutakahirano commented 5 years ago

Chromium: Fixed.

https://bugs.chromium.org/p/chromium/issues/detail?id=993271#c1