[
https://issues.apache.org/jira/browse/WW-5697?focusedWorklogId=1038438&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1038438
]
ASF GitHub Bot logged work on WW-5697:
--------------------------------------
Author: ASF GitHub Bot
Created on: 28/Aug/26 06:37
Start Date: 28/Aug/26 06:37
Worklog Time Spent: 10m
Work Description: lukaszlenart commented on code in PR #1871:
URL: https://github.com/apache/struts/pull/1871#discussion_r3878442031
##########
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:
Reworded in the current head. The javadoc now says Struts core never sets
the key, and states what does still hold: application or plugin code that sets
it itself continues to suppress the fast path, which is why the constant is
deprecated rather than removed outright.
That behaviour also has tests now, both ways — the key set, and the key set
to `false`.
##########
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:
Not adding a cache — the expensive half is already cached by OGNL.
`OgnlRuntime.getPropertyDescriptor` goes through `getPropertyDescriptors`,
which reads `OgnlCache.propertyDescriptorCache`, a `ClassCache<Map<String,
PropertyDescriptor>>` (`OgnlCache.java:129`), so `Introspector.getBeanInfo`
runs once per class rather than once per invocation. `getMethods(Class, String,
boolean)` is cached the same way. A second cache in front of those would
duplicate them and add another class-keyed static map, which is the shape
WW-5537 was about.
What is left per call is a `startsWith`, a `substring` and a `decapitalize`.
The check has also moved behind the deny test in the current head, so none of
it runs unless method execution is actually denied.
##########
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:
Confirmed — this was checked before the PR was opened, and again against
SECURITY.md.
WW-5697 was triaged as an ordinary bug rather than a vulnerability: the
control defect is ours, but the impact is application-owned, so it is handled
publicly with no CVE and no security framing. The dead
`DENY_INDEXED_ACCESS_EXECUTION` constant is removed separately under WW-5699,
targeted at 8.0.0.
Issue Time Tracking
-------------------
Worklog Id: (was: 1038438)
Time Spent: 1h 20m (was: 1h 10m)
> 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 20m
> 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)
