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

* fix(producer): external assets work on Windows (GH #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 #321. * fix(producer): address review on #324 — UNC + integration test Addresses the non-blocking observations from the PR #324 staff review (https://github.com/heygen-com/hyperframes/pull/324#issuecomment): 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.
2026-09-01 19:42:03 +00:00 · 2026-04-18 20:59:51 +02:00
parent 64e3735100
commit e8a48a62d0
6 changed files with 342 additions and 9 deletions
@@ -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}>
@@ -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;
  }
@@ -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);
+  });
+});
@@ -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;
    }
@@ -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);
+  });
+});
@@ -2,7 +2,7 @@
 * Path resolution utilities for the render pipeline.
 */

-import { resolve, basename, join } from "node:path";
+import { resolve, basename, join, relative, isAbsolute } from "node:path";

 export interface RenderPaths {
  absoluteProjectDir: string;
@@ -13,6 +13,79 @@ const DEFAULT_RENDERS_DIR =
  process.env.PRODUCER_RENDERS_DIR ??
  resolve(new URL(import.meta.url).pathname, "../../..", "renders");

+/**
+ * Cross-platform containment check.
+ *
+ * `child.startsWith(parent + "/")` breaks on Windows because the path
+ * separator is `\`, not `/`. This helper uses `path.relative()` which
+ * normalises separators per-platform and returns `..`-prefixed output
+ * for out-of-tree paths — the canonical way to ask "is `child` inside
+ * `parent`?" on every supported OS.
+ *
+ * Both inputs are normalised via `resolve()` so callers don't need to.
+ * Equality counts as "inside" (a directory contains itself).
+ */
+export function isPathInside(childPath: string, parentPath: string): boolean {
+  const absChild = resolve(childPath);
+  const absParent = resolve(parentPath);
+  if (absChild === absParent) return true;
+  const rel = relative(absParent, absChild);
+  // `relative()` returns "" when paths are equal, ".." or "..\\foo" when child
+  // is above the parent, and an absolute path when they live on different
+  // drives/volumes (Windows) — none of which count as "inside".
+  return rel !== "" && !rel.startsWith("..") && !isAbsolute(rel);
+}
+
+/**
+ * Build a safe, cross-platform relative key for an absolute asset path
+ * that lives outside the project directory.
+ *
+ * Windows absolute paths (`D:\coder\assets\segment.wav`) break two
+ * downstream assumptions when passed as-is to `path.join(compileDir, key)`:
+ *   1. The drive letter makes the path absolute, so `join()` silently
+ *      discards `compileDir`.
+ *   2. The backslashes and colon are invalid inside some OS sandboxes
+ *      and HTTP URL encodings.
+ *
+ * We sanitise into `hf-ext/...` form using forward slashes, stripping
+ * the colon after drive letters, the Windows extended-length prefix
+ * (`\\?\`), and the UNC prefix (`\\server\share\`). The result is a
+ * pure relative path that joins cleanly on every platform.
+ *
+ * Caller contract: `absPath` is expected to be canonical — typically
+ * produced by `path.resolve()` upstream. This helper does NOT strip
+ * `..` components on its own. `isPathInside` at copy time is the
+ * defensive backstop.
+ */
+export function toExternalAssetKey(absPath: string): string {
+  // Short-circuit if already a sanitised key — prevents double-wrap
+  // producing `hf-ext/hf-ext/...`.
+  if (absPath.startsWith("hf-ext/")) return absPath;
+
+  // Normalise to forward slashes first so every subsequent pattern is
+  // separator-agnostic.
+  let normalised = absPath.replace(/\\/g, "/");
+
+  // Windows extended-length prefix: `//?/` (was `\\?\`). Strip entirely —
+  // the actual path follows. `//?/UNC/server/share/...` is the UNC
+  // extended-length form; normalise to match the UNC branch below.
+  normalised = normalised.replace(/^\/\/\?\/UNC\//i, "//");
+  normalised = normalised.replace(/^\/\/\?\//, "");
+
+  // UNC paths (`\\server\share\file`). Collapse to
+  // `unc/server/share/file` so two different servers can't collide
+  // under the same relative key.
+  normalised = normalised.replace(/^\/\/([^/]+)\//, "unc/$1/");
+
+  // Strip remaining leading forward slashes (Unix absolute).
+  normalised = normalised.replace(/^\/+/, "");
+
+  // Strip a leading drive-letter colon (Windows: "D:/coder" → "D/coder").
+  normalised = normalised.replace(/^([A-Za-z]):\/?/, "$1/");
+
+  return "hf-ext/" + normalised;
+}
+
 export function resolveRenderPaths(
  projectDir: string,
  outputPath: string | null | undefined,