WW-5697 Restrict the indexed-access fast path in XWorkMethodAccessor to real indexed properties

core/src/main/java/org/apache/struts2/ognl/accessor/XWorkMethodAccessor.java

@@ -20,16 +20,22 @@
 
 import org.apache.struts2.util.reflection.ReflectionContextState;
 import ognl.MethodFailedException;
+import ognl.OgnlException;
 import ognl.ObjectMethodAccessor;
+import ognl.ObjectIndexedPropertyDescriptor;
 import ognl.OgnlContext;
 import ognl.OgnlRuntime;
 import ognl.PropertyAccessor;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 
+import java.beans.IndexedPropertyDescriptor;
+import java.beans.Introspector;
 import java.beans.PropertyDescriptor;
+import java.lang.reflect.Method;
 import java.util.Arrays;
 import java.util.Collection;
+import java.util.List;
 
 /**
  * Allows methods to be executed under normal cirumstances, except when {@link ReflectionContextState#DENY_METHOD_EXECUTION}
@@ -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)
+                && !isIndexedAccessDenied(context)) {
             return callMethodWithDebugInfo(context, object, string, objects);
-        } else {
-            return null;
         }
+        return null;
+    }
+
+    @SuppressWarnings("removal") // the constant is deprecated for removal in 8.0.0 (WW-5699); until then it is still honoured
+    private static boolean isIndexedAccessDenied(OgnlContext context) {
+        Boolean denied = (Boolean) context.get(ReflectionContextState.DENY_INDEXED_ACCESS_EXECUTION);
+        return denied != null && denied;
+    }
+
+    /**
+     * Whether this call is the indexed accessor of a property on the target type, as opposed to an ordinary
+     * method which merely shares the {@code get}/{@code set} prefix and argument count of one.
+     * <p>
+     * The property name alone is not enough to decide, for two reasons. A class declaring the indexed pair
+     * {@code getItem(int)} / {@code setItem(int, String)} may <em>also</em> declare an unrelated
+     * {@code getItem(String)} overload, and it is that overload OGNL dispatches a one-argument call to, since
+     * the argument types pick the method and the caller chooses those. And an indexed property may be
+     * read-only, whose name would otherwise legitimise an unrelated two-argument {@code setItem(String, String)}.
+     * So the descriptor's own accessor must be the method that will actually be invoked: same name, same
+     * direction, and no same-arity overload for the dispatcher to prefer instead.
+     */
+    private boolean isIndexedPropertyAccessor(Object object, String methodName, Object[] args) {
+        boolean reading = args.length == 1 && methodName.startsWith("get");
+        boolean writing = args.length == 2 && methodName.startsWith("set");
+        if (object == null || methodName.length() <= 3 || (!reading && !writing)) {
+            return false;
+        }
+        Class<?> targetType = object.getClass();
+        String propertyName = Introspector.decapitalize(methodName.substring(3));
+        try {
+            Method accessor = indexedAccessorOf(OgnlRuntime.getPropertyDescriptor(targetType, propertyName), reading);
+            return accessor != null
+                    && accessor.getName().equals(methodName)
+                    && isTheOnlyDispatchCandidate(targetType, methodName, args.length);
+        } catch (OgnlException e) {
+            LOG.debug("Could not determine whether [{}] is an indexed property of [{}]", propertyName, targetType, e);
+            return false;
+        }
+    }
+
+    /**
+     * The indexed accessor a descriptor declares for the requested direction, or {@code null} when the
+     * descriptor is not an indexed one or declares no accessor that way round. Both flavours are covered:
+     * JavaBeans int-indexed properties, and OGNL's arbitrary-object-indexed ones.
+     */
+    private static Method indexedAccessorOf(PropertyDescriptor descriptor, boolean reading) {
+        if (descriptor instanceof IndexedPropertyDescriptor indexed) {
+            return reading ? indexed.getIndexedReadMethod() : indexed.getIndexedWriteMethod();
+        }
+        if (descriptor instanceof ObjectIndexedPropertyDescriptor objectIndexed) {
+            return reading ? objectIndexed.getIndexedReadMethod() : objectIndexed.getIndexedWriteMethod();
+        }
+        return null;
+    }
+
+    /**
+     * Whether the named method is the only one of that argument count, and so is certainly the one OGNL
+     * dispatches to. With an overload present the argument values decide, and those come from the caller.
+     */
+    private static boolean isTheOnlyDispatchCandidate(Class<?> targetType, String methodName, int argCount) {
+        List<Method> candidates = OgnlRuntime.getMethods(targetType, methodName, false);
+        return candidates != null
+                && candidates.stream().filter(candidate -> candidate.getParameterCount() == argCount).count() == 1;
     }
 
     private Object callMethodWithDebugInfo(OgnlContext context, Object object, String methodName, Object[] objects) throws MethodFailedException {

core/src/main/java/org/apache/struts2/util/reflection/ReflectionContextState.java

@@ -38,6 +38,14 @@ 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. Struts core never sets this key, so it has no effect on
+	 * framework-driven binding. Indexed property access is now identified by inspecting the target type
+	 * rather than by trusting a method name prefix, which is the check the key was standing in for.
+	 * Application or plugin code which sets the key itself does still suppress the fast path, which is
+	 * why this is deprecated rather than removed outright. Scheduled for removal in 8.0.0 by WW-5699.
+	 */
+	@Deprecated(since = "7.4.0", forRemoval = true)
 	public static final String DENY_INDEXED_ACCESS_EXECUTION = "xwork.IndexedPropertyAccessor.denyMethodExecution";
 
     public static boolean isCreatingNullObjects(Map<String, Object> context) {