This is an automated email from the ASF dual-hosted git repository. lukaszlenart pushed a commit to branch WW-5700-no-conversion-possible-guard-6x in repository https://gitbox.apache.org/repos/asf/struts.git
commit 342c9b130dd6f8395395115aaa7d4b78e9e06b01 Author: Lukasz Lenart <[email protected]> AuthorDate: Thu Aug 27 18:18:40 2026 +0200 WW-5700 fix(ognl): skip the store when a map or list element cannot be converted Backport of the 7.4.0 fix (#1873) to the 6.x line. When XWorkConverter cannot convert a value it returns the marker string NO_CONVERSION_POSSIBLE. The map and list property accessors stored that marker straight into the target collection, so a Map<Long, Integer> could end up holding the String "ognl.NoConversionPossible" under a String key, and the next read of that collection failed with a ClassCastException far from the cause. Skip the assignment instead and log at debug. Co-Authored-By: Claude Opus 5 <[email protected]> --- .../ognl/accessor/XWorkListPropertyAccessor.java | 9 +++++ .../ognl/accessor/XWorkMapPropertyAccessor.java | 12 +++++- .../accessor/XWorkListPropertyAccessorTest.java | 16 ++++++++ .../accessor/XWorkMapPropertyAccessorTest.java | 45 ++++++++++++++++++++++ .../parameter/ParametersInterceptorTest.java | 39 +++++++++++++++++++ 5 files changed, 120 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/com/opensymphony/xwork2/ognl/accessor/XWorkListPropertyAccessor.java b/core/src/main/java/com/opensymphony/xwork2/ognl/accessor/XWorkListPropertyAccessor.java index 47a640438..4e5048527 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/accessor/XWorkListPropertyAccessor.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/accessor/XWorkListPropertyAccessor.java @@ -20,6 +20,7 @@ package com.opensymphony.xwork2.ognl.accessor; import com.opensymphony.xwork2.ObjectFactory; import com.opensymphony.xwork2.conversion.ObjectTypeDeterminer; +import com.opensymphony.xwork2.conversion.TypeConverter; import com.opensymphony.xwork2.conversion.impl.XWorkConverter; import com.opensymphony.xwork2.inject.Inject; import com.opensymphony.xwork2.ognl.OgnlUtil; @@ -29,6 +30,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.Map; */ 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 && name instanceof Number) { //make sure there are enough spaces in the List to set diff --git a/core/src/main/java/com/opensymphony/xwork2/ognl/accessor/XWorkMapPropertyAccessor.java b/core/src/main/java/com/opensymphony/xwork2/ognl/accessor/XWorkMapPropertyAccessor.java index bad7932ce..dd465fd48 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/accessor/XWorkMapPropertyAccessor.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/accessor/XWorkMapPropertyAccessor.java @@ -20,6 +20,7 @@ package com.opensymphony.xwork2.ognl.accessor; import com.opensymphony.xwork2.ObjectFactory; import com.opensymphony.xwork2.conversion.ObjectTypeDeterminer; +import com.opensymphony.xwork2.conversion.TypeConverter; import com.opensymphony.xwork2.conversion.impl.XWorkConverter; import com.opensymphony.xwork2.inject.Inject; import com.opensymphony.xwork2.util.reflection.ReflectionContextState; @@ -126,8 +127,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(Map context, Object value) { diff --git a/core/src/test/java/com/opensymphony/xwork2/ognl/accessor/XWorkListPropertyAccessorTest.java b/core/src/test/java/com/opensymphony/xwork2/ognl/accessor/XWorkListPropertyAccessorTest.java index 4a89d8935..9df833198 100644 --- a/core/src/test/java/com/opensymphony/xwork2/ognl/accessor/XWorkListPropertyAccessorTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/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/com/opensymphony/xwork2/ognl/accessor/XWorkMapPropertyAccessorTest.java b/core/src/test/java/com/opensymphony/xwork2/ognl/accessor/XWorkMapPropertyAccessorTest.java index 1835e938a..b0629c1ef 100644 --- a/core/src/test/java/com/opensymphony/xwork2/ognl/accessor/XWorkMapPropertyAccessorTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/ognl/accessor/XWorkMapPropertyAccessorTest.java @@ -25,6 +25,7 @@ import com.opensymphony.xwork2.util.ValueStack; import com.opensymphony.xwork2.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; 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 cf87495bc..2101b701d 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 @@ -38,6 +38,7 @@ import com.opensymphony.xwork2.ognl.OgnlValueStack; import com.opensymphony.xwork2.ognl.OgnlValueStackFactory; import com.opensymphony.xwork2.ognl.accessor.CompoundRootAccessor; import com.opensymphony.xwork2.ognl.accessor.RootAccessor; +import com.opensymphony.xwork2.util.Element; import com.opensymphony.xwork2.util.ValueStack; import com.opensymphony.xwork2.util.ValueStackFactory; import com.opensymphony.xwork2.util.reflection.ReflectionContextState; @@ -1012,6 +1013,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 {
