[jira] [Resolved] (WW-5697) Restrict the indexed-access fast path in XWorkMethodAccessor to real indexed property accessors

["Lukasz Lenart (Jira)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/WW-5697?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Lukasz Lenart resolved WW-5697.
-------------------------------
    Resolution: Fixed

> 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 50m
>  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)