[
https://issues.apache.org/jira/browse/WW-5677?focusedWorklogId=1037283&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1037283
]
ASF GitHub Bot logged work on WW-5677:
--------------------------------------
Author: ASF GitHub Bot
Created on: 23/Aug/26 16:35
Start Date: 23/Aug/26 16:35
Worklog Time Spent: 10m
Work Description: lukaszlenart opened a new pull request, #1859:
URL: https://github.com/apache/struts/pull/1859
Fixes [WW-5677](https://issues.apache.org/jira/browse/WW-5677)
Sub-task of WW-5667. WW-5674 replaced `Class.getPackage()` with the cached
`Class.getPackageName()` inside `toPackageName`, but deliberately left two
neighbouring call sites alone as out of scope. Both sit on the same per-access
path, within twenty lines of it. This finishes the job. **No behaviour change.**
## 1. `checkDefaultPackageAccess` made four `getPackage()` calls
```diff
-if (memberClass.getPackage() == null ||
memberClass.getPackage().getName().isEmpty()) {
+if (toPackageName(memberClass).isEmpty()) {
```
…and the same for `targetClass`. `Class.getPackage()` resolves through the
defining classloader's package map on every call, and the old condition
evaluated it twice per class, for up to two classes per access.
The two forms agree for every class shape: `getPackage()` returns `null` for
arrays, primitives and `void`, and names the unnamed package with the empty
string — all of which `toPackageName` reports as empty.
## 2. `isExcludedPackageNamePatterns` recomputed the package name per pattern
```diff
-return excludedPackageNamePatterns.stream().anyMatch(pattern ->
pattern.matcher(toPackageName(clazz)).matches());
+String packageName = toPackageName(clazz);
+return excludedPackageNamePatterns.stream().anyMatch(pattern ->
pattern.matcher(packageName).matches());
```
`toPackageName` was evaluated inside the lambda, so it ran once per
configured pattern. Now once per call.
## Testing
This is the OGNL security gate, so per the ticket the equivalence is
**asserted rather than argued**:
- **`defaultPackageConditionMatchesLegacyAcrossClassShapes`** runs the
replaced condition — frozen verbatim as `legacyDefaultPackageCondition`, which
calls `getPackage()` directly and never delegates to production code — against
the new one over the existing `classShapes()` matrix: arrays, primitives,
`void`, a default-package class, a lambda and a JDK proxy.
- **`testDefaultPackageAccessPermitsNamedPackageClass`** — a named-package
class still passes the gate with `struts.disallowDefaultPackageAccess=true`.
- **`testDefaultPackageAccessBlocksArrayTarget`** — an array target, the
shape most likely to break the equivalence, stays blocked. The member declares
in `java.lang`, so only the target branch can block.
All three were mutation-checked rather than assumed non-vacuous:
| mutation | fails |
|---|---|
| arrays/primitives resolve to `java.lang` |
`defaultPackageConditionMatchesLegacyAcrossClassShapes`,
`testDefaultPackageAccessBlocksArrayTarget` (plus the two WW-5674 equivalence
tests) |
| member-class condition inverted |
`testDefaultPackageAccessPermitsNamedPackageClass` (plus the existing
`testDefaultPackageExclusionSetting`) |
`mvn test -DskipAssembly -pl core` — 3190 tests, 0 failures, 0 errors.
## Scope deliberately not taken
- The `LOG.warn` calls in `checkExclusionList` also call `getPackage()`, but
only on the deny path, so there is no hot-path value — and switching them would
change the log text from `package java.io` to `java.io`.
- Not threading a single package name through `isPackageExcluded` into both
helpers. That would remove the last redundant call, but
`isExcludedPackageNames`/`isExcludedPackageNamePatterns` are `protected` and
subclasses may override them, so changing their signatures is source-breaking.
WW-5678 owns that cleanup for 8.0.0. `toPackageName` is now a cached field read
plus a branch, so calling it twice is negligible.
## Impact
Neither path runs by default: `checkDefaultPackageAccess` only when
`struts.disallowDefaultPackageAccess` is enabled, and the pattern loop only
when `struts.excludedPackageNamePatterns` is configured — both pattern
constants are commented out in `struts-excluded-classes.xml`. This is a
consistency fix for deployments that do enable them, not where the WW-5667 9%
lives; that was WW-5675.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Issue Time Tracking
-------------------
Worklog Id: (was: 1037283)
Remaining Estimate: 0h
Time Spent: 10m
> Remove the remaining redundant getPackage() lookups on the OGNL member-access
> path
> ----------------------------------------------------------------------------------
>
> Key: WW-5677
> URL: https://issues.apache.org/jira/browse/WW-5677
> Project: Struts 2
> Issue Type: Sub-task
> Reporter: Lukasz Lenart
> Assignee: Lukasz Lenart
> Priority: Major
> Fix For: 7.4.0
>
> Time Spent: 10m
> Remaining Estimate: 0h
>
> Sub-task of WW-5667. Residual items found while reviewing WW-5674, which
> optimised {{isClassBelongsToPackages}} and {{toPackageName}} but deliberately
> left these two call sites alone as out of scope.
> Both sit on the same per-access hot path WW-5674 just optimised, within
> twenty lines of it, so leaving them is a visible inconsistency.
> h2. 1. {{checkDefaultPackageAccess}} makes four {{getPackage()}} calls
> {{SecurityMemberAccess.checkDefaultPackageAccess}} still calls
> {{clazz.getPackage()}} directly, twice per class and for up to two classes:
> {code:java}
> if (memberClass.getPackage() == null ||
> memberClass.getPackage().getName().isEmpty()) {
> {code}
> {{Class.getPackage()}} resolves through the defining classloader's package
> map on every call. WW-5674 removed exactly this lookup from {{toPackageName}}
> by switching to the cached {{Class.getPackageName()}}, so this method is now
> the only remaining user of the slow form.
> The condition is equivalent to {{toPackageName(memberClass).isEmpty()}} —
> including for arrays and primitives, where both forms treat the class as
> being in the default package and therefore block. Replacing it would also
> collapse the double {{getPackage()}} evaluation per class.
> Note that this method only runs when {{struts.disallowDefaultPackageAccess}}
> is enabled, so the impact is limited to deployments that turn it on.
> h2. 2. {{isExcludedPackageNamePatterns}} recomputes the package name per
> pattern
> {code:java}
> protected boolean isExcludedPackageNamePatterns(Class clazz) {
> return excludedPackageNamePatterns.stream().anyMatch(pattern ->
> pattern.matcher(toPackageName(clazz)).matches());
> }
> {code}
> {{toPackageName(clazz)}} is evaluated inside the lambda, so it runs once per
> pattern rather than once per call. Hoisting it out of the stream is a
> one-line change.
> The call is cheaper after WW-5674, but it is still N redundant calls.
> {{struts.excludedPackageNamePatterns}} is empty by default (both pattern
> constants are commented out in {{struts-excluded-classes.xml}}), so this only
> affects deployments that configure it.
> h2. Constraints
> This is the OGNL security gate. Both changes must be behaviour-preserving,
> and the equivalence of {{getPackage() == null ||
> getPackage().getName().isEmpty()}} with {{toPackageName(...).isEmpty()}} must
> be asserted by test rather than argued — in particular for arrays,
> primitives, {{void}} and default-package classes.
> {{SecurityMemberAccessPackageMatchingTest}} already provides the class-shape
> matrix and the frozen {{legacyToPackageName}} reference oracle to build on.
> h2. Related
> {{ConfigParseUtil.validatePackageNames}} evaluating
> {{Pattern.compile("\\s")}} once per package name rather than once overall is
> a per-instantiation cost, not a per-access one, and is tracked on WW-5675
> instead.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
