feat(cache): migrate API key helper cache to OS credential store#264
feat(cache): migrate API key helper cache to OS credential store#264
Conversation
- Replace file-based API key caching with OS credential store backed storage - Remove cache directory and file path handling in favor of hashed credstore keys - Introduce a namespaced credstore key format for helper command caches - Simplify cache serialization and adjust error handling for credstore operations - Update documentation comments to reflect keyring-based caching behavior - Adapt tests to use credstore cleanup and validation instead of filesystem checks - Replace file permission tests with verification of stored credstore contents Signed-off-by: Bo-Yi Wu <appleboy.tw@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR migrates the api_key_helper cache from per-command plaintext JSON files to a credstore-backed secure store (OS keyring with file fallback), improving how dynamically generated API keys are persisted.
Changes:
- Replace file-based helper cache with
GetCredential/SetCredentialstorage keyed by a namespaced SHA-256 (helper:<sha256hex>). - Remove cache directory/file helpers and related filesystem imports.
- Update tests to validate credstore storage and clean up helper cache entries.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| util/api_key_helper.go | Switch helper cache read/write to credstore keys and add helperKeyPrefix + helperCacheKey() helper. |
| util/api_key_helper_test.go | Update caching tests to clean up credstore entries and validate stored JSON content. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func TestGetAPIKeyFromHelperWithCache_WithCaching(t *testing.T) { | ||
| // Create a temporary directory for testing | ||
| // Use a counter file to generate different values each time the command runs. | ||
| // The tmpDir path is included in the command, making the credstore key unique per run. | ||
| tmpDir := t.TempDir() | ||
|
|
||
| // Override home directory for testing | ||
| t.Setenv("HOME", tmpDir) | ||
|
|
||
| // Use a counter file to generate different values each time the command runs | ||
| counterFile := filepath.Join(tmpDir, "counter.txt") |
There was a problem hiding this comment.
These cache tests still use the global credStore instance, which can write into the developer/CI OS keyring and/or the real fallback file under ~/.config/.... To keep tests hermetic (see newTestCredStore pattern in util/credstore_test.go), override credStore to a temp file-backed store at the start of the test and restore it via t.Cleanup.
| func TestGetAPIKeyFromHelperWithCache_StoresInCredstore(t *testing.T) { | ||
| command := "echo 'test-credstore-storage'" | ||
| t.Cleanup(func() { cleanupHelperCache(t, command) }) | ||
|
|
||
| command := "echo 'test-permissions'" | ||
|
|
||
| // Execute command to create cache file | ||
| // Execute command to populate credstore cache | ||
| _, err := GetAPIKeyFromHelperWithCache(context.Background(), command, 5*time.Second) |
There was a problem hiding this comment.
This test only deletes the cache entry in t.Cleanup, so a pre-existing credstore entry for the same command could make the test pass without exercising the write path. Consider ensuring a clean state first (e.g., delete the key before calling GetAPIKeyFromHelperWithCache, or use a per-test temp credStore instance).
| cache := apiKeyCache{ | ||
| APIKey: apiKey, | ||
| LastFetchTime: time.Now(), | ||
| HelperCmd: helperCmd, | ||
| } |
There was a problem hiding this comment.
writeCache persists the full helperCmd string inside the cached JSON (HelperCmd: helperCmd). Since the credstore key is already derived from the helper command hash, storing the raw command is redundant and can unintentionally persist sensitive data if the command embeds secrets (especially in the file-fallback backend). Consider removing HelperCmd from the cached payload (or storing only a hash) and dropping the equality check in readCache.
- Replace cleanupHelperCache with overrideCredStore pattern (mirrors credstore_test.go) so cache tests never touch the OS keyring or real credential files - Remove HelperCmd field from apiKeyCache: the credstore key already encodes the command via SHA-256, and storing the raw command string risks persisting sensitive arguments in the file-fallback backend - Drop the now-redundant HelperCmd equality check in readCache Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Summary
~/.config/codegpt/.cache/<sha256>.json) withcredstore-backed storage (OS keyring with file fallback)helperCacheKey()to produce namespaced credstore keys in the formathelper:<sha256hex>getCacheDir()andgetCacheFilePath()helpers; dropfmt,os,path/filepathimports"helper:"namespace prefix to a named constanthelperKeyPrefixSecurity improvement
API keys retrieved via
api_key_helperwere previously cached as plaintext JSON. They are now protected by the OS keyring (macOS Keychain / Linux Secret Service / Windows Credential Manager), falling back to the existingcredentials.jsonfile store when no keyring is available.Test plan
TestGetAPIKeyFromHelperWithCache_WithCaching— cache hit returns same valueTestGetAPIKeyFromHelperWithCache_CacheExpiration— expired cache fetches fresh valueTestGetAPIKeyFromHelperWithCache_DifferentCommands— distinct commands produce distinct keysTestGetAPIKeyFromHelperWithCache_StoresInCredstore— replaced file-permission test; verifies entry is written to credstore and JSON is well-formedmake buildpassesmake testpassesmake lintpasses (0 issues)🤖 Generated with Claude Code