Merge branch 'WW-5343-sec-extend' into WW-5364-populate-allowlist

# Conflicts:
#	core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java

core/src/main/java/com/opensymphony/xwork2/config/impl/DefaultConfiguration.java

@@ -134,16 +134,13 @@ public class DefaultConfiguration implements Configuration {
     static {
         Map<String, Object> 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);
     }
 

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<String, Object> 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);
     }
 

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;
@@ -259,8 +258,5 @@ public void register(ContainerBuilder builder, LocatableProperties props)
         for (Map.Entry<String, Object> 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());
     }
 }

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);
     }
@@ -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,20 +250,23 @@ protected void setContainer(Container container) {
      */
     @Deprecated
     protected void setAllowStaticFieldAccess(String allowStaticFieldAccess) {
+        // Must be set directly on SecurityMemberAccess
     }
 
     /**
      * @deprecated since 6.4.0, no replacement.
      */
     @Deprecated
     protected void setDisallowProxyMemberAccess(String disallowProxyMemberAccess) {
+        // Must be set directly on SecurityMemberAccess
     }
 
     /**
      * @deprecated since 6.4.0, no replacement.
      */
     @Deprecated
     protected void setDisallowDefaultPackageAccess(String disallowDefaultPackageAccess) {
+        // Must be set directly on SecurityMemberAccess
     }
 
     /**

core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java

@@ -48,6 +48,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;
 
 /**
@@ -71,10 +72,10 @@ public class SecurityMemberAccess implements MemberAccess {
     )));
 
     private final ProviderAllowlist providerAllowlist;
-    private final boolean allowStaticFieldAccess;
+    private boolean allowStaticFieldAccess = true;
     private Set<Pattern> excludeProperties = emptySet();
     private Set<Pattern> acceptProperties = emptySet();
-    private Set<String> excludedClasses = emptySet();
+    private Set<String> excludedClasses = unmodifiableSet(new HashSet<>(singletonList(Object.class.getName())));
     private Set<Pattern> excludedPackageNamePatterns = emptySet();
     private Set<String> excludedPackageNames = emptySet();
     private Set<String> excludedPackageExemptClasses = emptySet();
@@ -85,9 +86,7 @@ public class SecurityMemberAccess implements MemberAccess {
     private boolean disallowDefaultPackageAccess = false;
 
     @Inject
-    public SecurityMemberAccess(@Inject(value = StrutsConstants.STRUTS_ALLOW_STATIC_FIELD_ACCESS) String allowStaticFieldAccess, @Inject ProviderAllowlist providerAllowlist) {
-        this.allowStaticFieldAccess = BooleanUtils.toBoolean(allowStaticFieldAccess);
-        useExcludedClasses(""); // Initialise default exclusions
+    public SecurityMemberAccess(@Inject ProviderAllowlist providerAllowlist) {
         this.providerAllowlist = providerAllowlist;
     }
 
@@ -97,9 +96,12 @@ 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(ProviderAllowlist)} instead.
      */
+    @Deprecated
     public SecurityMemberAccess(boolean allowStaticFieldAccess) {
-        this(String.valueOf(allowStaticFieldAccess), null);
+        this(null);
+        useAllowStaticFieldAccess(String.valueOf(allowStaticFieldAccess));
     }
 
     @Override
@@ -396,20 +398,24 @@ public void useAcceptProperties(Set<Pattern> 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<String> 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);
@@ -419,6 +425,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);

core/src/main/java/com/opensymphony/xwork2/util/ConfigParseUtil.java

@@ -34,6 +34,9 @@
 
 public class ConfigParseUtil {
 
+    private ConfigParseUtil() {
+    }
+
     public static Set<String> toClassesSet(String newDelimitedClasses) throws ConfigurationException {
         Set<String> classNames = commaDelimitedStringToSet(newDelimitedClasses);
         validateClasses(classNames, OgnlUtil.class.getClassLoader());

core/src/main/resources/org/apache/struts2/default.properties

@@ -218,20 +218,13 @@ 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
 
 ### 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

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()");

core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java

@@ -65,11 +65,16 @@ public void setUp() throws Exception {
 
     protected void assignNewSma(boolean allowStaticFieldAccess) {
         when(mockedProviderAllowlist.getProviderAllowlist()).thenReturn(new HashSet<>());
-        sma = new SecurityMemberAccess(String.valueOf(allowStaticFieldAccess), mockedProviderAllowlist);
+        sma = new SecurityMemberAccess(mockedProviderAllowlist);
+        sma.useAllowStaticFieldAccess(String.valueOf(allowStaticFieldAccess));
     }
 
     private <T> T reflectField(String fieldName) throws IllegalAccessException {
-        return (T) FieldUtils.readField(sma, fieldName, true);
+        return reflectField(sma, fieldName);
+    }
+
+    public static <T> T reflectField(Object instance, String fieldName) throws IllegalAccessException {
+        return (T) FieldUtils.readField(instance, fieldName, true);
     }
 
     @Test
@@ -97,7 +102,8 @@ public void configurationCollectionsImmutable() throws Exception {
             Collection<String> 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);
             }
         }