WW-5642 fix(rest): authorize @StrutsParameter on record/creator-bound REST body properties#1774
WW-5642 fix(rest): authorize @StrutsParameter on record/creator-bound REST body properties#1774g0w6y wants to merge 3 commits into
Conversation
…dy properties ParameterAuthorizingModule enforces @StrutsParameter on REST/JSON body deserialization by wrapping each property's deserializeAndSet/ deserializeSetAndReturn. Jackson never calls either method for creator-bound properties (Java records, @JsonCreator constructors, @ConstructorProperties) — it calls SettableBeanProperty#deserialize directly, which is declared final and bypasses the wrapper entirely. With struts.parameters.requireAnnotations enabled, any record-typed field anywhere in a REST action's request body was populated with no authorization check at all. Add AuthorizingValueDeserializer, which wraps the property's value deserializer instead of the property itself, and install it from AuthorizingSettableBeanProperty#withValueDeserializer — scoped to CreatorProperty so ordinary setter/field/builder properties, already authorized via the existing wrapper, aren't checked twice.
|
Thanks for the PR but please create a JIRA ticket to cover this work 🙏 |
g0w6y
changed the title
|
Created WW-5642 to track this: https://issues.apache.org/jira/browse/WW-5642. Ready for review/merge whenever you get a chance thanks! |
|
Given this fixes an authorization bypass where unauthorized REST body fields could be set, should we handle this as a security advisory instead of a normal fix? Happy to follow whichever process you prefer, just let me know. |
|
Thanks — the creator-bound gap is real and worth closing. As it stands though the coverage isn't complete: it handles records/ Are you planning to extend it, or would you prefer I take it forward under a linked JIRA? |
…zed, not fatal AuthorizingValueDeserializer substitutes null for a rejected creator-bound property (record component, @JsonCreator/@ConstructorProperties param). For reference-typed, unvalidated components this is a harmless stand-in for "not set" -- but two cases turn that substitution into an unhandled exception that crashes deserialization of the entire request body instead of just dropping the unauthorized subtree: - A record/constructor with its own non-null validation (e.g. a compact constructor doing Objects.requireNonNull) throws ValueInstantiationException when the redacted component reaches it. - With DeserializationFeature.FAIL_ON_NULL_FOR_PRIMITIVES enabled, Jackson itself throws MismatchedInputException when a primitive-typed creator component is redacted to null. Add RedactionAwareDeserializer, wrapping every bean-type deserializer via a new BeanDeserializerModifier#modifyDeserializer hook. It tracks (via a new redaction-scope stack in ParameterAuthorizationContext) whether the object currently under construction had a property redacted by authorization; if construction then throws, the object is treated as unauthorized (returns null) instead of propagating the raw exception -- matching the same fail-closed outcome already used when a non-creator nested property is rejected outright. A guard test confirms genuine, unrelated validation failures (nothing redacted) still propagate normally, so real client errors aren't masked. Also verified (and added regression coverage for) the other gaps raised in review: static factory-method @JsonCreator, @ConstructorProperties, top-level records, 3-level nested creator chains, and List/Map creator params whose elements are further creator-bound or plain-POJO types -- all of these were already handled correctly by the existing withValueDeserializer interception.
|
I recommend reading SECURITY guideline first to understand how responsible disclosure looks like |
|
Thanks for the thorough follow-up —
The Three small, non-blocking notes:
None of these block. Nice work tightening it up. |
There was a problem hiding this comment.
Pull request overview
This PR strengthens Struts REST/Jackson request-body parameter authorization so @StrutsParameter enforcement also applies to creator-bound properties (Java records, @JsonCreator/@ConstructorProperties), closing an authorization gap when struts.parameters.requireAnnotations is enabled.
Changes:
- Add
AuthorizingValueDeserializerand wire it viaAuthorizingSettableBeanProperty#withValueDeserializer(scoped toCreatorProperty) to authorize creator-bound properties that bypassdeserializeAndSet/deserializeSetAndReturn. - Add
RedactionAwareDeserializerwrapper (installed fromParameterAuthorizingModule) plusParameterAuthorizationContextredaction-scope tracking to fail-closed (drop object) when redaction causes creator/builder construction failures. - Expand REST plugin tests with regression coverage for records, creators, nested/collection/map paths, and redaction-triggered construction failures.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| plugins/rest/src/test/java/org/apache/struts2/rest/handler/jackson/ParameterAuthorizingModuleTest.java | Adds regression tests covering record/creator-bound authorization paths and redaction-driven fail-closed behavior. |
| plugins/rest/src/main/java/org/apache/struts2/rest/handler/jackson/RedactionAwareDeserializer.java | New bean-deserializer wrapper to drop objects (return null) when construction fails after authorization redaction. |
| plugins/rest/src/main/java/org/apache/struts2/rest/handler/jackson/ParameterAuthorizingModule.java | Installs RedactionAwareDeserializer via BeanDeserializerModifier#modifyDeserializer. |
| plugins/rest/src/main/java/org/apache/struts2/rest/handler/jackson/AuthorizingValueDeserializer.java | New value-deserializer wrapper to enforce authorization for creator-bound properties. |
| plugins/rest/src/main/java/org/apache/struts2/rest/handler/jackson/AuthorizingSettableBeanProperty.java | Wraps creator-property value deserializers; marks redaction when rejecting properties. |
| core/src/main/java/org/apache/struts2/interceptor/parameter/ParameterAuthorizationContext.java | Adds redaction-scope stack APIs (push/popRedactionScope, markRedacted, wasRedactedInCurrentScope). |
| core/src/test/java/org/apache/struts2/interceptor/parameter/ParameterAuthorizationContextTest.java | Adds unit tests for redaction-scope semantics and cleanup on unbind(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try { | ||
| return super.deserialize(p, ctxt); | ||
| } catch (JsonMappingException e) { | ||
| if (!ParameterAuthorizationContext.wasRedactedInCurrentScope()) { | ||
| throw e; | ||
| } | ||
| LOG.warn("REST body object of type [{}] failed to construct after @StrutsParameter " + | ||
| "redaction dropped one of its properties; treating the object as unauthorized: {}", | ||
| handledType() != null ? handledType().getName() : "?", e.getMessage()); | ||
| swallowed = true; | ||
| return null; | ||
| } |
Addresses the three non-blocking review notes on WW-5642: - Add testArrayOfRecordsAsCreatorParam_elementsAuthorizedByIndexedPath and a WithArray fixture, exercising the type.isArray() branch of AuthorizingValueDeserializer#prefixForNested so the collection matrix (List/Map/array) is fully covered. - Document in AuthorizingValueDeserializer that redacting a primitive creator component becomes the type default (0/false) when FAIL_ON_NULL_FOR_PRIMITIVES is off -- a deliberate choice, the client value never lands either way. - Document in RedactionAwareDeserializer that a redaction co-located with an unrelated mapping error is folded into "object dropped" -- a deliberate fail-closed trade-off, never exposing a partial object.
|
Addressed all three notes in 3b184bf. Added the array creator param test (testArrayOfRecordsAsCreatorParam) with a WithArray fixture, so the collection matrix now covers List, Map and array. Also added the two clarifying comments you suggested: one in AuthorizingValueDeserializer noting that a redacted primitive component falls back to the type default when FAIL_ON_NULL_FOR_PRIMITIVES is off, and one in RedactionAwareDeserializer noting that a redaction co located with an unrelated mapping error is folded into object dropped as a deliberate fail closed choice. Full ParameterAuthorizingModuleTest suite passes locally (24 tests). This change lives in the REST Jackson plugin and is independent of the JSON plugin per request refactor in #1782, so it can land on its own. Ready whenever you are. |
|
Thanks for this — the analysis is correct and the fix is well-targeted. To frame it for anyone reading: the per-property The approach (wrapping the creator-property value deserializer, plus fail-closing construction that fails after a redaction) is sound, and the record / static-factory / |
Fixes WW-5642
Summary
ParameterAuthorizingModuleenforces@StrutsParameteron REST/JSON body deserialization by wrapping each Jackson property'sdeserializeAndSet/deserializeSetAndReturn(AuthorizingSettableBeanProperty). Jackson never calls either method for creator-bound properties — Javarecords,@JsonCreatorconstructors,@ConstructorProperties— it instead callsSettableBeanProperty#deserializedirectly, which Jackson declaresfinal, so the existing wrapper cannot intercept it.Practical effect: with
struts.parameters.requireAnnotationsenabled, anyrecord-typed field anywhere in a REST action's request body (top-level or nested) is populated with no@StrutsParametercheck at all, silently defeating the protection for that entire subtree. This is easy to hit unintentionally —recordis the idiomatic way to model immutable REST DTOs on the Java versions Struts now targets.Verified with a live reproduction before writing the fix: a
recordfield with an unauthorized component was populated anyway; after the fix, the same component is correctly rejected while an authorized sibling component still passes through.Fix
Add
AuthorizingValueDeserializer, which wraps the property's value deserializer rather than theSettableBeanProperty— the value deserializer is what the finaldeserialize()method delegates to, so wrapping it is the only available interception point for the creator-bound path. It is installed fromAuthorizingSettableBeanProperty#withValueDeserializer, scoped toCreatorPropertyspecifically, so ordinary setter/field/builder-pattern properties — already correctly authorized via the existingdeserializeAndSet/deserializeSetAndReturnoverrides — are not checked a second time (which would also double-push the path-authorization stack and produce incorrect nested paths).Test plan
testRecordComponentAuthorizedByPathinParameterAuthorizingModuleTest: arecord-typed nested property with one authorized and one unauthorized component — confirms the unauthorized component is rejected and the authorized one still deserializes.ParameterAuthorizingModuleTestsuite (setter, builder, nested, collection, map, array authorization paths) passes unchanged — confirms no regression from scoping the new check to creator-bound properties only.struts2-rest-pluginmodule test suite passes (106/106).