This is an automated email from the ASF dual-hosted git repository.
kusal pushed a commit to branch WW-5534-annotation-allowlist-proxy
in repository https://gitbox.apache.org/repos/asf/struts.git
commit ccd06612abde48e90995d166881829f2c24bd51f
Author: Kusal Kithul-Godage <g...@kusal.io>
AuthorDate: Fri Feb 28 04:42:36 2025 +1100
WW-5534 Allow @StrutsParameter recognition and OGNL allowlist for Spring
proxies
---
.../parameter/ParametersInterceptor.java | 34 +++++++++++++++------
.../apache/struts2/ognl/SecurityMemberAccess.java | 35 +++++++---------------
2 files changed, 36 insertions(+), 33 deletions(-)
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 92449415e..4cd1c3faa 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
@@ -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;
diff --git
a/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java
b/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java
index 64a8fa5a4..928dda78d 100644
--- a/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java
+++ b/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java
@@ -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)
