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-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
