From b9f475471ab9f9be3666601269a956a78ca828c4 Mon Sep 17 00:00:00 2001 From: skylartaylor <26588250+skylartaylor@users.noreply.github.com> Date: Tue, 13 Oct 2026 02:26:24 +0200 Subject: [PATCH 1/2] Support normal new-file permissions with private atomic staging --- .github/workflows/atomic.yml | 35 ++++ crates/atomic/README.md | 35 ++++ crates/atomic/src/lib.rs | 261 ++++++++++++++++++++++++++++- crates/atomic/tests/permissions.rs | 132 +++++++++++++++ 4 files changed, 459 insertions(+), 4 deletions(-) create mode 100644 .github/workflows/atomic.yml create mode 100644 crates/atomic/tests/permissions.rs diff --git a/.github/workflows/atomic.yml b/.github/workflows/atomic.yml new file mode 100644 index 00000000..b41790b7 --- /dev/null +++ b/.github/workflows/atomic.yml @@ -0,0 +1,35 @@ +name: Atomic file writes + +on: + workflow_dispatch: + push: + branches: [main] + paths: ['crates/atomic/**', 'Cargo.toml', 'Cargo.lock', '.github/workflows/atomic.yml'] + pull_request: + paths: ['crates/atomic/**', 'Cargo.toml', 'Cargo.lock', '.github/workflows/atomic.yml'] + +permissions: + contents: read + +jobs: + atomic: + timeout-minutes: 15 + strategy: + fail-fast: false + matrix: + os: [ubuntu-latest, windows-latest, macos-latest] + runs-on: ${{ matrix.os }} + steps: + - uses: actions/checkout@v4 + - uses: dtolnay/rust-toolchain@stable + with: + components: rustfmt, clippy + - name: Install ACL tools + if: runner.os == 'Linux' + run: sudo apt-get update && sudo apt-get install -y acl + - run: cargo fmt --check + - run: cargo clippy -p craft-atomic --all-targets --locked -- -D warnings + - run: cargo test -p craft-atomic --locked -- --nocapture + - name: Check inherited Linux ACLs + if: runner.os == 'Linux' + run: cargo test -p craft-atomic --locked -- --ignored --nocapture diff --git a/crates/atomic/README.md b/crates/atomic/README.md index 423dc52f..c8973724 100644 --- a/crates/atomic/README.md +++ b/crates/atomic/README.md @@ -63,6 +63,41 @@ let path = Path::new("export.png"); write_new(path, b"png bytes...").expect("exclusive write"); ``` +### Normal Permissions for Documents and Exports + +The free functions and `WriteOptions::default()` keep new files private. Settings and recovery +backups can continue using them. For a new user document or export that should follow normal +file-creation permissions, opt in explicitly: + +```rust +use std::path::Path; +use craft_atomic::{NewFilePermissions, WriteOptions}; + +let documents = WriteOptions { new_file_permissions: NewFilePermissions::PlatformDefault }; +documents.atomic_write(Path::new("project.craft"), b"document bytes")?; +documents.write_new(Path::new("export.pdf"), b"pdf bytes")?; +# Ok::<(), std::io::Error>(()) +``` + +`atomic_write_with` and `write_new_with` on these options also support streaming. On Unix, +the library samples normal creation with mode `0666` using a separate, empty file beside the +destination. The OS applies the process umask or the directory's default ACL; process-global +umask is never changed. The empty probe is removed before a distinct `0600` payload stage is +created. Even an open handle to the probe never receives document bytes. This follows the +[Qt QSaveFile approach](https://github.com/qt/qtbase/blob/72b2a0c2f59014a34c2c433fc1fb1d18cff7ec48/src/corelib/io/qsavefile.cpp). + +After writing finishes, the selected final permissions are applied before the file flush and +publication. On replacement, the existing Unix mode always takes precedence. On new files, +`PlatformDefault` takes precedence over callback-selected modes; the private default keeps +the original callback behavior. The exclusive-copy fallback still starts private and applies +final permissions after copying. Other platforms keep their existing creation behavior. + +Sampling adds one empty-file create/stat/remove per new file with this policy, independent of +payload size. A sampling, probe-cleanup or permission-setting failure aborts the save; the library +does not guess permissions. As with ordinary file creation, this samples the directory policy +at creation time; later umask or directory-policy changes are not tracked. Matching a sampled +Unix mode does not promise preservation of all ACLs or other metadata across replacements. + ### Fault Injection & Testing ```rust diff --git a/crates/atomic/src/lib.rs b/crates/atomic/src/lib.rs index cc4b9d6e..038b055b 100644 --- a/crates/atomic/src/lib.rs +++ b/crates/atomic/src/lib.rs @@ -51,6 +51,13 @@ pub trait AtomicFs { /// Create `path`, which must not exist, and open it for writing. /// On Unix, create it with owner-only permissions so payload written afterward is private. fn create_new(&self, path: &Path) -> io::Result; + /// Exclusively create an empty permissions probe using normal file-creation permissions. + /// The probe is never used for payload and is removed before staging begins. On Unix, + /// request mode `0666` so the OS applies the process umask or directory's default ACL. + /// Existing implementations can leave this unsupported when using the private default. + fn create_permissions_probe(&self, _path: &Path) -> io::Result { + Err(io::Error::new(io::ErrorKind::Unsupported, "normal file-creation permissions are not supported")) + } /// Flush a written file to durable storage. fn sync(&self, file: &File) -> io::Result<()>; /// Rename `from` over `to`, replacing it atomically. @@ -61,7 +68,7 @@ pub trait AtomicFs { fn remove(&self, path: &Path) -> io::Result<()>; /// Flush a directory entry change to durable storage (Unix); no-op elsewhere. fn sync_dir(&self, dir: &Path) -> io::Result<()>; - /// Give the open temporary file the permissions of the file it replaces. + /// Give the open temporary file its final permissions. fn set_permissions(&self, file: &File, perms: std::fs::Permissions) -> io::Result<()>; } @@ -83,6 +90,10 @@ impl AtomicFs for RealFs { options.open(path) } + fn create_permissions_probe(&self, path: &Path) -> io::Result { + OpenOptions::new().write(true).create_new(true).open(path) + } + fn sync(&self, file: &File) -> io::Result<()> { sync_file(file) } @@ -161,6 +172,53 @@ impl RenameRetry { } } +/// Final permissions for a newly created file on Unix. Existing destination modes are always +/// preserved. On other platforms, file creation keeps the existing platform behavior. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] +pub enum NewFilePermissions { + /// Keep the private staging mode (`0600`, subject to umask/default ACL), unless the streaming + /// callback selects another mode. This is the behavior of the original free functions. + #[default] + Private, + /// Use the permissions normal creation with mode `0666` would receive in the destination + /// directory. The OS applies its umask/default ACL; the library never changes process umask. + /// These permissions are applied after the callback, before syncing and publication. + PlatformDefault, +} + +/// Permission policy for document saves and exports. Defaults to private new files, matching +/// [`atomic_write`] and [`write_new`]. Select [`NewFilePermissions::PlatformDefault`] for normal +/// user-file creation permissions while retaining private payload staging. +#[derive(Debug, Clone, Copy, Default)] +pub struct WriteOptions { + /// Final permissions for new files; ignored when replacing an existing Unix file. + pub new_file_permissions: NewFilePermissions, +} + +impl WriteOptions { + /// [`atomic_write`] with this new-file permission policy. + pub fn atomic_write(self, path: &Path, bytes: &[u8]) -> io::Result<()> { + self.atomic_write_with(path, |f| f.write_all(bytes)) + } + + /// [`atomic_write_with`] with this policy. Existing modes and `PlatformDefault` are applied + /// after the callback, overriding callback-selected permissions. `Private` keeps the + /// original callback behavior for new files. + pub fn atomic_write_with(self, path: &Path, write_fn: impl FnOnce(&mut File) -> io::Result<()>) -> io::Result<()> { + replace_with_options(&RealFs, RenameRetry::platform(), path, self.new_file_permissions, write_fn) + } + + /// [`write_new`] with this new-file permission policy; never replaces an existing file. + pub fn write_new(self, path: &Path, bytes: &[u8]) -> io::Result<()> { + self.write_new_with(path, |f| f.write_all(bytes)) + } + + /// [`write_new_with`] with this policy. `PlatformDefault` is applied after the callback. + pub fn write_new_with(self, path: &Path, write_fn: impl FnOnce(&mut File) -> io::Result<()>) -> io::Result<()> { + create_with_options(&RealFs, path, self.new_file_permissions, write_fn) + } +} + /// Atomically writes `bytes` to `path`, replacing any existing content. /// /// If write or rename fails, the target file is untouched and any temporary staging file is removed. @@ -188,6 +246,16 @@ pub fn atomic_write_custom(fs: &dyn AtomicFs, retry: RenameRetry, path: &Path, b /// The replace path behind every `atomic_write*` function: stage the new content beside the /// target, then rename it over the target. fn replace(fs: &dyn AtomicFs, retry: RenameRetry, path: &Path, fill: impl FnOnce(&mut File) -> io::Result<()>) -> io::Result<()> { + replace_with_options(fs, retry, path, NewFilePermissions::Private, fill) +} + +fn replace_with_options( + fs: &dyn AtomicFs, + retry: RenameRetry, + path: &Path, + new_permissions: NewFilePermissions, + fill: impl FnOnce(&mut File) -> io::Result<()>, +) -> io::Result<()> { let target = resolve_links(path)?; let (dir, name) = split(&target, path)?; let existing = std::fs::metadata(&target).ok(); @@ -196,7 +264,7 @@ fn replace(fs: &dyn AtomicFs, retry: RenameRetry, path: &Path, fill: impl FnOnce check_writable(&target).map_err(|e| read_only_error(e, path))?; } - let tmp = stage(fs, &dir, &name, path, existing.as_ref(), fill)?; + let tmp = stage(fs, &dir, &name, path, existing.as_ref(), new_permissions, fill)?; if let Err(e) = retry_rename(retry, || fs.rename(&tmp, &target)) { let _ = fs.remove(&tmp); @@ -241,11 +309,15 @@ fn stage( name: &str, path: &Path, replaced: Option<&std::fs::Metadata>, + new_permissions: NewFilePermissions, fill: impl FnOnce(&mut File) -> io::Result<()>, ) -> io::Result { let existed = replaced.is_some(); // Windows files carry only a read-only flag, and a read-only file can't be replaced. - let perms = replaced.filter(|_| cfg!(unix)).map(|m| m.permissions()); + let mut perms = replaced.filter(|_| cfg!(unix)).map(|m| m.permissions()); + if cfg!(unix) && !existed && new_permissions == NewFilePermissions::PlatformDefault { + perms = Some(sample_permissions(fs, dir, name).map_err(|e| write_error(e, path, dir, false))?); + } for _ in 0..8 { let tmp = dir.join(temp_name(name)); let mut file = match fs.create_new(&tmp) { @@ -266,6 +338,26 @@ fn stage( Err(io::Error::new(io::ErrorKind::AlreadyExists, format!("{}: could not create a temporary file", dir.display()))) } +/// Ask the OS for normal creation permissions without changing process-global umask. An empty +/// probe also observes directory default ACLs, which can override umask. Remove it before +/// creating a separate private payload inode: a third party holding the probe open must never +/// receive document bytes. This is the approach used by Qt's QSaveFile. +fn sample_permissions(fs: &dyn AtomicFs, dir: &Path, name: &str) -> io::Result { + for _ in 0..8 { + let probe = dir.join(temp_name(name)); + let file = match fs.create_permissions_probe(&probe) { + Ok(f) => f, + Err(e) if e.kind() == io::ErrorKind::AlreadyExists => continue, + Err(e) => return Err(e), + }; + let sampled = file.metadata().map(|m| m.permissions()); + drop(file); + let removed = fs.remove(&probe); + return sampled.and_then(|perms| removed.map(|()| perms)); + } + Err(io::Error::new(io::ErrorKind::AlreadyExists, format!("{}: could not create a permissions probe", dir.display()))) +} + /// Exclusively creates and writes `path` without replacing existing files. pub fn write_new(path: &Path, bytes: &[u8]) -> io::Result<()> { write_new_with(path, |f| f.write_all(bytes)) @@ -290,13 +382,22 @@ pub fn write_new_custom(fs: &dyn AtomicFs, path: &Path, bytes: &[u8]) -> io::Res /// then give it the target's name with a hard link, which fails rather than replace a file that /// appeared in the meantime. Volumes without hard links get a copy into a newly created file. fn create(fs: &dyn AtomicFs, path: &Path, fill: impl FnOnce(&mut File) -> io::Result<()>) -> io::Result<()> { + create_with_options(fs, path, NewFilePermissions::Private, fill) +} + +fn create_with_options( + fs: &dyn AtomicFs, + path: &Path, + new_permissions: NewFilePermissions, + fill: impl FnOnce(&mut File) -> io::Result<()>, +) -> io::Result<()> { let target = resolve_links(path)?; let (dir, name) = split(&target, path)?; if target.exists() { return Err(write_error(io::Error::from(io::ErrorKind::AlreadyExists), path, &dir, true)); } - let tmp = stage(fs, &dir, &name, path, None, fill)?; + let tmp = stage(fs, &dir, &name, path, None, new_permissions, fill)?; let placed = match fs.hard_link(&tmp, &target) { Err(e) if e.kind() != io::ErrorKind::AlreadyExists => copy_new(fs, &tmp, &target), @@ -501,11 +602,30 @@ mod tests { no_hard_links: bool, fail_permissions: bool, fail_sync_dir: bool, + fail_probe: bool, + probe_collisions: Cell, + fail_probe_remove: bool, + probe_path: std::cell::RefCell>, + probe_handle: std::cell::RefCell>, /// Another process creates the target just before the hard link. created_meanwhile: Option<&'static [u8]>, } impl AtomicFs for Faulty { + fn create_permissions_probe(&self, path: &Path) -> io::Result { + self.events.borrow_mut().push("probe"); + if self.fail_probe { + return Err(io::Error::new(io::ErrorKind::PermissionDenied, "injected: probe creation failed")); + } + if self.probe_collisions.get() > 0 { + self.probe_collisions.set(self.probe_collisions.get() - 1); + return Err(io::Error::from(io::ErrorKind::AlreadyExists)); + } + let file = RealFs.create_permissions_probe(path)?; + *self.probe_path.borrow_mut() = Some(path.to_path_buf()); + *self.probe_handle.borrow_mut() = Some(file.try_clone()?); + Ok(file) + } fn create_new(&self, path: &Path) -> io::Result { self.events.borrow_mut().push("create_new"); let file = RealFs.create_new(path)?; @@ -548,6 +668,9 @@ mod tests { } fn remove(&self, path: &Path) -> io::Result<()> { self.events.borrow_mut().push("remove"); + if self.fail_probe_remove && self.probe_path.borrow().as_deref() == Some(path) { + return Err(io::Error::new(io::ErrorKind::PermissionDenied, "injected: probe cleanup failed")); + } RealFs.remove(path) } fn sync_dir(&self, dir: &Path) -> io::Result<()> { @@ -740,6 +863,136 @@ mod tests { assert_no_temp(&d.0); } + #[cfg(unix)] + #[test] + fn normal_permissions_survive_exclusive_copy_and_probe_never_receives_payload() -> io::Result<()> { + use std::os::unix::fs::PermissionsExt; + use std::process::Command; + if std::env::var_os("CRAFT_ATOMIC_COPY_CHILD").is_none() { + // Test both publication paths under real umasks without changing the test runner's + // process-global mask. In particular, 002 must produce a shared 0664 final file. + for mask in ["000", "002", "022", "027", "077"] { + let output = Command::new("sh") + .args([ + "-c", + "umask \"$1\"; exec \"$2\" --exact tests::normal_permissions_survive_exclusive_copy_and_probe_never_receives_payload --nocapture", + "copy-permissions", + mask, + ]) + .arg(std::env::current_exe()?) + .env("CRAFT_ATOMIC_COPY_CHILD", "1") + .output()?; + assert!(output.status.success(), "umask {mask}: {}", String::from_utf8_lossy(&output.stdout)); + assert!(String::from_utf8_lossy(&output.stdout).contains("1 passed")); + } + return Ok(()); + } + let d = TempDir::new("normal-copy"); + let reference = d.0.join("reference"); + std::fs::write(&reference, b"normal creation")?; + let expected = std::fs::metadata(reference)?.permissions().mode() & 0o777; + for no_hard_links in [false, true] { + let p = d.0.join(format!("export-{no_hard_links}")); + let fs = Faulty { no_hard_links, probe_collisions: Cell::new(2), ..Default::default() }; + create_with_options(&fs, &p, NewFilePermissions::PlatformDefault, |f| { + assert_eq!(f.metadata()?.permissions().mode() & 0o077, 0); + f.write_all(b"document payload") + })?; + assert_eq!(std::fs::metadata(&p)?.permissions().mode() & 0o777, expected); + assert_eq!(std::fs::read(&p)?, b"document payload"); + let probe = fs.probe_handle.borrow(); + assert_eq!(probe.as_ref().ok_or_else(|| io::Error::other("missing probe handle"))?.metadata()?.len(), 0); + assert!(!fs.probe_path.borrow().as_ref().is_some_and(|p| p.exists())); + assert!(fs.created_modes.borrow().iter().all(|m| m & 0o077 == 0), "stage and copy destination must start private"); + } + assert_no_temp(&d.0); + Ok(()) + } + + #[cfg(unix)] + #[test] + fn normal_policy_preserves_existing_modes_without_probing() -> io::Result<()> { + use std::os::unix::fs::PermissionsExt; + let d = TempDir::new("normal-existing"); + for mode in [0o600, 0o640, 0o664] { + let p = d.0.join(format!("existing-{mode:o}")); + std::fs::write(&p, ORIGINAL)?; + std::fs::set_permissions(&p, std::fs::Permissions::from_mode(mode))?; + let fs = Faulty { fail_probe: true, ..Default::default() }; + replace_with_options(&fs, NO_RETRY, &p, NewFilePermissions::PlatformDefault, |f| f.write_all(b"replacement"))?; + assert_eq!(std::fs::metadata(&p)?.permissions().mode() & 0o777, mode); + assert!(!fs.events.borrow().contains(&"probe")); + } + assert_no_temp(&d.0); + Ok(()) + } + + #[cfg(unix)] + #[test] + fn normal_permission_failures_abort_before_publication() -> io::Result<()> { + let d = TempDir::new("normal-failures"); + for exclusive in [false, true] { + for (tag, fs, kind, callback_runs) in [ + ("probe", Faulty { fail_probe: true, ..Default::default() }, io::ErrorKind::PermissionDenied, false), + ("collisions", Faulty { probe_collisions: Cell::new(8), ..Default::default() }, io::ErrorKind::AlreadyExists, false), + ("cleanup", Faulty { fail_probe_remove: true, ..Default::default() }, io::ErrorKind::PermissionDenied, false), + ("permissions", Faulty { fail_permissions: true, ..Default::default() }, io::ErrorKind::PermissionDenied, true), + ("sync", Faulty { fail_sync: true, ..Default::default() }, io::ErrorKind::StorageFull, true), + ] { + let p = d.0.join(format!("{tag}-{exclusive}")); + let called = Cell::new(false); + let fill = |f: &mut File| { + called.set(true); + f.write_all(b"payload") + }; + let result = if exclusive { + create_with_options(&fs, &p, NewFilePermissions::PlatformDefault, fill) + } else { + replace_with_options(&fs, NO_RETRY, &p, NewFilePermissions::PlatformDefault, fill) + }; + assert_eq!(result.map_err(|e| e.kind()), Err(kind), "{tag}"); + assert_eq!(called.get(), callback_runs, "{tag}"); + assert!(!p.exists(), "{tag}"); + if fs.fail_probe_remove { + // The only leftover is an empty probe whose removal was explicitly denied. + let probe = fs.probe_path.borrow(); + let probe = probe.as_ref().ok_or_else(|| io::Error::other("missing probe path"))?; + assert_eq!(std::fs::metadata(probe)?.len(), 0); + std::fs::remove_file(probe)?; + } + assert_no_temp(&d.0); + } + } + Ok(()) + } + + #[cfg(target_os = "linux")] + #[test] + #[ignore = "requires setfacl/getfacl and a filesystem with POSIX ACL support"] + fn normal_permissions_follow_inherited_acl_with_hard_links_and_copy() -> io::Result<()> { + use std::process::Command; + let d = TempDir::new("normal-acl"); + let output = Command::new("setfacl").args(["-m", "d:u::rwx,d:u:12345:r--,d:g::r-x,d:m::r-x,d:o::---"]).arg(&d.0).output()?; + assert!(output.status.success(), "{}", String::from_utf8_lossy(&output.stderr)); + let acl = |path: &Path| -> io::Result> { + let output = Command::new("getfacl").args(["-c", "-n"]).arg(path).output()?; + assert!(output.status.success(), "{}", String::from_utf8_lossy(&output.stderr)); + Ok(output.stdout) + }; + let reference = d.0.join("reference"); + std::fs::write(&reference, b"normal creation")?; + let expected = acl(&reference)?; + for no_hard_links in [false, true] { + let fs = Faulty { no_hard_links, ..Default::default() }; + let p = d.0.join(format!("export-{no_hard_links}")); + create_with_options(&fs, &p, NewFilePermissions::PlatformDefault, |f| f.write_all(b"export"))?; + assert_eq!(acl(&p)?, expected, "same inherited ACL as ordinary creation"); + assert_eq!(std::fs::read(&p)?, b"export"); + } + assert_no_temp(&d.0); + Ok(()) + } + #[test] fn retry_delays_are_capped() { let d = TempDir::new("cap"); diff --git a/crates/atomic/tests/permissions.rs b/crates/atomic/tests/permissions.rs new file mode 100644 index 00000000..3ccd35ac --- /dev/null +++ b/crates/atomic/tests/permissions.rs @@ -0,0 +1,132 @@ +#![cfg(unix)] + +use craft_atomic::{NewFilePermissions, WriteOptions, atomic_write, write_new}; +use std::fs::{self, File, Permissions}; +use std::io::{self, Write}; +use std::os::unix::fs::PermissionsExt; +use std::path::{Path, PathBuf}; +use std::process::Command; +use std::sync::atomic::{AtomicU64, Ordering}; + +struct TempDir(PathBuf); +impl TempDir { + fn new() -> io::Result { + static N: AtomicU64 = AtomicU64::new(0); + let path = std::env::temp_dir().join(format!("craft-permissions-{}-{}", std::process::id(), N.fetch_add(1, Ordering::Relaxed))); + fs::create_dir(&path)?; + Ok(Self(path)) + } +} +impl Drop for TempDir { + fn drop(&mut self) { + let _ = fs::remove_dir_all(&self.0); + } +} + +fn mode(path: &Path) -> io::Result { + Ok(fs::metadata(path)?.permissions().mode() & 0o777) +} + +/// Set umask only in a separate shell process; changing it in a parallel test process races. +fn child(dir: &Path, mask: &str, expected: &str, check_acl: bool) -> io::Result<()> { + let output = Command::new("sh") + .args(["-c", "umask \"$1\"; exec \"$2\" --exact permission_child --nocapture", "permissions", mask]) + .arg(std::env::current_exe()?) + .env("CRAFT_PERMISSION_TEST_DIR", dir) + .env("CRAFT_PERMISSION_TEST_EXPECTED", expected) + .env("CRAFT_PERMISSION_TEST_ACL", if check_acl { "1" } else { "0" }) + .output()?; + assert!(output.status.success(), "umask {mask}:\n{}\n{}", String::from_utf8_lossy(&output.stdout), String::from_utf8_lossy(&output.stderr)); + assert!(String::from_utf8_lossy(&output.stdout).contains("1 passed"), "child must execute its test"); + Ok(()) +} + +#[test] +fn normal_creation_respects_umask_and_preserves_private_staging() -> io::Result<()> { + for (mask, expected) in [("000", "666"), ("002", "664"), ("022", "644"), ("027", "640"), ("077", "600")] { + let dir = TempDir::new()?; + child(&dir.0, mask, expected, false)?; + } + Ok(()) +} + +#[test] +fn permission_child() -> io::Result<()> { + let Some(dir) = std::env::var_os("CRAFT_PERMISSION_TEST_DIR") else { return Ok(()) }; + let dir = Path::new(&dir); + let expected = u32::from_str_radix(&std::env::var("CRAFT_PERMISSION_TEST_EXPECTED").map_err(io::Error::other)?, 8).map_err(io::Error::other)?; + let ordinary = dir.join("ordinary"); + fs::write(&ordinary, b"ordinary")?; + assert_eq!(mode(&ordinary)?, expected); + let options = WriteOptions { new_file_permissions: NewFilePermissions::PlatformDefault }; + let payload = b"document bytes"; + for (tag, streaming, exclusive) in + [("replace", false, false), ("replace-stream", true, false), ("exclusive", false, true), ("exclusive-stream", true, true)] + { + let path = dir.join(tag); + let fill = |f: &mut File| { + assert_eq!(f.metadata()?.permissions().mode() & 0o777, 0o600, "private before writing"); + f.write_all(payload)?; + assert_eq!(f.metadata()?.permissions().mode() & 0o777, 0o600, "private during writing"); + // Normal creation policy is applied after the callback, just as an existing mode is. + f.set_permissions(Permissions::from_mode(0o600)) + }; + match (streaming, exclusive) { + (false, false) => options.atomic_write(&path, payload)?, + (false, true) => options.write_new(&path, payload)?, + (true, false) => options.atomic_write_with(&path, fill)?, + (true, true) => options.write_new_with(&path, fill)?, + } + assert_eq!(mode(&path)?, expected, "{tag}"); + assert_eq!(fs::read(&path)?, payload); + if std::env::var("CRAFT_PERMISSION_TEST_ACL").as_deref() == Ok("1") { + let acl = |p: &Path| -> io::Result> { + let output = Command::new("getfacl").args(["-c", "-n"]).arg(p).output()?; + assert!(output.status.success()); + Ok(output.stdout) + }; + assert_eq!(acl(&path)?, acl(&ordinary)?, "same inherited ACL for {tag}"); + } + if exclusive { + assert_eq!(options.write_new(&path, b"must not replace").map_err(|e| e.kind()), Err(io::ErrorKind::AlreadyExists)); + assert_eq!(fs::read(&path)?, payload); + } else { + fs::set_permissions(&path, Permissions::from_mode(0o660))?; + options.atomic_write(&path, b"replacement")?; + assert_eq!(mode(&path)?, 0o660, "preserve existing mode regardless of current umask"); + } + } + for (tag, write) in [ + ("private", atomic_write as fn(&Path, &[u8]) -> io::Result<()>), + ("private-exclusive", write_new), + ("private-options", |p: &Path, b: &[u8]| WriteOptions::default().atomic_write(p, b)), + ("private-options-exclusive", |p: &Path, b: &[u8]| WriteOptions::default().write_new(p, b)), + ] { + let p = dir.join(tag); + write(&p, b"private")?; + assert_eq!(mode(&p)?, 0o600); + } + for exclusive in [false, true] { + let path = dir.join(format!("callback-error-{exclusive}")); + let fill = |f: &mut File| { + f.write_all(b"partial")?; + Err(io::Error::from(io::ErrorKind::StorageFull)) + }; + let result = if exclusive { options.write_new_with(&path, fill) } else { options.atomic_write_with(&path, fill) }; + assert_eq!(result.map_err(|e| e.kind()), Err(io::ErrorKind::StorageFull)); + assert!(!path.exists()); + } + assert!(fs::read_dir(dir)?.all(|e| e.is_ok_and(|e| !e.file_name().to_string_lossy().ends_with(".craft_tmp")))); + Ok(()) +} + +#[cfg(target_os = "linux")] +#[test] +#[ignore = "requires setfacl/getfacl and a filesystem with POSIX ACL support"] +fn directory_default_acl_takes_precedence_over_restrictive_umask() -> io::Result<()> { + let dir = TempDir::new()?; + let output = Command::new("setfacl").args(["-m", "d:u::rwx,d:u:12345:r--,d:g::r-x,d:m::r-x,d:o::---"]).arg(&dir.0).output()?; + assert!(output.status.success(), "{}", String::from_utf8_lossy(&output.stderr)); + // With this default ACL, ordinary creation yields 0640 even under umask 077. + child(&dir.0, "077", "640", true) +} From b55b21cdeb0a477b7321066f0419fc4efff9830b Mon Sep 17 00:00:00 2001 From: skylartaylor Date: Tue, 13 Oct 2026 04:33:01 +0200 Subject: [PATCH 2/2] Apply permission policy review before first release --- .github/workflows/atomic.yml | 8 +- crates/atomic/README.md | 84 ++++--- crates/atomic/src/lib.rs | 343 ++++++++++++++++++---------- crates/atomic/tests/atomic_tests.rs | 13 +- crates/atomic/tests/permissions.rs | 34 +-- crates/settings/src/recovery.rs | 8 +- crates/settings/src/store.rs | 11 +- 7 files changed, 318 insertions(+), 183 deletions(-) diff --git a/.github/workflows/atomic.yml b/.github/workflows/atomic.yml index b41790b7..25423780 100644 --- a/.github/workflows/atomic.yml +++ b/.github/workflows/atomic.yml @@ -4,9 +4,9 @@ on: workflow_dispatch: push: branches: [main] - paths: ['crates/atomic/**', 'Cargo.toml', 'Cargo.lock', '.github/workflows/atomic.yml'] + paths: ['crates/atomic/**', 'crates/settings/**', 'Cargo.toml', 'Cargo.lock', '.github/workflows/atomic.yml'] pull_request: - paths: ['crates/atomic/**', 'Cargo.toml', 'Cargo.lock', '.github/workflows/atomic.yml'] + paths: ['crates/atomic/**', 'crates/settings/**', 'Cargo.toml', 'Cargo.lock', '.github/workflows/atomic.yml'] permissions: contents: read @@ -28,8 +28,8 @@ jobs: if: runner.os == 'Linux' run: sudo apt-get update && sudo apt-get install -y acl - run: cargo fmt --check - - run: cargo clippy -p craft-atomic --all-targets --locked -- -D warnings - - run: cargo test -p craft-atomic --locked -- --nocapture + - run: cargo clippy -p craft-atomic -p craft-settings --all-targets --locked -- -D warnings + - run: cargo test -p craft-atomic -p craft-settings --locked -- --nocapture - name: Check inherited Linux ACLs if: runner.os == 'Linux' run: cargo test -p craft-atomic --locked -- --ignored --nocapture diff --git a/crates/atomic/README.md b/crates/atomic/README.md index c8973724..bd4c4215 100644 --- a/crates/atomic/README.md +++ b/crates/atomic/README.md @@ -12,7 +12,7 @@ Extracted from the storage engine in **PhotoCraft** (refs #2, #14), it guarantee - **Directory Flush (Unix)**: After rename, syncing the parent directory table (`sync_dir`) is attempted on a best-effort basis. If directory sync fails, the error is ignored because the rename has already committed and the destination has already been replaced. - **Hardware Durability Limits**: The sequence `sync_file < rename < sync_dir` proves ordering and kernel dispatch. Software tests do not prove survival across physical power interruption or unbuffered drive controller write caches. 3. **Transient Lock Backoff (Windows)**: Antivirus scanners and search indexers hold newly written files momentarily. `RenameRetry` automatically retries renames encountering `PermissionDenied`, sharing violations (32), or lock violations (33) with exponential backoff capped at 200ms. -4. **Permission & Symlink Invariants**: On Unix, staging files are created with mode `0600` (restricted by the process umask), so the payload starts private even in a shared folder. Existing destination permissions are restored after the streaming callback. New files remain private by default; a callback may select their final permissions, which are kept both with hard links and the exclusive-copy fallback. Symlink chains are resolved with a hop limit, writing through the symlink to the ultimate target without replacing the link itself. +4. **Permission & Symlink Invariants**: On Unix, new documents and exports use normal creation permissions (`0666`, subject to umask/default ACL), including their staging files. Private data must explicitly select `NewFilePermissions::Private` to create staging files with mode `0600`. Replacements always stage privately and restore the existing destination mode after the streaming callback. New files keep callback-selected modes with both hard links and the exclusive-copy fallback. Symlink chains are resolved with a hop limit, writing through the symlink to the ultimate target without replacing the link itself. 5. **Safe Exclusive Creation (`write_new`)**: Uses hard links where supported, or exclusive create (`O_EXCL`), guaranteeing that an existing file is never replaced under race conditions. 6. **Fault-Injection Testability**: The `AtomicFs` trait enables unit and integration test suites to simulate disk-full, locked files, and permission failures without physical disk I/O. @@ -63,53 +63,75 @@ let path = Path::new("export.png"); write_new(path, b"png bytes...").expect("exclusive write"); ``` -### Normal Permissions for Documents and Exports +### Document Permissions and Private Data -The free functions and `WriteOptions::default()` keep new files private. Settings and recovery -backups can continue using them. For a new user document or export that should follow normal -file-creation permissions, opt in explicitly: +The free functions and `WriteOptions::default()` use `NewFilePermissions::PlatformDefault`. +On Unix, a new document/export stage is created directly with mode `0666`; the OS applies the +process umask, setgid directory group and default ACL. This matches ordinary file creation +(for example, `0644` under umask `022`, or `0664` under `002`). No process-global umask changes, +empty permission probe, or `chmod` is needed for an unchanged new-file mode. The same policy +applies when exclusive creation falls back to copying on volumes without hard links. + +This is a deliberate change from the earlier untagged revision's private default. Settings, +credentials and recovery data must opt into private creation **before writing any bytes**: ```rust use std::path::Path; use craft_atomic::{NewFilePermissions, WriteOptions}; -let documents = WriteOptions { new_file_permissions: NewFilePermissions::PlatformDefault }; -documents.atomic_write(Path::new("project.craft"), b"document bytes")?; -documents.write_new(Path::new("export.pdf"), b"pdf bytes")?; -# Ok::<(), std::io::Error>(()) +fn save_private_data() -> std::io::Result<()> { + let private = WriteOptions::default().with_new_file_permissions(NewFilePermissions::Private); + private.atomic_write(Path::new("settings.toml"), b"theme = 'dark'")?; + private.write_new(Path::new("recovery.bin"), b"recovery bytes") +} ``` -`atomic_write_with` and `write_new_with` on these options also support streaming. On Unix, -the library samples normal creation with mode `0666` using a separate, empty file beside the -destination. The OS applies the process umask or the directory's default ACL; process-global -umask is never changed. The empty probe is removed before a distinct `0600` payload stage is -created. Even an open handle to the probe never receives document bytes. This follows the -[Qt QSaveFile approach](https://github.com/qt/qtbase/blob/72b2a0c2f59014a34c2c433fc1fb1d18cff7ec48/src/corelib/io/qsavefile.cpp). +`WriteOptions` and `NewFilePermissions` are non-exhaustive: start with `default()` and select +the policy with the builder, so future options do not break callers. Downstream matches on the +enum must include a fallback arm. Byte and streaming methods support both policies. +`craft-settings` explicitly chooses `Private` for its config and backup writes. -After writing finishes, the selected final permissions are applied before the file flush and -publication. On replacement, the existing Unix mode always takes precedence. On new files, -`PlatformDefault` takes precedence over callback-selected modes; the private default keeps -the original callback behavior. The exclusive-copy fallback still starts private and applies -final permissions after copying. Other platforms keep their existing creation behavior. +Normal stages can be readable (or writable) by others allowed by the directory, umask and ACL +while the callback is running, and can remain after a crash. Choose `Private` up front for +sensitive payloads; changing permissions inside a callback is too late to guarantee that no +other process opened the stage. Private stages request `0600`; actual enforcement depends on +the filesystem's permission support. Neither policy adds Windows ACL guarantees. -Sampling adds one empty-file create/stat/remove per new file with this policy, independent of -payload size. A sampling, probe-cleanup or permission-setting failure aborts the save; the library -does not guess permissions. As with ordinary file creation, this samples the directory policy -at creation time; later umask or directory-policy changes are not tracked. Matching a sampled -Unix mode does not promise preservation of all ACLs or other metadata across replacements. +Replacements still stage privately and restore the existing Unix mode after writing. A new +file keeps any mode selected by its callback. In the normal exclusive-copy fallback, a changed +mode is applied before copying bytes; explicit private copies stay private during copying and +apply callback-selected modes afterward. A required mode change that fails is reported, not +silently ignored. Existing-file replacements and deliberate callback changes can therefore +still require `chmod`; this is not a claim that every save works on every external filesystem. + +These policies do not promise preservation of existing ACLs, ownership or other metadata +across replacements. The copy fallback never overwrites an existing target, but a reader may +see the newly created target while it is being copied. A failed copy is removed. ### Fault Injection & Testing -```rust -use craft_atomic::{AtomicFs, RenameRetry, atomic_write_custom}; +`AtomicFs` lets callers simulate disk-full, locked files and permission failures. Its +`create_new(path, permissions)` operation receives the selected policy; there is no separate +probe operation or unsupported-probe fallback. Custom implementations migrating from an earlier +untagged revision must accept the policy argument and implement both creation modes. -// Inject simulated failures (e.g. storage full mid-stream) -struct FaultyFs; -impl AtomicFs for FaultyFs { - // ... custom failure simulation ... +The options API supports the same injection for byte and streaming writes: + +```rust +use std::path::Path; +use craft_atomic::{AtomicFs, NewFilePermissions, RenameRetry, WriteOptions}; + +fn save_with_faults(fs: &dyn AtomicFs, path: &Path) -> std::io::Result<()> { + let options = WriteOptions::default().with_new_file_permissions(NewFilePermissions::Private); + let retry = RenameRetry { attempts: 0, first_delay_ms: 0 }; + options.atomic_write_custom(fs, retry, path, b"private payload") } ``` +Use `atomic_write_with_custom` for streaming replacements, or `write_new_custom` and +`write_new_with_custom` for exclusive creation. The free `atomic_write_custom` and +`write_new_custom` functions select the default document policy. + ## Guarantees & Doctrines - **Zero App Code**: Zero dependencies on application crates. diff --git a/crates/atomic/src/lib.rs b/crates/atomic/src/lib.rs index 038b055b..36168b31 100644 --- a/crates/atomic/src/lib.rs +++ b/crates/atomic/src/lib.rs @@ -10,7 +10,7 @@ //! //! ### Guarantees: Atomicity vs. Durability //! - **Atomicity Guarantee (All-or-Nothing)**: -//! [`atomic_write`] guarantees that external readers never observe a partial write. Either the target +//! [`atomic_write`] guarantees that readers of the destination never observe a partial write. Either the target //! file retains its previous bytes, or it is replaced completely with the new contents. If an error occurs //! during staging or rename, the target remains unmodified and the staging file is removed in user space. //! An abnormal process termination (e.g. `SIGKILL`, crash, power loss) bypasses user-space cleanup and @@ -28,6 +28,11 @@ //! (`sync_file < rename < sync_dir`), software flushes cannot guarantee data survival across //! unbuffered drive controller write-caching or physical power cutoffs. //! +//! ### Permissions +//! New documents and exports use normal OS creation permissions, including while staging. +//! Use [`WriteOptions`] with [`NewFilePermissions::Private`] for settings or sensitive payloads. +//! Existing destinations always stage privately and retain their original Unix mode. +//! //! ### Platform Characteristics //! - **Windows**: Antivirus scanners, search indexers, and backup utilities briefly hold files //! open with sharing violations. The rename is retried with exponential backoff (`RenameRetry`). @@ -48,16 +53,10 @@ use std::sync::atomic::{AtomicU64, Ordering}; /// File-system operations performed by atomic writes. /// [`RealFs`] provides the production implementation; test suites implement this to inject faults. pub trait AtomicFs { - /// Create `path`, which must not exist, and open it for writing. - /// On Unix, create it with owner-only permissions so payload written afterward is private. - fn create_new(&self, path: &Path) -> io::Result; - /// Exclusively create an empty permissions probe using normal file-creation permissions. - /// The probe is never used for payload and is removed before staging begins. On Unix, - /// request mode `0666` so the OS applies the process umask or directory's default ACL. - /// Existing implementations can leave this unsupported when using the private default. - fn create_permissions_probe(&self, _path: &Path) -> io::Result { - Err(io::Error::new(io::ErrorKind::Unsupported, "normal file-creation permissions are not supported")) - } + /// Exclusively create a payload file with the requested permission policy. On Unix, + /// request mode `0600` for `Private` or `0666` for `PlatformDefault`; the OS applies its + /// umask/default ACL. Other platforms keep their native creation behavior. + fn create_new(&self, path: &Path, permissions: NewFilePermissions) -> io::Result; /// Flush a written file to durable storage. fn sync(&self, file: &File) -> io::Result<()>; /// Rename `from` over `to`, replacing it atomically. @@ -77,23 +76,24 @@ pub trait AtomicFs { pub struct RealFs; impl AtomicFs for RealFs { - fn create_new(&self, path: &Path) -> io::Result { + fn create_new(&self, path: &Path, permissions: NewFilePermissions) -> io::Result { let mut options = OpenOptions::new(); options.write(true).create_new(true); #[cfg(unix)] { use std::os::unix::fs::OpenOptionsExt; - // Set the mode at creation: chmod afterward leaves a window in which another - // user can open the stage, including a stage left behind by an abrupt exit. - options.mode(0o600); + // Apply the policy at creation: chmod afterward cannot revoke an already-open + // handle to private payload, and ordinary new documents need no chmod at all. + options.mode(match permissions { + NewFilePermissions::Private => 0o600, + NewFilePermissions::PlatformDefault => 0o666, + }); } + #[cfg(not(unix))] + let _ = permissions; options.open(path) } - fn create_permissions_probe(&self, path: &Path) -> io::Result { - OpenOptions::new().write(true).create_new(true).open(path) - } - fn sync(&self, file: &File) -> io::Result<()> { sync_file(file) } @@ -172,23 +172,32 @@ impl RenameRetry { } } -/// Final permissions for a newly created file on Unix. Existing destination modes are always -/// preserved. On other platforms, file creation keeps the existing platform behavior. +/// Creation permissions for a new file on Unix. Existing destination modes are preserved. +/// Other platforms keep their existing creation behavior. Streaming callbacks may change a +/// new file's mode; choose `Private` up front when payload must not start with broader access. +#[non_exhaustive] #[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] pub enum NewFilePermissions { /// Keep the private staging mode (`0600`, subject to umask/default ACL), unless the streaming - /// callback selects another mode. This is the behavior of the original free functions. - #[default] + /// callback selects another mode. Use this for settings, credentials and recovery data. Private, /// Use the permissions normal creation with mode `0666` would receive in the destination /// directory. The OS applies its umask/default ACL; the library never changes process umask. - /// These permissions are applied after the callback, before syncing and publication. + /// The stage has these permissions while being written, just like ordinary file creation. + #[default] PlatformDefault, } -/// Permission policy for document saves and exports. Defaults to private new files, matching -/// [`atomic_write`] and [`write_new`]. Select [`NewFilePermissions::PlatformDefault`] for normal -/// user-file creation permissions while retaining private payload staging. +/// Permission policy for document saves and exports. Defaults to normal new-file creation, +/// matching [`atomic_write`] and [`write_new`]. Select [`NewFilePermissions::Private`] explicitly +/// for settings and other private data. Existing files always stage privately before replacement. +/// +/// ``` +/// use craft_atomic::{NewFilePermissions, WriteOptions}; +/// let private = WriteOptions::default().with_new_file_permissions(NewFilePermissions::Private); +/// # let _ = private; +/// ``` +#[non_exhaustive] #[derive(Debug, Clone, Copy, Default)] pub struct WriteOptions { /// Final permissions for new files; ignored when replacing an existing Unix file. @@ -196,31 +205,63 @@ pub struct WriteOptions { } impl WriteOptions { - /// [`atomic_write`] with this new-file permission policy. - pub fn atomic_write(self, path: &Path, bytes: &[u8]) -> io::Result<()> { - self.atomic_write_with(path, |f| f.write_all(bytes)) + /// Select creation permissions without depending on the complete set of options. + pub const fn with_new_file_permissions(mut self, permissions: NewFilePermissions) -> Self { + self.new_file_permissions = permissions; + self } - /// [`atomic_write_with`] with this policy. Existing modes and `PlatformDefault` are applied - /// after the callback, overriding callback-selected permissions. `Private` keeps the - /// original callback behavior for new files. + /// [`atomic_write`] with this new-file permission policy. + pub fn atomic_write(self, path: &Path, bytes: &[u8]) -> io::Result<()> { + self.atomic_write_custom(&RealFs, RenameRetry::platform(), path, bytes) + } + + /// [`atomic_write_with`] with this policy. Existing modes are restored after the callback; + /// for new files, callback-selected modes are kept. pub fn atomic_write_with(self, path: &Path, write_fn: impl FnOnce(&mut File) -> io::Result<()>) -> io::Result<()> { - replace_with_options(&RealFs, RenameRetry::platform(), path, self.new_file_permissions, write_fn) + self.atomic_write_with_custom(&RealFs, RenameRetry::platform(), path, write_fn) + } + + /// [`atomic_write_custom`] with this policy, an injected filesystem and caller-selected retries. + pub fn atomic_write_custom(self, fs: &dyn AtomicFs, retry: RenameRetry, path: &Path, bytes: &[u8]) -> io::Result<()> { + self.atomic_write_with_custom(fs, retry, path, |f| f.write_all(bytes)) + } + + /// Stream an atomic replacement through an injected filesystem with this policy. + pub fn atomic_write_with_custom( + self, + fs: &dyn AtomicFs, + retry: RenameRetry, + path: &Path, + write_fn: impl FnOnce(&mut File) -> io::Result<()>, + ) -> io::Result<()> { + replace_with_options(fs, retry, path, self.new_file_permissions, write_fn) } /// [`write_new`] with this new-file permission policy; never replaces an existing file. pub fn write_new(self, path: &Path, bytes: &[u8]) -> io::Result<()> { - self.write_new_with(path, |f| f.write_all(bytes)) + self.write_new_custom(&RealFs, path, bytes) } - /// [`write_new_with`] with this policy. `PlatformDefault` is applied after the callback. + /// [`write_new_with`] with this policy. Callback-selected modes are kept. pub fn write_new_with(self, path: &Path, write_fn: impl FnOnce(&mut File) -> io::Result<()>) -> io::Result<()> { - create_with_options(&RealFs, path, self.new_file_permissions, write_fn) + self.write_new_with_custom(&RealFs, path, write_fn) + } + + /// [`write_new_custom`] with this policy and an injected filesystem. + pub fn write_new_custom(self, fs: &dyn AtomicFs, path: &Path, bytes: &[u8]) -> io::Result<()> { + self.write_new_with_custom(fs, path, |f| f.write_all(bytes)) + } + + /// Stream exclusive creation through an injected filesystem with this policy. + pub fn write_new_with_custom(self, fs: &dyn AtomicFs, path: &Path, write_fn: impl FnOnce(&mut File) -> io::Result<()>) -> io::Result<()> { + create_with_options(fs, path, self.new_file_permissions, write_fn) } } /// Atomically writes `bytes` to `path`, replacing any existing content. /// +/// New files use normal OS creation permissions. Use [`WriteOptions`] for private data. /// If write or rename fails, the target file is untouched and any temporary staging file is removed. pub fn atomic_write(path: &Path, bytes: &[u8]) -> io::Result<()> { atomic_write_custom(&RealFs, RenameRetry::platform(), path, bytes) @@ -228,9 +269,9 @@ pub fn atomic_write(path: &Path, bytes: &[u8]) -> io::Result<()> { /// Atomically writes to `path` using a streaming writer callback `F`. /// -/// On Unix, the staging file starts with mode `0600` (restricted by the process umask). -/// Existing destination permissions are restored after the callback. For a new destination, -/// callback-selected permissions are kept, or the file remains private by default. +/// On Unix, a new destination is staged with mode `0666`, subject to umask/default ACL. +/// Replacements stage privately (`0600`) and restore the existing mode after the callback. +/// New files keep callback-selected modes. Use [`WriteOptions`] for private new files. pub fn atomic_write_with(path: &Path, write_fn: F) -> io::Result<()> where F: FnOnce(&mut File) -> io::Result<()>, @@ -246,7 +287,7 @@ pub fn atomic_write_custom(fs: &dyn AtomicFs, retry: RenameRetry, path: &Path, b /// The replace path behind every `atomic_write*` function: stage the new content beside the /// target, then rename it over the target. fn replace(fs: &dyn AtomicFs, retry: RenameRetry, path: &Path, fill: impl FnOnce(&mut File) -> io::Result<()>) -> io::Result<()> { - replace_with_options(fs, retry, path, NewFilePermissions::Private, fill) + replace_with_options(fs, retry, path, NewFilePermissions::default(), fill) } fn replace_with_options( @@ -314,13 +355,13 @@ fn stage( ) -> io::Result { let existed = replaced.is_some(); // Windows files carry only a read-only flag, and a read-only file can't be replaced. - let mut perms = replaced.filter(|_| cfg!(unix)).map(|m| m.permissions()); - if cfg!(unix) && !existed && new_permissions == NewFilePermissions::PlatformDefault { - perms = Some(sample_permissions(fs, dir, name).map_err(|e| write_error(e, path, dir, false))?); - } + let perms = replaced.filter(|_| cfg!(unix)).map(|m| m.permissions()); + // New documents inherit the directory's policy directly: no probe or chmod is needed. + // Existing destinations still stage privately, including when their final mode is shared. + let creation = if existed { NewFilePermissions::Private } else { new_permissions }; for _ in 0..8 { let tmp = dir.join(temp_name(name)); - let mut file = match fs.create_new(&tmp) { + let mut file = match fs.create_new(&tmp, creation) { Ok(f) => f, Err(e) if e.kind() == io::ErrorKind::AlreadyExists => continue, Err(e) => return Err(write_error(e, path, dir, existed)), @@ -338,34 +379,15 @@ fn stage( Err(io::Error::new(io::ErrorKind::AlreadyExists, format!("{}: could not create a temporary file", dir.display()))) } -/// Ask the OS for normal creation permissions without changing process-global umask. An empty -/// probe also observes directory default ACLs, which can override umask. Remove it before -/// creating a separate private payload inode: a third party holding the probe open must never -/// receive document bytes. This is the approach used by Qt's QSaveFile. -fn sample_permissions(fs: &dyn AtomicFs, dir: &Path, name: &str) -> io::Result { - for _ in 0..8 { - let probe = dir.join(temp_name(name)); - let file = match fs.create_permissions_probe(&probe) { - Ok(f) => f, - Err(e) if e.kind() == io::ErrorKind::AlreadyExists => continue, - Err(e) => return Err(e), - }; - let sampled = file.metadata().map(|m| m.permissions()); - drop(file); - let removed = fs.remove(&probe); - return sampled.and_then(|perms| removed.map(|()| perms)); - } - Err(io::Error::new(io::ErrorKind::AlreadyExists, format!("{}: could not create a permissions probe", dir.display()))) -} - -/// Exclusively creates and writes `path` without replacing existing files. +/// Exclusively creates and writes `path` with normal OS permissions, without replacing existing files. +/// Use [`WriteOptions`] for private data. pub fn write_new(path: &Path, bytes: &[u8]) -> io::Result<()> { write_new_with(path, |f| f.write_all(bytes)) } /// Exclusively creates and writes `path` with a streaming writer callback `F`. -/// On Unix, the file starts private (`0600`, restricted by the process umask); the callback -/// may choose its final permissions. +/// On Unix, new files start with normal creation permissions (`0666`, subject to umask/default +/// ACL); the callback may change the mode. Use [`WriteOptions`] for private new files. pub fn write_new_with(path: &Path, write_fn: F) -> io::Result<()> where F: FnOnce(&mut File) -> io::Result<()>, @@ -382,7 +404,7 @@ pub fn write_new_custom(fs: &dyn AtomicFs, path: &Path, bytes: &[u8]) -> io::Res /// then give it the target's name with a hard link, which fails rather than replace a file that /// appeared in the meantime. Volumes without hard links get a copy into a newly created file. fn create(fs: &dyn AtomicFs, path: &Path, fill: impl FnOnce(&mut File) -> io::Result<()>) -> io::Result<()> { - create_with_options(fs, path, NewFilePermissions::Private, fill) + create_with_options(fs, path, NewFilePermissions::default(), fill) } fn create_with_options( @@ -400,7 +422,7 @@ fn create_with_options( let tmp = stage(fs, &dir, &name, path, None, new_permissions, fill)?; let placed = match fs.hard_link(&tmp, &target) { - Err(e) if e.kind() != io::ErrorKind::AlreadyExists => copy_new(fs, &tmp, &target), + Err(e) if e.kind() != io::ErrorKind::AlreadyExists => copy_new(fs, &tmp, &target, new_permissions), linked => linked, }; let _ = fs.remove(&tmp); @@ -417,14 +439,34 @@ fn create_with_options( /// Copy `tmp` into `target`, which must not exist yet, and flush it. A partly written `target` is /// removed. -fn copy_new(fs: &dyn AtomicFs, tmp: &Path, target: &Path) -> io::Result<()> { - let mut to = fs.create_new(target)?; +fn copy_new(fs: &dyn AtomicFs, tmp: &Path, target: &Path, permissions: NewFilePermissions) -> io::Result<()> { + let mut to = fs.create_new(target, permissions)?; let copied = File::open(tmp).and_then(|mut from| { - io::copy(&mut from, &mut to)?; - // Keep callback-selected modes just as hard-link publication does, but only - // after copying the payload into a destination that was created privately. #[cfg(unix)] - fs.set_permissions(&to, from.metadata()?.permissions())?; + let final_permissions = { + use std::os::unix::fs::PermissionsExt; + let source = from.metadata()?.permissions(); + let current = to.metadata()?.permissions(); + (source.mode() != current.mode()).then_some(source) + }; + // Normal creation already inherits the correct mode/ACL. Avoid a redundant chmod on + // volumes that reject it. If the callback narrowed the stage, narrow the destination + // before copying any payload, so it is never exposed with the default broader mode. + #[cfg(unix)] + if permissions == NewFilePermissions::PlatformDefault + && let Some(ref mode) = final_permissions + { + fs.set_permissions(&to, mode.clone())?; + } + io::copy(&mut from, &mut to)?; + // Explicit private writes keep their copy destination private until writing finishes, + // then preserve any mode the callback deliberately selected, as the hard link does. + #[cfg(unix)] + if permissions == NewFilePermissions::Private + && let Some(mode) = final_permissions + { + fs.set_permissions(&to, mode)?; + } fs.sync(&to) }); drop(to); @@ -592,6 +634,8 @@ mod tests { events: std::cell::RefCell>, #[cfg(unix)] created_modes: std::cell::RefCell>, + #[cfg(unix)] + permission_lengths: std::cell::RefCell>, fail_sync: bool, /// Syncs fail once this many have succeeded. syncs_before_failure: Option, @@ -602,33 +646,24 @@ mod tests { no_hard_links: bool, fail_permissions: bool, fail_sync_dir: bool, - fail_probe: bool, - probe_collisions: Cell, - fail_probe_remove: bool, - probe_path: std::cell::RefCell>, - probe_handle: std::cell::RefCell>, + fail_normal_create: bool, + normal_collisions: Cell, /// Another process creates the target just before the hard link. created_meanwhile: Option<&'static [u8]>, } impl AtomicFs for Faulty { - fn create_permissions_probe(&self, path: &Path) -> io::Result { - self.events.borrow_mut().push("probe"); - if self.fail_probe { - return Err(io::Error::new(io::ErrorKind::PermissionDenied, "injected: probe creation failed")); + fn create_new(&self, path: &Path, permissions: NewFilePermissions) -> io::Result { + let normal = permissions == NewFilePermissions::PlatformDefault; + self.events.borrow_mut().push(if normal { "create_new_normal" } else { "create_new" }); + if normal && self.fail_normal_create { + return Err(io::Error::new(io::ErrorKind::PermissionDenied, "injected: normal creation failed")); } - if self.probe_collisions.get() > 0 { - self.probe_collisions.set(self.probe_collisions.get() - 1); + if normal && self.normal_collisions.get() > 0 { + self.normal_collisions.set(self.normal_collisions.get() - 1); return Err(io::Error::from(io::ErrorKind::AlreadyExists)); } - let file = RealFs.create_permissions_probe(path)?; - *self.probe_path.borrow_mut() = Some(path.to_path_buf()); - *self.probe_handle.borrow_mut() = Some(file.try_clone()?); - Ok(file) - } - fn create_new(&self, path: &Path) -> io::Result { - self.events.borrow_mut().push("create_new"); - let file = RealFs.create_new(path)?; + let file = RealFs.create_new(path, permissions)?; #[cfg(unix)] { use std::os::unix::fs::PermissionsExt; @@ -668,9 +703,6 @@ mod tests { } fn remove(&self, path: &Path) -> io::Result<()> { self.events.borrow_mut().push("remove"); - if self.fail_probe_remove && self.probe_path.borrow().as_deref() == Some(path) { - return Err(io::Error::new(io::ErrorKind::PermissionDenied, "injected: probe cleanup failed")); - } RealFs.remove(path) } fn sync_dir(&self, dir: &Path) -> io::Result<()> { @@ -682,6 +714,8 @@ mod tests { } fn set_permissions(&self, file: &File, perms: std::fs::Permissions) -> io::Result<()> { self.events.borrow_mut().push("set_permissions"); + #[cfg(unix)] + self.permission_lengths.borrow_mut().push(file.metadata()?.len()); if self.fail_permissions { return Err(io::Error::new(io::ErrorKind::PermissionDenied, "injected: operation not permitted")); } @@ -771,6 +805,40 @@ mod tests { assert_no_temp(&d.0); } + #[test] + fn options_inject_policy_failures_and_retry_budget() -> io::Result<()> { + let d = TempDir::new("options-injection"); + for policy in [NewFilePermissions::Private, NewFilePermissions::PlatformDefault] { + let options = WriteOptions::default().with_new_file_permissions(policy); + for streaming in [false, true] { + let path = d.0.join(format!("{policy:?}-{streaming}")); + let fs = Faulty { locked_renames: Cell::new(1), ..Default::default() }; + let retry = RenameRetry { attempts: 1, first_delay_ms: 0 }; + if streaming { + options.atomic_write_with_custom(&fs, retry, &path, |f| f.write_all(b"payload"))?; + } else { + options.atomic_write_custom(&fs, retry, &path, b"payload")?; + } + assert_eq!(fs.renames.get(), 2); + assert_eq!(std::fs::read(&path)?, b"payload"); + let event = if policy == NewFilePermissions::Private { "create_new" } else { "create_new_normal" }; + assert_eq!(fs.events.borrow()[0], event); + std::fs::remove_file(&path)?; + let fs = Faulty { fail_sync: true, ..Default::default() }; + let result = if streaming { + options.write_new_with_custom(&fs, &path, |f| f.write_all(b"payload")) + } else { + options.write_new_custom(&fs, &path, b"payload") + }; + assert_eq!(result.err().map(|e| e.kind()), Some(io::ErrorKind::StorageFull)); + assert_eq!(fs.events.borrow()[0], event); + assert!(!path.exists()); + assert_no_temp(&d.0); + } + } + Ok(()) + } + #[test] fn write_new_never_overwrites_existing() { let d = TempDir::new("new"); @@ -836,7 +904,7 @@ mod tests { for mode in [0o600, 0o640] { let p = d.0.join(format!("export-{mode:o}.craft")); let fs = Faulty { no_hard_links: true, ..Default::default() }; - create(&fs, &p, |f| { + create_with_options(&fs, &p, NewFilePermissions::Private, |f| { f.write_all(b"private export")?; f.set_permissions(Permissions::from_mode(mode)) })?; @@ -865,7 +933,7 @@ mod tests { #[cfg(unix)] #[test] - fn normal_permissions_survive_exclusive_copy_and_probe_never_receives_payload() -> io::Result<()> { + fn normal_creation_needs_no_chmod_even_when_hard_links_fail() -> io::Result<()> { use std::os::unix::fs::PermissionsExt; use std::process::Command; if std::env::var_os("CRAFT_ATOMIC_COPY_CHILD").is_none() { @@ -875,7 +943,7 @@ mod tests { let output = Command::new("sh") .args([ "-c", - "umask \"$1\"; exec \"$2\" --exact tests::normal_permissions_survive_exclusive_copy_and_probe_never_receives_payload --nocapture", + "umask \"$1\"; exec \"$2\" --exact tests::normal_creation_needs_no_chmod_even_when_hard_links_fail --nocapture", "copy-permissions", mask, ]) @@ -893,35 +961,39 @@ mod tests { let expected = std::fs::metadata(reference)?.permissions().mode() & 0o777; for no_hard_links in [false, true] { let p = d.0.join(format!("export-{no_hard_links}")); - let fs = Faulty { no_hard_links, probe_collisions: Cell::new(2), ..Default::default() }; + let fs = Faulty { no_hard_links, normal_collisions: Cell::new(2), fail_permissions: true, ..Default::default() }; create_with_options(&fs, &p, NewFilePermissions::PlatformDefault, |f| { - assert_eq!(f.metadata()?.permissions().mode() & 0o077, 0); + assert_eq!(f.metadata()?.permissions().mode() & 0o777, expected); f.write_all(b"document payload") })?; assert_eq!(std::fs::metadata(&p)?.permissions().mode() & 0o777, expected); assert_eq!(std::fs::read(&p)?, b"document payload"); - let probe = fs.probe_handle.borrow(); - assert_eq!(probe.as_ref().ok_or_else(|| io::Error::other("missing probe handle"))?.metadata()?.len(), 0); - assert!(!fs.probe_path.borrow().as_ref().is_some_and(|p| p.exists())); - assert!(fs.created_modes.borrow().iter().all(|m| m & 0o077 == 0), "stage and copy destination must start private"); + assert!(fs.created_modes.borrow().iter().all(|m| *m == expected), "stage and copy match ordinary creation"); + assert!(!fs.events.borrow().contains(&"set_permissions")); } + let p = d.0.join("new-document"); + let fs = Faulty { fail_permissions: true, ..Default::default() }; + atomic_write_custom(&fs, NO_RETRY, &p, b"document")?; + assert_eq!(std::fs::metadata(&p)?.permissions().mode() & 0o777, expected); + assert!(!fs.events.borrow().contains(&"set_permissions")); assert_no_temp(&d.0); Ok(()) } #[cfg(unix)] #[test] - fn normal_policy_preserves_existing_modes_without_probing() -> io::Result<()> { + fn normal_policy_preserves_existing_modes_with_private_staging() -> io::Result<()> { use std::os::unix::fs::PermissionsExt; let d = TempDir::new("normal-existing"); for mode in [0o600, 0o640, 0o664] { let p = d.0.join(format!("existing-{mode:o}")); std::fs::write(&p, ORIGINAL)?; std::fs::set_permissions(&p, std::fs::Permissions::from_mode(mode))?; - let fs = Faulty { fail_probe: true, ..Default::default() }; + let fs = Faulty { fail_normal_create: true, ..Default::default() }; replace_with_options(&fs, NO_RETRY, &p, NewFilePermissions::PlatformDefault, |f| f.write_all(b"replacement"))?; assert_eq!(std::fs::metadata(&p)?.permissions().mode() & 0o777, mode); - assert!(!fs.events.borrow().contains(&"probe")); + assert!(!fs.events.borrow().contains(&"create_new_normal")); + assert_eq!(*fs.created_modes.borrow(), [0o600]); } assert_no_temp(&d.0); Ok(()) @@ -933,10 +1005,8 @@ mod tests { let d = TempDir::new("normal-failures"); for exclusive in [false, true] { for (tag, fs, kind, callback_runs) in [ - ("probe", Faulty { fail_probe: true, ..Default::default() }, io::ErrorKind::PermissionDenied, false), - ("collisions", Faulty { probe_collisions: Cell::new(8), ..Default::default() }, io::ErrorKind::AlreadyExists, false), - ("cleanup", Faulty { fail_probe_remove: true, ..Default::default() }, io::ErrorKind::PermissionDenied, false), - ("permissions", Faulty { fail_permissions: true, ..Default::default() }, io::ErrorKind::PermissionDenied, true), + ("create", Faulty { fail_normal_create: true, ..Default::default() }, io::ErrorKind::PermissionDenied, false), + ("collisions", Faulty { normal_collisions: Cell::new(8), ..Default::default() }, io::ErrorKind::AlreadyExists, false), ("sync", Faulty { fail_sync: true, ..Default::default() }, io::ErrorKind::StorageFull, true), ] { let p = d.0.join(format!("{tag}-{exclusive}")); @@ -953,19 +1023,46 @@ mod tests { assert_eq!(result.map_err(|e| e.kind()), Err(kind), "{tag}"); assert_eq!(called.get(), callback_runs, "{tag}"); assert!(!p.exists(), "{tag}"); - if fs.fail_probe_remove { - // The only leftover is an empty probe whose removal was explicitly denied. - let probe = fs.probe_path.borrow(); - let probe = probe.as_ref().ok_or_else(|| io::Error::other("missing probe path"))?; - assert_eq!(std::fs::metadata(probe)?.len(), 0); - std::fs::remove_file(probe)?; - } assert_no_temp(&d.0); } } Ok(()) } + #[cfg(unix)] + #[test] + fn normal_copy_preserves_callback_modes_and_refuses_permission_failures() -> io::Result<()> { + use std::os::unix::fs::PermissionsExt; + let d = TempDir::new("normal-callback-copy"); + for mode in [0o600, 0o640, 0o660] { + for no_hard_links in [false, true] { + let p = d.0.join(format!("{mode:o}-{no_hard_links}")); + let fs = Faulty { no_hard_links, ..Default::default() }; + create(&fs, &p, |f| { + f.set_permissions(std::fs::Permissions::from_mode(mode))?; + f.write_all(b"callback payload") + })?; + assert_eq!(std::fs::metadata(&p)?.permissions().mode() & 0o777, mode); + assert_eq!(std::fs::read(&p)?, b"callback payload"); + assert!(fs.permission_lengths.borrow().iter().all(|len| *len == 0), "adjust normal copies before exposing any payload"); + } + } + let p = d.0.join("permission-failure"); + let fs = Faulty { no_hard_links: true, fail_permissions: true, ..Default::default() }; + let result = create(&fs, &p, |f| { + // Toggle a bit from ordinary creation so the fallback really needs a mode change, + // independently of the test runner's umask. + let mode = f.metadata()?.permissions().mode() ^ 0o040; + f.set_permissions(std::fs::Permissions::from_mode(mode))?; + f.write_all(b"must not survive") + }); + assert_eq!(result.map_err(|e| e.kind()), Err(io::ErrorKind::PermissionDenied)); + assert_eq!(*fs.permission_lengths.borrow(), [0], "refuse before copying payload"); + assert!(!p.exists()); + assert_no_temp(&d.0); + Ok(()) + } + #[cfg(target_os = "linux")] #[test] #[ignore = "requires setfacl/getfacl and a filesystem with POSIX ACL support"] @@ -983,7 +1080,7 @@ mod tests { std::fs::write(&reference, b"normal creation")?; let expected = acl(&reference)?; for no_hard_links in [false, true] { - let fs = Faulty { no_hard_links, ..Default::default() }; + let fs = Faulty { no_hard_links, fail_permissions: true, ..Default::default() }; let p = d.0.join(format!("export-{no_hard_links}")); create_with_options(&fs, &p, NewFilePermissions::PlatformDefault, |f| f.write_all(b"export"))?; assert_eq!(acl(&p)?, expected, "same inherited ACL as ordinary creation"); diff --git a/crates/atomic/tests/atomic_tests.rs b/crates/atomic/tests/atomic_tests.rs index 617cd0c5..b5001bc7 100644 --- a/crates/atomic/tests/atomic_tests.rs +++ b/crates/atomic/tests/atomic_tests.rs @@ -1,5 +1,5 @@ #[cfg(unix)] -use craft_atomic::{atomic_write, write_new}; +use craft_atomic::{NewFilePermissions, WriteOptions, atomic_write, write_new}; use craft_atomic::{atomic_write_with, or_plain_fsync, write_new_with}; use std::cell::Cell; use std::io::{self, Write}; @@ -204,11 +204,12 @@ fn private_staging_preserves_existing_modes_after_callback() -> io::Result<()> { #[cfg(unix)] #[test] -fn new_files_start_private_and_keep_callback_permissions() -> io::Result<()> { +fn explicit_private_files_keep_callback_permissions() -> io::Result<()> { use std::fs::Permissions; use std::os::unix::fs::PermissionsExt; let d = TempDir::new("private_new"); + let options = WriteOptions::default().with_new_file_permissions(NewFilePermissions::Private); for exclusive in [false, true] { let p = d.0.join(format!("new-{exclusive}.craft")); let fill = |f: &mut std::fs::File| { @@ -218,9 +219,9 @@ fn new_files_start_private_and_keep_callback_permissions() -> io::Result<()> { f.set_permissions(Permissions::from_mode(0o640)) }; if exclusive { - write_new_with(&p, fill)?; + options.write_new_with(&p, fill)?; } else { - atomic_write_with(&p, fill)?; + options.atomic_write_with(&p, fill)?; } assert_eq!(std::fs::metadata(&p)?.permissions().mode() & 0o777, 0o640); assert_eq!(std::fs::read(&p)?, b"new payload"); @@ -228,9 +229,9 @@ fn new_files_start_private_and_keep_callback_permissions() -> io::Result<()> { for exclusive in [false, true] { let p = d.0.join(format!("default-{exclusive}.craft")); if exclusive { - write_new(&p, b"private by default")?; + options.write_new(&p, b"explicitly private")?; } else { - atomic_write(&p, b"private by default")?; + options.atomic_write(&p, b"explicitly private")?; } assert_eq!(std::fs::metadata(&p)?.permissions().mode() & 0o777, 0o600); } diff --git a/crates/atomic/tests/permissions.rs b/crates/atomic/tests/permissions.rs index 3ccd35ac..1d0c8946 100644 --- a/crates/atomic/tests/permissions.rs +++ b/crates/atomic/tests/permissions.rs @@ -42,7 +42,7 @@ fn child(dir: &Path, mask: &str, expected: &str, check_acl: bool) -> io::Result< } #[test] -fn normal_creation_respects_umask_and_preserves_private_staging() -> io::Result<()> { +fn normal_creation_matches_os_permissions() -> io::Result<()> { for (mask, expected) in [("000", "666"), ("002", "664"), ("022", "644"), ("027", "640"), ("077", "600")] { let dir = TempDir::new()?; child(&dir.0, mask, expected, false)?; @@ -58,18 +58,17 @@ fn permission_child() -> io::Result<()> { let ordinary = dir.join("ordinary"); fs::write(&ordinary, b"ordinary")?; assert_eq!(mode(&ordinary)?, expected); - let options = WriteOptions { new_file_permissions: NewFilePermissions::PlatformDefault }; + let options = WriteOptions::default(); let payload = b"document bytes"; for (tag, streaming, exclusive) in [("replace", false, false), ("replace-stream", true, false), ("exclusive", false, true), ("exclusive-stream", true, true)] { let path = dir.join(tag); let fill = |f: &mut File| { - assert_eq!(f.metadata()?.permissions().mode() & 0o777, 0o600, "private before writing"); + assert_eq!(f.metadata()?.permissions().mode() & 0o777, expected, "normal creation before writing"); f.write_all(payload)?; - assert_eq!(f.metadata()?.permissions().mode() & 0o777, 0o600, "private during writing"); - // Normal creation policy is applied after the callback, just as an existing mode is. - f.set_permissions(Permissions::from_mode(0o600)) + assert_eq!(f.metadata()?.permissions().mode() & 0o777, expected, "normal creation during writing"); + Ok(()) }; match (streaming, exclusive) { (false, false) => options.atomic_write(&path, payload)?, @@ -96,14 +95,23 @@ fn permission_child() -> io::Result<()> { assert_eq!(mode(&path)?, 0o660, "preserve existing mode regardless of current umask"); } } - for (tag, write) in [ - ("private", atomic_write as fn(&Path, &[u8]) -> io::Result<()>), - ("private-exclusive", write_new), - ("private-options", |p: &Path, b: &[u8]| WriteOptions::default().atomic_write(p, b)), - ("private-options-exclusive", |p: &Path, b: &[u8]| WriteOptions::default().write_new(p, b)), - ] { + for (tag, write) in [("default", atomic_write as fn(&Path, &[u8]) -> io::Result<()>), ("default-exclusive", write_new)] { let p = dir.join(tag); - write(&p, b"private")?; + write(&p, b"document")?; + assert_eq!(mode(&p)?, expected); + } + let private = WriteOptions::default().with_new_file_permissions(NewFilePermissions::Private); + for exclusive in [false, true] { + let p = dir.join(format!("private-{exclusive}")); + let fill = |f: &mut File| { + assert_eq!(f.metadata()?.permissions().mode() & 0o077, 0, "private from creation"); + f.write_all(b"private") + }; + if exclusive { + private.write_new_with(&p, fill)?; + } else { + private.atomic_write_with(&p, fill)?; + } assert_eq!(mode(&p)?, 0o600); } for exclusive in [false, true] { diff --git a/crates/settings/src/recovery.rs b/crates/settings/src/recovery.rs index 19daf1c2..074a1b4a 100644 --- a/crates/settings/src/recovery.rs +++ b/crates/settings/src/recovery.rs @@ -50,12 +50,16 @@ pub fn write_backup(path: &Path, text: &str) -> io::Result<()> { if backup.exists() { std::fs::set_permissions(&backup, std::fs::Permissions::from_mode(mode))?; } - craft_atomic::atomic_write(&backup, text.as_bytes())?; + craft_atomic::WriteOptions::default() + .with_new_file_permissions(craft_atomic::NewFilePermissions::Private) + .atomic_write(&backup, text.as_bytes())?; std::fs::set_permissions(&backup, std::fs::Permissions::from_mode(mode)) } #[cfg(not(unix))] { - craft_atomic::atomic_write(&backup, text.as_bytes()) + craft_atomic::WriteOptions::default() + .with_new_file_permissions(craft_atomic::NewFilePermissions::Private) + .atomic_write(&backup, text.as_bytes()) } } diff --git a/crates/settings/src/store.rs b/crates/settings/src/store.rs index 5ddfbe33..50369459 100644 --- a/crates/settings/src/store.rs +++ b/crates/settings/src/store.rs @@ -355,10 +355,13 @@ impl SettingsStore { merge_into(&mut self.doc, &self.value).map_err(|e| SettingsError::Serialize(e.to_string()))?; set_schema_version(&mut self.doc, T::SCHEMA_VERSION); let text = document::render(&self.doc); - craft_atomic::atomic_write_with(&path, |file| { - restrict_permissions(file)?; - file.write_all(text.as_bytes()) - })?; + craft_atomic::WriteOptions::default().with_new_file_permissions(craft_atomic::NewFilePermissions::Private).atomic_write_with( + &path, + |file| { + restrict_permissions(file)?; + file.write_all(text.as_bytes()) + }, + )?; self.seen_text = Some(text); self.dirty = false; self.written = true;