fix(include): require opt-in for remote includes

crates/rustmotion/src/include.rs

@@ -1,3 +1,4 @@
+use std::net::{IpAddr, SocketAddr, ToSocketAddrs};
 use std::path::{Path, PathBuf};
 
 use crate::error::Result;
@@ -11,6 +12,30 @@ use crate::schema::{
 
 const MAX_INCLUDE_DEPTH: u8 = 8;
 
+/// Response-size cap for a remote `include` fetch. Scenario JSON is
+/// not expected to be large; this is deliberately far below ureq's own 10 MB
+/// default for `read_to_vec`/`read_to_string`.
+const MAX_REMOTE_INCLUDE_BYTES: u64 = 4 * 1024 * 1024;
+
+/// The CLI flag whose semantics this module implements the mechanism for:
+/// remote `include` is denied unless a caller explicitly opts in by passing
+/// `RemoteIncludePolicy::Allow` to [`resolve_includes_with_policy`]. Naming
+/// it here keeps the error message and the flag rustmotion's CLI is expected
+/// to expose in sync.
+const ALLOW_REMOTE_INCLUDE_FLAG: &str = "--allow-remote-include";
+
+/// Whether a [`resolve_includes_with_policy`] pass may perform outbound
+/// network requests for `include: "https://..."` directives ( remote
+/// fetching is deliberate design, but it previously had no allowlist and no
+/// opt-out — any scenario, including one merely being `validate`d, could
+/// make this process issue arbitrary GETs). Defaults to [`Self::Deny`].
+#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)]
+pub enum RemoteIncludePolicy {
+    #[default]
+    Deny,
+    Allow,
+}
+
 /// Where the parent scenario was loaded from — determines how relative paths are resolved.
 pub enum IncludeSource {
     /// Loaded from a file; relative paths resolve against this file's directory.
@@ -20,7 +45,23 @@ pub enum IncludeSource {
 }
 
 /// Expand all include directives in a scenario, producing resolved views.
+///
+/// Remote (`http(s)://`) includes are denied by default — see
+/// [`resolve_includes_with_policy`] to opt in. This is exactly
+/// `resolve_includes_with_policy(scenario, source, RemoteIncludePolicy::Deny)`,
+/// kept as its own entry point so every existing caller stays secure by
+/// default without having to be rewritten to pass a policy.
 pub fn resolve_includes(scenario: Scenario, source: &IncludeSource) -> Result<ResolvedScenario> {
+    resolve_includes_with_policy(scenario, source, RemoteIncludePolicy::Deny)
+}
+
+/// Same as [`resolve_includes`], with explicit control over whether remote
+/// `include` directives may reach the network.
+pub fn resolve_includes_with_policy(
+    scenario: Scenario,
+    source: &IncludeSource,
+    remote_policy: RemoteIncludePolicy,
+) -> Result<ResolvedScenario> {
     let mut audio = scenario.audio;
     let mut included_paths = Vec::new();
     let has_scenes = !scenario.scenes.is_empty();
@@ -36,8 +77,14 @@ pub fn resolve_includes(scenario: Scenario, source: &IncludeSource) -> Result<Re
         // New format: composition with views
         let mut views = Vec::with_capacity(composition.len());
         for view in composition {
-            let mut scenes =
-                resolve_entries(view.scenes, source, 0, &mut audio, &mut included_paths)?;
+            let mut scenes = resolve_entries(
+                view.scenes,
+                source,
+                0,
+                &mut audio,
+                &mut included_paths,
+                remote_policy,
+            )?;
             for scene in &mut scenes {
                 resolve_scene_background(scene, templates)?;
             }
@@ -58,8 +105,14 @@ pub fn resolve_includes(scenario: Scenario, source: &IncludeSource) -> Result<Re
         views
     } else {
         // Backward compat: wrap top-level scenes in a single slide view
-        let mut scenes =
-            resolve_entries(scenario.scenes, source, 0, &mut audio, &mut included_paths)?;
+        let mut scenes = resolve_entries(
+            scenario.scenes,
+            source,
+            0,
+            &mut audio,
+            &mut included_paths,
+            remote_policy,
+        )?;
         for scene in &mut scenes {
             resolve_scene_background(scene, templates)?;
         }
@@ -88,6 +141,7 @@ fn resolve_entries(
     depth: u8,
     audio: &mut Vec<crate::schema::AudioTrack>,
     included_paths: &mut Vec<PathBuf>,
+    remote_policy: RemoteIncludePolicy,
 ) -> Result<Vec<Scene>> {
     let mut result = Vec::new();
 
@@ -103,8 +157,14 @@ fn resolve_entries(
                         path: directive.include.clone(),
                     });
                 }
-                let scenes =
-                    fetch_and_resolve(&directive, source, depth + 1, audio, included_paths)?;
+                let scenes = fetch_and_resolve(
+                    &directive,
+                    source,
+                    depth + 1,
+                    audio,
+                    included_paths,
+                    remote_policy,
+                )?;
                 result.extend(scenes);
             }
         }
@@ -119,12 +179,22 @@ fn fetch_and_resolve(
     depth: u8,
     audio: &mut Vec<crate::schema::AudioTrack>,
     included_paths: &mut Vec<PathBuf>,
+    remote_policy: RemoteIncludePolicy,
 ) -> Result<Vec<Scene>> {
     let is_remote =
         directive.include.starts_with("http://") || directive.include.starts_with("https://");
 
     let (json_str, child_source) = if is_remote {
-        let body = fetch_remote(&directive.include)?;
+        if remote_policy != RemoteIncludePolicy::Allow {
+            return Err(RustmotionError::Generic(format!(
+                "include '{}' is a remote URL, but remote includes are disabled by default \
+ — pass {ALLOW_REMOTE_INCLUDE_FLAG} to opt in",
+                directive.include
+            )));
+        }
+        let verified = verify_remote_url(&directive.include)?;
+        eprintln!("include: fetching remote scenario {}", verified.as_str());
+        let body = fetch_remote(&verified)?;
         let child_source = IncludeSource::File(PathBuf::from(&directive.include));
         (body, child_source)
     } else {
@@ -178,6 +248,7 @@ fn fetch_and_resolve(
         depth,
         audio,
         included_paths,
+        remote_policy,
     )?;
 
     // Apply scene index filter if specified
@@ -217,22 +288,130 @@ fn resolve_local_path(relative: &str, source: &IncludeSource) -> Result<PathBuf>
     }
 }
 
-fn fetch_remote(url: &str) -> Result<String> {
-    let response = ureq::get(url)
+/// A remote `include` URL that has already passed the SSRF policy check in
+/// [`verify_remote_url`] — every address its host resolves to was confirmed
+/// public. This is the only way to reach [`fetch_remote`]: an unchecked
+/// `&str` cannot be passed to it, by construction.
+struct VerifiedRemoteUrl(String);
+
+impl VerifiedRemoteUrl {
+    fn as_str(&self) -> &str {
+        &self.0
+    }
+}
+
+/// Resolves `url`'s host and rejects it if any resolved address is not
+/// publicly routable: loopback, link-local (169.254.169.254, the cloud
+/// metadata endpoint, included), or an RFC1918/ULA private range.
+///
+/// This check runs once, ahead of the request `fetch_remote` makes moments
+/// later; a DNS answer that changes between this resolution and that
+/// connection ("DNS rebinding") is not defended against — the audit's
+/// remediation asks for a resolve-time check, and closing the rebinding gap
+/// fully would mean replacing `ureq`'s connector rather than configuring it.
+fn verify_remote_url(url: &str) -> Result<VerifiedRemoteUrl> {
+    let (host, port) = split_host_port(url)?;
+    let addrs: Vec<SocketAddr> = (host.as_str(), port)
+        .to_socket_addrs()
+        .map_err(|e| {
+            RustmotionError::Generic(format!("include '{url}' could not be resolved: {e}"))
+        })?
+        .collect();
+    if addrs.is_empty() {
+        return Err(RustmotionError::Generic(format!(
+            "include '{url}' resolved to no addresses"
+        )));
+    }
+    for addr in &addrs {
+        let ip = addr.ip().to_canonical();
+        if is_non_public(ip) {
+            return Err(RustmotionError::Generic(format!(
+                "include '{url}' resolves to {ip}, which is not a public address \
+                 (loopback/link-local/private range) — refusing to prevent SSRF"
+            )));
+        }
+    }
+    Ok(VerifiedRemoteUrl(url.to_string()))
+}
+
+fn is_non_public(ip: IpAddr) -> bool {
+    match ip {
+        IpAddr::V4(v4) => {
+            v4.is_loopback()
+                || v4.is_private()
+                || v4.is_link_local()
+                || v4.is_unspecified()
+                || v4.is_broadcast()
+                || v4.is_documentation()
+                || v4.is_multicast()
+        }
+        IpAddr::V6(v6) => {
+            v6.is_loopback()
+                || v6.is_unspecified()
+                || v6.is_unique_local()
+                || v6.is_unicast_link_local()
+                || v6.is_multicast()
+        }
+    }
+}
+
+/// Splits `scheme://[user:pass@]host[:port][/...]` into `(host, port)`,
+/// defaulting the port from the scheme. Deliberately minimal — this crate
+/// takes no `url` dependency for this, and only needs to know where to point
+/// the resolver; `ureq` itself is still the one that rejects a malformed URL
+/// when the actual request is made.
+fn split_host_port(url: &str) -> Result<(String, u16)> {
+    let (rest, default_port) = if let Some(rest) = url.strip_prefix("https://") {
+        (rest, 443)
+    } else if let Some(rest) = url.strip_prefix("http://") {
+        (rest, 80)
+    } else {
+        return Err(RustmotionError::Generic(format!(
+            "include '{url}' is not an http(s) URL"
+        )));
+    };
+    let authority = rest.split(['/', '?', '#']).next().unwrap_or(rest);
+    let authority = authority.rsplit('@').next().unwrap_or(authority);
+
+    if let Some(rest) = authority.strip_prefix('[') {
+        let end = rest.find(']').ok_or_else(|| {
+            RustmotionError::Generic(format!("include '{url}' has an unterminated IPv6 host"))
+        })?;
+        let host = rest[..end].to_string();
+        let port = rest[end + 1..]
+            .strip_prefix(':')
+            .and_then(|p| p.parse().ok())
+            .unwrap_or(default_port);
+        return Ok((host, port));
+    }
+
+    match authority.rsplit_once(':') {
+        Some((host, port_str)) => match port_str.parse() {
+            Ok(port) => Ok((host.to_string(), port)),
+            Err(_) => Ok((authority.to_string(), default_port)),
+        },
+        None => Ok((authority.to_string(), default_port)),
+    }
+}
+
+fn fetch_remote(url: &VerifiedRemoteUrl) -> Result<String> {
+    let response = rustmotion_core::engine::renderer::http_agent()
+        .get(url.as_str())
         .call()
         .map_err(|e| RustmotionError::IncludeRemoteFetch {
-            url: url.to_string(),
+            url: url.as_str().to_string(),
             reason: e.to_string(),
         })?;
-    let body =
-        response
-            .into_body()
-            .read_to_string()
-            .map_err(|e| RustmotionError::IncludeRemoteFetch {
-                url: url.to_string(),
-                reason: e.to_string(),
-            })?;
-    Ok(body)
+    response
+        .into_body()
+        .with_config()
+        .limit(MAX_REMOTE_INCLUDE_BYTES)
+        .lossy_utf8(true)
+        .read_to_string()
+        .map_err(|e| RustmotionError::IncludeRemoteFetch {
+            url: url.as_str().to_string(),
+            reason: e.to_string(),
+        })
 }
 
 // --- Background template resolution ---

crates/rustmotion/tests/audit_ws_h.rs

@@ -0,0 +1,90 @@
+//! Regression tests for the workstream H (untrusted scenario ingestion)
+//! audit finding that lives in the `rustmotion` crate:.
+//!
+//! Remote fetching for `include: "https://..."` is deliberate design; the
+//! finding is the absence of any control around it. These tests exercise
+//! `include::resolve_includes`/`resolve_includes_with_policy` directly
+//! rather than through `rustmotion::loader`, so they do not depend on the
+//! CLI wiring a sibling workstream owns.
+
+use rustmotion::include::{
+    resolve_includes, resolve_includes_with_policy, IncludeSource, RemoteIncludePolicy,
+};
+use rustmotion::schema::Scenario;
+
+fn scenario_with_remote_include(url: &str) -> Scenario {
+    serde_json::from_value(serde_json::json!({
+        "video": { "width": 100, "height": 100 },
+        "scenes": [{ "include": url }]
+    }))
+    .expect("scenario with a remote include parses")
+}
+
+// ----, part 1: remote includes are denied by default ----
+
+#[test]
+fn remote_include_is_denied_by_default() {
+    let scenario = scenario_with_remote_include("https://example.invalid/scenario.json");
+    let err = resolve_includes(scenario, &IncludeSource::Inline)
+        .expect_err("a remote include must be refused unless explicitly opted into");
+    let msg = err.to_string();
+    assert!(
+        msg.contains("disabled by default"),
+        "the denial must explain that remote includes are opt-in: {msg}"
+    );
+}
+
+#[test]
+fn remote_include_denial_names_the_opt_in() {
+    let scenario = scenario_with_remote_include("https://example.invalid/scenario.json");
+    let err =
+        resolve_includes_with_policy(scenario, &IncludeSource::Inline, RemoteIncludePolicy::Deny)
+            .expect_err("Deny must refuse the same way the default does");
+    assert!(
+        err.to_string().contains("--allow-remote-include"),
+        "the error should name the flag that opts in, for a CLI to surface: {err}"
+    );
+}
+
+// ----, part 2: once opted in, resolved addresses are still checked
+// against loopback/link-local/private ranges before any request is made ----
+
+#[test]
+fn opted_in_remote_include_still_refuses_the_cloud_metadata_endpoint() {
+    // A literal IP in the URL resolves without any DNS/network I/O (`(&str,
+    // u16): ToSocketAddrs` parses a numeric host directly), so this is a
+    // deterministic, offline test of the SSRF check itself.
+    let scenario = scenario_with_remote_include(
+        "http://169.254.169.254/latest/meta-data/iam/security-credentials/",
+    );
+    let err =
+        resolve_includes_with_policy(scenario, &IncludeSource::Inline, RemoteIncludePolicy::Allow)
+            .expect_err(
+                "the cloud metadata address must be refused even when remote includes are allowed",
+            );
+    let msg = err.to_string();
+    assert!(
+        msg.contains("169.254.169.254"),
+        "the error should name the disallowed address: {msg}"
+    );
+}
+
+#[test]
+fn opted_in_remote_include_still_refuses_loopback() {
+    let scenario = scenario_with_remote_include("http://127.0.0.1:8500/scenario.json");
+    let err =
+        resolve_includes_with_policy(scenario, &IncludeSource::Inline, RemoteIncludePolicy::Allow)
+            .expect_err("loopback must be refused even when remote includes are allowed");
+    assert!(err.to_string().contains("127.0.0.1"), "{err}");
+}
+
+#[test]
+fn opted_in_remote_include_still_refuses_rfc1918_private_ranges() {
+    let scenario = scenario_with_remote_include("http://10.0.0.5/scenario.json");
+    let err =
+        resolve_includes_with_policy(scenario, &IncludeSource::Inline, RemoteIncludePolicy::Allow)
+            .expect_err(
+                "an RFC1918 private address must be refused even when remote includes are allowed",
+            );
+    assert!(err.to_string().contains("10.0.0.5"), "{err}");
+}