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
commit 1d976cdc106a35ecbf794acf14b91b2dccd49cac Author: Emil Huseynov <[email protected]> AuthorDate: Sun Nov 1 10:58:44 2026 +0400 WW-5820 Judge the object that actually takes the write, Maps included Review follow-up. findDeclaringObjectBelow skipped Maps and ignored the order CompoundRootAccessor binds in, so a Map-backed model's namesake further down was wrongly rejected, and a Map left in the root below the action wrongly took the custom-accessor fallback. Replace it with takesParameter (a bindable accessor or public field, or a Map, matching CompoundRootAccessor.setProperty) and walk the root to the first object that would take the parameter: exempt when that is the model, hold it to the annotation requirement when it is the action or anything below. Add the two Map cases and pin the non-chained DefaultTextProvider-below-the-action case. --- .../parameter/StrutsParameterAuthorizer.java | 97 +++++++++++++--------- .../parameter/ParameterAuthorizerTest.java | 54 ++++++++++++ 2 files changed, 110 insertions(+), 41 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/interceptor/parameter/StrutsParameterAuthorizer.java b/core/src/main/java/org/apache/struts2/interceptor/parameter/StrutsParameterAuthorizer.java index e17d8ce7d..2d53378bf 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/parameter/StrutsParameterAuthorizer.java +++ b/core/src/main/java/org/apache/struts2/interceptor/parameter/StrutsParameterAuthorizer.java @@ -39,6 +39,7 @@ import java.lang.reflect.Method; import java.lang.reflect.Modifier; import java.util.Arrays; import java.util.List; +import java.util.Map; import java.util.Objects; import java.util.Optional; @@ -158,63 +159,77 @@ public class StrutsParameterAuthorizer implements ParameterAuthorizer { /** * Decides authorization for a {@link ModelDriven} action, whose model is on top of the value stack. * <p> - * Returning an object from {@code getModel()} declares that object to be request surface, so anything the - * model itself can take is exempt from the {@link StrutsParameter} requirement. The exemption stops there: - * OGNL resolves the parameter name against the whole stack, which also holds the action, so a property - * declared on the action is still subject to the annotation requirement. Without that distinction a - * ModelDriven action would silently expose its own members. + * {@code CompoundRootAccessor} binds a parameter to the first object in the value stack root that would take + * it - the first with a matching accessor, or, failing that, the first modifiable {@code Map} - so this walks + * the root in that same order and judges the object that actually absorbs the write, see + * {@link #takesParameter} and {@link #findFirstObjectTakingParameterBelow}. * <p> - * A property declared on neither is allowed: typically it is bound by a custom OGNL property accessor on - * the model, such as a Map-backed model. That fallback guarantees less than it may appear to - only that - * the name reaches no member {@link #declaresProperty} can see. OGNL walks the whole stack, so such a name - * can still land on the action wherever the action absorbs it by a route introspection here does not model, - * being a {@code Map} itself for instance. That is no more permissive than the blanket exemption this - * scoping replaces. + * The model is on top, and returning it from {@code getModel()} declares it request surface, so when the model + * takes the parameter it is exempt from the {@link StrutsParameter} requirement. The exemption stops there: when + * the write instead lands on the action or on anything below it in the root, that object is held to the + * annotation requirement. Without that distinction a ModelDriven action would silently expose its own members, + * and a {@code Map} left in the root by a chain result would absorb any name unchecked. A {@code Map} below the + * action carries no annotated member, so a parameter that lands there is rejected. * <p> - * The fallback only applies when no other object in the value stack root declares the property either. A - * chain result leaves the previous action in the root below the chained action and its model, and OGNL - * writes to the first object in the root with a matching accessor, so a name declared there is held to the - * annotation requirement on that object, see {@link #findDeclaringObjectBelow}. + * When no object in the root takes the parameter it is allowed: typically it is bound by a custom OGNL property + * accessor on the model that introspection here cannot see. That fallback guarantees only that the name reaches + * no object this code can model, which is no more permissive than the blanket exemption this scoping replaces. * <p> - * {@code class} is the exception to that fallback: it is invisible to introspection here rather than absent, - * so it is rejected instead of taking the fallback, which keeps a ModelDriven action from handing OGNL a + * {@code class} is the exception to that fallback: it is invisible to introspection here rather than absent, so + * it is rejected instead of taking the fallback, which keeps a ModelDriven action from handing OGNL a * {@code class} path that the ordinary non-ModelDriven path would have rejected for want of an annotation. */ protected boolean isAuthorizedOnModelDrivenAction(String rootProperty, Object model, Object action, long paramDepth) { - if (declaresProperty(model, rootProperty, paramDepth)) { - LOG.debug("Property [{}] belongs to the ModelDriven model, exempting from @StrutsParameter annotation requirement", + if (takesParameter(model, rootProperty, paramDepth)) { + LOG.debug("Property [{}] is taken by the ModelDriven model, exempting from @StrutsParameter annotation requirement", rootProperty); return true; } - if (!declaresProperty(action, rootProperty, paramDepth)) { - if (CLASS_PROPERTY.equals(rootProperty)) { - LOG.debug("Property [class] is not an unknown property but Object.getClass() on every object alike, so the fallback for a custom accessor does not apply; rejecting"); - return false; - } - Object declaringObject = findDeclaringObjectBelow(rootProperty, model, action, paramDepth); - if (declaringObject != null) { - LOG.debug("Property [{}] is declared on neither the model nor the action but on [{}] further down the value stack, applying the @StrutsParameter annotation requirement to it", - rootProperty, declaringObject.getClass().getName()); - return hasValidAnnotatedMember(rootProperty, declaringObject, paramDepth); - } - LOG.debug("Property [{}] is declared on neither the model nor the action, exempting from @StrutsParameter annotation requirement", + if (CLASS_PROPERTY.equals(rootProperty)) { + LOG.debug("Property [class] is not an unknown property but Object.getClass() on every object alike, so the fallback for a custom accessor does not apply; rejecting"); + return false; + } + if (takesParameter(action, rootProperty, paramDepth)) { + LOG.debug("Property [{}] is taken by the ModelDriven action itself, applying the @StrutsParameter annotation requirement", rootProperty); - return true; + return hasValidAnnotatedMember(rootProperty, action, paramDepth); + } + Object taker = findFirstObjectTakingParameterBelow(rootProperty, model, action, paramDepth); + if (taker != null) { + LOG.debug("Property [{}] is taken by [{}] further down the value stack, applying the @StrutsParameter annotation requirement to it", + rootProperty, taker.getClass().getName()); + return hasValidAnnotatedMember(rootProperty, taker, paramDepth); } - LOG.debug("Property [{}] is declared on the ModelDriven action itself, applying the @StrutsParameter annotation requirement", + LOG.debug("Property [{}] is taken by no object in the value stack root, exempting from @StrutsParameter annotation requirement", rootProperty); - return hasValidAnnotatedMember(rootProperty, action, paramDepth); + return true; + } + + /** + * Whether {@code candidate} would absorb this parameter the way {@code CompoundRootAccessor} binds it: either it + * {@link #declaresProperty declares the property} through a bindable accessor or public field, or it is a + * {@code Map}, into which the accessor puts a terminal parameter under any name and through which it traverses a + * nested parameter whose key the map already holds. A {@code Map} cannot be told apart from an unmodifiable one + * without writing to it, so a modifiable {@code Map} is assumed for a terminal parameter, matching that + * accessor's own default of attempting the put and only moving on when it throws. + */ + protected boolean takesParameter(Object candidate, String property, long paramDepth) { + if (declaresProperty(candidate, property, paramDepth)) { + return true; + } + return candidate instanceof Map && (paramDepth == 0 || ((Map<?, ?>) candidate).containsKey(property)); } /** - * Finds the first object in the value stack root, other than the model and the action, that declares the - * property. {@code CompoundRootAccessor} writes a parameter to the first object in the root with a matching - * accessor, so a name declared on neither the model nor the action can still land on an object further down, - * such as the previous action left in the root by a chain result. + * The first object in the value stack root below the model and the action that would take this parameter, in the + * order {@code CompoundRootAccessor} tries them. A name taken by neither the model nor the action can still land + * further down - on the previous action left in the root by a chain result, or on a {@code Map} standing in the + * root - and the accessor binds it to the first such object, so that is the object the annotation requirement + * applies to. * - * @return the declaring object, or {@code null} if no other object in the root declares the property + * @return the object that would take the parameter, or {@code null} if no object below the model and action does */ - protected Object findDeclaringObjectBelow(String rootProperty, Object model, Object action, long paramDepth) { + protected Object findFirstObjectTakingParameterBelow(String rootProperty, Object model, Object action, long paramDepth) { ActionContext context = ActionContext.getContext(); if (context == null || context.getValueStack() == null) { return null; @@ -223,7 +238,7 @@ public class StrutsParameterAuthorizer implements ParameterAuthorizer { if (candidate == null || candidate == model || candidate == action) { continue; } - if (declaresProperty(candidate, rootProperty, paramDepth)) { + if (takesParameter(candidate, rootProperty, paramDepth)) { return candidate; } } 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 325c559fc..3313f7001 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 @@ -26,12 +26,15 @@ import org.apache.struts2.ognl.DefaultOgnlExpressionCacheFactory; import org.apache.struts2.ognl.OgnlUtil; import org.apache.struts2.ognl.StrutsOgnlGuard; import org.apache.struts2.ognl.StrutsProxyCacheFactory; +import org.apache.struts2.text.DefaultTextProvider; import org.apache.struts2.util.StrutsProxyService; import org.junit.After; import org.junit.Before; import org.junit.Test; +import java.util.HashMap; import java.util.List; +import java.util.Map; import static org.apache.struts2.ognl.OgnlCacheFactory.CacheType.LRU; import static org.assertj.core.api.Assertions.assertThat; @@ -444,6 +447,50 @@ public class ParameterAuthorizerTest { assertThat(authorizer.isAuthorized("actionSecret", action.getModel(), action)).isFalse(); } + @Test + public void modelDriven_mapModel_namesakeUnannotatedBelowInStack_authorized() { + // A Map-backed model absorbs the parameter through map.put before the write can reach a namesake setter + // further down, so the model takes it and the exemption applies. CompoundRootAccessor stops at the model. + var action = new MapModelAction(); + var valueStack = new StubValueStack(); + valueStack.push(new PreviousAction()); + valueStack.push(action); + valueStack.push(action.getModel()); + ActionContext.of().withValueStack(valueStack).bind(); + + assertThat(authorizer.isAuthorized("foo", action.getModel(), action)).isTrue(); + } + + @Test + public void modelDriven_propertyOnNothingInStack_previousMapModelBelow_rejected() { + // A previous action's Map model sitting below the action takes any name through map.put, so the parameter + // lands there rather than taking the fallback. A Map carries no annotated member, so it is rejected. + var action = new ModelActionWithOwnMembers(); + var valueStack = new StubValueStack(); + valueStack.push(new PreviousAction()); + valueStack.push(new HashMap<String, Object>()); + valueStack.push(action); + valueStack.push(action.getModel()); + ActionContext.of().withValueStack(valueStack).bind(); + + assertThat(authorizer.isAuthorized("mapKey", action.getModel(), action)).isFalse(); + } + + @Test + public void modelDriven_nonChained_textProviderPropertyBelowInStack_rejected() { + // A real OgnlValueStack keeps a DefaultTextProvider at the bottom of the root, so even a non-chained + // ModelDriven request has an object below the action whose own settable properties this scoping now holds + // to the annotation requirement. localizedTextProvider is one such unannotated property. + var action = new ModelActionWithOwnMembers(); + var valueStack = new StubValueStack(); + valueStack.push(new DefaultTextProvider()); + valueStack.push(action); + valueStack.push(action.getModel()); + ActionContext.of().withValueStack(valueStack).bind(); + + assertThat(authorizer.isAuthorized("localizedTextProvider", action.getModel(), action)).isFalse(); + } + // --- Inner test classes --- public static class PreviousAction { @@ -528,6 +575,13 @@ public class ParameterAuthorizerTest { public String getShared() { return shared; } } + public static class MapModelAction implements ModelDriven<Map<String, Object>> { + private final Map<String, Object> model = new HashMap<>(); + + @Override + public Map<String, Object> getModel() { return model; } + } + public static class ReadOnlyShadowModel { public String getShadow() { return "read-only"; } }
