Skip to content

Commit

Permalink
Fix #9334 Cookie Compliance
Browse files Browse the repository at this point in the history
Fix incorrect change to RFC6265 to not support dollars in cookie names.

Signed-off-by: gregw <gregw@webtide.com>
  • Loading branch information
gregw committed Feb 21, 2023
1 parent 7a7d69a commit 298a881
Show file tree
Hide file tree
Showing 4 changed files with 202 additions and 45 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -128,13 +128,12 @@ public String getDescription()
* <p>A CookieCompliance mode that enforces <a href="https://tools.ietf.org/html/rfc6265">RFC 6265</a> compliance,
* but allows:</p>
* <ul>
* <li>{@link Violation#ATTRIBUTES}</li>
* <li>{@link Violation#INVALID_COOKIES}</li>
* <li>{@link Violation#OPTIONAL_WHITE_SPACE}</li>
* </ul>
*/
public static final CookieCompliance RFC6265 = new CookieCompliance("RFC6265", of(
Violation.ATTRIBUTES, Violation.INVALID_COOKIES, Violation.OPTIONAL_WHITE_SPACE)
Violation.INVALID_COOKIES, Violation.OPTIONAL_WHITE_SPACE)
);

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ public void parseField(String field)

if (token.isRfc2616Token())
{
if (!StringUtil.isBlank(cookieName) && c != '$')
if (!StringUtil.isBlank(cookieName) && !(c == '$' && (_complianceMode.allows(ATTRIBUTES) || _complianceMode.allows(ATTRIBUTE_VALUES))))
{
_handler.addCookie(cookieName, cookieValue, cookieVersion, cookieDomain, cookiePath, cookieComment);
cookieName = null;
Expand Down Expand Up @@ -332,7 +332,6 @@ else if ((c == ' ' || c == '\t') && _complianceMode.allows(OPTIONAL_WHITE_SPACE)
continue;
}

boolean knownAttribute = true;
if (StringUtil.isBlank(attributeName))
{
cookieValue = value;
Expand All @@ -358,37 +357,26 @@ else if ((c == ' ' || c == '\t') && _complianceMode.allows(OPTIONAL_WHITE_SPACE)
cookieVersion = Integer.parseInt(value);
break;
default:
knownAttribute = false;
if (!_complianceMode.allows(INVALID_COOKIES))
throw new IllegalArgumentException("Invalid Cookie attribute");
reportComplianceViolation(INVALID_COOKIES, field);
state = State.INVALID_COOKIE;
break;
}
}
else if (_complianceMode.allows(ATTRIBUTES))
{
reportComplianceViolation(ATTRIBUTES, field);
}
else if (_complianceMode.allows(INVALID_COOKIES))
{
reportComplianceViolation(INVALID_COOKIES, field);
state = State.INVALID_COOKIE;
continue;
}
else
{
throw new IllegalArgumentException("Invalid Cookie with attributes");
cookieName = attributeName;
cookieValue = value;
}
attributeName = null;
}
value = null;

if (!knownAttribute)
{
if (!_complianceMode.allows(INVALID_COOKIES))
throw new IllegalArgumentException("Invalid Cookie attribute");
reportComplianceViolation(INVALID_COOKIES, field);
state = State.INVALID_COOKIE;
continue;
}

if (state == State.END)
throw new IllegalStateException("Invalid cookie");
break;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
//
// ========================================================================
// Copyright (c) 1995 Mort Bay Consulting Pty Ltd and others.
//
// This program and the accompanying materials are made available under the
// terms of the Eclipse Public License v. 2.0 which is available at
// https://www.eclipse.org/legal/epl-2.0, or the Apache License, Version 2.0
// which is available at https://www.apache.org/licenses/LICENSE-2.0.
//
// SPDX-License-Identifier: EPL-2.0 OR Apache-2.0
// ========================================================================
//

package org.eclipse.jetty.http;

import java.util.ArrayList;
import java.util.List;
import java.util.stream.Stream;

import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.is;

/**
* Tests of poor various name=value scenarios and expectations of results
* due to our efforts at being lenient with what we receive.
*/
public class RFC6265CookieParserLenientTest
{
public static Stream<Arguments> data()
{
return Stream.of(
// Simple test to verify behavior
Arguments.of("x=y", "x", "y"),
Arguments.of("key=value", "key", "value"),

// Tests that conform to RFC2109
// RFC2109 - token values
// token = 1*<any CHAR except CTLs or tspecials>
// CHAR = <any US-ASCII character (octets 0 - 127)>
// CTL = <any US-ASCII control character
// (octets 0 - 31) and DEL (127)>
// SP = <US-ASCII SP, space (32)>
// HT = <US-ASCII HT, horizontal-tab (9)>
// tspecials = "(" | ")" | "<" | ">" | "@"
// | "," | ";" | ":" | "\" | <">
// | "/" | "[" | "]" | "?" | "="
// | "{" | "}" | SP | HT
Arguments.of("abc=xyz", "abc", "xyz"),
Arguments.of("abc=!bar", "abc", "!bar"),
Arguments.of("abc=#hash", "abc", "#hash"),
Arguments.of("abc=#hash", "abc", "#hash"),
// RFC2109 - quoted-string values
// quoted-string = ( <"> *(qdtext) <"> )
// qdtext = <any TEXT except <">>

// rejected, as name cannot be DQUOTED
Arguments.of("\"a\"=bcd", null, null),
Arguments.of("\"a\"=\"b c d\"", null, null),

// lenient with spaces and EOF
Arguments.of("abc=", "abc", ""),
Arguments.of("abc= ", "abc", ""),
Arguments.of("abc= x", "abc", "x"),
Arguments.of("abc = ", "abc", ""),
Arguments.of("abc = ;", "abc", ""),
Arguments.of("abc = ; ", "abc", ""),
Arguments.of("abc = x ", "abc", "x"),
Arguments.of("abc=\"\"", "abc", ""),
Arguments.of("abc= \"\" ", "abc", ""),
Arguments.of("abc= \"x\" ", "abc", "x"),
Arguments.of("abc= \"x\" ;", "abc", "x"),
Arguments.of("abc= \"x\" ; ", "abc", "x"),

// The backslash character ("\") may be used as a single-character quoting
// mechanism only within quoted-string and comment constructs.
// quoted-pair = "\" CHAR
Arguments.of("abc=\"xyz\"", "abc", "xyz"),
Arguments.of("abc=\"!bar\"", "abc", "!bar"),
Arguments.of("abc=\"#hash\"", "abc", "#hash"),
// RFC2109 - other valid name types that conform to 'attr'/'token' syntax
Arguments.of("!f!o!o!=wat", "!f!o!o!", "wat"),
Arguments.of("__MyHost=Foo", "__MyHost", "Foo"),
Arguments.of("some-thing-else=to-parse", "some-thing-else", "to-parse"),
Arguments.of("$foo=bar", "$foo", "bar"),

// Tests that conform to RFC6265
Arguments.of("abc=foobar!", "abc", "foobar!"),
Arguments.of("abc=\"foobar!\"", "abc", "foobar!")

);
}

@ParameterizedTest
@MethodSource("data")
public void testLenientBehavior(String rawHeader, String expectedName, String expectedValue)
{
TestCutter cutter = new TestCutter();
cutter.parseField(rawHeader);

if (expectedName == null)
assertThat("Cookies.length", cutter.names.size(), is(0));
else
{
assertThat("Cookies.length", cutter.names.size(), is(1));
assertThat("Cookie.name", cutter.names.get(0), is(expectedName));
assertThat("Cookie.value", cutter.values.get(0), is(expectedValue));
}
}

static class TestCutter implements CookieParser.Handler
{
CookieParser parser;
List<String> names = new ArrayList<>();
List<String> values = new ArrayList<>();

protected TestCutter()
{
parser = new RFC6265CookieParser(this, CookieCompliance.RFC6265, null);
}

public void parseField(String field)
{
parser.parseField(field);
}

@Override
public void addCookie(String cookieName, String cookieValue, int cookieVersion, String cookieDomain, String cookiePath, String cookieComment)
{
names.add(cookieName);
values.add(cookieValue);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.is;
import static org.junit.jupiter.api.Assertions.assertThrows;

public class RFC6265CookieParserTest
{
Expand All @@ -36,7 +35,7 @@ public void testRFC2965Single()
String rawCookie = "$Version=\"1\"; Customer=\"WILE_E_COYOTE\"; $Path=\"/acme\"";

// Test with RFC 2965.
TestCookieParser parser = new TestCookieParser(CookieCompliance.RFC2965_LEGACY);
TestCookieParser parser = new TestCookieParser(CookieCompliance.RFC2965);
List<Cookie> cookies = parser.parseFields(rawCookie);

assertThat("Cookies.length", cookies.size(), is(1));
Expand All @@ -47,15 +46,24 @@ public void testRFC2965Single()
// Same test with RFC 6265.
parser = new TestCookieParser(CookieCompliance.RFC6265);
cookies = parser.parseFields(rawCookie);
assertThat("Cookies.length", cookies.size(), is(3));
assertCookie("Cookies[0]", cookies.get(0), "$Version", "1", 0, null);
assertCookie("Cookies[1]", cookies.get(1), "Customer", "WILE_E_COYOTE", 0, null);
assertCookie("Cookies[2]", cookies.get(2), "$Path", "/acme", 0, null);

assertThat("Cookies.length", cookies.size(), is(1));
assertCookie("Cookies[0]", cookies.get(0), "Customer", "WILE_E_COYOTE", 0, null);
// There are 2 attributes, so 2 violations.
assertThat(parser.violations.size(), is(2));
// There attributes are seen as just normal cookies, so no violations
assertThat(parser.violations.size(), is(0));

// Same test with RFC 6265 strict should throw.
TestCookieParser strictParser = new TestCookieParser(CookieCompliance.RFC6265_STRICT);
assertThrows(IllegalArgumentException.class, () -> strictParser.parseFields(rawCookie));
parser = new TestCookieParser(CookieCompliance.RFC6265_STRICT);
cookies = parser.parseFields(rawCookie);
assertThat("Cookies.length", cookies.size(), is(3));
assertCookie("Cookies[0]", cookies.get(0), "$Version", "1", 0, null);
assertCookie("Cookies[1]", cookies.get(1), "Customer", "WILE_E_COYOTE", 0, null);
assertCookie("Cookies[2]", cookies.get(2), "$Path", "/acme", 0, null);

// There attributes are seen as just normal cookies, so no violations
assertThat(parser.violations.size(), is(0));
}

/**
Expand All @@ -68,8 +76,26 @@ public void testRFC2965Double()
"Customer=\"WILE_E_COYOTE\"; $Path=\"/acme\"; " +
"Part_Number=\"Rocket_Launcher_0001\"; $Path=\"/acme\"";

Cookie[] cookies = parseCookieHeaders(CookieCompliance.RFC2965_LEGACY, rawCookie);
Cookie[] cookies = parseCookieHeaders(CookieCompliance.RFC2965, rawCookie);

assertThat("Cookies.length", cookies.length, is(2));
assertCookie("Cookies[0]", cookies[0], "Customer", "WILE_E_COYOTE", 1, "/acme");
assertCookie("Cookies[1]", cookies[1], "Part_Number", "Rocket_Launcher_0001", 1, "/acme");

cookies = parseCookieHeaders(CookieCompliance.RFC6265, rawCookie);
assertThat("Cookies.length", cookies.length, is(5));
assertCookie("Cookies[0]", cookies[0], "$Version", "1", 0, null);
assertCookie("Cookies[1]", cookies[1], "Customer", "WILE_E_COYOTE", 0, null);
assertCookie("Cookies[2]", cookies[2], "$Path", "/acme", 0, null);
assertCookie("Cookies[3]", cookies[3], "Part_Number", "Rocket_Launcher_0001", 0, null);
assertCookie("Cookies[4]", cookies[4], "$Path", "/acme", 0, null);

cookies = parseCookieHeaders(CookieCompliance.from("RFC6265,ATTRIBUTES"), rawCookie);
assertThat("Cookies.length", cookies.length, is(2));
assertCookie("Cookies[0]", cookies[0], "Customer", "WILE_E_COYOTE", 0, null);
assertCookie("Cookies[1]", cookies[1], "Part_Number", "Rocket_Launcher_0001", 0, null);

cookies = parseCookieHeaders(CookieCompliance.from("RFC6265,ATTRIBUTE_VALUES"), rawCookie);
assertThat("Cookies.length", cookies.length, is(2));
assertCookie("Cookies[0]", cookies[0], "Customer", "WILE_E_COYOTE", 1, "/acme");
assertCookie("Cookies[1]", cookies[1], "Part_Number", "Rocket_Launcher_0001", 1, "/acme");
Expand All @@ -86,7 +112,7 @@ public void testRFCTriple()
"Part_Number=\"Rocket_Launcher_0001\"; $Path=\"/acme\"; " +
"Shipping=\"FedEx\"; $Path=\"/acme\"";

Cookie[] cookies = parseCookieHeaders(CookieCompliance.RFC2965_LEGACY, rawCookie);
Cookie[] cookies = parseCookieHeaders(CookieCompliance.RFC2965, rawCookie);

assertThat("Cookies.length", cookies.length, is(3));
assertCookie("Cookies[0]", cookies[0], "Customer", "WILE_E_COYOTE", 1, "/acme");
Expand All @@ -104,7 +130,7 @@ public void testRFCPathExample()
"Part_Number=\"Riding_Rocket_0023\"; $Path=\"/acme/ammo\"; " +
"Part_Number=\"Rocket_Launcher_0001\"; $Path=\"/acme\"";

Cookie[] cookies = parseCookieHeaders(CookieCompliance.RFC2965_LEGACY, rawCookie);
Cookie[] cookies = parseCookieHeaders(CookieCompliance.RFC2965, rawCookie);

assertThat("Cookies.length", cookies.length, is(2));
assertCookie("Cookies[0]", cookies[0], "Part_Number", "Riding_Rocket_0023", 1, "/acme/ammo");
Expand All @@ -121,7 +147,7 @@ public void testRFC2109CookieSpoofingExample()
"session_id=\"1234\"; " +
"session_id=\"1111\"; $Domain=\".cracker.edu\"";

Cookie[] cookies = parseCookieHeaders(CookieCompliance.RFC2965_LEGACY, rawCookie);
Cookie[] cookies = parseCookieHeaders(CookieCompliance.RFC2965, rawCookie);

assertThat("Cookies.length", cookies.length, is(2));
assertCookie("Cookies[0]", cookies[0], "session_id", "1234", 1, null);
Expand All @@ -137,15 +163,25 @@ public void testRFC2965CookieSpoofingExample()
String rawCookie = "$Version=\"1\"; session_id=\"1234\", " +
"$Version=\"1\"; session_id=\"1111\"; $Domain=\".cracker.edu\"";

Cookie[] cookies = parseCookieHeaders(CookieCompliance.RFC2965_LEGACY, rawCookie);
Cookie[] cookies = parseCookieHeaders(CookieCompliance.RFC2965, rawCookie);

assertThat("Cookies.length", cookies.length, is(2));
assertCookie("Cookies[0]", cookies[0], "session_id", "1234", 1, null);
assertCookie("Cookies[1]", cookies[1], "session_id", "1111", 1, null);

cookies = parseCookieHeaders(CookieCompliance.RFC6265, rawCookie);
assertThat("Cookies.length", cookies.length, is(1));
assertCookie("Cookies[1]", cookies[0], "session_id", "1111", 0, null);
assertThat("Cookies.length", cookies.length, is(3));
assertCookie("Cookies[0]", cookies[0], "$Version", "1", 0, null);
assertCookie("Cookies[1]", cookies[1], "session_id", "1111", 0, null);
assertCookie("Cookies[2]", cookies[2], "$Domain", ".cracker.edu", 0, null);

cookies = parseCookieHeaders(CookieCompliance.from("RFC6265,COMMA_SEPARATOR"), rawCookie);
assertThat("Cookies.length", cookies.length, is(5));
assertCookie("Cookies[0]", cookies[0], "$Version", "1", 0, null);
assertCookie("Cookies[1]", cookies[1], "session_id", "1234", 0, null);
assertCookie("Cookies[3]", cookies[2], "$Version", "1", 0, null);
assertCookie("Cookies[3]", cookies[3], "session_id", "1111", 0, null);
assertCookie("Cookies[4]", cookies[4], "$Domain", ".cracker.edu", 0, null);
}

/**
Expand Down Expand Up @@ -201,7 +237,8 @@ public void testDollarName()

Cookie[] cookies = parseCookieHeaders(CookieCompliance.RFC6265, rawCookie);

assertThat("Cookies.length", cookies.length, is(0));
assertThat("Cookies.length", cookies.length, is(1));
assertCookie("Cookies[0]", cookies[0], "$key", "value", 0, null);
}

@Test
Expand Down Expand Up @@ -236,7 +273,7 @@ public void testExcessiveSemicolons()
public void testRFC2965QuotedEscape()
{
String rawCookie = "A=\"double\\\"quote\"";
Cookie[] cookies = parseCookieHeaders(CookieCompliance.RFC2965_LEGACY, rawCookie);
Cookie[] cookies = parseCookieHeaders(CookieCompliance.RFC2965, rawCookie);

assertThat("Cookies.length", cookies.length, is(1));
assertCookie("Cookies[0]", cookies[0], "A", "double\"quote", 0, null);
Expand All @@ -246,16 +283,12 @@ public void testRFC2965QuotedEscape()
public void testRFC2965QuotedSpecial()
{
String rawCookie = "A=\", ;\"";
Cookie[] cookies = parseCookieHeaders(CookieCompliance.RFC2965_LEGACY, rawCookie);
Cookie[] cookies = parseCookieHeaders(CookieCompliance.RFC2965, rawCookie);

assertThat("Cookies.length", cookies.length, is(1));
assertCookie("Cookies[0]", cookies[0], "A", ", ;", 0, null);
}

// TODO:
// $X; N=V
// $X=Y; N=V

public static List<Param> parameters()
{
return Arrays.asList(
Expand All @@ -281,8 +314,8 @@ public static List<Param> parameters()
new Param("A=\"1\u0007\"; B=2; C=3", "B=2", "C=3"),
new Param("€"),
new Param("@={}"),
new Param("$X=Y; N=V", "N=V"),
new Param("N=V; $X=Y", "N=V")
new Param("$X=Y; N=V", "$X=Y", "N=V"),
new Param("N=V; $X=Y", "N=V", "$X=Y")
);
}

Expand Down

0 comments on commit 298a881

Please sign in to comment.