From 925eb62949347eaf29867b295bcfbc55f210f23a Mon Sep 17 00:00:00 2001 From: JCgH4164838Gh792C124B5 <43964333+JCgH4164838Gh792C124B5@users.noreply.github.com> Date: Mon, 28 Jan 2019 02:45:57 -0500 Subject: [PATCH 1/2] Fix for access issue for 2.6 discovered in WW-5004 (2nd amended commit): - Restored ability to access public static fields (true by default). - Introduced a boolean configuration flag (allowStaticFieldAccess). - Replaced one remaining Boolean.parseBoolean() conversion in OgnlUtil use BooleanUtils.toBoolean(). - Enhanced unit tests to confirm proper operation of the fix. - Replicating L. Lenart's change in PR#317: - Removed injection parameter for setAllowStaticMethodAccess in OgnlValueStackFactory. - Replaced with lazy retrieval of allowStaticMethodAccess from container. - Used same pattern for the new allowStaticFieldAccess flag. - Added retrieval methods for both flags from the container. --- .../providers/XWorkConfigurationProvider.java | 1 + .../opensymphony/xwork2/ognl/OgnlUtil.java | 10 +- .../xwork2/ognl/OgnlValueStack.java | 19 +- .../xwork2/ognl/OgnlValueStackFactory.java | 32 +++- .../xwork2/ognl/SecurityMemberAccess.java | 42 ++++- .../org/apache/struts2/StrutsConstants.java | 3 + .../ParametersInterceptorTest.java | 2 +- .../xwork2/ognl/OgnlUtilTest.java | 166 +++++++++++++++++- .../xwork2/ognl/OgnlValueStackTest.java | 10 +- .../ognl/SecurityMemberAccessProxyTest.java | 4 +- .../xwork2/ognl/SecurityMemberAccessTest.java | 154 +++++++++++++--- .../xwork2/ognl/SetPropertiesTest.java | 2 +- .../result/ServletRedirectResultTest.java | 2 +- .../SecurityMemberAccessInServletsTest.java | 4 +- .../xwork-test-staticfield-false.xml | 87 +++++++++ .../providers/xwork-test-staticfield-true.xml | 87 +++++++++ 16 files changed, 553 insertions(+), 72 deletions(-) create mode 100644 core/src/test/resources/com/opensymphony/xwork2/config/providers/xwork-test-staticfield-false.xml create mode 100644 core/src/test/resources/com/opensymphony/xwork2/config/providers/xwork-test-staticfield-true.xml diff --git a/core/src/main/java/com/opensymphony/xwork2/config/providers/XWorkConfigurationProvider.java b/core/src/main/java/com/opensymphony/xwork2/config/providers/XWorkConfigurationProvider.java index a34a42133..cffb451e8 100644 --- a/core/src/main/java/com/opensymphony/xwork2/config/providers/XWorkConfigurationProvider.java +++ b/core/src/main/java/com/opensymphony/xwork2/config/providers/XWorkConfigurationProvider.java @@ -224,6 +224,7 @@ public class XWorkConfigurationProvider implements ConfigurationProvider { props.setProperty(StrutsConstants.STRUTS_ENABLE_OGNL_EVAL_EXPRESSION, Boolean.FALSE.toString()); props.setProperty(StrutsConstants.STRUTS_CONFIGURATION_XML_RELOAD, Boolean.FALSE.toString()); props.setProperty(StrutsConstants.STRUTS_ALLOW_STATIC_METHOD_ACCESS, Boolean.FALSE.toString()); + props.setProperty(StrutsConstants.STRUTS_ALLOW_STATIC_FIELD_ACCESS, Boolean.TRUE.toString()); } } diff --git a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java index 7fc0c750a..084723ea1 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java @@ -66,6 +66,7 @@ public class OgnlUtil { private Set excludedPackageNames; private Container container; + private boolean allowStaticFieldAccess = true; private boolean allowStaticMethodAccess; private boolean disallowProxyMemberAccess; @@ -173,6 +174,11 @@ public class OgnlUtil { this.container = container; } + @Inject(value = StrutsConstants.STRUTS_ALLOW_STATIC_FIELD_ACCESS, required = false) + protected void setAllowStaticFieldAccess(String allowStaticFieldAccess) { + this.allowStaticFieldAccess = BooleanUtils.toBoolean(allowStaticFieldAccess); + } + @Inject(value = StrutsConstants.STRUTS_ALLOW_STATIC_METHOD_ACCESS, required = false) protected void setAllowStaticMethodAccess(String allowStaticMethodAccess) { this.allowStaticMethodAccess = BooleanUtils.toBoolean(allowStaticMethodAccess); @@ -180,7 +186,7 @@ public class OgnlUtil { @Inject(value = StrutsConstants.STRUTS_DISALLOW_PROXY_MEMBER_ACCESS, required = false) protected void setDisallowProxyMemberAccess(String disallowProxyMemberAccess) { - this.disallowProxyMemberAccess = Boolean.parseBoolean(disallowProxyMemberAccess); + this.disallowProxyMemberAccess = BooleanUtils.toBoolean(disallowProxyMemberAccess); } public boolean isDisallowProxyMemberAccess() { @@ -699,7 +705,7 @@ public class OgnlUtil { resolver = container.getInstance(CompoundRootAccessor.class); } - SecurityMemberAccess memberAccess = new SecurityMemberAccess(allowStaticMethodAccess); + SecurityMemberAccess memberAccess = new SecurityMemberAccess(allowStaticMethodAccess, allowStaticFieldAccess); memberAccess.setExcludedClasses(excludedClasses); memberAccess.setExcludedPackageNamePatterns(excludedPackageNamePatterns); memberAccess.setExcludedPackageNames(excludedPackageNames); diff --git a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java index 8f4e2b2dd..6a0ef6633 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java @@ -72,13 +72,13 @@ public class OgnlValueStack implements Serializable, ValueStack, ClearableValueS private boolean devMode; private boolean logMissingProperties; - protected OgnlValueStack(XWorkConverter xworkConverter, CompoundRootAccessor accessor, TextProvider prov, boolean allowStaticAccess) { - setRoot(xworkConverter, accessor, new CompoundRoot(), allowStaticAccess); + protected OgnlValueStack(XWorkConverter xworkConverter, CompoundRootAccessor accessor, TextProvider prov, boolean allowStaticMethodAccess, boolean allowStaticFieldAccess) { + setRoot(xworkConverter, accessor, new CompoundRoot(), allowStaticMethodAccess, allowStaticFieldAccess); push(prov); } - protected OgnlValueStack(ValueStack vs, XWorkConverter xworkConverter, CompoundRootAccessor accessor, boolean allowStaticAccess) { - setRoot(xworkConverter, accessor, new CompoundRoot(vs.getRoot()), allowStaticAccess); + protected OgnlValueStack(ValueStack vs, XWorkConverter xworkConverter, CompoundRootAccessor accessor, boolean allowStaticMethodAccess, boolean allowStaticFieldAccess) { + setRoot(xworkConverter, accessor, new CompoundRoot(vs.getRoot()), allowStaticMethodAccess, allowStaticFieldAccess); } @Inject @@ -91,9 +91,9 @@ public class OgnlValueStack implements Serializable, ValueStack, ClearableValueS } protected void setRoot(XWorkConverter xworkConverter, CompoundRootAccessor accessor, CompoundRoot compoundRoot, - boolean allowStaticMethodAccess) { + boolean allowStaticMethodAccess, boolean allowStaticFieldAccess) { this.root = compoundRoot; - this.securityMemberAccess = new SecurityMemberAccess(allowStaticMethodAccess); + this.securityMemberAccess = new SecurityMemberAccess(allowStaticMethodAccess, allowStaticFieldAccess); this.context = Ognl.createDefaultContext(this.root, securityMemberAccess, accessor, new OgnlTypeConverterWrapper(xworkConverter)); context.put(VALUE_STACK, this); ((OgnlContext) context).setTraceEvaluations(false); @@ -448,10 +448,11 @@ public class OgnlValueStack implements Serializable, ValueStack, ClearableValueS XWorkConverter xworkConverter = cont.getInstance(XWorkConverter.class); CompoundRootAccessor accessor = (CompoundRootAccessor) cont.getInstance(PropertyAccessor.class, CompoundRoot.class.getName()); TextProvider prov = cont.getInstance(TextProvider.class, "system"); - boolean allow = BooleanUtils.toBoolean(cont.getInstance(String.class, StrutsConstants.STRUTS_ALLOW_STATIC_METHOD_ACCESS)); - OgnlValueStack aStack = new OgnlValueStack(xworkConverter, accessor, prov, allow); + final boolean allowStaticMethod = BooleanUtils.toBoolean(cont.getInstance(String.class, StrutsConstants.STRUTS_ALLOW_STATIC_METHOD_ACCESS)); + final boolean allowStaticField = BooleanUtils.toBoolean(cont.getInstance(String.class, StrutsConstants.STRUTS_ALLOW_STATIC_FIELD_ACCESS)); + OgnlValueStack aStack = new OgnlValueStack(xworkConverter, accessor, prov, allowStaticMethod, allowStaticField); aStack.setOgnlUtil(cont.getInstance(OgnlUtil.class)); - aStack.setRoot(xworkConverter, accessor, this.root, allow); + aStack.setRoot(xworkConverter, accessor, this.root, allowStaticMethod, allowStaticField); return aStack; } 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 d33bdf27a..bf4e5716c 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStackFactory.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStackFactory.java @@ -37,6 +37,7 @@ import org.apache.logging.log4j.Logger; import java.util.Map; import java.util.Set; +import org.apache.struts2.StrutsConstants; /** * Creates an Ognl value stack @@ -49,7 +50,6 @@ public class OgnlValueStackFactory implements ValueStackFactory { protected CompoundRootAccessor compoundRootAccessor; protected TextProvider textProvider; protected Container container; - private boolean allowStaticMethodAccess; @Inject protected void setXWorkConverter(XWorkConverter converter) { @@ -60,21 +60,18 @@ public class OgnlValueStackFactory implements ValueStackFactory { protected void setTextProvider(TextProvider textProvider) { this.textProvider = textProvider; } - - @Inject(value="allowStaticMethodAccess", required=false) - protected void setAllowStaticMethodAccess(String allowStaticMethodAccess) { - this.allowStaticMethodAccess = BooleanUtils.toBoolean(allowStaticMethodAccess); - } public ValueStack createValueStack() { - ValueStack stack = new OgnlValueStack(xworkConverter, compoundRootAccessor, textProvider, allowStaticMethodAccess); + ValueStack stack = new OgnlValueStack(xworkConverter, compoundRootAccessor, textProvider, + containerAllowsStaticMethodAccess(), containerAllowsStaticFieldAccess()); container.inject(stack); stack.getContext().put(ActionContext.CONTAINER, container); return stack; } public ValueStack createValueStack(ValueStack stack) { - ValueStack result = new OgnlValueStack(stack, xworkConverter, compoundRootAccessor, allowStaticMethodAccess); + ValueStack result = new OgnlValueStack(stack, xworkConverter, compoundRootAccessor, + containerAllowsStaticMethodAccess(), containerAllowsStaticFieldAccess()); container.inject(result); stack.getContext().put(ActionContext.CONTAINER, container); return result; @@ -116,4 +113,23 @@ public class OgnlValueStackFactory implements ValueStackFactory { } this.container = container; } + + /** + * Retrieve allowsStaticMethodAccess state from the container (allows for lazy fetching) + * + * @return + */ + protected boolean containerAllowsStaticMethodAccess() { + return BooleanUtils.toBoolean(container.getInstance(String.class, StrutsConstants.STRUTS_ALLOW_STATIC_METHOD_ACCESS)); + } + + /** + * Retrieve allowStaticFieldAccess state from the container (allows for lazy fetching) + * + * @return + */ + protected boolean containerAllowsStaticFieldAccess() { + return BooleanUtils.toBoolean(container.getInstance(String.class, StrutsConstants.STRUTS_ALLOW_STATIC_FIELD_ACCESS)); + } + } 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 89e861c21..9174185a4 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java @@ -24,6 +24,7 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import java.lang.reflect.AccessibleObject; +import java.lang.reflect.Field; import java.lang.reflect.Member; import java.lang.reflect.Modifier; import java.util.Collections; @@ -40,6 +41,7 @@ public class SecurityMemberAccess implements MemberAccess { private static final Logger LOG = LogManager.getLogger(SecurityMemberAccess.class); + private final boolean allowStaticFieldAccess; private final boolean allowStaticMethodAccess; private Set excludeProperties = Collections.emptySet(); private Set acceptProperties = Collections.emptySet(); @@ -51,18 +53,24 @@ public class SecurityMemberAccess implements MemberAccess { /** * 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 + * @param allowStaticFieldAccess */ - public SecurityMemberAccess(boolean allowStaticMethodAccess) { + public SecurityMemberAccess(boolean allowStaticMethodAccess, boolean allowStaticFieldAccess) { this.allowStaticMethodAccess = allowStaticMethodAccess; + this.allowStaticFieldAccess = allowStaticFieldAccess; } public boolean getAllowStaticMethodAccess() { return allowStaticMethodAccess; } + public boolean getAllowStaticFieldAccess() { + return allowStaticFieldAccess; + } + @Override public Object setup(Map context, Object target, Member member, String propertyName) { Object result = null; @@ -102,12 +110,13 @@ public class SecurityMemberAccess implements MemberAccess { return true; } - if (!checkStaticMethodAccess(member)) { + if (!checkStaticMemberAccess(member)) { LOG.warn("Access to static [{}] is blocked!", member); return false; } final Class memberClass = member.getDeclaringClass(); + final int memberModifiers = member.getModifiers(); if (isClassExcluded(memberClass)) { LOG.warn("Declaring class of member type [{}] is excluded!", member); @@ -115,7 +124,7 @@ public class SecurityMemberAccess implements MemberAccess { } // target can be null in case of accessing static fields, since OGNL 3.2.8 - final Class targetClass = Modifier.isStatic(member.getModifiers()) ? memberClass : target.getClass(); + final Class targetClass = Modifier.isStatic(memberModifiers) ? memberClass : target.getClass(); if (isPackageExcluded(targetClass.getPackage(), memberClass.getPackage())) { LOG.warn("Package [{}] of target class [{}] of target [{}] or package [{}] of member [{}] are excluded!", targetClass.getPackage(), targetClass, @@ -133,16 +142,31 @@ public class SecurityMemberAccess implements MemberAccess { return false; } - return Modifier.isPublic(member.getModifiers()) && isAcceptableProperty(propertyName); + return Modifier.isPublic(memberModifiers) && isAcceptableProperty(propertyName); } - protected boolean checkStaticMethodAccess(Member member) { + /** + * Check access for static members + * + * Static non-field access result is a logical and of allowStaticMethodAccess and public. + * Static field access result is a logical and of allowStaticFieldAccess and public. + * Note: For non-static members, the result is always true. + * + * @param member + * + * @return + */ + protected boolean checkStaticMemberAccess(Member member) { final int modifiers = member.getModifiers(); if (Modifier.isStatic(modifiers)) { - if (allowStaticMethodAccess) { - LOG.debug("Support for accessing static methods [member: {}] is deprecated!", member); + if (member instanceof Field) { + return allowStaticFieldAccess && Modifier.isPublic(modifiers); + } else { + if (allowStaticMethodAccess) { + LOG.debug("Support for accessing static methods [member: {}] is deprecated!", member); + } + return allowStaticMethodAccess && Modifier.isPublic(modifiers); } - return allowStaticMethodAccess; } else { return true; } diff --git a/core/src/main/java/org/apache/struts2/StrutsConstants.java b/core/src/main/java/org/apache/struts2/StrutsConstants.java index 68bf36d30..5a47ddb9f 100644 --- a/core/src/main/java/org/apache/struts2/StrutsConstants.java +++ b/core/src/main/java/org/apache/struts2/StrutsConstants.java @@ -225,6 +225,9 @@ public final class StrutsConstants { /** The name of the parameter to create when mapping an id (used by some action mappers) */ public static final String STRUTS_ID_PARAMETER_NAME = "struts.mapper.idParameterName"; + /** The name of the parameter to determine whether static field access will be allowed in OGNL expressions or not */ + public static final String STRUTS_ALLOW_STATIC_FIELD_ACCESS = "struts.ognl.allowStaticFieldAccess"; + /** The name of the parameter to determine whether static method access will be allowed in OGNL expressions or not */ public static final String STRUTS_ALLOW_STATIC_METHOD_ACCESS = "struts.ognl.allowStaticMethodAccess"; diff --git a/core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java b/core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java index 9fb45d312..d2e0e1e3a 100644 --- a/core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java @@ -764,7 +764,7 @@ public class ParametersInterceptorTest extends XWorkTestCase { ValueStack stack = new OgnlValueStack( container.getInstance(XWorkConverter.class), (CompoundRootAccessor) container.getInstance(PropertyAccessor.class, CompoundRoot.class.getName()), - container.getInstance(TextProvider.class, "system"), true) { + container.getInstance(TextProvider.class, "system"), true, true) { @Override public void setValue(String expr, Object value) { actual.put(expr, value); 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 9cca95aed..81308f0f1 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,16 @@ 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 @@ -1038,6 +1047,127 @@ public class OgnlUtilTest extends XWorkTestCase { assertTrue("fakepackage4.package not in exclusions?", excludedPackageNames.contains("fakepackage4.package")); } + /** + * Ensure getValue: + * 1) When allowStaticFieldAccess true - Permits public static field access, + * prevents non-public static field access. + * 2) When allowStaticFieldAccess false - blocks all static field access, + */ + public void testStaticFieldGetValue() { + OgnlContext context = null; + Object accessedValue; + + try { + reloadTestContainerConfiguration(true); // Test with allowStaticFieldAccess 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 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); // Re-test with allowStaticFieldAccess 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); + fail("static final public field access succeded ?"); + } catch (Exception ex) { + assertTrue("Exception not an OgnlException?", ex instanceof OgnlException); + } + try { + accessedValue = ognlUtil.getValue("@com.opensymphony.xwork2.ognl.OgnlUtilTest@STATIC_PUBLIC_ATTRIBUTE", context, null); + fail("static public field access succeded ?"); + } catch (Exception ex) { + assertTrue("Exception not an OgnlException?", ex instanceof OgnlException); + } + 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); @@ -1181,17 +1311,17 @@ public class OgnlUtilTest extends XWorkTestCase { } } - private void reloadTestContainerConfiguration(boolean devMode, boolean allowStatic) throws Exception { + private void reloadTestContainerConfiguration(boolean devMode, boolean allowStaticMethod) throws Exception { super.tearDown(); ConfigurationProvider configurationProvider; - if (devMode == true && allowStatic == true) { + if (devMode == true && allowStaticMethod == true) { configurationProvider = new XmlConfigurationProvider("com/opensymphony/xwork2/config/providers/xwork-test-allowstatic-devmode-true.xml", true); } - else if (devMode == true && allowStatic == false) { + else if (devMode == true && allowStaticMethod == false) { configurationProvider = new XmlConfigurationProvider("com/opensymphony/xwork2/config/providers/xwork-test-devmode-true.xml", true); } - else if (devMode == false && allowStatic == true) { + else if (devMode == false && allowStaticMethod == true) { configurationProvider = new XmlConfigurationProvider("com/opensymphony/xwork2/config/providers/xwork-test-allowstatic-true.xml", true); } else { // devMode, allowStatic both false @@ -1214,6 +1344,32 @@ public class OgnlUtilTest extends XWorkTestCase { ognlUtil = container.getInstance(OgnlUtil.class); } + private void reloadTestContainerConfiguration(boolean allowStaticField) throws Exception { + super.tearDown(); + + ConfigurationProvider configurationProvider; + if (allowStaticField) { + configurationProvider = new XmlConfigurationProvider("com/opensymphony/xwork2/config/providers/xwork-test-staticfield-true.xml", true); + } else { + configurationProvider = new XmlConfigurationProvider("com/opensymphony/xwork2/config/providers/xwork-test-staticfield-false.xml", true); + } + + configurationManager = new ConfigurationManager(Container.DEFAULT_NAME); + configurationManager.addContainerProvider(configurationProvider); + configuration = configurationManager.getConfiguration(); + container = configuration.getContainer(); + container.inject(configurationProvider); + configurationProvider.init(configuration); + actionProxyFactory = container.getInstance(ActionProxyFactory.class); + + // Reset the value stack + ValueStack stack = container.getInstance(ValueStackFactory.class).createValueStack(); + stack.getContext().put(ActionContext.CONTAINER, container); + ActionContext.setContext(new ActionContext(stack.getContext())); + + ognlUtil = container.getInstance(OgnlUtil.class); + } + public static class Email { String address; diff --git a/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java b/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java index 1d97004bf..c7fd3686f 100644 --- a/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java @@ -56,14 +56,14 @@ public class OgnlValueStackTest extends XWorkTestCase { } private OgnlValueStack createValueStack() { - return createValueStack(true); + return createValueStack(true, true); } - private OgnlValueStack createValueStack(boolean allowStaticMethodAccess) { + private OgnlValueStack createValueStack(boolean allowStaticMethodAccess, boolean allowStaticFieldAccess) { OgnlValueStack stack = new OgnlValueStack( container.getInstance(XWorkConverter.class), (CompoundRootAccessor) container.getInstance(PropertyAccessor.class, CompoundRoot.class.getName()), - container.getInstance(TextProvider.class, "system"), allowStaticMethodAccess); + container.getInstance(TextProvider.class, "system"), allowStaticMethodAccess, allowStaticFieldAccess); container.inject(stack); ognlUtil.setAllowStaticMethodAccess(Boolean.toString(allowStaticMethodAccess)); return stack; @@ -280,7 +280,7 @@ public class OgnlValueStackTest extends XWorkTestCase { } public void testStaticMethodDisallow() { - OgnlValueStack vs = createValueStack(false); + OgnlValueStack vs = createValueStack(false, true); Dog dog = new Dog(); dog.setDeity("fido"); @@ -908,7 +908,7 @@ public class OgnlValueStackTest extends XWorkTestCase { OgnlValueStack stack2 = new OgnlValueStack(stack, container.getInstance(XWorkConverter.class), - (CompoundRootAccessor) container.getInstance(PropertyAccessor.class, CompoundRoot.class.getName()), true); + (CompoundRootAccessor) container.getInstance(PropertyAccessor.class, CompoundRoot.class.getName()), true, true); container.inject(stack2); assertEquals(stack.getRoot(), stack2.getRoot()); diff --git a/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessProxyTest.java b/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessProxyTest.java index 975b018d8..62a9d07fa 100644 --- a/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessProxyTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessProxyTest.java @@ -44,7 +44,7 @@ public class SecurityMemberAccessProxyTest extends XWorkTestCase { ActionProxy proxy = actionProxyFactory.createActionProxy(null, "chaintoAOPedTestSubBeanAction", null, context); - SecurityMemberAccess sma = new SecurityMemberAccess(false); + SecurityMemberAccess sma = new SecurityMemberAccess(false, true); sma.setDisallowProxyMemberAccess(true); Member member = proxy.getAction().getClass().getMethod("isExposeProxy"); @@ -57,7 +57,7 @@ public class SecurityMemberAccessProxyTest extends XWorkTestCase { ActionProxy proxy = actionProxyFactory.createActionProxy(null, "chaintoAOPedTestSubBeanAction", null, context); - SecurityMemberAccess sma = new SecurityMemberAccess(false); + SecurityMemberAccess sma = new SecurityMemberAccess(false, true); Member member = proxy.getAction().getClass().getMethod("isExposeProxy"); diff --git a/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java b/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java index fbc8f5e55..41c1ecb84 100644 --- a/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java @@ -21,7 +21,9 @@ package com.opensymphony.xwork2.ognl; import com.opensymphony.xwork2.util.TextParseUtil; import junit.framework.TestCase; +import java.lang.reflect.Field; import java.lang.reflect.Member; +import java.lang.reflect.Modifier; import java.util.Arrays; import java.util.Collections; import java.util.HashMap; @@ -43,7 +45,7 @@ public class SecurityMemberAccessTest extends TestCase { public void testWithoutClassExclusion() throws Exception { // given - SecurityMemberAccess sma = new SecurityMemberAccess(false); + SecurityMemberAccess sma = new SecurityMemberAccess(false, true); String propertyName = "stringField"; Member member = FooBar.class.getMethod("get" + propertyName.substring(0, 1).toUpperCase() + propertyName.substring(1)); @@ -57,7 +59,7 @@ public class SecurityMemberAccessTest extends TestCase { public void testClassExclusion() throws Exception { // given - SecurityMemberAccess sma = new SecurityMemberAccess(false); + SecurityMemberAccess sma = new SecurityMemberAccess(false, true); String propertyName = "stringField"; Member member = FooBar.class.getDeclaredMethod("get" + propertyName.substring(0, 1).toUpperCase() + propertyName.substring(1)); @@ -75,7 +77,7 @@ public class SecurityMemberAccessTest extends TestCase { public void testObjectClassExclusion() throws Exception { // given - SecurityMemberAccess sma = new SecurityMemberAccess(false); + SecurityMemberAccess sma = new SecurityMemberAccess(false, true); String propertyName = "toString"; Member member = FooBar.class.getMethod(propertyName); @@ -89,7 +91,7 @@ public class SecurityMemberAccessTest extends TestCase { public void testObjectOverwrittenMethodsExclusion() throws Exception { // given - SecurityMemberAccess sma = new SecurityMemberAccess(false); + SecurityMemberAccess sma = new SecurityMemberAccess(false, true); String propertyName = "hashCode"; Member member = FooBar.class.getMethod(propertyName); @@ -103,7 +105,7 @@ public class SecurityMemberAccessTest extends TestCase { public void testInterfaceInheritanceExclusion() throws Exception { // given - SecurityMemberAccess sma = new SecurityMemberAccess(false); + SecurityMemberAccess sma = new SecurityMemberAccess(false, true); String propertyName = "barLogic"; Member member = BarInterface.class.getMethod(propertyName); @@ -121,7 +123,7 @@ public class SecurityMemberAccessTest extends TestCase { public void testMiddleOfInheritanceExclusion1() throws Exception { // given - SecurityMemberAccess sma = new SecurityMemberAccess(false); + SecurityMemberAccess sma = new SecurityMemberAccess(false, true); String propertyName = "fooLogic"; Member member = FooBar.class.getMethod(propertyName); @@ -139,7 +141,7 @@ public class SecurityMemberAccessTest extends TestCase { public void testMiddleOfInheritanceExclusion3() throws Exception { // given - SecurityMemberAccess sma = new SecurityMemberAccess(false); + SecurityMemberAccess sma = new SecurityMemberAccess(false, true); String propertyName = "barLogic"; Member member = BarInterface.class.getMethod(propertyName); @@ -153,7 +155,7 @@ public class SecurityMemberAccessTest extends TestCase { public void testMiddleOfInheritanceExclusion4() throws Exception { // given - SecurityMemberAccess sma = new SecurityMemberAccess(false); + SecurityMemberAccess sma = new SecurityMemberAccess(false, true); String propertyName = "barLogic"; Member member = BarInterface.class.getMethod(propertyName); @@ -171,7 +173,7 @@ public class SecurityMemberAccessTest extends TestCase { public void testPackageExclusion() throws Exception { // given - SecurityMemberAccess sma = new SecurityMemberAccess(false); + SecurityMemberAccess sma = new SecurityMemberAccess(false, true); Set excluded = new HashSet<>(); excluded.add(Pattern.compile("^" + FooBar.class.getPackage().getName().replaceAll("\\.", "\\\\.") + ".*")); @@ -189,7 +191,7 @@ public class SecurityMemberAccessTest extends TestCase { public void testPackageNameExclusion() throws Exception { // given - SecurityMemberAccess sma = new SecurityMemberAccess(false); + SecurityMemberAccess sma = new SecurityMemberAccess(false, true); Set excluded = new HashSet<>(); excluded.add(FooBar.class.getPackage().getName()); @@ -207,7 +209,7 @@ public class SecurityMemberAccessTest extends TestCase { public void testDefaultPackageExclusion() { // given - SecurityMemberAccess sma = new SecurityMemberAccess(false); + SecurityMemberAccess sma = new SecurityMemberAccess(false, true); Set excluded = new HashSet<>(); excluded.add(Pattern.compile("^" + FooBar.class.getPackage().getName().replaceAll("\\.", "\\\\.") + ".*")); @@ -222,7 +224,7 @@ public class SecurityMemberAccessTest extends TestCase { public void testDefaultPackageExclusion2() { // given - SecurityMemberAccess sma = new SecurityMemberAccess(false); + SecurityMemberAccess sma = new SecurityMemberAccess(false, true); Set excluded = new HashSet<>(); excluded.add(Pattern.compile("^$")); @@ -237,7 +239,7 @@ public class SecurityMemberAccessTest extends TestCase { public void testAccessEnum() throws Exception { // given - SecurityMemberAccess sma = new SecurityMemberAccess(false); + SecurityMemberAccess sma = new SecurityMemberAccess(false, true); // when Member values = MyValues.class.getMethod("values"); @@ -249,7 +251,7 @@ public class SecurityMemberAccessTest extends TestCase { public void testAccessStatic() throws Exception { // given - SecurityMemberAccess sma = new SecurityMemberAccess(true); + SecurityMemberAccess sma = new SecurityMemberAccess(true, true); sma.setExcludedClasses(new HashSet>(Collections.singletonList(Class.class))); // when @@ -262,7 +264,7 @@ public class SecurityMemberAccessTest extends TestCase { public void testAccessStaticField() throws Exception { // given - SecurityMemberAccess sma = new SecurityMemberAccess(true); + SecurityMemberAccess sma = new SecurityMemberAccess(true, true); sma.setExcludedClasses(new HashSet>(Collections.singletonList(Class.class))); // when @@ -275,7 +277,7 @@ public class SecurityMemberAccessTest extends TestCase { public void testBlockedStaticFieldWhenFlagIsFalse() throws Exception { // given - SecurityMemberAccess sma = new SecurityMemberAccess(false); + SecurityMemberAccess sma = new SecurityMemberAccess(false, true); sma.setExcludedClasses(new HashSet>(Collections.singletonList(Class.class))); // when @@ -283,12 +285,96 @@ public class SecurityMemberAccessTest extends TestCase { boolean actual = sma.isAccessible(context, null, method, null); // then - assertFalse("Access to static field isn't blocked!", actual); + assertTrue("Access to public static field is blocked?", actual); + + // public static final test + // given + sma = new SecurityMemberAccess(false, true); + sma.setExcludedClasses(new HashSet>(Collections.singletonList(Class.class))); + + // when + method = StaticTester.class.getField("MIN_VALUE"); + actual = sma.isAccessible(context, null, method, null); + + // then + assertTrue("Access to public final static field is blocked?", actual); + + // package static test + // given + sma = new SecurityMemberAccess(false, true); + sma.setExcludedClasses(new HashSet>(Collections.singletonList(Class.class))); + + // when + method = StaticTester.getFieldByName("PACKAGE_STRING"); + actual = sma.isAccessible(context, null, method, null); + + // then + assertFalse("Access to package static field is allowed?", actual); + + // package final static test + // given + sma = new SecurityMemberAccess(false, true); + sma.setExcludedClasses(new HashSet>(Collections.singletonList(Class.class))); + + // when + method = StaticTester.getFieldByName("FINAL_PACKAGE_STRING"); + actual = sma.isAccessible(context, null, method, null); + + // then + assertFalse("Access to package final static field is allowed?", actual); + + // protected static test + // given + sma = new SecurityMemberAccess(false, true); + sma.setExcludedClasses(new HashSet>(Collections.singletonList(Class.class))); + + // when + method = StaticTester.getFieldByName("PROTECTED_STRING"); + actual = sma.isAccessible(context, null, method, null); + + // then + assertFalse("Access to protected static field is allowed?", actual); + + // protected final static test + // given + sma = new SecurityMemberAccess(false, true); + sma.setExcludedClasses(new HashSet>(Collections.singletonList(Class.class))); + + // when + method = StaticTester.getFieldByName("FINAL_PROTECTED_STRING"); + actual = sma.isAccessible(context, null, method, null); + + // then + assertFalse("Access to protected final static field is allowed?", actual); + + // private static test + // given + sma = new SecurityMemberAccess(false, true); + sma.setExcludedClasses(new HashSet>(Collections.singletonList(Class.class))); + + // when + method = StaticTester.getFieldByName("PRIVATE_STRING"); + actual = sma.isAccessible(context, null, method, null); + + // then + assertFalse("Access to private static field is allowed?", actual); + + // private final static test + // given + sma = new SecurityMemberAccess(false, true); + sma.setExcludedClasses(new HashSet>(Collections.singletonList(Class.class))); + + // when + method = StaticTester.getFieldByName("FINAL_PRIVATE_STRING"); + actual = sma.isAccessible(context, null, method, null); + + // then + assertFalse("Access to private final static field is allowed?", actual); } public void testBlockedStaticFieldWhenClassIsExcluded() throws Exception { // given - SecurityMemberAccess sma = new SecurityMemberAccess(true); + SecurityMemberAccess sma = new SecurityMemberAccess(true, true); sma.setExcludedClasses(new HashSet<>(Arrays.asList(Class.class, StaticTester.class))); // when @@ -301,7 +387,7 @@ public class SecurityMemberAccessTest extends TestCase { public void testBlockStaticAccess() throws Exception { // given - SecurityMemberAccess sma = new SecurityMemberAccess(false); + SecurityMemberAccess sma = new SecurityMemberAccess(false, true); sma.setExcludedClasses(new HashSet>(Collections.singletonList(Class.class))); // when @@ -314,7 +400,7 @@ public class SecurityMemberAccessTest extends TestCase { public void testBlockStaticAccessIfClassIsExcluded() throws Exception { // given - SecurityMemberAccess sma = new SecurityMemberAccess(false); + SecurityMemberAccess sma = new SecurityMemberAccess(false, true); sma.setExcludedClasses(new HashSet>(Collections.singletonList(Class.class))); // when @@ -327,7 +413,7 @@ public class SecurityMemberAccessTest extends TestCase { public void testAllowStaticAccessIfClassIsNotExcluded() throws Exception { // given - SecurityMemberAccess sma = new SecurityMemberAccess(true); + SecurityMemberAccess sma = new SecurityMemberAccess(true, true); sma.setExcludedClasses(new HashSet>(Collections.singletonList(ClassLoader.class))); // when @@ -340,7 +426,7 @@ public class SecurityMemberAccessTest extends TestCase { public void testAccessPrimitiveInt() throws Exception { // given - SecurityMemberAccess sma = new SecurityMemberAccess(false); + SecurityMemberAccess sma = new SecurityMemberAccess(false, true); sma.setExcludedPackageNames(TextParseUtil.commaDelimitedStringToSet("java.lang.,ognl,javax")); String propertyName = "intField"; @@ -355,7 +441,7 @@ public class SecurityMemberAccessTest extends TestCase { public void testAccessPrimitiveDoubleWithNames() throws Exception { // given - SecurityMemberAccess sma = new SecurityMemberAccess(false); + SecurityMemberAccess sma = new SecurityMemberAccess(false, true); sma.setExcludedPackageNames(TextParseUtil.commaDelimitedStringToSet("ognl.,javax.")); @@ -407,7 +493,7 @@ public class SecurityMemberAccessTest extends TestCase { public void testAccessPrimitiveDoubleWithPackageRegExs() throws Exception { // given - SecurityMemberAccess sma = new SecurityMemberAccess(false); + SecurityMemberAccess sma = new SecurityMemberAccess(false, true); Set patterns = new HashSet<>(); patterns.add(Pattern.compile("^java\\.lang\\..*")); sma.setExcludedPackageNamePatterns(patterns); @@ -424,7 +510,7 @@ public class SecurityMemberAccessTest extends TestCase { public void testAccessMemberAccessIsAccessible() throws Exception { // given - SecurityMemberAccess sma = new SecurityMemberAccess(false); + SecurityMemberAccess sma = new SecurityMemberAccess(false, true); Set> excluded = new HashSet<>(); excluded.add(ognl.MemberAccess.class); sma.setExcludedClasses(excluded); @@ -442,7 +528,7 @@ public class SecurityMemberAccessTest extends TestCase { public void testAccessMemberAccessIsBlocked() throws Exception { // given - SecurityMemberAccess sma = new SecurityMemberAccess(false); + SecurityMemberAccess sma = new SecurityMemberAccess(false, true); Set> excluded = new HashSet<>(); excluded.add(SecurityMemberAccess.class); sma.setExcludedClasses(excluded); @@ -460,7 +546,7 @@ public class SecurityMemberAccessTest extends TestCase { public void testPackageNameExclusionAsCommaDelimited() { // given - SecurityMemberAccess sma = new SecurityMemberAccess(false); + SecurityMemberAccess sma = new SecurityMemberAccess(false, true); sma.setExcludedPackageNames(TextParseUtil.commaDelimitedStringToSet("java.lang.")); @@ -544,9 +630,23 @@ enum MyValues { class StaticTester { public static int MAX_VALUE = 0; + public static final int MIN_VALUE = 0; + static String PACKAGE_STRING = "package_string"; + static final String FINAL_PACKAGE_STRING = "final_package_string"; + static String PROTECTED_STRING = "protected_string"; + static final String FINAL_PROTECTED_STRING = "final_protected_string"; + static String PRIVATE_STRING = "private_string"; + static final String FINAL_PRIVATE_STRING = "final_private_string"; public static String sayHello() { return "Hello"; } + protected static Field getFieldByName(String fieldName) throws NoSuchFieldException { + if (fieldName != null && fieldName.length() > 0) { + return StaticTester.class.getDeclaredField(fieldName); + } else { + throw new NoSuchFieldException("field: " + fieldName + " does not exist"); + } + } } diff --git a/core/src/test/java/com/opensymphony/xwork2/ognl/SetPropertiesTest.java b/core/src/test/java/com/opensymphony/xwork2/ognl/SetPropertiesTest.java index 84e4de658..3d7ed9778 100644 --- a/core/src/test/java/com/opensymphony/xwork2/ognl/SetPropertiesTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/ognl/SetPropertiesTest.java @@ -57,7 +57,7 @@ public class SetPropertiesTest extends XWorkTestCase { } public void testOgnlUtilEmptyStringAsLong() { Bar bar = new Bar(); - Map context = Ognl.createDefaultContext(bar, new SecurityMemberAccess(false)); + Map context = Ognl.createDefaultContext(bar, new SecurityMemberAccess(false, true)); context.put(XWorkConverter.REPORT_CONVERSION_ERRORS, Boolean.TRUE); bar.setId(null); diff --git a/core/src/test/java/org/apache/struts2/result/ServletRedirectResultTest.java b/core/src/test/java/org/apache/struts2/result/ServletRedirectResultTest.java index aa15c3e0b..caec815f3 100644 --- a/core/src/test/java/org/apache/struts2/result/ServletRedirectResultTest.java +++ b/core/src/test/java/org/apache/struts2/result/ServletRedirectResultTest.java @@ -343,7 +343,7 @@ public class ServletRedirectResultTest extends StrutsInternalTestCase implements ActionConfig actionConfig = new ActionConfig.Builder("", "", "") .addResultConfigs(results).build(); - ActionContext ac = new ActionContext(Ognl.createDefaultContext(null, new SecurityMemberAccess(false))); + ActionContext ac = new ActionContext(Ognl.createDefaultContext(null, new SecurityMemberAccess(false, true))); ac.put(ServletActionContext.HTTP_REQUEST, requestMock.proxy()); ac.put(ServletActionContext.HTTP_RESPONSE, responseMock.proxy()); MockActionInvocation ai = new MockActionInvocation(); diff --git a/core/src/test/java/org/apache/struts2/util/SecurityMemberAccessInServletsTest.java b/core/src/test/java/org/apache/struts2/util/SecurityMemberAccessInServletsTest.java index a7e256172..bf29a33fd 100644 --- a/core/src/test/java/org/apache/struts2/util/SecurityMemberAccessInServletsTest.java +++ b/core/src/test/java/org/apache/struts2/util/SecurityMemberAccessInServletsTest.java @@ -41,7 +41,7 @@ public class SecurityMemberAccessInServletsTest extends StrutsInternalTestCase { public void testJavaxServletPackageAccess() throws Exception { // given - SecurityMemberAccess sma = new SecurityMemberAccess(false); + SecurityMemberAccess sma = new SecurityMemberAccess(false, true); Set excluded = new HashSet(); excluded.add(Pattern.compile("^(?!javax\\.servlet\\..+)(javax\\..+)")); @@ -59,7 +59,7 @@ public class SecurityMemberAccessInServletsTest extends StrutsInternalTestCase { public void testJavaxServletPackageExclusion() throws Exception { // given - SecurityMemberAccess sma = new SecurityMemberAccess(false); + SecurityMemberAccess sma = new SecurityMemberAccess(false, true); Set excluded = new HashSet(); excluded.add(Pattern.compile("^javax\\..+")); diff --git a/core/src/test/resources/com/opensymphony/xwork2/config/providers/xwork-test-staticfield-false.xml b/core/src/test/resources/com/opensymphony/xwork2/config/providers/xwork-test-staticfield-false.xml new file mode 100644 index 000000000..c6aeb4ad5 --- /dev/null +++ b/core/src/test/resources/com/opensymphony/xwork2/config/providers/xwork-test-staticfield-false.xml @@ -0,0 +1,87 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + \ No newline at end of file diff --git a/core/src/test/resources/com/opensymphony/xwork2/config/providers/xwork-test-staticfield-true.xml b/core/src/test/resources/com/opensymphony/xwork2/config/providers/xwork-test-staticfield-true.xml new file mode 100644 index 000000000..8cde06537 --- /dev/null +++ b/core/src/test/resources/com/opensymphony/xwork2/config/providers/xwork-test-staticfield-true.xml @@ -0,0 +1,87 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + \ No newline at end of file From 64bd12b26da0916b088910c21e1852ef1f2ccaeb Mon Sep 17 00:00:00 2001 From: JCgH4164838Gh792C124B5 <43964333+JCgH4164838Gh792C124B5@users.noreply.github.com> Date: Wed, 30 Jan 2019 18:31:51 -0500 Subject: [PATCH 2/2] Fix for access issue for 2.6 discovered in WW-5004 (Minor update to previous commit): - Restored ability to access public static fields (true by default). - Introduced a boolean configuration flag (allowStaticFieldAccess). - Replaced one remaining Boolean.parseBoolean() conversion in OgnlUtil use BooleanUtils.toBoolean(). - Enhanced unit tests to confirm proper operation of the fix. - Replicating L. Lenart's change in PR#317: - Removed injection parameter for setAllowStaticMethodAccess in OgnlValueStackFactory. - Replaced with lazy retrieval of allowStaticMethodAccess from container. - Used same pattern for the new allowStaticFieldAccess flag. - Added retrieval methods for both flags from the container. - Optimized calling sequence of isAccessible() based on feedback from previous commit. - Made a couple of getters and the protected checkXXX methods final (avoid descendant interference). --- .../xwork2/ognl/SecurityMemberAccess.java | 47 +++++++++++++------ 1 file changed, 33 insertions(+), 14 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 9174185a4..fd36972a5 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java @@ -63,11 +63,11 @@ public class SecurityMemberAccess implements MemberAccess { this.allowStaticFieldAccess = allowStaticFieldAccess; } - public boolean getAllowStaticMethodAccess() { + public final boolean getAllowStaticMethodAccess() { return allowStaticMethodAccess; } - public boolean getAllowStaticFieldAccess() { + public final boolean getAllowStaticFieldAccess() { return allowStaticFieldAccess; } @@ -110,13 +110,18 @@ public class SecurityMemberAccess implements MemberAccess { return true; } - if (!checkStaticMemberAccess(member)) { + final int memberModifiers = member.getModifiers(); + if (!checkStaticMemberAccess(member, memberModifiers)) { LOG.warn("Access to static [{}] is blocked!", member); return false; } + if (!checkPublicMemberAccess(memberModifiers)) { + LOG.trace("Access to non-public [{}] is blocked!", member); + return false; + } + final Class memberClass = member.getDeclaringClass(); - final int memberModifiers = member.getModifiers(); if (isClassExcluded(memberClass)) { LOG.warn("Declaring class of member type [{}] is excluded!", member); @@ -142,37 +147,51 @@ public class SecurityMemberAccess implements MemberAccess { return false; } - return Modifier.isPublic(memberModifiers) && isAcceptableProperty(propertyName); + return isAcceptableProperty(propertyName); } /** - * Check access for static members + * Check access for static members (via modifiers) + * + * Static non-field access result is allowStaticMethodAccess. + * Static field access result is allowStaticFieldAccess. * - * Static non-field access result is a logical and of allowStaticMethodAccess and public. - * Static field access result is a logical and of allowStaticFieldAccess and public. * Note: For non-static members, the result is always true. * * @param member + * @param memberModifiers (minor optimization) * * @return */ - protected boolean checkStaticMemberAccess(Member member) { - final int modifiers = member.getModifiers(); - if (Modifier.isStatic(modifiers)) { + protected final boolean checkStaticMemberAccess(Member member, int memberModifiers) { + if (Modifier.isStatic(memberModifiers)) { if (member instanceof Field) { - return allowStaticFieldAccess && Modifier.isPublic(modifiers); + return allowStaticFieldAccess; } else { if (allowStaticMethodAccess) { LOG.debug("Support for accessing static methods [member: {}] is deprecated!", member); } - return allowStaticMethodAccess && Modifier.isPublic(modifiers); + return allowStaticMethodAccess; } } else { return true; } } - protected boolean checkEnumAccess(Object target, Member member) { + /** + * Check access for public members (via modifiers) + * + * Returns true if-and-only-if the member is public. + * + * @param memberModifiers + * + * @return + */ + protected final boolean checkPublicMemberAccess(int memberModifiers) { + return Modifier.isPublic(memberModifiers); + } + + protected final boolean checkEnumAccess(Object target, Member member) { if (target instanceof Class) { final Class clazz = (Class) target; if (Enum.class.isAssignableFrom(clazz) && member.getName().equals("values")) {