Apache/struts
1
0
Fork 0
mirror of https://github.com/apache/struts.git synced 2026-08-05 14:47:09 +00:00
Code Issues Packages Projects Releases Wiki Activity

WW-5534 Allow @StrutsParameter recognition and OGNL allowlist for Spring proxies

Browse Source
This commit is contained in:
Kusal Kithul-Godage
2025-02-28 04:42:36 +11:00
parent 117b9c741b
commit aeaa4f26cf
3 changed files with 53 additions and 38 deletions
Download Patch File Download Diff File Expand all files Collapse all files
core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java
+25 -9
View File
@@ -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;
core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java
+11 -24
View File
@@ -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)
core/src/test/java/org/apache/struts2/interceptor/parameter/StrutsParameterAnnotationTest.java
+17 -5
View File
@@ -18,15 +18,20 @@
*/
package org.apache.struts2.interceptor.parameter;
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;
@@ -37,6 +42,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 +62,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);
}