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`.
--
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]
