diff --git a/cascade/doc.go b/cascade/doc.go index 8f622f2..be50987 100644 --- a/cascade/doc.go +++ b/cascade/doc.go @@ -27,35 +27,35 @@ // // PreRunCommand hooks control shell command execution: // -// hookshot.Run(func(input cascade.PreRunCommandInput) cascade.PreRunCommandOutput { +// hookshot.RunE(func(input cascade.PreRunCommandInput) (cascade.PreRunCommandOutput, error) { // // Block dangerous commands // if strings.Contains(input.ToolInfo.CommandLine, "rm -rf") { -// return cascade.Deny("Dangerous command blocked") +// return cascade.BlockCommand("Dangerous command blocked") // } -// return cascade.Allow() +// return cascade.AllowCommand(), nil // }) // // # PreWriteCode Hooks // // PreWriteCode hooks control file modifications: // -// hookshot.Run(func(input cascade.PreWriteCodeInput) cascade.PreWriteCodeOutput { +// hookshot.RunE(func(input cascade.PreWriteCodeInput) (cascade.PreWriteCodeOutput, error) { // // Block writes to sensitive files // if strings.Contains(input.ToolInfo.FilePath, ".env") { -// return cascade.Deny("Cannot modify environment files") +// return cascade.BlockWrite("Cannot modify environment files") // } -// return cascade.Allow() +// return cascade.AllowWrite(), nil // }) // // # PreUserPrompt Hooks // // PreUserPrompt hooks validate or modify user prompts: // -// hookshot.Run(func(input cascade.PreUserPromptInput) cascade.PreUserPromptOutput { +// hookshot.RunE(func(input cascade.PreUserPromptInput) (cascade.PreUserPromptOutput, error) { // if containsSecrets(input.ToolInfo.UserPrompt) { -// return cascade.BlockPrompt("Please don't include secrets in prompts") +// return cascade.BlockPromptWithError("Please don't include secrets in prompts") // } -// return cascade.AllowPrompt() +// return cascade.AllowPrompt(), nil // }) // // # Helper Functions @@ -64,7 +64,8 @@ // // Pre-hooks (PreRunCommand, PreWriteCode, PreReadCode, PreMCPToolUse): // - [Allow]: Permit the action -// - [Deny]: Block the action with a reason +// - [Deny]: Build deny JSON for the action +// - [Block]: Build deny JSON plus the error required for [hookshot.RunE] // - [Ask]: Prompt user to confirm (where supported) // // PreUserPrompt: diff --git a/cascade/helpers.go b/cascade/helpers.go index 3e29c1a..94a4bfc 100644 --- a/cascade/helpers.go +++ b/cascade/helpers.go @@ -1,5 +1,7 @@ package cascade +import "errors" + // ============================================================================= // Pre-Hook Decision Helpers (shared by PreRunCommand, PreWriteCode, etc.) // ============================================================================= @@ -11,7 +13,8 @@ func Allow() BaseOutput { } } -// Deny blocks the action with a message. +// Deny returns deny JSON with a message. Cascade pre-hooks must return this +// from RunE with a non-nil error to block the action. func Deny(message string) BaseOutput { return BaseOutput{ Decision: "deny", @@ -19,6 +22,12 @@ func Deny(message string) BaseOutput { } } +// Block returns a deny output plus the error RunE needs to make Cascade +// block the pre-hook with exit code 2. +func Block(message string) (BaseOutput, error) { + return Deny(message), errors.New(message) +} + // Ask prompts the user to confirm the action. func Ask(message string) BaseOutput { return BaseOutput{ @@ -38,13 +47,18 @@ func AllowCommand() PreRunCommandOutput { } } -// DenyCommand blocks the command from executing. +// DenyCommand returns deny JSON for a command. func DenyCommand(message string) PreRunCommandOutput { return PreRunCommandOutput{ BaseOutput: Deny(message), } } +// BlockCommand blocks the command from executing when returned from RunE. +func BlockCommand(message string) (PreRunCommandOutput, error) { + return DenyCommand(message), errors.New(message) +} + // AskCommand prompts the user to confirm the command. func AskCommand(message string) PreRunCommandOutput { return PreRunCommandOutput{ @@ -63,13 +77,18 @@ func AllowWrite() PreWriteCodeOutput { } } -// DenyWrite blocks the file write. +// DenyWrite returns deny JSON for a file write. func DenyWrite(message string) PreWriteCodeOutput { return PreWriteCodeOutput{ BaseOutput: Deny(message), } } +// BlockWrite blocks the file write when returned from RunE. +func BlockWrite(message string) (PreWriteCodeOutput, error) { + return DenyWrite(message), errors.New(message) +} + // AskWrite prompts the user to confirm the write. func AskWrite(message string) PreWriteCodeOutput { return PreWriteCodeOutput{ @@ -88,13 +107,18 @@ func AllowRead() PreReadCodeOutput { } } -// DenyRead blocks the file read. +// DenyRead returns deny JSON for a file read. func DenyRead(message string) PreReadCodeOutput { return PreReadCodeOutput{ BaseOutput: Deny(message), } } +// BlockRead blocks the file read when returned from RunE. +func BlockRead(message string) (PreReadCodeOutput, error) { + return DenyRead(message), errors.New(message) +} + // AskRead prompts the user to confirm the read. func AskRead(message string) PreReadCodeOutput { return PreReadCodeOutput{ @@ -113,13 +137,18 @@ func AllowMCP() PreMCPToolUseOutput { } } -// DenyMCP blocks the MCP tool from executing. +// DenyMCP returns deny JSON for an MCP tool. func DenyMCP(message string) PreMCPToolUseOutput { return PreMCPToolUseOutput{ BaseOutput: Deny(message), } } +// BlockMCP blocks the MCP tool when returned from RunE. +func BlockMCP(message string) (PreMCPToolUseOutput, error) { + return DenyMCP(message), errors.New(message) +} + // AskMCP prompts the user to confirm the MCP tool. func AskMCP(message string) PreMCPToolUseOutput { return PreMCPToolUseOutput{ @@ -145,6 +174,11 @@ func BlockPrompt(message string) PreUserPromptOutput { } } +// BlockPromptWithError blocks the user prompt when returned from RunE. +func BlockPromptWithError(message string) (PreUserPromptOutput, error) { + return BlockPrompt(message), errors.New(message) +} + // ============================================================================= // Post-Hook Helpers (all are fire-and-forget) // ============================================================================= diff --git a/cascade/helpers_test.go b/cascade/helpers_test.go index 8566e12..aa0bdf2 100644 --- a/cascade/helpers_test.go +++ b/cascade/helpers_test.go @@ -28,6 +28,19 @@ func TestDeny(t *testing.T) { } } +func TestBlock(t *testing.T) { + output, err := Block("Action blocked") + if output.Decision != "deny" { + t.Errorf("Decision = %q, want %q", output.Decision, "deny") + } + if output.Message != "Action blocked" { + t.Errorf("Message = %q, want %q", output.Message, "Action blocked") + } + if err == nil || err.Error() != "Action blocked" { + t.Fatalf("error = %v, want Action blocked", err) + } +} + func TestAsk(t *testing.T) { output := Ask("Confirm action?") if output.Decision != "ask" { @@ -62,6 +75,16 @@ func TestDenyCommand(t *testing.T) { } } +func TestBlockCommand(t *testing.T) { + output, err := BlockCommand("Command not allowed") + if output.Decision != "deny" || output.Message != "Command not allowed" { + t.Fatalf("output = %+v, want deny with message", output) + } + if err == nil || err.Error() != "Command not allowed" { + t.Fatalf("error = %v, want Command not allowed", err) + } +} + func TestAskCommand(t *testing.T) { output := AskCommand("Run this command?") if output.Decision != "ask" { @@ -96,6 +119,16 @@ func TestDenyWrite(t *testing.T) { } } +func TestBlockWrite(t *testing.T) { + output, err := BlockWrite("Write not permitted") + if output.Decision != "deny" || output.Message != "Write not permitted" { + t.Fatalf("output = %+v, want deny with message", output) + } + if err == nil || err.Error() != "Write not permitted" { + t.Fatalf("error = %v, want Write not permitted", err) + } +} + func TestAskWrite(t *testing.T) { output := AskWrite("Allow file write?") if output.Decision != "ask" { @@ -130,6 +163,16 @@ func TestDenyRead(t *testing.T) { } } +func TestBlockRead(t *testing.T) { + output, err := BlockRead("Cannot read file") + if output.Decision != "deny" || output.Message != "Cannot read file" { + t.Fatalf("output = %+v, want deny with message", output) + } + if err == nil || err.Error() != "Cannot read file" { + t.Fatalf("error = %v, want Cannot read file", err) + } +} + func TestAskRead(t *testing.T) { output := AskRead("Allow file read?") if output.Decision != "ask" { @@ -164,6 +207,16 @@ func TestDenyMCP(t *testing.T) { } } +func TestBlockMCP(t *testing.T) { + output, err := BlockMCP("MCP tool blocked") + if output.Decision != "deny" || output.Message != "MCP tool blocked" { + t.Fatalf("output = %+v, want deny with message", output) + } + if err == nil || err.Error() != "MCP tool blocked" { + t.Fatalf("error = %v, want MCP tool blocked", err) + } +} + func TestAskMCP(t *testing.T) { output := AskMCP("Run MCP tool?") if output.Decision != "ask" { @@ -198,6 +251,16 @@ func TestBlockPrompt(t *testing.T) { } } +func TestBlockPromptWithError(t *testing.T) { + output, err := BlockPromptWithError("Prompt blocked") + if output.Decision != "deny" || output.Message != "Prompt blocked" { + t.Fatalf("output = %+v, want deny with message", output) + } + if err == nil || err.Error() != "Prompt blocked" { + t.Fatalf("error = %v, want Prompt blocked", err) + } +} + // ============================================================================= // Post-Hook Helpers Tests // ============================================================================= diff --git a/codex/bash_write.go b/codex/bash_write.go index c1eaaa0..d49f9a2 100644 --- a/codex/bash_write.go +++ b/codex/bash_write.go @@ -29,9 +29,11 @@ import ( // Implementation note: detection runs the command through a real Bash // parser (mvdan.cc/sh/v3/syntax) and walks the AST looking for // CallExpr statements whose primary command is `cat` or `tee` paired -// with a heredoc redirect (`<<` or `<<-`, quoted or unquoted) on the -// same Stmt. This replaced an earlier regex-based implementation that -// reported only the first match in a command — a multi-heredoc shape +// with a heredoc redirect (`<<` or `<<-`, quoted or unquoted). For +// `cat < allowed.txt @@ -54,63 +56,90 @@ import ( // entry overwrite our pick). Every recognised write becomes a // PatchFile in the order it appears in the command. // -// We deliberately fail closed in three places. (1) If the command is -// not valid Bash (parser error) this returns (nil, false) — silently -// treating malformed input as a no-op write would let an attacker -// construct a heredoc the parser rejects but Codex's actual shell -// accepts. (2) If a cat statement has a heredoc but no extractable -// target path (e.g. `cat < $(some_cmd)`), that single write is -// dropped from the result but the rest of the command's writes are -// still reported, and ok stays true as long as at least one well-formed -// write was found. (3) If a tee statement has a heredoc but ANY of its -// target paths can't be cleanly extracted, the entire statement is -// dropped — partial coverage of a multi-target tee is the exact -// confused-deputy bypass we're trying to prevent, so we'd rather hand -// the raw command to the unified bridge's fallback than report a -// subset of writes. Callers that need a stricter posture should layer -// their own checks; the unified bridge then surfaces the raw command -// through the existing fallback path. +// We deliberately fall back to the raw command in three places. (1) If +// the command has heredoc syntax but is not valid Bash, a real shell may +// still have completed earlier writes before the syntax error. (2) If a +// write target requires shell evaluation, such as `$HOME/.env`, +// reporting the surface token as FilePath would invite path policies to +// validate before canonicalizing. (3) If a tee statement has ANY target +// path that can't be cleanly extracted, partial coverage of the remaining +// targets is worse than raw fallback because callers reasonably assume a +// successful parse reported every write. In all three cases this returns +// ok=true with no PatchFiles so the unified bridge dispatches its +// fallback event instead of silently returning PostToolOK. func ParseBashRedirectWrite(command string) ([]PatchFile, bool) { + files, _, ok := ParseBashRedirectWriteWithFallback(command) + return files, ok +} + +// ParseBashRedirectWriteWithFallback behaves like ParseBashRedirectWrite, and +// also reports whether callers should dispatch a raw-command fallback in +// addition to any parsed files. +func ParseBashRedirectWriteWithFallback(command string) ([]PatchFile, bool, bool) { // Cheap pre-check: every heredoc-style write contains `<<`. This // keeps the parser allocation off the hot path for plain Bash // commands (`pwd`, `ls`, `git diff`, …) which dominate real Codex // traffic. if !strings.Contains(command, "<<") { - return nil, false + return nil, false, false } file, err := syntax.NewParser().Parse(strings.NewReader(command), "") if err != nil { - return nil, false + return nil, true, true } var files []PatchFile + needsFallback := false syntax.Walk(file, func(node syntax.Node) bool { + if bin, ok := node.(*syntax.BinaryCmd); ok { + switch patches, status := bashRedirectWriteFromPipeline(command, bin); status { + case writeFound: + files = append(files, patches...) + case writeFallback: + needsFallback = true + } + return true + } stmt, ok := node.(*syntax.Stmt) if !ok { return true } - if patches, ok := bashRedirectWriteFromStmt(command, stmt); ok { + switch patches, status := bashRedirectWriteFromStmt(command, stmt); status { + case writeFound: files = append(files, patches...) + case writeFallback: + needsFallback = true } return true }) if len(files) == 0 { - return nil, false + if needsFallback { + return nil, true, true + } + return nil, false, false } - return files, true + return files, needsFallback, true } +type bashWriteStatus int + +const ( + writeNone bashWriteStatus = iota + writeFound + writeFallback +) + // bashRedirectWriteFromStmt extracts one PatchFile per write target from // stmt if its primary command is a recognised heredoc-style file write // (cat or tee). Most statements yield a single PatchFile, but `tee // FILE1 FILE2 …` produces one PatchFile per target so every write goes -// through the per-file policy pipeline. Any other shape returns -// ok=false so syntax.Walk can quietly skip it. -func bashRedirectWriteFromStmt(command string, stmt *syntax.Stmt) ([]PatchFile, bool) { +// through the per-file policy pipeline. Unsupported write-looking shapes +// return writeFallback so the caller can surface the raw command. +func bashRedirectWriteFromStmt(command string, stmt *syntax.Stmt) ([]PatchFile, bashWriteStatus) { call, ok := stmt.Cmd.(*syntax.CallExpr) if !ok || len(call.Args) == 0 { - return nil, false + return nil, writeNone } switch call.Args[0].Lit() { case "cat": @@ -118,7 +147,46 @@ func bashRedirectWriteFromStmt(command string, stmt *syntax.Stmt) ([]PatchFile, case "tee": return teeRedirectWrite(command, stmt, call) } - return nil, false + return nil, writeNone +} + +// bashRedirectWriteFromPipeline handles `cat < FILE` and `cat > FILE <` (RdrAll) and `&>>` (AppAll) combine stdout+stderr // to one file; that file IS a real write target, so we treat them // like stdout-bound RdrOut/AppOut. -func catRedirectWrite(command string, stmt *syntax.Stmt) ([]PatchFile, bool) { +func catRedirectWrite(command string, stmt *syntax.Stmt) ([]PatchFile, bashWriteStatus) { var hdoc, out *syntax.Redirect for _, r := range stmt.Redirs { switch r.Op { case syntax.Hdoc, syntax.DashHdoc: if hdoc != nil { - return nil, false + return nil, writeFallback } hdoc = r case syntax.RdrOut, syntax.AppOut: @@ -158,15 +226,15 @@ func catRedirectWrite(command string, stmt *syntax.Stmt) ([]PatchFile, bool) { } } if hdoc == nil || out == nil { - return nil, false + return nil, writeNone } - path := wordText(command, out.Word) - if path == "" { - return nil, false + path, ok := wordText(out.Word) + if !ok || path == "" { + return nil, writeFallback } body, ok := heredocBodyContent(command, hdoc) if !ok { - return nil, false + return nil, writeFallback } return []PatchFile{{ Operation: "add", @@ -175,7 +243,7 @@ func catRedirectWrite(command string, stmt *syntax.Stmt) ([]PatchFile, bool) { OldString: "", NewString: body, }}, - }}, true + }}, writeFound } // teeRedirectWrite handles `tee [-a] FILE… [< /dev/null] < len(command) || start >= end { - return "" + return wordToLiteral(w) +} + +func wordHasTildeExpansion(w *syntax.Word) bool { + if len(w.Parts) == 0 { + return false } - return command[start:end] + lit, ok := w.Parts[0].(*syntax.Lit) + return ok && strings.HasPrefix(lit.Value, "~") } // wordToLiteral returns w's effective literal text if every part is a diff --git a/codex/bash_write_test.go b/codex/bash_write_test.go index d3b0f59..fcc6c53 100644 --- a/codex/bash_write_test.go +++ b/codex/bash_write_test.go @@ -130,8 +130,8 @@ func TestParseBashRedirectWrite_NoHeredoc_PlainBash(t *testing.T) { "ls -la", "git diff", "cat README.md", - "echo hello > greet.txt", // simple redirect (no heredoc) — out of scope - "printf 'hi' > greet.txt", // not handled here + "echo hello > greet.txt", // simple redirect (no heredoc) — out of scope + "printf 'hi' > greet.txt", // not handled here "apply_patch <<'PATCH'\n*** Begin Patch\n*** Add File: a\n+x\n*** End Patch\nPATCH", } for _, cmd := range tests { @@ -142,11 +142,16 @@ func TestParseBashRedirectWrite_NoHeredoc_PlainBash(t *testing.T) { } func TestParseBashRedirectWrite_HeredocMissingTerminator(t *testing.T) { - // Without the closing EOF line we can't trust the body — bail. + // Without the closing EOF line we can't trust the body. Return ok=true + // with no structured files so the bridge dispatches raw fallback. cmd := "cat <<'EOF' > greet.txt\nhello world\n" - if _, ok := ParseBashRedirectWrite(cmd); ok { - t.Errorf("expected ok=false for heredoc missing terminator") + files, ok := ParseBashRedirectWrite(cmd) + if !ok { + t.Fatalf("expected ok=true so the bridge can dispatch raw fallback") + } + if len(files) != 0 { + t.Errorf("expected no structured files for heredoc missing terminator; got %+v", files) } } @@ -368,8 +373,12 @@ func TestParseBashRedirectWrite_TeeQuotedPathEscapesContainment(t *testing.T) { func TestParseBashRedirectWrite_InvalidBash_FailsClosed(t *testing.T) { cmd := "cat <<'EOF' > greet.txt\nhello\n" // missing terminator - if files, ok := ParseBashRedirectWrite(cmd); ok { - t.Errorf("expected ok=false on invalid Bash; got %+v", files) + files, ok := ParseBashRedirectWrite(cmd) + if !ok { + t.Fatalf("expected ok=true so the bridge can dispatch raw fallback") + } + if len(files) != 0 { + t.Errorf("expected no structured files on invalid Bash; got %+v", files) } } @@ -382,8 +391,12 @@ func TestParseBashRedirectWrite_PartiallyMalformed_FailsClosed(t *testing.T) { // command. The first write must not slip through. cmd := "cat <<'EOF' > a.txt\nA\nEOF\ncat <<'EOF' > b.txt\nB\n" - if files, ok := ParseBashRedirectWrite(cmd); ok { - t.Errorf("expected ok=false when later heredoc lacks terminator; got %+v", files) + files, ok := ParseBashRedirectWrite(cmd) + if !ok { + t.Fatalf("expected ok=true so the bridge can dispatch raw fallback") + } + if len(files) != 0 { + t.Errorf("expected no structured files when later heredoc lacks terminator; got %+v", files) } } @@ -414,6 +427,98 @@ func TestParseBashRedirectWrite_TeeMultipleTargets_AllReported(t *testing.T) { } } +func TestParseBashRedirectWrite_CatPipeTeeReported(t *testing.T) { + cmd := "cat <<'EOF' | tee allowed.txt ../../.ssh/authorized_keys\nattacker-controlled\nEOF" + + got, ok := ParseBashRedirectWrite(cmd) + if !ok { + t.Fatalf("ParseBashRedirectWrite(...) ok = false; want true") + } + want := []PatchFile{ + {Operation: "add", FilePath: "allowed.txt", Edits: []PatchEdit{{OldString: "", NewString: "attacker-controlled"}}}, + {Operation: "add", FilePath: "../../.ssh/authorized_keys", Edits: []PatchEdit{{OldString: "", NewString: "attacker-controlled"}}}, + } + if !reflect.DeepEqual(got, want) { + t.Errorf("ParseBashRedirectWrite =\n %+v\nwant\n %+v", got, want) + } +} + +func TestParseBashRedirectWrite_CatPipeTeeStdoutRedirectReported(t *testing.T) { + cmd := "cat <<'EOF' | tee allowed.txt > ../../.ssh/authorized_keys\nattacker-controlled\nEOF" + + got, ok := ParseBashRedirectWrite(cmd) + if !ok { + t.Fatalf("ParseBashRedirectWrite(...) ok = false; want true") + } + want := []PatchFile{ + {Operation: "add", FilePath: "allowed.txt", Edits: []PatchEdit{{OldString: "", NewString: "attacker-controlled"}}}, + {Operation: "add", FilePath: "../../.ssh/authorized_keys", Edits: []PatchEdit{{OldString: "", NewString: "attacker-controlled"}}}, + } + if !reflect.DeepEqual(got, want) { + t.Errorf("ParseBashRedirectWrite =\n %+v\nwant\n %+v", got, want) + } +} + +func TestParseBashRedirectWrite_CatPipeTeeStdoutOnlyReported(t *testing.T) { + cmd := "cat <<'EOF' | tee > out.txt\nbody\nEOF" + + got, ok := ParseBashRedirectWrite(cmd) + if !ok { + t.Fatalf("ParseBashRedirectWrite(...) ok = false; want true") + } + want := []PatchFile{ + {Operation: "add", FilePath: "out.txt", Edits: []PatchEdit{{OldString: "", NewString: "body"}}}, + } + if !reflect.DeepEqual(got, want) { + t.Errorf("ParseBashRedirectWrite =\n %+v\nwant\n %+v", got, want) + } +} + +func TestParseBashRedirectWrite_NonLiteralTargetUsesFallback(t *testing.T) { + tests := []struct { + name string + cmd string + }{ + { + name: "environment expansion", + cmd: "cat <<'EOF' > $HOME/.ssh/authorized_keys\nkey\nEOF", + }, + { + name: "tilde expansion", + cmd: "cat <<'EOF' > ~/secret.txt\nsecret\nEOF", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + files, ok := ParseBashRedirectWrite(tt.cmd) + if !ok { + t.Fatalf("expected ok=true so the bridge can dispatch raw fallback") + } + if len(files) != 0 { + t.Fatalf("files = %+v, want raw fallback with no structured files", files) + } + }) + } +} + +func TestParseBashRedirectWrite_MixedParsedAndFallbackPreservesFiles(t *testing.T) { + cmd := "cat <<'EOF' > .env\nTOKEN=secret\nEOF\ncat <<'EOF' > $HOME/out.txt\nbody\nEOF" + + files, fallback, ok := ParseBashRedirectWriteWithFallback(cmd) + if !ok { + t.Fatalf("ParseBashRedirectWriteWithFallback(...) ok = false; want true") + } + if !fallback { + t.Fatalf("fallback = false, want true") + } + want := []PatchFile{ + {Operation: "add", FilePath: ".env", Edits: []PatchEdit{{OldString: "", NewString: "TOKEN=secret"}}}, + } + if !reflect.DeepEqual(files, want) { + t.Errorf("files =\n %+v\nwant\n %+v", files, want) + } +} + // Regression: `tee -a FILE1 FILE2 < 0 { return codex.PostToolBlock(strings.Join(blockReasons, "\n")) } @@ -877,7 +875,7 @@ func OnAfterFileEdit(handler FileEditHandler) { Command string `json:"command"` } json.Unmarshal(input.ToolInput, &applyInput) - return dispatchPatch(codex.ParseApplyPatch(applyInput.Command), applyInput.Command) + return dispatchPatch(codex.ParseApplyPatch(applyInput.Command), applyInput.Command, false) case "Bash": // Codex routes file operations through Bash in two @@ -898,17 +896,19 @@ func OnAfterFileEdit(handler FileEditHandler) { // because it's the higher-fidelity shape (per-hunk // old/new) when both detectors would match, then fall // back to the heredoc-write detector. Non-edit Bash - // commands short-circuit at the second `if !ok` check - // without paying for either full parse pass. + // commands short-circuit at the second `if !ok` check. + // Unsupported or unsafe write-like heredoc shapes return + // ok=true with an empty file list so dispatchPatch sends + // the raw command to the handler instead of dropping it. var bashInput struct { Command string `json:"command"` } json.Unmarshal(input.ToolInput, &bashInput) if files, ok := codex.ParseApplyPatchFromBash(bashInput.Command); ok { - return dispatchPatch(files, bashInput.Command) + return dispatchPatch(files, bashInput.Command, false) } - if files, ok := codex.ParseBashRedirectWrite(bashInput.Command); ok { - return dispatchPatch(files, bashInput.Command) + if files, fallback, ok := codex.ParseBashRedirectWriteWithFallback(bashInput.Command); ok { + return dispatchPatch(files, bashInput.Command, fallback) } return codex.PostToolOK() diff --git a/unified_test.go b/unified_test.go index b3afe29..25d708a 100644 --- a/unified_test.go +++ b/unified_test.go @@ -116,24 +116,24 @@ func TestStopDecision_Helpers(t *testing.T) { func TestExecutionContext_IsMCP(t *testing.T) { tests := []struct { - name string + name string execType ExecutionType - want bool + want bool }{ { - name: "ExecutionMCP returns true", + name: "ExecutionMCP returns true", execType: ExecutionMCP, - want: true, + want: true, }, { - name: "ExecutionShell returns false", + name: "ExecutionShell returns false", execType: ExecutionShell, - want: false, + want: false, }, { - name: "ExecutionTool returns false", + name: "ExecutionTool returns false", execType: ExecutionTool, - want: false, + want: false, }, } @@ -1240,6 +1240,87 @@ func TestCodexPostToolUse_BashNonApplyPatchCommandSkipped(t *testing.T) { } } +func TestCodexPostToolUse_BashMalformedHeredocFallsBackToRawCommand(t *testing.T) { + ClearHandlers() + defer ClearHandlers() + + var got []FileEditContext + OnAfterFileEdit(func(ctx FileEditContext) FileEditDecision { + got = append(got, ctx) + return FileEditOK() + }) + + cmd := "cat <<'EOF' > a.txt\\nA\\nEOF\\ncat <<'EOF' > b.txt\\nB\\n" + stdin := `{"session_id":"s","tool_name":"Bash","tool_input":{"command":"` + cmd + `"},"cwd":"/repo"}` + runHandlerWithStdin(t, "codex-post-tool-use", stdin) + + if len(got) != 1 { + t.Fatalf("handler invocations = %d, want raw fallback invocation; got=%+v", len(got), got) + } + if got[0].FilePath != "" { + t.Errorf("FilePath = %q, want empty for raw fallback", got[0].FilePath) + } + if len(got[0].Edits) != 1 || !strings.Contains(got[0].Edits[0].NewString, "cat <<'EOF' > b.txt") { + t.Errorf("fallback edit = %+v, want raw Bash command", got[0].Edits) + } +} + +func TestCodexPostToolUse_BashExpandedPathFallsBackToRawCommand(t *testing.T) { + ClearHandlers() + defer ClearHandlers() + + var got []FileEditContext + OnAfterFileEdit(func(ctx FileEditContext) FileEditDecision { + got = append(got, ctx) + return FileEditOK() + }) + + cmd := "cat <<'EOF' > $HOME/.ssh/authorized_keys\\nkey\\nEOF" + stdin := `{"session_id":"s","tool_name":"Bash","tool_input":{"command":"` + cmd + `"},"cwd":"/repo"}` + runHandlerWithStdin(t, "codex-post-tool-use", stdin) + + if len(got) != 1 { + t.Fatalf("handler invocations = %d, want raw fallback invocation; got=%+v", len(got), got) + } + if got[0].FilePath != "" { + t.Errorf("FilePath = %q, want empty for raw fallback", got[0].FilePath) + } + if len(got[0].Edits) != 1 || !strings.Contains(got[0].Edits[0].NewString, "$HOME/.ssh/authorized_keys") { + t.Errorf("fallback edit = %+v, want raw Bash command", got[0].Edits) + } +} + +func TestCodexPostToolUse_BashMixedParsedAndFallbackDispatchesBoth(t *testing.T) { + ClearHandlers() + defer ClearHandlers() + + var got []FileEditContext + OnAfterFileEdit(func(ctx FileEditContext) FileEditDecision { + got = append(got, ctx) + return FileEditOK() + }) + + cmd := "cat <<'EOF' > .env\\nTOKEN=secret\\nEOF\\ncat <<'EOF' > $HOME/out.txt\\nbody\\nEOF" + stdin := `{"session_id":"s","tool_name":"Bash","tool_input":{"command":"` + cmd + `"},"cwd":"/repo"}` + runHandlerWithStdin(t, "codex-post-tool-use", stdin) + + if len(got) != 2 { + t.Fatalf("handler invocations = %d, want parsed file plus raw fallback; got=%+v", len(got), got) + } + if got[0].FilePath != ".env" { + t.Errorf("got[0].FilePath = %q, want .env", got[0].FilePath) + } + if len(got[0].Edits) != 1 || got[0].Edits[0].NewString != "TOKEN=secret" { + t.Errorf("got[0].Edits = %+v, want parsed .env body", got[0].Edits) + } + if got[1].FilePath != "" { + t.Errorf("got[1].FilePath = %q, want empty for raw fallback", got[1].FilePath) + } + if len(got[1].Edits) != 1 || !strings.Contains(got[1].Edits[0].NewString, "$HOME/out.txt") { + t.Errorf("got[1].Edits = %+v, want raw Bash command", got[1].Edits) + } +} + func TestCodexPostToolUse_BashCatHeredocWriteDispatches(t *testing.T) { // Codex 0.130.0+ routes greenfield writes through a plain Bash // `cat <<'EOF' > FILE … EOF` heredoc rather than apply_patch or any @@ -1278,6 +1359,55 @@ func TestCodexPostToolUse_BashCatHeredocWriteDispatches(t *testing.T) { } } +func TestCodexPostToolUse_BashCatPipeTeeDispatches(t *testing.T) { + ClearHandlers() + defer ClearHandlers() + + var got []FileEditContext + OnAfterFileEdit(func(ctx FileEditContext) FileEditDecision { + got = append(got, ctx) + return FileEditOK() + }) + + cmd := "cat <<'EOF' | tee allowed.txt ../../.ssh/authorized_keys\\nattacker-controlled\\nEOF" + stdin := `{"session_id":"s","tool_name":"Bash","tool_input":{"command":"` + cmd + `"},"cwd":"/repo"}` + runHandlerWithStdin(t, "codex-post-tool-use", stdin) + + if len(got) != 2 { + t.Fatalf("handler invocations = %d, want 2 tee targets; got=%+v", len(got), got) + } + if got[0].FilePath != "allowed.txt" || got[1].FilePath != "../../.ssh/authorized_keys" { + t.Fatalf("paths = [%q, %q], want allowed.txt and ../../.ssh/authorized_keys", got[0].FilePath, got[1].FilePath) + } + for _, ctx := range got { + if len(ctx.Edits) != 1 || ctx.Edits[0].NewString != "attacker-controlled" { + t.Errorf("Edits = %+v, want tee body", ctx.Edits) + } + } +} + +func TestCodexPostToolUse_BashCatPipeTeeStdoutRedirectDispatches(t *testing.T) { + ClearHandlers() + defer ClearHandlers() + + var got []FileEditContext + OnAfterFileEdit(func(ctx FileEditContext) FileEditDecision { + got = append(got, ctx) + return FileEditOK() + }) + + cmd := "cat <<'EOF' | tee allowed.txt > ../../.ssh/authorized_keys\\nattacker-controlled\\nEOF" + stdin := `{"session_id":"s","tool_name":"Bash","tool_input":{"command":"` + cmd + `"},"cwd":"/repo"}` + runHandlerWithStdin(t, "codex-post-tool-use", stdin) + + if len(got) != 2 { + t.Fatalf("handler invocations = %d, want 2 tee targets; got=%+v", len(got), got) + } + if got[0].FilePath != "allowed.txt" || got[1].FilePath != "../../.ssh/authorized_keys" { + t.Fatalf("paths = [%q, %q], want allowed.txt and ../../.ssh/authorized_keys", got[0].FilePath, got[1].FilePath) + } +} + func TestCodexPostToolUse_BashCatHeredocBlockPropagates(t *testing.T) { // Blocking decisions on cat-heredoc writes must surface as // PostToolBlock JSON, identical to the apply_patch path. This is