This is an automated email from the ASF dual-hosted git repository. lukaszlenart pushed a commit to branch fix/WW-5535-http-method-interceptor-wildcard in repository https://gitbox.apache.org/repos/asf/struts.git
commit e6ad2951a8526e32df890a107d0b273f30546aa5 Author: Lukasz Lenart <[email protected]> AuthorDate: Sun Feb 22 17:00:25 2026 +0100 fix(core): correct isMethodSpecified() for wildcard-resolved methods DefaultActionProxy.resolveMethod() unconditionally set methodSpecified=false when the method was not passed explicitly, including when it was resolved from ActionConfig (e.g., wildcard substitution like method="{1}"). This caused HttpMethodInterceptor to skip method-level annotation checks for wildcard actions, falling back to class-level annotations instead. Move methodSpecified=false inside the inner branch that defaults to "execute", so config-resolved methods (including wildcard-substituted ones) correctly report isMethodSpecified()=true. Update Javadoc to reflect the corrected semantics. Fixes [WW-5535](https://issues.apache.org/jira/browse/WW-5535) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]> --- .../main/java/org/apache/struts2/ActionProxy.java | 11 +- .../org/apache/struts2/DefaultActionProxy.java | 14 +- .../org/apache/struts2/DefaultActionProxyTest.java | 60 +++++-- .../httpmethod/HttpMethodInterceptorTest.java | 74 +++++++++ .../providers/xwork-test-allowed-methods.xml | 14 +- ...-22-WW-5535-http-method-interceptor-wildcard.md | 174 +++++++++++++++++++++ 6 files changed, 328 insertions(+), 19 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/ActionProxy.java b/core/src/main/java/org/apache/struts2/ActionProxy.java index 94b816bf8..e03c73f1f 100644 --- a/core/src/main/java/org/apache/struts2/ActionProxy.java +++ b/core/src/main/java/org/apache/struts2/ActionProxy.java @@ -93,9 +93,16 @@ public interface ActionProxy { String getMethod(); /** - * Gets status of the method value's initialization. + * Returns whether the action method was explicitly specified rather than defaulting to {@code "execute"}. + * <p> + * This returns {@code true} when the method was provided via the URL (DMI), passed as a constructor argument, + * or resolved from the action configuration (including wildcard-substituted values like {@code method="{1}"}). + * It returns {@code false} only when no method was specified anywhere and the framework fell back + * to the default {@code "execute"} method. + * </p> * - * @return true if the method returned by getMethod() is not a default initializer value. + * @return {@code true} if the method was explicitly provided or resolved from config; + * {@code false} only when defaulting to {@code "execute"} */ boolean isMethodSpecified(); diff --git a/core/src/main/java/org/apache/struts2/DefaultActionProxy.java b/core/src/main/java/org/apache/struts2/DefaultActionProxy.java index 43fb695ec..50d273ed0 100644 --- a/core/src/main/java/org/apache/struts2/DefaultActionProxy.java +++ b/core/src/main/java/org/apache/struts2/DefaultActionProxy.java @@ -71,14 +71,14 @@ public class DefaultActionProxy implements ActionProxy, Serializable { * <p> * The reason for the builder methods is so that you can use a subclass to create your own DefaultActionProxy instance * </p> - * + * <p> * (like a RMIActionProxy). * - * @param inv the action invocation - * @param namespace the namespace - * @param actionName the action name - * @param methodName the method name - * @param executeResult execute result + * @param inv the action invocation + * @param namespace the namespace + * @param actionName the action name + * @param methodName the method name + * @param executeResult execute result * @param cleanupContext cleanup context */ protected DefaultActionProxy(ActionInvocation inv, String namespace, String actionName, String methodName, boolean executeResult, boolean cleanupContext) { @@ -171,8 +171,8 @@ public class DefaultActionProxy implements ActionProxy, Serializable { this.method = config.getMethodName(); if (StringUtils.isEmpty(this.method)) { this.method = ActionConfig.DEFAULT_METHOD; + methodSpecified = false; } - methodSpecified = false; } } diff --git a/core/src/test/java/org/apache/struts2/DefaultActionProxyTest.java b/core/src/test/java/org/apache/struts2/DefaultActionProxyTest.java index 8484fac5a..902eaacf1 100644 --- a/core/src/test/java/org/apache/struts2/DefaultActionProxyTest.java +++ b/core/src/test/java/org/apache/struts2/DefaultActionProxyTest.java @@ -18,25 +18,67 @@ */ package org.apache.struts2; -import org.apache.struts2.mock.MockActionInvocation; -import org.apache.struts2.StrutsInternalTestCase; +import org.apache.struts2.config.ConfigurationException; import org.apache.struts2.config.StrutsXmlConfigurationProvider; -import org.junit.Test; +import org.apache.struts2.mock.MockActionInvocation; public class DefaultActionProxyTest extends StrutsInternalTestCase { - @Test - public void testThorwExceptionOnNotAllowedMethod() throws Exception { - final String filename = "org/apache/struts2/config/providers/xwork-test-allowed-methods.xml"; - loadConfigurationProviders(new StrutsXmlConfigurationProvider(filename)); + private static final String CONFIG = "org/apache/struts2/config/providers/xwork-test-allowed-methods.xml"; + + public void testThrowExceptionOnNotAllowedMethod() { + loadConfigurationProviders(new StrutsXmlConfigurationProvider(CONFIG)); DefaultActionProxy dap = new DefaultActionProxy(new MockActionInvocation(), "strict", "Default", "notAllowed", true, true); container.inject(dap); try { dap.prepare(); fail("Must throw exception!"); - } catch (Exception e) { - assertEquals(e.getMessage(), "Method notAllowed for action Default is not allowed!"); + } catch (ConfigurationException e) { + assertEquals("Method notAllowed for action Default is not allowed!", e.getMessage()); } } + + public void testMethodSpecifiedWhenPassedExplicitly() { + loadConfigurationProviders(new StrutsXmlConfigurationProvider(CONFIG)); + DefaultActionProxy dap = new DefaultActionProxy(new MockActionInvocation(), "default", "Default", "input", true, true); + container.inject(dap); + dap.prepare(); + + assertTrue("Method should be specified when passed as constructor argument", dap.isMethodSpecified()); + assertEquals("input", dap.getMethod()); + } + + public void testMethodSpecifiedWhenResolvedFromConfig() { + loadConfigurationProviders(new StrutsXmlConfigurationProvider(CONFIG)); + // ConfigMethod action has method="onPostOnly" in XML config, no method passed in constructor + DefaultActionProxy dap = new DefaultActionProxy(new MockActionInvocation(), "default", "ConfigMethod", null, true, true); + container.inject(dap); + dap.prepare(); + + assertTrue("Method should be specified when resolved from action config", dap.isMethodSpecified()); + assertEquals("onPostOnly", dap.getMethod()); + } + + public void testMethodNotSpecifiedWhenDefaultingToExecute() { + loadConfigurationProviders(new StrutsXmlConfigurationProvider(CONFIG)); + // NoMethod action has no method in XML config and no method passed in constructor + DefaultActionProxy dap = new DefaultActionProxy(new MockActionInvocation(), "default", "NoMethod", null, true, true); + container.inject(dap); + dap.prepare(); + + assertFalse("Method should not be specified when defaulting to execute", dap.isMethodSpecified()); + assertEquals("execute", dap.getMethod()); + } + + public void testMethodSpecifiedWithWildcardAction() { + loadConfigurationProviders(new StrutsXmlConfigurationProvider(CONFIG)); + // Wild-onPostOnly matches Wild-* with method="{1}" -> resolves to "onPostOnly" + DefaultActionProxy dap = new DefaultActionProxy(new MockActionInvocation(), "default", "Wild-onPostOnly", null, true, true); + container.inject(dap); + dap.prepare(); + + assertTrue("Method should be specified when resolved from wildcard config", dap.isMethodSpecified()); + assertEquals("onPostOnly", dap.getMethod()); + } } \ No newline at end of file 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 8c7aa3cab..5f29681df 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 @@ -217,6 +217,80 @@ public class HttpMethodInterceptorTest extends StrutsInternalTestCase { assertEquals(HttpMethod.POST, action.getHttpMethod()); } + /** + * Simulates a wildcard action like {@code <action name="Wild-*" method="{1}">} + * resolving to method "onPostOnly" (annotated with @HttpPost). + * With the fix in DefaultActionProxy.resolveMethod(), config-resolved methods + * set isMethodSpecified()=true, so the interceptor checks method-level annotations. + * A GET request should be rejected because @HttpPost only allows POST. + */ + public void testWildcardResolvedMethodWithPostAnnotationRejectsGet() throws Exception { + // given + HttpMethodsTestAction action = new HttpMethodsTestAction(); + prepareActionInvocation(action); + // Simulate wildcard resolution: Wild-onPostOnly -> method="onPostOnly" + actionProxy.setMethod("onPostOnly"); + // After the fix, config-resolved methods have methodSpecified=true + actionProxy.setMethodSpecified(true); + + invocation.setResultCode("onPostOnly"); + + prepareRequest("get"); + + // when + String resultName = interceptor.intercept(invocation); + + // then - interceptor checks method-level @HttpPost and rejects GET + assertEquals("bad-request", resultName); + } + + /** + * Counterpart: same wildcard scenario but with POST request — should succeed. + */ + public void testWildcardResolvedMethodWithPostAnnotationAllowsPost() throws Exception { + // given + HttpMethodsTestAction action = new HttpMethodsTestAction(); + prepareActionInvocation(action); + actionProxy.setMethod("onPostOnly"); + actionProxy.setMethodSpecified(true); + + invocation.setResultCode("onPostOnly"); + + prepareRequest("post"); + + // when + String resultName = interceptor.intercept(invocation); + + // then - interceptor checks method-level @HttpPost and allows POST + assertEquals("onPostOnly", resultName); + } + + /** + * Before the fix: when methodSpecified=false (bug), the interceptor falls through + * to class-level @AllowedHttpMethod(POST), which also rejects GET — but for the + * wrong reason (checking class-level instead of method-level annotation). + * This test verifies the interceptor uses method-level annotations when methodSpecified=true. + */ + public void testWildcardResolvedMethodGetAnnotationAllowsGet() throws Exception { + // given + HttpMethodsTestAction action = new HttpMethodsTestAction(); + prepareActionInvocation(action); + // Simulate wildcard resolution to onGetOnly (annotated with @HttpGet) + actionProxy.setMethod("onGetOnly"); + actionProxy.setMethodSpecified(true); + + invocation.setResultCode("onGetOnly"); + + prepareRequest("get"); + + // when + String resultName = interceptor.intercept(invocation); + + // then - method-level @HttpGet allows GET (class-level @AllowedHttpMethod(POST) would reject it) + assertEquals("onGetOnly", resultName); + assertEquals(HttpMethod.GET, action.getHttpMethod()); + } + private void prepareActionInvocation(Object action) { interceptor = new HttpMethodInterceptor(); invocation = new MockActionInvocation(); diff --git a/core/src/test/resources/org/apache/struts2/config/providers/xwork-test-allowed-methods.xml b/core/src/test/resources/org/apache/struts2/config/providers/xwork-test-allowed-methods.xml index 247b4ef27..0c78ff5cf 100644 --- a/core/src/test/resources/org/apache/struts2/config/providers/xwork-test-allowed-methods.xml +++ b/core/src/test/resources/org/apache/struts2/config/providers/xwork-test-allowed-methods.xml @@ -29,7 +29,7 @@ </action> <action name="Boring"> - <allowed-methods> </allowed-methods> + <allowed-methods></allowed-methods> </action> <action name="Foo"> @@ -43,6 +43,18 @@ <action name="Baz" method="baz"> <allowed-methods>foo,bar</allowed-methods> </action> + + <action name="Wild-*" class="org.apache.struts2.HttpMethodsTestAction" method="{1}"> + <allowed-methods>regex:.*</allowed-methods> + </action> + + <action name="ConfigMethod" class="org.apache.struts2.HttpMethodsTestAction" method="onPostOnly"> + <allowed-methods>regex:.*</allowed-methods> + </action> + + <action name="NoMethod" class="org.apache.struts2.HttpMethodsTestAction"> + <allowed-methods>regex:.*</allowed-methods> + </action> </package> <package name="strict" strict-method-invocation="true"> diff --git a/thoughts/shared/research/2026-02-22-WW-5535-http-method-interceptor-wildcard.md b/thoughts/shared/research/2026-02-22-WW-5535-http-method-interceptor-wildcard.md new file mode 100644 index 000000000..cff2a8928 --- /dev/null +++ b/thoughts/shared/research/2026-02-22-WW-5535-http-method-interceptor-wildcard.md @@ -0,0 +1,174 @@ +--- +date: 2026-02-22T12:00:00+01:00 +topic: "HttpMethodInterceptor does not work with action names using wildcards" +tags: [research, codebase, HttpMethodInterceptor, DefaultActionProxy, wildcard, isMethodSpecified, security] +status: complete +git_commit: a21c763d8a8592f1056086134414123f6d8d168d +--- + +# Research: WW-5535 — HttpMethodInterceptor does not work with wildcard action names + +**Date**: 2026-02-22 + +## Research Question + +Why does `HttpMethodInterceptor` fail to enforce HTTP method validation when actions use wildcard names like +`example-*`? + +## Summary + +The root cause is in `DefaultActionProxy.resolveMethod()`. For wildcard-matched actions, the method name is resolved +from the `ActionConfig` (after wildcard substitution) rather than being passed explicitly from the URL. Because +`resolveMethod()` treats any method resolved from config as "not specified", `isMethodSpecified()` returns `false`. This +causes `HttpMethodInterceptor` to check **class-level** annotations instead of **method-level** annotations, potentially +skipping validation entirely. + +## Detailed Findings + +### 1. The `methodSpecified` Flag Logic + +**File**: [ +`DefaultActionProxy.java`](https://github.com/apache/struts/blob/a21c763d8a8592f1056086134414123f6d8d168d/core/src/main/java/org/apache/struts2/DefaultActionProxy.java) + +```java +private boolean methodSpecified = true; // field default + +private void resolveMethod() { + if (StringUtils.isEmpty(this.method)) { + this.method = config.getMethodName(); + if (StringUtils.isEmpty(this.method)) { + this.method = ActionConfig.DEFAULT_METHOD; + } + methodSpecified = false; // <-- ONLY path that sets it false + } +} +``` + +The flag is set to `false` whenever the method was not passed explicitly to the proxy constructor. This conflates two +different concepts: + +- **"Was the method specified by the user via DMI?"** (security concern — user-controlled method invocation) +- **"Is a non-default method being invoked?"** (what `HttpMethodInterceptor` needs to know) + +### 2. Wildcard Resolution Flow + +For a config like `<action name="example-*" class="ExampleAction" method="do{1}">` and URL `example-create`: + +| Step | Component | Result | +|------|----------------------------------------------|-------------------------------------------------------------------------------------------------------------| +| 1 | `DefaultActionMapper.extractMethodName()` | `ActionMapping.method = null` (exact map lookup fails for wildcards) | +| 2 | `Dispatcher.serviceAction()` | Passes `null` method to `ActionProxyFactory` | +| 3 | `DefaultActionProxy` constructor | `this.method = null` | +| 4 | `RuntimeConfigurationImpl.getActionConfig()` | Triggers `ActionConfigMatcher.match()` → `convert()` substitutes `{1}` → `config.methodName = "docreate"` | +| 5 | `DefaultActionProxy.resolveMethod()` | `this.method` is empty → takes `config.getMethodName()` = `"docreate"` → **sets `methodSpecified = false`** | + +**Key file**: [ +`ActionConfigMatcher.java`](https://github.com/apache/struts/blob/a21c763d8a8592f1056086134414123f6d8d168d/core/src/main/java/org/apache/struts2/config/impl/ActionConfigMatcher.java) — +`convert()` performs `{n}` substitution on method name. + +**Key file**: [ +`DefaultActionMapper.java`](https://github.com/apache/struts/blob/a21c763d8a8592f1056086134414123f6d8d168d/core/src/main/java/org/apache/struts2/dispatcher/mapper/DefaultActionMapper.java) — +`extractMethodName()` uses `cfg.getActionConfigs().get(mapping.getName())` which is an exact map lookup that cannot +match wildcard patterns. + +### 3. HttpMethodInterceptor Behavior + +**File**: [ +`HttpMethodInterceptor.java`](https://github.com/apache/struts/blob/a21c763d8a8592f1056086134414123f6d8d168d/core/src/main/java/org/apache/struts2/interceptor/httpmethod/HttpMethodInterceptor.java) + +```java +if (invocation.getProxy().isMethodSpecified()) { + // Check METHOD-LEVEL annotations (e.g., @HttpGet on the specific method) + Method method = action.getClass().getMethod(invocation.getProxy().getMethod()); + if (AnnotationUtils.isAnnotatedBy(method, HTTP_METHOD_ANNOTATIONS)) { + return doIntercept(invocation, method); + } +} else if (AnnotationUtils.isAnnotatedBy(action.getClass(), HTTP_METHOD_ANNOTATIONS)) { + // Check CLASS-LEVEL annotations only + return doIntercept(invocation, action.getClass()); +} +// No annotations → allow request through (no validation) +``` + +**Impact for wildcard actions**: Since `isMethodSpecified()` returns `false`, the interceptor checks class-level +annotations. If the action class has no class-level HTTP method annotations (only method-level ones), validation is * +*skipped entirely**. + +### 4. The Semantic Mismatch + +The `ActionProxy.isMethodSpecified()` Javadoc says: + +> Returns true if the method returned by `getMethod()` is not a default initializer value. + +The current implementation interprets "default initializer" as "anything not explicitly passed from the URL/DMI", which +includes wildcard-configured methods. But for `HttpMethodInterceptor`, what matters is whether a *specific* method (not +`execute`) is being invoked, regardless of how it was determined. + +## Code References + +- [ + `DefaultActionProxy.java`](https://github.com/apache/struts/blob/a21c763d8a8592f1056086134414123f6d8d168d/core/src/main/java/org/apache/struts2/DefaultActionProxy.java) — + `resolveMethod()` and `methodSpecified` field +- [ + `ActionProxy.java`](https://github.com/apache/struts/blob/a21c763d8a8592f1056086134414123f6d8d168d/core/src/main/java/org/apache/struts2/ActionProxy.java) — + `isMethodSpecified()` interface and Javadoc +- [ + `HttpMethodInterceptor.java`](https://github.com/apache/struts/blob/a21c763d8a8592f1056086134414123f6d8d168d/core/src/main/java/org/apache/struts2/interceptor/httpmethod/HttpMethodInterceptor.java) — + `intercept()` branching on `isMethodSpecified()` +- [ + `ActionConfigMatcher.java`](https://github.com/apache/struts/blob/a21c763d8a8592f1056086134414123f6d8d168d/core/src/main/java/org/apache/struts2/config/impl/ActionConfigMatcher.java) — + wildcard `{n}` substitution in `convert()` +- [ + `DefaultActionMapper.java`](https://github.com/apache/struts/blob/a21c763d8a8592f1056086134414123f6d8d168d/core/src/main/java/org/apache/struts2/dispatcher/mapper/DefaultActionMapper.java) — + `extractMethodName()` exact-match lookup +- [ + `DefaultConfiguration.java`](https://github.com/apache/struts/blob/a21c763d8a8592f1056086134414123f6d8d168d/core/src/main/java/org/apache/struts2/config/impl/DefaultConfiguration.java) — + `findActionConfigInNamespace()` wildcard fallback +- [ + `HttpMethodInterceptorTest.java`](https://github.com/apache/struts/blob/a21c763d8a8592f1056086134414123f6d8d168d/core/src/test/java/org/apache/struts2/interceptor/httpmethod/HttpMethodInterceptorTest.java) — + existing tests +- [ + `DefaultActionProxyTest.java`](https://github.com/apache/struts/blob/a21c763d8a8592f1056086134414123f6d8d168d/core/src/test/java/org/apache/struts2/DefaultActionProxyTest.java) — + only tests disallowed method, not `isMethodSpecified()` + +## Architecture Insights + +The `isMethodSpecified()` flag was originally introduced (WW-3628) to distinguish user-controlled DMI method invocation +from default `execute()` dispatch. This was a security measure — DMI-specified methods need stricter validation. + +However, the flag now serves double duty: + +1. **Security gate** in DMI handling — was the method user-specified via URL? +2. **Dispatch hint** in `HttpMethodInterceptor` — should we check method-level or class-level annotations? + +These two concerns have different semantics for wildcard actions. A wildcard-configured method like `do{1}` is **not +user-controlled** (it's defined in the config), but it **is a specific method** that should have its annotations +checked. + +## Potential Fix Approaches + +1. **Fix in `HttpMethodInterceptor`**: Instead of relying on `isMethodSpecified()`, check method-level annotations + first, then fall back to class-level. This avoids changing the `ActionProxy` contract. + +2. **Fix in `DefaultActionProxy.resolveMethod()`**: Set `methodSpecified = true` when the method is resolved from + config (not just defaulted to `execute`). This changes the semantics of the flag but aligns with the Javadoc ("not a + default initializer value"). + +3. **Add a new flag**: Introduce `isMethodFromConfig()` or similar to distinguish "method from wildcard config" from " + method from DMI" from "default execute". This is the most precise but highest-impact change. + +## Test Gaps + +- No tests for `DefaultActionProxy` with wildcard-resolved action names +- No tests for `isMethodSpecified()` on a real `DefaultActionProxy` instance (all tests use `MockActionProxy`) +- No tests for `HttpMethodInterceptor` combined with wildcard-resolved methods + +## Historical Context (from thoughts/) + +No prior research documents found for WW-5535 or WW-3628. + +## Open Questions + +1. Are there other interceptors or components that depend on `isMethodSpecified()` semantics? +2. Would changing `methodSpecified` behavior for config-resolved methods break DMI security checks? +3. Should `DefaultActionMapper.extractMethodName()` be updated to also resolve wildcard-matched configs?
