lukaszlenart commented on code in PR #1872:
URL: https://github.com/apache/struts/pull/1872#discussion_r3878440524
##########
core/src/main/java/org/apache/struts2/interceptor/parameter/StrutsParameterAuthorizer.java:
##########
@@ -115,28 +115,74 @@ public boolean isAuthorized(String parameterName, Object
target, Object action)
long paramDepth = parameterName.codePoints().mapToObj(c -> (char)
c).filter(NESTING_CHARS::contains).count();
- // 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;
- }
+ 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);
- // Transition mode: depth-0 (non-nested) parameters are exempt
+ // 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;
}
- 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) {
+ return isAuthorizedOnModelDrivenAction(normalisedRootProperty,
target, action, paramDepth);
+ }
return hasValidAnnotatedMember(normalisedRootProperty, target,
paramDepth);
}
+ /**
+ * Decides authorization for a {@link ModelDriven} action, whose model is
on top of the value stack.
+ * <p>
+ * 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.
+ * <p>
+ * 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;
+ }
Review Comment:
Both halves confirmed and fixed, in 2f3ce9601 and 3aed80b6a.
**Read-only properties.** Verified on a real value stack: with the model on
top and only `getShadow()`, OGNL cannot assign to the model, moves on to the
action, and the action's unannotated setter takes the value. `declaresProperty`
now asks what the target can actually 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. That also covers a `public
static final` namesake, which cannot absorb a parameter either.
**`class`.** Right outcome, different mechanism. `OgnlUtil.getBeanInfo`
introspects with `Object` as the stop class, so `class` never appears among the
property descriptors at all; it was not matching a read-only descriptor but
taking the fallback for a property declared on neither model nor action, which
exists so a Map-backed model can bind through its own OGNL accessor. It is now
rejected there instead, which leaves a model or action that genuinely declares
a `class` property to be decided on its own terms.
##########
core/src/main/java/org/apache/struts2/interceptor/parameter/StrutsParameterAuthorizer.java:
##########
@@ -115,28 +115,74 @@ public boolean isAuthorized(String parameterName, Object
target, Object action)
long paramDepth = parameterName.codePoints().mapToObj(c -> (char)
c).filter(NESTING_CHARS::contains).count();
- // 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;
- }
+ int nestingIndex = indexOfAny(parameterName, NESTING_CHARS_STR);
+ String rootProperty = nestingIndex == -1 ? parameterName :
parameterName.substring(0, nestingIndex);
Review Comment:
Fixed in 2f3ce9601. A name beginning with a nesting character names no root
property, so it is now rejected before normalisation rather than reaching
`charAt(0)` on an empty string.
Of the two policies you offered I took "unauthorized". Falling back to the
whole `parameterName` would authorize against a root property that nobody
declared.
##########
core/src/main/java/org/apache/struts2/interceptor/parameter/StrutsParameterAuthorizer.java:
##########
@@ -115,28 +115,74 @@ public boolean isAuthorized(String parameterName, Object
target, Object action)
long paramDepth = parameterName.codePoints().mapToObj(c -> (char)
c).filter(NESTING_CHARS::contains).count();
- // 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;
- }
+ 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);
- // Transition mode: depth-0 (non-nested) parameters are exempt
+ // 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;
}
- 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) {
+ return isAuthorizedOnModelDrivenAction(normalisedRootProperty,
target, action, paramDepth);
+ }
return hasValidAnnotatedMember(normalisedRootProperty, target,
paramDepth);
}
+ /**
+ * Decides authorization for a {@link ModelDriven} action, whose model is
on top of the value stack.
+ * <p>
+ * 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.
+ * <p>
+ * 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());
Review Comment:
Confirmed and fixed in 2f3ce9601. OGNL sets an inherited public field on the
action as readily as a declared one — checked on a real value stack — so such a
parameter was counting as declared on neither model nor action and taking the
fallback meant for Map-backed models.
`declaresProperty` now uses `getField`, so inherited public fields count.
Static fields are excluded, since a constant is not per-instance request
surface, and a final field cannot take a depth-0 assignment.
`hasValidAnnotatedField` is deliberately left on `getDeclaredField`: that is
the pre-existing annotation check shared with the non-ModelDriven path, and
widening it would loosen authorization beyond this ticket.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
