Repository: netty/netty
Issue: RFC non complaince : RFC 7540 "Hypertext Transfer Protocol Version 2 (HTTP/2)" https://datatracker.ietf.org/doc/html/rfc7540

**RFC behavior :**

 RFC7540 Section 8.1.2.3 : "All HTTP/2 requests MUST include exactly one
valid value for the ":method", ":scheme", and ":path" pseudo-header field,
unless it is a CONNECT request. An HTTP request that omits mandatory
pseudo-header fields is malformed."
    RFC7540 Section 8.1.2.6 "Malformed requests or responses that are
detected MUST be treated as a stream error (Section 5.4.2) of type
PROTOCOL_ERROR"
    RFC7540 Section 5.4.2 : "An endpoint that detects a stream error sends a
RST_STREAM frame (Section 6.4) that contains the stream identifier of the
stream where the error occurred. The RST_STREAM frame includes an error code
that indicates the type of error."

Actaul Issue - NRF is our network function.

Empty Path:
NRF does not reset the stream when :path pseudo-header field is empty
(without /).
See: 221003-114918_SBA_conformance_pseudoHeadersEmptyPath_172.19.124.pcap

Invalid Content-Length:
NRF sends RST_STREAM with CANCEL reason instead of PROTOCOL_ERROR when
content-length header is no correct for sent data.
See:
221003-114925_SBA_conformance_forbiddenHeadersContentLength_172.19.124.pcap

Invalid Connection-Specific header fields.
RFC7540 Section 8.1.2.2 "Connection-Specific Header Fields"
https://www.rfc-editor.org/rfc/rfc7540#section-8.1.2.2
Noted:  Only valid connection-specific header field is "te" and must have
value of "trailers"

NRF does not reset stream when receiving a te header with a value other than
"trailers"
See:
221003-114929_SBA_conformance_forbiddenHeadersTE_172.19.124.pcap

NRF does not reset stream when receiving a request with other
connection-specific header fields.
See:
221003-114932_SBA_conformance_forbiddenHeadersTransferEncoding_172.19.124.pcap
 for "transfer-encoding" and
221003-114939_SBA_conformance_forbiddenHeadersKeepAlive_172.19.124.pcap for
"keep-alive"


**Netty version Used** : 1.0.24

**Java version used** : 17

Need to know whether Netty follow RFC 7540 and when the compliance will be available.

Issue comments:
>Netty version Used : 1.0.24


@adhanoti Is this version, Reactor Netty version?

If yes then may be we would better discuss this in Reactor Netty issue tracker
Thanks. Yes it is reactor netty version.

Do i need to create separate issue?

Regards,
Ashish

On Tue, Feb 21, 2023 at 12:07 PM Violeta Georgieva ***@***.***>
wrote:

> Netty version Used : 1.0.24
>
> @adhanoti <https://github.com/adhanoti> Is this version, Reactor Netty
> version?
>
> If yes then may be we would better discuss this in Reactor Netty issue
> tracker
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/netty/netty/issues/13235#issuecomment-1437929581>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AK5F3SMW3DUUTUI7RB3HXW3WYRPCLANCNFSM6AAAAAAVCUIW2I>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>

yes please
RFC non complaince : RFC 7540 "Hypertext Transfer Protocol Version 2
(HTTP/2)" https://datatracker.ietf.org/doc/html/rfc7540 · Issue #2702 ·
reactor/reactor-netty (github.com)
<https://github.com/reactor/reactor-netty/issues/2702>

Ticket Created.

Let me know if you need wireshark for same. In ticket i am not able to
upload wireshark.

Regards,
Ashish

On Tue, Feb 21, 2023 at 12:18 PM Violeta Georgieva ***@***.***>
wrote:

> yes please
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/netty/netty/issues/13235#issuecomment-1437937763>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AK5F3SIQG5XSC3UIIN2YKN3WYRQMBANCNFSM6AAAAAAVCUIW2I>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>

@normanmaurer @chrisvest  @idelpivnitskiy This PR #12975 removes `:path` not empty check. IMO we should keep it. Wdyt?
hm ... it was checking before for duplicate pseudo-header ... ok ignore my comment above
@adhanoti The issue with `:path` should be address with #13238

The issue with `connection-specific header` should be address with #12975
@normanmaurer @chrisvest @idelpivnitskiy 

The issue with `Content-Length` that is not matching the actual data is interesting.
Netty correctly identifies that there is a mismatch and throws a stream error.

`Http2ConnectionHandler` correctly resets the stream with `PROTOCOL_ERROR`
https://github.com/netty/netty/blob/59aa6e635b9996cf21cd946e64353270679adc73/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionHandler.java#L744

However when `Http2FrameCodec` and `Http2MultiplexHandler`, the flow is the following:

`Http2FrameCodec` forwards the stream error
https://github.com/netty/netty/blob/59aa6e635b9996cf21cd946e64353270679adc73/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2FrameCodec.java#L568

`Http2MultiplexHandler` notifies the child channel and closes it.
https://github.com/netty/netty/blob/59aa6e635b9996cf21cd946e64353270679adc73/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2MultiplexHandler.java#L274-L278

When the stream channel is closed, it is closed always with `CANCEL`
https://github.com/netty/netty/blob/59aa6e635b9996cf21cd946e64353270679adc73/codec-http2/src/main/java/io/netty/handler/codec/http2/AbstractHttp2StreamChannel.java#L681

 I don't see a way to propagate the actual error code :(

@violetagg let me have a look 
This should be fixed by https://github.com/netty/netty/pull/13309

Pull request review comments:
These lines should be moved into the `VALUE_VALIDATOR` on line 86.
The `VALUE_VALIDATOR` receives only the value. How can we match name `:path` with the value?
https://github.com/netty/netty/blob/59aa6e635b9996cf21cd946e64353270679adc73/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2Headers.java#L86-L95

Also `Http2FrameCodecBuilder ` always builds `DefaultHttp2HeadersDecoder` with value validation disabled.
https://github.com/netty/netty/blob/59aa6e635b9996cf21cd946e64353270679adc73/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2FrameCodecBuilder.java#L208-L210
Thanks for finding this missed case @violetagg.
After reading the whole [RFC section 8.3](https://datatracker.ietf.org/doc/html/rfc9113#section-8.3), it sounds like any pseudo-header filed must not be empty, not only the `:path`. The only legit way is to omit a pseudo-header. WDYT?

Performing the check inside `DefaultHttp2Headers` seem reasonable, we just need to find a good spot where we have access to both name and value. I don't have any better idea except `validateValue` method as it's right now. 
We need to protect this validation by a configuration flag to make sure users can disable it.

Taking into account the RFC requirements to always run this validation (at least by default), the fact that `validateValues == false` by default, I would propose to use `validate` flag that controls header name validation. While in practice the check validates a value, I would say that technically it's a "validation of a preudo-header name". The purpose of `validateValues` is quite different. It's disabled by default because it will cause significant perf overhead for most users.
Agree
Please check the new commit.
> Taking into account the RFC requirements to always run this validation (at least by default), the fact that validateValues == false by default, I would propose to use validate flag that controls header name validation.

@idelpivnitskiy Is this "always" requirement any different from checking that byte ranges for values are valid, which we skip by default?
The biggest differences are:
1. the cost associated with a check
2. how likely is it to hit the problem (messing up with pseudo-headers seems more likely)
3. how it will affect the h2-protocol related flow vs user's business logic (having a bad char in some header won't impact h2 codec or h2->h1 transition)
4. is the header name required to perform the check

This case seems to me close to `TE` header validation we have in `HpackDecoder`.

Patch that resolved the issue:
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2Headers.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2Headers.java
index 91a6545b763..60a2925873c 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2Headers.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2Headers.java
@@ -175,6 +175,13 @@ protected void validateName(NameValidator<CharSequence> validator, boolean forAd
     protected void validateValue(ValueValidator<CharSequence> validator, CharSequence name, CharSequence value) {
         // This method has a noop override for backward compatibility, see https://github.com/netty/netty/pull/12975
         super.validateValue(validator, name, value);
+        // https://datatracker.ietf.org/doc/html/rfc9113#section-8.3.1
+        // pseudo headers must not be empty
+        if (nameValidator() == HTTP2_NAME_VALIDATOR && (value == null || value.length() == 0) &&
+                hasPseudoHeaderFormat(name)) {
+            PlatformDependent.throwException(connectionError(
+                    PROTOCOL_ERROR, "HTTP/2 pseudo-header '%s' must not be empty.", name));
+        }
     }
 
     @Override


Test patch:
diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/HpackDecoderTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/HpackDecoderTest.java
index e376197f12b..f70f528e6ca 100644
--- a/codec-http2/src/test/java/io/netty/handler/codec/http2/HpackDecoderTest.java
+++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/HpackDecoderTest.java
@@ -33,6 +33,7 @@
 
 import io.netty.buffer.ByteBuf;
 import io.netty.buffer.Unpooled;
+import io.netty.util.AsciiString;
 import io.netty.util.internal.StringUtil;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
@@ -54,6 +55,7 @@
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.greaterThanOrEqualTo;
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertSame;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.mockingDetails;
@@ -863,4 +865,50 @@ public void execute() throws Throwable {
             in2.release();
         }
     }
+
+    @ParameterizedTest
+    @CsvSource(value = {":method,''", ":scheme,''", ":authority,''", ":path,''"})
+    public void testPseudoHeaderEmptyValidationEnabled(String name, String value) throws Exception {
+        final ByteBuf in = Unpooled.buffer(200);
+        try {
+            HpackEncoder hpackEncoder = new HpackEncoder(true);
+
+            Http2Headers toEncode = new InOrderHttp2Headers();
+            toEncode.add(name, value);
+            hpackEncoder.encodeHeaders(1, in, toEncode, NEVER_SENSITIVE);
+
+            final Http2Headers decoded = new DefaultHttp2Headers();
+
+            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));
+        } finally {
+            in.release();
+        }
+    }
+
+    @ParameterizedTest
+    @CsvSource(value = {":method,''", ":scheme,''", ":authority,''", ":path,''"})
+    public void testPseudoHeaderEmptyValidationDisabled(String name, String value) throws Exception {
+        final ByteBuf in = Unpooled.buffer(200);
+        try {
+            HpackEncoder hpackEncoder = new HpackEncoder(true);
+
+            Http2Headers toEncode = new InOrderHttp2Headers();
+            toEncode.add(name, value);
+            hpackEncoder.encodeHeaders(1, in, toEncode, NEVER_SENSITIVE);
+
+            final Http2Headers decoded = new DefaultHttp2Headers(false);
+            hpackDecoder.decode(3, in, decoded, true);
+
+            assertSame(AsciiString.EMPTY_STRING, decoded.get(name));
+        } finally {
+            in.release();
+        }
+    }
 }
diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/HttpConversionUtilTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/HttpConversionUtilTest.java
index 29e6da98ce9..365dbdad83d 100644
--- a/codec-http2/src/test/java/io/netty/handler/codec/http2/HttpConversionUtilTest.java
+++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/HttpConversionUtilTest.java
@@ -83,8 +83,20 @@ public void setHttp2AuthorityNullOrEmpty() {
         HttpConversionUtil.setHttp2Authority(null, headers);
         assertNull(headers.authority());
 
-        HttpConversionUtil.setHttp2Authority("", headers);
-        assertSame(AsciiString.EMPTY_STRING, headers.authority());
+        // https://datatracker.ietf.org/doc/html/rfc9113#section-8.3.1
+        // Clients that generate HTTP/2 requests directly MUST use the ":authority" pseudo-header
+        // field to convey authority information, unless there is no authority information to convey
+        // (in which case it MUST NOT generate ":authority").
+        // An intermediary that forwards a request over HTTP/2 MUST construct an ":authority" pseudo-header
+        // field using the authority information from the control data of the original request, unless the
+        // original request's target URI does not contain authority information
+        // (in which case it MUST NOT generate ":authority").
+        assertThrows(Http2Exception.class, new Executable() {
+            @Override
+            public void execute() {
+                HttpConversionUtil.setHttp2Authority("", new DefaultHttp2Headers());
+            }
+        });
     }
 
     @Test