diff --git a/util/api_key_helper.go b/util/api_key_helper.go index 0e1ea9a..d2271c1 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,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, diff --git a/util/api_key_helper_test.go b/util/api_key_helper_test.go index 64cc667..21bdc90 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" ) +// 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") } }