From 0857ab5465d2bef228c91be5df8187dae2c40d4a Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Wed, 4 Nov 2026 01:55:39 +0000 Subject: [PATCH] address review: canonicalize through the checked io gateway, treat any occupied export name as taken --- crates/engine/src/clip_group_file.rs | 40 +++++++++++++++++++++++-- crates/engine/src/io.rs | 45 ++++++++++++++++++++++++---- 2 files changed, 78 insertions(+), 7 deletions(-) diff --git a/crates/engine/src/clip_group_file.rs b/crates/engine/src/clip_group_file.rs index 66af0233..fbf1da9f 100644 --- a/crates/engine/src/clip_group_file.rs +++ b/crates/engine/src/clip_group_file.rs @@ -89,7 +89,7 @@ fn decode_group_audio( let (mut decoded, mut index, mut file_of) = (Vec::new(), BTreeMap::new(), BTreeMap::new()); // The folder as the file system knows it, so a link among the group's files can't lead out. let dir = if dir.as_os_str().is_empty() { Path::new(".") } else { dir }; - let root = std::fs::canonicalize(dir).map_err(|err| EngineError::Io(format!("{}: {err}", dir.display())))?; + let root = crate::io::canonical_path(dir)?; for (old, rel) in audio_entries { let full = dir.join(rel); let escapes = |rel: &str| EngineError::BadParams(cmd_id.into(), format!("audio path escapes the group folder: {rel}")); @@ -100,7 +100,7 @@ fn decode_group_audio( { return Err(escapes(rel)); } - let key = std::fs::canonicalize(&full).map_err(|err| EngineError::Io(format!("{rel}: {err}")))?; + let key = crate::io::canonical_path(&full)?; if !key.starts_with(&root) { return Err(escapes(rel)); } @@ -292,6 +292,42 @@ mod tests { let _ = std::fs::remove_dir_all(&dir); } + /// Under a read root, a link in the group folder to a folder outside the root is refused the same way + /// whether or not the file asked for exists there, so the group cannot be used to probe for names. + #[cfg(unix)] + #[test] + fn links_out_of_the_read_root_are_refused_alike_for_existing_and_missing_files() { + let base = std::env::temp_dir().join(format!("sc-grp-root-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&base); + let (dir, outside) = (base.join("group"), base.join("outside")); + std::fs::create_dir_all(&dir).unwrap(); + std::fs::create_dir_all(&outside).unwrap(); + let path = dir.join("drums.scgrp").to_string_lossy().to_string(); + let mut e = crate::demo::demo_engine(); + let kick = e.session().tracks.iter().find(|t| t.name == "Kick").map(|t| t.id).unwrap(); + let ids: Vec<_> = e.session().track(kick).unwrap().clips().iter().take(1).map(|c| c.id.0).collect(); + e.execute("file.export_clip_groups", &json!({"path": path, "clips": ids})).unwrap(); + let mut file: serde_json::Value = serde_json::from_str(&std::fs::read_to_string(&path).unwrap()).unwrap(); + let exported = file["audio"].as_object().unwrap().values().next().unwrap().as_str().unwrap().to_string(); + std::fs::copy(dir.join(&exported), outside.join("real.wav")).unwrap(); + std::os::unix::fs::symlink(&outside, dir.join("out")).unwrap(); + let roots = crate::file_access::AutomationRoots::new(&[&dir], &[]).unwrap().unwrap(); + let mut refusals = Vec::new(); + for target in ["out/real.wav", "out/missing.wav"] { + for rel in file["audio"].as_object_mut().unwrap().values_mut() { + *rel = json!(target); + } + std::fs::write(&path, file.to_string()).unwrap(); + let r = crate::file_access::confine(Some(&roots), || { + e.execute("file.import_clip_groups", &json!({"path": path, "at": 1000, "track": "Bass"})) + }); + refusals.push(r.unwrap_err().to_string()); + } + assert!(refusals[0].contains("outside the read root"), "{}", refusals[0]); + assert_eq!(refusals[0].replace("real.wav", "X"), refusals[1].replace("missing.wav", "X"), "existing and missing names are refused alike"); + let _ = std::fs::remove_dir_all(&base); + } + #[test] fn imported_clips_carry_no_auto_rename_scheme() { let dir = std::env::temp_dir().join(format!("sc-grp-names-{}", std::process::id())); diff --git a/crates/engine/src/io.rs b/crates/engine/src/io.rs index 7b3dc7ac..926f134a 100644 --- a/crates/engine/src/io.rs +++ b/crates/engine/src/io.rs @@ -85,6 +85,17 @@ pub(crate) fn make_dir(dir: impl AsRef) -> Result<()> { std::fs::create_dir_all(dir).map_err(|err| EngineError::Io(format!("{}: {err}", dir.display()))) } +/// `path` with its links followed, inside the automation read root when there is one. The root is +/// checked on the path as named (links followed, a missing tail resolved by its nearest folder) +/// before anything is resolved, so a name that leads out of the root is refused the same way +/// whether or not it exists there. +pub(crate) fn canonical_path(path: impl AsRef) -> Result { + let path = path.as_ref(); + let checked = if path.as_os_str().is_empty() { Path::new(".") } else { path }; + crate::file_access::check_read(checked).map_err(EngineError::Io)?; + std::fs::canonicalize(path).map_err(|err| EngineError::Io(format!("{}: {err}", path.display()))) +} + /// Write `bytes` to the file at `path`, inside the automation write root when there is one. pub(crate) fn write_path(path: impl AsRef, bytes: impl AsRef<[u8]>) -> Result<()> { let path = path.as_ref(); @@ -465,11 +476,16 @@ pub fn export_clips(e: &Engine, ids: &[soundcraft_model::ClipId], dir: &str) -> // The name is taken by creating the file, so whatever entry already has it (a link // to elsewhere included) is left alone rather than written through. let mut file = loop { - crate::file_access::check_write(&path).map_err(EngineError::Io)?; - match std::fs::OpenOptions::new().write(true).create_new(true).open(&path) { - Ok(f) => break f, - Err(err) if err.kind() == std::io::ErrorKind::AlreadyExists => {} - Err(err) => return Err(EngineError::Io(format!("{}: {err}", path.display()))), + // An occupied name is taken whatever it is (a link whose target is outside the write + // root too): it is only looked at, never resolved, so the root check does not stop the + // search for a free name. The folder itself was checked above. + if std::fs::symlink_metadata(&path).is_err() { + crate::file_access::check_write(&path).map_err(EngineError::Io)?; + match std::fs::OpenOptions::new().write(true).create_new(true).open(&path) { + Ok(f) => break f, + Err(err) if err.kind() == std::io::ErrorKind::AlreadyExists => {} + Err(err) => return Err(EngineError::Io(format!("{}: {err}", path.display()))), + } } path = Path::new(dir).join(format!("{stem}-{k}.wav")); k = k.saturating_add(1); @@ -849,6 +865,25 @@ mod exclusive_tests { assert!(dir.join(format!("{}-2.wav", sanitize_name(&c.name))).is_file(), "the next free name was used"); let _ = std::fs::remove_dir_all(&dir); } + + /// With a write root, a link out of it that holds the name is still just an occupied name. + #[cfg(unix)] + #[test] + fn export_skips_a_link_out_of_the_write_root_too() { + let dir = scratch("export-root-link"); + let e = crate::demo::demo_engine(); + let c = e.session().tracks.iter().find_map(|t| t.clips().iter().find(|c| c.is_audio()).cloned()).unwrap(); + let elsewhere = scratch("export-root-elsewhere"); + let target = elsewhere.join("target.wav"); + std::os::unix::fs::symlink(&target, dir.join(format!("{}.wav", sanitize_name(&c.name)))).unwrap(); + let roots = crate::file_access::AutomationRoots::new(&[], &[&dir]).unwrap().unwrap(); + let n = crate::file_access::confine(Some(&roots), || export_clips(&e, &[c.id], &dir.to_string_lossy())).unwrap(); + assert_eq!(n, 1); + assert!(!target.exists(), "the export followed the link out of the root"); + assert!(dir.join(format!("{}-2.wav", sanitize_name(&c.name))).is_file(), "the next free name was used"); + let _ = std::fs::remove_dir_all(&dir); + let _ = std::fs::remove_dir_all(&elsewhere); + } } #[cfg(test)]