Skip to content

WW-5366 Rejects empty files during upload #1307

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 0 additions & 11 deletions .claude/settings.local.json

This file was deleted.

3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -46,3 +46,6 @@ test-output

# Tidelift CLI scanner
.tidelift

# Claude Code local settings
.claude/
Original file line number Diff line number Diff line change
Expand Up @@ -424,6 +424,43 @@ protected File createTemporaryFile(String fileName, Path location) {
return file;
}

/**
* Validates that an uploaded file is not empty (0 bytes) and adds an error if it is.
*
* <p>Empty file uploads are rejected as they are not considered valid uploads.
* This validation ensures consistent behavior across all multipart implementations
* and provides proper user feedback when empty files are uploaded.</p>
*
* <p>When an empty file is detected:</p>
* <ul>
* <li>A debug log message is written with field name and filename</li>
* <li>A localized error message is created and added to the errors list</li>
* <li>The method returns true to indicate the file should be rejected</li>
* </ul>
*
* @param fileSize the size of the uploaded file in bytes
* @param fileName the original filename of the uploaded file
* @param fieldName the form field name containing the file upload
* @return true if the file is empty and should be rejected, false otherwise
* @see #buildErrorMessage(Class, String, Object[])
*/
protected boolean rejectEmptyFile(long fileSize, String fileName, String fieldName) {
if (fileSize == 0) {
LOG.debug("Rejecting empty file upload for field: {} with filename: {}",
normalizeSpace(fieldName), normalizeSpace(fileName));
LocalizedMessage errorMessage = buildErrorMessage(
IllegalArgumentException.class,
"Empty files are not allowed",
new Object[]{fileName, fieldName}
);
if (!errors.contains(errorMessage)) {
errors.add(errorMessage);
}
return true;
}
return false;
}

/* (non-Javadoc)
* @see org.apache.struts2.dispatcher.multipart.MultiPartRequest#cleanUp()
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,11 @@ protected void processFileField(DiskFileItem item, String saveDir) {
return;
}

// Reject empty files (0 bytes) as they are not considered valid uploads
if (rejectEmptyFile(item.getSize(), item.getName(), fieldName)) {
return;
}

List<UploadedFile> values = uploadedFiles.computeIfAbsent(fieldName, k -> new ArrayList<>());

if (item.isInMemory()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,15 @@ protected void processFileItemAsFileField(FileItemInput fileItemInput, Path loca

File file = createTemporaryFile(fileItemInput.getName(), location);
streamFileToDisk(fileItemInput, file);

// Reject empty files (0 bytes) as they are not considered valid uploads
if (rejectEmptyFile(file.length(), fileItemInput.getName(), fileItemInput.getFieldName())) {
// Clean up the empty temporary file
if (file.exists() && !file.delete()) {
LOG.warn("Failed to delete empty temporary file: {}", file.getAbsolutePath());
}
return;
}

Long currentFilesSize = maxSizeOfFiles != null ? actualSizeOfUploadedFiles() : null;
if (maxSizeOfFiles != null && currentFilesSize + file.length() >= maxSizeOfFiles) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,11 @@ struts.messages.upload.error.FileUploadContentTypeException=Request has wrong co
# Default error message when handling multi-part request
struts.messages.upload.error.FileUploadException=Error parsing the multi-part request.

# IllegalArgumentException for empty files
# 0 - original filename
# 1 - field name
struts.messages.upload.error.IllegalArgumentException=File {0} for field {1} is empty (0 bytes). Empty file uploads are not allowed.

devmode.notification=Developer Notification (set struts.devMode to false to disable this message):\n{0}

struts.exception.missing-package-action.with-context = There is no Action mapped for namespace [{0}] and action name [{1}] associated with context path [{2}].
Original file line number Diff line number Diff line change
Expand Up @@ -743,6 +743,119 @@ public void createTemporaryFileConsistentNaming() {
tempFiles.forEach(File::delete);
}

@Test
public void emptyFileUploadsAreRejected() throws IOException {
// Test that empty files (0 bytes) are rejected with proper error message
String content =
endline + "--" + boundary + endline +
"Content-Disposition: form-data; name=\"emptyfile\"; filename=\"empty.txt\"" + endline +
"Content-Type: text/plain" + endline +
endline +
// No content - this creates a 0-byte file
endline + "--" + boundary + "--";

mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8));

// when
multiPart.parse(mockRequest, tempDir);

// then - should reject empty file and add error
assertThat(multiPart.getErrors())
.hasSize(1)
.first()
.satisfies(error -> {
assertThat(error.getTextKey()).isEqualTo("struts.messages.upload.error.IllegalArgumentException");
assertThat(error.getArgs()).containsExactly("empty.txt", "emptyfile");
});
assertThat(multiPart.uploadedFiles).isEmpty();
assertThat(multiPart.getFile("emptyfile")).isEmpty();
}

@Test
public void mixedEmptyAndValidFilesProcessedCorrectly() throws IOException {
// Test that valid files are processed while empty files are rejected
String content =
endline + "--" + boundary + endline +
"Content-Disposition: form-data; name=\"emptyfile1\"; filename=\"empty1.txt\"" + endline +
"Content-Type: text/plain" + endline +
endline +
// No content - empty file
endline + "--" + boundary + endline +
"Content-Disposition: form-data; name=\"validfile\"; filename=\"valid.txt\"" + endline +
"Content-Type: text/plain" + endline +
endline +
"some valid content" +
endline + "--" + boundary + endline +
"Content-Disposition: form-data; name=\"emptyfile2\"; filename=\"empty2.txt\"" + endline +
"Content-Type: application/octet-stream" + endline +
endline +
// Another empty file
endline + "--" + boundary + "--";

mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8));

// when
multiPart.parse(mockRequest, tempDir);

// then - should have 2 errors for empty files, 1 valid file processed
assertThat(multiPart.getErrors()).hasSize(2);
assertThat(multiPart.getErrors().get(0))
.satisfies(error -> {
assertThat(error.getTextKey()).isEqualTo("struts.messages.upload.error.IllegalArgumentException");
assertThat(error.getArgs()).containsExactly("empty1.txt", "emptyfile1");
});
assertThat(multiPart.getErrors().get(1))
.satisfies(error -> {
assertThat(error.getTextKey()).isEqualTo("struts.messages.upload.error.IllegalArgumentException");
assertThat(error.getArgs()).containsExactly("empty2.txt", "emptyfile2");
});

// Only the valid file should be processed
assertThat(multiPart.uploadedFiles).hasSize(1);
assertThat(multiPart.getFile("validfile")).hasSize(1);
assertThat(multiPart.getFile("emptyfile1")).isEmpty();
assertThat(multiPart.getFile("emptyfile2")).isEmpty();

// Verify valid file content
assertThat(multiPart.getFile("validfile")[0].getContent())
.asInstanceOf(InstanceOfAssertFactories.FILE)
.content()
.isEqualTo("some valid content");
}

@Test
public void emptyFileTemporaryFileCleanup() throws IOException {
// Test that temporary files for empty files are properly cleaned up
String content =
endline + "--" + boundary + endline +
"Content-Disposition: form-data; name=\"emptyfile\"; filename=\"empty.txt\"" + endline +
"Content-Type: text/plain" + endline +
endline +
// Empty file
endline + "--" + boundary + "--";

mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8));

// Count temp files before processing
File[] tempFilesBefore = new File(tempDir).listFiles((dir, name) -> name.startsWith("upload_") && name.endsWith(".tmp"));
int countBefore = tempFilesBefore != null ? tempFilesBefore.length : 0;

// when
multiPart.parse(mockRequest, tempDir);

// then - should reject empty file and clean up temp file
assertThat(multiPart.getErrors()).hasSize(1);
assertThat(multiPart.uploadedFiles).isEmpty();

// Verify that temporary files are cleaned up (may have implementation differences)
// Some implementations create temp files first, others don't create any for empty uploads
File[] tempFilesAfter = new File(tempDir).listFiles((dir, name) -> name.startsWith("upload_") && name.endsWith(".tmp"));
int countAfter = tempFilesAfter != null ? tempFilesAfter.length : 0;

// Allow for implementation differences - just ensure no new temp files remain
assertThat(countAfter).isLessThanOrEqualTo(countBefore);
}

protected String formFile(String fieldName, String filename, String content) {
return endline +
"--" + boundary + endline +
Expand Down