From ae4d6d14c6d104a19dd23d1815fd6d9e22b2f7bf Mon Sep 17 00:00:00 2001 From: Idel Pivnitskiy Date: Fri, 23 Sep 2022 15:14:09 -0500 Subject: [PATCH] Improvements for `Set-Cookie` parsing, allow lax parsing of older specs (#2368) Motivation: - #2329 made parsing of `set-cookie` header strict according to RFC6265. In practice, there are still many implementations that encode cookies according to the obsolete RFC2965 and/or RFC2109. - Semicolon and space are not validated after a wrapped value. - Without a cookie name in the exception message it's harder to find a problematic cookie. Modifications: - Allow no space after semicolon by default; - Add a system property `io.servicetalk.http.api.headers.cookieParsingStrictRfc6265` to enforce strict parsing; - Instead of blindly skipping `SEMI` and `SP` after `DQUOTE`, validate skipped characters; - Include the cookie name (if already parsed) in all exception messages; - Enhance test coverage for `DefaultHttpSetCookie#parseSetCookie`; Result: 1. No space is required after semicolon by default. 2. Characters after a wrapped value are validated. 3. Exception messages include a cookie name when possible. 4. More test coverage. --- servicetalk-http-api/build.gradle | 1 + .../http/api/DefaultHttpSetCookie.java | 59 +++++---- .../io/servicetalk/http/api/HeaderUtils.java | 21 ++++ .../api/DefaultHttpSetCookiesRfc6265Test.java | 77 ++++++++++++ .../http/api/DefaultHttpSetCookiesTest.java | 117 ++++++++++++++++-- 5 files changed, 243 insertions(+), 32 deletions(-) create mode 100644 servicetalk-http-api/src/test/java/io/servicetalk/http/api/DefaultHttpSetCookiesRfc6265Test.java diff --git a/servicetalk-http-api/build.gradle b/servicetalk-http-api/build.gradle index ecab4900db..c6941dda42 100644 --- a/servicetalk-http-api/build.gradle +++ b/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")) diff --git a/servicetalk-http-api/src/main/java/io/servicetalk/http/api/DefaultHttpSetCookie.java b/servicetalk-http-api/src/main/java/io/servicetalk/http/api/DefaultHttpSetCookie.java index 8e49ee336b..fb56f12017 100644 --- a/servicetalk-http-api/src/main/java/io/servicetalk/http/api/DefaultHttpSetCookie.java +++ b/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)); } } diff --git a/servicetalk-http-api/src/main/java/io/servicetalk/http/api/HeaderUtils.java b/servicetalk-http-api/src/main/java/io/servicetalk/http/api/HeaderUtils.java index 8c3c204c52..80e99b4349 100644 --- a/servicetalk-http-api/src/main/java/io/servicetalk/http/api/HeaderUtils.java +++ b/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 ""; }; + /** + * Whether cookie parsing should be strictly spec compliant with + * RFC6265 ({@code true}), or allow some deviations that are + * commonly observed in practice and allowed by the obsolete + * RFC2965/ + * RFC2109 ({@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 filter) { final String simpleName = headers.getClass().getSimpleName(); diff --git a/servicetalk-http-api/src/test/java/io/servicetalk/http/api/DefaultHttpSetCookiesRfc6265Test.java b/servicetalk-http-api/src/test/java/io/servicetalk/http/api/DefaultHttpSetCookiesRfc6265Test.java new file mode 100644 index 0000000000..96672f5c9e --- /dev/null +++ b/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); + } +} diff --git a/servicetalk-http-api/src/test/java/io/servicetalk/http/api/DefaultHttpSetCookiesTest.java b/servicetalk-http-api/src/test/java/io/servicetalk/http/api/DefaultHttpSetCookiesTest.java index 0ea93c5ba7..efe7b1faa1 100644 --- a/servicetalk-http-api/src/test/java/io/servicetalk/http/api/DefaultHttpSetCookiesTest.java +++ b/servicetalk-http-api/src/test/java/io/servicetalk/http/api/DefaultHttpSetCookiesTest.java @@ -25,11 +25,16 @@ import javax.annotation.Nullable; import static io.servicetalk.buffer.api.CharSequences.contentEqualsIgnoreCase; +import static io.servicetalk.buffer.api.Matchers.contentEqualTo; +import static io.servicetalk.http.api.DefaultHttpSetCookie.parseSetCookie; 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 org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.allOf; import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.is; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; @@ -398,6 +403,22 @@ private static void percentEncodedNameCanBeDecoded(HttpHeaders headers) { assertFalse(cookieItr.hasNext()); } + @Test + void quotedValueWithSpace() { + final HttpHeaders headers = DefaultHttpHeadersFactory.INSTANCE.newHeaders(); + headers.add("set-cookie", "qwerty=\"12 345\""); + Exception e = assertThrows(IllegalArgumentException.class, () -> headers.getSetCookie("qwerty")); + assertThat(e.getMessage(), allOf(containsString("qwerty"), containsString("unexpected hex value"))); + } + + @Test + void noSemicolonAfterQuotedValue() { + final HttpHeaders headers = DefaultHttpHeadersFactory.INSTANCE.newHeaders(); + headers.add("set-cookie", "qwerty=\"12345\" max-age=12"); + Exception e = assertThrows(IllegalArgumentException.class, () -> headers.getSetCookie("qwerty")); + assertThat(e.getMessage(), allOf(containsString("qwerty"), containsString("expected semicolon"))); + } + @Test void quotesInValuePreserved() { final HttpHeaders headers = DefaultHttpHeadersFactory.INSTANCE.newHeaders(); @@ -406,9 +427,10 @@ void quotesInValuePreserved() { quotesInValuePreserved(headers); } - private static void quotesInValuePreserved(HttpHeaders headers) { + static void quotesInValuePreserved(HttpHeaders headers) { final HttpSetCookie cookie = headers.getSetCookie("qwerty"); assertNotNull(cookie); + assertThat(cookie.isWrapped(), is(true)); assertTrue(areSetCookiesEqual(new TestSetCookie("qwerty", "12345", "/", "somecompany.co.uk", "Wed, 30 Aug 2019 00:00:00 GMT", null, null, true, false, false), cookie)); @@ -606,11 +628,83 @@ void noEqualsValueReturnsNull() { assertNull(headers.getSetCookie("qwerty12345")); } + @Test + void expiresAfterSemicolon() { + final HttpHeaders headers = DefaultHttpHeadersFactory.INSTANCE.newHeaders(); + headers.add("set-cookie", + "qwerty=12345; Domain=somecompany.co.uk; Expires=Wed, 30 Aug 2019 00:00:00 GMT; Path=/"); + HttpSetCookie cookie = headers.getSetCookie("qwerty"); + assertNotNull(cookie); + assertThat(cookie.name(), contentEqualTo("qwerty")); + assertThat(cookie.value(), contentEqualTo("12345")); + assertThat(cookie.domain(), contentEqualTo("somecompany.co.uk")); + assertThat(cookie.expires(), contentEqualTo("Wed, 30 Aug 2019 00:00:00 GMT")); + assertThat(cookie.path(), contentEqualTo("/")); + } + + @Test + void emptyValue() { + final HttpHeaders headers = DefaultHttpHeadersFactory.INSTANCE.newHeaders(); + headers.add("set-cookie", "qwerty="); + HttpSetCookie cookie = headers.getSetCookie("qwerty"); + assertNotNull(cookie); + assertThat(cookie.value(), contentEqualTo("")); + } + + @Test + void emptyDomain() { + final HttpHeaders headers = DefaultHttpHeadersFactory.INSTANCE.newHeaders(); + headers.add("set-cookie", "qwerty=12345; Domain="); + HttpSetCookie cookie = headers.getSetCookie("qwerty"); + assertNotNull(cookie); + assertThat(cookie.value(), contentEqualTo("12345")); + assertThat(cookie.domain(), contentEqualTo("")); + } + + @Test + void valueContainsEqualsSign() { + final HttpHeaders headers = DefaultHttpHeadersFactory.INSTANCE.newHeaders(); + headers.add("set-cookie", "qwerty=123=45"); + HttpSetCookie cookie = headers.getSetCookie("qwerty"); + assertNotNull(cookie); + assertThat(cookie.value(), contentEqualTo("123=45")); + } + + @Test + void attributeValueContainsEqualsSign() { + final HttpHeaders headers = DefaultHttpHeadersFactory.INSTANCE.newHeaders(); + headers.add("set-cookie", "qwerty=12345; max-age=12=1"); + Exception e = assertThrows(IllegalArgumentException.class, () -> headers.getSetCookie("qwerty")); + assertThat(e.getMessage(), allOf(containsString("qwerty"), containsString("="))); + } + + @Test + void parseSetCookieWithEmptyName() { + Exception e = assertThrows(IllegalArgumentException.class, () -> parseSetCookie("=123", false)); + assertThat(e.getMessage(), is(equalTo("cookie name cannot be null or empty"))); + + e = assertThrows(IllegalArgumentException.class, () -> parseSetCookie("; max-age=123", false)); + assertThat(e.getMessage(), is(equalTo("cookie name cannot be null or empty"))); + } + + @Test + void parseSetCookieWithEmptyValue() { + Exception e = assertThrows(IllegalArgumentException.class, () -> parseSetCookie("q", false)); + assertThat(e.getMessage(), is(equalTo("set-cookie value not found at index 1"))); + } + + @Test + void parseSetCookieWithUnexpectedQuote() { + Exception e = assertThrows(IllegalArgumentException.class, () -> parseSetCookie("\"123\"", false)); + assertThat(e.getMessage(), is(equalTo("unexpected quote at index: 0"))); + } + @Test void trailingSemiColon() { final HttpHeaders headers = DefaultHttpHeadersFactory.INSTANCE.newHeaders(); headers.add("set-cookie", "qwerty=12345;"); - assertThrows(IllegalArgumentException.class, () -> headers.getSetCookie("qwerty")); + Exception e = assertThrows(IllegalArgumentException.class, () -> headers.getSetCookie("qwerty")); + assertThat(e.getMessage(), allOf(containsString("qwerty"), containsString(";"))); } @Test @@ -641,21 +735,22 @@ private static void valueIteratorThrowsIfNoNextCall(HttpHeaders headers) { } @Test - void throwIfNoSpaceBeforeCookieAttributeValue() { + void mustTolerateNoSpaceBeforeCookieAttributeValue() { 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"); - throwIfNoSpaceBeforeCookieAttributeValue(headers); + tolerateNoSpaceBeforeCookieAttributeValue(headers); } - private static void throwIfNoSpaceBeforeCookieAttributeValue(HttpHeaders headers) { - Exception exception; - - exception = assertThrows(IllegalArgumentException.class, () -> headers.getSetCookie("first")); - assertThat(exception.getMessage(), containsString("space is required after ;")); + private static void tolerateNoSpaceBeforeCookieAttributeValue(HttpHeaders headers) { + HttpSetCookie first = headers.getSetCookie("first"); + assertNotNull(first); + assertThat(first.value(), contentEqualTo("12345")); - exception = assertThrows(IllegalArgumentException.class, () -> headers.getSetCookie("second")); - assertThat(exception.getMessage(), containsString("space is required after ;")); + HttpSetCookie second = headers.getSetCookie("second"); + assertNotNull(second); + assertThat(second.value(), contentEqualTo("12345")); + assertThat(second.expires(), contentEqualTo("Mon, 22 Aug 2022 20:12:35 GMT")); } @Test