[
https://issues.apache.org/jira/browse/WW-5700?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Lukasz Lenart updated WW-5700:
------------------------------
Description:
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 as "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.java line 49 declares the marker as an Object field whose value
is the ordinary text "ognl.NoConversionPossible". XWorkConverter.convertValue
returns that marker on failure (lines 339, 351 and 361), _after_ correctly
registering the conversion error via handleConversionException.
Three property accessors then store that return value with no guard:
* XWorkMapPropertyAccessor.setProperty, around line 127 - both the key and the
value are unguarded
* XWorkListPropertyAccessor.getRealValue, line 188
* XWorkCollectionPropertyAccessor.getRealValue, line 263
By contrast OGNL itself guards this correctly in OgnlRuntime at 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 annotation
enforcement enabled. 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 the accepted-parameter-name patterns in
DefaultAcceptedPatternsChecker are asymmetric - the bare-bracket branch accepts
digits only, but the quoted-key branch accepts word characters:
{noformat}
(\[\d+]) bare brackets: digits only
(\['(\w-?|[\u4e00-\u9fa5]-?)+']) quoted key: word characters
{noformat}
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. Fix
Guard for the marker and skip the store; the error has already been registered
by convertValue, so nothing is lost. In the map accessor the key is guarded
before the value is even converted, because a bad key poisons iteration over
the whole map rather than one entry. In the list accessor the guard sits before
the auto-grow block, so an unconvertible value does not grow the list.
The comparison is by reference rather than equals(). That is correct rather
than incidental: the marker field is declared Object, not String, so it is not
a JLS constant variable and is not inlined into referencing class files - every
reference resolves to the one field value at runtime, third-party converters
included. A parameter value built by a servlet container from request bytes is
a distinct object, so reference comparison separates "the converter signalled
failure" from "the user submitted this text".
To be precise about the limit: this protects values arriving from a request,
which is the case that matters here. It does not protect a value that happens
to be interned, since all identical String literals share one instance -
application code calling the converter programmatically with such a literal
would still lose it. Closing that as well would mean giving the marker an
identity no user string can share, which changes a published constant and is a
binary-compatibility question rather than a bug fix.
h2. Not included
XWorkCollectionPropertyAccessor carries the same unguarded pattern but is left
untouched: its scalar setProperty path is not reachable through the value
stack. Setting {{ids[0]}} on a Set is rejected by OGNL before it gets there, so
no failing test could be written for it and it was not changed blind. Verified
independently during review.
h2. Precedent
WW-3762 fixed this same bug class in XWorkBasicConverter.doConvertToCollection
back in 2.3.3, and CollectionConverter still carries that guard today. The
property accessors were simply never given the equivalent check.
WW-5701 is the mirror-image defect found while reviewing this fix:
CollectionConverter's guard uses equals() and therefore drops a legitimate
element whose text genuinely is the marker.
h2. Backward compatibility
Narrow. Previously an unconvertible entry was stored as the marker; 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.
h2. Status
Fixed in PR https://github.com/apache/struts/pull/1873 - three regression
tests, written test-first and mutation-checked, plus an end-to-end test of the
reported checkbox scenario. Full core suite green.
was:
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 as "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.java line 49 declares the marker as an Object field whose value
is the ordinary text "ognl.NoConversionPossible". XWorkConverter.convertValue
returns that marker on failure (lines 339, 351 and 361), _after_ correctly
registering the conversion error via handleConversionException.
Three property accessors then store that return value with no guard:
* XWorkMapPropertyAccessor.setProperty, around line 127 - both the key and the
value are unguarded
* XWorkListPropertyAccessor.getRealValue, line 188
* XWorkCollectionPropertyAccessor.getRealValue, line 263
By contrast OGNL itself guards this correctly in OgnlRuntime at 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 annotation
enforcement enabled. 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 the accepted-parameter-name patterns in
DefaultAcceptedPatternsChecker are asymmetric - the bare-bracket branch accepts
digits only, but the quoted-key branch accepts word characters:
{noformat}
(\[\d+]) bare brackets: digits only
(\['(\w-?|[\u4e00-\u9fa5]-?)+']) quoted key: word characters
{noformat}
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. Fix
Guard for the marker and skip the store; the error has already been registered
by convertValue, so nothing is lost. In the map accessor the key is guarded
before the value is even converted, because a bad key poisons iteration over
the whole map rather than one entry. In the list accessor the guard sits before
the auto-grow block, so an unconvertible value does not grow the list.
The comparison is by reference rather than equals(). That is correct rather
than incidental: the marker field is declared Object, not String, so it is not
a JLS constant variable and is not inlined into referencing class files - every
reference resolves to the one field value at runtime, third-party converters
included. A parameter value built by a servlet container from request bytes is
a distinct object, so reference comparison separates "the converter signalled
failure" from "the user submitted this text".
To be precise about the limit: this protects values arriving from a request,
which is the case that matters here. It does not protect a value that happens
to be interned, since all identical String literals share one instance -
application code calling the converter programmatically with such a literal
would still lose it. Closing that as well would mean giving the marker an
identity no user string can share, which changes a published constant and is a
binary-compatibility question rather than a bug fix.
h2. Not included
XWorkCollectionPropertyAccessor carries the same unguarded pattern but is left
untouched: its scalar setProperty path is not reachable through the value
stack. Setting ids[0] on a Set is rejected by OGNL before it gets there, so no
failing test could be written for it and it was not changed blind. Verified
independently during review.
h2. Precedent
WW-3762 fixed this same bug class in XWorkBasicConverter.doConvertToCollection
back in 2.3.3, and CollectionConverter still carries that guard today. The
property accessors were simply never given the equivalent check.
WW-5701 is the mirror-image defect found while reviewing this fix:
CollectionConverter's guard uses equals() and therefore drops a legitimate
element whose text genuinely is the marker.
h2. Backward compatibility
Narrow. Previously an unconvertible entry was stored as the marker; 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.
h2. Status
Fixed in PR https://github.com/apache/struts/pull/1873 - three regression
tests, written test-first and mutation-checked, plus an end-to-end test of the
reported checkbox scenario. Full core suite green.
> 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
> Time Spent: 20m
> Remaining Estimate: 0h
>
> 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 as "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.java line 49 declares the marker as an Object field whose value
> is the ordinary text "ognl.NoConversionPossible". XWorkConverter.convertValue
> returns that marker on failure (lines 339, 351 and 361), _after_ correctly
> registering the conversion error via handleConversionException.
> Three property accessors then store that return value with no guard:
> * XWorkMapPropertyAccessor.setProperty, around line 127 - both the key and
> the value are unguarded
> * XWorkListPropertyAccessor.getRealValue, line 188
> * XWorkCollectionPropertyAccessor.getRealValue, line 263
> By contrast OGNL itself guards this correctly in OgnlRuntime at 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
> annotation enforcement enabled. 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 the accepted-parameter-name patterns in
> DefaultAcceptedPatternsChecker are asymmetric - the bare-bracket branch
> accepts digits only, but the quoted-key branch accepts word characters:
> {noformat}
> (\[\d+]) bare brackets: digits only
> (\['(\w-?|[\u4e00-\u9fa5]-?)+']) quoted key: word characters
> {noformat}
> 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. Fix
> Guard for the marker and skip the store; the error has already been
> registered by convertValue, so nothing is lost. In the map accessor the key
> is guarded before the value is even converted, because a bad key poisons
> iteration over the whole map rather than one entry. In the list accessor the
> guard sits before the auto-grow block, so an unconvertible value does not
> grow the list.
> The comparison is by reference rather than equals(). That is correct rather
> than incidental: the marker field is declared Object, not String, so it is
> not a JLS constant variable and is not inlined into referencing class files -
> every reference resolves to the one field value at runtime, third-party
> converters included. A parameter value built by a servlet container from
> request bytes is a distinct object, so reference comparison separates "the
> converter signalled failure" from "the user submitted this text".
> To be precise about the limit: this protects values arriving from a request,
> which is the case that matters here. It does not protect a value that happens
> to be interned, since all identical String literals share one instance -
> application code calling the converter programmatically with such a literal
> would still lose it. Closing that as well would mean giving the marker an
> identity no user string can share, which changes a published constant and is
> a binary-compatibility question rather than a bug fix.
> h2. Not included
> XWorkCollectionPropertyAccessor carries the same unguarded pattern but is
> left untouched: its scalar setProperty path is not reachable through the
> value stack. Setting {{ids[0]}} on a Set is rejected by OGNL before it gets
> there, so no failing test could be written for it and it was not changed
> blind. Verified independently during review.
> h2. Precedent
> WW-3762 fixed this same bug class in
> XWorkBasicConverter.doConvertToCollection back in 2.3.3, and
> CollectionConverter still carries that guard today. The property accessors
> were simply never given the equivalent check.
> WW-5701 is the mirror-image defect found while reviewing this fix:
> CollectionConverter's guard uses equals() and therefore drops a legitimate
> element whose text genuinely is the marker.
> h2. Backward compatibility
> Narrow. Previously an unconvertible entry was stored as the marker; 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.
> h2. Status
> Fixed in PR https://github.com/apache/struts/pull/1873 - three regression
> tests, written test-first and mutation-checked, plus an end-to-end test of
> the reported checkbox scenario. Full core suite green.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
