fix: Start login shells on macOS and Linux

kylecarbs · kylecarbs · commit 69ecb11bb5e6 · 2022-06-16T20:44:47.000Z
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`.
diff --git a/agent/agent.go b/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!
diff --git a/agent/agent_test.go b/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,25 @@ func TestAgent(t *testing.T) {
 		require.Equal(t, value, strings.TrimSpace(string(output)))
 	})
 
+	t.Run("EnvironmentVariableExpansion", func(t *testing.T) {
+		t.Parallel()
+		key := "EXAMPLE"
+		value := "$SOMETHINGNOTSET"
+		session := setupSSHSession(t, agent.Metadata{
+			EnvironmentVariables: map[string]string{
+				key: value,
+			},
+		})
+		command := "sh -c 'echo $" + key + "'"
+		if runtime.GOOS == "windows" {
+			command = "cmd.exe /c echo %" + key + "%"
+		}
+		output, err := session.Output(command)
+		require.NoError(t, err)
+		// Output should be empty, because the variable is not set!
+		require.Equal(t, "", strings.TrimSpace(string(output)))
+	})
+
 	t.Run("StartupScript", func(t *testing.T) {
 		t.Parallel()
 		tempPath := filepath.Join(os.TempDir(), "content.txt")
diff --git a/cli/agent.go b/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{
diff --git a/go.mod b/go.mod
@@ -134,7 +134,6 @@ require github.com/xi2/xz v0.0.0-20171230120015-48954b6210f8 // indirect
 
 require (
 	github.com/agnivade/levenshtein v1.0.1 // indirect
-	github.com/stretchr/objx v0.2.0 // indirect
 	github.com/vektah/gqlparser/v2 v2.4.4 // indirect
 	github.com/yuin/goldmark v1.4.12 // indirect
 )
@@ -234,7 +233,6 @@ require (
 	github.com/spf13/jwalterweatherman v1.1.0 // indirect
 	github.com/tadvi/systray v0.0.0-20190226123456-11a2b8fa57af // indirect
 	github.com/tdewolff/parse/v2 v2.6.0 // indirect
-	github.com/vektah/gqlparser/v2 v2.4.4 // indirect
 	github.com/xeipuuv/gojsonpointer v0.0.0-20190905194746-02993c407bfb // indirect
 	github.com/xeipuuv/gojsonreference v0.0.0-20180127040603-bd5ef7bd5415 // indirect
 	github.com/xeipuuv/gojsonschema v1.2.0 // indirect
diff --git a/go.sum b/go.sum
@@ -1510,7 +1510,6 @@ github.com/stoewer/go-strcase v1.2.0/go.mod h1:IBiWB2sKIp3wVVQ3Y035++gc+knqhUQag
 github.com/stretchr/objx v0.0.0-20180129172003-8a3f7159479f/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
 github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
 github.com/stretchr/objx v0.1.1/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
-github.com/stretchr/objx v0.2.0 h1:Hbg2NidpLE8veEBkEZTL3CvlkUIVzuU9jDplZO54c48=
 github.com/stretchr/objx v0.2.0/go.mod h1:qt09Ya8vawLte6SNmTgCsAVtYtaKzEcn8ATUoHMkEqE=
 github.com/stretchr/testify v0.0.0-20151208002404-e3a8ff8ce365/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs=
 github.com/stretchr/testify v0.0.0-20180303142811-b89eecf5ca5d/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs=
