From 4a1378f8d6e3acd7179a51da47623944010293cd Mon Sep 17 00:00:00 2001 From: Arkadiusz Fal Date: Mon, 31 Aug 2026 23:59:37 +0200 Subject: [PATCH] Close the two ways an adopted secret could still be uploaded MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of the silent migration found two paths that defeat the point of writing the adopted copy device-local. The marker is written after the copy it describes, so a launch that ends in between leaves an adopted credential nothing claims — and adoption never retries, because the key is no longer empty. Enabling iCloud sync then rewrites it as synchronizable. The resync now also recognizes the value: a scoped credential identical to the unscoped item it could only have come from is an adopted copy whether or not a marker says so. The opposite failure is a marker that outlives what it described. Signing in again replaces the credential with one the user proved they hold, but the marker stayed, so enabling sync afterwards skipped the account and left every other device showing a login it never receives. Storing a credential now retires the marker. Also corrects four comments that still describe adoption as something the user is asked about, and states plainly what the legacy-deletion conditions do and do not prove now that no one is asked. --- Yattee/Core/InstancesManager.swift | 4 +- .../AccountKeychainCredentials.swift | 28 +++++- .../CredentialAdoptionMarker.swift | 11 ++- .../Credentials/CredentialIdentityScope.swift | 6 +- .../Credentials/KeychainCredentialStore.swift | 11 ++- YatteeTests/AccountCredentialsTests.swift | 91 +++++++++++++++++++ 6 files changed, 136 insertions(+), 15 deletions(-) diff --git a/Yattee/Core/InstancesManager.swift b/Yattee/Core/InstancesManager.swift index b0d4d418..ee35ed0e 100644 --- a/Yattee/Core/InstancesManager.swift +++ b/Yattee/Core/InstancesManager.swift @@ -98,8 +98,8 @@ final class InstancesManager { // Instances arriving from iCloud are as much a change as a local edit. // Without this, a legacy authenticated instance delivered here never - // reaches account migration — the account, and the action to adopt its - // saved login, would appear only after an unrelated edit or a relaunch. + // reaches account migration — the account, and the adoption of its + // saved login, would happen only after an unrelated edit or a relaunch. NotificationCenter.default.post(name: .instancesDidChange, object: nil) } diff --git a/Yattee/Services/Credentials/AccountKeychainCredentials.swift b/Yattee/Services/Credentials/AccountKeychainCredentials.swift index de4a5512..ad822424 100644 --- a/Yattee/Services/Credentials/AccountKeychainCredentials.swift +++ b/Yattee/Services/Credentials/AccountKeychainCredentials.swift @@ -92,6 +92,14 @@ final class AccountKeychainCredentials { loginsByScope[scope, default: []].insert(account.id) + // Whatever was adopted here is gone, replaced by a login the user just + // proved they hold. Leaving the marker would keep this account out of + // the resync that follows the iCloud toggle, so the new credential + // would stay on this device and every other one would show an account + // it never receives a login for. It would also let a deletion retire an + // unscoped item this account no longer has anything to do with. + markers.removeMarker(service: store.service, instanceID: account.instanceID, scope: scope) + LoggingService.shared.info( "Stored \(credentialNoun)", category: .keychain, @@ -131,10 +139,22 @@ final class AccountKeychainCredentials { let adopted = markers.adoptedAccountIDs(service: store.service, scope: scope) for accountID in loginsByScope[scope] ?? [] where !adopted.contains(accountID) { - rewrite( - key: KeychainCredentialStore.credentialKey(scope: scope, accountID: accountID), - synchronizable: syncEnabled - ) + let credentialKey = KeychainCredentialStore.credentialKey(scope: scope, accountID: accountID) + + // The marker is written *after* the copy it describes, so a launch + // that ends in between leaves an adopted credential no marker will + // ever claim — and adoption will not retry, because the key is no + // longer empty. Recognising the value itself closes that gap: a + // scoped credential identical to the unscoped item it could only + // have come from is an adopted copy, marker or not. Migrated + // accounts reuse their instance's UUID, which is what the legacy + // key is made of, and they are the only accounts allowed to adopt. + if let stored = store.value(forKey: credentialKey), + stored == store.value(forKey: KeychainCredentialStore.legacyKey(instanceID: accountID)) { + continue + } + + rewrite(key: credentialKey, synchronizable: syncEnabled) rewrite( key: KeychainCredentialStore.usernameKey(scope: scope, accountID: accountID), synchronizable: syncEnabled diff --git a/Yattee/Services/Credentials/CredentialAdoptionMarker.swift b/Yattee/Services/Credentials/CredentialAdoptionMarker.swift index 9c0afc2e..0ce00fa8 100644 --- a/Yattee/Services/Credentials/CredentialAdoptionMarker.swift +++ b/Yattee/Services/Credentials/CredentialAdoptionMarker.swift @@ -60,8 +60,10 @@ final class CredentialAdoptionMarkerStore { /// Whether this identity has already adopted the legacy item for an instance. /// - /// Once true, the app stops offering adoption and stops reading the legacy - /// key entirely for that account. + /// Once true, the app stops adopting and stops reading the legacy key + /// entirely for that account. Cleared when the user signs in again: the + /// credential is then one they proved they hold, and nothing about the + /// adopted copy applies to it any more. func hasAdopted(service: String, instanceID: UUID, scope: String) -> Bool { marker(service: service, instanceID: instanceID, scope: scope) != nil } @@ -121,6 +123,11 @@ final class CredentialAdoptionMarkerStore { /// /// 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. + /// Since adoption became automatic this proves only that the device took + /// the secret, not that the identity signed in now originally owned it. + /// Deleting anyway is deliberate: the user is removing this login from + /// this device, which is exactly what the pre-revamp release did to the + /// same item on logout. Condition 4 is what carries the real weight now. /// 2. The marker belongs to the current identity — Apple account A's marker /// cannot authorize a deletion while B is signed in. /// 3. The marker belongs to the account being deleted. An instance can hold diff --git a/Yattee/Services/Credentials/CredentialIdentityScope.swift b/Yattee/Services/Credentials/CredentialIdentityScope.swift index 7fd350d3..325dd755 100644 --- a/Yattee/Services/Credentials/CredentialIdentityScope.swift +++ b/Yattee/Services/Credentials/CredentialIdentityScope.swift @@ -24,8 +24,10 @@ import Foundation /// keeps `kSecAttrSynchronizable = true` and still matches a strict /// `== true` query with **no** Apple account signed in — so the attribute /// cannot be used to tell "belongs to the current account" from "left over -/// from the previous one". Scoping the key is the only reliable defence, and -/// legacy items are never adopted automatically (see +/// from the previous one". Scoping the key is the only reliable defence. +/// A pre-revamp item is adopted into a scoped key automatically, but only as a +/// device-local copy, so an unproven secret never reaches the current +/// identity's iCloud Keychain (see /// ``AccountKeychainCredentials/adoptLegacyCredential(for:)``). /// /// ### Why the CloudKit user record, not the ubiquity token diff --git a/Yattee/Services/Credentials/KeychainCredentialStore.swift b/Yattee/Services/Credentials/KeychainCredentialStore.swift index 3e8749d5..98dc8b96 100644 --- a/Yattee/Services/Credentials/KeychainCredentialStore.swift +++ b/Yattee/Services/Credentials/KeychainCredentialStore.swift @@ -39,8 +39,9 @@ struct KeychainCredentialStore: Sendable { /// The pre-revamp key: a bare instance UUID, with no identity scope. /// - /// Only the adoption path may read this, and only when the user explicitly - /// asks for it. + /// Only the adoption path may read this. Ordinary credential reads never + /// fall back to it — an implicit read-through would leave no record of what + /// was taken, and nothing to bound a later deletion with. static func legacyKey(instanceID: UUID) -> String { instanceID.uuidString } @@ -75,9 +76,9 @@ struct KeychainCredentialStore: Sendable { /// 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. + /// Used by legacy adoption: iCloud Keychain may deliver the real scoped + /// credential while the migration is deciding to copy the older one, and a + /// replacing write would silently overwrite the newer secret. @discardableResult func addIfAbsent(_ value: String, forKey key: String, synchronizable: Bool) -> OSStatus { let query: [String: Any] = [ diff --git a/YatteeTests/AccountCredentialsTests.swift b/YatteeTests/AccountCredentialsTests.swift index 416d029d..ad11b1b2 100644 --- a/YatteeTests/AccountCredentialsTests.swift +++ b/YatteeTests/AccountCredentialsTests.swift @@ -281,6 +281,97 @@ struct LegacyCredentialAdoptionTests { store.delete(key: KeychainCredentialStore.legacyKey(instanceID: instanceID)) } + @Test("An adopted credential stays local even if its marker was never written") + func resyncRecognizesAnAdoptedCopyWithoutItsMarker() { + let service = makeService() + let instanceID = UUID() + let account = Account.migrated(from: instanceID) + seedLegacyItem(service: service, instanceID: instanceID, value: "legacy-secret") + + let settings = SettingsManager() + let previousSync = settings.iCloudSyncEnabled + let previousInstances = settings.syncInstances + settings.iCloudSyncEnabled = false + settings.syncInstances = false + defer { + settings.iCloudSyncEnabled = previousSync + settings.syncInstances = previousInstances + } + + let store = KeychainCredentialStore(service: service) + let key = KeychainCredentialStore.credentialKey(scope: "identity-A", accountID: account.id) + // Exactly the state left behind when the app dies between the adoption + // copy and the marker that describes it: the scoped item exists, no + // marker claims it, and adoption will not retry because the key is + // occupied. + store.set("legacy-secret", forKey: key, synchronizable: false) + + let credentials = makeCredentials( + service: service, + scope: "identity-A", + markers: makeMarkers(), + settingsManager: settings + ) + #expect(credentials.isLoggedIn(for: account) == true) + + settings.iCloudSyncEnabled = true + settings.syncInstances = true + credentials.resyncCredentialsWithCurrentSetting() + + #expect(store.isSynchronizable(key: key) == false) + + credentials.deleteCredential(for: account) + store.delete(key: KeychainCredentialStore.legacyKey(instanceID: instanceID)) + } + + @Test("Signing in again lets the new credential sync") + func signingInAgainClearsTheAdoptionMarker() { + let service = makeService() + let instanceID = UUID() + let account = Account.migrated(from: instanceID) + seedLegacyItem(service: service, instanceID: instanceID, value: "legacy-secret") + + let settings = SettingsManager() + let previousSync = settings.iCloudSyncEnabled + let previousInstances = settings.syncInstances + settings.iCloudSyncEnabled = false + settings.syncInstances = false + defer { + settings.iCloudSyncEnabled = previousSync + settings.syncInstances = previousInstances + } + + let markers = makeMarkers() + let credentials = makeCredentials( + service: service, + scope: "identity-A", + markers: markers, + settingsManager: settings + ) + #expect(credentials.adoptLegacyCredential(for: account) == true) + + // The user signs in again while sync is off, so the replacement is + // stored locally like any other credential. + credentials.setCredential("freshly-typed-secret", for: account) + #expect(markers.hasAdopted(service: service, instanceID: instanceID, scope: "identity-A") == false) + + // Turning sync on must now carry it to the other devices — they are + // showing an account whose credential never arrives. + settings.iCloudSyncEnabled = true + settings.syncInstances = true + credentials.resyncCredentialsWithCurrentSetting() + + let store = KeychainCredentialStore(service: service) + #expect( + store.isSynchronizable( + key: KeychainCredentialStore.credentialKey(scope: "identity-A", accountID: account.id) + ) == true + ) + + credentials.deleteCredential(for: account) + store.delete(key: KeychainCredentialStore.legacyKey(instanceID: instanceID)) + } + @Test("A login the user added themselves never inherits the legacy item") func onlyTheMigratedAccountAdopts() { let service = makeService()