diff --git a/core/src/main/java/org/apache/struts2/config/impl/DefaultConfiguration.java b/core/src/main/java/org/apache/struts2/config/impl/DefaultConfiguration.java index dcf4f1602e..53bf20dc85 100644 --- a/core/src/main/java/org/apache/struts2/config/impl/DefaultConfiguration.java +++ b/core/src/main/java/org/apache/struts2/config/impl/DefaultConfiguration.java @@ -121,6 +121,7 @@ import org.apache.struts2.factory.StrutsResultFactory; import org.apache.struts2.ognl.OgnlGuard; import org.apache.struts2.ognl.ProviderAllowlist; +import org.apache.struts2.ognl.SecurityMemberAccessConfig; import org.apache.struts2.ognl.StrutsOgnlGuard; import org.apache.struts2.ognl.ThreadAllowlist; @@ -417,6 +418,7 @@ public static ContainerBuilder bootstrapFactories(ContainerBuilder builder) { .factory(OgnlGuard.class, StrutsOgnlGuard.class, Scope.SINGLETON) .factory(ProviderAllowlist.class, Scope.SINGLETON) .factory(ThreadAllowlist.class, Scope.SINGLETON) + .factory(SecurityMemberAccessConfig.class, Scope.SINGLETON) .factory(ValueSubstitutor.class, EnvsValueSubstitutor.class, Scope.SINGLETON); } diff --git a/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java b/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java index badad3dee3..3e626e664d 100644 --- a/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java +++ b/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java @@ -56,12 +56,6 @@ public class SecurityMemberAccess implements MemberAccess { private static final Logger LOG = LogManager.getLogger(SecurityMemberAccess.class); - private static final Set ALLOWLIST_REQUIRED_PACKAGES = Set.of( - "org.apache.struts2.validator.validators", - "org.apache.struts2.components", - "org.apache.struts2.views.jsp" - ); - private static final Set> ALLOWLIST_REQUIRED_CLASSES = Set.of( java.lang.Enum.class, java.lang.String.class, @@ -86,16 +80,9 @@ public class SecurityMemberAccess implements MemberAccess { private Set excludedPackageNames = emptySet(); private Set excludedPackageExemptClasses = emptySet(); - private volatile boolean isDevModeInit; - private boolean isDevMode; - private Set devModeExcludedClasses = Set.of(Object.class.getName()); - private Set devModeExcludedPackageNamePatterns = emptySet(); - private Set devModeExcludedPackageNames = emptySet(); - private Set devModeExcludedPackageExemptClasses = emptySet(); - private boolean enforceAllowlistEnabled = false; private Set> allowlistClasses = emptySet(); - private Set allowlistPackageNames = emptySet(); + private Set allowlistPackageNamesUnion = SecurityMemberAccessConfig.ALLOWLIST_REQUIRED_PACKAGES; private boolean disallowProxyObjectAccess = false; private boolean disallowProxyMemberAccess = false; @@ -112,6 +99,40 @@ public void setProxyService(ProxyService proxyService) { this.proxyService = proxyService; } + /** + * Copies the shared, already-parsed configuration into this instance. This is the only injected + * member that touches the configuration fields, so the unspecified order in which the container + * iterates {@code getDeclaredMethods()} cannot affect the result. + * + * @since Struts 7.4.0 + */ + @Inject + public void useConfig(SecurityMemberAccessConfig config) { + this.allowStaticFieldAccess = config.isAllowStaticFieldAccess(); + this.excludedClasses = config.getExcludedClasses(); + this.excludedPackageNamePatterns = config.getExcludedPackageNamePatterns(); + this.excludedPackageNames = config.getExcludedPackageNames(); + this.excludedPackageExemptClasses = config.getExcludedPackageExemptClasses(); + this.enforceAllowlistEnabled = config.isEnforceAllowlistEnabled(); + this.allowlistClasses = config.getAllowlistClasses(); + this.allowlistPackageNamesUnion = config.getAllowlistPackageNamesUnion(); + this.disallowProxyObjectAccess = config.isDisallowProxyObjectAccess(); + this.disallowProxyMemberAccess = config.isDisallowProxyMemberAccess(); + this.disallowDefaultPackageAccess = config.isDisallowDefaultPackageAccess(); + } + + /** + * Used only by the deprecated {@link #useAllowlistPackageNames(String)} setter path. The injected + * configuration path seeds {@code allowlistPackageNamesUnion} directly from + * {@link SecurityMemberAccessConfig}, which precomputes the union exactly once per container; both + * routes call {@link SecurityMemberAccessConfig#union(Set, Set)}, so there remains exactly one place + * in the codebase that computes the union, and {@code ALLOWLIST_REQUIRED_PACKAGES} cannot be silently + * dropped from either. + */ + private void applyAllowlistPackageNames(Set packageNames) { + this.allowlistPackageNamesUnion = SecurityMemberAccessConfig.union(SecurityMemberAccessConfig.ALLOWLIST_REQUIRED_PACKAGES, packageNames); + } + @Override public Object setup(OgnlContext context, Object target, Member member, String propertyName) { Object result = null; @@ -254,14 +275,13 @@ protected boolean isClassAllowlisted(Class clazz) { || ALLOWLIST_REQUIRED_CLASSES.contains(clazz) || (providerAllowlist != null && providerAllowlist.getProviderAllowlist().contains(clazz)) || (threadAllowlist != null && threadAllowlist.getAllowlist().contains(clazz)) - || isClassBelongsToPackages(clazz, ALLOWLIST_REQUIRED_PACKAGES, allowlistPackageNames); + || isClassBelongsToPackages(clazz, allowlistPackageNamesUnion); } /** * @return {@code true} if member access is allowed */ protected boolean checkExclusionList(Object target, Member member) { - useDevModeConfiguration(); Class memberClass = member.getDeclaringClass(); if (isClassExcluded(memberClass)) { LOG.warn("Declaring class of member type [{}] is excluded!", memberClass); @@ -390,54 +410,38 @@ protected boolean isExcludedPackageNames(Class clazz) { } public static boolean isClassBelongsToPackages(Class clazz, Set matchingPackages) { - return isClassBelongsToPackages(clazz, matchingPackages, emptySet()); + return isPackageBelongsToPackages(toPackageName(clazz), matchingPackages); } /** - * Tests the class's package against two sets in a single walk. Equivalent to calling - * {@link #isClassBelongsToPackages(Class, Set)} once per set and OR-ing the results, but - * walks the package name only once. - * - * @param clazz the class whose package is tested - * @param first the first set of package names to match against - * @param second the second set of package names to match against - * @return {@code true} if the class's package or any parent package is in either set - */ - static boolean isClassBelongsToPackages(Class clazz, Set first, Set second) { - return isPackageBelongsToPackages(toPackageName(clazz), first, second); - } - - /** - * Tests whether the given package name, or any of its parent packages, is present in either - * set. Walks the name in place rather than building the full prefix list, since this runs on - * the OGNL member-access path. Shortest prefix first, so broad entries such as {@code java.io} + * Tests whether the given package name, or any of its parent packages, is present in the set. + * Walks the name in place rather than building the full prefix list, since this runs on the OGNL + * member-access path. Shortest prefix first, so broad entries such as {@code java.io} * short-circuit earliest. * *

- * The package name must not end in {@code '.'}. Such a name is probed one prefix more than by - * the implementation this replaced, which matches more broadly — tightening exclusion but - * loosening the allowlist. {@link Class#getPackageName()} cannot produce a trailing - * dot, so every current caller is safe; route any other string through here only after - * confirming the same. + * The package name must not end in {@code '.'}. Such a name is probed one prefix more than by the + * implementation this replaced, which matches more broadly — tightening exclusion but + * loosening the allowlist. {@link Class#getPackageName()} cannot produce a trailing dot, + * and {@code ConfigParseUtil.toPackageNamesSet} strips them from configured names, so every + * current caller is safe; route any other string through here only after confirming the same. * - * @param packageName the package name to test, empty for the default package, never ending in {@code '.'} - * @param first the first set of package names to match against - * @param second the second set of package names to match against - * @return {@code true} if the package or any parent package is in either set + * @param packageName the package name to test, empty for the default package, never ending in {@code '.'} + * @param matchingPackages the package names to match against + * @return {@code true} if the package or any parent package is in the set */ - static boolean isPackageBelongsToPackages(String packageName, Set first, Set second) { - if (first.isEmpty() && second.isEmpty()) { + static boolean isPackageBelongsToPackages(String packageName, Set matchingPackages) { + if (matchingPackages.isEmpty()) { return false; } int idx = packageName.indexOf('.'); while (idx != -1) { - String prefix = packageName.substring(0, idx); - if (first.contains(prefix) || second.contains(prefix)) { + if (matchingPackages.contains(packageName.substring(0, idx))) { return true; } idx = packageName.indexOf('.', idx + 1); } - return first.contains(packageName) || second.contains(packageName); + return matchingPackages.contains(packageName); } protected boolean isClassExcluded(Class clazz) { @@ -470,7 +474,13 @@ public void useAcceptProperties(Set acceptedProperties) { this.acceptProperties = acceptedProperties; } - @Inject(value = StrutsConstants.STRUTS_ALLOW_STATIC_FIELD_ACCESS, required = false) + /** + * @deprecated since 7.4.0, configuration is parsed once per container by + * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for + * tests and existing callers; it will be removed in Struts 8.0.0. The container no longer invokes + * this setter. + */ + @Deprecated(since = "7.4.0", forRemoval = true) public void useAllowStaticFieldAccess(String allowStaticFieldAccess) { this.allowStaticFieldAccess = BooleanUtils.toBoolean(allowStaticFieldAccess); if (!this.allowStaticFieldAccess) { @@ -478,27 +488,57 @@ public void useAllowStaticFieldAccess(String allowStaticFieldAccess) { } } - @Inject(value = StrutsConstants.STRUTS_EXCLUDED_CLASSES, required = false) + /** + * @deprecated since 7.4.0, configuration is parsed once per container by + * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for + * tests and existing callers; it will be removed in Struts 8.0.0. The container no longer invokes + * this setter. + */ + @Deprecated(since = "7.4.0", forRemoval = true) public void useExcludedClasses(String commaDelimitedClasses) { this.excludedClasses = toNewClassesSet(excludedClasses, commaDelimitedClasses); } - @Inject(value = StrutsConstants.STRUTS_EXCLUDED_PACKAGE_NAME_PATTERNS, required = false) + /** + * @deprecated since 7.4.0, configuration is parsed once per container by + * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for + * tests and existing callers; it will be removed in Struts 8.0.0. The container no longer invokes + * this setter. + */ + @Deprecated(since = "7.4.0", forRemoval = true) public void useExcludedPackageNamePatterns(String commaDelimitedPackagePatterns) { this.excludedPackageNamePatterns = toNewPatternsSet(excludedPackageNamePatterns, commaDelimitedPackagePatterns); } - @Inject(value = StrutsConstants.STRUTS_EXCLUDED_PACKAGE_NAMES, required = false) + /** + * @deprecated since 7.4.0, configuration is parsed once per container by + * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for + * tests and existing callers; it will be removed in Struts 8.0.0. The container no longer invokes + * this setter. + */ + @Deprecated(since = "7.4.0", forRemoval = true) public void useExcludedPackageNames(String commaDelimitedPackageNames) { this.excludedPackageNames = toNewPackageNamesSet(excludedPackageNames, commaDelimitedPackageNames); } - @Inject(value = StrutsConstants.STRUTS_EXCLUDED_PACKAGE_EXEMPT_CLASSES, required = false) + /** + * @deprecated since 7.4.0, configuration is parsed once per container by + * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for + * tests and existing callers; it will be removed in Struts 8.0.0. The container no longer invokes + * this setter. + */ + @Deprecated(since = "7.4.0", forRemoval = true) public void useExcludedPackageExemptClasses(String commaDelimitedClasses) { this.excludedPackageExemptClasses = toClassesSet(commaDelimitedClasses); } - @Inject(value = StrutsConstants.STRUTS_ALLOWLIST_ENABLE, required = false) + /** + * @deprecated since 7.4.0, configuration is parsed once per container by + * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for + * tests and existing callers; it will be removed in Struts 8.0.0. The container no longer invokes + * this setter. + */ + @Deprecated(since = "7.4.0", forRemoval = true) public void useEnforceAllowlistEnabled(String enforceAllowlistEnabled) { this.enforceAllowlistEnabled = BooleanUtils.toBoolean(enforceAllowlistEnabled); if (!this.enforceAllowlistEnabled) { @@ -510,66 +550,59 @@ public void useEnforceAllowlistEnabled(String enforceAllowlistEnabled) { } } - @Inject(value = STRUTS_ALLOWLIST_CLASSES, required = false) + /** + * @deprecated since 7.4.0, configuration is parsed once per container by + * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for + * tests and existing callers; it will be removed in Struts 8.0.0. The container no longer invokes + * this setter. + */ + @Deprecated(since = "7.4.0", forRemoval = true) public void useAllowlistClasses(String commaDelimitedClasses) { this.allowlistClasses = toClassObjectsSet(commaDelimitedClasses); } - @Inject(value = STRUTS_ALLOWLIST_PACKAGE_NAMES, required = false) + /** + * @deprecated since 7.4.0, configuration is parsed once per container by + * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for + * tests and existing callers; it will be removed in Struts 8.0.0. The container no longer invokes + * this setter. + */ + @Deprecated(since = "7.4.0", forRemoval = true) public void useAllowlistPackageNames(String commaDelimitedPackageNames) { - this.allowlistPackageNames = toPackageNamesSet(commaDelimitedPackageNames); + applyAllowlistPackageNames(toPackageNamesSet(commaDelimitedPackageNames)); } - @Inject(value = StrutsConstants.STRUTS_DISALLOW_PROXY_OBJECT_ACCESS, required = false) + /** + * @deprecated since 7.4.0, configuration is parsed once per container by + * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for + * tests and existing callers; it will be removed in Struts 8.0.0. The container no longer invokes + * this setter. + */ + @Deprecated(since = "7.4.0", forRemoval = true) public void useDisallowProxyObjectAccess(String disallowProxyObjectAccess) { this.disallowProxyObjectAccess = BooleanUtils.toBoolean(disallowProxyObjectAccess); } - @Inject(value = StrutsConstants.STRUTS_DISALLOW_PROXY_MEMBER_ACCESS, required = false) + /** + * @deprecated since 7.4.0, configuration is parsed once per container by + * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for + * tests and existing callers; it will be removed in Struts 8.0.0. The container no longer invokes + * this setter. + */ + @Deprecated(since = "7.4.0", forRemoval = true) public void useDisallowProxyMemberAccess(String disallowProxyMemberAccess) { this.disallowProxyMemberAccess = BooleanUtils.toBoolean(disallowProxyMemberAccess); } - @Inject(value = StrutsConstants.STRUTS_DISALLOW_DEFAULT_PACKAGE_ACCESS, required = false) + /** + * @deprecated since 7.4.0, configuration is parsed once per container by + * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for + * tests and existing callers; it will be removed in Struts 8.0.0. The container no longer invokes + * this setter. + */ + @Deprecated(since = "7.4.0", forRemoval = true) public void useDisallowDefaultPackageAccess(String disallowDefaultPackageAccess) { this.disallowDefaultPackageAccess = BooleanUtils.toBoolean(disallowDefaultPackageAccess); } - @Inject(StrutsConstants.STRUTS_DEVMODE) - protected void useDevMode(String devMode) { - this.isDevMode = BooleanUtils.toBoolean(devMode); - } - - @Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_CLASSES, required = false) - public void useDevModeExcludedClasses(String commaDelimitedClasses) { - this.devModeExcludedClasses = toNewClassesSet(devModeExcludedClasses, commaDelimitedClasses); - } - - @Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_PACKAGE_NAME_PATTERNS, required = false) - public void useDevModeExcludedPackageNamePatterns(String commaDelimitedPackagePatterns) { - this.devModeExcludedPackageNamePatterns = toNewPatternsSet(devModeExcludedPackageNamePatterns, commaDelimitedPackagePatterns); - } - - @Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_PACKAGE_NAMES, required = false) - public void useDevModeExcludedPackageNames(String commaDelimitedPackageNames) { - this.devModeExcludedPackageNames = toNewPackageNamesSet(devModeExcludedPackageNames, commaDelimitedPackageNames); - } - - @Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_PACKAGE_EXEMPT_CLASSES, required = false) - public void useDevModeExcludedPackageExemptClasses(String commaDelimitedClasses) { - this.devModeExcludedPackageExemptClasses = toClassesSet(commaDelimitedClasses); - } - - private void useDevModeConfiguration() { - if (!isDevMode || isDevModeInit) { - return; - } - logWarningForFirstOccurrence("devMode", LOG, - "DevMode enabled, using DevMode excluded classes and packages for OGNL security enforcement!"); - isDevModeInit = true; - excludedClasses = devModeExcludedClasses; - excludedPackageNamePatterns = devModeExcludedPackageNamePatterns; - excludedPackageNames = devModeExcludedPackageNames; - excludedPackageExemptClasses = devModeExcludedPackageExemptClasses; - } } diff --git a/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccessConfig.java b/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccessConfig.java new file mode 100644 index 0000000000..6278717d24 --- /dev/null +++ b/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccessConfig.java @@ -0,0 +1,270 @@ +/* + * 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 org.apache.struts2.ognl; + +import org.apache.commons.lang3.BooleanUtils; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; +import org.apache.struts2.StrutsConstants; +import org.apache.struts2.inject.Inject; +import org.apache.struts2.inject.Initializable; + +import java.util.HashSet; +import java.util.Set; +import java.util.regex.Pattern; + +import static java.util.Collections.emptySet; +import static java.util.Collections.unmodifiableSet; +import static org.apache.struts2.StrutsConstants.STRUTS_ALLOWLIST_CLASSES; +import static org.apache.struts2.StrutsConstants.STRUTS_ALLOWLIST_PACKAGE_NAMES; +import static org.apache.struts2.util.ConfigParseUtil.toClassObjectsSet; +import static org.apache.struts2.util.ConfigParseUtil.toClassesSet; +import static org.apache.struts2.util.ConfigParseUtil.toNewClassesSet; +import static org.apache.struts2.util.ConfigParseUtil.toNewPackageNamesSet; +import static org.apache.struts2.util.ConfigParseUtil.toNewPatternsSet; +import static org.apache.struts2.util.ConfigParseUtil.toPackageNamesSet; +import static org.apache.struts2.util.DebugUtils.logWarningForFirstOccurrence; + +/** + * Holds the parsed OGNL security configuration for one container. + *

+ * {@link SecurityMemberAccess} is a {@code Scope.PROTOTYPE} bean, constructed once per value stack and + * again for each OGNL context. Parsing the roughly ninety configuration entries on every one of those + * was the dominant cost identified by WW-5667. This bean is a {@code Scope.SINGLETON}, so the parsing + * happens once per container and each {@code SecurityMemberAccess} merely copies immutable references. + *

+ * Dev-mode is resolved in {@link #init()} rather than in a setter, because the container iterates + * {@code getDeclaredMethods()}, whose order the JDK leaves unspecified. If {@code init()} never runs, + * the normal production exclusions stay in force, which fails closed. + * + * @since Struts 7.4.0 + */ +public class SecurityMemberAccessConfig implements Initializable { + + private static final Logger LOG = LogManager.getLogger(SecurityMemberAccessConfig.class); + + /** + * Struts' own component packages, which must always be allowlisted regardless of what an + * application configures via {@code struts.allowlist.packageNames}. Lives here, alongside + * {@link #union(Set, Set)}, because this is the single place that computes + * {@code allowlistPackageNamesUnion}; {@link SecurityMemberAccess} references both statically for + * its default field value and its deprecated {@code useAllowlistPackageNames} setter, so the + * computation is never duplicated. + */ + static final Set ALLOWLIST_REQUIRED_PACKAGES = Set.of( + "org.apache.struts2.validator.validators", + "org.apache.struts2.components", + "org.apache.struts2.views.jsp" + ); + + private boolean allowStaticFieldAccess = true; + + private Set excludedClasses = Set.of(Object.class.getName()); + private Set excludedPackageNamePatterns = emptySet(); + private Set excludedPackageNames = emptySet(); + private Set excludedPackageExemptClasses = emptySet(); + + private boolean isDevMode; + private Set devModeExcludedClasses = Set.of(Object.class.getName()); + private Set devModeExcludedPackageNamePatterns = emptySet(); + private Set devModeExcludedPackageNames = emptySet(); + private Set devModeExcludedPackageExemptClasses = emptySet(); + + private boolean enforceAllowlistEnabled = false; + private Set> allowlistClasses = emptySet(); + private Set allowlistPackageNames = emptySet(); + private Set allowlistPackageNamesUnion = ALLOWLIST_REQUIRED_PACKAGES; + + private boolean disallowProxyObjectAccess = false; + private boolean disallowProxyMemberAccess = false; + private boolean disallowDefaultPackageAccess = false; + + @Override + public void init() { + if (!isDevMode) { + return; + } + logWarningForFirstOccurrence("devMode", LOG, + "DevMode enabled, using DevMode excluded classes and packages for OGNL security enforcement!"); + excludedClasses = devModeExcludedClasses; + excludedPackageNamePatterns = devModeExcludedPackageNamePatterns; + excludedPackageNames = devModeExcludedPackageNames; + excludedPackageExemptClasses = devModeExcludedPackageExemptClasses; + } + + @Inject(value = StrutsConstants.STRUTS_ALLOW_STATIC_FIELD_ACCESS, required = false) + void useAllowStaticFieldAccess(String allowStaticFieldAccess) { + this.allowStaticFieldAccess = BooleanUtils.toBoolean(allowStaticFieldAccess); + if (!this.allowStaticFieldAccess) { + useExcludedClasses(Class.class.getName()); + } + } + + @Inject(value = StrutsConstants.STRUTS_EXCLUDED_CLASSES, required = false) + void useExcludedClasses(String commaDelimitedClasses) { + this.excludedClasses = toNewClassesSet(excludedClasses, commaDelimitedClasses); + } + + @Inject(value = StrutsConstants.STRUTS_EXCLUDED_PACKAGE_NAME_PATTERNS, required = false) + void useExcludedPackageNamePatterns(String commaDelimitedPackagePatterns) { + this.excludedPackageNamePatterns = toNewPatternsSet(excludedPackageNamePatterns, commaDelimitedPackagePatterns); + } + + @Inject(value = StrutsConstants.STRUTS_EXCLUDED_PACKAGE_NAMES, required = false) + void useExcludedPackageNames(String commaDelimitedPackageNames) { + this.excludedPackageNames = toNewPackageNamesSet(excludedPackageNames, commaDelimitedPackageNames); + } + + @Inject(value = StrutsConstants.STRUTS_EXCLUDED_PACKAGE_EXEMPT_CLASSES, required = false) + void useExcludedPackageExemptClasses(String commaDelimitedClasses) { + this.excludedPackageExemptClasses = toClassesSet(commaDelimitedClasses); + } + + @Inject(value = StrutsConstants.STRUTS_ALLOWLIST_ENABLE, required = false) + void useEnforceAllowlistEnabled(String enforceAllowlistEnabled) { + this.enforceAllowlistEnabled = BooleanUtils.toBoolean(enforceAllowlistEnabled); + if (!this.enforceAllowlistEnabled) { + String msg = "OGNL allowlist is disabled!" + + " We strongly recommend keeping it enabled to protect against critical vulnerabilities." + + " Set the configuration `{}=true` to enable it." + + " Please refer to the Struts 7.0 migration guide and security documentation for further information."; + logWarningForFirstOccurrence("allowlist", LOG, msg, StrutsConstants.STRUTS_ALLOWLIST_ENABLE); + } + } + + @Inject(value = STRUTS_ALLOWLIST_CLASSES, required = false) + void useAllowlistClasses(String commaDelimitedClasses) { + this.allowlistClasses = toClassObjectsSet(commaDelimitedClasses); + } + + @Inject(value = STRUTS_ALLOWLIST_PACKAGE_NAMES, required = false) + void useAllowlistPackageNames(String commaDelimitedPackageNames) { + this.allowlistPackageNames = toPackageNamesSet(commaDelimitedPackageNames); + this.allowlistPackageNamesUnion = union(ALLOWLIST_REQUIRED_PACKAGES, allowlistPackageNames); + } + + /** + * The only place in the codebase that computes the allowlist package union. Both + * {@link #useAllowlistPackageNames(String)} above and {@link SecurityMemberAccess}'s deprecated + * setter path call this method, so {@code ALLOWLIST_REQUIRED_PACKAGES} can never silently drop out + * of the union through a second, drifted implementation. + *

+ * The result is always immutable, whatever the caller passes. When nothing is configured the + * required set is returned through {@link Set#copyOf}, which the JDK short-circuits to the same + * instance for an already-immutable set — so the usual case allocates nothing, while a mutable + * {@code required} would still be defensively copied rather than aliased into a set shared by + * every {@link SecurityMemberAccess} in the container. + */ + static Set union(Set required, Set configured) { + if (configured.isEmpty()) { + return Set.copyOf(required); + } + Set union = new HashSet<>(required); + union.addAll(configured); + return unmodifiableSet(union); + } + + @Inject(value = StrutsConstants.STRUTS_DISALLOW_PROXY_OBJECT_ACCESS, required = false) + void useDisallowProxyObjectAccess(String disallowProxyObjectAccess) { + this.disallowProxyObjectAccess = BooleanUtils.toBoolean(disallowProxyObjectAccess); + } + + @Inject(value = StrutsConstants.STRUTS_DISALLOW_PROXY_MEMBER_ACCESS, required = false) + void useDisallowProxyMemberAccess(String disallowProxyMemberAccess) { + this.disallowProxyMemberAccess = BooleanUtils.toBoolean(disallowProxyMemberAccess); + } + + @Inject(value = StrutsConstants.STRUTS_DISALLOW_DEFAULT_PACKAGE_ACCESS, required = false) + void useDisallowDefaultPackageAccess(String disallowDefaultPackageAccess) { + this.disallowDefaultPackageAccess = BooleanUtils.toBoolean(disallowDefaultPackageAccess); + } + + @Inject(StrutsConstants.STRUTS_DEVMODE) + void useDevMode(String devMode) { + this.isDevMode = BooleanUtils.toBoolean(devMode); + } + + @Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_CLASSES, required = false) + void useDevModeExcludedClasses(String commaDelimitedClasses) { + this.devModeExcludedClasses = toNewClassesSet(devModeExcludedClasses, commaDelimitedClasses); + } + + @Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_PACKAGE_NAME_PATTERNS, required = false) + void useDevModeExcludedPackageNamePatterns(String commaDelimitedPackagePatterns) { + this.devModeExcludedPackageNamePatterns = toNewPatternsSet(devModeExcludedPackageNamePatterns, commaDelimitedPackagePatterns); + } + + @Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_PACKAGE_NAMES, required = false) + void useDevModeExcludedPackageNames(String commaDelimitedPackageNames) { + this.devModeExcludedPackageNames = toNewPackageNamesSet(devModeExcludedPackageNames, commaDelimitedPackageNames); + } + + @Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_PACKAGE_EXEMPT_CLASSES, required = false) + void useDevModeExcludedPackageExemptClasses(String commaDelimitedClasses) { + this.devModeExcludedPackageExemptClasses = toClassesSet(commaDelimitedClasses); + } + + public boolean isAllowStaticFieldAccess() { + return allowStaticFieldAccess; + } + + public Set getExcludedClasses() { + return excludedClasses; + } + + public Set getExcludedPackageNamePatterns() { + return excludedPackageNamePatterns; + } + + public Set getExcludedPackageNames() { + return excludedPackageNames; + } + + public Set getExcludedPackageExemptClasses() { + return excludedPackageExemptClasses; + } + + public boolean isEnforceAllowlistEnabled() { + return enforceAllowlistEnabled; + } + + public Set> getAllowlistClasses() { + return allowlistClasses; + } + + public Set getAllowlistPackageNames() { + return allowlistPackageNames; + } + + public Set getAllowlistPackageNamesUnion() { + return allowlistPackageNamesUnion; + } + + public boolean isDisallowProxyObjectAccess() { + return disallowProxyObjectAccess; + } + + public boolean isDisallowProxyMemberAccess() { + return disallowProxyMemberAccess; + } + + public boolean isDisallowDefaultPackageAccess() { + return disallowDefaultPackageAccess; + } +} diff --git a/core/src/main/java/org/apache/struts2/util/ConfigParseUtil.java b/core/src/main/java/org/apache/struts2/util/ConfigParseUtil.java index 116ca637dd..06dc1a43c5 100644 --- a/core/src/main/java/org/apache/struts2/util/ConfigParseUtil.java +++ b/core/src/main/java/org/apache/struts2/util/ConfigParseUtil.java @@ -47,6 +47,8 @@ public class ConfigParseUtil { .maximumSize(MAX_CLASSLOADER_CACHE_SIZE) .build(); + private static final Pattern WHITESPACE = Pattern.compile("\\s"); + private ConfigParseUtil() { } @@ -140,7 +142,7 @@ public static Set toNewPackageNamesSet(Collection oldPackageName } public static void validatePackageNames(Collection packageNames) { - if (packageNames.stream().anyMatch(s -> Pattern.compile("\\s").matcher(s).find())) { + if (packageNames.stream().anyMatch(s -> WHITESPACE.matcher(s).find())) { throw new ConfigurationException("Excluded package names could not be parsed due to erroneous whitespace characters: " + packageNames); } } diff --git a/core/src/main/resources/struts-beans.xml b/core/src/main/resources/struts-beans.xml index 2de4ebc405..84f0919dcd 100644 --- a/core/src/main/resources/struts-beans.xml +++ b/core/src/main/resources/struts-beans.xml @@ -174,6 +174,7 @@ class="org.apache.struts2.ognl.StrutsOgnlGuard"/> + diff --git a/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigProductionRegistrationTest.java b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigProductionRegistrationTest.java new file mode 100644 index 0000000000..cd1018d50a --- /dev/null +++ b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigProductionRegistrationTest.java @@ -0,0 +1,43 @@ +/* + * 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 org.apache.struts2.ognl; + +import org.apache.struts2.StrutsInternalTestCase; + +/** + * Covers the {@code struts-beans.xml} registration of {@link SecurityMemberAccessConfig}, which + * {@link SecurityMemberAccessConfigSharingTest} cannot: that test extends {@link org.apache.struts2.XWorkTestCase} + * directly, whose container is built from {@code StrutsDefaultConfigurationProvider} alone and never loads + * {@code struts-beans.xml}. Production, via {@link org.apache.struts2.dispatcher.Dispatcher#init()}, never adds + * that provider and relies entirely on the {@code struts-beans.xml} entry. + *

+ * {@link StrutsInternalTestCase} boots a real {@link org.apache.struts2.dispatcher.Dispatcher}, so its container + * is wired the way production's is. Without this test, the singleton scope of the {@code struts-beans.xml} + * entry — the entire point of WW-5675 sharing parsed configuration across {@link SecurityMemberAccess} + * instances — could regress to {@code scope="prototype"} with the whole suite staying green. + */ +public class SecurityMemberAccessConfigProductionRegistrationTest extends StrutsInternalTestCase { + + public void testConfigBeanIsASingletonInTheProductionContainer() { + SecurityMemberAccessConfig first = container.getInstance(SecurityMemberAccessConfig.class); + assertNotNull("SecurityMemberAccessConfig is not registered in the production container", first); + assertSame("SecurityMemberAccessConfig is not a singleton in the production container", + first, container.getInstance(SecurityMemberAccessConfig.class)); + } +} diff --git a/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigSharingTest.java b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigSharingTest.java new file mode 100644 index 0000000000..afcffc19a3 --- /dev/null +++ b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigSharingTest.java @@ -0,0 +1,192 @@ +/* + * 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 org.apache.struts2.ognl; + +import org.apache.struts2.StrutsConstants; +import org.apache.struts2.XWorkTestCase; + +import java.util.Map; +import java.util.Set; + +import static org.assertj.core.api.Assertions.assertThat; + +public class SecurityMemberAccessConfigSharingTest extends XWorkTestCase { + + /** + * Reference identity proves no re-parsing occurred: any re-parse necessarily + * allocates a fresh set. + *

+ * The instance-to-instance {@code assertSame} calls below are necessary but not sufficient: + * for a field whose default is {@link java.util.Collections#emptySet()}, two independently + * unseeded instances would also compare same, since {@code emptySet()} returns a + * JVM-wide singleton. Only {@code excludedClasses}, whose default {@code Set.of(...)} allocates + * a fresh instance per object, is proven by the instance-to-instance form alone. Every field is + * therefore additionally compared directly against the shared {@link SecurityMemberAccessConfig} + * bean, which fails on omission regardless of the default's identity. + *

+ * That direct comparison is itself vacuous unless the configured value actually differs from the + * hardcoded default: {@code SecurityMemberAccess} and {@code SecurityMemberAccessConfig} share the + * same hardcoded defaults, so an unseeded field and a config parsed from an all-default container + * would also compare equal/same by coincidence. The container is therefore reloaded here with every + * relevant constant set away from its default, so a config value only matches the instance's field + * when {@code useConfig} actually ran. + */ + public void testConfigDerivedSetsAreSharedAcrossInstances() throws Exception { + loadButSet(Map.ofEntries( + Map.entry(StrutsConstants.STRUTS_ALLOW_STATIC_FIELD_ACCESS, "false"), + Map.entry(StrutsConstants.STRUTS_EXCLUDED_PACKAGE_NAME_PATTERNS, "^org\\.apache\\.struts2\\.ognl\\.testpkg\\..*"), + Map.entry(StrutsConstants.STRUTS_EXCLUDED_PACKAGE_NAMES, "org.apache.struts2.ognl.testpkg"), + Map.entry(StrutsConstants.STRUTS_EXCLUDED_PACKAGE_EXEMPT_CLASSES, "java.lang.String"), + Map.entry(StrutsConstants.STRUTS_ALLOWLIST_ENABLE, "true"), + Map.entry(StrutsConstants.STRUTS_ALLOWLIST_CLASSES, "java.lang.String"), + Map.entry(StrutsConstants.STRUTS_ALLOWLIST_PACKAGE_NAMES, "org.apache.struts2.ognl.testpkg"), + Map.entry(StrutsConstants.STRUTS_DISALLOW_PROXY_OBJECT_ACCESS, "true"), + Map.entry(StrutsConstants.STRUTS_DISALLOW_PROXY_MEMBER_ACCESS, "true"), + Map.entry(StrutsConstants.STRUTS_DISALLOW_DEFAULT_PACKAGE_ACCESS, "true"))); + + SecurityMemberAccess first = container.getInstance(SecurityMemberAccess.class); + SecurityMemberAccess second = container.getInstance(SecurityMemberAccess.class); + SecurityMemberAccessConfig config = container.getInstance(SecurityMemberAccessConfig.class); + + assertNotSame("expected a prototype bean", first, second); + + Set firstExcluded = SecurityMemberAccessTest.reflectField(first, "excludedClasses"); + Set secondExcluded = SecurityMemberAccessTest.reflectField(second, "excludedClasses"); + assertSame("excluded classes were re-parsed per instance", firstExcluded, secondExcluded); + + Set firstPackages = SecurityMemberAccessTest.reflectField(first, "excludedPackageNames"); + Set secondPackages = SecurityMemberAccessTest.reflectField(second, "excludedPackageNames"); + assertSame("excluded package names were re-parsed per instance", firstPackages, secondPackages); + + assertSame("excludedClasses not seeded from config", + config.getExcludedClasses(), SecurityMemberAccessTest.reflectField(first, "excludedClasses")); + assertSame("excludedPackageNamePatterns not seeded from config", + config.getExcludedPackageNamePatterns(), SecurityMemberAccessTest.reflectField(first, "excludedPackageNamePatterns")); + assertSame("excludedPackageNames not seeded from config", + config.getExcludedPackageNames(), SecurityMemberAccessTest.reflectField(first, "excludedPackageNames")); + assertSame("excludedPackageExemptClasses not seeded from config", + config.getExcludedPackageExemptClasses(), SecurityMemberAccessTest.reflectField(first, "excludedPackageExemptClasses")); + assertSame("allowlistClasses not seeded from config", + config.getAllowlistClasses(), SecurityMemberAccessTest.reflectField(first, "allowlistClasses")); + Set firstAllowlistPackageNamesUnion = SecurityMemberAccessTest.reflectField(first, "allowlistPackageNamesUnion"); + assertSame("allowlistPackageNamesUnion not seeded from config", + config.getAllowlistPackageNamesUnion(), firstAllowlistPackageNamesUnion); + assertThat(firstAllowlistPackageNamesUnion).contains("org.apache.struts2.ognl.testpkg", "org.apache.struts2.components"); + + boolean firstAllowStaticFieldAccess = SecurityMemberAccessTest.reflectField(first, "allowStaticFieldAccess"); + assertEquals("allowStaticFieldAccess not seeded from config", + config.isAllowStaticFieldAccess(), firstAllowStaticFieldAccess); + boolean firstEnforceAllowlistEnabled = SecurityMemberAccessTest.reflectField(first, "enforceAllowlistEnabled"); + assertEquals("enforceAllowlistEnabled not seeded from config", + config.isEnforceAllowlistEnabled(), firstEnforceAllowlistEnabled); + boolean firstDisallowProxyObjectAccess = SecurityMemberAccessTest.reflectField(first, "disallowProxyObjectAccess"); + assertEquals("disallowProxyObjectAccess not seeded from config", + config.isDisallowProxyObjectAccess(), firstDisallowProxyObjectAccess); + boolean firstDisallowProxyMemberAccess = SecurityMemberAccessTest.reflectField(first, "disallowProxyMemberAccess"); + assertEquals("disallowProxyMemberAccess not seeded from config", + config.isDisallowProxyMemberAccess(), firstDisallowProxyMemberAccess); + boolean firstDisallowDefaultPackageAccess = SecurityMemberAccessTest.reflectField(first, "disallowDefaultPackageAccess"); + assertEquals("disallowDefaultPackageAccess not seeded from config", + config.isDisallowDefaultPackageAccess(), firstDisallowDefaultPackageAccess); + } + + public void testConfigBeanIsASingleton() { + SecurityMemberAccessConfig instance = container.getInstance(SecurityMemberAccessConfig.class); + assertNotNull("SecurityMemberAccessConfig is not registered in the container", instance); + assertSame(instance, container.getInstance(SecurityMemberAccessConfig.class)); + } + + /** + * The shared sets must not be perturbed by a deprecated setter call on one instance. + */ + public void testDeprecatedSetterDoesNotLeakToSiblings() throws Exception { + SecurityMemberAccess mutated = container.getInstance(SecurityMemberAccess.class); + SecurityMemberAccess untouched = container.getInstance(SecurityMemberAccess.class); + SecurityMemberAccessConfig config = container.getInstance(SecurityMemberAccessConfig.class); + + Set before = SecurityMemberAccessTest.reflectField(untouched, "excludedClasses"); + mutated.useExcludedClasses("java.lang.Runtime"); + Set after = SecurityMemberAccessTest.reflectField(untouched, "excludedClasses"); + + assertSame("a sibling instance was affected", before, after); + assertFalse("the shared config was mutated", config.getExcludedClasses().contains("java.lang.Runtime")); + + Set mutatedSet = SecurityMemberAccessTest.reflectField(mutated, "excludedClasses"); + assertTrue("the setter did not affect its own instance", mutatedSet.contains("java.lang.Runtime")); + } + + /** + * Guards the fail-open hole avoided by using setter rather than constructor injection: + * a subclass calling the two-argument super constructor must still receive the config. + */ + public void testSubclassReceivesConfigThroughInheritedSetter() throws Exception { + SubclassedSecurityMemberAccess subclassed = new SubclassedSecurityMemberAccess( + container.getInstance(ProviderAllowlist.class), + container.getInstance(ThreadAllowlist.class)); + + container.inject(subclassed); + + Set excluded = SecurityMemberAccessTest.reflectField(subclassed, "excludedClasses"); + assertSame("subclass did not receive the shared config", + container.getInstance(SecurityMemberAccessConfig.class).getExcludedClasses(), excluded); + } + + static class SubclassedSecurityMemberAccess extends SecurityMemberAccess { + SubclassedSecurityMemberAccess(ProviderAllowlist providerAllowlist, ThreadAllowlist threadAllowlist) { + super(providerAllowlist, threadAllowlist); + } + } + + /** + * Dev-mode exclusions must be in force from the first access, with no lazy flip. + */ + public void testDevModeExclusionsApplyWithoutAnAccess() throws Exception { + loadButSet(Map.of( + StrutsConstants.STRUTS_DEVMODE, "true", + StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_CLASSES, "java.lang.ProcessBuilder")); + + SecurityMemberAccess sma = container.getInstance(SecurityMemberAccess.class); + Set excluded = SecurityMemberAccessTest.reflectField(sma, "excludedClasses"); + + assertTrue("dev-mode exclusions were not applied at startup", + excluded.contains("java.lang.ProcessBuilder")); + // The `contains` check above is non-vacuous only because the test container does not load + // struts-excluded-classes.xml, where java.lang.ProcessBuilder happens to sit in both the + // production and dev-mode excluded-classes sets. Asserting identity with the config bean's + // set keeps this test meaningful even if the harness starts loading that file. + assertSame("excludedClasses was not seeded from the dev-mode-resolved config", + container.getInstance(SecurityMemberAccessConfig.class).getExcludedClasses(), excluded); + } + + public void testDevModeMethodsAreGone() throws Exception { + Set removedMethods = Set.of("useDevMode", "useDevModeExcludedClasses", + "useDevModeExcludedPackageNamePatterns", "useDevModeExcludedPackageNames", + "useDevModeExcludedPackageExemptClasses", "useDevModeConfiguration"); + for (java.lang.reflect.Method method : SecurityMemberAccess.class.getDeclaredMethods()) { + assertFalse("SecurityMemberAccess still declares " + method.getName(), removedMethods.contains(method.getName())); + } + + Set removedFields = Set.of("isDevModeInit", "isDevMode", "devModeExcludedClasses", + "devModeExcludedPackageNamePatterns", "devModeExcludedPackageNames", + "devModeExcludedPackageExemptClasses"); + for (java.lang.reflect.Field field : SecurityMemberAccess.class.getDeclaredFields()) { + assertFalse("SecurityMemberAccess still declares field " + field.getName(), removedFields.contains(field.getName())); + } + } +} diff --git a/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigTest.java b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigTest.java new file mode 100644 index 0000000000..652d26d8c0 --- /dev/null +++ b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigTest.java @@ -0,0 +1,164 @@ +/* + * 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 org.apache.struts2.ognl; + +import org.junit.Test; + +import java.util.Set; +import java.util.regex.Pattern; + +import static org.apache.struts2.util.ConfigParseUtil.toNewClassesSet; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +public class SecurityMemberAccessConfigTest { + + /** + * Frozen oracle: the accumulation SecurityMemberAccess performed before WW-5675. + * Never delete this, and never make it delegate to production code. + */ + private static Set legacyExcludedClassAccumulation(boolean allowStaticFieldAccess, String configured) { + Set excludedClasses = Set.of(Object.class.getName()); + if (!allowStaticFieldAccess) { + excludedClasses = toNewClassesSet(excludedClasses, Class.class.getName()); + } + return toNewClassesSet(excludedClasses, configured); + } + + private SecurityMemberAccessConfig configWith(boolean devMode, String excludedClasses, String devModeExcludedClasses) { + SecurityMemberAccessConfig config = new SecurityMemberAccessConfig(); + config.useDevMode(String.valueOf(devMode)); + config.useExcludedClasses(excludedClasses); + config.useDevModeExcludedClasses(devModeExcludedClasses); + config.init(); + return config; + } + + @Test + public void excludedClassesMatchLegacyAccumulation() { + SecurityMemberAccessConfig config = new SecurityMemberAccessConfig(); + config.useExcludedClasses("java.lang.Runtime,java.lang.ProcessBuilder"); + config.init(); + + assertEquals(legacyExcludedClassAccumulation(true, "java.lang.Runtime,java.lang.ProcessBuilder"), + config.getExcludedClasses()); + } + + @Test + public void disallowingStaticFieldAccessAddsClassToExclusions() { + SecurityMemberAccessConfig config = new SecurityMemberAccessConfig(); + config.useAllowStaticFieldAccess("false"); + config.useExcludedClasses("java.lang.Runtime"); + config.init(); + + assertFalse(config.isAllowStaticFieldAccess()); + assertEquals(legacyExcludedClassAccumulation(false, "java.lang.Runtime"), config.getExcludedClasses()); + } + + /** + * The container iterates getDeclaredMethods(), whose order the JDK leaves unspecified. + * The accumulation must therefore be commutative, as it was before WW-5675. + */ + @Test + public void setterOrderDoesNotAffectExcludedClasses() { + SecurityMemberAccessConfig forward = new SecurityMemberAccessConfig(); + forward.useAllowStaticFieldAccess("false"); + forward.useExcludedClasses("java.lang.Runtime"); + forward.init(); + + SecurityMemberAccessConfig reverse = new SecurityMemberAccessConfig(); + reverse.useExcludedClasses("java.lang.Runtime"); + reverse.useAllowStaticFieldAccess("false"); + reverse.init(); + + assertEquals(forward.getExcludedClasses(), reverse.getExcludedClasses()); + } + + @Test + public void devModeDisabledPublishesNormalExclusions() { + SecurityMemberAccessConfig config = configWith(false, "java.lang.Runtime", "java.lang.ProcessBuilder"); + + assertTrue(config.getExcludedClasses().contains("java.lang.Runtime")); + assertFalse(config.getExcludedClasses().contains("java.lang.ProcessBuilder")); + } + + @Test + public void devModeEnabledPublishesDevModeExclusions() { + SecurityMemberAccessConfig config = configWith(true, "java.lang.Runtime", "java.lang.ProcessBuilder"); + + assertTrue(config.getExcludedClasses().contains("java.lang.ProcessBuilder")); + assertFalse(config.getExcludedClasses().contains("java.lang.Runtime")); + } + + @Test + public void packageNamesAreStrippedOfDots() { + SecurityMemberAccessConfig config = new SecurityMemberAccessConfig(); + config.useExcludedPackageNames("java.io.,.java.net"); + config.init(); + + assertTrue(config.getExcludedPackageNames().contains("java.io")); + assertTrue(config.getExcludedPackageNames().contains("java.net")); + } + + @Test + public void patternsAreCompiledOnce() { + SecurityMemberAccessConfig config = new SecurityMemberAccessConfig(); + config.useExcludedPackageNamePatterns("^java\\.lang\\..*"); + config.init(); + + Set patterns = config.getExcludedPackageNamePatterns(); + assertEquals(1, patterns.size()); + assertTrue(patterns.iterator().next().matcher("java.lang.Runtime").matches()); + } + + /** + * A missing init() must fail closed: production exclusions, never the dev-mode ones. + */ + @Test + public void withoutInitTheNormalExclusionsApply() { + SecurityMemberAccessConfig config = new SecurityMemberAccessConfig(); + config.useDevMode("true"); + config.useExcludedClasses("java.lang.Runtime"); + config.useDevModeExcludedClasses("java.lang.ProcessBuilder"); + + assertTrue(config.getExcludedClasses().contains("java.lang.Runtime")); + } + + @Test + public void allowlistPackageNamesUnionDefaultsToRequiredPackagesOnly() { + SecurityMemberAccessConfig config = new SecurityMemberAccessConfig(); + + assertEquals(Set.of("org.apache.struts2.validator.validators", + "org.apache.struts2.components", + "org.apache.struts2.views.jsp"), + config.getAllowlistPackageNamesUnion()); + } + + @Test + public void allowlistPackageNamesUnionRetainsRequiredPackagesWhenConfigured() { + SecurityMemberAccessConfig config = new SecurityMemberAccessConfig(); + config.useAllowlistPackageNames("com.example.app"); + + assertTrue(config.getAllowlistPackageNamesUnion().contains("com.example.app")); + assertTrue(config.getAllowlistPackageNamesUnion().contains("org.apache.struts2.components")); + assertTrue(config.getAllowlistPackageNamesUnion().contains("org.apache.struts2.validator.validators")); + assertTrue(config.getAllowlistPackageNamesUnion().contains("org.apache.struts2.views.jsp")); + } +} diff --git a/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessPackageMatchingTest.java b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessPackageMatchingTest.java index 413b0c6695..3dfa540dd4 100644 --- a/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessPackageMatchingTest.java +++ b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessPackageMatchingTest.java @@ -117,21 +117,21 @@ private static List> classShapes() throws Exception { public void siblingPackageWithSharedCharacterPrefixDoesNotMatch() { Set excluded = Set.of("org.apache.struts2"); - assertThat(SecurityMemberAccess.isPackageBelongsToPackages("org.apache.struts2x", excluded, emptySet())) + assertThat(SecurityMemberAccess.isPackageBelongsToPackages("org.apache.struts2x", excluded)) .as("a sibling package sharing a character prefix must not match (production)") .isFalse(); assertThat(legacyPrefixMatch("org.apache.struts2x", excluded)) .as("a sibling package sharing a character prefix must not match (legacy oracle)") .isFalse(); - assertThat(SecurityMemberAccess.isPackageBelongsToPackages("org.apache.struts2", excluded, emptySet())) + assertThat(SecurityMemberAccess.isPackageBelongsToPackages("org.apache.struts2", excluded)) .as("an exact match must match (production)") .isTrue(); assertThat(legacyPrefixMatch("org.apache.struts2", excluded)) .as("an exact match must match (legacy oracle)") .isTrue(); - assertThat(SecurityMemberAccess.isPackageBelongsToPackages("org.apache.struts2.ognl", excluded, emptySet())) + assertThat(SecurityMemberAccess.isPackageBelongsToPackages("org.apache.struts2.ognl", excluded)) .as("a sub-package must match (production)") .isTrue(); assertThat(legacyPrefixMatch("org.apache.struts2.ognl", excluded)) @@ -192,7 +192,7 @@ public void classEntryPointMatchesLegacyAcrossCandidateSets() throws Exception { public void indexWalkMatchesLegacyAcrossPackageNameShapes() { for (String packageName : PACKAGE_NAMES) { for (Set candidates : CANDIDATE_SETS) { - assertThat(SecurityMemberAccess.isPackageBelongsToPackages(packageName, candidates, emptySet())) + assertThat(SecurityMemberAccess.isPackageBelongsToPackages(packageName, candidates)) .as("packageName=[%s] candidates=%s", packageName, candidates) .isEqualTo(legacyPrefixMatch(packageName, candidates)); } @@ -200,25 +200,37 @@ public void indexWalkMatchesLegacyAcrossPackageNameShapes() { } @Test - public void bothSetsEmptyShortCircuitsToFalse() { + public void emptyCandidateSetShortCircuitsToFalse() { for (String packageName : PACKAGE_NAMES) { - assertThat(SecurityMemberAccess.isPackageBelongsToPackages(packageName, emptySet(), emptySet())) + assertThat(SecurityMemberAccess.isPackageBelongsToPackages(packageName, emptySet())) .as("packageName=[%s] with no configured packages", packageName) .isFalse(); } } + /** + * The union must never lose ALLOWLIST_REQUIRED_PACKAGES. Dropping them would be a silent + * fail-open: Struts' own components would stop being allowlisted with nothing failing loudly. + */ @Test - public void twoSetOverloadEqualsDisjunctionOfSingleSetCalls() throws Exception { - for (Class clazz : classShapes()) { - for (Set first : CANDIDATE_SETS) { - for (Set second : CANDIDATE_SETS) { - assertThat(isClassBelongsToPackages(clazz, first, second)) - .as("clazz=[%s] first=%s second=%s", clazz.getName(), first, second) - .isEqualTo(isClassBelongsToPackages(clazz, first) - || isClassBelongsToPackages(clazz, second)); - } - } - } + public void allowlistUnionRetainsRequiredPackagesAfterSetterCall() throws Exception { + SecurityMemberAccess sma = new SecurityMemberAccess(null, null); + sma.useAllowlistPackageNames("com.example.app"); + + Set union = SecurityMemberAccessTest.reflectField(sma, "allowlistPackageNamesUnion"); + + assertThat(union).contains("com.example.app", "org.apache.struts2.components"); + } + + @Test + public void allowlistUnionContainsRequiredPackagesByDefault() throws Exception { + SecurityMemberAccess sma = new SecurityMemberAccess(null, null); + + Set union = SecurityMemberAccessTest.reflectField(sma, "allowlistPackageNamesUnion"); + + assertThat(union).contains( + "org.apache.struts2.components", + "org.apache.struts2.views.jsp", + "org.apache.struts2.validator.validators"); } } diff --git a/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessTest.java b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessTest.java index a9b7b8c12d..0338636316 100644 --- a/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessTest.java +++ b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessTest.java @@ -111,7 +111,7 @@ public void configurationCollectionsImmutable() throws Exception { "excludedPackageNamePatterns", "excludedPackageExemptClasses", "allowlistClasses", - "allowlistPackageNames", + "allowlistPackageNamesUnion", "excludeProperties", "acceptProperties"); for (String field : fields) { diff --git a/core/src/test/java/org/apache/struts2/util/ConfigParseUtilTest.java b/core/src/test/java/org/apache/struts2/util/ConfigParseUtilTest.java index 84cf9a0a33..e495e6902f 100644 --- a/core/src/test/java/org/apache/struts2/util/ConfigParseUtilTest.java +++ b/core/src/test/java/org/apache/struts2/util/ConfigParseUtilTest.java @@ -29,11 +29,13 @@ import java.util.HashMap; import java.util.HashSet; import java.util.LinkedHashSet; +import java.util.List; import java.util.Map; import java.util.Set; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; @@ -187,6 +189,37 @@ public String toString() { innerCache.estimatedSize() <= limit); } + @Test + public void validatePackageNamesAcceptsNamesWithoutWhitespace() { + ConfigParseUtil.validatePackageNames(Set.of("java.lang", "org.apache.struts2", "")); + } + + @Test + public void validatePackageNamesRejectsSpace() { + Set packageNames = Set.of("java.lang", "org.apache struts2"); + assertThrows(ConfigurationException.class, + () -> ConfigParseUtil.validatePackageNames(packageNames)); + } + + @Test + public void validatePackageNamesRejectsTab() { + Set packageNames = Set.of("java\tlang"); + assertThrows(ConfigurationException.class, + () -> ConfigParseUtil.validatePackageNames(packageNames)); + } + + @Test + public void validatePackageNamesRejectsNewline() { + Set packageNames = Set.of("java\nlang"); + assertThrows(ConfigurationException.class, + () -> ConfigParseUtil.validatePackageNames(packageNames)); + } + + @Test + public void validatePackageNamesAcceptsEmptyCollection() { + ConfigParseUtil.validatePackageNames(List.of()); + } + @SuppressWarnings("unchecked") private static Cache validatedClassCache() { try { diff --git a/docs/superpowers/plans/2026-08-14-WW-5675-share-parsed-ognl-security-config.md b/docs/superpowers/plans/2026-08-14-WW-5675-share-parsed-ognl-security-config.md new file mode 100644 index 0000000000..bb6361f024 --- /dev/null +++ b/docs/superpowers/plans/2026-08-14-WW-5675-share-parsed-ognl-security-config.md @@ -0,0 +1,1118 @@ +# WW-5675 Share Parsed OGNL Security Configuration — Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Parse the OGNL security configuration once per container instead of once per `SecurityMemberAccess` instantiation, without changing OGNL allow/deny semantics. + +**Architecture:** A new `Scope.SINGLETON` bean, `SecurityMemberAccessConfig`, takes over all sixteen `@Inject` configuration setters and does all parsing once per container, resolving dev-mode in `Initializable.init()`. `SecurityMemberAccess` stays `Scope.PROTOTYPE` and receives that bean through a single `@Inject` setter, copying immutable set references. The dev-mode lazy flip is deleted from the access path, and the allowlist two-set walk collapses into one precomputed union. + +**Tech Stack:** Java 17 (`maven.compiler.release=17`), Maven, JUnit 4 (`org.junit.Test`), AssertJ, Mockito, Log4j2, Caffeine. + +**Spec:** `docs/superpowers/specs/2026-08-14-WW-5675-security-member-access-config-sharing-design.md` + +## Global Constraints + +- **Branch:** `WW-5675-share-parsed-ognl-security-config`. Never push to `main`; finish via a PR. +- **Commit format:** `WW-5675 (): `, e.g. `WW-5675 perf(ognl): share parsed config across instances`. Every commit ends with the `Co-Authored-By: Claude Opus 5 ` trailer. +- **Never `git add -A` or `git add .`** in this repo — the tree carries roughly twenty long-lived untracked files. Stage explicit paths and verify with `git diff --cached --name-only` before every commit. +- **Core tests are JUnit 4** (`org.junit.Test`, `org.junit.Before`) or extend `XWorkTestCase`. A JUnit 5 `@Test` added to these suites silently never runs. +- **OGNL allow/deny semantics must not change.** No configuration may become more permissive. This is a security gate. +- **Test command:** `mvn test -DskipAssembly -pl core -Dtest=ClassName#methodName` +- **Full module suite:** `mvn test -DskipAssembly -pl core` +- Target version 7.4.0. + +--- + +## File Structure + +| File | Responsibility | +|---|---| +| `core/src/main/java/org/apache/struts2/util/ConfigParseUtil.java` | Modify: hoist the whitespace `Pattern` to a constant | +| `core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccessConfig.java` | **Create**: owns all config parsing and dev-mode resolution, one instance per container | +| `core/src/main/java/org/apache/struts2/config/impl/DefaultConfiguration.java` | Modify: register the new bean as `Scope.SINGLETON` | +| `core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java` | Modify: receive config by setter, deprecate eleven setters, delete dev-mode state, collapse the allowlist union | +| `core/src/test/java/org/apache/struts2/util/ConfigParseUtilTest.java` | Test: whitespace validation behaviour preserved | +| `core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigTest.java` | **Create**: differential parsing + dev-mode resolution | +| `core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigSharingTest.java` | **Create**: sharing proof, instance isolation, subclass injection | + +--- + +### Task 1: Hoist the whitespace pattern in `ConfigParseUtil` + +`validatePackageNames` currently calls `Pattern.compile("\\s")` once per package name — roughly 58 recompiles of a trivial pattern per `SecurityMemberAccess` instantiation under the default configuration. + +**Files:** +- Modify: `core/src/main/java/org/apache/struts2/util/ConfigParseUtil.java:142-146` +- Test: `core/src/test/java/org/apache/struts2/util/ConfigParseUtilTest.java` + +**Interfaces:** +- Consumes: nothing. +- Produces: no signature change. `public static void validatePackageNames(Collection packageNames)` keeps its exact behaviour and throws `ConfigurationException` on any whitespace. + +- [ ] **Step 1: Write the failing test** + +Create `core/src/test/java/org/apache/struts2/util/ConfigParseUtilTest.java` if it does not exist; otherwise append these methods to the existing class. If creating it, use the standard ASF license header copied verbatim from `ConfigParseUtil.java` lines 1-18. + +```java +package org.apache.struts2.util; + +import org.apache.struts2.config.ConfigurationException; +import org.junit.Test; + +import java.util.List; +import java.util.Set; + +import static org.junit.Assert.assertThrows; + +public class ConfigParseUtilTest { + + @Test + public void validatePackageNamesAcceptsNamesWithoutWhitespace() { + ConfigParseUtil.validatePackageNames(Set.of("java.lang", "org.apache.struts2", "")); + } + + @Test + public void validatePackageNamesRejectsSpace() { + assertThrows(ConfigurationException.class, + () -> ConfigParseUtil.validatePackageNames(Set.of("java.lang", "org.apache struts2"))); + } + + @Test + public void validatePackageNamesRejectsTab() { + assertThrows(ConfigurationException.class, + () -> ConfigParseUtil.validatePackageNames(Set.of("java\tlang"))); + } + + @Test + public void validatePackageNamesRejectsNewline() { + assertThrows(ConfigurationException.class, + () -> ConfigParseUtil.validatePackageNames(Set.of("java\nlang"))); + } + + @Test + public void validatePackageNamesAcceptsEmptyCollection() { + ConfigParseUtil.validatePackageNames(List.of()); + } +} +``` + +- [ ] **Step 2: Run the tests to verify they pass against the current implementation** + +Run: `mvn test -DskipAssembly -pl core -Dtest=ConfigParseUtilTest` +Expected: PASS. These tests characterise existing behaviour before the refactor — they are the guard, not a red test. Do not proceed if any fails; that would mean the characterisation is wrong. + +- [ ] **Step 3: Hoist the pattern** + +In `core/src/main/java/org/apache/struts2/util/ConfigParseUtil.java`, add the constant next to the existing cache constants near line 45: + +```java + private static final Pattern WHITESPACE = Pattern.compile("\\s"); +``` + +Then replace the body of `validatePackageNames`: + +```java + public static void validatePackageNames(Collection packageNames) { + if (packageNames.stream().anyMatch(s -> WHITESPACE.matcher(s).find())) { + throw new ConfigurationException("Excluded package names could not be parsed due to erroneous whitespace characters: " + packageNames); + } + } +``` + +- [ ] **Step 4: Run the tests again** + +Run: `mvn test -DskipAssembly -pl core -Dtest=ConfigParseUtilTest` +Expected: PASS, identical results to Step 2. + +- [ ] **Step 5: Commit** + +```bash +git add core/src/main/java/org/apache/struts2/util/ConfigParseUtil.java core/src/test/java/org/apache/struts2/util/ConfigParseUtilTest.java +git diff --cached --name-only +git commit -m "WW-5675 perf(config): hoist the whitespace pattern in validatePackageNames + +Co-Authored-By: Claude Opus 5 " +``` + +--- + +### Task 2: Create the `SecurityMemberAccessConfig` bean + +A standalone bean that owns all parsing. It does not touch `SecurityMemberAccess` yet, so it is independently testable. + +**Files:** +- Create: `core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccessConfig.java` +- Test: `core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigTest.java` + +**Interfaces:** +- Consumes: `ConfigParseUtil.toClassesSet`, `toClassObjectsSet`, `toNewClassesSet`, `toNewPatternsSet`, `toNewPackageNamesSet`, `toPackageNamesSet` (all `public static`, unchanged). +- Produces, relied on by Task 3: + - `boolean isAllowStaticFieldAccess()` + - `Set getExcludedClasses()` + - `Set getExcludedPackageNamePatterns()` + - `Set getExcludedPackageNames()` + - `Set getExcludedPackageExemptClasses()` + - `boolean isEnforceAllowlistEnabled()` + - `Set> getAllowlistClasses()` + - `Set getAllowlistPackageNames()` + - `boolean isDisallowProxyObjectAccess()` + - `boolean isDisallowProxyMemberAccess()` + - `boolean isDisallowDefaultPackageAccess()` + + The four excluded-* getters return the **effective** sets, with dev-mode already applied. + +- [ ] **Step 1: Write the failing test** + +Create `core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigTest.java` with the ASF license header copied verbatim from `SecurityMemberAccess.java` lines 1-18. + +The `legacy*` methods below are **frozen oracles**: verbatim copies of the accumulation logic they replace. They must never be deleted, nor rewritten to delegate to production code — that would make the differential vacuous. This mirrors the approach used in `SecurityMemberAccessPackageMatchingTest` for WW-5674. + +```java +package org.apache.struts2.ognl; + +import org.junit.Test; + +import java.util.Set; +import java.util.regex.Pattern; + +import static org.apache.struts2.util.ConfigParseUtil.toNewClassesSet; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +public class SecurityMemberAccessConfigTest { + + /** + * Frozen oracle: the accumulation SecurityMemberAccess performed before WW-5675. + * Never delete this, and never make it delegate to production code. + */ + private static Set legacyExcludedClassAccumulation(boolean allowStaticFieldAccess, String configured) { + Set excludedClasses = Set.of(Object.class.getName()); + if (!allowStaticFieldAccess) { + excludedClasses = toNewClassesSet(excludedClasses, Class.class.getName()); + } + return toNewClassesSet(excludedClasses, configured); + } + + private SecurityMemberAccessConfig configWith(boolean devMode, String excludedClasses, String devModeExcludedClasses) { + SecurityMemberAccessConfig config = new SecurityMemberAccessConfig(); + config.useDevMode(String.valueOf(devMode)); + config.useExcludedClasses(excludedClasses); + config.useDevModeExcludedClasses(devModeExcludedClasses); + config.init(); + return config; + } + + @Test + public void excludedClassesMatchLegacyAccumulation() { + SecurityMemberAccessConfig config = new SecurityMemberAccessConfig(); + config.useExcludedClasses("java.lang.Runtime,java.lang.ProcessBuilder"); + config.init(); + + assertEquals(legacyExcludedClassAccumulation(true, "java.lang.Runtime,java.lang.ProcessBuilder"), + config.getExcludedClasses()); + } + + @Test + public void disallowingStaticFieldAccessAddsClassToExclusions() { + SecurityMemberAccessConfig config = new SecurityMemberAccessConfig(); + config.useAllowStaticFieldAccess("false"); + config.useExcludedClasses("java.lang.Runtime"); + config.init(); + + assertFalse(config.isAllowStaticFieldAccess()); + assertEquals(legacyExcludedClassAccumulation(false, "java.lang.Runtime"), config.getExcludedClasses()); + } + + /** + * The container iterates getDeclaredMethods(), whose order the JDK leaves unspecified. + * The accumulation must therefore be commutative, as it was before WW-5675. + */ + @Test + public void setterOrderDoesNotAffectExcludedClasses() { + SecurityMemberAccessConfig forward = new SecurityMemberAccessConfig(); + forward.useAllowStaticFieldAccess("false"); + forward.useExcludedClasses("java.lang.Runtime"); + forward.init(); + + SecurityMemberAccessConfig reverse = new SecurityMemberAccessConfig(); + reverse.useExcludedClasses("java.lang.Runtime"); + reverse.useAllowStaticFieldAccess("false"); + reverse.init(); + + assertEquals(forward.getExcludedClasses(), reverse.getExcludedClasses()); + } + + @Test + public void devModeDisabledPublishesNormalExclusions() { + SecurityMemberAccessConfig config = configWith(false, "java.lang.Runtime", "java.lang.ProcessBuilder"); + + assertTrue(config.getExcludedClasses().contains("java.lang.Runtime")); + assertFalse(config.getExcludedClasses().contains("java.lang.ProcessBuilder")); + } + + @Test + public void devModeEnabledPublishesDevModeExclusions() { + SecurityMemberAccessConfig config = configWith(true, "java.lang.Runtime", "java.lang.ProcessBuilder"); + + assertTrue(config.getExcludedClasses().contains("java.lang.ProcessBuilder")); + assertFalse(config.getExcludedClasses().contains("java.lang.Runtime")); + } + + @Test + public void packageNamesAreStrippedOfDots() { + SecurityMemberAccessConfig config = new SecurityMemberAccessConfig(); + config.useExcludedPackageNames("java.io.,.java.net"); + config.init(); + + assertTrue(config.getExcludedPackageNames().contains("java.io")); + assertTrue(config.getExcludedPackageNames().contains("java.net")); + } + + @Test + public void patternsAreCompiledOnce() { + SecurityMemberAccessConfig config = new SecurityMemberAccessConfig(); + config.useExcludedPackageNamePatterns("^java\\.lang\\..*"); + config.init(); + + Set patterns = config.getExcludedPackageNamePatterns(); + assertEquals(1, patterns.size()); + assertTrue(patterns.iterator().next().matcher("java.lang.Runtime").matches()); + } + + /** + * A missing init() must fail closed: production exclusions, never the dev-mode ones. + */ + @Test + public void withoutInitTheNormalExclusionsApply() { + SecurityMemberAccessConfig config = new SecurityMemberAccessConfig(); + config.useDevMode("true"); + config.useExcludedClasses("java.lang.Runtime"); + config.useDevModeExcludedClasses("java.lang.ProcessBuilder"); + + assertTrue(config.getExcludedClasses().contains("java.lang.Runtime")); + } +} +``` + +- [ ] **Step 2: Run the test to verify it fails** + +Run: `mvn test -DskipAssembly -pl core -Dtest=SecurityMemberAccessConfigTest` +Expected: FAIL — compilation error, `SecurityMemberAccessConfig` does not exist. + +- [ ] **Step 3: Create the bean** + +Create `core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccessConfig.java` with the ASF license header copied verbatim from `SecurityMemberAccess.java` lines 1-18. + +Note that `init()` overwrites the normal fields with the dev-mode ones, exactly mirroring the `useDevModeConfiguration()` method it replaces. This is deliberate: if `init()` never runs, the normal production exclusions remain in force, which fails closed. + +```java +package org.apache.struts2.ognl; + +import org.apache.commons.lang3.BooleanUtils; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; +import org.apache.struts2.StrutsConstants; +import org.apache.struts2.inject.Inject; +import org.apache.struts2.inject.Initializable; + +import java.util.Set; +import java.util.regex.Pattern; + +import static java.util.Collections.emptySet; +import static org.apache.struts2.StrutsConstants.STRUTS_ALLOWLIST_CLASSES; +import static org.apache.struts2.StrutsConstants.STRUTS_ALLOWLIST_PACKAGE_NAMES; +import static org.apache.struts2.util.ConfigParseUtil.toClassObjectsSet; +import static org.apache.struts2.util.ConfigParseUtil.toClassesSet; +import static org.apache.struts2.util.ConfigParseUtil.toNewClassesSet; +import static org.apache.struts2.util.ConfigParseUtil.toNewPackageNamesSet; +import static org.apache.struts2.util.ConfigParseUtil.toNewPatternsSet; +import static org.apache.struts2.util.ConfigParseUtil.toPackageNamesSet; +import static org.apache.struts2.util.DebugUtils.logWarningForFirstOccurrence; + +/** + * Holds the parsed OGNL security configuration for one container. + *

+ * {@link SecurityMemberAccess} is a {@code Scope.PROTOTYPE} bean, constructed once per value stack and + * again for each OGNL context. Parsing the roughly ninety configuration entries on every one of those + * was the dominant cost identified by WW-5667. This bean is a {@code Scope.SINGLETON}, so the parsing + * happens once per container and each {@code SecurityMemberAccess} merely copies immutable references. + *

+ * Dev-mode is resolved in {@link #init()} rather than in a setter, because the container iterates + * {@code getDeclaredMethods()}, whose order the JDK leaves unspecified. If {@code init()} never runs, + * the normal production exclusions stay in force, which fails closed. + * + * @since Struts 7.4.0 + */ +public class SecurityMemberAccessConfig implements Initializable { + + private static final Logger LOG = LogManager.getLogger(SecurityMemberAccessConfig.class); + + private boolean allowStaticFieldAccess = true; + + private Set excludedClasses = Set.of(Object.class.getName()); + private Set excludedPackageNamePatterns = emptySet(); + private Set excludedPackageNames = emptySet(); + private Set excludedPackageExemptClasses = emptySet(); + + private boolean isDevMode; + private Set devModeExcludedClasses = Set.of(Object.class.getName()); + private Set devModeExcludedPackageNamePatterns = emptySet(); + private Set devModeExcludedPackageNames = emptySet(); + private Set devModeExcludedPackageExemptClasses = emptySet(); + + private boolean enforceAllowlistEnabled = false; + private Set> allowlistClasses = emptySet(); + private Set allowlistPackageNames = emptySet(); + + private boolean disallowProxyObjectAccess = false; + private boolean disallowProxyMemberAccess = false; + private boolean disallowDefaultPackageAccess = false; + + @Override + public void init() { + if (!isDevMode) { + return; + } + logWarningForFirstOccurrence("devMode", LOG, + "DevMode enabled, using DevMode excluded classes and packages for OGNL security enforcement!"); + excludedClasses = devModeExcludedClasses; + excludedPackageNamePatterns = devModeExcludedPackageNamePatterns; + excludedPackageNames = devModeExcludedPackageNames; + excludedPackageExemptClasses = devModeExcludedPackageExemptClasses; + } + + @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) { + 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); + } + + @Inject(value = StrutsConstants.STRUTS_EXCLUDED_PACKAGE_EXEMPT_CLASSES, required = false) + 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); + if (!this.enforceAllowlistEnabled) { + String msg = "OGNL allowlist is disabled!" + + " We strongly recommend keeping it enabled to protect against critical vulnerabilities." + + " Set the configuration `{}=true` to enable it." + + " Please refer to the Struts 7.0 migration guide and security documentation for further information."; + logWarningForFirstOccurrence("allowlist", LOG, msg, StrutsConstants.STRUTS_ALLOWLIST_ENABLE); + } + } + + @Inject(value = STRUTS_ALLOWLIST_CLASSES, required = false) + public void useAllowlistClasses(String commaDelimitedClasses) { + this.allowlistClasses = toClassObjectsSet(commaDelimitedClasses); + } + + @Inject(value = STRUTS_ALLOWLIST_PACKAGE_NAMES, required = false) + public void useAllowlistPackageNames(String commaDelimitedPackageNames) { + this.allowlistPackageNames = toPackageNamesSet(commaDelimitedPackageNames); + } + + @Inject(value = StrutsConstants.STRUTS_DISALLOW_PROXY_OBJECT_ACCESS, required = false) + public void useDisallowProxyObjectAccess(String disallowProxyObjectAccess) { + this.disallowProxyObjectAccess = BooleanUtils.toBoolean(disallowProxyObjectAccess); + } + + @Inject(value = StrutsConstants.STRUTS_DISALLOW_PROXY_MEMBER_ACCESS, required = false) + public void useDisallowProxyMemberAccess(String disallowProxyMemberAccess) { + this.disallowProxyMemberAccess = BooleanUtils.toBoolean(disallowProxyMemberAccess); + } + + @Inject(value = StrutsConstants.STRUTS_DISALLOW_DEFAULT_PACKAGE_ACCESS, required = false) + public void useDisallowDefaultPackageAccess(String disallowDefaultPackageAccess) { + this.disallowDefaultPackageAccess = BooleanUtils.toBoolean(disallowDefaultPackageAccess); + } + + @Inject(StrutsConstants.STRUTS_DEVMODE) + public void useDevMode(String devMode) { + this.isDevMode = BooleanUtils.toBoolean(devMode); + } + + @Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_CLASSES, required = false) + public void useDevModeExcludedClasses(String commaDelimitedClasses) { + this.devModeExcludedClasses = toNewClassesSet(devModeExcludedClasses, commaDelimitedClasses); + } + + @Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_PACKAGE_NAME_PATTERNS, required = false) + public void useDevModeExcludedPackageNamePatterns(String commaDelimitedPackagePatterns) { + this.devModeExcludedPackageNamePatterns = toNewPatternsSet(devModeExcludedPackageNamePatterns, commaDelimitedPackagePatterns); + } + + @Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_PACKAGE_NAMES, required = false) + public void useDevModeExcludedPackageNames(String commaDelimitedPackageNames) { + this.devModeExcludedPackageNames = toNewPackageNamesSet(devModeExcludedPackageNames, commaDelimitedPackageNames); + } + + @Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_PACKAGE_EXEMPT_CLASSES, required = false) + public void useDevModeExcludedPackageExemptClasses(String commaDelimitedClasses) { + this.devModeExcludedPackageExemptClasses = toClassesSet(commaDelimitedClasses); + } + + public boolean isAllowStaticFieldAccess() { + return allowStaticFieldAccess; + } + + public Set getExcludedClasses() { + return excludedClasses; + } + + public Set getExcludedPackageNamePatterns() { + return excludedPackageNamePatterns; + } + + public Set getExcludedPackageNames() { + return excludedPackageNames; + } + + public Set getExcludedPackageExemptClasses() { + return excludedPackageExemptClasses; + } + + public boolean isEnforceAllowlistEnabled() { + return enforceAllowlistEnabled; + } + + public Set> getAllowlistClasses() { + return allowlistClasses; + } + + public Set getAllowlistPackageNames() { + return allowlistPackageNames; + } + + public boolean isDisallowProxyObjectAccess() { + return disallowProxyObjectAccess; + } + + public boolean isDisallowProxyMemberAccess() { + return disallowProxyMemberAccess; + } + + public boolean isDisallowDefaultPackageAccess() { + return disallowDefaultPackageAccess; + } +} +``` + +- [ ] **Step 4: Run the test to verify it passes** + +Run: `mvn test -DskipAssembly -pl core -Dtest=SecurityMemberAccessConfigTest` +Expected: PASS, 9 tests. + +- [ ] **Step 5: Commit** + +```bash +git add core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccessConfig.java core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigTest.java +git diff --cached --name-only +git commit -m "WW-5675 feat(ognl): add a container-singleton OGNL security config bean + +Co-Authored-By: Claude Opus 5 " +``` + +--- + +### Task 3: Register the bean and wire it into `SecurityMemberAccess` + +`SecurityMemberAccess` starts reading the shared configuration. Its own setters lose `@Inject` and become deprecated, but keep mutating the instance so the roughly 110 existing direct call sites behave identically. + +**Files:** +- Modify: `core/src/main/java/org/apache/struts2/config/impl/DefaultConfiguration.java:416-420` +- Modify: `core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java:473-536` +- Test: `core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigSharingTest.java` + +**Interfaces:** +- Consumes: every getter from Task 2. +- Produces: `public void useConfig(SecurityMemberAccessConfig config)` on `SecurityMemberAccess`, annotated `@Inject`. Task 4 removes the dev-mode setters; Task 5 changes the allowlist walk. + +- [ ] **Step 1: Write the failing test** + +Create `core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigSharingTest.java` with the ASF license header copied verbatim from `SecurityMemberAccess.java` lines 1-18. + +`XWorkTestCase` lives in `core/src/main/java/org/apache/struts2/XWorkTestCase.java` and exposes `protected Container container`. It is a JUnit 3 style `TestCase`, so test methods must be named `testXxx` — an `@Test` annotation alone will not run them. + +```java +package org.apache.struts2.ognl; + +import org.apache.struts2.XWorkTestCase; + +import java.util.Set; + +public class SecurityMemberAccessConfigSharingTest extends XWorkTestCase { + + /** + * Reference identity proves no re-parsing occurred: any re-parse necessarily + * allocates a fresh set. + */ + public void testConfigDerivedSetsAreSharedAcrossInstances() throws Exception { + SecurityMemberAccess first = container.getInstance(SecurityMemberAccess.class); + SecurityMemberAccess second = container.getInstance(SecurityMemberAccess.class); + + assertNotSame("expected a prototype bean", first, second); + + Set firstExcluded = SecurityMemberAccessTest.reflectField(first, "excludedClasses"); + Set secondExcluded = SecurityMemberAccessTest.reflectField(second, "excludedClasses"); + assertSame("excluded classes were re-parsed per instance", firstExcluded, secondExcluded); + + Set firstPackages = SecurityMemberAccessTest.reflectField(first, "excludedPackageNames"); + Set secondPackages = SecurityMemberAccessTest.reflectField(second, "excludedPackageNames"); + assertSame("excluded package names were re-parsed per instance", firstPackages, secondPackages); + } + + public void testConfigBeanIsASingleton() { + assertSame(container.getInstance(SecurityMemberAccessConfig.class), + container.getInstance(SecurityMemberAccessConfig.class)); + } + + /** + * The shared sets must not be perturbed by a deprecated setter call on one instance. + */ + public void testDeprecatedSetterDoesNotLeakToSiblings() throws Exception { + SecurityMemberAccess mutated = container.getInstance(SecurityMemberAccess.class); + SecurityMemberAccess untouched = container.getInstance(SecurityMemberAccess.class); + SecurityMemberAccessConfig config = container.getInstance(SecurityMemberAccessConfig.class); + + Set before = SecurityMemberAccessTest.reflectField(untouched, "excludedClasses"); + mutated.useExcludedClasses("java.lang.Runtime"); + Set after = SecurityMemberAccessTest.reflectField(untouched, "excludedClasses"); + + assertSame("a sibling instance was affected", before, after); + assertFalse("the shared config was mutated", config.getExcludedClasses().contains("java.lang.Runtime")); + + Set mutatedSet = SecurityMemberAccessTest.reflectField(mutated, "excludedClasses"); + assertTrue("the setter did not affect its own instance", mutatedSet.contains("java.lang.Runtime")); + } + + /** + * Guards the fail-open hole avoided by using setter rather than constructor injection: + * a subclass calling the two-argument super constructor must still receive the config. + */ + public void testSubclassReceivesConfigThroughInheritedSetter() throws Exception { + SubclassedSecurityMemberAccess subclassed = new SubclassedSecurityMemberAccess( + container.getInstance(ProviderAllowlist.class), + container.getInstance(ThreadAllowlist.class)); + + container.inject(subclassed); + + Set excluded = SecurityMemberAccessTest.reflectField(subclassed, "excludedClasses"); + assertSame("subclass did not receive the shared config", + container.getInstance(SecurityMemberAccessConfig.class).getExcludedClasses(), excluded); + } + + static class SubclassedSecurityMemberAccess extends SecurityMemberAccess { + SubclassedSecurityMemberAccess(ProviderAllowlist providerAllowlist, ThreadAllowlist threadAllowlist) { + super(providerAllowlist, threadAllowlist); + } + } +} +``` + +`Container.inject(Object)` is declared at `core/src/main/java/org/apache/struts2/inject/Container.java:83`, so the call above drives the real injection path — including `ContainerImpl.addInjectors`, which recurses into superclasses at `ContainerImpl.java:97`. That recursion is exactly what this test exists to protect. + +- [ ] **Step 2: Run the test to verify it fails** + +Run: `mvn test -DskipAssembly -pl core -Dtest=SecurityMemberAccessConfigSharingTest` +Expected: FAIL — `SecurityMemberAccessConfig` is not registered in the container, and the sets are still parsed per instance so `assertSame` fails. + +- [ ] **Step 3: Register the bean** + +In `core/src/main/java/org/apache/struts2/config/impl/DefaultConfiguration.java`, inside `bootstrapFactories`, add the registration immediately after the `ThreadAllowlist` line: + +```java + .factory(ProviderAllowlist.class, Scope.SINGLETON) + .factory(ThreadAllowlist.class, Scope.SINGLETON) + .factory(SecurityMemberAccessConfig.class, Scope.SINGLETON) +``` + +Add the import alongside the existing OGNL imports near line 123: + +```java +import org.apache.struts2.ognl.SecurityMemberAccessConfig; +``` + +- [ ] **Step 4: Add the config setter to `SecurityMemberAccess`** + +In `core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java`, add this method immediately after `setProxyService` (around line 113): + +```java + /** + * Copies the shared, already-parsed configuration into this instance. This is the only injected + * member that touches the configuration fields, so the unspecified order in which the container + * iterates {@code getDeclaredMethods()} cannot affect the result. + * + * @since Struts 7.4.0 + */ + @Inject + public void useConfig(SecurityMemberAccessConfig config) { + this.allowStaticFieldAccess = config.isAllowStaticFieldAccess(); + this.excludedClasses = config.getExcludedClasses(); + this.excludedPackageNamePatterns = config.getExcludedPackageNamePatterns(); + this.excludedPackageNames = config.getExcludedPackageNames(); + this.excludedPackageExemptClasses = config.getExcludedPackageExemptClasses(); + this.enforceAllowlistEnabled = config.isEnforceAllowlistEnabled(); + this.allowlistClasses = config.getAllowlistClasses(); + this.allowlistPackageNames = config.getAllowlistPackageNames(); + this.disallowProxyObjectAccess = config.isDisallowProxyObjectAccess(); + this.disallowProxyMemberAccess = config.isDisallowProxyMemberAccess(); + this.disallowDefaultPackageAccess = config.isDisallowDefaultPackageAccess(); + } +``` + +- [ ] **Step 5: Deprecate the eleven remaining setters** + +Still in `SecurityMemberAccess.java`, for each of these methods remove the `@Inject(...)` annotation and add `@Deprecated` plus a Javadoc `@deprecated` tag. Leave every method body exactly as it is. + +Apply to: `useAllowStaticFieldAccess`, `useExcludedClasses`, `useExcludedPackageNamePatterns`, `useExcludedPackageNames`, `useExcludedPackageExemptClasses`, `useEnforceAllowlistEnabled`, `useAllowlistClasses`, `useAllowlistPackageNames`, `useDisallowProxyObjectAccess`, `useDisallowProxyMemberAccess`, `useDisallowDefaultPackageAccess`. + +The pattern for each, shown for `useExcludedClasses`: + +```java + /** + * @deprecated since 7.4.0, configuration is parsed once per container by + * {@link SecurityMemberAccessConfig}. This method still mutates this instance and is retained for + * tests and existing callers; it will be removed in Struts 8.0.0. + */ + @Deprecated + public void useExcludedClasses(String commaDelimitedClasses) { + this.excludedClasses = toNewClassesSet(excludedClasses, commaDelimitedClasses); + } +``` + +Do **not** annotate `useDevMode` or the four `useDevModeExcluded*` methods — Task 4 deletes those. Do **not** touch `useAcceptProperties` or `useExcludeProperties`; they carry per-request state, not configuration, and are not deprecated. + +Deprecation is by annotation only. Do not add runtime warnings — roughly 110 test call sites would flood the build output. + +- [ ] **Step 6: Run the new test** + +Run: `mvn test -DskipAssembly -pl core -Dtest=SecurityMemberAccessConfigSharingTest` +Expected: PASS, 4 tests. + +- [ ] **Step 7: Run the existing security suites** + +Run: `mvn test -DskipAssembly -pl core -Dtest='SecurityMemberAccessTest,ExternalSecurityMemberAccessTest,OgnlUtilTest,OgnlValueStackTest'` +Expected: PASS. Surefire needs comma separation; `+` is not valid. + +If `SecurityMemberAccessTest` fails, the deprecated setters have not kept their exact semantics — fix the setter, not the test. + +- [ ] **Step 8: Commit** + +```bash +git add core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java core/src/main/java/org/apache/struts2/config/impl/DefaultConfiguration.java core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigSharingTest.java +git diff --cached --name-only +git commit -m "WW-5675 perf(ognl): share parsed config across SecurityMemberAccess instances + +Co-Authored-By: Claude Opus 5 " +``` + +--- + +### Task 4: Delete the dev-mode state from `SecurityMemberAccess` + +The lazy dev-mode flip runs on the access path today. With dev-mode resolved once by the config bean, all of it goes. + +**Files:** +- Modify: `core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java:89-94, 264, 538-574` +- Test: `core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigSharingTest.java` + +**Interfaces:** +- Consumes: `SecurityMemberAccessConfig` getters, which already publish dev-mode-resolved sets. +- Produces: removal only. `useDevMode`, `useDevModeExcludedClasses`, `useDevModeExcludedPackageNamePatterns`, `useDevModeExcludedPackageNames`, `useDevModeExcludedPackageExemptClasses` and `useDevModeConfiguration` no longer exist on `SecurityMemberAccess`. + +- [ ] **Step 1: Write the failing test** + +Append to `SecurityMemberAccessConfigSharingTest`: + +```java + /** + * Dev-mode exclusions must be in force from the first access, with no lazy flip. + */ + public void testDevModeExclusionsApplyWithoutAnAccess() throws Exception { + loadConfigurationProviders(new StubConfigurationProvider() { + @Override + public void register(ContainerBuilder builder, LocatableProperties props) { + props.setProperty(StrutsConstants.STRUTS_DEVMODE, "true"); + props.setProperty(StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_CLASSES, "java.lang.ProcessBuilder"); + } + }); + + SecurityMemberAccess sma = container.getInstance(SecurityMemberAccess.class); + Set excluded = SecurityMemberAccessTest.reflectField(sma, "excludedClasses"); + + assertTrue("dev-mode exclusions were not applied at startup", + excluded.contains("java.lang.ProcessBuilder")); + } + + public void testDevModeMethodsAreGone() throws Exception { + for (String name : new String[]{"useDevMode", "useDevModeExcludedClasses", + "useDevModeExcludedPackageNamePatterns", "useDevModeExcludedPackageNames", + "useDevModeExcludedPackageExemptClasses", "useDevModeConfiguration"}) { + for (java.lang.reflect.Method method : SecurityMemberAccess.class.getDeclaredMethods()) { + assertFalse("SecurityMemberAccess still declares " + name, method.getName().equals(name)); + } + } + } +``` + +Add these imports to the test file: + +```java +import org.apache.struts2.StrutsConstants; +import org.apache.struts2.inject.ContainerBuilder; +import org.apache.struts2.test.StubConfigurationProvider; +import org.apache.struts2.util.location.LocatableProperties; +``` + +Note `LocatableProperties` is in `org.apache.struts2.util.location`, not `org.apache.struts2.config.entities`. + +- [ ] **Step 2: Run the test to verify it fails** + +Run: `mvn test -DskipAssembly -pl core -Dtest=SecurityMemberAccessConfigSharingTest#testDevModeMethodsAreGone` +Expected: FAIL — the methods still exist. + +- [ ] **Step 3: Delete the dev-mode state** + +In `core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java`: + +Delete these fields (lines 89-94): + +```java + private volatile boolean isDevModeInit; + private boolean isDevMode; + private Set devModeExcludedClasses = Set.of(Object.class.getName()); + private Set devModeExcludedPackageNamePatterns = emptySet(); + private Set devModeExcludedPackageNames = emptySet(); + private Set devModeExcludedPackageExemptClasses = emptySet(); +``` + +Delete the `useDevModeConfiguration()` method entirely, and its call at the top of `checkExclusionList` so that the method begins: + +```java + protected boolean checkExclusionList(Object target, Member member) { + Class memberClass = member.getDeclaringClass(); +``` + +Delete the five dev-mode setters: `useDevMode`, `useDevModeExcludedClasses`, `useDevModeExcludedPackageNamePatterns`, `useDevModeExcludedPackageNames`, `useDevModeExcludedPackageExemptClasses`. + +- [ ] **Step 4: Run the tests** + +Run: `mvn test -DskipAssembly -pl core -Dtest=SecurityMemberAccessConfigSharingTest` +Expected: PASS, 6 tests. + +- [ ] **Step 5: Run the existing security suites** + +Run: `mvn test -DskipAssembly -pl core -Dtest='SecurityMemberAccessTest,ExternalSecurityMemberAccessTest'` +Expected: PASS. If a test called a dev-mode setter directly, the earlier survey was wrong — stop and report rather than deleting the test. + +- [ ] **Step 6: Commit** + +```bash +git add core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigSharingTest.java +git diff --cached --name-only +git commit -m "WW-5675 refactor(ognl): drop the lazy dev-mode flip from the access path + +Co-Authored-By: Claude Opus 5 " +``` + +--- + +### Task 5: Collapse the allowlist two-set walk into one precomputed union + +WW-5674 merged the two allowlist walks with a three-argument helper. With the configuration shared, the union can be precomputed, so the helper reverts to a single set. + +**Files:** +- Modify: `core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java:257-263, 392-441` +- Test: `core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessPackageMatchingTest.java` + +**Interfaces:** +- Consumes: `SecurityMemberAccessConfig.getAllowlistPackageNames()`. +- Produces: `static boolean isPackageBelongsToPackages(String packageName, Set matchingPackages)` — two arguments, replacing the three-argument form. The three-argument `isClassBelongsToPackages(Class, Set, Set)` overload is deleted. + +- [ ] **Step 1: Write the failing test** + +Work in `core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessPackageMatchingTest.java`. **Do not modify or delete the frozen oracles** (`legacyPrefixMatch`, `legacyToPackageName`) or the `PACKAGE_NAMES` / `CANDIDATE_SETS` / `classShapes()` fixtures. This file uses AssertJ (`assertThat`), not JUnit assertions — match that style. + +Narrowing the signatures in this task breaks three tests already in the file. Handle them exactly as follows; do not improvise, and do not delete a test merely because it fails to compile. + +**a. Convert `indexWalkMatchesLegacyAcrossPackageNameShapes` to the two-argument walk:** + +```java + @Test + public void indexWalkMatchesLegacyAcrossPackageNameShapes() { + for (String packageName : PACKAGE_NAMES) { + for (Set candidates : CANDIDATE_SETS) { + assertThat(SecurityMemberAccess.isPackageBelongsToPackages(packageName, candidates)) + .as("packageName=[%s] candidates=%s", packageName, candidates) + .isEqualTo(legacyPrefixMatch(packageName, candidates)); + } + } + } +``` + +**b. Convert `bothSetsEmptyShortCircuitsToFalse`, renaming it since there is now one set:** + +```java + @Test + public void emptyCandidateSetShortCircuitsToFalse() { + for (String packageName : PACKAGE_NAMES) { + assertThat(SecurityMemberAccess.isPackageBelongsToPackages(packageName, emptySet())) + .as("packageName=[%s] with no configured packages", packageName) + .isFalse(); + } + } +``` + +**c. Delete `twoSetOverloadEqualsDisjunctionOfSingleSetCalls` entirely.** It characterises the equivalence of the three-argument overload against two single-set calls; that overload no longer exists, so the property it asserts is gone. This is the one test in this file you may remove. + +**d. Append these two new tests:** + +```java + /** + * The union must never lose ALLOWLIST_REQUIRED_PACKAGES. Dropping them would be a silent + * fail-open: Struts' own components would stop being allowlisted with nothing failing loudly. + */ + @Test + public void allowlistUnionRetainsRequiredPackagesAfterSetterCall() throws Exception { + SecurityMemberAccess sma = new SecurityMemberAccess(null, null); + sma.useAllowlistPackageNames("com.example.app"); + + Set union = SecurityMemberAccessTest.reflectField(sma, "allowlistPackageNamesUnion"); + + assertThat(union).contains("com.example.app", "org.apache.struts2.components"); + } + + @Test + public void allowlistUnionContainsRequiredPackagesByDefault() throws Exception { + SecurityMemberAccess sma = new SecurityMemberAccess(null, null); + + Set union = SecurityMemberAccessTest.reflectField(sma, "allowlistPackageNamesUnion"); + + assertThat(union).contains( + "org.apache.struts2.components", + "org.apache.struts2.views.jsp", + "org.apache.struts2.validator.validators"); + } +``` + +- [ ] **Step 2: Run the test to verify it fails** + +Run: `mvn test -DskipAssembly -pl core -Dtest=SecurityMemberAccessPackageMatchingTest` +Expected: FAIL — `allowlistPackageNamesUnion` does not exist and `isPackageBelongsToPackages` still takes three arguments. + +- [ ] **Step 3: Add the union field and its single computation site** + +In `SecurityMemberAccess.java`, replace the allowlist field declarations (around line 97-98): + +```java + private Set> allowlistClasses = emptySet(); + private Set allowlistPackageNames = emptySet(); + private Set allowlistPackageNamesUnion = ALLOWLIST_REQUIRED_PACKAGES; +``` + +The union defaults to `ALLOWLIST_REQUIRED_PACKAGES` so that an instance which never receives configuration — the eight direct `new SecurityMemberAccess(null, null)` test constructions — still allowlists Struts' own packages, exactly as before. + +Add the single computation site and its helper: + +```java + /** + * The only place the allowlist union is computed. Both the injected configuration and the + * deprecated setter route through here; splitting this in two would risk silently dropping + * {@code ALLOWLIST_REQUIRED_PACKAGES}, which fails open. + */ + private void applyAllowlistPackageNames(Set packageNames) { + this.allowlistPackageNames = packageNames; + this.allowlistPackageNamesUnion = union(ALLOWLIST_REQUIRED_PACKAGES, packageNames); + } + + private static Set union(Set required, Set configured) { + if (configured.isEmpty()) { + return required; + } + Set union = new HashSet<>(required); + union.addAll(configured); + return unmodifiableSet(union); + } +``` + +Add these imports: + +```java +import java.util.HashSet; + +import static java.util.Collections.unmodifiableSet; +``` + +- [ ] **Step 4: Route both callers through it** + +In `useConfig`, replace the direct assignment: + +```java + this.allowlistPackageNames = config.getAllowlistPackageNames(); +``` + +with: + +```java + applyAllowlistPackageNames(config.getAllowlistPackageNames()); +``` + +In the deprecated `useAllowlistPackageNames`, replace the body: + +```java + @Deprecated + public void useAllowlistPackageNames(String commaDelimitedPackageNames) { + applyAllowlistPackageNames(toPackageNamesSet(commaDelimitedPackageNames)); + } +``` + +- [ ] **Step 5: Collapse the walk** + +Replace the last clause of `isClassAllowlisted`: + +```java + || isClassBelongsToPackages(clazz, allowlistPackageNamesUnion); +``` + +Delete the three-argument `isClassBelongsToPackages(Class, Set, Set)` overload entirely, and rewrite the remaining pair: + +```java + public static boolean isClassBelongsToPackages(Class clazz, Set matchingPackages) { + return isPackageBelongsToPackages(toPackageName(clazz), matchingPackages); + } + + /** + * Tests whether the given package name, or any of its parent packages, is present in the set. + * Walks the name in place rather than building the full prefix list, since this runs on the OGNL + * member-access path. Shortest prefix first, so broad entries such as {@code java.io} + * short-circuit earliest. + * + *

+ * The package name must not end in {@code '.'}. Such a name is probed one prefix more than by the + * implementation this replaced, which matches more broadly — tightening exclusion but + * loosening the allowlist. {@link Class#getPackageName()} cannot produce a trailing dot, + * and {@code ConfigParseUtil.toPackageNamesSet} strips them from configured names, so every + * current caller is safe; route any other string through here only after confirming the same. + * + * @param packageName the package name to test, empty for the default package, never ending in {@code '.'} + * @param matchingPackages the package names to match against + * @return {@code true} if the package or any parent package is in the set + */ + static boolean isPackageBelongsToPackages(String packageName, Set matchingPackages) { + if (matchingPackages.isEmpty()) { + return false; + } + int idx = packageName.indexOf('.'); + while (idx != -1) { + if (matchingPackages.contains(packageName.substring(0, idx))) { + return true; + } + idx = packageName.indexOf('.', idx + 1); + } + return matchingPackages.contains(packageName); + } +``` + +Remove the now-unused `emptySet` static import only if no other usage remains — check with `grep -n "emptySet" core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java`. + +- [ ] **Step 6: Run the package-matching tests** + +Run: `mvn test -DskipAssembly -pl core -Dtest=SecurityMemberAccessPackageMatchingTest` +Expected: PASS. The file had 9 tests; one is deleted and two are added, so expect 10. + +- [ ] **Step 7: Run the security suites** + +Run: `mvn test -DskipAssembly -pl core -Dtest='SecurityMemberAccessTest,ExternalSecurityMemberAccessTest,SecurityMemberAccessConfigSharingTest,SecurityMemberAccessConfigTest'` +Expected: PASS. + +- [ ] **Step 8: Commit** + +```bash +git add core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessPackageMatchingTest.java +git diff --cached --name-only +git commit -m "WW-5675 perf(ognl): precompute the allowlist package union + +Co-Authored-By: Claude Opus 5 " +``` + +--- + +### Task 6: Verify the whole module and the plugins + +**Files:** none modified unless a failure is found. + +**Interfaces:** none. + +- [ ] **Step 1: Run the full core suite** + +Run: `mvn test -DskipAssembly -pl core` +Expected: PASS with zero failures and zero errors. For reference, the suite stood at 3158 tests when WW-5674 merged; this plan adds roughly 15. + +Record the actual counts. Do not describe the work as complete without this output in hand. + +- [ ] **Step 2: Run the plugin suites that touch `SecurityMemberAccess`** + +Run: `mvn test -DskipAssembly -pl plugins/spring,plugins/cdi` +Expected: PASS. Both modules construct `new SecurityMemberAccess(null, null)` directly in proxy tests, which exercises the un-injected path. + +- [ ] **Step 3: Confirm no stray `@Inject` remains on the deprecated setters** + +Run: + +```bash +grep -n -B2 "public void use" core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java | grep -A2 "@Inject" +``` + +Expected: only `useConfig` and `setProxyService` appear. Any other hit means a setter kept its annotation and will still be injected per instance, silently defeating the change. + +- [ ] **Step 4: Confirm the dev-mode state is gone** + +Run: + +```bash +grep -n "devMode\|DevMode" core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java +``` + +Expected: no matches. + +- [ ] **Step 5: Commit any fixes, then push and open a draft PR** + +Only if Steps 1-4 are clean: + +```bash +git push -u origin WW-5675-share-parsed-ognl-security-config +``` + +Open a draft PR titled `WW-5675 Share parsed OGNL security configuration across SecurityMemberAccess instances`, with `Fixes [WW-5675](https://issues.apache.org/jira/browse/WW-5675)` in the description. + +The PR description must state plainly that removing the five dev-mode setters is a source-breaking change in a minor release, why it was accepted, and that a Migration Guide entry is owed for 7.4.0. + +--- + +## Follow-ups (not part of this plan) + +- File a Jira Improvement for 8.0.0 to remove the eleven deprecated setters, cross-referencing WW-5675 and WW-5678. +- Add the Version Notes and Migration Guide entry for the dev-mode setter removal. +- Update WW-5667 to record that this ticket, not WW-5674, is the one expected to move the reported 9%. +- WW-5678's first item is resolved here by Task 5; the remaining visibility narrowing stays with that ticket. diff --git a/docs/superpowers/specs/2026-08-14-WW-5675-security-member-access-config-sharing-design.md b/docs/superpowers/specs/2026-08-14-WW-5675-security-member-access-config-sharing-design.md new file mode 100644 index 0000000000..8d5339adfd --- /dev/null +++ b/docs/superpowers/specs/2026-08-14-WW-5675-security-member-access-config-sharing-design.md @@ -0,0 +1,362 @@ +# WW-5675 — Share parsed OGNL security configuration across `SecurityMemberAccess` instances + +**Ticket:** [WW-5675](https://issues.apache.org/jira/browse/WW-5675) (sub-task of [WW-5667](https://issues.apache.org/jira/browse/WW-5667)) +**Target:** 7.4.0 +**Date:** 2026-08-14 +**Status:** Design approved, pending implementation plan + +## Problem + +`SecurityMemberAccess` is a `Scope.PROTOTYPE` bean, registered in two places: + +- `DefaultConfiguration.java:416` — `.factory(SecurityMemberAccess.class, Scope.PROTOTYPE)` +- `StrutsBeanSelectionProvider.java:456` — aliased to `STRUTS_MEMBER_ACCESS`, making it user-overridable + +Every `container.getInstance(SecurityMemberAccess.class)` therefore constructs a fresh instance and re-runs all +sixteen `@Inject` configuration setters, each of which re-parses a raw comma-delimited string from scratch. With +the stock `struts-excluded-classes.xml` that is roughly 90 configuration entries per instantiation: comma +splitting, `strip`, classloader validation, `Pattern.compile`, and `HashSet` accumulation. + +New instances are created on the request path from at least: + +- `OgnlValueStackFactory.createValueStack(...)` — once per value stack, and `ParametersInterceptor` creates an + additional stack per request +- `OgnlUtil.createDefaultContext(Object, ClassResolver)` at `OgnlUtil.java:738` — reached from `setProperties`, + `copy`, `getBeanMap` and friends. Note `OgnlUtil.copy` calls it **twice** (`OgnlUtil.java:551-552`), so a single + copy costs two full configuration rebuilds. + +This is the dominant half of the parent report. The sibling ticket WW-5674 (merged as `81b34c295`) addressed the +per-*access* allocations; this ticket addresses the per-*instantiation* cost, which is where the reported 9% lives. + +The fix proposed on the parent ticket — caching the parsed set in a `SecurityMemberAccess` field — cannot work, +because the instance holding the field is itself discarded and rebuilt each time. + +### Also in scope + +`ConfigParseUtil.validatePackageNames` (`ConfigParseUtil.java:143`) evaluates `Pattern.compile("\\s")` once per +package name rather than once overall — roughly 58 recompiles of a trivial pattern per instantiation under the +default configuration. Hoist it to a static constant. + +## Goals + +- Parse the OGNL security configuration once per container instead of once per `SecurityMemberAccess`. +- Preserve OGNL allow/deny semantics exactly. No configuration may become more permissive. +- Keep source compatibility for 7.4.0: existing subclasses and direct setter callers must continue to compile and + behave identically. The five dev-mode setters are the one signed-off exception — see "`SecurityMemberAccess` + changes". +- Collapse the two-set allowlist walk introduced by WW-5674 into a single precomputed set. + +## Non-goals + +- Changing array/primitive package-resolution semantics — that is WW-5676, deliberately separate because it is a + security-semantics decision rather than a performance fix. +- Removing the residual per-access `getPackage()` lookups — that is WW-5677. +- Removing the deprecated setters. They are scheduled for 8.0.0 (see Follow-ups). +- Adding JMH or any benchmarking infrastructure to the build. + +## Approach + +Introduce a container-singleton configuration bean that owns all parsing. `SecurityMemberAccess` stays +`Scope.PROTOTYPE` and copies immutable set *references* out of that bean. + +Two alternatives were considered and rejected: + +**Memoize parsing inside `ConfigParseUtil`** (keyed by raw config string, following the existing Caffeine +precedent in that file). Smallest possible diff and no API change, but it recovers the least: every instantiation +still invokes sixteen setters, still builds the accumulated `HashSet` copies, and still runs the lazy dev-mode +flip. It also does not unblock the allowlist union collapse. + +**Revert `SecurityMemberAccess` to `Scope.SINGLETON`**, relocating `acceptProperties`/`excludeProperties` into the +OGNL context. Largest theoretical win, but it reverses a deliberate WW-5343 decision, converts two fields into +shared mutable state requiring thread-safety on the OGNL security gate, and changes the `MemberAccessValueStack` +contract that `ParametersInterceptor` depends on. Under the chosen approach the per-instantiation cost is already +about a dozen reference copies, so this buys very little for substantially more risk. + +## Design + +### New bean: `SecurityMemberAccessConfig` + +Registered in `DefaultConfiguration` beside the existing internal singletons: + +```java +.factory(SecurityMemberAccessConfig.class, Scope.SINGLETON) +``` + +Concrete class, no interface, **not** aliased in `StrutsBeanSelectionProvider`. It is internal plumbing, following +the shape of `ProviderAllowlist` and `ThreadAllowlist` (`DefaultConfiguration.java:418-419`), not a user extension +point. + +**The bean must be registered in two places.** An earlier draft of this design claimed `bootstrapFactories` was on +the production path because `ConfigurationManager.addDefaultContainerProviders` (`ConfigurationManager.java:94`) +registers `StrutsDefaultConfigurationProvider`, which calls it at +`StrutsDefaultConfigurationProvider.java:116`. **That claim is wrong**, and it was only caught when the full core +suite failed with 1579 errors during implementation. + +`ConfigurationManager.addDefaultContainerProviders()` fires only when `containerProviders.isEmpty()` +(`ConfigurationManager.java:78-80`). `Dispatcher.init()` (`Dispatcher.java:711-719`) installs its own provider +list — including `StrutsBeanSelectionProvider` via `init_AliasStandardObjects` — so the list is never empty and +`StrutsDefaultConfigurationProvider` is never added. The production container is built from +`StrutsBeanSelectionProvider` plus `struts-beans.xml`, and `bootstrapFactories` is not on the path of that *main +Dispatcher* container. + +It is, however, on a different, load-bearing path: `DefaultConfiguration.reloadContainer` builds a **bootstrap** +container from `bootstrapFactories` (`DefaultConfiguration.java:283`, via `createBootstrapContainer` at +`DefaultConfiguration.java:348-373`), then calls `setContext(bootstrap)` (`DefaultConfiguration.java:307`), which +calls `bootstrap.getInstance(ValueStackFactory.class).createValueStack()` — and building a value stack instantiates +`SecurityMemberAccess` through `CompoundRootAccessor`/`RootAccessor`. So the bootstrap container's registration +of `SecurityMemberAccessConfig` is not a fallback for some other, unused path: it is exercised on every +`reloadContainer()` call, before the main Dispatcher container even exists. + +The registration therefore goes in both places, which is precisely what `ProviderAllowlist` and `ThreadAllowlist` +already do — `DefaultConfiguration.java:418-419` and `struts-beans.xml:175-176`: + +```xml + +``` + +The `DefaultConfiguration` registration serves the bootstrap container (`DefaultConfiguration.java:360`) and the +`XWorkTestCase` harness; the `struts-beans.xml` entry serves the real Dispatcher container. **Both registrations +are load-bearing** — production would throw at startup without either, since `useConfig` is a mandatory `@Inject` +on `SecurityMemberAccess`. The bootstrap container carries only `BOOTSTRAP_CONSTANTS`, so most security constants +are absent there, the `required = false` setters do not fire, and the bean falls back to defaults — exactly as a +`SecurityMemberAccess` constructed in that container behaves today. + +This failure mode is loud, not silent: `useConfig` is a mandatory `@Inject`, so a container missing the binding +fails closed with a `DependencyException`, not by running with empty exclusions. Because `SecurityMemberAccess` is +`Scope.PROTOTYPE` and `ContainerImpl`'s injector cache is built lazily, that exception fires at the first +`getInstance(SecurityMemberAccess.class)` rather than at `builder.create(...)` — still loud and still fail-closed, +just not at container-build time. + +The `TODO: SpringObjectFactoryTest fails when these are SINGLETON` comment at the top of `bootstrapFactories` +applies to the `*Factory` beans in the first block, not to this region, where singletons are already the norm. + +It takes over these sixteen `@Inject` setters from `SecurityMemberAccess`: + +| Setter | Constant | +|---|---| +| `useAllowStaticFieldAccess` | `STRUTS_ALLOW_STATIC_FIELD_ACCESS` | +| `useExcludedClasses` | `STRUTS_EXCLUDED_CLASSES` | +| `useExcludedPackageNamePatterns` | `STRUTS_EXCLUDED_PACKAGE_NAME_PATTERNS` | +| `useExcludedPackageNames` | `STRUTS_EXCLUDED_PACKAGE_NAMES` | +| `useExcludedPackageExemptClasses` | `STRUTS_EXCLUDED_PACKAGE_EXEMPT_CLASSES` | +| `useEnforceAllowlistEnabled` | `STRUTS_ALLOWLIST_ENABLE` | +| `useAllowlistClasses` | `STRUTS_ALLOWLIST_CLASSES` | +| `useAllowlistPackageNames` | `STRUTS_ALLOWLIST_PACKAGE_NAMES` | +| `useDisallowProxyObjectAccess` | `STRUTS_DISALLOW_PROXY_OBJECT_ACCESS` | +| `useDisallowProxyMemberAccess` | `STRUTS_DISALLOW_PROXY_MEMBER_ACCESS` | +| `useDisallowDefaultPackageAccess` | `STRUTS_DISALLOW_DEFAULT_PACKAGE_ACCESS` | +| `useDevMode` | `STRUTS_DEVMODE` | +| `useDevModeExcludedClasses` | `STRUTS_DEV_MODE_EXCLUDED_CLASSES` | +| `useDevModeExcludedPackageNamePatterns` | `STRUTS_DEV_MODE_EXCLUDED_PACKAGE_NAME_PATTERNS` | +| `useDevModeExcludedPackageNames` | `STRUTS_DEV_MODE_EXCLUDED_PACKAGE_NAMES` | +| `useDevModeExcludedPackageExemptClasses` | `STRUTS_DEV_MODE_EXCLUDED_PACKAGE_EXEMPT_CLASSES` | + +`setProxyService` and the `@Inject` constructor stay on `SecurityMemberAccess` — those inject collaborators, not +configuration. + +The bean implements `Initializable`. Dev-mode resolution cannot happen inside any individual setter, because +`ContainerImpl.addInjectorsForMembers` iterates `getDeclaredMethods()`, whose order the JDK explicitly leaves +unspecified. `Initializable.init()` runs after the whole dependency graph is built +(`InitializableFactory.wrapIfNeeded`, applied from `Scope` for singleton scope; `DefaultValidatorFactory` is the +existing precedent). `init()` therefore: + +1. Selects the effective excluded sets — dev-mode variants when `struts.devMode=true`, otherwise the normal ones. +2. Precomputes the allowlist package union. + +The bean exposes only immutable getters, and publishes the **effective** excluded sets with dev-mode already +applied, so nothing downstream needs to know dev-mode exists. + +### `SecurityMemberAccess` changes + +Gains exactly one injected member: + +```java +@Inject +public void useConfig(SecurityMemberAccessConfig config) { … } +``` + +which seeds its fields by copying immutable set references — no parsing, no `HashSet` construction, no +`Pattern.compile`. + +**Fields removed:** `isDevModeInit` (volatile), `isDevMode`, `devModeExcludedClasses`, +`devModeExcludedPackageNamePatterns`, `devModeExcludedPackageNames`, `devModeExcludedPackageExemptClasses`. + +**Method removed:** `useDevModeConfiguration()`, along with its call from `checkExclusionList` +(`SecurityMemberAccess.java:264`). The lazy dev-mode flip disappears from the access path entirely. + +**Field added:** `allowlistPackageNamesUnion`. + +The five dev-mode setters are **deleted outright rather than deprecated** — decided 2026-08-14. This is a +deliberate, signed-off deviation from the "additive and deprecate, no breakage in a minor" policy that governs the +rest of this change. + +They are `public`, but only ever container-injected, with no direct caller anywhere in core, plugins, or tests. +Preserving them faithfully would mean keeping `isDevMode` plus the four dev-mode set fields on the instance and +reinstating some form of the lazy flip — that is, keeping precisely the code this change exists to delete, to +serve a caller that does not demonstrably exist. Retention in simplified form was rejected because today's +semantics are subtle enough that any simplification would silently change them: a manual +`useDevModeExcludedClasses` call accumulates into the dev-mode set, which then *replaces* — rather than unions +with — `excludedClasses` on first access. + +The accepted risk is that a deployment calling these methods directly breaks at compile time on upgrade to 7.4.0. +This is a loud, immediate failure with an obvious fix, not a silent behavioural change, which is what makes it +acceptable where the constructor break discussed below was not. + +The remaining eleven configuration setters stay as `@Deprecated` methods with their `@Inject` annotations removed. +They keep mutating that instance exactly as they do now. Deprecation is by annotation only — no runtime warnings, +which would flood test output given roughly 110 direct call sites across core and plugins. + +`useAllowStaticFieldAccess` retains its side effect of calling `useExcludedClasses(Class.class.getName())`, and +the configuration bean must reproduce that accumulation exactly. + +### Why setter injection rather than constructor injection + +Constructor injection would be the obvious way to guarantee ordering, but it forces a constructor signature +change. A user subclass calling `super(providerAllowlist, threadAllowlist)` — precisely the shape of the existing +`ExternalSecurityMemberAccess` test fixture — would then either fail to compile, or, if a deprecated 2-arg +overload were retained, compile cleanly and silently run with empty exclusions. **That is a fail-open hole**, and +the kind that fails silently rather than loudly. + +Setter injection avoids it: `ContainerImpl.addInjectors` recurses into superclasses first +(`ContainerImpl.java:97`), so inherited `@Inject` setters are injected on subclass instances. Existing subclasses +keep compiling *and* receive the configuration. + +Injection ordering is safe by construction. Today the setters survive unspecified ordering only because they +*accumulate* rather than assign, making them commutative — a subtlety that is easy to destroy accidentally. After +this change `SecurityMemberAccess` has exactly one injected member touching those fields, so ordering stops +mattering at all. + +A null configuration is also safe: the eight direct `new SecurityMemberAccess(null, null)` test sites never have +the setter called, so their fields keep today's hardcoded defaults. Reads only ever touch fields, never the +configuration object, so there is no null path on the access path. + +### Allowlist union + +With the sets precomputed per container, `isClassAllowlisted` collapses to a single set and a single walk: + +```java +|| isClassBelongsToPackages(clazz, allowlistPackageNamesUnion); +``` + +This deletes the three-argument `isClassBelongsToPackages` overload and the two-set parameters on +`isPackageBelongsToPackages`, resolving WW-5678's first item as a side effect. + +The ticket flagged this as a fail-open hazard: if the union were computed in two places — once seeded from +configuration, once when the deprecated `useAllowlistPackageNames` setter fires — the two could drift, silently +dropping `ALLOWLIST_REQUIRED_PACKAGES` from the allowlist with nothing failing loudly. It is also a fail-open +hazard if the union is *re-computed* per instance: that reintroduces exactly the per-instantiation `HashSet` +allocation this ticket exists to remove, and lands on the deployments that configure the allowlist properly, +inverting the ticket's intent. + +Both hazards are avoided by moving `ALLOWLIST_REQUIRED_PACKAGES` and the `union(...)` helper onto +`SecurityMemberAccessConfig`, which precomputes `allowlistPackageNamesUnion` once, inside its own +`useAllowlistPackageNames` setter, when the constant fires during container construction: + +```java +// SecurityMemberAccessConfig +static final Set ALLOWLIST_REQUIRED_PACKAGES = Set.of( + "org.apache.struts2.validator.validators", + "org.apache.struts2.components", + "org.apache.struts2.views.jsp" +); + +public void useAllowlistPackageNames(String commaDelimitedPackageNames) { + this.allowlistPackageNames = toPackageNamesSet(commaDelimitedPackageNames); + this.allowlistPackageNamesUnion = union(ALLOWLIST_REQUIRED_PACKAGES, allowlistPackageNames); +} + +static Set union(Set required, Set configured) { … } +``` + +`SecurityMemberAccess.useConfig` copies the precomputed reference (`config.getAllowlistPackageNamesUnion()`) — +no allocation on the hot instantiation path. Its deprecated `useAllowlistPackageNames` setter, which still mutates +a single instance directly and has no `SecurityMemberAccessConfig` to read from, calls the same +`SecurityMemberAccessConfig.union(...)` static method. Both routes therefore funnel through the one method, so +exactly one line in the codebase computes the union, and `ALLOWLIST_REQUIRED_PACKAGES` cannot drift out of it +through a second implementation. The constant and helper live on the config bean — the class that owns computing +and exposing configuration-derived state — rather than being duplicated onto `SecurityMemberAccess`, whose +deprecated setter merely calls back into it. + +`isPackageBelongsToPackages` currently early-returns on `first.isEmpty() && second.isEmpty()`. Since +`ALLOWLIST_REQUIRED_PACKAGES` is never empty, that guard simply stops firing on the allowlist path; the exclusion +path, where both sets genuinely can be empty, keeps it. The guard was only ever an optimization, so this is not a +semantic change. + +## Data flow + +| Tier | Frequency | Work | +|---|---|---| +| `SecurityMemberAccessConfig` construction | Once per container | All parsing, class validation, pattern compilation, dev-mode resolution, union precomputation | +| `useConfig` | Once per `SecurityMemberAccess` | About a dozen immutable reference copies | +| `ParametersInterceptor` | Once per request | Sets `acceptProperties`/`excludeProperties` on the instance (unchanged) | +| `isAccessible` | Per OGNL member access | Field reads only | + +## Error handling + +Parsing failures — `ConfigurationException` for an unloadable class, an invalid regex, or whitespace in a package +name — move from being thrown on every instantiation to being thrown once, when the singleton is first built. +Still fatal, still loud, just earlier and once. Nothing degrades to a warning. + +The `struts.allowlist.enable=false` warning already dedupes via `logWarningForFirstOccurrence`; moving it to the +configuration bean makes it a genuine once-per-container event. + +## Behaviour changes + +One, accepted during design review: the `"DevMode enabled, using DevMode excluded classes and packages for OGNL +security enforcement!"` warning currently fires on the first OGNL access and will now fire when the configuration +singleton's `init()` runs. The main Dispatcher container is built with `builder.create(false)` (lazy singletons), so +for it that is still triggered by first use — the config bean is constructed the first time something asks for a +`SecurityMemberAccess`, not at container-build/startup time. The bootstrap container does use `create(true)` and so +does log eagerly there. The change is still worth making — it moves the warning from being contingent on OGNL +traffic to being contingent on the config bean's first use, which happens earlier and more predictably — but it is +not a guaranteed startup-time log line for the main container. + +No other externally visible behaviour changes. OGNL allow/deny semantics are identical. + +## Testing + +Core tests are JUnit 4 or extend `XWorkTestCase`. A JUnit 5 `@Test` added to these suites silently never runs. + +1. **Sharing proof.** Request several `SecurityMemberAccess` instances from one container and assert their + configuration-derived sets are reference-identical (`assertSame`, not `assertEquals`). Reference identity is a + dependency-free proof that no re-parsing occurred, since any re-parse necessarily produces a fresh set; this is + the sound substitute for a counting probe and is what the implementation actually asserts. +2. **Instance isolation.** Calling a deprecated setter on one instance must not perturb a sibling instance or the + singleton. The sets are `unmodifiableSet`, so an in-place mutation bug would throw rather than corrupt + silently, but this invariant deserves an explicit assertion. +3. **Subclass injection.** A subclass declaring the 2-arg constructor and calling + `super(providerAllowlist, threadAllowlist)` must receive the configuration through the inherited setter. This + is the test that would catch a future refactor to constructor injection reintroducing the fail-open hole. +4. **Behaviour preservation.** Following WW-5674's differential pattern: for default, dev-mode, and custom + configurations, the sets the new bean publishes must equal what a legacy-style accumulation produces. This is + where the `useAllowStaticFieldAccess` → `useExcludedClasses` side effect gets pinned down. +5. **Dev-mode.** With `struts.devMode=true` the effective sets are the dev-mode ones from the start, with no OGNL + access required to trigger the switch. + +The principal safety net is the existing suite. `SecurityMemberAccessTest` and its siblings drive these setters +directly from roughly 110 call sites across core and plugins and must pass untouched. If the deprecated setters +have kept their exact semantics, that suite cannot tell the difference — the strongest available evidence that +OGNL allow/deny semantics are unchanged. + +## Risks + +| Risk | Mitigation | +|---|---| +| Shared sets mutated in place, poisoning every instance in the container | Sets are already `unmodifiableSet`; test 2 asserts isolation explicitly | +| Allowlist union drifts from `ALLOWLIST_REQUIRED_PACKAGES` (fail-open) | Single computation site; test 4 covers custom allowlist configurations | +| Configuration bean fails to reproduce the accumulate-not-assign semantics | Test 4 is differential against the legacy accumulation, not against hand-written expectations | +| A future refactor moves configuration to constructor injection, reintroducing the silent fail-open | Test 3 encodes the subclass contract; the rationale is recorded above and in the class Javadoc | +| `Initializable` is documented "should be only used internally" | The bean is internal and unaliased; `DefaultValidatorFactory` is the existing precedent | + +## Follow-ups + +- **8.0.0 — remove the deprecated configuration setters.** The eleven methods left on `SecurityMemberAccess` + should be removed once the major version allows it. To be filed as its own ticket, cross-referencing WW-5675 and + WW-5678. +- **WW-5678** — its first item (the package-private overload sharing a name with a public method) is resolved for + free here by the union collapse. The remaining visibility narrowing stays with that ticket. +- **WW-5667** — the parent should be updated to note that this ticket, not WW-5674, is the one expected to move + the reported 9%. +- **Migration guide entry for 7.4.0** — the removal of the five dev-mode setters is a source-breaking change in a + minor release and must be called out in the Version Notes and Migration Guide, however narrow the affected + audience.