Redo HTTP cookie parsing (#510)

* Redo HTTP cookie parsing using strptime

* Make String(utf8Slice:from:) less ugly

* Adjust cookie component parsing to better match RFC-6562

Author: karwa

Diff for: Package.swift

@@ -29,9 +29,11 @@ let package = Package(
         .package(url: "https://github.com/apple/swift-log.git", from: "1.4.0"),
     ],
     targets: [
+        .target(name: "CAsyncHTTPClient"),
         .target(
             name: "AsyncHTTPClient",
             dependencies: [
+                .target(name: "CAsyncHTTPClient"),
                 .product(name: "NIO", package: "swift-nio"),
                 .product(name: "NIOCore", package: "swift-nio"),
                 .product(name: "NIOPosix", package: "swift-nio"),

Diff for: Sources/AsyncHTTPClient/FoundationExtensions.swift

@@ -0,0 +1,56 @@
+//===----------------------------------------------------------------------===//
+//
+// This source file is part of the AsyncHTTPClient open source project
+//
+// Copyright (c) 2018-2021 Apple Inc. and the AsyncHTTPClient project authors
+// Licensed under Apache License v2.0
+//
+// See LICENSE.txt for license information
+// See CONTRIBUTORS.txt for the list of AsyncHTTPClient project authors
+//
+// SPDX-License-Identifier: Apache-2.0
+//
+//===----------------------------------------------------------------------===//
+
+// Extensions which provide better ergonomics when using Foundation types,
+// or by using Foundation APIs.
+
+import Foundation
+
+extension HTTPClient.Cookie {
+    /// The cookie's expiration date.
+    public var expires: Date? {
+        get {
+            expires_timestamp.map { Date(timeIntervalSince1970: TimeInterval($0)) }
+        }
+        set {
+            expires_timestamp = newValue.map { Int64($0.timeIntervalSince1970) }
+        }
+    }
+
+    /// Create HTTP cookie.
+    ///
+    /// - parameters:
+    ///     - name: The name of the cookie.
+    ///     - value: The cookie's string value.
+    ///     - path: The cookie's path.
+    ///     - domain: The domain of the cookie, defaults to nil.
+    ///     - expires: The cookie's expiration date, defaults to nil.
+    ///     - maxAge: The cookie's age in seconds, defaults to nil.
+    ///     - httpOnly: Whether this cookie should be used by HTTP servers only, defaults to false.
+    ///     - secure: Whether this cookie should only be sent using secure channels, defaults to false.
+    public init(name: String, value: String, path: String = "/", domain: String? = nil, expires: Date? = nil, maxAge: Int? = nil, httpOnly: Bool = false, secure: Bool = false) {
+        // FIXME: This should be failable and validate the inputs
+        // (for example, checking that the strings are ASCII, path begins with "/", domain is not empty, etc).
+        self.init(
+            name: name,
+            value: value,
+            path: path,
+            domain: domain,
+            expires_timestamp: expires.map { Int64($0.timeIntervalSince1970) },
+            maxAge: maxAge,
+            httpOnly: httpOnly,
+            secure: secure
+        )
+    }
+}

Diff for: Sources/AsyncHTTPClient/HTTPClient+HTTPCookie.swift

@@ -12,8 +12,13 @@
 //
 //===----------------------------------------------------------------------===//
 
-import Foundation
 import NIOHTTP1
+#if canImport(Darwin)
+import Darwin
+#elseif canImport(Glibc)
+import Glibc
+#endif
+import CAsyncHTTPClient
 
 extension HTTPClient {
     /// A representation of an HTTP cookie.
@@ -26,8 +31,8 @@ extension HTTPClient {
         public var path: String
         /// The domain of the cookie.
         public var domain: String?
-        /// The cookie's expiration date.
-        public var expires: Date?
+        /// The cookie's expiration date, as a number of seconds since the Unix epoch.
+        var expires_timestamp: Int64?
         /// The cookie's age in seconds.
         public var maxAge: Int?
         /// Whether the cookie should only be sent to HTTP servers.
@@ -42,79 +47,72 @@ extension HTTPClient {
         ///     - defaultDomain: Default domain to use if cookie was sent without one.
         /// - returns: nil if the header is invalid.
         public init?(header: String, defaultDomain: String) {
-            let components = header.components(separatedBy: ";").map {
-                $0.trimmingCharacters(in: .whitespaces)
-            }
-
-            if components.isEmpty {
+            // The parsing of "Set-Cookie" headers is defined by Section 5.2, RFC-6265:
+            // https://datatracker.ietf.org/doc/html/rfc6265#section-5.2
+            var components = header.utf8.split(separator: UInt8(ascii: ";"), omittingEmptySubsequences: false)[...]
+            guard let keyValuePair = components.popFirst()?.trimmingASCIISpaces() else {
                 return nil
             }
-
-            let nameAndValue = components[0].split(separator: "=", maxSplits: 1, omittingEmptySubsequences: false).map {
-                $0.trimmingCharacters(in: .whitespaces)
-            }
-
-            guard nameAndValue.count == 2 else {
+            guard let (trimmedName, trimmedValue) = keyValuePair.parseKeyValuePair() else {
                 return nil
             }
-
-            self.name = nameAndValue[0]
-            self.value = nameAndValue[1].omittingQuotes()
-
-            guard !self.name.isEmpty else {
+            guard !trimmedName.isEmpty else {
                 return nil
             }
 
-            self.path = "/"
-            self.domain = defaultDomain
-            self.expires = nil
+            self.name = String(aligningUTF8: trimmedName)
+            self.value = String(aligningUTF8: trimmedValue.trimmingPairedASCIIQuote())
+            self.expires_timestamp = nil
             self.maxAge = nil
             self.httpOnly = false
             self.secure = false
 
-            for component in components[1...] {
-                switch self.parseComponent(component) {
-                case (nil, nil):
-                    continue
-                case ("path", .some(let value)):
-                    self.path = value
-                case ("domain", .some(let value)):
-                    self.domain = value
-                case ("expires", let value):
-                    guard let value = value else {
+            var parsedPath: String.UTF8View.SubSequence?
+            var parsedDomain: String.UTF8View.SubSequence?
+
+            for component in components {
+                switch component.parseCookieComponent() {
+                case ("path", let value)?:
+                    // Unlike other values, unspecified, empty, and invalid paths reset to the default path.
+                    // https://datatracker.ietf.org/doc/html/rfc6265#section-5.2.4
+                    guard let value = value, value.first == UInt8(ascii: "/") else {
+                        parsedPath = nil
                         continue
                     }
-
-                    let formatter = DateFormatter()
-                    formatter.locale = Locale(identifier: "en_US")
-                    formatter.timeZone = TimeZone(identifier: "GMT")
-
-                    formatter.dateFormat = "EEE, dd MMM yyyy HH:mm:ss z"
-                    if let date = formatter.date(from: value) {
-                        self.expires = date
+                    parsedPath = value
+                case ("domain", let value)?:
+                    guard var value = value, !value.isEmpty else {
                         continue
                     }
-
-                    formatter.dateFormat = "EEE, dd-MMM-yy HH:mm:ss z"
-                    if let date = formatter.date(from: value) {
-                        self.expires = date
+                    if value.first == UInt8(ascii: ".") {
+                        value.removeFirst()
+                    }
+                    guard !value.isEmpty else {
+                        parsedDomain = nil
                         continue
                     }
-
-                    formatter.dateFormat = "EEE MMM d hh:mm:s yyyy"
-                    if let date = formatter.date(from: value) {
-                        self.expires = date
+                    parsedDomain = value
+                case ("expires", let value)?:
+                    guard let value = value, let timestamp = parseCookieTime(value) else {
+                        continue
                     }
-                case ("max-age", let value):
-                    self.maxAge = value.flatMap(Int.init)
-                case ("secure", nil):
+                    self.expires_timestamp = timestamp
+                case ("max-age", let value)?:
+                    guard let value = value, let age = Int(Substring(value)) else {
+                        continue
+                    }
+                    self.maxAge = age
+                case ("secure", _)?:
                     self.secure = true
-                case ("httponly", nil):
+                case ("httponly", _)?:
                     self.httpOnly = true
                 default:
                     continue
                 }
             }
+
+            self.domain = parsedDomain.map { Substring($0).lowercased() } ?? defaultDomain.lowercased()
+            self.path = parsedPath.map { String(aligningUTF8: $0) } ?? "/"
         }
 
         /// Create HTTP cookie.
@@ -124,51 +122,114 @@ extension HTTPClient {
         ///     - value: The cookie's string value.
         ///     - path: The cookie's path.
         ///     - domain: The domain of the cookie, defaults to nil.
-        ///     - expires: The cookie's expiration date, defaults to nil.
+        ///     - expires_timestamp: The cookie's expiration date, as a number of seconds since the Unix epoch. defaults to nil.
         ///     - maxAge: The cookie's age in seconds, defaults to nil.
         ///     - httpOnly: Whether this cookie should be used by HTTP servers only, defaults to false.
         ///     - secure: Whether this cookie should only be sent using secure channels, defaults to false.
-        public init(name: String, value: String, path: String = "/", domain: String? = nil, expires: Date? = nil, maxAge: Int? = nil, httpOnly: Bool = false, secure: Bool = false) {
+        internal init(name: String, value: String, path: String = "/", domain: String? = nil, expires_timestamp: Int64? = nil, maxAge: Int? = nil, httpOnly: Bool = false, secure: Bool = false) {
             self.name = name
             self.value = value
             self.path = path
             self.domain = domain
-            self.expires = expires
+            self.expires_timestamp = expires_timestamp
             self.maxAge = maxAge
             self.httpOnly = httpOnly
             self.secure = secure
         }
+    }
+}
 
-        func parseComponent(_ component: String) -> (String?, String?) {
-            let nameAndValue = component.split(separator: "=", maxSplits: 1).map {
-                $0.trimmingCharacters(in: .whitespaces)
-            }
-            if nameAndValue.count == 2 {
-                return (nameAndValue[0].lowercased(), nameAndValue[1])
-            } else if nameAndValue.count == 1 {
-                return (nameAndValue[0].lowercased(), nil)
-            }
-            return (nil, nil)
-        }
+extension HTTPClient.Response {
+    /// List of HTTP cookies returned by the server.
+    public var cookies: [HTTPClient.Cookie] {
+        return self.headers["set-cookie"].compactMap { HTTPClient.Cookie(header: $0, defaultDomain: self.host) }
     }
 }
 
 extension String {
-    fileprivate func omittingQuotes() -> String {
-        let dquote = "\""
-        if !hasPrefix(dquote) || !hasSuffix(dquote) {
-            return self
+    /// Creates a String from a slice of UTF8 code-units, aligning the bounds to unicode scalar boundaries if needed.
+    fileprivate init(aligningUTF8 utf8Slice: String.UTF8View.SubSequence) {
+        self.init(Substring(utf8Slice))
+    }
+}
+
+extension String.UTF8View.SubSequence {
+    fileprivate func trimmingASCIISpaces() -> SubSequence {
+        guard let start = self.firstIndex(where: { $0 != UInt8(ascii: " ") }) else {
+            return self[self.endIndex..<self.endIndex]
+        }
+        let end = self.lastIndex(where: { $0 != UInt8(ascii: " ") })!
+        return self[start...end]
+    }
+
+    /// If this collection begins and ends with an ASCII double-quote ("),
+    /// returns a version of self trimmed of those quotes. Otherwise, returns self.
+    fileprivate func trimmingPairedASCIIQuote() -> SubSequence {
+        let quoteChar = UInt8(ascii: "\"")
+        var trimmed = self
+        if trimmed.popFirst() == quoteChar && trimmed.popLast() == quoteChar {
+            return trimmed
+        }
+        return self
+    }
+
+    /// Splits this collection in to a key and value at the first ASCII '=' character.
+    /// Both the key and value are trimmed of ASCII spaces.
+    fileprivate func parseKeyValuePair() -> (key: SubSequence, value: SubSequence)? {
+        guard let keyValueSeparator = self.firstIndex(of: UInt8(ascii: "=")) else {
+            return nil
         }
+        let trimmedName = self[..<keyValueSeparator].trimmingASCIISpaces()
+        let trimmedValue = self[self.index(after: keyValueSeparator)...].trimmingASCIISpaces()
+        return (trimmedName, trimmedValue)
+    }
 
-        let begin = index(after: startIndex)
-        let end = index(before: endIndex)
-        return String(self[begin..<end])
+    /// Parses this collection as either a key-value pair, or a plain key.
+    /// The returned key is trimmed of ASCII spaces and normalized to lowercase.
+    /// The returned value is trimmed of ASCII spaces.
+    fileprivate func parseCookieComponent() -> (key: String, value: SubSequence?)? {
+        let (trimmedName, trimmedValue) = self.parseKeyValuePair() ?? (self.trimmingASCIISpaces(), nil)
+        guard !trimmedName.isEmpty else {
+            return nil
+        }
+        return (Substring(trimmedName).lowercased(), trimmedValue)
     }
 }
 
-extension HTTPClient.Response {
-    /// List of HTTP cookies returned by the server.
-    public var cookies: [HTTPClient.Cookie] {
-        return self.headers["set-cookie"].compactMap { HTTPClient.Cookie(header: $0, defaultDomain: self.host) }
+private let posixLocale: UnsafeMutableRawPointer = {
+    // All POSIX systems must provide a "POSIX" locale, and its date/time formats are US English.
+    // https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap07.html#tag_07_03_05
+    let _posixLocale = newlocale(LC_TIME_MASK | LC_NUMERIC_MASK, "POSIX", nil)!
+    return UnsafeMutableRawPointer(_posixLocale)
+}()
+
+private func parseTimestamp(_ utf8: String.UTF8View.SubSequence, format: String) -> tm? {
+    var timeComponents = tm()
+    let success = Substring(utf8).withCString { cString in
+        swiftahc_cshims_strptime_l(cString, format, &timeComponents, posixLocale)
+    }
+    return success ? timeComponents : nil
+}
+
+private func parseCookieTime(_ timestampUTF8: String.UTF8View.SubSequence) -> Int64? {
+    if timestampUTF8.contains(where: { $0 < 0x20 /* Control characters */ || $0 == 0x7F /* DEL */ }) {
+        return nil
+    }
+    var timestampUTF8 = timestampUTF8
+    if timestampUTF8.hasSuffix("GMT".utf8) {
+        let timezoneStart = timestampUTF8.index(timestampUTF8.endIndex, offsetBy: -3)
+        timestampUTF8 = timestampUTF8[..<timezoneStart].trimmingASCIISpaces()
+        guard timestampUTF8.endIndex != timezoneStart else {
+            return nil
+        }
+    }
+    guard
+        var timeComponents = parseTimestamp(timestampUTF8, format: "%a, %d %b %Y %H:%M:%S")
+        ?? parseTimestamp(timestampUTF8, format: "%a, %d-%b-%y %H:%M:%S")
+        ?? parseTimestamp(timestampUTF8, format: "%a %b %d %H:%M:%S %Y")
+    else {
+        return nil
     }
+    let timestamp = Int64(timegm(&timeComponents))
+    return timestamp == -1 && errno == EOVERFLOW ? nil : timestamp
 }

Diff for: Sources/AsyncHTTPClient/Utils.swift

@@ -41,3 +41,23 @@ public final class HTTPClientCopyingDelegate: HTTPClientResponseDelegate {
 internal func debugOnly(_ body: () -> Void) {
     assert({ body(); return true }())
 }
+
+extension BidirectionalCollection where Element: Equatable {
+    /// Returns a Boolean value indicating whether the collection ends with the specified suffix.
+    ///
+    /// If `suffix` is empty, this function returns `true`.
+    /// If all elements of the collections are equal, this function also returns `true`.
+    func hasSuffix<Suffix>(_ suffix: Suffix) -> Bool where Suffix: BidirectionalCollection, Suffix.Element == Element {
+        var ourIdx = self.endIndex
+        var suffixIdx = suffix.endIndex
+        while ourIdx > self.startIndex, suffixIdx > suffix.startIndex {
+            self.formIndex(before: &ourIdx)
+            suffix.formIndex(before: &suffixIdx)
+            guard self[ourIdx] == suffix[suffixIdx] else { return false }
+        }
+        guard suffixIdx == suffix.startIndex else {
+            return false // Exhausted self, but 'suffix' has elements remaining.
+        }
+        return true // Exhausted 'other' without finding a mismatch.
+    }
+}