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 f15b50af1e..3963ac7307 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 void restore(Map context, Object target, Member member, String propertyNa 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 @@ protected boolean checkAllowlist(Object target, Member member) { 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 895cfb7eec..22c3444466 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 static Class ultimateTargetClass(Object candidate) { * @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 static boolean isProxyMember(Member member, Object object) { */ 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 7fb560c5b0..500e6adf6a 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 void testOgnlValueStackFromOgnlValueStackFactoryAllStaticAccess() throws 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/ognl/SecurityMemberAccessProxyTest.java b/plugins/spring/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessProxyTest.java index 885665a120..7a9d017fe0 100644 --- a/plugins/spring/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessProxyTest.java +++ b/plugins/spring/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessProxyTest.java @@ -31,6 +31,7 @@ import java.util.Map; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; public class SecurityMemberAccessProxyTest extends XWorkJUnit4TestCase { @@ -87,4 +88,91 @@ public void allowAllProxyAccess() { assertTrue(sma.isAccessible(context, proxy.getAction(), proxyObjectProxyMember, "")); assertTrue(sma.isAccessible(context, proxy.getAction(), proxyObjectNonProxyMember, "")); } + + @Test + public void nullTargetAndTargetAndMemberNotAllowed() { + sma.useDisallowProxyObjectAccess(Boolean.TRUE.toString()); + sma.useDisallowProxyMemberAccess(Boolean.TRUE.toString()); + assertTrue(sma.isAccessible(context, null, proxyObjectProxyMember, "")); + } + + @Test + public void nullTargetAndTargetAllowedAndMemberNotAllowed() { + sma.useDisallowProxyObjectAccess(Boolean.FALSE.toString()); + sma.useDisallowProxyMemberAccess(Boolean.TRUE.toString()); + assertTrue(sma.isAccessible(context, null, proxyObjectProxyMember, "")); + } + + @Test + public void nullTargetAndTargetAndMemberAllowed() { + sma.useDisallowProxyObjectAccess(Boolean.FALSE.toString()); + sma.useDisallowProxyMemberAccess(Boolean.FALSE.toString()); + assertTrue(sma.isAccessible(context, null, proxyObjectProxyMember, "")); + } + + @Test + public void nullMemberAndTargetAndMemberNotAllowed() { + sma.useDisallowProxyObjectAccess(Boolean.TRUE.toString()); + sma.useDisallowProxyMemberAccess(Boolean.TRUE.toString()); + Object action = proxy.getAction(); + assertThrows("Member cannot be null!", IllegalArgumentException.class, + () -> sma.isAccessible(context, action, null, "")); + } + + @Test + public void nullMemberAndTargetAllowedAndMemberNotAllowed() { + sma.useDisallowProxyObjectAccess(Boolean.FALSE.toString()); + sma.useDisallowProxyMemberAccess(Boolean.TRUE.toString()); + Object action = proxy.getAction(); + assertThrows("Member cannot be null!", IllegalArgumentException.class, + () -> sma.isAccessible(context, action, null, "")); + } + + @Test + public void nullMemberAndTargetNotAllowedAndMemberAllowed() { + sma.useDisallowProxyObjectAccess(Boolean.TRUE.toString()); + sma.useDisallowProxyMemberAccess(Boolean.FALSE.toString()); + Object action = proxy.getAction(); + assertThrows("Member cannot be null!", IllegalArgumentException.class, + () -> sma.isAccessible(context, action, null, "")); + } + + @Test + public void nullTargetAndMemberAndTargetAndMemberNotAllowed() { + sma.useDisallowProxyObjectAccess(Boolean.TRUE.toString()); + sma.useDisallowProxyMemberAccess(Boolean.TRUE.toString()); + assertThrows("Member cannot be null!", IllegalArgumentException.class, + () -> sma.isAccessible(context, null, null, "")); + } + + @Test + public void nullTargetAndMemberAndTargetNotAllowedAndMemberAllowed() { + sma.useDisallowProxyObjectAccess(Boolean.TRUE.toString()); + sma.useDisallowProxyMemberAccess(Boolean.FALSE.toString()); + assertThrows("Member cannot be null!", IllegalArgumentException.class, + () -> sma.isAccessible(context, null, null, "")); + } + + @Test + public void nullTargetAndMemberAndTargetAllowedAndMemberNotAllowed() { + sma.useDisallowProxyObjectAccess(Boolean.FALSE.toString()); + sma.useDisallowProxyMemberAccess(Boolean.TRUE.toString()); + assertThrows("Member cannot be null!", IllegalArgumentException.class, + () -> sma.isAccessible(context, null, null, "")); + } + + @Test + public void nullTargetAndMemberAndTargetAndMemberAllowed() { + sma.useDisallowProxyObjectAccess(Boolean.FALSE.toString()); + sma.useDisallowProxyMemberAccess(Boolean.FALSE.toString()); + assertThrows("Member cannot be null!", IllegalArgumentException.class, + () -> sma.isAccessible(context, null, null, "")); + } + + @Test + public void nullPropertyName() { + sma.useDisallowProxyMemberAccess(Boolean.FALSE.toString()); + Object action = proxy.getAction(); + assertTrue(sma.isAccessible(context, action, proxyObjectProxyMember, null)); + } } 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 7b77294e51..dc6e58cb3a 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 void setUp() throws Exception { } public void testIsProxy() throws Exception { + assertFalse(ProxyUtil.isProxy(null)); + Object simpleAction = appContext.getBean("simple-action"); assertFalse(ProxyUtil.isProxy(simpleAction));