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

[epmog]

What was the problem/requirement? (What/Why)

An embedded file's filename must be a plain, single path component per the
2023-09 spec — directory pathing is disallowed. Several gaps let unsafe values
through:

• The model-layer template validator only rejected / and \. It accepted
  ., .., and filenames containing a null byte.
• The sessions layer, which joins the (already-validated) filename to the
  session files directory, had no check for strings that contain no separator
  yet are still not plain basenames — most notably Windows drive-relative
  anchors like D:relative and bare drives like C:. Path::join treats
  these as a drive prefix and would resolve to a location outside the target
  directory.
• Windows strips trailing dots and spaces from the final path component, so a
  filename made up entirely of dots and spaces (".. ", "...", ". ")
  collapses to . or .. at the OS layer and resolves to the current/parent
  directory. Rust's Path only special-cases the exact strings ./.., so
  these slipped past the checks above as ordinary components. (On POSIX these
  are legitimate, distinct filenames that stay inside the directory.)

What was the solution? (How)

• Model layer (openjd-model): extend embedded-file filename validation
  to also reject a null character and the values . and .., with
  Pydantic-compatible error paths/messages.
• Sessions layer (openjd-sessions): require the resolved filename to be
  exactly one "normal" path component for the host platform (rejecting Windows
  drive-relative anchors and bare drives, which Path parses as a prefix/root
  on Windows and as ordinary filenames on POSIX). On Windows, additionally
  reject filenames that consist entirely of dots and spaces, which the OS would
  collapse to ./... As a final defense-in-depth guarantee, the joined path
  is lexically normalized (collapsing ./.. without touching the filesystem)
  and must still resolve to a location inside the session files directory
  before the file is created; otherwise a SessionError::EmbeddedFilePath is
  raised. This containment check now requires both paths to be rooted (which
  target_directory always is), because lexical .. handling is only sound
  for rooted paths.

The Windows-specific rules (drive-relative anchors, all-dots-and-spaces names)
are intentionally enforced in the sessions layer rather than the model layer:
template validation is platform-independent (a template authored on Linux may
target Windows), while the sessions layer knows the execution host and can
parse path components correctly for it. The corresponding strings are valid,
contained filenames on POSIX, so rejecting them everywhere would forbid
legitimate names for no security benefit.

What is the impact of this change?

Templates with an embedded-file filename of ., .., or containing a null
byte are now rejected at validation time with a clear error. At session time,
filenames that are not a single safe path component for the host — Windows
drive-relative paths, and Windows names made only of dots and spaces — are
rejected before any file is written, closing paths that could escape the
session files directory. Legitimate basename filenames are unaffected on every
platform.

How was this change tested?

• Have you run the unit tests? Yes.
  • cargo test -p openjd-sessions — 196 unit + 287 integration + 6 doc tests
    passed.
  • cargo test -p openjd-model (embedded-filename tests) — pass.
  • cargo clippy --all-targets --workspace -- -D warnings — clean.
  • cargo fmt --all -- --check — clean.
• Added model integration tests asserting the full field path + message for
  ., .., and null-character filenames.
• Added sessions unit tests for the single-path-component check, the
  lexical-normalization/containment helpers (including rejection of relative,
  non-rooted paths), and the Windows all-dots-and-spaces rule; plus Windows-only
  integration tests for drive-relative anchors (D:relative, C:thing.exe,
  C:) and all-dots-and-spaces names (".. ", "...").
• The Windows-only rules are #[cfg(windows)], so those tests compile and run
  on the Windows CI job.
• Verified the new model tests genuinely gate the new behavior by running them
  against the pre-change source in a temporary worktree: all three failed
  (templates parsed successfully where an error was expected), while the
  pre-existing filename tests continued to pass.

Was this change documented?

• Are relevant docstrings in the code base updated? Yes. The
  validate_resolved_filename, normalize_lexical, and ensure_within
  docstrings describe the rules and rationale (including the Windows
  all-dots-and-spaces rule and the rooted-path precondition). specs/model/validation.md
  now lists the full embedded-file filename rules (non-empty, no separators,
  no null, not ./..), and specs/sessions/embedded-files.md documents the
  single-path-component rule, the Windows all-dots-and-spaces rule, and the
  final containment check.

Is this a breaking change?

No. This tightens validation to reject inputs that were never valid per the
spec. No public API signatures change, and previously-valid basename filenames
continue to work. (Templates that relied on the previously-accepted-but-invalid
./../null filenames — or, on Windows, drive-relative and all-dots-and-spaces
filenames — will now be rejected, which is the intended fix.)

Does this change impact security?

Yes — this is a path-traversal hardening change. It adds defense-in-depth so an
embedded file cannot be written outside the session files directory, including
a final containment check on the resolved path that is now sound only for
rooted paths, and Windows-specific rejections for names the OS would otherwise
collapse to a parent/current-directory reference. It does not change file
ownership or permissions semantics.

---

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[github-actions]

The Component::Normal rule is now unreachable on both platforms, and the spec/doc examples attached to it are no longer accurate.

By the time control reaches this check the input has already been filtered for / and \, \0, exactly ./.., and — on Windows — any : plus all-dots-and-spaces. On Windows a non-Normal leading component requires either a separator (RootDir, UNC/verbatim prefixes) or a colon (drive prefix), both already rejected above. On POSIX only / or ./.. can yield a non-Normal component. So components() always yields exactly one Component::Normal here and this branch cannot fire.

That is fine as a fail-closed backstop, but the documentation presents it as the rule that catches drive-relative paths, and that is no longer true:

• specs/sessions/embedded-files.md:104 — the table row not a single "normal" path component (e.g. D:relative, C:)
• the doc comment at embedded_files.rs:202-208 — "most notably Windows drive-relative paths like D:relative"

The new tests contradict both: rejects_windows_drive_relative and rejects_windows_bare_drive assert the reason is the colon message, not must be a single path component. A maintainer reading the spec would conclude drive-relative handling rests on Path::components parsing for the host OS, when it now rests entirely on the colon rule — which matters, because the colon rule is #[cfg(windows)]-gated while this component check is not.

Suggest attributing D:relative/C: to the colon rule in both the spec row and the doc comment, and describing the component check as a backstop with no currently-reaching input.

[github-actions]

Scoping question on the "final defense-in-depth guarantee" wording: because the check is purely lexical, it does not hold in the cross-user configuration, and the doc/spec text reads as if it does.

TempDir::new creates the embedded-files directory 0o770 when user is a non-process user (tempdir.rs:175-190), i.e. group-writable by the session user. So the session user can pre-create a symlink in that directory at the name a template declares (filename: "script.sh"), and write_file_contents reaches the file via fs::write (embedded_files.rs:522, write_embedded_file_with_options → fs::write), which follows symlinks. The resulting write lands wherever the link points, as the process user — outside the session files directory — while ensure_within reports the path as contained, because it explicitly does no symlink resolution.

The same applies to the fs::write(&path, b"") + set_permissions in the unnamed branch, though the random 32-hex name makes that far harder to pre-empt.

This is pre-existing behavior, not introduced here — the concern is that this change adds a claim that is stronger than what the code provides. specs/sessions/embedded-files.md now says the resulting path "must still lie within the session files directory ... so a bug there cannot lead to a write outside the session directory", and this doc comment calls it "the final defense-in-depth guarantee". Both are true only for lexical/.. escapes, not for symlinks.

Either narrow the wording (state that symlink-based escapes are out of scope and why, next to the existing out-of-scope note for reserved device names), or make the write itself refuse to follow a link — e.g. OpenOptions::new().write(true).create_new(true) for the initial creation, plus custom_flags(libc::O_NOFOLLOW) on unix.

[github-actions]

This creates a directory inside the source tree. The test resolves against the process CWD, which for cargo test/cargo nextest is the crate manifest directory — so this writes crates/openjd-sessions/openjd_rel_root_test_<pid>_<nanos>/ into the checkout. The Cleanup guard handles panics, but not a killed/aborted test process (SIGKILL, CI timeout, --no-capture interrupt), leaving an untracked directory behind.

Two smaller points on the same test:

• The whole session working directory tree lives under this relative root, so anything that leaks is a full session dir, not an empty one.
• The assertion (working_directory().is_absolute()) does not exercise the behavior the doc comment describes as the regression — that a named embedded file now succeeds under a relative root. ensure_within is applied to files_directory, and the test never allocates an embedded file, so a future change that absolutized working_directory but not files_directory would still pass.

Both are addressable without the CWD dependency: construct a PathBuf from a tempfile::TempDir path with a ./.. segment spliced in (e.g. <tmp>/x/../x) so std::path::absolute has something to normalize, or drop the Session layer and assert on EmbeddedFiles::new + allocate_file_paths with a relative target directory directly — which also covers the named-file case.

[leongdl]

Please reply to the approved comments.

[leongdl]

Reviewed at 92b49b6, checked out locally against merge-base 361b2c5. cargo clippy --all-targets -- -D warnings clean; cargo test -p openjd-sessions -p openjd-model → 2509 passed, 0 failed, 16 ignored.

TLDR

The change is correct and well-tested, and it closes real gaps. Two things worth raising, one misleading-but-cosmetic and one substantive:

1. The Component::Normal single-component guard cannot fire on either platform as ordered. The separator and colon checks in front of it already reject everything that would produce a non-Normal component. Probe on macOS, over strings that survive the preceding checks:

input	components()	single Normal?
C:	Normal("C:")	yes
D:relative	Normal("D:relative")	yes
script.sh:evil	Normal("script.sh:evil")	yes
..., .. , , ~	Normal(..)	yes

On POSIX, RootDir needs /, CurDir is only ., ParentDir only .., and Prefix never occurs — all rejected above, so the guard is dead. On Windows (reasoned from std::path prefix parsing, not measured — no Windows host on my side) every Prefix variant requires either : or \, and RootDir requires \ or /, both rejected above, so it is dead there too. grep -rn "single path component" crates finds the string only in embedded_files.rs:272 and an error.rs doc comment — no test pins it, and the Windows integration tests for D:relative/C: assert must not contain ':', confirming the colon check fires first.

The doc comment, specs/sessions/embedded-files.md, and the PR description all attribute the drive-relative rejection to this guard. Swapping the two checks makes both live and separately testable: D:relative/C: would fail on components, script.sh:evil would fall through to the colon rule. Otherwise the three prose claims need correcting and the guard relabelling as an unreachable backstop.

2. The containment guarantee claimed is stronger than what holds, because none of this is symlink-safe. Probe against the crate's own public write path, with a symlink planted at <files_dir>/script.sh → <outside>/victim.txt:

victim content after write: "PWNED"
victim is outside files dir: true
victim mode after write:     600     (was 644)

fs::write and fs::set_permissions both followed it, and chown_for_user's nix::unistd::chown follows too. The target name is predictable because it is the declared filename, and in cross-user mode the files directory is 0o770 group-owned by the session user (tempdir.rs:175-191), so a prior environment action running as the job's user can plant it. Lexical normalization cannot see this by construction.

I proved the mechanism; I did not stand up two users, so the 0o770 group-write premise is read from tempdir.rs rather than exercised. If the goal is to close it rather than soften the claim in the description and spec, the unnamed branch already has the right shape — pre-create named files with O_CREAT|O_EXCL|O_NOFOLLOW in allocate_file_paths and do the write, chmod and chown through the fd (fchmod/fchown), which detects a planted symlink instead of following it.

How it works — the two call stacks

Template validation, openjd-model:

JobTemplate / EnvironmentTemplate parse
  → validate_v2023_09::validate            (structure pass)
      → validate_embedded_files()          structure.rs:939
            filename: empty → separators → NUL → "." / ".."   :978-999

Reached from two call sites, structure.rs:320 (job/step scripts) and :429 (environment scripts). The three new tests only exercise the environment path.

Session materialization, openjd-sessions:

Session::with_config                                        session.rs:589
  std::path::absolute(session_root_directory)               :601   ← new
  TempDir::new(root, session_id, user)      → working_directory
  TempDir::new(working_dir, "embedded_files", user) → files_directory
        0o700 same-user / 0o770 group=session-user cross-user   tempdir.rs:175
  …
  run_task / enter_environment
    → runner::step_script:130 | runner::env_script:230 | session.rs:2676,2792
        EmbeddedFiles::new(scope, files_directory, session_id)
        EmbeddedFiles::allocate_file_paths()                 embedded_files.rs:376
          Some(filename):
            validate_resolved_filename()                    :219   ← extended
              empty → NUL → '/','\' → "."/".." → [win] ':' → [win] all-dots-and-spaces
              → single Normal component
            joined = target_directory.join(fname)            :410
            ensure_within(target_directory, joined)          :303   ← new
                normalize_lexical(base), normalize_lexical(candidate)   :277
                candidate.starts_with(base)
            symtab.set("Task.File.<name>" | "Env.File.<name>", Path(joined))
          None:
            random_hex_filename() → fs::write(empty) → 0o600
        EmbeddedFiles::write_file_contents()                 :491
          data.resolve_string_with(symtab)     ← only `data` is interpolated
          write_embedded_file_with_options()                  :70
              fs::create_dir_all(parent) / fs::write / fs::set_permissions
          chown_for_user()                                    :102
              nix::unistd::chown + fs::set_permissions

The load-bearing design fact is at the top of validate_resolved_filename: filename is a plain string in the 2023-09 schema, not an @fmtstring. Only data goes through expression resolution, and that happens in phase 2 after the path is already fixed. So one validation point at allocate time is sufficient — there is no later step that can reintroduce a separator. Worth keeping that comment prominent, since it is the reason this isn't a TOCTOU.

[github-actions]

This doc comment introduces "session_files_directory must be an absolute path" as a precondition, but new does not enforce it and there are public entry points that do not satisfy it — so the violation surfaces later as a misleading error rather than at construction.

Session::with_config is not the only way to reach EmbeddedFiles::new with a caller-supplied directory:

• runner::step_script::StepScriptRunner::new(session_id, working_directory, files_directory, user) (step_script.rs:31) is pub in a pub mod, and passes files_directory straight through to EmbeddedFiles::new at step_script.rs:132.
• runner::env_script::EnvironmentScriptRunner::new (env_script.rs:41) does the same at env_script.rs:232 and env_script.rs:257.
• Session::new_for_test (session.rs:520-524) derives files_directory = working_directory.join("embedded_files") from a caller-supplied working_directory with no absolutization; the test-utils feature that gates it is enabled for this crate's own integration tests.

None of those paths go through the std::path::absolute call added in with_config. An external consumer that constructs a runner directly with a relative files_directory now gets, for every named embedded file:

Embedded file 'X' has unsafe filename 'script.sh': path containment can only be checked for rooted paths

which blames the filename when the actual problem is the target directory — the same spurious-error mode the with_config change was written to fix, just reached by a different door. Before this PR those call paths worked with a relative directory.

Two ways to make the stated precondition hold rather than be assumed:

• Absolutize in EmbeddedFiles::new itself (std::path::absolute on session_files_directory), which covers every entry point at once and lets with_config's change stay purely about logging/derived-path clarity. new currently returns Self, so this needs either an infallible fallback or a signature change.
• Or keep new infallible and make the failure legible: give ensure_within's rooted-path rejection its own SessionError variant that names the directory, instead of reusing EmbeddedFilePath { filename, .. }.