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 {
-case200..<300:
+case200..<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
 privatefunc handleConnect(context:ChannelHandlerContext)->EventLoopFuture<Void>{
 returnself.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):
 varpart= 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
-publicprotocolHTTPClientResponseDelegate:class{
+publicprotocolHTTPClientResponseDelegate:AnyObject{
 associatedtypeResponse
 
 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
 leterror=HTTPClientError.readTimeout
- delegate.didReceiveError(task:self.task, error)
- promise.fail(error)
+self.delegate.didReceiveError(task:self.task, error)
+self.promise.fail(error)
 }elseif(event as?TaskCancelEvent)!=nil{
 self.state =.end
 leterror=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
 leterror=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
 
 internalstructRedirectHandler<T>{
 letrequest:HTTPClient.Request
-letexecute:((HTTPClient.Request)->HTTPClient.Task<T>)
+letexecute:(HTTPClient.Request)->HTTPClient.Task<T>
 
 func redirectTarget(status:HTTPResponseStatus, headers:HTTPHeaders)->URL?{
 switch status {
@@ -443,6 +443,18 @@ internal struct RedirectHandler<T>{
 varrequest=self.request
  request.url = redirectURL
 
+iflet redirectHost = redirectURL.host {
+ request.host = redirectHost
+}else{
+preconditionFailure("redirectURL doesn't contain a host")
+}
+
+iflet redirectScheme = redirectURL.scheme {
+ request.scheme = redirectScheme
+}else{
+preconditionFailure("redirectURL doesn't contain a scheme")
+}
+
 varconvertToGet=false
 if status ==.seeOther, request.method !=.HEAD {
  convertToGet =true

Sources/NIOHTTPClient/SwiftNIOHTTP.swift

@@ -115,7 +115,6 @@ public class HTTPClient{
 
 publicfunc execute<T:HTTPClientResponseDelegate>(request:Request, delegate:T, timeout:Timeout?=nil)->Task<T.Response>{
 lettimeout= timeout ?? configuration.timeout
-
 letpromise:EventLoopPromise<T.Response>=self.eventLoopGroup.next().makePromise()
 
 letredirectHandler:RedirectHandler<T.Response>?
@@ -151,12 +150,12 @@ public class HTTPClient{
 lettaskHandler=TaskHandler(task: task, delegate: delegate, promise: promise, redirectHandler: redirectHandler)
 return channel.pipeline.addHandler(taskHandler)
 }
-}
+}
 
 iflet connectTimeout = timeout.connect {
  bootstrap = bootstrap.connectTimeout(connectTimeout)
 }
-
+
 letaddress=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
 }
 
-privatefunc resolveAddress(request:Request, proxy:Proxy?)->(host:String, port:Int){
+privatefunc resolveAddress(request:Request, proxy:Proxy?)->(host:String, port:Int){
 switchself.configuration.proxy {
 case.none:
 return(request.host, request.port)
@@ -216,7 +215,7 @@ public class HTTPClient{
 privateextensionChannelPipeline{
 func addProxyHandler(for request:HTTPClient.Request, decoder:ByteToMessageHandler<HTTPResponseDecoder>, encoder:HTTPRequestEncoder, tlsConfiguration:TLSConfiguration?)->EventLoopFuture<Void>{
 lethandler=HTTPClientProxyHandler(host: request.host, port: request.port, onConnect:{ channel in
-returnchannel.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):
 varbuffer= 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
-returnchannel.pipeline.configureHTTPServerPipeline(withPipeliningAssistance:true, withErrorHandling:true).flatMap{
+ channel.pipeline.configureHTTPServerPipeline(withPipeliningAssistance:true, withErrorHandling:true).flatMap{
 iflet simulateProxy = simulateProxy {
 return channel.pipeline.addHandler(HTTPProxySimulator(option: simulateProxy), position:.first)
 }else{
@@ -216,13 +216,27 @@ internal final class HttpBinHandler: ChannelInboundHandler{
 case"/redirect/302":
 varheaders=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":
-letport=value(for:"port", from: url.query!)
+letport=self.value(for:"port", from: url.query!)
 varheaders=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":
+letport=self.value(for:"port", from: url.query!)
+varheaders=HTTPHeaders()
+ headers.add(name:"Location", value:"http://127.0.0.1:\(port)/echohostheader")
+self.resps.append(HTTPResponseBuilder(status:.found, headers: headers))
+return
+case"/echohostheader":
+varbuilder=HTTPResponseBuilder(status:.ok)
+lethostValue= req.headers["Host"].first ??""
+varbuff= 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):
-varresponse= resps.removeFirst()
+varresponse=self.resps.removeFirst()
  response.add(body)
- resps.prepend(response)
+self.resps.prepend(response)
 case.end:
 ifself.resps.isEmpty {
 return
 }
-letresponse= resps.removeFirst()
+letresponse=self.resps.removeFirst()
  context.write(wrapOutboundOut(.head(response.head)), promise:nil)
 iflet body = response.body {
 letdata= body.withUnsafeReadableBytes{
@@ -288,7 +302,7 @@ internal final class HttpBinHandler: ChannelInboundHandler{
 }
 }
 
-fileprivateletcert="""
+privateletcert="""
 -----BEGIN CERTIFICATE-----
 MIICmDCCAYACCQCPC8JDqMh1zzANBgkqhkiG9w0BAQsFADANMQswCQYDVQQGEwJ1
 czAgFw0xODEwMzExNTU1MjJaGA8yMTE4MTAwNzE1NTUyMlowDTELMAkGA1UEBhMC
@@ -307,7 +321,7 @@ Au4LoEYwT730QKC/VQxxEVZobjn9/sTrq9CZlbPYHxX4fz6e00sX7H9i49vk9zQ5
 -----END CERTIFICATE-----
 """
 
-fileprivateletkey="""
+privateletkey="""
 -----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
 @testableimport NIOHTTP1
 @testableimport NIOHTTPClient
 import NIOSSL
@@ -171,6 +172,30 @@ class SwiftHTTPTests: XCTestCase{
 XCTAssertEqual(response.status,.ok)
 }
 
+func testHttpHostRedirect()throws{
+lethttpBin=HttpBin(ssl:false)
+lethttpClient=HTTPClient(eventLoopGroupProvider:.createNew,
+ configuration:HTTPClient.Configuration(certificateVerification:.none, followRedirects:true))
+
+defer{
+try! httpClient.syncShutdown()
+ httpBin.shutdown()
+}
+
+letresponse=try httpClient.get(url:"http://localhost:\(httpBin.port)/redirect/loopback?port=\(httpBin.port)").wait()
+guardvar body = response.body else{
+XCTFail("The target page should have a body containing the value of the Host header")
+return
+}
+guardlet responseData = body.readData(length: body.readableBytes)else{
+XCTFail("Read data shouldn't be nil since we passed body.readableBytes to body.readData")
+return
+}
+letdecoder=JSONDecoder()
+lethostName=try decoder.decode([String:String].self, from: responseData)["data"]
+XCTAssert(hostName =="127.0.0.1")
+}
+
 func testMultipleContentLengthHeaders()throws{
 lethttpClient=HTTPClient(eventLoopGroupProvider:.createNew)
 defer{