WW-5844 fix(core)!: enforce HTTP method annotations across action hierarchies and plugin stacks by g0w6y · Pull Request #2120 · apache/struts

g0w6y · 2026-11-07T11:45:23Z

Checks method annotations on implicit execute() and follows annotated class, superclass and interface declarations, including parent interfaces. An annotation on the resolved public instance method takes precedence. Unrelated private, package-private and static declarations do not impose restrictions.

Adds httpMethod to restDefaultStack and beanValidationDefaultStack. Rejection tests check 405, Allow and that the action did not run, with accepted-request controls. Missing explicitly configured methods still throw NoSuchMethodException; an absent implicit execute() continues through normal invocation after class/XML restrictions are checked.

Compatibility: inherited annotations and annotations on implicit execute() now apply where they were previously ignored. The two plugin stacks also begin enforcing annotations and XML http-method restrictions. Review base-class and interface annotations and declare the intended methods explicitly on overrides where needed.

REST restrictions use the actual HTTP request method. _method=put/delete changes routing only: @HttpPut/@HttpDelete alone require real PUT/DELETE requests. For POST forms, add @HttpPost on update()/destroy() (alongside the native annotation if both transports are supported), and include POST in any XML http-method restriction. Mapper-integrated tests cover native requests, rejected POST tunnelling and the POST-enabled controls.

Validation: fresh reactor run passed: core 3761 tests (2 skipped), REST 254, Bean Validation 26, Convention 94 and JSON 177, with no failures or errors. RAT passed for the affected modules and their parent modules. The 49 focused core tests produce 9 assertion failures against the original PR interceptor and all pass with this revision.

@http

…nd 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.

lukaszlenart

Thanks for the patch and the thorough test set. I applied the whole diff locally: all new tests pass, the existing HttpMethodInterceptorTest (29) and the WW-5718 stack pins stay green, and both plugin stacks return 405 for a rejected GET. The core fix is right. What blocks the merge is below, with the details inline.

Blocking

Nine new files have no ASF license header, which is why the RAT check fails and the build is red: the three Acc* test classes, struts-acc-http.xml, the REST plugin's PostOnlyAction, RestStackHttpMethodEnforcementTest, and both ww-httpmethod-stack.xml files. The Bean Validation test classes have the header, so that is the form to copy.
Two test Javadocs reference files on your machine (~/hunt/reports2/...). Please remove them; anything worth keeping from those notes belongs in the PR body.
The new org.apache.struts2.access package and the Acc* names do not fit the tree. Please move the tests next to the interceptor under org.apache.struts2.interceptor.httpmethod and name them by what they check (for example a hierarchy test and a default-stacks test).

Scope and behaviour

The hierarchy walk and the two stack additions change behaviour for existing applications beyond the implicit execute() case in the title: an annotation on a base class or an overridden method now applies to subclasses that were served before, and applications on rest-default or struts-bean-validation start enforcing annotations they currently ignore. That needs a fix(core)!: marker and a short migration note in the PR body, or those parts split into their own PR. The docs will also need updating, since the action-configuration page lists exactly the three core stacks as enforcing.
Adding httpMethod to restDefaultStack meets the REST mapper's form tunnelling, see the inline comment on struts-plugin.xml.

Smaller

Comments: the six-line history block in intercept() and the three Javadocs that are longer than the methods they describe should shrink to a line each; the commit message already carries the reasoning.
The plugin enforcement tests assert 405 for the core control but only "not invoked" for the plugin stack. 405 holds in both, so assert it symmetrically.
Please shorten the PR title to the repo form, for example WW-5844 fix(core): enforce method-level HTTP method annotations on the implicit execute(). It becomes the squash commit headline.

lukaszlenart · 2026-11-07T20:47:29Z

-        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


This block narrates the previous behaviour. One line at most, or nothing: the test and the commit message carry the history.

lukaszlenart · 2026-11-07T20:47:29Z

+        }
+        try {
+            return actionClass.getMethod(methodName);
+        } catch (NoSuchMethodException e) {


Swallowing NoSuchMethodException changes what a typo in method= looks like: before, the interceptor threw and the configuration error surfaced on the first request; now a GET on a class-annotated action gets a 405 and a POST fails later inside the invocation with a less specific error. Either rethrow here, or state in the PR why falling through is wanted and add a test for it.

lukaszlenart · 2026-11-07T20:47:29Z

+            if (found != null) {
+                return found;
+            }
+            for (Class<?> iface : type.getInterfaces()) {


getInterfaces() gives the interfaces this type declares, not their super-interfaces. An annotation on interface PostOnly { @HttpPost default String save() } reached through interface Audited extends PostOnly is not found. Either walk the transitive set (core has AnnotationUtils.addAllInterfaces / ClassUtils.getAllInterfaces) or drop the "full hierarchy" claim from the Javadoc and the PR body.

lukaszlenart · 2026-11-07T20:47:29Z

+
+    private AnnotatedElement declaredOverrideAnnotatedIn(Class<?> type, Method method) {
+        try {
+            Method candidate = type.getDeclaredMethod(method.getName(), method.getParameterTypes());


getDeclaredMethod also returns private, static and package-private methods, which are not overrides of the public action method. A base class with a private helper save() carrying an annotation would impose it on the subclass's public save(). Filter the candidate to public and non-static, which is what getMethod matched before.

lukaszlenart · 2026-11-07T20:47:29Z

                <interceptor-ref name="exception"/>
                <interceptor-ref name="alias"/>
                <interceptor-ref name="servletConfig"/>
+                <interceptor-ref name="httpMethod"/>


The REST mapper treats a POST carrying _method=put or _method=delete as PUT or DELETE and routes it to update/destroy (RestActionMapper.isPut/isDelete). This interceptor compares the raw request method, so once it runs in this stack, @HttpPut on update() or @HttpDelete on destroy() rejects every browser form that uses the plugin's own tunnelling with a 405. Today nothing breaks, because the annotations are ignored here, but the new enforcement is unusable with the documented form shape. Either honour the same override the mapper honours, or document that these annotations only work with real PUT/DELETE requests under rest-default.

lukaszlenart · 2026-11-07T20:47:29Z

+
+/**
+ * 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


References to ~/hunt/reports2/... are files on your machine; nobody else can follow them. Please remove them and keep the Javadoc to what the test asserts.

lukaszlenart · 2026-11-07T20:47:29Z

+
+    /** 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<String> ALLOWED_BARE = Set.of();


An empty allowlist plus a comment explaining it is machinery nothing uses. Drop it until a stack needs it.

lukaszlenart · 2026-11-07T20:47:29Z

+            .withServletRequest(new MockHttpServletRequest(verb, "/" + action))
+            .withServletResponse(res).getContextMap();
+        AccHttpActions.RAN.set(0);
+        ActionProxy p = actionProxyFactory.createActionProxy("", action, null, ctx, false, true);


executeResult=false means the seven httpheader result mappings and the result type in struts-acc-http.xml are never used; please remove them. And assert the rejection itself, response.getStatus() == 405 plus the Allow header as HttpMethodInterceptorTest does, rather than only that the counter stayed at zero.

lukaszlenart · 2026-11-07T20:47:29Z

+    }
+
+    public void testRestDefaultStackRejectsGetOnPostOnlyAction() throws Exception {
+        getAndReportStatus("/rest");


The core control above asserts 405; this one only checks the action did not run. 405 holds here too, so assertEquals(405, getAndReportStatus("/rest")). Same for the Bean Validation copy.

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.

g0w6y

done

lukaszlenart requested changes Nov 7, 2026

View reviewed changes

g0w6y changed the title ~~WW-5844 Enforce @HttpGet/@HttpPost on the implicit execute() method and add httpMethod to the REST and Bean Validation default stacks~~ WW-5844 fix(core)!: enforce HTTP method annotations across action hierarchies and plugin stacks Nov 8, 2026

g0w6y commented Nov 8, 2026

View reviewed changes

Navigation Menu

Repository navigation

WW-5844 fix(core)!: enforce HTTP method annotations across action hierarchies and plugin stacks - #2120

WW-5844 fix(core)!: enforce HTTP method annotations across action hierarchies and plugin stacks#2120
g0w6y wants to merge 2 commits into
apache:mainfrom
g0w6y:WW-5844-httpmethod-guard

g0w6y commented Nov 7, 2026 •

edited

Loading

Uh oh!

lukaszlenart left a comment

Uh oh!

lukaszlenart Nov 7, 2026

Uh oh!

lukaszlenart Nov 7, 2026

Uh oh!

lukaszlenart Nov 7, 2026

Uh oh!

lukaszlenart Nov 7, 2026

Uh oh!

lukaszlenart Nov 7, 2026

Uh oh!

lukaszlenart Nov 7, 2026

Uh oh!

lukaszlenart Nov 7, 2026

Uh oh!

lukaszlenart Nov 7, 2026

Uh oh!

lukaszlenart Nov 7, 2026

Uh oh!

g0w6y left a comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

g0w6y commented Nov 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lukaszlenart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

g0w6y left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

g0w6y commented Nov 7, 2026 •

edited

Loading