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 6028e9dc86b43c8467daadb2a015950a893bc0e9 Author: Lukasz Lenart <[email protected]> AuthorDate: Fri Aug 14 14:59:23 2026 +0200 WW-5675 perf(ognl): precompute the allowlist package union Co-Authored-By: Claude Opus 5 <[email protected]> --- .../apache/struts2/ognl/SecurityMemberAccess.java | 76 ++++++++++++---------- .../SecurityMemberAccessPackageMatchingTest.java | 46 ++++++++----- 2 files changed, 70 insertions(+), 52 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 b7ed69caa..80c36f05c 100644 --- a/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java +++ b/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java @@ -32,12 +32,14 @@ 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; @@ -89,6 +91,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 boolean disallowProxyObjectAccess = false; private boolean disallowProxyMemberAccess = false; @@ -121,12 +124,31 @@ public class SecurityMemberAccess implements MemberAccess { this.excludedPackageExemptClasses = config.getExcludedPackageExemptClasses(); this.enforceAllowlistEnabled = config.isEnforceAllowlistEnabled(); this.allowlistClasses = config.getAllowlistClasses(); - this.allowlistPackageNames = config.getAllowlistPackageNames(); + applyAllowlistPackageNames(config.getAllowlistPackageNames()); 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. + */ + 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); + } + @Override public Object setup(OgnlContext context, Object target, Member member, String propertyName) { Object result = null; @@ -269,7 +291,7 @@ public class SecurityMemberAccess implements MemberAccess { || 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); } /** @@ -404,54 +426,38 @@ public class SecurityMemberAccess implements MemberAccess { } public static boolean isClassBelongsToPackages(Class<?> clazz, Set<String> matchingPackages) { - return isClassBelongsToPackages(clazz, matchingPackages, emptySet()); - } - - /** - * 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<String> first, Set<String> second) { - return isPackageBelongsToPackages(toPackageName(clazz), first, second); + return isPackageBelongsToPackages(toPackageName(clazz), matchingPackages); } /** - * 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. * * <p> - * 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 - * <em>loosening</em> 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 + * <em>loosening</em> 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<String> first, Set<String> second) { - if (first.isEmpty() && second.isEmpty()) { + static boolean isPackageBelongsToPackages(String packageName, Set<String> 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) { @@ -571,7 +577,7 @@ public class SecurityMemberAccess implements MemberAccess { */ @Deprecated public void useAllowlistPackageNames(String commaDelimitedPackageNames) { - this.allowlistPackageNames = toPackageNamesSet(commaDelimitedPackageNames); + applyAllowlistPackageNames(toPackageNamesSet(commaDelimitedPackageNames)); } /** 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 413b0c669..3dfa540dd 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 @@ public class SecurityMemberAccessPackageMatchingTest { public void siblingPackageWithSharedCharacterPrefixDoesNotMatch() { Set<String> 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 class SecurityMemberAccessPackageMatchingTest { public void indexWalkMatchesLegacyAcrossPackageNameShapes() { for (String packageName : PACKAGE_NAMES) { for (Set<String> 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 class SecurityMemberAccessPackageMatchingTest { } @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<String> first : CANDIDATE_SETS) { - for (Set<String> 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<String> 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<String> union = SecurityMemberAccessTest.reflectField(sma, "allowlistPackageNamesUnion"); + + assertThat(union).contains( + "org.apache.struts2.components", + "org.apache.struts2.views.jsp", + "org.apache.struts2.validator.validators"); } }
