From 7a2a6917367e6dd7ce22f4c321c4a852dcf58dfd Mon Sep 17 00:00:00 2001 From: James Russo Date: Sun, 6 Sep 2026 01:18:16 -0400 Subject: [PATCH] fix(catalog): bind asset reads to checked project files (#3735) * fix(catalog): bind asset reads to checked project files * fix(catalog): budget the exact bytes published * fix(catalog): bound directory asset reads --- .github/workflows/windows-render.yml | 5 + scripts/catalog-payload-assets.ts | 197 +++++++---- .../catalog-payload-assets.file-race.test.ts | 317 ++++++++++++++++++ 3 files changed, 449 insertions(+), 70 deletions(-) create mode 100644 scripts/catalog/catalog-payload-assets.file-race.test.ts diff --git a/.github/workflows/windows-render.yml b/.github/workflows/windows-render.yml index 40737c7888..db3100e127 100644 --- a/.github/workflows/windows-render.yml +++ b/.github/workflows/windows-render.yml @@ -484,6 +484,11 @@ jobs: shell: pwsh run: bunx vitest run packages/producer/src/services/hyperframeLint.file-race.test.ts --maxWorkers=2 + - name: Verify catalog source reads on Windows + if: matrix.lane == 'studio-engine-cli' + shell: pwsh + run: bunx vitest run scripts/catalog/catalog-payload-assets.file-race.test.ts --maxWorkers=2 + - name: Run runtime contract test if: matrix.runtime_contract shell: pwsh diff --git a/scripts/catalog-payload-assets.ts b/scripts/catalog-payload-assets.ts index 2550338bc8..de6462b376 100644 --- a/scripts/catalog-payload-assets.ts +++ b/scripts/catalog-payload-assets.ts @@ -8,8 +8,21 @@ */ import { createHash } from "node:crypto"; -import { existsSync, mkdirSync, readdirSync, readFileSync, statSync, writeFileSync } from "node:fs"; -import { extname, join, resolve } from "node:path"; +import { + existsSync, + mkdirSync, + readdirSync, + readFileSync, + readSync, + statSync, + writeFileSync, + realpathSync, + openSync, + fstatSync, + closeSync, + constants, +} from "node:fs"; +import { extname, join, resolve, relative, isAbsolute, sep } from "node:path"; export const MIME_TYPES: Record = { ".png": "image/png", @@ -127,6 +140,65 @@ export interface AssetResult { unresolved: string[]; } +function isWithin(root: string, filePath: string): boolean { + const rel = relative(root, filePath); + return rel !== ".." && !rel.startsWith(`..${sep}`) && !isAbsolute(rel); +} + +function isMissingFile(error: unknown): boolean { + return ( + error instanceof Error && + "code" in error && + (error.code === "ENOENT" || error.code === "ENOTDIR") + ); +} + +/** Keep oversized or concurrently growing directory assets within the read budget. */ +function readWithinBudget(fd: number, maxBytes: number): Buffer { + const chunks: Buffer[] = []; + let remaining = maxBytes + 1; + while (remaining > 0) { + const chunk = Buffer.alloc(Math.min(64 * 1024, remaining)); + const count = readSync(fd, chunk, 0, chunk.length, null); + if (count === 0) break; + chunks.push(chunk.subarray(0, count)); + remaining -= count; + } + return Buffer.concat(chunks); +} + +/** Read one checked file from the prepared project, including internal links. */ +function readProjectFile( + root: string, + filePath: string, + maxBytes?: number, +): Buffer | null { + if (!isWithin(root, filePath)) return null; + let source: string; + try { + source = realpathSync(filePath); + if (!isWithin(realpathSync(root), source)) return null; + } catch (error) { + if (isMissingFile(error)) return null; + throw error; + } + let fd: number; + try { + // Nonblocking mode lets fstat reject named pipes without waiting for a writer. + fd = openSync(source, constants.O_RDONLY | constants.O_NONBLOCK); + } catch (error) { + if (isMissingFile(error)) return null; + if (!statSync(source, { throwIfNoEntry: false })?.isFile()) return null; + throw error; + } + try { + if (!fstatSync(fd).isFile()) return null; + return maxBytes === undefined ? readFileSync(fd) : readWithinBudget(fd, maxBytes); + } finally { + closeSync(fd); + } +} + export interface AssetTarget { /** Directory shared by every item, so one font is stored once. */ dir: string; @@ -164,8 +236,8 @@ export function processAssets(html: string, projectDir: string, target: AssetTar // A composition reaching outside its own directory would pull an arbitrary // file from the build machine into a published payload. - const contained = source === root || source.startsWith(`${root}/`); - if (!contained || !existsSync(source) || !statSync(source).isFile()) { + const bytes = readProjectFile(root, source); + if (bytes === null) { // A name a script passed around that turned out not to be a file is just // a string; only a reference we are sure about counts as a broken one. if (strict) unresolved.push(ref); @@ -179,7 +251,6 @@ export function processAssets(html: string, projectDir: string, target: AssetTar continue; } - const bytes = readFileSync(source); if (HOSTED_EXTENSIONS.has(ext)) { const name = `${createHash("sha256").update(bytes).digest("hex").slice(0, 16)}${ext}`; const dest = join(target.dir, name); @@ -247,21 +318,30 @@ export function externalizeDataUris( return { html: out, externalized }; } -/** Copy a directory's publishable files into `destDir`, flattening one level. */ -function walkInto(from: string, rel: string, destDir: string, onCopy: () => void): void { +/** Enumerate publishable paths without using pathname sizes to authorize reads. */ +function* hostedPaths(from: string, rel = ""): Generator { for (const entry of readdirSync(from, { withFileTypes: true })) { if (entry.isSymbolicLink()) continue; const childRel = rel ? `${rel}/${entry.name}` : entry.name; - const childFrom = join(from, entry.name); if (entry.isDirectory()) { - walkInto(childFrom, childRel, destDir, onCopy); - continue; + yield* hostedPaths(join(from, entry.name), childRel); + } else if (HOSTED_EXTENSIONS.has(extname(entry.name).toLowerCase())) { + yield childRel; } - if (!HOSTED_EXTENSIONS.has(extname(entry.name).toLowerCase())) continue; - const to = join(destDir, childRel); - mkdirSync(join(to, ".."), { recursive: true }); - writeFileSync(to, readFileSync(childFrom)); - onCopy(); + } +} + +/** Internal directory aliases share the buffers already collected at their real paths. */ +function downloadMirrorPrefix(projectDir: string): string | null { + try { + const root = realpathSync(projectDir); + const downloads = realpathSync(join(projectDir, "_downloads")); + if (!isWithin(root, downloads) || !statSync(downloads).isDirectory()) return null; + const rel = relative(root, downloads).split(sep).join("/"); + return rel ? `${rel}/` : ""; + } catch (error) { + if (isMissingFile(error)) return null; + throw error; } } @@ -278,22 +358,6 @@ function walkInto(from: string, rel: string, destDir: string, onCopy: () => void * Only publishable types are copied; a composition needing something the host * drops still falls back to inlining, which is handled by the caller. */ -/** Publishable bytes an item would add, counted before anything is written. */ -function directoryBytes(dir: string): number { - let total = 0; - for (const entry of readdirSync(dir, { withFileTypes: true })) { - if (entry.isSymbolicLink()) continue; - const child = join(dir, entry.name); - if (entry.isDirectory()) { - total += directoryBytes(child); - continue; - } - if (!HOSTED_EXTENSIONS.has(extname(entry.name).toLowerCase())) continue; - total += statSync(child).size; - } - return total; -} - /** * What an item may add by publishing its own directory. * @@ -304,45 +368,37 @@ function directoryBytes(dir: string): number { export const MAX_HOSTED_DIRECTORY_BYTES = 2_000_000; export function hostItemDirectory(projectDir: string, destDir: string, urlBase: string): string { - if (directoryBytes(projectDir) > MAX_HOSTED_DIRECTORY_BYTES) return ""; - let copied = 0; - - const walk = (from: string, rel: string): void => { - for (const entry of readdirSync(from, { withFileTypes: true })) { - // Nothing here should ever leave the prepared copy, and a symlink is the - // one entry that could point back out of it. - if (entry.isSymbolicLink()) continue; - const childRel = rel ? `${rel}/${entry.name}` : entry.name; - const childFrom = join(from, entry.name); - if (entry.isDirectory()) { - walk(childFrom, childRel); - continue; - } - if (!HOSTED_EXTENSIONS.has(extname(entry.name).toLowerCase())) continue; - const to = join(destDir, childRel); - mkdirSync(join(to, ".."), { recursive: true }); - writeFileSync(to, readFileSync(childFrom)); - copied += 1; - } - }; - - // Both layouts are published. The prepared copy holds each asset twice, once - // as the registry stores it and once at its install path, and which one a - // composition asks for differs per item: the texture masks use the registry - // spelling, the caption textures the install one. Guessing wrong is a 404 at - // run time, so the budget below is what keeps the cost in check instead. - walk(projectDir, ""); - - // The compiler pulls remote media into `_downloads/` but rewrites references - // as if the document sat inside it, so `_remote_media/x.woff2` has to resolve - // from the item root too. Mirroring rather than moving keeps both spellings - // working, and the files are content-identical either way. - const downloads = join(projectDir, "_downloads"); - if (existsSync(downloads) && statSync(downloads).isDirectory()) { - walkInto(downloads, "", destDir, () => (copied += 1)); + const mirrorPrefix = downloadMirrorPrefix(projectDir); + const files = new Map>(); + let total = 0; + for (const path of hostedPaths(projectDir)) { + const bytes = readProjectFile( + projectDir, + join(projectDir, path), + MAX_HOSTED_DIRECTORY_BYTES - total, + ); + if (bytes === null) continue; + total += bytes.length; + if (total > MAX_HOSTED_DIRECTORY_BYTES) return ""; + files.set(path, bytes); } - return copied > 0 ? urlBase : ""; + // Charge each source once, as before. Publish only after the entire item fits, + // and reuse the collected bytes for both registry and install-path layouts. + const publish = (path: string, bytes: Buffer): void => { + const to = join(destDir, path); + mkdirSync(join(to, ".."), { recursive: true }); + writeFileSync(to, bytes); + }; + for (const [path, bytes] of files) publish(path, bytes); + + // Compiler references omit `_downloads/`. Preserve both spellings and the + // existing mirror-wins collision order without reopening any source file. + for (const [path, bytes] of files) { + if (mirrorPrefix !== null && path.startsWith(mirrorPrefix)) + publish(path.slice(mirrorPrefix.length), bytes); + } + return files.size > 0 ? urlBase : ""; } /** @@ -376,8 +432,9 @@ export function inlineMountedComposition(html: string, projectDir: string): stri (whole, quote: string, ref: string) => { if (/^(https?:|data:)/i.test(ref)) return whole; const source = resolve(projectDir, ref.replace(/^\.\//, "").split(/[?#]/)[0] ?? ref); - if (!source.startsWith(resolve(projectDir)) || !existsSync(source)) return whole; - const encoded = readFileSync(source).toString("base64"); + const bytes = readProjectFile(resolve(projectDir), source); + if (bytes === null) return whole; + const encoded = bytes.toString("base64"); return `data-composition-src=${quote}data:text/html;base64,${encoded}${quote}`; }, ); diff --git a/scripts/catalog/catalog-payload-assets.file-race.test.ts b/scripts/catalog/catalog-payload-assets.file-race.test.ts new file mode 100644 index 0000000000..cd5cd4b0e2 --- /dev/null +++ b/scripts/catalog/catalog-payload-assets.file-race.test.ts @@ -0,0 +1,317 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { join, dirname } from "node:path"; +import { tmpdir } from "node:os"; +import { execFileSync } from "node:child_process"; +import { createHash } from "node:crypto"; +import { + processAssets, + inlineMountedComposition, + hostItemDirectory, + MAX_HOSTED_DIRECTORY_BYTES, +} from "../catalog-payload-assets.ts"; + +const hooks = vi.hoisted(() => { + const state: { + target: string; + swap?: () => void; + beforeOpen?: () => void; + beforeWrite?: () => void; + afterStat?: () => void; + forbidUnbounded: boolean; + bytesRead: number; + maxReadSize: number; + readCount: number; + failStat: boolean; + failRead: boolean; + fds: Map; + } = { + target: "", + forbidUnbounded: false, + bytesRead: 0, + maxReadSize: Infinity, + readCount: 1, + failStat: false, + failRead: false, + fds: new Map(), + }; + return state; +}); +vi.mock("node:fs", async (importOriginal) => { + const fs = await importOriginal(); + function beforeRead(file: number | string): void { + const path = typeof file === "number" ? hooks.fds.get(file) : file; + if (!path || !hooks.target) return; + if (fs.realpathSync(path) !== fs.realpathSync(hooks.target)) return; + if (--hooks.readCount !== 0) return; + const swap = hooks.swap; + hooks.swap = undefined; + swap?.(); + if (hooks.failRead) throw new Error("read failure"); + } + return { + ...fs, + openSync: (...args: Parameters) => { + const beforeOpen = hooks.beforeOpen; + hooks.beforeOpen = undefined; + beforeOpen?.(); + const fd = fs.openSync(...args); + hooks.fds.set(fd, String(args[0])); + return fd; + }, + fstatSync: (...args: Parameters) => { + if (hooks.failStat) throw new Error("stat failure"); + const stat = fs.fstatSync(...args); + const afterStat = hooks.afterStat; + hooks.afterStat = undefined; + afterStat?.(); + return stat; + }, + readFileSync: (...args: Parameters) => { + if (hooks.forbidUnbounded) throw new Error("unbounded directory read"); + beforeRead(typeof args[0] === "number" ? args[0] : String(args[0])); + return fs.readFileSync(...args); + }, + readSync: ( + fd: number, + buffer: Buffer, + offset: number, + length: number, + position: number | null, + ) => { + beforeRead(fd); + const count = fs.readSync(fd, buffer, offset, Math.min(length, hooks.maxReadSize), position); + hooks.bytesRead += count; + return count; + }, + writeFileSync: (...args: Parameters) => { + const beforeWrite = hooks.beforeWrite; + hooks.beforeWrite = undefined; + beforeWrite?.(); + return fs.writeFileSync(...args); + }, + closeSync: (fd: number) => { + fs.closeSync(fd); + hooks.fds.delete(fd); + }, + }; +}); +const fs = await vi.importActual("node:fs"); +let root: string; +let project: string; +let out: { dir: string; urlBase: string }; +beforeEach(() => { + root = fs.mkdtempSync(join(tmpdir(), "hf-catalog-security-")); + project = join(root, "project"); + fs.mkdirSync(project); + out = { dir: join(root, "output"), urlBase: "/assets" }; + Object.assign(hooks, { + target: "", + swap: undefined, + beforeOpen: undefined, + beforeWrite: undefined, + afterStat: undefined, + forbidUnbounded: false, + bytesRead: 0, + maxReadSize: Infinity, + readCount: 1, + failStat: false, + failRead: false, + }); +}); +afterEach(() => { + const leaked = [...hooks.fds.keys()]; + for (const fd of leaked) fs.closeSync(fd); + hooks.fds.clear(); + fs.rmSync(root, { recursive: true, force: true }); + expect(leaked).toEqual([]); +}); +function file(name: string, bytes = "checked") { + const path = join(project, name); + fs.mkdirSync(dirname(path), { recursive: true }); + fs.writeFileSync(path, bytes); + return path; +} +function arm(path: string) { + hooks.target = path; + hooks.swap = () => { + fs.renameSync(path, join(root, "original")); + fs.writeFileSync(path, "unchecked"); + }; +} + +describe("catalog source reads", () => { + it.each(["png", "glb"])("uses checked .%s bytes for hosted and inlined references", (ext) => { + arm(file(`asset.${ext}`)); + const result = processAssets(``, project, out); + if (ext === "png") { + const digest = createHash("sha256").update("checked").digest("hex").slice(0, 16); + expect(result.html).toBe(``); + expect(fs.readFileSync(join(out.dir, `${digest}.png`), "utf8")).toBe("checked"); + } else expect(result.html).toContain(Buffer.from("checked").toString("base64")); + expect(hooks.swap).toBeUndefined(); + }); + it("uses checked bytes for mounted composition HTML", () => { + arm(file("clip.html")); + expect(inlineMountedComposition('
', project)).toContain( + Buffer.from("checked").toString("base64"), + ); + expect(hooks.swap).toBeUndefined(); + }); + it.each([false, true])( + "uses checked bytes in directory copies (download mirror: %s)", + (mirror) => { + arm(file(mirror ? "_downloads/font.woff2" : "font.woff2")); + + expect(hostItemDirectory(project, out.dir, "/item/")).toBe("/item/"); + expect(fs.readFileSync(join(out.dir, "font.woff2"), "utf8")).toBe("checked"); + expect(hooks.swap).toBeUndefined(); + }, + ); + it("rejects a replacement that grows beyond the directory budget before open", () => { + const path = file("asset.png", "x"); + hooks.beforeOpen = () => { + fs.renameSync(path, join(root, "original")); + fs.writeFileSync(path, Buffer.alloc(MAX_HOSTED_DIRECTORY_BYTES + 1)); + }; + expect(hostItemDirectory(project, out.dir, "/item/")).toBe(""); + expect(hooks.beforeOpen).toBeUndefined(); + expect(fs.existsSync(out.dir)).toBe(false); + }); + it("bounds a sparse oversized source to the remaining directory budget plus one byte", () => { + file("a.png", "already counted"); + const path = file("large.mp4", ""); + fs.truncateSync(path, 32 * 1024 * 1024); + hooks.forbidUnbounded = true; + expect(hostItemDirectory(project, out.dir, "/item/")).toBe(""); + expect(hooks.bytesRead).toBe(MAX_HOSTED_DIRECTORY_BYTES + 1); + expect(fs.existsSync(out.dir)).toBe(false); + }); + it("bounds a file that grows after its descriptor stat", () => { + const path = file("asset.webm", "x"); + hooks.afterStat = () => fs.truncateSync(path, MAX_HOSTED_DIRECTORY_BYTES * 2); + hooks.forbidUnbounded = true; + expect(hostItemDirectory(project, out.dir, "/item/")).toBe(""); + expect(hooks.afterStat).toBeUndefined(); + expect(hooks.bytesRead).toBe(MAX_HOSTED_DIRECTORY_BYTES + 1); + expect(fs.existsSync(out.dir)).toBe(false); + }); + it("handles short descriptor reads without truncating a directory asset", () => { + file("font.woff2"); + hooks.forbidUnbounded = true; + hooks.maxReadSize = 2; + expect(hostItemDirectory(project, out.dir, "/item/")).toBe("/item/"); + expect(fs.readFileSync(join(out.dir, "font.woff2"), "utf8")).toBe("checked"); + expect(hooks.bytesRead).toBe(7); + }); + it("closes a directory source descriptor when its bounded read fails", () => { + hooks.target = file("asset.png"); + hooks.failRead = true; + expect(() => hostItemDirectory(project, out.dir, "/item/")).toThrow("read failure"); + expect(hooks.fds.size).toBe(0); + expect(fs.existsSync(out.dir)).toBe(false); + }); + it("leaves existing output untouched when collected files exceed the budget", () => { + file("a.png", "a"); + file("b.png", "b".repeat(MAX_HOSTED_DIRECTORY_BYTES)); + fs.mkdirSync(out.dir); + fs.writeFileSync(join(out.dir, "a.png"), "existing"); + expect(hostItemDirectory(project, out.dir, "/item/")).toBe(""); + expect(fs.readdirSync(out.dir)).toEqual(["a.png"]); + expect(fs.readFileSync(join(out.dir, "a.png"), "utf8")).toBe("existing"); + }); + it("reuses budgeted bytes for download mirrors after sources change during publication", () => { + const path = file("_downloads/font.woff2", "x".repeat(MAX_HOSTED_DIRECTORY_BYTES)); + hooks.beforeWrite = () => fs.writeFileSync(path, "unchecked"); + expect(hostItemDirectory(project, out.dir, "/item/")).toBe("/item/"); + expect(hooks.beforeWrite).toBeUndefined(); + const original = Buffer.alloc(MAX_HOSTED_DIRECTORY_BYTES, "x"); + expect(fs.readFileSync(join(out.dir, "_downloads/font.woff2")).equals(original)).toBe(true); + expect(fs.readFileSync(join(out.dir, "font.woff2")).equals(original)).toBe(true); + }); + it("preserves an internal downloads directory alias using the same collected bytes", () => { + arm(file("media/font.woff2")); + fs.symlinkSync( + join(project, "media"), + join(project, "_downloads"), + process.platform === "win32" ? "junction" : "dir", + ); + expect(hostItemDirectory(project, out.dir, "/item/")).toBe("/item/"); + expect(fs.readFileSync(join(out.dir, "media/font.woff2"), "utf8")).toBe("checked"); + expect(fs.readFileSync(join(out.dir, "font.woff2"), "utf8")).toBe("checked"); + expect(fs.existsSync(join(out.dir, "_downloads"))).toBe(false); + expect(hooks.swap).toBeUndefined(); + }); + it("preserves download mirror precedence over a colliding root asset", () => { + file("font.woff2", "root"); + file("_downloads/font.woff2", "download"); + expect(hostItemDirectory(project, out.dir, "/item/")).toBe("/item/"); + expect(fs.readFileSync(join(out.dir, "font.woff2"), "utf8")).toBe("download"); + }); + it.each(["png", "glb", "html"])("does not publish an external .%s symlink target", (ext) => { + const outside = join(root, `outside.${ext}`); + fs.writeFileSync(outside, "secret"); + fs.symlinkSync(outside, join(project, `linked.${ext}`), "file"); + if (ext === "html") { + const html = '
'; + expect(inlineMountedComposition(html, project)).toBe(html); + } else { + const result = processAssets(``, project, out); + expect(result.unresolved).toEqual([`linked.${ext}`]); + expect(result.hosted + result.inlined).toBe(0); + } + expect(fs.existsSync(out.dir)).toBe(false); + }); + it("does not mirror a downloads directory linked outside the project", () => { + const outside = join(root, "outside"); + fs.mkdirSync(outside); + fs.writeFileSync(join(outside, "secret.png"), "secret"); + fs.symlinkSync( + outside, + join(project, "_downloads"), + process.platform === "win32" ? "junction" : "dir", + ); + expect(hostItemDirectory(project, out.dir, "/item/")).toBe(""); + expect(fs.existsSync(out.dir)).toBe(false); + }); + it("keeps internal file links and a symlinked project root working", () => { + const target = file("asset.glb"); + fs.symlinkSync(target, join(project, "linked.glb"), "file"); + const alias = join(root, "alias"); + fs.symlinkSync(project, alias, process.platform === "win32" ? "junction" : "dir"); + const result = processAssets('', alias, out); + expect(result.inlined).toBe(1); + expect(result.unresolved).toEqual([]); + }); + it("rejects sibling-prefix mounted composition escapes", () => { + const sibling = join(root, "project-other"); + fs.mkdirSync(sibling); + fs.writeFileSync(join(sibling, "clip.html"), "secret"); + const html = '
'; + expect(inlineMountedComposition(html, project)).toBe(html); + }); + it("preserves missing/probable reference and empty-file behavior", () => { + fs.mkdirSync(join(project, "dir.png")); + file("empty.glb", ""); + const result = processAssets( + '', + project, + out, + ); + expect(result.unresolved).toEqual(["dir.png", "missing.png"]); + expect(result.inlined).toBe(1); + expect(result.html).toContain('src="data:model/gltf-binary;base64,"'); + }); + it.skipIf(process.platform === "win32")("rejects a FIFO without waiting for a writer", () => { + execFileSync("mkfifo", [join(project, "pipe.png")]); + expect(processAssets('', project, out).unresolved).toEqual(["pipe.png"]); + }); + it.each(["stat", "read"])("closes the source descriptor after %s failure", (failure) => { + hooks.target = file("asset.png"); + hooks.failStat = failure === "stat"; + hooks.failRead = failure === "read"; + expect(() => processAssets('', project, out)).toThrow( + `${failure} failure`, + ); + expect(hooks.fds.size).toBe(0); + }); +});