Skip to content

Commit

Permalink
Servlet 61 cookie fixes (#11936)
Browse files Browse the repository at this point in the history 
* Fix #11934 Servlet 6.1 Cookies
* Added compliance mode MAINTAIN_QUOTES to keep the quotes as part of the cookie value.  Added mode RFC6265_QUOTED that includes this violation
* Never send a zero valued max-age parameter
* Partitioned is set if any attribute that is not "false" is set.
* Avoid equal sign for empty valued attributes
* Pushed responses delete max-age==0 cookies
  • Loading branch information
@gregw
gregw authored Jun 23, 2024
1 parent beff0fa commit 1e241d8
Show file tree
Hide file tree
Showing 16 changed files with 279 additions and 86 deletions.
23 changes: 21 additions & 2 deletions jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/CookieCompliance.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,12 @@ public enum Violation implements ComplianceViolation
/**
* Allow spaces within values without quotes.
*/
SPACE_IN_VALUES("https://www.rfc-editor.org/rfc/rfc6265#section-5.2", "Space in value");
SPACE_IN_VALUES("https://www.rfc-editor.org/rfc/rfc6265#section-5.2", "Space in value"),

/**
* Allows quotes to be stripped from values.
*/
STRIPPED_QUOTES("https://www.rfc-editor.org/rfc/rfc6265#section-4.1.1", "Strip quotes from the cookie value");

private final String url;
private final String description;
Expand Down Expand Up @@ -136,9 +141,23 @@ public String getDescription()
* <li>{@link Violation#INVALID_COOKIES}</li>
* <li>{@link Violation#OPTIONAL_WHITE_SPACE}</li>
* <li>{@link Violation#SPACE_IN_VALUES}</li>
* <li>{@link Violation#STRIPPED_QUOTES}</li>
* </ul>
*/
public static final CookieCompliance RFC6265 = new CookieCompliance("RFC6265", of(
Violation.INVALID_COOKIES, Violation.OPTIONAL_WHITE_SPACE, Violation.SPACE_IN_VALUES, Violation.STRIPPED_QUOTES)
);

/**
* <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#INVALID_COOKIES}</li>
* <li>{@link Violation#OPTIONAL_WHITE_SPACE}</li>
* <li>{@link Violation#SPACE_IN_VALUES}</li>
* </ul>
*/
public static final CookieCompliance RFC6265_QUOTED = new CookieCompliance("RFC6265_QUOTED", of(
Violation.INVALID_COOKIES, Violation.OPTIONAL_WHITE_SPACE, Violation.SPACE_IN_VALUES)
);

Expand Down Expand Up @@ -182,7 +201,7 @@ public String getDescription()
Violation.BAD_QUOTES, Violation.COMMA_NOT_VALID_OCTET, Violation.RESERVED_NAMES_NOT_DOLLAR_PREFIXED)
));

private static final List<CookieCompliance> KNOWN_MODES = Arrays.asList(RFC6265, RFC6265_STRICT, RFC6265_LEGACY, RFC2965, RFC2965_LEGACY);
private static final List<CookieCompliance> KNOWN_MODES = Arrays.asList(RFC6265, RFC6265_QUOTED, RFC6265_STRICT, RFC6265_LEGACY, RFC2965, RFC2965_LEGACY);
private static final AtomicInteger __custom = new AtomicInteger();

public static CookieCompliance valueOf(String name)
Expand Down
13 changes: 10 additions & 3 deletions jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCookie.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import java.util.TreeMap;

import org.eclipse.jetty.util.Index;
import org.eclipse.jetty.util.StringUtil;

/**
* <p>Implementation of RFC6265 HTTP Cookies (with fallback support for RFC2965).</p>
Expand Down Expand Up @@ -127,7 +128,7 @@ default String getPath()
*/
default boolean isSecure()
{
return Boolean.parseBoolean(getAttributes().get(SECURE_ATTRIBUTE));
return isSetToNotFalse(SECURE_ATTRIBUTE);
}

/**
Expand All @@ -145,7 +146,7 @@ default SameSite getSameSite()
*/
default boolean isHttpOnly()
{
return Boolean.parseBoolean(getAttributes().get(HTTP_ONLY_ATTRIBUTE));
return isSetToNotFalse(HTTP_ONLY_ATTRIBUTE);
}

/**
Expand All @@ -154,7 +155,13 @@ default boolean isHttpOnly()
*/
default boolean isPartitioned()
{
return Boolean.parseBoolean(getAttributes().get(PARTITIONED_ATTRIBUTE));
return isSetToNotFalse(PARTITIONED_ATTRIBUTE);
}

private boolean isSetToNotFalse(String attribute)
{
String value = getAttributes().get(attribute);
return value != null && !StringUtil.asciiEqualsIgnoreCase("false", value);
}

/**
Expand Down
14 changes: 13 additions & 1 deletion jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/RFC6265CookieParser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import static org.eclipse.jetty.http.CookieCompliance.Violation.OPTIONAL_WHITE_SPACE;
import static org.eclipse.jetty.http.CookieCompliance.Violation.SPACE_IN_VALUES;
import static org.eclipse.jetty.http.CookieCompliance.Violation.SPECIAL_CHARS_IN_QUOTES;
import static org.eclipse.jetty.http.CookieCompliance.Violation.STRIPPED_QUOTES;

/**
* Cookie parser
Expand Down Expand Up @@ -194,6 +195,8 @@ else if (_complianceMode.allows(INVALID_COOKIES))
string.setLength(0);
if (c == '"')
{
if (!_complianceMode.allows(STRIPPED_QUOTES))
string.append(c);
state = State.IN_QUOTED_VALUE;
}
else if (c == ';')
Expand Down Expand Up @@ -276,7 +279,16 @@ else if (_complianceMode.allows(INVALID_COOKIES))
case IN_QUOTED_VALUE:
if (c == '"')
{
value = string.toString();
if (_complianceMode.allows(STRIPPED_QUOTES))
{
value = string.toString();
reportComplianceViolation(STRIPPED_QUOTES, value);
}
else
{
string.append(c);
value = string.toString();
}
state = State.AFTER_QUOTED_VALUE;
}
else if (c == '\\' && _complianceMode.allows(ESCAPE_IN_QUOTES))
Expand Down
62 changes: 49 additions & 13 deletions jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/RFC6265CookieParserTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import org.junit.jupiter.params.provider.MethodSource;

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

public class RFC6265CookieParserTest
Expand All @@ -40,8 +41,8 @@ public void testRFC2965Single()

assertThat("Cookies.length", cookies.size(), is(1));
assertCookie("Cookies[0]", cookies.get(0), "Customer", "WILE_E_COYOTE", 1, "/acme");
// There are 2 attributes, so 2 violations.
assertThat(parser.violations.size(), is(2));
// There are 2 attributes, so 2 violations, plus 3 violations for stripping quotes.
assertThat(parser.violations.size(), is(2 + 3));

// Same test with RFC6265.
parser = new TestCookieParser(CookieCompliance.RFC6265);
Expand All @@ -51,36 +52,36 @@ public void testRFC2965Single()
assertCookie("Cookies[1]", cookies.get(1), "Customer", "WILE_E_COYOTE", 0, null);
assertCookie("Cookies[2]", cookies.get(2), "$Path", "/acme", 0, null);

// Normal cookies with attributes, so no violations
assertThat(parser.violations.size(), is(0));
// Normal cookies with attributes, so just 3 violations for stripping quotes
assertThat(parser.violations.size(), is(3));

// Same again, but allow attributes which are ignored
parser = new TestCookieParser(CookieCompliance.from("RFC6265,ATTRIBUTES"));
cookies = parser.parseFields(rawCookie);
assertThat("Cookies.length", cookies.size(), is(1));
assertCookie("Cookies[0]", cookies.get(0), "Customer", "WILE_E_COYOTE", 0, null);

// There are 2 attributes that are seen as violations
assertThat(parser.violations.size(), is(2));
// There are 2 attributes, so 2 violations, plus 3 violations for stripping quotes.
assertThat(parser.violations.size(), is(2 + 3));

// Same again, but allow attributes which are not ignored
parser = new TestCookieParser(CookieCompliance.from("RFC6265,ATTRIBUTE_VALUES"));
cookies = parser.parseFields(rawCookie);
assertThat("Cookies.length", cookies.size(), is(1));
assertCookie("Cookies[0]", cookies.get(0), "Customer", "WILE_E_COYOTE", 1, "/acme");

// There are 2 attributes that are seen as violations
assertThat(parser.violations.size(), is(2));
// There are 2 attributes, so 2 violations, plus 3 violations for stripping quotes.
assertThat(parser.violations.size(), is(2 + 3));

// Same test, but with RFC 6265 strict.
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);
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);

// Normal cookies with attributes, so no violations
// No violations for the maintained quotes
assertThat(parser.violations.size(), is(0));
}

Expand Down Expand Up @@ -296,6 +297,24 @@ public void testExcessiveSemicolons()
assertCookie("Cookies[1]", cookies[1], "xyz", "pdq", 0, null);
}

@Test
public void testRFC6265QuotesMaintained()
{
String rawCookie = "A=\"quotedValue\"";

Cookie[] cookies = parseCookieHeaders(CookieCompliance.RFC6265, rawCookie);
assertThat("Cookies.length", cookies.length, is(1));
assertCookie("Cookies[0]", cookies[0], "A", "quotedValue", 0, null);

cookies = parseCookieHeaders(CookieCompliance.RFC6265_QUOTED, rawCookie);
assertThat("Cookies.length", cookies.length, is(1));
assertCookie("Cookies[0]", cookies[0], "A", "\"quotedValue\"", 0, null);

cookies = parseCookieHeaders(CookieCompliance.RFC6265_STRICT, rawCookie);
assertThat("Cookies.length", cookies.length, is(1));
assertCookie("Cookies[0]", cookies[0], "A", "\"quotedValue\"", 0, null);
}

@Test
public void testRFC2965QuotedEscape()
{
Expand Down Expand Up @@ -360,6 +379,23 @@ public void testRFC6265Cookie(Param param)
}
}

@ParameterizedTest
@MethodSource("parameters")
public void testRFC6265QuotedCookie(Param param)
{
Cookie[] cookies = parseCookieHeaders(CookieCompliance.RFC6265_QUOTED, param.input);

assertThat("Cookies.length", cookies.length, is(param.expected.size()));
boolean quoted = param.input.contains("\"");

for (int i = 0; i < cookies.length; i++)
{
Cookie cookie = cookies[i];
String value = cookie.getValue();
assertThat(param.expected.get(i), anyOf(is(cookie.getName() + "=" + value), is(cookie.getName() + "=" + QuotedCSV.unquote(value))));
}
}

static class Cookie
{
String name;
Expand Down Expand Up @@ -462,7 +498,7 @@ public void addCookie(String cookieName, String cookieValue, int cookieVersion,
}
}

private static class Param
public static class Param
{
private final String input;
private final List<String> expected;
Expand Down
2 changes: 0 additions & 2 deletions jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/ConnectionMetaData.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,7 @@ default boolean isSecure()

/**
* @return whether the functionality of pushing resources is supported
* @deprecated in favour of 103 Early Hints
*/
@Deprecated(since = "12.0.1")
default boolean isPushSupported()
{
return false;
Expand Down
33 changes: 19 additions & 14 deletions jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpCookieUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import org.eclipse.jetty.util.Attributes;
import org.eclipse.jetty.util.Index;
import org.eclipse.jetty.util.QuotedStringTokenizer;
import org.eclipse.jetty.util.StringUtil;

/**
* <p>Utility methods for server-side HTTP cookie handling.</p>
Expand Down Expand Up @@ -106,16 +107,16 @@ public static HttpCookie.SameSite getSameSiteDefault(Attributes contextAttribute

public static String getSetCookie(HttpCookie httpCookie, CookieCompliance compliance)
{
if (compliance == null || CookieCompliance.RFC6265_LEGACY.compliesWith(compliance))
return getRFC6265SetCookie(httpCookie);
return getRFC2965SetCookie(httpCookie);
if (compliance != null && compliance.getName().contains("RFC2965"))
return getRFC2965SetCookie(httpCookie);
return getRFC6265SetCookie(httpCookie);
}

public static String getRFC2965SetCookie(HttpCookie httpCookie)
{
// Check arguments
String name = httpCookie.getName();
if (name == null || name.length() == 0)
if (name == null || name.isEmpty())
throw new IllegalArgumentException("Invalid cookie name");

StringBuilder builder = new StringBuilder();
Expand All @@ -129,11 +130,11 @@ public static String getRFC2965SetCookie(HttpCookie httpCookie)

// Look for domain and path fields and check if they need to be quoted.
String domain = httpCookie.getDomain();
boolean hasDomain = domain != null && domain.length() > 0;
boolean hasDomain = domain != null && !domain.isEmpty();
boolean quoteDomain = hasDomain && isQuoteNeeded(domain);

String path = httpCookie.getPath();
boolean hasPath = path != null && path.length() > 0;
boolean hasPath = path != null && !path.isEmpty();
boolean quotePath = hasPath && isQuoteNeeded(path);

// Upgrade the version if we have a comment or we need to quote value/path/domain or if they were already quoted
Expand Down Expand Up @@ -209,7 +210,7 @@ public static String getRFC6265SetCookie(HttpCookie httpCookie)
{
// Check arguments
String name = httpCookie.getName();
if (name == null || name.length() == 0)
if (name == null || name.isEmpty())
throw new IllegalArgumentException("Bad cookie name");

try
Expand All @@ -233,12 +234,12 @@ public static String getRFC6265SetCookie(HttpCookie httpCookie)

// Append path
String path = httpCookie.getPath();
if (path != null && path.length() > 0)
if (path != null && !path.isEmpty())
builder.append("; Path=").append(path);

// Append domain
String domain = httpCookie.getDomain();
if (domain != null && domain.length() > 0)
if (domain != null && !domain.isEmpty())
builder.append("; Domain=").append(domain);

// Handle max-age and/or expires
Expand All @@ -253,8 +254,11 @@ public static String getRFC6265SetCookie(HttpCookie httpCookie)
else
builder.append(HttpCookie.formatExpires(Instant.now().plusSeconds(maxAge)));

builder.append("; Max-Age=");
builder.append(maxAge);
if (maxAge > 0)
{
builder.append("; Max-Age=");
builder.append(maxAge);
}
}

// add the other fields
Expand Down Expand Up @@ -288,8 +292,9 @@ public static String getRFC6265SetCookie(HttpCookie httpCookie)
{
if (KNOWN_ATTRIBUTES.contains(e.getKey()))
continue;
builder.append("; ").append(e.getKey()).append("=");
builder.append(e.getValue());
builder.append("; ").append(e.getKey());
if (StringUtil.isNotBlank(e.getValue()))
builder.append("=").append(e.getValue());
}

return builder.toString();
Expand All @@ -304,7 +309,7 @@ public static String getRFC6265SetCookie(HttpCookie httpCookie)
*/
private static boolean isQuoteNeeded(String text)
{
if (text == null || text.length() == 0)
if (text == null || text.isEmpty())
return true;

if (QuotedStringTokenizer.isQuoted(text))
Expand Down
Loading

0 comments on commit 1e241d8

Please sign in to comment.