Limit usage of Tomcat's Connector#setProperty to a minimum #160
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The SSL configuration of Tomcat 9 does not work any longer. The existing implementation used deprecated property names. Additionally, the SSLHostConfig configuration object is no longer created automatically if missing. Compare: http://tomcat.apache.org/tomcat-9.0-doc/config/http.html#SSL_Support_-_Connector_-_NIO_and_NIO2_(deprecated) to: http://tomcat.apache.org/tomcat-10.0-doc/config/http.html#SSL_Support_-_SSLHostConfig
Calling toString() is mandatory, otherwise the call does not match the method signature of setProperty(String, String). This is now analog to the treatment of httpIdleTimeout, which already stringifies the property value.
Connector#setProperty is a bit of a lie. It does not set properties
on the Connector. Rather, the method set properties on the protocol
handler which is part of the connector.
Thus, the previous implementation tried to set a connector property
using setProperty, which was never successful.
Instead we directly set the property of the connector. To achieve
the original intent ("unlimited post size") the value must be
below zero. See:
http://tomcat.apache.org/tomcat-10.0-doc/config/http.html#Common_Attributes
Connector#setProperty returns a success boolean. If the property is deprecated or goes missing, we will immediately notice.
boris-petrov
approved these changes
May 8, 2020
|
@f4lco - looks great to me! Feel free to merge whenever you want. :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR contains a couple of general fixes / improvements, which I developed while integrating Tomcat 10 (#144 / #159), that are also useful to existing Tomcat servlet containers prior to version ten.
The changes common motive consists of limiting usage of the stringly-typed method
Connector#setProperty(String name, String value):SSLHostConfig. This gets rid of properties which are already deprecated in Tomcat 9, and cease to exist in Tomcat 10.httpsIdleTimeout, the caller failed to match the signature ofsetProperty, because the second argument was of type Integer, causing a runtime error that prevented using that feature.maxPostSize, that call ofsetPropertynever worked, because the target object has no property with that name. Also, the config value of '0' is wrong for the stated purpose. According to Tomcat 8/8.5/9/10 documentation, a value below zero configures unlimited post size.setPropertyshould never fail silently. We should assert on the return value ofsetProperty, which is a bool indicating whether that call succeeded (that the caller has met requirements for property name and data type).