Skip to content

WW-5627 Gate CookieInterceptor through ParameterAuthorizer#1681

Open
lukaszlenart wants to merge 11 commits into
mainfrom
WW-5627-cookie-authorization
Open

WW-5627 Gate CookieInterceptor through ParameterAuthorizer#1681
lukaszlenart wants to merge 11 commits into
mainfrom
WW-5627-cookie-authorization

Conversation

@lukaszlenart
Copy link
Copy Markdown
Member

@lukaszlenart lukaszlenart commented May 9, 2026

Summary

Fixes WW-5627 — CookieInterceptor.populateCookieValueIntoStack calls stack.setValue directly (CookieInterceptor.java:339,348 pre-fix), bypassing StrutsParameterAuthorizer so cookies write to action setters that are not annotated with @StrutsParameter even when struts.parameters.requireAnnotations=true.

This PR retrofits the @StrutsParameter enforcement onto the cookie input channel:

  • New ParameterAllowlister interface (extension point) + default OgnlParameterAllowlister impl extracted from ParametersInterceptor.performOgnlAllowlisting. Mirrors the ParameterAuthorizer pair from WW-5626. Apps can swap implementations via <constant name="struts.parameterAllowlister" value="..."/>.
  • ParametersInterceptor delegates to the new component (pure refactor, all 35 existing tests pass unchanged).
  • CookieInterceptor adds a new protected void populateCookieValueIntoStack(name, value, map, stack, action) extension hook. The default impl runs parameterAuthorizer.isAuthorized → parameterAllowlister.allowlistAuthorizedPath → existing 4-arg form. The 4-arg form is @Deprecated(since="7.2.0") with body unchanged, so existing subclass overrides automatically inherit the gate.
  • DI registration in struts-beans.xml, StrutsBeanSelectionProvider, and DefaultConfiguration.bootstrapFactories().

Default-config invariant: when struts.parameters.requireAnnotations=false (the 7.x default), ParameterAuthorizer.isAuthorized short-circuits to true, so existing apps see no behavior change. Only opt-in apps see cookies gated.

Behavior matrix

Config Cookie name Setter @StrutsParameter? Outcome
requireAnnotations=false (default) any any Injected (unchanged)
requireAnnotations=true flatName yes Injected
requireAnnotations=true flatName no Skipped, debug log
requireAnnotations=true user.role yes (depth ≥1) Injected; ThreadAllowlist primed
requireAnnotations=true user.role no / depth too low Skipped, debug log
requireAnnotations=true, transition flatName no Injected (depth-0 exemption)
requireAnnotations=true, ModelDriven model any n/a Injected against model

Migration notes (for the Confluence wiki)

Behavior change for struts.parameters.requireAnnotations=true adopters: CookieInterceptor now enforces @StrutsParameter annotations on action setters when struts.parameters.requireAnnotations=true. Apps using <param name="cookiesName">*</param> together with annotation enforcement must annotate any setter intended to receive cookie values, or that cookie will be silently skipped (debug-logged). No change for the default config.

Stacking

Stacked on top of WW-5626-approach-c (where ParameterAuthorizer and StrutsParameterAuthorizer were introduced). When WW-5626 merges to main, this PR's base will be retargeted automatically.

Test plan

  • mvn test -DskipAssembly -pl core — 2952/2952 green.
  • mvn test -DskipAssembly -pl plugins/json — 125/125 green (transitive contract sanity).
  • New: OgnlParameterAllowlisterTest (6 unit tests).
  • New: CookieInterceptorAnnotationTest (8 tests covering the full behavior matrix above, including subclass-override-of-deprecated-hook gets the gate, ModelDriven exemption, transition mode).
  • Existing CookieInterceptorTest (9 tests) updated to wire pass-through lambdas via a disableAuthorizationGate(...) helper since those tests construct CookieInterceptor via new rather than the container.
  • Existing ParametersInterceptorTest, ActionMappingParametersInterceptorTest, StaticParametersInterceptorTest, StrutsParameterAnnotationTest all green after the refactor.

Out of scope (separate Jira candidates)

  • ScopeInterceptor (ScopeInterceptor.java:306,328) has the same stack.setValue pattern from session/application scope — likely warrants its own ticket.
  • ParametersInterceptor.hasValidAnnotated* helpers (lines 432–548) appear to be dead post-WW-5626 (only StrutsParameterAuthorizer's versions are reached at runtime). Cleanup is a separate audit.
  • ParameterNameAware consultation in CookieInterceptor — not currently consulted; adding it would be a larger behavior change.

🤖 Generated with Claude Code

lukaszlenart added a commit that referenced this pull request May 9, 2026
@lukaszlenart
WW-5627 address SonarCloud findings on PR #1681
73f456b 
- S1948: mark transient on the new ParameterAuthorizer/ParameterAllowlister
  fields in CookieInterceptor and ParametersInterceptor (the host classes
  are Serializable; the injected services are not).
- S1874: suppress the deprecation warning on the new 5-arg
  populateCookieValueIntoStack — the delegation to the deprecated 4-arg
  form is the contract that lets existing subclass overrides participate.
- S3776: extract `allowlistViaPropertyDescriptor` and
  `allowlistViaPublicField` from `OgnlParameterAllowlister.allowlistAuthorizedPath`
  to drop cognitive complexity below the threshold.
- S1068: remove the unused `mapping` test fixture field.
Base automatically changed from WW-5626-approach-c to main May 14, 2026 13:01
lukaszlenart and others added 10 commits May 14, 2026 15:06
@lukaszlenart @claude
WW-5627 add ParameterAllowlister interface and STRUTS_PARAMETER_ALLOW…
b92364e 
…LISTER constant

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@lukaszlenart @claude
WW-5627 add OgnlParameterAllowlister default implementation
94c6ba4 
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@lukaszlenart
WW-5627 register ParameterAllowlister bean in struts-default DI
025cb04
@lukaszlenart @claude
WW-5627 delegate ParametersInterceptor OGNL allowlisting to OgnlParam…
1e838a3 
…eterAllowlister

Also register ParameterAllowlister in DefaultConfiguration bootstrap
factories so it is available in test containers (parallel to how
ParameterAuthorizer was already registered there).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@lukaszlenart @claude
WW-5627 test(cookie): failing test for unannotated setter skip
4a0aacc 
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@lukaszlenart
WW-5627 gate CookieInterceptor cookie injection through ParameterAuth…
c3d8a4c 
…orizer

Adds a 5-arg `populateCookieValueIntoStack(name, value, map, stack, action)` hook
that runs cookie writes through `ParameterAuthorizer.isAuthorized` and primes
`ThreadAllowlist` via `ParameterAllowlister` for nested paths, then delegates
to the legacy 4-arg form. The 4-arg form is `@Deprecated(since="7.2.0")` but
its body is unchanged, so existing subclass overrides automatically receive
only authorized cookies. Default-config behavior is preserved because the
authorizer short-circuits when `requireAnnotations=false`.

Existing `CookieInterceptorTest` instantiates `new CookieInterceptor()` rather
than going through the container, leaving the new injected fields null. Wires
explicit pass-through lambdas through a `disableAuthorizationGate(...)` helper
so those tests continue to exercise default-config behavior.
@lukaszlenart
WW-5627 cover CookieInterceptor authorization matrix in CookieInterce…
e710cb2 
…ptorAnnotationTest
@lukaszlenart
WW-5627 docs(cookie): document new 5-arg extension hook and deprecation
dd82f90
@lukaszlenart
WW-5627 wire OgnlParameterAllowlister in StrutsParameterAnnotationTes…
7d4c474 
…t fixture
@lukaszlenart
WW-5627 address SonarCloud findings on PR #1681
a751b59 
- S1948: mark transient on the new ParameterAuthorizer/ParameterAllowlister
  fields in CookieInterceptor and ParametersInterceptor (the host classes
  are Serializable; the injected services are not).
- S1874: suppress the deprecation warning on the new 5-arg
  populateCookieValueIntoStack — the delegation to the deprecated 4-arg
  form is the contract that lets existing subclass overrides participate.
- S3776: extract `allowlistViaPropertyDescriptor` and
  `allowlistViaPublicField` from `OgnlParameterAllowlister.allowlistAuthorizedPath`
  to drop cognitive complexity below the threshold.
- S1068: remove the unused `mapping` test fixture field.
@lukaszlenart lukaszlenart force-pushed the WW-5627-cookie-authorization branch from 73f456b to a751b59 Compare May 14, 2026 13:08
@lukaszlenart lukaszlenart marked this pull request as ready for review May 14, 2026 13:09
@lukaszlenart
WW-5627 clarify ParameterAllowlister contract and tidy ParametersInte…
590aca6 
…rceptor

Rename `ParameterAllowlister#allowlistAuthorizedPath` to `primeAllowlistForPath`
to make the contract explicit: the SAM is a side-effect-only priming hook that
runs after `ParameterAuthorizer#isAuthorized` has already decided. A no-op
return means "no priming needed or possible", never "rejected". The interface
name stays channel-agnostic; only the impl class (`OgnlParameterAllowlister`)
binds the priming to OGNL's `ThreadAllowlist`.

Add a `LOG.debug` in `OgnlParameterAllowlister` for the case where authorization
passed but no `@StrutsParameter` could be located on the root property
(e.g. `ModelDriven` models without per-property annotations) so the
authorize-vs-prime gap is observable instead of surfacing later as an opaque
OGNL traversal failure.

Drop the dead `performOgnlAllowlisting` pass-through and its unused `paramDepth`
parameter from `ParametersInterceptor` — the depth check is already enforced
inside `OgnlParameterAllowlister.primeAllowlistForPath`, so the outer guard was
a redundant computation.

No behavior change.
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented May 14, 2026

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
88.7% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

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

Reviewers

@rgielen rgielen Awaiting requested review from rgielen

@jogep jogep Awaiting requested review from jogep

@aleksandr-m aleksandr-m Awaiting requested review from aleksandr-m

@yasserzamani yasserzamani Awaiting requested review from yasserzamani

@sdutry sdutry Awaiting requested review from sdutry

@cnenning cnenning Awaiting requested review from cnenning

@kusalk kusalk Awaiting requested review from kusalk

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@lukaszlenart