Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sort headers as if ASCII-uppercased #248

Closed
hsivonen opened this issue May 20, 2019 · 12 comments · Fixed by #249
Closed

Sort headers as if ASCII-uppercased #248

hsivonen opened this issue May 20, 2019 · 12 comments · Fixed by #249
Labels
compat Standard is not web compatible or proprietary feature needs standardizing

Comments

@hsivonen
Copy link
Member

hsivonen commented May 20, 2019

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
Copy link
Member

annevk commented May 20, 2019

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

cc @youennf @yutakahirano

@annevk annevk added the compat Standard is not web compatible or proprietary feature needs standardizing label May 20, 2019
@youennf
Copy link

youennf commented May 20, 2019

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
Copy link
Member Author

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

@annevk
Copy link
Member

annevk commented May 21, 2019

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
Copy link
Member

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

@annevk
Copy link
Member

annevk commented May 22, 2019

@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
Copy link
Member

Thanks!

annevk added a commit to whatwg/infra that referenced this issue May 23, 2019
annevk added a commit that referenced this issue May 23, 2019
@annevk
Copy link
Member

annevk commented May 23, 2019

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

annevk added a commit to whatwg/infra that referenced this issue May 24, 2019
@hsivonen
Copy link
Member Author

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

@annevk
Copy link
Member

annevk commented May 24, 2019

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
Copy link
Member

annevk commented Aug 9, 2019

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.

aarongable pushed a commit to chromium/chromium that referenced this issue Aug 14, 2019
Implement whatwg/xhr#248. We haven't sorted
the headers, so there is no compatibility issue.

Bug: 993271, 651750
Change-Id: Ie69fb45a90795ea303324358312b16b7e6e227aa
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1750476
Reviewed-by: Yoichi Osato <[email protected]>
Reviewed-by: Adam Rice <[email protected]>
Commit-Queue: Yutaka Hirano <[email protected]>
Cr-Commit-Position: refs/heads/master@{#686673}
@yutakahirano
Copy link
Member

berniegp added a commit to berniegp/mock-xmlhttprequest that referenced this issue Aug 31, 2020
Added in version 2020-02-17 of XMLHttpRequest
whatwg/xhr#248
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compat Standard is not web compatible or proprietary feature needs standardizing
Development

Successfully merging a pull request may close this issue.

4 participants