diff --git a/core/src/main/java/org/apache/struts2/StrutsConstants.java b/core/src/main/java/org/apache/struts2/StrutsConstants.java index 13fc2d3d29..8b9ff58c18 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 9c18d88696..6508ed0472 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.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 @@ *
  • struts.chaining.copyErrors - set to true to copy Action Errors
  • *
  • struts.chaining.copyFieldErrors - set to true to copy Field Errors
  • *
  • struts.chaining.copyMessages - set to true to copy Action Messages
  • + *
  • 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).
  • * * *

    @@ -135,6 +145,9 @@ public class ChainingInterceptor extends AbstractInterceptor { protected Collection 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 void setProxyService(ProxyService proxyService) { 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 @@ private void copyStack(ActionInvocation invocation, CompoundRoot root) { List list = prepareList(root); Map 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 ctxMap) { + Class editable = null; + if (proxyService.isProxy(action)) { + editable = proxyService.ultimateTargetClass(action); + } + Collection 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 excludeUnauthorizedProperties(Collection baseExcludes, + BeanInfo beanInfo, Class targetClass, Object action) { + Set 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 fcf9c8543a..e034f2835a 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 0000000000..d5d69741f4 --- /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 85df164cce..1a35bd73b2 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.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(); 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 0000000000..fd1502de4e --- /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; + } +}