[
https://issues.apache.org/jira/browse/WW-5474?focusedWorklogId=1031703&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1031703
]
ASF GitHub Bot logged work on WW-5474:
--------------------------------------
Author: ASF GitHub Bot
Created on: 22/Jul/26 14:57
Start Date: 22/Jul/26 14:57
Worklog Time Spent: 10m
Work Description: Copilot commented on code in PR #1806:
URL: https://github.com/apache/struts/pull/1806#discussion_r3631310969
##########
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:
`toIterable()` returns an `Iterable` wrapper around the iterator, not a
`List`. Casting it via `asInstanceOf(InstanceOfAssertFactories.LIST)` is likely
to fail at runtime (ClassCastException). You can assert the contents directly
on the `IterableAssert`.
##########
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:
This log line runs for both form fields and file parts, but always says
"Processing a form field". For file parts it becomes misleading (and you
already log "Processing a file" in the else-branch). Consider making this
message neutral to avoid confusing debug output.
##########
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:
`toIterable()` returns an `Iterable` wrapper around the iterator, not a
`List`. Casting it via `asInstanceOf(InstanceOfAssertFactories.LIST)` is likely
to fail at runtime (ClassCastException). You can assert the contents directly
on the `IterableAssert`.
Issue Time Tracking
-------------------
Worklog Id: (was: 1031703)
Time Spent: 1h (was: 50m)
> 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
> Priority: Major
> Fix For: 7.3.0
>
> Time Spent: 1h
> 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)
