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.

Sign up for GitHub

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

Servlet 61 cookie fixes #11936

Merged
merged 9 commits into from
Jun 23, 2024
Merged

Servlet 61 cookie fixes #11936

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
6c41fff
Fix #11934 Servlet 6.1 Cookies
gregw Jun 20, 2024
acced9f
Fix #11934 Servlet 6.1 Cookies
gregw Jun 20, 2024
d96d4bc
Fix #11934 Servlet 6.1 Cookies
gregw Jun 20, 2024
120ae21
Fix #11934 Servlet 6.1 Cookies
gregw Jun 20, 2024
2c85b14
Pushed responses delete max-age==0 cookies
gregw Jun 20, 2024
709260e
Fixed tests to expect quotes
gregw Jun 20, 2024
f96f7ae
updates from review
gregw Jun 21, 2024
111c84a
Fixed flaky tests
gregw Jun 21, 2024
bc714ca
updates from review
gregw Jun 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 24 additions & 4 deletions jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/CookieCompliance.java
View file
Open in desktop
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"),

/**
* Maintain quotes around values .
*/
MAINTAIN_QUOTES("https://www.rfc-editor.org/rfc/rfc6265#section-4.1.1", "Quotes are part of the cookie value");

private final String url;
private final String description;
Expand Down Expand Up @@ -142,10 +147,24 @@ public String getDescription()
Violation.INVALID_COOKIES, Violation.OPTIONAL_WHITE_SPACE, Violation.SPACE_IN_VALUES)
);

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

/**
* A CookieCompliance mode that enforces <a href="https://tools.ietf.org/html/rfc6265">RFC 6265</a> compliance.
*/
public static final CookieCompliance RFC6265_STRICT = new CookieCompliance("RFC6265_STRICT", noneOf(Violation.class));
public static final CookieCompliance RFC6265_STRICT = new CookieCompliance("RFC6265_STRICT", of(Violation.MAINTAIN_QUOTES));

/**
* <p>A CookieCompliance mode that enforces <a href="https://tools.ietf.org/html/rfc6265">RFC 6265</a> compliance,
Expand Down Expand Up @@ -176,13 +195,14 @@ public String getDescription()
* <li>{@link Violation#BAD_QUOTES}</li>
* <li>{@link Violation#COMMA_NOT_VALID_OCTET}</li>
* <li>{@link Violation#RESERVED_NAMES_NOT_DOLLAR_PREFIXED}</li>
* <li>{@link Violation#MAINTAIN_QUOTES}</li>
* </ul>
*/
public static final CookieCompliance RFC2965 = new CookieCompliance("RFC2965", complementOf(of(
Violation.BAD_QUOTES, Violation.COMMA_NOT_VALID_OCTET, Violation.RESERVED_NAMES_NOT_DOLLAR_PREFIXED)
Violation.BAD_QUOTES, Violation.COMMA_NOT_VALID_OCTET, Violation.RESERVED_NAMES_NOT_DOLLAR_PREFIXED, Violation.MAINTAIN_QUOTES)
));

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
Open in desktop
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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import static org.eclipse.jetty.http.CookieCompliance.Violation.COMMA_SEPARATOR;
import static org.eclipse.jetty.http.CookieCompliance.Violation.ESCAPE_IN_QUOTES;
import static org.eclipse.jetty.http.CookieCompliance.Violation.INVALID_COOKIES;
import static org.eclipse.jetty.http.CookieCompliance.Violation.MAINTAIN_QUOTES;
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;
Expand Down Expand Up @@ -194,6 +195,8 @@ else if (_complianceMode.allows(INVALID_COOKIES))
string.setLength(0);
if (c == '"')
{
if (_complianceMode.allows(MAINTAIN_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(MAINTAIN_QUOTES))
{
string.append(c);
value = string.toString();
reportComplianceViolation(MAINTAIN_QUOTES, value);
gregw marked this conversation as resolved.
Show resolved Hide resolved
}
else
{
value = string.toString();
}
state = State.AFTER_QUOTED_VALUE;
}
else if (c == '\\' && _complianceMode.allows(ESCAPE_IN_QUOTES))
Expand Down
44 changes: 38 additions & 6 deletions jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/RFC6265CookieParserTest.java
View file
Open in desktop
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 Down Expand Up @@ -76,12 +77,12 @@ public void testRFC2965Single()
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
assertThat(parser.violations.size(), is(0));
// Three violations for each of the maintained quotes
assertThat(parser.violations.size(), is(3));
}

/**
Expand Down Expand Up @@ -296,6 +297,20 @@ 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);
gregw marked this conversation as resolved.
Show resolved Hide resolved
}

@Test
public void testRFC2965QuotedEscape()
{
Expand Down Expand Up @@ -360,6 +375,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 +494,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
Open in desktop
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()
gregw marked this conversation as resolved.
Show resolved Hide resolved
{
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
Open in desktop
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
Loading