From 1076fa9ac52984b24db347ad727a771bc219b614 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sun, 23 Aug 2026 18:05:35 +0200 Subject: [PATCH] WW-5676 test(ognl): pin the array package-resolution decision WW-5676 asked whether toPackageName should resolve arrays to the element type's package instead of the empty package. It should not, and the premise the ticket was filed on turns out to be wrong. The ticket assumed clone() is reachable on an array target with the array type as its declaring class, leaving java.io.File[] free to slip past the java.io entry in struts.excludedPackageNames. Array clone() is a JVM-internal method and is absent from the reflection view: on Temurin 17, 21 and 25, getMethods() on an array class returns only java.lang.Object's methods, getDeclaredMethods() is empty and getMethod("clone") throws. java.lang.Object is in turn permanently excluded - it is the built-in default of excludedClasses, and useExcludedClasses accumulates onto that default rather than replacing it, so no configuration can drop it. Since checkExclusionList tests the declaring class before the package, an array target is always denied at the first check and the package comparison is never reached. Resolving arrays to the element package would therefore tighten nothing while genuinely loosening the allowlist, so the behaviour stays as is. This commit records the reasoning where it can rot loudly instead of quietly: SecurityMemberAccessArrayTargetTest pins both facts the decision rests on, and the comment on toPackageName no longer states the false premise. No behaviour change. Co-Authored-By: Claude Opus 5 --- .../struts2/ognl/SecurityMemberAccess.java | 9 +- .../SecurityMemberAccessArrayTargetTest.java | 151 ++++++++++++++++++ 2 files changed, 158 insertions(+), 2 deletions(-) create mode 100644 core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessArrayTargetTest.java 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 3e626e664d..4d552d5948 100644 --- a/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java +++ b/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java @@ -393,8 +393,13 @@ public static String toPackageName(Class clazz) { // call, whereas getPackageName() is computed once and cached on the Class. getPackage() // returns null for exactly arrays, primitives and void, so the guard reproduces the // previous result for every input. Note that void.class.isPrimitive() is true. - // Arrays deliberately keep the empty package here: getPackageName() would resolve them - // to the element type's package, which would loosen the allowlist. See WW-5674. + // Arrays deliberately keep the empty package. WW-5676 weighed resolving them to the element + // type's package and decided against it: that would tighten nothing, because the package + // check is unreachable for array targets -- every reflectively reachable member of an array + // class declares in java.lang.Object, which is permanently excluded -- while it would loosen + // the allowlist, implicitly allowlisting com.app.Thing[] for any application configuring + // struts.allowlist.packageNames=com.app. SecurityMemberAccessArrayTargetTest pins that + // reasoning; reopen WW-5676 if it ever stops holding. if (clazz.isArray() || clazz.isPrimitive()) { return ""; } diff --git a/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessArrayTargetTest.java b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessArrayTargetTest.java new file mode 100644 index 0000000000..e229145e8e --- /dev/null +++ b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessArrayTargetTest.java @@ -0,0 +1,151 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.struts2.ognl; + +import ognl.OgnlContext; +import org.apache.struts2.util.StrutsProxyService; +import org.junit.Before; +import org.junit.Test; + +import java.io.File; +import java.lang.reflect.Method; +import java.util.HashSet; +import java.util.List; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatExceptionOfType; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +/** + * Pins the reasoning behind WW-5676, which decided that {@link SecurityMemberAccess#toPackageName} + * must keep resolving arrays to the empty package rather than to the element type's package. + *

+ * The concern WW-5676 was raised to investigate was that an array of an excluded-package type, say + * {@code java.io.File[]}, resolves to the empty package and so slips past + * {@code struts.excludedPackageNames} even though {@code java.io} is excluded by default. That + * reads like a defensive gap, but the package check is unreachable for array targets, because of + * the two facts pinned below: + *

    + *
  1. every member reflectively reachable on an array class declares in {@code java.lang.Object} + * — array {@code clone()} is a JVM-internal method absent from the reflection view; and
    2. + *
  2. {@code java.lang.Object} is permanently excluded — it is the built-in default of + * {@code excludedClasses} and the setters only ever accumulate onto that default.
    3. + *
+ * {@code checkExclusionList} tests the declaring class before the package, so it always denies at + * the first check and never reaches the package comparison. + *

+ * Resolving arrays to the element package would therefore tighten nothing, while genuinely + * loosening the allowlist: {@code struts.allowlist.packageNames=com.app} would begin to allowlist + * {@code com.app.Thing[]} implicitly, which today requires an explicit + * {@code struts.allowlist.classes} entry. These tests fail loudly if either fact stops holding, + * because that is what would turn the decision around. + */ +public class SecurityMemberAccessArrayTargetTest { + + private static final List> ARRAY_SHAPES = List.of( + String[].class, + File[].class, + int[].class, + Object[][].class, + SecurityMemberAccess[].class); + + private OgnlContext context; + private SecurityMemberAccess sma; + + @Before + public void setUp() { + context = ognl.Ognl.createDefaultContext(null); + ProviderAllowlist providerAllowlist = mock(ProviderAllowlist.class); + ThreadAllowlist threadAllowlist = mock(ThreadAllowlist.class); + when(providerAllowlist.getProviderAllowlist()).thenReturn(new HashSet<>()); + when(threadAllowlist.getAllowlist()).thenReturn(new HashSet<>()); + sma = new SecurityMemberAccess(providerAllowlist, threadAllowlist); + sma.setProxyService(new StrutsProxyService(new StrutsProxyCacheFactory<>("1000", "basic"))); + } + + /** + * Fact one. If a future JDK exposes further members on array classes, the unreachability + * argument breaks and WW-5676 has to be reopened. + */ + @Test + public void everyReflectiveMemberOfAnArrayClassDeclaresInObject() { + for (Class arrayClass : ARRAY_SHAPES) { + assertThat(arrayClass.getMethods()) + .as("public methods of %s", arrayClass.getName()) + .isNotEmpty() + .allSatisfy(method -> assertThat(method.getDeclaringClass()).isEqualTo(Object.class)); + assertThat(arrayClass.getDeclaredMethods()) + .as("declared methods of %s", arrayClass.getName()) + .isEmpty(); + assertThat(arrayClass.getFields()) + .as("public fields of %s, including the synthetic length", arrayClass.getName()) + .isEmpty(); + } + } + + /** + * Fact one, continued. Array {@code clone()} is a JVM-internal method: the JLS gives array + * types a public {@code clone()}, but it is not reflectively discoverable, so it can never + * reach {@code checkExclusionList} with the array type as its declaring class. + */ + @Test + public void arrayCloneIsNotReflectivelyReachable() { + for (Class arrayClass : ARRAY_SHAPES) { + assertThatExceptionOfType(NoSuchMethodException.class) + .as("clone() of %s", arrayClass.getName()) + .isThrownBy(() -> arrayClass.getMethod("clone")); + } + } + + /** + * Fact two. {@code useExcludedClasses} folds into the existing set rather than replacing it, + * so no configuration can drop the built-in {@code java.lang.Object} entry. + */ + @Test + public void objectStaysExcludedWhateverIsConfigured() { + assertThat(sma.isClassExcluded(Object.class)) + .as("java.lang.Object is excluded by default") + .isTrue(); + + sma.useExcludedClasses("java.lang.Class,java.lang.Runtime"); + + assertThat(sma.isClassExcluded(Object.class)) + .as("java.lang.Object stays excluded after excludedClasses is configured without it") + .isTrue(); + } + + /** + * The payoff: no member of an array target is accessible, so the empty package name that + * {@code toPackageName} returns for arrays is never compared against + * {@code struts.excludedPackageNames} in the first place. + */ + @Test + public void noMemberOfAnArrayTargetIsAccessible() { + for (boolean allowlistEnabled : new boolean[]{true, false}) { + sma.useEnforceAllowlistEnabled(String.valueOf(allowlistEnabled)); + File[] target = {new File("/tmp")}; + for (Method member : target.getClass().getMethods()) { + assertThat(sma.isAccessible(context, target, member, member.getName())) + .as("allowlistEnabled=%s member=%s", allowlistEnabled, member) + .isFalse(); + } + } + } +}