diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCookie.java b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCookie.java index 736a18473d1..7a2e2eb1697 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCookie.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCookie.java @@ -24,7 +24,7 @@ import java.util.concurrent.TimeUnit; import org.eclipse.jetty.util.QuotedStringTokenizer; import org.eclipse.jetty.util.StringUtil; -// TODO consider replacing this with java.net.HttpCookie +// TODO consider replacing this with java.net.HttpCookie (once it supports RFC6265) public class HttpCookie { private static final String __COOKIE_DELIM = "\",;\\ \t"; @@ -33,14 +33,14 @@ public class HttpCookie /** *If this string is found within the comment parsed with {@link #isHttpOnlyInComment(String)} the check will return true **/ - private static final String HTTP_ONLY_COMMENT = "__HTTP_ONLY__"; + public static final String HTTP_ONLY_COMMENT = "__HTTP_ONLY__"; /** *These strings are used by {@link #getSameSiteFromComment(String)} to check for a SameSite specifier in the comment **/ private static final String SAME_SITE_COMMENT = "__SAME_SITE_"; - private static final String SAME_SITE_NONE_COMMENT = SAME_SITE_COMMENT + "NONE__"; - private static final String SAME_SITE_LAX_COMMENT = SAME_SITE_COMMENT + "LAX__"; - private static final String SAME_SITE_STRICT_COMMENT = SAME_SITE_COMMENT + "STRICT__"; + public static final String SAME_SITE_NONE_COMMENT = SAME_SITE_COMMENT + "NONE__"; + public static final String SAME_SITE_LAX_COMMENT = SAME_SITE_COMMENT + "LAX__"; + public static final String SAME_SITE_STRICT_COMMENT = SAME_SITE_COMMENT + "STRICT__"; public enum SameSite { @@ -431,17 +431,17 @@ public class HttpCookie { if (comment != null) { - if (comment.contains(SAME_SITE_NONE_COMMENT)) + if (comment.contains(SAME_SITE_STRICT_COMMENT)) { - return SameSite.NONE; + return SameSite.STRICT; } if (comment.contains(SAME_SITE_LAX_COMMENT)) { return SameSite.LAX; } - if (comment.contains(SAME_SITE_STRICT_COMMENT)) + if (comment.contains(SAME_SITE_NONE_COMMENT)) { - return SameSite.STRICT; + return SameSite.NONE; } } @@ -465,6 +465,44 @@ public class HttpCookie return strippedComment.length() == 0 ? null : strippedComment; } + public static String getCommentWithAttributes(String comment, boolean httpOnly, SameSite sameSite) + { + if (comment == null && sameSite == null) + return null; + + StringBuilder builder = new StringBuilder(); + if (StringUtil.isNotBlank(comment)) + { + comment = getCommentWithoutAttributes(comment); + if (StringUtil.isNotBlank(comment)) + builder.append(comment); + } + if (httpOnly) + builder.append(HTTP_ONLY_COMMENT); + + if (sameSite != null) + { + switch (sameSite) + { + case NONE: + builder.append(SAME_SITE_NONE_COMMENT); + break; + case STRICT: + builder.append(SAME_SITE_STRICT_COMMENT); + break; + case LAX: + builder.append(SAME_SITE_LAX_COMMENT); + break; + default: + throw new IllegalArgumentException(sameSite.toString()); + } + } + + if (builder.length() == 0) + return null; + return builder.toString(); + } + public static class SetCookieHttpField extends HttpField { final HttpCookie _cookie; diff --git a/jetty-http/src/test/java/org/eclipse/jetty/http/HttpCookieTest.java b/jetty-http/src/test/java/org/eclipse/jetty/http/HttpCookieTest.java index 6fc0ef8c964..243cac89750 100644 --- a/jetty-http/src/test/java/org/eclipse/jetty/http/HttpCookieTest.java +++ b/jetty-http/src/test/java/org/eclipse/jetty/http/HttpCookieTest.java @@ -18,17 +18,25 @@ package org.eclipse.jetty.http; +import java.util.stream.Stream; + import org.hamcrest.Matchers; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; +import org.junit.jupiter.params.provider.ValueSource; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.allOf; import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.nullValue; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; -import static org.junit.jupiter.api.Assertions.fail; public class HttpCookieTest { @@ -93,7 +101,6 @@ public class HttpCookieTest httpCookie = new HttpCookie("everything", "value", "domain", "path", 0, true, true, null, -1); assertEquals("everything=value; Path=path; Domain=domain; Expires=Thu, 01-Jan-1970 00:00:00 GMT; Max-Age=0; Secure; HttpOnly", httpCookie.getRFC6265SetCookie()); - httpCookie = new HttpCookie("everything", "value", "domain", "path", 0, true, true, null, -1, HttpCookie.SameSite.NONE); assertEquals("everything=value; Path=path; Domain=domain; Expires=Thu, 01-Jan-1970 00:00:00 GMT; Max-Age=0; Secure; HttpOnly; SameSite=None", httpCookie.getRFC6265SetCookie()); @@ -102,8 +109,11 @@ public class HttpCookieTest httpCookie = new HttpCookie("everything", "value", "domain", "path", 0, true, true, null, -1, HttpCookie.SameSite.STRICT); assertEquals("everything=value; Path=path; Domain=domain; Expires=Thu, 01-Jan-1970 00:00:00 GMT; Max-Age=0; Secure; HttpOnly; SameSite=Strict", httpCookie.getRFC6265SetCookie()); + } - String[] badNameExamples = { + public static Stream rfc6265BadNameSource() + { + return Stream.of( "\"name\"", "name\t", "na me", @@ -113,25 +123,32 @@ public class HttpCookieTest "{name}", "[name]", "\"" - }; + ); + } - for (String badNameExample : badNameExamples) - { - try + @ParameterizedTest + @MethodSource("rfc6265BadNameSource") + public void testSetRFC6265CookieBadName(String badNameExample) + { + IllegalArgumentException ex = assertThrows(IllegalArgumentException.class, + () -> { - httpCookie = new HttpCookie(badNameExample, "value", null, "/", 1, true, true, null, -1); + HttpCookie httpCookie = new HttpCookie(badNameExample, "value", null, "/", 1, true, true, null, -1); httpCookie.getRFC6265SetCookie(); - fail(badNameExample); - } - catch (IllegalArgumentException ex) - { - // System.err.printf("%s: %s%n", ex.getClass().getSimpleName(), ex.getMessage()); - assertThat("Testing bad name: [" + badNameExample + "]", ex.getMessage(), - allOf(containsString("RFC6265"), containsString("RFC2616"))); - } - } + }); + // make sure that exception mentions just how mad of a name it truly is + assertThat("message", ex.getMessage(), + allOf( + // violation of Cookie spec + containsString("RFC6265"), + // violation of HTTP spec + containsString("RFC2616") + )); + } - String[] badValueExamples = { + public static Stream rfc6265BadValueSource() + { + return Stream.of( "va\tlue", "\t", "value\u0000", @@ -143,39 +160,44 @@ public class HttpCookieTest "val\\ue", "val\"ue", "\"" - }; + ); + } - for (String badValueExample : badValueExamples) - { - try + @ParameterizedTest + @MethodSource("rfc6265BadValueSource") + public void testSetRFC6265CookieBadValue(String badValueExample) + { + IllegalArgumentException ex = assertThrows(IllegalArgumentException.class, + () -> { - httpCookie = new HttpCookie("name", badValueExample, null, "/", 1, true, true, null, -1); + HttpCookie httpCookie = new HttpCookie("name", badValueExample, null, "/", 1, true, true, null, -1); httpCookie.getRFC6265SetCookie(); - fail(); - } - catch (IllegalArgumentException ex) - { - // System.err.printf("%s: %s%n", ex.getClass().getSimpleName(), ex.getMessage()); - assertThat("Testing bad value [" + badValueExample + "]", ex.getMessage(), Matchers.containsString("RFC6265")); - } - } + }); + assertThat("message", ex.getMessage(), containsString("RFC6265")); + } - String[] goodNameExamples = { + public static Stream rfc6265GoodNameSource() + { + return Stream.of( "name", "n.a.m.e", "na-me", "+name", "na*me", "na$me", - "#name" - }; + "#name"); + } - for (String goodNameExample : goodNameExamples) - { - httpCookie = new HttpCookie(goodNameExample, "value", null, "/", 1, true, true, null, -1); - // should not throw an exception - } + @ParameterizedTest + @MethodSource("rfc6265GoodNameSource") + public void testSetRFC6265CookieGoodName(String goodNameExample) + { + new HttpCookie(goodNameExample, "value", null, "/", 1, true, true, null, -1); + // should not throw an exception + } + public static Stream rfc6265GoodValueSource() + { String[] goodValueExamples = { "value", "", @@ -185,37 +207,150 @@ public class HttpCookieTest "val/ue", "v.a.l.u.e" }; + return Stream.of(goodValueExamples); + } - for (String goodValueExample : goodValueExamples) + @ParameterizedTest + @MethodSource("rfc6265GoodValueSource") + public void testSetRFC6265CookieGoodValue(String goodValueExample) + { + new HttpCookie("name", goodValueExample, null, "/", 1, true, true, null, -1); + // should not throw an exception + } + + @ParameterizedTest + @ValueSource(strings = { + "__HTTP_ONLY__", + "__HTTP_ONLY__comment", + "comment__HTTP_ONLY__" + }) + public void testIsHttpOnlyInCommentTrue(String comment) + { + assertTrue(HttpCookie.isHttpOnlyInComment(comment), "Comment \"" + comment + "\""); + } + + @ParameterizedTest + @ValueSource(strings = { + "comment", + "", + "__", + "__HTTP__ONLY__", + "__http_only__", + "HTTP_ONLY", + "__HTTP__comment__ONLY__" + }) + public void testIsHttpOnlyInCommentFalse(String comment) + { + assertFalse(HttpCookie.isHttpOnlyInComment(comment), "Comment \"" + comment + "\""); + } + + @ParameterizedTest + @ValueSource(strings = { + "__SAME_SITE_NONE__", + "__SAME_SITE_NONE____SAME_SITE_NONE__" + }) + public void testGetSameSiteFromCommentNONE(String comment) + { + assertEquals(HttpCookie.getSameSiteFromComment(comment), HttpCookie.SameSite.NONE, "Comment \"" + comment + "\""); + } + + @ParameterizedTest + @ValueSource(strings = { + "__SAME_SITE_LAX__", + "__SAME_SITE_LAX____SAME_SITE_NONE__", + "__SAME_SITE_NONE____SAME_SITE_LAX__", + "__SAME_SITE_LAX____SAME_SITE_NONE__" + }) + public void testGetSameSiteFromCommentLAX(String comment) + { + assertEquals(HttpCookie.getSameSiteFromComment(comment), HttpCookie.SameSite.LAX, "Comment \"" + comment + "\""); + } + + @ParameterizedTest + @ValueSource(strings = { + "__SAME_SITE_STRICT__", + "__SAME_SITE_NONE____SAME_SITE_STRICT____SAME_SITE_LAX__", + "__SAME_SITE_STRICT____SAME_SITE_LAX____SAME_SITE_NONE__", + "__SAME_SITE_STRICT____SAME_SITE_STRICT__" + }) + public void testGetSameSiteFromCommentSTRICT(String comment) + { + assertEquals(HttpCookie.getSameSiteFromComment(comment), HttpCookie.SameSite.STRICT, "Comment \"" + comment + "\""); + } + + /** + * A comment that does not have a declared SamesSite attribute defined + */ + @ParameterizedTest + @ValueSource(strings = { + "__HTTP_ONLY__", + "comment", + // not jetty attributes + "SameSite=None", + "SameSite=Lax", + "SameSite=Strict", + // incomplete jetty attributes + "SAME_SITE_NONE", + "SAME_SITE_LAX", + "SAME_SITE_STRICT", + }) + public void testGetSameSiteFromCommentUndefined(String comment) + { + assertNull(HttpCookie.getSameSiteFromComment(comment), "Comment \"" + comment + "\""); + } + + public static Stream getCommentWithoutAttributesSource() + { + return Stream.of( + // normal - only attribute comment + Arguments.of("__SAME_SITE_LAX__", null), + // normal - no attribute comment + Arguments.of("comment", "comment"), + // mixed - attributes at end + Arguments.of("comment__SAME_SITE_NONE__", "comment"), + Arguments.of("comment__HTTP_ONLY____SAME_SITE_NONE__", "comment"), + // mixed - attributes at start + Arguments.of("__SAME_SITE_NONE__comment", "comment"), + Arguments.of("__HTTP_ONLY____SAME_SITE_NONE__comment", "comment"), + // mixed - attributes at start and end + Arguments.of("__SAME_SITE_NONE__comment__HTTP_ONLY__", "comment"), + Arguments.of("__HTTP_ONLY__comment__SAME_SITE_NONE__", "comment") + ); + } + + @ParameterizedTest + @MethodSource("getCommentWithoutAttributesSource") + public void testGetCommentWithoutAttributes(String rawComment, String expectedComment) + { + String actualComment = HttpCookie.getCommentWithoutAttributes(rawComment); + if (expectedComment == null) { - httpCookie = new HttpCookie("name", goodValueExample, null, "/", 1, true, true, null, -1); - // should not throw an exception + assertNull(actualComment); + } + else + { + assertEquals(actualComment, expectedComment); } } @Test - public void testGetHttpOnlyFromComment() + public void testGetCommentWithAttributes() { - assertTrue(HttpCookie.isHttpOnlyInComment("__HTTP_ONLY__")); - assertTrue(HttpCookie.isHttpOnlyInComment("__HTTP_ONLY__comment")); - assertFalse(HttpCookie.isHttpOnlyInComment("comment")); - } + assertThat(HttpCookie.getCommentWithAttributes(null, false, null), nullValue()); + assertThat(HttpCookie.getCommentWithAttributes("", false, null), nullValue()); + assertThat(HttpCookie.getCommentWithAttributes("hello", false, null), is("hello")); - @Test - public void testGetSameSiteFromComment() - { - assertEquals(HttpCookie.getSameSiteFromComment("__SAME_SITE_NONE__"), HttpCookie.SameSite.NONE); - assertEquals(HttpCookie.getSameSiteFromComment("__SAME_SITE_LAX__"), HttpCookie.SameSite.LAX); - assertEquals(HttpCookie.getSameSiteFromComment("__SAME_SITE_STRICT__"), HttpCookie.SameSite.STRICT); - assertEquals(HttpCookie.getSameSiteFromComment("__SAME_SITE_NONE____SAME_SITE_STRICT__"), HttpCookie.SameSite.NONE); - assertNull(HttpCookie.getSameSiteFromComment("comment")); - } + assertThat(HttpCookie.getCommentWithAttributes(null, true, HttpCookie.SameSite.STRICT), + is("__HTTP_ONLY____SAME_SITE_STRICT__")); + assertThat(HttpCookie.getCommentWithAttributes("", true, HttpCookie.SameSite.NONE), + is("__HTTP_ONLY____SAME_SITE_NONE__")); + assertThat(HttpCookie.getCommentWithAttributes("hello", true, HttpCookie.SameSite.LAX), + is("hello__HTTP_ONLY____SAME_SITE_LAX__")); - @Test - public void getCommentWithoutAttributes() - { - assertEquals(HttpCookie.getCommentWithoutAttributes("comment__SAME_SITE_NONE__"), "comment"); - assertEquals(HttpCookie.getCommentWithoutAttributes("comment__HTTP_ONLY____SAME_SITE_NONE__"), "comment"); - assertNull(HttpCookie.getCommentWithoutAttributes("__SAME_SITE_LAX__")); + assertThat(HttpCookie.getCommentWithAttributes("__HTTP_ONLY____SAME_SITE_LAX__", false, null), nullValue()); + assertThat(HttpCookie.getCommentWithAttributes("__HTTP_ONLY____SAME_SITE_LAX__", true, HttpCookie.SameSite.NONE), + is("__HTTP_ONLY____SAME_SITE_NONE__")); + assertThat(HttpCookie.getCommentWithAttributes("__HTTP_ONLY____SAME_SITE_LAX__hello", true, HttpCookie.SameSite.LAX), + is("hello__HTTP_ONLY____SAME_SITE_LAX__")); } } diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/session/SessionHandler.java b/jetty-server/src/main/java/org/eclipse/jetty/server/session/SessionHandler.java index 65c170a0be2..7f3c455cb89 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/session/SessionHandler.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/session/SessionHandler.java @@ -530,6 +530,16 @@ public class SessionHandler extends ScopedHandler return _httpOnly; } + /** + * @return The sameSite setting for session cookies or null for no setting + * @see HttpCookie#getSameSite() + */ + @ManagedAttribute("SameSite setting for session cookies") + public HttpCookie.SameSite getSameSite() + { + return HttpCookie.getSameSiteFromComment(_sessionComment); + } + /** * Returns the HttpSession with the given session id * @@ -650,30 +660,18 @@ public class SessionHandler extends ScopedHandler sessionPath = (StringUtil.isEmpty(sessionPath)) ? "/" : sessionPath; String id = getExtendedId(session); HttpCookie cookie = null; - if (_sessionComment == null) - { - cookie = new HttpCookie( - _cookieConfig.getName(), - id, - _cookieConfig.getDomain(), - sessionPath, - _cookieConfig.getMaxAge(), - _cookieConfig.isHttpOnly(), - _cookieConfig.isSecure() || (isSecureRequestOnly() && requestIsSecure)); - } - else - { - cookie = new HttpCookie( - _cookieConfig.getName(), - id, - _cookieConfig.getDomain(), - sessionPath, - _cookieConfig.getMaxAge(), - _cookieConfig.isHttpOnly(), - _cookieConfig.isSecure() || (isSecureRequestOnly() && requestIsSecure), - _sessionComment, - 1); - } + + cookie = new HttpCookie( + _cookieConfig.getName(), + id, + _cookieConfig.getDomain(), + sessionPath, + _cookieConfig.getMaxAge(), + _cookieConfig.isHttpOnly(), + _cookieConfig.isSecure() || (isSecureRequestOnly() && requestIsSecure), + HttpCookie.getCommentWithoutAttributes(_cookieConfig.getComment()), + 0, + HttpCookie.getSameSiteFromComment(_cookieConfig.getComment())); return cookie; } @@ -814,13 +812,28 @@ public class SessionHandler extends ScopedHandler } /** - * @param httpOnly The httpOnly to set. + * Set if Session cookies should use HTTP Only + * @param httpOnly True if cookies should be HttpOnly. + * @see HttpCookie */ public void setHttpOnly(boolean httpOnly) { _httpOnly = httpOnly; } + /** + * Set Session cookie sameSite mode. + * Currently this is encoded in the session comment until sameSite is supported by {@link SessionCookieConfig} + * @param sameSite The sameSite setting for Session cookies (or null for no sameSite setting) + */ + public void setSameSite(HttpCookie.SameSite sameSite) + { + // Encode in comment whilst not supported by SessionConfig, so that it can be set/saved in + // web.xml and quickstart. + // Always pass false for httpOnly as it has it's own setter. + _sessionComment = HttpCookie.getCommentWithAttributes(_sessionComment, false, sameSite); + } + /** * @param metaManager The metaManager used for cross context session management. */ @@ -1364,6 +1377,8 @@ public class SessionHandler extends ScopedHandler * CookieConfig * * Implementation of the javax.servlet.SessionCookieConfig. + * SameSite configuration can be achieved by using setComment + * @see HttpCookie */ public final class CookieConfig implements SessionCookieConfig {