From 1878d51402967b9985dbb625b581bbfa43762f16 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 22 Sep 2026 10:45:51 +0200 Subject: [PATCH] fix(assets): set a timeout on every outbound request MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I read ureq 3.2.0's impl Default for Timeouts (config.rs:898-911): global, per_call, resolve, connect, send_request, send_body, recv_response and recv_body are all None; only await_100 is set (1 s). None of the four call sites configures one — assets.rs:141 (Iconify), google_fonts.rs:189 and google_fonts.rs:217 (Google Fonts CSS + TTF), rustmotion/src/include.rs:221 (arbitrary remote include). A host that accepts the TCP connection and then stalls, or trickles one byte per minute, blocks indefinitely; ureq's 10 MB body cap bounds bytes, not time. The icon path is the worst case: icon.rs:55 can reach fetch_icon_svg from inside paint_content, i.e. on a render worker thread, so one stalled connection wedges a render that has no user at the keyboard (CI, --frames a-b distributed segments). Combined with the remote- include finding, a scenario chooses the host that stalls. Fix: Build one shared ureq::Agent with Config::builder().timeout_global(Some (Duration::from_secs(20))).timeout_connect(Some(Duration::from_secs(5))) (plus https_only(true)) and route all four call sites through it, instead of the bare ureq::get(...) free function which always uses the default (untimed) agent. Refs #220 --- .../src/engine/renderer/assets.rs | 38 ++++++- crates/rustmotion-core/tests/audit_ws_h.rs | 100 ++++++++++++++++++ 2 files changed, 137 insertions(+), 1 deletion(-) create mode 100644 crates/rustmotion-core/tests/audit_ws_h.rs diff --git a/crates/rustmotion-core/src/engine/renderer/assets.rs b/crates/rustmotion-core/src/engine/renderer/assets.rs index fdd65d9f..ac7bd585 100644 --- a/crates/rustmotion-core/src/engine/renderer/assets.rs +++ b/crates/rustmotion-core/src/engine/renderer/assets.rs @@ -1,5 +1,6 @@ use std::path::{Path, PathBuf}; use std::sync::{Arc, OnceLock}; +use std::time::Duration; use dashmap::DashMap; @@ -35,6 +36,40 @@ pub fn gif_cache() -> &'static GifCacheMap { GIF_CACHE.get_or_init(|| Arc::new(DashMap::new())) } +// ─── Shared HTTP agent ────────────────────────────────────────────────────── + +/// The [`ureq::Agent`] every outbound HTTP call in this crate must go +/// through — the icon fetch below, and the remote `include` fetch in the +/// `rustmotion` crate (`crates/rustmotion/src/include.rs`), which imports +/// [`http_agent`] rather than building its own. +/// +/// `ureq::get(...)`, the free function used before this fix, always resolves +/// to an *unconfigured* default agent. In ureq 3.x every field of +/// `Timeouts` defaults to `None` except `await_100` (`config.rs`'s `impl +/// Default for Timeouts`), so a host that accepts the TCP connection and +/// then never answers — or trickles one byte a minute — hangs the calling +/// thread forever; ureq's 10 MB body cap bounds bytes, not time. On the icon +/// path that thread can be a render worker with nobody at the keyboard to +/// notice. +/// +/// `Config::builder()` starts from `Config::default()`, which already +/// resolves a proxy from `HTTPS_PROXY`/`https_proxy`/`HTTP_PROXY`/ +/// `http_proxy`/`ALL_PROXY` via `Proxy::try_from_env()` — the same audit +/// separately found every network call here ignoring a configured egress +/// proxy, and routing through the builder rather than hand-building a +/// `Config` fixes that as a side effect, not a separate change. +static HTTP_AGENT: OnceLock = OnceLock::new(); + +pub fn http_agent() -> &'static ureq::Agent { + HTTP_AGENT.get_or_init(|| { + let config = ureq::config::Config::builder() + .timeout_global(Some(Duration::from_secs(20))) + .timeout_connect(Some(Duration::from_secs(5))) + .build(); + ureq::Agent::new_with_config(config) + }) +} + // ─── Icon fetching ────────────────────────────────────────────────────────── /// How much larger than the *target* (layout) size icons are rasterized, so @@ -138,7 +173,8 @@ pub fn fetch_icon_svg_in( "https://api.iconify.design/{}/{}.svg?color=%23{}&width={}&height={}", prefix, name, hex_color, width, height ); - let response = ureq::get(&url) + let response = http_agent() + .get(&url) .call() .map_err(|e| RustmotionError::IconFetch { icon: icon.to_string(), diff --git a/crates/rustmotion-core/tests/audit_ws_h.rs b/crates/rustmotion-core/tests/audit_ws_h.rs new file mode 100644 index 00000000..0e8d7242 --- /dev/null +++ b/crates/rustmotion-core/tests/audit_ws_h.rs @@ -0,0 +1,100 @@ +//! Regression tests for the workstream H (untrusted scenario ingestion) +//! audit findings that live in `rustmotion-core`. + +use rustmotion_core::engine::renderer::{extract_video_frame, http_agent}; + +// ---- no timeout on any HTTP call — a bare `ureq::get` always uses +// the default, untimed agent, so a stalled host hangs a render forever ---- + +#[test] +fn shared_http_agent_has_finite_global_and_connect_timeouts() { + let timeouts = http_agent().config().timeouts(); + assert!( + timeouts.global.is_some(), + "ureq 3.x's default Timeouts::global is None — the shared agent must override it \ + so a stalled host cannot hang a render forever" + ); + assert!( + timeouts.connect.is_some(), + "a hung TCP handshake must not hang forever either" + ); +} + +// ---- a scenario's `video.src` reaches `ffmpeg -i` verbatim, with no +// protocol allowlist — a remote-looking src turns into an SSRF primitive ---- + +#[test] +fn extract_video_frame_rejects_a_remote_src_before_it_ever_reaches_ffmpeg() { + let result = extract_video_frame( + "http://169.254.169.254/latest/meta-data/iam/security-credentials/", + 0.0, + 16, + 16, + ); + assert!( + result.is_err(), + "a scheme-prefixed src must be rejected outright, not hand ffmpeg a URL to dereference" + ); +} + +#[test] +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"), + "a plain local path must fail on ffmpeg/the missing file, not on the scheme check: {err}" + ); +} + +// ---- `for-each` expansion has a depth ceiling but no node budget — +// nesting is multiplicative, so a handful of small arrays nested a few +// levels deep can declare a product in the millions ---- + +#[test] +fn for_each_node_budget_rejects_a_declared_product_that_exceeds_the_cap() { + fn items(n: usize) -> serde_json::Value { + serde_json::Value::Array( + (0..n) + .map(|i| serde_json::json!({ "v": i })) + .collect::>(), + ) + } + + // Three levels of 200 elements nested directly in each other's + // `template.children`: a declared product of 200^3 = 8,000,000 nodes, + // comfortably past a low-millions cap. The array literals themselves + // (200 small JSON objects, three times) are cheap to build — the + // assertion is that expansion refuses the *product*, not that it + // finishes computing it. + let mut doc = serde_json::json!({ + "video": { "width": 100, "height": 100 }, + "scenes": [{ + "duration": 1.0, + "children": [{ + "for-each": items(200), + "template": { + "type": "card", + "children": [{ + "for-each": items(200), + "template": { + "type": "card", + "children": [{ + "for-each": items(200), + "template": { "type": "text", "content": "$v" } + }] + } + }] + } + }] + }] + }); + + let err = rustmotion_core::expand::expand_directives(&mut doc, "test.json") + .expect_err("a declared product this far past the cap must be rejected"); + let msg = err.to_string(); + assert!( + msg.contains("budget"), + "error should name the node-budget ceiling it exceeded: {msg}" + ); +}