From 3955df796e14db6fa981100ebc4a2c3b36d6989e Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Mon, 8 Jun 2026 14:24:32 +0300 Subject: [PATCH] fix(agent): unify working directory resolution (#26102) agentssh's CommandEnv, sftpHandler, and agentproc each resolved the session working directory on their own and had drifted: sftpHandler used the configured directory without checking it exists and bypassed the injected EnvInfoer, while the others stat-checked and fell back to home. Home and shell lookups could also skip the EnvInfoer seam through the exported usershell.HomeDir and Get. Resolve through a single usershell.ResolveWorkingDirectory and confine host home and shell lookups to usershell, so SSH sessions, the process API, and tests can no longer diverge and the injected environment is always honored. This also fixes SFTP landing in a configured directory that no longer exists. Refs coder/coder#26099 --- agent/agent.go | 39 ++++------ agent/agent_test.go | 34 ++++++++- agent/agentproc/api.go | 5 +- agent/agentproc/api_test.go | 8 +- agent/agentproc/process.go | 34 +++++---- agent/agentssh/agentssh.go | 70 ++++++----------- agent/usershell/usershell.go | 28 +++++-- agent/usershell/usershell_darwin.go | 7 +- agent/usershell/usershell_other.go | 7 +- agent/usershell/usershell_test.go | 108 +++++++++++++++++++++++++-- agent/usershell/usershell_windows.go | 7 +- 11 files changed, 231 insertions(+), 116 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index 5deb9893f3..0a3977583f 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -14,7 +14,6 @@ import ( "net/http" "net/netip" "os" - "os/user" "path/filepath" "slices" "strconv" @@ -151,6 +150,9 @@ func New(options Options) Agent { if options.Filesystem == nil { options.Filesystem = afero.NewOsFs() } + if options.EnvInfo == nil { + options.EnvInfo = &usershell.SystemEnvInfo{} + } if options.TempDir == "" { options.TempDir = os.TempDir() } @@ -427,7 +429,7 @@ func (a *agent) init() { pathStore := agentgit.NewPathStore() a.filesAPI = agentfiles.NewAPI(a.logger.Named("files"), a.filesystem, pathStore) - a.processAPI = agentproc.NewAPI(a.logger.Named("processes"), a.execer, pathStore, a.envInfo, a.updateCommandEnv, func() string { + a.processAPI = agentproc.NewAPI(a.logger.Named("processes"), a.execer, a.filesystem, pathStore, a.envInfo, a.updateCommandEnv, func() string { if m := a.manifest.Load(); m != nil { return m.Directory } @@ -1303,12 +1305,12 @@ func (a *agent) handleManifest(manifestOK *checkpoint) func(ctx context.Context, // // An example is VS Code Remote, which must know the directory // before initializing a connection. - manifest.Directory, err = expandPathToAbs(manifest.Directory) + manifest.Directory, err = a.expandPathToAbs(manifest.Directory) if err != nil { return xerrors.Errorf("expand directory: %w", err) } // Normalize all devcontainer paths by making them absolute. - manifest.Devcontainers = agentcontainers.ExpandAllDevcontainerPaths(a.logger, expandPathToAbs, manifest.Devcontainers) + manifest.Devcontainers = agentcontainers.ExpandAllDevcontainerPaths(a.logger, a.expandPathToAbs, manifest.Devcontainers) subsys, err := agentsdk.ProtoFromSubsystems(a.subsystems) if err != nil { a.logger.Critical(ctx, "failed to convert subsystems", slog.Error(err)) @@ -1337,7 +1339,7 @@ func (a *agent) handleManifest(manifestOK *checkpoint) func(ctx context.Context, // longer than file writes. Startup scripts still wait because they run // sequentially below. Env var injection is unaffected because it // happens lazily per-command in updateCommandEnv. - homeDir, err := os.UserHomeDir() + homeDir, err := a.envInfo.HomeDir() if err != nil { a.logger.Warn(ctx, "failed to resolve home directory for secret files", slog.Error(err)) } @@ -2347,31 +2349,16 @@ lifecycleWaitLoop: return nil } -// userHomeDir returns the home directory of the current user, giving -// priority to the $HOME environment variable. -func userHomeDir() (string, error) { - // First we check the environment. - homedir, err := os.UserHomeDir() - if err == nil { - return homedir, nil - } - - // As a fallback, we try the user information. - u, err := user.Current() - if err != nil { - return "", xerrors.Errorf("current user: %w", err) - } - return u.HomeDir, nil -} - // expandPathToAbs converts a path to an absolute path. It primarily resolves -// the home directory and any environment variables that may be set. -func expandPathToAbs(path string) (string, error) { +// the home directory and any environment variables that may be set. The home +// directory is resolved through the agent's EnvInfoer so the injected +// environment is honored. +func (a *agent) expandPathToAbs(path string) (string, error) { if path == "" { return "", nil } if path[0] == '~' { - home, err := userHomeDir() + home, err := a.envInfo.HomeDir() if err != nil { return "", err } @@ -2380,7 +2367,7 @@ func expandPathToAbs(path string) (string, error) { path = os.ExpandEnv(path) if !filepath.IsAbs(path) { - home, err := userHomeDir() + home, err := a.envInfo.HomeDir() if err != nil { return "", err } diff --git a/agent/agent_test.go b/agent/agent_test.go index a9b9431156..ac50b34aa7 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -1473,10 +1473,12 @@ func TestAgent_SFTP(t *testing.T) { expectedDir = "/" + strings.ReplaceAll(customDir, "\\", "/") } - //nolint:dogsled - conn, agentClient, _, _, _ := setupAgent(t, agentsdk.Manifest{ + conn, agentClient, _, fs, _ := setupAgent(t, agentsdk.Manifest{ Directory: customDir, }, 0) + // The agent stats the working directory against its filesystem, so + // the directory must exist there for it to be honored. + require.NoError(t, fs.MkdirAll(customDir, 0o700)) sshClient, err := conn.SSHClient(ctx) require.NoError(t, err) defer sshClient.Close() @@ -1491,6 +1493,34 @@ func TestAgent_SFTP(t *testing.T) { _ = client.Close() assertConnectionReport(t, agentClient, proto.Connection_SSH, 0, "") }) + + t.Run("MissingWorkingDirectory", func(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + home, err := os.UserHomeDir() + require.NoError(t, err, "get home dir") + if runtime.GOOS == "windows" { + home = "/" + strings.ReplaceAll(home, "\\", "/") + } + + // A configured directory that does not exist on the agent's + // filesystem must fall back to the home directory. + missingDir := filepath.Join(t.TempDir(), "does-not-exist") + //nolint:dogsled + conn, _, _, _, _ := setupAgent(t, agentsdk.Manifest{ + Directory: missingDir, + }, 0) + sshClient, err := conn.SSHClient(ctx) + require.NoError(t, err) + defer sshClient.Close() + client, err := sftp.NewClient(sshClient) + require.NoError(t, err) + defer client.Close() + wd, err := client.Getwd() + require.NoError(t, err, "get working directory") + require.Equal(t, home, wd, "working directory should fall back to user home") + }) } func TestAgent_SCP(t *testing.T) { diff --git a/agent/agentproc/api.go b/agent/agentproc/api.go index 30c4a8c0da..4713485e1b 100644 --- a/agent/agentproc/api.go +++ b/agent/agentproc/api.go @@ -11,6 +11,7 @@ import ( "github.com/go-chi/chi/v5" "github.com/google/uuid" + "github.com/spf13/afero" "cdr.dev/slog/v3" "github.com/coder/coder/v2/agent/agentchat" @@ -37,10 +38,10 @@ type API struct { } // NewAPI creates a new process API handler. -func NewAPI(logger slog.Logger, execer agentexec.Execer, pathStore *agentgit.PathStore, envInfo usershell.EnvInfoer, updateEnv func(current []string) (updated []string, err error), workingDir func() string) *API { +func NewAPI(logger slog.Logger, execer agentexec.Execer, fs afero.Fs, pathStore *agentgit.PathStore, envInfo usershell.EnvInfoer, updateEnv func(current []string) (updated []string, err error), workingDir func() string) *API { return &API{ logger: logger, - manager: newManager(logger, execer, envInfo, updateEnv, workingDir), + manager: newManager(logger, execer, fs, envInfo, updateEnv, workingDir), pathStore: pathStore, } } diff --git a/agent/agentproc/api_test.go b/agent/agentproc/api_test.go index ff90ff58b0..6a9cf8f130 100644 --- a/agent/agentproc/api_test.go +++ b/agent/agentproc/api_test.go @@ -138,7 +138,7 @@ func newTestAPIWithOptions(t *testing.T, updateEnv func([]string) ([]string, err logger := slogtest.Make(t, &slogtest.Options{ IgnoreErrors: true, }).Leveled(slog.LevelDebug) - api := agentproc.NewAPI(logger, agentexec.DefaultExecer, nil, nil, updateEnv, workingDir) + api := agentproc.NewAPI(logger, agentexec.DefaultExecer, nil, nil, nil, updateEnv, workingDir) t.Cleanup(func() { _ = api.Close() }) @@ -153,7 +153,7 @@ func newTestAPIWithEnvInfo(t *testing.T, workingDir func() string, envInfo users logger := slogtest.Make(t, &slogtest.Options{ IgnoreErrors: true, }).Leveled(slog.LevelDebug) - api := agentproc.NewAPI(logger, agentexec.DefaultExecer, nil, envInfo, nil, workingDir) + api := agentproc.NewAPI(logger, agentexec.DefaultExecer, nil, nil, envInfo, nil, workingDir) t.Cleanup(func() { _ = api.Close() }) @@ -174,7 +174,7 @@ func TestAccessLogIncludesChatID(t *testing.T) { sink := testutil.NewFakeSink(t) logger := sink.Logger() - api := agentproc.NewAPI(logger, agentexec.DefaultExecer, nil, nil, nil, nil) + api := agentproc.NewAPI(logger, agentexec.DefaultExecer, nil, nil, nil, nil, nil) t.Cleanup(func() { _ = api.Close() }) @@ -1144,7 +1144,7 @@ func TestHandleStartProcess_ChatHeaders_EmptyWorkDir_StillNotifies(t *testing.T) defer unsub() logger := slogtest.Make(t, nil).Leveled(slog.LevelDebug) - api := agentproc.NewAPI(logger, agentexec.DefaultExecer, pathStore, nil, func(current []string) ([]string, error) { + api := agentproc.NewAPI(logger, agentexec.DefaultExecer, nil, pathStore, nil, func(current []string) ([]string, error) { return current, nil }, nil) defer api.Close() diff --git a/agent/agentproc/process.go b/agent/agentproc/process.go index 8f0ca53322..c5c93a2a1a 100644 --- a/agent/agentproc/process.go +++ b/agent/agentproc/process.go @@ -10,6 +10,7 @@ import ( "time" "github.com/google/uuid" + "github.com/spf13/afero" "golang.org/x/xerrors" "cdr.dev/slog/v3" @@ -75,6 +76,7 @@ type manager struct { mu sync.Mutex logger slog.Logger execer agentexec.Execer + fs afero.Fs clock quartz.Clock procs map[string]*process closed bool @@ -84,13 +86,17 @@ type manager struct { } // newManager creates a new process manager. -func newManager(logger slog.Logger, execer agentexec.Execer, envInfo usershell.EnvInfoer, updateEnv func(current []string) (updated []string, err error), workingDir func() string) *manager { +func newManager(logger slog.Logger, execer agentexec.Execer, fs afero.Fs, envInfo usershell.EnvInfoer, updateEnv func(current []string) (updated []string, err error), workingDir func() string) *manager { + if fs == nil { + fs = afero.NewOsFs() + } if envInfo == nil { envInfo = &usershell.SystemEnvInfo{} } return &manager{ logger: logger, execer: execer, + fs: fs, clock: quartz.NewReal(), procs: make(map[string]*process), updateEnv: updateEnv, @@ -122,7 +128,7 @@ func (m *manager) start(req workspacesdk.StartProcessRequest, chatID string) (*p // the process is not tied to any HTTP request. ctx, cancel := context.WithCancel(context.Background()) cmd := m.execer.CommandContext(ctx, "sh", "-c", req.Command) - cmd.Dir = m.resolveWorkDir(req.WorkDir) + cmd.Dir = m.resolveWorkingDirectory(req.WorkDir) cmd.Stdin = nil cmd.SysProcAttr = procSysProcAttr() @@ -370,23 +376,21 @@ func (p *process) waitForOutput(ctx context.Context) error { return ctx.Err() } -// resolveWorkDir returns the directory a process should start in. -// Priority: explicit request dir > agent configured dir > $HOME. -// Falls through when a candidate is empty or does not exist on -// disk, matching the behavior of SSH sessions. -func (m *manager) resolveWorkDir(requested string) string { +// resolveWorkingDirectory returns the directory a process should start in. +// Priority: explicit request dir > agent configured dir > user home. +// The configured dir > home tail is shared with SSH sessions via +// usershell.ResolveWorkingDirectory so the two cannot drift. +func (m *manager) resolveWorkingDirectory(requested string) string { if requested != "" { return requested } + var configured string if m.workingDir != nil { - if dir := m.workingDir(); dir != "" { - if info, err := os.Stat(dir); err == nil && info.IsDir() { - return dir - } - } + configured = m.workingDir() } - if home, err := m.envInfo.HomeDir(); err == nil { - return home + dir, err := usershell.ResolveWorkingDirectory(m.fs, m.envInfo, configured) + if err != nil { + return "" } - return "" + return dir } diff --git a/agent/agentssh/agentssh.go b/agent/agentssh/agentssh.go index 1f7f714b56..eb2e9ebb6b 100644 --- a/agent/agentssh/agentssh.go +++ b/agent/agentssh/agentssh.go @@ -9,7 +9,6 @@ import ( "net" "os" "os/exec" - "os/user" "path/filepath" "runtime" "slices" @@ -185,13 +184,9 @@ func NewServer(ctx context.Context, logger slog.Logger, prometheusRegistry *prom config.AnnouncementBanners = func() *[]codersdk.BannerConfig { return &[]codersdk.BannerConfig{} } } if config.WorkingDirectory == nil { - config.WorkingDirectory = func() string { - home, err := userHomeDir() - if err != nil { - return "" - } - return home - } + // Empty means unset, so resolveWorkingDirectory falls back to the + // EnvInfo home directory. + config.WorkingDirectory = func() string { return "" } } if config.EnvInfo == nil { config.EnvInfo = &usershell.SystemEnvInfo{} @@ -749,7 +744,7 @@ func (s *Server) startPTYSession(logger slog.Logger, session ptySession, magicTy } } - if !isQuietLogin(s.fs, session.RawCommand()) { + if !isQuietLogin(s.fs, s.config.EnvInfo, session.RawCommand()) { err := showMOTD(s.fs, session, s.config.MOTDFile()) if err != nil { logger.Error(ctx, "agent failed to show MOTD", slog.Error(err)) @@ -878,13 +873,14 @@ func (s *Server) sftpHandler(logger slog.Logger, session ssh.Session) error { // Change current working directory to the configured // directory (or home directory if not set) so that SFTP // connections land there. - dir := s.config.WorkingDirectory() - if dir == "" { - var err error - dir, err = userHomeDir() - if err != nil { - logger.Warn(ctx, "get sftp working directory failed, unable to get home dir", slog.Error(err)) - } + // + // The host EnvInfo is used here, not a container's. This is + // correct only while SFTP is blocked for container sessions + // (see the closeCause guard above). If container SFTP is added, + // the container EnvInfo must be resolved and passed here. + dir, err := s.resolveWorkingDirectory(s.config.EnvInfo) + if err != nil { + logger.Warn(ctx, "resolve sftp working directory failed", slog.Error(err)) } if dir != "" { opts = append(opts, sftp.WithServerWorkingDirectory(dir)) @@ -916,6 +912,12 @@ func (s *Server) sftpHandler(logger slog.Logger, session ssh.Session) error { return xerrors.Errorf("sftp server closed with error: %w", err) } +// resolveWorkingDirectory returns the working directory for a session, binding +// the server filesystem and configured directory to the shared resolver. +func (s *Server) resolveWorkingDirectory(ei usershell.EnvInfoer) (string, error) { + return usershell.ResolveWorkingDirectory(s.fs, ei, s.config.WorkingDirectory()) +} + func (s *Server) CommandEnv(ei usershell.EnvInfoer, addEnv []string) (shell, dir string, env []string, err error) { if ei == nil { ei = &usershell.SystemEnvInfo{} @@ -932,18 +934,9 @@ func (s *Server) CommandEnv(ei usershell.EnvInfoer, addEnv []string) (shell, dir return "", "", nil, xerrors.Errorf("get user shell: %w", err) } - dir = s.config.WorkingDirectory() - - // If the metadata directory doesn't exist, we run the command - // in the users home directory. - _, err = os.Stat(dir) - if dir == "" || err != nil { - // Default to user home if a directory is not set. - homedir, err := ei.HomeDir() - if err != nil { - return "", "", nil, xerrors.Errorf("get home dir: %w", err) - } - dir = homedir + dir, err = s.resolveWorkingDirectory(ei) + if err != nil { + return "", "", nil, xerrors.Errorf("resolve working dir: %w", err) } env = append(ei.Environ(), addEnv...) // Set login variables (see `man login`). @@ -1288,7 +1281,7 @@ func isLoginShell(rawCommand string) bool { // isQuietLogin checks if the SSH server should perform a quiet login or not. // // https://github.com/openssh/openssh-portable/blob/25bd659cc72268f2858c5415740c442ee950049f/session.c#L816 -func isQuietLogin(fs afero.Fs, rawCommand string) bool { +func isQuietLogin(fs afero.Fs, ei usershell.EnvInfoer, rawCommand string) bool { // We are always quiet unless this is a login shell. if !isLoginShell(rawCommand) { return true @@ -1296,7 +1289,7 @@ func isQuietLogin(fs afero.Fs, rawCommand string) bool { // Best effort, if we can't get the home directory, // we can't lookup .hushlogin. - homedir, err := userHomeDir() + homedir, err := ei.HomeDir() if err != nil { return false } @@ -1355,23 +1348,6 @@ func writeWithCarriageReturn(src io.Reader, dest io.Writer) error { return nil } -// userHomeDir returns the home directory of the current user, giving -// priority to the $HOME environment variable. -func userHomeDir() (string, error) { - // First we check the environment. - homedir, err := os.UserHomeDir() - if err == nil { - return homedir, nil - } - - // As a fallback, we try the user information. - u, err := user.Current() - if err != nil { - return "", xerrors.Errorf("current user: %w", err) - } - return u.HomeDir, nil -} - // UpdateHostSigner updates the host signer with a new key generated from the provided seed. // If an existing host key exists with the same algorithm, it is overwritten func (s *Server) UpdateHostSigner(seed int64) error { diff --git a/agent/usershell/usershell.go b/agent/usershell/usershell.go index 1819eb468a..7a386a6079 100644 --- a/agent/usershell/usershell.go +++ b/agent/usershell/usershell.go @@ -4,13 +4,15 @@ import ( "os" "os/user" + "github.com/spf13/afero" "golang.org/x/xerrors" ) -// HomeDir returns the home directory of the current user, giving -// priority to the $HOME environment variable. -// Deprecated: use EnvInfoer.HomeDir() instead. -func HomeDir() (string, error) { +// homeDir returns the home directory of the current user, giving +// priority to the $HOME environment variable. It backs +// SystemEnvInfo.HomeDir. Callers outside this package resolve the home +// directory through an EnvInfoer so the injected environment is honored. +func homeDir() (string, error) { // First we check the environment. homedir, err := os.UserHomeDir() if err == nil { @@ -25,6 +27,20 @@ func HomeDir() (string, error) { return u.HomeDir, nil } +// ResolveWorkingDirectory returns dir when it is non-empty and an existing +// directory on fs. Otherwise it falls back to the home directory +// reported by ei. SSH sessions and the process API share this so their +// working directory resolution cannot drift, and the home fallback goes +// through the injected EnvInfoer rather than the host directly. +func ResolveWorkingDirectory(fs afero.Fs, ei EnvInfoer, dir string) (string, error) { + if dir != "" { + if info, err := fs.Stat(dir); err == nil && info.IsDir() { + return dir, nil + } + } + return ei.HomeDir() +} + // EnvInfoer encapsulates external information about the environment. type EnvInfoer interface { // User returns the current user. @@ -64,11 +80,11 @@ func (SystemEnvInfo) Environ() []string { } func (SystemEnvInfo) HomeDir() (string, error) { - return HomeDir() + return homeDir() } func (SystemEnvInfo) Shell(username string) (string, error) { - return Get(username) + return get(username) } func (SystemEnvInfo) ModifyCommand(name string, args ...string) (string, []string) { diff --git a/agent/usershell/usershell_darwin.go b/agent/usershell/usershell_darwin.go index acc990db83..42500d7a72 100644 --- a/agent/usershell/usershell_darwin.go +++ b/agent/usershell/usershell_darwin.go @@ -9,9 +9,10 @@ import ( "golang.org/x/xerrors" ) -// Get returns the $SHELL environment variable. -// Deprecated: use SystemEnvInfo.UserShell instead. -func Get(username string) (string, error) { +// get resolves the user's shell via dscl, falling back to $SHELL. It +// backs SystemEnvInfo.Shell. Callers resolve the shell through an +// EnvInfoer. +func get(username string) (string, error) { // This command will output "UserShell: /bin/zsh" if successful, we // can ignore the error since we have fallback behavior. if !filepath.IsLocal(username) { diff --git a/agent/usershell/usershell_other.go b/agent/usershell/usershell_other.go index 6ee3ad2368..9093949655 100644 --- a/agent/usershell/usershell_other.go +++ b/agent/usershell/usershell_other.go @@ -10,9 +10,10 @@ import ( "golang.org/x/xerrors" ) -// Get returns the /etc/passwd entry for the username provided. -// Deprecated: use SystemEnvInfo.UserShell instead. -func Get(username string) (string, error) { +// get resolves the user's shell from /etc/passwd, falling back to +// $SHELL. It backs SystemEnvInfo.Shell. Callers resolve the shell +// through an EnvInfoer. +func get(username string) (string, error) { contents, err := os.ReadFile("/etc/passwd") if err != nil { return "", xerrors.Errorf("read /etc/passwd: %w", err) diff --git a/agent/usershell/usershell_test.go b/agent/usershell/usershell_test.go index 40873b5dee..5687b34e99 100644 --- a/agent/usershell/usershell_test.go +++ b/agent/usershell/usershell_test.go @@ -1,26 +1,32 @@ package usershell_test import ( + "os" "os/user" + "path/filepath" "runtime" "testing" + "github.com/spf13/afero" "github.com/stretchr/testify/require" + "golang.org/x/xerrors" "github.com/coder/coder/v2/agent/usershell" ) //nolint:paralleltest,tparallel // This test sets an environment variable. -func TestGet(t *testing.T) { +func TestShell(t *testing.T) { if runtime.GOOS == "windows" { t.SkipNow() } + ei := usershell.SystemEnvInfo{} + t.Run("Fallback", func(t *testing.T) { t.Setenv("SHELL", "/bin/sh") t.Run("NonExistentUser", func(t *testing.T) { - shell, err := usershell.Get("notauser") + shell, err := ei.Shell("notauser") require.NoError(t, err) require.Equal(t, "/bin/sh", shell) }) @@ -31,14 +37,14 @@ func TestGet(t *testing.T) { t.Setenv("SHELL", "") t.Run("NotFound", func(t *testing.T) { - _, err := usershell.Get("notauser") + _, err := ei.Shell("notauser") require.Error(t, err) }) t.Run("User", func(t *testing.T) { u, err := user.Current() require.NoError(t, err) - shell, err := usershell.Get(u.Username) + shell, err := ei.Shell(u.Username) require.NoError(t, err) require.NotEmpty(t, shell) }) @@ -46,10 +52,102 @@ func TestGet(t *testing.T) { t.Run("Remove GOTRACEBACK=none", func(t *testing.T) { t.Setenv("GOTRACEBACK", "none") - ei := usershell.SystemEnvInfo{} env := ei.Environ() for _, e := range env { require.NotEqual(t, "GOTRACEBACK=none", e) } }) } + +// homeEnvInfo reports a fixed home directory and otherwise delegates to +// SystemEnvInfo, isolating ResolveWorkingDirectory tests from the host's real +// home directory. +type homeEnvInfo struct { + usershell.SystemEnvInfo + home string +} + +func (e homeEnvInfo) HomeDir() (string, error) { return e.home, nil } + +// errorEnvInfo reports an error from HomeDir to exercise the fallback +// error path. +type errorEnvInfo struct { + usershell.SystemEnvInfo + err error +} + +func (e errorEnvInfo) HomeDir() (string, error) { return "", e.err } + +func TestResolveWorkingDirectory(t *testing.T) { + t.Parallel() + + const home = "/home/coder" + ei := homeEnvInfo{home: home} + + t.Run("Exists", func(t *testing.T) { + t.Parallel() + fs := afero.NewMemMapFs() + require.NoError(t, fs.MkdirAll("/work", 0o700)) + dir, err := usershell.ResolveWorkingDirectory(fs, ei, "/work") + require.NoError(t, err) + require.Equal(t, "/work", dir) + }) + + t.Run("Missing", func(t *testing.T) { + t.Parallel() + dir, err := usershell.ResolveWorkingDirectory(afero.NewMemMapFs(), ei, "/work") + require.NoError(t, err) + require.Equal(t, home, dir) + }) + + t.Run("Empty", func(t *testing.T) { + t.Parallel() + dir, err := usershell.ResolveWorkingDirectory(afero.NewMemMapFs(), ei, "") + require.NoError(t, err) + require.Equal(t, home, dir) + }) + + t.Run("NotADirectory", func(t *testing.T) { + t.Parallel() + fs := afero.NewMemMapFs() + require.NoError(t, afero.WriteFile(fs, "/work", []byte("file"), 0o600)) + dir, err := usershell.ResolveWorkingDirectory(fs, ei, "/work") + require.NoError(t, err) + require.Equal(t, home, dir) + }) + + t.Run("HomeDirError", func(t *testing.T) { + t.Parallel() + ei := errorEnvInfo{err: xerrors.New("no home")} + _, err := usershell.ResolveWorkingDirectory(afero.NewMemMapFs(), ei, "") + require.ErrorContains(t, err, "no home") + }) + + t.Run("Symlink", func(t *testing.T) { + t.Parallel() + if runtime.GOOS == "windows" { + t.Skip("symlink creation requires privileges on Windows") + } + // MemMapFs cannot model symlinks. Use the real filesystem to + // confirm Stat follows symlinks: a link to a directory is honored, + // a link to a non-directory falls back to home. + fs := afero.NewOsFs() + base := t.TempDir() + + realDir := filepath.Join(base, "real") + require.NoError(t, os.Mkdir(realDir, 0o700)) + linkToDir := filepath.Join(base, "link-dir") + require.NoError(t, os.Symlink(realDir, linkToDir)) + dir, err := usershell.ResolveWorkingDirectory(fs, ei, linkToDir) + require.NoError(t, err) + require.Equal(t, linkToDir, dir, "symlink to a directory should be honored") + + realFile := filepath.Join(base, "file") + require.NoError(t, os.WriteFile(realFile, []byte("x"), 0o600)) + linkToFile := filepath.Join(base, "link-file") + require.NoError(t, os.Symlink(realFile, linkToFile)) + dir, err = usershell.ResolveWorkingDirectory(fs, ei, linkToFile) + require.NoError(t, err) + require.Equal(t, home, dir, "symlink to a non-directory should fall back to home") + }) +} diff --git a/agent/usershell/usershell_windows.go b/agent/usershell/usershell_windows.go index 52823d900d..7ddf27ed2a 100644 --- a/agent/usershell/usershell_windows.go +++ b/agent/usershell/usershell_windows.go @@ -2,9 +2,10 @@ package usershell import "os/exec" -// Get returns the command prompt binary name. -// Deprecated: use SystemEnvInfo.UserShell instead. -func Get(username string) (string, error) { +// get resolves the Windows shell, preferring pwsh.exe, then +// powershell.exe, then cmd.exe. It backs SystemEnvInfo.Shell. Callers +// resolve the shell through an EnvInfoer. +func get(username string) (string, error) { _, err := exec.LookPath("pwsh.exe") if err == nil { return "pwsh.exe", nil