From 48f5004a28e4e31b6a20e50f2ae44dd699c54e48 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=B3=95=E2=B2=9B=CF=84=E2=B2=89=E2=B2=85=E2=B2=A5?= =?UTF-8?q?=E2=B2=89=E2=B3=8F=CF=84=E2=B2=9F=E2=B2=85=20=F0=9F=95=B5?= =?UTF-8?q?=F0=9F=8F=BB?= <192411347+g0w6y@users.noreply.github.com> Date: Sat, 7 Nov 2026 14:45:36 +0530 Subject: [PATCH 1/2] WW-5841 Restrict Struts Boot to cookie-only session tracking and add SameSite=Lax Without a web.xml, the embedded Tomcat context defaulted to COOKIE+URL session tracking, so a session id supplied only via ";jsessionid=" in the URL was honoured with no fresh Set-Cookie -- a session-fixation vector. Live-reproduced before this fix: a session established by cookie was resumed by replaying the id via the URL parameter with no Cookie header sent at all. Restricts session tracking to COOKIE only, and the session cookie now carries SameSite=Lax (previously unset). Secure is deliberately not forced: Boot opens a plain HTTP listener by default, and an operator terminating TLS in front of it sets Secure themselves, as with any other servlet container. Boot is part of 7.5.0 (unreleased, no production deployments yet), so this lands before anyone is exposed to it. Tests: StrutsBootTomcatTest gains testSessionCookieHasSameSiteLax and a URL-supplied-session-id-is-not-honoured test, with a cookie-based-session- still-works control. Full boot suite: 52/0 failures. --- .../apache/struts2/boot/StrutsBootServer.java | 28 ++++++++++- .../struts2/boot/StrutsBootTomcatTest.java | 47 ++++++++++++++++++ .../apache/struts2/boot/fixture/HelloApp.java | 1 + .../boot/fixture/SessionProbeAction.java | 49 +++++++++++++++++++ .../test/resources/templates/sessionProbe.ftl | 21 ++++++++ 5 files changed, 145 insertions(+), 1 deletion(-) create mode 100644 boot/src/test/java/org/apache/struts2/boot/fixture/SessionProbeAction.java create mode 100644 boot/src/test/resources/templates/sessionProbe.ftl diff --git a/boot/src/main/java/org/apache/struts2/boot/StrutsBootServer.java b/boot/src/main/java/org/apache/struts2/boot/StrutsBootServer.java index d73b21ed70..44304209b4 100644 --- a/boot/src/main/java/org/apache/struts2/boot/StrutsBootServer.java +++ b/boot/src/main/java/org/apache/struts2/boot/StrutsBootServer.java @@ -20,6 +20,7 @@ import jakarta.servlet.FilterConfig; import jakarta.servlet.ServletException; +import jakarta.servlet.SessionTrackingMode; import org.apache.catalina.Context; import org.apache.catalina.LifecycleState; import org.apache.catalina.connector.Connector; @@ -30,6 +31,7 @@ import org.apache.struts2.dispatcher.filter.StrutsPrepareAndExecuteFilter; import org.apache.tomcat.util.descriptor.web.FilterDef; import org.apache.tomcat.util.descriptor.web.FilterMap; +import org.apache.tomcat.util.http.Rfc6265CookieProcessor; import java.io.File; import java.io.IOException; @@ -39,6 +41,7 @@ import java.nio.file.Path; import java.nio.file.attribute.PosixFilePermissions; import java.util.Comparator; +import java.util.EnumSet; import java.util.UUID; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicReference; @@ -71,8 +74,17 @@ static StrutsBootServer start(ActionRegistry registry, BootSettings settings) { hideErrorReportDetails(tomcat); Path docBase = Files.createDirectory(baseDir.resolve("webapp")); Context context = tomcat.addContext(settings.contextPath(), docBase.toString()); + hardenSessionTracking(context); context.addServletContainerInitializer( - (classes, servletContext) -> servletContext.setAttribute(ActionRegistry.SERVLET_CONTEXT_ATTRIBUTE, registry), + (classes, servletContext) -> { + servletContext.setAttribute(ActionRegistry.SERVLET_CONTEXT_ATTRIBUTE, registry); + // Without a web.xml, a bare embedded context defaults to the servlet-spec + // tracking modes (COOKIE + URL): a session id supplied as ";jsessionid=" + // in the URL is accepted and kept alive with no Set-Cookie, which is a + // session-fixation vector (the id can be planted via a link/Referer + // before the victim authenticates). Restrict to COOKIE only. + servletContext.setSessionTrackingModes(EnumSet.of(SessionTrackingMode.COOKIE)); + }, null); Tomcat.addServlet(context, "default", new DefaultServlet()); context.addServletMappingDecoded("/", "default"); @@ -177,6 +189,20 @@ private static void assertContextStarted(Context context, CapturingStrutsFilter } } + /** + * The session id cookie (JSESSIONID) otherwise carries no SameSite attribute at all under + * the embedded default, which is weaker than every modern browser's own unset-cookie + * default (treated as Lax). Secure is deliberately not forced here: Boot opens a plain + * HTTP listener by default (§5/§5a transport is the operator's), and a Secure cookie would + * simply be refused by the browser on that default deployment -- an operator who terminates + * TLS in front of Boot sets this themselves, as with any other servlet container. + */ + private static void hardenSessionTracking(Context context) { + Rfc6265CookieProcessor cookieProcessor = new Rfc6265CookieProcessor(); + cookieProcessor.setSameSiteCookies("lax"); + context.setCookieProcessor(cookieProcessor); + } + private static void hideErrorReportDetails(Tomcat tomcat) { ErrorReportValve valve = new ErrorReportValve(); valve.setShowReport(false); diff --git a/boot/src/test/java/org/apache/struts2/boot/StrutsBootTomcatTest.java b/boot/src/test/java/org/apache/struts2/boot/StrutsBootTomcatTest.java index c3009eb250..5ca3e9d059 100644 --- a/boot/src/test/java/org/apache/struts2/boot/StrutsBootTomcatTest.java +++ b/boot/src/test/java/org/apache/struts2/boot/StrutsBootTomcatTest.java @@ -125,6 +125,53 @@ public void testErrorPageHidesStackTraceAndServerInfo() throws Exception { assertThat(response.body()).doesNotContain("at org.apache").doesNotContain("Apache Tomcat/"); } + /** + * The session id cookie must carry SameSite -- the + * embedded default (no web.xml, bare {@code Context}) sets none at all. + */ + @Test + public void testSessionCookieHasSameSiteLax() throws Exception { + HttpResponse response = get(server.baseUrl() + "/sessionProbe.action"); + + assertThat(response.headers().firstValue("Set-Cookie")).hasValueSatisfying(cookie -> + assertThat(cookie).contains("SameSite=Lax")); + } + + /** + * Without a web.xml, a bare embedded {@code Context} + * defaults to the servlet-spec tracking modes (COOKIE + URL), so a session id supplied + * only via {@code ;jsessionid=} in the URL (no cookie at all) is honoured and kept alive + * with no fresh {@code Set-Cookie} -- the session-fixation vector. Control: the normal + * cookie-based flow must still carry a session across requests. + */ + @Test + public void testUrlSuppliedSessionIdIsNotHonored() throws Exception { + HttpResponse first = get(server.baseUrl() + "/sessionProbe.action"); + assertThat(first.body()).contains("COUNTER=1"); + String sessionId = extractSessionId(first); + + // Attacker-style request: the session id travels only in the URL, never as a cookie. + HttpResponse viaUrl = get(server.baseUrl() + "/sessionProbe.action;jsessionid=" + sessionId); + // GREEN: a brand-new session is created (counter resets to 1, a fresh id is issued) -- + // the URL-supplied id from the first request was not resumed. + assertThat(viaUrl.body()).contains("COUNTER=1"); + assertThat(extractSessionId(viaUrl)).isNotEqualTo(sessionId); + + // Control: the ordinary cookie-based flow still resumes the same session. + HttpResponse viaCookie = CLIENT.send( + HttpRequest.newBuilder(URI.create(server.baseUrl() + "/sessionProbe.action")) + .header("Cookie", "JSESSIONID=" + sessionId) + .GET().build(), + HttpResponse.BodyHandlers.ofString()); + assertThat(viaCookie.body()).contains("COUNTER=2"); + } + + private static String extractSessionId(HttpResponse response) { + String setCookie = response.headers().firstValue("Set-Cookie") + .orElseThrow(() -> new AssertionError("Expected a Set-Cookie header")); + return setCookie.substring("JSESSIONID=".length(), setCookie.indexOf(';')); + } + @Test public void testAddressRestrictsListeningInterface() throws Exception { try (StrutsBootServer local = StrutsBoot.start(HelloApp.class, "--struts.boot.port=0", "--struts.boot.address=127.0.0.1")) { diff --git a/boot/src/test/java/org/apache/struts2/boot/fixture/HelloApp.java b/boot/src/test/java/org/apache/struts2/boot/fixture/HelloApp.java index 541b5a6e83..0edcc305ec 100644 --- a/boot/src/test/java/org/apache/struts2/boot/fixture/HelloApp.java +++ b/boot/src/test/java/org/apache/struts2/boot/fixture/HelloApp.java @@ -29,6 +29,7 @@ public class HelloApp implements StrutsBootApplication { public void actions(ActionRegistry actions) { actions.action("hello", HelloAction.class).success("hello.ftl"); actions.action("boom", ThrowingAction.class).success("hello.ftl"); + actions.action("sessionProbe", SessionProbeAction.class).success("sessionProbe.ftl"); actions.namespace("/admin", admin -> { admin.action("users", UserAction.class).method("list").success("admin/users.ftl"); admin.action("saveUser", UserAction.class).method("save").httpMethods(POST).redirectAction("success", "users"); diff --git a/boot/src/test/java/org/apache/struts2/boot/fixture/SessionProbeAction.java b/boot/src/test/java/org/apache/struts2/boot/fixture/SessionProbeAction.java new file mode 100644 index 0000000000..1e2be1b5a4 --- /dev/null +++ b/boot/src/test/java/org/apache/struts2/boot/fixture/SessionProbeAction.java @@ -0,0 +1,49 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.struts2.boot.fixture; + +import org.apache.struts2.action.SessionAware; + +import java.util.Map; + +/** + * Forces an {@link jakarta.servlet.http.HttpSession} to exist and reports a counter stored in + * it, so a test can tell whether two requests landed in the *same* session (counter keeps + * incrementing) or different ones (counter resets to 1). + */ +public class SessionProbeAction implements SessionAware { + + private Map session; + + public String execute() { + Object current = session.get("probeCounter"); + int next = (current instanceof Integer i ? i : 0) + 1; + session.put("probeCounter", next); + return "success"; + } + + public String getCounter() { + return String.valueOf(session.get("probeCounter")); + } + + @Override + public void withSession(Map session) { + this.session = session; + } +} diff --git a/boot/src/test/resources/templates/sessionProbe.ftl b/boot/src/test/resources/templates/sessionProbe.ftl new file mode 100644 index 0000000000..db042437f9 --- /dev/null +++ b/boot/src/test/resources/templates/sessionProbe.ftl @@ -0,0 +1,21 @@ +<#-- +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +--> +COUNTER=<@s.property value="counter"/> From bfc956e43e59b43983b8a36bb67fa4edafb82b71 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=B3=95=E2=B2=9B=CF=84=E2=B2=89=E2=B2=85=E2=B2=A5?= =?UTF-8?q?=E2=B2=89=E2=B3=8F=CF=84=E2=B2=9F=E2=B2=85=20=F0=9F=95=B5?= =?UTF-8?q?=F0=9F=8F=BB?= <192411347+g0w6y@users.noreply.github.com> Date: Sun, 8 Nov 2026 09:00:02 +0530 Subject: [PATCH 2/2] WW-5841 Address review: single session-tracking method, shorter comments, threat model bullets - fold the tracking-mode initializer and the SameSite cookie processor into one method, restrictSessionTracking(Context) - drop the Secure claim from the Javadoc and reduce the comments to what the code does - reduce the test Javadoc and drop the narrating comment lines - THREAT_MODEL.md section 5: add the cookie-only tracking and SameSite=Lax defaults to the Boot container defaults list --- THREAT_MODEL.md | 2 ++ .../apache/struts2/boot/StrutsBootServer.java | 26 +++++++------------ .../struts2/boot/StrutsBootTomcatTest.java | 16 ++---------- 3 files changed, 13 insertions(+), 31 deletions(-) diff --git a/THREAT_MODEL.md b/THREAT_MODEL.md index d11007a1fd..f6a8521aa5 100644 --- a/THREAT_MODEL.md +++ b/THREAT_MODEL.md @@ -238,6 +238,8 @@ documented anti-pattern is `OUT-OF-MODEL`. *(documented/inferred — §14 Q-defa - error responses render without exception report or server version - `DefaultServlet` serves only an empty per-run docBase - Tomcat's base directory is a fresh owner-only temp directory, deleted on shutdown + - session tracking is cookie-only + - the session cookie carries `SameSite=Lax` - `devMode` is never switched on A deviation from these defaults is a Struts finding. Anything the developer adds on diff --git a/boot/src/main/java/org/apache/struts2/boot/StrutsBootServer.java b/boot/src/main/java/org/apache/struts2/boot/StrutsBootServer.java index 44304209b4..46935e34b3 100644 --- a/boot/src/main/java/org/apache/struts2/boot/StrutsBootServer.java +++ b/boot/src/main/java/org/apache/struts2/boot/StrutsBootServer.java @@ -74,17 +74,9 @@ static StrutsBootServer start(ActionRegistry registry, BootSettings settings) { hideErrorReportDetails(tomcat); Path docBase = Files.createDirectory(baseDir.resolve("webapp")); Context context = tomcat.addContext(settings.contextPath(), docBase.toString()); - hardenSessionTracking(context); + restrictSessionTracking(context); context.addServletContainerInitializer( - (classes, servletContext) -> { - servletContext.setAttribute(ActionRegistry.SERVLET_CONTEXT_ATTRIBUTE, registry); - // Without a web.xml, a bare embedded context defaults to the servlet-spec - // tracking modes (COOKIE + URL): a session id supplied as ";jsessionid=" - // in the URL is accepted and kept alive with no Set-Cookie, which is a - // session-fixation vector (the id can be planted via a link/Referer - // before the victim authenticates). Restrict to COOKIE only. - servletContext.setSessionTrackingModes(EnumSet.of(SessionTrackingMode.COOKIE)); - }, + (classes, servletContext) -> servletContext.setAttribute(ActionRegistry.SERVLET_CONTEXT_ATTRIBUTE, registry), null); Tomcat.addServlet(context, "default", new DefaultServlet()); context.addServletMappingDecoded("/", "default"); @@ -190,14 +182,14 @@ private static void assertContextStarted(Context context, CapturingStrutsFilter } /** - * The session id cookie (JSESSIONID) otherwise carries no SameSite attribute at all under - * the embedded default, which is weaker than every modern browser's own unset-cookie - * default (treated as Lax). Secure is deliberately not forced here: Boot opens a plain - * HTTP listener by default (§5/§5a transport is the operator's), and a Secure cookie would - * simply be refused by the browser on that default deployment -- an operator who terminates - * TLS in front of Boot sets this themselves, as with any other servlet container. + * Restricts the embedded context to cookie-only session tracking with {@code SameSite=Lax}; + * Secure is left to the operator's TLS setup. */ - private static void hardenSessionTracking(Context context) { + private static void restrictSessionTracking(Context context) { + // Tomcat defaults to COOKIE+URL when no web.xml sets ; keep cookies only. + context.addServletContainerInitializer( + (classes, servletContext) -> servletContext.setSessionTrackingModes(EnumSet.of(SessionTrackingMode.COOKIE)), + null); Rfc6265CookieProcessor cookieProcessor = new Rfc6265CookieProcessor(); cookieProcessor.setSameSiteCookies("lax"); context.setCookieProcessor(cookieProcessor); diff --git a/boot/src/test/java/org/apache/struts2/boot/StrutsBootTomcatTest.java b/boot/src/test/java/org/apache/struts2/boot/StrutsBootTomcatTest.java index 5ca3e9d059..d693b3fb90 100644 --- a/boot/src/test/java/org/apache/struts2/boot/StrutsBootTomcatTest.java +++ b/boot/src/test/java/org/apache/struts2/boot/StrutsBootTomcatTest.java @@ -125,10 +125,7 @@ public void testErrorPageHidesStackTraceAndServerInfo() throws Exception { assertThat(response.body()).doesNotContain("at org.apache").doesNotContain("Apache Tomcat/"); } - /** - * The session id cookie must carry SameSite -- the - * embedded default (no web.xml, bare {@code Context}) sets none at all. - */ + /** The session cookie carries {@code SameSite=Lax}. */ @Test public void testSessionCookieHasSameSiteLax() throws Exception { HttpResponse response = get(server.baseUrl() + "/sessionProbe.action"); @@ -137,23 +134,14 @@ public void testSessionCookieHasSameSiteLax() throws Exception { assertThat(cookie).contains("SameSite=Lax")); } - /** - * Without a web.xml, a bare embedded {@code Context} - * defaults to the servlet-spec tracking modes (COOKIE + URL), so a session id supplied - * only via {@code ;jsessionid=} in the URL (no cookie at all) is honoured and kept alive - * with no fresh {@code Set-Cookie} -- the session-fixation vector. Control: the normal - * cookie-based flow must still carry a session across requests. - */ + /** A session id supplied only in the URL is not resumed; the cookie-based flow still is. */ @Test public void testUrlSuppliedSessionIdIsNotHonored() throws Exception { HttpResponse first = get(server.baseUrl() + "/sessionProbe.action"); assertThat(first.body()).contains("COUNTER=1"); String sessionId = extractSessionId(first); - // Attacker-style request: the session id travels only in the URL, never as a cookie. HttpResponse viaUrl = get(server.baseUrl() + "/sessionProbe.action;jsessionid=" + sessionId); - // GREEN: a brand-new session is created (counter resets to 1, a fresh id is issued) -- - // the URL-supplied id from the first request was not resumed. assertThat(viaUrl.body()).contains("COUNTER=1"); assertThat(extractSessionId(viaUrl)).isNotEqualTo(sessionId);