fix: filter hop-by-hop headers in dev server proxy (#24092) (CP: 24.9) (#24102)

The dev server proxy in `AbstractDevServerRunner` forwarded all HTTP
headers between the browser and the Vite dev server, including
hop-by-hop headers that must not be forwarded by a proxy per RFC 9110
Section 7.6.1. It also forwarded the upstream Content-Length which may
not match the actual bytes after HttpURLConnection decoding, causing
broken responses on some servlet containers.

This change filters hop-by-hop headers and Content-Length from proxied
requests and responses, and avoids closing the output stream after
`sendError()`.

Related to #23564

Author: mcollovati

vaadin-dev-server/src/main/java/com/vaadin/base/devserver/AbstractDevServerRunner.java

@@ -29,7 +29,9 @@
 import java.nio.charset.StandardCharsets;
 import java.util.Enumeration;
 import java.util.List;
+import java.util.Locale;
 import java.util.Map;
+import java.util.Set;
 import java.util.UUID;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.CompletionException;
@@ -98,6 +100,13 @@ public abstract class AbstractDevServerRunner implements DevModeHandler {
     private static final Pattern WEBPACK_ILLEGAL_CHAR_PATTERN = Pattern
             .compile("\"|%22");
 
+    // Hop-by-hop headers per RFC 9110 Section 7.6.1: these are
+    // per-connection headers that a proxy must not forward to the next hop.
+    // Stored in lowercase for case-insensitive matching.
+    private static final Set<String> HOP_BY_HOP_HEADERS = Set.of("connection",
+            "keep-alive", "proxy-authenticate", "proxy-authorization", "te",
+            "trailer", "transfer-encoding", "upgrade");
+
     private static final int DEFAULT_BUFFER_SIZE = 32 * 1024;
     private static final int DEFAULT_TIMEOUT = 120 * 1000;
 
@@ -731,15 +740,20 @@ public boolean serveDevModeRequest(HttpServletRequest request,
         HttpURLConnection connection = prepareConnection(devServerRequestPath,
                 request.getMethod());
 
-        // Copies all the headers from the original request
+        // Copies request headers, skipping hop-by-hop headers
+        // (RFC 9110 Section 7.6.1) which are per-connection and must not
+        // be forwarded by a proxy
         Enumeration<String> headerNames = request.getHeaderNames();
         while (headerNames.hasMoreElements()) {
             String header = headerNames.nextElement();
-            connection.setRequestProperty(header,
-                    // Exclude keep-alive
-                    "Connect".equals(header) ? "close"
-                            : request.getHeader(header));
+            if (!HOP_BY_HOP_HEADERS
+                    .contains(header.toLowerCase(Locale.ENGLISH))) {
+                connection.setRequestProperty(header,
+                        request.getHeader(header));
+            }
         }
+        // Connection is not reused, signal to the dev server
+        connection.setRequestProperty("Connection", "close");
 
         // Send the request
         getLogger().debug("Requesting resource from {} {}", getServerName(),
@@ -756,13 +770,18 @@ public boolean serveDevModeRequest(HttpServletRequest request,
         getLogger().debug("Served resource by {}: {} {}", getServerName(),
                 responseCode, devServerRequestPath);
 
-        // Copies response headers
+        // Copies response headers, skipping hop-by-hop headers
+        // (RFC 9110 Section 7.6.1) and Content-Length. Content-Length is
+        // dropped because HttpURLConnection may transparently decode the
+        // body (de-chunking, decompression), making the original value
+        // incorrect. The servlet container will determine the correct
+        // framing from the actual bytes written (RFC 9110 Section 8.6).
         connection.getHeaderFields().forEach((header, values) -> {
-            if (header != null) {
-                if ("Transfer-Encoding".equals(header)) {
-                    return;
-                }
-                response.addHeader(header, values.get(0));
+            if (header != null
+                    && !HOP_BY_HOP_HEADERS
+                            .contains(header.toLowerCase(Locale.ENGLISH))
+                    && !"Content-Length".equalsIgnoreCase(header)) {
+                response.setHeader(header, values.get(0));
             }
         });
 
@@ -775,16 +794,18 @@ public boolean serveDevModeRequest(HttpServletRequest request,
             // Copies response payload
             writeStream(response.getOutputStream(),
                     connection.getInputStream());
+            // Close request to avoid issues in CI and Chrome
+            response.getOutputStream().close();
         } else if (responseCode < 400) {
             response.setStatus(responseCode);
+            response.getOutputStream().close();
         } else {
-            // Copies response code
+            // sendError() commits the response and may generate an error
+            // page; closing the output stream after that can throw in
+            // some servlet containers.
             response.sendError(responseCode);
         }
 
-        // Close request to avoid issues in CI and Chrome
-        response.getOutputStream().close();
-
         return true;
     }
 

vaadin-dev-server/src/test/java/com/vaadin/base/devserver/AbstractDevServerRunnerTest.java

@@ -1,5 +1,6 @@
 package com.vaadin.base.devserver;
 
+import java.io.ByteArrayInputStream;
 import java.io.File;
 import java.io.IOException;
 import java.net.HttpURLConnection;
@@ -11,6 +12,7 @@
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashMap;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.CompletableFuture;
@@ -179,6 +181,160 @@ private InetAddress findLocalhostAddress(
         }
     }
 
+    @Test
+    public void hopByHopRequestHeadersAreNotForwardedToDevServer()
+            throws Exception {
+        handler = new DummyRunner();
+        waitForDevServer();
+        DevModeHandler devServer = Mockito.spy(handler);
+
+        HttpServletResponse response = Mockito.mock(HttpServletResponse.class);
+        Mockito.when(response.getOutputStream())
+                .thenReturn(Mockito.mock(ServletOutputStream.class));
+
+        HttpServletRequest request = Mockito.mock(HttpServletRequest.class);
+        Mockito.when(request.getPathInfo()).thenReturn("/VAADIN/test.js");
+        Mockito.when(request.getRequestURI()).thenReturn("/VAADIN/test.js");
+
+        // Simulate browser headers including hop-by-hop
+        List<String> headerNames = List.of("Connection", "Keep-Alive", "Accept",
+                "Host", "Accept-Encoding");
+        Mockito.when(request.getHeaderNames())
+                .thenReturn(Collections.enumeration(headerNames));
+        Mockito.when(request.getHeader("Connection")).thenReturn("keep-alive");
+        Mockito.when(request.getHeader("Keep-Alive")).thenReturn("timeout=5");
+        Mockito.when(request.getHeader("Accept")).thenReturn("*/*");
+        Mockito.when(request.getHeader("Host")).thenReturn("localhost:8080");
+        Mockito.when(request.getHeader("Accept-Encoding")).thenReturn("gzip");
+
+        HttpURLConnection connection = Mockito.mock(HttpURLConnection.class);
+        Mockito.when(connection.getResponseCode())
+                .thenReturn(HttpURLConnection.HTTP_OK);
+        Mockito.when(connection.getHeaderFields()).thenReturn(Map.of());
+        Mockito.when(connection.getInputStream())
+                .thenReturn(new ByteArrayInputStream(new byte[0]));
+
+        Mockito.when(devServer.prepareConnection(Mockito.any(), Mockito.any()))
+                .thenReturn(connection);
+
+        devServer.serveDevModeRequest(request, response);
+
+        // Hop-by-hop headers must not be forwarded
+        Mockito.verify(connection, Mockito.never()).setRequestProperty(
+                Mockito.eq("Connection"), Mockito.eq("keep-alive"));
+        Mockito.verify(connection, Mockito.never()).setRequestProperty(
+                Mockito.eq("Keep-Alive"), Mockito.anyString());
+
+        // Connection must be set to close since it's not reused
+        Mockito.verify(connection).setRequestProperty("Connection", "close");
+
+        // Non-hop-by-hop headers must be forwarded
+        Mockito.verify(connection).setRequestProperty("Accept", "*/*");
+        Mockito.verify(connection).setRequestProperty("Host", "localhost:8080");
+        Mockito.verify(connection).setRequestProperty("Accept-Encoding",
+                "gzip");
+    }
+
+    @Test
+    public void hopByHopAndContentLengthResponseHeadersAreNotForwardedToBrowser()
+            throws Exception {
+        handler = new DummyRunner();
+        waitForDevServer();
+        DevModeHandler devServer = Mockito.spy(handler);
+
+        HttpServletResponse response = Mockito.mock(HttpServletResponse.class);
+        ServletOutputStream outputStream = Mockito
+                .mock(ServletOutputStream.class);
+        Mockito.when(response.getOutputStream()).thenReturn(outputStream);
+
+        HttpServletRequest request = Mockito.mock(HttpServletRequest.class);
+        Mockito.when(request.getPathInfo()).thenReturn("/VAADIN/test.js");
+        Mockito.when(request.getRequestURI()).thenReturn("/VAADIN/test.js");
+        Mockito.when(request.getHeaderNames())
+                .thenReturn(Collections.emptyEnumeration());
+
+        // Dev server response with hop-by-hop headers, Content-Length, and
+        // a legitimate end-to-end header
+        Map<String, List<String>> responseHeaders = new LinkedHashMap<>();
+        responseHeaders.put(null, List.of("HTTP/1.1 200 OK"));
+        responseHeaders.put("Content-Type", List.of("application/javascript"));
+        responseHeaders.put("Content-Length", List.of("42"));
+        responseHeaders.put("Connection", List.of("keep-alive"));
+        responseHeaders.put("Keep-Alive", List.of("timeout=5"));
+        responseHeaders.put("Transfer-Encoding", List.of("chunked"));
+
+        HttpURLConnection connection = Mockito.mock(HttpURLConnection.class);
+        Mockito.when(connection.getResponseCode())
+                .thenReturn(HttpURLConnection.HTTP_OK);
+        Mockito.when(connection.getHeaderFields()).thenReturn(responseHeaders);
+        Mockito.when(connection.getInputStream())
+                .thenReturn(new ByteArrayInputStream(new byte[0]));
+
+        Mockito.when(devServer.prepareConnection(Mockito.any(), Mockito.any()))
+                .thenReturn(connection);
+
+        devServer.serveDevModeRequest(request, response);
+
+        // Only end-to-end headers should be forwarded
+        Mockito.verify(response).setHeader("Content-Type",
+                "application/javascript");
+
+        // Hop-by-hop headers must not be forwarded (RFC 9110 Section 7.6.1)
+        Mockito.verify(response, Mockito.never())
+                .setHeader(Mockito.eq("Connection"), Mockito.anyString());
+        Mockito.verify(response, Mockito.never())
+                .addHeader(Mockito.eq("Connection"), Mockito.anyString());
+        Mockito.verify(response, Mockito.never())
+                .setHeader(Mockito.eq("Keep-Alive"), Mockito.anyString());
+        Mockito.verify(response, Mockito.never())
+                .addHeader(Mockito.eq("Keep-Alive"), Mockito.anyString());
+        Mockito.verify(response, Mockito.never()).setHeader(
+                Mockito.eq("Transfer-Encoding"), Mockito.anyString());
+        Mockito.verify(response, Mockito.never()).addHeader(
+                Mockito.eq("Transfer-Encoding"), Mockito.anyString());
+
+        // Content-Length must not be forwarded (RFC 9110 Section 8.6:
+        // may not match actual bytes after HttpURLConnection decoding)
+        Mockito.verify(response, Mockito.never())
+                .setHeader(Mockito.eq("Content-Length"), Mockito.anyString());
+        Mockito.verify(response, Mockito.never())
+                .addHeader(Mockito.eq("Content-Length"), Mockito.anyString());
+    }
+
+    @Test
+    public void outputStreamNotClosedAfterSendError() throws Exception {
+        handler = new DummyRunner();
+        waitForDevServer();
+        DevModeHandler devServer = Mockito.spy(handler);
+
+        HttpServletResponse response = Mockito.mock(HttpServletResponse.class);
+        ServletOutputStream outputStream = Mockito
+                .mock(ServletOutputStream.class);
+        Mockito.when(response.getOutputStream()).thenReturn(outputStream);
+
+        HttpServletRequest request = Mockito.mock(HttpServletRequest.class);
+        Mockito.when(request.getPathInfo()).thenReturn("/VAADIN/test.js");
+        Mockito.when(request.getRequestURI()).thenReturn("/VAADIN/test.js");
+        Mockito.when(request.getHeaderNames())
+                .thenReturn(Collections.emptyEnumeration());
+
+        HttpURLConnection connection = Mockito.mock(HttpURLConnection.class);
+        Mockito.when(connection.getResponseCode())
+                .thenReturn(HttpURLConnection.HTTP_INTERNAL_ERROR);
+        Mockito.when(connection.getHeaderFields()).thenReturn(Map.of());
+
+        Mockito.when(devServer.prepareConnection(Mockito.any(), Mockito.any()))
+                .thenReturn(connection);
+
+        devServer.serveDevModeRequest(request, response);
+
+        // sendError() commits the response; closing the output stream
+        // after that can throw in some servlet containers
+        Mockito.verify(response)
+                .sendError(HttpURLConnection.HTTP_INTERNAL_ERROR);
+        Mockito.verify(outputStream, Mockito.never()).close();
+    }
+
     private void assertOnDevProcessEnvironment(
             Class<? extends InetAddress> loopbackAddressType,
             Consumer<Map<String, String>> op) {