This is an automated email from the ASF dual-hosted git repository. lukaszlenart pushed a commit to branch WW-5474-multipart-maxfiles-semantics in repository https://gitbox.apache.org/repos/asf/struts.git
commit 8b530a61f99d95ed0d2f4b1b5851f8525bd6ebda Author: Lukasz Lenart <[email protected]> AuthorDate: Wed Jul 22 15:27:57 2026 +0200 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 <[email protected]> --- .../java/org/apache/struts2/StrutsConstants.java | 5 ++ .../multipart/AbstractMultiPartRequest.java | 75 +++++++++++++++++++++- .../FileUploadParameterCountLimitException.java | 45 +++++++++++++ .../multipart/JakartaMultiPartRequest.java | 12 +++- .../org/apache/struts2/default.properties | 3 + .../org/apache/struts2/struts-messages.properties | 4 ++ .../multipart/AbstractMultiPartRequestTest.java | 6 ++ .../multipart/JakartaMultiPartRequestTest.java | 68 ++++++++++++++++++++ 8 files changed, 214 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/StrutsConstants.java b/core/src/main/java/org/apache/struts2/StrutsConstants.java index e76fac4e5..72f04c859 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 cb5c6b9ec..0c5e618bd 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 abstract class AbstractMultiPartRequest implements MultiPartRequest { 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,10 @@ public abstract class AbstractMultiPartRequest implements MultiPartRequest { 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 && maxParameterCount != null) { + long maxParts = maxFiles + maxParameterCount; + 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 +312,40 @@ public abstract class AbstractMultiPartRequest implements MultiPartRequest { 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) { + 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) { + 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 +372,14 @@ public abstract class AbstractMultiPartRequest implements MultiPartRequest { } 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 +393,23 @@ public abstract class AbstractMultiPartRequest implements MultiPartRequest { } } + /** + * 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<UploadedFile> 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 000000000..f6f87b996 --- /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 6b19962eb..3ad31f6a5 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,6 +115,8 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { RequestContext requestContext = createRequestContext(request); + 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 @@ -123,10 +125,18 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { 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++; + } processFileField(item, saveDir); } } diff --git a/core/src/main/resources/org/apache/struts2/default.properties b/core/src/main/resources/org/apache/struts2/default.properties index 1c501cb33..f742c0798 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 2e2eb30f9..63514b000 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 6a2450201..e2abbd6bc 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 @@ abstract class AbstractMultiPartRequestTest { .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 15b59f5dd..641388ff7 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 class JakartaMultiPartRequestTest extends AbstractMultiPartRequestTest { 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 =
