WW-5626 cleanup follow-ups for @StrutsParameter JSON/REST enforcement#1673
Open
lukaszlenart wants to merge 5 commits intomainfrom
Open
WW-5626 cleanup follow-ups for @StrutsParameter JSON/REST enforcement#1673lukaszlenart wants to merge 5 commits intomainfrom
lukaszlenart wants to merge 5 commits intomainfrom
Conversation
…iven resolution Move the ValueStack peek logic that derives the target object from action+ModelDriven state out of ParametersInterceptor and into ParameterAuthorizer. Callers that need both authorization and the resolved target (for downstream OGNL allowlisting) can now call resolveTarget once and reuse the result.
Replace the inline ValueStack peek in ParametersInterceptor#isParameterAnnotatedAndAllowlist with a call to ParameterAuthorizer#resolveTarget. The ModelDriven import is no longer needed in this class.
The (String) cast in filterUnauthorizedKeysRecursive threw ClassCastException for any custom JSONReader producing non-String keys. Replace with an instanceof pattern that debug-logs and skips entries whose key cannot be converted to a parameter path.
…meter filtering The existing ContentTypeInterceptorTest uses mock ContentTypeHandlers, so its requireAnnotations=true tests verify only that intercept() returns SUCCESS — they assert nothing about which properties were actually filtered. These integration tests use a real JacksonJsonHandler + a real StrutsParameterAuthorizer to verify end-to-end property-level filtering for top-level annotated/unannotated properties and nested properties at varying authorized depths. The SecureRestAction fixture documents a semantic divergence: REST's recursive copy authorizes each path level independently, so depth-0 authorization on the top-level property requires @StrutsParameter on the setter even when nested field access is the actual goal. ParametersInterceptor only requires the getter annotation. This divergence is tracked for the Approach C refactor.
…eserve SAM Making resolveTarget abstract broke ParameterAuthorizer as a functional interface, which the existing JSON and REST plugin tests rely on for lambda-based stubs: interceptor.setParameterAuthorizer((parameterName, target, action) -> true); The default returns the action unchanged — adequate for lambda test stubs whose authorization decisions don't depend on the resolved target. The production implementation (StrutsParameterAuthorizer) overrides this with the proper ModelDriven value-stack peek.
lukaszlenart
requested review from
aleksandr-m,
cnenning,
jogep,
kusalk,
rgielen,
sdutry and
yasserzamani
|
3 tasks
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.
Fixes WW-5626.
Part 1 of 2. Approach C — handler-level per-property authorization that replaces
ContentTypeInterceptor's two-phase deserialize-then-copy with a newAuthorizationAwareContentTypeHandlerinterface — will follow in a separate PR. This PR addresses only the three concerns from the WW-5624 review that survive that refactor:Changes
Centralized ModelDriven target resolution.
ParametersInterceptor.isParameterAnnotatedAndAllowlistpreviously did the value-stack peek itself, thenStrutsParameterAuthorizer.isAuthorizedindependently checkedtarget != action && action instanceof ModelDriven. NewParameterAuthorizer.resolveTarget(action)consolidates the logic. Kept as adefaultmethod to preserve the interface as a SAM (lambda-based test stubs continue to work).Defensive guard against non-String JSON keys.
JSONInterceptor.filterUnauthorizedKeysRecursivedidString key = (String) entry.getKey();on a rawMap. Safe with the built-inStrutsJSONReader, but a custom reader producing non-String keys would throwClassCastException. Replaced withinstanceof String keypattern that debug-logs and skips.Real REST integration tests.
ContentTypeInterceptorTest.testRequireAnnotationsEnabled_*usecom.mockobjectsmocks forContentTypeHandler, sotoObjectis a no-op — the tests proveintercept()returns SUCCESS but assert nothing about which properties were actually filtered. NewContentTypeInterceptorIntegrationTestuses a realJacksonJsonHandlerand a realStrutsParameterAuthorizerend-to-end with a mixed-annotationSecureRestActionfixture.Finding for follow-up PR
Building the REST integration tests surfaced a real semantic divergence (documented inline in
SecureRestAction.java): REST's recursive copy authorizes EACH path level independently, so@StrutsParameter(depth=1)ongetAddressis not enough to bindaddress.city— the settersetAddressalso needs@StrutsParameterto authorize the top-leveladdressat depth 0.ParametersInterceptoronly requires the getter annotation. This is captured for the Approach C PR.Out of scope
ContentTypeInterceptordeep-copy/scrub complexity, theisNestedBeanTypepackage-name heuristic, and the two-phase architectural smell — all to be replaced by Approach C.Test plan
mvn test -DskipAssembly -pl core— 2926 tests, zero regressionsmvn test -DskipAssembly -pl plugins/json— 125 tests, zero regressionsmvn test -DskipAssembly -pl plugins/rest— 81 tests, zero regressionsmvn test -DskipAssembly -pl '!plugins/bean-validation,!plugins/tiles'— full multi-module BUILD SUCCESS (the two excluded modules fail identically onmainfor unrelated reasons:AnnotationFormatErrorfrom Hibernate Validator on a test model, and missing Velocity dependencies in the tiles plugin)