fix(guardrails): isolated-session remedy, crash fail-open, strict option parse (0.29.10)

[kyle-sexton]

Closes #3130

Summary

block-hook-bypass documented a dead end for isolated sessions (shell write blocked, Write/Edit refused) and failed open on its own crash with no signal. Option values other than exact true silently disabled the guard.

Fix

The block message names block_hook_bypass_scratch_roots and session-scoped --settings ahead of the user-global switch. The operator-only sentence moves to systemMessage. Crash posture stays fail-open (hottest-path availability) and now emits a dual-channel "guard did not run" notice. block_hook_bypass_enabled accepts only exact true/false (unset → true); any other value keeps the guard on and says so. README records the 60s hooks.json timeout fail-open and MCP-provided write tools as residuals. Fleet-wide sibling hooks and the PowerShell lane are out of scope.

Verification

• plugins/guardrails/hooks/block-hook-bypass.test.sh — PASS=564 FAIL=0 (isolated-session remedy, typo enable stays on, crash fail-open notice)
• scripts/check-changelog-parity.sh --check-bump origin/main — pass

Related

Refs #912, #3128

[claude]

Claude finished @kyle-sexton's task in 4m 21s —— View job

---

Security review — /review:security-review

Todo list

• Gather context (PR diff, changed files, CLAUDE.md)
• Invoke /review:security-review per custom instructions
• Post final findings/summary

Scope: plugins/guardrails/hooks/block-hook-bypass.sh and .test.sh (the only security-relevant surface in this PR — plugin.json, CHANGELOG.md, and README.md are docs/version only).

Findings

IMPORTANT — block-hook-bypass.sh:57-60 ships an unconditional, env-var-gated bypass in the production hook:

if [[ "${BLOCK_HOOK_BYPASS_TEST_CRASH:-}" == "1" ]]; then
  exit 99
fi

Setting BLOCK_HOOK_BYPASS_TEST_CRASH=1 in the process environment trips the new block_hook_bypass_on_exit trap, which converts exit 99 into exit 0 (allow) — i.e. it fully disables this guard and lets the exact shell file-write bypass it exists to block through. Two things elevate this beyond a nit:

1. It's reachable from ambient process environment (a repo's devcontainer.json, .envrc, CI env block, inherited shell rc file) — a broader, less-trusted surface than the block_hook_bypass_enabled CLAUDE_PLUGIN_OPTION_* knob this same PR deliberately hardens to strict true/false parsing.
2. The resulting bypass is logged as "guard did not run (internal error, rc=99)" — indistinguishable from a genuine crash, which undercuts this PR's own stated principle that "a safety switch does not silently disable."

Confirmed via grep this pattern is unique to this file — not an established convention elsewhere in plugins/guardrails/hooks/. Full detail and a suggested fix direction posted as an inline comment with a [Fix this →] deep link.

Permalink:

claude-code-plugins/plugins/guardrails/hooks/block-hook-bypass.sh

Lines 57 to 60 in bfe03ae

	# Test injection: trip the crash path without depending on a real script error.
	if [[ "${BLOCK_HOOK_BYPASS_TEST_CRASH:-}" == "1" ]]; then
	exit 99
	fi

Reviewed and clear

• hook::emit_channels / hook::json_escape (hook-utils.sh) properly escape all interpolated values (including the raw _bbh_enabled config value and crash rc) before embedding in hand-built JSON — no injection into the hook-output channel.
• The new strict block_hook_bypass_enabled parser (true/false exact match, unset → enabled) has no bypass path itself; an unrecognized value keeps the guard on and names the bad value, matching the PR's intent.
• The block_hook_bypass_on_exit EXIT trap correctly passes through the guard's real block (rc=2) and allow (rc=0) paths without interference, and doesn't conflict with any other trap in hook-utils.sh (none registered there).
• The new operator-facing systemMessage in block_bypass() is a static string with no interpolated user/attacker-controlled data.
• No changes to GitHub Actions workflows, permissions, or dependency pins in this PR.

One finding, no other security-relevant issues in the changed surface.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bfe03ae4ae

ℹ️ 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".

[github-actions]

Last security-reviewed head: bfe03ae4aebcb5e74fb5f1be30b9e29d04b7d7f6. On the next push, the relevance gate compares only the commits since this SHA; delete this comment to force a full re-review.

[claude]

Claude finished @cursor[bot]'s task in 3m 2s —— View job

---

Code review — /review:code-review

Todo list

• Gather context (PR diff, changed files, CLAUDE.md)
• Verify PR state (still open; this is the bfe03ae4…→560a516a synchronize push)
• Invoke /review:code-review per custom instructions
• Post final findings/summary

Scope: this push (bfe03ae4a…560a516a) touches plugins/guardrails/hooks/block-hook-bypass.sh, its .test.sh, README.md, and CHANGELOG.md — the commit responds directly to the two prior review findings (Codex's stderr-channel finding and the security lane's BLOCK_HOOK_BYPASS_TEST_CRASH env-var bypass). Per this lane's scoping, security posture itself stays with /review:security-review; below is a correctness/maintainability read of the fix.

Findings

None. Both prior findings are resolved correctly:

1. Env-var crash-injection bypass (BLOCK_HOOK_BYPASS_TEST_CRASH) — removed entirely from the shipped hook (block-hook-bypass.sh:54-58 diff). The test now trips the fail-open path by awk-injecting exit 99 right after trap block_hook_bypass_on_exit EXIT into a copy of the hook under $TEST_TMPDIR/crash-hook/, alongside a copied hook-utils.sh (the hook resolves its source via $(dirname "${BASH_SOURCE[0]}"), so the copy resolves correctly). A new guard assertion (block-hook-bypass.test.sh:523-525) fails the suite if the string ever reappears in the shipped script. The injected exit 99 fires before the PowerShell-only ps-command.sh sourcing branch, so the copy's CLAUDE_PLUGIN_ROOT fallback resolving one level up from crash-hook/ (which lacks lib/powershell/) is never exercised — no latent breakage there.

2. Operator guidance discarded on the exit-2 path — block_bypass() now echos the operator-lever sentence to stderr (block-hook-bypass.sh:1191-1201) in addition to keeping it on hook::emit_channels' systemMessage (confirmed hook::emit_channels only ever writes to stdout, so this isn't a duplicate stderr write). New assertions (block-hook-bypass.test.sh:1239-1249) pin the kill-switch-specific sentence ("not actionable by the blocked agent", "user-scoped", "Re-enable it") on stderr specifically — the substrings that weren't already reachable via stderr before this fix.

README/CHANGELOG updates accurately describe both changes.

Not independently verified: I could not execute block-hook-bypass.test.sh in this sandbox (Bash tool calls required approval that wasn't grantable here), so the PR's claimed PASS=564 FAIL=0 is author-claimed and unverified by me. I traced the crash-copy and stderr-assertion logic by hand instead and found it sound.

[github-actions]

Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count.