From b6a6e02762d0c5d65e3e39bf1e61acae3ba507b5 Mon Sep 17 00:00:00 2001 From: Thomas Rademaker Date: Thu, 16 Apr 2026 16:13:58 -0400 Subject: [PATCH] Security Hotfixes --- Sources/CoreATProtocol/APEnvironment.swift | 2 +- Sources/CoreATProtocol/DPoPNonceStore.swift | 23 +++++++ Sources/CoreATProtocol/Networking.swift | 64 +++++++++++++------ .../CoreATProtocol/OAuth/ATProtoOAuth.swift | 46 ++++++++++--- 4 files changed, 107 insertions(+), 28 deletions(-) create mode 100644 Sources/CoreATProtocol/DPoPNonceStore.swift diff --git a/Sources/CoreATProtocol/APEnvironment.swift b/Sources/CoreATProtocol/APEnvironment.swift index e161ba8..702efe3 100644 --- a/Sources/CoreATProtocol/APEnvironment.swift +++ b/Sources/CoreATProtocol/APEnvironment.swift @@ -18,7 +18,7 @@ public class APEnvironment { public var tokenRefreshHandler: (@Sendable () async throws -> Bool)? public var dpopPrivateKey: ES256PrivateKey? public var dpopKeys: JWTKeyCollection? - public var dpopNonce: String? + public let dpopNonceStore = DPoPNonceStore() public let routerDelegate = APRouterDelegate() private init() {} diff --git a/Sources/CoreATProtocol/DPoPNonceStore.swift b/Sources/CoreATProtocol/DPoPNonceStore.swift new file mode 100644 index 0000000..f98cf7c --- /dev/null +++ b/Sources/CoreATProtocol/DPoPNonceStore.swift @@ -0,0 +1,23 @@ +// +// DPoPNonceStore.swift +// CoreATProtocol +// + +/// Serialises reads and writes to the DPoP server-issued nonce. +/// +/// RFC 9449 allows a server to rotate the DPoP nonce on any response. Multiple +/// in-flight requests can observe a nonce update concurrently, so a dedicated +/// actor is used to keep the read/update pair ordered. +public actor DPoPNonceStore { + private var nonce: String? + + public init(nonce: String? = nil) { + self.nonce = nonce + } + + public func get() -> String? { nonce } + + public func update(_ nonce: String) { self.nonce = nonce } + + public func clear() { nonce = nil } +} diff --git a/Sources/CoreATProtocol/Networking.swift b/Sources/CoreATProtocol/Networking.swift index db941f7..3a970d3 100644 --- a/Sources/CoreATProtocol/Networking.swift +++ b/Sources/CoreATProtocol/Networking.swift @@ -9,6 +9,10 @@ import Foundation import Crypto import JWTKit import NetworkingKit +import OAuthenticator +import os + +private let networkingLog = Logger(subsystem: "com.sparrowtek.CoreATProtocol", category: "Networking") extension JSONDecoder { public static var atDecoder: JSONDecoder { @@ -54,10 +58,19 @@ public class APRouterDelegate: NetworkRouterDelegate { if let dpopKey = await APEnvironment.current.dpopPrivateKey, let keys = await APEnvironment.current.dpopKeys { // DPoP-bound token: use "DPoP" scheme + DPoP proof header - request.setValue("DPoP \(accessToken)", forHTTPHeaderField: "Authorization") - - if let proof = await generateDPoPProof(for: request, accessToken: accessToken, privateKey: dpopKey, keys: keys) { + do { + let proof = try await generateDPoPProof(for: request, accessToken: accessToken, privateKey: dpopKey, keys: keys) + request.setValue("DPoP \(accessToken)", forHTTPHeaderField: "Authorization") request.setValue(proof, forHTTPHeaderField: "DPoP") + } catch { + // DPoP signing failed. Do NOT attach the DPoP-bound access token + // without a proof โ€” that would send a malformed authenticated + // request and waste a round trip. Clearing the header causes + // the networking layer to fail with an unauthenticated error, + // which surfaces a useful signal to callers. + networkingLog.error("DPoP proof generation failed: \(error.localizedDescription, privacy: .public)") + request.setValue(nil, forHTTPHeaderField: "Authorization") + request.setValue(nil, forHTTPHeaderField: "DPoP") } } else { // Standard Bearer token @@ -70,9 +83,11 @@ public class APRouterDelegate: NetworkRouterDelegate { accessToken: String, privateKey: ES256PrivateKey, keys: JWTKeyCollection - ) async -> String? { + ) async throws -> String { guard let method = request.httpMethod, - let url = request.url else { return nil } + let url = request.url else { + throw AtError.message(ErrorMessage(error: "DPoPProofInvalidRequest", message: "Request is missing method or URL")) + } // Strip query and fragment per DPoP spec var components = URLComponents(url: url, resolvingAgainstBaseURL: false) @@ -80,7 +95,9 @@ public class APRouterDelegate: NetworkRouterDelegate { components?.fragment = nil let htu = components?.url?.absoluteString ?? url.absoluteString - let nonce = await APEnvironment.current.dpopNonce + // 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() // ath: base64url-encoded SHA-256 hash of the access token (RFC 9449 ยง4.2) let hash = SHA256.hash(data: Data(accessToken.utf8)) @@ -120,21 +137,21 @@ public class APRouterDelegate: NetworkRouterDelegate { ] } - return try? await keys.sign(payload, header: header) + return try await keys.sign(payload, header: header) } public func didReceiveErrorResponse(_ response: HTTPURLResponse) async { - let nonce = response.value(forHTTPHeaderField: "DPoP-Nonce") + let headerNonce = response.value(forHTTPHeaderField: "DPoP-Nonce") ?? response.value(forHTTPHeaderField: "dpop-nonce") - if let nonce { - await storeDPoPNonce(nonce) + if let headerNonce { + await APEnvironment.current.dpopNonceStore.update(headerNonce) + lastErrorHadNonceHeader = true + } else { + lastErrorHadNonceHeader = false } } - @APActor - private func storeDPoPNonce(_ nonce: String) { - APEnvironment.current.dpopNonce = nonce - } + private var lastErrorHadNonceHeader: Bool = false public func shouldRetry(error: Error, attempts: Int) async throws -> Bool { guard attempts <= 2 else { return false } @@ -196,10 +213,21 @@ public class APRouterDelegate: NetworkRouterDelegate { data = nil } - guard let data, - let json = try? JSONSerialization.jsonObject(with: data) as? [String: Any], - let errorType = json["error"] as? String else { return false } - return errorType == "use_dpop_nonce" + if let data { + do { + let oauthError = try JSONDecoder().decode(OAuthErrorResponse.self, from: data) + if oauthError.error == "use_dpop_nonce" { return true } + // Body decoded but wasn't a nonce challenge. + return false + } catch { + let body = String(data: data, encoding: .utf8) ?? "" + networkingLog.debug("Failed to decode OAuth error body for nonce check: \(error.localizedDescription, privacy: .public) โ€” body: \(body, privacy: .public)") + } + } + + // Fallback: a DPoP-Nonce header on an error response is a strong + // nonce-challenge signal even when the body is absent or malformed. + return lastErrorHadNonceHeader } } diff --git a/Sources/CoreATProtocol/OAuth/ATProtoOAuth.swift b/Sources/CoreATProtocol/OAuth/ATProtoOAuth.swift index 1ed5f89..43ce810 100644 --- a/Sources/CoreATProtocol/OAuth/ATProtoOAuth.swift +++ b/Sources/CoreATProtocol/OAuth/ATProtoOAuth.swift @@ -1,6 +1,9 @@ import Foundation import OAuthenticator import JWTKit +import os + +private let oauthLog = Logger(subsystem: "com.sparrowtek.CoreATProtocol", category: "OAuth") // MARK: - Re-export OAuthenticator types for convenience public typealias Login = OAuthenticator.Login @@ -78,6 +81,7 @@ public enum ATProtoOAuthError: LocalizedError, Sendable { case subjectMismatch(expected: String, actual: String) case issuerMismatch(expected: String, actual: String) case malformedAuthorizationCallback + case malformedServerMetadata(field: String, value: String) case tokenRequestFailed(String) case invalidTokenResponse @@ -99,6 +103,8 @@ public enum ATProtoOAuthError: LocalizedError, Sendable { "Issuer mismatch โ€” expected \(expected), got \(actual)" case .malformedAuthorizationCallback: "Authorization callback URL is malformed" + case .malformedServerMetadata(let field, let value): + "Server metadata field \(field) is not a valid URL: \(value)" case .tokenRequestFailed(let detail): "Token request failed: \(detail)" case .invalidTokenResponse: @@ -239,7 +245,7 @@ public final class ATProtoOAuth: Sendable { } // Step 7: Create authenticator configuration - let tokenHandling = buildTokenHandling( + let tokenHandling = try buildTokenHandling( accountHint: identifier, server: serverConfig, jwtGenerator: jwtGenerator, @@ -383,7 +389,7 @@ public final class ATProtoOAuth: Sendable { } } - let tokenHandling = buildTokenHandling( + let tokenHandling = try buildTokenHandling( accountHint: accountIdentifier, server: serverConfig, jwtGenerator: jwtGenerator, @@ -630,16 +636,31 @@ public final class ATProtoOAuth: Sendable { } private func retrieveAuthProxyKeyID(for login: Login) async -> String? { - try? await storage.retrieveAuthProxyKeyID?(login) + guard let retrieve = storage.retrieveAuthProxyKeyID else { return nil } + do { + return try await retrieve(login) + } catch { + oauthLog.error("Failed to retrieve auth proxy key ID: \(error.localizedDescription, privacy: .public)") + return nil + } } private func persistAuthProxyKeyID(_ keyID: String, for login: Login) async { - try? await storage.storeAuthProxyKeyID?(login, keyID) + guard let store = storage.storeAuthProxyKeyID else { return } + do { + try await store(login, keyID) + } catch { + oauthLog.error("Failed to persist auth proxy key ID: \(error.localizedDescription, privacy: .public)") + } } private func clearAuthProxyKeyID(for login: Login?) async { - guard let login else { return } - try? await storage.clearAuthProxyKeyID?(login) + guard let login, let clear = storage.clearAuthProxyKeyID else { return } + do { + try await clear(login) + } catch { + oauthLog.error("Failed to clear auth proxy key ID: \(error.localizedDescription, privacy: .public)") + } } private func buildTokenHandling( @@ -648,10 +669,17 @@ public final class ATProtoOAuth: Sendable { jwtGenerator: @escaping DPoPSigner.JWTGenerator, expectedSubjectDID: String?, expectedAuthorizationServer: String - ) -> TokenHandling { - TokenHandling( + ) throws -> TokenHandling { + guard let parURL = URL(string: server.pushedAuthorizationRequestEndpoint) else { + throw ATProtoOAuthError.malformedServerMetadata( + field: "pushed_authorization_request_endpoint", + value: server.pushedAuthorizationRequestEndpoint + ) + } + + return TokenHandling( parConfiguration: PARConfiguration( - url: URL(string: server.pushedAuthorizationRequestEndpoint)!, + url: parURL, parameters: { if let accountHint { ["login_hint": accountHint] } else { [:] } }() ), authorizationURLProvider: authorizationURLProvider(server: server), -- 2.51.2