diff --git a/core/src/main/java/org/apache/struts2/ActionProxy.java b/core/src/main/java/org/apache/struts2/ActionProxy.java index 94b816bf8c..e03c73f1f6 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"}. + *

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

* - * @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 43fb695ec8..50d273ed0d 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 { *

* The reason for the builder methods is so that you can use a subclass to create your own DefaultActionProxy instance *

- * + *

* (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 @@ private void resolveMethod() { 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 8484fac5a9..902eaacf14 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/config/providers/XmlConfigurationProviderAllowedMethodsTest.java b/core/src/test/java/org/apache/struts2/config/providers/XmlConfigurationProviderAllowedMethodsTest.java index 462b7a03e4..8419f79d3a 100644 --- a/core/src/test/java/org/apache/struts2/config/providers/XmlConfigurationProviderAllowedMethodsTest.java +++ b/core/src/test/java/org/apache/struts2/config/providers/XmlConfigurationProviderAllowedMethodsTest.java @@ -42,7 +42,7 @@ public void testDefaultAllowedMethods() throws ConfigurationException { Map actionConfigs = pkg.getActionConfigs(); // assertions - assertEquals(5, actionConfigs.size()); + assertEquals(8, actionConfigs.size()); ActionConfig action = (ActionConfig) actionConfigs.get("Default"); assertEquals(1, action.getAllowedMethods().size()); 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 8c7aa3cabc..5b2548479a 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,54 @@ public void testPostOnPutOrPostMethod() throws Exception { assertEquals(HttpMethod.POST, action.getHttpMethod()); } + /** + * Simulates a wildcard action like {@code } + * 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); + } + 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 247b4ef271..0c78ff5cf9 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 @@ - + @@ -43,6 +43,18 @@ foo,bar + + + regex:.* + + + + regex:.* + + + + regex:.* + 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 0000000000..cff2a89287 --- /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 `` 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?