mirror of
https://github.com/apache/struts.git
synced 2026-08-05 14:47:09 +00:00
WW-5474 Count files only for maxFiles, add maxParameterCount (#1806)
* WW-5474 docs(multipart): design for files-only maxFiles + maxParameterCount Spec for correcting struts.multipart.maxFiles to count file parts only (consistently across the jakarta and jakarta-stream parsers) and adding struts.multipart.maxParameterCount to cap non-file form fields, restoring the DoS guard the old accidental total-part cap provided. Fail-closed on breach. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5474 docs(multipart): implementation plan for maxFiles/maxParameterCount Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * WW-5474 fix(multipart): honor -1 unlimited sentinel in total-parts backstop prepareServletFileUpload applied the total-parts backstop whenever both maxFiles and maxParameterCount were non-null, without checking for the -1 "unlimited" sentinel already honored by enforceMaxFiles/enforceMaxParameterCount. With maxFiles=-1 and maxParameterCount=256, maxParts computed to 255 and was passed to commons-fileupload2's setMaxFileCount (which counts ALL parts), wrongly rejecting large file-only uploads. Only apply the backstop when both limits are finite (non-null and >= 0). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * WW-5474 fix(multipart): track all parsed items to avoid temp-file leak on fail-closed breach servletFileUpload.parseRequest() fully materializes every part - spilling large ones to disk - before processUpload() iterates over the result. The loop only added each DiskFileItem to diskFileItems as it was reached, so when enforceMaxFiles/enforceMaxParameterCount threw mid-loop on a breach, every item positioned after the breaching one was never registered for cleanup. With no FileCleaningTracker on the factory, cleanUp() had no way to reclaim those temp files, leaking disk space on the hardening path. Materialize the parsed list once and register all items for cleanup before processing so cleanUp() reclaims every temp file regardless of where enforcement aborts. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5474 fix(multipart): guard debug logging in enforce helpers (Sonar S2629) Wrap the LOG.debug calls in enforceMaxFiles/enforceMaxParameterCount with isDebugEnabled() so normalizeSpace() is not evaluated when debug is disabled, matching the exceedsMaxStringLength pattern in the same class. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5474 fix(multipart): address Copilot review - parser parity + overflow guard - JakartaMultiPartRequest: only count/enforce a file part toward maxFiles when it has a non-null field name, matching JakartaStreamMultiPartRequest's accept criteria (defensive: commons-fileupload2 already drops parts without a name attribute before parseRequest returns, so the two parsers stay consistent regardless). - AbstractMultiPartRequest: compute the total-parts backstop with Math.addExact and clamp to Long.MAX_VALUE on overflow, so extremely large configured limits cannot wrap negative and silently disable the commons backstop. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5474 fix(multipart): log "processing a form field" only for form fields Move the debug log into the isFormField branch so file parts are not mislabelled; the file branch already logs "Processing a file". Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
Lukasz Lenart
@@ -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.
|
||||
*/
|
||||
|
||||
+83
-3
@@ -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,17 @@ 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 && 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 @@ 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) {
|
||||
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 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 +404,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.
|
||||
*
|
||||
|
||||
+45
@@ -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;
|
||||
}
|
||||
}
|
||||
+20
-7
@@ -108,18 +108,31 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest {
|
||||
|
||||
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<DiskFileItem> 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);
|
||||
}
|
||||
}
|
||||
|
||||
+10
-24
@@ -20,7 +20,6 @@ package org.apache.struts2.dispatcher.multipart;
|
||||
|
||||
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 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest {
|
||||
return;
|
||||
}
|
||||
|
||||
enforceMaxParameterCount(parameterCount, fieldName);
|
||||
parameterCount++;
|
||||
|
||||
String fieldValue = readStream(fileItemInput.getInputStream());
|
||||
if (exceedsMaxStringLength(fieldName, fieldValue)) {
|
||||
return;
|
||||
@@ -148,26 +155,6 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest {
|
||||
.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 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest {
|
||||
return;
|
||||
}
|
||||
|
||||
if (exceedsMaxFiles(fileItemInput)) {
|
||||
return;
|
||||
}
|
||||
enforceMaxFiles(fileCount, fileItemInput.getName());
|
||||
fileCount++;
|
||||
|
||||
File file = createTemporaryFile(fileItemInput.getName(), location);
|
||||
streamFileToDisk(fileItemInput, file);
|
||||
|
||||
@@ -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=
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
+6
@@ -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") +
|
||||
|
||||
+138
@@ -27,6 +27,7 @@ import java.io.IOException;
|
||||
import java.io.InputStream;
|
||||
import java.lang.reflect.Field;
|
||||
import java.nio.charset.StandardCharsets;
|
||||
import java.nio.file.Files;
|
||||
|
||||
import static org.apache.commons.lang3.StringUtils.normalizeSpace;
|
||||
import static org.assertj.core.api.Assertions.assertThat;
|
||||
@@ -246,6 +247,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 =
|
||||
@@ -277,6 +346,75 @@ public class JakartaMultiPartRequestTest extends AbstractMultiPartRequestTest {
|
||||
.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();
|
||||
}
|
||||
}
|
||||
|
||||
@Test
|
||||
public void inMemoryUploadIsNotWrittenToDiskUntilContentRequested() throws IOException {
|
||||
// given - a small file that Commons FileUpload keeps in memory
|
||||
|
||||
+56
-7
@@ -216,14 +216,63 @@ public class JakartaStreamMultiPartRequestTest extends AbstractMultiPartRequestT
|
||||
// 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
|
||||
|
||||
@@ -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 <type>(<scope>): <desc>`; end commit body with `Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>`.
|
||||
- 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=<ClassName>[#<method>]`.
|
||||
- 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<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();
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **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 <noreply@anthropic.com>
|
||||
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<String> 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 <noreply@anthropic.com>
|
||||
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).
|
||||
@@ -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`.
|
||||
Reference in New Issue
Block a user