feat: add configurable secret scanner mode to fix false positives#324
Open
VinhTruong2001 wants to merge 3 commits intospacedriveapp:mainfrom
Open
feat: add configurable secret scanner mode to fix false positives#324VinhTruong2001 wants to merge 3 commits intospacedriveapp:mainfrom
VinhTruong2001 wants to merge 3 commits intospacedriveapp:mainfrom
Conversation
…acedriveapp#45) Add SecretScanMode enum (strict/own_secrets_only/disabled) to control regex-based leak detection. When set to own_secrets_only, only exact-match scrubbing of stored secrets runs — regex patterns are skipped to eliminate false positives from public API keys found in scraped web content. Configurable per-agent via [agents.sandbox] secret_scanner in TOML config. Default remains strict to preserve existing behavior.
Contributor
WalkthroughAdds a configurable SecretScanMode (Strict, OwnSecretsOnly, Disabled), wires it through AgentDeps and SandboxConfig, extends scrubbing APIs, and propagates mode-aware scrubbing and leak-detection gating across hooks, workers, channels, tools, and dispatch logic. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Keep both our scan_mode capture and upstream's process_control_registry + runtime_config additions.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/agent/cortex.rs (1)
2431-2443:⚠️ Potential issue | 🟠 Major
disabledmode is not fully honored in detached task output scrubbing.On Line 2438,
scrub_with_storeis still applied unconditionally, so Line 2443 cannot makedisabledskip all scanning/scrubbing behavior as specified by this PR’s mode contract.Suggested fix
- let scrub = |text: String| -> String { - let scrubbed = if let Some(store) = secrets_snapshot.as_ref() { - crate::secrets::scrub::scrub_with_store(&text, store) - } else { - text - }; - scan_mode.maybe_scrub_leaks(scrubbed) - }; + let scrub = |text: String| -> String { + if matches!(scan_mode, crate::secrets::scrub::SecretScanMode::Disabled) { + return text; + } + let scrubbed = if let Some(store) = secrets_snapshot.as_ref() { + crate::secrets::scrub::scrub_with_store(&text, store) + } else { + text + }; + scan_mode.maybe_scrub_leaks(scrubbed) + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/cortex.rs` around lines 2431 - 2443, The detached-task scrub closure currently always applies crate::secrets::scrub::scrub_with_store even when scan_mode should be disabled; change the closure so it first checks the scan_mode disabled state (e.g., via a method like scan_mode.is_disabled() or equivalent) and immediately returns the original text when disabled, otherwise proceed to optionally call scrub_with_store when secrets_snapshot is Some and then pass that result into scan_mode.maybe_scrub_leaks; update the closure that captures scan_mode and secrets_snapshot to enforce this short-circuit behavior so disabled truly skips all scrubbing.
🧹 Nitpick comments (4)
src/agent/channel.rs (1)
1937-1937: Consider extracting the repeated strict-mode leak gate into one helper.The same
Strict && scan_for_leaks(...)condition appears in three fallback branches. A shared helper would reduce drift and make future mode changes safer.Also applies to: 2002-2002, 2060-2060
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` at line 1937, Extract the repeated gate into a single helper like is_strict_mode_and_leak(&self, text: &str) that returns true when self.deps.secret_scan_mode() == crate::secrets::scrub::SecretScanMode::Strict && crate::secrets::scrub::scan_for_leaks(text). Replace the three occurrences (current line with self.deps.secret_scan_mode() == ... && let Some(leak) = crate::secrets::scrub::scan_for_leaks(text) and the blocks at the other two locations) with that helper and, if needed, have the helper return the detected leak (Option<_>) or a boolean depending on how the callers use the leak value so callers can act on the result without duplicating the condition.src/agent/channel_dispatch.rs (3)
508-513: Snapshotsecret_scan_modeonce per worker spawn.
state.deps.secret_scan_mode()is read multiple times in one spawn flow. If config reloads in between, worker internals and completion scrubbing can run with different modes for the same worker.Based on learnings: "For changes in async/stateful paths (worker lifecycle, cancellation, retrigger, recall cache behavior), include explicit race/terminal-state reasoning in the PR summary and run targeted tests in addition to `just gate-pr`."Suggested patch
+ let scan_mode = state.deps.secret_scan_mode(); @@ - worker.with_secret_scan_mode(state.deps.secret_scan_mode()) + worker.with_secret_scan_mode(scan_mode) @@ - worker.with_secret_scan_mode(state.deps.secret_scan_mode()) + worker.with_secret_scan_mode(scan_mode) @@ - state.deps.secret_scan_mode(), + scan_mode,Also applies to: 522-527, 538-545
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel_dispatch.rs` around lines 508 - 513, Snapshot secret_scan_mode once at worker spawn: read state.deps.secret_scan_mode() into a local let secret_scan_mode variable before modifying/returning worker so the same value is passed to worker.with_secret_scan_mode(...) and reused wherever the mode is needed (e.g., building internals and completion-scrub paths that currently call state.deps.secret_scan_mode() again). Update usages around oc_secrets_store handling and the other spots noted (the blocks using worker.with_secret_scan_mode and any subsequent calls that re-query state.deps.secret_scan_mode()) to use the captured secret_scan_mode variable so a single consistent mode is applied for the entire worker lifecycle.
609-657: Add mode-specific tests for the new worker completion helper.
spawn_worker_task_with_scan_modechanged core completion sanitization logic, but tests currently cover only cancellation classification. Please add strict/own_secrets_only/disabled assertions for both success and failure message paths.Based on learnings: "For changes in async/stateful paths (worker lifecycle, cancellation, retrigger, recall cache behavior), include explicit race/terminal-state reasoning in the PR summary and run targeted tests in addition to
just gate-pr."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel_dispatch.rs` around lines 609 - 657, Add unit tests exercising spawn_worker_task_with_scan_mode for each SecretScanMode variant (Strict, OwnSecretsOnly, Disabled) covering both successful futures (Future -> Ok(text)) and failing futures (Future -> Err(spacebot Error -> WorkerCompletionError::Failed message)); assert that returned success texts and failure messages are scrubbed according to the mode (exact-store redaction via secrets_store + regex-based scan_mode.maybe_scrub_leaks behavior). Use the same helper types referenced in the diff (spawn_worker_task_with_scan_mode, SecretScanMode, WorkerCompletionError, secrets::scrub::scrub_with_store) to locate code paths, simulate a secrets_store with known secrets, and keep existing cancellation tests unchanged while adding explicit assertions for each mode on both success and failure paths. Ensure tests run under tokio runtime and include strict/own_secrets_only/disabled expectations for both outcomes.
271-276: Scrub policy is split and can drift across modes/paths.This file always applies
scrub_with_store(...)before mode-gated regex scrubbing. With mode logic also handled differently in other paths, behavior can diverge by event type. Consider centralizing exact-match + regex decisions behind one mode-aware helper and using it everywhere.Also applies to: 643-657
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel_dispatch.rs` around lines 271 - 276, The scrub logic currently applies exact-store scrubbing (crate::secrets::scrub::scrub_with_store) before mode-gated regex scrubbing (scan_mode.maybe_scrub_leaks), which can diverge from other callers; extract and centralize the exact-match + regex decisions into a single mode-aware helper (e.g., in crate::secrets::scrub) that accepts (raw, Option<store>, scan_mode) and internally decides whether to call scrub_with_store and/or maybe_scrub_leaks and in which order, then replace the current inline sequence in channel_dispatch.rs (the code using secrets_snapshot, scrub_with_store, and scan_mode.maybe_scrub_leaks) and the other duplicated path (around lines 643-657) to call that helper so all event types share identical scrubbing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/hooks/spacebot.rs`:
- Around line 575-580: The current branch treats any non-Strict mode as
passthrough so OwnSecretsOnly skips redaction for ProcessEvent::ToolCompleted;
update the logic around self.secret_scan_mode (used in the ToolCompleted
handling) to explicitly handle SecretScanMode::OwnSecretsOnly: when
OwnSecretsOnly is set call a function that performs exact-match-only redaction
(e.g., a scrub_exact_matches-style helper in crate::secrets::scrub) on result,
keep Strict using scrub_leaks(result), and only return result.to_string() for
modes that truly mean no redaction; reference secret_scan_mode,
SecretScanMode::OwnSecretsOnly, SecretScanMode::Strict, and the ToolCompleted
path to locate where to add the new branch or helper and implement exact-match
redaction.
In `@src/secrets/scrub.rs`:
- Around line 44-53: The SecretScanMode currently only gates regex redaction via
maybe_scrub_leaks, leaving exact-match scrubbing to callers which causes
Disabled vs OwnSecretsOnly semantics to drift; update SecretScanMode to
centralize end-to-end semantics by adding/renaming a method (e.g.,
apply_scrubbing or maybe_scrub_all) that takes the original text and the
exact-match scrubber (or calls the existing exact-match logic) and returns the
final text such that: Strict performs regex (scrub_leaks) then exact-match
scrubbing, OwnSecretsOnly performs only exact-match scrubbing, and Disabled
returns the input unchanged; replace callers of maybe_scrub_leaks to use this
new centralized helper (referencing SecretScanMode::maybe_scrub_leaks,
scrub_leaks, and the new apply_scrubbing method) so mode behavior is enforced
consistently.
In `@src/tools/reply.rs`:
- Around line 389-400: The reply path only runs regex leak detection when
SecretScanMode::Strict via crate::secrets::scrub::scan_for_leaks, so exact-match
redaction for own_secrets_only is missing and replies can leak protected
secrets; update the reply flow in reply.rs to perform an exact-match secret
redaction step (use the same secret set used elsewhere) before returning
user-visible content in all non-disabled modes, and ensure when ReplyTool is
constructed in src/tools.rs it is passed the secrets via with_tool_secrets(...)
so ReplyTool has the own-secret set available; specifically add a pre-send
exact-match check/redact using the tool’s secret set (in addition to existing
scan_for_leaks branch) and wire with_tool_secrets into ReplyTool construction.
---
Outside diff comments:
In `@src/agent/cortex.rs`:
- Around line 2431-2443: The detached-task scrub closure currently always
applies crate::secrets::scrub::scrub_with_store even when scan_mode should be
disabled; change the closure so it first checks the scan_mode disabled state
(e.g., via a method like scan_mode.is_disabled() or equivalent) and immediately
returns the original text when disabled, otherwise proceed to optionally call
scrub_with_store when secrets_snapshot is Some and then pass that result into
scan_mode.maybe_scrub_leaks; update the closure that captures scan_mode and
secrets_snapshot to enforce this short-circuit behavior so disabled truly skips
all scrubbing.
---
Nitpick comments:
In `@src/agent/channel_dispatch.rs`:
- Around line 508-513: Snapshot secret_scan_mode once at worker spawn: read
state.deps.secret_scan_mode() into a local let secret_scan_mode variable before
modifying/returning worker so the same value is passed to
worker.with_secret_scan_mode(...) and reused wherever the mode is needed (e.g.,
building internals and completion-scrub paths that currently call
state.deps.secret_scan_mode() again). Update usages around oc_secrets_store
handling and the other spots noted (the blocks using
worker.with_secret_scan_mode and any subsequent calls that re-query
state.deps.secret_scan_mode()) to use the captured secret_scan_mode variable so
a single consistent mode is applied for the entire worker lifecycle.
- Around line 609-657: Add unit tests exercising
spawn_worker_task_with_scan_mode for each SecretScanMode variant (Strict,
OwnSecretsOnly, Disabled) covering both successful futures (Future -> Ok(text))
and failing futures (Future -> Err(spacebot Error ->
WorkerCompletionError::Failed message)); assert that returned success texts and
failure messages are scrubbed according to the mode (exact-store redaction via
secrets_store + regex-based scan_mode.maybe_scrub_leaks behavior). Use the same
helper types referenced in the diff (spawn_worker_task_with_scan_mode,
SecretScanMode, WorkerCompletionError, secrets::scrub::scrub_with_store) to
locate code paths, simulate a secrets_store with known secrets, and keep
existing cancellation tests unchanged while adding explicit assertions for each
mode on both success and failure paths. Ensure tests run under tokio runtime and
include strict/own_secrets_only/disabled expectations for both outcomes.
- Around line 271-276: The scrub logic currently applies exact-store scrubbing
(crate::secrets::scrub::scrub_with_store) before mode-gated regex scrubbing
(scan_mode.maybe_scrub_leaks), which can diverge from other callers; extract and
centralize the exact-match + regex decisions into a single mode-aware helper
(e.g., in crate::secrets::scrub) that accepts (raw, Option<store>, scan_mode)
and internally decides whether to call scrub_with_store and/or maybe_scrub_leaks
and in which order, then replace the current inline sequence in
channel_dispatch.rs (the code using secrets_snapshot, scrub_with_store, and
scan_mode.maybe_scrub_leaks) and the other duplicated path (around lines
643-657) to call that helper so all event types share identical scrubbing
behavior.
In `@src/agent/channel.rs`:
- Line 1937: Extract the repeated gate into a single helper like
is_strict_mode_and_leak(&self, text: &str) that returns true when
self.deps.secret_scan_mode() == crate::secrets::scrub::SecretScanMode::Strict &&
crate::secrets::scrub::scan_for_leaks(text). Replace the three occurrences
(current line with self.deps.secret_scan_mode() == ... && let Some(leak) =
crate::secrets::scrub::scan_for_leaks(text) and the blocks at the other two
locations) with that helper and, if needed, have the helper return the detected
leak (Option<_>) or a boolean depending on how the callers use the leak value so
callers can act on the result without duplicating the condition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0c9f3a80-aa28-4f83-8abf-5b38db2cf471
📒 Files selected for processing (16)
src/agent/branch.rssrc/agent/channel.rssrc/agent/channel_dispatch.rssrc/agent/compactor.rssrc/agent/cortex.rssrc/agent/cortex_chat.rssrc/agent/ingestion.rssrc/agent/worker.rssrc/hooks/spacebot.rssrc/lib.rssrc/opencode/worker.rssrc/sandbox.rssrc/secrets/scrub.rssrc/tools.rssrc/tools/reply.rssrc/tools/set_status.rs
Comment on lines
+44
to
+53
| impl SecretScanMode { | ||
| /// Apply regex-based leak scrubbing only when in `Strict` mode. | ||
| /// Returns the scrubbed text in Strict mode, or the input unchanged otherwise. | ||
| pub fn maybe_scrub_leaks(&self, text: String) -> String { | ||
| match self { | ||
| Self::Strict => scrub_leaks(&text), | ||
| Self::OwnSecretsOnly | Self::Disabled => text, | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
SecretScanMode API does not enforce a distinct disabled behavior end-to-end.
maybe_scrub_leaks gates only regex redaction, and both non-strict modes return the input unchanged. Since exact-match scrubbing is done externally, disabled vs own_secrets_only semantics are left to each caller and can drift from the intended contract.
🧩 Suggested refactor to centralize mode semantics
impl SecretScanMode {
+ pub fn scrub_text_with_pairs(self, text: &str, tool_secrets: &[(String, String)]) -> String {
+ let layer1 = match self {
+ Self::Strict | Self::OwnSecretsOnly => scrub_secrets(text, tool_secrets),
+ Self::Disabled => text.to_string(),
+ };
+ match self {
+ Self::Strict => scrub_leaks(&layer1),
+ Self::OwnSecretsOnly | Self::Disabled => layer1,
+ }
+ }
+
pub fn maybe_scrub_leaks(&self, text: String) -> String {
match self {
Self::Strict => scrub_leaks(&text),
Self::OwnSecretsOnly | Self::Disabled => text,
}
}
}Then have egress call sites use the centralized helper instead of manually composing layer behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/secrets/scrub.rs` around lines 44 - 53, The SecretScanMode currently only
gates regex redaction via maybe_scrub_leaks, leaving exact-match scrubbing to
callers which causes Disabled vs OwnSecretsOnly semantics to drift; update
SecretScanMode to centralize end-to-end semantics by adding/renaming a method
(e.g., apply_scrubbing or maybe_scrub_all) that takes the original text and the
exact-match scrubber (or calls the existing exact-match logic) and returns the
final text such that: Strict performs regex (scrub_leaks) then exact-match
scrubbing, OwnSecretsOnly performs only exact-match scrubbing, and Disabled
returns the input unchanged; replace callers of maybe_scrub_leaks to use this
new centralized helper (referencing SecretScanMode::maybe_scrub_leaks,
scrub_leaks, and the new apply_scrubbing method) so mode behavior is enforced
consistently.
Comment on lines
+389
to
400
| if self.secret_scan_mode == crate::secrets::scrub::SecretScanMode::Strict { | ||
| if let Some(leak) = crate::secrets::scrub::scan_for_leaks(&converted_content) { | ||
| tracing::error!( | ||
| conversation_id = %self.conversation_id, | ||
| leak_prefix = %&leak[..leak.len().min(8)], | ||
| "reply tool blocked content matching secret pattern" | ||
| ); | ||
| return Err(ReplyError( | ||
| "blocked reply content: potential secret detected".into(), | ||
| )); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
own_secrets_only mode currently drops own-secret protection in reply output.
This block only performs regex leak detection in Strict, and ReplyTool still has no exact-match secret redaction step before sending user-visible content. That makes own_secrets_only behave like disabled for replies, which breaks the intended mode contract.
🔧 Suggested direction
pub struct ReplyTool {
response_tx: mpsc::Sender<OutboundResponse>,
@@
agent_display_name: String,
+ tool_secret_pairs: Vec<(String, String)>,
secret_scan_mode: crate::secrets::scrub::SecretScanMode,
}
impl ReplyTool {
@@
Self {
@@
+ tool_secret_pairs: Vec::new(),
secret_scan_mode: crate::secrets::scrub::SecretScanMode::default(),
}
}
+ pub fn with_tool_secrets(mut self, pairs: Vec<(String, String)>) -> Self {
+ self.tool_secret_pairs = pairs;
+ self
+ }
@@
- let converted_content = convert_mentions(...).await;
+ let converted_content = convert_mentions(...).await;
+ let converted_content = match self.secret_scan_mode {
+ crate::secrets::scrub::SecretScanMode::Strict
+ | crate::secrets::scrub::SecretScanMode::OwnSecretsOnly => {
+ crate::secrets::scrub::scrub_secrets(&converted_content, &self.tool_secret_pairs)
+ }
+ crate::secrets::scrub::SecretScanMode::Disabled => converted_content,
+ };
if self.secret_scan_mode == crate::secrets::scrub::SecretScanMode::Strict {
if let Some(leak) = crate::secrets::scrub::scan_for_leaks(&converted_content) {Also wire with_tool_secrets(...) where ReplyTool is constructed in src/tools.rs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tools/reply.rs` around lines 389 - 400, The reply path only runs regex
leak detection when SecretScanMode::Strict via
crate::secrets::scrub::scan_for_leaks, so exact-match redaction for
own_secrets_only is missing and replies can leak protected secrets; update the
reply flow in reply.rs to perform an exact-match secret redaction step (use the
same secret set used elsewhere) before returning user-visible content in all
non-disabled modes, and ensure when ReplyTool is constructed in src/tools.rs it
is passed the secrets via with_tool_secrets(...) so ReplyTool has the own-secret
set available; specifically add a pre-send exact-match check/redact using the
tool’s secret set (in addition to existing scan_for_leaks branch) and wire
with_tool_secrets into ReplyTool construction.
Resolves CodeRabbit review findings on PR spacedriveapp#324. The core issue was that SecretScanMode only gated regex-based scanning (layer 2), while exact-match scrubbing (layer 1) was handled ad-hoc by callers — causing OwnSecretsOnly to behave like Disabled in several egress paths. - Add apply_scrubbing_with_store() and apply_scrubbing_with_pairs() to SecretScanMode for centralized layer 1 + layer 2 enforcement - Add secrets_snapshot to SpacebotHook for ToolCompleted exact-match scrubbing on event payloads - Add tool_secret_pairs to ReplyTool for pre-send exact-match redaction - Fix cortex.rs detached task scrub to fully honor Disabled mode - Snapshot secret_scan_mode once per OpenCode worker spawn - Extract strict_mode_leak() helper in channel.rs (DRY, 3 call sites) - Migrate all scrub call sites to centralized methods - Add tests for all three modes on apply_scrubbing_with_pairs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/agent/channel_dispatch.rs (1)
650-659:⚠️ Potential issue | 🟠 MajorScrub panic payloads before emitting worker failure text.
The panic branch bypasses
scan_mode.apply_scrubbing_with_store, so panic messages can leak secrets throughWorkerComplete.result.🔧 Suggested fix
Err(panic_payload) => { let panic_message = crate::agent::panic_payload_to_string(&*panic_payload); + let store_ref = secrets_store.as_ref().map(|s| s.as_ref()); + let scrubbed_panic_message = + scan_mode.apply_scrubbing_with_store(&panic_message, store_ref); tracing::error!( worker_id = %worker_id, - panic_message = %panic_message, + panic_message = %scrubbed_panic_message, "worker task panicked" ); Err(WorkerCompletionError::failed(format!( - "worker task panicked: {panic_message}" + "worker task panicked: {scrubbed_panic_message}" ))) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel_dispatch.rs` around lines 650 - 659, The panic branch currently converts the payload with panic_payload_to_string and logs/returns it directly, which can leak secrets; instead pass the panic_message through scan_mode.apply_scrubbing_with_store (using the same store used elsewhere in this scope) to produce a scrubbed_message, then use that scrubbed_message in the tracing::error payload and in the WorkerCompletionError::failed text so no raw panic payload is emitted; update the branch around panic_payload_to_string, tracing::error, and WorkerCompletionError::failed to use the scrubbed result.src/tools/set_status.rs (1)
103-115:⚠️ Potential issue | 🟠 MajorScrub before truncation to avoid partial secret leakage.
Right now, truncation happens first. If a secret is cut at the boundary, exact-match redaction won’t fire and a secret fragment can be emitted.
🔐 Suggested fix
- let status = if args.status.len() > 256 { - let end = args.status.floor_char_boundary(256); - let boundary = args.status[..end].rfind(char::is_whitespace).unwrap_or(end); - format!("{}...", &args.status[..boundary]) - } else { - args.status - }; - - // Apply centralized scrubbing: exact-match (layer 1) + regex (layer 2) per mode. - let status = self + // Scrub first so truncation cannot split a secret and bypass exact-match redaction. + let scrubbed_status = self .secret_scan_mode - .apply_scrubbing_with_pairs(&status, &self.tool_secret_pairs); + .apply_scrubbing_with_pairs(&args.status, &self.tool_secret_pairs); + + let status = if scrubbed_status.len() > 256 { + let end = scrubbed_status.floor_char_boundary(256); + let boundary = scrubbed_status[..end] + .rfind(char::is_whitespace) + .unwrap_or(end); + format!("{}...", &scrubbed_status[..boundary]) + } else { + scrubbed_status + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/set_status.rs` around lines 103 - 115, The code currently truncates args.status before scrubbing which can leave partial secrets; instead call self.secret_scan_mode.apply_scrubbing_with_pairs on the full args.status (using &args.status and &self.tool_secret_pairs) to produce a scrubbed string, then apply the existing truncation logic (floor_char_boundary, rfind(char::is_whitespace), adding "..." when truncated) to that scrubbed result and assign to status; update references to use the scrubbed full string when computing end and boundary so no secret fragments survive truncation.
♻️ Duplicate comments (1)
src/hooks/spacebot.rs (1)
587-590:⚠️ Potential issue | 🟠 MajorPrevent fail-open redaction when
OwnSecretsOnlyhas no secrets snapshot.This call becomes passthrough in
OwnSecretsOnlyifself.secrets_snapshotisNone. A provided cross-file snippet (src/agent/ingestion.rs:495-502) shows a hook construction path withoutwith_secrets_snapshot(...), so tool output can be emitted unsanitized.🛡️ Suggested hardening in this path
- let scrubbed = self.secret_scan_mode.apply_scrubbing_with_store( - result, - self.secrets_snapshot.as_ref().map(|arc| arc.as_ref()), - ); + let store_ref = self.secrets_snapshot.as_ref().map(|arc| arc.as_ref()); + let scrubbed = match (self.secret_scan_mode, store_ref) { + (crate::secrets::scrub::SecretScanMode::OwnSecretsOnly, None) => { + tracing::warn!( + process_id = %self.process_id, + "OwnSecretsOnly enabled without secrets snapshot; applying regex-only fallback scrub" + ); + crate::secrets::scrub::scrub_leaks(result) + } + _ => self.secret_scan_mode.apply_scrubbing_with_store(result, store_ref), + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/spacebot.rs` around lines 587 - 590, The call to self.secret_scan_mode.apply_scrubbing_with_store(...) can become a passthrough when self.secrets_snapshot is None and the mode is OwnSecretsOnly; detect when self.secret_scan_mode is OwnSecretsOnly and self.secrets_snapshot.is_none() and instead pass a safe empty snapshot (or otherwise force full redaction) into apply_scrubbing_with_store so tool output cannot be emitted unsanitized. Locate the use of secret_scan_mode.apply_scrubbing_with_store, inspect self.secrets_snapshot and the SecretScanMode::OwnSecretsOnly variant, and if missing create/derive an empty snapshot (or a safe stub) and pass that to apply_scrubbing_with_store rather than None.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/secrets/scrub.rs`:
- Around line 39-41: Update the documentation for the Disabled enum variant to
reflect the actual behavior: it performs no scrubbing and returns the input
unchanged (remove the note about "Exact-match scrubbing of stored secrets (Layer
1) still runs"). Locate the enum variant Disabled in the scrub.rs declaration
and change its doc comment to state that Disabled disables all scrubbing,
matching the existing implementation paths that return the original input
unchanged.
---
Outside diff comments:
In `@src/agent/channel_dispatch.rs`:
- Around line 650-659: The panic branch currently converts the payload with
panic_payload_to_string and logs/returns it directly, which can leak secrets;
instead pass the panic_message through scan_mode.apply_scrubbing_with_store
(using the same store used elsewhere in this scope) to produce a
scrubbed_message, then use that scrubbed_message in the tracing::error payload
and in the WorkerCompletionError::failed text so no raw panic payload is
emitted; update the branch around panic_payload_to_string, tracing::error, and
WorkerCompletionError::failed to use the scrubbed result.
In `@src/tools/set_status.rs`:
- Around line 103-115: The code currently truncates args.status before scrubbing
which can leave partial secrets; instead call
self.secret_scan_mode.apply_scrubbing_with_pairs on the full args.status (using
&args.status and &self.tool_secret_pairs) to produce a scrubbed string, then
apply the existing truncation logic (floor_char_boundary,
rfind(char::is_whitespace), adding "..." when truncated) to that scrubbed result
and assign to status; update references to use the scrubbed full string when
computing end and boundary so no secret fragments survive truncation.
---
Duplicate comments:
In `@src/hooks/spacebot.rs`:
- Around line 587-590: The call to
self.secret_scan_mode.apply_scrubbing_with_store(...) can become a passthrough
when self.secrets_snapshot is None and the mode is OwnSecretsOnly; detect when
self.secret_scan_mode is OwnSecretsOnly and self.secrets_snapshot.is_none() and
instead pass a safe empty snapshot (or otherwise force full redaction) into
apply_scrubbing_with_store so tool output cannot be emitted unsanitized. Locate
the use of secret_scan_mode.apply_scrubbing_with_store, inspect
self.secrets_snapshot and the SecretScanMode::OwnSecretsOnly variant, and if
missing create/derive an empty snapshot (or a safe stub) and pass that to
apply_scrubbing_with_store rather than None.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 942206a2-813e-4716-847a-b72109a4da4e
📒 Files selected for processing (10)
src/agent/branch.rssrc/agent/channel.rssrc/agent/channel_dispatch.rssrc/agent/cortex.rssrc/agent/worker.rssrc/hooks/spacebot.rssrc/secrets/scrub.rssrc/tools.rssrc/tools/reply.rssrc/tools/set_status.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- src/tools.rs
- src/agent/worker.rs
- src/agent/branch.rs
- src/agent/channel.rs
Comment on lines
+39
to
+41
| /// No regex-based leak detection. Exact-match scrubbing of stored | ||
| /// secrets (Layer 1) still runs. Use with caution. | ||
| Disabled, |
Contributor
There was a problem hiding this comment.
Correct Disabled mode docs to match implementation.
The doc says Disabled still runs layer-1 exact-match scrubbing, but Line 66 and Line 95 return input unchanged. This mismatch can cause misconfiguration expectations.
📝 Suggested doc fix
- /// No regex-based leak detection. Exact-match scrubbing of stored
- /// secrets (Layer 1) still runs. Use with caution.
+ /// No scrubbing or leak detection. Returns input unchanged.
+ /// Use with caution.
Disabled,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/secrets/scrub.rs` around lines 39 - 41, Update the documentation for the
Disabled enum variant to reflect the actual behavior: it performs no scrubbing
and returns the input unchanged (remove the note about "Exact-match scrubbing of
stored secrets (Layer 1) still runs"). Locate the enum variant Disabled in the
scrub.rs declaration and change its doc comment to state that Disabled disables
all scrubbing, matching the existing implementation paths that return the
original input unchanged.
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 #45 — Secret scanner erroneously flagging intentionally-public keys (e.g. Algolia search keys embedded in scraped pages) as credentials, killing the worker before returning results.
Adds a configurable
SecretScanModewith three modes:strict(default)own_secrets_onlydisabledown_secrets_onlyis the recommended mode for agents that scrape public websites — it still redacts stored secrets but stops flagging third-party API keys found in scraped content.Configuration
Per-agent via
sandbox.toml:Changes
src/secrets/scrub.rs—SecretScanModeenum +maybe_scrub_leaks()helpersrc/sandbox.rs—secret_scannerfield onSandboxConfigsrc/lib.rs—secret_scan_mode()accessor onAgentDepssrc/hooks/spacebot.rs— Mode-awarescan_for_leaks()+ 3 testssrc/tools/reply.rs,src/tools/set_status.rs— Respect scan mode on output scrubbingsrc/opencode/worker.rs— Mode-aware leak scanningsrc/agent/{channel,channel_dispatch,branch,cortex,worker,compactor,cortex_chat,ingestion}.rs— Thread scan mode through agent pipeline16 files changed, 268 insertions, 56 deletions.
Test Plan
test_secret_scan_mode_deserialization— TOML deserializes all 3 variantstest_scan_for_leaks_strict_mode— Regex scanning fires in Stricttest_scan_for_leaks_own_secrets_only_mode— Regex scanning skipped in OwnSecretsOnlytest_scan_for_leaks_disabled_mode— Scanning skipped in Disabledcargo test --lib+cargo clippy