fix(scripts): make registry path containment filesystem-aware

Miguel's second P1 on #2975, and he is right that my first fix only closed half
of it. `resolve()` and `relative()` are string operations and do not follow
links. Registry items are copied in recursively with symlinks preserved, so a
PR shipping `escape -> /tmp/outside` and declaring `target: "escape/pwned.txt"`
passed the lexical check, `mkdirSync` followed the link, and `cpSync` wrote
outside the project.

Reproduced before fixing: the old predicate returned one allowed copy and the
file appeared outside the project. Both directions were exposed — a symlinked
`path` reads a runner file in just as readily.

Containment is now filesystem-aware. No existing component of a candidate may
be a symlink, and the candidate's real location — resolved through its deepest
existing ancestor — has to sit under the project's own real path. A symlink is
refused rather than followed, even one pointing back inside the project:
nothing in the registry needs one, and following it would mean trusting the
target not to change between the check and the copy.

The tests are real fixtures now instead of string cases, because a purely
lexical suite is exactly what stayed green through the bypass. Twelve of them,
covering a symlinked target directory, a symlinked source file, a deeper path
through a symlinked component, an inward-pointing symlink, plus the lexical and
absolute cases from before.

Author: ukimsanov

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` 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` resolves to `root` itself or something beneath it. */
+/**
+ * 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))

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"), "<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 }));
+
+const allow = (files) => resolveContainedCopies(project, files, existsSync);
+
+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", () => {
+  assert.equal(allow([{ path: "nested/../demo.html", target: "out/demo.html" }]).length, 1);
+});
+
+// Lexical escapes.
 
 test("a traversing path cannot read outside the project", () => {
-  const copies = resolveContainedCopies(
-    ROOT,
-    [{ path: "../../../../etc/passwd", target: "leak.txt" }],
-    always,
-  );
-  assert.deepEqual(copies, []);
+  assert.deepEqual(allow([{ path: "../secret.txt", target: "stolen.txt" }]), []);
 });
 
 test("a traversing target cannot write outside the project", () => {
-  const copies = resolveContainedCopies(
-    ROOT,
-    [{ path: "demo.html", target: "../../../../home/runner/.bashrc" }],
-    always,
-  );
-  assert.deepEqual(copies, []);
+  assert.deepEqual(allow([{ path: "demo.html", target: "../pwned.txt" }]), []);
 });
 
 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),
-    [],
-  );
+  assert.deepEqual(allow([{ path: "/etc/passwd", target: "stolen.txt" }]), []);
+  assert.deepEqual(allow([{ path: "demo.html", target: "/tmp/pwned.txt" }]), []);
 });
 
-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(project, "../project-evil/x"), false);
 });
 
-test("a sibling directory sharing the project's prefix is still outside", () => {
-  assert.equal(isContainedIn(ROOT, "../hf-catalog-demo-evil/x"), false);
+// 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("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("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);
 });