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 d064fbf68e..6c5848bd1d 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. + * + *

Callers that need both authorization checks AND the resolved target (e.g. for downstream OGNL allowlisting) + * should call this once and reuse the result.

+ * + *

This is a {@code default} method to preserve the interface as a functional interface (SAM) for + * lambda-based test stubs.

+ * + * @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 6173a85c65..5491b585f2 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.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 @@ protected boolean isParameterAnnotatedAndAllowlist(String name, Object action) { 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 82e47717e3..03cc22c0e5 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 @@ 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 void setRequireAnnotationsTransitionMode(String transitionMode) { 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 f7c16fbb92..43eb57bcc2 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 void setUp() { authorizer.setProxyService(proxyService); } + @After + public void tearDown() { + ActionContext.clear(); + } + // --- requireAnnotations=false (backward compat) --- @Test @@ -182,6 +190,37 @@ public void emptyParameterName_rejectedEvenWhenAnnotationsNotRequired() { 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 a0279f2f09..d0acfd4ce0 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 @@ private void filterUnauthorizedKeysRecursive(Map json, String prefix, Object tar Iterator 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 2203f6f290..ece2f1272d 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 void testParameterAuthorizerRejectsUnauthorizedKeys() throws Exception { 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 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 0000000000..dc11c6fdfc --- /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 0000000000..16966dec1d --- /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; } + } +}