[
https://issues.apache.org/jira/browse/WW-5758?focusedWorklogId=1045857&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1045857
]
ASF GitHub Bot logged work on WW-5758:
--------------------------------------
Author: ASF GitHub Bot
Created on: 06/Oct/26 05:22
Start Date: 06/Oct/26 05:22
Worklog Time Spent: 10m
Work Description: lukaszlenart commented on PR #1991:
URL: https://github.com/apache/struts/pull/1991#issuecomment-6009928257
Thanks, the split looks right now. I ran the tests at 36ddb055d and all 12
pass. If I drop `|| requireChainingAnnotations`, the new test fails, so it
really does check the new gate.
A few things are left:
1. **The `WARN` fires even when there was nothing to copy.** The
authorization check runs before `if (value.isDefined())`. With
`struts.chaining.requireAnnotations=true`, an alias whose source doesn't
resolve and has no matching request parameter still logs `Alias: property [...]
not copied`. Your new test shows it: no parameters are sent, yet
`unannotatedTarget` gets that warning. Could you move the check inside the
`value.isDefined()` branch?
2. **The inline comments from the first review on the test files are still
open:**
- `AliasInterceptorTest.java:141`: the comment block above the first test.
- `AliasInterceptorTest.java:152`: please use a plain value such as
`"value-from-request"`.
- `AliasInterceptorTest.java:157`: one line is enough.
- `AliasAuthorizationTestAction.java`: the field comments.
- `struts-alias-authorization.xml:29`: the comment block.
3. **The new comments are long too.** The 5-line block above the check in
`AliasInterceptor` and the one above
`testUnannotatedStackToStackAliasTargetIsRejectedWhenChainingAnnotationsRequired`
repeat what the PR description already says. A one-line pointer to WW-5631
above the check would do, and the test can go without one.
4. **`default.properties`:** the description of
`struts.chaining.requireAnnotations` still says "Whether ChainingInterceptor
enforces @StrutsParameter...". Please mention `AliasInterceptor` there as well.
5. **Nit:** `ChainingInterceptor` reads the same constant with
`BooleanUtils.toBoolean`. Please use that in `setRequireChainingAnnotations`
too, so both interceptors interpret the setting the same way.
On the docs: yes, please go ahead with the struts-site PR. It should cover
the Alias Interceptor page, a line for the alias interceptor in the "Where
authorization applies" list on the `@StrutsParameter` page, and a note that
`struts.parameters.requireAnnotations.transitionMode` exempts non-nested alias
targets during migration.
Issue Time Tracking
-------------------
Worklog Id: (was: 1045857)
Time Spent: 40m (was: 0.5h)
> Enforce @StrutsParameter authorization in AliasInterceptor's raw-parameter
> fallback
> -----------------------------------------------------------------------------------
>
> Key: WW-5758
> URL: https://issues.apache.org/jira/browse/WW-5758
> Project: Struts 2
> Issue Type: Bug
> Components: Core
> Affects Versions: 7.0.0, 7.3.0, 7.4.0
> Reporter: Gouri Sankar A
> Priority: Major
> Fix For: 7.5.0
>
> Time Spent: 40m
> Remaining Estimate: 0h
>
> AliasInterceptor copies a value from one named source to a developer-chosen
> target property, falling
> back to the raw HTTP request parameter of the source name when the source
> does not resolve on the stack
> (its own documented behavior, shown in its own javadoc example). That
> fallback value was never checked
> against @StrutsParameter before being written to the target, unlike every
> other binding path in the
> framework: ParametersInterceptor, ChainingInterceptor, CookieInterceptor, and
> the REST/JSON plugins (my
> own earlier finding, WW-5642, already merged) all call into the shared
> ParameterAuthorizer before
> binding. AliasInterceptor never did. alias is in defaultStack, so it runs on
> every request to any
> default-configured action.
> Tested on main at a235259 (7.5.0-SNAPSHOT), embedded Tomcat 10.1.34, stock
> struts-default, an action with
> one @StrutsParameter-annotated property and one not. Binding the unannotated
> property directly
> (?secret=DIRECT_ATTEMPT) is correctly rejected, stays untouched. The same
> property, reached by
> configuring aliases to map a raw HTTP parameter name onto it and sending that
> parameter instead
> (?rawParam=PWNED_VIA_ALIAS), is not rejected and takes the attacker supplied
> value directly.
> The fix is deliberately narrower than checking every alias target.
> AliasInterceptor has two different
> value sources: a name that resolves directly on the stack, which names state
> an earlier, properly
> authorized bind already produced such as an earlier action in a chain, and
> the raw HTTP parameter
> fallback, which is the only path that carries never authorized external
> input. Checking every target
> unconditionally would have also blocked the first, legitimate category. I
> built that broader version
> first, live tested it, found it silently broke a same request stack to stack
> copy with no HTTP parameter
> involved at all, and narrowed the check to apply only when the value came
> from the fallback, matching
> exactly what ChainingInterceptor already exempts by default for the same
> underlying reason.
> Affects 7.0.0 through 7.4.0, verified directly on all three released versions
> by reading the code at each
> tag. requireAnnotations has been the default since the very first 7.0 release
> and AliasInterceptor has
> never checked it. Does not affect 6.x: requireAnnotations is not set in 6.x's
> default.properties at all,
> confirmed directly on 6.12.0, so there is no default on control for any
> interceptor to bypass on that
> line.
> This is the same bug class as WW-5642, already merged: a secondary binding
> path that does not enforce
> @StrutsParameter the way ParametersInterceptor does.
> I have a patch with a regression test ready and will open a PR referencing
> this ticket.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
