Clients can't push! (#332)

Motivation:

Nowhere in the PUSH_PROMISE code do we explicitly check whether or not
we're a client or a server, and we don't provide special handling for
SETTINGS_ENABLE_PUSH contingent on that fact. As a result, we do
technically allow clients to push. This eventually gets caught, but not
until we've created an idle stream, which we shouldn't do.

Modifications:

- Forbid clients from pushing.

Result:

Clients won't be allowed to push.

Co-authored-by: George Barnett <[email protected]>

Author: Lukasa

Sources/NIOHTTP2/ConnectionStateMachine/FrameReceivingStates/ReceivingPushPromiseState.swift

@@ -68,8 +68,8 @@ extension ReceivingPushPromiseState {
 
     /// Whether the remote peer may push.
     var peerMayPush: Bool {
-        // In the case where we don't have local settings, we have to assume the default value, in which the peer may push.
-        return true
+        // In the case where we don't have local settings, we have to assume the default value, in which servers may push and clients may not.
+        return self.role == .client
     }
 }
 
@@ -93,6 +93,6 @@ extension ReceivingPushPromiseState where Self: LocallyQuiescingState {
 
 extension ReceivingPushPromiseState where Self: HasLocalSettings {
     var peerMayPush: Bool {
-        return self.localSettings.enablePush == 1
+        return self.localSettings.enablePush == 1 && self.role == .client
     }
 }

Sources/NIOHTTP2/ConnectionStateMachine/FrameSendingStates/SendingPushPromiseState.swift

@@ -18,6 +18,8 @@ import NIOHPACK
 ///
 /// This protocol should only be conformed to by states for the HTTP/2 connection state machine.
 protocol SendingPushPromiseState: HasFlowControlWindows {
+    var role: HTTP2ConnectionStateMachine.ConnectionRole { get }
+
     var headerBlockValidation: HTTP2ConnectionStateMachine.ValidationState { get }
 
     var streamState: ConnectionStreamState { get set }
@@ -67,8 +69,8 @@ extension SendingPushPromiseState {
 
     /// Whether we may push.
     var mayPush: Bool {
-        // In the case where we don't have remote settings, we have to assume the default value, in which case we may push.
-        return true
+        // In the case where we don't have remote settings, we have to assume the default value, in which case if we're a server we may push.
+        return self.role == .server
     }
 
 }
@@ -83,6 +85,6 @@ extension SendingPushPromiseState where Self: RemotelyQuiescingState {
 
 extension SendingPushPromiseState where Self: HasRemoteSettings {
     var mayPush: Bool {
-        return self.remoteSettings.enablePush == 1
+        return self.remoteSettings.enablePush == 1 && self.role == .server
     }
 }

Tests/NIOHTTP2Tests/ConnectionStateMachineTests+XCTest.swift

@@ -82,6 +82,7 @@ extension ConnectionStateMachineTests {
                 ("testUnknownSettingsAreIgnored", testUnknownSettingsAreIgnored),
                 ("testMaxConcurrentStreamsEnforcement", testMaxConcurrentStreamsEnforcement),
                 ("testDisablingPushPreventsPush", testDisablingPushPreventsPush),
+                ("testClientsCannotPush", testClientsCannotPush),
                 ("testRatchetingGoawayEvenWhenFullyQueisced", testRatchetingGoawayEvenWhenFullyQueisced),
                 ("testRatchetingGoawayForBothPeersEvenWhenFullyQuiesced", testRatchetingGoawayForBothPeersEvenWhenFullyQuiesced),
                 ("testClientTrailersMustHaveEndStreamSet", testClientTrailersMustHaveEndStreamSet),

Tests/NIOHTTP2Tests/ConnectionStateMachineTests.swift

@@ -237,29 +237,22 @@ class ConnectionStateMachineTests: XCTestCase {
 
         assertBadStreamStateTransition(type: .halfOpenRemoteLocalIdle, self.server.sendData(streamID: streamOne, contentLength: 0, flowControlledBytes: 0, isEndStreamSet: false))
         self.server = savedServerState
-        
-        assertBadStreamStateTransition(type: .halfOpenRemoteLocalIdle, self.server.receivePushPromise(originalStreamID: streamOne, childStreamID: streamTwo, headers: testHeaders))
-        self.server = savedServerState
 
         // Move state to fullyOpen
         let testSendHeaders: HPACKHeaders = [":status": "y"]
         assertSucceeds(self.server.sendHeaders(streamID: streamOne, headers: testSendHeaders, isEndStreamSet: false))
         savedServerState = self.server
 
-        let testPushPromiseHeaders: HPACKHeaders = ["test": "value"]
-        assertBadStreamStateTransition(type: .fullyOpen, self.server.receivePushPromise(originalStreamID: streamOne, childStreamID: streamTwo, headers: testPushPromiseHeaders))
-        self.server = savedServerState
-
         // Move state to halfClosedLocalPeerActive
         assertSucceeds(self.server.sendData(streamID: streamOne, contentLength: 0, flowControlledBytes: 0, isEndStreamSet: true))
         savedServerState = self.server
 
         assertBadStreamStateTransition(type: .halfClosedLocalPeerActive, self.server.sendData(streamID: streamOne, contentLength: 0, flowControlledBytes: 0, isEndStreamSet: true))
         self.server = savedServerState
+
+        let testPushPromiseHeaders: HPACKHeaders = ["test": "value"]
         assertBadStreamStateTransition(type: .halfClosedLocalPeerActive, self.server.sendPushPromise(originalStreamID: streamOne, childStreamID: streamTwo, headers: testPushPromiseHeaders))
         self.server = savedServerState
-        assertBadStreamStateTransition(type: .halfClosedLocalPeerActive, self.server.receivePushPromise(originalStreamID: streamOne, childStreamID: streamTwo, headers: testPushPromiseHeaders))
-        self.server = savedServerState
         let lastBadStreamStateTransition = assertBadStreamStateTransition(type: .halfClosedLocalPeerActive, self.server.sendHeaders(streamID: streamOne, headers: testPushPromiseHeaders, isEndStreamSet: false))
         self.server = savedServerState
 
@@ -1778,6 +1771,27 @@ class ConnectionStateMachineTests: XCTestCase {
                               self.server.receiveSettings(.settings([HTTP2Setting(parameter: .enablePush, value: 2)]), frameEncoder: &self.serverEncoder, frameDecoder: &self.serverDecoder))
     }
 
+    func testClientsCannotPush() {
+        let streamOne = HTTP2StreamID(1)
+        let streamTwo = HTTP2StreamID(2)
+
+        // Default settings.
+        assertSucceeds(self.client.sendSettings([]))
+        assertSucceeds(self.server.receiveSettings(.settings([]), frameEncoder: &self.serverEncoder, frameDecoder: &self.serverDecoder))
+        assertSucceeds(self.server.sendSettings([]))
+        assertSucceeds(self.client.receiveSettings(.settings([]), frameEncoder: &self.clientEncoder, frameDecoder: &self.clientDecoder))
+        assertSucceeds(self.client.receiveSettings(.ack, frameEncoder: &self.clientEncoder, frameDecoder: &self.clientDecoder))
+        assertSucceeds(self.server.receiveSettings(.ack, frameEncoder: &self.serverEncoder, frameDecoder: &self.serverDecoder))
+
+        // Client attempts to push, using a _server_ stream ID, with stream one not open. This should fail on both server and client.
+        assertConnectionError(type: .protocolError, self.client.sendPushPromise(originalStreamID: streamOne, childStreamID: streamTwo, headers: ConnectionStateMachineTests.requestHeaders))
+        assertConnectionError(type: .protocolError, self.server.receivePushPromise(originalStreamID: streamOne, childStreamID: streamTwo, headers: ConnectionStateMachineTests.requestHeaders))
+
+        // Client then sends a HEADERS frame on the stream it just pushed. This should also fail.
+        assertConnectionError(type: .protocolError, self.client.sendHeaders(streamID: streamTwo, headers: ConnectionStateMachineTests.requestHeaders, isEndStreamSet: false))
+        assertConnectionError(type: .protocolError, self.server.receiveHeaders(streamID: streamTwo, headers: ConnectionStateMachineTests.requestHeaders, isEndStreamSet: false))
+    }
+
     func testRatchetingGoawayEvenWhenFullyQueisced() {
         let streamOne = HTTP2StreamID(1)
         let streamThree = HTTP2StreamID(3)