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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
