WW-5697 Restrict the indexed-access fast path in XWorkMethodAccessor to real indexed properties

[lukaszlenart]

Fixes 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:

//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 call really is the indexed accessor of a property on the target type. The property name alone is not enough to decide that, which review of the first cut showed the hard way:

• a class declaring the indexed pair getItem(int) / setItem(int, String) may also declare an unrelated getItem(String) overload, and a one-argument call dispatches to that overload — the argument types choose the method, and the caller chooses the arguments;
• the check has to know its direction, or a read-only getItem(int) legitimises an unrelated two-argument setItem(String, String).

So the descriptor's own indexed accessor must be the method that will actually run: same name, same direction (getIndexedReadMethod / getIndexedWriteMethod, for both IndexedPropertyDescriptor and OGNL's ObjectIndexedPropertyDescriptor), and no same-arity overload for the dispatcher to prefer instead. Everything else falls through to the denyMethodExecution check.

The deny check is hoisted ahead of the indexed-property block, which it now guards. The two are equivalent — with execution permitted, both paths ended in the same callMethodWithDebugInfo call — but this way the introspection is skipped entirely on the common path, and the block reads as the exception it is.

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. It is still honoured, so the framework's own read of it carries @SuppressWarnings("removal") rather than warning on every core build.

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.

Two narrowings worth calling out for the migration guide, both silent (debug-level) when they bite, and both only while method execution is denied:

• An unpaired object-indexed getter is no longer exempt. String getKeyed(String) with no matching setKeyed(String, String) reports INDEXED_PROPERTY_NONE, because OGNL's findObjectIndexedPropertyDescriptors requires an exact pair with matching key and value types. The same applies when the types do not line up, e.g. String getKeyed(String) against void setKeyed(String, Object). OGNL itself refuses bean.keyed['k'] for such beans, so this is consistent, but it is a change.
• Bare get(...) / set(...) are no longer exempt. "get".startsWith("get") matched the old fast path, so myMap.get('k').field bound under denyMethodExecution. There is no property name left once the prefix is stripped, so it cannot be an indexed accessor.

Tests

XWorkMethodAccessorTest is new and covers eight behaviours, including:

• an argument-taking getter that is not an indexed property is not executed while method execution is denied
• an overload of a genuine indexed accessor is not executed while denied
• an unrelated two-argument setter named after a read-only indexed property is not executed while denied
• object-indexed and int-indexed accessors still execute while denied
• a bare get(String) is not executed while denied
• with the deny flag unset, an argument-taking getter still executes, as before

Note that a getItem(int) / setItem(int, String) pair reports as INDEXED_PROPERTY_OBJECT, not _INT: findObjectIndexedPropertyDescriptors overwrites the java.beans descriptor whenever it finds a matching pair. A read-only int-indexed getter is the only shape that reaches the _INT branch, so there is a test for it specifically.

Mutation-checked both ways: restoring the old name-only predicate fails exactly the two overload/direction tests, and forcing the predicate to always reject fails exactly the three "still works" tests. None of them pass vacuously.

Full core suite green: 3205 tests, 0 failures, 0 errors.

Related

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

[Copilot]

Pull request overview

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Fixes WW-5697 by preventing XWorkMethodAccessor.callMethod(...) from bypassing denyMethodExecution for arbitrary get*/set* methods, restricting the “indexed-access” fast path to real indexed JavaBeans properties.

Changes:

• Restricts the indexed-access fast path to methods backed by an indexed property on the target type (via OgnlRuntime.getIndexedPropertyType(...)).
• Deprecates DENY_INDEXED_ACCESS_EXECUTION (public API) since it was never set by the framework and is no longer meaningful.
• Adds a focused test suite covering blocked vs. allowed method execution under deny-mode, including both int- and object-indexed accessors.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
core/src/test/java/org/apache/struts2/ognl/accessor/XWorkMethodAccessorTest.java	Adds regression tests for the indexed-access restriction and deny-mode behavior.
core/src/main/java/org/apache/struts2/util/reflection/ReflectionContextState.java	Deprecates the now-obsolete indexed-access deny flag constant with rationale.
core/src/main/java/org/apache/struts2/ognl/accessor/XWorkMethodAccessor.java	Implements indexed-property detection and applies it to gate the fast path.

---

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
91.7% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud