Better security#33
Merged
Merged
Conversation
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis PR introduces agent sandboxing by adding configurable work-directory boundaries and optional shell-execution gating. New ChangesAgent Sandboxing Feature
Sequence DiagramsequenceDiagram
participant Client
participant OpenheimBuilder
participant AppConfig
participant AgentState
participant SandboxedExecutor
participant ValidatePath
Client->>OpenheimBuilder: .work_dir("/path").allow_shell(false)
OpenheimBuilder->>OpenheimBuilder: store overrides
Client->>OpenheimBuilder: build()
OpenheimBuilder->>AppConfig: apply work_dir/allow_shell overrides
OpenheimBuilder->>AgentState: new(app_config)
AgentState->>AgentState: resolve work_dir, derive allow_shell
Client->>AgentState: acp_prompt()
AgentState->>SandboxedExecutor: new(executor, work_dir, allow_shell)
SandboxedExecutor->>SandboxedExecutor: intercept read_file/write_file/execute_command
SandboxedExecutor->>ValidatePath: validate requested path
ValidatePath->>ValidatePath: canonicalize work_dir, resolve requested
ValidatePath-->>SandboxedExecutor: validated path or error
SandboxedExecutor-->>AgentState: tool result or sandbox violation error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/tools/sandboxed_executor.rs (1)
46-48: ⚡ Quick winHide
execute_commandfromlist_tools()when shell is disabled.
execute()rejectsexecute_commandwhenallow_shellisfalse, butlist_tools()still republishes whatever the inner executor advertises. SinceSandboxedExecutoris public and wraps anyToolExecutor, that can present a disabled tool to the model outside the currentSystemToolExecutor::buildpath.🤖 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 `@src/tools/sandboxed_executor.rs` around lines 46 - 48, list_tools currently returns inner.list_tools() unchanged even when SandboxedExecutor has allow_shell == false, exposing the execute_command tool; change SandboxedExecutor::list_tools to call self.inner.list_tools(), then if self.allow_shell is false filter out the tool that corresponds to the shell command (the tool named "execute_command" or the Tool whose handler maps to execute_command), and return the filtered Vec<Tool>; keep all other tools unchanged so the public SandboxedExecutor does not advertise a disabled execute_command (see methods SandboxedExecutor::execute and SystemToolExecutor::build for how execute_command is rejected and identified).src/tools/mod.rs (1)
119-127: ⚡ Quick winAdd a regression test for the
allow_shellbranch.This is a security gate, but this file still has no test proving
build(..., false)actually omitsexecute_command. A small async test here would make regressions obvious.Suggested test
#[cfg(test)] mod tests { use super::*; + use std::collections::BTreeMap; #[test] fn new_executor_is_empty() { let executor = SystemToolExecutor::new(); assert_eq!(executor.handlers.len(), 0); @@ async fn executor_returns_error_for_unknown_tool() { let executor = SystemToolExecutor::new(); let result = executor.execute("nonexistent_tool", "{}").await; assert!(result.is_err()); assert!(result.unwrap_err().to_string().contains("Unknown tool")); } + + #[tokio::test] + async fn build_omits_execute_command_when_shell_is_disabled() { + let (executor, _statuses) = SystemToolExecutor::build(&BTreeMap::new(), false).await; + assert!(!executor.handlers.contains_key("execute_command")); + assert!(executor.handlers.contains_key("read_file")); + assert!(executor.handlers.contains_key("write_file")); + } }🤖 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 `@src/tools/mod.rs` around lines 119 - 127, Add an async regression test that calls tools::Executor::build(...) with allow_shell set to false and asserts the built executor does not contain the "execute_command" handler; specifically, invoke build (referencing the build function) after constructing minimal mcp_configs, then inspect the returned executor.handlers (the handlers map populated by register_builtins) and assert that the key "execute_command" is absent when allow_shell is false to prevent regressions in that security gate.
🤖 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 `@src/acp/mod.rs`:
- Around line 63-66: The code currently falls back to PathBuf::from(".") when
std::env::current_dir() fails, weakening the sandbox root; update the work_dir
resolution in mod.rs so that if app_config.work_dir is None you propagate the
current_dir() error instead of defaulting to "." — change the unwrap_or_else
that constructs work_dir to return a Result (or use ? with map/ok_or) and return
an explicit error (with context) when current_dir() fails, referencing the
work_dir variable and app_config.work_dir to locate the change.
In `@src/client.rs`:
- Around line 421-422: The builder currently sets allow_shell: true making
execute_command available by default; change the secure default by initializing
allow_shell to false in the builder/ClientBuilder (the struct field named
allow_shell) so shell access becomes explicit opt-in via .allow_shell(true);
update any related constructors or tests that assume the old default and ensure
execute_command remains gated by this field.
- Around line 369-371: In build(), resolve and validate self.work_dir before
storing it into app_config.work_dir: if let Some(wd) = self.work_dir {
canonicalize (or std::fs::canonicalize) wd to an absolute path (or join with
std::env::current_dir then canonicalize), return an error from build() on
failure, and set app_config.work_dir = Some(canonicalized_path); do not copy the
raw PathBuf. This ensures the sandbox boundary is normalized and fails early if
the path is inaccessible.
In `@src/config/types.rs`:
- Around line 49-56: The default for shell access is currently opt-out: change
the default from true to false so models must be explicitly opted in; update the
function default_allow_shell() to return false and ensure any docs/tests
referencing allow_shell reflect the new default behavior and that tools removal
logic (which checks allow_shell) still functions correctly with the new default.
In `@src/tools/sandbox.rs`:
- Around line 31-58: The code currently treats a broken symlink leaf as
non-existent because it uses Path::exists(), letting write_file follow and
overwrite outside the sandbox; fix by calling resolved.symlink_metadata()
(instead of or before exists()) to detect if the leaf is a symlink even when
broken: if symlink_metadata.file_type().is_symlink() then return
Err(Error::ToolExecutionError(...)) rejecting the dangling symlink leaf (include
requested/work_dir in the message) so write_file cannot follow it; update the
logic around resolved, canonicalize, work_dir_canonical and write_file
accordingly and add a regression test that creates a dangling symlink inside the
work_dir and verifies it is rejected.
---
Nitpick comments:
In `@src/tools/mod.rs`:
- Around line 119-127: Add an async regression test that calls
tools::Executor::build(...) with allow_shell set to false and asserts the built
executor does not contain the "execute_command" handler; specifically, invoke
build (referencing the build function) after constructing minimal mcp_configs,
then inspect the returned executor.handlers (the handlers map populated by
register_builtins) and assert that the key "execute_command" is absent when
allow_shell is false to prevent regressions in that security gate.
In `@src/tools/sandboxed_executor.rs`:
- Around line 46-48: list_tools currently returns inner.list_tools() unchanged
even when SandboxedExecutor has allow_shell == false, exposing the
execute_command tool; change SandboxedExecutor::list_tools to call
self.inner.list_tools(), then if self.allow_shell is false filter out the tool
that corresponds to the shell command (the tool named "execute_command" or the
Tool whose handler maps to execute_command), and return the filtered Vec<Tool>;
keep all other tools unchanged so the public SandboxedExecutor does not
advertise a disabled execute_command (see methods SandboxedExecutor::execute and
SystemToolExecutor::build for how execute_command is rejected and identified).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 53f622b4-837a-4662-a7af-8159db20f61b
📒 Files selected for processing (13)
CHANGELOG.mdREADME.mddocs/configuration.mddocs/library.mdsrc/acp/mod.rssrc/client.rssrc/config/client.rssrc/config/config.toml.defaultsrc/config/resolve.rssrc/config/types.rssrc/tools/mod.rssrc/tools/sandbox.rssrc/tools/sandboxed_executor.rs
Comment on lines
+63
to
+66
| let work_dir = app_config | ||
| .work_dir | ||
| .clone() | ||
| .unwrap_or_else(|| std::env::current_dir().unwrap_or_else(|_| PathBuf::from("."))); |
There was a problem hiding this comment.
Fail closed when the sandbox root cannot be resolved.
Line 66 silently falls back to "." if current_dir() fails. For a security boundary, that turns a startup/configuration error into an ambiguous relative sandbox root. Return an error here instead of weakening the sandbox.
Safer fallback
let work_dir = app_config
.work_dir
.clone()
- .unwrap_or_else(|| std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")));
+ .unwrap_or(std::env::current_dir().map_err(|e| {
+ Error::Other(format!("failed to resolve sandbox work_dir: {e}"))
+ })?);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let work_dir = app_config | |
| .work_dir | |
| .clone() | |
| .unwrap_or_else(|| std::env::current_dir().unwrap_or_else(|_| PathBuf::from("."))); | |
| let work_dir = app_config | |
| .work_dir | |
| .clone() | |
| .unwrap_or(std::env::current_dir().map_err(|e| { | |
| Error::Other(format!("failed to resolve sandbox work_dir: {e}")) | |
| })?); |
🤖 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 `@src/acp/mod.rs` around lines 63 - 66, The code currently falls back to
PathBuf::from(".") when std::env::current_dir() fails, weakening the sandbox
root; update the work_dir resolution in mod.rs so that if app_config.work_dir is
None you propagate the current_dir() error instead of defaulting to "." — change
the unwrap_or_else that constructs work_dir to return a Result (or use ? with
map/ok_or) and return an explicit error (with context) when current_dir() fails,
referencing the work_dir variable and app_config.work_dir to locate the change.
Comment on lines
+369
to
+371
| if let Some(wd) = self.work_dir { | ||
| app_config.work_dir = Some(wd); | ||
| } |
There was a problem hiding this comment.
Resolve work_dir before storing it as the sandbox boundary.
This copies the raw PathBuf into app_config, so a relative work_dir is only canonicalized later in src/tools/sandbox.rs at tool-execution time. If the host process changes cwd after build(), the effective sandbox root changes too. Normalize this to an absolute/canonical path during build and fail early if it is inaccessible.
🤖 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 `@src/client.rs` around lines 369 - 371, In build(), resolve and validate
self.work_dir before storing it into app_config.work_dir: if let Some(wd) =
self.work_dir { canonicalize (or std::fs::canonicalize) wd to an absolute path
(or join with std::env::current_dir then canonicalize), return an error from
build() on failure, and set app_config.work_dir = Some(canonicalized_path); do
not copy the raw PathBuf. This ensures the sandbox boundary is normalized and
fails early if the path is inaccessible.
Comment on lines
+421
to
+422
| work_dir: None, | ||
| allow_shell: true, |
There was a problem hiding this comment.
Programmatic builds still enable shell by default.
This keeps execute_command exposed for builder-only users unless they remember to call .allow_shell(false). That makes the safer behavior opt-out in the programmatic path as well. Align this with a secure default and require explicit opt-in for shell access here too.
🤖 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 `@src/client.rs` around lines 421 - 422, The builder currently sets
allow_shell: true making execute_command available by default; change the secure
default by initializing allow_shell to false in the builder/ClientBuilder (the
struct field named allow_shell) so shell access becomes explicit opt-in via
.allow_shell(true); update any related constructors or tests that assume the old
default and ensure execute_command remains gated by this field.
Comment on lines
+49
to
+56
| /// Whether to expose the `execute_command` shell tool to the LLM. | ||
| /// Defaults to `true`. Set to `false` to disable shell access entirely. | ||
| #[serde(default = "default_allow_shell")] | ||
| pub allow_shell: bool, | ||
| } | ||
|
|
||
| fn default_allow_shell() -> bool { | ||
| true |
There was a problem hiding this comment.
Default shell access should be opt-in.
With default_allow_shell() returning true, configs that omit this field still expose execute_command to the model. Since src/tools/mod.rs only removes that tool when allow_shell == false, this keeps the riskiest capability enabled by default and weakens the security posture this PR is adding. Default this to false and require an explicit opt-in instead.
🤖 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 `@src/config/types.rs` around lines 49 - 56, The default for shell access is
currently opt-out: change the default from true to false so models must be
explicitly opted in; update the function default_allow_shell() to return false
and ensure any docs/tests referencing allow_shell reflect the new default
behavior and that tools removal logic (which checks allow_shell) still functions
correctly with the new default.
Comment on lines
+31
to
+58
| let check = if resolved.exists() { | ||
| resolved.canonicalize().map_err(Error::IoError)? | ||
| } else { | ||
| // Walk up the tree until we find an existing ancestor, canonicalize | ||
| // that, and verify it is within the work directory. | ||
| let mut ancestor: &Path = &resolved; | ||
| loop { | ||
| ancestor = ancestor.parent().ok_or_else(|| { | ||
| Error::ToolExecutionError(format!( | ||
| "path '{}' has no accessible ancestor within the filesystem", | ||
| requested | ||
| )) | ||
| })?; | ||
| if ancestor.exists() { | ||
| let canonical_ancestor = ancestor.canonicalize().map_err(Error::IoError)?; | ||
| if !canonical_ancestor.starts_with(&work_dir_canonical) { | ||
| return Err(Error::ToolExecutionError(format!( | ||
| "path '{}' is outside the work directory '{}'", | ||
| requested, | ||
| work_dir.display() | ||
| ))); | ||
| } | ||
| // The non-existing tail of the path is fine; return it as-is | ||
| // so the caller (write_file) can create it. | ||
| return Ok(resolved); | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
Reject dangling symlink leaves before treating the path as “new.”
Path::exists() is false for a broken symlink, so a pre-existing dangling symlink inside work_dir falls into this branch and is accepted as a writable “new file.” write_file then follows that symlink and can create/overwrite a target outside the sandbox.
Suggested fix
let check = if resolved.exists() {
resolved.canonicalize().map_err(Error::IoError)?
} else {
+ match std::fs::symlink_metadata(&resolved) {
+ Ok(meta) if meta.file_type().is_symlink() => {
+ return Err(Error::ToolExecutionError(format!(
+ "path '{}' resolves through a symlink and cannot be written safely",
+ requested
+ )));
+ }
+ Ok(_) => {}
+ Err(err) if err.kind() == std::io::ErrorKind::NotFound => {}
+ Err(err) => return Err(Error::IoError(err)),
+ }
+
// Walk up the tree until we find an existing ancestor, canonicalize
// that, and verify it is within the work directory.
let mut ancestor: &Path = &resolved;
loop {Please add a regression test for a dangling symlink leaf under the work dir.
🤖 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 `@src/tools/sandbox.rs` around lines 31 - 58, The code currently treats a
broken symlink leaf as non-existent because it uses Path::exists(), letting
write_file follow and overwrite outside the sandbox; fix by calling
resolved.symlink_metadata() (instead of or before exists()) to detect if the
leaf is a symlink even when broken: if symlink_metadata.file_type().is_symlink()
then return Err(Error::ToolExecutionError(...)) rejecting the dangling symlink
leaf (include requested/work_dir in the message) so write_file cannot follow it;
update the logic around resolved, canonicalize, work_dir_canonical and
write_file accordingly and add a regression test that creates a dangling symlink
inside the work_dir and verifies it is rejected.
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 by CodeRabbit
New Features
allow_shellconfiguration and builder methodsDocumentation