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 c602678388..54b2988e31 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 @@ -18,6 +18,7 @@ */ package org.apache.struts2.interceptor.httpmethod; +import org.apache.commons.lang3.ClassUtils; import org.apache.struts2.ActionInvocation; import org.apache.struts2.action.Action; import org.apache.struts2.interceptor.AbstractInterceptor; @@ -32,11 +33,13 @@ import java.lang.annotation.Annotation; import java.lang.reflect.AnnotatedElement; import java.lang.reflect.Method; +import java.lang.reflect.Modifier; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; import java.util.Collections; import java.util.EnumSet; +import java.util.LinkedHashSet; import java.util.List; import java.util.Set; import java.util.stream.Collectors; @@ -54,6 +57,11 @@ *

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

+ * Method annotations apply to the invoked method, including the implicit {@code execute()} method. + * Its annotation takes precedence; otherwise annotated declarations are looked up through the class + * and interface hierarchy. Class annotations are looked up only when no method annotation is found. + * Restrictions apply to {@link HttpServletRequest#getMethod()}, not a {@code _method} parameter. + *

* 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. *

@@ -97,18 +105,20 @@ public String intercept(ActionInvocation invocation) throws Exception { ((HttpMethodAware) (action)).setMethod(HttpMethod.parse(request.getMethod())); } - if (invocation.getProxy().isMethodSpecified()) { - Method method = action.getClass().getMethod(invocation.getProxy().getMethod()); - if (AnnotationUtils.isAnnotatedBy(method, HTTP_METHOD_ANNOTATIONS)) { + Method method = resolveInvokedMethod(invocation); + if (method != null) { + AnnotatedElement methodElement = findNearestAnnotatedOverride(action.getClass(), method); + if (methodElement != null) { LOG.debug("Action's method: {} annotated with: {}, checking if request: {} meets allowed methods!", invocation.getProxy().getMethod(), AllowedHttpMethod.class.getSimpleName(), request.getMethod()); - return doIntercept(invocation, method); + return doIntercept(invocation, methodElement); } } - if (AnnotationUtils.isAnnotatedBy(action.getClass(), HTTP_METHOD_ANNOTATIONS)) { + AnnotatedElement classElement = findNearestAnnotatedType(action.getClass()); + if (classElement != null) { LOG.debug("Action: {} annotated with: {}, checking if request: {} meets allowed methods!", action, AllowedHttpMethod.class.getSimpleName(), request.getMethod()); - return doIntercept(invocation, action.getClass()); + return doIntercept(invocation, classElement); } Set configuredMethods = invocation.getProxy().getConfig().getHttpMethods(); @@ -133,6 +143,83 @@ protected String doIntercept(ActionInvocation invocation, AnnotatedElement eleme return checkAllowedMethods(invocation, allowedMethods); } + private Method resolveInvokedMethod(ActionInvocation invocation) throws NoSuchMethodException { + String methodName = invocation.getProxy().getMethod(); + if (methodName == null || methodName.isEmpty()) { + return null; + } + try { + return invocation.getAction().getClass().getMethod(methodName); + } catch (NoSuchMethodException e) { + if (invocation.getProxy().isMethodSpecified()) { + throw e; + } + // An absent implicit execute() can be handled by an UnknownHandler during invocation. + return null; + } + } + + /** Resolves the invoked method first, then annotated declarations at each class and its interfaces. */ + private AnnotatedElement findNearestAnnotatedOverride(Class actionClass, Method method) { + if (!isPublicInstanceMethod(method)) { + return null; + } + if (AnnotationUtils.isAnnotatedBy(method, HTTP_METHOD_ANNOTATIONS)) { + return method; + } + for (Class type = actionClass; type != null; type = type.getSuperclass()) { + AnnotatedElement found = declaredOverrideAnnotatedIn(type, method); + if (found != null) { + return found; + } + for (Class iface : interfacesAtLevel(type)) { + found = declaredOverrideAnnotatedIn(iface, method); + if (found != null) { + return found; + } + } + } + return null; + } + + private AnnotatedElement declaredOverrideAnnotatedIn(Class type, Method method) { + try { + Method candidate = type.getDeclaredMethod(method.getName(), method.getParameterTypes()); + return isPublicInstanceMethod(candidate) && AnnotationUtils.isAnnotatedBy(candidate, HTTP_METHOD_ANNOTATIONS) + ? candidate : null; + } catch (NoSuchMethodException e) { + return null; + } + } + + private boolean isPublicInstanceMethod(Method method) { + return Modifier.isPublic(method.getModifiers()) && !Modifier.isStatic(method.getModifiers()); + } + + /** Checks each class before its interfaces, including their superinterfaces, then its superclass. */ + private AnnotatedElement findNearestAnnotatedType(Class actionClass) { + for (Class type = actionClass; type != null; type = type.getSuperclass()) { + if (AnnotationUtils.isAnnotatedBy(type, HTTP_METHOD_ANNOTATIONS)) { + return type; + } + for (Class iface : interfacesAtLevel(type)) { + if (AnnotationUtils.isAnnotatedBy(iface, HTTP_METHOD_ANNOTATIONS)) { + return iface; + } + } + } + return null; + } + + private Set> interfacesAtLevel(Class type) { + Set> interfaces = new LinkedHashSet<>(); + for (Class iface : type.getInterfaces()) { + interfaces.add(iface); + interfaces.addAll(ClassUtils.getAllInterfaces(iface)); + } + return interfaces; + } + private String checkAllowedMethods(ActionInvocation invocation, Set allowedMethods) throws Exception { HttpServletRequest request = invocation.getInvocationContext().getServletRequest(); String requestedMethod = request.getMethod(); diff --git a/core/src/test/java/org/apache/struts2/interceptor/httpmethod/HttpMethodDefaultStacksTest.java b/core/src/test/java/org/apache/struts2/interceptor/httpmethod/HttpMethodDefaultStacksTest.java new file mode 100644 index 0000000000..b76adad821 --- /dev/null +++ b/core/src/test/java/org/apache/struts2/interceptor/httpmethod/HttpMethodDefaultStacksTest.java @@ -0,0 +1,59 @@ +/* + * 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.interceptor.httpmethod; + +import org.apache.struts2.StrutsInternalTestCase; +import org.apache.struts2.config.entities.InterceptorMapping; +import org.apache.struts2.config.entities.InterceptorStackConfig; +import org.apache.struts2.config.entities.PackageConfig; + +import java.util.ArrayList; +import java.util.List; +import java.util.Map; + +/** Every bundled core stack that binds parameters also enforces HTTP method restrictions. */ +public class HttpMethodDefaultStacksTest extends StrutsInternalTestCase { + + public void testEveryStackWithParamsAlsoHasHttpMethod() { + PackageConfig pc = configuration.getPackageConfig("struts-default"); + Map stacks = allStacks(pc); + assertTrue("expected to find bundled interceptor-stacks", stacks.size() > 5); + + List violations = new ArrayList<>(); + for (Map.Entry e : stacks.entrySet()) { + String name = e.getKey(); + List names = e.getValue().getInterceptors().stream().map(InterceptorMapping::getName).toList(); + if (names.contains("params") && !names.contains("httpMethod")) { + violations.add(name); + } + } + assertTrue("stacks binding params but not enforcing httpMethod: " + violations, violations.isEmpty()); + } + + private Map allStacks(PackageConfig pc) { + Map all = pc.getAllInterceptorConfigs(); + Map out = new java.util.LinkedHashMap<>(); + for (Map.Entry e : all.entrySet()) { + if (e.getValue() instanceof InterceptorStackConfig stackConfig) { + out.put(e.getKey(), stackConfig); + } + } + return out; + } +} diff --git a/core/src/test/java/org/apache/struts2/interceptor/httpmethod/HttpMethodHierarchyActions.java b/core/src/test/java/org/apache/struts2/interceptor/httpmethod/HttpMethodHierarchyActions.java new file mode 100644 index 0000000000..49e1096afa --- /dev/null +++ b/core/src/test/java/org/apache/struts2/interceptor/httpmethod/HttpMethodHierarchyActions.java @@ -0,0 +1,65 @@ +/* + * 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.interceptor.httpmethod; + +import org.apache.struts2.ActionSupport; +import org.apache.struts2.interceptor.httpmethod.HttpPost; + +public class HttpMethodHierarchyActions { + public static final java.util.concurrent.atomic.AtomicInteger RAN = new java.util.concurrent.atomic.AtomicInteger(); + + public static class ExecutePost extends ActionSupport { + @HttpPost + @Override + public String execute() { RAN.incrementAndGet(); return SUCCESS; } + } + public static class NamedPost extends ActionSupport { + @HttpPost + public String save() { RAN.incrementAndGet(); return SUCCESS; } + } + @HttpPost + public static class ClassPost extends ActionSupport { + @Override + public String execute() { RAN.incrementAndGet(); return SUCCESS; } + } + public static class SubOfClassPost extends ClassPost { } + public static class BaseMethodPost extends ActionSupport { + @HttpPost + public String save() { RAN.incrementAndGet(); return SUCCESS; } + } + public static class SubOverride extends BaseMethodPost { + @Override + public String save() { RAN.incrementAndGet(); return SUCCESS; } + } + + public interface MethodPostMarker { + @HttpPost + default String save() { HttpMethodHierarchyActions.RAN.incrementAndGet(); return ActionSupport.SUCCESS; } + } + public static class ImplementsMethodMarker extends ActionSupport implements MethodPostMarker { } + public static class SubOfImplementsMethodMarker extends ImplementsMethodMarker { } + + @HttpPost + public interface ClassPostMarker { } + public static class ImplementsClassMarker extends ActionSupport implements ClassPostMarker { + @Override + public String execute() { RAN.incrementAndGet(); return SUCCESS; } + } + public static class SubOfImplementsClassMarker extends ImplementsClassMarker { } +} diff --git a/core/src/test/java/org/apache/struts2/interceptor/httpmethod/HttpMethodHierarchyTest.java b/core/src/test/java/org/apache/struts2/interceptor/httpmethod/HttpMethodHierarchyTest.java new file mode 100644 index 0000000000..150c9c408a --- /dev/null +++ b/core/src/test/java/org/apache/struts2/interceptor/httpmethod/HttpMethodHierarchyTest.java @@ -0,0 +1,88 @@ +/* + * 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.interceptor.httpmethod; + +import org.apache.struts2.ActionContext; +import org.apache.struts2.ActionProxy; +import org.apache.struts2.StrutsInternalTestCase; +import org.apache.struts2.config.StrutsXmlConfigurationProvider; +import org.springframework.mock.web.MockHttpServletRequest; +import org.springframework.mock.web.MockHttpServletResponse; + +import java.util.Map; + +public class HttpMethodHierarchyTest extends StrutsInternalTestCase { + + private MockHttpServletResponse run(String action, String verb) throws Exception { + loadConfigurationProviders(new StrutsXmlConfigurationProvider("struts-http-method-hierarchy.xml")); + MockHttpServletResponse response = new MockHttpServletResponse(); + Map context = ActionContext.of() + .withServletRequest(new MockHttpServletRequest(verb, "/" + action)) + .withServletResponse(response).getContextMap(); + HttpMethodHierarchyActions.RAN.set(0); + ActionProxy proxy = actionProxyFactory.createActionProxy("", action, null, context, false, true); + proxy.execute(); + return response; + } + + private void assertGetRejected(String action) throws Exception { + MockHttpServletResponse response = run(action, "GET"); + assertEquals(405, response.getStatus()); + assertEquals("POST", response.getHeader("Allow")); + assertEquals("action must not run", 0, HttpMethodHierarchyActions.RAN.get()); + } + + public void testControlNamedMethodGetRejected() throws Exception { + assertGetRejected("NamedPost"); + } + + public void testControlClassLevelGetRejected() throws Exception { + assertGetRejected("ClassPost"); + } + + public void testExecuteMethodAnnotationGetRejected() throws Exception { + assertGetRejected("ExecutePost"); + } + + public void testSubclassOfClassLevelGetRejected() throws Exception { + assertGetRejected("SubOfClassPost"); + } + + public void testOverriddenMethodRetainsAnnotation() throws Exception { + assertGetRejected("SubOverride"); + } + + public void testSubclassOfInterfaceDefaultMethodGetRejected() throws Exception { + assertGetRejected("SubOfImplementsMethodMarker"); + } + + public void testSubclassOfImplementedInterfaceClassLevelGetRejected() throws Exception { + assertGetRejected("SubOfImplementsClassMarker"); + } + + public void testPostAllowedForEveryAnnotatedActionShape() throws Exception { + for (String action : new String[]{"NamedPost", "ClassPost", "ExecutePost", "SubOfClassPost", + "SubOverride", "SubOfImplementsMethodMarker", "SubOfImplementsClassMarker"}) { + MockHttpServletResponse response = run(action, "POST"); + assertEquals(action, 200, response.getStatus()); + assertNull(action, response.getHeader("Allow")); + assertEquals(action, 1, HttpMethodHierarchyActions.RAN.get()); + } + } +} 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 63ad8118f1..f226511166 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 @@ -540,6 +540,207 @@ public void testHttpMethodAttributeOnWildcardAction() throws Exception { assertEquals(HttpServletResponse.SC_METHOD_NOT_ALLOWED, response.getStatus()); } + public void testMissingExplicitMethodThrowsForGet() throws Exception { + assertMissingExplicitMethodThrows("GET"); + } + + public void testMissingExplicitMethodThrowsForPost() throws Exception { + assertMissingExplicitMethodThrows("POST"); + } + + private void assertMissingExplicitMethodThrows(String httpMethod) throws Exception { + prepareActionInvocation(new HttpMethodsTestAction(), actionConfig()); + actionProxy.setMethod("missingMethod"); + actionProxy.setMethodSpecified(true); + MockHttpServletResponse response = prepareRequest(httpMethod); + + try { + interceptor.intercept(invocation); + fail("An explicitly configured missing method must throw"); + } catch (NoSuchMethodException expected) { + assertTrue(expected.getMessage().contains("missingMethod")); + } + assertEquals(HttpServletResponse.SC_OK, response.getStatus()); + } + + public void testMissingImplicitExecuteReachesInvocation() throws Exception { + prepareActionInvocation(new Object(), actionConfig()); + actionProxy.setMethod("execute"); + actionProxy.setMethodSpecified(false); + invocation.setResultCode("unknown-handler-result"); + MockHttpServletResponse response = prepareRequest("GET"); + + assertEquals("unknown-handler-result", interceptor.intercept(invocation)); + assertEquals(HttpServletResponse.SC_OK, response.getStatus()); + } + + public void testMissingImplicitExecuteStillChecksClassAnnotation() throws Exception { + prepareActionInvocation(new PostOnlyWithoutExecute(), actionConfig()); + actionProxy.setMethod("execute"); + actionProxy.setMethodSpecified(false); + invocation.setResultCode("success"); + MockHttpServletResponse response = prepareRequest("GET"); + + assertEquals(Action.NONE, interceptor.intercept(invocation)); + assertEquals(HttpServletResponse.SC_METHOD_NOT_ALLOWED, response.getStatus()); + assertEquals("POST", response.getHeader("Allow")); + + response = prepareRequest("POST"); + assertEquals("success", interceptor.intercept(invocation)); + assertEquals(HttpServletResponse.SC_OK, response.getStatus()); + } + + public void testMissingImplicitExecuteStillChecksConfiguredHttpMethods() throws Exception { + ActionConfig config = new ActionConfig.Builder(actionConfig()) + .addHttpMethods(List.of(HttpMethod.POST)) + .build(); + prepareActionInvocation(new Object(), config); + actionProxy.setMethod("execute"); + actionProxy.setMethodSpecified(false); + invocation.setResultCode("success"); + MockHttpServletResponse response = prepareRequest("GET"); + + assertEquals(Action.NONE, interceptor.intercept(invocation)); + assertEquals(HttpServletResponse.SC_METHOD_NOT_ALLOWED, response.getStatus()); + assertEquals("POST", response.getHeader("Allow")); + + response = prepareRequest("POST"); + assertEquals("success", interceptor.intercept(invocation)); + assertEquals(HttpServletResponse.SC_OK, response.getStatus()); + } + + public void testMethodAnnotationOnSuperinterfaceIsEnforced() throws Exception { + assertPostOnly(new TransitiveMethodAction(), "save"); + } + + public void testTypeAnnotationOnSuperinterfaceIsEnforced() throws Exception { + assertPostOnly(new TransitiveTypeAction(), "execute"); + } + + public void testInheritedClassMethodAnnotationPrecedesInterfaceAnnotation() throws Exception { + assertPostOnly(new ClassMethodAndInterfaceAction(), "save"); + } + + private void assertPostOnly(Object action, String method) throws Exception { + prepareActionInvocation(action, actionConfig()); + actionProxy.setMethod(method); + actionProxy.setMethodSpecified(true); + invocation.setResultCode("success"); + MockHttpServletResponse response = prepareRequest("GET"); + + assertEquals(Action.NONE, interceptor.intercept(invocation)); + assertEquals(HttpServletResponse.SC_METHOD_NOT_ALLOWED, response.getStatus()); + assertEquals("POST", response.getHeader("Allow")); + + response = prepareRequest("POST"); + assertEquals("success", interceptor.intercept(invocation)); + assertEquals(HttpServletResponse.SC_OK, response.getStatus()); + } + + public void testPrivateAncestorMethodDoesNotRestrictPublicActionMethod() throws Exception { + assertUnrestrictedMethod(new PublicMethodOverPrivateHelperAction()); + } + + public void testPrivateStaticAncestorMethodDoesNotRestrictPublicActionMethod() throws Exception { + assertUnrestrictedMethod(new PublicMethodOverStaticHelperAction()); + } + + public void testPackagePrivateAncestorMethodDoesNotRestrictPublicActionMethod() throws Exception { + assertUnrestrictedMethod(new PublicMethodOverPackageHelperAction()); + } + + public void testStaticInterfaceMethodDoesNotRestrictPublicActionMethod() throws Exception { + assertUnrestrictedMethod(new PublicMethodWithStaticInterfaceAction()); + } + + private void assertUnrestrictedMethod(Object action) throws Exception { + prepareActionInvocation(action, actionConfig()); + actionProxy.setMethod("save"); + actionProxy.setMethodSpecified(true); + invocation.setResultCode("success"); + for (String method : List.of("GET", "POST")) { + MockHttpServletResponse response = prepareRequest(method); + assertEquals("success", interceptor.intercept(invocation)); + assertEquals(HttpServletResponse.SC_OK, response.getStatus()); + } + } + + @HttpPost + public static class PostOnlyWithoutExecute { + } + + public interface PostMethod { + @HttpPost + String save(); + } + + public interface DerivedPostMethod extends PostMethod { + } + + public static class TransitiveMethodAction implements DerivedPostMethod { + public String save() { return "success"; } + } + + @HttpPost + public interface PostType { + } + + public interface DerivedPostType extends PostType { + } + + public static class TransitiveTypeAction implements DerivedPostType { + public String execute() { return "success"; } + } + + public static class PostMethodBase { + @HttpPost + public String save() { return "success"; } + } + + public interface GetMethod { + @HttpGet + String save(); + } + + public static class ClassMethodAndInterfaceAction extends PostMethodBase implements GetMethod { + } + + public static class PrivateHelperBase { + @HttpPost + private String save() { return "helper"; } + } + + public static class PublicMethodOverPrivateHelperAction extends PrivateHelperBase { + public String save() { return "success"; } + } + + public static class StaticHelperBase { + @HttpPost + private static String save() { return "helper"; } + } + + public static class PublicMethodOverStaticHelperAction extends StaticHelperBase { + public String save() { return "success"; } + } + + public static class PackageHelperBase { + @HttpPost + String save() { return "helper"; } + } + + public static class PublicMethodOverPackageHelperAction extends PackageHelperBase { + public String save() { return "success"; } + } + + public interface StaticMethod { + @HttpPost + static String save() { return "helper"; } + } + + public static class PublicMethodWithStaticInterfaceAction implements StaticMethod { + public String save() { return "success"; } + } + private ActionProxy proxyFor(String namespace, String actionName, String httpMethod, MockHttpServletResponse response) { loadConfigurationProviders(new StrutsXmlConfigurationProvider( "org/apache/struts2/config/providers/struts-http-method.xml")); diff --git a/core/src/test/resources/struts-http-method-hierarchy.xml b/core/src/test/resources/struts-http-method-hierarchy.xml new file mode 100644 index 0000000000..90a84e0757 --- /dev/null +++ b/core/src/test/resources/struts-http-method-hierarchy.xml @@ -0,0 +1,33 @@ + + + + + + + + + + + + + + + + diff --git a/plugins/bean-validation/pom.xml b/plugins/bean-validation/pom.xml index a377e1ba02..fa332c99ff 100644 --- a/plugins/bean-validation/pom.xml +++ b/plugins/bean-validation/pom.xml @@ -73,6 +73,16 @@ jakarta.activation-api test + + org.springframework + spring-test + test + + + org.springframework + spring-web + test + diff --git a/plugins/bean-validation/src/main/resources/struts-plugin.xml b/plugins/bean-validation/src/main/resources/struts-plugin.xml index 80b3111cda..d4ee1217c2 100644 --- a/plugins/bean-validation/src/main/resources/struts-plugin.xml +++ b/plugins/bean-validation/src/main/resources/struts-plugin.xml @@ -42,6 +42,7 @@ + diff --git a/plugins/bean-validation/src/test/java/org/apache/struts/beanvalidation/BeanValidationStackHttpMethodEnforcementTest.java b/plugins/bean-validation/src/test/java/org/apache/struts/beanvalidation/BeanValidationStackHttpMethodEnforcementTest.java new file mode 100644 index 0000000000..9ca391f9e0 --- /dev/null +++ b/plugins/bean-validation/src/test/java/org/apache/struts/beanvalidation/BeanValidationStackHttpMethodEnforcementTest.java @@ -0,0 +1,82 @@ +/* + * 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.struts.beanvalidation; + +import org.apache.struts2.ActionContext; +import org.apache.struts2.ActionProxy; +import org.apache.struts2.StrutsStatics; +import org.apache.struts2.XWorkTestCase; +import org.apache.struts2.config.DefaultPropertiesProvider; +import org.apache.struts2.config.StrutsBeanSelectionProvider; +import org.apache.struts2.config.StrutsXmlConfigurationProvider; +import org.springframework.mock.web.MockHttpServletRequest; +import org.springframework.mock.web.MockHttpServletResponse; + +import java.util.HashMap; +import java.util.Map; + +/** The Bean Validation default stack enforces HTTP method annotations. */ +public class BeanValidationStackHttpMethodEnforcementTest extends XWorkTestCase { + + @Override + protected void setUp() throws Exception { + super.setUp(); + loadConfigurationProviders( + new StrutsXmlConfigurationProvider("struts-default.xml"), + new StrutsXmlConfigurationProvider("struts-plugin.xml"), + new StrutsXmlConfigurationProvider("ww-httpmethod-stack.xml"), + new DefaultPropertiesProvider(), + new StrutsBeanSelectionProvider()); + } + + private MockHttpServletResponse invoke(String namespace, String verb) throws Exception { + PostOnlyAction.invoked = false; + MockHttpServletRequest request = new MockHttpServletRequest(verb, namespace + "/mark"); + MockHttpServletResponse response = new MockHttpServletResponse(); + Map ctx = new HashMap<>(); + ctx.put(StrutsStatics.HTTP_REQUEST, request); + ctx.put(StrutsStatics.HTTP_RESPONSE, response); + ctx.putAll(ActionContext.of(ctx).withParameters(org.apache.struts2.dispatcher.HttpParameters.create(new HashMap()).build()).getContextMap()); + ActionProxy proxy = actionProxyFactory.createActionProxy(namespace, "mark", null, ctx); + proxy.execute(); + return response; + } + + private void assertGetRejected(String namespace) throws Exception { + MockHttpServletResponse response = invoke(namespace, "GET"); + assertEquals(405, response.getStatus()); + assertEquals("POST", response.getHeader("Allow")); + assertFalse("action must not run", PostOnlyAction.invoked); + } + + public void testControlCoreDefaultStackRejectsGetOnPostOnlyAction() throws Exception { + assertGetRejected("/core"); + } + + public void testBeanValidationDefaultStackRejectsGetOnPostOnlyAction() throws Exception { + assertGetRejected("/bv"); + } + + public void testBeanValidationDefaultStackAllowsPostOnPostOnlyAction() throws Exception { + MockHttpServletResponse response = invoke("/bv", "POST"); + assertEquals(200, response.getStatus()); + assertNull(response.getHeader("Allow")); + assertTrue("action must run", PostOnlyAction.invoked); + } +} diff --git a/plugins/bean-validation/src/test/java/org/apache/struts/beanvalidation/PostOnlyAction.java b/plugins/bean-validation/src/test/java/org/apache/struts/beanvalidation/PostOnlyAction.java new file mode 100644 index 0000000000..91826f9f68 --- /dev/null +++ b/plugins/bean-validation/src/test/java/org/apache/struts/beanvalidation/PostOnlyAction.java @@ -0,0 +1,34 @@ +/* + * 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.struts.beanvalidation; + +import org.apache.struts2.ActionSupport; +import org.apache.struts2.interceptor.httpmethod.HttpPost; + +/** Action that declares itself POST-only. */ +@HttpPost +public class PostOnlyAction extends ActionSupport { + public static volatile boolean invoked; + + @Override + public String execute() { + invoked = true; + return NONE; + } +} diff --git a/plugins/bean-validation/src/test/resources/ww-httpmethod-stack.xml b/plugins/bean-validation/src/test/resources/ww-httpmethod-stack.xml new file mode 100644 index 0000000000..3898b9dda1 --- /dev/null +++ b/plugins/bean-validation/src/test/resources/ww-httpmethod-stack.xml @@ -0,0 +1,30 @@ + + + + + + + + + + + diff --git a/plugins/rest/src/main/java/org/apache/struts2/rest/RestActionMapper.java b/plugins/rest/src/main/java/org/apache/struts2/rest/RestActionMapper.java index b8ecdaf85a..b347576b5d 100644 --- a/plugins/rest/src/main/java/org/apache/struts2/rest/RestActionMapper.java +++ b/plugins/rest/src/main/java/org/apache/struts2/rest/RestActionMapper.java @@ -90,6 +90,13 @@ * To simulate the HTTP methods PUT and DELETE, since they aren't supported by HTML, * the HTTP parameter "_method" will be used. *

+ *

+ * The {@code httpMethod} interceptor checks the actual request method, not {@code _method}. + * A handler used by a POST form must allow POST, even when {@code _method} routes it to + * {@code update} or {@code destroy}. {@code @HttpPut} and {@code @HttpDelete} alone require + * actual PUT and DELETE requests; add {@code @HttpPost} to also accept tunneled POST forms. + * XML {@code http-method} restrictions must likewise allow the actual request method. + *

* */ public class RestActionMapper extends DefaultActionMapper { diff --git a/plugins/rest/src/main/resources/struts-plugin.xml b/plugins/rest/src/main/resources/struts-plugin.xml index c2116856e5..37a91731f7 100644 --- a/plugins/rest/src/main/resources/struts-plugin.xml +++ b/plugins/rest/src/main/resources/struts-plugin.xml @@ -76,6 +76,7 @@ + AUTOMATIC diff --git a/plugins/rest/src/test/java/org/apache/struts2/rest/PostOnlyAction.java b/plugins/rest/src/test/java/org/apache/struts2/rest/PostOnlyAction.java new file mode 100644 index 0000000000..dca1a48d05 --- /dev/null +++ b/plugins/rest/src/test/java/org/apache/struts2/rest/PostOnlyAction.java @@ -0,0 +1,34 @@ +/* + * 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.rest; + +import org.apache.struts2.ActionSupport; +import org.apache.struts2.interceptor.httpmethod.HttpPost; + +/** State-changing action that declares itself POST-only. */ +@HttpPost +public class PostOnlyAction extends ActionSupport { + public static volatile boolean invoked; + + @Override + public String execute() { + invoked = true; + return NONE; + } +} diff --git a/plugins/rest/src/test/java/org/apache/struts2/rest/RestStackHttpMethodEnforcementTest.java b/plugins/rest/src/test/java/org/apache/struts2/rest/RestStackHttpMethodEnforcementTest.java new file mode 100644 index 0000000000..ebd541741b --- /dev/null +++ b/plugins/rest/src/test/java/org/apache/struts2/rest/RestStackHttpMethodEnforcementTest.java @@ -0,0 +1,199 @@ +/* + * 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.rest; + +import org.apache.struts2.ActionContext; +import org.apache.struts2.ActionProxy; +import org.apache.struts2.ActionSupport; +import org.apache.struts2.StrutsStatics; +import org.apache.struts2.XWorkTestCase; +import org.apache.struts2.config.DefaultPropertiesProvider; +import org.apache.struts2.config.StrutsBeanSelectionProvider; +import org.apache.struts2.config.StrutsXmlConfigurationProvider; +import org.apache.struts2.dispatcher.HttpParameters; +import org.apache.struts2.dispatcher.mapper.ActionMapping; +import org.apache.struts2.interceptor.httpmethod.HttpDelete; +import org.apache.struts2.interceptor.httpmethod.HttpMethod; +import org.apache.struts2.interceptor.httpmethod.HttpMethodAware; +import org.apache.struts2.interceptor.httpmethod.HttpPost; +import org.apache.struts2.interceptor.httpmethod.HttpPut; +import org.springframework.mock.web.MockHttpServletRequest; +import org.springframework.mock.web.MockHttpServletResponse; + +import java.util.HashMap; +import java.util.Map; + +/** The REST default stack enforces HTTP method annotations. */ +public class RestStackHttpMethodEnforcementTest extends XWorkTestCase { + + @Override + protected void setUp() throws Exception { + super.setUp(); + loadConfigurationProviders( + new StrutsXmlConfigurationProvider("struts-default.xml"), + new StrutsXmlConfigurationProvider("struts-plugin.xml"), + new StrutsXmlConfigurationProvider("ww-httpmethod-stack.xml"), + new DefaultPropertiesProvider(), + new StrutsBeanSelectionProvider()); + } + + private MockHttpServletResponse invoke(String namespace, String verb) throws Exception { + PostOnlyAction.invoked = false; + MockHttpServletRequest request = new MockHttpServletRequest(verb, namespace + "/mark"); + MockHttpServletResponse response = new MockHttpServletResponse(); + Map ctx = new HashMap<>(); + ctx.put(StrutsStatics.HTTP_REQUEST, request); + ctx.put(StrutsStatics.HTTP_RESPONSE, response); + ctx.putAll(ActionContext.of(ctx).withParameters(org.apache.struts2.dispatcher.HttpParameters.create(new HashMap()).build()).getContextMap()); + ActionProxy proxy = actionProxyFactory.createActionProxy(namespace, "mark", null, ctx); + proxy.execute(); + return response; + } + + private void assertGetRejected(String namespace) throws Exception { + MockHttpServletResponse response = invoke(namespace, "GET"); + assertEquals(405, response.getStatus()); + assertEquals("POST", response.getHeader("Allow")); + assertFalse("action must not run", PostOnlyAction.invoked); + } + + public void testControlCoreDefaultStackRejectsGetOnPostOnlyAction() throws Exception { + assertGetRejected("/core"); + } + + public void testRestDefaultStackRejectsGetOnPostOnlyAction() throws Exception { + assertGetRejected("/rest"); + } + + public void testRestDefaultStackAllowsPostOnPostOnlyAction() throws Exception { + MockHttpServletResponse response = invoke("/rest", "POST"); + assertEquals(200, response.getStatus()); + assertNull(response.getHeader("Allow")); + assertTrue("action must run", PostOnlyAction.invoked); + } + + public void testActualPutIsAllowedOnPutAnnotatedUpdate() throws Exception { + assertMappedRequest("native", "PUT", null, "update", 200, null, 1); + } + + public void testActualDeleteIsAllowedOnDeleteAnnotatedDestroy() throws Exception { + assertMappedRequest("native", "DELETE", null, "destroy", 200, null, 1); + } + + public void testTunneledPutIsRejectedOnPutAnnotatedUpdate() throws Exception { + assertMappedRequest("native", "POST", "put", "update", 405, "PUT", 0); + } + + public void testTunneledDeleteIsRejectedOnDeleteAnnotatedDestroy() throws Exception { + assertMappedRequest("native", "POST", "delete", "destroy", 405, "DELETE", 0); + } + + public void testTunneledPutIsAllowedOnPostAnnotatedUpdate() throws Exception { + assertMappedRequest("tunneled", "POST", "put", "update", 200, null, 1); + } + + public void testTunneledDeleteIsAllowedOnPostAnnotatedDestroy() throws Exception { + assertMappedRequest("tunneled", "POST", "delete", "destroy", 200, null, 1); + } + + public void testActualPutIsStillAllowedWhenUpdateAlsoAllowsPost() throws Exception { + assertMappedRequest("tunneled", "PUT", null, "update", 200, null, 1); + } + + public void testActualDeleteIsStillAllowedWhenDestroyAlsoAllowsPost() throws Exception { + assertMappedRequest("tunneled", "DELETE", null, "destroy", 200, null, 1); + } + + private void assertMappedRequest(String actionName, String verb, String override, String actionMethod, + int status, String allow, int invocations) throws Exception { + String path = "/rest/" + actionName + "/1"; + MockHttpServletRequest request = new MockHttpServletRequest(verb, path); + request.setServletPath(path); + if (override != null) { + request.setParameter(RestActionMapper.HTTP_METHOD_PARAM, override); + } + RestActionMapper mapper = new RestActionMapper(); + container.inject(mapper); + ActionMapping mapping = mapper.getMapping(request, configurationManager); + assertNotNull(mapping); + assertEquals("/rest", mapping.getNamespace()); + assertEquals(actionName, mapping.getName()); + assertEquals(actionMethod, mapping.getMethod()); + + MockHttpServletResponse response = new MockHttpServletResponse(); + Map context = ActionContext.of() + .withServletRequest(request) + .withServletResponse(response) + .withActionMapping(mapping) + .withParameters(HttpParameters.create(request.getParameterMap()).build()) + .getContextMap(); + ActionProxy proxy = actionProxyFactory.createActionProxy(mapping.getNamespace(), mapping.getName(), + mapping.getMethod(), context, false, true); + NativeVerbAction action = (NativeVerbAction) proxy.getAction(); + proxy.execute(); + + assertEquals(status, response.getStatus()); + assertEquals(allow, response.getHeader("Allow")); + assertEquals(invocations, action.invocations); + assertEquals(HttpMethod.parse(verb), action.method); + } + + public static class NativeVerbAction extends ActionSupport implements HttpMethodAware { + private int invocations; + private HttpMethod method; + + @HttpPut + public String update() { + invocations++; + return NONE; + } + + @HttpDelete + public String destroy() { + invocations++; + return NONE; + } + + @Override + public void setMethod(HttpMethod method) { + this.method = method; + } + + @Override + public String getBadRequestResultName() { + return null; + } + } + + public static class TunneledVerbAction extends NativeVerbAction { + @HttpPost + @HttpPut + @Override + public String update() { + return super.update(); + } + + @HttpPost + @HttpDelete + @Override + public String destroy() { + return super.destroy(); + } + } +} diff --git a/plugins/rest/src/test/resources/ww-httpmethod-stack.xml b/plugins/rest/src/test/resources/ww-httpmethod-stack.xml new file mode 100644 index 0000000000..31672848ef --- /dev/null +++ b/plugins/rest/src/test/resources/ww-httpmethod-stack.xml @@ -0,0 +1,32 @@ + + + + + + + + + + + + +