mirror of
https://github.com/apache/struts.git
synced 2026-08-07 23:57:03 +00:00
Compare commits
1 Commits
| Author | SHA1 | Date | |
|---|---|---|---|
 dependabot[bot]
|
dcacbccbe6 |
@@ -53,12 +53,12 @@ jobs:
|
||||
java-version: 17
|
||||
cache: 'maven'
|
||||
- name: Initialize CodeQL
|
||||
uses: github/codeql-action/init@v4.37.3
|
||||
uses: github/codeql-action/init@v4.37.4
|
||||
with:
|
||||
languages: ${{ matrix.language }}
|
||||
- name: Autobuild
|
||||
uses: github/codeql-action/autobuild@v4.37.3
|
||||
uses: github/codeql-action/autobuild@v4.37.4
|
||||
- name: Perform CodeQL Analysis
|
||||
uses: github/codeql-action/analyze@v4.37.3
|
||||
uses: github/codeql-action/analyze@v4.37.4
|
||||
with:
|
||||
category: "/language:${{matrix.language}}"
|
||||
|
||||
@@ -32,9 +32,11 @@ import java.lang.reflect.Constructor;
|
||||
import java.lang.reflect.Field;
|
||||
import java.lang.reflect.Member;
|
||||
import java.lang.reflect.Modifier;
|
||||
import java.util.List;
|
||||
import java.util.Set;
|
||||
import java.util.regex.Matcher;
|
||||
import java.util.regex.Pattern;
|
||||
import java.util.stream.IntStream;
|
||||
|
||||
import static java.text.MessageFormat.format;
|
||||
import static java.util.Collections.emptySet;
|
||||
@@ -254,7 +256,8 @@ public class SecurityMemberAccess implements MemberAccess {
|
||||
|| ALLOWLIST_REQUIRED_CLASSES.contains(clazz)
|
||||
|| (providerAllowlist != null && providerAllowlist.getProviderAllowlist().contains(clazz))
|
||||
|| (threadAllowlist != null && threadAllowlist.getAllowlist().contains(clazz))
|
||||
|| isClassBelongsToPackages(clazz, ALLOWLIST_REQUIRED_PACKAGES, allowlistPackageNames);
|
||||
|| isClassBelongsToPackages(clazz, ALLOWLIST_REQUIRED_PACKAGES)
|
||||
|| isClassBelongsToPackages(clazz, allowlistPackageNames);
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -369,16 +372,10 @@ public class SecurityMemberAccess implements MemberAccess {
|
||||
}
|
||||
|
||||
public static String toPackageName(Class<?> clazz) {
|
||||
// Class.getPackage() resolves through the defining classloader's package map on every
|
||||
// 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.
|
||||
if (clazz.isArray() || clazz.isPrimitive()) {
|
||||
if (clazz.getPackage() == null) {
|
||||
return "";
|
||||
}
|
||||
return clazz.getPackageName();
|
||||
return clazz.getPackage().getName();
|
||||
}
|
||||
|
||||
protected boolean isExcludedPackageNamePatterns(Class<?> clazz) {
|
||||
@@ -390,54 +387,10 @@ public class SecurityMemberAccess implements MemberAccess {
|
||||
}
|
||||
|
||||
public static boolean isClassBelongsToPackages(Class<?> clazz, Set<String> matchingPackages) {
|
||||
return isClassBelongsToPackages(clazz, matchingPackages, emptySet());
|
||||
}
|
||||
|
||||
/**
|
||||
* Tests the class's package against two sets in a single walk. Equivalent to calling
|
||||
* {@link #isClassBelongsToPackages(Class, Set)} once per set and OR-ing the results, but
|
||||
* walks the package name only once.
|
||||
*
|
||||
* @param clazz the class whose package is tested
|
||||
* @param first the first set of package names to match against
|
||||
* @param second the second set of package names to match against
|
||||
* @return {@code true} if the class's package or any parent package is in either set
|
||||
*/
|
||||
static boolean isClassBelongsToPackages(Class<?> clazz, Set<String> first, Set<String> second) {
|
||||
return isPackageBelongsToPackages(toPackageName(clazz), first, second);
|
||||
}
|
||||
|
||||
/**
|
||||
* Tests whether the given package name, or any of its parent packages, is present in either
|
||||
* set. Walks the name in place rather than building the full prefix list, since this runs on
|
||||
* the OGNL member-access path. Shortest prefix first, so broad entries such as {@code java.io}
|
||||
* short-circuit earliest.
|
||||
*
|
||||
* <p>
|
||||
* The package name must not end in {@code '.'}. Such a name is probed one prefix more than by
|
||||
* the implementation this replaced, which matches more broadly — tightening exclusion but
|
||||
* <em>loosening</em> the allowlist. {@link Class#getPackageName()} cannot produce a trailing
|
||||
* dot, so every current caller is safe; route any other string through here only after
|
||||
* confirming the same.
|
||||
*
|
||||
* @param packageName the package name to test, empty for the default package, never ending in {@code '.'}
|
||||
* @param first the first set of package names to match against
|
||||
* @param second the second set of package names to match against
|
||||
* @return {@code true} if the package or any parent package is in either set
|
||||
*/
|
||||
static boolean isPackageBelongsToPackages(String packageName, Set<String> first, Set<String> second) {
|
||||
if (first.isEmpty() && second.isEmpty()) {
|
||||
return false;
|
||||
}
|
||||
int idx = packageName.indexOf('.');
|
||||
while (idx != -1) {
|
||||
String prefix = packageName.substring(0, idx);
|
||||
if (first.contains(prefix) || second.contains(prefix)) {
|
||||
return true;
|
||||
}
|
||||
idx = packageName.indexOf('.', idx + 1);
|
||||
}
|
||||
return first.contains(packageName) || second.contains(packageName);
|
||||
List<String> packageParts = List.of(toPackageName(clazz).split("\\."));
|
||||
return IntStream.range(0, packageParts.size())
|
||||
.mapToObj(i -> String.join(".", packageParts.subList(0, i + 1)))
|
||||
.anyMatch(matchingPackages::contains);
|
||||
}
|
||||
|
||||
protected boolean isClassExcluded(Class<?> clazz) {
|
||||
|
||||
-224
@@ -1,224 +0,0 @@
|
||||
/*
|
||||
* 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 org.apache.struts2.util.ConfigParseUtil;
|
||||
import org.junit.Test;
|
||||
|
||||
import java.lang.reflect.Proxy;
|
||||
import java.util.List;
|
||||
import java.util.Map;
|
||||
import java.util.Set;
|
||||
import java.util.stream.IntStream;
|
||||
|
||||
import static java.util.Collections.emptySet;
|
||||
import static org.apache.struts2.ognl.SecurityMemberAccess.isClassBelongsToPackages;
|
||||
import static org.apache.struts2.ognl.SecurityMemberAccess.toPackageName;
|
||||
import static org.assertj.core.api.Assertions.assertThat;
|
||||
|
||||
/**
|
||||
* Characterisation and equivalence tests for the static package-matching helpers in
|
||||
* {@link SecurityMemberAccess}, covering WW-5674.
|
||||
* <p>
|
||||
* These helpers gate OGNL member access, so the rewrite in WW-5674 must be exactly
|
||||
* behaviour-preserving. That is proven here by running the replaced implementation
|
||||
* side by side with the new one over a matrix of inputs.
|
||||
*/
|
||||
public class SecurityMemberAccessPackageMatchingTest {
|
||||
|
||||
/**
|
||||
* The implementation replaced by WW-5674, retained verbatim apart from taking the package
|
||||
* name directly instead of a {@link Class}. Used as the reference oracle for the rewrite.
|
||||
*/
|
||||
private static boolean legacyPrefixMatch(String packageName, Set<String> matchingPackages) {
|
||||
List<String> packageParts = List.of(packageName.split("\\."));
|
||||
return IntStream.range(0, packageParts.size())
|
||||
.mapToObj(i -> String.join(".", packageParts.subList(0, i + 1)))
|
||||
.anyMatch(matchingPackages::contains);
|
||||
}
|
||||
|
||||
/**
|
||||
* The {@code toPackageName} implementation replaced by WW-5674, retained as the reference oracle.
|
||||
*/
|
||||
private static String legacyToPackageName(Class<?> clazz) {
|
||||
if (clazz.getPackage() == null) {
|
||||
return "";
|
||||
}
|
||||
return clazz.getPackage().getName();
|
||||
}
|
||||
|
||||
/**
|
||||
* Package-name shapes. Deliberately excludes trailing-dot inputs such as {@code "a.b."}:
|
||||
* {@code split} drops trailing empty segments where an index walk would not, and
|
||||
* {@code Class.getPackage().getName()} cannot produce a trailing dot, so the shape is
|
||||
* unreachable through every caller. See the spec's "Verified current semantics" section.
|
||||
*/
|
||||
private static final List<String> PACKAGE_NAMES = List.of(
|
||||
"",
|
||||
"java",
|
||||
"a.b.c",
|
||||
"a..b",
|
||||
".a",
|
||||
"org.apache.struts2",
|
||||
"org.apache.struts2.ognl",
|
||||
"org.apache.struts2x",
|
||||
"java.io",
|
||||
"java.io.tmp",
|
||||
"javax.servlet.http");
|
||||
|
||||
private static final List<Set<String>> CANDIDATE_SETS = List.of(
|
||||
emptySet(),
|
||||
Set.of(""),
|
||||
Set.of("java"),
|
||||
Set.of("java.io"),
|
||||
Set.of("org.apache.struts2"),
|
||||
Set.of("a"),
|
||||
Set.of("a."),
|
||||
Set.of("a.b"),
|
||||
Set.of("zzz.not.matching"),
|
||||
Set.of("java.io", "org.apache.struts2", "javax"));
|
||||
|
||||
private static List<Class<?>> classShapes() throws Exception {
|
||||
return List.of(
|
||||
String.class,
|
||||
Map.Entry.class,
|
||||
SecurityMemberAccess.class,
|
||||
Class.forName("PackagelessAction"),
|
||||
int.class,
|
||||
void.class,
|
||||
int[].class,
|
||||
String[].class,
|
||||
String[][].class,
|
||||
((Runnable) () -> {
|
||||
}).getClass(),
|
||||
Proxy.newProxyInstance(
|
||||
SecurityMemberAccessPackageMatchingTest.class.getClassLoader(),
|
||||
new Class<?>[]{Runnable.class},
|
||||
(proxy, method, args) -> null).getClass());
|
||||
}
|
||||
|
||||
@Test
|
||||
public void siblingPackageWithSharedCharacterPrefixDoesNotMatch() {
|
||||
Set<String> excluded = Set.of("org.apache.struts2");
|
||||
|
||||
assertThat(SecurityMemberAccess.isPackageBelongsToPackages("org.apache.struts2x", excluded, emptySet()))
|
||||
.as("a sibling package sharing a character prefix must not match (production)")
|
||||
.isFalse();
|
||||
assertThat(legacyPrefixMatch("org.apache.struts2x", excluded))
|
||||
.as("a sibling package sharing a character prefix must not match (legacy oracle)")
|
||||
.isFalse();
|
||||
|
||||
assertThat(SecurityMemberAccess.isPackageBelongsToPackages("org.apache.struts2", excluded, emptySet()))
|
||||
.as("an exact match must match (production)")
|
||||
.isTrue();
|
||||
assertThat(legacyPrefixMatch("org.apache.struts2", excluded))
|
||||
.as("an exact match must match (legacy oracle)")
|
||||
.isTrue();
|
||||
|
||||
assertThat(SecurityMemberAccess.isPackageBelongsToPackages("org.apache.struts2.ognl", excluded, emptySet()))
|
||||
.as("a sub-package must match (production)")
|
||||
.isTrue();
|
||||
assertThat(legacyPrefixMatch("org.apache.struts2.ognl", excluded))
|
||||
.as("a sub-package must match (legacy oracle)")
|
||||
.isTrue();
|
||||
}
|
||||
|
||||
@Test
|
||||
public void dotOnlyConfigurationYieldsEmptyStringPackageName() {
|
||||
assertThat(ConfigParseUtil.toPackageNamesSet("."))
|
||||
.as("struts.excludedPackageNames=\".\" strips to the empty string")
|
||||
.containsExactly("");
|
||||
}
|
||||
|
||||
@Test
|
||||
public void defaultPackageMatchesOnlyWhenEmptyStringConfigured() throws Exception {
|
||||
Class<?> packageless = Class.forName("PackagelessAction");
|
||||
|
||||
assertThat(toPackageName(packageless)).isEmpty();
|
||||
assertThat(isClassBelongsToPackages(packageless, Set.of("")))
|
||||
.as("a default-package class is matched by the empty-string entry")
|
||||
.isTrue();
|
||||
assertThat(isClassBelongsToPackages(packageless, Set.of("java")))
|
||||
.as("a default-package class is not matched by an unrelated entry")
|
||||
.isFalse();
|
||||
}
|
||||
|
||||
@Test
|
||||
public void toPackageNameMatchesLegacyAcrossClassShapes() throws Exception {
|
||||
for (Class<?> clazz : classShapes()) {
|
||||
assertThat(toPackageName(clazz))
|
||||
.as("toPackageName(%s)", clazz.getName())
|
||||
.isEqualTo(legacyToPackageName(clazz));
|
||||
}
|
||||
}
|
||||
|
||||
@Test
|
||||
public void arraysAndPrimitivesResolveToTheEmptyPackage() {
|
||||
assertThat(toPackageName(int.class)).isEmpty();
|
||||
assertThat(toPackageName(void.class)).isEmpty();
|
||||
assertThat(toPackageName(int[].class)).isEmpty();
|
||||
assertThat(toPackageName(String[].class)).isEmpty();
|
||||
assertThat(toPackageName(String[][].class)).isEmpty();
|
||||
}
|
||||
|
||||
@Test
|
||||
public void classEntryPointMatchesLegacyAcrossCandidateSets() throws Exception {
|
||||
for (Class<?> clazz : classShapes()) {
|
||||
for (Set<String> candidates : CANDIDATE_SETS) {
|
||||
assertThat(isClassBelongsToPackages(clazz, candidates))
|
||||
.as("clazz=[%s] candidates=%s", clazz.getName(), candidates)
|
||||
.isEqualTo(legacyPrefixMatch(legacyToPackageName(clazz), candidates));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@Test
|
||||
public void indexWalkMatchesLegacyAcrossPackageNameShapes() {
|
||||
for (String packageName : PACKAGE_NAMES) {
|
||||
for (Set<String> candidates : CANDIDATE_SETS) {
|
||||
assertThat(SecurityMemberAccess.isPackageBelongsToPackages(packageName, candidates, emptySet()))
|
||||
.as("packageName=[%s] candidates=%s", packageName, candidates)
|
||||
.isEqualTo(legacyPrefixMatch(packageName, candidates));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@Test
|
||||
public void bothSetsEmptyShortCircuitsToFalse() {
|
||||
for (String packageName : PACKAGE_NAMES) {
|
||||
assertThat(SecurityMemberAccess.isPackageBelongsToPackages(packageName, emptySet(), emptySet()))
|
||||
.as("packageName=[%s] with no configured packages", packageName)
|
||||
.isFalse();
|
||||
}
|
||||
}
|
||||
|
||||
@Test
|
||||
public void twoSetOverloadEqualsDisjunctionOfSingleSetCalls() throws Exception {
|
||||
for (Class<?> clazz : classShapes()) {
|
||||
for (Set<String> first : CANDIDATE_SETS) {
|
||||
for (Set<String> second : CANDIDATE_SETS) {
|
||||
assertThat(isClassBelongsToPackages(clazz, first, second))
|
||||
.as("clazz=[%s] first=%s second=%s", clazz.getName(), first, second)
|
||||
.isEqualTo(isClassBelongsToPackages(clazz, first)
|
||||
|| isClassBelongsToPackages(clazz, second));
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -1,625 +0,0 @@
|
||||
# WW-5674 — Reduce `isClassBelongsToPackages` Allocations Implementation Plan
|
||||
|
||||
> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
|
||||
|
||||
**Goal:** Remove the per-OGNL-access allocation overhead in `SecurityMemberAccess.isClassBelongsToPackages` and `toPackageName`, with zero change to allow/deny semantics proven by test.
|
||||
|
||||
**Architecture:** Replace a `split` + `IntStream` + `String.join` prefix construction with an index walk over the package-name string, extracted into a package-private pure function so it can be tested against shapes no real `Class` can produce. Swap `Class.getPackage().getName()` for the cached `Class.getPackageName()` behind an `isArray()/isPrimitive()` guard that reproduces the old result exactly. Collapse the allowlist path's two walks into one via a two-set overload.
|
||||
|
||||
**Tech Stack:** Java 17, Maven, JUnit 4, AssertJ, Mockito.
|
||||
|
||||
**Spec:** `docs/superpowers/specs/2026-08-03-WW-5674-isclassbelongstopackages-allocation-design.md`
|
||||
|
||||
**Ticket:** [WW-5674](https://issues.apache.org/jira/browse/WW-5674), sub-task of [WW-5667](https://issues.apache.org/jira/browse/WW-5667)
|
||||
|
||||
## Global Constraints
|
||||
|
||||
- **Java release target is 17** (`maven.compiler.release=17` in root `pom.xml`). `Class.getPackageName()` is Java 9+, so it is available.
|
||||
- **Core tests are JUnit 4, never JUnit 5.** Use `org.junit.Test` and `org.junit.Before`. An `@org.junit.jupiter.api.Test` added here silently never runs.
|
||||
- **AssertJ** (`org.assertj.core.api.Assertions.assertThat`) and **Mockito** are already on the core test classpath.
|
||||
- **Zero behavior change is the acceptance criterion.** Any test that changes an existing assertion means the change is wrong, not the test.
|
||||
- **Do not change array or primitive package semantics.** `toPackageName` must keep returning `""` for arrays, primitives and `void`. This is deliberate — see the spec's *Deliberately out of scope* section. Adopting `getPackageName()` semantics there loosens the allowlist.
|
||||
- **Branch is `WW-5674-isclassbelongstopackages-allocation`, already checked out.** Never commit to `main`.
|
||||
- **Commit message format:** `WW-5674 <type>(<scope>): <description>`, e.g. `WW-5674 test(ognl): ...`. Ticket prefix is mandatory.
|
||||
- **No JMH, no timing assertions.** The project has no benchmark harness and none is added. Wall-clock assertions are unreliable in CI.
|
||||
- Single test run: `mvn test -DskipAssembly -pl core -Dtest=<ClassName>`
|
||||
- Full core suite: `mvn test -DskipAssembly -pl core`
|
||||
|
||||
## File Structure
|
||||
|
||||
| File | Responsibility |
|
||||
|---|---|
|
||||
| `core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java` | Modified. Lines 374–379 (`toPackageName`), 389–394 (`isClassBelongsToPackages`), 254–261 (`isClassAllowlisted`), imports at 35 and 39. |
|
||||
| `core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessPackageMatchingTest.java` | Created. All new tests for the static package-matching utilities. |
|
||||
| `core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessTest.java` | Untouched. Must stay green without edits. |
|
||||
| `core/src/test/java/PackagelessAction.java` | Existing default-package class, reused via `Class.forName("PackagelessAction")`. Do not modify. |
|
||||
|
||||
**Note on test file placement.** The spec named `SecurityMemberAccessTest.java` as the test home. That file is 1136 lines, and the new tests are a self-contained block of pure-static table-driven checks that need `List` and `IntStream` imports the existing file does not have. They go in a new focused test class in the same package instead — `isPackageBelongsToPackages` is package-private, so the test must live in `org.apache.struts2.ognl`. This is a deliberate, flagged deviation and it strengthens the spec's requirement that the existing suite pass unmodified.
|
||||
|
||||
---
|
||||
|
||||
### Task 1: Characterization tests that lock in current behavior
|
||||
|
||||
Create the test file and pin the *existing* behavior before touching any production code. These tests exercise only the current public API (`isClassBelongsToPackages(Class, Set)` and `toPackageName(Class)`), so they **pass immediately against unmodified code**.
|
||||
|
||||
This is intentional and is the gate for the whole plan: if any assertion here fails, the semantic claims in the spec are wrong and you must stop and re-derive them rather than "fixing" the test.
|
||||
|
||||
**Files:**
|
||||
- Create: `core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessPackageMatchingTest.java`
|
||||
|
||||
**Interfaces:**
|
||||
- Consumes: existing `SecurityMemberAccess.isClassBelongsToPackages(Class<?>, Set<String>)`, `SecurityMemberAccess.toPackageName(Class<?>)`, `ConfigParseUtil.toPackageNamesSet(String)`.
|
||||
- Produces: the constants `PACKAGE_NAMES` and `CANDIDATE_SETS`, and the helpers `legacyPrefixMatch(String, Set<String>)` and `legacyToPackageName(Class<?>)`, all reused by Tasks 3 and 4.
|
||||
|
||||
- [ ] **Step 1: Create the test file**
|
||||
|
||||
```java
|
||||
/*
|
||||
* 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 org.apache.struts2.util.ConfigParseUtil;
|
||||
import org.junit.Test;
|
||||
|
||||
import java.lang.reflect.Proxy;
|
||||
import java.util.List;
|
||||
import java.util.Map;
|
||||
import java.util.Set;
|
||||
import java.util.stream.IntStream;
|
||||
|
||||
import static java.util.Collections.emptySet;
|
||||
import static org.apache.struts2.ognl.SecurityMemberAccess.isClassBelongsToPackages;
|
||||
import static org.apache.struts2.ognl.SecurityMemberAccess.toPackageName;
|
||||
import static org.assertj.core.api.Assertions.assertThat;
|
||||
|
||||
/**
|
||||
* Characterisation and equivalence tests for the static package-matching helpers in
|
||||
* {@link SecurityMemberAccess}, covering WW-5674.
|
||||
* <p>
|
||||
* These helpers gate OGNL member access, so the rewrite in WW-5674 must be exactly
|
||||
* behaviour-preserving. That is proven here by running the replaced implementation
|
||||
* side by side with the new one over a matrix of inputs.
|
||||
*/
|
||||
public class SecurityMemberAccessPackageMatchingTest {
|
||||
|
||||
/**
|
||||
* The implementation replaced by WW-5674, retained verbatim apart from taking the package
|
||||
* name directly instead of a {@link Class}. Used as the reference oracle for the rewrite.
|
||||
*/
|
||||
private static boolean legacyPrefixMatch(String packageName, Set<String> matchingPackages) {
|
||||
List<String> packageParts = List.of(packageName.split("\\."));
|
||||
return IntStream.range(0, packageParts.size())
|
||||
.mapToObj(i -> String.join(".", packageParts.subList(0, i + 1)))
|
||||
.anyMatch(matchingPackages::contains);
|
||||
}
|
||||
|
||||
/**
|
||||
* The {@code toPackageName} implementation replaced by WW-5674, retained as the reference oracle.
|
||||
*/
|
||||
private static String legacyToPackageName(Class<?> clazz) {
|
||||
if (clazz.getPackage() == null) {
|
||||
return "";
|
||||
}
|
||||
return clazz.getPackage().getName();
|
||||
}
|
||||
|
||||
/**
|
||||
* Package-name shapes. Deliberately excludes trailing-dot inputs such as {@code "a.b."}:
|
||||
* {@code split} drops trailing empty segments where an index walk would not, and
|
||||
* {@code Class.getPackage().getName()} cannot produce a trailing dot, so the shape is
|
||||
* unreachable through every caller. See the spec's "Verified current semantics" section.
|
||||
*/
|
||||
private static final List<String> PACKAGE_NAMES = List.of(
|
||||
"",
|
||||
"java",
|
||||
"a.b.c",
|
||||
"a..b",
|
||||
".a",
|
||||
"org.apache.struts2",
|
||||
"org.apache.struts2.ognl",
|
||||
"org.apache.struts2x",
|
||||
"java.io",
|
||||
"java.io.tmp",
|
||||
"javax.servlet.http");
|
||||
|
||||
private static final List<Set<String>> CANDIDATE_SETS = List.of(
|
||||
emptySet(),
|
||||
Set.of(""),
|
||||
Set.of("java"),
|
||||
Set.of("java.io"),
|
||||
Set.of("org.apache.struts2"),
|
||||
Set.of("a"),
|
||||
Set.of("a.b"),
|
||||
Set.of("zzz.not.matching"),
|
||||
Set.of("java.io", "org.apache.struts2", "javax"));
|
||||
|
||||
private static List<Class<?>> classShapes() throws Exception {
|
||||
return List.of(
|
||||
String.class,
|
||||
Map.Entry.class,
|
||||
SecurityMemberAccess.class,
|
||||
Class.forName("PackagelessAction"),
|
||||
int.class,
|
||||
void.class,
|
||||
int[].class,
|
||||
String[].class,
|
||||
String[][].class,
|
||||
((Runnable) () -> {
|
||||
}).getClass(),
|
||||
Proxy.newProxyInstance(
|
||||
SecurityMemberAccessPackageMatchingTest.class.getClassLoader(),
|
||||
new Class<?>[]{Runnable.class},
|
||||
(proxy, method, args) -> null).getClass());
|
||||
}
|
||||
|
||||
@Test
|
||||
public void siblingPackageWithSharedCharacterPrefixDoesNotMatch() {
|
||||
Set<String> excluded = Set.of("org.apache.struts2");
|
||||
|
||||
assertThat(legacyPrefixMatch("org.apache.struts2x", excluded))
|
||||
.as("a sibling package sharing a character prefix must not match")
|
||||
.isFalse();
|
||||
assertThat(legacyPrefixMatch("org.apache.struts2", excluded))
|
||||
.as("an exact match must match")
|
||||
.isTrue();
|
||||
assertThat(legacyPrefixMatch("org.apache.struts2.ognl", excluded))
|
||||
.as("a sub-package must match")
|
||||
.isTrue();
|
||||
}
|
||||
|
||||
@Test
|
||||
public void dotOnlyConfigurationYieldsEmptyStringPackageName() {
|
||||
assertThat(ConfigParseUtil.toPackageNamesSet("."))
|
||||
.as("struts.excludedPackageNames=\".\" strips to the empty string")
|
||||
.containsExactly("");
|
||||
}
|
||||
|
||||
@Test
|
||||
public void defaultPackageMatchesOnlyWhenEmptyStringConfigured() throws Exception {
|
||||
Class<?> packageless = Class.forName("PackagelessAction");
|
||||
|
||||
assertThat(toPackageName(packageless)).isEmpty();
|
||||
assertThat(isClassBelongsToPackages(packageless, Set.of("")))
|
||||
.as("a default-package class is matched by the empty-string entry")
|
||||
.isTrue();
|
||||
assertThat(isClassBelongsToPackages(packageless, Set.of("java")))
|
||||
.as("a default-package class is not matched by an unrelated entry")
|
||||
.isFalse();
|
||||
}
|
||||
|
||||
@Test
|
||||
public void toPackageNameMatchesLegacyAcrossClassShapes() throws Exception {
|
||||
for (Class<?> clazz : classShapes()) {
|
||||
assertThat(toPackageName(clazz))
|
||||
.as("toPackageName(%s)", clazz.getName())
|
||||
.isEqualTo(legacyToPackageName(clazz));
|
||||
}
|
||||
}
|
||||
|
||||
@Test
|
||||
public void arraysAndPrimitivesResolveToTheEmptyPackage() {
|
||||
assertThat(toPackageName(int.class)).isEmpty();
|
||||
assertThat(toPackageName(void.class)).isEmpty();
|
||||
assertThat(toPackageName(int[].class)).isEmpty();
|
||||
assertThat(toPackageName(String[].class)).isEmpty();
|
||||
assertThat(toPackageName(String[][].class)).isEmpty();
|
||||
}
|
||||
|
||||
@Test
|
||||
public void classEntryPointMatchesLegacyAcrossCandidateSets() throws Exception {
|
||||
for (Class<?> clazz : classShapes()) {
|
||||
for (Set<String> candidates : CANDIDATE_SETS) {
|
||||
assertThat(isClassBelongsToPackages(clazz, candidates))
|
||||
.as("clazz=[%s] candidates=%s", clazz.getName(), candidates)
|
||||
.isEqualTo(legacyPrefixMatch(legacyToPackageName(clazz), candidates));
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Run the tests — they must all PASS against unmodified production code**
|
||||
|
||||
Run: `mvn test -DskipAssembly -pl core -Dtest=SecurityMemberAccessPackageMatchingTest`
|
||||
|
||||
Expected: BUILD SUCCESS, 6 tests run, 0 failures.
|
||||
|
||||
This is a characterization suite, so passing immediately is correct. **If anything fails, stop.** It means the spec's description of current behavior is wrong — re-derive the semantics before changing production code. Do not edit the assertions to make them green.
|
||||
|
||||
- [ ] **Step 3: Commit**
|
||||
|
||||
```bash
|
||||
git add core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessPackageMatchingTest.java
|
||||
git commit -m "WW-5674 test(ognl): characterise SecurityMemberAccess package matching
|
||||
|
||||
Pins the current behaviour of isClassBelongsToPackages and toPackageName
|
||||
before the WW-5674 rewrite, including the default-package empty-string edge
|
||||
reachable via struts.excludedPackageNames=\".\" and the package-boundary case
|
||||
where org.apache.struts2x must not match org.apache.struts2."
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Task 2: Make `toPackageName` use the cached `getPackageName()`
|
||||
|
||||
Swap the classloader package-map lookup for the value cached on the `Class`, behind a guard covering exactly the cases where `getPackage()` returns null.
|
||||
|
||||
**Files:**
|
||||
- Modify: `core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java:374-379`
|
||||
- Test: `core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessPackageMatchingTest.java` (no changes — Task 1's `toPackageNameMatchesLegacyAcrossClassShapes` and `arraysAndPrimitivesResolveToTheEmptyPackage` are the gate)
|
||||
|
||||
**Interfaces:**
|
||||
- Consumes: `legacyToPackageName(Class<?>)` and `classShapes()` from Task 1.
|
||||
- Produces: `SecurityMemberAccess.toPackageName(Class<?>)` — unchanged signature `public static String`, unchanged results.
|
||||
|
||||
- [ ] **Step 1: Replace the method body**
|
||||
|
||||
Replace lines 374–379 of `SecurityMemberAccess.java`:
|
||||
|
||||
```java
|
||||
public static String toPackageName(Class<?> clazz) {
|
||||
if (clazz.getPackage() == null) {
|
||||
return "";
|
||||
}
|
||||
return clazz.getPackage().getName();
|
||||
}
|
||||
```
|
||||
|
||||
with:
|
||||
|
||||
```java
|
||||
public static String toPackageName(Class<?> clazz) {
|
||||
// Class.getPackage() resolves through the defining classloader's package map on every
|
||||
// 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.
|
||||
if (clazz.isArray() || clazz.isPrimitive()) {
|
||||
return "";
|
||||
}
|
||||
return clazz.getPackageName();
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Run the tests to verify behavior is unchanged**
|
||||
|
||||
Run: `mvn test -DskipAssembly -pl core -Dtest=SecurityMemberAccessPackageMatchingTest`
|
||||
|
||||
Expected: PASS, 6 tests, 0 failures. `toPackageNameMatchesLegacyAcrossClassShapes` compares the new implementation against the retained legacy oracle across all eleven class shapes, so a regression here fails loudly.
|
||||
|
||||
- [ ] **Step 3: Run the existing SecurityMemberAccess suite**
|
||||
|
||||
Run: `mvn test -DskipAssembly -pl core -Dtest=SecurityMemberAccessTest`
|
||||
|
||||
Expected: PASS, 0 failures, with no edits to that file.
|
||||
|
||||
- [ ] **Step 4: Commit**
|
||||
|
||||
```bash
|
||||
git add core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java
|
||||
git commit -m "WW-5674 perf(ognl): resolve package names via cached Class.getPackageName
|
||||
|
||||
getPackage() performs a classloader package-map lookup on every call; the name
|
||||
returned by getPackageName() is computed once and cached on the Class. The
|
||||
isArray()/isPrimitive() guard covers exactly the inputs for which getPackage()
|
||||
returns null, so results are unchanged for every class shape."
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Task 3: Replace the prefix construction with an index walk
|
||||
|
||||
Extract the walk into a package-private pure function over the package-name string, and delegate the existing public method to it.
|
||||
|
||||
**Files:**
|
||||
- Modify: `core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java:389-394` (method body), `:35` and `:39` (imports)
|
||||
- Modify: `core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessPackageMatchingTest.java` (add one test)
|
||||
|
||||
**Interfaces:**
|
||||
- Consumes: `toPackageName(Class<?>)` from Task 2; `legacyPrefixMatch(String, Set<String>)`, `PACKAGE_NAMES`, `CANDIDATE_SETS` from Task 1.
|
||||
- Produces: `static boolean SecurityMemberAccess.isPackageBelongsToPackages(String packageName, Set<String> first, Set<String> second)` — package-private, pure, no allocation beyond one substring per package level. Consumed by Task 4.
|
||||
|
||||
- [ ] **Step 1: Write the failing test**
|
||||
|
||||
Add to `SecurityMemberAccessPackageMatchingTest`:
|
||||
|
||||
```java
|
||||
@Test
|
||||
public void indexWalkMatchesLegacyAcrossPackageNameShapes() {
|
||||
for (String packageName : PACKAGE_NAMES) {
|
||||
for (Set<String> candidates : CANDIDATE_SETS) {
|
||||
assertThat(SecurityMemberAccess.isPackageBelongsToPackages(packageName, candidates, emptySet()))
|
||||
.as("packageName=[%s] candidates=%s", packageName, candidates)
|
||||
.isEqualTo(legacyPrefixMatch(packageName, candidates));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@Test
|
||||
public void bothSetsEmptyShortCircuitsToFalse() {
|
||||
for (String packageName : PACKAGE_NAMES) {
|
||||
assertThat(SecurityMemberAccess.isPackageBelongsToPackages(packageName, emptySet(), emptySet()))
|
||||
.as("packageName=[%s] with no configured packages", packageName)
|
||||
.isFalse();
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Run test to verify it fails**
|
||||
|
||||
Run: `mvn test -DskipAssembly -pl core -Dtest=SecurityMemberAccessPackageMatchingTest`
|
||||
|
||||
Expected: COMPILATION FAILURE — `cannot find symbol: method isPackageBelongsToPackages(String,Set<String>,Set<String>)`. That is the red state for this task.
|
||||
|
||||
- [ ] **Step 3: Write the implementation**
|
||||
|
||||
Replace lines 389–394 of `SecurityMemberAccess.java`:
|
||||
|
||||
```java
|
||||
public static boolean isClassBelongsToPackages(Class<?> clazz, Set<String> matchingPackages) {
|
||||
List<String> packageParts = List.of(toPackageName(clazz).split("\\."));
|
||||
return IntStream.range(0, packageParts.size())
|
||||
.mapToObj(i -> String.join(".", packageParts.subList(0, i + 1)))
|
||||
.anyMatch(matchingPackages::contains);
|
||||
}
|
||||
```
|
||||
|
||||
with:
|
||||
|
||||
```java
|
||||
public static boolean isClassBelongsToPackages(Class<?> clazz, Set<String> matchingPackages) {
|
||||
return isPackageBelongsToPackages(toPackageName(clazz), matchingPackages, emptySet());
|
||||
}
|
||||
|
||||
/**
|
||||
* Tests whether the given package name, or any of its parent packages, is present in either
|
||||
* set. Walks the name in place rather than building the full prefix list, since this runs on
|
||||
* the OGNL member-access path. Shortest prefix first, so broad entries such as {@code java.io}
|
||||
* short-circuit earliest.
|
||||
*
|
||||
* @param packageName the package name to test, empty for the default package
|
||||
* @param first the first set of package names to match against
|
||||
* @param second the second set of package names to match against
|
||||
* @return {@code true} if the package or any parent package is in either set
|
||||
*/
|
||||
static boolean isPackageBelongsToPackages(String packageName, Set<String> first, Set<String> second) {
|
||||
if (first.isEmpty() && second.isEmpty()) {
|
||||
return false;
|
||||
}
|
||||
int idx = packageName.indexOf('.');
|
||||
while (idx != -1) {
|
||||
String prefix = packageName.substring(0, idx);
|
||||
if (first.contains(prefix) || second.contains(prefix)) {
|
||||
return true;
|
||||
}
|
||||
idx = packageName.indexOf('.', idx + 1);
|
||||
}
|
||||
return first.contains(packageName) || second.contains(packageName);
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 4: Remove the now-unused imports**
|
||||
|
||||
Delete line 35 (`import java.util.List;`) and line 39 (`import java.util.stream.IntStream;`) from `SecurityMemberAccess.java`. Both are used only by the code just replaced — verify with:
|
||||
|
||||
```bash
|
||||
grep -n '\bList\b\|\bIntStream\b' core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java
|
||||
```
|
||||
|
||||
Expected after deletion: no output. `emptySet` is already statically imported at line 42 and is now used by the delegate; leave it.
|
||||
|
||||
- [ ] **Step 5: Run tests to verify they pass**
|
||||
|
||||
Run: `mvn test -DskipAssembly -pl core -Dtest=SecurityMemberAccessPackageMatchingTest`
|
||||
|
||||
Expected: PASS, 8 tests, 0 failures.
|
||||
|
||||
- [ ] **Step 6: Run the existing SecurityMemberAccess suite**
|
||||
|
||||
Run: `mvn test -DskipAssembly -pl core -Dtest=SecurityMemberAccessTest`
|
||||
|
||||
Expected: PASS, 0 failures, still with no edits to that file.
|
||||
|
||||
- [ ] **Step 7: Commit**
|
||||
|
||||
```bash
|
||||
git add core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java \
|
||||
core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessPackageMatchingTest.java
|
||||
git commit -m "WW-5674 perf(ognl): walk package names in place instead of building prefixes
|
||||
|
||||
Replaces the split/IntStream/String.join prefix construction with an index walk,
|
||||
extracted into a pure package-private helper so it can be tested against package
|
||||
name shapes no real Class can produce. Per call this drops a String[], a list
|
||||
wrapper, a stream pipeline, N sublist views and N joined strings, leaving one
|
||||
substring per package level.
|
||||
|
||||
Equivalence with the replaced implementation is asserted over a matrix of
|
||||
package name shapes and candidate sets."
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Task 4: Collapse the allowlist path to a single walk
|
||||
|
||||
`isClassAllowlisted` walks the same package name twice, once per allowlist set. Add a two-set overload and use it.
|
||||
|
||||
**Files:**
|
||||
- Modify: `core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java:254-261` (`isClassAllowlisted`) and the `isClassBelongsToPackages` block from Task 3
|
||||
- Modify: `core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessPackageMatchingTest.java` (add one test)
|
||||
|
||||
**Interfaces:**
|
||||
- Consumes: `isPackageBelongsToPackages(String, Set<String>, Set<String>)` from Task 3.
|
||||
- Produces: `static boolean SecurityMemberAccess.isClassBelongsToPackages(Class<?> clazz, Set<String> first, Set<String> second)` — package-private, matching `isPackageBelongsToPackages` beside it. (The plan originally specified `public static`; it was narrowed during the final review, since the overload has one caller and its only test is in the same package.)
|
||||
|
||||
- [ ] **Step 1: Write the failing test**
|
||||
|
||||
Add to `SecurityMemberAccessPackageMatchingTest`:
|
||||
|
||||
```java
|
||||
@Test
|
||||
public void twoSetOverloadEqualsDisjunctionOfSingleSetCalls() throws Exception {
|
||||
for (Class<?> clazz : classShapes()) {
|
||||
for (Set<String> first : CANDIDATE_SETS) {
|
||||
for (Set<String> second : CANDIDATE_SETS) {
|
||||
assertThat(isClassBelongsToPackages(clazz, first, second))
|
||||
.as("clazz=[%s] first=%s second=%s", clazz.getName(), first, second)
|
||||
.isEqualTo(isClassBelongsToPackages(clazz, first)
|
||||
|| isClassBelongsToPackages(clazz, second));
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Run test to verify it fails**
|
||||
|
||||
Run: `mvn test -DskipAssembly -pl core -Dtest=SecurityMemberAccessPackageMatchingTest`
|
||||
|
||||
Expected: COMPILATION FAILURE — `cannot find symbol: method isClassBelongsToPackages(Class<CAP#1>,Set<String>,Set<String>)`. That is the red state for this task.
|
||||
|
||||
- [ ] **Step 3: Add the overload**
|
||||
|
||||
In `SecurityMemberAccess.java`, replace the two-argument method written in Task 3:
|
||||
|
||||
```java
|
||||
public static boolean isClassBelongsToPackages(Class<?> clazz, Set<String> matchingPackages) {
|
||||
return isPackageBelongsToPackages(toPackageName(clazz), matchingPackages, emptySet());
|
||||
}
|
||||
```
|
||||
|
||||
with the delegating pair:
|
||||
|
||||
```java
|
||||
public static boolean isClassBelongsToPackages(Class<?> clazz, Set<String> matchingPackages) {
|
||||
return isClassBelongsToPackages(clazz, matchingPackages, emptySet());
|
||||
}
|
||||
|
||||
/**
|
||||
* Tests the class's package against two sets in a single walk. Equivalent to calling
|
||||
* {@link #isClassBelongsToPackages(Class, Set)} once per set and OR-ing the results, but
|
||||
* walks the package name only once.
|
||||
*
|
||||
* @param clazz the class whose package is tested
|
||||
* @param first the first set of package names to match against
|
||||
* @param second the second set of package names to match against
|
||||
* @return {@code true} if the class's package or any parent package is in either set
|
||||
*/
|
||||
static boolean isClassBelongsToPackages(Class<?> clazz, Set<String> first, Set<String> second) {
|
||||
return isPackageBelongsToPackages(toPackageName(clazz), first, second);
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 4: Use the overload in `isClassAllowlisted`**
|
||||
|
||||
In `SecurityMemberAccess.java`, replace the final two clauses of `isClassAllowlisted` (lines 259–260):
|
||||
|
||||
```java
|
||||
|| isClassBelongsToPackages(clazz, ALLOWLIST_REQUIRED_PACKAGES)
|
||||
|| isClassBelongsToPackages(clazz, allowlistPackageNames);
|
||||
```
|
||||
|
||||
with a single clause:
|
||||
|
||||
```java
|
||||
|| isClassBelongsToPackages(clazz, ALLOWLIST_REQUIRED_PACKAGES, allowlistPackageNames);
|
||||
```
|
||||
|
||||
The full method then reads:
|
||||
|
||||
```java
|
||||
protected boolean isClassAllowlisted(Class<?> clazz) {
|
||||
return allowlistClasses.contains(clazz)
|
||||
|| ALLOWLIST_REQUIRED_CLASSES.contains(clazz)
|
||||
|| (providerAllowlist != null && providerAllowlist.getProviderAllowlist().contains(clazz))
|
||||
|| (threadAllowlist != null && threadAllowlist.getAllowlist().contains(clazz))
|
||||
|| isClassBelongsToPackages(clazz, ALLOWLIST_REQUIRED_PACKAGES, allowlistPackageNames);
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 5: Run tests to verify they pass**
|
||||
|
||||
Run: `mvn test -DskipAssembly -pl core -Dtest=SecurityMemberAccessPackageMatchingTest`
|
||||
|
||||
Expected: PASS, 9 tests, 0 failures.
|
||||
|
||||
- [ ] **Step 6: Run the full core suite**
|
||||
|
||||
Run: `mvn test -DskipAssembly -pl core`
|
||||
|
||||
Expected: BUILD SUCCESS, 0 failures, 0 errors. This is the real gate — `SecurityMemberAccessTest`, `OgnlValueStackTest`, `OgnlUtilTest` and the allowlist tests all exercise these paths end to end. Confirm `SecurityMemberAccessTest.java` is still unmodified:
|
||||
|
||||
```bash
|
||||
git status --porcelain core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessTest.java
|
||||
```
|
||||
|
||||
Expected: no output.
|
||||
|
||||
- [ ] **Step 7: Commit**
|
||||
|
||||
```bash
|
||||
git add core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java \
|
||||
core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessPackageMatchingTest.java
|
||||
git commit -m "WW-5674 perf(ognl): match both allowlist package sets in one walk
|
||||
|
||||
isClassAllowlisted walked the class's package name twice, once for
|
||||
ALLOWLIST_REQUIRED_PACKAGES and once for the configured allowlist. A two-set
|
||||
overload probes both sets at each prefix, halving the work on a path that runs
|
||||
for every OGNL member access.
|
||||
|
||||
Asserted equivalent to OR-ing the two single-set calls across a matrix of class
|
||||
shapes and candidate sets."
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Self-Review
|
||||
|
||||
**Spec coverage.** Every requirement maps to a task:
|
||||
|
||||
| Spec requirement | Task |
|
||||
|---|---|
|
||||
| `toPackageName` guard + `getPackageName()` | Task 2 |
|
||||
| Extract `isPackageBelongsToPackages` (package-private) | Task 3 |
|
||||
| Index walk, shortest-prefix-first, `isEmpty()` short-circuit | Task 3 |
|
||||
| Two public entry points delegate to one walk | Tasks 3, 4 |
|
||||
| Single walk in `isClassAllowlisted` | Task 4 |
|
||||
| Import cleanup (`List`, `IntStream`) | Task 3, Step 4 |
|
||||
| Test 1 — differential vs legacy over shapes | Task 3, `indexWalkMatchesLegacyAcrossPackageNameShapes` |
|
||||
| Test 2 — package-boundary regression | Task 1, `siblingPackageWithSharedCharacterPrefixDoesNotMatch` |
|
||||
| Test 3 — default-package `""` edge | Task 1, `defaultPackageMatchesOnlyWhenEmptyStringConfigured` + `dotOnlyConfigurationYieldsEmptyStringPackageName` |
|
||||
| Test 4 — `toPackageName` over 11 class shapes | Task 1, `toPackageNameMatchesLegacyAcrossClassShapes` |
|
||||
| Test 5 — two-set overload equivalence | Task 4, `twoSetOverloadEqualsDisjunctionOfSingleSetCalls` |
|
||||
| Test 6 — existing suite unchanged, full core green | Tasks 2, 3, 4 (Step 6) |
|
||||
| No JMH, no timing assertions | Global Constraints |
|
||||
| Array/primitive semantics unchanged | Task 2 comment + `arraysAndPrimitivesResolveToTheEmptyPackage` |
|
||||
|
||||
One spec deviation, flagged in *File Structure*: tests live in a new `SecurityMemberAccessPackageMatchingTest` rather than the 1136-line `SecurityMemberAccessTest`.
|
||||
|
||||
**Placeholder scan.** No TBD/TODO, no "handle edge cases", no "similar to Task N". Every code step carries complete, compilable content.
|
||||
|
||||
**Type consistency.** `isPackageBelongsToPackages(String, Set<String>, Set<String>)` is package-private and named identically in Tasks 3 and 4. `isClassBelongsToPackages` keeps its two-argument signature throughout and gains a three-argument overload in Task 4 only. `legacyPrefixMatch(String, Set<String>)`, `legacyToPackageName(Class<?>)`, `classShapes()`, `PACKAGE_NAMES` and `CANDIDATE_SETS` are defined once in Task 1 and referenced under those exact names in Tasks 3 and 4. `classShapes()` throws `Exception` (via `Class.forName`), so every test using it declares `throws Exception` — checked in Tasks 1 and 4.
|
||||
|
||||
## Follow-ups not in scope
|
||||
|
||||
All of these are now filed, so they may be referenced from code and commit messages without breaching the project's no-placeholder-TODO rule.
|
||||
|
||||
- **WW-5675** covers the dominant cost (config re-parsing driven by the `Scope.PROTOTYPE` bean). WW-5674 alone will not move the 9% figure much. It also absorbed **`ConfigParseUtil.validatePackageNames`** (`ConfigParseUtil.java:143`), which evaluates `Pattern.compile("\\s")` once per package name rather than once overall — same root cause, per-instantiation work that should happen once.
|
||||
- **WW-5676** — whether array and primitive types should resolve to their element package. Tightens the exclusion list, loosens the allowlist. Filed as a standalone Improvement against 7.4.0 rather than a sub-task, because it is a security-semantics decision rather than a performance fix.
|
||||
- **WW-5677** — the remaining per-access `getPackage()` lookups in `checkDefaultPackageAccess` and `isExcludedPackageNamePatterns`. Same file as this plan, same hot path, but left alone here to keep this change reviewable as a single concern.
|
||||
-381
@@ -1,381 +0,0 @@
|
||||
# WW-5674 — Cut the per-call allocations in `SecurityMemberAccess.isClassBelongsToPackages`
|
||||
|
||||
> The walk is allocation-*reduced*, not allocation-free: it still creates one
|
||||
> `substring` per package level. What it removes is everything around that — the
|
||||
> `String[]`, the list wrapper, the stream pipeline, the sublist views, and the
|
||||
> joined result strings.
|
||||
|
||||
**Date:** 2026-08-03
|
||||
**Ticket:** [WW-5674](https://issues.apache.org/jira/browse/WW-5674) (sub-task of [WW-5667](https://issues.apache.org/jira/browse/WW-5667))
|
||||
**Sibling:** [WW-5675](https://issues.apache.org/jira/browse/WW-5675) — config re-parsing on every `SecurityMemberAccess` instantiation (separate spec)
|
||||
|
||||
## Background
|
||||
|
||||
WW-5667 reports that OGNL security checks consume 9% of RUNNABLE CPU samples in a
|
||||
2-minute JFR profile of a preprod Payara server under moderate load. The report
|
||||
contains two stack samples that point at two independent problems.
|
||||
|
||||
This spec covers **sample 1** only — the per-OGNL-access cost of
|
||||
`SecurityMemberAccess.isClassBelongsToPackages`:
|
||||
|
||||
```
|
||||
java.lang.String.split(String)
|
||||
SecurityMemberAccess.isClassBelongsToPackages(Class, Set) :390
|
||||
SecurityMemberAccess.isExcludedPackageNames(Class) :386
|
||||
SecurityMemberAccess.isPackageExcluded(Class) :371
|
||||
```
|
||||
|
||||
Sample 2 — repeated re-parsing of the raw configuration strings, caused by
|
||||
`SecurityMemberAccess` being a `Scope.PROTOTYPE` bean — is the dominant cost but
|
||||
is a different change with a different risk profile. It is tracked separately as
|
||||
WW-5675.
|
||||
|
||||
The fix proposed on WW-5667 (cache the parsed `Set` in a `SecurityMemberAccess`
|
||||
field) addresses neither problem: it does not touch this hot path at all, and it
|
||||
cannot help sample 2 because the instance holding the field is itself discarded
|
||||
and rebuilt on each container lookup.
|
||||
|
||||
## Problem
|
||||
|
||||
```java
|
||||
public static boolean isClassBelongsToPackages(Class<?> clazz, Set<String> matchingPackages) {
|
||||
List<String> packageParts = List.of(toPackageName(clazz).split("\\."));
|
||||
return IntStream.range(0, packageParts.size())
|
||||
.mapToObj(i -> String.join(".", packageParts.subList(0, i + 1)))
|
||||
.anyMatch(matchingPackages::contains);
|
||||
}
|
||||
```
|
||||
|
||||
For a class whose package has N segments, one call allocates: a `String[]` plus
|
||||
its N element substrings from `split`, a `List.of` wrapper, an `IntStream`
|
||||
pipeline, N `subList` views, and N `StringJoiner`-built result strings. For
|
||||
`org.apache.struts2.ognl` that is roughly a dozen objects.
|
||||
|
||||
The method is invoked up to four times per `isAccessible()` call — the
|
||||
excluded-package check and the allowlist check, each applied to both the
|
||||
member's declaring class and the target class.
|
||||
|
||||
A second, smaller cost sits underneath it:
|
||||
|
||||
```java
|
||||
public static String toPackageName(Class<?> clazz) {
|
||||
if (clazz.getPackage() == null) {
|
||||
return "";
|
||||
}
|
||||
return clazz.getPackage().getName();
|
||||
}
|
||||
```
|
||||
|
||||
`Class.getPackage()` resolves through the defining classloader's package map on
|
||||
every call. `Class.getPackageName()` (Java 9+) computes the name once and caches
|
||||
it on the `Class` object.
|
||||
|
||||
## Goals
|
||||
|
||||
- Cut the per-call allocation overhead on the OGNL member-access hot path down to
|
||||
one substring per package level.
|
||||
- **Zero change to allow/deny semantics**, demonstrated by test, not by argument.
|
||||
- Keep the change small enough to review as a pure optimisation.
|
||||
|
||||
## Non-goals
|
||||
|
||||
- The `Scope.PROTOTYPE` config re-parsing (WW-5675).
|
||||
- Changing how arrays and primitives resolve to package names — see
|
||||
*Deliberately out of scope* below.
|
||||
- Any caching layer, memoisation, or new data structure.
|
||||
- Any change to `struts.excludedPackageNames`, `struts.allowlist.packageNames`,
|
||||
or the surrounding configuration.
|
||||
|
||||
## Verified current semantics
|
||||
|
||||
The rewrite must reproduce the existing prefix set exactly. The following was
|
||||
established empirically on the target JDK (17, `maven.compiler.release=17`),
|
||||
not inferred:
|
||||
|
||||
| package name | `split("\\.")` | prefixes probed |
|
||||
|---|---|---|
|
||||
| `""` | `[""]` (length 1) | `[""]` |
|
||||
| `"java"` | `["java"]` | `["java"]` |
|
||||
| `"a.b.c"` | `["a","b","c"]` | `["a", "a.b", "a.b.c"]` |
|
||||
| `"a..b"` | `["a","","b"]` | `["a", "a.", "a..b"]` |
|
||||
| `".a"` | `["","a"]` | `["", ".a"]` |
|
||||
| `"a.b."` | `["a","b"]` | `["a", "a.b"]` |
|
||||
|
||||
Two consequences worth stating explicitly, because both are easy to regress:
|
||||
|
||||
1. **The default package probes `contains("")`.** `"".split("\\.")` yields a
|
||||
one-element array containing the empty string, so a class in the default
|
||||
package tests the set for `""`. This is reachable in practice:
|
||||
`commaDelimitedStringToSet` filters empty entries *before*
|
||||
`ConfigParseUtil.toPackageNamesSet` applies `strip(s, ".")`, so a
|
||||
configuration of `struts.excludedPackageNames="."` puts `""` into the set and
|
||||
excludes default-package classes. Confirmed live:
|
||||
`isClassBelongsToPackages(defaultPkgClass, Set.of("")) == true`.
|
||||
|
||||
2. **Trailing-dot inputs are the only divergence.** `split` drops trailing empty
|
||||
segments, so `"a.b."` probes `["a", "a.b"]` whereas an index walk would also
|
||||
probe `"a.b."`. `Class.getPackage().getName()` cannot produce a trailing dot,
|
||||
so this shape is unreachable through every caller. It is recorded here so a
|
||||
future reader does not mistake it for a bug.
|
||||
|
||||
Every other shape is exactly reproducible by an index walk: probe
|
||||
`P.substring(0, j)` at each `j` where `P.charAt(j) == '.'`, then probe `P`.
|
||||
|
||||
### `toPackageName` guard equivalence
|
||||
|
||||
`clazz.getPackage()` returns null for exactly primitives, `void`, and arrays.
|
||||
Verified across eleven class shapes:
|
||||
|
||||
| class | `getPackage()` | current result | `isArray()/isPrimitive()` guard |
|
||||
|---|---|---|---|
|
||||
| `String` | non-null | `"java.lang"` | `"java.lang"` |
|
||||
| default-package class | non-null | `""` | `""` |
|
||||
| nested (`Map.Entry`) | non-null | `"java.util"` | `"java.util"` |
|
||||
| lambda (hidden class) | non-null | `"org.apache.struts2.ognl"` | `"org.apache.struts2.ognl"` |
|
||||
| JDK proxy | non-null | `"jdk.proxy1"` | `"jdk.proxy1"` |
|
||||
| `int`, `void` | null | `""` | `""` |
|
||||
| `int[]`, `String[]`, `String[][]` | null | `""` | `""` |
|
||||
|
||||
All eleven agree. Note that `void.class.isPrimitive()` is `true`, so `void` is
|
||||
covered by the guard.
|
||||
|
||||
## Design
|
||||
|
||||
One file: `core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java`.
|
||||
|
||||
### 1. Cheaper package-name lookup, identical result
|
||||
|
||||
```java
|
||||
public static String toPackageName(Class<?> clazz) {
|
||||
if (clazz.isArray() || clazz.isPrimitive()) {
|
||||
return "";
|
||||
}
|
||||
return clazz.getPackageName();
|
||||
}
|
||||
```
|
||||
|
||||
The guard covers precisely the cases where `getPackage()` returns null, so the
|
||||
result is unchanged for every input while avoiding the classloader package-map
|
||||
lookup on the common path.
|
||||
|
||||
### 2. Extract the walk over a package-name string
|
||||
|
||||
Taking a `String` rather than a `Class` makes the prefix logic directly testable
|
||||
with shapes no real `Class` can produce (`""`, `"a..b"`, `".a"`), which is what
|
||||
the differential test needs.
|
||||
|
||||
```java
|
||||
static boolean isPackageBelongsToPackages(String packageName, Set<String> first, Set<String> second) {
|
||||
if (first.isEmpty() && second.isEmpty()) {
|
||||
return false;
|
||||
}
|
||||
int idx = packageName.indexOf('.');
|
||||
while (idx != -1) {
|
||||
String prefix = packageName.substring(0, idx);
|
||||
if (first.contains(prefix) || second.contains(prefix)) {
|
||||
return true;
|
||||
}
|
||||
idx = packageName.indexOf('.', idx + 1);
|
||||
}
|
||||
return first.contains(packageName) || second.contains(packageName);
|
||||
}
|
||||
```
|
||||
|
||||
Package-private: it is an implementation detail, exposed only far enough for the
|
||||
test in the same package to reach it.
|
||||
|
||||
Shortest-prefix-first ordering is preserved. Ordering does not affect the result
|
||||
(the operation is a disjunction) but it short-circuits earliest on broad
|
||||
exclusions such as `java.io`, which are the common case.
|
||||
|
||||
The `isEmpty()` short-circuit skips the walk — and therefore every substring
|
||||
allocation — when neither set is configured. That requires both sets to be
|
||||
empty, so it does not fire on the allowlist path, where
|
||||
`ALLOWLIST_REQUIRED_PACKAGES` is always non-empty (see §4), nor on the
|
||||
exclusion path under the shipped configuration, where
|
||||
`struts.excludedPackageNames` carries roughly thirty entries by default. It
|
||||
protects deployments that configure both sets empty.
|
||||
|
||||
### 3. Both entry points delegate to it
|
||||
|
||||
```java
|
||||
public static boolean isClassBelongsToPackages(Class<?> clazz, Set<String> matchingPackages) {
|
||||
return isClassBelongsToPackages(clazz, matchingPackages, emptySet());
|
||||
}
|
||||
|
||||
static boolean isClassBelongsToPackages(Class<?> clazz, Set<String> first, Set<String> second) {
|
||||
return isPackageBelongsToPackages(toPackageName(clazz), first, second);
|
||||
}
|
||||
```
|
||||
|
||||
One copy of the prefix logic, reached by every caller.
|
||||
|
||||
The existing two-argument signature is retained unchanged. It is `public static`
|
||||
on a public class, so it is nominally API even though a repository-wide search
|
||||
finds no caller outside `SecurityMemberAccess` itself.
|
||||
|
||||
The new three-argument overload is package-private. It has exactly one caller
|
||||
(`isClassAllowlisted`) and its only test lives in the same package, so
|
||||
package-private reaches everything that needs it, and it matches the visibility
|
||||
of `isPackageBelongsToPackages` beside it. Publishing it would freeze it as
|
||||
`struts2-core` API until the next major release for no benefit — particularly
|
||||
unwelcome while WW-4759 is drawing the `struts2-api` boundary.
|
||||
|
||||
That leaves a package-private overload sharing a name with a public method, which
|
||||
invites a later contributor to widen it to `public` as a consistency tidy-up
|
||||
without realising that adds permanent API surface. Renaming it — along with the
|
||||
awkward `is...BelongsTo...` grammar, and narrowing the two public statics that
|
||||
have no callers outside this class — is tracked as WW-5678 against 8.0.0, since
|
||||
the narrowing is source-breaking.
|
||||
|
||||
### 4. Single walk on the allowlist path
|
||||
|
||||
```java
|
||||
protected boolean isClassAllowlisted(Class<?> clazz) {
|
||||
return allowlistClasses.contains(clazz)
|
||||
|| ALLOWLIST_REQUIRED_CLASSES.contains(clazz)
|
||||
|| (providerAllowlist != null && providerAllowlist.getProviderAllowlist().contains(clazz))
|
||||
|| (threadAllowlist != null && threadAllowlist.getAllowlist().contains(clazz))
|
||||
|| isClassBelongsToPackages(clazz, ALLOWLIST_REQUIRED_PACKAGES, allowlistPackageNames);
|
||||
}
|
||||
```
|
||||
|
||||
Two walks over the same package name become one. `ALLOWLIST_REQUIRED_PACKAGES`
|
||||
is a non-empty constant, so the `isEmpty()` short-circuit does not fire here;
|
||||
the saving is the second walk.
|
||||
|
||||
Semantically identical: probing prefix `p` against `A` then `B` at each step
|
||||
yields the same disjunction as walking all prefixes against `A` and then all
|
||||
prefixes against `B`.
|
||||
|
||||
`isExcludedPackageNames` continues to call the two-argument form and is
|
||||
unchanged apart from inheriting the faster implementation.
|
||||
|
||||
### 5. Import cleanup
|
||||
|
||||
`java.util.List` and `java.util.stream.IntStream` become unused in
|
||||
`SecurityMemberAccess` once the stream pipeline is gone and must be removed.
|
||||
`java.util.Collections.emptySet` is already statically imported and is reused by
|
||||
the two-argument delegate.
|
||||
|
||||
## Data flow and error handling
|
||||
|
||||
Unchanged. Same inputs, same boolean output, no new exceptions, no new state, no
|
||||
caching, no new thread-safety considerations. `isPackageBelongsToPackages` is a
|
||||
pure function of its arguments.
|
||||
|
||||
## Deliberately out of scope: array and primitive package semantics
|
||||
|
||||
`Class.getPackageName()` resolves arrays to their element type's package
|
||||
(`java.io.File[]` → `"java.io"`, `String[]` → `"java.lang"`) and primitives to
|
||||
`"java.lang"`, whereas the current code yields `""` for both. Adopting those
|
||||
semantics was considered and rejected for this ticket because the change is
|
||||
**bidirectional**, not a pure hardening:
|
||||
|
||||
- **Exclusion path tightens.** `java.io.File[]` currently escapes
|
||||
`struts.excludedPackageNames` because its package is `""`; it would become
|
||||
excluded.
|
||||
- **Allowlist path loosens.** An application that allowlists `com.app.actions`
|
||||
does not today thereby allowlist `com.app.actions.MyThing[]`. It would. Arrays
|
||||
of allowlisted-package types become reachable where they previously required
|
||||
an explicit `struts.allowlist.classes` entry.
|
||||
|
||||
The allowlist is the primary OGNL defence in Struts 7.x and is enabled by
|
||||
default, so a change that makes it more permissive needs its own security
|
||||
reasoning, its own tests, and its own release note. The `isArray()/isPrimitive()`
|
||||
guard in this spec preserves current behaviour exactly and captures the
|
||||
`getPackageName()` performance win for ordinary classes, which is all real
|
||||
traffic.
|
||||
|
||||
A follow-up ticket should be filed to decide the array/primitive question on its
|
||||
own merits. This spec does not prejudge it.
|
||||
|
||||
## Testing
|
||||
|
||||
The equivalence proof is the deliverable; the speedup is a consequence. Tests go
|
||||
in `core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessTest.java`,
|
||||
which is **JUnit 4** (`org.junit.Test`, `org.junit.Before`, plain class, AssertJ
|
||||
and Mockito) — not JUnit 5.
|
||||
|
||||
1. **Differential test.** Add the current algorithm to the test class as a
|
||||
private reference implementation, transcribed so that it takes the package
|
||||
name directly rather than a `Class` — the body is otherwise verbatim:
|
||||
|
||||
```java
|
||||
private static boolean legacyPrefixMatch(String packageName, Set<String> matchingPackages) {
|
||||
List<String> packageParts = List.of(packageName.split("\\."));
|
||||
return IntStream.range(0, packageParts.size())
|
||||
.mapToObj(i -> String.join(".", packageParts.subList(0, i + 1)))
|
||||
.anyMatch(matchingPackages::contains);
|
||||
}
|
||||
```
|
||||
|
||||
Taking a `String` is what lets the matrix cover shapes no real `Class` can
|
||||
produce; `toPackageName` equivalence is proven separately by test 4, so the
|
||||
two halves of the original method are each covered.
|
||||
|
||||
Assert `legacyPrefixMatch(p, s)` equals
|
||||
`isPackageBelongsToPackages(p, s, emptySet())` across a matrix of
|
||||
package-name shapes × candidate sets. Shapes: `""`, `"java"`, `"a.b.c"`,
|
||||
`"a..b"`, `".a"`, and realistic deep package names — but **not** trailing-dot
|
||||
inputs, which are the one known divergence and are unreachable through every
|
||||
caller (see *Verified current semantics*). Sets: empty, exact match,
|
||||
parent-package match, no match, and the `""` set.
|
||||
|
||||
This is the strongest available evidence that the rewrite is
|
||||
behaviour-preserving, and it stays readable in review.
|
||||
|
||||
2. **Package-boundary regression.** `org.apache.struts2x` must not match a set
|
||||
containing `org.apache.struts2`. This is the classic prefix-matching bug the
|
||||
rewrite could plausibly introduce and the single most important assertion in
|
||||
the change.
|
||||
|
||||
3. **Default-package edge.** A set containing `""` must still match
|
||||
default-package classes. Currently untested, obscure, and easy to regress
|
||||
silently.
|
||||
|
||||
4. **`toPackageName` equivalence** over the eleven class shapes tabulated above,
|
||||
asserting the guard agrees with `getPackage()`-based resolution.
|
||||
|
||||
5. **Two-set overload equivalence.** `isClassBelongsToPackages(c, A, B)` equals
|
||||
`isClassBelongsToPackages(c, A) || isClassBelongsToPackages(c, B)` across the
|
||||
matrix, covering empty-`A`, empty-`B`, and both-empty.
|
||||
|
||||
6. **Existing suite unchanged.** `SecurityMemberAccessTest` passes without
|
||||
modification to any existing assertion, and the full `core` module suite is
|
||||
green: `mvn test -DskipAssembly -pl core`.
|
||||
|
||||
### Performance verification
|
||||
|
||||
The project has no JMH harness and none is added for this change. The win is
|
||||
established by allocation count — a `String[]` plus N substrings, a list
|
||||
wrapper, a stream pipeline, N sublist views and N joined strings, reduced to N
|
||||
substrings — and confirmed with a throwaway benchmark that is **not** committed.
|
||||
No timing assertion is added to the test suite, since wall-clock assertions are
|
||||
unreliable in CI.
|
||||
|
||||
## Risks
|
||||
|
||||
| Risk | Mitigation |
|
||||
|---|---|
|
||||
| Prefix matching without package-boundary awareness silently widens exclusion or allowlist matching | Test 2 asserts `org.apache.struts2x` does not match `org.apache.struts2` |
|
||||
| Default-package `""` edge regresses unnoticed | Test 3 pins it |
|
||||
| `toPackageName` guard misses a null-`getPackage()` case | Guard verified against eleven class shapes; test 4 pins them |
|
||||
| Two-set overload changes evaluation semantics | Test 5 asserts equivalence to the disjunction of two single-set calls |
|
||||
| Reviewer mistakes this for the fix WW-5667 asked for | Ticket descriptions and PR body state that WW-5667's proposed fix addresses neither problem, and that WW-5675 covers the dominant cost |
|
||||
|
||||
## Out of scope for this spec
|
||||
|
||||
- WW-5675 (config re-parsing / `Scope.PROTOTYPE`) — separate spec and PR.
|
||||
- Array and primitive package semantics — WW-5676, see above.
|
||||
- `isExcludedPackageNamePatterns`, which walks `excludedPackageNamePatterns` with
|
||||
a stream and calls `toPackageName` per pattern. It benefits from the cheaper
|
||||
`toPackageName` for free, but its own stream overhead is not addressed here;
|
||||
the pattern set is empty by default. Tracked as WW-5677.
|
||||
- `checkDefaultPackageAccess`, which still inspects `clazz.getPackage()` directly —
|
||||
two classloader package-map lookups per class, up to four per `isAccessible()`
|
||||
when `struts.disallowDefaultPackageAccess` is enabled. Its condition is
|
||||
equivalent to `toPackageName(clazz).isEmpty()`, including for arrays and
|
||||
primitives, so routing it through `toPackageName` would remove exactly the
|
||||
lookup this spec eliminates fifteen lines away. Deliberately left out to keep
|
||||
this change to one concern; tracked as WW-5677.
|
||||
Reference in New Issue
Block a user