From 4023a2ea525e0c46d9ea793770d83f75df8a1707 Mon Sep 17 00:00:00 2001 From: Nocturnal Date: Wed, 4 Jul 2018 15:14:00 -0600 Subject: [PATCH] Issues related to ClientRequest and 3xx redirection (#270) * Add tests for various redirection cases. * Fix off-by-one error when testing if we've redirected too much. * Implement correct handling of 303 status code when preparing to redirect. * Tweak where we count redirects so we don't erase response body when we could do a redirect but aren't. --- Sources/KituraNet/ClientRequest.swift | 26 +++++++-- Tests/KituraNetTests/ClientE2ETests.swift | 71 ++++++++++++++++++++++- 2 files changed, 90 insertions(+), 7 deletions(-) diff --git a/Sources/KituraNet/ClientRequest.swift b/Sources/KituraNet/ClientRequest.swift index e32ea3c..702adc9 100644 --- a/Sources/KituraNet/ClientRequest.swift +++ b/Sources/KituraNet/ClientRequest.swift @@ -675,11 +675,25 @@ private class CurlInvoker { var redirectUrl: UnsafeMutablePointer? = nil let infoRc = curlHelperGetInfoCString(handle, CURLINFO_REDIRECT_URL, &redirectUrl) if infoRc == CURLE_OK { - if redirectUrl != nil { - curlHelperSetOptString(handle, CURLOPT_URL, redirectUrl) - redirected = true - delegate?.prepareForRedirect() - redirectCount+=1 + if redirectUrl != nil { + redirectCount += 1 + if redirectCount <= maxRedirects { + // Prepare to do a redirect + curlHelperSetOptString(handle, CURLOPT_URL, redirectUrl) + var status: Int = -1 + let codeRc = curlHelperGetInfoLong(handle, CURLINFO_RESPONSE_CODE, &status) + // If the status code was 303 See Other, ensure that + // the redirect is done with a GET query rather than + // whatever might have just been used. + if codeRc == CURLE_OK && status == 303 { + _ = curlHelperSetOptInt(handle, CURLOPT_HTTPGET, 1) + } + redirected = true + delegate?.prepareForRedirect() + } + else { + redirected = false + } } else { redirected = false @@ -687,7 +701,7 @@ private class CurlInvoker { } } - } while rc == CURLE_OK && redirected && redirectCount < maxRedirects + } while rc == CURLE_OK && redirected } return rc diff --git a/Tests/KituraNetTests/ClientE2ETests.swift b/Tests/KituraNetTests/ClientE2ETests.swift index 5475d45..e6b9f15 100644 --- a/Tests/KituraNetTests/ClientE2ETests.swift +++ b/Tests/KituraNetTests/ClientE2ETests.swift @@ -16,6 +16,7 @@ import Foundation import Dispatch +import LoggerAPI import XCTest @@ -36,7 +37,8 @@ class ClientE2ETests: KituraNetTest { ("testPipelining", testPipelining), ("testPipeliningSpanningPackets", testPipeliningSpanningPackets), ("testSimpleHTTPClient", testSimpleHTTPClient), - ("testUrlURL", testUrlURL) + ("testUrlURL", testUrlURL), + ("testRedirection", testRedirection) ] } @@ -381,6 +383,47 @@ class ClientE2ETests: KituraNetTest { } } + func testRedirection() { + performServerTest(TestRedirectDelegate()) { expectation in + let redirLimitZero: ((ClientRequest) -> Void) = {request in + request.set(.maxRedirects(0)) + } + let redirLimitOne: ((ClientRequest) -> Void) = {request in + request.set(.maxRedirects(1)) + } + let redirLimitTwo: ((ClientRequest) -> Void) = {request in + request.set(.maxRedirects(2)) + } + self.performRequest("get", path: "/redir301", callback: {response in + XCTAssertEqual(response?.statusCode, HTTPStatusCode.OK, "Status code wasn't .Ok was \(String(describing: response?.statusCode))") + }) + self.performRequest("post", path: "/redir303", callback: {response in + XCTAssertEqual(response?.statusCode, HTTPStatusCode.OK, "Status code wasn't .OK was \(String(describing: response?.statusCode))") + XCTAssertEqual(try? response?.readString() ?? "", "GET", "Method after 303 redirection was \(String(describing: try? response?.readString())) instead of GET") + }) + self.performRequest("post", path: "/redir307", callback: {response in + XCTAssertEqual(response?.statusCode, HTTPStatusCode.OK, "Status code wasn't .OK was \(String(describing: response?.statusCode))") + XCTAssertEqual(try? response?.readString() ?? "", "POST", "Method after 307 redirection was \(String(describing: try? response?.readString())) instead of POST") + }) + // Test no redirection + self.performRequest("get", path: "/redir303", callback: {response in + XCTAssertEqual(response?.statusCode, HTTPStatusCode.seeOther, "Status code wasn't .seeOther was \(String(describing: response?.statusCode))") + }, requestModifier: redirLimitZero) + // Test one redirect limit + self.performRequest("get", path: "/redir303", callback: {response in + XCTAssertEqual(response?.statusCode, HTTPStatusCode.OK, "Status code wasn't .OK was \(String(describing: response?.statusCode))") + }, requestModifier: redirLimitOne) + self.performRequest("get", path: "/redir303Twice", callback: {response in + XCTAssertEqual(response?.statusCode, HTTPStatusCode.seeOther, "Status code wasn't .seeOther was \(String(describing: response?.statusCode))") + }, requestModifier: redirLimitOne) + // Test two redirect limit + self.performRequest("get", path: "/redir303Twice", callback: {response in + XCTAssertEqual(response?.statusCode, HTTPStatusCode.OK, "Status code wasn't .OK was \(String(describing: response?.statusCode))") + }, requestModifier: redirLimitTwo) + expectation.fulfill() + } + } + class TestServerDelegate: ServerDelegate { func handle(request: ServerRequest, response: ServerResponse) { @@ -427,6 +470,32 @@ class ClientE2ETests: KituraNetTest { } } + class TestRedirectDelegate: ServerDelegate { + + func handle(request: ServerRequest, response: ServerResponse) { + switch request.urlURL.path { + case "/target": + response.statusCode = .OK + try! response.write(from: request.method.uppercased()) + case "/redir301": + response.statusCode = .movedPermanently + response.headers["Location"] = ["/target"] + case "/redir303": + response.statusCode = .seeOther + response.headers["Location"] = ["/target"] + case "/redir307": + response.statusCode = .temporaryRedirect + response.headers["Location"] = ["/target"] + case "/redir303Twice": + response.statusCode = .seeOther + response.headers["Location"] = ["/redir303"] + default: + XCTFail("Unexpected request path in TestRedirectDelegate.handle()") + } + try! response.end() + } + } + class TestURLDelegate: ServerDelegate { func handle(request: ServerRequest, response: ServerResponse) {