Repository: netty/netty
Issue: CombinedHttpHeaders indiscriminately combines all headers
Labels: defect

### Expected behavior

`CombinedHttpHeaders` combines all headers per name, even if they shouldn't be combined, like for example the `Set-Cookie` header (per [RFC-7230, section 3.2.2](https://tools.ietf.org/html/rfc7230#section-3.2.2)). In fact, there's only a limited set of headers that can be combined (see this [SO thread](http://stackoverflow.com/a/29550711/387927)).

### Actual behavior

`CombinedHttpHeaders` should support a configurable set of header names that can safely be combined (or a set of headers that should never be combined, if we want to combine be default). A default set should be available to in order to provide a sensible behaviour out-of-the-box.

### Netty version

`4.1.x`


Issue comments:
thanks for reporting @ddossot !
@ddossot <3

Pull request review comments:
@madgnome can we please keep the original constructor as well ?
please store this as non-mutable set 
You always use the opposite `!canBeCombined` so IMHO it would be better to have a `cantBeCombined` instead (I would expect IntelliJ to complain as well).
If I'm not missing something, this should be case insensitive. You should probably be using the hasher instead.
Thats a very good point...  @madgnome please also add a unit test for this.
Could we just instead special-case this one key (with a plain condition)?
Adding this constructor seems like an overkill, to let users provide their own new headers-that-are-broken since [HTTP itself only specifies Set-Cookie as an exception](https://tools.ietf.org/html/rfc7230#section-3.2.2).
Oh yeah good point, at some point I was using `canBeCombined` without the negation and wanted to avoid the double negative but it's gone now, so will change it.
I think you could just use `SET_COOKIE.contentEqualsIgnoreCase(...)`.
Fixed!

Patch that resolved the issue:
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/CombinedHttpHeaders.java b/codec-http/src/main/java/io/netty/handler/codec/http/CombinedHttpHeaders.java
index 2d43b7ad04a..ae494934d7c 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/CombinedHttpHeaders.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/CombinedHttpHeaders.java
@@ -26,6 +26,7 @@
 import java.util.List;
 import java.util.Map;
 
+import static io.netty.handler.codec.http.HttpHeaderNames.SET_COOKIE;
 import static io.netty.util.AsciiString.CASE_INSENSITIVE_HASHER;
 import static io.netty.util.internal.StringUtil.COMMA;
 import static io.netty.util.internal.StringUtil.unescapeCsvFields;
@@ -87,7 +88,7 @@ public CombinedHttpHeadersImpl(HashingStrategy<CharSequence> nameHashingStrategy
         @Override
         public Iterator<CharSequence> valueIterator(CharSequence name) {
             Iterator<CharSequence> itr = super.valueIterator(name);
-            if (!itr.hasNext()) {
+            if (!itr.hasNext() || cannotBeCombined(name)) {
                 return itr;
             }
             Iterator<CharSequence> unescapedItr = unescapeCsvFields(itr.next()).iterator();
@@ -100,7 +101,7 @@ public Iterator<CharSequence> valueIterator(CharSequence name) {
         @Override
         public List<CharSequence> getAll(CharSequence name) {
             List<CharSequence> values = super.getAll(name);
-            if (values.isEmpty()) {
+            if (values.isEmpty() || cannotBeCombined(name)) {
                 return values;
             }
             if (values.size() != 1) {
@@ -213,9 +214,13 @@ public CombinedHttpHeadersImpl setObject(CharSequence name, Iterable<?> values)
             return this;
         }
 
+        private static boolean cannotBeCombined(CharSequence name) {
+            return SET_COOKIE.contentEqualsIgnoreCase(name);
+        }
+
         private CombinedHttpHeadersImpl addEscapedValue(CharSequence name, CharSequence escapedValue) {
             CharSequence currentValue = super.get(name);
-            if (currentValue == null) {
+            if (currentValue == null || cannotBeCombined(name)) {
                 super.add(name, escapedValue);
             } else {
                 super.set(name, commaSeparateEscapedValues(currentValue, escapedValue));


Test patch:
diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/CombinedHttpHeadersTest.java b/codec-http/src/test/java/io/netty/handler/codec/http/CombinedHttpHeadersTest.java
index e0433efb97c..c63c885a955 100644
--- a/codec-http/src/test/java/io/netty/handler/codec/http/CombinedHttpHeadersTest.java
+++ b/codec-http/src/test/java/io/netty/handler/codec/http/CombinedHttpHeadersTest.java
@@ -22,9 +22,12 @@
 import java.util.Collections;
 import java.util.Iterator;
 
+import static io.netty.handler.codec.http.HttpHeaderNames.SET_COOKIE;
 import static io.netty.util.AsciiString.contentEquals;
+import static org.hamcrest.Matchers.hasSize;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertThat;
 import static org.junit.Assert.assertTrue;
 
 public class CombinedHttpHeadersTest {
@@ -66,6 +69,28 @@ public void addCombinedHeadersWhenNotEmpty() {
         assertEquals("a,b,c", headers.get(HEADER_NAME).toString());
     }
 
+    @Test
+    public void dontCombineSetCookieHeaders() {
+        final CombinedHttpHeaders headers = newCombinedHttpHeaders();
+        headers.add(SET_COOKIE, "a");
+        final CombinedHttpHeaders otherHeaders = newCombinedHttpHeaders();
+        otherHeaders.add(SET_COOKIE, "b");
+        otherHeaders.add(SET_COOKIE, "c");
+        headers.add(otherHeaders);
+        assertThat(headers.getAll(SET_COOKIE), hasSize(3));
+    }
+
+    @Test
+    public void dontCombineSetCookieHeadersRegardlessOfCase() {
+        final CombinedHttpHeaders headers = newCombinedHttpHeaders();
+        headers.add("Set-Cookie", "a");
+        final CombinedHttpHeaders otherHeaders = newCombinedHttpHeaders();
+        otherHeaders.add("set-cookie", "b");
+        otherHeaders.add("SET-COOKIE", "c");
+        headers.add(otherHeaders);
+        assertThat(headers.getAll(SET_COOKIE), hasSize(3));
+    }
+
     @Test
     public void setCombinedHeadersWhenNotEmpty() {
         final CombinedHttpHeaders headers = newCombinedHttpHeaders();
@@ -274,6 +299,15 @@ public void testGetAll() {
         assertEquals(Arrays.asList("a,b,c"), headers.getAll(HEADER_NAME));
     }
 
+    @Test
+    public void getAllDontCombineSetCookie() {
+        final CombinedHttpHeaders headers = newCombinedHttpHeaders();
+        headers.add(SET_COOKIE, "a");
+        headers.add(SET_COOKIE, "b");
+        assertThat(headers.getAll(SET_COOKIE), hasSize(2));
+        assertEquals(Arrays.asList("a", "b"), headers.getAll(SET_COOKIE));
+    }
+
     @Test
     public void owsTrimming() {
         final CombinedHttpHeaders headers = newCombinedHttpHeaders();
@@ -314,6 +348,22 @@ public void valueIterator() {
         assertValueIterator(headers.valueCharSequenceIterator(HEADER_NAME));
     }
 
+    @Test
+    public void nonCombinableHeaderIterator() {
+        final CombinedHttpHeaders headers = newCombinedHttpHeaders();
+        headers.add(SET_COOKIE, "c");
+        headers.add(SET_COOKIE, "b");
+        headers.add(SET_COOKIE, "a");
+
+        final Iterator<String> strItr = headers.valueStringIterator(SET_COOKIE);
+        assertTrue(strItr.hasNext());
+        assertEquals("a", strItr.next());
+        assertTrue(strItr.hasNext());
+        assertEquals("b", strItr.next());
+        assertTrue(strItr.hasNext());
+        assertEquals("c", strItr.next());
+    }
+
     private static void assertValueIterator(Iterator<? extends CharSequence> strItr) {
         assertTrue(strItr.hasNext());
         assertEquals("a", strItr.next());