From c3e9db1b21fc9e8453c654e2f7a18e13bf3cf8d3 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sun, 26 Jul 2026 04:29:05 +0200 Subject: [PATCH] WW-5413 Avoid writing small in-memory multipart uploads to disk (#1805) * 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 * WW-5413 docs(core): implementation plan for in-memory upload optimization Co-Authored-By: Claude Opus 4.8 * WW-5413 feat(core): add UploadedFile.getInputStream() streaming accessor Co-Authored-By: Claude Opus 4.8 * WW-5413 feat(core): add lazily-materializing StrutsInMemoryUploadedFile Co-Authored-By: Claude Opus 4.8 * WW-5413 fix(core): make StrutsInMemoryUploadedFile serializable and thread-safe * WW-5413 refactor(core): drop eager temp-file write for in-memory uploads * WW-5413 test(core): cover deferred-write behavior for in-memory uploads Co-Authored-By: Claude Opus 4.8 * WW-5413 perf(core): avoid materializing in-memory uploads during interceptor validation * WW-5413 chore(core): clean up partial materialization and cover isMissing() * WW-5413 docs(core): sync design/plan with interceptor fix and deviations Co-Authored-By: Claude Opus 4.8 * 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 * 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 * 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 --------- Co-authored-by: Claude Opus 4.8 --- .../multipart/JakartaMultiPartRequest.java | 123 +-- .../multipart/StrutsInMemoryUploadedFile.java | 213 +++++ .../multipart/StrutsUploadedFile.java | 8 + .../dispatcher/multipart/UploadedFile.java | 38 + .../AbstractFileUploadInterceptor.java | 16 +- .../AbstractMultiPartRequestTest.java | 20 +- .../JakartaMultiPartRequestTest.java | 217 +---- .../StrutsInMemoryUploadedFileTest.java | 217 +++++ .../multipart/StrutsUploadedFileTest.java | 49 ++ .../multipart/UploadedFileTest.java | 91 ++ .../ActionFileUploadInterceptorTest.java | 40 + ...22-WW-5413-inmemory-upload-optimization.md | 827 ++++++++++++++++++ ...413-inmemory-upload-optimization-design.md | 136 +++ 13 files changed, 1702 insertions(+), 293 deletions(-) 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 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 create mode 100644 docs/superpowers/plans/2026-07-22-WW-5413-inmemory-upload-optimization.md create mode 100644 docs/superpowers/specs/2026-07-22-WW-5413-inmemory-upload-optimization-design.md 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 6b19962eb..7177aaaea 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.fileupload2.jakarta.servlet6.JakartaServletDiskFileUpl 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 @@ import static org.apache.commons.lang3.StringUtils.normalizeSpace; * 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,21 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { *
      *
    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 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(...)} */ - protected void processFileField(DiskFileItem item, String saveDir) { + // 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()) { LOG.debug(() -> "No file has been uploaded for the field: " + normalizeSpace(item.getFieldName())); @@ -215,40 +209,14 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { 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 +244,8 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { * 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 +266,26 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { } } - /** - * 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 +293,8 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { super.cleanUp(); try { cleanUpDiskFileItems(); - cleanUpTemporaryFiles(); } finally { diskFileItems.clear(); - temporaryFiles.clear(); } } 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 000000000..5ce01cf88 --- /dev/null +++ b/core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFile.java @@ -0,0 +1,213 @@ +/* + * 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.FileAlreadyExistsException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.StandardOpenOption; +import java.util.Objects; +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.

    + * + *

    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 { + + private static final long serialVersionUID = 1L; + + private static final Logger LOG = LogManager.getLogger(StrutsInMemoryUploadedFile.class); + + private final byte[] content; + private final File targetFile; + private final String contentType; + private final String originalName; + private final String inputName; + + private transient volatile File materializedFile; + + private StrutsInMemoryUploadedFile(byte[] content, Path saveDir, String contentType, + String originalName, String inputName) { + // 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; + this.originalName = originalName; + this.inputName = inputName; + } + + private synchronized File materialize() { + if (materializedFile == null) { + try { + // 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) { + e.addSuppressed(suppressed); + } + throw new StrutsException("Could not materialize in-memory uploaded file: " + targetFile.getName(), e); + } + materializedFile = targetFile; + LOG.debug("Materialized in-memory uploaded item to {}", targetFile.getAbsolutePath()); + } + return materializedFile; + } + + @Override + public InputStream getInputStream() { + return new ByteArrayInputStream(content); + } + + @Override + public boolean isMissing() { + return false; + } + + @Override + public Long length() { + return (long) content.length; + } + + @Override + public String getName() { + return targetFile.getName(); + } + + @Override + public boolean isFile() { + File f = materializedFile; + return f != null && f.isFile(); + } + + @Override + public boolean delete() { + File f = materializedFile; + 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; + } + } + + @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/main/java/org/apache/struts2/dispatcher/multipart/StrutsUploadedFile.java b/core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsUploadedFile.java index 492b46fb0..418808945 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 class StrutsUploadedFile implements UploadedFile { 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 c0af59742..590d1e0c0 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,37 @@ 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()); + } + + /** + * 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 ee698721b..c7cbb65c8 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"; @@ -114,8 +121,8 @@ public abstract class AbstractFileUploadInterceptor extends AbstractInterceptor 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 +131,6 @@ public abstract class AbstractFileUploadInterceptor extends AbstractInterceptor 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/dispatcher/multipart/AbstractMultiPartRequestTest.java b/core/src/test/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequestTest.java index 6a2450201..c20b72b6d 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 @@ abstract class AbstractMultiPartRequestTest { .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 @@ abstract class AbstractMultiPartRequestTest { .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 @@ abstract class AbstractMultiPartRequestTest { .exists() .content() .isEqualTo("5,6,7,8"); + assertThat(file.isFile()) + .isTrue(); }); } @@ -258,8 +258,6 @@ abstract class AbstractMultiPartRequestTest { .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 @@ abstract class AbstractMultiPartRequestTest { .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 @@ abstract class AbstractMultiPartRequestTest { .exists() .content() .isEqualTo("5,6,7,8"); + assertThat(file.isFile()) + .isTrue(); }); List uploadedFiles = new ArrayList<>(); @@ -431,8 +431,6 @@ abstract class AbstractMultiPartRequestTest { .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 @@ abstract class AbstractMultiPartRequestTest { .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 15b59f5dd..05af17a70 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,18 +18,15 @@ */ 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; import java.io.File; import java.io.IOException; +import java.io.InputStream; 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 +38,6 @@ public class JakartaMultiPartRequestTest extends AbstractMultiPartRequestTest { 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 +106,6 @@ public class JakartaMultiPartRequestTest extends AbstractMultiPartRequestTest { 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 @@ -424,7 +248,7 @@ public class JakartaMultiPartRequestTest extends AbstractMultiPartRequestTest { @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 + @@ -436,12 +260,12 @@ public class JakartaMultiPartRequestTest extends AbstractMultiPartRequestTest { 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); @@ -453,4 +277,35 @@ public class JakartaMultiPartRequestTest extends AbstractMultiPartRequestTest { .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(); + } + } 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 000000000..8b220963a --- /dev/null +++ b/core/src/test/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFileTest.java @@ -0,0 +1,217 @@ +/* + * 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.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 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 { + + @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()).isEmptyDirectory(); + } + + @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()).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 + 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()).isEmptyDirectory(); + } + + @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()).isEmptyDirectory(); + } + + @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(); + } + + @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()).isEmptyDirectory(); + } + + @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"); + } + } +} 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 000000000..23df88dbc --- /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 000000000..49282cffa --- /dev/null +++ b/core/src/test/java/org/apache/struts2/dispatcher/multipart/UploadedFileTest.java @@ -0,0 +1,91 @@ +/* + * 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.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 { + + @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"); + } + } + + @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(); + 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"; } + }; + } +} 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 61339327d..058dabe11 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.ValidationAwareSupport; 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 class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { .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); 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 000000000..1d7810597 --- /dev/null +++ b/docs/superpowers/plans/2026-07-22-WW-5413-inmemory-upload-optimization.md @@ -0,0 +1,827 @@ +# 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. + +--- + +## 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 new file mode 100644 index 000000000..e84237d12 --- /dev/null +++ b/docs/superpowers/specs/2026-07-22-WW-5413-inmemory-upload-optimization-design.md @@ -0,0 +1,136 @@ +# 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 + +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? | +|---|---|---| +| `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. + +### 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: + +```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 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.