feat: add secret detection for AI input with confirmation gate#196
feat: add secret detection for AI input with confirmation gate#196jexShain wants to merge 1 commit into
Conversation
|
Thanks for the pull request. A maintainer will review it when available. Please keep the PR focused, explain the why in the description, and make sure local checks pass before requesting review. Contribution guide: https://github.com/AI-Shell-Team/aish/blob/main/CONTRIBUTING.md |
|
This pull request description looks incomplete. Please update the missing sections below before review. Missing items:
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds regex-based secret detection (builtin + custom patterns), integrates a SecretScanner into SecurityManager and policy loading, gates AI inputs with an interactive terminal confirmation prompt, wires the check into PTY and REPL flows (with redaction/restore via an in-memory vault), and supplies localization and policy documentation. ChangesSecret Detection and Interactive Confirmation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 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 |
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/aish-pty/src/persistent.rs`:
- Around line 2374-2378: In the match arm that handles 0x03 (the unsafe block
calling libc::write), fix the off-by-one by passing the correct byte count for
the buffer (use the actual length of b"^C\r\n" instead of 5); locate the unsafe
block in persistent.rs where libc::write is called and replace the hardcoded 5
with the correct length (e.g., use b"^C\r\n".len() or the literal 4) so the
write does not read past the buffer.
- Around line 1248-1254: The code block around the secret detection gate
(references: secret_check, checker, question, show_secret_confirmation,
libc::STDIN_FILENO) is misformatted and failing `cargo fmt --check`; run `cargo
fmt` (or apply rustfmt) to reformat the block so spacing/indentation matches
project style—specifically ensure the nested if-let and function call arguments
are properly aligned and indented as rustfmt would produce (preserve the logic:
if let Some(ref checker) = secret_check { if let Some(warning) =
checker(&question) { let confirmed = show_secret_confirmation(&warning,
libc::STDIN_FILENO); if !confirmed { ... } } }).
In `@crates/aish-security/src/policy.rs`:
- Around line 454-460: The current construction of secret_patterns silently
drops parse errors because seq.iter().filter_map(|v|
serde_yaml::from_value(v.clone()).ok()) ignores errors; update this to collect
both successful CustomPattern values and parse failures and emit diagnostics
instead of discarding them: replace the filter_map(.ok()) usage in the
secret_patterns assembly (the let secret_patterns:
Vec<crate::secret::CustomPattern> block that uses mapping_get and
Value::as_sequence) with logic that attempts serde_yaml::from_value for each
entry, pushes successful parses into secret_patterns, and for failures pushes a
warning/entry into the module’s validation_issues (or calls the existing
diagnostics emitter) including the original value and the parse error message so
malformed patterns are reported.
In `@crates/aish-security/src/secret/patterns.rs`:
- Around line 37-39: The file fails rustfmt causing CI errors; run rustfmt/cargo
fmt on the crate to fix formatting mismatches around the Pattern struct methods
(e.g., format_reason) and other affected blocks (lines around the methods
starting at format_reason, the block at 58-74, and the block at 104-105).
Reformat the file by running cargo fmt in the workspace/crate so the code
conforms to rustfmt and the CI will pass.
In `@crates/aish-security/src/secret/scanner.rs`:
- Line 163: The test currently hardcodes a JWT-looking literal in the string
assigned to input, which trips secret scanners; change the test to build the
token at runtime instead: construct the token by joining safe parts (e.g.,
base64-encoding small header and payload strings and a benign signature or
concatenating static safe substrings) and then assemble the overall input string
(the variable named input) so the value still matches expected format but no
secret-like literal appears in the source; update the test that uses input (look
for the test function referencing input in secret::scanner.rs) to use this
dynamically built token instead.
In `@crates/aish-shell/src/app.rs`:
- Around line 999-1001: The file has formatting issues: reflow the
secret_check_closure assignment (the call to Self::build_secret_check_closure
and its argument security_manager.secret_scanner().clone()), the
detected_secrets mapping (and its reasons formatting) around the
detected_secrets variable, and the wrapping of the argument to
pty.send_command_interactive so they conform to rustfmt style; run cargo fmt and
commit the resulting changes to fix all formatting mismatches.
In `@crates/aish-shell/src/tui.rs`:
- Around line 79-83: The function show_confirmation_dialog has formatting issues
and redundant .to_string() calls; update the signature/spacing to match rustfmt
(run cargo fmt) and remove the unnecessary conversions by passing yes_label and
no_label directly into DialogOption::new (which accepts impl Into<String>)
instead of yes_label.to_string() and no_label.to_string(); ensure the Vec
options uses DialogOption::new("yes", yes_label) and DialogOption::new("no",
no_label) so formatting and types satisfy cargo fmt and the compiler.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e760bce0-2570-47d1-ace8-0221ea17ec48
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
config/security_policy.yamlcrates/aish-i18n/locales/en-US.yamlcrates/aish-i18n/locales/zh-CN.yamlcrates/aish-pty/src/persistent.rscrates/aish-security/Cargo.tomlcrates/aish-security/src/decision.rscrates/aish-security/src/lib.rscrates/aish-security/src/manager.rscrates/aish-security/src/policy.rscrates/aish-security/src/sandbox/degraded.rscrates/aish-security/src/secret/mod.rscrates/aish-security/src/secret/patterns.rscrates/aish-security/src/secret/scanner.rscrates/aish-shell/src/app.rscrates/aish-shell/src/tui.rs
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/aish-security/src/secret/scanner.rs (1)
157-157:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid committing JWT-looking literal in tests.
This test embeds a JWT-shaped string that can trigger secret scanners. Building the token dynamically keeps coverage without false-positive alerts.
🔧 Suggested dynamic token construction
- let input = "key=sk-abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuv token=eyJhbGciOiJIUzI1NiJ9.eyJzdWIiOiIxMjM0NTY3ODkwIn0.dozjgNryP4J3jVmNHl0w5N_XgL0n3I9PlFUP0THsR8U"; + let jwt = format!("eyJ{}.eyJ{}.doz{}", "hbGciOiJIUzI1NiJ9", "zdWIiOiIxMjM0NTY3ODkwIn0", "jgNryP4J3jVmNHl0w5N_XgL0n3I9PlFUP0THsR8U"); + let input = format!("key=sk-abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuv token={}", jwt);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aish-security/src/secret/scanner.rs` at line 157, The test currently assigns a JWT-looking literal to the variable `input` which triggers secret scanners; modify the test in `crates/aish-security/src/secret/scanner.rs` to construct the token dynamically (e.g., build header, payload, and signature parts at runtime using benign strings and base64/url-safe encoding or simple concatenation) and then set `input` to "key=sk-... token=" + constructed_token so the test coverage remains but no hardcoded JWT-like literal is committed; locate the assignment to `input` in the test and replace it with the runtime construction logic using the same variable name.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@crates/aish-security/src/secret/scanner.rs`:
- Line 157: The test currently assigns a JWT-looking literal to the variable
`input` which triggers secret scanners; modify the test in
`crates/aish-security/src/secret/scanner.rs` to construct the token dynamically
(e.g., build header, payload, and signature parts at runtime using benign
strings and base64/url-safe encoding or simple concatenation) and then set
`input` to "key=sk-... token=" + constructed_token so the test coverage remains
but no hardcoded JWT-like literal is committed; locate the assignment to `input`
in the test and replace it with the runtime construction logic using the same
variable name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e97a252c-8e99-486c-b94a-65e511541d56
📒 Files selected for processing (4)
crates/aish-pty/src/persistent.rscrates/aish-security/src/secret/scanner.rscrates/aish-shell/src/app.rscrates/aish-shell/src/tui.rs
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/aish-security/src/secret/patterns.rs (1)
13-17:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDerive
DefaultforSecretTypeto resolveclippy::derivable-impls.
crates/aish-security/src/secret/patterns.rsdefines a manualimpl Default for SecretTypereturningSecretType::ApiKeywhile the enum has noDefaultderive; this is the patternclippy::derivable-implsflags (and the repo runs clippy with-D warnings). Replace the manual impl with#[derive(Default)]and mark the default variant.Proposed fix
-#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, Default)] #[serde(rename_all = "snake_case")] pub enum SecretType { + #[default] ApiKey, Token, Password, Credential, } - -impl Default for SecretType { - fn default() -> Self { - Self::ApiKey - } -}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aish-security/src/secret/patterns.rs` around lines 13 - 17, The manual impl Default for SecretType (fn default() -> Self { Self::ApiKey }) should be replaced by deriving Default on the enum: add #[derive(Default)] to the SecretType declaration, annotate the ApiKey variant with #[default], and remove the existing impl Default block so clippy's clippy::derivable-impls warning is resolved; locate the SecretType enum and the impl Default in patterns.rs to make these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/aish-pty/src/persistent.rs`:
- Around line 913-935: The restore logic for secret placeholders is only applied
to one AI-command execution path (it builds cmd_restored via
secret_vault.lock().restore and writes an info message) but the pending-response
branch still uses the original cmd and close_unclosed_heredoc(cmd), allowing
unresolved placeholders to be executed; fix by factoring the restoration into a
shared helper (or reuse the existing cmd_restored logic) so both execution
branches use the restored command variable (e.g., use cmd_restored instead of
cmd and call close_unclosed_heredoc(cmd_restored) where the pending-response
execution happens), referencing the secret_vault, restore(...) result,
cmd_restored variable and close_unclosed_heredoc to locate and update the code
paths.
- Around line 1306-1335: The current branch only redacts when secret_vault is
Some, so if the user chose Redact but secret_vault is None the code silently
sends the original question; change the logic around secret_vault so that when
the user selected Redact and secret_vault.is_none() you explicitly abort or
return an error (or refuse to send) and emit a clear message to the user instead
of proceeding. Locate the block that references secret_vault, vault.lock(),
redact(...), question and detected_secrets and add a guard that handles None by
reporting the failure (and not sending the original question) or by forcing safe
fallback behavior (e.g., canceling the send or prompting to configure a vault)
before any write/return occurs.
In `@crates/aish-security/src/secret/vault.rs`:
- Around line 7-11: The SecretEntry struct contains an unused field secret_type
which triggers clippy warnings; either remove the secret_type field from
SecretEntry or use it where entries are constructed/consumed. Locate the
SecretEntry definition and the places that create or match on SecretEntry
(references to SecretEntry and its fields placeholder and original) and either
delete the secret_type field from the struct and update all constructors/usages
to stop supplying or expecting it, or thread the SecretType through
creation/consumption sites so secret_type is actually read (e.g., when building,
matching, or serializing SecretEntry) to eliminate the dead-field warning.
In `@crates/aish-shell/src/app.rs`:
- Around line 1317-1329: The code in the SecretDialogChoice::Redact branch
unwraps decision.analysis.detected_secrets which can panic; change it to safely
handle None (e.g., if let Some(secrets) = &decision.analysis.detected_secrets {
... } else { log or print a warning and skip redaction/leave question unchanged
}). Ensure the logic still calls
self.secret_vault.lock().unwrap().redact(&secrets, &question) only when secrets
is Some, updates count via self.secret_vault.lock().unwrap().len(), prints the
redaction message, and assigns question = redacted; otherwise avoid calling
redact and don’t unwrap the Option.
---
Outside diff comments:
In `@crates/aish-security/src/secret/patterns.rs`:
- Around line 13-17: The manual impl Default for SecretType (fn default() ->
Self { Self::ApiKey }) should be replaced by deriving Default on the enum: add
#[derive(Default)] to the SecretType declaration, annotate the ApiKey variant
with #[default], and remove the existing impl Default block so clippy's
clippy::derivable-impls warning is resolved; locate the SecretType enum and the
impl Default in patterns.rs to make these changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a513f25a-1379-48d4-9995-7c5875bcbb21
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
crates/aish-i18n/locales/en-US.yamlcrates/aish-i18n/locales/zh-CN.yamlcrates/aish-pty/Cargo.tomlcrates/aish-pty/src/lib.rscrates/aish-pty/src/persistent.rscrates/aish-security/src/lib.rscrates/aish-security/src/secret/mod.rscrates/aish-security/src/secret/patterns.rscrates/aish-security/src/secret/vault.rscrates/aish-shell/src/app.rscrates/aish-shell/src/tui.rs
✅ Files skipped from review due to trivial changes (1)
- crates/aish-pty/Cargo.toml
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/aish-security/src/secret/vault.rs (2)
146-146:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winReplace
sort_bywithsort_by_keyto fix clippy warning.Clippy flags
unnecessary_sort_bywhen sorting by a key projection. Usesort_by_keywithReversefor clarity and to pass CI.🐛 Proposed fix
- replacements.sort_by(|a, b| b.1.cmp(&a.1)); + replacements.sort_by_key(|r| std::cmp::Reverse(r.1));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aish-security/src/secret/vault.rs` at line 146, Clippy warns about using sort_by with a direct key comparison in replacements.sort_by(|a, b| b.1.cmp(&a.1)); — replace that call with sort_by_key and Reverse to express the descending key sort clearly: use replacements.sort_by_key(|t| std::cmp::Reverse(t.1)); this removes the unnecessary_sort_by lint while keeping the same sort order (reference the replacements.sort_by call and use std::cmp::Reverse and sort_by_key).
168-168:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winReplace
sort_bywithsort_by_keyto fix clippy warning.Clippy flags
unnecessary_sort_bywhen sorting by a key projection. Usesort_by_keywithReversefor clarity and to pass CI.🐛 Proposed fix
- entries.sort_by(|a, b| b.placeholder.len().cmp(&a.placeholder.len())); + entries.sort_by_key(|e| std::cmp::Reverse(e.placeholder.len()));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aish-security/src/secret/vault.rs` at line 168, Clippy warns about using sort_by where a key projection suffices; change the call to use sort_by_key with Reverse on the key so the sort remains descending: replace the entries.sort_by(...) invocation that compares b.placeholder.len() to a.placeholder.len() with entries.sort_by_key(|e| Reverse(e.placeholder.len())) (reference symbols: entries.sort_by, placeholder.len(), sort_by_key, Reverse).
♻️ Duplicate comments (1)
crates/aish-security/src/secret/vault.rs (1)
7-11:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRemove unused
secret_typefield to fix build failure.This field is stored (line 136) but never read, causing clippy
dead_codeerrors that fail CI with-D warnings. Either remove the field entirely or use it where entries are consumed (e.g., inrestoreor for logging/debugging).🐛 Proposed fix: remove the field
struct SecretEntry { placeholder: String, original: String, - secret_type: SecretType, }self.entries.insert( value.to_string(), SecretEntry { placeholder: placeholder.clone(), original: value.to_string(), - secret_type: sm.secret_type, }, );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aish-security/src/secret/vault.rs` around lines 7 - 11, The SecretEntry struct contains an unused field secret_type causing dead_code warnings; remove the secret_type field from the SecretEntry definition and any code that constructs or assigns to that field (e.g., the place where entries are pushed/stored around the code that corresponds to line 136), and update any pattern matches or constructors referencing SecretEntry::secret_type so they use only placeholder and original (or add use of the type if you prefer to keep it); ensure all places creating SecretEntry (constructors/vec pushes) are updated to the new two-field shape to fix the build.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/aish-security/src/secret/vault.rs`:
- Line 146: Clippy warns about using sort_by with a direct key comparison in
replacements.sort_by(|a, b| b.1.cmp(&a.1)); — replace that call with sort_by_key
and Reverse to express the descending key sort clearly: use
replacements.sort_by_key(|t| std::cmp::Reverse(t.1)); this removes the
unnecessary_sort_by lint while keeping the same sort order (reference the
replacements.sort_by call and use std::cmp::Reverse and sort_by_key).
- Line 168: Clippy warns about using sort_by where a key projection suffices;
change the call to use sort_by_key with Reverse on the key so the sort remains
descending: replace the entries.sort_by(...) invocation that compares
b.placeholder.len() to a.placeholder.len() with entries.sort_by_key(|e|
Reverse(e.placeholder.len())) (reference symbols: entries.sort_by,
placeholder.len(), sort_by_key, Reverse).
---
Duplicate comments:
In `@crates/aish-security/src/secret/vault.rs`:
- Around line 7-11: The SecretEntry struct contains an unused field secret_type
causing dead_code warnings; remove the secret_type field from the SecretEntry
definition and any code that constructs or assigns to that field (e.g., the
place where entries are pushed/stored around the code that corresponds to line
136), and update any pattern matches or constructors referencing
SecretEntry::secret_type so they use only placeholder and original (or add use
of the type if you prefer to keep it); ensure all places creating SecretEntry
(constructors/vec pushes) are updated to the new two-field shape to fix the
build.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a595ca60-a3dd-46bc-b206-b10fccf6cac1
📒 Files selected for processing (2)
crates/aish-security/src/secret/vault.rscrates/aish-shell/src/app.rs
2554047 to
9c658d1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/aish-tools/src/bash.rs`:
- Line 12: The import line using aish_security (symbols load_policy,
secret::SecretVault, SecurityDecision, SecurityManager, SecurityRequest) is not
formatted according to rustfmt; run `cargo fmt --all` (or reformat the use
statement to match rustfmt style) so the use declaration is split/ordered as
rustfmt expects and CI passes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 49e8e897-2e85-4436-a4b3-ce95fccabf7c
📒 Files selected for processing (3)
crates/aish-security/src/secret/vault.rscrates/aish-shell/src/app.rscrates/aish-tools/src/bash.rs
3f78f04 to
ee7c869
Compare
Add regex-based secret scanner (API keys, JWTs, passwords) and an in-memory SecretVault that redacts detected secrets to semantic placeholders ($SECRET_*) before sending to the LLM, then restores them for command execution and re-redacts command output. - SecretScanner with RegexSet for efficient multi-pattern matching - SecretVault with redact/restore/redact_output round-trip - Three-option dialog: redact to env vars, send plaintext, or abort - Both TUI (inquire) and SSH (raw terminal I/O) dialog paths - BashTool integration: restore before execution, redact output before returning to AI, restore in preflight for accurate security checks
Summary
aish-security::secretmodule with built-in patterns for detecting API keys (OpenAI, Anthropic, AWS, Google, GitHub, Stripe, Slack, Fireworks), JWT tokens, and URL-embedded passwords in AI input textsecret_patternsinconfig/security_policy.yamlSecurityManagerinapp.rsto load policy from config (viaload_policy()) instead of usingSecurityPolicy::default(), so user-definedsecret_patternsare appliedRegexSet/compiledVec index alignment withempty()fallback instead of mismatched["^$"]patternTest plan
cargo check -p aish-shellpassescargo test -p aish-securitypasses (4/4 tests)security_policy.yaml, restart aish, verify it's detectedSummary by CodeRabbit
New Features
Documentation