From 9fee06cea030447c4bae650bde40282a93e85cd2 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sat, 1 Feb 2025 07:40:18 +0100 Subject: [PATCH] WW-5525 Fixes NPE when checking if expressions is acceptable --- .../struts2/ognl/SecurityMemberAccess.java | 10 +-- .../ognl/SecurityMemberAccessTest.java | 1 + .../ognl/SecurityMemberAccessProxyTest.java | 84 +++++++++++++++++++ 3 files changed, 90 insertions(+), 5 deletions(-) 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..e04af4221 100644 --- a/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java +++ b/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java @@ -160,12 +160,12 @@ public class SecurityMemberAccess implements MemberAccess { } } - if (!checkProxyObjectAccess(target)) { + if (target != null && !checkProxyObjectAccess(target)) { LOG.warn("Access to proxy is blocked! Target [{}], proxy class [{}]", target, target.getClass().getName()); return false; } - if (!checkProxyMemberAccess(target, member)) { + if (target != null && !checkProxyMemberAccess(target, member)) { LOG.warn("Access to proxy is blocked! Member class [{}] of target [{}], member [{}]", member.getDeclaringClass(), target, member); return false; } @@ -185,15 +185,15 @@ public class SecurityMemberAccess implements MemberAccess { return false; } - if (!checkDefaultPackageAccess(target, member)) { + if (target != null && !checkDefaultPackageAccess(target, member)) { return false; } - if (!checkExclusionList(target, member)) { + if (target != null && !checkExclusionList(target, member)) { return false; } - if (!checkAllowlist(target, member)) { + if (target != null && !checkAllowlist(target, member)) { return false; } diff --git a/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessTest.java b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessTest.java index e3d3fa589..1cd7e9a60 100644 --- a/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessTest.java +++ b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessTest.java @@ -649,6 +649,7 @@ public class SecurityMemberAccessTest { @Test public void testBlockedStaticFieldWhenClassIsExcluded() throws Exception { // given + assignNewSma(false); sma.useExcludedClasses(String.join(",", Class.class.getName(), StaticTester.class.getName())); // when diff --git a/plugins/spring/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessProxyTest.java b/plugins/spring/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessProxyTest.java index 91ffae19e..242010a25 100644 --- a/plugins/spring/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessProxyTest.java +++ b/plugins/spring/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessProxyTest.java @@ -31,6 +31,7 @@ import java.util.HashMap; 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,87 @@ public class SecurityMemberAccessProxyTest extends XWorkJUnit4TestCase { 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()); + assertThrows("Member cannot be null!", IllegalArgumentException.class, + () -> sma.isAccessible(context, proxy.getAction(), null, "")); + } + + @Test + public void nullMemberAndTargetAllowedAndMemberNotAllowed() { + sma.useDisallowProxyObjectAccess(Boolean.FALSE.toString()); + sma.useDisallowProxyMemberAccess(Boolean.TRUE.toString()); + assertThrows("Member cannot be null!", IllegalArgumentException.class, + () -> sma.isAccessible(context, proxy.getAction(), null, "")); + } + + @Test + public void nullMemberAndTargetNotAllowedAndMemberAllowed() { + sma.useDisallowProxyObjectAccess(Boolean.TRUE.toString()); + sma.useDisallowProxyMemberAccess(Boolean.FALSE.toString()); + assertThrows("Member cannot be null!", IllegalArgumentException.class, + () -> sma.isAccessible(context, proxy.getAction(), 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()); + assertTrue(sma.isAccessible(context, proxy.getAction(), proxyObjectProxyMember, null)); + } }