Always overwrite Transport-Encoding and Content-Length headers (#479)

Author: dnadoba

Sources/AsyncHTTPClient/ConnectionPool/RequestBodyLength.swift

@@ -0,0 +1,20 @@
+//===----------------------------------------------------------------------===//
+//
+// This source file is part of the AsyncHTTPClient open source project
+//
+// Copyright (c) 2021 Apple Inc. and the AsyncHTTPClient project authors
+// Licensed under Apache License v2.0
+//
+// See LICENSE.txt for license information
+// See CONTRIBUTORS.txt for the list of AsyncHTTPClient project authors
+//
+// SPDX-License-Identifier: Apache-2.0
+//
+//===----------------------------------------------------------------------===//
+
+enum RequestBodyLength: Hashable {
+    /// size of the request body is not known before starting the request
+    case dynamic
+    /// size of the request body is fixed and exactly `length` bytes
+    case fixed(length: Int)
+}

Sources/AsyncHTTPClient/HTTPClient.swift

@@ -883,6 +883,7 @@ public struct HTTPClientError: Error, Equatable, CustomStringConvertible {
         case remoteConnectionClosed
         case cancelled
         case identityCodingIncorrectlyPresent
+        @available(*, deprecated, message: "AsyncHTTPClient now silently corrects this invalid header.")
         case chunkedSpecifiedMultipleTimes
         case invalidProxyResponse
         case contentLengthMissing
@@ -894,6 +895,7 @@ public struct HTTPClientError: Error, Equatable, CustomStringConvertible {
         case invalidHeaderFieldNames([String])
         case bodyLengthMismatch
         case writeAfterRequestSent
+        @available(*, deprecated, message: "AsyncHTTPClient now silently corrects invalid headers.")
         case incompatibleHeaders
         case connectTimeout
         case socksHandshakeTimeout
@@ -937,6 +939,7 @@ public struct HTTPClientError: Error, Equatable, CustomStringConvertible {
     /// Request contains invalid identity encoding.
     public static let identityCodingIncorrectlyPresent = HTTPClientError(code: .identityCodingIncorrectlyPresent)
     /// Request contains multiple chunks definitions.
+    @available(*, deprecated, message: "AsyncHTTPClient now silently corrects this invalid header.")
     public static let chunkedSpecifiedMultipleTimes = HTTPClientError(code: .chunkedSpecifiedMultipleTimes)
     /// Proxy response was invalid.
     public static let invalidProxyResponse = HTTPClientError(code: .invalidProxyResponse)
@@ -959,6 +962,7 @@ public struct HTTPClientError: Error, Equatable, CustomStringConvertible {
     /// Body part was written after request was fully sent.
     public static let writeAfterRequestSent = HTTPClientError(code: .writeAfterRequestSent)
     /// Incompatible headers specified, for example `Transfer-Encoding` and `Content-Length`.
+    @available(*, deprecated, message: "AsyncHTTPClient now silently corrects invalid headers.")
     public static let incompatibleHeaders = HTTPClientError(code: .incompatibleHeaders)
     /// Creating a new tcp connection timed out
     public static let connectTimeout = HTTPClientError(code: .connectTimeout)

Sources/AsyncHTTPClient/HTTPHandler.swift

@@ -43,8 +43,8 @@ extension HTTPClient {
             }
         }
 
-        /// Body size. Request validation will be failed with `HTTPClientErrors.contentLengthMissing` if nil,
-        /// unless `Trasfer-Encoding: chunked` header is set.
+        /// Body size. if nil,`Transfer-Encoding` will automatically be set to `chunked`. Otherwise a `Content-Length`
+        /// header is set with the given `length`.
         public var length: Int?
         /// Body chunk provider.
         public var stream: (StreamWriter) -> EventLoopFuture<Void>
@@ -62,8 +62,8 @@ extension HTTPClient {
         /// Create and stream body using `StreamWriter`.
         ///
         /// - parameters:
-        ///     - length: Body size. Request validation will be failed with `HTTPClientErrors.contentLengthMissing` if nil,
-        ///               unless `Transfer-Encoding: chunked` header is set.
+        ///     - length: Body size. If nil, `Transfer-Encoding` will automatically be set to `chunked`. Otherwise a `Content-Length`
+        /// header is set with the given `length`.
         ///     - stream: Body chunk provider.
         public static func stream(length: Int? = nil, _ stream: @escaping (StreamWriter) -> EventLoopFuture<Void>) -> Body {
             return Body(length: length, stream: stream)
@@ -309,7 +309,7 @@ extension HTTPClient {
                 head.headers.add(name: "host", value: host)
             }
 
-            let metadata = try head.headers.validate(method: self.method, body: self.body)
+            let metadata = try head.headers.validateAndSetTransportFraming(method: self.method, bodyLength: .init(self.body))
 
             return (head, metadata)
         }
@@ -820,3 +820,17 @@ internal struct RedirectHandler<ResponseType> {
         }
     }
 }
+
+extension RequestBodyLength {
+    init(_ body: HTTPClient.Body?) {
+        guard let body = body else {
+            self = .fixed(length: 0)
+            return
+        }
+        guard let length = body.length else {
+            self = .dynamic
+            return
+        }
+        self = .fixed(length: length)
+    }
+}

Sources/AsyncHTTPClient/RequestValidation.swift

@@ -16,90 +16,32 @@ import NIOCore
 import NIOHTTP1
 
 extension HTTPHeaders {
-    mutating func validate(method: HTTPMethod, body: HTTPClient.Body?) throws -> RequestFramingMetadata {
-        var metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(0))
-
-        if self[canonicalForm: "connection"].lazy.map({ $0.lowercased() }).contains("close") {
-            metadata.connectionClose = true
-        }
-
-        // validate transfer encoding and content length (https://tools.ietf.org/html/rfc7230#section-3.3.1)
-        if self.contains(name: "Transfer-Encoding"), self.contains(name: "Content-Length") {
-            throw HTTPClientError.incompatibleHeaders
-        }
-
-        var transferEncoding: String?
-        var contentLength: Int?
-        let encodings = self[canonicalForm: "Transfer-Encoding"].map { $0.lowercased() }
-
-        guard !encodings.contains("identity") else {
-            throw HTTPClientError.identityCodingIncorrectlyPresent
-        }
-
-        self.remove(name: "Transfer-Encoding")
-
+    mutating func validateAndSetTransportFraming(
+        method: HTTPMethod,
+        bodyLength: RequestBodyLength
+    ) throws -> RequestFramingMetadata {
         try self.validateFieldNames()
 
-        guard let body = body else {
-            self.remove(name: "Content-Length")
-            // if we don't have a body we might not need to send the Content-Length field
-            // https://tools.ietf.org/html/rfc7230#section-3.3.2
-            switch method {
-            case .GET, .HEAD, .DELETE, .CONNECT, .TRACE:
-                // A user agent SHOULD NOT send a Content-Length header field when the request
-                // message does not contain a payload body and the method semantics do not
-                // anticipate such a body.
-                return metadata
-            default:
-                // A user agent SHOULD send a Content-Length in a request message when
-                // no Transfer-Encoding is sent and the request method defines a meaning
-                // for an enclosed payload body.
-                self.add(name: "Content-Length", value: "0")
-                return metadata
-            }
-        }
-
         if case .TRACE = method {
-            // A client MUST NOT send a message body in a TRACE request.
-            // https://tools.ietf.org/html/rfc7230#section-4.3.8
-            throw HTTPClientError.traceRequestWithBody
-        }
-
-        guard (encodings.lazy.filter { $0 == "chunked" }.count <= 1) else {
-            throw HTTPClientError.chunkedSpecifiedMultipleTimes
-        }
-
-        if encodings.isEmpty {
-            if let length = body.length {
-                self.remove(name: "Content-Length")
-                contentLength = length
-            } else if !self.contains(name: "Content-Length") {
-                transferEncoding = "chunked"
-            }
-        } else {
-            self.remove(name: "Content-Length")
-
-            transferEncoding = encodings.joined(separator: ", ")
-            if !encodings.contains("chunked") {
-                guard let length = body.length else {
-                    throw HTTPClientError.contentLengthMissing
-                }
-                contentLength = length
+            switch bodyLength {
+            case .fixed(length: 0):
+                break
+            case .dynamic, .fixed:
+                // A client MUST NOT send a message body in a TRACE request.
+                // https://tools.ietf.org/html/rfc7230#section-4.3.8
+                throw HTTPClientError.traceRequestWithBody
             }
         }
 
-        // add headers if required
-        if let enc = transferEncoding {
-            self.add(name: "Transfer-Encoding", value: enc)
-            metadata.body = .stream
-        } else if let length = contentLength {
-            // A sender MUST NOT send a Content-Length header field in any message
-            // that contains a Transfer-Encoding header field.
-            self.add(name: "Content-Length", value: String(length))
-            metadata.body = .fixedSize(length)
-        }
+        self.setTransportFraming(method: method, bodyLength: bodyLength)
 
-        return metadata
+        let connectionClose = self[canonicalForm: "connection"].lazy.map { $0.lowercased() }.contains("close")
+        switch bodyLength {
+        case .dynamic:
+            return .init(connectionClose: connectionClose, body: .stream)
+        case .fixed(let length):
+            return .init(connectionClose: connectionClose, body: .fixedSize(length))
+        }
     }
 
     private func validateFieldNames() throws {
@@ -137,4 +79,34 @@ extension HTTPHeaders {
             throw HTTPClientError.invalidHeaderFieldNames(invalidFieldNames)
         }
     }
+
+    private mutating func setTransportFraming(
+        method: HTTPMethod,
+        bodyLength: RequestBodyLength
+    ) {
+        self.remove(name: "Content-Length")
+        self.remove(name: "Transfer-Encoding")
+
+        switch bodyLength {
+        case .fixed(0):
+            // if we don't have a body we might not need to send the Content-Length field
+            // https://tools.ietf.org/html/rfc7230#section-3.3.2
+            switch method {
+            case .GET, .HEAD, .DELETE, .CONNECT, .TRACE:
+                // A user agent SHOULD NOT send a Content-Length header field when the request
+                // message does not contain a payload body and the method semantics do not
+                // anticipate such a body.
+                break
+            default:
+                // A user agent SHOULD send a Content-Length in a request message when
+                // no Transfer-Encoding is sent and the request method defines a meaning
+                // for an enclosed payload body.
+                self.add(name: "Content-Length", value: "0")
+            }
+        case .fixed(let length):
+            self.add(name: "Content-Length", value: String(length))
+        case .dynamic:
+            self.add(name: "Transfer-Encoding", value: "chunked")
+        }
+    }
 }

Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift

@@ -128,7 +128,7 @@ extension HTTPClientTests {
             ("testErrorAfterCloseWhileBackpressureExerted", testErrorAfterCloseWhileBackpressureExerted),
             ("testRequestSpecificTLS", testRequestSpecificTLS),
             ("testConnectionPoolSizeConfigValueIsRespected", testConnectionPoolSizeConfigValueIsRespected),
-            ("testRequestWithHeaderTransferEncodingIdentityFails", testRequestWithHeaderTransferEncodingIdentityFails),
+            ("testRequestWithHeaderTransferEncodingIdentityDoesNotFail", testRequestWithHeaderTransferEncodingIdentityDoesNotFail),
         ]
     }
 }

Tests/AsyncHTTPClientTests/HTTPClientTests.swift

@@ -2941,22 +2941,23 @@ class HTTPClientTests: XCTestCase {
         XCTAssertEqual(httpBin.createdConnections, poolSize)
     }
 
-    func testRequestWithHeaderTransferEncodingIdentityFails() {
+    func testRequestWithHeaderTransferEncodingIdentityDoesNotFail() {
         let group = MultiThreadedEventLoopGroup(numberOfThreads: 1)
         defer { XCTAssertNoThrow(try group.syncShutdownGracefully()) }
 
         let client = HTTPClient(eventLoopGroupProvider: .shared(group))
         defer { XCTAssertNoThrow(try client.syncShutdown()) }
 
-        guard var request = try? Request(url: "http://localhost/get") else {
+        let httpBin = HTTPBin()
+        defer { XCTAssertNoThrow(try httpBin.shutdown()) }
+
+        guard var request = try? Request(url: "http://127.0.0.1:\(httpBin.port)/get") else {
             return XCTFail("Expected to have a request here.")
         }
         request.headers.add(name: "X-Test-Header", value: "X-Test-Value")
         request.headers.add(name: "Transfer-Encoding", value: "identity")
         request.body = .string("1234")
 
-        XCTAssertThrowsError(try client.execute(request: request).wait()) {
-            XCTAssertEqual($0 as? HTTPClientError, .identityCodingIncorrectlyPresent)
-        }
+        XCTAssertNoThrow(try client.execute(request: request).wait())
     }
 }

Tests/AsyncHTTPClientTests/RequestValidationTests+XCTest.swift

@@ -43,6 +43,10 @@ extension RequestValidationTests {
             ("testBothHeadersNoBody", testBothHeadersNoBody),
             ("testBothHeadersHasBody", testBothHeadersHasBody),
             ("testHostHeaderIsSetCorrectlyInCreateRequestHead", testHostHeaderIsSetCorrectlyInCreateRequestHead),
+            ("testTraceMethodIsNotAllowedToHaveAFixedLengthBody", testTraceMethodIsNotAllowedToHaveAFixedLengthBody),
+            ("testTraceMethodIsNotAllowedToHaveADynamicLengthBody", testTraceMethodIsNotAllowedToHaveADynamicLengthBody),
+            ("testTransferEncodingsAreOverwrittenIfBodyLengthIsFixed", testTransferEncodingsAreOverwrittenIfBodyLengthIsFixed),
+            ("testTransferEncodingsAreOverwrittenIfBodyLengthIsDynamic", testTransferEncodingsAreOverwrittenIfBodyLengthIsDynamic),
         ]
     }
 }