From a203f96d5dee4a40cd221f213ef1a72dd3bbeaf0 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Wed, 22 Jul 2026 09:56:12 +0200 Subject: [PATCH 01/13] WW-5413 docs(core): design for in-memory multipart upload optimization Lazy-materializing UploadedFile plus a new getInputStream() accessor so small (in-memory) uploads no longer eagerly write a temp file, while getContent() keeps returning a File for backward compatibility. Co-Authored-By: Claude Opus 4.8 --- ...413-inmemory-upload-optimization-design.md | 126 ++++++++++++++++++ 1 file changed, 126 insertions(+) create mode 100644 docs/superpowers/specs/2026-07-22-WW-5413-inmemory-upload-optimization-design.md diff --git a/docs/superpowers/specs/2026-07-22-WW-5413-inmemory-upload-optimization-design.md b/docs/superpowers/specs/2026-07-22-WW-5413-inmemory-upload-optimization-design.md new file mode 100644 index 0000000000..b7ef076cb5 --- /dev/null +++ b/docs/superpowers/specs/2026-07-22-WW-5413-inmemory-upload-optimization-design.md @@ -0,0 +1,126 @@ +# WW-5413 — In-memory multipart upload optimization + +**Jira:** [WW-5413](https://issues.apache.org/jira/browse/WW-5413) · **Fix version:** 7.3.0 · **Component:** Core +**Date:** 2026-07-22 + +## Background + +WW-5413 was originally filed against 6.3.0: commons-io 2.16.0 broke `DeferredFileOutputStream`/`ThresholdingOutputStream`, cascading through commons-fileupload's `DiskFileItem` so multipart uploads were read as empty. A prior Struts workaround forced the disk-spill threshold to `-1`, spilling *every* field to disk. The ticket proposed dropping the `-1` threshold, handling the `isInMemory()` case properly, and avoiding unnecessary filesystem writes. + +**The root-cause bug is already resolved on `main`.** Struts has since migrated to **commons-fileupload2 2.0.0-M5** + **commons-io 2.22.0**. `AbstractMultiPartRequest.createJakartaFileUpload()` sets no threshold, so it uses `DiskFileItemFactory`'s default (~8 KB); small uploads legitimately stay in memory (`isInMemory() == true`). + +**What remains** is the performance half of the ticket. `JakartaMultiPartRequest.processFileField()` still takes every in-memory item and eagerly writes it to a temp file via `FileOutputStream`, then wraps that `File` in `StrutsUploadedFile`. The redundant filesystem write the ticket complained about is still present — it just moved from the fileupload layer into Struts' own code, because `UploadedFile`/`StrutsUploadedFile` are `File`-backed with no in-memory representation. + +## Goal + +Eliminate the redundant temp-file write for small (in-memory) uploads, while keeping 100% backward compatibility for existing `UploadedFile` consumers — including the legacy `File`-typed action property path and third-party `UploadedFile` implementations. + +## Constraints & compatibility facts + +- `UploadedFile` was designed for this: `isFile()` documents "real file or maybe just in-memory stream", `getContent()` returns `Object`, `getAbsolutePath()` is "if possible". `UploadedFile extends Serializable`. +- `getContent()` de-facto returns a `java.io.File` everywhere today: `UploadedFileConverter` (legacy `File`-typed action properties), `apps/showcase` actions (`FileUploadAction.getContent()`), and user actions in the wild. **The runtime type of `getContent()` must remain `File`** — a size-dependent `byte[]`/`File` return would break these consumers non-deterministically. This is why we do **not** expose bytes through `getContent()`. +- `AbstractMultiPartRequest.cleanUp()` deletes an uploaded file by calling `UploadedFile.delete()` **only when `isFile()` is true**. The lazy design plugs into this existing hook. +- The in-memory `DiskFileItem` buffer stays valid until `cleanUp()` runs (after action processing), so reading `item.get()` at parse time is safe. + +## Approach: lazy materialization + a streaming accessor + +Keep `getContent()`/`getAbsolutePath()` returning a `File` (materialized on demand), and add a new, correctly-typed door for reading bytes without forcing a disk write. + +### 1. `UploadedFile` interface — new `getInputStream()` + +Add one method as a **`default`** so existing third-party implementations keep compiling: + +```java +default InputStream getInputStream() throws IOException { + Object c = getContent(); + if (c instanceof File f) return new FileInputStream(f); + if (c instanceof byte[] b) return new ByteArrayInputStream(b); + throw new IOException("No content stream available for " + getName()); +} +``` + +This is the type-safe "give me the bytes without forcing a file" path. `getContent()` still returns a `File` for every implementation, so no existing consumer changes. + +### 2. New `StrutsInMemoryUploadedFile` + +A second `UploadedFile` implementation beside `StrutsUploadedFile` — each class stays single-purpose; `Struts*` naming per project convention. It holds: + +- `byte[] content` — the small upload's bytes, from `item.get()` +- `Path saveDir` — where a temp file will be written if ever demanded +- a **stable temp-file name chosen at construction** (`upload_.tmp`) — only the *write* is deferred, so `getName()` is stable and matches the eventual file +- metadata: `contentType`, `originalName`, `inputName` +- `transient File materializedFile` — populated lazily, cached + +`byte[]` + `Path` keep the object `Serializable` (a `DiskFileItem` reference would not). + +| Method | Behavior | Touches disk? | +|---|---|---| +| `getInputStream()` | `new ByteArrayInputStream(content)` | **No** | +| `length()` | `content.length` | No | +| `getName()` | the pre-chosen `upload_.tmp` | No | +| `getContent()` | `materialize()` → returns the `File` | **Yes, once** (cached) | +| `getAbsolutePath()` | `materialize()` → path string | **Yes, once** (cached) | +| `isFile()` | true only *after* materialization | No | +| `getContentType()` / `getOriginalName()` / `getInputName()` | metadata | No | +| `delete()` | deletes the materialized file if it exists; no-op otherwise | — | + +`materialize()` is `synchronized`, writes the bytes once to the pre-chosen path in `saveDir`, and caches the resulting `File`. The temp file is created with the project's secure UUID-named pattern (`upload_.tmp`); the naming logic is shared with / extracted alongside `AbstractMultiPartRequest.createTemporaryFile` to avoid divergence. + +**Net effect:** rejected uploads, size/type checks (`length()`), and `getInputStream()` consumers **never write**; legacy `File`/`getContent()` consumers write exactly once — same as today, but deferred. + +### 3. `JakartaMultiPartRequest.processFileField` + cleanup simplification + +The `item.isInMemory()` branch stops writing a temp file eagerly: + +```java +if (item.isInMemory()) { + values.add(StrutsInMemoryUploadedFile.Builder + .create(item.get(), Path.of(saveDir)) + .withOriginalName(item.getName()) + .withContentType(item.getContentType()) + .withInputName(item.getFieldName()) + .build()); +} else { + // unchanged File-based path via item.getPath() +} +``` + +Because in-memory files no longer create temp files eagerly: + +- the `temporaryFiles` field, `cleanUpTemporaryFiles()`, and the `FileOutputStream` write block in `JakartaMultiPartRequest` are removed; +- cleanup of any *materialized* file happens through the existing `AbstractMultiPartRequest.cleanUp()` loop, which already calls `delete()` when `isFile()` is true — no new cleanup path is added; +- `createTemporaryFile` remains in `AbstractMultiPartRequest` (`JakartaStreamMultiPartRequest` still uses it). + +`StrutsUploadedFile` keeps its current `File`-backed behavior; it may override `getInputStream()` to return `new FileInputStream(file)` directly rather than relying on the interface default. + +`JakartaStreamMultiPartRequest` is unchanged: it always streams to disk (no in-memory threshold) and stays `File`-backed via `StrutsUploadedFile`, inheriting `getInputStream()` for free. + +### 4. Error-handling behavior change (explicit) + +`getContent()`/`getAbsolutePath()` have no `throws` clause, so a materialization **write failure** surfaces as an unchecked `StrutsException` **during consumption**, rather than as a gracefully-collected `LocalizedMessage` at parse time (today's behavior). This affects only the rare "cannot write to `saveDir`" case, and only on the legacy `File`/`getContent()` path — the `getInputStream()` fast path never writes. Accepted as a reasonable trade for the optimization. + +## Testing + +**`StrutsInMemoryUploadedFile` unit tests** + +- `getInputStream()` returns the exact bytes and creates **no** file on disk +- `getContent()` and `getAbsolutePath()` create the temp file **exactly once** and cache it (second call reuses the same `File`) +- `isFile()` is false before materialization, true after +- `length()` and metadata accessors do **not** materialize +- `delete()` removes a materialized file and is a safe no-op when nothing was materialized + +**`JakartaMultiPartRequest` tests** + +- a small (in-memory) upload leaves **no** temp file on disk after `parse()`, until `getContent()`/`getAbsolutePath()` is called +- content is readable via both `getInputStream()` and `getContent()` +- a large (on-disk) upload keeps the existing `File`-backed path unchanged +- legacy `File`-typed action-property conversion (`UploadedFileConverter`) still works for a small upload +- `cleanUp()` removes any materialized file and leaves nothing behind + +Tests follow the existing multipart test base (core tests are JUnit4 / `XWorkTestCase`-style — an `@Test` added to a `TestCase` subclass silently never runs). + +## Out of scope + +- Migrating existing consumers (showcase actions, converter) onto `getInputStream()` — they continue using `getContent()`. +- Changing the disk-spill threshold or `JakartaStreamMultiPartRequest` streaming strategy. +- Any change to `getContent()`'s runtime type (must remain `File`). From 9bef77ee437e5f73373fe08a322b23dbde5871bd Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Wed, 22 Jul 2026 10:08:11 +0200 Subject: [PATCH 02/13] WW-5413 docs(core): implementation plan for in-memory upload optimization Co-Authored-By: Claude Opus 4.8 --- ...22-WW-5413-inmemory-upload-optimization.md | 809 ++++++++++++++++++ 1 file changed, 809 insertions(+) create mode 100644 docs/superpowers/plans/2026-07-22-WW-5413-inmemory-upload-optimization.md diff --git a/docs/superpowers/plans/2026-07-22-WW-5413-inmemory-upload-optimization.md b/docs/superpowers/plans/2026-07-22-WW-5413-inmemory-upload-optimization.md new file mode 100644 index 0000000000..1a98bb67f7 --- /dev/null +++ b/docs/superpowers/plans/2026-07-22-WW-5413-inmemory-upload-optimization.md @@ -0,0 +1,809 @@ +# WW-5413 In-memory Multipart Upload Optimization — 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:** Stop eagerly writing small (in-memory) multipart uploads to a temporary file; keep them in memory and materialize a file only when a caller demands one, while `getContent()` still returns a `java.io.File` for full backward compatibility. + +**Architecture:** Add a `default InputStream getInputStream()` to the `UploadedFile` interface (the type-safe, no-disk read path). Introduce `StrutsInMemoryUploadedFile`, a byte-array-backed `UploadedFile` that lazily writes a temp file only on `getContent()`/`getAbsolutePath()`. Rewire `JakartaMultiPartRequest.processFileField()` to build it for `item.isInMemory()`, and drop the now-obsolete eager-write / `temporaryFiles` bookkeeping (cleanup rides the existing `AbstractMultiPartRequest.cleanUp()` `delete()` loop). + +**Tech Stack:** Java (Struts core module), Apache Commons FileUpload2 2.0.0-M5, JUnit 4 + AssertJ, Maven. + +## Global Constraints + +- **Commit prefix:** every commit message MUST start with `WW-5413` followed by a conventional type, e.g. `WW-5413 feat(core): ...`. +- **`getContent()` runtime type MUST remain `java.io.File`** for every `UploadedFile` implementation — never return `byte[]` from it. The bytes-without-a-file path is `getInputStream()` only. +- **Core tests are JUnit 4** (`org.junit.Test`, AssertJ). Do NOT use JUnit 5. New standalone test classes must NOT extend `XWorkTestCase` (irrelevant here) — plain JUnit 4 classes run fine under Surefire. +- **Secure temp-file naming:** materialized files MUST use the pattern `upload_.tmp` in the provided save directory; the user-supplied original filename MUST NOT influence the on-disk name. +- **Backward compatibility:** `UploadedFile.getInputStream()` MUST be a `default` method so third-party implementations keep compiling. +- **Build/test command:** `mvn test -DskipAssembly -pl core -Dtest=ClassName#methodName` (single test) or `-Dtest=ClassName` (whole class). + +--- + +## File Structure + +- `core/src/main/java/org/apache/struts2/dispatcher/multipart/UploadedFile.java` — **modify**: add `default InputStream getInputStream()`. +- `core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsUploadedFile.java` — **modify**: override `getInputStream()` to stream the backing file. +- `core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFile.java` — **create**: byte-array-backed, lazily-materializing `UploadedFile`. +- `core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java` — **modify**: build `StrutsInMemoryUploadedFile` for in-memory items; remove eager write, `temporaryFiles`, `cleanUpTemporaryFiles()`. +- `core/src/test/java/org/apache/struts2/dispatcher/multipart/UploadedFileTest.java` — **create**: interface default-method test. +- `core/src/test/java/org/apache/struts2/dispatcher/multipart/StrutsUploadedFileTest.java` — **create**: `getInputStream()` override test. +- `core/src/test/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFileTest.java` — **create**: full lazy-materialization unit tests. +- `core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequestTest.java` — **modify**: remove 5 tests that reference removed internals; add integration tests for the new behavior. + +--- + +## Task 1: Streaming accessor on the File-backed path + +Adds `getInputStream()` to the interface (default) and overrides it in the existing `File`-backed implementation. + +**Files:** +- Modify: `core/src/main/java/org/apache/struts2/dispatcher/multipart/UploadedFile.java` +- Modify: `core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsUploadedFile.java` +- Test: `core/src/test/java/org/apache/struts2/dispatcher/multipart/UploadedFileTest.java` (create) +- Test: `core/src/test/java/org/apache/struts2/dispatcher/multipart/StrutsUploadedFileTest.java` (create) + +**Interfaces:** +- Produces: `UploadedFile.getInputStream() throws IOException` returning an `InputStream` over the file's bytes. Default impl: `File` content → `FileInputStream`; `byte[]` content → `ByteArrayInputStream`; otherwise `IOException`. + +- [ ] **Step 1: Write the failing interface default-method test** + +Create `core/src/test/java/org/apache/struts2/dispatcher/multipart/UploadedFileTest.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.junit.Test; + +import java.io.IOException; +import java.io.InputStream; + +import static java.nio.charset.StandardCharsets.UTF_8; +import static org.assertj.core.api.Assertions.assertThat; + +public class UploadedFileTest { + + @Test + public void defaultGetInputStreamReadsByteArrayContent() throws IOException { + UploadedFile file = new UploadedFile() { + public Long length() { return 3L; } + public String getName() { return "x"; } + public String getOriginalName() { return "x"; } + public boolean isFile() { return false; } + public boolean delete() { return true; } + public String getAbsolutePath() { return null; } + public Object getContent() { return "abc".getBytes(UTF_8); } + public String getContentType() { return "text/plain"; } + public String getInputName() { return "file"; } + }; + + try (InputStream in = file.getInputStream()) { + assertThat(new String(in.readAllBytes(), UTF_8)).isEqualTo("abc"); + } + } +} +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `mvn test -DskipAssembly -pl core -Dtest=UploadedFileTest` +Expected: COMPILE FAILURE — `getInputStream()` is not defined on `UploadedFile`. + +- [ ] **Step 3: Add the default method to the interface** + +In `UploadedFile.java`, add these imports below `import java.io.Serializable;`: + +```java +import java.io.ByteArrayInputStream; +import java.io.File; +import java.io.FileInputStream; +import java.io.IOException; +import java.io.InputStream; +``` + +Add this method inside the interface (e.g. after `getInputName()`): + +```java + /** + * Streams the uploaded content without forcing it to disk. Implementations backed by + * in-memory bytes can return the bytes directly; file-backed implementations stream the + * file. The default reads whatever {@link #getContent()} exposes. + * + * @return an input stream over the uploaded content + * @throws IOException if the content cannot be read + * @since 7.3.0 + */ + default InputStream getInputStream() throws IOException { + Object content = getContent(); + if (content instanceof File file) { + return new FileInputStream(file); + } + if (content instanceof byte[] bytes) { + return new ByteArrayInputStream(bytes); + } + throw new IOException("No content stream available for " + getName()); + } +``` + +- [ ] **Step 4: Run test to verify it passes** + +Run: `mvn test -DskipAssembly -pl core -Dtest=UploadedFileTest` +Expected: PASS. + +- [ ] **Step 5: Write the failing StrutsUploadedFile override test** + +Create `core/src/test/java/org/apache/struts2/dispatcher/multipart/StrutsUploadedFileTest.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.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; + +import java.io.File; +import java.io.IOException; +import java.io.InputStream; +import java.nio.file.Files; + +import static java.nio.charset.StandardCharsets.UTF_8; +import static org.assertj.core.api.Assertions.assertThat; + +public class StrutsUploadedFileTest { + + @Rule + public TemporaryFolder tempFolder = new TemporaryFolder(); + + @Test + public void getInputStreamReadsFileContent() throws IOException { + File backing = tempFolder.newFile("upload_test.tmp"); + Files.writeString(backing.toPath(), "hello"); + + UploadedFile file = StrutsUploadedFile.Builder.create(backing).build(); + + try (InputStream in = file.getInputStream()) { + assertThat(new String(in.readAllBytes(), UTF_8)).isEqualTo("hello"); + } + } +} +``` + +- [ ] **Step 6: Run test to verify it passes via the interface default** + +Run: `mvn test -DskipAssembly -pl core -Dtest=StrutsUploadedFileTest` +Expected: PASS (the default method already handles the `File` branch). + +- [ ] **Step 7: Add an explicit override in StrutsUploadedFile** + +In `StrutsUploadedFile.java`, replace the import block: + +```java +import java.io.File; +``` + +with: + +```java +import java.io.File; +import java.io.FileInputStream; +import java.io.IOException; +import java.io.InputStream; +``` + +Add this method (e.g. after `getContent()`): + +```java + @Override + public InputStream getInputStream() throws IOException { + return new FileInputStream(file); + } +``` + +- [ ] **Step 8: Run both tests to verify they pass** + +Run: `mvn test -DskipAssembly -pl core -Dtest=UploadedFileTest,StrutsUploadedFileTest` +Expected: PASS. + +- [ ] **Step 9: Commit** + +```bash +git add core/src/main/java/org/apache/struts2/dispatcher/multipart/UploadedFile.java \ + core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsUploadedFile.java \ + core/src/test/java/org/apache/struts2/dispatcher/multipart/UploadedFileTest.java \ + core/src/test/java/org/apache/struts2/dispatcher/multipart/StrutsUploadedFileTest.java +git commit -m "WW-5413 feat(core): add UploadedFile.getInputStream() streaming accessor" +``` + +--- + +## Task 2: `StrutsInMemoryUploadedFile` (lazy materialization) + +The byte-array-backed implementation. This is the core of the optimization. + +**Files:** +- Create: `core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFile.java` +- Test: `core/src/test/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFileTest.java` (create) + +**Interfaces:** +- Consumes: `UploadedFile` (incl. `getInputStream()` from Task 1), `org.apache.struts2.StrutsException`. +- Produces: + - `StrutsInMemoryUploadedFile.Builder.create(byte[] content, java.nio.file.Path saveDir)` → `Builder` + - `Builder.withContentType(String).withOriginalName(String).withInputName(String).build()` → `UploadedFile` + - Behavior: `getInputStream()` → `ByteArrayInputStream` (no disk); `getContent()`/`getAbsolutePath()` write once to `saveDir/upload_.tmp` and cache; `isFile()` false until materialized; `getName()` returns the pre-chosen `upload_.tmp`; `delete()` removes the materialized file (no-op, returns `true`, if never materialized). + +- [ ] **Step 1: Write the failing unit tests** + +Create `core/src/test/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFileTest.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.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; + +import java.io.File; +import java.io.IOException; +import java.io.InputStream; +import java.nio.file.Path; + +import static java.nio.charset.StandardCharsets.UTF_8; +import static org.assertj.core.api.Assertions.assertThat; + +public class StrutsInMemoryUploadedFileTest { + + @Rule + public TemporaryFolder tempFolder = new TemporaryFolder(); + + private Path saveDir; + + @Before + public void setUp() { + saveDir = tempFolder.getRoot().toPath(); + } + + private UploadedFile build(byte[] content) { + return StrutsInMemoryUploadedFile.Builder + .create(content, saveDir) + .withOriginalName("orig.txt") + .withContentType("text/plain") + .withInputName("file") + .build(); + } + + @Test + public void getInputStreamReturnsBytesWithoutWritingFile() throws IOException { + UploadedFile file = build("hello".getBytes(UTF_8)); + + try (InputStream in = file.getInputStream()) { + assertThat(new String(in.readAllBytes(), UTF_8)).isEqualTo("hello"); + } + + assertThat(file.isFile()).isFalse(); + assertThat(tempFolder.getRoot().listFiles()).isEmpty(); + } + + @Test + public void getContentMaterializesFileExactlyOnce() { + UploadedFile file = build("data".getBytes(UTF_8)); + + File first = (File) file.getContent(); + File second = (File) file.getContent(); + + assertThat(first).exists().hasContent("data"); + assertThat(second).isSameAs(first); + assertThat(tempFolder.getRoot().listFiles()).hasSize(1); + } + + @Test + public void getAbsolutePathMaterializesFile() { + UploadedFile file = build("data".getBytes(UTF_8)); + + String path = file.getAbsolutePath(); + + assertThat(new File(path)).exists().hasContent("data"); + assertThat(file.isFile()).isTrue(); + } + + @Test + public void isFileFalseBeforeMaterializationTrueAfter() { + UploadedFile file = build("x".getBytes(UTF_8)); + + assertThat(file.isFile()).isFalse(); + file.getContent(); + assertThat(file.isFile()).isTrue(); + } + + @Test + public void lengthAndMetadataDoNotMaterialize() { + UploadedFile file = build("abcd".getBytes(UTF_8)); + + assertThat(file.length()).isEqualTo(4L); + assertThat(file.getContentType()).isEqualTo("text/plain"); + assertThat(file.getOriginalName()).isEqualTo("orig.txt"); + assertThat(file.getInputName()).isEqualTo("file"); + assertThat(file.getName()).startsWith("upload_").endsWith(".tmp"); + + assertThat(file.isFile()).isFalse(); + assertThat(tempFolder.getRoot().listFiles()).isEmpty(); + } + + @Test + public void deleteRemovesMaterializedFile() { + UploadedFile file = build("x".getBytes(UTF_8)); + File materialized = (File) file.getContent(); + assertThat(materialized).exists(); + + assertThat(file.delete()).isTrue(); + assertThat(materialized).doesNotExist(); + } + + @Test + public void deleteIsNoOpWhenNotMaterialized() { + UploadedFile file = build("x".getBytes(UTF_8)); + + assertThat(file.delete()).isTrue(); + assertThat(tempFolder.getRoot().listFiles()).isEmpty(); + } + + @Test + public void maliciousOriginalNameDoesNotLeakIntoTempName() { + UploadedFile file = StrutsInMemoryUploadedFile.Builder + .create("x".getBytes(UTF_8), saveDir) + .withOriginalName("../../etc/passwd") + .build(); + + file.getContent(); // materialize + + assertThat(file.getName()).startsWith("upload_").endsWith(".tmp"); + assertThat(file.getName()).doesNotContain("..").doesNotContain("/").doesNotContain("\\"); + assertThat(new File(saveDir.toFile(), file.getName())).exists(); + } +} +``` + +- [ ] **Step 2: Run tests to verify they fail** + +Run: `mvn test -DskipAssembly -pl core -Dtest=StrutsInMemoryUploadedFileTest` +Expected: COMPILE FAILURE — `StrutsInMemoryUploadedFile` does not exist. + +- [ ] **Step 3: Implement `StrutsInMemoryUploadedFile`** + +Create `core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFile.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.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; +import org.apache.struts2.StrutsException; + +import java.io.ByteArrayInputStream; +import java.io.File; +import java.io.IOException; +import java.io.InputStream; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.UUID; + +/** + * In-memory backed {@link UploadedFile} for small multipart uploads that Commons FileUpload kept + * in memory ({@code DiskFileItem.isInMemory() == true}). + * + *

The content is held as a byte array and is written to a temporary file only the first time a + * caller demands a {@link File} through {@link #getContent()} or {@link #getAbsolutePath()} (lazy + * materialization). Callers reading through {@link #getInputStream()} never touch the disk. The + * temporary file uses the secure {@code upload_.tmp} naming and ignores the user-supplied + * original filename.

+ * + * @since 7.3.0 + */ +public class StrutsInMemoryUploadedFile implements UploadedFile { + + private static final Logger LOG = LogManager.getLogger(StrutsInMemoryUploadedFile.class); + + private final byte[] content; + private final Path saveDir; + private final String name; + private final String contentType; + private final String originalName; + private final String inputName; + + private transient File materializedFile; + + private StrutsInMemoryUploadedFile(byte[] content, Path saveDir, String contentType, + String originalName, String inputName) { + this.content = content; + this.saveDir = saveDir; + this.contentType = contentType; + this.originalName = originalName; + this.inputName = inputName; + this.name = "upload_" + UUID.randomUUID().toString().replace("-", "_") + ".tmp"; + } + + private synchronized File materialize() { + if (materializedFile == null) { + File target = saveDir.resolve(name).toFile(); + try { + Files.write(target.toPath(), content); + } catch (IOException e) { + throw new StrutsException("Could not materialize in-memory uploaded file: " + name, e); + } + materializedFile = target; + LOG.debug("Materialized in-memory uploaded item to {}", target.getAbsolutePath()); + } + return materializedFile; + } + + @Override + public InputStream getInputStream() { + return new ByteArrayInputStream(content); + } + + @Override + public Long length() { + return (long) content.length; + } + + @Override + public String getName() { + return name; + } + + @Override + public boolean isFile() { + return materializedFile != null && materializedFile.isFile(); + } + + @Override + public boolean delete() { + if (materializedFile != null) { + return materializedFile.delete(); + } + return true; + } + + @Override + public String getAbsolutePath() { + return materialize().getAbsolutePath(); + } + + @Override + public File getContent() { + return materialize(); + } + + @Override + public String getContentType() { + return contentType; + } + + @Override + public String getOriginalName() { + return originalName; + } + + @Override + public String getInputName() { + return inputName; + } + + @Override + public String toString() { + return "StrutsInMemoryUploadedFile{" + + "contentType='" + contentType + '\'' + + ", originalName='" + originalName + '\'' + + ", inputName='" + inputName + '\'' + + ", size=" + content.length + + '}'; + } + + public static class Builder { + private final byte[] content; + private final Path saveDir; + private String contentType; + private String originalName; + private String inputName; + + private Builder(byte[] content, Path saveDir) { + this.content = content; + this.saveDir = saveDir; + } + + public static Builder create(byte[] content, Path saveDir) { + return new Builder(content, saveDir); + } + + public Builder withContentType(String contentType) { + this.contentType = contentType; + return this; + } + + public Builder withOriginalName(String originalName) { + this.originalName = originalName; + return this; + } + + public Builder withInputName(String inputName) { + this.inputName = inputName; + return this; + } + + public UploadedFile build() { + return new StrutsInMemoryUploadedFile(content, saveDir, contentType, originalName, inputName); + } + } +} +``` + +> Note: `org.apache.struts2.StrutsException` is the same exception class used in `AbstractMultiPartRequest`. Confirm the import resolves; it is an unchecked `RuntimeException`. + +- [ ] **Step 4: Run tests to verify they pass** + +Run: `mvn test -DskipAssembly -pl core -Dtest=StrutsInMemoryUploadedFileTest` +Expected: PASS (all 8 tests). + +- [ ] **Step 5: Commit** + +```bash +git add core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFile.java \ + core/src/test/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFileTest.java +git commit -m "WW-5413 feat(core): add lazily-materializing StrutsInMemoryUploadedFile" +``` + +--- + +## Task 3: Rewire `JakartaMultiPartRequest` and clean up obsolete internals + +Switch the in-memory branch to `StrutsInMemoryUploadedFile`, remove the eager write and `temporaryFiles` bookkeeping, and update the existing tests that reference the removed internals so the suite compiles and passes. + +**Files:** +- Modify: `core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java` +- Modify: `core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequestTest.java` + +**Interfaces:** +- Consumes: `StrutsInMemoryUploadedFile.Builder` (Task 2), existing `StrutsUploadedFile.Builder`. +- Produces: `JakartaMultiPartRequest.processFileField(DiskFileItem, String)` no longer creates temp files eagerly; the `temporaryFiles` field and `cleanUpTemporaryFiles()` method are removed. + +- [ ] **Step 1: Replace the in-memory branch in `processFileField`** + +In `JakartaMultiPartRequest.java`, replace the entire `if (item.isInMemory()) { ... } else { ... }` block (the eager `FileOutputStream` write path) with: + +```java + if (item.isInMemory()) { + LOG.debug(() -> "Keeping in-memory uploaded item without writing to disk: " + normalizeSpace(item.getFieldName())); + UploadedFile uploadedFile = StrutsInMemoryUploadedFile.Builder + .create(item.get(), Path.of(saveDir)) + .withOriginalName(item.getName()) + .withContentType(item.getContentType()) + .withInputName(item.getFieldName()) + .build(); + values.add(uploadedFile); + } else { + UploadedFile uploadedFile = StrutsUploadedFile.Builder + .create(item.getPath().toFile()) + .withOriginalName(item.getName()) + .withContentType(item.getContentType()) + .withInputName(item.getFieldName()) + .build(); + values.add(uploadedFile); + } +``` + +- [ ] **Step 2: Remove the `temporaryFiles` field** + +Delete this field and its Javadoc (the block around lines 83–86): + +```java + /** + * List to track temporary files created for in-memory uploads + */ + private final List temporaryFiles = new ArrayList<>(); +``` + +- [ ] **Step 3: Remove `cleanUpTemporaryFiles()` and its call** + +Delete the entire `cleanUpTemporaryFiles()` method (its Javadoc + body). In `cleanUp()`, remove the `cleanUpTemporaryFiles();` line and the `temporaryFiles.clear();` line, leaving: + +```java + @Override + public void cleanUp() { + super.cleanUp(); + try { + cleanUpDiskFileItems(); + } finally { + diskFileItems.clear(); + } + } +``` + +- [ ] **Step 4: Fix imports** + +Remove `import java.io.File;` (no longer used in this class). Leave `java.nio.file.Path` (used by `Path.of(saveDir)`). If the compiler reports any other now-unused import (e.g. `java.nio.file.Files`), remove it too. Keep `StringUtils` (still used by `processNormalFormField`). + +- [ ] **Step 5: Remove the 5 obsolete tests** + +In `JakartaMultiPartRequestTest.java`, delete these test methods entirely — they reference the removed `temporaryFiles` field / `cleanUpTemporaryFiles()` method, or assert the removed parse-time eager-write behavior. Their coverage is replaced by Task 2 (secure naming, materialization) and Task 4 (no eager write, cleanup): + - `temporaryFileCleanupForInMemoryUploads` + - `cleanupMethodsCanBeOverridden` + - `temporaryFileCreationFailureAddsError` + - `temporaryFilesCreatedInSaveDirectory` + - `secureTemporaryFileNaming` + +Leave all other tests untouched (`temporaryFileCreationErrorsAreNotDuplicated`, `cleanupIsIdempotent`, `endToEndMultipartProcessingWithCleanup`, `inMemoryVsDiskFileHandling`, `processNormalFormFieldHandlesNullFieldName`, `processFileFieldHandlesNullFieldName`, `diskFileItemCleanupCoverage`, `errorDuplicationPrevention`, `processFileFieldHandlesEmptyFileName`). After deleting, remove any imports left unused by the deletions (e.g. `java.lang.reflect.Field` if no longer referenced). + +- [ ] **Step 6: Run the whole class to verify it compiles and passes** + +Run: `mvn test -DskipAssembly -pl core -Dtest=JakartaMultiPartRequestTest` +Expected: PASS. In particular `inMemoryVsDiskFileHandling` still passes — its small-file assertion goes through `getContent()`, which now materializes the file on demand and returns a `File`. + +- [ ] **Step 7: Commit** + +```bash +git add core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java \ + core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequestTest.java +git commit -m "WW-5413 refactor(core): drop eager temp-file write for in-memory uploads" +``` + +--- + +## Task 4: Integration tests for the deferred-write behavior + +Prove end-to-end that in-memory uploads are not written to disk until content is demanded, are readable via the stream path without a write, and are cleaned up after materialization. + +**Files:** +- Modify: `core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequestTest.java` + +**Interfaces:** +- Consumes: the parse pipeline from Task 3, `UploadedFile.getInputStream()` from Task 1. + +- [ ] **Step 1: Add the imports the new test needs** + +In `JakartaMultiPartRequestTest.java`, ensure these imports are present (add any missing): + +```java +import java.io.InputStream; +``` + +(`java.io.File`, `java.nio.charset.StandardCharsets`, and `static org.assertj.core.api.Assertions.assertThat` are already imported.) + +- [ ] **Step 2: Write the failing integration test** + +Add this test method to `JakartaMultiPartRequestTest`: + +```java + @Test + public void inMemoryUploadIsNotWrittenToDiskUntilContentRequested() throws IOException { + // given - a small file that Commons FileUpload keeps in memory + String content = formFile("file1", "test1.csv", "a,b,c,d") + + endline + "--" + boundary + "--"; + mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8)); + + // when + multiPart.parse(mockRequest, tempDir); + + UploadedFile file = multiPart.getFile("file1")[0]; + + // then - nothing written to disk right after parse + assertThat(file.isFile()).isFalse(); + + // and - content is readable via the stream path without materializing + try (InputStream in = file.getInputStream()) { + assertThat(new String(in.readAllBytes(), StandardCharsets.UTF_8)).isEqualTo("a,b,c,d"); + } + assertThat(file.isFile()).isFalse(); + + // and - getContent() materializes a real file on demand + File materialized = (File) file.getContent(); + assertThat(materialized).exists().hasContent("a,b,c,d"); + assertThat(file.isFile()).isTrue(); + + // and - cleanUp removes the materialized file + multiPart.cleanUp(); + assertThat(materialized).doesNotExist(); + } +``` + +- [ ] **Step 3: Run it to verify it passes** + +Run: `mvn test -DskipAssembly -pl core -Dtest=JakartaMultiPartRequestTest#inMemoryUploadIsNotWrittenToDiskUntilContentRequested` +Expected: PASS. + +- [ ] **Step 4: Run the full multipart test package as a regression check** + +Run: `mvn test -DskipAssembly -pl core -Dtest='org.apache.struts2.dispatcher.multipart.*'` +Expected: PASS (all multipart tests, including `JakartaStreamMultiPartRequestTest` and `AbstractMultiPartRequestApiCheckTest`). + +- [ ] **Step 5: Commit** + +```bash +git add core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequestTest.java +git commit -m "WW-5413 test(core): cover deferred-write behavior for in-memory uploads" +``` + +--- + +## Final verification + +- [ ] **Run the core module's dispatcher tests** to catch any converter/interceptor fallout from the interface change: + +Run: `mvn test -DskipAssembly -pl core -Dtest='org.apache.struts2.dispatcher.**,org.apache.struts2.interceptor.**,org.apache.struts2.conversion.**'` +Expected: PASS. Pay attention to `UploadedFileConverter`-related tests (legacy `File`-typed action property path) — a small upload now reaches the converter as a `StrutsInMemoryUploadedFile` whose `getContent()` returns a materialized `File`, so conversion must still succeed. + +--- + +## Notes on the spec's error-handling trade-off (§4) + +The spec deliberately accepts that a materialization **write failure** now surfaces as an unchecked `StrutsException` during consumption (via `getContent()`/`getAbsolutePath()`), rather than as a parse-time `LocalizedMessage`. This is why the old `temporaryFileCreationFailureAddsError` test is removed rather than rewritten: the parse-time graceful-degradation path for in-memory items no longer exists. No new test asserts the exception, since it only fires on genuine filesystem failures in the save directory. From b1a86efbeedabb000909018bd38e7cf2f479e0e3 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Wed, 22 Jul 2026 10:11:20 +0200 Subject: [PATCH 03/13] WW-5413 feat(core): add UploadedFile.getInputStream() streaming accessor Co-Authored-By: Claude Opus 4.8 --- .../multipart/StrutsUploadedFile.java | 8 +++ .../dispatcher/multipart/UploadedFile.java | 25 ++++++++++ .../multipart/StrutsUploadedFileTest.java | 49 +++++++++++++++++++ .../multipart/UploadedFileTest.java | 49 +++++++++++++++++++ 4 files changed, 131 insertions(+) create mode 100644 core/src/test/java/org/apache/struts2/dispatcher/multipart/StrutsUploadedFileTest.java create mode 100644 core/src/test/java/org/apache/struts2/dispatcher/multipart/UploadedFileTest.java diff --git a/core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsUploadedFile.java b/core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsUploadedFile.java index 492b46fb05..418808945e 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsUploadedFile.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsUploadedFile.java @@ -19,6 +19,9 @@ package org.apache.struts2.dispatcher.multipart; import java.io.File; +import java.io.FileInputStream; +import java.io.IOException; +import java.io.InputStream; public class StrutsUploadedFile implements UploadedFile { @@ -64,6 +67,11 @@ public File getContent() { return file; } + @Override + public InputStream getInputStream() throws IOException { + return new FileInputStream(file); + } + @Override public String getContentType() { return this.contentType; diff --git a/core/src/main/java/org/apache/struts2/dispatcher/multipart/UploadedFile.java b/core/src/main/java/org/apache/struts2/dispatcher/multipart/UploadedFile.java index c0af59742a..e6d48a163a 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/multipart/UploadedFile.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/multipart/UploadedFile.java @@ -18,6 +18,11 @@ */ package org.apache.struts2.dispatcher.multipart; +import java.io.ByteArrayInputStream; +import java.io.File; +import java.io.FileInputStream; +import java.io.IOException; +import java.io.InputStream; import java.io.Serializable; /** @@ -74,4 +79,24 @@ public interface UploadedFile extends Serializable { */ String getInputName(); + /** + * Streams the uploaded content without forcing it to disk. Implementations backed by + * in-memory bytes can return the bytes directly; file-backed implementations stream the + * file. The default reads whatever {@link #getContent()} exposes. + * + * @return an input stream over the uploaded content + * @throws IOException if the content cannot be read + * @since 7.3.0 + */ + default InputStream getInputStream() throws IOException { + Object content = getContent(); + if (content instanceof File file) { + return new FileInputStream(file); + } + if (content instanceof byte[] bytes) { + return new ByteArrayInputStream(bytes); + } + throw new IOException("No content stream available for " + getName()); + } + } diff --git a/core/src/test/java/org/apache/struts2/dispatcher/multipart/StrutsUploadedFileTest.java b/core/src/test/java/org/apache/struts2/dispatcher/multipart/StrutsUploadedFileTest.java new file mode 100644 index 0000000000..23df88dbca --- /dev/null +++ b/core/src/test/java/org/apache/struts2/dispatcher/multipart/StrutsUploadedFileTest.java @@ -0,0 +1,49 @@ +/* + * 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.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; + +import java.io.File; +import java.io.IOException; +import java.io.InputStream; +import java.nio.file.Files; + +import static java.nio.charset.StandardCharsets.UTF_8; +import static org.assertj.core.api.Assertions.assertThat; + +public class StrutsUploadedFileTest { + + @Rule + public TemporaryFolder tempFolder = new TemporaryFolder(); + + @Test + public void getInputStreamReadsFileContent() throws IOException { + File backing = tempFolder.newFile("upload_test.tmp"); + Files.writeString(backing.toPath(), "hello"); + + UploadedFile file = StrutsUploadedFile.Builder.create(backing).build(); + + try (InputStream in = file.getInputStream()) { + assertThat(new String(in.readAllBytes(), UTF_8)).isEqualTo("hello"); + } + } +} diff --git a/core/src/test/java/org/apache/struts2/dispatcher/multipart/UploadedFileTest.java b/core/src/test/java/org/apache/struts2/dispatcher/multipart/UploadedFileTest.java new file mode 100644 index 0000000000..ec96ce329a --- /dev/null +++ b/core/src/test/java/org/apache/struts2/dispatcher/multipart/UploadedFileTest.java @@ -0,0 +1,49 @@ +/* + * 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.junit.Test; + +import java.io.IOException; +import java.io.InputStream; + +import static java.nio.charset.StandardCharsets.UTF_8; +import static org.assertj.core.api.Assertions.assertThat; + +public class UploadedFileTest { + + @Test + public void defaultGetInputStreamReadsByteArrayContent() throws IOException { + UploadedFile file = new UploadedFile() { + public Long length() { return 3L; } + public String getName() { return "x"; } + public String getOriginalName() { return "x"; } + public boolean isFile() { return false; } + public boolean delete() { return true; } + public String getAbsolutePath() { return null; } + public Object getContent() { return "abc".getBytes(UTF_8); } + public String getContentType() { return "text/plain"; } + public String getInputName() { return "file"; } + }; + + try (InputStream in = file.getInputStream()) { + assertThat(new String(in.readAllBytes(), UTF_8)).isEqualTo("abc"); + } + } +} From e1aaab9ca8ebfe79df29d50b4da214749f44b224 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Wed, 22 Jul 2026 13:26:11 +0200 Subject: [PATCH 04/13] WW-5413 feat(core): add lazily-materializing StrutsInMemoryUploadedFile Co-Authored-By: Claude Opus 4.8 --- .../multipart/StrutsInMemoryUploadedFile.java | 180 ++++++++++++++++++ .../StrutsInMemoryUploadedFileTest.java | 143 ++++++++++++++ 2 files changed, 323 insertions(+) create mode 100644 core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFile.java create mode 100644 core/src/test/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFileTest.java diff --git a/core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFile.java b/core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFile.java new file mode 100644 index 0000000000..f50acefe15 --- /dev/null +++ b/core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFile.java @@ -0,0 +1,180 @@ +/* + * 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.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; +import org.apache.struts2.StrutsException; + +import java.io.ByteArrayInputStream; +import java.io.File; +import java.io.IOException; +import java.io.InputStream; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.UUID; + +/** + * In-memory backed {@link UploadedFile} for small multipart uploads that Commons FileUpload kept + * in memory ({@code DiskFileItem.isInMemory() == true}). + * + *

The content is held as a byte array and is written to a temporary file only the first time a + * caller demands a {@link File} through {@link #getContent()} or {@link #getAbsolutePath()} (lazy + * materialization). Callers reading through {@link #getInputStream()} never touch the disk. The + * temporary file uses the secure {@code upload_.tmp} naming and ignores the user-supplied + * original filename.

+ * + * @since 7.3.0 + */ +public class StrutsInMemoryUploadedFile implements UploadedFile { + + private static final Logger LOG = LogManager.getLogger(StrutsInMemoryUploadedFile.class); + + private final byte[] content; + private final Path saveDir; + private final String name; + private final String contentType; + private final String originalName; + private final String inputName; + + private transient File materializedFile; + + private StrutsInMemoryUploadedFile(byte[] content, Path saveDir, String contentType, + String originalName, String inputName) { + this.content = content; + this.saveDir = saveDir; + this.contentType = contentType; + this.originalName = originalName; + this.inputName = inputName; + this.name = "upload_" + UUID.randomUUID().toString().replace("-", "_") + ".tmp"; + } + + private synchronized File materialize() { + if (materializedFile == null) { + File target = saveDir.resolve(name).toFile(); + try { + Files.write(target.toPath(), content); + } catch (IOException e) { + throw new StrutsException("Could not materialize in-memory uploaded file: " + name, e); + } + materializedFile = target; + LOG.debug("Materialized in-memory uploaded item to {}", target.getAbsolutePath()); + } + return materializedFile; + } + + @Override + public InputStream getInputStream() { + return new ByteArrayInputStream(content); + } + + @Override + public Long length() { + return (long) content.length; + } + + @Override + public String getName() { + return name; + } + + @Override + public boolean isFile() { + return materializedFile != null && materializedFile.isFile(); + } + + @Override + public boolean delete() { + if (materializedFile != null) { + return materializedFile.delete(); + } + return true; + } + + @Override + public String getAbsolutePath() { + return materialize().getAbsolutePath(); + } + + @Override + public File getContent() { + return materialize(); + } + + @Override + public String getContentType() { + return contentType; + } + + @Override + public String getOriginalName() { + return originalName; + } + + @Override + public String getInputName() { + return inputName; + } + + @Override + public String toString() { + return "StrutsInMemoryUploadedFile{" + + "contentType='" + contentType + '\'' + + ", originalName='" + originalName + '\'' + + ", inputName='" + inputName + '\'' + + ", size=" + content.length + + '}'; + } + + public static class Builder { + private final byte[] content; + private final Path saveDir; + private String contentType; + private String originalName; + private String inputName; + + private Builder(byte[] content, Path saveDir) { + this.content = content; + this.saveDir = saveDir; + } + + public static Builder create(byte[] content, Path saveDir) { + return new Builder(content, saveDir); + } + + public Builder withContentType(String contentType) { + this.contentType = contentType; + return this; + } + + public Builder withOriginalName(String originalName) { + this.originalName = originalName; + return this; + } + + public Builder withInputName(String inputName) { + this.inputName = inputName; + return this; + } + + public UploadedFile build() { + return new StrutsInMemoryUploadedFile(content, saveDir, contentType, originalName, inputName); + } + } +} diff --git a/core/src/test/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFileTest.java b/core/src/test/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFileTest.java new file mode 100644 index 0000000000..62a88d6118 --- /dev/null +++ b/core/src/test/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFileTest.java @@ -0,0 +1,143 @@ +/* + * 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.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; + +import java.io.File; +import java.io.IOException; +import java.io.InputStream; +import java.nio.file.Path; + +import static java.nio.charset.StandardCharsets.UTF_8; +import static org.assertj.core.api.Assertions.assertThat; + +public class StrutsInMemoryUploadedFileTest { + + @Rule + public TemporaryFolder tempFolder = new TemporaryFolder(); + + private Path saveDir; + + @Before + public void setUp() { + saveDir = tempFolder.getRoot().toPath(); + } + + private UploadedFile build(byte[] content) { + return StrutsInMemoryUploadedFile.Builder + .create(content, saveDir) + .withOriginalName("orig.txt") + .withContentType("text/plain") + .withInputName("file") + .build(); + } + + @Test + public void getInputStreamReturnsBytesWithoutWritingFile() throws IOException { + UploadedFile file = build("hello".getBytes(UTF_8)); + + try (InputStream in = file.getInputStream()) { + assertThat(new String(in.readAllBytes(), UTF_8)).isEqualTo("hello"); + } + + assertThat(file.isFile()).isFalse(); + assertThat(tempFolder.getRoot().listFiles()).isEmpty(); + } + + @Test + public void getContentMaterializesFileExactlyOnce() { + UploadedFile file = build("data".getBytes(UTF_8)); + + File first = (File) file.getContent(); + File second = (File) file.getContent(); + + assertThat(first).exists().hasContent("data"); + assertThat(second).isSameAs(first); + assertThat(tempFolder.getRoot().listFiles()).hasSize(1); + } + + @Test + public void getAbsolutePathMaterializesFile() { + UploadedFile file = build("data".getBytes(UTF_8)); + + String path = file.getAbsolutePath(); + + assertThat(new File(path)).exists().hasContent("data"); + assertThat(file.isFile()).isTrue(); + } + + @Test + public void isFileFalseBeforeMaterializationTrueAfter() { + UploadedFile file = build("x".getBytes(UTF_8)); + + assertThat(file.isFile()).isFalse(); + file.getContent(); + assertThat(file.isFile()).isTrue(); + } + + @Test + public void lengthAndMetadataDoNotMaterialize() { + UploadedFile file = build("abcd".getBytes(UTF_8)); + + assertThat(file.length()).isEqualTo(4L); + assertThat(file.getContentType()).isEqualTo("text/plain"); + assertThat(file.getOriginalName()).isEqualTo("orig.txt"); + assertThat(file.getInputName()).isEqualTo("file"); + assertThat(file.getName()).startsWith("upload_").endsWith(".tmp"); + + assertThat(file.isFile()).isFalse(); + assertThat(tempFolder.getRoot().listFiles()).isEmpty(); + } + + @Test + public void deleteRemovesMaterializedFile() { + UploadedFile file = build("x".getBytes(UTF_8)); + File materialized = (File) file.getContent(); + assertThat(materialized).exists(); + + assertThat(file.delete()).isTrue(); + assertThat(materialized).doesNotExist(); + } + + @Test + public void deleteIsNoOpWhenNotMaterialized() { + UploadedFile file = build("x".getBytes(UTF_8)); + + assertThat(file.delete()).isTrue(); + assertThat(tempFolder.getRoot().listFiles()).isEmpty(); + } + + @Test + public void maliciousOriginalNameDoesNotLeakIntoTempName() { + UploadedFile file = StrutsInMemoryUploadedFile.Builder + .create("x".getBytes(UTF_8), saveDir) + .withOriginalName("../../etc/passwd") + .build(); + + file.getContent(); // materialize + + assertThat(file.getName()).startsWith("upload_").endsWith(".tmp"); + assertThat(file.getName()).doesNotContain("..").doesNotContain("/").doesNotContain("\\"); + assertThat(new File(saveDir.toFile(), file.getName())).exists(); + } +} From b6d431e155411792aa38f4d7481066ac53918728 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Wed, 22 Jul 2026 13:34:07 +0200 Subject: [PATCH 05/13] WW-5413 fix(core): make StrutsInMemoryUploadedFile serializable and thread-safe --- .../multipart/StrutsInMemoryUploadedFile.java | 30 ++++++++++--------- .../StrutsInMemoryUploadedFileTest.java | 28 +++++++++++++++++ 2 files changed, 44 insertions(+), 14 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFile.java b/core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFile.java index f50acefe15..cc7fa13032 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFile.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFile.java @@ -44,37 +44,37 @@ */ public class StrutsInMemoryUploadedFile implements UploadedFile { + private static final long serialVersionUID = 1L; + private static final Logger LOG = LogManager.getLogger(StrutsInMemoryUploadedFile.class); private final byte[] content; - private final Path saveDir; - private final String name; + private final File targetFile; private final String contentType; private final String originalName; private final String inputName; - private transient File materializedFile; + private volatile transient File materializedFile; private StrutsInMemoryUploadedFile(byte[] content, Path saveDir, String contentType, String originalName, String inputName) { this.content = content; - this.saveDir = saveDir; + String name = "upload_" + UUID.randomUUID().toString().replace("-", "_") + ".tmp"; + this.targetFile = saveDir.resolve(name).toFile(); this.contentType = contentType; this.originalName = originalName; this.inputName = inputName; - this.name = "upload_" + UUID.randomUUID().toString().replace("-", "_") + ".tmp"; } private synchronized File materialize() { if (materializedFile == null) { - File target = saveDir.resolve(name).toFile(); try { - Files.write(target.toPath(), content); + Files.write(targetFile.toPath(), content); } catch (IOException e) { - throw new StrutsException("Could not materialize in-memory uploaded file: " + name, e); + throw new StrutsException("Could not materialize in-memory uploaded file: " + targetFile.getName(), e); } - materializedFile = target; - LOG.debug("Materialized in-memory uploaded item to {}", target.getAbsolutePath()); + materializedFile = targetFile; + LOG.debug("Materialized in-memory uploaded item to {}", targetFile.getAbsolutePath()); } return materializedFile; } @@ -91,18 +91,20 @@ public Long length() { @Override public String getName() { - return name; + return targetFile.getName(); } @Override public boolean isFile() { - return materializedFile != null && materializedFile.isFile(); + File f = materializedFile; + return f != null && f.isFile(); } @Override public boolean delete() { - if (materializedFile != null) { - return materializedFile.delete(); + File f = materializedFile; + if (f != null) { + return f.delete(); } return true; } diff --git a/core/src/test/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFileTest.java b/core/src/test/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFileTest.java index 62a88d6118..96d5741197 100644 --- a/core/src/test/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFileTest.java +++ b/core/src/test/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFileTest.java @@ -23,9 +23,13 @@ import org.junit.Test; import org.junit.rules.TemporaryFolder; +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; import java.io.File; import java.io.IOException; import java.io.InputStream; +import java.io.ObjectInputStream; +import java.io.ObjectOutputStream; import java.nio.file.Path; import static java.nio.charset.StandardCharsets.UTF_8; @@ -140,4 +144,28 @@ public void maliciousOriginalNameDoesNotLeakIntoTempName() { assertThat(file.getName()).doesNotContain("..").doesNotContain("/").doesNotContain("\\"); assertThat(new File(saveDir.toFile(), file.getName())).exists(); } + + @Test + public void isSerializableWhenNotMaterialized() throws IOException, ClassNotFoundException { + UploadedFile file = build("payload".getBytes(UTF_8)); + + byte[] bytes; + try (ByteArrayOutputStream bos = new ByteArrayOutputStream(); + ObjectOutputStream oos = new ObjectOutputStream(bos)) { + oos.writeObject(file); + bytes = bos.toByteArray(); + } + + UploadedFile restored; + try (ObjectInputStream ois = new ObjectInputStream(new ByteArrayInputStream(bytes))) { + restored = (UploadedFile) ois.readObject(); + } + + assertThat(restored.length()).isEqualTo(7L); + assertThat(restored.getContentType()).isEqualTo("text/plain"); + assertThat(restored.getName()).startsWith("upload_").endsWith(".tmp"); + try (InputStream in = restored.getInputStream()) { + assertThat(new String(in.readAllBytes(), UTF_8)).isEqualTo("payload"); + } + } } From 8c97cdb76f91625e521039baba751ba131384f53 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Wed, 22 Jul 2026 13:47:51 +0200 Subject: [PATCH 06/13] WW-5413 refactor(core): drop eager temp-file write for in-memory uploads --- .../multipart/JakartaMultiPartRequest.java | 119 +++--------- .../AbstractMultiPartRequestTest.java | 20 +- .../JakartaMultiPartRequestTest.java | 177 ------------------ 3 files changed, 34 insertions(+), 282 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java b/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java index 6b19962eb3..061ecc593c 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java @@ -25,12 +25,9 @@ import org.apache.commons.lang3.StringUtils; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; -import org.apache.struts2.dispatcher.LocalizedMessage; -import java.io.File; import java.io.IOException; import java.nio.charset.Charset; -import java.nio.file.Files; import java.nio.file.Path; import java.util.ArrayList; import java.util.List; @@ -41,13 +38,14 @@ * Multipart form data request adapter for Jakarta Commons FileUpload package. * *

This implementation provides secure handling of multipart requests with proper - * resource management and cleanup. It tracks all temporary files created during - * the upload process and ensures they are properly cleaned up to prevent - * resource leaks and security vulnerabilities.

- * + * resource management and cleanup. It tracks all {@link DiskFileItem} instances + * created during the upload process and ensures they are properly cleaned up to + * prevent resource leaks and security vulnerabilities. In-memory uploads are kept + * as byte arrays and only materialized to a temporary file lazily, on demand.

+ * *

Key features:

*
    - *
  • Automatic tracking and cleanup of temporary files
    • + *
  • Automatic tracking and cleanup of underlying disk file items
    • *
  • Proper error handling with user-friendly error messages
    • *
  • Support for both in-memory and disk-based file uploads
    • *
  • Extensible cleanup mechanisms for customization
    • @@ -79,11 +77,6 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { * List to track all DiskFileItem instances for proper cleanup */ private final List diskFileItems = new ArrayList<>(); - - /** - * List to track temporary files created for in-memory uploads - */ - private final List temporaryFiles = new ArrayList<>(); /** * Processes the multipart upload request using Jakarta Commons FileUpload. @@ -181,20 +174,17 @@ protected void processNormalFormField(DiskFileItem item, Charset charset) throws *
      *
    1. Validating the file name and field name are not null/empty
      2. *
    2. Determining if the file is stored in memory or on disk
      3. - *
    3. For in-memory files: creating a temporary file and copying content
      4. + *
    4. For in-memory files: wrapping the content in a {@link StrutsInMemoryUploadedFile} + * that only writes to disk lazily, on demand
      5. *
    5. For disk files: using the existing file directly
      6. *
    6. Creating an {@link UploadedFile} abstraction
      7. *
    7. Adding the file to the uploaded files collection
      8. *
    - * - *

    Temporary files created for in-memory uploads are automatically - * tracked for cleanup. Any errors during temporary file creation are - * logged and added to the error list for user feedback.

    - * + * * @param item the disk file item representing the uploaded file - * @see #cleanUpTemporaryFiles() + * @throws IOException if an error occurs reading the in-memory item's content */ - protected void processFileField(DiskFileItem item, String saveDir) { + protected void processFileField(DiskFileItem item, String saveDir) throws IOException { // Skip file uploads that don't have a file name - meaning that no file was selected. if (item.getName() == null || item.getName().trim().isEmpty()) { LOG.debug(() -> "No file has been uploaded for the field: " + normalizeSpace(item.getFieldName())); @@ -215,40 +205,14 @@ protected void processFileField(DiskFileItem item, String saveDir) { List values = uploadedFiles.computeIfAbsent(fieldName, k -> new ArrayList<>()); if (item.isInMemory()) { - LOG.debug(() -> "Creating temporary file representing in-memory uploaded item: " + normalizeSpace(item.getFieldName())); - try { - File tempFile = createTemporaryFile(item.getName(), Path.of(saveDir)); - - // Track the temporary file for explicit cleanup - temporaryFiles.add(tempFile); - - // Write the in-memory content to the temporary file - try (java.io.FileOutputStream fos = new java.io.FileOutputStream(tempFile)) { - fos.write(item.get()); - } - - UploadedFile uploadedFile = StrutsUploadedFile.Builder - .create(tempFile) - .withOriginalName(item.getName()) - .withContentType(item.getContentType()) - .withInputName(item.getFieldName()) - .build(); - values.add(uploadedFile); - - if (LOG.isDebugEnabled()) { - LOG.debug("Created temporary file for in-memory uploaded item: {} at {}", - normalizeSpace(item.getName()), tempFile.getAbsolutePath()); - } - } catch (IOException e) { - LOG.warn("Failed to create temporary file for in-memory uploaded item: {}", - normalizeSpace(item.getName()), e); - - // Add the error to the errors list for proper user feedback - LocalizedMessage errorMessage = buildErrorMessage(e.getClass(), e.getMessage(), new Object[]{item.getName()}); - if (!errors.contains(errorMessage)) { - errors.add(errorMessage); - } - } + LOG.debug(() -> "Keeping in-memory uploaded item without writing to disk: " + normalizeSpace(item.getFieldName())); + UploadedFile uploadedFile = StrutsInMemoryUploadedFile.Builder + .create(item.get(), Path.of(saveDir)) + .withOriginalName(item.getName()) + .withContentType(item.getContentType()) + .withInputName(item.getFieldName()) + .build(); + values.add(uploadedFile); } else { UploadedFile uploadedFile = StrutsUploadedFile.Builder .create(item.getPath().toFile()) @@ -276,9 +240,8 @@ protected void processFileField(DiskFileItem item, String saveDir) { * be overridden by subclasses to customize cleanup behavior. All exceptions * are caught and logged to prevent cleanup failures from affecting the * overall cleanup process.

    - * + * * @see #cleanUp() - * @see #cleanUpTemporaryFiles() */ protected void cleanUpDiskFileItems() { LOG.debug("Clean up all DiskFileItem instances (both form fields and file uploads"); @@ -299,58 +262,26 @@ protected void cleanUpDiskFileItems() { } } - /** - * Cleans up temporary files created for in-memory uploads. - * - *

    This method deletes all temporary files that were created when - * processing in-memory uploads. These files are created in - * {@link #processFileField(DiskFileItem, String)} when an uploaded file is - * stored in memory and needs to be written to disk.

    - * - *

    The cleanup process:

    - *
      - *
    1. Iterates through all tracked temporary files
      2. - *
    2. Checks if each file still exists
      3. - *
    3. Attempts to delete existing files
      4. - *
    4. Logs warnings for files that cannot be deleted
      5. - *
    - * - *

    This method can be overridden by subclasses to customize cleanup - * behavior. All exceptions are caught and logged to ensure cleanup - * continues even if individual file deletions fail.

    - * - * @see #cleanUp() - * @see #cleanUpDiskFileItems() - */ - protected void cleanUpTemporaryFiles() { - LOG.debug("Cleaning up {} temporary files created for in-memory uploads", temporaryFiles.size()); - for (File tempFile : temporaryFiles) { - deleteFile(tempFile.toPath()); - } - } - /** * Performs complete cleanup of all resources associated with this request. - * + * *

    This method extends the parent cleanup functionality to ensure proper * cleanup of Jakarta-specific resources:

    *
      *
    1. Calls parent cleanup to handle base class resources
      2. *
    2. Cleans up all tracked {@link DiskFileItem} instances
      3. - *
    3. Cleans up all temporary files created for in-memory uploads
      4. *
    4. Clears internal tracking collections
      5. *
    - * + * *

    This method is designed to be safe to call multiple times and will * not throw exceptions even if cleanup operations fail. All errors are * logged for debugging purposes.

    - * + * *

    Important: This method should always be called in a * finally block to ensure resources are properly released, even if * exceptions occur during request processing.

    - * + * * @see #cleanUpDiskFileItems() - * @see #cleanUpTemporaryFiles() * @see AbstractMultiPartRequest#cleanUp() */ @Override @@ -358,10 +289,8 @@ public void cleanUp() { super.cleanUp(); try { cleanUpDiskFileItems(); - cleanUpTemporaryFiles(); } finally { diskFileItems.clear(); - temporaryFiles.clear(); } } diff --git a/core/src/test/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequestTest.java b/core/src/test/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequestTest.java index 6a2450201a..c20b72b6d0 100644 --- a/core/src/test/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequestTest.java +++ b/core/src/test/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequestTest.java @@ -207,8 +207,6 @@ public void uploadedFilesWithLargeBuffer() throws IOException { .asInstanceOf(InstanceOfAssertFactories.LIST) .containsOnly("file1", "file2"); assertThat(multiPart.getFile("file1")).allSatisfy(file -> { - assertThat(file.isFile()) - .isTrue(); assertThat(file.getOriginalName()) .isEqualTo("test1.csv"); assertThat(file.getContentType()) @@ -220,10 +218,10 @@ public void uploadedFilesWithLargeBuffer() throws IOException { .exists() .content() .isEqualTo("1,2,3,4"); - }); - assertThat(multiPart.getFile("file2")).allSatisfy(file -> { assertThat(file.isFile()) .isTrue(); + }); + assertThat(multiPart.getFile("file2")).allSatisfy(file -> { assertThat(file.getOriginalName()) .isEqualTo("test2.csv"); assertThat(file.getInputName()) @@ -233,6 +231,8 @@ public void uploadedFilesWithLargeBuffer() throws IOException { .exists() .content() .isEqualTo("5,6,7,8"); + assertThat(file.isFile()) + .isTrue(); }); } @@ -258,8 +258,6 @@ public void cleanUp() throws IOException { .asInstanceOf(InstanceOfAssertFactories.LIST) .containsOnly("file1", "file2"); assertThat(multiPart.getFile("file1")).allSatisfy(file -> { - assertThat(file.isFile()) - .isTrue(); assertThat(file.getOriginalName()) .isEqualTo("test1.csv"); assertThat(file.getContentType()) @@ -270,10 +268,10 @@ public void cleanUp() throws IOException { .exists() .content() .isEqualTo("1,2,3,4"); - }); - assertThat(multiPart.getFile("file2")).allSatisfy(file -> { assertThat(file.isFile()) .isTrue(); + }); + assertThat(multiPart.getFile("file2")).allSatisfy(file -> { assertThat(file.getOriginalName()) .isEqualTo("test2.csv"); assertThat(file.getContentType()) @@ -285,6 +283,8 @@ public void cleanUp() throws IOException { .exists() .content() .isEqualTo("5,6,7,8"); + assertThat(file.isFile()) + .isTrue(); }); List uploadedFiles = new ArrayList<>(); @@ -431,8 +431,6 @@ public void mismatchCharset() throws IOException { .asInstanceOf(InstanceOfAssertFactories.LIST) .containsOnly("file1"); assertThat(multiPart.getFile("file1")).allSatisfy(file -> { - assertThat(file.isFile()) - .isTrue(); assertThat(file.getOriginalName()) .isEqualTo("test1.csv"); assertThat(file.getContentType()) @@ -442,6 +440,8 @@ public void mismatchCharset() throws IOException { .exists() .content() .isEqualTo("Ł,Ś,Ż,Ó"); + assertThat(file.isFile()) + .isTrue(); }); } diff --git a/core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequestTest.java b/core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequestTest.java index 15b59f5dd2..fd7727a7b9 100644 --- a/core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequestTest.java +++ b/core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequestTest.java @@ -23,13 +23,9 @@ import org.assertj.core.api.InstanceOfAssertFactories; import org.junit.Test; -import java.io.File; import java.io.IOException; import java.lang.reflect.Field; import java.nio.charset.StandardCharsets; -import java.nio.file.Files; -import java.nio.file.Path; -import java.util.List; import static org.apache.commons.lang3.StringUtils.normalizeSpace; import static org.assertj.core.api.Assertions.assertThat; @@ -41,119 +37,6 @@ protected AbstractMultiPartRequest createMultipartRequest() { return new JakartaMultiPartRequest(); } - @Test - public void temporaryFileCleanupForInMemoryUploads() throws IOException, NoSuchFieldException, IllegalAccessException { - // given - small files that will be in-memory - String content = formFile("file1", "test1.csv", "a,b,c,d") + - formFile("file2", "test2.csv", "1,2,3,4") + - endline + "--" + boundary + "--"; - - mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8)); - - // when - multiPart.parse(mockRequest, tempDir); - - // Access private field to verify temporary files are tracked - Field tempFilesField = JakartaMultiPartRequest.class.getDeclaredField("temporaryFiles"); - tempFilesField.setAccessible(true); - @SuppressWarnings("unchecked") - List temporaryFiles = (List) tempFilesField.get(multiPart); - - // Store file paths before cleanup for verification - List tempFilePaths = temporaryFiles.stream() - .map(File::getAbsolutePath) - .toList(); - - // Verify temporary files exist before cleanup - assertThat(temporaryFiles).isNotEmpty(); - for (File tempFile : temporaryFiles) { - assertThat(tempFile).exists(); - } - - // when - cleanup - multiPart.cleanUp(); - - // then - verify files are deleted and tracking list is cleared - for (String tempFilePath : tempFilePaths) { - assertThat(new File(tempFilePath)).doesNotExist(); - } - assertThat(temporaryFiles).isEmpty(); - } - - @Test - public void cleanupMethodsCanBeOverridden() { - // Create a custom implementation to test extensibility - class CustomJakartaMultiPartRequest extends JakartaMultiPartRequest { - boolean diskFileItemsCleanedUp = false; - boolean temporaryFilesCleanedUp = false; - - @Override - protected void cleanUpDiskFileItems() { - diskFileItemsCleanedUp = true; - super.cleanUpDiskFileItems(); - } - - @Override - protected void cleanUpTemporaryFiles() { - temporaryFilesCleanedUp = true; - super.cleanUpTemporaryFiles(); - } - } - - CustomJakartaMultiPartRequest customMultiPart = new CustomJakartaMultiPartRequest(); - - // when - customMultiPart.cleanUp(); - - // then - assertThat(customMultiPart.diskFileItemsCleanedUp).isTrue(); - assertThat(customMultiPart.temporaryFilesCleanedUp).isTrue(); - } - - @Test - public void temporaryFileCreationFailureAddsError() throws IOException { - // Create a custom implementation that simulates temp file creation failure - class FaultyJakartaMultiPartRequest extends JakartaMultiPartRequest { - @Override - protected void processFileField(DiskFileItem item, String saveDir) { - // Simulate in-memory upload that fails to create temp file - if (item.isInMemory()) { - try { - // Simulate IOException during temp file creation - throw new IOException("Simulated temp file creation failure"); - } catch (IOException e) { - // Add the error to the errors list for proper user feedback - LocalizedMessage errorMessage = buildErrorMessage(e.getClass(), e.getMessage(), - new Object[]{item.getName()}); - if (!errors.contains(errorMessage)) { - errors.add(errorMessage); - } - } - } else { - super.processFileField(item, saveDir); - } - } - } - - FaultyJakartaMultiPartRequest faultyMultiPart = new FaultyJakartaMultiPartRequest(); - - // given - small file that would normally be in-memory - String content = formFile("file1", "test1.csv", "a,b") + - endline + "--" + boundary + "--"; - - mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8)); - - // when - faultyMultiPart.parse(mockRequest, tempDir); - - // then - verify error is properly captured - assertThat(faultyMultiPart.getErrors()) - .hasSize(1) - .first() - .extracting(LocalizedMessage::getTextKey) - .isEqualTo("struts.messages.upload.error.IOException"); - } - @Test public void temporaryFileCreationErrorsAreNotDuplicated() throws IOException { // Test that duplicate errors are not added to the errors list @@ -222,66 +105,6 @@ public void endToEndMultipartProcessingWithCleanup() throws IOException { assertThat(multiPart.parameters).isEmpty(); } - @Test - public void temporaryFilesCreatedInSaveDirectory() throws IOException, NoSuchFieldException, IllegalAccessException { - // Test that temporary files for in-memory uploads are created in the saveDir, not system temp - String content = formFile("file1", "test1.csv", "small,content") + - endline + "--" + boundary + "--"; - - mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8)); - - // when - multiPart.parse(mockRequest, tempDir); - - // Access private field to get temporary files - Field tempFilesField = JakartaMultiPartRequest.class.getDeclaredField("temporaryFiles"); - tempFilesField.setAccessible(true); - @SuppressWarnings("unchecked") - List temporaryFiles = (List) tempFilesField.get(multiPart); - - // then - verify temporary files are created in saveDir - assertThat(temporaryFiles).isNotEmpty(); - for (File tempFile : temporaryFiles) { - // Verify the temporary file is in the saveDir, not system temp - assertThat(tempFile.getParent()).isEqualTo(tempDir); - assertThat(tempFile.getName()).startsWith("upload_"); - assertThat(tempFile.getName()).endsWith(".tmp"); - assertThat(tempFile).exists(); - } - } - - @Test - public void secureTemporaryFileNaming() throws IOException, NoSuchFieldException, IllegalAccessException { - // Test that temporary files use UUID-based naming for security - String content = formFile("file1", "malicious../../../etc/passwd", "content") + - endline + "--" + boundary + "--"; - - mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8)); - - // when - multiPart.parse(mockRequest, tempDir); - - // Access private field to get temporary files - Field tempFilesField = JakartaMultiPartRequest.class.getDeclaredField("temporaryFiles"); - tempFilesField.setAccessible(true); - @SuppressWarnings("unchecked") - List temporaryFiles = (List) tempFilesField.get(multiPart); - - // then - verify secure naming prevents directory traversal - assertThat(temporaryFiles).isNotEmpty(); - for (File tempFile : temporaryFiles) { - // Verify the temporary file uses secure UUID naming - assertThat(tempFile.getName()).startsWith("upload_"); - assertThat(tempFile.getName()).endsWith(".tmp"); - // Verify it doesn't contain malicious path elements - assertThat(tempFile.getName()).doesNotContain(".."); - assertThat(tempFile.getName()).doesNotContain("/"); - assertThat(tempFile.getName()).doesNotContain("\\"); - // Verify it's in the correct directory - assertThat(tempFile.getParent()).isEqualTo(tempDir); - } - } - @Test public void processNormalFormFieldHandlesNullFieldName() throws IOException { // Test null field name handling in processNormalFormField From 7c08afa06a33aef41357d6295bf6342d81aa6a33 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Wed, 22 Jul 2026 13:56:46 +0200 Subject: [PATCH 07/13] WW-5413 test(core): cover deferred-write behavior for in-memory uploads Co-Authored-By: Claude Opus 4.8 --- .../JakartaMultiPartRequestTest.java | 41 +++++++++++++++++-- 1 file changed, 37 insertions(+), 4 deletions(-) diff --git a/core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequestTest.java b/core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequestTest.java index fd7727a7b9..b2263a6b67 100644 --- a/core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequestTest.java +++ b/core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequestTest.java @@ -23,7 +23,9 @@ import org.assertj.core.api.InstanceOfAssertFactories; import org.junit.Test; +import java.io.File; import java.io.IOException; +import java.io.InputStream; import java.lang.reflect.Field; import java.nio.charset.StandardCharsets; @@ -247,7 +249,7 @@ public void errorDuplicationPrevention() throws IOException { @Test public void processFileFieldHandlesEmptyFileName() throws IOException { - String content = + String content = endline + "--" + boundary + endline + "Content-Disposition: form-data; name=\"emptyfile\"; filename=\"\"" + endline + "Content-Type: text/plain" + endline + @@ -259,12 +261,12 @@ public void processFileFieldHandlesEmptyFileName() throws IOException { endline + "valid file content" + endline + "--" + boundary + "--"; - + mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8)); - + // when multiPart.parse(mockRequest, tempDir); - + // then - should only process the file with valid filename assertThat(multiPart.getErrors()).isEmpty(); assertThat(multiPart.uploadedFiles).hasSize(1); @@ -276,4 +278,35 @@ public void processFileFieldHandlesEmptyFileName() throws IOException { .isEqualTo("valid file content"); } + @Test + public void inMemoryUploadIsNotWrittenToDiskUntilContentRequested() throws IOException { + // given - a small file that Commons FileUpload keeps in memory + String content = formFile("file1", "test1.csv", "a,b,c,d") + + endline + "--" + boundary + "--"; + mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8)); + + // when + multiPart.parse(mockRequest, tempDir); + + UploadedFile file = multiPart.getFile("file1")[0]; + + // then - nothing written to disk right after parse + assertThat(file.isFile()).isFalse(); + + // and - content is readable via the stream path without materializing + try (InputStream in = file.getInputStream()) { + assertThat(new String(in.readAllBytes(), StandardCharsets.UTF_8)).isEqualTo("a,b,c,d"); + } + assertThat(file.isFile()).isFalse(); + + // and - getContent() materializes a real file on demand + File materialized = (File) file.getContent(); + assertThat(materialized).exists().hasContent("a,b,c,d"); + assertThat(file.isFile()).isTrue(); + + // and - cleanUp removes the materialized file + multiPart.cleanUp(); + assertThat(materialized).doesNotExist(); + } + } From 166f14800d0bf890a05fc76693c6d718be1bbd57 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Wed, 22 Jul 2026 14:14:53 +0200 Subject: [PATCH 08/13] WW-5413 perf(core): avoid materializing in-memory uploads during interceptor validation --- .../multipart/StrutsInMemoryUploadedFile.java | 5 +++ .../dispatcher/multipart/UploadedFile.java | 13 ++++++ .../AbstractFileUploadInterceptor.java | 9 +---- .../ActionFileUploadInterceptorTest.java | 40 +++++++++++++++++++ 4 files changed, 60 insertions(+), 7 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFile.java b/core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFile.java index cc7fa13032..22df11e893 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFile.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFile.java @@ -84,6 +84,11 @@ public InputStream getInputStream() { return new ByteArrayInputStream(content); } + @Override + public boolean isMissing() { + return false; + } + @Override public Long length() { return (long) content.length; diff --git a/core/src/main/java/org/apache/struts2/dispatcher/multipart/UploadedFile.java b/core/src/main/java/org/apache/struts2/dispatcher/multipart/UploadedFile.java index e6d48a163a..590d1e0c02 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/multipart/UploadedFile.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/multipart/UploadedFile.java @@ -99,4 +99,17 @@ default InputStream getInputStream() throws IOException { throw new IOException("No content stream available for " + getName()); } + /** + * Indicates whether this upload has no content available (for example, the upload failed). + * Implementations that hold their content in memory should override this to answer WITHOUT + * materializing the content to disk. The default reports missing when {@link #getContent()} + * is {@code null}. + * + * @return true if there is no content backing this upload + * @since 7.3.0 + */ + default boolean isMissing() { + return getContent() == null; + } + } diff --git a/core/src/main/java/org/apache/struts2/interceptor/AbstractFileUploadInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/AbstractFileUploadInterceptor.java index ee698721bd..3e533754a1 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/AbstractFileUploadInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/AbstractFileUploadInterceptor.java @@ -114,8 +114,8 @@ protected boolean acceptFile(Object action, UploadedFile file, String originalFi validation = validationAware; } - // If it's null the upload failed - if (file == null || file.getContent() == null) { + // If it's missing the upload failed + if (file == null || file.isMissing()) { String errMsg = getTextMessage(action, STRUTS_MESSAGES_ERROR_UPLOADING_KEY, new String[]{inputName}); if (validation != null) { validation.addFieldError(inputName, errMsg); @@ -124,11 +124,6 @@ protected boolean acceptFile(Object action, UploadedFile file, String originalFi return false; } - if (file.getContent() == null) { - String errMsg = getTextMessage(action, STRUTS_MESSAGES_INVALID_CONTENT_TYPE_KEY, new String[]{originalFilename}); - errorMessages.add(errMsg); - LOG.warn(errMsg); - } if (maximumSize != null && maximumSize < file.length()) { String errMsg = getTextMessage(action, STRUTS_MESSAGES_ERROR_FILE_TOO_LARGE_KEY, new String[]{ inputName, originalFilename, file.getName(), "" + file.length(), getMaximumSizeStr(action) diff --git a/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java b/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java index 61339327d8..058dabe115 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java @@ -27,6 +27,7 @@ import org.apache.struts2.action.UploadedFilesAware; import org.apache.struts2.dispatcher.multipart.JakartaMultiPartRequest; import org.apache.struts2.dispatcher.multipart.MultiPartRequestWrapper; +import org.apache.struts2.dispatcher.multipart.StrutsInMemoryUploadedFile; import org.apache.struts2.dispatcher.multipart.StrutsUploadedFile; import org.apache.struts2.dispatcher.multipart.UploadedFile; import org.apache.struts2.locale.DefaultLocaleProvider; @@ -171,6 +172,45 @@ public void testAcceptFileWithNoContent() { .contains("inputName"); } + public void testAcceptFileDoesNotMaterializeInMemoryUpload() { + interceptor.setAllowedTypes("text/plain"); + + ValidationAwareSupport validation = new ValidationAwareSupport(); + UploadedFile file = StrutsInMemoryUploadedFile.Builder + .create("hello".getBytes(StandardCharsets.UTF_8), tempDir.toPath()) + .withContentType("text/plain") + .withOriginalName("f.txt") + .withInputName("inputName") + .build(); + + boolean ok = interceptor.acceptFile(validation, file, "f.txt", "text/plain", "inputName"); + + assertThat(ok).isTrue(); + assertThat(validation.hasErrors()).isFalse(); + // The optimization: validation must NOT have written the in-memory upload to disk. + assertThat(file.isFile()).isFalse(); + } + + public void testRejectedInMemoryUploadIsStillNotMaterialized() { + interceptor.setAllowedTypes("text/plain"); + + ValidationAwareSupport validation = new ValidationAwareSupport(); + UploadedFile file = StrutsInMemoryUploadedFile.Builder + .create("hello".getBytes(StandardCharsets.UTF_8), tempDir.toPath()) + .withContentType("text/html") + .withOriginalName("f.html") + .withInputName("inputName") + .build(); + + // wrong content type -> rejected + boolean ok = interceptor.acceptFile(validation, file, "f.html", "text/html", "inputName"); + + assertThat(ok).isFalse(); + assertThat(validation.hasErrors()).isTrue(); + // Even on rejection, no disk write happened. + assertThat(file.isFile()).isFalse(); + } + public void testAcceptFileWithMaxSize() throws Exception { interceptor.setMaximumSize(10L); From e46732371745951d0dc2bed33b28add4e0e4fb89 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Wed, 22 Jul 2026 14:20:23 +0200 Subject: [PATCH 09/13] WW-5413 chore(core): clean up partial materialization and cover isMissing() --- .../multipart/StrutsInMemoryUploadedFile.java | 11 ++++++++++ .../StrutsInMemoryUploadedFileTest.java | 9 +++++++++ .../multipart/UploadedFileTest.java | 20 +++++++++++++++++++ 3 files changed, 40 insertions(+) diff --git a/core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFile.java b/core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFile.java index 22df11e893..5615a5c808 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFile.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFile.java @@ -40,6 +40,12 @@ * temporary file uses the secure {@code upload_.tmp} naming and ignores the user-supplied * original filename.

    * + *

    Clustered deployments: the target temporary path is resolved on the node that + * created this instance. If an un-materialized instance is serialized (for example, session + * replication) and deserialized on another node, a later {@link #getContent()} materializes to that + * originating node's path, which may not exist on the new node. Read via {@link #getInputStream()}, + * which never touches disk, when content must survive cross-node replication.

    + * * @since 7.3.0 */ public class StrutsInMemoryUploadedFile implements UploadedFile { @@ -71,6 +77,11 @@ private synchronized File materialize() { try { Files.write(targetFile.toPath(), content); } catch (IOException e) { + try { + Files.deleteIfExists(targetFile.toPath()); + } catch (IOException suppressed) { + e.addSuppressed(suppressed); + } throw new StrutsException("Could not materialize in-memory uploaded file: " + targetFile.getName(), e); } materializedFile = targetFile; diff --git a/core/src/test/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFileTest.java b/core/src/test/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFileTest.java index 96d5741197..eb4af0cec7 100644 --- a/core/src/test/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFileTest.java +++ b/core/src/test/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFileTest.java @@ -145,6 +145,15 @@ public void maliciousOriginalNameDoesNotLeakIntoTempName() { assertThat(new File(saveDir.toFile(), file.getName())).exists(); } + @Test + public void isMissingIsFalseWithoutMaterializing() { + UploadedFile file = build("x".getBytes(UTF_8)); + + assertThat(file.isMissing()).isFalse(); + assertThat(file.isFile()).isFalse(); // did not materialize + assertThat(tempFolder.getRoot().listFiles()).isEmpty(); + } + @Test public void isSerializableWhenNotMaterialized() throws IOException, ClassNotFoundException { UploadedFile file = build("payload".getBytes(UTF_8)); diff --git a/core/src/test/java/org/apache/struts2/dispatcher/multipart/UploadedFileTest.java b/core/src/test/java/org/apache/struts2/dispatcher/multipart/UploadedFileTest.java index ec96ce329a..1885031667 100644 --- a/core/src/test/java/org/apache/struts2/dispatcher/multipart/UploadedFileTest.java +++ b/core/src/test/java/org/apache/struts2/dispatcher/multipart/UploadedFileTest.java @@ -46,4 +46,24 @@ public void defaultGetInputStreamReadsByteArrayContent() throws IOException { assertThat(new String(in.readAllBytes(), UTF_8)).isEqualTo("abc"); } } + + @Test + public void defaultIsMissingReflectsContent() { + assertThat(uploadedFileReturning("abc".getBytes(UTF_8)).isMissing()).isFalse(); + assertThat(uploadedFileReturning(null).isMissing()).isTrue(); + } + + private static UploadedFile uploadedFileReturning(Object content) { + return new UploadedFile() { + public Long length() { return 0L; } + public String getName() { return "x"; } + public String getOriginalName() { return "x"; } + public boolean isFile() { return false; } + public boolean delete() { return true; } + public String getAbsolutePath() { return null; } + public Object getContent() { return content; } + public String getContentType() { return "text/plain"; } + public String getInputName() { return "file"; } + }; + } } From 31acd34bfb9b2ed162ee2c2d0dfc1f3f743f90cf Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Wed, 22 Jul 2026 14:22:55 +0200 Subject: [PATCH 10/13] WW-5413 docs(core): sync design/plan with interceptor fix and deviations Co-Authored-By: Claude Opus 4.8 --- ...-22-WW-5413-inmemory-upload-optimization.md | 18 ++++++++++++++++++ ...5413-inmemory-upload-optimization-design.md | 14 ++++++++++++-- 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/docs/superpowers/plans/2026-07-22-WW-5413-inmemory-upload-optimization.md b/docs/superpowers/plans/2026-07-22-WW-5413-inmemory-upload-optimization.md index 1a98bb67f7..1d78105979 100644 --- a/docs/superpowers/plans/2026-07-22-WW-5413-inmemory-upload-optimization.md +++ b/docs/superpowers/plans/2026-07-22-WW-5413-inmemory-upload-optimization.md @@ -807,3 +807,21 @@ Expected: PASS. Pay attention to `UploadedFileConverter`-related tests (legacy ` ## Notes on the spec's error-handling trade-off (§4) The spec deliberately accepts that a materialization **write failure** now surfaces as an unchecked `StrutsException` during consumption (via `getContent()`/`getAbsolutePath()`), rather than as a parse-time `LocalizedMessage`. This is why the old `temporaryFileCreationFailureAddsError` test is removed rather than rewritten: the parse-time graceful-degradation path for in-memory items no longer exists. No new test asserts the exception, since it only fires on genuine filesystem failures in the save directory. + +--- + +## Task 5: Non-materializing interceptor validation (added after whole-branch review) + +The whole-branch review found `AbstractFileUploadInterceptor.acceptFile()` calls `file.getContent() == null` first, materializing every in-memory upload during validation on the standard `fileUpload`/`actionFileUpload` path — defeating the optimization for the common consumer. + +**Files:** `UploadedFile.java`, `StrutsInMemoryUploadedFile.java`, `AbstractFileUploadInterceptor.java`, `ActionFileUploadInterceptorTest.java`. + +Steps: add `default boolean isMissing()` (= `getContent() == null`) to `UploadedFile`; override in `StrutsInMemoryUploadedFile` to `return false` (non-materializing); change the `acceptFile()` guard to `file == null || file.isMissing()` and delete the dead `getContent() == null` block; add JUnit3-style (`public void testXxx`) interceptor tests asserting `isFile()==false` after accept and after reject. Preserve `testAcceptFileWithNoContent` (null-content test-double rejected via the interface default). Commit: `WW-5413 perf(core): avoid materializing in-memory uploads during interceptor validation`. + +## Task 6: Polish (from whole-branch review) + +**Files:** `StrutsInMemoryUploadedFile.java`, `StrutsInMemoryUploadedFileTest.java`, `UploadedFileTest.java`. + +Steps: in `materialize()`, `Files.deleteIfExists(targetFile.toPath())` (with `addSuppressed`) before rethrowing `StrutsException`, so a partial write on failure isn't leaked; add a class Javadoc note on the clustered-deployment serialization limitation; add direct unit tests for `isMissing()` (the override is non-materializing; the interface default reflects `getContent()`). Commit: `WW-5413 chore(core): clean up partial materialization and cover isMissing()`. + +> Note left out of scope deliberately: the now-unused `STRUTS_MESSAGES_INVALID_CONTENT_TYPE_KEY` constant in `AbstractFileUploadInterceptor` is a `public static final` and was left in place to avoid an API break. The vestigial `throws IOException` on `JakartaMultiPartRequest.processFileField` was also left in place — removing it could break a subclass that catches `IOException` from `super.processFileField(...)`. diff --git a/docs/superpowers/specs/2026-07-22-WW-5413-inmemory-upload-optimization-design.md b/docs/superpowers/specs/2026-07-22-WW-5413-inmemory-upload-optimization-design.md index b7ef076cb5..e84237d125 100644 --- a/docs/superpowers/specs/2026-07-22-WW-5413-inmemory-upload-optimization-design.md +++ b/docs/superpowers/specs/2026-07-22-WW-5413-inmemory-upload-optimization-design.md @@ -51,7 +51,7 @@ A second `UploadedFile` implementation beside `StrutsUploadedFile` — each clas - metadata: `contentType`, `originalName`, `inputName` - `transient File materializedFile` — populated lazily, cached -`byte[]` + `Path` keep the object `Serializable` (a `DiskFileItem` reference would not). +The object must stay `Serializable` (a `DiskFileItem` reference would not be). **Implementation note:** rather than a `Path saveDir` + `String name` (a concrete `Path` such as `sun.nio.fs.UnixPath` is *not* guaranteed `Serializable`), the shipped code stores a single pre-computed `java.io.File targetFile` (which *is* `Serializable`) plus a `serialVersionUID`; `materializedFile` is `volatile transient`. | Method | Behavior | Touches disk? | |---|---|---| @@ -68,6 +68,12 @@ A second `UploadedFile` implementation beside `StrutsUploadedFile` — each clas **Net effect:** rejected uploads, size/type checks (`length()`), and `getInputStream()` consumers **never write**; legacy `File`/`getContent()` consumers write exactly once — same as today, but deferred. +### 3a. Non-materializing interceptor validation (added after whole-branch review) + +The net-effect claim above is only real if the framework's own consumers don't force materialization during validation. They did: `AbstractFileUploadInterceptor.acceptFile()` — run for every uploaded file on the standard `fileUpload`/`actionFileUpload` path — called `file.getContent() == null` as its first (failed-upload) guard, which materialized every small in-memory upload before any size/type check. `acceptFile()` only validates metadata; it never needs the bytes. + +Fix: add `default boolean isMissing()` to `UploadedFile` (default = `getContent() == null`, so third-party impls and the "no content = failed upload" contract are preserved), override it in `StrutsInMemoryUploadedFile` to return `false` (answered from the in-memory byte array, no materialization), change the `acceptFile()` guard to `file == null || file.isMissing()`, and delete the now-dead second `getContent() == null` block. Result: the `UploadedFilesAware` flow validates and hands files to the action without ever writing a small upload to disk; only a consumer that explicitly asks for a `File` (the legacy `File`-typed action property via `UploadedFileConverter`, which genuinely needs one) triggers the write. + ### 3. `JakartaMultiPartRequest.processFileField` + cleanup simplification The `item.isInMemory()` branch stops writing a temp file eagerly: @@ -121,6 +127,10 @@ Tests follow the existing multipart test base (core tests are JUnit4 / `XWorkTes ## Out of scope -- Migrating existing consumers (showcase actions, converter) onto `getInputStream()` — they continue using `getContent()`. +- Migrating showcase actions / `UploadedFileConverter` onto `getInputStream()` — they continue using `getContent()`. (The upload **interceptor** validation path *was* brought in scope and made non-materializing — see §3a — because it defeated the optimization for the common consumer.) - Changing the disk-spill threshold or `JakartaStreamMultiPartRequest` streaming strategy. - Any change to `getContent()`'s runtime type (must remain `File`). + +## Known limitation + +`StrutsInMemoryUploadedFile.targetFile` is an absolute path resolved on the originating node. If an un-materialized instance is serialized (e.g. session replication) and deserialized on another node, a later `getContent()` materializes to that originating node's path, which may not exist there. Consumers that need content to survive cross-node replication should read via `getInputStream()` (never touches disk). Documented on the class Javadoc. From da6588e6c18d4d486f8bbf2d608fb689bea8d4cd Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Wed, 22 Jul 2026 14:35:26 +0200 Subject: [PATCH 11/13] WW-5413 chore(core): deprecate now-unused STRUTS_MESSAGES_INVALID_CONTENT_TYPE_KEY Mark the orphaned constant @Deprecated(forRemoval = true) instead of leaving it silently unused. The message key it referenced was only emitted from an unreachable block in acceptFile() that was removed with the in-memory upload optimization. Co-Authored-By: Claude Opus 4.8 --- .../struts2/interceptor/AbstractFileUploadInterceptor.java | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/core/src/main/java/org/apache/struts2/interceptor/AbstractFileUploadInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/AbstractFileUploadInterceptor.java index 3e533754a1..c7cbb65c8d 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/AbstractFileUploadInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/AbstractFileUploadInterceptor.java @@ -48,6 +48,13 @@ public abstract class AbstractFileUploadInterceptor extends AbstractInterceptor public static final String STRUTS_MESSAGES_ERROR_UPLOADING_KEY = "struts.messages.error.uploading"; public static final String STRUTS_MESSAGES_ERROR_FILE_TOO_LARGE_KEY = "struts.messages.error.file.too.large"; public static final String STRUTS_MESSAGES_INVALID_FILE_KEY = "struts.messages.invalid.file"; + /** + * @deprecated since 7.3.0, no longer used. The unreachable content-type null-check in + * {@code acceptFile()} that referenced this key was removed as part of the in-memory upload + * optimization (WW-5413); there is no replacement. This constant will be removed in a future + * version. + */ + @Deprecated(since = "7.3.0", forRemoval = true) public static final String STRUTS_MESSAGES_INVALID_CONTENT_TYPE_KEY = "struts.messages.invalid.content.type"; public static final String STRUTS_MESSAGES_ERROR_CONTENT_TYPE_NOT_ALLOWED_KEY = "struts.messages.error.content.type.not.allowed"; public static final String STRUTS_MESSAGES_ERROR_FILE_EXTENSION_NOT_ALLOWED_KEY = "struts.messages.error.file.extension.not.allowed"; From 41762c8e406343b79a6ea37552073dafd007b58a Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Wed, 22 Jul 2026 19:13:22 +0200 Subject: [PATCH 12/13] WW-5413 test(core): cover materialization failure and getInputStream default branches Address review follow-ups on PR #1805: - document that processFileField's retained 'throws IOException' is intentional (subclass source compatibility), not an oversight - add a negative test: getContent() on an unwritable save dir throws StrutsException, stays unmaterialized, and leaves no partial file behind - cover the UploadedFile.getInputStream() default File branch and the no-content IOException branch Co-Authored-By: Claude Opus 4.8 --- .../multipart/JakartaMultiPartRequest.java | 6 ++++- .../StrutsInMemoryUploadedFileTest.java | 21 ++++++++++++++++++ .../multipart/UploadedFileTest.java | 22 +++++++++++++++++++ 3 files changed, 48 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java b/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java index 061ecc593c..7177aaaeaa 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java @@ -182,8 +182,12 @@ protected void processNormalFormField(DiskFileItem item, Charset charset) throws * * * @param item the disk file item representing the uploaded file - * @throws IOException if an error occurs reading the in-memory item's content + * @throws IOException never thrown by this implementation (in-memory content is materialized + * lazily elsewhere); the clause is retained on the signature deliberately for + * source compatibility with subclasses that override this method or catch it + * from {@code super.processFileField(...)} */ + // NOTE: `throws IOException` is intentionally retained for subclass source compatibility - do not remove. protected void processFileField(DiskFileItem item, String saveDir) throws IOException { // Skip file uploads that don't have a file name - meaning that no file was selected. if (item.getName() == null || item.getName().trim().isEmpty()) { diff --git a/core/src/test/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFileTest.java b/core/src/test/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFileTest.java index eb4af0cec7..fc09f9cc33 100644 --- a/core/src/test/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFileTest.java +++ b/core/src/test/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFileTest.java @@ -32,8 +32,11 @@ import java.io.ObjectOutputStream; import java.nio.file.Path; +import org.apache.struts2.StrutsException; + import static java.nio.charset.StandardCharsets.UTF_8; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; public class StrutsInMemoryUploadedFileTest { @@ -68,6 +71,24 @@ public void getInputStreamReturnsBytesWithoutWritingFile() throws IOException { assertThat(tempFolder.getRoot().listFiles()).isEmpty(); } + @Test + public void getContentThrowsStrutsExceptionAndLeavesNoFileWhenWriteFails() { + // save directory does not exist -> Files.write in materialize() fails + Path missingDir = tempFolder.getRoot().toPath().resolve("no-such-dir"); + UploadedFile file = StrutsInMemoryUploadedFile.Builder + .create("x".getBytes(UTF_8), missingDir) + .withOriginalName("orig.txt") + .withContentType("text/plain") + .withInputName("file") + .build(); + + assertThatThrownBy(file::getContent).isInstanceOf(StrutsException.class); + + assertThat(file.isFile()).isFalse(); // not marked materialized + assertThat(missingDir.toFile()).doesNotExist(); // no partial file left behind + assertThat(tempFolder.getRoot().listFiles()).isEmpty(); // nothing leaked into the save root + } + @Test public void getContentMaterializesFileExactlyOnce() { UploadedFile file = build("data".getBytes(UTF_8)); diff --git a/core/src/test/java/org/apache/struts2/dispatcher/multipart/UploadedFileTest.java b/core/src/test/java/org/apache/struts2/dispatcher/multipart/UploadedFileTest.java index 1885031667..49282cffa1 100644 --- a/core/src/test/java/org/apache/struts2/dispatcher/multipart/UploadedFileTest.java +++ b/core/src/test/java/org/apache/struts2/dispatcher/multipart/UploadedFileTest.java @@ -20,11 +20,14 @@ import org.junit.Test; +import java.io.File; import java.io.IOException; import java.io.InputStream; +import java.nio.file.Files; import static java.nio.charset.StandardCharsets.UTF_8; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; public class UploadedFileTest { @@ -47,6 +50,25 @@ public void defaultGetInputStreamReadsByteArrayContent() throws IOException { } } + @Test + public void defaultGetInputStreamReadsFileContent() throws IOException { + File f = File.createTempFile("upload_", ".tmp"); + try { + Files.writeString(f.toPath(), "hi"); + try (InputStream in = uploadedFileReturning(f).getInputStream()) { + assertThat(new String(in.readAllBytes(), UTF_8)).isEqualTo("hi"); + } + } finally { + assertThat(f.delete()).isTrue(); + } + } + + @Test + public void defaultGetInputStreamThrowsWhenNoContent() { + assertThatThrownBy(uploadedFileReturning(null)::getInputStream) + .isInstanceOf(IOException.class); + } + @Test public void defaultIsMissingReflectsContent() { assertThat(uploadedFileReturning("abc".getBytes(UTF_8)).isMissing()).isFalse(); From d32a533d1209b2d5e7addc0017ad462480a43e33 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Wed, 22 Jul 2026 19:38:47 +0200 Subject: [PATCH 13/13] WW-5413 fix(core): address SonarCloud and Copilot review findings - materialize() now writes with StandardOpenOption.CREATE_NEW and fails closed if the target already exists, so a pre-planted file/symlink is never overwritten or followed (Copilot security note) + regression test - defensively copy the content byte array on construction and reject null content, so the instance owns its bytes and cannot observe caller mutation (Copilot / review) - delete() uses Files.deleteIfExists and logs the real cause on failure instead of a silent File.delete() boolean (Sonar MAJOR) - reorder field modifiers to JLS order 'transient volatile' (Sonar) - tests: assertThat(dir).isEmptyDirectory() instead of listFiles().isEmpty() (Sonar); drop unused DiskFileItem import (Sonar) Co-Authored-By: Claude Opus 4.8 --- .../multipart/StrutsInMemoryUploadedFile.java | 27 ++++++++++++++----- .../JakartaMultiPartRequestTest.java | 1 - .../StrutsInMemoryUploadedFileTest.java | 26 ++++++++++++++---- 3 files changed, 42 insertions(+), 12 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFile.java b/core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFile.java index 5615a5c808..5ce01cf88d 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFile.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFile.java @@ -26,8 +26,11 @@ import java.io.File; import java.io.IOException; import java.io.InputStream; +import java.nio.file.FileAlreadyExistsException; import java.nio.file.Files; import java.nio.file.Path; +import java.nio.file.StandardOpenOption; +import java.util.Objects; import java.util.UUID; /** @@ -60,11 +63,12 @@ public class StrutsInMemoryUploadedFile implements UploadedFile { private final String originalName; private final String inputName; - private volatile transient File materializedFile; + private transient volatile File materializedFile; private StrutsInMemoryUploadedFile(byte[] content, Path saveDir, String contentType, String originalName, String inputName) { - this.content = content; + // Defensive copy: the instance owns its bytes so callers cannot mutate content after construction. + this.content = Objects.requireNonNull(content, "content").clone(); String name = "upload_" + UUID.randomUUID().toString().replace("-", "_") + ".tmp"; this.targetFile = saveDir.resolve(name).toFile(); this.contentType = contentType; @@ -75,8 +79,14 @@ private StrutsInMemoryUploadedFile(byte[] content, Path saveDir, String contentT private synchronized File materialize() { if (materializedFile == null) { try { - Files.write(targetFile.toPath(), content); + // CREATE_NEW fails closed if the target already exists, so we never overwrite or + // follow a pre-planted file/symlink in the upload directory. + Files.write(targetFile.toPath(), content, StandardOpenOption.CREATE_NEW); + } catch (FileAlreadyExistsException e) { + // A file already occupies the target path; do not touch it (possible planted file/symlink). + throw new StrutsException("Refusing to overwrite existing file while materializing in-memory uploaded file: " + targetFile.getName(), e); } catch (IOException e) { + // Remove a partial file this call may have created before rethrowing. try { Files.deleteIfExists(targetFile.toPath()); } catch (IOException suppressed) { @@ -119,10 +129,15 @@ public boolean isFile() { @Override public boolean delete() { File f = materializedFile; - if (f != null) { - return f.delete(); + if (f == null) { + return true; + } + try { + return Files.deleteIfExists(f.toPath()); + } catch (IOException e) { + LOG.warn("Could not delete materialized in-memory uploaded file: {}", f.getAbsolutePath(), e); + return false; } - return true; } @Override diff --git a/core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequestTest.java b/core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequestTest.java index b2263a6b67..05af17a705 100644 --- a/core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequestTest.java +++ b/core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequestTest.java @@ -18,7 +18,6 @@ */ package org.apache.struts2.dispatcher.multipart; -import org.apache.commons.fileupload2.core.DiskFileItem; import org.apache.struts2.dispatcher.LocalizedMessage; import org.assertj.core.api.InstanceOfAssertFactories; import org.junit.Test; diff --git a/core/src/test/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFileTest.java b/core/src/test/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFileTest.java index fc09f9cc33..8b220963a3 100644 --- a/core/src/test/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFileTest.java +++ b/core/src/test/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFileTest.java @@ -68,7 +68,7 @@ public void getInputStreamReturnsBytesWithoutWritingFile() throws IOException { } assertThat(file.isFile()).isFalse(); - assertThat(tempFolder.getRoot().listFiles()).isEmpty(); + assertThat(tempFolder.getRoot()).isEmptyDirectory(); } @Test @@ -86,7 +86,23 @@ public void getContentThrowsStrutsExceptionAndLeavesNoFileWhenWriteFails() { assertThat(file.isFile()).isFalse(); // not marked materialized assertThat(missingDir.toFile()).doesNotExist(); // no partial file left behind - assertThat(tempFolder.getRoot().listFiles()).isEmpty(); // nothing leaked into the save root + assertThat(tempFolder.getRoot()).isEmptyDirectory(); // nothing leaked into the save root + } + + @Test + public void getContentFailsClosedWhenTargetAlreadyExists() throws IOException { + UploadedFile file = build("x".getBytes(UTF_8)); + + // Pre-plant a file at the exact target name (simulates a collision or planted file/symlink). + File planted = new File(tempFolder.getRoot(), file.getName()); + java.nio.file.Files.writeString(planted.toPath(), "pre-existing"); + + assertThatThrownBy(file::getContent).isInstanceOf(StrutsException.class); + + // The pre-existing file must be neither overwritten nor deleted. + assertThat(planted).exists(); + assertThat(java.nio.file.Files.readString(planted.toPath())).isEqualTo("pre-existing"); + assertThat(file.isFile()).isFalse(); } @Test @@ -131,7 +147,7 @@ public void lengthAndMetadataDoNotMaterialize() { assertThat(file.getName()).startsWith("upload_").endsWith(".tmp"); assertThat(file.isFile()).isFalse(); - assertThat(tempFolder.getRoot().listFiles()).isEmpty(); + assertThat(tempFolder.getRoot()).isEmptyDirectory(); } @Test @@ -149,7 +165,7 @@ public void deleteIsNoOpWhenNotMaterialized() { UploadedFile file = build("x".getBytes(UTF_8)); assertThat(file.delete()).isTrue(); - assertThat(tempFolder.getRoot().listFiles()).isEmpty(); + assertThat(tempFolder.getRoot()).isEmptyDirectory(); } @Test @@ -172,7 +188,7 @@ public void isMissingIsFalseWithoutMaterializing() { assertThat(file.isMissing()).isFalse(); assertThat(file.isFile()).isFalse(); // did not materialize - assertThat(tempFolder.getRoot().listFiles()).isEmpty(); + assertThat(tempFolder.getRoot()).isEmptyDirectory(); } @Test