Skip to content

Commit 0ed00b8

Browse files
dnadobadnadoba
authored
Always overwrite Transport-Encoding and Content-Length headers (#479)
1 parent 8c48625 commit 0ed00b8

File tree

8 files changed

+178
-145
lines changed

8 files changed

+178
-145
lines changed

‎Sources/AsyncHTTPClient/ConnectionPool/RequestBodyLength.swift‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
//===----------------------------------------------------------------------===//
2+
//
3+
// This source file is part of the AsyncHTTPClient open source project
4+
//
5+
// Copyright (c) 2021 Apple Inc. and the AsyncHTTPClient project authors
6+
// Licensed under Apache License v2.0
7+
//
8+
// See LICENSE.txt for license information
9+
// See CONTRIBUTORS.txt for the list of AsyncHTTPClient project authors
10+
//
11+
// SPDX-License-Identifier: Apache-2.0
12+
//
13+
//===----------------------------------------------------------------------===//
14+
15+
enum RequestBodyLength: Hashable {
16+
/// size of the request body is not known before starting the request
17+
case dynamic
18+
/// size of the request body is fixed and exactly `length` bytes
19+
case fixed(length: Int)
20+
}

‎Sources/AsyncHTTPClient/HTTPClient.swift‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -883,6 +883,7 @@ public struct HTTPClientError: Error, Equatable, CustomStringConvertible {
883883
case remoteConnectionClosed
884884
case cancelled
885885
case identityCodingIncorrectlyPresent
886+
@available(*, deprecated, message: "AsyncHTTPClient now silently corrects this invalid header.")
886887
case chunkedSpecifiedMultipleTimes
887888
case invalidProxyResponse
888889
case contentLengthMissing
@@ -894,6 +895,7 @@ public struct HTTPClientError: Error, Equatable, CustomStringConvertible {
894895
case invalidHeaderFieldNames([String])
895896
case bodyLengthMismatch
896897
case writeAfterRequestSent
898+
@available(*, deprecated, message: "AsyncHTTPClient now silently corrects invalid headers.")
897899
case incompatibleHeaders
898900
case connectTimeout
899901
case socksHandshakeTimeout
@@ -937,6 +939,7 @@ public struct HTTPClientError: Error, Equatable, CustomStringConvertible {
937939
/// Request contains invalid identity encoding.
938940
public static let identityCodingIncorrectlyPresent = HTTPClientError(code: .identityCodingIncorrectlyPresent)
939941
/// Request contains multiple chunks definitions.
942+
@available(*, deprecated, message: "AsyncHTTPClient now silently corrects this invalid header.")
940943
public static let chunkedSpecifiedMultipleTimes = HTTPClientError(code: .chunkedSpecifiedMultipleTimes)
941944
/// Proxy response was invalid.
942945
public static let invalidProxyResponse = HTTPClientError(code: .invalidProxyResponse)
@@ -959,6 +962,7 @@ public struct HTTPClientError: Error, Equatable, CustomStringConvertible {
959962
/// Body part was written after request was fully sent.
960963
public static let writeAfterRequestSent = HTTPClientError(code: .writeAfterRequestSent)
961964
/// Incompatible headers specified, for example `Transfer-Encoding` and `Content-Length`.
965+
@available(*, deprecated, message: "AsyncHTTPClient now silently corrects invalid headers.")
962966
public static let incompatibleHeaders = HTTPClientError(code: .incompatibleHeaders)
963967
/// Creating a new tcp connection timed out
964968
public static let connectTimeout = HTTPClientError(code: .connectTimeout)

‎Sources/AsyncHTTPClient/HTTPHandler.swift‎

Lines changed: 19 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -43,8 +43,8 @@ extension HTTPClient {
4343
}
4444
}
4545

46-
/// Body size. Request validation will be failed with `HTTPClientErrors.contentLengthMissing` if nil,
47-
/// unless `Trasfer-Encoding: chunked` header is set.
46+
/// Body size. if nil,`Transfer-Encoding` will automatically be set to `chunked`. Otherwise a `Content-Length`
47+
/// header is set with the given `length`.
4848
public var length: Int?
4949
/// Body chunk provider.
5050
public var stream: (StreamWriter) -> EventLoopFuture<Void>
@@ -62,8 +62,8 @@ extension HTTPClient {
6262
/// Create and stream body using `StreamWriter`.
6363
///
6464
/// - parameters:
65-
/// - length: Body size. Request validation will be failed with `HTTPClientErrors.contentLengthMissing` if nil,
66-
/// unless `Transfer-Encoding: chunked` header is set.
65+
/// - length: Body size. If nil, `Transfer-Encoding` will automatically be set to `chunked`. Otherwise a `Content-Length`
66+
/// header is set with the given `length`.
6767
/// - stream: Body chunk provider.
6868
public static func stream(length: Int? = nil, _ stream: @escaping (StreamWriter) -> EventLoopFuture<Void>) -> Body {
6969
return Body(length: length, stream: stream)
@@ -309,7 +309,7 @@ extension HTTPClient {
309309
head.headers.add(name: "host", value: host)
310310
}
311311

312-
let metadata = try head.headers.validate(method: self.method, body: self.body)
312+
let metadata = try head.headers.validateAndSetTransportFraming(method: self.method, bodyLength: .init(self.body))
313313

314314
return (head, metadata)
315315
}
@@ -820,3 +820,17 @@ internal struct RedirectHandler<ResponseType> {
820820
}
821821
}
822822
}
823+
824+
extension RequestBodyLength {
825+
init(_ body: HTTPClient.Body?) {
826+
guard let body = body else {
827+
self = .fixed(length: 0)
828+
return
829+
}
830+
guard let length = body.length else {
831+
self = .dynamic
832+
return
833+
}
834+
self = .fixed(length: length)
835+
}
836+
}

‎Sources/AsyncHTTPClient/RequestValidation.swift‎

Lines changed: 49 additions & 77 deletions
Original file line numberDiff line numberDiff line change
@@ -16,90 +16,32 @@ import NIOCore
1616
import NIOHTTP1
1717

1818
extension HTTPHeaders {
19-
mutating func validate(method: HTTPMethod, body: HTTPClient.Body?) throws -> RequestFramingMetadata {
20-
var metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(0))
21-
22-
if self[canonicalForm: "connection"].lazy.map({ $0.lowercased() }).contains("close") {
23-
metadata.connectionClose = true
24-
}
25-
26-
// validate transfer encoding and content length (https://tools.ietf.org/html/rfc7230#section-3.3.1)
27-
if self.contains(name: "Transfer-Encoding"), self.contains(name: "Content-Length") {
28-
throw HTTPClientError.incompatibleHeaders
29-
}
30-
31-
var transferEncoding: String?
32-
var contentLength: Int?
33-
let encodings = self[canonicalForm: "Transfer-Encoding"].map { $0.lowercased() }
34-
35-
guard !encodings.contains("identity") else {
36-
throw HTTPClientError.identityCodingIncorrectlyPresent
37-
}
38-
39-
self.remove(name: "Transfer-Encoding")
40-
19+
mutating func validateAndSetTransportFraming(
20+
method: HTTPMethod,
21+
bodyLength: RequestBodyLength
22+
) throws -> RequestFramingMetadata {
4123
try self.validateFieldNames()
4224

43-
guard let body = body else {
44-
self.remove(name: "Content-Length")
45-
// if we don't have a body we might not need to send the Content-Length field
46-
// https://tools.ietf.org/html/rfc7230#section-3.3.2
47-
switch method {
48-
case .GET, .HEAD, .DELETE, .CONNECT, .TRACE:
49-
// A user agent SHOULD NOT send a Content-Length header field when the request
50-
// message does not contain a payload body and the method semantics do not
51-
// anticipate such a body.
52-
return metadata
53-
default:
54-
// A user agent SHOULD send a Content-Length in a request message when
55-
// no Transfer-Encoding is sent and the request method defines a meaning
56-
// for an enclosed payload body.
57-
self.add(name: "Content-Length", value: "0")
58-
return metadata
59-
}
60-
}
61-
6225
if case .TRACE = method {
63-
// A client MUST NOT send a message body in a TRACE request.
64-
// https://tools.ietf.org/html/rfc7230#section-4.3.8
65-
throw HTTPClientError.traceRequestWithBody
66-
}
67-
68-
guard (encodings.lazy.filter { $0 == "chunked" }.count <= 1) else {
69-
throw HTTPClientError.chunkedSpecifiedMultipleTimes
70-
}
71-
72-
if encodings.isEmpty {
73-
if let length = body.length {
74-
self.remove(name: "Content-Length")
75-
contentLength = length
76-
} else if !self.contains(name: "Content-Length") {
77-
transferEncoding = "chunked"
78-
}
79-
} else {
80-
self.remove(name: "Content-Length")
81-
82-
transferEncoding = encodings.joined(separator: ", ")
83-
if !encodings.contains("chunked") {
84-
guard let length = body.length else {
85-
throw HTTPClientError.contentLengthMissing
86-
}
87-
contentLength = length
26+
switch bodyLength {
27+
case .fixed(length: 0):
28+
break
29+
case .dynamic, .fixed:
30+
// A client MUST NOT send a message body in a TRACE request.
31+
// https://tools.ietf.org/html/rfc7230#section-4.3.8
32+
throw HTTPClientError.traceRequestWithBody
8833
}
8934
}
9035

91-
// add headers if required
92-
if let enc = transferEncoding {
93-
self.add(name: "Transfer-Encoding", value: enc)
94-
metadata.body = .stream
95-
} else if let length = contentLength {
96-
// A sender MUST NOT send a Content-Length header field in any message
97-
// that contains a Transfer-Encoding header field.
98-
self.add(name: "Content-Length", value: String(length))
99-
metadata.body = .fixedSize(length)
100-
}
36+
self.setTransportFraming(method: method, bodyLength: bodyLength)
10137

102-
return metadata
38+
let connectionClose = self[canonicalForm: "connection"].lazy.map { $0.lowercased() }.contains("close")
39+
switch bodyLength {
40+
case .dynamic:
41+
return .init(connectionClose: connectionClose, body: .stream)
42+
case .fixed(let length):
43+
return .init(connectionClose: connectionClose, body: .fixedSize(length))
44+
}
10345
}
10446

10547
private func validateFieldNames() throws {
@@ -137,4 +79,34 @@ extension HTTPHeaders {
13779
throw HTTPClientError.invalidHeaderFieldNames(invalidFieldNames)
13880
}
13981
}
82+
83+
private mutating func setTransportFraming(
84+
method: HTTPMethod,
85+
bodyLength: RequestBodyLength
86+
) {
87+
self.remove(name: "Content-Length")
88+
self.remove(name: "Transfer-Encoding")
89+
90+
switch bodyLength {
91+
case .fixed(0):
92+
// if we don't have a body we might not need to send the Content-Length field
93+
// https://tools.ietf.org/html/rfc7230#section-3.3.2
94+
switch method {
95+
case .GET, .HEAD, .DELETE, .CONNECT, .TRACE:
96+
// A user agent SHOULD NOT send a Content-Length header field when the request
97+
// message does not contain a payload body and the method semantics do not
98+
// anticipate such a body.
99+
break
100+
default:
101+
// A user agent SHOULD send a Content-Length in a request message when
102+
// no Transfer-Encoding is sent and the request method defines a meaning
103+
// for an enclosed payload body.
104+
self.add(name: "Content-Length", value: "0")
105+
}
106+
case .fixed(let length):
107+
self.add(name: "Content-Length", value: String(length))
108+
case .dynamic:
109+
self.add(name: "Transfer-Encoding", value: "chunked")
110+
}
111+
}
140112
}

‎Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,7 @@ extension HTTPClientTests {
128128
("testErrorAfterCloseWhileBackpressureExerted", testErrorAfterCloseWhileBackpressureExerted),
129129
("testRequestSpecificTLS", testRequestSpecificTLS),
130130
("testConnectionPoolSizeConfigValueIsRespected", testConnectionPoolSizeConfigValueIsRespected),
131-
("testRequestWithHeaderTransferEncodingIdentityFails", testRequestWithHeaderTransferEncodingIdentityFails),
131+
("testRequestWithHeaderTransferEncodingIdentityDoesNotFail", testRequestWithHeaderTransferEncodingIdentityDoesNotFail),
132132
]
133133
}
134134
}

‎Tests/AsyncHTTPClientTests/HTTPClientTests.swift‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2941,22 +2941,23 @@ class HTTPClientTests: XCTestCase {
29412941
XCTAssertEqual(httpBin.createdConnections, poolSize)
29422942
}
29432943

2944-
func testRequestWithHeaderTransferEncodingIdentityFails() {
2944+
func testRequestWithHeaderTransferEncodingIdentityDoesNotFail() {
29452945
let group = MultiThreadedEventLoopGroup(numberOfThreads: 1)
29462946
defer { XCTAssertNoThrow(try group.syncShutdownGracefully()) }
29472947

29482948
let client = HTTPClient(eventLoopGroupProvider: .shared(group))
29492949
defer { XCTAssertNoThrow(try client.syncShutdown()) }
29502950

2951-
guard var request = try? Request(url: "http://localhost/get") else {
2951+
let httpBin = HTTPBin()
2952+
defer { XCTAssertNoThrow(try httpBin.shutdown()) }
2953+
2954+
guard var request = try? Request(url: "http://127.0.0.1:\(httpBin.port)/get") else {
29522955
return XCTFail("Expected to have a request here.")
29532956
}
29542957
request.headers.add(name: "X-Test-Header", value: "X-Test-Value")
29552958
request.headers.add(name: "Transfer-Encoding", value: "identity")
29562959
request.body = .string("1234")
29572960

2958-
XCTAssertThrowsError(try client.execute(request: request).wait()) {
2959-
XCTAssertEqual($0 as? HTTPClientError, .identityCodingIncorrectlyPresent)
2960-
}
2961+
XCTAssertNoThrow(try client.execute(request: request).wait())
29612962
}
29622963
}

‎Tests/AsyncHTTPClientTests/RequestValidationTests+XCTest.swift‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,10 @@ extension RequestValidationTests {
4343
("testBothHeadersNoBody", testBothHeadersNoBody),
4444
("testBothHeadersHasBody", testBothHeadersHasBody),
4545
("testHostHeaderIsSetCorrectlyInCreateRequestHead", testHostHeaderIsSetCorrectlyInCreateRequestHead),
46+
("testTraceMethodIsNotAllowedToHaveAFixedLengthBody", testTraceMethodIsNotAllowedToHaveAFixedLengthBody),
47+
("testTraceMethodIsNotAllowedToHaveADynamicLengthBody", testTraceMethodIsNotAllowedToHaveADynamicLengthBody),
48+
("testTransferEncodingsAreOverwrittenIfBodyLengthIsFixed", testTransferEncodingsAreOverwrittenIfBodyLengthIsFixed),
49+
("testTransferEncodingsAreOverwrittenIfBodyLengthIsDynamic", testTransferEncodingsAreOverwrittenIfBodyLengthIsDynamic),
4650
]
4751
}
4852
}

0 commit comments

Comments
 (0)