[
https://issues.apache.org/jira/browse/WW-5624?focusedWorklogId=1014321&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1014321
]
ASF GitHub Bot logged work on WW-5624:
--------------------------------------
Author: ASF GitHub Bot
Created on: 09/Apr/26 16:37
Start Date: 09/Apr/26 16:37
Worklog Time Spent: 10m
Work Description: lukaszlenart commented on PR #1657:
URL: https://github.com/apache/struts/pull/1657#issuecomment-4215870666
The new PR is much closer to the right direction than `#1651` / `#1652`
because it extracts shared authorization logic and avoids duplicating ad hoc
checks in each plugin. I still see a few remaining gaps that should be
addressed before this is fully aligned with `ParametersInterceptor` semantics:
issue (blocking): The `ModelDriven` exemption in
`DefaultParameterAuthorizer` looks too broad. Using `target != action` as the
signal means any non-action JSON root object can be treated like a
`ModelDriven` model and bypass `@StrutsParameter` checks. I think this should
mirror the existing `ParametersInterceptor` semantics more closely, instead of
inferring `ModelDriven` purely from object identity.
issue (blocking): `JSONInterceptor.filterUnauthorizedKeys()` only filters
top-level JSON keys. That improves the previous behavior, but it still does not
enforce nested `@StrutsParameter(depth=N)` semantics. Once an allowed top-level
key remains in the map, `JSONPopulator` can still recursively populate the
nested object graph underneath it. I think this needs path-aware authorization,
not only root-key filtering. Either walk nested maps/lists and authorize
synthesized paths recursively, or move the authorization check into the
recursive population flow so each nested segment is checked against the same
depth rules as `ParametersInterceptor`.
issue (blocking): The two-phase REST approach in
`ContentTypeInterceptor.copyAuthorizedProperties()` is better than the previous
direct merge, but it still authorizes only top-level bean properties. If an
allowed property is a complex object, the full nested graph deserialized into
the staging instance is copied over in one step without nested authorization. I
think the copy phase needs to become a deep authorized merge, not a shallow
bean-property copy.
issue: `copyAuthorizedProperties()` currently skips `null` values, which
changes semantics when `struts.parameters.requireAnnotations=true`. That means
an explicitly provided `null` in the request body can no longer clear an
authorized property. Please confirm whether this behavior change is
intentional. If not, explicit nulls for authorized properties should probably
still be copied. If it is intentional, it should be documented and covered by
tests.
issue: The two-phase deserialization path now requires a public no-arg
constructor on the target type. That is a new constraint which may break
existing actions/models when `requireAnnotations=true`. Please verify whether
this constructor requirement is acceptable for Struts REST usage. If it is, I
would at least add explicit tests and mention it in the PR/JIRA notes as a
compatibility constraint.
suggestion: Please also add regression tests for transition mode with nested
JSON bodies, and for `JSONInterceptor.root` resolving to a non-action object.
Those are the cases most likely to expose the remaining semantic gaps.
Issue Time Tracking
-------------------
Worklog Id: (was: 1014321)
Time Spent: 1h 10m (was: 1h)
> Request body population bypasses @StrutsParameter contract outside
> ParametersInterceptor
> ----------------------------------------------------------------------------------------
>
> Key: WW-5624
> URL: https://issues.apache.org/jira/browse/WW-5624
> Project: Struts 2
> Issue Type: Bug
> Components: Plugin - JSON, Plugin - REST
> Affects Versions: 7.1.1
> Reporter: Tran Quac
> Priority: Major
> Fix For: 7.2.0
>
> Time Spent: 1h 10m
> Remaining Estimate: 0h
>
> h2. Summary
> {{@StrutsParameter}} enforcement is currently implemented in
> {{ParametersInterceptor}} for standard request parameter binding, but
> request-body based binders in some plugins bypass that authorization model
> and populate action/model objects directly.
> This creates inconsistent behavior between URL/form parameters and JSON/XML
> request bodies, and may allow mass assignment of properties that would
> normally be rejected by {{ParametersInterceptor}}.
> h2. Affected areas currently identified
> * JSON plugin:
> {{JSONPopulator.populateObject()}} sets properties via direct reflection and
> does not follow the full {{@StrutsParameter}} authorization rules.
> * REST plugin:
> {{JacksonJsonHandler.toObject()}} updates target objects directly via Jackson
> and does not follow the full {{@StrutsParameter}} authorization rules.
> h2. Problem scope
> The issue is broader than checking whether a setter is annotated. The current
> core contract in {{ParametersInterceptor}} also includes:
> * permitted nesting depth
> * authorization based on the exposed root member
> * ModelDriven handling
> * transition mode semantics
> * related allowlisting behavior
> Any request-body binding implementation should align with that same contract,
> otherwise Struts applies different security rules depending on how input
> reaches the action/model.
> h2. Expected direction
> Instead of implementing separate partial checks in each plugin, Struts should
> reuse or extract the shared parameter-binding authorization logic from
> {{ParametersInterceptor}} and apply it consistently across request-body
> binders.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
