[
https://issues.apache.org/jira/browse/WW-5624?focusedWorklogId=1014895&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1014895
]
ASF GitHub Bot logged work on WW-5624:
--------------------------------------
Author: ASF GitHub Bot
Created on: 11/Apr/26 10:57
Start Date: 11/Apr/26 10:57
Worklog Time Spent: 10m
Work Description: tranquac commented on PR #1657:
URL: https://github.com/apache/struts/pull/1657#issuecomment-4229305319
Thanks for the continued review. v4 patch (commit `72cc282`) addresses both
points:
---
### Blocking fix — bulk-copy fallback removed from `copyAuthorizedProperties`
The previous code fell back to `writeMethod.invoke(target, sourceValue)`
when a nested target bean was null and `createFreshInstance` failed:
```java
// BEFORE (v3 — bug):
} else {
writeMethod.invoke(target, sourceValue); // entire object graph copied
unfiltered
continue;
}
```
This copied the entire nested object graph in one step, bypassing per-path
authorization for every property underneath that node.
Fix: log a warning and skip, consistent with the policy already applied to
collection/array elements:
```java
// AFTER (v4):
} else {
LOG.warn("REST nested bean [{}] skipped — no no-arg constructor for
[{}],"
+ " cannot authorize its nested properties",
fullPath, sourceValue.getClass().getName());
continue;
}
```
---
### Suggestion implemented — reject body when no no-arg constructor at top
level
Previously the fallback was to deserialize directly into the target and then
apply a best-effort scrub. As you noted, the scrub path cannot guarantee that
all nested unauthorized properties are nulled out.
v4 rejects body deserialization entirely when `requireAnnotations=true` and
no no-arg constructor is available:
```java
Object freshInstance = createFreshInstance(target.getClass());
if (freshInstance != null) {
handler.toObject(invocation, reader, freshInstance);
copyAuthorizedProperties(freshInstance, target, invocation.getAction(),
target, "");
} else {
LOG.warn("REST body rejected: requireAnnotations=true but [{}] has no
no-arg constructor; "
+ "body deserialization skipped to preserve @StrutsParameter
authorization integrity",
target.getClass().getName());
// handler.toObject() never called
}
```
`scrubUnauthorizedProperties` and its recursive helper have been removed as
dead code.
---
### Test results
```
plugins/rest: 76 tests, 0 failures
```
Issue Time Tracking
-------------------
Worklog Id: (was: 1014895)
Time Spent: 2h 50m (was: 2h 40m)
> 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: 2h 50m
> 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)
