From 89b8db77185fddf8aee562b874f06731f4b57c9e Mon Sep 17 00:00:00 2001 From: aiongg <67543387+aiongg@users.noreply.github.com> Date: Fri, 23 Oct 2026 13:02:36 -0400 Subject: [PATCH] Footage, Lottie and models: resolve stored paths with craft-docpath's resolve_absolute (#1975) * Footage: use resolve_absolute for stored paths * Lottie and models: use resolve_absolute for linked files * CI: release only with git dependencies from storytold * Use the shared git-sources check and pin checkout in release.yml * Resolve a model's side files through resolve_absolute; report C:\x as invalid * CI: parse cargo config, including the legacy .cargo/config, in the git-sources check --- .github/workflows/release.yml | 40 +++++++++++++++++++++++++++++ crates/engine/src/doc_path.rs | 25 +++++++++++++----- crates/engine/src/doc_path/tests.rs | 16 ++++++++++++ crates/lottie/src/import.rs | 8 +++--- crates/media/src/pool.rs | 2 +- crates/media/src/probe.rs | 27 +++++++++++++++++-- 6 files changed, 105 insertions(+), 13 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 808e1628..4b630f9f 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -18,7 +18,47 @@ concurrency: cancel-in-progress: false jobs: + git-sources: + name: Git dependencies come from storytold + runs-on: ubuntu-latest + permissions: + contents: read + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + - name: Git dependencies come from storytold + run: | + set -euo pipefail + test -f Cargo.lock || { echo "Cargo.lock is missing"; exit 1; } + # Source replacement, patches and path overrides in cargo config never show up + # in Cargo.lock. Parsed rather than grepped: TOML has many spellings of one key. + # Cargo still reads the legacy .cargo/config. + python3 - <<'PY' + import os, sys, tomllib + for path in (".cargo/config", ".cargo/config.toml"): + if not os.path.exists(path): + continue + config = tomllib.load(open(path, "rb")) + replaced = [name for name, source in config.get("source", {}).items() + if isinstance(source, dict) and "replace-with" in source] + if replaced or "patch" in config or "paths" in config: + print(f"{path} replaces, patches or overrides a source") + sys.exit(1) + PY + # Fails when Cargo.lock is out of date with the manifests. + cargo metadata --locked --format-version 1 > /dev/null + python3 - <<'PY' + import sys, tomllib + lock = tomllib.load(open("Cargo.lock", "rb")) + bad = sorted({p["source"] for p in lock.get("package", []) + if p.get("source", "").startswith("git+") + and not p["source"].startswith("git+https://github.com/storytold/")}) + if bad: + print("Git dependencies outside github.com/storytold:", *bad, sep="\n") + sys.exit(1) + PY + release: + needs: git-sources # Intentionally tracked at @main (not SHA-pinned) so org-wide CI improvements, security fixes, and build optimizations propagate without PR churn across all craftapp repos. uses: storytold/craft-common-actions/.github/workflows/release.yml@main with: diff --git a/crates/engine/src/doc_path.rs b/crates/engine/src/doc_path.rs index c7416a8c..1f8e10e8 100644 --- a/crates/engine/src/doc_path.rs +++ b/crates/engine/src/doc_path.rs @@ -46,13 +46,24 @@ pub fn resolve(stored: &str, project: Option<&str>) -> Result + { + NotFollowed::NoProjectFolder + } + why => NotFollowed::Refused(why), + }) +} + +/// Whether `p` is a Windows drive or share path, which a project cannot make absolute outside Windows. +fn is_windows_rooted(p: &Path) -> bool { + let s = p.to_string_lossy(); + let b = s.as_bytes(); + (b.len() >= 2 && b[0].is_ascii_alphabetic() && b[1] == b':') || s.starts_with(r"\\") || s.starts_with("//") } /// The paths of `f` (its file and every frame of an image sequence) resolved against the document diff --git a/crates/engine/src/doc_path/tests.rs b/crates/engine/src/doc_path/tests.rs index 74680df7..d9d97124 100644 --- a/crates/engine/src/doc_path/tests.rs +++ b/crates/engine/src/doc_path/tests.rs @@ -149,7 +149,23 @@ fn stored_paths_resolve_against_the_project() { let doc = doc.to_str().unwrap(); assert_eq!(super::resolve("clip.mov", Some(doc)), Ok(std::env::temp_dir().join("art").join("clip.mov"))); assert_eq!(super::resolve("clip.mov", None), Err(super::NotFollowed::NoProjectFolder)); + assert_eq!(super::resolve("../logo.png", None), Err(super::NotFollowed::NoProjectFolder)); + // A drive path is not a local file on Unix, with or without a project: it is not a missing project. + #[cfg(unix)] + for stored in [r"C:\x.mov", "C:x.mov"] { + assert_eq!(super::resolve(stored, None), Err(super::NotFollowed::Refused(super::Refused::Invalid)), "{stored}"); + } assert_eq!(super::resolve(r"\\192.0.2.1\share\x.mov", Some(doc)), Err(super::NotFollowed::Refused(super::Refused::Network))); + assert_eq!(super::resolve("jar:clip.mov", Some(doc)), Err(super::NotFollowed::Refused(super::Refused::Url))); + // Windows reads a colon in a name as an alternate data stream; elsewhere it is a plain character. + if cfg!(windows) { + assert_eq!(super::resolve("shots/take:1.mov", Some(doc)), Err(super::NotFollowed::Refused(super::Refused::Invalid))); + } else { + assert_eq!(super::resolve("shots/take:1.mov", Some(doc)), Ok(std::env::temp_dir().join("art").join("shots").join("take:1.mov"))); + } + // A Windows drive path names no file on other platforms. + #[cfg(unix)] + assert_eq!(super::resolve(r"C:\art\clip.mov", Some(doc)), Err(super::NotFollowed::Refused(super::Refused::Invalid))); #[cfg(windows)] assert!(super::resolve(r"\\server\share\x.mov", Some(r"\\SERVER\share\job\intro.ecproj")).is_ok()); } diff --git a/crates/lottie/src/import.rs b/crates/lottie/src/import.rs index 7730017d..b2eeece4 100644 --- a/crates/lottie/src/import.rs +++ b/crates/lottie/src/import.rs @@ -96,9 +96,11 @@ fn external_image(stored: &str, base_dir: &str) -> Result { let doc = std::path::Path::new(base_dir).join("lottie.json"); std::path::absolute(&doc).unwrap_or(doc) }); - match craft_docpath::resolve(stored, doc.as_deref()) { - Ok(path) if path.is_absolute() => Ok(path.to_string_lossy().into_owned()), - Ok(_) => Err(format!("{stored} is relative and the Lottie file's folder is not known")), + match craft_docpath::resolve_absolute(stored, doc.as_deref()) { + Ok(path) => Ok(path.to_string_lossy().into_owned()), + Err(craft_docpath::Refused::Invalid) if doc.is_none() && craft_docpath::resolve(stored, None).is_ok() => { + Err(format!("{stored} is relative and the Lottie file's folder is not known")) + } Err(why) => Err(format!("{stored}: {why}")), } } diff --git a/crates/media/src/pool.rs b/crates/media/src/pool.rs index 9a8ec5c2..ffd65739 100644 --- a/crates/media/src/pool.rs +++ b/crates/media/src/pool.rs @@ -559,7 +559,7 @@ impl Inner { let loaded = self.read(path).map_err(|e| e.to_string()).and_then(|bytes| { // Files the model names are read only when local or on the model's own network share. let resolve = |uri: &str| -> Option> { - let p = craft_docpath::resolve(uri, Some(std::path::Path::new(path))).ok().filter(|p| p.is_absolute())?; + let p = craft_docpath::resolve_absolute(uri, Some(std::path::Path::new(path))).ok()?; self.read(&p.to_string_lossy()).ok().map(|b| b.to_vec()) }; effectcraft_model::load(path, &bytes, &resolve).map_err(|e| e.to_string()) diff --git a/crates/media/src/probe.rs b/crates/media/src/probe.rs index 2793be25..6bba682b 100644 --- a/crates/media/src/probe.rs +++ b/crates/media/src/probe.rs @@ -27,12 +27,21 @@ pub fn probe(path: impl AsRef) -> Result { } let bytes = std::fs::read(path).map_err(|e| MediaError::Io(format!("{}: {e}", path.display())))?; if crate::MODEL_EXTENSIONS.contains(&ext_of(path).as_str()) { - let dir = path.parent().map(Path::to_path_buf).unwrap_or_default(); - return probe_model(&path.to_string_lossy(), &bytes, &|uri| std::fs::read(dir.join(uri)).ok()); + return probe_model(&path.to_string_lossy(), &bytes, &side_file_reader(path)); } probe_bytes(&path.to_string_lossy(), bytes.into()) } +/// Reads the files a model file names (buffers, textures, MTL files). Like the pool, it follows only +/// local files and files on the model's own network share: a URI is a path stored in the model. +fn side_file_reader(model: &Path) -> impl Fn(&str) -> Option> + use<> { + let model = std::path::absolute(model).unwrap_or_else(|_| model.to_path_buf()); + move |uri| { + let p = craft_docpath::resolve_absolute(uri, Some(&model)).ok()?; + std::fs::read(p).ok() + } +} + /// Probe a 3D model (glTF 2.0 / OBJ / STL / FBX): a [`FootageKind::Model`] item whose duration is its /// longest animation. `resolve` reads sibling resources. pub fn probe_model(path: &str, bytes: &[u8], resolve: &dyn Fn(&str) -> Option>) -> Result { @@ -316,6 +325,20 @@ mod tests { (0..n).map(|k| (at(k + 1) - at(k)) as u32).collect() } + /// A model's side files are read from beside it; a URI to another computer is not followed. + #[test] + fn model_side_files_are_not_read_from_other_computers() { + let dir = std::env::temp_dir().join(format!("ec-probe-side-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + std::fs::write(dir.join("buf.bin"), b"abc").unwrap(); + let read = side_file_reader(&dir.join("model.gltf")); + assert_eq!(read("buf.bin"), Some(b"abc".to_vec())); + assert_eq!(read(r"\\192.0.2.1\share\buf.bin"), None); + assert_eq!(read("file://192.0.2.1/share/buf.bin"), None); + assert_eq!(read(r"C:\buf.bin"), None); + let _ = std::fs::remove_dir_all(&dir); + } + #[test] fn millisecond_rounded_frame_times_average_to_their_rate() { assert_eq!(rounded(FrameRate::FPS_30, 1000, 6), [33, 34, 33, 33, 34, 33]);