From 1d9c58baa18f9332f6e9af245d12b0ccc5004e7c Mon Sep 17 00:00:00 2001 From: Stephane Landelle Date: Fri, 18 Mar 2016 17:40:28 +0100 Subject: [PATCH] Drop broken DefaultCookie name validation, close #4999 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../handler/codec/http/cookie/CookieUtil.java | 51 ++++++++++++---- .../codec/http/cookie/DefaultCookie.java | 61 +++++-------------- .../http/cookie/ServerCookieEncoderTest.java | 31 +++++++++- 3 files changed, 83 insertions(+), 60 deletions(-) diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/cookie/CookieUtil.java b/codec-http/src/main/java/io/netty/handler/codec/http/cookie/CookieUtil.java index 699587c858..1e9d9c8f87 100644 --- a/codec-http/src/main/java/io/netty/handler/codec/http/cookie/CookieUtil.java +++ b/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* + // 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* - // separators = "(" | ")" | "<" | ">" | "@" - // | "," | ";" | ":" | "\" | <"> - // | "/" | "[" | "]" | "?" | "=" - // | "{" | "}" | SP | HT - private static BitSet validCookieNameOctets() { + // path-value = + private static BitSet validCookieAttributeValueOctets() { 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); - } + bits.set(';', 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 } diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/cookie/DefaultCookie.java b/codec-http/src/main/java/io/netty/handler/codec/http/cookie/DefaultCookie.java index c5de61c9d4..fb98555bb8 100644 --- a/codec-http/src/main/java/io/netty/handler/codec/http/cookie/DefaultCookie.java +++ b/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; - } } diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/cookie/ServerCookieEncoderTest.java b/codec-http/src/test/java/io/netty/handler/codec/http/cookie/ServerCookieEncoderTest.java index 0ff4ed73be..d171679b6c 100644 --- a/codec-http/src/test/java/io/netty/handler/codec/http/cookie/ServerCookieEncoderTest.java +++ b/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 illegalChars = new HashSet(); + // 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 illegalChars = new HashSet(); // 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++; }