diff --git a/internal/devbox/devbox.go b/internal/devbox/devbox.go index 7c5af9161cf..6d93e82e5ad 100644 --- a/internal/devbox/devbox.go +++ b/internal/devbox/devbox.go @@ -394,9 +394,9 @@ func (d *Devbox) EnvExports(ctx context.Context, opts devopt.EnvExportsOpts) (st // Use the appropriate export format based on shell type var envStr string if opts.ShellFormat == devopt.ShellFormatNushell { - envStr = exportifyNushell(envs) + envStr = exportifyNushell(d.stderr, envs) } else { - envStr = exportify(envs) + envStr = exportify(d.stderr, envs) } if opts.RunHooks { diff --git a/internal/devbox/envvars.go b/internal/devbox/envvars.go index ec8231e19ab..307546f30e0 100644 --- a/internal/devbox/envvars.go +++ b/internal/devbox/envvars.go @@ -4,22 +4,59 @@ package devbox import ( + "fmt" + "io" "os" + "regexp" "slices" "strings" "go.jetify.com/devbox/internal/devbox/envpath" "go.jetify.com/devbox/internal/envir" + "go.jetify.com/devbox/internal/ux" ) const devboxSetPrefix = "__DEVBOX_SET_" +// envNameRegexp matches a valid POSIX shell environment variable name: it must +// start with a letter or underscore and contain only letters, digits, and +// underscores. Names that don't match (e.g. a "//" comment key in devbox.json's +// env block) can't be exported without producing invalid shell syntax. +var envNameRegexp = regexp.MustCompile(`^[a-zA-Z_][a-zA-Z0-9_]*$`) + +func isValidEnvName(name string) bool { + return envNameRegexp.MatchString(name) +} + +// warnInvalidEnvNames prints a single warning naming any environment variables +// that were skipped because they aren't valid shell identifiers. The most common +// cause is a "//" comment key in a devbox.json env block, which devbox would +// otherwise emit as `export //=...` and break the entire shell with a cryptic +// error. +func warnInvalidEnvNames(w io.Writer, names []string) { + if len(names) == 0 { + return + } + quoted := make([]string, len(names)) + for i, name := range names { + quoted[i] = fmt.Sprintf("%q", name) + } + ux.Fwarningf( + w, + "Skipping %d environment variable(s) with invalid names: %s.\n"+ + "Environment variable names must match ^[a-zA-Z_][a-zA-Z0-9_]*$. "+ + "If these are \"//\" comments in your devbox.json env block, remove or rename them.\n", + len(names), + strings.Join(quoted, ", "), + ) +} + // 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 { +func exportify(w io.Writer, vars map[string]string) string { keys := make([]string, len(vars)) i := 0 for k := range vars { @@ -28,6 +65,7 @@ func exportify(vars map[string]string) string { } slices.Sort(keys) // for reproducibility + var invalidNames []string strb := strings.Builder{} for _, key := range keys { if strings.HasPrefix(key, "BASH_FUNC_") && strings.HasSuffix(key, "%%") { @@ -41,7 +79,13 @@ func exportify(vars map[string]string) string { strb.WriteString(funcName) strb.WriteString("\n") } else { - // Regular variable + // Regular variable. Skip names that aren't valid shell + // identifiers; exporting them would produce invalid syntax that + // breaks the whole shell (e.g. `export //=...`). + if !isValidEnvName(key) { + invalidNames = append(invalidNames, key) + continue + } strb.WriteString("export ") strb.WriteString(key) strb.WriteString(`="`) @@ -57,12 +101,13 @@ func exportify(vars map[string]string) string { strb.WriteString("\";\n") } } + warnInvalidEnvNames(w, invalidNames) return strings.TrimSpace(strb.String()) } // exportifyNushell formats vars as nushell environment variable assignments. // Each line is of the form `$env.KEY = "value"` with special characters escaped. -func exportifyNushell(vars map[string]string) string { +func exportifyNushell(w io.Writer, vars map[string]string) string { // Nushell protected environment variables that cannot be set manually // See: https://www.nushell.sh/book/environment.html#automatic-environment-variables protectedVars := map[string]bool{ @@ -82,6 +127,7 @@ func exportifyNushell(vars map[string]string) string { } slices.Sort(keys) // for reproducibility + var invalidNames []string strb := strings.Builder{} for _, key := range keys { // Skip bash functions for nushell @@ -94,6 +140,12 @@ func exportifyNushell(vars map[string]string) string { continue } + // Skip names that aren't valid environment variable identifiers. + if !isValidEnvName(key) { + invalidNames = append(invalidNames, key) + continue + } + // Nushell environment variable syntax: $env.KEY = "value" strb.WriteString("$env.") strb.WriteString(key) @@ -108,6 +160,7 @@ func exportifyNushell(vars map[string]string) string { } strb.WriteString("\"\n") } + warnInvalidEnvNames(w, invalidNames) return strings.TrimSpace(strb.String()) } diff --git a/internal/devbox/envvars_test.go b/internal/devbox/envvars_test.go new file mode 100644 index 00000000000..d387feb8c19 --- /dev/null +++ b/internal/devbox/envvars_test.go @@ -0,0 +1,62 @@ +// Copyright 2024 Jetify Inc. and contributors. All rights reserved. +// Use of this source code is governed by the license in the LICENSE file. + +package devbox + +import ( + "io" + "strings" + "testing" +) + +func TestIsValidEnvName(t *testing.T) { + valid := []string{"FOO", "_foo", "foo_BAR_123", "a", "_"} + for _, name := range valid { + if !isValidEnvName(name) { + t.Errorf("isValidEnvName(%q) = false, want true", name) + } + } + + invalid := []string{"//", "//ccache", "bad.name", "1leading", "with space", "with-dash", ""} + for _, name := range invalid { + if isValidEnvName(name) { + t.Errorf("isValidEnvName(%q) = true, want false", name) + } + } +} + +// TestExportifySkipsInvalidNames ensures that env vars whose names aren't valid +// shell identifiers (e.g. a "//" comment key in devbox.json) are dropped instead +// of producing invalid shell that breaks the whole shell. +func TestExportifySkipsInvalidNames(t *testing.T) { + got := exportify(io.Discard, map[string]string{ + "GOOD": "value", + "//": "comment-as-json-hack", + "//ccache": "another comment", + "bad.name": "dotted", + "1leading": "starts with digit", + }) + + if !strings.Contains(got, `export GOOD="value";`) { + t.Errorf("expected valid var to be exported, got:\n%s", got) + } + for _, bad := range []string{"//", "//ccache", "bad.name", "1leading"} { + if strings.Contains(got, bad) { + t.Errorf("expected invalid name %q to be skipped, got:\n%s", bad, got) + } + } +} + +func TestExportifyNushellSkipsInvalidNames(t *testing.T) { + got := exportifyNushell(io.Discard, map[string]string{ + "GOOD": "value", + "//": "comment", + }) + + if !strings.Contains(got, `$env.GOOD = "value"`) { + t.Errorf("expected valid var to be exported, got:\n%s", got) + } + if strings.Contains(got, "//") { + t.Errorf("expected invalid name to be skipped, got:\n%s", got) + } +} diff --git a/internal/devbox/shell.go b/internal/devbox/shell.go index e17e5df7dbf..f3e198582db 100644 --- a/internal/devbox/shell.go +++ b/internal/devbox/shell.go @@ -338,7 +338,7 @@ func (s *DevboxShell) writeDevboxShellrc() (path string, err error) { HooksFilePath: shellgen.ScriptPath(s.projectDir, shellgen.HooksFilename), ShellStartTime: telemetry.FormatShellStart(s.shellStartTime), HistoryFile: strings.TrimSpace(s.historyFile), - ExportEnv: exportify(s.env), + ExportEnv: exportify(s.devbox.stderr, s.env), ShellName: string(s.name), ShellAliases: s.aliasLines(), RefreshAliasName: s.devbox.refreshAliasName(),