diff --git a/core/src/main/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequest.java b/core/src/main/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequest.java index 3bb0d17d2f..cb5c6b9ecd 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequest.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequest.java @@ -19,6 +19,7 @@ package org.apache.struts2.dispatcher.multipart; import jakarta.servlet.http.HttpServletRequest; +import org.apache.commons.fileupload2.core.AbstractFileUpload; import org.apache.commons.fileupload2.core.DiskFileItemFactory; import org.apache.commons.fileupload2.core.FileUploadByteCountLimitException; import org.apache.commons.fileupload2.core.FileUploadContentTypeException; @@ -32,6 +33,7 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.apache.struts2.StrutsConstants; +import org.apache.struts2.StrutsException; import org.apache.struts2.dispatcher.LocalizedMessage; import org.apache.struts2.inject.Inject; @@ -60,6 +62,12 @@ public abstract class AbstractMultiPartRequest implements MultiPartRequest { private static final Logger LOG = LogManager.getLogger(AbstractMultiPartRequest.class); + /** + * Verified once per JVM: whether the commons-fileupload2 API on the classpath matches what + * Struts compiled against. Guards against a mismatched milestone resolving at runtime. + */ + private static volatile boolean fileUploadApiVerified; + /** * Defines the internal buffer size used during streaming operations. */ @@ -211,6 +219,7 @@ protected JakartaServletDiskFileUpload createJakartaFileUpload(Charset charset, } protected JakartaServletDiskFileUpload prepareServletFileUpload(Charset charset, Path saveDir) { + ensureFileUploadApiVerified(); JakartaServletDiskFileUpload servletFileUpload = createJakartaFileUpload(charset, saveDir); if (maxSize != null) { @@ -228,6 +237,48 @@ protected JakartaServletDiskFileUpload prepareServletFileUpload(Charset charset, return servletFileUpload; } + /** + * Verifies once per JVM that the commons-fileupload2 API on the classpath matches what Struts + * compiled against, failing fast with an actionable message instead of a deep-stack + * {@link NoSuchMethodError} when a mismatched milestone is resolved. + */ + private static void ensureFileUploadApiVerified() { + if (!fileUploadApiVerified) { + verifyFileUploadApi(JakartaServletDiskFileUpload.class); + fileUploadApiVerified = true; + } + } + + /** + * Probes {@code uploadClass} for the size-limit setters Struts invokes in + * {@link #prepareServletFileUpload}. Package-private for testing. + * + * @param uploadClass the file upload class to verify + * @throws StrutsException if any required method is absent, indicating a binary-incompatible + * commons-fileupload2 version on the classpath + */ + static void verifyFileUploadApi(Class uploadClass) { + for (String method : new String[]{"setMaxSize", "setMaxFileCount", "setMaxFileSize"}) { + try { + uploadClass.getMethod(method, long.class); + } catch (NoSuchMethodException e) { + throw new StrutsException(String.format( + "Incompatible Apache Commons FileUpload on the classpath: %s.%s(long) is missing. " + + "Detected commons-fileupload2-core version [%s] and commons-fileupload2-jakarta-servlet6 version [%s]. " + + "Align commons-fileupload2-core with commons-fileupload2-jakarta-servlet6 (use the same release for both).", + uploadClass.getName(), method, + implementationVersion(AbstractFileUpload.class), + implementationVersion(uploadClass)), e); + } + } + } + + private static String implementationVersion(Class clazz) { + Package pkg = clazz.getPackage(); + String version = pkg != null ? pkg.getImplementationVersion() : null; + return version != null ? version : "unknown"; + } + protected RequestContext createRequestContext(HttpServletRequest request) { return new StrutsRequestContext(request); } diff --git a/core/src/test/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequestApiCheckTest.java b/core/src/test/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequestApiCheckTest.java new file mode 100644 index 0000000000..3dba48ccf0 --- /dev/null +++ b/core/src/test/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequestApiCheckTest.java @@ -0,0 +1,47 @@ +/* + * 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.dispatcher.multipart; + +import org.apache.commons.fileupload2.jakarta.servlet6.JakartaServletDiskFileUpload; +import org.apache.struts2.StrutsException; +import org.junit.Test; + +import static org.assertj.core.api.Assertions.assertThatCode; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +public class AbstractMultiPartRequestApiCheckTest { + + @Test + public void verifyFileUploadApiPassesForCompatibleClass() { + assertThatCode(() -> AbstractMultiPartRequest.verifyFileUploadApi(JakartaServletDiskFileUpload.class)) + .doesNotThrowAnyException(); + } + + @Test + public void verifyFileUploadApiThrowsForIncompatibleClass() { + assertThatThrownBy(() -> AbstractMultiPartRequest.verifyFileUploadApi(IncompatibleFileUpload.class)) + .isInstanceOf(StrutsException.class) + .hasMessageContaining("setMaxSize") + .hasMessageContaining("Align commons-fileupload2-core"); + } + + /** Stub lacking the size-limit setters, simulating a binary-incompatible fileupload version. */ + private static class IncompatibleFileUpload { + } +} diff --git a/docs/superpowers/plans/2026-06-10-WW-5632-fileupload2-milestone-hardening.md b/docs/superpowers/plans/2026-06-10-WW-5632-fileupload2-milestone-hardening.md new file mode 100644 index 0000000000..d52edb7f47 --- /dev/null +++ b/docs/superpowers/plans/2026-06-10-WW-5632-fileupload2-milestone-hardening.md @@ -0,0 +1,392 @@ +# commons-fileupload2 Milestone Hardening 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:** Make a `commons-fileupload2-core` / `-jakarta-servlet6` version skew impossible in Struts's own build and turn a future runtime `NoSuchMethodError` into a clear, actionable `StrutsException`. + +**Architecture:** Three independent changes. (A1) Introduce a single `commons-fileupload2.version` property and manage *both* fileupload artifacts in `parent/pom.xml`. (A2) Activate the dormant `maven-enforcer-plugin` with a fileupload-scoped `bannedDependencies` rule. (B) Add a once-per-JVM reflective API guard in `AbstractMultiPartRequest`. + +**Tech Stack:** Maven (multi-module), `maven-enforcer-plugin` 3.6.3, Java 17, JUnit 4 + AssertJ (the `core` module's established test stack), Apache Commons FileUpload 2.0.0-M5. + +**Ticket:** [WW-5632](https://issues.apache.org/jira/browse/WW-5632) +**Spec:** `docs/superpowers/specs/2026-06-10-fileupload2-milestone-hardening-design.md` +**Branch:** `WW-5632-fileupload2-milestone-hardening` (already checked out) + +--- + +## File Structure + +- `pom.xml` (root) — add the `commons-fileupload2.version` property; change the enforcer rule from `dependencyConvergence` to a scoped `bannedDependencies`; bind the enforcer into the active `` section. +- `parent/pom.xml` — reference the new property for `commons-fileupload2-jakarta-servlet6` and add a managed entry for `commons-fileupload2-core`. +- `core/src/main/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequest.java` — add the runtime API guard and call it from `prepareServletFileUpload`. +- `core/src/test/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequestApiCheckTest.java` (new) — unit tests for the guard. + +--- + +## Task 1: Manage both fileupload artifacts via a single version property (A1) + +**Files:** +- Modify: `pom.xml:118-119` (properties block) +- Modify: `parent/pom.xml:128-132` (dependencyManagement entry) + +- [ ] **Step 1: Add the version property to the root POM** + +In `pom.xml`, inside the `` block, add the property in alphabetical order between `byte-buddy.version` (line 118) and `freemarker.version` (line 119): + +```xml + 1.18.8 + 2.0.0-M5 + 2.3.34 +``` + +- [ ] **Step 2: Reference the property and add the `-core` managed entry** + +In `parent/pom.xml`, replace the existing single `commons-fileupload2-jakarta-servlet6` management entry (currently lines 128-132): + +```xml + + org.apache.commons + commons-fileupload2-jakarta-servlet6 + 2.0.0-M5 + +``` + +with two entries, both referencing the property (the volatile API lives in `-core`, so it must be pinned too): + +```xml + + org.apache.commons + commons-fileupload2-core + ${commons-fileupload2.version} + + + org.apache.commons + commons-fileupload2-jakarta-servlet6 + ${commons-fileupload2.version} + +``` + +- [ ] **Step 3: Verify both artifacts resolve to the pinned version** + +Run: +```bash +mvn -q -pl core dependency:list -DskipAssembly '-Dincludes=org.apache.commons:commons-fileupload2*' +``` +Expected: both `commons-fileupload2-core` and `commons-fileupload2-jakarta-servlet6` listed at `2.0.0-M5`. + +- [ ] **Step 4: Verify the reactor still builds** + +Run: +```bash +mvn -q validate -DskipAssembly +``` +Expected: `BUILD SUCCESS` (no errors from the new property / managed dependency). + +- [ ] **Step 5: Commit** + +```bash +git add pom.xml parent/pom.xml +git commit -m "WW-5632 build(deps): manage commons-fileupload2-core alongside jakarta-servlet6 + +Pin both commons-fileupload2 artifacts to a single +commons-fileupload2.version property so the volatile -core API can no +longer skew from -jakarta-servlet6 in the reactor. + +Co-Authored-By: Claude Opus 4.8 " +``` + +--- + +## Task 2: Activate a fileupload-scoped enforcer rule (A2) + +**Files:** +- Modify: `pom.xml:349-353` (enforcer rule config in ``) +- Modify: `pom.xml:373-378` (active `` section) + +- [ ] **Step 1: Replace the dormant `dependencyConvergence` rule with a scoped `bannedDependencies` rule** + +In `pom.xml`, inside the `maven-enforcer-plugin` execution in ``, replace the current configuration (lines 349-353): + +```xml + + + + + +``` + +with a rule that bans all commons-fileupload2 versions except the pinned one (`` are exceptions to the `` bans): + +```xml + + + + + org.apache.commons:commons-fileupload2-core + org.apache.commons:commons-fileupload2-jakarta-servlet6 + + + org.apache.commons:commons-fileupload2-core:${commons-fileupload2.version} + org.apache.commons:commons-fileupload2-jakarta-servlet6:${commons-fileupload2.version} + + + + +``` + +- [ ] **Step 2: Bind the enforcer into the active `` section** + +In `pom.xml`, inside the active `` block, add the enforcer plugin entry immediately after the `maven-release-plugin` entry (after line 378; version is inherited from ``): + +```xml + + org.apache.maven.plugins + maven-release-plugin + 3.3.1 + + + org.apache.maven.plugins + maven-enforcer-plugin + +``` + +- [ ] **Step 3: Verify the enforcer now executes and passes on the clean tree** + +Run: +```bash +mvn -q validate -DskipAssembly +``` +Expected: `BUILD SUCCESS`. To confirm the rule actually ran (not skipped), run: +```bash +mvn validate -DskipAssembly -pl core | grep -i "enforce" +``` +Expected: a line showing `maven-enforcer-plugin:3.6.3:enforce (enforce)` executing. + +- [ ] **Step 4: Verify the rule catches a skew (manual negative check, then revert)** + +Temporarily edit `parent/pom.xml` to set the `commons-fileupload2-core` managed version to a different value (e.g. `2.0.0-M4` instead of `${commons-fileupload2.version}`), then run: +```bash +mvn validate -DskipAssembly -pl core +``` +Expected: `BUILD FAILURE` with a `bannedDependencies` violation naming `commons-fileupload2-core`. +Then revert the edit: +```bash +git checkout -- parent/pom.xml +``` + +- [ ] **Step 5: Commit** + +```bash +git add pom.xml +git commit -m "WW-5632 build: enforce a single commons-fileupload2 version + +Activate maven-enforcer-plugin (previously dormant in pluginManagement) +with a fileupload-scoped bannedDependencies rule so any divergent +commons-fileupload2 version fails the build early. + +Co-Authored-By: Claude Opus 4.8 " +``` + +--- + +## Task 3: Runtime API guard in AbstractMultiPartRequest (B) + +**Files:** +- Test: `core/src/test/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequestApiCheckTest.java` (create) +- Modify: `core/src/main/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequest.java` (imports ~line 22 & 34; new method block near `prepareServletFileUpload` at line 213; call site at line 214) + +- [ ] **Step 1: Write the failing test** + +Create `core/src/test/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequestApiCheckTest.java`: + +```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.dispatcher.multipart; + +import org.apache.commons.fileupload2.jakarta.servlet6.JakartaServletDiskFileUpload; +import org.apache.struts2.StrutsException; +import org.junit.Test; + +import static org.assertj.core.api.Assertions.assertThatCode; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +public class AbstractMultiPartRequestApiCheckTest { + + @Test + public void verifyFileUploadApiPassesForCompatibleClass() { + assertThatCode(() -> AbstractMultiPartRequest.verifyFileUploadApi(JakartaServletDiskFileUpload.class)) + .doesNotThrowAnyException(); + } + + @Test + public void verifyFileUploadApiThrowsForIncompatibleClass() { + assertThatThrownBy(() -> AbstractMultiPartRequest.verifyFileUploadApi(IncompatibleFileUpload.class)) + .isInstanceOf(StrutsException.class) + .hasMessageContaining("setMaxSize") + .hasMessageContaining("Align commons-fileupload2-core"); + } + + /** Stub lacking the size-limit setters, simulating a binary-incompatible fileupload version. */ + private static class IncompatibleFileUpload { + } +} +``` + +- [ ] **Step 2: Run the test to verify it fails** + +Run: +```bash +mvn test -DskipAssembly -pl core -Dtest=AbstractMultiPartRequestApiCheckTest +``` +Expected: FAIL — compilation error `cannot find symbol: method verifyFileUploadApi(java.lang.Class)` (the guard does not exist yet). This is the red state. + +- [ ] **Step 3: Add the two imports** + +In `AbstractMultiPartRequest.java`, add the `-core` `AbstractFileUpload` import alongside the existing `fileupload2.core` imports (after line 22, `import org.apache.commons.fileupload2.core.DiskFileItemFactory;` — keep alphabetical, so `AbstractFileUpload` goes *before* it): + +```java +import org.apache.commons.fileupload2.core.AbstractFileUpload; +import org.apache.commons.fileupload2.core.DiskFileItemFactory; +``` + +And add the `StrutsException` import after the existing `StrutsConstants` import (line 34): + +```java +import org.apache.struts2.StrutsConstants; +import org.apache.struts2.StrutsException; +``` + +- [ ] **Step 4: Implement the guard and wire it into `prepareServletFileUpload`** + +In `AbstractMultiPartRequest.java`, add `ensureFileUploadApiVerified();` as the first statement of `prepareServletFileUpload` (currently line 213-214): + +```java + protected JakartaServletDiskFileUpload prepareServletFileUpload(Charset charset, Path saveDir) { + ensureFileUploadApiVerified(); + JakartaServletDiskFileUpload servletFileUpload = createJakartaFileUpload(charset, saveDir); +``` + +Then add the following members. Place the field next to the other static members (e.g. directly after the `LOG` field at line 61), and the three methods directly after the `prepareServletFileUpload` method (after its closing brace at line 229): + +Field (after line 61): + +```java + /** + * Verified once per JVM: whether the commons-fileupload2 API on the classpath matches what + * Struts compiled against. Guards against a mismatched milestone resolving at runtime. + */ + private static volatile boolean fileUploadApiVerified; +``` + +Methods (after `prepareServletFileUpload`): + +```java + /** + * Verifies once per JVM that the commons-fileupload2 API on the classpath matches what Struts + * compiled against, failing fast with an actionable message instead of a deep-stack + * {@link NoSuchMethodError} when a mismatched milestone is resolved. + */ + private void ensureFileUploadApiVerified() { + if (!fileUploadApiVerified) { + verifyFileUploadApi(JakartaServletDiskFileUpload.class); + fileUploadApiVerified = true; + } + } + + /** + * Probes {@code uploadClass} for the size-limit setters Struts invokes in + * {@link #prepareServletFileUpload}. Package-private for testing. + * + * @param uploadClass the file upload class to verify + * @throws StrutsException if any required method is absent, indicating a binary-incompatible + * commons-fileupload2 version on the classpath + */ + static void verifyFileUploadApi(Class uploadClass) { + for (String method : new String[]{"setMaxSize", "setMaxFileCount", "setMaxFileSize"}) { + try { + uploadClass.getMethod(method, long.class); + } catch (NoSuchMethodException e) { + throw new StrutsException(String.format( + "Incompatible Apache Commons FileUpload on the classpath: %s.%s(long) is missing. " + + "Detected commons-fileupload2-core version [%s] and commons-fileupload2-jakarta-servlet6 version [%s]. " + + "Align commons-fileupload2-core with commons-fileupload2-jakarta-servlet6 (use the same release for both).", + uploadClass.getName(), method, + implementationVersion(AbstractFileUpload.class), + implementationVersion(uploadClass)), e); + } + } + } + + private static String implementationVersion(Class clazz) { + Package pkg = clazz.getPackage(); + String version = pkg != null ? pkg.getImplementationVersion() : null; + return version != null ? version : "unknown"; + } +``` + +- [ ] **Step 5: Run the test to verify it passes** + +Run: +```bash +mvn test -DskipAssembly -pl core -Dtest=AbstractMultiPartRequestApiCheckTest +``` +Expected: PASS — both tests green. + +- [ ] **Step 6: Run the multipart regression tests** + +Run: +```bash +mvn test -DskipAssembly -pl core -Dtest='*MultiPartRequest*' +``` +Expected: PASS — `JakartaMultiPartRequestTest` and `JakartaStreamMultiPartRequestTest` still green (the guard runs once and does not change upload behavior). + +- [ ] **Step 7: Commit** + +```bash +git add core/src/main/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequest.java \ + core/src/test/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequestApiCheckTest.java +git commit -m "WW-5632 fix(fileupload): fail fast on incompatible commons-fileupload2 API + +Verify once per JVM that the fileupload size-limit setters exist and +throw a clear StrutsException reporting the core/jakarta version skew, +replacing an opaque deep-stack NoSuchMethodError in downstream runtimes. + +Co-Authored-By: Claude Opus 4.8 " +``` + +--- + +## Final Verification + +- [ ] **Run the full core test suite** + +Run: +```bash +mvn test -DskipAssembly -pl core +``` +Expected: `BUILD SUCCESS`, all tests pass, enforcer rule executed during `validate`. + +- [ ] **Confirm the working tree is clean and the branch holds three commits** + +Run: +```bash +git status --short && git log --oneline -3 +``` +Expected: no uncommitted changes; the three WW-5632 commits on top of the spec commit. diff --git a/docs/superpowers/specs/2026-06-10-fileupload2-milestone-hardening-design.md b/docs/superpowers/specs/2026-06-10-fileupload2-milestone-hardening-design.md new file mode 100644 index 0000000000..551486c29c --- /dev/null +++ b/docs/superpowers/specs/2026-06-10-fileupload2-milestone-hardening-design.md @@ -0,0 +1,206 @@ +# Design: Harden commons-fileupload2 against milestone churn + +**Date:** 2026-06-10 +**Status:** Approved design — pending implementation plan +**Ticket:** [WW-5632](https://issues.apache.org/jira/browse/WW-5632) +**Origin:** [user@struts mailing list thread](https://lists.apache.org/thread/fcdls8xvd9tp9o6dcog65vkqozv4nq5x) +(Tamás Barta, Struts 7.1.1 file upload `NoSuchMethodError`) +**Related (closed):** [WW-5615](https://issues.apache.org/jira/browse/WW-5615) — "Adapt to renamed +methods in Apache Commons FileUpload 2.0.0-M5", fixed in 7.2.0 via PR #1584 / commit `d2810d42f`. + +## Context + +A user on Struts 7.1.1 reported `java.lang.NoSuchMethodError: +'void org.apache.commons.fileupload2.jakarta.servlet6.JakartaServletDiskFileUpload.setSizeMax(long)'` +at upload time. Apache Commons FileUpload 2.0.0-M5 renamed several `AbstractFileUpload` methods +(`setSizeMax`→`setMaxSize`, `setFileSizeMax`→`setMaxFileSize`, `setFileCountMax`→`setMaxFileCount`), +breaking binary compatibility with M4. Struts declared M4 but the user's build resolved M5. + +WW-5615 (PR #1584) fixed the **symptom** for 7.2.0: it renamed the three call sites in +`AbstractMultiPartRequest.java` and bumped `commons-fileupload2-jakarta-servlet6` M4 → M5 in +`parent/pom.xml`. That commit did **nothing else**. + +This design addresses the **class of failure** that WW-5615 left open. + +## Root-cause chain (verified on current `main`, 7.2.0-SNAPSHOT) + +1. **Milestone dependency.** Struts depends on `-M` builds of commons-fileupload2, which break + binary compatibility between milestones. Until a 2.0.0 GA exists, Struts is committed to + milestone artifacts. +2. **The volatile API lives in an unmanaged artifact.** `setMaxSize(long)` / `setMaxFileCount(long)` + / `setMaxFileSize(long)` are declared on `org.apache.commons.fileupload2.core.AbstractFileUpload` + in **`commons-fileupload2-core`** (verified via `javap`). `JakartaServletDiskFileUpload` merely + inherits them. `parent/pom.xml` `` pins only + `commons-fileupload2-jakarta-servlet6` — **`commons-fileupload2-core` is unmanaged.** A transitive + dependency pulling a different `-core` milestone reproduces the exact `NoSuchMethodError` even + when `-jakarta-servlet6` is pinned correctly. +3. **The build guardrail is dormant.** `maven-enforcer-plugin` is configured with a + `dependencyConvergence` rule, but **only inside ``** of the root `pom.xml`; it is + never bound to an active `` section, so it never executes. Struts's own build would not + catch a fileupload version skew. +4. **The BOM does not help consumers.** `struts2-bom` exports only Struts module versions, not the + fileupload version. Downstream apps importing the BOM get no convergence assistance. + +Net effect: a downstream/transitive dependency can select a mismatched `commons-fileupload2-core` +milestone, and because milestones break binary compatibility, the user gets a runtime +`NoSuchMethodError` deep in request handling, with no build-time warning. + +## Goals + +- Make a `commons-fileupload2-core` / `-jakarta-servlet6` version skew **impossible within Struts's + own build**, deterministically. +- Fail the Struts build **early and clearly** if a future transitive dependency wants a + commons-fileupload2 version other than the tested one. +- For downstream consumer runtimes (where Struts's build guards cannot reach), replace the opaque + deep-stack `NoSuchMethodError` with a **clear, actionable `StrutsException`**. + +## Non-goals + +- **Shading/relocating commons-fileupload2.** Rejected: the library is security-sensitive (CVE + history); shading would force Struts to re-release on every fileupload CVE, against Apache norms, + and bloats the artifact. +- **Exporting the fileupload version through `struts2-bom`.** Considered and deferred — out of scope + for this change. +- **Migrating off milestone versions.** Not actionable until a commons-fileupload2 2.0.0 GA exists. + +## Design + +### Part A — Build-time fail-fast (POM) + +**A1. Manage both artifacts at one version.** +Introduce a single `commons-fileupload2.version` property (single source of truth) and add a +`` entry for `org.apache.commons:commons-fileupload2-core` alongside the +existing `commons-fileupload2-jakarta-servlet6` entry in `parent/pom.xml`, both referencing the +property. Because `` wins Maven version mediation, this forces a single, +matched `-core` version across the entire Struts reactor regardless of transitive requests — +closing root-cause #2 deterministically for Struts's own build. + +**A2. Activate a narrowly-scoped enforcer (chosen over global `dependencyConvergence`).** +Bind `maven-enforcer-plugin` into an active `` section with a `bannedDependencies` rule +scoped **only** to commons-fileupload2: ban all versions of +`org.apache.commons:commons-fileupload2-core` and +`org.apache.commons:commons-fileupload2-jakarta-servlet6` **except** the pinned +`${commons-fileupload2.version}`. This fails the build immediately if any transitive dependency +introduces a different fileupload version, with effectively zero blast radius on unrelated +dependencies. + +> **Why not global `dependencyConvergence`?** It has never actually run; activating it may surface +> many pre-existing, unrelated version conflicts across the multi-module build, ballooning scope +> unpredictably. The fileupload-scoped `bannedDependencies` rule targets exactly the failure mode in +> this report. (Global convergence remains a reasonable separate cleanup task, out of scope here.) + +The pinned version string lives once in the `commons-fileupload2.version` property and is referenced +by both the `` entries and the enforcer rule — no duplicated literals. + +### Part B — Runtime diagnostics guard + +Add a one-time, package-private static check in `AbstractMultiPartRequest`, invoked on first use +(e.g. at the top of `prepareServletFileUpload`), guarded so the reflective probe runs **once** per +JVM — no per-request cost. + +**Probe (testable, pure):** +`static void verifyFileUploadApi(Class uploadClass)` reflectively confirms that `uploadClass` +declares (inherited included) `setMaxSize(long)`, `setMaxFileCount(long)`, and `setMaxFileSize(long)`. +If any is absent it throws `org.apache.struts2.StrutsException`. + +**Self-maintaining message (no hardcoded "expected" version):** the exception reports the +implementation versions read at runtime from both packages — +`org.apache.commons.fileupload2.core.AbstractFileUpload.class.getPackage().getImplementationVersion()` +(the `-core` version) and `JakartaServletDiskFileUpload.class.getPackage().getImplementationVersion()` +(the `-jakarta-servlet6` version) — names the missing method, and instructs the user to align +`commons-fileupload2-core` with `commons-fileupload2-jakarta-servlet6`. Versions fall back to +`"unknown"` when no manifest implementation version is present. Surfacing the **skew** (core vs +jakarta versions) is the actionable signal; no version constant is baked into Struts to drift. + +**One-time guard:** the caller wraps `verifyFileUploadApi(JakartaServletDiskFileUpload.class)` with a +JVM-once gate (`static volatile boolean` or a holder). The probe method itself is stateless so tests +can call it repeatedly. + +## Testing & verification + +**Part A:** +- `mvn validate -DskipAssembly` runs the enforcer clean on the current tree (no fileupload skew + exists today). +- Manual negative check: temporarily declare a conflicting `commons-fileupload2-core` version and + confirm the build fails with the banned-dependency message; revert. + +**Part B (unit tests in `AbstractMultiPartRequestTest`):** +- `verifyFileUploadApi(JakartaServletDiskFileUpload.class)` does **not** throw (real classpath has the + M5 API). +- `verifyFileUploadApi()` throws `StrutsException`; assert the message + names the missing method and the remediation (align `-core` with `-jakarta-servlet6`). + +Full suite: `mvn test -DskipAssembly -pl core`. + +## Risks + +- **Enforcer noise (mitigated).** Scoping `bannedDependencies` to commons-fileupload2 only avoids the + unbounded scope risk of global `dependencyConvergence`. +- **Reflective probe drift.** If a future commons-fileupload2 release renames these setters again, the + probe's hardcoded method names become a deliberate tripwire to update alongside the dependency bump + — acceptable and intended. +- **Null implementation version.** Handled via `"unknown"` fallback so the guard never NPEs while + building its diagnostic message. + +## Out of scope / follow-ups + +- Tracked under [WW-5632](https://issues.apache.org/jira/browse/WW-5632). +- Global `dependencyConvergence` cleanup across the reactor. +- Exporting third-party versions through `struts2-bom`. +- Revisiting the dependency once commons-fileupload2 2.0.0 GA ships. + +## JIRA ticket + +**Summary (title):** + +``` +Harden commons-fileupload2 dependency against milestone binary-incompatibility +``` + +**Description (JIRA wiki markup — paste into the description field):** + +``` +h3. Background + +[WW-5615|https://issues.apache.org/jira/browse/WW-5615] fixed the {{NoSuchMethodError}} +caused by Apache Commons FileUpload 2.0.0-M5 renaming {{setSizeMax}} -> {{setMaxSize}} +(and friends), shipped in 7.2.0 via [#1584|https://github.com/apache/struts/pull/1584]. +That fix addressed the *symptom* for one milestone bump but not the underlying *class of +failure*. + +h3. Problem + +Struts depends on _milestone_ ({{-M}}) builds of commons-fileupload2, which break binary +compatibility between milestones. Three gaps remain on {{main}}: + +* The renamed setters ({{setMaxSize}}, {{setMaxFileCount}}, {{setMaxFileSize}}) live in + *{{commons-fileupload2-core}}* ({{AbstractFileUpload}}), but only + {{commons-fileupload2-jakarta-servlet6}} is pinned in {{dependencyManagement}} — + {{-core}} is unmanaged, so a transitive dependency can pull a mismatched {{-core}} + milestone and reproduce the {{NoSuchMethodError}}. +* The {{maven-enforcer-plugin}} {{dependencyConvergence}} rule sits only in + {{}} and is never bound to an active {{}} section, so it + never runs — the build cannot catch a fileupload version skew. +* Downstream consumer runtimes get an opaque, deep-stack {{NoSuchMethodError}} with no + actionable guidance. + +h3. Proposed changes + +* *(A1)* Introduce a single {{commons-fileupload2.version}} property and manage *both* + {{commons-fileupload2-core}} and {{commons-fileupload2-jakarta-servlet6}} at that version + in {{parent/pom.xml}}, forcing a matched {{-core}} version across the reactor. +* *(A2)* Activate {{maven-enforcer-plugin}} with a fileupload-scoped {{bannedDependencies}} + rule that fails the build on any commons-fileupload2 version other than the pinned one + (narrow scope, near-zero blast radius). +* *(B)* Add a once-per-JVM reflective guard in {{AbstractMultiPartRequest}} that throws a + clear {{StrutsException}} reporting the {{-core}} vs {{-jakarta-servlet6}} version skew + instead of an opaque {{NoSuchMethodError}}. + +Full design: {{docs/superpowers/specs/2026-06-10-fileupload2-milestone-hardening-design.md}} + +h3. Affects / Fix version + +* Affects: 7.1.1+ (root cause present on 7.2.0-SNAPSHOT {{main}}) +* Component: File Upload +``` + diff --git a/parent/pom.xml b/parent/pom.xml index 74325b8ba1..b5f2e4a30b 100644 --- a/parent/pom.xml +++ b/parent/pom.xml @@ -125,10 +125,15 @@ commons-collections4 4.5.0 + + org.apache.commons + commons-fileupload2-core + ${commons-fileupload2.version} + org.apache.commons commons-fileupload2-jakarta-servlet6 - 2.0.0-M5 + ${commons-fileupload2.version} commons-io diff --git a/pom.xml b/pom.xml index 2c4b9ade6a..0ea54064e6 100644 --- a/pom.xml +++ b/pom.xml @@ -116,6 +116,7 @@ 9.10.1 1.18.8 + 2.0.0-M5 2.3.34 8.0.2.Final 2.21.3 @@ -348,7 +349,17 @@ enforce - + + commons-fileupload2 version skew detected: only ${commons-fileupload2.version} is allowed. Align all commons-fileupload2 artifacts (core and jakarta-servlet6) to the version defined by the commons-fileupload2.version property in the root POM. + + org.apache.commons:commons-fileupload2-core + org.apache.commons:commons-fileupload2-jakarta-servlet6 + + + org.apache.commons:commons-fileupload2-core:${commons-fileupload2.version} + org.apache.commons:commons-fileupload2-jakarta-servlet6:${commons-fileupload2.version} + + @@ -376,6 +387,10 @@ maven-release-plugin 3.3.1 + + org.apache.maven.plugins + maven-enforcer-plugin + maven-jar-plugin