[
https://issues.apache.org/jira/browse/WW-5790?focusedWorklogId=1049538&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1049538
]
ASF GitHub Bot logged work on WW-5790:
--------------------------------------
Author: ASF GitHub Bot
Created on: 23/Oct/26 05:42
Start Date: 23/Oct/26 05:42
Worklog Time Spent: 10m
Work Description: lukaszlenart opened a new pull request, #2051:
URL: https://github.com/apache/struts/pull/2051
Fixes [WW-5790](https://issues.apache.org/jira/browse/WW-5790)
## What was wrong
With `struts.parameters.requireAnnotations=true`, a content-type handler
that does not implement `AuthorizationAwareContentTypeHandler` goes through the
two-phase copy in `ContentTypeInterceptor`: a fresh instance is built with the
no-arg constructor, the handler fills it, and every non-null authorized
property is copied onto the real target. The copy could not tell a value the
body supplied from a value the constructor assigned, so every property the body
omitted but the constructor initialised was written over what `params` had
bound or `prepare()` had loaded. Scalars, constructor-initialised collections
(an empty list replaced a populated one) and constructor-initialised nested
beans (the recursion reset their leaves) all suffered. Follow-up to #2050,
which covered the handlers that never read the body; this one covers
`XStreamHandler` and application-provided handlers.
## Change
- `applyTwoPhaseDeserialize` builds a second, untouched fresh instance as a
baseline.
- `copyAuthorizedProperties` takes the baseline and skips a property whose
filled value `Objects.deepEquals` the baseline value. The baseline is threaded
through the nested-bean recursion: the baseline's own nested value, else a
fresh instance of the nested class, else null, which keeps today's
copy-non-null rule for that subtree. Collection, map and array elements are new
objects already, so their recursion passes no baseline.
- The authorization check runs before any copy, unchanged. The copy now
writes a subset of what it wrote before; nothing new becomes bindable.
- No public API or configuration change.
## Documented limits
- A body value equal to the constructor default, sent while the target holds
a different value, is not applied. Mirrors the null ambiguity accepted in
WW-5624.
- The target's constructor runs twice per request on this path.
- Non-deterministic defaults (`UUID.randomUUID()`, `Instant.now()`) always
differ from the baseline and are still copied, as before.
Behaviour changes only when all three hold: `requireAnnotations=true`, a
non-aware handler, and a property with a non-null constructor default. The
default handlers are unaffected.
## Tests
Eleven tests in `ContentTypeInterceptorIntegrationTest` on a ModelDriven
fixture driven by a scripted non-aware handler: omitted scalar default kept,
set scalar applied, omitted collection default kept, omitted nested default
kept, partially set nested bean applies only the set leaf, nested type without
a no-arg constructor still applies the set leaf, the documented limits pinned
by name (body value equal to the default, primitive zero value, empty
collection), a nested subtype whose own leaf is applied, and a nested bean
whose id-only `equals` does not hide a changed leaf. Five failed before the
change with the constructor value in place of the pre-existing one; the subtype
and partial-equals tests came out of the review and failed before their fix.
## Verification
- `plugins/rest` suite with core compiled from the branch: 237 tests, 0
failures.
- Security review of the diff: no findings. The copy writes a subset of what
it wrote before; the authorization check in front of every copy is unchanged.
- Docs: apache/struts-site#362.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Issue Time Tracking
-------------------
Worklog Id: (was: 1049538)
Time Spent: 20m (was: 10m)
> REST two-phase body copy writes constructor defaults over values already set
> on the target
> ------------------------------------------------------------------------------------------
>
> Key: WW-5790
> URL: https://issues.apache.org/jira/browse/WW-5790
> Project: Struts 2
> Issue Type: Bug
> Components: Plugin - REST
> Reporter: Lukasz Lenart
> Assignee: Lukasz Lenart
> Priority: Major
> Fix For: 7.5.0
>
> Time Spent: 20m
> Remaining Estimate: 0h
>
> Follow-up to [WW-5789|https://issues.apache.org/jira/browse/WW-5789], which
> covered the handlers that do not read the body at all.
> When {{struts.parameters.requireAnnotations=true}} and the selected
> content-type handler does not implement
> {{AuthorizationAwareContentTypeHandler}},
> {{ContentTypeInterceptor.applyTwoPhaseDeserialize}} creates a fresh instance
> of the target with its no-arg constructor, lets the handler fill it from the
> body, and then copies every non-null property from that instance onto the
> real target. The copy cannot tell a value the body supplied from a value the
> constructor assigned, so a property the body omits is still copied when it
> has a non-null constructor default. That overwrites whatever was on the
> target before the {{rest}} interceptor ran: a value bound by {{params}}, or
> one loaded in {{prepare()}}.
> The same happens in three shapes: a scalar with a constructor default, a
> collection initialised in the constructor (an empty list replaces a populated
> one on the target), and a nested bean initialised in the constructor (the
> copy recurses into it and resets the target's nested leaves).
> Affected handlers: {{XStreamHandler}} (shipped, registered by the
> application) and any application-provided handler that does not declare the
> marker. The handlers shipped as default ({{JacksonJsonHandler}},
> {{JacksonXmlHandler}}, {{JuneauXmlHandler}}) are not affected, they authorize
> during deserialization. The JSON plugin uses a different mechanism and is not
> affected.
> Example: a ModelDriven model declares {{private String status = "new";}}. The
> request carries {{status=approved}} as a query parameter and an XML body that
> only contains {{name}}. After the stack runs, {{status}} is {{new}}.
> h2. Design
> Fix the copy, not the handlers. The affected handlers are opt-in or external
> and cannot report which properties they set, so the interceptor has to work
> it out on its own.
> * Build a second, untouched fresh instance of the target as a baseline next
> to the one the handler fills.
> * Copy a property only when its filled value differs from the baseline value,
> compared with {{Objects.deepEquals}} so arrays compare by content. A nested
> bean the handler instantiated compares by identity and therefore counts as
> changed, which is what is wanted.
> * Thread the baseline through the nested-bean recursion. When the baseline's
> nested value is null but the filled one is not, use a fresh instance of the
> nested class as the baseline at that level. When that class has no no-arg
> constructor, fall back to today's behaviour for that subtree.
> * Collections, maps and arrays are compared against the baseline at the
> property level only. Their elements are already rebuilt as new objects, so
> skipping defaults inside them would change nothing.
> * No public API change: {{ContentTypeHandler}},
> {{AuthorizationAwareContentTypeHandler}} and the public surface of
> {{ContentTypeInterceptor}} stay as they are. No new configuration constant: a
> switch whose old setting is the defect has no sensible default.
> * The existing null-skip rule stays. Document the new default-skip rule next
> to it, in one place.
> Known limits, to be stated in the docs:
> * A body value equal to the constructor default, sent while the target holds
> a different value, is no longer applied. This mirrors the null ambiguity
> accepted in WW-5624.
> * Non-deterministic defaults such as {{UUID.randomUUID()}} or
> {{Instant.now()}} always differ from the baseline and keep being copied, as
> today.
> * The target's constructor runs twice per request on this path.
> Rejected: deserializing into a deep copy of the target. It would remove the
> ambiguity entirely, but needs a reflective deep copy of arbitrary object
> graphs, and a shallow copy would let the handler mutate the target's nested
> objects before authorization.
> Behaviour changes only when all three hold: {{requireAnnotations=true}}, a
> non-aware handler, and a property with a non-null constructor default.
> Applications on the default handlers see nothing.
> Tests to add in {{ContentTypeInterceptorIntegrationTest}}, with a non-aware
> handler that sets some properties and leaves others: omitted scalar default
> kept; set scalar applied; omitted collection default kept; omitted nested
> default kept; partially set nested bean applies only the set leaf; the
> documented limit pinned by name.
> Docs: a paragraph under the 7.2.0 enforcement note on the REST plugin page,
> and a line in the Version Notes.
> Out of scope, candidates for their own tickets: making {{XStreamHandler}}
> authorization-aware so no shipped handler is left on the two-phase path, and
> deprecating the two-phase path for removal at 8.0.0 once that is done.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
