smashimo/oh-my-pi
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(coding-agent): segment bash approval commands with shell-aware tokenizer

Browse Source 
The regex splitter only recognized `&&`, `||`, `;`, `|` and newlines, so a
single `&` (background operator) — also a command terminator — slipped a
dangerous command past a deny rule (`sleep 1 & rm -rf /tmp/x`), which under
approvalMode: yolo executed with no prompt.

Extract the shell-aware tokenizer from gh-cache-invalidation into a shared
tools/shell-tokenize.ts and reuse it for deny/prompt segmentation. It honors
every command boundary (`&&`, `||`, `;`, `|`, single `&`, subshells,
newlines) plus quoting and escapes, so both callers share one implementation.

Fixes #6695
This commit is contained in:
roboomp
2026-07-26 11:36:19 +00:00
parent 85110c366c
commit e650607cad
6 changed files with 99 additions and 92 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/settings.md
+1 -1
View File
@@ -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
packages/coding-agent/CHANGELOG.md
+1 -1
View File
@@ -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
packages/coding-agent/src/tools/bash.ts
+7 -8
View File
@@ -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);
}
packages/coding-agent/src/tools/gh-cache-invalidation.ts
+2 -82
View File
@@ -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 <subcmd>` 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 <mutating-subcmd>` 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;
packages/coding-agent/src/tools/shell-tokenize.ts
+83
View File
@@ -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;
}
packages/coding-agent/test/tools/approval.test.ts
+5
View File
@@ -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.