This is an automated email from the ASF dual-hosted git repository. markt pushed a commit to branch 11.0.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/11.0.x by this push: new bac6f1dd48 Fix BZ 69731 - correct maxParameterCount tracking. bac6f1dd48 is described below commit bac6f1dd489535fe6d3eaec9db4878898ce380ca 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 6f4d313928..2b34c03534 100644 --- a/java/org/apache/catalina/connector/Request.java +++ b/java/org/apache/catalina/connector/Request.java @@ -111,6 +111,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; @@ -2482,6 +2483,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); @@ -2582,11 +2603,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) { @@ -2834,11 +2858,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 98673d96e0..cfff9178c4 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -126,6 +126,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