diff --git a/src/install.ts b/src/install.ts index 6fc6b88..480a98f 100644 --- a/src/install.ts +++ b/src/install.ts @@ -1,6 +1,6 @@ /* eslint-disable no-console */ -import { exec, spawn } from "child_process"; +import { execFile, spawn } from "child_process"; import { existsSync, readFileSync } from "fs"; import inquirer from "inquirer"; import os from "os"; @@ -60,13 +60,24 @@ const TOOL_CONFIGS = { cursor: path.join(os.homedir(), ".cursor", "mcp.json"), } as const; +const execFileAsync = promisify(execFile); + // Cross-platform command finder -const findCommand = async (command: string): Promise => { +// Uses execFile to prevent shell injection vulnerabilities +// Exported for testing +export const findCommand = async (command: string): Promise => { const finder = process.platform === "win32" ? "where" : "which"; - const { stdout } = await promisify(exec)(`${finder} ${command}`); - const lines = stdout.trim().split("\n"); - if (!lines?.[0]) throw new Error(`${command} not found`); - return lines[0]!; + + try { + const { stdout } = await execFileAsync(finder, [command]); + const lines = stdout.trim().split("\n"); + if (!lines?.[0]) { + throw new Error(`${command} not found`); + } + return lines[0]!; + } catch (_error) { + throw new Error(`${command} not found`); + } }; // Auto-detect if running from a local development build @@ -945,12 +956,10 @@ export const setupMcpServer = async (): Promise => { const configJson = JSON.stringify(iterableMcpConfig); - // Remove existing config - try { - await promisify(exec)("claude mcp remove iterable"); - } catch { - // Ignore errors - } + // Remove existing config (ignore errors) + await execFileAsync("claude", ["mcp", "remove", "iterable"]).catch( + () => {} + ); spinner.start("Configuring Claude Code..."); diff --git a/src/key-manager.ts b/src/key-manager.ts index 32f2eaf..54edd43 100644 --- a/src/key-manager.ts +++ b/src/key-manager.ts @@ -22,11 +22,12 @@ */ import { logger } from "@iterable/api"; -import { spawn } from "child_process"; +import { execFile } from "child_process"; import { randomUUID } from "crypto"; import { promises as fs } from "fs"; import os from "os"; import path from "path"; +import { promisify } from "util"; import { isHttpsOrLocalhost, isLocalhostHost } from "./utils/url.js"; @@ -35,43 +36,21 @@ import { isHttpsOrLocalhost, isLocalhostHost } from "./utils/url.js"; // Tests should use dependency injection with mock execSecurity instead. const SERVICE_NAME = "iterable-mcp"; +const execFileAsync = promisify(execFile); + /** * Safely execute macOS security command with proper argument escaping - * Uses spawn to prevent shell injection vulnerabilities + * Uses execFile to prevent shell injection vulnerabilities */ async function execSecurityDefault(args: string[]): Promise { - return new Promise((resolve, reject) => { - const child = spawn("security", args, { - stdio: ["ignore", "pipe", "pipe"], - }); - - let stdout = ""; - let stderr = ""; - - child.stdout.on("data", (data) => { - stdout += data.toString(); - }); - - child.stderr.on("data", (data) => { - stderr += data.toString(); - }); - - child.on("error", (error) => { - reject(new Error(`Failed to execute security command: ${error.message}`)); - }); - - child.on("close", (code) => { - if (code === 0) { - resolve(stdout.trim()); - } else { - reject( - new Error( - `Security command failed with code ${code}: ${stderr.trim() || stdout.trim()}` - ) - ); - } - }); - }); + try { + const { stdout } = await execFileAsync("security", args); + return stdout.trim(); + } catch (error: any) { + throw new Error( + `Security command failed: ${error.stderr?.toString().trim() || error.message}` + ); + } } // Type for the security executor function (for dependency injection in tests) diff --git a/src/keys-cli.ts b/src/keys-cli.ts index 45b6f51..9a84ef3 100644 --- a/src/keys-cli.ts +++ b/src/keys-cli.ts @@ -3,7 +3,7 @@ * CLI commands for API key management with beautiful modern UI */ -import { exec, spawn } from "child_process"; +import { execFile, spawn } from "child_process"; import { promises as fs, readFileSync } from "fs"; import inquirer from "inquirer"; import os from "os"; @@ -16,6 +16,8 @@ const { dirname, join } = path; import { getSpinner, loadUi } from "./utils/cli-env.js"; import { promptForApiKey } from "./utils/password-prompt.js"; +const execFileAsync = promisify(execFile); + // Get package version const packageJson = JSON.parse( readFileSync( @@ -471,17 +473,20 @@ export async function handleKeysCommand(): Promise { // Update Claude Code CLI registry if available try { - await promisify(exec)("claude --version"); + await execFileAsync("claude", ["--version"]); // Build config using existing helper (keeps local/npx logic consistent) const iterableMcpConfig = buildMcpConfig({ env: mcpEnv }); const configJson = JSON.stringify(iterableMcpConfig); // Remove existing registration (ignore errors) - await promisify(exec)("claude mcp remove iterable").catch( - () => {} - ); + await execFileAsync("claude", [ + "mcp", + "remove", + "iterable", + ]).catch(() => {}); + // Add new registration with inherited stdio to show Claude CLI output await new Promise((resolve, reject) => { const child = spawn( "claude", diff --git a/tests/unit/execfile-security.test.ts b/tests/unit/execfile-security.test.ts new file mode 100644 index 0000000..0d55f07 --- /dev/null +++ b/tests/unit/execfile-security.test.ts @@ -0,0 +1,32 @@ +/** + * Tests for execFile security refactoring + * Verifies that we use execFile instead of shell-spawning commands + */ + +import { describe, expect, it } from "@jest/globals"; + +import { findCommand } from "../../src/install.js"; + +describe("execFile Security", () => { + it("should find valid system commands without using shell", async () => { + // Test that findCommand works with a known command + // This verifies execFile is working correctly + const result = await findCommand("node"); + expect(result).toBeTruthy(); + expect(typeof result).toBe("string"); + expect(result.length).toBeGreaterThan(0); + }); + + it("should reject commands with shell metacharacters gracefully", async () => { + // Attempt to find a command with shell injection characters + // execFile should treat this as a literal command name (which won't exist) + await expect(findCommand("node; echo pwned")).rejects.toThrow("not found"); + }); + + it("should handle non-existent commands without shell execution", async () => { + // This should fail cleanly without any shell interpretation + await expect( + findCommand("this-command-does-not-exist-xyz") + ).rejects.toThrow("not found"); + }); +}); diff --git a/tests/unit/find-command.test.ts b/tests/unit/find-command.test.ts new file mode 100644 index 0000000..4bd79af --- /dev/null +++ b/tests/unit/find-command.test.ts @@ -0,0 +1,17 @@ +/* eslint-disable simple-import-sort/imports */ +import { describe, expect, it } from "@jest/globals"; +import { findCommand } from "../../src/install"; + +describe("findCommand", () => { + it("finds node command successfully", async () => { + const nodePath = await findCommand("node"); + expect(nodePath).toBeTruthy(); + expect(nodePath).toContain("node"); + }); + + it("throws error for non-existent command", async () => { + await expect( + findCommand("this-command-does-not-exist-xyz") + ).rejects.toThrow("not found"); + }); +});