Repository: netty/netty
Issue: ServerCookieDecoder does not support duplicate cookies

### Expected behavior

According to [RFC-6265 (section 4.2.2)](https://tools.ietf.org/html/rfc6265#section-4.2.2), multiple cookie pairs with the same name are not explicitly excluded from happening. It is even hinted that it should be expected, 

   In particular, if the Cookie header contains two cookies with the same name (e.g.,
   that were set with different Path or Domain attributes), servers SHOULD NOT rely 
   upon the order in which these cookies appear in the header. 

I would expect that ServerCookieDecoder (and the deprecated CookieDecoder) would return multiple cookies of the same name.

This is particularly important because a client might have multiple cookies for different paths or domains, and the client WILL send them all. It's unspecified if clients will send the duplicate cookies in a single Cookie header or multiple Cookie headers, but it is commonly seen in a single Cookie headers, which is why this an issue.

### Actual behavior

ServerCookieDecoder will return a single cookie when provided with multiple cookies with identical names.

[This happens because a TreeSet is used to collect the Cookies](https://github.com/quidryan/netty/blob/4.1/codec-http/src/main/java/io/netty/handler/codec/http/cookie/ServerCookieDecoder.java#L71) while the DefaultCookie class has a hashCode/equals/compareTo that doesn't take into account the value of the cookie.

### Steps to reproduce

```
    @Test
    public void testDecodingDuplicateCookies() {
        String c1 = "myCookie=myValue;";
        String c2 = "myCookie=myValue2;";
        String c3 = "myCookie=myValue3;";

        Set<Cookie> cookies = ServerCookieDecoder.STRICT.decode(c1 + c2 + c3);
        assertEquals(3, cookies.size());
        Iterator<Cookie> it = cookies.iterator();
        Cookie cookie = it.next();
        assertNotNull(cookie);
        assertEquals("myValue", cookie.value());
        cookie = it.next();
        assertNotNull(cookie);
        assertEquals("myValue2", cookie.value());
        cookie = it.next();
        assertNotNull(cookie);
        assertEquals("myValue3", cookie.value());
    }
```

The `assertEquals(3, cookies.size());` will fail, since only one cookie will be returned.

### Minimal yet complete reproducer code (or URL to code)

See above.

### Netty version

4.1.11.FINAL


Issue comments:
The fix can't just be to use a different Set implementation, the current equals/hashCode of io.netty.handler.codec.http.cookie.DefaultCookie will prevent any Set from ever holding multiple Cookies with the same name, because there is no path or domain to differentiate on. Ideally ServerCookieDecoder::decode would return a List<Cookie>, but that would be a breaking interface change.

I would recommend adding `value` to the equals/hashCode/compareTo of DefaultCookie. If this is the right approach, I'm happen to provide a PR.

Additionally the javadoc for ServerCookieDecoder::decode say Set-Cookie, but I'm pretty sure the class is meant to decode Cookie headers, not Set-Cookie. I would also recommend a LinkedHashSet instead of  TreeSet, since there is some value in maintaining the order that the client sent the cookie pairs in.
> Ideally ServerCookieDecoder::decode would return a List, but that would be a breaking interface change.

Maybe provide a different set of decode methods and deprecate existing ones. Thoughts?

> I would recommend adding value to the equals/hashCode/compareTo of DefaultCookie. If this is the right approach, I'm happen to provide a PR.

We could also pass a Comparator to the TreeSet constructor that would account for the value.
I don't even think the decoder should return Cookie classes if only name and value are ever set. At least I got confused and thought I could access domain/path.

Two suggestions:

- decode to Map<String, List<String>> just like QueryStringDecoder#parameters()

- decode to an Iterable<Map.Entry<String, String>> just like HttpHeaders
Just hit this today on 4.1.30

Pull request review comments:

Patch that resolved the issue:
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/cookie/ServerCookieDecoder.java b/codec-http/src/main/java/io/netty/handler/codec/http/cookie/ServerCookieDecoder.java
index cf5349b0863..f56fb919a80 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/cookie/ServerCookieDecoder.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/cookie/ServerCookieDecoder.java
@@ -17,7 +17,10 @@
 
 import static io.netty.util.internal.ObjectUtil.checkNotNull;
 
+import java.util.ArrayList;
+import java.util.Collection;
 import java.util.Collections;
+import java.util.List;
 import java.util.Set;
 import java.util.TreeSet;
 
@@ -56,20 +59,41 @@ private ServerCookieDecoder(boolean strict) {
         super(strict);
     }
 
+    /**
+     * Decodes the specified Set-Cookie HTTP header value into a {@link Cookie}.  Unlike {@link #decode(String)}, this
+     * includes all cookie values present, even if they have the same name.
+     *
+     * @return the decoded {@link Cookie}
+     */
+    public List<Cookie> decodeAll(String header) {
+        List<Cookie> cookies = new ArrayList<Cookie>();
+        decode(cookies, header);
+        return Collections.unmodifiableList(cookies);
+    }
+
     /**
      * Decodes the specified Set-Cookie HTTP header value into a {@link Cookie}.
      *
      * @return the decoded {@link Cookie}
      */
     public Set<Cookie> decode(String header) {
+        Set<Cookie> cookies = new TreeSet<Cookie>();
+        decode(cookies, header);
+        return cookies;
+    }
+
+    /**
+     * Decodes the specified Set-Cookie HTTP header value into a {@link Cookie}.
+     *
+     * @return the decoded {@link Cookie}
+     */
+    private void decode(Collection<? super Cookie> cookies, String header) {
         final int headerLen = checkNotNull(header, "header").length();
 
         if (headerLen == 0) {
-            return Collections.emptySet();
+            return;
         }
 
-        Set<Cookie> cookies = new TreeSet<Cookie>();
-
         int i = 0;
 
         boolean rfc2965Style = false;
@@ -149,7 +173,5 @@ public Set<Cookie> decode(String header) {
                 cookies.add(cookie);
             }
         }
-
-        return cookies;
     }
 }


Test patch:
diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/cookie/ServerCookieDecoderTest.java b/codec-http/src/test/java/io/netty/handler/codec/http/cookie/ServerCookieDecoderTest.java
index b157bc3c73b..b6b33655757 100644
--- a/codec-http/src/test/java/io/netty/handler/codec/http/cookie/ServerCookieDecoderTest.java
+++ b/codec-http/src/test/java/io/netty/handler/codec/http/cookie/ServerCookieDecoderTest.java
@@ -15,6 +15,7 @@
  */
 package io.netty.handler.codec.http.cookie;
 
+import java.util.List;
 import org.junit.Test;
 
 import java.util.Iterator;
@@ -53,6 +54,26 @@ public void testDecodingMultipleCookies() {
         assertEquals("myValue3", cookie.value());
     }
 
+    @Test
+    public void testDecodingAllMultipleCookies() {
+        String c1 = "myCookie=myValue;";
+        String c2 = "myCookie=myValue2;";
+        String c3 = "myCookie=myValue3;";
+
+        List<Cookie> cookies = ServerCookieDecoder.STRICT.decodeAll(c1 + c2 + c3);
+        assertEquals(3, cookies.size());
+        Iterator<Cookie> it = cookies.iterator();
+        Cookie cookie = it.next();
+        assertNotNull(cookie);
+        assertEquals("myValue", cookie.value());
+        cookie = it.next();
+        assertNotNull(cookie);
+        assertEquals("myValue2", cookie.value());
+        cookie = it.next();
+        assertNotNull(cookie);
+        assertEquals("myValue3", cookie.value());
+    }
+
     @Test
     public void testDecodingGoogleAnalyticsCookie() {
         String source =