Issue #4247 SameSite Session Cookie (#4271)

* Issue #4247 SameSite Session Cookie

Allows sameSite cookie settings to be configured in SessionCookieConfig comments

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Issue #4247 SameSite Session Cookies

Use non versioned cookie

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Issue #4247 SameSite Session Cookies

Added test and fixed getCommentWithAttributes

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Issue #4247 - Updating unit tests for HttpCookie

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>

* Issue #4247 SameSite Session Cookie

While it may be best practise to always use Secure cookies when SameSite is None, there is nothing in the RFC that mandates it and thus I don't believe we should prevent such a configuration.  If browsers enforce this, then users will find out soon enough... and if browsers change, then we are not required to do a new release to match.

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Issue #4247 SameSite Session Cookie

For cookie comments with multiple SameSite attributes, the most strict
value is used. So `Strict` has precedence over `Lax` which has
precedence over `None`.

Signed-off-by: Greg Wilkins <gregw@webtide.com>

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__";
+    public static final String SAME_SITE_NONE_COMMENT = SAME_SITE_COMMENT + "NONE__";
-    private static final String SAME_SITE_LAX_COMMENT = SAME_SITE_COMMENT + "LAX__";
+    public 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_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;

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<String> rfc6265BadNameSource()
+    {
+        return Stream.of(
             "\"name\"",
             "name\t",
             "na me",
@@ -113,25 +123,32 @@ public class HttpCookieTest
             "{name}",
             "[name]",
             "\""
-        };
+        );
+    }
 
-        for (String badNameExample : badNameExamples)
+    @ParameterizedTest
-        {
+    @MethodSource("rfc6265BadNameSource")
-            try
+    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);
+            });
-            }
+        // make sure that exception mentions just how mad of a name it truly is
-            catch (IllegalArgumentException ex)
+        assertThat("message", ex.getMessage(),
-            {
+            allOf(
-                // System.err.printf("%s: %s%n", ex.getClass().getSimpleName(), ex.getMessage());
+                // violation of Cookie spec
-                assertThat("Testing bad name: [" + badNameExample + "]", ex.getMessage(),
+                containsString("RFC6265"),
-                    allOf(containsString("RFC6265"), containsString("RFC2616")));
+                // violation of HTTP spec
-            }
+                containsString("RFC2616")
-        }
+            ));
+    }
 
-        String[] badValueExamples = {
+    public static Stream<String> rfc6265BadValueSource()
+    {
+        return Stream.of(
             "va\tlue",
             "\t",
             "value\u0000",
@@ -143,39 +160,44 @@ public class HttpCookieTest
             "val\\ue",
             "val\"ue",
             "\""
-        };
+        );
+    }
 
-        for (String badValueExample : badValueExamples)
+    @ParameterizedTest
-        {
+    @MethodSource("rfc6265BadValueSource")
-            try
+    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();
+            });
-            }
+        assertThat("message", ex.getMessage(), containsString("RFC6265"));
-            catch (IllegalArgumentException ex)
+    }
-            {
-                // System.err.printf("%s: %s%n", ex.getClass().getSimpleName(), ex.getMessage());
-                assertThat("Testing bad value [" + badValueExample + "]", ex.getMessage(), Matchers.containsString("RFC6265"));
-            }
-        }
 
-        String[] goodNameExamples = {
+    public static Stream<String> rfc6265GoodNameSource()
+    {
+        return Stream.of(
             "name",
             "n.a.m.e",
             "na-me",
             "+name",
             "na*me",
             "na$me",
-            "#name"
+            "#name");
-        };
+    }
 
-        for (String goodNameExample : goodNameExamples)
+    @ParameterizedTest
-        {
+    @MethodSource("rfc6265GoodNameSource")
-            httpCookie = new HttpCookie(goodNameExample, "value", null, "/", 1, true, true, null, -1);
+    public void testSetRFC6265CookieGoodName(String goodNameExample)
-            // should not throw an exception
+    {
-        }
+        new HttpCookie(goodNameExample, "value", null, "/", 1, true, true, null, -1);
+        // should not throw an exception
+    }
 
+    public static Stream<String> 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<Arguments> 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);
+            assertNull(actualComment);
-            // should not throw an exception
+        }
+        else
+        {
+            assertEquals(actualComment, expectedComment);
         }
     }
 
     @Test
-    public void testGetHttpOnlyFromComment()
+    public void testGetCommentWithAttributes()
     {
-        assertTrue(HttpCookie.isHttpOnlyInComment("__HTTP_ONLY__"));
+        assertThat(HttpCookie.getCommentWithAttributes(null, false, null), nullValue());
-        assertTrue(HttpCookie.isHttpOnlyInComment("__HTTP_ONLY__comment"));
+        assertThat(HttpCookie.getCommentWithAttributes("", false, null), nullValue());
-        assertFalse(HttpCookie.isHttpOnlyInComment("comment"));
+        assertThat(HttpCookie.getCommentWithAttributes("hello", false, null), is("hello"));
-    }
 
-    @Test
+        assertThat(HttpCookie.getCommentWithAttributes(null, true, HttpCookie.SameSite.STRICT),
-    public void testGetSameSiteFromComment()
+            is("__HTTP_ONLY____SAME_SITE_STRICT__"));
-    {
+        assertThat(HttpCookie.getCommentWithAttributes("", true, HttpCookie.SameSite.NONE),
-        assertEquals(HttpCookie.getSameSiteFromComment("__SAME_SITE_NONE__"), HttpCookie.SameSite.NONE);
+            is("__HTTP_ONLY____SAME_SITE_NONE__"));
-        assertEquals(HttpCookie.getSameSiteFromComment("__SAME_SITE_LAX__"), HttpCookie.SameSite.LAX);
+        assertThat(HttpCookie.getCommentWithAttributes("hello", true, HttpCookie.SameSite.LAX),
-        assertEquals(HttpCookie.getSameSiteFromComment("__SAME_SITE_STRICT__"), HttpCookie.SameSite.STRICT);
+            is("hello__HTTP_ONLY____SAME_SITE_LAX__"));
-        assertEquals(HttpCookie.getSameSiteFromComment("__SAME_SITE_NONE____SAME_SITE_STRICT__"), HttpCookie.SameSite.NONE);
-        assertNull(HttpCookie.getSameSiteFromComment("comment"));
-    }
 
-    @Test
+        assertThat(HttpCookie.getCommentWithAttributes("__HTTP_ONLY____SAME_SITE_LAX__", false, null), nullValue());
-    public void getCommentWithoutAttributes()
+        assertThat(HttpCookie.getCommentWithAttributes("__HTTP_ONLY____SAME_SITE_LAX__", true, HttpCookie.SameSite.NONE),
-    {
+            is("__HTTP_ONLY____SAME_SITE_NONE__"));
-        assertEquals(HttpCookie.getCommentWithoutAttributes("comment__SAME_SITE_NONE__"), "comment");
+        assertThat(HttpCookie.getCommentWithAttributes("__HTTP_ONLY____SAME_SITE_LAX__hello", true, HttpCookie.SameSite.LAX),
-        assertEquals(HttpCookie.getCommentWithoutAttributes("comment__HTTP_ONLY____SAME_SITE_NONE__"), "comment");
+            is("hello__HTTP_ONLY____SAME_SITE_LAX__"));
-        assertNull(HttpCookie.getCommentWithoutAttributes("__SAME_SITE_LAX__"));
     }
 }

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 <code>HttpSession</code> 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(
-                cookie = new HttpCookie(
+                _cookieConfig.getName(),
-                    _cookieConfig.getName(),
+                id,
-                    id,
+                _cookieConfig.getDomain(),
-                    _cookieConfig.getDomain(),
+                sessionPath,
-                    sessionPath,
+                _cookieConfig.getMaxAge(),
-                    _cookieConfig.getMaxAge(),
+                _cookieConfig.isHttpOnly(),
-                    _cookieConfig.isHttpOnly(),
+                _cookieConfig.isSecure() || (isSecureRequestOnly() && requestIsSecure),
-                    _cookieConfig.isSecure() || (isSecureRequestOnly() && requestIsSecure));
+                HttpCookie.getCommentWithoutAttributes(_cookieConfig.getComment()),
-            }
+                0,
-            else
+                HttpCookie.getSameSiteFromComment(_cookieConfig.getComment()));
-            {
-                cookie = new HttpCookie(
-                    _cookieConfig.getName(),
-                    id,
-                    _cookieConfig.getDomain(),
-                    sessionPath,
-                    _cookieConfig.getMaxAge(),
-                    _cookieConfig.isHttpOnly(),
-                    _cookieConfig.isSecure() || (isSecureRequestOnly() && requestIsSecure),
-                    _sessionComment,
-                    1);
-            }
 
             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
     {