-
Notifications
You must be signed in to change notification settings - Fork 358
Change combined values #813
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
Conversation
This marks XMLHttpRequest's approach in setRequestHeader() for combining values as naïve (it doesn't follow the spirit of HTTP, but is technically allowed) and changes "combined value" to more closely match HTTP semantics. Tests: web-platform-tests/wpt#13471. Fixes #757. (Addresses #752 to some extent, but leaving that open for a better API.)
@ddragana @MattMenke2 @youennf @mnot please take a look. I think the one further improvement here could be (and why I wrote "more closely match" above) to not include the space (0x20) as HTTP doesn't seem to do that either when combining (for the non-naïve case), but I'm rather worried about compatibility. Firefox passes the tests. Chrome and Safari would have some work to do if we agree on this. (I haven't tested Edge yet since BrowserStack keeps being annoying with local tests.) |
The other thing I'm interested in is if we should use this same definition when defining parsers for particular headers, such as @wisniewskit I suppose you might have thoughts on this too. |
I'm concerned about ignoring empty response header fields. Something like: Content-Length: Seems more like something that should be rejected than treated as the logical equivalent to "Content-Length: 10". Seems like allowing it would potentially allow for more shenanigans around deliberately tricking (Or accidentally confusing) proxies and security software. |
An intermediary that combines would make that Note also that Given that HTTP combining is a thing, I'm not sure what alternative strategy we have. Not ignore empty/whitespace values and contradict the HTTP standard? |
Unless we change the spec? What spec requires ignoring empty response headers? Or do you mean your proposed changes here? Under what pre-existing spec is "Content-Length: ,10" the same as "Content-Length: 10"? Am I missing something? |
Sigh, I got misled by https://tools.ietf.org/html/rfc7230#section-7 and the discussion at httpwg/http-extensions#596 (comment). But that should happen at the level of parsing individual headers, it shouldn't happen at this level. My bad, this is a bug in Firefox. Thanks a lot for catching this! |
No spec, but this is what browsers do at the moment. (Whereas two separate headers does result in a network error.) |
Closing as this was bogus. I'm changing the expected results in web-platform-tests/wpt#13471 and will file a bug against Firefox for eliding empty/whitespace header values when there's multiple headers of the same name. The |
I'm surprised that Chrome does this, but you're absolutely correct - deep within Chrome's parser is code that just throws away empty HTTP response headers. |
This marks XMLHttpRequest's approach in setRequestHeader() for combining values as naïve (it doesn't follow the spirit of HTTP, but is technically allowed) and changes "combined value" to more closely match HTTP semantics.
Tests: web-platform-tests/wpt#13471.
Fixes #757. (Addresses #752 to some extent, but leaving that open for a better API.)
Preview | Diff