From 2ce0e8508a9c0205b49a70955366b2981dd5f919 Mon Sep 17 00:00:00 2001
From: Joakim Erdfelt <joakim.erdfelt@gmail.com>
Date: Wed, 13 Sep 2023 10:34:13 -0500
Subject: [PATCH 1/2] Refactor ResponseHeadersTest to modern standards

---
 .../ee10/servlet/ResponseHeadersTest.java     | 282 +++++++++---------
 1 file changed, 148 insertions(+), 134 deletions(-)

diff --git a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ResponseHeadersTest.java b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ResponseHeadersTest.java
index 55e504efd68b..8b1ac62da6a3 100644
--- a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ResponseHeadersTest.java
+++ b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ResponseHeadersTest.java
@@ -29,8 +29,8 @@
 import org.eclipse.jetty.server.Server;
 import org.eclipse.jetty.util.StringUtil;
 import org.eclipse.jetty.util.URIUtil;
-import org.junit.jupiter.api.AfterAll;
-import org.junit.jupiter.api.BeforeAll;
+import org.eclipse.jetty.util.component.LifeCycle;
+import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.Test;
 
 import static org.hamcrest.MatcherAssert.assertThat;
@@ -39,153 +39,46 @@
 
 public class ResponseHeadersTest
 {
-    public static class SimulateUpgradeServlet extends HttpServlet
-    {
-        @Override
-        protected void doGet(HttpServletRequest req, HttpServletResponse response) throws ServletException, IOException
-        {
-            response.setHeader("Upgrade", "WebSocket");
-            response.addHeader("Connection", "Upgrade");
-            response.addHeader("Sec-WebSocket-Accept", "123456789==");
-
-            response.setStatus(HttpServletResponse.SC_SWITCHING_PROTOCOLS);
-        }
-    }
-
-    public static class MultilineResponseValueServlet extends HttpServlet
-    {
-        @Override
-        protected void doGet(HttpServletRequest req, HttpServletResponse response) throws ServletException, IOException
-        {
-            // The bad use-case
-            String pathInfo = URIUtil.decodePath(req.getPathInfo());
-            if (pathInfo != null && pathInfo.length() > 1 && pathInfo.startsWith("/"))
-            {
-                pathInfo = pathInfo.substring(1);
-            }
-            response.setHeader("X-example", pathInfo);
-
-            // The correct use
-            response.setContentType("text/plain");
-            response.setCharacterEncoding("utf-8");
-            response.getWriter().println("Got request uri - " + req.getRequestURI());
-        }
-    }
-
-    public static class CharsetResetToJsonMimeTypeServlet extends HttpServlet
-    {
-        @Override
-        protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException
-        {
-            // We set an initial desired behavior.
-            response.setContentType("text/html; charset=US-ASCII");
-            PrintWriter writer = response.getWriter();
-
-            // We reset the response, as we don't want it to be HTML anymore.
-            response.reset();
-
-            // switch to json operation
-            // The use of application/json is always assumed to be UTF-8
-            // and should never have a `charset=` entry on the `Content-Type` response header
-            response.setContentType("application/json");
-            writer.println("{ \"what\": \"should this be?\" }");
-        }
-    }
-
-    public static class CharsetChangeToJsonMimeTypeServlet extends HttpServlet
-    {
-        @Override
-        protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException
-        {
-            // We set an initial desired behavior.
-            response.setContentType("text/html; charset=US-ASCII");
-
-            // switch to json behavior.
-            // The use of application/json is always assumed to be UTF-8
-            // and should never have a `charset=` entry on the `Content-Type` response header
-            response.setContentType("application/json");
+    private Server server;
+    private LocalConnector connector;
 
-            PrintWriter writer = response.getWriter();
-            writer.println("{ \"what\": \"should this be?\" }");
-        }
-    }
-
-    public static class CharsetChangeToJsonMimeTypeSetCharsetToNullServlet extends HttpServlet
-    {
-        @Override
-        protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException
-        {
-            // We set an initial desired behavior.
-
-            response.setContentType("text/html; charset=us-ascii");
-            PrintWriter writer = response.getWriter();
-
-            // switch to json behavior.
-            // The use of application/json is always assumed to be UTF-8
-            // and should never have a `charset=` entry on the `Content-Type` response header
-            response.setContentType("application/json");
-            // attempt to indicate that there is truly no charset meant to be used in the response header
-            response.setCharacterEncoding(null);
-
-            writer.println("{ \"what\": \"should this be?\" }");
-        }
-    }
-
-    public static class FlushResponseServlet extends HttpServlet
-    {
-        @Override
-        protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException
-        {
-            PrintWriter writer = response.getWriter();
-            writer.println("Hello");
-            writer.flush();
-            if (!response.isCommitted())
-                throw new IllegalStateException();
-            writer.println("World");
-            writer.close();
-        }
-    }
-
-    private static Server server;
-    private static LocalConnector connector;
-
-    @BeforeAll
-    public static void startServer() throws Exception
+    public void startServer(ServletContextHandler contextHandler) throws Exception
     {
         server = new Server();
         connector = new LocalConnector(server);
         server.addConnector(connector);
 
-        ServletContextHandler context = new ServletContextHandler();
-        context.setContextPath("/");
-        server.setHandler(context);
-
-        context.addServlet(new ServletHolder(new SimulateUpgradeServlet()), "/ws/*");
-        context.addServlet(new ServletHolder(new MultilineResponseValueServlet()), "/multiline/*");
-        context.addServlet(CharsetResetToJsonMimeTypeServlet.class, "/charset/json-reset/*");
-        context.addServlet(CharsetChangeToJsonMimeTypeServlet.class, "/charset/json-change/*");
-        context.addServlet(CharsetChangeToJsonMimeTypeSetCharsetToNullServlet.class, "/charset/json-change-null/*");
-        context.addServlet(FlushResponseServlet.class, "/flush/*");
-
+        server.setHandler(contextHandler);
         server.start();
     }
 
-    @AfterAll
-    public static void stopServer()
+    @AfterEach
+    public void stopServer()
     {
-        try
-        {
-            server.stop();
-        }
-        catch (Exception e)
-        {
-            e.printStackTrace(System.err);
-        }
+        LifeCycle.stop(server);
     }
 
     @Test
     public void testResponseWebSocketHeaderFormat() throws Exception
     {
+        ServletContextHandler contextHandler = new ServletContextHandler();
+        contextHandler.setContextPath("/");
+        HttpServlet fakeWsServlet = new HttpServlet()
+        {
+            @Override
+            protected void doGet(HttpServletRequest req, HttpServletResponse response) throws ServletException, IOException
+            {
+                response.setHeader("Upgrade", "WebSocket");
+                response.addHeader("Connection", "Upgrade");
+                response.addHeader("Sec-WebSocket-Accept", "123456789==");
+
+                response.setStatus(HttpServletResponse.SC_SWITCHING_PROTOCOLS);
+            }
+        };
+        contextHandler.addServlet(fakeWsServlet, "/ws/*");
+
+        startServer(contextHandler);
+
         HttpTester.Request request = new HttpTester.Request();
         request.setMethod("GET");
         request.setURI("/ws/");
@@ -204,6 +97,32 @@ public void testResponseWebSocketHeaderFormat() throws Exception
     @Test
     public void testMultilineResponseHeaderValue() throws Exception
     {
+        ServletContextHandler contextHandler = new ServletContextHandler();
+        contextHandler.setContextPath("/");
+
+        HttpServlet multilineServlet = new HttpServlet()
+        {
+            @Override
+            protected void doGet(HttpServletRequest req, HttpServletResponse response) throws ServletException, IOException
+            {
+                // The bad use-case
+                String pathInfo = URIUtil.decodePath(req.getPathInfo());
+                if (pathInfo != null && pathInfo.length() > 1 && pathInfo.startsWith("/"))
+                {
+                    pathInfo = pathInfo.substring(1);
+                }
+                response.setHeader("X-example", pathInfo);
+
+                // The correct use
+                response.setContentType("text/plain");
+                response.setCharacterEncoding("utf-8");
+                response.getWriter().println("Got request uri - " + req.getRequestURI());
+            }
+        };
+
+        contextHandler.addServlet(multilineServlet, "/multiline/*");
+        startServer(contextHandler);
+
         String actualPathInfo = "%0A%20Content-Type%3A%20image/png%0A%20Content-Length%3A%208%0A%20%0A%20yuck<!--";
 
         HttpTester.Request request = new HttpTester.Request();
@@ -230,6 +149,32 @@ public void testMultilineResponseHeaderValue() throws Exception
     @Test
     public void testCharsetResetToJsonMimeType() throws Exception
     {
+        ServletContextHandler contextHandler = new ServletContextHandler();
+        contextHandler.setContextPath("/");
+
+        HttpServlet charsetResetToJsonMimeTypeServlet = new HttpServlet()
+        {
+            @Override
+            protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException
+            {
+                // We set an initial desired behavior.
+                response.setContentType("text/html; charset=US-ASCII");
+                PrintWriter writer = response.getWriter();
+
+                // We reset the response, as we don't want it to be HTML anymore.
+                response.reset();
+
+                // switch to json operation
+                // The use of application/json is always assumed to be UTF-8
+                // and should never have a `charset=` entry on the `Content-Type` response header
+                response.setContentType("application/json");
+                writer.println("{ \"what\": \"should this be?\" }");
+            }
+        };
+
+        contextHandler.addServlet(charsetResetToJsonMimeTypeServlet, "/charset/json-reset/*");
+        startServer(contextHandler);
+
         HttpTester.Request request = new HttpTester.Request();
         request.setMethod("GET");
         request.setURI("/charset/json-reset/");
@@ -250,6 +195,29 @@ public void testCharsetResetToJsonMimeType() throws Exception
     @Test
     public void testCharsetChangeToJsonMimeType() throws Exception
     {
+        ServletContextHandler contextHandler = new ServletContextHandler();
+        contextHandler.setContextPath("/");
+        HttpServlet charsetChangeToJsonMimeTypeServlet = new HttpServlet()
+        {
+            @Override
+            protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException
+            {
+                // We set an initial desired behavior.
+                response.setContentType("text/html; charset=US-ASCII");
+
+                // switch to json behavior.
+                // The use of application/json is always assumed to be UTF-8
+                // and should never have a `charset=` entry on the `Content-Type` response header
+                response.setContentType("application/json");
+
+                PrintWriter writer = response.getWriter();
+                writer.println("{ \"what\": \"should this be?\" }");
+            }
+        };
+
+        contextHandler.addServlet(charsetChangeToJsonMimeTypeServlet, "/charset/json-change/*");
+        startServer(contextHandler);
+
         HttpTester.Request request = new HttpTester.Request();
         request.setMethod("GET");
         request.setURI("/charset/json-change/");
@@ -270,6 +238,32 @@ public void testCharsetChangeToJsonMimeType() throws Exception
     @Test
     public void testCharsetChangeToJsonMimeTypeSetCharsetToNull() throws Exception
     {
+        ServletContextHandler contextHandler = new ServletContextHandler();
+        contextHandler.setContextPath("/");
+        HttpServlet charsetChangeToJsonMimeTypeSetCharsetToNullServlet = new HttpServlet()
+        {
+            @Override
+            protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException
+            {
+                // We set an initial desired behavior.
+
+                response.setContentType("text/html; charset=us-ascii");
+                PrintWriter writer = response.getWriter();
+
+                // switch to json behavior.
+                // The use of application/json is always assumed to be UTF-8
+                // and should never have a `charset=` entry on the `Content-Type` response header
+                response.setContentType("application/json");
+                // attempt to indicate that there is truly no charset meant to be used in the response header
+                response.setCharacterEncoding(null);
+
+                writer.println("{ \"what\": \"should this be?\" }");
+            }
+        };
+
+        contextHandler.addServlet(charsetChangeToJsonMimeTypeSetCharsetToNullServlet, "/charset/json-change-null/*");
+        startServer(contextHandler);
+
         HttpTester.Request request = new HttpTester.Request();
         request.setMethod("GET");
         request.setURI("/charset/json-change-null/");
@@ -290,6 +284,26 @@ public void testCharsetChangeToJsonMimeTypeSetCharsetToNull() throws Exception
     @Test
     public void testFlushPrintWriter() throws Exception
     {
+        ServletContextHandler contextHandler = new ServletContextHandler();
+        contextHandler.setContextPath("/");
+        HttpServlet flushResponseServlet = new HttpServlet()
+        {
+            @Override
+            protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException
+            {
+                PrintWriter writer = response.getWriter();
+                writer.println("Hello");
+                writer.flush();
+                if (!response.isCommitted())
+                    throw new IllegalStateException();
+                writer.println("World");
+                writer.close();
+            }
+        };
+
+        contextHandler.addServlet(flushResponseServlet, "/flush/*");
+        startServer(contextHandler);
+
         HttpTester.Request request = new HttpTester.Request();
         request.setMethod("GET");
         request.setURI("/flush");

From bec4f33e360e1873775f506b8eae2e0585d55ad1 Mon Sep 17 00:00:00 2001
From: Joakim Erdfelt <joakim.erdfelt@gmail.com>
Date: Wed, 13 Sep 2023 11:00:19 -0500
Subject: [PATCH 2/2] Issue #10508 - honor Servlet spec behaviors for null in
 addHeader / setHeader calls

---
 .../ee10/servlet/ServletApiResponse.java      |  18 ++
 .../ee10/servlet/ResponseHeadersTest.java     | 165 +++++++++++++++++-
 2 files changed, 182 insertions(+), 1 deletion(-)

diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiResponse.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiResponse.java
index c694176b8f75..e5ed3e95b79f 100644
--- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiResponse.java
+++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiResponse.java
@@ -196,30 +196,45 @@ public void sendRedirect(int code, String location) throws IOException
     @Override
     public void setDateHeader(String name, long date)
     {
+        if (name == null)
+            return; // Spec is to do nothing
+
         getResponse().getHeaders().putDate(name, date);
     }
 
     @Override
     public void addDateHeader(String name, long date)
     {
+        if (name == null)
+            return; // Spec is to do nothing
+
         getResponse().getHeaders().addDateField(name, date);
     }
 
     @Override
     public void setHeader(String name, String value)
     {
+        if (name == null)
+            return; // Spec is to do nothing
+
         getResponse().getHeaders().put(name, value);
     }
 
     @Override
     public void addHeader(String name, String value)
     {
+        if (name == null || value == null)
+            return; // Spec is to do nothing
+
         getResponse().getHeaders().add(name, value);
     }
 
     @Override
     public void setIntHeader(String name, int value)
     {
+        if (name == null)
+            return; // Spec is to do nothing
+
         // TODO do we need int versions?
         if (!isCommitted())
             getResponse().getHeaders().put(name, value);
@@ -228,6 +243,9 @@ public void setIntHeader(String name, int value)
     @Override
     public void addIntHeader(String name, int value)
     {
+        if (name == null)
+            return; // Spec is to do nothing
+
         // TODO do we need a native version?
         if (!isCommitted())
             getResponse().getHeaders().add(name, Integer.toString(value));
diff --git a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ResponseHeadersTest.java b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ResponseHeadersTest.java
index 8b1ac62da6a3..1c79ec3f2387 100644
--- a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ResponseHeadersTest.java
+++ b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ResponseHeadersTest.java
@@ -34,8 +34,10 @@
 import org.junit.jupiter.api.Test;
 
 import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.endsWith;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.nullValue;
 
 public class ResponseHeadersTest
 {
@@ -246,7 +248,6 @@ public void testCharsetChangeToJsonMimeTypeSetCharsetToNull() throws Exception
             protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException
             {
                 // We set an initial desired behavior.
-
                 response.setContentType("text/html; charset=us-ascii");
                 PrintWriter writer = response.getWriter();
 
@@ -281,6 +282,168 @@ protected void doGet(HttpServletRequest request, HttpServletResponse response) t
         assertThat("Response Header Content-Type", response.get("Content-Type"), is("application/json"));
     }
 
+    @Test
+    public void testAddSetHeaderNulls() throws Exception
+    {
+        ServletContextHandler contextHandler = new ServletContextHandler();
+        contextHandler.setContextPath("/");
+        HttpServlet addHeaderNullServlet = new HttpServlet()
+        {
+            @Override
+            protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException
+            {
+                // Add the header
+                response.addHeader("X-Foo", "foo-value");
+
+                // Add a null header value (should result in no change, and no exception)
+                response.addHeader("X-Foo", null);
+
+                // Add a null name (should result in no change, and no exception)
+                response.addHeader(null, "bogus");
+
+                // Add a null name and null value (should result in no change, and no exception)
+                response.addHeader(null, null);
+
+                // Set a new header
+                response.setHeader("X-Bar", "bar-value");
+
+                // Set same header with null (should remove header)
+                response.setHeader("X-Bar", null);
+
+                // Set a header with null name (should result in no change, and no exception)
+                response.setHeader(null, "bogus");
+
+                // Set a header with null name and null value (should result in no change, and no exception)
+                response.setHeader(null, null);
+
+                response.setCharacterEncoding("utf-8");
+                response.setContentType("text/plain");
+
+                PrintWriter writer = response.getWriter();
+                writer.println("Done");
+            }
+        };
+
+        contextHandler.addServlet(addHeaderNullServlet, "/add-header-nulls/*");
+        startServer(contextHandler);
+
+        HttpTester.Request request = new HttpTester.Request();
+        request.setMethod("GET");
+        request.setURI("/add-header-nulls/");
+        request.setVersion(HttpVersion.HTTP_1_1);
+        request.setHeader("Connection", "close");
+        request.setHeader("Host", "test");
+
+        ByteBuffer responseBuffer = connector.getResponse(request.generate());
+        // System.err.println(BufferUtil.toUTF8String(responseBuffer));
+        HttpTester.Response response = HttpTester.parseResponse(responseBuffer);
+
+        // Now test for properly formatted HTTP Response Headers.
+        assertThat("Response Code", response.getStatus(), is(200));
+        // The X-Foo header should be present an unchanged
+        assertThat("Response Header X-Foo", response.get("X-Foo"), is("foo-value"));
+        assertThat("Response Header X-Bar should not exist", response.getField("X-Bar"), nullValue());
+    }
+
+    @Test
+    public void testAddSetDateHeaderNulls() throws Exception
+    {
+        ServletContextHandler contextHandler = new ServletContextHandler();
+        contextHandler.setContextPath("/");
+        final long date = System.currentTimeMillis();
+        HttpServlet addHeaderNullServlet = new HttpServlet()
+        {
+            @Override
+            protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException
+            {
+                // Add the header
+                response.addDateHeader("X-Foo", date);
+                String dateStr = response.getHeader("X-Foo"); // get the formatted Date as a String
+
+                // Add a null name (should result in no change, and no exception)
+                response.addDateHeader(null, 123456);
+
+                // Set a null name (should result in no change, and no exception)
+                response.setDateHeader(null, 987654);
+
+                response.setCharacterEncoding("utf-8");
+                response.setContentType("text/plain");
+
+                PrintWriter writer = response.getWriter();
+                writer.println(dateStr);
+            }
+        };
+
+        contextHandler.addServlet(addHeaderNullServlet, "/add-dateheader-nulls/*");
+        startServer(contextHandler);
+
+        HttpTester.Request request = new HttpTester.Request();
+        request.setMethod("GET");
+        request.setURI("/add-dateheader-nulls/");
+        request.setVersion(HttpVersion.HTTP_1_1);
+        request.setHeader("Connection", "close");
+        request.setHeader("Host", "test");
+
+        ByteBuffer responseBuffer = connector.getResponse(request.generate());
+        // System.err.println(BufferUtil.toUTF8String(responseBuffer));
+        HttpTester.Response response = HttpTester.parseResponse(responseBuffer);
+
+        // Now test for properly formatted HTTP Response Headers.
+        assertThat("Response Code", response.getStatus(), is(200));
+        String dateStr = response.getContent().trim();
+        assertThat("Should have seen a Date String", dateStr, endsWith(" GMT"));
+        // The X-Foo header should be present an unchanged
+        assertThat("Response Header X-Foo", response.get("X-Foo"), is(dateStr));
+    }
+
+    @Test
+    public void testAddSetIntHeaderNulls() throws Exception
+    {
+        ServletContextHandler contextHandler = new ServletContextHandler();
+        contextHandler.setContextPath("/");
+        final int foovalue = 22222222;
+        HttpServlet addHeaderNullServlet = new HttpServlet()
+        {
+            @Override
+            protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException
+            {
+                // Add the header
+                response.addIntHeader("X-Foo", foovalue);
+
+                // Add a null name (should result in no change, and no exception)
+                response.addIntHeader(null, 123456);
+
+                // Set a null name (should result in no change, and no exception)
+                response.setIntHeader(null, 987654);
+
+                response.setCharacterEncoding("utf-8");
+                response.setContentType("text/plain");
+
+                PrintWriter writer = response.getWriter();
+                writer.println("Done");
+            }
+        };
+
+        contextHandler.addServlet(addHeaderNullServlet, "/add-intheader-nulls/*");
+        startServer(contextHandler);
+
+        HttpTester.Request request = new HttpTester.Request();
+        request.setMethod("GET");
+        request.setURI("/add-intheader-nulls/");
+        request.setVersion(HttpVersion.HTTP_1_1);
+        request.setHeader("Connection", "close");
+        request.setHeader("Host", "test");
+
+        ByteBuffer responseBuffer = connector.getResponse(request.generate());
+        // System.err.println(BufferUtil.toUTF8String(responseBuffer));
+        HttpTester.Response response = HttpTester.parseResponse(responseBuffer);
+
+        // Now test for properly formatted HTTP Response Headers.
+        assertThat("Response Code", response.getStatus(), is(200));
+        // The X-Foo header should be present an unchanged
+        assertThat("Response Header X-Foo", response.getField("X-Foo").getIntValue(), is(foovalue));
+    }
+
     @Test
     public void testFlushPrintWriter() throws Exception
     {