diff --git a/src/cmd-injection-regression.test.ts b/src/cmd-injection-regression.test.ts new file mode 100644 index 0000000..4b14f90 --- /dev/null +++ b/src/cmd-injection-regression.test.ts @@ -0,0 +1,108 @@ +// Regression tests for command-injection via filesystem-derived paths +// reaching shell-interpreted command strings in the validation pipeline. +// +// `runCommand` in src/exec.ts uses `spawn(..., { shell: true })`, so any +// command string the validation pipeline produces is passed straight to +// /bin/sh -c. Each mapper that interpolates a filesystem-derived path or +// package-config-derived name into a command string must shell-quote that +// value via `shellQuotePath` so attacker-controlled metacharacters cannot +// be parsed as shell syntax. +// +// Each test below builds a project tree with attacker-controlled names +// and asserts the produced commands are shell-safe (no unquoted `$(...)`, +// no unquoted `;`, etc.). + +import { mkdtemp, mkdir, writeFile, rm } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { describe, expect, it } from "vitest"; +import { mapFeatures } from "./mapper.js"; +import { validationCommandsForFeature } from "./validation.js"; +import { detectProject } from "./detect.js"; +import { scriptCommand } from "./mappers/projects.js"; +import { nodeScriptCommand } from "./mappers/shared.js"; + +function isShellSafe(command: string): boolean { + // Strip double-quoted segments — anything `$(...)` or `;` inside `"..."` + // is shell-literal (backslash-escaped `$` and `\`). + const withoutQuoted = command.replace(/"(?:\\.|[^"\\])*"/gu, ""); + if (/\$\(/u.test(withoutQuoted)) return false; + if (/`/u.test(withoutQuoted)) return false; + if (/(?:^|\s);/u.test(withoutQuoted)) return false; + if (/&&|\|\|/u.test(withoutQuoted)) return false; + return true; +} + +describe("validation pipeline shell-quotes filesystem-derived names (regression)", () => { + it("scriptCommand: malicious workspace package-root is shell-quoted", () => { + for (const pm of ["pnpm", "yarn", "bun", "npm"] as const) { + const cmd = scriptCommand(pm, "packages/$(id)-pkg", "test"); + expect(isShellSafe(cmd), `pm=${pm} cmd=${cmd}`).toBe(true); + } + }); + + it("scriptCommand: malicious package.json script-name is shell-quoted", () => { + for (const pm of ["pnpm", "yarn", "bun", "npm"] as const) { + const cmd = scriptCommand(pm, "packages/ok", "$(id)"); + expect(isShellSafe(cmd), `pm=${pm} cmd=${cmd}`).toBe(true); + } + }); + + it("scriptCommand: ordinary inputs are unchanged (no over-quoting)", () => { + expect(scriptCommand("pnpm", "packages/ui", "test")).toBe("pnpm --dir packages/ui test"); + expect(scriptCommand("npm", "packages/ui", "test")).toBe("npm --prefix packages/ui run test"); + expect(scriptCommand("yarn", "packages/ui", "test")).toBe("yarn --cwd packages/ui test"); + expect(scriptCommand("bun", "packages/ui", "test")).toBe("bun --cwd packages/ui run test"); + expect(scriptCommand("pnpm", ".", "test")).toBe("pnpm test"); + expect(scriptCommand("npm", ".", "test")).toBe("npm run test"); + expect(scriptCommand("bun", ".", "test")).toBe("bun run test"); + }); + + it("nodeScriptCommand: malicious workspace package-root is shell-quoted", () => { + for (const pm of ["pnpm", "yarn", "bun", "npm"] as const) { + const cmd = nodeScriptCommand(pm, "packages/$(id)-pkg", "test"); + expect(isShellSafe(cmd), `pm=${pm} cmd=${cmd}`).toBe(true); + } + }); + + it("nodeScriptCommand: ordinary inputs are unchanged", () => { + expect(nodeScriptCommand("pnpm", "apps/web", "test")).toBe("pnpm --dir apps/web test"); + expect(nodeScriptCommand("npm", "apps/web", "test")).toBe("npm --prefix apps/web run test"); + }); + + it("swift end-to-end: malicious package-root directory cannot inject into swift test --package-path", async () => { + const root = await mkdtemp(join(tmpdir(), "clawpatch-cmd-inj-swift-")); + try { + const maliciousPkg = "$(id)-pkg"; + await mkdir(join(root, maliciousPkg, "Sources", "Evil"), { recursive: true }); + await writeFile( + join(root, maliciousPkg, "Package.swift"), + '// swift-tools-version:5.9\nimport PackageDescription\nlet package = Package(name: "evil", targets: [.target(name: "Evil"), .testTarget(name: "EvilTests", dependencies: ["Evil"])])\n', + ); + await writeFile( + join(root, maliciousPkg, "Sources", "Evil", "Evil.swift"), + "public struct Evil {}\n", + ); + await mkdir(join(root, maliciousPkg, "Tests", "EvilTests"), { recursive: true }); + await writeFile( + join(root, maliciousPkg, "Tests", "EvilTests", "EvilTests.swift"), + "import XCTest\nclass EvilTests: XCTestCase {}\n", + ); + const project = await detectProject(root); + const result = await mapFeatures(root, project, []); + for (const feature of result.features) { + const commands = validationCommandsForFeature(feature, { + typecheck: null, + lint: null, + format: null, + test: null, + }); + for (const c of commands) { + expect(isShellSafe(c), `unsafe command produced: ${c}`).toBe(true); + } + } + } finally { + await rm(root, { recursive: true, force: true }); + } + }); +}); diff --git a/src/mappers/elixir.ts b/src/mappers/elixir.ts index 6942d29..739a50f 100644 --- a/src/mappers/elixir.ts +++ b/src/mappers/elixir.ts @@ -1,6 +1,7 @@ import { readFile, readdir } from "node:fs/promises"; import { basename, join } from "node:path"; import { pathExists } from "../fs.js"; +import { shellQuotePath } from "../shell.js"; import { packageKind, pathMatchesPrefix, shouldSkip, stripLineComments, walk } from "./shared.js"; import { FeatureSeed, SeedTestRef } from "./types.js"; @@ -301,7 +302,7 @@ function associatedTests(files: string[], testFiles: string[]): SeedTestRef[] { return testFiles .filter((path) => prefixes.some((prefix) => pathMatchesPrefix(path, prefix))) .slice(0, elixirTestGroupMaxFiles) - .map((path) => ({ path, command: `mix test ${path}` })); + .map((path) => ({ path, command: `mix test ${shellQuotePath(path)}` })); } function testPrefixesForSource(path: string): string[] { diff --git a/src/mappers/projects.ts b/src/mappers/projects.ts index dcf60f2..3983cb3 100644 --- a/src/mappers/projects.ts +++ b/src/mappers/projects.ts @@ -2,6 +2,7 @@ import { lstat, readFile, readdir, realpath } from "node:fs/promises"; import { basename, dirname, join } from "node:path"; import { packageScripts, readPackageJson } from "../detect.js"; import { pathExists } from "../fs.js"; +import { shellQuotePath } from "../shell.js"; import { isSafeDirectory, normalize, pathMatchesPrefix, shouldSkip } from "./shared.js"; import { taskGraphCommand, type WorkspaceTaskGraph } from "./task-graph.js"; import type { SeedFileRef } from "./types.js"; @@ -194,22 +195,26 @@ export function packageRelativePath(packageRoot: string, path: string): string { } export function scriptCommand(packageManager: string, packageRoot: string, script: string): string { + const quotedRoot = shellQuotePath(packageRoot); + const quotedScript = shellQuotePath(script); if (packageRoot === ".") { if (packageManager === "bun") { - return `bun run ${script}`; + return `bun run ${quotedScript}`; } - return packageManager === "npm" ? `npm run ${script}` : `${packageManager} ${script}`; + return packageManager === "npm" + ? `npm run ${quotedScript}` + : `${packageManager} ${quotedScript}`; } if (packageManager === "pnpm") { - return `pnpm --dir ${packageRoot} ${script}`; + return `pnpm --dir ${quotedRoot} ${quotedScript}`; } if (packageManager === "yarn") { - return `yarn --cwd ${packageRoot} ${script}`; + return `yarn --cwd ${quotedRoot} ${quotedScript}`; } if (packageManager === "bun") { - return `bun --cwd ${packageRoot} run ${script}`; + return `bun --cwd ${quotedRoot} run ${quotedScript}`; } - return `npm --prefix ${packageRoot} run ${script}`; + return `npm --prefix ${quotedRoot} run ${quotedScript}`; } export function projectDisplayName(info: NodeProjectInfo): string { diff --git a/src/mappers/shared.ts b/src/mappers/shared.ts index 07a71e1..45656b7 100644 --- a/src/mappers/shared.ts +++ b/src/mappers/shared.ts @@ -1,6 +1,7 @@ import { lstat, readdir, realpath } from "node:fs/promises"; import { dirname, isAbsolute, join, relative, sep } from "node:path"; import { pathExists } from "../fs.js"; +import { shellQuotePath } from "../shell.js"; import { TrustBoundary } from "../types.js"; import { FeatureSeed } from "./types.js"; @@ -359,22 +360,26 @@ export function nodeScriptCommand( packageRoot: string, script: string, ): string { + const quotedRoot = shellQuotePath(packageRoot); + const quotedScript = shellQuotePath(script); if (packageRoot === ".") { if (packageManager === "bun") { - return `bun run ${script}`; + return `bun run ${quotedScript}`; } - return packageManager === "npm" ? `npm run ${script}` : `${packageManager} ${script}`; + return packageManager === "npm" + ? `npm run ${quotedScript}` + : `${packageManager} ${quotedScript}`; } if (packageManager === "pnpm") { - return `pnpm --dir ${packageRoot} ${script}`; + return `pnpm --dir ${quotedRoot} ${quotedScript}`; } if (packageManager === "yarn") { - return `yarn --cwd ${packageRoot} ${script}`; + return `yarn --cwd ${quotedRoot} ${quotedScript}`; } if (packageManager === "bun") { - return `bun --cwd ${packageRoot} run ${script}`; + return `bun --cwd ${quotedRoot} run ${quotedScript}`; } - return `npm --prefix ${packageRoot} run ${script}`; + return `npm --prefix ${quotedRoot} run ${quotedScript}`; } function isTestPath(path: string): boolean { diff --git a/src/mappers/swift.ts b/src/mappers/swift.ts index b2d20d3..2377a70 100644 --- a/src/mappers/swift.ts +++ b/src/mappers/swift.ts @@ -1,6 +1,7 @@ import { lstat, readFile, readdir } from "node:fs/promises"; import { join } from "node:path"; import { pathExists } from "../fs.js"; +import { shellQuotePath } from "../shell.js"; import { normalize, isSampleProjectPath, @@ -174,7 +175,8 @@ function prefixSwiftSeed(seed: FeatureSeed, packageRoot: string): FeatureSeed { if (packageRoot === ".") { return seed; } - const testCommand = seed.testCommand === null ? null : `swift test --package-path ${packageRoot}`; + const testCommand = + seed.testCommand === null ? null : `swift test --package-path ${shellQuotePath(packageRoot)}`; const prefixed: FeatureSeed = { ...seed, title: `${seed.title} (${packageRoot})`, @@ -214,7 +216,8 @@ function prefixTestRefs( return refs?.map((ref) => ({ ...ref, path: prefixSwiftPath(packageRoot, ref.path), - command: ref.command === null ? null : `swift test --package-path ${packageRoot}`, + command: + ref.command === null ? null : `swift test --package-path ${shellQuotePath(packageRoot)}`, })); }