Repository: netty/netty
Issue: CombinedHttpHeaders unescape csv values
Labels: feature

From https://github.com/netty/netty/issues/3435#issuecomment-74892285 (unescape was added by https://github.com/netty/netty/pull/4853) we should add unescape methods to [CombinedHttpHeaders](https://github.com/netty/netty/blob/4.1/codec-http/src/main/java/io/netty/handler/codec/http/CombinedHttpHeaders.java).

> Just as #3271 added the ability to automatically combine and CSV escape values associated with the same header name; we should also add the ability to automatically unescape values returned by the accessor methods. For example CharSequence get(Charsequence name) in DefaultTextHeaders should be unescaping the values that are returned (under some condition...which may be the same condition that activates escaping).


Issue comments:

Pull request review comments:
maybe use some lower capacity as the default one ?

consider optimistically incrementing `i` here (`++i`) and decrement if found to be invalid `throw newInvalidEscapedCsvFieldException(value, i - 1);`


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 2c639176071..79c5fda97a0 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
@@ -23,6 +23,7 @@
 
 import java.util.Collection;
 import java.util.Iterator;
+import java.util.List;
 import java.util.Map;
 
 import static io.netty.util.AsciiString.CASE_INSENSITIVE_HASHER;
@@ -77,6 +78,18 @@ public CombinedHttpHeadersImpl(HashingStrategy<CharSequence> nameHashingStrategy
             super(nameHashingStrategy, valueConverter, nameValidator);
         }
 
+        @Override
+        public List<CharSequence> getAll(CharSequence name) {
+            List<CharSequence> values = super.getAll(name);
+            if (values.isEmpty()) {
+                return values;
+            }
+            if (values.size() != 1) {
+                throw new IllegalStateException("CombinedHttpHeaders should only have one value");
+            }
+            return StringUtil.unescapeCsvFields(values.get(0));
+        }
+
         @Override
         public CombinedHttpHeadersImpl add(Headers<? extends CharSequence, ? extends CharSequence, ?> headers) {
             // Override the fast-copy mechanism used by DefaultHeaders
@@ -158,6 +171,12 @@ public CombinedHttpHeadersImpl set(CharSequence name, Iterable<? extends CharSeq
             return this;
         }
 
+        @Override
+        public CombinedHttpHeadersImpl setObject(CharSequence name, Object value) {
+            super.set(name, commaSeparate(objectEscaper(), value));
+            return this;
+        }
+
         @Override
         public CombinedHttpHeadersImpl setObject(CharSequence name, Object... values) {
             super.set(name, commaSeparate(objectEscaper(), values));
diff --git a/common/src/main/java/io/netty/util/internal/StringUtil.java b/common/src/main/java/io/netty/util/internal/StringUtil.java
index e60c734bb00..10461e348d5 100644
--- a/common/src/main/java/io/netty/util/internal/StringUtil.java
+++ b/common/src/main/java/io/netty/util/internal/StringUtil.java
@@ -16,6 +16,7 @@
 package io.netty.util.internal;
 
 import java.io.IOException;
+import java.util.ArrayList;
 import java.util.Formatter;
 import java.util.List;
 
@@ -425,6 +426,76 @@ public static CharSequence unescapeCsv(CharSequence value) {
     }
 
     /**
+     * Unescapes the specified escaped CSV fields according to
+     * <a href="https://tools.ietf.org/html/rfc4180#section-2">RFC-4180</a>.
+     *
+     * @param value A string with multiple CSV escaped fields which will be unescaped according to
+     *              <a href="https://tools.ietf.org/html/rfc4180#section-2">RFC-4180</a>
+     * @return {@link List} the list of unescaped fields
+     */
+    public static List<CharSequence> unescapeCsvFields(CharSequence value) {
+        List<CharSequence> unescaped = new ArrayList<CharSequence>(2);
+        StringBuilder current = InternalThreadLocalMap.get().stringBuilder();
+        boolean quoted = false;
+        int last = value.length() - 1;
+        for (int i = 0; i <= last; i++) {
+            char c = value.charAt(i);
+            if (quoted) {
+                switch (c) {
+                    case DOUBLE_QUOTE:
+                        if (i == last) {
+                            // Add the last field and return
+                            unescaped.add(current.toString());
+                            return unescaped;
+                        }
+                        char next = value.charAt(++i);
+                        if (next == DOUBLE_QUOTE) {
+                            // 2 double-quotes should be unescaped to one
+                            current.append(DOUBLE_QUOTE);
+                            break;
+                        }
+                        if (next == COMMA) {
+                            // This is the end of a field. Let's start to parse the next field.
+                            quoted = false;
+                            unescaped.add(current.toString());
+                            current.setLength(0);
+                            break;
+                        }
+                        // double-quote followed by other character is invalid
+                        throw newInvalidEscapedCsvFieldException(value, i - 1);
+                    default:
+                        current.append(c);
+                }
+            } else {
+                switch (c) {
+                    case COMMA:
+                        // Start to parse the next field
+                        unescaped.add(current.toString());
+                        current.setLength(0);
+                        break;
+                    case DOUBLE_QUOTE:
+                        if (current.length() == 0) {
+                            quoted = true;
+                            break;
+                        }
+                        // double-quote appears without being enclosed with double-quotes
+                    case LINE_FEED:
+                    case CARRIAGE_RETURN:
+                        // special characters appears without being enclosed with double-quotes
+                        throw newInvalidEscapedCsvFieldException(value, i);
+                    default:
+                        current.append(c);
+                }
+            }
+        }
+        if (quoted) {
+            throw newInvalidEscapedCsvFieldException(value, last);
+        }
+        unescaped.add(current.toString());
+        return unescaped;
+    }
+
+    /**s
      * Validate if {@code value} is a valid csv field without double-quotes.
      *
      * @throws IllegalArgumentException if {@code value} needs to be encoded with double-quotes.


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 59ae6b544d9..6ec9bfaf36a 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
@@ -18,6 +18,7 @@
 import io.netty.handler.codec.http.HttpHeadersTestUtils.HeaderValue;
 import org.junit.Test;
 
+import java.util.Arrays;
 import java.util.Collections;
 
 import static io.netty.util.AsciiString.contentEquals;
@@ -101,7 +102,7 @@ public void addCharSequencesCsvWithValueContainingComma() {
         final CombinedHttpHeaders headers = newCombinedHttpHeaders();
         headers.add(HEADER_NAME, HeaderValue.SIX_QUOTED.subset(4));
         assertTrue(contentEquals(HeaderValue.SIX_QUOTED.subsetAsCsvString(4), headers.get(HEADER_NAME)));
-        assertTrue(contentEquals(HeaderValue.SIX_QUOTED.subsetAsCsvString(4), headers.getAll(HEADER_NAME).get(0)));
+        assertEquals(HeaderValue.SIX_QUOTED.subset(4), headers.getAll(HEADER_NAME));
     }
 
     @Test
@@ -109,7 +110,7 @@ public void addCharSequencesCsvWithValueContainingCommas() {
         final CombinedHttpHeaders headers = newCombinedHttpHeaders();
         headers.add(HEADER_NAME, HeaderValue.EIGHT.subset(6));
         assertTrue(contentEquals(HeaderValue.EIGHT.subsetAsCsvString(6), headers.get(HEADER_NAME)));
-        assertTrue(contentEquals(HeaderValue.EIGHT.subsetAsCsvString(6), headers.getAll(HEADER_NAME).get(0)));
+        assertEquals(HeaderValue.EIGHT.subset(6), headers.getAll(HEADER_NAME));
     }
 
     @Test (expected = NullPointerException.class)
@@ -168,7 +169,7 @@ public void addIterableCsvSingleValue() {
     public void addIterableCsvEmtpy() {
         final CombinedHttpHeaders headers = newCombinedHttpHeaders();
         headers.add(HEADER_NAME, Collections.<CharSequence>emptyList());
-        assertTrue(contentEquals("", headers.getAll(HEADER_NAME).get(0)));
+        assertEquals(Arrays.asList(""), headers.getAll(HEADER_NAME));
     }
 
     @Test
@@ -234,7 +235,7 @@ private static CombinedHttpHeaders newCombinedHttpHeaders() {
 
     private static void assertCsvValues(final CombinedHttpHeaders headers, final HeaderValue headerValue) {
         assertTrue(contentEquals(headerValue.asCsv(), headers.get(HEADER_NAME)));
-        assertTrue(contentEquals(headerValue.asCsv(), headers.getAll(HEADER_NAME).get(0)));
+        assertEquals(headerValue.asList(), headers.getAll(HEADER_NAME));
     }
 
     private static void assertCsvValue(final CombinedHttpHeaders headers, final HeaderValue headerValue) {
@@ -253,4 +254,21 @@ private static void addObjectValues(final CombinedHttpHeaders headers, HeaderVal
             headers.add(HEADER_NAME, v.toString());
         }
     }
+
+    @Test
+    public void testGetAll() {
+        final CombinedHttpHeaders headers = newCombinedHttpHeaders();
+        headers.set(HEADER_NAME, Arrays.asList("a", "b", "c"));
+        assertEquals(Arrays.asList("a", "b", "c"), headers.getAll(HEADER_NAME));
+        headers.set(HEADER_NAME, Arrays.asList("a,", "b,", "c,"));
+        assertEquals(Arrays.asList("a,", "b,", "c,"), headers.getAll(HEADER_NAME));
+        headers.set(HEADER_NAME, Arrays.asList("a\"", "b\"", "c\""));
+        assertEquals(Arrays.asList("a\"", "b\"", "c\""), headers.getAll(HEADER_NAME));
+        headers.set(HEADER_NAME, Arrays.asList("\"a\"", "\"b\"", "\"c\""));
+        assertEquals(Arrays.asList("a", "b", "c"), headers.getAll(HEADER_NAME));
+        headers.set(HEADER_NAME, "a,b,c");
+        assertEquals(Arrays.asList("a,b,c"), headers.getAll(HEADER_NAME));
+        headers.set(HEADER_NAME, "\"a,b,c\"");
+        assertEquals(Arrays.asList("a,b,c"), headers.getAll(HEADER_NAME));
+    }
 }
diff --git a/common/src/test/java/io/netty/util/internal/StringUtilTest.java b/common/src/test/java/io/netty/util/internal/StringUtilTest.java
index 3084094f00a..1ee27e59ffa 100644
--- a/common/src/test/java/io/netty/util/internal/StringUtilTest.java
+++ b/common/src/test/java/io/netty/util/internal/StringUtilTest.java
@@ -15,6 +15,7 @@
  */
 package io.netty.util.internal;
 
+import java.util.Arrays;
 import org.junit.Test;
 
 import static io.netty.util.internal.StringUtil.*;
@@ -376,6 +377,47 @@ private void assertEscapeCsvAndUnEscapeCsv(String value) {
         assertEquals(value, unescapeCsv(StringUtil.escapeCsv(value)));
     }
 
+    @Test
+    public void testUnescapeCsvFields() {
+        assertEquals(Arrays.asList(""), unescapeCsvFields(""));
+        assertEquals(Arrays.asList("", ""), unescapeCsvFields(","));
+        assertEquals(Arrays.asList("a", ""), unescapeCsvFields("a,"));
+        assertEquals(Arrays.asList("", "a"), unescapeCsvFields(",a"));
+        assertEquals(Arrays.asList("\""), unescapeCsvFields("\"\"\"\""));
+        assertEquals(Arrays.asList("\"", "\""), unescapeCsvFields("\"\"\"\",\"\"\"\""));
+        assertEquals(Arrays.asList("netty"), unescapeCsvFields("netty"));
+        assertEquals(Arrays.asList("hello", "netty"), unescapeCsvFields("hello,netty"));
+        assertEquals(Arrays.asList("hello,netty"), unescapeCsvFields("\"hello,netty\""));
+        assertEquals(Arrays.asList("hello", "netty"), unescapeCsvFields("\"hello\",\"netty\""));
+        assertEquals(Arrays.asList("a\"b", "c\"d"), unescapeCsvFields("\"a\"\"b\",\"c\"\"d\""));
+        assertEquals(Arrays.asList("a\rb", "c\nd"), unescapeCsvFields("\"a\rb\",\"c\nd\""));
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void unescapeCsvFieldsWithCRWithoutQuote() {
+        unescapeCsvFields("a,\r");
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void unescapeCsvFieldsWithLFWithoutQuote() {
+        unescapeCsvFields("a,\r");
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void unescapeCsvFieldsWithQuote() {
+        unescapeCsvFields("a,\"");
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void unescapeCsvFieldsWithQuote2() {
+        unescapeCsvFields("\",a");
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void unescapeCsvFieldsWithQuote3() {
+        unescapeCsvFields("a\"b,a");
+    }
+
     @Test
     public void testSimpleClassName() throws Exception {
         testSimpleClassName(String.class);