Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jetty 12.0.x ee10 convert cookie #9057

Merged
merged 9 commits into from
Dec 19, 2022
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ public CookieCache(CookieCompliance compliance, ComplianceViolation.Listener com
@Override
protected void addCookie(String cookieName, String cookieValue, String cookieDomain, String cookiePath, int cookieVersion, String cookieComment)
{
_cookieList.add(new HttpCookie(cookieName, cookieValue));
_cookieList.add(new HttpCookie(cookieName, cookieValue, cookieDomain, cookiePath, -1, false, false, cookieComment, cookieVersion));
}
};
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,21 @@ else if (tokenstart >= 0)
case "$version" -> cookieVersion = Integer.parseInt(value);
}
}
else if (CookieCompliance.RFC6265.equals(_complianceMode))
{
//$ prefixed names are treated as separate cookies, so add the completed last cookie if we have one
//and start a new cookie
if (cookieName != null)
{
if (!reject)
addCookie(cookieName, cookieValue, cookieDomain, cookiePath, cookieVersion, cookieComment);
cookieDomain = null;
cookiePath = null;
cookieComment = null;
}
cookieName = name;
cookieValue = value;
}
}
else
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -367,12 +367,8 @@ else if (version > 1)
else
DateGenerator.formatCookieDate(buf, System.currentTimeMillis() + 1000L * _maxAge);

// for v1 cookies, also send max-age
if (version >= 1)
{
buf.append(";Max-Age=");
buf.append(_maxAge);
}
buf.append(";Max-Age=");
joakime marked this conversation as resolved.
Show resolved Hide resolved
buf.append(_maxAge);
}

// add the other fields
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,17 @@

import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
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.hasEntry;
import static org.hamcrest.Matchers.is;

/**
Expand All @@ -35,8 +38,8 @@ 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"),
Arguments.of("x=y", Collections.singletonMap("x", "y")),
Arguments.of("key=value", Collections.singletonMap("key", "value")),

// Tests that conform to RFC2109
// RFC2109 - token values
Expand All @@ -50,133 +53,133 @@ public static Stream<Arguments> data()
// | "," | ";" | ":" | "\" | <">
// | "/" | "[" | "]" | "?" | "="
// | "{" | "}" | 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"),
Arguments.of("abc=xyz", Collections.singletonMap("abc", "xyz")),
Arguments.of("abc=!bar", Collections.singletonMap("abc", "!bar")),
Arguments.of("abc=#hash", Collections.singletonMap("abc", "#hash")),
Arguments.of("abc=#hash", Collections.singletonMap("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),
Arguments.of("\"a\"=bcd", null),
Arguments.of("\"a\"=\"b c d\"", null),

// lenient with spaces and EOF
Arguments.of("abc=", "abc", ""),
Arguments.of("abc = ", "abc", ""),
Arguments.of("abc = ;", "abc", ""),
Arguments.of("abc = ; ", "abc", ""),
Arguments.of("abc = x ", "abc", "x"),
Arguments.of("abc = e f g ", "abc", "e f g"),
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"),
Arguments.of("abc=", Collections.singletonMap("abc", "")),
Arguments.of("abc = ", Collections.singletonMap("abc", "")),
Arguments.of("abc = ;", Collections.singletonMap("abc", "")),
Arguments.of("abc = ; ", Collections.singletonMap("abc", "")),
Arguments.of("abc = x ", Collections.singletonMap("abc", "x")),
Arguments.of("abc = e f g ", Collections.singletonMap("abc", "e f g")),
Arguments.of("abc=\"\"", Collections.singletonMap("abc", "")),
Arguments.of("abc= \"\" ", Collections.singletonMap("abc", "")),
Arguments.of("abc= \"x\" ", Collections.singletonMap("abc", "x")),
Arguments.of("abc= \"x\" ;", Collections.singletonMap("abc", "x")),
Arguments.of("abc= \"x\" ; ", Collections.singletonMap("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"),
Arguments.of("abc=\"xyz\"", Collections.singletonMap("abc", "xyz")),
Arguments.of("abc=\"!bar\"", Collections.singletonMap("abc", "!bar")),
Arguments.of("abc=\"#hash\"", Collections.singletonMap("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("!f!o!o!=wat", Collections.singletonMap("!f!o!o!", "wat")),
Arguments.of("__MyHost=Foo", Collections.singletonMap("__MyHost", "Foo")),
Arguments.of("some-thing-else=to-parse", Collections.singletonMap("some-thing-else", "to-parse")),
// RFC2109 - names with attr/token syntax starting with '$' (and not a cookie reserved word)
// See https://tools.ietf.org/html/draft-ietf-httpbis-cookie-prefixes-00#section-5.2
// Cannot pass names through as jakarta.servlet.http.Cookie class does not allow them
Arguments.of("$foo=bar", null, null),
Arguments.of("$foo=bar", Collections.singletonMap("$foo", "bar")),

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

// Internal quotes
Arguments.of("foo=bar\"baz", "foo", "bar\"baz"),
Arguments.of("foo=\"bar\"baz\"", "foo", "bar\"baz"),
Arguments.of("foo=\"bar\"-\"baz\"", "foo", "bar\"-\"baz"),
Arguments.of("foo=\"bar'-\"baz\"", "foo", "bar'-\"baz"),
Arguments.of("foo=\"bar''-\"baz\"", "foo", "bar''-\"baz"),
Arguments.of("foo=bar\"baz", Collections.singletonMap("foo", "bar\"baz")),
Arguments.of("foo=\"bar\"baz\"", Collections.singletonMap("foo", "bar\"baz")),
Arguments.of("foo=\"bar\"-\"baz\"", Collections.singletonMap("foo", "bar\"-\"baz")),
Arguments.of("foo=\"bar'-\"baz\"", Collections.singletonMap("foo", "bar'-\"baz")),
Arguments.of("foo=\"bar''-\"baz\"", Collections.singletonMap("foo", "bar''-\"baz")),
// These seem dubious until you realize the "lots of equals signs" below works
Arguments.of("foo=\"bar\"=\"baz\"", "foo", "bar\"=\"baz"),
Arguments.of("query=\"?b=c\"&\"d=e\"", "query", "?b=c\"&\"d=e"),
Arguments.of("foo=\"bar\"=\"baz\"", Collections.singletonMap("foo", "bar\"=\"baz")),
Arguments.of("query=\"?b=c\"&\"d=e\"", Collections.singletonMap("query", "?b=c\"&\"d=e")),
// Escaped quotes
Arguments.of("foo=\"bar\\\"=\\\"baz\"", "foo", "bar\"=\"baz"),
Arguments.of("foo=\"bar\\\"=\\\"baz\"", Collections.singletonMap("foo", "bar\"=\"baz")),

// Unterminated Quotes
Arguments.of("x=\"abc", "x", "\"abc"),
Arguments.of("x=\"abc", Collections.singletonMap("x", "\"abc")),
// Unterminated Quotes with valid cookie params after it
Arguments.of("x=\"abc $Path=/", "x", "\"abc $Path=/"),
Arguments.of("x=\"abc $Path=/", Collections.singletonMap("x", "\"abc $Path=/")),
// Unterminated Quotes with trailing escape
Arguments.of("x=\"abc\\", "x", "\"abc\\"),
Arguments.of("x=\"abc\\", Collections.singletonMap("x", "\"abc\\")),

// UTF-8 raw values (not encoded) - VIOLATION of RFC6265
Arguments.of("2sides=\u262F", null, null), // 2 byte (YIN YANG) - rejected due to not being DQUOTED
Arguments.of("currency=\"\u20AC\"", "currency", "\u20AC"), // 3 byte (EURO SIGN)
Arguments.of("gothic=\"\uD800\uDF48\"", "gothic", "\uD800\uDF48"), // 4 byte (GOTHIC LETTER HWAIR)
Arguments.of("2sides=\u262F", null), // 2 byte (YIN YANG) - rejected due to not being DQUOTED
Arguments.of("currency=\"\u20AC\"", Collections.singletonMap("currency", "\u20AC")), // 3 byte (EURO SIGN)
Arguments.of("gothic=\"\uD800\uDF48\"", Collections.singletonMap("gothic", "\uD800\uDF48")), // 4 byte (GOTHIC LETTER HWAIR)

// Spaces
Arguments.of("foo=bar baz", "foo", "bar baz"),
Arguments.of("foo=\"bar baz\"", "foo", "bar baz"),
Arguments.of("z=a b c d e f g", "z", "a b c d e f g"),
Arguments.of("foo=bar baz", Collections.singletonMap("foo", "bar baz")),
Arguments.of("foo=\"bar baz\"", Collections.singletonMap("foo", "bar baz")),
Arguments.of("z=a b c d e f g", Collections.singletonMap("z", "a b c d e f g")),

// Bad tspecials usage - VIOLATION of RFC6265
Arguments.of("foo=bar;baz", "foo", "bar"),
Arguments.of("foo=\"bar;baz\"", "foo", "bar;baz"),
Arguments.of("z=a;b,c:d;e/f[g]", "z", "a"),
Arguments.of("z=\"a;b,c:d;e/f[g]\"", "z", "a;b,c:d;e/f[g]"),
Arguments.of("name=quoted=\"\\\"badly\\\"\"", "name", "quoted=\"\\\"badly\\\"\""), // someone attempting to escape a DQUOTE from within a DQUOTED pair)
Arguments.of("foo=bar;baz", Collections.singletonMap("foo", "bar")),
Arguments.of("foo=\"bar;baz\"", Collections.singletonMap("foo", "bar;baz")),
Arguments.of("z=a;b,c:d;e/f[g]", Collections.singletonMap("z", "a")),
Arguments.of("z=\"a;b,c:d;e/f[g]\"", Collections.singletonMap("z", "a;b,c:d;e/f[g]")),
Arguments.of("name=quoted=\"\\\"badly\\\"\"", Collections.singletonMap("name", "quoted=\"\\\"badly\\\"\"")), // someone attempting to escape a DQUOTE from within a DQUOTED pair)

// Quoted with other Cookie keywords
Arguments.of("x=\"$Version=0\"", "x", "$Version=0"),
Arguments.of("x=\"$Path=/\"", "x", "$Path=/"),
Arguments.of("x=\"$Path=/ $Domain=.foo.com\"", "x", "$Path=/ $Domain=.foo.com"),
Arguments.of("x=\" $Path=/ $Domain=.foo.com \"", "x", " $Path=/ $Domain=.foo.com "),
Arguments.of("a=\"b; $Path=/a; c=d; $PATH=/c; e=f\"; $Path=/e/", "a", "b; $Path=/a; c=d; $PATH=/c; e=f"), // VIOLATES RFC6265
Arguments.of("x=\"$Version=0\"", Collections.singletonMap("x", "$Version=0")),
Arguments.of("x=\"$Path=/\"", Collections.singletonMap("x", "$Path=/")),
Arguments.of("x=\"$Path=/ $Domain=.foo.com\"", Collections.singletonMap("x", "$Path=/ $Domain=.foo.com")),
Arguments.of("x=\" $Path=/ $Domain=.foo.com \"", Collections.singletonMap("x", " $Path=/ $Domain=.foo.com ")),
Arguments.of("a=\"b; $Path=/a; c=d; $PATH=/c; e=f\"; $Path=/e/", Map.of("a", "b; $Path=/a; c=d; $PATH=/c; e=f", "$Path", "/e/")), // VIOLATES RFC6265

// Lots of equals signs
Arguments.of("query=b=c&d=e", "query", "b=c&d=e"),
Arguments.of("query=b=c&d=e", Collections.singletonMap("query", "b=c&d=e")),

// Escaping
Arguments.of("query=%7B%22sessionCount%22%3A5%2C%22sessionTime%22%3A14151%7D", "query", "%7B%22sessionCount%22%3A5%2C%22sessionTime%22%3A14151%7D"),
Arguments.of("query=%7B%22sessionCount%22%3A5%2C%22sessionTime%22%3A14151%7D", Collections.singletonMap("query", "%7B%22sessionCount%22%3A5%2C%22sessionTime%22%3A14151%7D")),

// Google cookies (seen in wild, has `tspecials` of ':' in value)
Arguments.of("GAPS=1:A1aaaAaAA1aaAAAaa1a11a:aAaaAa-aaA1-", "GAPS", "1:A1aaaAaAA1aaAAAaa1a11a:aAaaAa-aaA1-"),
Arguments.of("GAPS=1:A1aaaAaAA1aaAAAaa1a11a:aAaaAa-aaA1-", Collections.singletonMap("GAPS", "1:A1aaaAaAA1aaAAAaa1a11a:aAaaAa-aaA1-")),

// Strong abuse of cookie spec (lots of tspecials) - VIOLATION of RFC6265
Arguments.of("$Version=0; rToken=F_TOKEN''!--\"</a>=&{()}", "rToken", "F_TOKEN''!--\"</a>=&{()}"),
Arguments.of("$Version=0; rToken=F_TOKEN''!--\"</a>=&{()}", Map.of("rToken", "F_TOKEN''!--\"</a>=&{()}", "$Version", "0")),

// Commas that were not commas
Arguments.of("name=foo,bar", "name", "foo,bar"),
Arguments.of("name=foo , bar", "name", "foo , bar"),
Arguments.of("name=foo , bar, bob", "name", "foo , bar, bob")
Arguments.of("name=foo,bar", Collections.singletonMap("name", "foo,bar")),
Arguments.of("name=foo , bar", Collections.singletonMap("name", "foo , bar")),
Arguments.of("name=foo , bar, bob", Collections.singletonMap("name", "foo , bar, bob"))
);
}

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

if (expectedName == null)
assertThat("Cookies.length", cutter.names.size(), is(0));
if (expected == null)
assertThat("Cookies.length", cutter.pairs.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));
assertThat("Cookies.length", cutter.pairs.size(), is(expected.size()));

for (Map.Entry<String, String> entry : expected.entrySet())
assertThat("Cookie pairs", cutter.pairs, hasEntry(entry.getKey(), entry.getValue()));
}
}

class TestCutter extends CookieCutter
{
List<String> names = new ArrayList<>();
List<String> values = new ArrayList<>();
Map<String, String> pairs = new HashMap<>();

protected TestCutter()
{
Expand All @@ -186,8 +189,7 @@ protected TestCutter()
@Override
protected void addCookie(String cookieName, String cookieValue, String cookieDomain, String cookiePath, int cookieVersion, String cookieComment)
{
names.add(cookieName);
values.add(cookieValue);
pairs.put(cookieName, cookieValue);
}

public void parseField(String field)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -146,9 +146,11 @@ public void testRFC2965CookieSpoofingExample()
assertCookie("Cookies[1]", cookies[1], "session_id", "1111", 1, null);

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

/**
Expand Down Expand Up @@ -204,7 +206,7 @@ public void testDollarName()

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

assertThat("Cookies.length", cookies.length, is(0));
assertThat("Cookies.length", cookies.length, is(1));
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@
import org.eclipse.jetty.ee10.servlet.security.Authentication;
import org.eclipse.jetty.ee10.servlet.security.UserIdentity;
import org.eclipse.jetty.http.BadMessageException;
import org.eclipse.jetty.http.CookieCompliance;
import org.eclipse.jetty.http.HttpCookie;
import org.eclipse.jetty.http.HttpField;
import org.eclipse.jetty.http.HttpFields;
Expand Down Expand Up @@ -477,22 +478,23 @@ public String getAuthType()
@Override
public Cookie[] getCookies()
{
// TODO: optimize this.
return Request.getCookies(getRequest()).stream()
List<HttpCookie> httpCookies = Request.getCookies(getRequest());
if (httpCookies.isEmpty())
return null;
return httpCookies.stream()
.map(this::convertCookie)
.toArray(Cookie[]::new);
}

public Cookie convertCookie(HttpCookie cookie)
{
Cookie result = new Cookie(cookie.getName(), cookie.getValue());
// TODO: inbound (client-to-server) cookies don't have all these parameters.
// result.setPath(cookie.getPath());
// result.setDomain(cookie.getDomain());
// result.setSecure(cookie.isSecure());
// result.setHttpOnly(cookie.isHttpOnly());
// result.setMaxAge((int)cookie.getMaxAge());
// TODO: sameSite?
//RFC2965 defines the cookie header as supporting path and domain but RFC6265 permits only name=value
if (CookieCompliance.RFC2965.equals(getRequest().getConnectionMetaData().getHttpConfiguration().getRequestCookieCompliance()))
{
result.setPath(cookie.getPath());
result.setDomain(cookie.getDomain());
}
return result;
}

Expand Down