Skip to content

WW-5712 Cover Jackson any-setters in REST parameter authorization - #1889

Open
carrerasdarren-cell wants to merge 2 commits into
apache:mainfrom
carrerasdarren-cell:ww-5712-rest-anysetter-hardening
Open

WW-5712 Cover Jackson any-setters in REST parameter authorization#1889
carrerasdarren-cell wants to merge 2 commits into
apache:mainfrom
carrerasdarren-cell:ww-5712-rest-anysetter-hardening

Conversation

@carrerasdarren-cell

@carrerasdarren-cell carrerasdarren-cell commented Aug 31, 2026

Copy link
Copy Markdown

Fixes WW-5712.

This adds opt-in authorization coverage for Jackson any-setters in the REST
plugin:

  • adds @StrutsParameter(allowDynamicKeys = true) for explicit dynamic-key
    consent;
  • adds struts.rest.anySetter.requireAnnotations, defaulting to false for
    compatibility;
  • applies the consent and depth() limit to method and field any-setters;
  • rejects creator-parameter any-setters when enforcement is enabled; and
  • keeps JSON and XML behavior aligned while preserving existing behavior when
    enforcement is disabled.

Verification:

  • 142 REST plugin tests passed;
  • 83 focused core parameter-authorization and annotation tests passed;
  • REST plugin verify and Apache RAT passed; and
  • git diff --check passed.

@lukaszlenart

Copy link
Copy Markdown
Member

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.

Assisted-by: OpenAI Codex
@carrerasdarren-cell

Copy link
Copy Markdown
Author

Thanks for the detailed review. Addressed in 514b92bf4:

  • reject a detached any-setter parser when currentName() is null, before creating either authorization scope;
  • place both scope pushes under guarded finally cleanup;
  • clear dynamic-key request state after every Jackson JSON/XML mapper read, including exceptional exits;
  • make the module configuration flag volatile; and
  • clarify the ordinary query/form binding effect of placing @StrutsParameter on a field.

I added regressions for the null-name token-buffer path and for request-boundary cleanup after a failed Jackson read. Verification after the update: 41 focused authorization tests pass, all 144 REST plugin tests pass, REST plugin verify and Apache RAT pass, and git diff --check passes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants