From 9b1fac7bfd397f4b814edfc331f508fad286f7f0 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Fri, 16 Oct 2026 16:34:24 +0200 Subject: [PATCH 1/4] WW-3717 feat(core): restrict an action to HTTP methods with an http-method attribute Adds an optional http-method attribute to in the 7.5 DTD, enforced by HttpMethodInterceptor together with the existing @HttpGet/@HttpPost/@AllowedHttpMethod annotations; when both apply the request must satisfy both. Wildcard and regex actions keep the restriction, and an invalid method name fails configuration loading. A rejected request still gets the bad-request result when the action maps one; otherwise the interceptor now answers 405 with an Allow header instead of failing with "No result defined". A warning is logged for an action that declares http-method but whose interceptors do not include httpMethod. Co-Authored-By: Claude Opus 5.5 --- .../struts2/config/entities/ActionConfig.java | 31 ++++ .../config/impl/ActionConfigMatcher.java | 1 + .../config/impl/DefaultConfiguration.java | 10 ++ .../XmlDocConfigurationProvider.java | 21 +++ .../httpmethod/HttpMethodInterceptor.java | 63 ++++++- core/src/main/resources/struts-7.5.dtd | 1 + ...mlConfigurationProviderHttpMethodTest.java | 87 ++++++++++ .../httpmethod/HttpMethodInterceptorTest.java | 155 +++++++++++++++++- .../providers/struts-http-method-invalid.xml | 29 ++++ .../config/providers/struts-http-method.xml | 74 +++++++++ 10 files changed, 465 insertions(+), 7 deletions(-) create mode 100644 core/src/test/java/org/apache/struts2/config/providers/XmlConfigurationProviderHttpMethodTest.java create mode 100644 core/src/test/resources/org/apache/struts2/config/providers/struts-http-method-invalid.xml create mode 100644 core/src/test/resources/org/apache/struts2/config/providers/struts-http-method.xml diff --git a/core/src/main/java/org/apache/struts2/config/entities/ActionConfig.java b/core/src/main/java/org/apache/struts2/config/entities/ActionConfig.java index 53599638ab..47039fc572 100644 --- a/core/src/main/java/org/apache/struts2/config/entities/ActionConfig.java +++ b/core/src/main/java/org/apache/struts2/config/entities/ActionConfig.java @@ -18,6 +18,7 @@ */ package org.apache.struts2.config.entities; +import org.apache.struts2.interceptor.httpmethod.HttpMethod; import org.apache.struts2.util.location.Located; import org.apache.struts2.util.location.Location; import org.apache.commons.lang3.StringUtils; @@ -26,6 +27,7 @@ import java.util.ArrayList; import java.util.Collection; import java.util.Collections; +import java.util.EnumSet; import java.util.HashSet; import java.util.LinkedHashMap; import java.util.List; @@ -66,6 +68,7 @@ public class ActionConfig extends Located implements Serializable { protected String name; protected boolean strictMethodInvocation = true; protected AllowedMethods allowedMethods; + protected Set httpMethods; protected ActionConfig(String packageName, String name, String className) { this.packageName = packageName; @@ -75,6 +78,7 @@ protected ActionConfig(String packageName, String name, String className) { results = new LinkedHashMap<>(); interceptors = new ArrayList<>(); exceptionMappings = new ArrayList<>(); + httpMethods = EnumSet.noneOf(HttpMethod.class); } /** @@ -92,6 +96,8 @@ protected ActionConfig(ActionConfig orig) { this.results = new LinkedHashMap<>(orig.results); this.exceptionMappings = new ArrayList<>(orig.exceptionMappings); this.allowedMethods = orig.allowedMethods; + this.httpMethods = EnumSet.noneOf(HttpMethod.class); + this.httpMethods.addAll(orig.httpMethods); this.location = orig.location; this.strictMethodInvocation = orig.strictMethodInvocation; } @@ -116,6 +122,14 @@ public Set getAllowedMethods() { return allowedMethods.list(); } + /** + * @return HTTP methods this action accepts, empty when it accepts any + * @since 7.5.0 + */ + public Set getHttpMethods() { + return httpMethods; + } + /** * Returns name of the action method * @@ -186,6 +200,10 @@ public boolean equals(Object o) { return false; } + if (!Objects.equals(httpMethods, actionConfig.httpMethods)) { + return false; + } + return true; } @@ -201,6 +219,7 @@ public int hashCode() { result = 31 * result + (packageName != null ? packageName.hashCode() : 0); result = 31 * result + (name != null ? name.hashCode() : 0); result = 31 * result + (allowedMethods != null ? allowedMethods.hashCode() : 0); + result = 31 * result + (httpMethods != null ? httpMethods.hashCode() : 0); return result; } @@ -216,6 +235,9 @@ public String toString() { sb.append(")"); sb.append(" - ").append(location); sb.append(" - ").append(allowedMethods); + if (!httpMethods.isEmpty()) { + sb.append(" - ").append(httpMethods); + } sb.append("}"); return sb.toString(); } @@ -342,6 +364,14 @@ public Builder addAllowedMethod(Collection methods) { return this; } + /** + * @since 7.5.0 + */ + public Builder addHttpMethods(Collection methods) { + target.httpMethods.addAll(methods); + return this; + } + public Builder location(Location loc) { target.location = loc; return this; @@ -362,6 +392,7 @@ public ActionConfig build() { target.results = Collections.unmodifiableMap(target.results); target.interceptors = Collections.unmodifiableList(target.interceptors); target.exceptionMappings = Collections.unmodifiableList(target.exceptionMappings); + target.httpMethods = Collections.unmodifiableSet(target.httpMethods); target.allowedMethods = AllowedMethods.build(target.strictMethodInvocation, allowedMethods, methodRegex != null ? methodRegex : DEFAULT_METHOD_REGEX); ActionConfig result = target; diff --git a/core/src/main/java/org/apache/struts2/config/impl/ActionConfigMatcher.java b/core/src/main/java/org/apache/struts2/config/impl/ActionConfigMatcher.java index 0cd42f4f14..cdd7e7fa2c 100644 --- a/core/src/main/java/org/apache/struts2/config/impl/ActionConfigMatcher.java +++ b/core/src/main/java/org/apache/struts2/config/impl/ActionConfigMatcher.java @@ -145,6 +145,7 @@ public ActionConfig convert(String path, ActionConfig orig, .addResultConfigs(results) .setStrictMethodInvocation(orig.isStrictMethodInvocation()) .addAllowedMethod(orig.getAllowedMethods()) + .addHttpMethods(orig.getHttpMethods()) .addInterceptors(orig.getInterceptors()) .addExceptionMappings(exs) .location(orig.getLocation()) diff --git a/core/src/main/java/org/apache/struts2/config/impl/DefaultConfiguration.java b/core/src/main/java/org/apache/struts2/config/impl/DefaultConfiguration.java index 4f908bfb63..7d940275f7 100644 --- a/core/src/main/java/org/apache/struts2/config/impl/DefaultConfiguration.java +++ b/core/src/main/java/org/apache/struts2/config/impl/DefaultConfiguration.java @@ -93,6 +93,7 @@ import org.apache.struts2.ognl.accessor.CompoundRootAccessor; import org.apache.struts2.ognl.accessor.RootAccessor; import org.apache.struts2.ognl.accessor.XWorkMethodAccessor; +import org.apache.struts2.interceptor.httpmethod.HttpMethodInterceptor; import org.apache.struts2.interceptor.parameter.OgnlParameterAllowlister; import org.apache.struts2.interceptor.parameter.ParameterAllowlister; import org.apache.struts2.interceptor.parameter.StrutsParameterAuthorizer; @@ -559,6 +560,8 @@ private ActionConfig buildFullActionConfig(PackageConfig packageContext, ActionC } } + warnIfHttpMethodNotEnforced(baseConfig, interceptors); + String methodRegex = container.getInstance(String.class, StrutsConstants.STRUTS_SMI_METHOD_REGEX); if (methodRegex == null) { methodRegex = ActionConfig.DEFAULT_METHOD_REGEX; @@ -577,6 +580,13 @@ private ActionConfig buildFullActionConfig(PackageConfig packageContext, ActionC .build(); } + private void warnIfHttpMethodNotEnforced(ActionConfig baseConfig, List interceptors) { + if (!baseConfig.getHttpMethods().isEmpty() + && interceptors.stream().noneMatch(mapping -> mapping.getInterceptor() instanceof HttpMethodInterceptor)) { + LOG.warn("Action [{}] in package [{}] declares http-method {} but its interceptors do not include {}, the restriction will not be enforced", + baseConfig.getName(), baseConfig.getPackageName(), baseConfig.getHttpMethods(), HttpMethodInterceptor.class.getSimpleName()); + } + } private static class RuntimeConfigurationImpl implements RuntimeConfiguration { diff --git a/core/src/main/java/org/apache/struts2/config/providers/XmlDocConfigurationProvider.java b/core/src/main/java/org/apache/struts2/config/providers/XmlDocConfigurationProvider.java index a5547d9350..e7e7d40663 100644 --- a/core/src/main/java/org/apache/struts2/config/providers/XmlDocConfigurationProvider.java +++ b/core/src/main/java/org/apache/struts2/config/providers/XmlDocConfigurationProvider.java @@ -39,6 +39,7 @@ import org.apache.struts2.inject.ContainerBuilder; import org.apache.struts2.inject.Inject; import org.apache.struts2.inject.Scope; +import org.apache.struts2.interceptor.httpmethod.HttpMethod; import org.apache.struts2.util.ClassLoaderUtil; import org.apache.struts2.util.DomHelper; import org.apache.struts2.util.location.LocatableProperties; @@ -58,10 +59,12 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; +import java.util.EnumSet; import java.util.HashMap; import java.util.HashSet; import java.util.LinkedHashMap; import java.util.List; +import java.util.Locale; import java.util.Map; import java.util.Set; import java.util.function.Consumer; @@ -499,6 +502,7 @@ protected ActionConfig buildActionConfig(Element actionElement, List interceptorList = buildInterceptorList(actionElement, packageContext); List exceptionMappings = buildExceptionMappings(actionElement); Set allowedMethods = buildAllowedMethods(actionElement, packageContext); + Set httpMethods = buildHttpMethods(actionElement, location); return new ActionConfig.Builder(packageContext.getName(), actionName, className) .methodName(methodName) @@ -508,10 +512,27 @@ protected ActionConfig buildActionConfig(Element actionElement, .addParams(XmlHelper.getParams(actionElement)) .setStrictMethodInvocation(packageContext.isStrictMethodInvocation()) .addAllowedMethod(allowedMethods) + .addHttpMethods(httpMethods) .location(location) .build(); } + /** + * @since 7.5.0 + */ + protected Set buildHttpMethods(Element actionElement, Location location) { + Set httpMethods = EnumSet.noneOf(HttpMethod.class); + for (String value : commaDelimitedStringToSet(actionElement.getAttribute("http-method"))) { + try { + httpMethods.add(HttpMethod.valueOf(value.toUpperCase(Locale.ROOT))); + } catch (IllegalArgumentException e) { + throw new ConfigurationException(format("Invalid http-method [%s] on action [%s], allowed values: %s", + value, actionElement.getAttribute("name"), Arrays.toString(HttpMethod.values())), e, location); + } + } + return httpMethods; + } + protected void verifyAction(String className, Location loc) { if (className.contains("{")) { LOG.debug("Action class [{}] contains a wildcard replacement value, so it can't be verified", className); diff --git a/core/src/main/java/org/apache/struts2/interceptor/httpmethod/HttpMethodInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/httpmethod/HttpMethodInterceptor.java index 8e732f3260..0a006e585d 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/httpmethod/HttpMethodInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/httpmethod/HttpMethodInterceptor.java @@ -19,19 +19,25 @@ package org.apache.struts2.interceptor.httpmethod; import org.apache.struts2.ActionInvocation; +import org.apache.struts2.action.Action; import org.apache.struts2.interceptor.AbstractInterceptor; import org.apache.struts2.util.AnnotationUtils; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; import java.lang.annotation.Annotation; import java.lang.reflect.AnnotatedElement; import java.lang.reflect.Method; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collection; import java.util.Collections; +import java.util.EnumSet; import java.util.List; +import java.util.Set; +import java.util.stream.Collectors; /** * Interceptor is used to control with what http methods action can be called, @@ -45,6 +51,12 @@ * {@link HttpPut}, {@link HttpDelete} and {@link HttpGetOrPost} *

* You can combine any of these annotations to achieve required allowed methods' filtering. + *

+ * The allowed methods can also be declared on the action in XML with the {@code http-method} attribute. + * When both the attribute and annotations are present, the request method must satisfy both. + *

+ * If the action has no result mapped under the bad request result name, the response is + * {@code 405 Method Not Allowed} with an {@code Allow} header listing the accepted methods. * * @see HttpMethodAware * @see HttpMethod @@ -97,23 +109,64 @@ public String intercept(ActionInvocation invocation) throws Exception { return doIntercept(invocation, action.getClass()); } + Set configuredMethods = invocation.getProxy().getConfig().getHttpMethods(); + if (!configuredMethods.isEmpty()) { + LOG.debug("Action: {} declares http-method: {}, checking if request: {} meets allowed methods!", + action, configuredMethods, request.getMethod()); + return checkAllowedMethods(invocation, configuredMethods); + } + return invocation.invoke(); } protected String doIntercept(ActionInvocation invocation, AnnotatedElement element) throws Exception { - List allowedMethods = readAllowedMethods(element); + Set allowedMethods = EnumSet.noneOf(HttpMethod.class); + allowedMethods.addAll(readAllowedMethods(element)); + + Set configuredMethods = invocation.getProxy().getConfig().getHttpMethods(); + if (!configuredMethods.isEmpty()) { + allowedMethods.retainAll(configuredMethods); + } + + return checkAllowedMethods(invocation, allowedMethods); + } + + private String checkAllowedMethods(ActionInvocation invocation, Set allowedMethods) throws Exception { HttpServletRequest request = invocation.getInvocationContext().getServletRequest(); - HttpMethod requestedMethod = HttpMethod.parse(request.getMethod()); + String requestedMethod = request.getMethod(); - if (allowedMethods.contains(requestedMethod)) { + if (isAllowed(requestedMethod, allowedMethods)) { LOG.trace("Request method: {} matches allowed methods: {}, continuing invocation!", requestedMethod, allowedMethods); return invocation.invoke(); } else { - LOG.trace("Request method: {} doesn't match allowed methods: {}, continuing invocation!", requestedMethod, allowedMethods); - return getBadRequestResultName(invocation); + LOG.trace("Request method: {} doesn't match allowed methods: {}, rejecting request!", requestedMethod, allowedMethods); + return rejectRequest(invocation, allowedMethods); } } + private boolean isAllowed(String requestedMethod, Set allowedMethods) { + return allowedMethods.stream().anyMatch(method -> method.name().equalsIgnoreCase(requestedMethod)); + } + + /** + * @return the bad request result name when the action maps that result, otherwise {@link Action#NONE} + * after sending {@code 405 Method Not Allowed} with an {@code Allow} header + * @since 7.5.0 + */ + protected String rejectRequest(ActionInvocation invocation, Collection allowedMethods) throws Exception { + String resultName = getBadRequestResultName(invocation); + if (invocation.getProxy().getConfig().getResults().containsKey(resultName)) { + return resultName; + } + + LOG.debug("No result mapped for: {}, sending: {} with allowed methods: {}", + resultName, HttpServletResponse.SC_METHOD_NOT_ALLOWED, allowedMethods); + HttpServletResponse response = invocation.getInvocationContext().getServletResponse(); + response.setHeader("Allow", allowedMethods.stream().map(HttpMethod::name).collect(Collectors.joining(", "))); + response.sendError(HttpServletResponse.SC_METHOD_NOT_ALLOWED); + return Action.NONE; + } + protected List readAllowedMethods(AnnotatedElement element) { List allowedMethods = new ArrayList<>(); if (AnnotationUtils.isAnnotatedBy(element, AllowedHttpMethod.class)) { diff --git a/core/src/main/resources/struts-7.5.dtd b/core/src/main/resources/struts-7.5.dtd index 307f5f0e0a..b534fd5c84 100644 --- a/core/src/main/resources/struts-7.5.dtd +++ b/core/src/main/resources/struts-7.5.dtd @@ -105,6 +105,7 @@ class CDATA #IMPLIED method CDATA #IMPLIED converter CDATA #IMPLIED + http-method CDATA #IMPLIED > diff --git a/core/src/test/java/org/apache/struts2/config/providers/XmlConfigurationProviderHttpMethodTest.java b/core/src/test/java/org/apache/struts2/config/providers/XmlConfigurationProviderHttpMethodTest.java new file mode 100644 index 0000000000..5ceceea250 --- /dev/null +++ b/core/src/test/java/org/apache/struts2/config/providers/XmlConfigurationProviderHttpMethodTest.java @@ -0,0 +1,87 @@ +/* + * 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.config.providers; + +import org.apache.struts2.StrutsInternalTestCase; +import org.apache.struts2.config.ConfigurationException; +import org.apache.struts2.config.StrutsXmlConfigurationProvider; +import org.apache.struts2.config.entities.ActionConfig; +import org.apache.struts2.config.impl.DefaultConfiguration; +import org.apache.struts2.test.LogCapture; + +import static org.apache.struts2.interceptor.httpmethod.HttpMethod.GET; +import static org.apache.struts2.interceptor.httpmethod.HttpMethod.POST; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +public class XmlConfigurationProviderHttpMethodTest extends StrutsInternalTestCase { + + private static final String CONFIG = "org/apache/struts2/config/providers/struts-http-method.xml"; + private static final String INVALID_CONFIG = "org/apache/struts2/config/providers/struts-http-method-invalid.xml"; + + public void testSingleHttpMethod() { + loadConfigurationProviders(new StrutsXmlConfigurationProvider(CONFIG)); + + assertThat(actionConfig("", "PostOnly").getHttpMethods()).containsExactly(POST); + } + + public void testHttpMethodListIsTrimmedAndCaseInsensitive() { + loadConfigurationProviders(new StrutsXmlConfigurationProvider(CONFIG)); + + assertThat(actionConfig("", "GetOrPost").getHttpMethods()).containsExactlyInAnyOrder(GET, POST); + } + + public void testNoHttpMethodMeansUnrestricted() { + loadConfigurationProviders(new StrutsXmlConfigurationProvider(CONFIG)); + + assertThat(actionConfig("", "Unrestricted").getHttpMethods()).isEmpty(); + } + + public void testWildcardActionKeepsHttpMethod() { + loadConfigurationProviders(new StrutsXmlConfigurationProvider(CONFIG)); + + ActionConfig resolved = actionConfig("", "PostWild-execute"); + + assertThat(resolved.getName()).isEqualTo("PostWild-*"); + assertThat(resolved.getMethodName()).isEqualTo("execute"); + assertThat(resolved.getHttpMethods()).containsExactly(POST); + } + + public void testInvalidHttpMethodFailsConfiguration() { + assertThatThrownBy(() -> loadConfigurationProviders(new StrutsXmlConfigurationProvider(INVALID_CONFIG))) + .isInstanceOf(ConfigurationException.class) + .hasStackTraceContaining("Invalid http-method [fetch] on action [Fetch]"); + } + + public void testWarnsWhenStackHasNoHttpMethodInterceptor() { + try (LogCapture log = new LogCapture(DefaultConfiguration.class)) { + loadConfigurationProviders(new StrutsXmlConfigurationProvider(CONFIG)); + + assertThat(log.messages()) + .anyMatch(message -> message.contains("[PostOnlyWithoutInterceptor]") && message.contains("http-method")) + .anyMatch(message -> message.contains("[PostOnlyWithoutStack]") && message.contains("http-method")) + .noneMatch(message -> message.contains("[PostOnly]")) + .noneMatch(message -> message.contains("[Unrestricted]")); + } + } + + private ActionConfig actionConfig(String namespace, String name) { + return configuration.getRuntimeConfiguration().getActionConfig(namespace, name); + } +} diff --git a/core/src/test/java/org/apache/struts2/interceptor/httpmethod/HttpMethodInterceptorTest.java b/core/src/test/java/org/apache/struts2/interceptor/httpmethod/HttpMethodInterceptorTest.java index 0403152ba3..ab7b7bb234 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/httpmethod/HttpMethodInterceptorTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/httpmethod/HttpMethodInterceptorTest.java @@ -18,16 +18,22 @@ */ package org.apache.struts2.interceptor.httpmethod; +import jakarta.servlet.http.HttpServletResponse; +import org.apache.struts2.action.Action; import org.apache.struts2.ActionContext; import org.apache.struts2.ActionProxy; import org.apache.struts2.config.StrutsXmlConfigurationProvider; +import org.apache.struts2.config.entities.ActionConfig; +import org.apache.struts2.config.entities.ResultConfig; import org.apache.struts2.mock.MockActionInvocation; import org.apache.struts2.mock.MockActionProxy; import org.apache.struts2.HttpMethodsTestAction; import org.apache.struts2.StrutsInternalTestCase; import org.apache.struts2.TestAction; import org.springframework.mock.web.MockHttpServletRequest; +import org.springframework.mock.web.MockHttpServletResponse; +import java.util.List; import java.util.Map; public class HttpMethodInterceptorTest extends StrutsInternalTestCase { @@ -270,13 +276,26 @@ public void testWildcardResolvedMethodWithPostAnnotationAllowsPost() throws Exce } private void prepareActionInvocation(Object action) { + prepareActionInvocation(action, actionConfig("bad-request", "custom-bad-request", "action-bad-request")); + } + + private void prepareActionInvocation(Object action, ActionConfig config) { interceptor = new HttpMethodInterceptor(); invocation = new MockActionInvocation(); invocation.setAction(action); actionProxy = new MockActionProxy(); + actionProxy.setConfig(config); invocation.setProxy(actionProxy); } + private ActionConfig actionConfig(String... resultNames) { + ActionConfig.Builder builder = new ActionConfig.Builder("package", "action", TestAction.class.getName()); + for (String resultName : resultNames) { + builder.addResultConfig(new ResultConfig.Builder(resultName, "result").build()); + } + return builder.build(); + } + /** * Regression: wildcard-resolved method with NO method-level annotation on a class @@ -336,8 +355,10 @@ public void testWildcardResolvedExecuteRejectsGetThroughRealProxy() throws Excep "org/apache/struts2/config/providers/xwork-test-allowed-methods.xml")); MockHttpServletRequest request = new MockHttpServletRequest("GET", "/Wild-execute"); + MockHttpServletResponse response = new MockHttpServletResponse(); Map extraContext = ActionContext.of() .withServletRequest(request) + .withServletResponse(response) .getContextMap(); ActionProxy proxy = actionProxyFactory.createActionProxy("", "Wild-execute", null, extraContext); @@ -351,13 +372,143 @@ public void testWildcardResolvedExecuteRejectsGetThroughRealProxy() throws Excep // class-level @AllowedHttpMethod(POST) must still be enforced even though the resolved // method carries no method-level annotation — this is what #1690 fixed + assertEquals(Action.NONE, result); + assertEquals(HttpServletResponse.SC_METHOD_NOT_ALLOWED, response.getStatus()); + assertEquals("POST", response.getHeader("Allow")); + } + + public void testAnnotationWithoutMappedResultSends405() throws Exception { + prepareActionInvocation(new HttpMethodsTestAction(), actionConfig()); + MockHttpServletResponse response = prepareRequest("get"); + + String resultName = interceptor.intercept(invocation); + + assertEquals(Action.NONE, resultName); + assertEquals(HttpServletResponse.SC_METHOD_NOT_ALLOWED, response.getStatus()); + assertEquals("POST", response.getHeader("Allow")); + } + + public void testConfigAndAnnotationMustBothAllowTheMethod() throws Exception { + ActionConfig config = new ActionConfig.Builder(actionConfig("bad-request")) + .addHttpMethods(List.of(HttpMethod.GET)) + .build(); + prepareActionInvocation(new HttpMethodsTestAction(), config); + actionProxy.setMethod("onGetPostOnly"); + actionProxy.setMethodSpecified(true); + invocation.setResultCode("onGetPostOnly"); + + prepareRequest("get"); + assertEquals("onGetPostOnly", interceptor.intercept(invocation)); + + prepareRequest("post"); + assertEquals("bad-request", interceptor.intercept(invocation)); + } + + public void testConfigRestrictsMethodWithoutAnnotation() throws Exception { + ActionConfig config = new ActionConfig.Builder(actionConfig("bad-request")) + .addHttpMethods(List.of(HttpMethod.GET)) + .build(); + prepareActionInvocation(new TestAction(), config); + invocation.setResultCode("success"); + + prepareRequest("get"); + assertEquals("success", interceptor.intercept(invocation)); + + prepareRequest("post"); + assertEquals("bad-request", interceptor.intercept(invocation)); + } + + public void testConfigRejectsUnknownRequestMethod() throws Exception { + ActionConfig config = new ActionConfig.Builder(actionConfig()) + .addHttpMethods(List.of(HttpMethod.GET)) + .build(); + prepareActionInvocation(new TestAction(), config); + MockHttpServletResponse response = prepareRequest("PROPFIND"); + + String resultName = interceptor.intercept(invocation); + + assertEquals(Action.NONE, resultName); + assertEquals(HttpServletResponse.SC_METHOD_NOT_ALLOWED, response.getStatus()); + assertEquals("GET", response.getHeader("Allow")); + } + + public void testHttpMethodAttributeRejectsWith405AndAllowHeader() throws Exception { + MockHttpServletResponse response = new MockHttpServletResponse(); + ActionProxy proxy = proxyFor("", "PostOnly", "GET", response); + + String result = new HttpMethodInterceptor().intercept(proxy.getInvocation()); + + assertEquals(Action.NONE, result); + assertEquals(HttpServletResponse.SC_METHOD_NOT_ALLOWED, response.getStatus()); + assertEquals("POST", response.getHeader("Allow")); + } + + public void testHttpMethodAttributeAllowsListedMethod() throws Exception { + ActionProxy proxy = proxyFor("", "PostOnly", "POST", new MockHttpServletResponse()); + + String result = new HttpMethodInterceptor().intercept(proxy.getInvocation()); + + assertEquals(Action.SUCCESS, result); + } + + public void testHttpMethodAttributeListsAllMethodsInAllowHeader() throws Exception { + MockHttpServletResponse response = new MockHttpServletResponse(); + ActionProxy proxy = proxyFor("", "GetOrPost", "PUT", response); + + String result = new HttpMethodInterceptor().intercept(proxy.getInvocation()); + + assertEquals(Action.NONE, result); + assertEquals("GET, POST", response.getHeader("Allow")); + } + + public void testHttpMethodAttributeUsesMappedActionResult() throws Exception { + MockHttpServletResponse response = new MockHttpServletResponse(); + ActionProxy proxy = proxyFor("", "PostOnlyWithResult", "GET", response); + + String result = new HttpMethodInterceptor().intercept(proxy.getInvocation()); + assertEquals("bad-request", result); + assertEquals(HttpServletResponse.SC_OK, response.getStatus()); + } + + public void testHttpMethodAttributeUsesMappedGlobalResult() throws Exception { + MockHttpServletResponse response = new MockHttpServletResponse(); + ActionProxy proxy = proxyFor("/global", "PostOnly", "GET", response); + + String result = new HttpMethodInterceptor().intercept(proxy.getInvocation()); + + assertEquals("bad-request", result); + assertEquals(HttpServletResponse.SC_OK, response.getStatus()); + } + + public void testHttpMethodAttributeOnWildcardAction() throws Exception { + MockHttpServletResponse response = new MockHttpServletResponse(); + ActionProxy proxy = proxyFor("", "PostWild-execute", "GET", response); + + String result = new HttpMethodInterceptor().intercept(proxy.getInvocation()); + + assertEquals(Action.NONE, result); + assertEquals(HttpServletResponse.SC_METHOD_NOT_ALLOWED, response.getStatus()); + } + + private ActionProxy proxyFor(String namespace, String actionName, String httpMethod, MockHttpServletResponse response) { + loadConfigurationProviders(new StrutsXmlConfigurationProvider( + "org/apache/struts2/config/providers/struts-http-method.xml")); + + Map extraContext = ActionContext.of() + .withServletRequest(new MockHttpServletRequest(httpMethod, "/" + actionName)) + .withServletResponse(response) + .getContextMap(); + + return actionProxyFactory.createActionProxy(namespace, actionName, null, extraContext, false, true); } - private void prepareRequest(String httpMethod) { + private MockHttpServletResponse prepareRequest(String httpMethod) { MockHttpServletRequest request = new MockHttpServletRequest(httpMethod, "/action"); - ActionContext.getContext().withServletRequest(request); + MockHttpServletResponse response = new MockHttpServletResponse(); + ActionContext.getContext().withServletRequest(request).withServletResponse(response); invocation.setInvocationContext(ActionContext.getContext()); + return response; } } diff --git a/core/src/test/resources/org/apache/struts2/config/providers/struts-http-method-invalid.xml b/core/src/test/resources/org/apache/struts2/config/providers/struts-http-method-invalid.xml new file mode 100644 index 0000000000..0b599ac3d9 --- /dev/null +++ b/core/src/test/resources/org/apache/struts2/config/providers/struts-http-method-invalid.xml @@ -0,0 +1,29 @@ + + + + + + + + diff --git a/core/src/test/resources/org/apache/struts2/config/providers/struts-http-method.xml b/core/src/test/resources/org/apache/struts2/config/providers/struts-http-method.xml new file mode 100644 index 0000000000..c96342e1d4 --- /dev/null +++ b/core/src/test/resources/org/apache/struts2/config/providers/struts-http-method.xml @@ -0,0 +1,74 @@ + + + + + + + + + + + + + + + + + + + + + + + + + /bad-request.jsp + + + + + regex:.* + + + + + + + + + + + + + + + + /bad-request.jsp + + + + + + + From c3155ce9b714bd9ca27f84e202bdb5f8f8f4ecba Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Fri, 16 Oct 2026 16:39:52 +0200 Subject: [PATCH 2/4] WW-3717 fix(core): resolve the bad-request result like the action invocation does Before answering 405 the interceptor now also accepts a catch-all "*" result and a result supplied by an unknown handler (e.g. the Convention plugin's {action}-bad-request page), so existing annotation users keep their page. Adds a test where the annotation narrows the configured methods. Co-Authored-By: Claude Opus 5.5 --- .../httpmethod/HttpMethodInterceptor.java | 24 ++++++- .../httpmethod/HttpMethodInterceptorTest.java | 67 ++++++++++++++++--- .../config/providers/struts-http-method.xml | 5 ++ 3 files changed, 86 insertions(+), 10 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/interceptor/httpmethod/HttpMethodInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/httpmethod/HttpMethodInterceptor.java index 0a006e585d..3247f2a52a 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/httpmethod/HttpMethodInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/httpmethod/HttpMethodInterceptor.java @@ -19,7 +19,10 @@ package org.apache.struts2.interceptor.httpmethod; import org.apache.struts2.ActionInvocation; +import org.apache.struts2.UnknownHandlerManager; import org.apache.struts2.action.Action; +import org.apache.struts2.config.entities.ActionConfig; +import org.apache.struts2.inject.Inject; import org.apache.struts2.interceptor.AbstractInterceptor; import org.apache.struts2.util.AnnotationUtils; import org.apache.logging.log4j.LogManager; @@ -55,7 +58,7 @@ * The allowed methods can also be declared on the action in XML with the {@code http-method} attribute. * When both the attribute and annotations are present, the request method must satisfy both. *

- * If the action has no result mapped under the bad request result name, the response is + * If no result can be resolved under the bad request result name, the response is * {@code 405 Method Not Allowed} with an {@code Allow} header listing the accepted methods. * * @see HttpMethodAware @@ -84,6 +87,13 @@ public class HttpMethodInterceptor extends AbstractInterceptor { private String badRequestResultName = "bad-request"; + private UnknownHandlerManager unknownHandlerManager; + + @Inject + public void setUnknownHandlerManager(UnknownHandlerManager unknownHandlerManager) { + this.unknownHandlerManager = unknownHandlerManager; + } + @Override public String intercept(ActionInvocation invocation) throws Exception { Object action = invocation.getAction(); @@ -155,7 +165,7 @@ private boolean isAllowed(String requestedMethod, Set allowedMethods */ protected String rejectRequest(ActionInvocation invocation, Collection allowedMethods) throws Exception { String resultName = getBadRequestResultName(invocation); - if (invocation.getProxy().getConfig().getResults().containsKey(resultName)) { + if (canResolveResult(invocation, resultName)) { return resultName; } @@ -167,6 +177,16 @@ protected String rejectRequest(ActionInvocation invocation, Collection readAllowedMethods(AnnotatedElement element) { List allowedMethods = new ArrayList<>(); if (AnnotationUtils.isAnnotatedBy(element, AllowedHttpMethod.class)) { diff --git a/core/src/test/java/org/apache/struts2/interceptor/httpmethod/HttpMethodInterceptorTest.java b/core/src/test/java/org/apache/struts2/interceptor/httpmethod/HttpMethodInterceptorTest.java index ab7b7bb234..468ebbcb92 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/httpmethod/HttpMethodInterceptorTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/httpmethod/HttpMethodInterceptorTest.java @@ -22,6 +22,8 @@ import org.apache.struts2.action.Action; import org.apache.struts2.ActionContext; import org.apache.struts2.ActionProxy; +import org.apache.struts2.result.Result; +import org.apache.struts2.UnknownHandlerManager; import org.apache.struts2.config.StrutsXmlConfigurationProvider; import org.apache.struts2.config.entities.ActionConfig; import org.apache.struts2.config.entities.ResultConfig; @@ -36,6 +38,11 @@ import java.util.List; import java.util.Map; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + public class HttpMethodInterceptorTest extends StrutsInternalTestCase { private HttpMethodInterceptor interceptor; @@ -280,7 +287,7 @@ private void prepareActionInvocation(Object action) { } private void prepareActionInvocation(Object action, ActionConfig config) { - interceptor = new HttpMethodInterceptor(); + interceptor = newInterceptor(); invocation = new MockActionInvocation(); invocation.setAction(action); actionProxy = new MockActionProxy(); @@ -288,6 +295,12 @@ private void prepareActionInvocation(Object action, ActionConfig config) { invocation.setProxy(actionProxy); } + private HttpMethodInterceptor newInterceptor() { + HttpMethodInterceptor httpMethodInterceptor = new HttpMethodInterceptor(); + container.inject(httpMethodInterceptor); + return httpMethodInterceptor; + } + private ActionConfig actionConfig(String... resultNames) { ActionConfig.Builder builder = new ActionConfig.Builder("package", "action", TestAction.class.getName()); for (String resultName : resultNames) { @@ -367,7 +380,7 @@ public void testWildcardResolvedExecuteRejectsGetThroughRealProxy() throws Excep assertEquals("execute", proxy.getMethod()); assertTrue("Wildcard-resolved method must report isMethodSpecified()=true", proxy.isMethodSpecified()); - HttpMethodInterceptor realInterceptor = new HttpMethodInterceptor(); + HttpMethodInterceptor realInterceptor = newInterceptor(); String result = realInterceptor.intercept(proxy.getInvocation()); // class-level @AllowedHttpMethod(POST) must still be enforced even though the resolved @@ -404,6 +417,34 @@ public void testConfigAndAnnotationMustBothAllowTheMethod() throws Exception { assertEquals("bad-request", interceptor.intercept(invocation)); } + public void testAnnotationNarrowsConfig() throws Exception { + ActionConfig config = new ActionConfig.Builder(actionConfig("bad-request")) + .addHttpMethods(List.of(HttpMethod.GET, HttpMethod.POST)) + .build(); + prepareActionInvocation(new HttpMethodsTestAction(), config); + invocation.setResultCode("success"); + + prepareRequest("post"); + assertEquals("success", interceptor.intercept(invocation)); + + prepareRequest("get"); + assertEquals("bad-request", interceptor.intercept(invocation)); + } + + public void testUnknownHandlerResultIsUsedInsteadOf405() throws Exception { + prepareActionInvocation(new HttpMethodsTestAction(), actionConfig()); + UnknownHandlerManager unknownHandlerManager = mock(UnknownHandlerManager.class); + when(unknownHandlerManager.hasUnknownHandlers()).thenReturn(true); + when(unknownHandlerManager.handleUnknownResult(any(), any(), any(), eq("bad-request"))).thenReturn(mock(Result.class)); + interceptor.setUnknownHandlerManager(unknownHandlerManager); + MockHttpServletResponse response = prepareRequest("get"); + + String resultName = interceptor.intercept(invocation); + + assertEquals("bad-request", resultName); + assertEquals(HttpServletResponse.SC_OK, response.getStatus()); + } + public void testConfigRestrictsMethodWithoutAnnotation() throws Exception { ActionConfig config = new ActionConfig.Builder(actionConfig("bad-request")) .addHttpMethods(List.of(HttpMethod.GET)) @@ -436,7 +477,7 @@ public void testHttpMethodAttributeRejectsWith405AndAllowHeader() throws Excepti MockHttpServletResponse response = new MockHttpServletResponse(); ActionProxy proxy = proxyFor("", "PostOnly", "GET", response); - String result = new HttpMethodInterceptor().intercept(proxy.getInvocation()); + String result = newInterceptor().intercept(proxy.getInvocation()); assertEquals(Action.NONE, result); assertEquals(HttpServletResponse.SC_METHOD_NOT_ALLOWED, response.getStatus()); @@ -446,7 +487,7 @@ public void testHttpMethodAttributeRejectsWith405AndAllowHeader() throws Excepti public void testHttpMethodAttributeAllowsListedMethod() throws Exception { ActionProxy proxy = proxyFor("", "PostOnly", "POST", new MockHttpServletResponse()); - String result = new HttpMethodInterceptor().intercept(proxy.getInvocation()); + String result = newInterceptor().intercept(proxy.getInvocation()); assertEquals(Action.SUCCESS, result); } @@ -455,7 +496,7 @@ public void testHttpMethodAttributeListsAllMethodsInAllowHeader() throws Excepti MockHttpServletResponse response = new MockHttpServletResponse(); ActionProxy proxy = proxyFor("", "GetOrPost", "PUT", response); - String result = new HttpMethodInterceptor().intercept(proxy.getInvocation()); + String result = newInterceptor().intercept(proxy.getInvocation()); assertEquals(Action.NONE, result); assertEquals("GET, POST", response.getHeader("Allow")); @@ -465,7 +506,7 @@ public void testHttpMethodAttributeUsesMappedActionResult() throws Exception { MockHttpServletResponse response = new MockHttpServletResponse(); ActionProxy proxy = proxyFor("", "PostOnlyWithResult", "GET", response); - String result = new HttpMethodInterceptor().intercept(proxy.getInvocation()); + String result = newInterceptor().intercept(proxy.getInvocation()); assertEquals("bad-request", result); assertEquals(HttpServletResponse.SC_OK, response.getStatus()); @@ -475,7 +516,17 @@ public void testHttpMethodAttributeUsesMappedGlobalResult() throws Exception { MockHttpServletResponse response = new MockHttpServletResponse(); ActionProxy proxy = proxyFor("/global", "PostOnly", "GET", response); - String result = new HttpMethodInterceptor().intercept(proxy.getInvocation()); + String result = newInterceptor().intercept(proxy.getInvocation()); + + assertEquals("bad-request", result); + assertEquals(HttpServletResponse.SC_OK, response.getStatus()); + } + + public void testHttpMethodAttributeUsesCatchAllResult() throws Exception { + MockHttpServletResponse response = new MockHttpServletResponse(); + ActionProxy proxy = proxyFor("", "PostOnlyWithCatchAllResult", "GET", response); + + String result = newInterceptor().intercept(proxy.getInvocation()); assertEquals("bad-request", result); assertEquals(HttpServletResponse.SC_OK, response.getStatus()); @@ -485,7 +536,7 @@ public void testHttpMethodAttributeOnWildcardAction() throws Exception { MockHttpServletResponse response = new MockHttpServletResponse(); ActionProxy proxy = proxyFor("", "PostWild-execute", "GET", response); - String result = new HttpMethodInterceptor().intercept(proxy.getInvocation()); + String result = newInterceptor().intercept(proxy.getInvocation()); assertEquals(Action.NONE, result); assertEquals(HttpServletResponse.SC_METHOD_NOT_ALLOWED, response.getStatus()); diff --git a/core/src/test/resources/org/apache/struts2/config/providers/struts-http-method.xml b/core/src/test/resources/org/apache/struts2/config/providers/struts-http-method.xml index c96342e1d4..cc309d5984 100644 --- a/core/src/test/resources/org/apache/struts2/config/providers/struts-http-method.xml +++ b/core/src/test/resources/org/apache/struts2/config/providers/struts-http-method.xml @@ -46,6 +46,11 @@ /bad-request.jsp + + + /any.jsp + + regex:.* From ced991c4f56c48e8e90f457784de89a6d0962190 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Fri, 16 Oct 2026 17:24:12 +0200 Subject: [PATCH 3/4] WW-3717 fix(core): look up the unknown handler manager when a request is rejected Injecting UnknownHandlerManager into HttpMethodInterceptor built the unknown handlers while the interceptor was created during configuration loading, before ConventionUnknownHandler could find its parent package, which broke every Convention plugin application at startup. Resolve the manager from the request's container only on the reject path instead. Co-Authored-By: Claude Opus 5.5 --- .../httpmethod/HttpMethodInterceptor.java | 9 +----- .../httpmethod/HttpMethodInterceptorTest.java | 29 +++++++++---------- 2 files changed, 14 insertions(+), 24 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/interceptor/httpmethod/HttpMethodInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/httpmethod/HttpMethodInterceptor.java index 3247f2a52a..30cde2be88 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/httpmethod/HttpMethodInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/httpmethod/HttpMethodInterceptor.java @@ -22,7 +22,6 @@ import org.apache.struts2.UnknownHandlerManager; import org.apache.struts2.action.Action; import org.apache.struts2.config.entities.ActionConfig; -import org.apache.struts2.inject.Inject; import org.apache.struts2.interceptor.AbstractInterceptor; import org.apache.struts2.util.AnnotationUtils; import org.apache.logging.log4j.LogManager; @@ -87,13 +86,6 @@ public class HttpMethodInterceptor extends AbstractInterceptor { private String badRequestResultName = "bad-request"; - private UnknownHandlerManager unknownHandlerManager; - - @Inject - public void setUnknownHandlerManager(UnknownHandlerManager unknownHandlerManager) { - this.unknownHandlerManager = unknownHandlerManager; - } - @Override public String intercept(ActionInvocation invocation) throws Exception { Object action = invocation.getAction(); @@ -182,6 +174,7 @@ private boolean canResolveResult(ActionInvocation invocation, String resultName) if (config.getResults().containsKey(resultName) || config.getResults().containsKey("*")) { return true; } + UnknownHandlerManager unknownHandlerManager = invocation.getInvocationContext().getContainer().getInstance(UnknownHandlerManager.class); return unknownHandlerManager.hasUnknownHandlers() && unknownHandlerManager.handleUnknownResult(invocation.getInvocationContext(), invocation.getProxy().getActionName(), config, resultName) != null; diff --git a/core/src/test/java/org/apache/struts2/interceptor/httpmethod/HttpMethodInterceptorTest.java b/core/src/test/java/org/apache/struts2/interceptor/httpmethod/HttpMethodInterceptorTest.java index 468ebbcb92..2a4abdd7fb 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/httpmethod/HttpMethodInterceptorTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/httpmethod/HttpMethodInterceptorTest.java @@ -27,6 +27,7 @@ import org.apache.struts2.config.StrutsXmlConfigurationProvider; import org.apache.struts2.config.entities.ActionConfig; import org.apache.struts2.config.entities.ResultConfig; +import org.apache.struts2.inject.Container; import org.apache.struts2.mock.MockActionInvocation; import org.apache.struts2.mock.MockActionProxy; import org.apache.struts2.HttpMethodsTestAction; @@ -287,7 +288,7 @@ private void prepareActionInvocation(Object action) { } private void prepareActionInvocation(Object action, ActionConfig config) { - interceptor = newInterceptor(); + interceptor = new HttpMethodInterceptor(); invocation = new MockActionInvocation(); invocation.setAction(action); actionProxy = new MockActionProxy(); @@ -295,12 +296,6 @@ private void prepareActionInvocation(Object action, ActionConfig config) { invocation.setProxy(actionProxy); } - private HttpMethodInterceptor newInterceptor() { - HttpMethodInterceptor httpMethodInterceptor = new HttpMethodInterceptor(); - container.inject(httpMethodInterceptor); - return httpMethodInterceptor; - } - private ActionConfig actionConfig(String... resultNames) { ActionConfig.Builder builder = new ActionConfig.Builder("package", "action", TestAction.class.getName()); for (String resultName : resultNames) { @@ -380,7 +375,7 @@ public void testWildcardResolvedExecuteRejectsGetThroughRealProxy() throws Excep assertEquals("execute", proxy.getMethod()); assertTrue("Wildcard-resolved method must report isMethodSpecified()=true", proxy.isMethodSpecified()); - HttpMethodInterceptor realInterceptor = newInterceptor(); + HttpMethodInterceptor realInterceptor = new HttpMethodInterceptor(); String result = realInterceptor.intercept(proxy.getInvocation()); // class-level @AllowedHttpMethod(POST) must still be enforced even though the resolved @@ -436,8 +431,10 @@ public void testUnknownHandlerResultIsUsedInsteadOf405() throws Exception { UnknownHandlerManager unknownHandlerManager = mock(UnknownHandlerManager.class); when(unknownHandlerManager.hasUnknownHandlers()).thenReturn(true); when(unknownHandlerManager.handleUnknownResult(any(), any(), any(), eq("bad-request"))).thenReturn(mock(Result.class)); - interceptor.setUnknownHandlerManager(unknownHandlerManager); + Container mockContainer = mock(Container.class); + when(mockContainer.getInstance(UnknownHandlerManager.class)).thenReturn(unknownHandlerManager); MockHttpServletResponse response = prepareRequest("get"); + ActionContext.getContext().withContainer(mockContainer); String resultName = interceptor.intercept(invocation); @@ -477,7 +474,7 @@ public void testHttpMethodAttributeRejectsWith405AndAllowHeader() throws Excepti MockHttpServletResponse response = new MockHttpServletResponse(); ActionProxy proxy = proxyFor("", "PostOnly", "GET", response); - String result = newInterceptor().intercept(proxy.getInvocation()); + String result = new HttpMethodInterceptor().intercept(proxy.getInvocation()); assertEquals(Action.NONE, result); assertEquals(HttpServletResponse.SC_METHOD_NOT_ALLOWED, response.getStatus()); @@ -487,7 +484,7 @@ public void testHttpMethodAttributeRejectsWith405AndAllowHeader() throws Excepti public void testHttpMethodAttributeAllowsListedMethod() throws Exception { ActionProxy proxy = proxyFor("", "PostOnly", "POST", new MockHttpServletResponse()); - String result = newInterceptor().intercept(proxy.getInvocation()); + String result = new HttpMethodInterceptor().intercept(proxy.getInvocation()); assertEquals(Action.SUCCESS, result); } @@ -496,7 +493,7 @@ public void testHttpMethodAttributeListsAllMethodsInAllowHeader() throws Excepti MockHttpServletResponse response = new MockHttpServletResponse(); ActionProxy proxy = proxyFor("", "GetOrPost", "PUT", response); - String result = newInterceptor().intercept(proxy.getInvocation()); + String result = new HttpMethodInterceptor().intercept(proxy.getInvocation()); assertEquals(Action.NONE, result); assertEquals("GET, POST", response.getHeader("Allow")); @@ -506,7 +503,7 @@ public void testHttpMethodAttributeUsesMappedActionResult() throws Exception { MockHttpServletResponse response = new MockHttpServletResponse(); ActionProxy proxy = proxyFor("", "PostOnlyWithResult", "GET", response); - String result = newInterceptor().intercept(proxy.getInvocation()); + String result = new HttpMethodInterceptor().intercept(proxy.getInvocation()); assertEquals("bad-request", result); assertEquals(HttpServletResponse.SC_OK, response.getStatus()); @@ -516,7 +513,7 @@ public void testHttpMethodAttributeUsesMappedGlobalResult() throws Exception { MockHttpServletResponse response = new MockHttpServletResponse(); ActionProxy proxy = proxyFor("/global", "PostOnly", "GET", response); - String result = newInterceptor().intercept(proxy.getInvocation()); + String result = new HttpMethodInterceptor().intercept(proxy.getInvocation()); assertEquals("bad-request", result); assertEquals(HttpServletResponse.SC_OK, response.getStatus()); @@ -526,7 +523,7 @@ public void testHttpMethodAttributeUsesCatchAllResult() throws Exception { MockHttpServletResponse response = new MockHttpServletResponse(); ActionProxy proxy = proxyFor("", "PostOnlyWithCatchAllResult", "GET", response); - String result = newInterceptor().intercept(proxy.getInvocation()); + String result = new HttpMethodInterceptor().intercept(proxy.getInvocation()); assertEquals("bad-request", result); assertEquals(HttpServletResponse.SC_OK, response.getStatus()); @@ -536,7 +533,7 @@ public void testHttpMethodAttributeOnWildcardAction() throws Exception { MockHttpServletResponse response = new MockHttpServletResponse(); ActionProxy proxy = proxyFor("", "PostWild-execute", "GET", response); - String result = newInterceptor().intercept(proxy.getInvocation()); + String result = new HttpMethodInterceptor().intercept(proxy.getInvocation()); assertEquals(Action.NONE, result); assertEquals(HttpServletResponse.SC_METHOD_NOT_ALLOWED, response.getStatus()); From 1dade4fa7695da4d93fe570f3d2623138adb9f9c Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sat, 17 Oct 2026 09:10:46 +0200 Subject: [PATCH 4/4] WW-3717 refactor(core): address Sonar findings on the http-method change Makes ActionConfig.httpMethods private, narrows rejectRequest to throw IOException, simplifies the end of equals, and tidies two tests. Co-Authored-By: Claude Opus 5.5 --- .../org/apache/struts2/config/entities/ActionConfig.java | 8 ++------ .../interceptor/httpmethod/HttpMethodInterceptor.java | 3 ++- .../providers/XmlConfigurationProviderHttpMethodTest.java | 3 ++- .../interceptor/httpmethod/HttpMethodInterceptorTest.java | 3 ++- 4 files changed, 8 insertions(+), 9 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/config/entities/ActionConfig.java b/core/src/main/java/org/apache/struts2/config/entities/ActionConfig.java index 47039fc572..b2088a2690 100644 --- a/core/src/main/java/org/apache/struts2/config/entities/ActionConfig.java +++ b/core/src/main/java/org/apache/struts2/config/entities/ActionConfig.java @@ -68,7 +68,7 @@ public class ActionConfig extends Located implements Serializable { protected String name; protected boolean strictMethodInvocation = true; protected AllowedMethods allowedMethods; - protected Set httpMethods; + private Set httpMethods; protected ActionConfig(String packageName, String name, String className) { this.packageName = packageName; @@ -200,11 +200,7 @@ public boolean equals(Object o) { return false; } - if (!Objects.equals(httpMethods, actionConfig.httpMethods)) { - return false; - } - - return true; + return Objects.equals(httpMethods, actionConfig.httpMethods); } @Override diff --git a/core/src/main/java/org/apache/struts2/interceptor/httpmethod/HttpMethodInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/httpmethod/HttpMethodInterceptor.java index 30cde2be88..c6dbfc0d3b 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/httpmethod/HttpMethodInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/httpmethod/HttpMethodInterceptor.java @@ -29,6 +29,7 @@ import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; +import java.io.IOException; import java.lang.annotation.Annotation; import java.lang.reflect.AnnotatedElement; import java.lang.reflect.Method; @@ -155,7 +156,7 @@ private boolean isAllowed(String requestedMethod, Set allowedMethods * after sending {@code 405 Method Not Allowed} with an {@code Allow} header * @since 7.5.0 */ - protected String rejectRequest(ActionInvocation invocation, Collection allowedMethods) throws Exception { + protected String rejectRequest(ActionInvocation invocation, Collection allowedMethods) throws IOException { String resultName = getBadRequestResultName(invocation); if (canResolveResult(invocation, resultName)) { return resultName; diff --git a/core/src/test/java/org/apache/struts2/config/providers/XmlConfigurationProviderHttpMethodTest.java b/core/src/test/java/org/apache/struts2/config/providers/XmlConfigurationProviderHttpMethodTest.java index 5ceceea250..235ec3ca38 100644 --- a/core/src/test/java/org/apache/struts2/config/providers/XmlConfigurationProviderHttpMethodTest.java +++ b/core/src/test/java/org/apache/struts2/config/providers/XmlConfigurationProviderHttpMethodTest.java @@ -64,7 +64,8 @@ public void testWildcardActionKeepsHttpMethod() { } public void testInvalidHttpMethodFailsConfiguration() { - assertThatThrownBy(() -> loadConfigurationProviders(new StrutsXmlConfigurationProvider(INVALID_CONFIG))) + StrutsXmlConfigurationProvider provider = new StrutsXmlConfigurationProvider(INVALID_CONFIG); + assertThatThrownBy(() -> loadConfigurationProviders(provider)) .isInstanceOf(ConfigurationException.class) .hasStackTraceContaining("Invalid http-method [fetch] on action [Fetch]"); } diff --git a/core/src/test/java/org/apache/struts2/interceptor/httpmethod/HttpMethodInterceptorTest.java b/core/src/test/java/org/apache/struts2/interceptor/httpmethod/HttpMethodInterceptorTest.java index 2a4abdd7fb..63ad8118f1 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/httpmethod/HttpMethodInterceptorTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/httpmethod/HttpMethodInterceptorTest.java @@ -430,7 +430,8 @@ public void testUnknownHandlerResultIsUsedInsteadOf405() throws Exception { prepareActionInvocation(new HttpMethodsTestAction(), actionConfig()); UnknownHandlerManager unknownHandlerManager = mock(UnknownHandlerManager.class); when(unknownHandlerManager.hasUnknownHandlers()).thenReturn(true); - when(unknownHandlerManager.handleUnknownResult(any(), any(), any(), eq("bad-request"))).thenReturn(mock(Result.class)); + Result unknownResult = mock(Result.class); + when(unknownHandlerManager.handleUnknownResult(any(), any(), any(), eq("bad-request"))).thenReturn(unknownResult); Container mockContainer = mock(Container.class); when(mockContainer.getInstance(UnknownHandlerManager.class)).thenReturn(unknownHandlerManager); MockHttpServletResponse response = prepareRequest("get");