diff --git a/Sources/CoreATProtocol/OAuth/ATProtoOAuth.swift b/Sources/CoreATProtocol/OAuth/ATProtoOAuth.swift index d1e93f7..68a9b9a 100644 --- a/Sources/CoreATProtocol/OAuth/ATProtoOAuth.swift +++ b/Sources/CoreATProtocol/OAuth/ATProtoOAuth.swift @@ -783,22 +783,13 @@ public final class ATProtoOAuth: Sendable { } let tokenResponse = try Self.decodeTokenResponse(from: data) - guard tokenResponse.tokenType == "DPoP" else { - throw AuthenticatorError.dpopTokenExpected(tokenResponse.tokenType) - } - guard tokenResponse.scopes.contains("atproto") else { - throw ATProtoOAuthError.missingRequiredScope("atproto") - } + try TokenValidator(expectedSubjectDID: expectedSubjectDID).validate(tokenResponse) if iss != server.issuer { throw AuthenticatorError.issuingServerMismatch(iss, server.issuer) } try Self.validateIssuer(iss, matches: expectedAuthorizationServer) - if let expectedSubjectDID, tokenResponse.subject != expectedSubjectDID { - throw ATProtoOAuthError.subjectMismatch(expected: expectedSubjectDID, actual: tokenResponse.subject) - } - return tokenResponse.login(for: iss) } } @@ -839,16 +830,7 @@ public final class ATProtoOAuth: Sendable { } let tokenResponse = try Self.decodeTokenResponse(from: data) - guard tokenResponse.tokenType == "DPoP" else { - throw AuthenticatorError.dpopTokenExpected(tokenResponse.tokenType) - } - guard tokenResponse.scopes.contains("atproto") else { - throw ATProtoOAuthError.missingRequiredScope("atproto") - } - - if let expectedSubjectDID, tokenResponse.subject != expectedSubjectDID { - throw ATProtoOAuthError.subjectMismatch(expected: expectedSubjectDID, actual: tokenResponse.subject) - } + try TokenValidator(expectedSubjectDID: expectedSubjectDID).validate(tokenResponse) return tokenResponse.login(for: login.issuingServer ?? server.issuer) } @@ -865,19 +847,10 @@ public final class ATProtoOAuth: Sendable { nonisolated private static func validateIssuer(_ issuer: String, matches expectedAuthorizationServer: String) throws { guard let issuerURL = URL(string: issuer), let expectedURL = URL(string: expectedAuthorizationServer), - normalizedOrigin(issuerURL) == normalizedOrigin(expectedURL) else { + URLOrigin.normalized(issuerURL) == URLOrigin.normalized(expectedURL) else { throw ATProtoOAuthError.issuerMismatch(expected: expectedAuthorizationServer, actual: issuer) } } - - nonisolated private static func normalizedOrigin(_ url: URL) -> String? { - guard let scheme = url.scheme?.lowercased(), - let host = url.host?.lowercased() else { - return nil - } - let port = url.port.map { ":\($0)" } ?? "" - return "\(scheme)://\(host)\(port)" - } } private actor DPoPRequestActor { @@ -942,7 +915,7 @@ private struct OAuthRefreshTokenRequest: Codable { } } -private struct OAuthTokenResponse: Codable { +struct OAuthTokenResponse: Codable, Sendable { let accessToken: String let refreshToken: String? let subject: String diff --git a/Sources/CoreATProtocol/OAuth/DPoPProofSigner.swift b/Sources/CoreATProtocol/OAuth/DPoPProofSigner.swift index d0b03e8..0215433 100644 --- a/Sources/CoreATProtocol/OAuth/DPoPProofSigner.swift +++ b/Sources/CoreATProtocol/OAuth/DPoPProofSigner.swift @@ -64,7 +64,7 @@ public actor DPoPProofSigner { let nonce: String? if let explicit = params.explicitNonce { nonce = explicit - } else if let origin = Self.origin(for: params.url) { + } else if let origin = URLOrigin.normalized(params.url) { nonce = noncesByOrigin[origin] } else { nonce = nil @@ -100,7 +100,7 @@ public actor DPoPProofSigner { /// growth bounded — a true LRU is overkill since the realistic origin /// count is two (auth server and PDS). public func cacheNonce(_ nonce: String, from url: URL) { - guard let origin = Self.origin(for: url) else { return } + guard let origin = URLOrigin.normalized(url) else { return } if noncesByOrigin.count >= nonceCacheLimit, noncesByOrigin[origin] == nil { if let key = noncesByOrigin.keys.first { noncesByOrigin.removeValue(forKey: key) @@ -122,15 +122,6 @@ public actor DPoPProofSigner { return components?.url?.absoluteString ?? url.absoluteString } - static func origin(for url: URL) -> String? { - guard let scheme = url.scheme?.lowercased(), - let host = url.host?.lowercased() else { - return nil - } - let port = url.port.map { ":\($0)" } ?? "" - return "\(scheme)://\(host)\(port)" - } - static func athClaim(for accessToken: String) -> String { let hash = SHA256.hash(data: Data(accessToken.utf8)) return Data(hash).base64URLEncodedString() diff --git a/Sources/CoreATProtocol/OAuth/IdentityResolver.swift b/Sources/CoreATProtocol/OAuth/IdentityResolver.swift index 2e086bf..02d7e1f 100644 --- a/Sources/CoreATProtocol/OAuth/IdentityResolver.swift +++ b/Sources/CoreATProtocol/OAuth/IdentityResolver.swift @@ -94,13 +94,13 @@ public struct IdentityResolver: Sendable { ) async throws -> Bool { let metadata = try await protectedResourceMetadata(for: pdsEndpoint) guard let authorizationServerURL = URL(string: authorizationServer), - let inputOrigin = normalizedOrigin(from: authorizationServerURL) else { + let inputOrigin = URLOrigin.normalized(authorizationServerURL) else { return false } let allowedOrigins: [String] = metadata.authorizationServers.compactMap { value in guard let url = URL(string: value) else { return nil } - return normalizedOrigin(from: url) + return URLOrigin.normalized(url) } return allowedOrigins.contains(inputOrigin) @@ -231,7 +231,7 @@ public struct IdentityResolver: Sendable { throw IdentityError.noAuthServerFound } guard let authServerURL = URL(string: authServer), - normalizedOrigin(from: authServerURL) != nil else { + URLOrigin.normalized(authServerURL) != nil else { throw IdentityError.noAuthServerFound } return authServer @@ -284,15 +284,6 @@ public struct IdentityResolver: Sendable { return nil } - private func normalizedOrigin(from url: URL) -> String? { - guard let scheme = url.scheme?.lowercased(), - let host = url.host?.lowercased() else { - return nil - } - 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 diff --git a/Sources/CoreATProtocol/OAuth/TokenValidator.swift b/Sources/CoreATProtocol/OAuth/TokenValidator.swift new file mode 100644 index 0000000..3b9f4c7 --- /dev/null +++ b/Sources/CoreATProtocol/OAuth/TokenValidator.swift @@ -0,0 +1,36 @@ +// +// TokenValidator.swift +// CoreATProtocol +// + +import Foundation +import OAuthenticator + +/// Validates the response payload from the AT Protocol token endpoint +/// against the requirements that apply to both initial login and refresh: +/// the token type must be `DPoP`, the `atproto` scope must be granted, and +/// — when the caller knows which DID they expected — the response's `sub` +/// must match. +/// +/// Issuer matching is *not* the validator's job. The login flow validates +/// the `iss` query parameter from the authorization callback (which the +/// validator never sees), and the refresh flow has no `iss` to validate +/// against because the auth-server URL is fixed by the time refresh runs. +struct TokenValidator: Sendable { + let expectedSubjectDID: String? + + func validate(_ response: OAuthTokenResponse) throws { + guard response.tokenType == "DPoP" else { + throw AuthenticatorError.dpopTokenExpected(response.tokenType) + } + guard response.scopes.contains("atproto") else { + throw ATProtoOAuthError.missingRequiredScope("atproto") + } + if let expectedSubjectDID, response.subject != expectedSubjectDID { + throw ATProtoOAuthError.subjectMismatch( + expected: expectedSubjectDID, + actual: response.subject + ) + } + } +} diff --git a/Sources/CoreATProtocol/OAuth/URLOrigin.swift b/Sources/CoreATProtocol/OAuth/URLOrigin.swift new file mode 100644 index 0000000..f3770a7 --- /dev/null +++ b/Sources/CoreATProtocol/OAuth/URLOrigin.swift @@ -0,0 +1,25 @@ +// +// URLOrigin.swift +// CoreATProtocol +// + +import Foundation + +/// Single source of truth for the "origin" representation used by the OAuth +/// layer to compare URLs that should be considered equivalent endpoints +/// (issuer matching, per-origin nonce caching, auth-server validation). +/// +/// The shape — `scheme://host[:port]` with both lowercased — matches RFC 6454 +/// and what AT Protocol metadata documents declare. Any divergence between +/// call sites would silently misclassify equivalent endpoints, so all +/// callers go through this one helper. +enum URLOrigin { + static func normalized(_ url: URL) -> String? { + guard let scheme = url.scheme?.lowercased(), + let host = url.host?.lowercased() else { + return nil + } + let port = url.port.map { ":\($0)" } ?? "" + return "\(scheme)://\(host)\(port)" + } +} diff --git a/Tests/CoreATProtocolTests/TokenValidatorTests.swift b/Tests/CoreATProtocolTests/TokenValidatorTests.swift new file mode 100644 index 0000000..9fb270d --- /dev/null +++ b/Tests/CoreATProtocolTests/TokenValidatorTests.swift @@ -0,0 +1,79 @@ +import Foundation +import Testing +import OAuthenticator +@testable import CoreATProtocol + +@Suite("TokenValidator") +struct TokenValidatorTests { + @Test("Valid response: DPoP token type, atproto scope, matching subject — no throw") + func successPath() throws { + let response = makeResponse() + let validator = TokenValidator(expectedSubjectDID: "did:plc:abc") + #expect(throws: Never.self) { + try validator.validate(response) + } + } + + @Test("Wrong token type throws AuthenticatorError.dpopTokenExpected") + func wrongTokenType() { + let response = makeResponse(tokenType: "Bearer") + let validator = TokenValidator(expectedSubjectDID: nil) + #expect(throws: AuthenticatorError.self) { + try validator.validate(response) + } + } + + @Test("Missing atproto scope throws ATProtoOAuthError.missingRequiredScope") + func missingAtprotoScope() { + let response = makeResponse(scope: "transition:generic") + let validator = TokenValidator(expectedSubjectDID: nil) + #expect(throws: ATProtoOAuthError.self) { + try validator.validate(response) + } + } + + @Test("Subject mismatch when expected DID is set throws ATProtoOAuthError.subjectMismatch") + func subjectMismatch() { + let response = makeResponse(subject: "did:plc:wrong") + let validator = TokenValidator(expectedSubjectDID: "did:plc:abc") + #expect(throws: ATProtoOAuthError.self) { + try validator.validate(response) + } + } + + @Test("Subject is not checked when expected DID is nil") + func subjectIgnoredWhenExpectedNil() throws { + let response = makeResponse(subject: "did:plc:anything") + let validator = TokenValidator(expectedSubjectDID: nil) + #expect(throws: Never.self) { + try validator.validate(response) + } + } + + @Test("Multi-scope response containing atproto passes") + func multiScopeWithAtprotoPasses() throws { + let response = makeResponse(scope: "atproto transition:generic transition:chat.bsky") + let validator = TokenValidator(expectedSubjectDID: nil) + #expect(throws: Never.self) { + try validator.validate(response) + } + } + + private func makeResponse( + accessToken: String = "access-token", + refreshToken: String? = "refresh-token", + subject: String = "did:plc:abc", + scope: String = "atproto transition:generic", + tokenType: String = "DPoP", + expiresIn: Int = 3600 + ) -> OAuthTokenResponse { + OAuthTokenResponse( + accessToken: accessToken, + refreshToken: refreshToken, + subject: subject, + scope: scope, + tokenType: tokenType, + expiresIn: expiresIn + ) + } +} diff --git a/update_auth.md b/update_auth.md index 9769a73..459799f 100644 --- a/update_auth.md +++ b/update_auth.md @@ -4,11 +4,11 @@ Goal: keep OAuthenticator as the OAuth 2.1 transport, but tighten the AT-Proto-s ## Status (2026-04-29) -- **Steps 1–2 are landed.** Per-origin DPoP nonce caching is live. CoreATProtocol's 66 tests pass; bskyKit and EffemKit rebuild cleanly against the local checkout. -- **Steps 3–5 are next.** They're independent of each other and any pair of them is a reasonable single-session chunk. +- **Steps 1–4 are landed.** Per-origin DPoP nonce caching, the shared `URLOrigin.normalized` helper, and the extracted `TokenValidator` are all live. CoreATProtocol's 72 tests pass; bskyKit and EffemKit rebuild cleanly against the local checkout. +- **Step 5 is next.** Resolver protocol + offline tests; additive, low risk. - **Step 6 remains gated** behind a SemVer-major bump. -See "Notes from the Steps 1+2 implementation" near the bottom for the deviations from the original plan. +See "Notes from the Steps 1+2 implementation" and "Notes from the Steps 3+4 implementation" near the bottom for deviations from the original plan. ## Inventory of what we have today @@ -197,7 +197,7 @@ Tests to update: - `DPoPStoreTests.swift` continues to test `DPoPNonceStore` for now. The new `DPoPSignerTests` covers the new path. - Add an integration test in `OAuthTests.swift` that drives a fake `URLResponseProvider`, sees a `DPoP-Nonce` header, and confirms the next outbound proof carries the nonce. -### Step 3 — Centralize URL canonicalization + origin helpers +### Step 3 — Centralize URL canonicalization + origin helpers — DONE The `htu` and origin logic now lives in `DPoPSigner` (steps 1–2). Two more places use it: - `ATProtoOAuth.normalizedOrigin(_:)` (line 901) — for issuer comparison. @@ -218,7 +218,7 @@ enum URLOrigin { Update three call sites (`DPoPSigner.origin`, `ATProtoOAuth.normalizedOrigin`, `IdentityResolver.normalizedOrigin`) to call `URLOrigin.normalized`. Internal helper, no public API change. -### Step 4 — Extract the token validator closure +### Step 4 — Extract the token validator closure — DONE `ATProtoOAuth.loginProvider` (line 759) inlines all the token-response validation: DPoP token type, `atproto` scope, issuer matches expected auth server, sub matches expected DID. Same checks duplicate in `refreshProvider` (line 834). @@ -325,8 +325,8 @@ After all of this lands, update `~/.claude/projects/-Users-rademaker-Developer-S 1. **PR 1**: Step 1 — `DPoPSigner` + tests. Standalone, no consumer changes. **DONE**, bundled with PR 2. 2. **PR 2**: Step 2 — wire `DPoPSigner` into `Networking.swift` + `ATProtoOAuth.swift`. Largest diff. Verify against Atprosphere + effem before merge. **DONE**. -3. **PR 3**: Steps 3 + 4 — URL canonicalization + token validator. Small refactor PR. **NEXT**. -4. **PR 4**: Step 5 — resolver protocol + offline tests. +3. **PR 3**: Steps 3 + 4 — URL canonicalization + token validator. Small refactor PR. **DONE**. +4. **PR 4**: Step 5 — resolver protocol + offline tests. **NEXT**. 5. **PR 5** (later, behind a SemVer bump): Step 6 — file split and deprecation removal. Steps 1–5 are non-breaking. Step 6 is breaking and can wait until you have another reason to bump the major version. @@ -339,3 +339,11 @@ Steps 1–5 are non-breaking. Step 6 is breaking and can wait until you have ano - **Dropped the integration test from Step 2.** The plan suggested an end-to-end test that drives `APRouterDelegate.didReceiveErrorResponse` then `intercept` and verifies the cached nonce flows through. Implementation revealed that any test touching `ATProtoSession.shared` races with other suites that call `await ATProtoSession.shared.reset()` (`NonceDetectionTests`, `RefreshLoginTests`, `DPoPTests`). Swift Testing's `.serialized` trait only orders within a suite, not across suites. Two paths are open here: (1) globally serialize all session-touching suites, or (2) make the session instance-based so tests can spin up isolated sessions. (2) is the long-term fix and is closer to what's contemplated in `APEnvironment.swift`'s "future major release will make it instance-based" comment. Until then, the per-origin caching is fully covered by the unit tests in `DPoPProofSignerTests`, and the wiring through `Networking.swift` is straightforward delegation (the kind of code production traffic exercises immediately). - **Verified consumers.** bskyKit and EffemKit were rebuilt against the local CoreATProtocol checkout via a temporary `.package(path:)` swap — both succeeded. Atprosphere and the effem iOS app are `.xcodeproj` consumers; their call sites were inspected and only touch preserved public APIs (`setDPoPPrivateKey(pem:)`, `ATProtoOAuth(config:storage:)`, `ATProtoOAuthConfig`, `ATProtoOAuthError`, `ATProtoSession.shared.host`, `ATProtoSession.shared.routerDelegate`). - **Updated test:** `nonceCacheBounded` was rewritten — the original draft asserted "≥ 1 of the 5 oldest origins was evicted," which was probabilistic since Swift dictionaries don't guarantee key ordering. The new assertion is the deterministic invariant: after 30 insertions into a 25-slot cache, exactly 25 entries survive. + +## Notes from the Steps 3+4 implementation + +- **Deleted the `normalizedOrigin` wrappers entirely** rather than having them delegate. `DPoPProofSigner.origin(for:)`, `ATProtoOAuth.normalizedOrigin(_:)`, and `IdentityResolver.normalizedOrigin(from:)` are gone; their five call sites now invoke `URLOrigin.normalized` directly. Wrappers that only forward to a one-liner are noise. +- **`TokenValidator` is in its own file** — `Sources/CoreATProtocol/OAuth/TokenValidator.swift`. The plan placed it as a private nested type in `ATProtoOAuth.swift`, but the file is already 970+ lines and the validator is unit-tested directly (`TokenValidatorTests`), so a separate file is cleaner. The struct is `internal` (not `private`) so tests reach it via `@testable import CoreATProtocol`. +- **`OAuthTokenResponse` was bumped from `private` to `internal` (and `Sendable`).** Tests construct it via the synthesized memberwise init — much simpler than building JSON and round-tripping through `Codable`. `OAuthTokenRequest` and `OAuthRefreshTokenRequest` stay `private`; nothing outside `ATProtoOAuth.swift` needs them. +- **Issuer validation stays inline in `loginProvider`, not in `TokenValidator`.** The plan implied both providers shared an issuer check, but they don't: the auth-callback flow validates the `iss` query parameter (which the validator never sees), and the refresh flow has no `iss` to validate against — the auth-server URL is fixed by the time refresh runs. The validator handles only the response-payload checks (token type, scope, sub) that genuinely duplicated. Net change: ~10 lines of duplicated guards collapsed to two `validator.validate(_:)` calls. +- **Tests added:** `TokenValidatorTests` covers the success path, wrong token type, missing `atproto` scope, sub mismatch, sub passthrough when expected DID is nil, and a multi-scope success case. Six tests, all sub-millisecond. Total suite is now 72 tests.