[
https://issues.apache.org/jira/browse/WW-5712?focusedWorklogId=1038991&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1038991
]
ASF GitHub Bot logged work on WW-5712:
--------------------------------------
Author: ASF GitHub Bot
Created on: 01/Sep/26 05:40
Start Date: 01/Sep/26 05:40
Worklog Time Spent: 10m
Work Description: lukaszlenart commented on PR #1889:
URL: https://github.com/apache/struts/pull/1889#issuecomment-5489440532
Thanks Darren — I reviewed this closely, with most of the attention on the
one change that
touches the existing enforcement path rather than on the new code.
## On the redirect of the existing checks
`AuthorizingSettableBeanProperty` (lines 86, 107) and
`AuthorizingValueDeserializer` (line 61)
now consult `DynamicKeyAuthorizationContext.isAuthorized(path)` instead of
`ParameterAuthorizationContext.isAuthorized(path)`. That path runs on every
REST body, since
`struts.parameters.requireAnnotations=true` is the 7.x default, so it was
worth being sure about.
It holds. `DynamicKeyAuthorizationContext.isAuthorized` falls through to the
original check
whenever the scope deque is empty, a scope is only ever pushed from
`AuthorizingSettableAnyProperty`,
and that class is only installed when the new constant is on — which
`struts-plugin.xml` ships as
`false`. So with the default configuration the deque is always empty and
both classes behave exactly
as before. With enforcement on, I could not construct a case where a scope
is live while something
outside the sink subtree is deserialized; the `@JsonUnwrapped` token replay
in `BeanDeserializer`
runs after the scope has been popped.
The rest also checks out: the boundary-character test in `remainingDepth`
means an empty or
nesting-char-bearing dynamic key tightens the check rather than escaping it,
and it agrees with
`NESTING_CHARS` in the core authorizer; `limitForNestedScope` narrows
monotonically so a nested
any-setter cannot buy back depth budget; `REJECTED_VALUE` cannot reach
application state, because
every consumer of `deserialize`'s return value goes through the
identity-filtered `set`; and the
creator-parameter form is fail-closed, with the creator receiving an empty
map rather than a
partially populated one.
## One thing to fix before merge
`AuthorizingSettableAnyProperty:129-130` and `:161-162` — the scope can
leak, and it leaks onto a
pooled container thread.
`deserialize()` takes its path from `parser.currentName()`, which is `null`
when Jackson hands it a
detached `TokenBuffer` parser (`deserializeWithUnwrapped` with a
property-based creator and an
any-setter on the same bean). `pathFor(null)` returns `null`, and `pushPath`
does
`PATH_STACK.get().push(null)` on an `ArrayDeque`, which throws NPE. That
`pushPath` sits outside the
`try`, immediately after `DynamicKeyAuthorizationContext.push(...)`, so the
scope is never popped —
and `ParameterAuthorizationContext.unbind()` clears `STATE`, `PATH_STACK`
and `REDACTION_STACK` but
knows nothing about the new `SCOPES` ThreadLocal, so it survives the request.
The direction is safe — the leaked scope has a null `basePath`,
`remainingDepth` returns `-1` for
that, so the thread fails closed and denies everything — but a thread that
silently drops every REST
body parameter until the container recycles it is a bad failure mode. Three
cheap changes:
1. Move `DynamicKeyAuthorizationContext.push(...)` inside the `try`, or put
both pushes under one
`try`/`finally`, so the scope cannot outlive the frame that created it.
2. Guard `deserialize()` on a null current name and reject explicitly,
rather than letting it reach
`pathFor`.
3. Have the scope stack cleared alongside the other ThreadLocals when the
request ends, so no future
leak can cross a request boundary. Adding it to
`ParameterAuthorizationContext.unbind()` would
invert the dependency, so probably a
`DynamicKeyAuthorizationContext.clear()` called from the same
place in `ContentTypeInterceptor`'s `finally`.
A regression test for the null-name path would be worth having, since it is
not currently covered.
## Smaller points
- `ParameterAuthorizingModule.requireAnySetterAnnotations` is a non-final
field mutated after the
`ObjectMapper` has been constructed. The ordering works today — injection
happens at container
build time, and `registerModule` builds no deserializers — but the
javadoc's "set this before the
mapper is first used" is a constraint the type cannot enforce. Marking the
field `volatile` costs
nothing and documents the cross-thread read.
- The javadoc on `allowDynamicKeys` says ordinary parameter injection
ignores the flag. True, but on
the field form the `@StrutsParameter` annotation itself still makes that
map bindable from ordinary
query and form parameters. Worth a sentence, so nobody reads the
annotation as inert on that
channel.
- An opted-in any-setter's `depth` can exceed the depth declared on the
member above it — a
`depth = 1` setter holding a bean whose sink declares `depth = 2` grants
the deeper path
structurally. Strictly more restrictive than before this change, and the
`depth = 2` is an explicit
declaration, so I am not asking for a behaviour change; a test pinning the
intended semantics would
be useful though.
Nothing here changes the shape of the design — the sink-level consent model,
the default-off constant
and the depth accounting all land the way we discussed. Fix the scope leak
and I am happy with it.
Issue Time Tracking
-------------------
Worklog Id: (was: 1038991)
Time Spent: 20m (was: 10m)
> ParameterAuthorizingModule does not wrap the Jackson any-setter in the REST
> plugin
> ----------------------------------------------------------------------------------
>
> Key: WW-5712
> URL: https://issues.apache.org/jira/browse/WW-5712
> Project: Struts 2
> Issue Type: Bug
> Reporter: Lukasz Lenart
> Priority: Major
> Fix For: 7.4.0
>
> Time Spent: 20m
> Remaining Estimate: 0h
>
> {{ParameterAuthorizingModule.updateBuilder}} wraps the properties returned by
> {{BeanDeserializerBuilder.getProperties()}} with
> {{AuthorizingSettableBeanProperty}}, but it never consults
> {{BeanDeserializerBuilder.getAnySetter()}}. The {{SettableAnyProperty}} is
> therefore left unwrapped, so unknown keys dispatched to a Jackson
> {{@JsonAnySetter}} are set without passing through the {{@StrutsParameter}}
> authorization context — while an ordinary unannotated setter on the same
> class, in the same request body, is correctly rejected.
> All three any-setter forms are affected (method, field and {{@JsonCreator}}
> parameter). Both {{JacksonJsonHandler}} and {{JacksonXmlHandler}} register
> the module, so JSON and XML bodies behave the same. {{@JsonUnwrapped}} is not
> affected, and an any-setter beneath a rejected parent is already skipped
> through {{skipChildren()}}.
> The obvious fix does not work: path-based authorization resolves a
> parameter's root name against declared members of the target, so
> {{pathFor(key)}} matches nothing for a dynamic key by construction. Routing
> any-setter keys through the existing check would reject every key and break
> every existing {{@JsonAnySetter}} user. Consent has to be expressed at the
> sink rather than at the key.
> Proposed shape:
> * A new annotation member, {{@StrutsParameter(allowDynamicKeys = true)}},
> marking one any-setter as permitted to receive arbitrary keys. A plain
> {{@StrutsParameter}} is deliberately not sufficient — arbitrary-key access
> should have to be typed out, not granted by accident.
> * Enforcement behind a plugin-local constant,
> {{struts.rest.anySetter.requireAnnotations}}, defaulting to {{false}}, so
> nothing changes for existing applications until they opt in. Same shape as
> {{struts.chaining.requireAnnotations}}; the default can be revisited in a
> major release.
> * {{depth()}} keeps its current meaning, counted beneath the dynamic key. The
> default {{depth = 0}} accepts a scalar value under any key and rejects a
> nested object under it; {{depth = 1}} permits one level of nesting. That is
> the rule an annotated {{Map}} property already follows.
> * {{@Target}} stays {{METHOD, FIELD}}. A {{@JsonCreator}}-parameter
> any-setter has nowhere to carry the marker and is rejected with a warning
> when enforcement is on. Narrowing that rare form now is preferable to
> advertising an annotation placement the framework does not honour elsewhere,
> and {{@Target}} can be widened later but never narrowed.
> * The wrapper mirrors {{AuthorizingSettableBeanProperty}}: delegate untouched
> when the authorization context is inactive; on rejection log a warning, mark
> the context redacted and call {{skipChildren()}} so nested graphs are never
> constructed.
> Test coverage should include default-off compatibility, the method and field
> forms, missing and explicit consent, depth 0/1/2, creator parameters,
> unauthorized parents, {{@JsonUnwrapped}}, both the JSON and XML handlers, and
> context cleanup.
> Documentation follow-up: the {{@StrutsParameter}} page states that the JSON
> and REST plugins authorize each property during deserialization "so that
> unauthorized fields are never set". That is not accurate as written — correct
> the wording and document the any-setter behaviour explicitly.
> Reported by Darren Carreras (GitHub: carrerasdarren-cell), who has a patch
> implementing the above and will open the pull request.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
