markt-asf commented on PR #893: URL: https://github.com/apache/tomcat/pull/893#issuecomment-3284412138
-1 (veto) to the proposed solution. If header lookup was case-sensitive, this solution would break case insensitive header lookup from the point the header is parsed until processing reaches the new Valve. There are a lot of header look ups in that time and this proposal would break those look ups. However... `MimeHeaders` has been using `MessageBytes.equalsIgnoreCase()` all the way back to the original Tomcat 4.1.x code (almost 25 years ago) so https://bz.apache.org/bugzilla/show_bug.cgi?id=58464#c3 does not appear to be entirely correct. Which begs the question, why was the conversion to lower case added to `Http11InputBuffer`? Hmm. That too goes all the way back to the original Tomcat 4.1.x code. The proposed solution is still the wrong solution but mostly because it is based on incorrect information. Getting back to the original problem - preserving the case of the HTTP request header names as provided by the client - it looks removing the conversion to lowercase is the only change that is required. The header names are only returned in two places in Tomcat. The first is in response to a call to `HttpServletRequest.getHeaderNames()`. Given the HTTP spec says HTTP header names are case insensitive, applications should be treating any header names obtained from this method in a case insensitive manner. However, that is as much a reason not to change the current behaviour as it is to change it. Fundamentally, it should not matter what case is used for these header names and an application/library that is case sensitive for these names is broken and should be fixed. The Tomcat community generally does NOT provide fixes/workarounds for other projects' broken code. Doing so perpetuates the problem and creates a maintenance burden for Tomcat (which is doing the right thing) rather than the broken application/library (which is not). The second is in the validation of an HTTP/2 request. Removing the forcing to lowercase from HTTP/1.1 processing would not affect HTTP/2. I would be against implementing any change simply to benefit broken clients/libraries. However, looking at this issue has shown that there is potential for some performance optimisations. And those are always of interest. There appears to be two different optimisations that could be applied. A. Remove the forcing of header name to lower case from HTTP/1.1 requests B. Take advantage of headers being stored in lower case and switch `MimeHeaders` to use `equals()` rather than `equalsIgnoreCase()` Option A looks to be the simpler solution to implement but only removes a single mixed-case to lowercase conversion. Option B may provide more opportunity for performance gains. `equalsIgnoresCase()` essentially converts both inputs to lower case before comparison. If all header names are known to be lower case then the number of these conversions can be reduced. There is a greater risk of regressions with option B since all response header names would need to be forced to lowercase. Again, any issues with the case of the HTTP header names would be with the client/library not being HTTP/1.1 compliant. My current thinking is that Tomcat 9.0.x to 11.0.x could implement option A whereas option B could be explored for Tomact 12 onwards. I'd like to have an idea of the performance improvements for both before any implementation changes though. I'll do some experimenting. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
