fix: Start login shells on macOS and Linux (#2437)

This appends `-l` to the shell command on macOS and Linux.
It also adds environment variable expansion to allow for
chaining from `coder_agent.env`.

agent/agent.go

@@ -328,6 +328,11 @@ func (a *agent) createCommand(ctx context.Context, rawCommand string, env []stri
 	command := rawCommand
 	if len(command) == 0 {
 		command = shell
+		if runtime.GOOS != "windows" {
+			// On Linux and macOS, we should start a login
+			// shell to consume juicy environment variables!
+			command += " -l"
+		}
 	}
 
 	// OpenSSH executes all commands with the users current shell.
@@ -348,7 +353,6 @@ func (a *agent) createCommand(ctx context.Context, rawCommand string, env []stri
 		return nil, xerrors.Errorf("getting os executable: %w", err)
 	}
 	cmd.Env = append(cmd.Env, fmt.Sprintf("USER=%s", username))
-	cmd.Env = append(cmd.Env, fmt.Sprintf(`PATH=%s%c%s`, os.Getenv("PATH"), filepath.ListSeparator, filepath.Dir(executablePath)))
 	// Git on Windows resolves with UNIX-style paths.
 	// If using backslashes, it's unable to find the executable.
 	unixExecutablePath := strings.ReplaceAll(executablePath, "\\", "/")
@@ -363,7 +367,10 @@ func (a *agent) createCommand(ctx context.Context, rawCommand string, env []stri
 	// Load environment variables passed via the agent.
 	// These should override all variables we manually specify.
 	for key, value := range metadata.EnvironmentVariables {
-		cmd.Env = append(cmd.Env, fmt.Sprintf("%s=%s", key, value))
+		// Expanding environment variables allows for customization
+		// of the $PATH, among other variables. Customers can prepand
+		// or append to the $PATH, so allowing expand is required!
+		cmd.Env = append(cmd.Env, fmt.Sprintf("%s=%s", key, os.ExpandEnv(value)))
 	}
 
 	// Agent-level environment variables should take over all!

agent/agent_test.go

@@ -68,21 +68,6 @@ func TestAgent(t *testing.T) {
 		require.True(t, strings.HasSuffix(strings.TrimSpace(string(output)), "gitssh --"))
 	})
 
-	t.Run("PATHHasCoder", func(t *testing.T) {
-		t.Parallel()
-		session := setupSSHSession(t, agent.Metadata{})
-		command := "sh -c 'echo $PATH'"
-		if runtime.GOOS == "windows" {
-			command = "cmd.exe /c echo %PATH%"
-		}
-		output, err := session.Output(command)
-		require.NoError(t, err)
-		ex, err := os.Executable()
-		t.Log(ex)
-		require.NoError(t, err)
-		require.True(t, strings.Contains(strings.TrimSpace(string(output)), filepath.Dir(ex)))
-	})
-
 	t.Run("SessionTTY", func(t *testing.T) {
 		t.Parallel()
 		if runtime.GOOS == "windows" {
@@ -182,6 +167,28 @@ func TestAgent(t *testing.T) {
 		require.Equal(t, value, strings.TrimSpace(string(output)))
 	})
 
+	t.Run("EnvironmentVariableExpansion", func(t *testing.T) {
+		t.Parallel()
+		key := "EXAMPLE"
+		session := setupSSHSession(t, agent.Metadata{
+			EnvironmentVariables: map[string]string{
+				key: "$SOMETHINGNOTSET",
+			},
+		})
+		command := "sh -c 'echo $" + key + "'"
+		if runtime.GOOS == "windows" {
+			command = "cmd.exe /c echo %" + key + "%"
+		}
+		output, err := session.Output(command)
+		require.NoError(t, err)
+		expect := ""
+		if runtime.GOOS == "windows" {
+			expect = "%EXAMPLE%"
+		}
+		// Output should be empty, because the variable is not set!
+		require.Equal(t, expect, strings.TrimSpace(string(output)))
+	})
+
 	t.Run("StartupScript", func(t *testing.T) {
 		t.Parallel()
 		tempPath := filepath.Join(os.TempDir(), "content.txt")

cli/agent.go

@@ -2,6 +2,7 @@ package cli
 
 import (
 	"context"
+	"fmt"
 	"net/http"
 	_ "net/http/pprof" //nolint: gosec
 	"net/url"
@@ -138,6 +139,15 @@ func workspaceAgent() *cobra.Command {
 				}
 			}
 
+			executablePath, err := os.Executable()
+			if err != nil {
+				return xerrors.Errorf("getting os executable: %w", err)
+			}
+			err = os.Setenv("PATH", fmt.Sprintf("%s%c%s", os.Getenv("PATH"), filepath.ListSeparator, filepath.Dir(executablePath)))
+			if err != nil {
+				return xerrors.Errorf("add executable to $PATH: %w", err)
+			}
+
 			closer := agent.New(client.ListenWorkspaceAgent, &agent.Options{
 				Logger: logger,
 				EnvironmentVariables: map[string]string{