Skip to content
Open
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
143 changes: 143 additions & 0 deletions src/cmd-injection-regression.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,143 @@
// 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, nxCommand } from "./mappers/projects.js";
import { nodeScriptCommand } from "./mappers/shared.js";
import { turboCommand } from "./mappers/turbo.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("turboCommand: malicious turbo --filter (package name or root) is shell-quoted", () => {
for (const pm of ["pnpm", "yarn", "bun", "npm"] as const) {
// package.json `name`
expect(
isShellSafe(turboCommand(pm, "test", "$(id)")),
`pm=${pm}`,
).toBe(true);
// root fallback `./${project.root}`
expect(
isShellSafe(turboCommand(pm, "test", "./packages/$(id)-pkg")),
`pm=${pm}`,
).toBe(true);
}
});

it("turboCommand: ordinary inputs are unchanged (no over-quoting)", () => {
expect(turboCommand("pnpm", "test", "my-pkg")).toBe("pnpm turbo run test --filter my-pkg");
expect(turboCommand("npm", "test", "./packages/ui")).toBe(
"npx turbo run test --filter ./packages/ui",
);
});

it("nxCommand: malicious Nx project name is shell-quoted", () => {
for (const pm of ["npm", "bun", "pnpm", "yarn"] as const) {
const cmd = nxCommand(pm, "test", "$(id)");
expect(isShellSafe(cmd), `pm=${pm} cmd=${cmd}`).toBe(true);
}
});

it("nxCommand: ordinary inputs are unchanged", () => {
expect(nxCommand("npm", "test", "my-app")).toBe("npx nx test my-app");
expect(nxCommand("pnpm", "build", "my-app")).toBe("pnpm nx build my-app");
});

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 });
}
});
});
3 changes: 2 additions & 1 deletion src/mappers/elixir.ts
Original file line number Diff line number Diff line change
@@ -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";

Expand Down Expand Up @@ -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[] {
Expand Down
29 changes: 19 additions & 10 deletions src/mappers/projects.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -994,12 +999,16 @@ async function detectNodePackageManager(root: string): Promise<string> {
return "npm";
}

function nxCommand(packageManager: string, target: string, projectName: string): string {
export function nxCommand(packageManager: string, target: string, projectName: string): string {
// `projectName` is repo-derived (Nx project.json / package.json name / dir
// basename), so it must be shell-quoted before reaching the `shell: true`
// runner. `target` comes from the validationTaskNames allowlist.
const quotedName = shellQuotePath(projectName);
if (packageManager === "npm") {
return `npx nx ${target} ${projectName}`;
return `npx nx ${target} ${quotedName}`;
}
if (packageManager === "bun") {
return `bunx nx ${target} ${projectName}`;
return `bunx nx ${target} ${quotedName}`;
}
return `${packageManager} nx ${target} ${projectName}`;
return `${packageManager} nx ${target} ${quotedName}`;
}
17 changes: 11 additions & 6 deletions src/mappers/shared.ts
Original file line number Diff line number Diff line change
@@ -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";

Expand Down Expand Up @@ -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 {
Expand Down
7 changes: 5 additions & 2 deletions src/mappers/swift.ts
Original file line number Diff line number Diff line change
@@ -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,
Expand Down Expand Up @@ -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})`,
Expand Down Expand Up @@ -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)}`,
}));
}

Expand Down
15 changes: 10 additions & 5 deletions src/mappers/turbo.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { readFile } from "node:fs/promises";
import { join } from "node:path";
import { packageScripts } from "../detect.js";
import { pathExists } from "../fs.js";
import { shellQuotePath } from "../shell.js";
import type { NodeProjectInfo } from "./projects.js";
import { detectNodePackageManager } from "./shared.js";
import {
Expand Down Expand Up @@ -124,17 +125,21 @@ function stringArray(value: unknown): string[] {
: [];
}

function turboCommand(packageManager: string, task: string, filter: string): string {
export function turboCommand(packageManager: string, task: string, filter: string): string {
// `filter` is repo-derived (package.json name or on-disk root), so it must be
// shell-quoted before reaching the `shell: true` runner — same as the other
// validation commands. `task` comes from the validationTaskNames allowlist.
const quotedFilter = shellQuotePath(filter);
if (packageManager === "pnpm") {
return `pnpm turbo run ${task} --filter ${filter}`;
return `pnpm turbo run ${task} --filter ${quotedFilter}`;
}
if (packageManager === "yarn") {
return `yarn turbo run ${task} --filter ${filter}`;
return `yarn turbo run ${task} --filter ${quotedFilter}`;
}
if (packageManager === "bun") {
return `bunx turbo run ${task} --filter ${filter}`;
return `bunx turbo run ${task} --filter ${quotedFilter}`;
}
return `npx turbo run ${task} --filter ${filter}`;
return `npx turbo run ${task} --filter ${quotedFilter}`;
}

function turboPackageName(project: NodeProjectInfo): string | null {
Expand Down