From 009bfd7a60b436d226d1dfa0a1214a8d21604db8 Mon Sep 17 00:00:00 2001 From: Artem Redkin Date: Tue, 23 Jun 2020 15:46:31 +0100 Subject: [PATCH 1/7] fail if user tries writing bytes after request is sent --- Sources/AsyncHTTPClient/HTTPClient.swift | 3 ++ Sources/AsyncHTTPClient/HTTPHandler.swift | 19 +++++++--- .../HTTPClientTests+XCTest.swift | 1 + .../HTTPClientTests.swift | 35 +++++++++++++++++++ 4 files changed, 54 insertions(+), 4 deletions(-) diff --git a/Sources/AsyncHTTPClient/HTTPClient.swift b/Sources/AsyncHTTPClient/HTTPClient.swift index ca2343045..402ec89bd 100644 --- a/Sources/AsyncHTTPClient/HTTPClient.swift +++ b/Sources/AsyncHTTPClient/HTTPClient.swift @@ -967,6 +967,7 @@ public struct HTTPClientError: Error, Equatable, CustomStringConvertible { case traceRequestWithBody case invalidHeaderFieldNames([String]) case bodyLengthMismatch + case writeAfterRequestSent } private var code: Code @@ -1019,4 +1020,6 @@ public struct HTTPClientError: Error, Equatable, CustomStringConvertible { public static func invalidHeaderFieldNames(_ names: [String]) -> HTTPClientError { return HTTPClientError(code: .invalidHeaderFieldNames(names)) } /// Body length is not equal to `Content-Length`. public static let bodyLengthMismatch = HTTPClientError(code: .bodyLengthMismatch) + /// Body part was written after request was fully sent. + public static let writeAfterRequestSent = HTTPClientError(code: .writeAfterRequestSent) } diff --git a/Sources/AsyncHTTPClient/HTTPHandler.swift b/Sources/AsyncHTTPClient/HTTPHandler.swift index 41867ac3a..daa37d2d4 100644 --- a/Sources/AsyncHTTPClient/HTTPHandler.swift +++ b/Sources/AsyncHTTPClient/HTTPHandler.swift @@ -876,12 +876,10 @@ extension TaskHandler: ChannelDuplexHandler { let promise = self.task.eventLoop.makePromise(of: Void.self) // All writes have to be switched to the channel EL if channel and task ELs differ if channel.eventLoop.inEventLoop { - self.actualBodyLength += part.readableBytes - context.writeAndFlush(self.wrapOutboundOut(.body(part)), promise: promise) + self.writeBodyPart(context: context, part: part, promise: promise) } else { channel.eventLoop.execute { - self.actualBodyLength += part.readableBytes - context.writeAndFlush(self.wrapOutboundOut(.body(part)), promise: promise) + self.writeBodyPart(context: context, part: part, promise: promise) } } @@ -901,6 +899,19 @@ extension TaskHandler: ChannelDuplexHandler { } } + private func writeBodyPart(context: ChannelHandlerContext, part: IOData, promise: EventLoopPromise) { + switch self.state { + case .idle: + self.actualBodyLength += part.readableBytes + context.writeAndFlush(self.wrapOutboundOut(.body(part)), promise: promise) + default: + let error = HTTPClientError.writeAfterRequestSent + promise.fail(error) + self.state = .endOrError + self.failTaskAndNotifyDelegate(error: error, self.delegate.didReceiveError) + } + } + public func read(context: ChannelHandlerContext) { if self.mayRead { self.pendingRead = false diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift index 0de4d0c07..5555ab7f3 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift @@ -118,6 +118,7 @@ extension HTTPClientTests { ("testDelegateCallinsTolerateRandomEL", testDelegateCallinsTolerateRandomEL), ("testContentLengthTooLongFails", testContentLengthTooLongFails), ("testContentLengthTooShortFails", testContentLengthTooShortFails), + ("testBodyUploadAfterEndFails", testBodyUploadAfterEndFails), ] } } diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index eaf4c6564..016a7a351 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -2502,4 +2502,39 @@ class HTTPClientTests: XCTestCase { XCTAssertEqual(info.connectionNumber, 1) XCTAssertEqual(info.requestNumber, 1) } + + func testBodyUploadAfterEndFails() { + let url = self.defaultHTTPBinURLPrefix + "post" + + func uploader(_ streamWriter: HTTPClient.Body.StreamWriter) -> EventLoopFuture { + let done = streamWriter.write(.byteBuffer(ByteBuffer(string: "X"))) + done.recover { error -> Void in + XCTFail("unexpected error \(error)") + }.whenSuccess { + // This is executed when we have already sent the end of the request. + done.eventLoop.execute { + streamWriter.write(.byteBuffer(ByteBuffer(string: "BAD BAD BAD"))).whenComplete { result in + switch result { + case .success: + XCTFail("we succeeded writing bytes after the end!?") + case .failure(let error): + XCTAssertEqual(HTTPClientError.writeAfterRequestSent, error as? HTTPClientError) + } + } + } + } + return done + } + + XCTAssertThrowsError( + try self.defaultClient.execute(request: + Request(url: url, + body: .stream(length: 1, uploader))).wait()) { error in + XCTAssertEqual(HTTPClientError.writeAfterRequestSent, error as? HTTPClientError) + } + + // Quickly try another request and check that it works. If we by accident wrote some extra bytes into the + // stream (and reuse the connection) that could cause problems. + XCTAssertNoThrow(try self.defaultClient.get(url: self.defaultHTTPBinURLPrefix + "get").wait()) + } } From 86faf471da3be292437db01411068aeeffe9e318 Mon Sep 17 00:00:00 2001 From: Artem Redkin Date: Tue, 23 Jun 2020 16:36:20 +0100 Subject: [PATCH 2/7] rewrite test --- .../HTTPClientInternalTests.swift | 22 +++++++++++-------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift index 34891c632..cf4121efe 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift @@ -424,11 +424,10 @@ class HTTPClientInternalTests: XCTestCase { let randoEL = group.next() let httpClient = HTTPClient(eventLoopGroupProvider: .shared(group)) - let promise: EventLoopPromise = httpClient.eventLoopGroup.next().makePromise() - let httpBin = HTTPBin(channelPromise: promise) + let server = NIOHTTP1TestServer(group: MultiThreadedEventLoopGroup(numberOfThreads: 1)) defer { + XCTAssertNoThrow(try server.stop()) XCTAssertNoThrow(try httpClient.syncShutdown(requiresCleanClose: true)) - XCTAssertNoThrow(try httpBin.shutdown()) } let body: HTTPClient.Body = .stream(length: 8) { writer in @@ -439,7 +438,7 @@ class HTTPClientInternalTests: XCTestCase { } } - let request = try Request(url: "http://127.0.0.1:\(httpBin.port)/custom", + let request = try Request(url: "http://127.0.0.1:\(server.serverPort)/custom", body: body) let delegate = Delegate(expectedEventLoop: delegateEL, randomOtherEventLoop: randoEL) let future = httpClient.execute(request: request, @@ -447,13 +446,18 @@ class HTTPClientInternalTests: XCTestCase { eventLoop: .init(.testOnly_exact(channelOn: channelEL, delegateOn: delegateEL))).futureResult - let channel = try promise.futureResult.wait() + XCTAssertNoThrow(try server.readInbound()) // .head + XCTAssertNoThrow(try server.readInbound()) // .body + XCTAssertNoThrow(try server.readInbound()) // .end // Send 3 parts, but only one should be received until the future is complete - let buffer = channel.allocator.buffer(string: "1234") - try channel.writeAndFlush(HTTPServerResponsePart.body(.byteBuffer(buffer))).wait() + XCTAssertNoThrow(try server.writeOutbound(.head(.init(version: .init(major: 1, minor: 1), + status: .ok, + headers: HTTPHeaders([("Transfer-Encoding", "chunked")]))))) + let buffer = ByteBuffer(string: "1234") + XCTAssertNoThrow(try server.writeOutbound(.body(.byteBuffer(buffer)))) + XCTAssertNoThrow(try server.writeOutbound(.end(nil))) - try channel.writeAndFlush(HTTPServerResponsePart.end(nil)).wait() let (receivedMessages, sentMessages) = try future.wait() XCTAssertEqual(2, receivedMessages.count) XCTAssertEqual(4, sentMessages.count) @@ -488,7 +492,7 @@ class HTTPClientInternalTests: XCTestCase { switch receivedMessages.dropFirst(0).first { case .some(.head(let head)): - XCTAssertEqual(["transfer-encoding": "chunked"], head.headers) + XCTAssertEqual(head.headers["transfer-encoding"].first, "chunked") default: XCTFail("wrong message") } From b2199f0f44b10afae142cf311fdf3a3c878ac29f Mon Sep 17 00:00:00 2001 From: Artem Redkin Date: Tue, 23 Jun 2020 16:40:48 +0100 Subject: [PATCH 3/7] swiftformat --- Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift index cf4121efe..2c7a996b2 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift @@ -452,8 +452,8 @@ class HTTPClientInternalTests: XCTestCase { // Send 3 parts, but only one should be received until the future is complete XCTAssertNoThrow(try server.writeOutbound(.head(.init(version: .init(major: 1, minor: 1), - status: .ok, - headers: HTTPHeaders([("Transfer-Encoding", "chunked")]))))) + status: .ok, + headers: HTTPHeaders([("Transfer-Encoding", "chunked")]))))) let buffer = ByteBuffer(string: "1234") XCTAssertNoThrow(try server.writeOutbound(.body(.byteBuffer(buffer)))) XCTAssertNoThrow(try server.writeOutbound(.end(nil))) From fa123144105aa5fa6735a087273d8286eadf2843 Mon Sep 17 00:00:00 2001 From: Artem Redkin Date: Tue, 23 Jun 2020 16:59:05 +0100 Subject: [PATCH 4/7] small fix --- Sources/AsyncHTTPClient/HTTPHandler.swift | 13 +++++++++++-- .../HTTPClientInternalTests.swift | 4 +++- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/Sources/AsyncHTTPClient/HTTPHandler.swift b/Sources/AsyncHTTPClient/HTTPHandler.swift index daa37d2d4..b949cff21 100644 --- a/Sources/AsyncHTTPClient/HTTPHandler.swift +++ b/Sources/AsyncHTTPClient/HTTPHandler.swift @@ -665,6 +665,7 @@ internal struct TaskCancelEvent {} internal class TaskHandler: RemovableChannelHandler { enum State { case idle + case bodySent case sent case head case redirected(HTTPResponseHead, URL) @@ -839,6 +840,7 @@ extension TaskHandler: ChannelDuplexHandler { }.flatMap { self.writeBody(request: request, context: context) }.flatMap { + self.state = .bodySent context.eventLoop.assertInEventLoop() if let expectedBodyLength = self.expectedBodyLength, expectedBodyLength != self.actualBodyLength { self.state = .endOrError @@ -902,13 +904,20 @@ extension TaskHandler: ChannelDuplexHandler { private func writeBodyPart(context: ChannelHandlerContext, part: IOData, promise: EventLoopPromise) { switch self.state { case .idle: + if let limit = self.expectedBodyLength, self.actualBodyLength + part.readableBytes > limit { + let error = HTTPClientError.bodyLengthMismatch + self.state = .endOrError + self.failTaskAndNotifyDelegate(error: error, self.delegate.didReceiveError) + promise.fail(error) + return + } self.actualBodyLength += part.readableBytes context.writeAndFlush(self.wrapOutboundOut(.body(part)), promise: promise) default: let error = HTTPClientError.writeAfterRequestSent - promise.fail(error) self.state = .endOrError self.failTaskAndNotifyDelegate(error: error, self.delegate.didReceiveError) + promise.fail(error) } } @@ -1004,7 +1013,7 @@ extension TaskHandler: ChannelDuplexHandler { switch self.state { case .endOrError: break - case .body, .head, .idle, .redirected, .sent: + case .body, .head, .idle, .redirected, .sent, .bodySent: self.state = .endOrError let error = HTTPClientError.remoteConnectionClosed self.failTaskAndNotifyDelegate(error: error, self.delegate.didReceiveError) diff --git a/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift index 2c7a996b2..ff4dc2940 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift @@ -415,8 +415,10 @@ class HTTPClientInternalTests: XCTestCase { } let group = getDefaultEventLoopGroup(numberOfThreads: 3) + let serverGroup = MultiThreadedEventLoopGroup(numberOfThreads: 1) defer { XCTAssertNoThrow(try group.syncShutdownGracefully()) + XCTAssertNoThrow(try serverGroup.syncShutdownGracefully()) } let channelEL = group.next() @@ -424,7 +426,7 @@ class HTTPClientInternalTests: XCTestCase { let randoEL = group.next() let httpClient = HTTPClient(eventLoopGroupProvider: .shared(group)) - let server = NIOHTTP1TestServer(group: MultiThreadedEventLoopGroup(numberOfThreads: 1)) + let server = NIOHTTP1TestServer(group: serverGroup) defer { XCTAssertNoThrow(try server.stop()) XCTAssertNoThrow(try httpClient.syncShutdown(requiresCleanClose: true)) From 373d19eb50883ceb95f906411590c2c1e8de8f48 Mon Sep 17 00:00:00 2001 From: Artem Redkin Date: Thu, 16 Jul 2020 16:23:59 +0100 Subject: [PATCH 5/7] add another test --- .../HTTPClientTests.swift | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index c7bce83c4..51ee3b5f6 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -2580,4 +2580,26 @@ class HTTPClientTests: XCTestCase { // stream (and reuse the connection) that could cause problems. XCTAssertNoThrow(try self.defaultClient.get(url: self.defaultHTTPBinURLPrefix + "get").wait()) } + + func testNoBytesSentOverBodyLimit() throws { + let server = NIOHTTP1TestServer(group: self.serverGroup) + defer { + XCTAssertNoThrow(try server.stop()) + } + + let tooLong = "XBAD BAD BAD NOT HTTP/1.1\r\n\r\n" + let future = self.defaultClient.execute( + request: try Request(url: "http://localhost:\(server.serverPort)", + body: .stream(length: 1) { streamWriter in + streamWriter.write(.byteBuffer(ByteBuffer(string: tooLong))) + })) + + XCTAssertNoThrow(try server.readInbound()) // .head + // this should fail if client detects that we are about to send more bytes than body limit and closes the connection + // We can test that this test actually fails if we remove limit check in `writeBodyPart` - it will send bytes, meaning that the next + // call will not throw, but the future will still throw body mismatch error + XCTAssertThrowsError(try server.readInbound()) { error in XCTAssertEqual(error as? HTTPParserError, HTTPParserError.invalidEOFState) } + + XCTAssertThrowsError(try future.wait()) + } } From d1126953416dbdcfd5dcf6c9acfaefad9e862f62 Mon Sep 17 00:00:00 2001 From: Artem Redkin Date: Thu, 16 Jul 2020 17:06:44 +0100 Subject: [PATCH 6/7] additional test --- .../HTTPClientInternalTests+XCTest.swift | 1 + .../HTTPClientInternalTests.swift | 46 +++++++++++++++++++ .../HTTPClientTests+XCTest.swift | 1 + .../HTTPClientTests.swift | 16 +++---- 4 files changed, 56 insertions(+), 8 deletions(-) diff --git a/Tests/AsyncHTTPClientTests/HTTPClientInternalTests+XCTest.swift b/Tests/AsyncHTTPClientTests/HTTPClientInternalTests+XCTest.swift index 14c830e51..6177127d0 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientInternalTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientInternalTests+XCTest.swift @@ -45,6 +45,7 @@ extension HTTPClientInternalTests { ("testTaskPromiseBoundToEL", testTaskPromiseBoundToEL), ("testConnectErrorCalloutOnCorrectEL", testConnectErrorCalloutOnCorrectEL), ("testInternalRequestURI", testInternalRequestURI), + ("testBodyPartStreamStateChangedBeforeNotification", testBodyPartStreamStateChangedBeforeNotification), ] } } diff --git a/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift index ff4dc2940..ce35de4a6 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift @@ -1031,4 +1031,50 @@ class HTTPClientInternalTests: XCTestCase { XCTAssertEqual(request5.socketPath, "/tmp/file") XCTAssertEqual(request5.uri, "/file/path") } + + func testBodyPartStreamStateChangedBeforeNotification() throws { + class StateValidationDelegate: HTTPClientResponseDelegate { + typealias Response = Void + + var handler: TaskHandler! + var triggered = false + + func didReceiveError(task: HTTPClient.Task, _ error: Error) { + self.triggered = true + switch self.handler.state { + case .endOrError: + // expected + break + default: + XCTFail("unexpected state: \(self.handler.state)") + } + } + + func didFinishRequest(task: HTTPClient.Task) throws {} + } + + let channel = EmbeddedChannel() + XCTAssertNoThrow(try channel.connect(to: try SocketAddress(unixDomainSocketPath: "/fake")).wait()) + + let task = Task(eventLoop: channel.eventLoop, logger: HTTPClient.loggingDisabled) + + let delegate = StateValidationDelegate() + let handler = TaskHandler(task: task, + kind: .host, + delegate: delegate, + redirectHandler: nil, + ignoreUncleanSSLShutdown: false, + logger: HTTPClient.loggingDisabled) + + delegate.handler = handler + try channel.pipeline.addHandler(handler).wait() + + var request = try Request(url: "http://localhost:8080/post") + request.body = .stream(length: 1) { writer in + writer.write(.byteBuffer(ByteBuffer(string: "1234"))) + } + + XCTAssertThrowsError(try channel.writeOutbound(request)) + XCTAssertTrue(delegate.triggered) + } } diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift index f97eed370..eb4dd7cb5 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift @@ -121,6 +121,7 @@ extension HTTPClientTests { ("testContentLengthTooLongFails", testContentLengthTooLongFails), ("testContentLengthTooShortFails", testContentLengthTooShortFails), ("testBodyUploadAfterEndFails", testBodyUploadAfterEndFails), + ("testNoBytesSentOverBodyLimit", testNoBytesSentOverBodyLimit), ] } } diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index 51ee3b5f6..5c031c6f4 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -1524,7 +1524,7 @@ class HTTPClientTests: XCTestCase { XCTAssertEqual(.ok, firstResponse.status) return localClient.get(url: url) // <== interesting bit here } - }.wait().status)) + }.wait().status)) } func testMakeSecondRequestWhilstFirstIsOngoing() { @@ -1910,7 +1910,7 @@ class HTTPClientTests: XCTestCase { body: .stream { streamWriter in streamWriterPromise.succeed(streamWriter) return sentOffAllBodyPartsPromise.futureResult - }) + }) } guard let server = makeServer(), let request = makeRequest(server: server) else { @@ -2502,7 +2502,7 @@ class HTTPClientTests: XCTestCase { streamWriter.write(.byteBuffer(ByteBuffer(string: "1"))).cascade(to: promise) } return promise.futureResult - })).wait()) { error in + })).wait()) { error in XCTAssertEqual(error as! HTTPClientError, HTTPClientError.bodyLengthMismatch) } // Quickly try another request and check that it works. @@ -2528,7 +2528,7 @@ class HTTPClientTests: XCTestCase { Request(url: url, body: .stream(length: 1) { streamWriter in streamWriter.write(.byteBuffer(ByteBuffer(string: tooLong))) - })).wait()) { error in + })).wait()) { error in XCTAssertEqual(error as! HTTPClientError, HTTPClientError.bodyLengthMismatch) } // Quickly try another request and check that it works. If we by accident wrote some extra bytes into the @@ -2589,10 +2589,10 @@ class HTTPClientTests: XCTestCase { let tooLong = "XBAD BAD BAD NOT HTTP/1.1\r\n\r\n" let future = self.defaultClient.execute( - request: try Request(url: "http://localhost:\(server.serverPort)", - body: .stream(length: 1) { streamWriter in - streamWriter.write(.byteBuffer(ByteBuffer(string: tooLong))) - })) + request: try Request(url: "http://localhost:\(server.serverPort)", + body: .stream(length: 1) { streamWriter in + streamWriter.write(.byteBuffer(ByteBuffer(string: tooLong))) + })) XCTAssertNoThrow(try server.readInbound()) // .head // this should fail if client detects that we are about to send more bytes than body limit and closes the connection From 51231154cac3cee3fa95deec895bf245c657c335 Mon Sep 17 00:00:00 2001 From: Artem Redkin Date: Fri, 17 Jul 2020 16:08:39 +0100 Subject: [PATCH 7/7] review fix --- Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift index ce35de4a6..52348ba94 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift @@ -1076,5 +1076,8 @@ class HTTPClientInternalTests: XCTestCase { XCTAssertThrowsError(try channel.writeOutbound(request)) XCTAssertTrue(delegate.triggered) + + XCTAssertNoThrow(try channel.readOutbound(as: HTTPClientRequestPart.self)) // .head + XCTAssertNoThrow(XCTAssertTrue(try channel.finish().isClean)) } }