address review: canonicalize through the checked io gateway, treat any occupied export name as taken

crates/engine/src/clip_group_file.rs

@@ -87,23 +87,30 @@ fn decode_group_audio(
     cmd_id: &str,
 ) -> Result<(Vec<(String, soundcraft_audio_io::AudioBuffer)>, BTreeMap<u64, usize>)> {
     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 = 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}"));
         if !full.starts_with(dir)
             || Path::new(rel)
                 .components()
                 .any(|c| matches!(c, std::path::Component::ParentDir | std::path::Component::RootDir | std::path::Component::Prefix(_)))
         {
-            return Err(EngineError::BadParams(cmd_id.into(), format!("audio path escapes the group folder: {rel}")));
+            return Err(escapes(rel));
+        }
+        let key = crate::io::canonical_path(&full)?;
+        if !key.starts_with(&root) {
+            return Err(escapes(rel));
         }
-        let key = crate::io::file_key(&full);
         let n = match index.get(&key) {
             Some(n) => *n,
             None => {
                 let bytes = crate::io::read_file(&full)?;
                 let (_, buf) = soundcraft_audio_io::decode(&bytes, Some("wav")).map_err(|err| EngineError::Io(format!("{rel}: {err}")))?;
                 let buf = crate::io::at_rate(buf, rate, crate::io::MAX_IMPORT_SAMPLES)
-                    .map_err(|r| EngineError::Io(format!("{rel}: {}", r.message("too large to import"))))?;
+                    .ok_or_else(|| EngineError::Io(format!("{rel}: too large to import")))?;
                 decoded.push((rel.clone(), buf));
                 index.insert(key, decoded.len() - 1);
                 decoded.len() - 1
@@ -268,9 +275,59 @@ mod tests {
         std::fs::write(&path, file.to_string()).unwrap();
         let r = e.execute("file.import_clip_groups", &json!({"path": path, "at": 1000, "track": "Bass"}));
         assert!(matches!(r, Err(crate::EngineError::BadParams(..))), "absolute paths are refused: {r:?}");
+        // A link inside the folder that leads out of it is refused once resolved.
+        #[cfg(unix)]
+        {
+            let outside = std::env::temp_dir().join(format!("sc-grp-outside-{}.wav", std::process::id()));
+            std::fs::copy(dir.join("Wait... what_0.wav"), &outside).unwrap();
+            std::os::unix::fs::symlink(&outside, dir.join("link.wav")).unwrap();
+            for rel in file["audio"].as_object_mut().unwrap().values_mut() {
+                *rel = json!("link.wav");
+            }
+            std::fs::write(&path, file.to_string()).unwrap();
+            let r = e.execute("file.import_clip_groups", &json!({"path": path, "at": 1000, "track": "Bass"}));
+            assert!(matches!(r, Err(crate::EngineError::BadParams(..))), "links out of the folder are refused: {r:?}");
+            let _ = std::fs::remove_file(&outside);
+        }
         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()));

crates/engine/src/cmd/sampler.rs

@@ -75,7 +75,7 @@ pub(crate) fn attach(e: &mut Engine, t: TrackId, name: &str, bytes: &[u8], path:
         return Err(EngineError::Io(format!("{name}: no audio")));
     }
     let buf = crate::io::at_rate(buf, e.session().sample_rate.hz(), MAX_SAMPLE_VALUES)
-        .map_err(|r| EngineError::Io(format!("{name}: {}", r.message("too long for a sample (ten minutes of 192 kHz stereo at most)"))))?;
+        .ok_or_else(|| EngineError::Io(format!("{name}: too long for a sample (ten minutes of 192 kHz stereo at most)")))?;
     let channels = buf.num_channels();
     let s = e.session_mut();
     let src = crate::io::add_source(s, name, buf, path, format.format);

crates/engine/src/io.rs

@@ -85,6 +85,17 @@ pub(crate) fn make_dir(dir: impl AsRef<Path>) -> 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<Path>) -> Result<PathBuf> {
+    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<Path>, bytes: impl AsRef<[u8]>) -> Result<()> {
     let path = path.as_ref();
@@ -329,7 +340,7 @@ impl Out {
         if cfg!(target_arch = "wasm32") {
             return Ok(Out::Memory(std::io::Cursor::new(Vec::new())));
         }
-        Ok(Out::File(std::io::BufWriter::with_capacity(1 << 20, std::fs::File::create(part)?)))
+        Ok(Out::File(std::io::BufWriter::with_capacity(1 << 20, create_new(part)?)))
     }
 
     /// Puts the finished file at `path` and returns its size: the part file is synced and renamed
@@ -462,14 +473,27 @@ pub fn export_clips(e: &Engine, ids: &[soundcraft_model::ClipId], dir: &str) ->
         let stem = sanitize_name(&c.name);
         let mut path = Path::new(dir).join(format!("{stem}.wav"));
         let mut k = 2u32;
-        while path.exists() {
+        // 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 {
+            // 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);
             if k > 10_000 {
                 return Err(EngineError::Io(format!("{dir}: too many files named {stem}")));
             }
-        }
-        write_path(&path, bytes)?;
+        };
+        std::io::Write::write_all(&mut file, &bytes).map_err(|err| EngineError::Io(format!("{}: {err}", path.display())))?;
         n += 1;
     }
     Ok(n)
@@ -506,13 +530,13 @@ fn write_session(session: &Session, path: &str, overwrite: bool) -> Result<(Sess
         {
             src.path = f.join(&src.path).to_string_lossy().into_owned();
         }
-        let exists_rel = same_dir && !Path::new(&src.path).is_absolute() && dir.join(&src.path).exists();
+        let exists_rel = same_dir && !Path::new(&src.path).is_absolute() && file_found(dir.join(&src.path));
         let shared = shared_paths.get(i).copied().unwrap_or(false);
         if exists_rel && !src.unsaved && !shared {
             continue;
         }
         let in_folder = Path::new(&src.path).strip_prefix(&audio_dir).ok().and_then(Path::to_str).map(str::to_string);
-        if let Some(f) = in_folder.filter(|_| !src.unsaved && Path::new(&src.path).is_file()) {
+        if let Some(f) = in_folder.filter(|_| !src.unsaved && file_found(&src.path)) {
             src.path = format!("Audio Files/{f}");
             continue;
         }
@@ -528,7 +552,7 @@ fn write_session(session: &Session, path: &str, overwrite: bool) -> Result<(Sess
         if overwrite {
             // One file per source id so autosaves never collide or pile up.
             stem = format!("{stem}_{}", src.id.0);
-            if audio_dir.join(format!("{stem}.wav")).exists() {
+            if file_found(audio_dir.join(format!("{stem}.wav"))) {
                 src.path = format!("Audio Files/{stem}.wav");
                 src.unsaved = false;
                 files.insert(Arc::as_ptr(audio), src.path.clone());
@@ -588,11 +612,22 @@ fn sync_dir(dir: &Path) {
 
 fn write_synced(path: &Path, bytes: &[u8]) -> std::io::Result<()> {
     use std::io::Write;
-    let mut f = std::fs::File::create(path)?;
+    let mut f = create_new(path)?;
     f.write_all(bytes)?;
     f.sync_all()
 }
 
+/// Creates `path` as a new file. A leftover of that name (a part file from an interrupted write)
+/// is removed first, but an existing entry is never opened: a link planted at a predictable name
+/// in the destination folder can't redirect the write elsewhere.
+fn create_new(path: &Path) -> std::io::Result<std::fs::File> {
+    match std::fs::remove_file(path) {
+        Err(err) if err.kind() != std::io::ErrorKind::NotFound => return Err(err),
+        _ => {}
+    }
+    std::fs::OpenOptions::new().write(true).create_new(true).open(path)
+}
+
 /// Copies `from` to `to`, which must not exist yet; an existing entry, even a link, is never followed.
 fn copy_new(from: &Path, to: &Path) -> std::io::Result<()> {
     let mut src = std::fs::File::open(from)?;
@@ -687,7 +722,12 @@ fn store_wav(dir: &Path, stem: &str, bytes: &[u8]) -> Result<(String, bool)> {
                 return Ok((name, true));
             }
             Err(err) if err.kind() == std::io::ErrorKind::AlreadyExists => {
-                if std::fs::metadata(&path).is_ok_and(|m| m.len() == bytes.len() as u64) && std::fs::read(&path).is_ok_and(|b| b == bytes) {
+                // Reused only when it may be read: whether it equals the audio must not be
+                // learnt about a file the roots allow writing beside but not reading.
+                if crate::file_access::check_read(&path).is_ok()
+                    && std::fs::metadata(&path).is_ok_and(|m| m.len() == bytes.len() as u64)
+                    && std::fs::read(&path).is_ok_and(|b| b == bytes)
+                {
                     return Ok((name, false));
                 }
             }
@@ -785,6 +825,67 @@ pub fn plan_takes(e: &Engine, unsaved_dir: &Path, input_channels: usize) -> Vec<
         .collect()
 }
 
+#[cfg(test)]
+mod exclusive_tests {
+    use super::*;
+
+    fn scratch(name: &str) -> PathBuf {
+        let dir = std::env::temp_dir().join(format!("soundcraft-io-{name}-{}", std::process::id()));
+        let _ = std::fs::remove_dir_all(&dir);
+        std::fs::create_dir_all(&dir).unwrap();
+        dir
+    }
+
+    #[cfg(unix)]
+    #[test]
+    fn a_planted_link_at_the_part_name_is_not_written_through() {
+        let dir = scratch("part-link");
+        let (target, part) = (dir.join("elsewhere"), dir.join("session.scraft.tmp"));
+        std::os::unix::fs::symlink(&target, &part).unwrap();
+        write_synced(&part, b"new").unwrap();
+        assert!(!target.exists(), "the link's target was created");
+        assert_eq!(std::fs::read(&part).unwrap(), b"new", "the link itself was replaced by the file");
+        // A leftover regular part file is still replaced.
+        write_synced(&part, b"newer").unwrap();
+        assert_eq!(std::fs::read(&part).unwrap(), b"newer");
+        let _ = std::fs::remove_dir_all(&dir);
+    }
+
+    #[cfg(unix)]
+    #[test]
+    fn export_skips_a_name_held_by_a_dangling_link() {
+        let dir = scratch("export-link");
+        let e = crate::demo::demo_engine();
+        let (t, c) = e.session().tracks.iter().find_map(|t| t.clips().iter().find(|c| c.is_audio()).map(|c| (t.id, c.clone()))).unwrap();
+        let _ = t;
+        let target = dir.join("elsewhere.wav");
+        std::os::unix::fs::symlink(&target, dir.join(format!("{}.wav", sanitize_name(&c.name)))).unwrap();
+        assert_eq!(export_clips(&e, &[c.id], &dir.to_string_lossy()).unwrap(), 1);
+        assert!(!target.exists(), "the export followed the link");
+        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)]
 mod stem_tests {
     #[test]

crates/engine/src/io/import.rs

@@ -7,48 +7,18 @@ use super::{
 /// Largest import we accept (samples per channel × channels), ~4 GiB of f32.
 pub(crate) const MAX_IMPORT_SAMPLES: usize = 1 << 30;
 
-/// The most a file's audio may grow when converted to the session rate. Sessions run at 8 kHz to
-/// 768 kHz (96×); past this a file is lying about its rate, and converting a few kilobytes would
-/// otherwise allocate and filter hundreds of megabytes.
-const MAX_RATE_EXPANSION: u32 = 128;
-
-/// Why [`at_rate`] refused a conversion.
-#[derive(Debug, PartialEq, Eq)]
-pub(crate) enum RateError {
-    /// The converted audio would hold more samples than the limit.
-    TooLarge,
-    /// The file's rate is so far below the session's that converting would grow it past [`MAX_RATE_EXPANSION`].
-    Ratio { from: u32, to: u32 },
-}
-
-impl RateError {
-    /// The refusal as a sentence: `size` says what a size refusal means to the caller.
-    pub(crate) fn message(&self, size: &str) -> String {
-        match self {
-            RateError::TooLarge => size.to_string(),
-            RateError::Ratio { from, to } => {
-                format!("its {from} Hz sample rate is too low to convert to the session's {to} Hz (at most {MAX_RATE_EXPANSION}-fold)")
-            }
-        }
-    }
-}
-
-/// `buf` converted to `rate`, unless it would then hold more than `limit` samples, or grow more
-/// than [`MAX_RATE_EXPANSION`]-fold: a file with a tiny sample rate grows by orders of magnitude.
-pub(crate) fn at_rate(mut buf: AudioBuffer, rate: u32, limit: usize) -> std::result::Result<AudioBuffer, RateError> {
+/// `buf` converted to `rate`, unless it would then hold more than `limit` samples: a file with a
+/// tiny sample rate grows by orders of magnitude.
+pub(crate) fn at_rate(mut buf: AudioBuffer, rate: u32, limit: usize) -> Option<AudioBuffer> {
     let from = buf.sample_rate.max(1);
-    // widened integers: dividing first would truncate 128.86-fold down to 128
-    if u64::from(rate) > u64::from(from) * u64::from(MAX_RATE_EXPANSION) {
-        return Err(RateError::Ratio { from, to: rate });
-    }
     if buf.frames() as f64 * buf.num_channels() as f64 * f64::from(rate) / f64::from(from) > limit as f64 {
-        return Err(RateError::TooLarge);
+        return None;
     }
     if from != rate {
         buf.channels = soundcraft_dsp::offline::resample(&buf.channels, from, rate);
         buf.sample_rate = rate;
     }
-    Ok(buf)
+    Some(buf)
 }
 
 /// A relative `Audio Files/` path no other source in `s` already uses.
@@ -145,7 +115,7 @@ pub fn decode_import(path: &str, rate: u32) -> Result<DecodedImport> {
 fn decode_import_bytes(name: &str, bytes: &[u8], rate: u32) -> Result<(soundcraft_audio_io::AudioInfo, Arc<SourceAudio>)> {
     let ext = Path::new(name).extension().and_then(OsStr::to_str).map(str::to_ascii_lowercase);
     let (info, buf) = soundcraft_audio_io::decode(bytes, ext.as_deref()).map_err(|err| EngineError::Io(format!("{name}: {err}")))?;
-    let buf = at_rate(buf, rate, MAX_IMPORT_SAMPLES).map_err(|e| EngineError::Io(format!("{name}: {}", e.message("file is too large to import"))))?;
+    let buf = at_rate(buf, rate, MAX_IMPORT_SAMPLES).ok_or_else(|| EngineError::Io(format!("{name}: file is too large to import")))?;
     if buf.frames() == 0 {
         return Err(EngineError::Io(format!("{name}: no audio")));
     }
@@ -245,22 +215,8 @@ mod rate_tests {
     #[test]
     fn a_tiny_sample_rate_is_refused_before_it_is_converted() {
         let buf = |rate| AudioBuffer { sample_rate: rate, channels: vec![vec![0.5; 100]; 2] };
-        assert!(at_rate(buf(1), 48_000, 1_000_000).is_err());
+        assert!(at_rate(buf(1), 48_000, 1_000_000).is_none());
         let ok = at_rate(buf(8_000), 48_000, 1_000_000).unwrap();
         assert_eq!((ok.sample_rate, ok.frames()), (48_000, 600));
     }
-
-    #[test]
-    fn an_extreme_rate_expansion_is_refused_whatever_the_sample_budget() {
-        let buf = |rate| AudioBuffer { sample_rate: rate, channels: vec![vec![0.5; 100]; 2] };
-        // 100 frames at 300 Hz would fit the budget as 128 000 samples, but grow 640-fold.
-        assert_eq!(at_rate(buf(300), 192_000, usize::MAX).unwrap_err(), RateError::Ratio { from: 300, to: 192_000 });
-        assert!(at_rate(buf(1_500), 192_000, usize::MAX).is_ok(), "128-fold is the supported extreme");
-        assert!(at_rate(buf(8_000), 768_000, usize::MAX).is_ok(), "the slowest file at the fastest session");
-        // 128.86-fold: integer division would truncate it to 128 and let it through
-        assert_eq!(at_rate(buf(1_490), 192_000, usize::MAX).unwrap_err(), RateError::Ratio { from: 1_490, to: 192_000 });
-        // a short file that is only over the sample budget is a size refusal, not a rate one
-        assert_eq!(at_rate(buf(48_000), 48_000, 10).unwrap_err(), RateError::TooLarge);
-        assert!(RateError::Ratio { from: 300, to: 192_000 }.message("big").contains("300 Hz"));
-    }
 }

crates/engine/src/io/session.rs

@@ -72,7 +72,7 @@ pub(crate) fn open_session_linked(e: &mut Engine, path: &Path, links: &Path) ->
 /// Decode one source's audio file at the session rate.
 pub(super) fn decode_source(bytes: &[u8], ext: Option<&str>, rate: u32) -> std::result::Result<Arc<SourceAudio>, String> {
     let (_, buf) = soundcraft_audio_io::decode(bytes, ext).map_err(|err| err.to_string())?;
-    let buf = at_rate(buf, rate, MAX_IMPORT_SAMPLES).map_err(|e| e.message("too large to load"))?;
+    let buf = at_rate(buf, rate, MAX_IMPORT_SAMPLES).ok_or("too large to load")?;
     Ok(Arc::new(SourceAudio::new(buf)))
 }