[
https://issues.apache.org/jira/browse/WW-5676?focusedWorklogId=1037278&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1037278
]
ASF GitHub Bot logged work on WW-5676:
--------------------------------------
Author: ASF GitHub Bot
Created on: 23/Aug/26 16:06
Start Date: 23/Aug/26 16:06
Worklog Time Spent: 10m
Work Description: lukaszlenart opened a new pull request, #1858:
URL: https://github.com/apache/struts/pull/1858
Fixes [WW-5676](https://issues.apache.org/jira/browse/WW-5676)
WW-5676 asked whether `SecurityMemberAccess.toPackageName` should resolve
arrays to the element type's package, and primitives/`void` to `java.lang`,
instead of the empty package. **The answer is no, and the premise the ticket
was filed on is wrong.** This PR changes no behaviour — it pins the reasoning
so the question is not reopened from the same false premise.
## The premise that failed
The ticket argued there was a defensive gap on the exclusion path:
> A `java.io.File[]` currently has package `""` and escapes
`struts.excludedPackageNames` entirely, even though `java.io` is excluded by
default.
and identified `clone()` as the way in:
> for an array target, `member.getDeclaringClass()` is usually
`java.lang.Object`, which is already in `struts.excludedClasses`; `clone()` is
the notable exception, as its declaring class is the array type itself.
Array `clone()` is a JVM-internal method. The JLS gives array types a public
`clone()`, but it is not in the reflection view. Verified on Temurin 17.0.14,
21.0.7 and 25.0.1, for `String[]`, `java.io.File[]`, `int[]` and `Object[][]`:
| probe | result |
|---|---|
| `getMethods()` | exactly the 7 public `java.lang.Object` methods |
| `getDeclaredMethods()` | `[]` |
| `getMethod("clone")` | `NoSuchMethodException` |
| `getFields()` / `getField("length")` | `[]` / `NoSuchFieldException` |
So **every member reflectively reachable on an array class declares in
`java.lang.Object`**.
## Why the package check is unreachable
`java.lang.Object` cannot be configured out of the exclusion list. It is the
built-in default of `SecurityMemberAccess.excludedClasses`, and
`useExcludedClasses` goes through `ConfigParseUtil.toNewClassesSet`, which
*accumulates* onto the existing set rather than replacing it.
`checkExclusionList` tests `isClassExcluded(member.getDeclaringClass())`
before `isPackageExcluded`, so an array target is denied at the first check
every time. An end-to-end check against a real `SecurityMemberAccess` with the
production `struts-excluded-classes.xml` values and `target = new
java.io.File[]{...}` denies all 9 reachable members, with the allowlist both
enabled and disabled.
## Why changing it would be a net loss
- **Exclusion path**: tightens nothing, because it is unreachable.
- **Allowlist path**: loosens genuinely.
`struts.allowlist.packageNames=com.app` would begin implicitly allowlisting
`com.app.Thing[]`, which today requires an explicit `struts.allowlist.classes`
entry. The allowlist is the primary OGNL defence in 7.x and is on by default.
- **Consistency**: keeping `""` also keeps `toPackageName` in agreement with
`checkDefaultPackageAccess`'s raw `getPackage() == null` reading, which is what
WW-5677 wants to unify.
Primitives and `void` are moot either way: `target.getClass()` is never
primitive and `member.getDeclaringClass()` is never primitive, so they cannot
appear at either call site.
## What this PR contains
- **`SecurityMemberAccessArrayTargetTest`** (new, 4 tests) pins both facts
the decision rests on, so either one breaking fails loudly rather than silently
invalidating the reasoning:
- `everyReflectiveMemberOfAnArrayClassDeclaresInObject`
- `arrayCloneIsNotReflectivelyReachable`
- `objectStaysExcludedWhateverIsConfigured`
- `noMemberOfAnArrayTargetIsAccessible` — the payoff, over allowlist on
and off
- **`SecurityMemberAccess.toPackageName`**: the comment no longer states the
false premise, and points at the test.
The two behavioural tests were mutation-checked: setting `excludedClasses`
to `emptySet()` fails both, `noMemberOfAnArrayTargetIsAccessible` specifically
at `allowlistEnabled=false`.
No production behaviour changes, no configuration changes, and no
migration-guide note is needed. The existing
`arraysAndPrimitivesResolveToTheEmptyPackage` and
`toPackageNameMatchesLegacyAcrossClassShapes` in
`SecurityMemberAccessPackageMatchingTest` continue to pin the behaviour itself
and are untouched.
## Testing
`mvn test -DskipAssembly -pl core` — 3185 tests, 0 failures, 0 errors.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Issue Time Tracking
-------------------
Worklog Id: (was: 1037278)
Remaining Estimate: 0h
Time Spent: 10m
> Decide whether array and primitive types should resolve to their
> element/wrapper package in OGNL security checks
> ----------------------------------------------------------------------------------------------------------------
>
> Key: WW-5676
> URL: https://issues.apache.org/jira/browse/WW-5676
> Project: Struts 2
> Issue Type: Improvement
> Reporter: Lukasz Lenart
> Priority: Major
> Fix For: 7.4.0
>
> Time Spent: 10m
> Remaining Estimate: 0h
>
> Split out of [WW-5674|https://issues.apache.org/jira/browse/WW-5674], where
> the question was deliberately deferred so that ticket could remain a pure
> no-behaviour-change optimisation.
> h2. Current behaviour
> {{SecurityMemberAccess.toPackageName(Class)}} returns the empty string for
> arrays, primitives and {{void}}:
> {code:java}
> public static String toPackageName(Class clazz) {
> if (clazz.isArray() || clazz.isPrimitive()) {
> return "";
> }
> return clazz.getPackageName();
> }
> {code}
> (The real signature is generic; generics are omitted here because the issue
> tracker mangles them.)
> The {{isArray() || isPrimitive()}} guard exists specifically to preserve
> this. It reproduces the pre-WW-5674 implementation, which used
> {{clazz.getPackage()}} — and {{getPackage()}} returns null for exactly those
> three categories.
> {{Class.getPackageName()}} resolves them differently: arrays resolve to the
> element type's package and primitives to {{java.lang}}. Verified on Temurin
> 17:
> || class || current {{toPackageName}} || {{getPackageName()}} ||
> | {{String[]}} | {{""}} | {{java.lang}} |
> | {{java.io.File[]}} | {{""}} | {{java.io}} |
> | {{int}}, {{int[]}}, {{void}} | {{""}} | {{java.lang}} |
> | {{com.app.MyThing[]}} | {{""}} | {{com.app}} |
> h2. Why this is not a simple hardening fix
> {{toPackageName}} feeds two checks that the change would push in _opposite_
> directions.
> _The exclusion path would tighten._ {{isExcludedPackageNames}} would start
> matching arrays. A {{java.io.File[]}} currently has package {{""}} and
> escapes {{struts.excludedPackageNames}} entirely, even though {{java.io}} is
> excluded by default. That looks like a genuine defensive gap.
> _The allowlist path would loosen._ {{isClassAllowlisted}} would start
> matching arrays too. An application that sets
> {{struts.allowlist.packageNames=com.app.actions}} does _not_ today thereby
> allowlist {{com.app.actions.MyThing[]}}, because the array's package is
> {{""}}; arrays must be listed explicitly in {{struts.allowlist.classes}}.
> After the change they would be allowlisted implicitly.
> The allowlist is the primary OGNL defence in Struts 7.x and is enabled by
> default ({{struts.allowlist.enable=true}}), so making it more permissive
> needs its own security reasoning rather than riding along with a performance
> change.
> h2. What needs deciding
> * Whether the exclusion-path gap (arrays of excluded-package types escaping
> exclusion) is exploitable in practice. Note that for an array target,
> {{member.getDeclaringClass()}} is usually {{java.lang.Object}}, which is
> already in {{struts.excludedClasses}}; {{clone()}} is the notable exception,
> as its declaring class is the array type itself.
> * Whether the allowlist loosening is acceptable, or whether the two paths
> should use different package semantics.
> * Whether {{void}} and primitives should be treated separately from arrays —
> they can never be an OGNL target in the same way.
> * Whether any change here needs a migration-guide note, since it can break
> applications that rely on the current behaviour in either direction.
> h2. Constraints
> Any change must keep {{checkDefaultPackageAccess}} consistent — it
> independently inspects {{getPackage()}} and would otherwise disagree with
> {{toPackageName}} about what "the default package" means. See WW-5677, which
> proposes routing that method through {{toPackageName}}.
> Requires equivalence tests in {{SecurityMemberAccessPackageMatchingTest}},
> which already pins the current array/primitive behaviour in
> {{arraysAndPrimitivesResolveToTheEmptyPackage}} and
> {{toPackageNameMatchesLegacyAcrossClassShapes}}. Those tests will need
> updating deliberately, not incidentally.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
