Skip to content

URLSession: Add a limit on the number of HTTP Redirects that are followed #2756

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 4, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 15 additions & 3 deletions Sources/FoundationNetworking/URLSession/HTTP/HTTPURLProtocol.swift
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ internal class _HTTPURLProtocol: _NativeProtocol {
// and the 3xx response is returned to the client and the data is sent via the delegate notify
// mechanism. `lastRedirectBody` holds the body of the redirect currently being processed.
var lastRedirectBody: Data? = nil
private var redirectCount = 0

public required init(task: URLSessionTask, cachedResponse: CachedURLResponse?, client: URLProtocolClient?) {
super.init(task: task, cachedResponse: cachedResponse, client: client)
Expand Down Expand Up @@ -440,13 +441,24 @@ internal class _HTTPURLProtocol: _NativeProtocol {
}

override func redirectFor(request: URLRequest) {
//TODO: Should keep track of the number of redirects that this
// request has gone through and err out once it's too large, i.e.
// call into `failWith(errorCode: )` with NSURLErrorHTTPTooManyRedirects
guard case .transferCompleted(response: let response, bodyDataDrain: let bodyDataDrain) = self.internalState else {
fatalError("Trying to redirect, but the transfer is not complete.")
}

// Avoid a never ending redirect chain by having a hard limit on the number of redirects.
// This value mirrors Darwin.
redirectCount += 1
if redirectCount > 16 {
self.internalState = .transferFailed
let error = NSError(domain: NSURLErrorDomain, code: NSURLErrorHTTPTooManyRedirects,
userInfo: [NSLocalizedDescriptionKey: "too many HTTP redirects"])
guard let request = task?.currentRequest else {
fatalError("In a redirect chain but no current task/request")
}
failWith(error: error, request: request)
return
}

guard let session = task?.session as? URLSession else { fatalError() }
switch session.behaviour(for: task!) {
case .taskDelegate(let delegate):
Expand Down
42 changes: 42 additions & 0 deletions Tests/Foundation/Tests/TestURLSession.swift
Original file line number Diff line number Diff line change
Expand Up @@ -798,6 +798,47 @@ class TestURLSession: LoopbackServerTest {
}
}

func test_httpRedirectionExceededMaxRedirects() throws {
let expectedMaxRedirects = 16
let urlString = "http://127.0.0.1:\(TestURLSession.serverPort)/redirect/99"
let url = try XCTUnwrap(URL(string: urlString))
let exceededCountUrlString = "http://127.0.0.1:\(TestURLSession.serverPort)/redirect/\(99 - expectedMaxRedirects)"
let exceededCountUrl = try XCTUnwrap(URL(string: exceededCountUrlString))

for method in httpMethods {
var request = URLRequest(url: url)
request.httpMethod = method
let delegate = SessionDelegate(with: expectation(description: "\(method) \(urlString): with HTTP redirection"))

var redirectRequests: [(HTTPURLResponse, URLRequest)] = []
delegate.redirectionHandler = { (response: HTTPURLResponse, request: URLRequest, completionHandler: @escaping (URLRequest?) -> Void) in
redirectRequests.append((response, request))
completionHandler(request)
}
delegate.run(with: request, timeoutInterval: 5)
waitForExpectations(timeout: 20)

XCTAssertNil(delegate.response)
XCTAssertNil(delegate.receivedData)

XCTAssertNotNil(delegate.error)
let error = delegate.error as? URLError
XCTAssertEqual(error?.code.rawValue, NSURLErrorHTTPTooManyRedirects)
XCTAssertEqual(error?.localizedDescription, "too many HTTP redirects")
let userInfo = error?.userInfo as? [String: Any]
let errorURL = userInfo?[NSURLErrorFailingURLErrorKey] as? URL
XCTAssertEqual(errorURL, exceededCountUrl)

// Check the last Redirection response/request received.
XCTAssertEqual(redirectRequests.count, expectedMaxRedirects)
let lastResponse = redirectRequests.last?.0
let lastRequest = redirectRequests.last?.1

XCTAssertEqual(lastResponse?.statusCode, 302)
XCTAssertEqual(lastRequest?.url, exceededCountUrl)
}
}

func test_httpNotFound() throws {
let urlString = "http://127.0.0.1:\(TestURLSession.serverPort)/404"
let url = try XCTUnwrap(URL(string: urlString))
Expand Down Expand Up @@ -1691,6 +1732,7 @@ class TestURLSession: LoopbackServerTest {
("test_httpRedirectionWithDefaultPort", test_httpRedirectionWithDefaultPort),
("test_httpRedirectionTimeout", test_httpRedirectionTimeout),
("test_httpRedirectionChainInheritsTimeoutInterval", test_httpRedirectionChainInheritsTimeoutInterval),
("test_httpRedirectionExceededMaxRedirects", test_httpRedirectionExceededMaxRedirects),
("test_httpNotFound", test_httpNotFound),
("test_http0_9SimpleResponses", test_http0_9SimpleResponses),
("test_outOfRangeButCorrectlyFormattedHTTPCode", test_outOfRangeButCorrectlyFormattedHTTPCode),
Expand Down