This is an automated email from the ASF dual-hosted git repository. lukaszlenart pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/struts.git
commit d3f444d8e53198e2c597a2b595b1ac1be243b6a4 Author: Lukasz Lenart <[email protected]> AuthorDate: Fri Oct 23 13:16:24 2026 +0200 WW-5791 fix(boot): harden embedded Tomcat defaults and normalise boot settings Error pages no longer carry stack traces or the Tomcat version, the connector is opened as an explicit step after the Struts filter started, struts.boot.address restricts the listening interface (all interfaces stay the default), and struts.boot.context-path is normalised. Co-Authored-By: Claude Opus 5.5 <[email protected]> --- .../java/org/apache/struts2/boot/BootSettings.java | 28 ++++++++- .../org/apache/struts2/boot/StrutsBootServer.java | 66 ++++++++++++++++------ .../org/apache/struts2/boot/BootSettingsTest.java | 43 ++++++++++++++ .../apache/struts2/boot/StrutsBootTomcatTest.java | 62 ++++++++++++++++++++ .../org/apache/struts2/boot/fixture/HelloApp.java | 1 + .../struts2/boot/fixture/ThrowingAction.java} | 21 +++---- boot/src/test/resources/struts.xml | 3 + 7 files changed, 191 insertions(+), 33 deletions(-) diff --git a/boot/src/main/java/org/apache/struts2/boot/BootSettings.java b/boot/src/main/java/org/apache/struts2/boot/BootSettings.java index 7632a4694..534389a79 100644 --- a/boot/src/main/java/org/apache/struts2/boot/BootSettings.java +++ b/boot/src/main/java/org/apache/struts2/boot/BootSettings.java @@ -22,6 +22,7 @@ import org.apache.struts2.config.ConfigurationException; import java.io.IOException; import java.io.InputStream; +import java.util.Optional; import java.util.Properties; public final class BootSettings { @@ -29,14 +30,17 @@ public final class BootSettings { private static final String PREFIX = "struts.boot."; private static final String PORT = PREFIX + "port"; private static final String CONTEXT_PATH = PREFIX + "context-path"; + private static final String ADDRESS = PREFIX + "address"; private static final String ARGUMENT_PREFIX = "--"; private final int port; private final String contextPath; + private final String address; - private BootSettings(int port, String contextPath) { + private BootSettings(int port, String contextPath, String address) { this.port = port; this.contextPath = contextPath; + this.address = address; } public static BootSettings load(String[] args) { @@ -51,7 +55,8 @@ public final class BootSettings { } return new BootSettings( parsePort(merged.getProperty(PORT, "8080")), - merged.getProperty(CONTEXT_PATH, "")); + normaliseContextPath(merged.getProperty(CONTEXT_PATH, "")), + blankToNull(merged.getProperty(ADDRESS))); } public int port() { @@ -62,6 +67,10 @@ public final class BootSettings { return contextPath; } + public Optional<String> address() { + return Optional.ofNullable(address); + } + private static void applyArgument(Properties target, String arg) { if (!arg.startsWith(ARGUMENT_PREFIX + PREFIX)) { return; @@ -73,6 +82,21 @@ public final class BootSettings { target.setProperty(arg.substring(ARGUMENT_PREFIX.length(), equals), arg.substring(equals + 1)); } + private static String normaliseContextPath(String value) { + String path = value.trim(); + while (path.endsWith("/")) { + path = path.substring(0, path.length() - 1); + } + if (path.isEmpty()) { + return ""; + } + return path.startsWith("/") ? path : "/" + path; + } + + private static String blankToNull(String value) { + return value == null || value.isBlank() ? null : value.trim(); + } + private static int parsePort(String value) { try { return Integer.parseInt(value.trim()); 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 4b42a731e..14763c81e 100644 --- a/boot/src/main/java/org/apache/struts2/boot/StrutsBootServer.java +++ b/boot/src/main/java/org/apache/struts2/boot/StrutsBootServer.java @@ -22,8 +22,10 @@ import jakarta.servlet.FilterConfig; import jakarta.servlet.ServletException; import org.apache.catalina.Context; import org.apache.catalina.LifecycleState; +import org.apache.catalina.connector.Connector; import org.apache.catalina.servlets.DefaultServlet; import org.apache.catalina.startup.Tomcat; +import org.apache.catalina.valves.ErrorReportValve; import org.apache.struts2.config.ConfigurationException; import org.apache.struts2.dispatcher.filter.StrutsPrepareAndExecuteFilter; import org.apache.tomcat.util.descriptor.web.FilterDef; @@ -45,24 +47,25 @@ public final class StrutsBootServer implements AutoCloseable { private final Tomcat tomcat; private final Path baseDir; - private final String contextPath; + private final BootSettings settings; private final AtomicBoolean closed = new AtomicBoolean(); private final Thread shutdownHook = new Thread(this::close, "struts-boot-shutdown"); + private Connector connector; - private StrutsBootServer(Tomcat tomcat, Path baseDir, String contextPath) { + private StrutsBootServer(Tomcat tomcat, Path baseDir, BootSettings settings) { this.tomcat = tomcat; this.baseDir = baseDir; - this.contextPath = contextPath; + this.settings = settings; } static StrutsBootServer start(ActionRegistry registry, BootSettings settings) { Path baseDir = createBaseDir(); Tomcat tomcat = new Tomcat(); - StrutsBootServer server = new StrutsBootServer(tomcat, baseDir, settings.contextPath()); + StrutsBootServer server = new StrutsBootServer(tomcat, baseDir, settings); CapturingStrutsFilter filter = new CapturingStrutsFilter(); try { tomcat.setBaseDir(baseDir.toString()); - tomcat.setPort(settings.port()); + hideErrorReportDetails(tomcat); Path docBase = Files.createDirectory(baseDir.resolve("webapp")); Context context = tomcat.addContext(settings.contextPath(), docBase.toString()); context.addServletContainerInitializer( @@ -73,7 +76,8 @@ public final class StrutsBootServer implements AutoCloseable { addStrutsFilter(context, filter); tomcat.start(); - server.assertStarted(context, filter); + assertContextStarted(context, filter); + server.openConnector(); } catch (Exception e) { ConfigurationException failure = e instanceof ConfigurationException configurationException ? configurationException @@ -89,11 +93,11 @@ public final class StrutsBootServer implements AutoCloseable { } public int port() { - return tomcat.getConnector().getLocalPort(); + return connector.getLocalPort(); } public String baseUrl() { - return "http://127.0.0.1:"; + port() + contextPath; + return "http://"; + urlHost() + ":" + port() + settings.contextPath(); } public Path baseDir() { @@ -128,29 +132,55 @@ public final class StrutsBootServer implements AutoCloseable { } } - private void assertStarted(Context context, CapturingStrutsFilter filter) { - Throwable initFailure = filter.failure.get(); - if (initFailure != null || context.getState() != LifecycleState.STARTED) { - String reason = initFailure == null ? "context state " + context.getState() : rootMessage(initFailure); - throw new ConfigurationException("Struts Boot failed to start: " + reason, initFailure); - } - if (tomcat.getConnector().getState() != LifecycleState.STARTED || port() < 0) { + private void openConnector() { + Connector created = new Connector(); + created.setPort(settings.port()); + settings.address().ifPresent(address -> { + if (!created.setProperty("address", address)) { + throw new ConfigurationException("Invalid struts.boot.address: '" + address + "'"); + } + }); + connector = created; + tomcat.setConnector(created); + if (created.getState() != LifecycleState.STARTED || created.getLocalPort() < 0) { throw new ConfigurationException("Struts Boot failed to start: connector state " - + tomcat.getConnector().getState() + " on port " + tomcat.getConnector().getPort()); + + created.getState() + " on port " + settings.port()); } } + private String urlHost() { + return settings.address() + .filter(address -> !address.equals("0.0.0.0") && !address.equals("::")) + .map(address -> address.contains(":") ? "[" + address + "]" : address) + .orElse("127.0.0.1"); + } + private void removeShutdownHook() { if (Thread.currentThread() == shutdownHook) { return; } try { Runtime.getRuntime().removeShutdownHook(shutdownHook); - } catch (IllegalStateException | IllegalArgumentException ignored) { - // JVM already shutting down, or the hook was never registered + } catch (IllegalStateException ignored) { + // the JVM is already shutting down } } + private static void assertContextStarted(Context context, CapturingStrutsFilter filter) { + Throwable initFailure = filter.failure.get(); + if (initFailure != null || context.getState() != LifecycleState.STARTED) { + String reason = initFailure == null ? "context state " + context.getState() : rootMessage(initFailure); + throw new ConfigurationException("Struts Boot failed to start: " + reason, initFailure); + } + } + + private static void hideErrorReportDetails(Tomcat tomcat) { + ErrorReportValve valve = new ErrorReportValve(); + valve.setShowReport(false); + valve.setShowServerInfo(false); + tomcat.getHost().getPipeline().addValve(valve); + } + private static void addStrutsFilter(Context context, CapturingStrutsFilter filter) { FilterDef filterDef = new FilterDef(); filterDef.setFilterName(FILTER_NAME); diff --git a/boot/src/test/java/org/apache/struts2/boot/BootSettingsTest.java b/boot/src/test/java/org/apache/struts2/boot/BootSettingsTest.java index f4fc3b7fb..5f7b836bc 100644 --- a/boot/src/test/java/org/apache/struts2/boot/BootSettingsTest.java +++ b/boot/src/test/java/org/apache/struts2/boot/BootSettingsTest.java @@ -69,6 +69,49 @@ public class BootSettingsTest { assertThat(settings.contextPath()).isEqualTo("/ctx"); } + @Test + public void testAddressDefaultsToAllInterfaces() { + assertThat(BootSettings.load(new Properties(), new String[0]).address()).isEmpty(); + } + + @Test + public void testAddressFromFile() { + Properties file = new Properties(); + file.setProperty("struts.boot.address", "127.0.0.1"); + + assertThat(BootSettings.load(file, new String[0]).address()).contains("127.0.0.1"); + } + + @Test + public void testAddressArgumentBeatsFile() { + Properties file = new Properties(); + file.setProperty("struts.boot.address", "0.0.0.0"); + + BootSettings settings = BootSettings.load(file, new String[]{"--struts.boot.address=127.0.0.1"}); + + assertThat(settings.address()).contains("127.0.0.1"); + } + + @Test + public void testBlankAddressMeansAllInterfaces() { + assertThat(BootSettings.load(new Properties(), new String[]{"--struts.boot.address= "}).address()).isEmpty(); + } + + @Test + public void testContextPathNormalised() { + assertThat(contextPath("ctx")).isEqualTo("/ctx"); + assertThat(contextPath("/")).isEmpty(); + assertThat(contextPath("")).isEmpty(); + assertThat(contextPath(" ")).isEmpty(); + assertThat(contextPath("/ctx/")).isEqualTo("/ctx"); + assertThat(contextPath(" /ctx ")).isEqualTo("/ctx"); + assertThat(contextPath("/a/b")).isEqualTo("/a/b"); + } + + private static String contextPath(String value) { + return BootSettings.load(new Properties(), new String[]{"--struts.boot.context-path=" + value}).contextPath(); + } + @Test public void testNonNumericPortThrows() { assertThatThrownBy(() -> BootSettings.load(new Properties(), new String[]{"--struts.boot.port=abc"})) 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 19ede906f..7a8397f25 100644 --- a/boot/src/test/java/org/apache/struts2/boot/StrutsBootTomcatTest.java +++ b/boot/src/test/java/org/apache/struts2/boot/StrutsBootTomcatTest.java @@ -27,19 +27,27 @@ import org.junit.BeforeClass; import org.junit.Test; import java.io.IOException; +import java.net.Inet4Address; +import java.net.InetAddress; +import java.net.InetSocketAddress; +import java.net.NetworkInterface; import java.net.ServerSocket; +import java.net.Socket; +import java.net.SocketException; import java.net.URI; import java.net.http.HttpClient; import java.net.http.HttpRequest; import java.net.http.HttpResponse; import java.nio.file.Files; import java.nio.file.Path; +import java.util.Optional; import java.util.Set; import java.util.stream.Collectors; import java.util.stream.Stream; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.junit.Assume.assumeTrue; public class StrutsBootTomcatTest { @@ -94,6 +102,42 @@ public class StrutsBootTomcatTest { assertThat(response.statusCode()).isNotEqualTo(200); } + @Test + public void testUnregisteredMethodNotInvokableViaMethodPrefix() throws Exception { + UserAction.deleteInvoked = false; + get(server.baseUrl() + "/xml/legacyUsers.action?method:delete="); + assertThat(UserAction.deleteInvoked).isTrue(); + + UserAction.deleteInvoked = false; + HttpResponse<String> response = get(server.baseUrl() + "/admin/users.action?method:delete="); + + assertThat(UserAction.deleteInvoked).isFalse(); + assertThat(response.statusCode()).isNotEqualTo(200); + } + + @Test + public void testErrorPageHidesStackTraceAndServerInfo() throws Exception { + HttpResponse<String> response = get(server.baseUrl() + "/boom.action"); + + assertThat(response.statusCode()).isEqualTo(500); + assertThat(response.body()).doesNotContain("at org.apache").doesNotContain("Apache Tomcat/"); + } + + @Test + public void testAddressRestrictsListeningInterface() throws Exception { + try (StrutsBootServer local = StrutsBoot.start(HelloApp.class, "--struts.boot.port=0", "--struts.boot.address=127.0.0.1")) { + assertThat(get("http://127.0.0.1:"; + local.port() + "/hello.action").statusCode()).isEqualTo(200); + + Optional<InetAddress> external = nonLoopbackIpv4Address(); + assumeTrue("no non-loopback IPv4 interface to probe", external.isPresent()); + assertThatThrownBy(() -> { + try (Socket socket = new Socket()) { + socket.connect(new InetSocketAddress(external.get(), local.port()), 2000); + } + }).isInstanceOf(IOException.class); + } + } + @Test public void testXmlAndRegistryCoexist() throws Exception { assertThat(get(server.baseUrl() + "/xml/legacy.action").statusCode()).isEqualTo(200); @@ -102,8 +146,10 @@ public class StrutsBootTomcatTest { @Test public void testBaseDirFilesAreNotServed() throws Exception { + Files.writeString(server.baseDir().resolve("webapp").resolve("doc-base-file.txt"), "public"); Files.writeString(server.baseDir().resolve("base-dir-file.txt"), "not public"); + assertThat(get(server.baseUrl() + "/doc-base-file.txt").statusCode()).isEqualTo(200); assertThat(get(server.baseUrl() + "/base-dir-file.txt").statusCode()).isNotEqualTo(200); } @@ -165,6 +211,22 @@ public class StrutsBootTomcatTest { return CLIENT.send(request, HttpResponse.BodyHandlers.ofString()); } + private static Optional<InetAddress> nonLoopbackIpv4Address() throws SocketException { + return NetworkInterface.networkInterfaces() + .filter(StrutsBootTomcatTest::isUp) + .flatMap(NetworkInterface::inetAddresses) + .filter(address -> address instanceof Inet4Address && !address.isLoopbackAddress()) + .findFirst(); + } + + private static boolean isUp(NetworkInterface networkInterface) { + try { + return networkInterface.isUp() && !networkInterface.isLoopback(); + } catch (SocketException e) { + return false; + } + } + private static Set<Path> bootTempDirs() throws IOException { try (Stream<Path> entries = Files.list(Path.of(System.getProperty("java.io.tmpdir")))) { return entries.filter(path -> path.getFileName().toString().startsWith("struts-boot-")).collect(Collectors.toSet()); 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 eaf21163a..541b5a6e8 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 @@ -28,6 +28,7 @@ public class HelloApp implements StrutsBootApplication { @Override public void actions(ActionRegistry actions) { actions.action("hello", HelloAction.class).success("hello.ftl"); + actions.action("boom", ThrowingAction.class).success("hello.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/resources/struts.xml b/boot/src/test/java/org/apache/struts2/boot/fixture/ThrowingAction.java similarity index 62% copy from boot/src/test/resources/struts.xml copy to boot/src/test/java/org/apache/struts2/boot/fixture/ThrowingAction.java index 925f15ddc..57e5d9cd8 100644 --- a/boot/src/test/resources/struts.xml +++ b/boot/src/test/java/org/apache/struts2/boot/fixture/ThrowingAction.java @@ -1,5 +1,3 @@ -<?xml version="1.0" encoding="UTF-8"?> -<!-- /* * Licensed to the Apache Software Foundation (ASF) under one * or more contributor license agreements. See the NOTICE file @@ -18,14 +16,11 @@ * specific language governing permissions and limitations * under the License. */ ---> -<!DOCTYPE struts PUBLIC - "-//Apache Software Foundation//DTD Struts Configuration 6.0//EN" - "https://struts.apache.org/dtds/struts-6.0.dtd";> -<struts> - <package name="xml-pkg" namespace="/xml" extends="struts-default"> - <action name="legacy" class="org.apache.struts2.boot.fixture.HelloAction"> - <result type="freemarker">/templates/hello.ftl</result> - </action> - </package> -</struts> +package org.apache.struts2.boot.fixture; + +public class ThrowingAction { + + public String execute() { + throw new IllegalStateException("boom"); + } +} diff --git a/boot/src/test/resources/struts.xml b/boot/src/test/resources/struts.xml index 925f15ddc..4a759d7bd 100644 --- a/boot/src/test/resources/struts.xml +++ b/boot/src/test/resources/struts.xml @@ -27,5 +27,8 @@ <action name="legacy" class="org.apache.struts2.boot.fixture.HelloAction"> <result type="freemarker">/templates/hello.ftl</result> </action> + <action name="legacyUsers" class="org.apache.struts2.boot.fixture.UserAction"> + <result type="freemarker">/templates/hello.ftl</result> + </action> </package> </struts>
