[
https://issues.apache.org/jira/browse/WW-5698?focusedWorklogId=1038437&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1038437
]
ASF GitHub Bot logged work on WW-5698:
--------------------------------------
Author: ASF GitHub Bot
Created on: 28/Aug/26 06:36
Start Date: 28/Aug/26 06:36
Worklog Time Spent: 10m
Work Description: 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.
Issue Time Tracking
-------------------
Worklog Id: (was: 1038437)
Time Spent: 1h 20m (was: 1h 10m)
> ModelDriven exemption in StrutsParameterAuthorizer also exempts the action's
> own members
> ----------------------------------------------------------------------------------------
>
> Key: WW-5698
> URL: https://issues.apache.org/jira/browse/WW-5698
> Project: Struts 2
> Issue Type: Task
> Components: Core
> Reporter: Lukasz Lenart
> Assignee: Lukasz Lenart
> Priority: Major
> Fix For: 7.4.0
>
> Time Spent: 1h 20m
> Remaining Estimate: 0h
>
> {{StrutsParameterAuthorizer.isAuthorized(...)}} exempts {{ModelDriven}}
> actions from the {{@StrutsParameter}} requirement:
> {code:java}// ModelDriven exemption: only exempt when the action explicitly
> implements ModelDriven
> // and the target is its model object. ...
> if (target != action && action instanceof ModelDriven) {
> return true;
> }
> {code}
> The intent is sound and is what makes model binding work: implementing
> {{ModelDriven}} and returning an object from {{getModel()}} is a type-level
> declaration that the model is request surface, so its properties do not each
> need annotating.
> The effect is wider than the intent. The method returns {{true}} for _any_
> parameter name, and the name is subsequently resolved by OGNL against the
> whole {{CompoundRoot}}, which holds the model on top of the action.
> Authorization is therefore decided about the model, while the resulting write
> may land on the action. The practical result is that the {{@StrutsParameter}}
> requirement does not apply to an action's own members once that action
> implements {{ModelDriven}}.
> h2. Observed
> Same unannotated setter, declared on the action class in both cases, with
> {{struts.parameters.requireAnnotations=true}}:
> {code}plain action parameter actionSecret=... -> not bound
> (correctly rejected)
> ModelDriven action parameter actionSecret=... -> bound
> {code}
> Both runs also bound a second, expected parameter, confirming the negative
> result is a real rejection rather than a harness that binds nothing.
> A related consequence is that framework members inherited from
> {{ActionSupport}} become reachable on {{ModelDriven}} actions in the same way
> — a parameter name of {{getText('some-key').property}} invokes
> {{ActionSupport.getText(String)}}, which is not annotated and is not part of
> any model. That particular call is inert, since it is a resource bundle
> lookup whose result is discarded, but it illustrates that the exempted
> surface is the whole stack rather than the model.
> h2. Proposed change
> Keep the exemption, but scope it to what it is meant to cover: authorize
> members of the model object, and continue to apply the annotation requirement
> to members of the action itself. {{resolveTarget(...)}} already distinguishes
> the two, so the information needed is present at the decision point.
> h2. Compatibility
> This is a behavioural change. An application with a {{ModelDriven}} action
> that currently relies on binding unannotated members declared on the action
> will stop binding them once the requirement applies, and will need those
> members annotated with {{@StrutsParameter}}. That is the same migration those
> members would have needed had the action not been {{ModelDriven}}, but it is
> still a change for existing applications, so it may belong in 8.0.0 rather
> than 7.4.0, or behind {{struts.parameters.requireAnnotations.transitionMode}}
> for a release. Worth deciding before the change is written.
> Related to WW-5697, which concerns method invocation during binding and has a
> separate cause and a separate fix; the two only overlap in that a
> {{ModelDriven}} action is the easiest way to reach both.
> The exemption is also currently undocumented. Whatever scope it ends up with
> should be stated in the {{@StrutsParameter}} and ModelDriven documentation,
> together with the advice that a model should be a request DTO rather than a
> domain or persistence object.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
