diff --git a/scripts/registry-target-paths.mjs b/scripts/registry-target-paths.mjs index 470bf9655..fedbf7802 100644 --- a/scripts/registry-target-paths.mjs +++ b/scripts/registry-target-paths.mjs @@ -6,29 +6,84 @@ * `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. + * Two escapes, and the second is why this is not a one-line check: + * + * Lexical — `join()` walks out of its first argument, so `files[].path` of + * `../../../../etc/passwd` reads an arbitrary runner file into the project and + * `files[].target` of the same shape writes an arbitrary runner path. + * + * Symbolic — `resolve()` and `relative()` are pure string operations and do + * not follow links. The registry item is copied in recursively with symlinks + * preserved, so a PR shipping `escape -> /tmp/outside` and declaring + * `target: "escape/pwned.txt"` passes any lexical test; `mkdirSync` then + * follows the link and `cpSync` writes outside the project. + * + * So containment is filesystem-aware: no existing component of a candidate may + * be a symlink, and the candidate's real location — resolved through its + * deepest existing ancestor — must sit under the project's own real path. + * Both fields are checked, not just the one that looks like input. */ -import { isAbsolute, relative, resolve } from "node:path"; +import { existsSync, lstatSync, realpathSync } from "node:fs"; +import { dirname, isAbsolute, relative, resolve, sep } from "node:path"; -/** True when `candidate` resolves to `root` itself or something beneath it. */ +/** True when `candidate` is `root` itself or lexically beneath it. */ +function isBeneath(root, candidate) { + const step = relative(root, candidate); + return step === "" || (!step.startsWith(`..${sep}`) && step !== ".." && !isAbsolute(step)); +} + +/** Every directory from `root` down to `candidate`, inclusive. */ +function componentsUnder(root, candidate) { + const chain = []; + for (let current = candidate; current !== root && isBeneath(root, current); ) { + chain.push(current); + const parent = dirname(current); + if (parent === current) break; + current = parent; + } + return chain; +} + +/** `candidate` with symlinks resolved as far as the filesystem knows it. */ +function realLocation(candidate) { + const existing = componentsUnder("", candidate).find((part) => existsSync(part)); + if (!existing) return candidate; + return resolve(realpathSync(existing), relative(existing, candidate)); +} + +/** + * True when `candidate` really lands inside `root`. + * + * Lexical containment first, then a refusal of any existing component that is a + * symlink, then a real-path check — a link is rejected outright rather than + * followed, so a link pointing back inside the project is still refused. That + * is deliberate: nothing in the registry needs one, and allowing it would mean + * trusting the link target not to change between the check and the copy. + */ export function isContainedIn(root, candidate) { - const step = relative(resolve(root), resolve(root, candidate)); - return step === "" || (!step.startsWith("..") && !isAbsolute(step)); + const realRoot = realpathSync(resolve(root)); + const absolute = resolve(realRoot, candidate); + if (!isBeneath(realRoot, absolute)) return false; + if (componentsUnder(realRoot, absolute).some(isSymlink)) return false; + return isBeneath(realRoot, realLocation(absolute)); +} + +function isSymlink(target) { + return ( + existsSync(dirname(target)) && lstatSync(target, { throwIfNoEntry: false })?.isSymbolicLink() + ); } /** * 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. + * `exists` is injected so a caller can test the containment rule 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); + const root = realpathSync(resolve(projectDir)); return (files ?? []) .filter((file) => file?.path && file?.target) .filter((file) => isContainedIn(root, file.path) && isContainedIn(root, file.target)) diff --git a/scripts/registry-target-paths.test.mjs b/scripts/registry-target-paths.test.mjs index 02a332f84..4ed3d6c82 100644 --- a/scripts/registry-target-paths.test.mjs +++ b/scripts/registry-target-paths.test.mjs @@ -1,76 +1,99 @@ import { strict as assert } from "node:assert"; -import { test } from "node:test"; +import { + existsSync, + mkdirSync, + mkdtempSync, + realpathSync, + rmSync, + symlinkSync, + writeFileSync, +} from "node:fs"; +import { tmpdir } from "node:os"; +import { join, resolve } from "node:path"; +import { after, before, test } from "node:test"; import { isContainedIn, resolveContainedCopies } from "./registry-target-paths.mjs"; -const ROOT = "/tmp/hf-catalog-demo"; -const always = () => true; +// Real fixtures rather than string cases: the second escape this guards is a +// symlink, which only exists on a filesystem. A purely lexical test suite is +// exactly what stayed green through the first version of this check. +let sandbox; +let project; -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`]]); +before(() => { + sandbox = mkdtempSync(join(tmpdir(), "hf-registry-paths-")); + project = join(sandbox, "project"); + mkdirSync(join(project, "nested"), { recursive: true }); + mkdirSync(join(sandbox, "outside"), { recursive: true }); + writeFileSync(join(project, "demo.html"), "\n"); + writeFileSync(join(sandbox, "secret.txt"), "runner secret\n"); + symlinkSync(join(sandbox, "outside"), join(project, "escape")); + symlinkSync(join(sandbox, "secret.txt"), join(project, "leak.txt")); + symlinkSync(join(project, "nested"), join(project, "inward")); }); -// Both fields are attacker-controlled: catalog-previews.yml runs on -// pull_request for any registry change, so the manifest arrives from the PR. +after(() => rmSync(sandbox, { recursive: true, force: true })); -test("a traversing path cannot read outside the project", () => { - const copies = resolveContainedCopies( - ROOT, - [{ path: "../../../../etc/passwd", target: "leak.txt" }], - always, - ); - assert.deepEqual(copies, []); -}); +const allow = (files) => resolveContainedCopies(project, files, existsSync); -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("an ordinary manifest entry is copied", () => { + // Compared against the real path: the helper resolves the project root, which + // matters on macOS where the temp directory is itself a symlink. + const real = realpathSync(project); + assert.deepEqual(allow([{ path: "demo.html", target: "compositions/demo.html" }]), [ + [resolve(real, "demo.html"), resolve(real, "compositions/demo.html")], + ]); }); 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`]]); + assert.equal(allow([{ path: "nested/../demo.html", target: "out/demo.html" }]).length, 1); +}); + +// Lexical escapes. + +test("a traversing path cannot read outside the project", () => { + assert.deepEqual(allow([{ path: "../secret.txt", target: "stolen.txt" }]), []); +}); + +test("a traversing target cannot write outside the project", () => { + assert.deepEqual(allow([{ path: "demo.html", target: "../pwned.txt" }]), []); +}); + +test("an absolute path or target is refused on either side", () => { + assert.deepEqual(allow([{ path: "/etc/passwd", target: "stolen.txt" }]), []); + assert.deepEqual(allow([{ path: "demo.html", target: "/tmp/pwned.txt" }]), []); }); test("a sibling directory sharing the project's prefix is still outside", () => { - assert.equal(isContainedIn(ROOT, "../hf-catalog-demo-evil/x"), false); + assert.equal(isContainedIn(project, "../project-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), - [], - ); +// Symbolic escapes. resolve()/relative() do not follow links, so every case +// below passed the first, lexical-only version of this check. + +test("a symlinked target directory cannot be written through", () => { + assert.deepEqual(allow([{ path: "demo.html", target: "escape/pwned.txt" }]), []); +}); + +test("a symlinked source file cannot be read through", () => { + assert.deepEqual(allow([{ path: "leak.txt", target: "stolen.txt" }]), []); +}); + +test("a symlink is refused even when it points back inside the project", () => { + // Rejected rather than followed: nothing in the registry needs a symlink, and + // allowing one means trusting its target not to change before the copy. + assert.deepEqual(allow([{ path: "demo.html", target: "inward/a.txt" }]), []); +}); + +test("a deeper path through a symlinked component is refused", () => { + assert.deepEqual(allow([{ path: "demo.html", target: "escape/a/b/c.txt" }]), []); }); test("incomplete entries are skipped rather than resolved", () => { - assert.deepEqual( - resolveContainedCopies(ROOT, [{ path: "demo.html" }, { target: "x" }, {}], always), - [], - ); + assert.deepEqual(allow([{ path: "demo.html" }, { target: "x" }, {}]), []); +}); + +test("containment does not depend on the candidate existing", () => { + assert.equal(isContainedIn(project, "../../etc/passwd"), false); + assert.equal(isContainedIn(project, "not-created-yet/file.txt"), true); });