Adding Redirect and Proxy Error Reporting Valves

[maxfortun]

These 2 error reporting valves allow to delegate error report for dynamic generation to an external source either via issuing a redirect or proxying the request.

[ChristopherSchultz]

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 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

[markt-asf]

Writing an HTTP proxy is non-trivial. I am going to need a LOT of convincing that the proxy Valve is both safe and compliant with the relevant RFCs.

[ChristopherSchultz]

Writing an HTTP proxy is non-trivial. I am going to need a LOT of convincing that the proxy Valve is both safe and compliant with the relevant RFCs.

I'd like to point out that, despite it's name, this is not a proxy. It's just fetching its error response from another server, so IMHO doesn't have any expectation of being compliant with any particular RFC(s).

That said, I'm curious as to why any of this is necessary.

You can set up whatever you want in <error-page> elements in WEB-INF/web.xml and handle errors any way you want including redirects, forwards, external-fetches, etc. There is no particular reason to create any new Valves for these purposes.

[maxfortun]

@ChristopherSchultz thank you for your meticulous review, I'll make the adjustments. As far as the "why". We are deploying hundreds of containerized apps that need the same tomcat behavior and do not want to configure this on app level. It is more straightforward to delegate error formatting to external error formatter and make updates to formatter ONLY, without having to redeploy a multitude of apps.

[markt-asf]

This feels more like a custom ErrorReportValve rather than something that would ship with Tomcat. I do agree with Chris that a lot (all?) of this functionality could be achieved with the existing error handling mechanism and a suitable Servlet / JSP as the target for the error reports.

[maxfortun]

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.

Changed the wording.

2. Each valve is configured using an external properties file instead of within server.xml like valves are typically configured.

The idea here is to be able to provide different resource bundles based on the locale of the requests. French errors may looks somewhat different from English ones. Do you believe It is a requirement to configure valves only using their attributes?

3. The documentation (XML) disagrees with the implementation, including documenting 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.

The documented attributes are not for the valve, but for the resource bundle. I believe that what the text states. How do you suggest I reword this to avoid confusion?

4. Trace log statement(s) is/are not guarded by e.g. log.isTraceEnabled

Added

5. The URL parameters are not consistently sanitized using URLEncoder.encode

Added

6. There doesn't seem to be a reason to prohibit URL parameters from the "base URL" for each valve

Added support for arbitrary url params.

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

Added content-length

2. This valve will fail if the request has already been using a Writer
3. This valve will fail if url.openStream returns null

Upon any failure valves return handling to ErrorReportValve.

4. Performance: Please use StringBuilder instead of string-concatenation when possible

Changed

[maxfortun]

This feels more like a custom ErrorReportValve rather than something that would ship with Tomcat. I do agree with Chris that a lot (all?) of this functionality could be achieved with the existing error handling mechanism and a suitable Servlet / JSP as the target for the error reports.

@markt-asf You are right. This is a Custom error reporting valve. We are deploying this as a standalone artifact within our tomcat distribution. It is a small and a rather generic use-case that others may benefit from. This is the only reason I am contributing this.

As far as leveraging an existing error handling mechanism to achieve the same.. I spent a few hours searching github, stack overflow, and google for examples of delegating error reporting to something outside of an application. I was hoping to find something that would intercept an unhandled error and forward it to /ROOT/error.jsp, or some thing like that. I couldn't find anything meaningful. Could you please suggest an example of how I may achieve this "with the existing error handling mechanism"?

Thank you.

[markt-asf]

This would be per web application but if you put the following in web.xml

<error-page>
  <location>/WEB-INF/default-error-handler.jsp</location>
</error-page>

you should get close to what you need. That page can forward elsewhere if necessary.

(The error page is placed under WEB-INF so it isn't directly accessible)

[maxfortun]

@markt-asf I guess you are right and the JSP can do the same proxying as the valve to the external url. Still this does not scale well for our needs. We would need to rebuild a few hundred servlets with this approach, where many webapps are legacy ones where build processes no longer work. This is very time-costly and produces the same result as a quick tomcat config change. Just not worth the time for us. Thank you for your ideas.

[michael-o]

I would recommend to make this TWO PRs because both new valves serve different purposes...

[rmaucher]

I had a look at it and ended up merging the feature as 2c2a1bd
We'll see what the final feedback on it is ...