Skip to content

Redo HTTP cookie parsing #510

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.

Sign up for GitHub

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 3 commits into from
Jan 6, 2022
Merged

Redo HTTP cookie parsing #510

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
273ebad
Redo HTTP cookie parsing using strptime
karwa Jan 4, 2022
3bebe58
Make String(utf8Slice:from:) less ugly
karwa Jan 4, 2022
899e29c
Adjust cookie component parsing to better match RFC-6562
karwa Jan 4, 2022
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
2 changes: 2 additions & 0 deletions Package.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
Expand Down
56 changes: 56 additions & 0 deletions Sources/AsyncHTTPClient/FoundationExtensions.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this is public API, should we make some room for ourselves by making this failable now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making it failable would be an API-breaking change (semver-major) though, right?

It would be good to do, but I'd rather land this and check that out as a follow-up.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, I lost track of the fact that this is an existing init. We'd need to deprecate-and-replace it. Happy to delay that to a follow-up.

// 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
)
}
}
215 changes: 138 additions & 77 deletions Sources/AsyncHTTPClient/HTTPClient+HTTPCookie.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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.
Expand All @@ -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 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fun task for the future: let's just write our own Int.init(_ bytes: Substring.UTF8View) initializer. It doesn't really matter, but it'd be fun and very slightly faster. Let's not do it now though, just planting seeds to keep you engaged in the future 😉.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's not too difficult. I already did a similar thing for WebURL, because the standard library's version only accepts StringProtocol rather than arbitrary buffers of bytes: https://github.com/karwa/swift-url/blob/1f1263f94e25eb43d8688dc668de6638e092f748/Sources/WebURL/Util/ASCII.swift#L502

But yeah, it can be a follow-up. I'll have a patch up which addresses the nits in a little bit.

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.
Expand All @@ -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
}
20 changes: 20 additions & 0 deletions Sources/AsyncHTTPClient/Utils.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
}
}
Loading