From 79ffc86b68c94cdf8e2f7b4526d415ab1f2ec9c7 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Thu, 31 Aug 2023 23:56:02 +1000 Subject: [PATCH 01/19] WW-5343 Delete unused code and consolidate constructors --- .../xwork2/ognl/OgnlValueStack.java | 36 +++++++------- .../xwork2/util/location/LocationImpl.java | 47 ++++++++----------- 2 files changed, 35 insertions(+), 48 deletions(-) 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..69802c5c2f 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java @@ -21,7 +21,6 @@ import com.opensymphony.xwork2.ActionContext; import com.opensymphony.xwork2.TextProvider; import com.opensymphony.xwork2.conversion.impl.XWorkConverter; -import com.opensymphony.xwork2.inject.Container; import com.opensymphony.xwork2.inject.Inject; import com.opensymphony.xwork2.ognl.accessor.CompoundRootAccessor; import com.opensymphony.xwork2.util.ClearableValueStack; @@ -34,7 +33,6 @@ import ognl.Ognl; import ognl.OgnlContext; import ognl.OgnlException; -import ognl.PropertyAccessor; import org.apache.commons.lang3.BooleanUtils; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -77,13 +75,26 @@ public class OgnlValueStack implements Serializable, ValueStack, ClearableValueS private boolean devMode; private boolean logMissingProperties; + protected OgnlValueStack(ValueStack vs, + XWorkConverter xworkConverter, + CompoundRootAccessor accessor, + TextProvider prov, + boolean allowStaticFieldAccess) { + setRoot(xworkConverter, + accessor, + vs != null ? new CompoundRoot(vs.getRoot()) : new CompoundRoot(), + allowStaticFieldAccess); + if (prov != null) { + push(prov); + } + } + protected OgnlValueStack(XWorkConverter xworkConverter, CompoundRootAccessor accessor, TextProvider prov, boolean allowStaticFieldAccess) { - setRoot(xworkConverter, accessor, new CompoundRoot(), allowStaticFieldAccess); - push(prov); + this(null, xworkConverter, accessor, prov, allowStaticFieldAccess); } protected OgnlValueStack(ValueStack vs, XWorkConverter xworkConverter, CompoundRootAccessor accessor, boolean allowStaticFieldAccess) { - setRoot(xworkConverter, accessor, new CompoundRoot(vs.getRoot()), allowStaticFieldAccess); + this(vs, xworkConverter, accessor, null, allowStaticFieldAccess); } @Inject @@ -464,21 +475,6 @@ public int size() { return root.size(); } - private Object readResolve() { - // TODO: this should be done better - ActionContext ac = ActionContext.getContext(); - Container cont = ac.getContainer(); - 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); - aStack.setOgnlUtil(cont.getInstance(OgnlUtil.class)); - aStack.setRoot(xworkConverter, accessor, this.root, allowStaticField); - - return aStack; - } - public void clearContextValues() { //this is an OGNL ValueStack so the context will be an OgnlContext 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..26b3072df0 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,33 +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}. - * - * @return resolved location as object - */ - private Object readResolve() { - return this.equals(Location.UNKNOWN) ? Location.UNKNOWN : this; - } - + private boolean testEquals(Object object1, Object object2) { if (object1 == object2) { return true; From 08253299599394c15111bf0f48ce04bd4eced89d Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Wed, 15 Nov 2023 00:18:48 +1100 Subject: [PATCH 02/19] WW-5343 Extract ConfigParseUtil --- .../opensymphony/xwork2/ognl/OgnlUtil.java | 59 -------------- .../xwork2/util/ConfigParseUtil.java | 77 +++++++++++++++++++ 2 files changed, 77 insertions(+), 59 deletions(-) create mode 100644 core/src/main/java/com/opensymphony/xwork2/util/ConfigParseUtil.java 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..bbcf3bdff3 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java @@ -185,30 +185,6 @@ protected void setDevModeExcludedClasses(String commaDelimitedClasses) { devModeExcludedClasses = toNewClassesSet(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) protected void setExcludedPackageNamePatterns(String commaDelimitedPackagePatterns) { excludedPackageNamePatterns = toNewPatternsSet(excludedPackageNamePatterns, commaDelimitedPackagePatterns); @@ -219,19 +195,6 @@ protected void setDevModeExcludedPackageNamePatterns(String commaDelimitedPackag devModeExcludedPackageNamePatterns = toNewPatternsSet(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) protected void setExcludedPackageNames(String commaDelimitedPackageNames) { excludedPackageNames = toNewPackageNamesSet(excludedPackageNames, commaDelimitedPackageNames); @@ -242,28 +205,6 @@ 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); - } - - 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) public void setExcludedPackageExemptClasses(String commaDelimitedClasses) { excludedPackageExemptClasses = toClassesSet(commaDelimitedClasses); 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..3b7028db14 --- /dev/null +++ b/core/src/main/java/com/opensymphony/xwork2/util/ConfigParseUtil.java @@ -0,0 +1,77 @@ +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 { + + 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); + } + } +} From b0b80bac77fac56a019f3c4f5b8bad9e9bf42c01 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Wed, 15 Nov 2023 00:19:47 +1100 Subject: [PATCH 03/19] WW-5343 Extract deprecated methods as default interface methods --- .../com/opensymphony/xwork2/ognl/OgnlValueStack.java | 10 ---------- .../xwork2/util/MemberAccessValueStack.java | 8 ++++++-- 2 files changed, 6 insertions(+), 12 deletions(-) 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 69802c5c2f..90846f5fe2 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java @@ -482,20 +482,10 @@ public void clearContextValues() { ((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); } 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); From 9e556e9ed4a49f358c692ea955e2150842a10f3e Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Wed, 15 Nov 2023 00:20:58 +1100 Subject: [PATCH 04/19] WW-5343 Deprecate unnecessary setter --- .../java/com/opensymphony/xwork2/ognl/OgnlValueStack.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) 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 90846f5fe2..a003972d5a 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java @@ -115,6 +115,7 @@ protected void setRoot(XWorkConverter xworkConverter, CompoundRootAccessor acces this.root = compoundRoot; this.securityMemberAccess = new SecurityMemberAccess(allowStaticFieldAccess); 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); @@ -490,8 +491,11 @@ 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 } } From 90344b38108852d0f27c8eb2c52a3c2b8881b0dd Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Wed, 15 Nov 2023 00:22:36 +1100 Subject: [PATCH 05/19] WW-5343 Make SecurityMemberAccess a prototype bean --- .../xwork2/config/impl/DefaultConfiguration.java | 4 +++- .../config/providers/StrutsDefaultConfigurationProvider.java | 2 ++ .../com/opensymphony/xwork2/ognl/SecurityMemberAccess.java | 5 +++++ core/src/main/java/org/apache/struts2/StrutsConstants.java | 2 ++ .../apache/struts2/config/StrutsBeanSelectionProvider.java | 2 ++ core/src/main/resources/struts-beans.xml | 3 ++- 6 files changed, 16 insertions(+), 2 deletions(-) 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..d0cbcef1cd 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; @@ -133,7 +134,6 @@ public class DefaultConfiguration implements Configuration { 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); @@ -142,6 +142,7 @@ public class DefaultConfiguration implements Configuration { 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_ALLOW_STATIC_FIELD_ACCESS, Boolean.TRUE); BOOTSTRAP_CONSTANTS = Collections.unmodifiableMap(constants); } @@ -385,6 +386,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/providers/StrutsDefaultConfigurationProvider.java b/core/src/main/java/com/opensymphony/xwork2/config/providers/StrutsDefaultConfigurationProvider.java index 625a4fb17b..09eeb7c855 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; @@ -230,6 +231,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) 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..f5a9132938 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java @@ -62,6 +62,11 @@ public class SecurityMemberAccess implements MemberAccess { private boolean disallowProxyMemberAccess = false; private boolean disallowDefaultPackageAccess = false; + @Inject + public SecurityMemberAccess(@Inject(value = StrutsConstants.STRUTS_ALLOW_STATIC_FIELD_ACCESS) String allowStaticFieldAccess) { + this(BooleanUtils.toBoolean(allowStaticFieldAccess)); + } + /** * SecurityMemberAccess * - access decisions based on whether member is static (or not) 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/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"/> From 7e92a8d7b4c2f06a174fb9330786174abbf23b0a Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Wed, 15 Nov 2023 00:23:51 +1100 Subject: [PATCH 06/19] WW-5343 Refactor OgnlValueStackFactory to utilise SecurityMemberAccess bean --- .../xwork2/ognl/OgnlValueStack.java | 74 +++++++++++++++---- .../xwork2/ognl/OgnlValueStackFactory.java | 23 ++---- 2 files changed, 67 insertions(+), 30 deletions(-) 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 a003972d5a..63802717a0 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java @@ -75,45 +75,76 @@ 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, - boolean allowStaticFieldAccess) { + SecurityMemberAccess securityMemberAccess) { setRoot(xworkConverter, accessor, vs != null ? new CompoundRoot(vs.getRoot()) : new CompoundRoot(), - allowStaticFieldAccess); + 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) { - this(null, xworkConverter, accessor, prov, allowStaticFieldAccess); + 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) { - this(vs, xworkConverter, accessor, null, 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); @@ -121,6 +152,19 @@ protected void setRoot(XWorkConverter xworkConverter, CompoundRootAccessor acces ((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 + protected void setSecurityMemberAccess(SecurityMemberAccess securityMemberAccess) { + this.securityMemberAccess = securityMemberAccess; + } + @Inject(StrutsConstants.STRUTS_DEVMODE) protected void setDevMode(String mode) { this.devMode = BooleanUtils.toBoolean(mode); 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)); } - } From b518635e2e7b1f56d1b837611f85938fff138dbb Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Wed, 15 Nov 2023 00:24:39 +1100 Subject: [PATCH 07/19] WW-5343 Update OgnlUtil#createDefaultContext to utilise SecurityMemberAccess bean --- .../java/com/opensymphony/xwork2/ognl/OgnlUtil.java | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) 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 bbcf3bdff3..55b27b0e2f 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java @@ -863,8 +863,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()) { @@ -875,14 +874,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); From 4490d9d7727d4915fd6a8e899bc03eded2ee2afc Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Wed, 15 Nov 2023 00:25:29 +1100 Subject: [PATCH 08/19] WW-5343 Move configuration injection from OgnlUtil to SecurityMemberAccess --- .../opensymphony/xwork2/ognl/OgnlUtil.java | 148 +++++++++--------- .../xwork2/ognl/SecurityMemberAccess.java | 114 +++++--------- 2 files changed, 106 insertions(+), 156 deletions(-) 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 55b27b0e2f..62e635fbc6 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; @@ -84,27 +77,15 @@ public class OgnlUtil { private final OgnlGuard ognlGuard; private boolean devMode; - private boolean enableExpressionCache = true; + private boolean enableExpressionCache; 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 @@ -175,87 +156,84 @@ 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); } @Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_CLASSES, required = false) protected void setDevModeExcludedClasses(String commaDelimitedClasses) { - devModeExcludedClasses = toNewClassesSet(devModeExcludedClasses, commaDelimitedClasses); + this.devModeExcludedClasses = commaDelimitedClasses; } - @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); } @Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_PACKAGE_NAME_PATTERNS, required = false) protected void setDevModeExcludedPackageNamePatterns(String commaDelimitedPackagePatterns) { - devModeExcludedPackageNamePatterns = toNewPatternsSet(devModeExcludedPackageNamePatterns, commaDelimitedPackagePatterns); + this.devModeExcludedPackageNamePatterns = commaDelimitedPackagePatterns; } - @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); } @Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_PACKAGE_NAMES, required = false) protected void setDevModeExcludedPackageNames(String commaDelimitedPackageNames) { - devModeExcludedPackageNames = toNewPackageNamesSet(devModeExcludedPackageNames, commaDelimitedPackageNames); + this.devModeExcludedPackageNames = commaDelimitedPackageNames; } - @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); } @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 @@ -263,19 +241,25 @@ 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); } - @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); } - @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); } /** @@ -297,12 +281,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)); } /** 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 f5a9132938..4f56533ff6 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,14 @@ 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.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. @@ -76,7 +83,7 @@ public SecurityMemberAccess(@Inject(value = StrutsConstants.STRUTS_ALLOW_STATIC_ */ public SecurityMemberAccess(boolean allowStaticFieldAccess) { this.allowStaticFieldAccess = allowStaticFieldAccess; - useExcludedClasses(excludedClasses); // Initialise default exclusions + useExcludedClasses(""); // Initialise default exclusions } @Override @@ -361,40 +368,17 @@ 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); + @Inject(value = StrutsConstants.STRUTS_EXCLUDED_CLASSES, required = false) + protected void useExcludedClasses(String commaDelimitedClasses) { + Set newExcludedClasses = new HashSet<>(toNewClassesSet(excludedClasses, commaDelimitedClasses)); newExcludedClasses.add(Object.class.getName()); if (!allowStaticFieldAccess) { newExcludedClasses.add(Class.class.getName()); @@ -402,67 +386,41 @@ public void useExcludedClasses(Set excludedClasses) { this.excludedClasses = unmodifiableSet(newExcludedClasses); } - /** - * @deprecated please use {@link #useExcludedPackageNamePatterns(Set)} - */ - @Deprecated - public void setExcludedPackageNamePatterns(Set excludedPackageNamePatterns) { - this.excludedPackageNamePatterns = excludedPackageNamePatterns; - } - - public void useExcludedPackageNamePatterns(Set excludedPackageNamePatterns) { - this.excludedPackageNamePatterns = excludedPackageNamePatterns; - } - - /** - * @deprecated please use {@link #useExcludedPackageNames(Set)} - */ - @Deprecated - public void setExcludedPackageNames(Set excludedPackageNames) { - this.excludedPackageNames = excludedPackageNames; - } - - public void useExcludedPackageNames(Set excludedPackageNames) { - this.excludedPackageNames = excludedPackageNames; + @Inject(value = StrutsConstants.STRUTS_EXCLUDED_PACKAGE_NAME_PATTERNS, required = false) + public void useExcludedPackageNamePatterns(String commaDelimitedPackagePatterns) { + this.excludedPackageNamePatterns = toNewPatternsSet(excludedPackageNamePatterns, commaDelimitedPackagePatterns); } - - /** - * @deprecated please use {@link #useExcludedPackageExemptClasses(Set)} - */ - @Deprecated - public void setExcludedPackageExemptClasses(Set> excludedPackageExemptClasses) { - useExcludedPackageExemptClasses(excludedPackageExemptClasses.stream().map(Class::getName).collect(toSet())); - } - - public void useExcludedPackageExemptClasses(Set excludedPackageExemptClasses) { - this.excludedPackageExemptClasses = excludedPackageExemptClasses; + @Inject(value = StrutsConstants.STRUTS_EXCLUDED_PACKAGE_NAMES, required = false) + public void useExcludedPackageNames(String commaDelimitedPackageNames) { + this.excludedPackageNames = toNewPackageNamesSet(excludedPackageNames, commaDelimitedPackageNames); } - public void useEnforceAllowlistEnabled(boolean enforceAllowlistEnabled) { - this.enforceAllowlistEnabled = enforceAllowlistEnabled; + @Inject(value = StrutsConstants.STRUTS_EXCLUDED_PACKAGE_EXEMPT_CLASSES, required = false) + public void useExcludedPackageExemptClasses(String commaDelimitedClasses) { + this.excludedPackageExemptClasses = toClassesSet(commaDelimitedClasses); } - - public void useAllowlistClasses(Set allowlistClasses) { - this.allowlistClasses = allowlistClasses; + @Inject(value = StrutsConstants.STRUTS_ALLOWLIST_ENABLE, required = false) + public void useEnforceAllowlistEnabled(String enforceAllowlistEnabled) { + this.enforceAllowlistEnabled = BooleanUtils.toBoolean(enforceAllowlistEnabled); } - public void useAllowlistPackageNames(Set allowlistPackageNames) { - this.allowlistPackageNames = allowlistPackageNames; + @Inject(value = StrutsConstants.STRUTS_ALLOWLIST_CLASSES, required = false) + public void useAllowlistClasses(String commaDelimitedClasses) { + this.allowlistClasses = toClassesSet(commaDelimitedClasses); } - /** - * @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); } } From 8bf47b3679d6b66984a237eb435feaaab8d0b042 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Wed, 15 Nov 2023 00:28:46 +1100 Subject: [PATCH 09/19] WW-5343 Fix OgnlUtilTest#testBeanMapExpressions --- .../java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) 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..b1ed266a03 100644 --- a/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java @@ -889,11 +889,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 +910,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() { From 62988f783fd6ce78aefb876ac5d84fb314bc5db8 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Wed, 15 Nov 2023 00:30:12 +1100 Subject: [PATCH 10/19] WW-5343 Fix unit test compilation errors --- .../xwork2/ognl/OgnlValueStackTest.java | 2 - .../xwork2/ognl/SecurityMemberAccessTest.java | 155 +++++++----------- .../SecurityMemberAccessInServletsTest.java | 11 +- .../ognl/SecurityMemberAccessProxyTest.java | 2 +- .../test/SecurityMemberAccessProxyTest.java | 49 +----- 5 files changed, 60 insertions(+), 159 deletions(-) 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..210f7ea8bb 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; @@ -100,7 +99,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; } 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..f25ecd30bb 100644 --- a/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java @@ -20,23 +20,19 @@ import com.opensymphony.xwork2.TestBean; import com.opensymphony.xwork2.test.TestBean2; -import com.opensymphony.xwork2.util.TextParseUtil; +import ognl.MemberAccess; import org.junit.Before; import org.junit.Test; import java.lang.reflect.Field; import java.lang.reflect.Member; import java.lang.reflect.Method; -import java.util.Arrays; import java.util.HashMap; import java.util.HashSet; 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.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; @@ -77,9 +73,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 +114,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 +129,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 +144,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 +159,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 +171,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 +186,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 +203,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 +218,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 +235,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 +253,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 +271,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 +284,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 +295,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 +329,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 +342,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 +356,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 +368,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 +380,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 +392,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 +404,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 +416,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 +428,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 +440,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 +543,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 +556,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 +569,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 +582,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 +596,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 +609,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 +628,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 +643,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 +652,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 +696,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,9 +712,7 @@ 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"; @@ -771,9 +728,7 @@ 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"; @@ -789,7 +744,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 +756,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 +795,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 +806,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 +817,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 { } From ceff6cde03777f06d03aee605b4353729f77bad5 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Thu, 16 Nov 2023 23:06:13 +1100 Subject: [PATCH 11/19] WW-5343 Remove unnecessary method --- .../java/com/opensymphony/xwork2/ognl/OgnlValueStack.java | 5 ----- 1 file changed, 5 deletions(-) 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 63802717a0..57a5111981 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java @@ -160,11 +160,6 @@ protected void setRoot(XWorkConverter xworkConverter, CompoundRootAccessor acces setRoot(xworkConverter, accessor, compoundRoot, new SecurityMemberAccess(allowStaticFieldAccess)); } - @Inject - protected void setSecurityMemberAccess(SecurityMemberAccess securityMemberAccess) { - this.securityMemberAccess = securityMemberAccess; - } - @Inject(StrutsConstants.STRUTS_DEVMODE) protected void setDevMode(String mode) { this.devMode = BooleanUtils.toBoolean(mode); From f87d7d7340a53bfd7ab9179404aadc50b8e5ce9b Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Fri, 17 Nov 2023 11:38:50 +1100 Subject: [PATCH 12/19] WW-5343 Add missing license --- .../xwork2/util/ConfigParseUtil.java | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/core/src/main/java/com/opensymphony/xwork2/util/ConfigParseUtil.java b/core/src/main/java/com/opensymphony/xwork2/util/ConfigParseUtil.java index 3b7028db14..53475b7a88 100644 --- a/core/src/main/java/com/opensymphony/xwork2/util/ConfigParseUtil.java +++ b/core/src/main/java/com/opensymphony/xwork2/util/ConfigParseUtil.java @@ -1,3 +1,21 @@ +/* + * 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; From a402e5c80a4fb5d3e98b1ab857549981269468e1 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Fri, 17 Nov 2023 15:28:14 +1100 Subject: [PATCH 13/19] WW-5343 Revert and fix serializability --- .../xwork2/ognl/OgnlValueStack.java | 18 ++++++++++++++++++ .../xwork2/util/location/LocationImpl.java | 9 +++++++++ 2 files changed, 27 insertions(+) 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 57a5111981..ddcd4429a1 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java @@ -21,6 +21,7 @@ import com.opensymphony.xwork2.ActionContext; import com.opensymphony.xwork2.TextProvider; import com.opensymphony.xwork2.conversion.impl.XWorkConverter; +import com.opensymphony.xwork2.inject.Container; import com.opensymphony.xwork2.inject.Inject; import com.opensymphony.xwork2.ognl.accessor.CompoundRootAccessor; import com.opensymphony.xwork2.util.ClearableValueStack; @@ -33,6 +34,7 @@ import ognl.Ognl; import ognl.OgnlContext; import ognl.OgnlException; +import ognl.PropertyAccessor; import org.apache.commons.lang3.BooleanUtils; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -515,6 +517,22 @@ 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(); + Container cont = ac.getContainer(); + XWorkConverter xworkConverter = cont.getInstance(XWorkConverter.class); + CompoundRootAccessor accessor = (CompoundRootAccessor) cont.getInstance(PropertyAccessor.class, CompoundRoot.class.getName()); + TextProvider prov = cont.getInstance(TextProvider.class, "system"); + 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, sma); + return aStack; + } public void clearContextValues() { //this is an OGNL ValueStack so the context will be an OgnlContext 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 26b3072df0..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 @@ -202,6 +202,15 @@ public String toString() { return LocationUtils.toString(this); } + /** + * Ensure serialized unknown location resolve to {@link Location#UNKNOWN}. + * + * @return resolved location as object + */ + private Object readResolve() { + return this.equals(Location.UNKNOWN) ? Location.UNKNOWN : this; + } + private boolean testEquals(Object object1, Object object2) { if (object1 == object2) { return true; From d0d10d9389bd5d80e4ed774a84bb09452569a229 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Fri, 17 Nov 2023 15:42:20 +1100 Subject: [PATCH 14/19] WW-5343 Fix MemberAccess access blocked tests --- .../opensymphony/xwork2/ognl/SecurityMemberAccess.java | 2 +- .../xwork2/ognl/SecurityMemberAccessTest.java | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java b/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java index 4f56533ff6..c68423014c 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java @@ -377,7 +377,7 @@ public void useAcceptProperties(Set acceptedProperties) { } @Inject(value = StrutsConstants.STRUTS_EXCLUDED_CLASSES, required = false) - protected void useExcludedClasses(String commaDelimitedClasses) { + public void useExcludedClasses(String commaDelimitedClasses) { Set newExcludedClasses = new HashSet<>(toNewClassesSet(excludedClasses, commaDelimitedClasses)); newExcludedClasses.add(Object.class.getName()); if (!allowStaticFieldAccess) { 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 f25ecd30bb..70165fcde2 100644 --- a/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java @@ -715,8 +715,8 @@ public void testAccessMemberAccessIsAccessible() throws Exception { 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); @@ -731,8 +731,8 @@ public void testAccessMemberAccessIsBlocked() throws Exception { 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); From 68f5584d9fbdedd3809f3267ded6f2b230145bdb Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Fri, 17 Nov 2023 15:42:36 +1100 Subject: [PATCH 15/19] WW-5343 Remove defunct test now that constant is required --- .../xwork2/ognl/OgnlValueStackTest.java | 48 ++----------------- 1 file changed, 3 insertions(+), 45 deletions(-) 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 210f7ea8bb..42cc4b1108 100644 --- a/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java @@ -118,7 +118,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) { @@ -1178,43 +1178,6 @@ 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. @@ -1283,17 +1246,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); From 9640f5bde09d07076b82c6204ac26a6a2e80e897 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Thu, 23 Nov 2023 00:56:28 +1100 Subject: [PATCH 16/19] WW-5343 Migrate tests to SecurityMemberAccessTest --- .../xwork2/ognl/OgnlUtilStrutsTest.java | 27 -- .../xwork2/ognl/OgnlUtilTest.java | 247 ------------------ .../xwork2/ognl/SecurityMemberAccessTest.java | 84 ++++++ 3 files changed, 84 insertions(+), 274 deletions(-) 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 b1ed266a03..16f1c38506 100644 --- a/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java @@ -57,15 +57,9 @@ 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.junit.Assert.assertThrows; public class OgnlUtilTest extends XWorkTestCase { @@ -1337,18 +1331,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 { @@ -1365,85 +1347,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, @@ -1625,55 +1528,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); @@ -1692,107 +1546,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/SecurityMemberAccessTest.java b/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java index 70165fcde2..ebbc797ee1 100644 --- a/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java @@ -19,21 +19,29 @@ 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.Foo; import ognl.MemberAccess; +import org.apache.commons.lang3.reflect.FieldUtils; import org.junit.Before; import org.junit.Test; import java.lang.reflect.Field; 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 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; @@ -54,6 +62,82 @@ protected void assignNewSma(boolean allowStaticFieldAccess) { sma = new SecurityMemberAccess(allowStaticFieldAccess); } + private T reflectField(String fieldName) throws IllegalAccessException { + return (T) FieldUtils.readField(sma, 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()) { + assertThrows(UnsupportedOperationException.class, () -> fieldVal.remove(fieldVal.iterator().next())); + 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 From eba79a784436912ab3e57cb9cafba64f6eaee6d5 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Thu, 23 Nov 2023 01:09:49 +1100 Subject: [PATCH 17/19] WW-5343 Fix final test --- .../xwork2/ognl/OgnlUtilTest.java | 27 +++++++++---------- 1 file changed, 12 insertions(+), 15 deletions(-) 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 16f1c38506..5df410e2bc 100644 --- a/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java @@ -61,6 +61,9 @@ import java.util.Locale; import java.util.Map; +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.Assert.assertThrows; + public class OgnlUtilTest extends XWorkTestCase { // Fields for static field access test @@ -1174,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() { From 85d2c742cda110182a5e864751926ff0302124a9 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Sun, 26 Nov 2023 16:52:07 +1100 Subject: [PATCH 18/19] WW-5343 Clean up bootstrap constants --- .../config/impl/DefaultConfiguration.java | 5 +-- .../xwork2/config/impl/MockConfiguration.java | 2 -- .../StrutsDefaultConfigurationProvider.java | 4 --- .../opensymphony/xwork2/ognl/OgnlUtil.java | 4 +-- .../xwork2/ognl/SecurityMemberAccess.java | 31 +++++++++++-------- .../org/apache/struts2/default.properties | 7 ----- .../xwork2/ognl/OgnlValueStackTest.java | 23 +++++++------- .../xwork2/ognl/SecurityMemberAccessTest.java | 6 +++- 8 files changed, 37 insertions(+), 45 deletions(-) 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 d0cbcef1cd..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 @@ -133,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_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_ALLOW_STATIC_FIELD_ACCESS, Boolean.TRUE); + constants.put(StrutsConstants.STRUTS_ENABLE_DYNAMIC_METHOD_INVOCATION, Boolean.FALSE); BOOTSTRAP_CONSTANTS = Collections.unmodifiableMap(constants); } 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 09eeb7c855..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 @@ -114,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; @@ -257,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 62e635fbc6..de81c26855 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java @@ -77,7 +77,7 @@ public class OgnlUtil { private final OgnlGuard ognlGuard; private boolean devMode; - private boolean enableExpressionCache; + private boolean enableExpressionCache = true; private boolean enableEvalExpression; private String devModeExcludedClasses = ""; @@ -126,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); } 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 c68423014c..a5d2aa0b43 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java @@ -46,6 +46,7 @@ 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; /** @@ -56,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(); @@ -69,9 +70,7 @@ public class SecurityMemberAccess implements MemberAccess { private boolean disallowProxyMemberAccess = false; private boolean disallowDefaultPackageAccess = false; - @Inject - public SecurityMemberAccess(@Inject(value = StrutsConstants.STRUTS_ALLOW_STATIC_FIELD_ACCESS) String allowStaticFieldAccess) { - this(BooleanUtils.toBoolean(allowStaticFieldAccess)); + public SecurityMemberAccess() { } /** @@ -80,10 +79,11 @@ public SecurityMemberAccess(@Inject(value = StrutsConstants.STRUTS_ALLOW_STATIC_ * - 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(""); // Initialise default exclusions + useAllowStaticFieldAccess(String.valueOf(allowStaticFieldAccess)); } @Override @@ -376,20 +376,24 @@ public void useAcceptProperties(Set acceptedProperties) { this.acceptProperties = acceptedProperties; } + @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()); + } + } + @Inject(value = StrutsConstants.STRUTS_EXCLUDED_CLASSES, required = false) public void useExcludedClasses(String commaDelimitedClasses) { - Set newExcludedClasses = new HashSet<>(toNewClassesSet(excludedClasses, commaDelimitedClasses)); - newExcludedClasses.add(Object.class.getName()); - if (!allowStaticFieldAccess) { - newExcludedClasses.add(Class.class.getName()); - } - this.excludedClasses = unmodifiableSet(newExcludedClasses); + this.excludedClasses = toNewClassesSet(excludedClasses, commaDelimitedClasses); } @Inject(value = StrutsConstants.STRUTS_EXCLUDED_PACKAGE_NAME_PATTERNS, required = false) public void useExcludedPackageNamePatterns(String commaDelimitedPackagePatterns) { this.excludedPackageNamePatterns = toNewPatternsSet(excludedPackageNamePatterns, commaDelimitedPackagePatterns); } + @Inject(value = StrutsConstants.STRUTS_EXCLUDED_PACKAGE_NAMES, required = false) public void useExcludedPackageNames(String commaDelimitedPackageNames) { this.excludedPackageNames = toNewPackageNamesSet(excludedPackageNames, commaDelimitedPackageNames); @@ -399,6 +403,7 @@ public void useExcludedPackageNames(String commaDelimitedPackageNames) { public void useExcludedPackageExemptClasses(String commaDelimitedClasses) { this.excludedPackageExemptClasses = toClassesSet(commaDelimitedClasses); } + @Inject(value = StrutsConstants.STRUTS_ALLOWLIST_ENABLE, required = false) public void useEnforceAllowlistEnabled(String enforceAllowlistEnabled) { this.enforceAllowlistEnabled = BooleanUtils.toBoolean(enforceAllowlistEnabled); 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/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java b/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java index 42cc4b1108..8068dbb869 100644 --- a/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java @@ -61,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 @@ -1148,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()"); @@ -1182,14 +1183,13 @@ public void testOgnlValueStackFromOgnlValueStackFactoryDefaultConfig() { * 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()"); @@ -1216,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()"); 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 ebbc797ee1..7d3f04fc7e 100644 --- a/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java @@ -63,7 +63,11 @@ protected void assignNewSma(boolean allowStaticFieldAccess) { } private T reflectField(String fieldName) throws IllegalAccessException { - return (T) FieldUtils.readField(sma, fieldName, true); + return reflectField(sma, fieldName); + } + + public static T reflectField(Object instance, String fieldName) throws IllegalAccessException { + return (T) FieldUtils.readField(instance, fieldName, true); } @Test From 7929d8634c58b60709db1133e8a6d42a30c2827f Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Sun, 26 Nov 2023 17:02:34 +1100 Subject: [PATCH 19/19] WW-5343 Address SonarCloud code smells --- .../main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java | 7 +++++++ .../java/com/opensymphony/xwork2/util/ConfigParseUtil.java | 3 +++ .../opensymphony/xwork2/ognl/SecurityMemberAccessTest.java | 3 ++- 3 files changed, 12 insertions(+), 1 deletion(-) 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 de81c26855..a9e3045cc9 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java @@ -161,6 +161,7 @@ protected void setEnableEvalExpression(String evalExpression) { */ @Deprecated protected void setExcludedClasses(String commaDelimitedClasses) { + // Must be set directly on SecurityMemberAccess } @Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_CLASSES, required = false) @@ -173,6 +174,7 @@ protected void setDevModeExcludedClasses(String commaDelimitedClasses) { */ @Deprecated protected void setExcludedPackageNamePatterns(String commaDelimitedPackagePatterns) { + // Must be set directly on SecurityMemberAccess } @Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_PACKAGE_NAME_PATTERNS, required = false) @@ -185,6 +187,7 @@ protected void setDevModeExcludedPackageNamePatterns(String commaDelimitedPackag */ @Deprecated protected void setExcludedPackageNames(String commaDelimitedPackageNames) { + // Must be set directly on SecurityMemberAccess } @Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_PACKAGE_NAMES, required = false) @@ -197,6 +200,7 @@ protected void setDevModeExcludedPackageNames(String commaDelimitedPackageNames) */ @Deprecated public void setExcludedPackageExemptClasses(String commaDelimitedClasses) { + // Must be set directly on SecurityMemberAccess } @Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_PACKAGE_EXEMPT_CLASSES, required = false) @@ -246,6 +250,7 @@ protected void setContainer(Container container) { */ @Deprecated protected void setAllowStaticFieldAccess(String allowStaticFieldAccess) { + // Must be set directly on SecurityMemberAccess } /** @@ -253,6 +258,7 @@ protected void setAllowStaticFieldAccess(String allowStaticFieldAccess) { */ @Deprecated protected void setDisallowProxyMemberAccess(String disallowProxyMemberAccess) { + // Must be set directly on SecurityMemberAccess } /** @@ -260,6 +266,7 @@ protected void setDisallowProxyMemberAccess(String disallowProxyMemberAccess) { */ @Deprecated protected void setDisallowDefaultPackageAccess(String disallowDefaultPackageAccess) { + // Must be set directly on SecurityMemberAccess } /** diff --git a/core/src/main/java/com/opensymphony/xwork2/util/ConfigParseUtil.java b/core/src/main/java/com/opensymphony/xwork2/util/ConfigParseUtil.java index 53475b7a88..8debd07db0 100644 --- a/core/src/main/java/com/opensymphony/xwork2/util/ConfigParseUtil.java +++ b/core/src/main/java/com/opensymphony/xwork2/util/ConfigParseUtil.java @@ -34,6 +34,9 @@ public class ConfigParseUtil { + private ConfigParseUtil() { + } + public static Set toClassesSet(String newDelimitedClasses) throws ConfigurationException { Set classNames = commaDelimitedStringToSet(newDelimitedClasses); validateClasses(classNames, OgnlUtil.class.getClassLoader()); 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 7d3f04fc7e..3549ed27f8 100644 --- a/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java @@ -95,7 +95,8 @@ public void configurationCollectionsImmutable() throws Exception { Collection fieldVal = reflectField(field); assertThrows(UnsupportedOperationException.class, () -> fieldVal.add("foo")); if (!fieldVal.isEmpty()) { - assertThrows(UnsupportedOperationException.class, () -> fieldVal.remove(fieldVal.iterator().next())); + String firstVal = fieldVal.iterator().next(); + assertThrows(UnsupportedOperationException.class, () -> fieldVal.remove(firstVal)); assertThrows(UnsupportedOperationException.class, fieldVal::clear); } }