This is an automated email from the ASF dual-hosted git repository.
lukaszlenart pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/struts.git
The following commit(s) were added to refs/heads/main by this push:
new 9011c32b3 WW-5631 Add opt-in @StrutsParameter enforcement to
ChainingInterceptor (#1719)
9011c32b3 is described below
commit 9011c32b38598181c5535580e3084932a59d2472
Author: Lukasz Lenart <[email protected]>
AuthorDate: Wed Jun 10 13:19:43 2026 +0200
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 <[email protected]>
* WW-5631 feat(chaining): default struts.chaining.requireAnnotations=false
Co-Authored-By: Claude Opus 4.7 <[email protected]>
* WW-5631 test(chaining): add annotated/unannotated chaining fixtures
Co-Authored-By: Claude Opus 4.7 <[email protected]>
* WW-5631 test(chaining): add failing @StrutsParameter enforcement tests
Co-Authored-By: Claude Opus 4.7 <[email protected]>
* WW-5631 feat(chaining): enforce @StrutsParameter on target when opted in
Co-Authored-By: Claude Opus 4.7 <[email protected]>
* 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 <[email protected]>
* WW-5631 test(chaining): cover includes interaction and proxied target
Co-Authored-By: Claude Opus 4.7 <[email protected]>
* WW-5631 docs(chaining): document struts.chaining.requireAnnotations
Co-Authored-By: Claude Opus 4.7 <[email protected]>
* 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 <[email protected]>
* 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 <[email protected]>
---------
Co-authored-by: Claude Opus 4.7 <[email protected]>
---
.../java/org/apache/struts2/StrutsConstants.java | 1 +
.../struts2/interceptor/ChainingInterceptor.java | 89 +++++++++++-
.../org/apache/struts2/default.properties | 5 +
.../interceptor/AnnotatedChainingAction.java | 45 ++++++
.../interceptor/ChainingInterceptorTest.java | 151 +++++++++++++++++++++
.../interceptor/UnannotatedChainingAction.java | 43 ++++++
6 files changed, 328 insertions(+), 6 deletions(-)
diff --git a/core/src/main/java/org/apache/struts2/StrutsConstants.java
b/core/src/main/java/org/apache/struts2/StrutsConstants.java
index 13fc2d3d2..8b9ff58c1 100644
--- a/core/src/main/java/org/apache/struts2/StrutsConstants.java
+++ b/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";
/**
diff --git
a/core/src/main/java/org/apache/struts2/interceptor/ChainingInterceptor.java
b/core/src/main/java/org/apache/struts2/interceptor/ChainingInterceptor.java
index 9c18d8869..6508ed047 100644
--- a/core/src/main/java/org/apache/struts2/interceptor/ChainingInterceptor.java
+++ b/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.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;
}
}
diff --git a/core/src/main/resources/org/apache/struts2/default.properties
b/core/src/main/resources/org/apache/struts2/default.properties
index fcf9c8543..e034f2835 100644
--- a/core/src/main/resources/org/apache/struts2/default.properties
+++ b/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
diff --git
a/core/src/test/java/org/apache/struts2/interceptor/AnnotatedChainingAction.java
b/core/src/test/java/org/apache/struts2/interceptor/AnnotatedChainingAction.java
new file mode 100644
index 000000000..d5d69741f
--- /dev/null
+++
b/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;
+ }
+}
diff --git
a/core/src/test/java/org/apache/struts2/interceptor/ChainingInterceptorTest.java
b/core/src/test/java/org/apache/struts2/interceptor/ChainingInterceptorTest.java
index 85df164cc..1a35bd73b 100644
---
a/core/src/test/java/org/apache/struts2/interceptor/ChainingInterceptorTest.java
+++
b/core/src/test/java/org/apache/struts2/interceptor/ChainingInterceptorTest.java
@@ -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();
diff --git
a/core/src/test/java/org/apache/struts2/interceptor/UnannotatedChainingAction.java
b/core/src/test/java/org/apache/struts2/interceptor/UnannotatedChainingAction.java
new file mode 100644
index 000000000..fd1502de4
--- /dev/null
+++
b/core/src/test/java/org/apache/struts2/interceptor/UnannotatedChainingAction.java
@@ -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;
+ }
+}
