h2spec http2/8 tests should accept 400 Bad Request

[gstrauss]

(Thank you very much for h2spec!)

h2spec http2/8 tests should accept 400 Bad Request in addition to accepting RST_STREAM or GOAWAY.
My server responds with 400 Bad Request, and h2spec http2/8 tests fail with

             Expected: GOAWAY Frame (Error Code: PROTOCOL_ERROR)
                       RST_STREAM Frame (Error Code: PROTOCOL_ERROR)
                       Connection closed
               Actual: DATA Frame (length:0, flags:0x01, stream_id:1)

RFC 7540 Section 8.1.2.6. Malformed Requests and Responses

Intermediaries that process HTTP requests or responses (i.e., any intermediary not acting as a tunnel) MUST NOT forward a malformed request or response. Malformed requests or responses that are detected MUST be treated as a stream error (Section 5.4.2) of type PROTOCOL_ERROR.

For malformed requests, a server MAY send an HTTP response prior to closing or resetting the stream.

While intermediates must send PROTOCOL_ERROR, origin servers may send a response, e.g. 400 Bad Request which tells the client "do not retry this request unmodified", instead of the generic HTTP/2 error PROTOCOL_ERROR which is less precise.

Please allow 400 Bad Request as a valid response.

(If you agree, please post so and I might learn some golang and submit a PR. :))

[gstrauss]

The following works for me to allow 400 Bad Request in VerifyStreamError() in lieu of an HTTP/2 stream error. Feedback appreciated -- I am not fluent in golang. If it looks good to you, please let me know and I will submit a PR.

--- a/spec/verifier.go
+++ b/spec/verifier.go
@@ -5,6 +5,7 @@ import (
        "reflect"
 
        "golang.org/x/net/http2"
+       "golang.org/x/net/http2/hpack"
 )
 
 const (
@@ -101,6 +106,15 @@ func VerifyStreamError(conn *Conn, codes ...http2.ErrCode) error {
                ev := conn.WaitEvent()
 
                switch event := ev.(type) {
+               case HeadersFrameEvent:
+                       decoder := hpack.NewDecoder(2048, nil)
+                       hf, _ := decoder.DecodeFull(event.HeadersFrame.HeaderBlockFragment())
+                       for _, h := range hf {
+                               if h.Name == ":status" && h.Value == "400" {
+                                       passed = true
+                                       break
+                               }
+                       }
                case ConnectionClosedEvent:
                        passed = true
                case GoAwayFrameEvent:

[KingMob]

I think not sending a RST_STREAM/GOAWAY is incorrect, unfortunately.

Based on:

Malformed requests or responses that are detected MUST be treated as a stream error (Section 5.4.2) of type PROTOCOL_ERROR.

and

For malformed requests, a server MAY send an HTTP response prior to closing or resetting the stream.

and from § 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.

This implies that:

1. FWIW, any response is acceptable, not just 400. Doesn't even have to be an error 4xx/5xx status.
2. However, the server must still treat it as malformed, which means sending a RST_STREAM is required, though the RFC doesn't place any limits on how soon you have to send it. (GOAWAY is ok too, since "... an endpoint MAY choose to treat a stream error as a connection error." )
3. If you look at the author's comment here, he's saying that the frame h2spec reports is the last one received, which means your server never sent a RST_STREAM or GOAWAY before it timed out, or the conn closed.

[KingMob]

As I mentioned in #126 (comment), I think the way h2spec reports the last seen frame is a little misleading sometimes.

It's not that the HEADERS/DATA frames are always invalid, it's that it never saw the frame it expected to.

[gstrauss]

@KingMob I think you cut the RFC quote short, skipping relevant information.

For malformed requests, a server MAY send an HTTP response prior to closing or resetting the stream.

is part of the paragraph which starts with "Intermediaries" and is instructions for intermediaries

Intermediaries that process HTTP requests or responses (i.e., any intermediary not acting as a tunnel) MUST NOT forward a malformed request or response. Malformed requests or responses that are detected MUST be treated as a stream error (Section 5.4.2) of type PROTOCOL_ERROR.

As I wrote in the original post:

While intermediates must send PROTOCOL_ERROR, origin servers may send a response, e.g. 400 Bad Request which tells the client "do not retry this request unmodified", instead of the generic HTTP/2 error PROTOCOL_ERROR which is less precise.

[KingMob]

I considered that for a few minutes, because it's a little unclear. But there's a few reasons I think servers are required to do more than send a response.

First, the preceding sentence mentions intermediaries, but "Malformed requests or responses that are detected MUST be treated as a stream error (Section 5.4.2) of type PROTOCOL_ERROR" itself does not.

Second, the next sentence says "For malformed requests, a server MAY send an HTTP response prior to closing or resetting the stream", which implies that the server will in fact, be "closing or resetting the stream." It doesn't seem optional.

Finally, not sending a RST_STREAM (or GOAWAY) would conflict with the part of §5.4.2 I quoted above:

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.

Now this isn't perfectly clear, either. There's no MUST, but otherwise, it does say that an endpoint that detects a stream error will be sending a RST_STREAM (or a GOAWAY, since that's always allowed by §5.4.1)

Most of the language points to expectations that RST_STREAM/GOAWAY is expected, and a response is optional, but only if followed by a RST_STREAM/GOAWAY, so that's what I'm going with.

But you know, why argue, when we can ask the authors? I'll email them and see what they say.

[gstrauss]

Sure, please ask for clarification. And thank you for referencing the newer RFC 9113.
https://www.rfc-editor.org/rfc/rfc9113.html#RST_STREAM

For malformed requests, a server MAY send an HTTP response prior to closing or resetting the stream.

That sentence was in RFC 7540, too. Please note it says "... prior to closing or resetting the stream." Sending an HTTP response and closing the stream does not result in RST_STREAM; resetting the stream would result in RST_STREAM. Herein lies the difference. As a web server developer, I would prefer to send 400 Bad Request and close the stream so that the client knows it received a complete HTTP response, rather than resetting the stream, in which case the client did not necessarily receive a complete HTTP response. 400 Bad Request is (slightly) more precise than RST_STREAM with the (more generic) PROTOCOL_ERROR.

[KingMob]

The HTTP Working Group weighed in, and surprisingly (at least to me), there was no consensus.

Even the people who disagreed with Glenn's interpretation agreed it was defensible from the RFC as written, so I'm onboard with this issue, though I'll personally be going with the suggestion to follow a response with a RST_STREAM for Aleph.