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 + } + }