From 590b82a4c07ee7dd28072a9ef861353c5552f075 Mon Sep 17 00:00:00 2001 From: Sharang Gupta Date: Tue, 13 Oct 2026 18:15:24 +0100 Subject: [PATCH] [FILEUPLOAD-360] FileUploadBase.getItemIterator(...) does not respect fileCountMax setFileCountMax(long) was only enforced by parseRequest(RequestContext). The streaming API (getItemIterator) accepted any number of items, so an application that relies on the limit while iterating got no protection. FileItemIteratorImpl now counts the items it hands out and throws FileCountLimitExceededException from hasNext() as soon as the next item would exceed the limit. The check covers plain form-data parts, files nested in a multipart/mixed part and multipart/related parts, and mirrors the 2.x fix (FileItemInputIteratorImpl, #481). The four copies of the "item is valid" bookkeeping in findNextItem() are folded into one itemValid(FileItemHeaders) method, which also counts. --- src/changes/changes.xml | 1 + .../commons/fileupload/FileUploadBase.java | 41 ++++-- .../commons/fileupload/FileUploadTest.java | 128 ++++++++++++++++++ 3 files changed, 160 insertions(+), 10 deletions(-) diff --git a/src/changes/changes.xml b/src/changes/changes.xml index 84ad19540..3d87c047f 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -54,6 +54,7 @@ The type attribute can be add,update,fix,remove. RFC2231Utils decodeText - Incorrect handling of percent encoded text (#472). Reject truncated percent-encoding in RFC2231Utility.decodeText (#477). Ignore invalid RFC 2231 extended value so it cannot erase a valid filename (1.x) (#480). + FileUploadBase.getItemIterator(...) does not respect fileCountMax. Bump org.apache.commons:commons-parent from 84 to 105. diff --git a/src/main/java/org/apache/commons/fileupload/FileUploadBase.java b/src/main/java/org/apache/commons/fileupload/FileUploadBase.java index e2ef3597d..c8930bf5a 100644 --- a/src/main/java/org/apache/commons/fileupload/FileUploadBase.java +++ b/src/main/java/org/apache/commons/fileupload/FileUploadBase.java @@ -270,6 +270,11 @@ public void setHeaders(final FileItemHeaders headers) { */ private boolean itemValid; + /** + * The number of items found so far. + */ + private long curFileCount; + /** * Whether we have seen the end of the file. */ @@ -346,13 +351,25 @@ protected void raiseError(final long sizeMax, final long count) throws IOExcepti findNextItem(); } + /** + * Throws if accepting another item would exceed {@link FileUploadBase#getFileCountMax()}. + * + * @throws FileCountLimitExceededException The maximum number of files was reached. + */ + private void checkFileCountMax() throws FileCountLimitExceededException { + if (curFileCount == fileCountMax) { + throw new FileCountLimitExceededException(format("Maximum file count %s exceeded.", Long.valueOf(fileCountMax)), fileCountMax); + } + } + /** * Called for finding the next item, if any. * * @return True, if an next item was found, otherwise false. + * @throws FileUploadException The maximum number of files was exceeded. * @throws IOException An I/O error occurred. */ - private boolean findNextItem() throws IOException { + private boolean findNextItem() throws FileUploadException, IOException { if (eof) { return false; } @@ -380,11 +397,10 @@ private boolean findNextItem() throws IOException { } final FileItemHeaders headers = getParsedHeaders(multi.readHeaders()); if (multipartRelated) { + checkFileCountMax(); currentFieldName = ""; currentItem = new FileItemStreamImpl(null, null, headers.getHeader(CONTENT_TYPE), false, getContentLength(headers)); - currentItem.setHeaders(headers); - notifier.noteItem(); - itemValid = true; + itemValid(headers); return true; } if (currentFieldName == null) { @@ -403,20 +419,18 @@ private boolean findNextItem() throws IOException { skipPreamble = true; continue; } + checkFileCountMax(); final String fileName = getFileName(headers); currentItem = new FileItemStreamImpl(fileName, fieldName, headers.getHeader(CONTENT_TYPE), fileName == null, getContentLength(headers)); - currentItem.setHeaders(headers); - notifier.noteItem(); - itemValid = true; + itemValid(headers); return true; } } else { final String fileName = getFileName(headers); if (fileName != null) { + checkFileCountMax(); currentItem = new FileItemStreamImpl(fileName, currentFieldName, headers.getHeader(CONTENT_TYPE), false, getContentLength(headers)); - currentItem.setHeaders(headers); - notifier.noteItem(); - itemValid = true; + itemValid(headers); return true; } } @@ -432,6 +446,13 @@ private long getContentLength(final FileItemHeaders headers) { } } + private void itemValid(final FileItemHeaders headers) { + currentItem.setHeaders(headers); + notifier.noteItem(); + itemValid = true; + curFileCount++; + } + /** * Returns, whether another instance of {@link FileItemStream} * is available. diff --git a/src/test/java/org/apache/commons/fileupload/FileUploadTest.java b/src/test/java/org/apache/commons/fileupload/FileUploadTest.java index ae47d5a48..556c3cccd 100644 --- a/src/test/java/org/apache/commons/fileupload/FileUploadTest.java +++ b/src/test/java/org/apache/commons/fileupload/FileUploadTest.java @@ -30,12 +30,15 @@ import java.util.List; import java.util.stream.Stream; +import javax.servlet.http.HttpServletRequest; + import org.apache.commons.fileupload.MultipartStream.IllegalBoundaryException; import org.apache.commons.fileupload.disk.DiskFileItemFactory; import org.apache.commons.fileupload.portlet.PortletFileUpload; import org.apache.commons.fileupload.portlet.PortletFileUploadTest; import org.apache.commons.fileupload.servlet.ServletFileUpload; import org.apache.commons.fileupload.servlet.ServletFileUploadTest; +import org.apache.commons.fileupload.servlet.ServletRequestContext; import org.apache.commons.fileupload.util.Streams; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; @@ -61,8 +64,77 @@ public static Stream data() { new PortletFileUpload(new DiskFileItemFactory())); } + private static final String RELATED_CONTENT_TYPE = "multipart/related; boundary=---1234"; + private FileUpload upload = new ServletFileUpload(new DiskFileItemFactory()); + /** + * Reads the permitted number of items from an iterator over {@code body}, then expects the next item to be rejected. + */ + private void assertFileCountMaxEnforced(final FileUpload upload, final String body, final String contentType) throws Exception { + final long limit = upload.getFileCountMax(); + final FileItemIterator iter = newItemIterator(upload, body, contentType); + for (long i = 0; i < limit; i++) { + assertTrue(iter.hasNext()); + iter.next(); + } + final FileCountLimitExceededException e = assertThrows(FileCountLimitExceededException.class, iter::hasNext); + assertEquals(limit, e.getLimit()); + } + + private void assertFileCountMaxNotExceeded(final FileUpload upload, final String body, final String contentType) throws Exception { + final FileItemIterator iter = newItemIterator(upload, body, contentType); + for (long i = 0; i < upload.getFileCountMax(); i++) { + assertTrue(iter.hasNext()); + iter.next(); + } + assertFalse(iter.hasNext()); + } + + private static String formDataFiles(final int count) { + final StringBuilder sb = new StringBuilder(); + for (int i = 1; i <= count; i++) { + sb.append("-----1234\r\n") + .append("Content-Disposition: form-data; name=\"file").append(i).append("\"; filename=\"file").append(i).append(".txt\"\r\n") + .append("Content-Type: text/plain\r\n") + .append("\r\n") + .append("Content of file ").append(i).append("\r\n"); + } + return sb.append("-----1234--\r\n").toString(); + } + + private static String mixedFiles(final int count) { + final StringBuilder sb = new StringBuilder("-----1234\r\n") + .append("Content-Disposition: form-data; name=\"files\"\r\n") + .append("Content-Type: multipart/mixed; boundary=---9876\r\n") + .append("\r\n"); + for (int i = 1; i <= count; i++) { + sb.append("-----9876\r\n") + .append("Content-Disposition: attachment; filename=\"file").append(i).append(".txt\"\r\n") + .append("Content-Type: text/plain\r\n") + .append("\r\n") + .append("Content of file ").append(i).append("\r\n"); + } + return sb.append("-----9876--\r\n").append("-----1234--\r\n").toString(); + } + + private static FileItemIterator newItemIterator(final FileUpload upload, final String body, final String contentType) throws Exception { + final HttpServletRequest request = new MockHttpServletRequest(body.getBytes(StandardCharsets.US_ASCII), contentType); + return upload.getItemIterator(new ServletRequestContext(request)); + } + + private static String relatedParts(final int count) { + final StringBuilder sb = new StringBuilder(); + for (int i = 1; i <= count; i++) { + sb.append("-----1234\r\n") + .append("Content-Type: text/plain\r\n") + .append("Content-ID: \r\n") + .append("\r\n") + .append("Content of part ").append(i).append("\r\n"); + } + return sb.append("-----1234--\r\n").toString(); + } + private void assertHeaders(final String[] headerNames, final String[] headerValues, final FileItem item, final int index) { for (int i = 0; i < headerNames.length; i++) { final String value = item.getHeaders().getHeader(headerNames[i]); @@ -138,6 +210,62 @@ public void testEmptyFile(FileUpload upload) } } + /** + * Test for FILEUPLOAD-360: the file count limit applies to the item iterator. + */ + @ParameterizedTest + @MethodSource("data") + public void testFileCountMaxFormDataItemIterator(final FileUpload upload) throws Exception { + upload.setFileCountMax(2); + assertFileCountMaxEnforced(upload, formDataFiles(3), Constants.CONTENT_TYPE); + } + + /** + * Test for FILEUPLOAD-360: the file count limit applies to files nested in a + * {@code multipart/mixed} part. + */ + @ParameterizedTest + @MethodSource("data") + public void testFileCountMaxMixedItemIterator(final FileUpload upload) throws Exception { + upload.setFileCountMax(2); + assertFileCountMaxEnforced(upload, mixedFiles(3), Constants.CONTENT_TYPE); + } + + /** + * Test for FILEUPLOAD-360: a request that holds exactly the permitted number of files is + * still accepted by the item iterator. + */ + @ParameterizedTest + @MethodSource("data") + public void testFileCountMaxNotExceededItemIterator(final FileUpload upload) throws Exception { + upload.setFileCountMax(3); + assertFileCountMaxNotExceeded(upload, formDataFiles(3), Constants.CONTENT_TYPE); + assertFileCountMaxNotExceeded(upload, mixedFiles(3), Constants.CONTENT_TYPE); + assertFileCountMaxNotExceeded(upload, relatedParts(3), RELATED_CONTENT_TYPE); + } + + /** + * {@link FileUploadBase#parseRequest(org.apache.commons.fileupload.RequestContext)} has always enforced the file count limit. + */ + @ParameterizedTest + @MethodSource("data") + public void testFileCountMaxParseRequest(final FileUpload upload) { + upload.setFileCountMax(2); + final FileCountLimitExceededException e = assertThrows(FileCountLimitExceededException.class, () -> Util.parseUpload(upload, formDataFiles(3))); + assertEquals(2, e.getLimit()); + } + + /** + * Test for FILEUPLOAD-360: the file count limit applies to the parts of a + * {@code multipart/related} request. + */ + @ParameterizedTest + @MethodSource("data") + public void testFileCountMaxRelatedItemIterator(final FileUpload upload) throws Exception { + upload.setFileCountMax(2); + assertFileCountMaxEnforced(upload, relatedParts(3), RELATED_CONTENT_TYPE); + } + @ParameterizedTest @MethodSource("data") public void testFilenameCaseSensitivity(FileUpload upload)