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

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

@@ -115,28 +115,74 @@ 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);
+        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, since it cannot be reaching a member of the action - typically
+     * it is bound by a custom OGNL property accessor on the model, such as a Map-backed model.
+     */
+    protected boolean isAuthorizedOnModelDrivenAction(String rootProperty, Object model, Object action, long paramDepth) {
+        if (declaresProperty(model, rootProperty)) {
+            LOG.debug("Property [{}] belongs to the ModelDriven model, exempting from @StrutsParameter annotation requirement",
+                    rootProperty);
+            return true;
+        }
+        if (!declaresProperty(action, rootProperty)) {
+            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} declares {@code property} as a bean property or a public field, irrespective of any
+     * {@link StrutsParameter} annotation.
+     */
+    protected boolean declaresProperty(Object target, String property) {
+        BeanInfo beanInfo = getBeanInfo(target);
+        if (beanInfo != null && Arrays.stream(beanInfo.getPropertyDescriptors())
+                .anyMatch(desc -> desc.getName().equals(property))) {
+            return true;
+        }
+        try {
+            return Modifier.isPublic(ultimateClass(target).getDeclaredField(property).getModifiers());
+        } catch (NoSuchFieldException e) {
+            return false;
+        }
+    }
+
     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,53 @@
         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 nonModelDrivenAction_differentTarget_notExempt() {
         // Regression test: when target != action but action does NOT implement ModelDriven,
@@ -267,9 +314,34 @@
         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 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; }
     }
 }