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,13 +20,15 @@
 
 import org.apache.struts2.util.reflection.ReflectionContextState;
 import ognl.MethodFailedException;
+import ognl.OgnlException;
 import ognl.ObjectMethodAccessor;
 import ognl.OgnlContext;
 import ognl.OgnlRuntime;
 import ognl.PropertyAccessor;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 
+import java.beans.Introspector;
 import java.beans.PropertyDescriptor;
 import java.util.Arrays;
 import java.util.Collection;
@@ -77,9 +79,12 @@
 
         }
 
-        //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"))) {
+        //Indexed property access, i.e. the setXXX(A,B) / getXXX(A) pattern. Restricted to methods which
+        //really are indexed property accessors on the target type: a name prefix and an argument count
+        //alone would let any method be called while method execution is denied.
+        if (((objects.length == 2 && string.startsWith("set")) || (objects.length == 1 && string.startsWith("get")))
+                && isIndexedPropertyAccessor(object, string)) {
             Boolean exec = (Boolean) context.get(ReflectionContextState.DENY_INDEXED_ACCESS_EXECUTION);
             boolean e = exec != null && exec;
             if (!e) {
                 return callMethodWithDebugInfo(context, object, string, objects);
@@ -94,6 +99,23 @@
         }
     }
 
+    /**
+     * 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;
+        }
+    }
+
     private Object callMethodWithDebugInfo(OgnlContext context, Object object, String methodName, Object[] objects) throws MethodFailedException {
         try {
             return super.callMethod(context, object, methodName, objects);

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

@@ -38,7 +38,14 @@
 	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.
+	 */
+	@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) {
 		//TODO

core/src/test/java/org/apache/struts2/ognl/accessor/XWorkMethodAccessorTest.java

@@ -0,0 +1,124 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *  http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.struts2.ognl.accessor;
+
+import org.apache.struts2.ActionContext;
+import org.apache.struts2.XWorkTestCase;
+import org.apache.struts2.util.ValueStack;
+import org.apache.struts2.util.reflection.ReflectionContextState;
+
+public class XWorkMethodAccessorTest extends XWorkTestCase {
+
+    public void testDenyMethodExecutionBlocksArgumentTakingGetterThatIsNotAnIndexedProperty() {
+        Bean bean = new Bean();
+        ValueStack vs = ActionContext.getContext().getValueStack();
+        vs.push(bean);
+        ReflectionContextState.setDenyMethodExecution(vs.getContext(), true);
+
+        vs.findValue("getAttack('PWNED')");
+
+        assertNull("getAttack(String) is not an indexed property accessor and must not be"
+                + " executed while method execution is denied", bean.attackArgument);
+    }
+
+    public void testDenyMethodExecutionAllowsIntIndexedPropertyAccessor() {
+        Bean bean = new Bean();
+        ValueStack vs = ActionContext.getContext().getValueStack();
+        vs.push(bean);
+        ReflectionContextState.setDenyMethodExecution(vs.getContext(), true);
+
+        Object value = vs.findValue("getItem(1)");
+
+        assertEquals("indexed property accessors must keep working while method execution is denied",
+                "item1", value);
+    }
+
+    public void testDenyMethodExecutionAllowsObjectIndexedPropertyAccessor() {
+        Bean bean = new Bean();
+        ValueStack vs = ActionContext.getContext().getValueStack();
+        vs.push(bean);
+        ReflectionContextState.setDenyMethodExecution(vs.getContext(), true);
+
+        Object value = vs.findValue("getKeyed('k')");
+
+        assertEquals("object indexed property accessors must keep working while method execution is denied",
+                "keyedk", value);
+    }
+
+    public void testDenyMethodExecutionBlocksBareGetAccessor() {
+        Bean bean = new Bean();
+        ValueStack vs = ActionContext.getContext().getValueStack();
+        vs.push(bean);
+        ReflectionContextState.setDenyMethodExecution(vs.getContext(), true);
+
+        vs.findValue("get('PWNED')");
+
+        assertNull("a map style get(String) is not an indexed property accessor and must not be"
+                + " executed while method execution is denied", bean.bareGetArgument);
+    }
+
+    public void testArgumentTakingGetterIsExecutedWhenMethodExecutionIsNotDenied() {
+        Bean bean = new Bean();
+        ValueStack vs = ActionContext.getContext().getValueStack();
+        vs.push(bean);
+
+        vs.findValue("getAttack('PWNED')");
+
+        assertEquals("outside parameter binding the deny flag is unset and methods still execute",
+                "PWNED", bean.attackArgument);
+    }
+
+    public static class Bean {
+        private String attackArgument;
+        private String bareGetArgument;
+
+        /**
+         * Named exactly "get", so there is no property name left once the prefix is removed.
+         */
+        public String get(String key) {
+            this.bareGetArgument = key;
+            return "irrelevant";
+        }
+
+        /**
+         * Not a JavaBeans property: takes an argument and has no matching setter, so it is not an
+         * indexed property accessor either.
+         */
+        public String getAttack(String argument) {
+            this.attackArgument = argument;
+            return "irrelevant";
+        }
+
+        public String getItem(int index) {
+            return "item" + index;
+        }
+
+        public void setItem(int index, String value) {
+            // present so that the pair forms an indexed property
+        }
+
+        public String getKeyed(String key) {
+            return "keyed" + key;
+        }
+
+        public void setKeyed(String key, String value) {
+            // present so that the pair forms an indexed property
+        }
+    }
+}