Skip to content

Commit b3bae35

Browse files
committed
fix(scripts): contain registry manifest paths in the preview renderer
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.
1 parent 5f26c59 commit b3bae35

4 files changed

Lines changed: 120 additions & 8 deletions

File tree

package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@
4848
"player:perf": "bun run --filter @hyperframes/player perf",
4949
"format:check": "oxfmt --check .",
5050
"knip": "knip",
51-
"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",
51+
"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",
5252
"test:skills": "node --test 'skills/**/*.test.mjs'",
5353
"generate:previews": "tsx scripts/generate-template-previews.ts",
5454
"generate:catalog-previews": "tsx scripts/generate-catalog-previews.ts",

scripts/generate-catalog-previews.ts

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@ import {
4343
executeRenderJob,
4444
} from "../packages/producer/src/index.js";
4545
import { compileForRender } from "../packages/producer/src/services/htmlCompiler.js";
46+
import { resolveContainedCopies } from "./registry-target-paths.mjs";
4647

4748
const scriptDir = dirname(fileURLToPath(import.meta.url));
4849
const repoRoot = resolve(scriptDir, "..");
@@ -139,13 +140,11 @@ function mirrorRegistryTargets(projectDir: string): void {
139140
files?: { path?: string; target?: string }[];
140141
};
141142

142-
const copies = (manifest.files ?? [])
143-
.map((file) => [file.path, file.target] as const)
144-
.filter((pair): pair is readonly [string, string] => Boolean(pair[0] && pair[1]))
145-
.map(([path, target]) => [join(projectDir, path), join(projectDir, target)] as const)
146-
.filter(([from, to]) => from !== to && existsSync(from));
147-
148-
for (const [from, to] of copies) {
143+
// registry-item.json is untrusted: catalog-previews.yml runs on pull_request
144+
// for any registry change, so the manifest arrives from the PR. Containment
145+
// lives in its own module so the traversal cases stay testable without this
146+
// file's producer imports.
147+
for (const [from, to] of resolveContainedCopies(projectDir, manifest.files, existsSync)) {
149148
mkdirSync(dirname(to), { recursive: true });
150149
cpSync(from, to);
151150
}

scripts/registry-target-paths.mjs

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
/**
2+
* Containment for registry manifest paths.
3+
*
4+
* `registry-item.json` is untrusted input. `catalog-previews.yml` runs on
5+
* `pull_request` for any change under `registry/blocks/**` or
6+
* `registry/components/**`, so a contributor's own manifest reaches the preview
7+
* renderer, and the job then uploads `docs/images/catalog/` as an artifact.
8+
*
9+
* `join()` happily walks out of its first argument, so a `files[].path` of
10+
* `../../../../etc/passwd` reads an arbitrary runner file into the project, and
11+
* a `files[].target` of the same shape writes an arbitrary runner path. Both
12+
* sides have to be resolved and checked, not just the one that looks like input.
13+
*/
14+
15+
import { isAbsolute, relative, resolve } from "node:path";
16+
17+
/** True when `candidate` resolves to `root` itself or something beneath it. */
18+
export function isContainedIn(root, candidate) {
19+
const step = relative(resolve(root), resolve(root, candidate));
20+
return step === "" || (!step.startsWith("..") && !isAbsolute(step));
21+
}
22+
23+
/**
24+
* The `[from, to]` pairs safe to copy, dropping any that escape `projectDir`.
25+
*
26+
* `exists` is injected so the containment rule can be tested without a fixture
27+
* tree — the traversal decision must not depend on whether the target happens
28+
* to be present on the runner.
29+
*/
30+
export function resolveContainedCopies(projectDir, files, exists) {
31+
const root = resolve(projectDir);
32+
return (files ?? [])
33+
.filter((file) => file?.path && file?.target)
34+
.filter((file) => isContainedIn(root, file.path) && isContainedIn(root, file.target))
35+
.map((file) => [resolve(root, file.path), resolve(root, file.target)])
36+
.filter(([from, to]) => from !== to && exists(from));
37+
}
Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
import { strict as assert } from "node:assert";
2+
import { test } from "node:test";
3+
4+
import { isContainedIn, resolveContainedCopies } from "./registry-target-paths.mjs";
5+
6+
const ROOT = "/tmp/hf-catalog-demo";
7+
const always = () => true;
8+
9+
test("ordinary manifest entries are copied", () => {
10+
const copies = resolveContainedCopies(
11+
ROOT,
12+
[{ path: "demo.html", target: "compositions/demo.html" }],
13+
always,
14+
);
15+
assert.deepEqual(copies, [[`${ROOT}/demo.html`, `${ROOT}/compositions/demo.html`]]);
16+
});
17+
18+
// Both fields are attacker-controlled: catalog-previews.yml runs on
19+
// pull_request for any registry change, so the manifest arrives from the PR.
20+
21+
test("a traversing path cannot read outside the project", () => {
22+
const copies = resolveContainedCopies(
23+
ROOT,
24+
[{ path: "../../../../etc/passwd", target: "leak.txt" }],
25+
always,
26+
);
27+
assert.deepEqual(copies, []);
28+
});
29+
30+
test("a traversing target cannot write outside the project", () => {
31+
const copies = resolveContainedCopies(
32+
ROOT,
33+
[{ path: "demo.html", target: "../../../../home/runner/.bashrc" }],
34+
always,
35+
);
36+
assert.deepEqual(copies, []);
37+
});
38+
39+
test("an absolute path or target is refused on either side", () => {
40+
assert.deepEqual(
41+
resolveContainedCopies(ROOT, [{ path: "/etc/passwd", target: "leak.txt" }], always),
42+
[],
43+
);
44+
assert.deepEqual(
45+
resolveContainedCopies(ROOT, [{ path: "demo.html", target: "/etc/cron.d/x" }], always),
46+
[],
47+
);
48+
});
49+
50+
test("traversal that returns inside the project is allowed", () => {
51+
const copies = resolveContainedCopies(
52+
ROOT,
53+
[{ path: "nested/../demo.html", target: "out/demo.html" }],
54+
always,
55+
);
56+
assert.deepEqual(copies, [[`${ROOT}/demo.html`, `${ROOT}/out/demo.html`]]);
57+
});
58+
59+
test("a sibling directory sharing the project's prefix is still outside", () => {
60+
assert.equal(isContainedIn(ROOT, "../hf-catalog-demo-evil/x"), false);
61+
});
62+
63+
test("containment does not depend on the file existing", () => {
64+
assert.equal(isContainedIn(ROOT, "../../etc/passwd"), false);
65+
assert.deepEqual(
66+
resolveContainedCopies(ROOT, [{ path: "../../etc/passwd", target: "x" }], () => true),
67+
[],
68+
);
69+
});
70+
71+
test("incomplete entries are skipped rather than resolved", () => {
72+
assert.deepEqual(
73+
resolveContainedCopies(ROOT, [{ path: "demo.html" }, { target: "x" }, {}], always),
74+
[],
75+
);
76+
});

0 commit comments

Comments
 (0)