[g0w6y]

Fixes WW-5758

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). That fallback value was never checked against @StrutsParameter, unlike every other binding path (ParametersInterceptor, ChainingInterceptor, CookieInterceptor, the REST/JSON plugins): an alias target that is not annotated could still be set, with the value sourced directly from an attacker-controlled request parameter when the fallback fires.

Injects the same shared ParameterAuthorizer ParametersInterceptor and CookieInterceptor already use, and checks it for the alias target, but only when its value came from the raw-parameter fallback. A source name that resolves directly on the stack names something an earlier, properly-authorized bind already produced; copying that between properties is the same category of operation ChainingInterceptor already performs without requiring annotations by default, and stays unaffected. No new configuration flag: this follows the same struts.parameters.requireAnnotations default (true since 7.0) the rest of the framework already uses.

An annotated alias target, reached through the same fallback, is unaffected.

One regression test fails without the change: AliasInterceptorTest#testUnannotatedAliasTargetIsRejected, using a dedicated test action and XML fixture kept separate from the widely-shared SimpleAction fixture so it cannot affect, or be affected by, the other suites that reuse it. It proves all three cases together in one request: an unannotated target via the fallback is rejected, an annotated target via the same fallback still binds, and an unannotated target reached via a stack-resolved name (no fallback) still binds.

[lukaszlenart]

Thanks for this. I checked it out and ran it: AliasInterceptorTest passes with the change, and the new test fails on main. Resolving the target through parameterAuthorizer.resolveTarget(action) is right here: the alias writes through the value stack, just like ParametersInterceptor, so on a ModelDriven action the model gets the value.

Before this can be merged I'd like it to line up with how ChainingInterceptor handles the same question (WW-5631):

1. The stack-to-stack branch should follow struts.chaining.requireAnnotations. Copying a value that already resolves on the stack is the chaining case, and aliasing is documented mainly as glue for action chaining. ChainingInterceptor leaves that copy unchecked by default but lets an application switch the check on. The PR exempts the branch unconditionally, so an application that has set struts.chaining.requireAnnotations=true would still get unannotated properties filled through an alias on a chained action. Gating the request-parameter branch on struts.parameters.requireAnnotations (as now) and the stack branch on struts.chaining.requireAnnotations copies the split the framework already has, with no new constant.
2. Log a skipped alias at WARN, not debug. For existing applications this changes behaviour: the documented #{ 'foo' : 'bar' } example stops filling bar unless bar is annotated. ChainingInterceptor warns when it skips a property (ChainingInterceptor.java:250). A debug line means people find out from missing values, not from the log.
3. Documentation. This needs an update to the Alias Interceptor page, an entry 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 while an application migrates. I'll take care of the Version Notes entry. If you'd like to open the struts-site PR, that would be welcome.
4. Comments. The reasoning in the inline comments is already in the PR description. Please trim the in-file comments to what a reader can't see from the code (details inline).

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[g0w6y]

Thanks for the detailed review.

1 and 2 addressed: the stack-resolved branch now follows struts.chaining.requireAnnotations (default false, same as ChainingInterceptor per WW-5631), and rejections log at WARN matching ChainingInterceptor.java:250's message format. New test (testUnannotatedStackToStackAliasTargetIsRejectedWhenChainingAnnotationsRequired) proves the gate.

4 addressed: trimmed the in-file comment to the WW-5631 cross-reference; the rest of the reasoning is in the PR description.

3 (docs): happy to open the struts-site PR if that's still wanted — let me know.

[lukaszlenart]

Thanks, the split looks right now. I ran the tests at 36ddb05 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.

[g0w6y]

Good catch on the spurious warning — fixed, and thanks for pointing out I'd missed the inline comments from the first review (I'd only checked the top-level review body, not the line comments; fixed my process for next time).

1. Fixed: the check now runs only inside if (value.isDefined()), so an alias whose source never resolves at all (neither on the stack nor as a request parameter) is no longer warned about. New test (testNoWarningWhenAliasSourceNeverResolvesAtAll, using LogCapture) proves it; confirmed red by temporarily reverting the check's position.
2. All six inline comments addressed: the fallback comment is back to one line (your suggested wording), the WW-5631 cross-reference is one line above the check, both test-method comment blocks removed, the field comments in AliasAuthorizationTestAction removed, the XML fixture's comment block removed, and the warning now reads "not set on" per your example.
3. Test value changed from PWNED_VIA_ALIAS to value-from-request, matching the other fixtures.
4. setRequireChainingAnnotations now uses BooleanUtils.toBoolean, matching ChainingInterceptor.setRequireAnnotations.
5. default.properties: struts.chaining.requireAnnotations now mentions AliasInterceptor alongside ChainingInterceptor.

Also merged in main (WW-5759, WW-5757) — no conflicts, full suite green (3409 tests) plus ParametersInterceptorTest and the spring plugin.

On the struts-site PR: happy to open it, covering the Alias Interceptor page, the @StrutsParameter "Where authorization applies" entry, and the transitionMode note. I'll get to it as a follow-up.

[g0w6y]

Docs are up: apache/struts-site#347

[lukaszlenart]

Thanks for working through both rounds. I re-ran AliasInterceptorTest and ChainingInterceptorTest at c132367 and they pass. With the check moved back before isDefined(), testNoWarningWhenAliasSourceNeverResolvesAtAll fails, so it really checks the fix. Looks good to me.