diff --git a/CHANGELOG.md b/CHANGELOG.md index cc85f3793..a1e57dcdf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,10 @@ 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. @@ -20,9 +24,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - 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 `` 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) diff --git a/TablePro/AppDelegate.swift b/TablePro/AppDelegate.swift index bf3f1c360..370f9240e 100644 --- a/TablePro/AppDelegate.swift +++ b/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() diff --git a/TablePro/Core/Storage/AIKeyStorage.swift b/TablePro/Core/Storage/AIKeyStorage.swift index 726158ac7..d8db36f9f 100644 --- a/TablePro/Core/Storage/AIKeyStorage.swift +++ b/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) } } diff --git a/TablePro/Core/Storage/ConnectionStorage.swift b/TablePro/Core/Storage/ConnectionStorage.swift index 5b3b6c459..4ca0678fc 100644 --- a/TablePro/Core/Storage/ConnectionStorage.swift +++ b/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 diff --git a/TablePro/Core/Storage/KeychainHelper.swift b/TablePro/Core/Storage/KeychainHelper.swift index 9473ef8da..a638730bc 100644 --- a/TablePro/Core/Storage/KeychainHelper.swift +++ b/TablePro/Core/Storage/KeychainHelper.swift @@ -7,23 +7,25 @@ import Foundation import os import Security -enum KeychainLoadResult { - case success(Data) +enum KeychainResult: Sendable, Equatable { + case found(Data) case notFound case locked - case error(OSStatus) } -final class KeychainHelper { +enum KeychainStringResult: Sendable, Equatable { + case found(String) + case notFound + case locked +} + +final class KeychainHelper: Sendable { static let shared = KeychainHelper() + static let passwordSyncEnabledKey = "com.TablePro.keychainPasswordSyncEnabled" private let service = "com.TablePro" private let accessGroup = "D7HJ5TFYCU.com.TablePro.shared" private static let logger = Logger(subsystem: "com.TablePro", category: "KeychainHelper") - private static let migrationKey = "com.TablePro.keychainMigratedToDataProtection" - static let passwordSyncEnabledKey = "com.TablePro.keychainPasswordSyncEnabled" - - private let migrationLock = NSLock() private var isPasswordSyncEnabled: Bool { UserDefaults.standard.bool(forKey: Self.passwordSyncEnabledKey) @@ -31,85 +33,45 @@ final class KeychainHelper { private init() {} - // MARK: - Core Methods + // MARK: - Data API @discardableResult - func save(key: String, data: Data) -> Bool { - var addQuery: [String: Any] = [ - kSecClass as String: kSecClassGenericPassword, - kSecAttrService as String: service, - kSecAttrAccount as String: key, - kSecAttrAccessGroup as String: accessGroup, - kSecValueData as String: data, - kSecUseDataProtectionKeychain as String: true, - kSecAttrAccessible as String: kSecAttrAccessibleAfterFirstUnlock - ] - if isPasswordSyncEnabled { + func write(_ data: Data, forKey key: String) -> Bool { + let synchronizable = isPasswordSyncEnabled + let accessible = accessibility(forSync: synchronizable) + + var addQuery = baseQuery(forKey: key) + addQuery[kSecValueData as String] = data + addQuery[kSecAttrAccessible as String] = accessible + if synchronizable { addQuery[kSecAttrSynchronizable as String] = true } var status = SecItemAdd(addQuery as CFDictionary, nil) if status == errSecDuplicateItem { - let synchronizable = isPasswordSyncEnabled - let searchQuery: [String: Any] = [ - kSecClass as String: kSecClassGenericPassword, - kSecAttrService as String: service, - kSecAttrAccount as String: key, - kSecAttrAccessGroup as String: accessGroup, - kSecUseDataProtectionKeychain as String: true, - kSecAttrSynchronizable as String: kSecAttrSynchronizableAny - ] - let updateAttributes: [String: Any] = [ + var search = baseQuery(forKey: key) + search[kSecAttrSynchronizable as String] = kSecAttrSynchronizableAny + let attributes: [String: Any] = [ kSecValueData as String: data, - kSecAttrSynchronizable as String: synchronizable + kSecAttrSynchronizable as String: synchronizable, + kSecAttrAccessible as String: accessible ] - status = SecItemUpdate(searchQuery as CFDictionary, updateAttributes as CFDictionary) + status = SecItemUpdate(search as CFDictionary, attributes as CFDictionary) } if status != errSecSuccess { - Self.logger.error("Failed to save keychain item for key '\(key, privacy: .public)': \(status)") - } - - return status == errSecSuccess - } - - func load(key: String) -> Data? { - let query: [String: Any] = [ - kSecClass as String: kSecClassGenericPassword, - kSecAttrService as String: service, - kSecAttrAccount as String: key, - kSecAttrAccessGroup as String: accessGroup, - kSecUseDataProtectionKeychain as String: true, - kSecAttrSynchronizable as String: kSecAttrSynchronizableAny, - kSecReturnData as String: true, - kSecMatchLimit as String: kSecMatchLimitOne - ] - - var result: AnyObject? - let status = SecItemCopyMatching(query as CFDictionary, &result) - - guard status == errSecSuccess else { - if status != errSecItemNotFound { - Self.logger.error("Failed to load keychain item for key '\(key, privacy: .public)': \(status)") - } - return nil + log(status: status, operation: "write", key: key) + return false } - - return result as? Data + return true } - func loadWithStatus(key: String) -> KeychainLoadResult { - let query: [String: Any] = [ - kSecClass as String: kSecClassGenericPassword, - kSecAttrService as String: service, - kSecAttrAccount as String: key, - kSecAttrAccessGroup as String: accessGroup, - kSecUseDataProtectionKeychain as String: true, - kSecAttrSynchronizable as String: kSecAttrSynchronizableAny, - kSecReturnData as String: true, - kSecMatchLimit as String: kSecMatchLimitOne - ] + func read(forKey key: String) -> KeychainResult { + var query = baseQuery(forKey: key) + query[kSecAttrSynchronizable as String] = kSecAttrSynchronizableAny + query[kSecReturnData as String] = true + query[kSecMatchLimit as String] = kSecMatchLimitOne var result: AnyObject? let status = SecItemCopyMatching(query as CFDictionary, &result) @@ -117,231 +79,85 @@ final class KeychainHelper { switch status { case errSecSuccess: if let data = result as? Data { - return .success(data) + return .found(data) } return .notFound case errSecItemNotFound: return .notFound case errSecInteractionNotAllowed: - Self.logger.warning("Keychain locked (before first unlock) for key '\(key, privacy: .public)'") + Self.logger.warning("Keychain locked (before first unlock) for '\(key, privacy: .public)'") return .locked default: - Self.logger.error("Keychain error for key '\(key, privacy: .public)': \(status)") - return .error(status) + log(status: status, operation: "read", key: key) + return .notFound } } - func delete(key: String) { - let query: [String: Any] = [ - kSecClass as String: kSecClassGenericPassword, - kSecAttrService as String: service, - kSecAttrAccount as String: key, - kSecAttrAccessGroup as String: accessGroup, - kSecUseDataProtectionKeychain as String: true, - kSecAttrSynchronizable as String: kSecAttrSynchronizableAny - ] + func delete(forKey key: String) { + var query = baseQuery(forKey: key) + query[kSecAttrSynchronizable as String] = kSecAttrSynchronizableAny let status = SecItemDelete(query as CFDictionary) - if status != errSecSuccess, status != errSecItemNotFound { - Self.logger.error("Failed to delete keychain item for key '\(key, privacy: .public)': \(status)") + log(status: status, operation: "delete", key: key) } } // MARK: - String Convenience @discardableResult - func saveString(_ value: String, forKey key: String) -> Bool { + func writeString(_ value: String, forKey key: String) -> Bool { guard let data = value.data(using: .utf8) else { - Self.logger.error("Failed to encode string to UTF-8 for key '\(key, privacy: .public)'") + Self.logger.error("UTF-8 encode failed for '\(key, privacy: .public)'") return false } - return save(key: key, data: data) + return write(data, forKey: key) } - func loadString(forKey key: String) -> String? { - guard let data = load(key: key) else { - return nil + func readString(forKey key: String) -> String? { + if case .found(let value) = readStringResult(forKey: key) { + return value } - return String(data: data, encoding: .utf8) + return nil } - func loadStringWithStatus(forKey key: String) -> (value: String?, isLocked: Bool) { - switch loadWithStatus(key: key) { - case .success(let data): - return (String(data: data, encoding: .utf8), false) + func readStringResult(forKey key: String) -> KeychainStringResult { + switch read(forKey: key) { + case .found(let data): + guard let value = String(data: data, encoding: .utf8) else { + Self.logger.error("UTF-8 decode failed for '\(key, privacy: .public)'") + return .notFound + } + return .found(value) + case .notFound: + return .notFound case .locked: - return (nil, true) - case .notFound, .error: - return (nil, false) + return .locked } } - // MARK: - Migration - - func migrateFromLegacyKeychainIfNeeded() { - guard !UserDefaults.standard.bool(forKey: Self.migrationKey) else { - return - } - - Self.logger.info("Starting legacy keychain migration to Data Protection keychain") + // MARK: - Private - let legacyQuery: [String: Any] = [ + private func baseQuery(forKey key: String) -> [String: Any] { + [ kSecClass as String: kSecClassGenericPassword, kSecAttrService as String: service, - kSecMatchLimit as String: kSecMatchLimitAll, - kSecReturnAttributes as String: true, - kSecReturnData as String: true + kSecAttrAccount as String: key, + kSecAttrAccessGroup as String: accessGroup, + kSecUseDataProtectionKeychain as String: true ] - - var result: AnyObject? - let status = SecItemCopyMatching(legacyQuery as CFDictionary, &result) - - if status == errSecItemNotFound { - Self.logger.info("No legacy keychain items found, marking migration as complete") - UserDefaults.standard.set(true, forKey: Self.migrationKey) - return - } - - guard status == errSecSuccess, let items = result as? [[String: Any]] else { - Self.logger.error("Failed to query legacy keychain items: \(status)") - return - } - - Self.logger.info("Found \(items.count) legacy keychain items to migrate") - - var allSucceeded = true - - for item in items { - guard let account = item[kSecAttrAccount as String] as? String, - let data = item[kSecValueData as String] as? Data else { - Self.logger.warning("Skipping legacy item with missing account or data") - allSucceeded = false - continue - } - - let saved = save(key: account, data: data) - - if saved { - let deleteLegacyQuery: [String: Any] = [ - kSecClass as String: kSecClassGenericPassword, - kSecAttrService as String: service, - kSecAttrAccount as String: account - ] - let deleteStatus = SecItemDelete(deleteLegacyQuery as CFDictionary) - - if deleteStatus != errSecSuccess, deleteStatus != errSecItemNotFound { - Self.logger.warning( - "Migrated item '\(account, privacy: .public)' but failed to delete legacy entry: \(deleteStatus)" - ) - } else { - Self.logger.info("Successfully migrated item '\(account, privacy: .public)'") - } - } else { - Self.logger.error("Failed to migrate item '\(account, privacy: .public)' to Data Protection keychain") - allSucceeded = false - } - } - - if allSucceeded { - UserDefaults.standard.set(true, forKey: Self.migrationKey) - Self.logger.info("Legacy keychain migration completed successfully") - } else { - Self.logger.warning("Legacy keychain migration incomplete, will retry on next launch") - } } - // MARK: - Password Sync Migration - - /// Migrates all TablePro keychain items between local-only and iCloud-synchronizable. - /// Serialized via `migrationLock` to prevent concurrent migrations from rapid toggling. - func migratePasswordSyncState(synchronizable: Bool) { - migrationLock.lock() - defer { migrationLock.unlock() } - - Self.logger.info("Starting keychain sync migration: synchronizable=\(synchronizable)") - - let searchQuery: [String: Any] = [ - kSecClass as String: kSecClassGenericPassword, - kSecAttrService as String: service, - kSecUseDataProtectionKeychain as String: true, - kSecAttrSynchronizable as String: kSecAttrSynchronizableAny, - kSecMatchLimit as String: kSecMatchLimitAll, - kSecReturnAttributes as String: true, - kSecReturnData as String: true - ] - - var result: AnyObject? - let status = SecItemCopyMatching(searchQuery as CFDictionary, &result) - - guard status == errSecSuccess, let items = result as? [[String: Any]] else { - if status == errSecItemNotFound { - Self.logger.info("No keychain items to migrate") - } else { - Self.logger.error("Failed to query items for sync migration: \(status)") - } - return - } - - var migratedCount = 0 - var skippedCount = 0 - - for item in items { - guard let account = item[kSecAttrAccount as String] as? String, - let data = item[kSecValueData as String] as? Data - else { continue } - - let currentlySync = item[kSecAttrSynchronizable as String] as? Bool ?? false - if currentlySync == synchronizable { - skippedCount += 1 - continue - } - - var addQuery: [String: Any] = [ - kSecClass as String: kSecClassGenericPassword, - kSecAttrService as String: service, - kSecAttrAccount as String: account, - kSecValueData as String: data, - kSecUseDataProtectionKeychain as String: true, - kSecAttrAccessible as String: kSecAttrAccessibleAfterFirstUnlock - ] - if synchronizable { - addQuery[kSecAttrSynchronizable as String] = true - } - - let addStatus = SecItemAdd(addQuery as CFDictionary, nil) - - guard addStatus == errSecSuccess || addStatus == errSecDuplicateItem else { - Self.logger.error( - "Failed to create migrated item '\(account, privacy: .public)': \(addStatus)" - ) - continue - } - - // When opting IN (synchronizable=true), delete the old local-only item safely. - // When opting OUT (synchronizable=false), keep the synchronizable item — deleting it - // would propagate via iCloud Keychain and remove it from other Macs still opted in. - if synchronizable { - let deleteQuery: [String: Any] = [ - kSecClass as String: kSecClassGenericPassword, - kSecAttrService as String: service, - kSecAttrAccount as String: account, - kSecUseDataProtectionKeychain as String: true, - kSecAttrSynchronizable as String: false - ] - let deleteStatus = SecItemDelete(deleteQuery as CFDictionary) - if deleteStatus != errSecSuccess, deleteStatus != errSecItemNotFound { - Self.logger.warning( - "Migrated item '\(account, privacy: .public)' but failed to delete old entry: \(deleteStatus)" - ) - } - } - - migratedCount += 1 - } + private func accessibility(forSync synchronizable: Bool) -> CFString { + synchronizable + ? kSecAttrAccessibleAfterFirstUnlock + : kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly + } - Self.logger.info( - "Keychain sync migration complete: \(migratedCount) migrated, \(skippedCount) already correct" + private func log(status: OSStatus, operation: String, key: String) { + let message = SecCopyErrorMessageString(status, nil) as String? ?? "OSStatus \(status)" + Self.logger.error( + "Keychain \(operation, privacy: .public) failed for '\(key, privacy: .public)': \(message, privacy: .public)" ) } } diff --git a/TablePro/Core/Storage/LicenseStorage.swift b/TablePro/Core/Storage/LicenseStorage.swift index 994017fe4..4b4f8339f 100644 --- a/TablePro/Core/Storage/LicenseStorage.swift +++ b/TablePro/Core/Storage/LicenseStorage.swift @@ -26,19 +26,24 @@ final class LicenseStorage { // MARK: - License Key (Keychain) - /// Save license key to Keychain func saveLicenseKey(_ key: String) { - KeychainHelper.shared.saveString(key, forKey: Keys.keychainLicenseKey) + KeychainHelper.shared.writeString(key, forKey: Keys.keychainLicenseKey) } - /// Load license key from Keychain func loadLicenseKey() -> String? { - KeychainHelper.shared.loadString(forKey: Keys.keychainLicenseKey) + switch KeychainHelper.shared.readStringResult(forKey: Keys.keychainLicenseKey) { + case .found(let value): + return value + case .locked: + Self.logger.warning("License key unavailable — Keychain locked") + return nil + case .notFound: + return nil + } } - /// Delete license key from Keychain func deleteLicenseKey() { - KeychainHelper.shared.delete(key: Keys.keychainLicenseKey) + KeychainHelper.shared.delete(forKey: Keys.keychainLicenseKey) } // MARK: - Signed Payload (UserDefaults) diff --git a/TablePro/Core/Storage/SSHProfileStorage.swift b/TablePro/Core/Storage/SSHProfileStorage.swift index 6a3f2e2e4..a1a3baa5c 100644 --- a/TablePro/Core/Storage/SSHProfileStorage.swift +++ b/TablePro/Core/Storage/SSHProfileStorage.swift @@ -97,50 +97,64 @@ final class SSHProfileStorage { func saveSSHPassword(_ password: String, for profileId: UUID) { let key = "com.TablePro.sshprofile.password.\(profileId.uuidString)" - KeychainHelper.shared.saveString(password, forKey: key) + KeychainHelper.shared.writeString(password, forKey: key) } func loadSSHPassword(for profileId: UUID) -> String? { let key = "com.TablePro.sshprofile.password.\(profileId.uuidString)" - return KeychainHelper.shared.loadString(forKey: key) + return resolveString(label: "SSH profile password", profileId: profileId, forKey: key) } func deleteSSHPassword(for profileId: UUID) { let key = "com.TablePro.sshprofile.password.\(profileId.uuidString)" - KeychainHelper.shared.delete(key: key) + KeychainHelper.shared.delete(forKey: key) } // MARK: - Key Passphrase Storage func saveKeyPassphrase(_ passphrase: String, for profileId: UUID) { let key = "com.TablePro.sshprofile.keypassphrase.\(profileId.uuidString)" - KeychainHelper.shared.saveString(passphrase, forKey: key) + KeychainHelper.shared.writeString(passphrase, forKey: key) } func loadKeyPassphrase(for profileId: UUID) -> String? { let key = "com.TablePro.sshprofile.keypassphrase.\(profileId.uuidString)" - return KeychainHelper.shared.loadString(forKey: key) + return resolveString(label: "SSH profile key passphrase", profileId: profileId, forKey: key) } func deleteKeyPassphrase(for profileId: UUID) { let key = "com.TablePro.sshprofile.keypassphrase.\(profileId.uuidString)" - KeychainHelper.shared.delete(key: key) + KeychainHelper.shared.delete(forKey: key) } // MARK: - TOTP Secret Storage func saveTOTPSecret(_ secret: String, for profileId: UUID) { let key = "com.TablePro.sshprofile.totpsecret.\(profileId.uuidString)" - KeychainHelper.shared.saveString(secret, forKey: key) + KeychainHelper.shared.writeString(secret, forKey: key) } func loadTOTPSecret(for profileId: UUID) -> String? { let key = "com.TablePro.sshprofile.totpsecret.\(profileId.uuidString)" - return KeychainHelper.shared.loadString(forKey: key) + return resolveString(label: "SSH profile TOTP secret", profileId: profileId, forKey: key) } func deleteTOTPSecret(for profileId: UUID) { let key = "com.TablePro.sshprofile.totpsecret.\(profileId.uuidString)" - KeychainHelper.shared.delete(key: key) + KeychainHelper.shared.delete(forKey: key) + } + + private func resolveString(label: String, profileId: UUID, forKey key: String) -> String? { + switch KeychainHelper.shared.readStringResult(forKey: key) { + case .found(let value): + return value + case .locked: + Self.logger.warning( + "\(label, privacy: .public) unavailable — Keychain locked (profileId=\(profileId.uuidString, privacy: .public))" + ) + return nil + case .notFound: + return nil + } } } diff --git a/TablePro/Resources/Localizable.xcstrings b/TablePro/Resources/Localizable.xcstrings index e45d8fc82..d076f61e9 100644 --- a/TablePro/Resources/Localizable.xcstrings +++ b/TablePro/Resources/Localizable.xcstrings @@ -11948,6 +11948,9 @@ } } } + }, + "Copy Diagnostic Info" : { + }, "Copy error message" : { "localizations" : { @@ -15482,6 +15485,9 @@ } } } + }, + "Diagnostic Info" : { + }, "Diagram" : { "localizations" : { @@ -31989,6 +31995,9 @@ } } } + }, + "Open Issue Tracker" : { + }, "Open JSON Viewer" : { "extractionState" : "stale", @@ -44100,6 +44109,9 @@ } } } + }, + "Suggested Actions" : { + }, "Suspended" : { "localizations" : { diff --git a/TablePro/Views/Connection/PluginDiagnosticSheet.swift b/TablePro/Views/Connection/PluginDiagnosticSheet.swift index 13fc2f674..31aca08de 100644 --- a/TablePro/Views/Connection/PluginDiagnosticSheet.swift +++ b/TablePro/Views/Connection/PluginDiagnosticSheet.swift @@ -13,6 +13,7 @@ struct PluginDiagnosticItem: Identifiable, Equatable { let connectionTarget: String let username: String + @MainActor static func classify( error: Error, connection: DatabaseConnection, diff --git a/TablePro/Views/Settings/Sections/SyncSection.swift b/TablePro/Views/Settings/Sections/SyncSection.swift index 964c59955..c46d8e0d0 100644 --- a/TablePro/Views/Settings/Sections/SyncSection.swift +++ b/TablePro/Views/Settings/Sections/SyncSection.swift @@ -109,6 +109,12 @@ struct SyncSection: View { } .help("Syncs passwords via iCloud Keychain (end-to-end encrypted).") .padding(.leading, 20) + + Text("Only affects new saves. Re-save a password to update its sync.") + .font(.caption) + .foregroundStyle(.secondary) + .fixedSize(horizontal: false, vertical: true) + .padding(.leading, 20) } Toggle("Groups & Tags:", isOn: $settingsManager.sync.syncGroupsAndTags) @@ -121,20 +127,12 @@ struct SyncSection: View { private func onPasswordSyncChanged(_ enabled: Bool) { let effective = settingsManager.sync.enabled && settingsManager.sync.syncConnections && enabled - Task.detached { - KeychainHelper.shared.migratePasswordSyncState(synchronizable: effective) - UserDefaults.standard.set(effective, forKey: KeychainHelper.passwordSyncEnabledKey) - } + UserDefaults.standard.set(effective, forKey: KeychainHelper.passwordSyncEnabledKey) } private func updatePasswordSyncFlag() { let sync = settingsManager.sync let effective = sync.enabled && sync.syncConnections && sync.syncPasswords - let current = UserDefaults.standard.bool(forKey: KeychainHelper.passwordSyncEnabledKey) - guard effective != current else { return } - Task.detached { - KeychainHelper.shared.migratePasswordSyncState(synchronizable: effective) - UserDefaults.standard.set(effective, forKey: KeychainHelper.passwordSyncEnabledKey) - } + UserDefaults.standard.set(effective, forKey: KeychainHelper.passwordSyncEnabledKey) } } diff --git a/TableProTests/Core/Storage/KeychainAccessControlTests.swift b/TableProTests/Core/Storage/KeychainAccessControlTests.swift index b6976f22a..dbb108192 100644 --- a/TableProTests/Core/Storage/KeychainAccessControlTests.swift +++ b/TableProTests/Core/Storage/KeychainAccessControlTests.swift @@ -10,12 +10,18 @@ import Testing @Suite("Keychain Access Control") struct KeychainAccessControlTests { - @Test("AfterFirstUnlock constant is available for background access") + @Test("AfterFirstUnlock constant is available for syncable items") func correctConstantAvailable() { let expected = kSecAttrAccessibleAfterFirstUnlock #expect(expected != nil) } + @Test("AfterFirstUnlockThisDeviceOnly constant is available for non-syncable items") + func deviceOnlyConstantAvailable() { + let expected = kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly + #expect(expected != nil) + } + @Test("Data Protection keychain flag is a valid boolean") func dataProtectionKeychainFlag() { let flag = kSecUseDataProtectionKeychain diff --git a/TableProTests/Core/Storage/KeychainHelperTests.swift b/TableProTests/Core/Storage/KeychainHelperTests.swift index eb31b574f..f3be30e1c 100644 --- a/TableProTests/Core/Storage/KeychainHelperTests.swift +++ b/TableProTests/Core/Storage/KeychainHelperTests.swift @@ -11,54 +11,74 @@ import Testing struct KeychainHelperTests { private let helper = KeychainHelper.shared - @Test("Save and load round trip") - func saveAndLoadRoundTrip() { - let key = "test.roundtrip.\(UUID().uuidString)" - defer { helper.delete(key: key) } + @Test("writeString and readString round trip") + func writeAndReadStringRoundTrip() { + let key = "test.string.roundtrip.\(UUID().uuidString)" + defer { helper.delete(forKey: key) } - let saved = helper.saveString("hello", forKey: key) + let saved = helper.writeString("hello", forKey: key) #expect(saved) - let loaded = helper.loadString(forKey: key) + let loaded = helper.readString(forKey: key) #expect(loaded == "hello") } - @Test("Delete removes item") + @Test("write and read Data round trip") + func writeAndReadDataRoundTrip() { + let key = "test.data.roundtrip.\(UUID().uuidString)" + defer { helper.delete(forKey: key) } + + let payload = Data([0x00, 0x01, 0x02, 0xFF]) + let saved = helper.write(payload, forKey: key) + #expect(saved) + + let result = helper.read(forKey: key) + #expect(result == .found(payload)) + } + + @Test("delete removes item; subsequent read returns notFound") func deleteRemovesItem() { let key = "test.delete.\(UUID().uuidString)" - defer { helper.delete(key: key) } + defer { helper.delete(forKey: key) } - _ = helper.saveString("temporary", forKey: key) - helper.delete(key: key) + _ = helper.writeString("temporary", forKey: key) + helper.delete(forKey: key) - let loaded = helper.loadString(forKey: key) - #expect(loaded == nil) + #expect(helper.read(forKey: key) == .notFound) + #expect(helper.readString(forKey: key) == nil) } - @Test("Upsert overwrites existing value") - func upsertOverwritesExistingValue() { + @Test("write overwrites existing value") + func writeOverwritesExistingValue() { let key = "test.upsert.\(UUID().uuidString)" - defer { helper.delete(key: key) } + defer { helper.delete(forKey: key) } + + _ = helper.writeString("first", forKey: key) + _ = helper.writeString("second", forKey: key) - _ = helper.saveString("first", forKey: key) - _ = helper.saveString("second", forKey: key) + #expect(helper.readString(forKey: key) == "second") + } - let loaded = helper.loadString(forKey: key) - #expect(loaded == "second") + @Test("read returns notFound for missing key") + func readReturnsNotFoundForMissingKey() { + let key = "test.missing.\(UUID().uuidString)" + #expect(helper.read(forKey: key) == .notFound) + #expect(helper.readString(forKey: key) == nil) + #expect(helper.readStringResult(forKey: key) == .notFound) } - @Test("Load nonexistent returns nil") - func loadNonexistentReturnsNil() { - let key = "test.nonexistent.\(UUID().uuidString)" - defer { helper.delete(key: key) } + @Test("readStringResult exposes found case") + func readStringResultExposesFound() { + let key = "test.stringresult.\(UUID().uuidString)" + defer { helper.delete(forKey: key) } - let loaded = helper.loadString(forKey: key) - #expect(loaded == nil) + _ = helper.writeString("payload", forKey: key) + #expect(helper.readStringResult(forKey: key) == .found("payload")) } - @Test("Migration flag defaults to false") - func migrationFlagDefaultsFalse() { - let defaultsKey = "com.TablePro.keychainMigratedToDataProtection" + @Test("password sync flag defaults to false when unset") + func passwordSyncFlagDefaultsFalse() { + let defaultsKey = KeychainHelper.passwordSyncEnabledKey let previous = UserDefaults.standard.object(forKey: defaultsKey) defer { if let previous { @@ -69,7 +89,6 @@ struct KeychainHelperTests { } UserDefaults.standard.removeObject(forKey: defaultsKey) - let value = UserDefaults.standard.bool(forKey: defaultsKey) - #expect(value == false) + #expect(UserDefaults.standard.bool(forKey: defaultsKey) == false) } }