This is an automated email from the ASF dual-hosted git repository.
lukaszlenart pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/struts.git
The following commit(s) were added to refs/heads/main by this push:
new 015c9c833 WW-5676 test(ognl): pin the array package-resolution
decision (#1858)
015c9c833 is described below
commit 015c9c8333bfd6c7bbaa50463fe93721bd65a8fd
Author: Lukasz Lenart <[email protected]>
AuthorDate: Sun Aug 23 18:28:25 2026 +0200
WW-5676 test(ognl): pin the array package-resolution decision (#1858)
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 <[email protected]>
---
.../apache/struts2/ognl/SecurityMemberAccess.java | 9 +-
.../ognl/SecurityMemberAccessArrayTargetTest.java | 151 +++++++++++++++++++++
2 files changed, 158 insertions(+), 2 deletions(-)
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 3e626e664..4d552d594 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 class SecurityMemberAccess implements MemberAccess {
// 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 000000000..e229145e8
--- /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.
+ * <p>
+ * 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:
+ * <ol>
+ * <li>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</li>
+ * <li>{@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.</li>
+ * </ol>
+ * {@code checkExclusionList} tests the declaring class before the package, so
it always denies at
+ * the first check and never reaches the package comparison.
+ * <p>
+ * 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<Class<?>> 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();
+ }
+ }
+ }
+}
