[
https://issues.apache.org/jira/browse/WW-5697?focusedWorklogId=1038302&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1038302
]
ASF GitHub Bot logged work on WW-5697:
--------------------------------------
Author: ASF GitHub Bot
Created on: 27/Aug/26 12:53
Start Date: 27/Aug/26 12:53
Worklog Time Spent: 10m
Work Description: Copilot commented on code in PR #1871:
URL: https://github.com/apache/struts/pull/1871#discussion_r3871867844
##########
core/src/main/java/org/apache/struts2/util/reflection/ReflectionContextState.java:
##########
@@ -38,6 +38,13 @@ public class ReflectionContextState {
public static final String FULL_PROPERTY_PATH =
"current.property.path"; // TODO: Probably a bug
public static final String CREATE_NULL_OBJECTS =
"xwork.NullHandler.createNullObjects";
public static final String DENY_METHOD_EXECUTION =
"xwork.MethodAccessor.denyMethodExecution";
+ /**
+ * @deprecated since 7.4.0, no replacement. Nothing in the framework
has ever set this key, so it has
+ * never had any effect. Indexed property access is now identified by
inspecting the target type rather
+ * than by trusting a method name prefix, which leaves this flag with
nothing to guard. Scheduled for
+ * removal in 8.0.0 by WW-5699.
Review Comment:
The Javadoc states “Nothing in the framework has ever set this key,” but
this is a public API constant and may have been set by plugins or application
code. To avoid misleading consumers, consider rewording to something strictly
verifiable from this repo context (e.g., “Struts core does not set this key” /
“not set by the framework codebase”), while keeping the deprecation rationale.
##########
core/src/main/java/org/apache/struts2/ognl/accessor/XWorkMethodAccessor.java:
##########
@@ -94,6 +99,23 @@ public Object callMethod(OgnlContext context, Object object,
String string, Obje
}
}
+ /**
+ * Whether {@code methodName} is an indexed property accessor on the
target type, as opposed to an ordinary
+ * method which merely shares the {@code get}/{@code set} prefix and
argument count of one.
+ */
+ private boolean isIndexedPropertyAccessor(Object object, String
methodName) {
+ if (object == null || methodName.length() <= 3) {
+ return false;
+ }
+ String propertyName =
Introspector.decapitalize(methodName.substring(3));
+ try {
+ return OgnlRuntime.getIndexedPropertyType(object.getClass(),
propertyName) != OgnlRuntime.INDEXED_PROPERTY_NONE;
+ } catch (OgnlException e) {
+ LOG.debug("Could not determine whether [{}] is an indexed property
of [{}]", propertyName, object.getClass(), e);
+ return false;
+ }
+ }
Review Comment:
`isIndexedPropertyAccessor(...)` is called on the `get*/set*` path and
performs per-invocation string manipulation plus a runtime introspection call.
If `callMethod(...)` is on a hot path during binding, consider caching the
boolean result per `(Class, propertyName)` (or per method) to avoid repeated
checks across many invocations. This keeps the security fix while reducing
overhead under heavy parameter binding.
Issue Time Tracking
-------------------
Worklog Id: (was: 1038302)
Time Spent: 40m (was: 0.5h)
> 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: 40m
> 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)
