fix: persist a credential before caching it in AddOrReplace [patch] - #146
Merged
Merged
Conversation
AddOrReplace wrote the credential into the in-memory dictionary before calling Store.Save. When a native store rejected the write (an oversized blob on Windows Credential Manager, a transient keychain or libsecret failure), the caller saw the exception but a later TryGet still returned the credential from memory, so the process behaved as though it had been durably stored until a restart or a second cache instance revealed it was never written. Save first, then update the cache, so a failed save leaves the cache holding exactly what the store holds. Fixes #145 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016dQa3DK7HjcyQkRaYBxumh
|
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
1 participant
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.
Fixes #145
Problem
CredentialCache.AddOrReplacewrote the credential into_credentialsbefore callingStore.Save:Every native store in this repo throws on failure —
WindowsCredentialStore.SaveraisesCredentialStoreExceptionwhen the serialized blob exceedsCRED_MAX_CREDENTIAL_BLOB_SIZEorCredWriteWfails, andMacOsCredentialStore/LinuxSecretServiceCredentialStoredo the same on native API failure. When that happened the caller saw the exception and reasonably concluded nothing was saved, but a laterTryGetstill returned the credential from the in-memory dictionary, becauseTryGetshort-circuits on_credentialsand never consults the store once an entry exists. The process then behaved as though the credential were durably stored, and only a restart — or a secondCredentialCacheover the same store, the scenario the singleton/ConfigureStoreAPI exists to support — revealed that it had never been written.Fix
Persist first, update the cache only on success. A failed save now leaves the cache holding exactly what the store holds: nothing for a new persona, or the previously persisted credential when a replacement fails.
Tests
Two regression tests in
CredentialCacheTests, backed by a newThrowingCredentialStorefake (anInMemoryCredentialStorewrapper with aThrowOnSaveswitch):AddOrReplaceDoesNotCacheCredentialWhenStoreSaveThrows— after a failed save of a new persona,TryGetreturnsfalse.AddOrReplaceKeepsThePersistedCredentialWhenReplacementFailsToSave— after a failed replacement,TryGetreturns the credential the store actually holds, not the one whose save failed.Both were verified to fail against the old ordering and pass with the fix. Full suite on Linux: 24 total, 19 passed, 5 skipped (the pre-existing native-store tests, which are inconclusive off a platform with a native secret service).
🤖 Generated with Claude Code
https://claude.ai/code/session_016dQa3DK7HjcyQkRaYBxumh
Generated by Claude Code