From c01270e19d2d1a40502ae322bd48759d6e5aa9d6 Mon Sep 17 00:00:00 2001 From: tranquac Date: Thu, 9 Apr 2026 09:02:21 +0700 Subject: [PATCH 1/5] WW-5624 fix(security): enforce @StrutsParameter on JSON/REST body deserialization Extract ParameterAuthorizer service from ParametersInterceptor to share @StrutsParameter annotation enforcement across all input channels. The json-plugin (JSONInterceptor) and rest-plugin (ContentTypeInterceptor) previously bypassed @StrutsParameter checks when deserializing request bodies, allowing mass assignment even when struts.parameters.requireAnnotations=true. Changes: - New ParameterAuthorizer interface and DefaultParameterAuthorizer impl - JSONInterceptor: filter unauthorized Map keys before populateObject() - ContentTypeInterceptor: two-phase deserialization (fresh instance then copy authorized properties) when requireAnnotations=true; direct deserialization for backward compat when disabled - OGNL ThreadAllowlist side effects remain in ParametersInterceptor only - Full DI wiring: struts-beans.xml + StrutsBeanSelectionProvider + DefaultConfiguration - 15 new unit tests for ParameterAuthorizer, 2 for JSON plugin, 2 for REST plugin; 32 existing regression tests verified --- .../org/apache/struts2/StrutsConstants.java | 7 + .../config/StrutsBeanSelectionProvider.java | 2 + .../config/impl/DefaultConfiguration.java | 3 + .../parameter/DefaultParameterAuthorizer.java | 229 ++++++++++++++++++ .../parameter/ParameterAuthorizer.java | 48 ++++ .../parameter/ParametersInterceptor.java | 69 +++++- core/src/main/resources/struts-beans.xml | 3 + .../parameter/ParameterAuthorizerTest.java | 226 +++++++++++++++++ .../StrutsParameterAnnotationTest.java | 10 + .../apache/struts2/json/JSONInterceptor.java | 23 ++ .../struts2/json/JSONInterceptorTest.java | 45 ++++ .../struts2/rest/ContentTypeInterceptor.java | 73 +++++- .../rest/ContentTypeInterceptorTest.java | 82 +++++++ 13 files changed, 810 insertions(+), 10 deletions(-) create mode 100644 core/src/main/java/org/apache/struts2/interceptor/parameter/DefaultParameterAuthorizer.java create mode 100644 core/src/main/java/org/apache/struts2/interceptor/parameter/ParameterAuthorizer.java create mode 100644 core/src/test/java/org/apache/struts2/interceptor/parameter/ParameterAuthorizerTest.java diff --git a/core/src/main/java/org/apache/struts2/StrutsConstants.java b/core/src/main/java/org/apache/struts2/StrutsConstants.java index 84b9fd16e1..eb925b422a 100644 --- a/core/src/main/java/org/apache/struts2/StrutsConstants.java +++ b/core/src/main/java/org/apache/struts2/StrutsConstants.java @@ -551,6 +551,13 @@ public final class StrutsConstants { */ public static final String STRUTS_PROXYSERVICE = "struts.proxyService"; + /** + * The {@link org.apache.struts2.interceptor.parameter.ParameterAuthorizer} implementation class. + * + * @since 7.2.0 + */ + public static final String STRUTS_PARAMETER_AUTHORIZER = "struts.parameterAuthorizer"; + /** * Enables evaluation of OGNL expressions * diff --git a/core/src/main/java/org/apache/struts2/config/StrutsBeanSelectionProvider.java b/core/src/main/java/org/apache/struts2/config/StrutsBeanSelectionProvider.java index c584a4f587..f169b67f1c 100644 --- a/core/src/main/java/org/apache/struts2/config/StrutsBeanSelectionProvider.java +++ b/core/src/main/java/org/apache/struts2/config/StrutsBeanSelectionProvider.java @@ -73,6 +73,7 @@ import org.apache.struts2.url.UrlEncoder; import org.apache.struts2.util.ContentTypeMatcher; import org.apache.struts2.util.PatternMatcher; +import org.apache.struts2.interceptor.parameter.ParameterAuthorizer; import org.apache.struts2.util.ProxyService; import org.apache.struts2.util.TextParser; import org.apache.struts2.util.ValueStackFactory; @@ -446,6 +447,7 @@ public void register(ContainerBuilder builder, LocatableProperties props) { alias(BeanInfoCacheFactory.class, StrutsConstants.STRUTS_OGNL_BEANINFO_CACHE_FACTORY, builder, props, Scope.SINGLETON); alias(ProxyCacheFactory.class, StrutsConstants.STRUTS_PROXY_CACHE_FACTORY, builder, props, Scope.SINGLETON); alias(ProxyService.class, StrutsConstants.STRUTS_PROXYSERVICE, builder, props, Scope.SINGLETON); + alias(ParameterAuthorizer.class, StrutsConstants.STRUTS_PARAMETER_AUTHORIZER, builder, props, Scope.SINGLETON); alias(SecurityMemberAccess.class, StrutsConstants.STRUTS_MEMBER_ACCESS, builder, props, Scope.PROTOTYPE); alias(OgnlGuard.class, StrutsConstants.STRUTS_OGNL_GUARD, builder, props, Scope.SINGLETON); diff --git a/core/src/main/java/org/apache/struts2/config/impl/DefaultConfiguration.java b/core/src/main/java/org/apache/struts2/config/impl/DefaultConfiguration.java index 6c46fd2647..4ae87a5c5d 100644 --- a/core/src/main/java/org/apache/struts2/config/impl/DefaultConfiguration.java +++ b/core/src/main/java/org/apache/struts2/config/impl/DefaultConfiguration.java @@ -92,6 +92,8 @@ import org.apache.struts2.ognl.accessor.CompoundRootAccessor; import org.apache.struts2.ognl.accessor.RootAccessor; import org.apache.struts2.ognl.accessor.XWorkMethodAccessor; +import org.apache.struts2.interceptor.parameter.DefaultParameterAuthorizer; +import org.apache.struts2.interceptor.parameter.ParameterAuthorizer; import org.apache.struts2.util.StrutsProxyService; import org.apache.struts2.util.OgnlTextParser; import org.apache.struts2.util.PatternMatcher; @@ -406,6 +408,7 @@ public static ContainerBuilder bootstrapFactories(ContainerBuilder builder) { .factory(BeanInfoCacheFactory.class, DefaultOgnlBeanInfoCacheFactory.class, Scope.SINGLETON) .factory(ProxyCacheFactory.class, StrutsProxyCacheFactory.class, Scope.SINGLETON) .factory(ProxyService.class, StrutsProxyService.class, Scope.SINGLETON) + .factory(ParameterAuthorizer.class, DefaultParameterAuthorizer.class, Scope.SINGLETON) .factory(OgnlUtil.class, Scope.SINGLETON) .factory(SecurityMemberAccess.class, Scope.PROTOTYPE) .factory(OgnlGuard.class, StrutsOgnlGuard.class, Scope.SINGLETON) diff --git a/core/src/main/java/org/apache/struts2/interceptor/parameter/DefaultParameterAuthorizer.java b/core/src/main/java/org/apache/struts2/interceptor/parameter/DefaultParameterAuthorizer.java new file mode 100644 index 0000000000..d806664ee1 --- /dev/null +++ b/core/src/main/java/org/apache/struts2/interceptor/parameter/DefaultParameterAuthorizer.java @@ -0,0 +1,229 @@ +/* + * 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.apache.commons.lang3.BooleanUtils; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; +import org.apache.struts2.StrutsConstants; +import org.apache.struts2.inject.Inject; +import org.apache.struts2.ognl.OgnlUtil; +import org.apache.struts2.util.ProxyService; + +import java.beans.BeanInfo; +import java.beans.IntrospectionException; +import java.beans.PropertyDescriptor; +import java.lang.reflect.AnnotatedElement; +import java.lang.reflect.Field; +import java.lang.reflect.Method; +import java.lang.reflect.Modifier; +import java.util.Arrays; +import java.util.Optional; + +import static java.lang.String.format; +import static org.apache.commons.lang3.StringUtils.indexOfAny; +import static org.apache.struts2.security.DefaultAcceptedPatternsChecker.NESTING_CHARS; +import static org.apache.struts2.security.DefaultAcceptedPatternsChecker.NESTING_CHARS_STR; +import static org.apache.struts2.util.DebugUtils.notifyDeveloperOfError; + +/** + * Default implementation of {@link ParameterAuthorizer} that checks {@link StrutsParameter} annotations on the target + * object's members to determine whether a parameter is authorized for injection. + * + *

This implementation extracts the authorization logic from {@link ParametersInterceptor} so that it can be shared + * with other input channels (JSON plugin, REST plugin) without duplicating code.

+ * + *

Unlike {@link ParametersInterceptor}, this implementation does NOT perform OGNL ThreadAllowlist side effects. + * Those remain specific to the OGNL-based parameter injection path.

+ * + * @since 7.2.0 + */ +public class DefaultParameterAuthorizer implements ParameterAuthorizer { + + private static final Logger LOG = LogManager.getLogger(DefaultParameterAuthorizer.class); + + private boolean requireAnnotations = false; + private boolean requireAnnotationsTransitionMode = false; + private boolean devMode = false; + + private OgnlUtil ognlUtil; + private ProxyService proxyService; + + @Inject + public void setOgnlUtil(OgnlUtil ognlUtil) { + this.ognlUtil = ognlUtil; + } + + @Inject + public void setProxyService(ProxyService proxyService) { + this.proxyService = proxyService; + } + + @Inject(StrutsConstants.STRUTS_DEVMODE) + public void setDevMode(String mode) { + this.devMode = BooleanUtils.toBoolean(mode); + } + + @Inject(value = StrutsConstants.STRUTS_PARAMETERS_REQUIRE_ANNOTATIONS, required = false) + public void setRequireAnnotations(String requireAnnotations) { + this.requireAnnotations = BooleanUtils.toBoolean(requireAnnotations); + } + + @Inject(value = StrutsConstants.STRUTS_PARAMETERS_REQUIRE_ANNOTATIONS_TRANSITION, required = false) + public void setRequireAnnotationsTransitionMode(String transitionMode) { + this.requireAnnotationsTransitionMode = BooleanUtils.toBoolean(transitionMode); + } + + @Override + public boolean isAuthorized(String parameterName, Object target, Object action) { + if (parameterName == null || parameterName.isEmpty()) { + return false; + } + + if (!requireAnnotations) { + return true; + } + + long paramDepth = parameterName.codePoints().mapToObj(c -> (char) c).filter(NESTING_CHARS::contains).count(); + + // ModelDriven exemption: when target is the model (target != action), exempt from annotation requirements + if (target != action) { + LOG.debug("Model driven target detected, exempting from @StrutsParameter annotation requirement"); + return true; + } + + // Transition mode: depth-0 (non-nested) parameters are exempt + if (requireAnnotationsTransitionMode && paramDepth == 0) { + LOG.debug("Annotation transition mode enabled, exempting non-nested parameter [{}] from @StrutsParameter annotation requirement", + parameterName); + return true; + } + + int nestingIndex = indexOfAny(parameterName, NESTING_CHARS_STR); + String rootProperty = nestingIndex == -1 ? parameterName : parameterName.substring(0, nestingIndex); + String normalisedRootProperty = Character.toLowerCase(rootProperty.charAt(0)) + rootProperty.substring(1); + + return hasValidAnnotatedMember(normalisedRootProperty, target, paramDepth); + } + + protected boolean hasValidAnnotatedMember(String rootProperty, Object target, long paramDepth) { + LOG.debug("Checking target [{}] for a matching, correctly annotated member for property [{}]", + target.getClass().getSimpleName(), rootProperty); + BeanInfo beanInfo = getBeanInfo(target); + if (beanInfo == null) { + return hasValidAnnotatedField(target, rootProperty, paramDepth); + } + + Optional propDescOpt = Arrays.stream(beanInfo.getPropertyDescriptors()) + .filter(desc -> desc.getName().equals(rootProperty)).findFirst(); + if (propDescOpt.isEmpty()) { + return hasValidAnnotatedField(target, rootProperty, paramDepth); + } + + if (hasValidAnnotatedPropertyDescriptor(target, propDescOpt.get(), paramDepth)) { + return true; + } + + return hasValidAnnotatedField(target, rootProperty, paramDepth); + } + + protected boolean hasValidAnnotatedPropertyDescriptor(Object target, PropertyDescriptor propDesc, long paramDepth) { + Class targetClass = ultimateClass(target); + Method relevantMethod = paramDepth == 0 ? propDesc.getWriteMethod() : propDesc.getReadMethod(); + if (relevantMethod == null) { + return false; + } + if (getPermittedInjectionDepth(relevantMethod) < paramDepth) { + String logMessage = format( + "Parameter injection for method [%s] on target [%s] rejected. Ensure it is annotated with @StrutsParameter with an appropriate 'depth'.", + relevantMethod.getName(), + relevantMethod.getDeclaringClass().getName()); + if (devMode) { + notifyDeveloperOfError(LOG, target, logMessage); + } else { + LOG.debug(logMessage); + } + return false; + } + LOG.debug("Success: Matching annotated method [{}] found for property [{}] of depth [{}] on target [{}]", + relevantMethod.getName(), propDesc.getName(), paramDepth, targetClass.getSimpleName()); + return true; + } + + protected boolean hasValidAnnotatedField(Object target, String fieldName, long paramDepth) { + Class targetClass = ultimateClass(target); + LOG.debug("No matching annotated method found for property [{}] of depth [{}] on target [{}], now also checking for public field", + fieldName, paramDepth, targetClass.getSimpleName()); + Field field; + try { + field = targetClass.getDeclaredField(fieldName); + } catch (NoSuchFieldException e) { + LOG.debug("Matching field for property [{}] not found on target [{}]", fieldName, targetClass.getSimpleName()); + return false; + } + if (!Modifier.isPublic(field.getModifiers())) { + LOG.debug("Matching field [{}] is not public on target [{}]", field.getName(), targetClass.getSimpleName()); + return false; + } + if (getPermittedInjectionDepth(field) < paramDepth) { + String logMessage = format( + "Parameter injection for field [%s] on target [%s] rejected. Ensure it is annotated with @StrutsParameter with an appropriate 'depth'.", + field.getName(), + targetClass.getName()); + if (devMode) { + notifyDeveloperOfError(LOG, target, logMessage); + } else { + LOG.debug(logMessage); + } + return false; + } + LOG.debug("Success: Matching annotated public field [{}] found for property of depth [{}] on target [{}]", + field.getName(), paramDepth, targetClass.getSimpleName()); + return true; + } + + protected int getPermittedInjectionDepth(AnnotatedElement element) { + StrutsParameter annotation = getParameterAnnotation(element); + if (annotation == null) { + return -1; + } + return annotation.depth(); + } + + protected StrutsParameter getParameterAnnotation(AnnotatedElement element) { + return element.getAnnotation(StrutsParameter.class); + } + + protected Class ultimateClass(Object target) { + if (proxyService.isProxy(target)) { + return proxyService.ultimateTargetClass(target); + } + return target.getClass(); + } + + protected BeanInfo getBeanInfo(Object target) { + Class targetClass = ultimateClass(target); + try { + return ognlUtil.getBeanInfo(targetClass); + } catch (IntrospectionException e) { + LOG.warn("Error introspecting target {} for parameter authorization", targetClass, e); + return null; + } + } +} diff --git a/core/src/main/java/org/apache/struts2/interceptor/parameter/ParameterAuthorizer.java b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParameterAuthorizer.java new file mode 100644 index 0000000000..d064fbf68e --- /dev/null +++ b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParameterAuthorizer.java @@ -0,0 +1,48 @@ +/* + * 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; + +/** + * Service for determining whether a given parameter name is authorized for injection into a target object, based on + * {@link StrutsParameter} annotation presence and depth. + * + *

This service extracts the authorization logic from {@link ParametersInterceptor} so that it can be reused by other + * input channels (e.g. JSON plugin, REST plugin) that also need to enforce {@code @StrutsParameter} rules.

+ * + *

Implementations must NOT perform OGNL ThreadAllowlist side effects — those remain specific to + * {@link ParametersInterceptor}.

+ * + * @since 7.2.0 + */ +public interface ParameterAuthorizer { + + /** + * Determines whether a parameter with the given name is authorized for injection into the given target object. + * + *

When {@code struts.parameters.requireAnnotations} is {@code false}, this method always returns {@code true} + * for backward compatibility.

+ * + * @param parameterName the parameter name (e.g. "name", "address.city", "items[0].name") + * @param target the object receiving the parameter value (the action, or the model for ModelDriven actions) + * @param action the action instance; used to detect ModelDriven exemption (when {@code target != action}, + * the target is the model and is exempt from annotation requirements) + * @return {@code true} if the parameter is authorized for injection, {@code false} otherwise + */ + boolean isAuthorized(String parameterName, Object target, Object action); +} diff --git a/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java index 293f4968a9..6173a85c65 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java @@ -100,6 +100,7 @@ public class ParametersInterceptor extends MethodFilterInterceptor { private AcceptedPatternsChecker acceptedPatterns; private Set excludedValuePatterns = null; private Set acceptedValuePatterns = null; + private ParameterAuthorizer parameterAuthorizer; @Inject public void setValueStackFactory(ValueStackFactory valueStackFactory) { @@ -121,6 +122,11 @@ public void setProxyService(ProxyService proxyService) { this.proxyService = proxyService; } + @Inject + public void setParameterAuthorizer(ParameterAuthorizer parameterAuthorizer) { + this.parameterAuthorizer = parameterAuthorizer; + } + @Inject(StrutsConstants.STRUTS_DEVMODE) public void setDevMode(String mode) { this.devMode = BooleanUtils.toBoolean(mode); @@ -352,6 +358,9 @@ protected boolean isAcceptableParameterNameAware(String name, Object action) { * Checks if the Action class member corresponding to a parameter is appropriately annotated with * {@link StrutsParameter} and OGNL allowlists any necessary classes. *

+ * Authorization is delegated to {@link ParameterAuthorizer}. If authorized, OGNL allowlisting is performed as a + * second pass (this is specific to the OGNL-based parameter injection path and not shared with other input channels). + *

* Note that this logic relies on the use of {@link DefaultAcceptedPatternsChecker#NESTING_CHARS} and may also * be adversely impacted by the use of custom OGNL property accessors. */ @@ -360,23 +369,67 @@ protected boolean isParameterAnnotatedAndAllowlist(String name, Object action) { return true; } - long paramDepth = name.codePoints().mapToObj(c -> (char) c).filter(NESTING_CHARS::contains).count(); + // Resolve target for ModelDriven: if the ValueStack peek is different from the action, it's the model + Object target = action; + if (action instanceof ModelDriven) { + Object stackTop = ActionContext.getContext().getValueStack().peek(); + if (!stackTop.equals(action)) { + target = stackTop; + } + } - if (action instanceof ModelDriven && !ActionContext.getContext().getValueStack().peek().equals(action)) { - LOG.debug("Model driven Action detected, exempting from @StrutsParameter annotation requirement"); - return true; + // Delegate authorization check to shared ParameterAuthorizer (no OGNL side effects) + if (!parameterAuthorizer.isAuthorized(name, target, action)) { + return false; } - if (requireAnnotationsTransitionMode && paramDepth == 0) { - LOG.debug("Annotation transition mode enabled, exempting non-nested parameter [{}] from @StrutsParameter annotation requirement", name); - return true; + // OGNL-specific allowlisting: only needed for nested params (depth >= 1) + long paramDepth = name.codePoints().mapToObj(c -> (char) c).filter(NESTING_CHARS::contains).count(); + if (paramDepth >= 1) { + performOgnlAllowlisting(name, target, paramDepth); } + return true; + } + /** + * Performs OGNL ThreadAllowlist side effects for an authorized parameter. This is specific to OGNL-based parameter + * injection and must NOT be shared with other input channels (JSON, REST). + */ + private void performOgnlAllowlisting(String name, Object target, long paramDepth) { int nestingIndex = indexOfAny(name, NESTING_CHARS_STR); String rootProperty = nestingIndex == -1 ? name : name.substring(0, nestingIndex); String normalisedRootProperty = Character.toLowerCase(rootProperty.charAt(0)) + rootProperty.substring(1); - return hasValidAnnotatedMember(normalisedRootProperty, action, paramDepth); + BeanInfo beanInfo = getBeanInfo(target); + if (beanInfo != null) { + Optional propDescOpt = Arrays.stream(beanInfo.getPropertyDescriptors()) + .filter(desc -> desc.getName().equals(normalisedRootProperty)).findFirst(); + if (propDescOpt.isPresent()) { + PropertyDescriptor propDesc = propDescOpt.get(); + Method relevantMethod = paramDepth == 0 ? propDesc.getWriteMethod() : propDesc.getReadMethod(); + if (relevantMethod != null && getPermittedInjectionDepth(relevantMethod) >= paramDepth) { + allowlistClass(propDesc.getPropertyType()); + if (paramDepth >= 2) { + allowlistReturnTypeIfParameterized(relevantMethod); + } + return; + } + } + } + + // Fallback: check public field + Class targetClass = ultimateClass(target); + try { + Field field = targetClass.getDeclaredField(normalisedRootProperty); + if (Modifier.isPublic(field.getModifiers()) && getPermittedInjectionDepth(field) >= paramDepth) { + allowlistClass(field.getType()); + if (paramDepth >= 2) { + allowlistFieldIfParameterized(field); + } + } + } catch (NoSuchFieldException e) { + // No field to allowlist + } } /** diff --git a/core/src/main/resources/struts-beans.xml b/core/src/main/resources/struts-beans.xml index 6141786919..030acbc2b4 100644 --- a/core/src/main/resources/struts-beans.xml +++ b/core/src/main/resources/struts-beans.xml @@ -245,6 +245,9 @@ + + (String.valueOf(1000), LRU.toString()), + new DefaultOgnlBeanInfoCacheFactory<>(String.valueOf(1000), LRU.toString()), + new StrutsOgnlGuard()); + authorizer.setOgnlUtil(ognlUtil); + + var proxyService = new StrutsProxyService(new StrutsProxyCacheFactory<>("1000", "basic")); + authorizer.setProxyService(proxyService); + } + + // --- requireAnnotations=false (backward compat) --- + + @Test + public void requireAnnotationsDisabled_allAuthorized() { + authorizer.setRequireAnnotations(Boolean.FALSE.toString()); + assertThat(authorizer.isAuthorized("anything", new SecureAction(), new SecureAction())).isTrue(); + assertThat(authorizer.isAuthorized("unannotatedProp", new SecureAction(), new SecureAction())).isTrue(); + } + + // --- Simple property (depth 0) --- + + @Test + public void annotatedSetter_authorized() { + var action = new SecureAction(); + assertThat(authorizer.isAuthorized("name", action, action)).isTrue(); + } + + @Test + public void unannotatedSetter_rejected() { + var action = new SecureAction(); + assertThat(authorizer.isAuthorized("role", action, action)).isFalse(); + } + + // --- Nested property (depth >= 1) --- + + @Test + public void annotatedGetterDepthOne_nestedParam_authorized() { + var action = new SecureAction(); + assertThat(authorizer.isAuthorized("address.city", action, action)).isTrue(); + } + + @Test + public void annotatedGetterDepthZero_nestedParam_rejected() { + var action = new SecureAction(); + assertThat(authorizer.isAuthorized("addressShallow.city", action, action)).isFalse(); + } + + @Test + public void annotatedGetterDepthOne_doubleNested_rejected() { + var action = new SecureAction(); + assertThat(authorizer.isAuthorized("address.city.zip", action, action)).isFalse(); + } + + // --- Public field --- + + @Test + public void annotatedPublicField_authorized() { + var action = new FieldAction(); + assertThat(authorizer.isAuthorized("publicStr", action, action)).isTrue(); + } + + @Test + public void unannotatedPublicField_rejected() { + var action = new FieldAction(); + assertThat(authorizer.isAuthorized("publicStrNotAnnotated", action, action)).isFalse(); + } + + // --- ModelDriven exemption --- + + @Test + public void modelDriven_targetIsModel_allAuthorized() { + var action = new ModelAction(); + var model = action.getModel(); + // target != action → model is exempt + assertThat(authorizer.isAuthorized("anyProperty", model, action)).isTrue(); + assertThat(authorizer.isAuthorized("nested.deep", model, action)).isTrue(); + } + + // --- Transition mode --- + + @Test + public void transitionMode_depthZeroExempt() { + authorizer.setRequireAnnotationsTransitionMode(Boolean.TRUE.toString()); + var action = new SecureAction(); + // depth-0 unannotated property should be exempt + assertThat(authorizer.isAuthorized("role", action, action)).isTrue(); + } + + @Test + public void transitionMode_depthOneNotExempt() { + authorizer.setRequireAnnotationsTransitionMode(Boolean.TRUE.toString()); + var action = new SecureAction(); + // depth-1 unannotated property should NOT be exempt + assertThat(authorizer.isAuthorized("unannotatedNested.prop", action, action)).isFalse(); + } + + // --- No matching member --- + + @Test + public void nonexistentProperty_rejected() { + var action = new SecureAction(); + assertThat(authorizer.isAuthorized("doesNotExist", action, action)).isFalse(); + } + + // --- Empty/null parameter name --- + + @Test + public void nullParameterName_rejected() { + var action = new SecureAction(); + assertThat(authorizer.isAuthorized(null, action, action)).isFalse(); + } + + @Test + public void emptyParameterName_rejected() { + var action = new SecureAction(); + assertThat(authorizer.isAuthorized("", action, action)).isFalse(); + } + + @Test + public void emptyParameterName_rejectedEvenWhenAnnotationsNotRequired() { + authorizer.setRequireAnnotations(Boolean.FALSE.toString()); + var action = new SecureAction(); + assertThat(authorizer.isAuthorized("", action, action)).isFalse(); + assertThat(authorizer.isAuthorized(null, action, action)).isFalse(); + } + + // --- Inner test classes --- + + public static class SecureAction { + private String name; + private String role; + private Address address; + private Address addressShallow; + + @StrutsParameter + public void setName(String name) { this.name = name; } + public String getName() { return name; } + + // NO @StrutsParameter — must be rejected + public void setRole(String role) { this.role = role; } + public String getRole() { return role; } + + @StrutsParameter(depth = 1) + public Address getAddress() { return address; } + public void setAddress(Address address) { this.address = address; } + + @StrutsParameter + public Address getAddressShallow() { return addressShallow; } + public void setAddressShallow(Address address) { this.addressShallow = address; } + + // Unannotated getter for nested param test + public Object getUnannotatedNested() { return null; } + } + + public static class Address { + private String city; + public String getCity() { return city; } + public void setCity(String city) { this.city = city; } + } + + public static class FieldAction { + @StrutsParameter + public String publicStr; + + public String publicStrNotAnnotated; + } + + public static class ModelAction implements ModelDriven { + @Override + public Pojo getModel() { return new Pojo(); } + } + + public static class Pojo { + private String name; + public String getName() { return name; } + public void setName(String name) { this.name = name; } + } +} diff --git a/core/src/test/java/org/apache/struts2/interceptor/parameter/StrutsParameterAnnotationTest.java b/core/src/test/java/org/apache/struts2/interceptor/parameter/StrutsParameterAnnotationTest.java index 0040e98d3c..4ab8f5b5de 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/parameter/StrutsParameterAnnotationTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/parameter/StrutsParameterAnnotationTest.java @@ -56,6 +56,7 @@ public class StrutsParameterAnnotationTest { private ParametersInterceptor parametersInterceptor; + private DefaultParameterAuthorizer parameterAuthorizer; private ThreadAllowlist threadAllowlist; @@ -76,6 +77,13 @@ public void setUp() throws Exception { var proxyService = new StrutsProxyService(new StrutsProxyCacheFactory<>("1000", "basic")); parametersInterceptor.setProxyService(proxyService); + var parameterAuthorizer = new DefaultParameterAuthorizer(); + parameterAuthorizer.setOgnlUtil(ognlUtil); + parameterAuthorizer.setProxyService(proxyService); + parameterAuthorizer.setRequireAnnotations(Boolean.TRUE.toString()); + this.parameterAuthorizer = parameterAuthorizer; + parametersInterceptor.setParameterAuthorizer(parameterAuthorizer); + NotExcludedAcceptedPatternsChecker checker = mock(NotExcludedAcceptedPatternsChecker.class); when(checker.isAccepted(anyString())).thenReturn(IsAccepted.yes("")); when(checker.isExcluded(anyString())).thenReturn(IsExcluded.no(Set.of())); @@ -360,6 +368,7 @@ public void publicPojoMapDepthTwoMethod() { @Test public void publicStrNotAnnotated_transitionMode() { parametersInterceptor.setRequireAnnotationsTransitionMode(Boolean.TRUE.toString()); + parameterAuthorizer.setRequireAnnotationsTransitionMode(Boolean.TRUE.toString()); testParameter(new FieldAction(), "publicStrNotAnnotated", true); } @@ -369,6 +378,7 @@ public void publicStrNotAnnotated_transitionMode() { @Test public void publicStrNotAnnotatedMethod_transitionMode() { parametersInterceptor.setRequireAnnotationsTransitionMode(Boolean.TRUE.toString()); + parameterAuthorizer.setRequireAnnotationsTransitionMode(Boolean.TRUE.toString()); testParameter(new MethodAction(), "publicStrNotAnnotated", true); } diff --git a/plugins/json/src/main/java/org/apache/struts2/json/JSONInterceptor.java b/plugins/json/src/main/java/org/apache/struts2/json/JSONInterceptor.java index 6511da0336..8085230f79 100644 --- a/plugins/json/src/main/java/org/apache/struts2/json/JSONInterceptor.java +++ b/plugins/json/src/main/java/org/apache/struts2/json/JSONInterceptor.java @@ -22,6 +22,7 @@ import org.apache.struts2.ActionInvocation; import org.apache.struts2.inject.Inject; import org.apache.struts2.interceptor.AbstractInterceptor; +import org.apache.struts2.interceptor.parameter.ParameterAuthorizer; import org.apache.struts2.util.ValueStack; import org.apache.struts2.util.WildcardUtil; import org.apache.commons.lang3.BooleanUtils; @@ -72,6 +73,7 @@ public class JSONInterceptor extends AbstractInterceptor { private String jsonRpcContentType = "application/json-rpc"; private JSONUtil jsonUtil; + private ParameterAuthorizer parameterAuthorizer; private int maxElements = JSONReader.DEFAULT_MAX_ELEMENTS; private int maxDepth = JSONReader.DEFAULT_MAX_DEPTH; private int maxLength = 2_097_152; // 2MB @@ -131,6 +133,9 @@ public String intercept(ActionInvocation invocation) throws Exception { if (rootObject == null) // model overrides action rootObject = invocation.getStack().peek(); + // enforce @StrutsParameter authorization on JSON body keys + filterUnauthorizedKeys(json, rootObject, invocation.getAction()); + // populate fields populator.populateObject(rootObject, json); } else { @@ -200,6 +205,19 @@ private void applyLimitsToReader() { reader.setMaxKeyLength(maxKeyLength); } + @SuppressWarnings("rawtypes") + private void filterUnauthorizedKeys(Map json, Object target, Object action) { + Iterator it = json.keySet().iterator(); + while (it.hasNext()) { + String key = it.next(); + if (!parameterAuthorizer.isAuthorized(key, target, action)) { + LOG.warn("JSON body parameter [{}] rejected by @StrutsParameter authorization on [{}]", + key, target.getClass().getName()); + it.remove(); + } + } + } + protected String readContentType(HttpServletRequest request) { String contentType = request.getHeader("Content-Type"); LOG.debug("Content Type from request: {}", contentType); @@ -585,6 +603,11 @@ public void setJsonUtil(JSONUtil jsonUtil) { this.jsonUtil = jsonUtil; } + @Inject + public void setParameterAuthorizer(ParameterAuthorizer parameterAuthorizer) { + this.parameterAuthorizer = parameterAuthorizer; + } + @Inject(value = JSONConstants.JSON_MAX_ELEMENTS, required = false) public void setMaxElements(String maxElements) { this.maxElements = Integer.parseInt(maxElements); diff --git a/plugins/json/src/test/java/org/apache/struts2/json/JSONInterceptorTest.java b/plugins/json/src/test/java/org/apache/struts2/json/JSONInterceptorTest.java index 9f5c4a75f5..e263075c33 100644 --- a/plugins/json/src/test/java/org/apache/struts2/json/JSONInterceptorTest.java +++ b/plugins/json/src/test/java/org/apache/struts2/json/JSONInterceptorTest.java @@ -23,6 +23,7 @@ import org.apache.struts2.util.ValueStack; import org.apache.struts2.junit.StrutsTestCase; import org.apache.struts2.junit.util.TestUtils; +import org.apache.struts2.interceptor.parameter.ParameterAuthorizer; import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.mock.web.MockServletContext; @@ -47,6 +48,8 @@ private JSONInterceptor createInterceptor() { jsonUtil.setReader(new StrutsJSONReader()); jsonUtil.setWriter(new StrutsJSONWriter()); interceptor.setJsonUtil(jsonUtil); + // Default: allow all parameters (simulates requireAnnotations=false) + interceptor.setParameterAuthorizer((parameterName, target, action) -> true); return interceptor; } @@ -556,6 +559,48 @@ public void testMaxDepthEnforcedThroughInterceptor() throws Exception { } } + public void testParameterAuthorizerRejectsUnauthorizedKeys() throws Exception { + // JSON body with "foo" and "bar" keys, but authorizer only allows "foo" + this.request.setContent("{\"foo\":\"allowed\", \"bar\":\"blocked\"}".getBytes()); + this.request.addHeader("Content-Type", "application/json"); + + JSONInterceptor interceptor = new JSONInterceptor(); + JSONUtil jsonUtil = new JSONUtil(); + jsonUtil.setReader(new StrutsJSONReader()); + jsonUtil.setWriter(new StrutsJSONWriter()); + interceptor.setJsonUtil(jsonUtil); + // Only authorize "foo", reject "bar" + interceptor.setParameterAuthorizer((parameterName, target, action) -> "foo".equals(parameterName)); + TestAction action = new TestAction(); + + this.invocation.setAction(action); + this.invocation.getStack().push(action); + + interceptor.intercept(this.invocation); + + // "foo" should be set, "bar" should NOT be set + assertEquals("allowed", action.getFoo()); + assertNull(action.getBar()); + } + + public void testParameterAuthorizerAllowsAllWhenPermissive() throws Exception { + // Same JSON body, but authorizer allows all + this.request.setContent("{\"foo\":\"value1\", \"bar\":\"value2\"}".getBytes()); + this.request.addHeader("Content-Type", "application/json"); + + JSONInterceptor interceptor = createInterceptor(); + TestAction action = new TestAction(); + + this.invocation.setAction(action); + this.invocation.getStack().push(action); + + interceptor.intercept(this.invocation); + + // Both should be set + assertEquals("value1", action.getFoo()); + assertEquals("value2", action.getBar()); + } + public void testMaxElementsEnforcedThroughInterceptor() throws Exception { // JSON object with 5 keys, set maxElements to 3 this.request.setContent("{\"a\":1, \"b\":2, \"c\":3, \"d\":4, \"e\":5}".getBytes()); 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 14280064f0..15464dea43 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 @@ -20,27 +20,55 @@ import org.apache.struts2.ActionInvocation; import org.apache.struts2.ModelDriven; +import org.apache.struts2.StrutsConstants; import org.apache.struts2.inject.Inject; import org.apache.struts2.interceptor.AbstractInterceptor; +import org.apache.struts2.interceptor.parameter.ParameterAuthorizer; import org.apache.struts2.ServletActionContext; import org.apache.struts2.rest.handler.ContentTypeHandler; +import org.apache.commons.lang3.BooleanUtils; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; import jakarta.servlet.http.HttpServletRequest; +import java.beans.BeanInfo; +import java.beans.Introspector; +import java.beans.PropertyDescriptor; import java.io.InputStream; import java.io.InputStreamReader; +import java.lang.reflect.Method; /** - * Uses the content handler to apply the request body to the action + * Uses the content handler to apply the request body to the action. + *

+ * When {@code struts.parameters.requireAnnotations} is enabled, only properties annotated with + * {@link org.apache.struts2.interceptor.parameter.StrutsParameter} will be populated from the request body, + * consistent with the parameter authorization enforced by + * {@link org.apache.struts2.interceptor.parameter.ParametersInterceptor} for form/query parameters. */ public class ContentTypeInterceptor extends AbstractInterceptor { + private static final Logger LOG = LogManager.getLogger(ContentTypeInterceptor.class); + private ContentTypeHandlerManager selector; + private ParameterAuthorizer parameterAuthorizer; + private boolean requireAnnotations = false; @Inject public void setContentTypeHandlerSelector(ContentTypeHandlerManager selector) { this.selector = selector; } + @Inject + public void setParameterAuthorizer(ParameterAuthorizer parameterAuthorizer) { + this.parameterAuthorizer = parameterAuthorizer; + } + + @Inject(value = StrutsConstants.STRUTS_PARAMETERS_REQUIRE_ANNOTATIONS, required = false) + public void setRequireAnnotations(String requireAnnotations) { + this.requireAnnotations = BooleanUtils.toBoolean(requireAnnotations); + } + public String intercept(ActionInvocation invocation) throws Exception { HttpServletRequest request = ServletActionContext.getRequest(); ContentTypeHandler handler = selector.getHandlerForRequest(request); @@ -54,9 +82,50 @@ public String intercept(ActionInvocation invocation) throws Exception { final String encoding = request.getCharacterEncoding(); InputStream is = request.getInputStream(); InputStreamReader reader = encoding == null ? new InputStreamReader(is) : new InputStreamReader(is, encoding); - handler.toObject(invocation, reader, target); + + if (requireAnnotations) { + // Two-phase deserialization: deserialize into a fresh instance, then copy only authorized properties + Object freshInstance; + try { + freshInstance = target.getClass().getDeclaredConstructor().newInstance(); + } catch (ReflectiveOperationException e) { + throw new IllegalStateException( + "Cannot create fresh instance of " + target.getClass().getName() + + " for parameter authorization. REST body deserialization requires a public no-arg constructor" + + " when struts.parameters.requireAnnotations is enabled.", e); + } + + handler.toObject(invocation, reader, freshInstance); + copyAuthorizedProperties(freshInstance, target, invocation.getAction()); + } else { + // Direct deserialization (backward compat when requireAnnotations is not enabled) + handler.toObject(invocation, reader, target); + } } return invocation.invoke(); } + private void copyAuthorizedProperties(Object source, Object target, Object action) throws Exception { + BeanInfo beanInfo = Introspector.getBeanInfo(source.getClass(), Object.class); + for (PropertyDescriptor pd : beanInfo.getPropertyDescriptors()) { + Method readMethod = pd.getReadMethod(); + Method writeMethod = pd.getWriteMethod(); + if (readMethod == null || writeMethod == null) { + continue; + } + + if (!parameterAuthorizer.isAuthorized(pd.getName(), target, action)) { + LOG.warn("REST body parameter [{}] rejected by @StrutsParameter authorization on [{}]", + pd.getName(), target.getClass().getName()); + continue; + } + + Object value = readMethod.invoke(source); + if (value == null) { + continue; // Skip null values to avoid overwriting pre-initialized fields on target + } + writeMethod.invoke(target, value); + } + } + } diff --git a/plugins/rest/src/test/java/org/apache/struts2/rest/ContentTypeInterceptorTest.java b/plugins/rest/src/test/java/org/apache/struts2/rest/ContentTypeInterceptorTest.java index 2232ccb94c..a14d407eef 100644 --- a/plugins/rest/src/test/java/org/apache/struts2/rest/ContentTypeInterceptorTest.java +++ b/plugins/rest/src/test/java/org/apache/struts2/rest/ContentTypeInterceptorTest.java @@ -32,12 +32,14 @@ import org.apache.struts2.dispatcher.mapper.ActionMapping; import org.apache.struts2.rest.handler.ContentTypeHandler; +import org.apache.struts2.interceptor.parameter.ParameterAuthorizer; import org.springframework.mock.web.MockHttpServletRequest; public class ContentTypeInterceptorTest extends TestCase { public void testRequestWithoutEncoding() throws Exception { ContentTypeInterceptor interceptor = new ContentTypeInterceptor(); + interceptor.setParameterAuthorizer((parameterName, target, action) -> true); ActionSupport action = new ActionSupport(); @@ -76,6 +78,7 @@ public void testRequestWithEncodingAscii() throws Exception { final Charset charset = StandardCharsets.US_ASCII; ContentTypeInterceptor interceptor = new ContentTypeInterceptor(); + interceptor.setParameterAuthorizer((parameterName, target, action) -> true); ActionSupport action = new ActionSupport(); @@ -116,6 +119,7 @@ public void testRequestWithEncodingUtf() throws Exception { final Charset charset = StandardCharsets.UTF_8; ContentTypeInterceptor interceptor = new ContentTypeInterceptor(); + interceptor.setParameterAuthorizer((parameterName, target, action) -> true); ActionSupport action = new ActionSupport(); @@ -151,4 +155,82 @@ public boolean matches(Object[] args) { mockActionInvocation.verify(); mockContentTypeHandler.verify(); } + + public void testRequireAnnotationsEnabled_twoPhaseDeserialization() throws Exception { + ContentTypeInterceptor interceptor = new ContentTypeInterceptor(); + interceptor.setParameterAuthorizer((parameterName, target, action) -> false); + interceptor.setRequireAnnotations(Boolean.TRUE.toString()); + + ActionSupport action = new ActionSupport(); + + Mock mockActionInvocation = new Mock(ActionInvocation.class); + Mock mockContentTypeHandler = new Mock(ContentTypeHandler.class); + mockContentTypeHandler.expect("toObject", new AnyConstraintMatcher() { + public boolean matches(Object[] args) { + return true; + } + }); + mockActionInvocation.expectAndReturn("invoke", Action.SUCCESS); + mockActionInvocation.expectAndReturn("getAction", action); + mockActionInvocation.expectAndReturn("getAction", action); + Mock mockContentTypeHandlerManager = new Mock(ContentTypeHandlerManager.class); + mockContentTypeHandlerManager.expectAndReturn("getHandlerForRequest", new AnyConstraintMatcher() { + public boolean matches(Object[] args) { + return true; + } + }, mockContentTypeHandler.proxy()); + interceptor.setContentTypeHandlerSelector((ContentTypeHandlerManager) mockContentTypeHandlerManager.proxy()); + + MockHttpServletRequest request = new MockHttpServletRequest(); + request.setContent(new byte[] {1}); + + ActionContext.of() + .withActionMapping(new ActionMapping()) + .withServletRequest(request) + .bind(); + + interceptor.intercept((ActionInvocation) mockActionInvocation.proxy()); + mockContentTypeHandlerManager.verify(); + mockActionInvocation.verify(); + mockContentTypeHandler.verify(); + } + + public void testRequireAnnotationsEnabled_selectiveFilter() throws Exception { + ContentTypeInterceptor interceptor = new ContentTypeInterceptor(); + interceptor.setParameterAuthorizer((parameterName, target, action) -> "name".equals(parameterName)); + interceptor.setRequireAnnotations(Boolean.TRUE.toString()); + + ActionSupport action = new ActionSupport(); + + Mock mockActionInvocation = new Mock(ActionInvocation.class); + Mock mockContentTypeHandler = new Mock(ContentTypeHandler.class); + mockContentTypeHandler.expect("toObject", new AnyConstraintMatcher() { + public boolean matches(Object[] args) { + return true; + } + }); + mockActionInvocation.expectAndReturn("invoke", Action.SUCCESS); + mockActionInvocation.expectAndReturn("getAction", action); + mockActionInvocation.expectAndReturn("getAction", action); + Mock mockContentTypeHandlerManager = new Mock(ContentTypeHandlerManager.class); + mockContentTypeHandlerManager.expectAndReturn("getHandlerForRequest", new AnyConstraintMatcher() { + public boolean matches(Object[] args) { + return true; + } + }, mockContentTypeHandler.proxy()); + interceptor.setContentTypeHandlerSelector((ContentTypeHandlerManager) mockContentTypeHandlerManager.proxy()); + + MockHttpServletRequest request = new MockHttpServletRequest(); + request.setContent(new byte[] {1}); + + ActionContext.of() + .withActionMapping(new ActionMapping()) + .withServletRequest(request) + .bind(); + + interceptor.intercept((ActionInvocation) mockActionInvocation.proxy()); + mockContentTypeHandlerManager.verify(); + mockActionInvocation.verify(); + mockContentTypeHandler.verify(); + } } From aa78085a6423d6323e8b7422a8b69c8b95877810 Mon Sep 17 00:00:00 2001 From: tranquac Date: Fri, 10 Apr 2026 09:41:15 +0700 Subject: [PATCH 2/5] WW-5624 address review feedback from lukaszlenart on PR #1657 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. Rename DefaultParameterAuthorizer → StrutsParameterAuthorizer per Struts naming convention (inline suggestion) 2. Narrow ModelDriven exemption: require action instanceof ModelDriven before exempting target from @StrutsParameter checks. Prevents non-ModelDriven root objects (e.g. JSONInterceptor.root) from bypassing annotation enforcement. 3. Recursive JSON key filtering: filterUnauthorizedKeys() now recurses into nested Maps and Lists, building dot-notation paths (e.g. "address.city") for path-aware @StrutsParameter(depth=N) checks. 4. Deep REST property copy: copyAuthorizedProperties() now recurses into nested bean types with path-aware authorization. Collections, Maps, primitives, and java.lang/java.time types are copied directly. 5. Null-skip semantics preserved and documented: in two-phase deserialization, null in freshInstance is indistinguishable from "not present in request" — clearing would destroy pre-initialized fields. Kept as intentional design choice with inline documentation. 6. No-arg constructor fallback: when target class lacks a no-arg constructor, falls back to single-phase deserialization with post-scrub of unauthorized properties, preserving backward compat. 7. New regression tests: - Non-ModelDriven target with different object (must not exempt) - Nested JSON keys recursively filtered - Non-action root object still checked by authorizer All 280+ core tests, 124 JSON tests, 76 REST tests pass with 0 regressions. --- .../config/impl/DefaultConfiguration.java | 4 +- ...er.java => StrutsParameterAuthorizer.java} | 13 +- core/src/main/resources/struts-beans.xml | 2 +- .../parameter/ParameterAuthorizerTest.java | 18 ++- .../StrutsParameterAnnotationTest.java | 4 +- .../apache/struts2/json/JSONInterceptor.java | 34 ++++- .../struts2/json/JSONInterceptorTest.java | 61 +++++++++ .../struts2/rest/ContentTypeInterceptor.java | 127 +++++++++++++++--- 8 files changed, 226 insertions(+), 37 deletions(-) rename core/src/main/java/org/apache/struts2/interceptor/parameter/{DefaultParameterAuthorizer.java => StrutsParameterAuthorizer.java} (93%) diff --git a/core/src/main/java/org/apache/struts2/config/impl/DefaultConfiguration.java b/core/src/main/java/org/apache/struts2/config/impl/DefaultConfiguration.java index 4ae87a5c5d..9eb0095928 100644 --- a/core/src/main/java/org/apache/struts2/config/impl/DefaultConfiguration.java +++ b/core/src/main/java/org/apache/struts2/config/impl/DefaultConfiguration.java @@ -92,7 +92,7 @@ import org.apache.struts2.ognl.accessor.CompoundRootAccessor; import org.apache.struts2.ognl.accessor.RootAccessor; import org.apache.struts2.ognl.accessor.XWorkMethodAccessor; -import org.apache.struts2.interceptor.parameter.DefaultParameterAuthorizer; +import org.apache.struts2.interceptor.parameter.StrutsParameterAuthorizer; import org.apache.struts2.interceptor.parameter.ParameterAuthorizer; import org.apache.struts2.util.StrutsProxyService; import org.apache.struts2.util.OgnlTextParser; @@ -408,7 +408,7 @@ public static ContainerBuilder bootstrapFactories(ContainerBuilder builder) { .factory(BeanInfoCacheFactory.class, DefaultOgnlBeanInfoCacheFactory.class, Scope.SINGLETON) .factory(ProxyCacheFactory.class, StrutsProxyCacheFactory.class, Scope.SINGLETON) .factory(ProxyService.class, StrutsProxyService.class, Scope.SINGLETON) - .factory(ParameterAuthorizer.class, DefaultParameterAuthorizer.class, Scope.SINGLETON) + .factory(ParameterAuthorizer.class, StrutsParameterAuthorizer.class, Scope.SINGLETON) .factory(OgnlUtil.class, Scope.SINGLETON) .factory(SecurityMemberAccess.class, Scope.PROTOTYPE) .factory(OgnlGuard.class, StrutsOgnlGuard.class, Scope.SINGLETON) diff --git a/core/src/main/java/org/apache/struts2/interceptor/parameter/DefaultParameterAuthorizer.java b/core/src/main/java/org/apache/struts2/interceptor/parameter/StrutsParameterAuthorizer.java similarity index 93% rename from core/src/main/java/org/apache/struts2/interceptor/parameter/DefaultParameterAuthorizer.java rename to core/src/main/java/org/apache/struts2/interceptor/parameter/StrutsParameterAuthorizer.java index d806664ee1..82e47717e3 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/parameter/DefaultParameterAuthorizer.java +++ b/core/src/main/java/org/apache/struts2/interceptor/parameter/StrutsParameterAuthorizer.java @@ -21,6 +21,7 @@ import org.apache.commons.lang3.BooleanUtils; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; +import org.apache.struts2.ModelDriven; import org.apache.struts2.StrutsConstants; import org.apache.struts2.inject.Inject; import org.apache.struts2.ognl.OgnlUtil; @@ -54,9 +55,9 @@ * * @since 7.2.0 */ -public class DefaultParameterAuthorizer implements ParameterAuthorizer { +public class StrutsParameterAuthorizer implements ParameterAuthorizer { - private static final Logger LOG = LogManager.getLogger(DefaultParameterAuthorizer.class); + private static final Logger LOG = LogManager.getLogger(StrutsParameterAuthorizer.class); private boolean requireAnnotations = false; private boolean requireAnnotationsTransitionMode = false; @@ -102,9 +103,11 @@ public boolean isAuthorized(String parameterName, Object target, Object action) long paramDepth = parameterName.codePoints().mapToObj(c -> (char) c).filter(NESTING_CHARS::contains).count(); - // ModelDriven exemption: when target is the model (target != action), exempt from annotation requirements - if (target != action) { - LOG.debug("Model driven target detected, exempting from @StrutsParameter annotation requirement"); + // ModelDriven exemption: only exempt when the action explicitly implements ModelDriven + // and the target is its model object. This prevents non-ModelDriven root objects + // (e.g. JSONInterceptor's configurable rootObject) from bypassing annotation checks. + if (target != action && action instanceof ModelDriven) { + LOG.debug("ModelDriven target detected (action implements ModelDriven), exempting from @StrutsParameter annotation requirement"); return true; } diff --git a/core/src/main/resources/struts-beans.xml b/core/src/main/resources/struts-beans.xml index 030acbc2b4..232f0f4a42 100644 --- a/core/src/main/resources/struts-beans.xml +++ b/core/src/main/resources/struts-beans.xml @@ -246,7 +246,7 @@ class="org.apache.struts2.util.StrutsProxyService" scope="singleton"/> + class="org.apache.struts2.interceptor.parameter.StrutsParameterAuthorizer" scope="singleton"/> diff --git a/core/src/test/java/org/apache/struts2/interceptor/parameter/ParameterAuthorizerTest.java b/core/src/test/java/org/apache/struts2/interceptor/parameter/ParameterAuthorizerTest.java index 1b3cad5441..f7c16fbb92 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/parameter/ParameterAuthorizerTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/parameter/ParameterAuthorizerTest.java @@ -34,16 +34,16 @@ import static org.assertj.core.api.Assertions.assertThat; /** - * Tests for {@link DefaultParameterAuthorizer} — verifies that the extracted authorization logic works correctly + * Tests for {@link StrutsParameterAuthorizer} — verifies that the extracted authorization logic works correctly * without any OGNL ThreadAllowlist side effects. */ public class ParameterAuthorizerTest { - private DefaultParameterAuthorizer authorizer; + private StrutsParameterAuthorizer authorizer; @Before public void setUp() { - authorizer = new DefaultParameterAuthorizer(); + authorizer = new StrutsParameterAuthorizer(); authorizer.setRequireAnnotations(Boolean.TRUE.toString()); var ognlUtil = new OgnlUtil( @@ -119,11 +119,21 @@ public void unannotatedPublicField_rejected() { public void modelDriven_targetIsModel_allAuthorized() { var action = new ModelAction(); var model = action.getModel(); - // target != action → model is exempt + // target != action AND action instanceof ModelDriven → model is exempt assertThat(authorizer.isAuthorized("anyProperty", model, action)).isTrue(); assertThat(authorizer.isAuthorized("nested.deep", model, action)).isTrue(); } + @Test + public void nonModelDrivenAction_differentTarget_notExempt() { + // Regression test: when target != action but action does NOT implement ModelDriven, + // the target should NOT be exempt from annotation checks. + var action = new SecureAction(); + var nonActionTarget = new Pojo(); // different object, but action is not ModelDriven + // Pojo has no @StrutsParameter annotations, so this should be rejected + assertThat(authorizer.isAuthorized("name", nonActionTarget, action)).isFalse(); + } + // --- Transition mode --- @Test diff --git a/core/src/test/java/org/apache/struts2/interceptor/parameter/StrutsParameterAnnotationTest.java b/core/src/test/java/org/apache/struts2/interceptor/parameter/StrutsParameterAnnotationTest.java index 4ab8f5b5de..803e0dad5a 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/parameter/StrutsParameterAnnotationTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/parameter/StrutsParameterAnnotationTest.java @@ -56,7 +56,7 @@ public class StrutsParameterAnnotationTest { private ParametersInterceptor parametersInterceptor; - private DefaultParameterAuthorizer parameterAuthorizer; + private StrutsParameterAuthorizer parameterAuthorizer; private ThreadAllowlist threadAllowlist; @@ -77,7 +77,7 @@ public void setUp() throws Exception { var proxyService = new StrutsProxyService(new StrutsProxyCacheFactory<>("1000", "basic")); parametersInterceptor.setProxyService(proxyService); - var parameterAuthorizer = new DefaultParameterAuthorizer(); + var parameterAuthorizer = new StrutsParameterAuthorizer(); parameterAuthorizer.setOgnlUtil(ognlUtil); parameterAuthorizer.setProxyService(proxyService); parameterAuthorizer.setRequireAnnotations(Boolean.TRUE.toString()); diff --git a/plugins/json/src/main/java/org/apache/struts2/json/JSONInterceptor.java b/plugins/json/src/main/java/org/apache/struts2/json/JSONInterceptor.java index 8085230f79..e07b011512 100644 --- a/plugins/json/src/main/java/org/apache/struts2/json/JSONInterceptor.java +++ b/plugins/json/src/main/java/org/apache/struts2/json/JSONInterceptor.java @@ -207,13 +207,39 @@ private void applyLimitsToReader() { @SuppressWarnings("rawtypes") private void filterUnauthorizedKeys(Map json, Object target, Object action) { - Iterator it = json.keySet().iterator(); + filterUnauthorizedKeysRecursive(json, "", target, action); + } + + @SuppressWarnings("rawtypes") + private void filterUnauthorizedKeysRecursive(Map json, String prefix, Object target, Object action) { + Iterator it = json.entrySet().iterator(); while (it.hasNext()) { - String key = it.next(); - if (!parameterAuthorizer.isAuthorized(key, target, action)) { + Map.Entry entry = it.next(); + String key = (String) entry.getKey(); + String fullPath = prefix.isEmpty() ? key : prefix + "." + key; + + if (!parameterAuthorizer.isAuthorized(fullPath, target, action)) { LOG.warn("JSON body parameter [{}] rejected by @StrutsParameter authorization on [{}]", - key, target.getClass().getName()); + fullPath, target.getClass().getName()); it.remove(); + continue; + } + + // Recurse into nested Maps (JSON objects) to enforce depth-aware authorization + Object value = entry.getValue(); + if (value instanceof Map) { + filterUnauthorizedKeysRecursive((Map) value, fullPath, target, action); + } else if (value instanceof java.util.List) { + filterUnauthorizedList((java.util.List) value, fullPath, target, action); + } + } + } + + @SuppressWarnings("rawtypes") + private void filterUnauthorizedList(java.util.List list, String prefix, Object target, Object action) { + for (Object item : list) { + if (item instanceof Map) { + filterUnauthorizedKeysRecursive((Map) item, prefix, target, action); } } } diff --git a/plugins/json/src/test/java/org/apache/struts2/json/JSONInterceptorTest.java b/plugins/json/src/test/java/org/apache/struts2/json/JSONInterceptorTest.java index e263075c33..2203f6f290 100644 --- a/plugins/json/src/test/java/org/apache/struts2/json/JSONInterceptorTest.java +++ b/plugins/json/src/test/java/org/apache/struts2/json/JSONInterceptorTest.java @@ -620,6 +620,67 @@ public void testMaxElementsEnforcedThroughInterceptor() throws Exception { } } + /** + * Tests that nested JSON keys are recursively checked by the parameter authorizer. + * Regression test for lukaszlenart's review: nested @StrutsParameter(depth=N) enforcement. + */ + public void testNestedJsonKeysRecursivelyFiltered() throws Exception { + // JSON body with nested object: {"bean": {"stringField": "test", "intField": 42}} + this.request.setContent("{\"bean\": {\"stringField\": \"test\", \"intField\": 42}}".getBytes()); + this.request.addHeader("Content-Type", "application/json"); + + JSONInterceptor interceptor = new JSONInterceptor(); + JSONUtil jsonUtil = new JSONUtil(); + jsonUtil.setReader(new StrutsJSONReader()); + jsonUtil.setWriter(new StrutsJSONWriter()); + interceptor.setJsonUtil(jsonUtil); + // Authorize "bean" (top-level) and "bean.stringField" (nested) but reject "bean.intField" + interceptor.setParameterAuthorizer((parameterName, target, action) -> + "bean".equals(parameterName) || "bean.stringField".equals(parameterName)); + TestAction action = new TestAction(); + + this.invocation.setAction(action); + this.invocation.getStack().push(action); + + interceptor.intercept(this.invocation); + + // bean should exist with stringField set, but intField should be default (0) + assertNotNull(action.getBean()); + assertEquals("test", action.getBean().getStringField()); + assertEquals(0, action.getBean().getIntField()); + } + + /** + * Tests that when root resolves to a non-action object (not ModelDriven), + * annotation checks are still enforced. + * Regression test for lukaszlenart's review: non-action root bypass. + */ + public void testNonActionRootObjectStillChecked() throws Exception { + this.request.setContent("{\"stringField\":\"injected\", \"intField\":99}".getBytes()); + this.request.addHeader("Content-Type", "application/json"); + + JSONInterceptor interceptor = new JSONInterceptor(); + JSONUtil jsonUtil = new JSONUtil(); + jsonUtil.setReader(new StrutsJSONReader()); + jsonUtil.setWriter(new StrutsJSONWriter()); + interceptor.setJsonUtil(jsonUtil); + interceptor.setRoot("bean"); + // Reject all parameters — simulates strict requireAnnotations + interceptor.setParameterAuthorizer((parameterName, target, action) -> false); + TestAction4 action = new TestAction4(); + + this.invocation.setAction(action); + this.invocation.getStack().push(action); + + interceptor.intercept(this.invocation); + + // Both fields should remain at defaults since authorizer rejected everything + Bean bean = action.getBean(); + assertNotNull(bean); + assertNull(bean.getStringField()); + assertEquals(0, bean.getIntField()); + } + @Override protected void setUp() throws Exception { super.setUp(); 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 15464dea43..1a215b46b6 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 @@ -37,6 +37,8 @@ import java.io.InputStream; import java.io.InputStreamReader; import java.lang.reflect.Method; +import java.util.Collection; +import java.util.Map; /** * Uses the content handler to apply the request body to the action. @@ -84,19 +86,18 @@ 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 - Object freshInstance; - try { - freshInstance = target.getClass().getDeclaredConstructor().newInstance(); - } catch (ReflectiveOperationException e) { - throw new IllegalStateException( - "Cannot create fresh instance of " + target.getClass().getName() - + " for parameter authorization. REST body deserialization requires a public no-arg constructor" - + " when struts.parameters.requireAnnotations is enabled.", e); + // Two-phase deserialization: deserialize into a fresh instance, then copy only authorized properties. + // This requires a public no-arg constructor on the target class. If absent, fall back to + // single-phase deserialization with post-deserialization scrubbing of unauthorized properties. + Object freshInstance = createFreshInstance(target.getClass()); + if (freshInstance != null) { + handler.toObject(invocation, reader, freshInstance); + copyAuthorizedProperties(freshInstance, target, invocation.getAction(), ""); + } else { + LOG.warn("No no-arg constructor for [{}], using single-phase deserialization with post-scrub", target.getClass().getName()); + handler.toObject(invocation, reader, target); + scrubUnauthorizedProperties(target, invocation.getAction()); } - - handler.toObject(invocation, reader, freshInstance); - copyAuthorizedProperties(freshInstance, target, invocation.getAction()); } else { // Direct deserialization (backward compat when requireAnnotations is not enabled) handler.toObject(invocation, reader, target); @@ -105,7 +106,20 @@ public String intercept(ActionInvocation invocation) throws Exception { return invocation.invoke(); } - private void copyAuthorizedProperties(Object source, Object target, Object action) throws Exception { + private Object createFreshInstance(Class clazz) { + try { + return clazz.getDeclaredConstructor().newInstance(); + } catch (ReflectiveOperationException e) { + LOG.debug("Cannot create fresh instance of [{}] via no-arg constructor: {}", clazz.getName(), e.getMessage()); + return null; + } + } + + /** + * Recursively copies only authorized properties from source to target, enforcing {@code @StrutsParameter} + * depth semantics for nested object graphs. + */ + private void copyAuthorizedProperties(Object source, Object target, Object action, String prefix) throws Exception { BeanInfo beanInfo = Introspector.getBeanInfo(source.getClass(), Object.class); for (PropertyDescriptor pd : beanInfo.getPropertyDescriptors()) { Method readMethod = pd.getReadMethod(); @@ -114,18 +128,93 @@ private void copyAuthorizedProperties(Object source, Object target, Object actio continue; } - if (!parameterAuthorizer.isAuthorized(pd.getName(), target, action)) { + String fullPath = prefix.isEmpty() ? pd.getName() : prefix + "." + pd.getName(); + + if (!parameterAuthorizer.isAuthorized(fullPath, target, action)) { LOG.warn("REST body parameter [{}] rejected by @StrutsParameter authorization on [{}]", - pd.getName(), target.getClass().getName()); + fullPath, target.getClass().getName()); + continue; + } + + Object sourceValue = readMethod.invoke(source); + if (sourceValue == null) { + // Intentionally skip null values: in two-phase deserialization, properties NOT present in the + // request body will be null in the fresh instance. Copying null would clear pre-initialized + // fields on the target. This is the safer default — an explicit JSON null and a missing field + // are indistinguishable after deserialization into a fresh POJO. continue; } - Object value = readMethod.invoke(source); - if (value == null) { - continue; // Skip null values to avoid overwriting pre-initialized fields on target + // For complex bean types (not primitives, strings, collections, etc.), recurse to enforce + // nested authorization. Collections/Maps/arrays are copied as-is since their contents were + // already deserialized and the depth check on the parent property covers them. + if (isNestedBeanType(sourceValue.getClass())) { + Object targetValue = readMethod.invoke(target); + if (targetValue == null) { + Object newTarget = createFreshInstance(sourceValue.getClass()); + if (newTarget != null) { + writeMethod.invoke(target, newTarget); + targetValue = newTarget; + } else { + // Cannot recurse without a fresh target — copy whole value + writeMethod.invoke(target, sourceValue); + continue; + } + } + copyAuthorizedProperties(sourceValue, targetValue, action, fullPath); + } else { + writeMethod.invoke(target, sourceValue); } - writeMethod.invoke(target, value); } } + /** + * Fallback for actions without a no-arg constructor: scrub unauthorized properties after direct deserialization. + */ + private void scrubUnauthorizedProperties(Object target, Object action) throws Exception { + BeanInfo beanInfo = Introspector.getBeanInfo(target.getClass(), Object.class); + for (PropertyDescriptor pd : beanInfo.getPropertyDescriptors()) { + Method readMethod = pd.getReadMethod(); + Method writeMethod = pd.getWriteMethod(); + if (readMethod == null || writeMethod == null) { + continue; + } + + if (!parameterAuthorizer.isAuthorized(pd.getName(), target, action)) { + Object value = readMethod.invoke(target); + if (value != null) { + try { + writeMethod.invoke(target, (Object) null); + } catch (IllegalArgumentException e) { + // Primitive type — cannot null, set to default + LOG.debug("Cannot null primitive property [{}], skipping scrub", pd.getName()); + } + } + } + } + } + + /** + * Determines whether a class represents a nested bean that should be recursively authorized, + * as opposed to simple/leaf types that are copied directly. + */ + private boolean isNestedBeanType(Class clazz) { + if (clazz.isPrimitive() || clazz.isEnum() || clazz.isArray()) { + return false; + } + if (clazz.getName().startsWith("java.lang.") || clazz.getName().startsWith("java.math.")) { + return false; + } + if (java.util.Date.class.isAssignableFrom(clazz)) { + return false; + } + if (java.time.temporal.Temporal.class.isAssignableFrom(clazz)) { + return false; + } + if (Collection.class.isAssignableFrom(clazz) || Map.class.isAssignableFrom(clazz)) { + return false; + } + return true; + } + } From 9490dfab45163a5dfc9d9c4253657a0af1d15027 Mon Sep 17 00:00:00 2001 From: tranquac Date: Sat, 11 Apr 2026 07:52:14 +0700 Subject: [PATCH 3/5] =?UTF-8?q?WW-5624:=20v3=20=E2=80=94=20fix=20indexed-p?= =?UTF-8?q?ath=20depth=20parity=20with=20ParametersInterceptor?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four gaps identified by lukaszlenart's April 10 review are now fully addressed: 1. JSON filterUnauthorizedList: pass prefix+"[0]" instead of bare prefix so that list element properties gain one extra '[' in their path — e.g. "publicPojoListDepthOne[0].key" (depth=2) is now correctly rejected when @StrutsParameter(depth=1), matching ParametersInterceptor semantics. Also recurse into nested List> via an else-if branch. 2. REST copyAuthorizedProperties: add authTarget parameter (always = root action/model, passed unchanged through all recursion levels). isAuthorized() now checks the full path against the root class, so "address.city" is looked up on the action, not on the Address object. 3. REST Collection/Map/array deep authorization: replaced the as-is copy with deepCopyAuthorizedCollection(), deepCopyAuthorizedMap(), and deepCopyAuthorizedArray() helpers — each iterates elements with path+"[0]" prefix, authorizing every complex element individually. No-arg fallback skips the element rather than copying an unfiltered object graph (security fix over plan's original as-is suggestion). 4. REST scrubUnauthorizedProperties: now fully recursive via scrubUnauthorizedPropertiesRecursive() — visits nested beans, collection elements, and map values with authTarget always pointing to the root. Includes identity-based visited-set to guard against circular reference cycles. Tests: core 2920 + json 124 + rest 76 = 3120, 0 failures. Co-Authored-By: Claude Sonnet 4.6 --- .../apache/struts2/json/JSONInterceptor.java | 8 +- .../struts2/rest/ContentTypeInterceptor.java | 175 ++++++++++++++++-- 2 files changed, 168 insertions(+), 15 deletions(-) diff --git a/plugins/json/src/main/java/org/apache/struts2/json/JSONInterceptor.java b/plugins/json/src/main/java/org/apache/struts2/json/JSONInterceptor.java index e07b011512..a0279f2f09 100644 --- a/plugins/json/src/main/java/org/apache/struts2/json/JSONInterceptor.java +++ b/plugins/json/src/main/java/org/apache/struts2/json/JSONInterceptor.java @@ -237,9 +237,15 @@ private void filterUnauthorizedKeysRecursive(Map json, String prefix, Object tar @SuppressWarnings("rawtypes") private void filterUnauthorizedList(java.util.List list, String prefix, Object target, Object action) { + // Use prefix+"[0]" so that list element properties pick up one extra '[' in their path, + // matching the indexed-path semantics of ParametersInterceptor (e.g. "items[0].key" → depth 2). + String elementPrefix = prefix + "[0]"; for (Object item : list) { if (item instanceof Map) { - filterUnauthorizedKeysRecursive((Map) item, prefix, target, action); + filterUnauthorizedKeysRecursive((Map) item, elementPrefix, target, action); + } else if (item instanceof java.util.List) { + // Handle nested lists (e.g. List>) by recursing with the same elementPrefix + filterUnauthorizedList((java.util.List) item, elementPrefix, target, action); } } } 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 1a215b46b6..9d8b941c42 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 @@ -36,9 +36,15 @@ import java.beans.PropertyDescriptor; import java.io.InputStream; import java.io.InputStreamReader; +import java.lang.reflect.Array; import java.lang.reflect.Method; +import java.util.ArrayList; import java.util.Collection; +import java.util.HashSet; +import java.util.LinkedHashMap; +import java.util.List; import java.util.Map; +import java.util.Set; /** * Uses the content handler to apply the request body to the action. @@ -92,7 +98,7 @@ public String intercept(ActionInvocation invocation) throws Exception { Object freshInstance = createFreshInstance(target.getClass()); if (freshInstance != null) { handler.toObject(invocation, reader, freshInstance); - copyAuthorizedProperties(freshInstance, target, invocation.getAction(), ""); + copyAuthorizedProperties(freshInstance, target, invocation.getAction(), target, ""); } else { LOG.warn("No no-arg constructor for [{}], using single-phase deserialization with post-scrub", target.getClass().getName()); handler.toObject(invocation, reader, target); @@ -116,10 +122,15 @@ private Object createFreshInstance(Class clazz) { } /** - * Recursively copies only authorized properties from source to target, enforcing {@code @StrutsParameter} - * depth semantics for nested object graphs. + * Recursively copies only authorized properties from {@code source} to {@code target}, + * enforcing {@code @StrutsParameter} depth semantics for nested object graphs. + * + *

{@code authTarget} is always the root action/model passed unchanged through all levels. + * {@code isAuthorized} uses the full dot/bracket path against the root class, so the root + * target must be used — not the nested object being visited at the current recursion depth. */ - private void copyAuthorizedProperties(Object source, Object target, Object action, String prefix) throws Exception { + private void copyAuthorizedProperties( + Object source, Object target, Object action, Object authTarget, String prefix) throws Exception { BeanInfo beanInfo = Introspector.getBeanInfo(source.getClass(), Object.class); for (PropertyDescriptor pd : beanInfo.getPropertyDescriptors()) { Method readMethod = pd.getReadMethod(); @@ -130,9 +141,10 @@ private void copyAuthorizedProperties(Object source, Object target, Object actio String fullPath = prefix.isEmpty() ? pd.getName() : prefix + "." + pd.getName(); - if (!parameterAuthorizer.isAuthorized(fullPath, target, action)) { + // Always check against authTarget (root action/model), never the nested object being traversed + if (!parameterAuthorizer.isAuthorized(fullPath, authTarget, action)) { LOG.warn("REST body parameter [{}] rejected by @StrutsParameter authorization on [{}]", - fullPath, target.getClass().getName()); + fullPath, authTarget.getClass().getName()); continue; } @@ -145,10 +157,8 @@ private void copyAuthorizedProperties(Object source, Object target, Object actio continue; } - // For complex bean types (not primitives, strings, collections, etc.), recurse to enforce - // nested authorization. Collections/Maps/arrays are copied as-is since their contents were - // already deserialized and the depth check on the parent property covers them. if (isNestedBeanType(sourceValue.getClass())) { + // Complex bean: recurse to authorize nested fields, passing authTarget unchanged Object targetValue = readMethod.invoke(target); if (targetValue == null) { Object newTarget = createFreshInstance(sourceValue.getClass()); @@ -161,17 +171,133 @@ private void copyAuthorizedProperties(Object source, Object target, Object actio continue; } } - copyAuthorizedProperties(sourceValue, targetValue, action, fullPath); + copyAuthorizedProperties(sourceValue, targetValue, action, authTarget, fullPath); + } else if (sourceValue instanceof Collection) { + writeMethod.invoke(target, + deepCopyAuthorizedCollection((Collection) sourceValue, fullPath, authTarget, action)); + } else if (sourceValue instanceof Map) { + writeMethod.invoke(target, + deepCopyAuthorizedMap((Map) sourceValue, fullPath, authTarget, action)); + } else if (sourceValue.getClass().isArray()) { + writeMethod.invoke(target, + deepCopyAuthorizedArray(sourceValue, fullPath, authTarget, action)); } else { writeMethod.invoke(target, sourceValue); } } } + /** + * Authorizes each complex element of a collection using indexed-path semantics ({@code path[0].field}), + * matching {@code ParametersInterceptor} depth counting. Scalar elements are copied directly. + * Elements whose class has no no-arg constructor are skipped to avoid copying an unfiltered object graph. + */ + @SuppressWarnings({"unchecked", "rawtypes"}) + private Collection deepCopyAuthorizedCollection( + Collection source, String collectionPath, Object authTarget, Object action) throws Exception { + List result = new ArrayList(); + for (Object element : source) { + if (element != null && isNestedBeanType(element.getClass())) { + String elementPath = collectionPath + "[0]"; + if (!parameterAuthorizer.isAuthorized(elementPath, authTarget, action)) { + LOG.warn("REST collection element [{}] rejected by @StrutsParameter authorization", elementPath); + continue; + } + Object newElement = createFreshInstance(element.getClass()); + if (newElement != null) { + copyAuthorizedProperties(element, newElement, action, authTarget, elementPath); + result.add(newElement); + } else { + // No no-arg constructor: skip element rather than copy an unfiltered object graph + LOG.warn("REST collection element [{}] skipped — no no-arg constructor for [{}]", + elementPath, element.getClass().getName()); + } + } else { + result.add(element); + } + } + return result; + } + + /** + * Authorizes each complex map value using indexed-path semantics ({@code path[0]}), + * consistent with OGNL bracket notation depth counting. Scalar values are copied directly. + */ + @SuppressWarnings({"unchecked", "rawtypes"}) + private Map deepCopyAuthorizedMap( + Map source, String mapPath, Object authTarget, Object action) throws Exception { + Map result = new LinkedHashMap(); + for (Map.Entry entry : source.entrySet()) { + Object value = entry.getValue(); + if (value != null && isNestedBeanType(value.getClass())) { + String valuePath = mapPath + "[0]"; + if (!parameterAuthorizer.isAuthorized(valuePath, authTarget, action)) { + LOG.warn("REST map value [{}] rejected by @StrutsParameter authorization", valuePath); + continue; + } + Object newValue = createFreshInstance(value.getClass()); + if (newValue != null) { + copyAuthorizedProperties(value, newValue, action, authTarget, valuePath); + result.put(entry.getKey(), newValue); + } else { + LOG.warn("REST map value [{}] skipped — no no-arg constructor for [{}]", + valuePath, value.getClass().getName()); + } + } else { + result.put(entry.getKey(), value); + } + } + return result; + } + + /** + * Authorizes each complex element of an array ({@code Pojo[]}) using indexed-path semantics, + * matching {@code ParametersInterceptor} depth counting. Scalar elements are copied directly. + */ + private Object deepCopyAuthorizedArray( + Object sourceArray, String arrayPath, Object authTarget, Object action) throws Exception { + int length = Array.getLength(sourceArray); + Class componentType = sourceArray.getClass().getComponentType(); + Object result = Array.newInstance(componentType, length); + for (int i = 0; i < length; i++) { + Object element = Array.get(sourceArray, i); + if (element != null && isNestedBeanType(element.getClass())) { + String elementPath = arrayPath + "[0]"; + if (!parameterAuthorizer.isAuthorized(elementPath, authTarget, action)) { + LOG.warn("REST array element [{}] rejected by @StrutsParameter authorization", elementPath); + continue; + } + Object newElement = createFreshInstance(element.getClass()); + if (newElement != null) { + copyAuthorizedProperties(element, newElement, action, authTarget, elementPath); + Array.set(result, i, newElement); + } else { + LOG.warn("REST array element [{}] skipped — no no-arg constructor for [{}]", + elementPath, element.getClass().getName()); + } + } else { + Array.set(result, i, element); + } + } + return result; + } + /** * Fallback for actions without a no-arg constructor: scrub unauthorized properties after direct deserialization. + * Recurses into nested beans, collection elements, and map values for full parity with + * {@code copyAuthorizedProperties} depth semantics. */ private void scrubUnauthorizedProperties(Object target, Object action) throws Exception { + scrubUnauthorizedPropertiesRecursive(target, action, target, "", new HashSet<>()); + } + + private void scrubUnauthorizedPropertiesRecursive( + Object target, Object action, Object authTarget, String prefix, + Set visited) throws Exception { + // Guard against circular references in the object graph + if (!visited.add(System.identityHashCode(target))) { + return; + } BeanInfo beanInfo = Introspector.getBeanInfo(target.getClass(), Object.class); for (PropertyDescriptor pd : beanInfo.getPropertyDescriptors()) { Method readMethod = pd.getReadMethod(); @@ -180,23 +306,44 @@ private void scrubUnauthorizedProperties(Object target, Object action) throws Ex continue; } - if (!parameterAuthorizer.isAuthorized(pd.getName(), target, action)) { - Object value = readMethod.invoke(target); + String fullPath = prefix.isEmpty() ? pd.getName() : prefix + "." + pd.getName(); + Object value = readMethod.invoke(target); + + if (!parameterAuthorizer.isAuthorized(fullPath, authTarget, action)) { if (value != null) { try { writeMethod.invoke(target, (Object) null); } catch (IllegalArgumentException e) { - // Primitive type — cannot null, set to default LOG.debug("Cannot null primitive property [{}], skipping scrub", pd.getName()); } } + } else if (value != null) { + // Authorized — recurse into complex types to scrub any nested unauthorized fields + if (isNestedBeanType(value.getClass())) { + scrubUnauthorizedPropertiesRecursive(value, action, authTarget, fullPath, visited); + } else if (value instanceof Collection) { + for (Object element : (Collection) value) { + if (element != null && isNestedBeanType(element.getClass())) { + scrubUnauthorizedPropertiesRecursive( + element, action, authTarget, fullPath + "[0]", visited); + } + } + } else if (value instanceof Map) { + for (Object mapValue : ((Map) value).values()) { + if (mapValue != null && isNestedBeanType(mapValue.getClass())) { + scrubUnauthorizedPropertiesRecursive( + mapValue, action, authTarget, fullPath + "[0]", visited); + } + } + } } } } /** * Determines whether a class represents a nested bean that should be recursively authorized, - * as opposed to simple/leaf types that are copied directly. + * as opposed to simple/leaf types (primitives, strings, collections, maps, arrays, enums) that + * are handled directly. */ private boolean isNestedBeanType(Class clazz) { if (clazz.isPrimitive() || clazz.isEnum() || clazz.isArray()) { From df533b3bb44bd13d0d65bb13dfb3efe5e2bb4bf5 Mon Sep 17 00:00:00 2001 From: tranquac Date: Sat, 11 Apr 2026 08:05:23 +0700 Subject: [PATCH 4/5] =?UTF-8?q?WW-5624:=20v3.1=20=E2=80=94=20fix=20collect?= =?UTF-8?q?ion=20type,=20identity=20set,=20isNestedBeanType=20coverage?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three correctness/security issues identified by independent review: 1. deepCopyAuthorizedCollection/deepCopyAuthorizedMap type preservation: Previously always returned ArrayList/LinkedHashMap. If the action field is typed Set or SortedMap, writeMethod.invoke would throw IllegalArgumentException. Now: SortedSet→TreeSet, Set→LinkedHashSet, List→ArrayList; SortedMap→TreeMap, Map→LinkedHashMap. 2. scrubUnauthorizedPropertiesRecursive visited-set identity safety: Replaced Set+System.identityHashCode (not collision-safe) with Collections.newSetFromMap(new IdentityHashMap<>()) which uses reference equality (==). A hash collision could have caused a valid nested object to be skipped, leaving unauthorized properties un-scrubbed. 3. isNestedBeanType now excludes all standard-library leaf packages: java.util.* non-Collection/Map types (UUID, Currency, Locale, Date), java.time.* (all temporal types, not just Temporal subinterface), java.net.*, java.io.*, java.nio.*. Previously UUID etc. would return true, causing the code to recurse into their internal fields and silently drop the value when no @StrutsParameter annotation matched. Tests: json 124 + rest 76 = 200, 0 failures. Co-Authored-By: Claude Sonnet 4.6 --- .../struts2/rest/ContentTypeInterceptor.java | 52 ++++++++++++++++--- 1 file changed, 44 insertions(+), 8 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 9d8b941c42..6c8247ded4 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 @@ -40,11 +40,17 @@ import java.lang.reflect.Method; import java.util.ArrayList; import java.util.Collection; -import java.util.HashSet; +import java.util.Collections; +import java.util.IdentityHashMap; import java.util.LinkedHashMap; +import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.SortedMap; +import java.util.SortedSet; +import java.util.TreeMap; +import java.util.TreeSet; /** * Uses the content handler to apply the request body to the action. @@ -195,7 +201,16 @@ private void copyAuthorizedProperties( @SuppressWarnings({"unchecked", "rawtypes"}) private Collection deepCopyAuthorizedCollection( Collection source, String collectionPath, Object authTarget, Object action) throws Exception { - List result = new ArrayList(); + // Preserve the collection type so that writeMethod.invoke does not fail when the setter + // parameter is typed as Set, SortedSet, etc. Fall back to ArrayList for unrecognised types. + Collection result; + if (source instanceof SortedSet) { + result = new TreeSet(((SortedSet) source).comparator()); + } else if (source instanceof Set) { + result = new LinkedHashSet(); + } else { + result = new ArrayList(); + } for (Object element : source) { if (element != null && isNestedBeanType(element.getClass())) { String elementPath = collectionPath + "[0]"; @@ -226,7 +241,14 @@ private Collection deepCopyAuthorizedCollection( @SuppressWarnings({"unchecked", "rawtypes"}) private Map deepCopyAuthorizedMap( Map source, String mapPath, Object authTarget, Object action) throws Exception { - Map result = new LinkedHashMap(); + // Preserve the map type so that writeMethod.invoke does not fail when the setter + // parameter is typed as SortedMap, TreeMap, etc. + Map result; + if (source instanceof SortedMap) { + result = new TreeMap(((SortedMap) source).comparator()); + } else { + result = new LinkedHashMap(); + } for (Map.Entry entry : source.entrySet()) { Object value = entry.getValue(); if (value != null && isNestedBeanType(value.getClass())) { @@ -288,14 +310,17 @@ private Object deepCopyAuthorizedArray( * {@code copyAuthorizedProperties} depth semantics. */ private void scrubUnauthorizedProperties(Object target, Object action) throws Exception { - scrubUnauthorizedPropertiesRecursive(target, action, target, "", new HashSet<>()); + // Use an identity-based Set to guard against circular references. + // System.identityHashCode is not guaranteed unique; IdentityHashMap uses reference equality (==). + scrubUnauthorizedPropertiesRecursive(target, action, target, "", + Collections.newSetFromMap(new IdentityHashMap<>())); } private void scrubUnauthorizedPropertiesRecursive( Object target, Object action, Object authTarget, String prefix, - Set visited) throws Exception { - // Guard against circular references in the object graph - if (!visited.add(System.identityHashCode(target))) { + Set visited) throws Exception { + // Guard against circular references in the object graph (identity equality, not equals()) + if (!visited.add(target)) { return; } BeanInfo beanInfo = Introspector.getBeanInfo(target.getClass(), Object.class); @@ -349,15 +374,26 @@ private boolean isNestedBeanType(Class clazz) { if (clazz.isPrimitive() || clazz.isEnum() || clazz.isArray()) { return false; } + // Exclude standard library value/leaf types that have no meaningful bean properties to recurse into. + // java.lang.*, java.math.* — primitives, String, Number subclasses, etc. + // java.util.* leaf types — UUID, Currency, Locale, Date, etc. (NOT Collection/Map which are handled separately) if (clazz.getName().startsWith("java.lang.") || clazz.getName().startsWith("java.math.")) { return false; } - if (java.util.Date.class.isAssignableFrom(clazz)) { + if (clazz.getName().startsWith("java.util.") && !Collection.class.isAssignableFrom(clazz) + && !Map.class.isAssignableFrom(clazz)) { return false; } if (java.time.temporal.Temporal.class.isAssignableFrom(clazz)) { return false; } + if (clazz.getName().startsWith("java.time.")) { + return false; + } + if (clazz.getName().startsWith("java.net.") || clazz.getName().startsWith("java.io.") + || clazz.getName().startsWith("java.nio.")) { + return false; + } if (Collection.class.isAssignableFrom(clazz) || Map.class.isAssignableFrom(clazz)) { return false; } From 72cc28210397ec59bfe3209193c6aad5cff5dc1a Mon Sep 17 00:00:00 2001 From: tranquac Date: Sat, 11 Apr 2026 17:57:04 +0700 Subject: [PATCH 5/5] =?UTF-8?q?WW-5624:=20v4=20=E2=80=94=20close=20bulk-co?= =?UTF-8?q?py=20fallback,=20reject=20body=20when=20no=20no-arg=20ctor?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two remaining gaps addressed per lukaszlenart's April 11 review: 1. copyAuthorizedProperties bulk-copy fallback removed: When a nested target bean is null and createFreshInstance fails (no no-arg constructor), the previous code fell back to writeMethod.invoke(target, sourceValue) — copying the whole nested object graph without per-path authorization. Now logs a warning and skips the property entirely (same policy as deepCopyAuthorizedCollection elements with no no-arg constructor). 2. Top-level no-arg constructor fallback changed from scrub to reject: When requireAnnotations=true and the target class has no no-arg constructor, body deserialization is now rejected entirely (handler.toObject is never called). The previous best-effort scrub path could not guarantee that all nested unauthorized properties were nulled out. scrubUnauthorizedProperties and its recursive helper are removed as dead code. Tests: rest 76, 0 failures. --- .../struts2/rest/ContentTypeInterceptor.java | 82 +++---------------- 1 file changed, 12 insertions(+), 70 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 6c8247ded4..73f3cd7ef3 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 @@ -40,8 +40,6 @@ import java.lang.reflect.Method; import java.util.ArrayList; import java.util.Collection; -import java.util.Collections; -import java.util.IdentityHashMap; import java.util.LinkedHashMap; import java.util.LinkedHashSet; import java.util.List; @@ -99,16 +97,17 @@ public String intercept(ActionInvocation invocation) throws Exception { if (requireAnnotations) { // Two-phase deserialization: deserialize into a fresh instance, then copy only authorized properties. - // This requires a public no-arg constructor on the target class. If absent, fall back to - // single-phase deserialization with post-deserialization scrubbing of unauthorized 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("No no-arg constructor for [{}], using single-phase deserialization with post-scrub", target.getClass().getName()); - handler.toObject(invocation, reader, target); - scrubUnauthorizedProperties(target, invocation.getAction()); + 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) @@ -172,8 +171,12 @@ private void copyAuthorizedProperties( writeMethod.invoke(target, newTarget); targetValue = newTarget; } else { - // Cannot recurse without a fresh target — copy whole value - writeMethod.invoke(target, sourceValue); + // No no-arg constructor for the nested bean: skip rather than bulk-copy the + // unfiltered source value, which would bypass per-path authorization for every + // property underneath this node. + LOG.warn("REST nested bean [{}] skipped — no no-arg constructor for [{}]," + + " cannot authorize its nested properties", + fullPath, sourceValue.getClass().getName()); continue; } } @@ -304,67 +307,6 @@ private Object deepCopyAuthorizedArray( return result; } - /** - * Fallback for actions without a no-arg constructor: scrub unauthorized properties after direct deserialization. - * Recurses into nested beans, collection elements, and map values for full parity with - * {@code copyAuthorizedProperties} depth semantics. - */ - private void scrubUnauthorizedProperties(Object target, Object action) throws Exception { - // Use an identity-based Set to guard against circular references. - // System.identityHashCode is not guaranteed unique; IdentityHashMap uses reference equality (==). - scrubUnauthorizedPropertiesRecursive(target, action, target, "", - Collections.newSetFromMap(new IdentityHashMap<>())); - } - - private void scrubUnauthorizedPropertiesRecursive( - Object target, Object action, Object authTarget, String prefix, - Set visited) throws Exception { - // Guard against circular references in the object graph (identity equality, not equals()) - if (!visited.add(target)) { - return; - } - BeanInfo beanInfo = Introspector.getBeanInfo(target.getClass(), Object.class); - for (PropertyDescriptor pd : beanInfo.getPropertyDescriptors()) { - Method readMethod = pd.getReadMethod(); - Method writeMethod = pd.getWriteMethod(); - if (readMethod == null || writeMethod == null) { - continue; - } - - String fullPath = prefix.isEmpty() ? pd.getName() : prefix + "." + pd.getName(); - Object value = readMethod.invoke(target); - - if (!parameterAuthorizer.isAuthorized(fullPath, authTarget, action)) { - if (value != null) { - try { - writeMethod.invoke(target, (Object) null); - } catch (IllegalArgumentException e) { - LOG.debug("Cannot null primitive property [{}], skipping scrub", pd.getName()); - } - } - } else if (value != null) { - // Authorized — recurse into complex types to scrub any nested unauthorized fields - if (isNestedBeanType(value.getClass())) { - scrubUnauthorizedPropertiesRecursive(value, action, authTarget, fullPath, visited); - } else if (value instanceof Collection) { - for (Object element : (Collection) value) { - if (element != null && isNestedBeanType(element.getClass())) { - scrubUnauthorizedPropertiesRecursive( - element, action, authTarget, fullPath + "[0]", visited); - } - } - } else if (value instanceof Map) { - for (Object mapValue : ((Map) value).values()) { - if (mapValue != null && isNestedBeanType(mapValue.getClass())) { - scrubUnauthorizedPropertiesRecursive( - mapValue, action, authTarget, fullPath + "[0]", visited); - } - } - } - } - } - } - /** * Determines whether a class represents a nested bean that should be recursively authorized, * as opposed to simple/leaf types (primitives, strings, collections, maps, arrays, enums) that