Reuses logic to create temporary file

lukaszlenart · lukaszlenart · commit 824e7121b079 · 2025-08-06T08:58:14.000+02:00
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
@@ -35,6 +35,7 @@
 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.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 String[] getParameterValues(String name) {
         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
@@ -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 @@ protected void processUpload(HttpServletRequest request, String saveDir) throws
             } else {
                 // Process file upload fields
                 LOG.debug(() -> "Processing a file: " + normalizeSpace(item.getFieldName()));
-                processFileField(item);
+                processFileField(item, saveDir);
             }
         }
     }
@@ -193,7 +193,7 @@ protected void processNormalFormField(DiskFileItem item, Charset charset) throws
      * @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 @@ protected void processFileField(DiskFileItem item) {
         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 @@ protected void cleanUpDiskFileItems() {
      * 
      * <p>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.</p>
      * 
      * <p>The cleanup process:</p>
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
@@ -26,6 +26,8 @@
 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 void temporaryFileCreationFailureAddsError() throws IOException {
         // 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 @@ protected void processFileField(DiskFileItem item) {
                         }
                     }
                 } else {
-                    super.processFileField(item);
+                    super.processFileField(item, saveDir);
                 }
             }
         }
@@ -219,4 +221,64 @@ public void endToEndMultipartProcessingWithCleanup() throws IOException {
         assertThat(multiPart.parameters).isEmpty();
     }
 
+    @Test
+    public void temporaryFilesCreatedInSaveDirectory() throws IOException, NoSuchFieldException, IllegalAccessException {
+        // Test that temporary files for in-memory uploads are created in the saveDir, not system temp
+        String content = formFile("file1", "test1.csv", "small,content") +
+                        endline + "--" + boundary + "--";
+        
+        mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8));
+        
+        // when
+        multiPart.parse(mockRequest, tempDir);
+        
+        // Access private field to get temporary files
+        Field tempFilesField = JakartaMultiPartRequest.class.getDeclaredField("temporaryFiles");
+        tempFilesField.setAccessible(true);
+        @SuppressWarnings("unchecked")
+        List<File> temporaryFiles = (List<File>) 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<File> temporaryFiles = (List<File>) 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);
+        }
+    }
+
 }
