diff --git a/docs/bash-tool-runtime.md b/docs/bash-tool-runtime.md index 34b691174..ad1c73b80 100644 --- a/docs/bash-tool-runtime.md +++ b/docs/bash-tool-runtime.md @@ -32,7 +32,7 @@ There are no structured `head` or `tail` tool parameters in the current schema, ## 2) Optional interception (blocked-command path) -If `bashInterceptor.enabled` is true, `BashTool` loads rules from settings (`getBashInterceptorRules()`) and runs `checkBashInterception()` against the command — checking both the original and the cwd-normalized form (after a leading `cd … &&` is extracted) when they differ. +If `bashInterceptor.enabled` is true, `BashTool` loads rules from settings (`getBashInterceptorRules()`) and runs `checkBashInterception()` against the command — checking both the original and the cwd-normalized form (after a leading `cd … &&` is extracted) when they differ. Rule syntax is unchanged: each rule checks the complete input first, then raw flat command fragments separated by unquoted/unescaped `&&`, `||`, `;`, `|`, `&`, or newlines, then those fragments with leading `NAME=value` assignments removed. Interception behavior: @@ -43,6 +43,7 @@ Interception behavior: - on block, `BashTool` throws `ToolError` with message: - `Blocked: ...` - original command included. +- heredocs, command substitutions, backticks, grouping, and malformed quoting do not produce extra fragments; they retain only the complete-input check. Interception is best-effort routing to dedicated tools, not a shell-security policy. Default rule patterns (defined in code) target common misuses: diff --git a/docs/settings.md b/docs/settings.md index 7f5c2bb10..3f1bc3c4d 100644 --- a/docs/settings.md +++ b/docs/settings.md @@ -171,6 +171,21 @@ Valid rule approvals are `allow`, `prompt`, and `deny`. Critical bash commands s Matching is asymmetric so that rules mean what they appear to: `deny` and `prompt` rules fire when the glob matches the whole command **or any single segment** of a compound line (split on `&&`, `||`, `;`, `|`, a single `&`, subshells, and newlines), so `match: "rm -rf *"` still denies `cd /tmp && rm -rf build` and `sleep 1 & rm -rf build`. `allow` rules must match the **entire** command and never apply to a compound line, so a narrow allow such as `match: "git *"` cannot vouch for `git status && rm -rf /`. +### Bash interceptor patterns + +`bashInterceptor` is separate from `bash.patterns`: it redirects Bash commands to dedicated tools rather than defining whether a command may execute. Enable it explicitly and configure regular-expression patterns with a replacement tool and a model-facing message: + +```yaml +bashInterceptor: + enabled: true + patterns: + - pattern: '^\s*(cat|head|tail)\s+' + tool: read + message: "Use the read tool instead." +``` + +The named replacement tool must be available in the current session or the interceptor does not block the Bash call. For a detailed comparison of permission policy and dedicated-tool routing, including compound-command behavior and ordering, see [the Bash tool documentation](tools/bash.md#command-policy-and-dedicated-tool-routing). + ### Worked example: global vs. project ```yaml diff --git a/docs/tools/bash.md b/docs/tools/bash.md index e3e868cdd..89d1aac6f 100644 --- a/docs/tools/bash.md +++ b/docs/tools/bash.md @@ -52,11 +52,82 @@ The tool returns a single `text` content block plus optional `details`. Stdout and stderr are merged before the model sees them. Definite non-zero exit codes are appended to the returned error result text as `Command exited with code `. +## Command policy and dedicated-tool routing + +Two independent settings can prevent a Bash subprocess from starting. They serve different purposes and run at different points in the tool-call lifecycle. + +| Setting | Purpose | Rule syntax | Result when matched | +| --- | --- | --- | --- | +| `bash.patterns` | Command-specific execution policy | Literal text with `*` wildcards | Allows the call, requests human approval, or denies it. | +| `bashInterceptor.patterns` | Prefer a dedicated tool over Bash | JavaScript regular expression, optional flags, tool name, and message | Returns a Bash tool error telling the model to call the named dedicated tool instead. | + +### `bash.patterns`: permission policy + +`bash.patterns` is for commands that must be allowed, confirmed by a person, or refused regardless of whether another tool could perform the work. Rules are ordered; the first matching rule wins. Each rule has a `match` glob and an `approval` value of `allow`, `prompt`, or `deny`. + +```yaml +bash: + patterns: + - match: "git *" + approval: allow + - match: "curl *" + approval: prompt + - match: "rm -rf *" + approval: deny +``` + +- `deny` stops the call before `BashTool.execute()` runs, including in `yolo` mode. +- `prompt` displays an approval request. Only an accepted request proceeds to `BashTool.execute()`. +- `allow` can lower the approval tier for a simple command, but it cannot approve a compound command. For example, `match: "git *"` does not approve `git status && rm -rf build`. +- `deny` and `prompt` check the complete command and each shell command segment. A rule such as `match: "rm -rf *"` therefore catches `cd /tmp && rm -rf build`. + +Use this setting for safety and user control. It remains useful for commands with no appropriate replacement tool, such as destructive removal, network access, deployment scripts, or project-specific scripts. + +### `bashInterceptor.patterns`: dedicated-tool routing + +`bashInterceptor` is an opt-in routing layer (`bashInterceptor.enabled` defaults to `false`). It is for commands that are technically valid Bash but are better expressed through an available dedicated tool. Each pattern is a regular expression and includes the name of that replacement tool and the explanation shown to the model. + +```yaml +bashInterceptor: + enabled: true + patterns: + - pattern: '^\s*(cat|head|tail)\s+' + tool: read + message: "Use the read tool instead; it handles binary files and provides better context." + - pattern: '^\s*(grep|rg)\s+' + tool: grep + message: "Use the grep tool instead; it respects .gitignore and returns structured results." +``` + +An interceptor rule only applies when its `tool` is available in the current session. If `read` is disabled, a `cat` rule targeting `read` does not block the Bash call. This makes the interceptor a best-effort capability preference rather than an execution-security boundary. + +The built-in default rules route common operations such as `cat` to `read`, `rg` to `grep`, in-place `sed` to `edit`, shell redirection to `write`, and unmanaged services/background processes to `hub`. See `DEFAULT_BASH_INTERCEPTOR_RULES` in `packages/coding-agent/src/config/settings-schema.ts` for the complete list. + +For compatibility with existing custom regexes, the interceptor always checks the complete original command first. It then checks raw, flat command fragments separated by unquoted and unescaped `&&`, `||`, `;`, `|`, `&`, or newlines. It also checks fragments after leading environment assignments are removed: + +```bash +git add file && git commit -m "message" +GIT_AUTHOR_NAME=Dev git commit -m "message" +``` + +An anchored rule such as `^\s*git\s+commit\b` can therefore match the `git commit` command in both examples. Quoted, escaped, and commented text is not treated as a command. Heredocs, command substitution, backticks, grouping, and malformed quoting retain only the complete-command check; the interceptor deliberately does not attempt to become a full shell parser. + +### Interaction and selection guide + +The approval policy is resolved before execution. A matching `bash.patterns` `deny` never reaches the interceptor. A matching `prompt` reaches the interceptor only after the user accepts the approval request. If an accepted call then matches an interceptor rule, the Bash call still does not run; the model receives the routing error and should invoke the dedicated tool. + +Avoid configuring the same operation in both places unless that two-step behavior is intended. For example, a `prompt` rule for `cat *` plus an enabled `cat`-to-`read` interceptor first asks the user to approve Bash, then rejects Bash and asks the model to use `read`. + +Choose the setting by the desired outcome: + +- Use `bash.patterns` when the question is **whether the command may execute**. +- Use `bashInterceptor.patterns` when the question is **which tool should perform the operation**. + ## Flow 1. `BashTool.execute()` in `packages/coding-agent/src/tools/bash.ts` reads `command`, normalizes `env`, and defaults `timeout` to `300`. Commands execute exactly as written — there is no pre-execution rewrite pass. 2. If `cwd` is absent, it rewrites a leading `cd && ...` into the structured `cwd` field and strips that prefix from `command`. 3. If `async: true` is requested while `async.enabled` is off, it throws `ToolError` before any execution. -4. If `bashInterceptor.enabled` is on, `checkBashInterception()` runs against both the original command and the `cd`-stripped command. A matching enabled rule throws before URL expansion or execution. +4. If `bashInterceptor.enabled` is on, `checkBashInterception()` runs against both the original command and the `cd`-stripped command. For each form, configured regexes still check the complete input first, then each flat command separated by unquoted/unescaped `&&`, `||`, `;`, `|`, `&`, or newlines, followed by versions of those fragments without leading `NAME=value` assignments. A matching enabled rule throws before URL expansion or execution. 5. `expandInternalUrls()` rewrites supported internal URLs inside `command`, each `env` value, and protocol-looking `cwd` values. Command replacements are shell-escaped; `env` and `cwd` replacements use raw filesystem/string values because they are not interpolated into shell text. 6. `resolveToCwd()` resolves `cwd` against `session.cwd`; `fs.stat()` verifies that the target exists and is a directory. 7. `clampTimeout("bash", requestedTimeoutSec)` enforces `TOOL_TIMEOUTS.bash` (`default: 300`, `min: 1`, `max: 3600`). When clamped, `#buildCompletedResult()` / `#buildBackgroundStartResult()` append a notice line. @@ -150,6 +221,7 @@ Stdout and stderr are merged before the model sees them. Definite non-zero exit - `strict = true` is set on `BashTool`; `concurrency` is resolved per call: `pty: true` is `"exclusive"` (it takes over the terminal UI), everything else is `"shared"`, so multiple non-pty bash calls in one assistant message run in parallel. When parallel calls overlap on the same shell session key, the first owns the persistent `Shell`; the rest run in isolated one-shot shells (see `shellSessionsInUse` in `bash-executor.ts`). - `command` URL expansions shell-escape replacements; `env` and `cwd` expansion use `noEscape: true` because they become environment values / filesystem paths, not shell text. - `checkBashInterception()` blocks only when the matching rule's `tool` name is present in `ctx.toolNames`; missing tools disable their corresponding rule. +- Interceptor configuration syntax is unchanged. It handles common flat command lists, not full shell parsing: heredocs, command substitution, backticks, grouping, and malformed quoting only receive the existing whole-input check. This is best-effort routing toward dedicated tools, not a security boundary. - Default interceptor rules come from `DEFAULT_BASH_INTERCEPTOR_RULES` in `packages/coding-agent/src/config/settings-schema.ts`: - `cat|head|tail|less|more` -> `read` - `grep|rg|ripgrep|ag|ack` -> `grep` diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 25c809467..f08b69980 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed Bash interceptor rules matching only the complete command input, so an anchored rule such as `^\s*git\s+commit\b` missed a later command in a flat compound command. Rules now also inspect unquoted/unescaped `&&`, `||`, `;`, `|`, `&`, and newline-separated command fragments, including forms with leading environment-variable assignments, while retaining the original whole-input match and conservatively skipping complex shell syntax. + ## [17.2.1] - 2026-07-30 ### Added diff --git a/packages/coding-agent/src/tools/bash-interceptor.ts b/packages/coding-agent/src/tools/bash-interceptor.ts index 8fca9e981..6d2e83613 100644 --- a/packages/coding-agent/src/tools/bash-interceptor.ts +++ b/packages/coding-agent/src/tools/bash-interceptor.ts @@ -6,6 +6,7 @@ * the specialized tools instead. */ import { type BashInterceptorRule, DEFAULT_BASH_INTERCEPTOR_RULES } from "../config/settings-schema"; +import { extractFlatShellCommandSegments } from "./shell-tokenize"; export interface InterceptionResult { /** If true, the bash command should be blocked */ @@ -32,6 +33,78 @@ function compileRules(rules: BashInterceptorRule[]): Array<{ rule: BashIntercept return compiled; } +/** Finds the end of a shell word, respecting quotes and escapes; returns null for incomplete syntax. */ +function skipShellWord(command: string, start: number): number | null { + let inSingle = false; + let inDouble = false; + for (let i = start; i < command.length; i++) { + const ch = command[i]; + if (inSingle) { + if (ch === "'") inSingle = false; + continue; + } + if (inDouble) { + if (ch === "\\") { + if (i + 1 >= command.length) return null; + i++; + continue; + } + if (ch === '"') inDouble = false; + continue; + } + if (ch === "'") { + inSingle = true; + continue; + } + if (ch === '"') { + inDouble = true; + continue; + } + if (ch === "\\") { + if (i + 1 >= command.length) return null; + i++; + continue; + } + if (ch === " " || ch === "\t") return i; + } + return inSingle || inDouble ? null : command.length; +} + +/** Removes leading `NAME=value` assignments without interpreting shell syntax. */ +function withoutLeadingEnvironmentAssignments(command: string): string | null { + let index = 0; + let foundAssignment = false; + while (index < command.length) { + while (command[index] === " " || command[index] === "\t") index++; + const assignmentStart = index; + if (!/[A-Za-z_]/.test(command[index] ?? "")) break; + let nameEnd = index + 1; + while (/[A-Za-z0-9_]/.test(command[nameEnd] ?? "")) nameEnd++; + if (command[nameEnd] !== "=") { + return foundAssignment ? command.slice(assignmentStart).trimStart() : null; + } + const wordEnd = skipShellWord(command, nameEnd + 1); + if (wordEnd === null) return null; + foundAssignment = true; + index = wordEnd; + if (index === command.length) return null; + } + if (!foundAssignment) return null; + const commandWithoutAssignments = command.slice(index).trimStart(); + return commandWithoutAssignments.length > 0 ? commandWithoutAssignments : null; +} + +function interceptionCandidates(command: string): string[] { + const candidates = [command.trim()]; + const segments = extractFlatShellCommandSegments(command); + candidates.push(...segments.map(segment => segment.trim())); + for (const segment of segments) { + const withoutAssignments = withoutLeadingEnvironmentAssignments(segment); + if (withoutAssignments) candidates.push(withoutAssignments); + } + return candidates; +} + /** * Check if a bash command should be intercepted. * @@ -43,10 +116,10 @@ export function checkBashInterception( command: string, availableTools: string[], rules: BashInterceptorRule[] = DEFAULT_BASH_INTERCEPTOR_RULES, + originalCommand = command, ): InterceptionResult { - // Normalize command for pattern matching - const normalizedCommand = command.trim(); const compiled = compileRules(rules); + const candidates = interceptionCandidates(command); for (const { rule, regex } of compiled) { // Only block if the suggested tool is actually available @@ -54,12 +127,16 @@ export function checkBashInterception( continue; } - if (regex.test(normalizedCommand)) { - return { - block: true, - message: `Blocked: ${rule.message}\n\nOriginal command: ${command}`, - suggestedTool: rule.tool, - }; + for (const candidate of candidates) { + // A configured global or sticky regex carries state across calls. + regex.lastIndex = 0; + if (regex.test(candidate)) { + return { + block: true, + message: `Blocked: ${rule.message}\n\nOriginal command: ${originalCommand}`, + suggestedTool: rule.tool, + }; + } } } diff --git a/packages/coding-agent/src/tools/bash.ts b/packages/coding-agent/src/tools/bash.ts index 0d7fffeb7..6b1e79ec5 100644 --- a/packages/coding-agent/src/tools/bash.ts +++ b/packages/coding-agent/src/tools/bash.ts @@ -920,7 +920,7 @@ export class BashTool implements AgentTool { + const segment = command.slice(segmentStart, end).trim(); + if (segment.length > 0) segments.push(segment); + }; + + for (let i = 0; i < command.length; i++) { + const ch = command[i]; + if (inSingle) { + if (ch === "'") inSingle = false; + continue; + } + if (inDouble) { + if (ch === "\\") { + if (i + 1 >= command.length) return []; + i++; + continue; + } + if (ch === '"') { + inDouble = false; + continue; + } + if (ch === "`" || (ch === "$" && command[i + 1] === "(")) return []; + continue; + } + + if (ch === "'") { + inSingle = true; + atWordStart = false; + continue; + } + if (ch === '"') { + inDouble = true; + atWordStart = false; + continue; + } + if (ch === "\\") { + if (i + 1 >= command.length) return []; + i++; + atWordStart = false; + continue; + } + if ( + ch === "`" || + ch === "(" || + ch === ")" || + (ch === "$" && command[i + 1] === "(") || + (ch === "<" && command[i + 1] === "<") || + ((ch === "{" || ch === "}") && + atWordStart && + (command[i + 1] === undefined || /[ \t\n;]/.test(command[i + 1]))) + ) { + return []; + } + if (ch === "#" && atWordStart) { + pushSegment(i); + const newline = command.indexOf("\n", i + 1); + if (newline === -1) return segments; + i = newline; + segmentStart = newline + 1; + atWordStart = true; + continue; + } + if (ch === "\n" || ch === ";" || ch === "|" || ch === "&") { + pushSegment(i); + if ((ch === "|" || ch === "&") && command[i + 1] === ch) i++; + segmentStart = i + 1; + atWordStart = true; + continue; + } + atWordStart = ch === " " || ch === "\t"; + } + + if (inSingle || inDouble) return []; + pushSegment(command.length); + return segments; +} diff --git a/packages/coding-agent/test/tools/bash-interceptor.test.ts b/packages/coding-agent/test/tools/bash-interceptor.test.ts index d9309ccb8..8b1a38fdc 100644 --- a/packages/coding-agent/test/tools/bash-interceptor.test.ts +++ b/packages/coding-agent/test/tools/bash-interceptor.test.ts @@ -54,11 +54,86 @@ describe("BashTool interception", () => { }, ]); + const command = "cd packages/coding-agent && cat package.json"; await expect( - tool.execute("tool-call", { command: "cd packages/coding-agent && cat package.json" }, undefined, undefined, { + tool.execute("tool-call", { command }, undefined, undefined, { toolNames: ["read"], } as AgentToolContext), - ).rejects.toThrow("Use read instead"); + ).rejects.toThrow(`Use read instead.\n\nOriginal command: ${command}`); + }); +}); + +describe("compound command interception", () => { + const rules: BashInterceptorRule[] = [ + { + pattern: "^\\s*git\\s+commit\\b", + tool: "commit", + message: "Use the commit tool instead.", + }, + ]; + + it.each([ + "git commit -m message", + "git add file && git commit -m message", + "git add file; git commit -m message", + "git add file || git commit -m message", + "git add file | git commit -m message", + "git add file & git commit -m message", + "git add file\ngit commit -m message", + ])("blocks a later command after %s", command => { + expect(checkBashInterception(command, ["commit"], rules).block).toBe(true); + }); + + it("removes one or more leading environment assignments before matching", () => { + expect( + checkBashInterception('GIT_AUTHOR_EMAIL="a@example.com" git commit -m message', ["commit"], rules).block, + ).toBe(true); + expect( + checkBashInterception( + 'GIT_AUTHOR_EMAIL="a@example.com" GIT_AUTHOR_NAME=Dev git commit -m message', + ["commit"], + rules, + ).block, + ).toBe(true); + }); + + it("does not treat quoted, escaped, or commented text as a later command", () => { + for (const command of [ + "printf '%s\\n' \"git add file && git commit -m message\"", + 'echo "git commit"', + "echo git\\ commit", + "echo ok # git commit -m message", + ]) { + expect(checkBashInterception(command, ["commit"], rules).block).toBe(false); + } + }); + + it("does not add matches for unsupported shell syntax", () => { + for (const command of [ + 'echo "$(git commit -m message)"', + "echo `git commit -m message`", + "( git commit -m message )", + "echo start; { true; git commit -m message; }", + "cat <<'EOF'\ngit commit -m message\nEOF", + ]) { + expect(checkBashInterception(command, ["commit"], rules).block).toBe(false); + } + }); + + it("keeps matching a rule written for the complete original input", () => { + const command = "git add file && git commit -m message"; + const completeInputRule: BashInterceptorRule[] = [ + { + pattern: "^git add file && git commit", + tool: "commit", + message: "Use the commit tool instead.", + }, + ]; + expect(checkBashInterception(command, ["commit"], completeInputRule).block).toBe(true); + }); + + it("does not block when the suggested tool is unavailable", () => { + expect(checkBashInterception("git add file && git commit -m message", [], rules).block).toBe(false); }); });