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.
Kitura · Jul 4, 2018 · 4023a2e · 4023a2e
1 parent f09bc9b
commit 4023a2e
Show file tree

Hide file tree

Showing 2 changed files with 90 additions and 7 deletions.
diff --git a/Sources/KituraNet/ClientRequest.swift b/Sources/KituraNet/ClientRequest.swift
@@ -675,19 +675,33 @@ private class CurlInvoker {
  var redirectUrl: UnsafeMutablePointer<Int8>? = 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
  }
  }
  }
 
- } 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
@@ -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) {