[
https://issues.apache.org/jira/browse/WW-5697?focusedWorklogId=1038196&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1038196
]
ASF GitHub Bot logged work on WW-5697:
--------------------------------------
Author: ASF GitHub Bot
Created on: 27/Aug/26 05:24
Start Date: 27/Aug/26 05:24
Worklog Time Spent: 10m
Work Description: lukaszlenart opened a new pull request, #1871:
URL: https://github.com/apache/struts/pull/1871
Fixes [WW-5697](https://issues.apache.org/jira/browse/WW-5697)
## Problem
`XWorkMethodAccessor.callMethod(...)` skipped the `denyMethodExecution`
check for any method whose name began with `get` and took one argument, or
`set` and took two:
```java
//HACK - we pass indexed method access i.e. setXXX(A,B) pattern
if ((objects.length == 2 && string.startsWith("set")) || (objects.length ==
1 && string.startsWith("get"))) {
```
That is a name prefix plus an argument count, not a property check. An
ordinary method such as `getSomething(String)` is not a JavaBeans property, but
it matches, so it was executed during parameter binding with the argument taken
from the parameter name — a parameter name of `getSomething('value').property`
calls `getSomething("value")`.
The flag the branch consults, `DENY_INDEXED_ACCESS_EXECUTION`, is never
written anywhere in main source, so `exec` is always `null` and the fast path
is unconditional.
## Change
The fast path now applies only where the target type genuinely declares an
indexed property accessor, determined with
`OgnlRuntime.getIndexedPropertyType(...)`. Everything else falls through to the
existing `denyMethodExecution` check.
Note the check is keyed on the *property* name while `callMethod` receives
the *method* name, hence the prefix strip and `Introspector.decapitalize`.
`DENY_INDEXED_ACCESS_EXECUTION` is public API, so it is deprecated here
rather than deleted; removal in 8.0.0 is tracked as
[WW-5699](https://issues.apache.org/jira/browse/WW-5699).
## Scope of the behaviour change
Confined to parameter binding. The fast path only *matters* when the deny
flag is set, and that flag is set only by `ParametersInterceptor`,
`AliasInterceptor` and `StaticParametersInterceptor` — with it unset, such a
call already fell through to the check below and executed. JSP and tag
rendering are unaffected.
Both kinds of indexed accessor keep working. Worth knowing for review: a
classic `getItem(int)` / `setItem(int, String)` pair reports as
`INDEXED_PROPERTY_OBJECT`, not `INDEXED_PROPERTY_INT`, so the predicate has to
be `!= INDEXED_PROPERTY_NONE` — an `INT`-only rule would break the very case
the original hack exists to serve.
## Tests
`XWorkMethodAccessorTest` is new and covers four behaviours:
- an argument-taking getter that is not an indexed property is not executed
while method execution is denied (this failed before the change)
- an int-indexed accessor still executes while denied
- an object-indexed accessor still executes while denied
- with the deny flag unset, an argument-taking getter still executes, as
before
The two indexed-property tests were mutation-checked: forcing the new
predicate to always reject fails exactly those two, so they are not passing
vacuously.
Full `core` suite green: 3201 tests, 0 failures, 0 errors.
## Related
[WW-5698](https://issues.apache.org/jira/browse/WW-5698) covers a separate
issue found alongside this one — the `ModelDriven` exemption in
`StrutsParameterAuthorizer` also exempting the action's own members. Different
cause, different fix, not addressed here. An end-to-end `ModelDriven` binding
test was deliberately left out of this PR because it would couple these tests
to the exemption WW-5698 is expected to change.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Issue Time Tracking
-------------------
Worklog Id: (was: 1038196)
Remaining Estimate: 0h
Time Spent: 10m
> Restrict the indexed-access fast path in XWorkMethodAccessor to real indexed
> property accessors
> -----------------------------------------------------------------------------------------------
>
> Key: WW-5697
> URL: https://issues.apache.org/jira/browse/WW-5697
> Project: Struts 2
> Issue Type: Task
> Components: Core
> Reporter: Lukasz Lenart
> Assignee: Lukasz Lenart
> Priority: Major
> Fix For: 7.4.0
>
> Time Spent: 10m
> Remaining Estimate: 0h
>
> {{XWorkMethodAccessor.callMethod(...)}} carries a long-standing fast path,
> inherited from XWork, that skips the {{denyMethodExecution}} check purely on
> the shape of the call:
> {code:java}//HACK - we pass indexed method access i.e. setXXX(A,B) pattern
> if ((objects.length == 2 && string.startsWith("set")) || (objects.length == 1
> && string.startsWith("get"))) {
> Boolean exec = (Boolean)
> context.get(ReflectionContextState.DENY_INDEXED_ACCESS_EXECUTION);
> boolean e = exec != null && exec;
> if (!e) {
> return callMethodWithDebugInfo(context, object, string, objects);
> }
> }
> boolean e = ReflectionContextState.isDenyMethodExecution(context);
> {code}
> Two problems.
> *The guard flag is never written.*
> {{ReflectionContextState.DENY_INDEXED_ACCESS_EXECUTION}} is declared in
> {{ReflectionContextState}} and read here, and nothing in main source ever
> sets it. {{exec}} is therefore always {{null}}, so the fast path is
> unconditional and {{DENY_METHOD_EXECUTION}} is never consulted for calls of
> this shape. {{ParametersInterceptor.batchApplyReflectionContextState(...)}}
> sets {{DENY_METHOD_EXECUTION}} before binding, as do {{AliasInterceptor}} and
> {{StaticParametersInterceptor}}, and that flag simply does not apply on this
> path.
> *The condition is a name-and-arity test, not a property test.* Any public
> method taking one argument whose name begins with {{get}} qualifies, whether
> or not it is an indexed property accessor. A method such as
> {{getSomething(String)}} is not a JavaBeans property at all, but it matches,
> and so it is invoked during parameter binding with the argument supplied in
> the parameter name.
> h2. Effect
> During parameter binding, a parameter name of the form
> {{getSomething('value').property}} results in {{getSomething("value")}} being
> called on an object reachable from the value stack. This only occurs where
> the object is already on the request surface, either as a {{ModelDriven}}
> model or via a property annotated with {{@StrutsParameter(depth = N)}} — a
> plain action with no annotated route is rejected by the annotation check. The
> argument is also constrained by {{DefaultAcceptedPatternsChecker}}, whose
> accepted pattern is a full match and permits only word characters and hyphens
> (plus a CJK range) inside the quotes, so values containing a slash, dot,
> colon or space never reach OGNL. The two-argument {{set}} half of the
> condition is not reachable through parameter names at all, since the accepted
> pattern has no alternation for comma-separated arguments.
> The practical consequences depend entirely on what the application's own
> methods do. The reason to change it is narrower: {{denyMethodExecution}} is
> documented to prevent method execution during parameter binding, and on this
> path it does not, and calling a non-property method is not something that
> opting an object into property binding was ever meant to permit.
> h2. Proposed change
> Restrict the fast path to genuine indexed property accessors, by resolving a
> {{PropertyDescriptor}} for the target type and confirming it is an indexed
> read or write accessor, and honour {{DENY_METHOD_EXECUTION}} for everything
> else.
> Real indexed getters of the form {{getFoo(int)}} depend on this path, so the
> change must keep them working; the accompanying tests should cover an indexed
> accessor as well as a same-shaped method that is not a property accessor.
> Also decide the fate of {{DENY_INDEXED_ACCESS_EXECUTION}}. As it is never
> written it is effectively dead configuration, and it should either be wired
> up or removed rather than left as an apparent control that does nothing.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
