From 85d2c742cda110182a5e864751926ff0302124a9 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Sun, 26 Nov 2023 16:52:07 +1100 Subject: [PATCH 1/2] 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 2/2] 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); } }