This is an automated email from the ASF dual-hosted git repository. joerghoh pushed a commit to branch SLING-13372 in repository https://gitbox.apache.org/repos/asf/sling-org-apache-sling-engine.git
commit 86699dab04423278f64c842e00555dd33e0272c1 Author: Joerg Hoh <[email protected]> AuthorDate: Thu Oct 1 17:11:16 2026 +0200 SLING-13372 enforce configured multipart limits on streamed uploads * A client could bypass all operator-configured multipart limits (request/file size, file count) simply by sending the Sling-uploadmode: stream header; RequestPartsIterator now applies the same ParameterSupport-configured limits as the buffered path * StreamedRequestPart.getSize() now returns -1 (unknown) instead of 0, so size-limit checks in downstream consumers no longer treat large parts as empty --- .../engine/impl/parameters/ParameterSupport.java | 6 +- .../impl/parameters/RequestPartsIterator.java | 42 +++++++++-- .../impl/parameters/ParameterSupportTest.java | 53 ++++++++++++++ .../impl/parameters/RequestPartsIteratorTest.java | 83 +++++++++++++++++++++- 4 files changed, 178 insertions(+), 6 deletions(-) diff --git a/src/main/java/org/apache/sling/engine/impl/parameters/ParameterSupport.java b/src/main/java/org/apache/sling/engine/impl/parameters/ParameterSupport.java index a4f0631..ca8bc1d 100644 --- a/src/main/java/org/apache/sling/engine/impl/parameters/ParameterSupport.java +++ b/src/main/java/org/apache/sling/engine/impl/parameters/ParameterSupport.java @@ -321,7 +321,11 @@ public class ParameterSupport { this.getServletRequest() .setAttribute( REQUEST_PARTS_ITERATOR_ATTRIBUTE, - new RequestPartsIterator(this.getMultiPartContext())); + new RequestPartsIterator( + this.getMultiPartContext(), + ParameterSupport.maxRequestSize, + ParameterSupport.maxFileSize, + ParameterSupport.maxFileCount)); this.log.debug( "getRequestParameterMapInternal: Iterator<javax.servlet.http.Part> available as request attribute named request-parts-iterator"); } catch (final FileUploadException | IOException e) { diff --git a/src/main/java/org/apache/sling/engine/impl/parameters/RequestPartsIterator.java b/src/main/java/org/apache/sling/engine/impl/parameters/RequestPartsIterator.java index ffad79f..132bc60 100644 --- a/src/main/java/org/apache/sling/engine/impl/parameters/RequestPartsIterator.java +++ b/src/main/java/org/apache/sling/engine/impl/parameters/RequestPartsIterator.java @@ -44,22 +44,51 @@ public class RequestPartsIterator implements Iterator<Part> { /** The CommonsFile Upload streaming API iterator */ private final FileItemIterator itemIterator; + /** The maximum number of parts allowed in the request, -1 for unlimited */ + private final long fileCountMax; + + /** The number of parts returned so far */ + private long partCount; + /** * Create and initialse the iterator using the request. The request must be fresh. Headers can have been read but the stream * must not have been parsed. - * @param servletRequest the request + * <p> + * The configured multipart limits are enforced on the streamed request just + * as they are for the buffered (non-streamed) code path: a client-selected + * upload mode must not bypass the operator-configured controls. + * + * @param context the request context + * @param sizeMax the maximum allowed size of the complete request (-1 for unlimited) + * @param fileSizeMax the maximum allowed size of a single file/part (-1 for unlimited) + * @param fileCountMax the maximum allowed number of files/parts in the request * @throws IOException when there is a problem reading the request. * @throws FileUploadException when there is a problem parsing the request. */ - public RequestPartsIterator(final RequestContext context) throws FileUploadException, IOException { + public RequestPartsIterator( + final RequestContext context, final long sizeMax, final long fileSizeMax, final long fileCountMax) + throws FileUploadException, IOException { + this.fileCountMax = fileCountMax; FileUpload upload = new FileUpload(); - upload.setFileCountMax(50); + upload.setSizeMax(sizeMax); + upload.setFileSizeMax(fileSizeMax); + upload.setFileCountMax(fileCountMax); itemIterator = upload.getItemIterator(context); } @Override public boolean hasNext() { try { + // enforce the part count limit here as well, as the streaming API of + // commons-fileupload 1.x does not check fileCountMax itself + if (fileCountMax >= 0 && partCount >= fileCountMax) { + if (itemIterator.hasNext()) { + LOG.error( + "hasNext: the request contains more than the allowed number of {} parts, further parts are not processed", + fileCountMax); + } + return false; + } return itemIterator.hasNext(); } catch (final FileUploadException | IOException e) { LOG.error("hasNext Item failed cause:" + e.getMessage(), e); @@ -70,6 +99,7 @@ public class RequestPartsIterator implements Iterator<Part> { @Override public Part next() { try { + partCount++; return new StreamedRequestPart(itemIterator.next()); } catch (final FileUploadException | IOException e) { LOG.error("next Item failed cause:" + e.getMessage(), e); @@ -111,7 +141,11 @@ public class RequestPartsIterator implements Iterator<Part> { @Override public long getSize() { - return 0; + // The part is streamed, so its size is not known in advance. Return + // -1 (unknown) instead of 0 so that consumers enforcing size limits + // via getSize() reject the part instead of accepting arbitrarily + // large parts as empty. + return -1; } @Override diff --git a/src/test/java/org/apache/sling/engine/impl/parameters/ParameterSupportTest.java b/src/test/java/org/apache/sling/engine/impl/parameters/ParameterSupportTest.java index 3a290ba..0ca47f0 100644 --- a/src/test/java/org/apache/sling/engine/impl/parameters/ParameterSupportTest.java +++ b/src/test/java/org/apache/sling/engine/impl/parameters/ParameterSupportTest.java @@ -22,16 +22,23 @@ import java.io.ByteArrayInputStream; import java.io.IOException; import java.io.UnsupportedEncodingException; import java.util.Collections; +import java.util.Iterator; import jakarta.servlet.ReadListener; import jakarta.servlet.ServletInputStream; import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.Part; import org.junit.Test; +import org.mockito.ArgumentCaptor; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; +import static org.mockito.Mockito.eq; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; /** @@ -161,6 +168,52 @@ public class ParameterSupportTest { support.getParameter("anything"); } + @Test + public void testStreamedUploadModeEnforcesConfiguredFileCountLimit() throws Exception { + // the streamed upload path used to apply a hardcoded fileCountMax of + // 50 regardless of configuration; configuring a stricter limit here + // (1) and sending two parts proves that ParameterSupport actually + // plumbs its configured limit into RequestPartsIterator instead of + // silently falling back to the old hardcoded default. + ParameterSupport.configure(-1, null, -1, -1, false, 1); + try { + final String boundary = "AaB03x"; + final String body = "--" + boundary + "\r\n" + + "Content-Disposition: form-data; name=\"file1\"; filename=\"a.txt\"\r\n" + + "Content-Type: text/plain\r\n" + + "\r\n" + + "hello\r\n" + + "--" + boundary + "\r\n" + + "Content-Disposition: form-data; name=\"file2\"; filename=\"b.txt\"\r\n" + + "Content-Type: text/plain\r\n" + + "\r\n" + + "world\r\n" + + "--" + boundary + "--\r\n"; + final HttpServletRequest request = postRequest("multipart/form-data; boundary=" + boundary, body); + when(request.getHeader(ParameterSupport.SLING_UPLOADMODE_HEADER)) + .thenReturn(ParameterSupport.STREAM_UPLOAD); + + final ParameterSupport support = ParameterSupport.getInstance(request); + support.getRequestParameterMap(); + + @SuppressWarnings("unchecked") + final ArgumentCaptor<Iterator<Part>> captor = ArgumentCaptor.forClass(Iterator.class); + verify(request).setAttribute(eq(ParameterSupport.REQUEST_PARTS_ITERATOR_ATTRIBUTE), captor.capture()); + final Iterator<Part> parts = captor.getValue(); + + assertTrue("the first part must still be reachable", parts.hasNext()); + parts.next(); + assertFalse( + "the configured file count limit of 1 must be enforced on the " + + "streamed path, not the previous hardcoded default of 50", + parts.hasNext()); + } finally { + // restore defaults so this test does not leak static state into + // the other tests in this class + ParameterSupport.configure(-1, null, -1, -1, false, 50); + } + } + private static HttpServletRequest postRequest(final String contentType, final String body) throws IOException { final HttpServletRequest request = mock(HttpServletRequest.class); when(request.getMethod()).thenReturn("POST"); diff --git a/src/test/java/org/apache/sling/engine/impl/parameters/RequestPartsIteratorTest.java b/src/test/java/org/apache/sling/engine/impl/parameters/RequestPartsIteratorTest.java index 8a05730..2fb9de0 100644 --- a/src/test/java/org/apache/sling/engine/impl/parameters/RequestPartsIteratorTest.java +++ b/src/test/java/org/apache/sling/engine/impl/parameters/RequestPartsIteratorTest.java @@ -22,12 +22,17 @@ import java.io.ByteArrayInputStream; import java.io.IOException; import java.lang.reflect.Field; +import jakarta.servlet.http.Part; import org.apache.commons.fileupload.FileItemIterator; import org.apache.commons.fileupload.FileUploadException; import org.apache.commons.fileupload.RequestContext; import org.junit.Test; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -35,9 +40,29 @@ import static org.mockito.Mockito.when; * Regression tests for SLING-13364: a stream turning malformed * mid-body must surface as an error from {@link RequestPartsIterator} * instead of silently truncating the part sequence. + * <p> + * Also covers the fix for the streamed upload mode bypassing the configured + * multipart limits: the configured {@code sizeMax}/{@code fileSizeMax}/ + * {@code fileCountMax} must be enforced on the streamed path exactly as they + * are on the buffered path, and {@link Part#getSize()} must report the size + * as unknown ({@code -1}) rather than falsely claiming an empty part. */ public class RequestPartsIteratorTest { + private static final String BOUNDARY = "AaB03x"; + + private static final String MULTI_PART_BODY = "--" + BOUNDARY + "\r\n" + + "Content-Disposition: form-data; name=\"file1\"; filename=\"a.txt\"\r\n" + + "Content-Type: text/plain\r\n" + + "\r\n" + + "hello\r\n" + + "--" + BOUNDARY + "\r\n" + + "Content-Disposition: form-data; name=\"file2\"; filename=\"b.txt\"\r\n" + + "Content-Type: text/plain\r\n" + + "\r\n" + + "world\r\n" + + "--" + BOUNDARY + "--\r\n"; + @Test(expected = SlingParameterParseException.class) public void testHasNextIsRejectedOnFileUploadException() throws Exception { final RequestPartsIterator iterator = newIterator(); @@ -102,7 +127,7 @@ public class RequestPartsIteratorTest { when(context.getCharacterEncoding()).thenReturn("UTF-8"); when(context.getContentLength()).thenReturn(body.length()); when(context.getInputStream()).thenReturn(new ByteArrayInputStream(body.getBytes(Util.ENCODING_DIRECT))); - return new RequestPartsIterator(context); + return new RequestPartsIterator(context, -1, -1, 50); } private static void injectDelegate(final RequestPartsIterator iterator, final FileItemIterator delegate) @@ -111,4 +136,60 @@ public class RequestPartsIteratorTest { field.setAccessible(true); field.set(iterator, delegate); } + + private static RequestContext multiPartContext() throws IOException { + final byte[] body = MULTI_PART_BODY.getBytes(Util.ENCODING_DIRECT); + final RequestContext context = mock(RequestContext.class); + when(context.getContentType()).thenReturn("multipart/form-data; boundary=" + BOUNDARY); + when(context.getCharacterEncoding()).thenReturn("UTF-8"); + when(context.getContentLength()).thenReturn(body.length); + when(context.getInputStream()).thenReturn(new ByteArrayInputStream(body)); + return context; + } + + @Test + public void testAllPartsIteratedWithoutLimits() throws Exception { + final RequestPartsIterator it = new RequestPartsIterator(multiPartContext(), -1, -1, 50); + assertTrue(it.hasNext()); + final Part first = it.next(); + assertNotNull(first); + assertEquals("file1", first.getName()); + assertTrue(it.hasNext()); + final Part second = it.next(); + assertNotNull(second); + assertEquals("file2", second.getName()); + assertFalse(it.hasNext()); + } + + @Test + public void testFileCountMaxEnforced() throws Exception { + // the configured count must be enforced even though commons-fileupload's + // streaming API does not check fileCountMax itself + final RequestPartsIterator it = new RequestPartsIterator(multiPartContext(), -1, -1, 1); + assertTrue(it.hasNext()); + assertNotNull(it.next()); + // the second part exceeds the configured count limit + assertFalse(it.hasNext()); + } + + @Test + public void testSizeMaxEnforced() throws Exception { + try { + new RequestPartsIterator(multiPartContext(), 10, -1, 50); + fail("Expected the configured request size limit to be enforced"); + } catch (FileUploadException expected) { + // the request exceeds the configured maximum request size + } + } + + @Test + public void testGetSizeIsUnknownNotZero() throws Exception { + final RequestPartsIterator it = new RequestPartsIterator(multiPartContext(), -1, -1, 50); + assertTrue(it.hasNext()); + final Part part = it.next(); + assertNotNull(part); + // the size of a streamed part is unknown: it must not read as an empty + // part to size-limit checks of downstream consumers + assertEquals(-1, part.getSize()); + } }
