[
https://issues.apache.org/jira/browse/WW-5820?focusedWorklogId=1051701&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1051701
]
ASF GitHub Bot logged work on WW-5820:
--------------------------------------
Author: ASF GitHub Bot
Created on: 01/Nov/26 07:37
Start Date: 01/Nov/26 07:37
Worklog Time Spent: 10m
Work Description: emilhuseyn commented on PR #2073:
URL: https://github.com/apache/struts/pull/2073#issuecomment-6555046534
Thanks, you were right that `findDeclaringObjectBelow` didn't follow the
write rule. I've replaced it with a `takesParameter` check (a bindable accessor
or public field, or a `Map`, matching `CompoundRootAccessor.setProperty`) and a
walk to the first object in the root that would actually take the parameter:
exempt when that is the model, held to `@StrutsParameter` when it is the action
or anything below, fallback only when nothing takes it. `class` stays rejected.
Both of your tests pass, and I added a `DefaultTextProvider` pin test for the
non-chained case.
Two points you raised, stated explicitly (and going into the description +
7.5.0 notes):
- **Non-chained stacks are affected too.** `OgnlValueStack` keeps a
`DefaultTextProvider` at the bottom of the root, so even a plain `ModelDriven`
request has objects below the action; their own settable properties (for
example `localizedTextProvider`) are now held to `@StrutsParameter` and
rejected when unannotated. Pinned by
`modelDriven_nonChained_textProviderPropertyBelowInStack_rejected`.
- **ModelDriven -> ModelDriven chains.** When the previous action is itself
`ModelDriven`, its model sits below the current action, so a parameter landing
on the previous model is now held to `@StrutsParameter` too, and the chain
stops binding the previous model's unannotated properties. Deliberate, not the
"model properties behave exactly as before" the first description implied.
On the SECURITY.md bot question: this is not a fix for a privately reported
vulnerability. It is a correctness/hardening follow-up to the public WW-5820
change already under review here, with no embargoed report behind it. Happy to
move it private if you would prefer.
Issue Time Tracking
-------------------
Worklog Id: (was: 1051701)
Time Spent: 0.5h (was: 20m)
> StrutsParameterAuthorizer's declared-on-neither fallback lets a parameter
> bind to an object below the action in the value stack
> -------------------------------------------------------------------------------------------------------------------------------
>
> Key: WW-5820
> URL: https://issues.apache.org/jira/browse/WW-5820
> Project: Struts 2
> Issue Type: Task
> Components: Core
> Reporter: Lukasz Lenart
> Priority: Major
> Fix For: 7.5.0
>
> Time Spent: 0.5h
> Remaining Estimate: 0h
>
> {{StrutsParameterAuthorizer.isAuthorizedOnModelDrivenAction}} applies a
> three-way rule since WW-5698: a property declared on the model is exempt from
> {{@StrutsParameter}}, a property declared on the action needs the annotation,
> and a property declared on neither is allowed so that Map-backed and
> custom-accessor models keep working.
> The decision is made about two objects, the model and the action, while the
> write that follows is made against the whole stack.
> {{OgnlValueStack.setParameter}} hands the name to OGNL, and
> {{CompoundRootAccessor.setProperty}} walks every object in the
> {{CompoundRoot}} and invokes the first matching setter. When the root holds
> more than the model and the action, a name that takes the "declared on
> neither" fallback can land on an object the authorizer did not look at.
> The ordinary way the root holds a third object is a chain result:
> {{ActionChainResult.execute}} runs the chained action on the same value stack
> with the same parameters, so the previous action stays in the root below the
> chained action and its model. The convention plugin's
> {{ConventionUnknownHandler}} builds the same chain result implicitly when an
> action {{foo}} returning {{bar}} has a sibling action {{foo-bar}}, so the
> shape also arises without a chain result declared in struts.xml.
> h2. Observed
> Three actions, {{struts.parameters.requireAnnotations=true}}, default stack.
> {{First}} declares an unannotated public setter {{setFoo}}. {{Second}} is
> {{ModelDriven}}, and neither it nor its model declares {{foo}}. {{Plain}} is
> not {{ModelDriven}} and does not declare {{foo}}.
> {code}First directly foo=x -> not bound (rejected,
> no annotation)
> First chained into Plain foo=x -> not bound (rejected, no
> annotation)
> First chained into Second (ModelDriven) foo=x -> bound on First.setFoo
> {code}
> In the third row the authorizer takes the "declared on neither" fallback for
> {{Second}} and its model, and {{CompoundRootAccessor}} finds the setter on
> {{First}}. The model property and the annotated members of {{Second}} bind as
> expected in the same request, so the first two rows are real rejections
> rather than a harness that binds nothing.
> The property copy done by {{ChainingInterceptor}} is not involved. {{chain}}
> runs before {{params}} in {{defaultStack}}, and {{foo}} on {{First}} is still
> null when the copy runs because the parameter phase of {{First}} already
> rejected the name. {{struts.chaining.requireAnnotations}} governs only that
> copy.
> h2. Proposed change
> Make the authorizer's decision and the landing object agree. Two shapes are
> open:
> * Extend the "declared on neither" branch: before allowing the name, look at
> the remaining objects in the {{CompoundRoot}}. Where one of them declares the
> property, apply the annotation requirement to that object instead of taking
> the fallback.
> * Apply the accepted parameter to the object the decision was made about, the
> model or the action, rather than to the whole root. The fallback then keeps
> serving Map-backed models because the model itself absorbs the name. This
> changes how {{ParametersInterceptor}} writes and needs a compatibility look
> before it is chosen.
> h2. Compatibility
> Either shape affects only a request whose parameter name is declared on
> neither the current model nor the current action and is declared on another
> object in the stack. Model binding, annotated action members and Map-backed
> models are unchanged. An application that relies on a chained-into
> {{ModelDriven}} action binding a request parameter onto the previous action
> will stop binding it and needs {{@StrutsParameter}} on that setter, which is
> the requirement the direct request already has.
> h2. Tests
> * Chain into a {{ModelDriven}} action: a name declared only on the previous
> action is rejected.
> * Chain into a {{ModelDriven}} action: the model property, an annotated
> action property and a Map-backed model key still bind.
> * Direct binding and a chain into a plain action keep their current behaviour
> (control cases).
> * The convention plugin's implicit chain reproduces the same case (plugin
> module, optional).
> Reported by Sungho Park (@gigio1023).
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
