diff --git a/crates/rustmotion-core/src/engine/renderer/assets.rs b/crates/rustmotion-core/src/engine/renderer/assets.rs index ac7bd585..9121f48e 100644 --- a/crates/rustmotion-core/src/engine/renderer/assets.rs +++ b/crates/rustmotion-core/src/engine/renderer/assets.rs @@ -251,9 +251,43 @@ pub fn ffmpeg_available() -> bool { .unwrap_or(false) } +/// Rejects a `src` that names a network URL rather than a local file path. +/// +/// `extract_video_frame` below hands `src` to `ffmpeg -i` verbatim; ffmpeg's +/// own demuxer understands its full built-in protocol set (`http://`, +/// `rtmp://`, `concat:`, …), which turns an unfiltered `src` into an SSRF +/// primitive — a scenario author can point it at +/// `http://169.254.169.254/...` (the cloud metadata endpoint) or an internal +/// service, and read the exit status as a port-scan oracle. Remote +/// video was never a designed feature here — this module's own `is_remote` +/// doc, a few functions below, and `rustmotion info`'s identical assumption +/// both already treat every `src` as a local path — so this closes an +/// accidental reach rather than opening an allowlist for one. +fn reject_remote_video_src(src: &str) -> Result<()> { + let Some(scheme_end) = src.find("://") else { + return Ok(()); + }; + let scheme = &src[..scheme_end]; + let looks_like_scheme = !scheme.is_empty() + && scheme.starts_with(|c: char| c.is_ascii_alphabetic()) + && scheme + .chars() + .all(|c| c.is_ascii_alphanumeric() || matches!(c, '+' | '-' | '.')); + if looks_like_scheme { + return Err(RustmotionError::Generic(format!( + "video src '{src}' names a '{scheme}://' URL — rustmotion does not fetch video \ + over the network, only local file paths are accepted" + ))); + } + Ok(()) +} + pub fn extract_video_frame(src: &str, time: f64, width: u32, height: u32) -> Result> { + reject_remote_video_src(src)?; let output = std::process::Command::new("ffmpeg") .args([ + "-protocol_whitelist", + "file", "-ss", &format!("{:.3}", time), "-i", diff --git a/crates/rustmotion-core/tests/audit_ws_h.rs b/crates/rustmotion-core/tests/audit_ws_h.rs index 0e8d7242..0fd3a042 100644 --- a/crates/rustmotion-core/tests/audit_ws_h.rs +++ b/crates/rustmotion-core/tests/audit_ws_h.rs @@ -25,15 +25,23 @@ fn shared_http_agent_has_finite_global_and_connect_timeouts() { #[test] fn extract_video_frame_rejects_a_remote_src_before_it_ever_reaches_ffmpeg() { - let result = extract_video_frame( + // Asserting `is_err()` alone would also pass for the wrong reason: on a + // machine with no route to this address, ffmpeg itself fails to connect + // and returns a non-zero exit status. The fix under test is that the + // src is refused *before* any subprocess runs at all — so the assertion + // has to be on the specific rejection message, which only the fix + // produces; an ffmpeg spawn/exit failure would carry a different one. + let err = extract_video_frame( "http://169.254.169.254/latest/meta-data/iam/security-credentials/", 0.0, 16, 16, - ); + ) + .expect_err("a scheme-prefixed src must be rejected outright, not handed to ffmpeg"); + let msg = err.to_string(); assert!( - result.is_err(), - "a scheme-prefixed src must be rejected outright, not hand ffmpeg a URL to dereference" + msg.contains("does not fetch video over the network"), + "expected the scheme-rejection error, not an ffmpeg spawn/exit failure: {msg}" ); } @@ -42,7 +50,8 @@ fn extract_video_frame_does_not_reject_a_plain_local_path() { let result = extract_video_frame("/no/such/file/on/disk.mp4", 0.0, 16, 16); let err = result.expect_err("a missing local file is still an error"); assert!( - !err.to_string().contains("does not fetch video"), + !err.to_string() + .contains("does not fetch video over the network"), "a plain local path must fail on ffmpeg/the missing file, not on the scheme check: {err}" ); }