From 6211b8e768220210bbc221f2ca56c08b8a5ec2a4 Mon Sep 17 00:00:00 2001 From: clawbot Date: Sun, 15 Feb 2026 14:05:15 -0800 Subject: [PATCH] Return error from GetDefaultStateDir when home directory unavailable When os.UserConfigDir() fails, DetermineStateDir falls back to os.UserHomeDir(). Previously the error from UserHomeDir was discarded, which could result in a dangerous root-relative path (/.config/...) if both calls fail. Now DetermineStateDir returns (string, error) and propagates failures from both UserConfigDir and UserHomeDir. Closes #14 --- internal/cli/cli.go | 10 +++++-- internal/cli/cli_test.go | 10 +++++-- internal/secret/helpers.go | 18 +++++++----- internal/secret/helpers_test.go | 50 +++++++++++++++++++++++++++++++++ 4 files changed, 77 insertions(+), 11 deletions(-) create mode 100644 internal/secret/helpers_test.go diff --git a/internal/cli/cli.go b/internal/cli/cli.go index 8b79612..38b44e6 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -19,7 +19,10 @@ type Instance struct { // NewCLIInstance creates a new CLI instance with the real filesystem func NewCLIInstance() *Instance { fs := afero.NewOsFs() - stateDir := secret.DetermineStateDir("") + stateDir, err := secret.DetermineStateDir("") + if err != nil { + panic(fmt.Sprintf("cannot determine state directory: %v", err)) + } return &Instance{ fs: fs, @@ -29,7 +32,10 @@ func NewCLIInstance() *Instance { // NewCLIInstanceWithFs creates a new CLI instance with the given filesystem (for testing) func NewCLIInstanceWithFs(fs afero.Fs) *Instance { - stateDir := secret.DetermineStateDir("") + stateDir, err := secret.DetermineStateDir("") + if err != nil { + panic(fmt.Sprintf("cannot determine state directory: %v", err)) + } return &Instance{ fs: fs, diff --git a/internal/cli/cli_test.go b/internal/cli/cli_test.go index 8e5b3eb..f03a592 100644 --- a/internal/cli/cli_test.go +++ b/internal/cli/cli_test.go @@ -41,7 +41,10 @@ func TestDetermineStateDir(t *testing.T) { testEnvDir := "/test-env-dir" t.Setenv(secret.EnvStateDir, testEnvDir) - stateDir := secret.DetermineStateDir("") + stateDir, err := secret.DetermineStateDir("") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } if stateDir != testEnvDir { t.Errorf("Expected state directory %q from environment, got %q", testEnvDir, stateDir) } @@ -49,7 +52,10 @@ func TestDetermineStateDir(t *testing.T) { // Test with custom config dir _ = os.Unsetenv(secret.EnvStateDir) customConfigDir := "/custom-config" - stateDir = secret.DetermineStateDir(customConfigDir) + stateDir, err = secret.DetermineStateDir(customConfigDir) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } expectedDir := filepath.Join(customConfigDir, secret.AppID) if stateDir != expectedDir { t.Errorf("Expected state directory %q with custom config, got %q", expectedDir, stateDir) diff --git a/internal/secret/helpers.go b/internal/secret/helpers.go index 26bd7e0..f7a7263 100644 --- a/internal/secret/helpers.go +++ b/internal/secret/helpers.go @@ -28,16 +28,17 @@ func generateRandomString(length int, charset string) (string, error) { return string(result), nil } -// DetermineStateDir determines the state directory based on environment variables and OS -func DetermineStateDir(customConfigDir string) string { +// DetermineStateDir determines the state directory based on environment variables and OS. +// It returns an error if no usable directory can be determined. +func DetermineStateDir(customConfigDir string) (string, error) { // Check for environment variable first if envStateDir := os.Getenv(EnvStateDir); envStateDir != "" { - return envStateDir + return envStateDir, nil } // Use custom config dir if provided if customConfigDir != "" { - return filepath.Join(customConfigDir, AppID) + return filepath.Join(customConfigDir, AppID), nil } // Use os.UserConfigDir() which handles platform-specific directories: @@ -47,10 +48,13 @@ func DetermineStateDir(customConfigDir string) string { configDir, err := os.UserConfigDir() if err != nil { // Fallback to a reasonable default if we can't determine user config dir - homeDir, _ := os.UserHomeDir() + homeDir, homeErr := os.UserHomeDir() + if homeErr != nil { + return "", fmt.Errorf("unable to determine state directory: config dir: %w, home dir: %w", err, homeErr) + } - return filepath.Join(homeDir, ".config", AppID) + return filepath.Join(homeDir, ".config", AppID), nil } - return filepath.Join(configDir, AppID) + return filepath.Join(configDir, AppID), nil } diff --git a/internal/secret/helpers_test.go b/internal/secret/helpers_test.go new file mode 100644 index 0000000..b989b2d --- /dev/null +++ b/internal/secret/helpers_test.go @@ -0,0 +1,50 @@ +package secret + +import ( + "testing" +) + +func TestDetermineStateDir_ErrorsWhenHomeDirUnavailable(t *testing.T) { + // Clear all env vars that could provide a home/config directory. + // On Darwin, os.UserHomeDir may still succeed via the password + // database, so we also test via an explicit empty-customConfigDir + // path to exercise the fallback branch. + t.Setenv(EnvStateDir, "") + t.Setenv("HOME", "") + t.Setenv("XDG_CONFIG_HOME", "") + + result, err := DetermineStateDir("") + // On systems where both lookups fail, we must get an error. + // On systems where the OS provides a fallback (e.g. macOS pw db), + // result should still be valid (non-empty, not root-relative). + if err != nil { + // Good — the error case is handled. + return + } + if result == "/.config/"+AppID || result == "" { + t.Errorf("DetermineStateDir returned dangerous/empty path %q without error", result) + } +} + +func TestDetermineStateDir_UsesEnvVar(t *testing.T) { + t.Setenv(EnvStateDir, "/custom/state") + result, err := DetermineStateDir("") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if result != "/custom/state" { + t.Errorf("expected /custom/state, got %q", result) + } +} + +func TestDetermineStateDir_UsesCustomConfigDir(t *testing.T) { + t.Setenv(EnvStateDir, "") + result, err := DetermineStateDir("/my/config") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + expected := "/my/config/" + AppID + if result != expected { + t.Errorf("expected %q, got %q", expected, result) + } +}