[
https://issues.apache.org/jira/browse/WW-5624?focusedWorklogId=1014514&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1014514
]
ASF GitHub Bot logged work on WW-5624:
--------------------------------------
Author: ASF GitHub Bot
Created on: 10/Apr/26 11:54
Start Date: 10/Apr/26 11:54
Worklog Time Spent: 10m
Work Description: tranquac commented on PR #1657:
URL: https://github.com/apache/struts/pull/1657#issuecomment-4223563657
Thanks for the thorough review @lukaszlenart! I've pushed a new commit
([aa78085](https://github.com/tranquac/struts/commit/aa78085)) addressing all 5
blocking issues plus the inline suggestion. Here's a point-by-point summary:
---
### 1. Rename `DefaultParameterAuthorizer` → `StrutsParameterAuthorizer` ✅
Done — class renamed, all references updated (`struts-beans.xml`,
`DefaultConfiguration.java`, both test files). Git detected it as a rename (93%
similarity).
---
### 2. ModelDriven exemption too broad ✅
Changed the guard in `StrutsParameterAuthorizer.isAuthorized()` from:
```java
if (target != action) { return true; }
```
to:
```java
if (target != action && action instanceof ModelDriven) { return true; }
```
Now only actions that explicitly implement `ModelDriven` will have their
model exempted. If a `JSONInterceptor` `root` configuration resolves to a
non-action object on a non-ModelDriven action, annotation checks are still
enforced. Added a regression test
`nonModelDrivenAction_differentTarget_notExempt()` to lock this in.
---
### 3. Nested enforcement missing in `JSONInterceptor` ✅
`filterUnauthorizedKeys()` now delegates to a recursive
`filterUnauthorizedKeysRecursive()` that builds dot-notation paths
(`"address.city"`) as it descends into nested Maps and Lists:
```java
String fullPath = prefix.isEmpty() ? key : prefix + "." + key;
if (!parameterAuthorizer.isAuthorized(fullPath, target, action)) {
it.remove(); }
// recurse into nested Map / List
```
This reuses the existing `@StrutsParameter(depth=N)` depth-counting in
`StrutsParameterAuthorizer` — a nested key `"address.city"` has depth=1, which
requires `getAddress()` to carry `@StrutsParameter(depth≥1)`. Added regression
tests for nested key filtering and for the non-action root object case.
---
### 4. REST shallow copy problem ✅
`copyAuthorizedProperties()` is now recursive. For each property, if the
value is a *complex bean type* (not primitive, String, Number, Date, Temporal,
Collection, Map, array, enum), it creates a fresh target instance and recurses
with the extended path prefix:
```java
if (isNestedBeanType(sourceValue.getClass())) {
copyAuthorizedProperties(sourceValue, targetValue, action, fullPath); //
depth-aware recurse
} else {
writeMethod.invoke(target, sourceValue);
}
```
Collections and Maps are copied as-is — their contents were already
deserialized and the depth check on the parent property is sufficient.
---
### 5. Null-skipping semantics — kept intentionally, now documented ✅
After careful consideration: in two-phase deserialization, `null` in
`freshInstance` is **ambiguous** — it could mean "field was explicitly set to
null in the request body" or "field was not present at all". Copying null would
silently wipe pre-initialized fields on the target for every property not sent
by the client.
The safer default is to skip null, and I've added an inline comment
explaining the reasoning:
```java
// Intentionally skip null values: in two-phase deserialization, properties
NOT present in the
// request body will be null in the fresh instance. Copying null would clear
pre-initialized
// fields on the target. This is the safer default.
```
If you feel strongly that explicit null should be propagated, I can add a
`clearMissingFields` flag (default `false`) — but I wanted your input before
adding API surface.
---
### 6. No-arg constructor requirement ✅
Added a `createFreshInstance()` helper that catches
`ReflectiveOperationException` and returns `null` on failure. The interceptor
now has two paths:
- **Two-phase (preferred)**: deserialize into `freshInstance`, then
`copyAuthorizedProperties()` — requires no-arg constructor
- **Single-phase fallback**: deserialize directly into `target`, then
`scrubUnauthorizedProperties()` nulls out any unauthorized property — no
constructor requirement
The scrub fallback is logged at `WARN` so operators know the less-safe path
is active. Backward compat fully preserved.
---
### Test results
```
core: 2920 tests — 0 failures, 0 errors
plugins/json: 124 tests — 0 failures, 0 errors
plugins/rest: 76 tests — 0 failures, 0 errors
```
New regression tests added:
- `ParameterAuthorizerTest#nonModelDrivenAction_differentTarget_notExempt`
- `JSONInterceptorTest#testNestedJsonKeysRecursivelyFiltered`
- `JSONInterceptorTest#testNonActionRootObjectStillChecked`
Please let me know if anything needs further adjustment!
Issue Time Tracking
-------------------
Worklog Id: (was: 1014514)
Time Spent: 1h 50m (was: 1h 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: 1h 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)
