This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 3a8f5bb23f7329fdb9d16edb3aca13991e0ba1ca Author: opencode <[email protected]> AuthorDate: Mon Oct 12 11:39:52 2026 +0200 Fix silent loss of error reports in ProxyErrorReportValve proxy mode In proxy mode, the target URL is commonly a relative error page location, for example the location of an <error-page> element. Such a URL cannot be converted to a URL for the proxy connection, the resulting exception was logged only at debug level and the client was left with an empty response carrying only the error status: the error report was silently lost. The fallback to the default ErrorReportValve was unreachable in this case because the one-shot reported flag had already been consumed before the proxy attempt. Rework the handling of the reported flag so it is consumed only when the report is actually produced. The proxy path now falls back to the default error report whenever proxying fails, which also covers unreachable targets, targets that respond with an error status and non-HTTP target URLs. Only absolute HTTP and HTTPS URLs are proxied, removing a ClassCastException for other protocols, and connect and read timeouts are applied to the proxy connection so an unresponsive target can no longer block the request thread indefinitely. Also obtain the target response before applying target metadata to the current response and clear a stale Content-Length before falling back, so a failure after the target connection was opened can no longer truncate or suppress the fallback report, and log proxy failures at warn level rather than debug level. Add tests for the relative location, unreachable target, unsupported protocol and target error status cases, and document the proxy mode URL requirements, the fallback behaviour and the timeouts. --- .../catalina/valves/ProxyErrorReportValve.java | 61 +++++-- .../catalina/valves/TestProxyErrorReportValve.java | 178 +++++++++++++++++++++ webapps/docs/changelog.xml | 11 ++ webapps/docs/config/valve.xml | 7 + 4 files changed, 244 insertions(+), 13 deletions(-) diff --git a/java/org/apache/catalina/valves/ProxyErrorReportValve.java b/java/org/apache/catalina/valves/ProxyErrorReportValve.java index 75ed77f3a5..97b63e8221 100644 --- a/java/org/apache/catalina/valves/ProxyErrorReportValve.java +++ b/java/org/apache/catalina/valves/ProxyErrorReportValve.java @@ -50,6 +50,13 @@ import org.apache.tomcat.util.res.StringManager; public class ProxyErrorReportValve extends ErrorReportValve { private static final Log log = LogFactory.getLog(ProxyErrorReportValve.class); + /* + * Timeouts for the connection used when proxying an error report. These bound the time the request thread can be + * blocked by an unresponsive error report target. + */ + private static final int PROXY_CONNECT_TIMEOUT_MS = 5000; + private static final int PROXY_READ_TIMEOUT_MS = 10000; + /** * Default constructor. */ @@ -163,8 +170,11 @@ public class ProxyErrorReportValve extends ErrorReportValve { return; } - // No need to delegate anymore - if (!response.setErrorReported()) { + // If the error has already been reported, there is nothing more to do. + // Note: the reported flag is deliberately not consumed here. A failed + // proxy attempt must still be able to fall back to super.report(), + // which consumes the flag itself. + if (!response.isErrorReportRequired()) { return; } @@ -216,6 +226,10 @@ public class ProxyErrorReportValve extends ErrorReportValve { if (log.isTraceEnabled()) { log.trace("Redirecting error reporting to " + urlString); } + // No need to delegate anymore + if (!response.setErrorReported()) { + return; + } try { response.sendRedirect(urlString); } catch (IOException ioe) { @@ -226,25 +240,46 @@ public class ProxyErrorReportValve extends ErrorReportValve { log.trace("Proxying error reporting to " + urlString); } HttpURLConnection httpURLConnection = null; + boolean proxied = false; + Exception failure = null; try { URL url = (new URI(urlString)).toURL(); - httpURLConnection = (HttpURLConnection) url.openConnection(); - httpURLConnection.connect(); - response.setContentType(httpURLConnection.getContentType()); - response.setContentLength(httpURLConnection.getContentLength()); - OutputStream outputStream = response.getOutputStream(); - InputStream inputStream = httpURLConnection.getInputStream(); - IOTools.flow(inputStream, outputStream); - } catch (URISyntaxException | IOException | IllegalArgumentException e) { - if (log.isDebugEnabled()) { - log.debug(sm.getString("proxyErrorReportValve.error", urlString), e); + String protocol = url.getProtocol(); + if ("http".equalsIgnoreCase(protocol) || "https".equalsIgnoreCase(protocol)) { + httpURLConnection = (HttpURLConnection) url.openConnection(); + httpURLConnection.setConnectTimeout(PROXY_CONNECT_TIMEOUT_MS); + httpURLConnection.setReadTimeout(PROXY_READ_TIMEOUT_MS); + httpURLConnection.connect(); + // Obtain the target response before modifying this response. Otherwise, a failure after the + // connection was opened would leave the target's metadata on this response, corrupting the + // fallback error report produced below. + InputStream inputStream = httpURLConnection.getInputStream(); + response.setContentType(httpURLConnection.getContentType()); + response.setContentLength(httpURLConnection.getContentLength()); + OutputStream outputStream = response.getOutputStream(); + IOTools.flow(inputStream, outputStream); + proxied = true; } - // Ignore + } catch (URISyntaxException | IOException | IllegalArgumentException | IllegalStateException e) { + failure = e; } finally { if (httpURLConnection != null) { httpURLConnection.disconnect(); } } + if (proxied) { + // No need to delegate anymore + response.setErrorReported(); + } else { + if (log.isWarnEnabled()) { + log.warn(sm.getString("proxyErrorReportValve.error", urlString), failure); + } + // Discard any target metadata that was applied to this response before the failure, so that a stale + // Content-Length cannot truncate or suppress the fallback report. Fall back to the default error + // report rather than returning an empty response. The reported flag is consumed by super.report(). + response.setContentLength(-1); + super.report(request, response, throwable); + } } } } diff --git a/test/org/apache/catalina/valves/TestProxyErrorReportValve.java b/test/org/apache/catalina/valves/TestProxyErrorReportValve.java index 416ba915b7..bbd10da136 100644 --- a/test/org/apache/catalina/valves/TestProxyErrorReportValve.java +++ b/test/org/apache/catalina/valves/TestProxyErrorReportValve.java @@ -18,6 +18,7 @@ package org.apache.catalina.valves; import java.io.IOException; import java.io.Serial; +import java.net.ServerSocket; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -336,6 +337,183 @@ public class TestProxyErrorReportValve extends TomcatBaseTest { } + @Test + public void testProxyModeRelativeLocationFallsBackToSuper() throws Exception { + Tomcat tomcat = getTomcatInstance(); + StandardHost host = (StandardHost) tomcat.getHost(); + host.setErrorReportValveClass(PROXY_VALVE); + + Context ctx = getProgrammaticRootContext(); + + Tomcat.addServlet(ctx, "error", new SendErrorServlet( + HttpServletResponse.SC_NOT_FOUND, "Not found")); + ctx.addServletMapping("/", "error"); + + tomcat.start(); + + ProxyErrorReportValve valve = null; + Valve[] valves = host.getPipeline().getValves(); + for (Valve valveCandidate : valves) { + if (PROXY_VALVE.equals(valveCandidate.getClass().getName())) { + valve = (ProxyErrorReportValve)valveCandidate; + break; + } + } + Assert.assertNotNull(valve); + valve.setUseRedirect(false); + // Relative locations cannot be proxied. The default error report must + // be produced rather than an empty response. + valve.setProperty("errorCode." + HttpServletResponse.SC_NOT_FOUND, "/error-page"); + + ByteChunk res = new ByteChunk(); + int rc = getUrl("http://localhost:"; + getPort(), res, null); + + Assert.assertEquals(HttpServletResponse.SC_NOT_FOUND, rc); + + String body = res.toString(); + Assert.assertNotNull("Response body should not be empty", body); + Assert.assertTrue("Should contain HTML error report", + body.contains("html") && + body.contains(String.valueOf(HttpServletResponse.SC_NOT_FOUND))); + } + + + @Test + public void testProxyModeUnreachableTargetFallsBackToSuper() throws Exception { + Tomcat tomcat = getTomcatInstance(); + StandardHost host = (StandardHost) tomcat.getHost(); + host.setErrorReportValveClass(PROXY_VALVE); + + Context ctx = getProgrammaticRootContext(); + + Tomcat.addServlet(ctx, "error", new SendErrorServlet( + HttpServletResponse.SC_NOT_FOUND, "Not found")); + ctx.addServletMapping("/", "error"); + + // Find a port that is not in use + int deadPort; + try (ServerSocket listener = new ServerSocket(0)) { + deadPort = listener.getLocalPort(); + } + + tomcat.start(); + + ProxyErrorReportValve valve = null; + Valve[] valves = host.getPipeline().getValves(); + for (Valve valveCandidate : valves) { + if (PROXY_VALVE.equals(valveCandidate.getClass().getName())) { + valve = (ProxyErrorReportValve)valveCandidate; + break; + } + } + Assert.assertNotNull(valve); + valve.setUseRedirect(false); + // An unreachable proxy target must not result in an empty response. + valve.setProperty("errorCode." + HttpServletResponse.SC_NOT_FOUND, + "http://localhost:"; + deadPort + "/error-page"); + + ByteChunk res = new ByteChunk(); + int rc = getUrl("http://localhost:"; + getPort(), res, null); + + Assert.assertEquals(HttpServletResponse.SC_NOT_FOUND, rc); + + String body = res.toString(); + Assert.assertNotNull("Response body should not be empty", body); + Assert.assertTrue("Should contain HTML error report", + body.contains("html") && + body.contains(String.valueOf(HttpServletResponse.SC_NOT_FOUND))); + } + + + @Test + public void testProxyModeUnsupportedProtocolFallsBackToSuper() throws Exception { + Tomcat tomcat = getTomcatInstance(); + StandardHost host = (StandardHost) tomcat.getHost(); + host.setErrorReportValveClass(PROXY_VALVE); + + Context ctx = getProgrammaticRootContext(); + + Tomcat.addServlet(ctx, "error", new SendErrorServlet( + HttpServletResponse.SC_NOT_FOUND, "Not found")); + ctx.addServletMapping("/", "error"); + + tomcat.start(); + + ProxyErrorReportValve valve = null; + Valve[] valves = host.getPipeline().getValves(); + for (Valve valveCandidate : valves) { + if (PROXY_VALVE.equals(valveCandidate.getClass().getName())) { + valve = (ProxyErrorReportValve)valveCandidate; + break; + } + } + Assert.assertNotNull(valve); + valve.setUseRedirect(false); + // Only http and https URLs can be proxied. The default error report + // must be produced rather than an empty response. + valve.setProperty("errorCode." + HttpServletResponse.SC_NOT_FOUND, + "file:///tmp/does-not-exist"); + + ByteChunk res = new ByteChunk(); + int rc = getUrl("http://localhost:"; + getPort(), res, null); + + Assert.assertEquals(HttpServletResponse.SC_NOT_FOUND, rc); + + String body = res.toString(); + Assert.assertNotNull("Response body should not be empty", body); + Assert.assertTrue("Should contain HTML error report", + body.contains("html") && + body.contains(String.valueOf(HttpServletResponse.SC_NOT_FOUND))); + } + + + @Test + public void testProxyModeTargetErrorStatusFallsBackToSuper() throws Exception { + Tomcat tomcat = getTomcatInstance(); + StandardHost host = (StandardHost) tomcat.getHost(); + host.setErrorReportValveClass(PROXY_VALVE); + + Context ctx = getProgrammaticRootContext(); + + Tomcat.addServlet(ctx, "error", new SendErrorServlet( + HttpServletResponse.SC_NOT_FOUND, "Not found")); + ctx.addServletMapping("/", "error"); + + // The proxy target itself responds with an error status + Tomcat.addServlet(ctx, "failingErrorPage", new SendErrorServlet( + HttpServletResponse.SC_INTERNAL_SERVER_ERROR, "Target broke")); + ctx.addServletMapping("/error-page", "failingErrorPage"); + + tomcat.start(); + + ProxyErrorReportValve valve = null; + Valve[] valves = host.getPipeline().getValves(); + for (Valve valveCandidate : valves) { + if (PROXY_VALVE.equals(valveCandidate.getClass().getName())) { + valve = (ProxyErrorReportValve)valveCandidate; + break; + } + } + Assert.assertNotNull(valve); + valve.setUseRedirect(false); + valve.setProperty("errorCode." + HttpServletResponse.SC_NOT_FOUND, + "http://localhost:"; + getPort() + "/error-page"); + + ByteChunk res = new ByteChunk(); + int rc = getUrl("http://localhost:"; + getPort(), res, null); + + Assert.assertEquals(HttpServletResponse.SC_NOT_FOUND, rc); + + // A target that responds with an error status cannot be proxied. The + // default error report must not be corrupted by target metadata. + String body = res.toString(); + Assert.assertNotNull("Response body should not be empty", body); + Assert.assertTrue("Should contain HTML error report", + body.contains("html") && + body.contains(String.valueOf(HttpServletResponse.SC_NOT_FOUND))); + } + + private static final class SendErrorServlet extends HttpServlet { @Serial diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index 25d41f6fa0..84deed3283 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -347,6 +347,17 @@ <code>throwOnFailure</code>, so the now reachable skip path cannot silently drop a customised configuration. (remm) </fix> + <fix> + Correctly handle error reports that cannot be proxied by + <code>ProxyErrorReportValve</code>. Previously, a relative location + or an unreachable, non-HTTP or failing proxy target resulted in an + empty response with only the error status set. The valve now falls + back to the error report of the default <code>ErrorReportValve</code> + in these cases, no longer applies proxy target metadata to the + response before the target response has been received, restricts + proxying to absolute HTTP and HTTPS URLs and applies connect and + read timeouts to the connection to the proxy target. (remm) + </fix> </changelog> </subsection> <subsection name="Coyote"> diff --git a/webapps/docs/config/valve.xml b/webapps/docs/config/valve.xml index 08435424f3..a1aade158f 100644 --- a/webapps/docs/config/valve.xml +++ b/webapps/docs/config/valve.xml @@ -2413,6 +2413,13 @@ RequestHeader unset SSL_CLIENT_ESCAPED_CERT</source> key named <code>0</code> should be used to match any other unmapped code to a redirect or proxy URL.</p> + <p>When proxying (as opposed to redirecting), the base URL must be an + absolute HTTP or HTTPS URL. If the error report cannot be proxied, for + example because the URL is relative or the target is unreachable, the + valve falls back to the error report generated by the default + <code>ErrorReportValve</code>. Connections used for proxying are subject + to fixed connect (5 second) and read (10 second) timeouts.</p> + </subsection> </section> --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
