diff --git a/core/src/main/java/org/apache/struts2/ognl/accessor/XWorkMethodAccessor.java b/core/src/main/java/org/apache/struts2/ognl/accessor/XWorkMethodAccessor.java index 025553997d..9c3a3bcd99 100644 --- a/core/src/main/java/org/apache/struts2/ognl/accessor/XWorkMethodAccessor.java +++ b/core/src/main/java/org/apache/struts2/ognl/accessor/XWorkMethodAccessor.java @@ -20,6 +20,7 @@ import org.apache.struts2.util.reflection.ReflectionContextState; import ognl.MethodFailedException; +import ognl.OgnlException; import ognl.ObjectMethodAccessor; import ognl.OgnlContext; import ognl.OgnlRuntime; @@ -27,6 +28,7 @@ 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,8 +79,11 @@ 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"))) { + //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) { @@ -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; + } + } + private Object callMethodWithDebugInfo(OgnlContext context, Object object, String methodName, Object[] objects) throws MethodFailedException { try { return super.callMethod(context, object, methodName, objects); diff --git a/core/src/main/java/org/apache/struts2/util/reflection/ReflectionContextState.java b/core/src/main/java/org/apache/struts2/util/reflection/ReflectionContextState.java index cc2f457c3e..d1cb8fb5b7 100644 --- a/core/src/main/java/org/apache/struts2/util/reflection/ReflectionContextState.java +++ b/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. + */ + @Deprecated(since = "7.4.0", forRemoval = true) public static final String DENY_INDEXED_ACCESS_EXECUTION = "xwork.IndexedPropertyAccessor.denyMethodExecution"; public static boolean isCreatingNullObjects(Map context) { diff --git a/core/src/test/java/org/apache/struts2/ognl/accessor/XWorkMethodAccessorTest.java b/core/src/test/java/org/apache/struts2/ognl/accessor/XWorkMethodAccessorTest.java new file mode 100644 index 0000000000..a460fab9c2 --- /dev/null +++ b/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 + } + } +}