Repository: netty/netty
Issue: CombinedHttpHeaders does not trim optional white space between comma separated values

### Expected behavior
The updated HTTP/1.x RFC allows for header values to be CSV and separated by `OWS` [1]. `CombinedHttpHeaders` should remove this `OWS` on insertion.

[1] https://tools.ietf.org/html/rfc7230#section-7

### Actual behavior
`CombinedHttpHeaders` doesn't account for the `OWS` and returns it back to the user as part of the value.

### Netty version
4.1.x


Issue comments:

Pull request review comments:
better make `length != 1` as first condition bcz it has lower cost.

UPD: Although may be not. One-char params is not very likely..
sorry for noise.
IMHO, make sense add comment that if the `value` don't contain non-OWS chars, it returns `length`, but not `-1`.
If `quoted==true`, can we increment `start` and decrement `last` to exclude check `i == start || i == last` from the loop?
`' '` -> `0x20`?
I think `' '` is easier to read... 
+1
I just wanted to emphasize specific unicode value.
Not everything can be distinguished in appearance )
Spaces may be different: https://www.cs.tut.fi/~jkorpela/chars/spaces.html
I honestly haven't found a way to make the code better while doing the suggested increment/decrement. There's more involved than just excluding the `i == start || i == last` from the loop, and I have tried to not alter too much the existing code in this PR.

@fenik17 please send a follow-up PR to implement the change you envisioned.
![MAJOR](https://raw.githubusercontent.com/SonarCommunity/sonar-github/master/images/severity-major.png) The Cyclomatic Complexity of this method "escapeCsv" is 28 which is greater than 25 authorized. [![rule](https://raw.githubusercontent.com/SonarCommunity/sonar-github/master/images/rule.png)](https://garage.netty.io/sonarqube/coding_rules#rule_key=squid%3AMethodCyclomaticComplexity)

Ok. I'll do it soon. Thanks for an effort!
@ddossot done in #6840

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 79c5fda97a0..d3c0ac72664 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
@@ -16,9 +16,9 @@
 package io.netty.handler.codec.http;
 
 import io.netty.handler.codec.DefaultHeaders;
+import io.netty.handler.codec.Headers;
 import io.netty.handler.codec.ValueConverter;
 import io.netty.util.HashingStrategy;
-import io.netty.handler.codec.Headers;
 import io.netty.util.internal.StringUtil;
 
 import java.util.Collection;
@@ -39,6 +39,11 @@ public CombinedHttpHeaders(boolean validate) {
         super(new CombinedHttpHeadersImpl(CASE_INSENSITIVE_HASHER, valueConverter(validate), nameValidator(validate)));
     }
 
+    @Override
+    public boolean containsValue(CharSequence name, CharSequence value, boolean ignoreCase) {
+        return super.containsValue(name, StringUtil.trimOws(value), ignoreCase);
+    }
+
     private static final class CombinedHttpHeadersImpl
             extends DefaultHeaders<CharSequence, CharSequence, CombinedHttpHeadersImpl> {
         /**
@@ -53,7 +58,7 @@ private CsvValueEscaper<Object> objectEscaper() {
                 objectEscaper = new CsvValueEscaper<Object>() {
                     @Override
                     public CharSequence escape(Object value) {
-                        return StringUtil.escapeCsv(valueConverter().convertObject(value));
+                        return StringUtil.escapeCsv(valueConverter().convertObject(value), true);
                     }
                 };
             }
@@ -65,7 +70,7 @@ private CsvValueEscaper<CharSequence> charSequenceEscaper() {
                 charSequenceEscaper = new CsvValueEscaper<CharSequence>() {
                     @Override
                     public CharSequence escape(CharSequence value) {
-                        return StringUtil.escapeCsv(value);
+                        return StringUtil.escapeCsv(value, true);
                     }
                 };
             }
@@ -136,7 +141,7 @@ public CombinedHttpHeadersImpl setAll(Headers<? extends CharSequence, ? extends
 
         @Override
         public CombinedHttpHeadersImpl add(CharSequence name, CharSequence value) {
-            return addEscapedValue(name, StringUtil.escapeCsv(value));
+            return addEscapedValue(name, charSequenceEscaper().escape(value));
         }
 
         @Override
@@ -149,6 +154,11 @@ public CombinedHttpHeadersImpl add(CharSequence name, Iterable<? extends CharSeq
             return addEscapedValue(name, commaSeparate(charSequenceEscaper(), values));
         }
 
+        @Override
+        public CombinedHttpHeadersImpl addObject(CharSequence name, Object value) {
+            return addEscapedValue(name, commaSeparate(objectEscaper(), value));
+        }
+
         @Override
         public CombinedHttpHeadersImpl addObject(CharSequence name, Iterable<?> values) {
             return addEscapedValue(name, commaSeparate(objectEscaper(), values));
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpHeaders.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpHeaders.java
index 2b5d75faec0..ed1d7763776 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpHeaders.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpHeaders.java
@@ -1573,7 +1573,7 @@ public boolean contains(String name, String value, boolean ignoreCase) {
 
     /**
      * Returns {@code true} if a header with the {@code name} and {@code value} exists, {@code false} otherwise.
-     * This also handles multiple values that are seperated with a {@code ,}.
+     * This also handles multiple values that are separated with a {@code ,}.
      * <p>
      * If {@code ignoreCase} is {@code true} then a case insensitive compare is done on the value.
      * @param name the name of the header to find
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 b18103bd849..ca45cb6a49e 100644
--- a/common/src/main/java/io/netty/util/internal/StringUtil.java
+++ b/common/src/main/java/io/netty/util/internal/StringUtil.java
@@ -34,6 +34,7 @@ public final class StringUtil {
     public static final char LINE_FEED = '\n';
     public static final char CARRIAGE_RETURN = '\r';
     public static final char TAB = '\t';
+    public static final char SPACE = 0x20;
 
     private static final String[] BYTE2HEX_PAD = new String[256];
     private static final String[] BYTE2HEX_NOPAD = new String[256];
@@ -48,16 +49,16 @@ public final class StringUtil {
     static {
         // Generate the lookup table that converts a byte into a 2-digit hexadecimal integer.
         int i;
-        for (i = 0; i < 10; i ++) {
+        for (i = 0; i < 10; i++) {
             BYTE2HEX_PAD[i] = "0" + i;
             BYTE2HEX_NOPAD[i] = String.valueOf(i);
         }
-        for (; i < 16; i ++) {
+        for (; i < 16; i++) {
             char c = (char) ('a' + i - 10);
             BYTE2HEX_PAD[i] = "0" + c;
             BYTE2HEX_NOPAD[i] = String.valueOf(c);
         }
-        for (; i < BYTE2HEX_PAD.length; i ++) {
+        for (; i < BYTE2HEX_PAD.length; i++) {
             String str = Integer.toHexString(i);
             BYTE2HEX_PAD[i] = str;
             BYTE2HEX_NOPAD[i] = str;
@@ -84,8 +85,8 @@ public static String substringAfter(String value, char delim) {
     /**
      * Checks if two strings have the same suffix of specified length
      *
-     * @param s            string
-     * @param p            string
+     * @param s   string
+     * @param p   string
      * @param len length of the common suffix
      * @return true if both s and p are not null and both have the same suffix. Otherwise - false
      */
@@ -138,7 +139,7 @@ public static <T extends Appendable> T toHexStringPadded(T dst, byte[] src) {
      */
     public static <T extends Appendable> T toHexStringPadded(T dst, byte[] src, int offset, int length) {
         final int end = offset + length;
-        for (int i = offset; i < end; i ++) {
+        for (int i = offset; i < end; i++) {
             byteToHexStringPadded(dst, src[i]);
         }
         return dst;
@@ -198,13 +199,13 @@ public static <T extends Appendable> T toHexString(T dst, byte[] src, int offset
         int i;
 
         // Skip preceding zeroes.
-        for (i = offset; i < endMinusOne; i ++) {
+        for (i = offset; i < endMinusOne; i++) {
             if (src[i] != 0) {
                 break;
             }
         }
 
-        byteToHexString(dst, src[i ++]);
+        byteToHexString(dst, src[i++]);
         int remaining = end - i;
         toHexStringPadded(dst, src, i, remaining);
 
@@ -244,22 +245,51 @@ public static String simpleClassName(Class<?> clazz) {
      * @return {@link CharSequence} the escaped value if necessary, or the value unchanged
      */
     public static CharSequence escapeCsv(CharSequence value) {
+        return escapeCsv(value, false);
+    }
+
+    /**
+     * Escapes the specified value, if necessary according to
+     * <a href="https://tools.ietf.org/html/rfc4180#section-2">RFC-4180</a>.
+     *
+     * @param value          The value which will be escaped according to
+     *                       <a href="https://tools.ietf.org/html/rfc4180#section-2">RFC-4180</a>
+     * @param trimWhiteSpace The value will first be trimmed of its optional white-space characters,
+     *                       according to <a href="https://tools.ietf.org/html/rfc7230#section-7">RFC-7230</a>
+     * @return {@link CharSequence} the escaped value if necessary, or the value unchanged
+     */
+    public static CharSequence escapeCsv(CharSequence value, boolean trimWhiteSpace) {
         int length = checkNotNull(value, "value").length();
         if (length == 0) {
             return value;
         }
+
+        int start = 0;
         int last = length - 1;
-        boolean quoted = isDoubleQuote(value.charAt(0)) && isDoubleQuote(value.charAt(last)) && length != 1;
+        boolean trimmed = false;
+        if (trimWhiteSpace) {
+            start = indexOfFirstNonOwsChar(value, length);
+            if (start == length) {
+                return EMPTY_STRING;
+            }
+            last = indexOfLastNonOwsChar(value, start, length);
+            trimmed = start > 0 || last < length - 1;
+            if (trimmed) {
+                length = last - start + 1;
+            }
+        }
+
+        StringBuilder result = new StringBuilder(length + CSV_NUMBER_ESCAPE_CHARACTERS);
+        boolean quoted = isDoubleQuote(value.charAt(start)) && isDoubleQuote(value.charAt(last)) && length != 1;
         boolean foundSpecialCharacter = false;
         boolean escapedDoubleQuote = false;
-        StringBuilder escaped = new StringBuilder(length + CSV_NUMBER_ESCAPE_CHARACTERS).append(DOUBLE_QUOTE);
-        for (int i = 0; i < length; i++) {
+        for (int i = start; i <= last; i++) {
             char current = value.charAt(i);
             switch (current) {
                 case DOUBLE_QUOTE:
-                    if (i == 0 || i == last) {
+                    if (i == start || i == last) {
                         if (!quoted) {
-                            escaped.append(DOUBLE_QUOTE);
+                            result.append(DOUBLE_QUOTE);
                         } else {
                             continue;
                         }
@@ -267,7 +297,7 @@ public static CharSequence escapeCsv(CharSequence value) {
                         boolean isNextCharDoubleQuote = isDoubleQuote(value.charAt(i + 1));
                         if (!isDoubleQuote(value.charAt(i - 1)) &&
                                 (!isNextCharDoubleQuote || i + 1 == last)) {
-                            escaped.append(DOUBLE_QUOTE);
+                            result.append(DOUBLE_QUOTE);
                             escapedDoubleQuote = true;
                         }
                         break;
@@ -277,10 +307,20 @@ public static CharSequence escapeCsv(CharSequence value) {
                 case COMMA:
                     foundSpecialCharacter = true;
             }
-            escaped.append(current);
+            result.append(current);
+        }
+
+        if (escapedDoubleQuote || foundSpecialCharacter && !quoted) {
+            return quote(result);
         }
-        return escapedDoubleQuote || foundSpecialCharacter && !quoted ?
-                escaped.append(DOUBLE_QUOTE) : value;
+        if (trimmed) {
+            return quoted ? quote(result) : result;
+        }
+        return value;
+    }
+
+    private static StringBuilder quote(StringBuilder builder) {
+        return builder.insert(0, DOUBLE_QUOTE).append(DOUBLE_QUOTE);
     }
 
     /**
@@ -390,7 +430,7 @@ public static List<CharSequence> unescapeCsvFields(CharSequence value) {
         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.
@@ -430,7 +470,8 @@ public static boolean isNullOrEmpty(String s) {
 
     /**
      * Find the index of the first non-white space character in {@code s} starting at {@code offset}.
-     * @param seq The string to search.
+     *
+     * @param seq    The string to search.
      * @param offset The offset to start searching at.
      * @return the index of the first non-white space character or &lt;{@code 0} if none was found.
      */
@@ -446,6 +487,7 @@ public static int indexOfNonWhiteSpace(CharSequence seq, int offset) {
     /**
      * Determine if {@code c} lies within the range of values defined for
      * <a href="http://unicode.org/glossary/#surrogate_code_point">Surrogate Code Point</a>.
+     *
      * @param c the character to check.
      * @return {@code true} if {@code c} lies within the range of values defined for
      * <a href="http://unicode.org/glossary/#surrogate_code_point">Surrogate Code Point</a>. {@code false} otherwise.
@@ -469,4 +511,47 @@ public static boolean endsWith(CharSequence s, char c) {
         int len = s.length();
         return len > 0 && s.charAt(len - 1) == c;
     }
+
+    /**
+     * Trim optional white-space characters from the specified value,
+     * according to <a href="https://tools.ietf.org/html/rfc7230#section-7">RFC-7230</a>.
+     *
+     * @param value the value to trim
+     * @return {@link CharSequence} the trimmed value if necessary, or the value unchanged
+     */
+    public static CharSequence trimOws(CharSequence value) {
+        final int length = value.length();
+        if (length == 0) {
+            return value;
+        }
+        int start = indexOfFirstNonOwsChar(value, length);
+        int end = indexOfLastNonOwsChar(value, start, length);
+        return start == 0 && end == length - 1 ? value : value.subSequence(start, end + 1);
+    }
+
+    /**
+     * @return {@code length} if no OWS is found.
+     */
+    private static int indexOfFirstNonOwsChar(CharSequence value, int length) {
+        int i = 0;
+        while (i < length && isOws(value.charAt(i))) {
+            i++;
+        }
+        return i;
+    }
+
+    /**
+     * @return {@code start} if no OWS is found.
+     */
+    private static int indexOfLastNonOwsChar(CharSequence value, int start, int length) {
+        int i = length - 1;
+        while (i > start && isOws(value.charAt(i))) {
+            i--;
+        }
+        return i;
+    }
+
+    private static boolean isOws(char c) {
+        return c == SPACE || c == TAB;
+    }
 }


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 6ec9bfaf36a..258208f4625 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
@@ -16,6 +16,7 @@
 package io.netty.handler.codec.http;
 
 import io.netty.handler.codec.http.HttpHeadersTestUtils.HeaderValue;
+import io.netty.util.internal.StringUtil;
 import org.junit.Test;
 
 import java.util.Arrays;
@@ -23,6 +24,8 @@
 
 import static io.netty.util.AsciiString.contentEquals;
 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 {
@@ -271,4 +274,32 @@ public void testGetAll() {
         headers.set(HEADER_NAME, "\"a,b,c\"");
         assertEquals(Arrays.asList("a,b,c"), headers.getAll(HEADER_NAME));
     }
+
+    @Test
+    public void owsTrimming() {
+        final CombinedHttpHeaders headers = newCombinedHttpHeaders();
+        headers.set(HEADER_NAME, Arrays.asList("\ta", "   ", "  b ", "\t \t"));
+        headers.add(HEADER_NAME, " c, d \t");
+
+        assertEquals(Arrays.asList("a", "", "b", "", "c, d"), headers.getAll(HEADER_NAME));
+        assertEquals("a,,b,,\"c, d\"", headers.get(HEADER_NAME));
+
+        assertTrue(headers.containsValue(HEADER_NAME, "a", true));
+        assertTrue(headers.containsValue(HEADER_NAME, " a ", true));
+        assertTrue(headers.containsValue(HEADER_NAME, "a", true));
+        assertFalse(headers.containsValue(HEADER_NAME, "a,b", true));
+
+        assertFalse(headers.containsValue(HEADER_NAME, " c, d ", true));
+        assertFalse(headers.containsValue(HEADER_NAME, "c, d", true));
+        assertTrue(headers.containsValue(HEADER_NAME, " c ", true));
+        assertTrue(headers.containsValue(HEADER_NAME, "d", true));
+
+        assertTrue(headers.containsValue(HEADER_NAME, "\t", true));
+        assertTrue(headers.containsValue(HEADER_NAME, "", true));
+
+        assertFalse(headers.containsValue(HEADER_NAME, "e", true));
+
+        HttpHeaders copiedHeaders = newCombinedHttpHeaders().add(headers);
+        assertEquals(Arrays.asList("a", "", "b", "", "c, d"), copiedHeaders.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 13016259e04..c8a13f58c06 100644
--- a/common/src/test/java/io/netty/util/internal/StringUtilTest.java
+++ b/common/src/test/java/io/netty/util/internal/StringUtilTest.java
@@ -19,9 +19,22 @@
 
 import java.util.Arrays;
 
-import static io.netty.util.internal.StringUtil.*;
-import static org.hamcrest.CoreMatchers.*;
-import static org.junit.Assert.*;
+import static io.netty.util.internal.StringUtil.NEWLINE;
+import static io.netty.util.internal.StringUtil.commonSuffixOfLength;
+import static io.netty.util.internal.StringUtil.simpleClassName;
+import static io.netty.util.internal.StringUtil.substringAfter;
+import static io.netty.util.internal.StringUtil.toHexString;
+import static io.netty.util.internal.StringUtil.toHexStringPadded;
+import static io.netty.util.internal.StringUtil.unescapeCsv;
+import static io.netty.util.internal.StringUtil.unescapeCsvFields;
+import static org.hamcrest.CoreMatchers.is;
+import static org.junit.Assert.assertArrayEquals;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertSame;
+import static org.junit.Assert.assertThat;
+import static org.junit.Assert.assertTrue;
 
 public class StringUtilTest {
 
@@ -331,13 +344,39 @@ public void escapeCsvWithCRLFCharacter() {
     }
 
     private static void escapeCsv(CharSequence value, CharSequence expected) {
+        escapeCsv(value, expected, false);
+    }
+
+    private static void escapeCsvWithTrimming(CharSequence value, CharSequence expected) {
+        escapeCsv(value, expected, true);
+    }
+
+    private static void escapeCsv(CharSequence value, CharSequence expected, boolean trimOws) {
         CharSequence escapedValue = value;
         for (int i = 0; i < 10; ++i) {
-            escapedValue = StringUtil.escapeCsv(escapedValue);
+            escapedValue = StringUtil.escapeCsv(escapedValue, trimOws);
             assertEquals(expected, escapedValue.toString());
         }
     }
 
+    @Test
+    public void escapeCsvWithTrimming() {
+        assertSame("", StringUtil.escapeCsv("", true));
+        assertSame("ab", StringUtil.escapeCsv("ab", true));
+
+        escapeCsvWithTrimming("", "");
+        escapeCsvWithTrimming(" \t ", "");
+        escapeCsvWithTrimming("ab", "ab");
+        escapeCsvWithTrimming("a b", "a b");
+        escapeCsvWithTrimming(" \ta \tb", "a \tb");
+        escapeCsvWithTrimming("a \tb \t", "a \tb");
+        escapeCsvWithTrimming("\t a \tb \t", "a \tb");
+        escapeCsvWithTrimming("\"\t a b \"", "\"\t a b \"");
+        escapeCsvWithTrimming(" \"\t a b \"\t", "\"\t a b \"");
+        escapeCsvWithTrimming(" testing\t\n ", "\"testing\t\n\"");
+        escapeCsvWithTrimming("\ttest,ing ", "\"test,ing\"");
+    }
+
     @Test
     public void testUnescapeCsv() {
         assertEquals("", unescapeCsv(""));
@@ -465,4 +504,21 @@ public void testEndsWith() {
         assertFalse(StringUtil.endsWith("-", 'u'));
         assertFalse(StringUtil.endsWith("u-", 'u'));
     }
+
+    @Test
+    public void trimOws() {
+        assertSame("", StringUtil.trimOws(""));
+        assertEquals("", StringUtil.trimOws(" \t "));
+        assertSame("a", StringUtil.trimOws("a"));
+        assertEquals("a", StringUtil.trimOws(" a"));
+        assertEquals("a", StringUtil.trimOws("a "));
+        assertEquals("a", StringUtil.trimOws(" a "));
+        assertSame("abc", StringUtil.trimOws("abc"));
+        assertEquals("abc", StringUtil.trimOws("\tabc"));
+        assertEquals("abc", StringUtil.trimOws("abc\t"));
+        assertEquals("abc", StringUtil.trimOws("\tabc\t"));
+        assertSame("a\t b", StringUtil.trimOws("a\t b"));
+        assertEquals("", StringUtil.trimOws("\t ").toString());
+        assertEquals("a b", StringUtil.trimOws("\ta b \t").toString());
+    }
 }