always overwrite Transport-Encoding and Content-Length

dnadoba · dnadoba · commit afad35fc06b8 · 2021-11-22T14:41:15.000+01:00
diff --git a/Sources/AsyncHTTPClient/HTTPClient.swift b/Sources/AsyncHTTPClient/HTTPClient.swift
@@ -883,8 +883,8 @@ public struct HTTPClientError: Error, Equatable, CustomStringConvertible {
         case remoteConnectionClosed
         case cancelled
         case identityCodingIncorrectlyPresent
+        @available(*, deprecated)
         case chunkedSpecifiedMultipleTimes
-        case transferEncodingSpecifiedButChunkedIsNotTheFinalEncoding
         case invalidProxyResponse
         case contentLengthMissing
         case proxyAuthenticationRequired
@@ -895,6 +895,7 @@ public struct HTTPClientError: Error, Equatable, CustomStringConvertible {
         case invalidHeaderFieldNames([String])
         case bodyLengthMismatch
         case writeAfterRequestSent
+        @available(*, deprecated)
         case incompatibleHeaders
         case connectTimeout
         case socksHandshakeTimeout
@@ -938,9 +939,8 @@ 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)
     public static let chunkedSpecifiedMultipleTimes = HTTPClientError(code: .chunkedSpecifiedMultipleTimes)
-    /// Request specifies `Transfer-Encoding` but `chunked` is not the final encoding
-    public static let transferEncodingSpecifiedButChunkedIsNotTheFinalEncoding = HTTPClientError(code: .transferEncodingSpecifiedButChunkedIsNotTheFinalEncoding)
     /// Proxy response was invalid.
     public static let invalidProxyResponse = HTTPClientError(code: .invalidProxyResponse)
     /// Request does not contain `Content-Length` header.
@@ -962,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)
     public static let incompatibleHeaders = HTTPClientError(code: .incompatibleHeaders)
     /// Creating a new tcp connection timed out
     public static let connectTimeout = HTTPClientError(code: .connectTimeout)
diff --git a/Sources/AsyncHTTPClient/HTTPHandler.swift b/Sources/AsyncHTTPClient/HTTPHandler.swift
@@ -308,7 +308,7 @@ extension HTTPClient {
                 head.headers.add(name: "host", value: host)
             }
 
-            let metadata = try head.headers.validateAndFixTransportFraming(method: self.method, bodyLength: .init(self.body))
+            let metadata = try head.headers.validateAndSetTransportFraming(method: self.method, bodyLength: .init(self.body))
 
             return (head, metadata)
         }
diff --git a/Sources/AsyncHTTPClient/RequestValidation.swift b/Sources/AsyncHTTPClient/RequestValidation.swift
@@ -16,79 +16,35 @@ import NIOCore
 import NIOHTTP1
 
 extension HTTPHeaders {
-    mutating func validateAndFixTransportFraming(
+    mutating func validateAndSetTransportFraming(
         method: HTTPMethod,
         bodyLength: RequestBodyLength
     ) throws -> RequestFramingMetadata {
-        let contentLength = self.first(name: "Content-Length")
-        let encodings = self[canonicalForm: "Transfer-Encoding"]
-
-        // "Transfer-Encoding" and "Content-Length" are not allowed to present at the same time (https://tools.ietf.org/html/rfc7230#section-3.3.1)
-        guard encodings.isEmpty || contentLength == nil else {
-            throw HTTPClientError.incompatibleHeaders
-        }
-
         try self.validateFieldNames()
-        try Self.validateTransferEncoding(encodings)
-
-        if contentLength != nil {
-            self.remove(name: "Content-Length")
-        }
-        if !encodings.isEmpty {
-            self.remove(name: "Transfer-Encoding")
-        }
-
-        let connectionClose = self[canonicalForm: "connection"].lazy.map { $0.lowercased() }.contains("close")
-
-        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.
+        
+        if case .TRACE = method {
+            switch bodyLength {
+            case .fixed(length: 0):
                 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")
-            }
-            return .init(connectionClose: connectionClose, body: .fixedSize(0))
-        case .fixed(let length):
-            if case .TRACE = method {
+            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
             }
-            if encodings.isEmpty {
-                self.add(name: "Content-Length", value: String(length))
-                return .init(connectionClose: connectionClose, body: .fixedSize(length))
-            } else {
-                self.add(name: "Transfer-Encoding", value: encodings.joined(separator: ", "))
-                return .init(connectionClose: connectionClose, body: .stream)
-            }
+        }
+        
+        self.setTransportFraming(method: method, bodyLength: bodyLength)
+        
+        let connectionClose = self[canonicalForm: "connection"].lazy.map { $0.lowercased() }.contains("close")
+        switch bodyLength {
         case .dynamic:
-            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
-            }
-
-            if encodings.isEmpty && contentLength == nil {
-                // if a user forgot to specify a Content-Length and Transfer-Encoding, we will set it for them
-                self.add(name: "Transfer-Encoding", value: "chunked")
-            } else {
-                self.add(name: "Transfer-Encoding", value: encodings.joined(separator: ", "))
-            }
-
             return .init(connectionClose: connectionClose, body: .stream)
+        case .fixed(let length):
+            return .init(connectionClose: connectionClose, body: .fixedSize(length))
         }
     }
 
-    func validateFieldNames() throws {
+    private func validateFieldNames() throws {
         let invalidFieldNames = self.compactMap { (name, _) -> String? in
             let satisfy = name.utf8.allSatisfy { (char) -> Bool in
                 switch char {
@@ -123,33 +79,33 @@ extension HTTPHeaders {
             throw HTTPClientError.invalidHeaderFieldNames(invalidFieldNames)
         }
     }
-
-    static func validateTransferEncoding<Encodings>(
-        _ encodings: Encodings
-    ) throws where Encodings: Sequence, Encodings.Element: StringProtocol {
-        let encodings = encodings.map { $0.lowercased() }
-
-        guard !encodings.contains("identity") else {
-            throw HTTPClientError.identityCodingIncorrectlyPresent
-        }
-
-        // If `Transfer-Encoding` is specified, `chunked` needs to be the last encoding and should not be specified multiple times
-        // https://datatracker.ietf.org/doc/html/rfc7230#section-3.3.1
-        let chunkedEncodingCount = encodings.lazy.filter { $0 == "chunked" }.count
-        switch chunkedEncodingCount {
-        case 0:
-            if !encodings.isEmpty {
-                throw HTTPClientError.transferEncodingSpecifiedButChunkedIsNotTheFinalEncoding
-            }
-        case 1:
-            guard encodings.last == "chunked" else {
-                throw HTTPClientError.transferEncodingSpecifiedButChunkedIsNotTheFinalEncoding
+    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 2...:
-            throw HTTPClientError.chunkedSpecifiedMultipleTimes
-        default:
-            // unreachable because `chunkedEncodingCount` is guaranteed to be positive
-            preconditionFailure()
+        case .fixed(let length):
+            self.add(name: "Content-Length", value: String(length))
+        case .dynamic:
+            self.add(name: "Transfer-Encoding", value: "chunked")
         }
     }
 }
diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift
@@ -128,7 +128,7 @@ extension HTTPClientTests {
             ("testErrorAfterCloseWhileBackpressureExerted", testErrorAfterCloseWhileBackpressureExerted),
             ("testRequestSpecificTLS", testRequestSpecificTLS),
             ("testConnectionPoolSizeConfigValueIsRespected", testConnectionPoolSizeConfigValueIsRespected),
-            ("testRequestWithHeaderTransferEncodingIdentityFails", testRequestWithHeaderTransferEncodingIdentityFails),
+            ("testRequestWithHeaderTransferEncodingIdentityDoesNotFail", testRequestWithHeaderTransferEncodingIdentityDoesNotFail),
         ]
     }
 }
diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/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()) }
+        
+        let httpBin = HTTPBin()
+        defer { XCTAssertNoThrow(try httpBin.shutdown()) }
 
-        guard var request = try? Request(url: "http://localhost/get") else {
+        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())
     }
 }
diff --git a/Tests/AsyncHTTPClientTests/RequestValidationTests+XCTest.swift b/Tests/AsyncHTTPClientTests/RequestValidationTests+XCTest.swift
@@ -45,9 +45,8 @@ extension RequestValidationTests {
             ("testHostHeaderIsSetCorrectlyInCreateRequestHead", testHostHeaderIsSetCorrectlyInCreateRequestHead),
             ("testTraceMethodIsNotAllowedToHaveAFixedLengthBody", testTraceMethodIsNotAllowedToHaveAFixedLengthBody),
             ("testTraceMethodIsNotAllowedToHaveADynamicLengthBody", testTraceMethodIsNotAllowedToHaveADynamicLengthBody),
-            ("testTransferEncodingsArePreservedIfBodyLengthIsFixed", testTransferEncodingsArePreservedIfBodyLengthIsFixed),
-            ("testTransferEncodingsArePreservedIfBodyLengthIsDynamic", testTransferEncodingsArePreservedIfBodyLengthIsDynamic),
-            ("testTransferEncodingValidation", testTransferEncodingValidation),
+            ("testTransferEncodingsAreOverwritteIfBodyLengthIsFixed", testTransferEncodingsAreOverwritteIfBodyLengthIsFixed),
+            ("testTransferEncodingsAreOverwrittenIfBodyLengthIsDynamic", testTransferEncodingsAreOverwrittenIfBodyLengthIsDynamic),
         ]
     }
 }
diff --git a/Tests/AsyncHTTPClientTests/RequestValidationTests.swift b/Tests/AsyncHTTPClientTests/RequestValidationTests.swift

Original file line number	Diff line number	Diff line change
`@@ -308,7 +308,7 @@ extension HTTPClient {`
`308`	`308`	`head.headers.add(name: "host", value: host)`
`309`	`309`	`}`
`310`	`310`
`311`		`- let metadata = try head.headers.validateAndFixTransportFraming(method: self.method, bodyLength: .init(self.body))`
	`311`	`+ let metadata = try head.headers.validateAndSetTransportFraming(method: self.method, bodyLength: .init(self.body))`
`312`	`312`
`313`	`313`	`return (head, metadata)`
`314`	`314`	`}`
Original file line number	Diff line number	Diff line change
`@@ -128,7 +128,7 @@ extension HTTPClientTests {`
`128`	`128`	`("testErrorAfterCloseWhileBackpressureExerted", testErrorAfterCloseWhileBackpressureExerted),`
`129`	`129`	`("testRequestSpecificTLS", testRequestSpecificTLS),`
`130`	`130`	`("testConnectionPoolSizeConfigValueIsRespected", testConnectionPoolSizeConfigValueIsRespected),`
`131`		`- ("testRequestWithHeaderTransferEncodingIdentityFails", testRequestWithHeaderTransferEncodingIdentityFails),`
	`131`	`+ ("testRequestWithHeaderTransferEncodingIdentityDoesNotFail", testRequestWithHeaderTransferEncodingIdentityDoesNotFail),`
`132`	`132`	`]`
`133`	`133`	`}`
`134`	`134`	`}`
Original file line number	Diff line number	Diff line change
`@@ -45,9 +45,8 @@ extension RequestValidationTests {`
`45`	`45`	`("testHostHeaderIsSetCorrectlyInCreateRequestHead", testHostHeaderIsSetCorrectlyInCreateRequestHead),`
`46`	`46`	`("testTraceMethodIsNotAllowedToHaveAFixedLengthBody", testTraceMethodIsNotAllowedToHaveAFixedLengthBody),`
`47`	`47`	`("testTraceMethodIsNotAllowedToHaveADynamicLengthBody", testTraceMethodIsNotAllowedToHaveADynamicLengthBody),`
`48`		`- ("testTransferEncodingsArePreservedIfBodyLengthIsFixed", testTransferEncodingsArePreservedIfBodyLengthIsFixed),`
`49`		`- ("testTransferEncodingsArePreservedIfBodyLengthIsDynamic", testTransferEncodingsArePreservedIfBodyLengthIsDynamic),`
`50`		`- ("testTransferEncodingValidation", testTransferEncodingValidation),`
	`48`	`+ ("testTransferEncodingsAreOverwritteIfBodyLengthIsFixed", testTransferEncodingsAreOverwritteIfBodyLengthIsFixed),`
	`49`	`+ ("testTransferEncodingsAreOverwrittenIfBodyLengthIsDynamic", testTransferEncodingsAreOverwrittenIfBodyLengthIsDynamic),`
`51`	`50`	`]`
`52`	`51`	`}`
`53`	`52`	`}`