This is an automated email from the ASF dual-hosted git repository. lukaszlenart pushed a commit to branch WW-5725-rest-buffered-set in repository https://gitbox.apache.org/repos/asf/struts.git
commit a209cc7930b422cd475e31c2950a876169948106 Author: Lukasz Lenart <[email protected]> AuthorDate: Fri Sep 11 15:58:58 2026 +0200 WW-5725 fix(rest): authorize the buffered creator path in AuthorizingSettableBeanProperty AuthorizingSettableBeanProperty authorized a property in deserializeAndSet and deserializeSetAndReturn and wrapped the value deserializer only for creator-bound properties. Jackson takes neither route for a non-creator property it buffers during property-based creation: a setter property that appears in the body before the last creator parameter is read through the final SettableBeanProperty.deserialize(), with no authorization and no path push, and assigned after construction through PropertyValue.Regular.assign -> SettableBeanProperty.set(), which the Delegating base forwarded unchecked. The same property after the last creator parameter, or on a setter-only type, was already rejected, so member order alone decided whether the check applied, and the members of a buffered bean-valued property were checked one level too shallow. Three gates now cover the paths between them: - deserializeAndSet / deserializeSetAndReturn keep authorizing the direct path and skipping the value on rejection, so the setter never fires, but no longer push the path themselves. - AuthorizingValueDeserializer wraps every property's value deserializer, not only creator properties, and owns the path push for nested members. It now covers all three of Jackson's entry points -- deserialize(p, ctxt), the in-place deserialize(p, ctxt, intoValue) used for setterless collections, and deserializeWithType for polymorphic properties -- and classifies the [0] element prefix on the property's declared type rather than the deserializer's handled type. On the direct path it re-checks a path deserializeAndSet already accepted; the authorizer call is stateless, so the answer is the same. - set / setAndReturn authorize the already-materialized assignment, which also covers a buffered null (Jackson skips the value deserializer for a null token) and the other callers of set() in jackson-databind: @JsonMerge, @JsonManagedReference, inner-class valued properties, EXTERNAL_PROPERTY type ids and the @JsonIdentityInfo id property, none of which were authorized before. All gates go through DynamicKeyAuthorizationContext so a dynamic-key scope authorizes by depth on every path. Tests cover the setter before and after the last creator parameter, a setter-only type with the same member order, a nested creator, a creator-plus-setter type inside a dynamic-key scope, a buffered bean-valued setter whose members must be authorized at their own depth, a setterless collection, and a polymorphic property -- the last two in the direction that matters: a sibling grant on the enclosing bean must not authorize a collapsed nested path. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> --- .../jackson/AuthorizingSettableBeanProperty.java | 74 ++++---- .../jackson/AuthorizingValueDeserializer.java | 81 +++++++-- .../jackson/ParameterAuthorizingModuleTest.java | 187 +++++++++++++++++++++ 3 files changed, 292 insertions(+), 50 deletions(-) diff --git a/plugins/rest/src/main/java/org/apache/struts2/rest/handler/jackson/AuthorizingSettableBeanProperty.java b/plugins/rest/src/main/java/org/apache/struts2/rest/handler/jackson/AuthorizingSettableBeanProperty.java index 14c3bcec7..a3365a8ff 100644 --- a/plugins/rest/src/main/java/org/apache/struts2/rest/handler/jackson/AuthorizingSettableBeanProperty.java +++ b/plugins/rest/src/main/java/org/apache/struts2/rest/handler/jackson/AuthorizingSettableBeanProperty.java @@ -20,9 +20,7 @@ package org.apache.struts2.rest.handler.jackson; import com.fasterxml.jackson.core.JsonParser; import com.fasterxml.jackson.databind.DeserializationContext; -import com.fasterxml.jackson.databind.JavaType; import com.fasterxml.jackson.databind.JsonDeserializer; -import com.fasterxml.jackson.databind.deser.CreatorProperty; import com.fasterxml.jackson.databind.deser.SettableBeanProperty; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -37,10 +35,10 @@ import java.io.IOException; * skipped via {@link JsonParser#skipChildren()}, so any nested object graph is never instantiated * and setter side effects on unauthorized properties never fire. * - * <p>Path tracking: the wrapper pushes the full path of the current property onto the context's - * path stack before delegating, then pops in a {@code finally} block. For collection / map / array-typed - * properties, the path pushed is suffixed with {@code [0]} so nested element members produce paths like - * {@code items[0].field} — matching {@code ParametersInterceptor} depth semantics.</p> + * <p>Path tracking for nested members is done by the {@link AuthorizingValueDeserializer} wrapped + * around every property's value deserializer, so the direct path and the buffered creator path + * compute the same paths. Values Jackson assigns after construction go through {@link #set} and + * {@link #setAndReturn}, which apply the same authorization to the already-materialized value.</p> * * <p>When {@link ParameterAuthorizationContext#isActive()} is {@code false}, this wrapper is a * straight pass-through to the delegate — no overhead for default-config requests.</p> @@ -61,17 +59,17 @@ public class AuthorizingSettableBeanProperty extends SettableBeanProperty.Delega } /** - * Creator-bound properties (records, {@code @JsonCreator} constructors) never reach - * {@link #deserializeAndSet}/{@link #deserializeSetAndReturn}: Jackson calls the {@code final} - * {@code SettableBeanProperty#deserialize} directly, through this property's own value deserializer. - * Wrap that deserializer with {@link AuthorizingValueDeserializer}, scoped to {@link CreatorProperty} - * so ordinary setter/field/builder properties -- already authorized below -- aren't double-checked. + * Creator-bound properties, and non-creator properties Jackson buffers while collecting creator + * parameters, never reach {@link #deserializeAndSet}/{@link #deserializeSetAndReturn}: Jackson calls + * the {@code final} {@code SettableBeanProperty#deserialize} directly, through this property's own + * value deserializer. Wrap that deserializer with {@link AuthorizingValueDeserializer} for every + * property; it owns the path push for nested members on both the direct and the buffered path. */ @Override public SettableBeanProperty withValueDeserializer(JsonDeserializer<?> deser) { JsonDeserializer<?> effective = deser; - if (delegate instanceof CreatorProperty && !(deser instanceof AuthorizingValueDeserializer)) { - effective = new AuthorizingValueDeserializer(deser, getName()); + if (!(deser instanceof AuthorizingValueDeserializer)) { + effective = new AuthorizingValueDeserializer(deser, getName(), getType()); } return _with(delegate.withValueDeserializer(effective)); } @@ -90,12 +88,7 @@ public class AuthorizingSettableBeanProperty extends SettableBeanProperty.Delega p.skipChildren(); return; } - ParameterAuthorizationContext.pushPath(prefixForNested(path)); - try { - delegate.deserializeAndSet(p, ctxt, instance); - } finally { - ParameterAuthorizationContext.popPath(); - } + delegate.deserializeAndSet(p, ctxt, instance); } @Override @@ -111,24 +104,41 @@ public class AuthorizingSettableBeanProperty extends SettableBeanProperty.Delega p.skipChildren(); return instance; } - ParameterAuthorizationContext.pushPath(prefixForNested(path)); - try { - return delegate.deserializeSetAndReturn(p, ctxt, instance); - } finally { - ParameterAuthorizationContext.popPath(); + return delegate.deserializeSetAndReturn(p, ctxt, instance); + } + + @Override + public void set(Object instance, Object value) throws IOException { + if (isAuthorizedForSet(instance)) { + delegate.set(instance, value); } } + @Override + public Object setAndReturn(Object instance, Object value) throws IOException { + if (isAuthorizedForSet(instance)) { + return delegate.setAndReturn(instance, value); + } + return instance; + } + /** - * For Collection / Map / Array properties, the path to push for nested element members is - * {@code path + "[0]"} — matching {@code ParametersInterceptor} bracket-depth semantics. Scalar / - * bean properties push the path unchanged. + * Guards the already-materialized assignment path: Jackson buffers non-creator properties seen + * before the last creator parameter and assigns them after construction via + * {@code PropertyValue.Regular.assign} -> {@code set()}, which does not go through + * {@link #deserializeAndSet}. */ - private String prefixForNested(String pathOfThisProperty) { - JavaType type = getType(); - if (type != null && (type.isCollectionLikeType() || type.isMapLikeType() || type.isArrayType())) { - return pathOfThisProperty + "[0]"; + private boolean isAuthorizedForSet(Object instance) { + if (!ParameterAuthorizationContext.isActive()) { + return true; + } + String path = ParameterAuthorizationContext.pathFor(getName()); + if (DynamicKeyAuthorizationContext.isAuthorized(path)) { + return true; } - return pathOfThisProperty; + LOG.warn("REST body parameter [{}] rejected by @StrutsParameter authorization on [{}]", + path, instance.getClass().getName()); + ParameterAuthorizationContext.markRedacted(); + return false; } } diff --git a/plugins/rest/src/main/java/org/apache/struts2/rest/handler/jackson/AuthorizingValueDeserializer.java b/plugins/rest/src/main/java/org/apache/struts2/rest/handler/jackson/AuthorizingValueDeserializer.java index fc9532960..a30cfe8ef 100644 --- a/plugins/rest/src/main/java/org/apache/struts2/rest/handler/jackson/AuthorizingValueDeserializer.java +++ b/plugins/rest/src/main/java/org/apache/struts2/rest/handler/jackson/AuthorizingValueDeserializer.java @@ -20,36 +20,40 @@ package org.apache.struts2.rest.handler.jackson; import com.fasterxml.jackson.core.JsonParser; import com.fasterxml.jackson.databind.DeserializationContext; +import com.fasterxml.jackson.databind.JavaType; import com.fasterxml.jackson.databind.JsonDeserializer; import com.fasterxml.jackson.databind.deser.std.DelegatingDeserializer; +import com.fasterxml.jackson.databind.jsontype.TypeDeserializer; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.apache.struts2.interceptor.parameter.ParameterAuthorizationContext; import java.io.IOException; -import java.util.Collection; -import java.util.Map; /** - * Enforces {@code @StrutsParameter} authorization for creator-bound properties (Java records, - * {@code @JsonCreator} constructors, {@code @ConstructorProperties}), which Jackson deserializes - * through the value deserializer directly rather than through a {@code SettableBeanProperty}. - * See {@link AuthorizingSettableBeanProperty#withValueDeserializer} for where this is installed. + * Enforces {@code @StrutsParameter} authorization on a property's value deserializer, and owns the + * path push for its nested members. It is installed on every property by + * {@link AuthorizingSettableBeanProperty#withValueDeserializer}, so the same path is computed whether + * Jackson reaches the value through {@code deserializeAndSet}, through the {@code final} + * {@code SettableBeanProperty#deserialize} used for creator parameters and buffered properties, in + * place for a setterless collection, or through a type deserializer for a polymorphic property. */ final class AuthorizingValueDeserializer extends DelegatingDeserializer { private static final Logger LOG = LogManager.getLogger(AuthorizingValueDeserializer.class); private final String propertyName; + private final JavaType propertyType; - AuthorizingValueDeserializer(JsonDeserializer<?> delegate, String propertyName) { + AuthorizingValueDeserializer(JsonDeserializer<?> delegate, String propertyName, JavaType propertyType) { super(delegate); this.propertyName = propertyName; + this.propertyType = propertyType; } @Override protected JsonDeserializer<?> newDelegatingInstance(JsonDeserializer<?> newDelegatee) { - return new AuthorizingValueDeserializer(newDelegatee, propertyName); + return new AuthorizingValueDeserializer(newDelegatee, propertyName, propertyType); } @Override @@ -58,10 +62,7 @@ final class AuthorizingValueDeserializer extends DelegatingDeserializer { return super.deserialize(p, ctxt); } String path = ParameterAuthorizationContext.pathFor(propertyName); - if (!DynamicKeyAuthorizationContext.isAuthorized(path)) { - LOG.warn("REST body parameter [{}] rejected by @StrutsParameter authorization (creator-bound property)", path); - ParameterAuthorizationContext.markRedacted(); - p.skipChildren(); + if (!authorize(path, p)) { // Returning null redacts the value. For a primitive creator component this becomes the // type default (0/false) unless FAIL_ON_NULL_FOR_PRIMITIVES is on (then construction // fails and RedactionAwareDeserializer drops the whole object) -- either way the @@ -76,15 +77,59 @@ final class AuthorizingValueDeserializer extends DelegatingDeserializer { } } + @Override + public Object deserialize(JsonParser p, DeserializationContext ctxt, Object intoValue) throws IOException { + if (!ParameterAuthorizationContext.isActive()) { + return super.deserialize(p, ctxt, intoValue); + } + String path = ParameterAuthorizationContext.pathFor(propertyName); + if (!authorize(path, p)) { + return intoValue; + } + ParameterAuthorizationContext.pushPath(prefixForNested(path)); + try { + return super.deserialize(p, ctxt, intoValue); + } finally { + ParameterAuthorizationContext.popPath(); + } + } + + @Override + public Object deserializeWithType(JsonParser p, DeserializationContext ctxt, TypeDeserializer typeDeserializer) + throws IOException { + if (!ParameterAuthorizationContext.isActive()) { + return super.deserializeWithType(p, ctxt, typeDeserializer); + } + String path = ParameterAuthorizationContext.pathFor(propertyName); + if (!authorize(path, p)) { + return null; + } + ParameterAuthorizationContext.pushPath(prefixForNested(path)); + try { + return super.deserializeWithType(p, ctxt, typeDeserializer); + } finally { + ParameterAuthorizationContext.popPath(); + } + } + + private boolean authorize(String path, JsonParser p) throws IOException { + if (DynamicKeyAuthorizationContext.isAuthorized(path)) { + return true; + } + LOG.warn("REST body parameter [{}] rejected by @StrutsParameter authorization", path); + ParameterAuthorizationContext.markRedacted(); + p.skipChildren(); + return false; + } + /** - * For Collection / Map / Array-valued creator parameters, the path to push for nested element - * members is {@code path + "[0]"} -- matching {@code ParametersInterceptor} bracket-depth - * semantics, and {@link AuthorizingSettableBeanProperty#prefixForNested}. Scalar / bean-valued - * parameters push the path unchanged. + * For Collection / Map / Array properties, the path to push for nested element members is + * {@code path + "[0]"} -- matching {@code ParametersInterceptor} bracket-depth semantics. Scalar / + * bean-valued properties push the path unchanged. */ private String prefixForNested(String pathOfThisProperty) { - Class<?> type = handledType(); - if (type != null && (Collection.class.isAssignableFrom(type) || Map.class.isAssignableFrom(type) || type.isArray())) { + if (propertyType != null + && (propertyType.isCollectionLikeType() || propertyType.isMapLikeType() || propertyType.isArrayType())) { return pathOfThisProperty + "[0]"; } return pathOfThisProperty; diff --git a/plugins/rest/src/test/java/org/apache/struts2/rest/handler/jackson/ParameterAuthorizingModuleTest.java b/plugins/rest/src/test/java/org/apache/struts2/rest/handler/jackson/ParameterAuthorizingModuleTest.java index 61ee26eaf..ccee59d44 100644 --- a/plugins/rest/src/test/java/org/apache/struts2/rest/handler/jackson/ParameterAuthorizingModuleTest.java +++ b/plugins/rest/src/test/java/org/apache/struts2/rest/handler/jackson/ParameterAuthorizingModuleTest.java @@ -21,6 +21,8 @@ package org.apache.struts2.rest.handler.jackson; import com.fasterxml.jackson.annotation.JsonAnySetter; import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonProperty; +import com.fasterxml.jackson.annotation.JsonSubTypes; +import com.fasterxml.jackson.annotation.JsonTypeInfo; import com.fasterxml.jackson.annotation.JsonUnwrapped; import com.fasterxml.jackson.core.JsonParser; import com.fasterxml.jackson.databind.BeanDescription; @@ -434,6 +436,96 @@ public class ParameterAuthorizingModuleTest extends TestCase { assertNull(result.secret); } + public void testSetterBufferedBeforeCreatorParamIsAuthorized() throws Exception { + // A non-creator property that appears before the last creator parameter is buffered by + // Jackson and assigned through SettableBeanProperty.set() after construction. + bind((path, t, a) -> "name".equals(path), new CreatorWithSetter("")); + CreatorWithSetter result = mapper.readValue("{\"role\":\"admin\",\"name\":\"alice\"}", CreatorWithSetter.class); + assertEquals("alice", result.name); + assertNull("buffered setter property assigned without authorization ?", result.getRole()); + } + + public void testSetterAfterCreatorParamIsAuthorized() throws Exception { + // Control: the same property after the last creator parameter takes the direct path. + bind((path, t, a) -> "name".equals(path), new CreatorWithSetter("")); + CreatorWithSetter result = mapper.readValue("{\"name\":\"alice\",\"role\":\"admin\"}", CreatorWithSetter.class); + assertEquals("alice", result.name); + assertNull(result.getRole()); + } + + public void testSetterOnlyTypeSameMemberOrderIsAuthorized() throws Exception { + // Control: without a creator nothing is buffered, so member order does not matter. + bind((path, t, a) -> "name".equals(path), new Person()); + Person result = mapper.readValue("{\"role\":\"admin\",\"name\":\"alice\"}", Person.class); + assertEquals("alice", result.name); + assertNull(result.role); + } + + public void testNestedSetterBufferedBeforeCreatorParamIsAuthorized() throws Exception { + bind((path, t, a) -> "inner".equals(path) || "inner.name".equals(path), new CreatorHolder(null)); + CreatorHolder result = mapper.readValue( + "{\"inner\":{\"role\":\"admin\",\"name\":\"alice\"}}", CreatorHolder.class); + assertNotNull(result.inner); + assertEquals("alice", result.inner.name); + assertNull("nested buffered setter property assigned without authorization ?", result.inner.getRole()); + } + + public void testBufferedBeanValuedSetterChildrenAuthorizedAtOwnDepth() throws Exception { + // The buffered read goes through the final SettableBeanProperty.deserialize(), so the + // nested bean's members must still be authorized under the property's own prefix. + bind((path, t, a) -> "inner".equals(path) || "inner.name".equals(path) + || "inner.address".equals(path) || "inner.address.city".equals(path), new CreatorAddressHolder(null)); + CreatorAddressHolder result = mapper.readValue( + "{\"inner\":{\"address\":{\"city\":\"Warsaw\",\"zip\":\"00-001\"},\"name\":\"alice\"}}", + CreatorAddressHolder.class); + assertEquals("alice", result.inner.name); + assertNotNull("buffered nested bean dropped ?", result.inner.getAddress()); + assertEquals("nested member checked at the wrong depth ?", "Warsaw", result.inner.getAddress().city); + assertNull(result.inner.getAddress().zip); + } + + public void testBeanValuedSetterAfterCreatorParamChildrenAuthorizedAtOwnDepth() throws Exception { + // Control: the direct path for the same property and authorizer. + bind((path, t, a) -> "inner".equals(path) || "inner.name".equals(path) + || "inner.address".equals(path) || "inner.address.city".equals(path), new CreatorAddressHolder(null)); + CreatorAddressHolder result = mapper.readValue( + "{\"inner\":{\"name\":\"alice\",\"address\":{\"city\":\"Warsaw\",\"zip\":\"00-001\"}}}", + CreatorAddressHolder.class); + assertEquals("alice", result.inner.name); + assertEquals("Warsaw", result.inner.getAddress().city); + assertNull(result.inner.getAddress().zip); + } + + public void testSetterlessCollectionElementsAuthorizedAtOwnDepth() throws Exception { + // A collection getter without a setter is deserialized in place through the three-argument + // deserialize(); element members must be checked under items[0], not under the parent. + bind((path, t, a) -> "order".equals(path) || "order.items".equals(path) || "order.name".equals(path), + new OrderHolder()); + OrderHolder result = mapper.readValue("{\"order\":{\"items\":[{\"name\":\"x\"}]}}", OrderHolder.class); + assertEquals(1, result.order.getItems().size()); + assertNull("element member authorized by the parent's sibling grant ?", result.order.getItems().get(0).name); + } + + public void testPolymorphicPropertyMembersAuthorizedAtOwnDepth() throws Exception { + // A @JsonTypeInfo property is deserialized through deserializeWithType(); the subtype's + // members must be checked under pet, not against the enclosing bean. + bind((path, t, a) -> "pet".equals(path) || "owner".equals(path), new Kennel()); + Kennel result = mapper.readValue("{\"pet\":{\"@type\":\"dog\",\"owner\":\"alice\"}}", Kennel.class); + assertTrue(result.pet instanceof Dog); + assertNull("subtype member authorized by the enclosing bean's sibling grant ?", ((Dog) result.pet).owner); + } + + public void testBufferedSetterInsideDynamicKeyScopeIsAuthorizedByDepth() throws Exception { + // Inside a dynamic-key scope the buffered path must consult the same depth rule as the + // direct path, not the annotation authorizer (which rejects everything here). + ObjectMapper enforcingMapper = enforcingMapper(); + bind((path, t, a) -> false, new DynamicDepthTwoCreatorAnySetterBean()); + DynamicDepthTwoCreatorAnySetterBean result = enforcingMapper.readValue( + "{\"home\":{\"role\":\"admin\",\"name\":\"alice\"}}", DynamicDepthTwoCreatorAnySetterBean.class); + assertEquals("alice", result.values.get("home").name); + assertEquals("admin", result.values.get("home").getRole()); + } + public void testCreatorPropertyEntirelyRejected_dropsWholeSubtree() throws Exception { // "inner" itself is never authorized -- the whole nested creator-bound object must be // dropped, matching how a rejected non-creator nested bean property behaves (see @@ -555,6 +647,91 @@ public class ParameterAuthorizingModuleTest extends TestCase { public RecordAddress recordAddress; } + public static class CreatorWithSetter { + public final String name; + private String role; + + @JsonCreator(mode = JsonCreator.Mode.PROPERTIES) + public CreatorWithSetter(@JsonProperty("name") String name) { + this.name = name; + } + + public String getRole() { + return role; + } + + public void setRole(String role) { + this.role = role; + } + } + + public static class CreatorWithAddress { + public final String name; + private Address address; + + @JsonCreator(mode = JsonCreator.Mode.PROPERTIES) + public CreatorWithAddress(@JsonProperty("name") String name) { + this.name = name; + } + + public Address getAddress() { + return address; + } + + public void setAddress(Address address) { + this.address = address; + } + } + + public static class CreatorAddressHolder { + public final CreatorWithAddress inner; + + @JsonCreator(mode = JsonCreator.Mode.PROPERTIES) + public CreatorAddressHolder(@JsonProperty("inner") CreatorWithAddress inner) { + this.inner = inner; + } + } + + public static class OrderHolder { + public Order order; + } + + public static class Order { + public String name; + private final java.util.List<OrderItem> itemList = new java.util.ArrayList<>(); + + public java.util.List<OrderItem> getItems() { + return itemList; + } + } + + public static class OrderItem { + public String name; + } + + @JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "@type") + @JsonSubTypes(@JsonSubTypes.Type(value = Dog.class, name = "dog")) + public abstract static class Animal { + } + + public static class Dog extends Animal { + public String owner; + } + + public static class Kennel { + public Animal pet; + public String owner; + } + + public static class CreatorHolder { + public final CreatorWithSetter inner; + + @JsonCreator(mode = JsonCreator.Mode.PROPERTIES) + public CreatorHolder(@JsonProperty("inner") CreatorWithSetter inner) { + this.inner = inner; + } + } + public static class Address { public String city; public String zip; @@ -626,6 +803,16 @@ public class ParameterAuthorizingModuleTest extends TestCase { } } + public static class DynamicDepthTwoCreatorAnySetterBean { + public final Map<String, CreatorWithSetter> values = new LinkedHashMap<>(); + + @JsonAnySetter + @StrutsParameter(allowDynamicKeys = true, depth = 2) + public void put(String name, CreatorWithSetter value) { + values.put(name, value); + } + } + public static class DynamicDepthTwoAnySetterBean { public final Map<String, Address> values = new LinkedHashMap<>();
