From ebc8fe1beb85598f627b35161f20b7462b861f8e Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Thu, 6 Oct 2022 20:13:27 +0200 Subject: [PATCH 1/8] Handle ResponseAccumulator not being able to buffer large response in memory Check content length header for early exit --- Sources/AsyncHTTPClient/HTTPHandler.swift | 46 ++++++++++- .../HTTPClientTestUtils.swift | 13 ++++ .../HTTPClientTests.swift | 76 +++++++++++++++++++ 3 files changed, 134 insertions(+), 1 deletion(-) diff --git a/Sources/AsyncHTTPClient/HTTPHandler.swift b/Sources/AsyncHTTPClient/HTTPHandler.swift index 744139f68..bcc39e01a 100644 --- a/Sources/AsyncHTTPClient/HTTPHandler.swift +++ b/Sources/AsyncHTTPClient/HTTPHandler.swift @@ -356,7 +356,7 @@ extension HTTPClient { /// /// This ``HTTPClientResponseDelegate`` buffers a complete HTTP response in memory. It does not stream the response body in. /// The resulting ``Response`` type is ``HTTPClient/Response``. -public class ResponseAccumulator: HTTPClientResponseDelegate { +final public class ResponseAccumulator: HTTPClientResponseDelegate { public typealias Response = HTTPClient.Response enum State { @@ -366,9 +366,33 @@ public class ResponseAccumulator: HTTPClientResponseDelegate { case end case error(Error) } + + public struct ResponseTooBigError: Error, CustomStringConvertible { + var maxBodySize: Int + + public var description: String { + return "ResponseTooBigError: received response body exceeds maximum accepted size of \(maxBodySize) bytes" + } + } var state = State.idle let request: HTTPClient.Request + + static let maxByteBufferSize = Int(UInt32.max) + + /// Maximum size in bytes of the HTTP response body that ``ResponseAccumulator`` will accept + /// until it will abort the request and throw an ``ResponseTooBigError``. + /// + /// Default is 2^32. + /// - precondition: not allowed to exceed 2^32 because ``ByteBuffer`` can not store more bytes + public var maxBodySize: Int = maxByteBufferSize { + didSet { + precondition( + maxBodySize <= Self.maxByteBufferSize, + "maxBodyLength is not allowed to exceed 2^32 because ByteBuffer can not store more bytes" + ) + } + } public init(request: HTTPClient.Request) { self.request = request @@ -377,6 +401,14 @@ public class ResponseAccumulator: HTTPClientResponseDelegate { public func didReceiveHead(task: HTTPClient.Task, _ head: HTTPResponseHead) -> EventLoopFuture { switch self.state { case .idle: + if let contentLength = head.headers.first(name: "Content-Length"), + let announcedBodySize = Int(contentLength), + announcedBodySize > self.maxBodySize { + let error = ResponseTooBigError(maxBodySize: maxBodySize) + self.state = .error(error) + return task.eventLoop.makeFailedFuture(error) + } + self.state = .head(head) case .head: preconditionFailure("head already set") @@ -395,8 +427,20 @@ public class ResponseAccumulator: HTTPClientResponseDelegate { case .idle: preconditionFailure("no head received before body") case .head(let head): + guard part.readableBytes <= self.maxBodySize else { + let error = ResponseTooBigError(maxBodySize: self.maxBodySize) + self.state = .error(error) + return task.eventLoop.makeFailedFuture(error) + } self.state = .body(head, part) case .body(let head, var body): + let newBufferSize = body.writerIndex + part.readableBytes + guard newBufferSize <= self.maxBodySize else { + let error = ResponseTooBigError(maxBodySize: self.maxBodySize) + self.state = .error(error) + return task.eventLoop.makeFailedFuture(error) + } + // The compiler can't prove that `self.state` is dead here (and it kinda isn't, there's // a cross-module call in the way) so we need to drop the original reference to `body` in // `self.state` or we'll get a CoW. To fix that we temporarily set the state to `.end` (which diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift index 91358a361..ca7cf2178 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift @@ -365,6 +365,19 @@ internal final class HTTPBin where var socketAddress: SocketAddress { return self.serverChannel.localAddress! } + + + var baseURL: String { + let scheme: String = { + switch mode { + case .http1_1, .refuse: + return "http" + case .http2: + return "https" + } + }() + return "\(scheme)://localhost:\(self.port)/" + } private let mode: Mode private let sslContext: NIOSSLContext? diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index 3c275c5eb..f214ffdc9 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -3091,6 +3091,82 @@ class HTTPClientTests: XCTestCase { XCTAssertNoThrow(try future.wait()) XCTAssertNil(try delegate.next().wait()) } + + func testResponseAccumulatorMaxBodySizeLimitExceedingWithContentLength() throws { + let httpBin = HTTPBin(.http1_1(ssl: false, compress: false)) { _ in HTTPEchoHandler() } + defer { XCTAssertNoThrow(try httpBin.shutdown()) } + + let body = ByteBuffer(bytes: 0..<11) + + var request = try Request(url: httpBin.baseURL) + request.body = .byteBuffer(body) + let delegate = ResponseAccumulator(request: request) + delegate.maxBodySize = 10 + XCTAssertThrowsError(try self.defaultClient.execute( + request: request, + delegate: delegate + ).wait()) { error in + XCTAssertTrue(error is ResponseAccumulator.ResponseTooBigError, "unexpected error \(error)") + } + } + + func testResponseAccumulatorMaxBodySizeLimitNotExceedingWithContentLength() throws { + let httpBin = HTTPBin(.http1_1(ssl: false, compress: false)) { _ in HTTPEchoHandler() } + defer { XCTAssertNoThrow(try httpBin.shutdown()) } + + let body = ByteBuffer(bytes: 0..<10) + + var request = try Request(url: httpBin.baseURL) + request.body = .byteBuffer(body) + let delegate = ResponseAccumulator(request: request) + delegate.maxBodySize = 10 + let response = try self.defaultClient.execute( + request: request, + delegate: delegate + ).wait() + + XCTAssertEqual(response.body, body) + } + + func testResponseAccumulatorMaxBodySizeLimitExceedingWithTransferEncodingChuncked() throws { + let httpBin = HTTPBin(.http1_1(ssl: false, compress: false)) { _ in HTTPEchoHandler() } + defer { XCTAssertNoThrow(try httpBin.shutdown()) } + + let body = ByteBuffer(bytes: 0..<11) + + var request = try Request(url: httpBin.baseURL) + request.body = .stream { writer in + writer.write(.byteBuffer(body)) + } + let delegate = ResponseAccumulator(request: request) + delegate.maxBodySize = 10 + XCTAssertThrowsError(try self.defaultClient.execute( + request: request, + delegate: delegate + ).wait()) { error in + XCTAssertTrue(error is ResponseAccumulator.ResponseTooBigError, "unexpected error \(error)") + } + } + + func testResponseAccumulatorMaxBodySizeLimitNotExceedingWithTransferEncodingChuncked() throws { + let httpBin = HTTPBin(.http1_1(ssl: false, compress: false)) { _ in HTTPEchoHandler() } + defer { XCTAssertNoThrow(try httpBin.shutdown()) } + + let body = ByteBuffer(bytes: 0..<10) + + var request = try Request(url: httpBin.baseURL) + request.body = .stream { writer in + writer.write(.byteBuffer(body)) + } + let delegate = ResponseAccumulator(request: request) + delegate.maxBodySize = 10 + let response = try self.defaultClient.execute( + request: request, + delegate: delegate + ).wait() + + XCTAssertEqual(response.body, body) + } // In this test, we test that a request can continue to stream its body after the response head and end // was received where the end is a 200. From 0b95c8a645ab441849b884619e55770df9d5ebaa Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Thu, 6 Oct 2022 19:53:04 +0200 Subject: [PATCH 2/8] Add test which currently hangs indefinitely --- .../RequestBagTests.swift | 65 +++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/Tests/AsyncHTTPClientTests/RequestBagTests.swift b/Tests/AsyncHTTPClientTests/RequestBagTests.swift index 43062405c..b057effc1 100644 --- a/Tests/AsyncHTTPClientTests/RequestBagTests.swift +++ b/Tests/AsyncHTTPClientTests/RequestBagTests.swift @@ -454,6 +454,71 @@ final class RequestBagTests: XCTestCase { XCTAssertEqual($0 as? HTTPClientError, .cancelled) } } + + func testDidReceiveBodyPartFailedPromise() { + let embeddedEventLoop = EmbeddedEventLoop() + defer { XCTAssertNoThrow(try embeddedEventLoop.syncShutdownGracefully()) } + let logger = Logger(label: "test") + + var maybeRequest: HTTPClient.Request? + + XCTAssertNoThrow(maybeRequest = try HTTPClient.Request( + url: "https://swift.org", + method: .POST, + body: .byteBuffer(.init(bytes: [1])) + )) + guard let request = maybeRequest else { return XCTFail("Expected to have a request") } + + struct MyError: Error, Equatable {} + final class Delegate: HTTPClientResponseDelegate { + typealias Response = Void + let didFinishPromise: EventLoopPromise + init(didFinishPromise: EventLoopPromise) { + self.didFinishPromise = didFinishPromise + } + func didReceiveBodyPart(task: HTTPClient.Task, _ buffer: ByteBuffer) -> EventLoopFuture { + task.eventLoop.makeFailedFuture(MyError()) + } + func didReceiveError(task: HTTPClient.Task, _ error: Error) { + self.didFinishPromise.fail(error) + } + func didFinishRequest(task: AsyncHTTPClient.HTTPClient.Task) throws -> Void { + XCTFail("\(#function) should not be called") + self.didFinishPromise.succeed(()) + } + } + let delegate = Delegate(didFinishPromise: embeddedEventLoop.makePromise()) + var maybeRequestBag: RequestBag? + XCTAssertNoThrow(maybeRequestBag = try RequestBag( + request: request, + eventLoopPreference: .delegate(on: embeddedEventLoop), + task: .init(eventLoop: embeddedEventLoop, logger: logger), + redirectHandler: nil, + connectionDeadline: .now() + .seconds(30), + requestOptions: .forTests(), + delegate: delegate + )) + guard let bag = maybeRequestBag else { return XCTFail("Expected to be able to create a request bag.") } + + let executor = MockRequestExecutor(eventLoop: embeddedEventLoop) + + executor.runRequest(bag) + + bag.resumeRequestBodyStream() + XCTAssertNoThrow(try executor.receiveRequestBody { XCTAssertEqual($0, ByteBuffer(bytes: [1])) }) + + bag.receiveResponseHead(.init(version: .http1_1, status: .ok)) + + bag.succeedRequest([ByteBuffer([1])]) + + + XCTAssertThrowsError(try delegate.didFinishPromise.futureResult.wait()) { error in + XCTAssertEqualTypeAndValue(error, MyError()) + } + XCTAssertThrowsError(try bag.task.futureResult.wait()) { error in + XCTAssertEqualTypeAndValue(error, MyError()) + } + } func testDidReceiveBodyPartFailedPromise() { let embeddedEventLoop = EmbeddedEventLoop() From 918db8f7c62e0b2c29504e3e87e367fc099441b7 Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Fri, 7 Oct 2022 13:20:56 +0200 Subject: [PATCH 3/8] Run `generate_linux_tests.rb` and `SwiftFormat` --- Tests/AsyncHTTPClientTests/RequestBagTests.swift | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/Tests/AsyncHTTPClientTests/RequestBagTests.swift b/Tests/AsyncHTTPClientTests/RequestBagTests.swift index b057effc1..9d1842e43 100644 --- a/Tests/AsyncHTTPClientTests/RequestBagTests.swift +++ b/Tests/AsyncHTTPClientTests/RequestBagTests.swift @@ -454,7 +454,7 @@ final class RequestBagTests: XCTestCase { XCTAssertEqual($0 as? HTTPClientError, .cancelled) } } - + func testDidReceiveBodyPartFailedPromise() { let embeddedEventLoop = EmbeddedEventLoop() defer { XCTAssertNoThrow(try embeddedEventLoop.syncShutdownGracefully()) } @@ -468,7 +468,7 @@ final class RequestBagTests: XCTestCase { body: .byteBuffer(.init(bytes: [1])) )) guard let request = maybeRequest else { return XCTFail("Expected to have a request") } - + struct MyError: Error, Equatable {} final class Delegate: HTTPClientResponseDelegate { typealias Response = Void @@ -476,13 +476,16 @@ final class RequestBagTests: XCTestCase { init(didFinishPromise: EventLoopPromise) { self.didFinishPromise = didFinishPromise } + func didReceiveBodyPart(task: HTTPClient.Task, _ buffer: ByteBuffer) -> EventLoopFuture { task.eventLoop.makeFailedFuture(MyError()) } + func didReceiveError(task: HTTPClient.Task, _ error: Error) { self.didFinishPromise.fail(error) } - func didFinishRequest(task: AsyncHTTPClient.HTTPClient.Task) throws -> Void { + + func didFinishRequest(task: AsyncHTTPClient.HTTPClient.Task) throws { XCTFail("\(#function) should not be called") self.didFinishPromise.succeed(()) } @@ -506,11 +509,10 @@ final class RequestBagTests: XCTestCase { bag.resumeRequestBodyStream() XCTAssertNoThrow(try executor.receiveRequestBody { XCTAssertEqual($0, ByteBuffer(bytes: [1])) }) - + bag.receiveResponseHead(.init(version: .http1_1, status: .ok)) - - bag.succeedRequest([ByteBuffer([1])]) + bag.succeedRequest([ByteBuffer([1])]) XCTAssertThrowsError(try delegate.didFinishPromise.futureResult.wait()) { error in XCTAssertEqualTypeAndValue(error, MyError()) From d4d61f7c855162e70eebb887cf87ca91e3325a8b Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Fri, 7 Oct 2022 15:55:20 +0200 Subject: [PATCH 4/8] Print type and value if assert fails --- Tests/AsyncHTTPClientTests/HTTPClientTests.swift | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index f214ffdc9..834e1953e 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -2677,8 +2677,8 @@ class HTTPClientTests: XCTestCase { let delegate = TestDelegate() XCTAssertThrowsError(try httpClient.execute(request: request, delegate: delegate).wait()) { - XCTAssertEqual(.connectTimeout, $0 as? HTTPClientError) - XCTAssertEqual(.connectTimeout, delegate.error as? HTTPClientError) + XCTAssertEqualTypeAndValue($0, HTTPClientError.connectTimeout) + XCTAssertEqualTypeAndValue(delegate.error, HTTPClientError.connectTimeout) } } From a9122e8c2239ecfff3a56ef4edb9669f68c993a2 Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Fri, 7 Oct 2022 15:57:48 +0200 Subject: [PATCH 5/8] Run `generate_linux_tests.rb` and `SwiftFormat` --- Sources/AsyncHTTPClient/HTTPHandler.swift | 20 ++++++------- .../HTTPClientTestUtils.swift | 3 +- .../HTTPClientTests+XCTest.swift | 4 +++ .../HTTPClientTests.swift | 28 +++++++++---------- .../RequestBagTests+XCTest.swift | 1 + 5 files changed, 30 insertions(+), 26 deletions(-) diff --git a/Sources/AsyncHTTPClient/HTTPHandler.swift b/Sources/AsyncHTTPClient/HTTPHandler.swift index bcc39e01a..1b72df2a9 100644 --- a/Sources/AsyncHTTPClient/HTTPHandler.swift +++ b/Sources/AsyncHTTPClient/HTTPHandler.swift @@ -356,7 +356,7 @@ extension HTTPClient { /// /// This ``HTTPClientResponseDelegate`` buffers a complete HTTP response in memory. It does not stream the response body in. /// The resulting ``Response`` type is ``HTTPClient/Response``. -final public class ResponseAccumulator: HTTPClientResponseDelegate { +public final class ResponseAccumulator: HTTPClientResponseDelegate { public typealias Response = HTTPClient.Response enum State { @@ -366,29 +366,29 @@ final public class ResponseAccumulator: HTTPClientResponseDelegate { case end case error(Error) } - + public struct ResponseTooBigError: Error, CustomStringConvertible { var maxBodySize: Int - + public var description: String { - return "ResponseTooBigError: received response body exceeds maximum accepted size of \(maxBodySize) bytes" + return "ResponseTooBigError: received response body exceeds maximum accepted size of \(self.maxBodySize) bytes" } } var state = State.idle let request: HTTPClient.Request - + static let maxByteBufferSize = Int(UInt32.max) - + /// Maximum size in bytes of the HTTP response body that ``ResponseAccumulator`` will accept /// until it will abort the request and throw an ``ResponseTooBigError``. - /// + /// /// Default is 2^32. /// - precondition: not allowed to exceed 2^32 because ``ByteBuffer`` can not store more bytes public var maxBodySize: Int = maxByteBufferSize { didSet { precondition( - maxBodySize <= Self.maxByteBufferSize, + self.maxBodySize <= Self.maxByteBufferSize, "maxBodyLength is not allowed to exceed 2^32 because ByteBuffer can not store more bytes" ) } @@ -408,7 +408,7 @@ final public class ResponseAccumulator: HTTPClientResponseDelegate { self.state = .error(error) return task.eventLoop.makeFailedFuture(error) } - + self.state = .head(head) case .head: preconditionFailure("head already set") @@ -440,7 +440,7 @@ final public class ResponseAccumulator: HTTPClientResponseDelegate { self.state = .error(error) return task.eventLoop.makeFailedFuture(error) } - + // The compiler can't prove that `self.state` is dead here (and it kinda isn't, there's // a cross-module call in the way) so we need to drop the original reference to `body` in // `self.state` or we'll get a CoW. To fix that we temporarily set the state to `.end` (which diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift index ca7cf2178..415070a5c 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift @@ -365,8 +365,7 @@ internal final class HTTPBin where var socketAddress: SocketAddress { return self.serverChannel.localAddress! } - - + var baseURL: String { let scheme: String = { switch mode { diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift index 2427d6cbf..04e507a82 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift @@ -132,6 +132,10 @@ extension HTTPClientTests { ("testSSLHandshakeErrorPropagationDelayedClose", testSSLHandshakeErrorPropagationDelayedClose), ("testWeCloseConnectionsWhenConnectionCloseSetByServer", testWeCloseConnectionsWhenConnectionCloseSetByServer), ("testBiDirectionalStreaming", testBiDirectionalStreaming), + ("testResponseAccumulatorMaxBodySizeLimitExceedingWithContentLength", testResponseAccumulatorMaxBodySizeLimitExceedingWithContentLength), + ("testResponseAccumulatorMaxBodySizeLimitNotExceedingWithContentLength", testResponseAccumulatorMaxBodySizeLimitNotExceedingWithContentLength), + ("testResponseAccumulatorMaxBodySizeLimitExceedingWithTransferEncodingChuncked", testResponseAccumulatorMaxBodySizeLimitExceedingWithTransferEncodingChuncked), + ("testResponseAccumulatorMaxBodySizeLimitNotExceedingWithTransferEncodingChuncked", testResponseAccumulatorMaxBodySizeLimitNotExceedingWithTransferEncodingChuncked), ("testBiDirectionalStreamingEarly200", testBiDirectionalStreamingEarly200), ("testBiDirectionalStreamingEarly200DoesntPreventUsFromSendingMoreRequests", testBiDirectionalStreamingEarly200DoesntPreventUsFromSendingMoreRequests), ("testCloseConnectionAfterEarly2XXWhenStreaming", testCloseConnectionAfterEarly2XXWhenStreaming), diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index 834e1953e..655a0bfcf 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -3091,13 +3091,13 @@ class HTTPClientTests: XCTestCase { XCTAssertNoThrow(try future.wait()) XCTAssertNil(try delegate.next().wait()) } - + func testResponseAccumulatorMaxBodySizeLimitExceedingWithContentLength() throws { let httpBin = HTTPBin(.http1_1(ssl: false, compress: false)) { _ in HTTPEchoHandler() } defer { XCTAssertNoThrow(try httpBin.shutdown()) } - + let body = ByteBuffer(bytes: 0..<11) - + var request = try Request(url: httpBin.baseURL) request.body = .byteBuffer(body) let delegate = ResponseAccumulator(request: request) @@ -3109,13 +3109,13 @@ class HTTPClientTests: XCTestCase { XCTAssertTrue(error is ResponseAccumulator.ResponseTooBigError, "unexpected error \(error)") } } - + func testResponseAccumulatorMaxBodySizeLimitNotExceedingWithContentLength() throws { let httpBin = HTTPBin(.http1_1(ssl: false, compress: false)) { _ in HTTPEchoHandler() } defer { XCTAssertNoThrow(try httpBin.shutdown()) } - + let body = ByteBuffer(bytes: 0..<10) - + var request = try Request(url: httpBin.baseURL) request.body = .byteBuffer(body) let delegate = ResponseAccumulator(request: request) @@ -3124,16 +3124,16 @@ class HTTPClientTests: XCTestCase { request: request, delegate: delegate ).wait() - + XCTAssertEqual(response.body, body) } - + func testResponseAccumulatorMaxBodySizeLimitExceedingWithTransferEncodingChuncked() throws { let httpBin = HTTPBin(.http1_1(ssl: false, compress: false)) { _ in HTTPEchoHandler() } defer { XCTAssertNoThrow(try httpBin.shutdown()) } - + let body = ByteBuffer(bytes: 0..<11) - + var request = try Request(url: httpBin.baseURL) request.body = .stream { writer in writer.write(.byteBuffer(body)) @@ -3147,13 +3147,13 @@ class HTTPClientTests: XCTestCase { XCTAssertTrue(error is ResponseAccumulator.ResponseTooBigError, "unexpected error \(error)") } } - + func testResponseAccumulatorMaxBodySizeLimitNotExceedingWithTransferEncodingChuncked() throws { let httpBin = HTTPBin(.http1_1(ssl: false, compress: false)) { _ in HTTPEchoHandler() } defer { XCTAssertNoThrow(try httpBin.shutdown()) } - + let body = ByteBuffer(bytes: 0..<10) - + var request = try Request(url: httpBin.baseURL) request.body = .stream { writer in writer.write(.byteBuffer(body)) @@ -3164,7 +3164,7 @@ class HTTPClientTests: XCTestCase { request: request, delegate: delegate ).wait() - + XCTAssertEqual(response.body, body) } diff --git a/Tests/AsyncHTTPClientTests/RequestBagTests+XCTest.swift b/Tests/AsyncHTTPClientTests/RequestBagTests+XCTest.swift index b6a05733c..748117ebe 100644 --- a/Tests/AsyncHTTPClientTests/RequestBagTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/RequestBagTests+XCTest.swift @@ -35,6 +35,7 @@ extension RequestBagTests { ("testFailsTaskWhenTaskIsWaitingForMoreFromServer", testFailsTaskWhenTaskIsWaitingForMoreFromServer), ("testChannelBecomingWritableDoesntCrashCancelledTask", testChannelBecomingWritableDoesntCrashCancelledTask), ("testDidReceiveBodyPartFailedPromise", testDidReceiveBodyPartFailedPromise), + ("testDidReceiveBodyPartFailedPromise", testDidReceiveBodyPartFailedPromise), ("testHTTPUploadIsCancelledEvenThoughRequestSucceeds", testHTTPUploadIsCancelledEvenThoughRequestSucceeds), ("testRaceBetweenConnectionCloseAndDemandMoreData", testRaceBetweenConnectionCloseAndDemandMoreData), ("testRedirectWith3KBBody", testRedirectWith3KBBody), From f0947e948d5bea5a46ac28bd608b90710ef8b55c Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Sat, 8 Oct 2022 14:17:40 +0100 Subject: [PATCH 6/8] Remove duplicate test due too merge conflict --- .../RequestBagTests+XCTest.swift | 1 - .../RequestBagTests.swift | 67 ------------------- 2 files changed, 68 deletions(-) diff --git a/Tests/AsyncHTTPClientTests/RequestBagTests+XCTest.swift b/Tests/AsyncHTTPClientTests/RequestBagTests+XCTest.swift index 748117ebe..b6a05733c 100644 --- a/Tests/AsyncHTTPClientTests/RequestBagTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/RequestBagTests+XCTest.swift @@ -35,7 +35,6 @@ extension RequestBagTests { ("testFailsTaskWhenTaskIsWaitingForMoreFromServer", testFailsTaskWhenTaskIsWaitingForMoreFromServer), ("testChannelBecomingWritableDoesntCrashCancelledTask", testChannelBecomingWritableDoesntCrashCancelledTask), ("testDidReceiveBodyPartFailedPromise", testDidReceiveBodyPartFailedPromise), - ("testDidReceiveBodyPartFailedPromise", testDidReceiveBodyPartFailedPromise), ("testHTTPUploadIsCancelledEvenThoughRequestSucceeds", testHTTPUploadIsCancelledEvenThoughRequestSucceeds), ("testRaceBetweenConnectionCloseAndDemandMoreData", testRaceBetweenConnectionCloseAndDemandMoreData), ("testRedirectWith3KBBody", testRedirectWith3KBBody), diff --git a/Tests/AsyncHTTPClientTests/RequestBagTests.swift b/Tests/AsyncHTTPClientTests/RequestBagTests.swift index 9d1842e43..43062405c 100644 --- a/Tests/AsyncHTTPClientTests/RequestBagTests.swift +++ b/Tests/AsyncHTTPClientTests/RequestBagTests.swift @@ -522,73 +522,6 @@ final class RequestBagTests: XCTestCase { } } - func testDidReceiveBodyPartFailedPromise() { - let embeddedEventLoop = EmbeddedEventLoop() - defer { XCTAssertNoThrow(try embeddedEventLoop.syncShutdownGracefully()) } - let logger = Logger(label: "test") - - var maybeRequest: HTTPClient.Request? - - XCTAssertNoThrow(maybeRequest = try HTTPClient.Request( - url: "https://swift.org", - method: .POST, - body: .byteBuffer(.init(bytes: [1])) - )) - guard let request = maybeRequest else { return XCTFail("Expected to have a request") } - - struct MyError: Error, Equatable {} - final class Delegate: HTTPClientResponseDelegate { - typealias Response = Void - let didFinishPromise: EventLoopPromise - init(didFinishPromise: EventLoopPromise) { - self.didFinishPromise = didFinishPromise - } - - func didReceiveBodyPart(task: HTTPClient.Task, _ buffer: ByteBuffer) -> EventLoopFuture { - task.eventLoop.makeFailedFuture(MyError()) - } - - func didReceiveError(task: HTTPClient.Task, _ error: Error) { - self.didFinishPromise.fail(error) - } - - func didFinishRequest(task: AsyncHTTPClient.HTTPClient.Task) throws { - XCTFail("\(#function) should not be called") - self.didFinishPromise.succeed(()) - } - } - let delegate = Delegate(didFinishPromise: embeddedEventLoop.makePromise()) - var maybeRequestBag: RequestBag? - XCTAssertNoThrow(maybeRequestBag = try RequestBag( - request: request, - eventLoopPreference: .delegate(on: embeddedEventLoop), - task: .init(eventLoop: embeddedEventLoop, logger: logger), - redirectHandler: nil, - connectionDeadline: .now() + .seconds(30), - requestOptions: .forTests(), - delegate: delegate - )) - guard let bag = maybeRequestBag else { return XCTFail("Expected to be able to create a request bag.") } - - let executor = MockRequestExecutor(eventLoop: embeddedEventLoop) - - executor.runRequest(bag) - - bag.resumeRequestBodyStream() - XCTAssertNoThrow(try executor.receiveRequestBody { XCTAssertEqual($0, ByteBuffer(bytes: [1])) }) - - bag.receiveResponseHead(.init(version: .http1_1, status: .ok)) - - bag.succeedRequest([ByteBuffer([1])]) - - XCTAssertThrowsError(try delegate.didFinishPromise.futureResult.wait()) { error in - XCTAssertEqualTypeAndValue(error, MyError()) - } - XCTAssertThrowsError(try bag.task.futureResult.wait()) { error in - XCTAssertEqualTypeAndValue(error, MyError()) - } - } - func testHTTPUploadIsCancelledEvenThoughRequestSucceeds() { let embeddedEventLoop = EmbeddedEventLoop() defer { XCTAssertNoThrow(try embeddedEventLoop.syncShutdownGracefully()) } From 3d8cac80d6ee8bca08ec5e9a2d0dd5e0ef3ef17f Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Sat, 8 Oct 2022 14:20:58 +0100 Subject: [PATCH 7/8] Validate that maxBodySize is positive --- Sources/AsyncHTTPClient/HTTPHandler.swift | 1 + 1 file changed, 1 insertion(+) diff --git a/Sources/AsyncHTTPClient/HTTPHandler.swift b/Sources/AsyncHTTPClient/HTTPHandler.swift index 1b72df2a9..f37b519e1 100644 --- a/Sources/AsyncHTTPClient/HTTPHandler.swift +++ b/Sources/AsyncHTTPClient/HTTPHandler.swift @@ -387,6 +387,7 @@ public final class ResponseAccumulator: HTTPClientResponseDelegate { /// - precondition: not allowed to exceed 2^32 because ``ByteBuffer`` can not store more bytes public var maxBodySize: Int = maxByteBufferSize { didSet { + precondition(self.maxBodySize >= 0, "maxBodyLength is not allowed to be negative") precondition( self.maxBodySize <= Self.maxByteBufferSize, "maxBodyLength is not allowed to exceed 2^32 because ByteBuffer can not store more bytes" From 986bb058f5e406c0a0909e1632703b064d7282e7 Mon Sep 17 00:00:00 2001 From: David Nadoba Date: Mon, 10 Oct 2022 10:20:43 +0100 Subject: [PATCH 8/8] Address review comments --- Sources/AsyncHTTPClient/HTTPHandler.swift | 36 +++++++++++++------ .../HTTPClientTestUtils.swift | 19 ++++++++++ .../HTTPClientTests+XCTest.swift | 1 + .../HTTPClientTests.swift | 32 ++++++++++------- 4 files changed, 65 insertions(+), 23 deletions(-) diff --git a/Sources/AsyncHTTPClient/HTTPHandler.swift b/Sources/AsyncHTTPClient/HTTPHandler.swift index f37b519e1..26880ef8b 100644 --- a/Sources/AsyncHTTPClient/HTTPHandler.swift +++ b/Sources/AsyncHTTPClient/HTTPHandler.swift @@ -368,7 +368,10 @@ public final class ResponseAccumulator: HTTPClientResponseDelegate { } public struct ResponseTooBigError: Error, CustomStringConvertible { - var maxBodySize: Int + public var maxBodySize: Int + public init(maxBodySize: Int) { + self.maxBodySize = maxBodySize + } public var description: String { return "ResponseTooBigError: received response body exceeds maximum accepted size of \(self.maxBodySize) bytes" @@ -385,24 +388,35 @@ public final class ResponseAccumulator: HTTPClientResponseDelegate { /// /// Default is 2^32. /// - precondition: not allowed to exceed 2^32 because ``ByteBuffer`` can not store more bytes - public var maxBodySize: Int = maxByteBufferSize { - didSet { - precondition(self.maxBodySize >= 0, "maxBodyLength is not allowed to be negative") - precondition( - self.maxBodySize <= Self.maxByteBufferSize, - "maxBodyLength is not allowed to exceed 2^32 because ByteBuffer can not store more bytes" - ) - } + public let maxBodySize: Int + + public convenience init(request: HTTPClient.Request) { + self.init(request: request, maxBodySize: Self.maxByteBufferSize) } - public init(request: HTTPClient.Request) { + /// - Parameters: + /// - request: The corresponding request of the response this delegate will be accumulating. + /// - maxBodySize: Maximum size in bytes of the HTTP response body that ``ResponseAccumulator`` will accept + /// until it will abort the request and throw an ``ResponseTooBigError``. + /// Default is 2^32. + /// - precondition: maxBodySize is not allowed to exceed 2^32 because ``ByteBuffer`` can not store more bytes + /// - warning: You can use ``ResponseAccumulator`` for just one request. + /// If you start another request, you need to initiate another ``ResponseAccumulator``. + public init(request: HTTPClient.Request, maxBodySize: Int) { + precondition(maxBodySize >= 0, "maxBodyLength is not allowed to be negative") + precondition( + maxBodySize <= Self.maxByteBufferSize, + "maxBodyLength is not allowed to exceed 2^32 because ByteBuffer can not store more bytes" + ) self.request = request + self.maxBodySize = maxBodySize } public func didReceiveHead(task: HTTPClient.Task, _ head: HTTPResponseHead) -> EventLoopFuture { switch self.state { case .idle: - if let contentLength = head.headers.first(name: "Content-Length"), + if self.request.method != .HEAD, + let contentLength = head.headers.first(name: "Content-Length"), let announcedBodySize = Int(contentLength), announcedBodySize > self.maxBodySize { let error = ResponseTooBigError(maxBodySize: maxBodySize) diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift index 415070a5c..884681123 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift @@ -1331,6 +1331,25 @@ class HTTPEchoHandler: ChannelInboundHandler { } } +final class HTTPEchoHeaders: ChannelInboundHandler { + typealias InboundIn = HTTPServerRequestPart + typealias OutboundOut = HTTPServerResponsePart + + func channelRead(context: ChannelHandlerContext, data: NIOAny) { + let request = self.unwrapInboundIn(data) + switch request { + case .head(let requestHead): + context.writeAndFlush(self.wrapOutboundOut(.head(.init(version: .http1_1, status: .ok, headers: requestHead.headers))), promise: nil) + case .body: + break + case .end: + context.writeAndFlush(self.wrapOutboundOut(.end(nil))).whenSuccess { + context.close(promise: nil) + } + } + } +} + final class HTTP200DelayedHandler: ChannelInboundHandler { typealias InboundIn = HTTPServerRequestPart typealias OutboundOut = HTTPServerResponsePart diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift index 04e507a82..ef6690c00 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift @@ -134,6 +134,7 @@ extension HTTPClientTests { ("testBiDirectionalStreaming", testBiDirectionalStreaming), ("testResponseAccumulatorMaxBodySizeLimitExceedingWithContentLength", testResponseAccumulatorMaxBodySizeLimitExceedingWithContentLength), ("testResponseAccumulatorMaxBodySizeLimitNotExceedingWithContentLength", testResponseAccumulatorMaxBodySizeLimitNotExceedingWithContentLength), + ("testResponseAccumulatorMaxBodySizeLimitExceedingWithContentLengthButMethodIsHead", testResponseAccumulatorMaxBodySizeLimitExceedingWithContentLengthButMethodIsHead), ("testResponseAccumulatorMaxBodySizeLimitExceedingWithTransferEncodingChuncked", testResponseAccumulatorMaxBodySizeLimitExceedingWithTransferEncodingChuncked), ("testResponseAccumulatorMaxBodySizeLimitNotExceedingWithTransferEncodingChuncked", testResponseAccumulatorMaxBodySizeLimitNotExceedingWithTransferEncodingChuncked), ("testBiDirectionalStreamingEarly200", testBiDirectionalStreamingEarly200), diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index 655a0bfcf..d5108bb27 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -3100,11 +3100,9 @@ class HTTPClientTests: XCTestCase { var request = try Request(url: httpBin.baseURL) request.body = .byteBuffer(body) - let delegate = ResponseAccumulator(request: request) - delegate.maxBodySize = 10 XCTAssertThrowsError(try self.defaultClient.execute( request: request, - delegate: delegate + delegate: ResponseAccumulator(request: request, maxBodySize: 10) ).wait()) { error in XCTAssertTrue(error is ResponseAccumulator.ResponseTooBigError, "unexpected error \(error)") } @@ -3118,16 +3116,30 @@ class HTTPClientTests: XCTestCase { var request = try Request(url: httpBin.baseURL) request.body = .byteBuffer(body) - let delegate = ResponseAccumulator(request: request) - delegate.maxBodySize = 10 let response = try self.defaultClient.execute( request: request, - delegate: delegate + delegate: ResponseAccumulator(request: request, maxBodySize: 10) ).wait() XCTAssertEqual(response.body, body) } + func testResponseAccumulatorMaxBodySizeLimitExceedingWithContentLengthButMethodIsHead() throws { + let httpBin = HTTPBin(.http1_1(ssl: false, compress: false)) { _ in HTTPEchoHeaders() } + defer { XCTAssertNoThrow(try httpBin.shutdown()) } + + let body = ByteBuffer(bytes: 0..<11) + + var request = try Request(url: httpBin.baseURL, method: .HEAD) + request.body = .byteBuffer(body) + let response = try self.defaultClient.execute( + request: request, + delegate: ResponseAccumulator(request: request, maxBodySize: 10) + ).wait() + + XCTAssertEqual(response.body ?? ByteBuffer(), ByteBuffer()) + } + func testResponseAccumulatorMaxBodySizeLimitExceedingWithTransferEncodingChuncked() throws { let httpBin = HTTPBin(.http1_1(ssl: false, compress: false)) { _ in HTTPEchoHandler() } defer { XCTAssertNoThrow(try httpBin.shutdown()) } @@ -3138,11 +3150,9 @@ class HTTPClientTests: XCTestCase { request.body = .stream { writer in writer.write(.byteBuffer(body)) } - let delegate = ResponseAccumulator(request: request) - delegate.maxBodySize = 10 XCTAssertThrowsError(try self.defaultClient.execute( request: request, - delegate: delegate + delegate: ResponseAccumulator(request: request, maxBodySize: 10) ).wait()) { error in XCTAssertTrue(error is ResponseAccumulator.ResponseTooBigError, "unexpected error \(error)") } @@ -3158,11 +3168,9 @@ class HTTPClientTests: XCTestCase { request.body = .stream { writer in writer.write(.byteBuffer(body)) } - let delegate = ResponseAccumulator(request: request) - delegate.maxBodySize = 10 let response = try self.defaultClient.execute( request: request, - delegate: delegate + delegate: ResponseAccumulator(request: request, maxBodySize: 10) ).wait() XCTAssertEqual(response.body, body)