[
https://issues.apache.org/jira/browse/WW-5697?focusedWorklogId=1038432&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1038432
]
ASF GitHub Bot logged work on WW-5697:
--------------------------------------
Author: ASF GitHub Bot
Created on: 28/Aug/26 06:08
Start Date: 28/Aug/26 06:08
Worklog Time Spent: 10m
Work Description: Copilot commented on code in PR #1871:
URL: https://github.com/apache/struts/pull/1871#discussion_r3878305846
##########
core/src/main/java/org/apache/struts2/ognl/accessor/XWorkMethodAccessor.java:
##########
@@ -77,21 +83,81 @@ public Object callMethod(OgnlContext context, Object
object, String string, Obje
}
- //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);
- }
+ if (!ReflectionContextState.isDenyMethodExecution(context)) {
+ return callMethodWithDebugInfo(context, object, string, objects);
}
- boolean e = ReflectionContextState.isDenyMethodExecution(context);
- if (!e) {
+ //Method execution is denied. Indexed property access, i.e. the
getXXX(A) / setXXX(A,B) pattern, is
+ //the one exception, because reading a['k'] must keep working during
parameter binding. It is
+ //restricted to calls which really are the indexed accessor of a
property on the target type: a name
+ //prefix and an argument count alone would let any method be called
while execution is denied.
+ if (isIndexedPropertyAccessor(object, string, objects)
Review Comment:
This change touches security-sensitive framework code. Please confirm it is
not a fix for a suspected vulnerability before merging — see `SECURITY.md`.
Vulnerability fixes go through the private process at
`[email protected]`, not a public pull request.
Issue Time Tracking
-------------------
Worklog Id: (was: 1038432)
Time Spent: 1h 10m (was: 1h)
> 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: 1h 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)
