WW-5698 Scope the ModelDriven exemption in StrutsParameterAuthorizer to the model object

core/src/main/java/org/apache/struts2/interceptor/parameter/StrutsParameterAuthorizer.java

@@ -60,6 +60,13 @@ public class StrutsParameterAuthorizer implements ParameterAuthorizer {
 
     private static final Logger LOG = LogManager.getLogger(StrutsParameterAuthorizer.class);
 
+    /**
+     * {@link OgnlUtil#getBeanInfo(Class)} introspects with {@link Object} as the stop class, so this one never
+     * appears among the property descriptors and cannot be told apart from a genuinely unknown name by evidence
+     * alone. It is not unknown, though: it resolves to {@link Object#getClass()} on every object alike.
+     */
+    private static final String CLASS_PROPERTY = "class";
+
     private boolean requireAnnotations = false;
     private boolean requireAnnotationsTransitionMode = false;
     private boolean devMode = false;
@@ -115,28 +122,112 @@ public boolean isAuthorized(String parameterName, Object target, Object action)
 
         long paramDepth = parameterName.codePoints().mapToObj(c -> (char) c).filter(NESTING_CHARS::contains).count();
 
-        // ModelDriven exemption: only exempt when the action explicitly implements ModelDriven
-        // and the target is its model object. This prevents non-ModelDriven root objects
-        // (e.g. JSONInterceptor's configurable rootObject) from bypassing annotation checks.
-        if (target != action && action instanceof ModelDriven) {
-            LOG.debug("ModelDriven target detected (action implements ModelDriven), exempting from @StrutsParameter annotation requirement");
-            return true;
+        int nestingIndex = indexOfAny(parameterName, NESTING_CHARS_STR);
+        String rootProperty = nestingIndex == -1 ? parameterName : parameterName.substring(0, nestingIndex);
+        if (rootProperty.isEmpty()) {
+            LOG.debug("Parameter [{}] begins with a nesting character, so it names no root property to authorize; rejecting",
+                    parameterName);
+            return false;
         }
+        String normalisedRootProperty = Character.toLowerCase(rootProperty.charAt(0)) + rootProperty.substring(1);
 
-        // Transition mode: depth-0 (non-nested) parameters are exempt
+        // Transition mode: depth-0 (non-nested) parameters are exempt. Checked before the ModelDriven
+        // exemption so that it also covers a ModelDriven action's own members, which would otherwise
+        // have no migration path once the exemption is scoped to the model.
         if (requireAnnotationsTransitionMode && paramDepth == 0) {
             LOG.debug("Annotation transition mode enabled, exempting non-nested parameter [{}] from @StrutsParameter annotation requirement",
                     parameterName);
             return true;
         }
 
-        int nestingIndex = indexOfAny(parameterName, NESTING_CHARS_STR);
-        String rootProperty = nestingIndex == -1 ? parameterName : parameterName.substring(0, nestingIndex);
-        String normalisedRootProperty = Character.toLowerCase(rootProperty.charAt(0)) + rootProperty.substring(1);
+        // ModelDriven exemption: only exempt when the action explicitly implements ModelDriven
+        // and the target is its model object. This prevents non-ModelDriven root objects
+        // (e.g. JSONInterceptor's configurable rootObject) from bypassing annotation checks.
+        if (target != action && action instanceof ModelDriven) {
+            return isAuthorizedOnModelDrivenAction(normalisedRootProperty, target, action, paramDepth);
+        }
 
         return hasValidAnnotatedMember(normalisedRootProperty, target, paramDepth);
     }
 
+    /**
+     * 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.
+     * <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, or declaring a setter that OGNL matches on name and arity while
+     * {@link java.beans.Introspector} does not, a fluent one for instance - see WW-5709. Neither case is 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} 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",
+                    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;
+            }
+            LOG.debug("Property [{}] is declared on neither the model nor the action, exempting from @StrutsParameter annotation requirement",
+                    rootProperty);
+            return true;
+        }
+        LOG.debug("Property [{}] is declared on the ModelDriven action itself, applying the @StrutsParameter annotation requirement",
+                rootProperty);
+        return hasValidAnnotatedMember(rootProperty, action, paramDepth);
+    }
+
+    /**
+     * Whether {@code target} can itself take {@code property} at this depth - as a bean property whose relevant
+     * accessor exists, the setter for a depth-0 parameter and the getter for a nested one, or as a public instance
+     * field. Any {@link StrutsParameter} annotation is irrelevant here; this asks only what the object can absorb.
+     * <p>
+     * It has to be bindability rather than the name alone, because OGNL walks the stack until an object actually
+     * accepts the assignment. A model which merely names the property without being able to take it - a getter-only
+     * property under a depth-0 parameter, say - does not absorb that parameter: OGNL moves on to the action, and an
+     * exemption granted on the name alone would hand over the action's own member, which is the very thing this
+     * scoping exists to prevent. Inherited public fields count for the same reason, that OGNL can set them.
+     */
+    protected boolean declaresProperty(Object target, String property, long paramDepth) {
+        BeanInfo beanInfo = getBeanInfo(target);
+        if (beanInfo != null && Arrays.stream(beanInfo.getPropertyDescriptors())
+                .filter(desc -> desc.getName().equals(property))
+                .anyMatch(desc -> (paramDepth == 0 ? desc.getWriteMethod() : desc.getReadMethod()) != null)) {
+            return true;
+        }
+        return declaresBindablePublicField(target, property, paramDepth);
+    }
+
+    /**
+     * Whether {@code target} exposes {@code property} as a public instance field that this parameter could bind
+     * through. {@link Class#getFields} covers inherited fields as well as declared ones, an inherited public field
+     * being just as settable as a declared one. Static fields are not per-instance request surface, and a final
+     * field cannot take a depth-0 assignment, so neither counts as absorbing the parameter.
+     * <p>
+     * Scanning the fields and matching the name here, rather than looking the name up with {@code getField},
+     * keeps the request-derived property name out of a reflection lookup. The two select the same fields.
+     */
+    protected boolean declaresBindablePublicField(Object target, String property, long paramDepth) {
+        return Arrays.stream(ultimateClass(target).getFields())
+                .filter(field -> field.getName().equals(property))
+                .anyMatch(field -> !Modifier.isStatic(field.getModifiers())
+                        && (paramDepth > 0 || !Modifier.isFinal(field.getModifiers())));
+    }
+
     protected boolean hasValidAnnotatedMember(String rootProperty, Object target, long paramDepth) {
         LOG.debug("Checking target [{}] for a matching, correctly annotated member for property [{}]",
                 target.getClass().getSimpleName(), rootProperty);

core/src/test/java/org/apache/struts2/interceptor/parameter/ParameterAuthorizerTest.java

@@ -132,6 +132,127 @@
         assertThat(authorizer.isAuthorized("nested.deep", model, action)).isTrue();
     }
 
+    @Test
+    public void modelDriven_unannotatedActionMember_rejected() {
+        // The exemption covers the model, which is declared request surface by getModel().
+        // It must not reach members declared on the action itself.
+        var action = new ModelActionWithOwnMembers();
+        assertThat(authorizer.isAuthorized("actionSecret", action.getModel(), action)).isFalse();
+    }
+
+    @Test
+    public void modelDriven_annotatedActionMember_authorized() {
+        var action = new ModelActionWithOwnMembers();
+        assertThat(authorizer.isAuthorized("actionAllowed", action.getModel(), action)).isTrue();
+    }
+
+    @Test
+    public void modelDriven_modelProperty_stillAuthorizedWithoutAnnotation() {
+        // The whole point of the exemption: model properties need no annotation.
+        var action = new ModelActionWithOwnMembers();
+        assertThat(authorizer.isAuthorized("name", action.getModel(), action)).isTrue();
+    }
+
+    @Test
+    public void modelDriven_propertyOnNeitherModelNorAction_authorized() {
+        // A model bound through a custom OGNL property accessor (e.g. a Map-backed model) declares no
+        // bean property, and such a name cannot be reaching a member of the action either.
+        var action = new ModelActionWithOwnMembers();
+        assertThat(authorizer.isAuthorized("noSuchPropertyAnywhere", action.getModel(), action)).isTrue();
+    }
+
+    @Test
+    public void modelDriven_modelPropertyShadowingUnannotatedActionProperty_authorized() {
+        // Declared on both. OGNL resolves against the stack top, which is the model, so the model's
+        // property wins and needs no annotation even though the action's namesake is unannotated.
+        var action = new ModelActionWithOwnMembers();
+        assertThat(authorizer.isAuthorized("shared", action.getModel(), action)).isTrue();
+    }
+
+    @Test
+    public void transitionMode_modelDrivenUnannotatedActionMember_exempt() {
+        // Transition mode exists so an application can turn requireAnnotations on while it works
+        // through annotating. It must reach ModelDriven actions too, or the actions affected by
+        // scoping the exemption have no migration path.
+        authorizer.setRequireAnnotationsTransitionMode(Boolean.TRUE.toString());
+        var action = new ModelActionWithOwnMembers();
+        assertThat(authorizer.isAuthorized("actionSecret", action.getModel(), action)).isTrue();
+    }
+
+    @Test
+    public void modelDriven_readOnlyModelPropertyShadowingUnannotatedActionSetter_rejected() {
+        // Verified against a real value stack: with the model on top and only a getter for "shadow",
+        // OGNL cannot assign to the model and moves on to the action, whose unannotated setter takes
+        // the value. Exempting on the name alone would therefore expose the action's own member.
+        var action = new ModelActionWithReadOnlyModelProperty();
+        assertThat(authorizer.isAuthorized("shadow", action.getModel(), action)).isFalse();
+    }
+
+    @Test
+    public void modelDriven_readOnlyModelProperty_stillAuthorizedForNestedParameter() {
+        // A getter is all a nested parameter needs of the root property: OGNL reads "shadow" from the
+        // model and assigns further in. The model does absorb this one, so the exemption still applies.
+        var action = new ModelActionWithReadOnlyModelProperty();
+        assertThat(authorizer.isAuthorized("shadow.anything", action.getModel(), action)).isTrue();
+    }
+
+    @Test
+    public void modelDriven_inheritedPublicFieldOnAction_rejected() {
+        // OGNL sets inherited public fields as readily as declared ones, so a field the action inherits
+        // is still the action's own member and still needs the annotation.
+        var action = new ModelActionInheritingPublicField();
+        assertThat(authorizer.isAuthorized("inheritedSecret", action.getModel(), action)).isFalse();
+    }
+
+    @Test
+    public void modelDriven_inheritedPublicFieldOnModel_authorized() {
+        // The mirror case: a public field the model inherits is model surface like any other.
+        var action = new ModelActionWithInheritingModel();
+        assertThat(authorizer.isAuthorized("inheritedModelField", action.getModel(), action)).isTrue();
+    }
+
+    @Test
+    public void modelDriven_staticFieldNamesakeOfUnannotatedActionProperty_rejected() {
+        // A constant is not per-instance request surface and cannot absorb the parameter, so it must not
+        // stand in for the model the way a real field would.
+        var action = new ModelActionWithConstantNamesake();
+        assertThat(authorizer.isAuthorized("constant", action.getModel(), action)).isFalse();
+    }
+
+    @Test
+    public void modelDriven_classProperty_rejected() {
+        // OgnlUtil introspects with Object as the stop class, so "class" shows up on no descriptor list
+        // and looks like a name declared nowhere - the shape the custom-accessor fallback exempts. It is
+        // not unknown, it is Object.getClass() on everything, and the non-ModelDriven path rejects it for
+        // want of an annotation. The exemption must not make a ModelDriven action the exception.
+        var action = new ModelActionWithOwnMembers();
+        assertThat(authorizer.isAuthorized("class.classLoader.foo", action.getModel(), action)).isFalse();
+        assertThat(authorizer.isAuthorized("class", action.getModel(), action)).isFalse();
+    }
+
+    @Test
+    public void nonModelDrivenAction_classProperty_rejected() {
+        // The behaviour the case above is being aligned with.
+        var action = new SecureAction();
+        assertThat(authorizer.isAuthorized("class.classLoader.foo", action, action)).isFalse();
+    }
+
+    @Test
+    public void parameterNameBeginningWithNestingChar_rejected() {
+        // Such a name has no root property to authorize. It used to reach charAt(0) on an empty string.
+        var action = new ModelActionWithOwnMembers();
+        assertThat(authorizer.isAuthorized(".actionSecret", action.getModel(), action)).isFalse();
+        assertThat(authorizer.isAuthorized("[0].actionSecret", action.getModel(), action)).isFalse();
+        assertThat(authorizer.isAuthorized("(actionSecret)", action.getModel(), action)).isFalse();
+    }
+
+    @Test
+    public void parameterNameBeginningWithNestingChar_nonModelDriven_rejected() {
+        var action = new SecureAction();
+        assertThat(authorizer.isAuthorized(".annotatedProp", action, action)).isFalse();
+        assertThat(authorizer.isAuthorized("[0].annotatedProp", action, action)).isFalse();
+    }
+
     @Test
     public void nonModelDrivenAction_differentTarget_notExempt() {
         // Regression test: when target != action but action does NOT implement ModelDriven,
@@ -267,9 +388,91 @@
         public Pojo getModel() { return new Pojo(); }
     }
 
+    public static class ModelActionWithOwnMembers implements ModelDriven<Pojo> {
+        private final Pojo model = new Pojo();
+        private String actionSecret;
+        private String actionAllowed;
+
+        @Override
+        public Pojo getModel() { return model; }
+
+        // NO @StrutsParameter — declared on the action, so the model exemption must not cover it
+        public void setActionSecret(String actionSecret) { this.actionSecret = actionSecret; }
+        public String getActionSecret() { return actionSecret; }
+
+        @StrutsParameter
+        public void setActionAllowed(String actionAllowed) { this.actionAllowed = actionAllowed; }
+        public String getActionAllowed() { return actionAllowed; }
+
+        // Namesake of a model property, deliberately unannotated
+        private String shared;
+        public void setShared(String shared) { this.shared = shared; }
+        public String getShared() { return shared; }
+    }
+
+    public static class ReadOnlyShadowModel {
+        public String getShadow() { return "read-only"; }
+    }
+
+    public static class ModelActionWithReadOnlyModelProperty implements ModelDriven<ReadOnlyShadowModel> {
+        private final ReadOnlyShadowModel model = new ReadOnlyShadowModel();
+        private String shadow;
+
+        @Override
+        public ReadOnlyShadowModel getModel() { return model; }
+
+        // NO @StrutsParameter — the model only reads "shadow", so a depth-0 parameter lands here
+        public void setShadow(String shadow) { this.shadow = shadow; }
+        public String getShadow() { return shadow; }
+    }
+
+    public static class BaseWithPublicField {
+        public String inheritedSecret;
+    }
+
+    public static class ModelActionInheritingPublicField extends BaseWithPublicField implements ModelDriven<Pojo> {
+        private final Pojo model = new Pojo();
+
+        @Override
+        public Pojo getModel() { return model; }
+    }
+
+    public static class ModelInheritingPublicField extends BaseWithPublicModelField {
+    }
+
+    public static class BaseWithPublicModelField {
+        public String inheritedModelField;
+    }
+
+    public static class ModelActionWithInheritingModel implements ModelDriven<ModelInheritingPublicField> {
+        private final ModelInheritingPublicField model = new ModelInheritingPublicField();
+
+        @Override
+        public ModelInheritingPublicField getModel() { return model; }
+    }
+
+    public static class ModelWithConstant {
+        public static final String constant = "not request surface";
+    }
+
+    public static class ModelActionWithConstantNamesake implements ModelDriven<ModelWithConstant> {
+        private final ModelWithConstant model = new ModelWithConstant();
+        private String constant;
+
+        @Override
+        public ModelWithConstant getModel() { return model; }
+
+        // NO @StrutsParameter
+        public void setConstant(String constant) { this.constant = constant; }
+        public String getConstant() { return constant; }
+    }
+
     public static class Pojo {
         private String name;
+        private String shared;
         public String getName() { return name; }
         public void setName(String name) { this.name = name; }
+        public String getShared() { return shared; }
+        public void setShared(String shared) { this.shared = shared; }
     }
 }