Copilot commented on code in PR #1872:
URL: https://github.com/apache/struts/pull/1872#discussion_r3873973748
##########
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:
`rootProperty.charAt(0)` will throw if `rootProperty` is empty (e.g., if
`parameterName` begins with a nesting char and `nestingIndex == 0`, producing
`substring(0, 0)`). If such parameter names can occur in practice (even via
malformed input), this becomes an exception path during authorization. A
concrete fix would be to handle `nestingIndex == 0` / empty `rootProperty`
explicitly (e.g., skip normalization and fall back to `parameterName`, or treat
it as unauthorized/authorized per the intended policy).
##########
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:
`declaresProperty()` currently treats any `PropertyDescriptor` name match as
“declared”, even if it’s read-only (no setter). In a ModelDriven stack, that
can re-introduce the original bypass: if the model has a getter-only property
(or an inherited synthetic property like `class` from `getClass()`), OGNL may
still end up setting the action’s setter, but this code would authorize based
on the model. Consider restricting the “declared property” check to writable
properties (e.g., `desc.getWriteMethod() != null`) and explicitly excluding
`class` (and any other synthetic/non-bindable descriptors) from being treated
as 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:
Field detection uses `getDeclaredField`, which does not consider inherited
fields. If a model/action exposes a public field via a superclass (or interface
constants, etc.), `declaresProperty()` will incorrectly return `false` and may
change authorization decisions. Consider using `getField` (public + inherited)
or walking the class hierarchy to find a declared field in superclasses.
--
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]
