Merge pull request #2838 from agisboye/SR-13027

SR-13027: Fix URL encoding in redirects

Author: millenomi

Sources/FoundationNetworking/URLSession/HTTP/HTTPURLProtocol.swift

@@ -476,9 +476,9 @@ internal class _HTTPURLProtocol: _NativeProtocol {
             // We need this ugly cast in order to be able to support `URLSessionTask.init()`
             session.delegateQueue.addOperation {
                 delegate.urlSession(session, task: self.task!, willPerformHTTPRedirection: response as! HTTPURLResponse, newRequest: request) { [weak self] (request: URLRequest?) in
-                    guard let task = self else { return }
-                    self?.task?.workQueue.async {
-                        task.didCompleteRedirectCallback(request)
+                    guard let self = self else { return }
+                    self.task?.workQueue.async {
+                        self.didCompleteRedirectCallback(request)
                     }
                 }
             }
@@ -651,7 +651,7 @@ internal extension _HTTPURLProtocol {
         //TODO: Do we ever want to redirect for HEAD requests?
         func methodAndURL() -> (String, URL)? {
             guard
-                let location = response.value(forHeaderField: .location, response: response),
+                let location = response.value(forHeaderField: .location),
                 let targetURL = URL(string: location)
                 else {
                     // Can't redirect when there's no location to redirect to.
@@ -689,31 +689,33 @@ internal extension _HTTPURLProtocol {
             return request
         }
 
-        let scheme = request.url?.scheme
-        let host = request.url?.host
-        let port = request.url?.port
+        guard
+            let fromUrl = fromRequest.url,
+            var components = URLComponents(url: fromUrl, resolvingAgainstBaseURL: false)
+            else { return nil }
 
-        var components = URLComponents()
-        components.scheme = scheme
-        components.host = host
-        // Use the original port if the new URL does not contain a host
-        // ie Location: /foo => <original host>:<original port>/Foo
-        // but Location: newhost/foo  will ignore the original port
-        if targetURL.host == nil {
-          components.port = port
+        // If the new URL contains a host, use the host and port from the new URL.
+        // Otherwise, the host and port from the original URL are used.
+        if targetURL.host != nil {
+            components.host = targetURL.host
+            components.port = targetURL.port
         }
-        //The path must either begin with "/" or be an empty string.
-        if targetURL.relativeString.first != "/" {
-            components.path = "/" + targetURL.relativeString
+        
+        // The path must either begin with "/" or be an empty string.
+        if targetURL.path.hasPrefix("/") {
+            components.path = targetURL.path
         } else {
-            components.path = targetURL.relativeString
+            components.path = "/" + targetURL.path
         }
+        
+        // The query and fragment components are set separately to prevent them from being
+        // percent encoded again.
+        components.percentEncodedQuery = targetURL.query
+        components.percentEncodedFragment = targetURL.fragment
 
-        guard let urlString = components.string else { fatalError("Invalid URL") }
-        request.url = URL(string: urlString)
+        guard let url = components.url else { fatalError("Invalid URL") }
+        request.url = url
 
-        // Inherit the timeout from the previous request
-        request.timeoutInterval = fromRequest.timeoutInterval
         return request
     }
 }
@@ -726,10 +728,9 @@ fileprivate extension HTTPURLResponse {
         case location = "Location"
     }
 
-    func value(forHeaderField field: _Field, response: HTTPURLResponse?) -> String? {
+    func value(forHeaderField field: _Field) -> String? {
         let value = field.rawValue
-        guard let response = response else { fatalError("Response is nil") }
-        if let location = response.allHeaderFields[value] as? String {
+        if let location = self.allHeaderFields[value] as? String {
             return location
         }
         return nil

Tests/Foundation/HTTPServer.swift

@@ -841,6 +841,11 @@ public class TestURLSessionServer {
                                            "Content-Encoding: gzip"].joined(separator: _HTTPUtils.CRLF),
                                  bodyData: helloWorld)
         }
+        
+        if uri == "/echo-query" {
+            let body = request.parameters.map { "\($0.key)=\($0.value)" }.joined(separator: "&")
+            return try _HTTPResponse(response: .OK, body: body)
+        }
 
         // Look for /xxx where xxx is a 3digit HTTP code
         if uri.hasPrefix("/") && uri.count == 4, let code = Int(String(uri.dropFirst())), code > 0 && code < 1000 {

Tests/Foundation/Tests/TestURLSession.swift

@@ -748,6 +748,21 @@ class TestURLSession: LoopbackServerTest {
         d.run(with: url)
         waitForExpectations(timeout: 12)
     }
+    
+    func test_httpRedirectionWithEncodedQuery() {
+        let location = "echo-query%3Fparam%3Dfoo" // "echo-query?param=foo" url encoded
+        let urlString = "http://127.0.0.1:\(TestURLSession.serverPort)/303?location=\(location)"
+        let url = URL(string: urlString)!
+        let d = HTTPRedirectionDataTask(with: expectation(description: "GET \(urlString): with HTTP redirection"))
+        d.run(with: url)
+        waitForExpectations(timeout: 12)
+        
+        if let body = String(data: d.receivedData, encoding: .utf8) {
+            XCTAssertEqual(body, "param=foo")
+        } else {
+            XCTFail("No string body")
+        }
+    }
 
      // temporarily disabled (https://bugs.swift.org/browse/SR-5751)
     func test_httpRedirectionTimeout() {
@@ -1774,6 +1789,7 @@ class TestURLSession: LoopbackServerTest {
             ("test_httpRedirectionWithCompleteRelativePath", test_httpRedirectionWithCompleteRelativePath),
             ("test_httpRedirectionWithInCompleteRelativePath", test_httpRedirectionWithInCompleteRelativePath),
             ("test_httpRedirectionWithDefaultPort", test_httpRedirectionWithDefaultPort),
+            ("test_httpRedirectionWithEncodedQuery", test_httpRedirectionWithEncodedQuery),
             ("test_httpRedirectionTimeout", test_httpRedirectionTimeout),
             ("test_httpRedirectionChainInheritsTimeoutInterval", test_httpRedirectionChainInheritsTimeoutInterval),
             ("test_httpRedirectionExceededMaxRedirects", test_httpRedirectionExceededMaxRedirects),