This is an automated email from the ASF dual-hosted git repository.
lukaszlenart pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/struts.git
The following commit(s) were added to refs/heads/main by this push:
new ee7f11d3c WW-5700 fix(ognl): skip the store when a map or list element
cannot be converted (#1873)
ee7f11d3c is described below
commit ee7f11d3cb76ceca88496551b8c21f8606d6cfc6
Author: Lukasz Lenart <[email protected]>
AuthorDate: Mon Aug 31 20:02:32 2026 +0200
WW-5700 fix(ognl): skip the store when a map or list element cannot be
converted (#1873)
XWorkConverter.convertValue() signals failure by returning
TypeConverter.NO_CONVERSION_POSSIBLE, which is itself a plain String,
"ognl.NoConversionPossible". XWorkMapPropertyAccessor and
XWorkListPropertyAccessor stored that return value into the target
collection without checking for it.
Because generics are erased at that point the store succeeds silently,
so the ClassCastException surfaces later, in application code reading
the entry back, with a stack trace that points away from the framework.
Guard both accessors and skip the assignment instead; the conversion
error has already been registered by convertValue(), so nothing is lost.
The map accessor guards the key as well as the value - a key that cannot
be converted poisons iteration over the whole map, not one entry.
Identity comparison is used rather than equals(), matching OGNL's own
guard in OgnlRuntime, so a form legitimately submitting the literal text
"ognl.NoConversionPossible" into a String-valued collection is not
silently discarded.
In the list accessor the guard sits before the auto-grow block so an
unconvertible value does not grow the list.
XWorkCollectionPropertyAccessor carries the same unguarded pattern but is
left untouched: its scalar setProperty path is not reachable through the
value stack, so no failing test could be written for it.
Reported on user@ as "Struts setting a String object instead of Integer
in the form", where an unchecked s:checkbox with submitUnchecked="true"
submits the CheckboxInterceptor uncheckedValue "false" into a
Map<Long, Integer>.
Co-authored-by: Claude Opus 5 <[email protected]>
---
.../ognl/accessor/XWorkListPropertyAccessor.java | 9 +++++
.../ognl/accessor/XWorkMapPropertyAccessor.java | 12 +++++-
.../parameter/ParametersInterceptorTest.java | 39 +++++++++++++++++++
.../accessor/XWorkListPropertyAccessorTest.java | 16 ++++++++
.../accessor/XWorkMapPropertyAccessorTest.java | 45 ++++++++++++++++++++++
5 files changed, 120 insertions(+), 1 deletion(-)
diff --git
a/core/src/main/java/org/apache/struts2/ognl/accessor/XWorkListPropertyAccessor.java
b/core/src/main/java/org/apache/struts2/ognl/accessor/XWorkListPropertyAccessor.java
index e741877cb..591d822b6 100644
---
a/core/src/main/java/org/apache/struts2/ognl/accessor/XWorkListPropertyAccessor.java
+++
b/core/src/main/java/org/apache/struts2/ognl/accessor/XWorkListPropertyAccessor.java
@@ -20,6 +20,7 @@ package org.apache.struts2.ognl.accessor;
import org.apache.struts2.ObjectFactory;
import org.apache.struts2.conversion.ObjectTypeDeterminer;
+import org.apache.struts2.conversion.TypeConverter;
import org.apache.struts2.conversion.impl.XWorkConverter;
import org.apache.struts2.inject.Inject;
import org.apache.struts2.ognl.OgnlUtil;
@@ -30,6 +31,8 @@ import ognl.OgnlException;
import ognl.PropertyAccessor;
import org.apache.struts2.StrutsConstants;
import org.apache.struts2.StrutsException;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
import java.util.Collection;
import java.util.List;
@@ -43,6 +46,8 @@ import java.util.List;
*/
public class XWorkListPropertyAccessor extends ListPropertyAccessor {
+ private static final Logger LOG =
LogManager.getLogger(XWorkListPropertyAccessor.class);
+
private XWorkCollectionPropertyAccessor _sAcc = new
XWorkCollectionPropertyAccessor();
private XWorkConverter xworkConverter;
@@ -167,6 +172,10 @@ public class XWorkListPropertyAccessor extends
ListPropertyAccessor {
}
Object realValue = getRealValue(context, value, convertToClass);
+ if (realValue == TypeConverter.NO_CONVERSION_POSSIBLE) {
+ LOG.debug("Unable to convert value for index [{}] to the declared
element type, skipping assignment", name);
+ return;
+ }
if (target instanceof List list && name instanceof Number) {
//make sure there are enough spaces in the List to set
diff --git
a/core/src/main/java/org/apache/struts2/ognl/accessor/XWorkMapPropertyAccessor.java
b/core/src/main/java/org/apache/struts2/ognl/accessor/XWorkMapPropertyAccessor.java
index f15223fdf..5b5a6aa14 100644
---
a/core/src/main/java/org/apache/struts2/ognl/accessor/XWorkMapPropertyAccessor.java
+++
b/core/src/main/java/org/apache/struts2/ognl/accessor/XWorkMapPropertyAccessor.java
@@ -20,6 +20,7 @@ package org.apache.struts2.ognl.accessor;
import org.apache.struts2.ObjectFactory;
import org.apache.struts2.conversion.ObjectTypeDeterminer;
+import org.apache.struts2.conversion.TypeConverter;
import org.apache.struts2.conversion.impl.XWorkConverter;
import org.apache.struts2.inject.Inject;
import org.apache.struts2.util.reflection.ReflectionContextState;
@@ -127,8 +128,17 @@ public class XWorkMapPropertyAccessor extends
MapPropertyAccessor {
LOG.trace("Entering setProperty({},{},{},{})", context, target, name,
value);
Object key = getKey(context, name);
+ if (key == TypeConverter.NO_CONVERSION_POSSIBLE) {
+ LOG.debug("Unable to convert key [{}] to the declared key type,
skipping assignment", name);
+ return;
+ }
+ Object convertedValue = getValue(context, value);
+ if (convertedValue == TypeConverter.NO_CONVERSION_POSSIBLE) {
+ LOG.debug("Unable to convert value for key [{}] to the declared
element type, skipping assignment", key);
+ return;
+ }
Map map = (Map) target;
- map.put(key, getValue(context, value));
+ map.put(key, convertedValue);
}
private Object getValue(OgnlContext context, Object value) {
diff --git
a/core/src/test/java/org/apache/struts2/interceptor/parameter/ParametersInterceptorTest.java
b/core/src/test/java/org/apache/struts2/interceptor/parameter/ParametersInterceptorTest.java
index ba2f48969..2b1b4386c 100644
---
a/core/src/test/java/org/apache/struts2/interceptor/parameter/ParametersInterceptorTest.java
+++
b/core/src/test/java/org/apache/struts2/interceptor/parameter/ParametersInterceptorTest.java
@@ -48,6 +48,7 @@ import org.apache.struts2.action.ParameterNameAware;
import org.apache.struts2.action.ParameterValueAware;
import org.apache.struts2.config.StrutsXmlConfigurationProvider;
import org.apache.struts2.dispatcher.HttpParameters;
+import org.apache.struts2.util.Element;
import org.junit.Assert;
import java.io.File;
@@ -1013,6 +1014,44 @@ public class ParametersInterceptorTest extends
XWorkTestCase {
container.inject(config.getInterceptors().get(0).getInterceptor());
}
+ /**
+ * WW-5700: a value that cannot be converted to the map's element type
must not be stored.
+ * An unchecked s:checkbox with submitUnchecked="true" submits the
CheckboxInterceptor's
+ * uncheckedValue, "false", which cannot become an Integer.
+ */
+ public void testUnconvertibleValueIsNotBoundIntoTypedMap() {
+ CheckboxAction action = new CheckboxAction();
+ ValueStack vs = ActionContext.getContext().getValueStack();
+ vs.push(action);
+
+ ParametersInterceptor pi = new ParametersInterceptor();
+ container.inject(pi);
+
+ Map<String, Object> params = new HashMap<>();
+ params.put("capDeferral[100]", "1");
+ params.put("capDeferral[200]", "false");
+
+ pi.applyParameters(action, vs, HttpParameters.create(params).build());
+
+ Map<Long, Integer> capDeferral = action.getCapDeferral();
+ assertEquals("sanity: the convertible value must still bind",
Integer.valueOf(1), capDeferral.get(100L));
+ for (Object entry : ((Map) capDeferral).entrySet()) {
+ Map.Entry e = (Map.Entry) entry;
+ assertTrue("key is not a Long: " + e.getKey(), e.getKey()
instanceof Long);
+ assertTrue("value is not an Integer: " + e.getValue(),
e.getValue() instanceof Integer);
+ }
+ }
+
+ public static class CheckboxAction {
+ @Element(value = Integer.class)
+ private final Map<Long, Integer> capDeferral = new HashMap<>();
+
+ @StrutsParameter(depth = 1)
+ public Map<Long, Integer> getCapDeferral() {
+ return capDeferral;
+ }
+ }
+
}
class ValidateAction implements ValidationAware {
diff --git
a/core/src/test/java/org/apache/struts2/ognl/accessor/XWorkListPropertyAccessorTest.java
b/core/src/test/java/org/apache/struts2/ognl/accessor/XWorkListPropertyAccessorTest.java
index c028c98a8..c1a393ac4 100644
---
a/core/src/test/java/org/apache/struts2/ognl/accessor/XWorkListPropertyAccessorTest.java
+++
b/core/src/test/java/org/apache/struts2/ognl/accessor/XWorkListPropertyAccessorTest.java
@@ -64,6 +64,22 @@ public class XWorkListPropertyAccessorTest extends
XWorkTestCase {
assertEquals(myList.size(), vs.findValue("strings.size"));
}
+ public void testUnconvertibleElementIsNotStored() {
+ ValueStack vs = ActionContext.getContext().getValueStack();
+ ListHolder listHolder = new ListHolder();
+ listHolder.setLongs(new ArrayList<>());
+ vs.push(listHolder);
+
+ vs.setValue("longs[0]", "1");
+ vs.setValue("longs[1]", "not-a-number");
+
+ assertEquals(Long.valueOf(1), listHolder.getLongs().get(0));
+ for (Object element : (List) listHolder.getLongs()) {
+ assertTrue("list must not hold a non-Long element: " + element,
+ element == null || element instanceof Long);
+ }
+ }
+
public void testAutoGrowthCollectionLimit() {
PropertyAccessor accessor =
container.getInstance(PropertyAccessor.class, ArrayList.class.getName());
((XWorkListPropertyAccessor) accessor).setAutoGrowCollectionLimit("2");
diff --git
a/core/src/test/java/org/apache/struts2/ognl/accessor/XWorkMapPropertyAccessorTest.java
b/core/src/test/java/org/apache/struts2/ognl/accessor/XWorkMapPropertyAccessorTest.java
index 7a1d9cd9c..8bc67fe4d 100644
---
a/core/src/test/java/org/apache/struts2/ognl/accessor/XWorkMapPropertyAccessorTest.java
+++
b/core/src/test/java/org/apache/struts2/ognl/accessor/XWorkMapPropertyAccessorTest.java
@@ -25,6 +25,7 @@ import org.apache.struts2.util.ValueStack;
import org.apache.struts2.util.reflection.ReflectionContextState;
import java.util.Collections;
+import java.util.HashMap;
import java.util.Map;
public class XWorkMapPropertyAccessorTest extends XWorkTestCase {
@@ -57,6 +58,50 @@ public class XWorkMapPropertyAccessorTest extends
XWorkTestCase {
assertNull(vs.findValue("map['key']"));
}
+ public void testUnconvertibleValueIsNotStored() {
+ TypedMapHolder holder = new TypedMapHolder();
+ ValueStack vs = ActionContext.getContext().getValueStack();
+ vs.push(holder);
+
+ vs.setValue("counts[1]", "5");
+ vs.setValue("counts[2]", "not-a-number");
+
+ assertEquals(Integer.valueOf(5), holder.getCounts().get(1L));
+ assertOnlyDeclaredTypes(holder.getCounts());
+ }
+
+ public void testUnconvertibleKeyIsNotStored() {
+ TypedMapHolder holder = new TypedMapHolder();
+ ValueStack vs = ActionContext.getContext().getValueStack();
+ vs.push(holder);
+
+ vs.setValue("counts[1]", "5");
+ vs.setValue("counts['abc']", "6");
+
+ assertEquals(Integer.valueOf(5), holder.getCounts().get(1L));
+ assertOnlyDeclaredTypes(holder.getCounts());
+ }
+
+ /**
+ * A Map declared to hold Long keys and Integer values must never be left
holding anything else.
+ */
+ private static void assertOnlyDeclaredTypes(Map<Long, Integer> map) {
+ for (Object o : ((Map) map).entrySet()) {
+ Map.Entry entry = (Map.Entry) o;
+ assertTrue("key is not a Long: " + entry.getKey(), entry.getKey()
instanceof Long);
+ assertTrue("value is not an Integer: " + entry.getValue(),
entry.getValue() instanceof Integer);
+ }
+ }
+
+ public static class TypedMapHolder {
+ @Element(value = Integer.class)
+ private final Map<Long, Integer> counts = new HashMap<>();
+
+ public Map<Long, Integer> getCounts() {
+ return counts;
+ }
+ }
+
private static class MapHolder {
private final Map map;
