HttpConversionUtil - Incorrect conversion of Cookie header

[blucas]

Netty Version: master (latest snapshot: a6816bd)

The browser (I used Chrome 46.0.2490.80 for testing) sent an HTTP/2 request containing multiple cookies to a netty service, the HttpConversionUtil generated multiple Cookie header entries in the HTTP/1.x request. According to one stackoverflow post, this is incorrect behaviour. The HttpConversionUtil should generate a single Cookie header containing all cookie values.

// cc @Scottmitch @nmittler - I could provide a patch, but I'm not too sure the best approach to take. Considerations about how to extract the cookies, and reformat them into a single header have to be made. If you could give me some tips, I might be able to come up with a PR.

[normanmaurer]

@Scottmitch @nmittler please have a look guys :)

[nmittler]

I wonder if HttpConversionUtil should just group all multi-valued headers into single entries (i.e. not treat Cookie as special here). It wouldn't be invalid and I suspect would perform better end-to-end.

@Scottmitch WDYT?

[normanmaurer]

@nmittler +1

[blucas]

@nmittler - As far as I can tell, that won't work. What is true for one header is not necessarily true for others. Specifically I can think of the Set-Cookie header. In this case (depending which version/browser you use) sending each cookie in a separate Set-Cookie header is actually the correct thing to do (log into or out of facebook to see an example of this).

[nmittler]

Yup, looking at https://tools.ietf.org/html/rfc7230#section-3.2.2 ...

Set-Cooke is the exception. It also sounds for Netty to function as a proxy, it should not merge headers.

And as described in http://tools.ietf.org/html/rfc6265#section-5.4: the user agent MUST NOT attach more than one Cookie header field.

@blucas Are you saying that Chrome is sending multiple Cookie headers, or that it's sending a multi-valued Cookie header that Netty is then splitting into separate headers? The former would appear to be a chrome bug.

[blucas]

+1 for not merging headers 😃

@nmittler I haven't checked that yet. My assumption is that it is a Netty related issue, as Firefox is also affected by this. I will check now and get back to you

[blucas]

Ok, so I still don't know if it is the browser's or netty's (possibly twitter hpack's) fault. All I can tell you is

• Both Firefox and Chrome exhibit the issue
• Single call to addFragment which has endOfHeaders set to true.

Maybe @Scottmitch could shed some light on this one?

BTW, looking at chrome://net-internals for the request generated this stack:

t= 38394 [st= 35693]    HTTP2_SESSION_SEND_HEADERS
                        --> fin = true
                        --> :authority: www.somesite.net
                            :method: GET
                            :path: /managemain/menu.cfm
                            :scheme: https
                            accept: text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,*/*;q=0.8
                            accept-encoding: gzip, deflate, sdch
                            accept-language: en-US,en;q=0.8
                            cache-control: max-age=0
                            cookie: [118 bytes were stripped]
                            upgrade-insecure-requests: 1
                            user-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/46.0.2490.80 Safari/537.36
                        --> priority = 0
                        --> stream_id = 3
                        --> unidirectional = false

Chrome did the [118 bytes were stripped] not me.

[Scottmitch]

@blucas - Can you post your server code somewhere (or some reduced set of it)? I just modified the example http2 helloworld server and client to both use the HTTP/1.x translation layer and added a cookie from the client (request.headers().add(HttpHeaderNames.COOKIE, "foo1, foo2");) and this is what I see on the server side:

DefaultFullHttpRequest(decodeResult: success, version: HTTP/1.1, content: UnpooledUnsafeHeapByteBuf(ridx: 0, widx: 0, cap: 0))
GET /whatever HTTP/1.1
x-http2-scheme: http
host: 127.0.0.1:8080
accept-encoding: gzip
accept-encoding: deflate
cookie: foo1, foo2
x-http2-stream-id: 3
content-length: 0

It looks like CSV headers are preserved (cookie), and multiple headers as different entries (accept-encoding) are preserved. Are you thinking we should always force everything to be a single CSV separated value unless otherwise prohibited from doing this by the spec?

[blucas]

@Scottmitch - I don't have a reproducer handy for you to test with, but all you need to do is make your HTTP/2 client send multiple cookie headers to your netty HTTP/2 -> HTTP/1.x translation server.

Are you thinking we should always force everything to be a single CSV separated value unless otherwise prohibited from doing this by the spec?

No. We shouldn't force everything to use single CSV.

I took a look at the HTTP/2 spec, and finally think I've found what I'm trying to explain.

To allow for better compression efficiency, the Cookie header field MAY be split into separate header fields, each with one or more cookie-pairs.

So, according to the spec, it is perfectly valid for a HTTP/2 client to send multiple cookie headers (thus Firefox and Chrome are complying with the spec). //cc @nmittler - I believe this answers your question.

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.

So I interpret that as Netty's HttpConversionUtil.Http2ToHttpHeaderTranslator should be responsible for concatenating one or more HTTP/2 cookie headers into a single HTTP/1.x Cookie header.

// Example 1 - Client sends multiple single value HTTP/2 cookie headers
cookie: a=b
cookie: c=d
cookie: e=f

// Netty should translate that to a single HTTP/1.x Cookie header
cookie: a=b; c=d; e=f

// Example 2 - Client sends single and multi-value HTTP/2 cookie headers
cookie: a=b; c=d
cookie: e=f

// Netty should translate that to a single HTTP/1.x Cookie header
cookie: a=b; c=d; e=f

I hope that makes more sense 😃

[blucas]

@Scottmitch - gentle ping :)

[normanmaurer]

@Scottmitch ^^

[Scottmitch]

@blucas - Sorry for the delay and thank you for the reference to the specification. I will submit a PR soon.