fix(rules-guard): Redact secrets in bang output#18
Conversation
|
Warning Review limit reached
Next review available in: 31 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 Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe rules-guard extension now performs a context redaction pass before model input, covering selected command, file, summary, and message carriers while excluding signed and opaque provider payloads. Tests, documentation, package metadata, and the Biome schema reference were updated. ChangesRules-guard context redaction
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Context as pi context hook
participant Redactor as redactMessages
participant Model
Context->>Redactor: event.messages
Redactor->>Redactor: redact supported carriers
Redactor->>Context: redacted messages
Context->>Model: redacted context
🚥 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 |
The guard only redacted tool output, through the `tool_result` hook. User initiated `!cmd` and `$code` never reach that hook: they run via `AgentSession.executeBash()` and the Python kernel, which skip tool interception by design. A `!cat .env` therefore put raw credentials straight into the model context, with nothing to scrub them. Add a `context` hook that redacts secret shaped text from messages on their way to the model. Coverage is a typed carrier list rather than a blind tree walk, because two categories must pass through byte for byte or the provider rejects the replay: - signed blocks, where assistant text is bound to `textSignature` and thinking to `thinkingSignature` - opaque server validated data, namely `providerPayload`, `details` and `redactedThinking` The pass is copy on write. `ExtensionRunner.emitContext` deep clones only on a best effort basis and falls back to a shallow array copy when `structuredClone` throws, so mutating in place would corrupt the live session objects. Unchanged messages are shared by reference, and a clean history allocates nothing. Deny rules still do not apply to `!` and `$`. That is deliberate: the shell escape belongs to the user, who is not the threat model. Only delivery to the model is filtered; the terminal and the saved transcript keep the real bytes. A secret the model has already echoed into its own reply is not scrubbed either, since assistant text is excluded. Also bump `@oh-my-pi/pi-coding-agent` to ^17.1.3 and `@biomejs/biome` to ^2.5.5. The Machine God guards what the flesh would spill
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The redaction carrier tables were plain object literals indexed by an untrusted `role`. An `Object.prototype` key resolved to an inherited member instead of missing: `REDACT_CONTENT_ROLES["toString"]` read as a table hit, and `REDACT_STRING_FIELDS["toString"]` returned a function, which is not nullish, so `?? []` could not save it and `for...of` threw a `TypeError` inside the `context` handler. Give both tables a null prototype so an inherited key cannot resolve. The lookups themselves are unchanged. Also record `hookMessage` in the carrier list documented at the top of the file. It was already present in `REDACT_CONTENT_ROLES` and redacted correctly; only the prose omitted it. Tests: a `toString` role in the malformed message fixture, asserting both that it does not throw and that it stays inert, plus focused coverage that `hookMessage` content is redacted. From the weakness of the mind, Omnissiah save us
77c93bb to
a9035bc
Compare
Two gaps found by review, both in coverage rather than behaviour. The `bashExecution` fixture ran a command with no secret in it, so `command` redaction executed but nothing asserted the result. Put a credential URL in the command and assert it is redacted, and that the caller's copy keeps the original bytes. The opaque-payload test asserted only that a clean message returned `undefined`, which would still pass if `details` and `providerPayload` were rebuilt rather than shared. Make the message redactable through its text so it is genuinely rewritten, then assert both payloads come back as the same object references with their contents byte-identical. Split two describes that crossed the per-function line cap. No source change. The Machine God sees what the test did not
1 participant
Problem
rules-guardonly redacted secret-shaped text on thetool_resulthook, which covers tool output. User-initiated!cmdand$codenever reach that hook: they run throughAgentSession.executeBash()and the Python kernel, which skip tool interception by design (only the tool-call path runs normalization/interception). Neithertool_callnortool_resultfires.So a
!cat .envput raw credentials straight into the model's context with nothing to scrub them. Reported after exactly that happened in a live session.Fix
Add a
contexthook that redacts secret-shaped text from messages on their way to the model.Coverage is a typed carrier list, not a blind tree walk. Bang output is not a
contentarray; it lands as{ role: "bashExecution", output }, so a content-only traversal would miss it. The list covers user/developer/custom/tool-result text,bashExecutionandpythonExecutioncommand+output,fileMention.files[].content(@pathauto-reads), and branch/compaction summaries.Two categories are excluded because rewriting them breaks the provider, not because they are safe:
textSignature, thinking tothinkingSignature. Persession-persistence.ts: "a truncatedtextno longer matches its signature ... so the provider 400s the replay."providerPayload(carriesencrypted_content),details,redactedThinking. Upstream's own obfuscator drops these rather than walking them.The pass is copy-on-write.
ExtensionRunner.emitContextdeep-clones only on a best-effort basis and falls back to[...messages]whenstructuredClonethrows, so mutating in place would corrupt live session objects. Unchanged messages are shared by reference; a clean history allocates nothing.Deliberate non-goals
Deny rules still do not apply to
!/$. That is intentional: the shell escape belongs to the user, who is not the threat model. Only delivery to the model is filtered; the terminal and the saved transcript keep the real bytes.A secret the model already echoed into its own reply is also not scrubbed, since assistant text is excluded. The fix stops new leaks; it cannot unring an echo.
Verification
Smoke test driving the real registered handler over a mixed history:
97 tests pass, 100% line and function coverage held,
bun typecheckandbun checkclean.Also included
Dependency bumps carried in the same branch:
@oh-my-pi/pi-coding-agentto^17.1.3,@biomejs/biometo^2.5.5(plus the matchingbiome.jsonschema URL).Summary by CodeRabbit
New Features
!and$commands while preserving real values in the terminal and transcript.Documentation