This is an automated email from the ASF dual-hosted git repository.
lukaszlenart pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/struts.git
The following commit(s) were added to refs/heads/main by this push:
new 04373ae8b WW-5758 Enforce @StrutsParameter authorization in
AliasInterceptor's raw-parameter fallback (#1991)
04373ae8b is described below
commit 04373ae8b1f9415430ccb44d4ceedd59a6eed3c3
Author: ⳕⲛτⲉⲅⲥⲉⳏτⲟⲅ 🕵🏻 <[email protected]>
AuthorDate: Tue Oct 6 23:38:08 2026 +0530
WW-5758 Enforce @StrutsParameter authorization in AliasInterceptor's
raw-parameter fallback (#1991)
* WW-5758 Enforce @StrutsParameter authorization in AliasInterceptor's
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.
* Gate the stack-resolved alias copy on struts.chaining.requireAnnotations
Addresses review feedback from lukaszlenart on PR #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.
* Fix spurious WARN, trim comments, address remaining review feedback
Addresses the rest of lukaszlenart's review on PR #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.
---
.../struts2/interceptor/AliasInterceptor.java | 28 +++++++-
.../org/apache/struts2/default.properties | 6 +-
.../struts2/AliasAuthorizationTestAction.java | 68 ++++++++++++++++++
.../struts2/interceptor/AliasInterceptorTest.java | 82 ++++++++++++++++++++++
.../test/resources/struts-alias-authorization.xml | 38 ++++++++++
5 files changed, 218 insertions(+), 4 deletions(-)
diff --git
a/core/src/main/java/org/apache/struts2/interceptor/AliasInterceptor.java
b/core/src/main/java/org/apache/struts2/interceptor/AliasInterceptor.java
index 37b5c383c..7ec93656a 100644
--- a/core/src/main/java/org/apache/struts2/interceptor/AliasInterceptor.java
+++ b/core/src/main/java/org/apache/struts2/interceptor/AliasInterceptor.java
@@ -18,6 +18,7 @@
*/
package org.apache.struts2.interceptor;
+import org.apache.commons.lang3.BooleanUtils;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.apache.struts2.ActionContext;
@@ -28,6 +29,7 @@ import org.apache.struts2.config.entities.ActionConfig;
import org.apache.struts2.dispatcher.HttpParameters;
import org.apache.struts2.dispatcher.Parameter;
import org.apache.struts2.inject.Inject;
+import org.apache.struts2.interceptor.parameter.ParameterAuthorizer;
import org.apache.struts2.security.AcceptedPatternsChecker;
import org.apache.struts2.security.ExcludedPatternsChecker;
import org.apache.struts2.util.ClearableValueStack;
@@ -101,15 +103,22 @@ public class AliasInterceptor extends AbstractInterceptor
{
protected ValueStackFactory valueStackFactory;
protected LocalizedTextProvider localizedTextProvider;
protected boolean devMode = false;
+ private boolean requireChainingAnnotations = false;
private ExcludedPatternsChecker excludedPatterns;
private AcceptedPatternsChecker acceptedPatterns;
+ private ParameterAuthorizer parameterAuthorizer;
@Inject(StrutsConstants.STRUTS_DEVMODE)
public void setDevMode(String mode) {
this.devMode = Boolean.parseBoolean(mode);
}
+ @Inject(value = StrutsConstants.STRUTS_CHAINING_REQUIRE_ANNOTATIONS,
required = false)
+ public void setRequireChainingAnnotations(String
requireChainingAnnotations) {
+ this.requireChainingAnnotations =
BooleanUtils.toBoolean(requireChainingAnnotations);
+ }
+
@Inject
public void setValueStackFactory(ValueStackFactory valueStackFactory) {
this.valueStackFactory = valueStackFactory;
@@ -130,6 +139,11 @@ public class AliasInterceptor extends AbstractInterceptor {
this.acceptedPatterns = acceptedPatterns;
}
+ @Inject
+ public void setParameterAuthorizer(ParameterAuthorizer
parameterAuthorizer) {
+ this.parameterAuthorizer = parameterAuthorizer;
+ }
+
/**
* <p>
* Sets the name of the action parameter to look for the alias map.
@@ -161,6 +175,8 @@ public class AliasInterceptor extends AbstractInterceptor {
Object obj = stack.findValue(aliasExpression);
if (obj instanceof Map aliases) {
+ Object authorizationTarget =
parameterAuthorizer.resolveTarget(action);
+
//get secure stack
ValueStack newStack =
valueStackFactory.createValueStack(stack);
boolean clearableStack = newStack instanceof
ClearableValueStack;
@@ -189,18 +205,28 @@ public class AliasInterceptor extends AbstractInterceptor
{
continue;
}
Evaluated value = new Evaluated(stack.findValue(name));
+ boolean fromRawRequestParameter = false;
if (!value.isDefined()) {
- // workaround
+ // name did not resolve on the stack, fall back to the
request parameter
HttpParameters contextParameters =
ActionContext.getContext().getParameters();
if (null != contextParameters) {
Parameter param = contextParameters.get(name);
if (param.isDefined()) {
value = new Evaluated(param.getValue());
+ fromRawRequestParameter = true;
}
}
}
if (value.isDefined()) {
+ // split follows ChainingInterceptor/WW-5631:
parameters.requireAnnotations gates
+ // the request-parameter fallback,
chaining.requireAnnotations gates the rest
+ if ((fromRawRequestParameter ||
requireChainingAnnotations)
+ && !parameterAuthorizer.isAuthorized(alias,
authorizationTarget, action)) {
+ LOG.warn("Alias: property [{}] not set on [{}]
because it is not annotated with @StrutsParameter",
+ alias, action.getClass().getName());
+ continue;
+ }
try {
newStack.setValue(alias, value.get());
} catch (RuntimeException e) {
diff --git a/core/src/main/resources/org/apache/struts2/default.properties
b/core/src/main/resources/org/apache/struts2/default.properties
index dfaf09a4e..cacf316da 100644
--- a/core/src/main/resources/org/apache/struts2/default.properties
+++ b/core/src/main/resources/org/apache/struts2/default.properties
@@ -284,9 +284,9 @@ struts.parameters.requireAnnotations=true
### Useful for transitioning legacy applications, but highly recommended to
set to false as soon as possible!
struts.parameters.requireAnnotations.transitionMode=false
-### Whether ChainingInterceptor enforces @StrutsParameter on the target action
when copying properties.
-### Opt-in hardening; default false preserves legacy chaining behaviour. Only
has effect when
-### struts.parameters.requireAnnotations is also enabled.
+### Whether ChainingInterceptor and AliasInterceptor enforce @StrutsParameter
on the target action when
+### copying a value already resolved on the stack. Opt-in hardening; default
false preserves legacy
+### chaining/aliasing behaviour. Only has effect when
struts.parameters.requireAnnotations is also enabled.
struts.chaining.requireAnnotations=false
### Whether to throw a RuntimeException when a property is not found
diff --git
a/core/src/test/java/org/apache/struts2/AliasAuthorizationTestAction.java
b/core/src/test/java/org/apache/struts2/AliasAuthorizationTestAction.java
new file mode 100644
index 000000000..f326e875b
--- /dev/null
+++ b/core/src/test/java/org/apache/struts2/AliasAuthorizationTestAction.java
@@ -0,0 +1,68 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.struts2;
+
+import org.apache.struts2.interceptor.parameter.StrutsParameter;
+
+/**
+ * A dedicated, minimal action for {@code AliasInterceptorTest}'s {@code
@StrutsParameter} authorization
+ * coverage, kept separate from the widely-shared {@link SimpleAction} so
these tests cannot be affected
+ * by, or affect, the dozens of other suites that reuse that fixture.
+ */
+public class AliasAuthorizationTestAction extends ActionSupport {
+
+ private String annotatedTarget;
+ private String unannotatedTarget = "untouched";
+ private String preExistingSource = "preset-value";
+ private String stackToStackTarget = "untouched";
+
+ @StrutsParameter
+ public String getAnnotatedTarget() {
+ return annotatedTarget;
+ }
+
+ @StrutsParameter
+ public void setAnnotatedTarget(String annotatedTarget) {
+ this.annotatedTarget = annotatedTarget;
+ }
+
+ public String getUnannotatedTarget() {
+ return unannotatedTarget;
+ }
+
+ public void setUnannotatedTarget(String unannotatedTarget) {
+ this.unannotatedTarget = unannotatedTarget;
+ }
+
+ public String getPreExistingSource() {
+ return preExistingSource;
+ }
+
+ public void setPreExistingSource(String preExistingSource) {
+ this.preExistingSource = preExistingSource;
+ }
+
+ public String getStackToStackTarget() {
+ return stackToStackTarget;
+ }
+
+ public void setStackToStackTarget(String stackToStackTarget) {
+ this.stackToStackTarget = stackToStackTarget;
+ }
+}
diff --git
a/core/src/test/java/org/apache/struts2/interceptor/AliasInterceptorTest.java
b/core/src/test/java/org/apache/struts2/interceptor/AliasInterceptorTest.java
index 35b4811a8..f588af43e 100644
---
a/core/src/test/java/org/apache/struts2/interceptor/AliasInterceptorTest.java
+++
b/core/src/test/java/org/apache/struts2/interceptor/AliasInterceptorTest.java
@@ -21,15 +21,22 @@ package org.apache.struts2.interceptor;
import org.apache.struts2.action.Action;
import org.apache.struts2.ActionContext;
import org.apache.struts2.ActionProxy;
+import org.apache.struts2.AliasAuthorizationTestAction;
import org.apache.struts2.SimpleAction;
import org.apache.struts2.SimpleFooAction;
+import org.apache.struts2.StrutsConstants;
import org.apache.struts2.XWorkTestCase;
import org.apache.struts2.config.entities.ActionConfig;
+import org.apache.struts2.config.ConfigurationException;
import org.apache.struts2.config.providers.XmlConfigurationProvider;
+import org.apache.struts2.inject.ContainerBuilder;
import org.apache.struts2.mock.MockActionInvocation;
import org.apache.struts2.mock.MockActionProxy;
import org.apache.struts2.config.StrutsXmlConfigurationProvider;
import org.apache.struts2.dispatcher.HttpParameters;
+import org.apache.struts2.test.LogCapture;
+import org.apache.struts2.test.StubConfigurationProvider;
+import org.apache.struts2.util.location.LocatableProperties;
import java.util.HashMap;
import java.util.Map;
@@ -132,6 +139,81 @@ public class AliasInterceptorTest extends XWorkTestCase {
return (SimpleAction) proxy.getAction();
}
+ public void testUnannotatedAliasTargetIsRejected() throws Exception {
+ Map<String, Object> httpParams = new HashMap<>();
+ httpParams.put("rawSourceAnnotated", "allowed-value");
+ httpParams.put("rawSourceUnannotated", "value-from-request");
+ ActionContext extraContext =
ActionContext.of().withParameters(HttpParameters.create(httpParams).build());
+
+ XmlConfigurationProvider provider = new
StrutsXmlConfigurationProvider("struts-alias-authorization.xml");
+ container.inject(provider);
+ // loadConfigurationProviders rebuilds the whole configuration each
call, so this property has to
+ // be registered in the same call as the XML provider, not a separate
loadButSet() call before it.
+ StubConfigurationProvider requireAnnotationsOn = new
StubConfigurationProvider() {
+ @Override
+ public void register(ContainerBuilder builder, LocatableProperties
props) throws ConfigurationException {
+
props.setProperty(StrutsConstants.STRUTS_PARAMETERS_REQUIRE_ANNOTATIONS,
"true");
+ }
+ };
+ loadConfigurationProviders(provider, requireAnnotationsOn);
+
+ ActionProxy proxy = actionProxyFactory.createActionProxy("",
"aliasAuthorizationTest", null, extraContext.getContextMap());
+ proxy.execute();
+ AliasAuthorizationTestAction action = (AliasAuthorizationTestAction)
proxy.getAction();
+
+ assertEquals("allowed-value", action.getAnnotatedTarget());
+ assertEquals("untouched", action.getUnannotatedTarget());
+ assertEquals("preset-value", action.getStackToStackTarget());
+ }
+
+ public void
testUnannotatedStackToStackAliasTargetIsRejectedWhenChainingAnnotationsRequired()
throws Exception {
+ ActionContext extraContext =
ActionContext.of().withParameters(HttpParameters.create(Map.of()).build());
+
+ XmlConfigurationProvider provider = new
StrutsXmlConfigurationProvider("struts-alias-authorization.xml");
+ container.inject(provider);
+ StubConfigurationProvider requireAnnotationsOn = new
StubConfigurationProvider() {
+ @Override
+ public void register(ContainerBuilder builder, LocatableProperties
props) throws ConfigurationException {
+
props.setProperty(StrutsConstants.STRUTS_PARAMETERS_REQUIRE_ANNOTATIONS,
"true");
+
props.setProperty(StrutsConstants.STRUTS_CHAINING_REQUIRE_ANNOTATIONS, "true");
+ }
+ };
+ loadConfigurationProviders(provider, requireAnnotationsOn);
+
+ ActionProxy proxy = actionProxyFactory.createActionProxy("",
"aliasAuthorizationTest", null, extraContext.getContextMap());
+ proxy.execute();
+ AliasAuthorizationTestAction action = (AliasAuthorizationTestAction)
proxy.getAction();
+
+ assertEquals("untouched", action.getStackToStackTarget());
+ }
+
+ // unannotatedTarget's source (rawSourceUnannotated) never resolves at all
here: not on the stack, and
+ // no matching HTTP parameter either. The authorization check must not run
for it, so it must not be
+ // warned about - only stackToStackTarget's source actually resolves and
is legitimately rejected.
+ public void testNoWarningWhenAliasSourceNeverResolvesAtAll() throws
Exception {
+ ActionContext extraContext =
ActionContext.of().withParameters(HttpParameters.create(Map.of()).build());
+
+ XmlConfigurationProvider provider = new
StrutsXmlConfigurationProvider("struts-alias-authorization.xml");
+ container.inject(provider);
+ StubConfigurationProvider requireAnnotationsOn = new
StubConfigurationProvider() {
+ @Override
+ public void register(ContainerBuilder builder, LocatableProperties
props) throws ConfigurationException {
+
props.setProperty(StrutsConstants.STRUTS_PARAMETERS_REQUIRE_ANNOTATIONS,
"true");
+
props.setProperty(StrutsConstants.STRUTS_CHAINING_REQUIRE_ANNOTATIONS, "true");
+ }
+ };
+ loadConfigurationProviders(provider, requireAnnotationsOn);
+
+ ActionProxy proxy = actionProxyFactory.createActionProxy("",
"aliasAuthorizationTest", null, extraContext.getContextMap());
+ try (LogCapture logCapture = new LogCapture(AliasInterceptor.class)) {
+ proxy.execute();
+ for (String message : logCapture.messages()) {
+ assertFalse("unannotatedTarget's source never resolved, so it
must not be warned about: " + message,
+ message.contains("unannotatedTarget"));
+ }
+ }
+ }
+
public void testNameNotAccepted() throws Exception {
Map<String, Object> params = new HashMap<>();
params.put("aliasSource", "source here");
diff --git a/core/src/test/resources/struts-alias-authorization.xml
b/core/src/test/resources/struts-alias-authorization.xml
new file mode 100644
index 000000000..d2329ee91
--- /dev/null
+++ b/core/src/test/resources/struts-alias-authorization.xml
@@ -0,0 +1,38 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+-->
+<!DOCTYPE struts PUBLIC
+ "-//Apache Software Foundation//DTD Struts Configuration 6.0//EN"
+ "https://struts.apache.org/dtds/struts-6.0.dtd";>
+<struts>
+ <include file="xwork-test-default.xml"/>
+ <package name="alias-authorization" extends="xwork-test-default">
+
+ <action name="aliasAuthorizationTest"
class="org.apache.struts2.AliasAuthorizationTestAction">
+ <param name="aliases">#{ "rawSourceAnnotated" : "annotatedTarget",
"rawSourceUnannotated" : "unannotatedTarget", "preExistingSource" :
"stackToStackTarget" }</param>
+ <interceptor-ref name="alias"/>
+ <interceptor-ref name="params"/>
+ <result name="success" type="mock"/>
+ <result name="error" type="mock"/>
+ </action>
+
+ </package>
+</struts>
