From 2e7cf639649e00b06374412c1ff917c4ba11129f Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Fri, 25 Jan 2019 12:29:48 +0100 Subject: [PATCH 1/3] WW-5007 Upgrades Jackson libraries to version 2.9.8 --- pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pom.xml b/pom.xml index b341a10a1..529f7a3dd 100644 --- a/pom.xml +++ b/pom.xml @@ -103,7 +103,7 @@ 3.0.8 1.0.7 2.11.1 - 2.9.7 + 2.9.8 1.7 From 5524c579d29dbe91fe82428a74fb4cb1888330ba Mon Sep 17 00:00:00 2001 From: JCgH4164838Gh792C124B5 <43964333+JCgH4164838Gh792C124B5@users.noreply.github.com> Date: Thu, 31 Jan 2019 09:31:41 -0500 Subject: [PATCH 2/3] 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 ++++- .../opensymphony/xwork2/util/ProxyUtil.java | 5 +- .../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 + .../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 a5f476fec..92193cbc4 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 4e1e964eb..f906f989d 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 0f9ec65a2..9b0e7d4ea 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 c32e85878..f8669ba22 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> 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 132870dd9..1f0c0ee90 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 @@ + 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 3065a1ade..7551cb10d 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 @@ + 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 6dc36eb3c..220432cae 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 @@ + 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 787d8a632..59914b6c4 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 @@ + diff --git a/pom.xml b/pom.xml index 529f7a3dd..31969b584 100644 --- a/pom.xml +++ b/pom.xml @@ -98,7 +98,7 @@ UTF-8 4.3.20.RELEASE - 3.1.21 + 3.1.22 5.2 3.0.8 1.0.7 From 71267a9b4b99ad05f20a376478b165c0b3f2b74a Mon Sep 17 00:00:00 2001 From: Aleksandr Mashchenko Date: Thu, 31 Jan 2019 17:32:36 +0200 Subject: [PATCH 3/3] Revert some changes to be more consistent with 2.6 version --- .../xwork2/ognl/SecurityMemberAccess.java | 34 ++----------------- 1 file changed, 3 insertions(+), 31 deletions(-) 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 f906f989d..0e32efd93 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java @@ -104,7 +104,7 @@ public class SecurityMemberAccess extends DefaultMemberAccess { } boolean allow = true; - if (!checkStaticMemberAccess(member)) { + if (!checkStaticMethodAccess(member)) { LOG.warn("Access to static [{}] is blocked!", member); allow = false; } @@ -118,38 +118,10 @@ 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) { - 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(); + int modifiers = member.getModifiers(); if (Modifier.isStatic(modifiers)) { - if (member instanceof Field) { - return Modifier.isPublic(modifiers); - } else { - return allowStaticMethodAccess && Modifier.isPublic(modifiers); - } + return allowStaticMethodAccess; } else { return true; }