From 809a97835fe200868223629f850f08a228fc2c3e Mon Sep 17 00:00:00 2001 From: James Date: Sat, 13 Jun 2026 01:24:37 +0000 Subject: [PATCH] refactor(core): route project paths through a single resolveWithinProject chokepoint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Structural follow-up to the symlink-escape fix. The recurring miss (#465 fixed isSafePath but left render.ts; the sweep then turned up play.ts, htmlBundler, ...) is because containment was enforced by convention — "remember to call isSafePath after every resolve()" — which a new call site can silently skip. Add resolveWithinProject(base, relativePath) -> string | null (resolve + containment in one call) and route the studio-api + bundler sites through it, so a caller cannot resolve a project-relative path without the guard: - studio-api routes/files.ts (read, rename, duplicate, upload-dir), preview.ts (sub-comp + static asset), render.ts (composition) — all the resolve()+isSafePath() pairs collapse to a single call. - compiler/htmlBundler.ts: its local safePath helper was exactly this; drop it for the shared one. Left intentionally on isSafePath: files.ts upload (resolves a name against a validated sub-dir but contains against the project root) and htmlBundler's CSS @import (resolves against the CSS file's dir, contains against the root) — these resolve and contain against *different* bases, which the single-base chokepoint doesn't model. Exported from @hyperframes/core and re-exported from studio-api/helpers for back-compat. Adds resolveWithinProject unit tests; all existing studio-api route tests pass unchanged (behavior is identical — same resolve, same containment, same reject paths). Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/core/src/compiler/htmlBundler.ts | 26 +++------- packages/core/src/index.ts | 2 +- packages/core/src/safePath.test.ts | 51 ++++++++++++++++++- packages/core/src/safePath.ts | 14 +++++ .../core/src/studio-api/helpers/safePath.ts | 2 +- packages/core/src/studio-api/routes/files.ts | 22 ++++---- .../core/src/studio-api/routes/preview.ts | 19 ++++--- packages/core/src/studio-api/routes/render.ts | 13 +++-- 8 files changed, 100 insertions(+), 49 deletions(-) diff --git a/packages/core/src/compiler/htmlBundler.ts b/packages/core/src/compiler/htmlBundler.ts index c0f47d76c..580fba595 100644 --- a/packages/core/src/compiler/htmlBundler.ts +++ b/packages/core/src/compiler/htmlBundler.ts @@ -17,17 +17,7 @@ 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. - * 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); - return isSafePath(projectDir, resolved) ? resolved : null; -} +import { isSafePath, resolveWithinProject } from "../safePath.js"; const DEFAULT_RUNTIME_SCRIPT_URL = ""; @@ -233,7 +223,7 @@ function maybeInlineRelativeAssetUrl(urlValue: string, projectDir: string): stri if (!urlValue || !isRelativeUrl(urlValue)) return null; const { basePath, suffix } = splitUrlSuffix(urlValue.trim()); if (!basePath) return null; - const filePath = safePath(projectDir, basePath); + const filePath = resolveWithinProject(projectDir, basePath); if (!filePath || !shouldInlineAsDataUrl(filePath)) return null; const content = safeReadFileBuffer(filePath); if (content == null) return null; @@ -643,7 +633,7 @@ export async function bundleToSingleHtml( for (const el of [...document.querySelectorAll('link[rel="stylesheet"]')]) { const href = el.getAttribute("href"); if (!href || !isRelativeUrl(href)) continue; - const cssPath = safePath(projectDir, href); + const cssPath = resolveWithinProject(projectDir, href); if (!cssPath) continue; const css = safeReadFile(cssPath); if (css == null) continue; @@ -675,7 +665,7 @@ export async function bundleToSingleHtml( for (const el of [...document.querySelectorAll("script[src]")]) { const src = el.getAttribute("src"); if (!src || !isRelativeUrl(src)) continue; - const jsPath = safePath(projectDir, src); + const jsPath = resolveWithinProject(projectDir, src); const js = jsPath ? safeReadFile(jsPath) : null; if (js == null) continue; localJsChunks.push(js); @@ -710,7 +700,7 @@ export async function bundleToSingleHtml( const subCompResult = inlineSubCompositions(document, subCompositionHosts, { resolveHtml: (srcPath: string) => { if (!isRelativeUrl(srcPath)) return null; - const compPath = safePath(projectDir, srcPath); + const compPath = resolveWithinProject(projectDir, srcPath); return compPath ? safeReadFile(compPath) : null; }, parseHtml: parseHTMLContent, @@ -741,7 +731,7 @@ export async function bundleToSingleHtml( if (seenCompScriptSrcs.has(extSrc)) continue; seenCompScriptSrcs.add(extSrc); if (isRelativeUrl(extSrc)) { - const jsPath = safePath(projectDir, extSrc); + const jsPath = resolveWithinProject(projectDir, extSrc); const js = jsPath ? safeReadFile(jsPath) : null; if (js != null) { compScriptChunks.push(js); @@ -806,7 +796,7 @@ export async function bundleToSingleHtml( if (!seenCompScriptSrcs.has(externalSrc)) { seenCompScriptSrcs.add(externalSrc); if (isRelativeUrl(externalSrc)) { - const jsPath = safePath(projectDir, externalSrc); + const jsPath = resolveWithinProject(projectDir, externalSrc); const js = jsPath ? safeReadFile(jsPath) : null; if (js != null) { compScriptChunks.push(js); @@ -861,7 +851,7 @@ export async function bundleToSingleHtml( if (!seenCompScriptSrcs.has(externalSrc)) { seenCompScriptSrcs.add(externalSrc); if (isRelativeUrl(externalSrc)) { - const jsPath = safePath(projectDir, externalSrc); + const jsPath = resolveWithinProject(projectDir, externalSrc); const js = jsPath ? safeReadFile(jsPath) : null; if (js != null) { compScriptChunks.push(js); diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 69a70818a..eb586f0bb 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -163,7 +163,7 @@ export { type MediaVisualStyleProperty, } from "./inline-scripts/parityContract"; export { redactTelemetryString } from "./telemetryRedaction"; -export { isSafePath } from "./safePath"; +export { isSafePath, resolveWithinProject } from "./safePath"; export type { HyperframePickerApi, HyperframePickerBoundingBox, diff --git a/packages/core/src/safePath.test.ts b/packages/core/src/safePath.test.ts index 47455ed79..5bffcc29f 100644 --- a/packages/core/src/safePath.test.ts +++ b/packages/core/src/safePath.test.ts @@ -2,7 +2,7 @@ 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"; +import { isSafePath, resolveWithinProject } from "./safePath.js"; describe("isSafePath", () => { const tmpDirs: string[] = []; @@ -106,3 +106,52 @@ describe("isSafePath", () => { expect(isSafePath(base, join(base, "file.txt"))).toBe(false); }); }); + +describe("resolveWithinProject", () => { + 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; + } + + function tryCreateSymlink(target: string, path: string, type: "dir" | "file"): boolean { + try { + symlinkSync(target, path, type); + return true; + } catch { + return false; + } + } + + it("returns the resolved absolute path for an in-project relative path", () => { + const base = tmpDir("rwp-base-"); + expect(resolveWithinProject(base, "assets/logo.png")).toBe(join(base, "assets", "logo.png")); + }); + + it("returns the resolved path for a not-yet-existing write target", () => { + const base = tmpDir("rwp-base-"); + expect(resolveWithinProject(base, "new/deep/file.txt")).toBe( + join(base, "new", "deep", "file.txt"), + ); + }); + + it("returns null for a `..` traversal that escapes the project", () => { + const base = tmpDir("rwp-base-"); + expect(resolveWithinProject(base, "../../etc/passwd")).toBeNull(); + }); + + it("returns null when the path resolves outside via an in-project symlink", () => { + const base = tmpDir("rwp-base-"); + const external = tmpDir("rwp-external-"); + writeFileSync(join(external, "secret.txt"), "top secret"); + if (!tryCreateSymlink(external, join(base, "link"), "dir")) return; + expect(resolveWithinProject(base, "link/secret.txt")).toBeNull(); + }); +}); diff --git a/packages/core/src/safePath.ts b/packages/core/src/safePath.ts index bf6517224..df38284ad 100644 --- a/packages/core/src/safePath.ts +++ b/packages/core/src/safePath.ts @@ -56,3 +56,17 @@ export function isSafePath(base: string, resolved: string): boolean { return targetReal === baseReal || targetReal.startsWith(baseReal + sep); } } + +/** + * Resolve `relativePath` against `base` and return the absolute path only if it + * stays within `base` (after symlink resolution); otherwise return `null`. + * + * Prefer this over a bare `resolve()` followed by a separate `isSafePath()` + * check: collapsing the two into one call means a caller cannot resolve a + * project-relative path and then forget the containment guard — the gap that + * let the symlink-escape slip past several call sites historically. + */ +export function resolveWithinProject(base: string, relativePath: string): string | null { + const resolved = resolve(base, relativePath); + return isSafePath(base, resolved) ? resolved : null; +} diff --git a/packages/core/src/studio-api/helpers/safePath.ts b/packages/core/src/studio-api/helpers/safePath.ts index 2f9a18e56..de1d90984 100644 --- a/packages/core/src/studio-api/helpers/safePath.ts +++ b/packages/core/src/studio-api/helpers/safePath.ts @@ -4,7 +4,7 @@ import { readdirSync } from "node:fs"; // `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"; +export { isSafePath, resolveWithinProject } from "../../safePath.js"; const IGNORE_DIRS = new Set([".thumbnails", "node_modules", ".git"]); diff --git a/packages/core/src/studio-api/routes/files.ts b/packages/core/src/studio-api/routes/files.ts index 0ce13021a..bf71761ab 100644 --- a/packages/core/src/studio-api/routes/files.ts +++ b/packages/core/src/studio-api/routes/files.ts @@ -16,7 +16,7 @@ import type { StudioApiAdapter } from "../types.js"; import { isAudioFile } from "../helpers/mime.js"; import { generateWaveformCache } from "../helpers/waveform.js"; import { validateUploadedMediaBuffer } from "../helpers/mediaValidation.js"; -import { isSafePath } from "../helpers/safePath.js"; +import { isSafePath, resolveWithinProject } from "../helpers/safePath.js"; import { backupPathForResponse, snapshotBeforeWrite } from "../helpers/backupJournal.js"; import { findUnsafeDomPatchValues, @@ -66,8 +66,8 @@ async function resolveProjectPath( return { error: c.json({ error: "forbidden" }, 403) } as const; } - const absPath = resolve(project.dir, filePath); - if (!isSafePath(project.dir, absPath)) { + const absPath = resolveWithinProject(project.dir, filePath); + if (!absPath) { return { error: c.json({ error: "forbidden" }, 403) } as const; } @@ -1037,8 +1037,8 @@ export function registerFileRoutes(api: Hono, adapter: StudioApiAdapter): void { return c.json({ error: "newPath required" }, 400); } - const newAbs = resolve(res.project.dir, body.newPath); - if (!isSafePath(res.project.dir, newAbs)) { + const newAbs = resolveWithinProject(res.project.dir, body.newPath); + if (!newAbs) { return c.json({ error: "forbidden" }, 403); } if (existsSync(newAbs)) { @@ -1065,14 +1065,14 @@ export function registerFileRoutes(api: Hono, adapter: StudioApiAdapter): void { return c.json({ error: "path required" }, 400); } - const srcAbs = resolve(project.dir, body.path); - if (!isSafePath(project.dir, srcAbs) || !existsSync(srcAbs)) { + const srcAbs = resolveWithinProject(project.dir, body.path); + if (!srcAbs || !existsSync(srcAbs)) { return c.json({ error: "not found" }, 404); } const copyPath = generateCopyPath(project.dir, body.path); - const destAbs = resolve(project.dir, copyPath); - if (!isSafePath(project.dir, destAbs)) { + const destAbs = resolveWithinProject(project.dir, copyPath); + if (!destAbs) { return c.json({ error: "forbidden" }, 403); } @@ -1098,8 +1098,8 @@ export function registerFileRoutes(api: Hono, adapter: StudioApiAdapter): void { // Optional subdirectory within the project (e.g. "assets/audio") const subDir = c.req.query("dir") ?? ""; - const targetDir = subDir ? resolve(project.dir, subDir) : project.dir; - if (!isSafePath(project.dir, targetDir)) return c.json({ error: "forbidden" }, 403); + const targetDir = subDir ? resolveWithinProject(project.dir, subDir) : project.dir; + if (!targetDir) return c.json({ error: "forbidden" }, 403); if (subDir && !existsSync(targetDir)) mkdirSync(targetDir, { recursive: true }); const formData = await c.req.formData(); diff --git a/packages/core/src/studio-api/routes/preview.ts b/packages/core/src/studio-api/routes/preview.ts index 7c13c192a..34846c7ea 100644 --- a/packages/core/src/studio-api/routes/preview.ts +++ b/packages/core/src/studio-api/routes/preview.ts @@ -1,9 +1,9 @@ import type { Hono } from "hono"; import { existsSync, readFileSync, statSync } from "node:fs"; -import { join, resolve } from "node:path"; +import { join } from "node:path"; import { injectScriptsIntoHtml } from "../../compiler/htmlDocument.js"; import type { StudioApiAdapter } from "../types.js"; -import { isSafePath } from "../helpers/safePath.js"; +import { resolveWithinProject } from "../helpers/safePath.js"; import { getMimeType } from "../helpers/mime.js"; import { buildSubCompositionHtml } from "../helpers/subComposition.js"; import { createProjectSignature } from "../helpers/projectSignature.js"; @@ -287,12 +287,8 @@ export function registerPreviewRoutes(api: Hono, adapter: StudioApiAdapter): voi const compPath = decodeURIComponent( c.req.path.replace(`/projects/${project.id}/preview/comp/`, "").split("?")[0] ?? "", ); - const compFile = resolve(project.dir, compPath); - if ( - !isSafePath(project.dir, compFile) || - !existsSync(compFile) || - !statSync(compFile).isFile() - ) { + const compFile = resolveWithinProject(project.dir, compPath); + if (!compFile || !existsSync(compFile) || !statSync(compFile).isFile()) { return c.text("not found", 404); } @@ -321,9 +317,12 @@ export function registerPreviewRoutes(api: Hono, adapter: StudioApiAdapter): voi const subPath = decodeURIComponent( c.req.path.replace(`/projects/${project.id}/preview/`, "").split("?")[0] ?? "", ); - const file = resolve(project.dir, subPath); + const file = resolveWithinProject(project.dir, subPath); + if (!file) { + return c.text("not found", 404); + } const stat = existsSync(file) ? statSync(file) : null; - if (!isSafePath(project.dir, file) || !stat?.isFile()) { + if (!stat?.isFile()) { return c.text("not found", 404); } const contentType = getMimeType(subPath); diff --git a/packages/core/src/studio-api/routes/render.ts b/packages/core/src/studio-api/routes/render.ts index 70c34a3ab..6b6bb058b 100644 --- a/packages/core/src/studio-api/routes/render.ts +++ b/packages/core/src/studio-api/routes/render.ts @@ -1,10 +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 } from "node:path"; +import { join } 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"; +import { resolveWithinProject } from "../helpers/safePath.js"; const VALID_RESOLUTIONS = new Set(VALID_CANVAS_RESOLUTIONS); @@ -80,11 +80,10 @@ export function registerRenderRoutes(api: Hono, adapter: StudioApiAdapter): void : undefined; let composition: string | undefined; if (typeof body.composition === "string" && body.composition.length > 0) { - const resolved = resolve(project.dir, body.composition); - // `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)) { + // `body.composition` is attacker-controlled (from c.req.json()). + // resolveWithinProject dereferences symlinks, so an in-project symlink + // pointing outside the root can't smuggle the render target out. + if (!resolveWithinProject(project.dir, body.composition)) { return c.json({ error: "composition path must be within the project directory" }, 400); } composition = body.composition;