fix: add rollback semantics to secure-storage credential operations#241
Draft
bukinoshita wants to merge 1 commit intomainfrom
Draft
fix: add rollback semantics to secure-storage credential operations#241bukinoshita wants to merge 1 commit intomainfrom
bukinoshita wants to merge 1 commit intomainfrom
Conversation
Resolve data integrity issue (BU-632) where secure-storage login/logout paths could leave local auth state inconsistent when a backend or filesystem failure occurs mid-operation. Changes to src/lib/config.ts: - storeApiKeyAsync: roll back backend.set() if credentials file write fails - removeApiKeyAsync: verify backend.delete() returns true before proceeding; restore the key in the backend if file removal fails afterward - removeAllApiKeysAsync: aggregate all delete results and abort with error if any backend deletion fails, preserving the credentials file - renameProfileAsync: verify backend.delete() returns true; roll back backend state if either the old-name delete fails or the file rename fails Tests added for each failure mode: - storeApiKeyAsync: backend rollback on file write failure - removeApiKeyAsync: throws on false delete return; restores key on file failure - removeAllApiKeysAsync: preserves file when any backend delete fails - renameProfileAsync: cleans up new-name on old-name delete failure; restores old-name on file write failure Co-authored-by: Bu Kinoshita <bukinoshita@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Resolves BU-632: CLI auth secure-storage desync strands profiles and credentials.
The secure-storage login/logout paths in
src/lib/config.tsupdated the OS credential backend andcredentials.jsonin separate steps with no rollback or result verification. A failed write/delete could leave local auth state inconsistent — profiles could remain listed without a usable key, orlogoutcould report success while the secret was still retained in the OS keychain.Changes
src/lib/config.tsstoreApiKeyAsync— IfwriteCredentials()throws afterbackend.set()succeeds, the just-written backend secret is cleaned up viabackend.delete()before the error is re-thrown.removeApiKeyAsync— The key is read from the backend before deletion.backend.delete()return value is now checked; afalsereturn is treated as a hard failure. If the subsequent file mutation (removeApiKey) fails, the key is restored in the backend.removeAllApiKeysAsync— Allbackend.delete()results are aggregated usingPromise.allwith individual result tracking. The credentials file is only removed when every backend delete succeeds. Partial failures throw with a descriptive error listing the failed profiles.renameProfileAsync—backend.delete()return value is verified. If the old-name delete fails, the new-name entry is cleaned up. If the file-levelrenameProfile()fails after successful backend operations, the backend state is restored (old key re-set, new key deleted).tests/lib/config-async.test.tsit()instead oftest()per coding standardsdeletereturn values fromundefinedtotrueto match the new verification behaviorstoreApiKeyAsync: backend rollback on file write failureremoveApiKeyAsync: throws on false delete return; restores key on file failureremoveAllApiKeysAsync: preserves credentials file when any backend delete failsrenameProfileAsync: cleans up new-name on old-name delete failure; restores old-name on file write failureVerification
Linear Issue: BU-632
Summary by cubic
Adds transactional rollback to secure-storage credential operations so the OS keychain and
credentials.jsonstay in sync. Fixes BU-632 by preventing stranded profiles and orphaned secrets on partial failures.storeApiKeyAsync: roll backbackend.set()if writingcredentials.jsonfails.removeApiKeyAsync: requirebackend.delete()to returntrue; restore the key if file removal fails.removeAllApiKeysAsync: remove the file only if all backend deletes succeed; throw with failed profile names otherwise.renameProfileAsync: verify old-name delete; clean up new-name or restore old-name if file rename fails.Written for commit 10df862. Summary will update on new commits.