This is an automated email from the ASF dual-hosted git repository.

markt pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git


The following commit(s) were added to refs/heads/main by this push:
     new b55723bc0f Fix BZ 69731 - correct maxParameterCount tracking.
b55723bc0f is described below

commit b55723bc0fd7e8b0c34d69c410f09c2a1de8f4fb
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Tue Jul 1 19:04:18 2025 +0100

    Fix BZ 69731 - correct maxParameterCount tracking.
    
    Limit was was smaller than intended for multipart uploads with non-file
    parts when the parts were processed before query string parameters
    
    https://bz.apache.org/bugzilla/show_bug.cgi?id=69731
---
 java/org/apache/catalina/connector/Request.java    |  50 +++++++-
 .../catalina/valves/TestParameterLimitValve.java   | 134 ++++++++++++++++++++-
 webapps/docs/changelog.xml                         |   6 +
 3 files changed, 184 insertions(+), 6 deletions(-)

diff --git a/java/org/apache/catalina/connector/Request.java 
b/java/org/apache/catalina/connector/Request.java
index ce06e6e10d..760e728e28 100644
--- a/java/org/apache/catalina/connector/Request.java
+++ b/java/org/apache/catalina/connector/Request.java
@@ -109,6 +109,7 @@ import org.apache.tomcat.util.http.ServerCookies;
 import org.apache.tomcat.util.http.fileupload.FileItem;
 import org.apache.tomcat.util.http.fileupload.FileUpload;
 import org.apache.tomcat.util.http.fileupload.disk.DiskFileItemFactory;
+import 
org.apache.tomcat.util.http.fileupload.impl.FileCountLimitExceededException;
 import org.apache.tomcat.util.http.fileupload.impl.InvalidContentTypeException;
 import org.apache.tomcat.util.http.fileupload.impl.SizeException;
 import org.apache.tomcat.util.http.fileupload.servlet.ServletRequestContext;
@@ -2374,6 +2375,26 @@ public class Request implements HttpServletRequest {
             }
         }
 
+        /*
+         * When the request body is multipart/form-data, both the parts and 
the query string count towards
+         * maxParameterCount. If parseParts() is called before 
getParameterXXX() then the parts will be parsed before
+         * the query string. Otherwise, the query string will be parsed first.
+         *
+         * maxParameterCount must be respected regardless of which is parsed 
first.
+         *
+         * maxParameterCount is reset from the Connector at the start of every 
request.
+         *
+         * If parts are parsed first, non-file parts will be added to the 
parameter map and any files will reduce
+         * maxParameterCount by 1 so that when the query string is parsed the 
difference between the size of the
+         * parameter map and maxParameterCount will be the original 
maxParameterCount less the number of parts. i.e. the
+         * maxParameterCount applied to the query string will be the original 
maxParameterCount less the number of
+         * parts.
+         *
+         * If the query string is parsed first, all parameters will be added 
to the parameter map and, ignoring
+         * maxPartCount, the part limit will be set to the original 
maxParameterCount less the size of the parameter
+         * map. i.e. the maxParameterCount applied to the parts will be the 
original maxParameterCount less the number
+         * of query parameters.
+         */
         Parameters parameters = coyoteRequest.getParameters();
         parameters.setLimit(maxParameterCount);
 
@@ -2474,11 +2495,14 @@ public class Request implements HttpServletRequest {
                         // Not possible
                     }
                     parameters.addParameter(name, value);
+                } else {
+                    // Adjust the limit to account for a file part which is 
not added to the parameter map.
+                    maxParameterCount--;
                 }
             }
         } catch (InvalidContentTypeException e) {
             partsParseException = new ServletException(e);
-        } catch (SizeException e) {
+        } catch (SizeException | FileCountLimitExceededException e) {
             checkSwallowInput();
             partsParseException = new InvalidParameterException(e, 
HttpServletResponse.SC_REQUEST_ENTITY_TOO_LARGE);
         } catch (IOException e) {
@@ -2726,11 +2750,27 @@ public class Request implements HttpServletRequest {
         }
         parametersParsed = true;
 
+        /*
+         * When the request body is multipart/form-data, both the parts and 
the query string count towards
+         * maxParameterCount. If parseParts() is called before 
getParameterXXX() then the parts will be parsed before
+         * the query string. Otherwise, the query string will be parsed first.
+         *
+         * maxParameterCount must be respected regardless of which is parsed 
first.
+         *
+         * maxParameterCount is reset from the Connector at the start of every 
request.
+         *
+         * If parts are parsed first, non-file parts will be added to the 
parameter map and any files will reduce
+         * maxParameterCount by 1 so that when the query string is parsed the 
difference between the size of the
+         * parameter map and maxParameterCount will be the original 
maxParameterCount less the number of parts. i.e. the
+         * maxParameterCount applied to the query string will be the original 
maxParameterCount less the number of
+         * parts.
+         *
+         * If the query string is parsed first, all parameters will be added 
to the parameter map and, ignoring
+         * maxPartCount, the part limit will be set to the original 
maxParameterCount less the size of the parameter
+         * map. i.e. the maxParameterCount applied to the parts will be the 
original maxParameterCount less the number
+         * of query parameters.
+         */
         Parameters parameters = coyoteRequest.getParameters();
-
-        if (parts != null && maxParameterCount > 0) {
-            maxParameterCount -= parts.size();
-        }
         parameters.setLimit(maxParameterCount);
 
         // getCharacterEncoding() may have been overridden to search for
diff --git a/test/org/apache/catalina/valves/TestParameterLimitValve.java 
b/test/org/apache/catalina/valves/TestParameterLimitValve.java
index 178ba31fe6..2152ba39b3 100644
--- a/test/org/apache/catalina/valves/TestParameterLimitValve.java
+++ b/test/org/apache/catalina/valves/TestParameterLimitValve.java
@@ -511,6 +511,138 @@ public class TestParameterLimitValve extends 
TomcatBaseTest {
     }
 
 
+    @Test
+    public void testMaxParameterCountLimitExceeded01_02_00_00() throws 
Exception {
+        doTestMaxParameterCountLimitExceeded(1, 2, 0, 0, false);
+    }
+
+
+    @Test
+    public void testMaxParameterCountLimitExceeded01_00_02_00() throws 
Exception {
+        doTestMaxParameterCountLimitExceeded(1, 0, 2, 0, false);
+    }
+
+
+    @Test
+    public void testMaxParameterCountLimitExceeded01_00_00_02() throws 
Exception {
+        doTestMaxParameterCountLimitExceeded(1, 0, 0, 2, false);
+    }
+
+
+    @Test
+    public void testMaxParameterCountLimitExceeded01_01_00_00() throws 
Exception {
+        doTestMaxParameterCountLimitExceeded(1, 1, 0, 0, true);
+    }
+
+
+    @Test
+    public void testMaxParameterCountLimitExceeded01_00_01_00() throws 
Exception {
+        doTestMaxParameterCountLimitExceeded(1, 0, 1, 0, true);
+    }
+
+
+    @Test
+    public void testMaxParameterCountLimitExceeded01_00_00_01() throws 
Exception {
+        doTestMaxParameterCountLimitExceeded(1, 0, 0, 1, true);
+    }
+
+
+    @Test
+    public void testMaxParameterCountLimitExceeded02_01_01_00() throws 
Exception {
+        doTestMaxParameterCountLimitExceeded(2, 1, 1, 0, true);
+    }
+
+
+    @Test
+    public void testMaxParameterCountLimitExceeded02_01_0_01() throws 
Exception {
+        doTestMaxParameterCountLimitExceeded(2, 1, 0, 1, true);
+    }
+
+
+    @Test
+    public void testMaxParameterCountLimitExceeded02_00_01_01() throws 
Exception {
+        doTestMaxParameterCountLimitExceeded(2, 0, 1, 1, true);
+    }
+
+
+    @Test
+    public void testMaxParameterCountLimitExceeded03_01_01_01() throws 
Exception {
+        doTestMaxParameterCountLimitExceeded(3, 1, 1, 1, true);
+    }
+
+
+    private void doTestMaxParameterCountLimitExceeded(int maxParameterCount, 
int textPartCount, int filePartCount,
+            int queryStringCount, boolean okExpected) throws Exception {
+
+        Tomcat tomcat = getTomcatInstance();
+        StandardContext ctx = (StandardContext) getProgrammaticRootContext();
+
+        ParameterLimitValve parameterLimitValve = new ParameterLimitValve();
+        ctx.getPipeline().addValve(parameterLimitValve);
+        // Only looking to test maxParameterCount
+        parameterLimitValve.setUrlPatternLimits("/upload/.*=" + 
Integer.toString(maxParameterCount) + ",-1,-1");
+
+        Wrapper w = Tomcat.addServlet(ctx, "multipart", new 
MultipartServlet());
+        // Use defaults for Multipart
+        w.setMultipartConfigElement(new MultipartConfigElement(""));
+        ctx.addServletMappingDecoded("/upload/*", "multipart");
+
+        tomcat.start();
+
+        // Construct a simple multi-part body
+        String boundary = "--simpleBoundary";
+
+        StringBuilder content = new StringBuilder();
+        int part = 1;
+
+        for (int i = 0; i < textPartCount; i++) {
+            content.append("--").append(boundary).append(CRLF);
+            content.append("Content-Disposition: form-data; 
name=\"part").append(part).append("\"").append(CRLF);
+            content.append(CRLF);
+            content.append("part value ").append(part).append(CRLF);
+            part++;
+        }
+
+        for (int i = 0; i < filePartCount; i++) {
+            content.append("--").append(boundary).append(CRLF);
+            content.append("Content-Disposition: form-data; 
name=\"part").append(part).append("\"; filename=\"part")
+                    .append(part).append("\"").append(CRLF);
+            content.append("Content-Type: text/plain").append(CRLF);
+            content.append(CRLF);
+            content.append("part value ").append(part).append(CRLF);
+            part++;
+        }
+
+        content.append("--").append(boundary).append("--").append(CRLF);
+
+        StringBuilder queryString = new StringBuilder();
+        for (int i = 0; i < queryStringCount; i++) {
+            if (i > 0) {
+                queryString.append("&");
+            }
+            queryString.append("param");
+            queryString.append(part);
+            queryString.append("=value");
+            queryString.append(part);
+            part++;
+        }
+
+
+        Map<String,List<String>> reqHeaders = new HashMap<>();
+        reqHeaders.put("Content-Type", List.of("multipart/form-data; 
boundary=" + boundary));
+        reqHeaders.put("Content-Length", 
List.of(Integer.toString(content.length())));
+
+        int rc = postUrl(content.toString().getBytes(), "http://localhost:"; + 
getPort() + "/upload/endpoint?" +
+                queryString.toString(), new ByteChunk(), reqHeaders, null);
+
+        if (okExpected) {
+            Assert.assertEquals(HttpServletResponse.SC_OK, rc);
+        } else {
+            Assert.assertTrue(Integer.toString(rc),
+                    rc == HttpServletResponse.SC_BAD_REQUEST || rc == 
HttpServletResponse.SC_REQUEST_ENTITY_TOO_LARGE);
+        }
+    }
+
     private static class MultipartServlet extends HttpServlet {
 
         private static final long serialVersionUID = 1L;
@@ -520,8 +652,8 @@ public class TestParameterLimitValve extends TomcatBaseTest 
{
             resp.setContentType("text/plain");
             resp.setCharacterEncoding(StandardCharsets.UTF_8);
             PrintWriter pw = resp.getWriter();
-            pw.println("Parameters: " + req.getParameterMap().size());
             pw.println("Parts: " + req.getParts().size());
+            pw.println("Parameters: " + req.getParameterMap().size());
         }
     }
 }
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 4dae024faf..ce4d131cb4 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -181,6 +181,12 @@
         below the web application root with a path that was terminated with a
         file separator. (remm/markt)
       </fix>
+      <fix>
+        <bug>69731</bug>: Fix an issue that meant that the value of
+        <code>maxParameterCount</code> applied was smaller than intended for
+        multipart uploads with non-file parts when the parts were processed
+        before query string parameters. (markt)
+      </fix>
     </changelog>
   </subsection>
   <subsection name="Coyote">


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Reply via email to