diff --git a/core/src/main/java/org/apache/struts2/StrutsConstants.java b/core/src/main/java/org/apache/struts2/StrutsConstants.java index e76fac4e5d..72f04c8591 100644 --- a/core/src/main/java/org/apache/struts2/StrutsConstants.java +++ b/core/src/main/java/org/apache/struts2/StrutsConstants.java @@ -226,6 +226,11 @@ public final class StrutsConstants { */ public static final String STRUTS_MULTIPART_MAX_FILES = "struts.multipart.maxFiles"; + /** + * The maximum number of non-file form fields (parameters) allowed in a multipart request. + */ + public static final String STRUTS_MULTIPART_MAX_PARAMETER_COUNT = "struts.multipart.maxParameterCount"; + /** * The maximum length of a string parameter in a multipart request. */ diff --git a/core/src/main/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequest.java b/core/src/main/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequest.java index cb5c6b9ecd..5bd0085936 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequest.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequest.java @@ -93,6 +93,11 @@ public abstract class AbstractMultiPartRequest implements MultiPartRequest { */ protected Long maxFiles; + /** + * Specifies the maximum number of non-file form fields (parameters) in one request. + */ + protected Long maxParameterCount; + /** * Specifies the maximum length of a string parameter in a multipart request. */ @@ -160,6 +165,14 @@ public void setMaxFiles(String maxFiles) { this.maxFiles = Long.parseLong(maxFiles); } + /** + * @param maxParameterCount Injects the Struts maximum number of non-file form fields. + */ + @Inject(StrutsConstants.STRUTS_MULTIPART_MAX_PARAMETER_COUNT) + public void setMaxParameterCount(String maxParameterCount) { + this.maxParameterCount = Long.parseLong(maxParameterCount); + } + /** * @param maxFileSize Injects the Struts maximum number of files, which can be uploaded. */ @@ -226,9 +239,17 @@ 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) { + // Clamp on overflow: a wrapped-negative sum would silently disable the backstop + // (commons-fileupload2 treats a negative count as "unlimited"). + long maxParts; + try { + maxParts = Math.addExact(maxFiles, maxParameterCount); + } catch (ArithmeticException overflow) { + maxParts = Long.MAX_VALUE; + } + LOG.debug("Applies total parts backstop: {} to file upload request", maxParts); + servletFileUpload.setMaxFileCount(maxParts); } if (maxFileSize != null) { LOG.debug("Applies max size of single file: {} to file upload request", maxFileSize); @@ -298,6 +319,44 @@ protected boolean exceedsMaxStringLength(String fieldName, String fieldValue) { return false; } + /** + * Fail-closed guard: throws when accepting another file would exceed {@link #maxFiles}. + * A negative {@link #maxFiles} means "no limit", matching commons-fileupload2's own + * {@code fileCountMax = -1} convention (see {@code AbstractFileUpload.setFileCountMax}). + * + * @param currentFileCount number of files already accepted in this request + * @param fileName name of the file being considered (for logging) + */ + protected void enforceMaxFiles(int currentFileCount, String fileName) throws FileUploadFileCountLimitException { + if (maxFiles != null && maxFiles >= 0 && currentFileCount >= maxFiles) { + if (LOG.isDebugEnabled()) { + LOG.debug("Cannot accept another file: {} as it would exceed max files: {}", normalizeSpace(fileName), maxFiles); + } + throw new FileUploadFileCountLimitException( + String.format("Request exceeds allowed number of files, permitted: %s", maxFiles), + maxFiles, currentFileCount + 1L); + } + } + + /** + * Fail-closed guard: throws when accepting another form field would exceed {@link #maxParameterCount}. + * A negative {@link #maxParameterCount} means "no limit", matching the same convention as + * {@link #maxFiles}. + * + * @param currentParameterCount number of form fields already accepted in this request + * @param fieldName name of the field being considered (for logging) + */ + protected void enforceMaxParameterCount(int currentParameterCount, String fieldName) throws FileUploadParameterCountLimitException { + if (maxParameterCount != null && maxParameterCount >= 0 && currentParameterCount >= maxParameterCount) { + if (LOG.isDebugEnabled()) { + LOG.debug("Cannot accept another parameter: {} as it would exceed max parameter count: {}", normalizeSpace(fieldName), maxParameterCount); + } + throw new FileUploadParameterCountLimitException( + String.format("Request exceeds allowed number of parameters, permitted: %s", maxParameterCount), + maxParameterCount, currentParameterCount + 1L); + } + } + /** * Processes the upload. * @@ -324,10 +383,14 @@ public void parse(HttpServletRequest request, String saveDir) throws IOException } else if (e instanceof FileUploadContentTypeException ex) { exClass = ex.getClass(); args = new Object[]{ex.getContentType()}; + } else if (e instanceof FileUploadParameterCountLimitException ex) { + exClass = ex.getClass(); + args = new Object[]{ex.getPermitted(), ex.getActual()}; } LocalizedMessage errorMessage = buildErrorMessage(exClass, e.getMessage(), args); addErrorIfAbsent(errorMessage); + clearCollectedData(); } catch (IOException e) { LOG.warn("Unable to parse request", e); LocalizedMessage errorMessage = buildErrorMessage(e.getClass(), e.getMessage(), new Object[]{}); @@ -341,6 +404,23 @@ private void addErrorIfAbsent(LocalizedMessage errorMessage) { } } + /** + * Fail-closed: discards everything collected so far so a rejected request exposes + * no partial parameters or files to the action. Deletes partial upload files first + * to avoid leaking temporary files. + */ + private void clearCollectedData() { + for (List files : uploadedFiles.values()) { + for (UploadedFile file : files) { + if (file.isFile() && !file.delete()) { + LOG.warn("Could not delete partial upload file: {}", file.getName()); + } + } + } + uploadedFiles.clear(); + parameters.clear(); + } + /** * Build error message. * diff --git a/core/src/main/java/org/apache/struts2/dispatcher/multipart/FileUploadParameterCountLimitException.java b/core/src/main/java/org/apache/struts2/dispatcher/multipart/FileUploadParameterCountLimitException.java new file mode 100644 index 0000000000..f6f87b9960 --- /dev/null +++ b/core/src/main/java/org/apache/struts2/dispatcher/multipart/FileUploadParameterCountLimitException.java @@ -0,0 +1,45 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.struts2.dispatcher.multipart; + +import org.apache.commons.fileupload2.core.FileUploadException; + +/** + * Thrown when a multipart request contains more non-file form fields (parameters) + * than allowed by {@code struts.multipart.maxParameterCount}. + */ +public class FileUploadParameterCountLimitException extends FileUploadException { + + private final long permitted; + private final long actual; + + public FileUploadParameterCountLimitException(final String message, final long permitted, final long actual) { + super(message); + this.permitted = permitted; + this.actual = actual; + } + + public long getPermitted() { + return permitted; + } + + public long getActual() { + return actual; + } +} diff --git a/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java b/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java index 6b19962eb3..8c1eeaeebe 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java +++ b/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); - - LOG.debug(() -> "Processing a form field: " + normalizeSpace(item.getFieldName())); + 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 items = servletFileUpload.parseRequest(requestContext); + diskFileItems.addAll(items); + for (DiskFileItem item : items) { if (item.isFormField()) { + LOG.debug(() -> "Processing a form field: " + normalizeSpace(item.getFieldName())); // 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 would actually be accepted: + // a real filename AND a non-null field name, matching JakartaStreamMultiPartRequest + // so both parsers enforce maxFiles identically on malformed parts). LOG.debug(() -> "Processing a file: " + normalizeSpace(item.getFieldName())); + if (item.getName() != null && !item.getName().trim().isEmpty() && item.getFieldName() != null) { + enforceMaxFiles(fileCount, item.getName()); + fileCount++; + } processFileField(item, saveDir); } } diff --git a/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java b/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java index a318f0511d..62ab294d8d 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java @@ -20,7 +20,6 @@ import jakarta.servlet.http.HttpServletRequest; import org.apache.commons.fileupload2.core.FileItemInput; -import org.apache.commons.fileupload2.core.FileUploadFileCountLimitException; import org.apache.commons.fileupload2.core.FileUploadSizeException; import org.apache.commons.fileupload2.jakarta.servlet6.JakartaServletDiskFileUpload; import org.apache.logging.log4j.LogManager; @@ -54,6 +53,9 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest { private static final Logger LOG = LogManager.getLogger(JakartaStreamMultiPartRequest.class); + private int fileCount; + private int parameterCount; + /** * Processes the upload. * @@ -64,6 +66,8 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest { protected void processUpload(HttpServletRequest request, String saveDir) throws IOException { Charset charset = readCharsetEncoding(request); Path location = Path.of(saveDir); + fileCount = 0; + parameterCount = 0; JakartaServletDiskFileUpload servletFileUpload = prepareServletFileUpload(charset, location); @@ -130,6 +134,9 @@ protected void processFileItemAsFormField(FileItemInput fileItemInput) throws IO return; } + enforceMaxParameterCount(parameterCount, fieldName); + parameterCount++; + String fieldValue = readStream(fileItemInput.getInputStream()); if (exceedsMaxStringLength(fieldName, fieldValue)) { return; @@ -148,26 +155,6 @@ protected Long actualSizeOfUploadedFiles() { .reduce(0L, Long::sum); } - private boolean exceedsMaxFiles(FileItemInput fileItemInput) { - if (maxFiles != null && maxFiles == uploadedFiles.size()) { - if (LOG.isDebugEnabled()) { - LOG.debug("Cannot accept another file: {} as it will exceed max files: {}", - normalizeSpace(fileItemInput.getName()), maxFiles); - } - LocalizedMessage errorMessage = buildErrorMessage( - FileUploadFileCountLimitException.class, - String.format("File %s exceeds allowed maximum number of files %s", - fileItemInput.getName(), maxFiles), - new Object[]{maxFiles, uploadedFiles.size()} - ); - if (!errors.contains(errorMessage)) { - errors.add(errorMessage); - } - return true; - } - return false; - } - private void exceedsMaxSizeOfFiles(FileItemInput fileItemInput, File file, Long currentFilesSize) { if (LOG.isDebugEnabled()) { LOG.debug("File: {} of size: {} exceeds allowed max size: {}, actual size of already uploaded files: {}", @@ -226,9 +213,8 @@ protected void processFileItemAsFileField(FileItemInput fileItemInput, Path loca return; } - if (exceedsMaxFiles(fileItemInput)) { - return; - } + enforceMaxFiles(fileCount, fileItemInput.getName()); + fileCount++; File file = createTemporaryFile(fileItemInput.getName(), location); streamFileToDisk(fileItemInput, file); diff --git a/core/src/main/resources/org/apache/struts2/default.properties b/core/src/main/resources/org/apache/struts2/default.properties index 1c501cb330..f742c0798d 100644 --- a/core/src/main/resources/org/apache/struts2/default.properties +++ b/core/src/main/resources/org/apache/struts2/default.properties @@ -67,7 +67,10 @@ struts.multipart.parser=jakarta ### Uses jakarta.servlet.context.tempdir by default struts.multipart.saveDir= struts.multipart.maxSize=2097152 +# Maximum number of uploaded files (files only, not form fields) struts.multipart.maxFiles=256 +# Maximum number of non-file form fields (parameters) +struts.multipart.maxParameterCount=256 struts.multipart.maxStringLength=4096 # struts.multipart.maxFileSize= diff --git a/core/src/main/resources/org/apache/struts2/struts-messages.properties b/core/src/main/resources/org/apache/struts2/struts-messages.properties index 2e2eb30f9c..63514b000f 100644 --- a/core/src/main/resources/org/apache/struts2/struts-messages.properties +++ b/core/src/main/resources/org/apache/struts2/struts-messages.properties @@ -63,6 +63,10 @@ struts.messages.upload.error.FileUploadByteCountLimitException=File {1} assigned # 0 - limit struts.messages.upload.error.FileUploadFileCountLimitException=Request exceeded allowed number of files! Permitted number of files is: {0}! +# FileUploadParameterCountLimitException +# 0 - limit +struts.messages.upload.error.FileUploadParameterCountLimitException=Request exceeded allowed number of parameters! Permitted number of parameters is: {0}! + # FileUploadSizeException # 1 - permitted size # 2 - actual size diff --git a/core/src/test/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequestTest.java b/core/src/test/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequestTest.java index 6a2450201a..e2abbd6bc6 100644 --- a/core/src/test/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequestTest.java +++ b/core/src/test/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequestTest.java @@ -389,6 +389,12 @@ public void maxFiles() throws IOException { .containsExactly("struts.messages.upload.error.FileUploadFileCountLimitException"); } + @Test + public void maxParameterCountSetterStoresValue() { + multiPart.setMaxParameterCount("42"); + assertThat(multiPart.maxParameterCount).isEqualTo(42L); + } + @Test public void maxStringLength() throws IOException { String content = formFile("file1", "test1.csv", "1,2,3,4") + diff --git a/core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequestTest.java b/core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequestTest.java index 15b59f5dd2..97a374af6a 100644 --- a/core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequestTest.java +++ b/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"); + } + + @Test + public void exceedsMaxFilesIsFailClosed() throws IOException { + String content = formField("param1", "value1") + + formFile("file1", "test1.csv", "1,2,3,4") + + formFile("file2", "test2.csv", "5,6,7,8") + + endline + "--" + boundary + "--"; + mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8)); + + multiPart.setMaxFiles("1"); + multiPart.parse(mockRequest, tempDir); + + assertThat(multiPart.getErrors()).map(LocalizedMessage::getTextKey) + .containsExactly("struts.messages.upload.error.FileUploadFileCountLimitException"); + assertThat(multiPart.getFileParameterNames().asIterator()).toIterable().isEmpty(); + assertThat(multiPart.getParameterNames().asIterator()).toIterable().isEmpty(); + } + + @Test + public void exceedsMaxParameterCountIsFailClosed() throws IOException { + String content = formField("field1", "a") + + formField("field2", "b") + + formField("field3", "c") + + endline + "--" + boundary + "--"; + mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8)); + + multiPart.setMaxParameterCount("2"); + multiPart.parse(mockRequest, tempDir); + + assertThat(multiPart.getErrors()).map(LocalizedMessage::getTextKey) + .containsExactly("struts.messages.upload.error.FileUploadParameterCountLimitException"); + assertThat(multiPart.getParameterNames().asIterator()).toIterable().isEmpty(); + } + + @Test + public void multipleFilesUnderOneFieldNameAreCounted() throws IOException { + String content = formFile("file", "a.csv", "1") + + formFile("file", "b.csv", "2") + + formFile("file", "c.csv", "3") + + endline + "--" + boundary + "--"; + mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8)); + + multiPart.setMaxFiles("2"); // 3 files share one field name -> still 3 files + multiPart.parse(mockRequest, tempDir); + + assertThat(multiPart.getErrors()).map(LocalizedMessage::getTextKey) + .containsExactly("struts.messages.upload.error.FileUploadFileCountLimitException"); + } + @Test public void processFileFieldHandlesEmptyFileName() throws IOException { String content = @@ -453,4 +521,73 @@ public void processFileFieldHandlesEmptyFileName() throws IOException { .isEqualTo("valid file content"); } + @Test + public void unlimitedMaxFilesIsNotClampedByTotalPartsBackstop() throws IOException { + // Regression for WW-5474: maxFiles=-1 (unlimited) combined with a finite + // maxParameterCount must not compute a finite total-parts backstop + // (maxFiles + maxParameterCount) and pass it to commons-fileupload2's + // setMaxFileCount, which counts ALL parts. Before the fix, -1 + 256 = 255 + // wrongly rejected a 300-file/0-field upload at part 256. + int fileCount = 300; + StringBuilder content = new StringBuilder(); + for (int i = 0; i < fileCount; i++) { + content.append(formFile("file" + i, "test" + i + ".csv", "1,2,3,4")); + } + content.append(endline).append("--").append(boundary).append("--"); + mockRequest.setContent(content.toString().getBytes(StandardCharsets.UTF_8)); + + multiPart.setMaxSize("-1"); // isolate: don't let the size backstop interfere + multiPart.setMaxFiles("-1"); // unlimited files + multiPart.setMaxParameterCount("256"); // finite, but must not clamp file count + + multiPart.parse(mockRequest, tempDir); + + assertThat(multiPart.getErrors()).isEmpty(); + assertThat(multiPart.getFileParameterNames().asIterator()).toIterable().hasSize(fileCount); + } + + @Test + public void trailingDiskFileItemsAreCleanedUpAfterFailClosedBreach() throws IOException { + // Regression for WW-5474: parseRequest() fully materializes every part - spilling + // large ones to disk - before processUpload() ever starts iterating. If items were + // only registered for cleanup as the loop reached them, a fail-closed breach mid-loop + // (e.g. enforceMaxFiles throwing) would leak the temp files of every part positioned + // after the breaching one, since cleanUp()/cleanUpDiskFileItems() never learns about them. + // Use a fresh, empty saveDir so the leftover-file assertion isn't polluted by other tests. + File freshSaveDir = Files.createTempDirectory("struts-ww5474-leak-test").toFile(); + try { + StringBuilder content = new StringBuilder(); + for (int i = 1; i <= 5; i++) { + content.append(formFile("file" + i, "test" + i + ".csv", "1,2,3,4")); + } + content.append(endline).append("--").append(boundary).append("--"); + mockRequest.setContent(content.toString().getBytes(StandardCharsets.UTF_8)); + + // force every part to spill to disk instead of staying in-memory + multiPart.setBufferSize("1"); + multiPart.setMaxFiles("2"); // breach occurs at file3, leaving file4/file5 as trailing parts + multiPart.parse(mockRequest, freshSaveDir.getAbsolutePath()); + + // then - fail-closed: breach reported, no files exposed + assertThat(multiPart.getErrors()).map(LocalizedMessage::getTextKey) + .containsExactly("struts.messages.upload.error.FileUploadFileCountLimitException"); + assertThat(multiPart.getFileParameterNames().asIterator()).toIterable().isEmpty(); + + // when - cleanup runs + multiPart.cleanUp(); + + // then - no leftover temp files remain, including the trailing parts after the breach + File[] leftover = freshSaveDir.listFiles((dir, name) -> name.startsWith("upload_") && name.endsWith(".tmp")); + assertThat(leftover).isNullOrEmpty(); + } finally { + File[] remaining = freshSaveDir.listFiles(); + if (remaining != null) { + for (File f : remaining) { + f.delete(); + } + } + freshSaveDir.delete(); + } + } + } diff --git a/core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequestTest.java b/core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequestTest.java index fc78021f54..988be53ebd 100644 --- a/core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequestTest.java +++ b/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"); + } + + @Test + public void streamMultipleFilesUnderOneFieldNameAreCounted() throws IOException { + String content = formFile("file", "a.csv", "1") + + formFile("file", "b.csv", "2") + + formFile("file", "c.csv", "3") + + endline + "--" + boundary + "--"; + mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8)); + + multiPart.setMaxFiles("2"); + multiPart.parse(mockRequest, tempDir); + + // Field-name counting bug would keep all 3 under one key; files-only counting rejects. + assertThat(multiPart.uploadedFiles).isEmpty(); + assertThat(multiPart.getErrors()).map(LocalizedMessage::getTextKey) + .containsExactly("struts.messages.upload.error.FileUploadFileCountLimitException"); + } + + @Test + public void streamExceedsMaxParameterCountIsFailClosed() throws IOException { + String content = formField("field1", "a") + + formField("field2", "b") + + formField("field3", "c") + + endline + "--" + boundary + "--"; + mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8)); + + multiPart.setMaxParameterCount("2"); + multiPart.parse(mockRequest, tempDir); + + assertThat(multiPart.getErrors()).map(LocalizedMessage::getTextKey) + .containsExactly("struts.messages.upload.error.FileUploadParameterCountLimitException"); + assertThat(multiPart.getParameterNames().asIterator()).toIterable().isEmpty(); } @Test diff --git a/docs/superpowers/plans/2026-07-22-WW-5474-multipart-maxfiles-semantics.md b/docs/superpowers/plans/2026-07-22-WW-5474-multipart-maxfiles-semantics.md new file mode 100644 index 0000000000..11dd4385d4 --- /dev/null +++ b/docs/superpowers/plans/2026-07-22-WW-5474-multipart-maxfiles-semantics.md @@ -0,0 +1,594 @@ +# WW-5474 — Files-only `maxFiles` + new `maxParameterCount` Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Make `struts.multipart.maxFiles` count uploaded *files only* (identically in the `jakarta` and `jakarta-stream` parsers) and add `struts.multipart.maxParameterCount` to cap non-file form fields, failing closed on breach. + +**Architecture:** Shared enforcement lives in `AbstractMultiPartRequest` as two throwing helpers (`enforceMaxFiles`, `enforceMaxParameterCount`) plus fail-closed cleanup in `parse()`. Each parser counts file parts and form-field parts separately and calls the helpers before accepting an item. The `jakarta` parser keeps a coarse commons-fileupload2 total-parts backstop (`maxFiles + maxParameterCount`) for early abort; the `jakarta-stream` parser aborts naturally during iteration. + +**Tech Stack:** Java 17, commons-fileupload2 `2.0.0-M5` (core + jakarta-servlet6), JUnit 4 + AssertJ (multipart unit tests), JUnit 3 / XWorkTestCase (interceptor tests), Maven. + +## Global Constraints + +- Ticket prefix on every commit: `WW-5474 (): `; end commit body with `Co-Authored-By: Claude Opus 4.8 `. +- New `.java` files MUST carry the ASF license header (copy verbatim from `JakartaMultiPartRequest.java` lines 1–18). +- New config constant value: `struts.multipart.maxParameterCount`; default `256`. +- New message key: `struts.messages.upload.error.FileUploadParameterCountLimitException`. +- Build/test command: `mvn test -DskipAssembly -pl core -Dtest=[#]`. +- Semantics: `maxFiles` = number of file parts (parts with a non-empty filename); `maxParameterCount` = number of non-file form-field parts (each value counts). A limit that is `null` is not enforced. +- Fail-closed: on any limit breach, the request exposes **no** parameters and **no** files; only the recorded upload error remains. + +--- + +### Task 1: `jakarta` parser — files-only `maxFiles` + new `maxParameterCount` + +Adds all shared infrastructure and wires the default (`jakarta`) parser. + +**Files:** +- Modify: `core/src/main/java/org/apache/struts2/StrutsConstants.java` (near line 227, after `STRUTS_MULTIPART_MAX_FILES`) +- Modify: `core/src/main/resources/org/apache/struts2/default.properties:70-71` +- Modify: `core/src/main/resources/org/apache/struts2/struts-messages.properties` (after the `FileUploadFileCountLimitException` entry, ~line 65) +- Create: `core/src/main/java/org/apache/struts2/dispatcher/multipart/FileUploadParameterCountLimitException.java` +- Modify: `core/src/main/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequest.java` +- Modify: `core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java` +- Test: `core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequestTest.java` +- Test: `core/src/test/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequestTest.java` + +**Interfaces:** +- Produces (consumed by Task 2): + - `protected void AbstractMultiPartRequest.enforceMaxFiles(int currentFileCount, String fileName) throws FileUploadFileCountLimitException` + - `protected void AbstractMultiPartRequest.enforceMaxParameterCount(int currentParameterCount, String fieldName) throws FileUploadParameterCountLimitException` + - `public void AbstractMultiPartRequest.setMaxParameterCount(String)` injected from `struts.multipart.maxParameterCount` + - `FileUploadParameterCountLimitException(String message, long permitted, long actual)` with `long getPermitted()` / `long getActual()` + +- [ ] **Step 1: Add the constant** + +In `StrutsConstants.java`, immediately after the `STRUTS_MULTIPART_MAX_FILES` declaration (line 227): + +```java + /** + * The maximum number of non-file form fields (parameters) allowed in a multipart request. + */ + public static final String STRUTS_MULTIPART_MAX_PARAMETER_COUNT = "struts.multipart.maxParameterCount"; +``` + +- [ ] **Step 2: Add the default property + fix the maxFiles comment** + +In `default.properties`, replace lines 69-71: + +```properties +struts.multipart.maxSize=2097152 +struts.multipart.maxFiles=256 +struts.multipart.maxStringLength=4096 +``` + +with: + +```properties +struts.multipart.maxSize=2097152 +# Maximum number of uploaded files (files only, not form fields) +struts.multipart.maxFiles=256 +# Maximum number of non-file form fields (parameters) +struts.multipart.maxParameterCount=256 +struts.multipart.maxStringLength=4096 +``` + +- [ ] **Step 3: Add the message key** + +In `struts-messages.properties`, after the `FileUploadFileCountLimitException` line (~line 64) add: + +```properties +# FileUploadParameterCountLimitException +# 0 - limit +struts.messages.upload.error.FileUploadParameterCountLimitException=Request exceeded allowed number of parameters! Permitted number of parameters is: {0}! +``` + +- [ ] **Step 4: Create the new exception** + +Create `FileUploadParameterCountLimitException.java` (with the ASF header copied from `JakartaMultiPartRequest.java` lines 1–18): + +```java +package org.apache.struts2.dispatcher.multipart; + +import org.apache.commons.fileupload2.core.FileUploadException; + +/** + * Thrown when a multipart request contains more non-file form fields (parameters) + * than allowed by {@code struts.multipart.maxParameterCount}. + */ +public class FileUploadParameterCountLimitException extends FileUploadException { + + private final long permitted; + private final long actual; + + public FileUploadParameterCountLimitException(final String message, final long permitted, final long actual) { + super(message); + this.permitted = permitted; + this.actual = actual; + } + + public long getPermitted() { + return permitted; + } + + public long getActual() { + return actual; + } +} +``` + +- [ ] **Step 5: Add the field, setter, and enforcement helpers to `AbstractMultiPartRequest`** + +Add the import (with the other `fileupload2.core` imports near line 27): + +```java +import org.apache.commons.fileupload2.core.FileUploadFileCountLimitException; +``` + +(`FileUploadFileCountLimitException` is already imported at line 27 — verify; if present, skip.) + +Add a field after `maxFiles` (line 94): + +```java + /** + * Specifies the maximum number of non-file form fields (parameters) in one request. + */ + protected Long maxParameterCount; +``` + +Add a setter after `setMaxFiles` (after line 161): + +```java + /** + * @param maxParameterCount Injects the Struts maximum number of non-file form fields. + */ + @Inject(StrutsConstants.STRUTS_MULTIPART_MAX_PARAMETER_COUNT) + public void setMaxParameterCount(String maxParameterCount) { + this.maxParameterCount = Long.parseLong(maxParameterCount); + } +``` + +Add the two helpers after `exceedsMaxStringLength` (after line 299): + +```java + /** + * Fail-closed guard: throws when accepting another file would exceed {@link #maxFiles}. + * + * @param currentFileCount number of files already accepted in this request + * @param fileName name of the file being considered (for logging) + */ + protected void enforceMaxFiles(int currentFileCount, String fileName) throws FileUploadFileCountLimitException { + if (maxFiles != null && currentFileCount >= maxFiles) { + LOG.debug("Cannot accept another file: {} as it would exceed max files: {}", normalizeSpace(fileName), maxFiles); + throw new FileUploadFileCountLimitException( + String.format("Request exceeds allowed number of files, permitted: %s", maxFiles), + maxFiles, currentFileCount + 1L); + } + } + + /** + * Fail-closed guard: throws when accepting another form field would exceed {@link #maxParameterCount}. + * + * @param currentParameterCount number of form fields already accepted in this request + * @param fieldName name of the field being considered (for logging) + */ + protected void enforceMaxParameterCount(int currentParameterCount, String fieldName) throws FileUploadParameterCountLimitException { + if (maxParameterCount != null && currentParameterCount >= maxParameterCount) { + LOG.debug("Cannot accept another parameter: {} as it would exceed max parameter count: {}", normalizeSpace(fieldName), maxParameterCount); + throw new FileUploadParameterCountLimitException( + String.format("Request exceeds allowed number of parameters, permitted: %s", maxParameterCount), + maxParameterCount, currentParameterCount + 1L); + } + } +``` + +- [ ] **Step 6: Fail-closed cleanup + message mapping in `parse()`** + +In `AbstractMultiPartRequest.parse()` (lines 307-336), add a branch for the new exception and clear collected data on abort. Replace the `FileUploadContentTypeException` else-if block and the trailing message-building lines (lines 324-330) with: + +```java + } else if (e instanceof FileUploadContentTypeException ex) { + exClass = ex.getClass(); + args = new Object[]{ex.getContentType()}; + } else if (e instanceof FileUploadParameterCountLimitException ex) { + exClass = ex.getClass(); + args = new Object[]{ex.getPermitted(), ex.getActual()}; + } + + LocalizedMessage errorMessage = buildErrorMessage(exClass, e.getMessage(), args); + addErrorIfAbsent(errorMessage); + clearCollectedData(); +``` + +Add the private helper after `addErrorIfAbsent` (after line 342). It deletes any partial upload files before clearing so the stream parser does not leak temp files: + +```java + /** + * Fail-closed: discards everything collected so far so a rejected request exposes + * no partial parameters or files to the action. Deletes partial upload files first + * to avoid leaking temporary files. + */ + private void clearCollectedData() { + for (List files : uploadedFiles.values()) { + for (UploadedFile file : files) { + if (file.isFile() && !file.delete()) { + LOG.warn("Could not delete partial upload file: {}", file.getName()); + } + } + } + uploadedFiles.clear(); + parameters.clear(); + } +``` + +- [ ] **Step 7: `jakarta` backstop in `prepareServletFileUpload`** + +In `AbstractMultiPartRequest.prepareServletFileUpload` (lines 221-238), replace the `if (maxFiles != null) { ... setMaxFileCount(maxFiles); }` block (lines 229-232) with a coarse total-parts backstop that only constrains when both limits are set (otherwise leaving commons unlimited so it cannot re-introduce the all-parts bug): + +```java + if (maxFiles != null && maxParameterCount != null) { + long maxParts = maxFiles + maxParameterCount; + LOG.debug("Applies total parts backstop: {} to file upload request", maxParts); + servletFileUpload.setMaxFileCount(maxParts); + } +``` + +- [ ] **Step 8: Write the failing `jakarta` tests** + +Add to `JakartaMultiPartRequestTest` (uses `formFile`, `formField`, `boundary`, `endline`, `tempDir`, `mockRequest`, `multiPart` from the base class): + +```java + @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"); + } + + @Test + public void exceedsMaxFilesIsFailClosed() throws IOException { + String content = formField("param1", "value1") + + formFile("file1", "test1.csv", "1,2,3,4") + + formFile("file2", "test2.csv", "5,6,7,8") + + endline + "--" + boundary + "--"; + mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8)); + + multiPart.setMaxFiles("1"); + multiPart.parse(mockRequest, tempDir); + + assertThat(multiPart.getErrors()).map(LocalizedMessage::getTextKey) + .containsExactly("struts.messages.upload.error.FileUploadFileCountLimitException"); + assertThat(multiPart.getFileParameterNames().asIterator()).toIterable().isEmpty(); + assertThat(multiPart.getParameterNames().asIterator()).toIterable().isEmpty(); + } + + @Test + public void exceedsMaxParameterCountIsFailClosed() throws IOException { + String content = formField("field1", "a") + + formField("field2", "b") + + formField("field3", "c") + + endline + "--" + boundary + "--"; + mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8)); + + multiPart.setMaxParameterCount("2"); + multiPart.parse(mockRequest, tempDir); + + assertThat(multiPart.getErrors()).map(LocalizedMessage::getTextKey) + .containsExactly("struts.messages.upload.error.FileUploadParameterCountLimitException"); + assertThat(multiPart.getParameterNames().asIterator()).toIterable().isEmpty(); + } + + @Test + public void multipleFilesUnderOneFieldNameAreCounted() throws IOException { + String content = formFile("file", "a.csv", "1") + + formFile("file", "b.csv", "2") + + formFile("file", "c.csv", "3") + + endline + "--" + boundary + "--"; + mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8)); + + multiPart.setMaxFiles("2"); // 3 files share one field name -> still 3 files + multiPart.parse(mockRequest, tempDir); + + assertThat(multiPart.getErrors()).map(LocalizedMessage::getTextKey) + .containsExactly("struts.messages.upload.error.FileUploadFileCountLimitException"); + } +``` + +Ensure these imports exist in `JakartaMultiPartRequestTest` (add any missing): `org.assertj.core.api.InstanceOfAssertFactories`, `org.apache.struts2.dispatcher.LocalizedMessage`, `java.nio.charset.StandardCharsets`, `java.io.IOException`, `static org.assertj.core.api.Assertions.assertThat`. + +Add to `AbstractMultiPartRequestTest` (runs for both parsers; verifies only the setter, safe before Task 2): + +```java + @Test + public void maxParameterCountSetterStoresValue() { + multiPart.setMaxParameterCount("42"); + assertThat(multiPart.maxParameterCount).isEqualTo(42L); + } +``` + +- [ ] **Step 9: Run the new tests to verify they fail** + +Run: `mvn test -DskipAssembly -pl core -Dtest=JakartaMultiPartRequestTest#manyFormFieldsWithFewFilesAreAccepted+exceedsMaxParameterCountIsFailClosed` +Expected: compile error / FAIL (methods `setMaxParameterCount`, `FileUploadParameterCountLimitException` key not yet wired into the parser). If Steps 1–7 are already applied, `manyFormFieldsWithFewFilesAreAccepted` fails because the parser is not yet wired (Step 10). + +- [ ] **Step 10: Wire `JakartaMultiPartRequest.processUpload`** + +Replace the `for` loop body in `processUpload` (lines 118-132) with per-category counting: + +```java + int fileCount = 0; + int parameterCount = 0; + 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); + + 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 (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++; + } + processFileField(item, saveDir); + } + } +``` + +`processUpload` already declares `throws IOException`; the enforcement exceptions extend `FileUploadException extends IOException`, so no signature change is needed. + +- [ ] **Step 11: Run the full jakarta + regression suites** + +Run: `mvn test -DskipAssembly -pl core -Dtest=JakartaMultiPartRequestTest` +Expected: PASS (all four new tests + existing). + +Run: `mvn test -DskipAssembly -pl core -Dtest=AbstractMultiPartRequestTest` +Expected: PASS (shared `maxFiles()` still yields exactly one `FileUploadFileCountLimitException`; new setter test passes for both subclasses). + +Run: `mvn test -DskipAssembly -pl core -Dtest=ActionFileUploadInterceptorTest` +Expected: PASS. In particular `testUnacceptedNumberOfFiles` (4 files, `maxFiles=3`) still reports null files + one action error `Request exceeded allowed number of files! Permitted number of files is: 3!`. + +- [ ] **Step 12: Commit** + +```bash +git add core/src/main/java/org/apache/struts2/StrutsConstants.java \ + core/src/main/resources/org/apache/struts2/default.properties \ + core/src/main/resources/org/apache/struts2/struts-messages.properties \ + core/src/main/java/org/apache/struts2/dispatcher/multipart/FileUploadParameterCountLimitException.java \ + core/src/main/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequest.java \ + core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java \ + core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequestTest.java \ + core/src/test/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequestTest.java +git commit -m "$(cat <<'EOF' +WW-5474 fix(multipart): count files only for maxFiles, add maxParameterCount (jakarta) + +The jakarta parser passed maxFiles to commons-fileupload2 setMaxFileCount, +which counts every part (fields + files), so maxFiles wrongly limited total +parameters. Enforce a files-only count and non-file field count in Struts, +failing closed on breach; keep a total-parts commons backstop. + +Co-Authored-By: Claude Opus 4.8 +EOF +)" +``` + +--- + +### Task 2: `jakarta-stream` parser — same enforcement + +**Files:** +- Modify: `core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java` +- Test: `core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequestTest.java` + +**Interfaces:** +- Consumes (from Task 1): `enforceMaxFiles(int, String)`, `enforceMaxParameterCount(int, String)`, fail-closed `parse()` behavior, `FileUploadParameterCountLimitException`. + +- [ ] **Step 1: Update the existing `exceedsMaxFilesPath` test to fail-closed** + +In `JakartaStreamMultiPartRequestTest`, the current test asserts `uploadedFiles` retains 1 file after breach. Under fail-closed it retains none. Replace the assertions block (the `// then` section, currently `assertThat(multiPart.uploadedFiles).hasSize(1);` and the errors assertion) with: + +```java + // then - fail-closed: no partial files, one error + assertThat(multiPart.uploadedFiles).isEmpty(); + assertThat(multiPart.getErrors()) + .map(LocalizedMessage::getTextKey) + .containsExactly("struts.messages.upload.error.FileUploadFileCountLimitException"); +``` + +Add the import if missing: `import org.apache.struts2.dispatcher.LocalizedMessage;` + +- [ ] **Step 2: Add the failing stream-specific tests** + +Add to `JakartaStreamMultiPartRequestTest`: + +```java + @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"); + } + + @Test + public void streamMultipleFilesUnderOneFieldNameAreCounted() throws IOException { + String content = formFile("file", "a.csv", "1") + + formFile("file", "b.csv", "2") + + formFile("file", "c.csv", "3") + + endline + "--" + boundary + "--"; + mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8)); + + multiPart.setMaxFiles("2"); + multiPart.parse(mockRequest, tempDir); + + // Field-name counting bug would keep all 3 under one key; files-only counting rejects. + assertThat(multiPart.uploadedFiles).isEmpty(); + assertThat(multiPart.getErrors()).map(LocalizedMessage::getTextKey) + .containsExactly("struts.messages.upload.error.FileUploadFileCountLimitException"); + } + + @Test + public void streamExceedsMaxParameterCountIsFailClosed() throws IOException { + String content = formField("field1", "a") + + formField("field2", "b") + + formField("field3", "c") + + endline + "--" + boundary + "--"; + mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8)); + + multiPart.setMaxParameterCount("2"); + multiPart.parse(mockRequest, tempDir); + + assertThat(multiPart.getErrors()).map(LocalizedMessage::getTextKey) + .containsExactly("struts.messages.upload.error.FileUploadParameterCountLimitException"); + assertThat(multiPart.getParameterNames().asIterator()).toIterable().isEmpty(); + } +``` + +Ensure imports: `org.assertj.core.api.InstanceOfAssertFactories`, `java.nio.charset.StandardCharsets`, `java.io.IOException`, `static org.assertj.core.api.Assertions.assertThat`. + +- [ ] **Step 3: Run to verify they fail** + +Run: `mvn test -DskipAssembly -pl core -Dtest=JakartaStreamMultiPartRequestTest#streamMultipleFilesUnderOneFieldNameAreCounted+streamExceedsMaxParameterCountIsFailClosed` +Expected: FAIL — the old `exceedsMaxFiles` counts field names (so `file`×3 passes) and there is no parameter-count enforcement yet. + +- [ ] **Step 4: Rewire the stream parser** + +In `JakartaStreamMultiPartRequest`, remove the entire `exceedsMaxFiles(FileItemInput)` method (lines 151-169) and the unused `FileUploadFileCountLimitException` import if it becomes unused (leave it if still referenced). + +Replace `processUpload` (lines 63-81) so counters are tracked across items (instance fields, reset per parse): + +```java + private int fileCount; + private int parameterCount; + + @Override + protected void processUpload(HttpServletRequest request, String saveDir) throws IOException { + Charset charset = readCharsetEncoding(request); + Path location = Path.of(saveDir); + fileCount = 0; + parameterCount = 0; + + JakartaServletDiskFileUpload servletFileUpload = + prepareServletFileUpload(charset, location); + + LOG.debug("Using Jakarta Stream API to process request"); + servletFileUpload.getItemIterator(request).forEachRemaining(item -> { + if (item.isFormField()) { + LOG.debug(() -> "Processing a form field: " + normalizeSpace(item.getFieldName())); + processFileItemAsFormField(item); + } else { + LOG.debug(() -> "Processing a file: " + normalizeSpace(item.getFieldName())); + processFileItemAsFileField(item, location); + } + }); + } +``` + +In `processFileItemAsFormField` (lines 126-140), after the `fieldName == null` guard, enforce and count before reading: + +```java + protected void processFileItemAsFormField(FileItemInput fileItemInput) throws IOException { + String fieldName = fileItemInput.getFieldName(); + if (fieldName == null) { + LOG.warn("Form field has null fieldName, skipping"); + return; + } + + enforceMaxParameterCount(parameterCount, fieldName); + parameterCount++; + + String fieldValue = readStream(fileItemInput.getInputStream()); + if (exceedsMaxStringLength(fieldName, fieldValue)) { + return; + } + + List values = parameters.computeIfAbsent(fieldName, k -> new ArrayList<>()); + values.add(fieldValue); + } +``` + +In `processFileItemAsFileField` (lines 216-249), replace the `if (exceedsMaxFiles(fileItemInput)) { return; }` block (lines 229-231) with the shared guard, counting after the empty-name / null-fieldName guards already above it: + +```java + enforceMaxFiles(fileCount, fileItemInput.getName()); + fileCount++; +``` + +Leave the rest of `processFileItemAsFileField` (temp file creation, empty-file rejection, size checks, `createUploadedFile`) unchanged. + +- [ ] **Step 5: Run the stream suite** + +Run: `mvn test -DskipAssembly -pl core -Dtest=JakartaStreamMultiPartRequestTest` +Expected: PASS (updated `exceedsMaxFilesPath` + three new tests + existing). + +- [ ] **Step 6: Run the full multipart + interceptor regression** + +Run: `mvn test -DskipAssembly -pl core -Dtest=AbstractMultiPartRequestTest+JakartaMultiPartRequestTest+JakartaStreamMultiPartRequestTest+ActionFileUploadInterceptorTest` +Expected: PASS for all. + +- [ ] **Step 7: Commit** + +```bash +git add core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java \ + core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequestTest.java +git commit -m "$(cat <<'EOF' +WW-5474 fix(multipart): apply files-only maxFiles + maxParameterCount to stream parser + +Replace the field-name-based exceedsMaxFiles with the shared files-only +enforcement and add parameter-count enforcement, matching the jakarta parser +and failing closed on breach. + +Co-Authored-By: Claude Opus 4.8 +EOF +)" +``` + +--- + +## Self-Review + +**Spec coverage:** +- §2 files-only `maxFiles` both parsers → Task 1 Step 10, Task 2 Step 4. ✓ +- §3 new constant/default/injection → Task 1 Steps 1-2, 5. ✓ +- §4 shared `enforce*` helpers + new exception → Task 1 Steps 4-5. ✓ +- §5 fail-closed clearing + message wiring → Task 1 Steps 3, 6. ✓ +- §6 jakarta backstop + counters; stream rewire → Task 1 Steps 7, 10; Task 2 Step 4. ✓ +- §7 tests (both parsers, many-fields, over-limit files, over-limit params, multi-file-one-field, fail-closed, setter) → Task 1 Step 8, Task 2 Steps 1-2. ✓ +- §7 note about JUnit 4 multipart tests honored; interceptor JUnit 3 suite run for regression (Task 1 Step 11). ✓ + +**Placeholder scan:** No TBD/TODO; all steps carry concrete code and exact commands. ✓ + +**Type consistency:** `enforceMaxFiles(int, String)` / `enforceMaxParameterCount(int, String)` and `FileUploadParameterCountLimitException(String, long, long)` with `getPermitted()`/`getActual()` are defined in Task 1 and consumed identically in Task 2 and in `parse()`. Message key string matches across property file, tests, and exception mapping. ✓ + +**Known wrinkles (from the spec, intentional):** a gross flood over `maxFiles + maxParameterCount` on the jakarta parser surfaces the generic `FileUploadFileCountLimitException` via the commons backstop rather than a per-category message; `clearCollectedData()` tightens all `FileUploadException` abort paths (safe — no test asserts partial retention). diff --git a/docs/superpowers/specs/2026-07-22-WW-5474-multipart-maxfiles-semantics-design.md b/docs/superpowers/specs/2026-07-22-WW-5474-multipart-maxfiles-semantics-design.md new file mode 100644 index 0000000000..d6a7b29e38 --- /dev/null +++ b/docs/superpowers/specs/2026-07-22-WW-5474-multipart-maxfiles-semantics-design.md @@ -0,0 +1,100 @@ +# WW-5474 — `struts.multipart.maxFiles` counts files only, plus new `maxParameterCount` + +- **Ticket:** [WW-5474](https://issues.apache.org/jira/browse/WW-5474) — *struts.multipart.maxFiles does not work as described/expected* +- **Type:** Bug +- **Fix version:** 7.3.0 +- **Date:** 2026-07-22 + +## 1. Problem + +`struts.multipart.maxFiles` (default `256`) is documented as a cap on the **number of uploaded files**. In practice it does not behave that way, and the two Jakarta parsers disagree: + +- **`jakarta` parser (`JakartaMultiPartRequest`, the default)** passes the value to commons-fileupload2 `setMaxFileCount(maxFiles)`. In commons-fileupload2 `2.0.0-M5`, `AbstractFileUpload.parseRequest(RequestContext)` throws `FileUploadFileCountLimitException` when `itemList.size() == maxFileCount`, and `itemList` holds **every** part — form fields *and* files. So `maxFiles` actually caps the **total number of parameters**, firing spuriously on forms with many normal fields and few (or zero) files. This is the exact defect reported in WW-5474. + +- **`jakarta-stream` parser (`JakartaStreamMultiPartRequest`)** calls `getItemIterator()`. In `2.0.0-M5` the streaming iterator (`FileItemInputIteratorImpl`) enforces only `sizeMax` and `fileSizeMax` — it **ignores `maxFileCount` entirely**. The class instead has its own `exceedsMaxFiles()` which compares `maxFiles` against `uploadedFiles.size()` — the number of **distinct field names**, not the number of files. Multiple files sharing one field name collapse to one, so this both under-counts and diverges from the `jakarta` parser. + +Net result: neither parser matches the documented "maximum number of files," and the two parsers behave differently from each other. + +### Side effect being preserved + +Today the `jakarta` parser's total-part cap of 256 *accidentally* guards against a parameter-count flooding DoS. Making `maxFiles` count files only would remove that incidental guard. `maxSize` (default 2 MB) bounds total bytes but not the number of tiny parts, so we replace the incidental guard with an explicit one (see §3). + +## 2. Goal + +1. `struts.multipart.maxFiles` counts **file parts only**, identically in both parsers, matching the documentation. +2. Add `struts.multipart.maxParameterCount` (default `256`) counting **non-file form fields only**, restoring explicit DoS protection. +3. The two limits are orthogonal: a request may carry up to `maxFiles` files **and** up to `maxParameterCount` form fields. +4. Exceeding **either** limit is **fail-closed**: the request is rejected with a recorded upload error and the action receives **no partial data**. + +Out of scope: the deprecated `cos` parser (untouched); the Struts website documentation (lives in the separate `struts-site` repo — only in-repo config comments, JavaDoc, and message bundles are updated here). + +## 3. New configuration + +- **Constant:** `StrutsConstants.STRUTS_MULTIPART_MAX_PARAMETER_COUNT = "struts.multipart.maxParameterCount"`. +- **Default:** `default.properties` → `struts.multipart.maxParameterCount=256`. Update the existing `struts.multipart.maxFiles` comment to state it limits *files only*. +- **Injection:** new field `protected Long maxParameterCount` on `AbstractMultiPartRequest` with an `@Inject(StrutsConstants.STRUTS_MULTIPART_MAX_PARAMETER_COUNT)` setter, following the existing `maxFiles` setter pattern. + +| Setting | Counts | Default | +|---|---|---| +| `struts.multipart.maxFiles` | file parts only | 256 | +| `struts.multipart.maxParameterCount` (new) | non-file form fields only | 256 | + +## 4. Shared enforcement (`AbstractMultiPartRequest`) + +Two helpers, called **before** accepting each item, used by both parsers so behavior is identical: + +- `enforceMaxFiles(int currentFileCount, String fileName)` — throws `FileUploadFileCountLimitException` (commons) when accepting one more file would exceed `maxFiles`. +- `enforceMaxParameterCount(int currentParameterCount, String fieldName)` — throws a **new** `FileUploadParameterCountLimitException` when accepting one more field would exceed `maxParameterCount`. + +Both exceptions extend `FileUploadException` (which `extends IOException`), so a breach unwinds the parse loop and is caught by the existing `parse()` handler. Each helper is a no-op when its limit is unset (`null`). + +### New exception + +`org.apache.struts2.dispatcher.multipart.FileUploadParameterCountLimitException extends FileUploadException`, carrying `permitted` and `actual` counts (mirroring `FileUploadFileCountLimitException`'s shape) for the localized message args. + +## 5. Fail-closed handling in `parse()` + +`AbstractMultiPartRequest.parse()` already catches `FileUploadException` and records a `LocalizedMessage`. Changes: + +1. **Discard partial data on any abort:** clear `parameters` and `uploadedFiles` in the `FileUploadException` path so the action sees only the upload error, never a partially-populated request. Temp files are reclaimed by the existing `cleanUp()` (items are tracked *before* the limit check — see §6). This tightens *all* abort paths (`maxSize`, `maxFileSize`, the new limits); today those maps happen to be empty on abort, so it is a safe hardening rather than a behavior change for existing limits. +2. **Message wiring:** add a branch mapping `FileUploadParameterCountLimitException` to args `{permitted, actual}`, and add message key `struts.messages.upload.error.FileUploadParameterCountLimitException` to `struts-messages.properties` (matching the existing `FileUploadFileCountLimitException` entry). + +## 6. Per-parser wiring + +### `jakarta` (`JakartaMultiPartRequest`, non-streaming) + +- Stop using `setMaxFileCount` as the *file* cap. +- Keep a cheap early-abort **backstop** against gross floods by setting commons `setMaxFileCount = maxFiles + maxParameterCount` (a total-parts ceiling — commons cannot distinguish categories). This aborts pathological requests before the full item list is materialized. +- In the `processUpload` loop, track each `DiskFileItem` for cleanup **first**, then maintain separate file and field counters and call `enforceMaxFiles` / `enforceMaxParameterCount` before processing the item. Precise, category-correct errors fire before the coarse backstop in all normal over-limit cases. + +**Wrinkle (accepted):** a *gross* flood exceeding `maxFiles + maxParameterCount` combined surfaces the generic `FileUploadFileCountLimitException` with the combined total rather than a precise per-category message. Normal over-limit cases (e.g. the 257th file, or 257th field) still produce precise messages. This is an acceptable defense-in-depth tradeoff given the non-streaming parser cannot count per category before `parseRequest` returns. + +### `jakarta-stream` (`JakartaStreamMultiPartRequest`) + +- Remove the buggy `exceedsMaxFiles` (field-name counting). +- Maintain true file and field counters during iteration and call the shared `enforceMaxFiles` / `enforceMaxParameterCount` helpers. The streaming iterator aborts naturally and early on the throw. + +## 7. Testing + +JUnit 4 (`org.junit.Test`) — the multipart tests are plain JUnit 4, not `XWorkTestCase`, so new `@Test` methods run. + +Add to **both** `JakartaMultiPartRequestTest` and `JakartaStreamMultiPartRequestTest`: + +- Many form fields + few files (e.g. 300 fields, 2 files) with `maxFiles=256` now **passes** (regression for the reported bug). +- More than `maxFiles` files → fails with key `struts.messages.upload.error.FileUploadFileCountLimitException`. +- More than `maxParameterCount` fields → fails with key `struts.messages.upload.error.FileUploadParameterCountLimitException`. +- Multiple files under a single field name are counted individually (guards the stream-parser regression). +- On breach the request exposes no parameters and no files (fail-closed): `getParameterNames()` / `getFileParameterNames()` empty, error present. + +Add to `AbstractMultiPartRequestTest`: the new setter parses and stores `maxParameterCount`; default resolves to 256. + +## 8. Files touched + +- `core/.../StrutsConstants.java` — new constant. +- `core/.../default.properties` — new default + corrected `maxFiles` comment. +- `core/.../struts-messages.properties` — new message key. +- `core/.../multipart/AbstractMultiPartRequest.java` — new field/setter, two `enforce…` helpers, fail-closed clearing, message arg mapping. +- `core/.../multipart/FileUploadParameterCountLimitException.java` — new exception. +- `core/.../multipart/JakartaMultiPartRequest.java` — counters + backstop wiring. +- `core/.../multipart/JakartaStreamMultiPartRequest.java` — replace `exceedsMaxFiles`, add counters. +- Tests: `JakartaMultiPartRequestTest`, `JakartaStreamMultiPartRequestTest`, `AbstractMultiPartRequestTest`.