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 cac69c15c303a7d684d2007eaac45f800716b999 Author: Lukasz Lenart <[email protected]> AuthorDate: Fri Aug 14 14:17:07 2026 +0200 WW-5675 perf(ognl): share parsed config across SecurityMemberAccess instances Co-Authored-By: Claude Opus 5 <[email protected]> --- .../struts2/config/impl/DefaultConfiguration.java | 2 + .../apache/struts2/ognl/SecurityMemberAccess.java | 99 +++++++++++++++++++--- .../SecurityMemberAccessConfigSharingTest.java | 91 ++++++++++++++++++++ 3 files changed, 181 insertions(+), 11 deletions(-) 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 dcf4f1602..53bf20dc8 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.conversion.StrutsTypeConverterHolder; 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 class DefaultConfiguration implements Configuration { .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 badad3dee..8ba19f540 100644 --- a/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java +++ b/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java @@ -112,6 +112,28 @@ public class SecurityMemberAccess implements MemberAccess { 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.allowlistPackageNames = config.getAllowlistPackageNames(); + this.disallowProxyObjectAccess = config.isDisallowProxyObjectAccess(); + this.disallowProxyMemberAccess = config.isDisallowProxyMemberAccess(); + this.disallowDefaultPackageAccess = config.isDisallowDefaultPackageAccess(); + } + @Override public Object setup(OgnlContext context, Object target, Member member, String propertyName) { Object result = null; @@ -470,7 +492,12 @@ public class SecurityMemberAccess implements MemberAccess { 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. + */ + @Deprecated public void useAllowStaticFieldAccess(String allowStaticFieldAccess) { this.allowStaticFieldAccess = BooleanUtils.toBoolean(allowStaticFieldAccess); if (!this.allowStaticFieldAccess) { @@ -478,27 +505,52 @@ public class SecurityMemberAccess implements MemberAccess { } } - @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. + */ + @Deprecated 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. + */ + @Deprecated 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. + */ + @Deprecated 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. + */ + @Deprecated 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. + */ + @Deprecated public void useEnforceAllowlistEnabled(String enforceAllowlistEnabled) { this.enforceAllowlistEnabled = BooleanUtils.toBoolean(enforceAllowlistEnabled); if (!this.enforceAllowlistEnabled) { @@ -510,27 +562,52 @@ public class SecurityMemberAccess implements MemberAccess { } } - @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. + */ + @Deprecated 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. + */ + @Deprecated public void useAllowlistPackageNames(String commaDelimitedPackageNames) { this.allowlistPackageNames = 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. + */ + @Deprecated 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. + */ + @Deprecated 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. + */ + @Deprecated public void useDisallowDefaultPackageAccess(String disallowDefaultPackageAccess) { this.disallowDefaultPackageAccess = BooleanUtils.toBoolean(disallowDefaultPackageAccess); } 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 000000000..b6af1db29 --- /dev/null +++ b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessConfigSharingTest.java @@ -0,0 +1,91 @@ +/* + * 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.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<String> firstExcluded = SecurityMemberAccessTest.reflectField(first, "excludedClasses"); + Set<String> secondExcluded = SecurityMemberAccessTest.reflectField(second, "excludedClasses"); + assertSame("excluded classes were re-parsed per instance", firstExcluded, secondExcluded); + + 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); + } + + 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<String> before = SecurityMemberAccessTest.reflectField(untouched, "excludedClasses"); + mutated.useExcludedClasses("java.lang.Runtime"); + Set<String> 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<String> 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<String> 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); + } + } +}
