feat(cache): migrate API key helper cache to OS credential store

util/api_key_helper.go

@@ -6,9 +6,6 @@ import (
 	"encoding/hex"
 	"encoding/json"
 	"errors"
-	"fmt"
-	"os"
-	"path/filepath"
 	"time"
 )
 
@@ -17,96 +14,59 @@ 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
+// 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"`
 }
 
-// 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)
-	}
-
-	// Verify the helper command matches
-	if cache.HelperCmd != helperCmd {
-		return nil, nil //nolint:nilnil // nil cache indicates cache miss, not an error
+	if err := json.Unmarshal([]byte(val), &cache); err != nil {
+		return nil, err
 	}
 
 	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 +97,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 +111,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,

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"
 )
 
+// 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()
+	original := credStore
+	t.Cleanup(func() { credStore = original })
+	credStore = newTestCredStore(t)
+}
+
 func TestGetAPIKeyFromHelper_Success(t *testing.T) {
 	tests := []struct {
 		name     string
@@ -207,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'"
 
@@ -229,13 +241,9 @@ func TestGetAPIKeyFromHelperWithCache_NoCaching(t *testing.T) {
 }
 
 func TestGetAPIKeyFromHelperWithCache_WithCaching(t *testing.T) {
-	// Create a temporary directory for testing
+	overrideCredStore(t)
+	// Use a counter file to generate different values each time the command runs.
 	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",
@@ -263,13 +271,9 @@ func TestGetAPIKeyFromHelperWithCache_WithCaching(t *testing.T) {
 }
 
 func TestGetAPIKeyFromHelperWithCache_CacheExpiration(t *testing.T) {
-	// Create a temporary directory for testing
+	overrideCredStore(t)
+	// Create a counter file that we'll update manually.
 	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)
 
@@ -311,12 +315,7 @@ 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)
-
+	overrideCredStore(t)
 	cmd1 := "echo 'key-one'"
 	cmd2 := "echo 'key-two'"
 
@@ -344,35 +343,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)
-
-	command := "echo 'test-permissions'"
+func TestGetAPIKeyFromHelperWithCache_StoresInCredstore(t *testing.T) {
+	overrideCredStore(t)
+	command := "echo 'test-credstore-storage'"
 
-	// 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")
 	}
 }