From bd7ea5d5ce7cab921c0b6e9df5ed92c47b01bb8a Mon Sep 17 00:00:00 2001 From: ukimsanov Date: Mon, 3 Aug 2026 21:02:26 -0700 Subject: [PATCH] fix(scripts): contain registry manifest paths in the preview renderer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Miguel's P1 on #2975, and it is real. `catalog-previews.yml` triggers on `pull_request` for anything under `registry/blocks/**` or `registry/components/**`, so `registry-item.json` arrives from the pull request and is untrusted. `mirrorRegistryTargets` joined `files[].path` and `files[].target` under the temp project and called `cpSync` on the result, and `join()` walks out of its first argument. A `path` of `../../../../etc/passwd` reads an arbitrary runner file into the project — which the job then uploads as an artifact — and a `target` of the same shape writes an arbitrary runner path. Both sides are now resolved and rejected when `relative(projectDir, candidate)` is absolute or starts with `..`. Traversal that lands back inside the project still works, so `nested/../demo.html` is unaffected. Containment lives in `scripts/registry-target-paths.mjs` rather than inline, because the traversal cases have to be testable and importing `generate-catalog-previews.ts` drags in the producer. `existsSync` is injected so the decision cannot depend on whether the target happens to exist on the runner. Eight tests, covering traversal on each field separately, absolute paths on each field, the sibling directory that shares the project's prefix, and traversal that returns inside. Verified end to end on a real tree, not only in unit tests: a manifest asking to read `../secret.txt` and write `../pwned.txt` produces neither file, while the legitimate entry still copies. I introduced the wrapper when I extracted this block for a complexity finding earlier in the stack, and did not look at what it was joining. --- package.json | 2 +- scripts/generate-catalog-previews.ts | 13 ++--- scripts/registry-target-paths.mjs | 37 +++++++++++++ scripts/registry-target-paths.test.mjs | 76 ++++++++++++++++++++++++++ 4 files changed, 120 insertions(+), 8 deletions(-) create mode 100644 scripts/registry-target-paths.mjs create mode 100644 scripts/registry-target-paths.test.mjs diff --git a/package.json b/package.json index 5fdf919ec..870deec88 100644 --- a/package.json +++ b/package.json @@ -48,7 +48,7 @@ "player:perf": "bun run --filter @hyperframes/player perf", "format:check": "oxfmt --check .", "knip": "knip", - "test:scripts": "node --import tsx --test scripts/check-tracked-artifacts.test.mjs scripts/check-docs-snippet-motion.test.mjs scripts/check-workspace-contracts.test.mjs scripts/check-package-cycles.test.mjs scripts/check-cli-process-ownership.test.mjs scripts/package-subpaths.test.mjs scripts/validate-release-channel.test.mjs scripts/publish-workflow.test.mjs scripts/draft-changelog.test.ts scripts/set-version.test.ts scripts/release-prepare.test.ts scripts/cli-options.test.ts scripts/changelog-weekly.test.ts scripts/claude-plugin-compression.test.ts scripts/studio-runtime-smoke.test.mjs scripts/verify-packed-manifests.test.mjs scripts/lint-skills.test.mjs packages/gcp-cloud-run/check-dockerfile-workspaces.test.mjs", + "test:scripts": "node --import tsx --test scripts/check-tracked-artifacts.test.mjs scripts/check-docs-snippet-motion.test.mjs scripts/registry-target-paths.test.mjs scripts/check-workspace-contracts.test.mjs scripts/check-package-cycles.test.mjs scripts/check-cli-process-ownership.test.mjs scripts/package-subpaths.test.mjs scripts/validate-release-channel.test.mjs scripts/publish-workflow.test.mjs scripts/draft-changelog.test.ts scripts/set-version.test.ts scripts/release-prepare.test.ts scripts/cli-options.test.ts scripts/changelog-weekly.test.ts scripts/claude-plugin-compression.test.ts scripts/studio-runtime-smoke.test.mjs scripts/verify-packed-manifests.test.mjs scripts/lint-skills.test.mjs packages/gcp-cloud-run/check-dockerfile-workspaces.test.mjs", "test:skills": "node --test 'skills/**/*.test.mjs'", "generate:previews": "tsx scripts/generate-template-previews.ts", "generate:catalog-previews": "tsx scripts/generate-catalog-previews.ts", diff --git a/scripts/generate-catalog-previews.ts b/scripts/generate-catalog-previews.ts index 91d2be843..df262f67d 100644 --- a/scripts/generate-catalog-previews.ts +++ b/scripts/generate-catalog-previews.ts @@ -43,6 +43,7 @@ import { executeRenderJob, } from "../packages/producer/src/index.js"; import { compileForRender } from "../packages/producer/src/services/htmlCompiler.js"; +import { resolveContainedCopies } from "./registry-target-paths.mjs"; const scriptDir = dirname(fileURLToPath(import.meta.url)); const repoRoot = resolve(scriptDir, ".."); @@ -139,13 +140,11 @@ function mirrorRegistryTargets(projectDir: string): void { files?: { path?: string; target?: string }[]; }; - const copies = (manifest.files ?? []) - .map((file) => [file.path, file.target] as const) - .filter((pair): pair is readonly [string, string] => Boolean(pair[0] && pair[1])) - .map(([path, target]) => [join(projectDir, path), join(projectDir, target)] as const) - .filter(([from, to]) => from !== to && existsSync(from)); - - for (const [from, to] of copies) { + // registry-item.json is untrusted: catalog-previews.yml runs on pull_request + // for any registry change, so the manifest arrives from the PR. Containment + // lives in its own module so the traversal cases stay testable without this + // file's producer imports. + for (const [from, to] of resolveContainedCopies(projectDir, manifest.files, existsSync)) { mkdirSync(dirname(to), { recursive: true }); cpSync(from, to); } diff --git a/scripts/registry-target-paths.mjs b/scripts/registry-target-paths.mjs new file mode 100644 index 000000000..470bf9655 --- /dev/null +++ b/scripts/registry-target-paths.mjs @@ -0,0 +1,37 @@ +/** + * Containment for registry manifest paths. + * + * `registry-item.json` is untrusted input. `catalog-previews.yml` runs on + * `pull_request` for any change under `registry/blocks/**` or + * `registry/components/**`, so a contributor's own manifest reaches the preview + * renderer, and the job then uploads `docs/images/catalog/` as an artifact. + * + * `join()` happily walks out of its first argument, so a `files[].path` of + * `../../../../etc/passwd` reads an arbitrary runner file into the project, and + * a `files[].target` of the same shape writes an arbitrary runner path. Both + * sides have to be resolved and checked, not just the one that looks like input. + */ + +import { isAbsolute, relative, resolve } from "node:path"; + +/** True when `candidate` resolves to `root` itself or something beneath it. */ +export function isContainedIn(root, candidate) { + const step = relative(resolve(root), resolve(root, candidate)); + return step === "" || (!step.startsWith("..") && !isAbsolute(step)); +} + +/** + * The `[from, to]` pairs safe to copy, dropping any that escape `projectDir`. + * + * `exists` is injected so the containment rule can be tested without a fixture + * tree — the traversal decision must not depend on whether the target happens + * to be present on the runner. + */ +export function resolveContainedCopies(projectDir, files, exists) { + const root = resolve(projectDir); + return (files ?? []) + .filter((file) => file?.path && file?.target) + .filter((file) => isContainedIn(root, file.path) && isContainedIn(root, file.target)) + .map((file) => [resolve(root, file.path), resolve(root, file.target)]) + .filter(([from, to]) => from !== to && exists(from)); +} diff --git a/scripts/registry-target-paths.test.mjs b/scripts/registry-target-paths.test.mjs new file mode 100644 index 000000000..02a332f84 --- /dev/null +++ b/scripts/registry-target-paths.test.mjs @@ -0,0 +1,76 @@ +import { strict as assert } from "node:assert"; +import { test } from "node:test"; + +import { isContainedIn, resolveContainedCopies } from "./registry-target-paths.mjs"; + +const ROOT = "/tmp/hf-catalog-demo"; +const always = () => true; + +test("ordinary manifest entries are copied", () => { + const copies = resolveContainedCopies( + ROOT, + [{ path: "demo.html", target: "compositions/demo.html" }], + always, + ); + assert.deepEqual(copies, [[`${ROOT}/demo.html`, `${ROOT}/compositions/demo.html`]]); +}); + +// Both fields are attacker-controlled: catalog-previews.yml runs on +// pull_request for any registry change, so the manifest arrives from the PR. + +test("a traversing path cannot read outside the project", () => { + const copies = resolveContainedCopies( + ROOT, + [{ path: "../../../../etc/passwd", target: "leak.txt" }], + always, + ); + assert.deepEqual(copies, []); +}); + +test("a traversing target cannot write outside the project", () => { + const copies = resolveContainedCopies( + ROOT, + [{ path: "demo.html", target: "../../../../home/runner/.bashrc" }], + always, + ); + assert.deepEqual(copies, []); +}); + +test("an absolute path or target is refused on either side", () => { + assert.deepEqual( + resolveContainedCopies(ROOT, [{ path: "/etc/passwd", target: "leak.txt" }], always), + [], + ); + assert.deepEqual( + resolveContainedCopies(ROOT, [{ path: "demo.html", target: "/etc/cron.d/x" }], always), + [], + ); +}); + +test("traversal that returns inside the project is allowed", () => { + const copies = resolveContainedCopies( + ROOT, + [{ path: "nested/../demo.html", target: "out/demo.html" }], + always, + ); + assert.deepEqual(copies, [[`${ROOT}/demo.html`, `${ROOT}/out/demo.html`]]); +}); + +test("a sibling directory sharing the project's prefix is still outside", () => { + assert.equal(isContainedIn(ROOT, "../hf-catalog-demo-evil/x"), false); +}); + +test("containment does not depend on the file existing", () => { + assert.equal(isContainedIn(ROOT, "../../etc/passwd"), false); + assert.deepEqual( + resolveContainedCopies(ROOT, [{ path: "../../etc/passwd", target: "x" }], () => true), + [], + ); +}); + +test("incomplete entries are skipped rather than resolved", () => { + assert.deepEqual( + resolveContainedCopies(ROOT, [{ path: "demo.html" }, { target: "x" }, {}], always), + [], + ); +});