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..878f3b8fe2 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,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. + *

+ * 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 also 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 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 { 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..d4ad46ea51 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,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 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..9fa17d1bcd --- /dev/null +++ b/core/src/test/java/org/apache/struts2/ognl/accessor/XWorkMethodAccessorTest.java @@ -0,0 +1,342 @@ +/* + * 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); + } + + /** + * Note the name: OGNL classifies this pair as {@code INDEXED_PROPERTY_OBJECT}, not {@code _INT}, because + * {@code findObjectIndexedPropertyDescriptors} overwrites the {@code java.beans} descriptor whenever it + * finds a matching get/set pair. {@link #testDenyMethodExecutionAllowsReadOnlyIntIndexedPropertyAccessor()} + * is what covers the {@code _INT} branch. + */ + public void testDenyMethodExecutionAllowsIndexedPropertyAccessorDeclaredOverAnIntIndex() { + 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); + } + + /** + * A read-only indexed property is the one shape that reaches {@code INDEXED_PROPERTY_INT}: with no + * matching setter, OGNL leaves the {@code java.beans} {@code IndexedPropertyDescriptor} in place. + */ + public void testDenyMethodExecutionAllowsReadOnlyIntIndexedPropertyAccessor() { + ReadOnlyIndexedBean bean = new ReadOnlyIndexedBean(); + ValueStack vs = ActionContext.getContext().getValueStack(); + vs.push(bean); + ReflectionContextState.setDenyMethodExecution(vs.getContext(), true); + + Object value = vs.findValue("getItem(1)"); + + assertEquals("a read-only indexed property accessor must keep working while method execution is denied", + "item1", value); + } + + /** + * The property name alone does not identify the method that will run. This bean really does declare the + * indexed pair getItem(int)/setItem(int, String), so the property is indexed - but the one-argument call + * below dispatches to the unrelated getItem(String) overload, because the argument types choose the + * method and the caller chooses the arguments. + */ + public void testDenyMethodExecutionBlocksAnOverloadOfAnIndexedAccessor() { + OverloadedIndexedBean bean = new OverloadedIndexedBean(); + ValueStack vs = ActionContext.getContext().getValueStack(); + vs.push(bean); + ReflectionContextState.setDenyMethodExecution(vs.getContext(), true); + + vs.findValue("getItem('PWNED')"); + + assertNull("an overload sharing an indexed accessor's name must not be executed while method" + + " execution is denied", bean.overloadArgument); + } + + /** + * The direction matters too: a read-only indexed property must not legitimise an unrelated two-argument + * setter that merely shares its name. + */ + public void testDenyMethodExecutionBlocksUnrelatedSetterNamedAfterAReadOnlyIndexedProperty() { + ReadOnlyIndexedBean bean = new ReadOnlyIndexedBean(); + ValueStack vs = ActionContext.getContext().getValueStack(); + vs.push(bean); + ReflectionContextState.setDenyMethodExecution(vs.getContext(), true); + + vs.findValue("setItem('PWNED', 'x')"); + + assertNull("a two-argument setter is not the accessor of a read-only indexed property and must not" + + " be executed while method execution is denied", bean.setterArgument); + } + + 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); + } + + /** + * The object indexed pair is what parameter binding itself walks through, so the mutator half has to keep + * working under the deny flag exactly as the accessor half does. + */ + public void testDenyMethodExecutionAllowsObjectIndexedPropertyMutator() { + Bean bean = new Bean(); + ValueStack vs = ActionContext.getContext().getValueStack(); + vs.push(bean); + ReflectionContextState.setDenyMethodExecution(vs.getContext(), true); + + vs.findValue("setKeyed('k', 'v')"); + + assertEquals("object indexed property mutators must keep working while method execution is denied", + "k=v", bean.keyedArgument); + } + + /** + * The prefix is half of what makes a call a candidate: a method taking the right number of arguments but + * named nothing like an accessor never reaches the property lookup at all. + */ + public void testDenyMethodExecutionBlocksAnUnprefixedOneArgumentMethod() { + Bean bean = new Bean(); + ValueStack vs = ActionContext.getContext().getValueStack(); + vs.push(bean); + ReflectionContextState.setDenyMethodExecution(vs.getContext(), true); + + vs.findValue("attack('PWNED')"); + + assertNull("a one argument method without the get prefix must not be executed while method" + + " execution is denied", bean.unprefixedArgument); + } + + public void testDenyMethodExecutionBlocksAnUnprefixedTwoArgumentMethod() { + Bean bean = new Bean(); + ValueStack vs = ActionContext.getContext().getValueStack(); + vs.push(bean); + ReflectionContextState.setDenyMethodExecution(vs.getContext(), true); + + vs.findValue("attack('PWNED', 'x')"); + + assertNull("a two argument method without the set prefix must not be executed while method" + + " execution is denied", bean.unprefixedArgument); + } + + /** + * The overload guard is scoped to the argument count, because that is what OGNL dispatches on. A method + * sharing the accessor's name but taking a different number of arguments is never a candidate for this + * call, and so must not cost the bean its indexed property access. + */ + public void testDenyMethodExecutionAllowsIndexedAccessorWithAnOverloadOfAnotherArity() { + DifferentArityOverloadBean bean = new DifferentArityOverloadBean(); + ValueStack vs = ActionContext.getContext().getValueStack(); + vs.push(bean); + ReflectionContextState.setDenyMethodExecution(vs.getContext(), true); + + Object value = vs.findValue("getItem(1)"); + + assertEquals("an overload of another arity cannot be dispatched to and must not block indexed" + + " property access", "item1", value); + assertNull("the overload itself must not have been executed", bean.overloadArgument); + } + + /** + * {@link ReflectionContextState#DENY_INDEXED_ACCESS_EXECUTION} is deprecated because Struts itself never + * sets it, but application and plugin code still can, and while it does the indexed accessor exemption + * has to stay switched off. That is the whole reason the key is deprecated rather than removed outright. + */ + @SuppressWarnings("removal") + public void testDenyIndexedAccessExecutionSuppressesTheIndexedAccessorExemption() { + Bean bean = new Bean(); + ValueStack vs = ActionContext.getContext().getValueStack(); + vs.push(bean); + ReflectionContextState.setDenyMethodExecution(vs.getContext(), true); + vs.getContext().put(ReflectionContextState.DENY_INDEXED_ACCESS_EXECUTION, Boolean.TRUE); + + Object value = vs.findValue("getItem(1)"); + + assertNull("setting the legacy key must suppress the indexed accessor exemption", value); + } + + @SuppressWarnings("removal") + public void testDenyIndexedAccessExecutionSetToFalseLeavesTheIndexedAccessorExemptionInPlace() { + Bean bean = new Bean(); + ValueStack vs = ActionContext.getContext().getValueStack(); + vs.push(bean); + ReflectionContextState.setDenyMethodExecution(vs.getContext(), true); + vs.getContext().put(ReflectionContextState.DENY_INDEXED_ACCESS_EXECUTION, Boolean.FALSE); + + Object value = vs.findValue("getItem(1)"); + + assertEquals("the legacy key set to false must leave indexed property access working", "item1", value); + } + + 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; + private String keyedArgument; + private String unprefixedArgument; + + /** + * 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) { + this.keyedArgument = key + "=" + value; + } + + /** + * Neither prefix, so no property name can be derived from it at all - whatever its argument count. + * Records into its own field, so a test can tell this apart from {@link #getAttack(String)} having run. + */ + public String attack(String argument) { + this.unprefixedArgument = argument; + return "irrelevant"; + } + + public String attack(String argument, String other) { + this.unprefixedArgument = argument + "," + other; + return "irrelevant"; + } + } + + public static class ReadOnlyIndexedBean { + private String setterArgument; + + public String getItem(int index) { + return "item" + index; + } + + /** + * Not the indexed setter of {@code item} - that would be {@code setItem(int, String)}. It only shares + * the name and the two-argument shape. + */ + public void setItem(String key, String value) { + this.setterArgument = key + "=" + value; + } + } + + public static class DifferentArityOverloadBean { + private String overloadArgument; + + public String getItem(int index) { + return "item" + index; + } + + public void setItem(int index, String value) { + // present so that the pair forms an indexed property + } + + /** + * Shares the name but not the argument count, so it is not what a one argument call resolves to. + */ + public String getItem(String key, String other) { + this.overloadArgument = key + "," + other; + return "irrelevant"; + } + } + + public static class OverloadedIndexedBean { + private String overloadArgument; + + 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 getItem(String key) { + this.overloadArgument = key; + return "irrelevant"; + } + } +}