WW-5758 Enforce @StrutsParameter authorization in AliasInterceptor's raw-parameter fallback by g0w6y · Pull Request #1991 · apache/struts

g0w6y · 2026-10-05T15:20:26Z

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.

…raw-parameter fallback AliasInterceptor copies a value from one named stack/request slot to another, falling back to the raw HTTP 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 before being written to the alias target, unlike every other binding path in the framework (ParametersInterceptor, ChainingInterceptor with struts.chaining.requireAnnotations, 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. Fix: inject the same shared ParameterAuthorizer used by ParametersInterceptor and CookieInterceptor, and check it for the alias target, but only when the value actually came from the raw-parameter fallback - not when the source name resolved directly on the stack. A name that resolves on the stack names something an earlier, properly-authorized bind already produced (e.g. an earlier action in a chain); copying that between properties is the same category of operation ChainingInterceptor performs without requiring annotations by default, and must stay unaffected. No new configuration flag: this follows the same struts.parameters.requireAnnotations default (true since 7.0) the rest of the framework already uses. Affects 7.0.0 through 7.4.0 (verified directly on 7.0.0, 7.3.0, 7.4.0): struts.parameters.requireAnnotations=true has been the default since the 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 (falls back to the Java field default of false), so there is no default-on control to bypass there.

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):

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.
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.
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.
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).

lukaszlenart · 2026-10-05T19:19:46Z

+                    boolean fromRawRequestParameter = false;
                    if (!value.isDefined()) {
-                        // workaround
+                        // workaround: name did not resolve on the stack (e.g. no earlier action in a


Could this go back to a one-line comment? Something like // name did not resolve on the stack, fall back to the request parameter. The comparison with ChainingInterceptor belongs in the PR description, where it already is.

lukaszlenart · 2026-10-05T19:19:46Z

                            }
                        }
                    }
+                    if (fromRawRequestParameter && !parameterAuthorizer.isAuthorized(alias, authorizationTarget, action)) {


This is where I'd expect the second branch: when the value came from the stack, check it the same way, gated on struts.chaining.requireAnnotations (injected like ChainingInterceptor.setRequireAnnotations, required = false). With the chaining flag off (the default), behaviour stays exactly as in this PR.

lukaszlenart · 2026-10-05T19:19:46Z

                        }
                    }
+                    if (fromRawRequestParameter && !parameterAuthorizer.isAuthorized(alias, authorizationTarget, action)) {
+                        LOG.debug("Alias target [{}] rejected by @StrutsParameter authorization on target [{}]",


LOG.warn here, please, and use the wording ChainingInterceptor uses, e.g. Alias: property [{}] not set on [{}] because it is not annotated with @StrutsParameter.

lukaszlenart · 2026-10-05T19:19:46Z

        return (SimpleAction) proxy.getAction();
    }

+    // An alias target that is not annotated with @StrutsParameter must not be set when the source value


The test name and its assertions already say what this comment says. Could it go?

Please also add a case for the stack-to-stack branch with struts.chaining.requireAnnotations=true (unannotated target stays untouched) next to the existing one with it off.

lukaszlenart · 2026-10-05T19:19:46Z

+    public void testUnannotatedAliasTargetIsRejected() throws Exception {
+        Map<String, Object> httpParams = new HashMap<>();
+        httpParams.put("rawSourceAnnotated", "allowed-value");
+        httpParams.put("rawSourceUnannotated", "PWNED_VIA_ALIAS");


Please use a plain value here, e.g. "value-from-request", to match the other fixtures in this class.

lukaszlenart · 2026-10-05T19:19:46Z

+
+        XmlConfigurationProvider provider = new StrutsXmlConfigurationProvider("struts-alias-authorization.xml");
+        container.inject(provider);
+        // loadConfigurationProviders tears down and rebuilds the whole configuration on every call, so the


This one is worth keeping, since the ordering trap isn't obvious. A single line is enough, though.

lukaszlenart · 2026-10-05T19:19:46Z

+
+    private String annotatedTarget;
+
+    // Deliberately NOT annotated with @StrutsParameter: an alias may not use this as a copy target


Can the field comments here go? Each field's role is clear from its name and the test that uses it.

lukaszlenart · 2026-10-05T19:19:47Z

+    <include file="xwork-test-default.xml"/>
+    <package name="alias-authorization" extends="xwork-test-default">
+
+        <!-- Neither raw source name resolves on the stack, forcing AliasInterceptor's documented


Same here: the action name and aliases make the fixture's purpose clear, so the comment block can go.

Copilot

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

Addresses review feedback from lukaszlenart on PR apache#1991 / WW-5758: 1. The stack-to-stack branch now follows struts.chaining.requireAnnotations (default false, matching WW-5631's ChainingInterceptor precedent) instead of being unconditionally exempt. An application that has turned struts.chaining.requireAnnotations on now gets the same enforcement on an aliased copy of an already-resolved value that it gets from chaining. 2. Rejections now log at WARN, matching ChainingInterceptor.java:250, since this is a behavior change for existing applications using the documented aliases example. 3. Trimmed the in-file comment to the one thing not visible from the code (the WW-5631 split); the full reasoning is in the PR description.

g0w6y · 2026-10-06T02:00:30Z

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 · 2026-10-06T05:22:53Z

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:

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?
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.
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.
default.properties: the description of struts.chaining.requireAnnotations still says "Whether ChainingInterceptor enforces @StrutsParameter...". Please mention AliasInterceptor there as well.
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.

Addresses the rest of lukaszlenart's review on PR apache#1991 / WW-5758: - Fix: the @StrutsParameter check now runs only inside the value.isDefined() branch, so an alias whose source never resolves at all (neither on the stack nor as a request parameter) is no longer spuriously warned about and rejected - there was nothing to copy either way. - WARN wording now matches his suggested text exactly ('not set on', not 'not copied to'). - Trimmed every flagged comment: the production fallback comment back to one line, the WW-5631 cross-reference to one line above the check, removed the two test-method comment blocks (name and assertions already say what they said), removed the field comments in AliasAuthorizationTestAction (clear from field names and the test), removed the XML fixture's comment block (clear from the action name and aliases). - Changed the HTTP parameter value in the first test from 'PWNED_VIA_ALIAS' to the plain 'value-from-request', matching the other fixtures in this class. - setRequireChainingAnnotations now uses BooleanUtils.toBoolean, matching ChainingInterceptor.setRequireAnnotations exactly. - default.properties: struts.chaining.requireAnnotations now mentions AliasInterceptor alongside ChainingInterceptor. - New test (testNoWarningWhenAliasSourceNeverResolvesAtAll, using LogCapture) proves the spurious- warning bug is fixed; red/green proven by temporarily reverting the check's position.

…ptor-authorization

g0w6y · 2026-10-06T13:57:02Z

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).

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.
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.
Test value changed from PWNED_VIA_ALIAS to value-from-request, matching the other fixtures.
setRequireChainingAnnotations now uses BooleanUtils.toBoolean, matching ChainingInterceptor.setRequireAnnotations.
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 · 2026-10-06T14:17:58Z

Docs are up: apache/struts-site#347

lukaszlenart requested changes Oct 5, 2026

View reviewed changes

lukaszlenart requested a balanced review from Copilot October 5, 2026 19:21

Copilot AI reviewed Oct 5, 2026

g0w6y added 2 commits October 6, 2026 19:23

Merge remote-tracking branch 'origin/main' into WW-5758-alias-interce…

c132367

…ptor-authorization

g0w6y mentioned this pull request Oct 6, 2026

Document AliasInterceptor's @StrutsParameter authorization apache/struts-site#347

Open


		private String annotatedTarget;

		// Deliberately NOT annotated with @StrutsParameter: an alias may not use this as a copy target

Conversation

g0w6y commented Oct 5, 2026

Uh oh!

lukaszlenart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

g0w6y commented Oct 6, 2026

Uh oh!

lukaszlenart commented Oct 6, 2026

Uh oh!

g0w6y commented Oct 6, 2026

Uh oh!

g0w6y commented Oct 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants