Skip to content

Commit

Permalink
Fix #9468 Space in Cookie name (#9471)
Browse files Browse the repository at this point in the history 
Added a violation to allow unquoted spaces in cookie values

Signed-off-by: gregw <gregw@webtide.com>
  • Loading branch information
@gregw
gregw committed Mar 8, 2023
1 parent 5249b90 commit 659f16d
Show file tree
Hide file tree
Showing 4 changed files with 105 additions and 48 deletions.
13 changes: 10 additions & 3 deletions 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 @@ -94,7 +94,12 @@ public enum Violation implements ComplianceViolation
/**
* Whitespace was found around the cookie name and/or around the cookie value.
*/
OPTIONAL_WHITE_SPACE("https://www.rfc-editor.org/rfc/rfc6265#section-5.2", "White space around name/value");
OPTIONAL_WHITE_SPACE("https://www.rfc-editor.org/rfc/rfc6265#section-5.2", "White space around name/value"),

/**
* Allow spaces within values without quotes.
*/
SPACE_IN_VALUES("https://www.rfc-editor.org/rfc/rfc6265#section-5.2", "Space in value");

private final String url;
private final String description;
Expand Down Expand Up @@ -130,10 +135,11 @@ public String getDescription()
* <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 = new CookieCompliance("RFC6265", of(
Violation.INVALID_COOKIES, Violation.OPTIONAL_WHITE_SPACE)
Violation.INVALID_COOKIES, Violation.OPTIONAL_WHITE_SPACE, Violation.SPACE_IN_VALUES)
);

/**
Expand All @@ -151,10 +157,11 @@ public String getDescription()
* <li>{@link Violation#INVALID_COOKIES}</li>
* <li>{@link Violation#OPTIONAL_WHITE_SPACE}</li>
* <li>{@link Violation#SPECIAL_CHARS_IN_QUOTES}</li>
* <li>{@link Violation#SPACE_IN_VALUES}</li>
* </ul>
*/
public static final CookieCompliance RFC6265_LEGACY = new CookieCompliance("RFC6265_LEGACY", EnumSet.of(
Violation.ATTRIBUTES, Violation.BAD_QUOTES, Violation.ESCAPE_IN_QUOTES, Violation.INVALID_COOKIES, Violation.OPTIONAL_WHITE_SPACE, Violation.SPECIAL_CHARS_IN_QUOTES)
Violation.ATTRIBUTES, Violation.BAD_QUOTES, Violation.ESCAPE_IN_QUOTES, Violation.INVALID_COOKIES, Violation.OPTIONAL_WHITE_SPACE, Violation.SPECIAL_CHARS_IN_QUOTES, Violation.SPACE_IN_VALUES)
);

/**
Expand Down
43 changes: 42 additions & 1 deletion 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.ESCAPE_IN_QUOTES;
import static org.eclipse.jetty.http.CookieCompliance.Violation.INVALID_COOKIES;
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 @@ -53,6 +54,7 @@ private enum State
AFTER_NAME,
VALUE,
IN_VALUE,
SPACE_IN_VALUE,
IN_QUOTED_VALUE,
ESCAPED_VALUE,
AFTER_QUOTED_VALUE,
Expand All @@ -74,6 +76,7 @@ public void parseField(String field)
String cookieComment = null;
int cookieVersion = 0;
boolean cookieInvalid = false;
int spaces = 0;

int length = field.length();
StringBuilder string = new StringBuilder();
Expand Down Expand Up @@ -220,7 +223,13 @@ else if (_complianceMode.allows(INVALID_COOKIES))
break;

case IN_VALUE:
if (c == ';' || c == ',' || c == ' ' || c == '\t')
if (c == ' ' && _complianceMode.allows(SPACE_IN_VALUES))
{
reportComplianceViolation(SPACE_IN_VALUES, field);
spaces = 1;
state = State.SPACE_IN_VALUE;
}
else if (c == ' ' || c == ';' || c == ',' || c == '\t')
{
value = string.toString();
i--;
Expand All @@ -241,6 +250,33 @@ else if (_complianceMode.allows(INVALID_COOKIES))
}
break;

case SPACE_IN_VALUE:
if (c == ' ')
{
spaces++;
}
else if (c == ';' || c == ',' || c == '\t')
{
value = string.toString();
i--;
state = State.END;
}
else if (token.isRfc6265CookieOctet())
{
string.append(" ".repeat(spaces)).append(c);
state = State.IN_VALUE;
}
else if (_complianceMode.allows(INVALID_COOKIES))
{
reportComplianceViolation(INVALID_COOKIES, field);
state = State.INVALID_COOKIE;
}
else
{
throw new InvalidCookieException("Bad Cookie value");
}
break;

case IN_QUOTED_VALUE:
if (c == '"')
{
Expand All @@ -265,6 +301,11 @@ else if (c == ',' && _complianceMode.allows(COMMA_NOT_VALID_OCTET))
reportComplianceViolation(COMMA_NOT_VALID_OCTET, field);
string.append(c);
}
else if (c == ' ' && _complianceMode.allows(SPACE_IN_VALUES))
{
reportComplianceViolation(SPACE_IN_VALUES, field);
string.append(c);
}
else if (_complianceMode.allows(INVALID_COOKIES))
{
string.append(c);
Expand Down
91 changes: 50 additions & 41 deletions jetty-http/src/test/java/org/eclipse/jetty/http/RFC6265CookieParserLenientTest.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.equalTo;
import static org.hamcrest.Matchers.is;

/**
Expand All @@ -34,8 +35,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", new String[]{"x", "y"}),
Arguments.of("key=value", new String[]{"key", "value"}),

// Tests that conform to RFC2109
// RFC2109 - token values
Expand All @@ -49,75 +50,83 @@ 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", new String[]{"abc", "xyz"}),
Arguments.of("abc=!bar", new String[]{"abc", "!bar"}),
Arguments.of("abc=#hash", new String[]{"abc", "#hash"}),
Arguments.of("abc=#hash", new String[]{"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", new String[0]),
Arguments.of("\"a\"=\"b c d\"", new String[0]),

// 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"),
Arguments.of("abc=", new String[]{"abc", ""}),
Arguments.of("abc= ", new String[]{"abc", ""}),
Arguments.of("abc= x", new String[]{"abc", "x"}),
Arguments.of("abc = ", new String[]{"abc", ""}),
Arguments.of("abc = ;", new String[]{"abc", ""}),
Arguments.of("abc = ; ", new String[]{"abc", ""}),
Arguments.of("abc = x ", new String[]{"abc", "x"}),
Arguments.of("abc=\"\"", new String[]{"abc", ""}),
Arguments.of("abc= \"\" ", new String[]{"abc", ""}),
Arguments.of("abc= \"x\" ", new String[]{"abc", "x"}),
Arguments.of("abc= \"x\" ;", new String[]{"abc", "x"}),
Arguments.of("abc= \"x\" ; ", new String[]{"abc", "x"}),
Arguments.of("abc = x y z ", new String[]{"abc", "x y z"}),
Arguments.of("abc = x y z ", new String[]{"abc", "x y z"}),
Arguments.of("abc=a:x b:y c:z", new String[]{"abc", "a:x b:y c:z"}),
Arguments.of("abc=a;x b;y c;z", new String[]{"abc", "a"}),
Arguments.of("abc=a ;b=x", new String[]{"abc", "a", "b", "x"}),

Arguments.of("abc=x y;def=w z", new String[]{"abc", "x y", "def", "w z"}),
Arguments.of("abc=\"x y\";def=w z", new String[]{"abc", "x y", "def", "w z"}),
Arguments.of("abc=x y;def=\"w z\"", new String[]{"abc", "x y", "def", "w z"}),

// 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\"", new String[]{"abc", "xyz"}),
Arguments.of("abc=\"!bar\"", new String[]{"abc", "!bar"}),
Arguments.of("abc=\"#hash\"", new String[]{"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"),
Arguments.of("!f!o!o!=wat", new String[]{"!f!o!o!", "wat"}),
Arguments.of("__MyHost=Foo", new String[]{"__MyHost", "Foo"}),
Arguments.of("some-thing-else=to-parse", new String[]{"some-thing-else", "to-parse"}),
Arguments.of("$foo=bar", new String[]{"$foo", "bar"}),

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

Arguments.of("abc=foobar!", new String[]{"abc", "foobar!"}),
Arguments.of("abc=\"foobar!\"", new String[]{"abc", "foobar!"})
);
}

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

if (expectedName == null)
assertThat("Cookies.length", cutter.names.size(), is(0));
else
assertThat(expected.length % 2, is(0));
assertThat(parser.names.size(), equalTo(expected.length / 2));
for (int i = 0; i < expected.length; i += 2)
{
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));
int cookie = i / 2;
assertThat("Cookie.name " + cookie, parser.names.get(cookie), is(expected[i]));
assertThat("Cookie.value " + cookie, parser.values.get(cookie), is(expected[i + 1]));
}
}

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

protected TestCutter()
protected TestParser()
{
parser = new RFC6265CookieParser(this, CookieCompliance.RFC6265, null);
}
Expand Down
6 changes: 3 additions & 3 deletions 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 @@ -315,15 +315,15 @@ public static List<Param> parameters()
new Param("A=1 ; B=2; C=3", "A=1", "B=2", "C=3"),
new Param("A=\"1; B=2\"; C=3", "C=3"),
new Param("A=\"1; B=2; C=3"),
new Param("A=\"1 B=2\"; C=3", "C=3"),
new Param("A=\"1 B=2\"; C=3", "A=1 B=2", "C=3"),
new Param("A=\"\"1; B=2; C=3", "B=2", "C=3"),
new Param("A=\"\" ; B=2; C=3", "A=", "B=2", "C=3"),
new Param("A=1\"\"; B=2; C=3", "B=2", "C=3"),
new Param("A=1\"; B=2; C=3", "B=2", "C=3"),
new Param("A=1\"1; B=2; C=3", "B=2", "C=3"),
new Param("A= 1; B=2; C=3", "A=1", "B=2", "C=3"),
new Param("A=\" 1\"; B=2; C=3", "B=2", "C=3"),
new Param("A=\"1 \"; B=2; C=3", "B=2", "C=3"),
new Param("A=\" 1\"; B=2; C=3", "A= 1", "B=2", "C=3"),
new Param("A=\"1 \"; B=2; C=3", "A=1 ", "B=2", "C=3"),
new Param("A=1,; B=2; C=3", "B=2", "C=3"),
new Param("A=\"1,\"; B=2; C=3", "B=2", "C=3"),
new Param("A=\\1; B=2; C=3", "B=2", "C=3"),
Expand Down

0 comments on commit 659f16d

Please sign in to comment.