`CookieMap#getAll` does not return all values for a given cookie name

[guilgaly]

Describe the bug The Scaladoc for com.twitter.finagle.http.CookieMap#getAll says that it "Fetches all cookies with the given name from this map.". However it appears to always return only the first value for a cookie name when handling a request and accessing the request cookies.

To Reproduce

1. Send an HTTP request with a cookie header containing different values for the same name (from a web browser, this will happen if the same cookie name was set with different paths, some more specific than others); e.g. Cookie: FOO=value1; FOO=value2; FOO=value3.
2. In the server code, retrieve the cookie values with request.cookies.getAll("FOO"); it will return Seq("value1") instead of Seq("value1", "value2", "value3").

Expected behavior I think CookieMap#getAll would be expected to return all values for a given name when several values are given in the request cookies.

Environment For the cookie header format: tested with Chrome 91 on macOS 10.15.7. For Finagle: tested with Finagle 20.3.0 (comes with Finch 0.32.1). Looking at the code for com.twitter.finagle.http.CookieMap and com.twitter.finagle.http.netty4.Netty4CookieCodec in the develop branch, it should still be the same.

Additional context I see two causes for this:

1. com.twitter.finagle.http.netty4.Netty4CookieCodec#decodeServer calls io.netty.handler.codec.http.cookie.ServerCookieDecoder#decode rather than io.netty.handler.codec.http.cookie.ServerCookieDecoder#decodeAll. According to the Netty Javadoc, decodeAll "includes all cookie values present, even if they have the same name", unlike decode.
2. If we change 1., then com.twitter.finagle.http.CookieMap#addNoRewrite only keeps the last value added for a given name (that's because of how equality works for com.twitter.finagle.http.Cookie; the cookies would only differ by their paths, and we of course cannot know the paths for the request cookies on the server side).

I'd be happy to try and submit a pull request, if this is in fact not the expected behavior.

[yufangong]

Hi @guilgaly, I agree with you this smells like a bug. Pull requests are welcomed! thanks!

In terms of 2, can you explain a bit more of what would be the behavior change and how it would affect the use of com.twitter.finagle.http.CookieMap#add? Thanks!

[guilgaly]

In terms of 2, can you explain a bit more of what would be the behavior change and how it would affect the use of com.twitter.finagle.http.CookieMap#add? Thanks!

I'd say that the current behavior is correct for a response (it doesn't make sense to set different values for the same combination of name/path/domain; but we can always specify different paths with the same name). So we can't just change the cookie equality to always consider the value (plus it might have other impacts elsewhere).

Maybe the best solution is to just check if message.isRequest in addNoRewrite and have two different behaviors:

• if message.isRequest, always add the value to the list returned by underlying(name) (without any de-duplication)
• if not, keep the current behavior (with de-duplication relying on Cookie#equals)

[yufangong]

@guilgaly thanks! That sounds like a legit solution. Looks like you are interested in patching this issue, I'll mark this in progress for now. ty!

[guilgaly]

@yufangong I forgot to mention: I opened a PR, and in addition to the above:

• I also had to preserve the insertion order for cookies of the same name, as seen in getAll and in the iterator, since that can matter for request cookies.
• In the tests, I split the cases where cookies are added with either + (which explicitly doesn't allow duplicates) or with add, but I don't know if that's clear enough and complete enough.
• I just realized I should probably add some tests to Netty4CookieMapTest too, I'll do that later.