Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kusalk merged 2 commits into from
Mar 6, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
aeaa4f2
WW-5534 Allow @StrutsParameter recognition and OGNL allowlist for Spr…
kusalk Feb 27, 2025
433c483
WW-5534 Add coverage for proxy resolution
kusalk Mar 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 25 additions & 9 deletions core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,20 +33,21 @@
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;
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;
Expand Down Expand Up @@ -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;
Expand All @@ -104,6 +106,11 @@ public void setValueStackFactory(ValueStackFactory valueStackFactory) {
this.valueStackFactory = valueStackFactory;
}

@Inject
public void setOgnlUtil(OgnlUtil ognlUtil) {
this.ognlUtil = ognlUtil;
}

@Inject
public void setThreadAllowlist(ThreadAllowlist threadAllowlist) {
this.threadAllowlist = threadAllowlist;
Expand Down Expand Up @@ -395,6 +402,7 @@ protected boolean hasValidAnnotatedMember(String rootProperty, Object action, lo
}

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;
Expand All @@ -412,7 +420,7 @@ protected boolean hasValidAnnotatedPropertyDescriptor(Object action, PropertyDes
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());
}
Expand Down Expand Up @@ -451,24 +459,25 @@ protected void allowlistClass(Class<?> clazz) {
}

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 {
Expand All @@ -477,7 +486,7 @@ protected boolean hasValidAnnotatedField(Object action, String fieldName, long p
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());
}
Expand Down Expand Up @@ -510,9 +519,16 @@ protected StrutsParameter getParameterAnnotation(AnnotatedElement element) {
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));
Copy link
Member Author

@kusalk kusalk Mar 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I implemented this class, I forgot OgnlUtil already had a cached variant of this capability. We are now using that, and resolving any proxies to ensure annotation detection works as expected.

} catch (IntrospectionException e) {
LOG.warn("Error introspecting Action {} for parameter injection validation", action.getClass(), e);
return null;
Expand Down
35 changes: 11 additions & 24 deletions core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,16 +212,17 @@ protected boolean checkAllowlist(Object target, Member member) {
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);
Copy link
Member Author

@kusalk kusalk Mar 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We replaced #getHibernateProxyTarget with #ultimateTargetClass which can also resolve Spring proxies. This allows the OGNL allowlist to function in the presence of Spring proxies in applications where struts.disallowProxyObjectAccess has been reverted to false.

if (newTargetClass != targetClass) {
targetClass = newTargetClass;
member = ProxyUtil.resolveTargetMember(member, newTargetClass);
}
}

Expand All @@ -231,10 +232,10 @@ protected boolean checkAllowlist(Object target, Member member) {
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);
Expand All @@ -243,20 +244,6 @@ protected boolean checkAllowlist(Object target, Member member) {
return true;
}

private void logAllowlistHibernateEntity(Object original, Object resolved) {
Copy link
Member Author

@kusalk kusalk Mar 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've decided to remove this warning as it's a tad aggressive. We've documented best practices in the migration guide and if users wish to still utilise Hibernate entities then that's up to them

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)
Expand Down
Loading