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), + [], + ); +});