Fix shell write bypass of write-only denies#2
Conversation
The command/code path-token scan in `decide` only checked read-class denies (`includeWrite = false`), so a path protected by a `Write(...)`/`Edit(...)` rule with no matching `Read(...)` deny could still be modified through a shell or eval command. The embedded `Edit(~/.bashrc)` deny, for instance, did not stop a shell append to that file. Shell and code execution can both read and write, and the token scan cannot tell which, so it now checks read- and write-class denies. A read-allowed but write-denied template (`.env.example`) is conservatively blocked in shell context; the read tool still permits it directly. Also tighten a `loadPolicyEntries` test to assert the string "123" is absent, so an accidental stringification of an invalid non-string entry would be caught. Glory to the Omnissiah
|
Warning Review limit reached
Next review available in: 21 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe decide() function's shell/code path-token scan now checks both read-class and write-class deny globs instead of only read-class, closing a bypass where write denies could be circumvented via token-based path detection. Corresponding tests were added or adjusted to reflect this conservative blocking behavior. ChangesWrite-Class Deny Detection Fix
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant Decide as decide()
participant TokenScan as Path-token scan
participant Policy as Deny policy globs
Caller->>Decide: decide("bash", { command })
Decide->>TokenScan: scan command for path tokens
TokenScan->>Policy: check read-class deny globs
TokenScan->>Policy: check write-class deny globs (new)
Policy-->>TokenScan: match found (write deny)
TokenScan-->>Decide: verdict block:true
Decide-->>Caller: { block: true }
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Broaden `redactText`'s `SECRET_OUTPUT` list from a handful of shapes to cover the common credential formats a tool might echo, as defense in depth on `tool_result`. - Widen the GitHub token pattern from `ghp_` to every prefix the `gh` CLI writes (`gho_`, `ghu_`, `ghs_`, `ghr_`). - Detect AWS secret access keys, but only in context (an `aws_secret_access_key`-style label plus value), since a bare 40-char base64 string is indistinguishable from a git SHA. - Redact credentials embedded in a connection URL (`scheme://user:password@host/...`), including the host, port, path, and any query-string secret, so a leaked DSN is fully masked. - Add Stripe, GitLab, Slack app tokens and webhooks, Google API/OAuth, npm, PyPI, SendGrid, DigitalOcean, Shopify, Twilio, Discord, and JWT. Bare high-entropy strings, git SHAs, and UUIDs are deliberately left untouched to keep false positives out of normal tool output. Add positive tests for every new shape, negative tests guarding the false-positive cases, and document the full list as a bullet list in the extension `README.md`. Glory to the Omnissiah
Add the complementary regression assertion flagged by CodeRabbit to the `decide` finding-1 test: alongside verifying that shell and `eval` writes to an `Edit(...)`-only-denied path are blocked, assert that a plain read of the same path is still permitted when no matching read deny exists. This locks in the read/write class asymmetry the write-bypass fix depends on. Glory to the Omnissiah
1 participant
Summary
Closes a security gap in
rules-guardsurfaced by a CodeRabbit review: a path protected only by aWrite(...)/Edit(...)rule (no matchingRead(...)deny) could be modified through a shell orevalcommand, because the command/code path-token scan checked read-class denies only.Finding 1 (critical) — code fix
decide's token scan forSHELL_FIELDS+CODE_FIELDSpassedincludeWrite = false, sowriteDenywas never consulted for shell/code. Example: the embeddedEdit(~/.bashrc)deny did not stopecho ... >> ~/.bashrc.index.ts— the token scan now passesincludeWrite = true. Shell and code execution can both read and write and the scan can't tell which, so it now enforces read- and write-class denies. Comment rewritten to document the reasoning and the trade-off..env.example) is now conservatively blocked in shell context. The directreadtool still permits it (read-class only), so the template-allow goal is preserved; only the ambiguous shell path is tightened.index.test.ts— new regression test: shell (echo >>) andeval(open(...,'w')) writes to aWrite-only-denied path both block. The existingcat .env.exampleshell test flips totrueto match the conservative behavior.Finding 3 (minor) — stronger assertion
index.test.ts—loadPolicyEntriestest now assertsnot.toContain("123")instead ofnot.toContain(123 as unknown as string), so an accidental stringification of an invalid non-string entry would be caught.Finding 2 (non-hermetic live-POLICY test) was intentionally out of scope for this PR.
Verification
bun test— 56 pass, 0 fail, 100.00% funcs / 100.00% lines, exit 0.bun typecheck— clean.bunx biome check— clean.coderabbit review --agenton the diff — 0 findings.hkhooks pass.Summary by CodeRabbit