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

Issue:
Hitting an inbound connection error can lead to InvalidStreamIDForPeer errors when acting as a client.
The code that handles shutting down on inboundConnectionError tends to hit InvalidStreamIDForPeer. This is because we attempt to send a GOAWAY with lastStreamID set to `.maxID`, which is technically an invalid stream ID for the peer. We should modify the check here, as it's reasonable to want to set `.maxID` in this case, even though the server cannot technically emit such a stream ID.

Pull request:
Motivation:

We would previously error if clients emitted a GOAWAY frame that had
a last stream ID set to the max value. This is a bit unnecessary, not
least given that we emitted those frames ourselves! In fact, this error
would mask real server protocol errors if we hit them.

Modifications:

- Allowed max stream ID as the last stream ID.
- Wrote a test to verify this.

Result:

Fewer unnecessary protocol errors.
Resolves #101.

Patch:
diff --git a/Sources/NIOHTTP2/ConnectionStateMachine/FrameReceivingStates/ReceivingGoAwayState.swift b/Sources/NIOHTTP2/ConnectionStateMachine/FrameReceivingStates/ReceivingGoAwayState.swift
index 143a4ca0..54759deb 100644
--- a/Sources/NIOHTTP2/ConnectionStateMachine/FrameReceivingStates/ReceivingGoAwayState.swift
+++ b/Sources/NIOHTTP2/ConnectionStateMachine/FrameReceivingStates/ReceivingGoAwayState.swift
@@ -24,7 +24,7 @@ protocol ReceivingGoawayState {
 
 extension ReceivingGoawayState {
     mutating func receiveGoAwayFrame(lastStreamID: HTTP2StreamID) -> StateMachineResultWithEffect {
-        guard lastStreamID.mayBeInitiatedBy(self.role) || lastStreamID == .rootStream else {
+        guard lastStreamID.mayBeInitiatedBy(self.role) || lastStreamID == .rootStream || lastStreamID == .maxID else {
             // The remote peer has sent a GOAWAY with an invalid stream ID.
             return .init(result: .connectionError(underlyingError: NIOHTTP2Errors.InvalidStreamIDForPeer(), type: .protocolError), effect: nil)
         }
@@ -37,7 +37,7 @@ extension ReceivingGoawayState {
 
 extension ReceivingGoawayState where Self: RemotelyQuiescingState {
     mutating func receiveGoAwayFrame(lastStreamID: HTTP2StreamID) -> StateMachineResultWithEffect {
-        guard lastStreamID.mayBeInitiatedBy(self.role) || lastStreamID == .rootStream else {
+        guard lastStreamID.mayBeInitiatedBy(self.role) || lastStreamID == .rootStream || lastStreamID == .maxID else {
             // The remote peer has sent a GOAWAY with an invalid stream ID.
             return .init(result: .connectionError(underlyingError: NIOHTTP2Errors.InvalidStreamIDForPeer(), type: .protocolError), effect: nil)
         }
diff --git a/Sources/NIOHTTP2/ConnectionStateMachine/FrameSendingStates/SendingGoawayState.swift b/Sources/NIOHTTP2/ConnectionStateMachine/FrameSendingStates/SendingGoawayState.swift
index 2bfe50fc..a1189daf 100644
--- a/Sources/NIOHTTP2/ConnectionStateMachine/FrameSendingStates/SendingGoawayState.swift
+++ b/Sources/NIOHTTP2/ConnectionStateMachine/FrameSendingStates/SendingGoawayState.swift
@@ -24,7 +24,7 @@ protocol SendingGoawayState {
 
 extension SendingGoawayState {
     mutating func sendGoAwayFrame(lastStreamID: HTTP2StreamID) -> StateMachineResultWithEffect {
-        guard lastStreamID.mayBeInitiatedBy(self.peerRole) || lastStreamID == .rootStream else {
+        guard lastStreamID.mayBeInitiatedBy(self.peerRole) || lastStreamID == .rootStream || lastStreamID == .maxID else {
             // The user has sent a GOAWAY with an invalid stream ID.
             return .init(result: .connectionError(underlyingError: NIOHTTP2Errors.InvalidStreamIDForPeer(), type: .protocolError), effect: nil)
         }
@@ -37,7 +37,7 @@ extension SendingGoawayState {
 
 extension SendingGoawayState where Self: LocallyQuiescingState {
     mutating func sendGoAwayFrame(lastStreamID: HTTP2StreamID) -> StateMachineResultWithEffect {
-        guard lastStreamID.mayBeInitiatedBy(self.peerRole) || lastStreamID == .rootStream else {
+        guard lastStreamID.mayBeInitiatedBy(self.peerRole) || lastStreamID == .rootStream || lastStreamID == .maxID else {
             // The user has sent a GOAWAY with an invalid stream ID.
             return .init(result: .connectionError(underlyingError: NIOHTTP2Errors.InvalidStreamIDForPeer(), type: .protocolError), effect: nil)
         }


Test patch:
diff --git a/Tests/NIOHTTP2Tests/SimpleClientServerTests+XCTest.swift b/Tests/NIOHTTP2Tests/SimpleClientServerTests+XCTest.swift
index 3d2bca0e..986653d8 100644
--- a/Tests/NIOHTTP2Tests/SimpleClientServerTests+XCTest.swift
+++ b/Tests/NIOHTTP2Tests/SimpleClientServerTests+XCTest.swift
@@ -56,6 +56,7 @@ extension SimpleClientServerTests {
                 ("testStreamErrorOnSelfDependentPriorityFrames", testStreamErrorOnSelfDependentPriorityFrames),
                 ("testStreamErrorOnSelfDependentHeadersFrames", testStreamErrorOnSelfDependentHeadersFrames),
                 ("testInvalidRequestHeaderBlockAllowsRstStream", testInvalidRequestHeaderBlockAllowsRstStream),
+                ("testClientConnectionErrorCorrectlyReported", testClientConnectionErrorCorrectlyReported),
            ]
    }
 }
diff --git a/Tests/NIOHTTP2Tests/SimpleClientServerTests.swift b/Tests/NIOHTTP2Tests/SimpleClientServerTests.swift
index eb2971d4..811e1894 100644
--- a/Tests/NIOHTTP2Tests/SimpleClientServerTests.swift
+++ b/Tests/NIOHTTP2Tests/SimpleClientServerTests.swift
@@ -1287,4 +1287,35 @@ class SimpleClientServerTests: XCTestCase {
         XCTAssertNoThrow(XCTAssertTrue(try self.clientChannel.finish().isClean))
         _ = try? self.serverChannel.finish()
     }
+
+    func testClientConnectionErrorCorrectlyReported() throws {
+        // Begin by getting the connection up.
+        try self.basicHTTP2Connection()
+
+        // Now we're going to try to trigger a connection error in the client. We'll do it by sending a weird SETTINGS frame that isn't valid.
+        let weirdSettingsFrame: [UInt8] = [
+            0x00, 0x00, 0x06,           // 3-byte payload length (6 bytes)
+            0x04,                       // 1-byte frame type (SETTINGS)
+            0x00,                       // 1-byte flags (none)
+            0x00, 0x00, 0x00, 0x00,     // 4-byte stream identifier
+            0x00, 0x02,                 // SETTINGS_ENABLE_PUSH
+            0x00, 0x00, 0x00, 0x03,     //      = 3, an invalid value
+        ]
+        var settingsBuffer = self.clientChannel.allocator.buffer(capacity: 128)
+        settingsBuffer.writeBytes(weirdSettingsFrame)
+
+        XCTAssertThrowsError(try self.clientChannel.writeInbound(settingsBuffer)) { error in
+            XCTAssertEqual(error as? NIOHTTP2Errors.InvalidSetting, NIOHTTP2Errors.InvalidSetting(setting: HTTP2Setting(parameter: .enablePush, value: 3)))
+        }
+
+        guard let goAwayFrame = try assertNoThrowWithValue(self.clientChannel.readOutbound(as: ByteBuffer.self)) else {
+            XCTFail("Did not receive GOAWAY frame")
+            return
+        }
+        XCTAssertNoThrow(try self.serverChannel.writeInbound(goAwayFrame))
+        try self.serverChannel.assertReceivedFrame().assertGoAwayFrame(lastStreamID: .maxID, errorCode: 0x01, opaqueData: nil)
+
+        XCTAssertNoThrow(XCTAssertTrue(try self.serverChannel.finish().isClean))
+        _ = try? self.clientChannel.finish()
+    }
 }