Skip to content

Commit 0bc1a43

Browse files
lukaszlenartlukaszlenart
authored
Merge pull request #1307 from apache/fix/WW-5366-empty-file
WW-5366 Rejects empty files during upload
2 parents 014c2bd + 0ecd95a commit 0bc1a43

File tree

7 files changed

+182
-26
lines changed

7 files changed

+182
-26
lines changed

‎.claude/settings.local.json

Lines changed: 0 additions & 11 deletions
This file was deleted.

‎.gitignore

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,3 +46,6 @@ test-output
4646

4747
# Tidelift CLI scanner
4848
.tidelift
49+
50+
# Claude Code local settings
51+
.claude/

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

Lines changed: 42 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,22 +18,22 @@
1818
*/
1919
package org.apache.struts2.dispatcher.multipart;
2020

21-
import org.apache.commons.fileupload2.core.DiskFileItemFactory;
22-
import org.apache.commons.fileupload2.core.RequestContext;
23-
import org.apache.struts2.inject.Inject;
2421
import jakarta.servlet.http.HttpServletRequest;
22+
import org.apache.commons.fileupload2.core.DiskFileItemFactory;
2523
import org.apache.commons.fileupload2.core.FileUploadByteCountLimitException;
2624
import org.apache.commons.fileupload2.core.FileUploadContentTypeException;
2725
import org.apache.commons.fileupload2.core.FileUploadException;
2826
import org.apache.commons.fileupload2.core.FileUploadFileCountLimitException;
2927
import org.apache.commons.fileupload2.core.FileUploadSizeException;
28+
import org.apache.commons.fileupload2.core.RequestContext;
3029
import org.apache.commons.fileupload2.jakarta.servlet6.JakartaServletDiskFileUpload;
3130
import org.apache.commons.io.FilenameUtils;
3231
import org.apache.commons.lang3.StringUtils;
3332
import org.apache.logging.log4j.LogManager;
3433
import org.apache.logging.log4j.Logger;
3534
import org.apache.struts2.StrutsConstants;
3635
import org.apache.struts2.dispatcher.LocalizedMessage;
36+
import org.apache.struts2.inject.Inject;
3737

3838
import java.io.File;
3939
import java.io.IOException;
@@ -424,6 +424,45 @@ protected File createTemporaryFile(String fileName, Path location) {
424424
return file;
425425
}
426426

427+
/**
428+
* Validates that an uploaded file is not empty (0 bytes) and adds an error if it is.
429+
*
430+
* <p>Empty file uploads are rejected as they are not considered valid uploads.
431+
* This validation ensures consistent behavior across all multipart implementations
432+
* and provides proper user feedback when empty files are uploaded.</p>
433+
*
434+
* <p>When an empty file is detected:</p>
435+
* <ul>
436+
* <li>A debug log message is written with field name and filename</li>
437+
* <li>A localized error message is created and added to the errors list</li>
438+
* <li>The method returns true to indicate the file should be rejected</li>
439+
* </ul>
440+
*
441+
* @param fileSize the size of the uploaded file in bytes
442+
* @param fileName the original filename of the uploaded file
443+
* @param fieldName the form field name containing the file upload
444+
* @return true if the file is empty and should be rejected, false otherwise
445+
* @see #buildErrorMessage(Class, String, Object[])
446+
*/
447+
protected boolean rejectEmptyFile(long fileSize, String fileName, String fieldName) {
448+
if (fileSize == 0) {
449+
if (LOG.isDebugEnabled()) {
450+
LOG.debug("Rejecting empty file upload for field: {} with filename: {}",
451+
normalizeSpace(fieldName), normalizeSpace(fileName));
452+
}
453+
LocalizedMessage errorMessage = buildErrorMessage(
454+
IllegalArgumentException.class,
455+
"Empty files are not allowed",
456+
new Object[]{fileName, fieldName}
457+
);
458+
if (!errors.contains(errorMessage)) {
459+
errors.add(errorMessage);
460+
}
461+
return true;
462+
}
463+
return false;
464+
}
465+
427466
/* (non-Javadoc)
428467
* @see org.apache.struts2.dispatcher.multipart.MultiPartRequest#cleanUp()
429468
*/

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

Lines changed: 10 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@
3030
import java.io.File;
3131
import java.io.IOException;
3232
import java.nio.charset.Charset;
33+
import java.nio.file.Files;
3334
import java.nio.file.Path;
3435
import java.util.ArrayList;
3536
import java.util.List;
@@ -206,6 +207,11 @@ protected void processFileField(DiskFileItem item, String saveDir) {
206207
return;
207208
}
208209

210+
// Reject empty files (0 bytes) as they are not considered valid uploads
211+
if (rejectEmptyFile(item.getSize(), item.getName(), fieldName)) {
212+
return;
213+
}
214+
209215
List<UploadedFile> values = uploadedFiles.computeIfAbsent(fieldName, k -> new ArrayList<>());
210216

211217
if (item.isInMemory()) {
@@ -285,11 +291,8 @@ protected void cleanUpDiskFileItems() {
285291
if (LOG.isDebugEnabled()) {
286292
LOG.debug("Cleaning up disk item: {} at {}", normalizeSpace(item.getFieldName()), itemPath);
287293
}
288-
if (itemPath != null) {
289-
File itemFile = itemPath.toFile();
290-
if (itemFile.exists() && !itemFile.delete()) {
291-
LOG.warn("There was a problem attempting to delete temporary file: {}", itemPath);
292-
}
294+
if (!Files.deleteIfExists(itemPath)) {
295+
LOG.warn("There was a problem attempting to delete uploaded file: {}", itemPath);
293296
}
294297
}
295298
} catch (Exception e) {
@@ -325,13 +328,8 @@ protected void cleanUpTemporaryFiles() {
325328
LOG.debug("Cleaning up {} temporary files created for in-memory uploads", temporaryFiles.size());
326329
for (File tempFile : temporaryFiles) {
327330
try {
328-
if (tempFile.exists()) {
329-
LOG.debug("Deleting temporary file: {}", tempFile.getAbsolutePath());
330-
if (!tempFile.delete()) {
331-
LOG.warn("There was a problem attempting to delete temporary file: {}", tempFile.getAbsolutePath());
332-
}
333-
} else {
334-
LOG.debug("Temporary file already deleted: {}", tempFile.getAbsolutePath());
331+
if (!Files.deleteIfExists(tempFile.toPath())) {
332+
LOG.warn("There was a problem attempting to delete temporary file: {}", tempFile.getAbsolutePath());
335333
}
336334
} catch (Exception e) {
337335
LOG.warn("Error cleaning up temporary file: {}", tempFile.getAbsolutePath(), e);

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

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -232,6 +232,15 @@ protected void processFileItemAsFileField(FileItemInput fileItemInput, Path loca
232232

233233
File file = createTemporaryFile(fileItemInput.getName(), location);
234234
streamFileToDisk(fileItemInput, file);
235+
236+
// Reject empty files (0 bytes) as they are not considered valid uploads
237+
if (rejectEmptyFile(file.length(), fileItemInput.getName(), fileItemInput.getFieldName())) {
238+
// Clean up the empty temporary file
239+
if (!Files.deleteIfExists(file.toPath())) {
240+
LOG.warn("Failed to delete empty temporary file: {}", file.getAbsolutePath());
241+
}
242+
return;
243+
}
235244

236245
Long currentFilesSize = maxSizeOfFiles != null ? actualSizeOfUploadedFiles() : null;
237246
if (maxSizeOfFiles != null && currentFilesSize + file.length() >= maxSizeOfFiles) {

‎core/src/main/resources/org/apache/struts2/struts-messages.properties

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,11 @@ struts.messages.upload.error.FileUploadContentTypeException=Request has wrong co
7575
# Default error message when handling multi-part request
7676
struts.messages.upload.error.FileUploadException=Error parsing the multi-part request.
7777

78+
# IllegalArgumentException for empty files
79+
# 0 - original filename
80+
# 1 - field name
81+
struts.messages.upload.error.IllegalArgumentException=File {0} for field {1} is empty (0 bytes). Empty file uploads are not allowed.
82+
7883
devmode.notification=Developer Notification (set struts.devMode to false to disable this message):\n{0}
7984

8085
struts.exception.missing-package-action.with-context = There is no Action mapped for namespace [{0}] and action name [{1}] associated with context path [{2}].

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

Lines changed: 113 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -743,6 +743,119 @@ public void createTemporaryFileConsistentNaming() {
743743
tempFiles.forEach(File::delete);
744744
}
745745

746+
@Test
747+
public void emptyFileUploadsAreRejected() throws IOException {
748+
// Test that empty files (0 bytes) are rejected with proper error message
749+
String content =
750+
endline + "--" + boundary + endline +
751+
"Content-Disposition: form-data; name=\"emptyfile\"; filename=\"empty.txt\"" + endline +
752+
"Content-Type: text/plain" + endline +
753+
endline +
754+
// No content - this creates a 0-byte file
755+
endline + "--" + boundary + "--";
756+
757+
mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8));
758+
759+
// when
760+
multiPart.parse(mockRequest, tempDir);
761+
762+
// then - should reject empty file and add error
763+
assertThat(multiPart.getErrors())
764+
.hasSize(1)
765+
.first()
766+
.satisfies(error -> {
767+
assertThat(error.getTextKey()).isEqualTo("struts.messages.upload.error.IllegalArgumentException");
768+
assertThat(error.getArgs()).containsExactly("empty.txt", "emptyfile");
769+
});
770+
assertThat(multiPart.uploadedFiles).isEmpty();
771+
assertThat(multiPart.getFile("emptyfile")).isEmpty();
772+
}
773+
774+
@Test
775+
public void mixedEmptyAndValidFilesProcessedCorrectly() throws IOException {
776+
// Test that valid files are processed while empty files are rejected
777+
String content =
778+
endline + "--" + boundary + endline +
779+
"Content-Disposition: form-data; name=\"emptyfile1\"; filename=\"empty1.txt\"" + endline +
780+
"Content-Type: text/plain" + endline +
781+
endline +
782+
// No content - empty file
783+
endline + "--" + boundary + endline +
784+
"Content-Disposition: form-data; name=\"validfile\"; filename=\"valid.txt\"" + endline +
785+
"Content-Type: text/plain" + endline +
786+
endline +
787+
"some valid content" +
788+
endline + "--" + boundary + endline +
789+
"Content-Disposition: form-data; name=\"emptyfile2\"; filename=\"empty2.txt\"" + endline +
790+
"Content-Type: application/octet-stream" + endline +
791+
endline +
792+
// Another empty file
793+
endline + "--" + boundary + "--";
794+
795+
mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8));
796+
797+
// when
798+
multiPart.parse(mockRequest, tempDir);
799+
800+
// then - should have 2 errors for empty files, 1 valid file processed
801+
assertThat(multiPart.getErrors()).hasSize(2);
802+
assertThat(multiPart.getErrors().get(0))
803+
.satisfies(error -> {
804+
assertThat(error.getTextKey()).isEqualTo("struts.messages.upload.error.IllegalArgumentException");
805+
assertThat(error.getArgs()).containsExactly("empty1.txt", "emptyfile1");
806+
});
807+
assertThat(multiPart.getErrors().get(1))
808+
.satisfies(error -> {
809+
assertThat(error.getTextKey()).isEqualTo("struts.messages.upload.error.IllegalArgumentException");
810+
assertThat(error.getArgs()).containsExactly("empty2.txt", "emptyfile2");
811+
});
812+
813+
// Only the valid file should be processed
814+
assertThat(multiPart.uploadedFiles).hasSize(1);
815+
assertThat(multiPart.getFile("validfile")).hasSize(1);
816+
assertThat(multiPart.getFile("emptyfile1")).isEmpty();
817+
assertThat(multiPart.getFile("emptyfile2")).isEmpty();
818+
819+
// Verify valid file content
820+
assertThat(multiPart.getFile("validfile")[0].getContent())
821+
.asInstanceOf(InstanceOfAssertFactories.FILE)
822+
.content()
823+
.isEqualTo("some valid content");
824+
}
825+
826+
@Test
827+
public void emptyFileTemporaryFileCleanup() throws IOException {
828+
// Test that temporary files for empty files are properly cleaned up
829+
String content =
830+
endline + "--" + boundary + endline +
831+
"Content-Disposition: form-data; name=\"emptyfile\"; filename=\"empty.txt\"" + endline +
832+
"Content-Type: text/plain" + endline +
833+
endline +
834+
// Empty file
835+
endline + "--" + boundary + "--";
836+
837+
mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8));
838+
839+
// Count temp files before processing
840+
File[] tempFilesBefore = new File(tempDir).listFiles((dir, name) -> name.startsWith("upload_") && name.endsWith(".tmp"));
841+
int countBefore = tempFilesBefore != null ? tempFilesBefore.length : 0;
842+
843+
// when
844+
multiPart.parse(mockRequest, tempDir);
845+
846+
// then - should reject empty file and clean up temp file
847+
assertThat(multiPart.getErrors()).hasSize(1);
848+
assertThat(multiPart.uploadedFiles).isEmpty();
849+
850+
// Verify that temporary files are cleaned up (may have implementation differences)
851+
// Some implementations create temp files first, others don't create any for empty uploads
852+
File[] tempFilesAfter = new File(tempDir).listFiles((dir, name) -> name.startsWith("upload_") && name.endsWith(".tmp"));
853+
int countAfter = tempFilesAfter != null ? tempFilesAfter.length : 0;
854+
855+
// Allow for implementation differences - just ensure no new temp files remain
856+
assertThat(countAfter).isLessThanOrEqualTo(countBefore);
857+
}
858+
746859
protected String formFile(String fieldName, String filename, String content) {
747860
return endline +
748861
"--" + boundary + endline +

0 commit comments

Comments
 (0)