fix(settings): preserve provider API keys until Keychain write succeeds#433
fix(settings): preserve provider API keys until Keychain write succeeds#433postoso wants to merge 1 commit into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c5c0851b9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| decoded[index] = SavedProvider( | ||
| id: provider.id, | ||
| name: provider.name, | ||
| baseURL: provider.baseURL, | ||
| apiKey: "", |
There was a problem hiding this comment.
When a legacy Keychain item cannot be deleted, KeychainService.saveStoredKeys() throws from removeLegacyEntries() after SecItemAdd/SecItemUpdate has already persisted the consolidated key. Because this new blanking now only runs if storeKey returns normally, that cleanup-only failure path leaves the provider API key in SavedProviders plaintext on every launch even though the replacement Keychain write succeeded; the migration should distinguish write failure from post-write cleanup failure or avoid surfacing cleanup failure as a failed store.
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
postoso
force-pushed
the
fix/keychain-migration-dataloss
branch
from
1c5c085 to
60f98ca
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60f98cac47
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| didMutate = true | ||
| try? self.keychain.removeLegacyEntries(providerIDs: Array(legacyKeychain.keys)) | ||
| if legacyKeychain.isEmpty == false { | ||
| merged.merge(self.sanitizeAPIKeys(legacyKeychain)) { current, _ in current } |
There was a problem hiding this comment.
When the same provider exists in both ProviderAPIKeys defaults and the per-provider legacy Keychain, but not yet in the consolidated item, line 3057 has already inserted the defaults value and this merge keeps that stale plaintext value, discarding the legacy Keychain value. In mixed-upgrade states where the Keychain entry has the current secret (the previous migration order gave it precedence), the later completion/defaults removal/legacy cleanup persists the stale key and loses the working one.
Useful? React with 👍 / 👎.
postoso
force-pushed
the
fix/keychain-migration-dataloss
branch
from
60f98ca to
70c5def
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70c5def051
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } catch { | ||
| self.logProviderAPIKeyPersistenceFailure(error) | ||
| return |
There was a problem hiding this comment.
When legacyProviderEntries() throws, such as for one corrupt or inaccessible legacy per-provider item, this return aborts before ProviderAPIKeys defaults are copied into the consolidated Keychain item. Since providerAPIKeys only reads fetchAllKeys(), users whose current key only exists in defaults will launch with no usable API key even though saving those defaults to the consolidated item may still be possible, and the plaintext defaults will remain stuck behind the failing legacy read on every launch.
Useful? React with 👍 / 👎.
postoso
force-pushed
the
fix/keychain-migration-dataloss
branch
from
70c5def to
d69d1ef
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d69d1efe37
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| merged.merge(self.sanitizeAPIKeys(legacyDefaults)) { _, new in new } | ||
| didMutate = true | ||
| if legacyKeychain.isEmpty == false { | ||
| merged.merge(self.sanitizeAPIKeys(legacyKeychain)) { current, _ in current } |
There was a problem hiding this comment.
Fresh evidence: after the new didReadLegacyKeychain == false path saves ProviderAPIKeys into the consolidated item without setting the completion flag, a later launch where legacyProviderEntries() succeeds will hit this merge and keep that fallback consolidated value over the legacy Keychain value. In the transient-read-failure case where defaults are stale but the legacy Keychain has the current secret, this marks the migration complete and removes the legacy sources, permanently replacing the working key with the stale defaults value.
Useful? React with 👍 / 👎.
postoso
force-pushed
the
fix/keychain-migration-dataloss
branch
from
d69d1ef to
edfd692
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: edfd692e61
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } catch { | ||
| self.logProviderAPIKeyPersistenceFailure(error) | ||
| } | ||
| guard didReadLegacyKeychain else { return } |
There was a problem hiding this comment.
When legacyProviderEntries() has a transient error, the defaults have already been saved to the consolidated Keychain by this point, but this return leaves ProviderAPIKeys in defaults and keeps the migration incomplete. If the user clears that API key before a later launch where the legacy read succeeds, the consolidated dictionary no longer contains the provider, so the stale defaults are merged again and then marked complete, undoing the user's deletion and restoring a key they explicitly removed.
Useful? React with 👍 / 👎.
postoso
force-pushed
the
fix/keychain-migration-dataloss
branch
from
edfd692 to
025d29f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 025d29ffe0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let legacyKeychainIDs = (try? self.keychain.legacyProviderEntries().keys).map(Array.init) ?? [] | ||
| let removed = Set(legacyDefaults.keys).union(legacyKeychainIDs).filter { stored[$0] == nil } |
There was a problem hiding this comment.
When a user clears a key while migration is still incomplete and legacyProviderEntries() is still failing, this falls back to an empty legacy ID list and lets the save report success after only removing defaults. Any stale per-provider legacy Keychain item remains unread and will be merged back into the consolidated store on a later launch once the read recovers, undoing the user's deletion; the purge should either fail the save or record/remove a tombstone for the unread legacy source.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Partially agree. The purge does attempt removeLegacyEntries with the defaults-derived provider list even when the enumeration read fails, so a failing read alone does not leave the legacy item behind. The remaining window needs the legacy delete itself to also fail silently at clear time, and then a later launch where the read recovers. Closing that triple-failure case needs deletion tombstones, which is more migration state than this PR should grow. The PR now covers the concrete dataloss paths: keys preserved until the Keychain write succeeds, recovered legacy Keychain values beating fallback copies, and deletions propagating to both legacy sources. If the tombstone hardening is wanted I am happy to do it as a follow-up.
1 participant
Description
Fixes provider API-key persistence so failed Keychain writes do not erase the last saved copy of a key.
The migration path now keeps legacy defaults and legacy Keychain entries until the consolidated Keychain save succeeds, marks migration complete only after that save, and keeps consolidated keys ahead of stale legacy values on retry. The saved-provider scrub path now blanks plaintext API keys only after
storeKeysucceeds. A smallProviderKeychainprotocol makes the behavior testable without the real Keychain.Type of Change
Related Issue or Discussion
Updates this PR: #433. No separate issue was filed.
Testing
swiftformat --config .swiftformat SourcesProviderAPIKeyMigrationTestspassed, 10 tests, 0 failuresgit diff --checkProviderAPIKeyMigrationTests.*.plistfiles remainedSelectedSpeechModelstayed/restored toparakeet-tdt-v2Screenshots / Video
Notes
Hosted-test defaults contamination is a separate follow-up and is not fixed by this PR.