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) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
