feat(security): add InputGuard for pre-execution input screening#247
Merged
Conversation
|
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 a complete ChangesInputGuard Security Screening
Sequence Diagram(s)sequenceDiagram
participant User
participant AishShell
participant InputGuard
participant PersistentPty
rect rgba(70, 130, 180, 0.5)
note over AishShell,InputGuard: Non-PTY command path
User->>AishShell: enter command
AishShell->>InputGuard: check(input, ShellCommand)
alt Block
InputGuard-->>AishShell: Block{reason}
AishShell->>User: print reason in red, record history exit=1
else Confirm/Unknown
InputGuard-->>AishShell: Confirm{reason}/Unknown{reason}
AishShell->>User: yellow warning + raw [y/N] keystroke
alt confirmed
User-->>AishShell: y/Y
AishShell->>PersistentPty: send_command_interactive(input_guard_enabled=true)
else declined
User-->>AishShell: n/N/Ctrl+C/ESC
AishShell->>User: print Block reason in red
AishShell->>AishShell: record history exit=1
end
else Allow
InputGuard-->>AishShell: Allow
AishShell->>PersistentPty: send_command_interactive(input_guard_enabled=true)
end
end
rect rgba(100, 180, 100, 0.5)
note over PersistentPty,InputGuard: Interactive PTY session path (session interceptor)
PersistentPty->>PersistentPty: SessionInterceptor::new(..., input_guard_enabled)
User->>PersistentPty: keystroke → feed_stdin(Enter)
PersistentPty->>InputGuard: check(guard_shadow, ShellCommand)
alt Blocked
InputGuard-->>PersistentPty: Block{reason}
PersistentPty->>PersistentPty: send Ctrl+C, display reason
else NeedConfirm
InputGuard-->>PersistentPty: Confirm{reason,line}
PersistentPty->>User: read_confirm_raw [y/N]
alt confirmed
User-->>PersistentPty: y/Y
PersistentPty->>PersistentPty: re-inject line + \r
else declined
User-->>PersistentPty: n/N/Ctrl+C/ESC
PersistentPty->>PersistentPty: send Ctrl+C, print Cancelled
end
else Allow
InputGuard-->>PersistentPty: Allow
PersistentPty->>PersistentPty: forward command
end
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Poem
🚥 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 |
Contributor
|
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 |
Contributor
|
This pull request description looks incomplete. Please update the missing sections below before review. Missing items:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/aish-shell/src/app.rs (2)
1748-1749:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScreen AI-corrected commands before execution.
The error-correction flow executes the AI-suggested
correctedcommand directly after a generic Y/n prompt, bypassingscreen_shell_command, so a destructive corrected command can skip the new InputGuard gate.Proposed fix
if answer == "y" || answer == "yes" || answer.is_empty() { - let exit_code = - self.execute_external_command(corrected); - self.record_history(corrected, exit_code); + if self.screen_shell_command(corrected) { + let exit_code = + self.execute_external_command(corrected); + self.record_history(corrected, exit_code); + } }🤖 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-shell/src/app.rs` around lines 1748 - 1749, The AI-corrected command is being executed directly without screening, which bypasses the InputGuard protection gate. In the error-correction flow where `self.execute_external_command(corrected)` is called, the `corrected` command needs to be screened through the `screen_shell_command` method first (just like other commands are) to ensure it passes the InputGuard validation before being executed. This ensures that destructive corrected commands are caught by the same safety gate as other commands.
2297-2351:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply InputGuard to slash-dismissed submissions.
read_line_after_slash_dismissroutes submitted text throughprocess_readline_submission, which executes commands/scripts withoutscreen_shell_command. Typing/, dismissing the popup, then submitting a destructive command bypasses the new guard.Proposed fix direction
crate::types::InputIntent::Command | crate::types::InputIntent::OperatorCommand => { + if !self.screen_shell_command(input) { + return false; + } self.set_phase(ShellPhase::Running); let exit_code = self.execute_external_command(input); @@ crate::types::InputIntent::ScriptCall => { + if !self.screen_shell_command(input) { + return false; + } let exit_code = self.execute_script(input); self.record_history(input, exit_code); falseAlso mirror the main-loop
route_to_ptyhandling for builtins such assudo/su, or preferably delegate both paths to one shared submission handler.🤖 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-shell/src/app.rs` around lines 2297 - 2351, The method `process_readline_submission` executes commands and scripts without applying the InputGuard protection (via `screen_shell_command`), allowing destructive commands to bypass security checks when a user dismisses the slash popup and submits. Apply InputGuard protection to both the `execute_external_command` call in the Command/OperatorCommand branch and the `execute_script` call in the ScriptCall branch by routing them through the same guard mechanism used in the main loop, or consolidate both submission paths into a single shared handler that applies the guard consistently.
🤖 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-config/src/loader.rs`:
- Around line 64-66: The current implementation writes the migrated config
directly to the target file using std::fs::write, which can result in a
corrupted config.yaml if the process crashes during the write operation. Instead
of writing directly to the path, create a temporary file in the same directory
as path, write the migrated content to this temporary file, and then use
std::fs::rename to atomically replace the original file. This ensures that the
target file remains intact if the write operation is interrupted.
- Around line 61-66: The current migration rewrite serializes only the typed
ConfigModel using serde_yaml::to_string(&config), which loses any unknown keys
and user extensions from the original YAML. Instead of re-serializing the typed
model, preserve the merged serde_yaml::Value after successfully deserializing it
to the typed model. Replace the to_string call in the migration block with
writing the preserved Value directly to ensure the migration is additive rather
than lossy and maintains all original content while applying the necessary
updates.
In `@crates/aish-pty/src/persistent.rs`:
- Around line 1091-1122: In the StdinAction::Blocked(reason) branch, after
displaying the block reason, clear the stdin_shadow buffer to keep it in sync.
In the StdinAction::NeedConfirm { reason, line } branch, when the user confirms
(confirmed is true), execute the same nested-session update logic that would run
in the Forward branch for the line parameter (updating remote_host_for_probe and
shared_host) before clearing stdin_shadow. When the user declines or in the
blocked case, also clear stdin_shadow to prevent stale data from persisting.
- Around line 453-455: The InputGuard is currently only applied in the
feed_stdin method, but AI-generated commands injected through response.command
and BashExec paths bypass this guard before being written to master_fd. Locate
all command injection sites in SessionInterceptor where AI responses or BashExec
commands are written to master_fd (at the specified line ranges 927-967,
2070-2103, and 3541-3563), and apply the same InputGuard logic that exists in
feed_stdin before these writes occur. Gate each injection point with the
InputGuard to ensure all shell commands, whether from user input or AI
responses, are screened and verified before execution.
- Around line 3019-3054: The read_confirm_raw function reads a confirmation byte
but does not drain remaining input from stdin after making a decision, which can
cause trailing bytes like Enter to be forwarded unintentionally to the remote
session. After each match arm in the match statement (the ones handling
Some(b'y'|b'Y'), Some(0x03), Some(0x1b), Some(_), and None), add a call to drain
any remaining bytes from stdin before returning the boolean decision. This
ensures that only the user's intentional confirmation byte is consumed and any
accidental trailing input is cleared.
- Around line 1095-1117: The issue is that in both the Blocked and NeedConfirm
branches, the Ctrl+C byte (0x03) is queued to write_buf via
interceptor.take_cancel_pty_line() but the buffer is not flushed before blocking
on show() or read_confirm_raw(). This causes the pending cancel byte to be sent
after user confirmation, interfering with the reinjected line. Fix this by
ensuring the write_buf containing the Ctrl+C is flushed to the PTY immediately
after pushing 0x03 and before calling show() in the Blocked branch or
read_confirm_raw() in the NeedConfirm branch, so the cancel is actually sent to
the remote shell before the user interaction or line reinjection occurs.
In `@crates/aish-pty/src/session_interceptor.rs`:
- Around line 289-296: The InputGuard screening in the bracketed paste handling
is checking only `line_shadow` which excludes pasted bytes, allowing commands to
bypass security checks when pasted. To fix this, modify the code around the
`self.input_guard.check()` call to construct the complete command line that
includes both the existing `line_shadow` content and any pasted bytes before
passing it to the guard. Either maintain a separate shadow buffer for the guard
that tracks all input including pastes, or reconstruct the full line by
combining line_shadow with the buffered paste content when creating the string
passed to `self.input_guard.check()`. Ensure that while the guard sees the
complete command for security evaluation, AI and status message triggers are
still suppressed separately for pasted newlines. Apply the same fix at the
second location noted in the comment.
In `@crates/aish-shell/src/app.rs`:
- Around line 2074-2077: The current code path calls screen_shell_command(input)
before routing input through natural language processing, which causes NL-like
inputs such as "how do I kill a process?" to be screened as shell commands first
and trigger shell confirmation prematurely. Reorder the logic in the input
handling section around screen_shell_command so that natural language routing is
evaluated first, allowing the InputGuard to properly route NL prompts to AI
context (which intentionally skips Confirm rules) before any shell command
screening occurs.
- Around line 402-407: The read_confirm_keystroke function consumes only a
single byte from stdin, leaving remaining input (such as the Enter key or
additional characters when a user types "yes<Enter>") queued in the input
buffer. These leftover bytes will be consumed by the next prompt as unintended
input. After calling read_confirm_keystroke(stdin_fd), drain the remaining bytes
from the stdin buffer before restoring the terminal settings with tcsetattr to
ensure no trailing input interferes with subsequent prompts. This same issue
also needs to be fixed in the other locations mentioned in the comment (lines
5382-5399).
- Around line 5390-5391: Replace the unsafe errno access pattern using
`libc::__errno_location()` at lines 5390-5391 with the portable standard library
approach. Instead of directly accessing errno through libc, use
`std::io::Error::last_os_error().raw_os_error()` which returns an `Option<i32>`.
Update the conditional check to compare against the unwrapped value and verify
it matches `libc::EINTR` to maintain the same logic while using the more
portable and consistent approach already used elsewhere in the codebase such as
in `crates/aish-pty/src/persistent.rs`.
---
Outside diff comments:
In `@crates/aish-shell/src/app.rs`:
- Around line 1748-1749: The AI-corrected command is being executed directly
without screening, which bypasses the InputGuard protection gate. In the
error-correction flow where `self.execute_external_command(corrected)` is
called, the `corrected` command needs to be screened through the
`screen_shell_command` method first (just like other commands are) to ensure it
passes the InputGuard validation before being executed. This ensures that
destructive corrected commands are caught by the same safety gate as other
commands.
- Around line 2297-2351: The method `process_readline_submission` executes
commands and scripts without applying the InputGuard protection (via
`screen_shell_command`), allowing destructive commands to bypass security checks
when a user dismisses the slash popup and submits. Apply InputGuard protection
to both the `execute_external_command` call in the Command/OperatorCommand
branch and the `execute_script` call in the ScriptCall branch by routing them
through the same guard mechanism used in the main loop, or consolidate both
submission paths into a single shared handler that applies the guard
consistently.
🪄 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: 3e817528-3dd5-4bcd-80b0-6c33fdf3e394
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
crates/aish-config/Cargo.tomlcrates/aish-config/src/loader.rscrates/aish-config/src/model.rscrates/aish-pty/src/bash_rc_wrapper.shcrates/aish-pty/src/persistent.rscrates/aish-pty/src/session_interceptor.rscrates/aish-security/src/input_guard/config.rscrates/aish-security/src/input_guard/mod.rscrates/aish-security/src/input_guard/patterns.rscrates/aish-security/src/input_guard/targets.rscrates/aish-security/src/input_guard/unanalyzable.rscrates/aish-security/src/lib.rscrates/aish-security/src/policy.rscrates/aish-shell/src/app.rs
jexShain
force-pushed
the
feature/input-guard
branch
from
a928d32 to
28f8c8c
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/aish-pty/src/session_interceptor.rs (1)
290-297:⚠️ Potential issue | 🔴 CriticalAdd InputGuard screening before invoking AI callback in persistent.rs SSH handler.
Remote SSH prompts bypass
InputGuardscreening. TheTriggerAihandler inpersistent.rs(line 1364+) invokescall_aidirectly without screening the question throughInputContext::AiPrompt. In contrast, local shell (app.rs) callsscreen_ai_prompt()before AI processing. Remote users can thus inject AI prompts containing dangerous patterns (e.g.,;rm -rf /) that are not validated by theInputGuard.Call
screen_ai_prompt(question)or equivalent before line 1587'sinterceptor.call_ai(question, ...)in the SSH session handler to match the local shell's security posture.🤖 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-pty/src/session_interceptor.rs` around lines 290 - 297, The SSH session handler in persistent.rs is invoking interceptor.call_ai without screening the AI question through InputGuard validation, creating a security vulnerability where dangerous patterns can bypass input validation. Before the call to interceptor.call_ai(question, ...) at line 1587 in persistent.rs, add a call to screen_ai_prompt(question) to validate the question through InputContext::AiPrompt, matching the security validation performed by the local shell handler in app.rs. This ensures remote SSH prompts receive the same InputGuard screening as local shell input.
🤖 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 1113-1209: The issue is that after a guard decision (Blocked or
NeedConfirm), the for &byte in data loop continues processing remaining bytes
from the current stdin batch instead of discarding them, which can leak extra
input after the guarded command. Fix this by adding a break statement at the end
of both the crate::StdinAction::Blocked(reason) and
crate::StdinAction::NeedConfirm { reason, line } match arms (after setting
skip_leading_newline in each case) to exit the loop and discard any remaining
bytes in the current data batch.
In `@crates/aish-pty/src/session_interceptor.rs`:
- Around line 258-260: The issue is that the code commits pending completion
when a non-tab byte arrives without waiting for actual PTY completion
confirmation, creating a security vulnerability where unconfirmed completion
state allows stale buffer content to be forwarded. Remove the call to
commit_pending_completion() in the condition checking self.awaiting_completion
&& byte != 0x09, and instead implement an "uncertain completion" state that
blocks line forwarding until the actual PTY completion response is received.
This same issue also affects the related code around lines 472-477, so both
locations need to be updated to treat unresolved completions as untrusted and
require explicit verification before allowing commands to proceed.
- Around line 299-308: The issue is that handle_escape_seq_byte forwards certain
CSI sequences without updating guard_shadow, causing desynchronization between
the shadow buffer and the actual terminal content. When users recall commands
using arrow keys and press Enter, the input guard checks against stale shadow
data instead of the actual command. To fix this: mark guard_shadow as untrusted
or invalidated whenever handle_escape_seq_byte forwards unsupported readline or
history editing sequences, then at the location where the line is extracted from
guard_shadow using String::from_utf8_lossy, add logic to detect and handle
desynchronized state by either blocking the command, requesting user
confirmation, or reconstructing the actual line before passing it to
input_guard.check. This same pattern should be applied to all locations
mentioned in the review including the ranges around lines 299-308 and 398-443.
- Around line 318-325: The saved variable is being assigned from line_shadow,
which excludes bracketed-paste bytes, but the NeedConfirm action uses this saved
value for reinjection after user approval. For a pasted command like sudo ls,
this results in reinjecting an empty or truncated line. Instead of cloning
line_shadow, clone guard_shadow which is the screened buffer that preserves all
necessary content including bracketed-paste information, ensuring the caller
reinjects the complete command after approval.
In `@crates/aish-security/src/input_guard/config.rs`:
- Around line 45-47: The to_input_rule method is silently discarding regex
compilation errors by using .ok(), which means misconfigured rule patterns are
dropped without any warning or validation feedback. Instead of using .ok() to
suppress the error, modify the to_input_rule method to return a Result type or
an error type that communicates what went wrong with the regex pattern. Then
update the merge_custom_rules method (which calls to_input_rule) to collect and
report the invalid rule names and their associated errors instead of silently
skipping them, ensuring that configuration mistakes fail visibly during policy
validation or through logging.
---
Outside diff comments:
In `@crates/aish-pty/src/session_interceptor.rs`:
- Around line 290-297: The SSH session handler in persistent.rs is invoking
interceptor.call_ai without screening the AI question through InputGuard
validation, creating a security vulnerability where dangerous patterns can
bypass input validation. Before the call to interceptor.call_ai(question, ...)
at line 1587 in persistent.rs, add a call to screen_ai_prompt(question) to
validate the question through InputContext::AiPrompt, matching the security
validation performed by the local shell handler in app.rs. This ensures remote
SSH prompts receive the same InputGuard screening as local shell input.
🪄 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: 70472fee-e500-41ab-afd8-9adfda13ba2e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
crates/aish-config/Cargo.tomlcrates/aish-config/src/loader.rscrates/aish-config/src/model.rscrates/aish-pty/src/bash_rc_wrapper.shcrates/aish-pty/src/persistent.rscrates/aish-pty/src/session_interceptor.rscrates/aish-security/src/input_guard/config.rscrates/aish-security/src/input_guard/mod.rscrates/aish-security/src/input_guard/patterns.rscrates/aish-security/src/input_guard/targets.rscrates/aish-security/src/input_guard/unanalyzable.rscrates/aish-security/src/lib.rscrates/aish-security/src/policy.rscrates/aish-shell/src/app.rs
✅ Files skipped from review due to trivial changes (1)
- crates/aish-config/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (10)
- crates/aish-security/src/lib.rs
- crates/aish-pty/src/bash_rc_wrapper.sh
- crates/aish-config/src/model.rs
- crates/aish-security/src/input_guard/targets.rs
- crates/aish-security/src/policy.rs
- crates/aish-config/src/loader.rs
- crates/aish-security/src/input_guard/patterns.rs
- crates/aish-security/src/input_guard/unanalyzable.rs
- crates/aish-security/src/input_guard/mod.rs
- crates/aish-shell/src/app.rs
jexShain
force-pushed
the
feature/input-guard
branch
from
28f8c8c to
67ff326
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/aish-security/src/input_guard/targets.rs (1)
21-26: 💤 Low valueConsider adding additional block device patterns for completeness.
The current regex covers common device types but misses some environments:
/dev/xvd*(Xen virtual devices, common in AWS)/dev/loop*(loop devices)/dev/dm-*(device mapper, LVM)/dev/md*(software RAID)♻️ Suggested enhancement
RE.get_or_init(|| { - Regex::new(r"/dev/(?:sd[a-z]\d*|nvme\d+n\d+(?:p\d+)?|vd[a-z]\d*|hd[a-z]\d*)").unwrap() + Regex::new(r"/dev/(?:sd[a-z]\d*|nvme\d+n\d+(?:p\d+)?|vd[a-z]\d*|hd[a-z]\d*|xvd[a-z]\d*|loop\d+|dm-\d+|md\d+)").unwrap() })🤖 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/input_guard/targets.rs` around lines 21 - 26, The block_device_re() function's regex pattern is incomplete and misses several important block device types used in different environments. Update the regex pattern in the Regex::new() call to include additional device patterns for Xen virtual devices (xvd*), loop devices (loop*), device mapper/LVM devices (dm-*), and software RAID devices (md*) alongside the existing patterns for SATA/SCSI, NVMe, KVM virtual, and IDE devices. Ensure the pattern properly handles the specific naming conventions and optional numbering for each device type.
🤖 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.
Nitpick comments:
In `@crates/aish-security/src/input_guard/targets.rs`:
- Around line 21-26: The block_device_re() function's regex pattern is
incomplete and misses several important block device types used in different
environments. Update the regex pattern in the Regex::new() call to include
additional device patterns for Xen virtual devices (xvd*), loop devices (loop*),
device mapper/LVM devices (dm-*), and software RAID devices (md*) alongside the
existing patterns for SATA/SCSI, NVMe, KVM virtual, and IDE devices. Ensure the
pattern properly handles the specific naming conventions and optional numbering
for each device type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3baead33-658c-444f-b2b6-5945c27e8e4b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
crates/aish-config/Cargo.tomlcrates/aish-config/src/loader.rscrates/aish-config/src/model.rscrates/aish-pty/src/bash_rc_wrapper.shcrates/aish-pty/src/persistent.rscrates/aish-pty/src/session_interceptor.rscrates/aish-security/src/input_guard/config.rscrates/aish-security/src/input_guard/mod.rscrates/aish-security/src/input_guard/patterns.rscrates/aish-security/src/input_guard/targets.rscrates/aish-security/src/input_guard/unanalyzable.rscrates/aish-security/src/lib.rscrates/aish-security/src/policy.rscrates/aish-shell/src/app.rs
🚧 Files skipped from review as they are similar to previous changes (11)
- crates/aish-security/src/lib.rs
- crates/aish-config/Cargo.toml
- crates/aish-pty/src/bash_rc_wrapper.sh
- crates/aish-security/src/input_guard/config.rs
- crates/aish-security/src/policy.rs
- crates/aish-security/src/input_guard/unanalyzable.rs
- crates/aish-security/src/input_guard/mod.rs
- crates/aish-security/src/input_guard/patterns.rs
- crates/aish-pty/src/persistent.rs
- crates/aish-pty/src/session_interceptor.rs
- crates/aish-shell/src/app.rs
jexShain
force-pushed
the
feature/input-guard
branch
2 times, most recently
from
3da65eb to
26e5446
Compare
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)
crates/aish-shell/src/app.rs (2)
2212-2214:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGate main-loop script invocations as well.
The slash-dismissed path now screens
ScriptCall, but the main REPL branch still executes scripts directly. If script parsing fails,execute_scriptfalls back toexecute_external_command(input), so the invocation itself should clear the same shell-command gate.Proposed fix
crate::types::InputIntent::ScriptCall => { + if !self.screen_shell_command(input) { + continue; + } let exit_code = self.execute_script(input); self.record_history(input, exit_code); }Also applies to: 2377-2383
🤖 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-shell/src/app.rs` around lines 2212 - 2214, The main REPL branch handling ScriptCall in the InputIntent match statement (around lines 2212-2214) and the similar code at lines 2377-2383 should apply the same screening gate that was applied to the slash-dismissed path. Before calling self.execute_script(input), wrap the script execution with the same conditional check that validates the invocation against the shell-command gate, ensuring consistent gating behavior across all script invocation paths in the main loop.
2020-2074:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScreen state-modifying builtins before mutating shell state.
handle_builtinruns before the InputGuard check, so a blocked/declinedcd/export/unsetcan already change Rust-side state. Also,screen_shell_commandrecords exit code1, then Line 2071 records the same rejected input as success.Proposed fix
let parts: Vec<&str> = input.split_whitespace().collect(); if let Some(cmd) = parts.first() { + let state_modifying = crate::commands::is_state_modifying(cmd) + && !crate::commands::is_rejected(cmd); + if state_modifying && !self.screen_shell_command(input) { + continue; + } + let result = self.state.handle_builtin(cmd, &parts[1..]); @@ - if crate::commands::is_state_modifying(cmd) - && !crate::commands::is_rejected(cmd) - { - if !self.screen_shell_command(input) { - // Destructive payload blocked — skip the sync - // so bash never sees it. State in Rust is - // already updated by handle_builtin above, - // but the destructive payload in the value - // never reaches bash. - self.record_history(input, 0); - continue; - } + if state_modifying { self.sync_command_to_pty(input); } @@ - if crate::commands::is_state_modifying(cmd) - && !crate::commands::is_rejected(cmd) - { + if state_modifying { entry.push_str(&format!("\n<cwd>{}</cwd>", self.state.cwd)); }🤖 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-shell/src/app.rs` around lines 2020 - 2074, The issue is that handle_builtin is called before InputGuard screening for state-modifying commands, allowing rejected commands to mutate shell state, and screen_shell_command records exit code 1 while record_history then records the same input with exit code 0. Move the InputGuard check for state-modifying commands (the is_state_modifying and is_rejected condition block) to execute BEFORE calling handle_builtin so rejected commands never reach handle_builtin and don't mutate state. Remove the redundant record_history call inside the blocked command branch to avoid double-recording since screen_shell_command already handles recording the rejection.
🤖 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-shell/src/app.rs`:
- Around line 3263-3283: The pre-screen logic in the script screening loop uses
a simple prefix check (starts_with "ai ") to skip lines, but this is
inconsistent with how the actual execution handler validates AI calls using a
regex pattern (ai_call_re). This allows malformed lines like `ai $(rm -rf /)` to
incorrectly bypass shell command screening. Replace the simple prefix checks in
the loop with the same regex pattern validation used by the actual execution
handler to ensure only properly formatted AI calls matching ai_call_re are
skipped from shell screening, and ensure all AI prompts are properly validated
through screen_ai_prompt before being processed.
---
Outside diff comments:
In `@crates/aish-shell/src/app.rs`:
- Around line 2212-2214: The main REPL branch handling ScriptCall in the
InputIntent match statement (around lines 2212-2214) and the similar code at
lines 2377-2383 should apply the same screening gate that was applied to the
slash-dismissed path. Before calling self.execute_script(input), wrap the script
execution with the same conditional check that validates the invocation against
the shell-command gate, ensuring consistent gating behavior across all script
invocation paths in the main loop.
- Around line 2020-2074: The issue is that handle_builtin is called before
InputGuard screening for state-modifying commands, allowing rejected commands to
mutate shell state, and screen_shell_command records exit code 1 while
record_history then records the same input with exit code 0. Move the InputGuard
check for state-modifying commands (the is_state_modifying and is_rejected
condition block) to execute BEFORE calling handle_builtin so rejected commands
never reach handle_builtin and don't mutate state. Remove the redundant
record_history call inside the blocked command branch to avoid double-recording
since screen_shell_command already handles recording the rejection.
🪄 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: 3a170eab-57a0-4fc1-a6e7-a64ec1abde43
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
config/security_policy.yamlcrates/aish-config/Cargo.tomlcrates/aish-config/src/loader.rscrates/aish-config/src/model.rscrates/aish-pty/src/bash_rc_wrapper.shcrates/aish-pty/src/persistent.rscrates/aish-pty/src/session_interceptor.rscrates/aish-security/src/input_guard/config.rscrates/aish-security/src/input_guard/mod.rscrates/aish-security/src/input_guard/patterns.rscrates/aish-security/src/input_guard/targets.rscrates/aish-security/src/input_guard/unanalyzable.rscrates/aish-security/src/lib.rscrates/aish-security/src/policy.rscrates/aish-shell/src/app.rs
✅ Files skipped from review due to trivial changes (1)
- crates/aish-config/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (13)
- crates/aish-security/src/lib.rs
- crates/aish-config/src/model.rs
- config/security_policy.yaml
- crates/aish-pty/src/bash_rc_wrapper.sh
- crates/aish-security/src/policy.rs
- crates/aish-security/src/input_guard/config.rs
- crates/aish-security/src/input_guard/unanalyzable.rs
- crates/aish-config/src/loader.rs
- crates/aish-security/src/input_guard/targets.rs
- crates/aish-security/src/input_guard/patterns.rs
- crates/aish-pty/src/persistent.rs
- crates/aish-security/src/input_guard/mod.rs
- crates/aish-pty/src/session_interceptor.rs
jexShain
force-pushed
the
feature/input-guard
branch
2 times, most recently
from
c9dfc27 to
4596bf4
Compare
Add aish-security::input_guard module that screens shell commands and AI prompts before execution. Rules cover destructive commands, system compromise, resource abuse, network exfiltration, privilege escalation, and code injection, with target extraction for paths, hosts, and services so messages can name the concrete risk target. Wire the guard into both input paths: - aish-shell: AishShell::screen_input drives verdicts to Allow / Confirm / Block, with a raw-mode y/N prompt for Confirm. Block and user-decline record history with exit code 1 so failed attempts stay visible. - aish-pty: SessionInterceptor gains Blocked and NeedConfirm StdinAction variants so remote PTY sessions get the same screening. Tab completion is captured from PTY output and merged into the shadow line so the guard sees the fully completed command on Enter. config.yaml's new input_guard_enabled mirrors security_policy.yaml's input_guard.enabled and takes precedence, giving users a single toggle for both paths. ConfigLoader now auto-migrates missing fields into existing yaml so the new key surfaces without manual editing; user values and ordering are preserved. bash_rc_wrapper.sh now strips C0 control bytes from the JSON event payload (RFC 8259 §7) and switches to LC_ALL=C for the substitution, fixing cases where stray bytes (e.g. Ctrl-U prefix from the frontend) caused the Rust control-event parser to reject the whole line.
jexShain
force-pushed
the
feature/input-guard
branch
from
4596bf4 to
0a69fe7
Compare
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
aish-security::input_guardmodule that screens shell commands and AI prompts before execution. Rules cover destructive commands, system compromise, resource abuse, network exfiltration, privilege escalation, and code injection, with target extraction (paths, hosts, services) so verdict messages can name the concrete risk target.AishShell::screen_inputdrives Allow / Confirm / Block verdicts with a raw-mode y/N prompt for Confirm. Block and user-decline record history with exit code 1 so failed attempts stay visible inhistory.SessionInterceptorgainsBlockedandNeedConfirmStdinActionvariants so remote PTY sessions get the same screening. Tab completion is captured from PTY output and merged into the shadow line so the guard sees the fully completed command on Enter.config.yaml's newinput_guard_enabledmirrorssecurity_policy.yaml'sinput_guard.enabledand takes precedence, giving users a single toggle for both paths.ConfigLoadernow auto-migrates missing fields into existing yaml so the new key surfaces without manual editing; user values and field ordering are preserved.bash_rc_wrapper.shnow strips C0 control bytes from the JSON event payload (RFC 8259 §7) and switches toLC_ALL=Cfor the substitution, fixing cases where stray bytes (e.g. the Ctrl-U prefix the frontend prepends) caused the Rust control-event parser to reject the whole line.Test plan
cargo fmt --all -- --check(clean)cargo clippy --all-targets -- -D warnings(clean)cargo test --workspace(all passing: 131 + 7 + 7 + 257 + 4 + 3 + 226 + 5 + 6 + 20 + 2 + 10 + 86 + 36 + 2 = 802 tests, 0 failures)rm -rf /) at the local shell prompts Block / ConfirmrmTab →rm-foo) is screened on the completed forminput_guard_enabled: falseinconfig.yamldisables screening on both pathsconfig.yamlwithout the new key gets it auto-appended on next launch without losing user valuesSummary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes