Merge pull request #1237 from apache/WW-5534-annotation-allowlist-proxy

WW-5534 Allow @StrutsParameter recognition and OGNL allowlist for Spring proxies
This commit is contained in:
Kusal Kithul-Godage
2025-03-06 12:24:07 +11:00
committed by GitHub
5 changed files with 223 additions and 52 deletions
@@ -33,12 +33,14 @@ import org.apache.struts2.dispatcher.HttpParameters;
import org.apache.struts2.dispatcher.Parameter;
import org.apache.struts2.inject.Inject;
import org.apache.struts2.interceptor.MethodFilterInterceptor;
import org.apache.struts2.ognl.OgnlUtil;
import org.apache.struts2.ognl.ThreadAllowlist;
import org.apache.struts2.security.AcceptedPatternsChecker;
import org.apache.struts2.security.DefaultAcceptedPatternsChecker;
import org.apache.struts2.security.ExcludedPatternsChecker;
import org.apache.struts2.util.ClearableValueStack;
import org.apache.struts2.util.MemberAccessValueStack;
import org.apache.struts2.util.ProxyUtil;
import org.apache.struts2.util.TextParseUtil;
import org.apache.struts2.util.ValueStack;
import org.apache.struts2.util.ValueStackFactory;
@@ -46,7 +48,6 @@ import org.apache.struts2.util.reflection.ReflectionContextState;
import java.beans.BeanInfo;
import java.beans.IntrospectionException;
import java.beans.Introspector;
import java.beans.PropertyDescriptor;
import java.lang.reflect.AnnotatedElement;
import java.lang.reflect.Field;
@@ -93,6 +94,7 @@ public class ParametersInterceptor extends MethodFilterInterceptor {
protected boolean requireAnnotationsTransitionMode = false;
private ValueStackFactory valueStackFactory;
private OgnlUtil ognlUtil;
protected ThreadAllowlist threadAllowlist;
private ExcludedPatternsChecker excludedPatterns;
private AcceptedPatternsChecker acceptedPatterns;
@@ -104,6 +106,11 @@ public class ParametersInterceptor extends MethodFilterInterceptor {
this.valueStackFactory = valueStackFactory;
}
@Inject
public void setOgnlUtil(OgnlUtil ognlUtil) {
this.ognlUtil = ognlUtil;
}
@Inject
public void setThreadAllowlist(ThreadAllowlist threadAllowlist) {
this.threadAllowlist = threadAllowlist;
@@ -395,6 +402,7 @@ public class ParametersInterceptor extends MethodFilterInterceptor {
}
protected boolean hasValidAnnotatedPropertyDescriptor(Object action, PropertyDescriptor propDesc, long paramDepth) {
Class<?> actionClass = ultimateClass(action);
Method relevantMethod = paramDepth == 0 ? propDesc.getWriteMethod() : propDesc.getReadMethod();
if (relevantMethod == null) {
return false;
@@ -412,7 +420,7 @@ public class ParametersInterceptor extends MethodFilterInterceptor {
return false;
}
LOG.debug("Success: Matching annotated method [{}] found for property [{}] of depth [{}] on Action [{}]",
relevantMethod.getName(), propDesc.getName(), paramDepth, action.getClass().getSimpleName());
relevantMethod.getName(), propDesc.getName(), paramDepth, actionClass.getSimpleName());
if (paramDepth >= 1) {
allowlistClass(propDesc.getPropertyType());
}
@@ -451,24 +459,25 @@ public class ParametersInterceptor extends MethodFilterInterceptor {
}
protected boolean hasValidAnnotatedField(Object action, String fieldName, long paramDepth) {
Class<?> actionClass = ultimateClass(action);
LOG.debug("No matching annotated method found for property [{}] of depth [{}] on Action [{}], now also checking for public field",
fieldName, paramDepth, action.getClass().getSimpleName());
fieldName, paramDepth, actionClass.getSimpleName());
Field field;
try {
field = action.getClass().getDeclaredField(fieldName);
field = actionClass.getDeclaredField(fieldName);
} catch (NoSuchFieldException e) {
LOG.debug("Matching field for property [{}] not found on Action [{}]", fieldName, action.getClass().getSimpleName());
LOG.debug("Matching field for property [{}] not found on Action [{}]", fieldName, actionClass.getSimpleName());
return false;
}
if (!Modifier.isPublic(field.getModifiers())) {
LOG.debug("Matching field [{}] is not public on Action [{}]", field.getName(), action.getClass().getSimpleName());
LOG.debug("Matching field [{}] is not public on Action [{}]", field.getName(), actionClass.getSimpleName());
return false;
}
if (getPermittedInjectionDepth(field) < paramDepth) {
String logMessage = format(
"Parameter injection for field [%s] on Action [%s] rejected. Ensure it is annotated with @StrutsParameter with an appropriate 'depth'.",
field.getName(),
action.getClass().getName());
actionClass.getName());
if (devMode) {
notifyDeveloperOfError(LOG, action, logMessage);
} else {
@@ -477,7 +486,7 @@ public class ParametersInterceptor extends MethodFilterInterceptor {
return false;
}
LOG.debug("Success: Matching annotated public field [{}] found for property of depth [{}] on Action [{}]",
field.getName(), paramDepth, action.getClass().getSimpleName());
field.getName(), paramDepth, actionClass.getSimpleName());
if (paramDepth >= 1) {
allowlistClass(field.getType());
}
@@ -510,9 +519,16 @@ public class ParametersInterceptor extends MethodFilterInterceptor {
return element.getAnnotation(StrutsParameter.class);
}
protected Class<?> ultimateClass(Object action) {
if (ProxyUtil.isProxy(action)) {
return ProxyUtil.ultimateTargetClass(action);
}
return action.getClass();
}
protected BeanInfo getBeanInfo(Object action) {
try {
return Introspector.getBeanInfo(action.getClass());
return ognlUtil.getBeanInfo(ultimateClass(action));
} catch (IntrospectionException e) {
LOG.warn("Error introspecting Action {} for parameter injection validation", action.getClass(), e);
return null;
@@ -212,16 +212,17 @@ public class SecurityMemberAccess implements MemberAccess {
return true;
}
Class<?> targetClass = target != null ? target.getClass() : null;
if (!disallowProxyObjectAccess && ProxyUtil.isProxy(target)) {
// If `disallowProxyObjectAccess` is not set, allow resolving Hibernate entities to their underlying
// classes/members. This allows the allowlist capability to continue working and offer some level of
// If `disallowProxyObjectAccess` is not set, allow resolving Hibernate entities and Spring proxies to their
// underlying classes/members. This allows the allowlist capability to continue working and still offer
// protection in applications where the developer has accepted the risk of allowing OGNL access to Hibernate
// entities. This is preferred to having to disable the allowlist capability entirely.
Object newTarget = ProxyUtil.getHibernateProxyTarget(target);
if (newTarget != target) {
logAllowlistHibernateEntity(target, newTarget);
target = newTarget;
member = ProxyUtil.resolveTargetMember(member, newTarget);
// entities and Spring proxies. This is preferred to having to disable the allowlist capability entirely.
Class<?> newTargetClass = ProxyUtil.ultimateTargetClass(target);
if (newTargetClass != targetClass) {
targetClass = newTargetClass;
member = ProxyUtil.resolveTargetMember(member, newTargetClass);
}
}
@@ -231,10 +232,10 @@ public class SecurityMemberAccess implements MemberAccess {
memberClass, member, STRUTS_ALLOWLIST_CLASSES, STRUTS_ALLOWLIST_PACKAGE_NAMES);
return false;
}
if (target == null || target.getClass() == memberClass) {
if (targetClass == null || targetClass == memberClass) {
return true;
}
Class<?> targetClass = target.getClass();
if (!isClassAllowlisted(targetClass)) {
LOG.warn("Target class [{}] of target [{}] is not allowlisted! Add to '{}' or '{}' configuration.",
targetClass, target, STRUTS_ALLOWLIST_CLASSES, STRUTS_ALLOWLIST_PACKAGE_NAMES);
@@ -243,20 +244,6 @@ public class SecurityMemberAccess implements MemberAccess {
return true;
}
private void logAllowlistHibernateEntity(Object original, Object resolved) {
if (!isDevMode && !LOG.isDebugEnabled()) {
return;
}
String msg = "Hibernate entity [{}] resolved to [{}] for purpose of OGNL allowlisting." +
" We don't recommend executing OGNL expressions against Hibernate entities, you may disallow this behaviour using the configuration `{}=true`.";
Object[] args = {original, resolved, StrutsConstants.STRUTS_DISALLOW_PROXY_OBJECT_ACCESS};
if (isDevMode) {
LOG.warn(msg, args);
} else {
LOG.debug(msg, args);
}
}
protected boolean isClassAllowlisted(Class<?> clazz) {
return allowlistClasses.contains(clazz)
|| ALLOWLIST_REQUIRED_CLASSES.contains(clazz)
@@ -18,18 +18,26 @@
*/
package org.apache.struts2.interceptor.parameter;
import org.aopalliance.intercept.Joinpoint;
import org.aopalliance.intercept.MethodInterceptor;
import org.apache.commons.lang3.ClassUtils;
import org.apache.struts2.ActionContext;
import org.apache.struts2.ModelDriven;
import org.apache.struts2.StubValueStack;
import org.apache.struts2.security.AcceptedPatternsChecker;
import org.apache.struts2.security.NotExcludedAcceptedPatternsChecker;
import org.apache.commons.lang3.ClassUtils;
import org.apache.struts2.dispatcher.HttpParameters;
import org.apache.struts2.dispatcher.Parameter;
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.ThreadAllowlist;
import org.apache.struts2.security.AcceptedPatternsChecker.IsAccepted;
import org.apache.struts2.security.ExcludedPatternsChecker.IsExcluded;
import org.apache.struts2.security.NotExcludedAcceptedPatternsChecker;
import org.junit.After;
import org.junit.Before;
import org.junit.Test;
import org.springframework.aop.framework.ProxyFactory;
import java.util.HashMap;
import java.util.HashSet;
@@ -37,6 +45,7 @@ import java.util.List;
import java.util.Map;
import java.util.Set;
import static org.apache.struts2.ognl.OgnlCacheFactory.CacheType.LRU;
import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.Mockito.mock;
@@ -56,9 +65,15 @@ public class StrutsParameterAnnotationTest {
threadAllowlist = new ThreadAllowlist();
parametersInterceptor.setThreadAllowlist(threadAllowlist);
var ognlUtil = new OgnlUtil(
new DefaultOgnlExpressionCacheFactory<>(String.valueOf(1000), LRU.toString()),
new DefaultOgnlBeanInfoCacheFactory<>(String.valueOf(1000), LRU.toString()),
new StrutsOgnlGuard());
parametersInterceptor.setOgnlUtil(ognlUtil);
NotExcludedAcceptedPatternsChecker checker = mock(NotExcludedAcceptedPatternsChecker.class);
when(checker.isAccepted(anyString())).thenReturn(AcceptedPatternsChecker.IsAccepted.yes(""));
when(checker.isExcluded(anyString())).thenReturn(NotExcludedAcceptedPatternsChecker.IsExcluded.no(new HashSet<>()));
when(checker.isAccepted(anyString())).thenReturn(IsAccepted.yes(""));
when(checker.isExcluded(anyString())).thenReturn(IsExcluded.no(Set.of()));
parametersInterceptor.setAcceptedPatterns(checker);
parametersInterceptor.setExcludedPatterns(checker);
}
@@ -94,174 +109,267 @@ public class StrutsParameterAnnotationTest {
return set;
}
/**
* Private String field cannot be injected even when annotated.
*/
@Test
public void privateStrAnnotated() {
testParameter(new FieldAction(), "privateStr", false);
}
/**
* Public String field can be injected when annotated.
*/
@Test
public void publicStrAnnotated() {
testParameter(new FieldAction(), "publicStr", true);
assertThat(threadAllowlist.getAllowlist()).isEmpty();
}
/**
* Public String field cannot be injected when not annotated.
*/
@Test
public void publicStrNotAnnotated() {
testParameter(new FieldAction(), "publicStrNotAnnotated", false);
}
/**
* Private Pojo field cannot be injected even when annotated with the appropriate depth.
*/
@Test
public void privatePojoAnnotated() {
testParameter(new FieldAction(), "privatePojo.key", false);
}
/**
* Public Pojo field cannot be injected when annotated with depth zero.
*/
@Test
public void publicPojoDepthZero() {
testParameter(new FieldAction(), "publicPojoDepthZero.key", false);
}
/**
* Public Pojo field can be injected when annotated with depth one.
*/
@Test
public void publicPojoDepthOne() {
testParameter(new FieldAction(), "publicPojoDepthOne.key", true);
assertThat(threadAllowlist.getAllowlist()).containsExactlyInAnyOrderElementsOf(getParentClasses(Pojo.class));
}
/**
* Public Pojo field can be injected when annotated with depth one, using the square bracket syntax.
*/
@Test
public void publicPojoDepthOne_sqrBracket() {
testParameter(new FieldAction(), "publicPojoDepthOne['key']", true);
assertThat(threadAllowlist.getAllowlist()).containsExactlyInAnyOrderElementsOf(getParentClasses(Pojo.class));
}
/**
* Public Pojo field can be injected when annotated with depth one, using the bracket syntax.
*/
@Test
public void publicPojoDepthOne_bracket() {
testParameter(new FieldAction(), "publicPojoDepthOne('key')", true);
assertThat(threadAllowlist.getAllowlist()).containsExactlyInAnyOrderElementsOf(getParentClasses(Pojo.class));
}
@Test
public void publicNestedPojoDepthOne() {
testParameter(new FieldAction(), "publicPojoDepthOne.key.key", false);
}
/**
* Public Pojo field can be injected when annotated with a depth greater than required.
*/
@Test
public void publicPojoDepthTwo() {
testParameter(new FieldAction(), "publicPojoDepthTwo.key", true);
assertThat(threadAllowlist.getAllowlist()).containsExactlyInAnyOrderElementsOf(getParentClasses(Pojo.class));
}
/**
* Public Pojo field cannot be injected two levels when only annotated with depth one.
*/
@Test
public void publicNestedPojoDepthOne() {
testParameter(new FieldAction(), "publicPojoDepthOne.key.key", false);
}
/**
* Public Pojo field can be injected two levels when annotated with depth two.
*/
@Test
public void publicNestedPojoDepthTwo() {
testParameter(new FieldAction(), "publicPojoDepthTwo.key.key", true);
assertThat(threadAllowlist.getAllowlist()).containsExactlyInAnyOrderElementsOf(getParentClasses(Pojo.class));
}
/**
* Public Pojo field can be injected two levels when annotated with depth two, using the square bracket syntax.
*/
@Test
public void publicNestedPojoDepthTwo_sqrBracket() {
testParameter(new FieldAction(), "publicPojoDepthTwo['key']['key']", true);
assertThat(threadAllowlist.getAllowlist()).containsExactlyInAnyOrderElementsOf(getParentClasses(Pojo.class));
}
/**
* Public Pojo field can be injected two levels when annotated with depth two, using the bracket syntax.
*/
@Test
public void publicNestedPojoDepthTwo_bracket() {
testParameter(new FieldAction(), "publicPojoDepthTwo('key')('key')", true);
assertThat(threadAllowlist.getAllowlist()).containsExactlyInAnyOrderElementsOf(getParentClasses(Pojo.class));
}
/**
* Private String setting method cannot be injected even when annotated.
*/
@Test
public void privateStrAnnotatedMethod() {
testParameter(new MethodAction(), "privateStr", false);
}
/**
* Public String setting method can be injected when annotated.
*/
@Test
public void publicStrAnnotatedMethod() {
testParameter(new MethodAction(), "publicStr", true);
assertThat(threadAllowlist.getAllowlist()).isEmpty();
}
/**
* Public String setting method cannot be injected when not annotated.
*/
@Test
public void publicStrNotAnnotatedMethod() {
testParameter(new MethodAction(), "publicStrNotAnnotated", false);
}
/**
* Private Pojo returning method cannot be injected even when annotated with the appropriate depth.
*/
@Test
public void privatePojoAnnotatedMethod() {
testParameter(new MethodAction(), "privatePojo.key", false);
}
/**
* Public Pojo returning method cannot be injected when annotated with depth zero.
*/
@Test
public void publicPojoDepthZeroMethod() {
testParameter(new MethodAction(), "publicPojoDepthZero.key", false);
}
/**
* Public Pojo returning method can be injected when annotated with depth one.
*/
@Test
public void publicPojoDepthOneMethod() {
testParameter(new MethodAction(), "publicPojoDepthOne.key", true);
assertThat(threadAllowlist.getAllowlist()).containsExactlyInAnyOrderElementsOf(getParentClasses(Pojo.class));
}
/**
* Public Pojo returning method cannot be injected two levels when only annotated with depth one.
*/
@Test
public void publicNestedPojoDepthOneMethod() {
testParameter(new MethodAction(), "publicPojoDepthOne.key.key", false);
}
/**
* Public Pojo returning method can be injected when annotated with a depth greater than required.
*/
@Test
public void publicPojoDepthTwoMethod() {
testParameter(new MethodAction(), "publicPojoDepthTwo.key", true);
assertThat(threadAllowlist.getAllowlist()).containsExactlyInAnyOrderElementsOf(getParentClasses(Pojo.class));
}
/**
* Public Pojo returning method can be injected two levels when annotated with depth two.
*/
@Test
public void publicNestedPojoDepthTwoMethod() {
testParameter(new MethodAction(), "publicPojoDepthTwo.key.key", true);
assertThat(threadAllowlist.getAllowlist()).containsExactlyInAnyOrderElementsOf(getParentClasses(Pojo.class));
}
/**
* Public list of Pojo field cannot be injected when annotated with depth one.
*/
@Test
public void publicPojoListDepthOne() {
testParameter(new FieldAction(), "publicPojoListDepthOne[0].key", false);
}
/**
* Public list of Pojo field can be injected when annotated with depth two.
*/
@Test
public void publicPojoListDepthTwo() {
testParameter(new FieldAction(), "publicPojoListDepthTwo[0].key", true);
assertThat(threadAllowlist.getAllowlist()).containsExactlyInAnyOrderElementsOf(getParentClasses(List.class, Pojo.class));
}
@Test
public void publicPojoMapDepthTwo() {
testParameter(new FieldAction(), "publicPojoMapDepthTwo['a'].key", true);
assertThat(threadAllowlist.getAllowlist()).containsExactlyInAnyOrderElementsOf(getParentClasses(Map.class, String.class, Pojo.class));
}
/**
* Public list of Pojo returning method cannot be injected when annotated with depth one.
*/
@Test
public void publicPojoListDepthOneMethod() {
testParameter(new MethodAction(), "publicPojoListDepthOne[0].key", false);
}
/**
* Public list of Pojo returning method can be injected when annotated with depth two.
*/
@Test
public void publicPojoListDepthTwoMethod() {
testParameter(new MethodAction(), "publicPojoListDepthTwo[0].key", true);
assertThat(threadAllowlist.getAllowlist()).containsExactlyInAnyOrderElementsOf(getParentClasses(List.class, Pojo.class));
}
/**
* Public map of Pojo field can be injected when annotated with depth two.
*/
@Test
public void publicPojoMapDepthTwo() {
testParameter(new FieldAction(), "publicPojoMapDepthTwo['a'].key", true);
assertThat(threadAllowlist.getAllowlist()).containsExactlyInAnyOrderElementsOf(getParentClasses(Map.class, String.class, Pojo.class));
}
/**
* Public map of Pojo returning method can be injected when annotated with depth two.
*/
@Test
public void publicPojoMapDepthTwoMethod() {
testParameter(new MethodAction(), "publicPojoMapDepthTwo['a'].key", true);
assertThat(threadAllowlist.getAllowlist()).containsExactlyInAnyOrderElementsOf(getParentClasses(Map.class, String.class, Pojo.class));
}
/**
* Public String field can be injected even when not annotated, if transition mode is enabled.
*/
@Test
public void publicStrNotAnnotated_transitionMode() {
parametersInterceptor.setRequireAnnotationsTransitionMode(Boolean.TRUE.toString());
testParameter(new FieldAction(), "publicStrNotAnnotated", true);
}
/**
* Public String setting method can be injected even when not annotated, if transition mode is enabled.
*/
@Test
public void publicStrNotAnnotatedMethod_transitionMode() {
parametersInterceptor.setRequireAnnotationsTransitionMode(Boolean.TRUE.toString());
testParameter(new MethodAction(), "publicStrNotAnnotated", true);
}
/**
* Models of ModelDriven actions can be injected without any annotations on the Action.
*/
@Test
public void publicModelPojo() {
var action = new ModelAction();
@@ -276,6 +384,27 @@ public class StrutsParameterAnnotationTest {
assertThat(threadAllowlist.getAllowlist()).containsExactlyInAnyOrderElementsOf(getParentClasses(Object.class, Pojo.class));
}
/**
* Models of ModelDriven actions can be injected without any annotations on the Action, even when the Action is
* proxied.
*/
@Test
public void publicModelPojo_proxied() {
var proxyFactory = new ProxyFactory(new ModelAction());
proxyFactory.setProxyTargetClass(true);
proxyFactory.addAdvice((MethodInterceptor) Joinpoint::proceed);
var proxiedAction = (ModelAction) proxyFactory.getProxy();
// Emulate ModelDrivenInterceptor running previously
var valueStack = new StubValueStack();
valueStack.push(proxiedAction.getModel());
ActionContext.of().withValueStack(valueStack).bind();
testParameter(proxiedAction, "name", true);
testParameter(proxiedAction, "name.nested", true);
assertThat(threadAllowlist.getAllowlist()).containsExactlyInAnyOrderElementsOf(getParentClasses(Object.class, Pojo.class));
}
static class FieldAction {
@StrutsParameter
private String privateStr;
@@ -18,12 +18,12 @@
*/
package org.apache.struts2.ognl;
import ognl.MemberAccess;
import org.apache.commons.lang3.reflect.FieldUtils;
import org.apache.struts2.TestBean;
import org.apache.struts2.config.ConfigurationException;
import org.apache.struts2.test.TestBean2;
import org.apache.struts2.util.Foo;
import ognl.MemberAccess;
import org.apache.commons.lang3.reflect.FieldUtils;
import org.hibernate.proxy.HibernateProxy;
import org.hibernate.proxy.LazyInitializer;
import org.junit.Before;
@@ -20,12 +20,15 @@ package org.apache.struts2.ognl;
import org.apache.struts2.ActionProxy;
import org.apache.struts2.XWorkJUnit4TestCase;
import org.apache.struts2.config.providers.XmlConfigurationProvider;
import org.apache.struts2.config.StrutsXmlConfigurationProvider;
import org.apache.struts2.config.providers.XmlConfigurationProvider;
import org.junit.Before;
import org.junit.Test;
import org.springframework.aop.MethodBeforeAdvice;
import org.springframework.aop.framework.ProxyFactory;
import java.lang.reflect.Member;
import java.lang.reflect.Method;
import java.util.Arrays;
import java.util.HashMap;
import java.util.Map;
@@ -175,4 +178,40 @@ public class SecurityMemberAccessProxyTest extends XWorkJUnit4TestCase {
Object action = proxy.getAction();
assertTrue(sma.isAccessible(context, action, proxyObjectProxyMember, null));
}
/**
* When the allowlist is enabled and proxy object access is allowed, Spring proxies should be allowlisted based
* on their underlying target object. Class allowlisting should work as expected.
*/
@Test
public void classInclusion_springProxy_allowProxyObjectAccess() throws Exception {
SpringService proxyObject = newSpringService();
Method proxyMethod = proxyObject.getClass().getMethod("doSomething");
sma.useEnforceAllowlistEnabled(Boolean.TRUE.toString());
sma.useDisallowProxyObjectAccess(Boolean.FALSE.toString());
sma.useAllowlistClasses(SpringServiceImpl.class.getName());
assertTrue(sma.checkAllowlist(proxyObject, proxyMethod));
}
private static SpringService newSpringService() {
SpringService target = new SpringServiceImpl();
ProxyFactory proxyFactory = new ProxyFactory(target);
proxyFactory.addAdvice(((MethodBeforeAdvice) (method, args, target1) -> {
System.out.println("Intercepting: " + method.getName());
}));
return (SpringService) proxyFactory.getProxy();
}
}
interface SpringService {
void doSomething();
}
class SpringServiceImpl implements SpringService {
@Override
public void doSomething() {
System.out.println("Doing something...");
}
}