Skip to content

improve request validation #67

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jul 30, 2019
Merged

improve request validation #67

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d87dc01
improve request validation
tomerd Jul 10, 2019
9c7e8d4
make [more] immutable again
tomerd Jul 23, 2019
a6d9e84
drop version
tomerd Jul 23, 2019
1ff199a
format
tomerd Jul 30, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 35 additions & 44 deletions Sources/AsyncHTTPClient/HTTPHandler.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,16 +87,14 @@ extension HTTPClient {

/// Represent HTTP request.
public struct Request {
/// Request HTTP version, defaults to `HTTP/1.1`.
public var version: HTTPVersion
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Johannes suggest to drop version from request completely (see discussion in #56)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

even better! done in 73c400f

/// Request HTTP method, defaults to `GET`.
public var method: HTTPMethod
public let method: HTTPMethod
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does method have to be let? It's not part of the url change verification, so we can keep it as var (maybe move closer to all other vars in the file?

/// Remote URL.
public var url: URL
public let url: URL
/// Remote HTTP scheme, resolved from `URL`.
public var scheme: String
public let scheme: String
/// Remote host, resolved from `URL`.
public var host: String
public let host: String
/// Request custom HTTP Headers, defaults to no headers.
public var headers: HTTPHeaders
/// Request body, defaults to no body.
Expand All @@ -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`.
Expand All @@ -135,8 +133,8 @@ 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 {
guard let scheme = url.scheme else {
public init(url: URL, method: HTTPMethod = .GET, headers: HTTPHeaders = HTTPHeaders(), body: Body? = nil) throws {
guard let scheme = url.scheme?.lowercased() else {
throw HTTPClientError.emptyScheme
}

Expand All @@ -148,7 +146,6 @@ extension HTTPClient {
throw HTTPClientError.emptyHost
}

self.version = version
self.method = method
self.url = url
self.scheme = scheme
Expand All @@ -159,15 +156,15 @@ extension HTTPClient {

/// Whether request will be executed using secure socket.
public var useTLS: Bool {
return self.url.scheme == "https"
return self.scheme == "https"
}

/// Resolved port.
public var port: Int {
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"
}
}
Expand Down Expand Up @@ -444,10 +441,10 @@ internal class TaskHandler<T: HTTPClientResponseDelegate>: 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)
}

Expand Down Expand Up @@ -640,7 +637,7 @@ internal struct RedirectHandler<T> {
return nil
}

guard HTTPClient.Request.isSchemeSupported(scheme: url.scheme) else {
guard HTTPClient.Request.isSchemeSupported(scheme: self.request.scheme) else {
return nil
}

Expand All @@ -652,44 +649,38 @@ internal struct RedirectHandler<T> {
}

func redirect(status: HTTPResponseStatus, to redirectURL: URL, promise: EventLoopPromise<T>) {
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")
}
let originalRequest = self.request
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think having something like:

var request = try HTTPClient.Request(url: redirectURL, method: method, headers: headers, body: body)
...
if convertToGet {
    request.method = .GET
    request.body = nil
    request.headers.remove(name: "Content-Length")
    request.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")
}

return self.execute(newRequest).futureResult.cascade(to: promise)

could look event simpler, wdyt?


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
}

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, method: method, headers: headers, body: body)
return self.execute(newRequest).futureResult.cascade(to: promise)
} catch {
return promise.fail(error)
}
}
}
2 changes: 2 additions & 0 deletions Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
18 changes: 17 additions & 1 deletion Tests/AsyncHTTPClientTests/HTTPClientTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Expand Down