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
The following commit(s) were added to refs/heads/main by this push:
new 20979d0ba WW-5709 fix(core): recognise fluent setters in
@StrutsParameter enforcement (#1931)
20979d0ba is described below
commit 20979d0ba5a426877d3c55459859c697f28a3ae3
Author: Lukasz Lenart <[email protected]>
AuthorDate: Sun Sep 13 18:22:27 2026 +0200
WW-5709 fix(core): recognise fluent setters in @StrutsParameter enforcement
(#1931)
java.beans.Introspector only records a write method for a void setter,
while OGNL binds through any public one-argument setX method regardless
of its return type. StrutsParameterAuthorizer derived its view of what a
target can take from PropertyDescriptor.getWriteMethod(), so a fluent
setter was invisible to it: an annotation placed on one never counted,
and on a ModelDriven action an unannotated fluent setter looked declared
on neither the model nor the action and took the custom-accessor
fallback that WW-5698 left for Map-backed models.
The authorizer now resolves the depth-0 accessor the way OGNL does, from
the cached BeanInfo's method descriptors by name and arity. Where several
qualify it prefers the most-derived declaration, so an annotated override
of a generic base setter is the one judged, and among overloads at that
level an annotated one, so a convenience overload beside the annotated
setter does not turn the property away. hasValidAnnotatedPropertyDescriptor
is kept as a delegating shim, deprecated for removal under WW-5739.
Co-authored-by: Claude Opus 5 (1M context) <[email protected]>
---
.../parameter/StrutsParameterAuthorizer.java | 96 ++++++++-----
.../parameter/ParameterAuthorizerTest.java | 149 +++++++++++++++++++++
.../parameter/ParametersInterceptorTest.java | 45 +++++++
3 files changed, 260 insertions(+), 30 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 423b885b0..481556eaf 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
@@ -30,15 +30,18 @@ import org.apache.struts2.util.ProxyService;
import java.beans.BeanInfo;
import java.beans.IntrospectionException;
+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.Objects;
import java.util.Optional;
import static java.lang.String.format;
+import static java.util.Comparator.comparingInt;
import static org.apache.commons.lang3.StringUtils.indexOfAny;
import static
org.apache.struts2.security.DefaultAcceptedPatternsChecker.NESTING_CHARS;
import static
org.apache.struts2.security.DefaultAcceptedPatternsChecker.NESTING_CHARS_STR;
@@ -162,10 +165,9 @@ public class StrutsParameterAuthorizer implements
ParameterAuthorizer {
* 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.
+ * 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.
* <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
@@ -203,13 +205,53 @@ public class StrutsParameterAuthorizer implements
ParameterAuthorizer {
* 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) {
+ return findBindableAccessor(target, property, paramDepth).isPresent()
+ || declaresBindablePublicField(target, property, paramDepth);
+ }
+
+ /**
+ * 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.
+ * <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.
+ */
+ protected Optional<Method> findBindableAccessor(Object target, String
property, long paramDepth) {
BeanInfo beanInfo = getBeanInfo(target);
- if (beanInfo != null &&
Arrays.stream(beanInfo.getPropertyDescriptors())
+ 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));
+ }
+ return Arrays.stream(beanInfo.getPropertyDescriptors())
.filter(desc -> desc.getName().equals(property))
- .anyMatch(desc -> (paramDepth == 0 ? desc.getWriteMethod() :
desc.getReadMethod()) != null)) {
- return true;
+ .map(PropertyDescriptor::getReadMethod)
+ .filter(Objects::nonNull)
+ .findFirst();
+ }
+
+ private static int inheritanceDepth(Class<?> type) {
+ int depth = 0;
+ for (Class<?> ancestor = type.getSuperclass(); ancestor != null;
ancestor = ancestor.getSuperclass()) {
+ depth++;
}
- return declaresBindablePublicField(target, property, paramDepth);
+ return depth;
}
/**
@@ -231,35 +273,29 @@ public class StrutsParameterAuthorizer implements
ParameterAuthorizer {
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);
- BeanInfo beanInfo = getBeanInfo(target);
- if (beanInfo == null) {
- return hasValidAnnotatedField(target, rootProperty, paramDepth);
- }
-
- Optional<PropertyDescriptor> propDescOpt =
Arrays.stream(beanInfo.getPropertyDescriptors())
- .filter(desc ->
desc.getName().equals(rootProperty)).findFirst();
- if (propDescOpt.isEmpty()) {
- return hasValidAnnotatedField(target, rootProperty, paramDepth);
- }
-
- if (hasValidAnnotatedPropertyDescriptor(target, propDescOpt.get(),
paramDepth)) {
+ Optional<Method> accessor = findBindableAccessor(target, rootProperty,
paramDepth);
+ if (accessor.isPresent() && hasValidAnnotatedMethod(target,
accessor.get(), paramDepth)) {
return true;
}
-
return hasValidAnnotatedField(target, rootProperty, paramDepth);
}
+ /**
+ * @deprecated a {@link PropertyDescriptor} cannot describe every setter
OGNL binds through; use
+ * {@link #findBindableAccessor} with {@link #hasValidAnnotatedMethod}
instead
+ */
+ @Deprecated(since = "7.4.0", forRemoval = true)
protected boolean hasValidAnnotatedPropertyDescriptor(Object target,
PropertyDescriptor propDesc, long paramDepth) {
- Class<?> targetClass = ultimateClass(target);
Method relevantMethod = paramDepth == 0 ? propDesc.getWriteMethod() :
propDesc.getReadMethod();
- if (relevantMethod == null) {
- return false;
- }
- if (getPermittedInjectionDepth(relevantMethod) < paramDepth) {
+ return relevantMethod != null && hasValidAnnotatedMethod(target,
relevantMethod, paramDepth);
+ }
+
+ protected boolean hasValidAnnotatedMethod(Object target, Method method,
long paramDepth) {
+ if (getPermittedInjectionDepth(method) < paramDepth) {
String logMessage = format(
"Parameter injection for method [%s] on target [%s]
rejected. Ensure it is annotated with @StrutsParameter with an appropriate
'depth'.",
- relevantMethod.getName(),
- relevantMethod.getDeclaringClass().getName());
+ method.getName(),
+ method.getDeclaringClass().getName());
if (devMode) {
notifyDeveloperOfError(LOG, target, logMessage);
} else {
@@ -267,8 +303,8 @@ public class StrutsParameterAuthorizer implements
ParameterAuthorizer {
}
return false;
}
- LOG.debug("Success: Matching annotated method [{}] found for property
[{}] of depth [{}] on target [{}]",
- relevantMethod.getName(), propDesc.getName(), paramDepth,
targetClass.getSimpleName());
+ LOG.debug("Success: Matching annotated method [{}] of depth [{}] found
on target [{}]",
+ method.getName(), paramDepth,
ultimateClass(target).getSimpleName());
return true;
}
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 1157ed81b..b412da4d7 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
@@ -219,6 +219,68 @@ public class ParameterAuthorizerTest {
assertThat(authorizer.isAuthorized("constant", action.getModel(),
action)).isFalse();
}
+ // --- Fluent setters (WW-5709) ---
+
+ @Test
+ public void annotatedFluentSetter_authorized() {
+ // OGNL binds through any public one-argument setX method, whatever it
returns; java.beans only
+ // sees void ones. The annotation has to count on the method OGNL will
actually call.
+ var action = new FluentAction();
+ assertThat(authorizer.isAuthorized("name", action, action)).isTrue();
+ }
+
+ @Test
+ public void unannotatedFluentSetter_rejected() {
+ var action = new FluentAction();
+ assertThat(authorizer.isAuthorized("role", action, action)).isFalse();
+ }
+
+ @Test
+ public void modelDriven_unannotatedFluentSetterOnAction_rejected() {
+ // Invisible to java.beans, the fluent setter used to look declared on
neither the model nor the
+ // action and took the custom-accessor fallback, which is the one gap
WW-5698 left open.
+ var action = new ModelActionWithFluentSetter();
+ assertThat(authorizer.isAuthorized("actionSecret", action.getModel(),
action)).isFalse();
+ }
+
+ @Test
+ public void modelDriven_annotatedFluentSetterOnAction_authorized() {
+ var action = new ModelActionWithFluentSetter();
+ assertThat(authorizer.isAuthorized("actionAllowed", action.getModel(),
action)).isTrue();
+ }
+
+ @Test
+ public void
modelDriven_fluentSetterOnModelShadowingUnannotatedActionSetter_authorized() {
+ // The model absorbs the parameter through its fluent setter, so the
action's unannotated
+ // namesake is never reached.
+ var action = new ModelActionWithFluentModel();
+ assertThat(authorizer.isAuthorized("shared", action.getModel(),
action)).isTrue();
+ }
+
+ @Test
+ public void
modelDriven_staticSetterNamesakeOfUnannotatedActionProperty_rejected() {
+ // OGNL never invokes static methods on a request path, so a static
setX on the model does not
+ // absorb the parameter and must not exempt the action's namesake.
+ var action = new ModelActionWithStaticSetterNamesake();
+ assertThat(authorizer.isAuthorized("constant", action.getModel(),
action)).isFalse();
+ }
+
+ @Test
+ public void annotatedSetterOverridingGenericBaseSetter_authorized() {
+ // The base class's erased setModel(Object) is listed alongside the
override. The override is the
+ // method OGNL invokes and the one the developer annotated, so it must
be the one judged.
+ var action = new GenericOverrideAction();
+ assertThat(authorizer.isAuthorized("model", action, action)).isTrue();
+ }
+
+ @Test
+ public void annotatedSetterBesideUnannotatedOverload_authorized() {
+ // Overloads tie on inheritance depth and the Introspector lists them
in type-name order, so
+ // setAge(int) comes before setAge(String). Annotating either overload
declares the property.
+ var action = new OverloadedSetterAction();
+ assertThat(authorizer.isAuthorized("age", action, action)).isTrue();
+ }
+
@Test
public void modelDriven_classProperty_rejected() {
// OgnlUtil introspects with Object as the stop class, so "class"
shows up on no descriptor list
@@ -467,6 +529,93 @@ public class ParameterAuthorizerTest {
public String getConstant() { return constant; }
}
+ public static class FluentAction {
+ private String name;
+ private String role;
+
+ @StrutsParameter
+ public FluentAction setName(String name) { this.name = name; return
this; }
+ public String getName() { return name; }
+
+ // NO @StrutsParameter
+ public FluentAction setRole(String role) { this.role = role; return
this; }
+ public String getRole() { return role; }
+ }
+
+ public static class ModelActionWithFluentSetter implements
ModelDriven<Pojo> {
+ private final Pojo model = new Pojo();
+ private String actionSecret;
+ private String actionAllowed;
+
+ @Override
+ public Pojo getModel() { return model; }
+
+ // NO @StrutsParameter
+ public ModelActionWithFluentSetter setActionSecret(String
actionSecret) { this.actionSecret = actionSecret; return this; }
+ public String getActionSecret() { return actionSecret; }
+
+ @StrutsParameter
+ public ModelActionWithFluentSetter setActionAllowed(String
actionAllowed) { this.actionAllowed = actionAllowed; return this; }
+ public String getActionAllowed() { return actionAllowed; }
+ }
+
+ public static class FluentModel {
+ private String shared;
+ public FluentModel setShared(String shared) { this.shared = shared;
return this; }
+ public String getShared() { return shared; }
+ }
+
+ public static class ModelActionWithFluentModel implements
ModelDriven<FluentModel> {
+ private final FluentModel model = new FluentModel();
+ private String shared;
+
+ @Override
+ public FluentModel getModel() { return model; }
+
+ // NO @StrutsParameter
+ public void setShared(String shared) { this.shared = shared; }
+ public String getShared() { return shared; }
+ }
+
+ public static class ModelWithStaticSetter {
+ public static void setConstant(String ignored) { }
+ }
+
+ public static class ModelActionWithStaticSetterNamesake implements
ModelDriven<ModelWithStaticSetter> {
+ private final ModelWithStaticSetter model = new
ModelWithStaticSetter();
+ private String constant;
+
+ @Override
+ public ModelWithStaticSetter getModel() { return model; }
+
+ // NO @StrutsParameter
+ public void setConstant(String constant) { this.constant = constant; }
+ public String getConstant() { return constant; }
+ }
+
+ public static class GenericBase<T> {
+ public void setModel(T model) { }
+ }
+
+ public static class GenericOverrideAction extends GenericBase<String> {
+ private String model;
+
+ @StrutsParameter
+ @Override
+ public void setModel(String model) { this.model = model; }
+ public String getModel() { return model; }
+ }
+
+ public static class OverloadedSetterAction {
+ private String age;
+
+ @StrutsParameter
+ public void setAge(String age) { this.age = age; }
+ // NO @StrutsParameter - convenience overload
+ public void setAge(int age) { this.age = String.valueOf(age); }
+ public String getAge() { return age; }
+ }
+
public static class Pojo {
private String name;
private String shared;
diff --git
a/core/src/test/java/org/apache/struts2/interceptor/parameter/ParametersInterceptorTest.java
b/core/src/test/java/org/apache/struts2/interceptor/parameter/ParametersInterceptorTest.java
index 2b1b4386c..c5c7fffc0 100644
---
a/core/src/test/java/org/apache/struts2/interceptor/parameter/ParametersInterceptorTest.java
+++
b/core/src/test/java/org/apache/struts2/interceptor/parameter/ParametersInterceptorTest.java
@@ -22,7 +22,9 @@ import org.apache.struts2.action.Action;
import org.apache.struts2.ActionContext;
import org.apache.struts2.ActionProxy;
import org.apache.struts2.ActionSupport;
+import org.apache.struts2.ModelDriven;
import org.apache.struts2.ModelDrivenAction;
+import org.apache.struts2.StrutsConstants;
import org.apache.struts2.SimpleAction;
import org.apache.struts2.TestBean;
import org.apache.struts2.text.TextProvider;
@@ -255,6 +257,32 @@ public class ParametersInterceptorTest extends
XWorkTestCase {
assertEquals(fooVal, action.getFoo());
}
+ /**
+ * WW-5709: OGNL binds through a public one-argument setX method whatever
it returns, so a fluent setter on a
+ * ModelDriven action is as much the action's own member as a void one and
needs the same annotation. The model
+ * property alongside it proves the parameters were applied at all.
+ */
+ public void testModelDrivenFluentSetterOnActionRequiresAnnotation() throws
Exception {
+
loadButSet(Map.of(StrutsConstants.STRUTS_PARAMETERS_REQUIRE_ANNOTATIONS,
"true"));
+ ParametersInterceptor pi = createParametersInterceptor();
+
+ FluentModelDrivenAction action = new FluentModelDrivenAction();
+ 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("secret", "leaked through the fluent setter");
+ params.put("allowed", "bound through the annotated fluent 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 fluent setter",
action.getAllowed());
+ assertNull(action.getSecret());
+ }
+
public void testParametersDoesNotAffectSession() throws Exception {
Map<String, Object> params = new HashMap<>();
params.put("blah", "This is blah");
@@ -996,6 +1024,23 @@ public class ParametersInterceptorTest extends
XWorkTestCase {
*/
+ public static class FluentModelDrivenAction implements
ModelDriven<TestBean> {
+ private final TestBean model = new TestBean();
+ private String secret;
+ private String allowed;
+
+ @Override
+ public TestBean getModel() { return model; }
+
+ // NO @StrutsParameter
+ public FluentModelDrivenAction setSecret(String secret) { this.secret
= secret; return this; }
+ public String getSecret() { return secret; }
+
+ @StrutsParameter
+ public FluentModelDrivenAction setAllowed(String allowed) {
this.allowed = allowed; return this; }
+ public String getAllowed() { return allowed; }
+ }
+
private class NoParametersAction implements Action, NoParameters {
public String execute() throws Exception {
