From 7c2002397ddb5c1f053ec5a9fc204dc289b728dc Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 3 Aug 2026 12:39:25 +0200 Subject: [PATCH 01/10] 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 --- ...lassbelongstopackages-allocation-design.md | 355 ++++++++++++++++++ 1 file changed, 355 insertions(+) create mode 100644 docs/superpowers/specs/2026-08-03-WW-5674-isclassbelongstopackages-allocation-design.md 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..f156d08047 --- /dev/null +++ b/docs/superpowers/specs/2026-08-03-WW-5674-isclassbelongstopackages-allocation-design.md @@ -0,0 +1,355 @@ +# WW-5674 — Make `SecurityMemberAccess.isClassBelongsToPackages` allocation-free + +**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 + +- Remove the per-call allocation overhead on the OGNL member-access hot path. +- **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 | `""` | `""` | +| 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. `allowlistPackageNames` is empty by +default, so this is the common path for the allowlist check. + +### 3. Both public entry points delegate to it + +```java +public static boolean isClassBelongsToPackages(Class clazz, Set matchingPackages) { + return isClassBelongsToPackages(clazz, matchingPackages, emptySet()); +} + +public 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. 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 `public static` for consistency with the two +public statics beside it. It has a single caller today; making it +package-private instead would be a defensible alternative and is a trivial +follow-up if the extra surface is unwelcome. + +### 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 — follow-up ticket, 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. From 05a936803dc1bd3672c66a83f129f18f3ecadbde Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 3 Aug 2026 12:44:35 +0200 Subject: [PATCH 02/10] 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 --- ...674-isclassbelongstopackages-allocation.md | 625 ++++++++++++++++++ 1 file changed, 625 insertions(+) create mode 100644 docs/superpowers/plans/2026-08-03-WW-5674-isclassbelongstopackages-allocation.md 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..b005346fef --- /dev/null +++ b/docs/superpowers/plans/2026-08-03-WW-5674-isclassbelongstopackages-allocation.md @@ -0,0 +1,625 @@ +# WW-5674 — Allocation-free `isClassBelongsToPackages` 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: `public static boolean SecurityMemberAccess.isClassBelongsToPackages(Class clazz, Set first, Set second)`. + +- [ ] **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 + */ + public 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 + +Neither is filed; file before referencing either from code, per the project's no-placeholder-TODO rule. + +- **WW-5675** is already filed and covers the dominant cost (config re-parsing driven by the `Scope.PROTOTYPE` bean). WW-5674 alone will not move the 9% figure much. +- **Array/primitive package semantics** — whether `getPackageName()` semantics should be adopted for arrays. Tightens the exclusion list, loosens the allowlist. Needs its own security reasoning. +- **`ConfigParseUtil.validatePackageNames`** (`ConfigParseUtil.java:143`) evaluates `Pattern.compile("\\s")` once per package name rather than once overall. One-line fix, belongs with WW-5675. From 197071def50f5b7a5b6febcd63b4b2f8132146db Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 3 Aug 2026 12:54:27 +0200 Subject: [PATCH 03/10] 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. --- ...curityMemberAccessPackageMatchingTest.java | 178 ++++++++++++++++++ 1 file changed, 178 insertions(+) create mode 100644 core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessPackageMatchingTest.java 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..9f8af5556a --- /dev/null +++ b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessPackageMatchingTest.java @@ -0,0 +1,178 @@ +/* + * 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)); + } + } + } +} From a8f6b4c59d660c4d7b8f3b17d1539e3866c1905f Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 3 Aug 2026 12:58:54 +0200 Subject: [PATCH 04/10] 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. --- .../org/apache/struts2/ognl/SecurityMemberAccess.java | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 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 d25bbe3774..0e22e74ce6 100644 --- a/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java +++ b/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java @@ -372,10 +372,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) { From 8237a1fd8bf2338bc1ac36ff032058e73543ef7d Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 3 Aug 2026 13:05:06 +0200 Subject: [PATCH 05/10] 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. --- .../struts2/ognl/SecurityMemberAccess.java | 33 +++++++++++++++---- ...curityMemberAccessPackageMatchingTest.java | 20 +++++++++++ 2 files changed, 47 insertions(+), 6 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 0e22e74ce6..e41004a2ba 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; @@ -393,10 +391,33 @@ 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 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); } 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 index 9f8af5556a..04e2da96cb 100644 --- a/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessPackageMatchingTest.java +++ b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessPackageMatchingTest.java @@ -175,4 +175,24 @@ public void classEntryPointMatchesLegacyAcrossCandidateSets() throws Exception { } } } + + @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(); + } + } } From 34e5cf8b2a5c3add24211b013761a892fef238f4 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 3 Aug 2026 13:12:27 +0200 Subject: [PATCH 06/10] 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 --- .../struts2/ognl/SecurityMemberAccess.java | 19 ++++++++++++++++--- ...curityMemberAccessPackageMatchingTest.java | 14 ++++++++++++++ 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java b/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java index e41004a2ba..b18e568fc1 100644 --- a/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java +++ b/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java @@ -254,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); } /** @@ -391,7 +390,21 @@ protected boolean isExcludedPackageNames(Class clazz) { } public static boolean isClassBelongsToPackages(Class clazz, Set matchingPackages) { - return isPackageBelongsToPackages(toPackageName(clazz), matchingPackages, emptySet()); + 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 + */ + public static boolean isClassBelongsToPackages(Class clazz, Set first, Set second) { + return isPackageBelongsToPackages(toPackageName(clazz), first, second); } /** diff --git a/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessPackageMatchingTest.java b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessPackageMatchingTest.java index 04e2da96cb..361a1c43b4 100644 --- a/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessPackageMatchingTest.java +++ b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessPackageMatchingTest.java @@ -195,4 +195,18 @@ public void bothSetsEmptyShortCircuitsToFalse() { .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)); + } + } + } + } } From 866722bbb6b93d93c2c70ed3f39ead1268238fbd Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 3 Aug 2026 13:28:48 +0200 Subject: [PATCH 07/10] 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. --- .../struts2/ognl/SecurityMemberAccess.java | 2 +- ...ecurityMemberAccessPackageMatchingTest.java | 18 +++++++++++++++--- ...classbelongstopackages-allocation-design.md | 10 +++++++--- 3 files changed, 23 insertions(+), 7 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 b18e568fc1..ffc7d5e895 100644 --- a/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java +++ b/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java @@ -403,7 +403,7 @@ public static boolean isClassBelongsToPackages(Class clazz, Set match * @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 */ - public static boolean isClassBelongsToPackages(Class clazz, Set first, Set second) { + static boolean isClassBelongsToPackages(Class clazz, Set first, Set second) { return isPackageBelongsToPackages(toPackageName(clazz), first, second); } diff --git a/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessPackageMatchingTest.java b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessPackageMatchingTest.java index 361a1c43b4..413b0c6695 100644 --- a/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessPackageMatchingTest.java +++ b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessPackageMatchingTest.java @@ -89,6 +89,7 @@ private static String legacyToPackageName(Class clazz) { 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")); @@ -116,14 +117,25 @@ private static List> classShapes() throws Exception { 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") + .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") + .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") + .as("a sub-package must match (legacy oracle)") .isTrue(); } 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 index f156d08047..5666a44e13 100644 --- 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 @@ -125,7 +125,7 @@ Verified across eleven class shapes: | `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 | `""` | `""` | +| 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 | `""` | `""` | @@ -183,8 +183,12 @@ Shortest-prefix-first ordering is preserved. Ordering does not affect the result 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. `allowlistPackageNames` is empty by -default, so this is the common path for the allowlist check. +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 public entry points delegate to it From 2543dd8fc21c1f0a5dcb1f415955f0133548728e Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 3 Aug 2026 13:45:40 +0200 Subject: [PATCH 08/10] 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 --- ...lassbelongstopackages-allocation-design.md | 20 ++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) 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 index 5666a44e13..313894764c 100644 --- 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 @@ -190,28 +190,30 @@ 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 public entry points delegate to it +### 3. Both entry points delegate to it ```java public static boolean isClassBelongsToPackages(Class clazz, Set matchingPackages) { return isClassBelongsToPackages(clazz, matchingPackages, emptySet()); } -public static boolean isClassBelongsToPackages(Class clazz, Set first, Set second) { +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. 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 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 `public static` for consistency with the two -public statics beside it. It has a single caller today; making it -package-private instead would be a defensible alternative and is a trivial -follow-up if the extra surface is unwelcome. +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. ### 4. Single walk on the allowlist path From 25ac94927a1dfde80e317d34c13daf984d0bf5d5 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 3 Aug 2026 15:03:27 +0200 Subject: [PATCH 09/10] 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 --- .../struts2/ognl/SecurityMemberAccess.java | 9 +++++++- ...674-isclassbelongstopackages-allocation.md | 14 ++++++------- ...lassbelongstopackages-allocation-design.md | 21 +++++++++++++++---- 3 files changed, 32 insertions(+), 12 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 ffc7d5e895..badad3dee3 100644 --- a/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java +++ b/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java @@ -413,7 +413,14 @@ static boolean isClassBelongsToPackages(Class clazz, Set first, Set + * 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 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 index b005346fef..6123163639 100644 --- a/docs/superpowers/plans/2026-08-03-WW-5674-isclassbelongstopackages-allocation.md +++ b/docs/superpowers/plans/2026-08-03-WW-5674-isclassbelongstopackages-allocation.md @@ -1,4 +1,4 @@ -# WW-5674 — Allocation-free `isClassBelongsToPackages` Implementation Plan +# 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. @@ -466,7 +466,7 @@ package name shapes and candidate sets." **Interfaces:** - Consumes: `isPackageBelongsToPackages(String, Set, Set)` from Task 3. -- Produces: `public static boolean SecurityMemberAccess.isClassBelongsToPackages(Class clazz, Set first, Set second)`. +- 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** @@ -521,7 +521,7 @@ with the delegating pair: * @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 */ - public static boolean isClassBelongsToPackages(Class clazz, Set first, Set second) { + static boolean isClassBelongsToPackages(Class clazz, Set first, Set second) { return isPackageBelongsToPackages(toPackageName(clazz), first, second); } ``` @@ -618,8 +618,8 @@ One spec deviation, flagged in *File Structure*: tests live in a new `SecurityMe ## Follow-ups not in scope -Neither is filed; file before referencing either from code, per the project's no-placeholder-TODO rule. +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** is already filed and covers the dominant cost (config re-parsing driven by the `Scope.PROTOTYPE` bean). WW-5674 alone will not move the 9% figure much. -- **Array/primitive package semantics** — whether `getPackageName()` semantics should be adopted for arrays. Tightens the exclusion list, loosens the allowlist. Needs its own security reasoning. -- **`ConfigParseUtil.validatePackageNames`** (`ConfigParseUtil.java:143`) evaluates `Pattern.compile("\\s")` once per package name rather than once overall. One-line fix, belongs with WW-5675. +- **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 index 313894764c..a9b609f9db 100644 --- 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 @@ -1,4 +1,9 @@ -# WW-5674 — Make `SecurityMemberAccess.isClassBelongsToPackages` allocation-free +# 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)) @@ -67,7 +72,8 @@ it on the `Class` object. ## Goals -- Remove the per-call allocation overhead on the OGNL member-access hot path. +- 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. @@ -354,8 +360,15 @@ unreliable in CI. ## Out of scope for this spec - WW-5675 (config re-parsing / `Scope.PROTOTYPE`) — separate spec and PR. -- Array and primitive package semantics — follow-up ticket, see above. +- 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. + 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. From 33627a9e43c39250048dc2dde8f109799b4adc65 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 3 Aug 2026 15:19:21 +0200 Subject: [PATCH 10/10] WW-5674 docs(ognl): note the naming cleanup deferred to WW-5678 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- ...3-WW-5674-isclassbelongstopackages-allocation-design.md | 7 +++++++ 1 file changed, 7 insertions(+) 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 index a9b609f9db..7f26fae6cd 100644 --- 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 @@ -221,6 +221,13 @@ 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