diff --git a/.github/workflows/atomic.yml b/.github/workflows/atomic.yml new file mode 100644 index 00000000..25423780 --- /dev/null +++ b/.github/workflows/atomic.yml @@ -0,0 +1,35 @@ +name: Atomic file writes + +on: + workflow_dispatch: + push: + branches: [main] + paths: ['crates/atomic/**', 'crates/settings/**', 'Cargo.toml', 'Cargo.lock', '.github/workflows/atomic.yml'] + pull_request: + paths: ['crates/atomic/**', 'crates/settings/**', '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 -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 423dc52f..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,18 +63,75 @@ let path = Path::new("export.png"); write_new(path, b"png bytes...").expect("exclusive write"); ``` -### Fault Injection & Testing +### Document Permissions and Private Data + +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 craft_atomic::{AtomicFs, RenameRetry, atomic_write_custom}; +use std::path::Path; +use craft_atomic::{NewFilePermissions, WriteOptions}; -// Inject simulated failures (e.g. storage full mid-stream) -struct FaultyFs; -impl AtomicFs for FaultyFs { - // ... custom failure simulation ... +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") } ``` +`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. + +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. + +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 + +`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. + +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 cc4b9d6e..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,9 +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 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. @@ -61,7 +67,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<()>; } @@ -70,16 +76,21 @@ 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) } @@ -161,8 +172,96 @@ impl RenameRetry { } } +/// 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. 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. + /// The stage has these permissions while being written, just like ordinary file creation. + #[default] + PlatformDefault, +} + +/// 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. + pub new_file_permissions: NewFilePermissions, +} + +impl WriteOptions { + /// 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 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<()> { + 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_custom(&RealFs, path, bytes) + } + + /// [`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<()> { + 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) @@ -170,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<()>, @@ -188,6 +287,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::default(), 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 +305,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,14 +350,18 @@ 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()); + // 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)), @@ -266,14 +379,15 @@ fn stage( Err(io::Error::new(io::ErrorKind::AlreadyExists, format!("{}: could not create a temporary file", 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<()>, @@ -290,16 +404,25 @@ 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::default(), 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), + Err(e) if e.kind() != io::ErrorKind::AlreadyExists => copy_new(fs, &tmp, &target, new_permissions), linked => linked, }; let _ = fs.remove(&tmp); @@ -316,14 +439,34 @@ fn create(fs: &dyn AtomicFs, path: &Path, fill: impl FnOnce(&mut File) -> io::Re /// 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); @@ -491,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, @@ -501,14 +646,24 @@ mod tests { no_hard_links: bool, fail_permissions: bool, fail_sync_dir: bool, + 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_new(&self, path: &Path) -> io::Result { - self.events.borrow_mut().push("create_new"); - let file = RealFs.create_new(path)?; + 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 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_new(path, permissions)?; #[cfg(unix)] { use std::os::unix::fs::PermissionsExt; @@ -559,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")); } @@ -648,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"); @@ -713,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)) })?; @@ -740,6 +931,165 @@ mod tests { assert_no_temp(&d.0); } + #[cfg(unix)] + #[test] + 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() { + // 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_creation_needs_no_chmod_even_when_hard_links_fail --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, normal_collisions: Cell::new(2), fail_permissions: true, ..Default::default() }; + create_with_options(&fs, &p, NewFilePermissions::PlatformDefault, |f| { + 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"); + 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_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_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(&"create_new_normal")); + assert_eq!(*fs.created_modes.borrow(), [0o600]); + } + 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 [ + ("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}")); + 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}"); + 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"] + 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, 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"); + 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/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 new file mode 100644 index 00000000..1d0c8946 --- /dev/null +++ b/crates/atomic/tests/permissions.rs @@ -0,0 +1,140 @@ +#![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_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)?; + } + 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::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, expected, "normal creation before writing"); + f.write_all(payload)?; + assert_eq!(f.metadata()?.permissions().mode() & 0o777, expected, "normal creation during writing"); + Ok(()) + }; + 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 [("default", atomic_write as fn(&Path, &[u8]) -> io::Result<()>), ("default-exclusive", write_new)] { + let p = dir.join(tag); + 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] { + 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) +} 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;