From 04f63bee49612a3283a5a7c2e1a72b2d9644c439 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Thu, 27 Aug 2026 07:14:55 +0200 Subject: [PATCH 1/6] WW-5697 fix(ognl): restrict the indexed-access fast path to real indexed properties XWorkMethodAccessor.callMethod skipped the denyMethodExecution check for any method whose name began with "get" and took one argument, or "set" and took two. That test is a name prefix plus an argument count, not a property check, so an ordinary method such as getSomething(String) qualified and was executed during parameter binding with the argument supplied in the parameter name. The fast path now applies only where the target type genuinely declares an indexed property accessor, determined with OgnlRuntime.getIndexedPropertyType. Anything else falls through to the existing denyMethodExecution check. Both int-indexed and object-indexed accessors continue to work. The new tests cover those two, the argument-taking method that must now be blocked while method execution is denied, and the unset-flag path where methods still execute as before, so the change is confined to parameter binding. DENY_INDEXED_ACCESS_EXECUTION is left in place for now; it is public API and is never set anywhere, so its removal is handled separately. Co-Authored-By: Claude Opus 5 --- .../ognl/accessor/XWorkMethodAccessor.java | 33 +++++- .../accessor/XWorkMethodAccessorTest.java | 103 ++++++++++++++++++ 2 files changed, 131 insertions(+), 5 deletions(-) create mode 100644 core/src/test/java/org/apache/struts2/ognl/accessor/XWorkMethodAccessorTest.java 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..c7bb1796f2 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,12 +79,16 @@ public Object callMethod(OgnlContext context, Object object, String string, Obje } - //HACK - we pass indexed method access i.e. setXXX(A,B) pattern + //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"))) { - Boolean exec = (Boolean) context.get(ReflectionContextState.DENY_INDEXED_ACCESS_EXECUTION); - boolean e = exec != null && exec; - if (!e) { - return callMethodWithDebugInfo(context, object, string, objects); + if (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); + } } } boolean e = ReflectionContextState.isDenyMethodExecution(context); @@ -94,6 +100,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/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..dcee2f3ad4 --- /dev/null +++ b/core/src/test/java/org/apache/struts2/ognl/accessor/XWorkMethodAccessorTest.java @@ -0,0 +1,103 @@ +/* + * 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 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; + + /** + * 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 + } + } +} From d1176ffe903e56cd0dae6bb0eea020f2bcd0caad Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Thu, 27 Aug 2026 07:23:16 +0200 Subject: [PATCH 2/6] WW-5697 chore(ognl): deprecate DENY_INDEXED_ACCESS_EXECUTION Nothing in the framework has ever written this key, so the check it guarded in XWorkMethodAccessor never fired. Now that indexed property access is identified from the target type rather than from a method name prefix, the flag has nothing left to guard. It is public API, so it is deprecated here rather than deleted, and removal is tracked for 8.0.0 in WW-5699. Co-Authored-By: Claude Opus 5 --- .../struts2/util/reflection/ReflectionContextState.java | 7 +++++++ 1 file changed, 7 insertions(+) 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..aa387c62df 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 public static final String DENY_INDEXED_ACCESS_EXECUTION = "xwork.IndexedPropertyAccessor.denyMethodExecution"; public static boolean isCreatingNullObjects(Map context) { From 368e561e9ad54866811266e50c01ab6d2eefe5c2 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Thu, 27 Aug 2026 07:36:16 +0200 Subject: [PATCH 3/6] WW-5697 refactor(ognl): address SonarQube findings Merge the nested indexed-property check into the enclosing condition (S1066) and give the deprecation its since/forRemoval arguments (S6355). Also cover the branch that rejects a method with nothing left after the "get" prefix, using a map style get(String) accessor. That is worth asserting in its own right: such a method is not an indexed property accessor, so it must not be executed while method execution is denied. Co-Authored-By: Claude Opus 5 --- .../ognl/accessor/XWorkMethodAccessor.java | 13 ++++++------ .../reflection/ReflectionContextState.java | 2 +- .../accessor/XWorkMethodAccessorTest.java | 21 +++++++++++++++++++ 3 files changed, 28 insertions(+), 8 deletions(-) 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 c7bb1796f2..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 @@ -82,13 +82,12 @@ public Object callMethod(OgnlContext context, Object object, String string, Obje //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"))) { - if (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); - } + 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); } } boolean e = ReflectionContextState.isDenyMethodExecution(context); 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 aa387c62df..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 @@ -44,7 +44,7 @@ public class ReflectionContextState { * 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 + @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 index dcee2f3ad4..a460fab9c2 100644 --- a/core/src/test/java/org/apache/struts2/ognl/accessor/XWorkMethodAccessorTest.java +++ b/core/src/test/java/org/apache/struts2/ognl/accessor/XWorkMethodAccessorTest.java @@ -61,6 +61,18 @@ public void testDenyMethodExecutionAllowsObjectIndexedPropertyAccessor() { "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(); @@ -74,6 +86,15 @@ public void testArgumentTakingGetterIsExecutedWhenMethodExecutionIsNotDenied() { 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 From 8fc78cee8d9e17e3b8df7a5e3e2aabeb387f1213 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Thu, 27 Aug 2026 18:40:30 +0200 Subject: [PATCH 4/6] WW-5697 fix(ognl): identify the indexed accessor by method, not by property name Addresses review of #1871. Keying the check on the property name left two ways through. A class declaring the indexed pair getItem(int)/setItem(int, String) may also declare an unrelated getItem(String) overload, and a one-argument call dispatches to that overload, because the argument types choose the method and the caller chooses the arguments. And the check was direction-agnostic, so a read-only getItem(int) legitimised an unrelated two-argument setItem(String, String). Both executed while method execution was denied. The descriptor's own indexed accessor must now be the method that will actually run: same name, same direction, and no same-arity overload for the dispatcher to prefer instead. The deny check is hoisted ahead of the indexed-property block, which it now guards. The two are equivalent - with execution permitted, both paths ended in the same call - but this way the introspection is skipped entirely on the common path, and the block reads as the exception it is. Also reword the deprecation javadoc, which claimed the key had never had any effect: application code that sets it does still suppress the fast path. Suppress the removal warning at the framework's own read of it. Tests: an overload of an indexed accessor, and an unrelated setter named after a read-only indexed property, are both blocked while execution is denied. Both fail against the previous predicate. A read-only int-indexed getter is added because it is the only shape that reaches INDEXED_PROPERTY_INT - OGNL reclassifies a get/set pair as _OBJECT - so the existing tests never covered that branch. Full core suite: 3205 tests, 0 failures. Co-Authored-By: Claude Opus 5 --- .../ognl/accessor/XWorkMethodAccessor.java | 82 +++++++++++++---- .../reflection/ReflectionContextState.java | 9 +- .../accessor/XWorkMethodAccessorTest.java | 91 ++++++++++++++++++- 3 files changed, 158 insertions(+), 24 deletions(-) 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 9c3a3bcd99..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 @@ -22,16 +22,20 @@ 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} @@ -79,43 +83,83 @@ public Object callMethod(OgnlContext context, Object object, String string, Obje } - //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); - } + 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 {@code methodName} is an indexed property accessor on the target type, as opposed to an ordinary + * 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) { - if (object == null || methodName.length() <= 3) { + 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 { - return OgnlRuntime.getIndexedPropertyType(object.getClass(), propertyName) != OgnlRuntime.INDEXED_PROPERTY_NONE; + 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, object.getClass(), 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 { 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 d1cb8fb5b7..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 @@ -39,10 +39,11 @@ public class ReflectionContextState { 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, 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"; 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 index a460fab9c2..e5d8fca3e7 100644 --- a/core/src/test/java/org/apache/struts2/ognl/accessor/XWorkMethodAccessorTest.java +++ b/core/src/test/java/org/apache/struts2/ognl/accessor/XWorkMethodAccessorTest.java @@ -37,7 +37,13 @@ public void testDenyMethodExecutionBlocksArgumentTakingGetterThatIsNotAnIndexedP + " executed while method execution is denied", bean.attackArgument); } - public void testDenyMethodExecutionAllowsIntIndexedPropertyAccessor() { + /** + * 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); @@ -61,6 +67,56 @@ public void testDenyMethodExecutionAllowsObjectIndexedPropertyAccessor() { "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(); @@ -121,4 +177,37 @@ public void setKeyed(String key, String value) { // present so that the pair forms an indexed property } } + + 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; + } + } + + 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"; + } + } } From 0c6c878add9e5403e70383da373ab93d47484f3f Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Fri, 28 Aug 2026 07:42:26 +0200 Subject: [PATCH 5/6] WW-5697 test(ognl): cover the branches of the indexed accessor fast path SonarCloud failed the quality gate on PR #1871 at 79.17% new-code coverage. The uncovered branches were all real behaviour that nothing asserted: - the deprecated DENY_INDEXED_ACCESS_EXECUTION key, both set and set to false. Its javadoc claims application code can still suppress the exemption with it, which is the reason it was deprecated rather than removed, and nothing tested that claim. - the object-indexed mutator half of the pair, which parameter binding itself walks through. - methods carrying neither prefix, which never reach the property lookup. - an overload of another arity, which cannot be dispatched to and so must not cost the bean its indexed property access. Each new test was checked by mutation: making the predicate always true, never honouring the legacy key, always honouring it, and dropping the argument-count filter each fail exactly the tests that assert that branch. New-code coverage goes from 79.17% to roughly 92%. What stays uncovered is defensive only: the null target, the OgnlException catch, and the two guards against a descriptor accessor that disagrees with the invoked method. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01P9Pjt4rvb1ASASjSTHsUhL --- .../accessor/XWorkMethodAccessorTest.java | 129 +++++++++++++++++- 1 file changed, 128 insertions(+), 1 deletion(-) 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 index e5d8fca3e7..43afb578c6 100644 --- a/core/src/test/java/org/apache/struts2/ognl/accessor/XWorkMethodAccessorTest.java +++ b/core/src/test/java/org/apache/struts2/ognl/accessor/XWorkMethodAccessorTest.java @@ -129,6 +129,99 @@ public void testDenyMethodExecutionBlocksBareGetAccessor() { + " 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.attackArgument); + } + + 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.attackArgument); + } + + /** + * 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(); @@ -143,6 +236,7 @@ public void testArgumentTakingGetterIsExecutedWhenMethodExecutionIsNotDenied() { public static class Bean { private String attackArgument; private String bareGetArgument; + private String keyedArgument; /** * Named exactly "get", so there is no property name left once the prefix is removed. @@ -174,7 +268,20 @@ public String getKeyed(String key) { } public void setKeyed(String key, String value) { - // present so that the pair forms an indexed property + this.keyedArgument = key + "=" + value; + } + + /** + * Neither prefix, so no property name can be derived from it at all - whatever its argument count. + */ + public String attack(String argument) { + this.attackArgument = argument; + return "irrelevant"; + } + + public String attack(String argument, String other) { + this.attackArgument = argument; + return "irrelevant"; } } @@ -194,6 +301,26 @@ public void setItem(String key, String 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; + return "irrelevant"; + } + } + public static class OverloadedIndexedBean { private String overloadArgument; From c179958cd2fc6d7a9dce33b92afddac42649da8e Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Fri, 28 Aug 2026 08:54:06 +0200 Subject: [PATCH 6/6] WW-5697 test(ognl): clear the four SonarCloud smells in the accessor fixtures The gate passes, but four issues stand, all in the test beans. S4144 is the one worth having: attack(String) and getAttack(String) had identical bodies because both recorded into attackArgument, so neither of the tests asserting on that field could tell which of the two methods had run. The unprefixed pair now records into its own field, which is what the tests naming it actually mean to assert. The three S1172s are unused second parameters on fixtures whose two-argument shape is the whole point, so the parameter cannot be removed. Each now records the full call instead of only its first argument, which is what the surrounding fixtures already did and costs nothing. Mutation still holds: forcing isIndexedPropertyAccessor to accept everything fails all six "blocks" tests, the two switched to the new field included. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01P9Pjt4rvb1ASASjSTHsUhL --- .../ognl/accessor/XWorkMethodAccessorTest.java | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) 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 index 43afb578c6..9fa17d1bcd 100644 --- a/core/src/test/java/org/apache/struts2/ognl/accessor/XWorkMethodAccessorTest.java +++ b/core/src/test/java/org/apache/struts2/ognl/accessor/XWorkMethodAccessorTest.java @@ -158,7 +158,7 @@ public void testDenyMethodExecutionBlocksAnUnprefixedOneArgumentMethod() { vs.findValue("attack('PWNED')"); assertNull("a one argument method without the get prefix must not be executed while method" - + " execution is denied", bean.attackArgument); + + " execution is denied", bean.unprefixedArgument); } public void testDenyMethodExecutionBlocksAnUnprefixedTwoArgumentMethod() { @@ -170,7 +170,7 @@ public void testDenyMethodExecutionBlocksAnUnprefixedTwoArgumentMethod() { vs.findValue("attack('PWNED', 'x')"); assertNull("a two argument method without the set prefix must not be executed while method" - + " execution is denied", bean.attackArgument); + + " execution is denied", bean.unprefixedArgument); } /** @@ -237,6 +237,7 @@ 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. @@ -273,14 +274,15 @@ public void setKeyed(String key, String 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.attackArgument = argument; + this.unprefixedArgument = argument; return "irrelevant"; } public String attack(String argument, String other) { - this.attackArgument = argument; + this.unprefixedArgument = argument + "," + other; return "irrelevant"; } } @@ -297,7 +299,7 @@ public String getItem(int index) { * the name and the two-argument shape. */ public void setItem(String key, String value) { - this.setterArgument = key; + this.setterArgument = key + "=" + value; } } @@ -316,7 +318,7 @@ public void setItem(int index, String value) { * 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; + this.overloadArgument = key + "," + other; return "irrelevant"; } }