From fe12d704354cd6a0a68c1f6ba915bd14be059c66 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=B3=95=E2=B2=9B=CF=84=E2=B2=89=E2=B2=85=E2=B2=A5?= =?UTF-8?q?=E2=B2=89=E2=B3=8F=CF=84=E2=B2=9F=E2=B2=85=20=F0=9F=95=B5?= =?UTF-8?q?=F0=9F=8F=BB?= <192411347+g0w6y@users.noreply.github.com> Date: Sat, 7 Nov 2026 17:14:49 +0530 Subject: [PATCH 1/2] WW-5844 Enforce @HttpGet/@HttpPost on the implicit execute() method and add httpMethod to the REST and Bean Validation default stacks HttpMethodInterceptor misses an @Http* annotation in four distinct, independent ways: a method-level annotation on the implicit execute() method (gated by isMethodSpecified(), while the class-level check runs unconditionally), a class- or method-level annotation lost to a subclass that doesn't repeat it (plain Java annotation semantics, none of the Http* annotations is @Inherited), and the same loss one level further out for an annotation declared on an interface reached only through an intermediate superclass. restDefaultStack and beanValidationDefaultStack are the only two bundled interceptor stacks anywhere in core or any plugin that carry params but omit httpMethod entirely. HttpMethodInterceptor now always resolves the invoked method, including the implicit execute() case, and walks the full class/superclass/interface hierarchy for the nearest annotation at both the method and class level, checking every level's own implemented interfaces. httpMethod added to both stacks. A new, generic AccStackInvariantsTest (core) walks every stack definition in struts-default.xml and asserts any stack carrying params also carries httpMethod. Tests: AccHttpMethodTest, AccStackInvariantsTest (new), plus RestStackHttpMethodEnforcementTest and BeanValidationStackHttpMethodEnforcementTest. Full suites: core 3749/0 failures (2 skipped), rest 245/0, bean-validation 25/0, convention 94/0, json 177/0. --- .../httpmethod/HttpMethodInterceptor.java | 95 +++++++++++++++++-- .../apache/struts2/access/AccHttpActions.java | 47 +++++++++ .../struts2/access/AccHttpMethodTest.java | 34 +++++++ .../access/AccStackInvariantsTest.java | 64 +++++++++++++ core/src/test/resources/struts-acc-http.xml | 15 +++ plugins/bean-validation/pom.xml | 10 ++ .../src/main/resources/struts-plugin.xml | 1 + ...idationStackHttpMethodEnforcementTest.java | 74 +++++++++++++++ .../struts/beanvalidation/PostOnlyAction.java | 35 +++++++ .../test/resources/ww-httpmethod-stack.xml | 12 +++ .../rest/src/main/resources/struts-plugin.xml | 1 + .../apache/struts2/rest/PostOnlyAction.java | 16 ++++ .../RestStackHttpMethodEnforcementTest.java | 55 +++++++++++ .../test/resources/ww-httpmethod-stack.xml | 12 +++ 14 files changed, 465 insertions(+), 6 deletions(-) create mode 100644 core/src/test/java/org/apache/struts2/access/AccHttpActions.java create mode 100644 core/src/test/java/org/apache/struts2/access/AccHttpMethodTest.java create mode 100644 core/src/test/java/org/apache/struts2/access/AccStackInvariantsTest.java create mode 100644 core/src/test/resources/struts-acc-http.xml create mode 100644 plugins/bean-validation/src/test/java/org/apache/struts/beanvalidation/BeanValidationStackHttpMethodEnforcementTest.java create mode 100644 plugins/bean-validation/src/test/java/org/apache/struts/beanvalidation/PostOnlyAction.java create mode 100644 plugins/bean-validation/src/test/resources/ww-httpmethod-stack.xml create mode 100644 plugins/rest/src/test/java/org/apache/struts2/rest/PostOnlyAction.java create mode 100644 plugins/rest/src/test/java/org/apache/struts2/rest/RestStackHttpMethodEnforcementTest.java create mode 100644 plugins/rest/src/test/resources/ww-httpmethod-stack.xml 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..9ef0c0bd0c 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 @@ -97,18 +97,26 @@ 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)) { + // Resolve the method that is actually about to be invoked - including the implicit "execute" that + // isMethodSpecified() reports as "unspecified": that flag only means no method name was configured + // for this action mapping, it does not mean execute() is some special, unguardable method. Skipping + // annotation lookup for it (as this interceptor used to) left every action that relies on the default + // execute() mapping (no method= attribute, no Convention @Action(method=...)) unguardable by + // @HttpPost/@HttpGet/etc, silently. + Method method = resolveInvokedMethod(action.getClass(), invocation.getProxy().getMethod()); + 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 +141,81 @@ protected String doIntercept(ActionInvocation invocation, AnnotatedElement eleme return checkAllowedMethods(invocation, allowedMethods); } + /** + * Resolves the {@link Method} the invocation is about to call, including when {@code methodName} is the + * implicit default ("execute") that an action class inherits rather than declares itself. Returns {@code + * null} (instead of propagating {@link NoSuchMethodException}) when the action has no such public method + * at all - that is not this interceptor's problem to raise, the normal invocation path will deal with it. + */ + private Method resolveInvokedMethod(Class actionClass, String methodName) { + if (methodName == null || methodName.isEmpty()) { + return null; + } + try { + return actionClass.getMethod(methodName); + } catch (NoSuchMethodException e) { + return null; + } + } + + /** + * {@code @HttpGet}/{@code @HttpPost}/etc are not {@code @Inherited}, so a subclass that overrides an + * annotated method - or merely extends a base action without repeating the annotation - would otherwise + * silently lose the restriction. This walks the type hierarchy (every superclass, and every interface + * implemented at each of those levels, not just the leaf class's own) looking for the nearest declaration + * of the same method signature that still carries one of the annotations, mirroring the superclass-walk + * {@link AnnotationUtils#findAnnotation(Class, Class)} already performs for class-level lookups (e.g. + * {@code @ParentPackage}, {@code @InterceptorRefs} in the convention plugin) - the same "closest wins, + * never widen" rule, applied per-method instead of per-class. {@code Class#getInterfaces()} only returns + * interfaces a class declares itself, not ones a superclass further up implements, so the interface check + * has to run at every level of the walk, not once on the leaf class. + */ + private AnnotatedElement findNearestAnnotatedOverride(Class actionClass, Method method) { + for (Class type = actionClass; type != null; type = type.getSuperclass()) { + AnnotatedElement found = declaredOverrideAnnotatedIn(type, method); + if (found != null) { + return found; + } + for (Class iface : type.getInterfaces()) { + 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 AnnotationUtils.isAnnotatedBy(candidate, HTTP_METHOD_ANNOTATIONS) ? candidate : null; + } catch (NoSuchMethodException e) { + return null; + } + } + + /** + * Same "closest declaration up the hierarchy wins" rule as {@link #findNearestAnnotatedOverride}, for a + * class-level annotation (e.g. {@code @HttpPost} on the action class itself, or on an interface it + * implements, rather than on one method) that a subclass of the annotated action would otherwise silently + * lose. Checks each superclass level's own directly-implemented interfaces too, for the same reason + * {@link #findNearestAnnotatedOverride} does. + */ + 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 : type.getInterfaces()) { + if (AnnotationUtils.isAnnotatedBy(iface, HTTP_METHOD_ANNOTATIONS)) { + return iface; + } + } + } + return null; + } + 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/access/AccHttpActions.java b/core/src/test/java/org/apache/struts2/access/AccHttpActions.java new file mode 100644 index 0000000000..9788e39735 --- /dev/null +++ b/core/src/test/java/org/apache/struts2/access/AccHttpActions.java @@ -0,0 +1,47 @@ +package org.apache.struts2.access; + +import org.apache.struts2.ActionSupport; +import org.apache.struts2.interceptor.httpmethod.HttpPost; + +public class AccHttpActions { + 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() { AccHttpActions.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/access/AccHttpMethodTest.java b/core/src/test/java/org/apache/struts2/access/AccHttpMethodTest.java new file mode 100644 index 0000000000..291d85b35f --- /dev/null +++ b/core/src/test/java/org/apache/struts2/access/AccHttpMethodTest.java @@ -0,0 +1,34 @@ +package org.apache.struts2.access; + +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 AccHttpMethodTest extends StrutsInternalTestCase { + + private int run(String action, String verb) throws Exception { + loadConfigurationProviders(new StrutsXmlConfigurationProvider("struts-acc-http.xml")); + MockHttpServletResponse res = new MockHttpServletResponse(); + Map ctx = ActionContext.of() + .withServletRequest(new MockHttpServletRequest(verb, "/" + action)) + .withServletResponse(res).getContextMap(); + AccHttpActions.RAN.set(0); + ActionProxy p = actionProxyFactory.createActionProxy("", action, null, ctx, false, true); + p.execute(); + return AccHttpActions.RAN.get(); + } + + public void testControlNamedMethodGetRejected() throws Exception { assertEquals(0, run("NamedPost", "GET")); } + public void testControlNamedMethodPostAllowed() throws Exception { assertEquals(1, run("NamedPost", "POST")); } + public void testControlClassLevelGetRejected() throws Exception { assertEquals(0, run("ClassPost", "GET")); } + public void testExecuteMethodAnnotationGetRejected() throws Exception { assertEquals("@HttpPost on execute() ignored", 0, run("ExecutePost", "GET")); } + public void testSubclassOfClassLevelGetRejected() throws Exception { assertEquals("class-level @HttpPost not inherited", 0, run("SubOfClassPost", "GET")); } + public void testOverrideDropsMethodAnnotation() throws Exception { assertEquals("override drops @HttpPost", 0, run("SubOverride", "GET")); } + public void testSubclassOfInterfaceDefaultMethodGetRejected() throws Exception { assertEquals("interface default-method @HttpPost, two levels down, not found", 0, run("SubOfImplementsMethodMarker", "GET")); } + public void testSubclassOfImplementedInterfaceClassLevelGetRejected() throws Exception { assertEquals("interface class-level @HttpPost, two levels down, not found", 0, run("SubOfImplementsClassMarker", "GET")); } +} diff --git a/core/src/test/java/org/apache/struts2/access/AccStackInvariantsTest.java b/core/src/test/java/org/apache/struts2/access/AccStackInvariantsTest.java new file mode 100644 index 0000000000..76ba3edbde --- /dev/null +++ b/core/src/test/java/org/apache/struts2/access/AccStackInvariantsTest.java @@ -0,0 +1,64 @@ +package org.apache.struts2.access; + +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; +import java.util.Set; + +/** + * Guard test for the "bundled stack omits httpMethod" class (WW-5718 only resynced coep/coop/fetchMetadata; + * httpMethod and csp were left out of restDefaultStack/beanValidationDefaultStack - see ~/hunt/reports2/plugins2.md + * F1 and ~/hunt/reports2/deep-httpmethod.md). Walks every interceptor-stack bundled in core's struts-default.xml + * (InterceptorStackConfig.getInterceptors() is already flattened by InterceptorBuilder, so stack-of-stack refs + * like validationWorkflowStack -> basicStack are resolved to the atomic interceptor list) and asserts: any stack + * that performs request-parameter binding (contains "params") also enforces the HTTP-method guard (contains + * "httpMethod"), UNLESS the stack is explicitly allowlisted as an intentionally bare building block that is never + * an application's own primary request stack (e.g. emptyStack: no params, so not even reachable; nothing in core + * is currently allowlisted - the invariant holds for every core stack today). + *

+ * This cannot see plugin struts-plugin.xml files (core has no dependency on the plugins), so restDefaultStack and + * beanValidationDefaultStack need their own per-plugin analogue - see + * plugins/rest/.../RestStackHttpMethodEnforcementTest and + * plugins/bean-validation/.../BeanValidationStackHttpMethodEnforcementTest. + */ +public class AccStackInvariantsTest extends StrutsInternalTestCase { + + /** Stacks that are deliberately bare (never bound to a package's default-interceptor-ref as its primary + * request stack; building blocks or dev-tool stacks only) are allowlisted here, by design, not by accident. */ + private static final Set ALLOWED_BARE = Set.of(); + + 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(); + if (ALLOWED_BARE.contains(name)) { + continue; + } + 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/resources/struts-acc-http.xml b/core/src/test/resources/struts-acc-http.xml new file mode 100644 index 0000000000..dc018de338 --- /dev/null +++ b/core/src/test/resources/struts-acc-http.xml @@ -0,0 +1,15 @@ + + + + + + + 204 + 204 + 204 + 204 + 204 + 204 + 204 + + 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..d4657d7e57 --- /dev/null +++ b/plugins/bean-validation/src/test/java/org/apache/struts/beanvalidation/BeanValidationStackHttpMethodEnforcementTest.java @@ -0,0 +1,74 @@ +/* + * 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; + +/** + * Plugin-side analogue of plugins/rest's RestStackHttpMethodEnforcementTest. An action annotated + * {@code @HttpPost} must not run for a GET. Core's defaultStack carries the httpMethod interceptor; + * beanValidationDefaultStack (forked from it, WW-5718) did not - see ~/hunt/reports2/deep-httpmethod.md. + */ +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 int getAndReportStatus(String namespace) throws Exception { + PostOnlyAction.invoked = false; + MockHttpServletRequest request = new MockHttpServletRequest("GET", 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.getStatus(); + } + + public void testControlCoreDefaultStackRejectsGetOnPostOnlyAction() throws Exception { + assertEquals(405, getAndReportStatus("/core")); + assertFalse("action must not run", PostOnlyAction.invoked); + } + + public void testBeanValidationDefaultStackRejectsGetOnPostOnlyAction() throws Exception { + getAndReportStatus("/bv"); + assertFalse("@HttpPost action ran for GET under beanValidationDefaultStack", 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..cab4b908be --- /dev/null +++ b/plugins/bean-validation/src/test/java/org/apache/struts/beanvalidation/PostOnlyAction.java @@ -0,0 +1,35 @@ +/* + * 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; + +/** State-changing action that declares itself POST-only via the implicit execute() method - the + * shape most showcase/example actions actually use (no method= override). */ +@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..72ac136fda --- /dev/null +++ b/plugins/bean-validation/src/test/resources/ww-httpmethod-stack.xml @@ -0,0 +1,12 @@ + + + + + + + + + + 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..4bf3e0a718 --- /dev/null +++ b/plugins/rest/src/test/java/org/apache/struts2/rest/PostOnlyAction.java @@ -0,0 +1,16 @@ +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..40d6aa52d6 --- /dev/null +++ b/plugins/rest/src/test/java/org/apache/struts2/rest/RestStackHttpMethodEnforcementTest.java @@ -0,0 +1,55 @@ +package org.apache.struts2.rest; + +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; + +/** + * An action annotated {@code @HttpPost} must not run for a GET. The core defaultStack carries the + * httpMethod interceptor; restDefaultStack (forked from it) does not, so the annotation is ignored. + */ +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 int getAndReportStatus(String namespace) throws Exception { + PostOnlyAction.invoked = false; + MockHttpServletRequest request = new MockHttpServletRequest("GET", 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.getStatus(); + } + + public void testControlCoreDefaultStackRejectsGetOnPostOnlyAction() throws Exception { + assertEquals(405, getAndReportStatus("/core")); + assertFalse("action must not run", PostOnlyAction.invoked); + } + + public void testRestDefaultStackRejectsGetOnPostOnlyAction() throws Exception { + getAndReportStatus("/rest"); + assertFalse("@HttpPost action ran for GET under restDefaultStack", PostOnlyAction.invoked); + } +} 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..39f0a92c45 --- /dev/null +++ b/plugins/rest/src/test/resources/ww-httpmethod-stack.xml @@ -0,0 +1,12 @@ + + + + + + + + + + From b0d9723c3ed6688d0b57e0b74a1971fba59f3471 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=B3=95=E2=B2=9B=CF=84=E2=B2=89=E2=B2=85=E2=B2=A5?= =?UTF-8?q?=E2=B2=89=E2=B3=8F=CF=84=E2=B2=9F=E2=B2=85=20=F0=9F=95=B5?= =?UTF-8?q?=F0=9F=8F=BB?= <192411347+g0w6y@users.noreply.github.com> Date: Sun, 8 Nov 2026 12:07:57 +0530 Subject: [PATCH 2/2] WW-5844 fix(core)!: address HTTP method guard review Resolve parent interfaces transitively, ignore unrelated non-public and static method declarations, and preserve annotations on the resolved instance method. Keep explicit missing-method errors while allowing normal implicit invocation fallback after class and XML method restrictions. Move and license the test fixtures, assert 405/Allow and accepted controls, and cover actual REST mapper routing with native and tunneled requests. Document that REST restrictions check the transport method. BREAKING CHANGE: inherited HTTP method annotations and implicit execute() annotations are now enforced. REST and Bean Validation default stacks now enforce HTTP method restrictions. POST-based REST forms must allow POST in handler annotations and any XML http-method restriction. --- .../httpmethod/HttpMethodInterceptor.java | 78 +++---- .../struts2/access/AccHttpMethodTest.java | 34 --- .../access/AccStackInvariantsTest.java | 64 ------ .../HttpMethodDefaultStacksTest.java | 59 +++++ .../HttpMethodHierarchyActions.java} | 24 ++- .../httpmethod/HttpMethodHierarchyTest.java | 88 ++++++++ .../httpmethod/HttpMethodInterceptorTest.java | 201 ++++++++++++++++++ core/src/test/resources/struts-acc-http.xml | 15 -- .../struts-http-method-hierarchy.xml | 33 +++ ...idationStackHttpMethodEnforcementTest.java | 32 +-- .../struts/beanvalidation/PostOnlyAction.java | 3 +- .../test/resources/ww-httpmethod-stack.xml | 18 ++ .../apache/struts2/rest/RestActionMapper.java | 7 + .../apache/struts2/rest/PostOnlyAction.java | 18 ++ .../RestStackHttpMethodEnforcementTest.java | 166 ++++++++++++++- .../test/resources/ww-httpmethod-stack.xml | 20 ++ 16 files changed, 682 insertions(+), 178 deletions(-) delete mode 100644 core/src/test/java/org/apache/struts2/access/AccHttpMethodTest.java delete mode 100644 core/src/test/java/org/apache/struts2/access/AccStackInvariantsTest.java create mode 100644 core/src/test/java/org/apache/struts2/interceptor/httpmethod/HttpMethodDefaultStacksTest.java rename core/src/test/java/org/apache/struts2/{access/AccHttpActions.java => interceptor/httpmethod/HttpMethodHierarchyActions.java} (62%) create mode 100644 core/src/test/java/org/apache/struts2/interceptor/httpmethod/HttpMethodHierarchyTest.java delete mode 100644 core/src/test/resources/struts-acc-http.xml create mode 100644 core/src/test/resources/struts-http-method-hierarchy.xml 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 9ef0c0bd0c..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,13 +105,7 @@ public String intercept(ActionInvocation invocation) throws Exception { ((HttpMethodAware) (action)).setMethod(HttpMethod.parse(request.getMethod())); } - // Resolve the method that is actually about to be invoked - including the implicit "execute" that - // isMethodSpecified() reports as "unspecified": that flag only means no method name was configured - // for this action mapping, it does not mean execute() is some special, unguardable method. Skipping - // annotation lookup for it (as this interceptor used to) left every action that relies on the default - // execute() mapping (no method= attribute, no Convention @Action(method=...)) unguardable by - // @HttpPost/@HttpGet/etc, silently. - Method method = resolveInvokedMethod(action.getClass(), invocation.getProxy().getMethod()); + Method method = resolveInvokedMethod(invocation); if (method != null) { AnnotatedElement methodElement = findNearestAnnotatedOverride(action.getClass(), method); if (methodElement != null) { @@ -141,42 +143,36 @@ protected String doIntercept(ActionInvocation invocation, AnnotatedElement eleme return checkAllowedMethods(invocation, allowedMethods); } - /** - * Resolves the {@link Method} the invocation is about to call, including when {@code methodName} is the - * implicit default ("execute") that an action class inherits rather than declares itself. Returns {@code - * null} (instead of propagating {@link NoSuchMethodException}) when the action has no such public method - * at all - that is not this interceptor's problem to raise, the normal invocation path will deal with it. - */ - private Method resolveInvokedMethod(Class actionClass, String methodName) { + private Method resolveInvokedMethod(ActionInvocation invocation) throws NoSuchMethodException { + String methodName = invocation.getProxy().getMethod(); if (methodName == null || methodName.isEmpty()) { return null; } try { - return actionClass.getMethod(methodName); + 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; } } - /** - * {@code @HttpGet}/{@code @HttpPost}/etc are not {@code @Inherited}, so a subclass that overrides an - * annotated method - or merely extends a base action without repeating the annotation - would otherwise - * silently lose the restriction. This walks the type hierarchy (every superclass, and every interface - * implemented at each of those levels, not just the leaf class's own) looking for the nearest declaration - * of the same method signature that still carries one of the annotations, mirroring the superclass-walk - * {@link AnnotationUtils#findAnnotation(Class, Class)} already performs for class-level lookups (e.g. - * {@code @ParentPackage}, {@code @InterceptorRefs} in the convention plugin) - the same "closest wins, - * never widen" rule, applied per-method instead of per-class. {@code Class#getInterfaces()} only returns - * interfaces a class declares itself, not ones a superclass further up implements, so the interface check - * has to run at every level of the walk, not once on the leaf class. - */ + /** 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 : type.getInterfaces()) { + for (Class iface : interfacesAtLevel(type)) { found = declaredOverrideAnnotatedIn(iface, method); if (found != null) { return found; @@ -189,25 +185,24 @@ private AnnotatedElement findNearestAnnotatedOverride(Class actionClass, Meth private AnnotatedElement declaredOverrideAnnotatedIn(Class type, Method method) { try { Method candidate = type.getDeclaredMethod(method.getName(), method.getParameterTypes()); - return AnnotationUtils.isAnnotatedBy(candidate, HTTP_METHOD_ANNOTATIONS) ? candidate : null; + return isPublicInstanceMethod(candidate) && AnnotationUtils.isAnnotatedBy(candidate, HTTP_METHOD_ANNOTATIONS) + ? candidate : null; } catch (NoSuchMethodException e) { return null; } } - /** - * Same "closest declaration up the hierarchy wins" rule as {@link #findNearestAnnotatedOverride}, for a - * class-level annotation (e.g. {@code @HttpPost} on the action class itself, or on an interface it - * implements, rather than on one method) that a subclass of the annotated action would otherwise silently - * lose. Checks each superclass level's own directly-implemented interfaces too, for the same reason - * {@link #findNearestAnnotatedOverride} does. - */ + 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 : type.getInterfaces()) { + for (Class iface : interfacesAtLevel(type)) { if (AnnotationUtils.isAnnotatedBy(iface, HTTP_METHOD_ANNOTATIONS)) { return iface; } @@ -216,6 +211,15 @@ private AnnotatedElement findNearestAnnotatedType(Class actionClass) { 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/access/AccHttpMethodTest.java b/core/src/test/java/org/apache/struts2/access/AccHttpMethodTest.java deleted file mode 100644 index 291d85b35f..0000000000 --- a/core/src/test/java/org/apache/struts2/access/AccHttpMethodTest.java +++ /dev/null @@ -1,34 +0,0 @@ -package org.apache.struts2.access; - -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 AccHttpMethodTest extends StrutsInternalTestCase { - - private int run(String action, String verb) throws Exception { - loadConfigurationProviders(new StrutsXmlConfigurationProvider("struts-acc-http.xml")); - MockHttpServletResponse res = new MockHttpServletResponse(); - Map ctx = ActionContext.of() - .withServletRequest(new MockHttpServletRequest(verb, "/" + action)) - .withServletResponse(res).getContextMap(); - AccHttpActions.RAN.set(0); - ActionProxy p = actionProxyFactory.createActionProxy("", action, null, ctx, false, true); - p.execute(); - return AccHttpActions.RAN.get(); - } - - public void testControlNamedMethodGetRejected() throws Exception { assertEquals(0, run("NamedPost", "GET")); } - public void testControlNamedMethodPostAllowed() throws Exception { assertEquals(1, run("NamedPost", "POST")); } - public void testControlClassLevelGetRejected() throws Exception { assertEquals(0, run("ClassPost", "GET")); } - public void testExecuteMethodAnnotationGetRejected() throws Exception { assertEquals("@HttpPost on execute() ignored", 0, run("ExecutePost", "GET")); } - public void testSubclassOfClassLevelGetRejected() throws Exception { assertEquals("class-level @HttpPost not inherited", 0, run("SubOfClassPost", "GET")); } - public void testOverrideDropsMethodAnnotation() throws Exception { assertEquals("override drops @HttpPost", 0, run("SubOverride", "GET")); } - public void testSubclassOfInterfaceDefaultMethodGetRejected() throws Exception { assertEquals("interface default-method @HttpPost, two levels down, not found", 0, run("SubOfImplementsMethodMarker", "GET")); } - public void testSubclassOfImplementedInterfaceClassLevelGetRejected() throws Exception { assertEquals("interface class-level @HttpPost, two levels down, not found", 0, run("SubOfImplementsClassMarker", "GET")); } -} diff --git a/core/src/test/java/org/apache/struts2/access/AccStackInvariantsTest.java b/core/src/test/java/org/apache/struts2/access/AccStackInvariantsTest.java deleted file mode 100644 index 76ba3edbde..0000000000 --- a/core/src/test/java/org/apache/struts2/access/AccStackInvariantsTest.java +++ /dev/null @@ -1,64 +0,0 @@ -package org.apache.struts2.access; - -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; -import java.util.Set; - -/** - * Guard test for the "bundled stack omits httpMethod" class (WW-5718 only resynced coep/coop/fetchMetadata; - * httpMethod and csp were left out of restDefaultStack/beanValidationDefaultStack - see ~/hunt/reports2/plugins2.md - * F1 and ~/hunt/reports2/deep-httpmethod.md). Walks every interceptor-stack bundled in core's struts-default.xml - * (InterceptorStackConfig.getInterceptors() is already flattened by InterceptorBuilder, so stack-of-stack refs - * like validationWorkflowStack -> basicStack are resolved to the atomic interceptor list) and asserts: any stack - * that performs request-parameter binding (contains "params") also enforces the HTTP-method guard (contains - * "httpMethod"), UNLESS the stack is explicitly allowlisted as an intentionally bare building block that is never - * an application's own primary request stack (e.g. emptyStack: no params, so not even reachable; nothing in core - * is currently allowlisted - the invariant holds for every core stack today). - *

- * This cannot see plugin struts-plugin.xml files (core has no dependency on the plugins), so restDefaultStack and - * beanValidationDefaultStack need their own per-plugin analogue - see - * plugins/rest/.../RestStackHttpMethodEnforcementTest and - * plugins/bean-validation/.../BeanValidationStackHttpMethodEnforcementTest. - */ -public class AccStackInvariantsTest extends StrutsInternalTestCase { - - /** Stacks that are deliberately bare (never bound to a package's default-interceptor-ref as its primary - * request stack; building blocks or dev-tool stacks only) are allowlisted here, by design, not by accident. */ - private static final Set ALLOWED_BARE = Set.of(); - - 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(); - if (ALLOWED_BARE.contains(name)) { - continue; - } - 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/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/access/AccHttpActions.java b/core/src/test/java/org/apache/struts2/interceptor/httpmethod/HttpMethodHierarchyActions.java similarity index 62% rename from core/src/test/java/org/apache/struts2/access/AccHttpActions.java rename to core/src/test/java/org/apache/struts2/interceptor/httpmethod/HttpMethodHierarchyActions.java index 9788e39735..49e1096afa 100644 --- a/core/src/test/java/org/apache/struts2/access/AccHttpActions.java +++ b/core/src/test/java/org/apache/struts2/interceptor/httpmethod/HttpMethodHierarchyActions.java @@ -1,9 +1,27 @@ -package org.apache.struts2.access; +/* + * 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 AccHttpActions { +public class HttpMethodHierarchyActions { public static final java.util.concurrent.atomic.AtomicInteger RAN = new java.util.concurrent.atomic.AtomicInteger(); public static class ExecutePost extends ActionSupport { @@ -32,7 +50,7 @@ public static class SubOverride extends BaseMethodPost { public interface MethodPostMarker { @HttpPost - default String save() { AccHttpActions.RAN.incrementAndGet(); return ActionSupport.SUCCESS; } + default String save() { HttpMethodHierarchyActions.RAN.incrementAndGet(); return ActionSupport.SUCCESS; } } public static class ImplementsMethodMarker extends ActionSupport implements MethodPostMarker { } public static class SubOfImplementsMethodMarker extends ImplementsMethodMarker { } 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-acc-http.xml b/core/src/test/resources/struts-acc-http.xml deleted file mode 100644 index dc018de338..0000000000 --- a/core/src/test/resources/struts-acc-http.xml +++ /dev/null @@ -1,15 +0,0 @@ - - - - - - - 204 - 204 - 204 - 204 - 204 - 204 - 204 - - 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/src/test/java/org/apache/struts/beanvalidation/BeanValidationStackHttpMethodEnforcementTest.java b/plugins/bean-validation/src/test/java/org/apache/struts/beanvalidation/BeanValidationStackHttpMethodEnforcementTest.java index d4657d7e57..9ca391f9e0 100644 --- 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 @@ -31,11 +31,7 @@ import java.util.HashMap; import java.util.Map; -/** - * Plugin-side analogue of plugins/rest's RestStackHttpMethodEnforcementTest. An action annotated - * {@code @HttpPost} must not run for a GET. Core's defaultStack carries the httpMethod interceptor; - * beanValidationDefaultStack (forked from it, WW-5718) did not - see ~/hunt/reports2/deep-httpmethod.md. - */ +/** The Bean Validation default stack enforces HTTP method annotations. */ public class BeanValidationStackHttpMethodEnforcementTest extends XWorkTestCase { @Override @@ -49,9 +45,9 @@ protected void setUp() throws Exception { new StrutsBeanSelectionProvider()); } - private int getAndReportStatus(String namespace) throws Exception { + private MockHttpServletResponse invoke(String namespace, String verb) throws Exception { PostOnlyAction.invoked = false; - MockHttpServletRequest request = new MockHttpServletRequest("GET", namespace + "/mark"); + MockHttpServletRequest request = new MockHttpServletRequest(verb, namespace + "/mark"); MockHttpServletResponse response = new MockHttpServletResponse(); Map ctx = new HashMap<>(); ctx.put(StrutsStatics.HTTP_REQUEST, request); @@ -59,16 +55,28 @@ private int getAndReportStatus(String namespace) throws Exception { 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.getStatus(); + return response; } - public void testControlCoreDefaultStackRejectsGetOnPostOnlyAction() throws Exception { - assertEquals(405, getAndReportStatus("/core")); + 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 { - getAndReportStatus("/bv"); - assertFalse("@HttpPost action ran for GET under beanValidationDefaultStack", PostOnlyAction.invoked); + 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 index cab4b908be..91826f9f68 100644 --- 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 @@ -21,8 +21,7 @@ import org.apache.struts2.ActionSupport; import org.apache.struts2.interceptor.httpmethod.HttpPost; -/** State-changing action that declares itself POST-only via the implicit execute() method - the - * shape most showcase/example actions actually use (no method= override). */ +/** Action that declares itself POST-only. */ @HttpPost public class PostOnlyAction extends ActionSupport { public static volatile boolean invoked; diff --git a/plugins/bean-validation/src/test/resources/ww-httpmethod-stack.xml b/plugins/bean-validation/src/test/resources/ww-httpmethod-stack.xml index 72ac136fda..3898b9dda1 100644 --- a/plugins/bean-validation/src/test/resources/ww-httpmethod-stack.xml +++ b/plugins/bean-validation/src/test/resources/ww-httpmethod-stack.xml @@ -1,4 +1,22 @@ + 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/test/java/org/apache/struts2/rest/PostOnlyAction.java b/plugins/rest/src/test/java/org/apache/struts2/rest/PostOnlyAction.java index 4bf3e0a718..dca1a48d05 100644 --- a/plugins/rest/src/test/java/org/apache/struts2/rest/PostOnlyAction.java +++ b/plugins/rest/src/test/java/org/apache/struts2/rest/PostOnlyAction.java @@ -1,3 +1,21 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ package org.apache.struts2.rest; import org.apache.struts2.ActionSupport; 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 index 40d6aa52d6..ebd541741b 100644 --- a/plugins/rest/src/test/java/org/apache/struts2/rest/RestStackHttpMethodEnforcementTest.java +++ b/plugins/rest/src/test/java/org/apache/struts2/rest/RestStackHttpMethodEnforcementTest.java @@ -1,22 +1,45 @@ +/* + * 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; -/** - * An action annotated {@code @HttpPost} must not run for a GET. The core defaultStack carries the - * httpMethod interceptor; restDefaultStack (forked from it) does not, so the annotation is ignored. - */ +/** The REST default stack enforces HTTP method annotations. */ public class RestStackHttpMethodEnforcementTest extends XWorkTestCase { @Override @@ -30,9 +53,9 @@ protected void setUp() throws Exception { new StrutsBeanSelectionProvider()); } - private int getAndReportStatus(String namespace) throws Exception { + private MockHttpServletResponse invoke(String namespace, String verb) throws Exception { PostOnlyAction.invoked = false; - MockHttpServletRequest request = new MockHttpServletRequest("GET", namespace + "/mark"); + MockHttpServletRequest request = new MockHttpServletRequest(verb, namespace + "/mark"); MockHttpServletResponse response = new MockHttpServletResponse(); Map ctx = new HashMap<>(); ctx.put(StrutsStatics.HTTP_REQUEST, request); @@ -40,16 +63,137 @@ private int getAndReportStatus(String namespace) throws Exception { 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.getStatus(); + return response; } - public void testControlCoreDefaultStackRejectsGetOnPostOnlyAction() throws Exception { - assertEquals(405, getAndReportStatus("/core")); + 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 { - getAndReportStatus("/rest"); - assertFalse("@HttpPost action ran for GET under restDefaultStack", PostOnlyAction.invoked); + 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 index 39f0a92c45..31672848ef 100644 --- a/plugins/rest/src/test/resources/ww-httpmethod-stack.xml +++ b/plugins/rest/src/test/resources/ww-httpmethod-stack.xml @@ -1,10 +1,30 @@ + + +