From 27ecf18d44e7f642faddef47c0e712a9f444af76 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Thu, 27 Aug 2026 08:16:32 +0200 Subject: [PATCH 1/2] 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/2] 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,