Drop broken DefaultCookie name validation, close #4999

Motivation:

DefaultCookie constructor performs a name validation that doesn’t match
RFC6265. Moreover, such validation is already performed in strict
encoders and decoders.

Modifications:

Drop DefaultCookie name validation, rely on encoders and decoders.

Result:

no more duplicate broken validation

codec-http/src/main/java/io/netty/handler/codec/http/cookie/CookieUtil.java

@@ -22,9 +22,29 @@ import java.util.BitSet;
 
 final class CookieUtil {
 
+    private static final BitSet VALID_COOKIE_NAME_OCTETS = validCookieNameOctets();
+
     private static final BitSet VALID_COOKIE_VALUE_OCTETS = validCookieValueOctets();
 
-    private static final BitSet VALID_COOKIE_NAME_OCTETS = validCookieNameOctets();
+    private static final BitSet VALID_COOKIE_ATTRIBUTE_VALUE_OCTETS = validCookieAttributeValueOctets();
+
+    // token = 1*<any CHAR except CTLs or separators>
+    // separators = "(" | ")" | "<" | ">" | "@"
+    // | "," | ";" | ":" | "\" | <">
+    // | "/" | "[" | "]" | "?" | "="
+    // | "{" | "}" | SP | HT
+    private static BitSet validCookieNameOctets() {
+        BitSet bits = new BitSet();
+        for (int i = 32; i < 127; i++) {
+            bits.set(i);
+        }
+        int[] separators = new int[]
+                { '(', ')', '<', '>', '@', ',', ';', ':', '\\', '"', '/', '[', ']', '?', '=', '{', '}', ' ', '\t' };
+        for (int separator : separators) {
+            bits.set(separator, false);
+        }
+        return bits;
+    }
 
     // cookie-octet = %x21 / %x23-2B / %x2D-3A / %x3C-5B / %x5D-7E
     // US-ASCII characters excluding CTLs, whitespace, DQUOTE, comma, semicolon, and backslash
@@ -46,21 +66,13 @@ final class CookieUtil {
         return bits;
     }
 
-    // token = 1*<any CHAR except CTLs or separators>
+    // path-value        = <any CHAR except CTLs or ";">
-    // separators = "(" | ")" | "<" | ">" | "@"
+    private static BitSet validCookieAttributeValueOctets() {
-    // | "," | ";" | ":" | "\" | <">
-    // | "/" | "[" | "]" | "?" | "="
-    // | "{" | "}" | SP | HT
-    private static BitSet validCookieNameOctets() {
         BitSet bits = new BitSet();
         for (int i = 32; i < 127; i++) {
             bits.set(i);
         }
-        int[] separators = new int[]
+        bits.set(';', false);
-                { '(', ')', '<', '>', '@', ',', ';', ':', '\\', '"', '/', '[', ']', '?', '=', '{', '}', ' ', '\t' };
-        for (int separator : separators) {
-            bits.set(separator, false);
-        }
         return bits;
     }
 
@@ -150,6 +162,21 @@ final class CookieUtil {
         return cs;
     }
 
+    static String validateAttributeValue(String name, String value) {
+        if (value == null) {
+            return null;
+        }
+        value = value.trim();
+        if (value.isEmpty()) {
+            return null;
+        }
+        int i = firstInvalidOctet(value, VALID_COOKIE_ATTRIBUTE_VALUE_OCTETS);
+        if (i != -1) {
+            throw new IllegalArgumentException(name + " contains the prohibited characters: " + value.charAt(i));
+        }
+        return value;
+    }
+
     private CookieUtil() {
         // Unused
     }

codec-http/src/main/java/io/netty/handler/codec/http/cookie/DefaultCookie.java

@@ -15,7 +15,7 @@
  */
 package io.netty.handler.codec.http.cookie;
 
-import static io.netty.handler.codec.http.cookie.CookieUtil.stringBuilder;
+import static io.netty.handler.codec.http.cookie.CookieUtil.*;
 import static io.netty.util.internal.ObjectUtil.checkNotNull;
 
 /**
@@ -40,28 +40,6 @@ public class DefaultCookie implements Cookie {
         if (name.isEmpty()) {
             throw new IllegalArgumentException("empty name");
         }
-
-        for (int i = 0; i < name.length(); i ++) {
-            char c = name.charAt(i);
-            if (c > 127) {
-                throw new IllegalArgumentException(
-                        "name contains non-ascii character: " + name);
-            }
-
-            // Check prohibited characters.
-            switch (c) {
-            case '\t': case '\n': case 0x0b: case '\f': case '\r':
-            case ' ':  case ',':  case ';':  case '=':
-                throw new IllegalArgumentException(
-                        "name contains one of the following prohibited characters: " +
-                        "=,; \\t\\r\\n\\v\\f: " + name);
-            }
-        }
-
-        if (name.charAt(0) == '$') {
-            throw new IllegalArgumentException("name starting with '$' not allowed: " + name);
-        }
-
         this.name = name;
         setValue(value);
     }
@@ -98,7 +76,7 @@ public class DefaultCookie implements Cookie {
 
     @Override
     public void setDomain(String domain) {
-        this.domain = validateValue("domain", domain);
+        this.domain = validateAttributeValue("domain", domain);
     }
 
     @Override
@@ -108,7 +86,7 @@ public class DefaultCookie implements Cookie {
 
     @Override
     public void setPath(String path) {
-        this.path = validateValue("path", path);
+        this.path = validateAttributeValue("path", path);
     }
 
     @Override
@@ -218,6 +196,19 @@ public class DefaultCookie implements Cookie {
         return 0;
     }
 
+    /**
+     * Validate a cookie attribute value, throws a {@link IllegalArgumentException} otherwise.
+     * Only intended to be used by {@link io.netty.handler.codec.http.DefaultCookie}.
+     * @param name attribute name
+     * @param value attribute value
+     * @return the trimmed, validated attribute value
+     * @deprecated CookieUtil is package private, will be removed once old Cookie API is dropped
+     */
+    @Deprecated
+    protected String validateValue(String name, String value) {
+        return validateAttributeValue(name, value);
+    }
+
     @Override
     public String toString() {
         StringBuilder buf = stringBuilder()
@@ -245,24 +236,4 @@ public class DefaultCookie implements Cookie {
         }
         return buf.toString();
     }
-
-    protected String validateValue(String name, String value) {
-        if (value == null) {
-            return null;
-        }
-        value = value.trim();
-        if (value.isEmpty()) {
-            return null;
-        }
-        for (int i = 0; i < value.length(); i ++) {
-            char c = value.charAt(i);
-            switch (c) {
-            case '\r': case '\n': case '\f': case 0x0b: case ';':
-                throw new IllegalArgumentException(
-                        name + " contains one of the following prohibited characters: " +
-                        ";\\r\\n\\f\\v (" + value + ')');
-            }
-        }
-        return value;
-    }
 }

codec-http/src/test/java/io/netty/handler/codec/http/cookie/ServerCookieEncoderTest.java

@@ -79,8 +79,34 @@ public class ServerCookieEncoderTest {
     }
 
     @Test
-    public void illegalCharInCookieValueMakesStrictEncoderThrowsException() {
+    public void illegalCharInCookieNameMakesStrictEncoderThrowsException() {
+        Set<Character> illegalChars = new HashSet<Character>();
+        // CTLs
+        for (int i = 0x00; i <= 0x1F; i++) {
+            illegalChars.add((char) i);
+        }
+        illegalChars.add((char) 0x7F);
+        // separators
+        for (char c : new char[] { '(', ')', '<', '>', '@', ',', ';', ':', '\\', '"', '/', '[', ']',
+                '?', '=', '{', '}', ' ', '\t' }) {
+            illegalChars.add(c);
+        }
 
+        int exceptions = 0;
+
+        for (char c : illegalChars) {
+            try {
+                ServerCookieEncoder.STRICT.encode(new DefaultCookie("foo" + c + "bar", "value"));
+            } catch (IllegalArgumentException e) {
+                exceptions++;
+            }
+        }
+
+        assertEquals(illegalChars.size(), exceptions);
+    }
+
+    @Test
+    public void illegalCharInCookieValueMakesStrictEncoderThrowsException() {
         Set<Character> illegalChars = new HashSet<Character>();
         // CTLs
         for (int i = 0x00; i <= 0x1F; i++) {
@@ -95,9 +121,8 @@ public class ServerCookieEncoderTest {
         int exceptions = 0;
 
         for (char c : illegalChars) {
-            Cookie cookie = new DefaultCookie("name", "value" + c);
             try {
-                ServerCookieEncoder.STRICT.encode(cookie);
+                ServerCookieEncoder.STRICT.encode(new DefaultCookie("name", "value" + c));
             } catch (IllegalArgumentException e) {
                 exceptions++;
             }