Issue #4334 - Improve testing of ErrorHandler behavior

+ Cleanup from PR review

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>

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

@@ -482,28 +482,26 @@ public class ErrorHandler extends AbstractHandler
         writer.append(json.entrySet().stream()
                 .map(e -> QuotedStringTokenizer.quote(e.getKey()) +
                         ":" +
-                        QuotedStringTokenizer.quote((e.getValue())))
+                    QuotedStringTokenizer.quote(StringUtil.sanitizeXmlString((e.getValue()))))
                 .collect(Collectors.joining(",\n", "{\n", "\n}")));
-
-
     }
 
     protected void writeErrorPageStacks(HttpServletRequest request, Writer writer)
         throws IOException
     {
         Throwable th = (Throwable)request.getAttribute(RequestDispatcher.ERROR_EXCEPTION);
-        while (th != null)
+        if (th != null)
         {
             writer.write("<h3>Caused by:</h3><pre>");
             // You have to pre-generate and then use #write(writer, String)
-            StringWriter sw = new StringWriter();
+            try (StringWriter sw = new StringWriter();
-            PrintWriter pw = new PrintWriter(sw);
+                 PrintWriter pw = new PrintWriter(sw))
-            th.printStackTrace(pw);
+            {
-            pw.flush();
+                th.printStackTrace(pw);
-            write(writer, sw.getBuffer().toString()); // IMPORTANT STEP
+                pw.flush();
+                write(writer, sw.getBuffer().toString()); // sanitize
+            }
             writer.write("</pre>\n");
-
-            th = th.getCause();
         }
     }
 

jetty-server/src/test/java/org/eclipse/jetty/server/ErrorHandlerTest.java

@@ -31,6 +31,7 @@ import org.eclipse.jetty.http.HttpHeader;
 import org.eclipse.jetty.http.HttpTester;
 import org.eclipse.jetty.server.handler.AbstractHandler;
 import org.eclipse.jetty.util.ajax.JSON;
+import org.eclipse.jetty.util.log.StacklessLogging;
 import org.junit.jupiter.api.AfterAll;
 import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.Test;
@@ -38,6 +39,7 @@ import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.ValueSource;
 
 import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.anyOf;
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.greaterThan;
 import static org.hamcrest.Matchers.is;
@@ -47,12 +49,14 @@ import static org.hamcrest.Matchers.nullValue;
 
 public class ErrorHandlerTest
 {
+    static StacklessLogging stacklessLogging;
     static Server server;
     static LocalConnector connector;
 
     @BeforeAll
     public static void before() throws Exception
     {
+        stacklessLogging = new StacklessLogging(HttpChannel.class);
         server = new Server();
         connector = new LocalConnector(server);
         server.addConnector(connector);
@@ -66,7 +70,7 @@ public class ErrorHandlerTest
                 if (baseRequest.getDispatcherType() == DispatcherType.ERROR)
                 {
                     baseRequest.setHandled(true);
-                    response.sendError(((Integer)request.getAttribute(RequestDispatcher.ERROR_STATUS_CODE)).intValue());
+                    response.sendError((Integer)request.getAttribute(RequestDispatcher.ERROR_STATUS_CODE));
                     return;
                 }
 
@@ -80,43 +84,40 @@ public class ErrorHandlerTest
 
                 if (target.startsWith("/badmessage/"))
                 {
-                    int code = Integer.valueOf(target.substring(target.lastIndexOf('/') + 1));
+                    int code = Integer.parseInt(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();
+                    String message = "{\n \"glossary\": {\n \"title\": \"example\"\n }\n }";
-                    message.append("{\n \"glossary\": {\n \"title\": \"example\"\n }\n }");
+                    throw new ServletException(new RuntimeException(message));
-                    throw new ServletException(new RuntimeException(message.toString()));
                 }
 
                 // produce an exception with an XML cause message
                 if (target.startsWith("/xmlmessage/"))
                 {
-                    StringBuilder message = new StringBuilder();
+                    String message =
-                    message.append("<!DOCTYPE glossary PUBLIC \"-//OASIS//DTD DocBook V3.1//EN\">\n");
+                        "<!DOCTYPE glossary PUBLIC \"-//OASIS//DTD DocBook V3.1//EN\">\n" +
-                    message.append(" <glossary>\n");
+                            " <glossary>\n" +
-                    message.append("  <title>example glossary</title>\n");
+                            "  <title>example glossary</title>\n" +
-                    message.append(" </glossary>");
+                            " </glossary>";
-                    throw new ServletException(new RuntimeException(message.toString()));
+                    throw new ServletException(new RuntimeException(message));
                 }
 
                 // produce an exception with an HTML cause message
                 if (target.startsWith("/htmlmessage/"))
                 {
-                    StringBuilder message = new StringBuilder();
+                    String message = "<hr/><script>alert(42)</script>%3Cscript%3E";
-                    message.append("<hr/><script>alert(42)</script>");
+                    throw new ServletException(new RuntimeException(message));
-                    throw new ServletException(new RuntimeException(message.toString()));
                 }
 
                 // produce an exception with a UTF-8 cause message
                 if (target.startsWith("/utf8message/"))
                 {
-                    StringBuilder message = new StringBuilder();
+                    String message = "Euro is &euro; and \u20AC and %E2%82%AC";
-                    message.append("Euro is &euro; and \u20AC and %E2%82%AC");
+                    throw new ServletException(new RuntimeException(message));
-                    throw new ServletException(new RuntimeException(message.toString()));
                 }
             }
         });
@@ -127,6 +128,7 @@ public class ErrorHandlerTest
     public static void after() throws Exception
     {
         server.stop();
+        stacklessLogging.close();
     }
 
     @Test
@@ -469,12 +471,16 @@ public class ErrorHandlerTest
         }
         else if (contentType.contains("text/json"))
         {
-            Map<Object, Object> 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 class ErrorHandlerTest
         if (path.startsWith("/utf8"))
         {
             // we are expecting UTF-8 output, look for it.
-            assertThat("content", content, containsString("Euro is &euro; and \u20AC and %E2%82%AC"));
+            assertThat("content", content, containsString("Euro is &amp;euro; and \u20AC and %E2%82%AC"));
         }
     }
 }