Apache/struts
1
0
Fork 0
mirror of https://github.com/apache/struts.git synced 2026-08-05 22:56:59 +00:00
Code Issues Packages Projects Releases Wiki Activity

WW-5366 Rejects empty files during upload

Browse Source
This commit is contained in:
Lukasz Lenart
2025-08-07 18:40:44 +02:00
parent dd68723a47
commit 0ecd95a16b
7 changed files with 182 additions and 26 deletions
Download Patch File Download Diff File Expand all files Collapse all files
.claude/settings.local.json
-11
View File
@@ -1,11 +0,0 @@
{
"permissions": {
"allow": [
"Bash(find:*)",
"Bash(mvn:*)",
"Bash(grep:*)",
"Bash(cat:*)"
],
"deny": []
}
}
.gitignore
+3
View File
@@ -46,3 +46,6 @@ test-output
# Tidelift CLI scanner
.tidelift
# Claude Code local settings
.claude/
core/src/main/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequest.java
+42 -3
View File
@@ -18,15 +18,14 @@
*/
package org.apache.struts2.dispatcher.multipart;
import org.apache.commons.fileupload2.core.DiskFileItemFactory;
import org.apache.commons.fileupload2.core.RequestContext;
import org.apache.struts2.inject.Inject;
import jakarta.servlet.http.HttpServletRequest;
import org.apache.commons.fileupload2.core.DiskFileItemFactory;
import org.apache.commons.fileupload2.core.FileUploadByteCountLimitException;
import org.apache.commons.fileupload2.core.FileUploadContentTypeException;
import org.apache.commons.fileupload2.core.FileUploadException;
import org.apache.commons.fileupload2.core.FileUploadFileCountLimitException;
import org.apache.commons.fileupload2.core.FileUploadSizeException;
import org.apache.commons.fileupload2.core.RequestContext;
import org.apache.commons.fileupload2.jakarta.servlet6.JakartaServletDiskFileUpload;
import org.apache.commons.io.FilenameUtils;
import org.apache.commons.lang3.StringUtils;
@@ -34,6 +33,7 @@ import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.apache.struts2.StrutsConstants;
import org.apache.struts2.dispatcher.LocalizedMessage;
import org.apache.struts2.inject.Inject;
import java.io.File;
import java.io.IOException;
@@ -424,6 +424,45 @@ public abstract class AbstractMultiPartRequest implements MultiPartRequest {
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) {
if (LOG.isDebugEnabled()) {
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()
*/
core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java
+10 -12
View File
@@ -30,6 +30,7 @@ import org.apache.struts2.dispatcher.LocalizedMessage;
import java.io.File;
import java.io.IOException;
import java.nio.charset.Charset;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.ArrayList;
import java.util.List;
@@ -206,6 +207,11 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest {
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()) {
@@ -285,11 +291,8 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest {
if (LOG.isDebugEnabled()) {
LOG.debug("Cleaning up disk item: {} at {}", normalizeSpace(item.getFieldName()), itemPath);
}
if (itemPath != null) {
File itemFile = itemPath.toFile();
if (itemFile.exists() && !itemFile.delete()) {
LOG.warn("There was a problem attempting to delete temporary file: {}", itemPath);
}
if (!Files.deleteIfExists(itemPath)) {
LOG.warn("There was a problem attempting to delete uploaded file: {}", itemPath);
}
}
} catch (Exception e) {
@@ -325,13 +328,8 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest {
LOG.debug("Cleaning up {} temporary files created for in-memory uploads", temporaryFiles.size());
for (File tempFile : temporaryFiles) {
try {
if (tempFile.exists()) {
LOG.debug("Deleting temporary file: {}", tempFile.getAbsolutePath());
if (!tempFile.delete()) {
LOG.warn("There was a problem attempting to delete temporary file: {}", tempFile.getAbsolutePath());
}
} else {
LOG.debug("Temporary file already deleted: {}", tempFile.getAbsolutePath());
if (!Files.deleteIfExists(tempFile.toPath())) {
LOG.warn("There was a problem attempting to delete temporary file: {}", tempFile.getAbsolutePath());
}
} catch (Exception e) {
LOG.warn("Error cleaning up temporary file: {}", tempFile.getAbsolutePath(), e);
core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java
+9
View File
@@ -232,6 +232,15 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest {
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 (!Files.deleteIfExists(file.toPath())) {
LOG.warn("Failed to delete empty temporary file: {}", file.getAbsolutePath());
}
return;
}
Long currentFilesSize = maxSizeOfFiles != null ? actualSizeOfUploadedFiles() : null;
if (maxSizeOfFiles != null && currentFilesSize + file.length() >= maxSizeOfFiles) {
core/src/main/resources/org/apache/struts2/struts-messages.properties
+5
View File
@@ -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}].
core/src/test/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequestTest.java
+113
View File
@@ -743,6 +743,119 @@ abstract class AbstractMultiPartRequestTest {
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 +