From 849bdc8a84bfa8dd73fcf02da3e1e5509755f90e Mon Sep 17 00:00:00 2001 From: Julien Hoarau Date: Sat, 1 Dec 2018 20:47:18 +1100 Subject: [PATCH] Set-Cookie headers should not be combined (#8611) Motivation: According to the HTTP spec set-cookie headers should not be combined because they are not using the list syntax. Modifications: Do not combine set-cookie headers. Result: Set-Cookie headers won't be combined anymore --- .../codec/http/CombinedHttpHeaders.java | 11 ++-- .../codec/http/CombinedHttpHeadersTest.java | 50 +++++++++++++++++++ 2 files changed, 58 insertions(+), 3 deletions(-) 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 2d43b7ad04..ae494934d7 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.Iterator; 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 class CombinedHttpHeaders extends DefaultHttpHeaders { @Override public Iterator valueIterator(CharSequence name) { Iterator itr = super.valueIterator(name); - if (!itr.hasNext()) { + if (!itr.hasNext() || cannotBeCombined(name)) { return itr; } Iterator unescapedItr = unescapeCsvFields(itr.next()).iterator(); @@ -100,7 +101,7 @@ public class CombinedHttpHeaders extends DefaultHttpHeaders { @Override public List getAll(CharSequence name) { List values = super.getAll(name); - if (values.isEmpty()) { + if (values.isEmpty() || cannotBeCombined(name)) { return values; } if (values.size() != 1) { @@ -213,9 +214,13 @@ public class CombinedHttpHeaders extends DefaultHttpHeaders { 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)); 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 e0433efb97..c63c885a95 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.Arrays; 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 class CombinedHttpHeadersTest { 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 class CombinedHttpHeadersTest { 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 class CombinedHttpHeadersTest { 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 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 strItr) { assertTrue(strItr.hasNext()); assertEquals("a", strItr.next());