Repository: apple/swift-nio-http2 (swift)

Issue:
we don't tolerate a PUSH_PROMISE for a stream that we haven't seen HEADERS for
The RFC seems to be unclear here but the gist of the issue is that `https://nghttp2.org/` when receiving a request on stream one replies with a PUSH_PROMISE (on stream 1, for stream 2) before having sent any HEADERS frames.

That leads to `NIOHTTP2.HTTP2StreamStateMachine` being in state `halfClosedLocalPeerIdle ` when receiving the PP to which it responds with `.init(result: .streamError(streamID: self.streamID, underlyingError: NIOHTTP2Errors.BadStreamStateTransition(), type: .protocolError), effect: nil)`.

Given the high profile of nghttp2, we will probably need to accept this. Screenshot shots the H2 protocol chat, `1.2.3.4:12345` being the remote peer, the `17.x.x.x` being the local peer which is the client).

![Screenshot 2019-07-18 at 2 54 29 pm](https://user-images.githubusercontent.com/624238/61463268-f47df100-a96b-11e9-8d53-ec01ceb70b08.png)

Pull request:
Motivation:

RFC 7540 is not particularly clear on whether the server is required to
send PUSH_PROMISE frames before or after the HEADERS frame, or whether
both are acceptable. We took a somewhat hard-line approach by asserting
that PUSH_PROMISE could only be sent after the server sent HEADERS, but
we've now caught at least one notable implementation in the wild doing
something different (nghttp2).

Given that there's no clear normative language pointing in either
direction, we should consider being a bit more lenient and allowing
both.

Modifications:

- Updated the stream state machine to allow pushes on a locally-idle
stream.

Result:

Servers can send PUSH_PROMISE before they have sent HEADERS.
Resolves #144.

Patch:
diff --git a/Sources/NIOHTTP2/StreamStateMachine.swift b/Sources/NIOHTTP2/StreamStateMachine.swift
index 7c4f16ca..238d5b5f 100644
--- a/Sources/NIOHTTP2/StreamStateMachine.swift
+++ b/Sources/NIOHTTP2/StreamStateMachine.swift
@@ -673,6 +673,8 @@ extension HTTP2StreamStateMachine {
         // We can send PUSH_PROMISE frames in the following states:
         //
         // - fullyOpen when we are a server. In this case we assert that the stream was initiated by the client.
+        // - halfOpenRemoteLocalIdle, which only servers can enter on streams initiated by clients.
+        // - halfClosedRemoteLocalIdle, which only servers can enter on streams initiated by clients.
         // - halfClosedRemoteLocalActive, when we are a server, and when the stream was initiated by the client.
         //
         // RFC 7540 § 6.6 forbids sending PUSH_PROMISE frames on locally-initiated streams.
@@ -680,13 +682,15 @@ extension HTTP2StreamStateMachine {
         // PUSH_PROMISE frames never have stream effects: they cannot create or close streams, or affect flow control state.
         switch self.state {
         case .fullyOpen(localRole: .server, localContentLength: _, remoteContentLength: _, localWindow: _, remoteWindow: _),
+             .halfOpenRemoteLocalIdle(localWindow: _, remoteContentLength: _, remoteWindow: _),
+             .halfClosedRemoteLocalIdle(localWindow: _),
              .halfClosedRemoteLocalActive(localRole: .server, initiatedBy: .client, localContentLength: _, localWindow: _):
             return self.processRequestHeaders(headers, validateHeaderBlock: validateHeaderBlock, targetState: self.state, targetEffect: nil)
 
         // Sending a PUSH_PROMISE frame outside any of these states is a stream error of type PROTOCOL_ERROR.
         // Authors note: I cannot find a citation for this in RFC 7540, but this seems a sensible choice.
         case .idle, .reservedLocal, .reservedRemote, .halfClosedLocalPeerIdle, .halfClosedLocalPeerActive,
-             .halfClosedRemoteLocalIdle, .halfOpenLocalPeerIdle, .halfOpenRemoteLocalIdle, .closed,
+             .halfOpenLocalPeerIdle, .closed,
              .fullyOpen(localRole: .client, localContentLength: _, remoteContentLength: _, localWindow: _, remoteWindow: _),
              .halfClosedRemoteLocalActive(localRole: .client, initiatedBy: _, localContentLength: _, localWindow: _),
              .halfClosedRemoteLocalActive(localRole: .server, initiatedBy: .server, localContentLength: _, localWindow: _):
@@ -698,18 +702,22 @@ extension HTTP2StreamStateMachine {
         // We can receive PUSH_PROMISE frames in the following states:
         //
         // - fullyOpen when we are a client. In this case we assert that the stream was initiated by us.
+        // - halfOpenLocalPeerIdle, which only clients can enter on streams they initiated.
+        // - halfClosedLocalPeerIdle, which only clients can enter on streams they initiated.
         // - halfClosedLocalPeerActive, when we are a client, and when the stream was initiated by us.
         //
         // RFC 7540 § 6.6 forbids receiving PUSH_PROMISE frames on remotely-initiated streams.
         switch self.state {
         case .fullyOpen(localRole: .client, localContentLength: _, remoteContentLength: _, localWindow: _, remoteWindow: _),
+             .halfOpenLocalPeerIdle(localWindow: _, localContentLength: _, remoteWindow: _),
+             .halfClosedLocalPeerIdle(remoteWindow: _),
              .halfClosedLocalPeerActive(localRole: .client, initiatedBy: .client, remoteContentLength: _, remoteWindow: _):
             return self.processRequestHeaders(headers, validateHeaderBlock: validateHeaderBlock, targetState: self.state, targetEffect: nil)
 
         // Receiving a PUSH_PROMISE frame outside any of these states is a stream error of type PROTOCOL_ERROR.
         // Authors note: I cannot find a citation for this in RFC 7540, but this seems a sensible choice.
-        case .idle, .reservedLocal, .reservedRemote, .halfClosedLocalPeerIdle, .halfClosedRemoteLocalIdle,
-             .halfClosedRemoteLocalActive, .halfOpenLocalPeerIdle, .halfOpenRemoteLocalIdle, .closed,
+        case .idle, .reservedLocal, .reservedRemote, .halfClosedRemoteLocalIdle,
+             .halfClosedRemoteLocalActive, .halfOpenRemoteLocalIdle, .closed,
              .fullyOpen(localRole: .server, localContentLength: _, remoteContentLength: _, localWindow: _, remoteWindow: _),
              .halfClosedLocalPeerActive(localRole: .server, initiatedBy: _, remoteContentLength: _, remoteWindow: _),
              .halfClosedLocalPeerActive(localRole: .client, initiatedBy: .server, remoteContentLength: _, remoteWindow: _):


Test patch:
diff --git a/Tests/NIOHTTP2Tests/ConnectionStateMachineTests.swift b/Tests/NIOHTTP2Tests/ConnectionStateMachineTests.swift
index 87a77113..0942be41 100644
--- a/Tests/NIOHTTP2Tests/ConnectionStateMachineTests.swift
+++ b/Tests/NIOHTTP2Tests/ConnectionStateMachineTests.swift
@@ -308,36 +308,38 @@ class ConnectionStateMachineTests: XCTestCase {
         let streamTwo = HTTP2StreamID(2)
         let streamThree = HTTP2StreamID(3)
         let streamFour = HTTP2StreamID(4)
+        let streamSix = HTTP2StreamID(6)
 
         self.exchangePreamble()
 
         assertSucceeds(self.client.sendHeaders(streamID: streamOne, headers: ConnectionStateMachineTests.requestHeaders, isEndStreamSet: true))
         assertSucceeds(self.server.receiveHeaders(streamID: streamOne, headers: ConnectionStateMachineTests.requestHeaders, isEndStreamSet: true))
 
-        // Server cannot push right away
-        assertStreamError(type: .protocolError, self.server.sendPushPromise(originalStreamID: streamOne, childStreamID: streamTwo, headers: ConnectionStateMachineTests.requestHeaders))
-        var tempClient = self.client!
-        assertStreamError(type: .protocolError, tempClient.receivePushPromise(originalStreamID: streamOne, childStreamID: streamTwo, headers: ConnectionStateMachineTests.requestHeaders))
+        // Server can push right away
+        assertSucceeds(self.server.sendPushPromise(originalStreamID: streamOne, childStreamID: streamTwo, headers: ConnectionStateMachineTests.requestHeaders))
+        assertSucceeds(self.client.receivePushPromise(originalStreamID: streamOne, childStreamID: streamTwo, headers: ConnectionStateMachineTests.requestHeaders))
 
         // Server sends its headers
         assertSucceeds(self.server.sendHeaders(streamID: streamOne, headers: ConnectionStateMachineTests.responseHeaders, isEndStreamSet: false))
         assertSucceeds(self.client.receiveHeaders(streamID: streamOne, headers: ConnectionStateMachineTests.responseHeaders, isEndStreamSet: false))
 
         // Server pushes, suceeeds, and completes the pushed response.
-        assertSucceeds(self.server.sendPushPromise(originalStreamID: streamOne, childStreamID: streamTwo, headers: ConnectionStateMachineTests.requestHeaders))
-        assertSucceeds(self.client.receivePushPromise(originalStreamID: streamOne, childStreamID: streamTwo, headers: ConnectionStateMachineTests.requestHeaders))
+        assertSucceeds(self.server.sendPushPromise(originalStreamID: streamOne, childStreamID: streamFour, headers: ConnectionStateMachineTests.requestHeaders))
+        assertSucceeds(self.client.receivePushPromise(originalStreamID: streamOne, childStreamID: streamFour, headers: ConnectionStateMachineTests.requestHeaders))
 
         // Server attempts to push with invalid stream ID, fails. Client rejects it too.
         assertConnectionError(type: .protocolError, self.server.sendPushPromise(originalStreamID: streamOne, childStreamID: streamThree, headers: ConnectionStateMachineTests.requestHeaders))
-        tempClient = self.client!
+        var tempClient = self.client!
         assertConnectionError(type: .protocolError, tempClient.receivePushPromise(originalStreamID: streamOne, childStreamID: streamThree, headers: ConnectionStateMachineTests.requestHeaders))
 
         // Server attempts to push on stream two, fails. Client rejects it too.
-        assertStreamError(type: .protocolError, self.server.sendPushPromise(originalStreamID: streamTwo, childStreamID: streamFour, headers: ConnectionStateMachineTests.requestHeaders))
+        assertStreamError(type: .protocolError, self.server.sendPushPromise(originalStreamID: streamTwo, childStreamID: streamSix, headers: ConnectionStateMachineTests.requestHeaders))
         tempClient = self.client!
-        assertStreamError(type: .protocolError, tempClient.receivePushPromise(originalStreamID: streamTwo, childStreamID: streamFour, headers: ConnectionStateMachineTests.requestHeaders))
+        assertStreamError(type: .protocolError, tempClient.receivePushPromise(originalStreamID: streamTwo, childStreamID: streamSix, headers: ConnectionStateMachineTests.requestHeaders))
 
-        // Server completes both streams.
+        // Server completes all streams.
+        assertSucceeds(self.server.sendHeaders(streamID: streamFour, headers: ConnectionStateMachineTests.responseHeaders, isEndStreamSet: true))
+        assertSucceeds(self.client.receiveHeaders(streamID: streamFour, headers: ConnectionStateMachineTests.responseHeaders, isEndStreamSet: true))
         assertSucceeds(self.server.sendHeaders(streamID: streamTwo, headers: ConnectionStateMachineTests.responseHeaders, isEndStreamSet: true))
         assertSucceeds(self.client.receiveHeaders(streamID: streamTwo, headers: ConnectionStateMachineTests.responseHeaders, isEndStreamSet: true))
         assertSucceeds(self.server.sendHeaders(streamID: streamOne, headers: ConnectionStateMachineTests.trailers, isEndStreamSet: true))
diff --git a/Tests/NIOHTTP2Tests/SimpleClientServerTests+XCTest.swift b/Tests/NIOHTTP2Tests/SimpleClientServerTests+XCTest.swift
index 7f726916..f66bc57c 100644
--- a/Tests/NIOHTTP2Tests/SimpleClientServerTests+XCTest.swift
+++ b/Tests/NIOHTTP2Tests/SimpleClientServerTests+XCTest.swift
@@ -59,6 +59,8 @@ extension SimpleClientServerTests {
                 ("testClientConnectionErrorCorrectlyReported", testClientConnectionErrorCorrectlyReported),
                 ("testChangingMaxConcurrentStreams", testChangingMaxConcurrentStreams),
                 ("testFailsPromisesForBufferedWrites", testFailsPromisesForBufferedWrites),
+                ("testAllowPushPromiseBeforeReceivingHeadersNoBody", testAllowPushPromiseBeforeReceivingHeadersNoBody),
+                ("testAllowPushPromiseBeforeReceivingHeadersWithPossibleBody", testAllowPushPromiseBeforeReceivingHeadersWithPossibleBody),
            ]
    }
 }
diff --git a/Tests/NIOHTTP2Tests/SimpleClientServerTests.swift b/Tests/NIOHTTP2Tests/SimpleClientServerTests.swift
index 0565844f..fa19d8c9 100644
--- a/Tests/NIOHTTP2Tests/SimpleClientServerTests.swift
+++ b/Tests/NIOHTTP2Tests/SimpleClientServerTests.swift
@@ -1394,4 +1394,45 @@ class SimpleClientServerTests: XCTestCase {
         XCTAssertEqual(promiseResults, [false, false, false, false, false])
         XCTAssertNoThrow(try self.serverChannel.finish())
     }
+
+    func testAllowPushPromiseBeforeReceivingHeadersNoBody() throws {
+        // Begin by getting the connection up.
+        try self.basicHTTP2Connection()
+
+        // We're now going to try to send a request from the client to the server.
+        let headers = HPACKHeaders([(":path", "/"), (":method", "GET"), (":scheme", "https"), (":authority", "localhost")])
+
+        let clientStreamID = HTTP2StreamID(1)
+        let reqFrame = HTTP2Frame(streamID: clientStreamID, payload: .headers(.init(headers: headers, endStream: true)))
+
+        XCTAssertNoThrow(try self.assertFramesRoundTrip(frames: [reqFrame], sender: self.clientChannel, receiver: self.serverChannel))
+
+        // The server will push.
+        let pushFrame = HTTP2Frame(streamID: clientStreamID, payload: .pushPromise(.init(pushedStreamID: 2, headers: headers)))
+        XCTAssertNoThrow(try self.assertFramesRoundTrip(frames: [pushFrame], sender: self.serverChannel, receiver: self.clientChannel))
+
+        XCTAssertNoThrow(try self.clientChannel.finish())
+        XCTAssertNoThrow(try self.serverChannel.finish())
+    }
+
+    func testAllowPushPromiseBeforeReceivingHeadersWithPossibleBody() throws {
+        // This is the same as the test above but we don't set END_STREAM on our initial HEADERS, leading to a different stream state.
+        // Begin by getting the connection up.
+        try self.basicHTTP2Connection()
+
+        // We're now going to try to send a request from the client to the server.
+        let headers = HPACKHeaders([(":path", "/"), (":method", "GET"), (":scheme", "https"), (":authority", "localhost")])
+
+        let clientStreamID = HTTP2StreamID(1)
+        let reqFrame = HTTP2Frame(streamID: clientStreamID, payload: .headers(.init(headers: headers, endStream: false)))
+
+        XCTAssertNoThrow(try self.assertFramesRoundTrip(frames: [reqFrame], sender: self.clientChannel, receiver: self.serverChannel))
+
+        // The server will push.
+        let pushFrame = HTTP2Frame(streamID: clientStreamID, payload: .pushPromise(.init(pushedStreamID: 2, headers: headers)))
+        XCTAssertNoThrow(try self.assertFramesRoundTrip(frames: [pushFrame], sender: self.serverChannel, receiver: self.clientChannel))
+
+        XCTAssertNoThrow(try self.clientChannel.finish())
+        XCTAssertNoThrow(try self.serverChannel.finish())
+    }
 }