search

hyperium / h2

HTTP 2.0 client & server implementation for Rust.
MIT License
1.34k stars 266 forks source link

Handling of RFC7540 8.1.2.5 #699

Open valkum opened 1 year ago

valkum commented 1 year ago

We ran into an issue where the HeaderMap returned in hyper as part of http::Request contains multiple entries for the cookie key.

The HTTP/2 spec states in 8.1.2.5:

If there are multiple Cookie header fields after decompression, these MUST be concatenated into a single octet string using the two-octet delimiter of 0x3B, 0x20 (the ASCII string "; ") before being passed into a non-HTTP/2 context, such as an HTTP/1.1 connection, or a generic HTTP server application.

Currently, If I see this right, during decoding each header is simply appended to the HeaderMap (see https://github.com/hyperium/h2/blob/master/src/frame/headers.rs#L895C38-L895C38)

This HeaderMap ends up in the request that is passed to the user of h2 (/hyper) without merging the Cookie header. I created a simple repro at https://github.com/valkum/h2-cookie-violation You need go installed because curl currently does not use 8.1.2.5. but the go http2 seems to do that (similar to Browsers).

If, for any reason, this is an expected deviation from the spec, I guess h2 and possible hyper should get some docs about this deviation. It seems the current ecosystem (for using cookies in Rust) settled on using HeaderMap::get_all for Cookies.

nox commented 9 months ago

I don't know if that should be done but at the very least it shouldn't be done by Hyper, as it would be weird for Hyper to do it only for HTTP/2 (as nothing in HTTP/1.1 says they must be concatenated before being passed to other stuff).

I feel like this has already been discussed in the past and the conclusion was that it isn't h2 nor Hyper's business to normalize Cookie headers.

nox commented 9 months ago

https://github.com/hyperium/hyper/issues/2528

valkum commented 9 months ago

Thanks for linking to this in hyper. Would you be open to a PR documenting the behavior somewhere? I am not sure for the best place inside hyper, as hyper::header and hyper::Requestare just re-exports. But as this only affects HTTP/2, I think it makes more sense to document it inside the Inbound Streams block.

juliuskreutz commented 3 months ago

Hello. Are there any updates on this? Because this unhandled specification has come up in pingora again, since it's using this crate.

Just curious, but shouldn't the h2 crate actually implement all the specifications for HTTP/2 or what was the agreement made here?

nox commented 3 months ago

The h2 crate is strictly about HTTP/2, there is no "non-HTTP/2 context, such as an HTTP/1.1 connection, or a generic HTTP server application" here. The place to implement that is in systems that do provide such things, such as Pingora or Hyper.