[
https://issues.apache.org/jira/browse/WW-5820?focusedWorklogId=1051787&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1051787
]
ASF GitHub Bot logged work on WW-5820:
--------------------------------------
Author: ASF GitHub Bot
Created on: 02/Nov/26 00:24
Start Date: 02/Nov/26 00:24
Worklog Time Spent: 10m
Work Description: emilhuseyn commented on PR #2073:
URL: https://github.com/apache/struts/pull/2073#issuecomment-6566131014
A side note while this is in review: I'd like to take WW-5761 next, if
nobody else has started. I don't have Jira access, so I'm asking here.
There is a prototype on my fork, built on this PR because both touch
`StrutsParameterAuthorizer`:
https://github.com/emilhuseyn/struts/tree/WW-5761-enforce-at-member-resolution.
I won't open a PR until this one is settled. `ParametersInterceptor` sets a
per-parameter guard on the binding stack's `SecurityMemberAccess`, so
`@StrutsParameter` is checked on the member OGNL actually resolves. The
authorizer stays the gate for Map-backed models and custom property accessors,
and the core suite passes. Plugin tests and the cost of the lookup are still to
do.
Two questions before I go further:
1. `main` is 7.5.0-SNAPSHOT and the ticket is marked 8.0.0. Should the PR go
to `main`, on by default with `requireAnnotations` or behind its own constant,
or wait for an 8.0.0 branch?
2. Is a per-parameter guard held by `SecurityMemberAccess` the shape you had
in mind, or would you rather keep that state in the OGNL context?
Issue Time Tracking
-------------------
Worklog Id: (was: 1051787)
Time Spent: 50m (was: 40m)
> 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: 50m
> 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)
