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 690c4c273 WW-5626 cleanup follow-ups for @StrutsParameter JSON/REST
enforcement (#1673)
690c4c273 is described below
commit 690c4c27371aca74abbc0c824d40764a237c7bef
Author: Lukasz Lenart <[email protected]>
AuthorDate: Sat May 9 12:09:03 2026 +0200
WW-5626 cleanup follow-ups for @StrutsParameter JSON/REST enforcement
(#1673)
* WW-5626 add ParameterAuthorizer#resolveTarget for centralized ModelDriven
resolution
Move the ValueStack peek logic that derives the target object from
action+ModelDriven
state out of ParametersInterceptor and into ParameterAuthorizer. Callers
that need
both authorization and the resolved target (for downstream OGNL
allowlisting) can
now call resolveTarget once and reuse the result.
* WW-5626 delegate ModelDriven target resolution to ParameterAuthorizer
Replace the inline ValueStack peek in
ParametersInterceptor#isParameterAnnotatedAndAllowlist
with a call to ParameterAuthorizer#resolveTarget. The ModelDriven import is
no longer
needed in this class.
* WW-5626 defensively skip non-String JSON keys in authorization filter
The (String) cast in filterUnauthorizedKeysRecursive threw
ClassCastException
for any custom JSONReader producing non-String keys. Replace with an
instanceof
pattern that debug-logs and skips entries whose key cannot be converted to a
parameter path.
* WW-5626 add real JacksonJsonHandler integration tests for
@StrutsParameter filtering
The existing ContentTypeInterceptorTest uses mock ContentTypeHandlers, so
its
requireAnnotations=true tests verify only that intercept() returns SUCCESS
— they
assert nothing about which properties were actually filtered. These
integration
tests use a real JacksonJsonHandler + a real StrutsParameterAuthorizer to
verify
end-to-end property-level filtering for top-level annotated/unannotated
properties
and nested properties at varying authorized depths.
The SecureRestAction fixture documents a semantic divergence: REST's
recursive
copy authorizes each path level independently, so depth-0 authorization on
the
top-level property requires @StrutsParameter on the setter even when nested
field access is the actual goal. ParametersInterceptor only requires the
getter
annotation. This divergence is tracked for the Approach C refactor.
* WW-5626 make ParameterAuthorizer#resolveTarget a default method to
preserve SAM
Making resolveTarget abstract broke ParameterAuthorizer as a functional
interface,
which the existing JSON and REST plugin tests rely on for lambda-based
stubs:
interceptor.setParameterAuthorizer((parameterName, target, action) ->
true);
The default returns the action unchanged — adequate for lambda test stubs
whose
authorization decisions don't depend on the resolved target. The production
implementation (StrutsParameterAuthorizer) overrides this with the proper
ModelDriven value-stack peek.
---
.../interceptor/parameter/ParameterAuthorizer.java | 20 +++
.../parameter/ParametersInterceptor.java | 10 +-
.../parameter/StrutsParameterAuthorizer.java | 12 ++
.../parameter/ParameterAuthorizerTest.java | 39 ++++++
.../org/apache/struts2/json/JSONInterceptor.java | 9 +-
.../apache/struts2/json/JSONInterceptorTest.java | 26 ++++
.../ContentTypeInterceptorIntegrationTest.java | 134 +++++++++++++++++++++
.../org/apache/struts2/rest/SecureRestAction.java | 70 +++++++++++
8 files changed, 310 insertions(+), 10 deletions(-)
diff --git
a/core/src/main/java/org/apache/struts2/interceptor/parameter/ParameterAuthorizer.java
b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParameterAuthorizer.java
index d064fbf68..6c5848bd1 100644
---
a/core/src/main/java/org/apache/struts2/interceptor/parameter/ParameterAuthorizer.java
+++
b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParameterAuthorizer.java
@@ -45,4 +45,24 @@ public interface ParameterAuthorizer {
* @return {@code true} if the parameter is authorized for injection,
{@code false} otherwise
*/
boolean isAuthorized(String parameterName, Object target, Object action);
+
+ /**
+ * Resolves the target object whose annotations should be checked for
authorization.
+ * For {@link org.apache.struts2.ModelDriven} actions, the default
implementation returns the action itself;
+ * the production implementation ({@link StrutsParameterAuthorizer})
overrides this to return the model from
+ * the value stack.
+ *
+ * <p>Callers that need both authorization checks AND the resolved target
(e.g. for downstream OGNL allowlisting)
+ * should call this once and reuse the result.</p>
+ *
+ * <p>This is a {@code default} method to preserve the interface as a
functional interface (SAM) for
+ * lambda-based test stubs.</p>
+ *
+ * @param action the action instance
+ * @return the resolved target — either the action or its model
+ * @since 7.2.0
+ */
+ default Object resolveTarget(Object action) {
+ return 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 6173a85c6..5491b585f 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
@@ -23,7 +23,6 @@ import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.apache.struts2.ActionContext;
import org.apache.struts2.ActionInvocation;
-import org.apache.struts2.ModelDriven;
import org.apache.struts2.StrutsConstants;
import org.apache.struts2.action.NoParameters;
import org.apache.struts2.action.ParameterNameAware;
@@ -369,14 +368,7 @@ public class ParametersInterceptor extends
MethodFilterInterceptor {
return true;
}
- // Resolve target for ModelDriven: if the ValueStack peek is different
from the action, it's the model
- Object target = action;
- if (action instanceof ModelDriven<?>) {
- Object stackTop =
ActionContext.getContext().getValueStack().peek();
- if (!stackTop.equals(action)) {
- target = stackTop;
- }
- }
+ Object target = parameterAuthorizer.resolveTarget(action);
// Delegate authorization check to shared ParameterAuthorizer (no OGNL
side effects)
if (!parameterAuthorizer.isAuthorized(name, target, action)) {
diff --git
a/core/src/main/java/org/apache/struts2/interceptor/parameter/StrutsParameterAuthorizer.java
b/core/src/main/java/org/apache/struts2/interceptor/parameter/StrutsParameterAuthorizer.java
index 82e47717e..03cc22c0e 100644
---
a/core/src/main/java/org/apache/struts2/interceptor/parameter/StrutsParameterAuthorizer.java
+++
b/core/src/main/java/org/apache/struts2/interceptor/parameter/StrutsParameterAuthorizer.java
@@ -21,6 +21,7 @@ package org.apache.struts2.interceptor.parameter;
import org.apache.commons.lang3.BooleanUtils;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
+import org.apache.struts2.ActionContext;
import org.apache.struts2.ModelDriven;
import org.apache.struts2.StrutsConstants;
import org.apache.struts2.inject.Inject;
@@ -91,6 +92,17 @@ public class StrutsParameterAuthorizer implements
ParameterAuthorizer {
this.requireAnnotationsTransitionMode =
BooleanUtils.toBoolean(transitionMode);
}
+ @Override
+ public Object resolveTarget(Object action) {
+ if (action instanceof ModelDriven<?>) {
+ Object stackTop =
ActionContext.getContext().getValueStack().peek();
+ if (!stackTop.equals(action)) {
+ return stackTop;
+ }
+ }
+ return action;
+ }
+
@Override
public boolean isAuthorized(String parameterName, Object target, Object
action) {
if (parameterName == null || parameterName.isEmpty()) {
diff --git
a/core/src/test/java/org/apache/struts2/interceptor/parameter/ParameterAuthorizerTest.java
b/core/src/test/java/org/apache/struts2/interceptor/parameter/ParameterAuthorizerTest.java
index f7c16fbb9..43eb57bcc 100644
---
a/core/src/test/java/org/apache/struts2/interceptor/parameter/ParameterAuthorizerTest.java
+++
b/core/src/test/java/org/apache/struts2/interceptor/parameter/ParameterAuthorizerTest.java
@@ -18,13 +18,16 @@
*/
package org.apache.struts2.interceptor.parameter;
+import org.apache.struts2.ActionContext;
import org.apache.struts2.ModelDriven;
+import org.apache.struts2.StubValueStack;
import org.apache.struts2.ognl.DefaultOgnlBeanInfoCacheFactory;
import org.apache.struts2.ognl.DefaultOgnlExpressionCacheFactory;
import org.apache.struts2.ognl.OgnlUtil;
import org.apache.struts2.ognl.StrutsOgnlGuard;
import org.apache.struts2.ognl.StrutsProxyCacheFactory;
import org.apache.struts2.util.StrutsProxyService;
+import org.junit.After;
import org.junit.Before;
import org.junit.Test;
@@ -56,6 +59,11 @@ public class ParameterAuthorizerTest {
authorizer.setProxyService(proxyService);
}
+ @After
+ public void tearDown() {
+ ActionContext.clear();
+ }
+
// --- requireAnnotations=false (backward compat) ---
@Test
@@ -182,6 +190,37 @@ public class ParameterAuthorizerTest {
assertThat(authorizer.isAuthorized(null, action, action)).isFalse();
}
+ // --- resolveTarget ---
+
+ @Test
+ public void resolveTarget_nonModelDriven_returnsAction() {
+ var action = new SecureAction();
+ assertThat(authorizer.resolveTarget(action)).isSameAs(action);
+ }
+
+ @Test
+ public void resolveTarget_modelDriven_returnsModelFromValueStack() {
+ var action = new ModelAction();
+ var model = action.getModel();
+ var valueStack = new StubValueStack();
+ valueStack.push(model);
+ ActionContext.of().withValueStack(valueStack).bind();
+
+ assertThat(authorizer.resolveTarget(action)).isSameAs(model);
+ }
+
+ @Test
+ public void resolveTarget_modelDriven_stackTopEqualsAction_returnsAction()
{
+ // Edge case: ModelDriven action where stack top equals the action
itself.
+ // No exemption applies — target stays as action.
+ var action = new ModelAction();
+ var valueStack = new StubValueStack();
+ valueStack.push(action);
+ ActionContext.of().withValueStack(valueStack).bind();
+
+ assertThat(authorizer.resolveTarget(action)).isSameAs(action);
+ }
+
// --- Inner test classes ---
public static class SecureAction {
diff --git
a/plugins/json/src/main/java/org/apache/struts2/json/JSONInterceptor.java
b/plugins/json/src/main/java/org/apache/struts2/json/JSONInterceptor.java
index a0279f2f0..d0acfd4ce 100644
--- a/plugins/json/src/main/java/org/apache/struts2/json/JSONInterceptor.java
+++ b/plugins/json/src/main/java/org/apache/struts2/json/JSONInterceptor.java
@@ -215,7 +215,14 @@ public class JSONInterceptor extends AbstractInterceptor {
Iterator<Map.Entry> it = json.entrySet().iterator();
while (it.hasNext()) {
Map.Entry entry = it.next();
- String key = (String) entry.getKey();
+ if (!(entry.getKey() instanceof String key)) {
+ // Defensive: a custom JSONReader could produce non-String
keys. Skip — we cannot
+ // construct a parameter path for authorization, and
JSONPopulator wouldn't bind
+ // these to bean properties anyway.
+ LOG.debug("Skipping JSON entry with non-String key [{}] of
type [{}] under prefix [{}]",
+ entry.getKey(), entry.getKey() == null ? "null" :
entry.getKey().getClass().getName(), prefix);
+ continue;
+ }
String fullPath = prefix.isEmpty() ? key : prefix + "." + key;
if (!parameterAuthorizer.isAuthorized(fullPath, target, action)) {
diff --git
a/plugins/json/src/test/java/org/apache/struts2/json/JSONInterceptorTest.java
b/plugins/json/src/test/java/org/apache/struts2/json/JSONInterceptorTest.java
index 2203f6f29..ece2f1272 100644
---
a/plugins/json/src/test/java/org/apache/struts2/json/JSONInterceptorTest.java
+++
b/plugins/json/src/test/java/org/apache/struts2/json/JSONInterceptorTest.java
@@ -583,6 +583,32 @@ public class JSONInterceptorTest extends StrutsTestCase {
assertNull(action.getBar());
}
+ public void testNonStringKeysAreSkippedByAuthorizationFilter() throws
Exception {
+ // Simulate a custom JSON reader producing a Map with a non-String key.
+ // The authorizer should skip the entry rather than throw
ClassCastException.
+ JSONInterceptor interceptor = new JSONInterceptor();
+ JSONUtil jsonUtil = new JSONUtil();
+ jsonUtil.setReader(new StrutsJSONReader());
+ jsonUtil.setWriter(new StrutsJSONWriter());
+ interceptor.setJsonUtil(jsonUtil);
+ interceptor.setParameterAuthorizer((parameterName, target, action) ->
true);
+
+ java.util.Map<Object, Object> mixedKeyMap = new
java.util.LinkedHashMap<>();
+ mixedKeyMap.put("validKey", "ok");
+ mixedKeyMap.put(42, "shouldBeSkipped"); // Integer key, not String
+
+ java.lang.reflect.Method method =
JSONInterceptor.class.getDeclaredMethod(
+ "filterUnauthorizedKeys", java.util.Map.class, Object.class,
Object.class);
+ method.setAccessible(true);
+
+ // Should not throw ClassCastException
+ method.invoke(interceptor, mixedKeyMap, new TestAction(), new
TestAction());
+
+ // The non-String key entry should still be present (skipped, not
removed)
+ assertTrue("non-String-key entry should remain (skipped, not
removed)", mixedKeyMap.containsKey(42));
+ assertTrue("String-key entry should remain",
mixedKeyMap.containsKey("validKey"));
+ }
+
public void testParameterAuthorizerAllowsAllWhenPermissive() throws
Exception {
// Same JSON body, but authorizer allows all
this.request.setContent("{\"foo\":\"value1\",
\"bar\":\"value2\"}".getBytes());
diff --git
a/plugins/rest/src/test/java/org/apache/struts2/rest/ContentTypeInterceptorIntegrationTest.java
b/plugins/rest/src/test/java/org/apache/struts2/rest/ContentTypeInterceptorIntegrationTest.java
new file mode 100644
index 000000000..dc11c6fdf
--- /dev/null
+++
b/plugins/rest/src/test/java/org/apache/struts2/rest/ContentTypeInterceptorIntegrationTest.java
@@ -0,0 +1,134 @@
+/*
+ * 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.rest;
+
+import com.mockobjects.dynamic.AnyConstraintMatcher;
+import com.mockobjects.dynamic.Mock;
+import junit.framework.TestCase;
+import org.apache.struts2.ActionContext;
+import org.apache.struts2.ActionInvocation;
+import org.apache.struts2.action.Action;
+import org.apache.struts2.dispatcher.mapper.ActionMapping;
+import org.apache.struts2.interceptor.parameter.StrutsParameterAuthorizer;
+import org.apache.struts2.ognl.DefaultOgnlBeanInfoCacheFactory;
+import org.apache.struts2.ognl.DefaultOgnlExpressionCacheFactory;
+import org.apache.struts2.ognl.OgnlUtil;
+import org.apache.struts2.ognl.StrutsOgnlGuard;
+import org.apache.struts2.ognl.StrutsProxyCacheFactory;
+import org.apache.struts2.rest.handler.JacksonJsonHandler;
+import org.apache.struts2.util.StrutsProxyService;
+import org.springframework.mock.web.MockHttpServletRequest;
+
+import static org.apache.struts2.ognl.OgnlCacheFactory.CacheType.LRU;
+
+/**
+ * Integration tests for ContentTypeInterceptor that use a real {@link
JacksonJsonHandler}
+ * and a real {@link StrutsParameterAuthorizer}, end-to-end. Verifies that
property
+ * filtering actually occurs on the deserialized object — not merely that the
wiring runs.
+ */
+public class ContentTypeInterceptorIntegrationTest extends TestCase {
+
+ private ContentTypeInterceptor interceptor;
+ private SecureRestAction action;
+ private Mock mockActionInvocation;
+ private Mock mockSelector;
+
+ @Override
+ protected void setUp() throws Exception {
+ super.setUp();
+ action = new SecureRestAction();
+
+ var ognlUtil = new OgnlUtil(
+ new DefaultOgnlExpressionCacheFactory<>("1000",
LRU.toString()),
+ new DefaultOgnlBeanInfoCacheFactory<>("1000", LRU.toString()),
+ new StrutsOgnlGuard());
+ var proxyService = new StrutsProxyService(new
StrutsProxyCacheFactory<>("1000", "basic"));
+
+ StrutsParameterAuthorizer authorizer = new StrutsParameterAuthorizer();
+ authorizer.setOgnlUtil(ognlUtil);
+ authorizer.setProxyService(proxyService);
+ authorizer.setRequireAnnotations(Boolean.TRUE.toString());
+
+ interceptor = new ContentTypeInterceptor();
+ interceptor.setParameterAuthorizer(authorizer);
+ interceptor.setRequireAnnotations(Boolean.TRUE.toString());
+
+ mockActionInvocation = new Mock(ActionInvocation.class);
+ mockSelector = new Mock(ContentTypeHandlerManager.class);
+ // ContentTypeInterceptor calls getAction() twice when
requireAnnotations=true
+ mockActionInvocation.expectAndReturn("getAction", action);
+ mockActionInvocation.expectAndReturn("getAction", action);
+ mockActionInvocation.expectAndReturn("invoke", Action.SUCCESS);
+ mockSelector.expectAndReturn("getHandlerForRequest", new
AnyConstraintMatcher() {
+ public boolean matches(Object[] args) { return true; }
+ }, new JacksonJsonHandler());
+ interceptor.setContentTypeHandlerSelector((ContentTypeHandlerManager)
mockSelector.proxy());
+ }
+
+ private void runWithBody(String body) throws Exception {
+ MockHttpServletRequest request = new MockHttpServletRequest();
+ request.setContent(body.getBytes());
+ request.setContentType("application/json");
+
+ ActionContext.of()
+ .withActionMapping(new ActionMapping())
+ .withServletRequest(request)
+ .bind();
+
+ interceptor.intercept((ActionInvocation) mockActionInvocation.proxy());
+ mockSelector.verify();
+ mockActionInvocation.verify();
+ }
+
+ public void testAnnotatedTopLevelPropertyIsApplied() throws Exception {
+ runWithBody("{\"name\":\"alice\"}");
+ assertEquals("alice", action.getName());
+ }
+
+ public void testUnannotatedTopLevelPropertyIsRejected() throws Exception {
+ runWithBody("{\"role\":\"admin\"}");
+ assertNull("unannotated 'role' must not be set", action.getRole());
+ }
+
+ public void testMixedPropertiesFilteredCorrectly() throws Exception {
+ runWithBody("{\"name\":\"alice\",\"role\":\"admin\"}");
+ assertEquals("alice", action.getName());
+ assertNull(action.getRole());
+ }
+
+ public void testNestedPropertyAuthorizedWhenDepthAllows() throws Exception
{
+ runWithBody("{\"address\":{\"city\":\"Warsaw\",\"zip\":\"00-001\"}}");
+ assertNotNull("address should be set", action.getAddress());
+ assertEquals("Warsaw", action.getAddress().getCity());
+ assertEquals("00-001", action.getAddress().getZip());
+ }
+
+ public void testNestedPropertyRejectedWhenDepthInsufficient() throws
Exception {
+ // shallowAddress has @StrutsParameter (depth=0) — its nested fields
should NOT be set
+
runWithBody("{\"shallowAddress\":{\"city\":\"Warsaw\",\"zip\":\"00-001\"}}");
+ // The top-level shallowAddress reference itself may be created
(Jackson behavior)
+ // but its nested city/zip must remain null because depth-1 isn't
allowed.
+ if (action.getShallowAddress() != null) {
+ assertNull("nested city should be rejected (depth insufficient)",
+ action.getShallowAddress().getCity());
+ assertNull("nested zip should be rejected (depth insufficient)",
+ action.getShallowAddress().getZip());
+ }
+ }
+}
diff --git
a/plugins/rest/src/test/java/org/apache/struts2/rest/SecureRestAction.java
b/plugins/rest/src/test/java/org/apache/struts2/rest/SecureRestAction.java
new file mode 100644
index 000000000..16966dec1
--- /dev/null
+++ b/plugins/rest/src/test/java/org/apache/struts2/rest/SecureRestAction.java
@@ -0,0 +1,70 @@
+/*
+ * 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.rest;
+
+import org.apache.struts2.ActionSupport;
+import org.apache.struts2.interceptor.parameter.StrutsParameter;
+
+/**
+ * Test fixture for ContentTypeInterceptor integration tests.
+ * Has annotated and unannotated properties to exercise authorization
filtering.
+ */
+public class SecureRestAction extends ActionSupport {
+
+ private String name;
+ private String role;
+ private Address address;
+ private Address shallowAddress;
+
+ public String getName() { return name; }
+
+ @StrutsParameter
+ public void setName(String name) { this.name = name; }
+
+ public String getRole() { return role; }
+ public void setRole(String role) { this.role = role; }
+
+ // Both annotations needed for REST: setter authorizes the top-level
"address" (depth 0),
+ // getter(depth=1) authorizes nested "address.city" (depth 1). Note:
ParametersInterceptor
+ // only requires the getter annotation — REST's recursive copy authorizes
each path level
+ // independently. This divergence is tracked for the Approach C refactor.
+ @StrutsParameter(depth = 1)
+ public Address getAddress() { return address; }
+
+ @StrutsParameter
+ public void setAddress(Address address) { this.address = address; }
+
+ // shallowAddress: depth-0 authorized (setter annotated), but nested
fields rejected
+ // because the getter has no depth>=1 annotation.
+ public Address getShallowAddress() { return shallowAddress; }
+
+ @StrutsParameter
+ public void setShallowAddress(Address shallowAddress) {
this.shallowAddress = shallowAddress; }
+
+ public static class Address {
+ private String city;
+ private String zip;
+
+ public String getCity() { return city; }
+ public void setCity(String city) { this.city = city; }
+
+ public String getZip() { return zip; }
+ public void setZip(String zip) { this.zip = zip; }
+ }
+}
