fix(core): correct isMethodSpecified() for wildcard-resolved methods (#1592)

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 <noreply@anthropic.com>
2026-08-05 06:36:58 +00:00 · 2026-02-27 13:25:38 +01:00
parent 8ac1511290
commit 4d2eb93835
7 changed files with 303 additions and 20 deletions
@@ -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();

@@ -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;
        }
    }

@@ -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());
+    }
 }
@@ -42,7 +42,7 @@ public class XmlConfigurationProviderAllowedMethodsTest extends ConfigurationTes
        Map actionConfigs = pkg.getActionConfigs();

        // assertions
-        assertEquals(5, actionConfigs.size());
+        assertEquals(8, actionConfigs.size());

        ActionConfig action = (ActionConfig) actionConfigs.get("Default");
        assertEquals(1, action.getAllowedMethods().size());
@@ -217,6 +217,54 @@ 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);
+    }
+
    private void prepareActionInvocation(Object action) {
        interceptor = new HttpMethodInterceptor();
        invocation = new MockActionInvocation();
@@ -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">
@@ -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?