Skip to content

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

Closed
wants to merge 1 commit into from
Closed

Change combined values #813

wants to merge 1 commit into from

Conversation

annevk
Copy link
Member

@annevk annevk commented Oct 11, 2018

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

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.)
@annevk
Copy link
Member Author

annevk commented Oct 11, 2018

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

@annevk
Copy link
Member Author

annevk commented Oct 11, 2018

The other thing I'm interested in is if we should use this same definition when defining parsers for particular headers, such as Content-Type, or if we should use a version that does not include the space. This ends up mattering a bit in edge cases unfortunately (double quotes split across headers). I guess ideally we'd fix the API here, but ugh.

@wisniewskit I suppose you might have thoughts on this too.

@MattMenke2
Copy link
Contributor

I'm concerned about ignoring empty response header fields. Something like:

Content-Length:
Content-Length: 10

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.

@annevk
Copy link
Member Author

annevk commented Oct 15, 2018

An intermediary that combines would make that Content-Length: 10 and you wouldn't be wiser. Unless we change the HTTP standard I'm not sure there's much we can do there?

Note also that Content-Length: ,10 (not the combined value, I know, but interesting) ends up being treated as 10. And 10, too! (Arguably we could/should fix these.)

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?

@MattMenke2
Copy link
Contributor

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?

@annevk
Copy link
Member Author

annevk commented Oct 16, 2018

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!

@annevk
Copy link
Member Author

annevk commented Oct 16, 2018

Under what pre-existing spec is "Content-Length: ,10" the same as "Content-Length: 10"? Am I missing something?

No spec, but this is what browsers do at the moment. (Whereas two separate headers does result in a network error.)

@annevk
Copy link
Member Author

annevk commented Oct 16, 2018

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 Content-Length discussion is probably best had in web-platform-tests/wpt#10548. It'd be ideal if we could make ,10 and 10, a network error.

@annevk annevk closed this Oct 16, 2018
@annevk annevk deleted the annevk/combining-values branch October 16, 2018 10:26
@MattMenke2
Copy link
Contributor

Under what pre-existing spec is "Content-Length: ,10" the same as "Content-Length: 10"? Am I missing something?

No spec, but this is what browsers do at the moment. (Whereas two separate headers does result in a network error.)

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

"combined value" is wrong
2 participants