From 64ba4cdd3e04950219c584bd8d8f595536070c66 Mon Sep 17 00:00:00 2001 From: Andrew Boni Date: Tue, 18 Nov 2025 17:05:40 -0800 Subject: [PATCH 1/5] Use spawn over exec --- src/install.ts | 48 +++++++++++++++++++++++++++------ tests/unit/find-command.test.ts | 17 ++++++++++++ 2 files changed, 57 insertions(+), 8 deletions(-) create mode 100644 tests/unit/find-command.test.ts diff --git a/src/install.ts b/src/install.ts index 6fc6b88..f31fce8 100644 --- a/src/install.ts +++ b/src/install.ts @@ -1,12 +1,11 @@ /* eslint-disable no-console */ -import { exec, spawn } from "child_process"; +import { spawn } from "child_process"; import { existsSync, readFileSync } from "fs"; import inquirer from "inquirer"; import os from "os"; import path from "path"; import { fileURLToPath } from "url"; -import { promisify } from "util"; const { dirname, join } = path; @@ -61,12 +60,39 @@ const TOOL_CONFIGS = { } as const; // Cross-platform command finder -const findCommand = async (command: string): Promise => { +// Uses spawn() 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]!; + + return new Promise((resolve, reject) => { + const child = spawn(finder, [command], { + stdio: ["ignore", "pipe", "pipe"], + }); + + let stdout = ""; + + child.stdout.on("data", (data) => { + stdout += data.toString(); + }); + + child.on("error", (error) => { + reject(new Error(`Failed to find command: ${error.message}`)); + }); + + child.on("close", (code) => { + if (code === 0) { + const lines = stdout.trim().split("\n"); + if (!lines?.[0]) { + reject(new Error(`${command} not found`)); + } else { + resolve(lines[0]!); + } + } else { + reject(new Error(`${command} not found`)); + } + }); + }); }; // Auto-detect if running from a local development build @@ -947,7 +973,13 @@ export const setupMcpServer = async (): Promise => { // Remove existing config try { - await promisify(exec)("claude mcp remove iterable"); + await new Promise((resolve) => { + const child = spawn("claude", ["mcp", "remove", "iterable"], { + stdio: "ignore", + }); + child.on("close", () => resolve()); + child.on("error", () => resolve()); // Ignore errors + }); } catch { // Ignore errors } 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"); + }); +}); From 55a69c8f2aa51f93ada87488708554e73d912a04 Mon Sep 17 00:00:00 2001 From: Andrew Boni Date: Tue, 18 Nov 2025 18:42:52 -0800 Subject: [PATCH 2/5] Use execFile instead of spawn --- TOOLS.md | 4 +- src/install.ts | 45 ++++++++--------------- src/key-manager.ts | 47 +++++++----------------- src/keys-cli.ts | 8 ++-- src/tool-filter.ts | 2 - tests/unit/execfile-security.test.ts | 35 ++++++++++++++++++ tests/unit/preview-tools-pii.test.ts | 55 ++++++++++++++++++++++++++++ 7 files changed, 125 insertions(+), 71 deletions(-) create mode 100644 tests/unit/execfile-security.test.ts create mode 100644 tests/unit/preview-tools-pii.test.ts diff --git a/TOOLS.md b/TOOLS.md index 73a2758..5b53918 100644 --- a/TOOLS.md +++ b/TOOLS.md @@ -101,8 +101,8 @@ - **get_sms_template**: Get details for specific SMS template by ID - **get_template_by_client_id**: Get template by client template ID - **get_templates**: Retrieve templates -- **preview_email_template**: Preview email template with custom data. Returns fully rendered HTML with user, event, and/or data feed data substituted. -- **preview_inapp_template**: Preview in-app message template with custom data. Returns fully rendered HTML with user, event, and/or data feed data substituted. +- **preview_email_template** 🔒: Preview email template with custom data. Returns fully rendered HTML with user, event, and/or data feed data substituted. +- **preview_inapp_template** 🔒: Preview in-app message template with custom data. Returns fully rendered HTML with user, event, and/or data feed data substituted. - **send_email_template_proof** 🔒✏️: Send a proof of a email template to a specific user - **send_inapp_template_proof** 🔒✏️: Send a proof of a in-app message template to a specific user - **send_push_template_proof** 🔒✏️: Send a proof of a push notification template to a specific user diff --git a/src/install.ts b/src/install.ts index f31fce8..be3f0d9 100644 --- a/src/install.ts +++ b/src/install.ts @@ -1,11 +1,12 @@ /* eslint-disable no-console */ -import { spawn } from "child_process"; +import { execFile, spawn } from "child_process"; import { existsSync, readFileSync } from "fs"; import inquirer from "inquirer"; import os from "os"; import path from "path"; import { fileURLToPath } from "url"; +import { promisify } from "util"; const { dirname, join } = path; @@ -59,40 +60,24 @@ const TOOL_CONFIGS = { cursor: path.join(os.homedir(), ".cursor", "mcp.json"), } as const; +const execFileAsync = promisify(execFile); + // Cross-platform command finder -// Uses spawn() to prevent shell injection vulnerabilities +// Uses execFile to prevent shell injection vulnerabilities // Exported for testing export const findCommand = async (command: string): Promise => { const finder = process.platform === "win32" ? "where" : "which"; - return new Promise((resolve, reject) => { - const child = spawn(finder, [command], { - stdio: ["ignore", "pipe", "pipe"], - }); - - let stdout = ""; - - child.stdout.on("data", (data) => { - stdout += data.toString(); - }); - - child.on("error", (error) => { - reject(new Error(`Failed to find command: ${error.message}`)); - }); - - child.on("close", (code) => { - if (code === 0) { - const lines = stdout.trim().split("\n"); - if (!lines?.[0]) { - reject(new Error(`${command} not found`)); - } else { - resolve(lines[0]!); - } - } else { - reject(new Error(`${command} not found`)); - } - }); - }); + 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 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..2fb2c77 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,14 +473,14 @@ 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( () => {} ); diff --git a/src/tool-filter.ts b/src/tool-filter.ts index 743d48e..2b0a226 100644 --- a/src/tool-filter.ts +++ b/src/tool-filter.ts @@ -48,8 +48,6 @@ export const NON_PII_TOOLS: Set = new Set([ "get_user_fields", "get_webhooks", "partial_update_catalog_item", - "preview_email_template", - "preview_inapp_template", "replace_catalog_item", "schedule_campaign", "send_campaign", diff --git a/tests/unit/execfile-security.test.ts b/tests/unit/execfile-security.test.ts new file mode 100644 index 0000000..2bf60eb --- /dev/null +++ b/tests/unit/execfile-security.test.ts @@ -0,0 +1,35 @@ +/** + * 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/preview-tools-pii.test.ts b/tests/unit/preview-tools-pii.test.ts new file mode 100644 index 0000000..1205e13 --- /dev/null +++ b/tests/unit/preview-tools-pii.test.ts @@ -0,0 +1,55 @@ +/** + * Tests for preview tools PII restriction fix + * Verifies that preview tools require PII permissions + */ + +import { describe, expect, it } from "@jest/globals"; + +import type { McpServerConfig } from "../../src/config.js"; +import { filterTools, NON_PII_TOOLS } from "../../src/tool-filter.js"; +import { createAllTools } from "../../src/tools/index.js"; + +const mockClient = {} as any; + +describe("Preview Tools PII Restriction", () => { + const allTools = createAllTools(mockClient); + + it("should NOT include preview_email_template in NON_PII_TOOLS", () => { + expect(NON_PII_TOOLS.has("preview_email_template")).toBe(false); + }); + + it("should NOT include preview_inapp_template in NON_PII_TOOLS", () => { + expect(NON_PII_TOOLS.has("preview_inapp_template")).toBe(false); + }); + + it("should block preview tools when PII is disabled", () => { + const noPiiConfig: McpServerConfig = { + allowUserPii: false, + allowWrites: true, + allowSends: true, + }; + + const filteredTools = filterTools(allTools, noPiiConfig); + const toolNames = filteredTools.map((t) => t.name); + + // Preview tools should be blocked when PII is disabled + expect(toolNames).not.toContain("preview_email_template"); + expect(toolNames).not.toContain("preview_inapp_template"); + }); + + it("should allow preview tools when PII is enabled", () => { + const piiEnabledConfig: McpServerConfig = { + allowUserPii: true, + allowWrites: false, + allowSends: false, + }; + + const filteredTools = filterTools(allTools, piiEnabledConfig); + const toolNames = filteredTools.map((t) => t.name); + + // Preview tools should be available when PII is enabled + expect(toolNames).toContain("preview_email_template"); + expect(toolNames).toContain("preview_inapp_template"); + }); +}); + From 2bfb9f8150b8c98b24d1eeb30787385bfcc88516 Mon Sep 17 00:00:00 2001 From: Andrew Boni Date: Tue, 18 Nov 2025 18:59:24 -0800 Subject: [PATCH 3/5] Use execFile --- src/install.ts | 16 ++++------------ src/keys-cli.ts | 1 + 2 files changed, 5 insertions(+), 12 deletions(-) diff --git a/src/install.ts b/src/install.ts index be3f0d9..480a98f 100644 --- a/src/install.ts +++ b/src/install.ts @@ -956,18 +956,10 @@ export const setupMcpServer = async (): Promise => { const configJson = JSON.stringify(iterableMcpConfig); - // Remove existing config - try { - await new Promise((resolve) => { - const child = spawn("claude", ["mcp", "remove", "iterable"], { - stdio: "ignore", - }); - child.on("close", () => resolve()); - child.on("error", () => resolve()); // Ignore errors - }); - } catch { - // Ignore errors - } + // Remove existing config (ignore errors) + await execFileAsync("claude", ["mcp", "remove", "iterable"]).catch( + () => {} + ); spinner.start("Configuring Claude Code..."); diff --git a/src/keys-cli.ts b/src/keys-cli.ts index 2fb2c77..f52d47d 100644 --- a/src/keys-cli.ts +++ b/src/keys-cli.ts @@ -484,6 +484,7 @@ export async function handleKeysCommand(): Promise { () => {} ); + // Add new registration with inherited stdio to show Claude CLI output await new Promise((resolve, reject) => { const child = spawn( "claude", From d64d0a84bda1645ce5d709f957ce40014fb0f261 Mon Sep 17 00:00:00 2001 From: Andrew Boni Date: Tue, 18 Nov 2025 19:15:03 -0800 Subject: [PATCH 4/5] Put preview email and inapp templates back in NON_PII_TOOLS --- TOOLS.md | 4 +- src/tool-filter.ts | 2 + tests/unit/preview-tools-pii.test.ts | 55 ---------------------------- 3 files changed, 4 insertions(+), 57 deletions(-) delete mode 100644 tests/unit/preview-tools-pii.test.ts diff --git a/TOOLS.md b/TOOLS.md index 5b53918..73a2758 100644 --- a/TOOLS.md +++ b/TOOLS.md @@ -101,8 +101,8 @@ - **get_sms_template**: Get details for specific SMS template by ID - **get_template_by_client_id**: Get template by client template ID - **get_templates**: Retrieve templates -- **preview_email_template** 🔒: Preview email template with custom data. Returns fully rendered HTML with user, event, and/or data feed data substituted. -- **preview_inapp_template** 🔒: Preview in-app message template with custom data. Returns fully rendered HTML with user, event, and/or data feed data substituted. +- **preview_email_template**: Preview email template with custom data. Returns fully rendered HTML with user, event, and/or data feed data substituted. +- **preview_inapp_template**: Preview in-app message template with custom data. Returns fully rendered HTML with user, event, and/or data feed data substituted. - **send_email_template_proof** 🔒✏️: Send a proof of a email template to a specific user - **send_inapp_template_proof** 🔒✏️: Send a proof of a in-app message template to a specific user - **send_push_template_proof** 🔒✏️: Send a proof of a push notification template to a specific user diff --git a/src/tool-filter.ts b/src/tool-filter.ts index 2b0a226..743d48e 100644 --- a/src/tool-filter.ts +++ b/src/tool-filter.ts @@ -48,6 +48,8 @@ export const NON_PII_TOOLS: Set = new Set([ "get_user_fields", "get_webhooks", "partial_update_catalog_item", + "preview_email_template", + "preview_inapp_template", "replace_catalog_item", "schedule_campaign", "send_campaign", diff --git a/tests/unit/preview-tools-pii.test.ts b/tests/unit/preview-tools-pii.test.ts deleted file mode 100644 index 1205e13..0000000 --- a/tests/unit/preview-tools-pii.test.ts +++ /dev/null @@ -1,55 +0,0 @@ -/** - * Tests for preview tools PII restriction fix - * Verifies that preview tools require PII permissions - */ - -import { describe, expect, it } from "@jest/globals"; - -import type { McpServerConfig } from "../../src/config.js"; -import { filterTools, NON_PII_TOOLS } from "../../src/tool-filter.js"; -import { createAllTools } from "../../src/tools/index.js"; - -const mockClient = {} as any; - -describe("Preview Tools PII Restriction", () => { - const allTools = createAllTools(mockClient); - - it("should NOT include preview_email_template in NON_PII_TOOLS", () => { - expect(NON_PII_TOOLS.has("preview_email_template")).toBe(false); - }); - - it("should NOT include preview_inapp_template in NON_PII_TOOLS", () => { - expect(NON_PII_TOOLS.has("preview_inapp_template")).toBe(false); - }); - - it("should block preview tools when PII is disabled", () => { - const noPiiConfig: McpServerConfig = { - allowUserPii: false, - allowWrites: true, - allowSends: true, - }; - - const filteredTools = filterTools(allTools, noPiiConfig); - const toolNames = filteredTools.map((t) => t.name); - - // Preview tools should be blocked when PII is disabled - expect(toolNames).not.toContain("preview_email_template"); - expect(toolNames).not.toContain("preview_inapp_template"); - }); - - it("should allow preview tools when PII is enabled", () => { - const piiEnabledConfig: McpServerConfig = { - allowUserPii: true, - allowWrites: false, - allowSends: false, - }; - - const filteredTools = filterTools(allTools, piiEnabledConfig); - const toolNames = filteredTools.map((t) => t.name); - - // Preview tools should be available when PII is enabled - expect(toolNames).toContain("preview_email_template"); - expect(toolNames).toContain("preview_inapp_template"); - }); -}); - From 3317a6064778aff91096dffebee46ec52e514288 Mon Sep 17 00:00:00 2001 From: Andrew Boni Date: Tue, 18 Nov 2025 19:17:29 -0800 Subject: [PATCH 5/5] Fix formatting --- src/keys-cli.ts | 8 +++++--- tests/unit/execfile-security.test.ts | 11 ++++------- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/src/keys-cli.ts b/src/keys-cli.ts index f52d47d..9a84ef3 100644 --- a/src/keys-cli.ts +++ b/src/keys-cli.ts @@ -480,9 +480,11 @@ export async function handleKeysCommand(): Promise { const configJson = JSON.stringify(iterableMcpConfig); // Remove existing registration (ignore errors) - await execFileAsync("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) => { diff --git a/tests/unit/execfile-security.test.ts b/tests/unit/execfile-security.test.ts index 2bf60eb..0d55f07 100644 --- a/tests/unit/execfile-security.test.ts +++ b/tests/unit/execfile-security.test.ts @@ -20,16 +20,13 @@ describe("execFile Security", () => { 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" - ); + 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" - ); + await expect( + findCommand("this-command-does-not-exist-xyz") + ).rejects.toThrow("not found"); }); }); -