WW-5631 Add opt-in @StrutsParameter enforcement to ChainingInterceptor

core/src/main/java/org/apache/struts2/StrutsConstants.java

@@ -734,6 +734,7 @@ public final class StrutsConstants {
     public static final String STRUTS_CHAINING_COPY_ERRORS = "struts.chaining.copyErrors";
     public static final String STRUTS_CHAINING_COPY_FIELD_ERRORS = "struts.chaining.copyFieldErrors";
     public static final String STRUTS_CHAINING_COPY_MESSAGES = "struts.chaining.copyMessages";
+    public static final String STRUTS_CHAINING_REQUIRE_ANNOTATIONS = "struts.chaining.requireAnnotations";
     public static final String STRUTS_OBJECT_FACTORY_CLASSLOADER = "struts.objectFactory.classloader";
 
     /**

core/src/main/java/org/apache/struts2/interceptor/ChainingInterceptor.java

@@ -18,12 +18,15 @@
  */
 package org.apache.struts2.interceptor;
 
+import org.apache.commons.lang3.BooleanUtils;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 import org.apache.struts2.ActionInvocation;
 import org.apache.struts2.StrutsConstants;
 import org.apache.struts2.Unchainable;
 import org.apache.struts2.inject.Inject;
+import org.apache.struts2.interceptor.parameter.ParameterAuthorizer;
+import org.apache.struts2.ognl.OgnlUtil;
 import org.apache.struts2.result.ActionChainResult;
 import org.apache.struts2.result.Result;
 import org.apache.struts2.util.CompoundRoot;
@@ -32,12 +35,16 @@
 import org.apache.struts2.util.ValueStack;
 import org.apache.struts2.util.reflection.ReflectionProvider;
 
+import java.beans.BeanInfo;
+import java.beans.IntrospectionException;
+import java.beans.PropertyDescriptor;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 
 
 /**
@@ -68,6 +75,9 @@
  * <li>struts.chaining.copyErrors - set to true to copy Action Errors</li>
  * <li>struts.chaining.copyFieldErrors - set to true to copy Field Errors</li>
  * <li>struts.chaining.copyMessages - set to true to copy Action Messages</li>
+ * <li>struts.chaining.requireAnnotations - set to true to only copy properties whose target
+ * Action member is annotated with {@code @StrutsParameter} (opt-in, default false). When the
+ * target cannot be introspected, no properties are copied (fail closed).</li>
  * </ul>
  *
  * <p>
@@ -135,6 +145,9 @@
     protected Collection<String> includes;
     protected ReflectionProvider reflectionProvider;
     private ProxyService proxyService;
+    private boolean requireAnnotations = false;
+    private ParameterAuthorizer parameterAuthorizer;
+    private OgnlUtil ognlUtil;
 
     @Inject
     public void setReflectionProvider(ReflectionProvider prov) {
@@ -146,6 +159,21 @@
         this.proxyService = proxyService;
     }
 
+    @Inject
+    public void setParameterAuthorizer(ParameterAuthorizer parameterAuthorizer) {
+        this.parameterAuthorizer = parameterAuthorizer;
+    }
+
+    @Inject
+    public void setOgnlUtil(OgnlUtil ognlUtil) {
+        this.ognlUtil = ognlUtil;
+    }
+
+    @Inject(value = StrutsConstants.STRUTS_CHAINING_REQUIRE_ANNOTATIONS, required = false)
+    public void setRequireAnnotations(String requireAnnotations) {
+        this.requireAnnotations = BooleanUtils.toBoolean(requireAnnotations);
+    }
+
     @Inject(value = StrutsConstants.STRUTS_CHAINING_COPY_ERRORS, required = false)
     public void setCopyErrors(String copyErrors) {
         this.copyErrors = "true".equalsIgnoreCase(copyErrors);
@@ -174,7 +202,7 @@
     private void copyStack(ActionInvocation invocation, CompoundRoot root) {
         List<Object> list = prepareList(root);
         Map<String, Object> ctxMap = invocation.getInvocationContext().getContextMap();
         for (Object object : list) {
             if (!shouldCopy(object)) {
                 continue;
             }
@@ -183,7 +211,54 @@
             if (proxyService.isProxy(action)) {
                 editable = proxyService.ultimateTargetClass(action);
             }
-            reflectionProvider.copy(object, action, ctxMap, prepareExcludes(), includes, editable);
+            Collection<String> copyExcludes = prepareExcludes();
+            if (requireAnnotations) {
+                Class<?> targetClass = editable != null ? editable : action.getClass();
+                BeanInfo beanInfo = getTargetBeanInfo(targetClass);
+                if (beanInfo == null) {
+                    // Fail closed: cannot prove which properties are annotated, so copy nothing.
+                    LOG.warn("Chaining: unable to introspect target [{}]; skipping property copy " +
+                            "(struts.chaining.requireAnnotations enabled)", targetClass.getName());
+                    continue;
+                }
+                copyExcludes = excludeUnauthorizedProperties(copyExcludes, beanInfo, targetClass, action);
+            }
+            reflectionProvider.copy(object, action, ctxMap, copyExcludes, includes, editable);
+        }
+    }
+
+    /**
+     * Returns the excludes to use for the copy: the base excludes unioned with the names of all
+     * writable target properties that are not authorized by {@code @StrutsParameter}.
+     */
+    private Collection<String> excludeUnauthorizedProperties(Collection<String> baseExcludes,
+                                                             BeanInfo beanInfo, Class<?> targetClass, Object action) {
+        Set<String> merged = new HashSet<>();
+        if (baseExcludes != null) {
+            merged.addAll(baseExcludes);
+        }
+        for (PropertyDescriptor descriptor : beanInfo.getPropertyDescriptors()) {
+            if (descriptor.getWriteMethod() == null) {
+                continue;
+            }
+            String name = descriptor.getName();
+            // target == action is deliberate: chaining copies onto the action object itself (not a
+            // ModelDriven model), so the authorizer's ModelDriven exemption must not apply here.
+            if (!parameterAuthorizer.isAuthorized(name, action, action)) {
+                LOG.warn("Chaining: property [{}] not copied to [{}] because it is not annotated with @StrutsParameter",
+                        name, targetClass.getName());
+                merged.add(name);
+            }
+        }
+        return merged;
+    }
+
+    private BeanInfo getTargetBeanInfo(Class<?> targetClass) {
+        try {
+            return ognlUtil.getBeanInfo(targetClass);
+        } catch (IntrospectionException e) {
+            LOG.warn("Chaining: error introspecting target [{}] for @StrutsParameter enforcement", targetClass, e);
+            return null;
         }
     }
 

core/src/main/resources/org/apache/struts2/default.properties

@@ -257,6 +257,11 @@ struts.parameters.requireAnnotations=true
 ### Useful for transitioning legacy applications, but highly recommended to set to false as soon as possible!
 struts.parameters.requireAnnotations.transitionMode=false
 
+### Whether ChainingInterceptor enforces @StrutsParameter on the target action when copying properties.
+### Opt-in hardening; default false preserves legacy chaining behaviour. Only has effect when
+### struts.parameters.requireAnnotations is also enabled.
+struts.chaining.requireAnnotations=false
+
 ### Whether to throw a RuntimeException when a property is not found
 ### in an expression, or when the expression evaluation fails
 struts.el.throwExceptionOnFailure=false

core/src/test/java/org/apache/struts2/interceptor/AnnotatedChainingAction.java

@@ -0,0 +1,45 @@
+/*
+ * 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;
+
+import org.apache.struts2.action.Action;
+import org.apache.struts2.interceptor.parameter.StrutsParameter;
+
+/**
+ * Test fixture: target/source action whose {@code managerApproved} property is annotated with
+ * {@link StrutsParameter}. Used by {@link ChainingInterceptorTest}.
+ */
+public class AnnotatedChainingAction implements Action {
+
+    private boolean managerApproved;
+
+    public boolean getManagerApproved() {
+        return managerApproved;
+    }
+
+    @StrutsParameter
+    public void setManagerApproved(boolean managerApproved) {
+        this.managerApproved = managerApproved;
+    }
+
+    @Override
+    public String execute() {
+        return SUCCESS;
+    }
+}

core/src/test/java/org/apache/struts2/interceptor/ChainingInterceptorTest.java

@@ -27,7 +27,13 @@
 import org.apache.struts2.TestBean;
 import org.apache.struts2.XWorkTestCase;
 import org.apache.struts2.util.ValueStack;
+import org.apache.struts2.interceptor.parameter.StrutsParameterAuthorizer;
+import org.apache.struts2.ognl.OgnlUtil;
+import org.apache.struts2.util.ProxyService;
+import org.mockito.ArgumentMatchers;
+import org.mockito.Mockito;
 
+import java.beans.IntrospectionException;
 import java.util.*;
 
 /**
@@ -149,6 +155,151 @@ public void testNullCompoundRootElementAllowsProcessToContinue() throws Exceptio
     }
 
 
+    private StrutsParameterAuthorizer buildAuthorizer(boolean requireAnnotations, boolean transitionMode) {
+        StrutsParameterAuthorizer authorizer = new StrutsParameterAuthorizer();
+        authorizer.setOgnlUtil(container.getInstance(OgnlUtil.class));
+        authorizer.setProxyService(container.getInstance(ProxyService.class));
+        authorizer.setRequireAnnotations(String.valueOf(requireAnnotations));
+        authorizer.setRequireAnnotationsTransitionMode(String.valueOf(transitionMode));
+        return authorizer;
+    }
+
+    private void enableChainingEnforcement(boolean requireAnnotations, boolean transitionMode) {
+        interceptor.setParameterAuthorizer(buildAuthorizer(requireAnnotations, transitionMode));
+        interceptor.setRequireAnnotations("true");
+    }
+
+    public void testFlagOffCopiesUnannotatedProperty() throws Exception {
+        AnnotatedChainingAction source = new AnnotatedChainingAction();
+        source.setManagerApproved(true);
+        UnannotatedChainingAction target = new UnannotatedChainingAction();
+        mockInvocation.matchAndReturn("getAction", target);
+        stack.push(source);
+        stack.push(target);
+
+        interceptor.intercept(invocation);
+
+        assertTrue("legacy chaining should copy the property when flag is off", target.getManagerApproved());
+    }
+
+    public void testFlagOnSkipsUnannotatedProperty() throws Exception {
+        AnnotatedChainingAction source = new AnnotatedChainingAction();
+        source.setManagerApproved(true);
+        UnannotatedChainingAction target = new UnannotatedChainingAction();
+        mockInvocation.matchAndReturn("getAction", target);
+        stack.push(source);
+        stack.push(target);
+
+        enableChainingEnforcement(true, false);
+        interceptor.intercept(invocation);
+
+        assertFalse("unannotated target property must NOT be copied when enforcement is on",
+                target.getManagerApproved());
+    }
+
+    public void testFlagOnCopiesAnnotatedProperty() throws Exception {
+        AnnotatedChainingAction source = new AnnotatedChainingAction();
+        source.setManagerApproved(true);
+        AnnotatedChainingAction target = new AnnotatedChainingAction();
+        mockInvocation.matchAndReturn("getAction", target);
+        stack.push(source);
+        stack.push(target);
+
+        enableChainingEnforcement(true, false);
+        interceptor.intercept(invocation);
+
+        assertTrue("annotated target property should be copied when enforcement is on",
+                target.getManagerApproved());
+    }
+
+    public void testTransitionModeCopiesNonNestedUnannotatedProperty() throws Exception {
+        AnnotatedChainingAction source = new AnnotatedChainingAction();
+        source.setManagerApproved(true);
+        UnannotatedChainingAction target = new UnannotatedChainingAction();
+        mockInvocation.matchAndReturn("getAction", target);
+        stack.push(source);
+        stack.push(target);
+
+        enableChainingEnforcement(true, true);
+        interceptor.intercept(invocation);
+
+        assertTrue("transition mode should copy depth-0 property without annotation",
+                target.getManagerApproved());
+    }
+
+    public void testRequireAnnotationsFalseIsNoOp() throws Exception {
+        AnnotatedChainingAction source = new AnnotatedChainingAction();
+        source.setManagerApproved(true);
+        UnannotatedChainingAction target = new UnannotatedChainingAction();
+        mockInvocation.matchAndReturn("getAction", target);
+        stack.push(source);
+        stack.push(target);
+
+        interceptor.setParameterAuthorizer(buildAuthorizer(false, false));
+        interceptor.setRequireAnnotations("true");
+        interceptor.intercept(invocation);
+
+        assertTrue("when global requireAnnotations is off, enforcement is a no-op",
+                target.getManagerApproved());
+    }
+
+    public void testEnforcementStillFiltersWithIncludesConfigured() throws Exception {
+        AnnotatedChainingAction source = new AnnotatedChainingAction();
+        source.setManagerApproved(true);
+        UnannotatedChainingAction target = new UnannotatedChainingAction();
+        mockInvocation.matchAndReturn("getAction", target);
+        stack.push(source);
+        stack.push(target);
+
+        interceptor.setIncludes("managerApproved");
+        enableChainingEnforcement(true, false);
+        interceptor.intercept(invocation);
+
+        assertFalse("unauthorized property must be excluded even when listed in includes",
+                target.getManagerApproved());
+    }
+
+    public void testEnforcementResolvesProxiedTargetClass() throws Exception {
+        AnnotatedChainingAction source = new AnnotatedChainingAction();
+        source.setManagerApproved(true);
+        UnannotatedChainingAction target = new UnannotatedChainingAction();
+        mockInvocation.matchAndReturn("getAction", target);
+        stack.push(source);
+        stack.push(target);
+
+        ProxyService proxyService = Mockito.mock(ProxyService.class);
+        Mockito.when(proxyService.isProxy(ArgumentMatchers.any())).thenReturn(true);
+        Mockito.when(proxyService.ultimateTargetClass(ArgumentMatchers.any()))
+                .thenReturn((Class) UnannotatedChainingAction.class);
+        interceptor.setProxyService(proxyService);
+
+        enableChainingEnforcement(true, false);
+        interceptor.intercept(invocation);
+
+        assertFalse("proxied unannotated target property must NOT be copied", target.getManagerApproved());
+    }
+
+    public void testFailsClosedWhenTargetCannotBeIntrospected() throws Exception {
+        AnnotatedChainingAction source = new AnnotatedChainingAction();
+        source.setManagerApproved(true);
+        AnnotatedChainingAction target = new AnnotatedChainingAction();
+        mockInvocation.matchAndReturn("getAction", target);
+        stack.push(source);
+        stack.push(target);
+
+        // Introspection failure must fail closed: copy nothing, even for an annotated property.
+        OgnlUtil ognlUtil = Mockito.mock(OgnlUtil.class);
+        Mockito.when(ognlUtil.getBeanInfo(ArgumentMatchers.any(Class.class)))
+                .thenThrow(new IntrospectionException("boom"));
+        interceptor.setOgnlUtil(ognlUtil);
+
+        enableChainingEnforcement(true, false);
+        interceptor.intercept(invocation);
+
+        assertFalse("nothing should be copied when the target cannot be introspected",
+                target.getManagerApproved());
+    }
+
     @Override
     protected void setUp() throws Exception {
         super.setUp();