[
https://issues.apache.org/jira/browse/WW-5700?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18111086#comment-18111086
]
Lukasz Lenart commented on WW-5700:
-----------------------------------
h2. Upgrade note: defensive workarounds written against this bug change meaning
The Backward compatibility section above says "nothing can reasonably depend on
the old behaviour". That holds for correct code, but not for the workarounds
people wrote _because_ of this bug — and the original reporter had written one.
Worth carrying into the 7.4.0 / 6.12.0 migration notes.
The pattern is: guard on the entry being present, then repair it by type.
{code:java}if (map.get(id) != null) {
if (map.get(id) instanceof Integer) {
// good value
} else {
map.put(id, Integer.valueOf(0)); // repair the marker
}
}
{code}
Before the fix the entry held the marker, so the outer null check passed and
the {{else}} normalised it to 0. After the fix the entry is absent, the outer
check is false, and the {{else}} never runs — so the key that used to end up
present with a 0 is now simply missing. Silent, and in the opposite direction
from what the reader of that code expects.
Verified on main, same binding either side of the fix:
{code}before: map = {100=1, 200=ognl.NoConversionPossible}
conversionErrors = {capDeferral[200]=ConversionData}
after: map = {100=1}
conversionErrors = {capDeferral[200]=ConversionData}
{code}
The conversion error is unchanged, as the description states — only the map
contents differ.
Also confirmed while answering the reporter, since it is the other half of the
question people will ask: with an unmodified {{defaultStack}} and a
ValidationAware action this ends at INPUT both before and after the fix.
{{StrutsConversionErrorInterceptor}} turns the entry into field error
{{capDeferral[200]}} and {{hasErrors()}} is then true. The stack order is
{{params -> conversionError -> validation -> workflow}}, so {{validate()}}
still runs while a conversion error is pending — which is exactly why the
reported ClassCastException surfaced inside the application's {{validate()}}
rather than being short-circuited earlier. None of that changes in 7.4.0.
Suggested guidance for anyone removing such a workaround: drop the type test
and key off absence instead.
{code:java}Integer v = map.get(id);
boolean selected = Integer.valueOf(1).equals(v);
{code}
> 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
> Fix For: 6.12.0, 7.4.0
>
> Time Spent: 1h
> 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)
