From 9e40fc9a6fbe93850233b7f03f048b1272530942 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Wed, 20 Nov 2019 09:47:33 -0600 Subject: [PATCH 1/2] Issue #4334 - Improve testing of ErrorHandler behavior Signed-off-by: Joakim Erdfelt --- .../jetty/server/handler/ErrorHandler.java | 19 +- .../jetty/server/ErrorHandlerTest.java | 340 ++++++++++++++---- 2 files changed, 283 insertions(+), 76 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ErrorHandler.java b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ErrorHandler.java index 4cc5389b1b2d..ed044c333fdc 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ErrorHandler.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ErrorHandler.java @@ -21,6 +21,7 @@ import java.io.IOException; import java.io.OutputStreamWriter; import java.io.PrintWriter; +import java.io.StringWriter; import java.io.Writer; import java.nio.BufferOverflowException; import java.nio.ByteBuffer; @@ -491,16 +492,18 @@ protected void writeErrorPageStacks(HttpServletRequest request, Writer writer) throws IOException { Throwable th = (Throwable)request.getAttribute(RequestDispatcher.ERROR_EXCEPTION); - if (_showStacks && th != null) + while (th != null) { - PrintWriter pw = writer instanceof PrintWriter ? (PrintWriter)writer : new PrintWriter(writer); - pw.write("
");
-            while (th != null)
-            {
-                th.printStackTrace(pw);
-                th = th.getCause();
-            }
+            writer.write("
Caused by:
");
+            // You have to pre-generate and then use #write(writer, String)
+            StringWriter sw = new StringWriter();
+            PrintWriter pw = new PrintWriter(sw);
+            th.printStackTrace(pw);
+            pw.flush();
+            write(writer, sw.getBuffer().toString()); // IMPORTANT STEP
             writer.write("
\n");
+
+            th = th.getCause();
         }
     }
 
diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/ErrorHandlerTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/ErrorHandlerTest.java
index dcc30b1ff727..97a7f736b40c 100644
--- a/jetty-server/src/test/java/org/eclipse/jetty/server/ErrorHandlerTest.java
+++ b/jetty-server/src/test/java/org/eclipse/jetty/server/ErrorHandlerTest.java
@@ -20,7 +20,6 @@
 
 import java.io.IOException;
 import java.util.Map;
-
 import javax.servlet.DispatcherType;
 import javax.servlet.RequestDispatcher;
 import javax.servlet.ServletException;
@@ -28,7 +27,6 @@
 import javax.servlet.http.HttpServletResponse;
 
 import org.eclipse.jetty.http.BadMessageException;
-import org.eclipse.jetty.http.HttpField;
 import org.eclipse.jetty.http.HttpHeader;
 import org.eclipse.jetty.http.HttpTester;
 import org.eclipse.jetty.server.handler.AbstractHandler;
@@ -36,13 +34,16 @@
 import org.junit.jupiter.api.AfterAll;
 import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
 
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.greaterThan;
 import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.not;
 import static org.hamcrest.Matchers.notNullValue;
-import static org.hamcrest.Matchers.startsWith;
+import static org.hamcrest.Matchers.nullValue;
 
 public class ErrorHandlerTest
 {
@@ -79,7 +80,43 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques
 
                 if (target.startsWith("/badmessage/"))
                 {
-                    throw new ServletException(new BadMessageException(Integer.valueOf(target.substring(12))));
+                    int code = Integer.valueOf(target.substring(target.lastIndexOf('/') + 1));
+                    throw new ServletException(new BadMessageException(code));
+                }
+
+                // produce an exception with an JSON formatted cause message
+                if (target.startsWith("/jsonmessage/"))
+                {
+                    StringBuilder message = new StringBuilder();
+                    message.append("{\n \"glossary\": {\n \"title\": \"example\"\n }\n }");
+                    throw new ServletException(new RuntimeException(message.toString()));
+                }
+
+                // produce an exception with an XML cause message
+                if (target.startsWith("/xmlmessage/"))
+                {
+                    StringBuilder message = new StringBuilder();
+                    message.append("\n");
+                    message.append(" \n");
+                    message.append("  example glossary\n");
+                    message.append(" ");
+                    throw new ServletException(new RuntimeException(message.toString()));
+                }
+
+                // produce an exception with an HTML cause message
+                if (target.startsWith("/htmlmessage/"))
+                {
+                    StringBuilder message = new StringBuilder();
+                    message.append("
");
+                    throw new ServletException(new RuntimeException(message.toString()));
+                }
+
+                // produce an exception with a UTF-8 cause message
+                if (target.startsWith("/utf8message/"))
+                {
+                    StringBuilder message = new StringBuilder();
+                    message.append("Euro is € and \u20AC and %E2%82%AC");
+                    throw new ServletException(new RuntimeException(message.toString()));
                 }
             }
         });
@@ -95,188 +132,232 @@ public static void after() throws Exception
     @Test
     public void test404NoAccept() throws Exception
     {
-        String response = connector.getResponse(
+        String rawResponse = connector.getResponse(
             "GET / HTTP/1.1\r\n" +
                 "Host: Localhost\r\n" +
                 "\r\n");
 
-        assertThat(response, startsWith("HTTP/1.1 404 "));
-        assertThat(response, not(containsString("Content-Length: 0")));
-        assertThat(response, containsString("Content-Type: text/html;charset=iso-8859-1"));
+        HttpTester.Response response = HttpTester.parseResponse(rawResponse);
+
+        assertThat("Response status code", response.getStatus(), is(404));
+        assertThat("Response Content-Length", response.getField(HttpHeader.CONTENT_LENGTH).getIntValue(), greaterThan(0));
+        assertThat("Response Content-Type", response.get(HttpHeader.CONTENT_TYPE), containsString("text/html;charset=ISO-8859-1"));
+
+        assertContent(response);
     }
 
     @Test
     public void test404EmptyAccept() throws Exception
     {
-        String response = connector.getResponse(
+        String rawResponse = connector.getResponse(
             "GET / HTTP/1.1\r\n" +
                 "Accept: \r\n" +
                 "Host: Localhost\r\n" +
                 "\r\n");
-        assertThat(response, startsWith("HTTP/1.1 404 "));
-        assertThat(response, containsString("Content-Length: 0"));
-        assertThat(response, not(containsString("Content-Type")));
+        HttpTester.Response response = HttpTester.parseResponse(rawResponse);
+
+        assertThat("Response status code", response.getStatus(), is(404));
+        assertThat("Response Content-Length", response.getField(HttpHeader.CONTENT_LENGTH).getIntValue(), is(0));
+        assertThat("Response Content-Type", response.getField(HttpHeader.CONTENT_TYPE), is(nullValue()));
     }
 
     @Test
     public void test404UnAccept() throws Exception
     {
-        String response = connector.getResponse(
+        String rawResponse = connector.getResponse(
             "GET / HTTP/1.1\r\n" +
                 "Accept: text/*;q=0\r\n" +
                 "Host: Localhost\r\n" +
                 "\r\n");
+        HttpTester.Response response = HttpTester.parseResponse(rawResponse);
 
-        assertThat(response, startsWith("HTTP/1.1 404 "));
-        assertThat(response, containsString("Content-Length: 0"));
-        assertThat(response, not(containsString("Content-Type")));
+        assertThat("Response status code", response.getStatus(), is(404));
+        assertThat("Response Content-Length", response.getField(HttpHeader.CONTENT_LENGTH).getIntValue(), is(0));
+        assertThat("Response Content-Type", response.getField(HttpHeader.CONTENT_TYPE), is(nullValue()));
     }
 
     @Test
     public void test404AllAccept() throws Exception
     {
-        String response = connector.getResponse(
+        String rawResponse = connector.getResponse(
             "GET / HTTP/1.1\r\n" +
                 "Host: Localhost\r\n" +
                 "Accept: */*\r\n" +
                 "\r\n");
+        HttpTester.Response response = HttpTester.parseResponse(rawResponse);
+
+        assertThat("Response status code", response.getStatus(), is(404));
+        assertThat("Response Content-Length", response.getField(HttpHeader.CONTENT_LENGTH).getIntValue(), greaterThan(0));
+        assertThat("Response Content-Type", response.get(HttpHeader.CONTENT_TYPE), containsString("text/html;charset=ISO-8859-1"));
 
-        assertThat(response, startsWith("HTTP/1.1 404 "));
-        assertThat(response, not(containsString("Content-Length: 0")));
-        assertThat(response, containsString("Content-Type: text/html;charset=iso-8859-1"));
+        assertContent(response);
     }
 
     @Test
     public void test404HtmlAccept() throws Exception
     {
-        String response = connector.getResponse(
+        String rawResponse = connector.getResponse(
             "GET / HTTP/1.1\r\n" +
                 "Host: Localhost\r\n" +
                 "Accept: text/html\r\n" +
                 "\r\n");
 
-        assertThat(response, startsWith("HTTP/1.1 404 "));
-        assertThat(response, not(containsString("Content-Length: 0")));
-        assertThat(response, containsString("Content-Type: text/html;charset=iso-8859-1"));
+        HttpTester.Response response = HttpTester.parseResponse(rawResponse);
+
+        assertThat("Response status code", response.getStatus(), is(404));
+        assertThat("Response Content-Length", response.getField(HttpHeader.CONTENT_LENGTH).getIntValue(), greaterThan(0));
+        assertThat("Response Content-Type", response.get(HttpHeader.CONTENT_TYPE), containsString("text/html;charset=ISO-8859-1"));
+
+        assertContent(response);
     }
 
     @Test
     public void testMoreSpecificAccept() throws Exception
     {
-        String response = connector.getResponse(
+        String rawResponse = connector.getResponse(
             "GET / HTTP/1.1\r\n" +
                 "Host: Localhost\r\n" +
                 "Accept: text/html, some/other;specific=true\r\n" +
                 "\r\n");
 
-        assertThat(response, startsWith("HTTP/1.1 404 "));
-        assertThat(response, not(containsString("Content-Length: 0")));
-        assertThat(response, containsString("Content-Type: text/html;charset=iso-8859-1"));
+        HttpTester.Response response = HttpTester.parseResponse(rawResponse);
+
+        assertThat("Response status code", response.getStatus(), is(404));
+        assertThat("Response Content-Length", response.getField(HttpHeader.CONTENT_LENGTH).getIntValue(), greaterThan(0));
+        assertThat("Response Content-Type", response.get(HttpHeader.CONTENT_TYPE), containsString("text/html;charset=ISO-8859-1"));
+
+        assertContent(response);
     }
 
     @Test
     public void test404HtmlAcceptAnyCharset() throws Exception
     {
-        String response = connector.getResponse(
+        String rawResponse = connector.getResponse(
             "GET / HTTP/1.1\r\n" +
                 "Host: Localhost\r\n" +
                 "Accept: text/html\r\n" +
                 "Accept-Charset: *\r\n" +
                 "\r\n");
 
-        assertThat(response, startsWith("HTTP/1.1 404 "));
-        assertThat(response, not(containsString("Content-Length: 0")));
-        assertThat(response, containsString("Content-Type: text/html;charset=utf-8"));
+        HttpTester.Response response = HttpTester.parseResponse(rawResponse);
+
+        assertThat("Response status code", response.getStatus(), is(404));
+        assertThat("Response Content-Length", response.getField(HttpHeader.CONTENT_LENGTH).getIntValue(), greaterThan(0));
+        assertThat("Response Content-Type", response.get(HttpHeader.CONTENT_TYPE), containsString("text/html;charset=UTF-8"));
+
+        assertContent(response);
     }
 
     @Test
     public void test404HtmlAcceptUtf8Charset() throws Exception
     {
-        String response = connector.getResponse(
+        String rawResponse = connector.getResponse(
             "GET / HTTP/1.1\r\n" +
                 "Host: Localhost\r\n" +
                 "Accept: text/html\r\n" +
                 "Accept-Charset: utf-8\r\n" +
                 "\r\n");
 
-        assertThat(response, startsWith("HTTP/1.1 404 "));
-        assertThat(response, not(containsString("Content-Length: 0")));
-        assertThat(response, containsString("Content-Type: text/html;charset=utf-8"));
+        HttpTester.Response response = HttpTester.parseResponse(rawResponse);
+
+        assertThat("Response status code", response.getStatus(), is(404));
+        assertThat("Response Content-Length", response.getField(HttpHeader.CONTENT_LENGTH).getIntValue(), greaterThan(0));
+        assertThat("Response Content-Type", response.get(HttpHeader.CONTENT_TYPE), containsString("text/html;charset=UTF-8"));
+
+        assertContent(response);
     }
 
     @Test
     public void test404HtmlAcceptNotUtf8Charset() throws Exception
     {
-        String response = connector.getResponse(
+        String rawResponse = connector.getResponse(
             "GET / HTTP/1.1\r\n" +
                 "Host: Localhost\r\n" +
                 "Accept: text/html\r\n" +
                 "Accept-Charset: utf-8;q=0\r\n" +
                 "\r\n");
 
-        assertThat(response, startsWith("HTTP/1.1 404 "));
-        assertThat(response, not(containsString("Content-Length: 0")));
-        assertThat(response, containsString("Content-Type: text/html;charset=iso-8859-1"));
+        HttpTester.Response response = HttpTester.parseResponse(rawResponse);
+
+        assertThat("Response status code", response.getStatus(), is(404));
+        assertThat("Response Content-Length", response.getField(HttpHeader.CONTENT_LENGTH).getIntValue(), greaterThan(0));
+        assertThat("Response Content-Type", response.get(HttpHeader.CONTENT_TYPE), containsString("text/html;charset=ISO-8859-1"));
+
+        assertContent(response);
     }
 
     @Test
     public void test404HtmlAcceptNotUtf8UnknownCharset() throws Exception
     {
-        String response = connector.getResponse(
+        String rawResponse = connector.getResponse(
             "GET / HTTP/1.1\r\n" +
                 "Host: Localhost\r\n" +
                 "Accept: text/html\r\n" +
                 "Accept-Charset: utf-8;q=0,unknown\r\n" +
                 "\r\n");
 
-        assertThat(response, startsWith("HTTP/1.1 404 "));
-        assertThat(response, containsString("Content-Length: 0"));
-        assertThat(response, not(containsString("Content-Type")));
+        HttpTester.Response response = HttpTester.parseResponse(rawResponse);
+
+        assertThat("Response status code", response.getStatus(), is(404));
+        assertThat("Response Content-Length", response.getField(HttpHeader.CONTENT_LENGTH).getIntValue(), is(0));
+        assertThat("Response Content-Type", response.getField(HttpHeader.CONTENT_TYPE), is(nullValue()));
     }
 
     @Test
     public void test404HtmlAcceptUnknownUtf8Charset() throws Exception
     {
-        String response = connector.getResponse(
+        String rawResponse = connector.getResponse(
             "GET / HTTP/1.1\r\n" +
                 "Host: Localhost\r\n" +
                 "Accept: text/html\r\n" +
                 "Accept-Charset: utf-8;q=0.1,unknown\r\n" +
                 "\r\n");
 
-        assertThat(response, startsWith("HTTP/1.1 404 "));
-        assertThat(response, not(containsString("Content-Length: 0")));
-        assertThat(response, containsString("Content-Type: text/html;charset=utf-8"));
+        HttpTester.Response response = HttpTester.parseResponse(rawResponse);
+
+        assertThat("Response status code", response.getStatus(), is(404));
+        assertThat("Response Content-Length", response.getField(HttpHeader.CONTENT_LENGTH).getIntValue(), greaterThan(0));
+        assertThat("Response Content-Type", response.get(HttpHeader.CONTENT_TYPE), containsString("text/html;charset=UTF-8"));
+
+        assertContent(response);
     }
 
     @Test
     public void test404PreferHtml() throws Exception
     {
-        String response = connector.getResponse(
+        String rawResponse = connector.getResponse(
             "GET / HTTP/1.1\r\n" +
                 "Host: Localhost\r\n" +
                 "Accept: text/html;q=1.0,text/json;q=0.5,*/*\r\n" +
                 "Accept-Charset: *\r\n" +
                 "\r\n");
 
-        assertThat(response, startsWith("HTTP/1.1 404 "));
-        assertThat(response, not(containsString("Content-Length: 0")));
-        assertThat(response, containsString("Content-Type: text/html;charset=utf-8"));
+        HttpTester.Response response = HttpTester.parseResponse(rawResponse);
+
+        assertThat("Response status code", response.getStatus(), is(404));
+        assertThat("Response Content-Length", response.getField(HttpHeader.CONTENT_LENGTH).getIntValue(), greaterThan(0));
+        assertThat("Response Content-Type", response.get(HttpHeader.CONTENT_TYPE), containsString("text/html;charset=UTF-8"));
+
+        assertContent(response);
     }
 
     @Test
     public void test404PreferJson() throws Exception
     {
-        String response = connector.getResponse(
+        String rawResponse = connector.getResponse(
             "GET / HTTP/1.1\r\n" +
                 "Host: Localhost\r\n" +
                 "Accept: text/html;q=0.5,text/json;q=1.0,*/*\r\n" +
                 "Accept-Charset: *\r\n" +
                 "\r\n");
 
-        assertThat(response, startsWith("HTTP/1.1 404 "));
-        assertThat(response, not(containsString("Content-Length: 0")));
-        assertThat(response, containsString("Content-Type: text/json"));
+        HttpTester.Response response = HttpTester.parseResponse(rawResponse);
+
+        assertThat("Response status code", response.getStatus(), is(404));
+        assertThat("Response Content-Length", response.getField(HttpHeader.CONTENT_LENGTH).getIntValue(), greaterThan(0));
+        assertThat("Response Content-Type", response.get(HttpHeader.CONTENT_TYPE), containsString("text/json"));
+
+        assertContent(response);
     }
 
     @Test
@@ -287,12 +368,14 @@ public void testCharEncoding() throws Exception
                 "Host: Localhost\r\n" +
                 "Accept: text/plain\r\n" +
                 "\r\n");
+
         HttpTester.Response response = HttpTester.parseResponse(rawResponse);
 
         assertThat("Response status code", response.getStatus(), is(404));
-        HttpField contentType = response.getField(HttpHeader.CONTENT_TYPE);
-        assertThat("Response Content-Type", contentType, is(notNullValue()));
-        assertThat("Response Content-Type value", contentType.getValue(), not(containsString("null")));
+        assertThat("Response Content-Length", response.getField(HttpHeader.CONTENT_LENGTH).getIntValue(), greaterThan(0));
+        assertThat("Response Content-Type", response.get(HttpHeader.CONTENT_TYPE), containsString("text/plain"));
+
+        assertContent(response);
     }
 
     @Test
@@ -302,29 +385,150 @@ public void testBadMessage() throws Exception
             "GET /badmessage/444 HTTP/1.1\r\n" +
                 "Host: Localhost\r\n" +
                 "\r\n");
+
         HttpTester.Response response = HttpTester.parseResponse(rawResponse);
 
         assertThat("Response status code", response.getStatus(), is(444));
+        assertThat("Response Content-Length", response.getField(HttpHeader.CONTENT_LENGTH).getIntValue(), greaterThan(0));
+        assertThat("Response Content-Type", response.get(HttpHeader.CONTENT_TYPE), containsString("text/html;charset=ISO-8859-1"));
+
+        assertContent(response);
+    }
+
+    @ParameterizedTest
+    @ValueSource(strings = {
+        "/jsonmessage/",
+        "/xmlmessage/",
+        "/htmlmessage/",
+        "/utf8message/",
+    })
+    public void testComplexCauseMessageNoAcceptHeader(String path) throws Exception
+    {
+        String rawResponse = connector.getResponse(
+            "GET " + path + " HTTP/1.1\r\n" +
+                "Host: Localhost\r\n" +
+                "\r\n");
+
+        HttpTester.Response response = HttpTester.parseResponse(rawResponse);
+
+        assertThat("Response status code", response.getStatus(), is(500));
+        assertThat("Response Content-Length", response.getField(HttpHeader.CONTENT_LENGTH).getIntValue(), greaterThan(0));
+        assertThat("Response Content-Type", response.get(HttpHeader.CONTENT_TYPE), containsString("text/html;charset=ISO-8859-1"));
+
+        String content = assertContent(response);
+
+        if (path.startsWith("/utf8"))
+        {
+            // we are Not expecting UTF-8 output, look for mangled ISO-8859-1 version
+            assertThat("content", content, containsString("Euro is € and ? and %E2%82%AC"));
+        }
+    }
+
+    @ParameterizedTest
+    @ValueSource(strings = {
+        "/jsonmessage/",
+        "/xmlmessage/",
+        "/htmlmessage/",
+        "/utf8message/",
+    })
+    public void testComplexCauseMessageAcceptUtf8Header(String path) throws Exception
+    {
+        String rawResponse = connector.getResponse(
+            "GET " + path + " HTTP/1.1\r\n" +
+                "Host: Localhost\r\n" +
+                "Accept: text/html\r\n" +
+                "Accept-Charset: utf-8\r\n" +
+                "\r\n");
+
+        HttpTester.Response response = HttpTester.parseResponse(rawResponse);
+
+        assertThat("Response status code", response.getStatus(), is(500));
+        assertThat("Response Content-Length", response.getField(HttpHeader.CONTENT_LENGTH).getIntValue(), greaterThan(0));
+        assertThat("Response Content-Type", response.get(HttpHeader.CONTENT_TYPE), containsString("text/html;charset=UTF-8"));
+
+        String content = assertContent(response);
+
+        if (path.startsWith("/utf8"))
+        {
+            // we are Not expecting UTF-8 output, look for mangled ISO-8859-1 version
+            assertThat("content", content, containsString("Euro is € and \u20AC and %E2%82%AC"));
+        }
+    }
+
+    private String assertContent(HttpTester.Response response)
+    {
+        String contentType = response.get(HttpHeader.CONTENT_TYPE);
+        String content = response.getContent();
+
+        if (contentType.contains("text/html"))
+        {
+            assertThat(content, not(containsString("");
-                    throw new ServletException(new RuntimeException(message.toString()));
+                    String message = "
%3Cscript%3E";
+                    throw new ServletException(new RuntimeException(message));
                 }
 
                 // produce an exception with a UTF-8 cause message
                 if (target.startsWith("/utf8message/"))
                 {
-                    StringBuilder message = new StringBuilder();
-                    message.append("Euro is € and \u20AC and %E2%82%AC");
-                    throw new ServletException(new RuntimeException(message.toString()));
+                    String message = "Euro is € and \u20AC and %E2%82%AC";
+                    throw new ServletException(new RuntimeException(message));
                 }
             }
         });
@@ -127,6 +128,7 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques
     public static void after() throws Exception
     {
         server.stop();
+        stacklessLogging.close();
     }
 
     @Test
@@ -469,12 +471,16 @@ private String assertContent(HttpTester.Response response)
         }
         else if (contentType.contains("text/json"))
         {
-            Map jo = (Map)JSON.parse(response.getContent());
+            Map jo = (Map)JSON.parse(response.getContent());
 
             assertThat("url field", jo.get("url"), is(notNullValue()));
             String expectedStatus = String.valueOf(response.getStatus());
             assertThat("status field", jo.get("status"), is(expectedStatus));
-            assertThat("message field", jo.get("message"), is(notNullValue()));
+            String message = (String)jo.get("message");
+            assertThat("message field", message, is(notNullValue()));
+            assertThat("message field", message, anyOf(
+                not(containsString("<")),
+                not(containsString(">"))));
         }
         else if (contentType.contains("text/plain"))
         {
@@ -528,7 +534,7 @@ public void testJsonResponseWorse(String path) throws Exception
         if (path.startsWith("/utf8"))
         {
             // we are expecting UTF-8 output, look for it.
-            assertThat("content", content, containsString("Euro is € and \u20AC and %E2%82%AC"));
+            assertThat("content", content, containsString("Euro is &euro; and \u20AC and %E2%82%AC"));
         }
     }
 }