Fix lock rewrites changing file owner (#1635)

crates/socket-patch-cli/CLI_CONTRACT.md

@@ -201,6 +201,8 @@ For a **9.0 root lock**, the CLI ensures `pnpm-workspace.yaml` carries `trustLoc
 
 **Vendored write durability (v5.0)**: every write is atomic (stage + rename), but only the durable commit points — lockfiles, `go.mod`/`go.sum`, `pom.xml`, `nuget.config`, `package.json`, `pnpm-workspace.yaml`, `.cargo/config.toml`, the Python/Ruby manifests, `.socket/vendor/state.json` and `redirect-state.json` — are fsynced on write. The content-verified artifacts under `.socket/vendor/<eco>/<uuid>/` (patched copies, packed/rebuilt archives and sidecars, markers) are written without an fsync and made durable by one barrier (file + directory fsync, one `F_FULLFSYNC` per device on macOS) ahead of the next commit point — and, for an artifact rebuilt in place that no commit point follows, at the end of the vendored run's commit and when the command releases the apply lock — so a crash can only lose an artifact that no durable commit point names yet, which the next run redownloads.
 
+**Rewritten file ownership**: a hosted or vendored rewrite of a project file that already exists (its lockfiles, package manifests and `.npmrc`, and their `rollback` / `remove` / `vendor --revert` restores; not socket-patch's own `.socket/` records) replaces it with a staged file that keeps the old file's permission bits and, best-effort, its owner and group, like npm's write-file-atomic. A root run (a Docker bind mount, `sudo`) leaves a user's `poetry.lock` owned by that user. An unprivileged run can't give a file to another user; it keeps the old group when it belongs to that group, and otherwise the file takes the runner's owner and group, with no warning.
+
 **Vendored group commit (v5.0)**: `vendor`, `scan --mode vendored` and `get --mode vendored` capture every lockfile / manifest / config edit and every ledger save of the run in memory (reads inside the run see them) and commit them ONCE after the per-package loop — including the packages that succeeded in a run where others failed, so a completed run leaves the same files per-package commits would. Captured: every file under the project root outside `.socket/`, plus `.socket/vendor/state.json` and `.socket/vendor/redirect-state.json`; artifacts are written directly (see the durability note). A multi-file commit goes through a roll-forward journal, `.socket/vendor/.commit-journal.json` (the new bytes of every changed file, plus the bytes each replaces and their sha256; deleted once the commit completes). **Crash semantics**: before the journal is durable, nothing is committed — the lockfiles and ledgers are the pre-run ones and the run's artifacts are unreferenced orphans; after it, the next command that takes the apply lock replays the journal before reading anything (files already at their new bytes are left alone), so a locked command never observes a half-committed run. A journal that matches neither side of some file (edited by hand since the crash) is renamed to `.socket/vendor/.commit-journal.set-aside-<uuid>.json` (keeping every file's pre-commit bytes) and stderr says what was done (`Warning: an interrupted vendored run's commit could not be finished as written: …`): the edited files are never written over; when they all still carry the commit's own lines the rest of the commit is finished around them, when none of them does the files the crash had already replaced are put back to their pre-commit bytes, and otherwise nothing is applied. A journal that is unreadable, names a path outside the lockfiles and ledgers, or would write through a symbolic link is set aside with nothing applied. A replay that fails on I/O keeps the journal and fails the lock acquire (`lock_io`, naming the journal). Read-only commands that take no lock (`vex`, `list`) may observe the interrupted state until then. A re-vendor under a newer uuid removes the replaced uuid's dir only after the commit (its `vendor_stale_artifact_removed` event follows the run's per-package events), and a golang takeover removes the `.socket/go-patches/` copy only after the commit that repoints `go.mod`. A commit never renames over a symbolic link: when a changed file is a symlink, the whole commit is refused before anything is written, with the top-level error `redirect_symlinked_file_unsupported` (exit 1; a `--dry-run` predicts it with a `vendor_would_refuse_symlinked_file` advisory) — the artifact dirs the run's loop added under `.socket/vendor/` are removed, and every package it vendored is reported as a `failed` event with that code instead of `applied` (its per-package `skipped` advisories from the same run, such as `vendor_prebuilt_downloaded`, are dropped). A commit write failure is the top-level error `vendor_commit_failed` (exit 1; the pre-run lockfiles and ledger stay, the artifact dirs the run added are removed and its vendored packages are `failed` events with that code, the same way — unless putting back the files already replaced failed too, in which case the journal is kept, the next locked command finishes the commit, and the per-package events stand). `repair`, `vendor --revert` and `rollback` still save per entry.
 
 `scan --sync` is sugar for `--mode agent --prune` — the canonical single-flag agent-mode bot invocation. `scan --json --sync` discovers, applies, and reconciles state in one pass.

crates/socket-patch-cli/tests/in_process_redirect.rs

@@ -4307,6 +4307,39 @@ async fn readonly_socket_vendor_does_not_block_a_hosted_run() {
     vlt_hosted_common::assert_no_ledger(tmp.path());
 }
 
+/// A hosted scan run as root over a user's checkout (a Docker bind mount,
+/// `sudo`) rewrites the lock but leaves it owned by that user, as the
+/// package manager's own writer does (#1635): a root-owned lock locks its
+/// owner out of the next in-place `poetry add` / `npm install`.
+///
+/// unix-only, and skipped unless root: only root can give a file away.
+#[cfg(unix)]
+#[tokio::test]
+#[serial]
+async fn root_hosted_scan_keeps_the_lock_owner() {
+    use std::os::unix::fs::MetadataExt;
+    if unsafe { libc::geteuid() } != 0 {
+        eprintln!("skipping: giving a file to another uid needs root");
+        return;
+    }
+    let server = MockServer::start().await;
+    mock_discovery(&server).await;
+    mock_reference(&server).await;
+    mock_view(&server).await;
+    let tmp = tempfile::tempdir().unwrap();
+    write_project(tmp.path());
+    let lock_path = tmp.path().join("package-lock.json");
+    std::os::unix::fs::chown(&lock_path, Some(1000), Some(1001)).unwrap();
+
+    let out = run_hosted_json_scan(tmp.path(), &server).await;
+    let stdout = String::from_utf8_lossy(&out.stdout);
+    assert_eq!(out.status.code(), Some(0), "stdout=\n{stdout}");
+    let lock = std::fs::read_to_string(&lock_path).unwrap();
+    assert!(lock.contains(HOSTED_URL), "{lock}");
+    let meta = std::fs::metadata(&lock_path).unwrap();
+    assert_eq!((meta.uid(), meta.gid()), (1000, 1001));
+}
+
 /// A MALFORMED pre-v5 redirect ledger (torn write, truncation, bad
 /// hand-edit) is IGNORED by v5 hosted scan: never read for planning, never
 /// quarantined, never an error. The run succeeds and redirects, the torn

crates/socket-patch-core/src/utils/fs.rs

@@ -430,7 +430,8 @@ pub(crate) async fn atomic_write_bytes(path: &Path, content: &[u8]) -> std::io::
 }
 
 /// [`atomic_write_bytes`], but the new inode keeps the destination's existing
-/// permission bits (when the destination exists).
+/// permission bits and, where the caller may set them, its owner and group
+/// (when the destination exists).
 ///
 /// The rename swaps in a fresh stage inode created with umask defaults, so the
 /// plain writer resets a user-owned file's mode — a 0600 private package.json
@@ -626,7 +627,8 @@ struct WriteOpts {
     /// A durable commit point: run the durability barrier first, fsync the
     /// stage, and fsync the parent directory after the rename.
     durable: bool,
-    /// The new inode keeps the destination's permission bits.
+    /// The new inode keeps the destination's permission bits and, where
+    /// the caller may set them, its owner and group.
     preserve_mode: bool,
     /// Record the written path for the next durability barrier.
     record: bool,
@@ -714,10 +716,34 @@ fn stage_open_options(perms: Option<&std::fs::Permissions>) -> std::fs::OpenOpti
     options
 }
 
+/// Give a mode-preserving stage the destination's owner and group before
+/// the rename, best-effort like npm's write-file-atomic: the fresh inode
+/// belongs to the euid/egid of this process, so a root run over a user's
+/// checkout (a Docker bind mount, `sudo`) would otherwise hand their
+/// `poetry.lock` to root, and a run in a shared group checkout would move it
+/// to the runner's primary group (#1635). An unprivileged caller can't give
+/// a file away (EPERM), but it may still keep the group when it belongs to
+/// it; failing that, the stage keeps the runner's ownership, as before.
+#[cfg(unix)]
+fn keep_owner(stage: &std::fs::File, dest: &std::fs::Metadata) {
+    use std::os::unix::fs::{fchown, MetadataExt};
+    let Ok(now) = stage.metadata() else {
+        return;
+    };
+    let (uid, gid) = (dest.uid(), dest.gid());
+    if (now.uid(), now.gid()) == (uid, gid) {
+        return;
+    }
+    if fchown(stage, Some(uid), Some(gid)).is_err() && now.gid() != gid {
+        let _ = fchown(stage, None::<u32>, Some(gid));
+    }
+}
+
 /// Stage `content` next to `path` and rename it over `path` — the one
 /// stage-and-rename core every writer above runs. `preserve_mode` keeps the
-/// destination's permission bits; `durable` fsyncs the stage before the
-/// rename and the parent directory after it.
+/// destination's permission bits and, where the caller may set them, its
+/// owner and group; `durable` fsyncs the stage before the rename and the
+/// parent directory after it.
 ///
 /// Off Unix every stage is fsynced here, through its own writable handle:
 /// Windows' `FlushFileBuffers` needs write access, so the barrier cannot
@@ -732,9 +758,10 @@ fn stage_and_rename_blocking(
 ) -> std::io::Result<()> {
     use std::io::Write as _;
     let parent = path.parent().unwrap_or_else(|| Path::new("."));
-    let perms = preserve_mode
-        .then(|| std::fs::metadata(path).ok().map(|m| m.permissions()))
+    let dest = preserve_mode
+        .then(|| std::fs::metadata(path).ok())
         .flatten();
+    let perms = dest.as_ref().map(|m| m.permissions());
     let sync_file = durable || !super::durability::DEFERS_FILE_SYNC;
     let stage = stage_path(path, ".socket-stage-");
 
@@ -746,6 +773,13 @@ fn stage_and_rename_blocking(
         // quota) surfaces here, before the truncated stage could be renamed
         // over the intact target.
         file.write_all(content)?;
+        // Owner before the fsync, so a durable write syncs it too, and
+        // before the mode: `chown(2)` clears setuid/setgid, so the chmod
+        // below must run last to restore the bits exactly.
+        #[cfg(unix)]
+        if let Some(dest) = &dest {
+            keep_owner(&file, dest);
+        }
         if sync_file {
             file.sync_all()?;
         }
@@ -1002,6 +1036,84 @@ mod tests {
         assert_eq!(tokio::fs::read(&fresh).await.unwrap(), b"x");
     }
 
+    /// A mode-preserving write keeps the destination's owner and group too
+    /// (#1635): a root run over a user's `poetry.lock` (a Docker bind mount,
+    /// `sudo`) must not hand the file to root, or Poetry can no longer
+    /// rewrite its own lock in place. Every preserving writer runs the one
+    /// stage core, so each is checked; the plain writer keeps umask/euid
+    /// defaults. The chown runs before the chmod, so a setuid bit survives.
+    #[cfg(unix)]
+    #[tokio::test]
+    async fn preserving_writes_keep_the_destination_owner() {
+        use std::os::unix::fs::{MetadataExt, PermissionsExt};
+        if unsafe { libc::geteuid() } != 0 {
+            eprintln!("skipping: giving a file to another uid needs root");
+            return;
+        }
+        let tmp = tempfile::tempdir().unwrap();
+        let fresh = |name: &str| {
+            let path = tmp.path().join(name);
+            std::fs::write(&path, b"old").unwrap();
+            std::os::unix::fs::chown(&path, Some(1000), Some(1001)).unwrap();
+            std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o4754)).unwrap();
+            path
+        };
+        let owner_and_mode = |path: &Path| {
+            let meta = std::fs::metadata(path).unwrap();
+            (meta.uid(), meta.gid(), meta.mode() & 0o7777)
+        };
+
+        let path = fresh("poetry.lock");
+        atomic_write_bytes_preserving_mode(&path, b"new")
+            .await
+            .unwrap();
+        assert_eq!(std::fs::read(&path).unwrap(), b"new");
+        assert_eq!(owner_and_mode(&path), (1000, 1001, 0o4754));
+
+        let path = fresh("sync.lock");
+        atomic_write_sync(&path, b"new", true).unwrap();
+        assert_eq!(owner_and_mode(&path), (1000, 1001, 0o4754));
+
+        let path = fresh("unsynced.lock");
+        atomic_write_unsynced(&path, b"new", true).await.unwrap();
+        assert_eq!(owner_and_mode(&path), (1000, 1001, 0o4754));
+
+        let path = fresh("plain");
+        atomic_write_bytes(&path, b"new").await.unwrap();
+        let (uid, _, _) = owner_and_mode(&path);
+        assert_eq!(uid, 0, "the plain writer is not preserving");
+    }
+
+    /// An unprivileged run can't give a file to another user, but it can
+    /// keep the group of a shared checkout (`ubuntu:devs` must not become
+    /// `ubuntu:ubuntu`, locking the other `devs` out, #1635), since the
+    /// caller may chgrp its own file to any group it belongs to.
+    #[cfg(unix)]
+    #[tokio::test]
+    async fn preserving_write_keeps_a_supplementary_group() {
+        use std::os::unix::fs::MetadataExt;
+        let egid = unsafe { libc::getegid() };
+        let count = unsafe { libc::getgroups(0, std::ptr::null_mut()) };
+        let mut groups = vec![0 as libc::gid_t; count.max(0) as usize];
+        let count = unsafe { libc::getgroups(count, groups.as_mut_ptr()) };
+        groups.truncate(count.max(0) as usize);
+        let Some(&group) = groups.iter().find(|g| **g != egid) else {
+            eprintln!("skipping: the caller has no supplementary group");
+            return;
+        };
+        let tmp = tempfile::tempdir().unwrap();
+        let path = tmp.path().join("poetry.lock");
+        std::fs::write(&path, b"old").unwrap();
+        std::os::unix::fs::chown(&path, None, Some(group)).unwrap();
+
+        atomic_write_bytes_preserving_mode(&path, b"new")
+            .await
+            .unwrap();
+        let meta = std::fs::metadata(&path).unwrap();
+        assert_eq!(meta.uid(), unsafe { libc::geteuid() });
+        assert_eq!(meta.gid(), group, "the shared group must survive");
+    }
+
     /// The stage of a mode-preserving write is CREATED with the preserved
     /// bits, never the 0666 & ~umask default: the full new content (a
     /// `.npmrc` auth token) is written and fsynced into it before the final