From 27ecf18d44e7f642faddef47c0e712a9f444af76 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Thu, 27 Aug 2026 08:16:32 +0200 Subject: [PATCH 1/5] WW-5698 fix(params): scope the ModelDriven exemption to the model object isAuthorized returned true for every parameter name once the action implemented ModelDriven. OGNL then resolves that name against the whole CompoundRoot, which holds the model on top of the action, so authorization was decided about the model while the write could land on the action. In effect the @StrutsParameter requirement did not apply to a ModelDriven action's own members: an unannotated setter declared on the action was bound, where the identical setter on a plain action is rejected. The exemption now covers what it was meant to cover. A property declared by the model is exempt, since returning an object from getModel() declares it request surface. A property declared by the action is subject to the annotation requirement as usual. A property declared by neither is still allowed, because it cannot be reaching a member of the action - that case is typically a model bound through a custom OGNL property accessor, such as a Map-backed model, and rejecting it would break those applications. The model is checked first so that a model property shadowing an action property still binds without an annotation, matching OGNL's own resolution against the stack top. Co-Authored-By: Claude Opus 5 --- .../parameter/StrutsParameterAuthorizer.java | 56 +++++++++++++++-- .../parameter/ParameterAuthorizerTest.java | 62 +++++++++++++++++++ 2 files changed, 112 insertions(+), 6 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 03cc22c0e5..63e48510a7 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 @@ -115,12 +115,15 @@ public boolean isAuthorized(String parameterName, Object target, Object action) long paramDepth = parameterName.codePoints().mapToObj(c -> (char) c).filter(NESTING_CHARS::contains).count(); + 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) { - LOG.debug("ModelDriven target detected (action implements ModelDriven), exempting from @StrutsParameter annotation requirement"); - return true; + return isAuthorizedOnModelDrivenAction(normalisedRootProperty, target, action, paramDepth); } // Transition mode: depth-0 (non-nested) parameters are exempt @@ -130,13 +133,54 @@ public boolean isAuthorized(String parameterName, Object target, Object action) 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); - return hasValidAnnotatedMember(normalisedRootProperty, target, paramDepth); } + /** + * Decides authorization for a {@link ModelDriven} action, whose model is on top of the value stack. + *

+ * 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. + *

+ * 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); 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 43eb57bcc2..c2e316bee5 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 @@ -132,6 +132,43 @@ public void modelDriven_targetIsModel_allAuthorized() { 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 nonModelDrivenAction_differentTarget_notExempt() { // Regression test: when target != action but action does NOT implement ModelDriven, @@ -267,9 +304,34 @@ public static class ModelAction implements ModelDriven { public Pojo getModel() { return new Pojo(); } } + public static class ModelActionWithOwnMembers implements ModelDriven { + 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; } } } From 69d309d855344a47704170806989c2c5d58047c9 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Thu, 27 Aug 2026 08:36:11 +0200 Subject: [PATCH 2/5] WW-5698 fix(params): let transition mode reach ModelDriven actions The ModelDriven branch returned before the transition mode check, so requireAnnotations.transitionMode never applied to a ModelDriven action. That did not matter while the exemption authorized everything, but once it is scoped to the model the action's own members are rejected, and those are exactly the members transition mode exists to keep binding during migration. Checking transition mode first gives the affected applications the same migration path they would have on any other action. Co-Authored-By: Claude Opus 5 --- .../parameter/StrutsParameterAuthorizer.java | 16 +++++++++------- .../parameter/ParameterAuthorizerTest.java | 10 ++++++++++ 2 files changed, 19 insertions(+), 7 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 63e48510a7..fae0395065 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 @@ -119,6 +119,15 @@ public boolean isAuthorized(String parameterName, Object target, Object action) 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. 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; + } + // 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. @@ -126,13 +135,6 @@ public boolean isAuthorized(String parameterName, Object target, Object action) return isAuthorizedOnModelDrivenAction(normalisedRootProperty, target, action, paramDepth); } - // Transition mode: depth-0 (non-nested) parameters are exempt - if (requireAnnotationsTransitionMode && paramDepth == 0) { - LOG.debug("Annotation transition mode enabled, exempting non-nested parameter [{}] from @StrutsParameter annotation requirement", - parameterName); - return true; - } - return hasValidAnnotatedMember(normalisedRootProperty, target, paramDepth); } 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 c2e316bee5..4e2915e7d7 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 @@ -169,6 +169,16 @@ public void modelDriven_modelPropertyShadowingUnannotatedActionProperty_authoriz 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, From 2f3ce9601080142e6d3b374d6e68caab48a74e4d Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Fri, 28 Aug 2026 07:56:16 +0200 Subject: [PATCH 3/5] WW-5698 fix(params): scope the exemption by what the model can bind, not by name Copilot's review of #1872 found three ways the scoped exemption still let a parameter through to the action's own members. All three reproduce. Keying the exemption on the property name alone is not enough, because OGNL walks the stack until an object actually accepts the assignment: - a getter-only property on the model cannot take a depth-0 parameter, so OGNL moves on and the action's unannotated setter takes it. Verified on a real value stack: the action's field ends up holding the value. - an inherited public field on the action was invisible to getDeclaredField, so the parameter counted as declared on neither model nor action and took the exemption meant for Map-backed models. OGNL sets inherited public fields as readily as declared ones. - a public static final namesake on the model cannot absorb a parameter either, and would have stood in for a real field. declaresProperty therefore now asks what the object can bind at this depth - the setter for a depth-0 parameter, the getter for a nested one, or a public instance field - rather than whether the name appears anywhere. Also rejects a parameter name that begins with a nesting character. It names no root property, and computing one ran charAt(0) on an empty string. Both changed methods are new in this PR, so their signatures are not yet API. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01P9Pjt4rvb1ASASjSTHsUhL --- .../parameter/StrutsParameterAuthorizer.java | 42 +++++-- .../parameter/ParameterAuthorizerTest.java | 113 ++++++++++++++++++ 2 files changed, 148 insertions(+), 7 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 fae0395065..bf210285a9 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 @@ -117,6 +117,11 @@ public boolean isAuthorized(String parameterName, Object target, Object action) 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. Checked before the ModelDriven @@ -151,12 +156,12 @@ public boolean isAuthorized(String parameterName, Object target, Object action) * 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)) { + 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)) { + if (!declaresProperty(action, rootProperty, paramDepth)) { LOG.debug("Property [{}] is declared on neither the model nor the action, exempting from @StrutsParameter annotation requirement", rootProperty); return true; @@ -167,20 +172,43 @@ protected boolean isAuthorizedOnModelDrivenAction(String rootProperty, Object mo } /** - * Whether {@code target} declares {@code property} as a bean property or a public field, irrespective of any - * {@link StrutsParameter} annotation. + * 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. + *

+ * 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) { + protected boolean declaresProperty(Object target, String property, long paramDepth) { BeanInfo beanInfo = getBeanInfo(target); if (beanInfo != null && Arrays.stream(beanInfo.getPropertyDescriptors()) - .anyMatch(desc -> desc.getName().equals(property))) { + .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#getField} rather than {@code getDeclaredField}, since an inherited public field is 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. + */ + protected boolean declaresBindablePublicField(Object target, String property, long paramDepth) { + Field field; try { - return Modifier.isPublic(ultimateClass(target).getDeclaredField(property).getModifiers()); + field = ultimateClass(target).getField(property); } catch (NoSuchFieldException e) { return false; } + if (Modifier.isStatic(field.getModifiers())) { + return false; + } + return paramDepth > 0 || !Modifier.isFinal(field.getModifiers()); } protected boolean hasValidAnnotatedMember(String rootProperty, Object target, long paramDepth) { 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 4e2915e7d7..a4729d0a77 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 @@ -179,6 +179,62 @@ public void transitionMode_modelDrivenUnannotatedActionMember_exempt() { 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 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, @@ -336,6 +392,63 @@ public static class ModelActionWithOwnMembers implements ModelDriven { public String getShared() { return shared; } } + public static class ReadOnlyShadowModel { + public String getShadow() { return "read-only"; } + } + + public static class ModelActionWithReadOnlyModelProperty implements ModelDriven { + 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 { + 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 { + 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 { + 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; From 3aed80b6ab00350fca1f1e7cf379b0ecbf62ea67 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Fri, 28 Aug 2026 08:16:48 +0200 Subject: [PATCH 4/5] WW-5698 fix(params): do not let a class path take the unknown-property fallback The remaining half of Copilot's first review comment on #1872: "class" was exempted for a ModelDriven action, where the ordinary path rejects it. Not for the reason the comment gives, though. OgnlUtil introspects with Object as the stop class, so "class" never appears among the property descriptors at all; it was not being matched as a read-only descriptor but taking the fallback for a property declared on neither model nor action, which exists to let a Map-backed model bind through its own OGNL accessor. That fallback is the wrong home for it: "class" is not an unknown name, it is Object.getClass() on every object alike, and the non-ModelDriven path rejects it for want of an annotation. Rejected there rather than earlier, so a model or action that really does declare a "class" property is still decided on its own terms. This is defence in depth, not a live bypass. Navigating a class path is already inert: java.lang.Class and java.lang.ClassLoader are both in the default struts.excludedClasses, and SecurityMemberAccess refuses their members - checked on a real value stack, where every class.* read returns null and every set has no effect. Worth closing anyway, because the two paths disagreeing is the very thing this ticket is about. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01P9Pjt4rvb1ASASjSTHsUhL --- .../parameter/StrutsParameterAuthorizer.java | 16 +++++++++++++++- .../parameter/ParameterAuthorizerTest.java | 18 ++++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) 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 bf210285a9..1d83a8e9e8 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 @@ -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; @@ -153,7 +160,10 @@ public boolean isAuthorized(String parameterName, Object target, Object action) * ModelDriven action would silently expose its own members. *

* 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. + * it is bound by a custom OGNL property accessor on the model, such as a Map-backed model. {@code class} is + * the exception to that: 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)) { @@ -162,6 +172,10 @@ protected boolean isAuthorizedOnModelDrivenAction(String rootProperty, Object mo 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; 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 a4729d0a77..1157ed81b7 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,24 @@ public void modelDriven_staticFieldNamesakeOfUnannotatedActionProperty_rejected( 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. From 9d53dd894ac56da81e6484ffc55cc011b17e3e63 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Fri, 28 Aug 2026 08:39:43 +0200 Subject: [PATCH 5/5] WW-5698 refactor(params): match the public field by scanning rather than lookup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SonarCloud failed the gate on javasecurity:S6173 — the request-derived property name reaching Class.getField as a reflection lookup. In substance a false positive: nothing is constructed or invoked, the Field is only inspected for its modifiers. But the sink is avoidable at no cost, so avoid it. Class.getFields() selects exactly the fields getField(name) searches — public, declared and inherited — so scanning them and comparing the name is the same decision without the name reaching a reflection API. It also reads consistently with the property descriptor stream just above it. Behaviour unchanged: core 3212, json 166, rest 124 all green. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01P9Pjt4rvb1ASASjSTHsUhL --- .../parameter/StrutsParameterAuthorizer.java | 23 ++++++++----------- 1 file changed, 10 insertions(+), 13 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 1d83a8e9e8..3f93776da1 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 @@ -208,21 +208,18 @@ protected boolean declaresProperty(Object target, String property, long paramDep /** * Whether {@code target} exposes {@code property} as a public instance field that this parameter could bind - * through. {@link Class#getField} rather than {@code getDeclaredField}, since an inherited public field is 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. + * 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. + *

+ * 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) { - Field field; - try { - field = ultimateClass(target).getField(property); - } catch (NoSuchFieldException e) { - return false; - } - if (Modifier.isStatic(field.getModifiers())) { - return false; - } - return paramDepth > 0 || !Modifier.isFinal(field.getModifiers()); + 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) {