impl: set SHELL environment variable (#1512)

We were assuming that most shells set `SHELL` upon startup, but this is
only true for bash. Usually `SHELL` is only set by `login`, so we need
to set it ourselves when launching the user's shell.

Also remove the `keyEqualsValue` which was only being used by the VSCode
integration. This function escapes environment variable values for a
shell, but we were setting the `code` process's environment with them
directly. This was mangling the values by adding quotes and backslashes.

Fixes #1454.

Author: gcurtis

internal/boxcli/integrate.go

@@ -7,6 +7,8 @@ import (
 	"bytes"
 	"encoding/json"
 	"os/exec"
+	"slices"
+	"strings"
 
 	"github.com/spf13/cobra"
 	"github.com/zealic/go2node"
@@ -54,7 +56,6 @@ type parentMessage struct {
 }
 
 func runIntegrateVSCodeCmd(cmd *cobra.Command) error {
-
 	// Setup process communication with node as parent
 	channel, err := go2node.RunAsNodeChild()
 	if err != nil {
@@ -85,6 +86,14 @@ func runIntegrateVSCodeCmd(cmd *cobra.Command) error {
 	if err != nil {
 		return err
 	}
+	envVars = slices.DeleteFunc(envVars, func(s string) bool {
+		k, _, ok := strings.Cut(s, "=")
+		// DEVBOX_OG_PATH_<hash> being set causes devbox global shellenv to overwrite the
+		// PATH after VSCode opens and resets it to global shellenv. This causes the VSCode
+		// terminal to not be able to find devbox packages after the reopen in devbox
+		// environment action is called.
+		return ok && (strings.HasPrefix(k, "DEVBOX_OG_PATH") || k == "HOME" || k == "NODE_CHANNEL_FD")
+	})
 
 	// Send message to parent process to terminate
 	err = channel.Write(&go2node.NodeMessage{

internal/impl/devbox.go

@@ -71,7 +71,6 @@ type Devbox struct {
 var legacyPackagesWarningHasBeenShown = false
 
 func Open(opts *devopt.Opts) (*Devbox, error) {
-
 	projectDir, err := findProjectDir(opts.Dir)
 	if err != nil {
 		return nil, err
@@ -318,7 +317,7 @@ func (d *Devbox) EnvVars(ctx context.Context) ([]string, error) {
 	if err != nil {
 		return nil, err
 	}
-	return keyEqualsValue(envs), nil
+	return mapToPairs(envs), nil
 }
 
 func (d *Devbox) ShellEnvHash(ctx context.Context) (string, error) {
@@ -684,7 +683,6 @@ func (d *Devbox) StartProcessManager(
 	background bool,
 	processComposeFileOrDir string,
 ) error {
-
 	if !d.IsEnvEnabled() {
 		args := []string{"services", "up"}
 		args = append(args, requestedServices...)

internal/impl/envvars.go

@@ -4,24 +4,31 @@
 package impl
 
 import (
-	"fmt"
 	"os"
-	"sort"
+	"slices"
 	"strings"
 )
 
 const devboxSetPrefix = "__DEVBOX_SET_"
 
+// mapToPairs creates a slice of environment variable "key=value" pairs from a
+// map.
 func mapToPairs(m map[string]string) []string {
-	pairs := []string{}
+	pairs := make([]string, len(m))
+	i := 0
 	for k, v := range m {
-		pairs = append(pairs, fmt.Sprintf("%s=%s", k, v))
+		pairs[i] = k + "=" + v
+		i++
 	}
+	slices.Sort(pairs) // for reproducibility
 	return pairs
 }
 
+// pairsToMap creates a map from a slice of "key=value" environment variable
+// pairs. Note that maps are not ordered, which can affect the final variable
+// values when pairs contains duplicate keys.
 func pairsToMap(pairs []string) map[string]string {
-	vars := map[string]string{}
+	vars := make(map[string]string, len(pairs))
 	for _, p := range pairs {
 		k, v, ok := strings.Cut(p, "=")
 		if !ok {
@@ -32,14 +39,19 @@ func pairsToMap(pairs []string) map[string]string {
 	return vars
 }
 
-// exportify takes a map of [string]string and returns a single string
-// of the form export KEY="VAL"; and escapes all the vals from special characters.
+// exportify formats vars as a line-separated string of shell export statements.
+// Each line is of the form `export key="value";` with any special characters in
+// value escaped. This means that the shell will always interpret values as
+// literal strings; no variable expansion or command substitution will take
+// place.
 func exportify(vars map[string]string) string {
-	keys := make([]string, 0, len(vars))
+	keys := make([]string, len(vars))
+	i := 0
 	for k := range vars {
-		keys = append(keys, k)
+		keys[i] = k
+		i++
 	}
-	sort.Strings(keys)
+	slices.Sort(keys) // for reproducibility
 
 	strb := strings.Builder{}
 	for _, k := range keys {
@@ -60,56 +72,6 @@ func exportify(vars map[string]string) string {
 	return strings.TrimSpace(strb.String())
 }
 
-// exportify takes a map of [string]string and returns an array of string
-// of the form KEY="VAL" and escapes all the vals from special characters.
-func keyEqualsValue(vars map[string]string) []string {
-	keys := make([]string, 0, len(vars))
-	for k := range vars {
-		keys = append(keys, k)
-	}
-	keyValues := make([]string, 0, len(vars))
-	sort.Strings(keys)
-
-	for _, k := range keys {
-		if isApproved(k) {
-			strb := strings.Builder{}
-			strb.WriteString(k)
-			strb.WriteString(`=`)
-			for _, r := range vars[k] {
-				switch r {
-				// Special characters inside double quotes:
-				// https://pubs.opengroup.org/onlinepubs/009604499/utilities/xcu_chap02.html#tag_02_02_03
-				case '$', '`', '"', '\\', '\n':
-					strb.WriteRune('\\')
-				}
-				strb.WriteRune(r)
-			}
-			keyValues = append(keyValues, strb.String())
-		}
-	}
-	return keyValues
-}
-
-func isApproved(key string) bool {
-	// list to keys
-	// should find the corrupt key
-	troublingEnvKeys := []string{
-		"HOME",
-		"NODE_CHANNEL_FD",
-	}
-	approved := true
-	for _, ak := range troublingEnvKeys {
-		// DEVBOX_OG_PATH_<hash> being set causes devbox global shellenv or overwrite
-		// the PATH after vscode opens and resets it to global shellenv
-		// This causes vscode terminal to not be able to find devbox packages
-		// after reopen in devbox environment action is called
-		if key == ak || strings.HasPrefix(key, "DEVBOX_OG_PATH") {
-			approved = false
-		}
-	}
-	return approved
-}
-
 // addEnvIfNotPreviouslySetByDevbox adds the key-value pairs from new to existing,
 // but only if the key was not previously set by devbox
 // Caveat, this won't mark the values as set by devbox automatically. Instead,

internal/impl/shell.go

@@ -244,6 +244,7 @@ func (s *DevboxShell) Run() error {
 	for k, v := range extraEnv {
 		env[k] = v
 	}
+	env["SHELL"] = s.binPath
 
 	cmd = exec.Command(s.binPath)
 	cmd.Env = mapToPairs(env)

internal/impl/shell_test.go

@@ -83,7 +83,7 @@ func testWriteDevboxShellrc(t *testing.T, testdirs []string) {
 			// set, and then proceed normally. The test should
 			// always pass in this case.
 			if *update {
-				err = os.WriteFile(test.goldShellrcPath, gotShellrc, 0666)
+				err = os.WriteFile(test.goldShellrcPath, gotShellrc, 0o666)
 				if err != nil {
 					t.Error("Error updating golden files:", err)
 				}