[
https://issues.apache.org/jira/browse/WW-5700?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Work on WW-5700 started by Lukasz Lenart.
-----------------------------------------
> Failed type conversion stores the NO_CONVERSION_POSSIBLE marker string into
> typed Maps, Lists and Collections
> -------------------------------------------------------------------------------------------------------------
>
> Key: WW-5700
> URL: https://issues.apache.org/jira/browse/WW-5700
> Project: Struts 2
> Issue Type: Bug
> Reporter: Lukasz Lenart
> Assignee: Lukasz Lenart
> Priority: Major
>
> h2. Summary
> When conversion of a request parameter into a typed collection fails, Struts
> stores its internal
> "conversion failed" marker into the collection instead of skipping the
> assignment. The marker is
> itself a {{java.lang.String}}, so it lands in a collection declared to hold
> some other type. Because
> generics are erased at that point the store succeeds silently, and the
> {{ClassCastException}} is
> deferred until application code reads the entry back.
> The resulting stack trace points at _application_ code rather than at Struts,
> which makes this very
> hard to recognise from a bug report.
> Reported on the user list:
> "Struts setting a String object instead of Integer in the form" (2026-08-14),
> against 7.2.1 and 7.3.0.
> h2. Root cause
> {{TypeConverter.NO\_CONVERSION\_POSSIBLE}} is not a sentinel object. It is a
> plain String:
> {code:java}
> // core/src/main/java/org/apache/struts2/conversion/TypeConverter.java:49
> Object NO\_CONVERSION\_POSSIBLE = "ognl.NoConversionPossible";
> {code}
> {{XWorkConverter.convertValue}} returns it on failure (lines 339, 351, 361),
> _after_ correctly
> registering the conversion error via {{handleConversionException}}. Three
> property accessors then
> store that return value with no guard:
> * {{XWorkMapPropertyAccessor.setProperty}} (~line 127) - both {{getKey()}}
> and {{getValue()}} unguarded
> * {{XWorkListPropertyAccessor.getRealValue}} (line 188)
> * {{XWorkCollectionPropertyAccessor.getRealValue}} (line 263)
> By contrast OGNL itself guards this correctly at {{OgnlRuntime}} line 1323,
> which is why a plain
> (non-collection) property is left untouched on a failed conversion.
> h2. Reproduced
> On {{main}} at 05ad78a06, end-to-end through {{ParametersInterceptor}} with
> {{struts.parameters.requireAnnotations=true}}. Both halves reproduce.
> _Value half_ - the reporter's case. An unchecked {{s:checkbox}} with
> {{submitUnchecked="true"}}
> causes {{CheckboxInterceptor}} to submit the parameter with its
> {{uncheckedValue}}, default
> {{"false"}}. Bound into a HashMap with Long keys and Integer values:
> {noformat}
> key=100 value=[1] (java.lang.Integer)
> key=200 value=[ognl.NoConversionPossible] (java.lang.String)
> conversionErrors={capDeferral[200]=ConversionData@...}
> {noformat}
> _Key half_ - the marker is stored as a map _key_, which breaks iteration over
> the entire map rather
> than a single entry:
> {noformat}
> acceptable=[capDeferral['abc'], capDeferral[7]]
> key=[ognl.NoConversionPossible] (java.lang.String) value=[1]
> key=[7] (java.lang.Long) value=[2]
> conversionErrors={capDeferral['abc']=ConversionData@...}
> {noformat}
> The key half is reachable because {{ACCEPTED\_PATTERNS}} in
> {{DefaultAcceptedPatternsChecker}} is asymmetric: the bare-bracket branch
> accepts digits only,
> {{(\[\d+])}}, but the quoted-key branch accepts word characters,
> {{(\['(\w-?|[一-龥]-?)+'])}}. So {{capDeferral['abc']}} is an accepted
> parameter name even
> where the declared map key type is numeric, and nothing downstream re-checks
> the key against that
> type. This is worth stating explicitly because the natural "map indices are
> numeric" intuition does
> not hold.
> Note the conversion error _is_ reported in both halves. This is therefore not
> a validation bypass:
> it only bites an action that reads the collection without acting on
> conversion errors.
> h2. Proposed fix
> Guard for the marker and skip the store; the error has already been
> registered, so nothing is lost.
> {code:java}
> Object key = getKey(context, name);
> if (key == TypeConverter.NO\_CONVERSION\_POSSIBLE) {
> return;
> }
> Object convertedValue = getValue(context, value);
> if (convertedValue == TypeConverter.NO\_CONVERSION\_POSSIBLE) {
> return;
> }
> map.put(key, convertedValue);
> {code}
> Same guard in the List and Collection accessors, placed immediately after
> {{getRealValue}} and, in
> the List case, _before_ the auto-grow block so an unconvertible value does
> not grow the list.
> Use identity comparison rather than {{equals}}, matching OGNL's own guard:
> the constant is a String
> literal and the converter returns that exact reference, so with {{equals}} a
> form legitimately
> submitting the text "ognl.NoConversionPossible" into a String-valued map
> would be silently
> discarded.
> h2. Precedent
> WW-3762 fixed exactly this class of bug in
> {{XWorkBasicConverter.doConvertToCollection}} back in
> 2.3.3. {{CollectionConverter}} still carries that guard today, three times.
> The property accessors
> were simply never given the equivalent check.
> h2. Backward compatibility
> Narrow. Previously an unconvertible entry was stored as the marker String;
> now nothing is stored.
> Nothing can reasonably depend on the old behaviour, and the conversion error
> is reported either way,
> so validation-driven actions see no change at all.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
