From 583b174fe6c47671531fdf1436830b07a8141599 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Thu, 6 Feb 2025 12:02:40 +1100 Subject: [PATCH] WW-5525 Fix NPE in ProxyUtil for SecurityMemberAccess originating static members --- .../xwork2/ognl/SecurityMemberAccess.java | 5 +++- .../opensymphony/xwork2/util/ProxyUtil.java | 3 +- .../xwork2/ognl/OgnlValueStackTest.java | 28 +++++++++++++++++++ .../xwork2/spring/SpringProxyUtilTest.java | 2 ++ 4 files changed, 36 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java b/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java index f15b50af1..3963ac730 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java @@ -160,6 +160,9 @@ public class SecurityMemberAccess implements MemberAccess { public boolean isAccessible(Map context, Object target, Member member, String propertyName) { LOG.debug("Checking access for [target: {}, member: {}, property: {}]", target, member, propertyName); + if (member == null) { + throw new IllegalArgumentException("Member cannot be null!"); + } if (target != null) { // Special case: Target is a Class object but not Class.class if (Class.class.equals(target.getClass()) && !Class.class.equals(target)) { @@ -228,7 +231,7 @@ public class SecurityMemberAccess implements MemberAccess { return true; } - if (!disallowProxyObjectAccess && target != null && ProxyUtil.isProxy(target)) { + 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 // protection in applications where the developer has accepted the risk of allowing OGNL access to Hibernate diff --git a/core/src/main/java/com/opensymphony/xwork2/util/ProxyUtil.java b/core/src/main/java/com/opensymphony/xwork2/util/ProxyUtil.java index 895cfb7ee..22c344446 100644 --- a/core/src/main/java/com/opensymphony/xwork2/util/ProxyUtil.java +++ b/core/src/main/java/com/opensymphony/xwork2/util/ProxyUtil.java @@ -81,6 +81,7 @@ public class ProxyUtil { * @param object the object to check */ public static boolean isProxy(Object object) { + if (object == null) return false; Class clazz = object.getClass(); Boolean flag = isProxyCache.get(clazz); if (flag != null) { @@ -121,7 +122,7 @@ public class ProxyUtil { */ public static boolean isHibernateProxy(Object object) { try { - return HibernateProxy.class.isAssignableFrom(object.getClass()); + return object != null && HibernateProxy.class.isAssignableFrom(object.getClass()); } catch (NoClassDefFoundError ignored) { return false; } diff --git a/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java b/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java index 7fb560c5b..500e6adf6 100644 --- a/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java @@ -1233,6 +1233,34 @@ public class OgnlValueStackTest extends XWorkTestCase { assertNull("accessed private field (result not null) ?", accessedValue); } + public void testFindValueWithConstructorAndProxyChecks() { + Map properties = new HashMap<>(); + properties.put(StrutsConstants.STRUTS_DISALLOW_PROXY_OBJECT_ACCESS, Boolean.TRUE.toString()); + properties.put(StrutsConstants.STRUTS_DISALLOW_PROXY_MEMBER_ACCESS, Boolean.TRUE.toString()); + loadButSet(properties); + refreshContainerFields(); + + String value = "test"; + String ognlResult = (String) vs.findValue( + "new com.opensymphony.xwork2.ognl.OgnlValueStackTest$ValueHolder('" + value + "').value", String.class); + + assertEquals(value, ognlResult); + } + + @SuppressWarnings({"unused"}) + public static class ValueHolder { + // See testFindValueWithConstructorAndProxyChecks + private final String value; + + public ValueHolder(String value) { + this.value = value; + } + + public String getValue() { + return value; + } + } + static class BadJavaBean { private int count; private int count2; diff --git a/plugins/spring/src/test/java/com/opensymphony/xwork2/spring/SpringProxyUtilTest.java b/plugins/spring/src/test/java/com/opensymphony/xwork2/spring/SpringProxyUtilTest.java index 7b77294e5..dc6e58cb3 100644 --- a/plugins/spring/src/test/java/com/opensymphony/xwork2/spring/SpringProxyUtilTest.java +++ b/plugins/spring/src/test/java/com/opensymphony/xwork2/spring/SpringProxyUtilTest.java @@ -46,6 +46,8 @@ public class SpringProxyUtilTest extends XWorkTestCase { } public void testIsProxy() throws Exception { + assertFalse(ProxyUtil.isProxy(null)); + Object simpleAction = appContext.getBean("simple-action"); assertFalse(ProxyUtil.isProxy(simpleAction));