diff --git a/internal/tools/credentialed_exec.go b/internal/tools/credentialed_exec.go index 7d818d48..a948620f 100644 --- a/internal/tools/credentialed_exec.go +++ b/internal/tools/credentialed_exec.go @@ -41,8 +41,71 @@ func parseCommandBinary(command string) (binary string, args []string, err error return words[0], words[1:], nil } +// detectUnquotedShellOperators scans a command string for shell metacharacters +// that appear OUTSIDE of single or double quotes. This prevents false positives +// when argument values contain characters like | (e.g. --jq '.[0] | .name'). +// Returns the list of detected operators, or nil if the command is clean. +func detectUnquotedShellOperators(command string) []string { + unquoted := extractUnquotedSegments(command) + if unquoted == "" { + return nil + } + return detectShellOperators(unquoted) +} + +// extractUnquotedSegments returns a string containing only the characters +// from command that are outside of single-quoted and double-quoted segments. +// Backslash escaping is handled both inside double quotes (\") and outside +// quotes (\' \" \\) to match go-shellwords parsing behavior — without this, +// \" outside quotes would incorrectly enter double-quote mode and hide +// subsequent shell operators from detection. +func extractUnquotedSegments(command string) string { + var buf strings.Builder + buf.Grow(len(command)) + + inSingle := false + inDouble := false + for i := 0; i < len(command); i++ { + ch := command[i] + switch { + case inSingle: + if ch == '\'' { + inSingle = false + } + case inDouble: + if ch == '\\' && i+1 < len(command) { + i++ // skip escaped character inside double quotes + } else if ch == '"' { + inDouble = false + } + default: + switch ch { + case '\\': + // Backslash outside quotes escapes the next character, preventing + // it from being treated as a quote delimiter. Both the backslash + // and the escaped character are emitted as unquoted content so + // that operator detection still sees them (e.g. \; remains visible). + buf.WriteByte(ch) + if i+1 < len(command) { + i++ + buf.WriteByte(command[i]) + } + case '\'': + inSingle = true + case '"': + inDouble = true + default: + buf.WriteByte(ch) + } + } + } + return buf.String() +} + // detectShellOperators scans a raw command string for shell metacharacters. // Returns the list of detected operators, or nil if the command is clean. +// NOTE: This function does not respect quoting — use detectUnquotedShellOperators +// for credentialed exec where argument values may contain metacharacters. func detectShellOperators(command string) []string { matches := shellOperatorPattern.FindAllString(command, -1) if len(matches) == 0 { @@ -101,17 +164,20 @@ func matchesBinaryDeny(args []string, denyPatternsJSON json.RawMessage) string { // executeCredentialed runs a CLI command in Direct Exec Mode (no shell). // Credentials are injected as env vars into the child process only. +// rawCommand is the original command string before shell-word parsing (preserves quoting). func (t *ExecTool) executeCredentialed(ctx context.Context, cred *store.SecureCLIBinary, - binary string, args []string, cwd string, sandboxKey string) *Result { + binary string, args []string, cwd string, sandboxKey string, rawCommand string) *Result { // Step 0: Reject NUL bytes (defense-in-depth — also checked in Execute()). - rawCommand := binary + " " + strings.Join(args, " ") if strings.ContainsRune(rawCommand, '\x00') { return ErrorResult("command contains invalid NUL byte") } - // Step 1: Check for shell operators (early detection for clear error) - if ops := detectShellOperators(rawCommand); len(ops) > 0 { + // Step 1: Check for shell operators in the ORIGINAL command (preserves quoting). + // We check the raw command string (before shell-word parsing) so that characters + // inside quoted argument values (e.g. | in --jq '.[0] | ...') are not falsely flagged. + // Only top-level (unquoted) shell operators indicate actual command chaining attempts. + if ops := detectUnquotedShellOperators(rawCommand); len(ops) > 0 { return credentialedShellOperatorError(rawCommand, ops) } @@ -302,10 +368,10 @@ func (t *ExecTool) lookupCredentialedBinary(ctx context.Context, command string) return nil, "", nil } if cred == nil { - slog.Warn("secure_cli.lookup: no credential found", "binary", binary, "agent_id", agentID) + slog.Debug("secure_cli.lookup: no credential found", "binary", binary, "agent_id", agentID) return nil, "", nil } - slog.Info("secure_cli.lookup: found credential", "binary", binary, "cred_id", cred.ID, "env_size", len(cred.EncryptedEnv)) + slog.Debug("secure_cli.lookup: found credential", "binary", binary, "cred_id", cred.ID, "env_size", len(cred.EncryptedEnv)) return cred, binary, args } diff --git a/internal/tools/credentialed_exec_test.go b/internal/tools/credentialed_exec_test.go new file mode 100644 index 00000000..6e950867 --- /dev/null +++ b/internal/tools/credentialed_exec_test.go @@ -0,0 +1,122 @@ +package tools + +import "testing" + +func TestDetectShellOperators(t *testing.T) { + tests := []struct { + name string + command string + want int // number of detected operators + }{ + {"clean command", "gh api repos/foo/bar", 0}, + {"pipe operator", "gh api foo | jq .", 1}, + {"semicolon", "echo a; echo b", 1}, + {"ampersand", "cmd1 && cmd2", 1}, + {"redirect", "cmd > /tmp/out", 1}, + {"backtick", "echo `whoami`", 1}, + {"subshell", "echo $(whoami)", 1}, + {"multiple operators", "cmd1 | cmd2 && cmd3", 2}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ops := detectShellOperators(tt.command) + if len(ops) != tt.want { + t.Errorf("detectShellOperators(%q) = %v (len %d), want len %d", tt.command, ops, len(ops), tt.want) + } + }) + } +} + +func TestExtractUnquotedSegments(t *testing.T) { + tests := []struct { + name string + command string + want string + }{ + {"no quotes", "gh api foo", "gh api foo"}, + {"single quoted pipe", "gh --jq '.[0] | .name'", "gh --jq "}, + {"double quoted pipe", `gh --jq ".[0] | .name"`, "gh --jq "}, + {"mixed quotes", `gh --jq '.[0] | .a' --format "b | c"`, "gh --jq --format "}, + {"escaped quote in double", `gh "say \"hello\""`, "gh "}, + {"empty single quotes", "gh ''", "gh "}, + {"unquoted metachar", "gh api foo | jq", "gh api foo | jq"}, + // Backslash escape outside quotes: \" should NOT start double-quoting + {"escaped dquote outside", `gh api \"foo | bar\"`, `gh api \"foo | bar\"`}, + {"escaped squote outside", `gh api \'foo | bar\'`, `gh api \'foo | bar\'`}, + {"double backslash", `gh api \\arg`, `gh api \\arg`}, + {"backslash at end", `gh api foo\`, `gh api foo\`}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := extractUnquotedSegments(tt.command) + if got != tt.want { + t.Errorf("extractUnquotedSegments(%q) = %q, want %q", tt.command, got, tt.want) + } + }) + } +} + +func TestDetectUnquotedShellOperators(t *testing.T) { + tests := []struct { + name string + command string + want int + }{ + // Should NOT detect (inside quotes) + {"pipe in single quotes", "gh api repos/foo --jq '.[0] | .name'", 0}, + {"pipe in double quotes", `gh api repos/foo --jq ".[0] | .name"`, 0}, + {"semicolon in quotes", `echo 'a; b'`, 0}, + {"backtick in single quotes", "echo 'hello `world`'", 0}, + {"complex jq", `gh api repos/org/repo/commits --jq '.[0] | "SHA: \(.sha)\nAuthor: \(.commit.author.name)"'`, 0}, + // Should detect (outside quotes) + {"unquoted pipe", "gh api foo | jq .", 1}, + {"unquoted semicolon", "echo a; echo b", 1}, + {"mixed: quoted safe + unquoted unsafe", "gh --jq '.[0] | .x' | cat", 1}, + {"redirect after quotes", "gh api foo --jq '.x' > out.json", 1}, + // Escaped quotes outside quotes: operators after \" must still be detected + // (backslash prevents " from starting a quoted section) + {"escaped dquote then pipe", `gh \"arg\" | env`, 1}, + {"escaped dquote with content pipe", `gh api \"foo | bar\"`, 1}, + {"escaped squote then pipe", `gh api \'foo | bar\'`, 1}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ops := detectUnquotedShellOperators(tt.command) + if len(ops) != tt.want { + t.Errorf("detectUnquotedShellOperators(%q) = %v (len %d), want len %d", tt.command, ops, len(ops), tt.want) + } + }) + } +} + +func TestParseCommandBinary(t *testing.T) { + tests := []struct { + name string + command string + wantBinary string + wantArgs int + wantErr bool + }{ + {"simple", "gh api foo", "gh", 2, false}, + {"with quotes", "gh api --jq '.[0] | .name'", "gh", 3, false}, + {"empty", "", "", 0, true}, + {"single binary", "gh", "gh", 0, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + binary, args, err := parseCommandBinary(tt.command) + if (err != nil) != tt.wantErr { + t.Errorf("parseCommandBinary(%q) err = %v, wantErr %v", tt.command, err, tt.wantErr) + return + } + if !tt.wantErr { + if binary != tt.wantBinary { + t.Errorf("binary = %q, want %q", binary, tt.wantBinary) + } + if len(args) != tt.wantArgs { + t.Errorf("args len = %d, want %d (args: %v)", len(args), tt.wantArgs, args) + } + } + }) + } +} diff --git a/internal/tools/shell.go b/internal/tools/shell.go index 5d7e1829..5d2904e6 100644 --- a/internal/tools/shell.go +++ b/internal/tools/shell.go @@ -221,7 +221,7 @@ func (t *ExecTool) Execute(ctx context.Context, args map[string]any) *Result { } } sandboxKey := ToolSandboxKeyFromCtx(ctx) - return t.executeCredentialed(ctx, cred, binary, cmdArgs, cwd, sandboxKey) + return t.executeCredentialed(ctx, cred, binary, cmdArgs, cwd, sandboxKey, command) } // Exec approval check (matching TS exec-approval.ts pipeline)