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)