WW-5627 Gate CookieInterceptor through ParameterAuthorizer#1681
Draft
lukaszlenart wants to merge 10 commits into
Draft
WW-5627 Gate CookieInterceptor through ParameterAuthorizer#1681lukaszlenart wants to merge 10 commits into
lukaszlenart wants to merge 10 commits into
Conversation
…LISTER constant Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…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>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…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.
…ptorAnnotationTest
- 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.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes WW-5627 —
CookieInterceptor.populateCookieValueIntoStackcallsstack.setValuedirectly (CookieInterceptor.java:339,348pre-fix), bypassingStrutsParameterAuthorizerso cookies write to action setters that are not annotated with@StrutsParametereven whenstruts.parameters.requireAnnotations=true.This PR retrofits the
@StrutsParameterenforcement onto the cookie input channel:ParameterAllowlisterinterface (extension point) + defaultOgnlParameterAllowlisterimpl extracted fromParametersInterceptor.performOgnlAllowlisting. Mirrors theParameterAuthorizerpair from WW-5626. Apps can swap implementations via<constant name="struts.parameterAllowlister" value="..."/>.ParametersInterceptordelegates to the new component (pure refactor, all 35 existing tests pass unchanged).CookieInterceptoradds a newprotected void populateCookieValueIntoStack(name, value, map, stack, action)extension hook. The default impl runsparameterAuthorizer.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.struts-beans.xml,StrutsBeanSelectionProvider, andDefaultConfiguration.bootstrapFactories().Default-config invariant: when
struts.parameters.requireAnnotations=false(the 7.x default),ParameterAuthorizer.isAuthorizedshort-circuits totrue, so existing apps see no behavior change. Only opt-in apps see cookies gated.Behavior matrix
@StrutsParameter?requireAnnotations=false(default)requireAnnotations=trueflatNamerequireAnnotations=trueflatNamerequireAnnotations=trueuser.roleThreadAllowlistprimedrequireAnnotations=trueuser.rolerequireAnnotations=true, transitionflatNamerequireAnnotations=true, ModelDriven modelMigration notes (for the Confluence wiki)
Stacking
Stacked on top of
WW-5626-approach-c(whereParameterAuthorizerandStrutsParameterAuthorizerwere introduced). When WW-5626 merges tomain, 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).OgnlParameterAllowlisterTest(6 unit tests).CookieInterceptorAnnotationTest(8 tests covering the full behavior matrix above, including subclass-override-of-deprecated-hook gets the gate, ModelDriven exemption, transition mode).CookieInterceptorTest(9 tests) updated to wire pass-through lambdas via adisableAuthorizationGate(...)helper since those tests constructCookieInterceptorvianewrather than the container.ParametersInterceptorTest,ActionMappingParametersInterceptorTest,StaticParametersInterceptorTest,StrutsParameterAnnotationTestall green after the refactor.Out of scope (separate Jira candidates)
ScopeInterceptor(ScopeInterceptor.java:306,328) has the samestack.setValuepattern from session/application scope — likely warrants its own ticket.ParametersInterceptor.hasValidAnnotated*helpers (lines 432–548) appear to be dead post-WW-5626 (onlyStrutsParameterAuthorizer's versions are reached at runtime). Cleanup is a separate audit.ParameterNameAwareconsultation inCookieInterceptor— not currently consulted; adding it would be a larger behavior change.🤖 Generated with Claude Code