Reuses logic to create temporary file

This commit is contained in:
Lukasz Lenart
2025-07-19 10:30:02 +02:00
parent 63f2c8bdec
commit 824e7121b0
3 changed files with 87 additions and 7 deletions
@@ -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()
*/
@@ -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 {
*
* <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>
@@ -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<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);
}
}
}