[
https://issues.apache.org/jira/browse/WW-5474?focusedWorklogId=1031718&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1031718
]
ASF GitHub Bot logged work on WW-5474:
--------------------------------------
Author: ASF GitHub Bot
Created on: 22/Jul/26 17:00
Start Date: 22/Jul/26 17:00
Worklog Time Spent: 10m
Work Description: 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.
Issue Time Tracking
-------------------
Worklog Id: (was: 1031718)
Time Spent: 1h 10m (was: 1h)
> struts.multipart.maxFiles does not work as described/expected
> -------------------------------------------------------------
>
> Key: WW-5474
> URL: https://issues.apache.org/jira/browse/WW-5474
> Project: Struts 2
> Issue Type: Bug
> Affects Versions: 6.3.0
> Reporter: nikos dimitrakas
> Assignee: Lukasz Lenart
> Priority: Major
> Fix For: 7.3.0
>
> Time Spent: 1h 10m
> Remaining Estimate: 0h
>
> According to the documentation the property struts.multipart.maxFiles (which
> defaults to 256) is supposed to set a limit to how many files a multi-part
> form submission may include. But in reality, the specified value sets a limit
> to how many parameter values the form may submit, including strings,
> integers, booleans (from all input fields, checkboxes, hiddens, etc).
> The problem is in the method JakartaMultiPartRequest.parsePrequest (part of
> struts) when in the last line it calls upload.parseRequest which is in
> FileUploadBase (part of commons-fileupload). That methods tries to find the
> FileItems from the provided RequestContext, but considers every parameter
> value to be a FileItem and then throws a new FileCountLimitExceededException
> when the number of parameter values (of any type) reaches the fileCountMax.
> I am not sure if the problem is in struts or in fileupload. Maybe the
> provided RequestContext should somehow only include the file parameters so
> that only they get counted. Or perhaps the documentation should be changed to
> clarify that struts.multipart.maxFiles specifies the maximum number of
> parameter values a multipart form may include.
> I also found the issue
> [FILEUPLOAD-351|https://issues.apache.org/jira/browse/FILEUPLOAD-351] that
> seems to point out the confusing behaviour. But until the issue has been
> resolved in fileupload, struts behaves contrary to its own documentation at
> [https://struts.apache.org/core-developers/file-upload]
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
