From 7090de5242ebe67d34695cc102c9b601216d69ad Mon Sep 17 00:00:00 2001 From: Thomas Rademaker Date: Thu, 16 Apr 2026 16:25:22 -0400 Subject: [PATCH] stability and correctness --- Sources/CoreATProtocol/APEnvironment.swift | 1 + Sources/CoreATProtocol/Base64URL.swift | 28 +++++++ Sources/CoreATProtocol/ClockSkewStore.swift | 66 ++++++++++++++++ Sources/CoreATProtocol/Networking.swift | 42 ++++------ .../CoreATProtocol/OAuth/ATProtoOAuth.swift | 48 ++++++++---- .../OAuth/IdentityResolver.swift | 78 ++++++++++++++++--- .../TokenRefreshCoordinator.swift | 29 +++++++ 7 files changed, 235 insertions(+), 57 deletions(-) create mode 100644 Sources/CoreATProtocol/Base64URL.swift create mode 100644 Sources/CoreATProtocol/ClockSkewStore.swift create mode 100644 Sources/CoreATProtocol/TokenRefreshCoordinator.swift diff --git a/Sources/CoreATProtocol/APEnvironment.swift b/Sources/CoreATProtocol/APEnvironment.swift index 702efe3..45de5e1 100644 --- a/Sources/CoreATProtocol/APEnvironment.swift +++ b/Sources/CoreATProtocol/APEnvironment.swift @@ -19,6 +19,7 @@ public class APEnvironment { public var dpopPrivateKey: ES256PrivateKey? public var dpopKeys: JWTKeyCollection? public let dpopNonceStore = DPoPNonceStore() + public let clockSkewStore = ClockSkewStore() public let routerDelegate = APRouterDelegate() private init() {} diff --git a/Sources/CoreATProtocol/Base64URL.swift b/Sources/CoreATProtocol/Base64URL.swift new file mode 100644 index 0000000..ae4fb3f --- /dev/null +++ b/Sources/CoreATProtocol/Base64URL.swift @@ -0,0 +1,28 @@ +// +// Base64URL.swift +// CoreATProtocol +// + +import Foundation + +extension Data { + /// Base64URL-encoded representation (RFC 4648 §5, unpadded). + /// + /// Used for DPoP proof generation and JWK encoding, where standard + /// base64 characters (`+`, `/`, `=`) are disallowed. + func base64URLEncodedString() -> String { + base64EncodedString() + .replacingOccurrences(of: "+", with: "-") + .replacingOccurrences(of: "/", with: "_") + .replacingOccurrences(of: "=", with: "") + } +} + +extension String { + /// Convert a standard base64 string to the base64url alphabet (unpadded). + func base64URLEncoded() -> String { + replacingOccurrences(of: "+", with: "-") + .replacingOccurrences(of: "/", with: "_") + .replacingOccurrences(of: "=", with: "") + } +} diff --git a/Sources/CoreATProtocol/ClockSkewStore.swift b/Sources/CoreATProtocol/ClockSkewStore.swift new file mode 100644 index 0000000..504d273 --- /dev/null +++ b/Sources/CoreATProtocol/ClockSkewStore.swift @@ -0,0 +1,66 @@ +// +// ClockSkewStore.swift +// CoreATProtocol +// + +import Foundation + +/// Tracks the observed offset between the local clock and a server's clock. +/// +/// DPoP proofs are rejected if `iat` is outside the server's acceptance window. +/// When a device clock drifts, proofs signed with `Date.now` fail with a +/// `invalid_dpop_proof` error. By watching the `Date` HTTP header on responses +/// we can record the skew and apply it to future `iat` values, preventing +/// repeated auth failures on skewed clients. +public actor ClockSkewStore { + private(set) var offset: TimeInterval = 0 + + public init(offset: TimeInterval = 0) { + self.offset = offset + } + + public func update(offset: TimeInterval) { + self.offset = offset + } + + /// Parses the `Date` HTTP header and stores the offset between the server + /// time and the local clock. Silently ignores missing or unparseable values. + public func updateFromServerDate(_ dateHeader: String?, localNow: Date = .now) { + guard let dateHeader, + let serverDate = Self.parse(dateHeader) else { return } + offset = serverDate.timeIntervalSince(localNow) + } + + /// Returns "now" adjusted for observed server skew. + public func serverAdjustedNow(localNow: Date = .now) -> Date { + localNow.addingTimeInterval(offset) + } + + // MARK: - Date parsing + + nonisolated static func parse(_ header: String) -> Date? { + for formatter in formatters { + if let date = formatter.date(from: header) { + return date + } + } + return nil + } + + nonisolated private static let formatters: [DateFormatter] = { + // HTTP permits three formats per RFC 7231 §7.1.1.1. RFC 1123 is the + // preferred one; RFC 850 and asctime are retained for legacy servers. + let patterns = [ + "EEE, dd MMM yyyy HH:mm:ss zzz", + "EEEE, dd-MMM-yy HH:mm:ss zzz", + "EEE MMM d HH:mm:ss yyyy", + ] + return patterns.map { pattern in + let formatter = DateFormatter() + formatter.dateFormat = pattern + formatter.locale = Locale(identifier: "en_US_POSIX") + formatter.timeZone = TimeZone(secondsFromGMT: 0) + return formatter + } + }() +} diff --git a/Sources/CoreATProtocol/Networking.swift b/Sources/CoreATProtocol/Networking.swift index 3a970d3..5f9206d 100644 --- a/Sources/CoreATProtocol/Networking.swift +++ b/Sources/CoreATProtocol/Networking.swift @@ -50,7 +50,7 @@ func shouldPerformRequest(lastFetched: Double, timeLimit: Int = 3600) -> Bool { @NetworkingKitActor public class APRouterDelegate: NetworkRouterDelegate { - private var refreshTask: Task? + private let refreshCoordinator = TokenRefreshCoordinator() public func intercept(_ request: inout URLRequest) async { guard let accessToken = await APEnvironment.current.accessToken else { return } @@ -98,18 +98,16 @@ public class APRouterDelegate: NetworkRouterDelegate { // Read the nonce at proof-generation time so a concurrent update // between intercept() and sign() is observed on the next retry. let nonce = await APEnvironment.current.dpopNonceStore.get() + let issuedAt = await APEnvironment.current.clockSkewStore.serverAdjustedNow() // ath: base64url-encoded SHA-256 hash of the access token (RFC 9449 §4.2) let hash = SHA256.hash(data: Data(accessToken.utf8)) - let ath = Data(hash).base64EncodedString() - .replacingOccurrences(of: "+", with: "-") - .replacingOccurrences(of: "/", with: "_") - .replacingOccurrences(of: "=", with: "") + let ath = Data(hash).base64URLEncodedString() let payload = DPoPProofPayload( htm: method, htu: htu, - iat: .init(value: .now), + iat: .init(value: issuedAt), jti: .init(value: UUID().uuidString), nonce: nonce, ath: ath @@ -120,20 +118,11 @@ public class APRouterDelegate: NetworkRouterDelegate { header.alg = "ES256" if let keyParams = privateKey.parameters { - let xBase64URL = keyParams.x - .replacingOccurrences(of: "+", with: "-") - .replacingOccurrences(of: "/", with: "_") - .replacingOccurrences(of: "=", with: "") - let yBase64URL = keyParams.y - .replacingOccurrences(of: "+", with: "-") - .replacingOccurrences(of: "/", with: "_") - .replacingOccurrences(of: "=", with: "") - header.jwk = [ "kty": .string("EC"), "crv": .string("P-256"), - "x": .string(xBase64URL), - "y": .string(yBase64URL), + "x": .string(keyParams.x.base64URLEncoded()), + "y": .string(keyParams.y.base64URLEncoded()), ] } @@ -141,6 +130,13 @@ public class APRouterDelegate: NetworkRouterDelegate { } public func didReceiveErrorResponse(_ response: HTTPURLResponse) async { + // Record clock skew from any response that carries a Date header. + // DPoP rejections often correlate with skew, so harvest this eagerly. + let dateHeader = response.value(forHTTPHeaderField: "Date") + if let dateHeader { + await APEnvironment.current.clockSkewStore.updateFromServerDate(dateHeader) + } + let headerNonce = response.value(forHTTPHeaderField: "DPoP-Nonce") ?? response.value(forHTTPHeaderField: "dpop-nonce") if let headerNonce { @@ -188,17 +184,7 @@ public class APRouterDelegate: NetworkRouterDelegate { guard let handler = await APEnvironment.current.tokenRefreshHandler else { return false } - - if let refreshTask { - return try await refreshTask.value - } - - let task = Task { try await handler() } - refreshTask = task - - defer { refreshTask = nil } - - return try await task.value + return try await refreshCoordinator.refresh(using: handler) } private func isDPoPNonceError(from error: Error) -> Bool { diff --git a/Sources/CoreATProtocol/OAuth/ATProtoOAuth.swift b/Sources/CoreATProtocol/OAuth/ATProtoOAuth.swift index 43ce810..0c9c795 100644 --- a/Sources/CoreATProtocol/OAuth/ATProtoOAuth.swift +++ b/Sources/CoreATProtocol/OAuth/ATProtoOAuth.swift @@ -84,6 +84,12 @@ public enum ATProtoOAuthError: LocalizedError, Sendable { case malformedServerMetadata(field: String, value: String) case tokenRequestFailed(String) case invalidTokenResponse + /// The stored refresh token is missing or expired. Re-authentication required. + case refreshTokenUnavailable + /// The DPoP private key was never persisted, so refresh cannot bind a new token. + case refreshKeyUnavailable + /// Could not determine the authorization server for refresh. + case refreshIssuerUnavailable public var errorDescription: String? { switch self { @@ -109,6 +115,12 @@ public enum ATProtoOAuthError: LocalizedError, Sendable { "Token request failed: \(detail)" case .invalidTokenResponse: "Invalid token response from server" + case .refreshTokenUnavailable: + "Refresh token is missing or expired — re-authentication required" + case .refreshKeyUnavailable: + "DPoP private key is not available — re-authentication required" + case .refreshIssuerUnavailable: + "Could not determine the authorization server for refresh" } } } @@ -331,26 +343,36 @@ public final class ATProtoOAuth: Sendable { } /// Refresh tokens if the stored access token is expired (or if forced). + /// + /// - Returns: the refreshed `Login`, or `nil` when refresh is a no-op + /// (no stored login, or the access token is still valid and `force` is false). + /// - Throws: `ATProtoOAuthError.refreshTokenUnavailable`, + /// `.refreshKeyUnavailable`, or `.refreshIssuerUnavailable` when the session + /// cannot be refreshed and the caller must re-authenticate. public func refreshLoginIfNeeded(handle: String? = nil, force: Bool = false) async throws -> Login? { try await refreshLoginIfNeeded(accountIdentifier: handle, force: force) } /// Refresh tokens if the stored access token is expired (or if forced). + /// + /// See the `handle:force:` overload for documentation. public func refreshLoginIfNeeded(accountIdentifier: String? = nil, force: Bool = false) async throws -> Login? { guard let login = try await storage.retrieveLogin() else { + // No stored session — genuine no-op. return nil } if !force, login.accessToken.valid { + // Access token still valid — genuine no-op. return nil } guard hasPersistedKey else { - return nil + throw ATProtoOAuthError.refreshKeyUnavailable } guard login.refreshToken?.valid == true else { - return nil + throw ATProtoOAuthError.refreshTokenUnavailable } let resolvedIdentity: IdentityResolver.ResolvedIdentity? @@ -366,7 +388,7 @@ public final class ATProtoOAuth: Sendable { } else if let identity = resolvedIdentity { issuer = identity.authorizationServer } else { - return nil + throw ATProtoOAuthError.refreshIssuerUnavailable } let provider = URLSession.defaultProvider @@ -398,7 +420,7 @@ public final class ATProtoOAuth: Sendable { ) guard let refreshProvider = tokenHandling.refreshProvider else { - return nil + throw ATProtoOAuthError.refreshIssuerUnavailable } // Use proxy-aware provider when auth proxy is configured @@ -485,10 +507,12 @@ public final class ATProtoOAuth: Sendable { // Strip query params and fragments from htu per DPoP spec let htu = stripQueryAndFragment(from: params.requestEndpoint) + let issuedAt = await APEnvironment.current.clockSkewStore.serverAdjustedNow() + let payload = DPoPPayload( htm: params.httpMethod, htu: htu, - iat: .init(value: .now), + iat: .init(value: issuedAt), jti: .init(value: UUID().uuidString), nonce: params.nonce ) @@ -500,21 +524,11 @@ public final class ATProtoOAuth: Sendable { // Get public key parameters and convert to base64url for JWK if let keyParams = privateKey.parameters { - // Convert from base64 to base64url (replace + with -, / with _, remove =) - let xBase64URL = keyParams.x - .replacingOccurrences(of: "+", with: "-") - .replacingOccurrences(of: "/", with: "_") - .replacingOccurrences(of: "=", with: "") - let yBase64URL = keyParams.y - .replacingOccurrences(of: "+", with: "-") - .replacingOccurrences(of: "/", with: "_") - .replacingOccurrences(of: "=", with: "") - header.jwk = [ "kty": .string("EC"), "crv": .string("P-256"), - "x": .string(xBase64URL), - "y": .string(yBase64URL) + "x": .string(keyParams.x.base64URLEncoded()), + "y": .string(keyParams.y.base64URLEncoded()) ] } diff --git a/Sources/CoreATProtocol/OAuth/IdentityResolver.swift b/Sources/CoreATProtocol/OAuth/IdentityResolver.swift index 884aae6..2e086bf 100644 --- a/Sources/CoreATProtocol/OAuth/IdentityResolver.swift +++ b/Sources/CoreATProtocol/OAuth/IdentityResolver.swift @@ -6,6 +6,7 @@ public enum IdentityError: Error, Sendable { case resolutionFailed case invalidResponse case noPDSFound + case invalidPDSEndpoint(String) case noAuthServerFound case handleVerificationFailed } @@ -117,9 +118,7 @@ public struct IdentityResolver: Sendable { } private func resolveViaHTTPS(handle: String) async throws -> String { - guard let url = URL(string: "https://\(handle)/.well-known/atproto-did") else { - throw IdentityError.invalidHandle - } + let url = try Self.makeURL(scheme: "https", host: handle, path: "/.well-known/atproto-did") let (data, response) = try await URLSession.shared.data(from: url) @@ -139,8 +138,16 @@ public struct IdentityResolver: Sendable { private func resolveViaDNS(handle: String) async throws -> String { // Use Cloudflare DNS-over-HTTPS for TXT record lookup - let hostname = "_atproto.\(handle)" - guard let dohURL = URL(string: "https://1.1.1.1/dns-query?name=\(hostname)&type=TXT") else { + try Self.validateHostname(handle) + var components = URLComponents() + components.scheme = "https" + components.host = "1.1.1.1" + components.path = "/dns-query" + components.queryItems = [ + URLQueryItem(name: "name", value: "_atproto.\(handle)"), + URLQueryItem(name: "type", value: "TXT"), + ] + guard let dohURL = components.url else { throw IdentityError.resolutionFailed } @@ -178,16 +185,15 @@ public struct IdentityResolver: Sendable { private func resolveDIDDocument(did: String) async throws -> DIDDocument { let url: URL if did.hasPrefix("did:plc:") { - guard let plcURL = URL(string: "https://plc.directory/\(did)") else { + let identifier = String(did.dropFirst("did:plc:".count)) + guard !identifier.isEmpty, + identifier.allSatisfy({ $0.isLetter || $0.isNumber }) else { throw IdentityError.invalidDID } - url = plcURL + url = try Self.makeURL(scheme: "https", host: "plc.directory", path: "/\(did)") } else if did.hasPrefix("did:web:") { - let domain = did.replacingOccurrences(of: "did:web:", with: "") - guard let webURL = URL(string: "https://\(domain)/.well-known/did.json") else { - throw IdentityError.invalidDID - } - url = webURL + let domain = String(did.dropFirst("did:web:".count)) + url = try Self.makeURL(scheme: "https", host: domain, path: "/.well-known/did.json") } else { throw IdentityError.invalidDID } @@ -209,6 +215,11 @@ public struct IdentityResolver: Sendable { guard let pds = document.pdsEndpoint else { throw IdentityError.noPDSFound } + guard let url = URL(string: pds), + url.scheme?.lowercased() == "https", + let host = url.host, !host.isEmpty else { + throw IdentityError.invalidPDSEndpoint(pds) + } return pds } @@ -281,6 +292,49 @@ public struct IdentityResolver: Sendable { let port = url.port.map { ":\($0)" } ?? "" return "\(scheme)://\(host)\(port)" } + + // MARK: - URL construction + + /// Build a URL from trusted components after validating the host as a DNS-style + /// name. Using `URLComponents` (rather than string interpolation into + /// `URL(string:)`) ensures hosts containing otherwise-illegal characters fail + /// fast instead of silently producing a valid URL with a mangled authority. + nonisolated private static func makeURL(scheme: String, host: String, path: String) throws -> URL { + try validateHostname(host) + var components = URLComponents() + components.scheme = scheme + components.host = host + components.path = path + guard let url = components.url else { + throw IdentityError.invalidHandle + } + return url + } + + /// Validates that the input is a DNS-style hostname (letters, digits, + /// hyphens, dots) with non-empty labels. Rejects anything that could + /// smuggle a URL authority component (`@`, `/`, `:`, `?`, `#`, spaces, + /// non-ASCII, etc.). + nonisolated static func validateHostname(_ host: String) throws { + guard !host.isEmpty, host.count <= 253 else { + throw IdentityError.invalidHandle + } + let labels = host.split(separator: ".", omittingEmptySubsequences: false) + guard labels.count >= 2 else { + throw IdentityError.invalidHandle + } + for label in labels { + guard !label.isEmpty, label.count <= 63 else { + throw IdentityError.invalidHandle + } + guard label.first != "-", label.last != "-" else { + throw IdentityError.invalidHandle + } + guard label.allSatisfy({ $0.isASCII && ($0.isLetter || $0.isNumber || $0 == "-") }) else { + throw IdentityError.invalidHandle + } + } + } } // MARK: - Supporting Types diff --git a/Sources/CoreATProtocol/TokenRefreshCoordinator.swift b/Sources/CoreATProtocol/TokenRefreshCoordinator.swift new file mode 100644 index 0000000..d8a0c92 --- /dev/null +++ b/Sources/CoreATProtocol/TokenRefreshCoordinator.swift @@ -0,0 +1,29 @@ +// +// TokenRefreshCoordinator.swift +// CoreATProtocol +// + +/// Serialises concurrent token-refresh attempts. +/// +/// When several in-flight requests receive a 401 at the same time, each would +/// otherwise race to call the refresh handler. This actor coalesces them onto +/// a single underlying refresh task so the handler is invoked exactly once +/// per burst — all callers await the same result. +actor TokenRefreshCoordinator { + private var refreshTask: Task? + + /// Run `handler` if no refresh is currently in flight; otherwise join the + /// existing attempt and return its result. + func refresh(using handler: @Sendable @escaping () async throws -> Bool) async throws -> Bool { + if let refreshTask { + return try await refreshTask.value + } + + let task = Task { try await handler() } + refreshTask = task + + defer { refreshTask = nil } + + return try await task.value + } +} -- 2.51.2