From 04091403a576c6094c6b4dafdc79d7b2fa6b5751 Mon Sep 17 00:00:00 2001 From: Patrick Dawkins Date: Thu, 1 Oct 2026 21:21:55 +0100 Subject: [PATCH 1/7] fix(config): resolve the home and writable dirs like the legacy CLI Go and PHP share the writable dir: Go now keeps its credentials there and cleans up the legacy CLI's files in it. Resolve it the same way: - The home dir comes from HOME, then HOME, then USERPROFILE. On Windows HOME can differ from USERPROFILE, e.g. in MSYS2. - If the writable dir cannot be written, e.g. on an application container, use /. Otherwise the migration could miss the legacy sessions, and logout could leave the legacy SSH certificate and API cache in place. Co-Authored-By: Claude Opus 5.5 --- internal/config/dir.go | 36 ++++++++++++++++++++---- internal/config/dir_test.go | 56 +++++++++++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 6 deletions(-) create mode 100644 internal/config/dir_test.go diff --git a/internal/config/dir.go b/internal/config/dir.go index f366cacf..155d7ada 100644 --- a/internal/config/dir.go +++ b/internal/config/dir.go @@ -59,7 +59,10 @@ func (c *Config) TempDir() (string, error) { return path, nil } -// WritableUserDir returns the path to a writable user-level directory. +// WritableUserDir returns the path to a writable user-level directory, e.g. for credentials and state. +// +// As in the legacy CLI, which shares it, a temporary directory is used if the directory in the home directory cannot +// be written, e.g. on an application container. // // Deprecated: unless backwards compatibility is desired, TempDir is preferable. func (c *Config) WritableUserDir() (string, error) { @@ -71,18 +74,39 @@ func (c *Config) WritableUserDir() (string, error) { return "", err } path := filepath.Join(hd, c.Application.WritableUserDir) - if err := os.MkdirAll(path, 0o700); err != nil { - return "", err + if err := mkdirWritable(path); err != nil { + path = filepath.Join(os.TempDir(), c.Application.TempSubDir) + if err := mkdirWritable(path); err != nil { + return "", err + } } c.writableUserDir = path return path, nil } -// HomeDir returns the home directory configured via an environment variable, or the OS's user home directory otherwise. +// mkdirWritable creates a directory if needed, and checks that files can be created in it. +func mkdirWritable(path string) error { + if err := os.MkdirAll(path, 0o700); err != nil { + return err + } + f, err := os.CreateTemp(path, ".write-test-*") + if err != nil { + return err + } + _ = f.Close() + return os.Remove(f.Name()) +} + +// HomeDir returns the user's home directory. +// +// It checks the same environment variables as the legacy CLI, in order: {ENV_PREFIX}HOME, HOME and USERPROFILE. +// On Windows, HOME can differ from USERPROFILE, e.g. in MSYS2 or Cygwin. func (c *Config) HomeDir() (string, error) { - if fromEnv := os.Getenv(c.Application.EnvPrefix + "HOME"); fromEnv != "" { - return fromEnv, nil + for _, name := range []string{c.Application.EnvPrefix + "HOME", "HOME", "USERPROFILE"} { + if v := os.Getenv(name); v != "" { + return v, nil + } } return os.UserHomeDir() } diff --git a/internal/config/dir_test.go b/internal/config/dir_test.go new file mode 100644 index 00000000..331d37aa --- /dev/null +++ b/internal/config/dir_test.go @@ -0,0 +1,56 @@ +package config_test + +import ( + "os" + "path/filepath" + "runtime" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/upsun/cli/internal/config" +) + +func TestHomeDir(t *testing.T) { + cases := []struct { + name string + env map[string]string + want string + }{ + {"prefixed var first", map[string]string{"EXAMPLE_CLI_HOME": "/a", "HOME": "/b", "USERPROFILE": "/c"}, "/a"}, + {"then HOME", map[string]string{"EXAMPLE_CLI_HOME": "", "HOME": "/b", "USERPROFILE": "/c"}, "/b"}, + {"then USERPROFILE", map[string]string{"EXAMPLE_CLI_HOME": "", "HOME": "", "USERPROFILE": "/c"}, "/c"}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + cnf, err := config.FromYAML([]byte(validConfig)) + require.NoError(t, err) + for k, v := range c.env { + t.Setenv(k, v) + } + home, err := cnf.HomeDir() + require.NoError(t, err) + assert.Equal(t, c.want, home) + }) + } +} + +func TestWritableUserDir_ReadOnlyHome(t *testing.T) { + if runtime.GOOS == "windows" || os.Geteuid() == 0 { + t.Skip("needs Unix permissions") + } + cnf, err := config.FromYAML([]byte(validConfig)) + require.NoError(t, err) + home := t.TempDir() + require.NoError(t, os.Chmod(home, 0o500)) + t.Cleanup(func() { _ = os.Chmod(home, 0o700) }) + tmp := t.TempDir() + t.Setenv("EXAMPLE_CLI_HOME", home) + t.Setenv("TMPDIR", tmp) + + // As in the legacy CLI, a temporary directory is used. + dir, err := cnf.WritableUserDir() + require.NoError(t, err) + assert.Equal(t, filepath.Join(tmp, "example-cli-tmp"), dir) +} From 34d49144fa753b593913c0c6e54859f12933a236 Mon Sep 17 00:00:00 2001 From: Patrick Dawkins Date: Thu, 1 Oct 2026 21:27:09 +0100 Subject: [PATCH 2/7] fix(config): choose the writable dir by permissions, like the legacy CLI Fall back to the temp dir only when the directory is not writable by permissions (access W_OK, or the read-only attribute on Windows), as PHP's Filesystem::canWrite does, rather than on any write failure. On a full disk or with Windows ACLs, Go and PHP otherwise chose different directories. Co-Authored-By: Claude Opus 5.5 --- internal/config/dir.go | 30 +++++++++++++++++------------- internal/config/dir_unix.go | 14 ++++++++++++++ internal/config/dir_windows.go | 8 ++++++++ 3 files changed, 39 insertions(+), 13 deletions(-) create mode 100644 internal/config/dir_unix.go create mode 100644 internal/config/dir_windows.go diff --git a/internal/config/dir.go b/internal/config/dir.go index 155d7ada..635d5168 100644 --- a/internal/config/dir.go +++ b/internal/config/dir.go @@ -74,28 +74,32 @@ func (c *Config) WritableUserDir() (string, error) { return "", err } path := filepath.Join(hd, c.Application.WritableUserDir) - if err := mkdirWritable(path); err != nil { + if !canWrite(path) { path = filepath.Join(os.TempDir(), c.Application.TempSubDir) - if err := mkdirWritable(path); err != nil { - return "", err - } + } + if err := os.MkdirAll(path, 0o700); err != nil { + return "", err } c.writableUserDir = path return path, nil } -// mkdirWritable creates a directory if needed, and checks that files can be created in it. -func mkdirWritable(path string) error { - if err := os.MkdirAll(path, 0o700); err != nil { - return err +// canWrite checks whether a directory is writable, or can be created, using permissions only. +// +// This matches the legacy CLI (Filesystem::canWrite), so both choose the same directory, e.g. even on a full disk. +func canWrite(path string) bool { + if info, err := os.Stat(path); err == nil { + return info.IsDir() && isWritable(path, info) } - f, err := os.CreateTemp(path, ".write-test-*") - if err != nil { - return err + for p := filepath.Dir(path); ; p = filepath.Dir(p) { + if info, err := os.Stat(p); err == nil { + return isWritable(p, info) + } + if filepath.Dir(p) == p { + return false + } } - _ = f.Close() - return os.Remove(f.Name()) } // HomeDir returns the user's home directory. diff --git a/internal/config/dir_unix.go b/internal/config/dir_unix.go new file mode 100644 index 00000000..9e5e65c1 --- /dev/null +++ b/internal/config/dir_unix.go @@ -0,0 +1,14 @@ +//go:build !windows + +package config + +import ( + "os" + + "golang.org/x/sys/unix" +) + +// isWritable checks write permission, like PHP's is_writable. +func isWritable(path string, _ os.FileInfo) bool { + return unix.Access(path, unix.W_OK) == nil +} diff --git a/internal/config/dir_windows.go b/internal/config/dir_windows.go new file mode 100644 index 00000000..67fc17d4 --- /dev/null +++ b/internal/config/dir_windows.go @@ -0,0 +1,8 @@ +package config + +import "os" + +// isWritable checks the read-only attribute, like PHP's is_writable on Windows. +func isWritable(_ string, info os.FileInfo) bool { + return info.Mode().Perm()&0o200 != 0 +} From ff17b2b935c0fba7cd274dc6d2b17e81693e395a Mon Sep 17 00:00:00 2001 From: Patrick Dawkins Date: Thu, 1 Oct 2026 21:32:04 +0100 Subject: [PATCH 3/7] test(config): cover a file in place of the writable dir Co-Authored-By: Claude Opus 5.5 --- internal/config/dir_test.go | 51 ++++++++++++++++++++++++++----------- 1 file changed, 36 insertions(+), 15 deletions(-) diff --git a/internal/config/dir_test.go b/internal/config/dir_test.go index 331d37aa..e0886c44 100644 --- a/internal/config/dir_test.go +++ b/internal/config/dir_test.go @@ -36,21 +36,42 @@ func TestHomeDir(t *testing.T) { } } -func TestWritableUserDir_ReadOnlyHome(t *testing.T) { - if runtime.GOOS == "windows" || os.Geteuid() == 0 { - t.Skip("needs Unix permissions") +// TestWritableUserDir_TempFallback checks the cases where the legacy CLI uses a temporary directory instead. +func TestWritableUserDir_TempFallback(t *testing.T) { + cases := []struct { + name string + setup func(t *testing.T, home string) + }{ + { + name: "read-only home", + setup: func(t *testing.T, home string) { + if runtime.GOOS == "windows" || os.Geteuid() == 0 { + t.Skip("needs Unix permissions") + } + require.NoError(t, os.Chmod(home, 0o500)) + t.Cleanup(func() { _ = os.Chmod(home, 0o700) }) + }, + }, + { + name: "a file in place of the directory", + setup: func(t *testing.T, home string) { + require.NoError(t, os.WriteFile(filepath.Join(home, ".example-cli"), nil, 0o600)) + }, + }, } - cnf, err := config.FromYAML([]byte(validConfig)) - require.NoError(t, err) - home := t.TempDir() - require.NoError(t, os.Chmod(home, 0o500)) - t.Cleanup(func() { _ = os.Chmod(home, 0o700) }) - tmp := t.TempDir() - t.Setenv("EXAMPLE_CLI_HOME", home) - t.Setenv("TMPDIR", tmp) + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + cnf, err := config.FromYAML([]byte(validConfig)) + require.NoError(t, err) + home := t.TempDir() + c.setup(t, home) + tmp := t.TempDir() + t.Setenv("EXAMPLE_CLI_HOME", home) + t.Setenv("TMPDIR", tmp) - // As in the legacy CLI, a temporary directory is used. - dir, err := cnf.WritableUserDir() - require.NoError(t, err) - assert.Equal(t, filepath.Join(tmp, "example-cli-tmp"), dir) + dir, err := cnf.WritableUserDir() + require.NoError(t, err) + assert.Equal(t, filepath.Join(tmp, "example-cli-tmp"), dir) + }) + } } From c5ca72a0e5fa85fe3c9a28296d17cb8846677782 Mon Sep 17 00:00:00 2001 From: Patrick Dawkins Date: Thu, 1 Oct 2026 21:35:12 +0100 Subject: [PATCH 4/7] test(config): set the temp dir on Windows too Co-Authored-By: Claude Opus 5.5 --- internal/config/dir_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/config/dir_test.go b/internal/config/dir_test.go index e0886c44..8e4b8a84 100644 --- a/internal/config/dir_test.go +++ b/internal/config/dir_test.go @@ -67,7 +67,8 @@ func TestWritableUserDir_TempFallback(t *testing.T) { c.setup(t, home) tmp := t.TempDir() t.Setenv("EXAMPLE_CLI_HOME", home) - t.Setenv("TMPDIR", tmp) + t.Setenv("TMPDIR", tmp) // Unix + t.Setenv("TMP", tmp) // Windows dir, err := cnf.WritableUserDir() require.NoError(t, err) From 22f810dfc1d739a4119433082c30a072be387bf8 Mon Sep 17 00:00:00 2001 From: Patrick Dawkins Date: Fri, 2 Oct 2026 17:46:30 +0100 Subject: [PATCH 5/7] fix(config): only use temporary and writable dirs private to the user The writable dir can fall back to a shared /tmp, and the temp dir can be in one too, e.g. if XDG_CACHE_HOME=/tmp or on a read-only filesystem. Another user could create the directory first, then read or replace the CLI's state, credentials or PHP binary. On Unix, both directories must now be owned by the current user, as must any symlink to them. Group and other permissions are removed. Windows temporary directories are already per user. Co-Authored-By: Claude Opus 5.5 --- internal/config/dir.go | 10 +++- internal/config/dir_unix.go | 41 ++++++++++++++ internal/config/dir_unix_test.go | 95 ++++++++++++++++++++++++++++++++ internal/config/dir_windows.go | 5 ++ 4 files changed, 149 insertions(+), 2 deletions(-) create mode 100644 internal/config/dir_unix_test.go diff --git a/internal/config/dir.go b/internal/config/dir.go index 635d5168..8b50c314 100644 --- a/internal/config/dir.go +++ b/internal/config/dir.go @@ -11,7 +11,7 @@ import ( // TempDir returns the path to a user-specific temporary directory, suitable for caches. // -// It creates the temporary directory if it does not already exist. +// It creates the temporary directory if it does not already exist, and checks that it is private to the user. // // The directory can be specified in the {ENV_PREFIX}TMP environment variable. // @@ -54,6 +54,9 @@ func (c *Config) TempDir() (string, error) { return "", err } } + if err := ensurePrivateDir(path); err != nil { + return "", err + } c.tempDir = path return path, nil @@ -62,7 +65,7 @@ func (c *Config) TempDir() (string, error) { // WritableUserDir returns the path to a writable user-level directory, e.g. for credentials and state. // // As in the legacy CLI, which shares it, a temporary directory is used if the directory in the home directory cannot -// be written, e.g. on an application container. +// be written, e.g. on an application container. The directory must be private to the user. // // Deprecated: unless backwards compatibility is desired, TempDir is preferable. func (c *Config) WritableUserDir() (string, error) { @@ -80,6 +83,9 @@ func (c *Config) WritableUserDir() (string, error) { if err := os.MkdirAll(path, 0o700); err != nil { return "", err } + if err := ensurePrivateDir(path); err != nil { + return "", err + } c.writableUserDir = path return path, nil diff --git a/internal/config/dir_unix.go b/internal/config/dir_unix.go index 9e5e65c1..bdbfe646 100644 --- a/internal/config/dir_unix.go +++ b/internal/config/dir_unix.go @@ -3,7 +3,9 @@ package config import ( + "fmt" "os" + "syscall" "golang.org/x/sys/unix" ) @@ -12,3 +14,42 @@ import ( func isWritable(path string, _ os.FileInfo) bool { return unix.Access(path, unix.W_OK) == nil } + +// checkPrivateDir checks that a directory, and any symlink to it, is owned by the user (uid), and makes it private. +// +// This prevents another user from controlling the directory, e.g. if it is in a shared /tmp. +func checkPrivateDir(path string, uid int) error { + // G703: the path is the user's own config or temporary directory. + info, err := os.Lstat(path) //nolint:gosec + if err != nil { + return err + } + if info.Mode()&os.ModeSymlink != 0 { + if !ownedBy(info, uid) { + return fmt.Errorf("the symlink is not owned by the current user: %s", path) + } + if info, err = os.Stat(path); err != nil { //nolint:gosec // G703: as above + return err + } + } + if !info.IsDir() { + return fmt.Errorf("not a directory: %s", path) + } + if !ownedBy(info, uid) { + return fmt.Errorf("the directory is not owned by the current user: %s", path) + } + if info.Mode().Perm()&0o077 != 0 { + return os.Chmod(path, 0o700) //nolint:gosec // G703: as above + } + return nil +} + +func ownedBy(info os.FileInfo, uid int) bool { + st, ok := info.Sys().(*syscall.Stat_t) + return ok && int(st.Uid) == uid +} + +// ensurePrivateDir checks that a directory is private to the current user. +func ensurePrivateDir(path string) error { + return checkPrivateDir(path, os.Geteuid()) +} diff --git a/internal/config/dir_unix_test.go b/internal/config/dir_unix_test.go new file mode 100644 index 00000000..4367aba6 --- /dev/null +++ b/internal/config/dir_unix_test.go @@ -0,0 +1,95 @@ +//go:build !windows + +package config + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestCheckPrivateDir(t *testing.T) { + cases := []struct { + name string + setup func(t *testing.T, path string) + uid int + wantErr string + wantMode os.FileMode + }{ + { + name: "private directory", + setup: func(t *testing.T, path string) { require.NoError(t, os.Mkdir(path, 0o700)) }, + uid: os.Geteuid(), + wantMode: 0o700, + }, + { + name: "shared directory is tightened", + setup: func(t *testing.T, path string) { + require.NoError(t, os.Mkdir(path, 0o700)) + require.NoError(t, os.Chmod(path, 0o777)) + }, + uid: os.Geteuid(), + wantMode: 0o700, + }, + { + name: "owned by another user", + setup: func(t *testing.T, path string) { require.NoError(t, os.Mkdir(path, 0o700)) }, + uid: os.Geteuid() + 1, + wantErr: "the directory is not owned by the current user", + }, + { + name: "own symlink", + setup: func(t *testing.T, path string) { + target := filepath.Join(t.TempDir(), "target") + require.NoError(t, os.Mkdir(target, 0o755)) + require.NoError(t, os.Symlink(target, path)) + }, + uid: os.Geteuid(), + wantMode: 0o700, + }, + { + name: "symlink owned by another user", + setup: func(t *testing.T, path string) { + require.NoError(t, os.Symlink(t.TempDir(), path)) + }, + uid: os.Geteuid() + 1, + wantErr: "the symlink is not owned by the current user", + }, + { + name: "symlink to a file", + setup: func(t *testing.T, path string) { + target := filepath.Join(t.TempDir(), "file") + require.NoError(t, os.WriteFile(target, nil, 0o600)) + require.NoError(t, os.Symlink(target, path)) + }, + uid: os.Geteuid(), + wantErr: "not a directory", + }, + { + name: "file", + setup: func(t *testing.T, path string) { + require.NoError(t, os.WriteFile(path, nil, 0o600)) + }, + uid: os.Geteuid(), + wantErr: "not a directory", + }, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + path := filepath.Join(t.TempDir(), "dir") + c.setup(t, path) + err := checkPrivateDir(path, c.uid) + if c.wantErr != "" { + assert.ErrorContains(t, err, c.wantErr) + return + } + require.NoError(t, err) + info, err := os.Stat(path) + require.NoError(t, err) + assert.Equal(t, c.wantMode, info.Mode().Perm()) + }) + } +} diff --git a/internal/config/dir_windows.go b/internal/config/dir_windows.go index 67fc17d4..0e0d966c 100644 --- a/internal/config/dir_windows.go +++ b/internal/config/dir_windows.go @@ -6,3 +6,8 @@ import "os" func isWritable(_ string, info os.FileInfo) bool { return info.Mode().Perm()&0o200 != 0 } + +// ensurePrivateDir does nothing on Windows, where the temporary directory is per user. +func ensurePrivateDir(_ string) error { + return nil +} From c00c0a551a66ee613a279fd650ff5bf72ca34095 Mon Sep 17 00:00:00 2001 From: Patrick Dawkins Date: Fri, 2 Oct 2026 22:33:53 +0100 Subject: [PATCH 6/7] fix(config): check dir ownership only in shared parents, and validate HOME The ownership check broke runs as root with a preserved HOME (sudo -E) or with an arbitrary UID, where the home directory belongs to another user. It now applies only when the parent is world-writable, e.g. /tmp, where another user could have created the directory. A symlink is followed and its target is checked in the same way. HomeDir now requires the directory to exist and returns its real path, like the legacy CLI's getHomeDirectory, so both layers agree on relative or symlinked values. The Unix file uses the "unix" build constraint, like internal/config/alt. Co-Authored-By: Claude Opus 5.5 --- internal/config/dir.go | 20 +++++-- internal/config/dir_test.go | 51 +++++++++++++++--- internal/config/dir_unix.go | 35 +++++++++---- internal/config/dir_unix_test.go | 90 +++++++++++++++++++------------- 4 files changed, 138 insertions(+), 58 deletions(-) diff --git a/internal/config/dir.go b/internal/config/dir.go index 8b50c314..9fcf280c 100644 --- a/internal/config/dir.go +++ b/internal/config/dir.go @@ -2,6 +2,7 @@ package config import ( "errors" + "fmt" "os" "path/filepath" "runtime" @@ -111,12 +112,25 @@ func canWrite(path string) bool { // HomeDir returns the user's home directory. // // It checks the same environment variables as the legacy CLI, in order: {ENV_PREFIX}HOME, HOME and USERPROFILE. -// On Windows, HOME can differ from USERPROFILE, e.g. in MSYS2 or Cygwin. +// On Windows, HOME can differ from USERPROFILE, e.g. in MSYS2 or Cygwin. As in the legacy CLI, the directory must +// exist, and its real path is returned. func (c *Config) HomeDir() (string, error) { for _, name := range []string{c.Application.EnvPrefix + "HOME", "HOME", "USERPROFILE"} { - if v := os.Getenv(name); v != "" { - return v, nil + v := os.Getenv(name) + if v == "" { + continue } + // G703: the user chooses their home directory. + if info, err := os.Stat(v); err != nil || !info.IsDir() { //nolint:gosec + return "", fmt.Errorf("invalid environment variable %s: %s (not a directory)", name, v) + } + // Resolve the path like PHP's realpath. + if abs, err := filepath.Abs(v); err == nil { + if resolved, err := filepath.EvalSymlinks(abs); err == nil { + return resolved, nil + } + } + return v, nil } return os.UserHomeDir() } diff --git a/internal/config/dir_test.go b/internal/config/dir_test.go index 8e4b8a84..3ec9a3b0 100644 --- a/internal/config/dir_test.go +++ b/internal/config/dir_test.go @@ -13,29 +13,66 @@ import ( ) func TestHomeDir(t *testing.T) { + a, b, c := t.TempDir(), t.TempDir(), t.TempDir() + link := filepath.Join(t.TempDir(), "link") + if runtime.GOOS != "windows" { + require.NoError(t, os.Symlink(a, link)) + } + resolved := func(p string) string { + r, err := filepath.EvalSymlinks(p) + require.NoError(t, err) + return r + } cases := []struct { - name string - env map[string]string - want string + name string + env map[string]string + want string + wantErr string }{ - {"prefixed var first", map[string]string{"EXAMPLE_CLI_HOME": "/a", "HOME": "/b", "USERPROFILE": "/c"}, "/a"}, - {"then HOME", map[string]string{"EXAMPLE_CLI_HOME": "", "HOME": "/b", "USERPROFILE": "/c"}, "/b"}, - {"then USERPROFILE", map[string]string{"EXAMPLE_CLI_HOME": "", "HOME": "", "USERPROFILE": "/c"}, "/c"}, + {name: "prefixed var first", env: map[string]string{"EXAMPLE_CLI_HOME": a, "HOME": b, "USERPROFILE": c}, want: a}, + {name: "then HOME", env: map[string]string{"EXAMPLE_CLI_HOME": "", "HOME": b, "USERPROFILE": c}, want: b}, + {name: "then USERPROFILE", env: map[string]string{"EXAMPLE_CLI_HOME": "", "HOME": "", "USERPROFILE": c}, want: c}, + {name: "symlink is resolved", env: map[string]string{"EXAMPLE_CLI_HOME": link}, want: a}, + { + name: "not a directory", + env: map[string]string{"EXAMPLE_CLI_HOME": filepath.Join(a, "missing")}, + wantErr: "invalid environment variable EXAMPLE_CLI_HOME", + }, } for _, c := range cases { t.Run(c.name, func(t *testing.T) { + if c.env["EXAMPLE_CLI_HOME"] == link && runtime.GOOS == "windows" { + t.Skip("symlinks need privileges on Windows") + } cnf, err := config.FromYAML([]byte(validConfig)) require.NoError(t, err) for k, v := range c.env { t.Setenv(k, v) } home, err := cnf.HomeDir() + if c.wantErr != "" { + assert.ErrorContains(t, err, c.wantErr) + return + } require.NoError(t, err) - assert.Equal(t, c.want, home) + assert.Equal(t, resolved(c.want), home) }) } } +func TestHomeDir_Relative(t *testing.T) { + dir := t.TempDir() + t.Chdir(dir) + cnf, err := config.FromYAML([]byte(validConfig)) + require.NoError(t, err) + t.Setenv("EXAMPLE_CLI_HOME", ".") + home, err := cnf.HomeDir() + require.NoError(t, err) + want, err := filepath.EvalSymlinks(dir) + require.NoError(t, err) + assert.Equal(t, want, home) +} + // TestWritableUserDir_TempFallback checks the cases where the legacy CLI uses a temporary directory instead. func TestWritableUserDir_TempFallback(t *testing.T) { cases := []struct { diff --git a/internal/config/dir_unix.go b/internal/config/dir_unix.go index bdbfe646..d0da71f0 100644 --- a/internal/config/dir_unix.go +++ b/internal/config/dir_unix.go @@ -1,10 +1,11 @@ -//go:build !windows +//go:build unix package config import ( "fmt" "os" + "path/filepath" "syscall" "golang.org/x/sys/unix" @@ -15,35 +16,47 @@ func isWritable(path string, _ os.FileInfo) bool { return unix.Access(path, unix.W_OK) == nil } -// checkPrivateDir checks that a directory, and any symlink to it, is owned by the user (uid), and makes it private. +// checkPrivateDir checks a directory if others could have created it, i.e. if its parent is world-writable, e.g. +// /tmp. The directory, and any symlink to it, must then be owned by the user (uid), and it is made private. // -// This prevents another user from controlling the directory, e.g. if it is in a shared /tmp. +// Otherwise, e.g. in a home directory, it can be owned by another user, as with "sudo -E" or an arbitrary UID. func checkPrivateDir(path string, uid int) error { // G703: the path is the user's own config or temporary directory. info, err := os.Lstat(path) //nolint:gosec if err != nil { return err } + shared, err := hasSharedParent(path) + if err != nil { + return err + } + if shared && !ownedBy(info, uid) { + return fmt.Errorf("not owned by the current user: %s", path) + } if info.Mode()&os.ModeSymlink != 0 { - if !ownedBy(info, uid) { - return fmt.Errorf("the symlink is not owned by the current user: %s", path) - } - if info, err = os.Stat(path); err != nil { //nolint:gosec // G703: as above + target, err := filepath.EvalSymlinks(path) + if err != nil { return err } + return checkPrivateDir(target, uid) } if !info.IsDir() { return fmt.Errorf("not a directory: %s", path) } - if !ownedBy(info, uid) { - return fmt.Errorf("the directory is not owned by the current user: %s", path) - } - if info.Mode().Perm()&0o077 != 0 { + if shared && info.Mode().Perm()&0o077 != 0 { return os.Chmod(path, 0o700) //nolint:gosec // G703: as above } return nil } +func hasSharedParent(path string) (bool, error) { + info, err := os.Stat(filepath.Dir(path)) //nolint:gosec // G703: as above + if err != nil { + return false, err + } + return info.Mode().Perm()&0o002 != 0, nil +} + func ownedBy(info os.FileInfo, uid int) bool { st, ok := info.Sys().(*syscall.Stat_t) return ok && int(st.Uid) == uid diff --git a/internal/config/dir_unix_test.go b/internal/config/dir_unix_test.go index 4367aba6..78f1d6d0 100644 --- a/internal/config/dir_unix_test.go +++ b/internal/config/dir_unix_test.go @@ -1,4 +1,4 @@ -//go:build !windows +//go:build unix package config @@ -12,74 +12,76 @@ import ( ) func TestCheckPrivateDir(t *testing.T) { + other := os.Geteuid() + 1 cases := []struct { name string + shared bool // Whether the parent directory is world-writable, like /tmp. setup func(t *testing.T, path string) uid int wantErr string wantMode os.FileMode }{ { - name: "private directory", - setup: func(t *testing.T, path string) { require.NoError(t, os.Mkdir(path, 0o700)) }, + name: "own directory in a shared parent", + shared: true, + setup: mkdir(0o700), uid: os.Geteuid(), wantMode: 0o700, }, { - name: "shared directory is tightened", - setup: func(t *testing.T, path string) { - require.NoError(t, os.Mkdir(path, 0o700)) - require.NoError(t, os.Chmod(path, 0o777)) - }, + name: "open directory in a shared parent is tightened", + shared: true, + setup: mkdir(0o777), uid: os.Geteuid(), wantMode: 0o700, }, { - name: "owned by another user", - setup: func(t *testing.T, path string) { require.NoError(t, os.Mkdir(path, 0o700)) }, - uid: os.Geteuid() + 1, - wantErr: "the directory is not owned by the current user", + name: "another user's directory in a shared parent", + shared: true, + setup: mkdir(0o700), + uid: other, + wantErr: "not owned by the current user", }, { - name: "own symlink", - setup: func(t *testing.T, path string) { - target := filepath.Join(t.TempDir(), "target") - require.NoError(t, os.Mkdir(target, 0o755)) - require.NoError(t, os.Symlink(target, path)) - }, - uid: os.Geteuid(), - wantMode: 0o700, + name: "another user's symlink in a shared parent", + shared: true, + setup: func(t *testing.T, path string) { require.NoError(t, os.Symlink(t.TempDir(), path)) }, + uid: other, + wantErr: "not owned by the current user", }, { - name: "symlink owned by another user", + name: "symlink to another user's directory in a shared parent", setup: func(t *testing.T, path string) { - require.NoError(t, os.Symlink(t.TempDir(), path)) + target := filepath.Join(sharedDir(t), "target") + require.NoError(t, os.Mkdir(target, 0o700)) + require.NoError(t, os.Symlink(target, path)) }, - uid: os.Geteuid() + 1, - wantErr: "the symlink is not owned by the current user", + uid: other, + wantErr: "not owned by the current user", }, { - name: "symlink to a file", - setup: func(t *testing.T, path string) { - target := filepath.Join(t.TempDir(), "file") - require.NoError(t, os.WriteFile(target, nil, 0o600)) - require.NoError(t, os.Symlink(target, path)) - }, - uid: os.Geteuid(), - wantErr: "not a directory", + // E.g. sudo -E, or an arbitrary UID in a container. + name: "another user's directory in a private parent", + setup: mkdir(0o755), + uid: other, + wantMode: 0o755, }, { - name: "file", - setup: func(t *testing.T, path string) { - require.NoError(t, os.WriteFile(path, nil, 0o600)) - }, + name: "file", + shared: true, + setup: func(t *testing.T, path string) { require.NoError(t, os.WriteFile(path, nil, 0o600)) }, uid: os.Geteuid(), wantErr: "not a directory", }, } for _, c := range cases { t.Run(c.name, func(t *testing.T) { - path := filepath.Join(t.TempDir(), "dir") + parent := t.TempDir() + require.NoError(t, os.Chmod(parent, 0o755)) + if c.shared { + parent = sharedDir(t) + } + path := filepath.Join(parent, "dir") c.setup(t, path) err := checkPrivateDir(path, c.uid) if c.wantErr != "" { @@ -93,3 +95,17 @@ func TestCheckPrivateDir(t *testing.T) { }) } } + +func mkdir(mode os.FileMode) func(t *testing.T, path string) { + return func(t *testing.T, path string) { + require.NoError(t, os.Mkdir(path, 0o700)) + require.NoError(t, os.Chmod(path, mode)) + } +} + +// sharedDir returns a directory that others can write to, like /tmp. +func sharedDir(t *testing.T) string { + dir := t.TempDir() + require.NoError(t, os.Chmod(dir, 0o777|os.ModeSticky)) + return dir +} From 6e6e69e8dc500db4c824434d812f8adb75c88795 Mon Sep 17 00:00:00 2001 From: Patrick Dawkins Date: Fri, 2 Oct 2026 22:36:43 +0100 Subject: [PATCH 7/7] docs(config): explain why group-writable parents are not checked Co-Authored-By: Claude Opus 5.5 --- internal/config/dir_unix.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/internal/config/dir_unix.go b/internal/config/dir_unix.go index d0da71f0..374102bb 100644 --- a/internal/config/dir_unix.go +++ b/internal/config/dir_unix.go @@ -49,6 +49,10 @@ func checkPrivateDir(path string, uid int) error { return nil } +// hasSharedParent reports whether a directory's parent is world-writable. +// +// Group-writable parents are not included: they are common with user private groups (umask 002), and in containers +// with arbitrary UIDs, where the home directory is owned by another user. func hasSharedParent(path string) (bool, error) { info, err := os.Stat(filepath.Dir(path)) //nolint:gosec // G703: as above if err != nil {