search

hyperium / hyper

An HTTP library for Rust
https://hyper.rs
MIT License
14.46k stars 1.59k forks source link

Hyper does not re-join cookie headers split by http/2 #2528

Open NeoLegends opened 3 years ago

NeoLegends commented 3 years ago

As per https://tools.ietf.org/html/rfc7540#section-8.1.2.5 http/2 allows splitting cookie headers into multiple parts if it improves compression efficiency. The spec mandates that the header needs to be rejoined if the associated request is proxied to a http/1 server, though.

To allow for better compression efficiency, the Cookie header field MAY be split into separate header fields, each with one or more cookie-pairs. 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, the Hyper server exposes the split cookie header as separate entries in the HeaderMap, but if the request is then proxied to a h1 server, the Client does not re-join them again (although it probably should).

I do not think it's desirable to always and forcibly re-join cookie headers before sending them out on http/1 connections, given that hyper is a low-level library and users might want to experiment with that. It might also be a breaking change for users of this library.

Therefore, to limit the blast radius of the change, I propose to only re-join headers iff the incoming request was h2 and the outgoing one is h1. Hyper neatly exposes the HTTP version on the request, which should let one do that. I'd be happy to send a PR implementing that.

Eventually, I think, hyper's server should re-join the cookie header transparently, before invoking the actual server-Service. This would then conform with the last sentence of the spec paragraph above.

nox commented 3 years ago

I guess it could do it there:

https://github.com/hyperium/hyper/blob/4e9a006498c7bdb5bb2ccb76a4c877f6da7e23b2/src/proto/h1/role.rs#L1033-L1036

Edit: fixed the link, which was previously pointing at the server encode function.

seanmonstar commented 3 years ago

Just curious, is this something that hyper itself should do? Or a responsibility of the proxy using hyper?

NeoLegends commented 3 years ago

This is a really interesting point. I've had some discussion about this with @nox on Discord, and IMO this mainly depends on whether you would consider the proxy service "generic" (as per the spec).

Since there are http-2 only proxies or services, I am not sure whether hyper should blindly merge Cookie values on http/2 connections before handing the request off to the service.

@nox and me came to the conclusion that it would make sense to implement the minimal approach first, where the hyper client merges cookie headers if it's coercing a h2 request into a h1 request, such that the "bad" requests at least don't leave the server in their broken (from an h1 perspective) state. I filed #2529 to implement this. Would appreciate your review!

In our production service we added a layer that merged the cookies, so working around this w/o hyper support is definitely not hard -- just surprising.

nox commented 3 years ago

Just curious, is this something that hyper itself should do? Or a responsibility of the proxy using hyper?

I feel like the only reason something would ever pass a h2 request to a h1 origin is that it is a proxy, and then the less expensive way to merge those headers is in the header encode loop deep in the role.rs file. But I don't have a strong opinion on this.