Various Cookie fixes (#1706)

maksimorlovich · parkera · commit 753e621b5f6c · 2018-10-02T04:21:18.000+07:00
* Various Cookie fixes - Add support for additional Set-Cookie formats that web servers can return - Properly handle HTTP header parsing to extract values since values can contain colons - Make sure to set cookies on redirect requests - Use setValue instead of addValue when applying cookies to requests otherwise, Cookie header might contain: cookie1=value1,cookie1=value1; cookie2=value2 - New unit tests for cookie formats and redirect with Set-Cookie (cherry picked from commit 97a93b5) * Remove two-digit year cookie format support & unit test (fails on Ubuntu 14.04) (#1707)
diff --git a/Foundation/HTTPCookie.swift b/Foundation/HTTPCookie.swift
@@ -93,6 +93,38 @@ open class HTTPCookie : NSObject {
     let _version: Int
     var _properties: [HTTPCookiePropertyKey : Any]
 
+    // See: https://tools.ietf.org/html/rfc2616#section-3.3.1
+
+    // Sun, 06 Nov 1994 08:49:37 GMT  ; RFC 822, updated by RFC 1123
+    static let _formatter1: DateFormatter = {
+        let formatter = DateFormatter()
+        formatter.locale = Locale(identifier: "en_US_POSIX")
+        formatter.dateFormat = "EEE, dd MMM yyyy HH:mm:ss O"
+        formatter.timeZone = TimeZone(abbreviation: "GMT")
+        return formatter
+    }()
+
+    // Sun Nov  6 08:49:37 1994       ; ANSI C's asctime() format
+    static let _formatter2: DateFormatter = {
+        let formatter = DateFormatter()
+        formatter.locale = Locale(identifier: "en_US_POSIX")
+        formatter.dateFormat = "EEE MMM d HH:mm:ss yyyy"
+        formatter.timeZone = TimeZone(abbreviation: "GMT")
+        return formatter
+    }()
+
+    // Sun, 06-Nov-1994 08:49:37 GMT  ; Tomcat servers sometimes return cookies in this format
+    static let _formatter3: DateFormatter = {
+        let formatter = DateFormatter()
+        formatter.locale = Locale(identifier: "en_US_POSIX")
+        formatter.dateFormat = "EEE, dd-MMM-yyyy HH:mm:ss O"
+        formatter.timeZone = TimeZone(abbreviation: "GMT")
+        return formatter
+    }()
+
+    static let _allFormatters: [DateFormatter]
+        = [_formatter1, _formatter2, _formatter3]
+
     static let _attributes: [HTTPCookiePropertyKey]
         = [.name, .value, .originURL, .version, .domain,
            .path, .secure, .expires, .comment, .commentURL,
@@ -292,12 +324,8 @@ open class HTTPCookie : NSObject {
             if let date = expiresProperty as? Date {
                 expDate = date
             } else if let dateString = expiresProperty as? String {
-                let formatter = DateFormatter()
-                formatter.locale = Locale(identifier: "en_US_POSIX")
-                formatter.dateFormat = "EEE, dd MMM yyyy HH:mm:ss O"   // per RFC 6265 '<rfc1123-date, defined in [RFC2616], Section 3.3.1>'
-                let timeZone = TimeZone(abbreviation: "GMT")
-                formatter.timeZone = timeZone
-                expDate = formatter.date(from: dateString)
+                let results = HTTPCookie._allFormatters.compactMap { $0.date(from: dateString) }
+                expDate = results.first
             }
         }
         _expiresDate = expDate
@@ -418,7 +446,7 @@ open class HTTPCookie : NSObject {
             let name = pair.components(separatedBy: "=")[0]
             var value = pair.components(separatedBy: "\(name)=")[1]  //a value can have an "="
             if canonicalize(name) == .expires {
-                value = value.insertComma(at: 3)    //re-insert the comma
+                value = value.unmaskCommas()    //re-insert the comma
             }
             properties[canonicalize(name)] = value
         }
@@ -439,7 +467,7 @@ open class HTTPCookie : NSObject {
     //we pass this to a map()
     private class func removeCommaFromDate(_ value: String) -> String {
         if value.hasPrefix("Expires") || value.hasPrefix("expires")  {
-            return value.removeCommas()
+            return value.maskCommas()
         }
         return value
     }
@@ -623,12 +651,12 @@ fileprivate extension String {
         return self.trimmingCharacters(in: NSCharacterSet.whitespacesAndNewlines)
     }
 
-    func removeCommas() -> String {
-        return self.replacingOccurrences(of: ",", with: "")
+    func maskCommas() -> String {
+        return self.replacingOccurrences(of: ",", with: "&comma")
     }
 
-    func insertComma(at index:Int) -> String {
-        return  String(self.prefix(index)) + ","  + String(self.suffix(self.count-index))
+    func unmaskCommas() -> String {
+        return self.replacingOccurrences(of: "&comma", with: ",")
     }
 }
 
diff --git a/Foundation/URLSession/Configuration.swift b/Foundation/URLSession/Configuration.swift
@@ -115,7 +115,7 @@ internal extension URLSession._Configuration {
             if let cookieStorage = self.httpCookieStorage, let url = request.url, let cookies = cookieStorage.cookies(for: url) {
                 let cookiesHeaderFields =  HTTPCookie.requestHeaderFields(with: cookies)
                 if let cookieValue = cookiesHeaderFields["Cookie"], cookieValue != "" {
-                    request.addValue(cookieValue, forHTTPHeaderField: "Cookie")
+                    request.setValue(cookieValue, forHTTPHeaderField: "Cookie")
                 }
             }
         }
diff --git a/Foundation/URLSession/http/HTTPURLProtocol.swift b/Foundation/URLSession/http/HTTPURLProtocol.swift
@@ -120,7 +120,9 @@ internal class _HTTPURLProtocol: _NativeProtocol {
             httpHeaders = hh
         }
 
-        if let hh = self.task?.originalRequest?.allHTTPHeaderFields {
+        // In case this is a redirect, take the headers from the current (redirect) request.
+        if let hh = self.task?.currentRequest?.allHTTPHeaderFields ??
+                    self.task?.originalRequest?.allHTTPHeaderFields {
             if httpHeaders == nil {
                 httpHeaders = hh
             } else {
@@ -211,8 +213,9 @@ internal class _HTTPURLProtocol: _NativeProtocol {
                 }
             }
         case .noDelegate, .dataCompletionHandler, .downloadCompletionHandler:
-            // Follow the redirect.
-            startNewTransfer(with: request)
+            // Follow the redirect. Need to configure new request with cookies, etc.
+            let configuredRequest = session._configuration.configure(request: request)
+            startNewTransfer(with: configuredRequest)
         }
     }
 
diff --git a/Foundation/URLSession/libcurl/EasyHandle.swift b/Foundation/URLSession/libcurl/EasyHandle.swift
@@ -540,9 +540,10 @@ fileprivate extension _EasyHandle {
     fileprivate func setCookies(headerData data: Data) {
         guard let config = _config, config.httpCookieAcceptPolicy !=  HTTPCookie.AcceptPolicy.never else { return }
         guard let headerData = String(data: data, encoding: String.Encoding.utf8) else { return }
-        //Convert headerData from a string to a dictionary.
-        //Ignore headers like 'HTTP/1.1 200 OK\r\n' which do not have a key value pair.
-        let headerComponents = headerData.split { $0 == ":" }
+        // Convert headerData from a string to a dictionary.
+        // Ignore headers like 'HTTP/1.1 200 OK\r\n' which do not have a key value pair.
+        // Value can have colons (ie, date), so only split at the first one, ie header:value
+        let headerComponents = headerData.split(separator: ":", maxSplits: 1)
         var headers: [String: String] = [:]
         //Trim the leading and trailing whitespaces (if any) before adding the header information to the dictionary.
         if headerComponents.count > 1 {
diff --git a/TestFoundation/HTTPServer.swift b/TestFoundation/HTTPServer.swift
@@ -435,6 +435,10 @@ public class TestURLSessionServer {
             let text = request.getCommaSeparatedHeaders()
             return _HTTPResponse(response: .OK, headers: "Content-Length: \(text.data(using: .utf8)!.count)", body: text)
         }
+        
+        if uri == "/redirectSetCookies" {
+            return _HTTPResponse(response: .REDIRECT, headers: "Location: /setCookies\r\nSet-Cookie: redirect=true; Max-Age=7776000; path=/", body: "")
+        }
 
         if uri == "/UnitedStates" {
             let value = capitals[String(uri.dropFirst())]!
diff --git a/TestFoundation/TestHTTPCookie.swift b/TestFoundation/TestHTTPCookie.swift
@@ -17,7 +17,8 @@ class TestHTTPCookie: XCTestCase {
             ("test_cookiesWithResponseHeader0cookies", test_cookiesWithResponseHeader0cookies),
             ("test_cookiesWithResponseHeader2cookies", test_cookiesWithResponseHeader2cookies),
             ("test_cookiesWithResponseHeaderNoDomain", test_cookiesWithResponseHeaderNoDomain),
-            ("test_cookiesWithResponseHeaderNoPathNoDomain", test_cookiesWithResponseHeaderNoPathNoDomain)
+            ("test_cookiesWithResponseHeaderNoPathNoDomain", test_cookiesWithResponseHeaderNoPathNoDomain),
+            ("test_cookieExpiresDateFormats", test_cookieExpiresDateFormats),
         ]
     }
 
@@ -168,4 +169,26 @@ class TestHTTPCookie: XCTestCase {
         XCTAssertEqual(cookies[0].domain, "example.com")
         XCTAssertEqual(cookies[0].path, "/")
     }
+    
+    func test_cookieExpiresDateFormats() {
+        let testDate = Date(timeIntervalSince1970: 1577881800)
+        let cookieString =
+            """
+            format1=true; expires=Wed, 01 Jan 2020 12:30:00 GMT; path=/; domain=swift.org; secure; httponly,
+            format2=true; expires=Wed Jan 1 12:30:00 2020; path=/; domain=swift.org; secure; httponly,
+            format3=true; expires=Wed, 01-Jan-2020 12:30:00 GMT; path=/; domain=swift.org; secure; httponly
+            """
+
+        let header = ["header1":"value1",
+                      "Set-Cookie": cookieString,
+                      "header2":"value2",
+                      "header3":"value3"]
+        let cookies = HTTPCookie.cookies(withResponseHeaderFields: header, for: URL(string: "https://swift.org")!)
+        XCTAssertEqual(cookies.count, 3)
+        cookies.forEach { cookie in
+            XCTAssertEqual(cookie.expiresDate, testDate)
+            XCTAssertEqual(cookie.domain, "swift.org")
+            XCTAssertEqual(cookie.path, "/")
+        }
+    }
 }
diff --git a/TestFoundation/TestURLSession.swift b/TestFoundation/TestURLSession.swift
@@ -42,6 +42,7 @@ class TestURLSession : LoopbackServerTest {
             ("test_cookiesStorage", test_cookiesStorage),
             ("test_setCookies", test_setCookies),
             ("test_dontSetCookies", test_dontSetCookies),
+            ("test_redirectionWithSetCookies", test_redirectionWithSetCookies),
         ]
     }
     
@@ -575,6 +576,31 @@ class TestURLSession : LoopbackServerTest {
         XCTAssertEqual(cookies?.count, 1)
     }
 
+    func test_redirectionWithSetCookies() {
+        let config = URLSessionConfiguration.default
+        config.timeoutIntervalForRequest = 5
+        if let storage = config.httpCookieStorage, let cookies = storage.cookies {
+            for cookie in cookies {
+                storage.deleteCookie(cookie)
+            }
+        }
+        let urlString = "http://127.0.0.1:\(TestURLSession.serverPort)/redirectSetCookies"
+        let session = URLSession(configuration: config, delegate: nil, delegateQueue: nil)
+        var expect = expectation(description: "POST \(urlString)")
+        var req = URLRequest(url: URL(string: urlString)!)
+        var task = session.dataTask(with: req) { (data, _, error) -> Void in
+            defer { expect.fulfill() }
+            XCTAssertNotNil(data)
+            XCTAssertNil(error as? URLError, "error = \(error as! URLError)")
+            guard let data = data else { return }
+            let headers = String(data: data, encoding: String.Encoding.utf8) ?? ""
+            print("headers here = \(headers)")
+            XCTAssertNotNil(headers.range(of: "Cookie: redirect=true"))
+        }
+        task.resume()
+        waitForExpectations(timeout: 30)
+    }
+
     func test_setCookies() {
         let config = URLSessionConfiguration.default
         config.timeoutIntervalForRequest = 5

Original file line number	Diff line number	Diff line change
`@@ -115,7 +115,7 @@ internal extension URLSession._Configuration {`
`115`	`115`	`if let cookieStorage = self.httpCookieStorage, let url = request.url, let cookies = cookieStorage.cookies(for: url) {`
`116`	`116`	`let cookiesHeaderFields = HTTPCookie.requestHeaderFields(with: cookies)`
`117`	`117`	`if let cookieValue = cookiesHeaderFields["Cookie"], cookieValue != "" {`
`118`		`- request.addValue(cookieValue, forHTTPHeaderField: "Cookie")`
	`118`	`+ request.setValue(cookieValue, forHTTPHeaderField: "Cookie")`
`119`	`119`	`}`
`120`	`120`	`}`
`121`	`121`	`}`
Original file line number	Diff line number	Diff line change
`@@ -120,7 +120,9 @@ internal class _HTTPURLProtocol: _NativeProtocol {`
`120`	`120`	`httpHeaders = hh`
`121`	`121`	`}`
`122`	`122`
`123`		`- if let hh = self.task?.originalRequest?.allHTTPHeaderFields {`
	`123`	`+ // In case this is a redirect, take the headers from the current (redirect) request.`
	`124`	`+ if let hh = self.task?.currentRequest?.allHTTPHeaderFields ??`
	`125`	`+ self.task?.originalRequest?.allHTTPHeaderFields {`
`124`	`126`	`if httpHeaders == nil {`
`125`	`127`	`httpHeaders = hh`
`126`	`128`	`} else {`
`@@ -211,8 +213,9 @@ internal class _HTTPURLProtocol: _NativeProtocol {`
`211`	`213`	`}`
`212`	`214`	`}`
`213`	`215`	`case .noDelegate, .dataCompletionHandler, .downloadCompletionHandler:`
`214`		`- // Follow the redirect.`
`215`		`- startNewTransfer(with: request)`
	`216`	`+ // Follow the redirect. Need to configure new request with cookies, etc.`
	`217`	`+ let configuredRequest = session._configuration.configure(request: request)`
	`218`	`+ startNewTransfer(with: configuredRequest)`
`216`	`219`	`}`
`217`	`220`	`}`
`218`	`221`