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 81b34c295 WW-5674 Cut the per-call allocations in SecurityMemberAccess
package matching (#1830)
81b34c295 is described below
commit 81b34c295f7e738060c0dacbb6501863cf92692f
Author: Lukasz Lenart <[email protected]>
AuthorDate: Fri Aug 14 13:14:09 2026 +0200
WW-5674 Cut the per-call allocations in SecurityMemberAccess package
matching (#1830)
* WW-5674 docs(ognl): add design for allocation-free
isClassBelongsToPackages
Covers sample 1 of WW-5667: the per-OGNL-access split/stream/join cost in
SecurityMemberAccess.isClassBelongsToPackages. Sample 2 (config re-parsing
caused by the PROTOTYPE bean scope) is tracked separately as WW-5675.
Records the current prefix-matching semantics verified against JDK 17,
including the default-package contains("") edge reachable via
struts.excludedPackageNames="." and the unreachable trailing-dot divergence.
Keeps array and primitive package semantics unchanged, since adopting
getPackageName() there tightens the exclusion list but loosens the
allowlist.
Co-Authored-By: Claude Opus 5 <[email protected]>
* WW-5674 docs(ognl): add implementation plan for allocation-free package
matching
Four TDD tasks: characterise current behaviour, swap toPackageName to the
cached getPackageName() behind an array/primitive guard, replace the
split/stream prefix construction with an index walk, then collapse the
allowlist path's two walks into one.
Task 1 is a characterisation suite that must pass against unmodified code;
a failure there means the spec's semantic claims are wrong.
Co-Authored-By: Claude Opus 5 <[email protected]>
* WW-5674 test(ognl): characterise SecurityMemberAccess package matching
Pins the current behaviour of isClassBelongsToPackages and toPackageName
before the WW-5674 rewrite, including the default-package empty-string edge
reachable via struts.excludedPackageNames="." and the package-boundary case
where org.apache.struts2x must not match org.apache.struts2.
* WW-5674 perf(ognl): resolve package names via cached Class.getPackageName
getPackage() performs a classloader package-map lookup on every call; the
name
returned by getPackageName() is computed once and cached on the Class. The
isArray()/isPrimitive() guard covers exactly the inputs for which
getPackage()
returns null, so results are unchanged for every class shape.
* WW-5674 perf(ognl): walk package names in place instead of building
prefixes
Replaces the split/IntStream/String.join prefix construction with an index
walk,
extracted into a pure package-private helper so it can be tested against
package
name shapes no real Class can produce. Per call this drops a String[], a
list
wrapper, a stream pipeline, N sublist views and N joined strings, leaving
one
substring per package level.
Equivalence with the replaced implementation is asserted over a matrix of
package name shapes and candidate sets.
* WW-5674 perf(ognl): match both allowlist package sets in one walk
isClassAllowlisted walked the class's package name twice, once for
ALLOWLIST_REQUIRED_PACKAGES and once for the configured allowlist. A two-set
overload probes both sets at each prefix, halving the work on a path that
runs
for every OGNL member access.
Asserted equivalent to OR-ing the two single-set calls across a matrix of
class
shapes and candidate sets.
Co-Authored-By: Claude Opus 5 <[email protected]>
* WW-5674 test(ognl): assert package-boundary case against the live gate
The sibling-package test asserted only against the test-local copy of the
replaced implementation, so it would have stayed green even if the
production
walk were gutted. It now asserts on both the live helper and the frozen
oracle.
Also narrows the three-argument isClassBelongsToPackages overload to
package-private: it has a single caller and its test is in the same package,
and public static on a public class is frozen API until the next major
release.
Adds a candidate set that makes the consecutive-dot prefix the deciding
probe,
and corrects two inaccuracies in the design document.
* WW-5674 docs(ognl): align the design doc with the package-private overload
The three-argument isClassBelongsToPackages was narrowed to package-private
during the final review, but section 3 still showed it as public static and
still carried the superseded justification for publishing it.
Co-Authored-By: Claude Opus 5 <[email protected]>
* WW-5674 docs(ognl): correct overstated claims and record the trailing-dot
direction
Copilot's review is right that "allocation-free" overclaims: the walk still
creates one substring per package level. What it removes is everything
around
that. Retitles the spec and plan accordingly and softens the goal statement.
Documents on isPackageBelongsToPackages that its one divergence from the
replaced implementation is directional. A package name ending in '.' probes
one
prefix more, which tightens exclusion but loosens the allowlist. No caller
can
produce one today, but the helper is a package-private pure String
function, so
a future caller routing some other string through it would inherit the
problem.
Also aligns the plan with the package-private overload it now ships, and
lists
WW-5676 and WW-5677 as filed rather than pending, including
checkDefaultPackageAccess
which the spec previously omitted from its out-of-scope list.
Co-Authored-By: Claude Opus 5 <[email protected]>
* WW-5674 docs(ognl): note the naming cleanup deferred to WW-5678
Records why the three-argument overload is package-private while sharing a
name
with a public method, and that renaming it — plus narrowing the two public
statics with no external callers — is tracked against 8.0.0.
Co-Authored-By: Claude Opus 5 <[email protected]>
---------
Co-authored-by: Claude Opus 5 <[email protected]>
---
.../apache/struts2/ognl/SecurityMemberAccess.java | 67 ++-
.../SecurityMemberAccessPackageMatchingTest.java | 224 ++++++++
...-WW-5674-isclassbelongstopackages-allocation.md | 625 +++++++++++++++++++++
...4-isclassbelongstopackages-allocation-design.md | 381 +++++++++++++
4 files changed, 1287 insertions(+), 10 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 d25bbe377..badad3dee 100644
--- a/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java
+++ b/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java
@@ -32,11 +32,9 @@ import java.lang.reflect.Constructor;
import java.lang.reflect.Field;
import java.lang.reflect.Member;
import java.lang.reflect.Modifier;
-import java.util.List;
import java.util.Set;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
-import java.util.stream.IntStream;
import static java.text.MessageFormat.format;
import static java.util.Collections.emptySet;
@@ -256,8 +254,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)
- || isClassBelongsToPackages(clazz, allowlistPackageNames);
+ || isClassBelongsToPackages(clazz,
ALLOWLIST_REQUIRED_PACKAGES, allowlistPackageNames);
}
/**
@@ -372,10 +369,16 @@ public class SecurityMemberAccess implements MemberAccess
{
}
public static String toPackageName(Class<?> clazz) {
- if (clazz.getPackage() == null) {
+ // Class.getPackage() resolves through the defining classloader's
package map on every
+ // call, whereas getPackageName() is computed once and cached on the
Class. getPackage()
+ // returns null for exactly arrays, primitives and void, so the guard
reproduces the
+ // previous result for every input. Note that void.class.isPrimitive()
is true.
+ // Arrays deliberately keep the empty package here: getPackageName()
would resolve them
+ // to the element type's package, which would loosen the allowlist.
See WW-5674.
+ if (clazz.isArray() || clazz.isPrimitive()) {
return "";
}
- return clazz.getPackage().getName();
+ return clazz.getPackageName();
}
protected boolean isExcludedPackageNamePatterns(Class<?> clazz) {
@@ -387,10 +390,54 @@ public class SecurityMemberAccess implements MemberAccess
{
}
public static boolean isClassBelongsToPackages(Class<?> clazz, Set<String>
matchingPackages) {
- List<String> packageParts = List.of(toPackageName(clazz).split("\\."));
- return IntStream.range(0, packageParts.size())
- .mapToObj(i -> String.join(".", packageParts.subList(0, i +
1)))
- .anyMatch(matchingPackages::contains);
+ 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);
+ }
+
+ /**
+ * 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}
+ * 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.
+ *
+ * @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
+ */
+ static boolean isPackageBelongsToPackages(String packageName, Set<String>
first, Set<String> second) {
+ if (first.isEmpty() && second.isEmpty()) {
+ return false;
+ }
+ int idx = packageName.indexOf('.');
+ while (idx != -1) {
+ String prefix = packageName.substring(0, idx);
+ if (first.contains(prefix) || second.contains(prefix)) {
+ return true;
+ }
+ idx = packageName.indexOf('.', idx + 1);
+ }
+ return first.contains(packageName) || second.contains(packageName);
}
protected boolean isClassExcluded(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
new file mode 100644
index 000000000..413b0c669
--- /dev/null
+++
b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessPackageMatchingTest.java
@@ -0,0 +1,224 @@
+/*
+ * 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.util.ConfigParseUtil;
+import org.junit.Test;
+
+import java.lang.reflect.Proxy;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.IntStream;
+
+import static java.util.Collections.emptySet;
+import static
org.apache.struts2.ognl.SecurityMemberAccess.isClassBelongsToPackages;
+import static org.apache.struts2.ognl.SecurityMemberAccess.toPackageName;
+import static org.assertj.core.api.Assertions.assertThat;
+
+/**
+ * Characterisation and equivalence tests for the static package-matching
helpers in
+ * {@link SecurityMemberAccess}, covering WW-5674.
+ * <p>
+ * These helpers gate OGNL member access, so the rewrite in WW-5674 must be
exactly
+ * behaviour-preserving. That is proven here by running the replaced
implementation
+ * side by side with the new one over a matrix of inputs.
+ */
+public class SecurityMemberAccessPackageMatchingTest {
+
+ /**
+ * The implementation replaced by WW-5674, retained verbatim apart from
taking the package
+ * name directly instead of a {@link Class}. Used as the reference oracle
for the rewrite.
+ */
+ private static boolean legacyPrefixMatch(String packageName, Set<String>
matchingPackages) {
+ List<String> packageParts = List.of(packageName.split("\\."));
+ return IntStream.range(0, packageParts.size())
+ .mapToObj(i -> String.join(".", packageParts.subList(0, i +
1)))
+ .anyMatch(matchingPackages::contains);
+ }
+
+ /**
+ * The {@code toPackageName} implementation replaced by WW-5674, retained
as the reference oracle.
+ */
+ private static String legacyToPackageName(Class<?> clazz) {
+ if (clazz.getPackage() == null) {
+ return "";
+ }
+ return clazz.getPackage().getName();
+ }
+
+ /**
+ * Package-name shapes. Deliberately excludes trailing-dot inputs such as
{@code "a.b."}:
+ * {@code split} drops trailing empty segments where an index walk would
not, and
+ * {@code Class.getPackage().getName()} cannot produce a trailing dot, so
the shape is
+ * unreachable through every caller. See the spec's "Verified current
semantics" section.
+ */
+ private static final List<String> PACKAGE_NAMES = List.of(
+ "",
+ "java",
+ "a.b.c",
+ "a..b",
+ ".a",
+ "org.apache.struts2",
+ "org.apache.struts2.ognl",
+ "org.apache.struts2x",
+ "java.io",
+ "java.io.tmp",
+ "javax.servlet.http");
+
+ private static final List<Set<String>> CANDIDATE_SETS = List.of(
+ emptySet(),
+ Set.of(""),
+ Set.of("java"),
+ Set.of("java.io"),
+ Set.of("org.apache.struts2"),
+ Set.of("a"),
+ Set.of("a."),
+ Set.of("a.b"),
+ Set.of("zzz.not.matching"),
+ Set.of("java.io", "org.apache.struts2", "javax"));
+
+ private static List<Class<?>> classShapes() throws Exception {
+ return List.of(
+ String.class,
+ Map.Entry.class,
+ SecurityMemberAccess.class,
+ Class.forName("PackagelessAction"),
+ int.class,
+ void.class,
+ int[].class,
+ String[].class,
+ String[][].class,
+ ((Runnable) () -> {
+ }).getClass(),
+ Proxy.newProxyInstance(
+
SecurityMemberAccessPackageMatchingTest.class.getClassLoader(),
+ new Class<?>[]{Runnable.class},
+ (proxy, method, args) -> null).getClass());
+ }
+
+ @Test
+ public void siblingPackageWithSharedCharacterPrefixDoesNotMatch() {
+ Set<String> excluded = Set.of("org.apache.struts2");
+
+
assertThat(SecurityMemberAccess.isPackageBelongsToPackages("org.apache.struts2x",
excluded, emptySet()))
+ .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()))
+ .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()))
+ .as("a sub-package must match (production)")
+ .isTrue();
+ assertThat(legacyPrefixMatch("org.apache.struts2.ognl", excluded))
+ .as("a sub-package must match (legacy oracle)")
+ .isTrue();
+ }
+
+ @Test
+ public void dotOnlyConfigurationYieldsEmptyStringPackageName() {
+ assertThat(ConfigParseUtil.toPackageNamesSet("."))
+ .as("struts.excludedPackageNames=\".\" strips to the empty
string")
+ .containsExactly("");
+ }
+
+ @Test
+ public void defaultPackageMatchesOnlyWhenEmptyStringConfigured() throws
Exception {
+ Class<?> packageless = Class.forName("PackagelessAction");
+
+ assertThat(toPackageName(packageless)).isEmpty();
+ assertThat(isClassBelongsToPackages(packageless, Set.of("")))
+ .as("a default-package class is matched by the empty-string
entry")
+ .isTrue();
+ assertThat(isClassBelongsToPackages(packageless, Set.of("java")))
+ .as("a default-package class is not matched by an unrelated
entry")
+ .isFalse();
+ }
+
+ @Test
+ public void toPackageNameMatchesLegacyAcrossClassShapes() throws Exception
{
+ for (Class<?> clazz : classShapes()) {
+ assertThat(toPackageName(clazz))
+ .as("toPackageName(%s)", clazz.getName())
+ .isEqualTo(legacyToPackageName(clazz));
+ }
+ }
+
+ @Test
+ public void arraysAndPrimitivesResolveToTheEmptyPackage() {
+ assertThat(toPackageName(int.class)).isEmpty();
+ assertThat(toPackageName(void.class)).isEmpty();
+ assertThat(toPackageName(int[].class)).isEmpty();
+ assertThat(toPackageName(String[].class)).isEmpty();
+ assertThat(toPackageName(String[][].class)).isEmpty();
+ }
+
+ @Test
+ public void classEntryPointMatchesLegacyAcrossCandidateSets() throws
Exception {
+ for (Class<?> clazz : classShapes()) {
+ for (Set<String> candidates : CANDIDATE_SETS) {
+ assertThat(isClassBelongsToPackages(clazz, candidates))
+ .as("clazz=[%s] candidates=%s", clazz.getName(),
candidates)
+
.isEqualTo(legacyPrefixMatch(legacyToPackageName(clazz), candidates));
+ }
+ }
+ }
+
+ @Test
+ public void indexWalkMatchesLegacyAcrossPackageNameShapes() {
+ for (String packageName : PACKAGE_NAMES) {
+ for (Set<String> candidates : CANDIDATE_SETS) {
+
assertThat(SecurityMemberAccess.isPackageBelongsToPackages(packageName,
candidates, emptySet()))
+ .as("packageName=[%s] candidates=%s", packageName,
candidates)
+ .isEqualTo(legacyPrefixMatch(packageName, candidates));
+ }
+ }
+ }
+
+ @Test
+ public void bothSetsEmptyShortCircuitsToFalse() {
+ for (String packageName : PACKAGE_NAMES) {
+
assertThat(SecurityMemberAccess.isPackageBelongsToPackages(packageName,
emptySet(), emptySet()))
+ .as("packageName=[%s] with no configured packages",
packageName)
+ .isFalse();
+ }
+ }
+
+ @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));
+ }
+ }
+ }
+ }
+}
diff --git
a/docs/superpowers/plans/2026-08-03-WW-5674-isclassbelongstopackages-allocation.md
b/docs/superpowers/plans/2026-08-03-WW-5674-isclassbelongstopackages-allocation.md
new file mode 100644
index 000000000..612316363
--- /dev/null
+++
b/docs/superpowers/plans/2026-08-03-WW-5674-isclassbelongstopackages-allocation.md
@@ -0,0 +1,625 @@
+# WW-5674 — Reduce `isClassBelongsToPackages` Allocations Implementation Plan
+
+> **For agentic workers:** REQUIRED SUB-SKILL: Use
superpowers:subagent-driven-development (recommended) or
superpowers:executing-plans to implement this plan task-by-task. Steps use
checkbox (`- [ ]`) syntax for tracking.
+
+**Goal:** Remove the per-OGNL-access allocation overhead in
`SecurityMemberAccess.isClassBelongsToPackages` and `toPackageName`, with zero
change to allow/deny semantics proven by test.
+
+**Architecture:** Replace a `split` + `IntStream` + `String.join` prefix
construction with an index walk over the package-name string, extracted into a
package-private pure function so it can be tested against shapes no real
`Class` can produce. Swap `Class.getPackage().getName()` for the cached
`Class.getPackageName()` behind an `isArray()/isPrimitive()` guard that
reproduces the old result exactly. Collapse the allowlist path's two walks into
one via a two-set overload.
+
+**Tech Stack:** Java 17, Maven, JUnit 4, AssertJ, Mockito.
+
+**Spec:**
`docs/superpowers/specs/2026-08-03-WW-5674-isclassbelongstopackages-allocation-design.md`
+
+**Ticket:** [WW-5674](https://issues.apache.org/jira/browse/WW-5674), sub-task
of [WW-5667](https://issues.apache.org/jira/browse/WW-5667)
+
+## Global Constraints
+
+- **Java release target is 17** (`maven.compiler.release=17` in root
`pom.xml`). `Class.getPackageName()` is Java 9+, so it is available.
+- **Core tests are JUnit 4, never JUnit 5.** Use `org.junit.Test` and
`org.junit.Before`. An `@org.junit.jupiter.api.Test` added here silently never
runs.
+- **AssertJ** (`org.assertj.core.api.Assertions.assertThat`) and **Mockito**
are already on the core test classpath.
+- **Zero behavior change is the acceptance criterion.** Any test that changes
an existing assertion means the change is wrong, not the test.
+- **Do not change array or primitive package semantics.** `toPackageName` must
keep returning `""` for arrays, primitives and `void`. This is deliberate — see
the spec's *Deliberately out of scope* section. Adopting `getPackageName()`
semantics there loosens the allowlist.
+- **Branch is `WW-5674-isclassbelongstopackages-allocation`, already checked
out.** Never commit to `main`.
+- **Commit message format:** `WW-5674 <type>(<scope>): <description>`, e.g.
`WW-5674 test(ognl): ...`. Ticket prefix is mandatory.
+- **No JMH, no timing assertions.** The project has no benchmark harness and
none is added. Wall-clock assertions are unreliable in CI.
+- Single test run: `mvn test -DskipAssembly -pl core -Dtest=<ClassName>`
+- Full core suite: `mvn test -DskipAssembly -pl core`
+
+## File Structure
+
+| File | Responsibility |
+|---|---|
+| `core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java` |
Modified. Lines 374–379 (`toPackageName`), 389–394
(`isClassBelongsToPackages`), 254–261 (`isClassAllowlisted`), imports at 35 and
39. |
+|
`core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessPackageMatchingTest.java`
| Created. All new tests for the static package-matching utilities. |
+| `core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessTest.java` |
Untouched. Must stay green without edits. |
+| `core/src/test/java/PackagelessAction.java` | Existing default-package
class, reused via `Class.forName("PackagelessAction")`. Do not modify. |
+
+**Note on test file placement.** The spec named
`SecurityMemberAccessTest.java` as the test home. That file is 1136 lines, and
the new tests are a self-contained block of pure-static table-driven checks
that need `List` and `IntStream` imports the existing file does not have. They
go in a new focused test class in the same package instead —
`isPackageBelongsToPackages` is package-private, so the test must live in
`org.apache.struts2.ognl`. This is a deliberate, flagged deviation and it s
[...]
+
+---
+
+### Task 1: Characterization tests that lock in current behavior
+
+Create the test file and pin the *existing* behavior before touching any
production code. These tests exercise only the current public API
(`isClassBelongsToPackages(Class, Set)` and `toPackageName(Class)`), so they
**pass immediately against unmodified code**.
+
+This is intentional and is the gate for the whole plan: if any assertion here
fails, the semantic claims in the spec are wrong and you must stop and
re-derive them rather than "fixing" the test.
+
+**Files:**
+- Create:
`core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessPackageMatchingTest.java`
+
+**Interfaces:**
+- Consumes: existing `SecurityMemberAccess.isClassBelongsToPackages(Class<?>,
Set<String>)`, `SecurityMemberAccess.toPackageName(Class<?>)`,
`ConfigParseUtil.toPackageNamesSet(String)`.
+- Produces: the constants `PACKAGE_NAMES` and `CANDIDATE_SETS`, and the
helpers `legacyPrefixMatch(String, Set<String>)` and
`legacyToPackageName(Class<?>)`, all reused by Tasks 3 and 4.
+
+- [ ] **Step 1: Create the test file**
+
+```java
+/*
+ * 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.util.ConfigParseUtil;
+import org.junit.Test;
+
+import java.lang.reflect.Proxy;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.IntStream;
+
+import static java.util.Collections.emptySet;
+import static
org.apache.struts2.ognl.SecurityMemberAccess.isClassBelongsToPackages;
+import static org.apache.struts2.ognl.SecurityMemberAccess.toPackageName;
+import static org.assertj.core.api.Assertions.assertThat;
+
+/**
+ * Characterisation and equivalence tests for the static package-matching
helpers in
+ * {@link SecurityMemberAccess}, covering WW-5674.
+ * <p>
+ * These helpers gate OGNL member access, so the rewrite in WW-5674 must be
exactly
+ * behaviour-preserving. That is proven here by running the replaced
implementation
+ * side by side with the new one over a matrix of inputs.
+ */
+public class SecurityMemberAccessPackageMatchingTest {
+
+ /**
+ * The implementation replaced by WW-5674, retained verbatim apart from
taking the package
+ * name directly instead of a {@link Class}. Used as the reference oracle
for the rewrite.
+ */
+ private static boolean legacyPrefixMatch(String packageName, Set<String>
matchingPackages) {
+ List<String> packageParts = List.of(packageName.split("\\."));
+ return IntStream.range(0, packageParts.size())
+ .mapToObj(i -> String.join(".", packageParts.subList(0, i +
1)))
+ .anyMatch(matchingPackages::contains);
+ }
+
+ /**
+ * The {@code toPackageName} implementation replaced by WW-5674, retained
as the reference oracle.
+ */
+ private static String legacyToPackageName(Class<?> clazz) {
+ if (clazz.getPackage() == null) {
+ return "";
+ }
+ return clazz.getPackage().getName();
+ }
+
+ /**
+ * Package-name shapes. Deliberately excludes trailing-dot inputs such as
{@code "a.b."}:
+ * {@code split} drops trailing empty segments where an index walk would
not, and
+ * {@code Class.getPackage().getName()} cannot produce a trailing dot, so
the shape is
+ * unreachable through every caller. See the spec's "Verified current
semantics" section.
+ */
+ private static final List<String> PACKAGE_NAMES = List.of(
+ "",
+ "java",
+ "a.b.c",
+ "a..b",
+ ".a",
+ "org.apache.struts2",
+ "org.apache.struts2.ognl",
+ "org.apache.struts2x",
+ "java.io",
+ "java.io.tmp",
+ "javax.servlet.http");
+
+ private static final List<Set<String>> CANDIDATE_SETS = List.of(
+ emptySet(),
+ Set.of(""),
+ Set.of("java"),
+ Set.of("java.io"),
+ Set.of("org.apache.struts2"),
+ Set.of("a"),
+ Set.of("a.b"),
+ Set.of("zzz.not.matching"),
+ Set.of("java.io", "org.apache.struts2", "javax"));
+
+ private static List<Class<?>> classShapes() throws Exception {
+ return List.of(
+ String.class,
+ Map.Entry.class,
+ SecurityMemberAccess.class,
+ Class.forName("PackagelessAction"),
+ int.class,
+ void.class,
+ int[].class,
+ String[].class,
+ String[][].class,
+ ((Runnable) () -> {
+ }).getClass(),
+ Proxy.newProxyInstance(
+
SecurityMemberAccessPackageMatchingTest.class.getClassLoader(),
+ new Class<?>[]{Runnable.class},
+ (proxy, method, args) -> null).getClass());
+ }
+
+ @Test
+ public void siblingPackageWithSharedCharacterPrefixDoesNotMatch() {
+ Set<String> excluded = Set.of("org.apache.struts2");
+
+ assertThat(legacyPrefixMatch("org.apache.struts2x", excluded))
+ .as("a sibling package sharing a character prefix must not
match")
+ .isFalse();
+ assertThat(legacyPrefixMatch("org.apache.struts2", excluded))
+ .as("an exact match must match")
+ .isTrue();
+ assertThat(legacyPrefixMatch("org.apache.struts2.ognl", excluded))
+ .as("a sub-package must match")
+ .isTrue();
+ }
+
+ @Test
+ public void dotOnlyConfigurationYieldsEmptyStringPackageName() {
+ assertThat(ConfigParseUtil.toPackageNamesSet("."))
+ .as("struts.excludedPackageNames=\".\" strips to the empty
string")
+ .containsExactly("");
+ }
+
+ @Test
+ public void defaultPackageMatchesOnlyWhenEmptyStringConfigured() throws
Exception {
+ Class<?> packageless = Class.forName("PackagelessAction");
+
+ assertThat(toPackageName(packageless)).isEmpty();
+ assertThat(isClassBelongsToPackages(packageless, Set.of("")))
+ .as("a default-package class is matched by the empty-string
entry")
+ .isTrue();
+ assertThat(isClassBelongsToPackages(packageless, Set.of("java")))
+ .as("a default-package class is not matched by an unrelated
entry")
+ .isFalse();
+ }
+
+ @Test
+ public void toPackageNameMatchesLegacyAcrossClassShapes() throws Exception
{
+ for (Class<?> clazz : classShapes()) {
+ assertThat(toPackageName(clazz))
+ .as("toPackageName(%s)", clazz.getName())
+ .isEqualTo(legacyToPackageName(clazz));
+ }
+ }
+
+ @Test
+ public void arraysAndPrimitivesResolveToTheEmptyPackage() {
+ assertThat(toPackageName(int.class)).isEmpty();
+ assertThat(toPackageName(void.class)).isEmpty();
+ assertThat(toPackageName(int[].class)).isEmpty();
+ assertThat(toPackageName(String[].class)).isEmpty();
+ assertThat(toPackageName(String[][].class)).isEmpty();
+ }
+
+ @Test
+ public void classEntryPointMatchesLegacyAcrossCandidateSets() throws
Exception {
+ for (Class<?> clazz : classShapes()) {
+ for (Set<String> candidates : CANDIDATE_SETS) {
+ assertThat(isClassBelongsToPackages(clazz, candidates))
+ .as("clazz=[%s] candidates=%s", clazz.getName(),
candidates)
+
.isEqualTo(legacyPrefixMatch(legacyToPackageName(clazz), candidates));
+ }
+ }
+ }
+}
+```
+
+- [ ] **Step 2: Run the tests — they must all PASS against unmodified
production code**
+
+Run: `mvn test -DskipAssembly -pl core
-Dtest=SecurityMemberAccessPackageMatchingTest`
+
+Expected: BUILD SUCCESS, 6 tests run, 0 failures.
+
+This is a characterization suite, so passing immediately is correct. **If
anything fails, stop.** It means the spec's description of current behavior is
wrong — re-derive the semantics before changing production code. Do not edit
the assertions to make them green.
+
+- [ ] **Step 3: Commit**
+
+```bash
+git add
core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessPackageMatchingTest.java
+git commit -m "WW-5674 test(ognl): characterise SecurityMemberAccess package
matching
+
+Pins the current behaviour of isClassBelongsToPackages and toPackageName
+before the WW-5674 rewrite, including the default-package empty-string edge
+reachable via struts.excludedPackageNames=\".\" and the package-boundary case
+where org.apache.struts2x must not match org.apache.struts2."
+```
+
+---
+
+### Task 2: Make `toPackageName` use the cached `getPackageName()`
+
+Swap the classloader package-map lookup for the value cached on the `Class`,
behind a guard covering exactly the cases where `getPackage()` returns null.
+
+**Files:**
+- Modify:
`core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java:374-379`
+- Test:
`core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessPackageMatchingTest.java`
(no changes — Task 1's `toPackageNameMatchesLegacyAcrossClassShapes` and
`arraysAndPrimitivesResolveToTheEmptyPackage` are the gate)
+
+**Interfaces:**
+- Consumes: `legacyToPackageName(Class<?>)` and `classShapes()` from Task 1.
+- Produces: `SecurityMemberAccess.toPackageName(Class<?>)` — unchanged
signature `public static String`, unchanged results.
+
+- [ ] **Step 1: Replace the method body**
+
+Replace lines 374–379 of `SecurityMemberAccess.java`:
+
+```java
+ public static String toPackageName(Class<?> clazz) {
+ if (clazz.getPackage() == null) {
+ return "";
+ }
+ return clazz.getPackage().getName();
+ }
+```
+
+with:
+
+```java
+ public static String toPackageName(Class<?> clazz) {
+ // Class.getPackage() resolves through the defining classloader's
package map on every
+ // call, whereas getPackageName() is computed once and cached on the
Class. getPackage()
+ // returns null for exactly arrays, primitives and void, so the guard
reproduces the
+ // previous result for every input. Note that void.class.isPrimitive()
is true.
+ // Arrays deliberately keep the empty package here: getPackageName()
would resolve them
+ // to the element type's package, which would loosen the allowlist.
See WW-5674.
+ if (clazz.isArray() || clazz.isPrimitive()) {
+ return "";
+ }
+ return clazz.getPackageName();
+ }
+```
+
+- [ ] **Step 2: Run the tests to verify behavior is unchanged**
+
+Run: `mvn test -DskipAssembly -pl core
-Dtest=SecurityMemberAccessPackageMatchingTest`
+
+Expected: PASS, 6 tests, 0 failures.
`toPackageNameMatchesLegacyAcrossClassShapes` compares the new implementation
against the retained legacy oracle across all eleven class shapes, so a
regression here fails loudly.
+
+- [ ] **Step 3: Run the existing SecurityMemberAccess suite**
+
+Run: `mvn test -DskipAssembly -pl core -Dtest=SecurityMemberAccessTest`
+
+Expected: PASS, 0 failures, with no edits to that file.
+
+- [ ] **Step 4: Commit**
+
+```bash
+git add core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java
+git commit -m "WW-5674 perf(ognl): resolve package names via cached
Class.getPackageName
+
+getPackage() performs a classloader package-map lookup on every call; the name
+returned by getPackageName() is computed once and cached on the Class. The
+isArray()/isPrimitive() guard covers exactly the inputs for which getPackage()
+returns null, so results are unchanged for every class shape."
+```
+
+---
+
+### Task 3: Replace the prefix construction with an index walk
+
+Extract the walk into a package-private pure function over the package-name
string, and delegate the existing public method to it.
+
+**Files:**
+- Modify:
`core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java:389-394`
(method body), `:35` and `:39` (imports)
+- Modify:
`core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessPackageMatchingTest.java`
(add one test)
+
+**Interfaces:**
+- Consumes: `toPackageName(Class<?>)` from Task 2; `legacyPrefixMatch(String,
Set<String>)`, `PACKAGE_NAMES`, `CANDIDATE_SETS` from Task 1.
+- Produces: `static boolean
SecurityMemberAccess.isPackageBelongsToPackages(String packageName, Set<String>
first, Set<String> second)` — package-private, pure, no allocation beyond one
substring per package level. Consumed by Task 4.
+
+- [ ] **Step 1: Write the failing test**
+
+Add to `SecurityMemberAccessPackageMatchingTest`:
+
+```java
+ @Test
+ public void indexWalkMatchesLegacyAcrossPackageNameShapes() {
+ for (String packageName : PACKAGE_NAMES) {
+ for (Set<String> candidates : CANDIDATE_SETS) {
+
assertThat(SecurityMemberAccess.isPackageBelongsToPackages(packageName,
candidates, emptySet()))
+ .as("packageName=[%s] candidates=%s", packageName,
candidates)
+ .isEqualTo(legacyPrefixMatch(packageName, candidates));
+ }
+ }
+ }
+
+ @Test
+ public void bothSetsEmptyShortCircuitsToFalse() {
+ for (String packageName : PACKAGE_NAMES) {
+
assertThat(SecurityMemberAccess.isPackageBelongsToPackages(packageName,
emptySet(), emptySet()))
+ .as("packageName=[%s] with no configured packages",
packageName)
+ .isFalse();
+ }
+ }
+```
+
+- [ ] **Step 2: Run test to verify it fails**
+
+Run: `mvn test -DskipAssembly -pl core
-Dtest=SecurityMemberAccessPackageMatchingTest`
+
+Expected: COMPILATION FAILURE — `cannot find symbol: method
isPackageBelongsToPackages(String,Set<String>,Set<String>)`. That is the red
state for this task.
+
+- [ ] **Step 3: Write the implementation**
+
+Replace lines 389–394 of `SecurityMemberAccess.java`:
+
+```java
+ public static boolean isClassBelongsToPackages(Class<?> clazz, Set<String>
matchingPackages) {
+ List<String> packageParts = List.of(toPackageName(clazz).split("\\."));
+ return IntStream.range(0, packageParts.size())
+ .mapToObj(i -> String.join(".", packageParts.subList(0, i +
1)))
+ .anyMatch(matchingPackages::contains);
+ }
+```
+
+with:
+
+```java
+ public static boolean isClassBelongsToPackages(Class<?> clazz, Set<String>
matchingPackages) {
+ return isPackageBelongsToPackages(toPackageName(clazz),
matchingPackages, emptySet());
+ }
+
+ /**
+ * 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}
+ * short-circuit earliest.
+ *
+ * @param packageName the package name to test, empty for the default
package
+ * @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
+ */
+ static boolean isPackageBelongsToPackages(String packageName, Set<String>
first, Set<String> second) {
+ if (first.isEmpty() && second.isEmpty()) {
+ return false;
+ }
+ int idx = packageName.indexOf('.');
+ while (idx != -1) {
+ String prefix = packageName.substring(0, idx);
+ if (first.contains(prefix) || second.contains(prefix)) {
+ return true;
+ }
+ idx = packageName.indexOf('.', idx + 1);
+ }
+ return first.contains(packageName) || second.contains(packageName);
+ }
+```
+
+- [ ] **Step 4: Remove the now-unused imports**
+
+Delete line 35 (`import java.util.List;`) and line 39 (`import
java.util.stream.IntStream;`) from `SecurityMemberAccess.java`. Both are used
only by the code just replaced — verify with:
+
+```bash
+grep -n '\bList\b\|\bIntStream\b'
core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java
+```
+
+Expected after deletion: no output. `emptySet` is already statically imported
at line 42 and is now used by the delegate; leave it.
+
+- [ ] **Step 5: Run tests to verify they pass**
+
+Run: `mvn test -DskipAssembly -pl core
-Dtest=SecurityMemberAccessPackageMatchingTest`
+
+Expected: PASS, 8 tests, 0 failures.
+
+- [ ] **Step 6: Run the existing SecurityMemberAccess suite**
+
+Run: `mvn test -DskipAssembly -pl core -Dtest=SecurityMemberAccessTest`
+
+Expected: PASS, 0 failures, still with no edits to that file.
+
+- [ ] **Step 7: Commit**
+
+```bash
+git add core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java \
+
core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessPackageMatchingTest.java
+git commit -m "WW-5674 perf(ognl): walk package names in place instead of
building prefixes
+
+Replaces the split/IntStream/String.join prefix construction with an index
walk,
+extracted into a pure package-private helper so it can be tested against
package
+name shapes no real Class can produce. Per call this drops a String[], a list
+wrapper, a stream pipeline, N sublist views and N joined strings, leaving one
+substring per package level.
+
+Equivalence with the replaced implementation is asserted over a matrix of
+package name shapes and candidate sets."
+```
+
+---
+
+### Task 4: Collapse the allowlist path to a single walk
+
+`isClassAllowlisted` walks the same package name twice, once per allowlist
set. Add a two-set overload and use it.
+
+**Files:**
+- Modify:
`core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java:254-261`
(`isClassAllowlisted`) and the `isClassBelongsToPackages` block from Task 3
+- Modify:
`core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessPackageMatchingTest.java`
(add one test)
+
+**Interfaces:**
+- Consumes: `isPackageBelongsToPackages(String, Set<String>, Set<String>)`
from Task 3.
+- Produces: `static boolean
SecurityMemberAccess.isClassBelongsToPackages(Class<?> clazz, Set<String>
first, Set<String> second)` — package-private, matching
`isPackageBelongsToPackages` beside it. (The plan originally specified `public
static`; it was narrowed during the final review, since the overload has one
caller and its only test is in the same package.)
+
+- [ ] **Step 1: Write the failing test**
+
+Add to `SecurityMemberAccessPackageMatchingTest`:
+
+```java
+ @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));
+ }
+ }
+ }
+ }
+```
+
+- [ ] **Step 2: Run test to verify it fails**
+
+Run: `mvn test -DskipAssembly -pl core
-Dtest=SecurityMemberAccessPackageMatchingTest`
+
+Expected: COMPILATION FAILURE — `cannot find symbol: method
isClassBelongsToPackages(Class<CAP#1>,Set<String>,Set<String>)`. That is the
red state for this task.
+
+- [ ] **Step 3: Add the overload**
+
+In `SecurityMemberAccess.java`, replace the two-argument method written in
Task 3:
+
+```java
+ public static boolean isClassBelongsToPackages(Class<?> clazz, Set<String>
matchingPackages) {
+ return isPackageBelongsToPackages(toPackageName(clazz),
matchingPackages, emptySet());
+ }
+```
+
+with the delegating pair:
+
+```java
+ 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);
+ }
+```
+
+- [ ] **Step 4: Use the overload in `isClassAllowlisted`**
+
+In `SecurityMemberAccess.java`, replace the final two clauses of
`isClassAllowlisted` (lines 259–260):
+
+```java
+ || isClassBelongsToPackages(clazz, ALLOWLIST_REQUIRED_PACKAGES)
+ || isClassBelongsToPackages(clazz, allowlistPackageNames);
+```
+
+with a single clause:
+
+```java
+ || isClassBelongsToPackages(clazz,
ALLOWLIST_REQUIRED_PACKAGES, allowlistPackageNames);
+```
+
+The full method then reads:
+
+```java
+ protected boolean isClassAllowlisted(Class<?> clazz) {
+ return allowlistClasses.contains(clazz)
+ || ALLOWLIST_REQUIRED_CLASSES.contains(clazz)
+ || (providerAllowlist != null &&
providerAllowlist.getProviderAllowlist().contains(clazz))
+ || (threadAllowlist != null &&
threadAllowlist.getAllowlist().contains(clazz))
+ || isClassBelongsToPackages(clazz,
ALLOWLIST_REQUIRED_PACKAGES, allowlistPackageNames);
+ }
+```
+
+- [ ] **Step 5: Run tests to verify they pass**
+
+Run: `mvn test -DskipAssembly -pl core
-Dtest=SecurityMemberAccessPackageMatchingTest`
+
+Expected: PASS, 9 tests, 0 failures.
+
+- [ ] **Step 6: Run the full core suite**
+
+Run: `mvn test -DskipAssembly -pl core`
+
+Expected: BUILD SUCCESS, 0 failures, 0 errors. This is the real gate —
`SecurityMemberAccessTest`, `OgnlValueStackTest`, `OgnlUtilTest` and the
allowlist tests all exercise these paths end to end. Confirm
`SecurityMemberAccessTest.java` is still unmodified:
+
+```bash
+git status --porcelain
core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessTest.java
+```
+
+Expected: no output.
+
+- [ ] **Step 7: Commit**
+
+```bash
+git add core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java \
+
core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessPackageMatchingTest.java
+git commit -m "WW-5674 perf(ognl): match both allowlist package sets in one
walk
+
+isClassAllowlisted walked the class's package name twice, once for
+ALLOWLIST_REQUIRED_PACKAGES and once for the configured allowlist. A two-set
+overload probes both sets at each prefix, halving the work on a path that runs
+for every OGNL member access.
+
+Asserted equivalent to OR-ing the two single-set calls across a matrix of class
+shapes and candidate sets."
+```
+
+---
+
+## Self-Review
+
+**Spec coverage.** Every requirement maps to a task:
+
+| Spec requirement | Task |
+|---|---|
+| `toPackageName` guard + `getPackageName()` | Task 2 |
+| Extract `isPackageBelongsToPackages` (package-private) | Task 3 |
+| Index walk, shortest-prefix-first, `isEmpty()` short-circuit | Task 3 |
+| Two public entry points delegate to one walk | Tasks 3, 4 |
+| Single walk in `isClassAllowlisted` | Task 4 |
+| Import cleanup (`List`, `IntStream`) | Task 3, Step 4 |
+| Test 1 — differential vs legacy over shapes | Task 3,
`indexWalkMatchesLegacyAcrossPackageNameShapes` |
+| Test 2 — package-boundary regression | Task 1,
`siblingPackageWithSharedCharacterPrefixDoesNotMatch` |
+| Test 3 — default-package `""` edge | Task 1,
`defaultPackageMatchesOnlyWhenEmptyStringConfigured` +
`dotOnlyConfigurationYieldsEmptyStringPackageName` |
+| Test 4 — `toPackageName` over 11 class shapes | Task 1,
`toPackageNameMatchesLegacyAcrossClassShapes` |
+| Test 5 — two-set overload equivalence | Task 4,
`twoSetOverloadEqualsDisjunctionOfSingleSetCalls` |
+| Test 6 — existing suite unchanged, full core green | Tasks 2, 3, 4 (Step 6) |
+| No JMH, no timing assertions | Global Constraints |
+| Array/primitive semantics unchanged | Task 2 comment +
`arraysAndPrimitivesResolveToTheEmptyPackage` |
+
+One spec deviation, flagged in *File Structure*: tests live in a new
`SecurityMemberAccessPackageMatchingTest` rather than the 1136-line
`SecurityMemberAccessTest`.
+
+**Placeholder scan.** No TBD/TODO, no "handle edge cases", no "similar to Task
N". Every code step carries complete, compilable content.
+
+**Type consistency.** `isPackageBelongsToPackages(String, Set<String>,
Set<String>)` is package-private and named identically in Tasks 3 and 4.
`isClassBelongsToPackages` keeps its two-argument signature throughout and
gains a three-argument overload in Task 4 only. `legacyPrefixMatch(String,
Set<String>)`, `legacyToPackageName(Class<?>)`, `classShapes()`,
`PACKAGE_NAMES` and `CANDIDATE_SETS` are defined once in Task 1 and referenced
under those exact names in Tasks 3 and 4. `classShapes [...]
+
+## Follow-ups not in scope
+
+All of these are now filed, so they may be referenced from code and commit
messages without breaching the project's no-placeholder-TODO rule.
+
+- **WW-5675** covers the dominant cost (config re-parsing driven by the
`Scope.PROTOTYPE` bean). WW-5674 alone will not move the 9% figure much. It
also absorbed **`ConfigParseUtil.validatePackageNames`**
(`ConfigParseUtil.java:143`), which evaluates `Pattern.compile("\\s")` once per
package name rather than once overall — same root cause, per-instantiation work
that should happen once.
+- **WW-5676** — whether array and primitive types should resolve to their
element package. Tightens the exclusion list, loosens the allowlist. Filed as a
standalone Improvement against 7.4.0 rather than a sub-task, because it is a
security-semantics decision rather than a performance fix.
+- **WW-5677** — the remaining per-access `getPackage()` lookups in
`checkDefaultPackageAccess` and `isExcludedPackageNamePatterns`. Same file as
this plan, same hot path, but left alone here to keep this change reviewable as
a single concern.
diff --git
a/docs/superpowers/specs/2026-08-03-WW-5674-isclassbelongstopackages-allocation-design.md
b/docs/superpowers/specs/2026-08-03-WW-5674-isclassbelongstopackages-allocation-design.md
new file mode 100644
index 000000000..7f26fae6c
--- /dev/null
+++
b/docs/superpowers/specs/2026-08-03-WW-5674-isclassbelongstopackages-allocation-design.md
@@ -0,0 +1,381 @@
+# WW-5674 — Cut the per-call allocations in
`SecurityMemberAccess.isClassBelongsToPackages`
+
+> The walk is allocation-*reduced*, not allocation-free: it still creates one
+> `substring` per package level. What it removes is everything around that —
the
+> `String[]`, the list wrapper, the stream pipeline, the sublist views, and the
+> joined result strings.
+
+**Date:** 2026-08-03
+**Ticket:** [WW-5674](https://issues.apache.org/jira/browse/WW-5674) (sub-task
of [WW-5667](https://issues.apache.org/jira/browse/WW-5667))
+**Sibling:** [WW-5675](https://issues.apache.org/jira/browse/WW-5675) — config
re-parsing on every `SecurityMemberAccess` instantiation (separate spec)
+
+## Background
+
+WW-5667 reports that OGNL security checks consume 9% of RUNNABLE CPU samples
in a
+2-minute JFR profile of a preprod Payara server under moderate load. The report
+contains two stack samples that point at two independent problems.
+
+This spec covers **sample 1** only — the per-OGNL-access cost of
+`SecurityMemberAccess.isClassBelongsToPackages`:
+
+```
+java.lang.String.split(String)
+SecurityMemberAccess.isClassBelongsToPackages(Class, Set) :390
+SecurityMemberAccess.isExcludedPackageNames(Class) :386
+SecurityMemberAccess.isPackageExcluded(Class) :371
+```
+
+Sample 2 — repeated re-parsing of the raw configuration strings, caused by
+`SecurityMemberAccess` being a `Scope.PROTOTYPE` bean — is the dominant cost
but
+is a different change with a different risk profile. It is tracked separately
as
+WW-5675.
+
+The fix proposed on WW-5667 (cache the parsed `Set` in a `SecurityMemberAccess`
+field) addresses neither problem: it does not touch this hot path at all, and
it
+cannot help sample 2 because the instance holding the field is itself discarded
+and rebuilt on each container lookup.
+
+## Problem
+
+```java
+public static boolean isClassBelongsToPackages(Class<?> clazz, Set<String>
matchingPackages) {
+ List<String> packageParts = List.of(toPackageName(clazz).split("\\."));
+ return IntStream.range(0, packageParts.size())
+ .mapToObj(i -> String.join(".", packageParts.subList(0, i + 1)))
+ .anyMatch(matchingPackages::contains);
+}
+```
+
+For a class whose package has N segments, one call allocates: a `String[]` plus
+its N element substrings from `split`, a `List.of` wrapper, an `IntStream`
+pipeline, N `subList` views, and N `StringJoiner`-built result strings. For
+`org.apache.struts2.ognl` that is roughly a dozen objects.
+
+The method is invoked up to four times per `isAccessible()` call — the
+excluded-package check and the allowlist check, each applied to both the
+member's declaring class and the target class.
+
+A second, smaller cost sits underneath it:
+
+```java
+public static String toPackageName(Class<?> clazz) {
+ if (clazz.getPackage() == null) {
+ return "";
+ }
+ return clazz.getPackage().getName();
+}
+```
+
+`Class.getPackage()` resolves through the defining classloader's package map on
+every call. `Class.getPackageName()` (Java 9+) computes the name once and
caches
+it on the `Class` object.
+
+## Goals
+
+- Cut the per-call allocation overhead on the OGNL member-access hot path down
to
+ one substring per package level.
+- **Zero change to allow/deny semantics**, demonstrated by test, not by
argument.
+- Keep the change small enough to review as a pure optimisation.
+
+## Non-goals
+
+- The `Scope.PROTOTYPE` config re-parsing (WW-5675).
+- Changing how arrays and primitives resolve to package names — see
+ *Deliberately out of scope* below.
+- Any caching layer, memoisation, or new data structure.
+- Any change to `struts.excludedPackageNames`, `struts.allowlist.packageNames`,
+ or the surrounding configuration.
+
+## Verified current semantics
+
+The rewrite must reproduce the existing prefix set exactly. The following was
+established empirically on the target JDK (17, `maven.compiler.release=17`),
+not inferred:
+
+| package name | `split("\\.")` | prefixes probed |
+|---|---|---|
+| `""` | `[""]` (length 1) | `[""]` |
+| `"java"` | `["java"]` | `["java"]` |
+| `"a.b.c"` | `["a","b","c"]` | `["a", "a.b", "a.b.c"]` |
+| `"a..b"` | `["a","","b"]` | `["a", "a.", "a..b"]` |
+| `".a"` | `["","a"]` | `["", ".a"]` |
+| `"a.b."` | `["a","b"]` | `["a", "a.b"]` |
+
+Two consequences worth stating explicitly, because both are easy to regress:
+
+1. **The default package probes `contains("")`.** `"".split("\\.")` yields a
+ one-element array containing the empty string, so a class in the default
+ package tests the set for `""`. This is reachable in practice:
+ `commaDelimitedStringToSet` filters empty entries *before*
+ `ConfigParseUtil.toPackageNamesSet` applies `strip(s, ".")`, so a
+ configuration of `struts.excludedPackageNames="."` puts `""` into the set
and
+ excludes default-package classes. Confirmed live:
+ `isClassBelongsToPackages(defaultPkgClass, Set.of("")) == true`.
+
+2. **Trailing-dot inputs are the only divergence.** `split` drops trailing
empty
+ segments, so `"a.b."` probes `["a", "a.b"]` whereas an index walk would also
+ probe `"a.b."`. `Class.getPackage().getName()` cannot produce a trailing
dot,
+ so this shape is unreachable through every caller. It is recorded here so a
+ future reader does not mistake it for a bug.
+
+Every other shape is exactly reproducible by an index walk: probe
+`P.substring(0, j)` at each `j` where `P.charAt(j) == '.'`, then probe `P`.
+
+### `toPackageName` guard equivalence
+
+`clazz.getPackage()` returns null for exactly primitives, `void`, and arrays.
+Verified across eleven class shapes:
+
+| class | `getPackage()` | current result | `isArray()/isPrimitive()` guard |
+|---|---|---|---|
+| `String` | non-null | `"java.lang"` | `"java.lang"` |
+| default-package class | non-null | `""` | `""` |
+| nested (`Map.Entry`) | non-null | `"java.util"` | `"java.util"` |
+| lambda (hidden class) | non-null | `"org.apache.struts2.ognl"` |
`"org.apache.struts2.ognl"` |
+| JDK proxy | non-null | `"jdk.proxy1"` | `"jdk.proxy1"` |
+| `int`, `void` | null | `""` | `""` |
+| `int[]`, `String[]`, `String[][]` | null | `""` | `""` |
+
+All eleven agree. Note that `void.class.isPrimitive()` is `true`, so `void` is
+covered by the guard.
+
+## Design
+
+One file:
`core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java`.
+
+### 1. Cheaper package-name lookup, identical result
+
+```java
+public static String toPackageName(Class<?> clazz) {
+ if (clazz.isArray() || clazz.isPrimitive()) {
+ return "";
+ }
+ return clazz.getPackageName();
+}
+```
+
+The guard covers precisely the cases where `getPackage()` returns null, so the
+result is unchanged for every input while avoiding the classloader package-map
+lookup on the common path.
+
+### 2. Extract the walk over a package-name string
+
+Taking a `String` rather than a `Class` makes the prefix logic directly
testable
+with shapes no real `Class` can produce (`""`, `"a..b"`, `".a"`), which is what
+the differential test needs.
+
+```java
+static boolean isPackageBelongsToPackages(String packageName, Set<String>
first, Set<String> second) {
+ if (first.isEmpty() && second.isEmpty()) {
+ return false;
+ }
+ int idx = packageName.indexOf('.');
+ while (idx != -1) {
+ String prefix = packageName.substring(0, idx);
+ if (first.contains(prefix) || second.contains(prefix)) {
+ return true;
+ }
+ idx = packageName.indexOf('.', idx + 1);
+ }
+ return first.contains(packageName) || second.contains(packageName);
+}
+```
+
+Package-private: it is an implementation detail, exposed only far enough for
the
+test in the same package to reach it.
+
+Shortest-prefix-first ordering is preserved. Ordering does not affect the
result
+(the operation is a disjunction) but it short-circuits earliest on broad
+exclusions such as `java.io`, which are the common case.
+
+The `isEmpty()` short-circuit skips the walk — and therefore every substring
+allocation — when neither set is configured. That requires both sets to be
+empty, so it does not fire on the allowlist path, where
+`ALLOWLIST_REQUIRED_PACKAGES` is always non-empty (see §4), nor on the
+exclusion path under the shipped configuration, where
+`struts.excludedPackageNames` carries roughly thirty entries by default. It
+protects deployments that configure both sets empty.
+
+### 3. Both entry points delegate to it
+
+```java
+public static boolean isClassBelongsToPackages(Class<?> clazz, Set<String>
matchingPackages) {
+ return isClassBelongsToPackages(clazz, matchingPackages, emptySet());
+}
+
+static boolean isClassBelongsToPackages(Class<?> clazz, Set<String> first,
Set<String> second) {
+ return isPackageBelongsToPackages(toPackageName(clazz), first, second);
+}
+```
+
+One copy of the prefix logic, reached by every caller.
+
+The existing two-argument signature is retained unchanged. It is `public
static`
+on a public class, so it is nominally API even though a repository-wide search
+finds no caller outside `SecurityMemberAccess` itself.
+
+The new three-argument overload is package-private. It has exactly one caller
+(`isClassAllowlisted`) and its only test lives in the same package, so
+package-private reaches everything that needs it, and it matches the visibility
+of `isPackageBelongsToPackages` beside it. Publishing it would freeze it as
+`struts2-core` API until the next major release for no benefit — particularly
+unwelcome while WW-4759 is drawing the `struts2-api` boundary.
+
+That leaves a package-private overload sharing a name with a public method,
which
+invites a later contributor to widen it to `public` as a consistency tidy-up
+without realising that adds permanent API surface. Renaming it — along with the
+awkward `is...BelongsTo...` grammar, and narrowing the two public statics that
+have no callers outside this class — is tracked as WW-5678 against 8.0.0, since
+the narrowing is source-breaking.
+
+### 4. Single walk on the allowlist path
+
+```java
+protected boolean isClassAllowlisted(Class<?> clazz) {
+ return allowlistClasses.contains(clazz)
+ || ALLOWLIST_REQUIRED_CLASSES.contains(clazz)
+ || (providerAllowlist != null &&
providerAllowlist.getProviderAllowlist().contains(clazz))
+ || (threadAllowlist != null &&
threadAllowlist.getAllowlist().contains(clazz))
+ || isClassBelongsToPackages(clazz, ALLOWLIST_REQUIRED_PACKAGES,
allowlistPackageNames);
+}
+```
+
+Two walks over the same package name become one. `ALLOWLIST_REQUIRED_PACKAGES`
+is a non-empty constant, so the `isEmpty()` short-circuit does not fire here;
+the saving is the second walk.
+
+Semantically identical: probing prefix `p` against `A` then `B` at each step
+yields the same disjunction as walking all prefixes against `A` and then all
+prefixes against `B`.
+
+`isExcludedPackageNames` continues to call the two-argument form and is
+unchanged apart from inheriting the faster implementation.
+
+### 5. Import cleanup
+
+`java.util.List` and `java.util.stream.IntStream` become unused in
+`SecurityMemberAccess` once the stream pipeline is gone and must be removed.
+`java.util.Collections.emptySet` is already statically imported and is reused
by
+the two-argument delegate.
+
+## Data flow and error handling
+
+Unchanged. Same inputs, same boolean output, no new exceptions, no new state,
no
+caching, no new thread-safety considerations. `isPackageBelongsToPackages` is a
+pure function of its arguments.
+
+## Deliberately out of scope: array and primitive package semantics
+
+`Class.getPackageName()` resolves arrays to their element type's package
+(`java.io.File[]` → `"java.io"`, `String[]` → `"java.lang"`) and primitives to
+`"java.lang"`, whereas the current code yields `""` for both. Adopting those
+semantics was considered and rejected for this ticket because the change is
+**bidirectional**, not a pure hardening:
+
+- **Exclusion path tightens.** `java.io.File[]` currently escapes
+ `struts.excludedPackageNames` because its package is `""`; it would become
+ excluded.
+- **Allowlist path loosens.** An application that allowlists `com.app.actions`
+ does not today thereby allowlist `com.app.actions.MyThing[]`. It would.
Arrays
+ of allowlisted-package types become reachable where they previously required
+ an explicit `struts.allowlist.classes` entry.
+
+The allowlist is the primary OGNL defence in Struts 7.x and is enabled by
+default, so a change that makes it more permissive needs its own security
+reasoning, its own tests, and its own release note. The
`isArray()/isPrimitive()`
+guard in this spec preserves current behaviour exactly and captures the
+`getPackageName()` performance win for ordinary classes, which is all real
+traffic.
+
+A follow-up ticket should be filed to decide the array/primitive question on
its
+own merits. This spec does not prejudge it.
+
+## Testing
+
+The equivalence proof is the deliverable; the speedup is a consequence. Tests
go
+in `core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessTest.java`,
+which is **JUnit 4** (`org.junit.Test`, `org.junit.Before`, plain class,
AssertJ
+and Mockito) — not JUnit 5.
+
+1. **Differential test.** Add the current algorithm to the test class as a
+ private reference implementation, transcribed so that it takes the package
+ name directly rather than a `Class` — the body is otherwise verbatim:
+
+ ```java
+ private static boolean legacyPrefixMatch(String packageName, Set<String>
matchingPackages) {
+ List<String> packageParts = List.of(packageName.split("\\."));
+ return IntStream.range(0, packageParts.size())
+ .mapToObj(i -> String.join(".", packageParts.subList(0, i + 1)))
+ .anyMatch(matchingPackages::contains);
+ }
+ ```
+
+ Taking a `String` is what lets the matrix cover shapes no real `Class` can
+ produce; `toPackageName` equivalence is proven separately by test 4, so the
+ two halves of the original method are each covered.
+
+ Assert `legacyPrefixMatch(p, s)` equals
+ `isPackageBelongsToPackages(p, s, emptySet())` across a matrix of
+ package-name shapes × candidate sets. Shapes: `""`, `"java"`, `"a.b.c"`,
+ `"a..b"`, `".a"`, and realistic deep package names — but **not**
trailing-dot
+ inputs, which are the one known divergence and are unreachable through every
+ caller (see *Verified current semantics*). Sets: empty, exact match,
+ parent-package match, no match, and the `""` set.
+
+ This is the strongest available evidence that the rewrite is
+ behaviour-preserving, and it stays readable in review.
+
+2. **Package-boundary regression.** `org.apache.struts2x` must not match a set
+ containing `org.apache.struts2`. This is the classic prefix-matching bug the
+ rewrite could plausibly introduce and the single most important assertion in
+ the change.
+
+3. **Default-package edge.** A set containing `""` must still match
+ default-package classes. Currently untested, obscure, and easy to regress
+ silently.
+
+4. **`toPackageName` equivalence** over the eleven class shapes tabulated
above,
+ asserting the guard agrees with `getPackage()`-based resolution.
+
+5. **Two-set overload equivalence.** `isClassBelongsToPackages(c, A, B)` equals
+ `isClassBelongsToPackages(c, A) || isClassBelongsToPackages(c, B)` across
the
+ matrix, covering empty-`A`, empty-`B`, and both-empty.
+
+6. **Existing suite unchanged.** `SecurityMemberAccessTest` passes without
+ modification to any existing assertion, and the full `core` module suite is
+ green: `mvn test -DskipAssembly -pl core`.
+
+### Performance verification
+
+The project has no JMH harness and none is added for this change. The win is
+established by allocation count — a `String[]` plus N substrings, a list
+wrapper, a stream pipeline, N sublist views and N joined strings, reduced to N
+substrings — and confirmed with a throwaway benchmark that is **not**
committed.
+No timing assertion is added to the test suite, since wall-clock assertions are
+unreliable in CI.
+
+## Risks
+
+| Risk | Mitigation |
+|---|---|
+| Prefix matching without package-boundary awareness silently widens exclusion
or allowlist matching | Test 2 asserts `org.apache.struts2x` does not match
`org.apache.struts2` |
+| Default-package `""` edge regresses unnoticed | Test 3 pins it |
+| `toPackageName` guard misses a null-`getPackage()` case | Guard verified
against eleven class shapes; test 4 pins them |
+| Two-set overload changes evaluation semantics | Test 5 asserts equivalence
to the disjunction of two single-set calls |
+| Reviewer mistakes this for the fix WW-5667 asked for | Ticket descriptions
and PR body state that WW-5667's proposed fix addresses neither problem, and
that WW-5675 covers the dominant cost |
+
+## Out of scope for this spec
+
+- WW-5675 (config re-parsing / `Scope.PROTOTYPE`) — separate spec and PR.
+- Array and primitive package semantics — WW-5676, see above.
+- `isExcludedPackageNamePatterns`, which walks `excludedPackageNamePatterns`
with
+ a stream and calls `toPackageName` per pattern. It benefits from the cheaper
+ `toPackageName` for free, but its own stream overhead is not addressed here;
+ the pattern set is empty by default. Tracked as WW-5677.
+- `checkDefaultPackageAccess`, which still inspects `clazz.getPackage()`
directly —
+ two classloader package-map lookups per class, up to four per
`isAccessible()`
+ when `struts.disallowDefaultPackageAccess` is enabled. Its condition is
+ equivalent to `toPackageName(clazz).isEmpty()`, including for arrays and
+ primitives, so routing it through `toPackageName` would remove exactly the
+ lookup this spec eliminates fifteen lines away. Deliberately left out to keep
+ this change to one concern; tracked as WW-5677.
