struts-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From yasserzam...@apache.org
Subject [struts] branch struts-2-5-x updated: Fix for NPE issue discovered in WW-5004. (#316)
Date Thu, 31 Jan 2019 14:31:46 GMT
This is an automated email from the ASF dual-hosted git repository.

yasserzamani pushed a commit to branch struts-2-5-x
in repository https://gitbox.apache.org/repos/asf/struts.git


The following commit(s) were added to refs/heads/struts-2-5-x by this push:
     new 5524c57  Fix for NPE issue discovered in WW-5004. (#316)
5524c57 is described below

commit 5524c579d29dbe91fe82428a74fb4cb1888330ba
Author: JCgH4164838Gh792C124B5 <43964333+JCgH4164838Gh792C124B5@users.noreply.github.com>
AuthorDate: Thu Jan 31 09:31:41 2019 -0500

    Fix for NPE issue discovered in WW-5004. (#316)
    
    * Fix for NPE issue discovered in WW-5004.
    - Guard fix for a NPE that can arise under certain conditions, identified by A. Mashchenko.
    
    * Fix for NPE issue discovered in WW-5004 (amended commit).
    - Guard fix for a NPE that can arise under certain conditions, identified by A. Mashchenko.
    - Requires the following elements to implement a fuller fix:
      - Back-port relevant guard logic in ProxyUtil from master into 2.5.x to deal with the
NPE.
      - Update SecurityMemberAccess to block access to static fields.
      - Upgrade to OGNL 3.1.22 (re-enables access to public static fields w/out access checks).
      - Add unit test to confirm proper functionality of the fix.
      - Correct missing entry in 4 test configuration XML files (needed for new unit test).
    - Replaced literal injection parameter name for setStaticFieldAccessLevel in OgnlValueStackFactory
with the appropriate constant.
      Note: Even though a constant was defined in StrutsConstants, the value for the injection
name in all places is the XWorkConstants.
            It has to remain the same to avoid breaking anything.
---
 .../xwork2/ognl/OgnlValueStackFactory.java         |   3 +-
 .../xwork2/ognl/SecurityMemberAccess.java          |  36 +++++-
 .../com/opensymphony/xwork2/util/ProxyUtil.java    |   5 +-
 .../com/opensymphony/xwork2/ognl/OgnlUtilTest.java | 130 ++++++++++++++++++++-
 .../xwork-test-allowstatic-devmode-false.xml       |   1 +
 .../xwork-test-allowstatic-devmode-true.xml        |   1 +
 .../providers/xwork-test-allowstatic-true.xml      |   1 +
 .../config/providers/xwork-test-devmode-true.xml   |   1 +
 pom.xml                                            |   2 +-
 9 files changed, 171 insertions(+), 9 deletions(-)

diff --git a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStackFactory.java b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStackFactory.java
index a5f476f..92193cb 100644
--- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStackFactory.java
+++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStackFactory.java
@@ -19,6 +19,7 @@
 package com.opensymphony.xwork2.ognl;
 
 import com.opensymphony.xwork2.ActionContext;
+import com.opensymphony.xwork2.XWorkConstants;
 import com.opensymphony.xwork2.TextProvider;
 import com.opensymphony.xwork2.conversion.NullHandler;
 import com.opensymphony.xwork2.conversion.impl.XWorkConverter;
@@ -61,7 +62,7 @@ public class OgnlValueStackFactory implements ValueStackFactory {
         this.textProvider = textProvider;
     }
     
-    @Inject(value="allowStaticMethodAccess", required=false)
+    @Inject(value = XWorkConstants.ALLOW_STATIC_METHOD_ACCESS, required = false)
     protected void setAllowStaticMethodAccess(String allowStaticMethodAccess) {
         this.allowStaticMethodAccess = BooleanUtils.toBoolean(allowStaticMethodAccess);
     }
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 4e1e964..f906f98 100644
--- a/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java
+++ b/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java
@@ -49,7 +49,7 @@ public class SecurityMemberAccess extends DefaultMemberAccess {
     /**
      * SecurityMemberAccess
      *   - access decisions based on whether member is static (or not)
-     *   - block or allow access to properties (configureable-after-construction)
+     *   - block or allow access to properties (configurable-after-construction)
      * 
      * @param allowStaticMethodAccess
      */
@@ -104,7 +104,7 @@ public class SecurityMemberAccess extends DefaultMemberAccess {
         }
 
         boolean allow = true;
-        if (!checkStaticMethodAccess(member)) {
+        if (!checkStaticMemberAccess(member)) {
             LOG.warn("Access to static [{}] is blocked!", member);
             allow = false;
         }
@@ -118,10 +118,38 @@ public class SecurityMemberAccess extends DefaultMemberAccess {
         return super.isAccessible(context, target, member, propertyName) && isAcceptableProperty(propertyName);
     }
 
+    /**
+     * Retain backwards-compatibility for any implementations extending this class prior
to 2.5.21.
+     * 
+     * Deprecated as of 2.5.21.
+     * 
+     * @param member
+     * 
+     * @return
+     */
+    @Deprecated
     protected boolean checkStaticMethodAccess(Member member) {
-        int modifiers = member.getModifiers();
+        return checkStaticMemberAccess(member);
+    }
+
+    /**
+     * Check access for static members
+     * 
+     * Static non-field access result is a logical and of allowStaticMethodAccess and public.
+     * Static field access result is true if-and-only-if the field is public.
+     * 
+     * @param member
+     * 
+     * @return
+     */
+    protected boolean checkStaticMemberAccess(Member member) {
+        final int modifiers = member.getModifiers();
         if (Modifier.isStatic(modifiers)) {
-            return allowStaticMethodAccess;
+            if (member instanceof Field) {
+                return Modifier.isPublic(modifiers);
+            } else {
+                return allowStaticMethodAccess && Modifier.isPublic(modifiers);
+            }
         } else {
             return true;
         }
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 0f9ec65..9b0e7d4 100644
--- a/core/src/main/java/com/opensymphony/xwork2/util/ProxyUtil.java
+++ b/core/src/main/java/com/opensymphony/xwork2/util/ProxyUtil.java
@@ -82,13 +82,14 @@ public class ProxyUtil {
     }
 
     /**
-     * Check whether the given member is a proxy member of a proxy object.
+     * Check whether the given member is a proxy member of a proxy object or is a static
proxy member.
      * @param member the member to check
      * @param object the object to check
      */
     public static boolean isProxyMember(Member member, Object object) {
-        if (!isProxy(object))
+        if (!Modifier.isStatic(member.getModifiers()) && !isProxy(object)) {
             return false;
+        }
 
         Boolean flag = isProxyMemberCache.get(member);
         if (flag != null) {
diff --git a/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java b/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java
index c32e858..f8669ba 100644
--- a/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java
+++ b/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java
@@ -39,7 +39,17 @@ import java.util.*;
 import java.util.regex.Pattern;
 
 public class OgnlUtilTest extends XWorkTestCase {
-    
+
+    // Fields for static field access test
+    public static final String STATIC_FINAL_PUBLIC_ATTRIBUTE = "Static_Final_Public_Attribute";
+    static final String STATIC_FINAL_PACKAGE_ATTRIBUTE = "Static_Final_Package_Attribute";
+    protected static final String STATIC_FINAL_PROTECTED_ATTRIBUTE = "Static_Final_Protected_Attribute";
+    private static final String STATIC_FINAL_PRIVATE_ATTRIBUTE = "Static_Final_Private_Attribute";
+    public static String STATIC_PUBLIC_ATTRIBUTE = "Static_Public_Attribute";
+    static String STATIC_PACKAGE_ATTRIBUTE = "Static_Package_Attribute";
+    protected static String STATIC_PROTECTED_ATTRIBUTE = "Static_Protected_Attribute";
+    private static String STATIC_PRIVATE_ATTRIBUTE = "Static_Private_Attribute";
+
     private OgnlUtil ognlUtil;
     
     @Override
@@ -1024,6 +1034,124 @@ public class OgnlUtilTest extends XWorkTestCase {
         assertTrue("fakepackage4.package not in exclusions?", excludedPackageNames.contains("fakepackage4.package"));
     }
 
+    /**
+     * Ensure getValue permits public static field access, but prevents non-public static
field access
+     */
+    public void testStaticFieldGetValue() {
+        OgnlContext context = null;
+        Object accessedValue;
+
+        try {
+            reloadTestContainerConfiguration(false, false);  // Test with allow static methods
false
+            context = (OgnlContext) ognlUtil.createDefaultContext(null);
+        } catch (Exception ex) {
+            fail("unable to reload test configuration? Exception: " + ex);
+        }
+        try {
+            accessedValue = ognlUtil.getValue("@com.opensymphony.xwork2.ognl.OgnlUtilTest@STATIC_FINAL_PUBLIC_ATTRIBUTE",
context, null);
+            assertEquals("accessed field value not equal to actual?", accessedValue, STATIC_FINAL_PUBLIC_ATTRIBUTE);
+        } catch (Exception ex) {
+            fail("static final public field access failed ? Exception: " + ex);
+        }
+        try {
+            accessedValue = ognlUtil.getValue("@com.opensymphony.xwork2.ognl.OgnlUtilTest@STATIC_PUBLIC_ATTRIBUTE",
context, null);
+            assertEquals("accessed field value not equal to actual?", accessedValue, STATIC_PUBLIC_ATTRIBUTE);
+        } catch (Exception ex) {
+            fail("static public field access failed ? Exception: " + ex);
+        }
+        try {
+            accessedValue = ognlUtil.getValue("@com.opensymphony.xwork2.ognl.OgnlUtilTest@STATIC_FINAL_PACKAGE_ATTRIBUTE",
context, null);
+            fail("static final package field access succeeded?");
+        } catch (Exception ex) {
+            assertTrue("Exception not an OgnlException?", ex instanceof OgnlException);
+        }
+        try {
+            accessedValue = ognlUtil.getValue("@com.opensymphony.xwork2.ognl.OgnlUtilTest@STATIC_PACKAGE_ATTRIBUTE",
context, null);
+            fail("static package field access succeeded?");
+        } catch (Exception ex) {
+            assertTrue("Exception not an OgnlException?", ex instanceof OgnlException);
+        }
+        try {
+            accessedValue = ognlUtil.getValue("@com.opensymphony.xwork2.ognl.OgnlUtilTest@STATIC_FINAL_PROTECTED_ATTRIBUTE",
context, null);
+            fail("static final protected field access succeeded?");
+        } catch (Exception ex) {
+            assertTrue("Exception not an OgnlException?", ex instanceof OgnlException);
+        }
+        try {
+            accessedValue = ognlUtil.getValue("@com.opensymphony.xwork2.ognl.OgnlUtilTest@STATIC_PROTECTED_ATTRIBUTE",
context, null);
+            fail("static protected field access succeeded?");
+        } catch (Exception ex) {
+            assertTrue("Exception not an OgnlException?", ex instanceof OgnlException);
+        }
+        try {
+            accessedValue = ognlUtil.getValue("@com.opensymphony.xwork2.ognl.OgnlUtilTest@STATIC_FINAL_PRIVATE_ATTRIBUTE",
context, null);
+            fail("static final private field access succeeded?");
+        } catch (Exception ex) {
+            assertTrue("Exception not an OgnlException?", ex instanceof OgnlException);
+        }
+        try {
+            accessedValue = ognlUtil.getValue("@com.opensymphony.xwork2.ognl.OgnlUtilTest@STATIC_PRIVATE_ATTRIBUTE",
context, null);
+            fail("static private field access succeeded?");
+        } catch (Exception ex) {
+            assertTrue("Exception not an OgnlException?", ex instanceof OgnlException);
+        }
+
+        try {
+            reloadTestContainerConfiguration(false, true);  // Re-test with allow static
methods true
+            context = (OgnlContext) ognlUtil.createDefaultContext(null);
+        } catch (Exception ex) {
+            fail("unable to reload test configuration? Exception: " + ex);
+        }
+        try {
+            accessedValue = ognlUtil.getValue("@com.opensymphony.xwork2.ognl.OgnlUtilTest@STATIC_FINAL_PUBLIC_ATTRIBUTE",
context, null);
+            assertEquals("accessed value not equal to actual?", accessedValue, STATIC_FINAL_PUBLIC_ATTRIBUTE);
+        } catch (Exception ex) {
+            fail("static final public field access failed ? Exception: " + ex);
+        }
+        try {
+            accessedValue = ognlUtil.getValue("@com.opensymphony.xwork2.ognl.OgnlUtilTest@STATIC_PUBLIC_ATTRIBUTE",
context, null);
+            assertEquals("accessed value not equal to actual?", accessedValue, STATIC_PUBLIC_ATTRIBUTE);
+        } catch (Exception ex) {
+            fail("static public field access failed ? Exception: " + ex);
+        }
+        try {
+            accessedValue = ognlUtil.getValue("@com.opensymphony.xwork2.ognl.OgnlUtilTest@STATIC_FINAL_PACKAGE_ATTRIBUTE",
context, null);
+            fail("static final package field access succeeded?");
+        } catch (Exception ex) {
+            assertTrue("Exception not an OgnlException?", ex instanceof OgnlException);
+        }
+        try {
+            accessedValue = ognlUtil.getValue("@com.opensymphony.xwork2.ognl.OgnlUtilTest@STATIC_PACKAGE_ATTRIBUTE",
context, null);
+            fail("static package field access succeeded?");
+        } catch (Exception ex) {
+            assertTrue("Exception not an OgnlException?", ex instanceof OgnlException);
+        }
+        try {
+            accessedValue = ognlUtil.getValue("@com.opensymphony.xwork2.ognl.OgnlUtilTest@STATIC_FINAL_PROTECTED_ATTRIBUTE",
context, null);
+            fail("static final protected field access succeeded?");
+        } catch (Exception ex) {
+            assertTrue("Exception not an OgnlException?", ex instanceof OgnlException);
+        }
+        try {
+            accessedValue = ognlUtil.getValue("@com.opensymphony.xwork2.ognl.OgnlUtilTest@STATIC_PROTECTED_ATTRIBUTE",
context, null);
+            fail("static protected field access succeeded?");
+        } catch (Exception ex) {
+            assertTrue("Exception not an OgnlException?", ex instanceof OgnlException);
+        }
+        try {
+            accessedValue = ognlUtil.getValue("@com.opensymphony.xwork2.ognl.OgnlUtilTest@STATIC_FINAL_PRIVATE_ATTRIBUTE",
context, null);
+            fail("static final private field access succeeded?");
+        } catch (Exception ex) {
+            assertTrue("Exception not an OgnlException?", ex instanceof OgnlException);
+        }
+        try {
+            accessedValue = ognlUtil.getValue("@com.opensymphony.xwork2.ognl.OgnlUtilTest@STATIC_PRIVATE_ATTRIBUTE",
context, null);
+            fail("static private field access succeeded?");
+        } catch (Exception ex) {
+            assertTrue("Exception not an OgnlException?", ex instanceof OgnlException);
+        }
+    }
+
     private void internalTestInitialEmptyOgnlUtilExclusions(OgnlUtil ognlUtilParam) throws
Exception {
         Set<Class<?>> excludedClasses = ognlUtilParam.getExcludedClasses();
         assertNotNull("parameter (default) exluded classes null?", excludedClasses);
diff --git a/core/src/test/resources/com/opensymphony/xwork2/config/providers/xwork-test-allowstatic-devmode-false.xml
b/core/src/test/resources/com/opensymphony/xwork2/config/providers/xwork-test-allowstatic-devmode-false.xml
index 132870d..1f0c0ee 100644
--- a/core/src/test/resources/com/opensymphony/xwork2/config/providers/xwork-test-allowstatic-devmode-false.xml
+++ b/core/src/test/resources/com/opensymphony/xwork2/config/providers/xwork-test-allowstatic-devmode-false.xml
@@ -33,6 +33,7 @@
     <constant name="enableOGNLExpressionCache" value="true" />
     <constant name="enableOGNLEvalExpression" value="false" />
     <constant name="reloadXmlConfiguration" value="false" />
+    <constant name="allowStaticMethodAccess" value="false" />
     <constant name="struts.ognl.allowStaticMethodAccess" value="false" />
     <constant name="struts.enable.DynamicMethodInvocation" value="false" />
     <constant name="struts.dispatcher.errorHandler" value="struts" />
diff --git a/core/src/test/resources/com/opensymphony/xwork2/config/providers/xwork-test-allowstatic-devmode-true.xml
b/core/src/test/resources/com/opensymphony/xwork2/config/providers/xwork-test-allowstatic-devmode-true.xml
index 3065a1a..7551cb1 100644
--- a/core/src/test/resources/com/opensymphony/xwork2/config/providers/xwork-test-allowstatic-devmode-true.xml
+++ b/core/src/test/resources/com/opensymphony/xwork2/config/providers/xwork-test-allowstatic-devmode-true.xml
@@ -33,6 +33,7 @@
     <constant name="enableOGNLExpressionCache" value="true" />
     <constant name="enableOGNLEvalExpression" value="false" />
     <constant name="reloadXmlConfiguration" value="false" />
+    <constant name="allowStaticMethodAccess" value="true" />
     <constant name="struts.ognl.allowStaticMethodAccess" value="true" />
     <constant name="struts.enable.DynamicMethodInvocation" value="false" />
     <constant name="struts.dispatcher.errorHandler" value="struts" />
diff --git a/core/src/test/resources/com/opensymphony/xwork2/config/providers/xwork-test-allowstatic-true.xml
b/core/src/test/resources/com/opensymphony/xwork2/config/providers/xwork-test-allowstatic-true.xml
index 6dc36eb..220432c 100644
--- a/core/src/test/resources/com/opensymphony/xwork2/config/providers/xwork-test-allowstatic-true.xml
+++ b/core/src/test/resources/com/opensymphony/xwork2/config/providers/xwork-test-allowstatic-true.xml
@@ -33,6 +33,7 @@
     <constant name="enableOGNLExpressionCache" value="true" />
     <constant name="enableOGNLEvalExpression" value="false" />
     <constant name="reloadXmlConfiguration" value="false" />
+    <constant name="allowStaticMethodAccess" value="true" />
     <constant name="struts.ognl.allowStaticMethodAccess" value="true" />
     <constant name="struts.enable.DynamicMethodInvocation" value="false" />
     <constant name="struts.dispatcher.errorHandler" value="struts" />
diff --git a/core/src/test/resources/com/opensymphony/xwork2/config/providers/xwork-test-devmode-true.xml
b/core/src/test/resources/com/opensymphony/xwork2/config/providers/xwork-test-devmode-true.xml
index 787d8a6..59914b6 100644
--- a/core/src/test/resources/com/opensymphony/xwork2/config/providers/xwork-test-devmode-true.xml
+++ b/core/src/test/resources/com/opensymphony/xwork2/config/providers/xwork-test-devmode-true.xml
@@ -33,6 +33,7 @@
     <constant name="enableOGNLExpressionCache" value="true" />
     <constant name="enableOGNLEvalExpression" value="false" />
     <constant name="reloadXmlConfiguration" value="false" />
+    <constant name="allowStaticMethodAccess" value="false" />
     <constant name="struts.ognl.allowStaticMethodAccess" value="false" />
     <constant name="struts.enable.DynamicMethodInvocation" value="false" />
     <constant name="struts.dispatcher.errorHandler" value="struts" />
diff --git a/pom.xml b/pom.xml
index 529f7a3..31969b5 100644
--- a/pom.xml
+++ b/pom.xml
@@ -98,7 +98,7 @@
         <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
 
         <spring.platformVersion>4.3.20.RELEASE</spring.platformVersion>
-        <ognl.version>3.1.21</ognl.version>
+        <ognl.version>3.1.22</ognl.version>
         <asm.version>5.2</asm.version>
         <tiles.version>3.0.8</tiles.version>
         <tiles-request.version>1.0.7</tiles-request.version>


Mime
View raw message