From 81aa47196fb370560dd9f5c43d0ee6c94707622c Mon Sep 17 00:00:00 2001 From: Arkadiusz Fal Date: Thu, 27 Aug 2026 23:57:28 +0200 Subject: [PATCH] Stop the legacy validator from overruling a local selection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Local data is always a valid context. The legacy validator predates that: it treats local as unavailable without a Yattee Server, which is a feed concern the feed reports itself — and since the correction now moves the data context with it, that would have quietly put users on somebody's backend account, overwriting a local choice made on another device. Stale feed responses no longer touch anything. Rejecting only the videos still let a request from a login the cache had moved off overwrite the new one's loading state, error state and pagination — replacement requests are allowed to overlap, so both were in flight. The unreachable retry timers are cancelled when the namespace reloads. A migrated account reuses its instance's UUID, so the same ID can exist under the next Apple account, and an old timer would declare that account reachable on the strength of a verdict about someone else's. Rewriting a credential's sync flag is skipped when it already matches and restores the original if the re-add fails. Changing that flag means delete-then-add, and the delete propagates to every device — turning sync on should never be able to log the user out. --- Yattee/Core/AccountsManager.swift | 6 +++ .../AccountKeychainCredentials.swift | 46 +++++++++++++------ .../Credentials/KeychainCredentialStore.swift | 24 ++++++++++ Yattee/Services/SubscriptionFeedCache.swift | 7 +++ .../SubscriptionAccountValidator.swift | 13 ++++++ 5 files changed, 83 insertions(+), 13 deletions(-) diff --git a/Yattee/Core/AccountsManager.swift b/Yattee/Core/AccountsManager.swift index bf2060c5..f815ecce 100644 --- a/Yattee/Core/AccountsManager.swift +++ b/Yattee/Core/AccountsManager.swift @@ -789,6 +789,12 @@ final class AccountsManager { func reloadForCurrentIdentity() { accounts = [] accountStatuses = [:] + // Their accounts belong to the namespace being left. A migrated account + // reuses its instance's UUID, so the same ID can exist under the new + // identity — and an old timer would then declare *that* account + // reachable again on the strength of a verdict about someone else's. + for expiry in unreachableExpiries.values { expiry.cancel() } + unreachableExpiries = [:] storedContext = StoredDataContext(context: .local) migratedInstanceIDs = [] pendingUploads = [] diff --git a/Yattee/Services/Credentials/AccountKeychainCredentials.swift b/Yattee/Services/Credentials/AccountKeychainCredentials.swift index 8f26ee01..6d492dda 100644 --- a/Yattee/Services/Credentials/AccountKeychainCredentials.swift +++ b/Yattee/Services/Credentials/AccountKeychainCredentials.swift @@ -121,22 +121,42 @@ final class AccountKeychainCredentials { func resyncCredentialsWithCurrentSetting() { guard let scope = identityScope.current else { return } + let syncEnabled = shouldSyncToiCloud + for accountID in loginsByScope[scope] ?? [] { - let credentialKey = KeychainCredentialStore.credentialKey(scope: scope, accountID: accountID) - let usernameKey = KeychainCredentialStore.usernameKey(scope: scope, accountID: accountID) - let syncEnabled = shouldSyncToiCloud - - if let credential = store.value(forKey: credentialKey) { - _ = store.set(credential, forKey: credentialKey, synchronizable: syncEnabled) - } - if let username = store.value(forKey: usernameKey) { - _ = store.set(username, forKey: usernameKey, synchronizable: syncEnabled) - } + rewrite( + key: KeychainCredentialStore.credentialKey(scope: scope, accountID: accountID), + synchronizable: syncEnabled + ) + rewrite( + key: KeychainCredentialStore.usernameKey(scope: scope, accountID: accountID), + synchronizable: syncEnabled + ) } + } - LoggingService.shared.info( - "Rewrote stored \(credentialNoun)s with the current iCloud sync setting", - category: .keychain + /// Rewrites one item so it carries the wanted sync flag. + /// + /// Skipped when the flag already matches: the only way to change it is to + /// delete and re-add, which risks the login for no reason — and deleting a + /// synchronizable item propagates to every device. If the re-add does fail, + /// the original is put back the way it was rather than left missing. + private func rewrite(key: String, synchronizable: Bool) { + guard let value = store.value(forKey: key) else { return } + + let current = store.isSynchronizable(key: key) + guard current != synchronizable else { return } + + let status = store.set(value, forKey: key, synchronizable: synchronizable) + guard status != errSecSuccess else { return } + + let restored = store.set(value, forKey: key, synchronizable: current ?? false) + LoggingService.shared.error( + restored == errSecSuccess + ? "Could not change how a \(credentialNoun) syncs — left it as it was" + : "Could not change how a \(credentialNoun) syncs, and could not restore it", + category: .keychain, + details: "status=\(store.describe(status))" ) } diff --git a/Yattee/Services/Credentials/KeychainCredentialStore.swift b/Yattee/Services/Credentials/KeychainCredentialStore.swift index 0f80b55e..3e8749d5 100644 --- a/Yattee/Services/Credentials/KeychainCredentialStore.swift +++ b/Yattee/Services/Credentials/KeychainCredentialStore.swift @@ -91,6 +91,30 @@ struct KeychainCredentialStore: Sendable { return SecItemAdd(query as CFDictionary, nil) } + /// Whether the stored item is marked as syncing through iCloud Keychain. + /// + /// `nil` when there is no item under this key. The attribute records what + /// was requested when the item was written — that is exactly the question + /// here, and deliberately not "does this item belong to the current Apple + /// account", which it cannot answer (measured). + func isSynchronizable(key: String) -> Bool? { + for synchronizable in [true, false] { + let query: [String: Any] = [ + kSecClass as String: kSecClassGenericPassword, + kSecAttrService as String: service, + kSecAttrAccount as String: key, + kSecAttrSynchronizable as String: synchronizable, + kSecMatchLimit as String: kSecMatchLimitOne + ] + + if SecItemCopyMatching(query as CFDictionary, nil) == errSecSuccess { + return synchronizable + } + } + + return nil + } + /// Writes a value, replacing any existing item in either synchronizable form. @discardableResult func set(_ value: String, forKey key: String, synchronizable: Bool) -> OSStatus { diff --git a/Yattee/Services/SubscriptionFeedCache.swift b/Yattee/Services/SubscriptionFeedCache.swift index 3c845211..18167f21 100644 --- a/Yattee/Services/SubscriptionFeedCache.swift +++ b/Yattee/Services/SubscriptionFeedCache.swift @@ -409,6 +409,7 @@ final class SubscriptionFeedCache { do { let pipedAPI = PipedAPI(httpClient: appEnvironment.httpClient) let feedVideos = try await pipedAPI.feed(instance: instance, authToken: authToken) + guard isCurrentAccountsResult(generation) else { return } LoggingService.shared.info("Piped feed: Received \(feedVideos.count) videos", category: .general) @@ -418,6 +419,7 @@ final class SubscriptionFeedCache { feedLoadState = .ready } catch { LoggingService.shared.error("Failed to fetch Piped feed: \(error.localizedDescription)", category: .general) + guard isCurrentAccountsResult(generation) else { return } feedLoadState = .error(.networkError(error.localizedDescription)) } @@ -465,6 +467,8 @@ final class SubscriptionFeedCache { maxResults: 50 ) + guard isCurrentAccountsResult(generation) else { return } + let feedVideos = response.videos LoggingService.shared.info( "Invidious feed: Received \(feedVideos.count) videos (page \(currentPage))", @@ -488,6 +492,7 @@ final class SubscriptionFeedCache { "Failed to fetch Invidious feed: \(error.localizedDescription)", category: .general ) + guard isCurrentAccountsResult(generation) else { return } feedLoadState = .error(.networkError(error.localizedDescription)) } @@ -519,6 +524,8 @@ final class SubscriptionFeedCache { maxResults: 50 ) + guard isCurrentAccountsResult(generation) else { return } + let feedVideos = response.videos LoggingService.shared.info( "Invidious feed: Loaded page \(nextPage), received \(feedVideos.count) videos", diff --git a/Yattee/Services/Subscriptions/SubscriptionAccountValidator.swift b/Yattee/Services/Subscriptions/SubscriptionAccountValidator.swift index 43733b53..95823446 100644 --- a/Yattee/Services/Subscriptions/SubscriptionAccountValidator.swift +++ b/Yattee/Services/Subscriptions/SubscriptionAccountValidator.swift @@ -127,6 +127,19 @@ final class SubscriptionAccountValidator { return } + // Local is always a valid context. This validator predates that: it + // treats local as unavailable unless a Yattee Server is configured, + // which is a *feed* concern — the feed says so itself — not a reason to + // move the user onto somebody's backend account, or to overwrite a + // local choice synced from another device. + if accountsManager?.currentContext.isLocal == true { + LoggingService.shared.debug( + "Leaving the local selection alone — local data is always available", + category: .general + ) + return + } + // A selection whose account still exists is never corrected here. This // validator predates account-scoped credentials and reads "not signed // in" as "not available", which is now routinely temporary — an