From d87dc01b93fe8a184324a257a865835e1a06d002 Mon Sep 17 00:00:00 2001 From: tomer doron Date: Tue, 9 Jul 2019 17:57:59 -0700 Subject: [PATCH 1/4] improve request validation motivation: safer handling of request validation and mutation changes: * fix scheme logic to be non-case sensitive * made request version, method and url immutable * made request scheme and host internal * adjusted redirect handler implementation to stricter request immutabllity * adjust and add tests --- Sources/AsyncHTTPClient/HTTPHandler.swift | 66 +++++++++++-------- .../HTTPClientTests+XCTest.swift | 2 + .../HTTPClientTests.swift | 18 ++++- 3 files changed, 56 insertions(+), 30 deletions(-) diff --git a/Sources/AsyncHTTPClient/HTTPHandler.swift b/Sources/AsyncHTTPClient/HTTPHandler.swift index 45810daf2..a3e38dc30 100644 --- a/Sources/AsyncHTTPClient/HTTPHandler.swift +++ b/Sources/AsyncHTTPClient/HTTPHandler.swift @@ -91,16 +91,13 @@ extension HTTPClient { public var version: HTTPVersion /// Request HTTP method, defaults to `GET`. public var method: HTTPMethod - /// Remote URL. - public var url: URL - /// Remote HTTP scheme, resolved from `URL`. - public var scheme: String - /// Remote host, resolved from `URL`. - public var host: String /// Request custom HTTP Headers, defaults to no headers. public var headers: HTTPHeaders /// Request body, defaults to no body. public var body: Body? + private var _url: URL! + private var _scheme: String! + private var _host: String! /// Create HTTP request. /// @@ -136,7 +133,15 @@ extension HTTPClient { /// - `unsupportedScheme` if URL does contains unsupported HTTP scheme. /// - `emptyHost` if URL does not contains a host. public init(url: URL, version: HTTPVersion = HTTPVersion(major: 1, minor: 1), method: HTTPMethod = .GET, headers: HTTPHeaders = HTTPHeaders(), body: Body? = nil) throws { - guard let scheme = url.scheme else { + self.version = version + self.method = method + self.headers = headers + self.body = body + try self.setURL(url) + } + + public mutating func setURL(_ url: URL) throws { + guard let scheme = url.scheme?.lowercased() else { throw HTTPClientError.emptyScheme } @@ -148,18 +153,29 @@ extension HTTPClient { throw HTTPClientError.emptyHost } - self.version = version - self.method = method - self.url = url - self.scheme = scheme - self.host = host - self.headers = headers - self.body = body + self._url = url + self._scheme = scheme + self._host = host + } + + /// Remote URL. + public var url: URL { + return self._url + } + + /// Remote HTTP scheme, resolved from `URL`. + public var scheme: String { + return self._scheme + } + + /// Remote host, resolved from `URL`. + public var host: String { + return self._host } /// Whether request will be executed using secure socket. public var useTLS: Bool { - return self.url.scheme == "https" + return self.scheme == "https" } /// Resolved port. @@ -167,7 +183,7 @@ extension HTTPClient { return self.url.port ?? (self.useTLS ? 443 : 80) } - static func isSchemeSupported(scheme: String?) -> Bool { + static func isSchemeSupported(scheme: String) -> Bool { return scheme == "http" || scheme == "https" } } @@ -640,7 +656,7 @@ internal struct RedirectHandler { return nil } - guard HTTPClient.Request.isSchemeSupported(scheme: url.scheme) else { + guard HTTPClient.Request.isSchemeSupported(scheme: request.scheme) else { return nil } @@ -655,18 +671,10 @@ internal struct RedirectHandler { let originalURL = self.request.url var request = self.request - request.url = redirectURL - - if let redirectHost = redirectURL.host { - request.host = redirectHost - } else { - preconditionFailure("redirectURL doesn't contain a host") - } - - if let redirectScheme = redirectURL.scheme { - request.scheme = redirectScheme - } else { - preconditionFailure("redirectURL doesn't contain a scheme") + do { + try request.setURL(redirectURL) + } catch { + return promise.fail(error) } var convertToGet = false diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift index 0313576e9..86c98bdbd 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift @@ -26,6 +26,8 @@ extension HTTPClientTests { static var allTests: [(String, (HTTPClientTests) -> () throws -> Void)] { return [ ("testRequestURI", testRequestURI), + ("testBadRequestURI", testBadRequestURI), + ("testSchemaCasing", testSchemaCasing), ("testGet", testGet), ("testGetWithSharedEventLoopGroup", testGetWithSharedEventLoopGroup), ("testPost", testPost), diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index c6a7f664b..78a8bdb76 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -23,7 +23,7 @@ class HTTPClientTests: XCTestCase { func testRequestURI() throws { let request1 = try Request(url: "https://someserver.com:8888/some/path?foo=bar") - XCTAssertEqual(request1.host, "someserver.com") + XCTAssertEqual(request1.url.host, "someserver.com") XCTAssertEqual(request1.url.path, "/some/path") XCTAssertEqual(request1.url.query!, "foo=bar") XCTAssertEqual(request1.port, 8888) @@ -33,6 +33,22 @@ class HTTPClientTests: XCTestCase { XCTAssertEqual(request2.url.path, "") } + func testBadRequestURI() throws { + XCTAssertThrowsError(try Request(url: "some/path"), "should throw") { error in + XCTAssertEqual(error as! HTTPClientError, HTTPClientError.emptyScheme) + } + XCTAssertThrowsError(try Request(url: "file://somewhere/some/path?foo=bar"), "should throw") { error in + XCTAssertEqual(error as! HTTPClientError, HTTPClientError.unsupportedScheme("file")) + } + XCTAssertThrowsError(try Request(url: "https:/foo"), "should throw") { error in + XCTAssertEqual(error as! HTTPClientError, HTTPClientError.emptyHost) + } + } + + func testSchemaCasing() throws { + XCTAssertNoThrow(try Request(url: "hTTpS://someserver.com:8888/some/path?foo=bar")) + } + func testGet() throws { let httpBin = HttpBin() let httpClient = HTTPClient(eventLoopGroupProvider: .createNew) From 9c7e8d4696e91cb26d767b4a9a3cdfafe0621a0a Mon Sep 17 00:00:00 2001 From: tomer doron Date: Tue, 23 Jul 2019 09:20:08 -0700 Subject: [PATCH 2/4] make [more] immutable again --- Sources/AsyncHTTPClient/HTTPHandler.swift | 84 ++++++++++------------- 1 file changed, 35 insertions(+), 49 deletions(-) diff --git a/Sources/AsyncHTTPClient/HTTPHandler.swift b/Sources/AsyncHTTPClient/HTTPHandler.swift index a3e38dc30..04420a388 100644 --- a/Sources/AsyncHTTPClient/HTTPHandler.swift +++ b/Sources/AsyncHTTPClient/HTTPHandler.swift @@ -88,16 +88,19 @@ extension HTTPClient { /// Represent HTTP request. public struct Request { /// Request HTTP version, defaults to `HTTP/1.1`. - public var version: HTTPVersion + public let version: HTTPVersion /// Request HTTP method, defaults to `GET`. - public var method: HTTPMethod + public let method: HTTPMethod + /// Remote URL. + public let url: URL + /// Remote HTTP scheme, resolved from `URL`. + public let scheme: String + /// Remote host, resolved from `URL`. + public let host: String /// Request custom HTTP Headers, defaults to no headers. public var headers: HTTPHeaders /// Request body, defaults to no body. public var body: Body? - private var _url: URL! - private var _scheme: String! - private var _host: String! /// Create HTTP request. /// @@ -133,14 +136,6 @@ extension HTTPClient { /// - `unsupportedScheme` if URL does contains unsupported HTTP scheme. /// - `emptyHost` if URL does not contains a host. public init(url: URL, version: HTTPVersion = HTTPVersion(major: 1, minor: 1), method: HTTPMethod = .GET, headers: HTTPHeaders = HTTPHeaders(), body: Body? = nil) throws { - self.version = version - self.method = method - self.headers = headers - self.body = body - try self.setURL(url) - } - - public mutating func setURL(_ url: URL) throws { guard let scheme = url.scheme?.lowercased() else { throw HTTPClientError.emptyScheme } @@ -153,24 +148,13 @@ extension HTTPClient { throw HTTPClientError.emptyHost } - self._url = url - self._scheme = scheme - self._host = host - } - - /// Remote URL. - public var url: URL { - return self._url - } - - /// Remote HTTP scheme, resolved from `URL`. - public var scheme: String { - return self._scheme - } - - /// Remote host, resolved from `URL`. - public var host: String { - return self._host + self.version = version + self.method = method + self.url = url + self.scheme = scheme + self.host = host + self.headers = headers + self.body = body } /// Whether request will be executed using secure socket. @@ -668,14 +652,7 @@ internal struct RedirectHandler { } func redirect(status: HTTPResponseStatus, to redirectURL: URL, promise: EventLoopPromise) { - let originalURL = self.request.url - - var request = self.request - do { - try request.setURL(redirectURL) - } catch { - return promise.fail(error) - } + let originalRequest = self.request var convertToGet = false if status == .seeOther, request.method != .HEAD { @@ -684,20 +661,29 @@ internal struct RedirectHandler { convertToGet = true } + var method = originalRequest.method + var headers = originalRequest.headers + var body = originalRequest.body + if convertToGet { - request.method = .GET - request.body = nil - request.headers.remove(name: "Content-Length") - request.headers.remove(name: "Content-Type") + method = .GET + body = nil + headers.remove(name: "Content-Length") + headers.remove(name: "Content-Type") } - if !originalURL.hasTheSameOrigin(as: redirectURL) { - request.headers.remove(name: "Origin") - request.headers.remove(name: "Cookie") - request.headers.remove(name: "Authorization") - request.headers.remove(name: "Proxy-Authorization") + if !originalRequest.url.hasTheSameOrigin(as: redirectURL) { + headers.remove(name: "Origin") + headers.remove(name: "Cookie") + headers.remove(name: "Authorization") + headers.remove(name: "Proxy-Authorization") } - return self.execute(request).futureResult.cascade(to: promise) + do { + let newRequest = try HTTPClient.Request(url: redirectURL, version: originalRequest.version, method: method, headers: headers, body: body) + return self.execute(newRequest).futureResult.cascade(to: promise) + } catch { + return promise.fail(error) + } } } From a6d9e8437e01322b606ed6041015f348e9dbea5b Mon Sep 17 00:00:00 2001 From: tomer doron Date: Tue, 23 Jul 2019 12:00:27 -0700 Subject: [PATCH 3/4] drop version --- Sources/AsyncHTTPClient/HTTPHandler.swift | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/Sources/AsyncHTTPClient/HTTPHandler.swift b/Sources/AsyncHTTPClient/HTTPHandler.swift index 04420a388..f1d494257 100644 --- a/Sources/AsyncHTTPClient/HTTPHandler.swift +++ b/Sources/AsyncHTTPClient/HTTPHandler.swift @@ -87,8 +87,6 @@ extension HTTPClient { /// Represent HTTP request. public struct Request { - /// Request HTTP version, defaults to `HTTP/1.1`. - public let version: HTTPVersion /// Request HTTP method, defaults to `GET`. public let method: HTTPMethod /// Remote URL. @@ -115,12 +113,12 @@ extension HTTPClient { /// - `emptyScheme` if URL does not contain HTTP scheme. /// - `unsupportedScheme` if URL does contains unsupported HTTP scheme. /// - `emptyHost` if URL does not contains a host. - public init(url: String, version: HTTPVersion = HTTPVersion(major: 1, minor: 1), method: HTTPMethod = .GET, headers: HTTPHeaders = HTTPHeaders(), body: Body? = nil) throws { + public init(url: String, method: HTTPMethod = .GET, headers: HTTPHeaders = HTTPHeaders(), body: Body? = nil) throws { guard let url = URL(string: url) else { throw HTTPClientError.invalidURL } - try self.init(url: url, version: version, method: method, headers: headers, body: body) + try self.init(url: url, method: method, headers: headers, body: body) } /// Create an HTTP `Request`. @@ -135,7 +133,7 @@ extension HTTPClient { /// - `emptyScheme` if URL does not contain HTTP scheme. /// - `unsupportedScheme` if URL does contains unsupported HTTP scheme. /// - `emptyHost` if URL does not contains a host. - public init(url: URL, version: HTTPVersion = HTTPVersion(major: 1, minor: 1), method: HTTPMethod = .GET, headers: HTTPHeaders = HTTPHeaders(), body: Body? = nil) throws { + public init(url: URL, method: HTTPMethod = .GET, headers: HTTPHeaders = HTTPHeaders(), body: Body? = nil) throws { guard let scheme = url.scheme?.lowercased() else { throw HTTPClientError.emptyScheme } @@ -148,7 +146,6 @@ extension HTTPClient { throw HTTPClientError.emptyHost } - self.version = version self.method = method self.url = url self.scheme = scheme @@ -444,10 +441,10 @@ internal class TaskHandler: ChannelInboundHandler self.state = .idle let request = unwrapOutboundIn(data) - var head = HTTPRequestHead(version: request.version, method: request.method, uri: request.url.uri) + var head = HTTPRequestHead(version: HTTPVersion(major: 1, minor: 1), method: request.method, uri: request.url.uri) var headers = request.headers - if request.version.major == 1, request.version.minor == 1, !request.headers.contains(name: "Host") { + if !request.headers.contains(name: "Host") { headers.add(name: "Host", value: request.host) } @@ -680,7 +677,7 @@ internal struct RedirectHandler { } do { - let newRequest = try HTTPClient.Request(url: redirectURL, version: originalRequest.version, method: method, headers: headers, body: body) + let newRequest = try HTTPClient.Request(url: redirectURL, method: method, headers: headers, body: body) return self.execute(newRequest).futureResult.cascade(to: promise) } catch { return promise.fail(error) From 1ff199a8af96a82e2b2dcb48e363e744c0960521 Mon Sep 17 00:00:00 2001 From: tomer doron Date: Tue, 30 Jul 2019 09:14:20 -0700 Subject: [PATCH 4/4] format --- Sources/AsyncHTTPClient/HTTPHandler.swift | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Sources/AsyncHTTPClient/HTTPHandler.swift b/Sources/AsyncHTTPClient/HTTPHandler.swift index f1d494257..3de8aaa68 100644 --- a/Sources/AsyncHTTPClient/HTTPHandler.swift +++ b/Sources/AsyncHTTPClient/HTTPHandler.swift @@ -637,7 +637,7 @@ internal struct RedirectHandler { return nil } - guard HTTPClient.Request.isSchemeSupported(scheme: request.scheme) else { + guard HTTPClient.Request.isSchemeSupported(scheme: self.request.scheme) else { return nil } @@ -652,9 +652,9 @@ internal struct RedirectHandler { let originalRequest = self.request var convertToGet = false - if status == .seeOther, request.method != .HEAD { + if status == .seeOther, self.request.method != .HEAD { convertToGet = true - } else if status == .movedPermanently || status == .found, request.method == .POST { + } else if status == .movedPermanently || status == .found, self.request.method == .POST { convertToGet = true }