Skip to content

Commit 953bab3

Browse files
jrusso1020claude
andauthored
fix(core): block symlink-based path escape in studio-api isSafePath (#1397)
* 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) <noreply@anthropic.com> * fix(core,cli): route render + play composition paths through isSafePath 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 (`<dir>-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) <noreply@anthropic.com> * refactor(core): promote isSafePath to a shared module + harden htmlBundler 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> * test(core): hedge symlink tests for Windows + copy before reverse (review nits) 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) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 3bcab3d commit 953bab3

9 files changed

Lines changed: 343 additions & 20 deletions

File tree

packages/cli/src/commands/play.ts

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,7 @@ export default defineCommand({
100100

101101
const { Hono } = await import("hono");
102102
const { createAdaptorServer } = await import("@hono/node-server");
103+
const { isSafePath } = await import("@hyperframes/core/studio-api");
103104

104105
const app = new Hono();
105106

@@ -124,8 +125,11 @@ export default defineCommand({
124125
const reqPath = ctx.req.path.replace("/composition/", "");
125126
const filePath = resolve(project.dir, reqPath);
126127

127-
// Security: don't allow path traversal outside project dir
128-
if (!filePath.startsWith(project.dir)) return ctx.text("Forbidden", 403);
128+
// Security: don't allow path traversal outside project dir. isSafePath
129+
// canonicalizes symlinks and applies a trailing-separator guard, so neither
130+
// an in-project symlink to an external target nor a sibling dir whose name
131+
// shares the project-dir prefix (e.g. `<dir>-evil`) can escape.
132+
if (!isSafePath(project.dir, filePath)) return ctx.text("Forbidden", 403);
129133
if (!existsSync(filePath)) return ctx.text("Not found", 404);
130134

131135
const content = readFileSync(filePath, "utf-8");

packages/core/src/compiler/htmlBundler.test.ts

Lines changed: 53 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
// @vitest-environment node
2-
import { mkdtempSync, writeFileSync, mkdirSync } from "node:fs";
2+
import { mkdtempSync, writeFileSync, mkdirSync, symlinkSync } from "node:fs";
33
import { tmpdir } from "node:os";
44
import { join } from "node:path";
55
import { parseHTML } from "linkedom";
@@ -17,6 +17,17 @@ function makeTempProject(files: Record<string, string>): string {
1717
return dir;
1818
}
1919

20+
// Mirror the repo convention (preview.test.ts): skip symlink cases on
21+
// non-symlink-privileged Windows runners rather than crash the suite.
22+
function tryCreateSymlink(target: string, path: string, type: "dir" | "file"): boolean {
23+
try {
24+
symlinkSync(target, path, type);
25+
return true;
26+
} catch {
27+
return false;
28+
}
29+
}
30+
2031
describe("bundleToSingleHtml", () => {
2132
it("does not merge author scripts into the runtime bootstrap placeholder", async () => {
2233
const dir = makeTempProject({
@@ -50,6 +61,47 @@ describe("bundleToSingleHtml", () => {
5061
expect(bundled).toContain('document.getElementById("scene")');
5162
});
5263

64+
it("inlines an in-project sub-composition script but not one reached through a symlink escaping the project root", async () => {
65+
// Security: a shared/cloned project may carry a symlink pointing outside the
66+
// root (e.g. ext -> /etc). The bundler reads+inlines local assets, so it must
67+
// refuse to follow such a symlink and leak external file contents.
68+
const dir = makeTempProject({
69+
"index.html": `<!doctype html>
70+
<html><head>
71+
<script src="https://cdn.jsdelivr.net/npm/gsap@3.14.2/dist/gsap.min.js"></script>
72+
</head><body>
73+
<div id="root" data-composition-id="main" data-width="1920" data-height="1080">
74+
<div id="scene-host"
75+
data-composition-id="scene"
76+
data-composition-src="compositions/scene.html"
77+
data-start="0" data-duration="5"></div>
78+
</div>
79+
<script>window.__timelines={}; const tl=gsap.timeline({paused:true}); window.__timelines["main"]=tl;</script>
80+
</body></html>`,
81+
"compositions/scene.html": `<template id="scene-template">
82+
<div data-composition-id="scene" data-width="1920" data-height="1080">
83+
<script src="assets/local.js"></script>
84+
<script src="ext/secret.js"></script>
85+
<script>
86+
window.__timelines = window.__timelines || {};
87+
window.__timelines["scene"] = gsap.timeline({ paused: true });
88+
</script>
89+
</div>
90+
</template>`,
91+
"assets/local.js": `window.__HF_LOCAL__ = "LOCAL_MARKER_INLINED";`,
92+
});
93+
const external = mkdtempSync(join(tmpdir(), "hf-bundler-external-"));
94+
writeFileSync(join(external, "secret.js"), `window.__HF_SECRET__ = "SECRET_MARKER_LEAKED";`);
95+
if (!tryCreateSymlink(external, join(dir, "ext"), "dir")) return;
96+
97+
const bundled = await bundleToSingleHtml(dir);
98+
99+
// Positive control: the in-project sub-comp script IS inlined, so the bundler
100+
// would have inlined the symlinked one too had isSafePath not rejected it.
101+
expect(bundled).toContain("LOCAL_MARKER_INLINED");
102+
expect(bundled).not.toContain("SECRET_MARKER_LEAKED");
103+
});
104+
53105
it("produces a self-contained runtime script when no HYPERFRAME_RUNTIME_URL is set", async () => {
54106
// Regression guard: hf#XXX. The bundler used to emit
55107
// <script ... src=""></script> when no runtime URL was configured. An

packages/core/src/compiler/htmlBundler.ts

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -17,13 +17,16 @@ import { validateHyperframeHtmlContract } from "./staticGuard";
1717
import { getHyperframeRuntimeScript } from "../generated/runtime-inline";
1818
import { readDeclaredDefaults } from "../runtime/getVariables";
1919
import { inlineSubCompositions } from "./inlineSubCompositions";
20+
import { isSafePath } from "../safePath.js";
2021

21-
/** Resolve a relative path within projectDir, rejecting traversal outside it. */
22+
/**
23+
* Resolve a relative path within projectDir, rejecting traversal outside it.
24+
* Uses isSafePath so an in-project symlink pointing outside the root can't
25+
* smuggle an external file into the bundle (this fn's result is read+inlined).
26+
*/
2227
function safePath(projectDir: string, relativePath: string): string | null {
2328
const resolved = resolve(projectDir, relativePath);
24-
const normalizedBase = resolve(projectDir) + sep;
25-
if (!resolved.startsWith(normalizedBase) && resolved !== resolve(projectDir)) return null;
26-
return resolved;
29+
return isSafePath(projectDir, resolved) ? resolved : null;
2730
}
2831

2932
const DEFAULT_RUNTIME_SCRIPT_URL = "";
@@ -155,8 +158,9 @@ function inlineCssFile(
155158
const importPath = urlPath ?? barePath;
156159
if (!importPath || !isRelativeUrl(importPath)) return full;
157160
const resolved = resolve(cssFileDir, importPath);
158-
const normalizedBase = resolve(projectDir) + sep;
159-
if (!resolved.startsWith(normalizedBase)) return full;
161+
// @import is resolved relative to the CSS file, but must stay within the
162+
// project root; isSafePath also blocks symlink escapes (content is inlined).
163+
if (!isSafePath(projectDir, resolved)) return full;
160164
if (visited.has(resolved)) return "";
161165
const content = safeReadFile(resolved);
162166
if (content == null) return full;

packages/core/src/index.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -163,6 +163,7 @@ export {
163163
type MediaVisualStyleProperty,
164164
} from "./inline-scripts/parityContract";
165165
export { redactTelemetryString } from "./telemetryRedaction";
166+
export { isSafePath } from "./safePath";
166167
export type {
167168
HyperframePickerApi,
168169
HyperframePickerBoundingBox,

packages/core/src/safePath.test.ts

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,108 @@
1+
import { describe, it, expect, afterEach } from "vitest";
2+
import { mkdtempSync, mkdirSync, writeFileSync, symlinkSync, rmSync } from "node:fs";
3+
import { tmpdir } from "node:os";
4+
import { join } from "node:path";
5+
import { isSafePath } from "./safePath.js";
6+
7+
describe("isSafePath", () => {
8+
const tmpDirs: string[] = [];
9+
10+
afterEach(() => {
11+
for (const d of tmpDirs) rmSync(d, { recursive: true, force: true });
12+
tmpDirs.length = 0;
13+
});
14+
15+
function tmpDir(prefix: string): string {
16+
const dir = mkdtempSync(join(tmpdir(), prefix));
17+
tmpDirs.push(dir);
18+
return dir;
19+
}
20+
21+
// Mirror the repo convention (preview.test.ts): non-symlink-privileged Windows
22+
// runners can't create symlinks — skip those cases rather than crash the suite.
23+
function tryCreateSymlink(target: string, path: string, type: "dir" | "file"): boolean {
24+
try {
25+
symlinkSync(target, path, type);
26+
return true;
27+
} catch {
28+
return false;
29+
}
30+
}
31+
32+
it("allows the base directory itself", () => {
33+
const base = tmpDir("safepath-base-");
34+
expect(isSafePath(base, base)).toBe(true);
35+
});
36+
37+
it("allows an existing nested path inside base", () => {
38+
const base = tmpDir("safepath-base-");
39+
const file = join(base, "assets", "logo.png");
40+
mkdirSync(join(base, "assets"));
41+
writeFileSync(file, "x");
42+
expect(isSafePath(base, file)).toBe(true);
43+
});
44+
45+
it("allows a not-yet-existing write target inside base", () => {
46+
const base = tmpDir("safepath-base-");
47+
// Neither the dir nor the file exist yet — the create/write case.
48+
expect(isSafePath(base, join(base, "new", "deep", "file.txt"))).toBe(true);
49+
});
50+
51+
it("rejects a `..` traversal that escapes base", () => {
52+
const base = tmpDir("safepath-base-");
53+
expect(isSafePath(base, join(base, "..", "..", "etc", "passwd"))).toBe(false);
54+
});
55+
56+
it("rejects an existing file reached through a symlink that points outside base", () => {
57+
const base = tmpDir("safepath-base-");
58+
const external = tmpDir("safepath-external-");
59+
const secret = join(external, "secret.txt");
60+
writeFileSync(secret, "top secret");
61+
// project/link -> external/ (the classic in-project symlink escape)
62+
if (!tryCreateSymlink(external, join(base, "link"), "dir")) return;
63+
expect(isSafePath(base, join(base, "link", "secret.txt"))).toBe(false);
64+
});
65+
66+
it("rejects a not-yet-existing write target whose parent is a symlink to outside base", () => {
67+
const base = tmpDir("safepath-base-");
68+
const external = tmpDir("safepath-external-");
69+
// base/link -> external; writing base/link/evil.txt would land in external.
70+
if (!tryCreateSymlink(external, join(base, "link"), "dir")) return;
71+
expect(isSafePath(base, join(base, "link", "evil.txt"))).toBe(false);
72+
});
73+
74+
it("rejects a file symlink inside base that targets a file outside base", () => {
75+
const base = tmpDir("safepath-base-");
76+
const external = tmpDir("safepath-external-");
77+
const secret = join(external, "secret.txt");
78+
writeFileSync(secret, "top secret");
79+
if (!tryCreateSymlink(secret, join(base, "passwd"), "file")) return;
80+
expect(isSafePath(base, join(base, "passwd"))).toBe(false);
81+
});
82+
83+
it("allows a symlink inside base that points to another location inside base", () => {
84+
const base = tmpDir("safepath-base-");
85+
const realDir = join(base, "real");
86+
mkdirSync(realDir);
87+
writeFileSync(join(realDir, "in.txt"), "x");
88+
if (!tryCreateSymlink(realDir, join(base, "alias"), "dir")) return;
89+
expect(isSafePath(base, join(base, "alias", "in.txt"))).toBe(true);
90+
});
91+
92+
it("canonicalizes base too: a symlinked base path still admits in-base targets", () => {
93+
// Guards against one-sided realpath: when base is reached via a symlink
94+
// (as on macOS where tmpdir lives under /var -> /private/var), an in-base
95+
// target must still be accepted.
96+
const realBase = tmpDir("safepath-realbase-");
97+
const linkParent = tmpDir("safepath-linkparent-");
98+
const baseLink = join(linkParent, "baseLink");
99+
if (!tryCreateSymlink(realBase, baseLink, "dir")) return;
100+
writeFileSync(join(realBase, "file.txt"), "x");
101+
expect(isSafePath(baseLink, join(baseLink, "file.txt"))).toBe(true);
102+
});
103+
104+
it("fails closed when the base directory does not exist", () => {
105+
const base = join(tmpdir(), "safepath-does-not-exist-zzz", "nope");
106+
expect(isSafePath(base, join(base, "file.txt"))).toBe(false);
107+
});
108+
});

packages/core/src/safePath.ts

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
import { resolve, sep, join, dirname, basename } from "node:path";
2+
import { realpathSync } from "node:fs";
3+
4+
/**
5+
* Reject paths that escape the `base` directory — including via symlinks.
6+
*
7+
* `path.resolve()` collapses `.`/`..` but does NOT dereference symlinks, so a
8+
* plain prefix check (`resolved.startsWith(base + sep)`) can be defeated by a
9+
* symlink that lives *inside* `base` but points outside it (e.g.
10+
* `base/link -> /etc`). A downstream `readFileSync`/`writeFileSync`/`statSync`
11+
* then follows that link to a file outside `base`. To close this we canonicalize
12+
* both sides with `realpathSync` before comparing.
13+
*
14+
* The target may not exist yet (e.g. creating a new file), so we canonicalize the
15+
* deepest *existing* ancestor and re-attach the trailing not-yet-existing
16+
* segments. Segments that don't exist cannot be symlinks at check time, so they
17+
* can't redirect the path outside `base` right now. (A symlink swapped in between
18+
* this check and the subsequent fs call is an inherent TOCTOU race this helper
19+
* does not, and cannot by itself, defend against.)
20+
*
21+
* Lives at the package root rather than under `studio-api/` because callers span
22+
* layers — `studio-api` routes, the `compiler`, the CLI, and the engine — and
23+
* `compiler` sits below `studio-api` in the dependency graph, so it cannot import
24+
* from there without a backwards edge.
25+
*/
26+
export function isSafePath(base: string, resolved: string): boolean {
27+
let baseReal: string;
28+
try {
29+
baseReal = realpathSync(resolve(base));
30+
} catch {
31+
// Base must exist and be resolvable; fail closed if not.
32+
return false;
33+
}
34+
35+
const target = resolve(resolved);
36+
const trailing: string[] = [];
37+
let probe = target;
38+
39+
for (;;) {
40+
let ancestorReal: string;
41+
try {
42+
ancestorReal = realpathSync(probe);
43+
} catch {
44+
const parent = dirname(probe);
45+
if (parent === probe) return false; // walked past the filesystem root
46+
trailing.push(basename(probe));
47+
probe = parent;
48+
continue;
49+
}
50+
51+
// Copy before reverse(): the array is only consumed once today, but a future
52+
// edit that loops would otherwise silently misorder the rebuilt segments.
53+
const targetReal = trailing.length
54+
? join(ancestorReal, ...[...trailing].reverse())
55+
: ancestorReal;
56+
return targetReal === baseReal || targetReal.startsWith(baseReal + sep);
57+
}
58+
}

packages/core/src/studio-api/helpers/safePath.ts

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,10 @@
1-
import { resolve, sep, join } from "node:path";
1+
import { join } from "node:path";
22
import { readdirSync } from "node:fs";
33

4-
/** Reject paths that escape the project directory. */
5-
export function isSafePath(base: string, resolved: string): boolean {
6-
const norm = resolve(base) + sep;
7-
return resolved.startsWith(norm) || resolved === resolve(base);
8-
}
4+
// `isSafePath` lives at the package root so non-studio-api layers (compiler,
5+
// CLI, engine) can share it without a backwards dependency on studio-api.
6+
// Re-exported here for back-compat with existing `../helpers/safePath.js` imports.
7+
export { isSafePath } from "../../safePath.js";
98

109
const IGNORE_DIRS = new Set([".thumbnails", ".hyperframes", "node_modules", ".git"]);
1110

0 commit comments

Comments
 (0)