agent: Re-enable sandbox

crates/acp_thread/src/acp_thread.rs

@@ -182,7 +182,12 @@ pub struct SandboxAuthorizationDetails {
     #[serde(default)]
     pub unsandboxed: bool,
     #[serde(default)]
-    pub write_paths: Vec<PathBuf>,
+    pub write_paths: Vec<settings::GrantedWritePath>,
+    /// Windows/WSL only: the command will write to a path on a Windows drive
+    /// (DrvFs), whose sandbox-integrity guarantees are weaker. Drives the
+    /// weaker-guarantee warning banner in the approval prompt.
+    #[serde(default)]
+    pub warn_windows_fs: bool,
     /// The agent-provided justification for requesting these permissions,
     /// shown to the user (attributed to the agent) in the approval prompt.
     #[serde(default)]

crates/acp_thread/src/terminal.rs

@@ -43,7 +43,12 @@ pub struct SandboxWrap {
     /// to make the trust boundary explicit: these originate from
     /// model-requested paths that passed a user-approval prompt. They are
     /// merged with `writable_paths` when generating the sandbox policy.
-    pub extra_write_paths: Vec<PathBuf>,
+    ///
+    /// Each grant carries the canonical target it resolved to at approval
+    /// time; enforcement rebuilds the location via a verifying reopen (see
+    /// [`granted_write_path_to_location`]) rather than re-resolving the bare
+    /// requested path, which closes a symlink TOCTOU.
+    pub extra_write_paths: Vec<settings::GrantedWritePath>,
     /// Outbound network access explicitly approved for this command.
     pub network: SandboxNetworkAccess,
     /// Additional paths that should remain readable but not writable, even when
@@ -156,6 +161,51 @@ impl LinuxWslSandboxError {
     }
 }
 
+/// Rebuild a user-approved write grant into an enforceable
+/// [`sandbox::HostFilesystemLocation`].
+///
+/// When the grant carries a resolved canonical (the normal case, established at
+/// approval time), the location is rebuilt via a verifying
+/// [`sandbox::HostFilesystemLocation::reopen`] — the load-bearing step of the
+/// TOCTOU fix. A legacy bare-string grant (no resolved canonical) falls back to
+/// a fresh [`sandbox::HostFilesystemLocation::capture`].
+pub fn granted_write_path_to_location(
+    granted: &settings::GrantedWritePath,
+) -> std::io::Result<sandbox::HostFilesystemLocation> {
+    match &granted.resolved {
+        Some(resolved) => sandbox::HostFilesystemLocation::reopen(&granted.requested, resolved),
+        None => sandbox::HostFilesystemLocation::capture(&granted.requested),
+    }
+}
+
+/// Rebuild a grant for enforcement, or log and drop it (fail-closed) if it
+/// can't be verified.
+///
+/// A failure here is frequently the symlink-TOCTOU defense firing: the grant's
+/// canonical was redirected or replaced by a symlink since approval, so
+/// [`sandbox::HostFilesystemLocation::reopen`] refuses it. That is a
+/// security-relevant event, so it must be logged rather than silently
+/// swallowed. The grant is dropped (the command runs without it) rather than
+/// bound unverified.
+///
+/// Only for **display** policies (the sandbox-status UI), where a stale grant
+/// should simply not be shown. Enforcement must not drop grants silently — a
+/// command would run with less access than the user approved with no signal —
+/// so [`SandboxWrap::to_policy`] uses the erroring
+/// [`granted_write_path_to_location`] instead.
+pub fn granted_write_path_to_location_or_log(
+    granted: &settings::GrantedWritePath,
+) -> Option<sandbox::HostFilesystemLocation> {
+    granted_write_path_to_location(granted)
+        .inspect_err(|error| {
+            log::warn!(
+                "dropping sandbox write grant {}: {error}",
+                granted.requested.display()
+            );
+        })
+        .ok()
+}
+
 impl SandboxWrap {
     /// Whether the OS sandbox for this request can actually be created right now,
     /// returning a structured [`LinuxWslSandboxError`] when it can't.
@@ -167,43 +217,88 @@ impl SandboxWrap {
     /// Linux, so call it off the main thread. On platforms whose sandbox can't
     /// fail to set up this way it always returns `Ok`.
     pub fn can_create_sandbox(&self) -> Result<(), LinuxWslSandboxError> {
-        sandbox::Sandbox::can_create(&self.to_policy()).map_err(LinuxWslSandboxError::from)
+        let policy = self
+            .to_policy()
+            .map_err(|error| LinuxWslSandboxError::Other(format!("{error:#}")))?;
+        sandbox::Sandbox::can_create(&policy).map_err(LinuxWslSandboxError::from)
     }
 
     /// Translate this request into the cross-platform [`sandbox::SandboxPolicy`].
     ///
     /// This is the enforcement-policy construction point, so it **captures** each
     /// grant as a [`sandbox::HostFilesystemLocation`] (pinning the inode / canonical
-    /// path) rather than passing a re-resolvable path. A location that can't be
-    /// captured (e.g. it doesn't exist) is dropped from the grant — fail-closed.
+    /// path) rather than passing a re-resolvable path.
+    ///
+    /// This function has **no filesystem side effects**: it never creates paths,
+    /// and it **fails** (rather than silently narrowing the policy) when a
+    /// writable path or approved grant can't be captured — running anyway would
+    /// give the command silently less access than the model and user were told
+    /// it has. On Linux a writable grant that doesn't exist can't be captured
+    /// (bwrap can't bind a missing path); the sanctioned way to get a grant to a
+    /// new directory is the `create_directory` tool, which creates it (pinning
+    /// the inode) before the grant is recorded. On macOS a missing leaf still
+    /// canonicalizes, so such grants are captured directly.
     ///
-    /// This function has **no filesystem side effects**: it never creates paths.
     /// It is used both by the side-effect-free [`Self::can_create_sandbox`] probe
-    /// and by real sandbox construction, and must behave identically. On Linux a
-    /// writable grant that doesn't exist yet simply can't be captured (bwrap
-    /// can't bind a missing path), so it's dropped here — the sanctioned way to
-    /// get a grant to a new directory is the `create_directory` tool, which
-    /// creates it (pinning the inode) before the grant is recorded. On macOS a
-    /// missing leaf still canonicalizes, so such grants are captured directly.
-    fn to_policy(&self) -> sandbox::SandboxPolicy {
+    /// and by real sandbox construction, and must behave identically.
+    ///
+    /// A grant failure here can also be the symlink-TOCTOU defense firing: the
+    /// grant's canonical was redirected or replaced by a symlink since approval,
+    /// so the verifying reopen refuses it. Failing the command surfaces that
+    /// security-relevant event instead of running with the grant quietly
+    /// missing.
+    ///
+    /// Protected paths, by contrast, are **best-effort**: we protect only the
+    /// ones that exist at creation time (`capture` succeeding *is* the existence
+    /// check), and silently drop the rest. Unlike a writable grant, a protection
+    /// can't be materialized — you can't pin the inode of a path that isn't
+    /// there — and there is an inherent, *accepted* loophole regardless: a
+    /// command in a non-git directory can `git init` and write hooks into a
+    /// `.git` that didn't exist when the sandbox was built. Since the protection
+    /// is defeatable that way no matter what, failing sandbox creation over a
+    /// currently-absent (or otherwise uncapturable) `.git` would only break
+    /// legitimate cases — non-git projects, single-file worktrees whose
+    /// synthesized `settings.json/.git` routes through a file — without closing
+    /// the hole. So we drop and move on.
+    fn to_policy(&self) -> Result<sandbox::SandboxPolicy> {
         let protected_paths = self
             .protected_paths
             .iter()
-            .filter_map(|path| sandbox::HostFilesystemLocation::new(path).ok())
-            .collect();
+            .filter_map(|path| sandbox::HostFilesystemLocation::capture(path).ok())
+            .collect::<Vec<_>>();
         let fs = if self.allow_fs_write {
             sandbox::SandboxFsPolicy::Unrestricted { protected_paths }
         } else {
-            let writable_paths = self
-                .writable_paths
-                .iter()
-                .chain(self.extra_write_paths.iter())
-                // Capture only — never create anything here (see the doc comment):
-                // materializing an approved-but-missing grant is deferred to
-                // `Sandbox::new` so it can never happen during the `can_create`
-                // probe, before the user has approved the grant.
-                .filter_map(|path| sandbox::HostFilesystemLocation::new(path).ok())
-                .collect();
+            // Project worktree paths are captured fresh; user-approved grants are
+            // rebuilt via the verifying reopen (or captured when legacy bare
+            // strings) through `granted_write_path_to_location`. A path that
+            // can't be captured fails the whole construction (never created).
+            let mut locations = Vec::new();
+            for path in &self.writable_paths {
+                let location = sandbox::HostFilesystemLocation::capture(path).map_err(|error| {
+                    anyhow::anyhow!(error).context(format!(
+                        "cannot capture writable sandbox path `{}`",
+                        path.display()
+                    ))
+                })?;
+                locations.push(location);
+            }
+            for granted in &self.extra_write_paths {
+                let location = granted_write_path_to_location(granted).map_err(|error| {
+                    anyhow::anyhow!(error).context(format!(
+                        "cannot re-verify approved sandbox write grant `{}` (if the \
+                         directory was removed, remove the grant or recreate the \
+                         directory)",
+                        granted.requested.display()
+                    ))
+                })?;
+                locations.push(location);
+            }
+            // Dedupe to a minimal cover on the captured canonical paths, so a
+            // grant nested under a worktree root (or another grant) is dropped
+            // rather than bound redundantly.
+            let writable_paths =
+                sandbox::normalize_host_filesystem_locations(locations.into_iter());
             sandbox::SandboxFsPolicy::Restricted {
                 writable_paths,
                 protected_paths,
@@ -220,7 +315,7 @@ impl SandboxWrap {
                     .collect(),
             },
         };
-        sandbox::SandboxPolicy { fs, network }
+        Ok(sandbox::SandboxPolicy { fs, network })
     }
 }
 
@@ -282,7 +377,7 @@ pub(crate) async fn prepare_sandbox_wrap(
     };
 
     let mut sandbox =
-        sandbox::Sandbox::new(sandbox_wrap.to_policy()).map_err(anyhow::Error::new)?;
+        sandbox::Sandbox::new(sandbox_wrap.to_policy()?).map_err(anyhow::Error::new)?;
     // Windows/WSL only: tell the sandbox which Linux `zed` to provision inside
     // WSL as its `--wsl-sandbox-helper`. A no-op (and a no-op setter) elsewhere.
     #[cfg(target_os = "windows")]
@@ -570,3 +665,90 @@ pub async fn create_terminal_entity(
         })
         .await
 }
+
+#[cfg(all(test, target_os = "linux"))]
+mod linux_tests {
+    use super::*;
+
+    /// Regression test for the bug where enforcement-policy construction
+    /// *created* missing write grants — famously turning a granted
+    /// `~/.config/zed/AGENTS.md` file path into a directory. A grant whose
+    /// target no longer exists must fail policy construction with an error
+    /// naming it, and nothing may be created — a required safety grant that
+    /// can't be honored must stop the command, not silently shrink its access.
+    #[test]
+    fn to_policy_fails_on_missing_grant_and_never_creates_it() {
+        let temp_dir = tempfile::tempdir().expect("create temp dir");
+        let missing = temp_dir.path().join("AGENTS.md");
+
+        let wrap = SandboxWrap {
+            extra_write_paths: vec![settings::GrantedWritePath::resolved(
+                missing.clone(),
+                missing.clone(),
+            )],
+            ..Default::default()
+        };
+
+        let error = wrap
+            .to_policy()
+            .expect_err("a grant to a missing path must fail policy construction");
+        assert!(
+            format!("{error:#}").contains("AGENTS.md"),
+            "error should name the failing grant: {error:#}"
+        );
+        assert!(
+            !missing.exists(),
+            "policy construction must never create the granted path"
+        );
+    }
+
+    /// A baseline writable path (worktree root / scratch dir) that doesn't
+    /// exist must also fail: silently narrowing the sandbox would hand the
+    /// command less access than the model was told it has.
+    #[test]
+    fn to_policy_fails_on_missing_writable_path() {
+        let temp_dir = tempfile::tempdir().expect("create temp dir");
+        let missing = temp_dir.path().join("gone");
+
+        let wrap = SandboxWrap {
+            writable_paths: vec![missing.clone()],
+            ..Default::default()
+        };
+
+        wrap.to_policy()
+            .expect_err("a missing writable path must fail policy construction");
+        assert!(
+            !missing.exists(),
+            "policy construction must never create a writable path"
+        );
+    }
+
+    /// Protected paths are best-effort: an uncapturable one is dropped, never
+    /// fatal. That covers a missing path (`NotFound`) and one routed through a
+    /// regular file (`NotADirectory`) — the latter is the synthesized `.git` of
+    /// a single-file worktree (e.g. `settings.json/.git`). Unlike a writable
+    /// grant, a protection can't be materialized, and `.git` protection has an
+    /// inherent accepted loophole (`git init`), so failing here would only break
+    /// legitimate cases. Unit-level companion to the `settings.json/.git` NixOS
+    /// check.
+    #[test]
+    fn to_policy_skips_uncapturable_protected_paths() {
+        let temp_dir = tempfile::tempdir().expect("create temp dir");
+        let writable = temp_dir.path().join("writable");
+        std::fs::create_dir(&writable).expect("create writable dir");
+        let single_file_root = temp_dir.path().join("settings.json");
+        std::fs::write(&single_file_root, b"{}").expect("create single-file worktree root");
+
+        let wrap = SandboxWrap {
+            writable_paths: vec![writable],
+            protected_paths: vec![
+                temp_dir.path().join("no-such-.git"),
+                single_file_root.join(".git"),
+            ],
+            ..Default::default()
+        };
+
+        wrap.to_policy()
+            .expect("uncapturable protected paths must be dropped, not fail the policy");
+    }
+}

crates/agent/src/db.rs

@@ -93,9 +93,13 @@ pub struct DbThread {
 /// thread blob; round-trips with [`crate::sandboxing::ThreadSandboxGrants`].
 #[derive(Debug, Clone, Default, PartialEq, Serialize, Deserialize)]
 pub struct DbSandboxGrants {
-    /// Canonicalized paths granted write access; each covers its whole subtree.
+    /// Paths granted write access, each paired with the canonical
+    /// (symlink-resolved) target established when the grant was approved; each
+    /// covers its whole subtree. Legacy rows stored a bare path string per
+    /// entry, which still deserializes (as a grant with no resolved canonical)
+    /// via [`settings::GrantedWritePath`]'s string-or-object format.
     #[serde(default)]
-    pub write_paths: Vec<PathBuf>,
+    pub write_paths: Vec<settings::GrantedWritePath>,
     /// Host patterns granted network access, in canonical string form (e.g.
     /// `github.com`, `*.npmjs.org`). Parsed back into patterns on load.
     #[serde(default)]
@@ -947,7 +951,16 @@ mod tests {
             Utc.with_ymd_and_hms(2024, 1, 1, 0, 0, 0).unwrap(),
         );
         let grants = DbSandboxGrants {
-            write_paths: vec![PathBuf::from("/tmp/build")],
+            write_paths: vec![
+                // A legacy bare-string grant (no resolved canonical) and a grant
+                // carrying its resolved canonical, to exercise both forms of the
+                // string-or-object round-trip.
+                settings::GrantedWritePath::from_requested(PathBuf::from("/tmp/build")),
+                settings::GrantedWritePath::resolved(
+                    PathBuf::from("/tmp/link"),
+                    PathBuf::from("/tmp/real"),
+                ),
+            ],
             network_hosts: vec!["github.com".to_string(), "*.npmjs.org".to_string()],
             network_any_host: false,
             allow_fs_write_all: false,