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 02fa6aa9e8248e6745951fbee629cf77849124c8 Author: Lukasz Lenart <[email protected]> AuthorDate: Fri Aug 14 14:29:59 2026 +0200 WW-5675 test(ognl): strengthen SecurityMemberAccessConfigSharingTest assertions Two review findings: (1) the config-derived-set assertions compared instance to instance only, which is vacuous for every emptySet()-defaulted field since Collections.emptySet() is a JVM-wide singleton shared by both the config bean's own default and SecurityMemberAccess's own default; deleting a useConfig assignment for such a field would still pass. Fixed by additionally asserting each field directly against the shared SecurityMemberAccessConfig bean, with the container reloaded to set every relevant constant away from its hardcoded default so the comparison is not itself vacuous by coincidence. (2) testConfigBeanIsASingleton passed on assertSame(null, null) when the bean was not registered at all; added assertNotNull before the identity check. Co-Authored-By: Claude Opus 5 <[email protected]> --- .../SecurityMemberAccessConfigSharingTest.java | 64 +++++++++++++++++++++- 1 file changed, 62 insertions(+), 2 deletions(-) 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 b6af1db29..5680e59fb 100644 --- a/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigSharingTest.java +++ b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigSharingTest.java @@ -18,8 +18,10 @@ */ package org.apache.struts2.ognl; +import org.apache.struts2.StrutsConstants; import org.apache.struts2.XWorkTestCase; +import java.util.Map; import java.util.Set; public class SecurityMemberAccessConfigSharingTest extends XWorkTestCase { @@ -27,10 +29,38 @@ public class SecurityMemberAccessConfigSharingTest extends XWorkTestCase { /** * Reference identity proves no re-parsing occurred: any re-parse necessarily * allocates a fresh set. + * <p> + * 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 + * <em>unseeded</em> 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. + * <p> + * 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.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")); + 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); @@ -41,11 +71,41 @@ public class SecurityMemberAccessConfigSharingTest extends XWorkTestCase { Set<String> firstPackages = SecurityMemberAccessTest.reflectField(first, "excludedPackageNames"); Set<String> 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")); + assertSame("allowlistPackageNames not seeded from config", + config.getAllowlistPackageNames(), SecurityMemberAccessTest.reflectField(first, "allowlistPackageNames")); + + 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() { - assertSame(container.getInstance(SecurityMemberAccessConfig.class), - container.getInstance(SecurityMemberAccessConfig.class)); + SecurityMemberAccessConfig instance = container.getInstance(SecurityMemberAccessConfig.class); + assertNotNull("SecurityMemberAccessConfig is not registered in the container", instance); + assertSame(instance, container.getInstance(SecurityMemberAccessConfig.class)); } /**
