This is an automated email from the ASF dual-hosted git repository. lukaszlenart pushed a commit to branch WW-5675-share-parsed-ognl-security-config in repository https://gitbox.apache.org/repos/asf/struts.git
commit 571014c98ccc6d8f6141a35c60fc9c5abef364b9 Author: Lukasz Lenart <[email protected]> AuthorDate: Fri Aug 14 15:21:40 2026 +0200 WW-5675 fix(ognl): move the allowlist union onto the config bean Final-review cleanup: SecurityMemberAccess.applyAllowlistPackageNames was still allocating a fresh HashSet per instantiation, landing precisely on deployments that configure struts.allowlist.packageNames. Move ALLOWLIST_REQUIRED_PACKAGES and union(...) onto SecurityMemberAccessConfig, which now precomputes allowlistPackageNamesUnion once per container; useConfig copies the reference, and the deprecated setter path reuses the same static union() method, so there remains exactly one computation site. Also: mark the eleven deprecated SecurityMemberAccess setters with since/forRemoval per repo convention, document union()'s Set.of(...) aliasing contract, pin allowlistPackageNamesUnion into the immutability and dev-mode-field-removal tests, restore alphabetical import order in ConfigParseUtilTest, switch the sharing test off the Map.of ten-pair ceiling, and correct the design doc's bootstrap-container wiring claim and drop its unimplemented counting-probe promise. Co-Authored-By: Claude Opus 5 <[email protected]> --- .../apache/struts2/ognl/SecurityMemberAccess.java | 55 ++++++++------------ .../struts2/ognl/SecurityMemberAccessConfig.java | 41 +++++++++++++++ .../SecurityMemberAccessConfigSharingTest.java | 44 ++++++++++------ .../ognl/SecurityMemberAccessConfigTest.java | 21 ++++++++ .../struts2/ognl/SecurityMemberAccessTest.java | 1 + .../apache/struts2/util/ConfigParseUtilTest.java | 2 +- ...security-member-access-config-sharing-design.md | 58 +++++++++++++++++----- 7 files changed, 159 insertions(+), 63 deletions(-) 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 80c36f05c..fcc337a63 100644 --- a/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java +++ b/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java @@ -32,14 +32,12 @@ import java.lang.reflect.Constructor; import java.lang.reflect.Field; import java.lang.reflect.Member; import java.lang.reflect.Modifier; -import java.util.HashSet; import java.util.Set; import java.util.regex.Matcher; import java.util.regex.Pattern; import static java.text.MessageFormat.format; 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; @@ -58,12 +56,6 @@ public class SecurityMemberAccess implements MemberAccess { private static final Logger LOG = LogManager.getLogger(SecurityMemberAccess.class); - private static final Set<String> ALLOWLIST_REQUIRED_PACKAGES = Set.of( - "org.apache.struts2.validator.validators", - "org.apache.struts2.components", - "org.apache.struts2.views.jsp" - ); - private static final Set<Class<?>> ALLOWLIST_REQUIRED_CLASSES = Set.of( java.lang.Enum.class, java.lang.String.class, @@ -91,7 +83,7 @@ public class SecurityMemberAccess implements MemberAccess { private boolean enforceAllowlistEnabled = false; private Set<Class<?>> allowlistClasses = emptySet(); private Set<String> allowlistPackageNames = emptySet(); - private Set<String> allowlistPackageNamesUnion = ALLOWLIST_REQUIRED_PACKAGES; + private Set<String> allowlistPackageNamesUnion = SecurityMemberAccessConfig.ALLOWLIST_REQUIRED_PACKAGES; private boolean disallowProxyObjectAccess = false; private boolean disallowProxyMemberAccess = false; @@ -124,29 +116,24 @@ public class SecurityMemberAccess implements MemberAccess { this.excludedPackageExemptClasses = config.getExcludedPackageExemptClasses(); this.enforceAllowlistEnabled = config.isEnforceAllowlistEnabled(); this.allowlistClasses = config.getAllowlistClasses(); - applyAllowlistPackageNames(config.getAllowlistPackageNames()); + this.allowlistPackageNames = config.getAllowlistPackageNames(); + this.allowlistPackageNamesUnion = config.getAllowlistPackageNamesUnion(); this.disallowProxyObjectAccess = config.isDisallowProxyObjectAccess(); this.disallowProxyMemberAccess = config.isDisallowProxyMemberAccess(); this.disallowDefaultPackageAccess = config.isDisallowDefaultPackageAccess(); } /** - * 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. + * Used only by the deprecated {@link #useAllowlistPackageNames(String)} setter path. The injected + * configuration path seeds both fields 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<String> packageNames) { this.allowlistPackageNames = packageNames; - this.allowlistPackageNamesUnion = union(ALLOWLIST_REQUIRED_PACKAGES, packageNames); - } - - private static Set<String> union(Set<String> required, Set<String> configured) { - if (configured.isEmpty()) { - return required; - } - Set<String> union = new HashSet<>(required); - union.addAll(configured); - return unmodifiableSet(union); + this.allowlistPackageNamesUnion = SecurityMemberAccessConfig.union(SecurityMemberAccessConfig.ALLOWLIST_REQUIRED_PACKAGES, packageNames); } @Override @@ -495,7 +482,7 @@ public class SecurityMemberAccess implements MemberAccess { * {@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 + @Deprecated(since = "7.4.0", forRemoval = true) public void useAllowStaticFieldAccess(String allowStaticFieldAccess) { this.allowStaticFieldAccess = BooleanUtils.toBoolean(allowStaticFieldAccess); if (!this.allowStaticFieldAccess) { @@ -508,7 +495,7 @@ public class SecurityMemberAccess implements MemberAccess { * {@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 + @Deprecated(since = "7.4.0", forRemoval = true) public void useExcludedClasses(String commaDelimitedClasses) { this.excludedClasses = toNewClassesSet(excludedClasses, commaDelimitedClasses); } @@ -518,7 +505,7 @@ public class SecurityMemberAccess implements MemberAccess { * {@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 + @Deprecated(since = "7.4.0", forRemoval = true) public void useExcludedPackageNamePatterns(String commaDelimitedPackagePatterns) { this.excludedPackageNamePatterns = toNewPatternsSet(excludedPackageNamePatterns, commaDelimitedPackagePatterns); } @@ -528,7 +515,7 @@ public class SecurityMemberAccess implements MemberAccess { * {@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 + @Deprecated(since = "7.4.0", forRemoval = true) public void useExcludedPackageNames(String commaDelimitedPackageNames) { this.excludedPackageNames = toNewPackageNamesSet(excludedPackageNames, commaDelimitedPackageNames); } @@ -538,7 +525,7 @@ public class SecurityMemberAccess implements MemberAccess { * {@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 + @Deprecated(since = "7.4.0", forRemoval = true) public void useExcludedPackageExemptClasses(String commaDelimitedClasses) { this.excludedPackageExemptClasses = toClassesSet(commaDelimitedClasses); } @@ -548,7 +535,7 @@ public class SecurityMemberAccess implements MemberAccess { * {@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 + @Deprecated(since = "7.4.0", forRemoval = true) public void useEnforceAllowlistEnabled(String enforceAllowlistEnabled) { this.enforceAllowlistEnabled = BooleanUtils.toBoolean(enforceAllowlistEnabled); if (!this.enforceAllowlistEnabled) { @@ -565,7 +552,7 @@ public class SecurityMemberAccess implements MemberAccess { * {@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 + @Deprecated(since = "7.4.0", forRemoval = true) public void useAllowlistClasses(String commaDelimitedClasses) { this.allowlistClasses = toClassObjectsSet(commaDelimitedClasses); } @@ -575,7 +562,7 @@ public class SecurityMemberAccess implements MemberAccess { * {@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 + @Deprecated(since = "7.4.0", forRemoval = true) public void useAllowlistPackageNames(String commaDelimitedPackageNames) { applyAllowlistPackageNames(toPackageNamesSet(commaDelimitedPackageNames)); } @@ -585,7 +572,7 @@ public class SecurityMemberAccess implements MemberAccess { * {@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 + @Deprecated(since = "7.4.0", forRemoval = true) public void useDisallowProxyObjectAccess(String disallowProxyObjectAccess) { this.disallowProxyObjectAccess = BooleanUtils.toBoolean(disallowProxyObjectAccess); } @@ -595,7 +582,7 @@ public class SecurityMemberAccess implements MemberAccess { * {@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 + @Deprecated(since = "7.4.0", forRemoval = true) public void useDisallowProxyMemberAccess(String disallowProxyMemberAccess) { this.disallowProxyMemberAccess = BooleanUtils.toBoolean(disallowProxyMemberAccess); } @@ -605,7 +592,7 @@ public class SecurityMemberAccess implements MemberAccess { * {@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 + @Deprecated(since = "7.4.0", forRemoval = true) public void useDisallowDefaultPackageAccess(String disallowDefaultPackageAccess) { this.disallowDefaultPackageAccess = BooleanUtils.toBoolean(disallowDefaultPackageAccess); } diff --git a/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccessConfig.java b/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccessConfig.java index 600973830..59aef3c03 100644 --- a/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccessConfig.java +++ b/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccessConfig.java @@ -25,10 +25,12 @@ 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; @@ -57,6 +59,20 @@ 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<String> 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<String> excludedClasses = Set.of(Object.class.getName()); @@ -73,6 +89,7 @@ public class SecurityMemberAccessConfig implements Initializable { private boolean enforceAllowlistEnabled = false; private Set<Class<?>> allowlistClasses = emptySet(); private Set<String> allowlistPackageNames = emptySet(); + private Set<String> allowlistPackageNamesUnion = ALLOWLIST_REQUIRED_PACKAGES; private boolean disallowProxyObjectAccess = false; private boolean disallowProxyMemberAccess = false; @@ -139,6 +156,26 @@ public class SecurityMemberAccessConfig implements Initializable { @Inject(value = STRUTS_ALLOWLIST_PACKAGE_NAMES, required = false) public 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. + * <p> + * The early return aliases {@code required} directly into the result, which is safe only because + * every caller passes an immutable {@code Set.of(...)} for that argument; a mutable set must not be + * passed as {@code required}. + */ + static Set<String> union(Set<String> required, Set<String> configured) { + if (configured.isEmpty()) { + return required; + } + Set<String> union = new HashSet<>(required); + union.addAll(configured); + return unmodifiableSet(union); } @Inject(value = StrutsConstants.STRUTS_DISALLOW_PROXY_OBJECT_ACCESS, required = false) @@ -213,6 +250,10 @@ public class SecurityMemberAccessConfig implements Initializable { return allowlistPackageNames; } + public Set<String> getAllowlistPackageNamesUnion() { + return allowlistPackageNamesUnion; + } + public boolean isDisallowProxyObjectAccess() { return disallowProxyObjectAccess; } diff --git a/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigSharingTest.java b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigSharingTest.java index ffce445f8..9d74bc637 100644 --- a/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigSharingTest.java +++ b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigSharingTest.java @@ -24,6 +24,8 @@ 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 { /** @@ -46,17 +48,17 @@ public class SecurityMemberAccessConfigSharingTest extends XWorkTestCase { * when {@code useConfig} actually ran. */ public void testConfigDerivedSetsAreSharedAcrossInstances() throws Exception { - loadButSet(Map.of( - StrutsConstants.STRUTS_ALLOW_STATIC_FIELD_ACCESS, "false", - StrutsConstants.STRUTS_EXCLUDED_PACKAGE_NAME_PATTERNS, "^org\\.apache\\.struts2\\.ognl\\.testpkg\\..*", - StrutsConstants.STRUTS_EXCLUDED_PACKAGE_NAMES, "org.apache.struts2.ognl.testpkg", - StrutsConstants.STRUTS_EXCLUDED_PACKAGE_EXEMPT_CLASSES, "java.lang.String", - StrutsConstants.STRUTS_ALLOWLIST_ENABLE, "true", - StrutsConstants.STRUTS_ALLOWLIST_CLASSES, "java.lang.String", - StrutsConstants.STRUTS_ALLOWLIST_PACKAGE_NAMES, "org.apache.struts2.ognl.testpkg", - StrutsConstants.STRUTS_DISALLOW_PROXY_OBJECT_ACCESS, "true", - StrutsConstants.STRUTS_DISALLOW_PROXY_MEMBER_ACCESS, "true", - StrutsConstants.STRUTS_DISALLOW_DEFAULT_PACKAGE_ACCESS, "true")); + 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); @@ -84,6 +86,10 @@ public class SecurityMemberAccessConfigSharingTest extends XWorkTestCase { config.getAllowlistClasses(), SecurityMemberAccessTest.reflectField(first, "allowlistClasses")); assertSame("allowlistPackageNames not seeded from config", config.getAllowlistPackageNames(), SecurityMemberAccessTest.reflectField(first, "allowlistPackageNames")); + Set<String> 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", @@ -171,12 +177,18 @@ public class SecurityMemberAccessConfigSharingTest extends XWorkTestCase { } public void testDevModeMethodsAreGone() throws Exception { - for (String name : new String[]{"useDevMode", "useDevModeExcludedClasses", + Set<String> removedMethods = Set.of("useDevMode", "useDevModeExcludedClasses", "useDevModeExcludedPackageNamePatterns", "useDevModeExcludedPackageNames", - "useDevModeExcludedPackageExemptClasses", "useDevModeConfiguration"}) { - for (java.lang.reflect.Method method : SecurityMemberAccess.class.getDeclaredMethods()) { - assertFalse("SecurityMemberAccess still declares " + name, method.getName().equals(name)); - } + "useDevModeExcludedPackageExemptClasses", "useDevModeConfiguration"); + for (java.lang.reflect.Method method : SecurityMemberAccess.class.getDeclaredMethods()) { + assertFalse("SecurityMemberAccess still declares " + method.getName(), removedMethods.contains(method.getName())); + } + + Set<String> 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 index e95e99e08..652d26d8c 100644 --- a/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigTest.java +++ b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigTest.java @@ -140,4 +140,25 @@ public class SecurityMemberAccessConfigTest { 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/SecurityMemberAccessTest.java b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessTest.java index a9b7b8c12..84c11a6dd 100644 --- a/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessTest.java +++ b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessTest.java @@ -112,6 +112,7 @@ public class SecurityMemberAccessTest { "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 6e9c79727..18567bd89 100644 --- a/core/src/test/java/org/apache/struts2/util/ConfigParseUtilTest.java +++ b/core/src/test/java/org/apache/struts2/util/ConfigParseUtilTest.java @@ -33,9 +33,9 @@ import java.util.List; import java.util.Map; import java.util.Set; -import static org.junit.Assert.assertThrows; 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; 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 index 319ce54ec..8dbdb7122 100644 --- 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 @@ -96,7 +96,16 @@ suite failed with 1579 errors during implementation. (`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 that path at all. +`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`: @@ -106,10 +115,11 @@ already do — `DefaultConfiguration.java:418-419` and `struts-beans.xml:175-176 ``` The `DefaultConfiguration` registration serves the bootstrap container (`DefaultConfiguration.java:360`) and the -`XWorkTestCase` harness; the `struts-beans.xml` entry serves the real Dispatcher container. 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. +`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 throws at build time rather than running with empty exclusions. @@ -230,16 +240,40 @@ This deletes the three-argument `isClassBelongsToPackages` overload and the two- 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. Both routes therefore -funnel through one private method, so exactly one line in the codebase computes the union: +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 -private void applyAllowlistPackageNames(Set<String> names) { - this.allowlistPackageNames = names; - this.allowlistPackageNamesUnion = union(ALLOWLIST_REQUIRED_PACKAGES, names); +// SecurityMemberAccessConfig +static final Set<String> 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<String> union(Set<String> required, Set<String> 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 @@ -277,8 +311,8 @@ Core tests are JUnit 4 or extend `XWorkTestCase`. A JUnit 5 `@Test` added to the 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. Backed - by a counting probe asserting exactly one parse per container. + 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.
