From 2b01744616506c9de7ff09f8dcf4ff231cbdffb6 Mon Sep 17 00:00:00 2001 From: xintaofei Date: Sat, 25 Jul 2026 13:10:58 +0800 Subject: [PATCH] fix(acp): let agents reach their own files outside the workspace The ACP `fs/read_text_file` / `fs/write_text_file` handlers confined every path to the session working directory, with no way to configure it. Agents that keep state outside any workspace could not use the channel codeg advertises to them: Grok's plan mode writes `/sessions///plan.md` and reads `/skills`, so those calls failed and Grok fell back to writing the file through a shell heredoc instead. - Replace the single hardcoded root with an `FsAccessPolicy`. Reads are unrestricted by default; writes are limited to the workspace, the agent's own data home, the temp dirs, and any user-listed extra roots. The 16MB/2MB size caps and the parent-directory rules are unchanged. - Add `CODEG_ACP_FS_POLICY` (`default` | `strict` | `unrestricted`) and `CODEG_ACP_FS_EXTRA_ROOTS`. Both resolve from the agent's `env_json` before codeg's own environment, so they can be set per agent from the existing settings UI; `strict` restores the previous behavior. - Derive each agent's data home from a declarative table that mirrors the resolvers in `parsers/`, carrying every relocation variable in its resolver's precedence order plus the sub-path and trim semantics that resolver applies. A test pins the table against those resolvers so the two cannot drift. - Track the environment the launched child actually sees: a non-blank `runtime_env` value replaces the parent's, a blank one is removed by the spawn layer so the agent falls back to its own default, and an absent one is inherited. Home-relative defaults follow the child's `HOME` / `USERPROFILE` rather than codeg's, so an isolated profile stays isolated and the inactive one is not writable. - Refuse a root that cannot be named safely instead of guessing: relative values are never resolved against codeg's cwd, and values are never trimmed or tilde-expanded, either of which could widen a root to `$HOME` or `/`. Containment is still checked on the canonicalized target, so a symlink leading out of an allowed root is rejected. --- src-tauri/src/acp/connection.rs | 14 +- src-tauri/src/acp/file_system_runtime.rs | 1576 +++++++++++++++++++++- 2 files changed, 1528 insertions(+), 62 deletions(-) diff --git a/src-tauri/src/acp/connection.rs b/src-tauri/src/acp/connection.rs index b5f5d7809..104d3a4c4 100644 --- a/src-tauri/src/acp/connection.rs +++ b/src-tauri/src/acp/connection.rs @@ -31,7 +31,7 @@ use tokio::sync::{mpsc, RwLock}; use crate::acp::background_watch; use crate::acp::error::AcpError; -use crate::acp::file_system_runtime::{FileSystemRuntime, FileSystemRuntimeError}; +use crate::acp::file_system_runtime::{FileSystemRuntime, FileSystemRuntimeError, FsAccessPolicy}; use crate::acp::registry::{self, AgentDistribution}; use crate::acp::session_state::SessionState; use crate::acp::terminal_runtime::{TerminalRuntime, TerminalRuntimeError}; @@ -852,6 +852,13 @@ pub async fn spawn_agent_connection( let launch_cwd = resolve_working_dir(working_dir.as_deref()); let agent = build_agent(agent_type, &runtime_env, &launch_cwd).await?; + // Path policy for the ACP `fs/*` channel. Built HERE rather than inside + // `run_connection` because it needs the full `runtime_env` (only the git + // credential keys survive into `terminal_base_env` below), and a per-agent + // relocation like `GROK_HOME` must move the allowed root along with the + // agent's state. Uses the same `launch_cwd` the process and ACP session get. + let fs_policy = FsAccessPolicy::from_env(&launch_cwd, agent_type, &runtime_env); + // Forward only the codeg git credential helper keys into the terminal // runtime — not the agent's API tokens or model provider credentials. // This makes `git fetch`/`git push` issued through the ACP @@ -940,6 +947,7 @@ pub async fn spawn_agent_connection( preferred_mode_id, preferred_config_values, delegation_injection, + fs_policy, ) .await; @@ -2466,6 +2474,7 @@ async fn run_connection( preferred_mode_id: Option, preferred_config_values: BTreeMap, delegation_injection: Option, + fs_policy: FsAccessPolicy, ) -> Result<(), AcpError> { let pending_perms: PendingPermissions = Arc::new(tokio::sync::Mutex::new(HashMap::new())); // `terminal_base_env` already filtered to just the credential helper @@ -2479,7 +2488,8 @@ async fn run_connection( TerminalRuntime::with_base_env(terminal_base_env).with_default_cwd(Some(cwd.clone())), ); let cwd_string = cwd.to_string_lossy().to_string(); - let file_system_runtime = Arc::new(FileSystemRuntime::new(cwd.clone())); + tracing::info!("[ACP] fs policy {}", fs_policy.describe()); + let file_system_runtime = Arc::new(FileSystemRuntime::with_policy(fs_policy)); let conn_id = connection_id.clone(); let emitter_clone = emitter.clone(); diff --git a/src-tauri/src/acp/file_system_runtime.rs b/src-tauri/src/acp/file_system_runtime.rs index 04d96c785..2524f1479 100644 --- a/src-tauri/src/acp/file_system_runtime.rs +++ b/src-tauri/src/acp/file_system_runtime.rs @@ -1,3 +1,5 @@ +use std::collections::BTreeMap; +use std::ffi::OsString; use std::fs::{File, OpenOptions}; use std::io::{BufRead, BufReader, ErrorKind, Write}; use std::path::{Path, PathBuf}; @@ -9,6 +11,8 @@ use sacp::schema::{ }; use tokio::sync::Semaphore; +use crate::models::agent::AgentType; + const FS_MAX_CONCURRENT_OPS: usize = 8; const FS_IO_TIMEOUT: Duration = Duration::from_secs(30); const FS_MAX_FILE_SIZE_BYTES: u64 = 16 * 1024 * 1024; @@ -31,27 +35,538 @@ impl FileSystemRuntimeError { } } +/// Per-agent `env_json` / process-env key selecting the path-containment policy +/// for the ACP `fs/*` channel. `default` | `strict` | `unrestricted`; anything +/// else warns and falls back to `default`. +pub(crate) const FS_POLICY_ENV: &str = "CODEG_ACP_FS_POLICY"; +/// Additional **writable** roots, joined with the platform `PATH` separator +/// (`:` on unix, `;` on Windows — parsed with `std::env::split_paths`, so a +/// Windows drive letter's colon is not mistaken for a separator). +pub(crate) const FS_EXTRA_ROOTS_ENV: &str = "CODEG_ACP_FS_EXTRA_ROOTS"; + +/// Which paths the ACP `fs/read_text_file` / `fs/write_text_file` handlers will +/// serve. +/// +/// An EMPTY root list means "unrestricted" for that direction — not "deny +/// everything". Reads default to unrestricted because the gate buys no safety: +/// codeg also advertises `terminal(true)`, so an agent that is refused a read +/// simply `cat`s the file through a shell instead (empirically what grok does — +/// it fell back to `run_terminal_command` with a `cat > … << 'PLAN_EOF'` +/// heredoc, re-sending the whole payload as a shell command). Writes keep a +/// root list mirroring the agents' own sandbox model (grok's `workspace` +/// profile: CWD + its own home + temp dirs). +/// NOTE: deliberately NOT `Default` — an all-empty policy means "unrestricted", +/// so a derived `Default` would let `..Default::default()` or a stray +/// `FsAccessPolicy::default()` silently open up writes. Construct through the +/// named constructors instead. +#[derive(Clone, Debug)] +pub struct FsAccessPolicy { + /// Roots a read may resolve into. Empty ⇒ unrestricted. + read_roots: Vec, + /// Roots a write may land in. Empty ⇒ unrestricted. + write_roots: Vec, +} + +impl FsAccessPolicy { + /// Today's historical behavior: reads and writes both confined to the + /// session working directory. Reachable via `CODEG_ACP_FS_POLICY=strict`. + pub fn strict(workspace_root: &Path) -> Self { + let roots = vec![canonical_root(workspace_root)]; + Self { + read_roots: roots.clone(), + write_roots: roots, + } + } + + /// No path gate at all in either direction. `CODEG_ACP_FS_POLICY=unrestricted`. + pub fn unrestricted() -> Self { + Self { + read_roots: Vec::new(), + write_roots: Vec::new(), + } + } + + /// The default: unrestricted reads; writes confined to the workspace, the + /// agent's own data home (so plan files, skills and session state work), + /// the temp dirs (attachments), and any `CODEG_ACP_FS_EXTRA_ROOTS` entries. + pub fn permissive( + workspace_root: &Path, + agent_type: AgentType, + runtime_env: &BTreeMap, + ) -> Self { + let mut write_roots = vec![canonical_root(workspace_root)]; + for root in agent_data_roots(agent_type, runtime_env) { + write_roots.push(canonical_root(&root)); + } + for root in temp_roots() { + write_roots.push(canonical_root(&root)); + } + for root in extra_write_roots(runtime_env) { + write_roots.push(canonical_root(&root)); + } + write_roots.sort(); + write_roots.dedup(); + + Self { + read_roots: Vec::new(), + write_roots, + } + } + + /// Resolve the policy for a connection from `CODEG_ACP_FS_POLICY`, checking + /// the agent's `runtime_env` first (so it can be set per agent through the + /// existing agent-settings `env_json`) then codeg's own process env. + pub fn from_env( + workspace_root: &Path, + agent_type: AgentType, + runtime_env: &BTreeMap, + ) -> Self { + match env_value(runtime_env, FS_POLICY_ENV) + .as_deref() + .map(str::trim) + { + None | Some("") | Some("default") => { + Self::permissive(workspace_root, agent_type, runtime_env) + } + Some("strict") => Self::strict(workspace_root), + Some("unrestricted") => Self::unrestricted(), + Some(other) => { + tracing::warn!( + "[ACP] unrecognized {FS_POLICY_ENV}={other:?}; falling back to \"default\"" + ); + Self::permissive(workspace_root, agent_type, runtime_env) + } + } + } + + /// One-line summary for the connection log. + pub fn describe(&self) -> String { + fn render(roots: &[PathBuf]) -> String { + if roots.is_empty() { + "unrestricted".to_string() + } else { + roots + .iter() + .map(|p| p.display().to_string()) + .collect::>() + .join(", ") + } + } + format!( + "read={} write={}", + render(&self.read_roots), + render(&self.write_roots) + ) + } +} + +/// Read a knob VERBATIM from the agent's `runtime_env` first, then codeg's +/// process env. Used where the value is a path (or path list) and must not be +/// normalized: trimming `" / "` into `/` would turn a relative entry into the +/// filesystem root. +fn raw_env_value(runtime_env: &BTreeMap, key: &str) -> Option { + match runtime_env.get(key) { + Some(value) if value.is_empty() => None, + Some(value) => Some(OsString::from(value)), + None => std::env::var_os(key).filter(|value| !value.is_empty()), + } +} + +/// Read a knob from the agent's `runtime_env` first, then codeg's process env, +/// trimmed — for values compared as keywords, never as paths. +/// +/// Mirrors `PI_ACP_TRUST_WORKSPACE` (`commands::acp`): a codeg-only key that +/// rides along in the per-agent `env_json`, so it is configurable per agent +/// from the existing settings UI without a new surface. +fn env_value(runtime_env: &BTreeMap, key: &str) -> Option { + runtime_env + .get(key) + .map(|raw| raw.trim().to_string()) + .filter(|value| !value.is_empty()) + .or_else(|| { + std::env::var(key) + .ok() + .map(|raw| raw.trim().to_string()) + .filter(|value| !value.is_empty()) + }) +} + +/// Extra writable roots from `CODEG_ACP_FS_EXTRA_ROOTS`. Relative entries are +/// dropped for the same reason as relative agent homes: they would be resolved +/// against codeg's cwd, so `.` or `..` silently widens the policy to an +/// unrelated tree (or to `/`). Dropping is fail-closed. +/// +/// The list is read VERBATIM — `split_paths` does not trim entries, so trimming +/// the value first would rewrite a relative `" / "` entry into `/` and hand out +/// the whole filesystem. An entry that IS exactly `/` is still honored: widening +/// is this knob's declared purpose, and then it is the user's explicit choice. +fn extra_write_roots(runtime_env: &BTreeMap) -> Vec { + let Some(raw) = raw_env_value(runtime_env, FS_EXTRA_ROOTS_ENV) else { + return Vec::new(); + }; + + std::env::split_paths(&raw) + .filter(|path| !path.as_os_str().is_empty()) + .filter(|path| { + if path.is_absolute() { + return true; + } + tracing::warn!( + "[ACP] ignoring relative {FS_EXTRA_ROOTS_ENV} entry {}; use an absolute path", + path.display() + ); + false + }) + .collect() +} + +/// Temp roots an agent legitimately writes through the `fs/*` channel (image +/// attachments, scratch files). `std::env::temp_dir()` is the per-user one +/// (`/var/folders/…` on macOS, which canonicalizes to `/private/var/folders/…` +/// — hence `canonical_root` on every entry). +fn temp_roots() -> Vec { + let mut roots = vec![std::env::temp_dir()]; + #[cfg(unix)] + { + roots.push(PathBuf::from("/tmp")); + roots.push(PathBuf::from("/var/tmp")); + } + roots +} + +/// The agent's own data home — where it keeps session state, plan files and +/// skills. Writes there are part of normal operation, not an escape: +/// grok's plan mode writes `/sessions///plan.md` and reads +/// `/skills/*/SKILL.md`, both outside any workspace. +/// +/// Reuses the per-agent resolvers in `crate::parsers` (the same ones +/// `external_transcript_sources` uses) so codeg has exactly ONE notion of where +/// each agent lives. Where the agent's home is relocatable, the agent's +/// `runtime_env` wins over codeg's process env — a per-agent `GROK_HOME` in +/// `env_json` moves the agent's state, so it must move the allowed root too. +/// Same shape as `commands::acp::pi_agent_dir_for_env`. +fn agent_data_roots(agent_type: AgentType, runtime_env: &BTreeMap) -> Vec { + agent_root_slots(agent_type) + .iter() + .filter_map(|slot| resolve_root_slot(slot, runtime_env)) + .collect() +} + +/// One independently-relocatable directory belonging to an agent. +/// +/// Modelled declaratively because three separate review findings were all the +/// same mistake — hand-rolling one agent's env semantics slightly differently +/// from its own resolver. +struct RootSlot { + /// Ordered `(env key, sub-path appended to that key's value)` candidates, in + /// the agent resolver's OWN precedence order. First one that reaches the + /// child wins. An empty sub-path means the value IS the root. + candidates: &'static [(&'static str, &'static str)], + /// Home-relative default when none of the candidates reaches the child. + default_rel: &'static [&'static str], + /// Whether the agent's own resolver TRIMS the value before using it. Hermes + /// (`resolve_hermes_home` / `hermes_home_for_launch`) and Cline + /// (`cline_data_dir`) do, so a whitespace-only value is genuinely unset for + /// them; every other resolver takes the raw `OsString` and only rejects an + /// empty one, so trimming on their behalf would misreport the child's path. + trims: bool, +} + +/// Resolve one slot to the directory the LAUNCHED agent will actually use, or +/// `None` when that directory cannot be named safely. +/// +/// The value is taken VERBATIM apart from the resolver's sub-path — no tilde +/// expansion, because the launched CLIs treat `~` literally (see +/// `commands::acp::hermes_home_for_launch`: "a non-empty value is used VERBATIM +/// … Hermes does NOT expand `~`"). Expanding it here would invent `$HOME` as a +/// writable root the user never selected. +fn resolve_root_slot(slot: &RootSlot, runtime_env: &BTreeMap) -> Option { + for (key, suffix) in slot.candidates { + // Absent for the child ⇒ try the next candidate, exactly as the agent's + // own resolver would. + let Some(value) = child_env_value(runtime_env, key, slot.trims) else { + continue; + }; + + // A RELATIVE value yields NO root for this slot — and must not fall + // through to a lower-precedence candidate or the default. + // + // Absolutizing it would resolve against CODEG's cwd while the child runs + // in the workspace cwd, and that mismatch WIDENS rather than merely + // mismatching: `CODEX_HOME="."` with codeg launched from `/` yields the + // root `/`, which every absolute path passes `starts_with` against. + // Falling through would be wrong too — the child DID receive this value + // and uses it, so the default (`~/.codex`) is an inactive profile holding + // config and credentials, exactly the tree the isolation invariant keeps + // unwritable. So: refuse the slot. That is fail-closed (the agent's write + // is rejected), and the user can still name the directory explicitly via + // `CODEG_ACP_FS_EXTRA_ROOTS`. + let base = PathBuf::from(value); + if !base.is_absolute() { + tracing::warn!( + "[ACP] relative {key}={} cannot be used as an fs write root; \ + the agent's own directory is NOT writable this launch. \ + Use an absolute path (or {FS_EXTRA_ROOTS_ENV}).", + base.display() + ); + return None; + } + + return Some(if suffix.is_empty() { + base + } else { + base.join(suffix) + }); + } + + // No relocation reaches the child, so it falls back to a home-relative + // default — resolved against the CHILD's home, not codeg's (see + // `child_home_dir`). + let mut root = child_home_dir(runtime_env)?; + for segment in slot.default_rel { + root.push(segment); + } + Some(root) +} + +/// The home directory the CHILD resolves its defaults against. +/// +/// `merge_agent_env` copies EVERY `runtime_env` entry into the child — `HOME` +/// included, and overriding it is the classic way to isolate a CLI's config — so +/// the home-relative defaults must follow the child's home. Using codeg's would +/// both grant the inactive profile (config/credentials the session never opens) +/// and refuse the active one. +/// +/// `None` when the child's home cannot be named as an absolute path, in which +/// case the caller refuses the slot rather than guessing at a root. +fn child_home_dir(runtime_env: &BTreeMap) -> Option { + // `dirs::home_dir()` ignores `$HOME` on Windows, where the home var the + // agents actually read is `USERPROFILE`. + #[cfg(windows)] + const HOME_KEY: &str = "USERPROFILE"; + #[cfg(not(windows))] + const HOME_KEY: &str = "HOME"; + + // The three states are NOT interchangeable, and collapsing the last two is a + // real bug: `dirs::home_dir()` consults CODEG's `$HOME` before falling back to + // the passwd entry, so it answers for codeg, not for a child whose `HOME` was + // removed. + let home = match runtime_env.get(HOME_KEY) { + // Explicitly REMOVED (blank ⇒ `env_remove`): the child resolves its home + // from the OS account database, which we cannot read without duplicating a + // passwd lookup — and codeg's `$HOME` is NOT that answer whenever the two + // differ. Refuse rather than guess; the user can still name the directory + // via `CODEG_ACP_FS_EXTRA_ROOTS`. + Some(value) if value.is_empty() => { + tracing::warn!( + "[ACP] {HOME_KEY} is removed for this launch, so the agent's own \ + directory cannot be located and is NOT writable; \ + set {FS_EXTRA_ROOTS_ENV} if it needs to be" + ); + return None; + } + // Overridden: the child sees exactly this. + Some(value) => PathBuf::from(value), + // Absent: the child inherits codeg's environment, so codeg's answer is + // exact — including its own passwd fallback when codeg has no `HOME`. + None => { + // On Windows the agents' home also derives from `HOMEDRIVE` + + // `HOMEPATH` when `USERPROFILE` is absent (Python's rule, which + // Hermes follows). If the launch relocates that pair without + // relocating `USERPROFILE`, codeg's answer may not be the child's, so + // fail closed instead of guessing. + #[cfg(windows)] + if runtime_env.contains_key("HOMEDRIVE") || runtime_env.contains_key("HOMEPATH") { + tracing::warn!( + "[ACP] HOMEDRIVE/HOMEPATH are relocated without {HOME_KEY}; \ + the agent's own directory cannot be located and is NOT writable" + ); + return None; + } + dirs::home_dir()? + } + }; + + // `dirs` accepts a relative `HOME` on unix, and a relative home would be + // absolutized against codeg's cwd — the same widening as a relative agent + // home, so refuse it the same way. + if !home.is_absolute() { + tracing::warn!( + "[ACP] relative {HOME_KEY}={} cannot anchor the fs write roots; \ + the agent's own directory is NOT writable this launch", + home.display() + ); + return None; + } + + Some(home) +} + +/// What the LAUNCHED child will see for `key`, which is NOT simply +/// "runtime_env else our env": +/// +/// * non-blank in `runtime_env` — `merge_agent_env` gives `runtime_env` the +/// highest precedence, so this REPLACES the parent's value in the child. +/// * blank in `runtime_env` — the vendored spawn layer treats an empty value as +/// `env_remove` ("an empty value means ensure this var is ABSENT from the +/// child", `vendor/sacp-tokio/src/acp_agent.rs`). The child therefore does NOT +/// inherit our value; the agent falls back to its own default. Reading through +/// to our process env here would point the root at a profile the child never +/// opens, re-breaking the writes this change exists to allow. +/// * absent from `runtime_env` — the child inherits our environment. +/// +/// The value is returned VERBATIM. Only an EXACTLY empty string counts as +/// removed, because that is the precise test the spawn layer applies +/// (`if env_var.value.is_empty() { env_remove }`) — a whitespace-only value is +/// passed through to the child untouched. Trimming here would both under-include +/// (treating `" "` as unset when the child receives it) and, worse, WIDEN: +/// `CODEX_HOME=" / "` reaches the child as a *relative* path, but trimmed it +/// becomes `/` and would make the entire filesystem writable. +/// +/// `trims` opts into the normalization the agent's OWN resolver performs: Hermes +/// and Cline trim their value (so a whitespace-only value really is unset for +/// them), while the rest take the raw `OsString` and only reject an empty one. +/// Inherited values go through `var_os` so a non-UTF-8 path is not silently +/// dropped. +fn child_env_value( + runtime_env: &BTreeMap, + key: &str, + trims: bool, +) -> Option { + let raw = match runtime_env.get(key) { + // Exactly empty ⇒ the spawn layer `env_remove`s it. + Some(value) if value.is_empty() => return None, + Some(value) => OsString::from(value), + None => std::env::var_os(key)?, + }; + + if trims { + let trimmed = raw.to_string_lossy().trim().to_string(); + return (!trimmed.is_empty()).then(|| OsString::from(trimmed)); + } + + (!raw.is_empty()).then_some(raw) +} + +/// Every agent's relocatable directories, mirroring the `resolve_*_from` bodies +/// in `crate::parsers` (pinned by `root_slots_match_parser_resolvers`). +/// +/// A wrong sub-path is not cosmetic: dropping Gemini's `.gemini` widens the root +/// by a whole level (to the bare home directory when `GEMINI_CLI_HOME` is the +/// home dir), and omitting a key narrows it so the agent's real directory is +/// rejected again. +fn agent_root_slots(agent_type: AgentType) -> &'static [RootSlot] { + match agent_type { + AgentType::Grok => &[RootSlot { + candidates: &[("GROK_HOME", "")], + trims: false, + default_rel: &[".grok"], + }], + AgentType::ClaudeCode => &[RootSlot { + candidates: &[("CLAUDE_CONFIG_DIR", "")], + trims: false, + default_rel: &[".claude"], + }], + AgentType::Codex => &[RootSlot { + candidates: &[("CODEX_HOME", "")], + trims: false, + default_rel: &[".codex"], + }], + // `resolve_gemini_base_dir_from` joins `.gemini` onto GEMINI_CLI_HOME. + AgentType::Gemini => &[RootSlot { + candidates: &[("GEMINI_CLI_HOME", ".gemini")], + trims: false, + default_rel: &[".gemini"], + }], + AgentType::CodeBuddy => &[RootSlot { + candidates: &[("CODEBUDDY_CONFIG_DIR", "")], + trims: false, + default_rel: &[".codebuddy"], + }], + AgentType::KimiCode => &[RootSlot { + candidates: &[("KIMI_CODE_HOME", "")], + trims: false, + default_rel: &[".kimi-code"], + }], + // `resolve_cursor_config_from` prefers CURSOR_CONFIG_DIR verbatim and + // only then `/cursor` — order matters. + AgentType::Cursor => &[RootSlot { + candidates: &[("CURSOR_CONFIG_DIR", ""), ("XDG_CONFIG_HOME", "cursor")], + trims: false, + default_rel: &[".cursor"], + }], + AgentType::Hermes => &[RootSlot { + candidates: &[("HERMES_HOME", "")], + trims: true, + default_rel: &[".hermes"], + }], + // `resolve_opencode_base_dir` is `/opencode`. + AgentType::OpenCode => &[RootSlot { + candidates: &[("XDG_DATA_HOME", "opencode")], + trims: false, + default_rel: &[".local", "share", "opencode"], + }], + AgentType::Cline => &[RootSlot { + candidates: &[("CLINE_DIR", "")], + trims: true, + default_rel: &[".cline", "data"], + }], + AgentType::OpenClaw => &[RootSlot { + candidates: &[], + trims: false, + default_rel: &[".openclaw"], + }], + // pi's agent home and its session store relocate INDEPENDENTLY, so both + // are genuine roots rather than alternatives. The sessions slot mirrors + // `resolve_pi_sessions_dir_from`: the session override wins, else + // `/sessions`, else `~/.pi/agent/sessions`. + AgentType::Pi => &[ + RootSlot { + candidates: &[("PI_CODING_AGENT_DIR", "")], + trims: false, + default_rel: &[".pi", "agent"], + }, + RootSlot { + candidates: &[ + ("PI_CODING_AGENT_SESSION_DIR", ""), + ("PI_CODING_AGENT_DIR", "sessions"), + ], + trims: false, + default_rel: &[".pi", "agent", "sessions"], + }, + ], + } +} + +/// Absolutize + canonicalize a configured root. Falls back to the absolutized +/// raw path when the directory does not exist yet (a non-existent root simply +/// never matches, which is the correct outcome). +fn canonical_root(root: &Path) -> PathBuf { + let absolute = if root.is_absolute() { + root.to_path_buf() + } else { + std::env::current_dir().unwrap_or_default().join(root) + }; + std::fs::canonicalize(&absolute).unwrap_or(absolute) +} + #[derive(Clone)] pub struct FileSystemRuntime { - workspace_root: PathBuf, - workspace_root_canonical: Option, + policy: Arc, io_semaphore: Arc, } impl FileSystemRuntime { + /// Confine both directions to `workspace_root`. Kept as the `new` shape + /// because that is what the historical constructor meant; production goes + /// through `with_policy`. pub fn new(workspace_root: PathBuf) -> Self { - let workspace_root = if workspace_root.is_absolute() { - workspace_root - } else { - std::env::current_dir() - .unwrap_or_default() - .join(workspace_root) - }; - let workspace_root_canonical = std::fs::canonicalize(&workspace_root).ok(); + Self::with_policy(FsAccessPolicy::strict(&workspace_root)) + } + pub fn with_policy(policy: FsAccessPolicy) -> Self { Self { - workspace_root, - workspace_root_canonical, + policy: Arc::new(policy), io_semaphore: Arc::new(Semaphore::new(FS_MAX_CONCURRENT_OPS)), } } @@ -80,8 +595,7 @@ impl FileSystemRuntime { FileSystemRuntimeError::Internal("filesystem runtime closed".to_string()) })?; - let workspace_root = self.workspace_root.clone(); - let workspace_root_canonical = self.workspace_root_canonical.clone(); + let policy = self.policy.clone(); let path = request.path; let line = request.line; let limit = request.limit; @@ -89,14 +603,8 @@ impl FileSystemRuntime { let path_for_log = path.clone(); let response = run_blocking_with_timeout("fs/read_text_file", move || { - read_text_file_impl( - &path, - line, - limit, - &workspace_root, - workspace_root_canonical.as_deref(), - ) - .map(ReadTextFileResponse::new) + read_text_file_impl(&path, line, limit, &policy.read_roots) + .map(ReadTextFileResponse::new) }) .await; @@ -130,20 +638,14 @@ impl FileSystemRuntime { FileSystemRuntimeError::Internal("filesystem runtime closed".to_string()) })?; - let workspace_root = self.workspace_root.clone(); - let workspace_root_canonical = self.workspace_root_canonical.clone(); + let policy = self.policy.clone(); let path = request.path; let content = request.content; let started_at = Instant::now(); let path_for_log = path.clone(); let response = run_blocking_with_timeout("fs/write_text_file", move || { - ensure_path_in_workspace( - &path, - &workspace_root, - workspace_root_canonical.as_deref(), - true, - )?; + ensure_path_allowed(&path, &policy.write_roots, true)?; atomic_write_text(&path, content.as_bytes())?; Ok(WriteTextFileResponse::new()) }) @@ -183,10 +685,9 @@ fn read_text_file_impl( path: &Path, line: Option, limit: Option, - workspace_root: &Path, - workspace_root_canonical: Option<&Path>, + read_roots: &[PathBuf], ) -> Result { - ensure_path_in_workspace(path, workspace_root, workspace_root_canonical, false)?; + ensure_path_allowed(path, read_roots, false)?; let metadata = std::fs::metadata(path).map_err(|err| map_io_error("read", path, err))?; if metadata.len() > FS_MAX_FILE_SIZE_BYTES { @@ -355,30 +856,34 @@ fn sync_directory(_path: &Path) -> Result<(), FileSystemRuntimeError> { Ok(()) } -fn canonical_workspace_root(workspace_root: &Path, canonical: Option<&Path>) -> PathBuf { - canonical - .map(Path::to_path_buf) - .or_else(|| std::fs::canonicalize(workspace_root).ok()) - .unwrap_or_else(|| workspace_root.to_path_buf()) -} - -fn ensure_path_in_workspace( +/// Gate a path against a root list. An EMPTY list means unrestricted — and it +/// short-circuits BEFORE canonicalizing, so an unrestricted read reports the +/// natural `NotFound` from the open() below rather than a containment error. +/// +/// Containment is checked on the CANONICAL target (`canonical_target_path` +/// resolves the parent for not-yet-existing writes), which is what makes this +/// symlink-safe. `Path::starts_with` compares whole components, so root +/// `/foo/bar` does not admit `/foo/barbaz`. +fn ensure_path_allowed( path: &Path, - workspace_root: &Path, - workspace_root_canonical: Option<&Path>, + roots: &[PathBuf], for_write: bool, ) -> Result<(), FileSystemRuntimeError> { - let root = canonical_workspace_root(workspace_root, workspace_root_canonical); - let target = canonical_target_path(path, for_write)?; + if roots.is_empty() { + return Ok(()); + } - if !target.starts_with(&root) { - return Err(FileSystemRuntimeError::InvalidParams(format!( - "path is outside workspace root: {}", - path.display() - ))); + let target = canonical_target_path(path, for_write)?; + if roots.iter().any(|root| target.starts_with(root)) { + return Ok(()); } - Ok(()) + let direction = if for_write { "write" } else { "read" }; + Err(FileSystemRuntimeError::InvalidParams(format!( + "path is outside the allowed {direction} roots: {} \ + (widen with {FS_EXTRA_ROOTS_ENV}, or set {FS_POLICY_ENV}=unrestricted)", + path.display() + ))) } fn canonical_target_path(path: &Path, for_write: bool) -> Result { @@ -458,29 +963,980 @@ mod tests { let _ = fs::remove_dir_all(workspace); } + /// The default policy's shape, pinned to an explicit root list so the + /// assertions don't depend on where this machine's temp dir / HOME live. + fn reads_open_writes_confined_to(workspace: &Path) -> FsAccessPolicy { + FsAccessPolicy { + read_roots: Vec::new(), + write_roots: vec![canonical_root(workspace)], + } + } + + fn invalid_params(error: FileSystemRuntimeError) -> String { + match error { + FileSystemRuntimeError::InvalidParams(message) => message, + other => panic!("expected InvalidParams, got: {other:?}"), + } + } + + /// `strict` is the historical behavior — keep a regression net on it, since + /// it is what `CODEG_ACP_FS_POLICY=strict` promises to restore. #[tokio::test(flavor = "current_thread")] - async fn rejects_path_outside_workspace() { + async fn strict_rejects_read_and_write_outside_workspace() { let workspace = temp_workspace(); let outside = std::env::temp_dir().join(format!("outside-{}.txt", uuid::Uuid::new_v4())); fs::write(&outside, "x").expect("write outside file"); - let runtime = FileSystemRuntime::new(workspace.clone()); - let error = runtime + let runtime = FileSystemRuntime::with_policy(FsAccessPolicy::strict(&workspace)); + + let read_err = invalid_params( + runtime + .read_text_file(ReadTextFileRequest::new("sid", &outside)) + .await + .expect_err("strict should reject an outside read"), + ); + assert!( + read_err.contains("outside the allowed read roots"), + "unexpected message: {read_err}" + ); + + let write_err = invalid_params( + runtime + .write_text_file(WriteTextFileRequest::new("sid", &outside, "nope")) + .await + .expect_err("strict should reject an outside write"), + ); + assert!( + write_err.contains("outside the allowed write roots"), + "unexpected message: {write_err}" + ); + assert_eq!( + fs::read_to_string(&outside).expect("outside file survives"), + "x", + "a rejected write must not touch the file" + ); + + let _ = fs::remove_file(outside); + let _ = fs::remove_dir_all(workspace); + } + + /// The regression this whole change exists for: an agent reading its own + /// out-of-workspace state (grok's `/skills/*/SKILL.md`, + /// attachment images under the temp dir) must succeed by default. + #[tokio::test(flavor = "current_thread")] + async fn default_allows_read_outside_workspace() { + let workspace = temp_workspace(); + let outside = std::env::temp_dir().join(format!("outside-{}.txt", uuid::Uuid::new_v4())); + fs::write(&outside, "agent-owned\n").expect("write outside file"); + + let runtime = FileSystemRuntime::with_policy(reads_open_writes_confined_to(&workspace)); + let response = runtime .read_text_file(ReadTextFileRequest::new("sid", &outside)) .await - .expect_err("should reject outside path"); + .expect("outside read should be allowed"); + assert_eq!(response.content, "agent-owned\n"); - match error { - FileSystemRuntimeError::InvalidParams(message) => { - assert!(message.contains("outside workspace")); + let _ = fs::remove_file(outside); + let _ = fs::remove_dir_all(workspace); + } + + /// Unrestricted reads must still surface a real missing file as such, not as + /// a containment error (the gate short-circuits before canonicalizing). + #[tokio::test(flavor = "current_thread")] + async fn unrestricted_read_reports_missing_file_naturally() { + let workspace = temp_workspace(); + let missing = workspace.join("nope.txt"); + + let runtime = FileSystemRuntime::with_policy(reads_open_writes_confined_to(&workspace)); + let message = invalid_params( + runtime + .read_text_file(ReadTextFileRequest::new("sid", &missing)) + .await + .expect_err("missing file should error"), + ); + assert!( + message.contains("failed to read") && !message.contains("allowed read roots"), + "unexpected message: {message}" + ); + + let _ = fs::remove_dir_all(workspace); + } + + #[tokio::test(flavor = "current_thread")] + async fn default_rejects_write_outside_every_root() { + let workspace = temp_workspace(); + let other = temp_workspace(); + let target = other.join("stray.txt"); + + let runtime = FileSystemRuntime::with_policy(reads_open_writes_confined_to(&workspace)); + let message = invalid_params( + runtime + .write_text_file(WriteTextFileRequest::new("sid", &target, "stray")) + .await + .expect_err("write outside every root should be rejected"), + ); + assert!( + message.contains("outside the allowed write roots") + && message.contains(FS_EXTRA_ROOTS_ENV), + "message should name the widening knob: {message}" + ); + assert!(!target.exists(), "rejected write must not create the file"); + + let _ = fs::remove_dir_all(workspace); + let _ = fs::remove_dir_all(other); + } + + /// A symlink inside the workspace pointing out of it must not become a + /// write tunnel: containment is checked on the canonicalized parent. + #[cfg(unix)] + #[tokio::test(flavor = "current_thread")] + async fn rejects_symlink_escape_on_write() { + let workspace = temp_workspace(); + let outside = temp_workspace(); + let link = workspace.join("escape"); + std::os::unix::fs::symlink(&outside, &link).expect("create symlink"); + + let runtime = FileSystemRuntime::with_policy(reads_open_writes_confined_to(&workspace)); + let message = invalid_params( + runtime + .write_text_file(WriteTextFileRequest::new( + "sid", + link.join("pwned.txt"), + "pwned", + )) + .await + .expect_err("symlink escape should be rejected"), + ); + assert!( + message.contains("outside the allowed write roots"), + "unexpected message: {message}" + ); + assert!(!outside.join("pwned.txt").exists(), "escape must not write"); + + let _ = fs::remove_file(link); + let _ = fs::remove_dir_all(workspace); + let _ = fs::remove_dir_all(outside); + } + + #[tokio::test(flavor = "current_thread")] + async fn unrestricted_policy_writes_anywhere() { + let workspace = temp_workspace(); + let elsewhere = temp_workspace(); + let target = elsewhere.join("free.txt"); + + let runtime = FileSystemRuntime::with_policy(FsAccessPolicy::unrestricted()); + runtime + .write_text_file(WriteTextFileRequest::new("sid", &target, "free")) + .await + .expect("unrestricted write should succeed"); + assert_eq!(fs::read_to_string(&target).expect("read back"), "free"); + + let _ = fs::remove_dir_all(workspace); + let _ = fs::remove_dir_all(elsewhere); + } + + #[test] + fn permissive_write_roots_include_workspace_agent_home_and_temp() { + let workspace = temp_workspace(); + let policy = FsAccessPolicy::permissive(&workspace, AgentType::Grok, &BTreeMap::new()); + + assert!( + policy.read_roots.is_empty(), + "default reads must be unrestricted" + ); + for expected in [ + canonical_root(&workspace), + canonical_root(&crate::parsers::grok::resolve_grok_home_dir()), + canonical_root(&std::env::temp_dir()), + ] { + assert!( + policy.write_roots.contains(&expected), + "missing write root {}: {:?}", + expected.display(), + policy.write_roots + ); + } + + let _ = fs::remove_dir_all(workspace); + } + + /// The reported regression, reproduced in its exact shape: grok's plan mode + /// writes `/sessions///plan.md`, which is by + /// design outside the workspace. Before this policy existed the write was + /// refused and grok fell back to shelling out a `cat > … << 'PLAN_EOF'` + /// heredoc — so this asserts the delegated write now simply lands. + #[tokio::test(flavor = "current_thread")] + async fn permissive_policy_writes_agent_plan_file_outside_workspace() { + let workspace = temp_workspace(); + let grok_home = temp_workspace(); + let session_dir = grok_home + .join("sessions") + .join("%2FUsers%2Fme%2Fwork%2Fmy-app") + .join("019f9517-cbde-7f63-ae52-d04fa96b4b6b"); + fs::create_dir_all(&session_dir).expect("create grok session dir"); + let plan = session_dir.join("plan.md"); + + let runtime_env = BTreeMap::from([( + "GROK_HOME".to_string(), + grok_home.to_string_lossy().to_string(), + )]); + let runtime = FileSystemRuntime::with_policy(FsAccessPolicy::permissive( + &workspace, + AgentType::Grok, + &runtime_env, + )); + + runtime + .write_text_file(WriteTextFileRequest::new("sid", &plan, "# Plan\n")) + .await + .expect("grok must be able to write its own plan file"); + assert_eq!(fs::read_to_string(&plan).expect("read back"), "# Plan\n"); + + // …and read it back, which is how grok fetches the plan body after the + // user approves `exit_plan_mode`. + let response = runtime + .read_text_file(ReadTextFileRequest::new("sid", &plan)) + .await + .expect("grok must be able to read its own plan file back"); + assert_eq!(response.content, "# Plan\n"); + + let _ = fs::remove_dir_all(workspace); + let _ = fs::remove_dir_all(grok_home); + } + + /// End-to-end on the REAL default policy, which is what catches root + /// canonicalization mismatches: on macOS the temp dir is handed to us as + /// `/var/folders/…` but canonicalizes to `/private/var/folders/…`, and the + /// rejected attachment paths in the bug report were the `/private/…` form. + /// A raw `starts_with` against the un-canonicalized root would fail here. + #[tokio::test(flavor = "current_thread")] + async fn permissive_policy_writes_into_temp_dir() { + let workspace = temp_workspace(); + let elsewhere_in_temp = temp_workspace(); + let target = elsewhere_in_temp.join("attachment.txt"); + + let runtime = FileSystemRuntime::with_policy(FsAccessPolicy::permissive( + &workspace, + AgentType::Grok, + &BTreeMap::new(), + )); + runtime + .write_text_file(WriteTextFileRequest::new("sid", &target, "payload")) + .await + .expect("temp dir should be writable under the default policy"); + assert_eq!(fs::read_to_string(&target).expect("read back"), "payload"); + + let _ = fs::remove_dir_all(workspace); + let _ = fs::remove_dir_all(elsewhere_in_temp); + } + + #[test] + fn extra_roots_env_adds_writable_roots() { + let workspace = temp_workspace(); + let extra_a = temp_workspace(); + let extra_b = temp_workspace(); + let joined = std::env::join_paths([&extra_a, &extra_b]).expect("join paths"); + + let runtime_env = BTreeMap::from([( + FS_EXTRA_ROOTS_ENV.to_string(), + joined.to_string_lossy().to_string(), + )]); + let policy = FsAccessPolicy::permissive(&workspace, AgentType::Grok, &runtime_env); + + for extra in [&extra_a, &extra_b] { + assert!( + policy.write_roots.contains(&canonical_root(extra)), + "extra root {} not honored", + extra.display() + ); + } + + for dir in [workspace, extra_a, extra_b] { + let _ = fs::remove_dir_all(dir); + } + } + + #[test] + fn policy_from_env_reads_runtime_env_first() { + let workspace = temp_workspace(); + let policy_for = |value: &str| { + FsAccessPolicy::from_env( + &workspace, + AgentType::Grok, + &BTreeMap::from([(FS_POLICY_ENV.to_string(), value.to_string())]), + ) + }; + + let strict = policy_for("strict"); + assert_eq!(strict.read_roots, vec![canonical_root(&workspace)]); + assert_eq!(strict.write_roots, vec![canonical_root(&workspace)]); + + let unrestricted = policy_for("unrestricted"); + assert!(unrestricted.read_roots.is_empty()); + assert!(unrestricted.write_roots.is_empty()); + + // An unrecognized value must degrade to the default, never to a + // fail-open or fail-closed surprise. + for value in ["default", "totally-bogus"] { + let fallback = policy_for(value); + assert!(fallback.read_roots.is_empty(), "{value}: reads open"); + assert!( + fallback.write_roots.contains(&canonical_root(&workspace)), + "{value}: workspace writable" + ); + } + + let _ = fs::remove_dir_all(workspace); + } + + /// A per-agent home relocation must move the allowed root with it, or the + /// agent's own state directory falls outside the policy again. + /// + /// Only the keys whose value IS the data root verbatim belong here; the ones + /// their resolver appends a sub-path to (Gemini, OpenCode, Cursor's XDG + /// fallback) are covered by `relocation_suffixes_match_the_agents_own_resolvers`. + #[test] + fn agent_data_roots_honor_runtime_env_relocation() { + let relocated = PathBuf::from("/tmp/codeg-relocated-agent-home"); + let cases = [ + (AgentType::Grok, "GROK_HOME"), + (AgentType::ClaudeCode, "CLAUDE_CONFIG_DIR"), + (AgentType::Codex, "CODEX_HOME"), + (AgentType::CodeBuddy, "CODEBUDDY_CONFIG_DIR"), + (AgentType::KimiCode, "KIMI_CODE_HOME"), + (AgentType::Cursor, "CURSOR_CONFIG_DIR"), + (AgentType::Hermes, "HERMES_HOME"), + (AgentType::Cline, "CLINE_DIR"), + (AgentType::Pi, "PI_CODING_AGENT_DIR"), + ]; + + for (agent_type, key) in cases { + let runtime_env = + BTreeMap::from([(key.to_string(), relocated.to_string_lossy().to_string())]); + let roots = agent_data_roots(agent_type, &runtime_env); + assert!( + roots.contains(&relocated), + "{agent_type:?}: {key} not honored, got {roots:?}" + ); + } + } + + /// Each agent's relocation key must land on the SAME directory its own + /// resolver would compute for that value — not one level above it. Getting + /// Gemini's `.gemini` suffix wrong would make the bare relocation target + /// writable, which for `GEMINI_CLI_HOME=$HOME` is the whole home directory. + #[test] + fn relocation_suffixes_match_the_agents_own_resolvers() { + let base = PathBuf::from("/tmp/codeg-relocation-base"); + let cases = [ + (AgentType::Gemini, "GEMINI_CLI_HOME", ".gemini"), + (AgentType::OpenCode, "XDG_DATA_HOME", "opencode"), + (AgentType::Cursor, "XDG_CONFIG_HOME", "cursor"), + ]; + + for (agent_type, key, suffix) in cases { + let runtime_env = + BTreeMap::from([(key.to_string(), base.to_string_lossy().to_string())]); + let roots = agent_data_roots(agent_type, &runtime_env); + + assert!( + roots.contains(&base.join(suffix)), + "{agent_type:?}: expected {key} to resolve to /{suffix}, got {roots:?}" + ); + assert!( + !roots.contains(&base), + "{agent_type:?}: the bare {key} value must NOT become a root \ + (that widens it by a level): {roots:?}" + ); + } + } + + /// No relocation value may be tilde-expanded: the launched CLIs treat `~` + /// literally, so expanding it here would hand out `$HOME` as a writable root + /// the user never selected. Hermes' launch contract + /// (`commands::acp::hermes_home_for_launch`) is explicit that `~` is NOT + /// expanded, and a BLANK value falls back to `~/.hermes` rather than + /// re-inheriting the parent. + #[test] + fn relocation_never_expands_tilde_into_home() { + let home = dirs::home_dir().expect("home dir"); + let cases = [ + (AgentType::Grok, "GROK_HOME"), + (AgentType::Codex, "CODEX_HOME"), + (AgentType::ClaudeCode, "CLAUDE_CONFIG_DIR"), + (AgentType::Hermes, "HERMES_HOME"), + (AgentType::Pi, "PI_CODING_AGENT_DIR"), + ]; + + for (agent_type, key) in cases { + // Pi's sessions slot has a second candidate that could be satisfied + // from this machine's environment; skip when that applies. + if agent_relocated_by_process_env(agent_type) { + continue; } - other => panic!("unexpected error: {other:?}"), + let runtime_env = BTreeMap::from([(key.to_string(), "~".to_string())]); + let roots = agent_data_roots(agent_type, &runtime_env); + + // The security property: `~` is never expanded into the home dir. + assert!( + !roots.contains(&home), + "{agent_type:?}: {key}=~ must not add $HOME as a root, got {roots:?}" + ); + // A bare `~` is a RELATIVE path, so the slot yields NO root — not the + // inactive default either, since the child really does use the value + // it was given (see `relative_agent_root_is_refused_not_absolutized`). + assert!( + roots.is_empty(), + "{agent_type:?}: {key}=~ should yield no root at all, got {roots:?}" + ); } + } + + /// A blank relocation value must fall back to Hermes' default home, not + /// re-inherit codeg's process env — mirroring the launch contract. + #[test] + fn hermes_blank_relocation_falls_back_to_default_home() { + let runtime_env = BTreeMap::from([("HERMES_HOME".to_string(), " ".to_string())]); + let roots = agent_data_roots(AgentType::Hermes, &runtime_env); + let expected = dirs::home_dir().expect("home dir").join(".hermes"); + assert_eq!(roots, vec![expected]); + } + + /// When `runtime_env` relocates a profile, `merge_agent_env` gives it the + /// highest precedence so the child NEVER reads the process-env default. That + /// inactive tree holds config and credentials, so it must not stay writable. + #[test] + fn relocation_excludes_the_inactive_default_root() { + let relocated = PathBuf::from("/tmp/codeg-isolated-profile"); + let cases = [ + (AgentType::Codex, "CODEX_HOME"), + (AgentType::Grok, "GROK_HOME"), + (AgentType::ClaudeCode, "CLAUDE_CONFIG_DIR"), + (AgentType::Hermes, "HERMES_HOME"), + ]; + + for (agent_type, key) in cases { + let runtime_env = + BTreeMap::from([(key.to_string(), relocated.to_string_lossy().to_string())]); + let roots = agent_data_roots(agent_type, &runtime_env); + + assert!( + roots.contains(&relocated), + "{agent_type:?}: relocated root missing, got {roots:?}" + ); + for default_root in builtin_default_roots(agent_type) { + assert!( + !roots.contains(&default_root), + "{agent_type:?}: inactive default {} must not stay writable, got {roots:?}", + default_root.display() + ); + } + } + } + + /// Whether THIS machine's environment already relocates the agent (e.g. an + /// exported `CODEX_HOME` / `XDG_DATA_HOME`). Such a relocation legitimately + /// produces a root without ever consulting the home directory, so tests that + /// assert "no root" for a broken home must skip these agents to stay + /// deterministic across developer machines. + fn agent_relocated_by_process_env(agent_type: AgentType) -> bool { + agent_root_slots(agent_type) + .iter() + .flat_map(|slot| slot.candidates.iter()) + .any(|(key, _)| std::env::var_os(key).is_some()) + } + + /// Force every candidate key "absent for the child" (blank ⇒ `env_remove`) + /// so the slots fall through to their home-relative defaults, independent of + /// whatever this machine's environment happens to set. + fn builtin_default_roots(agent_type: AgentType) -> Vec { + let blanked: BTreeMap = agent_root_slots(agent_type) + .iter() + .flat_map(|slot| slot.candidates.iter()) + .map(|(key, _)| ((*key).to_string(), String::new())) + .collect(); + agent_data_roots(agent_type, &blanked) + } + + /// A BLANK value in `env_json` is not "unset, use ours" — the vendored spawn + /// layer `env_remove`s it, so the child falls back to its OWN default. Reading + /// through to codeg's process env would aim the root at a profile the child + /// never opens, re-breaking the writes this change exists to allow. + #[test] + fn blank_runtime_value_falls_back_to_the_agents_default_not_our_env() { + let home = dirs::home_dir().expect("home dir"); + + // Deterministic regardless of this machine's env: blank short-circuits + // before any process-env lookup. + assert_eq!( + agent_data_roots( + AgentType::Codex, + &BTreeMap::from([("CODEX_HOME".to_string(), String::new())]) + ), + vec![home.join(".codex")] + ); + assert_eq!( + agent_data_roots( + AgentType::Cursor, + &BTreeMap::from([ + ("CURSOR_CONFIG_DIR".to_string(), String::new()), + ("XDG_CONFIG_HOME".to_string(), String::new()), + ]) + ), + vec![home.join(".cursor")] + ); + assert_eq!( + agent_data_roots( + AgentType::Pi, + &BTreeMap::from([ + ("PI_CODING_AGENT_DIR".to_string(), String::new()), + ("PI_CODING_AGENT_SESSION_DIR".to_string(), String::new()), + ]) + ), + vec![ + home.join(".pi").join("agent"), + home.join(".pi").join("agent").join("sessions") + ] + ); + } + + /// A masked higher-precedence key must fall through to the NEXT candidate, + /// not to the masked key's inherited value. + #[test] + fn blank_runtime_value_falls_through_to_the_next_candidate() { + let roots = agent_data_roots( + AgentType::Cursor, + &BTreeMap::from([ + ("CURSOR_CONFIG_DIR".to_string(), String::new()), + ("XDG_CONFIG_HOME".to_string(), "/tmp/codeg-xdg".to_string()), + ]), + ); + assert_eq!(roots, vec![PathBuf::from("/tmp/codeg-xdg/cursor")]); + } + + /// `child_env_value` must distinguish the ways a var reaches (or fails to + /// reach) the child, and must NOT normalize the value for resolvers that + /// don't. Uses a private key so it cannot race another test. + #[test] + fn child_env_value_models_the_spawn_layers_env_remove() { + let key = "CODEG_TEST_FS_CHILD_ENV"; + let os = OsString::from; + + temp_env::with_var(key, Some("/parent/value"), || { + let runtime = |value: &str| BTreeMap::from([(key.to_string(), value.to_string())]); + + // Silent in runtime_env ⇒ the child inherits codeg's value. + assert_eq!( + child_env_value(&BTreeMap::new(), key, false), + Some(os("/parent/value")) + ); + // EXACTLY empty ⇒ `env_remove` ⇒ the child sees NOTHING, and we must + // NOT read through to codeg's value. + assert_eq!(child_env_value(&runtime(""), key, false), None); + // Whitespace-only is NOT removed by the spawn layer — it reaches the + // child verbatim, so a non-trimming resolver must see it verbatim. + assert_eq!(child_env_value(&runtime(" "), key, false), Some(os(" "))); + // …but IS unset for the resolvers that trim (Hermes, Cline). + assert_eq!(child_env_value(&runtime(" "), key, true), None); + // Non-empty replaces codeg's value, verbatim when not trimming. + assert_eq!( + child_env_value(&runtime(" /child/value "), key, false), + Some(os(" /child/value ")) + ); + assert_eq!( + child_env_value(&runtime(" /child/value "), key, true), + Some(os("/child/value")) + ); + }); + + temp_env::with_var_unset(key, || { + assert_eq!(child_env_value(&BTreeMap::new(), key, false), None); + }); + } + + /// A RELATIVE agent home must never become a root. It would be resolved + /// against codeg's cwd while the child runs in the workspace cwd, and that + /// mismatch widens: `"."` with codeg launched from `/` yields `/`, which every + /// absolute path passes `starts_with` against; `".."` grants an unrelated tree. + #[test] + fn relative_agent_root_is_refused_not_absolutized() { + let workspace = temp_workspace(); + + for value in [".", "..", "relative/sub", "./x"] { + for (agent_type, key) in [ + (AgentType::Codex, "CODEX_HOME"), + (AgentType::Grok, "GROK_HOME"), + (AgentType::Hermes, "HERMES_HOME"), + (AgentType::Pi, "PI_CODING_AGENT_DIR"), + ] { + let runtime_env = BTreeMap::from([(key.to_string(), value.to_string())]); + let roots = agent_data_roots(agent_type, &runtime_env); + + assert!( + roots.iter().all(|root| root.is_absolute()), + "{agent_type:?}: {key}={value:?} produced a relative root: {roots:?}" + ); + assert!( + roots.iter().all(|root| root != Path::new("/")), + "{agent_type:?}: {key}={value:?} must not admit /: {roots:?}" + ); + // And it must NOT fall through to the inactive default: the child + // received this value and uses it, so the default tree holds + // config/credentials the session never opens. + for inactive in builtin_default_roots(agent_type) { + assert!( + !roots.contains(&inactive), + "{agent_type:?}: {key}={value:?} leaked the inactive default {}: {roots:?}", + inactive.display() + ); + } + + // End-to-end: the gate must not gain a cwd-derived root either. + let policy = FsAccessPolicy::permissive(&workspace, agent_type, &runtime_env); + let cwd = std::env::current_dir().expect("cwd"); + assert!( + !policy.write_roots.contains(&PathBuf::from("/")) + && !policy.write_roots.contains(&cwd), + "{agent_type:?}: {key}={value:?} leaked a cwd-derived write root: {:?}", + policy.write_roots + ); + } + } + + let _ = fs::remove_dir_all(workspace); + } + + /// `merge_agent_env` copies every `runtime_env` entry into the child, `HOME` + /// included — the classic way to isolate a CLI's config. The home-relative + /// defaults must therefore follow the CHILD's home: anchoring them on codeg's + /// would grant the inactive profile and refuse the active one. + #[cfg(not(windows))] + #[test] + fn home_relative_defaults_follow_the_childs_home() { + let codeg_home = dirs::home_dir().expect("home dir"); + let isolated = PathBuf::from("/tmp/codeg-isolated-home"); + let runtime_env = + BTreeMap::from([("HOME".to_string(), isolated.to_string_lossy().to_string())]); + + for (agent_type, rel) in [ + (AgentType::Codex, ".codex"), + (AgentType::Grok, ".grok"), + (AgentType::ClaudeCode, ".claude"), + ] { + let roots = agent_data_roots(agent_type, &runtime_env); + assert!( + roots.contains(&isolated.join(rel)), + "{agent_type:?}: active root {} missing, got {roots:?}", + isolated.join(rel).display() + ); + assert!( + !roots.contains(&codeg_home.join(rel)), + "{agent_type:?}: inactive {} must not stay writable, got {roots:?}", + codeg_home.join(rel).display() + ); + } + } + + /// A relative child `HOME` would be absolutized against codeg's cwd — the same + /// widening as a relative agent home — so the slot is refused instead. + #[cfg(not(windows))] + #[test] + fn relative_child_home_refuses_the_slot() { + for value in [".", "..", "relative/home"] { + let runtime_env = BTreeMap::from([("HOME".to_string(), value.to_string())]); + for agent_type in ALL_AGENT_TYPES { + if agent_relocated_by_process_env(agent_type) { + continue; + } + let roots = agent_data_roots(agent_type, &runtime_env); + assert!( + roots.is_empty(), + "{agent_type:?}: HOME={value:?} should yield no root, got {roots:?}" + ); + } + } + } + + /// A BLANK `HOME` is `env_remove`d, so the child resolves its home from the OS + /// account database — which codeg cannot read without duplicating a passwd + /// lookup, and `dirs::home_dir()` does NOT answer because it consults codeg's + /// own `$HOME` first. So the slot is refused rather than guessed at. + /// + /// Asserting "no roots" is stronger than asserting "not codeg's root", and it + /// needs no `temp_env`, so it cannot race other tests that read `$HOME`. + #[cfg(not(windows))] + #[test] + fn removed_child_home_refuses_the_slot() { + let runtime_env = BTreeMap::from([("HOME".to_string(), String::new())]); + let codeg_home = dirs::home_dir().expect("home dir"); + + for agent_type in ALL_AGENT_TYPES { + if agent_relocated_by_process_env(agent_type) { + continue; + } + + let roots = agent_data_roots(agent_type, &runtime_env); + assert!( + roots.is_empty(), + "{agent_type:?}: a removed HOME must yield no root (never codeg's \ + {}), got {roots:?}", + codeg_home.display() + ); + } + } + + /// Refusing a slot must never widen the gate: `permissive` still seeds the + /// workspace root, so an empty agent-root list cannot collapse into the + /// "empty ⇒ unrestricted" sentinel. + #[cfg(not(windows))] + #[tokio::test(flavor = "current_thread")] + async fn refused_agent_slot_does_not_produce_an_unrestricted_policy() { + let workspace = temp_workspace(); + let runtime_env = BTreeMap::from([("HOME".to_string(), String::new())]); + + let policy = FsAccessPolicy::permissive(&workspace, AgentType::Codex, &runtime_env); + assert!( + !policy.write_roots.is_empty(), + "write roots must never be empty (that means unrestricted)" + ); + assert!(policy.write_roots.contains(&canonical_root(&workspace))); + + // The gate must still refuse an unrelated path. + let outside = temp_workspace(); + let runtime = FileSystemRuntime::with_policy(FsAccessPolicy { + read_roots: Vec::new(), + write_roots: vec![canonical_root(&workspace)], + }); + assert!(runtime + .write_text_file(WriteTextFileRequest::new( + "sid", + outside.join("x.txt"), + "nope" + )) + .await + .is_err()); + + let _ = fs::remove_dir_all(workspace); + let _ = fs::remove_dir_all(outside); + } + + /// Same hazard through the user-facing knob. + #[test] + fn relative_extra_roots_are_dropped() { + let runtime_env = BTreeMap::from([( + FS_EXTRA_ROOTS_ENV.to_string(), + std::env::join_paths([Path::new("."), Path::new("/tmp/codeg-abs-extra")]) + .expect("join paths") + .to_string_lossy() + .to_string(), + )]); + + let roots = extra_write_roots(&runtime_env); + assert_eq!(roots, vec![PathBuf::from("/tmp/codeg-abs-extra")]); + } + + /// The extra-roots list must be read VERBATIM. `split_paths` does not trim + /// entries, so trimming the value first would rewrite the relative entry + /// `" / "` into `/` and hand out the entire filesystem. + #[test] + fn whitespace_padded_extra_root_is_not_trimmed_into_filesystem_root() { + for padded in [" / ", "\t/", "/ "] { + let runtime_env = + BTreeMap::from([(FS_EXTRA_ROOTS_ENV.to_string(), padded.to_string())]); + let roots = extra_write_roots(&runtime_env); + assert!( + !roots.contains(&PathBuf::from("/")), + "{padded:?} must not become the filesystem root, got {roots:?}" + ); + } + + // An entry that IS exactly `/` stays honored — widening is this knob's + // declared purpose, so that is the user's explicit choice. + let runtime_env = BTreeMap::from([(FS_EXTRA_ROOTS_ENV.to_string(), "/".to_string())]); + assert_eq!(extra_write_roots(&runtime_env), vec![PathBuf::from("/")]); + } + + /// Trimming a relocation value can WIDEN the policy, not just fail to match: + /// `" / "` reaches the child as a relative path, but trimmed it becomes the + /// filesystem root and would make everything writable. + #[test] + fn whitespace_padded_root_never_becomes_the_filesystem_root() { + for (agent_type, key) in [ + (AgentType::Codex, "CODEX_HOME"), + (AgentType::Grok, "GROK_HOME"), + (AgentType::Pi, "PI_CODING_AGENT_DIR"), + ] { + let runtime_env = BTreeMap::from([(key.to_string(), " / ".to_string())]); + let roots = agent_data_roots(agent_type, &runtime_env); + assert!( + !roots.contains(&PathBuf::from("/")), + "{agent_type:?}: {key}=\" / \" must not resolve to the filesystem root, \ + got {roots:?}" + ); + } + + // And the same value must not slip through the write gate either. + let workspace = temp_workspace(); + let runtime_env = BTreeMap::from([("CODEX_HOME".to_string(), " / ".to_string())]); + let policy = FsAccessPolicy::permissive(&workspace, AgentType::Codex, &runtime_env); + assert!( + !policy.write_roots.contains(&PathBuf::from("/")), + "write roots must not include /: {:?}", + policy.write_roots + ); - let _ = fs::remove_file(outside); let _ = fs::remove_dir_all(workspace); } + /// Pins the declarative slot table against the `crate::parsers` resolvers it + /// mirrors, so a change to either side cannot drift silently. Skips any agent + /// whose relocation vars are set in THIS process, where the resolver would + /// legitimately return a relocated path instead of the default. + #[test] + fn root_slots_match_parser_resolvers() { + use crate::parsers; + + let expected: [(AgentType, PathBuf); 11] = [ + (AgentType::Grok, parsers::grok::resolve_grok_home_dir()), + ( + AgentType::ClaudeCode, + parsers::claude::resolve_claude_config_dir(), + ), + (AgentType::Codex, parsers::codex::resolve_codex_home_dir()), + ( + AgentType::Gemini, + parsers::gemini::resolve_gemini_base_dir(), + ), + ( + AgentType::CodeBuddy, + parsers::codebuddy::resolve_codebuddy_config_dir(), + ), + ( + AgentType::KimiCode, + parsers::kimi_code::resolve_kimi_code_home_dir(), + ), + ( + AgentType::Cursor, + parsers::cursor::resolve_cursor_config_dir(), + ), + ( + AgentType::Hermes, + parsers::hermes::resolve_hermes_home_dir(), + ), + ( + AgentType::OpenCode, + parsers::opencode::resolve_opencode_base_dir(), + ), + (AgentType::Cline, parsers::cline::cline_data_dir()), + (AgentType::Pi, parsers::pi::resolve_pi_sessions_dir()), + ]; + + for (agent_type, resolver_root) in expected { + let env_configured = agent_root_slots(agent_type) + .iter() + .flat_map(|slot| slot.candidates.iter()) + .any(|(key, _)| std::env::var_os(key).is_some()); + if env_configured { + continue; + } + + let roots = builtin_default_roots(agent_type); + assert!( + roots.contains(&resolver_root), + "{agent_type:?}: slot default {roots:?} drifted from resolver {}", + resolver_root.display() + ); + } + } + + /// Cursor's resolver prefers `CURSOR_CONFIG_DIR` over `XDG_CONFIG_HOME`; + /// first match must win rather than both being admitted. + #[test] + fn cursor_relocation_respects_resolver_precedence() { + let runtime_env = BTreeMap::from([ + ( + "CURSOR_CONFIG_DIR".to_string(), + "/tmp/codeg-cursor".to_string(), + ), + ("XDG_CONFIG_HOME".to_string(), "/tmp/codeg-xdg".to_string()), + ]); + + let roots = agent_data_roots(AgentType::Cursor, &runtime_env); + assert_eq!(roots, vec![PathBuf::from("/tmp/codeg-cursor")]); + } + + /// pi relocates its session store independently of its agent home, so a + /// `PI_CODING_AGENT_SESSION_DIR` supplied through `env_json` must widen the + /// roots on its own — the process-env-only resolver cannot see it. + #[test] + fn pi_session_dir_relocation_is_honored() { + let sessions = PathBuf::from("/tmp/codeg-pi-sessions-elsewhere"); + let runtime_env = BTreeMap::from([( + "PI_CODING_AGENT_SESSION_DIR".to_string(), + sessions.to_string_lossy().to_string(), + )]); + + let roots = agent_data_roots(AgentType::Pi, &runtime_env); + assert!( + roots.contains(&sessions), + "pi session-dir relocation not honored: {roots:?}" + ); + } + + /// No agent may resolve a root so broad that the gate becomes meaningless — + /// checked both with a clean env AND with every relocation key set to `~`, + /// since a tilde-expanding bug would only show up in the latter. + #[test] + fn no_agent_data_root_is_the_home_dir_or_filesystem_root() { + let home = dirs::home_dir().expect("home dir"); + + let tilde_env: BTreeMap = ALL_AGENT_TYPES + .iter() + .flat_map(|agent_type| agent_root_slots(*agent_type).iter()) + .flat_map(|slot| slot.candidates.iter()) + .map(|(key, _)| ((*key).to_string(), "~".to_string())) + .collect(); + + for env in [BTreeMap::new(), tilde_env] { + for agent_type in ALL_AGENT_TYPES { + for root in agent_data_roots(agent_type, &env) { + assert!( + root != home && root.parent().is_some() && root != Path::new("/"), + "{agent_type:?} resolved an over-broad root: {}", + root.display() + ); + } + } + } + } + + const ALL_AGENT_TYPES: [AgentType; 12] = [ + AgentType::ClaudeCode, + AgentType::Codex, + AgentType::OpenCode, + AgentType::Gemini, + AgentType::OpenClaw, + AgentType::Cline, + AgentType::Hermes, + AgentType::CodeBuddy, + AgentType::KimiCode, + AgentType::Pi, + AgentType::Grok, + AgentType::Cursor, + ]; + + #[test] + fn every_agent_type_has_a_data_root() { + for agent_type in ALL_AGENT_TYPES { + let roots = agent_data_roots(agent_type, &BTreeMap::new()); + assert!(!roots.is_empty(), "{agent_type:?} resolved no data root"); + assert!( + roots.iter().all(|root| root.is_absolute()), + "{agent_type:?} produced a relative root: {roots:?}" + ); + } + } + #[tokio::test(flavor = "current_thread")] async fn write_replaces_existing_content() { let workspace = temp_workspace();