[
https://issues.apache.org/jira/browse/WW-5701?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Work on WW-5701 started by Lukasz Lenart.
-----------------------------------------
> CollectionConverter silently drops a legitimate element whose text equals the
> NO_CONVERSION_POSSIBLE marker
> -----------------------------------------------------------------------------------------------------------
>
> Key: WW-5701
> URL: https://issues.apache.org/jira/browse/WW-5701
> Project: Struts 2
> Issue Type: Bug
> Reporter: Lukasz Lenart
> Assignee: Lukasz Lenart
> Priority: Major
> Time Spent: 10m
> Remaining Estimate: 0h
>
> h2. Summary
> CollectionConverter decides whether an element converted successfully by
> comparing the converted value to the marker constant
> TypeConverter.NO\_CONVERSION\_POSSIBLE with equals() rather than reference
> identity. The marker's value is the ordinary text
> "ognl.NoConversionPossible", so a legitimate element whose content happens to
> be that text is mistaken for a conversion failure and silently dropped from
> the resulting collection.
> h2. Reproduced
> On main at 05ad78a06. A plain action property declared as a List of String,
> bound from a normal multi-valued request parameter:
> {noformat}
> vs.setValue("names", new String[]{"alpha", "ognl.NoConversionPossible",
> "omega"});
> result: [alpha, omega]
> expected: [alpha, ognl.NoConversionPossible, omega]
> {noformat}
> The middle element is discarded. No conversion error is registered, because
> no conversion actually failed - the element converted fine and was then
> thrown away by the guard. So the loss is entirely silent: the action sees a
> shorter collection with no indication anything happened.
> h2. Where
> Three sites in
> core/src/main/java/org/apache/struts2/conversion/impl/CollectionConverter.java,
> all using equals() against the marker:
> * line 64 - the array branch
> * line 75 - the Collection branch
> * line 83 - the single-value branch
> Note the exposure is wider than "collections explicitly declared to hold
> Strings". At lines 48-50 of the same file, when no element type can be
> determined the member type defaults to String.class, so an untyped collection
> is affected too.
> h2. Why identity is correct
> The constant is declared as Object, not String (TypeConverter.java:49), so it
> is not a JLS constant variable and is not inlined at compile time - every
> reader loads the one shared instance. A value arriving from an HTTP request
> is a distinct object even when its content is identical, so reference
> comparison distinguishes "the converter signalled failure" from "the user
> submitted this text", which is exactly the distinction being made here.
> OGNL itself compares this marker by identity - its own conversion loop
> compiles to an if\_acmpne against the same field.
> h2. Proposed fix
> Replace equals() with reference comparison at all three sites.
> h2. Relationship to WW-5700
> WW-5700 fixed the mirror-image defect in XWorkMapPropertyAccessor and
> XWorkListPropertyAccessor, where the marker was stored into a typed
> collection instead of being skipped; that fix deliberately used identity
> comparison for the reason given above. CollectionConverter is the older code
> that already had a guard, but used the weaker comparison. The two were found
> together during review of WW-5700 (PR #1873) and are separate defects:
> WW-5700 stores something it should not, this one drops something it should
> keep.
> h2. Backward compatibility
> Narrow, and strictly a fix. The only behaviour that changes is that an
> element whose text is exactly "ognl.NoConversionPossible" is now kept rather
> than discarded. Nothing can reasonably depend on the current silent removal.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
