Kestrel should merge HTTP/2 request Cookie headers

[Tratcher]

microsoft/reverse-proxy#437

https://www.rfc-editor.org/rfc/rfc6265.html#section-5.4 (COOKIE)

When the user agent generates an HTTP request, the user agent MUST
NOT attach more than one Cookie header field.

https://tools.ietf.org/html/rfc7540#section-8.1.2.5 (HTTP/2)

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.

I don't know if Kestrel properly accounts for this, we'd have to check. I don't see anything in the HPACK decoder or Kestrel to special case Cookies.

Edit: The client in the reported YARP scenario is Chrome using HTTP/2, so this is almost certainly something we should then a fix in Kestrel.

This hasn't come up before in common AspNetCore scenarios because the cookie parser is tolerant of multiple cookie headers and people don't usually look at the cookie header directly.

[halter73]

So if I'm reading this correctly, RFC 6265 simply says clients MUST NOT send more than one Cookie header. And then the HTTP/2 RFC supersedes that and says multiple Cookie headers are allowed if they are sent separately for better compression efficiency.

The HTTP/2 RFC then goes on to say that the Cookie header needs to be merged before being proxied over an HTTP/1.1 connection or passed to "a generic HTTP server application" which in the case of ASP.NET Core would mean any middleware.

So my question is, what do you think Kestrel should do when it receives multiple Cookie headers in an HTTP/1.1 request? Reject the request? Leave multiple Cookie header values in the StringValues like it does today? Or merge the Cookie header values like it will for HTTP/2 requests? Personally, I'm leaning towards the last option.

[Tratcher]

I'd leave HTTP/1.1 alone, we don't know of any client's that actually violate the spec like that, and it would still work with the current cookie parser so there's little reason to change it. There are plans to fix how HttpClient handles sending the cookie header that would avoid this on the outgoing side.

In the HTTP/2 scenario the client is within spec and the server is not.

No rush for a 5.0 fix, the mitigation is easy.

[BrennanConroy]

@captainsafia do you have capacity to get to this in preview1?

[captainsafia]

@BrennanConroy Unfortunately, no. :/

[ghost]

Thanks for contacting us.
We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. We will evaluate the request when we are planning the work for the next milestone. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

[Tratcher]

@captainsafia no worries, we'll re-asses for the next milestone.

[Tratcher]

Note the HTTP/3 spec has the same requirement as HTTP/2.
https://quicwg.org/base-drafts/draft-ietf-quic-http.html#section-4.1.1.2

[ghost]

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.