Improvements for Set-Cookie parsing, allow lax parsing of older specs

servicetalk-http-api/build.gradle

@@ -43,6 +43,7 @@ dependencies {
   implementation "com.google.code.findbugs:jsr305"
   implementation "org.slf4j:slf4j-api"
 
+  testImplementation testFixtures(project(":servicetalk-buffer-api"))
   testImplementation testFixtures(project(":servicetalk-concurrent-api"))
   testImplementation testFixtures(project(":servicetalk-concurrent-internal"))
   testImplementation testFixtures(project(":servicetalk-transport-netty-internal"))

servicetalk-http-api/src/main/java/io/servicetalk/http/api/DefaultHttpSetCookie.java

@@ -27,6 +27,7 @@
 import static io.servicetalk.http.api.HttpSetCookie.SameSite.Lax;
 import static io.servicetalk.http.api.HttpSetCookie.SameSite.None;
 import static io.servicetalk.http.api.HttpSetCookie.SameSite.Strict;
+import static java.lang.Integer.toHexString;
 
 /**
  * Default implementation of {@link HttpSetCookie}.
@@ -162,7 +163,8 @@ static HttpSetCookie parseSetCookie(final CharSequence setCookieString, boolean
                         ++i;
                         break;
                     } else {
-                        throw new IllegalArgumentException("unexpected = at index: " + i);
+                        throw new IllegalArgumentException(
+                                "set-cookie '" + name + "': unexpected = character at index " + i);
                     }
                     ++i;
                     begin = i;
@@ -172,14 +174,28 @@ static HttpSetCookie parseSetCookie(final CharSequence setCookieString, boolean
                         if (isWrapped) {
                             parseState = ParseState.Unknown;
                             value = setCookieString.subSequence(begin, i);
-                            if (validateContent) {
-                                // Increment by 3 because we are skipping DQUOTE SEMI SP
-                                // See https://www.rfc-editor.org/rfc/rfc6265#section-4.1.1
-                                // Specifically, how set-cookie-string interacts with quoted cookie-value.
-                                i += 3;
+                            // Increment by 3 because we are skipping DQUOTE SEMI SP
+                            // See https://www.rfc-editor.org/rfc/rfc6265#section-4.1.1
+                            // Specifically, how set-cookie-string interacts with quoted cookie-value.
+                            ++i;    // go to SEMI
+                            if (i < length && setCookieString.charAt(i) != ';') {
+                                throw new IllegalArgumentException(
+                                        "set-cookie '" + name + "': unexpected character 0x" +
+                                                toHexString(setCookieString.charAt(i)) + " at index " + i +
+                                                ", expected semicolon ';' after quoted value");
+                            }
+                            ++i;    // go to SP
+                            if (validateContent && HeaderUtils.cookieParsingStrictRfc6265()) {
+                                if (i < length && setCookieString.charAt(i) != ' ') {
+                                    throw new IllegalArgumentException("set-cookie '" + name +
+                                            "': a space is required after ; in cookie attribute-value lists");
+                                }
+                                ++i;    // skip SP
                             } else {
                                 // When validation is disabled, we need to check if there's an SP to skip
-                                i += i + 2 < length && setCookieString.charAt(i + 2) == ' ' ? 3 : 2;
+                                if (i < length && setCookieString.charAt(i) == ' ') {
+                                    ++i;
+                                }
                             }
                         } else {
                             isWrapped = true;
@@ -196,7 +212,8 @@ static HttpSetCookie parseSetCookie(final CharSequence setCookieString, boolean
                 case ';':
                     // end of value, or end of av-value
                     if (i + 1 == length && validateContent) {
-                        throw new IllegalArgumentException("unexpected trailing ';'");
+                        throw new IllegalArgumentException(
+                                "set-cookie '" + name + "' unexpected trailing semicolon ';'");
                     }
                     switch (parseState) {
                         case ParsingValue:
@@ -219,7 +236,7 @@ static HttpSetCookie parseSetCookie(final CharSequence setCookieString, boolean
                             break;
                         default:
                             if (name == null) {
-                                throw new IllegalArgumentException("cookie value not found at index " + i);
+                                throw new IllegalArgumentException("cookie name cannot be null or empty");
                             }
                             final CharSequence avName = setCookieString.subSequence(begin, i);
                             if (contentEqualsIgnoreCase(avName, "secure")) {
@@ -230,10 +247,10 @@ static HttpSetCookie parseSetCookie(final CharSequence setCookieString, boolean
                             break;
                     }
                     parseState = ParseState.Unknown;
-                    if (validateContent) {
+                    if (validateContent && HeaderUtils.cookieParsingStrictRfc6265()) {
                         if (i + 1 >= length || ' ' != setCookieString.charAt(i + 1)) {
-                            throw new IllegalArgumentException(
-                                    "a space is required after ; in cookie attribute-value lists");
+                            throw new IllegalArgumentException("set-cookie '" + name +
+                                    "': a space is required after ; in cookie attribute-value lists");
                         }
                         i += 2;
                     } else {
@@ -249,10 +266,10 @@ static HttpSetCookie parseSetCookie(final CharSequence setCookieString, boolean
                         if (parseState == ParseState.ParsingValue) {
                             // Cookie values need to conform to the cookie-octet rule of
                             // https://www.rfc-editor.org/rfc/rfc6265#section-4.1.1
-                            validateCookieOctetHexValue(c, i);
+                            validateCookieOctetHexValue(name, c, i);
                         } else {
                             // Cookie attribute-value rules are "any CHAR except CTLs or ';'"
-                            validateCookieAttributeValue(c, i);
+                            validateCookieAttributeValue(name, c, i);
                         }
                     }
                     ++i;
@@ -284,7 +301,7 @@ static HttpSetCookie parseSetCookie(final CharSequence setCookieString, boolean
                     break;
                 default:
                     if (name == null) {
-                        throw new IllegalArgumentException("cookie value not found at index " + i);
+                        throw new IllegalArgumentException("set-cookie value not found at index " + i);
                     }
                     final CharSequence avName = setCookieString.subSequence(begin, i);
                     if (contentEqualsIgnoreCase(avName, "secure")) {
@@ -552,13 +569,13 @@ private static SameSite fromSequence(CharSequence cs, int begin, int end) {
      * @param hexValue The decimal representation of the hexadecimal value.
      * @param index The index of the character in the inputs, for error reporting.
      */
-    private static void validateCookieOctetHexValue(final int hexValue, int index) {
+    private static void validateCookieOctetHexValue(final CharSequence name, final int hexValue, int index) {
         if (hexValue != 33 &&
                 (hexValue < 35 || hexValue > 43) &&
                 (hexValue < 45 || hexValue > 58) &&
                 (hexValue < 60 || hexValue > 91) &&
                 (hexValue < 93 || hexValue > 126)) {
-            throw unexpectedHexValue(hexValue, index);
+            throw unexpectedHexValue(name, hexValue, index);
         }
     }
 
@@ -570,14 +587,14 @@ private static void validateCookieOctetHexValue(final int hexValue, int index) {
      * @param hexValue The decimal representation of the hexadecimal value.
      * @param index The index of the character in the inputs, for error reporting.
      */
-    private static void validateCookieAttributeValue(final int hexValue, int index) {
+    private static void validateCookieAttributeValue(final CharSequence name, final int hexValue, int index) {
         if (hexValue == ';' || hexValue == 0x7F || hexValue <= 0x1F) {
-            throw unexpectedHexValue(hexValue, index);
+            throw unexpectedHexValue(name, hexValue, index);
         }
     }
 
-    private static IllegalArgumentException unexpectedHexValue(int hexValue, int index) {
+    private static IllegalArgumentException unexpectedHexValue(CharSequence name, int hexValue, int index) {
         return new IllegalArgumentException(
-                "Unexpected hex value at index " + index + ": 0x" + Integer.toHexString(hexValue));
+                "set-cookie '" + name + "': unexpected hex value at index " + index + ": 0x" + toHexString(hexValue));
     }
 }

servicetalk-http-api/src/main/java/io/servicetalk/http/api/HeaderUtils.java

@@ -52,7 +52,9 @@
 import static io.servicetalk.utils.internal.CharsetUtils.standardCharsets;
 import static io.servicetalk.utils.internal.NetworkUtils.isValidIpV4Address;
 import static io.servicetalk.utils.internal.NetworkUtils.isValidIpV6Address;
+import static java.lang.Boolean.parseBoolean;
 import static java.lang.Math.min;
+import static java.lang.System.getProperty;
 import static java.lang.System.lineSeparator;
 import static java.nio.charset.StandardCharsets.UTF_8;
 import static java.util.Arrays.asList;
@@ -83,6 +85,16 @@ public final class HeaderUtils {
                 }
                 return "<filtered>";
             };
+    /**
+     * Whether cookie parsing should be strictly spec compliant with
+     * <a href="https://www.rfc-editor.org/rfc/rfc6265">RFC6265</a> ({@code true}), or allow some deviations that are
+     * commonly observed in practice and allowed by the obsolete
+     * <a href="https://www.rfc-editor.org/rfc/rfc2965">RFC2965</a>/
+     * <a href="https://www.rfc-editor.org/rfc/rfc2109">RFC2109</a> ({@code false}, the default).
+     */
+    // not final for testing
+    private static boolean cookieParsingStrictRfc6265 = parseBoolean(getProperty(
+            "io.servicetalk.http.api.headers.cookieParsingStrictRfc6265", "false"));
     // ASCII symbols:
     private static final byte HT = 9;
     private static final byte DEL = 127;
@@ -100,6 +112,15 @@ private HeaderUtils() {
         // no instances
     }
 
+    static boolean cookieParsingStrictRfc6265() {
+        return cookieParsingStrictRfc6265;
+    }
+
+    // pkg-private for testing
+    static void cookieParsingStrictRfc6265(boolean value) {
+        cookieParsingStrictRfc6265 = value;
+    }
+
     static String toString(final HttpHeaders headers,
                            final BiFunction<? super CharSequence, ? super CharSequence, CharSequence> filter) {
         final String simpleName = headers.getClass().getSimpleName();

servicetalk-http-api/src/test/java/io/servicetalk/http/api/DefaultHttpSetCookiesRfc6265Test.java

@@ -0,0 +1,77 @@
+/*
+ * Copyright © 2022 Apple Inc. and the ServiceTalk project authors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package io.servicetalk.http.api;
+
+import org.hamcrest.MatcherAssert;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.parallel.Execution;
+import org.junit.jupiter.api.parallel.ExecutionMode;
+import org.junit.jupiter.api.parallel.Isolated;
+
+import static io.servicetalk.http.api.DefaultHttpSetCookiesTest.quotesInValuePreserved;
+import static org.hamcrest.Matchers.allOf;
+import static org.hamcrest.Matchers.containsString;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+@Isolated
+@Execution(ExecutionMode.SAME_THREAD)
+class DefaultHttpSetCookiesRfc6265Test {
+
+    @BeforeAll
+    static void enablePedantic() {
+        HeaderUtils.cookieParsingStrictRfc6265(true);
+    }
+
+    @AfterAll
+    static void disablePedantic() {
+        HeaderUtils.cookieParsingStrictRfc6265(false);
+    }
+
+    @Test
+    void throwIfNoSpaceBeforeCookieAttributeValue() {
+        final HttpHeaders headers = DefaultHttpHeadersFactory.INSTANCE.newHeaders();
+        headers.add("set-cookie", "first=12345;Extension");
+        headers.add("set-cookie", "second=12345;Expires=Mon, 22 Aug 2022 20:12:35 GMT");
+        headers.add("set-cookie", "third=\"12345\";Expires=Mon, 22 Aug 2022 20:12:35 GMT");
+        throwIfNoSpaceBeforeCookieAttributeValue(headers);
+    }
+
+    private static void throwIfNoSpaceBeforeCookieAttributeValue(HttpHeaders headers) {
+        Exception exception;
+
+        exception = assertThrows(IllegalArgumentException.class, () -> headers.getSetCookie("first"));
+        MatcherAssert.assertThat(exception.getMessage(),
+                allOf(containsString("first"), containsString("space is required after ;")));
+
+        exception = assertThrows(IllegalArgumentException.class, () -> headers.getSetCookie("second"));
+        MatcherAssert.assertThat(exception.getMessage(),
+                allOf(containsString("second"), containsString("space is required after ;")));
+
+        exception = assertThrows(IllegalArgumentException.class, () -> headers.getSetCookie("third"));
+        MatcherAssert.assertThat(exception.getMessage(),
+                allOf(containsString("third"), containsString("space is required after ;")));
+    }
+
+    @Test
+    void spaceAfterQuotedValue() {
+        final HttpHeaders headers = DefaultHttpHeadersFactory.INSTANCE.newHeaders();
+        headers.add("set-cookie",
+                "qwerty=\"12345\"; Domain=somecompany.co.uk; Path=/; Expires=Wed, 30 Aug 2019 00:00:00 GMT");
+        quotesInValuePreserved(headers);
+    }
+}