From 2f7e205fe6ed1192bec3a1a128e511e8c7ab90e8 Mon Sep 17 00:00:00 2001 From: Chenjp Date: Wed, 2 Jul 2025 15:58:40 +0800 Subject: [PATCH 1/6] CVE-2025-48976: Introduce partHeaderTotalCountMax and partHeaderTotalSizeMax Flexible limitation policy. Particularly, "partHeaderTotalSizeMax" provides a direct memory usage limits for all parts headers. See BZ69710#c31 --- .../fileupload2/core/AbstractFileUpload.java | 63 ++++++ .../fileupload2/core/FileItemHeaders.java | 6 + .../fileupload2/core/FileItemHeadersImpl.java | 9 +- .../core/FileItemInputIteratorImpl.java | 12 ++ .../fileupload2/core/MultipartInput.java | 13 ++ .../core/AbstractFileUploadTest.java | 186 +++++++++++++++--- 6 files changed, 260 insertions(+), 29 deletions(-) diff --git a/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/AbstractFileUpload.java b/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/AbstractFileUpload.java index eb676039bf..4d3711ff9e 100644 --- a/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/AbstractFileUpload.java +++ b/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/AbstractFileUpload.java @@ -134,6 +134,22 @@ public static final boolean isMultipartContent(final RequestContext ctx) { */ private long fileCountMax = -1; + /** + * The maximum count of the all parts headers in bytes that may be uploaded in a single request. A value of -1 indicates no maximum. + */ + private int partHeaderTotalCountMax = -1; + + + /** + * The headers count of all parts that have been read. + */ + private int totalHeaderCountRead = 0; + + /** + * The maximum size of the all parts headers in bytes that may be uploaded in a single request. A value of -1 indicates no maximum. + */ + private long partHeaderTotalSizeMax = -1; + /** * The content encoding to use when reading part headers. */ @@ -271,6 +287,32 @@ public long getFileSizeMax() { return fileSizeMax; } + /** + * Gets the maximum allowed number of all parts headers in a single uploaded request. + * + * @return Maximum number of all parts headers. + */ + public int getPartHeaderTotalCountMax() { + return partHeaderTotalCountMax; + } + + /** + * Gets the maximum allowed size of all parts headers in a single uploaded request. + * + * @return Maximum size in bytes of all parts headers. + */ + public long getPartHeaderTotalSizeMax() { + return partHeaderTotalSizeMax; + } + + + /** + * @return The headers count of all parts that have been read + */ + public int getTotalHeaderCountRead() { + return totalHeaderCountRead; + } + /** * Gets the character encoding used when reading the headers of an individual part. When not specified, or {@code null}, the request encoding is used. If * that is also not specified, or {@code null}, the platform default encoding is used. @@ -403,6 +445,7 @@ private int parseEndOfLine(final String headerPart, final int end) { * @param header Map where to store the current header. */ private void parseHeaderLine(final FileItemHeaders headers, final String header) { + totalHeaderCountRead++; final var colonOffset = header.indexOf(':'); if (colonOffset == -1) { // This header line is malformed, skip it. @@ -536,6 +579,26 @@ public void setFileSizeMax(final long fileSizeMax) { this.fileSizeMax = fileSizeMax; } + /** + * Sets the maximum allowed number of all parts headers. + * + * @see #getPartHeaderTotalCountMax() + * @param partHeaderTotalCountMax Maximum number of all parts headers. + */ + public void setPartHeaderTotalCountMax(int partHeaderTotalCountMax) { + this.partHeaderTotalCountMax = partHeaderTotalCountMax; + } + + /** + * Sets the maximum allowed size in bytes of all parts headers. + * + * @see #getPartHeaderTotalSizeMax() + * @param partHeaderTotalSizeMax Maximum size of all parts headers. + */ + public void setPartHeaderTotalSizeMax(long partHeaderTotalSizeMax) { + this.partHeaderTotalSizeMax = partHeaderTotalSizeMax; + } + /** * Specifies the character encoding to be used when reading the headers of individual part. When not specified, or {@code null}, the request encoding is * used. If that is also not specified, or {@code null}, the platform default encoding is used. diff --git a/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/FileItemHeaders.java b/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/FileItemHeaders.java index a23d169a72..1cbb397bf5 100644 --- a/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/FileItemHeaders.java +++ b/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/FileItemHeaders.java @@ -63,4 +63,10 @@ public interface FileItemHeaders { */ Iterator getHeaders(String name); + /** + * Gets the count of all the headers. + * + * @return the count of all the headers. + */ + int getHeaderCount(); } diff --git a/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/FileItemHeadersImpl.java b/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/FileItemHeadersImpl.java index eb01fbccb8..8db88d736b 100644 --- a/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/FileItemHeadersImpl.java +++ b/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/FileItemHeadersImpl.java @@ -29,6 +29,13 @@ */ class FileItemHeadersImpl implements FileItemHeaders { + private int headerCount = 0; + + @Override + public int getHeaderCount() { + return headerCount; + } + /** * Map of {@code String} keys to a {@code List} of {@code String} instances. */ @@ -42,6 +49,7 @@ class FileItemHeadersImpl implements FileItemHeaders { */ @Override public synchronized void addHeader(final String name, final String value) { + headerCount++; headerNameToValueListMap.computeIfAbsent(toLowerCase(name), k -> new ArrayList<>()).add(value); } @@ -84,5 +92,4 @@ private List getList(final String name) { private String toLowerCase(final String value) { return value.toLowerCase(Locale.ROOT); } - } diff --git a/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/FileItemInputIteratorImpl.java b/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/FileItemInputIteratorImpl.java index 3815baa82b..208c3c731c 100644 --- a/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/FileItemInputIteratorImpl.java +++ b/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/FileItemInputIteratorImpl.java @@ -133,6 +133,8 @@ private boolean findNextItem() throws FileUploadException, IOException { currentItem = null; } final var multi = getMultiPartInput(); + final var phtsm = fileUpload.getPartHeaderTotalSizeMax(); + final var phtcm = fileUpload.getPartHeaderTotalCountMax(); for (;;) { final boolean nextPart; if (skipPreamble) { @@ -152,6 +154,16 @@ private boolean findNextItem() throws FileUploadException, IOException { continue; } final var headers = fileUpload.getParsedHeaders(multi.readHeaders()); + if (phtsm != -1 && multi.getTotalHeaderSizeRead() > phtsm) { + throw new FileUploadSizeException(String.format( + "Total size of all prcessed part headers exceeds %s bytes", + Long.valueOf(phtsm)), phtsm, multi.getTotalHeaderSizeRead()); + } + if (phtcm != -1 && fileUpload.getTotalHeaderCountRead() > phtcm) { + throw new FileUploadSizeException(String.format( + "Total count of all prcessed part headers exceeds %s", + Long.valueOf(phtcm)), phtcm, fileUpload.getTotalHeaderCountRead()); + } if (multipartRelated) { currentFieldName = ""; currentItem = new FileItemInputImpl( diff --git a/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/MultipartInput.java b/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/MultipartInput.java index 18ff991bd1..713cdfb860 100644 --- a/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/MultipartInput.java +++ b/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/MultipartInput.java @@ -908,6 +908,18 @@ public byte readByte() throws IOException { return buffer[head++]; } + /** + * The byte size of all headers that have been read. + */ + private long totalHeaderSizeRead = 0L; + + /** + * @return The byte size of all headers that have been read + */ + public long getTotalHeaderSizeRead() { + return totalHeaderSizeRead; + } + /** * Reads the {@code header-part} of the current {@code encapsulation}. *

@@ -949,6 +961,7 @@ public String readHeaders() throws FileUploadSizeException, MalformedStreamExcep baos.write(b); } try { + totalHeaderSizeRead += baos.size(); return baos.toString(Charsets.toCharset(headerCharset, Charset.defaultCharset()).name()); } catch (final UnsupportedEncodingException e) { // not possible diff --git a/commons-fileupload2-core/src/test/java/org/apache/commons/fileupload2/core/AbstractFileUploadTest.java b/commons-fileupload2-core/src/test/java/org/apache/commons/fileupload2/core/AbstractFileUploadTest.java index 14c55defc7..2b4c3dba83 100644 --- a/commons-fileupload2-core/src/test/java/org/apache/commons/fileupload2/core/AbstractFileUploadTest.java +++ b/commons-fileupload2-core/src/test/java/org/apache/commons/fileupload2/core/AbstractFileUploadTest.java @@ -20,6 +20,7 @@ import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assertions.fail; import java.io.IOException; import java.nio.charset.StandardCharsets; @@ -27,22 +28,23 @@ import org.junit.jupiter.api.Test; /** - * Common tests for implementations of {@link AbstractFileUpload}. This is a parameterized test. Tests must be valid and common to all implementations of - * FileUpload added as parameter in this class. + * Common tests for implementations of {@link AbstractFileUpload}. This is a parameterized test. Tests must be valid and + * common to all implementations of FileUpload added as parameter in this class. * * @param The {@link AbstractFileUpload} type. * @param The FileUpload request type. * @param The FileItem type. * @param The FileItemFactory type. */ -public abstract class AbstractFileUploadTest, R, I extends FileItem, F extends FileItemFactory> - extends AbstractFileUploadWrapper { +public abstract class AbstractFileUploadTest, R, I extends FileItem, F extends FileItemFactory> + extends AbstractFileUploadWrapper { protected AbstractFileUploadTest(final AFU fileUpload) { super(fileUpload); } - private void assertHeaders(final String[] headerNames, final String[] headerValues, final I fileItems, final int index) { + private void assertHeaders(final String[] headerNames, final String[] headerValues, final I fileItems, + final int index) { for (var i = 0; i < headerNames.length; i++) { final var value = fileItems.getHeaders().getHeader(headerNames[i]); if (i == index) { @@ -343,8 +345,8 @@ void testFoldedHeaders() throws IOException { } /** - * Internet Explorer 5 for the Mac has a bug where the carriage return is missing on any boundary line immediately preceding an input with type=image. - * (type=submit does not have the bug.) + * Internet Explorer 5 for the Mac has a bug where the carriage return is missing on any boundary line immediately + * preceding an input with type=image. (type=submit does not have the bug.) * * @throws FileUploadException Test failure. */ @@ -395,34 +397,22 @@ void testIE5MacBug() throws IOException { } /** - * Test for multipart/related without any content-disposition Header. - * This kind of Content-Type is commonly used by SOAP-Requests with Attachments (MTOM) + * Test for multipart/related without any content-disposition Header. This kind of Content-Type is commonly used by + * SOAP-Requests with Attachments (MTOM) */ @Test void testMultipleRelated() throws Exception { - final String soapEnvelope = - "\r\n" + - " \r\n" + - " \r\n" + + final String soapEnvelope = "\r\n" + + " \r\n" + " \r\n" + " \r\n" + " \r\n" + " \r\n" + - " \r\n" + - " \r\n" + - " \r\n" + - ""; + " href=\"ref-to-attachment%40some.domain.org\"/>\r\n" + " \r\n" + + " \r\n" + " \r\n" + ""; - final String text = - "-----1234\r\n" + - "content-type: application/xop+xml; type=\"application/soap+xml\"\r\n" + - "\r\n" + - soapEnvelope + "\r\n" + - "-----1234\r\n" + - "Content-type: text/plain\r\n" + - "content-id: \r\n" + - "\r\n" + - "some text/plain content\r\n" + + final String text = "-----1234\r\n" + "content-type: application/xop+xml; type=\"application/soap+xml\"\r\n" + + "\r\n" + soapEnvelope + "\r\n" + "-----1234\r\n" + "Content-type: text/plain\r\n" + + "content-id: \r\n" + "\r\n" + "some text/plain content\r\n" + "-----1234--\r\n"; final var bytes = text.getBytes(StandardCharsets.US_ASCII); @@ -442,4 +432,144 @@ void testMultipleRelated() throws Exception { assertEquals("text/plain", part2.getContentType()); assertNull(part2.getName()); } + + @Test + public void testExceedTotalPartHeaderCountLimit() throws IOException { + upload.setPartHeaderTotalCountMax(6); + try { + // @formatter:off + final var fileItems = parseUpload(upload, + "-----1234\r\n" + + "Content-Disposition: " + + "form-data; name=\"file\"; filename=\"foo.tab\"\r\n" + + "Content-Type: text/whatever\r\n" + + "\r\n" + + "This is the content of the file\n" + + "\r\n" + + "-----1234\r\n" + + "Content-Disposition: form-data; name=\"field\"\r\n" + + "\r\n" + + "fieldValue\r\n" + + "-----1234\r\n" + + "Content-Disposition: form-data; name=\"multi\"\r\n" + + "\r\n" + + "value1\r\n" + + "-----1234\r\n" + + "Content-Disposition: form-data; name=\"multi\"\r\n" + + "Content-Type: text/plain\r\n" + + "Content-ID: multi-id\r\n" + + "\r\n" + + "value2\r\n" + + "-----1234--\r\n"); + // @formatter:on + fail("FileUploadSizeException expected!"); + } catch (FileUploadSizeException fse) { + assertEquals(6, fse.getPermitted()); + } + } + + @Test + public void testPassTotalPartHeaderCountLimit() throws IOException { + upload.setPartHeaderTotalCountMax(7); + try { + // @formatter:off + final var fileItems = parseUpload(upload, + "-----1234\r\n" + + "Content-Disposition: " + + "form-data; name=\"file\"; filename=\"foo.tab\"\r\n" + + "Content-Type: text/whatever\r\n" + + "\r\n" + + "This is the content of the file\n" + + "\r\n" + + "-----1234\r\n" + + "Content-Disposition: form-data; name=\"field\"\r\n" + + "\r\n" + + "fieldValue\r\n" + + "-----1234\r\n" + + "Content-Disposition: form-data; name=\"multi\"\r\n" + + "\r\n" + + "value1\r\n" + + "-----1234\r\n" + + "Content-Disposition: form-data; name=\"multi\"\r\n" + + "Content-Type: text/plain\r\n" + + "Content-ID: multi-id\r\n" + + "\r\n" + + "value2\r\n" + + "-----1234--\r\n"); + // @formatter:on + assertEquals(4, fileItems.size()); + } catch (FileUploadSizeException fse) { + fail(fse); + } + } + + @Test + public void testExceedTotalPartHeaderSizeLimit() throws IOException { + upload.setPartHeaderTotalSizeMax(120); + try { + // @formatter:off + final var fileItems = parseUpload(upload, + "-----1234\r\n" + + "Content-Disposition: " + + "form-data; name=\"file\"; filename=\"foo.tab\"\r\n" + + "Content-Type: text/whatever\r\n" + + "\r\n" + + "This is the content of the file\n" + + "\r\n" + + "-----1234\r\n" + + "Content-Disposition: form-data; name=\"field\"\r\n" + + "\r\n" + + "fieldValue\r\n" + + "-----1234\r\n" + + "Content-Disposition: form-data; name=\"multi\"\r\n" + + "\r\n" + + "value1\r\n" + + "-----1234\r\n" + + "Content-Disposition: form-data; name=\"multi\"\r\n" + + "Content-Type: text/plain\r\n" + + "Content-ID: multi-id\r\n" + + "\r\n" + + "value2\r\n" + + "-----1234--\r\n"); + // @formatter:on + fail("FileUploadSizeException expected!"); + } catch (FileUploadSizeException fse) { + assertEquals(120, fse.getPermitted()); + } + } + + @Test + public void testPassTotalPartHeaderSizeLimit() throws IOException { + upload.setPartHeaderTotalSizeMax(1 << 10); + try { + // @formatter:off + final var fileItems = parseUpload(upload, + "-----1234\r\n" + + "Content-Disposition: " + + "form-data; name=\"file\"; filename=\"foo.tab\"\r\n" + + "Content-Type: text/whatever\r\n" + + "\r\n" + + "This is the content of the file\n" + + "\r\n" + + "-----1234\r\n" + + "Content-Disposition: form-data; name=\"field\"\r\n" + + "\r\n" + + "fieldValue\r\n" + + "-----1234\r\n" + + "Content-Disposition: form-data; name=\"multi\"\r\n" + + "\r\n" + + "value1\r\n" + + "-----1234\r\n" + + "Content-Disposition: form-data; name=\"multi\"\r\n" + + "Content-Type: text/plain\r\n" + + "Content-ID: multi-id\r\n" + + "\r\n" + + "value2\r\n" + + "-----1234--\r\n"); + // @formatter:on + assertEquals(4, fileItems.size()); + } catch (FileUploadSizeException fse) { + fail(fse); + } + } } From 4173d7b4c588cfd3ad93bc123e908bc11b9401e6 Mon Sep 17 00:00:00 2001 From: Chenjp Date: Thu, 3 Jul 2025 09:06:27 +0800 Subject: [PATCH 2/6] Revoke partHeaderTotalCountMax per Mark's comment, partHeaderTotalCountMax is sufficient enough. --- .../fileupload2/core/AbstractFileUpload.java | 39 ----------- .../core/FileItemInputIteratorImpl.java | 8 +-- .../core/AbstractFileUploadTest.java | 70 ------------------- 3 files changed, 1 insertion(+), 116 deletions(-) diff --git a/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/AbstractFileUpload.java b/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/AbstractFileUpload.java index 4d3711ff9e..905f8637f5 100644 --- a/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/AbstractFileUpload.java +++ b/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/AbstractFileUpload.java @@ -134,17 +134,6 @@ public static final boolean isMultipartContent(final RequestContext ctx) { */ private long fileCountMax = -1; - /** - * The maximum count of the all parts headers in bytes that may be uploaded in a single request. A value of -1 indicates no maximum. - */ - private int partHeaderTotalCountMax = -1; - - - /** - * The headers count of all parts that have been read. - */ - private int totalHeaderCountRead = 0; - /** * The maximum size of the all parts headers in bytes that may be uploaded in a single request. A value of -1 indicates no maximum. */ @@ -287,15 +276,6 @@ public long getFileSizeMax() { return fileSizeMax; } - /** - * Gets the maximum allowed number of all parts headers in a single uploaded request. - * - * @return Maximum number of all parts headers. - */ - public int getPartHeaderTotalCountMax() { - return partHeaderTotalCountMax; - } - /** * Gets the maximum allowed size of all parts headers in a single uploaded request. * @@ -305,14 +285,6 @@ public long getPartHeaderTotalSizeMax() { return partHeaderTotalSizeMax; } - - /** - * @return The headers count of all parts that have been read - */ - public int getTotalHeaderCountRead() { - return totalHeaderCountRead; - } - /** * Gets the character encoding used when reading the headers of an individual part. When not specified, or {@code null}, the request encoding is used. If * that is also not specified, or {@code null}, the platform default encoding is used. @@ -445,7 +417,6 @@ private int parseEndOfLine(final String headerPart, final int end) { * @param header Map where to store the current header. */ private void parseHeaderLine(final FileItemHeaders headers, final String header) { - totalHeaderCountRead++; final var colonOffset = header.indexOf(':'); if (colonOffset == -1) { // This header line is malformed, skip it. @@ -579,16 +550,6 @@ public void setFileSizeMax(final long fileSizeMax) { this.fileSizeMax = fileSizeMax; } - /** - * Sets the maximum allowed number of all parts headers. - * - * @see #getPartHeaderTotalCountMax() - * @param partHeaderTotalCountMax Maximum number of all parts headers. - */ - public void setPartHeaderTotalCountMax(int partHeaderTotalCountMax) { - this.partHeaderTotalCountMax = partHeaderTotalCountMax; - } - /** * Sets the maximum allowed size in bytes of all parts headers. * diff --git a/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/FileItemInputIteratorImpl.java b/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/FileItemInputIteratorImpl.java index 208c3c731c..6edd201cb0 100644 --- a/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/FileItemInputIteratorImpl.java +++ b/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/FileItemInputIteratorImpl.java @@ -134,7 +134,6 @@ private boolean findNextItem() throws FileUploadException, IOException { } final var multi = getMultiPartInput(); final var phtsm = fileUpload.getPartHeaderTotalSizeMax(); - final var phtcm = fileUpload.getPartHeaderTotalCountMax(); for (;;) { final boolean nextPart; if (skipPreamble) { @@ -156,14 +155,9 @@ private boolean findNextItem() throws FileUploadException, IOException { final var headers = fileUpload.getParsedHeaders(multi.readHeaders()); if (phtsm != -1 && multi.getTotalHeaderSizeRead() > phtsm) { throw new FileUploadSizeException(String.format( - "Total size of all prcessed part headers exceeds %s bytes", + "The request was rejected because total header size of all parts exceeds the configured partHeaderTotalSizeMax (%s) bytes", Long.valueOf(phtsm)), phtsm, multi.getTotalHeaderSizeRead()); } - if (phtcm != -1 && fileUpload.getTotalHeaderCountRead() > phtcm) { - throw new FileUploadSizeException(String.format( - "Total count of all prcessed part headers exceeds %s", - Long.valueOf(phtcm)), phtcm, fileUpload.getTotalHeaderCountRead()); - } if (multipartRelated) { currentFieldName = ""; currentItem = new FileItemInputImpl( diff --git a/commons-fileupload2-core/src/test/java/org/apache/commons/fileupload2/core/AbstractFileUploadTest.java b/commons-fileupload2-core/src/test/java/org/apache/commons/fileupload2/core/AbstractFileUploadTest.java index 2b4c3dba83..a98f94d161 100644 --- a/commons-fileupload2-core/src/test/java/org/apache/commons/fileupload2/core/AbstractFileUploadTest.java +++ b/commons-fileupload2-core/src/test/java/org/apache/commons/fileupload2/core/AbstractFileUploadTest.java @@ -433,76 +433,6 @@ void testMultipleRelated() throws Exception { assertNull(part2.getName()); } - @Test - public void testExceedTotalPartHeaderCountLimit() throws IOException { - upload.setPartHeaderTotalCountMax(6); - try { - // @formatter:off - final var fileItems = parseUpload(upload, - "-----1234\r\n" + - "Content-Disposition: " - + "form-data; name=\"file\"; filename=\"foo.tab\"\r\n" + - "Content-Type: text/whatever\r\n" + - "\r\n" + - "This is the content of the file\n" + - "\r\n" + - "-----1234\r\n" + - "Content-Disposition: form-data; name=\"field\"\r\n" + - "\r\n" + - "fieldValue\r\n" + - "-----1234\r\n" + - "Content-Disposition: form-data; name=\"multi\"\r\n" + - "\r\n" + - "value1\r\n" + - "-----1234\r\n" + - "Content-Disposition: form-data; name=\"multi\"\r\n" + - "Content-Type: text/plain\r\n" + - "Content-ID: multi-id\r\n" + - "\r\n" + - "value2\r\n" + - "-----1234--\r\n"); - // @formatter:on - fail("FileUploadSizeException expected!"); - } catch (FileUploadSizeException fse) { - assertEquals(6, fse.getPermitted()); - } - } - - @Test - public void testPassTotalPartHeaderCountLimit() throws IOException { - upload.setPartHeaderTotalCountMax(7); - try { - // @formatter:off - final var fileItems = parseUpload(upload, - "-----1234\r\n" + - "Content-Disposition: " - + "form-data; name=\"file\"; filename=\"foo.tab\"\r\n" + - "Content-Type: text/whatever\r\n" + - "\r\n" + - "This is the content of the file\n" + - "\r\n" + - "-----1234\r\n" + - "Content-Disposition: form-data; name=\"field\"\r\n" + - "\r\n" + - "fieldValue\r\n" + - "-----1234\r\n" + - "Content-Disposition: form-data; name=\"multi\"\r\n" + - "\r\n" + - "value1\r\n" + - "-----1234\r\n" + - "Content-Disposition: form-data; name=\"multi\"\r\n" + - "Content-Type: text/plain\r\n" + - "Content-ID: multi-id\r\n" + - "\r\n" + - "value2\r\n" + - "-----1234--\r\n"); - // @formatter:on - assertEquals(4, fileItems.size()); - } catch (FileUploadSizeException fse) { - fail(fse); - } - } - @Test public void testExceedTotalPartHeaderSizeLimit() throws IOException { upload.setPartHeaderTotalSizeMax(120); From 76529257e4658dd515aac0ed43d272beb8d77ed7 Mon Sep 17 00:00:00 2001 From: Chenjp Date: Thu, 3 Jul 2025 09:19:39 +0800 Subject: [PATCH 3/6] unchanged code / unsed --- .../fileupload2/core/FileItemHeaders.java | 6 --- .../fileupload2/core/FileItemHeadersImpl.java | 8 --- .../core/AbstractFileUploadTest.java | 50 ++++++++++++------- 3 files changed, 31 insertions(+), 33 deletions(-) diff --git a/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/FileItemHeaders.java b/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/FileItemHeaders.java index 1cbb397bf5..a23d169a72 100644 --- a/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/FileItemHeaders.java +++ b/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/FileItemHeaders.java @@ -63,10 +63,4 @@ public interface FileItemHeaders { */ Iterator getHeaders(String name); - /** - * Gets the count of all the headers. - * - * @return the count of all the headers. - */ - int getHeaderCount(); } diff --git a/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/FileItemHeadersImpl.java b/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/FileItemHeadersImpl.java index 8db88d736b..ed482ba009 100644 --- a/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/FileItemHeadersImpl.java +++ b/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/FileItemHeadersImpl.java @@ -29,13 +29,6 @@ */ class FileItemHeadersImpl implements FileItemHeaders { - private int headerCount = 0; - - @Override - public int getHeaderCount() { - return headerCount; - } - /** * Map of {@code String} keys to a {@code List} of {@code String} instances. */ @@ -49,7 +42,6 @@ public int getHeaderCount() { */ @Override public synchronized void addHeader(final String name, final String value) { - headerCount++; headerNameToValueListMap.computeIfAbsent(toLowerCase(name), k -> new ArrayList<>()).add(value); } diff --git a/commons-fileupload2-core/src/test/java/org/apache/commons/fileupload2/core/AbstractFileUploadTest.java b/commons-fileupload2-core/src/test/java/org/apache/commons/fileupload2/core/AbstractFileUploadTest.java index a98f94d161..aeb5c5b841 100644 --- a/commons-fileupload2-core/src/test/java/org/apache/commons/fileupload2/core/AbstractFileUploadTest.java +++ b/commons-fileupload2-core/src/test/java/org/apache/commons/fileupload2/core/AbstractFileUploadTest.java @@ -28,23 +28,22 @@ import org.junit.jupiter.api.Test; /** - * Common tests for implementations of {@link AbstractFileUpload}. This is a parameterized test. Tests must be valid and - * common to all implementations of FileUpload added as parameter in this class. + * Common tests for implementations of {@link AbstractFileUpload}. This is a parameterized test. Tests must be valid and common to all implementations of + * FileUpload added as parameter in this class. * * @param The {@link AbstractFileUpload} type. * @param The FileUpload request type. * @param The FileItem type. * @param The FileItemFactory type. */ -public abstract class AbstractFileUploadTest, R, I extends FileItem, F extends FileItemFactory> - extends AbstractFileUploadWrapper { +public abstract class AbstractFileUploadTest, R, I extends FileItem, F extends FileItemFactory> + extends AbstractFileUploadWrapper { protected AbstractFileUploadTest(final AFU fileUpload) { super(fileUpload); } - private void assertHeaders(final String[] headerNames, final String[] headerValues, final I fileItems, - final int index) { + private void assertHeaders(final String[] headerNames, final String[] headerValues, final I fileItems, final int index) { for (var i = 0; i < headerNames.length; i++) { final var value = fileItems.getHeaders().getHeader(headerNames[i]); if (i == index) { @@ -345,8 +344,8 @@ void testFoldedHeaders() throws IOException { } /** - * Internet Explorer 5 for the Mac has a bug where the carriage return is missing on any boundary line immediately - * preceding an input with type=image. (type=submit does not have the bug.) + * Internet Explorer 5 for the Mac has a bug where the carriage return is missing on any boundary line immediately preceding an input with type=image. + * (type=submit does not have the bug.) * * @throws FileUploadException Test failure. */ @@ -397,22 +396,34 @@ void testIE5MacBug() throws IOException { } /** - * Test for multipart/related without any content-disposition Header. This kind of Content-Type is commonly used by - * SOAP-Requests with Attachments (MTOM) + * Test for multipart/related without any content-disposition Header. + * This kind of Content-Type is commonly used by SOAP-Requests with Attachments (MTOM) */ @Test void testMultipleRelated() throws Exception { - final String soapEnvelope = "\r\n" + - " \r\n" + " \r\n" + + final String soapEnvelope = + "\r\n" + + " \r\n" + + " \r\n" + " \r\n" + " \r\n" + " \r\n" + " \r\n" + - " \r\n" + " \r\n" + ""; + " href=\"ref-to-attachment%40some.domain.org\"/>\r\n" + + " \r\n" + + " \r\n" + + " \r\n" + + ""; - final String text = "-----1234\r\n" + "content-type: application/xop+xml; type=\"application/soap+xml\"\r\n" + - "\r\n" + soapEnvelope + "\r\n" + "-----1234\r\n" + "Content-type: text/plain\r\n" + - "content-id: \r\n" + "\r\n" + "some text/plain content\r\n" + + final String text = + "-----1234\r\n" + + "content-type: application/xop+xml; type=\"application/soap+xml\"\r\n" + + "\r\n" + + soapEnvelope + "\r\n" + + "-----1234\r\n" + + "Content-type: text/plain\r\n" + + "content-id: \r\n" + + "\r\n" + + "some text/plain content\r\n" + "-----1234--\r\n"; final var bytes = text.getBytes(StandardCharsets.US_ASCII); @@ -433,9 +444,10 @@ void testMultipleRelated() throws Exception { assertNull(part2.getName()); } + @Test public void testExceedTotalPartHeaderSizeLimit() throws IOException { - upload.setPartHeaderTotalSizeMax(120); + upload.setPartHeaderTotalSizeMax(250); try { // @formatter:off final var fileItems = parseUpload(upload, @@ -464,7 +476,7 @@ public void testExceedTotalPartHeaderSizeLimit() throws IOException { // @formatter:on fail("FileUploadSizeException expected!"); } catch (FileUploadSizeException fse) { - assertEquals(120, fse.getPermitted()); + assertEquals(upload.getPartHeaderTotalSizeMax(), fse.getPermitted()); } } From c8d07e195671806c33f34d5605f196bc46fee0e4 Mon Sep 17 00:00:00 2001 From: Chenjp Date: Thu, 3 Jul 2025 09:21:30 +0800 Subject: [PATCH 4/6] Update FileItemHeadersImpl.java --- .../org/apache/commons/fileupload2/core/FileItemHeadersImpl.java | 1 + 1 file changed, 1 insertion(+) diff --git a/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/FileItemHeadersImpl.java b/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/FileItemHeadersImpl.java index ed482ba009..eb01fbccb8 100644 --- a/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/FileItemHeadersImpl.java +++ b/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/FileItemHeadersImpl.java @@ -84,4 +84,5 @@ private List getList(final String name) { private String toLowerCase(final String value) { return value.toLowerCase(Locale.ROOT); } + } From 799945c673602327f9c5c1aafcec19b85de721da Mon Sep 17 00:00:00 2001 From: Chenjp Date: Thu, 3 Jul 2025 15:26:10 +0800 Subject: [PATCH 5/6] Code smell --- .../fileupload2/core/MultipartInput.java | 8 ++- .../core/AbstractFileUploadTest.java | 56 +++++++++---------- 2 files changed, 31 insertions(+), 33 deletions(-) diff --git a/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/MultipartInput.java b/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/MultipartInput.java index 713cdfb860..c35c0ed2bf 100644 --- a/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/MultipartInput.java +++ b/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/MultipartInput.java @@ -909,12 +909,14 @@ public byte readByte() throws IOException { } /** - * The byte size of all headers that have been read. + * The byte size of headers for all parts that have been read. */ - private long totalHeaderSizeRead = 0L; + private long totalHeaderSizeRead; /** - * @return The byte size of all headers that have been read + * Returns the byte size of headers for all parts that have been read. + * + * @return The byte size of headers for all parts that have been read */ public long getTotalHeaderSizeRead() { return totalHeaderSizeRead; diff --git a/commons-fileupload2-core/src/test/java/org/apache/commons/fileupload2/core/AbstractFileUploadTest.java b/commons-fileupload2-core/src/test/java/org/apache/commons/fileupload2/core/AbstractFileUploadTest.java index aeb5c5b841..62492f5a73 100644 --- a/commons-fileupload2-core/src/test/java/org/apache/commons/fileupload2/core/AbstractFileUploadTest.java +++ b/commons-fileupload2-core/src/test/java/org/apache/commons/fileupload2/core/AbstractFileUploadTest.java @@ -483,35 +483,31 @@ public void testExceedTotalPartHeaderSizeLimit() throws IOException { @Test public void testPassTotalPartHeaderSizeLimit() throws IOException { upload.setPartHeaderTotalSizeMax(1 << 10); - try { - // @formatter:off - final var fileItems = parseUpload(upload, - "-----1234\r\n" + - "Content-Disposition: " - + "form-data; name=\"file\"; filename=\"foo.tab\"\r\n" + - "Content-Type: text/whatever\r\n" + - "\r\n" + - "This is the content of the file\n" + - "\r\n" + - "-----1234\r\n" + - "Content-Disposition: form-data; name=\"field\"\r\n" + - "\r\n" + - "fieldValue\r\n" + - "-----1234\r\n" + - "Content-Disposition: form-data; name=\"multi\"\r\n" + - "\r\n" + - "value1\r\n" + - "-----1234\r\n" + - "Content-Disposition: form-data; name=\"multi\"\r\n" + - "Content-Type: text/plain\r\n" + - "Content-ID: multi-id\r\n" + - "\r\n" + - "value2\r\n" + - "-----1234--\r\n"); - // @formatter:on - assertEquals(4, fileItems.size()); - } catch (FileUploadSizeException fse) { - fail(fse); - } + // @formatter:off + final var fileItems = parseUpload(upload, + "-----1234\r\n" + + "Content-Disposition: " + + "form-data; name=\"file\"; filename=\"foo.tab\"\r\n" + + "Content-Type: text/whatever\r\n" + + "\r\n" + + "This is the content of the file\n" + + "\r\n" + + "-----1234\r\n" + + "Content-Disposition: form-data; name=\"field\"\r\n" + + "\r\n" + + "fieldValue\r\n" + + "-----1234\r\n" + + "Content-Disposition: form-data; name=\"multi\"\r\n" + + "\r\n" + + "value1\r\n" + + "-----1234\r\n" + + "Content-Disposition: form-data; name=\"multi\"\r\n" + + "Content-Type: text/plain\r\n" + + "Content-ID: multi-id\r\n" + + "\r\n" + + "value2\r\n" + + "-----1234--\r\n"); + // @formatter:on + assertEquals(4, fileItems.size()); } } From 576181aa882ee4ee63a331c33377ef106cb2248c Mon Sep 17 00:00:00 2001 From: Chenjp Date: Sun, 28 Sep 2025 16:30:35 +0800 Subject: [PATCH 6/6] Merge, and move total header size back to fileupload move total header size back to fileupload --- .../fileupload2/core/AbstractFileUpload.java | 23 +++++++++++++++++++ .../core/FileItemInputIteratorImpl.java | 6 ----- .../fileupload2/core/MultipartInput.java | 15 ------------ .../core/AbstractFileUploadTest.java | 5 ++-- 4 files changed, 26 insertions(+), 23 deletions(-) diff --git a/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/AbstractFileUpload.java b/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/AbstractFileUpload.java index 91797646c6..6178889b42 100644 --- a/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/AbstractFileUpload.java +++ b/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/AbstractFileUpload.java @@ -144,6 +144,11 @@ public static final boolean isMultipartContent(final RequestContext ctx) { */ private long partHeaderTotalSizeMax = -1; + /** + * The size of all headers of all parts that have been read. + */ + private long partHeaderTotalReads; + /** * The content encoding to use when reading part headers. */ @@ -347,6 +352,7 @@ public long getMaxSize() { */ public FileItemHeaders getParsedHeaders(final String headerPart) { final var len = headerPart.length(); + partHeaderTotalReads += len; final var headers = newFileItemHeaders(); var start = 0; for (;;) { @@ -493,6 +499,12 @@ public List parseRequest(final RequestContext requestContext) throws FileUplo String.format("Request '%s' failed: Maximum file count %,d exceeded.", MULTIPART_FORM_DATA, Long.valueOf(maxFileCount)), getMaxFileCount(), size); } + if (partHeaderTotalSizeMax > -1 && partHeaderTotalReads >= partHeaderTotalSizeMax) { + // The next item will exceed total headers size of all parts + throw new FileUploadSizeException( + "The request was rejected because total header size of all parts exceeds the configured partHeaderTotalSizeMax (%s) bytes", + partHeaderTotalSizeMax, partHeaderTotalReads); + } // Don't use getName() here to prevent an InvalidFileNameException. // @formatter:off final var fileItem = fileItemFactory.fileItemBuilder() @@ -580,6 +592,17 @@ public void setMaxPartHeaderSize(final int partHeaderSizeMax) { this.maxPartHeaderSize = partHeaderSizeMax; } + /** + * Sets the total size limit for the all parts headers + * + * @param partHeaderTotalSizeMax The maximum size of the all parts headers in + * bytes that may be uploaded in a single request. + * + */ + public void setPartHeaderTotalSizeMax(long partHeaderTotalSizeMax) { + this.partHeaderTotalSizeMax = partHeaderTotalSizeMax; + } + /** * Sets the maximum allowed size of a complete request, as opposed to {@link #setMaxFileSize(long)}. * diff --git a/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/FileItemInputIteratorImpl.java b/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/FileItemInputIteratorImpl.java index fa6115066c..157a10ebf4 100644 --- a/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/FileItemInputIteratorImpl.java +++ b/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/FileItemInputIteratorImpl.java @@ -134,7 +134,6 @@ private boolean findNextItem() throws FileUploadException, IOException { currentItem = null; } final var multi = getMultiPartInput(); - final var phtsm = fileUpload.getPartHeaderTotalSizeMax(); for (;;) { final boolean nextPart; if (skipPreamble) { @@ -154,11 +153,6 @@ private boolean findNextItem() throws FileUploadException, IOException { continue; } final var headers = fileUpload.getParsedHeaders(multi.readHeaders()); - if (phtsm != -1 && multi.getTotalHeaderSizeRead() > phtsm) { - throw new FileUploadSizeException(String.format( - "The request was rejected because total header size of all parts exceeds the configured partHeaderTotalSizeMax (%s) bytes", - Long.valueOf(phtsm)), phtsm, multi.getTotalHeaderSizeRead()); - } if (multipartRelated) { currentFieldName = ""; currentItem = new FileItemInputImpl( diff --git a/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/MultipartInput.java b/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/MultipartInput.java index 31342bc1e9..7e900f583d 100644 --- a/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/MultipartInput.java +++ b/commons-fileupload2-core/src/main/java/org/apache/commons/fileupload2/core/MultipartInput.java @@ -909,20 +909,6 @@ public byte readByte() throws IOException { return buffer[head++]; } - /** - * The byte size of headers for all parts that have been read. - */ - private long totalHeaderSizeRead; - - /** - * Returns the byte size of headers for all parts that have been read. - * - * @return The byte size of headers for all parts that have been read - */ - public long getTotalHeaderSizeRead() { - return totalHeaderSizeRead; - } - /** * Reads the {@code header-part} of the current {@code encapsulation}. *

@@ -964,7 +950,6 @@ public String readHeaders() throws FileUploadSizeException, MalformedStreamExcep baos.write(b); } try { - totalHeaderSizeRead += baos.size(); return baos.toString(Charsets.toCharset(headerCharset, Charset.defaultCharset()).name()); } catch (final UnsupportedEncodingException e) { // not possible diff --git a/commons-fileupload2-core/src/test/java/org/apache/commons/fileupload2/core/AbstractFileUploadTest.java b/commons-fileupload2-core/src/test/java/org/apache/commons/fileupload2/core/AbstractFileUploadTest.java index 7d3f186d12..1c8396d95b 100644 --- a/commons-fileupload2-core/src/test/java/org/apache/commons/fileupload2/core/AbstractFileUploadTest.java +++ b/commons-fileupload2-core/src/test/java/org/apache/commons/fileupload2/core/AbstractFileUploadTest.java @@ -483,7 +483,8 @@ void testMultipleRelated() throws Exception { @Test public void testExceedTotalPartHeaderSizeLimit() throws IOException { - upload.setPartHeaderTotalSizeMax(250); + long max = 250; + upload.setPartHeaderTotalSizeMax(max); try { // @formatter:off final var fileItems = parseUpload(upload, @@ -512,7 +513,7 @@ public void testExceedTotalPartHeaderSizeLimit() throws IOException { // @formatter:on fail("FileUploadSizeException expected!"); } catch (FileUploadSizeException fse) { - assertEquals(upload.getPartHeaderTotalSizeMax(), fse.getPermitted()); + assertEquals(max, fse.getPermitted()); } }