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 2f712f110 WW-5710 fix(core): prime the OGNL allowlist against the
action as well as the model (#1932)
2f712f110 is described below
commit 2f712f110a23219726862b0c7bca1e0bc3a046e5
Author: Lukasz Lenart <[email protected]>
AuthorDate: Sun Sep 13 19:37:14 2026 +0200
WW-5710 fix(core): prime the OGNL allowlist against the action as well as
the model (#1932)
Both OGNL channels resolved the binding target once and used it for the
authorization check and the allowlist priming alike. For a ModelDriven
action that target is the model, but since WW-5698 the authorizer may
grant a parameter on a member declared on the action itself. The
allowlister was still handed the model only, found no annotation there
and primed nothing, so with struts.allowlist.enable=true - the default -
a correctly annotated nested property on the action's own class was
authorized and then refused by SecurityMemberAccess, with nothing in the
logs pointing at the allowlist.
ParameterAllowlister gains a default three-argument form that primes the
resolved target and, when it differs, the action too; ParametersInterceptor
and CookieInterceptor use it. Each priming is a no-op unless that object
annotates the root property, so the second priming cannot allowlist
anything the developer did not declare.
Co-authored-by: Claude Opus 5 (1M context) <[email protected]>
---
.../struts2/interceptor/CookieInterceptor.java | 2 +-
.../parameter/OgnlParameterAllowlister.java | 9 ++--
.../parameter/ParameterAllowlister.java | 18 +++++++
.../parameter/ParametersInterceptor.java | 2 +-
.../struts2/interceptor/CookieInterceptorTest.java | 57 ++++++++++++++++++++++
.../parameter/ParametersInterceptorTest.java | 53 ++++++++++++++++++++
.../parameter/StrutsParameterAnnotationTest.java | 31 ++++++++++++
7 files changed, 165 insertions(+), 7 deletions(-)
diff --git
a/core/src/main/java/org/apache/struts2/interceptor/CookieInterceptor.java
b/core/src/main/java/org/apache/struts2/interceptor/CookieInterceptor.java
index 4d9685543..551887d4b 100644
--- a/core/src/main/java/org/apache/struts2/interceptor/CookieInterceptor.java
+++ b/core/src/main/java/org/apache/struts2/interceptor/CookieInterceptor.java
@@ -358,7 +358,7 @@ public class CookieInterceptor extends AbstractInterceptor {
LOG.debug("Cookie [{}] rejected by @StrutsParameter authorization
on target [{}]", cookieName, target.getClass().getSimpleName());
return;
}
- parameterAllowlister.primeAllowlistForPath(cookieName, target);
+ parameterAllowlister.primeAllowlistForPath(cookieName, target, action);
populateCookieValueIntoStack(cookieName, cookieValue, cookiesMap,
stack);
}
diff --git
a/core/src/main/java/org/apache/struts2/interceptor/parameter/OgnlParameterAllowlister.java
b/core/src/main/java/org/apache/struts2/interceptor/parameter/OgnlParameterAllowlister.java
index 0a24d8577..71e8f20fc 100644
---
a/core/src/main/java/org/apache/struts2/interceptor/parameter/OgnlParameterAllowlister.java
+++
b/core/src/main/java/org/apache/struts2/interceptor/parameter/OgnlParameterAllowlister.java
@@ -52,7 +52,7 @@ import static
org.apache.struts2.security.DefaultAcceptedPatternsChecker.NESTING
* <li>{@code paramDepth == 0} — shallow setter; OGNL does not need to
traverse</li>
* <li>the root property has no {@code @StrutsParameter} annotation
reachable via {@link java.beans.PropertyDescriptor}
* or as a public field (e.g. a {@code ModelDriven} model whose
properties are not individually annotated). A
- * {@code LOG.debug} surfaces this case so the gap between authorization
and OGNL traversal is observable.</li>
+ * {@code LOG.debug} names the object that primed nothing, so a path
OGNL then refuses can be traced back.</li>
* </ul>
*
* @since 7.2.0
@@ -100,10 +100,9 @@ public class OgnlParameterAllowlister implements
ParameterAllowlister {
if (allowlistViaPublicField(target, normalisedRootProperty,
paramDepth)) {
return;
}
- // Authorization passed but no @StrutsParameter on the root property —
e.g. ModelDriven model with no
- // per-property annotations. OGNL won't be able to walk this nested
path; surface the gap in logs.
- LOG.debug("Parameter [{}] authorized but no @StrutsParameter on root
property [{}] of [{}]; "
- + "OGNL allowlist not primed and nested traversal may be
blocked",
+ // No @StrutsParameter on the root property of this object - e.g. a
ModelDriven model with no per-property
+ // annotations. Whether OGNL can still walk the path depends on the
other objects primed for it.
+ LOG.debug("Parameter [{}] has no @StrutsParameter on root property
[{}] of [{}]; nothing allowlisted from it",
parameterName, normalisedRootProperty,
ultimateClass(target).getSimpleName());
}
diff --git
a/core/src/main/java/org/apache/struts2/interceptor/parameter/ParameterAllowlister.java
b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParameterAllowlister.java
index 846498c71..b70f7bbe7 100644
---
a/core/src/main/java/org/apache/struts2/interceptor/parameter/ParameterAllowlister.java
+++
b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParameterAllowlister.java
@@ -41,4 +41,22 @@ public interface ParameterAllowlister {
* @param target the object receiving the parameter value (the
action, or the model for ModelDriven actions)
*/
void primeAllowlistForPath(String parameterName, Object target);
+
+ /**
+ * Primes for a parameter that {@link ParameterAuthorizer#isAuthorized}
may have granted on either object: a
+ * {@link org.apache.struts2.ModelDriven} action's own annotated members
are authorized on the action while the
+ * resolved target is its model, so both are primed. Each priming is a
no-op unless that object annotates the
+ * root property, so priming the second object cannot allowlist anything
the developer did not declare.
+ *
+ * @param parameterName the parameter name (e.g. {@code "user.role"},
{@code "items[0].name"})
+ * @param target the object receiving the parameter value, as
resolved by {@link ParameterAuthorizer#resolveTarget}
+ * @param action the action instance
+ * @since 7.4.0
+ */
+ default void primeAllowlistForPath(String parameterName, Object target,
Object action) {
+ primeAllowlistForPath(parameterName, target);
+ if (target != action) {
+ primeAllowlistForPath(parameterName, action);
+ }
+ }
}
diff --git
a/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java
b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java
index 365868a2b..22d575eab 100644
---
a/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java
+++
b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java
@@ -378,7 +378,7 @@ public class ParametersInterceptor extends
MethodFilterInterceptor {
return false;
}
- parameterAllowlister.primeAllowlistForPath(name, target);
+ parameterAllowlister.primeAllowlistForPath(name, target, action);
return true;
}
diff --git
a/core/src/test/java/org/apache/struts2/interceptor/CookieInterceptorTest.java
b/core/src/test/java/org/apache/struts2/interceptor/CookieInterceptorTest.java
index 8bcfe704c..0314be46a 100644
---
a/core/src/test/java/org/apache/struts2/interceptor/CookieInterceptorTest.java
+++
b/core/src/test/java/org/apache/struts2/interceptor/CookieInterceptorTest.java
@@ -29,7 +29,11 @@ import jakarta.servlet.http.Cookie;
import org.apache.struts2.ServletActionContext;
import org.apache.struts2.StrutsInternalTestCase;
import org.apache.struts2.action.CookiesAware;
+import org.apache.struts2.ModelDriven;
+import org.apache.struts2.interceptor.parameter.ParameterAllowlister;
+import org.apache.struts2.interceptor.parameter.ParameterAuthorizer;
import org.apache.struts2.interceptor.parameter.StrutsParameter;
+import org.apache.struts2.ognl.ThreadAllowlist;
import org.springframework.mock.web.MockHttpServletRequest;
import java.util.Collections;
@@ -471,6 +475,59 @@ public class CookieInterceptorTest extends
StrutsInternalTestCase {
assertFalse(excludedName.get(reqCookieName));
}
+ /**
+ * WW-5710: a nested cookie path annotated on the ModelDriven action
itself is authorized on the action, so the
+ * allowlist has to be primed against the action as well, not only against
the model.
+ */
+ public void testNestedCookieOnModelDrivenActionMemberPrimesAllowlist()
throws Exception {
+ MockHttpServletRequest request = new MockHttpServletRequest();
+ request.setCookies(new Cookie("address.city", "London"));
+ ServletActionContext.setRequest(request);
+
+ ModelDrivenActionWithNestedMember action = new
ModelDrivenActionWithNestedMember();
+ ActionContext.getContext().getValueStack().push(action);
+ ActionContext.getContext().getValueStack().push(action.getModel());
+
+ ActionInvocation invocation = (ActionInvocation)
createMock(ActionInvocation.class);
+ expect(invocation.getAction()).andReturn(action);
+ expect(invocation.invoke()).andReturn(Action.SUCCESS);
+ replay(invocation);
+
+ CookieInterceptor interceptor = new CookieInterceptor();
+ interceptor.setCookiesName("*");
+ interceptor.setExcludedPatternsChecker(new
DefaultExcludedPatternsChecker());
+ interceptor.setAcceptedPatternsChecker(new
DefaultAcceptedPatternsChecker());
+
interceptor.setParameterAuthorizer(container.getInstance(ParameterAuthorizer.class));
+
interceptor.setParameterAllowlister(container.getInstance(ParameterAllowlister.class));
+ ThreadAllowlist threadAllowlist =
container.getInstance(ThreadAllowlist.class);
+
+ try {
+ interceptor.intercept(invocation);
+ assertTrue(threadAllowlist.getAllowlist().contains(Address.class));
+ assertEquals("London", action.getAddress().getCity());
+ } finally {
+ threadAllowlist.clearAllowlist();
+ }
+ verify(invocation);
+ }
+
+ public static class ModelDrivenActionWithNestedMember extends
ActionSupport implements ModelDriven<Object> {
+ private final Object model = new Object();
+ private final Address address = new Address();
+
+ @Override
+ public Object getModel() { return model; }
+
+ @StrutsParameter(depth = 1)
+ public Address getAddress() { return address; }
+ }
+
+ public static class Address {
+ private String city;
+ public String getCity() { return city; }
+ public void setCity(String city) { this.city = city; }
+ }
+
public static class MockActionWithCookieAware extends ActionSupport
implements CookiesAware {
private static final long serialVersionUID = -6202290616812813386L;
diff --git
a/core/src/test/java/org/apache/struts2/interceptor/parameter/ParametersInterceptorTest.java
b/core/src/test/java/org/apache/struts2/interceptor/parameter/ParametersInterceptorTest.java
index c5c7fffc0..de17ba7cb 100644
---
a/core/src/test/java/org/apache/struts2/interceptor/parameter/ParametersInterceptorTest.java
+++
b/core/src/test/java/org/apache/struts2/interceptor/parameter/ParametersInterceptorTest.java
@@ -38,6 +38,7 @@ import org.apache.struts2.interceptor.ValidationAware;
import org.apache.struts2.mock.MockActionInvocation;
import org.apache.struts2.ognl.OgnlValueStack;
import org.apache.struts2.ognl.OgnlValueStackFactory;
+import org.apache.struts2.ognl.ThreadAllowlist;
import org.apache.struts2.ognl.SecurityMemberAccess;
import org.apache.struts2.ognl.accessor.CompoundRootAccessor;
import org.apache.struts2.ognl.accessor.RootAccessor;
@@ -283,6 +284,41 @@ public class ParametersInterceptorTest extends
XWorkTestCase {
assertNull(action.getSecret());
}
+ /**
+ * WW-5710: a nested property annotated on the ModelDriven action itself
is authorized on the action, but the
+ * allowlist used to be primed against the model only, so with the
allowlist enabled (the default) OGNL refused
+ * the path the authorizer had just granted. The model property alongside
proves the parameters were applied.
+ */
+ public void testModelDrivenAnnotatedNestedPropertyOnActionIsAllowlisted()
throws Exception {
+ loadButSet(Map.of(
+ StrutsConstants.STRUTS_PARAMETERS_REQUIRE_ANNOTATIONS, "true",
+ StrutsConstants.STRUTS_ALLOWLIST_ENABLE, "true"));
+ ParametersInterceptor pi = createParametersInterceptor();
+
+ NestedModelDrivenAction action = new NestedModelDrivenAction();
+ ValueStack stack =
container.getInstance(ValueStackFactory.class).createValueStack();
+ stack.push(action);
+ stack.push(action.getModel());
+
ActionContext.of().withContainer(container).withValueStack(stack).bind();
+ // What configuration loading does for the action class and
ModelDrivenInterceptor for the model;
+ // Address is allowlisted by nothing but the priming under test
+ ThreadAllowlist threadAllowlist =
container.getInstance(ThreadAllowlist.class);
+ threadAllowlist.allowClassHierarchy(NestedModelDrivenAction.class);
+ threadAllowlist.allowClassHierarchy(TestBean.class);
+
+ Map<String, Object> params = new HashMap<>();
+ params.put("address.city", "bound through the action's annotated
getter");
+ params.put("name", "bound on the model");
+ pi.applyParameters(action, stack,
HttpParameters.create(params).build());
+
+ try {
+ assertEquals("bound on the model", action.getModel().getName());
+ assertEquals("bound through the action's annotated getter",
action.getAddress().getCity());
+ } finally {
+ threadAllowlist.clearAllowlist();
+ }
+ }
+
public void testParametersDoesNotAffectSession() throws Exception {
Map<String, Object> params = new HashMap<>();
params.put("blah", "This is blah");
@@ -1041,6 +1077,23 @@ public class ParametersInterceptorTest extends
XWorkTestCase {
public String getAllowed() { return allowed; }
}
+ public static class NestedModelDrivenAction implements
ModelDriven<TestBean> {
+ private final TestBean model = new TestBean();
+ private final Address address = new Address();
+
+ @Override
+ public TestBean getModel() { return model; }
+
+ @StrutsParameter(depth = 1)
+ public Address getAddress() { return address; }
+ }
+
+ public static class Address {
+ private String city;
+ public String getCity() { return city; }
+ public void setCity(String city) { this.city = city; }
+ }
+
private class NoParametersAction implements Action, NoParameters {
public String execute() throws Exception {
diff --git
a/core/src/test/java/org/apache/struts2/interceptor/parameter/StrutsParameterAnnotationTest.java
b/core/src/test/java/org/apache/struts2/interceptor/parameter/StrutsParameterAnnotationTest.java
index 8ec445253..d4bc9f43d 100644
---
a/core/src/test/java/org/apache/struts2/interceptor/parameter/StrutsParameterAnnotationTest.java
+++
b/core/src/test/java/org/apache/struts2/interceptor/parameter/StrutsParameterAnnotationTest.java
@@ -424,6 +424,23 @@ public class StrutsParameterAnnotationTest {
testParameter(proxiedAction, "name.nested", true);
}
+ /**
+ * WW-5710: a nested property annotated on the ModelDriven action itself
is authorized on the action, so the
+ * allowlist has to be primed against the action as well, not only against
the model.
+ */
+ @Test
+ public void
modelDrivenAction_annotatedNestedPropertyOnAction_allowlisted() {
+ var action = new ModelActionWithNestedProperty();
+
+ // Emulate ModelDrivenInterceptor running previously
+ var valueStack = new StubValueStack();
+ valueStack.push(action.getModel());
+ ActionContext.of().withValueStack(valueStack).bind();
+
+ testParameter(action, "publicPojo.key", true);
+
assertThat(threadAllowlist.getAllowlist()).containsExactlyInAnyOrderElementsOf(getParentClasses(Pojo.class));
+ }
+
public static class FieldAction {
@StrutsParameter
private String privateStr;
@@ -516,6 +533,20 @@ public class StrutsParameterAnnotationTest {
}
}
+ public static class ModelActionWithNestedProperty implements
ModelDriven<Pojo> {
+ private final Pojo model = new Pojo();
+
+ @Override
+ public Pojo getModel() {
+ return model;
+ }
+
+ @StrutsParameter(depth = 1)
+ public Pojo getPublicPojo() {
+ return null;
+ }
+ }
+
public static class Pojo {
}
}
