fix(producer): external assets work on Windows (GH heygen-com#321) (heygen-com#324)

* fix(producer): external assets work on Windows (GH heygen-com#321)

Two Unix-only assumptions in the external-asset pipeline caused every
absolute path on Windows to be rejected as "unsafe" at render time:

1. Containment checks used `child.startsWith(parent + "/")`. On Windows
   the separator is `\`, so the predicate is always false unless the
   paths are equal — every external asset tripped the safety guard in
   `renderOrchestrator.ts`. The reporter saw:

     [Render] Skipping external asset with unsafe path:
       hf-ext/D:\coder\reactGin\hyperframes\reading\assets\segment_001.wav

   Fix: use `path.relative()` through a shared helper
   `isPathInside(child, parent)` that normalises separators per-platform
   and correctly rejects siblings whose names start with the parent
   (e.g. `/foo/bar-sibling` is NOT inside `/foo/bar`).

2. The external-asset key was built as `"hf-ext/" + absPath.replace(/^\//, "")`.
   A Windows absolute path (`D:\coder\...`) became
   `"hf-ext/D:\\coder\\..."` — and because Node's `path.join` treats a
   drive-letter prefix as absolute, `join(compileDir, key)` silently
   escaped `compileDir`. Fix: `toExternalAssetKey()` strips the drive
   colon and normalises to forward slashes, producing
   `hf-ext/D/coder/...` — a pure relative path that `path.join` cannot
   promote to absolute on any OS.

Both helpers live in `packages/producer/src/utils/paths.ts` and are
exercised by 14 unit tests covering Unix paths, Windows drive-letter
paths, mixed separators, sibling-prefix confusion, and `..` traversal.

Docs: new "External assets" section in `docs/packages/producer.mdx`
describes detection, sanitised keys, and the cross-platform containment
invariant.

Closes heygen-com#321.

* fix(producer): address review on heygen-com#324 — UNC + integration test

Addresses the non-blocking observations from the PR heygen-com#324 staff review
(heygen-com#324 (comment)):

1. UNC and extended-length Windows paths.
   `toExternalAssetKey` now handles:
   - `\\?\D:\very\long\path\clip.mp4` (extended-length)      → `hf-ext/D/very/long/path/clip.mp4`
   - `\\server\share\file.wav` (plain UNC)                    → `hf-ext/unc/server/share/file.wav`
   - `\\?\UNC\server\share\file.wav` (extended-length UNC)   → `hf-ext/unc/server/share/file.wav`
   The UNC-collapsed form keeps the server boundary so two different
   servers exposing the same share/file name cannot collide under one
   relative key. Previously both edge cases silently produced keys with
   stray `?` or `:` characters that downstream `isPathInside` rejected —
   not a security hole, but a silent drop of user assets.

2. Short-circuit on already-sanitised input.
   `toExternalAssetKey("hf-ext/…")` now returns its input unchanged
   instead of prepending `hf-ext/` a second time. Makes the helper
   genuinely idempotent, which is what the unit test claimed all along.
   Renamed the test accordingly.

3. JSDoc caller contract.
   `toExternalAssetKey` now documents that it expects canonicalised
   input (`path.resolve`'d upstream) and does not strip `..`
   components. `isPathInside` at copy time is still the defensive
   backstop — called out explicitly in the doc so future callers read
   the contract before the code.

4. End-to-end integration test.
   `renderOrchestrator.test.ts` gains two seam tests that run the full
   external-asset pipeline — build the sanitised key, populate an
   `externalAssets` map, invoke `writeCompiledArtifacts`, and assert
   both the success path (the file lands under `<compileDir>/hf-ext/…`)
   and the escape-rejection path (a malicious `hf-ext/../../etc/passwd`
   key does NOT materialise above `compileDir`). `writeCompiledArtifacts`
   is exported for the test seam with a clear JSDoc disclaimer that
   it's not part of the public API.

22 tests pass across `paths.test.ts` (17) and `renderOrchestrator.test.ts` (5).

Out of scope for this follow-up (tracked as follow-ups):
- Centralising every `startsWith("/")` absolute-path check into a
  shared helper across htmlCompiler / audioExtractor / audioMixer /
  videoFrameExtractor. Mentioned in the review; touches 5 files and
  deserves its own PR.
- Windows CI runner.

Author: miguel-heygen

docs/packages/producer.mdx

@@ -252,6 +252,30 @@ bun run benchmark
 
 The benchmark runs several compositions with different quality and FPS settings and reports timing for each combination.
 
+## External assets (files outside `projectDir`)
+
+A composition can reference absolute paths to assets outside the project
+directory — a local voiceover in `~/Downloads`, a shared-drive image, a
+generated fixture at an absolute path. The producer handles these by:
+
+1. **Detection.** During compilation, the HTML compiler walks every
+   `[src]` / `[href]` and every `url(...)` in `<style>`. A path that
+   resolves to a file outside `projectDir` is collected into an
+   `externalAssets` map.
+2. **Sanitised keys.** Each absolute path is converted into a safe,
+   cross-platform relative key prefixed with `hf-ext/`. Windows
+   drive-letter colons are stripped (`D:\foo\x.wav` → `hf-ext/D/foo/x.wav`)
+   so that `path.join(compileDir, key)` stays inside the compile
+   directory on every OS.
+3. **Copy + rewrite.** The orchestrator copies the file under
+   `<compileDir>/hf-ext/...` and the HTML is rewritten to point at the
+   sanitised key. The file server then serves both project-internal and
+   external assets from the same root.
+
+The containment check uses `path.relative()` rather than a hardcoded
+separator, so external assets work identically on macOS, Linux, and
+Windows. See `packages/producer/src/utils/paths.ts` for the helpers.
+
 ## Related Packages
 
 <CardGroup cols={2}>

packages/producer/src/services/htmlCompiler.ts

@@ -24,6 +24,7 @@ import {
   rewriteCssAssetUrls,
 } from "@hyperframes/core";
 import { extractVideoMetadata, extractAudioMetadata } from "../utils/ffprobe.js";
+import { isPathInside, toExternalAssetKey } from "../utils/paths.js";
 import {
   parseVideoElements,
   type VideoElement,
@@ -804,12 +805,14 @@ export function collectExternalAssets(
       return null;
     }
     const absPath = resolve(absProjectDir, trimmed);
-    if (absPath.startsWith(absProjectDir + "/") || absPath === absProjectDir) {
+    if (isPathInside(absPath, absProjectDir)) {
       return null; // inside projectDir, file server handles this
     }
     if (!existsSync(absPath)) return null;
-    // resolve() already canonicalizes the path (no .. components remain)
-    const safeKey = "hf-ext/" + absPath.replace(/^\//, "");
+    // resolve() already canonicalises the path (no .. components remain);
+    // toExternalAssetKey() produces a cross-platform relative key that
+    // `path.join(compileDir, key)` cannot escape on any OS.
+    const safeKey = toExternalAssetKey(absPath);
     externalAssets.set(safeKey, absPath);
     return safeKey;
   }

packages/producer/src/services/renderOrchestrator.test.ts

@@ -1,5 +1,10 @@
-import { describe, expect, it } from "vitest";
-import { extractStandaloneEntryFromIndex } from "./renderOrchestrator.js";
+import { afterEach, describe, expect, it } from "vitest";
+import { existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs";
+import { join } from "node:path";
+import { tmpdir } from "node:os";
+
+import { extractStandaloneEntryFromIndex, writeCompiledArtifacts } from "./renderOrchestrator.js";
+import { toExternalAssetKey } from "../utils/paths.js";
 
 describe("extractStandaloneEntryFromIndex", () => {
   it("reuses the index wrapper and keeps only the requested composition host", () => {
@@ -59,3 +64,100 @@ describe("extractStandaloneEntryFromIndex", () => {
     expect(extracted).toBeNull();
   });
 });
+
+describe("writeCompiledArtifacts — external assets on Windows drive-letter paths (GH #321)", () => {
+  // End-to-end seam test: covers both `toExternalAssetKey` and
+  // `renderOrchestrator`'s copy step by simulating a Windows absolute
+  // path flowing through the full external-asset pipeline. The helpers
+  // are logically cross-platform, but this is the integration that
+  // guarantees they compose — catches any regression at the boundary.
+
+  const tempDirs: string[] = [];
+  afterEach(() => {
+    while (tempDirs.length > 0) {
+      const d = tempDirs.pop();
+      if (d) {
+        try {
+          rmSync(d, { recursive: true, force: true });
+        } catch {
+          /* ignore */
+        }
+      }
+    }
+  });
+
+  function makeWorkDir(): string {
+    const d = mkdtempSync(join(tmpdir(), "hf-orch-"));
+    tempDirs.push(d);
+    return d;
+  }
+
+  it("copies an external asset with a Windows-style drive-letter key into compileDir", () => {
+    const workDir = makeWorkDir();
+    // Simulate a real external asset: write a dummy file to an absolute
+    // path, then build the sanitised key the way `collectExternalAssets`
+    // would on Windows.
+    const sourceDir = mkdtempSync(join(tmpdir(), "hf-src-"));
+    tempDirs.push(sourceDir);
+    const srcFile = join(sourceDir, "segment.wav");
+    writeFileSync(srcFile, "fake wav bytes");
+
+    // The simulated Windows input is a path with backslashes and a drive
+    // letter — even though the test runs on Unix, the helper is expressed
+    // with regex on the string so we can exercise the Windows code path
+    // deterministically.
+    const windowsStyleInput = "D:\\coder\\assets\\segment.wav";
+    const key = toExternalAssetKey(windowsStyleInput);
+    expect(key).toBe("hf-ext/D/coder/assets/segment.wav");
+
+    const externalAssets = new Map<string, string>([[key, srcFile]]);
+    const compiled = {
+      html: "<!doctype html><html><body></body></html>",
+      subCompositions: new Map<string, string>(),
+      videos: [],
+      audios: [],
+      unresolvedCompositions: [],
+      externalAssets,
+      width: 1920,
+      height: 1080,
+      staticDuration: 10,
+    };
+
+    writeCompiledArtifacts(compiled, workDir, /* includeSummary */ false);
+
+    const landed = join(workDir, "compiled", key);
+    expect(existsSync(landed)).toBe(true);
+    expect(readFileSync(landed, "utf-8")).toBe("fake wav bytes");
+  });
+
+  it("rejects a maliciously crafted key that tries to escape compileDir", () => {
+    // Defense-in-depth: if a buggy upstream produced a key with `..`
+    // components, `isPathInside` at copy time must catch it and skip.
+    const workDir = makeWorkDir();
+    const sourceDir = mkdtempSync(join(tmpdir(), "hf-src-"));
+    tempDirs.push(sourceDir);
+    const srcFile = join(sourceDir, "evil.wav");
+    writeFileSync(srcFile, "should never be copied");
+
+    const externalAssets = new Map<string, string>([["hf-ext/../../etc/passwd", srcFile]]);
+    const compiled = {
+      html: "<!doctype html>",
+      subCompositions: new Map<string, string>(),
+      videos: [],
+      audios: [],
+      unresolvedCompositions: [],
+      externalAssets,
+      width: 1920,
+      height: 1080,
+      staticDuration: 10,
+    };
+
+    writeCompiledArtifacts(compiled, workDir, false);
+
+    // Assert that the file was NOT written outside compileDir (the
+    // attacker's target). We check the escape destination didn't
+    // materialise next to workDir.
+    const escapeTarget = join(workDir, "..", "..", "etc", "passwd");
+    expect(existsSync(escapeTarget)).toBe(false);
+  });
+});

packages/producer/src/services/renderOrchestrator.ts

@@ -68,6 +68,7 @@ import {
   type CompiledComposition,
 } from "./htmlCompiler.js";
 import { defaultLogger, type ProducerLogger } from "../logger.js";
+import { isPathInside } from "../utils/paths.js";
 
 /**
  * Wrap a cleanup operation so it never throws, but logs any failure.
@@ -246,7 +247,9 @@ function installDebugLogger(logPath: string, log: ProducerLogger = defaultLogger
 /**
  * Write compiled HTML and sub-compositions to the work directory.
  */
-function writeCompiledArtifacts(
+// Exported for integration tests. Not part of the stable public API —
+// callers outside this package should use `executeRenderJob` instead.
+export function writeCompiledArtifacts(
   compiled: CompiledComposition,
   workDir: string,
   includeSummary: boolean,
@@ -263,10 +266,13 @@ function writeCompiledArtifacts(
   }
 
   // Copy external assets (files outside projectDir) into the compiled directory
-  // so the file server can serve them.
+  // so the file server can serve them. The safe-path check uses
+  // `isPathInside()` rather than a hardcoded separator — on Windows,
+  // `compileDir + "/"` never matches because paths use `\\`, which caused
+  // every external asset to be wrongly rejected as "unsafe" (see GH #321).
   for (const [relativePath, absolutePath] of compiled.externalAssets) {
     const outPath = resolve(join(compileDir, relativePath));
-    if (!outPath.startsWith(compileDir + "/")) {
+    if (!isPathInside(outPath, compileDir)) {
       console.warn(`[Render] Skipping external asset with unsafe path: ${relativePath}`);
       continue;
     }

packages/producer/src/utils/paths.test.ts

@@ -0,0 +1,125 @@
+/**
+ * Cross-platform containment + external-asset-key tests.
+ *
+ * Regression coverage for GH #321 — on Windows, every external asset was
+ * wrongly rejected as "unsafe path" because the containment check used
+ * `startsWith(parent + "/")` and the safe key carried a drive-letter
+ * colon that made the downstream `path.join` absolute.
+ *
+ * We exercise both OS layouts by posing the hypothetical paths the
+ * respective platforms would generate — the logic itself is expressed
+ * using `path.relative()` so it works regardless of the runtime OS.
+ */
+
+import { describe, expect, it } from "vitest";
+import { resolve } from "node:path";
+
+import { isPathInside, toExternalAssetKey } from "./paths.js";
+
+describe("isPathInside", () => {
+  it("returns true when child is directly inside parent", () => {
+    expect(isPathInside(resolve("/foo/bar/baz.wav"), resolve("/foo/bar"))).toBe(true);
+  });
+
+  it("returns true when child is deeply nested inside parent", () => {
+    expect(isPathInside(resolve("/foo/bar/a/b/c/d.wav"), resolve("/foo/bar"))).toBe(true);
+  });
+
+  it("returns true when child equals parent (a dir contains itself)", () => {
+    expect(isPathInside(resolve("/foo/bar"), resolve("/foo/bar"))).toBe(true);
+  });
+
+  it("returns false when child is a sibling whose name starts with parent", () => {
+    // Regression: the old `startsWith(parent + "/")` accidentally worked for
+    // this case, but a naive rewrite without the trailing separator would
+    // admit `/foo/bar-sibling` as a child of `/foo/bar`. Verify we don't.
+    expect(isPathInside(resolve("/foo/bar-sibling/x"), resolve("/foo/bar"))).toBe(false);
+  });
+
+  it("returns false when child is outside parent", () => {
+    expect(isPathInside(resolve("/tmp/evil/file.wav"), resolve("/foo/bar"))).toBe(false);
+  });
+
+  it("returns false when child resolves above parent via ..", () => {
+    expect(isPathInside(resolve("/foo/bar/../../etc/passwd"), resolve("/foo/bar"))).toBe(false);
+  });
+
+  it("normalises trailing slashes on parent", () => {
+    expect(isPathInside(resolve("/foo/bar/baz"), resolve("/foo/bar/"))).toBe(true);
+  });
+});
+
+describe("toExternalAssetKey", () => {
+  it("prefixes with hf-ext/ and keeps a Unix absolute path", () => {
+    expect(toExternalAssetKey("/Users/miguel/assets/segment.wav")).toBe(
+      "hf-ext/Users/miguel/assets/segment.wav",
+    );
+  });
+
+  it("converts Windows drive-letter paths to a colonless, slash-delimited key", () => {
+    // GH #321: `D:\coder\reactGin\hyperframes\reading\assets\segment_001.wav`
+    // used to become `hf-ext/D:\coder\...`, which makes the downstream
+    // `path.join(compileDir, key)` absolute on Windows (drive letter wins).
+    expect(
+      toExternalAssetKey("D:\\coder\\reactGin\\hyperframes\\reading\\assets\\segment_001.wav"),
+    ).toBe("hf-ext/D/coder/reactGin/hyperframes/reading/assets/segment_001.wav");
+  });
+
+  it("handles Windows paths with forward slashes (mixed separators)", () => {
+    expect(toExternalAssetKey("C:/Users/Alice/Downloads/clip.mp4")).toBe(
+      "hf-ext/C/Users/Alice/Downloads/clip.mp4",
+    );
+  });
+
+  it("lowercases / uppercases drive letters faithfully (we don't munge)", () => {
+    expect(toExternalAssetKey("e:\\data\\a.wav")).toBe("hf-ext/e/data/a.wav");
+    expect(toExternalAssetKey("Z:\\data\\a.wav")).toBe("hf-ext/Z/data/a.wav");
+  });
+
+  it("is truly idempotent — double-wrap short-circuits on the hf-ext/ prefix", () => {
+    // Earlier revision of this test claimed "idempotent" but actually
+    // produced `hf-ext/hf-ext/...` — a silent doubling. The short-circuit
+    // on the hf-ext/ prefix makes the helper exactly idempotent now, so
+    // the invariant test matches the label.
+    const once = toExternalAssetKey("/foo/bar.mp3");
+    const twice = toExternalAssetKey(once);
+    expect(twice).toBe(once);
+  });
+
+  it("strips the Windows extended-length prefix (\\\\?\\)", () => {
+    expect(toExternalAssetKey("\\\\?\\D:\\very\\long\\path\\clip.mp4")).toBe(
+      "hf-ext/D/very/long/path/clip.mp4",
+    );
+  });
+
+  it("collapses UNC paths to unc/<server>/<share>/... so cross-server names can't collide", () => {
+    expect(toExternalAssetKey("\\\\server\\share\\file.wav")).toBe(
+      "hf-ext/unc/server/share/file.wav",
+    );
+  });
+
+  it("handles UNC extended-length form (\\\\?\\UNC\\server\\...)", () => {
+    expect(toExternalAssetKey("\\\\?\\UNC\\server\\share\\file.wav")).toBe(
+      "hf-ext/unc/server/share/file.wav",
+    );
+  });
+
+  it("treats leading double-slash as UNC (the Windows-correct reading)", () => {
+    // A leading `//host/share/...` is the Windows UNC form — NOT a Unix
+    // absolute path with an extra slash. The sanitiser now preserves the
+    // host/share boundary instead of collapsing it, matching the actual
+    // meaning of the input on the platform that produces these paths.
+    expect(toExternalAssetKey("//foo/bar.mp3")).toBe("hf-ext/unc/foo/bar.mp3");
+  });
+
+  it("produces a key that path.join(compileDir, key) keeps inside compileDir", () => {
+    // The real failure mode from #321: on Windows, join(compileDir, key) with
+    // a key containing a drive letter silently escaped compileDir. Our key
+    // must be a pure relative path — no `:`, no leading separator — so
+    // `isPathInside(join(compileDir, key), compileDir)` is always true.
+    const key = toExternalAssetKey("D:\\evil\\x.wav");
+    // Key cannot start with a separator or drive letter.
+    expect(key.startsWith("/")).toBe(false);
+    expect(/^[A-Za-z]:/.test(key)).toBe(false);
+  });
+});