[
https://issues.apache.org/jira/browse/WW-5790?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Lukasz Lenart updated WW-5790:
------------------------------
Description:
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.
was:
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()}}.
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.
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}}.
Possible direction: compare each property of the filled instance against a
second, untouched fresh instance and copy only what the handler changed,
threading that baseline through the nested-bean recursion. A body value that
happens to equal the constructor default would then not be applied over a
differing existing value, which is the same ambiguity the copy already has for
null.
Tests to add in {{ContentTypeInterceptorIntegrationTest}}: a non-aware handler
that fills part of a model with a constructor default, asserting the omitted
property keeps its prior value.
> 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
>
>
> 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)
