From b796bab4320617c41d9409f6a02b1e1db6be51b6 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 23 Jun 2025 01:15:25 +0000 Subject: [PATCH 1/9] Bump org.apache.commons:commons-fileupload2-jakarta-servlet6 Bumps org.apache.commons:commons-fileupload2-jakarta-servlet6 from 2.0.0-M2 to 2.0.0-M4. --- updated-dependencies: - dependency-name: org.apache.commons:commons-fileupload2-jakarta-servlet6 dependency-version: 2.0.0-M4 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] --- parent/pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/parent/pom.xml b/parent/pom.xml index fdbba7cce..e7ddabe20 100644 --- a/parent/pom.xml +++ b/parent/pom.xml @@ -133,7 +133,7 @@ org.apache.commons commons-fileupload2-jakarta-servlet6 - 2.0.0-M2 + 2.0.0-M4 commons-io From a1c4cb60a2a848e2b715eb37fd20ea95c8b9a356 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Wed, 16 Jul 2025 18:34:35 +0200 Subject: [PATCH 2/9] Uses a dedicated RequestContext to avoid NPE --- .../multipart/AbstractMultiPartRequest.java | 30 ++++++- .../multipart/JakartaMultiPartRequest.java | 86 ++++++++++++++----- .../JakartaStreamMultiPartRequest.java | 19 +--- .../multipart/StrutsRequestContext.java | 61 +++++++++++++ 4 files changed, 154 insertions(+), 42 deletions(-) create mode 100644 core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsRequestContext.java diff --git a/core/src/main/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequest.java b/core/src/main/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequest.java index 90ecbe816..038cef4bc 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequest.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequest.java @@ -18,6 +18,8 @@ */ package org.apache.struts2.dispatcher.multipart; +import org.apache.commons.fileupload2.core.DiskFileItemFactory; +import org.apache.commons.fileupload2.core.RequestContext; import org.apache.struts2.inject.Inject; import jakarta.servlet.http.HttpServletRequest; import org.apache.commons.fileupload2.core.FileUploadByteCountLimitException; @@ -43,6 +45,8 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import static org.apache.commons.lang3.StringUtils.normalizeSpace; + /** * Abstract class with some helper methods, it should be used * when starting development of another implementation of {@link MultiPartRequest} @@ -187,7 +191,21 @@ public abstract class AbstractMultiPartRequest implements MultiPartRequest { * @param charset used charset from incoming request * @param saveDir a temporary folder to store uploaded files (not always needed) */ - protected abstract JakartaServletDiskFileUpload createJakartaFileUpload(Charset charset, Path saveDir); + protected JakartaServletDiskFileUpload createJakartaFileUpload(Charset charset, Path saveDir) { + DiskFileItemFactory.Builder builder = DiskFileItemFactory.builder(); + + LOG.debug("Using file save directory: {}", saveDir); + builder.setPath(saveDir); + + LOG.debug("Sets buffer size: {}", bufferSize); + builder.setBufferSize(bufferSize); + + LOG.debug("Using charset: {}", charset); + builder.setCharset(charset); + + DiskFileItemFactory factory = builder.get(); + return new JakartaServletDiskFileUpload(factory); + } protected JakartaServletDiskFileUpload prepareServletFileUpload(Charset charset, Path saveDir) { JakartaServletDiskFileUpload servletFileUpload = createJakartaFileUpload(charset, saveDir); @@ -207,11 +225,15 @@ public abstract class AbstractMultiPartRequest implements MultiPartRequest { return servletFileUpload; } + protected RequestContext createRequestContext(HttpServletRequest request) { + return new StrutsRequestContext(request); + } + protected boolean exceedsMaxStringLength(String fieldName, String fieldValue) { if (maxStringLength != null && fieldValue.length() > maxStringLength) { if (LOG.isDebugEnabled()) { LOG.debug("Form field: {} of size: {} bytes exceeds limit of: {}.", - sanitizeNewlines(fieldName), fieldValue.length(), maxStringLength); + normalizeSpace(fieldName), fieldValue.length(), maxStringLength); } LocalizedMessage localizedMessage = new LocalizedMessage(this.getClass(), STRUTS_MESSAGES_UPLOAD_ERROR_PARAMETER_TOO_LONG_KEY, null, @@ -234,7 +256,7 @@ public abstract class AbstractMultiPartRequest implements MultiPartRequest { try { processUpload(request, saveDir); } catch (FileUploadException e) { - LOG.debug("Error parsing the multi-part request!", e); + LOG.warn("Error parsing the multi-part request!", e); Class exClass = FileUploadException.class; Object[] args = new Object[]{}; @@ -257,7 +279,7 @@ public abstract class AbstractMultiPartRequest implements MultiPartRequest { errors.add(errorMessage); } } catch (IOException e) { - LOG.debug("Unable to parse request", e); + LOG.warn("Unable to parse request", e); LocalizedMessage errorMessage = buildErrorMessage(e.getClass(), e.getMessage(), new Object[]{}); if (!errors.contains(errorMessage)) { errors.add(errorMessage); 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 d2ef13b1d..dbd344b5d 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 @@ -20,12 +20,13 @@ package org.apache.struts2.dispatcher.multipart; import jakarta.servlet.http.HttpServletRequest; import org.apache.commons.fileupload2.core.DiskFileItem; -import org.apache.commons.fileupload2.core.DiskFileItemFactory; +import org.apache.commons.fileupload2.core.RequestContext; import org.apache.commons.fileupload2.jakarta.servlet6.JakartaServletDiskFileUpload; import org.apache.commons.lang3.StringUtils; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; +import java.io.File; import java.io.IOException; import java.nio.charset.Charset; import java.nio.file.Path; @@ -41,6 +42,11 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { private static final Logger LOG = LogManager.getLogger(JakartaMultiPartRequest.class); + /** + * List to track all DiskFileItem instances for proper cleanup + */ + private final List diskFileItems = new ArrayList<>(); + @Override protected void processUpload(HttpServletRequest request, String saveDir) throws IOException { Charset charset = readCharsetEncoding(request); @@ -48,7 +54,12 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { JakartaServletDiskFileUpload servletFileUpload = prepareServletFileUpload(charset, Path.of(saveDir)); - for (DiskFileItem item : servletFileUpload.parseRequest(request)) { + RequestContext requestContext = createRequestContext(request); + + for (DiskFileItem item : servletFileUpload.parseRequest(requestContext)) { + // Track all DiskFileItem instances for cleanup + diskFileItems.add(item); + LOG.debug(() -> "Processing a form field: " + normalizeSpace(item.getFieldName())); if (item.isFormField()) { processNormalFormField(item, charset); @@ -59,23 +70,6 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { } } - @Override - protected JakartaServletDiskFileUpload createJakartaFileUpload(Charset charset, Path saveDir) { - DiskFileItemFactory.Builder builder = DiskFileItemFactory.builder(); - - LOG.debug("Using file save directory: {}", saveDir); - builder.setPath(saveDir); - - LOG.debug("Sets minimal buffer size to always write file to disk"); - builder.setBufferSize(1); - - LOG.debug("Using charset: {}", charset); - builder.setCharset(charset); - - DiskFileItemFactory factory = builder.get(); - return new JakartaServletDiskFileUpload(factory); - } - protected void processNormalFormField(DiskFileItem item, Charset charset) throws IOException { LOG.debug("Item: {} is a normal form field", normalizeSpace(item.getName())); @@ -114,7 +108,30 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { } if (item.isInMemory()) { - LOG.warn(() -> "Storing uploaded files just in memory isn't supported currently, skipping file: %s!".formatted(normalizeSpace(item.getName()))); + LOG.debug("Creating temporary file representing in-memory uploaded item: {}", normalizeSpace(item.getFieldName())); + try { + File tempFile = File.createTempFile("struts_upload_", "_" + item.getName()); + tempFile.deleteOnExit(); // Ensure cleanup on JVM exit + + // 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); + + 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); + } } else { UploadedFile uploadedFile = StrutsUploadedFile.Builder .create(item.getPath().toFile()) @@ -128,4 +145,33 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { uploadedFiles.put(item.getFieldName(), values); } + /** + * Override cleanUp to ensure all DiskFileItem instances are properly cleaned up + */ + @Override + public void cleanUp() { + super.cleanUp(); + try { + LOG.debug("Clean up all DiskFileItem instances (both form fields and file uploads"); + for (DiskFileItem item : diskFileItems) { + try { + if (item.isInMemory()) { + LOG.debug("Cleaning up in-memory item: {}", normalizeSpace(item.getFieldName())); + } else { + LOG.debug("Cleaning up disk item: {} at {}", normalizeSpace(item.getFieldName()), item.getPath()); + if (item.getPath() != null && item.getPath().toFile().exists()) { + if (!item.getPath().toFile().delete()) { + LOG.warn("There was a problem attempting to delete temporary file: {}", item.getPath()); + } + } + } + } catch (Exception e) { + LOG.warn("Error cleaning up DiskFileItem: {}", normalizeSpace(item.getFieldName()), e); + } + } + } finally { + diskFileItems.clear(); + } + } + } diff --git a/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java b/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java index 17b6f377b..769b03e42 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java @@ -19,7 +19,6 @@ package org.apache.struts2.dispatcher.multipart; import jakarta.servlet.http.HttpServletRequest; -import org.apache.commons.fileupload2.core.DiskFileItemFactory; import org.apache.commons.fileupload2.core.FileItemInput; import org.apache.commons.fileupload2.core.FileUploadFileCountLimitException; import org.apache.commons.fileupload2.core.FileUploadSizeException; @@ -45,7 +44,7 @@ import java.util.UUID; import static org.apache.commons.lang3.StringUtils.normalizeSpace; /** - * Multi-part form data request adapter for Jakarta Commons FileUpload package that + * Multipart form data request adapter for Jakarta Commons FileUpload package that * leverages the streaming API rather than the traditional non-streaming API. *

* For more details see WW-3025 @@ -82,22 +81,6 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest { }); } - protected JakartaServletDiskFileUpload createJakartaFileUpload(Charset charset, Path location) { - DiskFileItemFactory.Builder builder = DiskFileItemFactory.builder(); - - LOG.debug("Using file save directory: {}", location); - builder.setPath(location); - - LOG.debug("Sets buffer size: {}", bufferSize); - builder.setBufferSize(bufferSize); - - LOG.debug("Using charset: {}", charset); - builder.setCharset(charset); - - DiskFileItemFactory factory = builder.get(); - return new JakartaServletDiskFileUpload(factory); - } - private String readStream(InputStream inputStream) throws IOException { ByteArrayOutputStream result = new ByteArrayOutputStream(); byte[] buffer = new byte[1024]; diff --git a/core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsRequestContext.java b/core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsRequestContext.java new file mode 100644 index 000000000..1437649cc --- /dev/null +++ b/core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsRequestContext.java @@ -0,0 +1,61 @@ +/* + * 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 jakarta.servlet.http.HttpServletRequest; +import org.apache.commons.fileupload2.jakarta.servlet6.JakartaServletRequestContext; + +/** + * Provides a specialized request context for Struts applications, + * extending the Jakarta Servlet request context to add custom handling + * for multipart-related requests. + *

+ * This class overrides multipart detection logic to ensure that requests + * without a content type are not treated as multipart, improving robustness + * in file upload scenarios. + */ +public class StrutsRequestContext extends JakartaServletRequestContext { + + /** + * Constructs a context for this request. + * + * @param request The request to which this context applies. + */ + public StrutsRequestContext(HttpServletRequest request) { + super(request); + } + + /** + * Determines if the current request is multipart-related. + *

+ * This implementation first checks if the request's content type is set. + * If the content type is {@code null}, it returns {@code false} immediately. + * Otherwise, it delegates to the superclass implementation to perform + * further checks. + * + * @return {@code true} if the request is multipart-related; {@code false} otherwise. + */ + @Override + public boolean isMultipartRelated() { + if (this.getRequest().getContentType() == null) { + return false; + } + return super.isMultipartRelated(); + } +} From 201b9e860e27627759d136d639157d701c959418 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Wed, 16 Jul 2025 18:41:57 +0200 Subject: [PATCH 3/9] Cleans up temporary files --- .../multipart/JakartaMultiPartRequest.java | 78 ++++++++++++++----- 1 file changed, 59 insertions(+), 19 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java b/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java index dbd344b5d..cb17d7022 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 @@ -46,6 +46,11 @@ 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<>(); @Override protected void processUpload(HttpServletRequest request, String saveDir) throws IOException { @@ -111,7 +116,10 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { LOG.debug("Creating temporary file representing in-memory uploaded item: {}", normalizeSpace(item.getFieldName())); try { File tempFile = File.createTempFile("struts_upload_", "_" + item.getName()); - tempFile.deleteOnExit(); // Ensure cleanup on JVM exit + tempFile.deleteOnExit(); // Ensure cleanup on JVM exit as fallback + + // 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)) { @@ -146,31 +154,63 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { } /** - * Override cleanUp to ensure all DiskFileItem instances are properly cleaned up + * Cleans up disk file items by deleting associated temporary files. + * This method can be overridden by subclasses to customize cleanup behavior. + */ + protected void cleanUpDiskFileItems() { + LOG.debug("Clean up all DiskFileItem instances (both form fields and file uploads"); + for (DiskFileItem item : diskFileItems) { + try { + if (item.isInMemory()) { + LOG.debug("Cleaning up in-memory item: {}", normalizeSpace(item.getFieldName())); + } else { + LOG.debug("Cleaning up disk item: {} at {}", normalizeSpace(item.getFieldName()), item.getPath()); + if (item.getPath() != null && item.getPath().toFile().exists()) { + if (!item.getPath().toFile().delete()) { + LOG.warn("There was a problem attempting to delete temporary file: {}", item.getPath()); + } + } + } + } catch (Exception e) { + LOG.warn("Error cleaning up DiskFileItem: {}", normalizeSpace(item.getFieldName()), e); + } + } + } + + /** + * Cleans up temporary files created for in-memory uploads. + * This method can be overridden by subclasses to customize cleanup behavior. + */ + protected void cleanUpTemporaryFiles() { + LOG.debug("Cleaning up {} temporary files created for in-memory uploads", temporaryFiles.size()); + for (File tempFile : temporaryFiles) { + try { + if (tempFile.exists()) { + LOG.debug("Deleting temporary file: {}", tempFile.getAbsolutePath()); + if (!tempFile.delete()) { + LOG.warn("There was a problem attempting to delete temporary file: {}", tempFile.getAbsolutePath()); + } + } else { + LOG.debug("Temporary file already deleted: {}", tempFile.getAbsolutePath()); + } + } catch (Exception e) { + LOG.warn("Error cleaning up temporary file: {}", tempFile.getAbsolutePath(), e); + } + } + } + + /** + * Override cleanUp to ensure all DiskFileItem instances and temporary files are properly cleaned up */ @Override public void cleanUp() { super.cleanUp(); try { - LOG.debug("Clean up all DiskFileItem instances (both form fields and file uploads"); - for (DiskFileItem item : diskFileItems) { - try { - if (item.isInMemory()) { - LOG.debug("Cleaning up in-memory item: {}", normalizeSpace(item.getFieldName())); - } else { - LOG.debug("Cleaning up disk item: {} at {}", normalizeSpace(item.getFieldName()), item.getPath()); - if (item.getPath() != null && item.getPath().toFile().exists()) { - if (!item.getPath().toFile().delete()) { - LOG.warn("There was a problem attempting to delete temporary file: {}", item.getPath()); - } - } - } - } catch (Exception e) { - LOG.warn("Error cleaning up DiskFileItem: {}", normalizeSpace(item.getFieldName()), e); - } - } + cleanUpDiskFileItems(); + cleanUpTemporaryFiles(); } finally { diskFileItems.clear(); + temporaryFiles.clear(); } } From 359b6549ef78ce7b2de09d9d89f90e58c434cc3f Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Wed, 16 Jul 2025 18:46:23 +0200 Subject: [PATCH 4/9] Fixes readStream method to avoid to memory leaks --- .../multipart/JakartaMultiPartRequest.java | 7 + .../JakartaStreamMultiPartRequest.java | 11 +- .../AbstractMultiPartRequestTest.java | 95 +++++++++ .../JakartaMultiPartRequestTest.java | 194 ++++++++++++++++++ .../JakartaStreamMultiPartRequestTest.java | 90 ++++++++ 5 files changed, 392 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java b/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java index cb17d7022..56f9e0a49 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,6 +25,7 @@ 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; @@ -139,6 +140,12 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { } 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); + } } } else { UploadedFile uploadedFile = StrutsUploadedFile.Builder diff --git a/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java b/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java index 769b03e42..5dba4fdd3 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java @@ -82,12 +82,13 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest { } private String readStream(InputStream inputStream) throws IOException { - ByteArrayOutputStream result = new ByteArrayOutputStream(); - byte[] buffer = new byte[1024]; - for (int length; (length = inputStream.read(buffer)) != -1; ) { - result.write(buffer, 0, length); + try (ByteArrayOutputStream result = new ByteArrayOutputStream()) { + byte[] buffer = new byte[1024]; + for (int length; (length = inputStream.read(buffer)) != -1; ) { + result.write(buffer, 0, length); + } + return result.toString(StandardCharsets.UTF_8); } - return result.toString(StandardCharsets.UTF_8); } /** 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 0b4bd8e56..d968ec256 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 @@ -491,6 +491,101 @@ abstract class AbstractMultiPartRequestTest { .containsExactly("struts.messages.upload.error.FileUploadException"); } + @Test + public void cleanupDoesNotClearErrorsList() throws IOException { + // given - create a scenario that generates errors + String content = formFile("file1", "test1.csv", "1,2,3,4"); + mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8)); + + multiPart.setMaxSize("1"); // Very small to trigger error + multiPart.parse(mockRequest, tempDir); + + // Verify errors exist + assertThat(multiPart.getErrors()).isNotEmpty(); + int originalErrorCount = multiPart.getErrors().size(); + + // when + multiPart.cleanUp(); + + // then - errors should remain (cleanup doesn't clear errors) + assertThat(multiPart.getErrors()).hasSize(originalErrorCount); + } + + @Test + public void largeFileUploadHandling() throws IOException { + // Test that large files are handled properly + StringBuilder largeContent = new StringBuilder(); + for (int i = 0; i < 1000; i++) { + largeContent.append("line").append(i).append(","); + } + + String content = formFile("largefile", "large.csv", largeContent.toString()) + + endline + "--" + boundary + "--"; + + mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8)); + + // when + multiPart.parse(mockRequest, tempDir); + + // then - should complete without memory issues + assertThat(multiPart.getErrors()).isEmpty(); + assertThat(multiPart.getFile("largefile")).hasSize(1); + + // Cleanup should properly handle large files + multiPart.cleanUp(); + assertThat(multiPart.uploadedFiles).isEmpty(); + } + + @Test + public void multipleFileUploadWithMixedContent() throws IOException { + // Test mixed content with multiple files and parameters + String content = formFile("file1", "test1.csv", "1,2,3,4") + + formField("param1", "value1") + + formFile("file2", "test2.csv", "5,6,7,8") + + formField("param2", "value2") + + formFile("file3", "test3.csv", "9,10,11,12") + + formField("param3", "value3") + + endline + "--" + boundary + "--"; + + mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8)); + + // when + multiPart.parse(mockRequest, tempDir); + + // then - verify all content was processed + assertThat(multiPart.getErrors()).isEmpty(); + assertThat(multiPart.getFile("file1")).hasSize(1); + assertThat(multiPart.getFile("file2")).hasSize(1); + assertThat(multiPart.getFile("file3")).hasSize(1); + assertThat(multiPart.getParameter("param1")).isEqualTo("value1"); + assertThat(multiPart.getParameter("param2")).isEqualTo("value2"); + assertThat(multiPart.getParameter("param3")).isEqualTo("value3"); + + // Store file paths for post-cleanup verification + List filePaths = new ArrayList<>(); + for (UploadedFile file : multiPart.getFile("file1")) { + filePaths.add(file.getAbsolutePath()); + } + for (UploadedFile file : multiPart.getFile("file2")) { + filePaths.add(file.getAbsolutePath()); + } + for (UploadedFile file : multiPart.getFile("file3")) { + filePaths.add(file.getAbsolutePath()); + } + + // when - cleanup + multiPart.cleanUp(); + + // then - verify complete cleanup + assertThat(multiPart.uploadedFiles).isEmpty(); + assertThat(multiPart.parameters).isEmpty(); + + // Verify files are deleted + for (String filePath : filePaths) { + assertThat(new File(filePath)).doesNotExist(); + } + } + protected String formFile(String fieldName, String filename, String content) { return endline + "--" + boundary + endline + 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 781b7fbd0..83d6cd860 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,6 +18,19 @@ */ package org.apache.struts2.dispatcher.multipart; +import org.apache.commons.fileupload2.core.DiskFileItem; +import org.apache.struts2.dispatcher.LocalizedMessage; +import org.junit.Test; + +import java.io.File; +import java.io.IOException; +import java.lang.reflect.Field; +import java.nio.charset.StandardCharsets; +import java.util.List; + +import static org.apache.commons.lang3.StringUtils.normalizeSpace; +import static org.assertj.core.api.Assertions.assertThat; + public class JakartaMultiPartRequestTest extends AbstractMultiPartRequestTest { @Override @@ -25,4 +38,185 @@ 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) { + // 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); + } + } + } + + 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 + JakartaMultiPartRequest multiPartWithDuplicateErrors = new JakartaMultiPartRequest(); + + // Simulate adding the same error twice + IOException testException = new IOException("Test exception"); + LocalizedMessage errorMessage = multiPartWithDuplicateErrors.buildErrorMessage( + testException.getClass(), testException.getMessage(), new Object[]{"test.csv"}); + + // when - add same error twice + multiPartWithDuplicateErrors.errors.add(errorMessage); + if (!multiPartWithDuplicateErrors.errors.contains(errorMessage)) { + multiPartWithDuplicateErrors.errors.add(errorMessage); + } + + // then - only one error should be present + assertThat(multiPartWithDuplicateErrors.getErrors()).hasSize(1); + } + + @Test + public void cleanupIsIdempotent() throws IOException { + // given - process some files + String content = formFile("file1", "test1.csv", "1,2,3,4") + + endline + "--" + boundary + "--"; + + mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8)); + multiPart.parse(mockRequest, tempDir); + + // when - call cleanup multiple times + multiPart.cleanUp(); + multiPart.cleanUp(); + multiPart.cleanUp(); + + // then - should not throw exceptions and should be safe + assertThat(multiPart.uploadedFiles).isEmpty(); + assertThat(multiPart.parameters).isEmpty(); + } + + @Test + public void endToEndMultipartProcessingWithCleanup() throws IOException { + // Test complete multipart processing lifecycle + String content = formFile("file1", "test1.csv", "1,2,3,4") + + formField("param1", "value1") + + formFile("file2", "test2.csv", "5,6,7,8") + + formField("param2", "value2") + + endline + "--" + boundary + "--"; + + mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8)); + + // when - full processing + multiPart.parse(mockRequest, tempDir); + + // then - verify everything was processed + assertThat(multiPart.getErrors()).isEmpty(); + assertThat(multiPart.getFile("file1")).hasSize(1); + assertThat(multiPart.getFile("file2")).hasSize(1); + assertThat(multiPart.getParameter("param1")).isEqualTo("value1"); + assertThat(multiPart.getParameter("param2")).isEqualTo("value2"); + + // when - cleanup + multiPart.cleanUp(); + + // then - verify complete cleanup + assertThat(multiPart.uploadedFiles).isEmpty(); + assertThat(multiPart.parameters).isEmpty(); + } + } diff --git a/core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequestTest.java b/core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequestTest.java index 7a8ea19d9..a82c4b899 100644 --- a/core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequestTest.java +++ b/core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequestTest.java @@ -24,9 +24,13 @@ import org.assertj.core.api.InstanceOfAssertFactories; import org.junit.Test; import java.io.IOException; +import java.io.InputStream; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; import java.nio.charset.StandardCharsets; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; public class JakartaStreamMultiPartRequestTest extends AbstractMultiPartRequestTest { @@ -71,4 +75,90 @@ public class JakartaStreamMultiPartRequestTest extends AbstractMultiPartRequestT .containsExactly("struts.messages.upload.error.FileUploadSizeException"); } + @Test + public void readStreamProperlyHandlesResources() throws Exception { + // Create a test input stream with known data + byte[] testData = "test data for stream reading".getBytes(StandardCharsets.UTF_8); + InputStream testStream = new java.io.ByteArrayInputStream(testData); + + JakartaStreamMultiPartRequest streamMultiPart = new JakartaStreamMultiPartRequest(); + + // Use reflection to access private readStream method + Method readStreamMethod = JakartaStreamMultiPartRequest.class.getDeclaredMethod("readStream", InputStream.class); + readStreamMethod.setAccessible(true); + + // when + String result = (String) readStreamMethod.invoke(streamMultiPart, testStream); + + // then + assertThat(result).isEqualTo("test data for stream reading"); + } + + @Test + public void readStreamHandlesExceptionsProperly() throws Exception { + // Create a stream that throws an exception + InputStream faultyStream = new InputStream() { + @Override + public int read() throws IOException { + throw new IOException("Simulated stream failure"); + } + }; + + JakartaStreamMultiPartRequest streamMultiPart = new JakartaStreamMultiPartRequest(); + + // Use reflection to access private readStream method + Method readStreamMethod = JakartaStreamMultiPartRequest.class.getDeclaredMethod("readStream", InputStream.class); + readStreamMethod.setAccessible(true); + + // when/then - should propagate the exception + assertThatThrownBy(() -> readStreamMethod.invoke(streamMultiPart, faultyStream)) + .isInstanceOf(InvocationTargetException.class) + .cause() + .isInstanceOf(IOException.class) + .hasMessage("Simulated stream failure"); + } + + @Test + public void readStreamHandlesEmptyStream() throws Exception { + // Create an empty stream + InputStream emptyStream = new java.io.ByteArrayInputStream(new byte[0]); + + JakartaStreamMultiPartRequest streamMultiPart = new JakartaStreamMultiPartRequest(); + + // Use reflection to access private readStream method + Method readStreamMethod = JakartaStreamMultiPartRequest.class.getDeclaredMethod("readStream", InputStream.class); + readStreamMethod.setAccessible(true); + + // when + String result = (String) readStreamMethod.invoke(streamMultiPart, emptyStream); + + // then + assertThat(result).isEmpty(); + } + + @Test + public void readStreamHandlesLargeData() throws Exception { + // Create a large data stream to test buffer handling + StringBuilder largeData = new StringBuilder(); + for (int i = 0; i < 2000; i++) { + largeData.append("line").append(i).append("\n"); + } + + byte[] testData = largeData.toString().getBytes(StandardCharsets.UTF_8); + InputStream largeStream = new java.io.ByteArrayInputStream(testData); + + JakartaStreamMultiPartRequest streamMultiPart = new JakartaStreamMultiPartRequest(); + + // Use reflection to access private readStream method + Method readStreamMethod = JakartaStreamMultiPartRequest.class.getDeclaredMethod("readStream", InputStream.class); + readStreamMethod.setAccessible(true); + + // when + String result = (String) readStreamMethod.invoke(streamMultiPart, largeStream); + + // then + assertThat(result).isEqualTo(largeData.toString()); + assertThat(result.length()).isGreaterThan(1024); // Verify it's larger than internal buffer + } + } From 63f2c8bdecf8ecf094e1a7a2c80307dc784529de Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sat, 19 Jul 2025 09:48:22 +0200 Subject: [PATCH 5/9] Adds missing JavaDocs --- .../multipart/JakartaMultiPartRequest.java | 190 ++++++++++++++++-- .../JakartaStreamMultiPartRequest.java | 119 ++++++++--- 2 files changed, 260 insertions(+), 49 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java b/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java index 56f9e0a49..a8880d47a 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 @@ -38,6 +38,37 @@ 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.

+ * + *

Key features:

+ *
    + *
  • Automatic tracking and cleanup of temporary files
    • + *
  • Proper error handling with user-friendly error messages
    • + *
  • Support for both in-memory and disk-based file uploads
    • + *
  • Extensible cleanup mechanisms for customization
    • + *
+ * + *

Usage example:

+ * 
+ * JakartaMultiPartRequest multipartRequest = new JakartaMultiPartRequest();
+ * try {
+ *     multipartRequest.parse(request, "/tmp/uploads");
+ *     // Process uploaded files
+ *     for (String fieldName : multipartRequest.getFileParameterNames()) {
+ *         List<UploadedFile> files = multipartRequest.getFile(fieldName);
+ *         // Handle files
+ *     }
+ * } finally {
+ *     multipartRequest.cleanUp(); // Always clean up resources
+ * }
+ *
+ * + * @see AbstractMultiPartRequest + * @see org.apache.commons.fileupload2.jakarta.servlet6.JakartaServletDiskFileUpload */ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { @@ -53,6 +84,27 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { */ private final List temporaryFiles = new ArrayList<>(); + /** + * Processes the multipart upload request using Jakarta Commons FileUpload. + * + *

This method handles the core upload processing by:

+ *
    + *
  1. Reading the character encoding from the request
    2. + *
  2. Preparing the Jakarta servlet file upload handler
    3. + *
  3. Creating a request context for processing
    4. + *
  4. Iterating through all form items (fields and files)
    5. + *
  5. Processing each item appropriately based on its type
    6. + *
+ * + *

All {@link org.apache.commons.fileupload2.core.DiskFileItem} instances + * are automatically tracked for proper cleanup.

+ * + * @param request the HTTP servlet request containing the multipart data + * @param saveDir the directory where uploaded files will be stored + * @throws IOException if an error occurs during upload processing + * @see #processNormalFormField(DiskFileItem, Charset) + * @see #processFileField(DiskFileItem) + */ @Override protected void processUpload(HttpServletRequest request, String saveDir) throws IOException { Charset charset = readCharsetEncoding(request); @@ -63,29 +115,51 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { RequestContext requestContext = createRequestContext(request); for (DiskFileItem item : servletFileUpload.parseRequest(requestContext)) { - // Track all DiskFileItem instances for cleanup + // Track all DiskFileItem instances for cleanup - this is critical for security + // as it ensures temporary files are properly cleaned up even if processing fails diskFileItems.add(item); LOG.debug(() -> "Processing a form field: " + normalizeSpace(item.getFieldName())); if (item.isFormField()) { + // Process regular form fields (text inputs, checkboxes, etc.) processNormalFormField(item, charset); } else { + // Process file upload fields LOG.debug(() -> "Processing a file: " + normalizeSpace(item.getFieldName())); processFileField(item); } } } + /** + * Processes a normal form field (non-file) from the multipart request. + * + *

This method handles text form fields by:

+ *
    + *
  1. Validating the field name is not null
    2. + *
  2. Extracting the field value using the specified charset
    3. + *
  3. Checking if the field value exceeds maximum string length
    4. + *
  4. Adding the value to the parameters map
    5. + *
+ * + *

Fields with null names are skipped with a warning log message.

+ *

Empty form fields are stored as empty strings.

+ * + * @param item the disk file item representing the form field + * @param charset the character set to use for decoding the field value + * @throws IOException if an error occurs reading the field value + * @see #exceedsMaxStringLength(String, String) + */ protected void processNormalFormField(DiskFileItem item, Charset charset) throws IOException { LOG.debug("Item: {} is a normal form field", normalizeSpace(item.getName())); - List values; String fieldName = item.getFieldName(); - if (parameters.get(fieldName) != null) { - values = parameters.get(fieldName); - } else { - values = new ArrayList<>(); + if (fieldName == null) { + LOG.warn("Form field has null fieldName, skipping"); + return; } + + List values = parameters.computeIfAbsent(fieldName, k -> new ArrayList<>()); String fieldValue = item.getString(charset); if (exceedsMaxStringLength(fieldName, fieldValue)) { @@ -99,6 +173,26 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { parameters.put(fieldName, values); } + /** + * Processes a file field from the multipart request. + * + *

This method handles file uploads by:

+ *
    + *
  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 disk files: using the existing file directly
    5. + *
  5. Creating an {@link UploadedFile} abstraction
    6. + *
  6. Adding the file to the uploaded files collection
    7. + *
+ * + *

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() + */ protected void processFileField(DiskFileItem item) { // Skip file uploads that don't have a file name - meaning that no file was selected. if (item.getName() == null || item.getName().trim().isEmpty()) { @@ -106,18 +200,18 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { return; } - List values; - if (uploadedFiles.get(item.getFieldName()) != null) { - values = uploadedFiles.get(item.getFieldName()); - } else { - values = new ArrayList<>(); + String fieldName = item.getFieldName(); + if (fieldName == null) { + LOG.warn("File field has null fieldName, skipping"); + return; } + + 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 = File.createTempFile("struts_upload_", "_" + item.getName()); - tempFile.deleteOnExit(); // Ensure cleanup on JVM exit as fallback // Track the temporary file for explicit cleanup temporaryFiles.add(tempFile); @@ -157,12 +251,26 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { values.add(uploadedFile); } - uploadedFiles.put(item.getFieldName(), values); + uploadedFiles.put(fieldName, values); } /** * Cleans up disk file items by deleting associated temporary files. - * This method can be overridden by subclasses to customize cleanup behavior. + * + *

This method iterates through all tracked {@link DiskFileItem} instances + * and performs cleanup operations:

+ *
    + *
  • For in-memory items: logs cleanup (no files to delete)
    • + *
  • For disk items: deletes the associated temporary file
    • + *
+ * + *

This method is called automatically during {@link #cleanUp()} but can + * 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"); @@ -171,10 +279,12 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { if (item.isInMemory()) { LOG.debug("Cleaning up in-memory item: {}", normalizeSpace(item.getFieldName())); } else { - LOG.debug("Cleaning up disk item: {} at {}", normalizeSpace(item.getFieldName()), item.getPath()); - if (item.getPath() != null && item.getPath().toFile().exists()) { - if (!item.getPath().toFile().delete()) { - LOG.warn("There was a problem attempting to delete temporary file: {}", item.getPath()); + Path itemPath = item.getPath(); + LOG.debug("Cleaning up disk item: {} at {}", normalizeSpace(item.getFieldName()), itemPath); + if (itemPath != null) { + File itemFile = itemPath.toFile(); + if (itemFile.exists() && !itemFile.delete()) { + LOG.warn("There was a problem attempting to delete temporary file: {}", itemPath); } } } @@ -186,7 +296,26 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { /** * Cleans up temporary files created for in-memory uploads. - * This method can be overridden by subclasses to customize cleanup behavior. + * + *

This method deletes all temporary files that were created when + * processing in-memory uploads. These files are created in + * {@link #processFileField(DiskFileItem)} 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()); @@ -207,7 +336,28 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { } /** - * Override cleanUp to ensure all DiskFileItem instances and temporary files are properly cleaned up + * 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 public void cleanUp() { diff --git a/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java b/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java index 5dba4fdd3..9e59aa766 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java @@ -81,36 +81,62 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest { }); } + /** + * Reads the entire contents of an input stream into a string. + * + *

This method uses a buffered approach to efficiently read the stream + * content without loading the entire stream into memory at once. It uses + * try-with-resources to ensure proper cleanup of resources.

+ * + * @param inputStream the input stream to read from + * @return the stream contents as a UTF-8 string + * @throws IOException if an error occurs reading the stream + */ private String readStream(InputStream inputStream) throws IOException { + // Use try-with-resources to ensure ByteArrayOutputStream is properly closed try (ByteArrayOutputStream result = new ByteArrayOutputStream()) { - byte[] buffer = new byte[1024]; + byte[] buffer = new byte[1024]; // 1KB buffer for efficient reading + // Read the stream in chunks to avoid loading everything into memory at once for (int length; (length = inputStream.read(buffer)) != -1; ) { result.write(buffer, 0, length); } + // Convert to string using UTF-8 encoding return result.toString(StandardCharsets.UTF_8); } } /** - * Processes the FileItem as a normal form field. - * - * @param fileItemInput a form field item input + * Processes a normal form field (non-file) from the multipart request using streaming API. + * + *

This method handles text form fields by:

+ *
    + *
  1. Validating the field name is not null
    2. + *
  2. Reading the field value from the input stream
    3. + *
  3. Checking if the field value exceeds maximum string length
    4. + *
  4. Adding the value to the parameters collection
    5. + *
+ * + *

Fields with null names are skipped with a warning log message.

+ *

The streaming approach is more memory-efficient for large form data.

+ * + * @param fileItemInput a form field item input from the streaming API + * @throws IOException if an error occurs reading the input stream + * @see #readStream(InputStream) + * @see #exceedsMaxStringLength(String, String) */ protected void processFileItemAsFormField(FileItemInput fileItemInput) throws IOException { String fieldName = fileItemInput.getFieldName(); + if (fieldName == null) { + LOG.warn("Form field has null fieldName, skipping"); + return; + } + String fieldValue = readStream(fileItemInput.getInputStream()); - if (exceedsMaxStringLength(fieldName, fieldValue)) { return; } - List values; - if (parameters.containsKey(fieldName)) { - values = parameters.get(fieldName); - } else { - values = new ArrayList<>(); - parameters.put(fieldName, values); - } + List values = parameters.computeIfAbsent(fieldName, k -> new ArrayList<>()); values.add(fieldValue); } @@ -165,10 +191,28 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest { } /** - * Processes the FileItem as a file field. - * - * @param fileItemInput file item representing upload file - * @param location location + * Processes a file field from the multipart request using streaming API. + * + *

This method handles file uploads by:

+ *
    + *
  1. Validating the file name and field name are not null/empty
    2. + *
  2. Checking if the upload exceeds maximum file count
    3. + *
  3. Creating a temporary file in the specified location
    4. + *
  4. Streaming the file content directly to disk
    5. + *
  5. Checking if the total size exceeds maximum allowed size
    6. + *
  6. Creating an {@link UploadedFile} abstraction or cleaning up on size exceeded
    7. + *
+ * + *

Files with null names or field names are skipped with appropriate logging.

+ *

The streaming approach is more memory-efficient for large file uploads + * as it writes directly to disk rather than loading into memory first.

+ * + * @param fileItemInput file item representing upload file from streaming API + * @param location the directory where temporary files will be created + * @throws IOException if an error occurs during file processing + * @see #createTemporaryFile(String, Path) + * @see #streamFileToDisk(FileItemInput, File) + * @see #createUploadedFile(FileItemInput, File) */ protected void processFileItemAsFileField(FileItemInput fileItemInput, Path location) throws IOException { // Skip file uploads that don't have a file name - meaning that no file was selected. @@ -176,6 +220,12 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest { LOG.debug(() -> "No file has been uploaded for the field: " + normalizeSpace(fileItemInput.getFieldName())); return; } + + // Skip file uploads that don't have a field name + if (fileItemInput.getFieldName() == null) { + LOG.warn("File upload has null fieldName, skipping"); + return; + } if (exceedsMaxFiles(fileItemInput)) { return; @@ -224,29 +274,40 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest { } /** - * Create {@link UploadedFile} abstraction over uploaded file - * - * @param fileItemInput file item stream - * @param file the file + * Creates an {@link UploadedFile} abstraction over an uploaded file. + * + *

This method creates a wrapper around the uploaded file that provides + * a consistent interface for accessing file information and content. + * The created {@link UploadedFile} instance contains:

+ *
    + *
  • The original filename as provided by the client
    • + *
  • The content type (MIME type) if available
    • + *
  • The form field name that contained the file
    • + *
  • A reference to the temporary file on disk
    • + *
+ * + *

The file is automatically added to the uploaded files collection, + * grouped by field name to support multiple file uploads per field.

+ * + * @param fileItemInput file item stream containing file metadata + * @param file the temporary file containing the uploaded content + * @see UploadedFile + * @see StrutsUploadedFile */ protected void createUploadedFile(FileItemInput fileItemInput, File file) { String fileName = fileItemInput.getName(); String fieldName = fileItemInput.getFieldName(); - + + // fieldName null check already done in processFileItemAsFileField UploadedFile uploadedFile = StrutsUploadedFile.Builder .create(file) .withOriginalName(fileName) .withContentType(fileItemInput.getContentType()) - .withInputName(fileItemInput.getFieldName()) + .withInputName(fieldName) .build(); - if (uploadedFiles.containsKey(fieldName)) { - uploadedFiles.get(fieldName).add(uploadedFile); - } else { - List infos = new ArrayList<>(); - infos.add(uploadedFile); - uploadedFiles.put(fieldName, infos); - } + List infos = uploadedFiles.computeIfAbsent(fieldName, key -> new ArrayList<>()); + infos.add(uploadedFile); } } From 824e7121b0796349f3ddfdf96bdc53936e0d37dd Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sat, 19 Jul 2025 10:30:02 +0200 Subject: [PATCH 6/9] Reuses logic to create temporary file --- .../multipart/AbstractMultiPartRequest.java | 18 +++++ .../multipart/JakartaMultiPartRequest.java | 10 +-- .../JakartaMultiPartRequestTest.java | 66 ++++++++++++++++++- 3 files changed, 87 insertions(+), 7 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequest.java b/core/src/main/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequest.java index 038cef4bc..63afbdf22 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequest.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequest.java @@ -35,6 +35,7 @@ import org.apache.logging.log4j.Logger; import org.apache.struts2.StrutsConstants; import org.apache.struts2.dispatcher.LocalizedMessage; +import java.io.File; import java.io.IOException; import java.nio.charset.Charset; import java.nio.file.Path; @@ -44,6 +45,7 @@ import java.util.Enumeration; import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.UUID; import static org.apache.commons.lang3.StringUtils.normalizeSpace; @@ -406,6 +408,22 @@ public abstract class AbstractMultiPartRequest implements MultiPartRequest { return values.toArray(new String[0]); } + /** + * Creates a secure temporary file in the specified directory using UUID-based naming. + * This method ensures files are created in a controlled location rather than the + * system temporary directory, reducing security risks. + * + * @param fileName the original filename for logging purposes + * @param location the directory where the temporary file should be created + * @return a new temporary file in the specified location + */ + protected File createTemporaryFile(String fileName, Path location) { + String uid = UUID.randomUUID().toString().replace("-", "_"); + File file = location.resolve("upload_" + uid + ".tmp").toFile(); + LOG.debug("Creating temporary file: {} (originally: {})", file.getName(), fileName); + return file; + } + /* (non-Javadoc) * @see org.apache.struts2.dispatcher.multipart.MultiPartRequest#cleanUp() */ 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 a8880d47a..23d93d585 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 @@ -103,7 +103,7 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { * @param saveDir the directory where uploaded files will be stored * @throws IOException if an error occurs during upload processing * @see #processNormalFormField(DiskFileItem, Charset) - * @see #processFileField(DiskFileItem) + * @see #processFileField(DiskFileItem, String) */ @Override protected void processUpload(HttpServletRequest request, String saveDir) throws IOException { @@ -126,7 +126,7 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { } else { // Process file upload fields LOG.debug(() -> "Processing a file: " + normalizeSpace(item.getFieldName())); - processFileField(item); + processFileField(item, saveDir); } } } @@ -193,7 +193,7 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { * @param item the disk file item representing the uploaded file * @see #cleanUpTemporaryFiles() */ - protected void processFileField(DiskFileItem item) { + protected void processFileField(DiskFileItem item, String saveDir) { // 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())); @@ -211,7 +211,7 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { if (item.isInMemory()) { LOG.debug("Creating temporary file representing in-memory uploaded item: {}", normalizeSpace(item.getFieldName())); try { - File tempFile = File.createTempFile("struts_upload_", "_" + item.getName()); + File tempFile = createTemporaryFile(item.getName(), Path.of(saveDir)); // Track the temporary file for explicit cleanup temporaryFiles.add(tempFile); @@ -299,7 +299,7 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { * *

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

* *

The cleanup process:

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 83d6cd860..2ef0782ea 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 @@ -26,6 +26,8 @@ import java.io.File; import java.io.IOException; import java.lang.reflect.Field; import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; import java.util.List; import static org.apache.commons.lang3.StringUtils.normalizeSpace; @@ -112,7 +114,7 @@ public class JakartaMultiPartRequestTest extends AbstractMultiPartRequestTest { // Create a custom implementation that simulates temp file creation failure class FaultyJakartaMultiPartRequest extends JakartaMultiPartRequest { @Override - protected void processFileField(DiskFileItem item) { + protected void processFileField(DiskFileItem item, String saveDir) { // Simulate in-memory upload that fails to create temp file if (item.isInMemory()) { try { @@ -127,7 +129,7 @@ public class JakartaMultiPartRequestTest extends AbstractMultiPartRequestTest { } } } else { - super.processFileField(item); + super.processFileField(item, saveDir); } } } @@ -219,4 +221,64 @@ 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); + } + } + } From 37144a03f7f7bd4538b41a05d2ab85b1a7a4907a Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sat, 19 Jul 2025 11:01:54 +0200 Subject: [PATCH 7/9] Adds missing test cases of temporary files --- .../multipart/JakartaMultiPartRequest.java | 14 +- .../JakartaStreamMultiPartRequest.java | 14 -- .../AbstractMultiPartRequestTest.java | 157 ++++++++++++ .../JakartaMultiPartRequestTest.java | 172 +++++++++++++ .../JakartaStreamMultiPartRequestTest.java | 230 ++++++++++++++++++ 5 files changed, 568 insertions(+), 19 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java b/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java index 23d93d585..0b4ce88fe 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 @@ -209,7 +209,7 @@ 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())); + LOG.debug(() -> "Creating temporary file representing in-memory uploaded item: " + normalizeSpace(item.getFieldName())); try { File tempFile = createTemporaryFile(item.getName(), Path.of(saveDir)); @@ -229,8 +229,10 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { .build(); values.add(uploadedFile); - LOG.debug("Created temporary file for in-memory uploaded item: {} at {}", - normalizeSpace(item.getName()), tempFile.getAbsolutePath()); + 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); @@ -277,10 +279,12 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { for (DiskFileItem item : diskFileItems) { try { if (item.isInMemory()) { - LOG.debug("Cleaning up in-memory item: {}", normalizeSpace(item.getFieldName())); + LOG.debug(() -> "Cleaning up in-memory item: " + normalizeSpace(item.getFieldName())); } else { Path itemPath = item.getPath(); - LOG.debug("Cleaning up disk item: {} at {}", normalizeSpace(item.getFieldName()), itemPath); + if (LOG.isDebugEnabled()) { + LOG.debug("Cleaning up disk item: {} at {}", normalizeSpace(item.getFieldName()), itemPath); + } if (itemPath != null) { File itemFile = itemPath.toFile(); if (itemFile.exists() && !itemFile.delete()) { diff --git a/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java b/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java index 9e59aa766..7275743e5 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java @@ -242,20 +242,6 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest { } } - /** - * Creates a temporary file based on the given filename and location. - * - * @param fileName file name - * @param location location - * @return a temporary file based on the given filename and location - */ - protected File createTemporaryFile(String fileName, Path location) { - String uid = UUID.randomUUID().toString().replace("-", "_"); - File file = location.resolve("upload_" + uid + ".tmp").toFile(); - LOG.debug("Creating temporary file: {} (originally: {})", file.getName(), fileName); - return file; - } - /** * Streams the file upload stream to the specified file. * 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 d968ec256..98d1325ef 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 @@ -30,6 +30,9 @@ import org.springframework.mock.web.MockHttpServletRequest; import java.io.File; import java.io.IOException; import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; import java.util.ArrayList; import java.util.List; import java.util.Map; @@ -586,6 +589,160 @@ abstract class AbstractMultiPartRequestTest { } } + @Test + public void createTemporaryFileGeneratesSecureNames() { + // Create a test instance to access the protected method + AbstractMultiPartRequest testRequest = createMultipartRequest(); + Path testLocation = Paths.get(tempDir); + + // when - create multiple temporary files + File tempFile1 = testRequest.createTemporaryFile("test1.csv", testLocation); + File tempFile2 = testRequest.createTemporaryFile("test2.csv", testLocation); + File tempFile3 = testRequest.createTemporaryFile("../../../malicious.csv", testLocation); + + // then - verify secure naming + assertThat(tempFile1.getName()).startsWith("upload_"); + assertThat(tempFile1.getName()).endsWith(".tmp"); + assertThat(tempFile2.getName()).startsWith("upload_"); + assertThat(tempFile2.getName()).endsWith(".tmp"); + assertThat(tempFile3.getName()).startsWith("upload_"); + assertThat(tempFile3.getName()).endsWith(".tmp"); + + // Verify each file has a unique name + assertThat(tempFile1.getName()).isNotEqualTo(tempFile2.getName()); + assertThat(tempFile2.getName()).isNotEqualTo(tempFile3.getName()); + assertThat(tempFile1.getName()).isNotEqualTo(tempFile3.getName()); + + // Verify all files are in the correct location + assertThat(tempFile1.getParent()).isEqualTo(tempDir); + assertThat(tempFile2.getParent()).isEqualTo(tempDir); + assertThat(tempFile3.getParent()).isEqualTo(tempDir); + + // Verify malicious filename doesn't affect the location + assertThat(tempFile3.getName()).doesNotContain(".."); + assertThat(tempFile3.getName()).doesNotContain("/"); + assertThat(tempFile3.getName()).doesNotContain("\\"); + + // Clean up test files + tempFile1.delete(); + tempFile2.delete(); + tempFile3.delete(); + } + + @Test + public void createTemporaryFileInSpecificDirectory() throws IOException { + // Create a subdirectory for testing + Path subDir = Paths.get(tempDir, "subdir"); + Files.createDirectories(subDir); + + AbstractMultiPartRequest testRequest = createMultipartRequest(); + + // when + File tempFile = testRequest.createTemporaryFile("test.csv", subDir); + + // then - verify file is created in the specified subdirectory + assertThat(tempFile.getParent()).isEqualTo(subDir.toString()); + assertThat(tempFile.getName()).startsWith("upload_"); + assertThat(tempFile.getName()).endsWith(".tmp"); + + // Clean up + tempFile.delete(); + Files.delete(subDir); + } + + @Test + public void createTemporaryFileWithNullFileName() throws IOException { + AbstractMultiPartRequest testRequest = createMultipartRequest(); + Path testLocation = Paths.get(tempDir); + + // when - create temp file with null filename + File tempFile = testRequest.createTemporaryFile(null, testLocation); + + // then - should still create a valid temporary file + assertThat(tempFile.getName()).startsWith("upload_"); + assertThat(tempFile.getName()).endsWith(".tmp"); + assertThat(tempFile.getParent()).isEqualTo(tempDir); + + // Clean up + tempFile.delete(); + } + + @Test + public void createTemporaryFileWithEmptyFileName() throws IOException { + AbstractMultiPartRequest testRequest = createMultipartRequest(); + Path testLocation = Paths.get(tempDir); + + // when - create temp file with empty filename + File tempFile = testRequest.createTemporaryFile("", testLocation); + + // then - should still create a valid temporary file + assertThat(tempFile.getName()).startsWith("upload_"); + assertThat(tempFile.getName()).endsWith(".tmp"); + assertThat(tempFile.getParent()).isEqualTo(tempDir); + + // Clean up + tempFile.delete(); + } + + @Test + public void createTemporaryFileWithSpecialCharacters() { + AbstractMultiPartRequest testRequest = createMultipartRequest(); + Path testLocation = Paths.get(tempDir); + + // when - create temp files with various special characters + File tempFile1 = testRequest.createTemporaryFile("file with spaces.csv", testLocation); + File tempFile2 = testRequest.createTemporaryFile("file@#$%^&*().csv", testLocation); + File tempFile3 = testRequest.createTemporaryFile("файл.csv", testLocation); // Cyrillic + + // then - all should create valid secure temporary files + File[] tempFiles = {tempFile1, tempFile2, tempFile3}; + for (File tempFile : tempFiles) { + assertThat(tempFile.getName()).startsWith("upload_"); + assertThat(tempFile.getName()).endsWith(".tmp"); + assertThat(tempFile.getParent()).isEqualTo(tempDir); + // Verify no special characters leak into the actual filename + assertThat(tempFile.getName()).matches("upload_[a-zA-Z0-9_]+\\.tmp"); + } + + // All should have unique names + assertThat(tempFile1.getName()).isNotEqualTo(tempFile2.getName()); + assertThat(tempFile2.getName()).isNotEqualTo(tempFile3.getName()); + assertThat(tempFile1.getName()).isNotEqualTo(tempFile3.getName()); + + // Clean up + tempFile1.delete(); + tempFile2.delete(); + tempFile3.delete(); + } + + @Test + public void createTemporaryFileConsistentNaming() { + AbstractMultiPartRequest testRequest = createMultipartRequest(); + Path testLocation = Paths.get(tempDir); + + // when - create many temporary files to verify naming consistency + List tempFiles = new ArrayList<>(); + for (int i = 0; i < 100; i++) { + tempFiles.add(testRequest.createTemporaryFile("test" + i + ".csv", testLocation)); + } + + // then - all should follow the same naming pattern + for (File tempFile : tempFiles) { + assertThat(tempFile.getName()).startsWith("upload_"); + assertThat(tempFile.getName()).endsWith(".tmp"); + assertThat(tempFile.getParent()).isEqualTo(tempDir); + // Verify UUID pattern (without hyphens, replaced with underscores) + assertThat(tempFile.getName()).matches("upload_[a-zA-Z0-9_]+\\.tmp"); + } + + // Verify all names are unique + List fileNames = tempFiles.stream().map(File::getName).toList(); + assertThat(fileNames).doesNotHaveDuplicates(); + + // Clean up + tempFiles.forEach(File::delete); + } + protected String formFile(String fieldName, String filename, String content) { return endline + "--" + boundary + endline + 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 2ef0782ea..15b59f5dd 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 @@ -20,6 +20,7 @@ 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; @@ -281,4 +282,175 @@ public class JakartaMultiPartRequestTest extends AbstractMultiPartRequestTest { } } + @Test + public void processNormalFormFieldHandlesNullFieldName() throws IOException { + // Test null field name handling in processNormalFormField + String content = + endline + "--" + boundary + endline + + "Content-Disposition: form-data" + endline + // No name attribute + endline + + "field value without name" + + endline + "--" + boundary + endline + + "Content-Disposition: form-data; name=\"validfield\"" + endline + + endline + + "valid field value" + + endline + "--" + boundary + "--"; + + mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8)); + + // when + multiPart.parse(mockRequest, tempDir); + + // then - should only process the valid field + assertThat(multiPart.getErrors()).isEmpty(); + assertThat(multiPart.getParameter("validfield")).isEqualTo("valid field value"); + assertThat(multiPart.getParameterNames().asIterator()).toIterable().hasSize(1); + } + + @Test + public void processFileFieldHandlesNullFieldName() throws IOException { + // Test null field name handling in processFileField + String content = + endline + "--" + boundary + endline + + "Content-Disposition: form-data; filename=\"orphan.txt\"" + endline + // No name attribute + "Content-Type: text/plain" + endline + + endline + + "orphaned file content" + + endline + "--" + boundary + endline + + "Content-Disposition: form-data; name=\"validfile\"; filename=\"valid.txt\"" + endline + + "Content-Type: text/plain" + endline + + endline + + "valid file content" + + endline + "--" + boundary + "--"; + + mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8)); + + // when + multiPart.parse(mockRequest, tempDir); + + // then - should only process the valid file + assertThat(multiPart.getErrors()).isEmpty(); + assertThat(multiPart.uploadedFiles).hasSize(1); + assertThat(multiPart.getFile("validfile")).hasSize(1); + assertThat(multiPart.getFile("validfile")[0].getContent()) + .asInstanceOf(InstanceOfAssertFactories.FILE) + .content() + .isEqualTo("valid file content"); + } + + @Test + public void diskFileItemCleanupCoverage() throws IOException, NoSuchFieldException, IllegalAccessException { + // Test disk file item cleanup paths + String content = formFile("file1", "test1.csv", "1,2,3,4") + + endline + "--" + boundary + "--"; + + mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8)); + + // when - force files to disk with small buffer + multiPart.setBufferSize("1"); + multiPart.parse(mockRequest, tempDir); + + // Access private field to verify disk file items are tracked + Field diskFileItemsField = JakartaMultiPartRequest.class.getDeclaredField("diskFileItems"); + diskFileItemsField.setAccessible(true); + @SuppressWarnings("unchecked") + java.util.List diskFileItems = + (java.util.List) diskFileItemsField.get(multiPart); + + // then - should have disk file items tracked + assertThat(diskFileItems).isNotEmpty(); + + // when - cleanup + multiPart.cleanUp(); + + // then - should clear tracking + assertThat(diskFileItems).isEmpty(); + } + + @Test + public void inMemoryVsDiskFileHandling() throws IOException { + // Test both in-memory and disk file handling paths + String smallContent = "small"; // Should be in-memory + String largeContent = "x".repeat(20000); // Should go to disk + + String content = formFile("smallfile", "small.txt", smallContent) + + formFile("largefile", "large.txt", largeContent) + + endline + "--" + boundary + "--"; + + mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8)); + + // when - use default buffer size + multiPart.parse(mockRequest, tempDir); + + // then - both files should be processed + assertThat(multiPart.getErrors()).isEmpty(); + assertThat(multiPart.uploadedFiles).hasSize(2); + assertThat(multiPart.getFile("smallfile")).hasSize(1); + assertThat(multiPart.getFile("largefile")).hasSize(1); + + // Verify content + assertThat(multiPart.getFile("smallfile")[0].getContent()) + .asInstanceOf(InstanceOfAssertFactories.FILE) + .content() + .isEqualTo(smallContent); + assertThat(multiPart.getFile("largefile")[0].getContent()) + .asInstanceOf(InstanceOfAssertFactories.FILE) + .content() + .isEqualTo(largeContent); + } + + @Test + public void errorDuplicationPrevention() throws IOException { + // Test that duplicate errors are not added + JakartaMultiPartRequest multiPartRequest = new JakartaMultiPartRequest(); + + // Simulate adding the same error multiple times + IOException testException = new IOException("Test error"); + LocalizedMessage errorMessage = multiPartRequest.buildErrorMessage( + testException.getClass(), testException.getMessage(), new Object[]{"test.csv"}); + + // when - try to add same error multiple times + multiPartRequest.errors.add(errorMessage); + if (!multiPartRequest.errors.contains(errorMessage)) { + multiPartRequest.errors.add(errorMessage); // Should not be added + } + if (!multiPartRequest.errors.contains(errorMessage)) { + multiPartRequest.errors.add(errorMessage); // Should not be added + } + + // then - should only have one error + assertThat(multiPartRequest.getErrors()).hasSize(1); + } + + @Test + public void processFileFieldHandlesEmptyFileName() throws IOException { + String content = + endline + "--" + boundary + endline + + "Content-Disposition: form-data; name=\"emptyfile\"; filename=\"\"" + endline + + "Content-Type: text/plain" + endline + + endline + + "some content that should be ignored" + + endline + "--" + boundary + endline + + "Content-Disposition: form-data; name=\"validfile\"; filename=\"test.txt\"" + endline + + "Content-Type: text/plain" + endline + + 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); + assertThat(multiPart.getFile("validfile")).hasSize(1); + assertThat(multiPart.getFile("emptyfile")).isEmpty(); + assertThat(multiPart.getFile("validfile")[0].getContent()) + .asInstanceOf(InstanceOfAssertFactories.FILE) + .content() + .isEqualTo("valid file content"); + } + } diff --git a/core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequestTest.java b/core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequestTest.java index a82c4b899..fc78021f5 100644 --- a/core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequestTest.java +++ b/core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequestTest.java @@ -23,11 +23,15 @@ 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.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; import java.nio.charset.StandardCharsets; +import java.nio.file.Path; +import java.nio.file.Paths; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; @@ -161,4 +165,230 @@ public class JakartaStreamMultiPartRequestTest extends AbstractMultiPartRequestT assertThat(result.length()).isGreaterThan(1024); // Verify it's larger than internal buffer } + @Test + public void processFileItemAsFormFieldHandlesNullFieldName() throws IOException { + // Test the null field name path in processFileItemAsFormField + String content = formFile("", "test.csv", "data") + // Field name will be empty/null-like + endline + "--" + boundary + "--"; + + mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8)); + + // when + multiPart.parse(mockRequest, tempDir); + + // then - should complete without error, but no parameters should be added + assertThat(multiPart.getErrors()).isEmpty(); + } + + @Test + public void processFileItemAsFileFieldHandlesNullFieldName() throws IOException { + // This test covers the null field name path in processFileItemAsFileField + JakartaStreamMultiPartRequest streamMultiPart = new JakartaStreamMultiPartRequest(); + + // Create a mock file item with null field name + String content = "--" + boundary + endline + + "Content-Disposition: form-data; filename=\"test.csv\"" + endline + + "Content-Type: text/csv" + endline + + endline + + "test data" + + endline + "--" + boundary + "--"; + + mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8)); + + // when + streamMultiPart.parse(mockRequest, tempDir); + + // then - should complete without error but no files should be uploaded + assertThat(streamMultiPart.getErrors()).isEmpty(); + assertThat(streamMultiPart.uploadedFiles).isEmpty(); + } + + @Test + public void exceedsMaxFilesPath() throws IOException { + // Test the exceedsMaxFiles method path + String content = formFile("file1", "test1.csv", "data1") + + formFile("file2", "test2.csv", "data2") + + formFile("file3", "test3.csv", "data3") + + endline + "--" + boundary + "--"; + + mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8)); + + // when - set max files to 1 + multiPart.setMaxFiles("1"); + multiPart.parse(mockRequest, tempDir); + + // then - should have only 1 file and errors for others + assertThat(multiPart.uploadedFiles).hasSize(1); + assertThat(multiPart.getErrors()) + .isNotEmpty() + .allSatisfy(error -> + assertThat(error.getTextKey()).isEqualTo("struts.messages.upload.error.FileUploadFileCountLimitException") + ); + } + + @Test + public void actualSizeOfUploadedFilesCalculation() throws IOException { + // Test the actualSizeOfUploadedFiles method + String content = formFile("file1", "test1.csv", "data1234567890") + // 14 bytes + headers + formFile("file2", "test2.csv", "moredata") + // 8 bytes + headers + endline + "--" + boundary + "--"; + + mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8)); + + // when + multiPart.parse(mockRequest, tempDir); + + // then - should have uploaded files and calculate their total size + assertThat(multiPart.uploadedFiles).hasSize(2); + assertThat(multiPart.getFile("file1")).hasSize(1); + assertThat(multiPart.getFile("file2")).hasSize(1); + + // Verify files have the expected content + assertThat(multiPart.getFile("file1")[0].getContent()) + .asInstanceOf(InstanceOfAssertFactories.FILE) + .content() + .isEqualTo("data1234567890"); + assertThat(multiPart.getFile("file2")[0].getContent()) + .asInstanceOf(InstanceOfAssertFactories.FILE) + .content() + .isEqualTo("moredata"); + } + + @Test + public void createTemporaryFileMethod() throws Exception { + // Test the createTemporaryFile method directly + JakartaStreamMultiPartRequest streamMultiPart = new JakartaStreamMultiPartRequest(); + Path testLocation = Paths.get(tempDir); + + // when + File tempFile1 = streamMultiPart.createTemporaryFile("test.csv", testLocation); + File tempFile2 = streamMultiPart.createTemporaryFile("another.txt", testLocation); + + // then + assertThat(tempFile1.getName()).startsWith("upload_"); + assertThat(tempFile1.getName()).endsWith(".tmp"); + assertThat(tempFile1.getParent()).isEqualTo(tempDir); + + assertThat(tempFile2.getName()).startsWith("upload_"); + assertThat(tempFile2.getName()).endsWith(".tmp"); + assertThat(tempFile2.getParent()).isEqualTo(tempDir); + + // Should be unique names + assertThat(tempFile1.getName()).isNotEqualTo(tempFile2.getName()); + + // Clean up + tempFile1.delete(); + tempFile2.delete(); + } + + @Test + public void streamFileToDiskWithDifferentBufferSizes() throws IOException { + // Test streamFileToDisk with different buffer sizes + String largeContent = "x".repeat(5000); // Content larger than default buffer + String content = formFile("largefile", "large.csv", largeContent) + + endline + "--" + boundary + "--"; + + mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8)); + + // when - use small buffer size to ensure multiple reads + multiPart.setBufferSize("100"); + multiPart.parse(mockRequest, tempDir); + + // then + assertThat(multiPart.getErrors()).isEmpty(); + assertThat(multiPart.getFile("largefile")).hasSize(1); + assertThat(multiPart.getFile("largefile")[0].getContent()) + .asInstanceOf(InstanceOfAssertFactories.FILE) + .content() + .isEqualTo(largeContent); + } + + @Test + public void exceedsMaxSizeOfFilesWithFileCleanup() throws IOException { + // Test the file deletion path when max size is exceeded + String content = formFile("file1", "test1.csv", "small") + + formFile("file2", "test2.csv", "this is a much larger file content that should exceed the limit") + + endline + "--" + boundary + "--"; + + mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8)); + + // when - set very small max size + multiPart.setMaxSizeOfFiles("20"); + multiPart.parse(mockRequest, tempDir); + + // then - should have first file uploaded but error for second + assertThat(multiPart.uploadedFiles).hasSize(1); + assertThat(multiPart.getFile("file1")).hasSize(1); + assertThat(multiPart.getFile("file2")).isEmpty(); + assertThat(multiPart.getErrors()) + .isNotEmpty() + .anyMatch(error -> + error.getTextKey().equals("struts.messages.upload.error.FileUploadSizeException") + ); + } + + @Test + public void createUploadedFileWithVariousContentTypes() throws IOException { + // Test different content types and file names + String content = + endline + "--" + boundary + endline + + "Content-Disposition: form-data; name=\"textfile\"; filename=\"document.txt\"" + endline + + "Content-Type: text/plain" + endline + + endline + + "Plain text content" + + endline + "--" + boundary + endline + + "Content-Disposition: form-data; name=\"jsonfile\"; filename=\"data.json\"" + endline + + "Content-Type: application/json" + endline + + endline + + "{\"key\": \"value\"}" + + endline + "--" + boundary + "--"; + + mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8)); + + // when + multiPart.parse(mockRequest, tempDir); + + // then + assertThat(multiPart.getErrors()).isEmpty(); + assertThat(multiPart.uploadedFiles).hasSize(2); + + // Verify text file + assertThat(multiPart.getFile("textfile")).hasSize(1); + assertThat(multiPart.getFile("textfile")[0].getContentType()).isEqualTo("text/plain"); + assertThat(multiPart.getFile("textfile")[0].getOriginalName()).isEqualTo("document.txt"); + + // Verify JSON file + assertThat(multiPart.getFile("jsonfile")).hasSize(1); + assertThat(multiPart.getFile("jsonfile")[0].getContentType()).isEqualTo("application/json"); + assertThat(multiPart.getFile("jsonfile")[0].getOriginalName()).isEqualTo("data.json"); + } + + @Test + public void emptyFileNameFieldsAreSkipped() throws IOException { + // Test files with empty names are skipped + String content = + endline + "--" + boundary + endline + + "Content-Disposition: form-data; name=\"emptyfile\"; filename=\"\"" + endline + + "Content-Type: text/plain" + endline + + endline + + "This should be skipped" + + endline + "--" + boundary + endline + + "Content-Disposition: form-data; name=\"validfile\"; filename=\"valid.txt\"" + endline + + "Content-Type: text/plain" + endline + + endline + + "This should be processed" + + endline + "--" + boundary + "--"; + + mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8)); + + // when + multiPart.parse(mockRequest, tempDir); + + // then + assertThat(multiPart.getErrors()).isEmpty(); + assertThat(multiPart.uploadedFiles).hasSize(1); + assertThat(multiPart.getFile("emptyfile")).isEmpty(); + assertThat(multiPart.getFile("validfile")).hasSize(1); + } + } From c8ab33e3cebb35409123b6255956b58e73a88013 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Wed, 6 Aug 2025 09:00:43 +0200 Subject: [PATCH 8/9] Uses lambda Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- .../dispatcher/multipart/JakartaStreamMultiPartRequest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java b/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java index 7275743e5..e6dcf63ca 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java @@ -136,7 +136,7 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest { return; } - List values = parameters.computeIfAbsent(fieldName, k -> new ArrayList<>()); + List values = parameters.computeIfAbsent(fieldName, ArrayList::new); values.add(fieldValue); } From f16abd171b6455185380ff66b33f015f1996984e Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sat, 19 Jul 2025 11:01:54 +0200 Subject: [PATCH 9/9] Adds missing test cases of temporary files --- .../dispatcher/multipart/JakartaStreamMultiPartRequest.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java b/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java index e6dcf63ca..2942b7b03 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java @@ -39,7 +39,6 @@ import java.nio.file.Files; import java.nio.file.Path; import java.util.ArrayList; import java.util.List; -import java.util.UUID; import static org.apache.commons.lang3.StringUtils.normalizeSpace; @@ -136,7 +135,7 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest { return; } - List values = parameters.computeIfAbsent(fieldName, ArrayList::new); + List values = parameters.computeIfAbsent(fieldName, k -> new ArrayList<>()); values.add(fieldValue); }