diff --git a/.changeset/node-modules-bin-prettier-bug.md b/.changeset/node-modules-bin-prettier-bug.md new file mode 100644 index 00000000..c38930fe --- /dev/null +++ b/.changeset/node-modules-bin-prettier-bug.md @@ -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 @latest -- …`, and the pnpm/yarn `dlx` equivalents) so it runs the real tool instead of a project-local bin shadow. diff --git a/packages/cli-core/src/commands/init/format.test.ts b/packages/cli-core/src/commands/init/format.test.ts index 9dbad892..f036f3db 100644 --- a/packages/cli-core/src/commands/init/format.test.ts +++ b/packages/cli-core/src/commands/init/format.test.ts @@ -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", + ], ]); }); @@ -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"], ]); }); @@ -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"], ]); }); @@ -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 () => { @@ -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) }; @@ -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"], ]); }); @@ -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"], ]); }); @@ -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; @@ -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); }); }); diff --git a/packages/cli-core/src/commands/init/format.ts b/packages/cli-core/src/commands/init/format.ts index 272471a1..5b15af15 100644 --- a/packages/cli-core/src/commands/init/format.ts +++ b/packages/cli-core/src/commands/init/format.ts @@ -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], }, ]; @@ -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 { if (files.length === 0) return; @@ -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, diff --git a/packages/cli-core/src/lib/runners.test.ts b/packages/cli-core/src/lib/runners.test.ts index 194a90f3..eaa54ac8 100644 --- a/packages/cli-core/src/lib/runners.test.ts +++ b/packages/cli-core/src/lib/runners.test.ts @@ -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/ before the + // registry. Only an explicit version spec (`@latest`) forces a registry + // fetch; a bare `--package ` still runs the local bin. + test("pins @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 @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", + ]); }); }); @@ -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"]); }); }); diff --git a/packages/cli-core/src/lib/runners.ts b/packages/cli-core/src/lib/runners.ts index db147744..b2d91cc5 100644 --- a/packages/cli-core/src/lib/runners.ts +++ b/packages/cli-core/src/lib/runners.ts @@ -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/` before the + * registry, so a plain `bunx prettier` inside an untrusted project would run a + * planted bin. Pinning with an explicit version spec (`--package @latest`) + * forces the runner to fetch `pkg` from the registry and run its bin, never the + * project-local shadow — a bare `--package ` (no version) still resolves + * the local install. `pnpm dlx` / `yarn dlx` already fetch into an isolated + * store, but take the same `@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)]; } diff --git a/packages/cli-core/src/lib/skills.ts b/packages/cli-core/src/lib/skills.ts index 5de61f8c..494d541c 100644 --- a/packages/cli-core/src/lib/skills.ts +++ b/packages/cli-core/src/lib/skills.ts @@ -59,7 +59,7 @@ export async function runSkillsAdd( interactive: boolean, label: string, ): Promise { - 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();