From 6110b8f6baa9fe1aa7fe5ad5e2655a7cbe4a75d9 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 4 May 2026 13:40:45 +0200 Subject: [PATCH 01/13] WW-5626 spike: validate Jackson per-property authorization mechanism Validates that the Approach C design is feasible before committing to a detailed implementation plan. Wraps each SettableBeanProperty via BeanDeserializerModifier; intercepts deserializeAndSet to authorize against a path built from a ThreadLocal Deque; uses skipChildren() to discard unauthorized values; uses [0] suffix for collection/map/array elements to match ParametersInterceptor depth semantics. Findings: - Delegating base class via 'protected delegate' field is the right pattern - addOrReplaceProperty(prop, true) is the correct builder API - Reject-at-parent skips all nested deserialization (better security than two-phase copy: setter side effects on unauthorized properties never fire) - JavaType#isCollectionLikeType/isMapLikeType/isArrayType detects the indexed-path case Spike is kept under .../spike/ as a learning artifact; it will be replaced by production code + tests in subsequent commits. --- .../rest/spike/JacksonAuthSpikeTest.java | 257 ++++++++++++++++++ 1 file changed, 257 insertions(+) create mode 100644 plugins/rest/src/test/java/org/apache/struts2/rest/spike/JacksonAuthSpikeTest.java diff --git a/plugins/rest/src/test/java/org/apache/struts2/rest/spike/JacksonAuthSpikeTest.java b/plugins/rest/src/test/java/org/apache/struts2/rest/spike/JacksonAuthSpikeTest.java new file mode 100644 index 0000000000..204f87bbb5 --- /dev/null +++ b/plugins/rest/src/test/java/org/apache/struts2/rest/spike/JacksonAuthSpikeTest.java @@ -0,0 +1,257 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.struts2.rest.spike; + +import com.fasterxml.jackson.core.JsonParser; +import com.fasterxml.jackson.databind.BeanDescription; +import com.fasterxml.jackson.databind.DeserializationConfig; +import com.fasterxml.jackson.databind.DeserializationContext; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.deser.BeanDeserializerBuilder; +import com.fasterxml.jackson.databind.deser.BeanDeserializerModifier; +import com.fasterxml.jackson.databind.deser.SettableBeanProperty; +import com.fasterxml.jackson.databind.module.SimpleModule; +import junit.framework.TestCase; + +import java.util.ArrayDeque; +import java.util.Deque; +import java.util.Iterator; +import java.util.function.BiPredicate; + +/** + * SPIKE — NOT PRODUCTION CODE. + * + * Validates that Jackson's BeanDeserializerModifier + SettableBeanProperty wrapping can + * intercept per-property deserialization for Approach C (handler-level @StrutsParameter + * authorization). Path tracking uses a ThreadLocal Deque pushed/popped around each + * authorized property's deserialization. + * + * Three claims under test: + * (1) updateBuilder() can replace SettableBeanProperty instances on the builder + * (2) A wrapping property can call parser.skipChildren() to discard unauthorized values + * (3) Path tracking via ThreadLocal Deque produces dot/bracket paths matching + * ParametersInterceptor depth semantics for nested objects + * + * If green, this approach is viable for production. + */ +public class JacksonAuthSpikeTest extends TestCase { + + // --- ThreadLocal path tracking --- + + private static final ThreadLocal> PATH_STACK = ThreadLocal.withInitial(ArrayDeque::new); + + private static String currentPath(String propertyName) { + Deque stack = PATH_STACK.get(); + if (stack.isEmpty()) { + return propertyName; + } + // Build path: stack contains parent prefix(es) bottom-to-top + StringBuilder sb = new StringBuilder(); + Iterator it = stack.descendingIterator(); + while (it.hasNext()) { + sb.append(it.next()); + sb.append('.'); + } + sb.append(propertyName); + return sb.toString(); + } + + // --- Wrapping SettableBeanProperty --- + + static class AuthorizingSettableBeanProperty extends SettableBeanProperty.Delegating { + private final BiPredicate authorizer; + + AuthorizingSettableBeanProperty(SettableBeanProperty delegate, BiPredicate authorizer) { + super(delegate); + this.authorizer = authorizer; + } + + @Override + protected SettableBeanProperty withDelegate(SettableBeanProperty d) { + return new AuthorizingSettableBeanProperty(d, authorizer); + } + + /** + * For Collection/Map/Array properties, the path to push for nested element members must include + * the indexed bracket suffix so children build paths like "items[0].field" — matching + * ParametersInterceptor depth semantics and the existing JSONInterceptor recursive filter. + * For scalar/bean properties, push the path unchanged. + */ + private String prefixForNested(String pathOfThisProperty) { + com.fasterxml.jackson.databind.JavaType type = getType(); + if (type != null && (type.isCollectionLikeType() || type.isMapLikeType() || type.isArrayType())) { + return pathOfThisProperty + "[0]"; + } + return pathOfThisProperty; + } + + @Override + public void deserializeAndSet(JsonParser p, DeserializationContext ctxt, Object instance) throws java.io.IOException { + String path = currentPath(getName()); + if (!authorizer.test(path, instance)) { + p.skipChildren(); // discard the JSON value for this property + return; + } + PATH_STACK.get().push(prefixForNested(path)); + try { + delegate.deserializeAndSet(p, ctxt, instance); + } finally { + PATH_STACK.get().pop(); + } + } + + @Override + public Object deserializeSetAndReturn(JsonParser p, DeserializationContext ctxt, Object instance) throws java.io.IOException { + String path = currentPath(getName()); + if (!authorizer.test(path, instance)) { + p.skipChildren(); + return instance; + } + PATH_STACK.get().push(prefixForNested(path)); + try { + return delegate.deserializeSetAndReturn(p, ctxt, instance); + } finally { + PATH_STACK.get().pop(); + } + } + } + + // --- Module that registers the modifier --- + + static ObjectMapper buildAuthorizingMapper(BiPredicate authorizer) { + SimpleModule module = new SimpleModule(); + module.setDeserializerModifier(new BeanDeserializerModifier() { + @Override + public BeanDeserializerBuilder updateBuilder(DeserializationConfig config, + BeanDescription beanDesc, + BeanDeserializerBuilder builder) { + Iterator it = builder.getProperties(); + while (it.hasNext()) { + SettableBeanProperty original = it.next(); + builder.addOrReplaceProperty(new AuthorizingSettableBeanProperty(original, authorizer), true); + } + return builder; + } + }); + return new ObjectMapper().registerModule(module); + } + + // --- Test fixtures --- + + public static class Person { + public String name; + public String role; + public Address address; + public java.util.List
addresses; + public Address[] addressArray; + public java.util.Map addressMap; + } + + public static class Address { + public String city; + public String zip; + } + + // --- Tests --- + + @Override + protected void setUp() { + PATH_STACK.remove(); + } + + public void testTopLevelAuthorizedPropertyIsApplied() throws Exception { + ObjectMapper mapper = buildAuthorizingMapper((path, instance) -> "name".equals(path)); + Person p = mapper.readValue("{\"name\":\"alice\",\"role\":\"admin\"}", Person.class); + assertEquals("alice", p.name); + assertNull("role must be skipped", p.role); + } + + public void testNestedPropertyAuthorizedByFullPath() throws Exception { + // Authorize address (depth 0) and address.city (depth 1), but NOT address.zip + ObjectMapper mapper = buildAuthorizingMapper((path, instance) -> + "address".equals(path) || "address.city".equals(path)); + Person p = mapper.readValue("{\"address\":{\"city\":\"Warsaw\",\"zip\":\"00-001\"}}", Person.class); + assertNotNull("address should be set", p.address); + assertEquals("Warsaw", p.address.city); + assertNull("zip must be skipped because address.zip not authorized", p.address.zip); + } + + public void testNestedRejectedAtParent() throws Exception { + // Reject "address" entirely at depth 0; nested fields should not be visited + ObjectMapper mapper = buildAuthorizingMapper((path, instance) -> "name".equals(path)); + Person p = mapper.readValue("{\"name\":\"alice\",\"address\":{\"city\":\"Warsaw\"}}", Person.class); + assertEquals("alice", p.name); + assertNull("address must be rejected at the parent, no nested visit", p.address); + } + + // --- Collection / Array / Map indexed-path tests --- + + public void testListOfBeansUsesIndexedPath() throws Exception { + // Authorize "addresses" (depth 0) AND "addresses[0].city" (depth 2) but NOT "addresses[0].zip" + ObjectMapper mapper = buildAuthorizingMapper((path, instance) -> + "addresses".equals(path) || "addresses[0].city".equals(path)); + Person p = mapper.readValue( + "{\"addresses\":[{\"city\":\"Warsaw\",\"zip\":\"00-001\"},{\"city\":\"Krakow\",\"zip\":\"30-001\"}]}", + Person.class); + assertNotNull(p.addresses); + assertEquals(2, p.addresses.size()); + assertEquals("Warsaw", p.addresses.get(0).city); + assertNull("addresses[0].zip must be skipped on element 0", p.addresses.get(0).zip); + assertEquals("Krakow", p.addresses.get(1).city); + assertNull("addresses[0].zip must be skipped on element 1 too (same path token)", p.addresses.get(1).zip); + } + + public void testListRejectedAtParentSkipsAllElements() throws Exception { + ObjectMapper mapper = buildAuthorizingMapper((path, instance) -> "name".equals(path)); + Person p = mapper.readValue( + "{\"name\":\"alice\",\"addresses\":[{\"city\":\"Warsaw\"}]}", Person.class); + assertEquals("alice", p.name); + assertNull("addresses must be rejected at parent — Jackson never visits elements", p.addresses); + } + + public void testArrayOfBeansUsesIndexedPath() throws Exception { + ObjectMapper mapper = buildAuthorizingMapper((path, instance) -> + "addressArray".equals(path) || "addressArray[0].city".equals(path)); + Person p = mapper.readValue( + "{\"addressArray\":[{\"city\":\"Warsaw\",\"zip\":\"00-001\"}]}", Person.class); + assertNotNull(p.addressArray); + assertEquals(1, p.addressArray.length); + assertEquals("Warsaw", p.addressArray[0].city); + assertNull("addressArray[0].zip must be skipped", p.addressArray[0].zip); + } + + public void testMapOfBeansUsesIndexedPath() throws Exception { + // Map values use [0] suffix (matching ParametersInterceptor bracket semantics + JSONInterceptor) + ObjectMapper mapper = buildAuthorizingMapper((path, instance) -> + "addressMap".equals(path) || "addressMap[0].city".equals(path)); + Person p = mapper.readValue( + "{\"addressMap\":{\"home\":{\"city\":\"Warsaw\",\"zip\":\"00-001\"}}}", Person.class); + assertNotNull(p.addressMap); + assertEquals(1, p.addressMap.size()); + assertNotNull(p.addressMap.get("home")); + assertEquals("Warsaw", p.addressMap.get("home").city); + assertNull("addressMap[0].zip must be skipped", p.addressMap.get("home").zip); + } + + public void testPathStackIsCleanAfterDeserialization() throws Exception { + ObjectMapper mapper = buildAuthorizingMapper((path, instance) -> true); + mapper.readValue("{\"name\":\"alice\",\"address\":{\"city\":\"Warsaw\"}}", Person.class); + assertTrue("ThreadLocal stack must be empty after deserialization", PATH_STACK.get().isEmpty()); + } +} From dc3c590821f86eae230744e5e5c80e45a5227796 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 4 May 2026 13:53:40 +0200 Subject: [PATCH 02/13] WW-5626 add ParameterAuthorizationContext for deserializer-level authorization Co-Authored-By: Claude Sonnet 4.6 --- .../ParameterAuthorizationContext.java | 123 ++++++++++++++++++ .../ParameterAuthorizationContextTest.java | 106 +++++++++++++++ 2 files changed, 229 insertions(+) create mode 100644 core/src/main/java/org/apache/struts2/interceptor/parameter/ParameterAuthorizationContext.java create mode 100644 core/src/test/java/org/apache/struts2/interceptor/parameter/ParameterAuthorizationContextTest.java diff --git a/core/src/main/java/org/apache/struts2/interceptor/parameter/ParameterAuthorizationContext.java b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParameterAuthorizationContext.java new file mode 100644 index 0000000000..67bae09c80 --- /dev/null +++ b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParameterAuthorizationContext.java @@ -0,0 +1,123 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.struts2.interceptor.parameter; + +import java.util.ArrayDeque; +import java.util.Deque; + +/** + * ThreadLocal holder for per-request parameter authorization state, used by deserializer-level + * authorization (e.g. the REST plugin's Jackson module). All state — the {@link ParameterAuthorizer}, + * the target, the action, and the current property-path stack — is bound by + * {@link org.apache.struts2.rest.ContentTypeInterceptor} (or other input-channel interceptors) + * before invoking the deserializer, and unbound in a {@code finally} block afterwards. + * + *

Implementations that consult this context (e.g. {@code AuthorizingSettableBeanProperty}) call + * {@link #isActive()} to decide whether to enforce authorization at all — when no context is bound + * (default config, {@code requireAnnotations=false}), they short-circuit to the delegate behavior.

+ * + * @since 7.2.0 + */ +public final class ParameterAuthorizationContext { + + private static final ThreadLocal STATE = new ThreadLocal<>(); + private static final ThreadLocal> PATH_STACK = new ThreadLocal<>(); + + private ParameterAuthorizationContext() { + // utility + } + + public static void bind(ParameterAuthorizer authorizer, Object target, Object action) { + STATE.set(new State(authorizer, target, action)); + } + + public static void unbind() { + STATE.remove(); + PATH_STACK.remove(); + } + + public static boolean isActive() { + return STATE.get() != null; + } + + /** + * Authorizes a parameter at the given path against the bound authorizer. Returns {@code true} + * when no context is bound — callers that don't want enforcement at all should not bind context + * in the first place; this default keeps wrapping deserializers safe for non-authorized requests. + */ + public static boolean isAuthorized(String parameterPath) { + State state = STATE.get(); + if (state == null) { + return true; + } + return state.authorizer.isAuthorized(parameterPath, state.target, state.action); + } + + public static void pushPath(String fullPath) { + pathStack().push(fullPath); + } + + public static void popPath() { + Deque stack = PATH_STACK.get(); + if (stack != null && !stack.isEmpty()) { + stack.pop(); + } + } + + /** + * @return the current top-of-stack path prefix, or empty string if none + */ + public static String currentPathPrefix() { + Deque stack = PATH_STACK.get(); + if (stack == null || stack.isEmpty()) { + return ""; + } + return stack.peek(); + } + + /** + * Builds the full path for a property at the current nesting level: {@code prefix.propertyName} + * (or just {@code propertyName} when at the root). + */ + public static String pathFor(String propertyName) { + String prefix = currentPathPrefix(); + return prefix.isEmpty() ? propertyName : prefix + "." + propertyName; + } + + private static Deque pathStack() { + Deque stack = PATH_STACK.get(); + if (stack == null) { + stack = new ArrayDeque<>(); + PATH_STACK.set(stack); + } + return stack; + } + + private static final class State { + final ParameterAuthorizer authorizer; + final Object target; + final Object action; + + State(ParameterAuthorizer authorizer, Object target, Object action) { + this.authorizer = authorizer; + this.target = target; + this.action = action; + } + } +} diff --git a/core/src/test/java/org/apache/struts2/interceptor/parameter/ParameterAuthorizationContextTest.java b/core/src/test/java/org/apache/struts2/interceptor/parameter/ParameterAuthorizationContextTest.java new file mode 100644 index 0000000000..40971287e9 --- /dev/null +++ b/core/src/test/java/org/apache/struts2/interceptor/parameter/ParameterAuthorizationContextTest.java @@ -0,0 +1,106 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.struts2.interceptor.parameter; + +import org.junit.After; +import org.junit.Test; + +import static org.assertj.core.api.Assertions.assertThat; + +public class ParameterAuthorizationContextTest { + + @After + public void tearDown() { + ParameterAuthorizationContext.unbind(); + } + + @Test + public void notActive_byDefault() { + assertThat(ParameterAuthorizationContext.isActive()).isFalse(); + } + + @Test + public void bind_thenActive() { + ParameterAuthorizer authorizer = (n, t, a) -> true; + Object action = new Object(); + ParameterAuthorizationContext.bind(authorizer, action, action); + assertThat(ParameterAuthorizationContext.isActive()).isTrue(); + } + + @Test + public void unbind_clearsState() { + ParameterAuthorizer authorizer = (n, t, a) -> true; + Object action = new Object(); + ParameterAuthorizationContext.bind(authorizer, action, action); + ParameterAuthorizationContext.unbind(); + assertThat(ParameterAuthorizationContext.isActive()).isFalse(); + } + + @Test + public void isAuthorized_delegatesToBoundAuthorizer() { + Object action = new Object(); + ParameterAuthorizationContext.bind((n, t, a) -> "name".equals(n), action, action); + assertThat(ParameterAuthorizationContext.isAuthorized("name")).isTrue(); + assertThat(ParameterAuthorizationContext.isAuthorized("role")).isFalse(); + } + + @Test + public void isAuthorized_returnsTrue_whenNotActive() { + // Defensive default: no context bound = no enforcement + assertThat(ParameterAuthorizationContext.isAuthorized("anything")).isTrue(); + } + + @Test + public void pathStack_emptyByDefault() { + assertThat(ParameterAuthorizationContext.currentPathPrefix()).isEmpty(); + } + + @Test + public void pushPath_buildsPrefix() { + ParameterAuthorizationContext.pushPath("address"); + assertThat(ParameterAuthorizationContext.currentPathPrefix()).isEqualTo("address"); + ParameterAuthorizationContext.pushPath("address.city"); + assertThat(ParameterAuthorizationContext.currentPathPrefix()).isEqualTo("address.city"); + } + + @Test + public void popPath_unwinds() { + ParameterAuthorizationContext.pushPath("address"); + ParameterAuthorizationContext.pushPath("address.city"); + ParameterAuthorizationContext.popPath(); + assertThat(ParameterAuthorizationContext.currentPathPrefix()).isEqualTo("address"); + ParameterAuthorizationContext.popPath(); + assertThat(ParameterAuthorizationContext.currentPathPrefix()).isEmpty(); + } + + @Test + public void pathFor_concatenatesPropertyName() { + assertThat(ParameterAuthorizationContext.pathFor("name")).isEqualTo("name"); + ParameterAuthorizationContext.pushPath("address"); + assertThat(ParameterAuthorizationContext.pathFor("city")).isEqualTo("address.city"); + } + + @Test + public void unbind_clearsPathStack() { + ParameterAuthorizationContext.bind((n, t, a) -> true, new Object(), new Object()); + ParameterAuthorizationContext.pushPath("address"); + ParameterAuthorizationContext.unbind(); + assertThat(ParameterAuthorizationContext.currentPathPrefix()).isEmpty(); + } +} From 0f2d28a973b8335d93c0ff986fb80f08fc67c7c8 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 4 May 2026 13:59:44 +0200 Subject: [PATCH 03/13] WW-5626 address review feedback on ParameterAuthorizationContext Co-Authored-By: Claude Sonnet 4.6 --- .../ParameterAuthorizationContext.java | 57 +++++++++++++------ .../ParameterAuthorizationContextTest.java | 23 ++++++++ 2 files changed, 62 insertions(+), 18 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/interceptor/parameter/ParameterAuthorizationContext.java b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParameterAuthorizationContext.java index 67bae09c80..bcd35ce32e 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/parameter/ParameterAuthorizationContext.java +++ b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParameterAuthorizationContext.java @@ -20,13 +20,14 @@ import java.util.ArrayDeque; import java.util.Deque; +import java.util.Objects; /** * ThreadLocal holder for per-request parameter authorization state, used by deserializer-level - * authorization (e.g. the REST plugin's Jackson module). All state — the {@link ParameterAuthorizer}, - * the target, the action, and the current property-path stack — is bound by - * {@link org.apache.struts2.rest.ContentTypeInterceptor} (or other input-channel interceptors) - * before invoking the deserializer, and unbound in a {@code finally} block afterwards. + * authorization (e.g. the REST plugin's {@code ContentTypeInterceptor}). All state — the + * {@link ParameterAuthorizer}, the target, the action, and the current property-path stack — is + * bound by input-channel interceptors before invoking the deserializer, and unbound in a + * {@code finally} block afterwards. * *

Implementations that consult this context (e.g. {@code AuthorizingSettableBeanProperty}) call * {@link #isActive()} to decide whether to enforce authorization at all — when no context is bound @@ -37,21 +38,40 @@ public final class ParameterAuthorizationContext { private static final ThreadLocal STATE = new ThreadLocal<>(); - private static final ThreadLocal> PATH_STACK = new ThreadLocal<>(); + private static final ThreadLocal> PATH_STACK = ThreadLocal.withInitial(ArrayDeque::new); private ParameterAuthorizationContext() { // utility } + /** + * Binds an authorizer, target, and action to the current thread. {@code target} is the object + * being populated — typically the action itself, or the model object for {@code ModelDriven} + * actions (the same contract as {@link ParameterAuthorizer#isAuthorized}). {@code action} is + * always the action instance. A subsequent call without an intervening {@link #unbind()} replaces + * the prior state without resetting the path stack. + * + * @param authorizer the authorizer to use for this request; must not be {@code null} + * @param target the object being populated (action or model) + * @param action the action instance + */ public static void bind(ParameterAuthorizer authorizer, Object target, Object action) { + Objects.requireNonNull(authorizer, "authorizer"); STATE.set(new State(authorizer, target, action)); } + /** + * Removes the bound authorizer state and clears the path stack for the current thread. + * Safe to call even when no context has been bound. + */ public static void unbind() { STATE.remove(); PATH_STACK.remove(); } + /** + * Returns {@code true} if an authorizer has been bound on the current thread via {@link #bind}. + */ public static boolean isActive() { return STATE.get() != null; } @@ -69,13 +89,23 @@ public static boolean isAuthorized(String parameterPath) { return state.authorizer.isAuthorized(parameterPath, state.target, state.action); } - public static void pushPath(String fullPath) { - pathStack().push(fullPath); + /** + * Pushes the full cumulative path prefix onto the stack. Subsequent {@link #pathFor(String)} + * calls will append {@code name} to this prefix. Callers building a collection-element prefix + * (e.g. {@code items[0]}) must pass the full string including the suffix. + * + * @param cumulativePath the full path prefix to push (e.g. {@code "address"} or {@code "items[0]"}) + */ + public static void pushPath(String cumulativePath) { + PATH_STACK.get().push(cumulativePath); } + /** + * Pops the top path prefix from the stack. Has no effect if the stack is empty. + */ public static void popPath() { Deque stack = PATH_STACK.get(); - if (stack != null && !stack.isEmpty()) { + if (!stack.isEmpty()) { stack.pop(); } } @@ -85,7 +115,7 @@ public static void popPath() { */ public static String currentPathPrefix() { Deque stack = PATH_STACK.get(); - if (stack == null || stack.isEmpty()) { + if (stack.isEmpty()) { return ""; } return stack.peek(); @@ -100,15 +130,6 @@ public static String pathFor(String propertyName) { return prefix.isEmpty() ? propertyName : prefix + "." + propertyName; } - private static Deque pathStack() { - Deque stack = PATH_STACK.get(); - if (stack == null) { - stack = new ArrayDeque<>(); - PATH_STACK.set(stack); - } - return stack; - } - private static final class State { final ParameterAuthorizer authorizer; final Object target; diff --git a/core/src/test/java/org/apache/struts2/interceptor/parameter/ParameterAuthorizationContextTest.java b/core/src/test/java/org/apache/struts2/interceptor/parameter/ParameterAuthorizationContextTest.java index 40971287e9..76e4a3466b 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/parameter/ParameterAuthorizationContextTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/parameter/ParameterAuthorizationContextTest.java @@ -103,4 +103,27 @@ public void unbind_clearsPathStack() { ParameterAuthorizationContext.unbind(); assertThat(ParameterAuthorizationContext.currentPathPrefix()).isEmpty(); } + + @Test + public void bind_replacesPriorState_doesNotResetPathStack() { + Object firstAction = new Object(); + Object secondAction = new Object(); + ParameterAuthorizationContext.bind((n, t, a) -> "first".equals(n), firstAction, firstAction); + ParameterAuthorizationContext.pushPath("address"); + // Rebind with a different authorizer + ParameterAuthorizationContext.bind((n, t, a) -> "second".equals(n), secondAction, secondAction); + // New authorizer in effect + assertThat(ParameterAuthorizationContext.isAuthorized("first")).isFalse(); + assertThat(ParameterAuthorizationContext.isAuthorized("second")).isTrue(); + // Path stack is preserved across rebind (it's a separate ThreadLocal) + assertThat(ParameterAuthorizationContext.currentPathPrefix()).isEqualTo("address"); + } + + @Test + public void unbind_whenNeverBound_isSafeNoOp() { + // Should not throw; isActive should remain false + ParameterAuthorizationContext.unbind(); + assertThat(ParameterAuthorizationContext.isActive()).isFalse(); + assertThat(ParameterAuthorizationContext.currentPathPrefix()).isEmpty(); + } } From 303a3ea0d565b811829db9f6a19f86a2e5165ed4 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 4 May 2026 14:00:44 +0200 Subject: [PATCH 04/13] WW-5626 add AuthorizationAwareContentTypeHandler marker interface --- .../AuthorizationAwareContentTypeHandler.java | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 plugins/rest/src/main/java/org/apache/struts2/rest/handler/AuthorizationAwareContentTypeHandler.java diff --git a/plugins/rest/src/main/java/org/apache/struts2/rest/handler/AuthorizationAwareContentTypeHandler.java b/plugins/rest/src/main/java/org/apache/struts2/rest/handler/AuthorizationAwareContentTypeHandler.java new file mode 100644 index 0000000000..0e01c507ed --- /dev/null +++ b/plugins/rest/src/main/java/org/apache/struts2/rest/handler/AuthorizationAwareContentTypeHandler.java @@ -0,0 +1,46 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.struts2.rest.handler; + +/** + * Marker interface for {@link ContentTypeHandler} implementations that respect the + * {@code ParameterAuthorizationContext} ThreadLocal during deserialization, enforcing + * {@code @StrutsParameter} authorization per-property. + * + *

When {@code struts.parameters.requireAnnotations=true}, the REST plugin's + * {@code ContentTypeInterceptor} binds the authorization context before invoking handlers that + * implement this interface, allowing them to filter unauthorized properties during deserialization + * (rather than after, via reflection-based copying).

+ * + *

Handlers that do NOT implement this interface fall back to the legacy two-phase copy in + * {@code ContentTypeInterceptor} — correct but more expensive (and requires a no-arg constructor + * on the target).

+ * + *

Implementer responsibility: A handler that declares this interface MUST register + * the authorization-aware mechanism on its underlying parser (e.g. for Jackson, register + * {@code ParameterAuthorizingModule} on the {@code ObjectMapper}). If the handler implements the + * interface but its parser does not honor the context, authorization will silently do nothing — + * a serious security bug. The marker interface is the contract; implementations must uphold it.

+ * + * @since 7.2.0 + */ +public interface AuthorizationAwareContentTypeHandler extends ContentTypeHandler { + // Marker interface — no methods. Implementations signal that their toObject() method + // honors ParameterAuthorizationContext for per-property @StrutsParameter enforcement. +} From ee2646574d040dcde62228111a3c86072050c47d Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 4 May 2026 15:58:26 +0200 Subject: [PATCH 05/13] WW-5626 add AuthorizingSettableBeanProperty for Jackson per-property authorization --- .../AuthorizingSettableBeanProperty.java | 114 ++++++++++++++++++ 1 file changed, 114 insertions(+) create mode 100644 plugins/rest/src/main/java/org/apache/struts2/rest/handler/jackson/AuthorizingSettableBeanProperty.java 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 new file mode 100644 index 0000000000..d6c3a812aa --- /dev/null +++ b/plugins/rest/src/main/java/org/apache/struts2/rest/handler/jackson/AuthorizingSettableBeanProperty.java @@ -0,0 +1,114 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +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.deser.SettableBeanProperty; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; +import org.apache.struts2.interceptor.parameter.ParameterAuthorizationContext; + +import java.io.IOException; + +/** + * A {@link SettableBeanProperty.Delegating} that authorizes each property against the + * {@link ParameterAuthorizationContext} before delegating to the underlying property's + * {@code deserializeAndSet}. Unauthorized properties are silently dropped — the JSON value is + * skipped via {@link JsonParser#skipChildren()}, so any nested object graph is never instantiated + * and setter side effects on unauthorized properties never fire. + * + *

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.

+ * + *

When {@link ParameterAuthorizationContext#isActive()} is {@code false}, this wrapper is a + * straight pass-through to the delegate — no overhead for default-config requests.

+ * + * @since 7.2.0 + */ +public class AuthorizingSettableBeanProperty extends SettableBeanProperty.Delegating { + + private static final Logger LOG = LogManager.getLogger(AuthorizingSettableBeanProperty.class); + + public AuthorizingSettableBeanProperty(SettableBeanProperty delegate) { + super(delegate); + } + + @Override + protected SettableBeanProperty withDelegate(SettableBeanProperty d) { + return new AuthorizingSettableBeanProperty(d); + } + + @Override + public void deserializeAndSet(JsonParser p, DeserializationContext ctxt, Object instance) throws IOException { + if (!ParameterAuthorizationContext.isActive()) { + delegate.deserializeAndSet(p, ctxt, instance); + return; + } + String path = ParameterAuthorizationContext.pathFor(getName()); + if (!ParameterAuthorizationContext.isAuthorized(path)) { + LOG.warn("REST body parameter [{}] rejected by @StrutsParameter authorization on [{}]", + path, instance.getClass().getName()); + p.skipChildren(); + return; + } + ParameterAuthorizationContext.pushPath(prefixForNested(path)); + try { + delegate.deserializeAndSet(p, ctxt, instance); + } finally { + ParameterAuthorizationContext.popPath(); + } + } + + @Override + public Object deserializeSetAndReturn(JsonParser p, DeserializationContext ctxt, Object instance) throws IOException { + if (!ParameterAuthorizationContext.isActive()) { + return delegate.deserializeSetAndReturn(p, ctxt, instance); + } + String path = ParameterAuthorizationContext.pathFor(getName()); + if (!ParameterAuthorizationContext.isAuthorized(path)) { + LOG.warn("REST body parameter [{}] rejected by @StrutsParameter authorization on [{}]", + path, instance.getClass().getName()); + p.skipChildren(); + return instance; + } + ParameterAuthorizationContext.pushPath(prefixForNested(path)); + try { + return delegate.deserializeSetAndReturn(p, ctxt, instance); + } finally { + ParameterAuthorizationContext.popPath(); + } + } + + /** + * 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. + */ + private String prefixForNested(String pathOfThisProperty) { + JavaType type = getType(); + if (type != null && (type.isCollectionLikeType() || type.isMapLikeType() || type.isArrayType())) { + return pathOfThisProperty + "[0]"; + } + return pathOfThisProperty; + } +} From 1c3c8b70cc8bdbdd52cb71eca9b1fb5d1a20600f Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 4 May 2026 15:59:34 +0200 Subject: [PATCH 06/13] WW-5626 add ParameterAuthorizingModule installing the property wrapper on Jackson mappers --- .../jackson/ParameterAuthorizingModule.java | 64 +++++++++ .../ParameterAuthorizingModuleTest.java | 124 ++++++++++++++++++ 2 files changed, 188 insertions(+) create mode 100644 plugins/rest/src/main/java/org/apache/struts2/rest/handler/jackson/ParameterAuthorizingModule.java create mode 100644 plugins/rest/src/test/java/org/apache/struts2/rest/handler/jackson/ParameterAuthorizingModuleTest.java diff --git a/plugins/rest/src/main/java/org/apache/struts2/rest/handler/jackson/ParameterAuthorizingModule.java b/plugins/rest/src/main/java/org/apache/struts2/rest/handler/jackson/ParameterAuthorizingModule.java new file mode 100644 index 0000000000..c900164151 --- /dev/null +++ b/plugins/rest/src/main/java/org/apache/struts2/rest/handler/jackson/ParameterAuthorizingModule.java @@ -0,0 +1,64 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.struts2.rest.handler.jackson; + +import com.fasterxml.jackson.databind.BeanDescription; +import com.fasterxml.jackson.databind.DeserializationConfig; +import com.fasterxml.jackson.databind.deser.BeanDeserializerBuilder; +import com.fasterxml.jackson.databind.deser.BeanDeserializerModifier; +import com.fasterxml.jackson.databind.deser.SettableBeanProperty; +import com.fasterxml.jackson.databind.module.SimpleModule; + +import java.util.Iterator; + +/** + * Jackson {@link SimpleModule} that wraps every {@link SettableBeanProperty} on every bean type + * with an {@link AuthorizingSettableBeanProperty}, enforcing {@code @StrutsParameter} authorization + * during deserialization via the {@link org.apache.struts2.interceptor.parameter.ParameterAuthorizationContext} + * ThreadLocal. + * + *

Register this module once on each handler's mapper (e.g. in the constructor). All per-request + * authorization state is read from the ThreadLocal context, so the module + mapper combination is + * thread-safe and reusable across requests.

+ * + * @since 7.2.0 + */ +public class ParameterAuthorizingModule extends SimpleModule { + + private static final long serialVersionUID = 1L; + + public ParameterAuthorizingModule() { + setDeserializerModifier(new BeanDeserializerModifier() { + @Override + public BeanDeserializerBuilder updateBuilder(DeserializationConfig config, + BeanDescription beanDesc, + BeanDeserializerBuilder builder) { + Iterator it = builder.getProperties(); + while (it.hasNext()) { + SettableBeanProperty original = it.next(); + if (original instanceof AuthorizingSettableBeanProperty) { + continue; // idempotent; protect against double-registration + } + builder.addOrReplaceProperty(new AuthorizingSettableBeanProperty(original), true); + } + return builder; + } + }); + } +} 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 new file mode 100644 index 0000000000..fc3421b927 --- /dev/null +++ b/plugins/rest/src/test/java/org/apache/struts2/rest/handler/jackson/ParameterAuthorizingModuleTest.java @@ -0,0 +1,124 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.struts2.rest.handler.jackson; + +import com.fasterxml.jackson.databind.ObjectMapper; +import junit.framework.TestCase; +import org.apache.struts2.interceptor.parameter.ParameterAuthorizationContext; +import org.apache.struts2.interceptor.parameter.ParameterAuthorizer; + +public class ParameterAuthorizingModuleTest extends TestCase { + + private ObjectMapper mapper; + + @Override + protected void setUp() { + mapper = new ObjectMapper().registerModule(new ParameterAuthorizingModule()); + } + + @Override + protected void tearDown() { + ParameterAuthorizationContext.unbind(); + } + + private void bind(ParameterAuthorizer authorizer, Object instance) { + ParameterAuthorizationContext.bind(authorizer, instance, instance); + } + + public void testNoContext_passThrough() throws Exception { + // No bind → wrapper is a no-op + Person p = mapper.readValue("{\"name\":\"alice\",\"role\":\"admin\"}", Person.class); + assertEquals("alice", p.name); + assertEquals("admin", p.role); + } + + public void testTopLevelAuthorized() throws Exception { + bind((path, t, a) -> "name".equals(path), new Person()); + Person result = mapper.readValue("{\"name\":\"alice\",\"role\":\"admin\"}", Person.class); + assertEquals("alice", result.name); + assertNull(result.role); + } + + public void testNestedPropertyAuthorizedByPath() throws Exception { + bind((path, t, a) -> "address".equals(path) || "address.city".equals(path), new Person()); + Person result = mapper.readValue( + "{\"address\":{\"city\":\"Warsaw\",\"zip\":\"00-001\"}}", Person.class); + assertNotNull(result.address); + assertEquals("Warsaw", result.address.city); + assertNull(result.address.zip); + } + + public void testNestedRejectedAtParent() throws Exception { + bind((path, t, a) -> "name".equals(path), new Person()); + Person result = mapper.readValue( + "{\"name\":\"alice\",\"address\":{\"city\":\"Warsaw\"}}", Person.class); + assertEquals("alice", result.name); + assertNull(result.address); + } + + public void testListUsesIndexedPath() throws Exception { + bind((path, t, a) -> "addresses".equals(path) || "addresses[0].city".equals(path), new Person()); + Person result = mapper.readValue( + "{\"addresses\":[{\"city\":\"Warsaw\",\"zip\":\"00-001\"}]}", Person.class); + assertEquals(1, result.addresses.size()); + assertEquals("Warsaw", result.addresses.get(0).city); + assertNull(result.addresses.get(0).zip); + } + + public void testArrayUsesIndexedPath() throws Exception { + bind((path, t, a) -> "addressArray".equals(path) || "addressArray[0].city".equals(path), new Person()); + Person result = mapper.readValue( + "{\"addressArray\":[{\"city\":\"Warsaw\",\"zip\":\"00-001\"}]}", Person.class); + assertEquals(1, result.addressArray.length); + assertEquals("Warsaw", result.addressArray[0].city); + assertNull(result.addressArray[0].zip); + } + + public void testMapUsesIndexedPath() throws Exception { + bind((path, t, a) -> "addressMap".equals(path) || "addressMap[0].city".equals(path), new Person()); + Person result = mapper.readValue( + "{\"addressMap\":{\"home\":{\"city\":\"Warsaw\",\"zip\":\"00-001\"}}}", Person.class); + assertNotNull(result.addressMap.get("home")); + assertEquals("Warsaw", result.addressMap.get("home").city); + assertNull(result.addressMap.get("home").zip); + } + + public void testPathStackCleanAfterDeserialization() throws Exception { + bind((path, t, a) -> true, new Person()); + mapper.readValue("{\"name\":\"alice\",\"address\":{\"city\":\"Warsaw\"}}", Person.class); + assertEquals("path stack must be empty after deserialization", "", + ParameterAuthorizationContext.currentPathPrefix()); + } + + // --- Fixtures --- + + public static class Person { + public String name; + public String role; + public Address address; + public java.util.List
addresses; + public Address[] addressArray; + public java.util.Map addressMap; + } + + public static class Address { + public String city; + public String zip; + } +} From f88c7301782bb40679072b4a54210baba9664d46 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 4 May 2026 16:00:24 +0200 Subject: [PATCH 07/13] WW-5626 register ParameterAuthorizingModule on default Jackson REST handlers --- .../apache/struts2/rest/handler/JacksonJsonHandler.java | 5 +++-- .../apache/struts2/rest/handler/JacksonXmlHandler.java | 9 +++++++-- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/plugins/rest/src/main/java/org/apache/struts2/rest/handler/JacksonJsonHandler.java b/plugins/rest/src/main/java/org/apache/struts2/rest/handler/JacksonJsonHandler.java index d97af08e38..834661cd5d 100644 --- a/plugins/rest/src/main/java/org/apache/struts2/rest/handler/JacksonJsonHandler.java +++ b/plugins/rest/src/main/java/org/apache/struts2/rest/handler/JacksonJsonHandler.java @@ -32,11 +32,12 @@ /** * Handles JSON content using jackson-lib */ -public class JacksonJsonHandler implements ContentTypeHandler { +public class JacksonJsonHandler implements AuthorizationAwareContentTypeHandler { private static final String DEFAULT_CONTENT_TYPE = "application/json"; private String defaultEncoding = "ISO-8859-1"; - private ObjectMapper mapper = new ObjectMapper(); + private ObjectMapper mapper = new ObjectMapper() + .registerModule(new org.apache.struts2.rest.handler.jackson.ParameterAuthorizingModule()); @Override public void toObject(ActionInvocation invocation, Reader in, Object target) throws IOException { diff --git a/plugins/rest/src/main/java/org/apache/struts2/rest/handler/JacksonXmlHandler.java b/plugins/rest/src/main/java/org/apache/struts2/rest/handler/JacksonXmlHandler.java index a73ad4d212..ccc102023e 100644 --- a/plugins/rest/src/main/java/org/apache/struts2/rest/handler/JacksonXmlHandler.java +++ b/plugins/rest/src/main/java/org/apache/struts2/rest/handler/JacksonXmlHandler.java @@ -31,12 +31,17 @@ /** * Handles XML content using Jackson */ -public class JacksonXmlHandler implements ContentTypeHandler { +public class JacksonXmlHandler implements AuthorizationAwareContentTypeHandler { private static final Logger LOG = LogManager.getLogger(JacksonXmlHandler.class); private static final String DEFAULT_CONTENT_TYPE = "application/xml"; - private final XmlMapper mapper = new XmlMapper(); + private final XmlMapper mapper; + + public JacksonXmlHandler() { + mapper = new XmlMapper(); + mapper.registerModule(new org.apache.struts2.rest.handler.jackson.ParameterAuthorizingModule()); + } @Override public void toObject(ActionInvocation invocation, Reader in, Object target) throws IOException { From be5ba541315bac46ef4c2dd6447899d071d7cea3 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 4 May 2026 16:03:26 +0200 Subject: [PATCH 08/13] WW-5626 use AuthorizationAwareContentTypeHandler path when handler supports it --- .../struts2/rest/ContentTypeInterceptor.java | 37 +++++++++++++------ 1 file changed, 26 insertions(+), 11 deletions(-) diff --git a/plugins/rest/src/main/java/org/apache/struts2/rest/ContentTypeInterceptor.java b/plugins/rest/src/main/java/org/apache/struts2/rest/ContentTypeInterceptor.java index 73f3cd7ef3..35016467e3 100644 --- a/plugins/rest/src/main/java/org/apache/struts2/rest/ContentTypeInterceptor.java +++ b/plugins/rest/src/main/java/org/apache/struts2/rest/ContentTypeInterceptor.java @@ -96,18 +96,33 @@ public String intercept(ActionInvocation invocation) throws Exception { InputStreamReader reader = encoding == null ? new InputStreamReader(is) : new InputStreamReader(is, encoding); if (requireAnnotations) { - // Two-phase deserialization: deserialize into a fresh instance, then copy only authorized properties. - // Requires a public no-arg constructor on the target class. - // If absent, body processing is rejected entirely — a best-effort scrub cannot guarantee - // that every nested unauthorized property is nulled out, so the safer choice is to skip. - Object freshInstance = createFreshInstance(target.getClass()); - if (freshInstance != null) { - handler.toObject(invocation, reader, freshInstance); - copyAuthorizedProperties(freshInstance, target, invocation.getAction(), target, ""); + if (handler instanceof org.apache.struts2.rest.handler.AuthorizationAwareContentTypeHandler) { + // Handler authorizes per-property during deserialization (no two-phase copy needed). + // Bind context so the handler's deserializer can consult ParameterAuthorizationContext. + Object action = invocation.getAction(); + Object resolvedTarget = parameterAuthorizer.resolveTarget(action); + org.apache.struts2.interceptor.parameter.ParameterAuthorizationContext.bind( + parameterAuthorizer, resolvedTarget, action); + try { + handler.toObject(invocation, reader, target); + } finally { + org.apache.struts2.interceptor.parameter.ParameterAuthorizationContext.unbind(); + } } else { - LOG.warn("REST body rejected: requireAnnotations=true but [{}] has no no-arg constructor; " - + "body deserialization skipped to preserve @StrutsParameter authorization integrity", - target.getClass().getName()); + // Legacy two-phase deserialization for handlers that don't authorize themselves. + // Deserialize into a fresh instance, then copy only authorized properties. + // Requires a public no-arg constructor on the target class. + // If absent, body processing is rejected entirely — a best-effort scrub cannot guarantee + // that every nested unauthorized property is nulled out, so the safer choice is to skip. + Object freshInstance = createFreshInstance(target.getClass()); + if (freshInstance != null) { + handler.toObject(invocation, reader, freshInstance); + copyAuthorizedProperties(freshInstance, target, invocation.getAction(), target, ""); + } else { + LOG.warn("REST body rejected: requireAnnotations=true but [{}] has no no-arg constructor; " + + "body deserialization skipped to preserve @StrutsParameter authorization integrity", + target.getClass().getName()); + } } } else { // Direct deserialization (backward compat when requireAnnotations is not enabled) From 04f78e287acd72e318f1dc53b8cd90a7cba0e6c8 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 4 May 2026 16:05:25 +0200 Subject: [PATCH 09/13] WW-5626 add integration tests proving the new Jackson authorization path is used --- ...ContentTypeInterceptorIntegrationTest.java | 91 +++++++++++++++++-- 1 file changed, 81 insertions(+), 10 deletions(-) diff --git a/plugins/rest/src/test/java/org/apache/struts2/rest/ContentTypeInterceptorIntegrationTest.java b/plugins/rest/src/test/java/org/apache/struts2/rest/ContentTypeInterceptorIntegrationTest.java index dc11c6fdfc..dc4812c2a3 100644 --- a/plugins/rest/src/test/java/org/apache/struts2/rest/ContentTypeInterceptorIntegrationTest.java +++ b/plugins/rest/src/test/java/org/apache/struts2/rest/ContentTypeInterceptorIntegrationTest.java @@ -23,8 +23,10 @@ import junit.framework.TestCase; import org.apache.struts2.ActionContext; import org.apache.struts2.ActionInvocation; +import org.apache.struts2.ActionSupport; import org.apache.struts2.action.Action; import org.apache.struts2.dispatcher.mapper.ActionMapping; +import org.apache.struts2.interceptor.parameter.StrutsParameter; import org.apache.struts2.interceptor.parameter.StrutsParameterAuthorizer; import org.apache.struts2.ognl.DefaultOgnlBeanInfoCacheFactory; import org.apache.struts2.ognl.DefaultOgnlExpressionCacheFactory; @@ -53,7 +55,10 @@ public class ContentTypeInterceptorIntegrationTest extends TestCase { protected void setUp() throws Exception { super.setUp(); action = new SecureRestAction(); + setupInterceptorWithAction(action); + } + private void setupInterceptorWithAction(Object actionInstance) { var ognlUtil = new OgnlUtil( new DefaultOgnlExpressionCacheFactory<>("1000", LRU.toString()), new DefaultOgnlBeanInfoCacheFactory<>("1000", LRU.toString()), @@ -72,8 +77,8 @@ protected void setUp() throws Exception { mockActionInvocation = new Mock(ActionInvocation.class); mockSelector = new Mock(ContentTypeHandlerManager.class); // ContentTypeInterceptor calls getAction() twice when requireAnnotations=true - mockActionInvocation.expectAndReturn("getAction", action); - mockActionInvocation.expectAndReturn("getAction", action); + mockActionInvocation.expectAndReturn("getAction", actionInstance); + mockActionInvocation.expectAndReturn("getAction", actionInstance); mockActionInvocation.expectAndReturn("invoke", Action.SUCCESS); mockSelector.expectAndReturn("getHandlerForRequest", new AnyConstraintMatcher() { public boolean matches(Object[] args) { return true; } @@ -120,15 +125,81 @@ public void testNestedPropertyAuthorizedWhenDepthAllows() throws Exception { } public void testNestedPropertyRejectedWhenDepthInsufficient() throws Exception { - // shallowAddress has @StrutsParameter (depth=0) — its nested fields should NOT be set + // shallowAddress has @StrutsParameter on the setter (depth-0 authorized) but the getter + // has no depth>=1 annotation. The Jackson path enters shallowAddress (constructed by + // Jackson) but skipChildren on each inner property — so the Address is non-null but its + // city/zip fields stay null. runWithBody("{\"shallowAddress\":{\"city\":\"Warsaw\",\"zip\":\"00-001\"}}"); - // The top-level shallowAddress reference itself may be created (Jackson behavior) - // but its nested city/zip must remain null because depth-1 isn't allowed. - if (action.getShallowAddress() != null) { - assertNull("nested city should be rejected (depth insufficient)", - action.getShallowAddress().getCity()); - assertNull("nested zip should be rejected (depth insufficient)", - action.getShallowAddress().getZip()); + assertNotNull("shallowAddress is depth-0 authorized; Jackson constructs it", + action.getShallowAddress()); + assertNull("nested city must be rejected (depth-1 not authorized)", + action.getShallowAddress().getCity()); + assertNull("nested zip must be rejected (depth-1 not authorized)", + action.getShallowAddress().getZip()); + } + + // --- Tests proving the new Jackson authorization path is in use --- + + public void testJacksonHandlerDoesNotRequireNoArgConstructor() throws Exception { + // The legacy two-phase copy required a no-arg constructor on the target. Jackson's + // readerForUpdating populates the existing instance directly, so this constraint + // is gone — proof that the new AuthorizationAware path is being taken. + NoNoArgAction noNoArg = new NoNoArgAction("preserved-pre-deserialization-value"); + setupInterceptorWithAction(noNoArg); + runWithBody("{\"name\":\"alice\"}"); + assertEquals("alice", noNoArg.getName()); + assertEquals("pre-existing field must be preserved (no fresh-instance copy)", + "preserved-pre-deserialization-value", noNoArg.getRequiredField()); + } + + public void testRejectedAtParentNeverInstantiatesNestedObject() throws Exception { + // Stronger guarantee than the two-phase copy: when the parent property is rejected, + // Jackson's skipChildren() discards the entire JSON subtree and the nested object + // is never constructed. role-typed fixture: address requires a setter @StrutsParameter + // for depth-0 authorization. By giving address a fresh action where address is depth-0 + // unauthorized, we prove the setter is never called and address stays null. + // (We use a custom action where address has no setter annotation.) + UnauthorizedNestedAction restrictedAction = new UnauthorizedNestedAction(); + setupInterceptorWithAction(restrictedAction); + runWithBody("{\"unauthorized\":{\"city\":\"Warsaw\"}}"); + assertNull("unauthorized property must be rejected at parent — Jackson never enters", + restrictedAction.getUnauthorized()); + } + + // --- Test fixtures for new path verification --- + + /** + * Action with no public no-arg constructor — would fail the legacy two-phase copy's + * createFreshInstance check, but works fine with the Jackson authorization path. + */ + public static class NoNoArgAction extends ActionSupport { + private final String requiredField; + private String name; + + public NoNoArgAction(String requiredField) { + this.requiredField = requiredField; + } + + public String getName() { return name; } + + @StrutsParameter + public void setName(String name) { this.name = name; } + + public String getRequiredField() { return requiredField; } + } + + /** + * Action with a property that has NO @StrutsParameter on its setter — depth-0 authorization + * fails, so Jackson must never enter this property nor instantiate the nested object. + */ + public static class UnauthorizedNestedAction extends ActionSupport { + private SecureRestAction.Address unauthorized; + + public SecureRestAction.Address getUnauthorized() { return unauthorized; } + + // No @StrutsParameter annotation — depth-0 path "unauthorized" is rejected. + public void setUnauthorized(SecureRestAction.Address unauthorized) { + this.unauthorized = unauthorized; } } } From 32fa1fdeb730882102ab28ce159cadd53ed05726 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 4 May 2026 16:06:05 +0200 Subject: [PATCH 10/13] WW-5626 deprecate XStreamHandler in favor of JacksonXmlHandler --- .../apache/struts2/rest/handler/XStreamHandler.java | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/plugins/rest/src/main/java/org/apache/struts2/rest/handler/XStreamHandler.java b/plugins/rest/src/main/java/org/apache/struts2/rest/handler/XStreamHandler.java index 40a042a9c7..f0cf191ca4 100644 --- a/plugins/rest/src/main/java/org/apache/struts2/rest/handler/XStreamHandler.java +++ b/plugins/rest/src/main/java/org/apache/struts2/rest/handler/XStreamHandler.java @@ -42,8 +42,17 @@ import java.util.Set; /** - * Handles XML content + * Handles XML content via the XStream library. + * + * @deprecated since 7.2.0, scheduled for removal in a future major version. XStream has a long + * history of deserialization vulnerabilities and requires per-class allowlist + * maintenance. The default {@code xml} binding in {@code struts-plugin.xml} uses + * {@link JacksonXmlHandler}, which respects {@code @StrutsParameter} authorization + * via the {@link AuthorizationAwareContentTypeHandler} mechanism. Users who have + * explicitly overridden the {@code xml} handler to {@code XStreamHandler} should + * migrate to {@link JacksonXmlHandler}. */ +@Deprecated(since = "7.2.0", forRemoval = true) public class XStreamHandler implements ContentTypeHandler { private static final Logger LOG = LogManager.getLogger(XStreamHandler.class); From 0166c6a0b5356f9c5ded8a985d5dd75a7fc29018 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 4 May 2026 16:06:28 +0200 Subject: [PATCH 11/13] WW-5626 remove Jackson auth spike; replaced by production tests --- .../rest/spike/JacksonAuthSpikeTest.java | 257 ------------------ 1 file changed, 257 deletions(-) delete mode 100644 plugins/rest/src/test/java/org/apache/struts2/rest/spike/JacksonAuthSpikeTest.java diff --git a/plugins/rest/src/test/java/org/apache/struts2/rest/spike/JacksonAuthSpikeTest.java b/plugins/rest/src/test/java/org/apache/struts2/rest/spike/JacksonAuthSpikeTest.java deleted file mode 100644 index 204f87bbb5..0000000000 --- a/plugins/rest/src/test/java/org/apache/struts2/rest/spike/JacksonAuthSpikeTest.java +++ /dev/null @@ -1,257 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ -package org.apache.struts2.rest.spike; - -import com.fasterxml.jackson.core.JsonParser; -import com.fasterxml.jackson.databind.BeanDescription; -import com.fasterxml.jackson.databind.DeserializationConfig; -import com.fasterxml.jackson.databind.DeserializationContext; -import com.fasterxml.jackson.databind.ObjectMapper; -import com.fasterxml.jackson.databind.deser.BeanDeserializerBuilder; -import com.fasterxml.jackson.databind.deser.BeanDeserializerModifier; -import com.fasterxml.jackson.databind.deser.SettableBeanProperty; -import com.fasterxml.jackson.databind.module.SimpleModule; -import junit.framework.TestCase; - -import java.util.ArrayDeque; -import java.util.Deque; -import java.util.Iterator; -import java.util.function.BiPredicate; - -/** - * SPIKE — NOT PRODUCTION CODE. - * - * Validates that Jackson's BeanDeserializerModifier + SettableBeanProperty wrapping can - * intercept per-property deserialization for Approach C (handler-level @StrutsParameter - * authorization). Path tracking uses a ThreadLocal Deque pushed/popped around each - * authorized property's deserialization. - * - * Three claims under test: - * (1) updateBuilder() can replace SettableBeanProperty instances on the builder - * (2) A wrapping property can call parser.skipChildren() to discard unauthorized values - * (3) Path tracking via ThreadLocal Deque produces dot/bracket paths matching - * ParametersInterceptor depth semantics for nested objects - * - * If green, this approach is viable for production. - */ -public class JacksonAuthSpikeTest extends TestCase { - - // --- ThreadLocal path tracking --- - - private static final ThreadLocal> PATH_STACK = ThreadLocal.withInitial(ArrayDeque::new); - - private static String currentPath(String propertyName) { - Deque stack = PATH_STACK.get(); - if (stack.isEmpty()) { - return propertyName; - } - // Build path: stack contains parent prefix(es) bottom-to-top - StringBuilder sb = new StringBuilder(); - Iterator it = stack.descendingIterator(); - while (it.hasNext()) { - sb.append(it.next()); - sb.append('.'); - } - sb.append(propertyName); - return sb.toString(); - } - - // --- Wrapping SettableBeanProperty --- - - static class AuthorizingSettableBeanProperty extends SettableBeanProperty.Delegating { - private final BiPredicate authorizer; - - AuthorizingSettableBeanProperty(SettableBeanProperty delegate, BiPredicate authorizer) { - super(delegate); - this.authorizer = authorizer; - } - - @Override - protected SettableBeanProperty withDelegate(SettableBeanProperty d) { - return new AuthorizingSettableBeanProperty(d, authorizer); - } - - /** - * For Collection/Map/Array properties, the path to push for nested element members must include - * the indexed bracket suffix so children build paths like "items[0].field" — matching - * ParametersInterceptor depth semantics and the existing JSONInterceptor recursive filter. - * For scalar/bean properties, push the path unchanged. - */ - private String prefixForNested(String pathOfThisProperty) { - com.fasterxml.jackson.databind.JavaType type = getType(); - if (type != null && (type.isCollectionLikeType() || type.isMapLikeType() || type.isArrayType())) { - return pathOfThisProperty + "[0]"; - } - return pathOfThisProperty; - } - - @Override - public void deserializeAndSet(JsonParser p, DeserializationContext ctxt, Object instance) throws java.io.IOException { - String path = currentPath(getName()); - if (!authorizer.test(path, instance)) { - p.skipChildren(); // discard the JSON value for this property - return; - } - PATH_STACK.get().push(prefixForNested(path)); - try { - delegate.deserializeAndSet(p, ctxt, instance); - } finally { - PATH_STACK.get().pop(); - } - } - - @Override - public Object deserializeSetAndReturn(JsonParser p, DeserializationContext ctxt, Object instance) throws java.io.IOException { - String path = currentPath(getName()); - if (!authorizer.test(path, instance)) { - p.skipChildren(); - return instance; - } - PATH_STACK.get().push(prefixForNested(path)); - try { - return delegate.deserializeSetAndReturn(p, ctxt, instance); - } finally { - PATH_STACK.get().pop(); - } - } - } - - // --- Module that registers the modifier --- - - static ObjectMapper buildAuthorizingMapper(BiPredicate authorizer) { - SimpleModule module = new SimpleModule(); - module.setDeserializerModifier(new BeanDeserializerModifier() { - @Override - public BeanDeserializerBuilder updateBuilder(DeserializationConfig config, - BeanDescription beanDesc, - BeanDeserializerBuilder builder) { - Iterator it = builder.getProperties(); - while (it.hasNext()) { - SettableBeanProperty original = it.next(); - builder.addOrReplaceProperty(new AuthorizingSettableBeanProperty(original, authorizer), true); - } - return builder; - } - }); - return new ObjectMapper().registerModule(module); - } - - // --- Test fixtures --- - - public static class Person { - public String name; - public String role; - public Address address; - public java.util.List
addresses; - public Address[] addressArray; - public java.util.Map addressMap; - } - - public static class Address { - public String city; - public String zip; - } - - // --- Tests --- - - @Override - protected void setUp() { - PATH_STACK.remove(); - } - - public void testTopLevelAuthorizedPropertyIsApplied() throws Exception { - ObjectMapper mapper = buildAuthorizingMapper((path, instance) -> "name".equals(path)); - Person p = mapper.readValue("{\"name\":\"alice\",\"role\":\"admin\"}", Person.class); - assertEquals("alice", p.name); - assertNull("role must be skipped", p.role); - } - - public void testNestedPropertyAuthorizedByFullPath() throws Exception { - // Authorize address (depth 0) and address.city (depth 1), but NOT address.zip - ObjectMapper mapper = buildAuthorizingMapper((path, instance) -> - "address".equals(path) || "address.city".equals(path)); - Person p = mapper.readValue("{\"address\":{\"city\":\"Warsaw\",\"zip\":\"00-001\"}}", Person.class); - assertNotNull("address should be set", p.address); - assertEquals("Warsaw", p.address.city); - assertNull("zip must be skipped because address.zip not authorized", p.address.zip); - } - - public void testNestedRejectedAtParent() throws Exception { - // Reject "address" entirely at depth 0; nested fields should not be visited - ObjectMapper mapper = buildAuthorizingMapper((path, instance) -> "name".equals(path)); - Person p = mapper.readValue("{\"name\":\"alice\",\"address\":{\"city\":\"Warsaw\"}}", Person.class); - assertEquals("alice", p.name); - assertNull("address must be rejected at the parent, no nested visit", p.address); - } - - // --- Collection / Array / Map indexed-path tests --- - - public void testListOfBeansUsesIndexedPath() throws Exception { - // Authorize "addresses" (depth 0) AND "addresses[0].city" (depth 2) but NOT "addresses[0].zip" - ObjectMapper mapper = buildAuthorizingMapper((path, instance) -> - "addresses".equals(path) || "addresses[0].city".equals(path)); - Person p = mapper.readValue( - "{\"addresses\":[{\"city\":\"Warsaw\",\"zip\":\"00-001\"},{\"city\":\"Krakow\",\"zip\":\"30-001\"}]}", - Person.class); - assertNotNull(p.addresses); - assertEquals(2, p.addresses.size()); - assertEquals("Warsaw", p.addresses.get(0).city); - assertNull("addresses[0].zip must be skipped on element 0", p.addresses.get(0).zip); - assertEquals("Krakow", p.addresses.get(1).city); - assertNull("addresses[0].zip must be skipped on element 1 too (same path token)", p.addresses.get(1).zip); - } - - public void testListRejectedAtParentSkipsAllElements() throws Exception { - ObjectMapper mapper = buildAuthorizingMapper((path, instance) -> "name".equals(path)); - Person p = mapper.readValue( - "{\"name\":\"alice\",\"addresses\":[{\"city\":\"Warsaw\"}]}", Person.class); - assertEquals("alice", p.name); - assertNull("addresses must be rejected at parent — Jackson never visits elements", p.addresses); - } - - public void testArrayOfBeansUsesIndexedPath() throws Exception { - ObjectMapper mapper = buildAuthorizingMapper((path, instance) -> - "addressArray".equals(path) || "addressArray[0].city".equals(path)); - Person p = mapper.readValue( - "{\"addressArray\":[{\"city\":\"Warsaw\",\"zip\":\"00-001\"}]}", Person.class); - assertNotNull(p.addressArray); - assertEquals(1, p.addressArray.length); - assertEquals("Warsaw", p.addressArray[0].city); - assertNull("addressArray[0].zip must be skipped", p.addressArray[0].zip); - } - - public void testMapOfBeansUsesIndexedPath() throws Exception { - // Map values use [0] suffix (matching ParametersInterceptor bracket semantics + JSONInterceptor) - ObjectMapper mapper = buildAuthorizingMapper((path, instance) -> - "addressMap".equals(path) || "addressMap[0].city".equals(path)); - Person p = mapper.readValue( - "{\"addressMap\":{\"home\":{\"city\":\"Warsaw\",\"zip\":\"00-001\"}}}", Person.class); - assertNotNull(p.addressMap); - assertEquals(1, p.addressMap.size()); - assertNotNull(p.addressMap.get("home")); - assertEquals("Warsaw", p.addressMap.get("home").city); - assertNull("addressMap[0].zip must be skipped", p.addressMap.get("home").zip); - } - - public void testPathStackIsCleanAfterDeserialization() throws Exception { - ObjectMapper mapper = buildAuthorizingMapper((path, instance) -> true); - mapper.readValue("{\"name\":\"alice\",\"address\":{\"city\":\"Warsaw\"}}", Person.class); - assertTrue("ThreadLocal stack must be empty after deserialization", PATH_STACK.get().isEmpty()); - } -} From fc2f541557493c998a809f7947a814ddca76fe8f Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Tue, 5 May 2026 06:13:39 +0200 Subject: [PATCH 12/13] WW-5626 make JuneauXmlHandler authorization-aware via post-parse walk MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements AuthorizationAwareContentTypeHandler. When ParameterAuthorizationContext is active (set by ContentTypeInterceptor when requireAnnotations=true), the handler walks the parsed result tree and copies only authorized properties to the target, descending into nested beans/collections/maps/arrays with indexed-path semantics ([0] suffix) for parity with ParametersInterceptor. Note: Juneau parses the entire result tree before our walk runs, so setter side effects on transient nested objects can fire even for unauthorized properties — those transient objects are then discarded. This is functionally equivalent to the legacy two-phase copy in ContentTypeInterceptor; only the Jackson handlers achieve the stronger guarantee where unauthorized subtrees are never instantiated at all (they use Jackson's BeanDeserializerModifier + skipChildren). When no context is bound (default config), behavior is unchanged: parser.parse + BeanUtils.copyProperties. --- .../rest/handler/JuneauXmlHandler.java | 204 +++++++++++++++++- 1 file changed, 201 insertions(+), 3 deletions(-) diff --git a/plugins/rest/src/main/java/org/apache/struts2/rest/handler/JuneauXmlHandler.java b/plugins/rest/src/main/java/org/apache/struts2/rest/handler/JuneauXmlHandler.java index 30057c4eed..a749900fa2 100644 --- a/plugins/rest/src/main/java/org/apache/struts2/rest/handler/JuneauXmlHandler.java +++ b/plugins/rest/src/main/java/org/apache/struts2/rest/handler/JuneauXmlHandler.java @@ -27,17 +27,39 @@ import org.apache.juneau.xml.XmlSerializer; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; +import org.apache.struts2.interceptor.parameter.ParameterAuthorizationContext; +import java.beans.BeanInfo; +import java.beans.IntrospectionException; +import java.beans.Introspector; +import java.beans.PropertyDescriptor; import java.io.IOException; import java.io.Reader; import java.io.Writer; +import java.lang.reflect.Array; import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; +import java.util.Collection; +import java.util.Map; /** * Handles XML content using Apache Juneau - * http://juneau.apache.org/#marshall.html + * http://juneau.apache.org/#marshall.html + * + *

Implements {@link AuthorizationAwareContentTypeHandler}: when + * {@link ParameterAuthorizationContext#isActive()} is {@code true}, performs a post-parse walk + * over the parsed result and copies only authorized properties to the target. Without an active + * context, behavior is unchanged (Juneau parses, then {@code BeanUtils.copyProperties} populates + * the target).

+ * + *

Note: Juneau's parser builds the entire result tree before our authorization walk runs, so + * setter side effects on transient nested objects may fire even for unauthorized properties — + * those transient objects are then discarded. This is functionally equivalent to the legacy + * two-phase copy in {@code ContentTypeInterceptor}, with the same security model. Only the + * Jackson-based handlers ({@link JacksonJsonHandler}, {@link JacksonXmlHandler}) achieve the + * stronger guarantee where unauthorized subtrees are never instantiated at all.

*/ -public class JuneauXmlHandler implements ContentTypeHandler { +public class JuneauXmlHandler implements AuthorizationAwareContentTypeHandler { private static final Logger LOG = LogManager.getLogger(JuneauXmlHandler.class); @@ -51,12 +73,188 @@ public void toObject(ActionInvocation invocation, Reader in, Object target) thro LOG.debug("Converting input into an object of: {}", target.getClass().getName()); try { Object result = parser.parse(in, target.getClass()); - BeanUtils.copyProperties(target, result); + if (ParameterAuthorizationContext.isActive()) { + copyAuthorizedProperties(target, result, ""); + } else { + BeanUtils.copyProperties(target, result); + } } catch (ParseException | IllegalAccessException | InvocationTargetException e) { throw new IOException(e); } } + /** + * Recursively copies properties from {@code source} into {@code target}, consulting + * {@link ParameterAuthorizationContext} at each property. Unauthorized properties are skipped + * (target retains its existing value). Authorized scalar properties are copied directly; + * authorized nested beans are recursed into so their nested fields are individually authorized; + * authorized collections / maps / arrays use indexed-path semantics ({@code path[0].field}). + */ + private void copyAuthorizedProperties(Object target, Object source, String prefix) throws IOException { + if (source == null) { + return; + } + BeanInfo beanInfo; + try { + beanInfo = Introspector.getBeanInfo(source.getClass(), Object.class); + } catch (IntrospectionException e) { + throw new IOException("Unable to introspect " + source.getClass(), e); + } + for (PropertyDescriptor pd : beanInfo.getPropertyDescriptors()) { + copyAuthorizedProperty(target, source, prefix, pd); + } + } + + private void copyAuthorizedProperty(Object target, Object source, String prefix, PropertyDescriptor pd) + throws IOException { + Method readMethod = pd.getReadMethod(); + Method writeMethod = pd.getWriteMethod(); + if (readMethod == null || writeMethod == null) { + return; + } + String path = prefix.isEmpty() ? pd.getName() : prefix + "." + pd.getName(); + if (!ParameterAuthorizationContext.isAuthorized(path)) { + LOG.warn("REST body parameter [{}] rejected by @StrutsParameter authorization on [{}]", + path, target.getClass().getName()); + return; + } + Object value; + try { + value = readMethod.invoke(source); + } catch (ReflectiveOperationException e) { + throw new IOException("Failed reading " + path, e); + } + if (value == null) { + return; + } + try { + writeAuthorizedValue(target, readMethod, writeMethod, value, path); + } catch (ReflectiveOperationException e) { + throw new IOException("Failed writing " + path, e); + } + } + + private void writeAuthorizedValue(Object target, Method readMethod, Method writeMethod, Object value, String path) + throws ReflectiveOperationException, IOException { + if (value instanceof Collection collection) { + writeMethod.invoke(target, copyAuthorizedCollection(collection, path)); + } else if (value instanceof Map map) { + writeMethod.invoke(target, copyAuthorizedMap(map, path)); + } else if (value.getClass().isArray()) { + writeMethod.invoke(target, copyAuthorizedArray(value, path)); + } else if (isLeaf(value.getClass())) { + writeMethod.invoke(target, value); + } else { + writeAuthorizedNestedBean(target, readMethod, writeMethod, value, path); + } + } + + private void writeAuthorizedNestedBean(Object target, Method readMethod, Method writeMethod, + Object value, String path) + throws ReflectiveOperationException, IOException { + Object nestedTarget = readMethod.invoke(target); + if (nestedTarget == null) { + nestedTarget = newInstance(value.getClass()); + if (nestedTarget == null) { + // Cannot authorize without a fresh target instance; skip rather than + // bulk-copy the unfiltered value. + LOG.warn("REST nested bean [{}] skipped — no no-arg constructor for [{}]", + path, value.getClass().getName()); + return; + } + writeMethod.invoke(target, nestedTarget); + } + copyAuthorizedProperties(nestedTarget, value, path); + } + + private Collection copyAuthorizedCollection(Collection source, String prefix) throws IOException { + Collection result = newCollection(source); + String elementPath = prefix + "[0]"; + for (Object element : source) { + result.add(copyAuthorizedElement(element, elementPath)); + } + return result; + } + + private Map copyAuthorizedMap(Map source, String prefix) throws IOException { + Map result = newMap(source); + String elementPath = prefix + "[0]"; + for (Map.Entry entry : source.entrySet()) { + result.put(entry.getKey(), copyAuthorizedElement(entry.getValue(), elementPath)); + } + return result; + } + + private Object copyAuthorizedArray(Object sourceArray, String prefix) throws IOException { + int length = Array.getLength(sourceArray); + Object result = Array.newInstance(sourceArray.getClass().getComponentType(), length); + String elementPath = prefix + "[0]"; + for (int i = 0; i < length; i++) { + Object element = Array.get(sourceArray, i); + Object copied = copyAuthorizedElement(element, elementPath); + if (copied != null || !sourceArray.getClass().getComponentType().isPrimitive()) { + Array.set(result, i, copied); + } + } + return result; + } + + private Object copyAuthorizedElement(Object element, String elementPath) throws IOException { + if (element == null || isLeaf(element.getClass())) { + return element; + } + Object freshElement = newInstance(element.getClass()); + if (freshElement == null) { + LOG.warn("REST element [{}] skipped — no no-arg constructor for [{}]", + elementPath, element.getClass().getName()); + return null; + } + copyAuthorizedProperties(freshElement, element, elementPath); + return freshElement; + } + + /** + * Treats common JDK value types as leaves (no introspection needed). Mirrors the + * conservative classification used elsewhere in the REST plugin's two-phase copy. + */ + private static boolean isLeaf(Class c) { + if (c.isPrimitive() || c.isEnum()) return true; + String n = c.getName(); + return n.startsWith("java.lang.") + || n.startsWith("java.math.") + || n.startsWith("java.time.") + || n.startsWith("java.net.") + || n.startsWith("java.io.") + || n.startsWith("java.nio.") + || (n.startsWith("java.util.") && !Collection.class.isAssignableFrom(c) && !Map.class.isAssignableFrom(c)); + } + + private static Object newInstance(Class c) { + try { + return c.getDeclaredConstructor().newInstance(); + } catch (ReflectiveOperationException e) { + return null; + } + } + + @SuppressWarnings("unchecked") + private static Collection newCollection(Collection source) { + try { + return (Collection) source.getClass().getDeclaredConstructor().newInstance(); + } catch (ReflectiveOperationException e) { + return new java.util.ArrayList<>(); + } + } + + @SuppressWarnings("unchecked") + private static Map newMap(Map source) { + try { + return (Map) source.getClass().getDeclaredConstructor().newInstance(); + } catch (ReflectiveOperationException e) { + return new java.util.LinkedHashMap<>(); + } + } + @Override public String fromObject(ActionInvocation invocation, Object obj, String resultCode, Writer stream) throws IOException { LOG.debug("Converting an object of {} into string", obj.getClass().getName()); From 3c0be4a3bc4f85cd37ec60d8b4516cd62aa8d339 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Tue, 5 May 2026 06:13:45 +0200 Subject: [PATCH 13/13] WW-5626 add JuneauXmlHandler integration tests for @StrutsParameter authorization --- .../rest/JuneauXmlHandlerIntegrationTest.java | 136 ++++++++++++++++++ 1 file changed, 136 insertions(+) create mode 100644 plugins/rest/src/test/java/org/apache/struts2/rest/JuneauXmlHandlerIntegrationTest.java diff --git a/plugins/rest/src/test/java/org/apache/struts2/rest/JuneauXmlHandlerIntegrationTest.java b/plugins/rest/src/test/java/org/apache/struts2/rest/JuneauXmlHandlerIntegrationTest.java new file mode 100644 index 0000000000..3c98624944 --- /dev/null +++ b/plugins/rest/src/test/java/org/apache/struts2/rest/JuneauXmlHandlerIntegrationTest.java @@ -0,0 +1,136 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.struts2.rest; + +import com.mockobjects.dynamic.AnyConstraintMatcher; +import com.mockobjects.dynamic.Mock; +import junit.framework.TestCase; +import org.apache.struts2.ActionContext; +import org.apache.struts2.ActionInvocation; +import org.apache.struts2.action.Action; +import org.apache.struts2.dispatcher.mapper.ActionMapping; +import org.apache.struts2.interceptor.parameter.StrutsParameterAuthorizer; +import org.apache.struts2.ognl.DefaultOgnlBeanInfoCacheFactory; +import org.apache.struts2.ognl.DefaultOgnlExpressionCacheFactory; +import org.apache.struts2.ognl.OgnlUtil; +import org.apache.struts2.ognl.StrutsOgnlGuard; +import org.apache.struts2.ognl.StrutsProxyCacheFactory; +import org.apache.struts2.rest.handler.JuneauXmlHandler; +import org.apache.struts2.util.StrutsProxyService; +import org.springframework.mock.web.MockHttpServletRequest; + +import static org.apache.struts2.ognl.OgnlCacheFactory.CacheType.LRU; + +/** + * End-to-end integration tests for {@link JuneauXmlHandler} as an + * {@link org.apache.struts2.rest.handler.AuthorizationAwareContentTypeHandler}: a real + * Juneau parser + a real {@link StrutsParameterAuthorizer} + the {@link ContentTypeInterceptor} + * binding {@code ParameterAuthorizationContext} before invoking the handler. Verifies that + * authorization filtering happens during the handler's post-parse walk rather than via the + * legacy two-phase copy in the interceptor. + */ +public class JuneauXmlHandlerIntegrationTest extends TestCase { + + private ContentTypeInterceptor interceptor; + private SecureRestAction action; + private Mock mockActionInvocation; + private Mock mockSelector; + + @Override + protected void setUp() throws Exception { + super.setUp(); + action = new SecureRestAction(); + + var ognlUtil = new OgnlUtil( + new DefaultOgnlExpressionCacheFactory<>("1000", LRU.toString()), + new DefaultOgnlBeanInfoCacheFactory<>("1000", LRU.toString()), + new StrutsOgnlGuard()); + var proxyService = new StrutsProxyService(new StrutsProxyCacheFactory<>("1000", "basic")); + + StrutsParameterAuthorizer authorizer = new StrutsParameterAuthorizer(); + authorizer.setOgnlUtil(ognlUtil); + authorizer.setProxyService(proxyService); + authorizer.setRequireAnnotations(Boolean.TRUE.toString()); + + interceptor = new ContentTypeInterceptor(); + interceptor.setParameterAuthorizer(authorizer); + interceptor.setRequireAnnotations(Boolean.TRUE.toString()); + + mockActionInvocation = new Mock(ActionInvocation.class); + mockSelector = new Mock(ContentTypeHandlerManager.class); + mockActionInvocation.expectAndReturn("getAction", action); + mockActionInvocation.expectAndReturn("getAction", action); + mockActionInvocation.expectAndReturn("invoke", Action.SUCCESS); + mockActionInvocation.matchAndReturn("getInvocationContext", ActionContext.getContext()); + mockSelector.expectAndReturn("getHandlerForRequest", new AnyConstraintMatcher() { + public boolean matches(Object[] args) { return true; } + }, new JuneauXmlHandler()); + interceptor.setContentTypeHandlerSelector((ContentTypeHandlerManager) mockSelector.proxy()); + } + + private void runWithBody(String body) throws Exception { + MockHttpServletRequest request = new MockHttpServletRequest(); + request.setContent(body.getBytes()); + request.setContentType("application/xml"); + + ActionContext.of() + .withActionMapping(new ActionMapping()) + .withServletRequest(request) + .bind(); + + interceptor.intercept((ActionInvocation) mockActionInvocation.proxy()); + mockSelector.verify(); + mockActionInvocation.verify(); + } + + public void testAnnotatedTopLevelPropertyIsApplied() throws Exception { + runWithBody("alice"); + assertEquals("alice", action.getName()); + } + + public void testUnannotatedTopLevelPropertyIsRejected() throws Exception { + runWithBody("admin"); + assertNull("unannotated 'role' must not be set on target", action.getRole()); + } + + public void testMixedPropertiesFilteredCorrectly() throws Exception { + runWithBody("aliceadmin"); + assertEquals("alice", action.getName()); + assertNull(action.getRole()); + } + + public void testNestedPropertyAuthorizedWhenDepthAllows() throws Exception { + runWithBody("
Warsaw00-001
"); + assertNotNull("address should be set", action.getAddress()); + assertEquals("Warsaw", action.getAddress().getCity()); + assertEquals("00-001", action.getAddress().getZip()); + } + + public void testNestedPropertyRejectedWhenDepthInsufficient() throws Exception { + // shallowAddress: setter @StrutsParameter (depth-0 authorized) but getter has no depth>=1 + // annotation. The handler walks into the parsed Address and rejects city/zip individually. + runWithBody("Warsaw00-001"); + assertNotNull("shallowAddress is depth-0 authorized; handler enters it", + action.getShallowAddress()); + assertNull("nested city must be rejected (depth-1 not authorized)", + action.getShallowAddress().getCity()); + assertNull("nested zip must be rejected (depth-1 not authorized)", + action.getShallowAddress().getZip()); + } +}