refactor(core): promote isSafePath to a shared module + harden htmlBundler

jrusso1020 · claude · jrusso1020 · commit d620fe5ed6f5 · 2026-06-13T01:03:48.000Z
Per review on #1397: extend the symlink-escape fix to the compiler, and remove the duplicated path-safety logic. - Move isSafePath to packages/core/src/safePath.ts (a neutral package-root module). studio-api/helpers/safePath.ts re-exports it for back-compat (keeping walkDir), and it's now exported from the core entrypoint so non-studio-api layers can use it. compiler/ sits below studio-api in the dep graph, so it could not import the helper from its old home without a backwards edge — the promotion removes that constraint. - compiler/htmlBundler.ts: route both containment checks (the safePath helper and the inline CSS @import check) through isSafePath. The bundler reads+inlines these files, so an in-project symlink pointing outside the root would otherwise bake external content into the output. All callers already skip on a null/false result, so nothing is read on rejection. Tests: safePath.test.ts moves with the impl; htmlBundler.test.ts gains a case proving an in-project sub-composition script is inlined while a script reached through an escaping symlink is not (positive control + leak assertion). Deferred (tracked for a dedicated follow-up, see PR thread): the relative()-based isPathInside family (core/compiler/assetPaths, producer/services/fileServer, producer/utils/paths and their callers in the render pipeline) is symlink-blind in the same way, and engine videoFrameExtractor's asset resolver needs a caller-side gate (its http downloads land outside the project root, so a single-root check is wrong). Both are regression-sensitive render-pipeline surfaces that warrant their own focused, well-tested pass. renderArgs.ts is intentionally left: it is filesystem-free by design (injected stat) and its threat model is the user's own --composition CLI arg. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
diff --git a/packages/core/src/compiler/htmlBundler.test.ts b/packages/core/src/compiler/htmlBundler.test.ts
@@ -1,5 +1,5 @@
 // @vitest-environment node
-import { mkdtempSync, writeFileSync, mkdirSync } from "node:fs";
+import { mkdtempSync, writeFileSync, mkdirSync, symlinkSync } from "node:fs";
 import { tmpdir } from "node:os";
 import { join } from "node:path";
 import { parseHTML } from "linkedom";
@@ -50,6 +50,47 @@ describe("bundleToSingleHtml", () => {
     expect(bundled).toContain('document.getElementById("scene")');
   });
 
+  it("inlines an in-project sub-composition script but not one reached through a symlink escaping the project root", async () => {
+    // Security: a shared/cloned project may carry a symlink pointing outside the
+    // root (e.g. ext -> /etc). The bundler reads+inlines local assets, so it must
+    // refuse to follow such a symlink and leak external file contents.
+    const dir = makeTempProject({
+      "index.html": `<!doctype html>
+<html><head>
+  <script src="https://cdn.jsdelivr.net/npm/gsap@3.14.2/dist/gsap.min.js"></script>
+</head><body>
+  <div id="root" data-composition-id="main" data-width="1920" data-height="1080">
+    <div id="scene-host"
+      data-composition-id="scene"
+      data-composition-src="compositions/scene.html"
+      data-start="0" data-duration="5"></div>
+  </div>
+  <script>window.__timelines={}; const tl=gsap.timeline({paused:true}); window.__timelines["main"]=tl;</script>
+</body></html>`,
+      "compositions/scene.html": `<template id="scene-template">
+  <div data-composition-id="scene" data-width="1920" data-height="1080">
+    <script src="assets/local.js"></script>
+    <script src="ext/secret.js"></script>
+    <script>
+      window.__timelines = window.__timelines || {};
+      window.__timelines["scene"] = gsap.timeline({ paused: true });
+    </script>
+  </div>
+</template>`,
+      "assets/local.js": `window.__HF_LOCAL__ = "LOCAL_MARKER_INLINED";`,
+    });
+    const external = mkdtempSync(join(tmpdir(), "hf-bundler-external-"));
+    writeFileSync(join(external, "secret.js"), `window.__HF_SECRET__ = "SECRET_MARKER_LEAKED";`);
+    symlinkSync(external, join(dir, "ext"), "dir");
+
+    const bundled = await bundleToSingleHtml(dir);
+
+    // Positive control: the in-project sub-comp script IS inlined, so the bundler
+    // would have inlined the symlinked one too had isSafePath not rejected it.
+    expect(bundled).toContain("LOCAL_MARKER_INLINED");
+    expect(bundled).not.toContain("SECRET_MARKER_LEAKED");
+  });
+
   it("produces a self-contained runtime script when no HYPERFRAME_RUNTIME_URL is set", async () => {
     // Regression guard: hf#XXX. The bundler used to emit
     // <script ... src=""></script> when no runtime URL was configured. An
diff --git a/packages/core/src/compiler/htmlBundler.ts b/packages/core/src/compiler/htmlBundler.ts
@@ -17,13 +17,16 @@ import { validateHyperframeHtmlContract } from "./staticGuard";
 import { getHyperframeRuntimeScript } from "../generated/runtime-inline";
 import { readDeclaredDefaults } from "../runtime/getVariables";
 import { inlineSubCompositions } from "./inlineSubCompositions";
+import { isSafePath } from "../safePath.js";
 
-/** Resolve a relative path within projectDir, rejecting traversal outside it. */
+/**
+ * Resolve a relative path within projectDir, rejecting traversal outside it.
+ * Uses isSafePath so an in-project symlink pointing outside the root can't
+ * smuggle an external file into the bundle (this fn's result is read+inlined).
+ */
 function safePath(projectDir: string, relativePath: string): string | null {
   const resolved = resolve(projectDir, relativePath);
-  const normalizedBase = resolve(projectDir) + sep;
-  if (!resolved.startsWith(normalizedBase) && resolved !== resolve(projectDir)) return null;
-  return resolved;
+  return isSafePath(projectDir, resolved) ? resolved : null;
 }
 
 const DEFAULT_RUNTIME_SCRIPT_URL = "";
@@ -155,8 +158,9 @@ function inlineCssFile(
       const importPath = urlPath ?? barePath;
       if (!importPath || !isRelativeUrl(importPath)) return full;
       const resolved = resolve(cssFileDir, importPath);
-      const normalizedBase = resolve(projectDir) + sep;
-      if (!resolved.startsWith(normalizedBase)) return full;
+      // @import is resolved relative to the CSS file, but must stay within the
+      // project root; isSafePath also blocks symlink escapes (content is inlined).
+      if (!isSafePath(projectDir, resolved)) return full;
       if (visited.has(resolved)) return "";
       const content = safeReadFile(resolved);
       if (content == null) return full;
diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts
@@ -163,6 +163,7 @@ export {
   type MediaVisualStyleProperty,
 } from "./inline-scripts/parityContract";
 export { redactTelemetryString } from "./telemetryRedaction";
+export { isSafePath } from "./safePath";
 export type {
   HyperframePickerApi,
   HyperframePickerBoundingBox,
diff --git a/packages/core/src/safePath.test.ts b/packages/core/src/safePath.test.ts
diff --git a/packages/core/src/safePath.ts b/packages/core/src/safePath.ts
@@ -0,0 +1,54 @@
+import { resolve, sep, join, dirname, basename } from "node:path";
+import { realpathSync } from "node:fs";
+
+/**
+ * Reject paths that escape the `base` directory — including via symlinks.
+ *
+ * `path.resolve()` collapses `.`/`..` but does NOT dereference symlinks, so a
+ * plain prefix check (`resolved.startsWith(base + sep)`) can be defeated by a
+ * symlink that lives *inside* `base` but points outside it (e.g.
+ * `base/link -> /etc`). A downstream `readFileSync`/`writeFileSync`/`statSync`
+ * then follows that link to a file outside `base`. To close this we canonicalize
+ * both sides with `realpathSync` before comparing.
+ *
+ * The target may not exist yet (e.g. creating a new file), so we canonicalize the
+ * deepest *existing* ancestor and re-attach the trailing not-yet-existing
+ * segments. Segments that don't exist cannot be symlinks at check time, so they
+ * can't redirect the path outside `base` right now. (A symlink swapped in between
+ * this check and the subsequent fs call is an inherent TOCTOU race this helper
+ * does not, and cannot by itself, defend against.)
+ *
+ * Lives at the package root rather than under `studio-api/` because callers span
+ * layers — `studio-api` routes, the `compiler`, the CLI, and the engine — and
+ * `compiler` sits below `studio-api` in the dependency graph, so it cannot import
+ * from there without a backwards edge.
+ */
+export function isSafePath(base: string, resolved: string): boolean {
+  let baseReal: string;
+  try {
+    baseReal = realpathSync(resolve(base));
+  } catch {
+    // Base must exist and be resolvable; fail closed if not.
+    return false;
+  }
+
+  const target = resolve(resolved);
+  const trailing: string[] = [];
+  let probe = target;
+
+  for (;;) {
+    let ancestorReal: string;
+    try {
+      ancestorReal = realpathSync(probe);
+    } catch {
+      const parent = dirname(probe);
+      if (parent === probe) return false; // walked past the filesystem root
+      trailing.push(basename(probe));
+      probe = parent;
+      continue;
+    }
+
+    const targetReal = trailing.length ? join(ancestorReal, ...trailing.reverse()) : ancestorReal;
+    return targetReal === baseReal || targetReal.startsWith(baseReal + sep);
+  }
+}
diff --git a/packages/core/src/studio-api/helpers/safePath.ts b/packages/core/src/studio-api/helpers/safePath.ts
@@ -1,52 +1,10 @@
-import { resolve, sep, join, dirname, basename } from "node:path";
-import { readdirSync, realpathSync } from "node:fs";
+import { join } from "node:path";
+import { readdirSync } from "node:fs";
 
-/**
- * Reject paths that escape the project directory — including via symlinks.
- *
- * `path.resolve()` collapses `.`/`..` but does NOT dereference symlinks, so a
- * plain prefix check (`resolved.startsWith(base + sep)`) can be defeated by a
- * symlink that lives *inside* the project dir but points outside it (e.g.
- * `project/link -> /etc`). A downstream `readFileSync`/`writeFileSync`/`statSync`
- * then follows that link to a file outside `base`. To close this we canonicalize
- * both sides with `realpathSync` before comparing.
- *
- * The target may not exist yet (e.g. creating a new file), so we canonicalize the
- * deepest *existing* ancestor and re-attach the trailing not-yet-existing
- * segments. Segments that don't exist cannot be symlinks at check time, so they
- * can't redirect the path outside `base` right now. (A symlink swapped in between
- * this check and the subsequent fs call is an inherent TOCTOU race this helper
- * does not, and cannot by itself, defend against.)
- */
-export function isSafePath(base: string, resolved: string): boolean {
-  let baseReal: string;
-  try {
-    baseReal = realpathSync(resolve(base));
-  } catch {
-    // Base must exist and be resolvable; fail closed if not.
-    return false;
-  }
-
-  const target = resolve(resolved);
-  const trailing: string[] = [];
-  let probe = target;
-
-  for (;;) {
-    let ancestorReal: string;
-    try {
-      ancestorReal = realpathSync(probe);
-    } catch {
-      const parent = dirname(probe);
-      if (parent === probe) return false; // walked past the filesystem root
-      trailing.push(basename(probe));
-      probe = parent;
-      continue;
-    }
-
-    const targetReal = trailing.length ? join(ancestorReal, ...trailing.reverse()) : ancestorReal;
-    return targetReal === baseReal || targetReal.startsWith(baseReal + sep);
-  }
-}
+// `isSafePath` lives at the package root so non-studio-api layers (compiler,
+// CLI, engine) can share it without a backwards dependency on studio-api.
+// Re-exported here for back-compat with existing `../helpers/safePath.js` imports.
+export { isSafePath } from "../../safePath.js";
 
 const IGNORE_DIRS = new Set([".thumbnails", "node_modules", ".git"]);
 
