Browse Source

Tolerate missing content-type header for trailers-only responses (#927)

Motivation:

For trailers-only responses, where the server has 'fast-failed' an RPC,
the spec indicates that a content-type is required and must start with
"application/grpc". However, if this header is missing we will fail the
RPC with a status and message which differ from that sent back from the
server. This is somewhat unhelpful to the end user, as it hides the
real reason for the failure.

Modifications:

- Only validate the content-type for trailers-only responses if it is
  present
- Add tests (and a few additional tests related to trailers-only
  responses)

Result:

- Clients will see the status code and message sent from servers which do
  not include the content-type in their trailers-only responses.
- Resolves #926
George Barnett 5 years ago
parent
commit
e90e92fe0f

+ 6 - 3
Sources/GRPC/GRPCClientStateMachine.swift

@@ -34,7 +34,7 @@ enum ReceiveResponseHeadError: Error, Equatable {
   case invalidState
 }
 
-enum ReceiveEndOfResponseStreamError: Error {
+enum ReceiveEndOfResponseStreamError: Error, Equatable {
   /// The 'content-type' header was missing or the value is not supported by this implementation.
   case invalidContentType(String?)
 
@@ -719,8 +719,11 @@ extension GRPCClientStateMachine.State {
       }
     }
 
-    let contentTypeHeader = trailers.first(name: "content-type")
-    guard contentTypeHeader.map(ContentType.init) != nil else {
+    // Only validate the content-type header if it's present. This is a small deviation from the
+    // spec as the content-type is meant to be sent in "Trailers-Only" responses. However, if it's
+    // missing then we should avoid the error and propagate the status code and message sent by
+    // the server instead.
+    if let contentTypeHeader = trailers.first(name: "content-type"), ContentType(value: contentTypeHeader) == nil {
       return .failure(.invalidContentType(contentTypeHeader))
     }
 

+ 49 - 0
Tests/GRPCTests/GRPCClientStateMachineTests.swift

@@ -890,6 +890,55 @@ extension GRPCClientStateMachineTests {
       XCTAssertEqual(status.message, "foo bar 🚀")
     }
   }
+
+  func testReceiveTrailersOnlyEndOfResponseStreamWithoutContentType() throws {
+    var stateMachine = self.makeStateMachine(.clientActiveServerIdle(writeState: .one(), pendingReadState: .init(arity: .one, messageEncoding: .disabled)))
+
+    let trailers: HPACKHeaders = [
+      ":status": "200",
+      "grpc-status": "5",
+      "grpc-message": "foo bar 🚀"
+    ]
+    stateMachine.receiveEndOfResponseStream(trailers).assertSuccess { status in
+      XCTAssertEqual(status.code, GRPCStatus.Code(rawValue: 5))
+      XCTAssertEqual(status.message, "foo bar 🚀")
+    }
+  }
+
+  func testReceiveTrailersOnlyEndOfResponseStreamWithInvalidContentType() throws {
+    var stateMachine = self.makeStateMachine(.clientActiveServerIdle(writeState: .one(), pendingReadState: .init(arity: .one, messageEncoding: .disabled)))
+
+    let trailers: HPACKHeaders = [
+      ":status": "200",
+      "grpc-status": "5",
+      "grpc-message": "foo bar 🚀",
+      "content-type": "invalid"
+    ]
+    stateMachine.receiveEndOfResponseStream(trailers).assertFailure { error in
+      XCTAssertEqual(error, .invalidContentType("invalid"))
+    }
+  }
+
+  func testReceiveTrailersOnlyEndOfResponseStreamWithInvalidHTTPStatusAndValidGRPCStatus() throws {
+    var stateMachine = self.makeStateMachine(.clientActiveServerIdle(writeState: .one(), pendingReadState: .init(arity: .one, messageEncoding: .disabled)))
+
+    let trailers: HPACKHeaders = [
+      ":status": "418",
+      "grpc-status": "5",
+    ]
+    stateMachine.receiveEndOfResponseStream(trailers).assertFailure { error in
+      XCTAssertEqual(error, .invalidHTTPStatusWithGRPCStatus(GRPCStatus(code: GRPCStatus.Code(rawValue: 5)!, message: nil)))
+    }
+  }
+
+  func testReceiveTrailersOnlyEndOfResponseStreamWithInvalidHTTPStatusAndNoGRPCStatus() throws {
+    var stateMachine = self.makeStateMachine(.clientActiveServerIdle(writeState: .one(), pendingReadState: .init(arity: .one, messageEncoding: .disabled)))
+
+    let trailers: HPACKHeaders = [":status": "418"]
+    stateMachine.receiveEndOfResponseStream(trailers).assertFailure { error in
+      XCTAssertEqual(error, .invalidHTTPStatus("418"))
+    }
+  }
 }
 
 class ReadStateTests: GRPCTestCase {

+ 4 - 0
Tests/GRPCTests/XCTestManifests.swift

@@ -418,6 +418,10 @@ extension GRPCClientStateMachineTests {
         ("testReceiveResponseHeadersWithUnsupportedCompressionMechanism", testReceiveResponseHeadersWithUnsupportedCompressionMechanism),
         ("testReceiveTooManyRequests", testReceiveTooManyRequests),
         ("testReceiveTooManyRequestsInOneBuffer", testReceiveTooManyRequestsInOneBuffer),
+        ("testReceiveTrailersOnlyEndOfResponseStreamWithInvalidContentType", testReceiveTrailersOnlyEndOfResponseStreamWithInvalidContentType),
+        ("testReceiveTrailersOnlyEndOfResponseStreamWithInvalidHTTPStatusAndNoGRPCStatus", testReceiveTrailersOnlyEndOfResponseStreamWithInvalidHTTPStatusAndNoGRPCStatus),
+        ("testReceiveTrailersOnlyEndOfResponseStreamWithInvalidHTTPStatusAndValidGRPCStatus", testReceiveTrailersOnlyEndOfResponseStreamWithInvalidHTTPStatusAndValidGRPCStatus),
+        ("testReceiveTrailersOnlyEndOfResponseStreamWithoutContentType", testReceiveTrailersOnlyEndOfResponseStreamWithoutContentType),
         ("testSendEndOfRequestStreamFromActive", testSendEndOfRequestStreamFromActive),
         ("testSendEndOfRequestStreamFromClientActiveServerIdle", testSendEndOfRequestStreamFromClientActiveServerIdle),
         ("testSendEndOfRequestStreamFromClientClosedServerActive", testSendEndOfRequestStreamFromClientClosedServerActive),