[jira] [Work logged] (WW-5676) Decide whether array and primitive types should resolve to their element/wrapper package in OGNL security checks

["ASF GitHub Bot (Jira)" <ji...@apache.org>]

     [ 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)