From 86a06f7b59d628120deb06bdb37ca41dcab0a543 Mon Sep 17 00:00:00 2001 From: Lahfir <70215676+lahfir@users.noreply.github.com> Date: Mon, 27 Jul 2026 02:21:32 -0600 Subject: [PATCH] feat: route private-file primitives through a core-defined platform seam --- crates/core/src/lib.rs | 2 + crates/core/src/private_file.rs | 34 +++-- crates/core/src/private_file_ops.rs | 89 ++++++++++++ crates/core/src/private_file_ops_tests.rs | 165 ++++++++++++++++++++++ crates/core/src/private_file_parent.rs | 4 + crates/core/src/private_file_tests.rs | 74 +++++++++- 6 files changed, 358 insertions(+), 10 deletions(-) create mode 100644 crates/core/src/private_file_ops.rs create mode 100644 crates/core/src/private_file_ops_tests.rs diff --git a/crates/core/src/lib.rs b/crates/core/src/lib.rs index d572aec..5817fc4 100644 --- a/crates/core/src/lib.rs +++ b/crates/core/src/lib.rs @@ -68,6 +68,7 @@ pub mod permission_report; pub mod permission_state; mod point; mod private_file; +mod private_file_ops; mod private_file_parent; mod process_id; mod process_identity; @@ -224,6 +225,7 @@ pub use output::{ErrorPayload, Response}; pub use permission_report::PermissionReport; pub use permission_state::PermissionState; pub use point::Point; +pub use private_file_ops::{PrivateFileOps, install_private_file_ops}; pub use process_id::ProcessId; pub use process_identity::ProcessIdentity; pub use recovery_hint::RecoveryHint; diff --git a/crates/core/src/private_file.rs b/crates/core/src/private_file.rs index 248a182..08f8d8f 100644 --- a/crates/core/src/private_file.rs +++ b/crates/core/src/private_file.rs @@ -8,10 +8,14 @@ use std::sync::atomic::{AtomicU64, Ordering}; static TEMP_COUNTER: AtomicU64 = AtomicU64::new(0); pub(crate) fn open_private_lock(path: &Path, create: bool) -> std::io::Result { + crate::private_file_ops::with_active_ops(|ops| ops.open_private_lock(path, create)) +} + +pub(crate) fn open_private_lock_portable(path: &Path, create: bool) -> std::io::Result { let parent = path .parent() .ok_or_else(|| invalid_input("private file path has no parent"))?; - crate::private_file_parent::ensure_private(parent)?; + crate::private_file_parent::ensure_private_portable(parent)?; let mut options = OpenOptions::new(); options.read(true).write(true).create(create); configure_unix(&mut options, 0o600); @@ -20,10 +24,14 @@ pub(crate) fn open_private_lock(path: &Path, create: bool) -> std::io::Result std::io::Result { + crate::private_file_ops::with_active_ops(|ops| ops.open_private_append(path)) +} + /// Read access is requested alongside append because callers lock the returned /// handle, and Windows `LockFileEx` fails with `ERROR_ACCESS_DENIED` on a handle /// opened for append only. -pub(crate) fn open_private_append(path: &Path) -> std::io::Result { +pub(crate) fn open_private_append_portable(path: &Path) -> std::io::Result { let mut options = OpenOptions::new(); options.read(true).create(true).append(true); configure_unix(&mut options, 0o600); @@ -33,6 +41,13 @@ pub(crate) fn open_private_append(path: &Path) -> std::io::Result { } pub(crate) fn read_private_bounded(path: &Path, max_bytes: u64) -> std::io::Result> { + crate::private_file_ops::with_active_ops(|ops| ops.read_private_bounded(path, max_bytes)) +} + +pub(crate) fn read_private_bounded_portable( + path: &Path, + max_bytes: u64, +) -> std::io::Result> { let file = open_private_read(path)?; read_bounded(file, max_bytes) } @@ -50,10 +65,14 @@ pub(crate) fn read_regular_bounded(path: &Path, max_bytes: u64) -> std::io::Resu } pub(crate) fn write_atomic(path: &Path, bytes: &[u8]) -> std::io::Result<()> { + crate::private_file_ops::with_active_ops(|ops| ops.write_atomic(path, bytes)) +} + +pub(crate) fn write_atomic_portable(path: &Path, bytes: &[u8]) -> std::io::Result<()> { write_atomic_with( path, bytes, - crate::private_file_parent::ensure_private, + crate::private_file_parent::ensure_private_portable, sync_directory, validate_private_destination, ) @@ -82,16 +101,15 @@ fn write_atomic_with( ensure_parent(parent)?; validate_destination(path)?; let (temporary, mut file) = create_temporary(path)?; - let result = (|| { - file.write_all(bytes)?; - file.sync_all()?; + let written = file.write_all(bytes).and_then(|()| file.sync_all()); + drop(file); + let result = written.and_then(|()| { #[cfg(test)] crash_before_rename_if_requested(path); replace_atomic(&temporary, path)?; validate_private_regular(&open_private_read(path)?)?; sync_parent(parent) - })(); - drop(file); + }); if result.is_err() { let _ = std::fs::remove_file(&temporary); } diff --git a/crates/core/src/private_file_ops.rs b/crates/core/src/private_file_ops.rs new file mode 100644 index 0000000..bfe7b00 --- /dev/null +++ b/crates/core/src/private_file_ops.rs @@ -0,0 +1,89 @@ +use std::fs::File; +use std::path::Path; +use std::sync::OnceLock; + +/// Platform seam for the five private-file primitives. +/// +/// Every method defaults to the portable behavior used when no platform +/// implementation is installed, so an implementation overrides only the +/// operations its filesystem semantics require. +pub trait PrivateFileOps: Send + Sync { + /// Writes `bytes` to `path` as one atomic replacement, owning temporary + /// creation, handle lifetime, and replace ordering end to end. + fn write_atomic(&self, path: &Path, bytes: &[u8]) -> std::io::Result<()> { + crate::private_file::write_atomic_portable(path, bytes) + } + + /// Opens `path` for private appends; the returned handle must be lockable. + fn open_private_append(&self, path: &Path) -> std::io::Result { + crate::private_file::open_private_append_portable(path) + } + + /// Opens `path` read-write for locking, creating it when `create` is set. + fn open_private_lock(&self, path: &Path, create: bool) -> std::io::Result { + crate::private_file::open_private_lock_portable(path, create) + } + + /// Reads `path` fully, rejecting files larger than `max_bytes`. + fn read_private_bounded(&self, path: &Path, max_bytes: u64) -> std::io::Result> { + crate::private_file::read_private_bounded_portable(path, max_bytes) + } + + /// Creates the directory chain at `path` and enforces that it is private. + fn ensure_private(&self, path: &Path) -> std::io::Result<()> { + crate::private_file_parent::ensure_private_portable(path) + } +} + +struct PortablePrivateFileOps; + +impl PrivateFileOps for PortablePrivateFileOps {} + +static INSTALLED_OPS: OnceLock> = OnceLock::new(); + +/// Installs the process-wide private-file operations. +/// +/// The first installation wins; a later call leaves the installed operations +/// untouched and returns the rejected implementation. +pub fn install_private_file_ops( + ops: Box, +) -> Result<(), Box> { + INSTALLED_OPS.set(ops) +} + +pub(crate) fn with_active_ops(operate: impl FnOnce(&dyn PrivateFileOps) -> R) -> R { + #[cfg(test)] + if let Some(ops) = TEST_OPS_OVERRIDE.with(|cell| cell.borrow().clone()) { + return operate(ops.as_ref()); + } + match INSTALLED_OPS.get() { + Some(ops) => operate(ops.as_ref()), + None => operate(&PortablePrivateFileOps), + } +} + +#[cfg(test)] +thread_local! { + static TEST_OPS_OVERRIDE: std::cell::RefCell>> = + const { std::cell::RefCell::new(None) }; +} + +#[cfg(test)] +pub(crate) fn with_test_ops_override( + ops: std::rc::Rc, + run: impl FnOnce() -> R, +) -> R { + struct ClearOverrideOnDrop; + impl Drop for ClearOverrideOnDrop { + fn drop(&mut self) { + TEST_OPS_OVERRIDE.with(|cell| cell.borrow_mut().take()); + } + } + TEST_OPS_OVERRIDE.with(|cell| *cell.borrow_mut() = Some(ops)); + let _clear_override = ClearOverrideOnDrop; + run() +} + +#[cfg(test)] +#[path = "private_file_ops_tests.rs"] +mod tests; diff --git a/crates/core/src/private_file_ops_tests.rs b/crates/core/src/private_file_ops_tests.rs new file mode 100644 index 0000000..6ac15a6 --- /dev/null +++ b/crates/core/src/private_file_ops_tests.rs @@ -0,0 +1,165 @@ +use super::*; +use std::io::ErrorKind; +use std::path::PathBuf; +use std::rc::Rc; +use std::sync::Mutex; + +struct RecordingOps { + calls: Mutex>, +} + +impl RecordingOps { + fn new() -> Rc { + Rc::new(Self { + calls: Mutex::new(Vec::new()), + }) + } + + fn record(&self, call: &'static str) { + self.calls.lock().unwrap().push(call); + } + + fn calls(&self) -> Vec<&'static str> { + self.calls.lock().unwrap().clone() + } +} + +impl PrivateFileOps for RecordingOps { + fn write_atomic(&self, _path: &Path, _bytes: &[u8]) -> std::io::Result<()> { + self.record("write_atomic"); + Ok(()) + } + + fn open_private_append(&self, _path: &Path) -> std::io::Result { + self.record("open_private_append"); + Err(std::io::Error::new(ErrorKind::Unsupported, "fake append")) + } + + fn open_private_lock(&self, _path: &Path, _create: bool) -> std::io::Result { + self.record("open_private_lock"); + Err(std::io::Error::new(ErrorKind::Unsupported, "fake lock")) + } + + fn read_private_bounded(&self, _path: &Path, _max_bytes: u64) -> std::io::Result> { + self.record("read_private_bounded"); + Ok(b"routed".to_vec()) + } + + fn ensure_private(&self, _path: &Path) -> std::io::Result<()> { + self.record("ensure_private"); + Ok(()) + } +} + +fn untouched_path(name: &str) -> PathBuf { + std::env::temp_dir().join(format!( + "agent-desktop-ops-{name}-{}-{}", + std::process::id(), + crate::refs::new_snapshot_id() + )) +} + +fn scratch_directory(name: &str) -> PathBuf { + let directory = untouched_path(name); + crate::private_file_parent::ensure_private(&directory).unwrap(); + directory +} + +#[test] +fn every_primitive_routes_through_the_overriding_ops() { + let missing = untouched_path("route-five"); + let recorder = RecordingOps::new(); + + with_test_ops_override(recorder.clone(), || { + crate::private_file::write_atomic(&missing, b"bytes").unwrap(); + let append_error = crate::private_file::open_private_append(&missing).unwrap_err(); + assert_eq!(append_error.kind(), ErrorKind::Unsupported); + let lock_error = crate::private_file::open_private_lock(&missing, true).unwrap_err(); + assert_eq!(lock_error.kind(), ErrorKind::Unsupported); + assert_eq!( + crate::private_file::read_private_bounded(&missing, 8).unwrap(), + b"routed" + ); + crate::private_file_parent::ensure_private(&missing).unwrap(); + }); + + assert_eq!( + recorder.calls(), + vec![ + "write_atomic", + "open_private_append", + "open_private_lock", + "read_private_bounded", + "ensure_private", + ] + ); + assert!(!missing.exists()); +} + +#[test] +fn the_override_is_scoped_to_its_thread() { + let recorder = RecordingOps::new(); + + with_test_ops_override(recorder.clone(), || { + let directory = std::thread::spawn(|| { + let directory = scratch_directory("other-thread"); + crate::private_file::write_atomic(&directory.join("data"), b"portable").unwrap(); + directory + }) + .join() + .unwrap(); + + assert_eq!(std::fs::read(directory.join("data")).unwrap(), b"portable"); + assert_eq!( + crate::private_file::read_private_bounded(&directory.join("data"), 16).unwrap(), + b"routed" + ); + std::fs::remove_dir_all(directory).unwrap(); + }); + + assert_eq!(recorder.calls(), vec!["read_private_bounded"]); +} + +#[test] +fn portable_behavior_resumes_after_the_scoped_override() { + let directory = scratch_directory("restore"); + let path = directory.join("data"); + let recorder = RecordingOps::new(); + + with_test_ops_override(recorder.clone(), || { + crate::private_file::write_atomic(&path, b"faked").unwrap(); + }); + + assert!(!path.exists()); + crate::private_file::write_atomic(&path, b"portable").unwrap(); + assert_eq!( + crate::private_file::read_private_bounded(&path, 16).unwrap(), + b"portable" + ); + std::fs::remove_dir_all(directory).unwrap(); +} + +#[test] +fn a_panicking_override_scope_still_restores_portable_behavior() { + let directory = scratch_directory("panic-restore"); + let path = directory.join("data"); + let recorder = RecordingOps::new(); + + let panicked = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + with_test_ops_override(recorder, || panic!("scope failure")); + })); + + assert!(panicked.is_err()); + crate::private_file::write_atomic(&path, b"portable").unwrap(); + assert_eq!( + crate::private_file::read_private_bounded(&path, 16).unwrap(), + b"portable" + ); + std::fs::remove_dir_all(directory).unwrap(); +} + +#[test] +fn installing_ops_a_second_time_is_rejected() { + assert!(install_private_file_ops(Box::new(PortablePrivateFileOps)).is_ok()); + assert!(install_private_file_ops(Box::new(PortablePrivateFileOps)).is_err()); +} diff --git a/crates/core/src/private_file_parent.rs b/crates/core/src/private_file_parent.rs index 1777bff..2045db1 100644 --- a/crates/core/src/private_file_parent.rs +++ b/crates/core/src/private_file_parent.rs @@ -1,6 +1,10 @@ use std::path::Path; pub(super) fn ensure_private(path: &Path) -> std::io::Result<()> { + crate::private_file_ops::with_active_ops(|ops| ops.ensure_private(path)) +} + +pub(super) fn ensure_private_portable(path: &Path) -> std::io::Result<()> { ensure_directory_path(path)?; let metadata = std::fs::symlink_metadata(path)?; if metadata.file_type().is_symlink() || !metadata.is_dir() { diff --git a/crates/core/src/private_file_tests.rs b/crates/core/src/private_file_tests.rs index 5791267..293b16d 100644 --- a/crates/core/src/private_file_tests.rs +++ b/crates/core/src/private_file_tests.rs @@ -1,9 +1,11 @@ -#![cfg(unix)] - use super::*; +#[cfg(unix)] use std::ffi::CString; +#[cfg(unix)] use std::os::fd::AsRawFd; +#[cfg(unix)] use std::os::unix::fs::OpenOptionsExt; +#[cfg(unix)] use std::os::unix::fs::PermissionsExt; fn directory(label: &str) -> PathBuf { @@ -12,10 +14,12 @@ fn directory(label: &str) -> PathBuf { crate::refs::new_snapshot_id() )); std::fs::create_dir_all(&path).unwrap(); + #[cfg(unix)] std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o700)).unwrap(); path } +#[cfg(unix)] #[test] fn private_open_sets_nonblocking_and_close_on_exec() { let directory = directory("flags"); @@ -29,6 +33,7 @@ fn private_open_sets_nonblocking_and_close_on_exec() { std::fs::remove_dir_all(directory).unwrap(); } +#[cfg(unix)] #[test] fn private_open_rejects_symlink_fifo_device_and_hardlink() { let directory = directory("special"); @@ -70,6 +75,7 @@ fn private_read_enforces_the_bound_before_allocating() { std::fs::remove_dir_all(directory).unwrap(); } +#[cfg(unix)] #[test] fn private_writes_and_locks_reject_group_accessible_parent() { let directory = directory("hostile-parent"); @@ -89,6 +95,7 @@ fn private_writes_and_locks_reject_group_accessible_parent() { std::fs::remove_dir_all(directory).unwrap(); } +#[cfg(unix)] #[test] fn private_write_rejects_an_intermediate_directory_symlink() { let directory = directory("intermediate-symlink"); @@ -107,6 +114,7 @@ fn private_write_rejects_an_intermediate_directory_symlink() { std::fs::remove_dir_all(directory).unwrap(); } +#[cfg(unix)] #[test] fn user_write_overwrites_an_existing_group_readable_file() { let directory = directory("user-overwrite"); @@ -125,6 +133,7 @@ fn user_write_overwrites_an_existing_group_readable_file() { std::fs::remove_dir_all(directory).unwrap(); } +#[cfg(unix)] #[test] fn user_write_refuses_symlink_and_directory_destinations() { let directory = directory("user-refuse"); @@ -156,6 +165,7 @@ fn user_write_refuses_symlink_and_directory_destinations() { std::fs::remove_dir_all(directory).unwrap(); } +#[cfg(unix)] #[test] fn private_write_still_rejects_a_group_readable_destination() { let directory = directory("private-loose-destination"); @@ -170,6 +180,7 @@ fn private_write_still_rejects_a_group_readable_destination() { std::fs::remove_dir_all(directory).unwrap(); } +#[cfg(unix)] #[test] fn user_write_allows_the_system_temporary_directory() { let path = Path::new("/tmp").join(format!( @@ -186,3 +197,62 @@ fn user_write_allows_the_system_temporary_directory() { ); std::fs::remove_file(path).unwrap(); } + +#[test] +fn atomic_write_lands_content_and_replaces_the_previous_file() { + let directory = directory("atomic-replace"); + let path = directory.join("data"); + + write_atomic(&path, b"first").unwrap(); + write_atomic(&path, b"second").unwrap(); + + assert_eq!(read_private_bounded(&path, 64).unwrap(), b"second"); + assert_eq!(std::fs::read_dir(&directory).unwrap().count(), 1); + std::fs::remove_dir_all(directory).unwrap(); +} + +#[test] +fn appended_writes_accumulate_across_reopens() { + let directory = directory("append-accumulate"); + let path = directory.join("trace.jsonl"); + + open_private_append(&path) + .unwrap() + .write_all(b"one\n") + .unwrap(); + open_private_append(&path) + .unwrap() + .write_all(b"two\n") + .unwrap(); + + assert_eq!(read_private_bounded(&path, 64).unwrap(), b"one\ntwo\n"); + std::fs::remove_dir_all(directory).unwrap(); +} + +#[test] +fn private_lock_creates_and_reopens_the_lock_file() { + let directory = directory("lock-create"); + let path = directory.join("lock"); + + drop(open_private_lock(&path, true).unwrap()); + + assert!(path.is_file()); + drop(open_private_lock(&path, false).unwrap()); + std::fs::remove_dir_all(directory).unwrap(); +} + +#[test] +fn ensure_private_creates_the_nested_parent_chain() { + let directory = directory("nested-parents"); + let nested = directory.join("outer").join("inner"); + + crate::private_file_parent::ensure_private(&nested).unwrap(); + + assert!(nested.is_dir()); + write_atomic(&nested.join("data"), b"nested").unwrap(); + assert_eq!( + read_private_bounded(&nested.join("data"), 16).unwrap(), + b"nested" + ); + std::fs::remove_dir_all(directory).unwrap(); +}