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:
+ *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 {
private static final Logger LOG = LogManager.getLogger(JakartaMultiPartRequest.class);
+ /**
+ * List to track all DiskFileItem instances for proper cleanup
+ */
+ private final ListThis method handles the core upload processing by:
+ *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, String) + */ @Override protected void processUpload(HttpServletRequest request, String saveDir) throws IOException { Charset charset = readCharsetEncoding(request); @@ -48,44 +112,54 @@ 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 - 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); + processFileField(item, saveDir); } } } - @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); - } - + /** + * Processes a normal form field (non-file) from the multipart request. + * + *This method handles text form fields by:
+ *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())); - ListThis method handles file uploads by:
+ *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, 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())); return; } - ListThis method iterates through all tracked {@link DiskFileItem} instances + * and performs cleanup operations:
+ *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"); + for (DiskFileItem item : diskFileItems) { + try { + if (item.isInMemory()) { + LOG.debug(() -> "Cleaning up in-memory item: " + normalizeSpace(item.getFieldName())); + } else { + Path itemPath = item.getPath(); + 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()) { + LOG.warn("There was a problem attempting to delete temporary file: {}", itemPath); + } + } + } + } catch (Exception e) { + LOG.warn("Error cleaning up DiskFileItem: {}", normalizeSpace(item.getFieldName()), e); + } + } + } + + /** + * Cleans up temporary files created for in-memory uploads. + * + *This method deletes all temporary files that were created when + * processing in-memory uploads. These files are created in + * {@link #processFileField(DiskFileItem, String)} when an uploaded file is + * stored in memory and needs to be written to disk.
+ * + *The cleanup process:
+ *This method can be overridden by subclasses to customize cleanup + * behavior. All exceptions are caught and logged to ensure cleanup + * continues even if individual file deletions fail.
+ * + * @see #cleanUp() + * @see #cleanUpDiskFileItems() + */ + protected void cleanUpTemporaryFiles() { + LOG.debug("Cleaning up {} temporary files created for in-memory uploads", temporaryFiles.size()); + for (File tempFile : temporaryFiles) { + 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); + } + } + } + + /** + * 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:
+ *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() { + super.cleanUp(); + try { + cleanUpDiskFileItems(); + cleanUpTemporaryFiles(); + } finally { + diskFileItems.clear(); + temporaryFiles.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..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 @@ -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; @@ -40,12 +39,11 @@ 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; /** - * 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,51 +80,62 @@ 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); - } - + /** + * 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 { - ByteArrayOutputStream result = new ByteArrayOutputStream(); - byte[] buffer = new byte[1024]; - for (int length; (length = inputStream.read(buffer)) != -1; ) { - result.write(buffer, 0, length); + // Use try-with-resources to ensure ByteArrayOutputStream is properly closed + try (ByteArrayOutputStream result = new ByteArrayOutputStream()) { + 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); } - 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:
+ *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; } - ListThis method handles file uploads by:
+ *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. @@ -192,6 +219,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; @@ -208,20 +241,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. * @@ -240,29 +259,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 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+ * 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();
+ }
+}
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..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;
@@ -491,6 +494,255 @@ 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