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 d25bbe3774..badad3dee3 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.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 @@ protected boolean isClassAllowlisted(Class clazz) { || 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 @@ protected boolean isPackageExcluded(Class clazz) { } 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 @@ protected boolean isExcludedPackageNames(Class clazz) { } public static boolean isClassBelongsToPackages(Class clazz, Set matchingPackages) { - List 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 first, Set 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. + * + *

+ * 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 + * loosening 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 first, Set 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 0000000000..413b0c6695 --- /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. + *

+ * 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 matchingPackages) { + List 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 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> 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> 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 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 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 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 first : CANDIDATE_SETS) { + for (Set 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 0000000000..6123163639 --- /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 (): `, 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=` +- 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 strengthens the spec's requirement that the existing suite pass unmodified. + +--- + +### 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)`, `SecurityMemberAccess.toPackageName(Class)`, `ConfigParseUtil.toPackageNamesSet(String)`. +- Produces: the constants `PACKAGE_NAMES` and `CANDIDATE_SETS`, and the helpers `legacyPrefixMatch(String, Set)` 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. + *

+ * 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 matchingPackages) { + List 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 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> 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> 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 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 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)`, `PACKAGE_NAMES`, `CANDIDATE_SETS` from Task 1. +- Produces: `static boolean SecurityMemberAccess.isPackageBelongsToPackages(String packageName, Set first, Set 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 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,Set)`. 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 matchingPackages) { + List 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 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 first, Set 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, Set)` from Task 3. +- Produces: `static boolean SecurityMemberAccess.isClassBelongsToPackages(Class clazz, Set first, Set 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 first : CANDIDATE_SETS) { + for (Set 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,Set,Set)`. 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 matchingPackages) { + return isPackageBelongsToPackages(toPackageName(clazz), matchingPackages, emptySet()); + } +``` + +with the delegating pair: + +```java + public static boolean isClassBelongsToPackages(Class clazz, Set 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 first, Set 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, Set)` 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)`, `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()` throws `Exception` (via `Class.forName`), so every test using it declares `throws Exception` — checked in Tasks 1 and 4. + +## 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 0000000000..7f26fae6cd --- /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 matchingPackages) { + List 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 first, Set 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 matchingPackages) { + return isClassBelongsToPackages(clazz, matchingPackages, emptySet()); +} + +static boolean isClassBelongsToPackages(Class clazz, Set first, Set 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 matchingPackages) { + List 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.