diff --git a/internal/cli/run.go b/internal/cli/run.go index 2031dc11e..9b6f9cfa5 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -950,11 +950,14 @@ func bootstrapEnv(sandboxName, repoDir string, h *harness.Harness) error { if hf.Expand { // Read file, expand ${VAR} in content, write expanded version. + // Uses shell-safe quoting so user-authored values (e.g. + // HUMAN_INSTRUCTION) containing shell metacharacters do not + // cause syntax errors when the file is sourced. (#408, #615) raw, err := os.ReadFile(hostPath) if err != nil { return fmt.Errorf("reading host file %s for expansion: %w", hf.Src, err) } - expanded := os.ExpandEnv(string(raw)) + expanded := shellSafeExpandEnv(string(raw)) tmp, err := os.CreateTemp("", "fullsend-expand-*") if err != nil { @@ -994,6 +997,30 @@ func bootstrapEnv(sandboxName, repoDir string, h *harness.Harness) error { return nil } +// shellSafeExpandEnv expands ${VAR} references in text using the host +// environment, escaping characters that are special inside double quotes +// (", $, `, \) so the result is safe to source as a shell script. +// Templates use the standard export FOO="${FOO}" pattern; this function +// ensures substituted values cannot break out of the double-quote context. +// Fixes #408, #615. +func shellSafeExpandEnv(text string) string { + return os.Expand(text, func(key string) string { + return escapeForDoubleQuotes(os.Getenv(key)) + }) +} + +// escapeForDoubleQuotes escapes the four characters that have special +// meaning inside double-quoted shell strings: backslash, double quote, +// dollar sign, and backtick. Order matters: backslash must be escaped +// first to avoid double-escaping the others. +func escapeForDoubleQuotes(s string) string { + s = strings.ReplaceAll(s, `\`, `\\`) + s = strings.ReplaceAll(s, `"`, `\"`) + s = strings.ReplaceAll(s, `$`, `\$`) + s = strings.ReplaceAll(s, "`", "\\`") + return s +} + // envToList converts a map of env vars to a sorted list of KEY=VALUE strings. func envToList(env map[string]string) []string { keys := make([]string, 0, len(env)) diff --git a/internal/cli/run_test.go b/internal/cli/run_test.go index ca05647f5..bd57d4eda 100644 --- a/internal/cli/run_test.go +++ b/internal/cli/run_test.go @@ -13,6 +13,7 @@ import ( "net/http" "net/http/httptest" "os" + "os/exec" "path/filepath" "runtime" "strings" @@ -430,6 +431,209 @@ func TestEnvToList_Sorted(t *testing.T) { assert.Equal(t, "Z_VAR=z", list[2]) } +func TestShellSafeExpandEnv(t *testing.T) { + tests := []struct { + name string + template string + env map[string]string + want string + }{ + { + name: "simple value", + template: `export FOO="${FOO}"`, + env: map[string]string{"FOO": "bar"}, + want: `export FOO="bar"`, + }, + { + name: "value with double quotes", + template: `export MSG="${MSG}"`, + env: map[string]string{"MSG": `say "hello"`}, + want: `export MSG="say \"hello\""`, + }, + { + name: "value with parentheses", + template: `export MSG="${MSG}"`, + env: map[string]string{"MSG": "fix (example) thing"}, + want: `export MSG="fix (example) thing"`, + }, + { + name: "value with single quotes", + template: `export MSG="${MSG}"`, + env: map[string]string{"MSG": "it's broken"}, + want: `export MSG="it's broken"`, + }, + { + name: "value with dollar sign", + template: `export V="${V}"`, + env: map[string]string{"V": "$HOME"}, + want: `export V="\$HOME"`, + }, + { + name: "value with backticks", + template: `export CMD="${CMD}"`, + env: map[string]string{"CMD": "use `grep` here"}, + want: "export CMD=\"use \\`grep\\` here\"", + }, + { + name: "value with backslashes", + template: `export P="${P}"`, + env: map[string]string{"P": `C:\Users\test`}, + want: `export P="C:\\Users\\test"`, + }, + { + name: "value with all four special chars", + template: `export V="${V}"`, + env: map[string]string{"V": "a]\" $x `y` \\z"}, + want: `export V="a]\" \$x ` + "\\`y\\`" + ` \\z"`, + }, + { + name: "value with shell metacharacters safe inside double quotes", + template: `export CMD="${CMD}"`, + env: map[string]string{"CMD": "foo || true && bar; baz > /dev/null"}, + want: `export CMD="foo || true && bar; baz > /dev/null"`, + }, + { + name: "empty value", + template: `export FOO="${FOO}"`, + env: map[string]string{"FOO": ""}, + want: `export FOO=""`, + }, + { + name: "undefined variable", + template: `export FOO="${UNDEFINED}"`, + env: map[string]string{}, + want: `export FOO=""`, + }, + { + name: "static lines unchanged", + template: "export STATIC='hello world'", + env: map[string]string{}, + want: "export STATIC='hello world'", + }, + { + name: "multiple variables", + template: "export A=\"${A}\"\nexport B=\"${B}\"", + env: map[string]string{"A": "1", "B": "two (2)"}, + want: "export A=\"1\"\nexport B=\"two (2)\"", + }, + { + name: "unquoted template still safe", + template: "export FOO=${FOO}", + env: map[string]string{"FOO": "bar"}, + want: "export FOO=bar", + }, + { + name: "real-world HUMAN_INSTRUCTION from issue 615", + template: `export HUMAN_INSTRUCTION="${HUMAN_INSTRUCTION}"`, + env: map[string]string{"HUMAN_INSTRUCTION": `replacing --search "$ISSUE_NUMBER in:body,title" with timeline API || true`}, + want: `export HUMAN_INSTRUCTION="replacing --search \"\$ISSUE_NUMBER in:body,title\" with timeline API || true"`, + }, + { + name: "real-world instruction with parentheses from failing run", + template: `export HUMAN_INSTRUCTION="${HUMAN_INSTRUCTION}"`, + env: map[string]string{"HUMAN_INSTRUCTION": `An administrator with elevated access to the GCP project (for example, with the ability to set IAM policy) can grant all required roles`}, + want: `export HUMAN_INSTRUCTION="An administrator with elevated access to the GCP project (for example, with the ability to set IAM policy) can grant all required roles"`, + }, + { + name: "injection attempt: break out of double quotes", + template: `export V="${V}"`, + env: map[string]string{"V": `"; rm -rf /; echo "`}, + want: `export V="\"; rm -rf /; echo \""`, + }, + { + name: "injection attempt: command substitution", + template: `export V="${V}"`, + env: map[string]string{"V": `$(cat /etc/passwd)`}, + want: `export V="\$(cat /etc/passwd)"`, + }, + { + name: "injection attempt: backtick substitution", + template: `export V="${V}"`, + env: map[string]string{"V": "`cat /etc/passwd`"}, + want: "export V=\"\\`cat /etc/passwd\\`\"", + }, + { + name: "newlines in value", + template: `export V="${V}"`, + env: map[string]string{"V": "line1\nline2\nline3"}, + want: "export V=\"line1\nline2\nline3\"", + }, + { + name: "tabs and special whitespace", + template: `export V="${V}"`, + env: map[string]string{"V": "col1\tcol2"}, + want: "export V=\"col1\tcol2\"", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + for k, v := range tt.env { + t.Setenv(k, v) + } + got := shellSafeExpandEnv(tt.template) + assert.Equal(t, tt.want, got) + }) + } +} + +// TestShellSafeExpandEnv_ShellRoundtrip verifies that expanded env files +// produce the original value when sourced by a real shell. This is the +// definitive safety test: if the value survives a roundtrip through +// sh -c '. file && printf "%s" "$VAR"', the escaping is correct. +func TestShellSafeExpandEnv_ShellRoundtrip(t *testing.T) { + values := []struct { + name string + value string + }{ + {"simple", "hello world"}, + {"double quotes", `say "hello" to "world"`}, + {"single quotes", "it's a test"}, + {"parentheses", "fix (example) thing"}, + {"pipes and logic", "foo || true && bar"}, + {"dollar sign", "cost is $100 or $HOME"}, + {"command substitution", "$(rm -rf /)"}, + {"backtick substitution", "`rm -rf /`"}, + {"backslashes", `path\to\file`}, + {"semicolons", "cmd1; cmd2; cmd3"}, + {"redirects", "echo foo > /tmp/evil"}, + {"glob chars", "match *.go and file?.txt"}, + {"mixed injection", `"; $(evil) ` + "`more`" + ` && rm -rf / #`}, + {"all four special chars", `quote" dollar$ tick` + "`" + ` slash\`}, + {"newlines", "line1\nline2\nline3"}, + {"tabs", "col1\tcol2"}, + {"empty", ""}, + {"unicode", "こんにちは 🎉"}, + {"real issue 615", `replacing --search "$ISSUE_NUMBER in:body,title" with timeline API || true`}, + {"real failing run", `An administrator with elevated access to the GCP project (for example, with the ability to set IAM policy) can grant all required roles with a single script:`}, + {"already escaped backslash", `already \" escaped`}, + {"nested quotes", `He said "she said 'hello'" today`}, + {"hash comment char", "value # not a comment"}, + {"exclamation mark", "hello! world!"}, + {"curly braces", "use ${VAR} syntax"}, + {"square brackets", "array[0] = value"}, + {"tilde", "~user/path"}, + {"ampersand", "Tom & Jerry"}, + } + + for _, tt := range values { + t.Run(tt.name, func(t *testing.T) { + t.Setenv("TEST_VAL", tt.value) + expanded := shellSafeExpandEnv(`export TEST_VAL="${TEST_VAL}"`) + + // Write expanded content to a temp file and source it in sh. + envFile := filepath.Join(t.TempDir(), "test.env") + require.NoError(t, os.WriteFile(envFile, []byte(expanded+"\n"), 0o644)) + + // Use printf "%s" (not echo) to avoid interpretation of \n etc. + cmd := exec.Command("sh", "-c", fmt.Sprintf(`. %s && printf '%%s' "$TEST_VAL"`, envFile)) + out, err := cmd.Output() + require.NoError(t, err, "shell failed to source expanded env file; expanded content:\n%s", expanded) + assert.Equal(t, tt.value, string(out), "value did not survive shell roundtrip") + }) + } +} + func TestNeedsCrossCompilation(t *testing.T) { result := needsCrossCompilation() if runtime.GOOS == "linux" {