Skip to content

Commit 37144a0

Browse files
lukaszlenartlukaszlenart
committed
Adds missing test cases of temporary files
1 parent 824e712 commit 37144a0

File tree

5 files changed

+568
-19
lines changed

5 files changed

+568
-19
lines changed

‎core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -209,7 +209,7 @@ protected void processFileField(DiskFileItem item, String saveDir) {
209209
List<UploadedFile> values = uploadedFiles.computeIfAbsent(fieldName, k -> new ArrayList<>());
210210

211211
if (item.isInMemory()) {
212-
LOG.debug("Creating temporary file representing in-memory uploaded item: {}", normalizeSpace(item.getFieldName()));
212+
LOG.debug(() -> "Creating temporary file representing in-memory uploaded item: " + normalizeSpace(item.getFieldName()));
213213
try {
214214
File tempFile = createTemporaryFile(item.getName(), Path.of(saveDir));
215215

@@ -229,8 +229,10 @@ protected void processFileField(DiskFileItem item, String saveDir) {
229229
.build();
230230
values.add(uploadedFile);
231231

232-
LOG.debug("Created temporary file for in-memory uploaded item: {} at {}",
233-
normalizeSpace(item.getName()), tempFile.getAbsolutePath());
232+
if (LOG.isDebugEnabled()) {
233+
LOG.debug("Created temporary file for in-memory uploaded item: {} at {}",
234+
normalizeSpace(item.getName()), tempFile.getAbsolutePath());
235+
}
234236
} catch (IOException e) {
235237
LOG.warn("Failed to create temporary file for in-memory uploaded item: {}",
236238
normalizeSpace(item.getName()), e);
@@ -277,10 +279,12 @@ protected void cleanUpDiskFileItems() {
277279
for (DiskFileItem item : diskFileItems) {
278280
try {
279281
if (item.isInMemory()) {
280-
LOG.debug("Cleaning up in-memory item: {}", normalizeSpace(item.getFieldName()));
282+
LOG.debug(() -> "Cleaning up in-memory item: " + normalizeSpace(item.getFieldName()));
281283
} else {
282284
Path itemPath = item.getPath();
283-
LOG.debug("Cleaning up disk item: {} at {}", normalizeSpace(item.getFieldName()), itemPath);
285+
if (LOG.isDebugEnabled()) {
286+
LOG.debug("Cleaning up disk item: {} at {}", normalizeSpace(item.getFieldName()), itemPath);
287+
}
284288
if (itemPath != null) {
285289
File itemFile = itemPath.toFile();
286290
if (itemFile.exists() && !itemFile.delete()) {

‎core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java

Lines changed: 0 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -242,20 +242,6 @@ protected void processFileItemAsFileField(FileItemInput fileItemInput, Path loca
242242
}
243243
}
244244

245-
/**
246-
* Creates a temporary file based on the given filename and location.
247-
*
248-
* @param fileName file name
249-
* @param location location
250-
* @return a temporary file based on the given filename and location
251-
*/
252-
protected File createTemporaryFile(String fileName, Path location) {
253-
String uid = UUID.randomUUID().toString().replace("-", "_");
254-
File file = location.resolve("upload_" + uid + ".tmp").toFile();
255-
LOG.debug("Creating temporary file: {} (originally: {})", file.getName(), fileName);
256-
return file;
257-
}
258-
259245
/**
260246
* Streams the file upload stream to the specified file.
261247
*

‎core/src/test/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequestTest.java

Lines changed: 157 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,9 @@
3030
import java.io.File;
3131
import java.io.IOException;
3232
import java.nio.charset.StandardCharsets;
33+
import java.nio.file.Files;
34+
import java.nio.file.Path;
35+
import java.nio.file.Paths;
3336
import java.util.ArrayList;
3437
import java.util.List;
3538
import java.util.Map;
@@ -586,6 +589,160 @@ public void multipleFileUploadWithMixedContent() throws IOException {
586589
}
587590
}
588591

592+
@Test
593+
public void createTemporaryFileGeneratesSecureNames() {
594+
// Create a test instance to access the protected method
595+
AbstractMultiPartRequest testRequest = createMultipartRequest();
596+
Path testLocation = Paths.get(tempDir);
597+
598+
// when - create multiple temporary files
599+
File tempFile1 = testRequest.createTemporaryFile("test1.csv", testLocation);
600+
File tempFile2 = testRequest.createTemporaryFile("test2.csv", testLocation);
601+
File tempFile3 = testRequest.createTemporaryFile("../../../malicious.csv", testLocation);
602+
603+
// then - verify secure naming
604+
assertThat(tempFile1.getName()).startsWith("upload_");
605+
assertThat(tempFile1.getName()).endsWith(".tmp");
606+
assertThat(tempFile2.getName()).startsWith("upload_");
607+
assertThat(tempFile2.getName()).endsWith(".tmp");
608+
assertThat(tempFile3.getName()).startsWith("upload_");
609+
assertThat(tempFile3.getName()).endsWith(".tmp");
610+
611+
// Verify each file has a unique name
612+
assertThat(tempFile1.getName()).isNotEqualTo(tempFile2.getName());
613+
assertThat(tempFile2.getName()).isNotEqualTo(tempFile3.getName());
614+
assertThat(tempFile1.getName()).isNotEqualTo(tempFile3.getName());
615+
616+
// Verify all files are in the correct location
617+
assertThat(tempFile1.getParent()).isEqualTo(tempDir);
618+
assertThat(tempFile2.getParent()).isEqualTo(tempDir);
619+
assertThat(tempFile3.getParent()).isEqualTo(tempDir);
620+
621+
// Verify malicious filename doesn't affect the location
622+
assertThat(tempFile3.getName()).doesNotContain("..");
623+
assertThat(tempFile3.getName()).doesNotContain("/");
624+
assertThat(tempFile3.getName()).doesNotContain("\\");
625+
626+
// Clean up test files
627+
tempFile1.delete();
628+
tempFile2.delete();
629+
tempFile3.delete();
630+
}
631+
632+
@Test
633+
public void createTemporaryFileInSpecificDirectory() throws IOException {
634+
// Create a subdirectory for testing
635+
Path subDir = Paths.get(tempDir, "subdir");
636+
Files.createDirectories(subDir);
637+
638+
AbstractMultiPartRequest testRequest = createMultipartRequest();
639+
640+
// when
641+
File tempFile = testRequest.createTemporaryFile("test.csv", subDir);
642+
643+
// then - verify file is created in the specified subdirectory
644+
assertThat(tempFile.getParent()).isEqualTo(subDir.toString());
645+
assertThat(tempFile.getName()).startsWith("upload_");
646+
assertThat(tempFile.getName()).endsWith(".tmp");
647+
648+
// Clean up
649+
tempFile.delete();
650+
Files.delete(subDir);
651+
}
652+
653+
@Test
654+
public void createTemporaryFileWithNullFileName() throws IOException {
655+
AbstractMultiPartRequest testRequest = createMultipartRequest();
656+
Path testLocation = Paths.get(tempDir);
657+
658+
// when - create temp file with null filename
659+
File tempFile = testRequest.createTemporaryFile(null, testLocation);
660+
661+
// then - should still create a valid temporary file
662+
assertThat(tempFile.getName()).startsWith("upload_");
663+
assertThat(tempFile.getName()).endsWith(".tmp");
664+
assertThat(tempFile.getParent()).isEqualTo(tempDir);
665+
666+
// Clean up
667+
tempFile.delete();
668+
}
669+
670+
@Test
671+
public void createTemporaryFileWithEmptyFileName() throws IOException {
672+
AbstractMultiPartRequest testRequest = createMultipartRequest();
673+
Path testLocation = Paths.get(tempDir);
674+
675+
// when - create temp file with empty filename
676+
File tempFile = testRequest.createTemporaryFile("", testLocation);
677+
678+
// then - should still create a valid temporary file
679+
assertThat(tempFile.getName()).startsWith("upload_");
680+
assertThat(tempFile.getName()).endsWith(".tmp");
681+
assertThat(tempFile.getParent()).isEqualTo(tempDir);
682+
683+
// Clean up
684+
tempFile.delete();
685+
}
686+
687+
@Test
688+
public void createTemporaryFileWithSpecialCharacters() {
689+
AbstractMultiPartRequest testRequest = createMultipartRequest();
690+
Path testLocation = Paths.get(tempDir);
691+
692+
// when - create temp files with various special characters
693+
File tempFile1 = testRequest.createTemporaryFile("file with spaces.csv", testLocation);
694+
File tempFile2 = testRequest.createTemporaryFile("file@#$%^&*().csv", testLocation);
695+
File tempFile3 = testRequest.createTemporaryFile("файл.csv", testLocation); // Cyrillic
696+
697+
// then - all should create valid secure temporary files
698+
File[] tempFiles = {tempFile1, tempFile2, tempFile3};
699+
for (File tempFile : tempFiles) {
700+
assertThat(tempFile.getName()).startsWith("upload_");
701+
assertThat(tempFile.getName()).endsWith(".tmp");
702+
assertThat(tempFile.getParent()).isEqualTo(tempDir);
703+
// Verify no special characters leak into the actual filename
704+
assertThat(tempFile.getName()).matches("upload_[a-zA-Z0-9_]+\\.tmp");
705+
}
706+
707+
// All should have unique names
708+
assertThat(tempFile1.getName()).isNotEqualTo(tempFile2.getName());
709+
assertThat(tempFile2.getName()).isNotEqualTo(tempFile3.getName());
710+
assertThat(tempFile1.getName()).isNotEqualTo(tempFile3.getName());
711+
712+
// Clean up
713+
tempFile1.delete();
714+
tempFile2.delete();
715+
tempFile3.delete();
716+
}
717+
718+
@Test
719+
public void createTemporaryFileConsistentNaming() {
720+
AbstractMultiPartRequest testRequest = createMultipartRequest();
721+
Path testLocation = Paths.get(tempDir);
722+
723+
// when - create many temporary files to verify naming consistency
724+
List<File> tempFiles = new ArrayList<>();
725+
for (int i = 0; i < 100; i++) {
726+
tempFiles.add(testRequest.createTemporaryFile("test" + i + ".csv", testLocation));
727+
}
728+
729+
// then - all should follow the same naming pattern
730+
for (File tempFile : tempFiles) {
731+
assertThat(tempFile.getName()).startsWith("upload_");
732+
assertThat(tempFile.getName()).endsWith(".tmp");
733+
assertThat(tempFile.getParent()).isEqualTo(tempDir);
734+
// Verify UUID pattern (without hyphens, replaced with underscores)
735+
assertThat(tempFile.getName()).matches("upload_[a-zA-Z0-9_]+\\.tmp");
736+
}
737+
738+
// Verify all names are unique
739+
List<String> fileNames = tempFiles.stream().map(File::getName).toList();
740+
assertThat(fileNames).doesNotHaveDuplicates();
741+
742+
// Clean up
743+
tempFiles.forEach(File::delete);
744+
}
745+
589746
protected String formFile(String fieldName, String filename, String content) {
590747
return endline +
591748
"--" + boundary + endline +

0 commit comments

Comments
 (0)