lukaszlenart commented on code in PR #1806:
URL: https://github.com/apache/struts/pull/1806#discussion_r3632192101
##########
core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java:
##########
@@ -115,18 +115,29 @@ protected void processUpload(HttpServletRequest request,
String saveDir) throws
RequestContext requestContext = createRequestContext(request);
- for (DiskFileItem item :
servletFileUpload.parseRequest(requestContext)) {
- // Track all DiskFileItem instances for cleanup - this is critical
for security
- // as it ensures temporary files are properly cleaned up even if
processing fails
- diskFileItems.add(item);
-
+ int fileCount = 0;
+ int parameterCount = 0;
+ // parseRequest() fully materializes every part (spilling large ones
to disk) before we
+ // iterate, so register them all for cleanup up front - otherwise a
fail-closed breach
+ // mid-loop would leak the temp files of every part after the
breaching one.
+ List<DiskFileItem> items =
servletFileUpload.parseRequest(requestContext);
+ diskFileItems.addAll(items);
+ for (DiskFileItem item : items) {
LOG.debug(() -> "Processing a form field: " +
normalizeSpace(item.getFieldName()));
if (item.isFormField()) {
// Process regular form fields (text inputs, checkboxes, etc.)
+ if (item.getFieldName() != null) {
+ enforceMaxParameterCount(parameterCount,
item.getFieldName());
+ parameterCount++;
+ }
processNormalFormField(item, charset);
} else {
- // Process file upload fields
+ // Process file upload fields (only count parts that carry an
actual file)
LOG.debug(() -> "Processing a file: " +
normalizeSpace(item.getFieldName()));
+ if (item.getName() != null &&
!item.getName().trim().isEmpty()) {
+ enforceMaxFiles(fileCount, item.getName());
+ fileCount++;
+ }
Review Comment:
Addressed in c566aa42d. Worth noting: commons-fileupload2 `parseRequest`
silently drops parts that have no `name` attribute before we ever iterate, so
this divergence is not actually reachable via the `jakarta` path. I have
nonetheless added `&& item.getFieldName() != null` to the count/enforce guard
so the accept-criteria matches `JakartaStreamMultiPartRequest`. Thanks!
##########
core/src/main/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequest.java:
##########
@@ -226,9 +239,10 @@ protected JakartaServletDiskFileUpload
prepareServletFileUpload(Charset charset,
LOG.debug("Applies max size: {} to file upload request", maxSize);
servletFileUpload.setMaxSize(maxSize);
}
- if (maxFiles != null) {
- LOG.debug("Applies max files number: {} to file upload request",
maxFiles);
- servletFileUpload.setMaxFileCount(maxFiles);
+ if (maxFiles != null && maxFiles >= 0 && maxParameterCount != null &&
maxParameterCount >= 0) {
+ long maxParts = maxFiles + maxParameterCount;
+ LOG.debug("Applies total parts backstop: {} to file upload
request", maxParts);
+ servletFileUpload.setMaxFileCount(maxParts);
}
Review Comment:
Fixed in c566aa42d — the total-parts backstop now uses
`Math.addExact(maxFiles, maxParameterCount)` and clamps to `Long.MAX_VALUE` on
overflow, so it can no longer wrap to a negative value and silently disable the
commons file-count backstop. Thanks!
##########
core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequestTest.java:
##########
@@ -422,6 +422,74 @@ public void errorDuplicationPrevention() throws
IOException {
assertThat(multiPartRequest.getErrors()).hasSize(1);
}
+ @Test
+ public void manyFormFieldsWithFewFilesAreAccepted() throws IOException {
+ // Regression for WW-5474: maxFiles must not count form fields.
+ StringBuilder content = new StringBuilder();
+ for (int i = 0; i < 10; i++) {
+ content.append(formField("field" + i, "value" + i));
+ }
+ content.append(formFile("file1", "test1.csv", "1,2,3,4"));
+ content.append(formFile("file2", "test2.csv", "5,6,7,8"));
+ content.append(endline).append("--").append(boundary).append("--");
+
mockRequest.setContent(content.toString().getBytes(StandardCharsets.UTF_8));
+
+ multiPart.setMaxFiles("2"); // only 2 files, but 10 fields present
+ multiPart.parse(mockRequest, tempDir);
+
+ assertThat(multiPart.getErrors()).isEmpty();
+ assertThat(multiPart.getFileParameterNames().asIterator()).toIterable()
+
.asInstanceOf(InstanceOfAssertFactories.LIST).containsOnly("file1", "file2");
Review Comment:
False positive here: AssertJ `IteratorAssert.toIterable()` drains the
iterator into a `List`, so `asInstanceOf(InstanceOfAssertFactories.LIST)`
succeeds — no `ClassCastException` occurs (all tests pass). This idiom is
already used in several existing `AbstractMultiPartRequestTest` assertions, so
I am keeping it consistent with the surrounding tests.
##########
core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequestTest.java:
##########
@@ -216,14 +216,63 @@ public void exceedsMaxFilesPath() throws IOException {
// when - set max files to 1
multiPart.setMaxFiles("1");
multiPart.parse(mockRequest, tempDir);
-
- // then - should have only 1 file and errors for others
- assertThat(multiPart.uploadedFiles).hasSize(1);
+
+ // then - fail-closed: no partial files, one error
+ assertThat(multiPart.uploadedFiles).isEmpty();
assertThat(multiPart.getErrors())
- .isNotEmpty()
- .allSatisfy(error ->
-
assertThat(error.getTextKey()).isEqualTo("struts.messages.upload.error.FileUploadFileCountLimitException")
- );
+ .map(LocalizedMessage::getTextKey)
+
.containsExactly("struts.messages.upload.error.FileUploadFileCountLimitException");
+ }
+
+ @Test
+ public void streamManyFormFieldsWithFewFilesAreAccepted() throws
IOException {
+ StringBuilder content = new StringBuilder();
+ for (int i = 0; i < 10; i++) {
+ content.append(formField("field" + i, "value" + i));
+ }
+ content.append(formFile("file1", "test1.csv", "1,2,3,4"));
+ content.append(endline).append("--").append(boundary).append("--");
+
mockRequest.setContent(content.toString().getBytes(StandardCharsets.UTF_8));
+
+ multiPart.setMaxFiles("1");
+ multiPart.parse(mockRequest, tempDir);
+
+ assertThat(multiPart.getErrors()).isEmpty();
+ assertThat(multiPart.getFileParameterNames().asIterator()).toIterable()
+
.asInstanceOf(InstanceOfAssertFactories.LIST).containsOnly("file1");
Review Comment:
False positive here: AssertJ `IteratorAssert.toIterable()` drains the
iterator into a `List`, so `asInstanceOf(InstanceOfAssertFactories.LIST)`
succeeds — no `ClassCastException` occurs (all tests pass). This idiom is
already used in several existing `AbstractMultiPartRequestTest` assertions, so
I am keeping it consistent with the surrounding tests.
##########
core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java:
##########
@@ -115,18 +115,31 @@ protected void processUpload(HttpServletRequest request,
String saveDir) throws
RequestContext requestContext = createRequestContext(request);
- for (DiskFileItem item :
servletFileUpload.parseRequest(requestContext)) {
- // Track all DiskFileItem instances for cleanup - this is critical
for security
- // as it ensures temporary files are properly cleaned up even if
processing fails
- diskFileItems.add(item);
-
+ int fileCount = 0;
+ int parameterCount = 0;
+ // parseRequest() fully materializes every part (spilling large ones
to disk) before we
+ // iterate, so register them all for cleanup up front - otherwise a
fail-closed breach
+ // mid-loop would leak the temp files of every part after the
breaching one.
+ List<DiskFileItem> items =
servletFileUpload.parseRequest(requestContext);
+ diskFileItems.addAll(items);
+ for (DiskFileItem item : items) {
LOG.debug(() -> "Processing a form field: " +
normalizeSpace(item.getFieldName()));
Review Comment:
Good catch — fixed in 794c29db4 by moving the `"Processing a form field"`
debug line into the `isFormField` branch; the file branch already logs
`"Processing a file"`, so parts are no longer mislabelled.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
