[
https://issues.apache.org/jira/browse/WW-5626?focusedWorklogId=1018633&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1018633
]
ASF GitHub Bot logged work on WW-5626:
--------------------------------------
Author: ASF GitHub Bot
Created on: 04/May/26 16:46
Start Date: 04/May/26 16:46
Worklog Time Spent: 10m
Work Description: lukaszlenart opened a new pull request, #1674:
URL: https://github.com/apache/struts/pull/1674
Fixes [WW-5626](https://issues.apache.org/jira/browse/WW-5626).
**Part 2 of 2.** Builds on the cleanup PR #1673 (do not merge until that one
lands; base will be retargeted to `main` afterwards).
## What this changes
Replaces the post-hoc reflection-based property filtering in
`ContentTypeInterceptor` with per-property authorization performed *during*
deserialization, for the default Jackson REST handlers.
## Architecture
- **`ParameterAuthorizationContext`** (core) — `ThreadLocal` holder for
per-request `(authorizer, target, action, pathStack)`. The interceptor binds it
before invoking authorization-aware handlers and unbinds in a `finally` block.
- **`AuthorizationAwareContentTypeHandler`** (rest) — marker interface.
Handlers implementing it signal that their parser honors
`ParameterAuthorizationContext` for per-property `@StrutsParameter` enforcement.
- **`ParameterAuthorizingModule`** (rest) — Jackson `SimpleModule` with a
`BeanDeserializerModifier` that wraps every `SettableBeanProperty` with
`AuthorizingSettableBeanProperty`. Registered once on each handler's mapper.
- **`AuthorizingSettableBeanProperty`** (rest) — wraps each property; checks
`ParameterAuthorizationContext.isAuthorized(path)` before delegating, calls
`JsonParser.skipChildren()` for unauthorized values, manages the path stack
with `[0]` suffix for collection/map/array-typed properties.
- **`JacksonJsonHandler` / `JacksonXmlHandler`** — register the module on
construction; declare `implements AuthorizationAwareContentTypeHandler`.
- **`ContentTypeInterceptor`** — when `requireAnnotations=true` AND `handler
instanceof AuthorizationAwareContentTypeHandler`: binds context, calls
`handler.toObject(...)` directly. Otherwise: legacy two-phase copy (preserved
as-is for non-Jackson handlers).
- **`XStreamHandler`** — `@Deprecated(since="7.2.0", forRemoval=true)` with
Javadoc pointing users to `JacksonXmlHandler`. XStream's deserialization CVE
history and per-class allowlist requirement justify removing it in a future
major.
## Why this is better than the cleanup PR's two-phase copy
- **No no-arg constructor requirement** for the target class
- **Stronger security**: rejecting at the parent property means the
unauthorized JSON subtree is discarded entirely — Jackson never instantiates
nested objects, so setter side effects on unauthorized properties never fire
(vs. two-phase copy which deserializes everything then nulls out)
- **No reflection-based deep copy** for Jackson handlers — ~250 lines of
post-hoc filtering bypassed
- **No fragile `isNestedBeanType` package-name heuristic** — Jackson's
`JavaType` introspection handles type detection correctly
## Out of scope (preserved as legacy fallback)
The legacy two-phase copy in `ContentTypeInterceptor` is **not** removed —
`XStreamHandler`, `JuneauXmlHandler`, and any custom handlers without the
marker interface continue to use it. A future PR migrating those handlers can
finally delete the dead code.
## Spike
A spike that validated the Jackson `BeanDeserializerModifier` mechanism is
in commit `6110b8f6b` and removed in commit `0166c6a0b` (replaced by the
production tests in `ParameterAuthorizingModuleTest`).
## Test plan
- [x] `mvn test -DskipAssembly -pl core` —
`ParameterAuthorizationContextTest` 12 new tests pass; full core suite zero
regressions
- [x] `mvn test -DskipAssembly -pl plugins/rest` —
`ParameterAuthorizingModuleTest` 8 new tests pass;
`ContentTypeInterceptorIntegrationTest` 7 tests pass (5 from the cleanup PR now
silently exercise the new path + 2 new tests proving Jackson path is in use)
- [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: 1018633)
Time Spent: 0.5h (was: 20m)
> 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: 0.5h
> 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)
