From 9c4d04795c34e19ea6cb8f646b87b0ff65e3faa3 Mon Sep 17 00:00:00 2001 From: Richard Feldman Date: Mon, 9 Mar 2026 21:07:16 -0700 Subject: [PATCH] Add remaining code review items to code-review.md --- code-review.md | 157 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 157 insertions(+) create mode 100644 code-review.md diff --git a/code-review.md b/code-review.md new file mode 100644 index 0000000000000000000000000000000000000000..1fca13267ff9f1e50d10382adeee7d24f913760c --- /dev/null +++ b/code-review.md @@ -0,0 +1,157 @@ +# Remaining Code Review Items — `local-sandboxing` + +Items from the original review that have been addressed are not listed here. +Only items that are still present in the current code are included. + +--- + +## 1. `(allow signal)` is unrestricted on macOS + +**File:** `crates/terminal/src/sandbox_macos.rs`, line ~54 + +The SBPL profile contains a bare `(allow signal)` which permits the sandboxed +process to send signals (including SIGKILL) to any process owned by the same +user. This should be scoped: + +``` +(allow signal (target self)) +``` + +or at minimum `(target children)` if child-process signaling is needed. + +--- + +## 2. Dotfile lists are incomplete + +**Files:** `crates/terminal/src/sandbox_macos.rs` and `sandbox_linux.rs` + +The hardcoded dotfile lists cover zsh, bash, and a few generic files but miss: + +- **Shell history files** (`.bash_history`, `.zsh_history`) — if the shell + can't write history, users will get silent failures or error messages on + every command. Read-write access to these is likely needed. +- **Fish shell** — fish config lives in `~/.config/fish/`, which is partially + covered by the `~/.config` subpath rule but only if `~/.config` exists. +- **Nushell, PowerShell, elvish** — no coverage at all. + +The lists are also in different orders between the two files, adding +maintenance overhead for no benefit. + +**Fix:** Extract the dotfile list to a shared constant (e.g., on +`SandboxConfig`) so both platform implementations use the same list. Consider +adding history files with read-write access rather than read-only. + +--- + +## 3. `/proc/self` only gets read access on Linux + +**File:** `crates/terminal/src/sandbox_linux.rs`, line ~143 + +Bash process substitution (e.g., `<(command)`) creates FIFOs under +`/proc/self/fd/`. These FIFOs need write access — the shell writes to them. +The current `fs_read()` permission may cause process substitution to fail. + +**Fix:** Grant `fs_all()` (or at least read+write) on `/proc/self` instead of +`fs_read()`. + +--- + +## 4. `current_exe()` failure silently falls back to `"zed"` + +**File:** `crates/terminal/src/terminal.rs`, line ~553 + +```rust +let zed_binary = std::env::current_exe().unwrap_or_else(|_| PathBuf::from("zed")); +``` + +If `current_exe()` fails, this falls back to `"zed"` which relies on PATH +lookup. Inside a sandbox where PATH is restricted, this could fail in +confusing ways — the user would see a "command not found" error with no +indication that the sandbox wrapper binary couldn't be located. + +**Fix:** Propagate the error with `?` instead of silently falling back. The +terminal builder already returns `Result`. + +--- + +## 5. Duplicated sandbox config resolution logic + +**Files:** `crates/project/src/terminals.rs` (~lines 413-435) and +`crates/acp_thread/src/terminal.rs` (~lines 245-275) + +The sandbox config resolution (check feature flag → check enabled → check +`apply_to` → fallback project dir → build config) is duplicated verbatim +across the user-terminal and agent-terminal code paths. The only meaningful +differences are: + +- Which `SandboxApplyTo` variant to match (`Terminal` vs `Tool`) +- Where the project directory comes from + +**Fix:** Extract into a shared helper, e.g.: + +```rust +impl SandboxConfig { + pub fn resolve_if_enabled( + sandbox_settings: &SandboxSettingsContent, + target: SandboxApplyTo, + project_dir: PathBuf, + cx: &App, + ) -> Option { ... } +} +``` + +--- + +## 6. `let _ = write!(...)` suppresses errors + +**File:** `crates/terminal/src/sandbox_macos.rs`, lines ~191 and ~223 + +The project `.rules` say: "Never silently discard errors with `let _ =` on +fallible operations." While `write!` to a `String` is infallible in practice +(the `fmt::Write` impl for `String` cannot fail), the pattern still violates +the rule. + +**Fix:** Use `write!(...).unwrap()` (justified since `String` fmt::Write is +infallible) or restructure to use `push_str` + `format!`. + +--- + +## 7. No test for `additional_executable_paths` + +**File:** `crates/terminal/src/sandbox_tests.rs` + +There are integration tests for `additional_read_write_paths` and +`additional_read_only_paths`, but not for `additional_executable_paths`. A +test should verify that a binary placed in an additional executable path can +actually be executed by the sandboxed shell, and that binaries outside all +allowed paths cannot. + +--- + +## 8. No test for `canonicalize_paths()` with symlinks + +**File:** `crates/terminal/src/sandbox_tests.rs` + +The `canonicalize_paths` function is exercised indirectly (the test helper +calls it), but no test explicitly verifies that a symlinked project directory +or additional path is resolved before being added to sandbox rules. A test +could create a symlink to a temp directory, use it as the project dir, and +verify the sandbox enforces on the real path. + +--- + +## 9. macOS: `$TMPDIR` grants broad access via `/var/folders` + +**File:** `crates/terminal/src/terminal_settings.rs` (default read-write +paths) + +The default macOS read-write paths include `/private/var/folders`, which is +the parent of every user's per-session temp directory. This means the sandbox +grants read-write access to all temp files on the system, not just the +current user's. + +A tighter approach would resolve `$TMPDIR` at spawn time (which gives the +per-user, per-session temp directory like +`/private/var/folders/xx/xxxx/T/`) and only allow that specific +subdirectory. This would still let the shell use temp files but prevent +access to other users' temp directories.