|Summary:||Posting data exceeding maxPostSize should result in HTTP 413.|
|Product:||Tomcat 6||Reporter:||Christopher Simons <christopherleesimons>|
|Component:||Catalina||Assignee:||Tomcat Developers Mailing List <dev>|
Initial draft of patch, created against trunk.
Add FailedRequestFilter note to maxPostSize documentation.
Description Christopher Simons 2015-06-13 07:42:06 UTC
Created attachment 32821 [details] Initial draft of patch, created against trunk. When data exceeding the value of the "maxPostSize" configuration parameter is posted within a deployed application, the application sees an empty request parameter map and cannot access the posted data, yet Tomcat returns a 200 status, indicating to the client that everything was processed successfully (which likely will mislead the client). The correct behavior is to return a status of 413 ("Request Entity Too Large") to indicate that the request could not be fully processed due to the size being too large. Attached is a patch that will set the response status to 413 when Tomcat discovers that the posted data exceeds maxPostSize.
Comment 1 Christopher Schultz 2015-06-16 19:53:39 UTC
I'm iffy on this patch because it's not configurable (I'd like to point out that the current behavior is also not configurable, and this definitely is an improvement). The application itself might want to check this situation and react differently, like it can when there are too many request parameters. You should check out the FailedRequestFilter to see what happens in that case: http://tomcat.apache.org/tomcat-8.0-doc/config/filter.html#Failed_Request_Filter It does almost what you want, except that it returns a 400 response instead of 413, and only currently works for violations of maxParameterCount. Look into how Globals.PARAMETER_PARSE_FAILED_ATTR is used to communicate this situation to the application and see if you can come up with something that might work better than your current proposal.
Comment 2 Mark Thomas 2015-06-16 20:46:02 UTC
Agreed. The right response is definitely going to be application dependent. Most of the problem stems from getParameterXXX() methods not declaring any exceptions. We also run the risk of breaking lots of stuff if we start sending 413 responses where we currently send a 200 response. On balance I think setting some a custom request attribute that an application can check if they want to (and if not the current behaviour continues) along the lines of Globals.PARAMETER_PARSE_FAILED_ATTR is going to be the way to go.
Comment 3 Christopher Simons 2015-06-16 21:31:00 UTC
Created attachment 32829 [details] Add FailedRequestFilter note to maxPostSize documentation.
Comment 4 Christopher Simons 2015-06-16 21:31:39 UTC
(In reply to Christopher Schultz from comment #1) > Look into how Globals.PARAMETER_PARSE_FAILED_ATTR is used to communicate > this situation to the application and see if you can come up with something > that might work better than your current proposal. It appears that both checks for maxPostSize do result in Globals.PARAMETER_PARSE_FAILED_ATTR being set: in Request#parseParts, an exception is thrown, and in the finally block of the try/catch/finally block parameters.setParseFailed(true) is set. In Request#parseParameters, the try/catch/finally block is exited without setting 'success' to true, and the finally block calls parameters.setParseFailed(true) in this case. So it appears the original concern about return 200 in this case is addressed. The documentation for maxParameterCount notes that FailedRequestFilter can be used to reject requests that exceed this limit. I've attached a patch to include the same note in the maxPostSize documentation after verifying 400 is returned in this case (as noted above). > It does almost what you want, except that it returns a 400 response instead > of 413 Unconditionally sending a 400 from FailedRequestFilter upon failure seems incorrect as the HTTP spec states that 400 is to be used when the request failed "due to malformed syntax," which isn't the case for maxPostSize and maxParameterCount violations; in some parse failure cases 400 is correct, but for these violations 413 seems more appropriate. I'd propose implementing a mechanism to indicate from the parsing code which status should be returned from FailedRequestParameter. Thoughts?  HTTP/1.1: Status Code Definitions http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.4.1
Comment 5 Mark Thomas 2015-06-16 22:00:17 UTC
I was thinking a using the existing attribute to trigger a 400 response and a new attribute to trigger a 413 response. That should cover all the current possibilities for parameter/part issues. Note RFC 2616 is obsolete.
Comment 6 Konstantin Kolinko 2015-06-16 22:06:30 UTC
(In reply to Christopher L. Simons from comment #4) > > Unconditionally sending a 400 from FailedRequestFilter upon failure seems > incorrect as the HTTP spec states that 400 is to be used when the request > failed "due to malformed syntax," > >  HTTP/1.1: Status Code Definitions > http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.4.1 In my opinion, it would be nice to use 413, but it is not wrong to use 400. "400" is just the generic error code in 4xx series. It says "it is client's fault". Nothing more. Likewise "500" is the generic error code in 5xx series. Note that RFC2616 is obsolete. Current specification for HTTP/1.1 status codes is RFC 7231: http://tools.ietf.org/html/rfc7231#section-6.5.1 HTTP RFCs are listed at https://wiki.apache.org/tomcat/Specifications#HTTP
Comment 7 Konstantin Kolinko 2015-06-16 22:59:16 UTC
(In reply to Mark Thomas from comment #5) > I was thinking a using the existing attribute to trigger a 400 response and > a new attribute to trigger a 413 response. That should cover all the current > possibilities for parameter/part issues. -1. By my '-1' vote I mean that when "using a new attribute to trigger a 413 response" I am against changing the meaning of the existing attribute. If something failed, the existing attribute should continue to indicate failure. It can be 1) Globals.PARAMETER_PARSE_FAILED_ATTR to signal presence of an error 2) additional attribute to signal the kind of an error. Originally when introducing PARAMETER_PARSE_FAILED_ATTR I have not defined what its value is. It was documented as "absent = success", "any not-null value = failure" with an intent to introduce different not-null values for different use cases. Nowadays javadoc for Globals.PARAMETER_PARSE_FAILED_ATTR explicitly mentions Boolean.TRUE as the value. Thus for compatibility it is better to go with a separate attribute to communicate the reason of the failure. Technically, PARAMETER_PARSE_FAILED_ATTR is a facade that exposes the value of internal coyoteRequest.getParameters().isParseFailed() flag. For reference: r1200218 Historically this attribute and FailedRequestFilter were introduced as a review of CVE-2012-0022 fix that introduced "maxParameterCount" limit. The intent was to be able to perform a simple test that none parameters were lost. Also I think that exceeding the "maxParameterCount" limit also would better result in response status 413. I wish there were a Servlet API way to communicate parameter processing errors. HttpServletRequest.getParts() and getPart() methods implement some way to indicate errors by throwing an exception, but an IllegalStateException thrown there is used to indicate both "size limit exceeded" and "missing multipart-config" conditions.  There exist "javax.servlet.error.exception" and other standard Request attributes as defined in ch.10.9.1 of Servlet 3.1 specification, but IIRC they are used only when performing an error processing dispatch.  http://docs.oracle.com/javaee/7/api/javax/servlet/http/HttpServletRequest.html#getParts%28%29
Comment 8 Mark Thomas 2015-06-17 13:06:13 UTC
(In reply to Konstantin Kolinko from comment #7) > (In reply to Mark Thomas from comment #5) > > I was thinking a using the existing attribute to trigger a 400 response and > > a new attribute to trigger a 413 response. That should cover all the current > > possibilities for parameter/part issues. > > -1. Fair enough. How about using a second attribute to set the response code? If set use the defined code. If not set, use 400.
Comment 9 Christopher Schultz 2015-06-18 15:38:08 UTC
I'd prefer being able to set the response code using an init-param. If we want to keep the current behavior and add on to it, it seems we need a second attribute to indicate the second condition. In either case, I think it woul dbe nice for the user to be able to customize the HTTP response code. [I don't like returning 413 (Entity Too Large) for "too many request parameters" unless it's actually a POST with a request entity.]
Comment 10 Mark Thomas 2015-06-24 13:27:20 UTC
I've patched trunk (9.0.x) to make the reason for the failure available as a request attribute that the FailedRequestFilter can then use to provide a better status code to the client. I'll give it a few days for folks to review and comment on the patch before back-porting it.
Comment 11 Mark Thomas 2015-08-06 10:06:13 UTC
No objections were raised so I have back-ported this to 8.0.x (for 8.0.25 onwards) and 7.0.x (for 7.0.64 onwards). I have also proposed it for 6.0.x.
Comment 12 Christopher Simons 2015-10-21 13:54:31 UTC
Any objections to applying the attached documentation patch? I think it would save many users from having to search and dig up this post to realize they can use FailedRequestFilter to adjust the behavior that occurs when @maxPostSize is exceeded.
Comment 13 Mark Thomas 2015-11-03 12:34:03 UTC
Fixed in 6.0.x for 6.0.45 onwards.