fix: prefix grep with -e in permission-escalation hook#12
Open
0riginal-claw wants to merge 1 commit into
Open
Conversation
The full-variant PreToolUse permission-escalation hook in
full/settings.json runs
grep -qEi "--dangerously-skip-permissions|--bypass-permissions"
against each Bash command. Because the pattern begins with `--`,
grep parses it as an option-flag list rather than a regex. On GNU
grep the leading `--dangerously-...` is treated as an unknown long
option; on BSD/macOS ugrep it errors with "invalid argument -d
ACTION". Either way the guardrail silently no-ops and lets
`claude --dangerously-skip-permissions` through.
Fix: add the `-e` separator so the pattern is parsed as a pattern.
grep -qEi -e "--dangerously-skip-permissions|--bypass-permissions"
Reproduction (a 12-test workspace install was bypassed silently):
printf "claude --dangerously-skip-permissions\n" \
| grep -qEi "--dangerously-skip-permissions|--bypass-permissions"; echo "exit=$?"
# exit=1 (GNU) or exit=2 (BSD/ugrep), never 0 - hook never fires
After fix:
printf "claude --dangerously-skip-permissions\n" \
| grep -qEi -e "--dangerously-skip-permissions|--bypass-permissions"; echo "exit=$?"
# exit=0 - hook fires correctly
Regression test: new `escalation-hook` CI scenario in tests/ci-test.sh,
mirroring the test_push_hook pattern (dangerous strings built at
runtime so the test file itself does not trip the hook). Wired into
the GitHub Actions matrix. Verified the test FAILS without the
one-line fix (4 of 7 assertions fail) and PASSES with it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The full-variant PreToolUse permission-escalation hook in
full/settings.jsonrunsgrepagainst each Bash command using a pattern that begins with two dashes. Becausegrepparses leading---arguments as option flags rather than positional patterns, the hook silently no-ops on every input rather than blocking the dangerous flags it is meant to detect.Concretely:
ifis false, so the hook does nothing.ugrep(the binary shipped as/usr/bin/grepon recent macOS) errors withinvalid argument -d ACTION, valid arguments are 'skip', 'read', 'recurse' and 'dereference-recurse'and exits 2. Same end result: theifis false, the hook does nothing.This is the only guardrail in the pack that defends against the dangerous-launch flags being passed to a re-spawned
claudesession from within a Bash tool call. With this bug present, the guardrail has never actually fired since it was introduced.Why this PR body is redacted
The literal flag strings (the two specific
--…-permissionsarguments referenced in the diff) are omitted from this PR body. The reason: this workspace currently runs the very hook this PR fixes, and as belt-and-suspenders it also runs a sibling secret/dangerous-pattern hook that matches the literal flag string in any Bash command. When the previous attempt to file this PR included the literal flag in the PR body, thegh pr createinvocation itself was blocked by that sibling hook (the body text was inside the Bash command string and matched). Reviewers can see the literal pattern in the diff offull/settings.jsonand in the test scenario; this body intentionally avoids quoting it.Fix
One-character change in
full/settings.json— add an-eseparator so the pattern is parsed as a pattern instead of an option list:(See the file diff for the exact strings — they are not reproduced here per the redaction note above.)
Reproduction
A new scenario
escalation-hookis added totests/ci-test.sh. It builds the dangerous strings at runtime from parts (e.g.DD="--"; SKIP="${DD}dangerously-…") so the test source file itself never contains the literal flag and can be edited under hook-protected sessions. Seven assertions: four "must block" cases (both dangerous flags, and the same flags after abypassalias) and three "must allow" cases (innocuousclaudeinvocations, an unrelated flag, an empty command).Manual reproduction (build the pattern from parts to avoid tripping any sibling hook in your own environment):
Tests
7 passed, 0 failed.full/settings.jsonreverted (bug present), test scenario unchanged:3 passed, 4 failed(the four "must block" cases all fail because the buggy hook returns exit-non-zero / falls through on every input).CI matrix updated:
.github/workflows/test.ymladdsescalation-hookalongside the existing scenarios.What is in the commit
full/settings.json— one-linegrep -qEi→grep -qEi -echange.tests/ci-test.sh— newtest_escalation_hookfunction + dispatch entry + help-text update..github/workflows/test.yml—escalation-hookadded to the scenario matrix.Why this matters
Every other PreToolUse hook in the pack (destructive-delete, direct-push, pipe-to-shell, data-exfiltration, scan-commit) defends against a specific dangerous command. The escalation hook is the one that defends against the means by which the model could disable all the others plus the
permissions.denylist in a single Bash call by relaunchingclaudewith the bypass flag. v0.3.8 ships this hook documented and tested in the README/SETUP — but it has been a no-op since introduction, because of the leading---quoting bug. The fix is a single character.