Skip to content

[FILEUPLOAD-360] FileUploadBase.getItemIterator(...) does not respect fileCountMax #483

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
sharanggupta wants to merge 1 commit into
base: 1.x
Choose a base branch
from
+160 −10
Open

[FILEUPLOAD-360] FileUploadBase.getItemIterator(...) does not respect fileCountMax #483

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/changes/changes.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ The <action> type attribute can be add,update,fix,remove.
<action type="fix" dev="ggregory" due-to="Chenjp, Gary Gregory">RFC2231Utils decodeText - Incorrect handling of percent encoded text (#472).</action>
<action type="fix" dev="ggregory" due-to="alhuda, Gary Gregory">Reject truncated percent-encoding in RFC2231Utility.decodeText (#477).</action>
<action type="fix" dev="ggregory" due-to="alhuda, Gary Gregory">Ignore invalid RFC 2231 extended value so it cannot erase a valid filename (1.x) (#480).</action>
<action issue="FILEUPLOAD-360" type="fix" dev="ggregory" due-to="Stephan Markwalder, Sharang Gupta">FileUploadBase.getItemIterator(...) does not respect fileCountMax.</action>
<!-- ADD -->
<!-- UPDATE -->
<action type="update" dev="ggregory" due-to="Gary Gregory">Bump org.apache.commons:commons-parent from 84 to 105.</action>
Expand Down
41 changes: 31 additions & 10 deletions src/main/java/org/apache/commons/fileupload/FileUploadBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*/
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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) {
Expand All @@ -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;
}
}
Expand All @@ -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.
Expand Down
128 changes: 128 additions & 0 deletions src/test/java/org/apache/commons/fileupload/FileUploadTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -61,8 +64,77 @@ public static Stream<FileUpload> 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: <part").append(i).append("@example.org>\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]);
Expand Down Expand Up @@ -138,6 +210,62 @@ public void testEmptyFile(FileUpload upload)
}
}

/**
* Test for <a href="https://issues.apache.org/jira/browse/FILEUPLOAD-360">FILEUPLOAD-360</a>: 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 <a href="https://issues.apache.org/jira/browse/FILEUPLOAD-360">FILEUPLOAD-360</a>: 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 <a href="https://issues.apache.org/jira/browse/FILEUPLOAD-360">FILEUPLOAD-360</a>: 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 <a href="https://issues.apache.org/jira/browse/FILEUPLOAD-360">FILEUPLOAD-360</a>: 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)
Expand Down