Skip to content

add support for redirect limits #113

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 12 commits into from
Oct 23, 2019
Merged

add support for redirect limits #113

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
939aa2b
add support for redirect limits
artemredkin Sep 23, 2019
fca5971
Merge branch 'master' into redirect_limits
artemredkin Sep 30, 2019
b437111
add tests on linux
artemredkin Sep 30, 2019
99096c5
review fix: guard instead of if
artemredkin Sep 30, 2019
b3ffde6
review fixes
artemredkin Oct 1, 2019
bbad11f
review fix: make redirect state be part for request
artemredkin Oct 8, 2019
8b0e613
review fix: add a separate error for redirect cycles
artemredkin Oct 8, 2019
aa843de
Update Sources/AsyncHTTPClient/HTTPClient.swift
artemredkin Oct 11, 2019
7e2edf8
review fixes
artemredkin Oct 11, 2019
5209dc3
rewrite backpressure test
artemredkin Oct 14, 2019
cf2d4d6
fix formatting
artemredkin Oct 14, 2019
86cf7d8
Merge branch 'master' into redirect_limits
artemredkin Oct 23, 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
43 changes: 31 additions & 12 deletions Sources/AsyncHTTPClient/HTTPClient.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,15 +226,20 @@ public class HTTPClient {
channelEL: EventLoop? = nil,
deadline: NIODeadline? = nil) -> Task<Delegate.Response> {
let redirectHandler: RedirectHandler<Delegate.Response>?
if self.configuration.followRedirects {
switch self.configuration.redirects {
case .allow(let max, let allowCycles):
var request = request
if request.redirectState == nil {
request.redirectState = .init(count: max, visited: allowCycles ? nil : Set())
}
redirectHandler = RedirectHandler<Delegate.Response>(request: request) { newRequest in
self.execute(request: newRequest,
delegate: delegate,
eventLoop: delegateEL,
channelEL: channelEL,
deadline: deadline)
}
} else {
case .disallow:
redirectHandler = nil
}

Expand Down Expand Up @@ -317,33 +322,33 @@ public class HTTPClient {
/// - `305: Use Proxy`
/// - `307: Temporary Redirect`
/// - `308: Permanent Redirect`
public var followRedirects: Bool
public var redirects: RedirectPolicy
/// Default client timeout, defaults to no timeouts.
public var timeout: Timeout
/// Upstream proxy, defaults to no proxy.
public var proxy: Proxy?
/// Ignore TLS unclean shutdown error, defaults to `false`.
public var ignoreUncleanSSLShutdown: Bool

public init(tlsConfiguration: TLSConfiguration? = nil, followRedirects: Bool = false, timeout: Timeout = Timeout(), proxy: Proxy? = nil) {
self.init(tlsConfiguration: tlsConfiguration, followRedirects: followRedirects, timeout: timeout, proxy: proxy, ignoreUncleanSSLShutdown: false)
public init(tlsConfiguration: TLSConfiguration? = nil, redirects: RedirectPolicy = .allow(max: 5, allowCycles: false), timeout: Timeout = Timeout(), proxy: Proxy? = nil) {
Copy link
Contributor

Choose a reason for hiding this comment

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

There's quite a lot of code duplication here, with each initializer having the same hard-coded .allow(max: 5, allowCycles: false) default value. We should pull that out into a separate

static let defaultRedirectPolicy: RedirectPolicy = .allow(max: 5, allowCycles: false)

then this can be:

public init(tlsConfiguration: TLSConfiguration? = nil, redirects: RedirectPolicy = HTTPClient.defaultRedirectPolicy, timeout: Timeout = Timeout(), proxy: Proxy? = nil) {
  ...
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

then defaultRedirectPolicy would also have to be public, what if it will be optional instead?

self.init(tlsConfiguration: tlsConfiguration, redirects: redirects, timeout: timeout, proxy: proxy, ignoreUncleanSSLShutdown: false)
}

public init(tlsConfiguration: TLSConfiguration? = nil, followRedirects: Bool = false, timeout: Timeout = Timeout(), proxy: Proxy? = nil, ignoreUncleanSSLShutdown: Bool = false) {
public init(tlsConfiguration: TLSConfiguration? = nil, redirects: RedirectPolicy = .allow(max: 5, allowCycles: false), timeout: Timeout = Timeout(), proxy: Proxy? = nil, ignoreUncleanSSLShutdown: Bool = false) {
self.tlsConfiguration = tlsConfiguration
self.followRedirects = followRedirects
self.redirects = redirects
self.timeout = timeout
self.proxy = proxy
self.ignoreUncleanSSLShutdown = ignoreUncleanSSLShutdown
}

public init(certificateVerification: CertificateVerification, followRedirects: Bool = false, timeout: Timeout = Timeout(), proxy: Proxy? = nil) {
self.init(certificateVerification: certificateVerification, followRedirects: followRedirects, timeout: timeout, proxy: proxy, ignoreUncleanSSLShutdown: false)
public init(certificateVerification: CertificateVerification, redirects: RedirectPolicy = .allow(max: 5, allowCycles: false), timeout: Timeout = Timeout(), proxy: Proxy? = nil) {
self.init(certificateVerification: certificateVerification, redirects: redirects, timeout: timeout, proxy: proxy, ignoreUncleanSSLShutdown: false)
}

public init(certificateVerification: CertificateVerification, followRedirects: Bool = false, timeout: Timeout = Timeout(), proxy: Proxy? = nil, ignoreUncleanSSLShutdown: Bool = false) {
public init(certificateVerification: CertificateVerification, redirects: RedirectPolicy = .allow(max: 5, allowCycles: false), timeout: Timeout = Timeout(), proxy: Proxy? = nil, ignoreUncleanSSLShutdown: Bool = false) {
self.tlsConfiguration = TLSConfiguration.forClient(certificateVerification: certificateVerification)
self.followRedirects = followRedirects
self.redirects = redirects
self.timeout = timeout
self.proxy = proxy
self.ignoreUncleanSSLShutdown = ignoreUncleanSSLShutdown
Expand Down Expand Up @@ -423,6 +428,14 @@ extension HTTPClient.Configuration {
self.read = read
}
}

/// Specifies redirect processing settings.
public enum RedirectPolicy {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! What do people think about calling this RedirectConfiguration to match TLSConfiguration etc?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

+1 from me, @tomerd wdyt?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

/// Redirects are not followed.
case disallow
/// Redirects are followed with a specified limit. Cycle detection reqiures that all visited URL's are kept in memory.
case allow(max: Int, allowCycles: Bool)
Copy link
Contributor

@tomerd tomerd Oct 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

nit: "disable" and "enable" seem more natural than "disallow" and "allow"? since this is a feature flag not a security policy?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this struct was renamed to Policy, do you think RedirectPolicy.enable is better than RedirectPolicy.allow? I don't have a strong opinion here, wdyt? cc @ianpartridge

Copy link
Contributor

Choose a reason for hiding this comment

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

In the tests it reads as redirects: .allow(max: 5, allowCycles: false) which seems fine (although I would prefer redirection: instead of redirects:). .allow makes sense to me because redirection is something that is requested by the remote HTTP server via an HTTP return code, and we decide whether to allow it or not.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How about RedirectConfiguration.disallow and RedirectConfiguration.follow?

Copy link
Contributor

Choose a reason for hiding this comment

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

I like .follow 🙂

}
}

private extension ChannelPipeline {
Expand Down Expand Up @@ -472,6 +485,8 @@ public struct HTTPClientError: Error, Equatable, CustomStringConvertible {
case invalidProxyResponse
case contentLengthMissing
case proxyAuthenticationRequired
case redirectLimitReached
case redirectCycleDetected
}

private var code: Code
Expand Down Expand Up @@ -508,6 +523,10 @@ public struct HTTPClientError: Error, Equatable, CustomStringConvertible {
public static let invalidProxyResponse = HTTPClientError(code: .invalidProxyResponse)
/// Request does not contain `Content-Length` header.
public static let contentLengthMissing = HTTPClientError(code: .contentLengthMissing)
/// Proxy Authentication Required
/// Proxy Authentication Required.
public static let proxyAuthenticationRequired = HTTPClientError(code: .proxyAuthenticationRequired)
/// Redirect Limit reached.
public static let redirectLimitReached = HTTPClientError(code: .redirectLimitReached)
/// Redirect Cycle detected.
public static let redirectCycleDetected = HTTPClientError(code: .redirectCycleDetected)
}
32 changes: 31 additions & 1 deletion Sources/AsyncHTTPClient/HTTPHandler.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,13 @@ extension HTTPClient {
/// Request body, defaults to no body.
public var body: Body?

struct RedirectState {
var count: Int
var visited: Set<URL>?
}

var redirectState: RedirectState?

/// Create HTTP request.
///
/// - parameters:
Expand Down Expand Up @@ -152,6 +159,8 @@ extension HTTPClient {
self.host = host
self.headers = headers
self.body = body

self.redirectState = nil
}

/// Whether request will be executed using secure socket.
Expand Down Expand Up @@ -818,6 +827,26 @@ internal struct RedirectHandler<ResponseType> {
}

func redirect(status: HTTPResponseStatus, to redirectURL: URL, promise: EventLoopPromise<ResponseType>) {
var nextState: HTTPClient.Request.RedirectState?
if var state = request.redirectState {
guard state.count > 0 else {
return promise.fail(HTTPClientError.redirectLimitReached)
}

state.count -= 1

if var visited = state.visited {
guard !visited.contains(redirectURL) else {
return promise.fail(HTTPClientError.redirectCycleDetected)
}

visited.insert(redirectURL)
state.visited = visited
}

nextState = state
}

let originalRequest = self.request

var convertToGet = false
Expand Down Expand Up @@ -846,7 +875,8 @@ internal struct RedirectHandler<ResponseType> {
}

do {
let newRequest = try HTTPClient.Request(url: redirectURL, method: method, headers: headers, body: body)
var newRequest = try HTTPClient.Request(url: redirectURL, method: method, headers: headers, body: body)
newRequest.redirectState = nextState
return self.execute(newRequest).futureResult.cascade(to: promise)
} catch {
return promise.fail(error)
Expand Down
10 changes: 10 additions & 0 deletions Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,16 @@ internal final class HttpBinHandler: ChannelInboundHandler {
headers.add(name: "Location", value: "http://127.0.0.1:\(port)/echohostheader")
self.resps.append(HTTPResponseBuilder(status: .found, headers: headers))
return
case "/redirect/infinite1":
var headers = HTTPHeaders()
headers.add(name: "Location", value: "/redirect/infinite2")
self.resps.append(HTTPResponseBuilder(status: .found, headers: headers))
return
case "/redirect/infinite2":
var headers = HTTPHeaders()
headers.add(name: "Location", value: "/redirect/infinite1")
self.resps.append(HTTPResponseBuilder(status: .found, headers: headers))
return
// Since this String is taken from URL.path, the percent encoding has been removed
case "/percent encoded":
if req.method != .GET {
Expand Down
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 @@ -57,6 +57,8 @@ extension HTTPClientTests {
("testWrongContentLengthForSSLUncleanShutdown", testWrongContentLengthForSSLUncleanShutdown),
("testWrongContentLengthWithIgnoreErrorForSSLUncleanShutdown", testWrongContentLengthWithIgnoreErrorForSSLUncleanShutdown),
("testEventLoopArgument", testEventLoopArgument),
("testLoopDetectionRedirectLimit", testLoopDetectionRedirectLimit),
("testCountRedirectLimit", testCountRedirectLimit),
]
}
}
34 changes: 31 additions & 3 deletions Tests/AsyncHTTPClientTests/HTTPClientTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ class HTTPClientTests: XCTestCase {
let httpBin = HTTPBin(ssl: false)
let httpsBin = HTTPBin(ssl: true)
let httpClient = HTTPClient(eventLoopGroupProvider: .createNew,
configuration: HTTPClient.Configuration(certificateVerification: .none, followRedirects: true))
configuration: HTTPClient.Configuration(certificateVerification: .none, redirects: .allow(max: 10, allowCycles: true)))

defer {
XCTAssertNoThrow(try httpClient.syncShutdown())
Expand All @@ -148,7 +148,7 @@ class HTTPClientTests: XCTestCase {
func testHttpHostRedirect() throws {
let httpBin = HTTPBin(ssl: false)
let httpClient = HTTPClient(eventLoopGroupProvider: .createNew,
configuration: HTTPClient.Configuration(certificateVerification: .none, followRedirects: true))
configuration: HTTPClient.Configuration(certificateVerification: .none, redirects: .allow(max: 10, allowCycles: true)))

defer {
XCTAssertNoThrow(try httpClient.syncShutdown())
Expand Down Expand Up @@ -525,7 +525,7 @@ class HTTPClientTests: XCTestCase {
let httpBin = HTTPBin()
let eventLoopGroup = MultiThreadedEventLoopGroup(numberOfThreads: 5)
let httpClient = HTTPClient(eventLoopGroupProvider: .shared(eventLoopGroup),
configuration: HTTPClient.Configuration(followRedirects: true))
configuration: HTTPClient.Configuration(redirects: .allow(max: 10, allowCycles: true)))
defer {
XCTAssertNoThrow(try httpClient.syncShutdown())
XCTAssertNoThrow(try eventLoopGroup.syncShutdownGracefully())
Expand Down Expand Up @@ -563,4 +563,32 @@ class HTTPClientTests: XCTestCase {
response = try httpClient.execute(request: request, delegate: delegate, eventLoop: .delegate(on: eventLoop)).wait()
XCTAssertEqual(true, response)
}

func testLoopDetectionRedirectLimit() throws {
let httpBin = HTTPBin(ssl: true)
let httpClient = HTTPClient(eventLoopGroupProvider: .createNew,
configuration: HTTPClient.Configuration(certificateVerification: .none, redirects: .allow(max: 5, allowCycles: false)))
defer {
XCTAssertNoThrow(try httpClient.syncShutdown())
XCTAssertNoThrow(try httpBin.shutdown())
}

XCTAssertThrowsError(try httpClient.get(url: "https://localhost:\(httpBin.port)/redirect/infinite1").wait(), "Should fail with redirect limit") { error in
XCTAssertEqual(error as! HTTPClientError, HTTPClientError.redirectCycleDetected)
}
}

func testCountRedirectLimit() throws {
let httpBin = HTTPBin(ssl: true)
let httpClient = HTTPClient(eventLoopGroupProvider: .createNew,
configuration: HTTPClient.Configuration(certificateVerification: .none, redirects: .allow(max: 5, allowCycles: true)))
defer {
XCTAssertNoThrow(try httpClient.syncShutdown())
XCTAssertNoThrow(try httpBin.shutdown())
}

XCTAssertThrowsError(try httpClient.get(url: "https://localhost:\(httpBin.port)/redirect/infinite1").wait(), "Should fail with redirect limit") { error in
XCTAssertEqual(error as! HTTPClientError, HTTPClientError.redirectLimitReached)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want to test case insensitive loops?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

do you mean when schema and path are capitalised differently?

}