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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
