This is an automated email from the ASF dual-hosted git repository.
joerghoh pushed a commit to branch master
in repository
https://gitbox.apache.org/repos/asf/sling-org-apache-sling-engine.git
The following commit(s) were added to refs/heads/master by this push:
new 11bdcee SLING-13372 enforce configured multipart limits on streamed
uploads (#102)
11bdcee is described below
commit 11bdcee45f9f264ceb7384deae35c07dadffc317
Author: Jörg Hoh <[email protected]>
AuthorDate: Mon Oct 5 15:57:30 2026 +0200
SLING-13372 enforce configured multipart limits on streamed uploads (#102)
* 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 | 49 ++++++++-
.../impl/parameters/ParameterSupportTest.java | 53 +++++++++
.../impl/parameters/RequestPartsIteratorTest.java | 119 ++++++++++++++++++++-
4 files changed, 220 insertions(+), 7 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..14b3752 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
@@ -25,6 +25,7 @@ import java.util.Collection;
import java.util.Collections;
import java.util.Iterator;
import java.util.List;
+import java.util.NoSuchElementException;
import jakarta.servlet.http.Part;
import org.apache.commons.fileupload.FileItemIterator;
@@ -44,22 +45,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);
@@ -69,8 +99,13 @@ public class RequestPartsIterator implements Iterator<Part> {
@Override
public Part next() {
+ if (fileCountMax >= 0 && partCount >= fileCountMax) {
+ throw new NoSuchElementException("The configured limit of " +
fileCountMax + " parts has been reached");
+ }
try {
- return new StreamedRequestPart(itemIterator.next());
+ final Part part = new StreamedRequestPart(itemIterator.next());
+ partCount++;
+ return part;
} catch (final FileUploadException | IOException e) {
LOG.error("next Item failed cause:" + e.getMessage(), e);
throw new SlingParameterParseException("Error reading next part
from the request stream", e);
@@ -111,7 +146,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..8bb046e 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
@@ -21,23 +21,51 @@ package org.apache.sling.engine.impl.parameters;
import java.io.ByteArrayInputStream;
import java.io.IOException;
import java.lang.reflect.Field;
+import java.util.NoSuchElementException;
+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.assertThrows;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verifyNoInteractions;
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 +130,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 +139,93 @@ 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());
+ assertThrows(NoSuchElementException.class, it::next);
+ }
+
+ @Test
+ public void testFileCountMaxEnforcedWithoutHasNext() throws Exception {
+ final RequestPartsIterator it = new
RequestPartsIterator(multiPartContext(), -1, -1, 1);
+ assertEquals("file1", it.next().getName());
+ final FileItemIterator delegate = mock(FileItemIterator.class);
+ injectDelegate(it, delegate);
+
+ assertThrows(NoSuchElementException.class, it::next);
+ assertThrows(NoSuchElementException.class, it::next);
+ verifyNoInteractions(delegate);
+ }
+
+ @Test
+ public void testZeroFileCountMaxRejectsNext() throws Exception {
+ final RequestPartsIterator it = new
RequestPartsIterator(multiPartContext(), -1, -1, 0);
+ final FileItemIterator delegate = mock(FileItemIterator.class);
+ injectDelegate(it, delegate);
+
+ assertThrows(NoSuchElementException.class, it::next);
+ verifyNoInteractions(delegate);
+ }
+
+ @Test
+ public void testUnlimitedFileCountAllowsNextWithoutHasNext() throws
Exception {
+ final RequestPartsIterator it = new
RequestPartsIterator(multiPartContext(), -1, -1, -1);
+
+ assertEquals("file1", it.next().getName());
+ assertEquals("file2", it.next().getName());
+ assertThrows(NoSuchElementException.class, it::next);
+ 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());
+ }
}
