From db085dbeef3da43e7cfb427994ccebaf91becc16 Mon Sep 17 00:00:00 2001 From: JCgH4164838Gh792C124B5 <43964333+JCgH4164838Gh792C124B5@users.noreply.github.com> Date: Thu, 20 Dec 2018 06:55:34 -0500 Subject: [PATCH] Updates and cleanup for some configuration-related items (2.5.x). (#291) * Updates and cleanup for some configuration-related items. - Made several attributes final. - Added limiter logic to the setting of certain configuration elements, and increased logging around them. - Updated unit tests impacted by the limiter logic, including minor typo fixes and locale consistency. - Minor cleanup while making the changes. * Update core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStackFactory.java Whitespace cleanup Co-Authored-By: JCgH4164838Gh792C124B5 <43964333+JCgH4164838Gh792C124B5@users.noreply.github.com> * Updates and cleanup for some configuration-related items (1st revision). - Reworked this PR, taking into account feedback and suggestions. - Removed the if LOG.isxxxEnabled level checks that were added. - Removed the limiter logic and instead reduced access level of most of the injectable methods to protected. - Updated the previous unit test changes following removal of limiter logic. * Minor correction to two debug log statements (as requested by reviewer). * Update debug messages for pattern checkers, as requested by reviewers. Avoids confusing users when replacing an empty exclusion set (which improves safety). Still provides a (slightly milder) message when replacing an empty accepted set (which can reduce safety). --- .../providers/XWorkConfigurationProvider.java | 1 + .../xwork2/ognl/ErrorMessageBuilder.java | 2 +- .../xwork2/ognl/OgnlNullHandlerWrapper.java | 2 +- .../xwork2/ognl/OgnlTypeConverterWrapper.java | 2 +- .../opensymphony/xwork2/ognl/OgnlUtil.java | 48 +++-- .../xwork2/ognl/OgnlValueStack.java | 14 +- .../xwork2/ognl/OgnlValueStackFactory.java | 18 +- .../ognl/XWorkTypeConverterWrapper.java | 2 +- .../ognl/accessor/CompoundRootAccessor.java | 8 +- .../XWorkCollectionPropertyAccessor.java | 2 +- .../accessor/XWorkEnumerationAccessor.java | 2 +- .../XWorkIteratorPropertyAccessor.java | 2 +- .../DefaultAcceptedPatternsChecker.java | 15 +- .../DefaultExcludedPatternsChecker.java | 28 ++- core/src/main/resources/struts-default.xml | 1 + .../xwork2/ognl/OgnlUtilTest.java | 171 +++++++++++++++++- .../xwork2/ognl/OgnlValueStackTest.java | 41 ++++- .../xwork-test-allowstatic-devmode-false.xml | 88 +++++++++ .../xwork-test-allowstatic-devmode-true.xml | 88 +++++++++ .../providers/xwork-test-allowstatic-true.xml | 88 +++++++++ .../providers/xwork-test-devmode-true.xml | 88 +++++++++ 21 files changed, 657 insertions(+), 54 deletions(-) create mode 100644 core/src/test/resources/com/opensymphony/xwork2/config/providers/xwork-test-allowstatic-devmode-false.xml create mode 100644 core/src/test/resources/com/opensymphony/xwork2/config/providers/xwork-test-allowstatic-devmode-true.xml create mode 100644 core/src/test/resources/com/opensymphony/xwork2/config/providers/xwork-test-allowstatic-true.xml create mode 100644 core/src/test/resources/com/opensymphony/xwork2/config/providers/xwork-test-devmode-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 94c766cac..cd27ed374 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 @@ -226,6 +226,7 @@ public class XWorkConfigurationProvider implements ConfigurationProvider { props.setProperty(XWorkConstants.ENABLE_OGNL_EXPRESSION_CACHE, Boolean.TRUE.toString()); props.setProperty(XWorkConstants.ENABLE_OGNL_EVAL_EXPRESSION, Boolean.FALSE.toString()); props.setProperty(XWorkConstants.RELOAD_XML_CONFIGURATION, Boolean.FALSE.toString()); + props.setProperty(StrutsConstants.STRUTS_ALLOW_STATIC_METHOD_ACCESS, Boolean.FALSE.toString()); } } diff --git a/core/src/main/java/com/opensymphony/xwork2/ognl/ErrorMessageBuilder.java b/core/src/main/java/com/opensymphony/xwork2/ognl/ErrorMessageBuilder.java index 561f2bce5..b904f4ba1 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/ErrorMessageBuilder.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/ErrorMessageBuilder.java @@ -23,7 +23,7 @@ package com.opensymphony.xwork2.ognl; */ public class ErrorMessageBuilder { - private StringBuilder message = new StringBuilder(); + private final StringBuilder message = new StringBuilder(); public static ErrorMessageBuilder create() { return new ErrorMessageBuilder(); diff --git a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlNullHandlerWrapper.java b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlNullHandlerWrapper.java index 2f0a31dff..9b24261c8 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlNullHandlerWrapper.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlNullHandlerWrapper.java @@ -24,7 +24,7 @@ import java.util.Map; public class OgnlNullHandlerWrapper implements ognl.NullHandler { - private NullHandler wrapped; + private final NullHandler wrapped; public OgnlNullHandlerWrapper(NullHandler target) { this.wrapped = target; diff --git a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlTypeConverterWrapper.java b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlTypeConverterWrapper.java index fc87e113a..c2783b969 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlTypeConverterWrapper.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlTypeConverterWrapper.java @@ -28,7 +28,7 @@ import java.util.Map; */ public class OgnlTypeConverterWrapper implements ognl.TypeConverter { - private TypeConverter typeConverter; + private final TypeConverter typeConverter; public OgnlTypeConverterWrapper(TypeConverter converter) { if (converter == null) { 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 13691d571..08b2d862a 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java @@ -58,7 +58,7 @@ public class OgnlUtil { private final ConcurrentMap beanInfoCache = new ConcurrentHashMap<>(); private TypeConverter defaultConverter; - private boolean devMode = false; + private boolean devMode; private boolean enableExpressionCache = true; private boolean enableEvalExpression; @@ -77,31 +77,35 @@ public class OgnlUtil { } @Inject - public void setXWorkConverter(XWorkConverter conv) { + protected void setXWorkConverter(XWorkConverter conv) { this.defaultConverter = new OgnlTypeConverterWrapper(conv); } @Inject(XWorkConstants.DEV_MODE) - public void setDevMode(String mode) { + protected void setDevMode(String mode) { this.devMode = BooleanUtils.toBoolean(mode); + if (this.devMode) { + LOG.warn("Setting development mode [{}] affects the safety of your application!", + this.devMode); + } } @Inject(XWorkConstants.ENABLE_OGNL_EXPRESSION_CACHE) - public void setEnableExpressionCache(String cache) { + protected void setEnableExpressionCache(String cache) { enableExpressionCache = BooleanUtils.toBoolean(cache); } @Inject(value = XWorkConstants.ENABLE_OGNL_EVAL_EXPRESSION, required = false) - public void setEnableEvalExpression(String evalExpression) { - enableEvalExpression = "true".equals(evalExpression); - if(enableEvalExpression){ + protected void setEnableEvalExpression(String evalExpression) { + this.enableEvalExpression = BooleanUtils.toBoolean(evalExpression); + if (this.enableEvalExpression) { LOG.warn("Enabling OGNL expression evaluation may introduce security risks " + "(see http://struts.apache.org/release/2.3.x/docs/s2-013.html for further details)"); } } @Inject(value = XWorkConstants.OGNL_EXCLUDED_CLASSES, required = false) - public void setExcludedClasses(String commaDelimitedClasses) { + protected void setExcludedClasses(String commaDelimitedClasses) { Set> excludedClasses = new HashSet<>(); excludedClasses.addAll(this.excludedClasses); excludedClasses.addAll(parseExcludedClasses(commaDelimitedClasses)); @@ -124,7 +128,7 @@ public class OgnlUtil { } @Inject(value = XWorkConstants.OGNL_EXCLUDED_PACKAGE_NAME_PATTERNS, required = false) - public void setExcludedPackageNamePatterns(String commaDelimitedPackagePatterns) { + protected void setExcludedPackageNamePatterns(String commaDelimitedPackagePatterns) { Set excludedPackageNamePatterns = new HashSet<>(); excludedPackageNamePatterns.addAll(this.excludedPackageNamePatterns); excludedPackageNamePatterns.addAll(parseExcludedPackageNamePatterns(commaDelimitedPackagePatterns)); @@ -143,7 +147,7 @@ public class OgnlUtil { } @Inject(value = XWorkConstants.OGNL_EXCLUDED_PACKAGE_NAMES, required = false) - public void setExcludedPackageNames(String commaDelimitedPackageNames) { + protected void setExcludedPackageNames(String commaDelimitedPackageNames) { Set excludedPackageNames = new HashSet<>(); excludedPackageNames.addAll(this.excludedPackageNames); excludedPackageNames.addAll(parseExcludedPackageNames(commaDelimitedPackageNames)); @@ -167,18 +171,27 @@ public class OgnlUtil { } @Inject - public void setContainer(Container container) { + protected void setContainer(Container container) { this.container = container; } @Inject(value = XWorkConstants.ALLOW_STATIC_METHOD_ACCESS, required = false) - public void setAllowStaticMethodAccess(String allowStaticMethodAccess) { - this.allowStaticMethodAccess = Boolean.parseBoolean(allowStaticMethodAccess); + protected void setAllowStaticMethodAccess(String allowStaticMethodAccess) { + this.allowStaticMethodAccess = BooleanUtils.toBoolean(allowStaticMethodAccess); + if (this.allowStaticMethodAccess) { + LOG.warn("Setting allow static method access [{}] affects the safety of your application!", + this.allowStaticMethodAccess); + } } @Inject(value = StrutsConstants.STRUTS_DISALLOW_PROXY_MEMBER_ACCESS, required = false) - public void setDisallowProxyMemberAccess(String disallowProxyMemberAccess) { + protected void setDisallowProxyMemberAccess(String disallowProxyMemberAccess) { + this.disallowProxyMemberAccess = Boolean.parseBoolean(disallowProxyMemberAccess); + if (this.disallowProxyMemberAccess == false) { + LOG.warn("Setting disallow proxy member access [{}] should only be done intentionally!", + this.disallowProxyMemberAccess); + } } public boolean isDisallowProxyMemberAccess() { @@ -431,7 +444,7 @@ public class OgnlUtil { final T exec = task.execute(tree); // if cache is enabled and it's a valid expression, puts it in - if(enableExpressionCache) { + if (enableExpressionCache) { expressions.putIfAbsent(expression, tree); } return exec; @@ -452,7 +465,7 @@ public class OgnlUtil { final T exec = task.execute(tree); // if cache is enabled and it's a valid expression, puts it in - if(enableExpressionCache) { + if (enableExpressionCache) { expressions.putIfAbsent(expression, tree); } return exec; @@ -668,8 +681,7 @@ public class OgnlUtil { */ public BeanInfo getBeanInfo(Class clazz) throws IntrospectionException { synchronized (beanInfoCache) { - BeanInfo beanInfo; - beanInfo = beanInfoCache.get(clazz); + BeanInfo beanInfo = beanInfoCache.get(clazz); if (beanInfo == null) { beanInfo = Introspector.getBeanInfo(clazz, Object.class); beanInfoCache.putIfAbsent(clazz, beanInfo); 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 ab580691b..01a98bacb 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java @@ -82,7 +82,7 @@ public class OgnlValueStack implements Serializable, ValueStack, ClearableValueS } @Inject - public void setOgnlUtil(OgnlUtil ognlUtil) { + protected void setOgnlUtil(OgnlUtil ognlUtil) { this.ognlUtil = ognlUtil; securityMemberAccess.setExcludedClasses(ognlUtil.getExcludedClasses()); securityMemberAccess.setExcludedPackageNamePatterns(ognlUtil.getExcludedPackageNamePatterns()); @@ -102,12 +102,16 @@ public class OgnlValueStack implements Serializable, ValueStack, ClearableValueS } @Inject(XWorkConstants.DEV_MODE) - public void setDevMode(String mode) { + protected void setDevMode(String mode) { this.devMode = BooleanUtils.toBoolean(mode); + if (this.devMode) { + LOG.warn("Setting development mode [{}] affects the safety of your application!", + this.devMode); + } } @Inject(value = "logMissingProperties", required = false) - public void setLogMissingProperties(String logMissingProperties) { + protected void setLogMissingProperties(String logMissingProperties) { this.logMissingProperties = BooleanUtils.toBoolean(logMissingProperties); } @@ -379,7 +383,7 @@ public class OgnlValueStack implements Serializable, ValueStack, ClearableValueS * @param e The thrown exception. */ private void logLookupFailure(String expr, Exception e) { - if (devMode && LOG.isWarnEnabled()) { + if (devMode) { LOG.warn("Caught an exception while evaluating expression '{}' against value stack", expr, e); LOG.warn("NOTE: Previous warning message was issued due to devMode set to true."); } else { @@ -475,7 +479,7 @@ public class OgnlValueStack implements Serializable, ValueStack, ClearableValueS } @Inject - public void setXWorkConverter(final XWorkConverter converter) { + protected void setXWorkConverter(final XWorkConverter converter) { this.converter = converter; } } 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 6b46635ef..36ca51dcb 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStackFactory.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStackFactory.java @@ -32,6 +32,8 @@ import ognl.MethodAccessor; import ognl.OgnlRuntime; import ognl.PropertyAccessor; import org.apache.commons.lang3.BooleanUtils; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; import java.util.Map; import java.util.Set; @@ -41,25 +43,31 @@ import java.util.Set; */ public class OgnlValueStackFactory implements ValueStackFactory { + private static final Logger LOG = LogManager.getLogger(OgnlValueStackFactory.class); + protected XWorkConverter xworkConverter; protected CompoundRootAccessor compoundRootAccessor; protected TextProvider textProvider; protected Container container; - protected boolean allowStaticMethodAccess; + private boolean allowStaticMethodAccess; @Inject - public void setXWorkConverter(XWorkConverter converter) { + protected void setXWorkConverter(XWorkConverter converter) { this.xworkConverter = converter; } @Inject("system") - public void setTextProvider(TextProvider textProvider) { + protected void setTextProvider(TextProvider textProvider) { this.textProvider = textProvider; } @Inject(value="allowStaticMethodAccess", required=false) - public void setAllowStaticMethodAccess(String allowStaticMethodAccess) { + protected void setAllowStaticMethodAccess(String allowStaticMethodAccess) { this.allowStaticMethodAccess = BooleanUtils.toBoolean(allowStaticMethodAccess); + if (this.allowStaticMethodAccess) { + LOG.warn("Setting allow static method access [{}] affects the safety of your application!", + this.allowStaticMethodAccess); + } } public ValueStack createValueStack() { @@ -77,7 +85,7 @@ public class OgnlValueStackFactory implements ValueStackFactory { } @Inject - public void setContainer(Container container) throws ClassNotFoundException { + protected void setContainer(Container container) throws ClassNotFoundException { Set names = container.getInstanceNames(PropertyAccessor.class); for (String name : names) { Class cls = Class.forName(name); diff --git a/core/src/main/java/com/opensymphony/xwork2/ognl/XWorkTypeConverterWrapper.java b/core/src/main/java/com/opensymphony/xwork2/ognl/XWorkTypeConverterWrapper.java index 0ea356a4f..18a2c8e44 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/XWorkTypeConverterWrapper.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/XWorkTypeConverterWrapper.java @@ -28,7 +28,7 @@ import java.util.Map; */ public class XWorkTypeConverterWrapper implements TypeConverter { - private ognl.TypeConverter typeConverter; + private final ognl.TypeConverter typeConverter; public XWorkTypeConverterWrapper(ognl.TypeConverter conv) { this.typeConverter = conv; diff --git a/core/src/main/java/com/opensymphony/xwork2/ognl/accessor/CompoundRootAccessor.java b/core/src/main/java/com/opensymphony/xwork2/ognl/accessor/CompoundRootAccessor.java index 0bccc60fa..be0ea8d60 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/accessor/CompoundRootAccessor.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/accessor/CompoundRootAccessor.java @@ -63,11 +63,15 @@ public class CompoundRootAccessor implements PropertyAccessor, MethodAccessor, C private final static Logger LOG = LogManager.getLogger(CompoundRootAccessor.class); private final static Class[] EMPTY_CLASS_ARRAY = new Class[0]; private static Map invalidMethods = new ConcurrentHashMap<>(); - private boolean devMode = false; + private boolean devMode; @Inject(XWorkConstants.DEV_MODE) - public void setDevMode(String mode) { + protected void setDevMode(String mode) { this.devMode = BooleanUtils.toBoolean(mode); + if (this.devMode) { + LOG.warn("Setting development mode [{}] affects the safety of your application!", + this.devMode); + } } public void setProperty(Map context, Object target, Object name, Object value) throws OgnlException { diff --git a/core/src/main/java/com/opensymphony/xwork2/ognl/accessor/XWorkCollectionPropertyAccessor.java b/core/src/main/java/com/opensymphony/xwork2/ognl/accessor/XWorkCollectionPropertyAccessor.java index be2d136f8..837558f59 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/accessor/XWorkCollectionPropertyAccessor.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/accessor/XWorkCollectionPropertyAccessor.java @@ -48,7 +48,7 @@ public class XWorkCollectionPropertyAccessor extends SetPropertyAccessor { //use a basic object Ognl property accessor here //to access properties of the objects in the Set //so that nothing is put in the context to screw things up - private ObjectPropertyAccessor _accessor = new ObjectPropertyAccessor(); + private final ObjectPropertyAccessor _accessor = new ObjectPropertyAccessor(); private XWorkConverter xworkConverter; private ObjectFactory objectFactory; diff --git a/core/src/main/java/com/opensymphony/xwork2/ognl/accessor/XWorkEnumerationAccessor.java b/core/src/main/java/com/opensymphony/xwork2/ognl/accessor/XWorkEnumerationAccessor.java index 3e6f576bd..d0d713aa3 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/accessor/XWorkEnumerationAccessor.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/accessor/XWorkEnumerationAccessor.java @@ -30,7 +30,7 @@ import java.util.Map; */ public class XWorkEnumerationAccessor extends EnumerationPropertyAccessor { - ObjectPropertyAccessor opa = new ObjectPropertyAccessor(); + private final ObjectPropertyAccessor opa = new ObjectPropertyAccessor(); @Override public void setProperty(Map context, Object target, Object name, Object value) throws OgnlException { diff --git a/core/src/main/java/com/opensymphony/xwork2/ognl/accessor/XWorkIteratorPropertyAccessor.java b/core/src/main/java/com/opensymphony/xwork2/ognl/accessor/XWorkIteratorPropertyAccessor.java index 73ad710a3..89796080c 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/accessor/XWorkIteratorPropertyAccessor.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/accessor/XWorkIteratorPropertyAccessor.java @@ -30,7 +30,7 @@ import java.util.Map; */ public class XWorkIteratorPropertyAccessor extends IteratorPropertyAccessor { - ObjectPropertyAccessor opa = new ObjectPropertyAccessor(); + private final ObjectPropertyAccessor opa = new ObjectPropertyAccessor(); @Override public void setProperty(Map context, Object target, Object name, Object value) throws OgnlException { diff --git a/core/src/main/java/com/opensymphony/xwork2/security/DefaultAcceptedPatternsChecker.java b/core/src/main/java/com/opensymphony/xwork2/security/DefaultAcceptedPatternsChecker.java index 8dfd6ef3f..ce7ba344f 100644 --- a/core/src/main/java/com/opensymphony/xwork2/security/DefaultAcceptedPatternsChecker.java +++ b/core/src/main/java/com/opensymphony/xwork2/security/DefaultAcceptedPatternsChecker.java @@ -44,9 +44,9 @@ public class DefaultAcceptedPatternsChecker implements AcceptedPatternsChecker { } @Inject(value = XWorkConstants.OVERRIDE_ACCEPTED_PATTERNS, required = false) - public void setOverrideAcceptedPatterns(String acceptablePatterns) { + protected void setOverrideAcceptedPatterns(String acceptablePatterns) { LOG.warn("Overriding accepted patterns [{}] with [{}], be aware that this affects all instances and safety of your application!", - XWorkConstants.OVERRIDE_ACCEPTED_PATTERNS, acceptablePatterns); + acceptedPatterns, acceptablePatterns); acceptedPatterns = new HashSet<>(); for (String pattern : TextParseUtil.commaDelimitedStringToSet(acceptablePatterns)) { acceptedPatterns.add(Pattern.compile(pattern, Pattern.CASE_INSENSITIVE)); @@ -54,7 +54,7 @@ public class DefaultAcceptedPatternsChecker implements AcceptedPatternsChecker { } @Inject(value = XWorkConstants.ADDITIONAL_ACCEPTED_PATTERNS, required = false) - public void setAdditionalAcceptedPatterns(String acceptablePatterns) { + protected void setAdditionalAcceptedPatterns(String acceptablePatterns) { LOG.warn("Adding additional global patterns [{}] to accepted patterns!", acceptablePatterns); for (String pattern : TextParseUtil.commaDelimitedStringToSet(acceptablePatterns)) { acceptedPatterns.add(Pattern.compile(pattern, Pattern.CASE_INSENSITIVE)); @@ -70,7 +70,14 @@ public class DefaultAcceptedPatternsChecker implements AcceptedPatternsChecker { } public void setAcceptedPatterns(Set patterns) { - LOG.trace("Sets accepted patterns [{}]", patterns); + if (acceptedPatterns == null) { + // Limit unwanted log entries (for 1st call, acceptedPatterns null) + LOG.debug("Sets accepted patterns to [{}], note this impacts the safety of your application!", patterns); + } + else { + LOG.warn("Replacing accepted patterns [{}] with [{}], be aware that this affects all instances and safety of your application!", + acceptedPatterns, patterns); + } acceptedPatterns = new HashSet<>(patterns.size()); for (String pattern : patterns) { acceptedPatterns.add(Pattern.compile(pattern, Pattern.CASE_INSENSITIVE)); diff --git a/core/src/main/java/com/opensymphony/xwork2/security/DefaultExcludedPatternsChecker.java b/core/src/main/java/com/opensymphony/xwork2/security/DefaultExcludedPatternsChecker.java index 0148a35cf..e93683ab2 100644 --- a/core/src/main/java/com/opensymphony/xwork2/security/DefaultExcludedPatternsChecker.java +++ b/core/src/main/java/com/opensymphony/xwork2/security/DefaultExcludedPatternsChecker.java @@ -47,9 +47,15 @@ public class DefaultExcludedPatternsChecker implements ExcludedPatternsChecker { } @Inject(value = XWorkConstants.OVERRIDE_EXCLUDED_PATTERNS, required = false) - public void setOverrideExcludePatterns(String excludePatterns) { - LOG.warn("Overriding excluded patterns [{}] with [{}], be aware that this affects all instances and safety of your application!", - XWorkConstants.OVERRIDE_EXCLUDED_PATTERNS, excludePatterns); + protected void setOverrideExcludePatterns(String excludePatterns) { + if (excludedPatterns != null && excludedPatterns.size() > 0) { + LOG.warn("Overriding excluded patterns [{}] with [{}], be aware that this affects all instances and safety of your application!", + excludedPatterns, excludePatterns); + } + else { + // Limit unwanted log entries (when excludedPatterns null/empty - usually 1st call) + LOG.debug("Overriding excluded patterns with [{}]", excludePatterns); + } excludedPatterns = new HashSet(); for (String pattern : TextParseUtil.commaDelimitedStringToSet(excludePatterns)) { excludedPatterns.add(Pattern.compile(pattern, Pattern.CASE_INSENSITIVE)); @@ -65,11 +71,14 @@ public class DefaultExcludedPatternsChecker implements ExcludedPatternsChecker { } @Inject(StrutsConstants.STRUTS_ENABLE_DYNAMIC_METHOD_INVOCATION) - public void setDynamicMethodInvocation(String dmiValue) { - if (!BooleanUtils.toBoolean(dmiValue)) { + protected void setDynamicMethodInvocation(String dmiValue) { + if (BooleanUtils.toBoolean(dmiValue) == false) { LOG.debug("DMI is disabled, adding DMI related excluded patterns"); setAdditionalExcludePatterns("^(action|method):.*"); } + else { + LOG.warn("DMI is enabled, *NOT* adding DMI related excluded patterns"); + } } public void setExcludedPatterns(String commaDelimitedPatterns) { @@ -81,7 +90,14 @@ public class DefaultExcludedPatternsChecker implements ExcludedPatternsChecker { } public void setExcludedPatterns(Set patterns) { - LOG.trace("Sets excluded patterns [{}]", patterns); + if (excludedPatterns != null && excludedPatterns.size() > 0) { + LOG.warn("Replacing excluded patterns [{}] with [{}], be aware that this affects all instances and safety of your application!", + excludedPatterns, patterns); + } + else { + // Limit unwanted log entries (when excludedPatterns null/empty - usually 1st call) + LOG.debug("Sets excluded patterns to [{}]", patterns); + } excludedPatterns = new HashSet<>(patterns.size()); for (String pattern : patterns) { excludedPatterns.add(Pattern.compile(pattern, Pattern.CASE_INSENSITIVE)); diff --git a/core/src/main/resources/struts-default.xml b/core/src/main/resources/struts-default.xml index fda6cfbf3..e85bc6aa0 100644 --- a/core/src/main/resources/struts-default.xml +++ b/core/src/main/resources/struts-default.xml @@ -61,6 +61,7 @@ freemarker.ext.rhino., sun.reflect., javassist., + org.objectweb.asm., com.opensymphony.xwork2.ognl., com.opensymphony.xwork2.security., com.opensymphony.xwork2.util." /> 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 28d0d59e5..225ca2fad 100644 --- a/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java @@ -19,9 +19,14 @@ package com.opensymphony.xwork2.ognl; import com.opensymphony.xwork2.ActionContext; +import com.opensymphony.xwork2.ActionProxyFactory; import com.opensymphony.xwork2.XWorkException; import com.opensymphony.xwork2.XWorkTestCase; +import com.opensymphony.xwork2.config.ConfigurationManager; +import com.opensymphony.xwork2.config.ConfigurationProvider; +import com.opensymphony.xwork2.config.providers.XmlConfigurationProvider; import com.opensymphony.xwork2.conversion.impl.XWorkConverter; +import com.opensymphony.xwork2.inject.Container; import com.opensymphony.xwork2.interceptor.ChainingInterceptor; import com.opensymphony.xwork2.test.User; import com.opensymphony.xwork2.util.*; @@ -364,9 +369,10 @@ public class OgnlUtilTest extends XWorkTestCase { Map props = new HashMap(); props.put("birthday", "02/12/1982"); // US style test + context.put(ActionContext.LOCALE, Locale.US); ognlUtil.setProperties(props, foo, context); - Calendar cal = Calendar.getInstance(); + Calendar cal = Calendar.getInstance(Locale.US); cal.clear(); cal.set(Calendar.MONTH, Calendar.FEBRUARY); cal.set(Calendar.DAY_OF_MONTH, 12); @@ -375,7 +381,7 @@ public class OgnlUtilTest extends XWorkTestCase { assertEquals(cal.getTime(), foo.getBirthday()); //UK style test - cal = Calendar.getInstance(); + cal = Calendar.getInstance(Locale.UK); cal.clear(); cal.set(Calendar.MONTH, Calendar.OCTOBER); cal.set(Calendar.DAY_OF_MONTH, 18); @@ -389,7 +395,7 @@ public class OgnlUtilTest extends XWorkTestCase { .format(eventTime); props.put("event", formatted); - cal = Calendar.getInstance(); + cal = Calendar.getInstance(Locale.UK); cal.clear(); cal.set(Calendar.MONTH, Calendar.SEPTEMBER); cal.set(Calendar.DAY_OF_MONTH, 9); @@ -412,9 +418,10 @@ public class OgnlUtilTest extends XWorkTestCase { //test RFC 3339 date format for JSON props.put("event", "1996-12-19T16:39:57Z"); + context.put(ActionContext.LOCALE, Locale.US); ognlUtil.setProperties(props, foo, context); - cal = Calendar.getInstance(); + cal = Calendar.getInstance(Locale.US); cal.clear(); cal.set(Calendar.MONTH, Calendar.DECEMBER); cal.set(Calendar.DAY_OF_MONTH, 19); @@ -427,6 +434,7 @@ public class OgnlUtilTest extends XWorkTestCase { //test setting a calendar property props.put("calendar", "1996-12-19T16:39:57Z"); + context.put(ActionContext.LOCALE, Locale.US); ognlUtil.setProperties(props, foo, context); assertEquals(cal, foo.getCalendar()); } @@ -645,7 +653,7 @@ public class OgnlUtilTest extends XWorkTestCase { try { stack.setValue("1234", foo); - fail("non-valid expression: 1114778947765"); + fail("non-valid expression: 1234"); } catch(RuntimeException ex) { ; @@ -657,6 +665,126 @@ public class OgnlUtilTest extends XWorkTestCase { stack.setValue("1234", foo); } + public void testStackValueDevModeChange() throws Exception { + + try { + reloadTestContainerConfiguration(false, false); // Set dev mode false + } + catch (Exception ex) { + fail("Unable to reload container configuration - exception: " + ex); + } + + ValueStack stack = ActionContext.getContext().getValueStack(); + Map stackContext = stack.getContext(); + stackContext.put(ReflectionContextState.CREATE_NULL_OBJECTS, Boolean.FALSE); + stackContext.put(ReflectionContextState.DENY_METHOD_EXECUTION, Boolean.TRUE); + stackContext.put(XWorkConverter.REPORT_CONVERSION_ERRORS, Boolean.TRUE); + + String[] foo = new String[]{"asdf"}; + + // With dev mode false, the following set values should not cause failures + stack.setValue("list.1114778947765", foo); + stack.setValue("1114778947765", foo); + stack.setValue("1234", foo); + + try { + reloadTestContainerConfiguration(true, false); // Set dev mode true + } + catch (Exception ex) { + fail("Unable to reload container configuration - exception: " + ex); + } + + // Repeat stack/context set after retrieving updated stack + stack = ActionContext.getContext().getValueStack(); + stackContext = stack.getContext(); + stackContext.put(ReflectionContextState.CREATE_NULL_OBJECTS, Boolean.FALSE); + stackContext.put(ReflectionContextState.DENY_METHOD_EXECUTION, Boolean.TRUE); + stackContext.put(XWorkConverter.REPORT_CONVERSION_ERRORS, Boolean.TRUE); + + try { + stack.setValue("list.1114778947765", foo); + fail("non-valid expression: list.1114778947765"); + } + catch(RuntimeException ex) { + // Expected with dev mode true + } + try { + stack.setValue("1114778947765", foo); + fail("non-valid expression: 1114778947765"); + } + catch(RuntimeException ex) { + // Expected with dev mode true + } + try { + stack.setValue("1234", foo); + fail("non-valid expression: 1234"); + } + catch(RuntimeException ex) { + // Expected with dev mode true + } + + } + + public void testDevModeChange() throws Exception { + + try { + reloadTestContainerConfiguration(false, false); // Set dev mode false + } + catch (Exception ex) { + fail("Unable to reload container configuration - exception: " + ex); + } + + ValueStack stack = ActionContext.getContext().getValueStack(); + Map stackContext = stack.getContext(); + stackContext.put(ReflectionContextState.CREATE_NULL_OBJECTS, Boolean.FALSE); + stackContext.put(ReflectionContextState.DENY_METHOD_EXECUTION, Boolean.TRUE); + stackContext.put(XWorkConverter.REPORT_CONVERSION_ERRORS, Boolean.TRUE); + + String[] foo = new String[]{"asdf"}; + + // With dev mode false, the following set values should not cause failures + stack.setValue("list.1114778947765", foo); + stack.setValue("1114778947765", foo); + stack.setValue("1234", foo); + + try { + reloadTestContainerConfiguration(true, false); // Set dev mode true + } + catch (Exception ex) { + fail("Unable to reload container configuration - exception: " + ex); + } + + // Repeat stack/context set after retrieving updated stack + stack = ActionContext.getContext().getValueStack(); + stackContext = stack.getContext(); + stackContext.put(ReflectionContextState.CREATE_NULL_OBJECTS, Boolean.FALSE); + stackContext.put(ReflectionContextState.DENY_METHOD_EXECUTION, Boolean.TRUE); + stackContext.put(XWorkConverter.REPORT_CONVERSION_ERRORS, Boolean.TRUE); + + try { + stack.setValue("list.1114778947765", foo); + fail("non-valid expression: list.1114778947765"); + } + catch(RuntimeException ex) { + // Expected with dev mode true + } + try { + stack.setValue("1114778947765", foo); + fail("non-valid expression: 1114778947765"); + } + catch(RuntimeException ex) { + // Expected with dev mode true + } + try { + stack.setValue("1234", foo); + fail("non-valid expression: 1234"); + } + catch(RuntimeException ex) { + // Expected with dev mode true + } + + } + public void testAvoidCallingMethodsOnObjectClass() throws Exception { Foo foo = new Foo(); @@ -812,6 +940,39 @@ public class OgnlUtilTest extends XWorkTestCase { assertEquals(expected.getMessage(), "It isn't a simple method which can be called!"); } + private void reloadTestContainerConfiguration(boolean devMode, boolean allowStatic) throws Exception { + super.tearDown(); + + ConfigurationProvider configurationProvider; + if (devMode == true && allowStatic == true) { + configurationProvider = new XmlConfigurationProvider("com/opensymphony/xwork2/config/providers/xwork-test-allowstatic-devmode-true.xml", true); + } + else if (devMode == true && allowStatic == false) { + configurationProvider = new XmlConfigurationProvider("com/opensymphony/xwork2/config/providers/xwork-test-devmode-true.xml", true); + } + else if (devMode == false && allowStatic == true) { + configurationProvider = new XmlConfigurationProvider("com/opensymphony/xwork2/config/providers/xwork-test-allowstatic-true.xml", true); + } + else { // devMode, allowStatic both false + configurationProvider = new XmlConfigurationProvider("com/opensymphony/xwork2/config/providers/xwork-test-allowstatic-devmode-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 012122085..e66fc2466 100644 --- a/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java @@ -19,7 +19,11 @@ package com.opensymphony.xwork2.ognl; import com.opensymphony.xwork2.*; +import com.opensymphony.xwork2.config.ConfigurationManager; +import com.opensymphony.xwork2.config.ConfigurationProvider; +import com.opensymphony.xwork2.config.providers.XmlConfigurationProvider; import com.opensymphony.xwork2.conversion.impl.XWorkConverter; +import com.opensymphony.xwork2.inject.Container; import com.opensymphony.xwork2.ognl.accessor.CompoundRootAccessor; import com.opensymphony.xwork2.test.TestBean2; import com.opensymphony.xwork2.util.*; @@ -594,7 +598,7 @@ public class OgnlValueStackTest extends XWorkTestCase { assertEquals("bar:123", vs.findValue("foo.bar", String.class)); } - public void testPrimitiveSettingWithInvalidValueAddsFieldErrorInDevMode() { + public void testPrimitiveSettingWithInvalidValueAddsFieldErrorInDevMode() throws Exception { SimpleAction action = new SimpleAction(); OgnlValueStack stack = createValueStack(); stack.getContext().put(XWorkConverter.REPORT_CONVERSION_ERRORS, Boolean.TRUE); @@ -613,7 +617,7 @@ public class OgnlValueStackTest extends XWorkTestCase { assertTrue(conversionErrors.containsKey("bar")); } - public void testPrimitiveSettingWithInvalidValueAddsFieldErrorInNonDevMode() { + public void testPrimitiveSettingWithInvalidValueAddsFieldErrorInNonDevMode() throws Exception { SimpleAction action = new SimpleAction(); OgnlValueStack stack = createValueStack(); stack.getContext().put(XWorkConverter.REPORT_CONVERSION_ERRORS, Boolean.TRUE); @@ -968,6 +972,39 @@ public class OgnlValueStackTest extends XWorkTestCase { assertEquals(null, stack.findValue("address.country.name", String.class)); } + private void reloadTestContainerConfiguration(boolean devMode, boolean allowStatic) throws Exception { + super.tearDown(); + + ConfigurationProvider configurationProvider; + if (devMode == true && allowStatic == true) { + configurationProvider = new XmlConfigurationProvider("com/opensymphony/xwork2/config/providers/xwork-test-allowstatic-devmode-true.xml", true); + } + else if (devMode == true && allowStatic == false) { + configurationProvider = new XmlConfigurationProvider("com/opensymphony/xwork2/config/providers/xwork-test-devmode-true.xml", true); + } + else if (devMode == false && allowStatic == true) { + configurationProvider = new XmlConfigurationProvider("com/opensymphony/xwork2/config/providers/xwork-test-allowstatic-true.xml", true); + } + else { // devMode, allowStatic both false + configurationProvider = new XmlConfigurationProvider("com/opensymphony/xwork2/config/providers/xwork-test-allowstatic-devmode-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); + } + class BadJavaBean { private int count; private int count2; 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 new file mode 100644 index 000000000..132870dd9 --- /dev/null +++ b/core/src/test/resources/com/opensymphony/xwork2/config/providers/xwork-test-allowstatic-devmode-false.xml @@ -0,0 +1,88 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + \ No newline at end of file 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 new file mode 100644 index 000000000..3065a1ade --- /dev/null +++ b/core/src/test/resources/com/opensymphony/xwork2/config/providers/xwork-test-allowstatic-devmode-true.xml @@ -0,0 +1,88 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + \ No newline at end of file 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 new file mode 100644 index 000000000..6dc36eb3c --- /dev/null +++ b/core/src/test/resources/com/opensymphony/xwork2/config/providers/xwork-test-allowstatic-true.xml @@ -0,0 +1,88 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + \ No newline at end of file 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 new file mode 100644 index 000000000..787d8a632 --- /dev/null +++ b/core/src/test/resources/com/opensymphony/xwork2/config/providers/xwork-test-devmode-true.xml @@ -0,0 +1,88 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + \ No newline at end of file