[
https://issues.apache.org/jira/browse/WW-5698?focusedWorklogId=1038435&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1038435
]
ASF GitHub Bot logged work on WW-5698:
--------------------------------------
Author: ASF GitHub Bot
Created on: 28/Aug/26 06:16
Start Date: 28/Aug/26 06:16
Worklog Time Spent: 10m
Work Description: github-advanced-security[bot] commented on code in PR
#1872:
URL: https://github.com/apache/struts/pull/1872#discussion_r3878340751
##########
core/src/main/java/org/apache/struts2/interceptor/parameter/StrutsParameterAuthorizer.java:
##########
@@ -115,28 +115,102 @@
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);
+ if (rootProperty.isEmpty()) {
+ LOG.debug("Parameter [{}] begins with a nesting character, so it
names no root property to authorize; rejecting",
+ parameterName);
+ return false;
}
+ 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, paramDepth)) {
+ LOG.debug("Property [{}] belongs to the ModelDriven model,
exempting from @StrutsParameter annotation requirement",
+ rootProperty);
+ return true;
+ }
+ if (!declaresProperty(action, rootProperty, paramDepth)) {
+ 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.
+ * <p>
+ * 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#getField} rather than {@code getDeclaredField},
since an inherited public field is 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.
+ */
+ protected boolean declaresBindablePublicField(Object target, String
property, long paramDepth) {
+ Field field;
+ try {
+ field = ultimateClass(target).getField(property);
Review Comment:
## SonarCloud / Reflection should not be vulnerable to injection attacks
<!--SONAR_ISSUE_KEY:AaBHAzt2KNzgUaIETn2l-->Change this code to not construct
class or method names directly from user-controlled data. <p>See more on <a
href="https://sonarcloud.io/project/issues?id=apache_struts&issues=AaBHAzt2KNzgUaIETn2l&open=AaBHAzt2KNzgUaIETn2l&pullRequest=1872";>SonarQube
Cloud</a></p>
[Show more
details](https://github.com/apache/struts/security/code-scanning/1438)
Issue Time Tracking
-------------------
Worklog Id: (was: 1038435)
Time Spent: 1h (was: 50m)
> 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
> 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)
