From 57ef2805264f4bdbefb00616de285fd08bd71b45 Mon Sep 17 00:00:00 2001 From: Bo-Yi Wu Date: Wed, 11 Mar 2026 18:40:12 +0800 Subject: [PATCH 1/2] feat(cache): migrate API key caching to OS credential store - 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 --- util/api_key_helper.go | 78 ++++++++++--------------------------- util/api_key_helper_test.go | 72 ++++++++++++++++------------------ 2 files changed, 55 insertions(+), 95 deletions(-) diff --git a/util/api_key_helper.go b/util/api_key_helper.go index 0e1ea9a..5f5ec4a 100644 --- a/util/api_key_helper.go +++ b/util/api_key_helper.go @@ -6,9 +6,6 @@ import ( "encoding/hex" "encoding/json" "errors" - "fmt" - "os" - "path/filepath" "time" ) @@ -17,6 +14,9 @@ const ( HelperTimeout = 10 * time.Second // DefaultRefreshInterval is the default interval for refreshing API keys DefaultRefreshInterval = 900 * time.Second // 15 minutes + + // helperKeyPrefix is the credstore key namespace for helper command cache entries. + helperKeyPrefix = "helper:" ) // apiKeyCache stores cached API keys with their metadata @@ -26,52 +26,26 @@ type apiKeyCache struct { HelperCmd string `json:"helperCmd"` } -// getCacheDir returns the cache directory path -func getCacheDir() (string, error) { - home, err := os.UserHomeDir() - if err != nil { - return "", fmt.Errorf("failed to get home directory: %w", err) - } - cacheDir := filepath.Join(home, ".config", "codegpt", ".cache") - return cacheDir, nil -} - -// getCacheFilePath returns the cache file path for a given helper command -func getCacheFilePath(helperCmd string) (string, error) { - cacheDir, err := getCacheDir() - if err != nil { - return "", err - } - - // Create cache directory if it doesn't exist - if err := os.MkdirAll(cacheDir, 0o700); err != nil { - return "", fmt.Errorf("failed to create cache directory: %w", err) - } - - // Use hash of helper command as filename +// helperCacheKey returns the credstore key for a given helper command. +func helperCacheKey(helperCmd string) string { hash := sha256.Sum256([]byte(helperCmd)) - filename := hex.EncodeToString(hash[:]) + ".json" - return filepath.Join(cacheDir, filename), nil + return helperKeyPrefix + hex.EncodeToString(hash[:]) } -// readCache reads the cached API key from file +// readCache reads the cached API key from credstore. func readCache(helperCmd string) (*apiKeyCache, error) { - cachePath, err := getCacheFilePath(helperCmd) + key := helperCacheKey(helperCmd) + val, err := GetCredential(key) if err != nil { return nil, err } - - data, err := os.ReadFile(cachePath) - if err != nil { - if os.IsNotExist(err) { - return nil, nil //nolint:nilnil // nil cache indicates cache miss, not an error - } - return nil, fmt.Errorf("failed to read cache file: %w", err) + if val == "" { + return nil, nil //nolint:nilnil // nil cache indicates cache miss, not an error } var cache apiKeyCache - if err := json.Unmarshal(data, &cache); err != nil { - return nil, fmt.Errorf("failed to parse cache file: %w", err) + if err := json.Unmarshal([]byte(val), &cache); err != nil { + return nil, err } // Verify the helper command matches @@ -82,31 +56,21 @@ func readCache(helperCmd string) (*apiKeyCache, error) { return &cache, nil } -// writeCache writes the API key cache to file +// writeCache writes the API key cache to credstore. func writeCache(helperCmd, apiKey string) error { - cachePath, err := getCacheFilePath(helperCmd) - if err != nil { - return err - } - + key := helperCacheKey(helperCmd) cache := apiKeyCache{ APIKey: apiKey, LastFetchTime: time.Now(), HelperCmd: helperCmd, } - //nolint:gosec // G117: intentional API key cache with 0600 perms - data, err := json.MarshalIndent(cache, "", " ") + data, err := json.Marshal(cache) if err != nil { - return fmt.Errorf("failed to marshal cache: %w", err) - } - - // Write with restrictive permissions (only owner can read/write) - if err := os.WriteFile(cachePath, data, 0o600); err != nil { - return fmt.Errorf("failed to write cache file: %w", err) + return err } - return nil + return SetCredential(key, string(data)) } // needsRefresh checks if the cached key needs to be refreshed @@ -137,10 +101,11 @@ func needsRefresh(cache *apiKeyCache, refreshInterval time.Duration) bool { // Security note: The returned API key is sensitive and should not be logged. // GetAPIKeyFromHelperWithCache executes a shell command to dynamically generate an API key, -// with file-based caching support. The API key is cached for the specified refresh interval. +// with credstore-backed caching support. The API key is cached for the specified refresh interval. // If refreshInterval is 0, the cache is disabled and the command is executed every time. // -// The cache is stored in ~/.config/codegpt/.cache/ directory with restrictive permissions (0600). +// The cache is stored in the OS keyring (macOS Keychain / Linux Secret Service / +// Windows Credential Manager) with a file-based fallback. // // Parameters: // - ctx: Context for controlling execution and timeouts @@ -150,7 +115,6 @@ func needsRefresh(cache *apiKeyCache, refreshInterval time.Duration) bool { // Returns the API key from cache if still valid, otherwise executes the helper command. // // Security note: The returned API key is sensitive and should not be logged. -// Cache files are stored with 0600 permissions but contain the API key in JSON format. func GetAPIKeyFromHelperWithCache( ctx context.Context, helperCmd string, diff --git a/util/api_key_helper_test.go b/util/api_key_helper_test.go index 64cc667..6157f2a 100644 --- a/util/api_key_helper_test.go +++ b/util/api_key_helper_test.go @@ -2,6 +2,7 @@ package util import ( "context" + "encoding/json" "fmt" "os" "path/filepath" @@ -10,6 +11,16 @@ import ( "time" ) +// cleanupHelperCache removes helper cache entries from credstore for the given commands. +func cleanupHelperCache(t *testing.T, cmds ...string) { + t.Helper() + for _, cmd := range cmds { + if err := DeleteCredential(helperCacheKey(cmd)); err != nil { + t.Logf("cleanupHelperCache: failed to delete key for %q: %v", cmd, err) + } + } +} + func TestGetAPIKeyFromHelper_Success(t *testing.T) { tests := []struct { name string @@ -229,18 +240,15 @@ func TestGetAPIKeyFromHelperWithCache_NoCaching(t *testing.T) { } 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") command := fmt.Sprintf( "f=%s; echo $(($(cat $f 2>/dev/null || echo 0) + 1)) | tee $f", counterFile, ) + t.Cleanup(func() { cleanupHelperCache(t, command) }) // First call should execute and cache key1, err := GetAPIKeyFromHelperWithCache(context.Background(), command, 5*time.Second) @@ -263,15 +271,12 @@ func TestGetAPIKeyFromHelperWithCache_WithCaching(t *testing.T) { } func TestGetAPIKeyFromHelperWithCache_CacheExpiration(t *testing.T) { - // Create a temporary directory for testing + // Create a counter file that we'll update manually. + // 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) - - // Create a counter file that we'll update manually counterFile := filepath.Join(tmpDir, "counter2.txt") command := fmt.Sprintf("cat %q", counterFile) + t.Cleanup(func() { cleanupHelperCache(t, command) }) // Write initial value if err := os.WriteFile(counterFile, []byte("value1"), 0o600); err != nil { @@ -311,14 +316,9 @@ func TestGetAPIKeyFromHelperWithCache_CacheExpiration(t *testing.T) { } func TestGetAPIKeyFromHelperWithCache_DifferentCommands(t *testing.T) { - // Create a temporary directory for testing - tmpDir := t.TempDir() - - // Override home directory for testing - t.Setenv("HOME", tmpDir) - cmd1 := "echo 'key-one'" cmd2 := "echo 'key-two'" + t.Cleanup(func() { cleanupHelperCache(t, cmd1, cmd2) }) // Get keys from different commands key1, err := GetAPIKeyFromHelperWithCache(context.Background(), cmd1, 5*time.Second) @@ -344,35 +344,31 @@ func TestGetAPIKeyFromHelperWithCache_DifferentCommands(t *testing.T) { } } -func TestGetAPIKeyFromHelperWithCache_CacheFilePermissions(t *testing.T) { - // Create a temporary directory for testing - tmpDir := t.TempDir() - - // Override home directory for testing - t.Setenv("HOME", tmpDir) +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) if err != nil { t.Fatalf("GetAPIKeyFromHelperWithCache() error = %v", err) } - // Check cache file permissions - cachePath, err := getCacheFilePath(command) + // Verify the value was stored in credstore under the expected key + val, err := GetCredential(helperCacheKey(command)) if err != nil { - t.Fatalf("getCacheFilePath() error = %v", err) + t.Fatalf("GetCredential() error = %v", err) } - - info, err := os.Stat(cachePath) - if err != nil { - t.Fatalf("os.Stat() error = %v", err) + if val == "" { + t.Fatal("Expected credstore to contain cached entry, got empty string") } - // Check that file has restrictive permissions (0600) - perm := info.Mode().Perm() - if perm != 0o600 { - t.Errorf("Cache file should have 0600 permissions, got %o", perm) + // Verify the stored JSON contains the expected API key + var stored apiKeyCache + if err := json.Unmarshal([]byte(val), &stored); err != nil { + t.Fatalf("Failed to unmarshal stored credstore value: %v", err) + } + if stored.APIKey != "test-credstore-storage" { + t.Errorf("Stored APIKey = %q, want %q", stored.APIKey, "test-credstore-storage") } } From d197781a432089bc5e92fe5bd78ddcb98408744d Mon Sep 17 00:00:00 2001 From: Bo-Yi Wu Date: Wed, 11 Mar 2026 18:57:53 +0800 Subject: [PATCH 2/2] =?UTF-8?q?fix(cache):=20address=20PR=20review=20?= =?UTF-8?q?=E2=80=94=20isolate=20tests=20and=20drop=20HelperCmd=20field?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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) --- util/api_key_helper.go | 12 ++++-------- util/api_key_helper_test.go | 25 ++++++++++++------------- 2 files changed, 16 insertions(+), 21 deletions(-) diff --git a/util/api_key_helper.go b/util/api_key_helper.go index 5f5ec4a..d2271c1 100644 --- a/util/api_key_helper.go +++ b/util/api_key_helper.go @@ -19,11 +19,13 @@ const ( helperKeyPrefix = "helper:" ) -// apiKeyCache stores cached API keys with their metadata +// apiKeyCache stores cached API keys with their metadata. +// HelperCmd is intentionally omitted: the credstore key already encodes the +// command via SHA-256, so storing the raw command string here would +// unnecessarily persist potentially sensitive command-line arguments. type apiKeyCache struct { APIKey string `json:"apiKey"` LastFetchTime time.Time `json:"lastFetchTime"` - HelperCmd string `json:"helperCmd"` } // helperCacheKey returns the credstore key for a given helper command. @@ -48,11 +50,6 @@ func readCache(helperCmd string) (*apiKeyCache, error) { return nil, err } - // Verify the helper command matches - if cache.HelperCmd != helperCmd { - return nil, nil //nolint:nilnil // nil cache indicates cache miss, not an error - } - return &cache, nil } @@ -62,7 +59,6 @@ func writeCache(helperCmd, apiKey string) error { cache := apiKeyCache{ APIKey: apiKey, LastFetchTime: time.Now(), - HelperCmd: helperCmd, } data, err := json.Marshal(cache) diff --git a/util/api_key_helper_test.go b/util/api_key_helper_test.go index 6157f2a..21bdc90 100644 --- a/util/api_key_helper_test.go +++ b/util/api_key_helper_test.go @@ -11,14 +11,14 @@ import ( "time" ) -// cleanupHelperCache removes helper cache entries from credstore for the given commands. -func cleanupHelperCache(t *testing.T, cmds ...string) { +// overrideCredStore replaces the global credStore with an isolated file-backed +// store backed by a temp directory, then restores the original on cleanup. +// This prevents tests from touching the OS keyring or real credential files. +func overrideCredStore(t *testing.T) { t.Helper() - for _, cmd := range cmds { - if err := DeleteCredential(helperCacheKey(cmd)); err != nil { - t.Logf("cleanupHelperCache: failed to delete key for %q: %v", cmd, err) - } - } + original := credStore + t.Cleanup(func() { credStore = original }) + credStore = newTestCredStore(t) } func TestGetAPIKeyFromHelper_Success(t *testing.T) { @@ -218,6 +218,7 @@ func TestGetAPIKeyFromHelper_SecurityStderr(t *testing.T) { } func TestGetAPIKeyFromHelperWithCache_NoCaching(t *testing.T) { + overrideCredStore(t) // Test with refreshInterval = 0 (no caching) command := "echo 'test-key-no-cache'" @@ -240,15 +241,14 @@ func TestGetAPIKeyFromHelperWithCache_NoCaching(t *testing.T) { } func TestGetAPIKeyFromHelperWithCache_WithCaching(t *testing.T) { + overrideCredStore(t) // 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() counterFile := filepath.Join(tmpDir, "counter.txt") command := fmt.Sprintf( "f=%s; echo $(($(cat $f 2>/dev/null || echo 0) + 1)) | tee $f", counterFile, ) - t.Cleanup(func() { cleanupHelperCache(t, command) }) // First call should execute and cache key1, err := GetAPIKeyFromHelperWithCache(context.Background(), command, 5*time.Second) @@ -271,12 +271,11 @@ func TestGetAPIKeyFromHelperWithCache_WithCaching(t *testing.T) { } func TestGetAPIKeyFromHelperWithCache_CacheExpiration(t *testing.T) { + overrideCredStore(t) // Create a counter file that we'll update manually. - // The tmpDir path is included in the command, making the credstore key unique per run. tmpDir := t.TempDir() counterFile := filepath.Join(tmpDir, "counter2.txt") command := fmt.Sprintf("cat %q", counterFile) - t.Cleanup(func() { cleanupHelperCache(t, command) }) // Write initial value if err := os.WriteFile(counterFile, []byte("value1"), 0o600); err != nil { @@ -316,9 +315,9 @@ func TestGetAPIKeyFromHelperWithCache_CacheExpiration(t *testing.T) { } func TestGetAPIKeyFromHelperWithCache_DifferentCommands(t *testing.T) { + overrideCredStore(t) cmd1 := "echo 'key-one'" cmd2 := "echo 'key-two'" - t.Cleanup(func() { cleanupHelperCache(t, cmd1, cmd2) }) // Get keys from different commands key1, err := GetAPIKeyFromHelperWithCache(context.Background(), cmd1, 5*time.Second) @@ -345,8 +344,8 @@ func TestGetAPIKeyFromHelperWithCache_DifferentCommands(t *testing.T) { } func TestGetAPIKeyFromHelperWithCache_StoresInCredstore(t *testing.T) { + overrideCredStore(t) command := "echo 'test-credstore-storage'" - t.Cleanup(func() { cleanupHelperCache(t, command) }) // Execute command to populate credstore cache _, err := GetAPIKeyFromHelperWithCache(context.Background(), command, 5*time.Second)