[
https://issues.apache.org/jira/browse/WW-5697?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Lukasz Lenart reassigned WW-5697:
---------------------------------
Assignee: Lukasz Lenart
> 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
>
>
> {{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)
