Skip to content

fix(keychain): remove destructive migration + Apple-correct API refactor#972

Merged
datlechin merged 2 commits intomainfrom
fix/keychain-credential-loss
May 3, 2026
Merged

fix(keychain): remove destructive migration + Apple-correct API refactor#972
datlechin merged 2 commits intomainfrom
fix/keychain-credential-loss

Conversation

@datlechin
Copy link
Copy Markdown
Member

@datlechin datlechin commented May 3, 2026

Summary

Two stacked commits on the keychain layer:

1. Bug fix — saved connection passwords no longer disappear on app restart.
Root cause: the legacy-keychain migration ran on every launch, found items written through the Data Protection keychain (because on sandboxed configs the legacy query returns those too), copied them to itself, then deleted the only copy via SecItemDelete. Trace from log stream subsystem == \"com.TablePro\" category == \"KeychainHelper\": launch 1 writes succeed; launch 2 logs Successfully migrated, deletes the entry, and the next form load returns errSecItemNotFound (-25300). Removed migrateFromLegacyKeychainIfNeeded and migratePasswordSyncState plus their callers in AppDelegate and SyncSection. Also removed the migrationKey UserDefaults flag and the migrationLock NSLock.

2. Refactor — KeychainHelper API consolidated and tightened to match Apple's documented contracts.

  • Tighter accessibility for local-only secrets: non-syncing items now use kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly. Excludes them from unencrypted device backups (Apple-recommended pairing). Syncing items keep kSecAttrAccessibleAfterFirstUnlock because iCloud Keychain requires non-ThisDeviceOnly accessibility. Existing items keep their original class until next save.
  • Unified naming: write(_:forKey:) / writeString(_:forKey:) / read(forKey:) / readString(forKey:) / readStringResult(forKey:) / delete(forKey:). All argument labels are now forKey:.
  • New typed KeychainResult and KeychainStringResult enums with .found / .notFound / .locked cases. Surfaces the keychain-locked state to all callers instead of silently returning nil.
  • All consumers (ConnectionStorage, SSHProfileStorage, AIKeyStorage, LicenseStorage) updated. Each one now logs the locked case via OSLog with a contextual identifier (connection ID, profile ID, provider ID) instead of swallowing the failure.
  • KeychainHelper is now Sendable. Failure logging routes through SecCopyErrorMessageString for human-readable diagnostics instead of bare OSStatus numbers.
  • Settings > Sync > Passwords adds a one-line caption explaining the toggle only affects new saves; existing passwords keep their current sync state until re-saved (matches Apple's documented kSecAttrSynchronizable semantics).
  • Drive-by: PluginDiagnosticItem.classify marked @MainActor (it calls a @MainActor-isolated method on PluginManager.shared; only call site is already inside await MainActor.run { ... }).

Test plan

  • Launch from Xcode, add a new connection with password, connect successfully, quit (Cmd+Q), re-launch. Open Edit Connection: password field is populated.
  • Repeat with an SSH-tunneled connection (key passphrase + SSH password). Both fields populate after relaunch.
  • log stream --predicate 'subsystem == \"com.TablePro\" AND category == \"KeychainHelper\"' during the above. Confirm: no migrate* log lines (functions removed), no Keychain write/read/delete failed errors during normal operation.
  • Toggle Settings > Sync > Passwords ON, save a new connection password, verify the new item has kSecAttrSynchronizable: true (via Keychain Access). Toggle OFF, save another, verify new item is non-synchronizable. Existing items retain their original sync state. Caption is visible under the toggle.
  • On a Mac that hasn't been unlocked since reboot, attempt to load a connection — confirm OSLog shows the Keychain locked warning with the connection ID, and the form shows an empty password field rather than a misleading "saved successfully" state.
  • swiftlint lint --strict TablePro/Core/Storage/KeychainHelper.swift TablePro/Core/Storage/ConnectionStorage.swift TablePro/Core/Storage/SSHProfileStorage.swift TablePro/Core/Storage/AIKeyStorage.swift TablePro/Core/Storage/LicenseStorage.swift TablePro/Views/Settings/Sections/SyncSection.swift TablePro/Views/Connection/PluginDiagnosticSheet.swift — 0 violations.
  • Build green in Xcode. KeychainHelperTests and KeychainAccessControlTests pass.

@chatgpt-codex-connector
Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@datlechin datlechin changed the title fix(keychain): remove destructive legacy migration that wiped saved passwords fix(keychain): remove destructive migration + Apple-correct API refactor May 3, 2026
@datlechin datlechin merged commit d5554b9 into main May 3, 2026
2 checks passed
@datlechin datlechin deleted the fix/keychain-credential-loss branch May 3, 2026 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant