[GitHub] [tomcat] ChristopherSchultz commented on pull request #506: Adding Redirect and Proxy Error Reporting Valves

[GitBox <gi...@apache.org>]

ChristopherSchultz commented on PR #506:
URL: https://github.com/apache/tomcat/pull/506#issuecomment-1116329419

   Thank you for your contribution. I have a few comments:
   
   1. The javadoc was copy/pasted and inaccurate: both classes claim to serve JSPs for errors, and neither of them do this.
   2. Each valve is configured using an external properties file instead of within `server.xml` like valves are typically configured.
   3. The documentation (XML) disagrees with the implementation, including documenting <Valve> attributes that don't have any effect and confusing them with entries in the file-based configuration and failing to document `nnn=` vs `http.nnn=` in any way
   4. Trace log statement(s) is/are not guarded by e.g. `log.isTraceEnabled`
   5. The URL parameters are not consistently sanitized using `URLEncoder.encode`
   6. There doesn't seem to be a reason to prohibit URL parameters from the "base URL" for each valve
   
   For the "Proxy" error report valve:
   1. No attempt is made to set the response's Content-Length or other appropriate headers coming from the proxied connection (I do see the Content-Type in there, which is good
   2. This valve will fail if the request has already been using a Writer
   3. This valve will fail if `url.openStream` returns `null`
   4. Performance: Please use `StringBuilder` instead of string-concatenation when possible
   
   


-- 
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: dev-unsubscribe@tomcat.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org