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 ffc7d5e89..badad3dee 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 @@ 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 + *

+ * 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 b005346fe..612316363 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 313894764..a9b609f9d 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.