From 4978266d52a90252ae00b40894a4398292830d7f Mon Sep 17 00:00:00 2001 From: Luke Hutchison Date: Thu, 26 Nov 2015 03:51:52 -0800 Subject: [PATCH] Make cookie encoding conform better to RFC 6265 in STRICT mode. Motivation: - On the client, cookies should be sorted in decreasing order of path length. From RFC 6265: 5.4.2. The user agent SHOULD sort the cookie-list in the following order: * Cookies with longer paths are listed before cookies with shorter paths. * Among cookies that have equal-length path fields, cookies with earlier creation-times are listed before cookies with later creation-times. NOTE: Not all user agents sort the cookie-list in this order, but this order reflects common practice when this document was written, and, historically, there have been servers that (erroneously) depended on this order. Note that the RFC does not define the path length of cookies without a path. We sort pathless cookies before cookies with the longest path, since pathless cookies inherit the request path (and setting a path that is longer than the request path is of limited use, since it cannot be read from the context in which it is written). - On the server, if there are multiple cookies of the same name, only one of them should be encoded. RFC 6265 says: Servers SHOULD NOT include more than one Set-Cookie header field in the same response with the same cookie-name. Note that the RFC does not define which cookie should be set in the case of multiple cookies with the same name; we arbitrarily pick the last one. Modifications: - Changed the visibility of the 'strict' field to 'protected' in CookieEncoder. - Modified ClientCookieEncoder to sort cookies in decreasing order of path length when in strict mode. - Modified ServerCookieEncoder to return only the last cookie of a given name when in strict mode. - Added a fast path for both strict mode in both client and server code for cases with only one cookie, in order avoid the overhead of sorting and memory allocation. - Added unit tests for the new cases. Result: - Cookie generation on client and server is now more conformant to RFC 6265. --- .../http/cookie/ClientCookieEncoder.java | 137 +++++++++++++++--- .../codec/http/cookie/CookieEncoder.java | 2 +- .../http/cookie/ServerCookieEncoder.java | 79 +++++++--- .../http/cookie/ClientCookieEncoderTest.java | 20 +-- .../http/cookie/ServerCookieEncoderTest.java | 34 ++++- 5 files changed, 217 insertions(+), 55 deletions(-) diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/cookie/ClientCookieEncoder.java b/codec-http/src/main/java/io/netty/handler/codec/http/cookie/ClientCookieEncoder.java index 305c738075..fcb444d28c 100644 --- a/codec-http/src/main/java/io/netty/handler/codec/http/cookie/ClientCookieEncoder.java +++ b/codec-http/src/main/java/io/netty/handler/codec/http/cookie/ClientCookieEncoder.java @@ -15,16 +15,24 @@ */ package io.netty.handler.codec.http.cookie; -import static io.netty.handler.codec.http.cookie.CookieUtil.*; +import static io.netty.handler.codec.http.cookie.CookieUtil.add; +import static io.netty.handler.codec.http.cookie.CookieUtil.addQuoted; +import static io.netty.handler.codec.http.cookie.CookieUtil.stringBuilder; +import static io.netty.handler.codec.http.cookie.CookieUtil.stripTrailingSeparator; +import static io.netty.handler.codec.http.cookie.CookieUtil.stripTrailingSeparatorOrNull; import static io.netty.util.internal.ObjectUtil.checkNotNull; - -import java.util.Iterator; - import io.netty.handler.codec.http.HttpRequest; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collection; +import java.util.Comparator; +import java.util.Iterator; +import java.util.List; + /** - * A RFC6265 compliant cookie encoder to be used client side, - * so only name=value pairs are sent. + * A RFC6265 compliant cookie encoder to be used client side, so + * only name=value pairs are sent. * * User-Agents are not supposed to interpret cookies, so, if present, {@link Cookie#rawValue()} will be used. * Otherwise, {@link Cookie#value()} will be used unquoted. @@ -42,13 +50,14 @@ import io.netty.handler.codec.http.HttpRequest; public final class ClientCookieEncoder extends CookieEncoder { /** - * Strict encoder that validates that name and value chars are in the valid scope - * defined in RFC6265 + * Strict encoder that validates that name and value chars are in the valid scope and (for methods that accept + * multiple cookies) sorts cookies into order of decreasing path length, as specified in RFC6265. */ public static final ClientCookieEncoder STRICT = new ClientCookieEncoder(true); /** - * Lax instance that doesn't validate name and value + * Lax instance that doesn't validate name and value, and (for methods that accept multiple cookies) keeps + * cookies in the order in which they were given. */ public static final ClientCookieEncoder LAX = new ClientCookieEncoder(false); @@ -59,8 +68,10 @@ public final class ClientCookieEncoder extends CookieEncoder { /** * Encodes the specified cookie into a Cookie header value. * - * @param name the cookie name - * @param value the cookie value + * @param name + * the cookie name + * @param value + * the cookie value * @return a Rfc6265 style Cookie header value */ public String encode(String name, String value) { @@ -70,7 +81,8 @@ public final class ClientCookieEncoder extends CookieEncoder { /** * Encodes the specified cookie into a Cookie header value. * - * @param specified the cookie + * @param specified + * the cookie * @return a Rfc6265 style Cookie header value */ public String encode(Cookie cookie) { @@ -79,10 +91,37 @@ public final class ClientCookieEncoder extends CookieEncoder { return stripTrailingSeparator(buf); } + /** + * Sort cookies into decreasing order of path length, breaking ties by sorting into increasing chronological + * order of creation time, as recommended by RFC 6265. + */ + private static final Comparator COOKIE_COMPARATOR = new Comparator() { + @Override + public int compare(Cookie c1, Cookie c2) { + String path1 = c1.path(); + String path2 = c2.path(); + // Cookies with unspecified path default to the path of the request. We don't + // know the request path here, but we assume that the length of an unspecified + // path is longer than any specified path (i.e. pathless cookies come first), + // because setting cookies with a path longer than the request path is of + // limited use. + int len1 = path1 == null ? Integer.MAX_VALUE : path1.length(); + int len2 = path2 == null ? Integer.MAX_VALUE : path2.length(); + int diff = len2 - len1; + if (diff != 0) { + return diff; + } + // Rely on Java's sort stability to retain creation order in cases where + // cookies have same path length. + return -1; + } + }; + /** * Encodes the specified cookies into a single Cookie header value. * - * @param cookies some cookies + * @param cookies + * some cookies * @return a Rfc6265 style Cookie header value, null if no cookies are passed. */ public String encode(Cookie... cookies) { @@ -91,12 +130,51 @@ public final class ClientCookieEncoder extends CookieEncoder { } StringBuilder buf = stringBuilder(); - for (Cookie c : cookies) { - if (c == null) { - break; + if (strict) { + if (cookies.length == 1) { + encode(buf, cookies[0]); + } else { + Cookie[] cookiesSorted = Arrays.copyOf(cookies, cookies.length); + Arrays.sort(cookiesSorted, COOKIE_COMPARATOR); + for (Cookie c : cookiesSorted) { + encode(buf, c); + } } + } else { + for (Cookie c : cookies) { + encode(buf, c); + } + } + return stripTrailingSeparatorOrNull(buf); + } - encode(buf, c); + /** + * Encodes the specified cookies into a single Cookie header value. + * + * @param cookies + * some cookies + * @return a Rfc6265 style Cookie header value, null if no cookies are passed. + */ + public String encode(Collection cookies) { + if (checkNotNull(cookies, "cookies").isEmpty()) { + return null; + } + + StringBuilder buf = stringBuilder(); + if (strict) { + if (cookies.size() == 1) { + encode(buf, cookies.iterator().next()); + } else { + Cookie[] cookiesSorted = cookies.toArray(new Cookie[cookies.size()]); + Arrays.sort(cookiesSorted, COOKIE_COMPARATOR); + for (Cookie c : cookiesSorted) { + encode(buf, c); + } + } + } else { + for (Cookie c : cookies) { + encode(buf, c); + } } return stripTrailingSeparatorOrNull(buf); } @@ -114,13 +192,26 @@ public final class ClientCookieEncoder extends CookieEncoder { } StringBuilder buf = stringBuilder(); - while (cookiesIt.hasNext()) { - Cookie c = cookiesIt.next(); - if (c == null) { - break; + if (strict) { + Cookie firstCookie = cookiesIt.next(); + if (!cookiesIt.hasNext()) { + encode(buf, firstCookie); + } else { + List cookiesList = new ArrayList(); + cookiesList.add(firstCookie); + while (cookiesIt.hasNext()) { + cookiesList.add(cookiesIt.next()); + } + Cookie[] cookiesSorted = cookiesList.toArray(new Cookie[cookiesList.size()]); + Arrays.sort(cookiesSorted, COOKIE_COMPARATOR); + for (Cookie c : cookiesSorted) { + encode(buf, c); + } + } + } else { + while (cookiesIt.hasNext()) { + encode(buf, cookiesIt.next()); } - - encode(buf, c); } return stripTrailingSeparatorOrNull(buf); } diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/cookie/CookieEncoder.java b/codec-http/src/main/java/io/netty/handler/codec/http/cookie/CookieEncoder.java index 1748f0a120..d648768152 100644 --- a/codec-http/src/main/java/io/netty/handler/codec/http/cookie/CookieEncoder.java +++ b/codec-http/src/main/java/io/netty/handler/codec/http/cookie/CookieEncoder.java @@ -24,7 +24,7 @@ import static io.netty.handler.codec.http.cookie.CookieUtil.unwrapValue; */ public abstract class CookieEncoder { - private final boolean strict; + protected final boolean strict; protected CookieEncoder(boolean strict) { this.strict = strict; diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/cookie/ServerCookieEncoder.java b/codec-http/src/main/java/io/netty/handler/codec/http/cookie/ServerCookieEncoder.java index 17376903f3..4a832cec35 100644 --- a/codec-http/src/main/java/io/netty/handler/codec/http/cookie/ServerCookieEncoder.java +++ b/codec-http/src/main/java/io/netty/handler/codec/http/cookie/ServerCookieEncoder.java @@ -15,9 +15,11 @@ */ package io.netty.handler.codec.http.cookie; -import static io.netty.handler.codec.http.cookie.CookieUtil.*; +import static io.netty.handler.codec.http.cookie.CookieUtil.add; +import static io.netty.handler.codec.http.cookie.CookieUtil.addQuoted; +import static io.netty.handler.codec.http.cookie.CookieUtil.stringBuilder; +import static io.netty.handler.codec.http.cookie.CookieUtil.stripTrailingSeparator; import static io.netty.util.internal.ObjectUtil.checkNotNull; - import io.netty.handler.codec.http.HttpHeaderDateFormat; import io.netty.handler.codec.http.HttpRequest; @@ -25,7 +27,10 @@ import java.util.ArrayList; import java.util.Collection; import java.util.Collections; import java.util.Date; +import java.util.HashMap; +import java.util.Iterator; import java.util.List; +import java.util.Map; /** * A RFC6265 compliant cookie encoder to be used server side, @@ -47,12 +52,15 @@ public final class ServerCookieEncoder extends CookieEncoder { /** * Strict encoder that validates that name and value chars are in the valid scope - * defined in RFC6265 + * defined in RFC6265, and (for methods that accept multiple cookies) that only + * one cookie is encoded with any given name. (If multiple cookies have the same + * name, the last one is the one that is encoded.) */ public static final ServerCookieEncoder STRICT = new ServerCookieEncoder(true); /** - * Lax instance that doesn't validate name and value + * Lax instance that doesn't validate name and value, and that allows multiple + * cookies with the same name. */ public static final ServerCookieEncoder LAX = new ServerCookieEncoder(false); @@ -114,6 +122,26 @@ public final class ServerCookieEncoder extends CookieEncoder { return stripTrailingSeparator(buf); } + /** Deduplicate a list of encoded cookies by keeping only the last instance with a given name. + * + * @param encoded The list of encoded cookies. + * @param nameToLastIndex A map from cookie name to index of last cookie instance. + * @return The encoded list with all but the last instance of a named cookie. + */ + private List dedup(List encoded, Map nameToLastIndex) { + boolean[] isLastInstance = new boolean[encoded.size()]; + for (int idx : nameToLastIndex.values()) { + isLastInstance[idx] = true; + } + List dedupd = new ArrayList(nameToLastIndex.size()); + for (int i = 0, n = encoded.size(); i < n; i++) { + if (isLastInstance[i]) { + dedupd.add(encoded.get(i)); + } + } + return dedupd; + } + /** * Batch encodes cookies into Set-Cookie header values. * @@ -126,13 +154,16 @@ public final class ServerCookieEncoder extends CookieEncoder { } List encoded = new ArrayList(cookies.length); - for (Cookie c : cookies) { - if (c == null) { - break; - } + Map nameToIndex = strict && cookies.length > 1 ? new HashMap() : null; + boolean hasDupdName = false; + for (int i = 0; i < cookies.length; i++) { + Cookie c = cookies[i]; encoded.add(encode(c)); + if (nameToIndex != null) { + hasDupdName |= nameToIndex.put(c.name(), i) != null; + } } - return encoded; + return hasDupdName ? dedup(encoded, nameToIndex) : encoded; } /** @@ -147,13 +178,16 @@ public final class ServerCookieEncoder extends CookieEncoder { } List encoded = new ArrayList(cookies.size()); + Map nameToIndex = strict && cookies.size() > 1 ? new HashMap() : null; + int i = 0; + boolean hasDupdName = false; for (Cookie c : cookies) { - if (c == null) { - break; - } encoded.add(encode(c)); + if (nameToIndex != null) { + hasDupdName |= nameToIndex.put(c.name(), i++) != null; + } } - return encoded; + return hasDupdName ? dedup(encoded, nameToIndex) : encoded; } /** @@ -163,17 +197,24 @@ public final class ServerCookieEncoder extends CookieEncoder { * @return the corresponding bunch of Set-Cookie headers */ public List encode(Iterable cookies) { - if (!checkNotNull(cookies, "cookies").iterator().hasNext()) { + Iterator cookiesIt = checkNotNull(cookies, "cookies").iterator(); + if (!cookiesIt.hasNext()) { return Collections.emptyList(); } List encoded = new ArrayList(); - for (Cookie c : cookies) { - if (c == null) { - break; - } + Cookie firstCookie = cookiesIt.next(); + Map nameToIndex = strict && cookiesIt.hasNext() ? new HashMap() : null; + int i = 0; + encoded.add(encode(firstCookie)); + boolean hasDupdName = nameToIndex != null ? nameToIndex.put(firstCookie.name(), i++) != null : false; + while (cookiesIt.hasNext()) { + Cookie c = cookiesIt.next(); encoded.add(encode(c)); + if (nameToIndex != null) { + hasDupdName |= nameToIndex.put(c.name(), i++) != null; + } } - return encoded; + return hasDupdName ? dedup(encoded, nameToIndex) : encoded; } } diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/cookie/ClientCookieEncoderTest.java b/codec-http/src/test/java/io/netty/handler/codec/http/cookie/ClientCookieEncoderTest.java index f549b9eb1b..af81059dd8 100644 --- a/codec-http/src/test/java/io/netty/handler/codec/http/cookie/ClientCookieEncoderTest.java +++ b/codec-http/src/test/java/io/netty/handler/codec/http/cookie/ClientCookieEncoderTest.java @@ -23,21 +23,23 @@ public class ClientCookieEncoderTest { @Test public void testEncodingMultipleClientCookies() { - String c1 = "myCookie=myValue; "; - String c2 = "myCookie2=myValue2; "; + String c1 = "myCookie=myValue"; + String c2 = "myCookie2=myValue2"; String c3 = "myCookie3=myValue3"; - Cookie cookie = new DefaultCookie("myCookie", "myValue"); - cookie.setDomain(".adomainsomewhere"); - cookie.setMaxAge(50); - cookie.setPath("/apathsomewhere"); - cookie.setSecure(true); + Cookie cookie1 = new DefaultCookie("myCookie", "myValue"); + cookie1.setDomain(".adomainsomewhere"); + cookie1.setMaxAge(50); + cookie1.setPath("/apathsomewhere"); + cookie1.setSecure(true); Cookie cookie2 = new DefaultCookie("myCookie2", "myValue2"); cookie2.setDomain(".anotherdomainsomewhere"); cookie2.setPath("/anotherpathsomewhere"); cookie2.setSecure(false); Cookie cookie3 = new DefaultCookie("myCookie3", "myValue3"); - String encodedCookie = ClientCookieEncoder.STRICT.encode(cookie, cookie2, cookie3); - assertEquals(c1 + c2 + c3, encodedCookie); + String encodedCookie = ClientCookieEncoder.STRICT.encode(cookie1, cookie2, cookie3); + // Cookies should be sorted into decreasing order of path length, as per RFC6265. + // When no path is provided, we assume maximum path length (so cookie3 comes first). + assertEquals(c3 + "; " + c2 + "; " + c1, encodedCookie); } @Test 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 350499046a..58ca662d15 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 @@ -15,17 +15,20 @@ */ package io.netty.handler.codec.http.cookie; -import org.junit.Test; - +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; import io.netty.handler.codec.http.HttpHeaderDateFormat; import java.text.ParseException; +import java.util.ArrayList; import java.util.Date; import java.util.List; import java.util.regex.Matcher; import java.util.regex.Pattern; -import static org.junit.Assert.*; +import org.junit.Test; public class ServerCookieEncoderTest { @@ -60,4 +63,29 @@ public class ServerCookieEncoderTest { assertNotNull(encodedCookie2); assertTrue(encodedCookie2.isEmpty()); } + + @Test + public void testEncodingMultipleCookiesStrict() { + List result = new ArrayList(); + result.add("cookie2=value2"); + result.add("cookie1=value3"); + Cookie cookie1 = new DefaultCookie("cookie1", "value1"); + Cookie cookie2 = new DefaultCookie("cookie2", "value2"); + Cookie cookie3 = new DefaultCookie("cookie1", "value3"); + List encodedCookies = ServerCookieEncoder.STRICT.encode(cookie1, cookie2, cookie3); + assertEquals(result, encodedCookies); + } + + @Test + public void testEncodingMultipleCookiesLax() { + List result = new ArrayList(); + result.add("cookie1=value1"); + result.add("cookie2=value2"); + result.add("cookie1=value3"); + Cookie cookie1 = new DefaultCookie("cookie1", "value1"); + Cookie cookie2 = new DefaultCookie("cookie2", "value2"); + Cookie cookie3 = new DefaultCookie("cookie1", "value3"); + List encodedCookies = ServerCookieEncoder.LAX.encode(cookie1, cookie2, cookie3); + assertEquals(result, encodedCookies); + } }