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 <noreply@anthropic.com>
This commit is contained in:
Lukasz Lenart
2026-08-03 15:03:27 +02:00
parent 2543dd8fc2
commit 25ac94927a
3 changed files with 32 additions and 12 deletions
@@ -413,7 +413,14 @@ public class SecurityMemberAccess implements MemberAccess {
* 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
* <p>
* The package name must not end in {@code '.'}. Such a name is probed one prefix more than by
* the implementation this replaced, which matches more broadly — tightening exclusion but
* <em>loosening</em> the allowlist. {@link Class#getPackageName()} cannot produce a trailing
* dot, so every current caller is safe; route any other string through here only after
* confirming the same.
*
* @param packageName the package name to test, empty for the default package, never ending in {@code '.'}
* @param first the first set of package names to match against
* @param second the second set of package names to match against
* @return {@code true} if the package or any parent package is in either set
@@ -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<String>, Set<String>)` from Task 3.
- Produces: `public static boolean SecurityMemberAccess.isClassBelongsToPackages(Class<?> clazz, Set<String> first, Set<String> second)`.
- Produces: `static boolean SecurityMemberAccess.isClassBelongsToPackages(Class<?> clazz, Set<String> first, Set<String> second)` — package-private, matching `isPackageBelongsToPackages` beside it. (The plan originally specified `public static`; it was narrowed during the final review, since the overload has one caller and its only test is in the same package.)
- [ ] **Step 1: Write the failing test**
@@ -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<String> first, Set<String> second) {
static boolean isClassBelongsToPackages(Class<?> clazz, Set<String> first, Set<String> 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.
@@ -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.