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 d73b21ed70..46935e34b3 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,6 +74,7 @@ static StrutsBootServer start(ActionRegistry registry, BootSettings settings) { hideErrorReportDetails(tomcat); Path docBase = Files.createDirectory(baseDir.resolve("webapp")); Context context = tomcat.addContext(settings.contextPath(), docBase.toString()); + restrictSessionTracking(context); context.addServletContainerInitializer( (classes, servletContext) -> servletContext.setAttribute(ActionRegistry.SERVLET_CONTEXT_ATTRIBUTE, registry), null); @@ -177,6 +181,20 @@ private static void assertContextStarted(Context context, CapturingStrutsFilter } } + /** + * 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 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); + } + 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..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,6 +125,41 @@ public void testErrorPageHidesStackTraceAndServerInfo() throws Exception { assertThat(response.body()).doesNotContain("at org.apache").doesNotContain("Apache Tomcat/"); } + /** The session cookie carries {@code SameSite=Lax}. */ + @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")); + } + + /** 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); + + HttpResponse viaUrl = get(server.baseUrl() + "/sessionProbe.action;jsessionid=" + sessionId); + 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"/>