From 5b868c05742180f704635915c903b774661a6b65 Mon Sep 17 00:00:00 2001 From: James Date: Sat, 13 Jun 2026 00:26:54 +0000 Subject: [PATCH 1/4] fix(core): block symlink-based path escape in studio-api isSafePath path.resolve() collapses ./.. but does not dereference symlinks, so a symlink living inside the project dir but pointing outside it (e.g. project/link -> /etc) passed the prefix check, letting a downstream read/write/stat follow it to a file outside the project root. The `..` traversal case was already blocked; symlink traversal was the gap. Canonicalize both base and target with realpathSync before comparing. The target may not exist yet (new-file writes), so canonicalize the deepest existing ancestor and re-attach the trailing not-yet-existing segments, which cannot be symlinks at check time. Fail closed if base is unresolvable. Adds safePath.test.ts covering: in-base allow, not-yet-existing write target, `..` escape, existing-file-through-symlink escape, write-target under a symlinked parent, file-symlink escape, in-base symlink allow, symlinked-base canonicalization, and base-missing fail-closed. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/studio-api/helpers/safePath.test.ts | 97 +++++++++++++++++++ .../core/src/studio-api/helpers/safePath.ts | 51 +++++++++- 2 files changed, 143 insertions(+), 5 deletions(-) create mode 100644 packages/core/src/studio-api/helpers/safePath.test.ts diff --git a/packages/core/src/studio-api/helpers/safePath.test.ts b/packages/core/src/studio-api/helpers/safePath.test.ts new file mode 100644 index 000000000..c9428f168 --- /dev/null +++ b/packages/core/src/studio-api/helpers/safePath.test.ts @@ -0,0 +1,97 @@ +import { describe, it, expect, afterEach } from "vitest"; +import { mkdtempSync, mkdirSync, writeFileSync, symlinkSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { isSafePath } from "./safePath.js"; + +describe("isSafePath", () => { + const tmpDirs: string[] = []; + + afterEach(() => { + for (const d of tmpDirs) rmSync(d, { recursive: true, force: true }); + tmpDirs.length = 0; + }); + + function tmpDir(prefix: string): string { + const dir = mkdtempSync(join(tmpdir(), prefix)); + tmpDirs.push(dir); + return dir; + } + + it("allows the base directory itself", () => { + const base = tmpDir("safepath-base-"); + expect(isSafePath(base, base)).toBe(true); + }); + + it("allows an existing nested path inside base", () => { + const base = tmpDir("safepath-base-"); + const file = join(base, "assets", "logo.png"); + mkdirSync(join(base, "assets")); + writeFileSync(file, "x"); + expect(isSafePath(base, file)).toBe(true); + }); + + it("allows a not-yet-existing write target inside base", () => { + const base = tmpDir("safepath-base-"); + // Neither the dir nor the file exist yet — the create/write case. + expect(isSafePath(base, join(base, "new", "deep", "file.txt"))).toBe(true); + }); + + it("rejects a `..` traversal that escapes base", () => { + const base = tmpDir("safepath-base-"); + expect(isSafePath(base, join(base, "..", "..", "etc", "passwd"))).toBe(false); + }); + + it("rejects an existing file reached through a symlink that points outside base", () => { + const base = tmpDir("safepath-base-"); + const external = tmpDir("safepath-external-"); + const secret = join(external, "secret.txt"); + writeFileSync(secret, "top secret"); + // project/link -> external/ (the classic in-project symlink escape) + symlinkSync(external, join(base, "link"), "dir"); + expect(isSafePath(base, join(base, "link", "secret.txt"))).toBe(false); + }); + + it("rejects a not-yet-existing write target whose parent is a symlink to outside base", () => { + const base = tmpDir("safepath-base-"); + const external = tmpDir("safepath-external-"); + symlinkSync(external, join(base, "link"), "dir"); + // base/link -> external; writing base/link/evil.txt would land in external. + expect(isSafePath(base, join(base, "link", "evil.txt"))).toBe(false); + }); + + it("rejects a file symlink inside base that targets a file outside base", () => { + const base = tmpDir("safepath-base-"); + const external = tmpDir("safepath-external-"); + const secret = join(external, "secret.txt"); + writeFileSync(secret, "top secret"); + symlinkSync(secret, join(base, "passwd"), "file"); + expect(isSafePath(base, join(base, "passwd"))).toBe(false); + }); + + it("allows a symlink inside base that points to another location inside base", () => { + const base = tmpDir("safepath-base-"); + const realDir = join(base, "real"); + mkdirSync(realDir); + writeFileSync(join(realDir, "in.txt"), "x"); + symlinkSync(realDir, join(base, "alias"), "dir"); + expect(isSafePath(base, join(base, "alias", "in.txt"))).toBe(true); + }); + + it("canonicalizes base too: a symlinked base path still admits in-base targets", () => { + // Guards against one-sided realpath: when base is reached via a symlink + // (as on macOS where tmpdir lives under /var -> /private/var), an in-base + // target must still be accepted. + const realBase = tmpDir("safepath-realbase-"); + const linkParent = tmpDir("safepath-linkparent-"); + const baseLink = join(linkParent, "baseLink"); + symlinkSync(realBase, baseLink, "dir"); + writeFileSync(join(realBase, "file.txt"), "x"); + expect(isSafePath(baseLink, join(baseLink, "file.txt"))).toBe(true); + }); + + it("fails closed when the base directory does not exist", () => { + const base = join(tmpdir(), "safepath-does-not-exist-zzz", "nope"); + expect(isSafePath(base, join(base, "file.txt"))).toBe(false); + }); +}); diff --git a/packages/core/src/studio-api/helpers/safePath.ts b/packages/core/src/studio-api/helpers/safePath.ts index 7a925c3c6..81db63904 100644 --- a/packages/core/src/studio-api/helpers/safePath.ts +++ b/packages/core/src/studio-api/helpers/safePath.ts @@ -1,10 +1,51 @@ -import { resolve, sep, join } from "node:path"; -import { readdirSync } from "node:fs"; +import { resolve, sep, join, dirname, basename } from "node:path"; +import { readdirSync, realpathSync } from "node:fs"; -/** Reject paths that escape the project directory. */ +/** + * 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 { - const norm = resolve(base) + sep; - return resolved.startsWith(norm) || resolved === resolve(base); + 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); + } } const IGNORE_DIRS = new Set([".thumbnails", "node_modules", ".git"]); From e7127cdeef01e56c8494c050af97f536220938b0 Mon Sep 17 00:00:00 2001 From: James Date: Sat, 13 Jun 2026 00:43:48 +0000 Subject: [PATCH 2/4] fix(core,cli): route render + play composition paths through isSafePath MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review on #1397 found a third call site with the same vulnerable startsWith pattern. Apply Rule 2: fix every site sharing the contract (gate an attacker-influenced path before a symlink-following fs op). - studio-api routes/render.ts: body.composition (from c.req.json()) was checked with `resolved.startsWith(resolve(project.dir) + sep)`, which doesn't dereference symlinks — an in-project symlink to an external target escaped the project root. Now uses isSafePath(). - cli commands/play.ts: the `/composition/*` server route used `filePath.startsWith(project.dir)` with no trailing-separator guard, so both a sibling dir sharing the prefix (`-evil`) and symlink escapes passed. Now uses isSafePath() via @hyperframes/core/studio-api (the same lazy-import pattern commands/validate.ts already uses). Tests: render.test.ts gains a "composition path safety" block (in-base allow, `..` reject, in-project-symlink-to-outside reject, in-project symlink staying inside allow). The shared render test adapter now points at a real dir since isSafePath fails closed on an unresolvable base (production project dirs always exist on disk). Not in this change: compiler/htmlBundler.ts has the same class at two sites (safePath helper + inline CSS @import check), but the compiler sits below studio-api in the dependency graph and can't import isSafePath without a backwards edge; that fix needs the helper promoted to a neutral module and is tracked as a follow-up. renderArgs.ts / videoFrameExtractor.ts carry the trailing-sep guard and a local-CLI/engine-internal threat model. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/cli/src/commands/play.ts | 8 +- .../core/src/studio-api/routes/render.test.ts | 88 ++++++++++++++++++- packages/core/src/studio-api/routes/render.ts | 8 +- 3 files changed, 97 insertions(+), 7 deletions(-) diff --git a/packages/cli/src/commands/play.ts b/packages/cli/src/commands/play.ts index d237c555f..e9f254976 100644 --- a/packages/cli/src/commands/play.ts +++ b/packages/cli/src/commands/play.ts @@ -100,6 +100,7 @@ export default defineCommand({ const { Hono } = await import("hono"); const { createAdaptorServer } = await import("@hono/node-server"); + const { isSafePath } = await import("@hyperframes/core/studio-api"); const app = new Hono(); @@ -124,8 +125,11 @@ export default defineCommand({ const reqPath = ctx.req.path.replace("/composition/", ""); const filePath = resolve(project.dir, reqPath); - // Security: don't allow path traversal outside project dir - if (!filePath.startsWith(project.dir)) return ctx.text("Forbidden", 403); + // Security: don't allow path traversal outside project dir. isSafePath + // canonicalizes symlinks and applies a trailing-separator guard, so neither + // an in-project symlink to an external target nor a sibling dir whose name + // shares the project-dir prefix (e.g. `-evil`) can escape. + if (!isSafePath(project.dir, filePath)) return ctx.text("Forbidden", 403); if (!existsSync(filePath)) return ctx.text("Not found", 404); const content = readFileSync(filePath, "utf-8"); diff --git a/packages/core/src/studio-api/routes/render.test.ts b/packages/core/src/studio-api/routes/render.test.ts index b7a168747..9852eae94 100644 --- a/packages/core/src/studio-api/routes/render.test.ts +++ b/packages/core/src/studio-api/routes/render.test.ts @@ -1,6 +1,6 @@ -import { describe, expect, it, vi } from "vitest"; +import { afterEach, describe, expect, it, vi } from "vitest"; import { Hono } from "hono"; -import { mkdtempSync, rmSync } from "node:fs"; +import { mkdtempSync, mkdirSync, writeFileSync, symlinkSync, rmSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { VALID_CANVAS_RESOLUTIONS } from "../../core.types"; @@ -13,7 +13,9 @@ function createAdapter( ): { adapter: StudioApiAdapter; rendersDir: string } { const adapter: StudioApiAdapter = { listProjects: () => [], - resolveProject: async (id: string) => ({ id, dir: "/tmp/proj" }), + // Use a real, existing dir: isSafePath() canonicalizes the project dir with + // realpath and fails closed if it doesn't exist (real projects always do). + resolveProject: async (id: string) => ({ id, dir: tmpdir() }), bundle: async () => null, lint: async () => ({ findings: [] }), runtimeUrl: "/api/runtime.js", @@ -261,3 +263,83 @@ describe("POST /projects/:id/render — fps wire format", () => { } }); }); + +describe("POST /projects/:id/render — composition path safety", () => { + const tmpDirs: string[] = []; + + function buildAppWithProjectDir(spy: ReturnType): { + app: Hono; + projectDir: string; + } { + const projectDir = mkdtempSync(join(tmpdir(), "hf-render-proj-")); + const rendersDir = mkdtempSync(join(tmpdir(), "hf-render-out-")); + tmpDirs.push(projectDir, rendersDir); + const adapter: StudioApiAdapter = { + listProjects: () => [], + resolveProject: async (id: string) => ({ id, dir: projectDir }), + bundle: async () => null, + lint: async () => ({ findings: [] }), + runtimeUrl: "/api/runtime.js", + rendersDir: () => rendersDir, + startRender: (opts) => { + spy(opts); + return { id: opts.jobId, status: "rendering", progress: 0, outputPath: opts.outputPath }; + }, + }; + const app = new Hono(); + registerRenderRoutes(app, adapter); + return { app, projectDir }; + } + + afterEach(() => { + for (const d of tmpDirs) rmSync(d, { recursive: true, force: true }); + tmpDirs.length = 0; + }); + + async function postComposition(app: Hono, composition: string): Promise { + return app.request("http://localhost/projects/demo/render", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ fps: 30, quality: "standard", format: "mp4", composition }), + }); + } + + it("accepts a composition path inside the project directory", async () => { + const spy = vi.fn(); + const { app } = buildAppWithProjectDir(spy); + const res = await postComposition(app, "scenes/intro.html"); + expect(res.status).toBe(200); + expect(spy).toHaveBeenCalledOnce(); + }); + + it("rejects a `..` traversal in the composition path", async () => { + const spy = vi.fn(); + const { app } = buildAppWithProjectDir(spy); + const res = await postComposition(app, "../../etc/passwd"); + expect(res.status).toBe(400); + expect(spy).not.toHaveBeenCalled(); + }); + + it("rejects a composition reached through an in-project symlink pointing outside the project", async () => { + const spy = vi.fn(); + const { app, projectDir } = buildAppWithProjectDir(spy); + const external = mkdtempSync(join(tmpdir(), "hf-render-external-")); + tmpDirs.push(external); + writeFileSync(join(external, "secret.html"), ""); + symlinkSync(external, join(projectDir, "link"), "dir"); + const res = await postComposition(app, "link/secret.html"); + expect(res.status).toBe(400); + expect(spy).not.toHaveBeenCalled(); + }); + + it("allows a composition reached through an in-project symlink that stays inside the project", async () => { + const spy = vi.fn(); + const { app, projectDir } = buildAppWithProjectDir(spy); + mkdirSync(join(projectDir, "real")); + writeFileSync(join(projectDir, "real", "scene.html"), ""); + symlinkSync(join(projectDir, "real"), join(projectDir, "alias"), "dir"); + const res = await postComposition(app, "alias/scene.html"); + expect(res.status).toBe(200); + expect(spy).toHaveBeenCalledOnce(); + }); +}); diff --git a/packages/core/src/studio-api/routes/render.ts b/packages/core/src/studio-api/routes/render.ts index 18caaafa5..70c34a3ab 100644 --- a/packages/core/src/studio-api/routes/render.ts +++ b/packages/core/src/studio-api/routes/render.ts @@ -1,9 +1,10 @@ import type { Hono } from "hono"; import { streamSSE } from "hono/streaming"; import { existsSync, readFileSync, mkdirSync, unlinkSync, readdirSync, statSync } from "node:fs"; -import { join, resolve, sep } from "node:path"; +import { join, resolve } from "node:path"; import type { StudioApiAdapter, RenderJobState } from "../types.js"; import { VALID_CANVAS_RESOLUTIONS, parseFps, type CanvasResolution } from "../../core.types.js"; +import { isSafePath } from "../helpers/safePath.js"; const VALID_RESOLUTIONS = new Set(VALID_CANVAS_RESOLUTIONS); @@ -80,7 +81,10 @@ export function registerRenderRoutes(api: Hono, adapter: StudioApiAdapter): void let composition: string | undefined; if (typeof body.composition === "string" && body.composition.length > 0) { const resolved = resolve(project.dir, body.composition); - if (!resolved.startsWith(resolve(project.dir) + sep)) { + // `body.composition` is attacker-controlled (from c.req.json()). isSafePath + // dereferences symlinks so an in-project symlink pointing outside the root + // can't smuggle the render target out of the project dir. + if (!isSafePath(project.dir, resolved)) { return c.json({ error: "composition path must be within the project directory" }, 400); } composition = body.composition; From d620fe5ed6f5a3950815040ecaf68b24079897f1 Mon Sep 17 00:00:00 2001 From: James Date: Sat, 13 Jun 2026 01:03:48 +0000 Subject: [PATCH 3/4] refactor(core): promote isSafePath to a shared module + harden htmlBundler MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../core/src/compiler/htmlBundler.test.ts | 43 ++++++++++++++- packages/core/src/compiler/htmlBundler.ts | 16 +++--- packages/core/src/index.ts | 1 + .../{studio-api/helpers => }/safePath.test.ts | 0 packages/core/src/safePath.ts | 54 +++++++++++++++++++ .../core/src/studio-api/helpers/safePath.ts | 54 +++---------------- 6 files changed, 113 insertions(+), 55 deletions(-) rename packages/core/src/{studio-api/helpers => }/safePath.test.ts (100%) create mode 100644 packages/core/src/safePath.ts diff --git a/packages/core/src/compiler/htmlBundler.test.ts b/packages/core/src/compiler/htmlBundler.test.ts index bbf5bfef6..6493c916d 100644 --- 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": ` + + + +
+
+
+ +`, + "compositions/scene.html": ``, + "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 // when no runtime URL was configured. An diff --git a/packages/core/src/compiler/htmlBundler.ts b/packages/core/src/compiler/htmlBundler.ts index 121b89336..c0f47d76c 100644 --- 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 index 747e32abf..69a70818a 100644 --- 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/studio-api/helpers/safePath.test.ts b/packages/core/src/safePath.test.ts similarity index 100% rename from packages/core/src/studio-api/helpers/safePath.test.ts rename to packages/core/src/safePath.test.ts diff --git a/packages/core/src/safePath.ts b/packages/core/src/safePath.ts new file mode 100644 index 000000000..dea1661c3 --- /dev/null +++ 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 index 81db63904..8607549b6 100644 --- 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"]); From 6e1a89c1f86be0ac750dd0850d9f53e3574ace6b Mon Sep 17 00:00:00 2001 From: James Date: Sat, 13 Jun 2026 02:34:14 +0000 Subject: [PATCH 4/4] test(core): hedge symlink tests for Windows + copy before reverse (review nits) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses Via's non-blocking review notes on #1397: - Wrap every symlinkSync in the new tests with a tryCreateSymlink helper that returns false (and the test early-returns) when creation throws, mirroring the preview.test.ts convention. Non-symlink-privileged Windows runners no longer risk crashing the suite on EPERM. - safePath.ts: `[...trailing].reverse()` instead of mutating `trailing` in place — harmless today (single return) but future-proof against a looping edit. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../core/src/compiler/htmlBundler.test.ts | 13 +++++++++++- packages/core/src/safePath.test.ts | 21 ++++++++++++++----- packages/core/src/safePath.ts | 6 +++++- .../core/src/studio-api/routes/render.test.ts | 15 +++++++++++-- 4 files changed, 46 insertions(+), 9 deletions(-) diff --git a/packages/core/src/compiler/htmlBundler.test.ts b/packages/core/src/compiler/htmlBundler.test.ts index 6493c916d..235b8a61a 100644 --- a/packages/core/src/compiler/htmlBundler.test.ts +++ b/packages/core/src/compiler/htmlBundler.test.ts @@ -17,6 +17,17 @@ function makeTempProject(files: Record): string { return dir; } +// Mirror the repo convention (preview.test.ts): skip symlink cases on +// non-symlink-privileged Windows runners rather than crash the suite. +function tryCreateSymlink(target: string, path: string, type: "dir" | "file"): boolean { + try { + symlinkSync(target, path, type); + return true; + } catch { + return false; + } +} + describe("bundleToSingleHtml", () => { it("does not merge author scripts into the runtime bootstrap placeholder", async () => { const dir = makeTempProject({ @@ -81,7 +92,7 @@ describe("bundleToSingleHtml", () => { }); 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"); + if (!tryCreateSymlink(external, join(dir, "ext"), "dir")) return; const bundled = await bundleToSingleHtml(dir); diff --git a/packages/core/src/safePath.test.ts b/packages/core/src/safePath.test.ts index c9428f168..47455ed79 100644 --- a/packages/core/src/safePath.test.ts +++ b/packages/core/src/safePath.test.ts @@ -18,6 +18,17 @@ describe("isSafePath", () => { return dir; } + // Mirror the repo convention (preview.test.ts): non-symlink-privileged Windows + // runners can't create symlinks — skip those cases rather than crash the suite. + function tryCreateSymlink(target: string, path: string, type: "dir" | "file"): boolean { + try { + symlinkSync(target, path, type); + return true; + } catch { + return false; + } + } + it("allows the base directory itself", () => { const base = tmpDir("safepath-base-"); expect(isSafePath(base, base)).toBe(true); @@ -48,15 +59,15 @@ describe("isSafePath", () => { const secret = join(external, "secret.txt"); writeFileSync(secret, "top secret"); // project/link -> external/ (the classic in-project symlink escape) - symlinkSync(external, join(base, "link"), "dir"); + if (!tryCreateSymlink(external, join(base, "link"), "dir")) return; expect(isSafePath(base, join(base, "link", "secret.txt"))).toBe(false); }); it("rejects a not-yet-existing write target whose parent is a symlink to outside base", () => { const base = tmpDir("safepath-base-"); const external = tmpDir("safepath-external-"); - symlinkSync(external, join(base, "link"), "dir"); // base/link -> external; writing base/link/evil.txt would land in external. + if (!tryCreateSymlink(external, join(base, "link"), "dir")) return; expect(isSafePath(base, join(base, "link", "evil.txt"))).toBe(false); }); @@ -65,7 +76,7 @@ describe("isSafePath", () => { const external = tmpDir("safepath-external-"); const secret = join(external, "secret.txt"); writeFileSync(secret, "top secret"); - symlinkSync(secret, join(base, "passwd"), "file"); + if (!tryCreateSymlink(secret, join(base, "passwd"), "file")) return; expect(isSafePath(base, join(base, "passwd"))).toBe(false); }); @@ -74,7 +85,7 @@ describe("isSafePath", () => { const realDir = join(base, "real"); mkdirSync(realDir); writeFileSync(join(realDir, "in.txt"), "x"); - symlinkSync(realDir, join(base, "alias"), "dir"); + if (!tryCreateSymlink(realDir, join(base, "alias"), "dir")) return; expect(isSafePath(base, join(base, "alias", "in.txt"))).toBe(true); }); @@ -85,7 +96,7 @@ describe("isSafePath", () => { const realBase = tmpDir("safepath-realbase-"); const linkParent = tmpDir("safepath-linkparent-"); const baseLink = join(linkParent, "baseLink"); - symlinkSync(realBase, baseLink, "dir"); + if (!tryCreateSymlink(realBase, baseLink, "dir")) return; writeFileSync(join(realBase, "file.txt"), "x"); expect(isSafePath(baseLink, join(baseLink, "file.txt"))).toBe(true); }); diff --git a/packages/core/src/safePath.ts b/packages/core/src/safePath.ts index dea1661c3..bf6517224 100644 --- a/packages/core/src/safePath.ts +++ b/packages/core/src/safePath.ts @@ -48,7 +48,11 @@ export function isSafePath(base: string, resolved: string): boolean { continue; } - const targetReal = trailing.length ? join(ancestorReal, ...trailing.reverse()) : ancestorReal; + // Copy before reverse(): the array is only consumed once today, but a future + // edit that loops would otherwise silently misorder the rebuilt segments. + const targetReal = trailing.length + ? join(ancestorReal, ...[...trailing].reverse()) + : ancestorReal; return targetReal === baseReal || targetReal.startsWith(baseReal + sep); } } diff --git a/packages/core/src/studio-api/routes/render.test.ts b/packages/core/src/studio-api/routes/render.test.ts index 9852eae94..0d92e49c9 100644 --- a/packages/core/src/studio-api/routes/render.test.ts +++ b/packages/core/src/studio-api/routes/render.test.ts @@ -304,6 +304,17 @@ describe("POST /projects/:id/render — composition path safety", () => { }); } + // Mirror the repo convention (preview.test.ts): skip symlink cases on + // non-symlink-privileged Windows runners rather than crash the suite. + function tryCreateSymlink(target: string, path: string, type: "dir" | "file"): boolean { + try { + symlinkSync(target, path, type); + return true; + } catch { + return false; + } + } + it("accepts a composition path inside the project directory", async () => { const spy = vi.fn(); const { app } = buildAppWithProjectDir(spy); @@ -326,7 +337,7 @@ describe("POST /projects/:id/render — composition path safety", () => { const external = mkdtempSync(join(tmpdir(), "hf-render-external-")); tmpDirs.push(external); writeFileSync(join(external, "secret.html"), ""); - symlinkSync(external, join(projectDir, "link"), "dir"); + if (!tryCreateSymlink(external, join(projectDir, "link"), "dir")) return; const res = await postComposition(app, "link/secret.html"); expect(res.status).toBe(400); expect(spy).not.toHaveBeenCalled(); @@ -337,7 +348,7 @@ describe("POST /projects/:id/render — composition path safety", () => { const { app, projectDir } = buildAppWithProjectDir(spy); mkdirSync(join(projectDir, "real")); writeFileSync(join(projectDir, "real", "scene.html"), ""); - symlinkSync(join(projectDir, "real"), join(projectDir, "alias"), "dir"); + if (!tryCreateSymlink(join(projectDir, "real"), join(projectDir, "alias"), "dir")) return; const res = await postComposition(app, "alias/scene.html"); expect(res.status).toBe(200); expect(spy).toHaveBeenCalledOnce();