[
https://issues.apache.org/jira/browse/WW-5626?focusedWorklogId=1018561&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1018561
]
ASF GitHub Bot logged work on WW-5626:
--------------------------------------
Author: ASF GitHub Bot
Created on: 04/May/26 11:23
Start Date: 04/May/26 11:23
Worklog Time Spent: 10m
Work Description: lukaszlenart opened a new pull request, #1673:
URL: https://github.com/apache/struts/pull/1673
Fixes [WW-5626](https://issues.apache.org/jira/browse/WW-5626).
**Part 1 of 2.** Approach C — handler-level per-property authorization that
replaces `ContentTypeInterceptor`'s two-phase deserialize-then-copy with a new
`AuthorizationAwareContentTypeHandler` interface — will follow in a separate
PR. This PR addresses only the three concerns from the WW-5624 review that
survive that refactor:
## Changes
- **Centralized ModelDriven target resolution.**
`ParametersInterceptor.isParameterAnnotatedAndAllowlist` previously did the
value-stack peek itself, then `StrutsParameterAuthorizer.isAuthorized`
independently checked `target != action && action instanceof ModelDriven`. New
`ParameterAuthorizer.resolveTarget(action)` consolidates the logic. Kept as a
`default` method to preserve the interface as a SAM (lambda-based test stubs
continue to work).
- **Defensive guard against non-String JSON keys.**
`JSONInterceptor.filterUnauthorizedKeysRecursive` did `String key = (String)
entry.getKey();` on a raw `Map`. Safe with the built-in `StrutsJSONReader`, but
a custom reader producing non-String keys would throw `ClassCastException`.
Replaced with `instanceof String key` pattern that debug-logs and skips.
- **Real REST integration tests.**
`ContentTypeInterceptorTest.testRequireAnnotationsEnabled_*` use
`com.mockobjects` mocks for `ContentTypeHandler`, so `toObject` is a no-op —
the tests prove `intercept()` returns SUCCESS but assert nothing about which
properties were actually filtered. New `ContentTypeInterceptorIntegrationTest`
uses a real `JacksonJsonHandler` and a real `StrutsParameterAuthorizer`
end-to-end with a mixed-annotation `SecureRestAction` fixture.
## Finding for follow-up PR
Building the REST integration tests surfaced a real semantic divergence
(documented inline in `SecureRestAction.java`): REST's recursive copy
authorizes EACH path level independently, so `@StrutsParameter(depth=1)` on
`getAddress` is not enough to bind `address.city` — the setter `setAddress`
also needs `@StrutsParameter` to authorize the top-level `address` at depth 0.
`ParametersInterceptor` only requires the getter annotation. This is captured
for the Approach C PR.
## Out of scope
`ContentTypeInterceptor` deep-copy/scrub complexity, the `isNestedBeanType`
package-name heuristic, and the two-phase architectural smell — all to be
replaced by Approach C.
## Test plan
- [x] `mvn test -DskipAssembly -pl core` — 2926 tests, zero regressions
- [x] `mvn test -DskipAssembly -pl plugins/json` — 125 tests, zero
regressions
- [x] `mvn test -DskipAssembly -pl plugins/rest` — 81 tests, zero regressions
- [x] `mvn test -DskipAssembly -pl
'!plugins/bean-validation,!plugins/tiles'` — full multi-module BUILD SUCCESS
(the two excluded modules fail identically on `main` for unrelated reasons:
`AnnotationFormatError` from Hibernate Validator on a test model, and missing
Velocity dependencies in the tiles plugin)
Issue Time Tracking
-------------------
Worklog Id: (was: 1018561)
Remaining Estimate: 0h
Time Spent: 10m
> Refactor JSON/REST @StrutsParameter enforcement to per-property authorization
> -----------------------------------------------------------------------------
>
> Key: WW-5626
> URL: https://issues.apache.org/jira/browse/WW-5626
> Project: Struts 2
> Issue Type: Improvement
> Components: Plugin - JSON, Plugin - REST
> Reporter: Lukasz Lenart
> Assignee: Lukasz Lenart
> Priority: Major
> Fix For: 7.2.0
>
> Time Spent: 10m
> Remaining Estimate: 0h
>
> Follow-up to WW-5624. The initial fix enforces {{@StrutsParameter}} on
> JSON/REST request bodies via post-hoc reflection — {{ContentTypeInterceptor}}
> deserializes into a fresh instance, then recursively copies only authorized
> properties. This works but has several drawbacks:
> * ~250 lines of reflection-based copy logic in {{ContentTypeInterceptor}}
> ({{copyAuthorizedProperties}}, {{deepCopyAuthorizedCollection}},
> {{deepCopyAuthorizedMap}}, {{deepCopyAuthorizedArray}})
> * Requires a public no-arg constructor on target types; otherwise body
> deserialization is rejected entirely
> * Relies on a fragile package-name heuristic ({{isNestedBeanType}}) to
> distinguish nested beans from leaf types — may misclassify third-party value
> types
> h2. Proposed solution
> Replace the two-phase copy with per-property authorization performed
> _during_ deserialization, so unauthorized fields are never set.
> * New optional interface {{AuthorizationAwareContentTypeHandler}} extending
> {{ContentTypeHandler}}, exposing a property-level authorization callback
> * {{ContentTypeInterceptor}} checks {{instanceof}} and uses property-level
> filtering when available; falls back to the existing two-phase copy for
> handlers that don't implement it (backward compatible)
> * Jackson handlers ({{JacksonJsonHandler}}, {{JacksonXmlHandler}}) register
> a {{SimpleModule}} with a {{BeanDeserializerModifier}} that wraps each
> {{SettableBeanProperty}} with an authorizing decorator, consulting
> {{ParameterAuthorizer.isAuthorized(path, target, action)}} before
> {{deserializeAndSet()}}
> * {{XStreamHandler}} uses an equivalent mechanism (e.g.
> {{xstream.omitField()}} pre-pass or a custom {{ReflectionConverter}})
> * Authorization context propagated via {{ThreadLocal}}, consistent with the
> existing {{ActionContext}} pattern
> h2. Cleanup also included
> Minor follow-ups from the WW-5624 review:
> * Remove redundant {{ModelDriven}} resolution in
> {{ParametersInterceptor.isParameterAnnotatedAndAllowlist}} (now handled by
> {{StrutsParameterAuthorizer}})
> * Replace mock-based REST integration tests with real
> {{JacksonJsonHandler}} tests that assert actual property filtering behavior
> * Guard the unchecked {{String}} cast on JSON map keys in
> {{JSONInterceptor.filterUnauthorizedKeysRecursive}}
> h2. Outcome
> * Removes ~200 lines of reflection-based copy logic
> * Lifts the no-arg constructor requirement
> * Authorization happens at the right layer (the deserializer), matching
> {{ParametersInterceptor}}'s per-parameter check semantics
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
