WW-5759 Resolve @StrutsParameter accessors the way OGNL does

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

@@ -30,13 +30,15 @@
 
 import java.beans.BeanInfo;
 import java.beans.IntrospectionException;
+import java.beans.Introspector;
 import java.beans.MethodDescriptor;
 import java.beans.PropertyDescriptor;
 import java.lang.reflect.AnnotatedElement;
 import java.lang.reflect.Field;
 import java.lang.reflect.Method;
 import java.lang.reflect.Modifier;
 import java.util.Arrays;
+import java.util.List;
 import java.util.Objects;
 import java.util.Optional;
 
@@ -132,7 +134,7 @@ public boolean isAuthorized(String parameterName, Object target, Object action)
                     parameterName);
             return false;
         }
-        String normalisedRootProperty = Character.toLowerCase(rootProperty.charAt(0)) + rootProperty.substring(1);
+        String normalisedRootProperty = Introspector.decapitalize(rootProperty);
 
         // 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
@@ -213,37 +215,54 @@ protected boolean declaresProperty(Object target, String property, long paramDep
      * The method OGNL would go through to bind {@code property} on {@code target} at this depth: the setter for a
      * depth-0 parameter, the getter for a nested one.
      * <p>
-     * The setter is matched the way OGNL matches it - a public instance method named {@code set} plus the
-     * capitalised property name, taking one argument - and not through {@link PropertyDescriptor#getWriteMethod()},
-     * which {@link java.beans.Introspector} only fills in for a {@code void} setter. A fluent setter returning
-     * {@code this} is just as bindable to OGNL, so it has to be just as visible here, both to carry a
-     * {@link StrutsParameter} annotation and to count as declared on a {@link ModelDriven} action.
+     * The accessor is matched the way OGNL matches it - a public instance method named {@code set}, or {@code is}
+     * then {@code get}, plus each of {@link #accessorBaseNames}, taking one argument or none, the first name with a
+     * match winning - and not through a {@link PropertyDescriptor}. {@link java.beans.Introspector} only fills in
+     * the write method for a {@code void} setter, and names the property of {@code getURL} {@code URL} where OGNL
+     * reaches it as {@code uRL} too. A fluent setter returning {@code this} is just as bindable to OGNL, so it has
+     * to be just as visible here, both to carry a {@link StrutsParameter} annotation and to count as declared on a
+     * {@link ModelDriven} action.
      * <p>
-     * Where several setters qualify, the one declared furthest down the hierarchy wins - an override is what OGNL
-     * invokes and what the developer annotated, while the erased setter of a generic superclass is listed
-     * alongside it and carries no annotation - and among overloads declared at that level an annotated one, since
-     * annotating any overload declares the property request surface.
+     * Where several methods of the winning name qualify, the one declared furthest down the hierarchy wins - an
+     * override is what OGNL invokes and what the developer annotated, while the erased setter of a generic
+     * superclass is listed alongside it and carries no annotation - and among overloads declared at that level an
+     * annotated one, since annotating any overload declares the property request surface.
      */
     protected Optional<Method> findBindableAccessor(Object target, String property, long paramDepth) {
         BeanInfo beanInfo = getBeanInfo(target);
         if (beanInfo == null) {
             return Optional.empty();
         }
-        if (paramDepth == 0) {
-            String setterName = "set" + Character.toUpperCase(property.charAt(0)) + property.substring(1);
-            return Arrays.stream(beanInfo.getMethodDescriptors())
-                    .map(MethodDescriptor::getMethod)
-                    .filter(method -> method.getName().equals(setterName)
-                            && method.getParameterCount() == 1
-                            && !Modifier.isStatic(method.getModifiers()))
-                    .max(comparingInt((Method method) -> inheritanceDepth(method.getDeclaringClass()))
-                            .thenComparing(method -> getParameterAnnotation(method) != null));
+        List<String> prefixes = paramDepth == 0 ? List.of("set") : List.of("is", "get");
+        int parameterCount = paramDepth == 0 ? 1 : 0;
+        for (String baseName : accessorBaseNames(property)) {
+            for (String prefix : prefixes) {
+                String accessorName = prefix + baseName;
+                Optional<Method> accessor = Arrays.stream(beanInfo.getMethodDescriptors())
+                        .map(MethodDescriptor::getMethod)
+                        .filter(method -> method.getName().equals(accessorName)
+                                && method.getParameterCount() == parameterCount
+                                && !Modifier.isStatic(method.getModifiers()))
+                        .max(comparingInt((Method method) -> inheritanceDepth(method.getDeclaringClass()))
+                                .thenComparing(method -> getParameterAnnotation(method) != null));
+                if (accessor.isPresent()) {
+                    return accessor;
+                }
+            }
+        }
+        return Optional.empty();
+    }
+
+    /**
+     * The accessor base names OGNL tries for {@code property}, in its order: for a name like {@code uRange} the
+     * JavaBeans {@code uRange} before the capitalised {@code URange}, otherwise the capitalised name alone.
+     */
+    private static List<String> accessorBaseNames(String property) {
+        String capitalised = Character.toUpperCase(property.charAt(0)) + property.substring(1);
+        if (property.length() > 1 && Character.isLowerCase(property.charAt(0)) && Character.isUpperCase(property.charAt(1))) {
+            return List.of(property, capitalised);
         }
-        return Arrays.stream(beanInfo.getPropertyDescriptors())
-                .filter(desc -> desc.getName().equals(property))
-                .map(PropertyDescriptor::getReadMethod)
-                .filter(Objects::nonNull)
-                .findFirst();
+        return List.of(capitalised);
     }
 
     private static int inheritanceDepth(Class<?> type) {
@@ -353,6 +372,7 @@ protected StrutsParameter getParameterAnnotation(AnnotatedElement element) {
     }
 
     protected Class<?> ultimateClass(Object target) {
+        Objects.requireNonNull(target, "target");
         if (proxyService.isProxy(target)) {
             return proxyService.ultimateTargetClass(target);
         }

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

@@ -320,6 +320,104 @@ public void testModelDrivenAnnotatedNestedPropertyOnActionIsAllowlisted() throws
         }
     }
 
+    /**
+     * OGNL resolves {@code uRange} to the JavaBeans setter {@code setuRange} before {@code setURange}, so that is the
+     * method which has to carry the annotation. The model property alongside proves the parameters were applied.
+     */
+    public void testModelDrivenJavaBeansStyleSetterOnActionRequiresAnnotation() {
+        loadButSet(Map.of(StrutsConstants.STRUTS_PARAMETERS_REQUIRE_ANNOTATIONS, "true"));
+        ParametersInterceptor pi = createParametersInterceptor();
+
+        JavaBeansNamedModelDrivenAction action = new JavaBeansNamedModelDrivenAction();
+        ValueStack stack = container.getInstance(ValueStackFactory.class).createValueStack();
+        stack.push(action);
+        stack.push(action.getModel());
+        ActionContext.of().withContainer(container).withValueStack(stack).bind();
+
+        Map<String, Object> params = new HashMap<>();
+        params.put("uRange", "leaked through the unannotated setter");
+        params.put("xCoord", "bound through the annotated setter");
+        params.put("name", "bound on the model");
+        pi.applyParameters(action, stack, HttpParameters.create(params).build());
+
+        assertEquals("bound on the model", action.getModel().getName());
+        assertEquals("bound through the annotated setter", action.getxCoord());
+        assertNull(action.getuRange());
+    }
+
+    /**
+     * A nested parameter goes through the getter OGNL resolves, {@code getURL} for {@code uRL}, which
+     * introspection names property {@code URL}.
+     */
+    public void testModelDrivenUppercaseGetterOnActionRequiresAnnotation() {
+        loadButSet(Map.of(StrutsConstants.STRUTS_PARAMETERS_REQUIRE_ANNOTATIONS, "true"));
+        ParametersInterceptor pi = createParametersInterceptor();
+
+        JavaBeansNamedModelDrivenAction action = new JavaBeansNamedModelDrivenAction();
+        ValueStack stack = container.getInstance(ValueStackFactory.class).createValueStack();
+        stack.push(action);
+        stack.push(action.getModel());
+        ActionContext.of().withContainer(container).withValueStack(stack).bind();
+
+        Map<String, Object> params = new HashMap<>();
+        params.put("uRL.host", "leaked through the unannotated getter");
+        params.put("URL.path", "leaked through the unannotated getter");
+        params.put("name", "bound on the model");
+        pi.applyParameters(action, stack, HttpParameters.create(params).build());
+
+        assertEquals("bound on the model", action.getModel().getName());
+        assertTrue(action.getURL().isEmpty());
+    }
+
+    /**
+     * The annotation counts only on the setter OGNL actually invokes for the parameter name as written.
+     */
+    public void testAnnotationOnlyCountsOnTheSetterOgnlInvokes() {
+        loadButSet(Map.of(StrutsConstants.STRUTS_PARAMETERS_REQUIRE_ANNOTATIONS, "true"));
+        ParametersInterceptor pi = createParametersInterceptor();
+
+        JavaBeansNamedAction action = new JavaBeansNamedAction();
+        ValueStack stack = container.getInstance(ValueStackFactory.class).createValueStack();
+        stack.push(action);
+        ActionContext.of().withContainer(container).withValueStack(stack).bind();
+
+        Map<String, Object> params = new HashMap<>();
+        params.put("xCoord", "bound through the annotated setter");
+        params.put("uRange", "leaked through the unannotated setuRange");
+        params.put("VRange", "leaked through the unannotated setVRange");
+        pi.applyParameters(action, stack, HttpParameters.create(params).build());
+
+        assertEquals("bound through the annotated setter", action.getxCoord());
+        assertEquals(List.of(), action.getInvoked());
+    }
+
+    /**
+     * Names differing from an accessor's property only in case resolve to no accessor on either object, so they
+     * take the "declared on neither" route - which is safe only as long as OGNL does not bind them either.
+     */
+    public void testModelDrivenCaseVariantsBindNothingOnAction() {
+        loadButSet(Map.of(StrutsConstants.STRUTS_PARAMETERS_REQUIRE_ANNOTATIONS, "true"));
+        ParametersInterceptor pi = createParametersInterceptor();
+
+        JavaBeansNamedModelDrivenAction action = new JavaBeansNamedModelDrivenAction();
+        ValueStack stack = container.getInstance(ValueStackFactory.class).createValueStack();
+        stack.push(action);
+        stack.push(action.getModel());
+        ActionContext.of().withContainer(container).withValueStack(stack).bind();
+
+        Map<String, Object> params = new HashMap<>();
+        params.put("urange", "not bound");
+        params.put("URANGE", "not bound");
+        params.put("url.host", "not bound");
+        params.put("Url.path", "not bound");
+        params.put("name", "bound on the model");
+        pi.applyParameters(action, stack, HttpParameters.create(params).build());
+
+        assertEquals("bound on the model", action.getModel().getName());
+        assertNull(action.getuRange());
+        assertTrue(action.getURL().isEmpty());
+    }
+
     public void testParametersDoesNotAffectSession() throws Exception {
         Map<String, Object> params = new HashMap<>();
         params.put("blah", "This is blah");
@@ -1110,6 +1208,50 @@ public static class NestedModelDrivenAction implements ModelDriven<TestBean> {
         public Address getAddress() { return address; }
     }
 
+    public static class JavaBeansNamedModelDrivenAction implements ModelDriven<TestBean> {
+        private final TestBean model = new TestBean();
+        private final Map<String, String> url = new HashMap<>();
+        private String uRange;
+        private String xCoord;
+
+        @Override
+        public TestBean getModel() { return model; }
+
+        // NO @StrutsParameter
+        public void setuRange(String uRange) { this.uRange = uRange; }
+        public String getuRange() { return uRange; }
+
+        @StrutsParameter
+        public void setxCoord(String xCoord) { this.xCoord = xCoord; }
+        public String getxCoord() { return xCoord; }
+
+        // NO @StrutsParameter
+        public Map<String, String> getURL() { return url; }
+    }
+
+    public static class JavaBeansNamedAction {
+        private final List<String> invoked = new ArrayList<>();
+        private String xCoord;
+
+        @StrutsParameter
+        public void setxCoord(String xCoord) { this.xCoord = xCoord; }
+        public String getxCoord() { return xCoord; }
+
+        // NO @StrutsParameter - OGNL prefers it to the annotated setURange for "uRange"
+        public void setuRange(String uRange) { invoked.add("setuRange=" + uRange); }
+
+        @StrutsParameter
+        public void setURange(String uRange) { invoked.add("setURange=" + uRange); }
+
+        @StrutsParameter
+        public void setvRange(String vRange) { invoked.add("setvRange=" + vRange); }
+
+        // NO @StrutsParameter - the only setter OGNL considers for "VRange"
+        public void setVRange(String vRange) { invoked.add("setVRange=" + vRange); }
+
+        public List<String> getInvoked() { return invoked; }
+    }
+
     public static class Address {
         private String city;
         public String getCity() { return city; }

pom.xml

@@ -126,7 +126,7 @@
         <jaxb-impl.version>4.0.9</jaxb-impl.version>
         <log4j2.version>2.26.1</log4j2.version>
         <mockito.version>5.23.0</mockito.version>
-        <ognl.version>3.4.13</ognl.version>
+        <ognl.version>3.4.14</ognl.version>
         <spring.version>6.2.19</spring.version>
         <struts-annotations.version>2.0</struts-annotations.version>
         <velocity-tools.version>3.1</velocity-tools.version>