+ * 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. {@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)) { + LOG.debug("Property [{}] belongs to the ModelDriven model, exempting from @StrutsParameter annotation requirement", + rootProperty); + 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; + } + LOG.debug("Property [{}] is declared on the ModelDriven action itself, applying the @StrutsParameter annotation requirement", + rootProperty); + return hasValidAnnotatedMember(rootProperty, action, paramDepth); + } + + /** + * 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, long paramDepth) { + BeanInfo beanInfo = getBeanInfo(target); + if (beanInfo != null && Arrays.stream(beanInfo.getPropertyDescriptors()) + .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#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) {
+ 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) {
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..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
@@ -132,6 +132,127 @@ 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 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 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 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.
+ 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,
@@ -267,9 +388,91 @@ public static class ModelAction implements ModelDriven