feat(sandbox): add user-configurable read-only paths#542
feat(sandbox): add user-configurable read-only paths#542EZotoff wants to merge 3 commits intospacedriveapp:mainfrom
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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.
Pull request overview
Adds a new “middle ground” sandbox permission tier by introducing readable_paths (read-only allowlist) alongside existing writable_paths, aiming for OS-enforced read-only mounts plus tool-level write rejection.
Changes:
- Extend
SandboxConfigwithreadable_pathsand incorporate them into allowlist decisions. - Add
is_path_writable()and wire read-only paths into Linux bubblewrap mounts and macOS SBPL generation. - Refactor file tool path resolution to support “require writable” validation for mutating operations.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/sandbox.rs |
Adds readable_paths, updates allowlist logic, and attempts OS-level enforcement via bwrap/SBPL. |
src/tools/file.rs |
Refactors path resolution and introduces resolve_writable_path() for write enforcement (but has incorrect call-site usage). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| async fn call(&self, args: Self::Args) -> Result<Self::Output, Self::Error> { | ||
| let path = self.context.resolve_path(&args.path)?; | ||
| let path = self.context.resolve_writable_path(&args.path)?; |
There was a problem hiding this comment.
file_read is resolving paths with resolve_writable_path(), which will reject reads from SandboxConfig.readable_paths (read-only mounts). file_read should allow reading from read-only paths and only require writability for write/edit operations.
| let path = self.context.resolve_writable_path(&args.path)?; | |
| let path = self.context.resolve_readable_path(&args.path)?; |
src/sandbox.rs
Outdated
| for (index, path) in config.readable_paths.iter().enumerate() { | ||
| let canonical = canonicalize_or_self(path); | ||
| profile.push_str(&format!( | ||
| "; readable path {index}\n(allow file-read* (subpath \"{}\"))\n", |
There was a problem hiding this comment.
In the macOS SBPL profile generation, readable_paths only get a file-read* allow rule. If a readable_path overlaps with the workspace or any other writable allowlist, it will still be writable due to the broader file-write* allow rules. Add an explicit (deny file-write* (subpath ...)) for each readable path (similar to the existing data-dir deny) to make read-only mounts OS-enforced on macOS too.
| "; readable path {index}\n(allow file-read* (subpath \"{}\"))\n", | |
| "; readable path {index}\n(allow file-read* (subpath \"{}\"))\n(deny file-write* (subpath \"{}\"))\n", | |
| escape_sbpl_path(&canonical), |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/sandbox.rs (1)
1059-1064: Please add behavioral tests forreadable_paths.This only checks that the new field defaults to empty. The important contract is the read/write split: readable-only roots should pass
is_path_allowed(), failis_path_writable(), and the file tools should mirror that behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sandbox.rs` around lines 1059 - 1064, Add behavioral assertions to the SandboxConfig test to verify readable-only roots: in test_sandbox_config_defaults (or a new test) create a SandboxConfig with a path placed into SandboxConfig.readable_paths (and not in writable_paths), then assert that is_path_allowed(path) returns true, is_path_writable(path) returns false, and that the file access helpers used by the project (the file read tool succeeds and the file write tool fails or returns permission error) mirror that behavior; use existing symbols SandboxConfig, SandboxMode, readable_paths, is_path_allowed(), and is_path_writable() to locate where to add these checks and use temporary test files/dirs to exercise actual file-tool 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/sandbox.rs`:
- Around line 310-315: The readable-paths check in is_path_allowed currently
allows any prefix match from config.readable_paths; update the loop over
config.readable_paths (the allowed/canonical logic) to exclude matches that
would grant access into self.data_dir. Concretely, when iterating and computing
let allowed = path.canonicalize().unwrap_or_else(|_| path.clone()), only return
true if canonical.starts_with(&allowed) AND the candidate does not lie under
self.data_dir (e.g. !canonical.starts_with(&self.data_dir)) and also skip any
allowed that itself is inside self.data_dir (e.g.
!allowed.starts_with(&self.data_dir)); reference symbols: is_path_allowed,
config.readable_paths, self.data_dir, canonical, allowed.
In `@src/tools/file.rs`:
- Around line 335-336: The FileEditTool still calls resolve_path() leaving
readable_paths writable; update the FileEditTool call handler (the async fn call
implementation for FileEditTool) to use
context.resolve_writable_path(&args.path) instead of
context.resolve_path(&args.path), mirroring the change made in file_write;
ensure the returned Result propagates the same error when the path is not
writable so the read-only denial is enforced.
- Around line 228-229: The file_read tool's call method incorrectly uses
resolve_writable_path(&args.path), causing reads to fail when
Sandbox::is_path_writable() is false for readable-only roots; change the call in
the async fn call implementation to use resolve_path(&args.path) instead of
resolve_writable_path(&args.path) so reads use the read-only allowlist, and add
a regression test that exercises file_read against a readable-only root
(verifying resolve_path allows the read) to prevent future regressions.
---
Nitpick comments:
In `@src/sandbox.rs`:
- Around line 1059-1064: Add behavioral assertions to the SandboxConfig test to
verify readable-only roots: in test_sandbox_config_defaults (or a new test)
create a SandboxConfig with a path placed into SandboxConfig.readable_paths (and
not in writable_paths), then assert that is_path_allowed(path) returns true,
is_path_writable(path) returns false, and that the file access helpers used by
the project (the file read tool succeeds and the file write tool fails or
returns permission error) mirror that behavior; use existing symbols
SandboxConfig, SandboxMode, readable_paths, is_path_allowed(), and
is_path_writable() to locate where to add these checks and use temporary test
files/dirs to exercise actual file-tool behavior.
🪄 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
Run ID: 8326fb84-9e40-4f05-baa5-f7c922142d3a
📒 Files selected for processing (2)
src/sandbox.rssrc/tools/file.rs
There was a problem hiding this comment.
🧹 Nitpick comments (2)
prompts/en/cortex.md.j2 (1)
52-65: Decision memory "fields" will be stored as unstructured text.The prompt presents
decided_by,decision,rationale, andalternativesas if they are structured fields, but theMemoryschema only stores these in thecontentTEXT field. This means:
- The LLM must self-consistently format these attributes as prose/markdown in
content.- Memory consolidation and retrieval logic cannot query or filter by these "fields" programmatically.
- Future LLM calls parsing these memories may interpret the structure inconsistently.
Consider either:
- Clarifying in the prompt that these should be formatted as a structured block in
content(e.g., YAML/JSON snippet or markdown table), or- Extending the Memory schema to support typed decision metadata if this provenance data needs programmatic access.
The
importancethreshold (≥0.8) and association instructions (Updates,Contradicts) are valid—those fields/types exist in the schema.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@prompts/en/cortex.md.j2` around lines 52 - 65, The prompt describes Decision memory fields (`decided_by`, `decision`, `rationale`, `alternatives`, `importance`, and associations `Updates`/`Contradicts`) as if they are structured, but the Memory schema only stores TEXT in the `content` field; update the prompt text to require the LLM to render these attributes as a consistent structured block inside `content` (e.g., explicit YAML or JSON snippet or a fixed markdown section) and mention that retrieval cannot programmatically query separate columns, or alternatively note that the Memory schema must be extended to add typed decision metadata if programmatic filtering by `importance` or `decided_by` is required—include both options succinctly so implementers know to either enforce a serialized format in `content` or change the `Memory` schema.src/sandbox.rs (1)
328-330: Redundant condition check.At this point, line 305-307 already returned
falseifcanonical.starts_with(&data_dir_canonical). Code reaching line 329 is guaranteed to have!canonical.starts_with(&data_dir_canonical)evaluate totrue.♻️ Simplify to direct return
if canonical.starts_with(&allowed) { - return !canonical.starts_with(&data_dir_canonical); + return true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sandbox.rs` around lines 328 - 330, The conditional currently does `if canonical.starts_with(&allowed) { return !canonical.starts_with(&data_dir_canonical); }` but earlier code already returned false when `canonical.starts_with(&data_dir_canonical)`, so that negation is redundant; change the block that checks `canonical.starts_with(&allowed)` (in the function containing `canonical`, `allowed`, and `data_dir_canonical`) to simply return true (or just `return true`), removing the extra `starts_with(&data_dir_canonical)` check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@prompts/en/cortex.md.j2`:
- Around line 52-65: The prompt describes Decision memory fields (`decided_by`,
`decision`, `rationale`, `alternatives`, `importance`, and associations
`Updates`/`Contradicts`) as if they are structured, but the Memory schema only
stores TEXT in the `content` field; update the prompt text to require the LLM to
render these attributes as a consistent structured block inside `content` (e.g.,
explicit YAML or JSON snippet or a fixed markdown section) and mention that
retrieval cannot programmatically query separate columns, or alternatively note
that the Memory schema must be extended to add typed decision metadata if
programmatic filtering by `importance` or `decided_by` is required—include both
options succinctly so implementers know to either enforce a serialized format in
`content` or change the `Memory` schema.
In `@src/sandbox.rs`:
- Around line 328-330: The conditional currently does `if
canonical.starts_with(&allowed) { return
!canonical.starts_with(&data_dir_canonical); }` but earlier code already
returned false when `canonical.starts_with(&data_dir_canonical)`, so that
negation is redundant; change the block that checks
`canonical.starts_with(&allowed)` (in the function containing `canonical`,
`allowed`, and `data_dir_canonical`) to simply return true (or just `return
true`), removing the extra `starts_with(&data_dir_canonical)` check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1bcb1405-e83d-4658-a51a-ad41bc5c6643
📒 Files selected for processing (3)
prompts/en/cortex.md.j2src/sandbox.rssrc/tools/file.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/tools/file.rs
Add readable_paths to SandboxConfig for OS-enforced read-only mounts. Workers can read from these paths but cannot write or edit files there. Uses --ro-bind in bwrap (Linux) and SBPL allow file-read* rules (macOS). FileWriteTool and FileEditTool now use resolve_writable_path() which rejects write operations on read-only paths.
…ution, data_dir exclusion, macOS SBPL deny rules
EZotoff
force-pushed
the
feat/read-only-sandbox-paths
branch
from
bf37ec1 to
fa48c54
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/tools/file.rs (2)
228-229:⚠️ Potential issue | 🔴 CriticalDon't gate
file_readon writability.
Sandbox::is_path_writable()isfalsefor everyreadable_pathsprefix, so Line 229 blocks the exact read-only roots this PR adds.file_readshould resolve throughresolve_path(), with a regression test for reading from a readable-only root.Suggested fix
- let path = self.context.resolve_writable_path(&args.path)?; + let path = self.context.resolve_path(&args.path)?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/file.rs` around lines 228 - 229, The call implementation for file_read is using context.resolve_writable_path(&args.path) which incorrectly rejects readable-only roots; change it to use context.resolve_path(&args.path) (or the equivalent non-writable resolver) inside async fn call so reads are allowed on readable_paths, and add a regression test that creates a readable-only root and verifies file_read can read a file under that prefix; update any references to resolve_writable_path in the file_read code path to resolve_path and ensure error handling remains consistent.
335-336:⚠️ Potential issue | 🔴 CriticalMirror this writable-path guard in
file_edit.
FileEditToolstill callsresolve_path()at Line 421, so edits againstreadable_pathsbypass the new read-only denial. Switch that call toresolve_writable_path()too, and add a regression test that exercisesfile_editon a readable-only root.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/file.rs` around lines 335 - 336, FileEditTool still uses resolve_path which allows edits on read-only roots; change the call in FileEditTool (the async call implementation that currently invokes resolve_path for args.path) to use context.resolve_writable_path(&args.path) instead, and add a regression test that invokes the file_edit tool against a readable-only root to assert the operation is denied; reference the FileEditTool async call method, the resolve_path → resolve_writable_path replacement, and the file_edit behavior in the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/tools/file.rs`:
- Around line 228-229: The call implementation for file_read is using
context.resolve_writable_path(&args.path) which incorrectly rejects
readable-only roots; change it to use context.resolve_path(&args.path) (or the
equivalent non-writable resolver) inside async fn call so reads are allowed on
readable_paths, and add a regression test that creates a readable-only root and
verifies file_read can read a file under that prefix; update any references to
resolve_writable_path in the file_read code path to resolve_path and ensure
error handling remains consistent.
- Around line 335-336: FileEditTool still uses resolve_path which allows edits
on read-only roots; change the call in FileEditTool (the async call
implementation that currently invokes resolve_path for args.path) to use
context.resolve_writable_path(&args.path) instead, and add a regression test
that invokes the file_edit tool against a readable-only root to assert the
operation is denied; reference the FileEditTool async call method, the
resolve_path → resolve_writable_path replacement, and the file_edit behavior in
the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fb6880a2-2ad4-48c6-bbab-596a62c62c5a
📒 Files selected for processing (2)
src/sandbox.rssrc/tools/file.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/sandbox.rs
Summary
The sandbox currently supports
writable_pathsfor full access, but there's no middle ground for read-only access to specific paths. This PR addsreadable_pathstoSandboxConfigfor OS-enforced read-only mounts.Changes
src/sandbox.rsreadable_paths: Vec<PathBuf>toSandboxConfigis_path_allowed()to include readable pathsis_path_writable()method (returnsfalsefor read-only paths)--ro-bindmounts for readable paths in bwrap builder (Linux)(allow file-read*)SBPL rules for readable paths (macOS)prompt_read_allowlist()src/tools/file.rsresolve_path()intoresolve_path_inner(raw, require_writable)resolve_writable_path()that rejects write operations on read-only pathsFileWriteToolandFileEditToolnow useresolve_writable_path()"ACCESS DENIED: This path is mounted as read-only"on write attemptsConfig
Behavior
readable_pathswritable_pathsTesting
readable_pathsis empty — no behavior change for existing configs