Transfer scheme and host to new request when redirected (#20)

* Transfer scheme and host to new request when redirected

This solves a bug that could cause infinite redirections (eg: requesting example.com, being redirected to www.example.com, but requesting again example.com as host hadn't been correctly specified)

* Check that redirect correctly modifies Host to what's specified by the server response

* Move test to new test case, use Decodable inline struct instead of JSONSerialization

* Run swiftformat tool

* Use NIOFoundationCompat helper functions

* Stop using httpbin.org and use the local HttpBin server for the testHttpHostRedirect test

* Reformat files

* Check that redirectURL contains a host and scheme

* Run swiftformat

* Replace assertionFailure with preconditionFailure

Author: Trevör

Package.swift

@@ -31,7 +31,7 @@ let package = Package(
         ),
         .testTarget(
             name: "NIOHTTPClientTests",
-            dependencies: ["NIOHTTPClient"]
+            dependencies: ["NIOHTTPClient", "NIOFoundationCompat"]
         ),
     ]
 )

Sources/NIOHTTPClient/HTTPClientProxyHandler.swift

@@ -74,7 +74,7 @@ internal final class HTTPClientProxyHandler: ChannelDuplexHandler, RemovableChan
             switch res {
             case .head(let head):
                 switch head.status.code {
-                case 200..<300:
+                case 200 ..< 300:
                     // Any 2xx (Successful) response indicates that the sender (and all
                     // inbound proxies) will switch to tunnel mode immediately after the
                     // blank line that concludes the successful response's header section
@@ -116,7 +116,7 @@ internal final class HTTPClientProxyHandler: ChannelDuplexHandler, RemovableChan
     private func handleConnect(context: ChannelHandlerContext) -> EventLoopFuture<Void> {
         return self.onConnect(context.channel).flatMap {
             self.readState = .connected
-            
+
             // forward any buffered reads
             while !self.readBuffer.isEmpty {
                 context.fireChannelRead(self.readBuffer.removeFirst())

Sources/NIOHTTPClient/HTTPCookie.swift

@@ -68,7 +68,7 @@ public struct HTTPCookie {
                 formatter.locale = Locale(identifier: "en_US")
                 formatter.timeZone = TimeZone(identifier: "GMT")
                 formatter.dateFormat = "EEE, dd MMM yyyy HH:mm:ss z"
-                self.expires = parseComponentValue(component).flatMap { formatter.date(from: $0) }
+                self.expires = self.parseComponentValue(component).flatMap { formatter.date(from: $0) }
                 continue
             }
 

Sources/NIOHTTPClient/HTTPHandler.swift

@@ -140,7 +140,7 @@ internal class ResponseAccumulator: HTTPClientResponseDelegate {
         case .body(let head, var body):
             var part = part
             body.writeBuffer(&part)
-            state = .body(head, body)
+            self.state = .body(head, body)
         case .end:
             preconditionFailure("request already processed")
         case .error:
@@ -171,7 +171,7 @@ internal class ResponseAccumulator: HTTPClientResponseDelegate {
 /// This delegate is strongly held by the HTTPTaskHandler
 /// for the duration of the HTTPRequest processing and will be
 /// released together with the HTTPTaskHandler when channel is closed
-public protocol HTTPClientResponseDelegate: class {
+public protocol HTTPClientResponseDelegate: AnyObject {
     associatedtype Response
 
     func didTransmitRequestBody(task: HTTPClient.Task<Response>)
@@ -361,13 +361,13 @@ internal class TaskHandler<T: HTTPClientResponseDelegate>: ChannelInboundHandler
         if (event as? IdleStateHandler.IdleStateEvent) == .read {
             self.state = .end
             let error = HTTPClientError.readTimeout
-            delegate.didReceiveError(task: self.task, error)
-            promise.fail(error)
+            self.delegate.didReceiveError(task: self.task, error)
+            self.promise.fail(error)
         } else if (event as? TaskCancelEvent) != nil {
             self.state = .end
             let error = HTTPClientError.cancelled
-            delegate.didReceiveError(task: self.task, error)
-            promise.fail(error)
+            self.delegate.didReceiveError(task: self.task, error)
+            self.promise.fail(error)
         } else {
             context.fireUserInboundEventTriggered(event)
         }
@@ -380,8 +380,8 @@ internal class TaskHandler<T: HTTPClientResponseDelegate>: ChannelInboundHandler
         default:
             self.state = .end
             let error = HTTPClientError.remoteConnectionClosed
-            delegate.didReceiveError(task: self.task, error)
-            promise.fail(error)
+            self.delegate.didReceiveError(task: self.task, error)
+            self.promise.fail(error)
         }
     }
 
@@ -408,7 +408,7 @@ internal class TaskHandler<T: HTTPClientResponseDelegate>: ChannelInboundHandler
 
 internal struct RedirectHandler<T> {
     let request: HTTPClient.Request
-    let execute: ((HTTPClient.Request) -> HTTPClient.Task<T>)
+    let execute: (HTTPClient.Request) -> HTTPClient.Task<T>
 
     func redirectTarget(status: HTTPResponseStatus, headers: HTTPHeaders) -> URL? {
         switch status {
@@ -443,6 +443,18 @@ internal struct RedirectHandler<T> {
         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")
+        }
+
         var convertToGet = false
         if status == .seeOther, request.method != .HEAD {
             convertToGet = true

Sources/NIOHTTPClient/SwiftNIOHTTP.swift

@@ -115,7 +115,6 @@ public class HTTPClient {
 
     public func execute<T: HTTPClientResponseDelegate>(request: Request, delegate: T, timeout: Timeout? = nil) -> Task<T.Response> {
         let timeout = timeout ?? configuration.timeout
-
         let promise: EventLoopPromise<T.Response> = self.eventLoopGroup.next().makePromise()
 
         let redirectHandler: RedirectHandler<T.Response>?
@@ -151,12 +150,12 @@ public class HTTPClient {
                     let taskHandler = TaskHandler(task: task, delegate: delegate, promise: promise, redirectHandler: redirectHandler)
                     return channel.pipeline.addHandler(taskHandler)
                 }
-        }
+            }
 
         if let connectTimeout = timeout.connect {
             bootstrap = bootstrap.connectTimeout(connectTimeout)
         }
-        
+
         let address = self.resolveAddress(request: request, proxy: self.configuration.proxy)
         bootstrap.connect(host: address.host, port: address.port)
             .map { channel in
@@ -172,7 +171,7 @@ public class HTTPClient {
         return task
     }
 
-    private func resolveAddress(request: Request, proxy: Proxy?) -> (host: String, port: Int)  {
+    private func resolveAddress(request: Request, proxy: Proxy?) -> (host: String, port: Int) {
         switch self.configuration.proxy {
         case .none:
             return (request.host, request.port)
@@ -216,7 +215,7 @@ public class HTTPClient {
 private extension ChannelPipeline {
     func addProxyHandler(for request: HTTPClient.Request, decoder: ByteToMessageHandler<HTTPResponseDecoder>, encoder: HTTPRequestEncoder, tlsConfiguration: TLSConfiguration?) -> EventLoopFuture<Void> {
         let handler = HTTPClientProxyHandler(host: request.host, port: request.port, onConnect: { channel in
-            return channel.pipeline.removeHandler(decoder).flatMap {
+            channel.pipeline.removeHandler(decoder).flatMap {
                 return channel.pipeline.addHandler(
                     ByteToMessageHandler(HTTPResponseDecoder(leftOverBytesStrategy: .forwardBytes)),
                     position: .after(encoder)

Tests/NIOHTTPClientTests/HTTPClientTestUtils.swift

@@ -34,7 +34,7 @@ class TestHTTPDelegate: HTTPClientResponseDelegate {
         case .body(let head, var body):
             var buffer = buffer
             body.writeBuffer(&buffer)
-            state = .body(head, body)
+            self.state = .body(head, body)
         default:
             preconditionFailure("expecting head or body")
         }
@@ -94,11 +94,11 @@ internal class HttpBin {
     }
 
     init(ssl: Bool = false, simulateProxy: HTTPProxySimulator.Option? = nil) {
-        self.serverChannel = try! ServerBootstrap(group: group)
+        self.serverChannel = try! ServerBootstrap(group: self.group)
             .serverChannelOption(ChannelOptions.socket(SocketOptionLevel(SOL_SOCKET), SO_REUSEADDR), value: 1)
             .childChannelOption(ChannelOptions.socket(IPPROTO_TCP, TCP_NODELAY), value: 1)
             .childChannelInitializer { channel in
-                return channel.pipeline.configureHTTPServerPipeline(withPipeliningAssistance: true, withErrorHandling: true).flatMap {
+                channel.pipeline.configureHTTPServerPipeline(withPipeliningAssistance: true, withErrorHandling: true).flatMap {
                     if let simulateProxy = simulateProxy {
                         return channel.pipeline.addHandler(HTTPProxySimulator(option: simulateProxy), position: .first)
                     } else {
@@ -216,13 +216,27 @@ internal final class HttpBinHandler: ChannelInboundHandler {
             case "/redirect/302":
                 var headers = HTTPHeaders()
                 headers.add(name: "Location", value: "/ok")
-                resps.append(HTTPResponseBuilder(status: .found, headers: headers))
+                self.resps.append(HTTPResponseBuilder(status: .found, headers: headers))
                 return
             case "/redirect/https":
-                let port = value(for: "port", from: url.query!)
+                let port = self.value(for: "port", from: url.query!)
                 var headers = HTTPHeaders()
                 headers.add(name: "Location", value: "https://localhost:\(port)/ok")
-                resps.append(HTTPResponseBuilder(status: .found, headers: headers))
+                self.resps.append(HTTPResponseBuilder(status: .found, headers: headers))
+                return
+            case "/redirect/loopback":
+                let port = self.value(for: "port", from: url.query!)
+                var headers = HTTPHeaders()
+                headers.add(name: "Location", value: "http://127.0.0.1:\(port)/echohostheader")
+                self.resps.append(HTTPResponseBuilder(status: .found, headers: headers))
+                return
+            case "/echohostheader":
+                var builder = HTTPResponseBuilder(status: .ok)
+                let hostValue = req.headers["Host"].first ?? ""
+                var buff = context.channel.allocator.buffer(capacity: hostValue.utf8.count)
+                buff.writeString(hostValue)
+                builder.add(buff)
+                self.resps.append(builder)
                 return
             case "/wait":
                 return
@@ -246,14 +260,14 @@ internal final class HttpBinHandler: ChannelInboundHandler {
                 return
             }
         case .body(let body):
-            var response = resps.removeFirst()
+            var response = self.resps.removeFirst()
             response.add(body)
-            resps.prepend(response)
+            self.resps.prepend(response)
         case .end:
             if self.resps.isEmpty {
                 return
             }
-            let response = resps.removeFirst()
+            let response = self.resps.removeFirst()
             context.write(wrapOutboundOut(.head(response.head)), promise: nil)
             if let body = response.body {
                 let data = body.withUnsafeReadableBytes {
@@ -288,7 +302,7 @@ internal final class HttpBinHandler: ChannelInboundHandler {
     }
 }
 
-fileprivate let cert = """
+private let cert = """
 -----BEGIN CERTIFICATE-----
 MIICmDCCAYACCQCPC8JDqMh1zzANBgkqhkiG9w0BAQsFADANMQswCQYDVQQGEwJ1
 czAgFw0xODEwMzExNTU1MjJaGA8yMTE4MTAwNzE1NTUyMlowDTELMAkGA1UEBhMC
@@ -307,7 +321,7 @@ Au4LoEYwT730QKC/VQxxEVZobjn9/sTrq9CZlbPYHxX4fz6e00sX7H9i49vk9zQ5
 -----END CERTIFICATE-----
 """
 
-fileprivate let key = """
+private let key = """
 -----BEGIN PRIVATE KEY-----
 MIIEvwIBADANBgkqhkiG9w0BAQEFAASCBKkwggSlAgEAAoIBAQDiC+TGmbSP/nWW
 N1tjyNfnWCU5ATjtIOfdtP6ycx8JSeqkvyNXG21kNUn14jTTU8BglGL2hfVpCbMi

Tests/NIOHTTPClientTests/SwiftNIOHTTPTests+XCTest.swift

@@ -33,6 +33,7 @@ extension SwiftHTTPTests {
             ("testGetHttps", testGetHttps),
             ("testPostHttps", testPostHttps),
             ("testHttpRedirect", testHttpRedirect),
+            ("testHttpHostRedirect", testHttpHostRedirect),
             ("testMultipleContentLengthHeaders", testMultipleContentLengthHeaders),
             ("testStreaming", testStreaming),
             ("testRemoteClose", testRemoteClose),

Tests/NIOHTTPClientTests/SwiftNIOHTTPTests.swift

@@ -14,6 +14,7 @@
 
 import Foundation
 import NIO
+import NIOFoundationCompat
 @testable import NIOHTTP1
 @testable import NIOHTTPClient
 import NIOSSL
@@ -171,6 +172,30 @@ class SwiftHTTPTests: XCTestCase {
         XCTAssertEqual(response.status, .ok)
     }
 
+    func testHttpHostRedirect() throws {
+        let httpBin = HttpBin(ssl: false)
+        let httpClient = HTTPClient(eventLoopGroupProvider: .createNew,
+                                    configuration: HTTPClient.Configuration(certificateVerification: .none, followRedirects: true))
+
+        defer {
+            try! httpClient.syncShutdown()
+            httpBin.shutdown()
+        }
+
+        let response = try httpClient.get(url: "http://localhost:\(httpBin.port)/redirect/loopback?port=\(httpBin.port)").wait()
+        guard var body = response.body else {
+            XCTFail("The target page should have a body containing the value of the Host header")
+            return
+        }
+        guard let responseData = body.readData(length: body.readableBytes) else {
+            XCTFail("Read data shouldn't be nil since we passed body.readableBytes to body.readData")
+            return
+        }
+        let decoder = JSONDecoder()
+        let hostName = try decoder.decode([String: String].self, from: responseData)["data"]
+        XCTAssert(hostName == "127.0.0.1")
+    }
+
     func testMultipleContentLengthHeaders() throws {
         let httpClient = HTTPClient(eventLoopGroupProvider: .createNew)
         defer {