[HTTPDecoder] Decode informal headers correctly

Author: fabianfett

Sources/NIOHTTP1/HTTPDecoder.swift

@@ -293,16 +293,25 @@ private class BetterHTTPParser {
             // does not meet the requirement of RFC 7230. This is an outstanding http_parser issue:
             // https://github.com/nodejs/http-parser/issues/251. As a result, we check for these status
             // codes and override http_parser's handling as well.
-            guard let method = self.requestHeads.popFirst()?.method else {
+            guard !self.requestHeads.isEmpty else {
                 self.richerError = NIOHTTPDecoderError.unsolicitedResponse
                 return .error(HPE_UNKNOWN)
             }
-
-            if method == .HEAD || method == .CONNECT {
-                skipBody = true
-            } else if statusCode / 100 == 1 ||  // 1XX codes
-                statusCode == 204 || statusCode == 304 {
+            
+            if (HTTPResponseStatus.continue.code <= statusCode && statusCode < HTTPResponseStatus.ok.code)
+                && statusCode != HTTPResponseStatus.switchingProtocols.code {
+                // if the response status is in the range of 100..<200 but not 101 we don't want to
+                // pop the request method. The actual request head is expected with the next HTTP
+                // head.
                 skipBody = true
+            } else {
+                let method = self.requestHeads.removeFirst().method
+                if method == .HEAD || method == .CONNECT {
+                    skipBody = true
+                } else if statusCode / 100 == 1 ||  // 1XX codes
+                    statusCode == 204 || statusCode == 304 {
+                    skipBody = true
+                }
             }
         }
 
@@ -473,15 +482,36 @@ public final class HTTPDecoder<In, Out>: ByteToMessageDecoder, HTTPDecoderDelega
     // the actual state
     private let parser: BetterHTTPParser
     private let leftOverBytesStrategy: RemoveAfterUpgradeStrategy
+    private let informalResponseStrategy: InformalResponseStrategy
     private let kind: HTTPDecoderKind
     private var stopParsing = false // set on upgrade or HTTP version error
+    private var lastHeaderWasInformal = false
 
     /// Creates a new instance of `HTTPDecoder`.
     ///
     /// - parameters:
     ///     - leftOverBytesStrategy: The strategy to use when removing the decoder from the pipeline and an upgrade was,
     ///                              detected. Note that this does not affect what happens on EOF.
-    public init(leftOverBytesStrategy: RemoveAfterUpgradeStrategy = .dropBytes) {
+    public convenience init(leftOverBytesStrategy: RemoveAfterUpgradeStrategy = .dropBytes) {
+        self.init(leftOverBytesStrategy: leftOverBytesStrategy, informalResponseStrategy: .drop)
+    }
+    
+    /// Strategy to use when a HTTPDecoder receives an informal HTTP response (1xx except 101)
+    public enum InformalResponseStrategy {
+        /// Drop the informal response and only forward the "real" response
+        case drop
+        /// Forward the informal response and then forward the "real" response
+        case forward
+    }
+    
+    /// Creates a new instance of `HTTPDecoder`.
+    ///
+    /// - parameters:
+    ///     - leftOverBytesStrategy: The strategy to use when removing the decoder from the pipeline and an upgrade was,
+    ///                              detected. Note that this does not affect what happens on EOF.
+    ///     - supportInformalResponses: Should informal responses (like http status 100) be forwarded or dropped. Default is `.drop`
+    ///                              This property is only respected when decoding responses.
+    public init(leftOverBytesStrategy: RemoveAfterUpgradeStrategy = .dropBytes, informalResponseStrategy: InformalResponseStrategy = .drop) {
         self.headers.reserveCapacity(16)
         if In.self == HTTPServerRequestPart.self {
             self.kind = .request
@@ -492,6 +522,7 @@ public final class HTTPDecoder<In, Out>: ByteToMessageDecoder, HTTPDecoderDelega
         }
         self.parser = BetterHTTPParser(kind: kind)
         self.leftOverBytesStrategy = leftOverBytesStrategy
+        self.informalResponseStrategy = informalResponseStrategy
     }
 
     func didReceiveBody(_ bytes: UnsafeRawBufferPointer) {
@@ -545,7 +576,7 @@ public final class HTTPDecoder<In, Out>: ByteToMessageDecoder, HTTPDecoderDelega
                        method: http_method,
                        statusCode: Int,
                        keepAliveState: KeepAliveState) -> Bool {
-        let message: NIOAny
+        let message: NIOAny?
 
         guard versionMajor == 1 else {
             self.stopParsing = true
@@ -561,7 +592,23 @@ public final class HTTPDecoder<In, Out>: ByteToMessageDecoder, HTTPDecoderDelega
                                           headers: HTTPHeaders(self.headers,
                                                                keepAliveState: keepAliveState))
             message = NIOAny(HTTPServerRequestPart.head(reqHead))
+            
+        case .response where (100..<200).contains(statusCode) && statusCode != 101:
+            self.lastHeaderWasInformal = true
+            
+            switch self.informalResponseStrategy {
+            case .forward:
+                let resHead: HTTPResponseHead = HTTPResponseHead(version: .init(major: versionMajor, minor: versionMinor),
+                                                                 status: .init(statusCode: statusCode),
+                                                                 headers: HTTPHeaders(self.headers,
+                                                                                      keepAliveState: keepAliveState))
+                message = NIOAny(HTTPClientResponsePart.head(resHead))
+            case .drop:
+                message = nil
+            }
+            
         case .response:
+            self.lastHeaderWasInformal = false
             let resHead: HTTPResponseHead = HTTPResponseHead(version: .init(major: versionMajor, minor: versionMinor),
                                                              status: .init(statusCode: statusCode),
                                                              headers: HTTPHeaders(self.headers,
@@ -570,7 +617,9 @@ public final class HTTPDecoder<In, Out>: ByteToMessageDecoder, HTTPDecoderDelega
         }
         self.url = nil
         self.headers.removeAll(keepingCapacity: true)
-        self.context!.fireChannelRead(message)
+        if let message = message {
+            self.context!.fireChannelRead(message)
+        }
         self.isUpgrade = isUpgrade
         return true
     }
@@ -582,7 +631,9 @@ public final class HTTPDecoder<In, Out>: ByteToMessageDecoder, HTTPDecoderDelega
         case .request:
             self.context!.fireChannelRead(NIOAny(HTTPServerRequestPart.end(trailers.map(HTTPHeaders.init))))
         case .response:
-            self.context!.fireChannelRead(NIOAny(HTTPClientResponsePart.end(trailers.map(HTTPHeaders.init))))
+            if !self.lastHeaderWasInformal {
+                self.context!.fireChannelRead(NIOAny(HTTPClientResponsePart.end(trailers.map(HTTPHeaders.init))))
+            }
         }
         self.stopParsing = self.isUpgrade!
         self.isUpgrade = nil

Tests/NIOHTTP1Tests/HTTPDecoderLengthTest.swift

@@ -184,7 +184,8 @@ class HTTPDecoderLengthTest: XCTestCase {
                                            responseStatus: HTTPResponseStatus,
                                            responseFramingField: FramingField) throws {
         XCTAssertNoThrow(try channel.pipeline.addHandler(HTTPRequestEncoder()).wait())
-        XCTAssertNoThrow(try channel.pipeline.addHandler(ByteToMessageHandler(HTTPResponseDecoder())).wait())
+        let decoder = HTTPResponseDecoder(leftOverBytesStrategy: .dropBytes, informalResponseStrategy: .forward)
+        XCTAssertNoThrow(try channel.pipeline.addHandler(ByteToMessageHandler(decoder)).wait())
 
         let handler = MessageEndHandler<HTTPResponseHead, ByteBuffer>()
         XCTAssertNoThrow(try channel.pipeline.addHandler(handler).wait())
@@ -214,9 +215,18 @@ class HTTPDecoderLengthTest: XCTestCase {
 
         // We should have a response, no body, and immediately see EOF.
         XCTAssert(handler.seenHead)
-        XCTAssertFalse(handler.seenBody)
-        XCTAssert(handler.seenEnd)
-
+        switch responseStatus.code {
+        case 100, 102..<200:
+            // If an informal header is tested, we expect another "real" header to follow. For this
+            // reason, we don't expect an `.end` here.
+            XCTAssertFalse(handler.seenBody)
+            XCTAssertFalse(handler.seenEnd)
+            
+        default:
+            XCTAssertFalse(handler.seenBody)
+            XCTAssert(handler.seenEnd)
+        }
+        
         XCTAssertTrue(try channel.finish().isClean)
     }
 

Tests/NIOHTTP1Tests/HTTPDecoderTest+XCTest.swift

@@ -54,6 +54,8 @@ extension HTTPDecoderTest {
                 ("testAppropriateErrorWhenReceivingUnsolicitedResponse", testAppropriateErrorWhenReceivingUnsolicitedResponse),
                 ("testAppropriateErrorWhenReceivingUnsolicitedResponseDoesNotRecover", testAppropriateErrorWhenReceivingUnsolicitedResponseDoesNotRecover),
                 ("testOneRequestTwoResponses", testOneRequestTwoResponses),
+                ("testForwardContinueThanResponse", testForwardContinueThanResponse),
+                ("testDropContinueThanForwardResponse", testDropContinueThanForwardResponse),
                 ("testRefusesRequestSmugglingAttempt", testRefusesRequestSmugglingAttempt),
                 ("testTrimsTrailingOWS", testTrimsTrailingOWS),
                 ("testMassiveChunkDoesNotBufferAndGivesUsHoweverMuchIsAvailable", testMassiveChunkDoesNotBufferAndGivesUsHoweverMuchIsAvailable),

Tests/NIOHTTP1Tests/HTTPDecoderTest.swift

@@ -792,6 +792,58 @@ class HTTPDecoderTest: XCTestCase {
         XCTAssertEqual(["channelReadComplete", "write", "flush", "channelRead", "errorCaught"], eventCounter.allTriggeredEvents())
         XCTAssertNoThrow(XCTAssertTrue(try channel.finish().isClean))
     }
+    
+    func testForwardContinueThanResponse() {
+        let eventCounter = EventCounterHandler()
+        let responseDecoder = ByteToMessageHandler(HTTPResponseDecoder(leftOverBytesStrategy: .dropBytes, informalResponseStrategy: .forward))
+        let channel = EmbeddedChannel(handler: responseDecoder)
+        XCTAssertNoThrow(try channel.pipeline.addHandler(eventCounter).wait())
+
+        let requestHead: HTTPClientRequestPart = .head(.init(version: .http1_1, method: .POST, uri: "/"))
+        XCTAssertNoThrow(try channel.writeOutbound(requestHead))
+        var buffer = channel.allocator.buffer(capacity: 128)
+        buffer.writeString("HTTP/1.1 100 continue\r\n\r\nHTTP/1.1 200 ok\r\ncontent-length: 0\r\n\r\n")
+        XCTAssertNoThrow(try channel.writeInbound(buffer))
+        
+        XCTAssertEqual(try channel.readInbound(as: HTTPClientResponsePart.self), .head(.init(version: .http1_1, status: .continue)))
+        XCTAssertEqual(try channel.readInbound(as: HTTPClientResponsePart.self), .head(.init(version: .http1_1, status: .ok, headers: ["content-length": "0"])))
+        XCTAssertEqual(.end(nil), try channel.readInbound(as: HTTPClientResponsePart.self))
+        XCTAssertNil(try channel.readInbound(as: HTTPClientResponsePart.self))
+        XCTAssertNotNil(try channel.readOutbound())
+        
+        XCTAssertEqual(1, eventCounter.writeCalls)
+        XCTAssertEqual(1, eventCounter.flushCalls)
+        XCTAssertEqual(3, eventCounter.channelReadCalls) // .head, .head & .end
+        XCTAssertEqual(1, eventCounter.channelReadCompleteCalls)
+        XCTAssertEqual(["channelReadComplete", "channelRead", "write", "flush"], eventCounter.allTriggeredEvents())
+        XCTAssertNoThrow(XCTAssertTrue(try channel.finish().isClean))
+    }
+    
+    func testDropContinueThanForwardResponse() {
+        let eventCounter = EventCounterHandler()
+        let responseDecoder = ByteToMessageHandler(HTTPResponseDecoder(leftOverBytesStrategy: .dropBytes, informalResponseStrategy: .drop))
+        let channel = EmbeddedChannel(handler: responseDecoder)
+        XCTAssertNoThrow(try channel.pipeline.addHandler(eventCounter).wait())
+
+        let requestHead: HTTPClientRequestPart = .head(.init(version: .http1_1, method: .POST, uri: "/"))
+        XCTAssertNoThrow(try channel.writeOutbound(requestHead))
+        var buffer = channel.allocator.buffer(capacity: 128)
+        buffer.writeString("HTTP/1.1 100 continue\r\n\r\nHTTP/1.1 200 ok\r\ncontent-length: 0\r\n\r\n")
+        XCTAssertNoThrow(try channel.writeInbound(buffer))
+        
+        XCTAssertEqual(try channel.readInbound(as: HTTPClientResponsePart.self), .head(.init(version: .http1_1, status: .ok, headers: ["content-length": "0"])))
+        XCTAssertEqual(.end(nil), try channel.readInbound(as: HTTPClientResponsePart.self))
+        XCTAssertNil(try channel.readInbound(as: HTTPClientResponsePart.self))
+        XCTAssertNotNil(try channel.readOutbound())
+        
+        XCTAssertEqual(1, eventCounter.writeCalls)
+        XCTAssertEqual(1, eventCounter.flushCalls)
+        XCTAssertEqual(2, eventCounter.channelReadCalls) // .head & .end
+        XCTAssertEqual(1, eventCounter.channelReadCompleteCalls)
+        XCTAssertEqual(["channelReadComplete", "channelRead", "write", "flush"], eventCounter.allTriggeredEvents())
+        XCTAssertNoThrow(XCTAssertTrue(try channel.finish().isClean))
+    }
+
 
     func testRefusesRequestSmugglingAttempt() throws {
         XCTAssertNoThrow(try channel.pipeline.addHandler(ByteToMessageHandler(HTTPRequestDecoder())).wait())