Move validation of connection headers in HTTP/2 back to HpackDecoder

Motivation: netty#12755 added validation for presence of connection-related headers while `HpackDecoder` decodes the incoming frame. Then netty#12832 moved this validation from `HpackDecoder` to `DefaultHttp2Headers`. As the result, existing use-case that could use `DefaultHttp2Headers` for HTTP/2 and HTTP/1.X broke when users add any of the mentioned prohibited headers. The HTTP/1.X to HTTP/2 translation logic usually has sanitization process that removes connection-related headers. It's enough to run this validation only for incoming messages and we should preserve backward compatibility for 4.1. Modifications: - Move `isConnectionHeader` and `te` validations from `DefaultHttp2Headers` back to `HpackDecoder`; - Add tests to verify `HpackDecoder` fails incoming headers as expected; - Add tests to verify mentioned headers can be added to `DefaultHttp2Headers`; Result: Backward compatibility is preserved, while validation for connection-related headers is done in `HpackDecoder`.
idelpivnitskiy · Nov 9, 2022 · 83bd4ad · 83bd4ad
1 parent 6debb1b
commit 83bd4ad
Show file tree

Hide file tree

Showing 4 changed files with 60 additions and 19 deletions.
diff --git a/codec-http2/src/main/java/io/netty5/handler/codec/http2/HpackDecoder.java b/codec-http2/src/main/java/io/netty5/handler/codec/http2/HpackDecoder.java
@@ -33,6 +33,7 @@
 
 import io.netty5.buffer.Buffer;
 import io.netty5.handler.codec.http.headers.HeaderValidationException;
+import io.netty5.handler.codec.http.headers.HttpHeaderValidationUtil;
 import io.netty5.handler.codec.http2.HpackUtil.IndexType;
 import io.netty5.handler.codec.http2.headers.Http2Headers;
 import io.netty5.util.AsciiString;
@@ -535,7 +536,7 @@ void appendToHeaderList(AsciiString name, AsciiString value) {
             try {
                 headers.add(name, value);
                 if (validateHeaders) {
-                    previousType = validateHeader(streamId, name, previousType);
+                    previousType = validateHeader(streamId, name, value, previousType);
                 }
             } catch (HeaderValidationException ex) {
                 validationException = streamError(streamId, PROTOCOL_ERROR, ex,
@@ -545,8 +546,8 @@ void appendToHeaderList(AsciiString name, AsciiString value) {
             }
         }
 
-        private static HeaderType validateHeader(int streamId, AsciiString name, HeaderType previousHeaderType)
-                throws Http2Exception {
+        private static HeaderType validateHeader(int streamId, AsciiString name, AsciiString value,
+                HeaderType previousHeaderType) throws Http2Exception {
             if (hasPseudoHeaderFormat(name)) {
                 if (previousHeaderType == HeaderType.REGULAR_HEADER) {
                     throw streamError(streamId, PROTOCOL_ERROR,
@@ -560,6 +561,14 @@ private static HeaderType validateHeader(int streamId, AsciiString name, HeaderT
                 }
                 return currentHeaderType;
             }
+            if (HttpHeaderValidationUtil.isConnectionHeader(name, true)) {
+                throw streamError(streamId, PROTOCOL_ERROR,
+                        "Illegal connection-specific header '%s' encountered.", name);
+            }
+            if (HttpHeaderValidationUtil.isTeNotTrailers(name, value)) {
+                throw streamError(streamId, PROTOCOL_ERROR,
+                        "Illegal value specified for the 'TE' header (only 'trailers' is allowed).");
+            }
 
             return HeaderType.REGULAR_HEADER;
         }

diff --git a/codec-http2/src/main/java/io/netty5/handler/codec/http2/headers/DefaultHttp2Headers.java b/codec-http2/src/main/java/io/netty5/handler/codec/http2/headers/DefaultHttp2Headers.java
@@ -18,7 +18,6 @@
 import io.netty5.handler.codec.http.headers.DefaultHttpHeaders;
 import io.netty5.handler.codec.http.headers.HeaderValidationException;
 import io.netty5.handler.codec.http.headers.HttpCookiePair;
-import io.netty5.handler.codec.http.headers.HttpHeaderValidationUtil;
 import io.netty5.handler.codec.http.headers.HttpHeaders;
 import io.netty5.handler.codec.http.headers.HttpSetCookie;
 import io.netty5.handler.codec.http.headers.MultiMap;
@@ -93,24 +92,11 @@ protected CharSequence validateKey(@Nullable CharSequence name, boolean forAdd)
                         }
                     }
                 }
-                if (HttpHeaderValidationUtil.isConnectionHeader(name, true)) {
-                    throw new HeaderValidationException(
-                            "Illegal connection-specific header '" + name + "' encountered.");
-                }
             }
         }
         return name;
     }
 
-    @Override
-    protected CharSequence validateValue(CharSequence key, CharSequence value) {
-        if (validateValues && HttpHeaderValidationUtil.isTeNotTrailers(key, value)) {
-            throw new HeaderValidationException(
-                    "Illegal value specified for the 'TE' header (only 'trailers' is allowed).");
-        }
-        return super.validateValue(key, value);
-    }
-
     @Override
     public Http2Headers copy() {
         DefaultHttp2Headers copy = new DefaultHttp2Headers(

diff --git a/codec-http2/src/test/java/io/netty5/handler/codec/http2/DefaultHttp2HeadersTest.java b/codec-http2/src/test/java/io/netty5/handler/codec/http2/DefaultHttp2HeadersTest.java
@@ -21,6 +21,8 @@
 import io.netty5.util.internal.StringUtil;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.function.Executable;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.CsvSource;
 
 import java.util.Map.Entry;
 
@@ -191,6 +193,15 @@ void setMustOverwritePseudoHeaders() {
         assertEquals(of("http"), headers.scheme());
     }
 
+    @ParameterizedTest(name = "{displayName} [{index}] name={0} value={1}")
+    @CsvSource(value = {"upgrade,protocol1", "connection,close", "keep-alive,timeout=5", "proxy-connection,close",
+            "transfer-encoding,chunked", "te,something-else"})
+    void possibleToAddConnectionHeaders(String name, String value) {
+        Http2Headers headers = newHeaders();
+        headers.add(name, value);
+        assertTrue(headers.contains(name, value));
+    }
+
     private static void verifyAllPseudoHeadersPresent(Http2Headers headers) {
         for (PseudoHeaderName pseudoName : PseudoHeaderName.values()) {
             assertNotNull(headers.get(pseudoName.value()), () -> "did not find pseudo-header " + pseudoName);

diff --git a/codec-http2/src/test/java/io/netty5/handler/codec/http2/HpackDecoderTest.java b/codec-http2/src/test/java/io/netty5/handler/codec/http2/HpackDecoderTest.java
@@ -38,6 +38,8 @@
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.function.Executable;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.CsvSource;
 
 import java.util.Iterator;
 import java.util.List;
@@ -46,6 +48,7 @@
 
 import static io.netty5.buffer.DefaultBufferAllocators.onHeapAllocator;
 import static io.netty5.handler.codec.http2.HpackDecoder.decodeULE128;
+import static io.netty5.handler.codec.http2.Http2Error.PROTOCOL_ERROR;
 import static io.netty5.handler.codec.http2.Http2HeadersEncoder.NEVER_SENSITIVE;
 import static io.netty5.util.AsciiString.EMPTY_STRING;
 import static io.netty5.util.AsciiString.of;
@@ -672,12 +675,44 @@ public Iterator<Entry<CharSequence, CharSequence>> iterator() {
 
             final Http2Headers decoded = Http2Headers.newHeaders();
 
-            assertThrows(Http2Exception.StreamException.class, new Executable() {
+            Http2Exception.StreamException e = assertThrows(Http2Exception.StreamException.class, new Executable() {
                 @Override
                 public void execute() throws Throwable {
-                    hpackDecoder.decode(1, in, decoded, true);
+                    hpackDecoder.decode(3, in, decoded, true);
+                }
+            });
+            assertThat(e.streamId(), is(3));
+            assertThat(e.error(), is(PROTOCOL_ERROR));
+        }
+    }
+
+    @ParameterizedTest(name = "{displayName} [{index}] name={0} value={1}")
+    @CsvSource(value = {"upgrade,protocol1", "connection,close", "keep-alive,timeout=5", "proxy-connection,close",
+            "transfer-encoding,chunked", "te,something-else"})
+    public void receivedConnectionHeader(String name, String value) throws Exception {
+        try (Buffer in = onHeapAllocator().allocate(200)) {
+            HpackEncoder hpackEncoder = new HpackEncoder(true);
+
+            Http2Headers toEncode = new DefaultHttp2Headers(2, false, false, false) {
+                @Override
+                public Iterator<Entry<CharSequence, CharSequence>> iterator() {
+                    Entry<CharSequence, CharSequence> pseudo = Map.entry(":method", "GET");
+                    Entry<CharSequence, CharSequence> connection = Map.entry(name, value);
+                    return List.of(pseudo, connection).iterator();
+                }
+            };
+            hpackEncoder.encodeHeaders(1, in, toEncode, NEVER_SENSITIVE);
+
+            final Http2Headers decoded = Http2Headers.newHeaders();
+
+            Http2Exception.StreamException e = assertThrows(Http2Exception.StreamException.class, new Executable() {
+                @Override
+                public void execute() throws Throwable {
+                    hpackDecoder.decode(3, in, decoded, true);
                 }
             });
+            assertThat(e.streamId(), is(3));
+            assertThat(e.error(), is(PROTOCOL_ERROR));
         }
     }