diff --git a/core/src/main/java/com/opensymphony/xwork2/config/impl/DefaultConfiguration.java b/core/src/main/java/com/opensymphony/xwork2/config/impl/DefaultConfiguration.java index b254842228..3715c3bae1 100644 --- a/core/src/main/java/com/opensymphony/xwork2/config/impl/DefaultConfiguration.java +++ b/core/src/main/java/com/opensymphony/xwork2/config/impl/DefaultConfiguration.java @@ -85,6 +85,7 @@ import com.opensymphony.xwork2.ognl.OgnlReflectionProvider; import com.opensymphony.xwork2.ognl.OgnlUtil; import com.opensymphony.xwork2.ognl.OgnlValueStackFactory; +import com.opensymphony.xwork2.ognl.SecurityMemberAccess; import com.opensymphony.xwork2.ognl.accessor.CompoundRootAccessor; import com.opensymphony.xwork2.util.CompoundRoot; import com.opensymphony.xwork2.util.OgnlTextParser; @@ -132,16 +133,13 @@ public class DefaultConfiguration implements Configuration { static { Map constants = new HashMap<>(); constants.put(StrutsConstants.STRUTS_DEVMODE, Boolean.FALSE); - constants.put(StrutsConstants.STRUTS_OGNL_LOG_MISSING_PROPERTIES, Boolean.FALSE); - constants.put(StrutsConstants.STRUTS_OGNL_ENABLE_EVAL_EXPRESSION, Boolean.FALSE); - constants.put(StrutsConstants.STRUTS_OGNL_ENABLE_EXPRESSION_CACHE, Boolean.TRUE); constants.put(StrutsConstants.STRUTS_CONFIGURATION_XML_RELOAD, Boolean.FALSE); - constants.put(StrutsConstants.STRUTS_I18N_RELOAD, Boolean.FALSE); constants.put(StrutsConstants.STRUTS_MATCHER_APPEND_NAMED_PARAMETERS, Boolean.TRUE); constants.put(StrutsConstants.STRUTS_OGNL_EXPRESSION_CACHE_TYPE, OgnlCacheFactory.CacheType.BASIC); constants.put(StrutsConstants.STRUTS_OGNL_EXPRESSION_CACHE_MAXSIZE, 10000); constants.put(StrutsConstants.STRUTS_OGNL_BEANINFO_CACHE_TYPE, OgnlCacheFactory.CacheType.BASIC); constants.put(StrutsConstants.STRUTS_OGNL_BEANINFO_CACHE_MAXSIZE, 10000); + constants.put(StrutsConstants.STRUTS_ENABLE_DYNAMIC_METHOD_INVOCATION, Boolean.FALSE); BOOTSTRAP_CONSTANTS = Collections.unmodifiableMap(constants); } @@ -385,6 +383,7 @@ protected Container createBootstrapContainer(List providers) builder.factory(ExpressionCacheFactory.class, DefaultOgnlExpressionCacheFactory.class, Scope.SINGLETON); builder.factory(BeanInfoCacheFactory.class, DefaultOgnlBeanInfoCacheFactory.class, Scope.SINGLETON); builder.factory(OgnlUtil.class, Scope.SINGLETON); + builder.factory(SecurityMemberAccess.class, Scope.PROTOTYPE); builder.factory(OgnlGuard.class, StrutsOgnlGuard.class, Scope.SINGLETON); builder.factory(ValueSubstitutor.class, EnvsValueSubstitutor.class, Scope.SINGLETON); diff --git a/core/src/main/java/com/opensymphony/xwork2/config/impl/MockConfiguration.java b/core/src/main/java/com/opensymphony/xwork2/config/impl/MockConfiguration.java index d245ccf4b3..30eee9566e 100644 --- a/core/src/main/java/com/opensymphony/xwork2/config/impl/MockConfiguration.java +++ b/core/src/main/java/com/opensymphony/xwork2/config/impl/MockConfiguration.java @@ -30,7 +30,6 @@ import com.opensymphony.xwork2.inject.ContainerBuilder; import com.opensymphony.xwork2.inject.Scope; import com.opensymphony.xwork2.util.location.LocatableProperties; -import org.apache.struts2.StrutsConstants; import java.util.HashMap; import java.util.HashSet; @@ -62,7 +61,6 @@ public void selfRegister() { for (Map.Entry entry : DefaultConfiguration.BOOTSTRAP_CONSTANTS.entrySet()) { builder.constant(entry.getKey(), String.valueOf(entry.getValue())); } - builder.constant(StrutsConstants.STRUTS_ENABLE_DYNAMIC_METHOD_INVOCATION, "false"); container = builder.create(true); } diff --git a/core/src/main/java/com/opensymphony/xwork2/config/providers/StrutsDefaultConfigurationProvider.java b/core/src/main/java/com/opensymphony/xwork2/config/providers/StrutsDefaultConfigurationProvider.java index 625a4fb17b..af2eff4d8b 100644 --- a/core/src/main/java/com/opensymphony/xwork2/config/providers/StrutsDefaultConfigurationProvider.java +++ b/core/src/main/java/com/opensymphony/xwork2/config/providers/StrutsDefaultConfigurationProvider.java @@ -75,6 +75,7 @@ import com.opensymphony.xwork2.ognl.OgnlReflectionProvider; import com.opensymphony.xwork2.ognl.OgnlUtil; import com.opensymphony.xwork2.ognl.OgnlValueStackFactory; +import com.opensymphony.xwork2.ognl.SecurityMemberAccess; import com.opensymphony.xwork2.ognl.accessor.CompoundRootAccessor; import com.opensymphony.xwork2.ognl.accessor.HttpParametersPropertyAccessor; import com.opensymphony.xwork2.ognl.accessor.ObjectAccessor; @@ -113,7 +114,6 @@ import com.opensymphony.xwork2.validator.ValidatorFileParser; import ognl.MethodAccessor; import ognl.PropertyAccessor; -import org.apache.struts2.StrutsConstants; import org.apache.struts2.conversion.StrutsConversionPropertiesProcessor; import org.apache.struts2.conversion.StrutsTypeConverterCreator; import org.apache.struts2.conversion.StrutsTypeConverterHolder; @@ -230,6 +230,7 @@ public void register(ContainerBuilder builder, LocatableProperties props) .factory(ExpressionCacheFactory.class, DefaultOgnlExpressionCacheFactory.class, Scope.SINGLETON) .factory(BeanInfoCacheFactory.class, DefaultOgnlBeanInfoCacheFactory.class, Scope.SINGLETON) .factory(OgnlUtil.class, Scope.SINGLETON) + .factory(SecurityMemberAccess.class, Scope.PROTOTYPE) .factory(OgnlGuard.class, StrutsOgnlGuard.class, Scope.SINGLETON) .factory(CollectionConverter.class, Scope.SINGLETON) .factory(ArrayConverter.class, Scope.SINGLETON) @@ -255,8 +256,5 @@ public void register(ContainerBuilder builder, LocatableProperties props) for (Map.Entry entry : DefaultConfiguration.BOOTSTRAP_CONSTANTS.entrySet()) { props.setProperty(entry.getKey(), String.valueOf(entry.getValue())); } - - props.setProperty(StrutsConstants.STRUTS_ALLOW_STATIC_FIELD_ACCESS, Boolean.TRUE.toString()); - props.setProperty(StrutsConstants.STRUTS_ENABLE_DYNAMIC_METHOD_INVOCATION, Boolean.FALSE.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 1f019f64a2..a9e3045cc9 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java @@ -18,7 +18,6 @@ */ package com.opensymphony.xwork2.ognl; -import com.opensymphony.xwork2.config.ConfigurationException; import com.opensymphony.xwork2.conversion.impl.XWorkConverter; import com.opensymphony.xwork2.inject.Container; import com.opensymphony.xwork2.inject.Inject; @@ -46,22 +45,16 @@ import java.lang.reflect.Method; import java.util.Collection; import java.util.HashMap; -import java.util.HashSet; import java.util.Map; import java.util.Set; import java.util.concurrent.atomic.AtomicBoolean; import java.util.regex.Pattern; -import java.util.regex.PatternSyntaxException; -import static com.opensymphony.xwork2.util.TextParseUtil.commaDelimitedStringToSet; +import static com.opensymphony.xwork2.util.ConfigParseUtil.toClassesSet; +import static com.opensymphony.xwork2.util.ConfigParseUtil.toNewPatternsSet; +import static com.opensymphony.xwork2.util.ConfigParseUtil.toPackageNamesSet; import static java.util.Collections.emptySet; -import static java.util.Collections.unmodifiableSet; import static java.util.Objects.requireNonNull; -import static java.util.stream.Collectors.toSet; -import static org.apache.commons.lang3.StringUtils.strip; -import static org.apache.struts2.StrutsConstants.STRUTS_ALLOWLIST_CLASSES; -import static org.apache.struts2.StrutsConstants.STRUTS_ALLOWLIST_ENABLE; -import static org.apache.struts2.StrutsConstants.STRUTS_ALLOWLIST_PACKAGE_NAMES; import static org.apache.struts2.ognl.OgnlGuard.EXPR_BLOCKED; @@ -87,24 +80,12 @@ public class OgnlUtil { private boolean enableExpressionCache = true; private boolean enableEvalExpression; - private Set excludedClasses = emptySet(); - private Set excludedPackageNamePatterns = emptySet(); - private Set excludedPackageNames = emptySet(); - private Set excludedPackageExemptClasses = emptySet(); - - private boolean enforceAllowlistEnabled = false; - private Set allowlistClasses = emptySet(); - private Set allowlistPackageNames = emptySet(); - - private Set devModeExcludedClasses = emptySet(); - private Set devModeExcludedPackageNamePatterns = emptySet(); - private Set devModeExcludedPackageNames = emptySet(); - private Set devModeExcludedPackageExemptClasses = emptySet(); + private String devModeExcludedClasses = ""; + private String devModeExcludedPackageNamePatterns = ""; + private String devModeExcludedPackageNames = ""; + private String devModeExcludedPackageExemptClasses = ""; private Container container; - private boolean allowStaticFieldAccess = true; - private boolean disallowProxyMemberAccess = false; - private boolean disallowDefaultPackageAccess = false; /** * Construct a new OgnlUtil instance for use with the framework @@ -145,7 +126,7 @@ protected void setDevMode(String mode) { this.devMode = BooleanUtils.toBoolean(mode); } - @Inject(StrutsConstants.STRUTS_OGNL_ENABLE_EXPRESSION_CACHE) + @Inject(value = StrutsConstants.STRUTS_OGNL_ENABLE_EXPRESSION_CACHE, required = false) protected void setEnableExpressionCache(String cache) { enableExpressionCache = BooleanUtils.toBoolean(cache); } @@ -175,146 +156,88 @@ protected void setEnableEvalExpression(String evalExpression) { } } - @Inject(value = StrutsConstants.STRUTS_EXCLUDED_CLASSES, required = false) + /** + * @deprecated since 6.4.0, no replacement. + */ + @Deprecated protected void setExcludedClasses(String commaDelimitedClasses) { - excludedClasses = toNewClassesSet(excludedClasses, commaDelimitedClasses); + // Must be set directly on SecurityMemberAccess } @Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_CLASSES, required = false) protected void setDevModeExcludedClasses(String commaDelimitedClasses) { - devModeExcludedClasses = toNewClassesSet(devModeExcludedClasses, commaDelimitedClasses); + this.devModeExcludedClasses = commaDelimitedClasses; } - private static Set toClassesSet(String newDelimitedClasses) throws ConfigurationException { - Set classNames = commaDelimitedStringToSet(newDelimitedClasses); - validateClasses(classNames, OgnlUtil.class.getClassLoader()); - return unmodifiableSet(classNames); - } - - private static Set toNewClassesSet(Set oldClasses, String newDelimitedClasses) throws ConfigurationException { - Set classNames = commaDelimitedStringToSet(newDelimitedClasses); - validateClasses(classNames, OgnlUtil.class.getClassLoader()); - Set excludedClasses = new HashSet<>(oldClasses); - excludedClasses.addAll(classNames); - return unmodifiableSet(excludedClasses); - } - - private static void validateClasses(Set classNames, ClassLoader validatingClassLoader) throws ConfigurationException { - for (String className : classNames) { - try { - validatingClassLoader.loadClass(className); - } catch (ClassNotFoundException e) { - throw new ConfigurationException("Cannot load class for exclusion/exemption configuration: " + className, e); - } - } - } - - @Inject(value = StrutsConstants.STRUTS_EXCLUDED_PACKAGE_NAME_PATTERNS, required = false) + /** + * @deprecated since 6.4.0, no replacement. + */ + @Deprecated protected void setExcludedPackageNamePatterns(String commaDelimitedPackagePatterns) { - excludedPackageNamePatterns = toNewPatternsSet(excludedPackageNamePatterns, commaDelimitedPackagePatterns); + // Must be set directly on SecurityMemberAccess } @Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_PACKAGE_NAME_PATTERNS, required = false) protected void setDevModeExcludedPackageNamePatterns(String commaDelimitedPackagePatterns) { - devModeExcludedPackageNamePatterns = toNewPatternsSet(devModeExcludedPackageNamePatterns, commaDelimitedPackagePatterns); + this.devModeExcludedPackageNamePatterns = commaDelimitedPackagePatterns; } - private static Set toNewPatternsSet(Set oldPatterns, String newDelimitedPatterns) throws ConfigurationException { - Set patterns = commaDelimitedStringToSet(newDelimitedPatterns); - Set newPatterns = new HashSet<>(oldPatterns); - for (String pattern: patterns) { - try { - newPatterns.add(Pattern.compile(pattern)); - } catch (PatternSyntaxException e) { - throw new ConfigurationException("Excluded package name patterns could not be parsed due to invalid regex: " + pattern, e); - } - } - return unmodifiableSet(newPatterns); - } - - @Inject(value = StrutsConstants.STRUTS_EXCLUDED_PACKAGE_NAMES, required = false) + /** + * @deprecated since 6.4.0, no replacement. + */ + @Deprecated protected void setExcludedPackageNames(String commaDelimitedPackageNames) { - excludedPackageNames = toNewPackageNamesSet(excludedPackageNames, commaDelimitedPackageNames); + // Must be set directly on SecurityMemberAccess } @Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_PACKAGE_NAMES, required = false) protected void setDevModeExcludedPackageNames(String commaDelimitedPackageNames) { - devModeExcludedPackageNames = toNewPackageNamesSet(devModeExcludedPackageNames, commaDelimitedPackageNames); - } - - private static Set toPackageNamesSet(String newDelimitedPackageNames) throws ConfigurationException { - Set packageNames = commaDelimitedStringToSet(newDelimitedPackageNames) - .stream().map(s -> strip(s, ".")).collect(toSet()); - validatePackageNames(packageNames); - return unmodifiableSet(packageNames); - } - - private static Set toNewPackageNamesSet(Collection oldPackageNames, String newDelimitedPackageNames) throws ConfigurationException { - Set packageNames = commaDelimitedStringToSet(newDelimitedPackageNames) - .stream().map(s -> strip(s, ".")).collect(toSet()); - validatePackageNames(packageNames); - Set newPackageNames = new HashSet<>(oldPackageNames); - newPackageNames.addAll(packageNames); - return unmodifiableSet(newPackageNames); + this.devModeExcludedPackageNames = commaDelimitedPackageNames; } - private static void validatePackageNames(Collection packageNames) { - if (packageNames.stream().anyMatch(s -> Pattern.compile("\\s").matcher(s).find())) { - throw new ConfigurationException("Excluded package names could not be parsed due to erroneous whitespace characters: " + packageNames); - } - } - - @Inject(value = StrutsConstants.STRUTS_EXCLUDED_PACKAGE_EXEMPT_CLASSES, required = false) + /** + * @deprecated since 6.4.0, no replacement. + */ + @Deprecated public void setExcludedPackageExemptClasses(String commaDelimitedClasses) { - excludedPackageExemptClasses = toClassesSet(commaDelimitedClasses); + // Must be set directly on SecurityMemberAccess } @Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_PACKAGE_EXEMPT_CLASSES, required = false) public void setDevModeExcludedPackageExemptClasses(String commaDelimitedClasses) { - devModeExcludedPackageExemptClasses = toClassesSet(commaDelimitedClasses); + this.devModeExcludedPackageExemptClasses = commaDelimitedClasses; } + /** + * @deprecated since 6.4.0, no replacement. + */ + @Deprecated public Set getExcludedClasses() { - return excludedClasses; + return toClassesSet(container.getInstance(String.class, StrutsConstants.STRUTS_EXCLUDED_CLASSES)); } + /** + * @deprecated since 6.4.0, no replacement. + */ + @Deprecated public Set getExcludedPackageNamePatterns() { - return excludedPackageNamePatterns; + return toNewPatternsSet(emptySet(), container.getInstance(String.class, StrutsConstants.STRUTS_EXCLUDED_PACKAGE_NAME_PATTERNS)); } + /** + * @deprecated since 6.4.0, no replacement. + */ + @Deprecated public Set getExcludedPackageNames() { - return excludedPackageNames; + return toPackageNamesSet(container.getInstance(String.class, StrutsConstants.STRUTS_EXCLUDED_PACKAGE_NAMES)); } + /** + * @deprecated since 6.4.0, no replacement. + */ + @Deprecated public Set getExcludedPackageExemptClasses() { - return excludedPackageExemptClasses; - } - - @Inject(value = STRUTS_ALLOWLIST_ENABLE, required = false) - protected void setEnforceAllowlistEnabled(String enforceAllowlistEnabled) { - this.enforceAllowlistEnabled = BooleanUtils.toBoolean(enforceAllowlistEnabled); - } - - @Inject(value = STRUTS_ALLOWLIST_CLASSES, required = false) - protected void setAllowlistClasses(String commaDelimitedClasses) { - allowlistClasses = toClassesSet(commaDelimitedClasses); - } - - @Inject(value = STRUTS_ALLOWLIST_PACKAGE_NAMES, required = false) - protected void setAllowlistPackageNames(String commaDelimitedPackageNames) { - allowlistPackageNames = toPackageNamesSet(commaDelimitedPackageNames); - } - - public boolean isEnforceAllowlistEnabled() { - return enforceAllowlistEnabled; - } - - public Set getAllowlistClasses() { - return allowlistClasses; - } - - public Set getAllowlistPackageNames() { - return allowlistPackageNames; + return toClassesSet(container.getInstance(String.class, StrutsConstants.STRUTS_EXCLUDED_PACKAGE_EXEMPT_CLASSES)); } @Inject @@ -322,19 +245,28 @@ protected void setContainer(Container container) { this.container = container; } - @Inject(value = StrutsConstants.STRUTS_ALLOW_STATIC_FIELD_ACCESS, required = false) + /** + * @deprecated since 6.4.0, no replacement. + */ + @Deprecated protected void setAllowStaticFieldAccess(String allowStaticFieldAccess) { - this.allowStaticFieldAccess = BooleanUtils.toBoolean(allowStaticFieldAccess); + // Must be set directly on SecurityMemberAccess } - @Inject(value = StrutsConstants.STRUTS_DISALLOW_PROXY_MEMBER_ACCESS, required = false) + /** + * @deprecated since 6.4.0, no replacement. + */ + @Deprecated protected void setDisallowProxyMemberAccess(String disallowProxyMemberAccess) { - this.disallowProxyMemberAccess = BooleanUtils.toBoolean(disallowProxyMemberAccess); + // Must be set directly on SecurityMemberAccess } - @Inject(value = StrutsConstants.STRUTS_DISALLOW_DEFAULT_PACKAGE_ACCESS, required = false) + /** + * @deprecated since 6.4.0, no replacement. + */ + @Deprecated protected void setDisallowDefaultPackageAccess(String disallowDefaultPackageAccess) { - this.disallowDefaultPackageAccess = BooleanUtils.toBoolean(disallowDefaultPackageAccess); + // Must be set directly on SecurityMemberAccess } /** @@ -356,12 +288,20 @@ protected void applyExpressionMaxLength(String maxLength) { } } + /** + * @deprecated since 6.4.0, no replacement. + */ + @Deprecated public boolean isDisallowProxyMemberAccess() { - return disallowProxyMemberAccess; + return BooleanUtils.toBoolean(container.getInstance(String.class, StrutsConstants.STRUTS_DISALLOW_PROXY_MEMBER_ACCESS)); } + /** + * @deprecated since 6.4.0, no replacement. + */ + @Deprecated public boolean isDisallowDefaultPackageAccess() { - return disallowDefaultPackageAccess; + return BooleanUtils.toBoolean(container.getInstance(String.class, StrutsConstants.STRUTS_DISALLOW_DEFAULT_PACKAGE_ACCESS)); } /** @@ -922,8 +862,7 @@ protected Map createDefaultContext(Object root, ClassResolver cl resolver = container.getInstance(CompoundRootAccessor.class); } - SecurityMemberAccess memberAccess = new SecurityMemberAccess(allowStaticFieldAccess); - memberAccess.disallowProxyMemberAccess(disallowProxyMemberAccess); + SecurityMemberAccess memberAccess = container.getInstance(SecurityMemberAccess.class); if (devMode) { if (!warnReported.get()) { @@ -934,14 +873,6 @@ protected Map createDefaultContext(Object root, ClassResolver cl memberAccess.useExcludedPackageNamePatterns(devModeExcludedPackageNamePatterns); memberAccess.useExcludedPackageNames(devModeExcludedPackageNames); memberAccess.useExcludedPackageExemptClasses(devModeExcludedPackageExemptClasses); - } else { - memberAccess.useExcludedClasses(getExcludedClasses()); - memberAccess.useExcludedPackageNamePatterns(getExcludedPackageNamePatterns()); - memberAccess.useExcludedPackageNames(getExcludedPackageNames()); - memberAccess.useExcludedPackageExemptClasses(getExcludedPackageExemptClasses()); - memberAccess.useEnforceAllowlistEnabled(isEnforceAllowlistEnabled()); - memberAccess.useAllowlistClasses(getAllowlistClasses()); - memberAccess.useAllowlistPackageNames(getAllowlistPackageNames()); } return Ognl.createDefaultContext(root, memberAccess, resolver, defaultConverter); 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 01b6af81d7..ddcd4429a1 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java @@ -77,38 +77,91 @@ public class OgnlValueStack implements Serializable, ValueStack, ClearableValueS private boolean devMode; private boolean logMissingProperties; + /** + * @since 6.4.0 + */ + protected OgnlValueStack(ValueStack vs, + XWorkConverter xworkConverter, + CompoundRootAccessor accessor, + TextProvider prov, + SecurityMemberAccess securityMemberAccess) { + setRoot(xworkConverter, + accessor, + vs != null ? new CompoundRoot(vs.getRoot()) : new CompoundRoot(), + securityMemberAccess); + if (prov != null) { + push(prov); + } + } + + /** + * @since 6.4.0 + */ + protected OgnlValueStack(XWorkConverter xworkConverter, CompoundRootAccessor accessor, TextProvider prov, SecurityMemberAccess securityMemberAccess) { + this(null, xworkConverter, accessor, prov, securityMemberAccess); + } + + /** + * @since 6.4.0 + */ + protected OgnlValueStack(ValueStack vs, XWorkConverter xworkConverter, CompoundRootAccessor accessor, SecurityMemberAccess securityMemberAccess) { + this(vs, xworkConverter, accessor, null, securityMemberAccess); + } + + /** + * @deprecated since 6.4.0, use {@link #OgnlValueStack(ValueStack, XWorkConverter, CompoundRootAccessor, TextProvider, SecurityMemberAccess)} instead. + */ + @Deprecated + protected OgnlValueStack(ValueStack vs, + XWorkConverter xworkConverter, + CompoundRootAccessor accessor, + TextProvider prov, + boolean allowStaticFieldAccess) { + this(vs, xworkConverter, accessor, prov, new SecurityMemberAccess(allowStaticFieldAccess)); + } + + /** + * @deprecated since 6.4.0, use {@link #OgnlValueStack(XWorkConverter, CompoundRootAccessor, TextProvider, SecurityMemberAccess)} instead. + */ + @Deprecated protected OgnlValueStack(XWorkConverter xworkConverter, CompoundRootAccessor accessor, TextProvider prov, boolean allowStaticFieldAccess) { - setRoot(xworkConverter, accessor, new CompoundRoot(), allowStaticFieldAccess); - push(prov); + this(xworkConverter, accessor, prov, new SecurityMemberAccess(allowStaticFieldAccess)); } + /** + * @deprecated since 6.4.0, use {@link #OgnlValueStack(ValueStack, XWorkConverter, CompoundRootAccessor, SecurityMemberAccess)} instead. + */ + @Deprecated protected OgnlValueStack(ValueStack vs, XWorkConverter xworkConverter, CompoundRootAccessor accessor, boolean allowStaticFieldAccess) { - setRoot(xworkConverter, accessor, new CompoundRoot(vs.getRoot()), allowStaticFieldAccess); + this(vs, xworkConverter, accessor, new SecurityMemberAccess(allowStaticFieldAccess)); } @Inject protected void setOgnlUtil(OgnlUtil ognlUtil) { this.ognlUtil = ognlUtil; - securityMemberAccess.useExcludedClasses(ognlUtil.getExcludedClasses()); - securityMemberAccess.useExcludedPackageNamePatterns(ognlUtil.getExcludedPackageNamePatterns()); - securityMemberAccess.useExcludedPackageNames(ognlUtil.getExcludedPackageNames()); - securityMemberAccess.useExcludedPackageExemptClasses(ognlUtil.getExcludedPackageExemptClasses()); - securityMemberAccess.useEnforceAllowlistEnabled(ognlUtil.isEnforceAllowlistEnabled()); - securityMemberAccess.useAllowlistClasses(ognlUtil.getAllowlistClasses()); - securityMemberAccess.useAllowlistPackageNames(ognlUtil.getAllowlistPackageNames()); - securityMemberAccess.disallowProxyMemberAccess(ognlUtil.isDisallowProxyMemberAccess()); - securityMemberAccess.disallowDefaultPackageAccess(ognlUtil.isDisallowDefaultPackageAccess()); } - protected void setRoot(XWorkConverter xworkConverter, CompoundRootAccessor accessor, CompoundRoot compoundRoot, boolean allowStaticFieldAccess) { + /** + * @since 6.4.0 + */ + protected void setRoot(XWorkConverter xworkConverter, CompoundRootAccessor accessor, CompoundRoot compoundRoot, SecurityMemberAccess securityMemberAccess) { this.root = compoundRoot; - this.securityMemberAccess = new SecurityMemberAccess(allowStaticFieldAccess); + this.securityMemberAccess = securityMemberAccess; this.context = Ognl.createDefaultContext(this.root, securityMemberAccess, accessor, new OgnlTypeConverterWrapper(xworkConverter)); + this.converter = xworkConverter; context.put(VALUE_STACK, this); ((OgnlContext) context).setTraceEvaluations(false); ((OgnlContext) context).setKeepLastEvaluation(false); } + /** + * @deprecated since 6.4.0, use {@link #setRoot(XWorkConverter, CompoundRootAccessor, CompoundRoot, SecurityMemberAccess)} instead. + */ + @Deprecated + protected void setRoot(XWorkConverter xworkConverter, CompoundRootAccessor accessor, CompoundRoot compoundRoot, boolean allowStaticFieldAccess) { + setRoot(xworkConverter, accessor, compoundRoot, new SecurityMemberAccess(allowStaticFieldAccess)); + } + @Inject(StrutsConstants.STRUTS_DEVMODE) protected void setDevMode(String mode) { this.devMode = BooleanUtils.toBoolean(mode); @@ -464,6 +517,9 @@ public int size() { return root.size(); } + /** + * Retained for serializability - see {@link com.opensymphony.xwork2.ognl.OgnlValueStackTest#testSerializable} + */ private Object readResolve() { // TODO: this should be done better ActionContext ac = ActionContext.getContext(); @@ -471,41 +527,32 @@ private Object readResolve() { XWorkConverter xworkConverter = cont.getInstance(XWorkConverter.class); CompoundRootAccessor accessor = (CompoundRootAccessor) cont.getInstance(PropertyAccessor.class, CompoundRoot.class.getName()); TextProvider prov = cont.getInstance(TextProvider.class, "system"); - final boolean allowStaticField = BooleanUtils.toBoolean(cont.getInstance(String.class, StrutsConstants.STRUTS_ALLOW_STATIC_FIELD_ACCESS)); - OgnlValueStack aStack = new OgnlValueStack(xworkConverter, accessor, prov, allowStaticField); + SecurityMemberAccess sma = cont.getInstance(SecurityMemberAccess.class); + OgnlValueStack aStack = new OgnlValueStack(xworkConverter, accessor, prov, sma); aStack.setOgnlUtil(cont.getInstance(OgnlUtil.class)); - aStack.setRoot(xworkConverter, accessor, this.root, allowStaticField); - + aStack.setRoot(xworkConverter, accessor, this.root, sma); return aStack; } - public void clearContextValues() { //this is an OGNL ValueStack so the context will be an OgnlContext //it would be better to make context of type OgnlContext ((OgnlContext) context).getValues().clear(); } - @Deprecated - public void setAcceptProperties(Set acceptedProperties) { - securityMemberAccess.useAcceptProperties(acceptedProperties); - } - public void useAcceptProperties(Set acceptedProperties) { securityMemberAccess.useAcceptProperties(acceptedProperties); } - @Deprecated - public void setExcludeProperties(Set excludeProperties) { - securityMemberAccess.useExcludeProperties(excludeProperties); - } - public void useExcludeProperties(Set excludeProperties) { securityMemberAccess.useExcludeProperties(excludeProperties); } - @Inject + /** + * @deprecated since 6.4.0, no replacement. + */ + @Deprecated protected void setXWorkConverter(final XWorkConverter converter) { - this.converter = converter; + // no-op } } 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 69dd540262..111a44d793 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStackFactory.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStackFactory.java @@ -31,11 +31,8 @@ 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 org.apache.struts2.StrutsConstants; -import java.util.Map; import java.util.Set; /** @@ -59,21 +56,17 @@ protected void setTextProvider(TextProvider textProvider) { } public ValueStack createValueStack() { - ValueStack stack = new OgnlValueStack(xworkConverter, compoundRootAccessor, textProvider, containerAllowsStaticFieldAccess()); + ValueStack stack = new OgnlValueStack( + xworkConverter, compoundRootAccessor, textProvider, container.getInstance(SecurityMemberAccess.class)); container.inject(stack); - return stack.getActionContext() - .withContainer(container) - .withValueStack(stack) - .getValueStack(); + return stack.getActionContext().withContainer(container).withValueStack(stack).getValueStack(); } public ValueStack createValueStack(ValueStack stack) { - ValueStack result = new OgnlValueStack(stack, xworkConverter, compoundRootAccessor, containerAllowsStaticFieldAccess()); + ValueStack result = new OgnlValueStack( + stack, xworkConverter, compoundRootAccessor, container.getInstance(SecurityMemberAccess.class)); container.inject(result); - return result.getActionContext() - .withContainer(container) - .withValueStack(result) - .getValueStack(); + return result.getActionContext().withContainer(container).withValueStack(result).getValueStack(); } @Inject @@ -105,10 +98,10 @@ protected void setContainer(Container container) throws ClassNotFoundException { } /** - * Retrieve allowStaticFieldAccess state from the container (allows for lazy fetching) + * @deprecated since 6.4.0, no replacement. */ + @Deprecated 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 87af5e0b60..a5d2aa0b43 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java @@ -18,10 +18,13 @@ */ package com.opensymphony.xwork2.ognl; +import com.opensymphony.xwork2.inject.Inject; import com.opensymphony.xwork2.util.ProxyUtil; import ognl.MemberAccess; +import org.apache.commons.lang3.BooleanUtils; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; +import org.apache.struts2.StrutsConstants; import java.lang.reflect.AccessibleObject; import java.lang.reflect.Field; @@ -36,10 +39,15 @@ import java.util.regex.Matcher; import java.util.regex.Pattern; +import static com.opensymphony.xwork2.util.ConfigParseUtil.toClassesSet; +import static com.opensymphony.xwork2.util.ConfigParseUtil.toNewClassesSet; +import static com.opensymphony.xwork2.util.ConfigParseUtil.toNewPackageNamesSet; +import static com.opensymphony.xwork2.util.ConfigParseUtil.toNewPatternsSet; +import static com.opensymphony.xwork2.util.ConfigParseUtil.toPackageNamesSet; import static java.text.MessageFormat.format; import static java.util.Collections.emptySet; +import static java.util.Collections.singletonList; import static java.util.Collections.unmodifiableSet; -import static java.util.stream.Collectors.toSet; /** * Allows access decisions to be made on the basis of whether a member is static or not. @@ -49,10 +57,10 @@ public class SecurityMemberAccess implements MemberAccess { private static final Logger LOG = LogManager.getLogger(SecurityMemberAccess.class); - private final boolean allowStaticFieldAccess; + private boolean allowStaticFieldAccess = true; private Set excludeProperties = emptySet(); private Set acceptProperties = emptySet(); - private Set excludedClasses = emptySet(); + private Set excludedClasses = unmodifiableSet(new HashSet<>(singletonList(Object.class.getName()))); private Set excludedPackageNamePatterns = emptySet(); private Set excludedPackageNames = emptySet(); private Set excludedPackageExemptClasses = emptySet(); @@ -62,16 +70,20 @@ public class SecurityMemberAccess implements MemberAccess { private boolean disallowProxyMemberAccess = false; private boolean disallowDefaultPackageAccess = false; + public SecurityMemberAccess() { + } + /** * SecurityMemberAccess * - access decisions based on whether member is static (or not) * - block or allow access to properties (configurable-after-construction) * * @param allowStaticFieldAccess if set to true static fields (constants) will be accessible + * @deprecated since 6.4.0, use {@link #SecurityMemberAccess()} instead. */ + @Deprecated public SecurityMemberAccess(boolean allowStaticFieldAccess) { - this.allowStaticFieldAccess = allowStaticFieldAccess; - useExcludedClasses(excludedClasses); // Initialise default exclusions + useAllowStaticFieldAccess(String.valueOf(allowStaticFieldAccess)); } @Override @@ -356,108 +368,64 @@ protected boolean isExcluded(String paramName) { return excludeProperties.stream().map(pattern -> pattern.matcher(paramName)).anyMatch(Matcher::matches); } - /** - * @deprecated please use {@link #useExcludeProperties(Set)} - */ - @Deprecated - public void setExcludeProperties(Set excludeProperties) { - this.excludeProperties = excludeProperties; - } - public void useExcludeProperties(Set excludeProperties) { this.excludeProperties = excludeProperties; } - /** - * @deprecated please use {@link #useAcceptProperties(Set)} - */ - @Deprecated - public void setAcceptProperties(Set acceptedProperties) { - this.acceptProperties = acceptedProperties; - } - public void useAcceptProperties(Set acceptedProperties) { this.acceptProperties = acceptedProperties; } - /** - * @deprecated please use {@link #useExcludedClasses(Set)} - */ - @Deprecated - public void setExcludedClasses(Set> excludedClasses) { - useExcludedClasses(excludedClasses.stream().map(Class::getName).collect(toSet())); - } - - public void useExcludedClasses(Set excludedClasses) { - Set newExcludedClasses = new HashSet<>(excludedClasses); - newExcludedClasses.add(Object.class.getName()); - if (!allowStaticFieldAccess) { - newExcludedClasses.add(Class.class.getName()); + @Inject(value = StrutsConstants.STRUTS_ALLOW_STATIC_FIELD_ACCESS, required = false) + public void useAllowStaticFieldAccess(String allowStaticFieldAccess) { + this.allowStaticFieldAccess = BooleanUtils.toBoolean(allowStaticFieldAccess); + if (!this.allowStaticFieldAccess) { + useExcludedClasses(Class.class.getName()); } - this.excludedClasses = unmodifiableSet(newExcludedClasses); } - /** - * @deprecated please use {@link #useExcludedPackageNamePatterns(Set)} - */ - @Deprecated - public void setExcludedPackageNamePatterns(Set excludedPackageNamePatterns) { - this.excludedPackageNamePatterns = excludedPackageNamePatterns; + @Inject(value = StrutsConstants.STRUTS_EXCLUDED_CLASSES, required = false) + public void useExcludedClasses(String commaDelimitedClasses) { + this.excludedClasses = toNewClassesSet(excludedClasses, commaDelimitedClasses); } - public void useExcludedPackageNamePatterns(Set excludedPackageNamePatterns) { - this.excludedPackageNamePatterns = excludedPackageNamePatterns; + @Inject(value = StrutsConstants.STRUTS_EXCLUDED_PACKAGE_NAME_PATTERNS, required = false) + public void useExcludedPackageNamePatterns(String commaDelimitedPackagePatterns) { + this.excludedPackageNamePatterns = toNewPatternsSet(excludedPackageNamePatterns, commaDelimitedPackagePatterns); } - /** - * @deprecated please use {@link #useExcludedPackageNames(Set)} - */ - @Deprecated - public void setExcludedPackageNames(Set excludedPackageNames) { - this.excludedPackageNames = excludedPackageNames; + @Inject(value = StrutsConstants.STRUTS_EXCLUDED_PACKAGE_NAMES, required = false) + public void useExcludedPackageNames(String commaDelimitedPackageNames) { + this.excludedPackageNames = toNewPackageNamesSet(excludedPackageNames, commaDelimitedPackageNames); } - public void useExcludedPackageNames(Set excludedPackageNames) { - this.excludedPackageNames = excludedPackageNames; + @Inject(value = StrutsConstants.STRUTS_EXCLUDED_PACKAGE_EXEMPT_CLASSES, required = false) + public void useExcludedPackageExemptClasses(String commaDelimitedClasses) { + this.excludedPackageExemptClasses = toClassesSet(commaDelimitedClasses); } - /** - * @deprecated please use {@link #useExcludedPackageExemptClasses(Set)} - */ - @Deprecated - public void setExcludedPackageExemptClasses(Set> excludedPackageExemptClasses) { - useExcludedPackageExemptClasses(excludedPackageExemptClasses.stream().map(Class::getName).collect(toSet())); + @Inject(value = StrutsConstants.STRUTS_ALLOWLIST_ENABLE, required = false) + public void useEnforceAllowlistEnabled(String enforceAllowlistEnabled) { + this.enforceAllowlistEnabled = BooleanUtils.toBoolean(enforceAllowlistEnabled); } - public void useExcludedPackageExemptClasses(Set excludedPackageExemptClasses) { - this.excludedPackageExemptClasses = excludedPackageExemptClasses; + @Inject(value = StrutsConstants.STRUTS_ALLOWLIST_CLASSES, required = false) + public void useAllowlistClasses(String commaDelimitedClasses) { + this.allowlistClasses = toClassesSet(commaDelimitedClasses); } - public void useEnforceAllowlistEnabled(boolean enforceAllowlistEnabled) { - this.enforceAllowlistEnabled = enforceAllowlistEnabled; - } - - public void useAllowlistClasses(Set allowlistClasses) { - this.allowlistClasses = allowlistClasses; - } - - public void useAllowlistPackageNames(Set allowlistPackageNames) { - this.allowlistPackageNames = allowlistPackageNames; - } - - /** - * @deprecated please use {@link #disallowProxyMemberAccess(boolean)} - */ - @Deprecated - public void setDisallowProxyMemberAccess(boolean disallowProxyMemberAccess) { - this.disallowProxyMemberAccess = disallowProxyMemberAccess; + @Inject(value = StrutsConstants.STRUTS_ALLOWLIST_PACKAGE_NAMES, required = false) + public void useAllowlistPackageNames(String commaDelimitedPackageNames) { + this.allowlistPackageNames = toPackageNamesSet(commaDelimitedPackageNames); } - public void disallowProxyMemberAccess(boolean disallowProxyMemberAccess) { - this.disallowProxyMemberAccess = disallowProxyMemberAccess; + @Inject(value = StrutsConstants.STRUTS_DISALLOW_PROXY_MEMBER_ACCESS, required = false) + public void useDisallowProxyMemberAccess(String disallowProxyMemberAccess) { + this.disallowProxyMemberAccess = BooleanUtils.toBoolean(disallowProxyMemberAccess); } - public void disallowDefaultPackageAccess(boolean disallowDefaultPackageAccess) { - this.disallowDefaultPackageAccess = disallowDefaultPackageAccess; + @Inject(value = StrutsConstants.STRUTS_DISALLOW_DEFAULT_PACKAGE_ACCESS, required = false) + public void useDisallowDefaultPackageAccess(String disallowDefaultPackageAccess) { + this.disallowDefaultPackageAccess = BooleanUtils.toBoolean(disallowDefaultPackageAccess); } } diff --git a/core/src/main/java/com/opensymphony/xwork2/util/ConfigParseUtil.java b/core/src/main/java/com/opensymphony/xwork2/util/ConfigParseUtil.java new file mode 100644 index 0000000000..8debd07db0 --- /dev/null +++ b/core/src/main/java/com/opensymphony/xwork2/util/ConfigParseUtil.java @@ -0,0 +1,98 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package com.opensymphony.xwork2.util; + +import com.opensymphony.xwork2.config.ConfigurationException; +import com.opensymphony.xwork2.ognl.OgnlUtil; + +import java.util.Collection; +import java.util.HashSet; +import java.util.Set; +import java.util.regex.Pattern; +import java.util.regex.PatternSyntaxException; + +import static com.opensymphony.xwork2.util.TextParseUtil.commaDelimitedStringToSet; +import static java.util.Collections.unmodifiableSet; +import static java.util.stream.Collectors.toSet; +import static org.apache.commons.lang3.StringUtils.strip; + +public class ConfigParseUtil { + + private ConfigParseUtil() { + } + + public static Set toClassesSet(String newDelimitedClasses) throws ConfigurationException { + Set classNames = commaDelimitedStringToSet(newDelimitedClasses); + validateClasses(classNames, OgnlUtil.class.getClassLoader()); + return unmodifiableSet(classNames); + } + + public static Set toNewClassesSet(Set oldClasses, String newDelimitedClasses) throws ConfigurationException { + Set classNames = commaDelimitedStringToSet(newDelimitedClasses); + validateClasses(classNames, OgnlUtil.class.getClassLoader()); + Set excludedClasses = new HashSet<>(oldClasses); + excludedClasses.addAll(classNames); + return unmodifiableSet(excludedClasses); + } + + public static Set toNewPatternsSet(Set oldPatterns, String newDelimitedPatterns) throws ConfigurationException { + Set patterns = commaDelimitedStringToSet(newDelimitedPatterns); + Set newPatterns = new HashSet<>(oldPatterns); + for (String pattern: patterns) { + try { + newPatterns.add(Pattern.compile(pattern)); + } catch (PatternSyntaxException e) { + throw new ConfigurationException("Excluded package name patterns could not be parsed due to invalid regex: " + pattern, e); + } + } + return unmodifiableSet(newPatterns); + } + + public static void validateClasses(Set classNames, ClassLoader validatingClassLoader) throws ConfigurationException { + for (String className : classNames) { + try { + validatingClassLoader.loadClass(className); + } catch (ClassNotFoundException e) { + throw new ConfigurationException("Cannot load class for exclusion/exemption configuration: " + className, e); + } + } + } + + public static Set toPackageNamesSet(String newDelimitedPackageNames) throws ConfigurationException { + Set packageNames = commaDelimitedStringToSet(newDelimitedPackageNames) + .stream().map(s -> strip(s, ".")).collect(toSet()); + validatePackageNames(packageNames); + return unmodifiableSet(packageNames); + } + + public static Set toNewPackageNamesSet(Collection oldPackageNames, String newDelimitedPackageNames) throws ConfigurationException { + Set packageNames = commaDelimitedStringToSet(newDelimitedPackageNames) + .stream().map(s -> strip(s, ".")).collect(toSet()); + validatePackageNames(packageNames); + Set newPackageNames = new HashSet<>(oldPackageNames); + newPackageNames.addAll(packageNames); + return unmodifiableSet(newPackageNames); + } + + public static void validatePackageNames(Collection packageNames) { + if (packageNames.stream().anyMatch(s -> Pattern.compile("\\s").matcher(s).find())) { + throw new ConfigurationException("Excluded package names could not be parsed due to erroneous whitespace characters: " + packageNames); + } + } +} diff --git a/core/src/main/java/com/opensymphony/xwork2/util/MemberAccessValueStack.java b/core/src/main/java/com/opensymphony/xwork2/util/MemberAccessValueStack.java index 86b39f480b..de222be97b 100644 --- a/core/src/main/java/com/opensymphony/xwork2/util/MemberAccessValueStack.java +++ b/core/src/main/java/com/opensymphony/xwork2/util/MemberAccessValueStack.java @@ -31,7 +31,9 @@ public interface MemberAccessValueStack { * @deprecated please use {@link #useExcludeProperties(Set)} */ @Deprecated - void setExcludeProperties(Set excludeProperties); + default void setExcludeProperties(Set excludeProperties) { + useExcludeProperties(excludeProperties); + } void useExcludeProperties(Set excludeProperties); @@ -39,7 +41,9 @@ public interface MemberAccessValueStack { * @deprecated please use {@link #useAcceptProperties(Set)} */ @Deprecated - void setAcceptProperties(Set acceptedProperties); + default void setAcceptProperties(Set acceptedProperties) { + useAcceptProperties(acceptedProperties); + } void useAcceptProperties(Set acceptedProperties); diff --git a/core/src/main/java/com/opensymphony/xwork2/util/location/LocationImpl.java b/core/src/main/java/com/opensymphony/xwork2/util/location/LocationImpl.java index 298b34b3f4..50a1809fc4 100644 --- a/core/src/main/java/com/opensymphony/xwork2/util/location/LocationImpl.java +++ b/core/src/main/java/com/opensymphony/xwork2/util/location/LocationImpl.java @@ -37,7 +37,7 @@ public class LocationImpl implements Location, Serializable { private final int line; private final int column; private final String description; - + // Package private: outside this package, use Location.UNKNOWN. static final LocationImpl UNKNOWN = new LocationImpl(null, null, -1, -1); @@ -71,16 +71,16 @@ public LocationImpl(String description, String uri, int line, int column) { } this.description = StringUtils.trimToNull(description); } - + /** * Copy constructor. - * + * * @param location the location to be copied */ public LocationImpl(Location location) { this(location.getDescription(), location.getURI(), location.getLineNumber(), location.getColumnNumber()); } - + /** * Create a location from an existing one, but with a different description * @@ -90,14 +90,14 @@ public LocationImpl(Location location) { public LocationImpl(String description, Location location) { this(description, location.getURI(), location.getLineNumber(), location.getColumnNumber()); } - + /** * Obtain a LocationImpl from a {@link Location}. If location is * already a LocationImpl, it is returned, otherwise it is copied. *

* This method is useful when an immutable and serializable location is needed, such as in locatable * exceptions. - * + * * @param location the location * @return an immutable and serializable version of location */ @@ -110,19 +110,19 @@ public static LocationImpl get(Location location) { return new LocationImpl(location); } } - + /** * Get the description of this location - * + * * @return the description (can be null) */ public String getDescription() { return this.description; } - + /** * Get the URI of this location - * + * * @return the URI (null if unknown). */ public String getURI() { @@ -131,22 +131,22 @@ public String getURI() { /** * Get the line number of this location - * + * * @return the line number (-1 if unknown) */ public int getLineNumber() { return this.line; } - + /** * Get the column number of this location - * + * * @return the column number (-1 if unknown) */ public int getColumnNumber() { return this.column; } - + /** * @param padding The amount of lines before and after the error to include * @return a source code snippet with the default padding @@ -184,24 +184,24 @@ public boolean equals(Object obj) { && testEquals(this.uri, other.getURI()) && testEquals(this.description, other.getDescription()); } - + return false; } - + @Override public int hashCode() { int hash = line ^ column; if (uri != null) hash ^= uri.hashCode(); if (description != null) hash ^= description.hashCode(); - + return hash; } - + @Override public String toString() { return LocationUtils.toString(this); } - + /** * Ensure serialized unknown location resolve to {@link Location#UNKNOWN}. * @@ -210,7 +210,7 @@ public String toString() { private Object readResolve() { return this.equals(Location.UNKNOWN) ? Location.UNKNOWN : this; } - + private boolean testEquals(Object object1, Object object2) { if (object1 == object2) { 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 bcb07a69a3..f5fe67a50d 100644 --- a/core/src/main/java/org/apache/struts2/StrutsConstants.java +++ b/core/src/main/java/org/apache/struts2/StrutsConstants.java @@ -234,6 +234,8 @@ public final class StrutsConstants { /** 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"; + public static final String STRUTS_MEMBER_ACCESS = "struts.securityMemberAccess"; + public static final String STRUTS_OGNL_GUARD = "struts.ognlGuard"; /** The com.opensymphony.xwork2.validator.ActionValidatorManager implementation class */ diff --git a/core/src/main/java/org/apache/struts2/config/StrutsBeanSelectionProvider.java b/core/src/main/java/org/apache/struts2/config/StrutsBeanSelectionProvider.java index 70cc85135c..2ac92e8fbf 100644 --- a/core/src/main/java/org/apache/struts2/config/StrutsBeanSelectionProvider.java +++ b/core/src/main/java/org/apache/struts2/config/StrutsBeanSelectionProvider.java @@ -49,6 +49,7 @@ import com.opensymphony.xwork2.inject.Scope; import com.opensymphony.xwork2.ognl.BeanInfoCacheFactory; import com.opensymphony.xwork2.ognl.ExpressionCacheFactory; +import com.opensymphony.xwork2.ognl.SecurityMemberAccess; import com.opensymphony.xwork2.security.AcceptedPatternsChecker; import com.opensymphony.xwork2.security.ExcludedPatternsChecker; import com.opensymphony.xwork2.security.NotExcludedAcceptedPatternsChecker; @@ -435,6 +436,7 @@ public void register(ContainerBuilder builder, LocatableProperties props) { alias(ExpressionCacheFactory.class, StrutsConstants.STRUTS_OGNL_EXPRESSION_CACHE_FACTORY, builder, props, Scope.SINGLETON); alias(BeanInfoCacheFactory.class, StrutsConstants.STRUTS_OGNL_BEANINFO_CACHE_FACTORY, builder, props, Scope.SINGLETON); + alias(SecurityMemberAccess.class, StrutsConstants.STRUTS_MEMBER_ACCESS, builder, props, Scope.PROTOTYPE); alias(OgnlGuard.class, StrutsConstants.STRUTS_OGNL_GUARD, builder, props, Scope.SINGLETON); alias(QueryStringBuilder.class, StrutsConstants.STRUTS_URL_QUERY_STRING_BUILDER, builder, props, Scope.SINGLETON); diff --git a/core/src/main/resources/org/apache/struts2/default.properties b/core/src/main/resources/org/apache/struts2/default.properties index 6b3cb4dfea..96c5459fe6 100644 --- a/core/src/main/resources/org/apache/struts2/default.properties +++ b/core/src/main/resources/org/apache/struts2/default.properties @@ -218,9 +218,6 @@ struts.mapper.alwaysSelectFullNamespace=false ### Whether to allow static field access in OGNL expressions or not struts.ognl.allowStaticFieldAccess=true -### Whether to allow static method access in OGNL expressions or not -struts.ognl.allowStaticMethodAccess=false - ### Whether to throw a RuntimeException when a property is not found ### in an expression, or when the expression evaluation fails struts.el.throwExceptionOnFailure=false @@ -228,10 +225,6 @@ struts.el.throwExceptionOnFailure=false ### Logs as Warnings properties that are not found (very verbose) struts.ognl.logMissingProperties=false -### Caches parsed OGNL expressions, but can lead to memory leaks -### if the application generates a lot of different expressions -struts.ognl.enableExpressionCache=true - ### Specify the OGNL expression cache factory and BeanInfo cache factory to use. ### Currently, the default implementations are used, but can be replaced with custom ones if desired. # struts.ognl.expressionCacheFactory=customOgnlExpressionCacheFactory diff --git a/core/src/main/resources/struts-beans.xml b/core/src/main/resources/struts-beans.xml index 91fb65db77..273b43b87d 100644 --- a/core/src/main/resources/struts-beans.xml +++ b/core/src/main/resources/struts-beans.xml @@ -166,8 +166,9 @@ class="com.opensymphony.xwork2.validator.DefaultValidatorFileParser"/> + + class="org.apache.struts2.ognl.StrutsOgnlGuard"/> diff --git a/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilStrutsTest.java b/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilStrutsTest.java index 0eaf517a41..abaa861448 100644 --- a/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilStrutsTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilStrutsTest.java @@ -31,33 +31,6 @@ public void setUp() throws Exception { ognlUtil = container.getInstance(OgnlUtil.class); } - public void testDefaultExcludes() { - ognlUtil.setExcludedClasses(""); - ognlUtil.setExcludedPackageNames(""); - ognlUtil.setExcludedPackageNamePatterns(""); - assertFalse(ognlUtil.getExcludedClasses().isEmpty()); - assertFalse(ognlUtil.getExcludedPackageNames().isEmpty()); - - try { - ognlUtil.getExcludedClasses().clear(); - fail("Missing the expected Exception"); - } catch (Exception ex) { - assertTrue(ex instanceof UnsupportedOperationException); - } - try { - ognlUtil.getExcludedPackageNames().clear(); - fail("Missing the expected Exception"); - } catch (Exception ex) { - assertTrue(ex instanceof UnsupportedOperationException); - } - try { - ognlUtil.getExcludedPackageNamePatterns().clear(); - fail("Missing the expected Exception"); - } catch (Exception ex) { - assertTrue(ex instanceof UnsupportedOperationException); - } - } - public void testAccessToSizeMethod() throws Exception { // given TestArrayBean bean = new TestArrayBean(); 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 8db142ba68..5df410e2bc 100644 --- a/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java @@ -57,14 +57,11 @@ import java.util.Collection; import java.util.Date; import java.util.HashMap; -import java.util.HashSet; -import java.util.Iterator; import java.util.List; import java.util.Locale; import java.util.Map; -import java.util.Set; -import java.util.regex.Pattern; +import static org.assertj.core.api.Assertions.assertThat; import static org.junit.Assert.assertThrows; public class OgnlUtilTest extends XWorkTestCase { @@ -889,11 +886,11 @@ public void testStringToLong() { public void testBeanMapExpressions() throws OgnlException, NoSuchMethodException { Foo foo = new Foo(); - ognlUtil.setExcludedPackageNames( - "com.opensymphony.xwork2.ognl." - ); Map context = ognlUtil.createDefaultContext(foo); + SecurityMemberAccess sma = (SecurityMemberAccess) ((OgnlContext) context).getMemberAccess(); + + sma.useExcludedPackageNames("com.opensymphony.xwork2.ognl"); String expression = "%{\n" + "(#request.a=#@org.apache.commons.collections.BeanMap@{}) +\n" + @@ -910,8 +907,7 @@ public void testBeanMapExpressions() throws OgnlException, NoSuchMethodException assertEquals(foo.getTitle(), expression); - SecurityMemberAccess sma = (SecurityMemberAccess) ((OgnlContext) context).getMemberAccess(); - assertFalse(sma.isAccessible(context, sma, sma.getClass().getDeclaredMethod("useExcludedClasses", Set.class), "excludedClasses")); + assertFalse(sma.isAccessible(context, sma, sma.getClass().getDeclaredMethod("useExcludedClasses", String.class), "excludedClasses")); } public void testNullProperties() { @@ -1181,22 +1177,16 @@ public void testAllowCallingMethodsOnObjectClassInDevModeTrue() { assertNull(expected); } - public void testAllowCallingMethodsOnObjectClassInDevModeFalse() { - Exception expected = null; - try { - ognlUtil.setExcludedClasses(Foo.class.getName()); - ognlUtil.setDevModeExcludedClasses(""); - ognlUtil.setDevMode(Boolean.FALSE.toString()); + public void testExclusionListDevModeOnOff() throws Exception { + ognlUtil.setDevModeExcludedClasses(Foo.class.getName()); + Foo foo = new Foo(); - Foo foo = new Foo(); - String result = (String) ognlUtil.getValue("toString", ognlUtil.createDefaultContext(foo), foo, String.class); - assertEquals("Foo", result); - } catch (OgnlException e) { - expected = e; - } - assertNotNull(expected); - assertSame(NoSuchPropertyException.class, expected.getClass()); - assertEquals("com.opensymphony.xwork2.util.Foo.toString", expected.getMessage()); + ognlUtil.setDevMode(Boolean.TRUE.toString()); + OgnlException e = assertThrows(OgnlException.class, () -> ognlUtil.getValue("toString", ognlUtil.createDefaultContext(foo), foo, String.class)); + assertThat(e).hasMessageContaining("com.opensymphony.xwork2.util.Foo.toString"); + + ognlUtil.setDevMode(Boolean.FALSE.toString()); + assertEquals("Foo", (String) ognlUtil.getValue("toString", ognlUtil.createDefaultContext(foo), foo, String.class)); } public void testAvoidCallingMethodsOnObjectClassUpperCased() { @@ -1338,18 +1328,6 @@ public void testCallMethod() { assertEquals(expected.getMessage(), "It isn't a simple method which can be called!"); } - public void testXworkTestCaseOgnlUtilExclusions() { - internalTestInitialEmptyOgnlUtilExclusions(ognlUtil); - internalTestOgnlUtilExclusionsImmutable(ognlUtil); - } - - public void testDefaultOgnlUtilExclusions() { - OgnlUtil basicOgnlUtil = new OgnlUtil(); - - internalTestInitialEmptyOgnlUtilExclusions(basicOgnlUtil); - internalTestOgnlUtilExclusionsImmutable(basicOgnlUtil); - } - public void testDefaultOgnlUtilAlternateConstructorArguments() { // Code coverage test for the OgnlUtil alternate constructor method, and verify expected behaviour. try { @@ -1366,85 +1344,6 @@ public void testDefaultOgnlUtilAlternateConstructorArguments() { } } - public void testDefaultOgnlUtilExclusionsAlternateConstructorPopulated() { - OgnlUtil basicOgnlUtil = new OgnlUtil(new DefaultOgnlExpressionCacheFactory<>(), - new DefaultOgnlBeanInfoCacheFactory<>(), - new StrutsOgnlGuard()); - - internalTestInitialEmptyOgnlUtilExclusions(basicOgnlUtil); - internalTestOgnlUtilExclusionsImmutable(basicOgnlUtil); - } - - public void testOgnlUtilExcludedAdditivity() { - Set excludedClasses; - Set excludedPackageNamePatterns; - Iterator excludedPackageNamePatternsIterator; - Set excludedPackageNames; - - ognlUtil.setExcludedClasses("java.lang.String,java.lang.Integer"); - internalTestOgnlUtilExclusionsImmutable(ognlUtil); - excludedClasses = ognlUtil.getExcludedClasses(); - assertNotNull("initial excluded classes null?", excludedClasses); - assertEquals("initial excluded classes size not 2 after adds?", 2, excludedClasses.size()); - assertTrue("String not in exclusions?", excludedClasses.contains(String.class.getName())); - assertTrue("Integer not in exclusions?", excludedClasses.contains(Integer.class.getName())); - ognlUtil.setExcludedClasses("java.lang.Boolean,java.lang.Double"); - internalTestOgnlUtilExclusionsImmutable(ognlUtil); - excludedClasses = ognlUtil.getExcludedClasses(); - assertNotNull("updated excluded classes null?", excludedClasses); - assertEquals("updated excluded classes size not 4 after adds?", 4, excludedClasses.size()); - assertTrue("String not in exclusions?", excludedClasses.contains(String.class.getName())); - assertTrue("Integer not in exclusions?", excludedClasses.contains(Integer.class.getName())); - assertTrue("String not in exclusions?", excludedClasses.contains(Boolean.class.getName())); - assertTrue("Integer not in exclusions?", excludedClasses.contains(Double.class.getName())); - - ognlUtil.setExcludedPackageNamePatterns("fakepackage1.*,fakepackage2.*"); - internalTestOgnlUtilExclusionsImmutable(ognlUtil); - excludedPackageNamePatterns = ognlUtil.getExcludedPackageNamePatterns(); - assertNotNull("initial excluded package name patterns null?", excludedPackageNamePatterns); - assertEquals("initial excluded package name patterns size not 2 after adds?", 2, excludedPackageNamePatterns.size()); - excludedPackageNamePatternsIterator = excludedPackageNamePatterns.iterator(); - Set patternStrings = new HashSet<>(); - while (excludedPackageNamePatternsIterator.hasNext()) { - Pattern pattern = excludedPackageNamePatternsIterator.next(); - patternStrings.add(pattern.pattern()); - } - assertTrue("fakepackage1.* not in exclusions?", patternStrings.contains("fakepackage1.*")); - assertTrue("fakepackage2.* not in exclusions?", patternStrings.contains("fakepackage2.*")); - ognlUtil.setExcludedPackageNamePatterns("fakepackage3.*,fakepackage4.*"); - internalTestOgnlUtilExclusionsImmutable(ognlUtil); - excludedPackageNamePatterns = ognlUtil.getExcludedPackageNamePatterns(); - assertNotNull("updated excluded package name patterns null?", excludedPackageNamePatterns); - assertEquals("updated excluded package name patterns size not 4 after adds?", 4, excludedPackageNamePatterns.size()); - excludedPackageNamePatternsIterator = excludedPackageNamePatterns.iterator(); - patternStrings = new HashSet<>(); - while (excludedPackageNamePatternsIterator.hasNext()) { - Pattern pattern = excludedPackageNamePatternsIterator.next(); - patternStrings.add(pattern.pattern()); - } - assertTrue("fakepackage1.* not in exclusions?", patternStrings.contains("fakepackage1.*")); - assertTrue("fakepackage2.* not in exclusions?", patternStrings.contains("fakepackage2.*")); - assertTrue("fakepackage3.* not in exclusions?", patternStrings.contains("fakepackage3.*")); - assertTrue("fakepackage4.* not in exclusions?", patternStrings.contains("fakepackage4.*")); - - ognlUtil.setExcludedPackageNames("fakepackage1.package,fakepackage2.package"); - internalTestOgnlUtilExclusionsImmutable(ognlUtil); - excludedPackageNames = ognlUtil.getExcludedPackageNames(); - assertNotNull("initial exluded package names null?", excludedPackageNames); - assertEquals("initial exluded package names not 2 after adds?", 2, excludedPackageNames.size()); - assertTrue("fakepackage1.package not in exclusions?", excludedPackageNames.contains("fakepackage1.package")); - assertTrue("fakepackage2.package not in exclusions?", excludedPackageNames.contains("fakepackage2.package")); - ognlUtil.setExcludedPackageNames("fakepackage3.package,fakepackage4.package"); - internalTestOgnlUtilExclusionsImmutable(ognlUtil); - excludedPackageNames = ognlUtil.getExcludedPackageNames(); - assertNotNull("updated exluded package names null?", excludedPackageNames); - assertEquals("updated exluded package names not 4 after adds?", 4, excludedPackageNames.size()); - assertTrue("fakepackage1.package not in exclusions?", excludedPackageNames.contains("fakepackage1.package")); - assertTrue("fakepackage2.package not in exclusions?", excludedPackageNames.contains("fakepackage2.package")); - assertTrue("fakepackage3.package not in exclusions?", excludedPackageNames.contains("fakepackage3.package")); - assertTrue("fakepackage4.package not in exclusions?", excludedPackageNames.contains("fakepackage4.package")); - } - /** * Ensure getValue: * 1) When allowStaticFieldAccess true - Permits public static field access, @@ -1626,55 +1525,6 @@ public void testApplyExpressionMaxLength() { } } - private void internalTestInitialEmptyOgnlUtilExclusions(OgnlUtil ognlUtilParam) { - Set excludedClasses = ognlUtilParam.getExcludedClasses(); - assertNotNull("parameter (default) exluded classes null?", excludedClasses); - assertTrue("parameter (default) exluded classes not empty?", excludedClasses.isEmpty()); - - Set excludedPackageNamePatterns = ognlUtilParam.getExcludedPackageNamePatterns(); - assertNotNull("parameter (default) exluded package name patterns null?", excludedPackageNamePatterns); - assertTrue("parameter (default) exluded package name patterns not empty?", excludedPackageNamePatterns.isEmpty()); - - Set excludedPackageNames = ognlUtilParam.getExcludedPackageNames(); - assertNotNull("parameter (default) exluded package names null?", excludedPackageNames); - assertTrue("parameter (default) exluded package names not empty?", excludedPackageNames.isEmpty()); - } - - private void internalTestOgnlUtilExclusionsImmutable(OgnlUtil ognlUtilParam) { - Pattern somePattern = Pattern.compile("SomeRegexPattern"); - assertImmutability(ognlUtilParam.getExcludedClasses(), Integer.class.getName()); - assertImmutability(ognlUtilParam.getExcludedPackageNamePatterns(), somePattern); - assertImmutability(ognlUtilParam.getExcludedPackageNames(), "somepackagename"); - assertImmutability(ognlUtilParam.getExcludedPackageExemptClasses(), Integer.class.getName()); - } - - public static void assertImmutability(Collection collection, T item) { - assertNotNull("Collection is null", collection); - try { - if (!collection.isEmpty()) { - collection.clear(); - fail("Collection could be cleared"); - } - } catch (UnsupportedOperationException uoe) { - // Expected failure - } - try { - collection.add(item); - fail("Collection could be added to"); - } catch (UnsupportedOperationException uoe) { - // Expected failure - } - try { - int prevSize = collection.size(); - collection.remove(item); - if (prevSize != collection.size()) { - fail("Collection could be removed from"); - } - } catch (UnsupportedOperationException uoe) { - // Expected failure - } - } - public void testAccessContext() throws Exception { Map context = ognlUtil.createDefaultContext(null); @@ -1693,107 +1543,6 @@ public void testAccessContext() throws Exception { assertSame(that, root); } - public void testSetExcludedPackageNames() { - assertThrows(ConfigurationException.class, () -> ognlUtil.setExcludedPackageNames("java.lang\njava.awt")); - assertThrows(ConfigurationException.class, () -> ognlUtil.setExcludedPackageNames("java.lang\tjava.awt")); - ConfigurationException e = assertThrows(ConfigurationException.class, () -> ognlUtil.setExcludedPackageNames("java.lang java.awt")); - assertTrue(e.getMessage().contains("erroneous whitespace characters")); - } - - public void testGetExcludedPackageNames() { - // Getter should return an immutable collection - OgnlUtil util = new OgnlUtil(); - util.setExcludedPackageNames("java.lang,java.awt"); - assertEquals(util.getExcludedPackageNames().size(), 2); - try { - util.getExcludedPackageNames().clear(); - } catch (Exception ex) { - assertTrue(ex instanceof UnsupportedOperationException); - } finally { - assertEquals(util.getExcludedPackageNames().size(), 2); - } - } - - public void testGetExcludedPackageNamesAlternateConstructorPopulated() { - // Getter should return an immutable collection - OgnlUtil util = new OgnlUtil(new DefaultOgnlExpressionCacheFactory<>(), - new DefaultOgnlBeanInfoCacheFactory<>(), - new StrutsOgnlGuard()); - util.setExcludedPackageNames("java.lang,java.awt"); - assertEquals(util.getExcludedPackageNames().size(), 2); - try { - util.getExcludedPackageNames().clear(); - } catch (Exception ex) { - assertTrue(ex instanceof UnsupportedOperationException); - } finally { - assertEquals(util.getExcludedPackageNames().size(), 2); - } - } - - public void testGetExcludedClasses() { - // Getter should return an immutable collection - OgnlUtil util = new OgnlUtil(); - util.setExcludedClasses("java.lang.Runtime,java.lang.ProcessBuilder,java.net.URL"); - assertEquals(util.getExcludedClasses().size(), 3); - try { - util.getExcludedClasses().clear(); - } catch (Exception ex) { - assertTrue(ex instanceof UnsupportedOperationException); - } finally { - assertEquals(util.getExcludedClasses().size(), 3); - } - } - - public void testGetExcludedClassesAlternateConstructorPopulated() { - // Getter should return an immutable collection - OgnlUtil util = new OgnlUtil(new DefaultOgnlExpressionCacheFactory<>(), - new DefaultOgnlBeanInfoCacheFactory<>(), - new StrutsOgnlGuard()); - util.setExcludedClasses("java.lang.Runtime,java.lang.ProcessBuilder,java.net.URL"); - assertEquals(util.getExcludedClasses().size(), 3); - try { - util.getExcludedClasses().clear(); - } catch (Exception ex) { - assertTrue(ex instanceof UnsupportedOperationException); - } finally { - assertEquals(util.getExcludedClasses().size(), 3); - } - } - - public void testGetExcludedPackageNamePatterns() { - // Getter should return an immutable collection - OgnlUtil util = new OgnlUtil(); - util.setExcludedPackageNamePatterns("java.lang."); - assertEquals(util.getExcludedPackageNamePatterns().size(), 1); - try { - util.getExcludedPackageNamePatterns().clear(); - } catch (Exception ex) { - assertTrue(ex instanceof UnsupportedOperationException); - } finally { - assertEquals(util.getExcludedPackageNamePatterns().size(), 1); - } - } - - public void testInvalidExcludedPackageNamePatterns() { - assertThrows(ConfigurationException.class, () -> ognlUtil.setExcludedPackageNamePatterns("[")); - } - - public void testGetExcludedPackageNamePatternsAlternateConstructorPopulated() { - // Getter should return an immutable collection - OgnlUtil util = new OgnlUtil(new DefaultOgnlExpressionCacheFactory<>(), - new DefaultOgnlBeanInfoCacheFactory<>(), - new StrutsOgnlGuard()); - util.setExcludedPackageNamePatterns("java.lang."); - assertEquals(util.getExcludedPackageNamePatterns().size(), 1); - try { - util.getExcludedPackageNamePatterns().clear(); - } catch (Exception ex) { - assertTrue(ex instanceof UnsupportedOperationException); - } finally { - assertEquals(util.getExcludedPackageNamePatterns().size(), 1); - } - } - public void testOgnlUtilDefaultCacheClass() throws OgnlException { OgnlDefaultCache defaultCache = new OgnlDefaultCache<>(2, 16, 0.75f); assertEquals("Initial evictionLimit did not match initial value", 2, defaultCache.getEvictionLimit()); 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 5f0ac5b264..8068dbb869 100644 --- a/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java @@ -48,7 +48,6 @@ import org.apache.struts2.StrutsConstants; import org.apache.struts2.StrutsException; import org.apache.struts2.config.DefaultPropertiesProvider; -import org.apache.struts2.config.StrutsXmlConfigurationProvider; import java.io.ByteArrayInputStream; import java.io.ByteArrayOutputStream; @@ -62,6 +61,8 @@ import java.util.List; import java.util.Map; +import static com.opensymphony.xwork2.ognl.SecurityMemberAccessTest.reflectField; + public class OgnlValueStackTest extends XWorkTestCase { // Fields for static field access test @@ -100,7 +101,6 @@ private OgnlValueStack createValueStack(boolean allowStaticFieldAccess) { (CompoundRootAccessor) container.getInstance(PropertyAccessor.class, CompoundRoot.class.getName()), container.getInstance(TextProvider.class, "system"), allowStaticFieldAccess); container.inject(stack); - ognlUtil.setAllowStaticFieldAccess(Boolean.toString(allowStaticFieldAccess)); return stack; } @@ -120,7 +120,7 @@ private OgnlValueStackFactory getValueStackFactory() { * @param allowStaticField new allowStaticField configuration * @return a new OgnlValueStackFactory with specified new configuration */ - private OgnlValueStackFactory reloadValueStackFactory(Boolean allowStaticField) { + private OgnlValueStackFactory reloadValueStackFactory(boolean allowStaticField) { try { reloadTestContainerConfiguration(allowStaticField); } catch (Exception ex) { @@ -1150,14 +1150,13 @@ public void testWarnAboutInvalidProperties() { * Test a default OgnlValueStackFactory and OgnlValueStack generated by it * when a default configuration is used. */ - public void testOgnlValueStackFromOgnlValueStackFactoryDefaultConfig() { + public void testOgnlValueStackFromOgnlValueStackFactoryDefaultConfig() throws IllegalAccessException { OgnlValueStackFactory ognlValueStackFactory = getValueStackFactory(); OgnlValueStack ognlValueStack = (OgnlValueStack) ognlValueStackFactory.createValueStack(); Object accessedValue; - // An OgnlValueStackFactory using a container config with default (from XWorkConfigurationProvider) - // static access flag values present should prevent staticMethodAccess but allow staticFieldAccess. - assertTrue("OgnlValueStackFactory staticFieldAccess (default flags) not true?", ognlValueStackFactory.containerAllowsStaticFieldAccess()); + assertTrue("OgnlValueStackFactory staticFieldAccess (default flags) not true?", + reflectField(ognlValueStack.securityMemberAccess, "allowStaticFieldAccess")); // An OgnlValueStack created from the above OgnlValueStackFactory should allow public field access, // but prevent non-public field access. It should also deny static method access. accessedValue = ognlValueStack.findValue("@com.opensymphony.xwork2.ognl.OgnlValueStackTest@staticInteger100Method()"); @@ -1180,55 +1179,17 @@ public void testOgnlValueStackFromOgnlValueStackFactoryDefaultConfig() { assertNull("accessed private field (result not null) ?", accessedValue); } - /** - * Test a raw OgnlValueStackFactory and OgnlValueStack generated by it - * when no static access flags are set (not present in configuration). - */ - public void testOgnlValueStackFromOgnlValueStackFactoryNoFlagsSet() { - OgnlValueStackFactory ognlValueStackFactory = reloadValueStackFactory(null); - OgnlValueStack ognlValueStack = (OgnlValueStack) ognlValueStackFactory.createValueStack(); - Object accessedValue; - - // An OgnlValueStackFactory using a container config with no static access flag values present - // (such as from a DefaultConfiguration vs. XWorkConfigurationProvider) should - // prevent staticMethodAccess AND prevent staticFieldAccess. - // Note: Under normal circumstances, explicit static access configuration flags should be present, - // but this specific check verifies what happens if those configuration flags are not present. - assertFalse("OgnlValueStackFactory staticFieldAccess (no flag present) not false?", ognlValueStackFactory.containerAllowsStaticFieldAccess()); - // An OgnlValueStack created from the above OgnlValueStackFactory should prevent public field access, - // and prevent non-public field access. It should also deny static method access. - accessedValue = ognlValueStack.findValue("@com.opensymphony.xwork2.ognl.OgnlValueStackTest@staticInteger100Method()"); - assertNull("able to access static method (result not null) ?", accessedValue); - accessedValue = ognlValueStack.findValue("@com.opensymphony.xwork2.ognl.OgnlValueStackTest@STATIC_FINAL_PUBLIC_ATTRIBUTE"); - assertNull("able to access static final public field (result not null) ?", accessedValue); - accessedValue = ognlValueStack.findValue("@com.opensymphony.xwork2.ognl.OgnlValueStackTest@STATIC_PUBLIC_ATTRIBUTE"); - assertNull("able to access static public field (result not null) ?", accessedValue); - accessedValue = ognlValueStack.findValue("@com.opensymphony.xwork2.ognl.OgnlValueStackTest@STATIC_FINAL_PACKAGE_ATTRIBUTE"); - assertNull("accessed final package field (result not null) ?", accessedValue); - accessedValue = ognlValueStack.findValue("@com.opensymphony.xwork2.ognl.OgnlValueStackTest@STATIC_PACKAGE_ATTRIBUTE"); - assertNull("accessed package field (result not null) ?", accessedValue); - accessedValue = ognlValueStack.findValue("@com.opensymphony.xwork2.ognl.OgnlValueStackTest@STATIC_FINAL_PROTECTED_ATTRIBUTE"); - assertNull("accessed final protected field (result not null) ?", accessedValue); - accessedValue = ognlValueStack.findValue("@com.opensymphony.xwork2.ognl.OgnlValueStackTest@STATIC_PROTECTED_ATTRIBUTE"); - assertNull("accessed protected field (result not null) ?", accessedValue); - accessedValue = ognlValueStack.findValue("@com.opensymphony.xwork2.ognl.OgnlValueStackTest@STATIC_FINAL_PRIVATE_ATTRIBUTE"); - assertNull("accessed final private field (result not null) ?", accessedValue); - accessedValue = ognlValueStack.findValue("@com.opensymphony.xwork2.ognl.OgnlValueStackTest@STATIC_PRIVATE_ATTRIBUTE"); - assertNull("accessed private field (result not null) ?", accessedValue); - } - /** * Test a raw OgnlValueStackFactory and OgnlValueStack generated by it * when static access flag is set to false. */ - public void testOgnlValueStackFromOgnlValueStackFactoryNoStaticAccess() { + public void testOgnlValueStackFromOgnlValueStackFactoryNoStaticAccess() throws IllegalAccessException { OgnlValueStackFactory ognlValueStackFactory = reloadValueStackFactory(false); OgnlValueStack ognlValueStack = (OgnlValueStack) ognlValueStackFactory.createValueStack(); Object accessedValue; - // An OgnlValueStackFactory using a container config with both static access flags set false should - // prevent staticMethodAccess AND prevent staticFieldAccess. - assertFalse("OgnlValueStackFactory staticFieldAccess (set false) not false?", ognlValueStackFactory.containerAllowsStaticFieldAccess()); + assertFalse("OgnlValueStackFactory staticFieldAccess (set false) not false?", + reflectField(ognlValueStack.securityMemberAccess, "allowStaticFieldAccess")); // An OgnlValueStack created from the above OgnlValueStackFactory should prevent public field access, // and prevent non-public field access. It should also deny static method access. accessedValue = ognlValueStack.findValue("@com.opensymphony.xwork2.ognl.OgnlValueStackTest@staticInteger100Method()"); @@ -1255,14 +1216,13 @@ public void testOgnlValueStackFromOgnlValueStackFactoryNoStaticAccess() { * Test a raw OgnlValueStackFactory and OgnlValueStack generated by it * when static access flag is set to true. */ - public void testOgnlValueStackFromOgnlValueStackFactoryAllStaticAccess() { + public void testOgnlValueStackFromOgnlValueStackFactoryAllStaticAccess() throws IllegalAccessException { OgnlValueStackFactory ognlValueStackFactory = reloadValueStackFactory(true); OgnlValueStack ognlValueStack = (OgnlValueStack) ognlValueStackFactory.createValueStack(); Object accessedValue; - // An OgnlValueStackFactory using a container config with both static access flags set true should - // allow both staticMethodAccess AND staticFieldAccess. - assertTrue("OgnlValueStackFactory staticFieldAccess (set true) not true?", ognlValueStackFactory.containerAllowsStaticFieldAccess()); + assertTrue("OgnlValueStackFactory staticFieldAccess (set true) not true?", + reflectField(ognlValueStack.securityMemberAccess, "allowStaticFieldAccess")); // An OgnlValueStack created from the above OgnlValueStackFactory should allow public field access, // but prevent non-public field access. It should also allow static method access. accessedValue = ognlValueStack.findValue("@com.opensymphony.xwork2.ognl.OgnlValueStackTest@staticInteger100Method()"); @@ -1285,17 +1245,12 @@ public void testOgnlValueStackFromOgnlValueStackFactoryAllStaticAccess() { assertNull("accessed private field (result not null) ?", accessedValue); } - private void reloadTestContainerConfiguration(Boolean allowStaticField) throws Exception { + private void reloadTestContainerConfiguration(boolean allowStaticField) throws Exception { loadConfigurationProviders(new StubConfigurationProvider() { @Override public void register(ContainerBuilder builder, LocatableProperties props) throws ConfigurationException { - // null values simulate undefined (by removing). - // undefined values then should be evaluated to false - props.remove(StrutsConstants.STRUTS_ALLOW_STATIC_FIELD_ACCESS); - if (allowStaticField != null) { - props.setProperty(StrutsConstants.STRUTS_ALLOW_STATIC_FIELD_ACCESS, "" + allowStaticField); - } + props.setProperty(StrutsConstants.STRUTS_ALLOW_STATIC_FIELD_ACCESS, String.valueOf(allowStaticField)); } }); ognlUtil = container.getInstance(OgnlUtil.class); 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 08a3b919ea..3549ed27f8 100644 --- a/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java @@ -19,8 +19,11 @@ package com.opensymphony.xwork2.ognl; import com.opensymphony.xwork2.TestBean; +import com.opensymphony.xwork2.config.ConfigurationException; import com.opensymphony.xwork2.test.TestBean2; -import com.opensymphony.xwork2.util.TextParseUtil; +import com.opensymphony.xwork2.util.Foo; +import ognl.MemberAccess; +import org.apache.commons.lang3.reflect.FieldUtils; import org.junit.Before; import org.junit.Test; @@ -28,16 +31,17 @@ import java.lang.reflect.Member; import java.lang.reflect.Method; import java.util.Arrays; +import java.util.Collection; import java.util.HashMap; import java.util.HashSet; +import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Set; -import java.util.regex.Pattern; -import static java.util.Arrays.asList; -import static java.util.Collections.singletonList; +import static org.assertj.core.api.Assertions.assertThat; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; @@ -58,6 +62,87 @@ protected void assignNewSma(boolean allowStaticFieldAccess) { sma = new SecurityMemberAccess(allowStaticFieldAccess); } + private T reflectField(String fieldName) throws IllegalAccessException { + return reflectField(sma, fieldName); + } + + public static T reflectField(Object instance, String fieldName) throws IllegalAccessException { + return (T) FieldUtils.readField(instance, fieldName, true); + } + + @Test + public void defaultExclusionList() throws Exception { + Set excludedClasses = reflectField("excludedClasses"); + assertThat(excludedClasses).containsExactly(Object.class.getName()); + + assignNewSma(false); + excludedClasses = reflectField("excludedClasses"); + assertThat(excludedClasses).containsExactlyInAnyOrder(Object.class.getName(), Class.class.getName()); + } + + @Test + public void configurationCollectionsImmutable() throws Exception { + List fields = Arrays.asList( + "excludedClasses", + "excludedPackageNames", + "excludedPackageNamePatterns", + "excludedPackageExemptClasses", + "allowlistClasses", + "allowlistPackageNames", + "excludeProperties", + "acceptProperties"); + for (String field : fields) { + Collection fieldVal = reflectField(field); + assertThrows(UnsupportedOperationException.class, () -> fieldVal.add("foo")); + if (!fieldVal.isEmpty()) { + String firstVal = fieldVal.iterator().next(); + assertThrows(UnsupportedOperationException.class, () -> fieldVal.remove(firstVal)); + assertThrows(UnsupportedOperationException.class, fieldVal::clear); + } + } + } + + @Test + public void exclusionListsAreAdditive_classes() throws Exception { + Collection fieldVal = reflectField("excludedClasses"); + Set existing = new HashSet<>(fieldVal); + + Collection newExcludedClasses = Arrays.asList(FooBar.class.getName(), String.class.getName()); + sma.useExcludedClasses(String.join(",", newExcludedClasses)); + existing.addAll(newExcludedClasses); + + fieldVal = reflectField("excludedClasses"); + assertThat(fieldVal).containsExactlyInAnyOrderElementsOf(existing); + } + + @Test + public void exclusionListsAreAdditive_packages() throws Exception { + sma.useExcludedPackageNames(Foo.class.getPackage().getName()); + Collection fieldVal = reflectField("excludedPackageNames"); + Set existing = new HashSet<>(fieldVal); + + Collection newExcludedPackages = Arrays.asList(FooBar.class.getPackage().getName(), String.class.getPackage().getName()); + sma.useExcludedPackageNames(String.join(",", newExcludedPackages)); + existing.addAll(newExcludedPackages); + + fieldVal = reflectField("excludedPackageNames"); + assertThat(fieldVal).containsExactlyInAnyOrderElementsOf(existing); + } + + @Test + public void useExcludedPackageNames() { + assertThrows(ConfigurationException.class, () -> sma.useExcludedPackageNames("java.lang\njava.awt")); + assertThrows(ConfigurationException.class, () -> sma.useExcludedPackageNames("java.lang\tjava.awt")); + ConfigurationException e = assertThrows(ConfigurationException.class, () -> sma.useExcludedPackageNames("java.lang java.awt")); + assertTrue(e.getMessage().contains("erroneous whitespace characters")); + } + + @Test + public void useExcludedPackagePatterns() { + ConfigurationException e = assertThrows(ConfigurationException.class, () -> sma.useExcludedPackageNamePatterns("[")); + assertTrue(e.getMessage().contains("invalid regex")); + } + @Test public void testWithoutClassExclusion() throws Exception { // given @@ -77,9 +162,7 @@ public void testClassExclusion() throws Exception { String propertyName = "stringField"; Member member = FooBar.class.getDeclaredMethod(formGetterName(propertyName)); - Set excluded = new HashSet<>(); - excluded.add(FooBar.class.getName()); - sma.useExcludedClasses(excluded); + sma.useExcludedClasses(FooBar.class.getName()); // when boolean accessible = sma.isAccessible(context, target, member, propertyName); @@ -120,9 +203,7 @@ public void testInterfaceInheritanceExclusion() throws Exception { String propertyName = "barLogic"; Member member = BarInterface.class.getMethod(propertyName); - Set excluded = new HashSet<>(); - excluded.add(BarInterface.class.getName()); - sma.useExcludedClasses(excluded); + sma.useExcludedClasses(BarInterface.class.getName()); // when boolean accessible = sma.isAccessible(context, target, member, propertyName); @@ -137,9 +218,7 @@ public void testMiddleOfInheritanceExclusion1() throws Exception { String propertyName = "fooLogic"; Member member = FooBar.class.getMethod(propertyName); - Set excluded = new HashSet<>(); - excluded.add(BarInterface.class.getName()); - sma.useExcludedClasses(excluded); + sma.useExcludedClasses(BarInterface.class.getName()); // when boolean accessible = sma.isAccessible(context, target, member, propertyName); @@ -154,9 +233,7 @@ public void testMiddleOfInheritanceExclusion2() throws Exception { String propertyName = "barLogic"; Member member = BarInterface.class.getMethod(propertyName); - Set excluded = new HashSet<>(); - excluded.add(BarInterface.class.getName()); - sma.useExcludedClasses(excluded); + sma.useExcludedClasses(BarInterface.class.getName()); // when boolean accessible = sma.isAccessible(context, target, member, propertyName); @@ -171,9 +248,7 @@ public void testMiddleOfInheritanceExclusion3() throws Exception { String propertyName = "barLogic"; Member member = BarInterface.class.getMethod(propertyName); - Set excluded = new HashSet<>(); - excluded.add(FooInterface.class.getName()); - sma.useExcludedClasses(excluded); + sma.useExcludedClasses(FooInterface.class.getName()); // when boolean accessible = sma.isAccessible(context, target, member, propertyName); @@ -185,9 +260,7 @@ public void testMiddleOfInheritanceExclusion3() throws Exception { @Test public void testPackageExclusion() throws Exception { // given - Set excluded = new HashSet<>(); - excluded.add(Pattern.compile("^" + FooBar.class.getPackage().getName().replaceAll("\\.", "\\\\.") + ".*")); - sma.useExcludedPackageNamePatterns(excluded); + sma.useExcludedPackageNamePatterns("^" + FooBar.class.getPackage().getName().replaceAll("\\.", "\\\\.") + ".*"); String propertyName = "stringField"; Member member = FooBar.class.getMethod(formGetterName(propertyName)); @@ -202,13 +275,9 @@ public void testPackageExclusion() throws Exception { @Test public void testPackageExclusionExemption() throws Exception { // given - Set excluded = new HashSet<>(); - excluded.add(Pattern.compile("^" + FooBar.class.getPackage().getName().replaceAll("\\.", "\\\\.") + ".*")); - sma.useExcludedPackageNamePatterns(excluded); + sma.useExcludedPackageNamePatterns("^" + FooBar.class.getPackage().getName().replaceAll("\\.", "\\\\.") + ".*"); - Set allowed = new HashSet<>(); - allowed.add(FooBar.class.getName()); - sma.useExcludedPackageExemptClasses(allowed); + sma.useExcludedPackageExemptClasses(FooBar.class.getName()); String propertyName = "stringField"; Member member = FooBar.class.getMethod(formGetterName(propertyName)); @@ -223,9 +292,7 @@ public void testPackageExclusionExemption() throws Exception { @Test public void testPackageNameExclusion() throws Exception { // given - Set excluded = new HashSet<>(); - excluded.add(FooBar.class.getPackage().getName()); - sma.useExcludedPackageNames(excluded); + sma.useExcludedPackageNames(FooBar.class.getPackage().getName()); String propertyName = "stringField"; Member member = FooBar.class.getMethod(formGetterName(propertyName)); @@ -240,13 +307,9 @@ public void testPackageNameExclusion() throws Exception { @Test public void testPackageNameExclusionExemption() throws Exception { // given - Set excluded = new HashSet<>(); - excluded.add(FooBar.class.getPackage().getName()); - sma.useExcludedPackageNames(excluded); + sma.useExcludedPackageNames(FooBar.class.getPackage().getName()); - Set allowed = new HashSet<>(); - allowed.add(FooBar.class.getName()); - sma.useExcludedPackageExemptClasses(allowed); + sma.useExcludedPackageExemptClasses(FooBar.class.getName()); String propertyName = "stringField"; Member member = FooBar.class.getMethod(formGetterName(propertyName)); @@ -261,14 +324,10 @@ public void testPackageNameExclusionExemption() throws Exception { @Test public void testPackageNameExclusionExemption2() throws Exception { // given - Set excluded = new HashSet<>(); - excluded.add(FooBar.class.getPackage().getName()); - sma.useExcludedPackageNames(excluded); + sma.useExcludedPackageNames(FooBar.class.getPackage().getName()); // Exemption must exist for both classes (target and member) if they both match a banned package - Set allowed = new HashSet<>(); - allowed.add(BarInterface.class.getName()); - sma.useExcludedPackageExemptClasses(allowed); + sma.useExcludedPackageExemptClasses(BarInterface.class.getName()); String propertyName = "barLogic"; Member member = BarInterface.class.getMethod(propertyName); @@ -283,15 +342,10 @@ public void testPackageNameExclusionExemption2() throws Exception { @Test public void testPackageNameExclusionExemption3() throws Exception { // given - Set excluded = new HashSet<>(); - excluded.add(FooBar.class.getPackage().getName()); - sma.useExcludedPackageNames(excluded); + sma.useExcludedPackageNames(FooBar.class.getPackage().getName()); // Exemption must exist for both classes (target and member) if they both match a banned package - Set allowed = new HashSet<>(); - allowed.add(BarInterface.class.getName()); - allowed.add(FooBar.class.getName()); - sma.useExcludedPackageExemptClasses(allowed); + sma.useExcludedPackageExemptClasses(String.join(",", BarInterface.class.getName(), FooBar.class.getName())); String propertyName = "barLogic"; Member member = BarInterface.class.getMethod(propertyName); @@ -306,9 +360,7 @@ public void testPackageNameExclusionExemption3() throws Exception { @Test public void testDefaultPackageExclusion() throws Exception { // given - Set excluded = new HashSet<>(); - excluded.add(Pattern.compile("^" + FooBar.class.getPackage().getName().replaceAll("\\.", "\\\\.") + ".*")); - sma.useExcludedPackageNamePatterns(excluded); + sma.useExcludedPackageNamePatterns("^" + FooBar.class.getPackage().getName().replaceAll("\\.", "\\\\.") + ".*"); Class clazz = Class.forName("PackagelessAction"); @@ -321,7 +373,7 @@ public void testDefaultPackageExclusion() throws Exception { @Test public void testDefaultPackageExclusionSetting() throws Exception { - sma.disallowDefaultPackageAccess(true); + sma.useDisallowDefaultPackageAccess(Boolean.TRUE.toString()); Class clazz = Class.forName("PackagelessAction"); boolean actual = sma.isAccessible(null, clazz.getConstructor().newInstance(), clazz.getMethod("execute"), null); @@ -332,9 +384,7 @@ public void testDefaultPackageExclusionSetting() throws Exception { @Test public void testDefaultPackageExclusion2() throws Exception { // given - Set excluded = new HashSet<>(); - excluded.add(Pattern.compile("^$")); - sma.useExcludedPackageNamePatterns(excluded); + sma.useExcludedPackageNamePatterns("^$"); Class clazz = Class.forName("PackagelessAction"); @@ -368,7 +418,7 @@ public void testAccessEnum_alternateValues() throws Exception { @Test public void testAccessStaticMethod() throws Exception { // given - sma.useExcludedClasses(new HashSet<>(singletonList(Class.class.getName()))); + sma.useExcludedClasses(Class.class.getName()); // when Member method = StaticTester.class.getMethod("sayHello"); @@ -381,7 +431,7 @@ public void testAccessStaticMethod() throws Exception { @Test public void testAccessStaticField() throws Exception { // given - sma.useExcludedClasses(new HashSet<>(singletonList(Class.class.getName()))); + sma.useExcludedClasses(Class.class.getName()); // when Member method = StaticTester.class.getField("MAX_VALUE"); @@ -395,7 +445,7 @@ public void testAccessStaticField() throws Exception { public void testBlockedStaticFieldWhenFlagIsTrue() throws Exception { // given assignNewSma(true); - sma.useExcludedClasses(new HashSet<>(singletonList(Class.class.getName()))); + sma.useExcludedClasses(Class.class.getName()); // when Member method = StaticTester.class.getField("MAX_VALUE"); @@ -407,7 +457,7 @@ public void testBlockedStaticFieldWhenFlagIsTrue() throws Exception { // public static final test // given assignNewSma(true); - sma.useExcludedClasses(new HashSet<>(singletonList(Class.class.getName()))); + sma.useExcludedClasses(Class.class.getName()); // when method = StaticTester.class.getField("MIN_VALUE"); @@ -419,7 +469,7 @@ public void testBlockedStaticFieldWhenFlagIsTrue() throws Exception { // package static test // given assignNewSma(true); - sma.useExcludedClasses(new HashSet<>(singletonList(Class.class.getName()))); + sma.useExcludedClasses(Class.class.getName()); // when method = StaticTester.getFieldByName("PACKAGE_STRING"); @@ -431,7 +481,7 @@ public void testBlockedStaticFieldWhenFlagIsTrue() throws Exception { // package final static test // given assignNewSma(true); - sma.useExcludedClasses(new HashSet<>(singletonList(Class.class.getName()))); + sma.useExcludedClasses(Class.class.getName()); // when method = StaticTester.getFieldByName("FINAL_PACKAGE_STRING"); @@ -443,7 +493,7 @@ public void testBlockedStaticFieldWhenFlagIsTrue() throws Exception { // protected static test // given assignNewSma(true); - sma.useExcludedClasses(new HashSet<>(singletonList(Class.class.getName()))); + sma.useExcludedClasses(Class.class.getName()); // when method = StaticTester.getFieldByName("PROTECTED_STRING"); @@ -455,7 +505,7 @@ public void testBlockedStaticFieldWhenFlagIsTrue() throws Exception { // protected final static test // given assignNewSma(true); - sma.useExcludedClasses(new HashSet<>(singletonList(Class.class.getName()))); + sma.useExcludedClasses(Class.class.getName()); // when method = StaticTester.getFieldByName("FINAL_PROTECTED_STRING"); @@ -467,7 +517,7 @@ public void testBlockedStaticFieldWhenFlagIsTrue() throws Exception { // private static test // given assignNewSma(true); - sma.useExcludedClasses(new HashSet<>(singletonList(Class.class.getName()))); + sma.useExcludedClasses(Class.class.getName()); // when method = StaticTester.getFieldByName("PRIVATE_STRING"); @@ -479,7 +529,7 @@ public void testBlockedStaticFieldWhenFlagIsTrue() throws Exception { // private final static test // given assignNewSma(true); - sma.useExcludedClasses(new HashSet<>(singletonList(Class.class.getName()))); + sma.useExcludedClasses(Class.class.getName()); // when method = StaticTester.getFieldByName("FINAL_PRIVATE_STRING"); @@ -582,7 +632,7 @@ public void testBlockedStaticFieldWhenFlagIsFalse() throws Exception { @Test public void testBlockedStaticFieldWhenClassIsExcluded() throws Exception { // given - sma.useExcludedClasses(new HashSet<>(Arrays.asList(Class.class.getName(), StaticTester.class.getName()))); + sma.useExcludedClasses(String.join(",", Class.class.getName(), StaticTester.class.getName())); // when Member method = StaticTester.class.getField("MAX_VALUE"); @@ -595,7 +645,7 @@ public void testBlockedStaticFieldWhenClassIsExcluded() throws Exception { @Test public void testBlockStaticMethodAccess() throws Exception { // given - sma.useExcludedClasses(new HashSet<>(singletonList(Class.class.getName()))); + sma.useExcludedClasses(Class.class.getName()); // when Member method = StaticTester.class.getMethod("sayHello"); @@ -608,7 +658,7 @@ public void testBlockStaticMethodAccess() throws Exception { @Test public void testBlockAccessIfClassIsExcluded() throws Exception { // given - sma.useExcludedClasses(new HashSet<>(singletonList(Class.class.getName()))); + sma.useExcludedClasses(Class.class.getName()); // when Member method = Class.class.getMethod("getClassLoader"); @@ -621,7 +671,7 @@ public void testBlockAccessIfClassIsExcluded() throws Exception { @Test public void testBlockAccessIfClassIsExcluded_2() throws Exception { // given - sma.useExcludedClasses(new HashSet<>(singletonList(ClassLoader.class.getName()))); + sma.useExcludedClasses(ClassLoader.class.getName()); // when Member method = ClassLoader.class.getMethod("loadClass", String.class); @@ -635,7 +685,7 @@ public void testBlockAccessIfClassIsExcluded_2() throws Exception { @Test public void testAllowAccessIfClassIsNotExcluded() throws Exception { // given - sma.useExcludedClasses(new HashSet<>(singletonList(ClassLoader.class.getName()))); + sma.useExcludedClasses(ClassLoader.class.getName()); // when Member method = Class.class.getMethod("getClassLoader"); @@ -648,7 +698,7 @@ public void testAllowAccessIfClassIsNotExcluded() throws Exception { @Test public void testIllegalArgumentExceptionExpectedForTargetMemberMismatch() throws Exception { // given - sma.useExcludedClasses(new HashSet<>(singletonList(Class.class.getName()))); + sma.useExcludedClasses(Class.class.getName()); // when Member method = ClassLoader.class.getMethod("loadClass", String.class); @@ -667,7 +717,7 @@ public void testIllegalArgumentExceptionExpectedForTargetMemberMismatch() throws @Test public void testAccessPrimitiveInt() throws Exception { // given - sma.useExcludedPackageNames(TextParseUtil.commaDelimitedStringToSet("java.lang.,ognl,javax")); + sma.useExcludedPackageNames("java.lang.,ognl,javax"); String propertyName = "intField"; Member member = FooBar.class.getMethod(formGetterName(propertyName)); @@ -682,7 +732,7 @@ public void testAccessPrimitiveInt() throws Exception { @Test public void testAccessPrimitiveDoubleWithNames() throws Exception { // given - sma.useExcludedPackageNames(TextParseUtil.commaDelimitedStringToSet("ognl.,javax.")); + sma.useExcludedPackageNames("ognl.,javax."); Set excluded = new HashSet<>(); @@ -691,7 +741,7 @@ public void testAccessPrimitiveDoubleWithNames() throws Exception { excluded.add(System.class.getName()); excluded.add(Class.class.getName()); excluded.add(ClassLoader.class.getName()); - sma.useExcludedClasses(excluded); + sma.useExcludedClasses(String.join(",", excluded)); String propertyName = "doubleValue"; double myDouble = 1; @@ -735,9 +785,7 @@ public void testAccessPrimitiveDoubleWithNames() throws Exception { @Test public void testAccessPrimitiveDoubleWithPackageRegExs() throws Exception { // given - Set patterns = new HashSet<>(); - patterns.add(Pattern.compile("^java\\.lang\\..*")); - sma.useExcludedPackageNamePatterns(patterns); + sma.useExcludedPackageNamePatterns("^java\\.lang\\..*"); String propertyName = "doubleValue"; double myDouble = 1; @@ -753,13 +801,11 @@ public void testAccessPrimitiveDoubleWithPackageRegExs() throws Exception { @Test public void testAccessMemberAccessIsAccessible() throws Exception { // given - Set excluded = new HashSet<>(); - excluded.add(ognl.MemberAccess.class.getName()); - sma.useExcludedClasses(excluded); + sma.useExcludedClasses(MemberAccess.class.getName()); String propertyName = "excludedClasses"; - String setter = "setExcludedClasses"; - Member member = SecurityMemberAccess.class.getMethod(setter, Set.class); + String setter = "useExcludedClasses"; + Member member = SecurityMemberAccess.class.getMethod(setter, String.class); // when boolean accessible = sma.isAccessible(context, sma, member, propertyName); @@ -771,13 +817,11 @@ public void testAccessMemberAccessIsAccessible() throws Exception { @Test public void testAccessMemberAccessIsBlocked() throws Exception { // given - Set excluded = new HashSet<>(); - excluded.add(SecurityMemberAccess.class.getName()); - sma.useExcludedClasses(excluded); + sma.useExcludedClasses(SecurityMemberAccess.class.getName()); String propertyName = "excludedClasses"; - String setter = "setExcludedClasses"; - Member member = SecurityMemberAccess.class.getMethod(setter, Set.class); + String setter = "useExcludedClasses"; + Member member = SecurityMemberAccess.class.getMethod(setter, String.class); // when boolean accessible = sma.isAccessible(context, sma, member, propertyName); @@ -789,7 +833,7 @@ public void testAccessMemberAccessIsBlocked() throws Exception { @Test public void testPackageNameExclusionAsCommaDelimited() { // given - sma.useExcludedPackageNames(TextParseUtil.commaDelimitedStringToSet("java.lang")); + sma.useExcludedPackageNames("java.lang"); // when boolean actual = sma.isPackageExcluded(String.class); @@ -801,36 +845,36 @@ public void testPackageNameExclusionAsCommaDelimited() { @Test public void classInclusion() throws Exception { - sma.useEnforceAllowlistEnabled(true); + sma.useEnforceAllowlistEnabled(Boolean.TRUE.toString()); TestBean2 bean = new TestBean2(); Method method = TestBean2.class.getMethod("getData"); assertFalse(sma.checkAllowlist(bean, method)); - sma.useAllowlistClasses(new HashSet<>(singletonList(TestBean2.class.getName()))); + sma.useAllowlistClasses(TestBean2.class.getName()); assertTrue(sma.checkAllowlist(bean, method)); } @Test public void packageInclusion() throws Exception { - sma.useEnforceAllowlistEnabled(true); + sma.useEnforceAllowlistEnabled(Boolean.TRUE.toString()); TestBean2 bean = new TestBean2(); Method method = TestBean2.class.getMethod("getData"); assertFalse(sma.checkAllowlist(bean, method)); - sma.useAllowlistPackageNames(new HashSet<>(singletonList(TestBean2.class.getPackage().getName()))); + sma.useAllowlistPackageNames(TestBean2.class.getPackage().getName()); assertTrue(sma.checkAllowlist(bean, method)); } @Test public void classInclusion_subclass() throws Exception { - sma.useEnforceAllowlistEnabled(true); - sma.useAllowlistClasses(new HashSet<>(singletonList(TestBean2.class.getName()))); + sma.useEnforceAllowlistEnabled(Boolean.TRUE.toString()); + sma.useAllowlistClasses(TestBean2.class.getName()); TestBean2 bean = new TestBean2(); Method method = TestBean2.class.getMethod("getName"); @@ -840,8 +884,8 @@ public void classInclusion_subclass() throws Exception { @Test public void classInclusion_subclass_both() throws Exception { - sma.useEnforceAllowlistEnabled(true); - sma.useAllowlistClasses(new HashSet<>(asList(TestBean.class.getName(), TestBean2.class.getName()))); + sma.useEnforceAllowlistEnabled(Boolean.TRUE.toString()); + sma.useAllowlistClasses(String.join(",", TestBean.class.getName(), TestBean2.class.getName())); TestBean2 bean = new TestBean2(); Method method = TestBean2.class.getMethod("getName"); @@ -851,8 +895,8 @@ public void classInclusion_subclass_both() throws Exception { @Test public void packageInclusion_subclass() throws Exception { - sma.useEnforceAllowlistEnabled(true); - sma.useAllowlistPackageNames(new HashSet<>(singletonList(TestBean2.class.getPackage().getName()))); + sma.useEnforceAllowlistEnabled(Boolean.TRUE.toString()); + sma.useAllowlistPackageNames(TestBean2.class.getPackage().getName()); TestBean2 bean = new TestBean2(); Method method = TestBean2.class.getMethod("getName"); @@ -862,8 +906,10 @@ public void packageInclusion_subclass() throws Exception { @Test public void packageInclusion_subclass_both() throws Exception { - sma.useEnforceAllowlistEnabled(true); - sma.useAllowlistPackageNames(new HashSet<>(asList(TestBean.class.getPackage().getName(), TestBean2.class.getPackage().getName()))); + sma.useEnforceAllowlistEnabled(Boolean.TRUE.toString()); + sma.useAllowlistPackageNames(String.join(",", + TestBean.class.getPackage().getName(), + TestBean2.class.getPackage().getName())); TestBean2 bean = new TestBean2(); Method method = TestBean2.class.getMethod("getName"); 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 6fcfd9f1ec..e67ef535eb 100644 --- a/core/src/test/java/org/apache/struts2/util/SecurityMemberAccessInServletsTest.java +++ b/core/src/test/java/org/apache/struts2/util/SecurityMemberAccessInServletsTest.java @@ -25,10 +25,7 @@ import javax.servlet.jsp.tagext.TagSupport; import java.lang.reflect.Member; import java.util.HashMap; -import java.util.HashSet; import java.util.Map; -import java.util.Set; -import java.util.regex.Pattern; public class SecurityMemberAccessInServletsTest extends StrutsInternalTestCase { @@ -43,9 +40,7 @@ public void testJavaxServletPackageAccess() throws Exception { // given SecurityMemberAccess sma = new SecurityMemberAccess(true); - Set excluded = new HashSet(); - excluded.add(Pattern.compile("^(?!javax\\.servlet\\..+)(javax\\..+)")); - sma.useExcludedPackageNamePatterns(excluded); + sma.useExcludedPackageNamePatterns("^(?!javax\\.servlet\\..+)(javax\\..+)"); String propertyName = "value"; Member member = TagSupport.class.getMethod("doStartTag"); @@ -61,9 +56,7 @@ public void testJavaxServletPackageExclusion() throws Exception { // given SecurityMemberAccess sma = new SecurityMemberAccess(true); - Set excluded = new HashSet<>(); - excluded.add(Pattern.compile("^javax\\..+")); - sma.useExcludedPackageNamePatterns(excluded); + sma.useExcludedPackageNamePatterns("^javax\\..+"); String propertyName = "value"; Member member = TagSupport.class.getMethod("doStartTag"); diff --git a/plugins/spring/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessProxyTest.java b/plugins/spring/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessProxyTest.java index 61282f3d1c..4d8046de9f 100644 --- a/plugins/spring/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessProxyTest.java +++ b/plugins/spring/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessProxyTest.java @@ -46,7 +46,7 @@ public void testProxyAccessIsBlocked() throws Exception { "chaintoAOPedTestSubBeanAction", null, context); SecurityMemberAccess sma = new SecurityMemberAccess(true); - sma.disallowProxyMemberAccess(true); + sma.useDisallowProxyMemberAccess(Boolean.TRUE.toString()); Member member = proxy.getAction().getClass().getMethod("isExposeProxy"); diff --git a/plugins/spring/src/test/java/com/test/SecurityMemberAccessProxyTest.java b/plugins/spring/src/test/java/com/test/SecurityMemberAccessProxyTest.java index ef53fee704..a6ad274c3e 100644 --- a/plugins/spring/src/test/java/com/test/SecurityMemberAccessProxyTest.java +++ b/plugins/spring/src/test/java/com/test/SecurityMemberAccessProxyTest.java @@ -18,52 +18,5 @@ */ package com.test; -import com.opensymphony.xwork2.ActionProxy; -import com.opensymphony.xwork2.XWorkTestCase; -import com.opensymphony.xwork2.config.providers.XmlConfigurationProvider; -import com.opensymphony.xwork2.ognl.SecurityMemberAccess; -import org.apache.struts2.config.StrutsXmlConfigurationProvider; - -import java.lang.reflect.Member; -import java.util.HashMap; -import java.util.Map; - -public class SecurityMemberAccessProxyTest extends XWorkTestCase { - private Map context; - - @Override - public void setUp() throws Exception { - super.setUp(); - - context = new HashMap<>(); - // Set up XWork - XmlConfigurationProvider provider = new StrutsXmlConfigurationProvider("com/opensymphony/xwork2/spring/actionContext-xwork.xml"); - container.inject(provider); - loadConfigurationProviders(provider); - } - - public void testProxyAccessIsBlocked() throws Exception { - ActionProxy proxy = actionProxyFactory.createActionProxy(null, - "chaintoAOPedTestSubBeanAction", null, context); - - SecurityMemberAccess sma = new SecurityMemberAccess(true); - sma.disallowProxyMemberAccess(true); - - Member member = proxy.getAction().getClass().getMethod("isExposeProxy"); - - boolean accessible = sma.isAccessible(context, proxy.getAction(), member, ""); - assertFalse(accessible); - } - - public void testProxyAccessIsAccessible() throws Exception { - ActionProxy proxy = actionProxyFactory.createActionProxy(null, - "chaintoAOPedTestSubBeanAction", null, context); - - SecurityMemberAccess sma = new SecurityMemberAccess(true); - - Member member = proxy.getAction().getClass().getMethod("isExposeProxy"); - - boolean accessible = sma.isAccessible(context, proxy.getAction(), member, ""); - assertTrue(accessible); - } +public class SecurityMemberAccessProxyTest extends com.opensymphony.xwork2.ognl.SecurityMemberAccessProxyTest { }