From cf53094b7e668cac2f815330272e16f9ea5faa6e Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Thu, 27 Aug 2026 07:14:55 +0200 Subject: [PATCH 1/3] 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 0360f733daad1694ed630c03ee2d3a9df585d680 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Thu, 27 Aug 2026 07:23:16 +0200 Subject: [PATCH 2/3] 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 a701c00b60a5c143029ee94c19be13d50802c737 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Thu, 27 Aug 2026 07:36:16 +0200 Subject: [PATCH 3/3] 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