fix(sessions,model): reject embedded filenames that aren't a safe single path component

crates/openjd-model/src/template/validate_v2023_09/structure.rs

@@ -993,6 +993,18 @@ fn validate_embedded_files(
                     "must not contain path separators.",
                 );
             }
+            if fname.contains('\0') {
+                errors.add(
+                    &path_field(&f_path, "filename"),
+                    "must not contain null characters.",
+                );
+            }
+            if fname == "." || fname == ".." {
+                errors.add(
+                    &path_field(&f_path, "filename"),
+                    format!("must not be '{fname}'."),
+                );
+            }
         }
     }
 }

crates/openjd-model/tests/integration/test_environment_template.rs

@@ -448,6 +448,47 @@ fn test_embedded_filename_backslash() {
     ]);
 }
 
+#[test]
+fn test_embedded_filename_dot() {
+    check_env_err(
+        r#"{
+        "specificationVersion": "environment-2023-09",
+        "environment": {"name": "Foo", "script": {
+            "embeddedFiles": [{"name": "MyFile", "type": "TEXT", "data": "hello", "filename": "."}],
+            "actions": {"onEnter": {"command": "foo"}}
+        }}
+    }"#,
+        &["environment -> script -> embeddedFiles[0] -> filename:\n\tmust not be '.'."],
+    );
+}
+
+#[test]
+fn test_embedded_filename_dotdot() {
+    check_env_err(
+        r#"{
+        "specificationVersion": "environment-2023-09",
+        "environment": {"name": "Foo", "script": {
+            "embeddedFiles": [{"name": "MyFile", "type": "TEXT", "data": "hello", "filename": ".."}],
+            "actions": {"onEnter": {"command": "foo"}}
+        }}
+    }"#,
+        &["environment -> script -> embeddedFiles[0] -> filename:\n\tmust not be '..'."],
+    );
+}
+
+#[test]
+fn test_embedded_filename_null_character() {
+    check_env_err(r#"{
+        "specificationVersion": "environment-2023-09",
+        "environment": {"name": "Foo", "script": {
+            "embeddedFiles": [{"name": "MyFile", "type": "TEXT", "data": "hello", "filename": "a\u0000b"}],
+            "actions": {"onEnter": {"command": "foo"}}
+        }}
+    }"#, &[
+        "environment -> script -> embeddedFiles[0] -> filename:\n\tmust not contain null characters.",
+    ]);
+}
+
 #[test]
 fn test_embedded_duplicate_names() {
     check_env_err(

crates/openjd-sessions/src/embedded_files.rs

@@ -7,7 +7,7 @@
 //! Mirrors Python `_embedded_files.py`.
 
 use std::fs;
-use std::path::{Path, PathBuf};
+use std::path::{Component, Path, PathBuf};
 use std::sync::Arc;
 
 use openjd_expr::function_library::FunctionLibrary;
@@ -194,6 +194,27 @@ fn random_hex_filename() -> String {
 ///   path components by spec.
 /// - Contain a null byte.
 /// - Equal `.` or `..`.
+/// - **On Windows only:** contain a colon (`:`). A colon has no legitimate use
+///   in a basename and introduces two separator-free escapes: NTFS alternate
+///   data streams (`script.sh:evil` writes a stream of the `script.sh` object)
+///   and drive-relative anchors (`C:evil`, `C:`). On POSIX `:` is a legitimate
+///   filename character and is accepted.
+/// - Are not a single "normal" path component for the host platform. This
+///   catches platform-specific prefixes and roots that contain no separator
+///   and therefore slip past the checks above — most notably Windows
+///   drive-relative paths like `D:relative`, which `Path::join` treats as a
+///   drive prefix and would use to escape the target directory. `Path`
+///   parses components for the OS the session runs on, so the rule is always
+///   correct for the execution host.
+/// - **On Windows only:** consist entirely of dots and spaces (`".. "`,
+///   `"..."`, `". "`, ...). Windows strips trailing dots and spaces from the
+///   final path component, so such a name collapses to `.` or `..` at the OS
+///   layer and resolves to the current/parent directory — escaping the target.
+///   Rust only special-cases the exact strings `.`/`..`, so these otherwise
+///   parse as ordinary `Normal` components and slip past the checks above and
+///   the lexical containment check. Any filename with a separator is already
+///   rejected, so only a standalone all-dots-and-spaces name can reach this
+///   rule. On POSIX these are legitimate, distinct filenames and are accepted.
 ///
 /// Returns `Ok(())` if the filename is a safe single path component.
 fn validate_resolved_filename(resolved: &str) -> Result<(), String> {
@@ -209,9 +230,103 @@ fn validate_resolved_filename(resolved: &str) -> Result<(), String> {
     if resolved == "." || resolved == ".." {
         return Err(format!("must not be '{resolved}'"));
     }
+    // On Windows a colon is never valid in a basename and introduces two
+    // distinct escapes that contain no path separator (so they slip past the
+    // check above):
+    // - NTFS alternate data streams: `script.sh:evil` writes the `evil` stream
+    //   *of the `script.sh` file object* rather than a file named
+    //   `script.sh:evil`, so one embedded file's declared filename can attach
+    //   data to another embedded file.
+    // - Drive-relative anchors: `C:evil` / `C:` are parsed as a drive prefix by
+    //   `Path::join` and resolve relative to the current directory on that
+    //   drive, outside the target directory.
+    // Reject the colon outright. On POSIX `:` is an ordinary, legitimate
+    // filename character, so this rule is Windows-only.
+    #[cfg(windows)]
+    if resolved.contains(':') {
+        return Err("must not contain ':'".into());
+    }
+    // On Windows the filesystem strips trailing dots and spaces from the final
+    // path component, so a filename made up entirely of dots and spaces —
+    // `".. "`, `"..."`, `". "`, etc. — collapses to `.` or `..` at the OS layer
+    // and resolves to the current/parent directory, escaping the target. Rust
+    // only special-cases the exact strings `.`/`..`, so these reach here as
+    // ordinary `Normal` components and also pass the lexical containment check.
+    // Any filename containing a separator is already rejected above, so only a
+    // standalone all-dots-and-spaces name can get this far. On POSIX these are
+    // legitimate, distinct filenames, so the rule is Windows-only.
+    #[cfg(windows)]
+    if resolved.trim_end_matches([' ', '.']).is_empty() {
+        return Err("must not consist only of dots and spaces".into());
+    }
+    // Require exactly one normal path component. On Windows this rejects
+    // drive-relative anchors such as `D:relative` (parsed as `Prefix` +
+    // `Normal`) and bare drives like `C:` (`Prefix` only), neither of which
+    // contains a separator. On POSIX such strings are ordinary filenames and
+    // are accepted.
+    let mut components = Path::new(resolved).components();
+    if !matches!(
+        (components.next(), components.next()),
+        (Some(Component::Normal(_)), None)
+    ) {
+        return Err("must be a single path component".into());
+    }
     Ok(())
 }
 
+/// Lexically normalize a path by collapsing `.` and `..` components without
+/// touching the filesystem.
+///
+/// Unlike [`std::fs::canonicalize`], this performs no I/O and does not resolve
+/// symlinks — appropriate here because the target file does not exist yet and
+/// we only want to reason about the path the caller constructed.
+fn normalize_lexical(path: &Path) -> PathBuf {
+    let mut out = PathBuf::new();
+    for component in path.components() {
+        match component {
+            Component::CurDir => {}
+            Component::ParentDir => {
+                out.pop();
+            }
+            other => out.push(other.as_os_str()),
+        }
+    }
+    out
+}
+
+/// Verify that `candidate` lexically resolves to a location inside `base`.
+///
+/// This is the final defense-in-depth guarantee before an embedded file is
+/// created: even if filename validation were bypassed or a future change let
+/// something through, the path we are about to write must still sit within the
+/// session files directory. Comparison is lexical (see [`normalize_lexical`]),
+/// so it holds regardless of whether the paths exist on disk yet.
+///
+/// Both `base` and `candidate` must be rooted; a relative path is rejected
+/// with an error because lexical `..` handling is only sound for rooted paths.
+fn ensure_within(base: &Path, candidate: &Path) -> Result<(), String> {
+    // Lexical containment is only sound when both paths are rooted. For a
+    // relative path a `..` that pops past the start is silently dropped
+    // (`PathBuf::pop` on an empty buffer is a no-op), so an escaping path like
+    // `files/../../x` would normalize to `files/x` and spuriously compare as
+    // contained. Rooted paths clamp at the root instead. `target_directory` is
+    // always rooted because `Session::with_config` absolutizes the configured
+    // session root (and `EmbeddedFiles::new` is documented to require an
+    // absolute directory), so this branch is unreachable in practice; it is
+    // kept as a fail-closed guard rather than a silently-relied-upon
+    // precondition.
+    if !base.has_root() || !candidate.has_root() {
+        return Err("path containment can only be checked for rooted paths".into());
+    }
+    let base = normalize_lexical(base);
+    let candidate = normalize_lexical(candidate);
+    if candidate.starts_with(&base) {
+        Ok(())
+    } else {
+        Err("resolves to a path outside the session files directory".into())
+    }
+}
+
 struct FileRecord {
     symbol: String,
     filename: PathBuf,
@@ -231,6 +346,14 @@ pub struct EmbeddedFiles {
 }
 
 impl EmbeddedFiles {
+    /// Create a new embedded-file materializer targeting
+    /// `session_files_directory`.
+    ///
+    /// `session_files_directory` must be an absolute path. The final
+    /// containment check is only sound for rooted paths and will reject a
+    /// relative directory outright. Callers going through
+    /// [`Session::with_config`](crate::Session::with_config) get this for free
+    /// — it absolutizes the configured session root.
     pub fn new(
         scope: EmbeddedFilesScope,
         session_files_directory: PathBuf,
@@ -284,7 +407,18 @@ impl EmbeddedFiles {
                         reason,
                     }
                 })?;
-                self.target_directory.join(fname)
+                let joined = self.target_directory.join(fname);
+                // Final guarantee: the path we are about to create must resolve
+                // to a location inside the session files directory. This backs
+                // up the per-component filename validation above.
+                ensure_within(&self.target_directory, &joined).map_err(|reason| {
+                    SessionError::EmbeddedFilePath {
+                        name: file.name.clone(),
+                        filename: fname.clone(),
+                        reason,
+                    }
+                })?;
+                joined
             } else {
                 let name = random_hex_filename();
                 let path = self.target_directory.join(&name);
@@ -433,6 +567,103 @@ mod tests {
         assert_ne!(random_hex_filename(), random_hex_filename());
     }
 
+    #[test]
+    fn normalize_lexical_collapses_parent_and_current() {
+        assert_eq!(
+            normalize_lexical(Path::new("/a/b/../c/./d")),
+            PathBuf::from("/a/c/d")
+        );
+    }
+
+    #[test]
+    fn ensure_within_accepts_direct_child() {
+        let base = Path::new("/session/files");
+        assert!(ensure_within(base, &base.join("script.sh")).is_ok());
+    }
+
+    #[test]
+    fn ensure_within_rejects_parent_escape() {
+        let base = Path::new("/session/files");
+        assert!(ensure_within(base, Path::new("/session/files/../../etc/passwd")).is_err());
+    }
+
+    #[test]
+    fn ensure_within_rejects_sibling_with_shared_prefix() {
+        // `files-evil` must not be treated as being under `files`.
+        let base = Path::new("/session/files");
+        assert!(ensure_within(base, Path::new("/session/files-evil/x")).is_err());
+    }
+
+    #[test]
+    fn ensure_within_rejects_relative_paths() {
+        // Lexical `..` handling is only sound for rooted paths: a `..` that pops
+        // past the start of a relative path is silently dropped, so an escaping
+        // path would spuriously compare as contained. Relative inputs must be
+        // rejected outright.
+        assert!(ensure_within(Path::new("files"), Path::new("files/../../files/x")).is_err());
+    }
+
+    #[test]
+    fn validate_accepts_single_component() {
+        assert!(validate_resolved_filename("script.sh").is_ok());
+    }
+
+    #[test]
+    fn validate_rejects_double_dot() {
+        assert_eq!(
+            validate_resolved_filename(".."),
+            Err("must not be '..'".to_string())
+        );
+    }
+
+    // Windows strips trailing dots and spaces from the final path component, so
+    // a name made only of dots and spaces collapses to `.`/`..` at the OS layer
+    // and escapes the target directory. These are legitimate filenames on
+    // POSIX, so the rejection — and this test — is Windows-only.
+    #[cfg(windows)]
+    #[test]
+    fn validate_rejects_all_dots_and_spaces() {
+        for value in [".. ", "...", ". ", "   ", ".. ."] {
+            assert_eq!(
+                validate_resolved_filename(value),
+                Err("must not consist only of dots and spaces".to_string()),
+                "expected rejection for {value:?}"
+            );
+        }
+    }
+
+    #[cfg(windows)]
+    #[test]
+    fn validate_accepts_name_with_trailing_dot() {
+        // A named file that merely has a trailing dot does not collapse to a
+        // directory reference — Windows resolves `foo...` to `foo`, still inside
+        // the target — so it is accepted.
+        assert!(validate_resolved_filename("foo...").is_ok());
+    }
+
+    // On Windows a colon opens an NTFS alternate data stream or a drive-relative
+    // anchor, neither of which contains a path separator, so both slip past the
+    // separator check and must be rejected explicitly. On POSIX `:` is a
+    // legitimate filename character, so this rejection is Windows-only.
+    #[cfg(windows)]
+    #[test]
+    fn validate_rejects_colon() {
+        for value in ["script.sh:evil", "C:evil", "C:", "a:b:c"] {
+            assert_eq!(
+                validate_resolved_filename(value),
+                Err("must not contain ':'".to_string()),
+                "expected rejection for {value:?}"
+            );
+        }
+    }
+
+    #[cfg(unix)]
+    #[test]
+    fn validate_accepts_colon_on_posix() {
+        // A colon is an ordinary filename character on POSIX.
+        assert!(validate_resolved_filename("a:b").is_ok());
+    }
+
     #[cfg(unix)]
     #[test]
     fn test_chown_for_user_nonexistent_group_returns_error() {

crates/openjd-sessions/src/session.rs

@@ -100,6 +100,13 @@ pub struct SessionConfig {
     pub retain_working_dir: bool,
     pub callback: Option<SessionCallbackType>,
     pub os_env_vars: Option<HashMap<String, String>>,
+    /// Root directory under which the session's working and embedded-files
+    /// directories are created. If `None`, a temporary directory is used.
+    ///
+    /// A relative path is resolved to an absolute path (against the process
+    /// current working directory) when the session is created, so all derived
+    /// paths are always rooted — a requirement of the embedded-file
+    /// containment check.
     pub session_root_directory: Option<PathBuf>,
     pub user: Option<Arc<dyn SessionUser>>,
     /// Revision + extensions profile that drives expression-function
@@ -582,7 +589,19 @@ impl Session {
     /// Full constructor from SessionConfig.
     pub fn with_config(mut config: SessionConfig) -> Result<Self, SessionError> {
         let root_dir = match &config.session_root_directory {
-            Some(d) => d.clone(),
+            // Absolutize a caller-supplied root so every path derived from it
+            // (working directory, embedded-files directory) is rooted. The
+            // embedded-file containment check (`ensure_within`) is only sound
+            // for rooted paths and rejects relative ones outright; without this
+            // a relative `session_root_directory` would surface as a spurious
+            // "unsafe filename" error for every named embedded file. This is a
+            // purely lexical operation (`std::path::absolute` joins onto the
+            // current directory and does not touch the filesystem or resolve
+            // symlinks). `openjd_temp_dir` already returns an absolute path.
+            Some(d) => std::path::absolute(d).map_err(|source| SessionError::WorkingDirectory {
+                path: d.clone(),
+                source,
+            })?,
             None => crate::tempdir::openjd_temp_dir(None)?,
         };