fix(permissions): deny rg --pre arbitrary subprocess exec (rig-cli#187) - #211
Open
alex-mextner wants to merge 1 commit into
Open
fix(permissions): deny rg --pre arbitrary subprocess exec (rig-cli#187)#211alex-mextner wants to merge 1 commit into
alex-mextner wants to merge 1 commit into
Conversation
The default `rg` allowlist entry pre-approves ANY invocation of ripgrep for
read-only search, but `rg --pre <CMD>` / `--pre=<CMD>` runs CMD as an
arbitrary preprocessor on every matched file — a live arbitrary-subprocess
hole behind a grant meant to be read-only.
Adds deny rules across the belts that can express flag-position matching:
- claude-code: five `Bash(rg --pre...)` forms (flag-first + later
position, space + `=` value shapes).
- opencode: three value-shaped globs (`rg*--pre *`, `rg*--pre=*`,
`rg*--pre`) — a single substring-only `rg*--pre*` entry was rejected in
review for over-denying the legitimate `--pretty` flag.
- omp: a new `rg-pre` GuardRule doing precise ARGV-level matching (catches
the flag in any position with no `--pretty` false positive, and a
joined `flag=value` token). Required broadening `subcommand_flags` to
accept a 1-token (flat command) shape alongside the existing 2-token
(subcommand) shape, and adding a new PER-FLAG `flags_with_value` opt-in
for the joined-token match — NOT a family-wide change: an earlier draft
applied the `=` check to every subcommand_flags rule, which silently
widened `git-commit-no-verify` (deliberately an anywhere-token match, so
a commit MESSAGE mentioning the flag doesn't false-positive) to also
deny a message merely starting with "--no-verify=". Caught in review by
two independent reviewers; fixed by scoping the joined-token match to
only the flags that opt in. Bumps GUARD_TEMPLATE_VERSION (TS shape
changed, not just the rules).
- pi/commandcode inherit the fix automatically — their advisory instruction
block renders from the same OMP_GUARD_DENY_RULES registry.
- codex gets a cheap partial mitigation (`("rg", "--pre")` as an
unambiguous leading-token ban) — full coverage isn't expressible in its
coarse execpolicy, the same pre-existing limit force-push/--no-verify
already have there.
- `--pre-glob` is deliberately denied NOWHERE — not the claude-code/opencode
globs, not even the omp ARGV guard. Per rg's docs it has no effect
without `--pre`, and `--pre` is caught on its own by every belt, so
denying bare `--pre-glob` anywhere is pattern bloat / a false positive
for zero security value (an earlier draft kept it in the omp guard as
"defense in depth" — review correctly called that internally
inconsistent with the glob belts' own stated reasoning).
Documents rather than silently leaves several residual gaps: the
claude-code later-position forms can false-positive on a literal search
for the text "--pre" (a DIFFERENT call than --no-verify's, made on
purpose — see the module comment for why); RIPGREP_CONFIG_PATH set
inline on the same command line is argv-visible but currently stripped
as env-assignment noise before matching (a real bypass, not yet fixed,
now regression-pinned as a known gap rather than silently assumed safe);
an already-exported config-path env var is a true blind spot; ANSI-C
quoting and tab-separated flags (`rg $'--pre' cmd`, `rg --pre<TAB>cmd`)
bypass the glob belts / omp tokenizer, pre-existing and shared by every
subcommand_flags rule, not specific to this one; the omp guard doesn't
honor `--` end-of-options either. No claude-code-specific argv hook
exists yet for rg-pre (unlike --no-verify's `block-no-verify`) — that's
an agent-tools-side follow-up, not rig-cli.
Reviewed via `review diff --staged` across six rounds (Fable, k3 — all
[ok], each round's findings fixed before the next, including one real
correctness bug caught in round 4 and one design inconsistency caught in
round 6); codex hit a usage-limit outage every round.
Fixes #187
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This was referenced Aug 6, 2026
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
1 participant
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
Fixes #187: the default
rgallowlist entry pre-approves any ripgrep invocation for read-only search, butrg --pre <CMD>runsCMDas an arbitrary preprocessor on every matched file — a live arbitrary-subprocess hole. Adds deny rules across every harness belt that can express flag-position matching (claude-code globs, opencode globs, omp ARGV guard, codex execpolicy partial mitigation), documenting rather than silently leaving the residual gaps that remain (see commit message for the full breakdown).Follow-up tickets to file (not in this PR, tracked separately)
--hostname-binand other rg exec-capable flags beyond--pre/--pre-glob(Fable, review round 6).block-rg-preargv-level agent-hook (agent-tools side), mirroringblock-no-verify, to close the quoting/tab-whitespace bypass that the glob belts alone can't catch.RIPGREP_CONFIG_PATHenv-assignment awareness in the omp guard'sstripEnvpath (currently the assignment is stripped as noise before matching, even though it's argv-visible for the inline form).GuardRulearchitecture: consider per-matcher rule classes/tagged union instead of a growing__post_init__if/elif as more matcher-specific fields accrete (Fable, explicitly non-blocking).Test plan
test_codex_update_cli_missing_updater_returns_127is flaky/environment-dependent, confirmed pre-existing and unrelated), 18 skipped, across 8 runs during development.tests/test_permissions.py,tests/test_omp_guard.py,tests/test_execpolicy.pytargeted runs green (156 tests).rg --prerule vs. the pre-existingrgallow rule) verified against the realcodex execpolicy checkCLI, not assumed — confirms the more-specific forbidden rule wins.review diff --stagedacross 8 rounds (Fable + k3, all[ok]; codex hit a usage-limit outage every round) — fixed 2 real correctness bugs found along the way (a family-wide=-match that would have widenedgit-commit-no-verify's false-positive surface; an internally-inconsistent--pre-glob"defense in depth" exception in the omp guard that the glob belts' own stated reasoning argued against).🤖 Generated with Claude Code