WW-5700 fix(ognl): skip the store when a map or list element cannot be converted

core/src/main/java/org/apache/struts2/ognl/accessor/XWorkListPropertyAccessor.java

@@ -20,6 +20,7 @@
 
 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.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 @@
  */
 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 void setProperty(OgnlContext context, Object target, Object name, Object
         }
 
         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

core/src/main/java/org/apache/struts2/ognl/accessor/XWorkMapPropertyAccessor.java

@@ -20,6 +20,7 @@
 
 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 void setProperty(OgnlContext context, Object target, Object name, Object
         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) {

core/src/test/java/org/apache/struts2/interceptor/parameter/ParametersInterceptorTest.java

@@ -48,6 +48,7 @@
 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 @@ protected void setUp() throws Exception {
         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 {

core/src/test/java/org/apache/struts2/ognl/accessor/XWorkListPropertyAccessorTest.java

@@ -64,6 +64,22 @@ public void testCanAccessListSizeProperty() {
         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");

core/src/test/java/org/apache/struts2/ognl/accessor/XWorkMapPropertyAccessorTest.java

@@ -25,6 +25,7 @@
 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 void testNullIsReturnedWhenCreateNullObjectsIsSpecifiedAsFalse() {
         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;