WW-3717 feat(core): restrict an action to HTTP methods with an http-method attribute

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;
+    private Set<HttpMethod> 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<String> getAllowedMethods() {
         return allowedMethods.list();
     }
 
+    /**
+     * @return HTTP methods this action accepts, empty when it accepts any
+     * @since 7.5.0
+     */
+    public Set<HttpMethod> getHttpMethods() {
+        return httpMethods;
+    }
+
     /**
      * Returns name of the action method
      *
@@ -186,7 +200,7 @@ public boolean equals(Object o) {
             return false;
         }
 
-        return true;
+        return Objects.equals(httpMethods, actionConfig.httpMethods);
     }
 
     @Override
@@ -201,6 +215,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 +231,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 +360,14 @@ public Builder addAllowedMethod(Collection<String> methods) {
             return this;
         }
 
+        /**
+         * @since 7.5.0
+         */
+        public Builder addHttpMethods(Collection<HttpMethod> methods) {
+            target.httpMethods.addAll(methods);
+            return this;
+        }
+
         public Builder location(Location loc) {
             target.location = loc;
             return this;
@@ -362,6 +388,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;

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())

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<InterceptorMapping> 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 {
 

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<InterceptorMapping> interceptorList = buildInterceptorList(actionElement, packageContext);
         List<ExceptionMappingConfig> exceptionMappings = buildExceptionMappings(actionElement);
         Set<String> allowedMethods = buildAllowedMethods(actionElement, packageContext);
+        Set<HttpMethod> 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<HttpMethod> buildHttpMethods(Element actionElement, Location location) {
+        Set<HttpMethod> 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);

core/src/main/java/org/apache/struts2/interceptor/httpmethod/HttpMethodInterceptor.java

@@ -19,19 +19,28 @@
 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.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.io.IOException;
 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 +54,12 @@
  * {@link HttpPut}, {@link HttpDelete} and {@link HttpGetOrPost}
  * <p>
  * You can combine any of these annotations to achieve required allowed methods' filtering.
+ * <p>
+ * 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.
+ * <p>
+ * 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
  * @see HttpMethod
@@ -97,21 +112,73 @@ public String intercept(ActionInvocation invocation) throws Exception {
             return doIntercept(invocation, action.getClass());
         }
 
+        Set<HttpMethod> 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<HttpMethod> allowedMethods = readAllowedMethods(element);
+        Set<HttpMethod> allowedMethods = EnumSet.noneOf(HttpMethod.class);
+        allowedMethods.addAll(readAllowedMethods(element));
+
+        Set<HttpMethod> configuredMethods = invocation.getProxy().getConfig().getHttpMethods();
+        if (!configuredMethods.isEmpty()) {
+            allowedMethods.retainAll(configuredMethods);
+        }
+
+        return checkAllowedMethods(invocation, allowedMethods);
+    }
+
+    private String checkAllowedMethods(ActionInvocation invocation, Set<HttpMethod> 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<HttpMethod> 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<HttpMethod> allowedMethods) throws IOException {
+        String resultName = getBadRequestResultName(invocation);
+        if (canResolveResult(invocation, 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;
+    }
+
+    private boolean canResolveResult(ActionInvocation invocation, String resultName) {
+        ActionConfig config = invocation.getProxy().getConfig();
+        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;
     }
 
     protected List<HttpMethod> readAllowedMethods(AnnotatedElement element) {

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
 >
 
 <!ELEMENT param (#PCDATA)>