This is an automated email from the ASF dual-hosted git repository.
lukaszlenart pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/struts.git
The following commit(s) were added to refs/heads/main by this push:
new 74d7ec0d7 WW-5677 perf(ognl): drop the residual getPackage() lookups
on the access path (#1859)
74d7ec0d7 is described below
commit 74d7ec0d7464062eb19e5b7018e9cc210191f24a
Author: Lukasz Lenart <[email protected]>
AuthorDate: Sun Aug 23 19:06:38 2026 +0200
WW-5677 perf(ognl): drop the residual getPackage() lookups on the access
path (#1859)
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 <[email protected]>
---
.../apache/struts2/ognl/SecurityMemberAccess.java | 17 +++++++++++---
.../SecurityMemberAccessPackageMatchingTest.java | 24 +++++++++++++++++++
.../struts2/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 4d552d594..10fdf65ce 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 @@ public class SecurityMemberAccess implements MemberAccess {
}
/**
+ * Blocks access to classes in the default (unnamed) package.
+ * <p>
+ * 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 @@ public class SecurityMemberAccess implements MemberAccess {
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 @@ public class SecurityMemberAccess implements MemberAccess {
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 class SecurityMemberAccess implements MemberAccess {
}
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 3dfa540dd..d333b20af 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 @@ public class SecurityMemberAccessPackageMatchingTest {
.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 class SecurityMemberAccessPackageMatchingTest {
}
}
+ /**
+ * 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 033863631..4bfb3c226 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 class SecurityMemberAccessTest {
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
