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

* fix(keychain): remove destructive legacy migration that wiped saved passwords

* refactor(keychain): consolidate API, tighten accessibility for non-sync items

Author: datlechin

CHANGELOG.md

@@ -13,16 +13,24 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Oracle Test Connection now opens a focused diagnostic sheet for auth failures with copy-able diagnostic info, suggested actions, and a link to file an issue
 - Oracle connection negotiation now matches python-oracledb's 23ai compile-capability advertisement, including TTC4 explicit boundary, TTC5 token/pipelining/sessionless flags, OCI3 sync, dequeue selectors, and sparse vector features
 
+### Removed
+
+- Keychain: the legacy-keychain migration (`migrateFromLegacyKeychainIfNeeded`) and the password-sync-state migration (`migratePasswordSyncState`). The first violated Apple's Data Protection keychain contract on sandboxed macOS apps and corrupted user credentials; the second toggled `kSecAttrSynchronizable` at runtime, which Apple does not document as safe. The Sync Passwords settings toggle now applies to new saves only — existing keychain items keep their original sync state, matching Apple's documented behavior. Users with stale items in the legacy keychain can clean them via Keychain Access; the running app no longer touches them.
+
 ### Changed
 
 - Internal: introduce `TabSession` as the foundation type for the editor tab/window subsystem rewrite. Currently a parallel structure mirroring `QueryTab`; subsequent PRs migrate state ownership and lifecycle hooks per `docs/architecture/tab-subsystem-rewrite.md`. No user-visible behavior change in this PR.
 - Internal: row data and load epoch now live on `TabSession`. `TabSessionRegistry` exposes the row-access methods directly (`tableRows(for:)`, `setTableRows(_:for:)`, `evict(for:)`, etc.); the intermediate `TableRowsStore` facade is gone. All consumers (coordinator, extensions, views, command actions) now read row data from the registry. No user-visible behavior change.
 - Internal: hidden-column state moves from the per-window `ColumnVisibilityManager` into each tab's `columnLayout.hiddenColumns`. The shared manager is removed; `MainContentCoordinator` exposes `hideColumn`, `showColumn`, `toggleColumnVisibility`, `showAllColumns`, `hideAllColumns`, and `pruneHiddenColumns` that mutate the active tab directly. Per-table UserDefaults persistence moves into a small `ColumnVisibilityPersistence` service. Tab-switch save/restore swap is gone — each tab is its own source of truth. No user-visible behavior change.
 - Internal: filter state collapses from three places (the per-window `FilterStateManager`, the `TabFilterState` snapshot on `QueryTab`, and the per-table file-based restore) to a single source: `tab.filterState`. The shared manager is removed; `MainContentCoordinator` now exposes the full filter API (`addFilter`, `applyAllFilters`, `clearFilterState`, `toggleFilterPanel`, `setFKFilter`, `saveLastFilters(for:)`, `restoreLastFilters(for:)`, `saveFilterPreset`, `loadFilterPreset`, `generateFilterPreviewSQL`, etc.) that mutates the active tab. The file-based "restore last filters" persistence in `FilterSettingsStorage` is unchanged. `FilterPanelView`, `MainStatusBarView`, `MainContentCommandActions`, `MainContentView`, and `MainEditorContentView` read filter state directly off the active tab. No user-visible behavior change.
 - Internal: extract `QueryExecutor` service from `MainContentCoordinator`. Query data fetch, parallel schema fetch, schema parsing, parameter detection, row-cap policy, and DDL detection now live in `TablePro/Core/Services/Query/QueryExecutor.swift`. SQL parsing helpers (`extractTableName`, `stripTrailingOrderBy`, `parseSQLiteCheckConstraintValues`) move into `QuerySqlParser`. Coordinator methods become thin wrappers; behavior unchanged. No user-visible behavior change.
+- Security: non-syncing keychain items now use `kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly`. This keeps local-only secrets out of unencrypted device backups (the pairing Apple recommends for local secrets). Syncing items still use `kSecAttrAccessibleAfterFirstUnlock` because iCloud Keychain requires it. Existing items keep their accessibility class until you save them again.
+- Internal: `KeychainHelper` API consolidated. `save(key:data:)`/`saveString(_:forKey:)` become `write(_:forKey:)`/`writeString(_:forKey:)`; `load(key:)`/`loadWithStatus(key:)`/`loadString(forKey:)`/`loadStringWithStatus(forKey:)` become `read(forKey:)`/`readString(forKey:)`/`readStringResult(forKey:)`, all returning a typed `KeychainResult`/`KeychainStringResult` enum with `.found`/`.notFound`/`.locked` cases. `delete(key:)` renamed to `delete(forKey:)`. All consumers (`ConnectionStorage`, `SSHProfileStorage`, `AIKeyStorage`, `LicenseStorage`) updated to log when the keychain is locked rather than silently returning nil. `KeychainHelper` is now `Sendable`. Failure logging uses `SecCopyErrorMessageString`.
+- Settings > Sync > Passwords shows a caption explaining the toggle only affects new saves. Existing passwords keep their current sync state until you re-save them.
 
 ### Fixed
 
+- Saved connection passwords no longer disappear after quitting and relaunching the app. The legacy-keychain migration that ran on every launch was destructive on sandboxed macOS configurations: queries without `kSecUseDataProtectionKeychain` returned items that had been written *with* the flag, and the migration's "delete legacy entry" step then removed the only copy. Removed the legacy keychain migration entirely; `KeychainHelper` now exclusively reads and writes through the Data Protection keychain on every launch.
 - Tab switching: rapid Cmd+Number presses no longer leave a tail of tab transitions playing after the user releases the keys. The tab-selection setter (`NSWindowTabGroup.selectedWindow`) is now wrapped in `NSAnimationContext.runAnimationGroup` with `duration = 0`, so AppKit applies each switch synchronously without queuing a CAAnimation. Lazy-load also moved out of `windowDidBecomeKey` into `.task(id:)` view-appearance lifecycle per Apple's documentation. Note: extreme Cmd+Number bursts (e.g. holding the key for key-repeat) still incur per-switch AppKit window-focus overhead; this is platform-inherent to native NSWindow tabs and documented in `docs/architecture/tab-subsystem-rewrite.md` D2
 - Oracle TIMESTAMP, TIMESTAMP WITH TIME ZONE, TIMESTAMP WITH LOCAL TIME ZONE, INTERVAL DAY TO SECOND, INTERVAL YEAR TO MONTH, DATE, RAW, and BLOB columns now render through typed decoders instead of garbled text. Tables containing INTERVAL YEAR TO MONTH or BFILE columns no longer crash the app on row fetch. Unknown column types display `<unsupported: type>` instead of crashing (#965)
 - Oracle connections to 23ai cloud and containerized deployments no longer fail with `uncleanShutdown` mid-handshake. OOB urgent-byte send now requires the server to advertise `TNS_ACCEPT_FLAG_CHECK_OOB`, matching python-oracledb behavior (#483)

TablePro/AppDelegate.swift

@@ -41,16 +41,7 @@ class AppDelegate: NSObject, NSApplicationDelegate {
         NSWindow.allowsAutomaticWindowTabbing = true
         let syncSettings = AppSettingsStorage.shared.loadSync()
         let passwordSyncExpected = syncSettings.enabled && syncSettings.syncConnections && syncSettings.syncPasswords
-        let previousSyncState = UserDefaults.standard.bool(forKey: KeychainHelper.passwordSyncEnabledKey)
         UserDefaults.standard.set(passwordSyncExpected, forKey: KeychainHelper.passwordSyncEnabledKey)
-        Task.detached(priority: .utility) {
-            KeychainHelper.shared.migrateFromLegacyKeychainIfNeeded()
-        }
-        if passwordSyncExpected != previousSyncState {
-            Task.detached(priority: .background) {
-                KeychainHelper.shared.migratePasswordSyncState(synchronizable: passwordSyncExpected)
-            }
-        }
         DatabaseManager.shared.startObservingSystemEvents()
 
         MemoryPressureAdvisor.startMonitoring()

TablePro/Core/Storage/AIKeyStorage.swift

@@ -7,30 +7,37 @@
 //
 
 import Foundation
+import os
 
-/// Singleton Keychain storage for AI provider API keys
 final class AIKeyStorage {
     static let shared = AIKeyStorage()
 
-    private init() {}
+    private static let logger = Logger(subsystem: "com.TablePro", category: "AIKeyStorage")
 
-    // MARK: - API Key Operations
+    private init() {}
 
-    /// Save an API key to Keychain for the given provider
     func saveAPIKey(_ apiKey: String, for providerID: UUID) {
         let key = "com.TablePro.aikey.\(providerID.uuidString)"
-        KeychainHelper.shared.saveString(apiKey, forKey: key)
+        KeychainHelper.shared.writeString(apiKey, forKey: key)
     }
 
-    /// Load an API key from Keychain for the given provider
     func loadAPIKey(for providerID: UUID) -> String? {
         let key = "com.TablePro.aikey.\(providerID.uuidString)"
-        return KeychainHelper.shared.loadString(forKey: key)
+        switch KeychainHelper.shared.readStringResult(forKey: key) {
+        case .found(let value):
+            return value
+        case .locked:
+            Self.logger.warning(
+                "AI API key unavailable — Keychain locked (providerID=\(providerID.uuidString, privacy: .public))"
+            )
+            return nil
+        case .notFound:
+            return nil
+        }
     }
 
-    /// Delete an API key from Keychain for the given provider
     func deleteAPIKey(for providerID: UUID) {
         let key = "com.TablePro.aikey.\(providerID.uuidString)"
-        KeychainHelper.shared.delete(key: key)
+        KeychainHelper.shared.delete(forKey: key)
     }
 }

TablePro/Core/Storage/ConnectionStorage.swift

@@ -264,80 +264,68 @@ final class ConnectionStorage {
 
     func savePassword(_ password: String, for connectionId: UUID) {
         let key = "com.TablePro.password.\(connectionId.uuidString)"
-        KeychainHelper.shared.saveString(password, forKey: key)
+        KeychainHelper.shared.writeString(password, forKey: key)
     }
 
     func loadPassword(for connectionId: UUID) -> String? {
         let key = "com.TablePro.password.\(connectionId.uuidString)"
-        let (value, isLocked) = KeychainHelper.shared.loadStringWithStatus(forKey: key)
-        if isLocked {
-            Self.logger.warning("Database password unavailable — Keychain locked (connId=\(connectionId.uuidString, privacy: .public))")
-        }
-        return value
+        return resolveString(.init(label: "Database password", connectionId: connectionId), forKey: key)
     }
 
     func deletePassword(for connectionId: UUID) {
         let key = "com.TablePro.password.\(connectionId.uuidString)"
-        KeychainHelper.shared.delete(key: key)
+        KeychainHelper.shared.delete(forKey: key)
     }
 
     // MARK: - SSH Password Storage
 
     func saveSSHPassword(_ password: String, for connectionId: UUID) {
         let key = "com.TablePro.sshpassword.\(connectionId.uuidString)"
-        KeychainHelper.shared.saveString(password, forKey: key)
+        KeychainHelper.shared.writeString(password, forKey: key)
     }
 
     func loadSSHPassword(for connectionId: UUID) -> String? {
         let key = "com.TablePro.sshpassword.\(connectionId.uuidString)"
-        let (value, isLocked) = KeychainHelper.shared.loadStringWithStatus(forKey: key)
-        if isLocked {
-            Self.logger.warning("SSH password unavailable — Keychain locked (connId=\(connectionId.uuidString, privacy: .public))")
-        }
-        return value
+        return resolveString(.init(label: "SSH password", connectionId: connectionId), forKey: key)
     }
 
     func deleteSSHPassword(for connectionId: UUID) {
         let key = "com.TablePro.sshpassword.\(connectionId.uuidString)"
-        KeychainHelper.shared.delete(key: key)
+        KeychainHelper.shared.delete(forKey: key)
     }
 
     // MARK: - Key Passphrase Storage
 
     func saveKeyPassphrase(_ passphrase: String, for connectionId: UUID) {
         let key = "com.TablePro.keypassphrase.\(connectionId.uuidString)"
-        KeychainHelper.shared.saveString(passphrase, forKey: key)
+        KeychainHelper.shared.writeString(passphrase, forKey: key)
     }
 
     func loadKeyPassphrase(for connectionId: UUID) -> String? {
         let key = "com.TablePro.keypassphrase.\(connectionId.uuidString)"
-        let (value, isLocked) = KeychainHelper.shared.loadStringWithStatus(forKey: key)
-        if isLocked {
-            Self.logger.warning("Key passphrase unavailable — Keychain locked (connId=\(connectionId.uuidString, privacy: .public))")
-        }
-        return value
+        return resolveString(.init(label: "Key passphrase", connectionId: connectionId), forKey: key)
     }
 
     func deleteKeyPassphrase(for connectionId: UUID) {
         let key = "com.TablePro.keypassphrase.\(connectionId.uuidString)"
-        KeychainHelper.shared.delete(key: key)
+        KeychainHelper.shared.delete(forKey: key)
     }
 
     // MARK: - Plugin Secure Field Storage
 
     func savePluginSecureField(_ value: String, fieldId: String, for connectionId: UUID) {
         let key = "com.TablePro.plugin.\(fieldId).\(connectionId.uuidString)"
-        KeychainHelper.shared.saveString(value, forKey: key)
+        KeychainHelper.shared.writeString(value, forKey: key)
     }
 
     func loadPluginSecureField(fieldId: String, for connectionId: UUID) -> String? {
         let key = "com.TablePro.plugin.\(fieldId).\(connectionId.uuidString)"
-        return KeychainHelper.shared.loadString(forKey: key)
+        return resolveString(.init(label: "Plugin field \(fieldId)", connectionId: connectionId), forKey: key)
     }
 
     func deletePluginSecureField(fieldId: String, for connectionId: UUID) {
         let key = "com.TablePro.plugin.\(fieldId).\(connectionId.uuidString)"
-        KeychainHelper.shared.delete(key: key)
+        KeychainHelper.shared.delete(forKey: key)
     }
 
     func deleteAllPluginSecureFields(for connectionId: UUID, fieldIds: [String]) {
@@ -350,17 +338,36 @@ final class ConnectionStorage {
 
     func saveTOTPSecret(_ secret: String, for connectionId: UUID) {
         let key = "com.TablePro.totpsecret.\(connectionId.uuidString)"
-        KeychainHelper.shared.saveString(secret, forKey: key)
+        KeychainHelper.shared.writeString(secret, forKey: key)
     }
 
     func loadTOTPSecret(for connectionId: UUID) -> String? {
         let key = "com.TablePro.totpsecret.\(connectionId.uuidString)"
-        return KeychainHelper.shared.loadString(forKey: key)
+        return resolveString(.init(label: "TOTP secret", connectionId: connectionId), forKey: key)
     }
 
     func deleteTOTPSecret(for connectionId: UUID) {
         let key = "com.TablePro.totpsecret.\(connectionId.uuidString)"
-        KeychainHelper.shared.delete(key: key)
+        KeychainHelper.shared.delete(forKey: key)
+    }
+
+    private struct SecretContext {
+        let label: String
+        let connectionId: UUID
+    }
+
+    private func resolveString(_ context: SecretContext, forKey key: String) -> String? {
+        switch KeychainHelper.shared.readStringResult(forKey: key) {
+        case .found(let value):
+            return value
+        case .locked:
+            Self.logger.warning(
+                "\(context.label, privacy: .public) unavailable — Keychain locked (connId=\(context.connectionId.uuidString, privacy: .public))"
+            )
+            return nil
+        case .notFound:
+            return nil
+        }
     }
 
     // MARK: - Plugin Secure Field Migration