diff --git a/src/backend.go b/src/backend.go index e619167..6248e09 100644 --- a/src/backend.go +++ b/src/backend.go @@ -5,7 +5,10 @@ import "strings" // Backend abstracts an AI command backend (Claude, Codex, etc.). type Backend interface { // BuildCommand constructs the shell command string to execute. - BuildCommand(baseCmd, extraFlags, prompt string) string + // The prompt is intentionally not part of this string - it is passed to the + // process via stdin so that untrusted candidate/prompt content is never + // parsed by the shell (see RunAICommand). + BuildCommand(baseCmd, extraFlags string) string // ProcessLine parses one line of JSON output from the backend. // Returns text to stream to the terminal/log, and whether the session is complete. ProcessLine(line string) (streamText string, sessionDone bool) diff --git a/src/backend_claude.go b/src/backend_claude.go index ade8450..8a1e8fd 100644 --- a/src/backend_claude.go +++ b/src/backend_claude.go @@ -30,16 +30,15 @@ type ClaudeBackend struct { messageHasContent bool } -func (b *ClaudeBackend) BuildCommand(baseCmd, extraFlags, prompt string) string { - const delimiter = "__NIGEL_PROMPT_EOF__" +func (b *ClaudeBackend) BuildCommand(baseCmd, extraFlags string) string { jsonFlags := "--print --output-format stream-json --include-partial-messages --verbose" + // "-p" with no argument makes claude read the prompt from stdin, which + // RunAICommand supplies directly - the prompt never passes through the shell. if extraFlags != "" { - return fmt.Sprintf("%s %s %s -p <<'%s'\n%s\n%s", - baseCmd, jsonFlags, extraFlags, delimiter, prompt, delimiter) + return fmt.Sprintf("%s %s %s -p", baseCmd, jsonFlags, extraFlags) } - return fmt.Sprintf("%s %s -p <<'%s'\n%s\n%s", - baseCmd, jsonFlags, delimiter, prompt, delimiter) + return fmt.Sprintf("%s %s -p", baseCmd, jsonFlags) } func (b *ClaudeBackend) ProcessLine(line string) (string, bool) { diff --git a/src/backend_codex.go b/src/backend_codex.go index 5ee0bd0..9ab6fb2 100644 --- a/src/backend_codex.go +++ b/src/backend_codex.go @@ -21,21 +21,18 @@ type codexItem struct { // CodexBackend implements Backend for the OpenAI Codex CLI. type CodexBackend struct{} -func (b *CodexBackend) BuildCommand(baseCmd, extraFlags, prompt string) string { - const delimiter = "__NIGEL_PROMPT_EOF__" +func (b *CodexBackend) BuildCommand(baseCmd, extraFlags string) string { cmd := strings.TrimSpace(baseCmd) if cmd == "codex" { cmd = "codex exec" } - // codex exec --json reads the prompt from stdin when using "-" - // Heredoc avoids shell quoting issues + // "codex exec --json -" reads the prompt from stdin, which RunAICommand + // supplies directly - the prompt never passes through the shell. if extraFlags != "" { - return fmt.Sprintf("%s --json %s - <<'%s'\n%s\n%s", - cmd, extraFlags, delimiter, prompt, delimiter) + return fmt.Sprintf("%s --json %s -", cmd, extraFlags) } - return fmt.Sprintf("%s --json - <<'%s'\n%s\n%s", - cmd, delimiter, prompt, delimiter) + return fmt.Sprintf("%s --json -", cmd) } func (b *CodexBackend) ProcessLine(line string) (string, bool) { diff --git a/src/backend_codex_test.go b/src/backend_codex_test.go index 6f9c5f0..3d0362b 100644 --- a/src/backend_codex_test.go +++ b/src/backend_codex_test.go @@ -4,15 +4,25 @@ import "strings" import "testing" func TestCodexBuildCommandUsesExecForBareCodex(t *testing.T) { - cmd := (&CodexBackend{}).BuildCommand("codex", "", "hello") - if !strings.HasPrefix(cmd, "codex exec --json - <<") { + cmd := (&CodexBackend{}).BuildCommand("codex", "") + if cmd != "codex exec --json -" { t.Fatalf("BuildCommand() = %q, want codex exec invocation", cmd) } } func TestCodexBuildCommandDoesNotDoubleExec(t *testing.T) { - cmd := (&CodexBackend{}).BuildCommand("codex exec", "--sandbox read-only", "hello") - if !strings.HasPrefix(cmd, "codex exec --json --sandbox read-only - <<") { + cmd := (&CodexBackend{}).BuildCommand("codex exec", "--sandbox read-only") + if cmd != "codex exec --json --sandbox read-only -" { t.Fatalf("BuildCommand() = %q, want existing codex exec invocation preserved", cmd) } } + +func TestCodexBuildCommandExcludesPromptFromShellString(t *testing.T) { + // The prompt must never be embedded in the returned shell string - it is + // sent via stdin instead, so untrusted candidate content can't be parsed + // by the shell (see the heredoc-injection issue this replaces). + cmd := (&CodexBackend{}).BuildCommand("codex", "") + if strings.Contains(cmd, "<<") { + t.Fatalf("BuildCommand() = %q, should not contain a heredoc", cmd) + } +} diff --git a/src/executor.go b/src/executor.go index e431807..090010e 100644 --- a/src/executor.go +++ b/src/executor.go @@ -61,18 +61,23 @@ func KillRunningProcess() { // The streamCb callback is invoked for each chunk of text received. // Returns the accumulated output (for rate limit detection) and any error. func RunAICommand(backend Backend, baseCmd, extraFlags, prompt, workDir string, logWriter io.Writer, timeout time.Duration, streamCb StreamCallback) (string, error) { - // Build the command via the backend - cmdStr := backend.BuildCommand(baseCmd, extraFlags, prompt) + // Build the command via the backend. The prompt is deliberately excluded + // from this string - it previously was interpolated into a shell heredoc, + // which let candidate/prompt content containing a line matching the + // heredoc delimiter break out of the heredoc and inject arbitrary shell + // commands. Passing it via Stdin below avoids the shell parsing it at all. + cmdStr := backend.BuildCommand(baseCmd, extraFlags) // Log the exact command being executed (for debugging hangs) if logWriter != nil { - fmt.Fprintf(logWriter, "Command: %s\n", cmdStr) + fmt.Fprintf(logWriter, "Command: %s\nPrompt (sent via stdin):\n%s\n", cmdStr, prompt) } args := []string{"-c", cmdStr} cmd := exec.Command("bash", args...) cmd.Dir = workDir + cmd.Stdin = strings.NewReader(prompt) cmd.SysProcAttr = &syscall.SysProcAttr{ Setpgid: true, Pdeathsig: syscall.SIGTERM, diff --git a/src/executor_test.go b/src/executor_test.go index 8a1af53..a06da18 100644 --- a/src/executor_test.go +++ b/src/executor_test.go @@ -588,7 +588,7 @@ func TestRunCommandShowOnFail(t *testing.T) { type stderrBackend struct{} -func (b stderrBackend) BuildCommand(baseCmd, extraFlags, prompt string) string { +func (b stderrBackend) BuildCommand(baseCmd, extraFlags string) string { return "echo hidden-reason >&2; exit 7" } @@ -613,3 +613,52 @@ func TestRunAICommandIncludesStderrOnFailure(t *testing.T) { t.Fatalf("error = %q, want stderr included", err.Error()) } } + +// echoBackend runs "cat" so the test can assert exactly what the process +// received, independent of any AI CLI being installed. +type echoBackend struct{} + +func (b echoBackend) BuildCommand(baseCmd, extraFlags string) string { + return "cat" +} + +func (b echoBackend) ProcessLine(line string) (string, bool) { + return "", false +} + +func (b echoBackend) RateLimitPhrases() []string { + return nil +} + +func (b echoBackend) DisplayName() string { + return "Echo" +} + +// TestRunAICommandPromptNeverParsedByShell is a regression test for a heredoc +// injection: the prompt used to be interpolated into a `bash -c` string inside +// a `<<'__NIGEL_PROMPT_EOF__'` heredoc. A prompt containing a line that +// happened to match the delimiter closed the heredoc early, so any text after +// it was parsed and executed as a normal shell command. Since candidates (and +// therefore prompt content) can come from untrusted repository data via +// candidate_source/$INPUT, this was a real command injection. The prompt is +// now sent via Stdin instead, so it can never be interpreted by the shell no +// matter what it contains. +func TestRunAICommandPromptNeverParsedByShell(t *testing.T) { + dir := t.TempDir() + markerPath := dir + "/pwned" + + maliciousPrompt := "innocuous prefix\n__NIGEL_PROMPT_EOF__\ntouch " + markerPath + "\n" + + output, err := RunAICommand(echoBackend{}, "", "", maliciousPrompt, dir, nil, 0, nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if _, statErr := os.Stat(markerPath); statErr == nil { + t.Fatal("prompt content was executed as a shell command instead of being passed through as data") + } + + if !strings.Contains(output, "touch "+markerPath) { + t.Fatalf("expected prompt content to be echoed back verbatim via stdin, got %q", output) + } +}