From 5105afafa45c957b326ba62886af0b6244999362 Mon Sep 17 00:00:00 2001 From: Jan Bartel Date: Mon, 16 Sep 2024 08:48:06 +1000 Subject: [PATCH 1/3] Issue #12241 Restore SameSite config as session cookie comment. (#12263) * Issue #12241 Restore SameSite config as session cookie comment in ee8/9. --- .../eclipse/jetty/ee9/nested/Response.java | 20 ++++---- .../jetty/ee9/nested/SessionHandler.java | 18 ++++++- .../jetty/ee9/nested/SessionHandlerTest.java | 47 +++++++++++++++++-- 3 files changed, 69 insertions(+), 16 deletions(-) diff --git a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Response.java b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Response.java index 37f94674459..8251fa9a576 100644 --- a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Response.java +++ b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Response.java @@ -76,20 +76,20 @@ public class Response implements HttpServletResponse * String used in the {@code Comment} attribute of {@link Cookie} * to support the {@code HttpOnly} attribute. **/ - private static final String HTTP_ONLY_COMMENT = "__HTTP_ONLY__"; + protected static final String HTTP_ONLY_COMMENT = "__HTTP_ONLY__"; /** * String used in the {@code Comment} attribute of {@link Cookie} * to support the {@code Partitioned} attribute. **/ - private static final String PARTITIONED_COMMENT = "__PARTITIONED__"; + protected static final String PARTITIONED_COMMENT = "__PARTITIONED__"; /** * The strings used in the {@code Comment} attribute of {@link Cookie} * to support the {@code SameSite} attribute. **/ - 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__"; + protected static final String SAME_SITE_COMMENT = "__SAME_SITE_"; + protected static final String SAME_SITE_NONE_COMMENT = SAME_SITE_COMMENT + "NONE__"; + protected static final String SAME_SITE_LAX_COMMENT = SAME_SITE_COMMENT + "LAX__"; + protected static final String SAME_SITE_STRICT_COMMENT = SAME_SITE_COMMENT + "STRICT__"; public enum OutputType { @@ -1494,7 +1494,7 @@ public class Response implements HttpServletResponse } } - private static class HttpCookieFacade implements HttpCookie + protected static class HttpCookieFacade implements HttpCookie { private final Cookie _cookie; private final String _comment; @@ -1622,12 +1622,12 @@ public class Response implements HttpServletResponse return comment != null && comment.contains(HTTP_ONLY_COMMENT); } - private static boolean isPartitionedInComment(String comment) + protected static boolean isPartitionedInComment(String comment) { return comment != null && comment.contains(PARTITIONED_COMMENT); } - private static SameSite getSameSiteFromComment(String comment) + protected static SameSite getSameSiteFromComment(String comment) { if (comment == null) return null; @@ -1640,7 +1640,7 @@ public class Response implements HttpServletResponse return null; } - private static String getCommentWithoutAttributes(String comment) + protected static String getCommentWithoutAttributes(String comment) { if (comment == null) return null; diff --git a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/SessionHandler.java b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/SessionHandler.java index aae6b5596b6..214e2b871ce 100644 --- a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/SessionHandler.java +++ b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/SessionHandler.java @@ -53,6 +53,7 @@ import org.eclipse.jetty.session.SessionConfig; import org.eclipse.jetty.session.SessionIdManager; import org.eclipse.jetty.session.SessionManager; import org.eclipse.jetty.util.Callback; +import org.eclipse.jetty.util.StringUtil; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -613,7 +614,8 @@ public class SessionHandler extends ScopedHandler implements SessionConfig.Mutab * CookieConfig * * Implementation of the jakarta.servlet.SessionCookieConfig. - * SameSite configuration can be achieved by using setComment + * SameSite configuration can be achieved by using setComment. + * Partitioned configuration can be achieved by using setComment. * * @see HttpCookie */ @@ -671,7 +673,19 @@ public class SessionHandler extends ScopedHandler implements SessionConfig.Mutab public void setComment(String comment) { checkAvailable(); - _sessionManager.setSessionComment(comment); + + if (!StringUtil.isEmpty(comment)) + { + HttpCookie.SameSite sameSite = Response.HttpCookieFacade.getSameSiteFromComment(comment); + if (sameSite != null) + _sessionManager.setSameSite(sameSite); + + boolean partitioned = Response.HttpCookieFacade.isPartitionedInComment(comment); + if (partitioned) + _sessionManager.setPartitioned(partitioned); + + _sessionManager.setSessionComment(Response.HttpCookieFacade.getCommentWithoutAttributes(comment)); + } } @Override diff --git a/jetty-ee9/jetty-ee9-nested/src/test/java/org/eclipse/jetty/ee9/nested/SessionHandlerTest.java b/jetty-ee9/jetty-ee9-nested/src/test/java/org/eclipse/jetty/ee9/nested/SessionHandlerTest.java index 8053c8315ca..cf0ed7df1d1 100644 --- a/jetty-ee9/jetty-ee9-nested/src/test/java/org/eclipse/jetty/ee9/nested/SessionHandlerTest.java +++ b/jetty-ee9/jetty-ee9-nested/src/test/java/org/eclipse/jetty/ee9/nested/SessionHandlerTest.java @@ -168,7 +168,7 @@ public class SessionHandlerTest } @Test - public void testSessionCookie() throws Exception + public void testSessionCookieConfig() throws Exception { Server server = new Server(); MockSessionIdManager idMgr = new MockSessionIdManager(server); @@ -190,12 +190,51 @@ public class SessionHandlerTest sessionCookieConfig.setSecure(false); sessionCookieConfig.setPath("/foo"); sessionCookieConfig.setMaxAge(99); + + //test setting SameSite and Partitioned the old way in the comment + sessionCookieConfig.setComment(Response.PARTITIONED_COMMENT + " " + Response.SAME_SITE_STRICT_COMMENT); - //for < ee10, SameSite cannot be set on the SessionCookieConfig, only on the SessionManager, or - //a default value on the context attribute org.eclipse.jetty.cookie.sameSiteDefault + HttpCookie cookie = mgr.getSessionManager().getSessionCookie(session, false); + assertEquals("SPECIAL", cookie.getName()); + assertEquals("universe", cookie.getDomain()); + assertEquals("/foo", cookie.getPath()); + assertFalse(cookie.isHttpOnly()); + assertFalse(cookie.isSecure()); + assertTrue(cookie.isPartitioned()); + assertEquals(99, cookie.getMaxAge()); + assertEquals(HttpCookie.SameSite.STRICT, cookie.getSameSite()); + + String cookieStr = HttpCookieUtils.getRFC6265SetCookie(cookie); + assertThat(cookieStr, containsString("; Partitioned; SameSite=Strict")); + } + + @Test + public void testSessionCookieViaSetters() throws Exception + { + Server server = new Server(); + MockSessionIdManager idMgr = new MockSessionIdManager(server); + idMgr.setWorkerName("node1"); + SessionHandler mgr = new SessionHandler(); + MockSessionCache cache = new MockSessionCache(mgr.getSessionManager()); + cache.setSessionDataStore(new NullSessionDataStore()); + mgr.setSessionCache(cache); + mgr.setSessionIdManager(idMgr); + + long now = System.currentTimeMillis(); + + ManagedSession session = new ManagedSession(mgr.getSessionManager(), new SessionData("123", "_foo", "0.0.0.0", now, now, now, 30)); + session.setExtendedId("123.node1"); + + //test setting up session cookie via setters on SessionHandler + mgr.setSessionCookie("SPECIAL"); + mgr.setSessionDomain("universe"); + mgr.setHttpOnly(false); + mgr.setSecureCookies(false); + mgr.setSessionPath("/foo"); + mgr.setMaxCookieAge(99); mgr.setSameSite(HttpCookie.SameSite.STRICT); mgr.setPartitioned(true); - + HttpCookie cookie = mgr.getSessionManager().getSessionCookie(session, false); assertEquals("SPECIAL", cookie.getName()); assertEquals("universe", cookie.getDomain()); From 2018c439b699e386b4a5d5aec9726d684aedd2c9 Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Fri, 13 Sep 2024 10:06:47 +0200 Subject: [PATCH 2/3] #12268 reset _iterate flag when another processing is scheduled Signed-off-by: Ludovic Orban --- .../eclipse/jetty/util/IteratingCallback.java | 2 ++ .../jetty/util/IteratingCallbackTest.java | 35 +++++++++++++++++++ 2 files changed, 37 insertions(+) diff --git a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/IteratingCallback.java b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/IteratingCallback.java index 155577f58e2..9cd22280508 100644 --- a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/IteratingCallback.java +++ b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/IteratingCallback.java @@ -295,6 +295,7 @@ public abstract class IteratingCallback implements Callback } case SCHEDULED: { + _iterate = false; // we won the race against the callback, so the callback has to process and we can break processing _state = State.PENDING; break processing; @@ -321,6 +322,7 @@ public abstract class IteratingCallback implements Callback callOnSuccess = true; if (action != Action.SCHEDULED) throw new IllegalStateException(String.format("%s[action=%s]", this, action)); + _iterate = false; // we lost the race, so we have to keep processing _state = State.PROCESSING; continue; diff --git a/jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/IteratingCallbackTest.java b/jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/IteratingCallbackTest.java index 00f6b007b7d..bba07604ddd 100644 --- a/jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/IteratingCallbackTest.java +++ b/jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/IteratingCallbackTest.java @@ -22,7 +22,11 @@ import org.eclipse.jetty.util.thread.Scheduler; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; +import static org.awaitility.Awaitility.await; +import static org.hamcrest.Matchers.is; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertThrows; @@ -45,6 +49,37 @@ public class IteratingCallbackTest scheduler.stop(); } + @ParameterizedTest + @ValueSource(booleans = {true, false}) + public void testIterateWhileProcessingLoopCount(boolean succeededWinsRace) + { + var icb = new IteratingCallback() + { + int counter = 0; + + @Override + protected Action process() + { + int counter = this.counter++; + if (counter == 0) + { + iterate(); + if (succeededWinsRace) + succeeded(); + else + scheduler.schedule(this::succeeded, 100, TimeUnit.MILLISECONDS); + return Action.SCHEDULED; + } + return Action.IDLE; + } + }; + + icb.iterate(); + + await().atMost(5, TimeUnit.SECONDS).until(icb::isIdle, is(true)); + assertEquals(2, icb.counter); + } + @Test public void testNonWaitingProcess() throws Exception { From ceee65a7e509a767c0127b4d5f2111fb79803d8c Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Fri, 13 Sep 2024 10:31:22 +0200 Subject: [PATCH 3/3] #12268 do not rely on time Signed-off-by: Ludovic Orban --- .../org/eclipse/jetty/util/IteratingCallback.java | 8 ++++++++ .../eclipse/jetty/util/IteratingCallbackTest.java | 12 ++++++++++-- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/IteratingCallback.java b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/IteratingCallback.java index 9cd22280508..0d71ce2f73f 100644 --- a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/IteratingCallback.java +++ b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/IteratingCallback.java @@ -545,6 +545,14 @@ public abstract class IteratingCallback implements Callback onCompleteFailure(failure); } + boolean isPending() + { + try (AutoLock ignored = _lock.lock()) + { + return _state == State.PENDING; + } + } + /** * @return whether this callback is idle, and {@link #iterate()} needs to be called */ diff --git a/jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/IteratingCallbackTest.java b/jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/IteratingCallbackTest.java index bba07604ddd..edc02749b95 100644 --- a/jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/IteratingCallbackTest.java +++ b/jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/IteratingCallbackTest.java @@ -65,9 +65,17 @@ public class IteratingCallbackTest { iterate(); if (succeededWinsRace) + { succeeded(); + } else - scheduler.schedule(this::succeeded, 100, TimeUnit.MILLISECONDS); + { + new Thread(() -> + { + await().atMost(5, TimeUnit.SECONDS).until(this::isPending, is(true)); + succeeded(); + }).start(); + } return Action.SCHEDULED; } return Action.IDLE; @@ -76,7 +84,7 @@ public class IteratingCallbackTest icb.iterate(); - await().atMost(5, TimeUnit.SECONDS).until(icb::isIdle, is(true)); + await().atMost(10, TimeUnit.SECONDS).until(icb::isIdle, is(true)); assertEquals(2, icb.counter); }