This is an automated email from the ASF dual-hosted git repository. lukaszlenart pushed a commit to branch fix/WW-5700-no-conversion-possible-guard in repository https://gitbox.apache.org/repos/asf/struts.git
commit 1fbba509f209fbd67be8c24ecfbf3e7e01800877 Author: Lukasz Lenart <[email protected]> AuthorDate: Thu Aug 27 10:28:23 2026 +0200 WW-5700 fix(ognl): skip the store when a map or list element cannot be converted 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;
