[jira] [Assigned] (WW-5701) CollectionConverter silently drops a legitimate element whose text equals the NO_CONVERSION_POSSIBLE marker

["Lukasz Lenart (Jira)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/WW-5701?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Lukasz Lenart reassigned WW-5701:
---------------------------------

    Assignee: 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)