8354276: Strict HTTP header validation

djelinski · djelinski · commit c94a7ae11e58 · 2025-05-05T05:29:06.000Z
Reviewed-by: dfuchs, jpai
diff --git a/src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java b/src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java
@@ -69,6 +69,7 @@
 import jdk.internal.net.http.common.SequentialScheduler;
 import jdk.internal.net.http.common.Utils;
 import jdk.internal.net.http.common.ValidatingHeadersConsumer;
+import jdk.internal.net.http.common.ValidatingHeadersConsumer.Context;
 import jdk.internal.net.http.frame.ContinuationFrame;
 import jdk.internal.net.http.frame.DataFrame;
 import jdk.internal.net.http.frame.ErrorFrame;
@@ -89,7 +90,6 @@
 import jdk.internal.net.http.hpack.DecodingCallback;
 import jdk.internal.net.http.hpack.Encoder;
 import static java.nio.charset.StandardCharsets.UTF_8;
-import static jdk.internal.net.http.frame.SettingsFrame.DEFAULT_INITIAL_WINDOW_SIZE;
 import static jdk.internal.net.http.frame.SettingsFrame.ENABLE_PUSH;
 import static jdk.internal.net.http.frame.SettingsFrame.HEADER_TABLE_SIZE;
 import static jdk.internal.net.http.frame.SettingsFrame.INITIAL_CONNECTION_WINDOW_SIZE;
@@ -340,6 +340,7 @@ private final class PushPromiseDecoder extends HeaderDecoder implements Decoding
         final AtomicReference<Throwable> errorRef = new AtomicReference<>();
 
         PushPromiseDecoder(int parentStreamId, int pushPromiseStreamId, Stream<?> parent) {
+            super(Context.REQUEST);
             this.parentStreamId = parentStreamId;
             this.pushPromiseStreamId = pushPromiseStreamId;
             this.parent = parent;
@@ -984,7 +985,10 @@ void processFrame(Http2Frame frame) throws IOException {
                     // always decode the headers as they may affect
                     // connection-level HPACK decoding state
                     if (orphanedConsumer == null || frame.getClass() != ContinuationFrame.class) {
-                        orphanedConsumer = new ValidatingHeadersConsumer();
+                        orphanedConsumer = new ValidatingHeadersConsumer(
+                                frame instanceof PushPromiseFrame ?
+                                        Context.REQUEST :
+                                        Context.RESPONSE);
                     }
                     DecodingCallback decoder = orphanedConsumer::onDecoded;
                     try {
diff --git a/src/java.net.http/share/classes/jdk/internal/net/http/Stream.java b/src/java.net.http/share/classes/jdk/internal/net/http/Stream.java
@@ -1871,7 +1871,12 @@ void closeAsUnprocessed() {
         }
     }
 
-    private class HeadersConsumer extends ValidatingHeadersConsumer implements DecodingCallback {
+    private final class HeadersConsumer extends ValidatingHeadersConsumer
+            implements DecodingCallback {
+
+        private HeadersConsumer() {
+            super(Context.RESPONSE);
+        }
 
         boolean maxHeaderListSizeReached;
 
diff --git a/src/java.net.http/share/classes/jdk/internal/net/http/common/HeaderDecoder.java b/src/java.net.http/share/classes/jdk/internal/net/http/common/HeaderDecoder.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2023, 2024, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2023, 2025, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -30,7 +30,8 @@ public class HeaderDecoder extends ValidatingHeadersConsumer {
 
     private final HttpHeadersBuilder headersBuilder;
 
-    public HeaderDecoder() {
+    public HeaderDecoder(Context context) {
+        super(context);
         this.headersBuilder = new HttpHeadersBuilder();
     }
 
diff --git a/src/java.net.http/share/classes/jdk/internal/net/http/common/ValidatingHeadersConsumer.java b/src/java.net.http/share/classes/jdk/internal/net/http/common/ValidatingHeadersConsumer.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2023, 2025, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -26,15 +26,39 @@
 
 import java.io.IOException;
 import java.io.UncheckedIOException;
+import java.net.ProtocolException;
+import java.util.Map;
+import java.util.Objects;
 import java.util.Set;
 
 /*
  * Checks RFC 9113 rules (relaxed) compliance regarding pseudo-headers.
  */
 public class ValidatingHeadersConsumer {
 
-    private static final Set<String> PSEUDO_HEADERS =
-            Set.of(":authority", ":method", ":path", ":scheme", ":status");
+    private final Context context;
+
+    public ValidatingHeadersConsumer(Context context) {
+        this.context = Objects.requireNonNull(context);
+    }
+
+    public enum Context {
+        REQUEST,
+        RESPONSE,
+    }
+
+    // Map of permitted pseudo headers in requests and responses
+    private static final Map<String, Context> PSEUDO_HEADERS =
+            Map.of(":authority", Context.REQUEST,
+                    ":method", Context.REQUEST,
+                    ":path", Context.REQUEST,
+                    ":scheme", Context.REQUEST,
+                    ":status", Context.RESPONSE);
+
+    // connection-specific, prohibited by RFC 9113 section 8.2.2
+    private static final Set<String> PROHIBITED_HEADERS =
+            Set.of("connection", "proxy-connection", "keep-alive",
+                    "transfer-encoding", "upgrade");
 
     /** Used to check that if there are pseudo-headers, they go first */
     private boolean pseudoHeadersEnded;
@@ -60,11 +84,25 @@ public void onDecoded(CharSequence name, CharSequence value)
         if (n.startsWith(":")) {
             if (pseudoHeadersEnded) {
                 throw newException("Unexpected pseudo-header '%s'", n);
-            } else if (!PSEUDO_HEADERS.contains(n)) {
-                throw newException("Unknown pseudo-header '%s'", n);
+            } else {
+                Context expectedContext = PSEUDO_HEADERS.get(n);
+                if (expectedContext == null) {
+                    throw newException("Unknown pseudo-header '%s'", n);
+                } else if (expectedContext != context) {
+                    throw newException("Pseudo-header '%s' is not valid in context " + context, n);
+                }
             }
         } else {
             pseudoHeadersEnded = true;
+            // Check for prohibited connection-specific headers.
+            // Some servers echo request headers in push promises.
+            // If the request was a HTTP/1.1 upgrade, it included some prohibited headers.
+            // For compatibility, we ignore prohibited headers in push promises.
+            if (context != Context.REQUEST) {
+                if (PROHIBITED_HEADERS.contains(n)) {
+                    throw newException("Prohibited header name '%s'", n);
+                }
+            }
             // RFC-9113, section 8.2.1 for HTTP/2 and RFC-9114, section 4.2 state that
             // header name MUST be lowercase (and allowed characters)
             if (!Utils.isValidLowerCaseName(n)) {
@@ -84,6 +122,6 @@ protected String formatMessage(String message, String header) {
     protected UncheckedIOException newException(String message, String header)
     {
         return new UncheckedIOException(
-                new IOException(formatMessage(message, header)));
+                new ProtocolException(formatMessage(message, header)));
     }
 }
diff --git a/test/jdk/java/net/httpclient/http2/BadHeadersTest.java b/test/jdk/java/net/httpclient/http2/BadHeadersTest.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2018, 2023, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2018, 2025, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -23,7 +23,10 @@
 
 /*
  * @test
- * @bug 8303965
+ * @bug 8303965 8354276
+ * @summary This test verifies the behaviour of the HttpClient when presented
+ *          with a HEADERS frame followed by CONTINUATION frames, and when presented
+ *          with bad header fields.
  * @library /test/lib /test/jdk/java/net/httpclient/lib
  * @build jdk.httpclient.test.lib.http2.Http2TestServer jdk.test.lib.net.SimpleSSLContext
  * @run testng/othervm -Djdk.internal.httpclient.debug=true BadHeadersTest
@@ -44,6 +47,7 @@
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
+import java.net.ProtocolException;
 import java.net.URI;
 import java.net.http.HttpClient;
 import java.net.http.HttpHeaders;
@@ -76,6 +80,8 @@ public class BadHeadersTest {
         of(entry(":status", "200"),  entry("hell o", "value")),                    // Space in the name
         of(entry(":status", "200"),  entry("hello", "line1\r\n  line2\r\n")),      // Multiline value
         of(entry(":status", "200"),  entry("hello", "DE" + ((char) 0x7F) + "L")),  // Bad byte in value
+        of(entry(":status", "200"),  entry("connection", "close")),                // Prohibited connection-specific header
+        of(entry(":status", "200"),  entry(":scheme", "https")),                   // Request pseudo-header in response
         of(entry("hello", "world!"), entry(":status", "200"))                      // Pseudo header is not the first one
     );
 
@@ -86,7 +92,7 @@ public class BadHeadersTest {
     String https2URI;
 
     /**
-     * A function that returns a list of 1) a HEADERS frame ( with an empty
+     * A function that returns a list of 1) one HEADERS frame ( with an empty
      * payload ), and 2) a CONTINUATION frame with the actual headers.
      */
     static BiFunction<Integer,List<ByteBuffer>,List<Http2Frame>> oneContinuation =
@@ -100,7 +106,7 @@ public class BadHeadersTest {
             };
 
     /**
-     * A function that returns a list of a HEADERS frame followed by a number of
+     * A function that returns a list of one HEADERS frame followed by a number of
      * CONTINUATION frames. Each frame contains just a single byte of payload.
      */
     static BiFunction<Integer,List<ByteBuffer>,List<Http2Frame>> byteAtATime =
@@ -189,12 +195,13 @@ void testAsync(String uri,
             try {
                 HttpResponse<String> response = cc.sendAsync(request, BodyHandlers.ofString()).get();
                 fail("Expected exception, got :" + response + ", " + response.body());
-            } catch (Throwable t0) {
+            } catch (Exception t0) {
                 System.out.println("Got EXPECTED: " + t0);
                 if (t0 instanceof ExecutionException) {
-                    t0 = t0.getCause();
+                    t = t0.getCause();
+                } else {
+                    t = t0;
                 }
-                t = t0;
             }
             assertDetailMessage(t, i);
         }
@@ -204,15 +211,21 @@ void testAsync(String uri,
     // sync with implementation.
     static void assertDetailMessage(Throwable throwable, int iterationIndex) {
         try {
-            assertTrue(throwable instanceof IOException,
-                    "Expected IOException, got, " + throwable);
+            assertTrue(throwable instanceof ProtocolException,
+                    "Expected ProtocolException, got " + throwable);
             assertTrue(throwable.getMessage().contains("malformed response"),
                     "Expected \"malformed response\" in: " + throwable.getMessage());
 
             if (iterationIndex == 0) { // unknown
                 assertTrue(throwable.getMessage().contains("Unknown pseudo-header"),
                         "Expected \"Unknown pseudo-header\" in: " + throwable.getMessage());
-            } else if (iterationIndex == 4) { // unexpected
+            } else if (iterationIndex == 4) { // prohibited
+                assertTrue(throwable.getMessage().contains("Prohibited header name"),
+                        "Expected \"Prohibited header name\" in: " + throwable.getMessage());
+            } else if (iterationIndex == 5) { // unexpected type
+                assertTrue(throwable.getMessage().contains("not valid in context"),
+                        "Expected \"not valid in context\" in: " + throwable.getMessage());
+            } else if (iterationIndex == 6) { // unexpected sequence
                 assertTrue(throwable.getMessage().contains(" Unexpected pseudo-header"),
                         "Expected \" Unexpected pseudo-header\" in: " + throwable.getMessage());
             } else {
diff --git a/test/jdk/java/net/httpclient/http2/BadPushPromiseTest.java b/test/jdk/java/net/httpclient/http2/BadPushPromiseTest.java

Original file line number	Diff line number	Diff line change
`@@ -1871,7 +1871,12 @@ void closeAsUnprocessed() {`
`1871`	`1871`	`}`
`1872`	`1872`	`}`
`1873`	`1873`
`1874`		`- private class HeadersConsumer extends ValidatingHeadersConsumer implements DecodingCallback {`
	`1874`	`+ private final class HeadersConsumer extends ValidatingHeadersConsumer`
	`1875`	`+ implements DecodingCallback {`
	`1876`	`+`
	`1877`	`+ private HeadersConsumer() {`
	`1878`	`+ super(Context.RESPONSE);`
	`1879`	`+ }`
`1875`	`1880`
`1876`	`1881`	`boolean maxHeaderListSizeReached;`
`1877`	`1882`
Original file line number	Diff line number	Diff line change
`@@ -1,5 +1,5 @@`
`1`	`1`	`/*`
`2`		`- * Copyright (c) 2023, 2024, Oracle and/or its affiliates. All rights reserved.`
	`2`	`+ * Copyright (c) 2023, 2025, Oracle and/or its affiliates. All rights reserved.`
`3`	`3`	`* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.`
`4`	`4`	`*`
`5`	`5`	`* This code is free software; you can redistribute it and/or modify it`
`@@ -30,7 +30,8 @@ public class HeaderDecoder extends ValidatingHeadersConsumer {`
`30`	`30`
`31`	`31`	`private final HttpHeadersBuilder headersBuilder;`
`32`	`32`
`33`		`- public HeaderDecoder() {`
	`33`	`+ public HeaderDecoder(Context context) {`
	`34`	`+ super(context);`
`34`	`35`	`this.headersBuilder = new HttpHeadersBuilder();`
`35`	`36`	`}`
`36`	`37`