Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/node-modules-bin-prettier-bug.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"clerk": patch
---

Prevent `clerk init` from executing an attacker-planted `node_modules/.bin/{prettier,biome,skills}` binary from the project being set up. The formatter and skills steps now pin the package runner to the registry (`bunx --package <pkg>@latest -- …`, and the pnpm/yarn `dlx` equivalents) so it runs the real tool instead of a project-local bin shadow.
77 changes: 66 additions & 11 deletions packages/cli-core/src/commands/init/format.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,17 @@ describe("runFormatters", () => {
});
await runFormatters(ctx, ["src/a.ts", "src/b.ts"]);
expect(spawnCalls).toEqual([
["bunx", "prettier", "--ignore-unknown", "--write", "src/a.ts", "src/b.ts"],
[
"bunx",
"--package",
"prettier@latest",
"--",
"prettier",
"--ignore-unknown",
"--write",
"src/a.ts",
"src/b.ts",
],
]);
});

Expand All @@ -129,8 +139,10 @@ describe("runFormatters", () => {
deps: { "@biomejs/biome": "1.9.0" },
});
await runFormatters(ctx, ["src/a.ts"]);
// pnpm dlx fetches the package into an isolated store (no pin needed) and
// runs its `biome` bin.
expect(spawnCalls).toEqual([
["pnpm", "dlx", "@biomejs/biome", "format", "--write", "src/a.ts"],
["pnpm", "dlx", "@biomejs/biome@latest", "format", "--write", "src/a.ts"],
]);
});

Expand All @@ -142,8 +154,17 @@ describe("runFormatters", () => {
});
await runFormatters(ctx, ["x.ts"]);
expect(spawnCalls).toEqual([
["npx", "prettier", "--ignore-unknown", "--write", "x.ts"],
["npx", "@biomejs/biome", "format", "--write", "x.ts"],
[
"npx",
"--package",
"prettier@latest",
"--",
"prettier",
"--ignore-unknown",
"--write",
"x.ts",
],
["npx", "--package", "@biomejs/biome@latest", "--", "biome", "format", "--write", "x.ts"],
]);
});

Expand All @@ -156,7 +177,18 @@ describe("runFormatters", () => {
deps: { prettier: "3.0.0" },
});
await runFormatters(ctx, ["x.ts"]);
expect(spawnCalls).toEqual([["npx", "prettier", "--ignore-unknown", "--write", "x.ts"]]);
expect(spawnCalls).toEqual([
[
"npx",
"--package",
"prettier@latest",
"--",
"prettier",
"--ignore-unknown",
"--write",
"x.ts",
],
]);
});

test("skips silently when no runners are on PATH", async () => {
Expand Down Expand Up @@ -186,7 +218,7 @@ describe("runFormatters", () => {
const attempted: SpawnArgs[] = [];
setSpawn((cmd) => {
attempted.push(cmd);
if (cmd[1] === "prettier") {
if (cmd.includes("prettier")) {
throw new Error("spawn failed");
}
return { exited: Promise.resolve(0) };
Expand All @@ -199,8 +231,17 @@ describe("runFormatters", () => {
// Should not throw even though prettier spawn blows up.
await runFormatters(ctx, ["x.ts"]);
expect(attempted).toEqual([
["npx", "prettier", "--ignore-unknown", "--write", "x.ts"],
["npx", "@biomejs/biome", "format", "--write", "x.ts"],
[
"npx",
"--package",
"prettier@latest",
"--",
"prettier",
"--ignore-unknown",
"--write",
"x.ts",
],
["npx", "--package", "@biomejs/biome@latest", "--", "biome", "format", "--write", "x.ts"],
]);
});

Expand All @@ -217,8 +258,17 @@ describe("runFormatters", () => {
await runFormatters(ctx, ["x.ts"]);
// Both formatters attempted despite prettier exiting non-zero.
expect(spawnCalls).toEqual([
["npx", "prettier", "--ignore-unknown", "--write", "x.ts"],
["npx", "@biomejs/biome", "format", "--write", "x.ts"],
[
"npx",
"--package",
"prettier@latest",
"--",
"prettier",
"--ignore-unknown",
"--write",
"x.ts",
],
["npx", "--package", "@biomejs/biome@latest", "--", "biome", "format", "--write", "x.ts"],
]);
});

Expand All @@ -232,7 +282,7 @@ describe("runFormatters", () => {
expect(spawnCalls).toHaveLength(0);
});

test("spawns in the project cwd", async () => {
test("pins the package so the runner can't resolve a project-local bin", async () => {
let seenCwd: string | undefined;
setSpawn((cmd, opts?: { cwd?: string }) => {
seenCwd = opts?.cwd;
Expand All @@ -245,6 +295,11 @@ describe("runFormatters", () => {
deps: { prettier: "3.0.0" },
});
await runFormatters(ctx, ["x.ts"]);

const cmd = spawnCalls[0]!;
// The bin only appears after the `--`, never as a bare argv the runner
// could resolve from node_modules/.bin.
expect(cmd.slice(0, 4)).toEqual(["bunx", "--package", "prettier@latest", "--"]);
expect(seenCwd).toBe(tempDir);
});
});
13 changes: 8 additions & 5 deletions packages/cli-core/src/commands/init/format.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,18 @@ import type { ProjectContext } from "./frameworks/types.js";

type FormatterConfig = {
pkg: string;
/** Args after the runner: binary + flags + files. The runner is prepended at spawn time. */
binArgs: (files: string[]) => string[];
/** Bin invocation: bin name + flags + files. Prefixed with the runner at spawn time. */
command: (files: string[]) => string[];
};

const FORMATTERS: FormatterConfig[] = [
{
pkg: "prettier",
binArgs: (files) => ["prettier", "--ignore-unknown", "--write", ...files],
command: (files) => ["prettier", "--ignore-unknown", "--write", ...files],
},
{
pkg: "@biomejs/biome",
binArgs: (files) => ["@biomejs/biome", "format", "--write", ...files],
command: (files) => ["biome", "format", "--write", ...files],
},
];

Expand All @@ -24,6 +24,9 @@ const FORMATTERS: FormatterConfig[] = [
*
* Best-effort: failures are silent (stdio ignored, spawn errors swallowed)
* because formatting is purely cosmetic and shouldn't break init.
*
* The runner is pinned so it fetches the formatter from the registry rather
* than a project-local `node_modules/.bin` shadow — see {@link runnerCommand}.
*/
export async function runFormatters(ctx: ProjectContext, files: string[]): Promise<void> {
if (files.length === 0) return;
Expand All @@ -37,7 +40,7 @@ export async function runFormatters(ctx: ProjectContext, files: string[]): Promi
const runner = preferredRunner(ctx.packageManager, available);

for (const formatter of matchingFormatters) {
const command = runnerCommand(runner, formatter.binArgs(files));
const command = runnerCommand(runner, formatter.pkg, formatter.command(files));
try {
const proc = Bun.spawn(command, {
cwd: ctx.cwd,
Expand Down
50 changes: 36 additions & 14 deletions packages/cli-core/src/lib/runners.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,35 +83,57 @@ describe("runnerCommand", () => {
const pnpm = KNOWN_RUNNERS.find((r) => r.id === "pnpm")!;
const yarn = KNOWN_RUNNERS.find((r) => r.id === "yarn")!;

test("prepends the runner binary for prefix-less runners", () => {
expect(runnerCommand(bunx, ["skills", "add", "clerk/skills"])).toEqual([
// bunx/npx resolve a project-local node_modules/.bin/<bin> before the
// registry. Only an explicit version spec (`@latest`) forces a registry
// fetch; a bare `--package <pkg>` still runs the local bin.
test("pins <pkg>@latest for prefix-less runners (bunx/npx)", () => {
expect(runnerCommand(bunx, "skills", ["skills", "add", "clerk/skills"])).toEqual([
"bunx",
"--package",
"skills@latest",
"--",
"skills",
"add",
"clerk/skills",
]);
expect(runnerCommand(npx, ["prettier", "--write", "x.ts"])).toEqual([
expect(runnerCommand(npx, "prettier", ["prettier", "--write", "x.ts"])).toEqual([
"npx",
"--package",
"prettier@latest",
"--",
"prettier",
"--write",
"x.ts",
]);
});

test("inserts dlx between binary and args for pnpm/yarn", () => {
expect(runnerCommand(pnpm, ["prettier", "--write", "x.ts"])).toEqual([
test("pins by package name even when the bin name differs (biome)", () => {
expect(runnerCommand(bunx, "@biomejs/biome", ["biome", "format", "x.ts"])).toEqual([
"bunx",
"--package",
"@biomejs/biome@latest",
"--",
"biome",
"format",
"x.ts",
]);
});

test("uses dlx <pkg>@latest for pnpm/yarn, dropping the redundant bin name", () => {
expect(runnerCommand(pnpm, "prettier", ["prettier", "--write", "x.ts"])).toEqual([
"pnpm",
"dlx",
"prettier",
"prettier@latest",
"--write",
"x.ts",
]);
expect(runnerCommand(yarn, ["skills", "add"])).toEqual(["yarn", "dlx", "skills", "add"]);
});

test("handles empty args", () => {
expect(runnerCommand(bunx, [])).toEqual(["bunx"]);
expect(runnerCommand(pnpm, [])).toEqual(["pnpm", "dlx"]);
expect(runnerCommand(yarn, "@biomejs/biome", ["biome", "format", "x.ts"])).toEqual([
"yarn",
"dlx",
"@biomejs/biome@latest",
"format",
"x.ts",
]);
});
});

Expand Down Expand Up @@ -229,7 +251,7 @@ describe("detectAvailableRunners", () => {
const runner = preferredRunner("pnpm", available);
expect(runner.id).toBe("pnpm");

const command = runnerCommand(runner, ["prettier", "--write", "src/x.ts"]);
expect(command).toEqual(["pnpm", "dlx", "prettier", "--write", "src/x.ts"]);
const command = runnerCommand(runner, "prettier", ["prettier", "--write", "src/x.ts"]);
expect(command).toEqual(["pnpm", "dlx", "prettier@latest", "--write", "src/x.ts"]);
});
});
34 changes: 27 additions & 7 deletions packages/cli-core/src/lib/runners.ts
Original file line number Diff line number Diff line change
Expand Up @@ -134,17 +134,37 @@ export function preferredRunner(
}

/**
* Build the full spawn argv for invoking a command via a runner.
* Build the full spawn argv for running `pkg`'s binary via a runner.
*
* `pkg` is the npm package to fetch (`prettier`, `@biomejs/biome`, `skills`).
* `command` is the bin invocation: the bin name followed by its args
* (`["prettier", "--write", "file.ts"]`, `["biome", "format", …]`,
* `["skills", "add", "clerk/skills"]`).
*
* `bunx`/`npx` resolve a project-local `node_modules/.bin/<bin>` before the
* registry, so a plain `bunx prettier` inside an untrusted project would run a
* planted bin. Pinning with an explicit version spec (`--package <pkg>@latest`)
* forces the runner to fetch `pkg` from the registry and run its bin, never the
* project-local shadow — a bare `--package <pkg>` (no version) still resolves
* the local install. `pnpm dlx` / `yarn dlx` already fetch into an isolated
* store, but take the same `<pkg>@latest` spec for consistency.
*
* @example
* ```ts
* runnerCommand(bunx, ["skills", "add", "clerk/skills"])
* // => ["bunx", "skills", "add", "clerk/skills"]
* runnerCommand(bunx, "prettier", ["prettier", "--write", "file.ts"])
* // => ["bunx", "--package", "prettier@latest", "--", "prettier", "--write", "file.ts"]
*
* runnerCommand(pnpm, ["prettier", "--write", "file.ts"])
* // => ["pnpm", "dlx", "prettier", "--write", "file.ts"]
* runnerCommand(pnpm, "@biomejs/biome", ["biome", "format", "file.ts"])
* // => ["pnpm", "dlx", "@biomejs/biome@latest", "format", "file.ts"]
* ```
*/
export function runnerCommand(runner: Runner, args: readonly string[]): string[] {
return [runner.binary, ...runner.prefixArgs, ...args];
export function runnerCommand(runner: Runner, pkg: string, command: readonly string[]): string[] {
// The project's declared version is attacker-controlled (e.g. `file:./evil`),
// so pin to the registry's `@latest` rather than anything from package.json.
const spec = `${pkg}@latest`;
if (runner.prefixArgs.length === 0) {
return [runner.binary, "--package", spec, "--", ...command];
}
// dlx picks the bin from the package, so drop the redundant bin name at command[0].
return [runner.binary, ...runner.prefixArgs, spec, ...command.slice(1)];
}
2 changes: 1 addition & 1 deletion packages/cli-core/src/lib/skills.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ export async function runSkillsAdd(
interactive: boolean,
label: string,
): Promise<boolean> {
const command = runnerCommand(runner, buildSkillsArgs(source, skillNames, interactive));
const command = runnerCommand(runner, "skills", buildSkillsArgs(source, skillNames, interactive));
const displayCommand = `${runner.display} skills add ${source}`;

log.blank();
Expand Down