You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@struts.apache.org by ya...@apache.org on 2019/01/31 14:31:46 UTC
[struts] branch struts-2-5-x updated: Fix for NPE issue discovered
in WW-5004. (#316)
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 <43...@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>