From 8d0a1771f0a9119a2ff964a3c3b03f8e6056e575 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sun, 23 Aug 2026 18:34:02 +0200 Subject: [PATCH] WW-5677 perf(ognl): drop the residual getPackage() lookups on the access path WW-5674 replaced Class.getPackage() with the cached Class.getPackageName() in toPackageName, but deliberately left two neighbouring call sites alone as out of scope. Both sit on the same per-access path within twenty lines of it. checkDefaultPackageAccess still tested `getPackage() == null || getPackage().getName().isEmpty()`, which resolves through the defining classloader's package map twice per class, for up to two classes per access. It now tests toPackageName(clazz).isEmpty(). The two forms agree for every class shape: getPackage() is null for arrays, primitives and void, and names the unnamed package with the empty string, all of which toPackageName reports as empty. isExcludedPackageNamePatterns evaluated toPackageName inside the lambda, so it re-resolved the package name once per configured pattern. It is now resolved once per call. Both changes are behaviour-preserving. Per the ticket, the equivalence is asserted rather than argued: defaultPackageConditionMatchesLegacyAcrossClassShapes runs the replaced condition, frozen verbatim as an oracle that calls getPackage() directly, against the new one over the existing class-shape matrix -- arrays, primitives, void, a default-package class, a lambda and a JDK proxy. Two behavioural tests cover the gate itself: a named-package class still passes, and an array target, the shape most likely to break the equivalence, stays blocked. All three tests were mutation-checked. Resolving arrays to java.lang fails the equivalence test and the array-target test; inverting the member-class condition fails the named-package test. Neither path runs by default -- checkDefaultPackageAccess only when struts.disallowDefaultPackageAccess is enabled, and the pattern loop only when struts.excludedPackageNamePatterns is configured, both commented out in struts-excluded-classes.xml. Co-Authored-By: Claude Opus 5 --- .../struts2/ognl/SecurityMemberAccess.java | 17 +++++++++--- ...curityMemberAccessPackageMatchingTest.java | 24 +++++++++++++++++ .../ognl/SecurityMemberAccessTest.java | 27 +++++++++++++++++++ 3 files changed, 65 insertions(+), 3 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 4d552d5948..10fdf65ced 100644 --- a/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java +++ b/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java @@ -310,6 +310,15 @@ protected boolean checkExclusionList(Object target, Member member) { } /** + * Blocks access to classes in the default (unnamed) package. + *

+ * The emptiness of {@link #toPackageName(Class)} is the same test as the + * {@code getPackage() == null || getPackage().getName().isEmpty()} form this replaced, for every + * class shape: {@code getPackage()} is null for arrays, primitives and {@code void}, and names the + * unnamed package with the empty string, all of which {@code toPackageName} reports as empty. It + * avoids the {@code getPackage()} lookup through the defining classloader's package map, which + * ran twice per class here. See WW-5677. + * * @return {@code true} if member access is allowed */ protected boolean checkDefaultPackageAccess(Object target, Member member) { @@ -317,7 +326,7 @@ protected boolean checkDefaultPackageAccess(Object target, Member member) { return true; } Class memberClass = member.getDeclaringClass(); - if (memberClass.getPackage() == null || memberClass.getPackage().getName().isEmpty()) { + if (toPackageName(memberClass).isEmpty()) { LOG.warn("Class [{}] from the default package is excluded!", memberClass); return false; } @@ -325,7 +334,7 @@ protected boolean checkDefaultPackageAccess(Object target, Member member) { return true; } Class targetClass = target.getClass(); - if (targetClass.getPackage() == null || targetClass.getPackage().getName().isEmpty()) { + if (toPackageName(targetClass).isEmpty()) { LOG.warn("Class [{}] from the default package is excluded!", targetClass); return false; } @@ -407,7 +416,9 @@ public static String toPackageName(Class clazz) { } protected boolean isExcludedPackageNamePatterns(Class clazz) { - return excludedPackageNamePatterns.stream().anyMatch(pattern -> pattern.matcher(toPackageName(clazz)).matches()); + // Resolved once rather than inside the lambda, which re-resolved it per configured pattern. + String packageName = toPackageName(clazz); + return excludedPackageNamePatterns.stream().anyMatch(pattern -> pattern.matcher(packageName).matches()); } protected boolean isExcludedPackageNames(Class clazz) { 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 3dfa540dd4..d333b20af6 100644 --- a/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessPackageMatchingTest.java +++ b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessPackageMatchingTest.java @@ -53,6 +53,15 @@ private static boolean legacyPrefixMatch(String packageName, Set matchin .anyMatch(matchingPackages::contains); } + /** + * The default-package condition replaced by WW-5677 in {@code checkDefaultPackageAccess}, + * retained verbatim as the reference oracle. Deliberately calls {@link Class#getPackage()} + * directly rather than delegating to production code, so that it cannot drift with it. + */ + private static boolean legacyDefaultPackageCondition(Class clazz) { + return clazz.getPackage() == null || clazz.getPackage().getName().isEmpty(); + } + /** * The {@code toPackageName} implementation replaced by WW-5674, retained as the reference oracle. */ @@ -168,6 +177,21 @@ public void toPackageNameMatchesLegacyAcrossClassShapes() throws Exception { } } + /** + * WW-5677 replaced {@code getPackage() == null || getPackage().getName().isEmpty()} in + * {@code checkDefaultPackageAccess} with {@code toPackageName(clazz).isEmpty()}. That gate + * decides whether a class counts as living in the default package, so the equivalence is + * asserted against the frozen oracle over every class shape rather than argued. + */ + @Test + public void defaultPackageConditionMatchesLegacyAcrossClassShapes() throws Exception { + for (Class clazz : classShapes()) { + assertThat(toPackageName(clazz).isEmpty()) + .as("default-package condition for %s", clazz.getName()) + .isEqualTo(legacyDefaultPackageCondition(clazz)); + } + } + @Test public void arraysAndPrimitivesResolveToTheEmptyPackage() { assertThat(toPackageName(int.class)).isEmpty(); 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 0338636316..4bfb3c2267 100644 --- a/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessTest.java +++ b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessTest.java @@ -404,6 +404,33 @@ public void testDefaultPackageExclusionSetting() throws Exception { assertFalse("default package isn't excluded!", actual); } + /** + * WW-5677 routed {@code checkDefaultPackageAccess} through {@code toPackageName}. A class in a + * named package must still pass the gate when the setting is on. + */ + @Test + public void testDefaultPackageAccessPermitsNamedPackageClass() throws Exception { + sma.useDisallowDefaultPackageAccess(Boolean.TRUE.toString()); + + Member member = FooBar.class.getMethod("getStringField"); + + assertTrue("a named-package class is blocked!", sma.checkDefaultPackageAccess(new FooBar(), member)); + } + + /** + * WW-5677 equivalence at the shape most likely to break it: an array target reports the empty + * package under both the old {@code getPackage() == null} form and the new one, so it must stay + * blocked. The member here declares in {@code java.lang}, so only the target branch can block. + */ + @Test + public void testDefaultPackageAccessBlocksArrayTarget() throws Exception { + sma.useDisallowDefaultPackageAccess(Boolean.TRUE.toString()); + + Member member = Object.class.getMethod("toString"); + + assertFalse("an array target isn't blocked!", sma.checkDefaultPackageAccess(new String[0], member)); + } + @Test public void testDefaultPackageExclusion2() throws Exception { // given