Bug 61999

Summary: Setting maxSavePostSize=0 won't disable saving POST data
Product: Tomcat 8 Reporter: Michael <bsi.msa>
Component: CatalinaAssignee: Tomcat Developers Mailing List <dev>
Severity: normal CC: bsi.msa
Priority: P2    
Version: 8.5.x-trunk   
Target Milestone: ----   
Hardware: PC   
OS: All   

Description Michael 2018-01-15 10:16:24 UTC
The documentation for the Connector attribute "maxSavePostSize" says "Setting the attribute to zero will disable the saving of POST data during authentication.":

However, we tested this and maxSavePostSize=0 won't disable saving POST data. Instead, it actually tries to save the data with limit 0, so if there is any POST data, a 403 Forbidden is sent in the response.

Also, looking at the corresponding source code, there is no special handling for ignoring POST data if maxSavePostSize is set to zero:
FormAuthenticator#saveRequest(Request request, Session session) creates a ByteChunk with limit 0. When calling ByteChunk#append(byte src[], int off, int len) we get to the flushBuffer() method which throws an IOException caught by FormAuthenticator#doAuthenticate which then sends a 403 Forbidden.

There is only special handling for the case where maxSavePostSize is negative (i.e. no limit).
Comment 1 Remy Maucherat 2018-01-15 12:26:45 UTC
Ok, so the documentation isn't implemented properly when it comes to 0. Do you have an actual need to disable the feature, or is this one of these academic bug reports ? I'm asking since disabling the feature will make requests fail, while the actual cost of the feature is rather low with the default value, hence the user benefit is non existent and the fix would instead be to fix the docs [value <= 0 means no limit].
Comment 2 Michael 2018-01-15 13:21:24 UTC
Thanks for your fast reply. Yes we have an actual need to disable the "save post data" feature during authentication. We do not want the request to fail (with 403) just because the POST data was more than x KB since we do not need the POST data to be saved. Reading the documentation, we thought to have found a solution for this problem by setting maxSavePostSize=0 so the POST data would be ignored while the request is still being processed.

It is our understanding that saving the POST data of the request is a performance improvement rather than a necessity.

In our use case we use the FormAuthenticator but do not redirect to a simple HTML form but rather to a URL which does a programmatic login. The POST data is irrelevant for the login and will be sent again from the client after authentication.

Thus fixing the documentation would not help in our case. Furthermore the current implementation behaves as follows:
* value < 0 means no limit
* value >= 0 means limited to the value => so for value=0 every request with any POST data will fail (403 Forbidden)

So your suggestions to fix the documentation would not match with the current implementation.
Comment 3 Michael 2018-01-15 13:23:25 UTC
BTW: We are willing to provide a patch if you agree.
Comment 4 Remy Maucherat 2018-01-15 14:28:43 UTC
"Furthermore the current implementation behaves as follows:
* value < 0 means no limit
* value >= 0 means limited to the value => so for value=0 every request with any POST data will fail (403 Forbidden)"

I don't want to argue forever, but IMO this doesn't make much sense. Why would your request "not fail" if it is too large ? If the data is irrelevant, don't send it in the first place, especially since it will have to be read by the webserver anyway. Also, clients usually do not silently resend post data.

Last, actually, the current behavior is that <= 0 means no limit.
Comment 5 Remy Maucherat 2018-01-15 15:02:47 UTC
A fix will be in 9.0.4, 8.5.25, 8.0.49 and 7.0.84.
Comment 6 Michael 2018-01-16 09:58:21 UTC
(In reply to Remy Maucherat from comment #5)
> A fix will be in 9.0.4, 8.5.25, 8.0.49 and 7.0.84.

Great, thank you!

With this fix our request won't fail anymore for maxSavePostSize=0. And for maxSavePostSize > 0 it still fails if the POST data is larger than maxSavePostSize, as expected.

We are looking forward to integrate 8.5.25 as soon as it's released.

Just for clarity, why my understanding was that only strictly < 0 means no limit was this line of code (and the tests we made):