diff --git a/docs/settings.md b/docs/settings.md index b4290684f..61cb322f5 100644 --- a/docs/settings.md +++ b/docs/settings.md @@ -169,7 +169,7 @@ bash: Valid rule approvals are `allow`, `prompt`, and `deny`. Critical bash commands still require confirmation unless a matching rule explicitly denies them; broad allow rules such as `match: "*"` do not bypass the critical-command guard. -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 `&&`, `||`, `;`, `|`, and newlines), so `match: "rm -rf *"` still denies `cd /tmp && 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 /`. +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 /`. ### Worked example: global vs. project diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index e270bf226..d5bc8c7bb 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -12,7 +12,7 @@ - Fixed the Docker `natives-builder` stage failing to build releases ≥ 17.1.1: the native audio stack added bindgen (miniaudio needs libclang) and a bundled-opus CMake build (needs cmake + make), none of which were installed in the slim builder image. - Fixed `omp usage` duplicating org-less legacy accounts as "no usage data" rows whenever any sibling report carried an organization (mixed pools of pre-org-capture rows and fresh org-scoped logins): an org-less account is now covered by its own org-less report, while org-attributed sibling reports still never count as its coverage. - `omp usage` revalidates the broker credential snapshot before rendering: live usage reports were previously paired with a disk-cached account list up to an hour old, so a just-completed re-login (org-less row upserted to org-scoped) rendered as a phantom duplicate until the cache expired. -- Fixed `bash.patterns` `deny`/`prompt` rules matching only against the whole command string, so a dangerous command in any non-leading position of a compound line (e.g. `cd /tmp && rm -rf /tmp/x`) silently bypassed a `deny` rule and, under `approvalMode: yolo`, executed with no prompt. `deny`/`prompt` rules now also match each command segment (split on `&&`, `||`, `;`, `|`, newlines); `allow` rules still require the whole command to match and never apply to compound lines ([#6695](https://github.com/can1357/oh-my-pi/issues/6695)). +- Fixed `bash.patterns` `deny`/`prompt` rules matching only against the whole command string, so a dangerous command in any non-leading position of a compound line (e.g. `cd /tmp && rm -rf /tmp/x`, `sleep 1 & rm -rf /tmp/x`) silently bypassed a `deny` rule and, under `approvalMode: yolo`, executed with no prompt. `deny`/`prompt` rules now also match each command segment, split with a shell-aware tokenizer that honors every command boundary (`&&`, `||`, `;`, `|`, single `&`, subshells, newlines) and quoting; `allow` rules still require the whole command to match and never apply to compound lines ([#6695](https://github.com/can1357/oh-my-pi/issues/6695)). ## [17.1.3] - 2026-07-24 diff --git a/packages/coding-agent/src/tools/bash.ts b/packages/coding-agent/src/tools/bash.ts index fb9c89287..86bf6a730 100644 --- a/packages/coding-agent/src/tools/bash.ts +++ b/packages/coding-agent/src/tools/bash.ts @@ -48,6 +48,7 @@ import { previewWindowRows, replaceTabs, } from "./render-utils"; +import { tokenizeShellSegments } from "./shell-tokenize"; import { ToolAbortError, ToolError } from "./tool-errors"; import { toolResult } from "./tool-result"; import { clampTimeout, TOOL_TIMEOUTS } from "./tool-timeouts"; @@ -189,15 +190,13 @@ function commandMatchesBashApprovalPattern(command: string, pattern: string): bo return bashApprovalPatternToRegExp(pattern).test(normalizedCommand); } -// Shell operators that separate a compound command into independently executed -// segments. `deny`/`prompt` rules are matched per segment so a dangerous command -// buried in a compound line (`cd x && rm -rf /`) is still caught. -const BASH_COMMAND_SEGMENT_RE = /&&|\|\||[;|\n\r]/u; - +// `deny`/`prompt` rules are matched per segment so a dangerous command buried in +// a compound line (`cd x && rm -rf /`, `sleep 1 & rm -rf /`) is still caught. +// Reuse the shared shell tokenizer so segmentation stays in one place and honors +// every command boundary (`;`, `&&`, `||`, `|`, `&`, subshells, newlines). function bashCommandSegments(command: string): string[] { - return command - .split(BASH_COMMAND_SEGMENT_RE) - .map(segment => normalizeBashApprovalPattern(segment)) + return tokenizeShellSegments(command) + .map(segment => segment.join(" ")) .filter(segment => segment.length > 0); } diff --git a/packages/coding-agent/src/tools/gh-cache-invalidation.ts b/packages/coding-agent/src/tools/gh-cache-invalidation.ts index c156b6cc7..67b441ae0 100644 --- a/packages/coding-agent/src/tools/gh-cache-invalidation.ts +++ b/packages/coding-agent/src/tools/gh-cache-invalidation.ts @@ -18,6 +18,7 @@ * dwarfs the cost of one cache miss. */ import { invalidateAllForNumber, invalidateAllForRepo } from "./github-cache"; +import { tokenizeShellSegments } from "./shell-tokenize"; const PR_URL_PATTERN = /^https:\/\/github\.com\/([^/\s]+\/[^/\s]+)\/pull\/(\d+)(?:[/?#].*)?$/i; const ISSUE_URL_PATTERN = /^https:\/\/github\.com\/([^/\s]+\/[^/\s]+)\/issues\/(\d+)(?:[/?#].*)?$/i; @@ -154,87 +155,6 @@ function detectGhMutation(tokens: readonly string[]): { number?: number; repo?: return repo !== undefined ? { repo } : {}; } -/** - * Conservative tokenizer that splits a bash command into individual word - * tokens. Handles single/double-quoted strings, backslash escapes, and - * standard operators (`;`, `&&`, `||`, `|`, `&`, newlines) as token - * boundaries that emit a sentinel `";"` so the caller treats the segments - * as independent command sequences. We do not attempt full POSIX shell - * parsing — heredocs, command substitution, and arithmetic expansion are - * out of scope; the detector simply falls through when it cannot find a - * clean `gh issue|pr ` triple. - */ -function tokenize(command: string): string[][] { - const segments: string[][] = []; - let current: string[] = []; - let buffer = ""; - let inSingle = false; - let inDouble = false; - const pushBuffer = () => { - if (buffer.length > 0) { - current.push(buffer); - buffer = ""; - } - }; - const pushSegment = () => { - pushBuffer(); - if (current.length > 0) segments.push(current); - current = []; - }; - for (let i = 0; i < command.length; i++) { - const ch = command[i]; - if (inSingle) { - if (ch === "'") { - inSingle = false; - continue; - } - buffer += ch; - continue; - } - if (inDouble) { - if (ch === "\\" && i + 1 < command.length) { - const next = command[i + 1]; - if (next === '"' || next === "\\" || next === "$" || next === "`") { - buffer += next; - i++; - continue; - } - } - if (ch === '"') { - inDouble = false; - continue; - } - buffer += ch; - continue; - } - if (ch === "'") { - inSingle = true; - continue; - } - if (ch === '"') { - inDouble = true; - continue; - } - if (ch === "\\" && i + 1 < command.length) { - buffer += command[i + 1]; - i++; - continue; - } - if (ch === " " || ch === "\t") { - pushBuffer(); - continue; - } - if (ch === "\n" || ch === ";" || ch === "&" || ch === "|" || ch === "(" || ch === ")") { - pushSegment(); - // `&&`, `||` already collapsed by the segment break above. - continue; - } - buffer += ch; - } - pushSegment(); - return segments; -} - /** * Drop `github-cache` rows for any `gh issue|pr ` call * embedded in `command`. Safe to invoke unconditionally; no-op when the @@ -242,7 +162,7 @@ function tokenize(command: string): string[][] { */ export function invalidateGithubCacheForBashCommand(command: string): void { if (!command?.includes("gh")) return; - const segments = tokenize(command); + const segments = tokenizeShellSegments(command); for (const segment of segments) { const hit = detectGhMutation(segment); if (!hit) continue; diff --git a/packages/coding-agent/src/tools/shell-tokenize.ts b/packages/coding-agent/src/tools/shell-tokenize.ts new file mode 100644 index 000000000..a0541ab67 --- /dev/null +++ b/packages/coding-agent/src/tools/shell-tokenize.ts @@ -0,0 +1,83 @@ +/** + * Conservative shell command tokenizer shared by the bash approval-pattern + * matcher and the gh-cache invalidator. + * + * Splits a bash command into independent command segments, each a list of word + * tokens. Handles single/double-quoted strings, backslash escapes, and the + * standard operators (`;`, `&&`, `||`, `|`, `&`, `(`, `)`, newlines) as segment + * boundaries so callers treat the pieces as independent command sequences. + * + * It is deliberately not a full POSIX parser — heredocs, command substitution, + * and arithmetic expansion are out of scope; callers fall through when they + * cannot find the structure they need. + */ +export function tokenizeShellSegments(command: string): string[][] { + const segments: string[][] = []; + let current: string[] = []; + let buffer = ""; + let inSingle = false; + let inDouble = false; + const pushBuffer = () => { + if (buffer.length > 0) { + current.push(buffer); + buffer = ""; + } + }; + const pushSegment = () => { + pushBuffer(); + if (current.length > 0) segments.push(current); + current = []; + }; + for (let i = 0; i < command.length; i++) { + const ch = command[i]; + if (inSingle) { + if (ch === "'") { + inSingle = false; + continue; + } + buffer += ch; + continue; + } + if (inDouble) { + if (ch === "\\" && i + 1 < command.length) { + const next = command[i + 1]; + if (next === '"' || next === "\\" || next === "$" || next === "`") { + buffer += next; + i++; + continue; + } + } + if (ch === '"') { + inDouble = false; + continue; + } + buffer += ch; + continue; + } + if (ch === "'") { + inSingle = true; + continue; + } + if (ch === '"') { + inDouble = true; + continue; + } + if (ch === "\\" && i + 1 < command.length) { + buffer += command[i + 1]; + i++; + continue; + } + if (ch === " " || ch === "\t") { + pushBuffer(); + continue; + } + if (ch === "\n" || ch === ";" || ch === "&" || ch === "|" || ch === "(" || ch === ")") { + pushSegment(); + // `&&`, `||` already collapsed by the segment break above. + continue; + } + buffer += ch; + } + pushSegment(); + return segments; +} diff --git a/packages/coding-agent/test/tools/approval.test.ts b/packages/coding-agent/test/tools/approval.test.ts index ab8195169..c222a1713 100644 --- a/packages/coding-agent/test/tools/approval.test.ts +++ b/packages/coding-agent/test/tools/approval.test.ts @@ -290,6 +290,11 @@ describe("tool-owned dynamic approval declarations", () => { expect(bashApproval("cd /tmp && rm -rf /tmp/scratch-b && echo done", settingsOverrides)).toEqual(denied); expect(bashApproval("echo start; rm -rf /var/x", settingsOverrides)).toEqual(denied); expect(bashApproval("cat f | rm -rf /var/x", settingsOverrides)).toEqual(denied); + // Single `&` (background) and subshells are command boundaries too. + expect(bashApproval("sleep 1 & rm -rf /tmp/scratch-b", settingsOverrides)).toEqual(denied); + expect(bashApproval("(rm -rf /tmp/scratch-b)", settingsOverrides)).toEqual(denied); + // Quotes around the binary do not hide it from a deny rule. + expect(bashApproval('cd /tmp && "rm" -rf /tmp/scratch-b', settingsOverrides)).toEqual(denied); // Segments that do not match the glob must not be denied by it. `rm -rf` // on a relative target has no leading `/`, so the `/`-anchored rule stays out.