From 2c98e881d9efe4e459cca5137aaa7d4b49e9921b Mon Sep 17 00:00:00 2001 From: TimotheusG Date: Mon, 19 Oct 2026 22:54:20 -0400 Subject: [PATCH 1/8] G3: preserve existing exports until encoding succeeds --- .github/workflows/export-filesystem.yml | 35 ++ Cargo.lock | 44 +++ crates/export/Cargo.toml | 14 + crates/export/src/lib.rs | 12 +- crates/export/src/pending.rs | 66 ++++ crates/export/src/pending/native.rs | 459 ++++++++++++++++++++++++ crates/export/src/pending/unix.rs | 118 ++++++ crates/export/src/pending/windows.rs | 111 ++++++ crates/export/src/pipeline.rs | 29 +- crates/export/tests/render_settings.rs | 125 +++++++ docs/export-output-safety.md | 67 ++++ 11 files changed, 1055 insertions(+), 25 deletions(-) create mode 100644 .github/workflows/export-filesystem.yml create mode 100644 crates/export/src/pending.rs create mode 100644 crates/export/src/pending/native.rs create mode 100644 crates/export/src/pending/unix.rs create mode 100644 crates/export/src/pending/windows.rs create mode 100644 docs/export-output-safety.md diff --git a/.github/workflows/export-filesystem.yml b/.github/workflows/export-filesystem.yml new file mode 100644 index 000000000..26dc39486 --- /dev/null +++ b/.github/workflows/export-filesystem.yml @@ -0,0 +1,35 @@ +name: Export filesystem + +on: + push: + paths: ["crates/export/**", "Cargo.lock", ".github/workflows/export-filesystem.yml"] + pull_request: + paths: ["crates/export/**", "Cargo.lock", ".github/workflows/export-filesystem.yml"] + workflow_dispatch: + +permissions: + contents: read + +concurrency: + group: export-filesystem-${{ github.ref }} + cancel-in-progress: true + +jobs: + filesystem: + strategy: + fail-fast: false + matrix: + os: [ubuntu-latest, macos-latest, windows-latest] + runs-on: ${{ matrix.os }} + timeout-minutes: 30 + env: + CARGO_PROFILE_RELEASE_LTO: "false" + CARGO_PROFILE_RELEASE_CODEGEN_UNITS: "16" + steps: + - uses: actions/checkout@v4 + - uses: dtolnay/rust-toolchain@stable + - uses: Swatinem/rust-cache@v2 + - name: Native filesystem contract + run: cargo test --locked --release -p effectcraft-export --lib pending::native::tests + - name: Export integration + run: cargo test --locked --release -p effectcraft-export --test render_settings diff --git a/Cargo.lock b/Cargo.lock index 412fb6672..fc6a5a882 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1296,6 +1296,7 @@ dependencies = [ "effectcraft-render", "effectcraft-time", "effectcraft-vp9enc", + "exacl", "filmcraft-aac", "filmcraft-h264enc", "filmcraft-isobmff", @@ -1306,8 +1307,13 @@ dependencies = [ "image", "log", "rayon", + "tempfile", "thiserror 2.0.21", "web-time", + "winapi-util", + "windows-permissions", + "winsafe", + "xattr", ] [[package]] @@ -1993,6 +1999,18 @@ dependencies = [ "pin-project-lite", ] +[[package]] +name = "exacl" +version = "0.13.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8de9e551ced9d700a31dacb8b168129640240fcd7a279a3fa4d59608b84ebcae" +dependencies = [ + "bitflags 2.13.2", + "log", + "scopeguard", + "uuid", +] + [[package]] name = "exr" version = "1.74.2" @@ -6205,6 +6223,16 @@ dependencies = [ "windows-link", ] +[[package]] +name = "windows-permissions" +version = "0.2.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9e2ccdc3c6bf4d4a094e031b63fadd08d8e42abd259940eb8aa5fdc09d4bf9be" +dependencies = [ + "bitflags 1.3.2", + "winapi", +] + [[package]] name = "windows-result" version = "0.1.2" @@ -6542,6 +6570,12 @@ dependencies = [ "version_check", ] +[[package]] +name = "winsafe" +version = "0.0.29" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9ef0ffc427f045c0cc9ebffd6f4f91153dcd2a1547b5c3c29ee3f541e16e95c6" + [[package]] name = "wit-bindgen" version = "0.57.1" @@ -6592,6 +6626,16 @@ version = "0.13.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ea6fc2961e4ef194dcbfe56bb845534d0dc8098940c7e5c012a258bfec6701bd" +[[package]] +name = "xattr" +version = "1.6.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "32e45ad4206f6d2479085147f02bc2ef834ac85886624a23575ae137c8aa8156" +dependencies = [ + "libc", + "rustix 1.1.5", +] + [[package]] name = "xcursor" version = "0.3.11" diff --git a/crates/export/Cargo.toml b/crates/export/Cargo.toml index 2ef6f5ad8..096f39a7d 100644 --- a/crates/export/Cargo.toml +++ b/crates/export/Cargo.toml @@ -36,3 +36,17 @@ effectcraft-color = { workspace = true } filmcraft-opus = { workspace = true } filmcraft-vp9 = { workspace = true } effectcraft-keyframe = { workspace = true } + +[target.'cfg(not(target_arch = "wasm32"))'.dependencies] +tempfile = "3.27" + +[target.'cfg(windows)'.dependencies] +winsafe = { version = "0.0.29", features = ["advapi", "kernel"] } +windows-permissions = "0.2.4" +winapi-util = "0.1.11" + +[target.'cfg(unix)'.dependencies] +xattr = "1.6" + +[target.'cfg(any(target_os = "macos", target_os = "freebsd"))'.dependencies] +exacl = "0.13" diff --git a/crates/export/src/lib.rs b/crates/export/src/lib.rs index eda979958..da1358557 100644 --- a/crates/export/src/lib.rs +++ b/crates/export/src/lib.rs @@ -35,6 +35,7 @@ mod encode; mod hevc_av1; mod out; +mod pending; mod pipeline; mod uncompressed; mod webm; @@ -175,8 +176,17 @@ pub async fn export_async(job: &Job<'_>, progress: &mut dyn FnMut(&Progress) -> let t0 = Instant::now(); let cx = Cx::new(job); let mut r = export_with(&cx, progress, t0).await; + if r.is_ok() + && let Err(error) = cx.pending.publish() + { + r = Err(error); + } + if let Err(original) = &r + && let Err(cleanup) = cx.pending.discard() + { + return Err(io(format!("{original}; cleanup also failed: {cleanup}"))); + } if matches!(r, Err(ExportError::Cancelled)) { - cx.remove_cancelled_outputs()?; return r; } let log = cx.write_log(&r, t0.elapsed().as_secs_f64()); diff --git a/crates/export/src/pending.rs b/crates/export/src/pending.rs new file mode 100644 index 000000000..29d4720a4 --- /dev/null +++ b/crates/export/src/pending.rs @@ -0,0 +1,66 @@ +//! Job-owned staging: failed exports never clean up a final pathname. + +use crate::{Result, out}; + +#[cfg(not(target_arch = "wasm32"))] +mod native; + +#[derive(Default)] +pub(crate) struct Pending { + #[cfg(not(target_arch = "wasm32"))] + files: std::sync::Mutex>, +} + +impl Pending { + pub fn create<'a>(&self, sink: Option<&'a out::Sink>, path: &str) -> Result> { + if sink.is_some() { + return out::create(sink, path); + } + #[cfg(not(target_arch = "wasm32"))] + { + let mut files = self.files.lock().unwrap_or_else(std::sync::PoisonError::into_inner); + let (staged, file) = native::Staged::create(path).map_err(crate::io)?; + if files.iter().any(|other| other.path == staged.path) { + return Err(crate::io(format!("duplicate export destination: {path}"))); + } + files.push(staged); + Ok(out::Out::File(std::io::BufWriter::new(file))) + } + #[cfg(target_arch = "wasm32")] + Err(crate::ExportError::Unsupported("web exports require an output sink".into())) + } + + /// Each rename publishes one complete file. A sequence is not an atomic transaction: + /// previously published frames remain if a later publication fails. + pub fn publish(&self) -> Result<()> { + #[cfg(not(target_arch = "wasm32"))] + { + let mut files = self.files.lock().unwrap_or_else(std::sync::PoisonError::into_inner); + // Validate all destinations before publishing any of them. + for staged in files.iter() { + staged.validate().map_err(crate::io)?; + } + while let Some(staged) = files.pop() { + staged.publish().map_err(crate::io)?; + } + } + Ok(()) + } + + pub fn discard(&self) -> Result<()> { + #[cfg(not(target_arch = "wasm32"))] + { + let files = std::mem::take(&mut *self.files.lock().unwrap_or_else(std::sync::PoisonError::into_inner)); + let mut failure = None; + for staged in files { + if let Err(error) = staged.discard() { + failure.get_or_insert(error); + } + } + if let Some(error) = failure { + return Err(crate::io(error)); + } + } + Ok(()) + } +} diff --git a/crates/export/src/pending/native.rs b/crates/export/src/pending/native.rs new file mode 100644 index 000000000..b92bc95f8 --- /dev/null +++ b/crates/export/src/pending/native.rs @@ -0,0 +1,459 @@ +use std::{ + fs::{self, File, OpenOptions}, + io, + path::{Path, PathBuf}, + time::SystemTime, +}; + +#[cfg(unix)] +#[path = "unix.rs"] +mod platform; + +// Serialize validation and replacement across jobs in this process. External writers still +// require cooperation: portable filesystems do not provide compare-and-swap by file version. +static PUBLICATION: std::sync::Mutex<()> = std::sync::Mutex::new(()); +#[cfg(windows)] +#[path = "windows.rs"] +mod platform; + +pub(super) struct Staged { + temporary: tempfile::TempPath, + directory: Option, + pub path: PathBuf, + requested: PathBuf, + original: Option, + policy: platform::Policy, + parent_policy: platform::Policy, +} + +#[derive(PartialEq, Eq)] +struct Version { + identity: (u64, u64), + change_stamp: (i64, i64), + len: u64, + modified: SystemTime, + policy: platform::Policy, +} + +impl Version { + fn read(file: &File, path: &Path) -> io::Result { + let metadata = file.metadata()?; + if !metadata.is_file() || platform::links(file)? != 1 { + return Err(io::Error::other(format!("export destination must be a regular file with one hard link: {}", path.display()))); + } + Ok(Self { + identity: platform::identity(file)?, + change_stamp: platform::change_stamp(file)?, + len: metadata.len(), + modified: metadata.modified()?, + policy: platform::Policy::read(path)?, + }) + } +} + +impl Staged { + pub fn create(path: &str) -> io::Result<(Self, File)> { + let requested = std::path::absolute(path)?; + let path = resolve(&requested, true)?; + let parent = path.parent().ok_or_else(|| io::Error::other("output has no parent directory"))?; + let parent_policy = platform::Policy::read(parent)?; + match fs::metadata(&path) { + Ok(metadata) if !metadata.is_file() => return Err(io::Error::other("export destination is not a regular file")), + Err(error) if error.kind() != io::ErrorKind::NotFound => return Err(error), + _ => {} + } + let original = match OpenOptions::new().write(true).open(&path) { + Ok(file) => { + let version = Version::read(&file, &path)?; + Some(version) + } + Err(error) if error.kind() == io::ErrorKind::NotFound => None, + Err(error) => return Err(error), + }; + // The ordinary creation probe is always empty. No exported bytes ever enter an inode + // that another account might have opened before access was restricted. + let probe = platform::ordinary_file(parent)?; + let policy = match &original { + Some(version) => version.policy.clone(), + None => platform::Policy::read(probe.path())?, + }; + policy.apply(probe.path())?; + if platform::Policy::read(probe.path())? != policy { + return Err(io::Error::other("cannot preserve export destination access policy on this filesystem")); + } + probe.close()?; + let (temporary, directory) = platform::private_file(parent)?; + let (file, temporary) = temporary.into_parts(); + let staged = Self { temporary, directory, path, requested, original, policy, parent_policy }; + staged.validate()?; + Ok((staged, file)) + } + + pub fn validate(&self) -> io::Result<()> { + if resolve(&self.requested, false)? != self.path { + return Err(io::Error::other("export destination symlink changed while rendering")); + } + let parent = self.path.parent().ok_or_else(|| io::Error::other("output has no parent directory"))?; + if platform::Policy::read(parent)? != self.parent_policy { + return Err(io::Error::other("export directory access policy changed while rendering")); + } + match &self.original { + Some(expected) => { + let current = OpenOptions::new().write(true).open(&self.path)?; + if Version::read(¤t, &self.path)? != *expected { + return Err(io::Error::other(format!("export destination changed while rendering: {}", self.path.display()))); + } + } + None => match fs::symlink_metadata(&self.path) { + Err(error) if error.kind() == io::ErrorKind::NotFound => {} + Err(error) => return Err(error), + Ok(_) => return Err(io::Error::other(format!("export destination appeared while rendering: {}", self.path.display()))), + }, + } + Ok(()) + } + + pub fn publish(self) -> io::Result<()> { + let result = { + let _publication = PUBLICATION.lock().unwrap_or_else(std::sync::PoisonError::into_inner); + self.publish_inner() + }; + let cleanup = self.discard(); + match (result, cleanup) { + (Err(error), Err(cleanup)) => Err(io::Error::new(error.kind(), format!("{error}; cleanup also failed: {cleanup}"))), + (Err(error), _) | (_, Err(error)) => Err(error), + _ => Ok(()), + } + } + + fn publish_inner(&self) -> io::Result<()> { + self.validate()?; + self.policy.apply(&self.temporary)?; + if platform::Policy::read(&self.temporary)? != self.policy { + return Err(io::Error::other("staged export access policy could not be preserved")); + } + self.validate()?; + if self.original.is_some() { + fs::rename(&self.temporary, &self.path)?; + } else { + // Unlike a check followed by rename, this cannot overwrite a file that appeared + // after validation. Both names refer to complete bytes until staging is removed. + fs::hard_link(&self.temporary, &self.path)?; + } + Ok(()) + } + + pub fn discard(self) -> io::Result<()> { + if let Err(error) = self.temporary.close() + && error.kind() != io::ErrorKind::NotFound + { + return Err(io::Error::new(error.kind(), format!("cannot remove staging for {}: {error}", self.path.display()))); + } + if let Some(directory) = self.directory { + directory.close()?; + } + Ok(()) + } +} + +/// Resolve leaf links explicitly, including dangling relative links. The OS resolves directory +/// links with its own loop checks; our leaf-link walk has a fixed bound. +fn resolve(requested: &Path, create_parents: bool) -> io::Result { + let mut path = requested.to_path_buf(); + for _ in 0..40 { + let parent = path.parent().ok_or_else(|| io::Error::other("output has no parent directory"))?; + if create_parents { + fs::create_dir_all(parent)?; + } + let name = path.file_name().ok_or_else(|| io::Error::other("output has no filename"))?; + path = parent.canonicalize()?.join(name); + match fs::symlink_metadata(&path) { + Ok(metadata) if metadata.file_type().is_symlink() => { + let target = fs::read_link(&path)?; + path = if target.is_absolute() { target } else { path.parent().ok_or_else(|| io::Error::other("link has no parent"))?.join(target) }; + } + Ok(_) => return Ok(path), + Err(error) if error.kind() == io::ErrorKind::NotFound => return Ok(path), + Err(error) => return Err(error), + } + } + Err(io::Error::other("export destination has too many symbolic links")) +} + +#[cfg(test)] +mod tests { + use super::*; + use std::io::Write; + + fn stage(path: &Path, bytes: &[u8]) -> Staged { + let (staged, mut file) = Staged::create(path.to_str().unwrap()).unwrap(); + file.write_all(bytes).unwrap(); + file.flush().unwrap(); + drop(file); + staged + } + + #[test] + fn lifecycle_preserves_existing_bytes_until_publish_and_cleans_owned_files() { + for existing in [false, true] { + for action in ["drop", "discard", "publish"] { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("movie.mov"); + if existing { + fs::write(&path, b"old complete movie").unwrap(); + } + let expected = if existing { + platform::Policy::read(&path).unwrap() + } else { + let ordinary = dir.path().join("ordinary"); + fs::write(&ordinary, b"").unwrap(); + let policy = platform::Policy::read(&ordinary).unwrap(); + fs::remove_file(ordinary).unwrap(); + policy + }; + let staged = stage(&path, b"new complete movie"); + assert_eq!(fs::read(&path).ok(), existing.then(|| b"old complete movie".to_vec())); + match action { + "publish" => staged.publish().unwrap(), + "discard" => staged.discard().unwrap(), + _ => drop(staged), + } + if action == "publish" { + assert_eq!(fs::read(&path).unwrap(), b"new complete movie"); + assert_eq!(platform::Policy::read(&path).unwrap(), expected); + } else { + assert_eq!(fs::read(&path).ok(), existing.then(|| b"old complete movie".to_vec())); + } + assert_eq!(fs::read_dir(dir.path()).unwrap().count(), usize::from(existing || action == "publish")); + } + } + } + + #[test] + fn competing_jobs_and_external_writes_are_not_overwritten_or_deleted() { + for existing in [false, true] { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("movie.mov"); + if existing { + fs::write(&path, b"old").unwrap(); + } + let first = stage(&path, b"first job"); + let second = stage(&path, b"second job"); + second.publish().unwrap(); + assert!(first.publish().is_err()); + assert_eq!(fs::read(&path).unwrap(), b"second job"); + let third = stage(&path, b"third job"); + fs::write(&path, b"external writer with different length").unwrap(); + assert!(third.publish().is_err()); + assert_eq!(fs::read(&path).unwrap(), b"external writer with different length"); + assert_eq!(fs::read_dir(dir.path()).unwrap().count(), 1); + + let race = dir.path().join("race.mov"); + if existing { + fs::write(&race, b"old").unwrap(); + } + let a = stage(&race, b"a"); + let b = stage(&race, b"b"); + let barrier = std::sync::Arc::new(std::sync::Barrier::new(2)); + let barrier_b = barrier.clone(); + let successes = std::thread::scope(|scope| { + let a = scope.spawn(move || { + barrier.wait(); + a.publish().is_ok() + }); + let b = scope.spawn(move || { + barrier_b.wait(); + b.publish().is_ok() + }); + usize::from(a.join().unwrap()) + usize::from(b.join().unwrap()) + }); + assert_eq!(successes, 1); + assert!(matches!(fs::read(race).unwrap().as_slice(), b"a" | b"b")); + assert_eq!(fs::read_dir(dir.path()).unwrap().count(), 2); + } + } + + #[test] + fn publication_failure_leaves_destination_and_no_staging() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("movie.mov"); + let staged = stage(&path, b"complete movie"); + fs::create_dir(&path).unwrap(); + assert!(staged.publish().is_err()); + assert!(path.is_dir()); + assert_eq!(fs::read_dir(dir.path()).unwrap().count(), 1); + } + + #[test] + fn hardlink_aliases_are_rejected_without_touching_either_name() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("movie.mov"); + let alias = dir.path().join("alias.mov"); + fs::write(&path, b"original").unwrap(); + fs::hard_link(&path, &alias).unwrap(); + assert!(Staged::create(path.to_str().unwrap()).is_err()); + assert_eq!(fs::read(path).unwrap(), b"original"); + assert_eq!(fs::read(alias).unwrap(), b"original"); + assert_eq!(fs::read_dir(dir.path()).unwrap().count(), 2); + } + + #[cfg(unix)] + #[test] + fn relative_and_dangling_links_preserve_links_and_retargeting_fails() { + use std::os::unix::fs::symlink; + let dir = tempfile::tempdir().unwrap(); + let target = dir.path().join("movie.mov"); + let link = dir.path().join("link.mov"); + let chain = dir.path().join("chain.mov"); + symlink("movie.mov", &link).unwrap(); + symlink("link.mov", &chain).unwrap(); + stage(&chain, b"first").publish().unwrap(); + assert_eq!(fs::read(&target).unwrap(), b"first"); + stage(&chain, b"second").publish().unwrap(); + assert_eq!(fs::read(&target).unwrap(), b"second"); + assert_eq!(fs::read_link(&link).unwrap(), Path::new("movie.mov")); + let staged = stage(&chain, b"stale"); + fs::remove_file(&link).unwrap(); + symlink("elsewhere.mov", &link).unwrap(); + assert!(staged.publish().is_err()); + assert!(!dir.path().join("elsewhere.mov").exists()); + fs::remove_file(&link).unwrap(); + symlink("chain.mov", &link).unwrap(); + assert!(Staged::create(chain.to_str().unwrap()).is_err()); + assert_eq!(fs::read(&target).unwrap(), b"second"); + } + + #[cfg(unix)] + #[test] + fn unix_modes_group_and_policy_changes_are_preserved_or_rejected() { + use std::os::unix::fs::{MetadataExt, PermissionsExt}; + for mode in [0o600, 0o640, 0o660] { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("movie.mov"); + fs::write(&path, b"old").unwrap(); + fs::set_permissions(&path, fs::Permissions::from_mode(mode)).unwrap(); + let before = fs::metadata(&path).unwrap(); + let staged = stage(&path, b"replacement"); + assert_eq!(fs::metadata(&staged.temporary).unwrap().permissions().mode() & 0o077, 0); + staged.publish().unwrap(); + let after = fs::metadata(&path).unwrap(); + assert_eq!((after.uid(), after.gid(), after.mode()), (before.uid(), before.gid(), before.mode())); + let staged = stage(&path, b"must not publish"); + fs::set_permissions(&path, fs::Permissions::from_mode(0o400)).unwrap(); + assert!(staged.publish().is_err()); + assert_eq!(fs::read(&path).unwrap(), b"replacement"); + } + } + + #[cfg(windows)] + #[test] + fn windows_inherited_and_protected_acls_and_policy_changes() { + use windows_permissions::{ + constants::{SeObjectType as O, SecurityInformation as S}, + wrappers as w, + }; + for protected in [false, true] { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("movie.mov"); + fs::write(&path, b"old").unwrap(); + let owner = w::GetNamedSecurityInfo(path.as_os_str(), O::SE_FILE_OBJECT, S::Owner).unwrap(); + let sid = w::ConvertSidToStringSid(w::GetSecurityDescriptorOwner(&owner).unwrap().unwrap()).unwrap(); + let restricted = w::ConvertStringSecurityDescriptorToSecurityDescriptor(&format!("D:P(A;;FA;;;{})", sid.to_string_lossy())).unwrap(); + if protected { + w::SetNamedSecurityInfo( + path.as_os_str(), + O::SE_FILE_OBJECT, + S::Dacl | S::ProtectedDacl, + None, + None, + w::GetSecurityDescriptorDacl(&restricted).unwrap(), + None, + ) + .unwrap(); + } + let expected = platform::Policy::read(&path).unwrap(); + let staged = stage(&path, b"replacement"); + let private = w::GetNamedSecurityInfo(staged.temporary.as_os_str(), O::SE_FILE_OBJECT, S::Dacl).unwrap(); + let private_text = w::ConvertSecurityDescriptorToStringSecurityDescriptor(&private, S::Dacl).unwrap(); + assert_eq!(private_text.to_string_lossy().replace("D:PAI", "D:P"), format!("D:P(A;;FA;;;{})", sid.to_string_lossy())); + staged.publish().unwrap(); + assert_eq!(platform::Policy::read(&path).unwrap(), expected); + let staged = stage(&path, b"must not publish"); + let original_permissions = fs::metadata(&path).unwrap().permissions(); + let mut permissions = original_permissions.clone(); + permissions.set_readonly(true); + fs::set_permissions(&path, permissions).unwrap(); + assert!(staged.publish().is_err()); + assert_eq!(fs::read(&path).unwrap(), b"replacement"); + fs::set_permissions(&path, original_permissions).unwrap(); + } + } + + #[cfg(target_os = "linux")] + #[test] + fn linux_named_acl_and_setgid_parent_do_not_broaden_replacement() { + use std::os::unix::fs::{MetadataExt, PermissionsExt}; + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("movie.mov"); + fs::write(&path, b"old").unwrap(); + // Linux POSIX ACL xattr format: version followed by tag, permissions, numeric ID. + let mut acl = 2_u32.to_le_bytes().to_vec(); + for (tag, perms, id) in [(1_u16, 6_u16, u32::MAX), (2, 4, 12345), (4, 0, u32::MAX), (16, 4, u32::MAX), (32, 0, u32::MAX)] { + acl.extend(tag.to_le_bytes()); + acl.extend(perms.to_le_bytes()); + acl.extend(id.to_le_bytes()); + } + xattr::set(&path, "system.posix_acl_access", &acl).unwrap(); + let expected = platform::Policy::read(&path).unwrap(); + if fs::metadata(&path).unwrap().uid() == 0 { + // The local Linux probe runs as root, allowing a genuine different-group case. + std::os::unix::fs::chown(dir.path(), None, Some(54321)).unwrap(); + } + fs::set_permissions(dir.path(), fs::Permissions::from_mode(0o2770)).unwrap(); + let mut inherited_acl = acl.clone(); + inherited_acl[16..20].copy_from_slice(&23456_u32.to_le_bytes()); + xattr::set(dir.path(), "system.posix_acl_default", &inherited_acl).unwrap(); + stage(&path, b"replacement").publish().unwrap(); + assert_eq!(platform::Policy::read(&path).unwrap(), expected); + assert_eq!(xattr::get(&path, "system.posix_acl_access").unwrap().unwrap(), acl); + let staged = stage(&path, b"stale policy"); + fs::set_permissions(dir.path(), fs::Permissions::from_mode(0o2700)).unwrap(); + assert!(staged.publish().is_err()); + assert_eq!(fs::read(path).unwrap(), b"replacement"); + } + + #[cfg(target_os = "macos")] + #[test] + fn macos_inherited_acl_is_absent_from_private_staging_and_restored_on_publish() { + let dir = tempfile::tempdir().unwrap(); + let acl = + [exacl::AclEntry::allow_group("everyone", exacl::Perm::READ | exacl::Perm::EXECUTE, exacl::Flag::FILE_INHERIT | exacl::Flag::DIRECTORY_INHERIT)]; + exacl::setfacl(&[dir.path()], &acl, None).unwrap(); + let path = dir.path().join("movie.mov"); + fs::write(&path, b"old").unwrap(); + let expected = platform::Policy::read(&path).unwrap(); + let staged = stage(&path, b"replacement"); + assert!(exacl::getfacl(&staged.temporary, None).unwrap().is_empty()); + staged.publish().unwrap(); + assert_eq!(platform::Policy::read(&path).unwrap(), expected); + assert_eq!(fs::read(path).unwrap(), b"replacement"); + } + /// More frames than a typical descriptor limit: snapshots retain metadata, not open files. + #[test] + fn replacement_sequence_keeps_file_handle_use_constant() { + let dir = tempfile::tempdir().unwrap(); + let mut pending = Vec::new(); + for frame in 0..1536 { + let path = dir.path().join(format!("frame_{frame}.png")); + fs::write(&path, b"old").unwrap(); + pending.push(stage(&path, b"new")); + } + for staged in pending { + let path = staged.path.clone(); + assert_eq!(fs::read(&path).unwrap(), b"old"); + staged.publish().unwrap(); + assert_eq!(fs::read(&path).unwrap(), b"new"); + } + assert_eq!(fs::read_dir(dir.path()).unwrap().count(), 1536); + } +} diff --git a/crates/export/src/pending/unix.rs b/crates/export/src/pending/unix.rs new file mode 100644 index 000000000..e4918a4ec --- /dev/null +++ b/crates/export/src/pending/unix.rs @@ -0,0 +1,118 @@ +use std::{ + ffi::OsString, + fs::{self, File}, + io, + os::unix::fs::{MetadataExt, PermissionsExt}, + path::Path, +}; + +#[derive(Clone, Debug, PartialEq, Eq)] +pub(super) struct Policy { + uid: u32, + gid: u32, + mode: u32, + attributes: Vec<(OsString, Vec)>, + #[cfg(any(target_os = "macos", target_os = "freebsd"))] + acl: Vec, +} + +impl Policy { + pub fn read(path: &Path) -> io::Result { + let metadata = fs::metadata(path)?; + let mut attributes = Vec::new(); + for name in xattr::list(path)? { + let text = name.to_string_lossy(); + if text.starts_with("security.") || text.starts_with("system.") { + let value = xattr::get(path, &name)?.ok_or_else(|| io::Error::other("access attribute changed while reading"))?; + if value.len() > 1_048_576 || attributes.len() >= 128 { + return Err(io::Error::other("export access metadata exceeds supported limits")); + } + attributes.push((name, value)); + } + } + attributes.sort_by(|a, b| a.0.cmp(&b.0)); + Ok(Self { + uid: metadata.uid(), + gid: metadata.gid(), + mode: metadata.mode() & 0o7777, + attributes, + #[cfg(any(target_os = "macos", target_os = "freebsd"))] + acl: exacl::getfacl(path, None)?, + }) + } + + pub fn apply(&self, path: &Path) -> io::Result<()> { + // An export is data, not a privileged executable. Do not transfer privilege bits or + // file capabilities onto newly generated bytes. + if self.mode & 0o7000 != 0 || self.attributes.iter().any(|(name, _)| name == "security.capability") { + return Err(io::Error::other("cannot replace an export destination carrying executable privileges")); + } + let current = fs::metadata(path)?; + // Do not grant the destination owner the staging inode's initial owner-read bit + // while transferring ownership. ACL grants/denials are installed before final mode. + fs::set_permissions(path, fs::Permissions::from_mode(0))?; + if (current.uid(), current.gid()) != (self.uid, self.gid) { + std::os::unix::fs::chown(path, Some(self.uid), Some(self.gid))?; + } + for (name, _) in Self::read(path)?.attributes { + if !self.attributes.iter().any(|(wanted, _)| wanted == &name) { + xattr::remove(path, name)?; + } + } + for (name, value) in &self.attributes { + if xattr::get(path, name)?.as_deref() != Some(value.as_slice()) { + xattr::set(path, name, value)?; + } + } + #[cfg(any(target_os = "macos", target_os = "freebsd"))] + exacl::setfacl(&[path], &self.acl, None)?; + // Restore access ACLs before mode bits: chmod must not open an inherited ACL mask + // or permit access before a macOS deny entry has been installed. + fs::set_permissions(path, fs::Permissions::from_mode(self.mode))?; + Ok(()) + } +} + +pub(super) fn ordinary_file(parent: &Path) -> io::Result { + tempfile::Builder::new().prefix(".effectcraft-policy-").permissions(fs::Permissions::from_mode(0o666)).tempfile_in(parent) +} + +pub(super) fn private_file(parent: &Path) -> io::Result<(tempfile::NamedTempFile, Option)> { + #[cfg(any(target_os = "macos", target_os = "freebsd"))] + { + // macOS ACL allows can override mode 0600. Clear inheritance in an empty owned + // directory before creating the media inode; the empty directory never holds media. + let directory = tempfile::Builder::new().prefix(".effectcraft-export-").tempdir_in(parent)?; + #[cfg(target_os = "macos")] + exacl::setfacl(&[directory.path()], &[], None)?; + #[cfg(target_os = "freebsd")] + exacl::setfacl( + &[directory.path()], + &[ + exacl::AclEntry::allow_user("", exacl::Perm::READ | exacl::Perm::WRITE | exacl::Perm::EXECUTE, None), + exacl::AclEntry::allow_group("", exacl::Perm::empty(), None), + exacl::AclEntry::allow_other(exacl::Perm::empty(), None), + ], + None, + )?; + fs::set_permissions(directory.path(), fs::Permissions::from_mode(0o700))?; + let file = tempfile::NamedTempFile::new_in(directory.path())?; + Ok((file, Some(directory))) + } + #[cfg(not(any(target_os = "macos", target_os = "freebsd")))] + Ok((tempfile::Builder::new().prefix(".effectcraft-export-").tempfile_in(parent)?, None)) +} + +pub(super) fn identity(file: &File) -> io::Result<(u64, u64)> { + let metadata = file.metadata()?; + Ok((metadata.dev(), metadata.ino())) +} + +pub(super) fn links(file: &File) -> io::Result { + Ok(file.metadata()?.nlink()) +} + +pub(super) fn change_stamp(file: &File) -> io::Result<(i64, i64)> { + let metadata = file.metadata()?; + Ok((metadata.ctime(), metadata.ctime_nsec())) +} diff --git a/crates/export/src/pending/windows.rs b/crates/export/src/pending/windows.rs new file mode 100644 index 000000000..8b23ee441 --- /dev/null +++ b/crates/export/src/pending/windows.rs @@ -0,0 +1,111 @@ +use std::{ + ffi::OsString, + fs::{self, File, OpenOptions}, + io, + path::Path, +}; +use windows_permissions::{ + constants::{SeObjectType as O, SecurityInformation as S}, + wrappers as w, +}; + +const OBJECT: O = O::SE_FILE_OBJECT; + +#[derive(Clone, Debug, PartialEq, Eq)] +pub(super) struct Policy { + descriptor: OsString, + readonly: bool, +} + +impl Policy { + pub fn read(path: &Path) -> io::Result { + let descriptor = w::GetNamedSecurityInfo(path.as_os_str(), OBJECT, S::Owner | S::Group | S::Dacl | S::Label | S::Attribute | S::Scope)?; + Ok(Self { + descriptor: w::ConvertSecurityDescriptorToStringSecurityDescriptor(&descriptor, S::Owner | S::Group | S::Dacl | S::Sacl)?, + readonly: path.metadata()?.permissions().readonly(), + }) + } + + pub fn apply(&self, path: &Path) -> io::Result<()> { + let descriptor = w::ConvertStringSecurityDescriptorToSecurityDescriptor(&self.descriptor)?; + // SDDL's D: flags precede the first ACE. Preserve inheritance instead of freezing + // every inherited ACE as an explicit grant. Staging is a sibling of the final file. + let text = self.descriptor.to_string_lossy(); + let protected = text.split("D:").nth(1).unwrap_or("").split('(').next().unwrap_or("").split("S:").next().unwrap_or("").contains('P'); + let sacl = w::GetSecurityDescriptorSacl(&descriptor)?; + let mut flags = S::Owner | S::Group | S::Dacl | if protected { S::ProtectedDacl } else { S::UnprotectedDacl }; + if sacl.is_some() { + flags |= S::Label | S::Attribute | S::Scope; + } + w::SetNamedSecurityInfo( + path.as_os_str(), + OBJECT, + flags, + w::GetSecurityDescriptorOwner(&descriptor)?, + w::GetSecurityDescriptorGroup(&descriptor)?, + w::GetSecurityDescriptorDacl(&descriptor)?, + sacl, + )?; + let mut permissions = path.metadata()?.permissions(); + permissions.set_readonly(self.readonly); + fs::set_permissions(path, permissions) + } +} + +pub(super) fn ordinary_file(parent: &Path) -> io::Result { + tempfile::Builder::new().prefix(".effectcraft-policy-").tempfile_in(parent) +} + +pub(super) fn private_file(parent: &Path) -> io::Result<(tempfile::NamedTempFile, Option)> { + let file = tempfile::Builder::new().prefix(".effectcraft-export-").make_in(parent, |path| { + let name = path.to_str().ok_or_else(|| io::Error::other("non-Unicode export staging path"))?; + let mut acl = winsafe::ACL { AclRevision: 2, AclSize: std::mem::size_of::() as u16, ..Default::default() }; + let mut descriptor = winsafe::InitializeSecurityDescriptor().map_err(io::Error::other)?; + descriptor.Control = winsafe::co::SE::DACL_PRESENT | winsafe::co::SE::DACL_PROTECTED; + descriptor.Dacl = &mut acl; + let mut attributes = winsafe::SECURITY_ATTRIBUTES::default(); + attributes.set_lpSecurityDescriptor(Some(&mut descriptor)); + // An empty protected DACL denies access at creation, including during the transition + // to owner-only access. No broadly readable handle can predate the media writes. + let (handle, _) = winsafe::HFILE::CreateFile( + name, + winsafe::co::GENERIC::READ | winsafe::co::GENERIC::WRITE, + Some(winsafe::co::FILE_SHARE::READ | winsafe::co::FILE_SHARE::WRITE | winsafe::co::FILE_SHARE::DELETE), + Some(&attributes), + winsafe::co::DISPOSITION::CREATE_NEW, + winsafe::co::FILE_ATTRIBUTE::NORMAL, + None, + None, + None, + ) + .map_err(|error| io::Error::from_raw_os_error(error.raw() as i32))?; + let result = (|| { + let owner_descriptor = w::GetNamedSecurityInfo(path.as_os_str(), OBJECT, S::Owner)?; + let owner = w::GetSecurityDescriptorOwner(&owner_descriptor)?.ok_or_else(|| io::Error::other("staging has no owner"))?; + let sid = w::ConvertSidToStringSid(owner)?; + let private = w::ConvertStringSecurityDescriptorToSecurityDescriptor(&format!("D:P(A;;FA;;;{})", sid.to_string_lossy()))?; + w::SetNamedSecurityInfo(path.as_os_str(), OBJECT, S::Dacl | S::ProtectedDacl, None, None, w::GetSecurityDescriptorDacl(&private)?, None)?; + OpenOptions::new().read(true).write(true).open(path) + })(); + drop(handle); + if result.is_err() { + let _ = fs::remove_file(path); // No media exists; retain the original setup error. + } + result + })?; + Ok((file, None)) +} + +pub(super) fn identity(file: &File) -> io::Result<(u64, u64)> { + let info = winapi_util::file::information(file)?; + Ok((info.volume_serial_number(), info.file_index())) +} + +pub(super) fn links(file: &File) -> io::Result { + Ok(winapi_util::file::information(file)?.number_of_links()) +} + +pub(super) fn change_stamp(file: &File) -> io::Result<(i64, i64)> { + let stamp = winapi_util::file::information(file)?.creation_time().ok_or_else(|| io::Error::other("missing file creation time"))?; + Ok((stamp as i64, 0)) +} diff --git a/crates/export/src/pipeline.rs b/crates/export/src/pipeline.rs index 8dcee03e4..51867d8c5 100644 --- a/crates/export/src/pipeline.rs +++ b/crates/export/src/pipeline.rs @@ -104,7 +104,7 @@ pub(crate) struct Cx<'a> { pub frame_times: Option>>, /// Files written to an overflow folder. pub overflowed: Mutex>, - written: Mutex>, + pub pending: crate::pending::Pending, } impl<'a> std::ops::Deref for Cx<'a> { @@ -152,7 +152,7 @@ impl<'a> Cx<'a> { let cache = LayerCache::default(); // With deferred readbacks, buffers computed from a missed pass's placeholders are not kept. cache.set_gate(job.accel.and_then(|a| a.miss_gate())); - Cx { job, project, cache, frame_times, overflowed: Mutex::new(vec![]), written: Mutex::new(vec![]) } + Cx { job, project, cache, frame_times, overflowed: Mutex::new(vec![]), pending: crate::pending::Pending::default() } } pub fn comp(&self) -> Option<&Comp> { @@ -292,28 +292,9 @@ impl<'a> Cx<'a> { crate::fit(px, img.width, img.height, w, h) } - /// Open a file and remember ownership for cancellation cleanup (including overflow paths). + /// Stage media under this job's ownership; final paths remain unchanged until publication. pub fn create(&self, path: &str) -> crate::Result> { - let file = out::create(self.sink, path)?; - if self.sink.is_none() { - self.written.lock().unwrap_or_else(std::sync::PoisonError::into_inner).push(path.to_owned()); - } - Ok(file) - } - - pub fn remove_cancelled_outputs(&self) -> crate::Result<()> { - let mut failure = None; - for path in self.written.lock().unwrap_or_else(std::sync::PoisonError::into_inner).iter() { - if let Err(e) = std::fs::remove_file(path) - && e.kind() != std::io::ErrorKind::NotFound - { - failure.get_or_insert_with(|| crate::ExportError::Io(format!("cannot remove cancelled output {path}: {e}"))); - } - } - match failure { - Some(error) => Err(error), - None => Ok(()), - } + self.pending.create(self.sink, path) } /// Where a file goes: `path`, or (Use Storage Overflow) the first overflow folder with room @@ -375,7 +356,7 @@ impl<'a> Cx<'a> { s.push_str(&format!(" Frame {} ({:.3} s): rendered in {:.1} ms\n", f0 + i as i64, t.seconds(), secs * 1000.0)); } } - let mut f = self.create(&path).ok()?; + let mut f = out::create(self.sink, &path).ok()?; std::io::Write::write_all(&mut f, s.as_bytes()).ok()?; f.finish().ok()?; Some(path) diff --git a/crates/export/tests/render_settings.rs b/crates/export/tests/render_settings.rs index 9e9a0a18f..a4b5afd4b 100644 --- a/crates/export/tests/render_settings.rs +++ b/crates/export/tests/render_settings.rs @@ -476,3 +476,128 @@ fn a_job_renders_on_one_compositor() { assert_eq!(cpu.frames.load(Ordering::SeqCst), 0); assert_eq!(c[0].get_pixel(10, 44).0, [255, 0, 0, 255], "the CPU drew the red block"); } + +/// Cancellation after an encoder has opened its output must preserve the prior export. +#[test] +fn cancelled_replacement_preserves_existing_movie_and_sequence() { + let dir = std::env::temp_dir().join(format!("ec-preserve-export-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + let (p, cid) = project(FrameRate::new(25, 1), |_, _| {}); + for format in [OutputFormat::H264, OutputFormat::PngSequence] { + let om = OutputModule::for_format(format); + let path = dir.join(if format.is_sequence() { "existing_[#####].png" } else { "existing.mp4" }); + let final_path = if format.is_sequence() { dir.join("existing_00000.png") } else { path.clone() }; + std::fs::write(&final_path, b"previous complete export").unwrap(); + let result = export( + &Job { + project: &p, + footage: &NoFootage, + expr: None, + accel: None, + comp: cid, + settings: &RenderSettings::default(), + output: &om, + path: path.to_str().unwrap(), + sink: None, + nested_switches: true, + options: JobOptions::default(), + }, + &mut |progress| progress.done == 0, + ); + assert!(matches!(result, Err(effectcraft_export::ExportError::Cancelled))); + assert_eq!(std::fs::read(&final_path).ok().as_deref(), Some(b"previous complete export".as_slice())); + std::fs::remove_file(final_path).unwrap(); + assert_eq!(std::fs::read_dir(&dir).unwrap().count(), 0); + } + std::fs::remove_dir(dir).unwrap(); +} + +/// A complete two-second export matches the sink output byte-for-byte, both for a new +/// destination and a replacement. This exercises the real muxer and parallel sequence path. +#[test] +fn published_movie_and_sequence_match_complete_sink_outputs() { + let dir = tempfile::tempdir().unwrap(); + let (p, cid) = project(FrameRate::new(25, 1), |_, _| {}); + let settings = RenderSettings::default(); + for format in [OutputFormat::Uncompressed, OutputFormat::PngSequence] { + let om = OutputModule::for_format(format); + let path = dir.path().join(if format.is_sequence() { "movie_[#####].png" } else { "movie.mov" }); + let path = path.to_str().unwrap(); + let expected = run(&p, cid, &settings, &om, path); + for replace in [false, true] { + if replace { + for (name, _) in expected.lock().unwrap().iter() { + std::fs::write(name, b"previous export").unwrap(); + } + } + let result = export( + &Job { + project: &p, + footage: &NoFootage, + expr: None, + accel: None, + comp: cid, + settings: &settings, + output: &om, + path, + sink: None, + nested_switches: true, + options: JobOptions::default(), + }, + &mut |_| true, + ) + .unwrap(); + assert_eq!(result.frames, 50); + for (name, bytes) in expected.lock().unwrap().iter() { + assert_eq!(&std::fs::read(name).unwrap(), bytes, "{name}"); + } + } + for (name, _) in expected.lock().unwrap().iter() { + std::fs::remove_file(name).unwrap(); + } + assert_eq!(std::fs::read_dir(dir.path()).unwrap().count(), 0); + } +} + +/// An I/O failure after a sequence starts must also preserve all earlier complete outputs. +#[test] +fn sequence_write_failure_preserves_existing_frames() { + let dir = tempfile::tempdir().unwrap(); + let blocked = dir.path().join("blocked-overflow"); + std::fs::write(&blocked, b"ordinary file, not a directory").unwrap(); + let path = dir.path().join("movie_[#####].png"); + let path = path.to_str().unwrap(); + for frame in 0..50 { + std::fs::write(sequence_path(path, frame), b"old complete frame").unwrap(); + } + struct FailAfterFirst(AtomicUsize); + impl StorageQuota for FailAfterFirst { + fn has_room(&self, path: &str, _: u64) -> bool { + path.contains("blocked-overflow") || self.0.fetch_add(1, Ordering::SeqCst) == 0 + } + } + let storage = FailAfterFirst(AtomicUsize::new(0)); + let (p, cid) = project(FrameRate::new(25, 1), |_, _| {}); + let result = export( + &Job { + project: &p, + footage: &NoFootage, + expr: None, + accel: None, + comp: cid, + settings: &RenderSettings { storage_overflow: true, ..Default::default() }, + output: &OutputModule::for_format(OutputFormat::PngSequence), + path, + sink: None, + nested_switches: true, + options: JobOptions { storage: Some(&storage), overflow: vec![blocked.to_string_lossy().into_owned()], ..Default::default() }, + }, + &mut |_| true, + ); + assert!(matches!(result, Err(effectcraft_export::ExportError::Io(_))), "{result:?}"); + for frame in 0..50 { + assert_eq!(std::fs::read(sequence_path(path, frame)).unwrap(), b"old complete frame"); + } + assert_eq!(std::fs::read(blocked).unwrap(), b"ordinary file, not a directory"); + assert!(std::fs::read_dir(dir.path()).unwrap().all(|entry| !entry.unwrap().file_name().to_string_lossy().starts_with(".effectcraft-"))); +} diff --git a/docs/export-output-safety.md b/docs/export-output-safety.md new file mode 100644 index 000000000..903079cd7 --- /dev/null +++ b/docs/export-output-safety.md @@ -0,0 +1,67 @@ +# Export output lifetime + +Disk exports encode into job-owned staging on the destination filesystem. Existing +outputs are untouched until all encoding succeeds. A cancelled, failed, or abandoned +job removes its staging, never a final output selected by its filename. Browser +sinks retain per-file delivery, and render logs retain their separate behavior. + +Publication replaces one complete file at a time. A sequence is **not** an atomic +transaction: if publication of a later frame fails, previously published complete +frames remain. Staging needs enough free space for the new export alongside the +old export. This provides error/cancellation safety, not a power-loss durability +guarantee or automatic recovery of staging left by a terminated process. + +## Access policy + +Before receiving media, staging is restricted to its owner. An empty, separate +probe checks that the intended output policy can be reproduced. That probe never +receives media, so a reader retaining its handle cannot later read export bytes. +New outputs use ordinary creation permissions, including umask and directory +inheritance. Existing outputs retain owner, group, mode/access controls supported +by the platform adapter; errors preserve the old output instead of weakening access. + +- Windows creates staging with a protected, empty DACL, then grants its owner + access before reopening for encoding. Staging stays beside the destination so + publication can restore inherited as well as protected DACLs. The adapter checks + owner, group, DACL, integrity/resource access metadata and readonly state. +- Linux preserves numeric ownership, permission bits, and security/system xattrs, + including POSIX access ACLs. A setgid directory cannot silently change the owner + group of an existing output. No libacl dependency is added on Linux. +- macOS uses an empty private staging directory with its inherited ACL cleared + before creating the media file, because ACL grants can override ordinary mode + bits. Native ACL entries are restored along with ownership and mode. +- FreeBSD uses the native ACL adapter and an owner-only POSIX staging-directory + ACL. Filesystem policies the adapter cannot reproduce are rejected; unsupported + ACL/filesystem combinations must not silently fall back to chmod alone. + +Privilege-bearing files (setuid/setgid/sticky file modes or Linux file capabilities) +are not export replacement targets. This change preserves access policy, not all +arbitrary filesystem metadata such as user tags or alternate data streams. + +## Links and concurrent changes + +Leaf symbolic links are resolved with a bound of 40 links. Relative and dangling +links target their referenced filename; the link itself is retained. Directory +links are resolved by the filesystem. Files with multiple hard links are rejected +because replacing their inode would silently disconnect the other aliases. + +The destination identity, length, modification time, access policy, resolved target, +and parent access policy are checked again before publication. Changed destinations +cause an error. Jobs in one process serialize their final validation/replacement. +New destinations are published with a no-clobber hard link, so an intervening creator +cannot be overwritten. This requires a filesystem that supports hard links. + +External writers must cooperate. Portable rename does not provide compare-and-swap +against a file version, so there remains a race between the final check and replacing +an existing destination. This is not protection against arbitrary concurrent changes +by another process with authority to modify the output directory or its security +policy. Use separate output paths for independent processes. + +## Tests + +The native tests cover the staging lifecycle, competing jobs, policy changes, +failed publication, hard-link rejection, symlink targets, and platform ACL behavior. +Linux includes a named-user ACL and a differing-group setgid case when run as root. +The export integration tests cancel after writing begins and compare successful +movie/sequence output against sink bytes. The filesystem workflow runs these on +Windows, Linux and macOS. Local cross-compilation alone is not runtime validation. From a06d4dee171f95ad3b466fc0c3b75663eceb337e Mon Sep 17 00:00:00 2001 From: TimotheusG Date: Mon, 19 Oct 2026 23:13:01 -0400 Subject: [PATCH 2/8] G3: read access metadata before restricting staging permissions --- crates/export/src/pending/native.rs | 5 ++++- crates/export/src/pending/unix.rs | 12 +++++++----- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/crates/export/src/pending/native.rs b/crates/export/src/pending/native.rs index b92bc95f8..b011f8dd3 100644 --- a/crates/export/src/pending/native.rs +++ b/crates/export/src/pending/native.rs @@ -78,7 +78,10 @@ impl Staged { None => platform::Policy::read(probe.path())?, }; policy.apply(probe.path())?; - if platform::Policy::read(probe.path())? != policy { + let restored = platform::Policy::read(probe.path())?; + if restored != policy { + #[cfg(test)] + eprintln!("policy restoration mismatch: expected {policy:?}; restored {restored:?}"); return Err(io::Error::other("cannot preserve export destination access policy on this filesystem")); } probe.close()?; diff --git a/crates/export/src/pending/unix.rs b/crates/export/src/pending/unix.rs index e4918a4ec..1bbd2d787 100644 --- a/crates/export/src/pending/unix.rs +++ b/crates/export/src/pending/unix.rs @@ -47,20 +47,22 @@ impl Policy { if self.mode & 0o7000 != 0 || self.attributes.iter().any(|(name, _)| name == "security.capability") { return Err(io::Error::other("cannot replace an export destination carrying executable privileges")); } - let current = fs::metadata(path)?; + // Read access attributes before removing read permission: macOS requires it even + // when the caller owns the inode. Reuse this snapshot while restoring attributes. + let current = Self::read(path)?; // Do not grant the destination owner the staging inode's initial owner-read bit // while transferring ownership. ACL grants/denials are installed before final mode. fs::set_permissions(path, fs::Permissions::from_mode(0))?; - if (current.uid(), current.gid()) != (self.uid, self.gid) { + if (current.uid, current.gid) != (self.uid, self.gid) { std::os::unix::fs::chown(path, Some(self.uid), Some(self.gid))?; } - for (name, _) in Self::read(path)?.attributes { - if !self.attributes.iter().any(|(wanted, _)| wanted == &name) { + for (name, _) in ¤t.attributes { + if !self.attributes.iter().any(|(wanted, _)| wanted == name) { xattr::remove(path, name)?; } } for (name, value) in &self.attributes { - if xattr::get(path, name)?.as_deref() != Some(value.as_slice()) { + if !current.attributes.iter().any(|(old_name, old_value)| old_name == name && old_value == value) { xattr::set(path, name, value)?; } } From c8a9da2699596d8e60655eeb47cfa69722825a76 Mon Sep 17 00:00:00 2001 From: TimotheusG Date: Mon, 19 Oct 2026 23:36:28 -0400 Subject: [PATCH 3/8] G3: preserve equivalent Windows grants and preflight publication --- .github/workflows/export-filesystem.yml | 2 +- crates/export/src/pending.rs | 2 + crates/export/src/pending/native.rs | 15 +++++ crates/export/src/pending/unix.rs | 7 ++ crates/export/src/pending/windows.rs | 85 ++++++++++++++++++++++++- docs/export-output-safety.md | 7 +- 6 files changed, 115 insertions(+), 3 deletions(-) diff --git a/.github/workflows/export-filesystem.yml b/.github/workflows/export-filesystem.yml index 26dc39486..78ece3d0a 100644 --- a/.github/workflows/export-filesystem.yml +++ b/.github/workflows/export-filesystem.yml @@ -30,6 +30,6 @@ jobs: - uses: dtolnay/rust-toolchain@stable - uses: Swatinem/rust-cache@v2 - name: Native filesystem contract - run: cargo test --locked --release -p effectcraft-export --lib pending::native::tests + run: cargo test --locked --release -p effectcraft-export --lib pending::native - name: Export integration run: cargo test --locked --release -p effectcraft-export --test render_settings diff --git a/crates/export/src/pending.rs b/crates/export/src/pending.rs index 29d4720a4..ba5eb69fe 100644 --- a/crates/export/src/pending.rs +++ b/crates/export/src/pending.rs @@ -12,6 +12,7 @@ pub(crate) struct Pending { } impl Pending { + /// Open a sink directly or register a unique, private native staging file for this job. pub fn create<'a>(&self, sink: Option<&'a out::Sink>, path: &str) -> Result> { if sink.is_some() { return out::create(sink, path); @@ -47,6 +48,7 @@ impl Pending { Ok(()) } + /// Attempt cleanup of every pending file, returning the first cleanup failure. pub fn discard(&self) -> Result<()> { #[cfg(not(target_arch = "wasm32"))] { diff --git a/crates/export/src/pending/native.rs b/crates/export/src/pending/native.rs index b011f8dd3..e2302838e 100644 --- a/crates/export/src/pending/native.rs +++ b/crates/export/src/pending/native.rs @@ -36,6 +36,7 @@ struct Version { } impl Version { + /// Capture the writable destination's identity, content timestamp, and access policy. fn read(file: &File, path: &Path) -> io::Result { let metadata = file.metadata()?; if !metadata.is_file() || platform::links(file)? != 1 { @@ -52,6 +53,7 @@ impl Version { } impl Staged { + /// Check destination capabilities before creating private, job-owned media storage. pub fn create(path: &str) -> io::Result<(Self, File)> { let requested = std::path::absolute(path)?; let path = resolve(&requested, true)?; @@ -73,6 +75,14 @@ impl Staged { // The ordinary creation probe is always empty. No exported bytes ever enter an inode // that another account might have opened before access was restricted. let probe = platform::ordinary_file(parent)?; + if original.is_none() { + // Reject unsupported no-clobber publication before rendering. The probe contains + // no media; the owned directory also cleans its link on an early return. + let links = tempfile::Builder::new().prefix(".effectcraft-policy-").tempdir_in(parent)?; + fs::hard_link(probe.path(), links.path().join("link")) + .map_err(|error| io::Error::new(error.kind(), format!("export filesystem cannot publish new files with hard links: {error}")))?; + links.close()?; + } let policy = match &original { Some(version) => version.policy.clone(), None => platform::Policy::read(probe.path())?, @@ -92,6 +102,7 @@ impl Staged { Ok((staged, file)) } + /// Reject changed links, destination versions, or parent access policy. pub fn validate(&self) -> io::Result<()> { if resolve(&self.requested, false)? != self.path { return Err(io::Error::other("export destination symlink changed while rendering")); @@ -116,6 +127,7 @@ impl Staged { Ok(()) } + /// Serialize final validation/publication and clean staging even when publication fails. pub fn publish(self) -> io::Result<()> { let result = { let _publication = PUBLICATION.lock().unwrap_or_else(std::sync::PoisonError::into_inner); @@ -129,6 +141,7 @@ impl Staged { } } + /// Restore access policy and publish one complete file without clobbering a new creator. fn publish_inner(&self) -> io::Result<()> { self.validate()?; self.policy.apply(&self.temporary)?; @@ -146,6 +159,7 @@ impl Staged { Ok(()) } + /// Remove only owned staging paths; a successful rename already consumed the file path. pub fn discard(self) -> io::Result<()> { if let Err(error) = self.temporary.close() && error.kind() != io::ErrorKind::NotFound @@ -188,6 +202,7 @@ mod tests { use super::*; use std::io::Write; + /// Prepare completed test bytes and close the writer before publication assertions. fn stage(path: &Path, bytes: &[u8]) -> Staged { let (staged, mut file) = Staged::create(path.to_str().unwrap()).unwrap(); file.write_all(bytes).unwrap(); diff --git a/crates/export/src/pending/unix.rs b/crates/export/src/pending/unix.rs index 1bbd2d787..1b51f0bc7 100644 --- a/crates/export/src/pending/unix.rs +++ b/crates/export/src/pending/unix.rs @@ -17,6 +17,7 @@ pub(super) struct Policy { } impl Policy { + /// Snapshot ownership, access controls, and permission state used for publication checks. pub fn read(path: &Path) -> io::Result { let metadata = fs::metadata(path)?; let mut attributes = Vec::new(); @@ -41,6 +42,7 @@ impl Policy { }) } + /// Restore the captured access policy; unsupported or unauthorized changes fail closed. pub fn apply(&self, path: &Path) -> io::Result<()> { // An export is data, not a privileged executable. Do not transfer privilege bits or // file capabilities onto newly generated bytes. @@ -75,10 +77,12 @@ impl Policy { } } +/// Create an empty probe using the destination directory's ordinary creation policy. pub(super) fn ordinary_file(parent: &Path) -> io::Result { tempfile::Builder::new().prefix(".effectcraft-policy-").permissions(fs::Permissions::from_mode(0o666)).tempfile_in(parent) } +/// Create job-owned staging that is private before any media bytes are written. pub(super) fn private_file(parent: &Path) -> io::Result<(tempfile::NamedTempFile, Option)> { #[cfg(any(target_os = "macos", target_os = "freebsd"))] { @@ -105,15 +109,18 @@ pub(super) fn private_file(parent: &Path) -> io::Result<(tempfile::NamedTempFile Ok((tempfile::Builder::new().prefix(".effectcraft-export-").tempfile_in(parent)?, None)) } +/// Identify the open inode independently of its pathname. pub(super) fn identity(file: &File) -> io::Result<(u64, u64)> { let metadata = file.metadata()?; Ok((metadata.dev(), metadata.ino())) } +/// Count aliases so replacement cannot silently detach a hard-linked destination. pub(super) fn links(file: &File) -> io::Result { Ok(file.metadata()?.nlink()) } +/// Capture inode change time to detect content or policy changes beyond modification time. pub(super) fn change_stamp(file: &File) -> io::Result<(i64, i64)> { let metadata = file.metadata()?; Ok((metadata.ctime(), metadata.ctime_nsec())) diff --git a/crates/export/src/pending/windows.rs b/crates/export/src/pending/windows.rs index 8b23ee441..1f462eac7 100644 --- a/crates/export/src/pending/windows.rs +++ b/crates/export/src/pending/windows.rs @@ -11,13 +11,66 @@ use windows_permissions::{ const OBJECT: O = O::SE_FILE_OBJECT; -#[derive(Clone, Debug, PartialEq, Eq)] +#[derive(Clone, Debug, Eq)] pub(super) struct Policy { descriptor: OsString, readonly: bool, } +impl PartialEq for Policy { + /// Compare access policy while tolerating Windows' duplicate-grant legacy conversion. + fn eq(&self, other: &Self) -> bool { + self.readonly == other.readonly + && comparison_descriptor(&self.descriptor.to_string_lossy()) == comparison_descriptor(&other.descriptor.to_string_lossy()) + } +} + +/// Canonicalize only simple, unprotected file allow ACLs whose inherited ACEs duplicate +/// explicit grants. SetNamedSecurityInfo adds these during legacy ACL conversion; AI +/// records conversion to the current inheritance model. All other descriptors remain +/// exact, including denies, new inherited rights, owner/group, protection, and SACLs. +fn comparison_descriptor(descriptor: &str) -> String { + let Some((owner, acl)) = descriptor.split_once("D:") else { return descriptor.into() }; + let (acl, sacl) = match acl.split_once("S:") { + Some((acl, sacl)) => (acl, Some(sacl)), + None => (acl, None), + }; + let acl = acl.strip_prefix("AI").unwrap_or(acl); + let aces = acl.split_inclusive(')').collect::>(); + if aces.is_empty() + || !aces.iter().all(|ace| { + let fields = ace.split(';').collect::>(); + fields.len() == 6 + && fields.first() == Some(&"(A") + && matches!(fields.get(1), Some(&"") | Some(&"ID")) + && fields.get(3) == Some(&"") + && fields.get(4) == Some(&"") + && ace.ends_with(')') + && ace.matches('(').count() == 1 + }) + { + return descriptor.into(); + } + for ace in &aces { + if let Some(grant) = ace.strip_prefix("(A;ID;") + && !aces.iter().any(|other| other.strip_prefix("(A;;") == Some(grant)) + { + return descriptor.into(); + } + } + let mut result = format!("{owner}D:"); + for ace in aces.iter().filter(|ace| ace.starts_with("(A;;")) { + result.push_str(ace); + } + if let Some(sacl) = sacl { + result.push_str("S:"); + result.push_str(sacl); + } + result +} + impl Policy { + /// Snapshot ownership, access controls, and permission state used for publication checks. pub fn read(path: &Path) -> io::Result { let descriptor = w::GetNamedSecurityInfo(path.as_os_str(), OBJECT, S::Owner | S::Group | S::Dacl | S::Label | S::Attribute | S::Scope)?; Ok(Self { @@ -26,6 +79,7 @@ impl Policy { }) } + /// Restore the captured access policy; unsupported or unauthorized changes fail closed. pub fn apply(&self, path: &Path) -> io::Result<()> { let descriptor = w::ConvertStringSecurityDescriptorToSecurityDescriptor(&self.descriptor)?; // SDDL's D: flags precede the first ACE. Preserve inheritance instead of freezing @@ -52,10 +106,12 @@ impl Policy { } } +/// Create an empty probe using the destination directory's ordinary creation policy. pub(super) fn ordinary_file(parent: &Path) -> io::Result { tempfile::Builder::new().prefix(".effectcraft-policy-").tempfile_in(parent) } +/// Create job-owned staging that is private before any media bytes are written. pub(super) fn private_file(parent: &Path) -> io::Result<(tempfile::NamedTempFile, Option)> { let file = tempfile::Builder::new().prefix(".effectcraft-export-").make_in(parent, |path| { let name = path.to_str().ok_or_else(|| io::Error::other("non-Unicode export staging path"))?; @@ -96,16 +152,43 @@ pub(super) fn private_file(parent: &Path) -> io::Result<(tempfile::NamedTempFile Ok((file, None)) } +/// Identify the open inode independently of its pathname. pub(super) fn identity(file: &File) -> io::Result<(u64, u64)> { let info = winapi_util::file::information(file)?; Ok((info.volume_serial_number(), info.file_index())) } +/// Count aliases so replacement cannot silently detach a hard-linked destination. pub(super) fn links(file: &File) -> io::Result { Ok(winapi_util::file::information(file)?.number_of_links()) } +/// Capture creation time as an additional guard against reused file identities. pub(super) fn change_stamp(file: &File) -> io::Result<(i64, i64)> { let stamp = winapi_util::file::information(file)?.creation_time().ok_or_else(|| io::Error::other("missing file creation time"))?; Ok((stamp as i64, 0)) } + +#[cfg(test)] +mod tests { + use super::*; + + /// The runner's legacy ACL conversion is equivalent; additional rights or changed + /// ownership, protection, denies, and security labels must still be rejected. + #[test] + fn legacy_conversion_accepts_only_duplicate_allow_grants() { + let old = "O:BAG:SYD:(A;;FA;;;SY)(A;;FA;;;BA)"; + let converted = "O:BAG:SYD:AI(A;;FA;;;SY)(A;;FA;;;BA)(A;ID;FA;;;SY)(A;ID;FA;;;BA)"; + assert_eq!(comparison_descriptor(old), comparison_descriptor(converted)); + for changed in [ + converted.replace("O:BA", "O:SY"), + converted.replace("D:AI", "D:PAI"), + format!("{converted}(A;ID;FA;;;WD)"), + converted.replace("(A;ID;FA;;;BA)", "(A;ID;FR;;;BA)"), + format!("{converted}S:(ML;;NW;;;LW)"), + ] { + assert_ne!(comparison_descriptor(old), comparison_descriptor(&changed), "{changed}"); + } + assert_ne!(comparison_descriptor("O:BAG:SYD:(D;;FW;;;WD)(A;;FA;;;SY)"), comparison_descriptor("O:BAG:SYD:AI(D;;FW;;;WD)(A;;FA;;;SY)(A;ID;FA;;;SY)")); + } +} diff --git a/docs/export-output-safety.md b/docs/export-output-safety.md index 903079cd7..81cad8b6c 100644 --- a/docs/export-output-safety.md +++ b/docs/export-output-safety.md @@ -24,6 +24,10 @@ by the platform adapter; errors preserve the old output instead of weakening acc access before reopening for encoding. Staging stays beside the destination so publication can restore inherited as well as protected DACLs. The adapter checks owner, group, DACL, integrity/resource access metadata and readonly state. + Windows may modernize a legacy unprotected file ACL by appending inherited + duplicates of existing unconditional allow grants. Only that equivalent form + is accepted; new rights, deny changes, protection changes, and SACL changes + still fail policy verification. - Linux preserves numeric ownership, permission bits, and security/system xattrs, including POSIX access ACLs. A setgid directory cannot silently change the owner group of an existing output. No libacl dependency is added on Linux. @@ -49,7 +53,8 @@ The destination identity, length, modification time, access policy, resolved tar and parent access policy are checked again before publication. Changed destinations cause an error. Jobs in one process serialize their final validation/replacement. New destinations are published with a no-clobber hard link, so an intervening creator -cannot be overwritten. This requires a filesystem that supports hard links. +cannot be overwritten. This requires a filesystem that supports hard links; an empty probe checks that +capability before encoding begins. External writers must cooperate. Portable rename does not provide compare-and-swap against a file version, so there remains a race between the final check and replacing From 689e5832386996496790eae117b32fcc71bc392b Mon Sep 17 00:00:00 2001 From: TimotheusG Date: Mon, 19 Oct 2026 23:59:40 -0400 Subject: [PATCH 4/8] G3: compare canonical SIDs in Windows staging test --- crates/export/src/pending/native.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/crates/export/src/pending/native.rs b/crates/export/src/pending/native.rs index e2302838e..259d5bdbd 100644 --- a/crates/export/src/pending/native.rs +++ b/crates/export/src/pending/native.rs @@ -393,7 +393,10 @@ mod tests { let staged = stage(&path, b"replacement"); let private = w::GetNamedSecurityInfo(staged.temporary.as_os_str(), O::SE_FILE_OBJECT, S::Dacl).unwrap(); let private_text = w::ConvertSecurityDescriptorToStringSecurityDescriptor(&private, S::Dacl).unwrap(); - assert_eq!(private_text.to_string_lossy().replace("D:PAI", "D:P"), format!("D:P(A;;FA;;;{})", sid.to_string_lossy())); + // SDDL abbreviates well-known SIDs (for example S-1-5-32-544 becomes BA). + // Compare descriptors serialized by the same API rather than a numeric SID. + let restricted_text = w::ConvertSecurityDescriptorToStringSecurityDescriptor(&restricted, S::Dacl).unwrap(); + assert_eq!(private_text.to_string_lossy().replace("D:PAI", "D:P"), restricted_text.to_string_lossy()); staged.publish().unwrap(); assert_eq!(platform::Policy::read(&path).unwrap(), expected); let staged = stage(&path, b"must not publish"); From f2177933850406c51c1e6b6a892d1194694e94ec Mon Sep 17 00:00:00 2001 From: TimotheusG Date: Tue, 20 Oct 2026 00:14:53 -0400 Subject: [PATCH 5/8] G3: preserve full Windows audit policy or refuse replacement --- .github/workflows/export-filesystem.yml | 5 ++ crates/export/examples/windows_audit_tests.rs | 39 ++++++++++ crates/export/src/pending/native.rs | 73 +++++++++++++++++-- crates/export/src/pending/unix.rs | 10 +++ crates/export/src/pending/windows.rs | 33 ++++++++- crates/export/tests/render_settings.rs | 3 + docs/export-output-safety.md | 14 ++++ 7 files changed, 170 insertions(+), 7 deletions(-) create mode 100644 crates/export/examples/windows_audit_tests.rs diff --git a/.github/workflows/export-filesystem.yml b/.github/workflows/export-filesystem.yml index 78ece3d0a..90debf018 100644 --- a/.github/workflows/export-filesystem.yml +++ b/.github/workflows/export-filesystem.yml @@ -30,6 +30,11 @@ jobs: - uses: dtolnay/rust-toolchain@stable - uses: Swatinem/rust-cache@v2 - name: Native filesystem contract + if: runner.os != 'Windows' run: cargo test --locked --release -p effectcraft-export --lib pending::native - name: Export integration + if: runner.os != 'Windows' run: cargo test --locked --release -p effectcraft-export --test render_settings + - name: Windows audit-safe replacement and unprivileged refusal + if: runner.os == 'Windows' + run: cargo run --locked --release -p effectcraft-export --example windows_audit_tests diff --git a/crates/export/examples/windows_audit_tests.rs b/crates/export/examples/windows_audit_tests.rs new file mode 100644 index 000000000..5a0c9da21 --- /dev/null +++ b/crates/export/examples/windows_audit_tests.rs @@ -0,0 +1,39 @@ +//! CI-only launcher for tests requiring Windows audit-policy privileges. +//! Enables a privilege already granted to this process; it cannot grant one to an +//! ordinary account. Children inherit it. The application never performs this step. + +/// Run the complete Windows filesystem and export contract with audit access enabled. +#[cfg(windows)] +fn main() -> Result<(), Box> { + use windows_permissions::{ + constants::{SeObjectType, SecurityInformation}, + wrappers, + }; + use winsafe::{DisabPriv, HPROCESS, LUID_AND_ATTRIBUTES, TOKEN_PRIVILEGES, co}; + + let token = HPROCESS::GetCurrentProcess().OpenProcessToken(co::TOKEN::ADJUST_PRIVILEGES | co::TOKEN::QUERY)?; + let luid = winsafe::LookupPrivilegeValue(None, co::SE_PRIV::SECURITY_NAME)?; + let privileges = TOKEN_PRIVILEGES::new(&[LUID_AND_ATTRIBUTES::new(luid, co::SE_PRIV_ATTR::ENABLED)])?; + token.AdjustTokenPrivileges(DisabPriv::Privs(&privileges))?; + // AdjustTokenPrivileges can succeed without assigning every requested privilege. + // Prove full SACL access before claiming that privileged tests can run. + let probe = tempfile::NamedTempFile::new()?; + wrappers::GetNamedSecurityInfo(probe.path().as_os_str(), SeObjectType::SE_FILE_OBJECT, SecurityInformation::Sacl)?; + probe.close()?; + + for selection in [vec!["--lib", "pending::native"], vec!["--test", "render_settings"]] { + let status = std::process::Command::new(env!("CARGO")) + .args(["test", "--locked", "--release", "-p", "effectcraft-export"]) + .args(selection) + .args(["--", "--include-ignored"]) + .status()?; + if !status.success() { + return Err(std::io::Error::other("Windows audit export tests failed").into()); + } + } + Ok(()) +} + +/// Audit privileges are Windows-specific; other platforms use the ordinary test commands. +#[cfg(not(windows))] +fn main() {} diff --git a/crates/export/src/pending/native.rs b/crates/export/src/pending/native.rs index 259d5bdbd..c2ab09450 100644 --- a/crates/export/src/pending/native.rs +++ b/crates/export/src/pending/native.rs @@ -47,7 +47,7 @@ impl Version { change_stamp: platform::change_stamp(file)?, len: metadata.len(), modified: metadata.modified()?, - policy: platform::Policy::read(path)?, + policy: platform::Policy::read_existing(path)?, }) } } @@ -88,10 +88,7 @@ impl Staged { None => platform::Policy::read(probe.path())?, }; policy.apply(probe.path())?; - let restored = platform::Policy::read(probe.path())?; - if restored != policy { - #[cfg(test)] - eprintln!("policy restoration mismatch: expected {policy:?}; restored {restored:?}"); + if !policy.matches(probe.path())? { return Err(io::Error::other("cannot preserve export destination access policy on this filesystem")); } probe.close()?; @@ -145,7 +142,7 @@ impl Staged { fn publish_inner(&self) -> io::Result<()> { self.validate()?; self.policy.apply(&self.temporary)?; - if platform::Policy::read(&self.temporary)? != self.policy { + if !self.policy.matches(&self.temporary)? { return Err(io::Error::other("staged export access policy could not be preserved")); } self.validate()?; @@ -212,6 +209,7 @@ mod tests { } #[test] + #[cfg_attr(windows, ignore = "requires enabled SeSecurityPrivilege; exercised by the Windows audit CI runner")] fn lifecycle_preserves_existing_bytes_until_publish_and_cleans_owned_files() { for existing in [false, true] { for action in ["drop", "discard", "publish"] { @@ -248,6 +246,7 @@ mod tests { } #[test] + #[cfg_attr(windows, ignore = "requires enabled SeSecurityPrivilege; exercised by the Windows audit CI runner")] fn competing_jobs_and_external_writes_are_not_overwritten_or_deleted() { for existing in [false, true] { let dir = tempfile::tempdir().unwrap(); @@ -365,6 +364,7 @@ mod tests { #[cfg(windows)] #[test] + #[cfg_attr(windows, ignore = "requires enabled SeSecurityPrivilege; exercised by the Windows audit CI runner")] fn windows_inherited_and_protected_acls_and_policy_changes() { use windows_permissions::{ constants::{SeObjectType as O, SecurityInformation as S}, @@ -461,6 +461,7 @@ mod tests { } /// More frames than a typical descriptor limit: snapshots retain metadata, not open files. #[test] + #[cfg_attr(windows, ignore = "requires enabled SeSecurityPrivilege; exercised by the Windows audit CI runner")] fn replacement_sequence_keeps_file_handle_use_constant() { let dir = tempfile::tempdir().unwrap(); let mut pending = Vec::new(); @@ -477,4 +478,64 @@ mod tests { } assert_eq!(fs::read_dir(dir.path()).unwrap().count(), 1536); } + + /// Refusal must leave existing bytes and policy intact without disabling new exports. + #[cfg(windows)] + #[test] + fn windows_audit_refusal_preserves_existing_and_allows_new() { + const CHILD: &str = "EFFECTCRAFT_AUDIT_REFUSAL_CHILD"; + if std::env::var_os(CHILD).is_none() { + let name = format!("{}::windows_audit_refusal_preserves_existing_and_allows_new", module_path!().split_once("::").unwrap().1); + let status = std::process::Command::new(std::env::current_exe().unwrap()).args(["--exact", &name, "--nocapture"]).env(CHILD, "1").status().unwrap(); + assert!(status.success()); + return; + } + // Privilege removal affects only this isolated child, never parallel tests. + let token = winsafe::HPROCESS::GetCurrentProcess().OpenProcessToken(winsafe::co::TOKEN::ADJUST_PRIVILEGES | winsafe::co::TOKEN::QUERY).unwrap(); + let luid = winsafe::LookupPrivilegeValue(None, winsafe::co::SE_PRIV::SECURITY_NAME).unwrap(); + let privileges = winsafe::TOKEN_PRIVILEGES::new(&[winsafe::LUID_AND_ATTRIBUTES::new(luid, winsafe::co::SE_PRIV_ATTR::REMOVED)]).unwrap(); + token.AdjustTokenPrivileges(winsafe::DisabPriv::Privs(&privileges)).unwrap(); + let dir = tempfile::tempdir().unwrap(); + let existing = dir.path().join("old.mov"); + fs::write(&existing, b"old complete movie").unwrap(); + let error = match Staged::create(existing.to_str().unwrap()) { + Ok(_) => panic!("replacement unexpectedly succeeded without audit privilege"), + Err(error) => error, + }; + assert!(error.to_string().contains("Windows audit policy"), "{error}"); + assert_eq!(fs::read(&existing).unwrap(), b"old complete movie"); + let new = dir.path().join("new.mov"); + stage(&new, b"new complete movie").publish().unwrap(); + assert_eq!(fs::read(new).unwrap(), b"new complete movie"); + assert_eq!(fs::read_dir(dir.path()).unwrap().count(), 2); + } + + /// Audit ACEs and SACL protection survive replacement; changes during rendering refuse it. + #[cfg(windows)] + #[test] + #[ignore = "requires enabled SeSecurityPrivilege; exercised by the Windows audit CI runner"] + fn windows_audit_entries_and_protection_are_preserved() { + use windows_permissions::{ + constants::{SeObjectType as O, SecurityInformation as S}, + wrappers as w, + }; + for protected in [false, true] { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("audited.mov"); + fs::write(&path, b"old complete movie").unwrap(); + let descriptor = w::ConvertStringSecurityDescriptorToSecurityDescriptor("S:P(AU;SAFA;FW;;;WD)").unwrap(); + let flags = S::Sacl | if protected { S::ProtectedSacl } else { S::UnprotectedSacl }; + w::SetNamedSecurityInfo(path.as_os_str(), O::SE_FILE_OBJECT, flags, None, None, None, w::GetSecurityDescriptorSacl(&descriptor).unwrap()).unwrap(); + let expected = platform::Policy::read_existing(&path).unwrap(); + stage(&path, b"complete replacement").publish().unwrap(); + assert!(expected.matches(&path).unwrap()); + let staged = stage(&path, b"must not publish"); + let changed = w::ConvertStringSecurityDescriptorToSecurityDescriptor("S:P(AU;SAFA;FR;;;WD)").unwrap(); + w::SetNamedSecurityInfo(path.as_os_str(), O::SE_FILE_OBJECT, flags, None, None, None, w::GetSecurityDescriptorSacl(&changed).unwrap()).unwrap(); + let changed = platform::Policy::read_existing(&path).unwrap(); + assert!(staged.publish().is_err()); + assert_eq!(fs::read(&path).unwrap(), b"complete replacement"); + assert!(changed.matches(&path).unwrap()); + } + } } diff --git a/crates/export/src/pending/unix.rs b/crates/export/src/pending/unix.rs index 1b51f0bc7..5c74bfdea 100644 --- a/crates/export/src/pending/unix.rs +++ b/crates/export/src/pending/unix.rs @@ -17,6 +17,16 @@ pub(super) struct Policy { } impl Policy { + /// Unix replacement uses the same ownership, mode, and access-ACL snapshot as creation. + pub fn read_existing(path: &Path) -> io::Result { + Self::read(path) + } + + /// Verify the restored access policy against this output's snapshot. + pub fn matches(&self, path: &Path) -> io::Result { + Ok(Self::read(path)? == *self) + } + /// Snapshot ownership, access controls, and permission state used for publication checks. pub fn read(path: &Path) -> io::Result { let metadata = fs::metadata(path)?; diff --git a/crates/export/src/pending/windows.rs b/crates/export/src/pending/windows.rs index 1f462eac7..887da04a5 100644 --- a/crates/export/src/pending/windows.rs +++ b/crates/export/src/pending/windows.rs @@ -15,12 +15,14 @@ const OBJECT: O = O::SE_FILE_OBJECT; pub(super) struct Policy { descriptor: OsString, readonly: bool, + audit: bool, } impl PartialEq for Policy { /// Compare access policy while tolerating Windows' duplicate-grant legacy conversion. fn eq(&self, other: &Self) -> bool { self.readonly == other.readonly + && self.audit == other.audit && comparison_descriptor(&self.descriptor.to_string_lossy()) == comparison_descriptor(&other.descriptor.to_string_lossy()) } } @@ -72,10 +74,35 @@ fn comparison_descriptor(descriptor: &str) -> String { impl Policy { /// Snapshot ownership, access controls, and permission state used for publication checks. pub fn read(path: &Path) -> io::Result { - let descriptor = w::GetNamedSecurityInfo(path.as_os_str(), OBJECT, S::Owner | S::Group | S::Dacl | S::Label | S::Attribute | S::Scope)?; + Self::read_with_audit(path, false) + } + + /// Replacing an inode requires its full audit policy, not only access-control metadata. + pub fn read_existing(path: &Path) -> io::Result { + Self::read_with_audit(path, true) + } + + /// Verify exactly the policy components captured for this output. + pub fn matches(&self, path: &Path) -> io::Result { + Ok(Self::read_with_audit(path, self.audit)? == *self) + } + + /// Full SACL reads require an already-enabled SeSecurityPrivilege; never silently + /// fall back to a partial snapshot or alter the application's privilege state. + fn read_with_audit(path: &Path, audit: bool) -> io::Result { + let mut flags = S::Owner | S::Group | S::Dacl | S::Label | S::Attribute | S::Scope; + if audit { + flags |= S::Sacl; + } + let descriptor = w::GetNamedSecurityInfo(path.as_os_str(), OBJECT, flags).map_err(|error| { + if audit { + io::Error::new(error.kind(), format!("cannot preserve the existing export's Windows audit policy: {error}; replacement requires enabled SeSecurityPrivilege; choose a new output filename")) + } else { error } + })?; Ok(Self { descriptor: w::ConvertSecurityDescriptorToStringSecurityDescriptor(&descriptor, S::Owner | S::Group | S::Dacl | S::Sacl)?, readonly: path.metadata()?.permissions().readonly(), + audit, }) } @@ -91,6 +118,10 @@ impl Policy { if sacl.is_some() { flags |= S::Label | S::Attribute | S::Scope; } + if self.audit { + let protected_sacl = text.split("S:").nth(1).unwrap_or("").split('(').next().unwrap_or("").contains('P'); + flags |= S::Sacl | if protected_sacl { S::ProtectedSacl } else { S::UnprotectedSacl }; + } w::SetNamedSecurityInfo( path.as_os_str(), OBJECT, diff --git a/crates/export/tests/render_settings.rs b/crates/export/tests/render_settings.rs index a4b5afd4b..1fc59d898 100644 --- a/crates/export/tests/render_settings.rs +++ b/crates/export/tests/render_settings.rs @@ -479,6 +479,7 @@ fn a_job_renders_on_one_compositor() { /// Cancellation after an encoder has opened its output must preserve the prior export. #[test] +#[cfg_attr(windows, ignore = "requires enabled SeSecurityPrivilege; exercised by the Windows audit CI runner")] fn cancelled_replacement_preserves_existing_movie_and_sequence() { let dir = std::env::temp_dir().join(format!("ec-preserve-export-{}", std::process::id())); std::fs::create_dir_all(&dir).unwrap(); @@ -515,6 +516,7 @@ fn cancelled_replacement_preserves_existing_movie_and_sequence() { /// A complete two-second export matches the sink output byte-for-byte, both for a new /// destination and a replacement. This exercises the real muxer and parallel sequence path. #[test] +#[cfg_attr(windows, ignore = "requires enabled SeSecurityPrivilege; exercised by the Windows audit CI runner")] fn published_movie_and_sequence_match_complete_sink_outputs() { let dir = tempfile::tempdir().unwrap(); let (p, cid) = project(FrameRate::new(25, 1), |_, _| {}); @@ -561,6 +563,7 @@ fn published_movie_and_sequence_match_complete_sink_outputs() { /// An I/O failure after a sequence starts must also preserve all earlier complete outputs. #[test] +#[cfg_attr(windows, ignore = "requires enabled SeSecurityPrivilege; exercised by the Windows audit CI runner")] fn sequence_write_failure_preserves_existing_frames() { let dir = tempfile::tempdir().unwrap(); let blocked = dir.path().join("blocked-overflow"); diff --git a/docs/export-output-safety.md b/docs/export-output-safety.md index 81cad8b6c..73618f647 100644 --- a/docs/export-output-safety.md +++ b/docs/export-output-safety.md @@ -28,6 +28,12 @@ by the platform adapter; errors preserve the old output instead of weakening acc duplicates of existing unconditional allow grants. Only that equivalent form is accepted; new rights, deny changes, protection changes, and SACL changes still fail policy verification. + Replacement also captures, restores, and verifies the full audit SACL and its + protection state. This requires SeSecurityPrivilege to already be enabled in + the exporting process. The application does not enable or acquire privileges. + If full audit policy cannot be inspected, replacement fails before rendering + and leaves the existing file intact; choose a new output filename instead. + New destinations do not require this privilege and retain normal inheritance. - Linux preserves numeric ownership, permission bits, and security/system xattrs, including POSIX access ACLs. A setgid directory cannot silently change the owner group of an existing output. No libacl dependency is added on Linux. @@ -70,3 +76,11 @@ Linux includes a named-user ACL and a differing-group setgid case when run as ro The export integration tests cancel after writing begins and compare successful movie/sequence output against sink bytes. The filesystem workflow runs these on Windows, Linux and macOS. Local cross-compilation alone is not runtime validation. + +Windows tests that replace existing files explicitly require enabled +SeSecurityPrivilege. An ordinary local test run marks those cases ignored rather +than pretending audit preservation was exercised. The Windows filesystem workflow +runs `windows_audit_tests`, a CI-only launcher that enables an already-granted +privilege and runs both native and integration suites with `--include-ignored`. +It also runs refusal in a separate child process with that privilege removed, +checking old bytes, absence of staging, and successful export to a new filename. From 7fabc356804e093fb5dc00f660522602dc174714 Mon Sep 17 00:00:00 2001 From: TimotheusG Date: Tue, 20 Oct 2026 00:32:30 -0400 Subject: [PATCH 6/8] G3: verify absent audit policy without materializing a null SACL --- crates/export/src/pending/windows.rs | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/crates/export/src/pending/windows.rs b/crates/export/src/pending/windows.rs index 887da04a5..92678a10b 100644 --- a/crates/export/src/pending/windows.rs +++ b/crates/export/src/pending/windows.rs @@ -84,7 +84,12 @@ impl Policy { /// Verify exactly the policy components captured for this output. pub fn matches(&self, path: &Path) -> io::Result { - Ok(Self::read_with_audit(path, self.audit)? == *self) + let actual = Self::read_with_audit(path, self.audit)?; + #[cfg(test)] + if actual != *self { + eprintln!("policy mismatch: expected {self:?}; actual {actual:?}"); + } + Ok(actual == *self) } /// Full SACL reads require an already-enabled SeSecurityPrivilege; never silently @@ -118,7 +123,10 @@ impl Policy { if sacl.is_some() { flags |= S::Label | S::Attribute | S::Scope; } - if self.audit { + // Setting a null SACL can materialize a different descriptor. Leave an absent + // SACL untouched; the full readback must still prove it matches the destination. + // Unexpected inherited audit entries therefore cause refusal, never silent loss. + if self.audit && sacl.is_some() { let protected_sacl = text.split("S:").nth(1).unwrap_or("").split('(').next().unwrap_or("").contains('P'); flags |= S::Sacl | if protected_sacl { S::ProtectedSacl } else { S::UnprotectedSacl }; } From 4cf22666e9c56982290ab46cad31024bd2b0e917 Mon Sep 17 00:00:00 2001 From: TimotheusG Date: Tue, 20 Oct 2026 17:03:20 -0400 Subject: [PATCH 7/8] G3: use library overwrite with render confirmation --- .github/workflows/export-filesystem.yml | 25 +- Cargo.lock | 43 +- apps/effectcraft-cli/src/main.rs | 15 +- apps/effectcraft-cli/tests/cli.rs | 28 + crates/automation/src/confine.rs | 2 +- crates/automation/src/tests.rs | 3 + crates/engine/src/commands/render_queue.rs | 31 +- crates/engine/src/lib.rs | 4 + crates/engine/src/offload.rs | 41 +- crates/engine/src/render_queue.rs | 296 ++++++-- crates/engine/src/rq_tests.rs | 96 +++ crates/export/Cargo.toml | 9 +- crates/export/examples/windows_audit_tests.rs | 39 -- crates/export/src/lib.rs | 22 +- crates/export/src/out.rs | 101 ++- crates/export/src/pending.rs | 129 +++- crates/export/src/pending/native.rs | 637 +++++++----------- crates/export/src/pending/unix.rs | 137 ---- crates/export/src/pending/windows.rs | 233 ------- crates/export/src/pipeline.rs | 7 +- crates/export/src/preflight.rs | 133 ++++ crates/export/tests/render_settings.rs | 262 ++++++- crates/host/Cargo.toml | 2 +- crates/host/src/lib.rs | 93 ++- crates/host/tests/overwrite_confirmation.rs | 147 ++++ crates/ui-egui/src/lib.rs | 2 + crates/ui-egui/src/menus.rs | 23 +- crates/ui-egui/src/panels/dialogs.rs | 3 + crates/ui-egui/src/panels/mod.rs | 1 + crates/ui-egui/src/panels/overwrite_render.rs | 195 ++++++ docs/control-protocol.md | 25 + docs/export-output-safety.md | 124 ++-- 32 files changed, 1829 insertions(+), 1079 deletions(-) delete mode 100644 crates/export/examples/windows_audit_tests.rs delete mode 100644 crates/export/src/pending/unix.rs delete mode 100644 crates/export/src/pending/windows.rs create mode 100644 crates/export/src/preflight.rs create mode 100644 crates/host/tests/overwrite_confirmation.rs create mode 100644 crates/ui-egui/src/panels/overwrite_render.rs diff --git a/.github/workflows/export-filesystem.yml b/.github/workflows/export-filesystem.yml index 90debf018..046757742 100644 --- a/.github/workflows/export-filesystem.yml +++ b/.github/workflows/export-filesystem.yml @@ -23,18 +23,27 @@ jobs: runs-on: ${{ matrix.os }} timeout-minutes: 30 env: + CARGO_BUILD_JOBS: "2" CARGO_PROFILE_RELEASE_LTO: "false" CARGO_PROFILE_RELEASE_CODEGEN_UNITS: "16" steps: - uses: actions/checkout@v4 - - uses: dtolnay/rust-toolchain@stable + with: + ref: ${{ github.event.pull_request.head.sha || github.sha }} + persist-credentials: false + - uses: dtolnay/rust-toolchain@1.99.1 - uses: Swatinem/rust-cache@v2 - - name: Native filesystem contract - if: runner.os != 'Windows' - run: cargo test --locked --release -p effectcraft-export --lib pending::native + - name: Record exact source and toolchain + shell: bash + run: | + git rev-parse HEAD 'HEAD^{tree}' + rustc -Vv + cargo -V + - name: Ordinary export unit and filesystem contracts + # Run the complete library suite so a retired module/filter cannot match zero tests. + run: cargo test --locked --release -p effectcraft-export --lib - name: Export integration - if: runner.os != 'Windows' run: cargo test --locked --release -p effectcraft-export --test render_settings - - name: Windows audit-safe replacement and unprivileged refusal - if: runner.os == 'Windows' - run: cargo run --locked --release -p effectcraft-export --example windows_audit_tests + - name: Ensure tests did not change tracked sources or lockfile + if: always() + run: git diff --exit-code HEAD diff --git a/Cargo.lock b/Cargo.lock index fc6a5a882..3cc3aa22e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1296,7 +1296,6 @@ dependencies = [ "effectcraft-render", "effectcraft-time", "effectcraft-vp9enc", - "exacl", "filmcraft-aac", "filmcraft-h264enc", "filmcraft-isobmff", @@ -1307,13 +1306,11 @@ dependencies = [ "image", "log", "rayon", + "serde_json", "tempfile", "thiserror 2.0.21", "web-time", "winapi-util", - "windows-permissions", - "winsafe", - "xattr", ] [[package]] @@ -1999,18 +1996,6 @@ dependencies = [ "pin-project-lite", ] -[[package]] -name = "exacl" -version = "0.13.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8de9e551ced9d700a31dacb8b168129640240fcd7a279a3fa4d59608b84ebcae" -dependencies = [ - "bitflags 2.13.2", - "log", - "scopeguard", - "uuid", -] - [[package]] name = "exr" version = "1.74.2" @@ -6223,16 +6208,6 @@ dependencies = [ "windows-link", ] -[[package]] -name = "windows-permissions" -version = "0.2.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9e2ccdc3c6bf4d4a094e031b63fadd08d8e42abd259940eb8aa5fdc09d4bf9be" -dependencies = [ - "bitflags 1.3.2", - "winapi", -] - [[package]] name = "windows-result" version = "0.1.2" @@ -6570,12 +6545,6 @@ dependencies = [ "version_check", ] -[[package]] -name = "winsafe" -version = "0.0.29" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9ef0ffc427f045c0cc9ebffd6f4f91153dcd2a1547b5c3c29ee3f541e16e95c6" - [[package]] name = "wit-bindgen" version = "0.57.1" @@ -6626,16 +6595,6 @@ version = "0.13.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ea6fc2961e4ef194dcbfe56bb845534d0dc8098940c7e5c012a258bfec6701bd" -[[package]] -name = "xattr" -version = "1.6.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "32e45ad4206f6d2479085147f02bc2ef834ac85886624a23575ae137c8aa8156" -dependencies = [ - "libc", - "rustix 1.1.5", -] - [[package]] name = "xcursor" version = "0.3.11" diff --git a/apps/effectcraft-cli/src/main.rs b/apps/effectcraft-cli/src/main.rs index c7a012c76..892378059 100644 --- a/apps/effectcraft-cli/src/main.rs +++ b/apps/effectcraft-cli/src/main.rs @@ -16,6 +16,7 @@ //! [--profile main|main10] [--level auto|4.1] [--rate-control bitrate|quality] [--video-quality 1-100] //! [--keyint FRAMES] [--webm-codec vp9|av1] [--audio-bitrate KBPS] [--opus-app audio|voice] //! (a relative --out is relative to the working directory; --queue outputs follow the project's own settings) +//! [--overwrite] replace existing outputs (only the reviewed frame range, no trailing-frame deletion) //! [--progress] report progress on stderr when it is not a terminal, up to four lines a second: //! `progress ` //! effectcraft-cli render --template T.ectemplate [--values V.json] --out FILE @@ -108,6 +109,7 @@ const USAGE: &str = "usage: effectcraft-cli ) render/render-frame --template T.ectemplate [--values V.json] render one template instance @@ -194,6 +196,7 @@ const FLAGS: &[&str] = &[ "--adv3d", "--autosave", "--progress", + "--overwrite", ]; struct Args { @@ -448,6 +451,9 @@ fn emit(v: &Value, json_out: bool) { } fn run(cmd: &str, args: &Args, json_out: bool) -> Result<(), Failure> { + if args.flag("--overwrite") && cmd != "render" { + return usage_err("--overwrite is only supported by render; engine commands require a preflight confirmation id"); + } if args.flag("--autosave") && cmd != "mcp" { return usage_err("--autosave is only supported by mcp"); } @@ -775,7 +781,14 @@ fn render(args: &Args, json_out: bool) -> Result<(), Failure> { r["outputModuleSummary"].as_str().unwrap_or("") ); } - s.execute("renderQueue.render", json!({"wait": false})).map_err(err)?; + let mut render_params = json!({"wait": false}); + if args.flag("--overwrite") { + // The explicit flag authorizes only the destinations observed in this invocation. + let review = s.execute("renderQueue.preflight", json!({})).map_err(err)?; + render_params["confirmation"] = review["confirmation"].clone(); + render_params["overwrite"] = json!(true); + } + s.execute("renderQueue.render", render_params).map_err(err)?; let tty = std::io::IsTerminal::is_terminal(&std::io::stderr()); // `--progress`: one line per update for a program reading stderr (a terminal gets the live line). let lines = args.flag("--progress") && !tty; diff --git a/apps/effectcraft-cli/tests/cli.rs b/apps/effectcraft-cli/tests/cli.rs index 1111db403..ad2b02aa5 100644 --- a/apps/effectcraft-cli/tests/cli.rs +++ b/apps/effectcraft-cli/tests/cli.rs @@ -510,6 +510,34 @@ fn a_failed_render_under_run_and_exec_exits_1() { assert!(v["error"].as_str().is_some_and(|e| e.starts_with("T: ")), "{v}"); } +#[test] +fn render_overwrite_flag_is_explicit_for_saved_queues() { + let root = tmp("overwrite-confirmation"); + std::fs::create_dir_all(&root).unwrap(); + let project = root.join("queue.ecproj"); + let output = root.join("out.gif"); + let add = json!({"format": "gif", "output": output}).to_string(); + ok_json(&[ + "run", + "--empty", + "comp.new", + r#"{"name":"T","width":4,"height":4,"frameRate":10,"duration":0.2}"#, + "renderQueue.add", + &add, + "--save-as", + project.to_str().unwrap(), + ]); + std::fs::write(&output, b"old output").unwrap(); + let args = ["render", "--project", project.to_str().unwrap(), "--queue"]; + let (code, result) = run_json(&args); + assert_eq!(code, 1, "{result}"); + assert!(result["error"].as_str().unwrap().contains("preflight")); + assert_eq!(std::fs::read(&output).unwrap(), b"old output"); + ok_json(&[args.as_slice(), &["--overwrite"]].concat()); + assert!(std::fs::read(&output).unwrap().starts_with(b"GIF89a")); + std::fs::remove_dir_all(root).unwrap(); +} + /// `render --bitrate` (#440) targets the bitrate in VP9 WebM. #[test] fn render_bitrate_applies_to_vp9_webm() { diff --git a/crates/automation/src/confine.rs b/crates/automation/src/confine.rs index f396b3da0..4d6553897 100644 --- a/crates/automation/src/confine.rs +++ b/crates/automation/src/confine.rs @@ -87,7 +87,7 @@ const WRITES: &[(&str, &[&str])] = &[ ]; /// Commands that write where they are not told: the project's file, the Render Queue's outputs. -const IMPLICIT_WRITES: &[&str] = &["file.save", "file.incrementAndSave", "renderQueue.render", "render.preRender"]; +const IMPLICIT_WRITES: &[&str] = &["file.save", "file.incrementAndSave", "renderQueue.render", "renderQueue.preflight", "render.preRender"]; /// What must be looked up before an implicitly writing command may run. #[derive(Debug, PartialEq, Eq)] diff --git a/crates/automation/src/tests.rs b/crates/automation/src/tests.rs index 6e622b377..687ed4646 100644 --- a/crates/automation/src/tests.rs +++ b/crates/automation/src/tests.rs @@ -1364,6 +1364,9 @@ fn automation_roots_confine_file_access() { assert!(queue["items"][0]["outputPath"].as_str().is_some_and(|o| o.starts_with(&*read.to_string_lossy())), "{queue}"); let msg = denied(&mut s, "execute_command", json!({"command": "renderQueue.render"})); assert!(msg.contains("cannot write"), "{msg}"); + // Reviewing destination metadata must obey the same root check before inspecting it. + let msg = denied(&mut s, "execute_command", json!({"command": "renderQueue.preflight"})); + assert!(msg.contains("cannot write"), "{msg}"); call_json(&mut s, "open_project", json!({"path": p(&write, "saved.ecproj")})); call_json(&mut s, "save_project", json!({})); // Through a symbolic link out of a root. diff --git a/crates/engine/src/commands/render_queue.rs b/crates/engine/src/commands/render_queue.rs index 09a7c7d2f..d555af35f 100644 --- a/crates/engine/src/commands/render_queue.rs +++ b/crates/engine/src/commands/render_queue.rs @@ -670,6 +670,14 @@ fn duplicate(s: &mut Session, p: &Value) -> Result { fn render(s: &mut Session, p: &Value) -> Result { let wait = b_p(p, "wait").unwrap_or(true); + let confirmation = match p.get("confirmation") { + None => None, + Some(v) => Some(v.as_u64().ok_or_else(|| bad("renderQueue.render", "confirmation must be a preflight id"))?), + }; + let overwrite = match p.get("overwrite") { + None => false, + Some(v) => v.as_bool().ok_or_else(|| bad("renderQueue.render", "overwrite must be a boolean"))?, + }; // Settings ▸ Project ▸ Auto-Save ▸ Save When Starting Render Queue. if s.prefs.auto_save.enabled && s.prefs.auto_save.save_on_render_start @@ -678,11 +686,20 @@ fn render(s: &mut Session, p: &Value) -> Result { { log::warn!("{e}"); } - let ids = s.start_render(wait).map_err(EngineError::Other)?; + let ids = s.start_render_reviewed(wait, confirmation, overwrite).map_err(EngineError::Other)?; let items: Vec = s.project.render_queue.iter().enumerate().filter(|(_, i)| ids.contains(&i.id)).map(|(k, i)| item_json(s, i, k)).collect(); Ok(json!({"rendering": s.is_rendering(), "items": items})) } +fn preflight(s: &mut Session, _: &Value) -> Result { + s.preflight_render().map_err(EngineError::Other) +} + +fn cancel_preflight(s: &mut Session, p: &Value) -> Result { + let id = p.get("confirmation").and_then(Value::as_u64).ok_or_else(|| bad("renderQueue.cancelPreflight", "confirmation must be a preflight id"))?; + Ok(json!({"cancelled": s.cancel_render_preflight(id)})) +} + fn stop(s: &mut Session, _: &Value) -> Result { Ok(json!({"stopped": s.stop_render()})) } @@ -1045,7 +1062,17 @@ pub fn specs() -> Vec { cmd!("render.saveCurrentPreview", "Save Current Preview...", ["Composition"], None, "{comp?, path}", has_comp, save_current_preview), cmd!("renderQueue.move", "Move in Render Queue", [], None, "{item?|index?, to: n (1-based)}", has_items, move_item), cmd!("renderQueue.duplicate", "Duplicate Render Item", [], None, "{item?|index?}", has_items, duplicate), - cmd!("renderQueue.render", "Render", [], None, "{wait?: bool (default true; the UI renders in the background)}", has_queued, render), + query!("renderQueue.preflight", "Review Render Outputs", "{} → {confirmation, collisions: [path]}", preflight), + query!("renderQueue.cancelPreflight", "Cancel Render Review", "{confirmation}", cancel_preflight), + cmd!( + "renderQueue.render", + "Render", + [], + None, + "{wait?: bool (default true), confirmation?: preflight id, overwrite?: bool (default false; requires the reviewed confirmation)}", + has_queued, + render + ), cmd!("renderQueue.stop", "Stop Rendering", [], None, "{}", rendering, stop), query!("renderQueue.list", "Render Queue Items", "{}", list), query!("renderQueue.templates", "Render Templates", "{}", templates), diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index cdfc7468e..ae17e1c89 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -422,6 +422,9 @@ pub struct Session { pub storage_quota: Option>, /// The running (or finished, not yet polled) render. pub render_job: Option, + /// Pending one-shot output review (managed by the render queue). + #[doc(hidden)] + pub render_review: Option, /// The running (or finished, not yet polled) track analysis. pub track_job: Option, /// The running (or finished, not yet polled) mask track. @@ -574,6 +577,7 @@ impl Default for Session { exporter: None, storage_quota: None, render_job: None, + render_review: None, track_job: None, mask_job: None, warp_job: None, diff --git a/crates/engine/src/offload.rs b/crates/engine/src/offload.rs index ad2c0cde0..a47cda61e 100644 --- a/crates/engine/src/offload.rs +++ b/crates/engine/src/offload.rs @@ -68,6 +68,8 @@ pub enum WorkerJob { /// output module. Render { items: Vec>, + #[serde(default)] + authorizations: Vec>, }, Warp { comp: ItemId, @@ -472,7 +474,7 @@ async fn run_job(s: &mut Session, job: WorkerJob, post: Post) -> Result<(), Stri let kind = job.kind(); let target = job.target(); match job { - WorkerJob::Render { items } => { + WorkerJob::Render { items, authorizations } => { let exporter = s.exporter.clone().ok_or("export is not available in this build")?; let shared = JobShared::default(); // Item updates and progress go out as they happen (progress at most every 100 ms). @@ -487,7 +489,7 @@ async fn run_job(s: &mut Session, job: WorkerJob, post: Post) -> Result<(), Stri post(WorkerReply::Render { state: sh.snapshot() }); } }; - crate::render_queue::run_items(s, exporter, items, &shared, &mut |sh| notify(sh, false)).await; + crate::render_queue::run_items(s, exporter, items, authorizations, &shared, &mut |sh| notify(sh, false)).await; notify(&shared, true); Ok(()) } @@ -1074,7 +1076,7 @@ mod tests { s.execute("renderQueue.add", serde_json::json!({})).unwrap(); let item = s.project.render_queue[0].clone(); let reqs = [ - WorkerJob::Render { items: vec![vec![(item.clone(), "/out.gif".into())]] }, + WorkerJob::Render { items: vec![vec![(item.clone(), "/out.gif".into())]], authorizations: vec![] }, WorkerJob::Warp { comp, layer: LayerId(3), effect: 9 }, WorkerJob::WarpPlan { comp, layer: LayerId(3), effect: 9, time: Tick::from_seconds_f64(0.5) }, WorkerJob::RotoFreeze { comp, layer: LayerId(3), effect: 9 }, @@ -1168,6 +1170,39 @@ mod tests { } } + #[test] + fn legacy_worker_render_uses_the_exporters_safe_default() { + struct Legacy(Arc>); + impl crate::Exporter for Legacy { + fn formats(&self) -> Vec { + vec![effectcraft_project::render_queue::OutputFormat::Gif] + } + fn export(&self, _: &crate::ExportJob, _: &mut dyn FnMut(u64, u64) -> bool) -> Result { + *self.0.lock().unwrap() += 1; + Ok(crate::ExportResult::default()) + } + fn export_authorized_async<'a>( + &'a self, + _: &'a crate::ExportJob<'a>, + _: &'a serde_json::Value, + _: &'a mut dyn FnMut(u64, u64) -> bool, + ) -> crate::render_queue::LocalFuture<'a, Result> { + Box::pin(async { Err("a legacy request has no authorization".into()) }) + } + } + let calls = Arc::new(Mutex::new(0)); + let mut s = Session { exporter: Some(Arc::new(Legacy(calls.clone()))), ..Default::default() }; + s.execute("comp.new", serde_json::json!({"width": 4, "height": 4, "duration": 0.2})).unwrap(); + s.execute("renderQueue.add", serde_json::json!({"format": "gif", "output": "legacy.gif"})).unwrap(); + let item = s.project.render_queue[0].clone(); + // The old wire format has no authorizations field. + let job: WorkerJob = serde_json::from_value(serde_json::json!({"kind": "render", "items": [[[item, "legacy.gif"]]]})).unwrap(); + let req = WorkerRequest { id: 1, project: s.project.to_json(), files: vec![], job }; + let post: Post = Rc::new(|_| {}); + run_request(&mut s, req, &post); + assert_eq!(*calls.lock().unwrap(), 1); + } + /// An [`Offload`] that runs the job right away in a second session, through JSON (what a /// Web Worker does, minus the threads). struct Inline; diff --git a/crates/engine/src/render_queue.rs b/crates/engine/src/render_queue.rs index 74b333151..e47385841 100644 --- a/crates/engine/src/render_queue.rs +++ b/crates/engine/src/render_queue.rs @@ -53,12 +53,27 @@ pub struct ExportResult { pub overflow: Vec, } +/// The export layer resolves all frame names and captures the existing destinations here. +/// `authorization` is opaque to the engine and must be revalidated by the exporter before writing. +#[derive(Clone, Debug, Default, Serialize, Deserialize)] +pub struct ExportPreflight { + pub collisions: Vec, + /// Canonical possible write targets, excluding skipped frames. Used to reject queue conflicts. + #[serde(default)] + pub destinations: Vec, + pub authorization: serde_json::Value, +} + /// Encodes render queue items (implemented by the export layer). pub trait Exporter: Send + Sync { /// Formats this build can write. fn formats(&self) -> Vec; + /// Inspect this output without writing. Exporters without filesystem outputs need no review. + fn preflight(&self, _job: &ExportJob) -> Result { + Ok(ExportPreflight::default()) + } /// Render and write one item (blocking). `progress(done, total)` returns `false` to cancel; - /// a cancelled export removes the files it opened for that output and returns `Err("cancelled")`. + /// A cancelled export removes its job-owned staging, preserves existing destinations, and returns `Err("cancelled")`. /// Completed earlier queue items and unrelated files must be preserved. fn export(&self, job: &ExportJob, progress: &mut dyn FnMut(u64, u64) -> bool) -> Result; /// [`Exporter::export`] as a future: renders whose GPU readbacks are deferred (a browser @@ -67,6 +82,40 @@ pub trait Exporter: Send + Sync { fn export_async<'a>(&'a self, job: &'a ExportJob<'a>, progress: &'a mut dyn FnMut(u64, u64) -> bool) -> LocalFuture<'a, Result> { Box::pin(async move { self.export(job, progress) }) } + /// Export with the exact destination snapshot accepted by the caller. Implementations must + /// reject newly appeared or changed destinations rather than treating this as blanket consent. + fn export_authorized( + &self, + job: &ExportJob, + _authorization: &serde_json::Value, + progress: &mut dyn FnMut(u64, u64) -> bool, + ) -> Result { + self.export(job, progress) + } + /// Authorized counterpart to `export_async`. The default preserves existing async exporters; + /// filesystem exporters that implement authorization override this as well. + fn export_authorized_async<'a>( + &'a self, + job: &'a ExportJob<'a>, + _authorization: &'a serde_json::Value, + progress: &'a mut dyn FnMut(u64, u64) -> bool, + ) -> LocalFuture<'a, Result> { + self.export_async(job, progress) + } +} + +type RenderItems = Vec>; + +/// A single session-local review. It is consumed on render or cancellation, never persisted. +#[doc(hidden)] +pub struct RenderReview { + id: u64, + project: Arc, + exporter: Arc, + items: RenderItems, + authorizations: Vec>, + collisions: Vec, + nested_switches: bool, } /// A boxed future that stays on its thread. @@ -153,6 +202,7 @@ struct Work { /// Per queue item: one (item with that output module, resolved path) per output module /// (and per segment, see [`Session::segments`]). items: Vec>, + authorizations: Vec>, /// Settings ▸ General ▸ Switches Affect Nested Comps. nested_switches: bool, } @@ -226,6 +276,7 @@ pub(crate) async fn run_items( s: &Session, exporter: Arc, items: Vec>, + authorizations: Vec>, shared: &JobShared, notify: &mut dyn FnMut(&JobShared), ) { @@ -237,6 +288,7 @@ pub(crate) async fn run_items( exporter, storage: s.storage_quota.clone(), items, + authorizations, nested_switches: s.prefs.general.switches_affect_nested_comps, }; run_work(work, shared, notify).await; @@ -282,21 +334,23 @@ async fn run_work(w: Work, shared: &JobShared, notify: &mut dyn FnMut(&JobShared label: format!("#{index} {name} (output module {})", mi + 1), nested_switches: w.nested_switches, }; - let rr = w - .exporter - .export_async(&job, &mut |done, total| { - { - let mut s = lock(&shared.state); - s.done = done; - s.total = total; - s.elapsed = t0.elapsed().as_secs_f64(); - s.item_elapsed = t1.elapsed().as_secs_f64(); - s.remaining = (done > 0).then(|| s.item_elapsed / done as f64 * total.saturating_sub(done) as f64); - } - notify(shared); - !shared.cancel.load(Ordering::Relaxed) - }) - .await; + let mut progress = |done, total: u64| { + { + let mut s = lock(&shared.state); + s.done = done; + s.total = total; + s.elapsed = t0.elapsed().as_secs_f64(); + s.item_elapsed = t1.elapsed().as_secs_f64(); + s.remaining = (done > 0).then(|| s.item_elapsed / done as f64 * total.saturating_sub(done) as f64); + } + notify(shared); + !shared.cancel.load(Ordering::Relaxed) + }; + let rr = match w.authorizations.get(k).and_then(|m| m.get(mi)) { + Some(token) => w.exporter.export_authorized_async(&job, token, &mut progress).await, + // Older worker requests have no review: retain the exporter's safe default. + None => w.exporter.export_async(&job, &mut progress).await, + }; match rr { Ok(res) if mi == 0 => r = Ok(res), Ok(_) => {} @@ -387,10 +441,7 @@ impl Session { pub fn segments(&self, item: &RenderQueueItem, path: &str) -> Vec<(RenderQueueItem, String)> { let Some(comp) = self.project.comp(item.comp) else { return vec![(item.clone(), path.to_string())] }; let fmt = item.output.format; - let frames = item.settings.frame_count(comp); - let (w, h) = item.settings.output_size(comp); - let bpf = bytes_per_frame(&item.output, w, h, item.settings.rate(comp).as_f64()); - let Some(per) = self.prefs.segment_frames(frames, fmt.is_sequence(), bpf) else { return vec![(item.clone(), path.to_string())] }; + let Some((frames, per)) = self.segment_span(item, &item.output) else { return vec![(item.clone(), path.to_string())] }; let p = std::path::Path::new(path); let dir = p.parent().map(|d| d.to_path_buf()).unwrap_or_default(); let stem = p.file_stem().map(|s| s.to_string_lossy().to_string()).unwrap_or_default(); @@ -422,6 +473,14 @@ impl Session { out } + fn segment_span(&self, item: &RenderQueueItem, output: &OutputModule) -> Option<(u64, u64)> { + let comp = self.project.comp(item.comp)?; + let frames = item.settings.frame_count(comp); + let (w, h) = item.settings.output_size(comp); + let bpf = bytes_per_frame(output, w, h, item.settings.rate(comp).as_f64()); + self.prefs.segment_frames(frames, output.format.is_sequence(), bpf).map(|per| (frames, per)) + } + /// Whether a render job is running. pub fn is_rendering(&self) -> bool { self.render_job.as_ref().is_some_and(|j| !j.is_finished()) @@ -432,26 +491,27 @@ impl Session { self.render_job.as_ref().map(|j| j.shared.snapshot()) } - /// Start rendering every queued item. `wait`: block until done (otherwise a background thread - /// renders, or the [`Session::offload`] worker; call [`Session::poll_render`] regularly). - /// Returns the ids being rendered. Without threads or an offload (wasm32) the render always - /// runs to completion before this returns. - pub fn start_render(&mut self, wait: bool) -> Result, String> { - let offload = self.offload.clone().filter(|_| !wait); - let wait = wait || (cfg!(target_arch = "wasm32") && offload.is_none()); - if self.is_rendering() { - return Err("a render is already in progress".into()); - } - self.poll_render(); - let exporter = self.exporter.clone().ok_or("export is not available in this build")?; - let formats = exporter.formats(); + fn render_items(&self, formats: &[OutputFormat]) -> Result<(RenderItems, Vec<(u64, RenderStatus)>), String> { let mut items = Vec::new(); let mut needs_output = Vec::new(); + let mut planned = 0_u64; + let mut plan_bytes = 0_usize; for it in self.project.render_queue.iter().filter(|i| i.is_queued()) { + if it.extra_outputs.len().saturating_add(1) > 100_000 { + return Err("render review exceeds 100000 output modules; render fewer queue items at once".into()); + } + // Every work entry exports one module. Avoid cloning all other modules per segment. + let mut single = it.clone(); + single.extra_outputs.clear(); let mut modules = vec![]; let mut problem = None; for om in it.output_modules() { - let mut m = it.clone(); + let count = self.segment_span(it, om).map_or(1, |(frames, per)| frames.div_ceil(per.max(1))); + planned = planned + .checked_add(count) + .filter(|n| *n <= 100_000) + .ok_or("render review exceeds 100000 output segments; render fewer queue items at once")?; + let mut m = single.clone(); m.output = om.clone(); match self.resolve_output(&m) { Some(p) if formats.contains(&om.format) => { @@ -459,6 +519,19 @@ impl Session { // Movie Files. let bits = output_bits(om.format, self.project.settings.bit_depth); let p = self.prefs.output_name(&p, bits); + // Count alone does not bound cloned strings: a long output template or + // module name repeated over many segments can exhaust memory first. + let item_bytes = serde_json::to_vec(&m).map_err(|e| e.to_string())?.len(); + let count = usize::try_from(count).map_err(|_| "render segment count is too large")?; + let bytes = item_bytes + .checked_add(p.len()) + .and_then(|n| n.checked_add(std::mem::size_of::() + 64)) + .and_then(|n| n.checked_mul(count)) + .ok_or("render review output plan is too large")?; + plan_bytes = plan_bytes + .checked_add(bytes) + .filter(|n| *n <= 64 * 1024 * 1024) + .ok_or("render review output plan exceeds 64 MiB; render fewer frames or queue items at once")?; modules.extend(self.segments(&m, &p)); } Some(_) => problem = Some(RenderStatus::Failed(format!("{} export is not available", om.format.label()))), @@ -474,15 +547,142 @@ impl Session { None => items.push(modules), } } - if !needs_output.is_empty() { - let p = Arc::make_mut(&mut self.project); - for (id, st) in needs_output { - if let Some(i) = p.render_queue.iter_mut().find(|i| i.id == id) { - i.status = st; + Ok((items, needs_output)) + } + + /// Review exact destinations once for this queue. No files are created and no item starts. + pub fn preflight_render(&mut self) -> Result { + self.render_review = None; + if self.is_rendering() { + return Err("a render is already in progress".into()); + } + self.poll_render(); + let exporter = self.exporter.clone().ok_or("export is not available in this build")?; + let (items, _) = self.render_items(&exporter.formats())?; + if items.is_empty() { + return Err("nothing is queued: add a composition and tick Render".into()); + } + let mut authorizations = Vec::new(); + let mut collisions = Vec::new(); + let mut review_bytes = 0_usize; + let mut destinations = std::collections::HashSet::new(); + for modules in &items { + let mut tokens = Vec::new(); + for (item, path) in modules { + let job = ExportJob { + project: &self.project, + footage: self.footage.as_ref(), + expr: self.expr.as_deref(), + accel: self.accel.as_deref(), + item, + path, + storage: self.storage_quota.as_deref(), + label: "Render preflight".into(), + nested_switches: self.prefs.general.switches_affect_nested_comps, + }; + let review = exporter.preflight(&job).map_err(|error| { + let name = self.project.item(item.comp).map(|i| i.name.as_str()).unwrap_or("Render output"); + format!("{name}: {error}") + })?; + for destination in review.destinations { + if destinations.contains(&destination) { + return Err(format!("queued outputs use the same output destination: {destination}; choose separate output paths")); + } + if destinations.len() >= 100_000 { + return Err("render review exceeds 100000 output destinations; render fewer queue items at once".into()); + } + destinations.insert(destination); + } + let bytes = serde_json::to_vec(&review.authorization).map_err(|e| e.to_string())?.len(); + review_bytes = review_bytes + .checked_add(bytes) + .filter(|n| *n <= 64 * 1024 * 1024) + .ok_or("render review exceeds 64 MiB; render fewer queue items at once")?; + if collisions.len().saturating_add(review.collisions.len()) > 100_000 { + return Err("render review exceeds 100000 existing outputs; render fewer queue items at once".into()); } + collisions.extend(review.collisions); + tokens.push(review.authorization); } - self.bump(); + authorizations.push(tokens); } + collisions.sort(); + collisions.dedup(); + let id = crate::offload::next_id(); + let result = serde_json::json!({"confirmation": id, "collisions": collisions}); + self.render_review = Some(RenderReview { + id, + project: self.project.clone(), + exporter, + items, + authorizations, + collisions, + nested_switches: self.prefs.general.switches_affect_nested_comps, + }); + Ok(result) + } + + /// Discard a declined review, so its consent cannot be reused later. + pub fn cancel_render_preflight(&mut self, confirmation: u64) -> bool { + if self.render_review.as_ref().is_some_and(|r| r.id == confirmation) { + self.render_review = None; + true + } else { + false + } + } + + /// Start rendering every queued item. `wait`: block until done (otherwise a background thread + /// renders, or the [`Session::offload`] worker; call [`Session::poll_render`] regularly). + /// Returns the ids being rendered. Without threads or an offload (wasm32) the render always + /// runs to completion before this returns. + pub fn start_render(&mut self, wait: bool) -> Result, String> { + self.start_render_reviewed(wait, None, false) + } + + /// Start with an optional one-shot destination review. Replacement requires both the review + /// id and explicit intent; changing the project, queue or resolved paths invalidates it. + pub fn start_render_reviewed(&mut self, wait: bool, confirmation: Option, overwrite: bool) -> Result, String> { + if confirmation.is_none() { + if overwrite { + return Err("overwrite requires renderQueue.preflight and its confirmation id".into()); + } + if self.is_rendering() { + return Err("a render is already in progress".into()); + } + self.poll_render(); + let exporter = self.exporter.clone().ok_or("export is not available in this build")?; + let (_, problems) = self.render_items(&exporter.formats())?; + self.set_render_problems(problems); + self.preflight_render()?; + } + let review = self.render_review.take().ok_or("render confirmation expired; run renderQueue.preflight again")?; + if confirmation.is_some_and(|id| id != review.id) { + return Err("render confirmation expired; run renderQueue.preflight again".into()); + } + let exporter = self.exporter.clone().ok_or("export is not available in this build")?; + let (items, needs_output) = self.render_items(&exporter.formats())?; + if !Arc::ptr_eq(&self.project, &review.project) + || !Arc::ptr_eq(&exporter, &review.exporter) + || items != review.items + || self.prefs.general.switches_affect_nested_comps != review.nested_switches + { + return Err("render destinations or project changed; run renderQueue.preflight again".into()); + } + if !review.collisions.is_empty() && !overwrite { + return Err( + "render output already exists; run renderQueue.preflight, review collisions, then render with overwrite:true and its confirmation id".into() + ); + } + let authorizations = review.authorizations; + let offload = self.offload.clone().filter(|_| !wait); + let wait = wait || (cfg!(target_arch = "wasm32") && offload.is_none()); + if self.is_rendering() { + return Err("a render is already in progress".into()); + } + self.poll_render(); + let exporter = self.exporter.clone().ok_or("export is not available in this build")?; + self.set_render_problems(needs_output); if items.is_empty() { return Err("nothing is queued: add a composition (Composition ▸ Add to Render Queue) and tick Render".into()); } @@ -494,7 +694,7 @@ impl Session { id, project: self.project.to_json(), files: crate::offload::footage_files(&self.project), - job: crate::offload::WorkerJob::Render { items }, + job: crate::offload::WorkerJob::Render { items, authorizations }, }; off.start(req, inbox.clone())?; let shared = Arc::new(JobShared::default()); @@ -511,6 +711,7 @@ impl Session { exporter, storage: self.storage_quota.clone(), items, + authorizations, nested_switches: self.prefs.general.switches_affect_nested_comps, }; let shared = Arc::new(JobShared::default()); @@ -544,6 +745,19 @@ impl Session { Ok(ids) } + fn set_render_problems(&mut self, problems: Vec<(u64, RenderStatus)>) { + if problems.is_empty() { + return; + } + let p = Arc::make_mut(&mut self.project); + for (id, status) in problems { + if let Some(item) = p.render_queue.iter_mut().find(|i| i.id == id) { + item.status = status; + } + } + self.bump(); + } + /// Stop the running render (the current item becomes "User Stopped"). pub fn stop_render(&mut self) -> bool { match &self.render_job { diff --git a/crates/engine/src/rq_tests.rs b/crates/engine/src/rq_tests.rs index 81a187226..11924d9d6 100644 --- a/crates/engine/src/rq_tests.rs +++ b/crates/engine/src/rq_tests.rs @@ -229,3 +229,99 @@ fn locked_resize_uses_height_for_height_only_output_module_updates() { let comp = s.project.comp(item.comp).unwrap(); assert_eq!(item.output.output_size(comp, &item.settings), (32, 32), "explicit width retains precedence"); } + +/// The engine holds backend authorization privately; callers only receive a one-shot review id. +#[test] +fn overwrite_review_is_explicit_bound_and_consumed() { + use crate::render_queue::{ExportPreflight, LocalFuture}; + struct Reviewed { + calls: Mutex>, + collisions: Vec, + } + impl Exporter for Reviewed { + fn formats(&self) -> Vec { + vec![OutputFormat::Gif] + } + fn preflight(&self, job: &ExportJob) -> Result { + Ok(ExportPreflight { collisions: self.collisions.clone(), destinations: vec![job.path.to_string()], authorization: json!({"snapshot": job.path}) }) + } + fn export(&self, _: &ExportJob, _: &mut dyn FnMut(u64, u64) -> bool) -> Result { + Err("unreviewed export must not run".into()) + } + fn export_authorized_async<'a>( + &'a self, + job: &'a ExportJob<'a>, + token: &'a serde_json::Value, + _: &'a mut dyn FnMut(u64, u64) -> bool, + ) -> LocalFuture<'a, Result> { + Box::pin(async move { + assert_eq!(token["snapshot"], job.path); + self.calls.lock().unwrap().push(token.clone()); + Ok(ExportResult { path: job.path.into(), ..Default::default() }) + }) + } + } + for existing in [false, true] { + let ex = Arc::new(Reviewed { calls: Mutex::default(), collisions: if existing { vec!["old.gif".into()] } else { vec![] } }); + let mut s = Session { exporter: Some(ex.clone()), ..Default::default() }; + s.execute("comp.new", json!({"width": 4, "height": 4, "duration": 0.2})).unwrap(); + s.execute("renderQueue.add", json!({"format": "gif", "output": crate::tests_stubs::tmp_out("review.gif")})).unwrap(); + if existing { + assert!(s.start_render(true).unwrap_err().contains("already exists")); + assert!(s.execute_checked("renderQueue.render", json!({"overwrite": true})).is_err()); + assert!(ex.calls.lock().unwrap().is_empty()); + } + let review = s.execute_checked("renderQueue.preflight", json!({})).unwrap(); + assert_eq!(review["collisions"].as_array().unwrap().len(), usize::from(existing)); + assert!(review.get("authorization").is_none()); + s.execute_checked("renderQueue.cancelPreflight", json!({"confirmation": review["confirmation"]})).unwrap(); + assert!(s.execute("renderQueue.render", json!({"confirmation": review["confirmation"], "overwrite": true})).is_err()); + assert!(ex.calls.lock().unwrap().is_empty(), "Cancel never calls the exporter"); + let review = s.execute("renderQueue.preflight", json!({})).unwrap(); + s.execute("renderQueue.setOutput", json!({"path": crate::tests_stubs::tmp_out("changed.gif")})).unwrap(); + assert!( + s.execute("renderQueue.render", json!({"confirmation": review["confirmation"], "overwrite": true})).unwrap_err().to_string().contains("changed") + ); + assert!(ex.calls.lock().unwrap().is_empty()); + let review = s.execute_checked("renderQueue.preflight", json!({})).unwrap(); + s.execute_checked("renderQueue.render", json!({"confirmation": review["confirmation"], "overwrite": existing})).unwrap(); + assert_eq!(ex.calls.lock().unwrap().len(), 1); + s.execute("renderQueue.setRender", json!({"render": true})).unwrap(); + assert!(s.execute("renderQueue.render", json!({"confirmation": review["confirmation"], "overwrite": true})).is_err()); + assert_eq!(ex.calls.lock().unwrap().len(), 1, "consent cannot carry over to a stored queue rerender"); + s.execute("renderQueue.duplicate", json!({})).unwrap(); + assert!(s.execute("renderQueue.preflight", json!({})).unwrap_err().to_string().contains("same output destination")); + assert_eq!(ex.calls.lock().unwrap().len(), 1, "overlapping queue outputs are rejected before rendering"); + } +} + +#[test] +fn overwrite_preflight_bounds_segments_before_materializing_them() { + let mut s = Session::default(); + let ex = Arc::new(MockExporter { log: Mutex::default() }); + s.exporter = Some(ex.clone()); + s.execute("comp.new", json!({"width": 4, "height": 4, "frameRate": 60, "duration": 86400})).unwrap(); + s.execute("renderQueue.add", json!({"format": "png", "output": "long_[#####].png"})).unwrap(); + s.prefs.export.segment_sequences = true; + s.prefs.export.segment_sequence_files = 1; + let error = s.preflight_render().unwrap_err(); + assert!(error.contains("100000 output segments"), "{error}"); + assert!(s.render_review.is_none()); + assert!(ex.log.lock().unwrap().is_empty()); +} + +#[test] +fn overwrite_preflight_bounds_repeated_path_bytes_before_segment_expansion() { + let mut s = Session::default(); + let ex = Arc::new(MockExporter { log: Mutex::default() }); + s.exporter = Some(ex.clone()); + s.execute("comp.new", json!({"width": 4, "height": 4, "frameRate": 10, "duration": 10})).unwrap(); + let path = format!("{}_[#####].png", "x".repeat(1_000_000)); + s.execute("renderQueue.add", json!({"format": "png", "output": path})).unwrap(); + s.prefs.export.segment_sequences = true; + s.prefs.export.segment_sequence_files = 1; + let error = s.preflight_render().unwrap_err(); + assert!(error.contains("output plan exceeds 64 MiB"), "{error}"); + assert!(s.render_review.is_none()); + assert!(ex.log.lock().unwrap().is_empty()); +} diff --git a/crates/export/Cargo.toml b/crates/export/Cargo.toml index 096f39a7d..9f2dcf125 100644 --- a/crates/export/Cargo.toml +++ b/crates/export/Cargo.toml @@ -28,6 +28,7 @@ rayon = { workspace = true } web-time = { workspace = true } thiserror = { workspace = true } log = { workspace = true } +serde_json = { workspace = true } [dev-dependencies] effectcraft-media = { workspace = true } @@ -41,12 +42,4 @@ effectcraft-keyframe = { workspace = true } tempfile = "3.27" [target.'cfg(windows)'.dependencies] -winsafe = { version = "0.0.29", features = ["advapi", "kernel"] } -windows-permissions = "0.2.4" winapi-util = "0.1.11" - -[target.'cfg(unix)'.dependencies] -xattr = "1.6" - -[target.'cfg(any(target_os = "macos", target_os = "freebsd"))'.dependencies] -exacl = "0.13" diff --git a/crates/export/examples/windows_audit_tests.rs b/crates/export/examples/windows_audit_tests.rs deleted file mode 100644 index 5a0c9da21..000000000 --- a/crates/export/examples/windows_audit_tests.rs +++ /dev/null @@ -1,39 +0,0 @@ -//! CI-only launcher for tests requiring Windows audit-policy privileges. -//! Enables a privilege already granted to this process; it cannot grant one to an -//! ordinary account. Children inherit it. The application never performs this step. - -/// Run the complete Windows filesystem and export contract with audit access enabled. -#[cfg(windows)] -fn main() -> Result<(), Box> { - use windows_permissions::{ - constants::{SeObjectType, SecurityInformation}, - wrappers, - }; - use winsafe::{DisabPriv, HPROCESS, LUID_AND_ATTRIBUTES, TOKEN_PRIVILEGES, co}; - - let token = HPROCESS::GetCurrentProcess().OpenProcessToken(co::TOKEN::ADJUST_PRIVILEGES | co::TOKEN::QUERY)?; - let luid = winsafe::LookupPrivilegeValue(None, co::SE_PRIV::SECURITY_NAME)?; - let privileges = TOKEN_PRIVILEGES::new(&[LUID_AND_ATTRIBUTES::new(luid, co::SE_PRIV_ATTR::ENABLED)])?; - token.AdjustTokenPrivileges(DisabPriv::Privs(&privileges))?; - // AdjustTokenPrivileges can succeed without assigning every requested privilege. - // Prove full SACL access before claiming that privileged tests can run. - let probe = tempfile::NamedTempFile::new()?; - wrappers::GetNamedSecurityInfo(probe.path().as_os_str(), SeObjectType::SE_FILE_OBJECT, SecurityInformation::Sacl)?; - probe.close()?; - - for selection in [vec!["--lib", "pending::native"], vec!["--test", "render_settings"]] { - let status = std::process::Command::new(env!("CARGO")) - .args(["test", "--locked", "--release", "-p", "effectcraft-export"]) - .args(selection) - .args(["--", "--include-ignored"]) - .status()?; - if !status.success() { - return Err(std::io::Error::other("Windows audit export tests failed").into()); - } - } - Ok(()) -} - -/// Audit privileges are Windows-specific; other platforms use the ordinary test commands. -#[cfg(not(windows))] -fn main() {} diff --git a/crates/export/src/lib.rs b/crates/export/src/lib.rs index da1358557..715365d67 100644 --- a/crates/export/src/lib.rs +++ b/crates/export/src/lib.rs @@ -37,6 +37,7 @@ mod hevc_av1; mod out; mod pending; mod pipeline; +mod preflight; mod uncompressed; mod webm; mod webm_av1; @@ -52,6 +53,7 @@ pub use effectcraft_project::render_queue; pub use effectcraft_project::render_queue::StorageQuota; pub use out::Sink; pub(crate) use pipeline::Cx; +pub use preflight::{OutputPreflight, preflight}; #[derive(Debug, thiserror::Error)] pub enum ExportError { @@ -168,13 +170,31 @@ pub fn export(job: &Job, progress: &mut dyn FnMut(&Progress) -> bool) -> Result< effectcraft_render::passes::block_on(export_async(job, progress)) } +/// Export only the destination versions reviewed by [`preflight`]. The host keeps the +/// returned authorization until the user accepts the grouped overwrite confirmation. +pub fn export_authorized(job: &Job, authorization: &serde_json::Value, progress: &mut dyn FnMut(&Progress) -> bool) -> Result { + effectcraft_render::passes::block_on(export_authorized_async(job, authorization, progress)) +} + /// [`export`] as a future: with an accelerator whose readbacks are deferred (WebGPU in a /// browser worker) every frame renders in passes, awaiting the device in between /// ([`effectcraft_render::passes`]); otherwise it never waits and frames render in parallel /// batches as in [`export`]. pub async fn export_async(job: &Job<'_>, progress: &mut dyn FnMut(&Progress) -> bool) -> Result { + export_inner(job, None, progress).await +} + +/// Asynchronous version of [`export_authorized`]. A boolean overwrite flag alone is +/// insufficient: the authorization must contain the destinations captured by preflight. +pub async fn export_authorized_async(job: &Job<'_>, authorization: &serde_json::Value, progress: &mut dyn FnMut(&Progress) -> bool) -> Result { + export_inner(job, Some(authorization), progress).await +} + +async fn export_inner(job: &Job<'_>, authorization: Option<&serde_json::Value>, progress: &mut dyn FnMut(&Progress) -> bool) -> Result { + preflight::validate(job, authorization)?; let t0 = Instant::now(); - let cx = Cx::new(job); + let mut cx = Cx::new(job); + cx.authorization = authorization; let mut r = export_with(&cx, progress, t0).await; if r.is_ok() && let Err(error) = cx.pending.publish() diff --git a/crates/export/src/out.rs b/crates/export/src/out.rs index c51049463..4b77cce7d 100644 --- a/crates/export/src/out.rs +++ b/crates/export/src/out.rs @@ -23,11 +23,38 @@ pub(crate) fn create<'a>(sink: Option<&'a Sink>, path: &str) -> Result> if let Some(dir) = std::path::Path::new(path).parent().filter(|d| !d.as_os_str().is_empty()) { std::fs::create_dir_all(dir).map_err(io)?; } - Ok(Out::File(BufWriter::new(std::fs::File::create(path).map_err(io)?))) + Ok(Out::File(BufWriter::new(std::fs::OpenOptions::new().write(true).create_new(true).open(path).map_err(io)?))) } } } +/// Logs never replace a prior pathname. In particular, an existing log symlink or +/// hard link is not permission to modify its unrelated target. Sink delivery keeps +/// the requested callback name; native collisions receive a uniquely created sibling. +pub(crate) fn create_log<'a>(sink: Option<&'a Sink>, path: &str) -> Result<(Out<'a>, String)> { + if sink.is_some() { + return Ok((create(sink, path)?, path.to_string())); + } + #[cfg(not(target_arch = "wasm32"))] + { + let requested = std::path::Path::new(path); + let parent = requested.parent().filter(|path| !path.as_os_str().is_empty()).unwrap_or_else(|| std::path::Path::new(".")); + std::fs::create_dir_all(parent).map_err(io)?; + match std::fs::OpenOptions::new().write(true).create_new(true).open(requested) { + Ok(file) => return Ok((Out::File(BufWriter::new(file)), path.to_string())), + Err(error) if error.kind() == std::io::ErrorKind::AlreadyExists || std::fs::symlink_metadata(requested).is_ok() => {} + Err(error) => return Err(io(error)), + } + let stem = requested.file_stem().and_then(|name| name.to_str()).unwrap_or("RenderLog"); + let log = tempfile::Builder::new().prefix(&format!("{stem}-")).suffix(".txt").tempfile_in(parent).map_err(io)?; + let (file, actual) = log.keep().map_err(io)?; + let actual = actual.to_str().ok_or_else(|| io("non-Unicode render log path"))?.to_string(); + Ok((Out::File(BufWriter::new(file)), actual)) + } + #[cfg(target_arch = "wasm32")] + Err(ExportError::Unsupported("web render logs require an output sink".into())) +} + /// Whether `path` already exists (never for a sink: it has no memory of earlier jobs). pub(crate) fn exists(sink: Option<&Sink>, path: &str) -> bool { sink.is_none() && std::path::Path::new(path).exists() @@ -75,3 +102,75 @@ impl Seek for Out<'_> { } } } + +#[cfg(test)] +#[cfg(not(target_arch = "wasm32"))] +mod tests { + use super::*; + use std::fs; + + #[test] + fn log_creation_preserves_existing_files_directories_and_link_targets() { + let kinds = if cfg!(unix) { vec!["file", "hardlink", "directory", "symlink", "dangling"] } else { vec!["file", "hardlink", "directory"] }; + for kind in kinds { + let dir = tempfile::tempdir().unwrap(); + let requested = dir.path().join("Movie_RenderLog.txt"); + let unrelated = dir.path().join("unrelated.ecproj"); + let missing = dir.path().join("missing.ecproj"); + fs::write(&unrelated, b"unrelated project").unwrap(); + match kind { + "file" => fs::write(&requested, b"previous log").unwrap(), + "hardlink" => fs::hard_link(&unrelated, &requested).unwrap(), + "directory" => fs::create_dir(&requested).unwrap(), + #[cfg(unix)] + "symlink" => std::os::unix::fs::symlink(&unrelated, &requested).unwrap(), + #[cfg(unix)] + "dangling" => std::os::unix::fs::symlink(&missing, &requested).unwrap(), + _ => panic!("unknown test fixture"), + } + let original = fs::read(&requested).ok(); + let (mut log, actual) = create_log(None, requested.to_str().unwrap()).unwrap(); + log.write_all(b"new complete log").unwrap(); + log.finish().unwrap(); + assert_ne!(std::path::Path::new(&actual), requested); + assert_eq!(fs::read(actual).unwrap(), b"new complete log"); + assert_eq!(fs::read(&requested).ok(), original); + assert_eq!(fs::read(unrelated).unwrap(), b"unrelated project"); + assert!(!missing.exists()); + #[cfg(unix)] + if kind == "symlink" || kind == "dangling" { + assert!(fs::symlink_metadata(&requested).unwrap().file_type().is_symlink()); + } + } + } + + #[test] + fn simultaneous_logs_get_distinct_paths_and_keep_their_own_bytes() { + let dir = tempfile::tempdir().unwrap(); + let requested = dir.path().join("Movie_RenderLog.txt"); + let barrier = std::sync::Barrier::new(8); + let logs = std::thread::scope(|scope| { + let threads: Vec<_> = (0..8) + .map(|job| { + let requested = &requested; + let barrier = &barrier; + scope.spawn(move || { + barrier.wait(); + let (mut log, actual) = create_log(None, requested.to_str().unwrap()).unwrap(); + let bytes = format!("complete log from job {job}").into_bytes(); + log.write_all(&bytes).unwrap(); + log.finish().unwrap(); + (actual, bytes) + }) + }) + .collect(); + threads.into_iter().map(|thread| thread.join().unwrap()).collect::>() + }); + let mut paths = std::collections::HashSet::new(); + for (path, bytes) in logs { + assert_eq!(fs::read(&path).unwrap(), bytes); + assert!(paths.insert(path)); + } + assert_eq!(paths.len(), 8); + } +} diff --git a/crates/export/src/pending.rs b/crates/export/src/pending.rs index ba5eb69fe..a28ea7f5d 100644 --- a/crates/export/src/pending.rs +++ b/crates/export/src/pending.rs @@ -13,14 +13,22 @@ pub(crate) struct Pending { impl Pending { /// Open a sink directly or register a unique, private native staging file for this job. + #[cfg(test)] pub fn create<'a>(&self, sink: Option<&'a out::Sink>, path: &str) -> Result> { + self.create_authorized(sink, path, None) + } + + /// Bind native staging to the destinations reviewed by the host's confirmation. + pub fn create_authorized<'a>(&self, sink: Option<&'a out::Sink>, path: &str, _authorization: Option<&serde_json::Value>) -> Result> { if sink.is_some() { return out::create(sink, path); } #[cfg(not(target_arch = "wasm32"))] { let mut files = self.files.lock().unwrap_or_else(std::sync::PoisonError::into_inner); + native::validate_approval(path, _authorization).map_err(crate::io)?; let (staged, file) = native::Staged::create(path).map_err(crate::io)?; + native::validate_approval(path, _authorization).map_err(crate::io)?; if files.iter().any(|other| other.path == staged.path) { return Err(crate::io(format!("duplicate export destination: {path}"))); } @@ -35,14 +43,48 @@ impl Pending { /// previously published frames remain if a later publication fails. pub fn publish(&self) -> Result<()> { #[cfg(not(target_arch = "wasm32"))] - { - let mut files = self.files.lock().unwrap_or_else(std::sync::PoisonError::into_inner); - // Validate all destinations before publishing any of them. - for staged in files.iter() { - staged.validate().map_err(crate::io)?; + self.publish_native(native::Staged::publish_prepared)?; + Ok(()) + } + + /// Keep preparation, final validation, publication, and cleanup in one job-level + /// critical section. The operation argument also permits deterministic I/O failures + /// in regression tests without changing filesystem permissions or timing threads. + #[cfg(not(target_arch = "wasm32"))] + fn publish_native(&self, mut publish: impl FnMut(&mut native::Staged) -> std::io::Result<()>) -> Result<()> { + let mut files = self.files.lock().unwrap_or_else(std::sync::PoisonError::into_inner); + let _publication = native::PUBLICATION.lock().unwrap_or_else(std::sync::PoisonError::into_inner); + // Check completed staging files before changing any final pathname. + for staged in files.iter() { + staged.prepare().map_err(|error| crate::io(format!("export preparation failed for {} before publication: {error}", staged.path.display())))?; + } + for staged in files.iter() { + staged.validate().map_err(crate::io)?; + } + let total = files.len(); + let mut published = 0_usize; + while let Some(mut staged) = files.pop() { + let result = publish(&mut staged); + if result.is_ok() { + published = published.saturating_add(1); } - while let Some(staged) = files.pop() { - staged.publish().map_err(crate::io)?; + let path = staged.path.clone(); + let cleanup = staged.discard(); + let error = match (result, cleanup) { + (Err(error), Err(cleanup)) => Some(format!("{error}; cleanup also failed: {cleanup}")), + (Err(error), _) | (_, Err(error)) => Some(error.to_string()), + _ => None, + }; + if let Some(error) = error { + let state = if published == total { + "all output files were published; staging cleanup failed" + } else { + "already published files remain and the output set may be incomplete or contain mixed generations" + }; + return Err(crate::io(format!( + "export publication/cleanup failed at {}: {error}; {published} of {total} files published; {state}", + path.display() + ))); } } Ok(()) @@ -66,3 +108,76 @@ impl Pending { Ok(()) } } + +/// Inspect a destination without creating it or its missing parent directories. +#[cfg(not(target_arch = "wasm32"))] +pub(crate) fn inspect(path: &str) -> std::io::Result<(String, serde_json::Value, bool)> { + native::inspect(path) +} + +#[cfg(not(target_arch = "wasm32"))] +pub(crate) fn validate_approval(path: &str, authorization: Option<&serde_json::Value>) -> std::io::Result<()> { + native::validate_approval(path, authorization) +} + +#[cfg(test)] +#[cfg(not(target_arch = "wasm32"))] +mod tests { + use super::*; + use std::{fs, io::Write, path::Path}; + + fn stage(pending: &Pending, path: &Path, bytes: &[u8]) { + let mut writer = pending.create(None, path.to_str().unwrap()).unwrap(); + writer.write_all(bytes).unwrap(); + writer.flush().unwrap(); + } + + #[test] + fn preparation_failure_publishes_no_frames() { + let dir = tempfile::tempdir().unwrap(); + let a = dir.path().join("frame_0001.png"); + let b = dir.path().join("frame_0002.png"); + let pending = Pending::default(); + stage(&pending, &a, b"new 1"); + // Remove precisely the first job-owned staging entry in this empty fixture. + let mut staging = fs::read_dir(dir.path()).unwrap().next().unwrap().unwrap().path(); + if staging.is_dir() { + staging = fs::read_dir(staging).unwrap().next().unwrap().unwrap().path(); + } + stage(&pending, &b, b"new 2"); + fs::remove_file(staging).unwrap(); + assert!(pending.publish().is_err()); + assert!(!a.exists()); + assert!(!b.exists()); + pending.discard().unwrap(); + assert_eq!(fs::read_dir(dir.path()).unwrap().count(), 0); + } + + #[test] + fn partial_publication_reports_count_and_retains_complete_files() { + let dir = tempfile::tempdir().unwrap(); + let a = dir.path().join("frame_0001.png"); + let b = dir.path().join("frame_0002.png"); + let pending = Pending::default(); + stage(&pending, &a, b"new 1"); + stage(&pending, &b, b"new 2"); + let mut calls = 0; + let error = pending + .publish_native(|staged| { + calls += 1; + if calls == 2 { + return Err(std::io::Error::other("injected publication failure")); + } + staged.publish_prepared() + }) + .unwrap_err() + .to_string(); + assert!(error.contains("1 of 2 files published"), "{error}"); + assert!(error.contains("mixed generations"), "{error}"); + assert!(error.contains(&a.display().to_string()), "{error}"); + assert!(!a.exists()); + assert_eq!(fs::read(&b).unwrap(), b"new 2"); + pending.discard().unwrap(); + assert_eq!(fs::read_dir(dir.path()).unwrap().count(), 1); + } +} diff --git a/crates/export/src/pending/native.rs b/crates/export/src/pending/native.rs index c2ab09450..64b5a4630 100644 --- a/crates/export/src/pending/native.rs +++ b/crates/export/src/pending/native.rs @@ -1,173 +1,199 @@ use std::{ - fs::{self, File, OpenOptions}, + fs::{self, File, Metadata}, io, path::{Path, PathBuf}, time::SystemTime, }; -#[cfg(unix)] -#[path = "unix.rs"] -mod platform; - -// Serialize validation and replacement across jobs in this process. External writers still -// require cooperation: portable filesystems do not provide compare-and-swap by file version. -static PUBLICATION: std::sync::Mutex<()> = std::sync::Mutex::new(()); -#[cfg(windows)] -#[path = "windows.rs"] -mod platform; +// Coordinate this process's exports. Uncooperating external writers can still race +// the final version check; ordinary filesystem replacement is not compare-and-swap. +pub(super) static PUBLICATION: std::sync::Mutex<()> = std::sync::Mutex::new(()); pub(super) struct Staged { - temporary: tempfile::TempPath, - directory: Option, + temporary: Option, pub path: PathBuf, requested: PathBuf, - original: Option, - policy: platform::Policy, - parent_policy: platform::Policy, + original: serde_json::Value, +} + +/// Inspect ordinary metadata for confirmation without opening the destination for writing. +pub(super) fn inspect(path: &str) -> io::Result<(String, serde_json::Value, bool)> { + let requested = requested_path(path)?; + let key = unicode(&requested)?.to_string(); + let resolved = resolve(&requested, false)?; + let parent = resolved.parent().ok_or_else(|| io::Error::other("output has no parent directory"))?; + let (_, anchor) = canonical_parent(parent)?; + let state = readable_state(&resolved)?; + let existing = !state.is_null(); + Ok(( + key, + serde_json::json!({ + "resolved": unicode(&resolved)?, "state": state, + "anchor": unicode(&anchor)?, "anchorPolicy": permissions_key(&fs::metadata(&anchor)?), + }), + existing, + )) +} + +pub(super) fn validate_approval(path: &str, authorization: Option<&serde_json::Value>) -> io::Result<()> { + let requested = requested_path(path)?; + let resolved = resolve(&requested, false)?; + let Some(authorization) = authorization else { + return match fs::symlink_metadata(&resolved) { + Err(error) if error.kind() == io::ErrorKind::NotFound => Ok(()), + Err(error) => Err(error), + Ok(_) => Err(io::Error::other(format!("overwrite confirmation required for {}", requested.display()))), + }; + }; + let targets = crate::preflight::token_targets(authorization)?; + let entry = targets.get(unicode(&requested)?).ok_or_else(|| io::Error::other(format!("export destination was not reviewed: {}", requested.display())))?; + if entry.get("resolved").and_then(serde_json::Value::as_str) != Some(unicode(&resolved)?) || entry.get("state") != Some(&readable_state(&resolved)?) { + return Err(io::Error::other(format!("export destination changed since preflight: {}", requested.display()))); + } + let anchor = entry + .get("anchor") + .and_then(serde_json::Value::as_str) + .filter(|path| path.len() <= 65_536) + .ok_or_else(|| io::Error::other("invalid export authorization directory"))?; + if entry.get("anchorPolicy") != Some(&permissions_key(&fs::metadata(anchor)?)) { + return Err(io::Error::other("export directory policy changed since preflight")); + } + Ok(()) } -#[derive(PartialEq, Eq)] -struct Version { - identity: (u64, u64), - change_stamp: (i64, i64), - len: u64, - modified: SystemTime, - policy: platform::Policy, +fn unicode(path: &Path) -> io::Result<&str> { + path.to_str().filter(|path| path.len() <= 65_536).ok_or_else(|| io::Error::other("export preflight resolved path is non-Unicode or exceeds 65536 bytes")) } -impl Version { - /// Capture the writable destination's identity, content timestamp, and access policy. - fn read(file: &File, path: &Path) -> io::Result { - let metadata = file.metadata()?; - if !metadata.is_file() || platform::links(file)? != 1 { - return Err(io::Error::other(format!("export destination must be a regular file with one hard link: {}", path.display()))); +fn requested_path(path: &str) -> io::Result { + if path.len() > 65_536 { + return Err(io::Error::other("export preflight path exceeds 65536 bytes")); + } + std::path::absolute(path) +} + +fn readable_state(path: &Path) -> io::Result { + #[cfg(windows)] + let file = { + use std::os::windows::fs::OpenOptionsExt; + match fs::OpenOptions::new().access_mode(0).open(path) { + Ok(file) => file, + Err(error) if error.kind() == io::ErrorKind::NotFound => return Ok(serde_json::Value::Null), + Err(error) => return Err(error), } - Ok(Self { - identity: platform::identity(file)?, - change_stamp: platform::change_stamp(file)?, - len: metadata.len(), - modified: metadata.modified()?, - policy: platform::Policy::read_existing(path)?, - }) + }; + #[cfg(windows)] + let metadata = file.metadata()?; + #[cfg(not(windows))] + let metadata = match fs::metadata(path) { + Ok(metadata) => metadata, + Err(error) if error.kind() == io::ErrorKind::NotFound => return Ok(serde_json::Value::Null), + Err(error) => return Err(error), + }; + if !metadata.is_file() { + return Err(io::Error::other("export destination must be a regular file")); + } + #[cfg(windows)] + let identity = { + let info = winapi_util::file::information(&file)?; + (info.volume_serial_number(), info.file_index()) + }; + #[cfg(unix)] + let identity = { + use std::os::unix::fs::MetadataExt; + (metadata.dev(), metadata.ino()) + }; + #[cfg(unix)] + let change = { + use std::os::unix::fs::MetadataExt; + (metadata.ctime(), metadata.ctime_nsec()) + }; + #[cfg(not(unix))] + let change = timestamp(metadata.created()?); + Ok(serde_json::json!({ + "identity": identity, "change": change, "len": metadata.len(), + "modified": timestamp(metadata.modified()?), "permissions": permissions_key(&metadata), + })) +} + +fn timestamp(time: SystemTime) -> (bool, u64, u32) { + match time.duration_since(SystemTime::UNIX_EPOCH) { + Ok(duration) => (false, duration.as_secs(), duration.subsec_nanos()), + Err(error) => (true, error.duration().as_secs(), error.duration().subsec_nanos()), } } +fn permissions_key(metadata: &Metadata) -> serde_json::Value { + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + serde_json::json!(metadata.permissions().mode()) + } + #[cfg(not(unix))] + serde_json::json!(metadata.permissions().readonly()) +} + impl Staged { - /// Check destination capabilities before creating private, job-owned media storage. + /// Encode into a unique sibling created by tempfile; never truncate a final path. pub fn create(path: &str) -> io::Result<(Self, File)> { - let requested = std::path::absolute(path)?; + let requested = requested_path(path)?; let path = resolve(&requested, true)?; + let original = readable_state(&path)?; let parent = path.parent().ok_or_else(|| io::Error::other("output has no parent directory"))?; - let parent_policy = platform::Policy::read(parent)?; - match fs::metadata(&path) { - Ok(metadata) if !metadata.is_file() => return Err(io::Error::other("export destination is not a regular file")), - Err(error) if error.kind() != io::ErrorKind::NotFound => return Err(error), - _ => {} - } - let original = match OpenOptions::new().write(true).open(&path) { - Ok(file) => { - let version = Version::read(&file, &path)?; - Some(version) - } - Err(error) if error.kind() == io::ErrorKind::NotFound => None, - Err(error) => return Err(error), - }; - // The ordinary creation probe is always empty. No exported bytes ever enter an inode - // that another account might have opened before access was restricted. - let probe = platform::ordinary_file(parent)?; - if original.is_none() { - // Reject unsupported no-clobber publication before rendering. The probe contains - // no media; the owned directory also cleans its link on an early return. - let links = tempfile::Builder::new().prefix(".effectcraft-policy-").tempdir_in(parent)?; - fs::hard_link(probe.path(), links.path().join("link")) - .map_err(|error| io::Error::new(error.kind(), format!("export filesystem cannot publish new files with hard links: {error}")))?; - links.close()?; - } - let policy = match &original { - Some(version) => version.policy.clone(), - None => platform::Policy::read(probe.path())?, - }; - policy.apply(probe.path())?; - if !policy.matches(probe.path())? { - return Err(io::Error::other("cannot preserve export destination access policy on this filesystem")); - } - probe.close()?; - let (temporary, directory) = platform::private_file(parent)?; - let (file, temporary) = temporary.into_parts(); - let staged = Self { temporary, directory, path, requested, original, policy, parent_policy }; + let (file, temporary) = tempfile::Builder::new().prefix(".effectcraft-export-").tempfile_in(parent)?.into_parts(); + let staged = Self { temporary: Some(temporary), path, requested, original }; staged.validate()?; Ok((staged, file)) } - /// Reject changed links, destination versions, or parent access policy. pub fn validate(&self) -> io::Result<()> { - if resolve(&self.requested, false)? != self.path { - return Err(io::Error::other("export destination symlink changed while rendering")); - } - let parent = self.path.parent().ok_or_else(|| io::Error::other("output has no parent directory"))?; - if platform::Policy::read(parent)? != self.parent_policy { - return Err(io::Error::other("export directory access policy changed while rendering")); - } - match &self.original { - Some(expected) => { - let current = OpenOptions::new().write(true).open(&self.path)?; - if Version::read(¤t, &self.path)? != *expected { - return Err(io::Error::other(format!("export destination changed while rendering: {}", self.path.display()))); - } - } - None => match fs::symlink_metadata(&self.path) { - Err(error) if error.kind() == io::ErrorKind::NotFound => {} - Err(error) => return Err(error), - Ok(_) => return Err(io::Error::other(format!("export destination appeared while rendering: {}", self.path.display()))), - }, + if resolve(&self.requested, false)? != self.path || readable_state(&self.path)? != self.original { + return Err(io::Error::other(format!("export destination changed while rendering: {}", self.path.display()))); } Ok(()) } - /// Serialize final validation/publication and clean staging even when publication fails. - pub fn publish(self) -> io::Result<()> { - let result = { - let _publication = PUBLICATION.lock().unwrap_or_else(std::sync::PoisonError::into_inner); - self.publish_inner() - }; - let cleanup = self.discard(); - match (result, cleanup) { - (Err(error), Err(cleanup)) => Err(io::Error::new(error.kind(), format!("{error}; cleanup also failed: {cleanup}"))), - (Err(error), _) | (_, Err(error)) => Err(error), - _ => Ok(()), + /// Check every completed staging file before the first sequence publication. + pub fn prepare(&self) -> io::Result<()> { + self.validate()?; + let temporary = self.temporary.as_ref().ok_or_else(|| io::Error::other("export staging file already published"))?; + if !fs::metadata(temporary)?.is_file() { + return Err(io::Error::other("export staging path is not a regular file")); } + Ok(()) } - /// Restore access policy and publish one complete file without clobbering a new creator. - fn publish_inner(&self) -> io::Result<()> { + /// Delegate same-directory publication and ordinary overwrite semantics to tempfile. + pub fn publish_prepared(&mut self) -> io::Result<()> { self.validate()?; - self.policy.apply(&self.temporary)?; - if !self.policy.matches(&self.temporary)? { - return Err(io::Error::other("staged export access policy could not be preserved")); - } - self.validate()?; - if self.original.is_some() { - fs::rename(&self.temporary, &self.path)?; - } else { - // Unlike a check followed by rename, this cannot overwrite a file that appeared - // after validation. Both names refer to complete bytes until staging is removed. - fs::hard_link(&self.temporary, &self.path)?; + let temporary = self.temporary.take().ok_or_else(|| io::Error::other("export staging file already published"))?; + let result = if self.original.is_null() { temporary.persist_noclobber(&self.path) } else { temporary.persist(&self.path) }; + if let Err(error) = result { + self.temporary = Some(error.path); + return Err(error.error); } Ok(()) } - /// Remove only owned staging paths; a successful rename already consumed the file path. + /// Clean only the owned temporary file. Successful publication consumed it already. pub fn discard(self) -> io::Result<()> { - if let Err(error) = self.temporary.close() + if let Some(temporary) = self.temporary + && let Err(error) = temporary.close() && error.kind() != io::ErrorKind::NotFound { return Err(io::Error::new(error.kind(), format!("cannot remove staging for {}: {error}", self.path.display()))); } - if let Some(directory) = self.directory { - directory.close()?; - } Ok(()) } + + #[cfg(test)] + fn publish(mut self) -> io::Result<()> { + let _publication = PUBLICATION.lock().unwrap_or_else(std::sync::PoisonError::into_inner); + self.prepare()?; + let result = self.publish_prepared(); + self.discard()?; + result + } } /// Resolve leaf links explicitly, including dangling relative links. The OS resolves directory @@ -180,13 +206,15 @@ fn resolve(requested: &Path, create_parents: bool) -> io::Result { fs::create_dir_all(parent)?; } let name = path.file_name().ok_or_else(|| io::Error::other("output has no filename"))?; - path = parent.canonicalize()?.join(name); + path = canonical_parent(parent)?.0.join(name); match fs::symlink_metadata(&path) { Ok(metadata) if metadata.file_type().is_symlink() => { let target = fs::read_link(&path)?; path = if target.is_absolute() { target } else { path.parent().ok_or_else(|| io::Error::other("link has no parent"))?.join(target) }; } - Ok(_) => return Ok(path), + // Include the leaf's actual on-disk spelling (not only its parent) in the + // reviewed destination. Existing Windows case aliases must compare equal. + Ok(_) => return path.canonicalize(), Err(error) if error.kind() == io::ErrorKind::NotFound => return Ok(path), Err(error) => return Err(error), } @@ -194,12 +222,45 @@ fn resolve(requested: &Path, create_parents: bool) -> io::Result { Err(io::Error::other("export destination has too many symbolic links")) } +/// Canonicalize the existing ancestor, then normalize only its missing descendants. +/// A preflight can review a new folder without creating it; existing symlinks still +/// resolve through the OS before any lexical normalization of missing components. +fn canonical_parent(parent: &Path) -> io::Result<(PathBuf, PathBuf)> { + let mut cursor = parent.to_path_buf(); + let mut missing = Vec::new(); + loop { + match cursor.canonicalize() { + Ok(anchor) => { + let mut resolved = anchor.clone(); + for component in missing.iter().rev() { + if component == std::ffi::OsStr::new("..") { + resolved.pop(); + } else if component != std::ffi::OsStr::new(".") { + resolved.push(component); + } + } + return Ok((resolved, anchor)); + } + Err(error) if error.kind() == io::ErrorKind::NotFound => { + if missing.len() >= 1024 { + return Err(io::Error::other("export directory has too many missing components")); + } + let component = cursor.components().next_back().ok_or_else(|| io::Error::other("output has no existing ancestor"))?.as_os_str().to_os_string(); + if !cursor.pop() { + return Err(error); + } + missing.push(component); + } + Err(error) => return Err(error), + } + } +} + #[cfg(test)] mod tests { use super::*; use std::io::Write; - /// Prepare completed test bytes and close the writer before publication assertions. fn stage(path: &Path, bytes: &[u8]) -> Staged { let (staged, mut file) = Staged::create(path.to_str().unwrap()).unwrap(); file.write_all(bytes).unwrap(); @@ -209,7 +270,6 @@ mod tests { } #[test] - #[cfg_attr(windows, ignore = "requires enabled SeSecurityPrivilege; exercised by the Windows audit CI runner")] fn lifecycle_preserves_existing_bytes_until_publish_and_cleans_owned_files() { for existing in [false, true] { for action in ["drop", "discard", "publish"] { @@ -218,15 +278,6 @@ mod tests { if existing { fs::write(&path, b"old complete movie").unwrap(); } - let expected = if existing { - platform::Policy::read(&path).unwrap() - } else { - let ordinary = dir.path().join("ordinary"); - fs::write(&ordinary, b"").unwrap(); - let policy = platform::Policy::read(&ordinary).unwrap(); - fs::remove_file(ordinary).unwrap(); - policy - }; let staged = stage(&path, b"new complete movie"); assert_eq!(fs::read(&path).ok(), existing.then(|| b"old complete movie".to_vec())); match action { @@ -234,308 +285,74 @@ mod tests { "discard" => staged.discard().unwrap(), _ => drop(staged), } - if action == "publish" { - assert_eq!(fs::read(&path).unwrap(), b"new complete movie"); - assert_eq!(platform::Policy::read(&path).unwrap(), expected); - } else { - assert_eq!(fs::read(&path).ok(), existing.then(|| b"old complete movie".to_vec())); - } - assert_eq!(fs::read_dir(dir.path()).unwrap().count(), usize::from(existing || action == "publish")); + let expected = if action == "publish" { Some(b"new complete movie".to_vec()) } else { existing.then(|| b"old complete movie".to_vec()) }; + assert_eq!(fs::read(&path).ok(), expected); + assert_eq!(fs::read_dir(dir.path()).unwrap().count(), usize::from(expected.is_some())); } } } #[test] - #[cfg_attr(windows, ignore = "requires enabled SeSecurityPrivilege; exercised by the Windows audit CI runner")] - fn competing_jobs_and_external_writes_are_not_overwritten_or_deleted() { + fn changed_destination_is_preserved_and_staging_is_cleaned() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("movie.mov"); for existing in [false, true] { - let dir = tempfile::tempdir().unwrap(); - let path = dir.path().join("movie.mov"); if existing { - fs::write(&path, b"old").unwrap(); + fs::write(&path, b"original").unwrap(); } - let first = stage(&path, b"first job"); - let second = stage(&path, b"second job"); - second.publish().unwrap(); - assert!(first.publish().is_err()); - assert_eq!(fs::read(&path).unwrap(), b"second job"); - let third = stage(&path, b"third job"); - fs::write(&path, b"external writer with different length").unwrap(); - assert!(third.publish().is_err()); - assert_eq!(fs::read(&path).unwrap(), b"external writer with different length"); + let staged = stage(&path, b"replacement"); + fs::write(&path, b"external writer").unwrap(); + assert!(staged.publish().is_err()); + assert_eq!(fs::read(&path).unwrap(), b"external writer"); assert_eq!(fs::read_dir(dir.path()).unwrap().count(), 1); - - let race = dir.path().join("race.mov"); - if existing { - fs::write(&race, b"old").unwrap(); - } - let a = stage(&race, b"a"); - let b = stage(&race, b"b"); - let barrier = std::sync::Arc::new(std::sync::Barrier::new(2)); - let barrier_b = barrier.clone(); - let successes = std::thread::scope(|scope| { - let a = scope.spawn(move || { - barrier.wait(); - a.publish().is_ok() - }); - let b = scope.spawn(move || { - barrier_b.wait(); - b.publish().is_ok() - }); - usize::from(a.join().unwrap()) + usize::from(b.join().unwrap()) - }); - assert_eq!(successes, 1); - assert!(matches!(fs::read(race).unwrap().as_slice(), b"a" | b"b")); - assert_eq!(fs::read_dir(dir.path()).unwrap().count(), 2); + fs::remove_file(&path).unwrap(); } } #[test] - fn publication_failure_leaves_destination_and_no_staging() { - let dir = tempfile::tempdir().unwrap(); - let path = dir.path().join("movie.mov"); - let staged = stage(&path, b"complete movie"); - fs::create_dir(&path).unwrap(); - assert!(staged.publish().is_err()); - assert!(path.is_dir()); - assert_eq!(fs::read_dir(dir.path()).unwrap().count(), 1); - } - - #[test] - fn hardlink_aliases_are_rejected_without_touching_either_name() { + fn hardlink_replacement_changes_only_the_requested_name() { let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("movie.mov"); let alias = dir.path().join("alias.mov"); fs::write(&path, b"original").unwrap(); fs::hard_link(&path, &alias).unwrap(); - assert!(Staged::create(path.to_str().unwrap()).is_err()); - assert_eq!(fs::read(path).unwrap(), b"original"); + stage(&path, b"replacement").publish().unwrap(); + assert_eq!(fs::read(&path).unwrap(), b"replacement"); assert_eq!(fs::read(alias).unwrap(), b"original"); - assert_eq!(fs::read_dir(dir.path()).unwrap().count(), 2); - } - - #[cfg(unix)] - #[test] - fn relative_and_dangling_links_preserve_links_and_retargeting_fails() { - use std::os::unix::fs::symlink; - let dir = tempfile::tempdir().unwrap(); - let target = dir.path().join("movie.mov"); - let link = dir.path().join("link.mov"); - let chain = dir.path().join("chain.mov"); - symlink("movie.mov", &link).unwrap(); - symlink("link.mov", &chain).unwrap(); - stage(&chain, b"first").publish().unwrap(); - assert_eq!(fs::read(&target).unwrap(), b"first"); - stage(&chain, b"second").publish().unwrap(); - assert_eq!(fs::read(&target).unwrap(), b"second"); - assert_eq!(fs::read_link(&link).unwrap(), Path::new("movie.mov")); - let staged = stage(&chain, b"stale"); - fs::remove_file(&link).unwrap(); - symlink("elsewhere.mov", &link).unwrap(); - assert!(staged.publish().is_err()); - assert!(!dir.path().join("elsewhere.mov").exists()); - fs::remove_file(&link).unwrap(); - symlink("chain.mov", &link).unwrap(); - assert!(Staged::create(chain.to_str().unwrap()).is_err()); - assert_eq!(fs::read(&target).unwrap(), b"second"); - } - - #[cfg(unix)] - #[test] - fn unix_modes_group_and_policy_changes_are_preserved_or_rejected() { - use std::os::unix::fs::{MetadataExt, PermissionsExt}; - for mode in [0o600, 0o640, 0o660] { - let dir = tempfile::tempdir().unwrap(); - let path = dir.path().join("movie.mov"); - fs::write(&path, b"old").unwrap(); - fs::set_permissions(&path, fs::Permissions::from_mode(mode)).unwrap(); - let before = fs::metadata(&path).unwrap(); - let staged = stage(&path, b"replacement"); - assert_eq!(fs::metadata(&staged.temporary).unwrap().permissions().mode() & 0o077, 0); - staged.publish().unwrap(); - let after = fs::metadata(&path).unwrap(); - assert_eq!((after.uid(), after.gid(), after.mode()), (before.uid(), before.gid(), before.mode())); - let staged = stage(&path, b"must not publish"); - fs::set_permissions(&path, fs::Permissions::from_mode(0o400)).unwrap(); - assert!(staged.publish().is_err()); - assert_eq!(fs::read(&path).unwrap(), b"replacement"); - } } #[cfg(windows)] #[test] - #[cfg_attr(windows, ignore = "requires enabled SeSecurityPrivilege; exercised by the Windows audit CI runner")] - fn windows_inherited_and_protected_acls_and_policy_changes() { - use windows_permissions::{ - constants::{SeObjectType as O, SecurityInformation as S}, - wrappers as w, - }; - for protected in [false, true] { - let dir = tempfile::tempdir().unwrap(); - let path = dir.path().join("movie.mov"); - fs::write(&path, b"old").unwrap(); - let owner = w::GetNamedSecurityInfo(path.as_os_str(), O::SE_FILE_OBJECT, S::Owner).unwrap(); - let sid = w::ConvertSidToStringSid(w::GetSecurityDescriptorOwner(&owner).unwrap().unwrap()).unwrap(); - let restricted = w::ConvertStringSecurityDescriptorToSecurityDescriptor(&format!("D:P(A;;FA;;;{})", sid.to_string_lossy())).unwrap(); - if protected { - w::SetNamedSecurityInfo( - path.as_os_str(), - O::SE_FILE_OBJECT, - S::Dacl | S::ProtectedDacl, - None, - None, - w::GetSecurityDescriptorDacl(&restricted).unwrap(), - None, - ) - .unwrap(); - } - let expected = platform::Policy::read(&path).unwrap(); - let staged = stage(&path, b"replacement"); - let private = w::GetNamedSecurityInfo(staged.temporary.as_os_str(), O::SE_FILE_OBJECT, S::Dacl).unwrap(); - let private_text = w::ConvertSecurityDescriptorToStringSecurityDescriptor(&private, S::Dacl).unwrap(); - // SDDL abbreviates well-known SIDs (for example S-1-5-32-544 becomes BA). - // Compare descriptors serialized by the same API rather than a numeric SID. - let restricted_text = w::ConvertSecurityDescriptorToStringSecurityDescriptor(&restricted, S::Dacl).unwrap(); - assert_eq!(private_text.to_string_lossy().replace("D:PAI", "D:P"), restricted_text.to_string_lossy()); - staged.publish().unwrap(); - assert_eq!(platform::Policy::read(&path).unwrap(), expected); - let staged = stage(&path, b"must not publish"); - let original_permissions = fs::metadata(&path).unwrap().permissions(); - let mut permissions = original_permissions.clone(); - permissions.set_readonly(true); - fs::set_permissions(&path, permissions).unwrap(); - assert!(staged.publish().is_err()); - assert_eq!(fs::read(&path).unwrap(), b"replacement"); - fs::set_permissions(&path, original_permissions).unwrap(); - } - } - - #[cfg(target_os = "linux")] - #[test] - fn linux_named_acl_and_setgid_parent_do_not_broaden_replacement() { - use std::os::unix::fs::{MetadataExt, PermissionsExt}; + fn library_replacement_error_preserves_locked_destination_and_cleans_staging() { + use std::os::windows::fs::OpenOptionsExt; let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("movie.mov"); - fs::write(&path, b"old").unwrap(); - // Linux POSIX ACL xattr format: version followed by tag, permissions, numeric ID. - let mut acl = 2_u32.to_le_bytes().to_vec(); - for (tag, perms, id) in [(1_u16, 6_u16, u32::MAX), (2, 4, 12345), (4, 0, u32::MAX), (16, 4, u32::MAX), (32, 0, u32::MAX)] { - acl.extend(tag.to_le_bytes()); - acl.extend(perms.to_le_bytes()); - acl.extend(id.to_le_bytes()); - } - xattr::set(&path, "system.posix_acl_access", &acl).unwrap(); - let expected = platform::Policy::read(&path).unwrap(); - if fs::metadata(&path).unwrap().uid() == 0 { - // The local Linux probe runs as root, allowing a genuine different-group case. - std::os::unix::fs::chown(dir.path(), None, Some(54321)).unwrap(); - } - fs::set_permissions(dir.path(), fs::Permissions::from_mode(0o2770)).unwrap(); - let mut inherited_acl = acl.clone(); - inherited_acl[16..20].copy_from_slice(&23456_u32.to_le_bytes()); - xattr::set(dir.path(), "system.posix_acl_default", &inherited_acl).unwrap(); + fs::write(&path, b"original").unwrap(); + let mut staged = stage(&path, b"replacement"); + // Metadata-only access remains possible, but the library cannot replace this + // file while an ordinary reader denies delete sharing. + let lock = fs::OpenOptions::new().read(true).share_mode(1).open(&path).unwrap(); + staged.prepare().unwrap(); + let error = staged.publish_prepared().unwrap_err(); + assert!(matches!(error.raw_os_error(), Some(5 | 32)), "{error}"); + staged.discard().unwrap(); + assert_eq!(fs::read(&path).unwrap(), b"original"); + assert_eq!(fs::read_dir(dir.path()).unwrap().count(), 1); + drop(lock); stage(&path, b"replacement").publish().unwrap(); - assert_eq!(platform::Policy::read(&path).unwrap(), expected); - assert_eq!(xattr::get(&path, "system.posix_acl_access").unwrap().unwrap(), acl); - let staged = stage(&path, b"stale policy"); - fs::set_permissions(dir.path(), fs::Permissions::from_mode(0o2700)).unwrap(); - assert!(staged.publish().is_err()); - assert_eq!(fs::read(path).unwrap(), b"replacement"); - } - - #[cfg(target_os = "macos")] - #[test] - fn macos_inherited_acl_is_absent_from_private_staging_and_restored_on_publish() { - let dir = tempfile::tempdir().unwrap(); - let acl = - [exacl::AclEntry::allow_group("everyone", exacl::Perm::READ | exacl::Perm::EXECUTE, exacl::Flag::FILE_INHERIT | exacl::Flag::DIRECTORY_INHERIT)]; - exacl::setfacl(&[dir.path()], &acl, None).unwrap(); - let path = dir.path().join("movie.mov"); - fs::write(&path, b"old").unwrap(); - let expected = platform::Policy::read(&path).unwrap(); - let staged = stage(&path, b"replacement"); - assert!(exacl::getfacl(&staged.temporary, None).unwrap().is_empty()); - staged.publish().unwrap(); - assert_eq!(platform::Policy::read(&path).unwrap(), expected); - assert_eq!(fs::read(path).unwrap(), b"replacement"); - } - /// More frames than a typical descriptor limit: snapshots retain metadata, not open files. - #[test] - #[cfg_attr(windows, ignore = "requires enabled SeSecurityPrivilege; exercised by the Windows audit CI runner")] - fn replacement_sequence_keeps_file_handle_use_constant() { - let dir = tempfile::tempdir().unwrap(); - let mut pending = Vec::new(); - for frame in 0..1536 { - let path = dir.path().join(format!("frame_{frame}.png")); - fs::write(&path, b"old").unwrap(); - pending.push(stage(&path, b"new")); - } - for staged in pending { - let path = staged.path.clone(); - assert_eq!(fs::read(&path).unwrap(), b"old"); - staged.publish().unwrap(); - assert_eq!(fs::read(&path).unwrap(), b"new"); - } - assert_eq!(fs::read_dir(dir.path()).unwrap().count(), 1536); } - /// Refusal must leave existing bytes and policy intact without disabling new exports. - #[cfg(windows)] + #[cfg(unix)] #[test] - fn windows_audit_refusal_preserves_existing_and_allows_new() { - const CHILD: &str = "EFFECTCRAFT_AUDIT_REFUSAL_CHILD"; - if std::env::var_os(CHILD).is_none() { - let name = format!("{}::windows_audit_refusal_preserves_existing_and_allows_new", module_path!().split_once("::").unwrap().1); - let status = std::process::Command::new(std::env::current_exe().unwrap()).args(["--exact", &name, "--nocapture"]).env(CHILD, "1").status().unwrap(); - assert!(status.success()); - return; - } - // Privilege removal affects only this isolated child, never parallel tests. - let token = winsafe::HPROCESS::GetCurrentProcess().OpenProcessToken(winsafe::co::TOKEN::ADJUST_PRIVILEGES | winsafe::co::TOKEN::QUERY).unwrap(); - let luid = winsafe::LookupPrivilegeValue(None, winsafe::co::SE_PRIV::SECURITY_NAME).unwrap(); - let privileges = winsafe::TOKEN_PRIVILEGES::new(&[winsafe::LUID_AND_ATTRIBUTES::new(luid, winsafe::co::SE_PRIV_ATTR::REMOVED)]).unwrap(); - token.AdjustTokenPrivileges(winsafe::DisabPriv::Privs(&privileges)).unwrap(); + fn relative_and_dangling_symlinks_keep_the_link() { + use std::os::unix::fs::symlink; let dir = tempfile::tempdir().unwrap(); - let existing = dir.path().join("old.mov"); - fs::write(&existing, b"old complete movie").unwrap(); - let error = match Staged::create(existing.to_str().unwrap()) { - Ok(_) => panic!("replacement unexpectedly succeeded without audit privilege"), - Err(error) => error, - }; - assert!(error.to_string().contains("Windows audit policy"), "{error}"); - assert_eq!(fs::read(&existing).unwrap(), b"old complete movie"); - let new = dir.path().join("new.mov"); - stage(&new, b"new complete movie").publish().unwrap(); - assert_eq!(fs::read(new).unwrap(), b"new complete movie"); - assert_eq!(fs::read_dir(dir.path()).unwrap().count(), 2); - } - - /// Audit ACEs and SACL protection survive replacement; changes during rendering refuse it. - #[cfg(windows)] - #[test] - #[ignore = "requires enabled SeSecurityPrivilege; exercised by the Windows audit CI runner"] - fn windows_audit_entries_and_protection_are_preserved() { - use windows_permissions::{ - constants::{SeObjectType as O, SecurityInformation as S}, - wrappers as w, - }; - for protected in [false, true] { - let dir = tempfile::tempdir().unwrap(); - let path = dir.path().join("audited.mov"); - fs::write(&path, b"old complete movie").unwrap(); - let descriptor = w::ConvertStringSecurityDescriptorToSecurityDescriptor("S:P(AU;SAFA;FW;;;WD)").unwrap(); - let flags = S::Sacl | if protected { S::ProtectedSacl } else { S::UnprotectedSacl }; - w::SetNamedSecurityInfo(path.as_os_str(), O::SE_FILE_OBJECT, flags, None, None, None, w::GetSecurityDescriptorSacl(&descriptor).unwrap()).unwrap(); - let expected = platform::Policy::read_existing(&path).unwrap(); - stage(&path, b"complete replacement").publish().unwrap(); - assert!(expected.matches(&path).unwrap()); - let staged = stage(&path, b"must not publish"); - let changed = w::ConvertStringSecurityDescriptorToSecurityDescriptor("S:P(AU;SAFA;FR;;;WD)").unwrap(); - w::SetNamedSecurityInfo(path.as_os_str(), O::SE_FILE_OBJECT, flags, None, None, None, w::GetSecurityDescriptorSacl(&changed).unwrap()).unwrap(); - let changed = platform::Policy::read_existing(&path).unwrap(); - assert!(staged.publish().is_err()); - assert_eq!(fs::read(&path).unwrap(), b"complete replacement"); - assert!(changed.matches(&path).unwrap()); - } + let target = dir.path().join("target.mov"); + let link = dir.path().join("link.mov"); + symlink("target.mov", &link).unwrap(); + stage(&link, b"first").publish().unwrap(); + stage(&link, b"second").publish().unwrap(); + assert_eq!(fs::read(&target).unwrap(), b"second"); + assert_eq!(fs::read_link(&link).unwrap(), Path::new("target.mov")); } } diff --git a/crates/export/src/pending/unix.rs b/crates/export/src/pending/unix.rs deleted file mode 100644 index 5c74bfdea..000000000 --- a/crates/export/src/pending/unix.rs +++ /dev/null @@ -1,137 +0,0 @@ -use std::{ - ffi::OsString, - fs::{self, File}, - io, - os::unix::fs::{MetadataExt, PermissionsExt}, - path::Path, -}; - -#[derive(Clone, Debug, PartialEq, Eq)] -pub(super) struct Policy { - uid: u32, - gid: u32, - mode: u32, - attributes: Vec<(OsString, Vec)>, - #[cfg(any(target_os = "macos", target_os = "freebsd"))] - acl: Vec, -} - -impl Policy { - /// Unix replacement uses the same ownership, mode, and access-ACL snapshot as creation. - pub fn read_existing(path: &Path) -> io::Result { - Self::read(path) - } - - /// Verify the restored access policy against this output's snapshot. - pub fn matches(&self, path: &Path) -> io::Result { - Ok(Self::read(path)? == *self) - } - - /// Snapshot ownership, access controls, and permission state used for publication checks. - pub fn read(path: &Path) -> io::Result { - let metadata = fs::metadata(path)?; - let mut attributes = Vec::new(); - for name in xattr::list(path)? { - let text = name.to_string_lossy(); - if text.starts_with("security.") || text.starts_with("system.") { - let value = xattr::get(path, &name)?.ok_or_else(|| io::Error::other("access attribute changed while reading"))?; - if value.len() > 1_048_576 || attributes.len() >= 128 { - return Err(io::Error::other("export access metadata exceeds supported limits")); - } - attributes.push((name, value)); - } - } - attributes.sort_by(|a, b| a.0.cmp(&b.0)); - Ok(Self { - uid: metadata.uid(), - gid: metadata.gid(), - mode: metadata.mode() & 0o7777, - attributes, - #[cfg(any(target_os = "macos", target_os = "freebsd"))] - acl: exacl::getfacl(path, None)?, - }) - } - - /// Restore the captured access policy; unsupported or unauthorized changes fail closed. - pub fn apply(&self, path: &Path) -> io::Result<()> { - // An export is data, not a privileged executable. Do not transfer privilege bits or - // file capabilities onto newly generated bytes. - if self.mode & 0o7000 != 0 || self.attributes.iter().any(|(name, _)| name == "security.capability") { - return Err(io::Error::other("cannot replace an export destination carrying executable privileges")); - } - // Read access attributes before removing read permission: macOS requires it even - // when the caller owns the inode. Reuse this snapshot while restoring attributes. - let current = Self::read(path)?; - // Do not grant the destination owner the staging inode's initial owner-read bit - // while transferring ownership. ACL grants/denials are installed before final mode. - fs::set_permissions(path, fs::Permissions::from_mode(0))?; - if (current.uid, current.gid) != (self.uid, self.gid) { - std::os::unix::fs::chown(path, Some(self.uid), Some(self.gid))?; - } - for (name, _) in ¤t.attributes { - if !self.attributes.iter().any(|(wanted, _)| wanted == name) { - xattr::remove(path, name)?; - } - } - for (name, value) in &self.attributes { - if !current.attributes.iter().any(|(old_name, old_value)| old_name == name && old_value == value) { - xattr::set(path, name, value)?; - } - } - #[cfg(any(target_os = "macos", target_os = "freebsd"))] - exacl::setfacl(&[path], &self.acl, None)?; - // Restore access ACLs before mode bits: chmod must not open an inherited ACL mask - // or permit access before a macOS deny entry has been installed. - fs::set_permissions(path, fs::Permissions::from_mode(self.mode))?; - Ok(()) - } -} - -/// Create an empty probe using the destination directory's ordinary creation policy. -pub(super) fn ordinary_file(parent: &Path) -> io::Result { - tempfile::Builder::new().prefix(".effectcraft-policy-").permissions(fs::Permissions::from_mode(0o666)).tempfile_in(parent) -} - -/// Create job-owned staging that is private before any media bytes are written. -pub(super) fn private_file(parent: &Path) -> io::Result<(tempfile::NamedTempFile, Option)> { - #[cfg(any(target_os = "macos", target_os = "freebsd"))] - { - // macOS ACL allows can override mode 0600. Clear inheritance in an empty owned - // directory before creating the media inode; the empty directory never holds media. - let directory = tempfile::Builder::new().prefix(".effectcraft-export-").tempdir_in(parent)?; - #[cfg(target_os = "macos")] - exacl::setfacl(&[directory.path()], &[], None)?; - #[cfg(target_os = "freebsd")] - exacl::setfacl( - &[directory.path()], - &[ - exacl::AclEntry::allow_user("", exacl::Perm::READ | exacl::Perm::WRITE | exacl::Perm::EXECUTE, None), - exacl::AclEntry::allow_group("", exacl::Perm::empty(), None), - exacl::AclEntry::allow_other(exacl::Perm::empty(), None), - ], - None, - )?; - fs::set_permissions(directory.path(), fs::Permissions::from_mode(0o700))?; - let file = tempfile::NamedTempFile::new_in(directory.path())?; - Ok((file, Some(directory))) - } - #[cfg(not(any(target_os = "macos", target_os = "freebsd")))] - Ok((tempfile::Builder::new().prefix(".effectcraft-export-").tempfile_in(parent)?, None)) -} - -/// Identify the open inode independently of its pathname. -pub(super) fn identity(file: &File) -> io::Result<(u64, u64)> { - let metadata = file.metadata()?; - Ok((metadata.dev(), metadata.ino())) -} - -/// Count aliases so replacement cannot silently detach a hard-linked destination. -pub(super) fn links(file: &File) -> io::Result { - Ok(file.metadata()?.nlink()) -} - -/// Capture inode change time to detect content or policy changes beyond modification time. -pub(super) fn change_stamp(file: &File) -> io::Result<(i64, i64)> { - let metadata = file.metadata()?; - Ok((metadata.ctime(), metadata.ctime_nsec())) -} diff --git a/crates/export/src/pending/windows.rs b/crates/export/src/pending/windows.rs deleted file mode 100644 index 92678a10b..000000000 --- a/crates/export/src/pending/windows.rs +++ /dev/null @@ -1,233 +0,0 @@ -use std::{ - ffi::OsString, - fs::{self, File, OpenOptions}, - io, - path::Path, -}; -use windows_permissions::{ - constants::{SeObjectType as O, SecurityInformation as S}, - wrappers as w, -}; - -const OBJECT: O = O::SE_FILE_OBJECT; - -#[derive(Clone, Debug, Eq)] -pub(super) struct Policy { - descriptor: OsString, - readonly: bool, - audit: bool, -} - -impl PartialEq for Policy { - /// Compare access policy while tolerating Windows' duplicate-grant legacy conversion. - fn eq(&self, other: &Self) -> bool { - self.readonly == other.readonly - && self.audit == other.audit - && comparison_descriptor(&self.descriptor.to_string_lossy()) == comparison_descriptor(&other.descriptor.to_string_lossy()) - } -} - -/// Canonicalize only simple, unprotected file allow ACLs whose inherited ACEs duplicate -/// explicit grants. SetNamedSecurityInfo adds these during legacy ACL conversion; AI -/// records conversion to the current inheritance model. All other descriptors remain -/// exact, including denies, new inherited rights, owner/group, protection, and SACLs. -fn comparison_descriptor(descriptor: &str) -> String { - let Some((owner, acl)) = descriptor.split_once("D:") else { return descriptor.into() }; - let (acl, sacl) = match acl.split_once("S:") { - Some((acl, sacl)) => (acl, Some(sacl)), - None => (acl, None), - }; - let acl = acl.strip_prefix("AI").unwrap_or(acl); - let aces = acl.split_inclusive(')').collect::>(); - if aces.is_empty() - || !aces.iter().all(|ace| { - let fields = ace.split(';').collect::>(); - fields.len() == 6 - && fields.first() == Some(&"(A") - && matches!(fields.get(1), Some(&"") | Some(&"ID")) - && fields.get(3) == Some(&"") - && fields.get(4) == Some(&"") - && ace.ends_with(')') - && ace.matches('(').count() == 1 - }) - { - return descriptor.into(); - } - for ace in &aces { - if let Some(grant) = ace.strip_prefix("(A;ID;") - && !aces.iter().any(|other| other.strip_prefix("(A;;") == Some(grant)) - { - return descriptor.into(); - } - } - let mut result = format!("{owner}D:"); - for ace in aces.iter().filter(|ace| ace.starts_with("(A;;")) { - result.push_str(ace); - } - if let Some(sacl) = sacl { - result.push_str("S:"); - result.push_str(sacl); - } - result -} - -impl Policy { - /// Snapshot ownership, access controls, and permission state used for publication checks. - pub fn read(path: &Path) -> io::Result { - Self::read_with_audit(path, false) - } - - /// Replacing an inode requires its full audit policy, not only access-control metadata. - pub fn read_existing(path: &Path) -> io::Result { - Self::read_with_audit(path, true) - } - - /// Verify exactly the policy components captured for this output. - pub fn matches(&self, path: &Path) -> io::Result { - let actual = Self::read_with_audit(path, self.audit)?; - #[cfg(test)] - if actual != *self { - eprintln!("policy mismatch: expected {self:?}; actual {actual:?}"); - } - Ok(actual == *self) - } - - /// Full SACL reads require an already-enabled SeSecurityPrivilege; never silently - /// fall back to a partial snapshot or alter the application's privilege state. - fn read_with_audit(path: &Path, audit: bool) -> io::Result { - let mut flags = S::Owner | S::Group | S::Dacl | S::Label | S::Attribute | S::Scope; - if audit { - flags |= S::Sacl; - } - let descriptor = w::GetNamedSecurityInfo(path.as_os_str(), OBJECT, flags).map_err(|error| { - if audit { - io::Error::new(error.kind(), format!("cannot preserve the existing export's Windows audit policy: {error}; replacement requires enabled SeSecurityPrivilege; choose a new output filename")) - } else { error } - })?; - Ok(Self { - descriptor: w::ConvertSecurityDescriptorToStringSecurityDescriptor(&descriptor, S::Owner | S::Group | S::Dacl | S::Sacl)?, - readonly: path.metadata()?.permissions().readonly(), - audit, - }) - } - - /// Restore the captured access policy; unsupported or unauthorized changes fail closed. - pub fn apply(&self, path: &Path) -> io::Result<()> { - let descriptor = w::ConvertStringSecurityDescriptorToSecurityDescriptor(&self.descriptor)?; - // SDDL's D: flags precede the first ACE. Preserve inheritance instead of freezing - // every inherited ACE as an explicit grant. Staging is a sibling of the final file. - let text = self.descriptor.to_string_lossy(); - let protected = text.split("D:").nth(1).unwrap_or("").split('(').next().unwrap_or("").split("S:").next().unwrap_or("").contains('P'); - let sacl = w::GetSecurityDescriptorSacl(&descriptor)?; - let mut flags = S::Owner | S::Group | S::Dacl | if protected { S::ProtectedDacl } else { S::UnprotectedDacl }; - if sacl.is_some() { - flags |= S::Label | S::Attribute | S::Scope; - } - // Setting a null SACL can materialize a different descriptor. Leave an absent - // SACL untouched; the full readback must still prove it matches the destination. - // Unexpected inherited audit entries therefore cause refusal, never silent loss. - if self.audit && sacl.is_some() { - let protected_sacl = text.split("S:").nth(1).unwrap_or("").split('(').next().unwrap_or("").contains('P'); - flags |= S::Sacl | if protected_sacl { S::ProtectedSacl } else { S::UnprotectedSacl }; - } - w::SetNamedSecurityInfo( - path.as_os_str(), - OBJECT, - flags, - w::GetSecurityDescriptorOwner(&descriptor)?, - w::GetSecurityDescriptorGroup(&descriptor)?, - w::GetSecurityDescriptorDacl(&descriptor)?, - sacl, - )?; - let mut permissions = path.metadata()?.permissions(); - permissions.set_readonly(self.readonly); - fs::set_permissions(path, permissions) - } -} - -/// Create an empty probe using the destination directory's ordinary creation policy. -pub(super) fn ordinary_file(parent: &Path) -> io::Result { - tempfile::Builder::new().prefix(".effectcraft-policy-").tempfile_in(parent) -} - -/// Create job-owned staging that is private before any media bytes are written. -pub(super) fn private_file(parent: &Path) -> io::Result<(tempfile::NamedTempFile, Option)> { - let file = tempfile::Builder::new().prefix(".effectcraft-export-").make_in(parent, |path| { - let name = path.to_str().ok_or_else(|| io::Error::other("non-Unicode export staging path"))?; - let mut acl = winsafe::ACL { AclRevision: 2, AclSize: std::mem::size_of::() as u16, ..Default::default() }; - let mut descriptor = winsafe::InitializeSecurityDescriptor().map_err(io::Error::other)?; - descriptor.Control = winsafe::co::SE::DACL_PRESENT | winsafe::co::SE::DACL_PROTECTED; - descriptor.Dacl = &mut acl; - let mut attributes = winsafe::SECURITY_ATTRIBUTES::default(); - attributes.set_lpSecurityDescriptor(Some(&mut descriptor)); - // An empty protected DACL denies access at creation, including during the transition - // to owner-only access. No broadly readable handle can predate the media writes. - let (handle, _) = winsafe::HFILE::CreateFile( - name, - winsafe::co::GENERIC::READ | winsafe::co::GENERIC::WRITE, - Some(winsafe::co::FILE_SHARE::READ | winsafe::co::FILE_SHARE::WRITE | winsafe::co::FILE_SHARE::DELETE), - Some(&attributes), - winsafe::co::DISPOSITION::CREATE_NEW, - winsafe::co::FILE_ATTRIBUTE::NORMAL, - None, - None, - None, - ) - .map_err(|error| io::Error::from_raw_os_error(error.raw() as i32))?; - let result = (|| { - let owner_descriptor = w::GetNamedSecurityInfo(path.as_os_str(), OBJECT, S::Owner)?; - let owner = w::GetSecurityDescriptorOwner(&owner_descriptor)?.ok_or_else(|| io::Error::other("staging has no owner"))?; - let sid = w::ConvertSidToStringSid(owner)?; - let private = w::ConvertStringSecurityDescriptorToSecurityDescriptor(&format!("D:P(A;;FA;;;{})", sid.to_string_lossy()))?; - w::SetNamedSecurityInfo(path.as_os_str(), OBJECT, S::Dacl | S::ProtectedDacl, None, None, w::GetSecurityDescriptorDacl(&private)?, None)?; - OpenOptions::new().read(true).write(true).open(path) - })(); - drop(handle); - if result.is_err() { - let _ = fs::remove_file(path); // No media exists; retain the original setup error. - } - result - })?; - Ok((file, None)) -} - -/// Identify the open inode independently of its pathname. -pub(super) fn identity(file: &File) -> io::Result<(u64, u64)> { - let info = winapi_util::file::information(file)?; - Ok((info.volume_serial_number(), info.file_index())) -} - -/// Count aliases so replacement cannot silently detach a hard-linked destination. -pub(super) fn links(file: &File) -> io::Result { - Ok(winapi_util::file::information(file)?.number_of_links()) -} - -/// Capture creation time as an additional guard against reused file identities. -pub(super) fn change_stamp(file: &File) -> io::Result<(i64, i64)> { - let stamp = winapi_util::file::information(file)?.creation_time().ok_or_else(|| io::Error::other("missing file creation time"))?; - Ok((stamp as i64, 0)) -} - -#[cfg(test)] -mod tests { - use super::*; - - /// The runner's legacy ACL conversion is equivalent; additional rights or changed - /// ownership, protection, denies, and security labels must still be rejected. - #[test] - fn legacy_conversion_accepts_only_duplicate_allow_grants() { - let old = "O:BAG:SYD:(A;;FA;;;SY)(A;;FA;;;BA)"; - let converted = "O:BAG:SYD:AI(A;;FA;;;SY)(A;;FA;;;BA)(A;ID;FA;;;SY)(A;ID;FA;;;BA)"; - assert_eq!(comparison_descriptor(old), comparison_descriptor(converted)); - for changed in [ - converted.replace("O:BA", "O:SY"), - converted.replace("D:AI", "D:PAI"), - format!("{converted}(A;ID;FA;;;WD)"), - converted.replace("(A;ID;FA;;;BA)", "(A;ID;FR;;;BA)"), - format!("{converted}S:(ML;;NW;;;LW)"), - ] { - assert_ne!(comparison_descriptor(old), comparison_descriptor(&changed), "{changed}"); - } - assert_ne!(comparison_descriptor("O:BAG:SYD:(D;;FW;;;WD)(A;;FA;;;SY)"), comparison_descriptor("O:BAG:SYD:AI(D;;FW;;;WD)(A;;FA;;;SY)(A;ID;FA;;;SY)")); - } -} diff --git a/crates/export/src/pipeline.rs b/crates/export/src/pipeline.rs index 51867d8c5..3376fbffb 100644 --- a/crates/export/src/pipeline.rs +++ b/crates/export/src/pipeline.rs @@ -105,6 +105,7 @@ pub(crate) struct Cx<'a> { /// Files written to an overflow folder. pub overflowed: Mutex>, pub pending: crate::pending::Pending, + pub authorization: Option<&'a serde_json::Value>, } impl<'a> std::ops::Deref for Cx<'a> { @@ -152,7 +153,7 @@ impl<'a> Cx<'a> { let cache = LayerCache::default(); // With deferred readbacks, buffers computed from a missed pass's placeholders are not kept. cache.set_gate(job.accel.and_then(|a| a.miss_gate())); - Cx { job, project, cache, frame_times, overflowed: Mutex::new(vec![]), pending: crate::pending::Pending::default() } + Cx { job, project, cache, frame_times, overflowed: Mutex::new(vec![]), pending: crate::pending::Pending::default(), authorization: None } } pub fn comp(&self) -> Option<&Comp> { @@ -294,7 +295,7 @@ impl<'a> Cx<'a> { /// Stage media under this job's ownership; final paths remain unchanged until publication. pub fn create(&self, path: &str) -> crate::Result> { - self.pending.create(self.sink, path) + self.pending.create_authorized(self.sink, path, self.authorization) } /// Where a file goes: `path`, or (Use Storage Overflow) the first overflow folder with room @@ -356,7 +357,7 @@ impl<'a> Cx<'a> { s.push_str(&format!(" Frame {} ({:.3} s): rendered in {:.1} ms\n", f0 + i as i64, t.seconds(), secs * 1000.0)); } } - let mut f = out::create(self.sink, &path).ok()?; + let (mut f, path) = out::create_log(self.sink, &path).ok()?; std::io::Write::write_all(&mut f, s.as_bytes()).ok()?; f.finish().ok()?; Some(path) diff --git a/crates/export/src/preflight.rs b/crates/export/src/preflight.rs new file mode 100644 index 000000000..4e59b5403 --- /dev/null +++ b/crates/export/src/preflight.rs @@ -0,0 +1,133 @@ +//! Read-only destination review, shared by interactive and control-channel hosts. + +use serde_json::{Map, Value, json}; + +use crate::{Job, Result, io}; + +const FORMAT: &str = "effectcraft.output-approval.v1"; +const MAX_TARGETS: usize = 100_000; +#[cfg(not(target_arch = "wasm32"))] +const MAX_TOKEN_BYTES: usize = 64 * 1024 * 1024; + +/// A host retains `authorization` with its queue/settings snapshot, shows the collisions +/// once, and supplies the unchanged value to `export_authorized` after acceptance. +#[derive(Clone, Debug)] +pub struct OutputPreflight { + pub collisions: Vec, + /// Canonical candidate write paths, for detecting conflicts across queued outputs. + pub destinations: Vec, + pub authorization: Value, +} + +/// Inspect exactly the paths the exporter can write, including sequence frame numbers +/// and configured overflow alternatives. Inspection creates no files or directories. +pub fn preflight(job: &Job<'_>) -> Result { + let (targets, collisions, destinations) = collect(job)?; + Ok(OutputPreflight { collisions, destinations, authorization: json!({"format": FORMAT, "targets": targets}) }) +} + +#[cfg(not(target_arch = "wasm32"))] +fn collect(job: &Job<'_>) -> Result<(Map, Vec, Vec)> { + let mut targets = Map::new(); + let mut collisions = Vec::new(); + let mut bytes = 0_usize; + let mut destinations = std::collections::BTreeSet::new(); + if job.sink.is_none() { + visit_paths(job, |path, skipped| { + // Skipped files need neither write access nor approval. Omitting them from + // the token also prevents a later disappearance from authorizing a write. + if skipped { + return Ok(()); + } + let (key, entry, existing) = crate::pending::inspect(path).map_err(io)?; + if targets.contains_key(&key) { + return Ok(()); + } + if targets.len() >= MAX_TARGETS { + return Err(io("export preflight exceeds 100000 distinct output paths")); + } + let size = serde_json::to_vec(&entry) + .map_err(io)? + .len() + .checked_add(key.len().saturating_mul(6)) + .and_then(|size| size.checked_add(8)) + .ok_or_else(|| io("export preflight metadata size overflow"))?; + bytes = bytes.checked_add(size).filter(|n| *n <= MAX_TOKEN_BYTES).ok_or_else(|| io("export preflight metadata exceeds 64 MiB"))?; + if existing { + collisions.push(path.to_string()); + } + let resolved = entry.get("resolved").and_then(Value::as_str).ok_or_else(|| io("invalid inspected destination"))?; + destinations.insert(resolved.to_string()); + targets.insert(key, entry); + Ok(()) + })?; + } + Ok((targets, collisions, destinations.into_iter().collect())) +} + +#[cfg(target_arch = "wasm32")] +fn collect(job: &Job<'_>) -> Result<(Map, Vec, Vec)> { + if job.sink.is_none() { + return Err(io("web exports require an output sink")); + } + Ok((Map::new(), Vec::new(), Vec::new())) +} + +pub(crate) fn validate(job: &Job<'_>, authorization: Option<&Value>) -> Result<()> { + if job.sink.is_some() { + return Ok(()); + } + if let Some(authorization) = authorization { + token_targets(authorization).map_err(io)?; + } + visit_paths(job, |path, skipped| { + if skipped { + return Ok(()); + } + #[cfg(not(target_arch = "wasm32"))] + return crate::pending::validate_approval(path, authorization).map_err(io); + #[cfg(target_arch = "wasm32")] + Err(io(format!("web exports require an output sink: {path}"))) + }) +} + +pub(crate) fn token_targets(authorization: &Value) -> std::io::Result<&Map> { + if authorization.get("format").and_then(Value::as_str) != Some(FORMAT) { + return Err(std::io::Error::other("invalid export authorization; run preflight again")); + } + authorization + .get("targets") + .and_then(Value::as_object) + .filter(|targets| targets.len() <= MAX_TARGETS) + .ok_or_else(|| std::io::Error::other("invalid or oversized export authorization")) +} + +fn visit_paths(job: &Job<'_>, mut visit: impl FnMut(&str, bool) -> Result<()>) -> Result<()> { + let comp = job.project.comp(job.comp).ok_or(crate::ExportError::NoComp)?; + let frames = if job.output.format.is_sequence() { job.settings.frame_count(comp) } else { 1 }; + let alternatives = if job.settings.storage_overflow && job.options.storage.is_some() { job.options.overflow.len() } else { 0 }; + let count = frames.checked_mul(alternatives.saturating_add(1) as u64).ok_or_else(|| io("export preflight path count overflow"))?; + if count > MAX_TARGETS as u64 { + return Err(io("export preflight exceeds 100000 output candidates")); + } + let first = job.settings.first_frame(comp); + for frame in 0..frames { + let path = if job.output.format.is_sequence() { + let frame = i64::try_from(frame).ok().and_then(|frame| first.checked_add(frame)).ok_or_else(|| io("export frame number overflow"))?; + crate::render_queue::sequence_path(job.path, frame) + } else { + job.path.to_string() + }; + let skipped = job.output.format.is_sequence() && job.settings.skip_existing && crate::out::exists(None, &path); + visit(&path, skipped)?; + if alternatives > 0 { + let name = std::path::Path::new(&path).file_name().ok_or_else(|| io("export output has no filename"))?; + for directory in &job.options.overflow { + let alternate = std::path::Path::new(directory).join(name); + let alternate = alternate.to_str().ok_or_else(|| io("non-Unicode overflow output path"))?; + visit(alternate, skipped)?; + } + } + } + Ok(()) +} diff --git a/crates/export/tests/render_settings.rs b/crates/export/tests/render_settings.rs index 1fc59d898..555869b2d 100644 --- a/crates/export/tests/render_settings.rs +++ b/crates/export/tests/render_settings.rs @@ -1,5 +1,5 @@ -//! Render Settings and Output Module features end to end (through a [`Sink`], so nothing touches -//! the disk): field rendering and 3:2 pulldown, crop / region of interest / resize, alpha-only +//! Render Settings and Output Module features end to end, through a [`Sink`] and native +//! temporary files: field rendering and 3:2 pulldown, crop / region of interest / resize, alpha-only //! and premultiplied output, the solo and guide-layer overrides, the render log, storage //! overflow, PCM audio formats and the compositor a job renders on. @@ -18,6 +18,29 @@ use effectcraft_time::{FrameRate, Tick}; const W: u32 = 64; const H: u32 = 48; +fn disk_job<'a>(p: &'a Project, cid: ItemId, settings: &'a RenderSettings, output: &'a OutputModule, path: &'a str) -> Job<'a> { + Job { + project: p, + footage: &NoFootage, + expr: None, + accel: None, + comp: cid, + settings, + output, + path, + sink: None, + nested_switches: true, + options: JobOptions::default(), + } +} + +/// Tests intentionally overwriting an old export use the same review/acceptance API +/// as an interactive host. +fn export_reviewed(job: &Job<'_>, progress: &mut dyn FnMut(&effectcraft_export::Progress) -> bool) -> effectcraft_export::Result { + let reviewed = effectcraft_export::preflight(job)?; + effectcraft_export::export_authorized(job, &reviewed.authorization, progress) +} + /// A comp at `rate`, black, with a white bar 8 px wide sweeping left to right (64 px/s) and a /// static red block at the bottom left; `extra` adds layers. fn project(rate: FrameRate, extra: impl FnOnce(&mut Project, &mut Comp)) -> (Project, ItemId) { @@ -479,7 +502,6 @@ fn a_job_renders_on_one_compositor() { /// Cancellation after an encoder has opened its output must preserve the prior export. #[test] -#[cfg_attr(windows, ignore = "requires enabled SeSecurityPrivilege; exercised by the Windows audit CI runner")] fn cancelled_replacement_preserves_existing_movie_and_sequence() { let dir = std::env::temp_dir().join(format!("ec-preserve-export-{}", std::process::id())); std::fs::create_dir_all(&dir).unwrap(); @@ -489,7 +511,7 @@ fn cancelled_replacement_preserves_existing_movie_and_sequence() { let path = dir.join(if format.is_sequence() { "existing_[#####].png" } else { "existing.mp4" }); let final_path = if format.is_sequence() { dir.join("existing_00000.png") } else { path.clone() }; std::fs::write(&final_path, b"previous complete export").unwrap(); - let result = export( + let result = export_reviewed( &Job { project: &p, footage: &NoFootage, @@ -516,7 +538,6 @@ fn cancelled_replacement_preserves_existing_movie_and_sequence() { /// A complete two-second export matches the sink output byte-for-byte, both for a new /// destination and a replacement. This exercises the real muxer and parallel sequence path. #[test] -#[cfg_attr(windows, ignore = "requires enabled SeSecurityPrivilege; exercised by the Windows audit CI runner")] fn published_movie_and_sequence_match_complete_sink_outputs() { let dir = tempfile::tempdir().unwrap(); let (p, cid) = project(FrameRate::new(25, 1), |_, _| {}); @@ -532,7 +553,7 @@ fn published_movie_and_sequence_match_complete_sink_outputs() { std::fs::write(name, b"previous export").unwrap(); } } - let result = export( + let result = export_reviewed( &Job { project: &p, footage: &NoFootage, @@ -561,9 +582,8 @@ fn published_movie_and_sequence_match_complete_sink_outputs() { } } -/// An I/O failure after a sequence starts must also preserve all earlier complete outputs. +/// An invalid overflow destination must preserve all earlier complete outputs. #[test] -#[cfg_attr(windows, ignore = "requires enabled SeSecurityPrivilege; exercised by the Windows audit CI runner")] fn sequence_write_failure_preserves_existing_frames() { let dir = tempfile::tempdir().unwrap(); let blocked = dir.path().join("blocked-overflow"); @@ -581,7 +601,7 @@ fn sequence_write_failure_preserves_existing_frames() { } let storage = FailAfterFirst(AtomicUsize::new(0)); let (p, cid) = project(FrameRate::new(25, 1), |_, _| {}); - let result = export( + let result = export_reviewed( &Job { project: &p, footage: &NoFootage, @@ -604,3 +624,227 @@ fn sequence_write_failure_preserves_existing_frames() { assert_eq!(std::fs::read(blocked).unwrap(), b"ordinary file, not a directory"); assert!(std::fs::read_dir(dir.path()).unwrap().all(|entry| !entry.unwrap().file_name().to_string_lossy().starts_with(".effectcraft-"))); } + +/// Lose a completed staging file immediately before publication through the real export +/// entry point. Preparation must reject the whole set, including every surviving frame. +#[test] +fn sequence_preparation_failure_publishes_no_frames() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("movie_[#####].png"); + let (p, cid) = project(FrameRate::new(25, 1), |_, _| {}); + let mut injected = false; + let result = export( + &Job { + project: &p, + footage: &NoFootage, + expr: None, + accel: None, + comp: cid, + settings: &RenderSettings::default(), + output: &OutputModule::for_format(OutputFormat::PngSequence), + path: path.to_str().unwrap(), + sink: None, + nested_switches: true, + options: JobOptions::default(), + }, + &mut |progress| { + if progress.done == progress.total && progress.total > 0 { + let mut staging = std::fs::read_dir(dir.path()).unwrap().next().unwrap().unwrap().path(); + if staging.is_dir() { + staging = std::fs::read_dir(staging).unwrap().next().unwrap().unwrap().path(); + } + std::fs::remove_file(staging).unwrap(); + injected = true; + } + true + }, + ); + assert!(injected); + assert!(matches!(result, Err(effectcraft_export::ExportError::Io(_))), "{result:?}"); + // Failure logs have their separate, existing delivery contract. No frame or staging + // file may remain; the only entry is the useful error log. + let log = std::path::PathBuf::from(log_path(path.to_str().unwrap())); + assert!(std::fs::read_to_string(&log).unwrap().contains("before publication")); + let entries: Vec<_> = std::fs::read_dir(dir.path()).unwrap().map(|entry| entry.unwrap().path()).collect(); + assert_eq!(entries, vec![log]); +} + +#[test] +fn preflight_is_read_only_and_authorized_new_movie_matches_sink() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("missing").join("subfolder").join("movie.mov"); + let (p, cid) = project(FrameRate::new(25, 1), |_, _| {}); + let settings = custom(0.0, 0.08); + let output = OutputModule::for_format(OutputFormat::Uncompressed); + let job = disk_job(&p, cid, &settings, &output, path.to_str().unwrap()); + let reviewed = effectcraft_export::preflight(&job).unwrap(); + assert!(reviewed.collisions.is_empty()); + assert!(!path.parent().unwrap().exists(), "inspection must not create folders"); + let expected = run(&p, cid, &settings, &output, job.path); + effectcraft_export::export_authorized(&job, &reviewed.authorization, &mut |_| true).unwrap(); + for (name, bytes) in expected.lock().unwrap().iter() { + assert_eq!(&std::fs::read(name).unwrap(), bytes); + } +} + +#[test] +fn existing_output_requires_confirmation_and_stale_review_preserves_external_bytes() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("movie.mov"); + std::fs::write(&path, b"old complete movie").unwrap(); + let (p, cid) = project(FrameRate::new(25, 1), |_, _| {}); + let settings = custom(0.0, 0.08); + let output = OutputModule::for_format(OutputFormat::Uncompressed); + let job = disk_job(&p, cid, &settings, &output, path.to_str().unwrap()); + let reviewed = effectcraft_export::preflight(&job).unwrap(); + assert_eq!(reviewed.collisions, vec![job.path]); + let mut progressed = false; + let error = export(&job, &mut |_| { + progressed = true; + true + }) + .unwrap_err() + .to_string(); + assert!(error.contains("confirmation required"), "{error}"); + assert!(!progressed); + assert_eq!(std::fs::read(&path).unwrap(), b"old complete movie"); + std::fs::write(&path, b"external writer changed this after review").unwrap(); + let error = effectcraft_export::export_authorized(&job, &reviewed.authorization, &mut |_| true).unwrap_err().to_string(); + assert!(error.contains("changed since preflight"), "{error}"); + assert_eq!(std::fs::read(&path).unwrap(), b"external writer changed this after review"); + assert_eq!(std::fs::read_dir(dir.path()).unwrap().count(), 1); +} + +#[test] +fn file_appearing_after_job_validation_is_not_an_approved_overwrite() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("movie_[#####].png"); + let external = dir.path().join("movie_00000.png"); + let (p, cid) = project(FrameRate::new(25, 1), |_, _| {}); + let settings = custom(0.0, 0.08); + let output = OutputModule::for_format(OutputFormat::PngSequence); + let job = disk_job(&p, cid, &settings, &output, path.to_str().unwrap()); + let reviewed = effectcraft_export::preflight(&job).unwrap(); + let mut injected = false; + let error = effectcraft_export::export_authorized(&job, &reviewed.authorization, &mut |progress| { + if progress.done == 0 && !injected { + // export_inner has checked every target; the first staging open has not run. + std::fs::write(&external, b"external creator").unwrap(); + injected = true; + } + true + }) + .unwrap_err() + .to_string(); + assert!(injected); + assert!(error.contains("changed since preflight"), "{error}"); + assert_eq!(std::fs::read(&external).unwrap(), b"external creator"); + assert!(!dir.path().join("movie_00001.png").exists()); + assert!(std::fs::read_dir(dir.path()).unwrap().all(|entry| !entry.unwrap().file_name().to_string_lossy().starts_with(".effectcraft-"))); +} + +#[test] +fn authorization_cannot_be_reused_for_an_unreviewed_path_or_replaced_by_a_boolean() { + let dir = tempfile::tempdir().unwrap(); + let a = dir.path().join("a.mov"); + let b = dir.path().join("unreviewed").join("b.mov"); + let (p, cid) = project(FrameRate::new(25, 1), |_, _| {}); + let settings = custom(0.0, 0.08); + let output = OutputModule::for_format(OutputFormat::Uncompressed); + let a_job = disk_job(&p, cid, &settings, &output, a.to_str().unwrap()); + let b_job = disk_job(&p, cid, &settings, &output, b.to_str().unwrap()); + let reviewed = effectcraft_export::preflight(&a_job).unwrap(); + let error = effectcraft_export::export_authorized(&b_job, &reviewed.authorization, &mut |_| true).unwrap_err().to_string(); + assert!(error.contains("was not reviewed"), "{error}"); + let error = effectcraft_export::export_authorized(&a_job, &serde_json::json!(true), &mut |_| true).unwrap_err().to_string(); + assert!(error.contains("invalid export authorization"), "{error}"); + assert_eq!(std::fs::read_dir(dir.path()).unwrap().count(), 0); +} + +#[test] +fn skip_existing_does_not_hide_an_overflow_replacement() { + struct Overflow; + impl StorageQuota for Overflow { + fn has_room(&self, path: &str, _: u64) -> bool { + path.contains("overflow") + } + } + let dir = tempfile::tempdir().unwrap(); + let overflow = dir.path().join("overflow"); + std::fs::create_dir(&overflow).unwrap(); + let path = dir.path().join("movie_[#####].png"); + let existing = overflow.join("movie_00000.png"); + std::fs::write(&existing, b"old overflow frame").unwrap(); + let (p, cid) = project(FrameRate::new(25, 1), |_, _| {}); + let settings = RenderSettings { skip_existing: true, storage_overflow: true, ..custom(0.0, 0.04) }; + let output = OutputModule::for_format(OutputFormat::PngSequence); + let mut job = disk_job(&p, cid, &settings, &output, path.to_str().unwrap()); + job.options.storage = Some(&Overflow); + job.options.overflow = vec![overflow.to_str().unwrap().to_string()]; + let reviewed = effectcraft_export::preflight(&job).unwrap(); + assert_eq!(reviewed.collisions, vec![existing.to_str().unwrap()]); + assert!(export(&job, &mut |_| true).unwrap_err().to_string().contains("confirmation required")); + let base = dir.path().join("movie_00000.png"); + std::fs::write(&base, b"already rendered").unwrap(); + let permissions = std::fs::metadata(&base).unwrap().permissions(); + let mut readonly = permissions.clone(); + readonly.set_readonly(true); + std::fs::set_permissions(&base, readonly).unwrap(); + let reviewed = effectcraft_export::preflight(&job).unwrap(); + assert!(reviewed.collisions.is_empty()); + assert_eq!(export(&job, &mut |_| true).unwrap().bytes, 0); + std::fs::set_permissions(&base, permissions).unwrap(); + assert_eq!(std::fs::read(existing).unwrap(), b"old overflow frame"); +} + +#[test] +fn render_log_collision_preserves_existing_file_and_returns_unique_paths() { + for hardlink in [false, true] { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("movie.mov"); + let wanted = std::path::PathBuf::from(log_path(path.to_str().unwrap())); + let unrelated = dir.path().join("unrelated-project.ecproj"); + std::fs::write(&unrelated, b"unrelated project bytes").unwrap(); + if hardlink { + std::fs::hard_link(&unrelated, &wanted).unwrap(); + } else { + std::fs::write(&wanted, b"previous render log").unwrap(); + } + let original = std::fs::read(&wanted).unwrap(); + let (p, cid) = project(FrameRate::new(25, 1), |_, _| {}); + let settings = custom(0.0, 0.08); + let output = OutputModule::for_format(OutputFormat::Uncompressed); + let mut job = disk_job(&p, cid, &settings, &output, path.to_str().unwrap()); + job.options.log = RenderLog::PlusSettings; + let first = export(&job, &mut |_| true).unwrap().log.unwrap(); + assert_eq!(std::fs::read(&wanted).unwrap(), original); + assert_eq!(std::fs::read(&unrelated).unwrap(), b"unrelated project bytes"); + assert_ne!(std::path::Path::new(&first), wanted); + let first_bytes = std::fs::read(&first).unwrap(); + std::fs::remove_file(&path).unwrap(); // Only this fixture's newly exported movie. + let second = export(&job, &mut |_| true).unwrap().log.unwrap(); + assert_ne!(first, second); + assert_eq!(std::fs::read(first).unwrap(), first_bytes); + assert_eq!(std::fs::read(wanted).unwrap(), original); + } +} + +#[cfg(unix)] +#[test] +fn render_log_symlink_does_not_follow_or_truncate_an_unrelated_target() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("movie.mov"); + let wanted = std::path::PathBuf::from(log_path(path.to_str().unwrap())); + let unrelated = dir.path().join("unrelated-project.ecproj"); + std::fs::write(&unrelated, b"unrelated project bytes").unwrap(); + std::os::unix::fs::symlink(&unrelated, &wanted).unwrap(); + let (p, cid) = project(FrameRate::new(25, 1), |_, _| {}); + let settings = custom(0.0, 0.08); + let output = OutputModule::for_format(OutputFormat::Uncompressed); + let mut job = disk_job(&p, cid, &settings, &output, path.to_str().unwrap()); + job.options.log = RenderLog::PlusSettings; + let actual = export(&job, &mut |_| true).unwrap().log.unwrap(); + assert_ne!(std::path::Path::new(&actual), wanted); + assert_eq!(std::fs::read_link(&wanted).unwrap(), unrelated); + assert_eq!(std::fs::read(&wanted).unwrap(), b"unrelated project bytes"); +} diff --git a/crates/host/Cargo.toml b/crates/host/Cargo.toml index 80f61e4c3..74b196a1e 100644 --- a/crates/host/Cargo.toml +++ b/crates/host/Cargo.toml @@ -13,6 +13,7 @@ workspace = true effectcraft-engine = { workspace = true } effectcraft-project = { workspace = true } log = { workspace = true } +serde_json = { workspace = true } effectcraft-media = { workspace = true } effectcraft-expr = { workspace = true } effectcraft-script = { workspace = true } @@ -24,7 +25,6 @@ effectcraft-plugin = { workspace = true } effectcraft-plugin = { workspace = true, features = ["wasm"] } [dev-dependencies] -serde_json = { workspace = true } effectcraft-psd = { workspace = true } effectcraft-svg = { workspace = true } effectcraft-pdf = { workspace = true } diff --git a/crates/host/src/lib.rs b/crates/host/src/lib.rs index bdb4e2bc3..687edbf65 100644 --- a/crates/host/src/lib.rs +++ b/crates/host/src/lib.rs @@ -29,39 +29,42 @@ pub struct FileExporter { pub sink: Option>, } -impl Exporter for FileExporter { - fn formats(&self) -> Vec { - effectcraft_export::available_formats() - } - fn export(&self, job: &ExportJob, progress: &mut dyn FnMut(u64, u64) -> bool) -> Result { - effectcraft_engine::render::passes::block_on(self.export_async(job, progress)) +impl FileExporter { + fn job<'a>(&'a self, job: &'a ExportJob<'a>) -> effectcraft_export::Job<'a> { + effectcraft_export::Job { + project: job.project, + footage: job.footage, + expr: job.expr, + accel: job.accel, + comp: job.item.comp, + settings: &job.item.settings, + output: &job.item.output, + path: job.path, + sink: self.sink.as_deref(), + options: effectcraft_export::JobOptions { + log: job.item.log, + label: job.label.clone(), + storage: job.storage, + overflow: job.project.render_prefs.overflow_folders.clone(), + }, + nested_switches: job.nested_switches, + } } - fn export_async<'a>( + fn run<'a>( &'a self, job: &'a ExportJob<'a>, + authorization: Option<&'a serde_json::Value>, progress: &'a mut dyn FnMut(u64, u64) -> bool, ) -> effectcraft_engine::render_queue::LocalFuture<'a, Result> { Box::pin(async move { - let j = effectcraft_export::Job { - project: job.project, - footage: job.footage, - expr: job.expr, - accel: job.accel, - comp: job.item.comp, - settings: &job.item.settings, - output: &job.item.output, - path: job.path, - sink: self.sink.as_deref(), - options: effectcraft_export::JobOptions { - log: job.item.log, - label: job.label.clone(), - storage: job.storage, - overflow: job.project.render_prefs.overflow_folders.clone(), - }, - nested_switches: job.nested_switches, + let j = self.job(job); + let mut report = |p: &effectcraft_export::Progress| progress(p.done, p.total); + let result = match authorization { + Some(token) => effectcraft_export::export_authorized_async(&j, token, &mut report).await, + None => effectcraft_export::export_async(&j, &mut report).await, }; - match effectcraft_export::export_async(&j, &mut |p| progress(p.done, p.total)).await { + match result { Ok(r) => Ok(ExportResult { path: r.path, frames: r.frames, @@ -80,6 +83,46 @@ impl Exporter for FileExporter { } } +impl Exporter for FileExporter { + fn formats(&self) -> Vec { + effectcraft_export::available_formats() + } + fn preflight(&self, job: &ExportJob) -> Result { + let review = effectcraft_export::preflight(&self.job(job)).map_err(|e| e.to_string())?; + Ok(effectcraft_engine::render_queue::ExportPreflight { + collisions: review.collisions, + destinations: review.destinations, + authorization: review.authorization, + }) + } + fn export(&self, job: &ExportJob, progress: &mut dyn FnMut(u64, u64) -> bool) -> Result { + effectcraft_engine::render::passes::block_on(self.run(job, None, progress)) + } + fn export_async<'a>( + &'a self, + job: &'a ExportJob<'a>, + progress: &'a mut dyn FnMut(u64, u64) -> bool, + ) -> effectcraft_engine::render_queue::LocalFuture<'a, Result> { + self.run(job, None, progress) + } + fn export_authorized( + &self, + job: &ExportJob, + authorization: &serde_json::Value, + progress: &mut dyn FnMut(u64, u64) -> bool, + ) -> Result { + effectcraft_engine::render::passes::block_on(self.run(job, Some(authorization), progress)) + } + fn export_authorized_async<'a>( + &'a self, + job: &'a ExportJob<'a>, + authorization: &'a serde_json::Value, + progress: &'a mut dyn FnMut(u64, u64) -> bool, + ) -> effectcraft_engine::render_queue::LocalFuture<'a, Result> { + self.run(job, Some(authorization), progress) + } +} + /// A new session with media, import, expressions, scripting and export enabled. pub fn session() -> Session { Session { diff --git a/crates/host/tests/overwrite_confirmation.rs b/crates/host/tests/overwrite_confirmation.rs new file mode 100644 index 000000000..4be64bbc2 --- /dev/null +++ b/crates/host/tests/overwrite_confirmation.rs @@ -0,0 +1,147 @@ +//! Headless real-export regressions for the engine's one-shot output review. + +use effectcraft_engine::Session; +use serde_json::{Value, json}; +use std::path::PathBuf; +use std::sync::atomic::{AtomicU64, Ordering}; + +struct Fixture(PathBuf); +impl Fixture { + fn new() -> Self { + static NEXT: AtomicU64 = AtomicU64::new(0); + let dir = std::env::temp_dir().join(format!("effectcraft-overwrite-{}-{}", std::process::id(), NEXT.fetch_add(1, Ordering::Relaxed))); + std::fs::create_dir(&dir).unwrap(); + Self(dir) + } + fn path(&self, name: &str) -> String { + self.0.join(name).to_string_lossy().into_owned() + } + fn write(&self, name: &str, bytes: &[u8]) { + std::fs::write(self.0.join(name), bytes).unwrap(); + } + fn read(&self, name: &str) -> Vec { + std::fs::read(self.0.join(name)).unwrap() + } +} +impl Drop for Fixture { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.0); + } +} + +fn session() -> Session { + let mut s = effectcraft_host::session(); + s.execute("comp.new", json!({"name": "Overwrite", "width": 4, "height": 4, "frameRate": 10, "duration": 0.2})).unwrap(); + s +} + +fn accept(s: &mut Session, review: &Value) { + let result = s.execute_checked("renderQueue.render", json!({"wait": true, "confirmation": review["confirmation"], "overwrite": true})).unwrap(); + for item in result["items"].as_array().unwrap() { + assert_eq!(item["statusLabel"], "Done", "{item}"); + } +} + +#[test] +fn same_filename_and_stored_queue_rerender_require_fresh_acceptance() { + let f = Fixture::new(); + f.write("movie.gif", b"old movie"); + let mut s = session(); + s.execute("renderQueue.add", json!({"format": "gif", "output": f.path("movie.gif")})).unwrap(); + assert!(s.execute("renderQueue.render", json!({})).is_err()); + let review = s.execute("renderQueue.preflight", json!({})).unwrap(); + assert_eq!(review["collisions"], json!([f.path("movie.gif")])); + assert_eq!(f.read("movie.gif"), b"old movie"); + s.execute("renderQueue.cancelPreflight", json!({"confirmation": review["confirmation"]})).unwrap(); + assert!(s.execute("renderQueue.render", json!({"confirmation": review["confirmation"], "overwrite": true})).is_err()); + assert_eq!(f.read("movie.gif"), b"old movie"); + let review = s.execute("renderQueue.preflight", json!({})).unwrap(); + accept(&mut s, &review); + let rendered = f.read("movie.gif"); + assert!(rendered.starts_with(b"GIF89a")); + s.execute("renderQueue.setRender", json!({"render": true})).unwrap(); + assert!(s.execute("renderQueue.render", json!({})).is_err()); + assert_eq!(f.read("movie.gif"), rendered); + let review = s.execute("renderQueue.preflight", json!({})).unwrap(); + accept(&mut s, &review); +} + +#[test] +fn sequence_review_covers_exact_frames_and_keeps_old_tail_and_unrelated_files() { + let f = Fixture::new(); + for name in ["seq_00.png", "seq_01.png", "seq_02.png", "unrelated.png"] { + f.write(name, name.as_bytes()); + } + let mut s = session(); + s.execute("renderQueue.add", json!({"format": "png", "output": f.path("seq_[##].png")})).unwrap(); + let review = s.execute("renderQueue.preflight", json!({})).unwrap(); + assert_eq!(review["collisions"], json!([f.path("seq_00.png"), f.path("seq_01.png")])); + s.execute("renderQueue.cancelPreflight", json!({"confirmation": review["confirmation"]})).unwrap(); + assert_eq!(f.read("seq_00.png"), b"seq_00.png"); + assert_eq!(f.read("seq_01.png"), b"seq_01.png"); + let review = s.execute("renderQueue.preflight", json!({})).unwrap(); + accept(&mut s, &review); + for name in ["seq_00.png", "seq_01.png"] { + assert!(f.read(name).starts_with(b"\x89PNG")); + } + assert_eq!(f.read("seq_02.png"), b"seq_02.png"); + assert_eq!(f.read("unrelated.png"), b"unrelated.png"); +} + +#[test] +fn fresh_destination_needs_no_acceptance_and_late_files_are_not_approved() { + let f = Fixture::new(); + let mut s = session(); + s.execute("renderQueue.add", json!({"format": "png", "output": f.path("seq_[##].png")})).unwrap(); + let review = s.execute("renderQueue.preflight", json!({})).unwrap(); + assert_eq!(review["collisions"], json!([])); + f.write("seq_01.png", b"appeared after review"); + let r = s.execute("renderQueue.render", json!({"wait": true, "confirmation": review["confirmation"]})).unwrap(); + assert_eq!(r["items"][0]["statusLabel"], "Failed"); + assert_eq!(f.read("seq_01.png"), b"appeared after review"); + assert!(!f.0.join("seq_00.png").exists(), "the backend refuses before publishing any frame"); + s.execute("renderQueue.setOutput", json!({"path": f.path("fresh_[##].png")})).unwrap(); + s.execute("renderQueue.setRender", json!({"render": true})).unwrap(); + let r = s.execute("renderQueue.render", json!({"wait": true})).unwrap(); + assert_eq!(r["items"][0]["statusLabel"], "Done", "{r}"); + assert!(f.read("fresh_00.png").starts_with(b"\x89PNG")); +} + +#[test] +fn cancelled_confirmed_sequence_preserves_previous_bytes() { + use effectcraft_engine::project::render_queue::OutputFormat; + use effectcraft_engine::render_queue::{ExportPreflight, LocalFuture}; + use effectcraft_engine::{ExportJob, ExportResult, Exporter}; + struct Cancel(effectcraft_host::FileExporter); + impl Exporter for Cancel { + fn formats(&self) -> Vec { + self.0.formats() + } + fn preflight(&self, job: &ExportJob) -> Result { + self.0.preflight(job) + } + fn export(&self, job: &ExportJob, _: &mut dyn FnMut(u64, u64) -> bool) -> Result { + self.0.export(job, &mut |_, _| false) + } + fn export_authorized_async<'a>( + &'a self, + job: &'a ExportJob<'a>, + authorization: &'a Value, + _: &'a mut dyn FnMut(u64, u64) -> bool, + ) -> LocalFuture<'a, Result> { + Box::pin(async move { self.0.export_authorized(job, authorization, &mut |_, _| false) }) + } + } + let f = Fixture::new(); + f.write("seq_00.png", b"old frame zero"); + f.write("seq_01.png", b"old frame one"); + let mut s = session(); + s.exporter = Some(std::sync::Arc::new(Cancel(effectcraft_host::FileExporter::default()))); + s.execute("renderQueue.add", json!({"format": "png", "output": f.path("seq_[##].png")})).unwrap(); + let review = s.execute("renderQueue.preflight", json!({})).unwrap(); + let r = s.execute("renderQueue.render", json!({"confirmation": review["confirmation"], "overwrite": true})).unwrap(); + assert_eq!(r["items"][0]["statusLabel"], "User Stopped", "{r}"); + assert_eq!(f.read("seq_00.png"), b"old frame zero"); + assert_eq!(f.read("seq_01.png"), b"old frame one"); + assert_eq!(std::fs::read_dir(&f.0).unwrap().count(), 2, "cancelled staging is cleaned up"); +} diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 72016cc53..7bcf28196 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -95,6 +95,8 @@ pub enum Dialog { UnsavedChanges, /// Delete Project items that compositions use? (`panels::delete_items`). DeleteItems, + /// Replace existing render outputs after reviewing the resolved destinations. + OverwriteRender, /// A gradient property's Gradient Editor (`panels::gradient_editor`, `prop.editGradient`). GradientEditor, /// Delete a user template from Home? (`panels::templates`). diff --git a/crates/ui-egui/src/menus.rs b/crates/ui-egui/src/menus.rs index 92893dcb6..3f9d8a3ab 100644 --- a/crates/ui-egui/src/menus.rs +++ b/crates/ui-egui/src/menus.rs @@ -772,16 +772,8 @@ pub fn invoke(app: &mut EffectcraftApp, ctx: &egui::Context, id: &str, params: V "keys.interpolation" if no_params(¶ms) => crate::panels::key_dialogs::open_interpolation(app)?, "layer.timeStretch" if no_params(¶ms) => crate::panels::key_dialogs::open_time_stretch(app)?, _ => { - if id == "renderQueue.render" && params.get("wait").is_none() { - // The UI renders in the background; the panel shows progress. - let mut p = params.clone(); - if let Some(m) = p.as_object_mut() { - m.insert("wait".into(), json!(false)); - } else { - p = json!({"wait": false}); - } - app.show_panel(PanelKind::RenderQueue); - return app.session.execute(id, p).map_err(|e| e.to_string()); + if id == "renderQueue.render" { + return crate::panels::overwrite_render::request(app, ctx, params); } if id == "renderQueue.add" { let r = app.session.execute(id, params).map_err(|e| e.to_string()); @@ -801,17 +793,6 @@ pub fn invoke(app: &mut EffectcraftApp, ctx: &egui::Context, id: &str, params: V if crate::panels::dialogs::open_form(app, id, ¶ms) { return Ok(json!({"dialog": id})); } - if id == "renderQueue.render" && params.get("wait").is_none() { - // The UI renders in the background; the panel shows progress. - let mut p = params.clone(); - if let Some(m) = p.as_object_mut() { - m.insert("wait".into(), json!(false)); - } else { - p = json!({"wait": false}); - } - app.show_panel(PanelKind::RenderQueue); - return app.session.execute(id, p).map_err(|e| e.to_string()); - } if id == "renderQueue.add" { let r = app.session.execute(id, params).map_err(|e| e.to_string()); match &r { diff --git a/crates/ui-egui/src/panels/dialogs.rs b/crates/ui-egui/src/panels/dialogs.rs index d773fc433..a474bd426 100644 --- a/crates/ui-egui/src/panels/dialogs.rs +++ b/crates/ui-egui/src/panels/dialogs.rs @@ -63,6 +63,8 @@ pub struct DialogState { pub unsaved: super::unsaved::Pending, /// Deleting Project items in use: the deletion waiting on the prompt. pub delete_items: super::delete_items::Pending, + /// The render request and destination review waiting for an answer. + pub overwrite_render: Option<(Value, Value)>, /// The Gradient Editor's draft and the property it edits. pub gradient: super::gradient_editor::Draft, /// The user template the Delete Template prompt asks about: its id and name. @@ -260,6 +262,7 @@ pub fn show(app: &mut EffectcraftApp, ctx: &egui::Context) { Dialog::RenderTemplates => super::rq_templates::show(app, ctx, &t), Dialog::UnsavedChanges => super::unsaved::show(app, ctx, &t), Dialog::DeleteItems => super::delete_items::show(app, ctx, &t), + Dialog::OverwriteRender => super::overwrite_render::show(app, ctx, &t), Dialog::GradientEditor => super::gradient_editor::show(app, ctx, &t), Dialog::DeleteTemplate => super::templates::show_delete(app, ctx, &t), } diff --git a/crates/ui-egui/src/panels/mod.rs b/crates/ui-egui/src/panels/mod.rs index 3aa9a19c5..974cc7130 100644 --- a/crates/ui-egui/src/panels/mod.rs +++ b/crates/ui-egui/src/panels/mod.rs @@ -31,6 +31,7 @@ pub mod markers_ui; pub mod media_panels; pub mod misc; pub mod object_matte_tool; +pub mod overwrite_render; pub mod paint_panels; pub mod panel_kit; pub mod path_vr_panels; diff --git a/crates/ui-egui/src/panels/overwrite_render.rs b/crates/ui-egui/src/panels/overwrite_render.rs new file mode 100644 index 000000000..8207894de --- /dev/null +++ b/crates/ui-egui/src/panels/overwrite_render.rs @@ -0,0 +1,195 @@ +//! One confirmation for the queue's existing output files, including sequence frames. + +use crate::i18n::tr; +use crate::theme::Tokens; +use crate::{Dialog, EffectcraftApp}; +use egui::vec2; +use serde_json::{Value, json}; + +/// All UI render entry points come here, including a stored queue's Render button. +pub fn request(app: &mut EffectcraftApp, ctx: &egui::Context, mut params: Value) -> Result { + if app.dialog == Some(Dialog::OverwriteRender) { + return Ok(json!({"dialog": "overwriteRender"})); + } + if !params.is_object() { + params = json!({}); + } + if params.get("wait").is_none() { + params["wait"] = json!(false); + } + // UI callers cannot turn a previous answer into a standing overwrite preference. + if let Some(p) = params.as_object_mut() { + p.remove("overwrite"); + p.remove("confirmation"); + } + let review = crate::menus::run_engine(app, ctx, "renderQueue.preflight", json!({}))?; + params["confirmation"] = review["confirmation"].clone(); + app.show_panel(crate::dock::PanelKind::RenderQueue); + if review["collisions"].as_array().is_some_and(|p| !p.is_empty()) { + app.dialog_state.overwrite_render = Some((params, review)); + app.dialog = Some(Dialog::OverwriteRender); + Ok(json!({"dialog": "overwriteRender"})) + } else { + crate::menus::run_engine(app, ctx, "renderQueue.render", params) + } +} + +fn answer(app: &mut EffectcraftApp, ctx: &egui::Context, replace: bool) -> Result { + let Some((mut params, _)) = app.dialog_state.overwrite_render.take() else { return Err("no render is awaiting confirmation".into()) }; + app.dialog = None; + if replace { + params["overwrite"] = json!(true); + crate::menus::run_engine(app, ctx, "renderQueue.render", params) + } else { + crate::menus::run_engine(app, ctx, "renderQueue.cancelPreflight", json!({"confirmation": params["confirmation"]})) + } +} + +pub fn show(app: &mut EffectcraftApp, ctx: &egui::Context, t: &Tokens) { + let Some(pending) = app.dialog_state.overwrite_render.take() else { + app.dialog = None; + return; + }; + let paths = pending.1["collisions"].as_array().map(Vec::as_slice).unwrap_or_default(); + let mut replace = None; + super::dialogs::modal(ctx, "Replace existing render outputs?", vec2(580.0, 340.0), t, |ui| { + let count = paths.len(); + ui.label(format!("This render may replace {} existing {}.", count, if count == 1 { "file" } else { "files" })); + ui.add_space(8.0); + egui::ScrollArea::both().max_height(170.0).show_rows(ui, 20.0, paths.len(), |ui, rows| { + for path in paths.get(rows).unwrap_or_default() { + let path = path.as_str().unwrap_or_default(); + let name = path.rsplit(['/', '\\']).next().filter(|name| !name.is_empty()).unwrap_or(path); + ui.add(egui::Label::new(format!("{name} — {path}")).wrap_mode(egui::TextWrapMode::Extend)).on_hover_text(path); + } + }); + ui.add_space(8.0); + ui.label(egui::RichText::new(tr("Sequence frames beyond this render's range are kept.")).color(t.text_dim)); + ui.add_space(12.0); + ui.with_layout(egui::Layout::right_to_left(egui::Align::Center), |ui| { + let r = ui.button(tr("Replace and Render")); + app.auto.add_response("dialog.overwriteRender.replace", &r, "Replace and Render"); + if r.clicked() { + replace = Some(true); + } + let r = ui.button(tr("Cancel")); + app.auto.add_response("dialog.overwriteRender.cancel", &r, "Cancel"); + if r.clicked() { + replace = Some(false); + } + }); + }); + if ctx.input(|i| i.key_pressed(egui::Key::Escape)) { + replace = Some(false); + } + app.dialog_state.overwrite_render = Some(pending); + if let Some(replace) = replace { + let _ = answer(app, ctx, replace); + } +} + +#[cfg(test)] +mod tests { + use super::*; + use effectcraft_engine::project::render_queue::OutputFormat; + use effectcraft_engine::render_queue::{ExportPreflight, LocalFuture}; + use effectcraft_engine::{ExportJob, ExportResult, Exporter, Session}; + use std::sync::{Arc, Mutex}; + + struct Output { + collisions: Vec, + calls: Mutex, + } + impl Exporter for Output { + fn formats(&self) -> Vec { + vec![OutputFormat::PngSequence] + } + fn preflight(&self, _: &ExportJob) -> Result { + Ok(ExportPreflight { collisions: self.collisions.clone(), destinations: vec![], authorization: json!({"reviewed": true}) }) + } + fn export(&self, _: &ExportJob, _: &mut dyn FnMut(u64, u64) -> bool) -> Result { + Err("unreviewed export".into()) + } + fn export_authorized_async<'a>( + &'a self, + job: &'a ExportJob<'a>, + token: &'a Value, + _: &'a mut dyn FnMut(u64, u64) -> bool, + ) -> LocalFuture<'a, Result> { + Box::pin(async move { + assert_eq!(token["reviewed"], true); + *self.calls.lock().unwrap() += 1; + Ok(ExportResult { path: job.path.into(), ..Default::default() }) + }) + } + } + + fn setup(existing: bool) -> (EffectcraftApp, egui::Context, Arc) { + let ex = Arc::new(Output { collisions: if existing { vec!["seq_00.png".into(), "seq_01.png".into()] } else { vec![] }, calls: Mutex::new(0) }); + let mut session = Session { exporter: Some(ex.clone()), ..Default::default() }; + session.execute("comp.new", json!({"width": 4, "height": 4, "duration": 0.2})).unwrap(); + session.execute("renderQueue.add", json!({"format": "png", "output": "seq_[##].png"})).unwrap(); + let app = EffectcraftApp::new(session); + let ctx = egui::Context::default(); + crate::theme::install(&ctx, &app.tokens, crate::i18n::language(&app)); + (app, ctx, ex) + } + + fn click(app: &mut EffectcraftApp, ctx: &egui::Context, id: &str) { + let frame = |app: &mut EffectcraftApp, events: Vec| { + let raw = egui::RawInput { screen_rect: Some(egui::Rect::from_min_size(egui::Pos2::ZERO, vec2(1024.0, 768.0))), events, ..Default::default() }; + let _ = ctx.run_ui(raw, |ui| { + app.auto.begin_frame(); + super::super::dialogs::show(app, ui.ctx()); + }); + }; + for _ in 0..3 { + frame(app, vec![]); + } + let [x, y, w, h] = app.auto.find(id).unwrap().rect; + let pos = egui::pos2(x + w * 0.5, y + h * 0.5); + for pressed in [true, false] { + frame( + app, + vec![ + egui::Event::PointerMoved(pos), + egui::Event::PointerButton { pos, button: egui::PointerButton::Primary, pressed, modifiers: egui::Modifiers::NONE }, + ], + ); + } + } + + #[test] + fn stored_queue_sequence_asks_once_and_cancel_does_not_export() { + let (mut app, ctx, ex) = setup(true); + // Same path as the real Render button, through the menu dispatcher with wait supplied. + crate::menus::invoke(&mut app, &ctx, "renderQueue.render", json!({"wait": true})).unwrap(); + assert_eq!(app.dialog, Some(Dialog::OverwriteRender)); + assert_eq!(*ex.calls.lock().unwrap(), 0); + let id = app.dialog_state.overwrite_render.as_ref().unwrap().1["confirmation"].clone(); + crate::menus::invoke(&mut app, &ctx, "renderQueue.render", json!({"wait": true})).unwrap(); + assert_eq!(app.dialog_state.overwrite_render.as_ref().unwrap().1["confirmation"], id, "repeat clicks keep one review"); + super::super::dialogs::press_key(&mut app, &ctx, egui::Key::Escape, egui::Modifiers::NONE); + assert!(app.dialog.is_none()); + assert_eq!(*ex.calls.lock().unwrap(), 0); + assert!(app.session.execute("renderQueue.render", json!({"confirmation": id, "overwrite": true})).is_err()); + crate::menus::invoke(&mut app, &ctx, "renderQueue.render", json!({"wait": true})).unwrap(); + // Enter is deliberately not a destructive answer. + super::super::dialogs::press_key(&mut app, &ctx, egui::Key::Enter, egui::Modifiers::NONE); + assert_eq!(app.dialog, Some(Dialog::OverwriteRender)); + click(&mut app, &ctx, "dialog.overwriteRender.replace"); + assert!(app.dialog.is_none()); + assert_eq!(*ex.calls.lock().unwrap(), 1); + app.session.execute("renderQueue.setRender", json!({"render": true})).unwrap(); + crate::menus::invoke(&mut app, &ctx, "renderQueue.render", json!({"wait": true})).unwrap(); + assert_eq!(app.dialog, Some(Dialog::OverwriteRender), "a saved queue rerender asks again"); + } + + #[test] + fn fresh_destination_renders_without_a_dialog() { + let (mut app, ctx, ex) = setup(false); + crate::menus::invoke(&mut app, &ctx, "renderQueue.render", json!({"wait": true})).unwrap(); + assert!(app.dialog.is_none()); + assert_eq!(*ex.calls.lock().unwrap(), 1); + } +} diff --git a/docs/control-protocol.md b/docs/control-protocol.md index 4d01ef2e9..cbb86df35 100644 --- a/docs/control-protocol.md +++ b/docs/control-protocol.md @@ -53,6 +53,31 @@ export formats, parity summary), `jobs.list` (Progress panel jobs, e.g. the back Every engine command is reachable through `engine.execute`, and every interactive widget of every panel registers an element id; both are enforced by `crates/ui-egui/tests/ui_agent_audit.rs`. +### Existing render outputs + +`engine.execute` stays headless: rendering a fresh destination needs no extra params. Existing +outputs require an explicit review and acceptance in the same session: + +1. Run `renderQueue.preflight {}`. It returns `{confirmation, collisions: [path]}` with the exact + existing movie/audio files or sequence frame paths (including configured overflow alternatives). + It creates no files and starts no render. +2. Review those paths, then run `renderQueue.render {confirmation: , overwrite: true}`. + Or decline with `renderQueue.cancelPreflight {confirmation: }`. + +A review is consumed once. Project/queue/output changes invalidate it; destination files that +appear or change after review are refused by the exporter. The engine holds the authorization +snapshot privately. Supplying `overwrite: true` alone is an error. A preflight with no collisions +can be rendered with its confirmation id and no overwrite flag. + +`ui.menu.invoke {id: "renderQueue.render"}` and the Render panel show one **Replace and Render / +Cancel** dialog for existing destinations. Escape cancels; Enter does not accept replacement. +Automation ids: `dialog.overwriteRender.replace`, `dialog.overwriteRender.cancel`. +Duplicate resolved destination names in the queue are rejected before rendering; the exporter's +destination validation also protects files that appear or change after the review. +Other files and sequence frames beyond the exported range are kept. The headless CLI accepts +`render --overwrite` (also with `--queue`); this explicitly reviews and accepts this invocation's +current destinations. Without it, existing outputs produce an actionable error. + ## UI state | Method | Params | Result | diff --git a/docs/export-output-safety.md b/docs/export-output-safety.md index 73618f647..642869615 100644 --- a/docs/export-output-safety.md +++ b/docs/export-output-safety.md @@ -1,86 +1,38 @@ -# Export output lifetime - -Disk exports encode into job-owned staging on the destination filesystem. Existing -outputs are untouched until all encoding succeeds. A cancelled, failed, or abandoned -job removes its staging, never a final output selected by its filename. Browser -sinks retain per-file delivery, and render logs retain their separate behavior. - -Publication replaces one complete file at a time. A sequence is **not** an atomic -transaction: if publication of a later frame fails, previously published complete -frames remain. Staging needs enough free space for the new export alongside the -old export. This provides error/cancellation safety, not a power-loss durability -guarantee or automatic recovery of staging left by a terminated process. - -## Access policy - -Before receiving media, staging is restricted to its owner. An empty, separate -probe checks that the intended output policy can be reproduced. That probe never -receives media, so a reader retaining its handle cannot later read export bytes. -New outputs use ordinary creation permissions, including umask and directory -inheritance. Existing outputs retain owner, group, mode/access controls supported -by the platform adapter; errors preserve the old output instead of weakening access. - -- Windows creates staging with a protected, empty DACL, then grants its owner - access before reopening for encoding. Staging stays beside the destination so - publication can restore inherited as well as protected DACLs. The adapter checks - owner, group, DACL, integrity/resource access metadata and readonly state. - Windows may modernize a legacy unprotected file ACL by appending inherited - duplicates of existing unconditional allow grants. Only that equivalent form - is accepted; new rights, deny changes, protection changes, and SACL changes - still fail policy verification. - Replacement also captures, restores, and verifies the full audit SACL and its - protection state. This requires SeSecurityPrivilege to already be enabled in - the exporting process. The application does not enable or acquire privileges. - If full audit policy cannot be inspected, replacement fails before rendering - and leaves the existing file intact; choose a new output filename instead. - New destinations do not require this privilege and retain normal inheritance. -- Linux preserves numeric ownership, permission bits, and security/system xattrs, - including POSIX access ACLs. A setgid directory cannot silently change the owner - group of an existing output. No libacl dependency is added on Linux. -- macOS uses an empty private staging directory with its inherited ACL cleared - before creating the media file, because ACL grants can override ordinary mode - bits. Native ACL entries are restored along with ownership and mode. -- FreeBSD uses the native ACL adapter and an owner-only POSIX staging-directory - ACL. Filesystem policies the adapter cannot reproduce are rejected; unsupported - ACL/filesystem combinations must not silently fall back to chmod alone. - -Privilege-bearing files (setuid/setgid/sticky file modes or Linux file capabilities) -are not export replacement targets. This change preserves access policy, not all -arbitrary filesystem metadata such as user tags or alternate data streams. - -## Links and concurrent changes - -Leaf symbolic links are resolved with a bound of 40 links. Relative and dangling -links target their referenced filename; the link itself is retained. Directory -links are resolved by the filesystem. Files with multiple hard links are rejected -because replacing their inode would silently disconnect the other aliases. - -The destination identity, length, modification time, access policy, resolved target, -and parent access policy are checked again before publication. Changed destinations -cause an error. Jobs in one process serialize their final validation/replacement. -New destinations are published with a no-clobber hard link, so an intervening creator -cannot be overwritten. This requires a filesystem that supports hard links; an empty probe checks that -capability before encoding begins. - -External writers must cooperate. Portable rename does not provide compare-and-swap -against a file version, so there remains a race between the final check and replacing -an existing destination. This is not protection against arbitrary concurrent changes -by another process with authority to modify the output directory or its security -policy. Use separate output paths for independent processes. - -## Tests - -The native tests cover the staging lifecycle, competing jobs, policy changes, -failed publication, hard-link rejection, symlink targets, and platform ACL behavior. -Linux includes a named-user ACL and a differing-group setgid case when run as root. -The export integration tests cancel after writing begins and compare successful -movie/sequence output against sink bytes. The filesystem workflow runs these on -Windows, Linux and macOS. Local cross-compilation alone is not runtime validation. - -Windows tests that replace existing files explicitly require enabled -SeSecurityPrivilege. An ordinary local test run marks those cases ignored rather -than pretending audit preservation was exercised. The Windows filesystem workflow -runs `windows_audit_tests`, a CI-only launcher that enables an already-granted -privilege and runs both native and integration suites with `--include-ignored`. -It also runs refusal in a separate child process with that privilege removed, -checking old bytes, absence of staging, and successful export to a new filename. +# Export output safety + +Native exports encode into unique sibling files created by `tempfile` 3.27 +(MIT OR Apache-2.0). After encoding completes and writers close, `TempPath::persist` +publishes an accepted replacement; `persist_noclobber` publishes a new destination +without overwriting an intervening creator. Publication uses the library's normal +platform replacement behavior. No audit privilege or custom metadata reconstruction +is required. + +Cancellation and encoding errors leave existing media untouched and clean up only +job-owned staging paths. Replacement errors are returned with the destination and +publication count. A sequence publishes one complete file at a time: a later failure +can leave a mixture of complete old and new frames. There is no whole-sequence +transaction, rollback, trailing-frame deletion, or unrelated-file cleanup. + +This contract does not preserve exact permissions, ownership, ACLs, audit entries, +or extended attributes. Published files have the library-created staging file's +ordinary metadata: Unix temporary files normally start with mode 0600; Windows +uses ordinary creation security inherited from the directory. The library performs +replacement without syncing files or directories, so this is not a crash or +power-loss recovery protocol. + +`preflight` inspects the precise movie, sequence, and overflow destinations without +creating files or folders. The host shows collisions and retains the authorization +with its queue/settings snapshot. `export_authorized` checks that reviewed paths, +file identities, lengths, timestamps, and basic permission metadata still match. +The default `export` refuses existing outputs without authorization. Skipped +sequence frames are not authorized for writes. Memory-sink exports keep their +existing callback contract. + +The checks reject observed destination changes, and a process-local lock coordinates +publication between exports. An uncooperating external writer can still race the +last check and an accepted replacement; this is not a filesystem compare-and-swap +guarantee. Existing leaf symlinks resolve to their targets; replacing a hard-linked +name leaves its other names pointing at the previous file. + +Render logs are separate, best-effort output. An existing log is preserved and a +unique sibling log is created; reports return the actual log path. From 2889d974c099dd9eb98ec6d76c7a430bfd1e4e77 Mon Sep 17 00:00:00 2001 From: TimotheusG Date: Tue, 20 Oct 2026 18:49:25 -0400 Subject: [PATCH 8/8] G3: preserve Unix output modes and allow long fresh sequences --- crates/engine/src/render_queue.rs | 3 -- crates/engine/src/rq_tests.rs | 26 +++++++++++++++ crates/export/src/pending/native.rs | 34 ++++++++++++++++++- crates/export/src/preflight.rs | 24 ++++++-------- crates/export/tests/render_settings.rs | 45 ++++++++++++++++++++++++++ docs/export-output-safety.md | 14 +++++--- 6 files changed, 122 insertions(+), 24 deletions(-) diff --git a/crates/engine/src/render_queue.rs b/crates/engine/src/render_queue.rs index e47385841..85892d4c7 100644 --- a/crates/engine/src/render_queue.rs +++ b/crates/engine/src/render_queue.rs @@ -588,9 +588,6 @@ impl Session { if destinations.contains(&destination) { return Err(format!("queued outputs use the same output destination: {destination}; choose separate output paths")); } - if destinations.len() >= 100_000 { - return Err("render review exceeds 100000 output destinations; render fewer queue items at once".into()); - } destinations.insert(destination); } let bytes = serde_json::to_vec(&review.authorization).map_err(|e| e.to_string())?.len(); diff --git a/crates/engine/src/rq_tests.rs b/crates/engine/src/rq_tests.rs index 11924d9d6..33f6bc32d 100644 --- a/crates/engine/src/rq_tests.rs +++ b/crates/engine/src/rq_tests.rs @@ -325,3 +325,29 @@ fn overwrite_preflight_bounds_repeated_path_bytes_before_segment_expansion() { assert!(s.render_review.is_none()); assert!(ex.log.lock().unwrap().is_empty()); } + +#[test] +fn fresh_preflight_accepts_more_than_100000_destinations() { + use crate::render_queue::ExportPreflight; + struct LongSequence; + impl Exporter for LongSequence { + fn formats(&self) -> Vec { + vec![OutputFormat::PngSequence] + } + fn preflight(&self, _: &ExportJob) -> Result { + Ok(ExportPreflight { + collisions: vec![], + destinations: (0..100_001).map(|frame| format!("/fresh/f_{frame}.png")).collect(), + authorization: json!({"bounded": true}), + }) + } + fn export(&self, _: &ExportJob, _: &mut dyn FnMut(u64, u64) -> bool) -> Result { + Err("preflight must not render".into()) + } + } + let mut session = Session { exporter: Some(Arc::new(LongSequence)), ..Default::default() }; + session.execute("comp.new", json!({"width":4, "height":4, "duration":1})).unwrap(); + session.execute("renderQueue.add", json!({"format":"png", "output": crate::tests_stubs::tmp_out("long_[#####].png")})).unwrap(); + let review = session.preflight_render().unwrap(); + assert!(review["collisions"].as_array().unwrap().is_empty()); +} diff --git a/crates/export/src/pending/native.rs b/crates/export/src/pending/native.rs index 64b5a4630..1870f9b62 100644 --- a/crates/export/src/pending/native.rs +++ b/crates/export/src/pending/native.rs @@ -140,7 +140,22 @@ impl Staged { let path = resolve(&requested, true)?; let original = readable_state(&path)?; let parent = path.parent().ok_or_else(|| io::Error::other("output has no parent directory"))?; - let (file, temporary) = tempfile::Builder::new().prefix(".effectcraft-export-").tempfile_in(parent)?.into_parts(); + let mut builder = tempfile::Builder::new(); + builder.prefix(".effectcraft-export-"); + #[cfg(unix)] + let existing_permissions = { + use std::os::unix::fs::PermissionsExt; + let permissions = if original.is_null() { None } else { Some(fs::metadata(&path)?.permissions()) }; + builder.permissions(permissions.clone().unwrap_or_else(|| fs::Permissions::from_mode(0o666))); + permissions + }; + let (file, temporary) = builder.tempfile_in(parent)?.into_parts(); + #[cfg(unix)] + if let Some(permissions) = existing_permissions { + // Creation applies umask even to an existing mode. Retain that mode on the + // owned staging file, including bits the current umask would remove. + file.set_permissions(permissions)?; + } let staged = Self { temporary: Some(temporary), path, requested, original }; staged.validate()?; Ok((staged, file)) @@ -321,6 +336,23 @@ mod tests { assert_eq!(fs::read(alias).unwrap(), b"original"); } + #[cfg(unix)] + #[test] + fn unix_publication_uses_normal_creation_mode_and_existing_mode() { + use std::os::unix::fs::PermissionsExt; + let dir = tempfile::tempdir().unwrap(); + let ordinary = dir.path().join("ordinary"); + drop(File::create(&ordinary).unwrap()); + let creation_mode = fs::metadata(&ordinary).unwrap().permissions().mode() & 0o777; + let path = dir.path().join("movie.mov"); + stage(&path, b"new output").publish().unwrap(); + assert_eq!(fs::metadata(&path).unwrap().permissions().mode() & 0o777, creation_mode); + fs::set_permissions(&path, fs::Permissions::from_mode(0o664)).unwrap(); + stage(&path, b"replacement").publish().unwrap(); + assert_eq!(fs::metadata(&path).unwrap().permissions().mode() & 0o777, 0o664); + assert_eq!(fs::read(&path).unwrap(), b"replacement"); + } + #[cfg(windows)] #[test] fn library_replacement_error_preserves_locked_destination_and_cleans_staging() { diff --git a/crates/export/src/preflight.rs b/crates/export/src/preflight.rs index 4e59b5403..734bd77ee 100644 --- a/crates/export/src/preflight.rs +++ b/crates/export/src/preflight.rs @@ -5,7 +5,8 @@ use serde_json::{Map, Value, json}; use crate::{Job, Result, io}; const FORMAT: &str = "effectcraft.output-approval.v1"; -const MAX_TARGETS: usize = 100_000; +#[cfg(not(target_arch = "wasm32"))] +const MAX_COLLISIONS: usize = 100_000; #[cfg(not(target_arch = "wasm32"))] const MAX_TOKEN_BYTES: usize = 64 * 1024 * 1024; @@ -30,7 +31,7 @@ pub fn preflight(job: &Job<'_>) -> Result { fn collect(job: &Job<'_>) -> Result<(Map, Vec, Vec)> { let mut targets = Map::new(); let mut collisions = Vec::new(); - let mut bytes = 0_usize; + let mut bytes = serde_json::to_vec(&json!({"format": FORMAT, "targets": {}})).map_err(io)?.len(); let mut destinations = std::collections::BTreeSet::new(); if job.sink.is_none() { visit_paths(job, |path, skipped| { @@ -43,14 +44,15 @@ fn collect(job: &Job<'_>) -> Result<(Map, Vec, Vec= MAX_TARGETS { - return Err(io("export preflight exceeds 100000 distinct output paths")); + if existing && collisions.len() >= MAX_COLLISIONS { + return Err(io("export preflight exceeds 100000 existing outputs")); } let size = serde_json::to_vec(&entry) .map_err(io)? .len() - .checked_add(key.len().saturating_mul(6)) - .and_then(|size| size.checked_add(8)) + .checked_add(serde_json::to_vec(&key).map_err(io)?.len()) + // The colon and comma; the initial envelope already includes braces. + .and_then(|size| size.checked_add(2)) .ok_or_else(|| io("export preflight metadata size overflow"))?; bytes = bytes.checked_add(size).filter(|n| *n <= MAX_TOKEN_BYTES).ok_or_else(|| io("export preflight metadata exceeds 64 MiB"))?; if existing { @@ -95,21 +97,13 @@ pub(crate) fn token_targets(authorization: &Value) -> std::io::Result<&Map, mut visit: impl FnMut(&str, bool) -> Result<()>) -> Result<()> { let comp = job.project.comp(job.comp).ok_or(crate::ExportError::NoComp)?; let frames = if job.output.format.is_sequence() { job.settings.frame_count(comp) } else { 1 }; let alternatives = if job.settings.storage_overflow && job.options.storage.is_some() { job.options.overflow.len() } else { 0 }; - let count = frames.checked_mul(alternatives.saturating_add(1) as u64).ok_or_else(|| io("export preflight path count overflow"))?; - if count > MAX_TARGETS as u64 { - return Err(io("export preflight exceeds 100000 output candidates")); - } let first = job.settings.first_frame(comp); for frame in 0..frames { let path = if job.output.format.is_sequence() { diff --git a/crates/export/tests/render_settings.rs b/crates/export/tests/render_settings.rs index 555869b2d..f4582c33f 100644 --- a/crates/export/tests/render_settings.rs +++ b/crates/export/tests/render_settings.rs @@ -848,3 +848,48 @@ fn render_log_symlink_does_not_follow_or_truncate_an_unrelated_target() { assert_eq!(std::fs::read_link(&wanted).unwrap(), unrelated); assert_eq!(std::fs::read(&wanted).unwrap(), b"unrelated project bytes"); } + +/// Review more than the former path limit without encoding 100001 frames or creating them. +#[test] +fn long_fresh_sequences_and_overflow_keep_exact_bounded_authorization() { + struct Room; + impl StorageQuota for Room { + fn has_room(&self, _: &str, _: u64) -> bool { + true + } + } + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("f_[#####].png"); + let overflow = dir.path().join("overflow"); + let (p, cid) = project(FrameRate::new(25, 1), |_, comp| { + comp.duration = Tick::from_seconds_f64(100_001.0 / 25.0); + }); + let output = OutputModule::for_format(OutputFormat::PngSequence); + for alternatives in [false, true] { + // 50001 frames with one overflow folder yield100002 candidates. + let time_span = + if alternatives { TimeSpan::Custom { start: Tick::ZERO, end: Tick::from_seconds_f64(50_001.0 / 25.0) } } else { TimeSpan::LengthOfComp }; + let settings = RenderSettings { time_span, storage_overflow: alternatives, ..Default::default() }; + let mut job = disk_job(&p, cid, &settings, &output, path.to_str().unwrap()); + job.options.storage = alternatives.then_some(&Room); + job.options.overflow = if alternatives { vec![overflow.to_str().unwrap().into()] } else { vec![] }; + let reviewed = effectcraft_export::preflight(&job).unwrap(); + let expected = if alternatives { 100_002 } else { 100_001 }; + assert!(reviewed.collisions.is_empty()); + assert_eq!(reviewed.destinations.len(), expected); + assert_eq!(reviewed.authorization["targets"].as_object().unwrap().len(), expected); + assert!(serde_json::to_vec(&reviewed.authorization).unwrap().len() <= 64 * 1024 * 1024); + let last = sequence_path(job.path, if alternatives { 50_000 } else { 100_000 }); + let key = std::path::absolute(&last).unwrap(); + let entry = &reviewed.authorization["targets"][key.to_str().unwrap()]; + assert!(entry["state"].is_null()); + assert!(entry["resolved"].is_string()); + assert!(entry["anchor"].is_string()); + assert!(entry.get("anchorPolicy").is_some()); + // This validates every reviewed path and a token larger than the former count + // limit, then cancels before opening staging or encoding the long sequence. + let error = effectcraft_export::export_authorized(&job, &reviewed.authorization, &mut |_| false).unwrap_err(); + assert!(matches!(error, effectcraft_export::ExportError::Cancelled), "{error}"); + assert_eq!(std::fs::read_dir(dir.path()).unwrap().count(), 0); + } +} diff --git a/docs/export-output-safety.md b/docs/export-output-safety.md index 642869615..11e78546c 100644 --- a/docs/export-output-safety.md +++ b/docs/export-output-safety.md @@ -13,10 +13,12 @@ publication count. A sequence publishes one complete file at a time: a later fai can leave a mixture of complete old and new frames. There is no whole-sequence transaction, rollback, trailing-frame deletion, or unrelated-file cleanup. -This contract does not preserve exact permissions, ownership, ACLs, audit entries, -or extended attributes. Published files have the library-created staging file's -ordinary metadata: Unix temporary files normally start with mode 0600; Windows -uses ordinary creation security inherited from the directory. The library performs +On Unix, new outputs use normal file creation mode (0666 filtered by the process +umask); replacements retain the existing file's mode bits. Staging is configured +through `tempfile::Builder::permissions`, with `File::set_permissions` retaining +replacement mode bits that the current umask would otherwise remove. Windows uses +ordinary creation security inherited from the directory. Ownership, ACLs, audit +entries, and extended attributes are not reconstructed. The library performs replacement without syncing files or directories, so this is not a crash or power-loss recovery protocol. @@ -26,7 +28,9 @@ with its queue/settings snapshot. `export_authorized` checks that reviewed paths file identities, lengths, timestamps, and basic permission metadata still match. The default `export` refuses existing outputs without authorization. Skipped sequence frames are not authorized for writes. Memory-sink exports keep their -existing callback contract. +existing callback contract. Collision lists are limited to 100000 existing outputs; +authorization metadata is limited to 64 MiB, counting its serialized size. Fresh +frames and overflow candidates do not consume the collision count limit. The checks reject observed destination changes, and a process-local lock coordinates publication between exports. An uncooperating external writer can still race the