[
https://issues.apache.org/jira/browse/WW-5624?focusedWorklogId=1014864&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1014864
]
ASF GitHub Bot logged work on WW-5624:
--------------------------------------
Author: ASF GitHub Bot
Created on: 11/Apr/26 00:52
Start Date: 11/Apr/26 00:52
Worklog Time Spent: 10m
Work Description: tranquac commented on PR #1657:
URL: https://github.com/apache/struts/pull/1657#issuecomment-4227514963
Thanks for the detailed review. v3 patch (commit `9490dfa`) addresses all
four gaps you identified:
---
### Gap 1 — JSON: indexed-path depth for list elements
`filterUnauthorizedList` was passing the bare `prefix` unchanged into
`filterUnauthorizedKeysRecursive`, so a path like `publicPojoListDepthOne.key`
(depth=1) was being checked instead of `publicPojoListDepthOne[0].key`
(depth=2). That let list element properties pass under a lower depth annotation
than `ParametersInterceptor` would allow.
Fix: pass `prefix + "[0]"` as `elementPrefix` for all items in the list.
`NESTING_CHARS` includes `[`, so the extra bracket increments `paramDepth` by
1, exactly matching the indexed-path depth semantics:
```java
String elementPrefix = prefix + "[0]";
for (Object item : list) {
if (item instanceof Map) {
filterUnauthorizedKeysRecursive((Map) item, elementPrefix, target,
action);
} else if (item instanceof java.util.List) {
filterUnauthorizedList((java.util.List) item, elementPrefix, target,
action);
}
}
```
The nested-list `else if` branch handles `List<List<Map>>` shapes that the
previous version left unfiltered.
---
### Gap 2 — REST: `authTarget` always = root action/model
`isAuthorized(fullPath, target, action)` uses `target` to look up the root
property annotation. On recursive calls, `target` was being replaced with the
nested object (e.g., `Address`), so `isAuthorized("address.city",
addressObject, action)` looked for a property named `"address"` on `Address` —
not found, silently rejecting all nested properties.
Fix: add a new `authTarget` parameter to `copyAuthorizedProperties`, always
set to the root action/model at the initial call site, and passed unchanged
through every level of recursion. All `isAuthorized` calls now use
`authTarget`, never the traversal-level `target`:
```java
// Initial call:
copyAuthorizedProperties(freshInstance, target, invocation.getAction(),
target, "");
// Signature:
private void copyAuthorizedProperties(
Object source, Object target, Object action, Object authTarget,
String prefix)
// All recursive calls:
copyAuthorizedProperties(sourceValue, targetValue, action, authTarget,
fullPath);
```
---
### Gap 3 — REST: Collection / Map / array deep authorization
Previously `Collection`, `Map`, and `Pojo[]` properties were copied as-is
after the parent-level path check. That skipped element-level authorization
entirely.
Three new helpers — `deepCopyAuthorizedCollection`, `deepCopyAuthorizedMap`,
`deepCopyAuthorizedArray` — iterate every element/value and:
1. Check `path + "[0]"` against `authTarget` (adds one `[` → depth +1,
matching `ParametersInterceptor`)
2. Create a fresh instance and recurse via `copyAuthorizedProperties` for
complex types
3. **Skip** the element (rather than copy it) when no no-arg constructor is
available — avoids leaking an unfiltered object graph
---
### Gap 4 — REST: `scrubUnauthorizedProperties` fallback — full recursion
`scrubUnauthorizedProperties` previously only iterated top-level property
descriptors, leaving nested beans and collection/map elements unscrubbed.
Fix: refactored into `scrubUnauthorizedPropertiesRecursive` which:
- Recurses into nested beans with `fullPath` prefix
- Iterates `Collection` elements with `fullPath + "[0]"` prefix
- Iterates `Map` values with `fullPath + "[0]"` prefix (new, missing from
the earlier plan)
- Uses an identity-based `visited` set (`System.identityHashCode`) to break
cycles
- Passes `authTarget` (root object) unchanged at every level
---
### Test results
```
core: 2920 tests, 0 failures
plugins/json: 124 tests, 0 failures
plugins/rest: 76 tests, 0 failures
Total: 3120 tests, 0 failures
```
Let me know if there is anything further you would like adjusted.
Issue Time Tracking
-------------------
Worklog Id: (was: 1014864)
Time Spent: 2h 10m (was: 2h)
> 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 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)
