Merge pull request #2023 from nuclearspike/codex/security-aaf-extraction-collision

engine: preserve existing files when AAF/OMF imports embedded audio

Cargo.lock

@@ -1988,6 +1988,7 @@ dependencies = [
  "log",
  "serde",
  "serde_json",
+ "tempfile",
  "thiserror 2.0.21",
 ]
 

apps/filmcraft-web/src/fs.rs

@@ -352,6 +352,24 @@ impl Services for WebServices {
         Ok(())
     }
 
+    fn write_file_new(&self, path: &str, data: &[u8]) -> io::Result<()> {
+        if path.starts_with("/opfs/") {
+            return Err(io::Error::new(io::ErrorKind::Unsupported, "exclusive OPFS writes are unavailable"));
+        }
+        let hook = FS.with(|fs| {
+            let mut fs = fs.borrow_mut();
+            if fs.entries.contains_key(path) {
+                return Err(io::Error::from(io::ErrorKind::AlreadyExists));
+            }
+            fs.entries.insert(path.to_owned(), Entry::Mem(Arc::from(data)));
+            Ok(fs.on_write.clone())
+        })?;
+        if let Some(h) = hook {
+            h(path, data);
+        }
+        Ok(())
+    }
+
     fn file_size(&self, path: &str) -> io::Result<u64> {
         Self::entry_reader(path).map(|r| r.len())
     }
@@ -435,3 +453,32 @@ pub fn download(name: &str, data: &[u8]) -> Result<(), wasm_bindgen::JsValue> {
     });
     Ok(())
 }
+
+#[cfg(test)]
+mod exclusive_write_tests {
+    use super::*;
+
+    #[test]
+    fn virtual_file_creation_is_exclusive_and_opfs_fails_closed() {
+        let host = WebServices;
+        let existing = "/files/collision-test.wav";
+        let fresh = "/files/new-extraction-test.wav";
+        FS.with(|fs| {
+            let mut fs = fs.borrow_mut();
+            fs.on_write = None;
+            fs.entries.insert(existing.into(), Entry::Mem(Arc::from(&b"original"[..])));
+            fs.entries.remove(fresh);
+        });
+        assert_eq!(host.write_file_new(existing, b"replacement").unwrap_err().kind(), io::ErrorKind::AlreadyExists);
+        assert_eq!(host.read_file(existing).unwrap(), b"original");
+        host.write_file_new(fresh, b"new").unwrap();
+        assert_eq!(host.read_file(fresh).unwrap(), b"new");
+        assert_eq!(host.write_file_new(fresh, b"again").unwrap_err().kind(), io::ErrorKind::AlreadyExists);
+        assert_eq!(host.write_file_new("/opfs/media/new.wav", b"new").unwrap_err().kind(), io::ErrorKind::Unsupported);
+        FS.with(|fs| {
+            let mut fs = fs.borrow_mut();
+            fs.entries.remove(existing);
+            fs.entries.remove(fresh);
+        });
+    }
+}

crates/engine/src/aaf_omf_tests.rs

@@ -87,6 +87,50 @@ fn aaf_embedded_trimmed_audio_round_trips_sample_exact() {
     let _ = std::fs::remove_dir_all(dir);
 }
 
+#[test]
+fn embedded_aaf_and_omf_import_leave_existing_media_files_intact() {
+    for (format, extension) in [(filmcraft_interchange::Format::Aaf, "aaf"), (filmcraft_interchange::Format::Omf, "omf")] {
+        let (mut s, _items, seq, dir) = setup(&format!("embedded-collision-{extension}"));
+        let path = dir.join(format!("cut.{extension}"));
+        let path_text = path.to_string_lossy().into_owned();
+        let command = if format == filmcraft_interchange::Format::Aaf { "file.exportAaf" } else { "file.exportOmf" };
+        s.execute(command, json!({"path": path_text, "audio": "embedded", "sequence": seq.0})).unwrap();
+        let bytes = std::fs::read(&path).unwrap();
+        let opts = filmcraft_interchange::ImportOptions { base_dir: Some(dir.to_string_lossy().into_owned()), name: Some("cut".into()), ..Default::default() };
+        let (_, extracted, _) = if format == filmcraft_interchange::Format::Aaf {
+            filmcraft_interchange::aaf::import(&bytes, &opts).unwrap()
+        } else {
+            filmcraft_interchange::omf::import(&bytes, &opts).unwrap()
+        };
+        let first = extracted.first().expect("embedded audio");
+        let original = b"ORIGINAL USER DATA";
+        std::fs::create_dir_all(std::path::Path::new(&first.path).parent().unwrap()).unwrap();
+        std::fs::write(&first.path, original).unwrap();
+
+        let response = s.execute("file.import", json!({"paths": [path_text]})).unwrap();
+        assert!(std::fs::read(&first.path).unwrap().as_slice() == original, "{extension}: existing file was overwritten");
+        let imported = s.project.sequence(imported_seq(&response)).unwrap();
+        let new_path = path_of(&s, imported.audio_tracks[0].items[0].item);
+        assert_ne!(new_path, first.path, "{extension}: imported audio still points at the existing file");
+        assert!(new_path.ends_with(" (2).wav"), "{extension}: {new_path}");
+        assert!(std::fs::read(&new_path).unwrap() == first.wav, "{extension}: imported audio was not extracted to the new path");
+        #[cfg(unix)]
+        {
+            let protected = dir.join("protected.wav");
+            std::fs::write(&protected, original).unwrap();
+            std::fs::remove_file(&first.path).unwrap();
+            std::os::unix::fs::symlink(&protected, &first.path).unwrap();
+            let again = s.execute("file.import", json!({"paths": [path.to_string_lossy()]})).unwrap();
+            assert!(std::fs::read(&protected).unwrap().as_slice() == original, "{extension}: symlink target was overwritten");
+            let imported_again = s.project.sequence(imported_seq(&again)).unwrap();
+            let third_path = path_of(&s, imported_again.audio_tracks[0].items[0].item);
+            assert!(third_path.ends_with(" (3).wav"), "{extension}: {third_path}");
+            assert!(std::fs::read(&third_path).unwrap() == first.wav, "{extension}: the third extraction is not the embedded audio");
+        }
+        let _ = std::fs::remove_dir_all(dir);
+    }
+}
+
 #[test]
 fn aaf_separate_aiff_breakout_and_linked() {
     let (mut s, items, seq, dir) = setup("aaf-separate");

crates/engine/src/interchange.rs

@@ -1,5 +1,7 @@
 //! Interchange documents (CMX 3600 EDL, FCP7 XML, FCPXML, OTIO, AAF, OMF) ↔ the session's project.
 
+use std::path::Path;
+
 use serde_json::{Value, json};
 
 use filmcraft_interchange::{ExportOptions, Format, ImportOptions};
@@ -24,6 +26,32 @@ fn is_file_media(item: &filmcraft_project::ProjectItem) -> bool {
     matches!(&item.kind, ItemKind::Media(m) if matches!(m.media, MediaRef::File { .. }))
 }
 
+/// Write embedded audio without replacing an occupied path; return the path used.
+fn write_embedded_audio(s: &Session, path: &str, wav: &[u8]) -> Result<String> {
+    let original = Path::new(path);
+    let stem = original.file_stem().ok_or_else(|| EngineError::Other(format!("{path}: missing audio file name")))?;
+    for number in 1..=1_000 {
+        let candidate = if number == 1 {
+            original.to_path_buf()
+        } else {
+            let mut name = stem.to_os_string();
+            name.push(format!(" ({number})"));
+            if let Some(extension) = original.extension() {
+                name.push(".");
+                name.push(extension);
+            }
+            original.with_file_name(name)
+        };
+        let candidate_text = candidate.to_string_lossy();
+        match s.services.write_file_new(&candidate_text, wav) {
+            Ok(()) => return Ok(candidate_text.into_owned()),
+            Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => {}
+            Err(e) => return Err(EngineError::Other(format!("{candidate_text}: {e}"))),
+        }
+    }
+    Err(EngineError::Other(format!("{path}: no unused embedded-audio filename after 1000 attempts")))
+}
+
 /// Import a document: merge its bins, media and sequences into the project (one undo step), then
 /// link each media file that exists on disk. Returns the new sequences.
 pub fn import(s: &mut Session, path: &str, bytes: &[u8], format: Format) -> Result<Value> {
@@ -37,12 +65,17 @@ pub fn import(s: &mut Session, path: &str, bytes: &[u8], format: Format) -> Resu
         // AAF / OMF may embed audio: write it next to the document (the media items point there)
         Format::Aaf | Format::Omf => {
             let r = if format == Format::Aaf { filmcraft_interchange::aaf::import(bytes, &opts) } else { filmcraft_interchange::omf::import(bytes, &opts) };
-            let (fragment, extracted, report) = r.map_err(|e| EngineError::Other(format!("{path}: {e}")))?;
+            let (mut fragment, extracted, report) = r.map_err(|e| EngineError::Other(format!("{path}: {e}")))?;
             for m in extracted {
-                if let Some(dir) = std::path::Path::new(&m.path).parent().filter(|_| !s.services.export_in_memory()) {
-                    let _ = std::fs::create_dir_all(dir);
+                let media = fragment
+                    .project
+                    .item_mut(m.item)
+                    .and_then(|item| item.as_media_mut())
+                    .ok_or_else(|| EngineError::Other(format!("{}: embedded audio has no media item", m.path)))?;
+                let written_path = write_embedded_audio(s, &m.path, &m.wav)?;
+                if written_path != m.path {
+                    media.media = MediaRef::File { path: written_path };
                 }
-                s.services.write_file(&m.path, &m.wav).map_err(|e| EngineError::Other(format!("{}: {e}", m.path)))?;
             }
             (fragment, report)
         }
@@ -152,3 +185,51 @@ pub fn export(s: &mut Session, p: &Value) -> Result<Value> {
     s.services.write_file(&path, &bytes).map_err(|e| EngineError::Other(e.to_string()))?;
     Ok(json!({"path": path, "bytes": bytes.len(), "report": report_json(&report)}))
 }
+
+#[cfg(test)]
+mod embedded_audio_tests {
+    use std::collections::HashMap;
+    use std::io;
+    use std::sync::{Arc, Mutex};
+
+    use crate::{Services, Session};
+
+    use super::write_embedded_audio;
+
+    #[derive(Default)]
+    struct VirtualFiles(Mutex<HashMap<String, Vec<u8>>>);
+
+    impl Services for VirtualFiles {
+        fn read_file(&self, path: &str) -> io::Result<Vec<u8>> {
+            self.0.lock().unwrap().get(path).cloned().ok_or_else(|| io::ErrorKind::NotFound.into())
+        }
+
+        fn write_file(&self, path: &str, data: &[u8]) -> io::Result<()> {
+            self.0.lock().unwrap().insert(path.to_owned(), data.to_vec());
+            Ok(())
+        }
+
+        fn write_file_new(&self, path: &str, data: &[u8]) -> io::Result<()> {
+            let mut files = self.0.lock().unwrap();
+            if files.contains_key(path) {
+                return Err(io::ErrorKind::AlreadyExists.into());
+            }
+            files.insert(path.to_owned(), data.to_vec());
+            Ok(())
+        }
+    }
+
+    #[test]
+    fn virtual_extraction_keeps_existing_media() {
+        let files = Arc::new(VirtualFiles::default());
+        files.0.lock().unwrap().insert("/files/cut Audio.wav".into(), b"first".to_vec());
+        files.0.lock().unwrap().insert("/files/cut Audio (2).wav".into(), b"second".to_vec());
+        let session = Session::new(files.clone());
+        let path = write_embedded_audio(&session, "/files/cut Audio.wav", b"new audio").unwrap();
+        assert_eq!(path, "/files/cut Audio (3).wav");
+        let stored = files.0.lock().unwrap();
+        assert_eq!(stored["/files/cut Audio.wav"], b"first");
+        assert_eq!(stored["/files/cut Audio (2).wav"], b"second");
+        assert_eq!(stored["/files/cut Audio (3).wav"], b"new audio");
+    }
+}

crates/engine/src/lib.rs

@@ -102,6 +102,14 @@ pub type Result<T> = std::result::Result<T, EngineError>;
 pub trait Services: Send + Sync {
     fn read_file(&self, path: &str) -> std::io::Result<Vec<u8>>;
     fn write_file(&self, path: &str, data: &[u8]) -> std::io::Result<()>;
+    /// Publish a complete new file only when the final path is unoccupied. Publication must
+    /// reject a concurrent collision; a prior existence check alone is insufficient.
+    /// Return `ErrorKind::AlreadyExists` for an occupied final path so the importer can retry
+    /// with a numbered name; return other errors without publishing a partial file.
+    /// Hosts without this operation fail closed.
+    fn write_file_new(&self, _path: &str, _data: &[u8]) -> std::io::Result<()> {
+        Err(std::io::Error::new(std::io::ErrorKind::Unsupported, "exclusive file creation is unavailable on this host"))
+    }
     /// Size of a file in bytes (an error when it is missing). Hosts should override this: the
     /// default reads the whole file.
     fn file_size(&self, path: &str) -> std::io::Result<u64> {
@@ -159,6 +167,23 @@ impl Services for FsServices {
         // Atomic + durable: temp file in the same directory, fsync, rename, fsync the directory.
         filmcraft_format::atomic_write(std::path::Path::new(path), data)
     }
+    fn write_file_new(&self, path: &str, data: &[u8]) -> std::io::Result<()> {
+        let path = std::path::Path::new(path);
+        let dir = path.parent().filter(|p| !p.as_os_str().is_empty()).unwrap_or_else(|| std::path::Path::new("."));
+        std::fs::create_dir_all(dir).map_err(|error| {
+            if error.kind() == std::io::ErrorKind::AlreadyExists {
+                std::io::Error::new(std::io::ErrorKind::NotADirectory, format!("{} is not a directory: {error}", dir.display()))
+            } else {
+                error
+            }
+        })?;
+        match std::fs::symlink_metadata(path) {
+            Ok(_) => return Err(std::io::ErrorKind::AlreadyExists.into()),
+            Err(e) if e.kind() == std::io::ErrorKind::NotFound => {}
+            Err(e) => return Err(e),
+        }
+        filmcraft_format::atomic_write_new(path, data)
+    }
     fn file_size(&self, path: &str) -> std::io::Result<u64> {
         let m = std::fs::metadata(path)?;
         if m.is_file() { Ok(m.len()) } else { Err(std::io::Error::new(std::io::ErrorKind::NotFound, format!("{path} is not a file"))) }

crates/format/Cargo.toml

@@ -17,5 +17,8 @@ serde = { workspace = true }
 serde_json = { workspace = true }
 thiserror = { workspace = true }
 
+[target.'cfg(not(target_arch = "wasm32"))'.dependencies]
+tempfile = "3.27"
+
 [dev-dependencies]
 filmcraft-time = { workspace = true }

crates/format/src/atomic.rs

@@ -46,8 +46,37 @@ pub fn atomic_write(path: &Path, data: &[u8]) -> io::Result<()> {
     Ok(())
 }
 
-/// Best-effort directory fsync (persists the rename on POSIX; not possible on Windows, where
-/// `MoveFileEx` with write-through semantics is already durable enough).
+/// Publish a complete new file without replacing an occupied final path, including a symlink.
+/// Bytes are staged and synced in the destination directory before no-clobber publication.
+/// A write failure leaves no partial final file; temporary-file cleanup is best effort.
+#[cfg(not(target_arch = "wasm32"))]
+pub fn atomic_write_new(path: &Path, data: &[u8]) -> io::Result<()> {
+    atomic_write_new_with(path, |file| file.write_all(data).and_then(|_| file.sync_all()))
+}
+
+#[cfg(target_arch = "wasm32")]
+pub fn atomic_write_new(_path: &Path, _data: &[u8]) -> io::Result<()> {
+    Err(io::Error::new(io::ErrorKind::Unsupported, "native exclusive file creation is unavailable in the browser"))
+}
+
+#[cfg(not(target_arch = "wasm32"))]
+fn atomic_write_new_with(path: &Path, write: impl FnOnce(&mut File) -> io::Result<()>) -> io::Result<()> {
+    let dir = parent_dir(path);
+    #[cfg(unix)]
+    let mut staged = {
+        use std::os::unix::fs::PermissionsExt;
+        // Match ordinary file-creation permissions after the process umask.
+        tempfile::Builder::new().permissions(fs::Permissions::from_mode(0o666)).tempfile_in(&dir)?
+    };
+    #[cfg(not(unix))]
+    let mut staged = tempfile::NamedTempFile::new_in(&dir)?;
+    write(staged.as_file_mut())?;
+    let _published = staged.persist_noclobber(path).map_err(|e| e.error)?;
+    sync_dir(&dir);
+    Ok(())
+}
+
+/// Best-effort directory sync; directory handles are not uniformly readable or syncable.
 fn sync_dir(dir: &Path) {
     if let Ok(d) = File::open(dir) {
         let _ = d.sync_all();
@@ -92,6 +121,66 @@ mod tests {
         fs::remove_dir_all(&d).unwrap();
     }
 
+    #[cfg(not(target_arch = "wasm32"))]
+    #[test]
+    fn new_file_is_published_complete_without_clobbering_or_temp_leaks() {
+        let d = tmpdir("new");
+        let p = d.join("audio.wav");
+        atomic_write_new(&p, b"complete").unwrap();
+        assert_eq!(fs::read(&p).unwrap(), b"complete");
+        #[cfg(unix)]
+        {
+            use std::os::unix::fs::PermissionsExt;
+            let reference = d.join("normal-create");
+            OpenOptions::new().write(true).create_new(true).open(&reference).unwrap();
+            assert_eq!(fs::metadata(&p).unwrap().permissions().mode() & 0o777, fs::metadata(&reference).unwrap().permissions().mode() & 0o777);
+            fs::remove_file(reference).unwrap();
+        }
+        assert_eq!(atomic_write_new(&p, b"replacement").unwrap_err().kind(), io::ErrorKind::AlreadyExists);
+        assert_eq!(fs::read(&p).unwrap(), b"complete");
+        #[cfg(unix)]
+        {
+            let link = d.join("link.wav");
+            std::os::unix::fs::symlink(&p, &link).unwrap();
+            assert_eq!(atomic_write_new(&link, b"replacement").unwrap_err().kind(), io::ErrorKind::AlreadyExists);
+            assert_eq!(fs::read(&p).unwrap(), b"complete");
+        }
+        let expected = if cfg!(unix) { 2 } else { 1 };
+        assert_eq!(fs::read_dir(&d).unwrap().count(), expected, "staging file remained after publication");
+        fs::remove_dir_all(&d).unwrap();
+    }
+
+    #[cfg(unix)]
+    #[test]
+    fn new_file_can_publish_in_a_directory_without_read_permission() {
+        use std::os::unix::fs::PermissionsExt;
+        let d = tmpdir("write-only-directory");
+        let original_permissions = fs::metadata(&d).unwrap().permissions();
+        fs::set_permissions(&d, fs::Permissions::from_mode(0o300)).unwrap();
+        let p = d.join("audio.wav");
+        let result = atomic_write_new(&p, b"complete");
+        fs::set_permissions(&d, original_permissions).unwrap();
+        assert!(result.is_ok(), "{result:?}");
+        assert_eq!(fs::read(&p).unwrap(), b"complete");
+        fs::remove_dir_all(&d).unwrap();
+    }
+
+    #[cfg(not(target_arch = "wasm32"))]
+    #[test]
+    fn failed_staging_leaves_no_partial_destination() {
+        let d = tmpdir("failed-new");
+        let p = d.join("audio.wav");
+        let error = atomic_write_new_with(&p, |file| {
+            file.write_all(b"partial")?;
+            Err(io::Error::other("simulated full disk"))
+        })
+        .unwrap_err();
+        assert_eq!(error.to_string(), "simulated full disk");
+        assert!(!p.exists());
+        assert_eq!(fs::read_dir(&d).unwrap().count(), 0);
+        fs::remove_dir_all(&d).unwrap();
+    }
+
     #[test]
     fn missing_directory_is_an_error() {
         let d = tmpdir("missing");

crates/format/src/lib.rs

@@ -49,7 +49,7 @@ use filmcraft_project::{Project, ProjectView};
 use serde::{Deserialize, Serialize};
 use serde_json::Value;
 
-pub use atomic::atomic_write;
+pub use atomic::{atomic_write, atomic_write_new};
 
 /// The `format` tag written in every envelope.
 pub const FORMAT_ID: &str = "filmcraft.project";