fix(tools): quote-aware shell operator detection in credentialed exec (#700) (#702)

* fix(tools): quote-aware shell operator detection in credentialed exec (#700)

- Replace detectShellOperators with detectUnquotedShellOperators in
  credentialed exec path — respects single/double quoting so that
  characters like | inside argument values (e.g. --jq '.[0] | .name')
  are not falsely flagged as shell operators
- Pass raw command string (preserving quotes) to executeCredentialed
  instead of reconstructing from parsed args
- Downgrade "no credential found" log from Warn to Debug (fires for
  every non-credentialed command, too noisy at Warn)
- Add extractUnquotedSegments() helper with comprehensive tests

* fix(tools): handle backslash escape outside quotes in shell operator detection

extractUnquotedSegments did not handle \ as an escape character outside
of quotes, causing \" to incorrectly enter double-quote mode. This hid
subsequent shell operators from detection (e.g. gh \"arg\" | env would
not detect the unquoted pipe).

Add backslash escape handling in the unquoted state to match
go-shellwords parsing behavior. Both \ and the escaped character are
emitted as unquoted content so operator detection still catches them.

---------

Co-authored-by: viettranx <viettranx@gmail.com>

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
 }
 

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)
+				}
+			}
+		})
+	}
+}

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)