Lukasz Lenart created WW-5697:
---------------------------------
Summary: 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
Fix For: 7.4.0
{{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)
