Merge pull request #1214 from apache/WW-5525-proxyutil-npe

WW-5525 Fix NPE in ProxyUtil for SecurityMemberAccess originating static members
apache · Feb 17, 2025 · 31c3fc5 · 31c3fc5
2 parents adcd1df + 3c856c9
commit 31c3fc5
Show file tree

Hide file tree

Showing 4 changed files with 55 additions and 22 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
@@ -18,13 +18,13 @@
  */
 package org.apache.struts2.ognl;
 
-import org.apache.struts2.inject.Inject;
-import org.apache.struts2.util.ProxyUtil;
 import ognl.MemberAccess;
 import org.apache.commons.lang3.BooleanUtils;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 import org.apache.struts2.StrutsConstants;
+import org.apache.struts2.inject.Inject;
+import org.apache.struts2.util.ProxyUtil;
 
 import java.lang.reflect.AccessibleObject;
 import java.lang.reflect.Constructor;
@@ -38,17 +38,17 @@
 import java.util.regex.Pattern;
 import java.util.stream.IntStream;
 
+import static java.text.MessageFormat.format;
+import static java.util.Collections.emptySet;
+import static org.apache.struts2.StrutsConstants.STRUTS_ALLOWLIST_CLASSES;
+import static org.apache.struts2.StrutsConstants.STRUTS_ALLOWLIST_PACKAGE_NAMES;
 import static org.apache.struts2.util.ConfigParseUtil.toClassObjectsSet;
 import static org.apache.struts2.util.ConfigParseUtil.toClassesSet;
 import static org.apache.struts2.util.ConfigParseUtil.toNewClassesSet;
 import static org.apache.struts2.util.ConfigParseUtil.toNewPackageNamesSet;
 import static org.apache.struts2.util.ConfigParseUtil.toNewPatternsSet;
 import static org.apache.struts2.util.ConfigParseUtil.toPackageNamesSet;
 import static org.apache.struts2.util.DebugUtils.logWarningForFirstOccurrence;
-import static java.text.MessageFormat.format;
-import static java.util.Collections.emptySet;
-import static org.apache.struts2.StrutsConstants.STRUTS_ALLOWLIST_CLASSES;
-import static org.apache.struts2.StrutsConstants.STRUTS_ALLOWLIST_PACKAGE_NAMES;
 
 /**
  * Allows access decisions to be made on the basis of whether a member is static or not.
@@ -141,6 +141,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)) {
@@ -209,7 +212,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/org/apache/struts2/util/ProxyUtil.java b/core/src/main/java/org/apache/struts2/util/ProxyUtil.java
@@ -18,12 +18,12 @@
  */
 package org.apache.struts2.util;
 
-import org.apache.struts2.ognl.DefaultOgnlCacheFactory;
-import org.apache.struts2.ognl.OgnlCache;
-import org.apache.struts2.ognl.OgnlCacheFactory;
 import org.apache.commons.lang3.reflect.ConstructorUtils;
 import org.apache.commons.lang3.reflect.FieldUtils;
 import org.apache.commons.lang3.reflect.MethodUtils;
+import org.apache.struts2.ognl.DefaultOgnlCacheFactory;
+import org.apache.struts2.ognl.OgnlCache;
+import org.apache.struts2.ognl.OgnlCacheFactory;
 import org.hibernate.Hibernate;
 import org.hibernate.proxy.HibernateProxy;
 
@@ -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/org/apache/struts2/ognl/OgnlValueStackTest.java b/core/src/test/java/org/apache/struts2/ognl/OgnlValueStackTest.java
@@ -18,17 +18,26 @@
  */
 package org.apache.struts2.ognl;
 
+import ognl.OgnlException;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.core.LogEvent;
+import org.apache.logging.log4j.core.Logger;
+import org.apache.logging.log4j.core.appender.AbstractAppender;
 import org.apache.struts2.SimpleAction;
+import org.apache.struts2.StrutsConstants;
+import org.apache.struts2.StrutsException;
 import org.apache.struts2.TestBean;
-import org.apache.struts2.text.TextProvider;
 import org.apache.struts2.XWorkTestCase;
 import org.apache.struts2.config.ConfigurationException;
+import org.apache.struts2.config.DefaultPropertiesProvider;
 import org.apache.struts2.conversion.impl.ConversionData;
 import org.apache.struts2.conversion.impl.XWorkConverter;
 import org.apache.struts2.inject.ContainerBuilder;
 import org.apache.struts2.ognl.accessor.RootAccessor;
 import org.apache.struts2.test.StubConfigurationProvider;
 import org.apache.struts2.test.TestBean2;
+import org.apache.struts2.text.TextProvider;
 import org.apache.struts2.util.Bar;
 import org.apache.struts2.util.BarJunior;
 import org.apache.struts2.util.Cat;
@@ -37,15 +46,6 @@
 import org.apache.struts2.util.ValueStackFactory;
 import org.apache.struts2.util.location.LocatableProperties;
 import org.apache.struts2.util.reflection.ReflectionContextState;
-import ognl.OgnlException;
-import org.apache.commons.lang3.StringUtils;
-import org.apache.logging.log4j.LogManager;
-import org.apache.logging.log4j.core.LogEvent;
-import org.apache.logging.log4j.core.Logger;
-import org.apache.logging.log4j.core.appender.AbstractAppender;
-import org.apache.struts2.StrutsConstants;
-import org.apache.struts2.StrutsException;
-import org.apache.struts2.config.DefaultPropertiesProvider;
 
 import java.io.ByteArrayInputStream;
 import java.io.ByteArrayOutputStream;
@@ -1234,6 +1234,33 @@ public void testOgnlValueStackFromOgnlValueStackFactoryAllStaticAccess() throws
         assertNull("accessed private field (result not null) ?", accessedValue);
     }
 
+    public void testFindValueWithConstructorAndProxyChecks() {
+        loadButSet(Map.of(
+                StrutsConstants.STRUTS_DISALLOW_PROXY_OBJECT_ACCESS, Boolean.TRUE.toString(),
+                StrutsConstants.STRUTS_DISALLOW_PROXY_MEMBER_ACCESS, Boolean.TRUE.toString()));
+        refreshContainerFields();
+
+        String value = "test";
+        String ognlResult = (String) vs.findValue(
+                "new org.apache.struts2.ognl.OgnlValueStackTest$ValueHolder('" + value + "').value", String.class);
+
+        assertEquals(value, ognlResult);
+    }
+
+    @SuppressWarnings({"unused", "ClassCanBeRecord"})
+    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/org/apache/struts2/spring/SpringProxyUtilTest.java b/plugins/spring/src/test/java/org/apache/struts2/spring/SpringProxyUtilTest.java
@@ -23,9 +23,9 @@
 import org.apache.struts2.TestBean;
 import org.apache.struts2.TestSubBean;
 import org.apache.struts2.XWorkTestCase;
+import org.apache.struts2.config.StrutsXmlConfigurationProvider;
 import org.apache.struts2.config.providers.XmlConfigurationProvider;
 import org.apache.struts2.util.ProxyUtil;
-import org.apache.struts2.config.StrutsXmlConfigurationProvider;
 import org.springframework.context.ApplicationContext;
 
 /**
@@ -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));