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.
--
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]
