WW-5631 Add opt-in @StrutsParameter enforcement to ChainingInterceptor (#1719)

* WW-5631 feat(chaining): add struts.chaining.requireAnnotations constant Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * WW-5631 feat(chaining): default struts.chaining.requireAnnotations=false Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * WW-5631 test(chaining): add annotated/unannotated chaining fixtures Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * WW-5631 test(chaining): add failing @StrutsParameter enforcement tests Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * WW-5631 feat(chaining): enforce @StrutsParameter on target when opted in Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * WW-5631 refactor(chaining): align requireAnnotations parsing with BooleanUtils Use BooleanUtils.toBoolean for the chaining requireAnnotations flag so it accepts the same values (yes/on/1) as the sibling struts.parameters.requireAnnotations switch, and unify the enforcement WARN message prefix. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * WW-5631 test(chaining): cover includes interaction and proxied target Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * WW-5631 docs(chaining): document struts.chaining.requireAnnotations Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * WW-5631 test(chaining): cover fail-closed introspection; clarify target==action Add a test asserting nothing is copied when the target action cannot be introspected (fail-closed), and document why isAuthorized is called with target == action for chaining (no ModelDriven exemption). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * WW-5631 fix(chaining): address SonarCloud findings - Mark injected parameterAuthorizer/ognlUtil fields transient (S1948); they are re-injected by the container, not serialized. - Extract per-object copy into copyObjectToAction so the copyStack loop uses no break/continue (S135); fail-closed path now returns from the helper instead of continuing the loop. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
2026-08-05 22:56:59 +00:00 · 2026-06-10 13:19:43 +02:00
parent 69cb2964f0
commit 9011c32b38
6 changed files with 328 additions and 6 deletions
@@ -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";

    /**
@@ -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.TextParseUtil;
 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 @@ import java.util.Map;
 * <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 @@ public class ChainingInterceptor extends AbstractInterceptor {
    protected Collection<String> includes;
    protected ReflectionProvider reflectionProvider;
    private ProxyService proxyService;
+    private boolean requireAnnotations = false;
+    private transient ParameterAuthorizer parameterAuthorizer;
+    private transient OgnlUtil ognlUtil;

    @Inject
    public void setReflectionProvider(ReflectionProvider prov) {
@@ -146,6 +159,21 @@ public class ChainingInterceptor extends AbstractInterceptor {
        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);
@@ -175,15 +203,64 @@ public class ChainingInterceptor extends AbstractInterceptor {
        List<Object> list = prepareList(root);
        Map<String, Object> ctxMap = invocation.getInvocationContext().getContextMap();
        for (Object object : list) {
-            if (!shouldCopy(object)) {
+            if (shouldCopy(object)) {
+                copyObjectToAction(object, invocation.getAction(), ctxMap);
+            }
+        }
+    }
+
+    private void copyObjectToAction(Object object, Object action, Map<String, Object> ctxMap) {
+        Class<?> editable = null;
+        if (proxyService.isProxy(action)) {
+            editable = proxyService.ultimateTargetClass(action);
+        }
+        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());
+                return;
+            }
+            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;
            }
-            Object action = invocation.getAction();
-            Class<?> editable = null;
-            if (proxyService.isProxy(action)) {
-                editable = proxyService.ultimateTargetClass(action);
+            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);
            }
-            reflectionProvider.copy(object, action, ctxMap, prepareExcludes(), includes, editable);
+        }
+        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;
        }
    }

@@ -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
@@ -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;
+    }
+}
@@ -27,7 +27,13 @@ import org.apache.struts2.SimpleAction;
 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 class ChainingInterceptorTest extends XWorkTestCase {
    }


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