Mirrored_Repos/yattee
1
0
Fork 0
mirror of https://github.com/yattee/yattee.git synced 2026-10-23 21:22:53 +00:00
Code Issues Packages Projects Releases Wiki Activity

Close identity and adoption holes in credential scoping

Browse Source 
Four issues from an external review of the previous commit.

The scope was resolved once at startup, so signing out or switching Apple
account while the app kept running left it pinned to the old identity -
reads and writes would then reach the previous account's retained
credentials, which is the exact leak scoping exists to prevent.
CredentialIdentityScope now observes NSUbiquityIdentityDidChange and
re-resolves.

Legacy deletion authority checked only the instance and the identity, not
the account. On an instance with several accounts, deleting any of them
satisfied the marker lookup and could retire the legacy item another
account had adopted, logging out devices still on the previous release.
The marker's accountID is now required to match.

A first-time resolution that failed (launched offline, CloudKit
unreachable) was never retried, disabling credentials for the rest of the
process. resolve() now retries with backoff, and resolveIfNeeded() runs on
foreground activation.

Adoption re-checked nothing at execution time, so a scoped credential
delivered by iCloud Keychain between rendering the offer and tapping it
would be overwritten with the older legacy secret. Adoption now re-verifies
eligibility and writes with a non-replacing add, leaving a duplicate
strictly alone.

Adds regression tests for the sibling-account deletion and the adoption
race.
This commit is contained in:
Arkadiusz Fal
2026-08-23 23:39:06 +02:00
parent 9fd0a71035
commit c4053d5100
6 changed files with 215 additions and 30 deletions
Download Patch File Download Diff File Expand all files Collapse all files

37
Yattee/Services/Credentials/AccountKeychainCredentials.swift
View File

@@ -186,20 +186,46 @@ final class AccountKeychainCredentials {
@discardableResult
func adoptLegacyCredential(for account: Account) -> Bool {
guard let scope = currentScope() else { return false }
guard !markers.hasAdopted(instanceID: account.instanceID, scope: scope) else { return false }
guard let legacy = legacyCredential(for: account) else { return false }
// Eligibility is re-checked here, not just when the offer was rendered:
// iCloud Keychain may have delivered the real credential in between, and
// replacing it with the older legacy secret would be a silent downgrade.
guard credential(for: account) == nil else {
LoggingService.shared.info(
"Skipping legacy \(credentialNoun) adoption — a scoped credential already arrived",
category: .keychain,
details: "accountID=\(account.id)"
)
return false
}
let key = KeychainCredentialStore.credentialKey(scope: scope, accountID: account.id)
let status = store.set(legacy, forKey: key, synchronizable: shouldSyncToiCloud)
let status = store.addIfAbsent(legacy, forKey: key, synchronizable: shouldSyncToiCloud)
// A duplicate means a scoped credential landed between the check above
// and this write — leave it strictly alone, it is newer than the legacy
// secret.
guard status == errSecSuccess else {
LoggingService.shared.error(
"Legacy \(credentialNoun) adoption did not write",
category: .keychain,
details: "accountID=\(account.id), status=\(store.describe(status))"
)
return false
}
// The marker is only written after the scoped copy reads back: a marker
// for a credential that never landed would suppress the adoption offer
// and strand the user with no way to sign in short of retyping.
guard status == errSecSuccess, store.value(forKey: key) == legacy else {
guard store.value(forKey: key) == legacy else {
LoggingService.shared.error(
"Legacy \(credentialNoun) adoption failed verification",
category: .keychain,
details: "accountID=\(account.id), status=\(store.describe(status))"
details: "accountID=\(account.id)"
)
// Safe to remove: this write is the item that just landed.
store.delete(key: key)
return false
}
@@ -229,13 +255,14 @@ final class AccountKeychainCredentials {
guard markers.canDeleteLegacyItem(
instanceID: account.instanceID,
accountID: account.id,
scope: scope,
currentLegacyValue: currentValue
) else {
LoggingService.shared.info(
"Leaving the legacy \(credentialNoun) in place — not this identity's to delete",
"Leaving the legacy \(credentialNoun) in place — not this account's to delete",
category: .keychain,
details: "instanceID=\(account.instanceID)"
details: "instanceID=\(account.instanceID), accountID=\(account.id)"
)
return
}

18
Yattee/Services/Credentials/CredentialAdoptionMarker.swift
View File

@@ -87,23 +87,28 @@ final class CredentialAdoptionMarkerStore {
// MARK: - Legacy deletion authority
/// Whether an account deletion may also delete the **unscoped** legacy item.
/// Whether deleting an account may also delete the **unscoped** legacy item.
///
/// Three conditions, all required:
/// Four conditions, all required:
///
/// 1. This device has a marker for the instance — no marker means this
/// device never adopted anything, so the legacy item is not ours to touch.
/// 2. The marker belongs to the current identity — Apple account A's marker
/// cannot authorize a deletion while B is signed in.
/// 3. The legacy item still holds the exact secret that was adopted — if it
/// has since been replaced (B signed in again on an older release, and
/// that release rewrote the unscoped key), it is someone else's live
/// login and must survive.
/// 3. The marker belongs to the account being deleted. An instance can hold
/// several accounts, and only the one that actually adopted the legacy
/// secret may retire it — deleting a *sibling* account must leave it
/// alone.
/// 4. The legacy item still holds the exact secret that was adopted — if it
/// has since been replaced (someone signed in again on an older release,
/// and that release rewrote the unscoped key), it is a live login of
/// theirs and must survive.
///
/// Deleting a synchronizable item propagates everywhere, so a wrong answer
/// here logs other devices out.
func canDeleteLegacyItem(
instanceID: UUID,
accountID: UUID,
scope: String,
currentLegacyValue: String?
) -> Bool {
@@ -111,6 +116,7 @@ final class CredentialAdoptionMarkerStore {
$0.instanceID == instanceID && $0.identityScope == scope
}) else { return false }
guard marker.accountID == accountID else { return false }
guard let currentLegacyValue else { return false }
return fingerprint(of: currentLegacyValue) == marker.secretFingerprint

114
Yattee/Services/Credentials/CredentialIdentityScope.swift
View File

@@ -58,9 +58,18 @@ final class CredentialIdentityScope {
private let defaults: UserDefaults
private let resolveUserRecordID: @Sendable () async -> String?
/// Written only on the main actor; read in `deinit`, which runs after the
/// last reference is gone and so cannot race with it.
private nonisolated(unsafe) var identityChangeObserver: NSObjectProtocol?
private static let cacheKey = "credentialIdentityScopeCache"
/// Attempts made for the Apple account currently being resolved. Reset
/// whenever the identity changes.
private var resolutionAttempts = 0
private var isResolving = false
private static let maxResolutionAttempts = 4
/// The resolved scope, or `nil` while unresolved.
var current: String? {
if case let .resolved(scope) = state { return scope }
@@ -76,6 +85,37 @@ final class CredentialIdentityScope {
self.defaults = defaults
self.resolveUserRecordID = resolveUserRecordID
adoptCurrentIdentity()
observeIdentityChanges()
}
deinit {
if let identityChangeObserver {
NotificationCenter.default.removeObserver(identityChangeObserver)
}
}
/// Re-scopes as soon as the Apple account changes under a running app.
///
/// Without this the scope would stay pinned to whichever account was
/// signed in at launch, and a sign-out or switch mid-session would let
/// reads and writes reach the previous account's retained credentials —
/// the exact leak scoping exists to prevent.
private func observeIdentityChanges() {
identityChangeObserver = NotificationCenter.default.addObserver(
forName: .NSUbiquityIdentityDidChange,
object: nil,
queue: .main
) { [weak self] _ in
MainActor.assumeIsolated {
guard let self else { return }
LoggingService.shared.info(
"Apple account changed — re-resolving the credential identity scope",
category: .keychain
)
self.adoptCurrentIdentity()
Task { await self.resolve() }
}
}
}
/// Test seam: a scope pinned up front, never consulting CloudKit.
@@ -96,6 +136,9 @@ final class CredentialIdentityScope {
/// closed shows up here, before any CloudKit request could report it.
func adoptCurrentIdentity() {
let current = CloudKitIdentity.ubiquityFingerprint
if current != fingerprint {
resolutionAttempts = 0
}
fingerprint = current
guard CloudKitAvailability.isAvailable else {
@@ -127,34 +170,67 @@ final class CredentialIdentityScope {
/// rather than "not signed in" — callers must not treat that as a missing
/// credential.
func resolve() async {
guard !isResolved else { return }
guard !isResolved, !isResolving else { return }
// Captured before the await so a switch mid-flight can be detected:
// a record ID fetched under account A must never be cached for B.
isResolving = true
defer { isResolving = false }
// Captured before the first await so a switch mid-flight can be
// detected: a record ID fetched under account A must never be cached
// for B.
let requested = fingerprint
guard let recordID = await resolveUserRecordID() else { return }
while resolutionAttempts < Self.maxResolutionAttempts {
resolutionAttempts += 1
guard CloudKitIdentity.ubiquityFingerprint == requested, fingerprint == requested else {
LoggingService.shared.info(
"Discarding credential scope resolved for a previous Apple account",
category: .keychain
)
return
if let recordID = await resolveUserRecordID() {
guard CloudKitIdentity.ubiquityFingerprint == requested, fingerprint == requested else {
LoggingService.shared.info(
"Discarding credential scope resolved for a previous Apple account",
category: .keychain
)
return
}
if let requested {
cache(recordID, for: requested)
}
state = .resolved(recordID)
LoggingService.shared.info(
"Resolved credential identity scope",
category: .keychain,
details: "fingerprint=\(requested ?? "none"), attempt=\(resolutionAttempts)"
)
return
}
guard resolutionAttempts < Self.maxResolutionAttempts else { break }
try? await Task.sleep(for: .seconds(pow(2.0, Double(resolutionAttempts))))
guard fingerprint == requested else { return }
}
if let requested {
cache(recordID, for: requested)
}
state = .resolved(recordID)
LoggingService.shared.info(
"Resolved credential identity scope",
category: .keychain,
details: "fingerprint=\(requested ?? "none")"
// Every credential read is disabled until this succeeds, so failing
// quietly would look exactly like "signed out". Foreground activation
// retries via resolveIfNeeded().
LoggingService.shared.warning(
"Could not resolve the credential identity scope — credentials stay unavailable until it does",
category: .keychain
)
}
/// Retries a resolution that previously ran out of attempts.
///
/// Called when the app returns to the foreground: a device that launched
/// offline would otherwise keep credentials unavailable for the entire
/// process lifetime, long after connectivity returned.
func resolveIfNeeded() async {
guard !isResolved else { return }
resolutionAttempts = 0
await resolve()
}
// MARK: - Cache
private func cachedScope(for fingerprint: String) -> String? {

19
Yattee/Services/Credentials/KeychainCredentialStore.swift
View File

@@ -72,6 +72,25 @@ struct KeychainCredentialStore: Sendable {
return value
}
/// Writes a value **only if the key is currently empty**, returning
/// `errSecDuplicateItem` otherwise.
///
/// Used by legacy adoption: between rendering the offer and the user
/// tapping it, iCloud Keychain may deliver the real scoped credential, and
/// a replacing write would silently overwrite it with the older secret.
@discardableResult
func addIfAbsent(_ value: String, forKey key: String, synchronizable: Bool) -> OSStatus {
let query: [String: Any] = [
kSecClass as String: kSecClassGenericPassword,
kSecAttrService as String: service,
kSecAttrAccount as String: key,
kSecAttrSynchronizable as String: synchronizable,
kSecValueData as String: Data(value.utf8)
]
return SecItemAdd(query as CFDictionary, nil)
}
/// Writes a value, replacing any existing item in either synchronizable form.
@discardableResult
func set(_ value: String, forKey key: String, synchronizable: Bool) -> OSStatus {

12
Yattee/YatteeApp.swift
View File

@@ -288,6 +288,12 @@ struct YatteeApp: App {
// Refresh media source password status (Keychain state can change in background)
appEnvironment.mediaSourcesManager.refreshPasswordStoredStatus()
// Retry a credential identity scope that never resolved (e.g.
// launched offline) - account credentials stay unavailable until it does
Task {
await appEnvironment.credentialIdentityScope.resolveIfNeeded()
}
// Validate subscription account (auto-correct if invalid)
appEnvironment.subscriptionAccountValidator.validateAndCorrectIfNeeded()
SubscriptionFeedCache.shared.warmIfNeeded(using: appEnvironment)
@@ -335,6 +341,12 @@ struct YatteeApp: App {
// Refresh media source password status (Keychain state can change in background)
appEnvironment.mediaSourcesManager.refreshPasswordStoredStatus()
// Retry a credential identity scope that never resolved (e.g.
// launched offline) - account credentials stay unavailable until it does
Task {
await appEnvironment.credentialIdentityScope.resolveIfNeeded()
}
// Validate subscription account (auto-correct if invalid)
appEnvironment.subscriptionAccountValidator.validateAndCorrectIfNeeded()
SubscriptionFeedCache.shared.warmIfNeeded(using: appEnvironment)

45
YatteeTests/AccountCredentialsTests.swift
View File

@@ -253,6 +253,51 @@ struct LegacyCredentialAdoptionTests {
identityA.deleteCredential(for: account)
}
@Test("Deleting a sibling account leaves the adopter's legacy item alone")
func legacyDeletionRequiresAccountMatch() {
let service = makeService()
let markers = makeMarkers()
let instanceID = UUID()
let adopter = Account.migrated(from: instanceID)
// A second login on the same instance — a fresh UUID, not the migrated one.
let sibling = Account(instanceID: instanceID)
seedLegacyItem(service: service, instanceID: instanceID, value: "legacy-secret")
let credentials = makeCredentials(service: service, scope: "identity-A", markers: markers)
#expect(credentials.adoptLegacyCredential(for: adopter) == true)
// Removing the sibling must not retire the secret the migrated account
// adopted — old-release devices are still using it.
credentials.deleteLegacyCredentialIfOwned(for: sibling)
#expect(legacyValue(service: service, instanceID: instanceID) == "legacy-secret")
credentials.deleteLegacyCredentialIfOwned(for: adopter)
#expect(legacyValue(service: service, instanceID: instanceID) == nil)
credentials.deleteCredential(for: adopter)
}
@Test("Adoption does not overwrite a scoped credential that arrived first")
func adoptionDoesNotClobberArrivedCredential() {
let service = makeService()
let instanceID = UUID()
let account = Account.migrated(from: instanceID)
seedLegacyItem(service: service, instanceID: instanceID, value: "legacy-secret")
let credentials = makeCredentials(service: service, scope: "identity-A", markers: makeMarkers())
// iCloud Keychain delivers the real credential between the offer being
// rendered and the user tapping it.
credentials.setCredential("synced-secret", for: account)
#expect(credentials.hasAdoptableLegacyCredential(for: account) == false)
#expect(credentials.adoptLegacyCredential(for: account) == false)
#expect(credentials.credential(for: account) == "synced-secret")
credentials.deleteCredential(for: account)
KeychainCredentialStore(service: service)
.delete(key: KeychainCredentialStore.legacyKey(instanceID: instanceID))
}
@Test("A legacy item replaced by another login survives the adopter's deletion")
func legacyDeletionRequiresFingerprintMatch() {
let service = makeService()