From 14b345c333aa33bb2ed5f75aecced3abb15b2bd7 Mon Sep 17 00:00:00 2001 From: Anubhav Rai Date: Tue, 27 Oct 2026 23:34:41 +0530 Subject: [PATCH 1/3] engine: a template's footage entry can't name a file outside the folder (#2123) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit EffectCraft writes its own templates as `media//` entries, and extraction trusted both halves. `base_name` splits on `/` and `\`, but the item id was taken with `split('/')` alone, and it is pasted in front of a base name another item already used. So a second `media/..\..\..\Startup\x/clip.mp4` entry became `/..\..\..\Startup\x-clip.mp4`, which on Windows is three folders up, with the attacker choosing how the name ends. The id is now used only when it is one, and the name goes through `safe_file_name`: separators, drive colons and NUL become `_`, and `.` / `..` become `footage`. Reached from `templates.create` with `path`, from File ▸ New from Template, and from a template dropped in the user Templates folder. Co-Authored-By: Claude Opus 5 --- crates/engine/src/home/templates.rs | 53 +++++++++++++++++++++++++++-- 1 file changed, 51 insertions(+), 2 deletions(-) diff --git a/crates/engine/src/home/templates.rs b/crates/engine/src/home/templates.rs index 500308b72..21b4b61d8 100644 --- a/crates/engine/src/home/templates.rs +++ b/crates/engine/src/home/templates.rs @@ -645,6 +645,15 @@ fn base_name(path: &str) -> String { path.rsplit(['/', '\\']).next().filter(|n| !n.is_empty()).unwrap_or("footage").to_string() } +/// A template entry's name, made safe to write inside a folder: a template is a file like any +/// other and can name its entries anything. Nothing a platform reads as a separator, a drive or +/// the folder above survives, and a name that leaves nothing usable becomes `footage` (#2123). +fn safe_file_name(name: &str) -> String { + let name: String = name.chars().map(|c| if matches!(c, '/' | '\\' | ':' | '\0') { '_' } else { c }).collect(); + // `.` and `..`, and names of dots and spaces alone (Windows drops the trailing ones). + if name.trim_matches(['.', ' ']).is_empty() { "footage".to_string() } else { name } +} + /// What [`embed_footage`] did. #[derive(Default)] struct Embedded { @@ -738,8 +747,10 @@ fn extract_footage(s: &Session, proj: &mut Project, entries: &[(String, Vec) let mut used = std::collections::HashSet::new(); for (k, v) in entries.iter().filter(|(k, _)| k.starts_with("media/")) { // `media//` → one folder; a name another item took gets the item's id. - let file = base_name(k); - let item = k.trim_start_matches("media/").split('/').next().unwrap_or("0"); + let file = safe_file_name(&base_name(k)); + // The item's id, and only that: it is written as one ([`embed_footage`]), but a template + // from elsewhere can hold anything there, and `..\` in it walks out of `dir` on Windows. + let item = k.trim_start_matches("media/").split('/').next().filter(|i| i.bytes().all(|b| b.is_ascii_digit())).unwrap_or("0"); let name = if used.insert(file.clone()) { file } else { format!("{item}-{file}") }; let out = format!("{dir}/{name}"); s.services.store_file(&out, v).map_err(|e| EngineError::Other(format!("cannot write {out}: {e}")))?; @@ -970,6 +981,44 @@ mod tests { assert_eq!(l.iter().find(|t| t.id == "lower-third").unwrap().controls, ["Name", "Title", "Bar Color"]); } + /// #2123: a template holds its footage as `media//` entries, but a template + /// from anywhere else can name them anything — and `..\\`, which only this platform reads as + /// a plain character, is a separator on Windows, so a second entry with a base name already + /// taken wrote `/..\\..\\..\\Startup\\x-evil.bat`. Whatever an entry is called, the file + /// lands in the footage folder. + #[test] + fn extracted_footage_stays_in_its_folder() { + let s = Session::default(); + let mut proj = effectcraft_project::Project::default(); + let dir = std::env::temp_dir().join(format!("ec-templates-media-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + let ds = dir.to_string_lossy().into_owned(); + let entry = |k: &str| (k.to_string(), b"x".to_vec()); + let entries = [ + entry("media/0/clip.mp4"), + // The base name is taken, so this one's "item id" is used — and it is not one. + entry("media/..\\..\\..\\Startup\\x/clip.mp4"), + entry("media/1/C:evil.bat"), + ]; + let written = extract_footage(&s, &mut proj, &entries, &ds).unwrap(); + assert_eq!(written.len(), entries.len()); + for w in &written { + let name = w.strip_prefix(&format!("{ds}/")).unwrap_or_else(|| panic!("{w} is outside {ds}")); + assert!(!name.contains(['/', '\\', ':']), "{w}"); + assert!(!name.trim_matches(['.', ' ']).is_empty(), "{w}"); + } + // One file each, all of them in the folder. + assert_eq!(std::fs::read_dir(&dir).map(|d| d.count()).unwrap_or(0), entries.len()); + let _ = std::fs::remove_dir_all(&dir); + // And the names that would leave nothing to write. + for n in ["..", ".", " .. ", "...", ""] { + assert_eq!(safe_file_name(n), "footage", "{n:?}"); + } + assert_eq!(safe_file_name("clip.mp4"), "clip.mp4"); + assert_eq!(safe_file_name(".hidden.mp4"), ".hidden.mp4"); + assert_eq!(safe_file_name("a/b\\c:d"), "a_b_c_d"); + } + #[test] fn create_opens_an_untitled_copy() { let mut s = Session::default(); From a89fbdab8f0d83db3c7128fb5cbb6957e6bfb6c9 Mon Sep 17 00:00:00 2001 From: Anubhav Rai Date: Wed, 28 Oct 2026 00:05:19 +0530 Subject: [PATCH 2/3] engine: two template footage entries can't become one file (#2123) The fallback name was never checked against the names already taken, so an entry called exactly what the fallback produces took the same file: `media/1/a:b.png`, `media/2/a_b.png` and `media/3/2-a_b.png` extracted to five files for six entries, the third overwriting the second, and both items' footage pointing at it. Deduplication now runs on the name actually written and counts up until it is free. The collision predates making the names safe (two entries named `a_b.png` and `2-a_b.png` collide on their own), but sanitising gives more ways in, so it belongs with it. Found by CodeRabbit on the pull request. Co-Authored-By: Claude Opus 5 --- crates/engine/src/home/templates.rs | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/crates/engine/src/home/templates.rs b/crates/engine/src/home/templates.rs index 21b4b61d8..515178730 100644 --- a/crates/engine/src/home/templates.rs +++ b/crates/engine/src/home/templates.rs @@ -751,7 +751,14 @@ fn extract_footage(s: &Session, proj: &mut Project, entries: &[(String, Vec) // The item's id, and only that: it is written as one ([`embed_footage`]), but a template // from elsewhere can hold anything there, and `..\` in it walks out of `dir` on Windows. let item = k.trim_start_matches("media/").split('/').next().filter(|i| i.bytes().all(|b| b.is_ascii_digit())).unwrap_or("0"); - let name = if used.insert(file.clone()) { file } else { format!("{item}-{file}") }; + // A name another entry has taken gets the item's id, and a number if that is taken too: + // the names an entry can ask for are not distinct, and two files must not become one. + let mut name = file.clone(); + let mut n = 0usize; + while !used.insert(name.clone()) { + n += 1; + name = if n == 1 { format!("{item}-{file}") } else { format!("{item}-{n}-{file}") }; + } let out = format!("{dir}/{name}"); s.services.store_file(&out, v).map_err(|e| EngineError::Other(format!("cannot write {out}: {e}")))?; if let Some(imp) = &s.importer { @@ -999,6 +1006,10 @@ mod tests { // The base name is taken, so this one's "item id" is used — and it is not one. entry("media/..\\..\\..\\Startup\\x/clip.mp4"), entry("media/1/C:evil.bat"), + // Three that collide once the first is made safe: each still gets its own file. + entry("media/1/a:b.png"), + entry("media/2/a_b.png"), + entry("media/3/2-a_b.png"), ]; let written = extract_footage(&s, &mut proj, &entries, &ds).unwrap(); assert_eq!(written.len(), entries.len()); @@ -1007,7 +1018,8 @@ mod tests { assert!(!name.contains(['/', '\\', ':']), "{w}"); assert!(!name.trim_matches(['.', ' ']).is_empty(), "{w}"); } - // One file each, all of them in the folder. + // One file each, all of them in the folder, none overwritten by the next. + assert_eq!(written.iter().collect::>().len(), entries.len(), "{written:?}"); assert_eq!(std::fs::read_dir(&dir).map(|d| d.count()).unwrap_or(0), entries.len()); let _ = std::fs::remove_dir_all(&dir); // And the names that would leave nothing to write. From 06a1e6d27b7ee4e3e3d26ab1958d98ac0087aa2f Mon Sep 17 00:00:00 2001 From: Anubhav Rai Date: Wed, 28 Oct 2026 00:32:34 +0530 Subject: [PATCH 3/3] engine: entry names are compared the way a filesystem might (#2123) Two names that differ only in case are one file on Windows and on macOS as they come, and Win32 drops a trailing dot or space, so `clip.png` with `CLIP.png`, and `a.png` with `a.png.`, still landed on each other: ten entries extracted to nine files on APFS, one footage item holding the other's picture. Collisions are now looked up by a key with the case folded and those characters off the end, while the name written stays as the entry asked. Found by CodeRabbit on the pull request. Co-Authored-By: Claude Opus 5 --- crates/engine/src/home/templates.rs | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/crates/engine/src/home/templates.rs b/crates/engine/src/home/templates.rs index 515178730..0b15baaf5 100644 --- a/crates/engine/src/home/templates.rs +++ b/crates/engine/src/home/templates.rs @@ -654,6 +654,12 @@ fn safe_file_name(name: &str) -> String { if name.trim_matches(['.', ' ']).is_empty() { "footage".to_string() } else { name } } +/// What two entry names must differ by to be sure of two files: a filesystem may ignore case, and +/// Win32 ignores trailing dots and spaces. +fn same_file_key(name: &str) -> String { + name.trim_end_matches(['.', ' ']).to_lowercase() +} + /// What [`embed_footage`] did. #[derive(Default)] struct Embedded { @@ -753,9 +759,12 @@ fn extract_footage(s: &Session, proj: &mut Project, entries: &[(String, Vec) let item = k.trim_start_matches("media/").split('/').next().filter(|i| i.bytes().all(|b| b.is_ascii_digit())).unwrap_or("0"); // A name another entry has taken gets the item's id, and a number if that is taken too: // the names an entry can ask for are not distinct, and two files must not become one. + // They are compared the way a filesystem might: `CLIP.png` is `clip.png` on a + // case-insensitive volume (Windows and macOS as they come), and Win32 drops a trailing + // dot or space, so `a.png.` is `a.png` there. The name written stays as it is. let mut name = file.clone(); let mut n = 0usize; - while !used.insert(name.clone()) { + while !used.insert(same_file_key(&name)) { n += 1; name = if n == 1 { format!("{item}-{file}") } else { format!("{item}-{n}-{file}") }; } @@ -1010,6 +1019,11 @@ mod tests { entry("media/1/a:b.png"), entry("media/2/a_b.png"), entry("media/3/2-a_b.png"), + // The same file on a case-insensitive volume, and on Windows, which drops the dot. + entry("media/4/clip.png"), + entry("media/5/CLIP.png"), + entry("media/6/a.png"), + entry("media/7/a.png."), ]; let written = extract_footage(&s, &mut proj, &entries, &ds).unwrap(); assert_eq!(written.len(), entries.len()); @@ -1029,6 +1043,9 @@ mod tests { assert_eq!(safe_file_name("clip.mp4"), "clip.mp4"); assert_eq!(safe_file_name(".hidden.mp4"), ".hidden.mp4"); assert_eq!(safe_file_name("a/b\\c:d"), "a_b_c_d"); + // Names a filesystem may not tell apart (only the case half of this shows on macOS). + assert_eq!(same_file_key("A.PNG. "), same_file_key("a.png")); + assert_ne!(same_file_key("a.png"), same_file_key("b.png")); } #[test]