Improvements and cleanups to ErrorHandler. (#11556)

Defaulted showStacks to false, to reduce false positives reported by penetration testing tools.
Deprecated ErrorHandler.badMessageError(), as it was not invoked anymore.
Implemented HttpStreamOverHTTP[2|3].onBadMessage() that was left as TODO.
Simplified the same code for HTTP/1.
Moved invocation of ComplianceViolation.Listener.onRequestEnd() to HandlerInvoker.completeStream().

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

jetty-core/jetty-http2/jetty-http2-server/src/main/java/org/eclipse/jetty/http2/server/internal/HttpStreamOverHTTP2.java

@@ -138,13 +138,17 @@ public void run()
         catch (Throwable x)
         {
             HttpException httpException = x instanceof HttpException http ? http : new HttpException.RuntimeException(HttpStatus.INTERNAL_SERVER_ERROR_500, x);
-            return () -> onBadMessage(httpException);
+            return onBadMessage(httpException);
         }
     }
 
-    private void onBadMessage(HttpException x)
+    private Runnable onBadMessage(HttpException x)
     {
-        // TODO
+        if (LOG.isDebugEnabled())
+            LOG.debug("badMessage {} {}", this, x);
+
+        Throwable failure = (Throwable)x;
+        return _httpChannel.onFailure(failure);
     }
 
     @Override

jetty-core/jetty-http3/jetty-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpStreamOverHTTP3.java

@@ -127,13 +127,17 @@ public void run()
             if (LOG.isDebugEnabled())
                 LOG.debug("onRequest() failure", x);
             HttpException httpException = x instanceof HttpException http ? http : new HttpException.RuntimeException(HttpStatus.INTERNAL_SERVER_ERROR_500, x);
-            return () -> onBadMessage(httpException);
+            return onBadMessage(httpException);
         }
     }
 
-    private void onBadMessage(HttpException x)
+    private Runnable onBadMessage(HttpException x)
     {
-        // TODO
+        if (LOG.isDebugEnabled())
+            LOG.debug("badMessage {} {}", this, x);
+
+        Throwable failure = (Throwable)x;
+        return httpChannel.onFailure(failure);
     }
 
     @Override

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ErrorHandler.java

@@ -46,7 +46,6 @@
 import org.eclipse.jetty.server.Response;
 import org.eclipse.jetty.server.Server;
 import org.eclipse.jetty.util.Attributes;
-import org.eclipse.jetty.util.BufferUtil;
 import org.eclipse.jetty.util.Callback;
 import org.eclipse.jetty.util.StringUtil;
 import org.eclipse.jetty.util.annotation.ManagedAttribute;
@@ -63,7 +62,6 @@
 @ManagedObject
 public class ErrorHandler implements Request.Handler
 {
-    // TODO This classes API needs to be majorly refactored/cleanup in jetty-10
     private static final Logger LOG = LoggerFactory.getLogger(ErrorHandler.class);
     public static final String ERROR_STATUS = "org.eclipse.jetty.server.error_status";
     public static final String ERROR_MESSAGE = "org.eclipse.jetty.server.error_message";
@@ -72,7 +70,7 @@ public class ErrorHandler implements Request.Handler
     public static final Set<String> ERROR_METHODS = Set.of("GET", "POST", "HEAD");
     public static final HttpField ERROR_CACHE_CONTROL = new PreEncodedHttpField(HttpHeader.CACHE_CONTROL, "must-revalidate,no-cache,no-store");
 
-    boolean _showStacks = true;
+    boolean _showStacks = false;
     boolean _showMessageInTitle = true;
     String _defaultResponseMimeType = Type.TEXT_HTML.asString();
     HttpField _cacheControl = new PreEncodedHttpField(HttpHeader.CACHE_CONTROL, "must-revalidate,no-cache,no-store");
@@ -230,7 +228,7 @@ else if (charsets.contains(StandardCharsets.ISO_8859_1))
                     if (showStacks)
                     {
                         if (LOG.isDebugEnabled())
-                            LOG.debug("Disable stacks for " + e.toString());
+                            LOG.debug("Disable stacks for " + e);
 
                         showStacks = false;
                         continue;
@@ -414,15 +412,12 @@ protected void writeErrorHtmlStacks(Request request, Writer writer) throws IOExc
      * @param reason The reason for the error code (may be null)
      * @param fields The header fields that will be sent with the response.
      * @return The content as a ByteBuffer, or null for no body.
+     * @deprecated Do not override. No longer invoked by Jetty.
      */
+    @Deprecated(since = "12.0.8", forRemoval = true)
     public ByteBuffer badMessageError(int status, String reason, HttpFields.Mutable fields)
     {
-        if (reason == null)
-            reason = HttpStatus.getMessage(status);
-        if (HttpStatus.hasNoBody(status))
-            return BufferUtil.EMPTY_BUFFER;
-        fields.put(HttpHeader.CONTENT_TYPE, Type.TEXT_HTML_8859_1.asString());
-        return BufferUtil.toBuffer("<h1>Bad Message " + status + "</h1><pre>reason: " + reason + "</pre>");
+        return null;
     }
 
     /**
@@ -492,7 +487,7 @@ public String getDefaultResponseMimeType()
      */
     public void setDefaultResponseMimeType(String defaultResponseMimeType)
     {
-        _defaultResponseMimeType = Objects.requireNonNull(defaultResponseMimeType);;
+        _defaultResponseMimeType = Objects.requireNonNull(defaultResponseMimeType);
     }
 
     protected void write(Writer writer, String string) throws IOException

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java

@@ -408,7 +408,7 @@ public Runnable onFailure(Throwable x)
             {
                 // If the channel doesn't have a request, then the error must have occurred during the parsing of
                 // the request line / headers, so make a temp request for logging and producing an error response.
-                MetaData.Request errorRequest = new MetaData.Request("GET", HttpURI.from("/"), HttpVersion.HTTP_1_0, HttpFields.EMPTY);
+                MetaData.Request errorRequest = new MetaData.Request("GET", HttpURI.from("/badRequest"), HttpVersion.HTTP_1_0, HttpFields.EMPTY);
                 _request = new ChannelRequest(this, errorRequest);
                 _response = new ChannelResponse(_request);
             }
@@ -719,6 +719,10 @@ private void completeStream(HttpStream stream, Throwable failure)
             }
             finally
             {
+                ComplianceViolation.Listener listener = getComplianceViolationListener();
+                if (listener != null)
+                    listener.onRequestEnd(_request);
+
                 // This is THE ONLY PLACE the stream is succeeded or failed.
                 if (failure == null)
                     stream.succeeded();

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java

@@ -1012,8 +1012,6 @@ public void badMessage(HttpException failure)
             if (LOG.isDebugEnabled())
                 LOG.debug("badMessage {} {}", HttpConnection.this, failure);
 
-            getHttpChannel().getComplianceViolationListener().onRequestEnd(getHttpChannel().getRequest());
-
             _failure = (Throwable)failure;
             _generator.setPersistent(false);
 
@@ -1025,6 +1023,9 @@ public void badMessage(HttpException failure)
                 _httpChannel.setHttpStream(stream);
             }
 
+            // If there is no request, build one temporarily to handle the error.
+            // This is also done by HttpChannel.onFailure(), but here we can build
+            // a request with more information, such as the method, the URI, etc.
             if (_httpChannel.getRequest() == null)
             {
                 HttpURI uri = stream._uri;