From c2f1c2c893270abd2e9dab9c39a79ca8048d55a2 Mon Sep 17 00:00:00 2001 From: Saleem Abdulrasool Date: Fri, 27 Mar 2020 22:46:15 -0700 Subject: [PATCH] Revert "URLSession: Fix redirection response handling" --- .../URLSession/HTTP/HTTPURLProtocol.swift | 32 +-- .../URLSession/NativeProtocol.swift | 7 - Tests/Foundation/Tests/TestURLSession.swift | 268 ++---------------- 3 files changed, 35 insertions(+), 272 deletions(-) diff --git a/Sources/FoundationNetworking/URLSession/HTTP/HTTPURLProtocol.swift b/Sources/FoundationNetworking/URLSession/HTTP/HTTPURLProtocol.swift index f56ecefdfa..22d06100d7 100644 --- a/Sources/FoundationNetworking/URLSession/HTTP/HTTPURLProtocol.swift +++ b/Sources/FoundationNetworking/URLSession/HTTP/HTTPURLProtocol.swift @@ -401,8 +401,9 @@ internal class _HTTPURLProtocol: _NativeProtocol { easyHandle.timeoutTimer = _TimeoutSource(queue: task.workQueue, milliseconds: timeoutInterval, handler: timeoutHandler) easyHandle.set(automaticBodyDecompression: true) easyHandle.set(requestMethod: request.httpMethod ?? "GET") - // Always set the status as it may change if a HEAD is converted to a GET. - easyHandle.set(noBody: request.httpMethod == "HEAD") + if request.httpMethod == "HEAD" { + easyHandle.set(noBody: true) + } } /// What action to take @@ -590,7 +591,7 @@ internal extension _HTTPURLProtocol { // For now, we'll notify the delegate, but won't pause the transfer, // and we'll disregard the completion handler: switch response.statusCode { - case 301, 302, 303, 305...308: + case 301, 302, 303, 307: break default: self.client?.urlProtocol(self, didReceive: response, cacheStoragePolicy: .notAllowed) @@ -618,26 +619,19 @@ internal extension _HTTPURLProtocol { return nil } - let method = fromRequest.httpMethod ?? "GET" // Check for a redirect: switch response.statusCode { - case 301, 302: - // Change "POST" into "GET" but leave other methods unchanged: - let newMethod = (method == "POST") ? "GET" : method - return (newMethod, targetURL) - - case 303: - return ("GET", targetURL) - - case 305...308: - // Re-use existing method: - return (method, targetURL) - - default: - return nil + //TODO: Should we do this for 300 "Multiple Choices", too? + case 301, 302, 303: + // Change into "GET": + return ("GET", targetURL) + case 307: + // Re-use existing method: + return (fromRequest.httpMethod ?? "GET", targetURL) + default: + return nil } } - guard let (method, targetURL) = methodAndURL() else { return nil } var request = fromRequest request.httpMethod = method diff --git a/Sources/FoundationNetworking/URLSession/NativeProtocol.swift b/Sources/FoundationNetworking/URLSession/NativeProtocol.swift index 1ae8a3a491..cb13b6e044 100644 --- a/Sources/FoundationNetworking/URLSession/NativeProtocol.swift +++ b/Sources/FoundationNetworking/URLSession/NativeProtocol.swift @@ -102,13 +102,6 @@ internal class _NativeProtocol: URLProtocol, _EasyHandleDelegate { if let response = validateHeaderComplete(transferState:ts) { ts.response = response } - - // Note this excludes code 300 which should return the response of the redirect and not follow it. - // For other redirect codes dont notify the delegate of the data received in the redirect response. - if let httpResponse = ts.response as? HTTPURLResponse, 301...308 ~= httpResponse.statusCode { - return .proceed - } - notifyDelegate(aboutReceivedData: data) internalState = .transferInProgress(ts.byAppending(bodyData: data)) return .proceed diff --git a/Tests/Foundation/Tests/TestURLSession.swift b/Tests/Foundation/Tests/TestURLSession.swift index cb814cc8cf..8454d2755f 100644 --- a/Tests/Foundation/Tests/TestURLSession.swift +++ b/Tests/Foundation/Tests/TestURLSession.swift @@ -1,13 +1,13 @@ // This source file is part of the Swift.org open source project // -// Copyright (c) 2014 - 2020 Apple Inc. and the Swift project authors +// Copyright (c) 2014 - 2016 Apple Inc. and the Swift project authors // Licensed under Apache License v2.0 with Runtime Library Exception // -// See https://swift.org/LICENSE.txt for license information -// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors +// See http://swift.org/LICENSE.txt for license information +// See http://swift.org/CONTRIBUTORS.txt for the list of Swift project authors // -class TestURLSession: LoopbackServerTest { +class TestURLSession : LoopbackServerTest { let httpMethods = ["HEAD", "GET", "PUT", "POST", "DELETE"] @@ -21,7 +21,7 @@ class TestURLSession: LoopbackServerTest { XCTAssertEqual(d.capital, "Kathmandu", "test_dataTaskWithURLRequest returned an unexpected result") } } - + func test_dataTaskWithURLCompletionHandler() { //shared session dataTaskWithURLCompletionHandler(with: URLSession.shared) @@ -255,7 +255,7 @@ class TestURLSession: LoopbackServerTest { XCTAssert(task.isEqual(task.copy())) } - // This test is buggy because the server could respond before the task is cancelled. + // This test is buggy becuase the server could respond before the task is cancelled. func test_cancelTask() { let urlString = "http://127.0.0.1:\(TestURLSession.serverPort)/Peru" var urlRequest = URLRequest(url: URL(string: urlString)!) @@ -391,205 +391,7 @@ class TestURLSession: LoopbackServerTest { waitForExpectations(timeout: 30) } - - func test_httpRedirectionWithCode300() throws { - let statusCode = 300 - for method in httpMethods { - let testMethod = "\(method) request with statusCode \(statusCode)" - let urlString = "http://127.0.0.1:\(TestURLSession.serverPort)/\(statusCode)?location=jsonBody" - let url = try XCTUnwrap(URL(string: urlString), "Cant create URL for \(testMethod)") - var request = URLRequest(url: url) - request.httpMethod = method - let d = HTTPRedirectionDataTask(with: expectation(description: "\(method) \(urlString): with HTTP redirection")) - d.run(with: request) - - waitForExpectations(timeout: 12) - XCTAssertNil(d.error) - - XCTAssertNil(d.redirectionResponse) - XCTAssertNotNil(d.response) - let httpresponse = d.response as? HTTPURLResponse - XCTAssertEqual(httpresponse?.statusCode, statusCode, "HTTP final response code is invalid for \(testMethod)") - - let callbackMsg = "Bad callback for \(testMethod)" - switch method { - case "HEAD": - XCTAssertEqual(d.callbackCount, 2, "Callback count for \(testMethod)") - XCTAssertEqual(d.callback(0), "urlSession(_:dataTask:didReceive:completionHandler:)", callbackMsg) - XCTAssertEqual(d.callback(1), "urlSession(_:task:didCompleteWithError:)", callbackMsg) - XCTAssertEqual(d.receivedData.count, 0) // No body for HEAD requests - - default: - XCTAssertEqual(d.callbackCount, 3, "Callback count for \(testMethod)") - XCTAssertEqual(d.callback(0), "urlSession(_:dataTask:didReceive:completionHandler:)", callbackMsg) - XCTAssertEqual(d.callback(1), "urlSession(_:dataTask:didReceive:)", callbackMsg) - XCTAssertEqual(d.callback(2), "urlSession(_:task:didCompleteWithError:)", callbackMsg) - - if let body = String(data: d.receivedData, encoding: .utf8) { - XCTAssertEqual(body, "Redirecting to \(method) jsonBody", "URI mismatch for \(testMethod)") - } else { - XCTFail("No JSON body for \(testMethod)") - } - } - } - } - - func test_httpRedirectionWithCode301_302() throws { - for statusCode in 301...302 { - for method in httpMethods { - let testMethod = "\(method) request with statusCode \(statusCode)" - let urlString = "http://127.0.0.1:\(TestURLSession.serverPort)/\(statusCode)?location=jsonBody" - let url = try XCTUnwrap(URL(string: urlString), "Cant create URL for \(testMethod)") - var request = URLRequest(url: url) - request.httpMethod = method - let d = HTTPRedirectionDataTask(with: expectation(description: "\(method) \(urlString): with HTTP redirection")) - d.run(with: request) - - waitForExpectations(timeout: 12) - XCTAssertNil(d.error) - - XCTAssertNotNil(d.response) - let httpresponse = d.response as? HTTPURLResponse - XCTAssertEqual(httpresponse?.statusCode, 200, "HTTP final response code is invalid for \(testMethod)") - XCTAssertEqual(d.redirectionResponse?.statusCode, statusCode, "HTTP redirection response code is invalid for \(testMethod)") - - let callbackMsg = "Bad callback for \(testMethod)" - switch method { - case "HEAD": - XCTAssertEqual(d.callbackCount, 3, "Callback count for \(testMethod)") - XCTAssertEqual(d.callback(0), "urlSession(_:task:willPerformHTTPRedirection:newRequest:completionHandler:)", callbackMsg) - XCTAssertEqual(d.callback(1), "urlSession(_:dataTask:didReceive:completionHandler:)", callbackMsg) - XCTAssertEqual(d.callback(2), "urlSession(_:task:didCompleteWithError:)", callbackMsg) - XCTAssertEqual(d.receivedData.count, 0) // No body for HEAD requests - - - default: - XCTAssertEqual(d.callbackCount, 4, "Callback count for \(testMethod)") - XCTAssertEqual(d.callback(0), "urlSession(_:task:willPerformHTTPRedirection:newRequest:completionHandler:)", callbackMsg) - XCTAssertEqual(d.callback(1), "urlSession(_:dataTask:didReceive:completionHandler:)", callbackMsg) - XCTAssertEqual(d.callback(2), "urlSession(_:dataTask:didReceive:)", callbackMsg) - XCTAssertEqual(d.callback(3), "urlSession(_:task:didCompleteWithError:)", callbackMsg) - - if let jsonBody = try? JSONSerialization.jsonObject(with: d.receivedData, options: []) as? [String: String] { - let uri = (method == "POST" ? "GET" : method) + " /jsonBody HTTP/1.1" - XCTAssertEqual(jsonBody["uri"], uri, "URI mismatch for \(testMethod)") - } else { - XCTFail("No JSON body for \(testMethod)") - } - } - } - } - } - - func test_httpRedirectionWithCode303() throws { - let statusCode = 303 - for method in httpMethods { - let testMethod = "\(method) request with statusCode \(statusCode)" - let urlString = "http://127.0.0.1:\(TestURLSession.serverPort)/\(statusCode)?location=jsonBody" - let url = try XCTUnwrap(URL(string: urlString), "Cant create URL for \(testMethod)") - var request = URLRequest(url: url) - request.httpMethod = method - let d = HTTPRedirectionDataTask(with: expectation(description: "\(method) \(urlString): with HTTP redirection")) - d.run(with: request) - - waitForExpectations(timeout: 12) - XCTAssertNil(d.error) - - XCTAssertNotNil(d.response) - let httpresponse = d.response as? HTTPURLResponse - XCTAssertEqual(httpresponse?.statusCode, 200, "HTTP final response code is invalid for \(testMethod)") - XCTAssertEqual(d.redirectionResponse?.statusCode, statusCode, "HTTP redirection response code is invalid for \(testMethod)") - - let callbackMsg = "Bad callback for \(testMethod)" - XCTAssertEqual(d.callbackCount, 4, "Callback count for \(testMethod)") - XCTAssertEqual(d.callback(0), "urlSession(_:task:willPerformHTTPRedirection:newRequest:completionHandler:)", callbackMsg) - XCTAssertEqual(d.callback(1), "urlSession(_:dataTask:didReceive:completionHandler:)", callbackMsg) - XCTAssertEqual(d.callback(2), "urlSession(_:dataTask:didReceive:)", callbackMsg) - XCTAssertEqual(d.callback(3), "urlSession(_:task:didCompleteWithError:)", callbackMsg) - if let jsonBody = try? JSONSerialization.jsonObject(with: d.receivedData, options: []) as? [String: String] { - let uri = "GET /jsonBody HTTP/1.1" - XCTAssertEqual(jsonBody["uri"], uri, "URI mismatch for \(testMethod)") - } else { - XCTFail("No jsonBody for \(testMethod)") - } - } - } - - func test_httpRedirectionWithCode304() throws { - let statusCode = 304 - for method in httpMethods { - let testMethod = "\(method) request with statusCode \(statusCode)" - let urlString = "http://127.0.0.1:\(TestURLSession.serverPort)/\(statusCode)?location=jsonBody" - let url = try XCTUnwrap(URL(string: urlString), "Cant create URL for \(testMethod)") - var request = URLRequest(url: url) - request.httpMethod = method - let d = HTTPRedirectionDataTask(with: expectation(description: "\(method) \(urlString): with HTTP redirection")) - d.run(with: request) - - waitForExpectations(timeout: 12) - XCTAssertNil(d.error) - - XCTAssertNotNil(d.response) - let httpresponse = d.response as? HTTPURLResponse - XCTAssertEqual(httpresponse?.statusCode, statusCode, "HTTP final response code is invalid for \(testMethod)") - XCTAssertNil(d.redirectionResponse) - - let callbackMsg = "Bad callback for \(testMethod)" - XCTAssertEqual(d.callbackCount, 2, "Callback count for \(testMethod)") - XCTAssertEqual(d.callback(0), "urlSession(_:dataTask:didReceive:completionHandler:)", callbackMsg) - XCTAssertEqual(d.callback(1), "urlSession(_:task:didCompleteWithError:)", callbackMsg) - - XCTAssertEqual(d.receivedData.count, 0) - let jsonBody = try? JSONSerialization.jsonObject(with: d.receivedData, options: []) as? [String: String] - XCTAssertNil(jsonBody) - } - } - - func test_httpRedirectionWithCode305_308() throws { - for statusCode in 305...308 { - for method in httpMethods { - let testMethod = "\(method) request with statusCode \(statusCode)" - let urlString = "http://127.0.0.1:\(TestURLSession.serverPort)/\(statusCode)?location=jsonBody" - let url = try XCTUnwrap(URL(string: urlString), "Cant create URL for \(testMethod)") - var request = URLRequest(url: url) - request.httpMethod = method - let d = HTTPRedirectionDataTask(with: expectation(description: "\(method) \(urlString): with HTTP redirection")) - d.run(with: request) - - waitForExpectations(timeout: 12) - XCTAssertNil(d.error) - - XCTAssertNotNil(d.response) - let httpresponse = d.response as? HTTPURLResponse - XCTAssertEqual(httpresponse?.statusCode, 200, "HTTP final response code is invalid for \(testMethod)") - XCTAssertEqual(d.redirectionResponse?.statusCode, statusCode, "HTTP redirection response code is invalid for \(testMethod)") - - let callbackMsg = "Bad callback for \(testMethod)" - switch method { - case "HEAD": - XCTAssertEqual(d.callbackCount, 3, "Callback count for \(testMethod)") - XCTAssertEqual(d.callback(0), "urlSession(_:task:willPerformHTTPRedirection:newRequest:completionHandler:)", callbackMsg) - XCTAssertEqual(d.callback(1), "urlSession(_:dataTask:didReceive:completionHandler:)", callbackMsg) - XCTAssertEqual(d.callback(2), "urlSession(_:task:didCompleteWithError:)", callbackMsg) - XCTAssertEqual(d.receivedData.count, 0) // No body for HEAD requests - - default: - XCTAssertEqual(d.callbackCount, 4, "Callback count for \(testMethod)") - XCTAssertEqual(d.callback(0), "urlSession(_:task:willPerformHTTPRedirection:newRequest:completionHandler:)", callbackMsg) - XCTAssertEqual(d.callback(1), "urlSession(_:dataTask:didReceive:completionHandler:)", callbackMsg) - XCTAssertEqual(d.callback(2), "urlSession(_:dataTask:didReceive:)", callbackMsg) - XCTAssertEqual(d.callback(3), "urlSession(_:task:didCompleteWithError:)", callbackMsg) - if let jsonBody = try? JSONSerialization.jsonObject(with: d.receivedData, options: []) as? [String: String] { - let uri = "\(method) /jsonBody HTTP/1.1" - XCTAssertEqual(jsonBody["uri"], uri, "URI mismatch for \(testMethod)") - } else { - XCTFail("No JSON body for \(testMethod)") - } - } - } - } - } - + func test_httpRedirectionWithCompleteRelativePath() { let urlString = "http://127.0.0.1:\(TestURLSession.serverPort)/UnitedStates" let url = URL(string: urlString)! @@ -1436,11 +1238,6 @@ class TestURLSession: LoopbackServerTest { ("test_verifyRequestHeaders", test_verifyRequestHeaders), ("test_verifyHttpAdditionalHeaders", test_verifyHttpAdditionalHeaders), ("test_timeoutInterval", test_timeoutInterval), - ("test_httpRedirectionWithCode300", test_httpRedirectionWithCode300), - ("test_httpRedirectionWithCode301_302", test_httpRedirectionWithCode301_302), - ("test_httpRedirectionWithCode303", test_httpRedirectionWithCode303), - ("test_httpRedirectionWithCode304", test_httpRedirectionWithCode304), - ("test_httpRedirectionWithCode305_308", test_httpRedirectionWithCode305_308), ("test_httpRedirectionWithCompleteRelativePath", test_httpRedirectionWithCompleteRelativePath), ("test_httpRedirectionWithInCompleteRelativePath", test_httpRedirectionWithInCompleteRelativePath), ("test_httpRedirectionWithDefaultPort", test_httpRedirectionWithDefaultPort), @@ -1766,19 +1563,15 @@ class FailFastProtocol: URLProtocol { // Intentionally blank } } -class HTTPRedirectionDataTask: NSObject { +class HTTPRedirectionDataTask : NSObject { let dataTaskExpectation: XCTestExpectation! var session: URLSession! = nil var task: URLSessionDataTask! = nil var cancelExpectation: XCTestExpectation? - private(set) var receivedData = Data() - private(set) var error: Error? - private(set) var response: URLResponse? - private(set) var redirectionResponse: HTTPURLResponse? - private var callbacks: [String] = [] - - - init(with expectation: XCTestExpectation, statusCode: Int = 200) { + + public var error = false + + init(with expectation: XCTestExpectation) { dataTaskExpectation = expectation } @@ -1792,7 +1585,7 @@ class HTTPRedirectionDataTask: NSObject { func run(with url: URL) { let config = URLSessionConfiguration.default - config.timeoutIntervalForRequest = 4 + config.timeoutIntervalForRequest = 8 session = URLSession(configuration: config, delegate: self, delegateQueue: nil) task = session.dataTask(with: url) task.resume() @@ -1801,46 +1594,29 @@ class HTTPRedirectionDataTask: NSObject { func cancel() { task.cancel() } - - var callbackCount: Int { callbacks.count } - - func callback(_ idx: Int) -> String? { - guard idx < callbacks.count else { return nil } - return callbacks[idx] - } } -extension HTTPRedirectionDataTask: URLSessionDataDelegate { - - public func urlSession(_ session: URLSession, dataTask: URLSessionDataTask, didReceive data: Data) { - if callbacks.last != #function { - callbacks.append(#function) - } - receivedData.append(data) - } - +extension HTTPRedirectionDataTask : URLSessionDataDelegate { public func urlSession(_ session: URLSession, dataTask: URLSessionDataTask, didReceive response: URLResponse, completionHandler: @escaping (URLSession.ResponseDisposition) -> Void) { - callbacks.append(#function) - - self.response = response - completionHandler(.allow) + guard let httpresponse = response as? HTTPURLResponse else { fatalError() } + XCTAssertNotNil(response) + XCTAssertEqual(200, httpresponse.statusCode, "HTTP response code is not 200") } } -extension HTTPRedirectionDataTask: URLSessionTaskDelegate { +extension HTTPRedirectionDataTask : URLSessionTaskDelegate { public func urlSession(_ session: URLSession, task: URLSessionTask, didCompleteWithError error: Error?) { - callbacks.append(#function) dataTaskExpectation.fulfill() - + guard (error as? URLError) != nil else { return } if let cancellation = cancelExpectation { cancellation.fulfill() } - self.error = error + self.error = true } public func urlSession(_ session: URLSession, task: URLSessionTask, willPerformHTTPRedirection response: HTTPURLResponse, newRequest request: URLRequest, completionHandler: @escaping (URLRequest?) -> Void) { - callbacks.append(#function) - redirectionResponse = response + XCTAssertNotNil(response) + XCTAssertEqual(302, response.statusCode, "HTTP response code is not 302") if let url = response.url, url.path.hasSuffix("/redirect-with-default-port") { XCTAssertEqual(request.url?.absoluteString, "http://127.0.0.1/redirected-with-default-port") // Don't follow the redirect as the test server is not running on port 80