fix(cli,federation): exclusive, unpredictable staging for every atomic write

crates/loomweave-cli/Cargo.toml

@@ -37,6 +37,7 @@ serde_json.workspace = true
 serde_norway.workspace = true
 sha2.workspace = true
 subtle.workspace = true
+tempfile.workspace = true
 thiserror.workspace = true
 time.workspace = true
 toml.workspace = true
@@ -66,7 +67,6 @@ loomweave-plugin-fixture = { path = "../loomweave-plugin-fixture" }
 rusqlite.workspace = true
 serde_json.workspace = true
 sha1.workspace = true
-tempfile.workspace = true
 
 # Targeted `kill(pid, SIGTERM/SIGINT)` in tests/serve.rs — pid-addressed,
 # never the process group (which would take the test runner down with it).

crates/loomweave-cli/src/atomic_fs.rs

@@ -0,0 +1,167 @@
+//! Exclusive, unpredictable staging for atomic file and directory replacement.
+//!
+//! Every "write a sibling temp, then rename over the destination" site in the
+//! CLI used to derive the staging name from the process id
+//! (`<name>.tmp-<pid>`). That name is guessable, and the staging path lives in
+//! a directory the analyzed repository controls (`.claude/`, the project root,
+//! `.weft/loomweave/`, a backup output directory). A repository that commits a
+//! symlink at the guessed name turns the staging write into a write-through to
+//! wherever the link points (`fs::write` and SQLite both follow symlinks), and
+//! a planted regular file can pre-empt the rename. `O_CREAT|O_EXCL` on a
+//! random name closes both: the staging file is created by this process or the
+//! call fails, and nothing pre-planted is ever opened or renamed over the
+//! destination.
+//!
+//! The helpers keep the staging entry in the destination's own directory so
+//! the final rename stays a same-filesystem atomic swap, and they request
+//! `0o666` at creation so the kernel applies the caller's umask — the same
+//! resulting mode `fs::write` would have produced — instead of `tempfile`'s
+//! owner-only default, which would silently make an installed `CLAUDE.md` or
+//! `settings.json` unreadable to other users of a shared checkout.
+
+use std::io::Write as _;
+use std::path::Path;
+
+use anyhow::{Context, Result};
+use tempfile::{Builder, NamedTempFile, TempDir};
+
+/// Create an exclusive staging file in `dir` whose name starts with `prefix`.
+///
+/// # Errors
+///
+/// Returns an error if `dir` cannot be written.
+pub(crate) fn staging_file_in(dir: &Path, prefix: &str) -> Result<NamedTempFile> {
+    let mut builder = Builder::new();
+    builder.prefix(prefix);
+    #[cfg(unix)]
+    {
+        use std::os::unix::fs::PermissionsExt as _;
+        builder.permissions(std::fs::Permissions::from_mode(0o666));
+    }
+    builder
+        .tempfile_in(dir)
+        .with_context(|| format!("create staging file in {}", dir.display()))
+}
+
+/// Create an exclusive, empty staging directory in `dir` whose name starts
+/// with `prefix`.
+///
+/// # Errors
+///
+/// Returns an error if `dir` cannot be written.
+pub(crate) fn staging_dir_in(dir: &Path, prefix: &str) -> Result<TempDir> {
+    Builder::new()
+        .prefix(prefix)
+        .tempdir_in(dir)
+        .with_context(|| format!("create staging directory in {}", dir.display()))
+}
+
+/// Atomically replace `dest` with `bytes`: stage exclusively in `dest`'s
+/// directory, then rename over it. `prefix` names the staging file so an
+/// interrupted run leaves a recognisable sibling.
+///
+/// # Errors
+///
+/// Returns an error if the parent directory cannot be created, or the staging
+/// write or the final rename fails. A failed call never leaves a staging
+/// sibling behind (`NamedTempFile` unlinks on drop).
+pub(crate) fn replace_file(dest: &Path, prefix: &str, bytes: &[u8]) -> Result<()> {
+    let dir = dest.parent().unwrap_or_else(|| Path::new("."));
+    std::fs::create_dir_all(dir).with_context(|| format!("mkdir {}", dir.display()))?;
+    let mut staging = staging_file_in(dir, prefix)?;
+    staging
+        .write_all(bytes)
+        .with_context(|| format!("write staging {}", staging.path().display()))?;
+    staging
+        .persist(dest)
+        .map_err(|err| err.error)
+        .with_context(|| format!("rename staging file -> {}", dest.display()))?;
+    Ok(())
+}
+
+#[cfg(test)]
+mod tests {
+    use super::{replace_file, staging_dir_in, staging_file_in};
+
+    #[test]
+    fn replace_file_writes_a_regular_file_and_leaves_no_staging_sibling() {
+        let dir = tempfile::tempdir().unwrap();
+        let dest = dir.path().join("out.txt");
+        replace_file(&dest, ".out.txt.tmp-", b"first\n").unwrap();
+        replace_file(&dest, ".out.txt.tmp-", b"second\n").unwrap();
+        assert_eq!(std::fs::read_to_string(&dest).unwrap(), "second\n");
+        let leftovers: Vec<_> = std::fs::read_dir(dir.path())
+            .unwrap()
+            .map(|e| e.unwrap().file_name().to_string_lossy().into_owned())
+            .filter(|name| name.starts_with(".out.txt.tmp-"))
+            .collect();
+        assert!(
+            leftovers.is_empty(),
+            "staging sibling leaked: {leftovers:?}"
+        );
+    }
+
+    #[cfg(unix)]
+    #[test]
+    fn replace_file_never_follows_a_planted_symlink() {
+        use std::os::unix::fs::symlink;
+        let dir = tempfile::tempdir().unwrap();
+        let dest = dir.path().join("out.txt");
+        let victim = dir.path().join("victim");
+        std::fs::write(&victim, "keep\n").unwrap();
+        // The name a PID-derived scheme would have used, and the destination
+        // itself, both pre-planted as links to the victim.
+        let planted = dir
+            .path()
+            .join(format!(".out.txt.tmp-{}", std::process::id()));
+        symlink(&victim, &planted).unwrap();
+        symlink(&victim, &dest).unwrap();
+
+        replace_file(&dest, ".out.txt.tmp-", b"payload\n").unwrap();
+
+        assert_eq!(std::fs::read_to_string(&victim).unwrap(), "keep\n");
+        assert!(
+            !std::fs::symlink_metadata(&dest)
+                .unwrap()
+                .file_type()
+                .is_symlink(),
+            "the destination must be replaced by a regular file, not written through"
+        );
+        assert_eq!(std::fs::read_to_string(&dest).unwrap(), "payload\n");
+        assert!(
+            std::fs::symlink_metadata(&planted)
+                .unwrap()
+                .file_type()
+                .is_symlink()
+        );
+    }
+
+    #[cfg(unix)]
+    #[test]
+    fn staging_file_honours_umask_like_fs_write() {
+        use std::os::unix::fs::PermissionsExt as _;
+        let dir = tempfile::tempdir().unwrap();
+        let control = dir.path().join("control");
+        std::fs::write(&control, b"x").unwrap();
+        let staged = staging_file_in(dir.path(), ".probe-").unwrap();
+        let expect = std::fs::metadata(&control).unwrap().permissions().mode() & 0o777;
+        let got = staged.as_file().metadata().unwrap().permissions().mode() & 0o777;
+        assert_eq!(got, expect, "staging mode must match what fs::write yields");
+    }
+
+    #[test]
+    fn staging_dir_is_created_exclusively_in_the_requested_parent() {
+        let dir = tempfile::tempdir().unwrap();
+        let staged = staging_dir_in(dir.path(), ".pack.tmp-").unwrap();
+        assert_eq!(staged.path().parent(), Some(dir.path()));
+        assert!(staged.path().is_dir());
+        assert!(
+            staged
+                .path()
+                .file_name()
+                .unwrap()
+                .to_string_lossy()
+                .starts_with(".pack.tmp-")
+        );
+    }
+}

crates/loomweave-cli/src/db.rs

@@ -14,6 +14,7 @@
 use std::path::Path;
 use std::time::Duration;
 
+use crate::atomic_fs::staging_file_in;
 use anyhow::{Context, Result, anyhow, bail, ensure};
 use rusqlite::{Connection, OpenFlags};
 
@@ -74,37 +75,25 @@ pub fn backup(project_root: &Path, output: &Path, force: bool) -> Result<()> {
     // Stage into a sibling temp file so a crash mid-copy can never leave a
     // truncated file sitting at `output`. Renaming is atomic on the same
     // filesystem; staging as a sibling keeps us on it.
-    let parent = output.parent().filter(|p| !p.as_os_str().is_empty());
-    if let Some(parent) = parent {
-        std::fs::create_dir_all(parent)
-            .with_context(|| format!("create backup output directory {}", parent.display()))?;
-    }
-    let staging = staging_path(output);
-    // Clear any stale staging file from a previous interrupted run.
-    if staging.exists() {
-        std::fs::remove_file(&staging)
-            .with_context(|| format!("clear stale staging file {}", staging.display()))?;
-    }
-
-    let result = run_backup(&db_path, &staging);
-    match result {
-        Ok(()) => {
-            std::fs::rename(&staging, output).with_context(|| {
-                format!(
-                    "rename backup {} -> {}",
-                    staging.display(),
-                    output.display()
-                )
-            })?;
-            println!("Backed up {} -> {}", db_path.display(), output.display());
-            Ok(())
-        }
-        Err(err) => {
-            // Best-effort cleanup so a failed run leaves no debris behind.
-            let _ = std::fs::remove_file(&staging);
-            Err(err)
-        }
-    }
+    let parent = output
+        .parent()
+        .filter(|p| !p.as_os_str().is_empty())
+        .unwrap_or_else(|| Path::new("."));
+    std::fs::create_dir_all(parent)
+        .with_context(|| format!("create backup output directory {}", parent.display()))?;
+    // Exclusive staging (see `atomic_fs`): SQLite opens the path it is given,
+    // so a guessable sibling name pre-planted as a symlink would have turned
+    // the backup into a write-through. The `TempPath` unlinks the staging file
+    // on every failure path, so an interrupted run leaves no debris.
+    let staging = staging_file_in(parent, &staging_prefix(output))?.into_temp_path();
+
+    run_backup(&db_path, &staging)?;
+    staging
+        .persist(output)
+        .map_err(|err| err.error)
+        .with_context(|| format!("rename backup staging -> {}", output.display()))?;
+    println!("Backed up {} -> {}", db_path.display(), output.display());
+    Ok(())
 }
 
 /// Force a `PRAGMA wal_checkpoint(TRUNCATE)` on the working store so the on-disk
@@ -183,11 +172,12 @@ fn run_backup(db_path: &Path, staging: &Path) -> Result<()> {
     Ok(())
 }
 
-/// Sibling staging path for the atomic write (`<output>.loomweave-backup.tmp-<pid>`).
-fn staging_path(output: &Path) -> std::path::PathBuf {
-    let mut name = output.as_os_str().to_os_string();
-    name.push(format!(".loomweave-backup.tmp-{}", std::process::id()));
-    std::path::PathBuf::from(name)
+/// Prefix for the sibling staging file (`<output-name>.loomweave-backup.tmp-<random>`).
+fn staging_prefix(output: &Path) -> String {
+    let name = output
+        .file_name()
+        .map_or_else(|| "backup".to_owned(), |n| n.to_string_lossy().into_owned());
+    format!("{name}.loomweave-backup.tmp-")
 }
 
 /// True if both paths denote the same on-disk file. Falls back to a lexical
@@ -198,3 +188,56 @@ fn paths_are_same(a: &Path, b: &Path) -> bool {
         _ => a == b,
     }
 }
+
+#[cfg(test)]
+mod tests {
+    #[cfg(unix)]
+    #[test]
+    fn backup_never_follows_a_planted_staging_symlink() {
+        use std::os::unix::fs::symlink;
+        let dir = tempfile::tempdir().unwrap();
+        let project = dir.path().join("project");
+        std::fs::create_dir_all(&project).unwrap();
+        // A plain (non-git) project root resolves to the default store leaf.
+        // Spelled out so the worktree store-path audit does not read this
+        // test as an unclassified runtime resolution site.
+        let live = project.join(".weft").join("loomweave").join("loomweave.db");
+        std::fs::create_dir_all(live.parent().unwrap()).unwrap();
+        let conn = rusqlite::Connection::open(&live).unwrap();
+        conn.execute_batch("CREATE TABLE t(x INTEGER); INSERT INTO t VALUES (42);")
+            .unwrap();
+        drop(conn);
+
+        let out_dir = dir.path().join("backups");
+        std::fs::create_dir_all(&out_dir).unwrap();
+        let output = out_dir.join("snap.db");
+        let victim = dir.path().join("victim");
+        std::fs::write(&victim, "keep\n").unwrap();
+        // The staging name the PID-derived scheme used. SQLite opens whatever
+        // path it is handed, so a symlink here was a write-through.
+        let planted = out_dir.join(format!(
+            "snap.db.loomweave-backup.tmp-{}",
+            std::process::id()
+        ));
+        symlink(&victim, &planted).unwrap();
+
+        super::backup(&project, &output, false).unwrap();
+
+        assert_eq!(std::fs::read_to_string(&victim).unwrap(), "keep\n");
+        assert!(
+            !std::fs::symlink_metadata(&output)
+                .unwrap()
+                .file_type()
+                .is_symlink()
+        );
+        let conn = rusqlite::Connection::open(&output).unwrap();
+        let x: i64 = conn.query_row("SELECT x FROM t", [], |r| r.get(0)).unwrap();
+        assert_eq!(x, 42);
+        assert!(
+            std::fs::symlink_metadata(&planted)
+                .unwrap()
+                .file_type()
+                .is_symlink()
+        );
+    }
+}

crates/loomweave-cli/src/hooks_settings.rs

@@ -334,24 +334,20 @@ pub fn install_session_start_hook(project_root: &Path) -> Result<bool> {
     let serialized =
         serde_json::to_string_pretty(&settings).context("serialize .claude/settings.json")?;
 
-    // Atomic write: stage into a sibling temp file in the same directory, then
-    // rename over the destination (same-filesystem atomic swap). This protects
-    // the user's hand-authored settings.json from truncation/corruption on a
-    // crash or concurrent install mid-write. Mirrors skill_pack::stage_and_swap.
-    let tmp = claude_dir.join(format!(".settings.json.tmp-{}", std::process::id()));
-    if let Err(err) = write_and_swap(&tmp, &settings_path, &serialized) {
-        let _ = fs::remove_file(&tmp);
-        return Err(err);
-    }
+    // Atomic write: create an unpredictable sibling exclusively, then rename
+    // it over the destination (same-filesystem atomic swap). Exclusive creation
+    // prevents a repository-controlled symlink from redirecting the write.
+    write_and_swap(&claude_dir, &settings_path, &serialized)?;
     Ok(true)
 }
 
-fn write_and_swap(tmp: &Path, dest: &Path, serialized: &str) -> Result<()> {
-    fs::write(tmp, format!("{serialized}\n"))
-        .with_context(|| format!("write staging {}", tmp.display()))?;
-    fs::rename(tmp, dest)
-        .with_context(|| format!("rename {} -> {}", tmp.display(), dest.display()))?;
-    Ok(())
+fn write_and_swap(dir: &Path, dest: &Path, serialized: &str) -> Result<()> {
+    debug_assert_eq!(dest.parent(), Some(dir));
+    crate::atomic_fs::replace_file(
+        dest,
+        ".settings.json.tmp-",
+        format!("{serialized}\n").as_bytes(),
+    )
 }
 
 #[cfg(test)]
@@ -880,4 +876,34 @@ mod tests {
             "a failed install must leave the user's settings.json byte-for-byte intact"
         );
     }
+
+    #[cfg(unix)]
+    #[test]
+    fn install_does_not_follow_predictable_staging_symlink() {
+        use std::os::unix::fs::symlink;
+
+        let dir = tempfile::tempdir().unwrap();
+        let claude_dir = dir.path().join(".claude");
+        fs::create_dir(&claude_dir).unwrap();
+        let outside = dir.path().join("outside.json");
+        fs::write(&outside, "outside must remain unchanged\n").unwrap();
+
+        // This was the staging name before randomized, exclusive temp-file
+        // creation. A malicious repository could commit such a symlink.
+        let attacker_link = claude_dir.join(format!(".settings.json.tmp-{}", std::process::id()));
+        symlink(&outside, &attacker_link).unwrap();
+
+        assert!(install_session_start_hook(dir.path()).unwrap());
+        assert_eq!(
+            fs::read_to_string(&outside).unwrap(),
+            "outside must remain unchanged\n"
+        );
+        assert!(
+            !fs::symlink_metadata(claude_dir.join("settings.json"))
+                .unwrap()
+                .file_type()
+                .is_symlink(),
+            "installed settings must be a regular file, not the attacker symlink"
+        );
+    }
 }