From ea39c8bf287f8a29a70a12c758e853ec1ea38238 Mon Sep 17 00:00:00 2001 From: A <258483684+la14-1@users.noreply.github.com> Date: Fri, 13 Feb 2026 08:34:04 -0800 Subject: [PATCH] fix: prevent command injection in update-check reExecWithArgs (#951) Replace execSync with execFileSync in reExecWithArgs() to prevent shell metacharacter injection via binary path. execFileSync bypasses the shell entirely, executing the binary directly with an argv array. The performAutoUpdate() call retains execSync since it legitimately needs a shell for piping (curl | bash). Fixes #950 Agent: security-auditor Co-authored-by: A <6723574+louisgv@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 (1M context) --- cli/src/__tests__/update-check.test.ts | 45 +++++++++++++++----------- cli/src/update-check.ts | 12 ++----- 2 files changed, 29 insertions(+), 28 deletions(-) diff --git a/cli/src/__tests__/update-check.test.ts b/cli/src/__tests__/update-check.test.ts index bffcec36..a0589d50 100644 --- a/cli/src/__tests__/update-check.test.ts +++ b/cli/src/__tests__/update-check.test.ts @@ -211,33 +211,37 @@ describe("update-check", () => { const fetchSpy = spyOn(global, "fetch").mockImplementation(mockFetch); const { executor } = await import("../update-check.js"); - const calls: string[] = []; + const execSyncCalls: string[] = []; const execSyncSpy = spyOn(executor, "execSync").mockImplementation((cmd: string) => { - calls.push(cmd); + execSyncCalls.push(cmd); }); + const execFileSyncSpy = spyOn(executor, "execFileSync").mockImplementation(() => {}); const { checkForUpdates } = await import("../update-check.js"); await checkForUpdates(); - // First call: install script, second call: re-exec with original args - expect(calls.length).toBe(2); - expect(calls[0]).toContain("install.sh"); - expect(calls[1]).toContain("spawn"); - expect(calls[1]).toContain("claude"); - expect(calls[1]).toContain("sprite"); + // execSync called once for the install script + expect(execSyncCalls.length).toBe(1); + expect(execSyncCalls[0]).toContain("install.sh"); + + // execFileSync called once for re-exec (no shell interpretation) + expect(execFileSyncSpy).toHaveBeenCalledTimes(1); + expect(execFileSyncSpy.mock.calls[0][0]).toContain("spawn"); + expect(execFileSyncSpy.mock.calls[0][1]).toEqual(["claude", "sprite"]); // Should show rerunning message const output = consoleErrorSpy.mock.calls.map((call) => call[0]).join("\n"); expect(output).toContain("Rerunning"); // Should set SPAWN_NO_UPDATE_CHECK=1 to prevent infinite loop - expect(execSyncSpy.mock.calls[1][1]).toHaveProperty("env"); - expect(execSyncSpy.mock.calls[1][1].env.SPAWN_NO_UPDATE_CHECK).toBe("1"); + expect(execFileSyncSpy.mock.calls[0][2]).toHaveProperty("env"); + expect(execFileSyncSpy.mock.calls[0][2].env.SPAWN_NO_UPDATE_CHECK).toBe("1"); expect(processExitSpy).toHaveBeenCalledWith(0); fetchSpy.mockRestore(); execSyncSpy.mockRestore(); + execFileSyncSpy.mockRestore(); process.argv = originalArgv; }); @@ -254,15 +258,12 @@ describe("update-check", () => { const fetchSpy = spyOn(global, "fetch").mockImplementation(mockFetch); const { executor } = await import("../update-check.js"); - let callCount = 0; - const execSyncSpy = spyOn(executor, "execSync").mockImplementation(() => { - callCount++; - if (callCount === 2) { - // Re-exec fails with exit code 1 - const err = new Error("Command failed") as Error & { status: number }; - err.status = 42; - throw err; - } + const execSyncSpy = spyOn(executor, "execSync").mockImplementation(() => {}); + const execFileSyncSpy = spyOn(executor, "execFileSync").mockImplementation(() => { + // Re-exec fails with exit code 42 + const err = new Error("Command failed") as Error & { status: number }; + err.status = 42; + throw err; }); const { checkForUpdates } = await import("../update-check.js"); @@ -273,6 +274,7 @@ describe("update-check", () => { fetchSpy.mockRestore(); execSyncSpy.mockRestore(); + execFileSyncSpy.mockRestore(); process.argv = originalArgv; }); @@ -293,6 +295,7 @@ describe("update-check", () => { const execSyncSpy = spyOn(executor, "execSync").mockImplementation((cmd: string) => { calls.push(cmd); }); + const execFileSyncSpy = spyOn(executor, "execFileSync").mockImplementation(() => {}); const { checkForUpdates } = await import("../update-check.js"); await checkForUpdates(); @@ -301,6 +304,9 @@ describe("update-check", () => { expect(calls.length).toBe(1); expect(calls[0]).toContain("install.sh"); + // execFileSync should NOT be called (no re-exec without args) + expect(execFileSyncSpy).not.toHaveBeenCalled(); + // Should show "Run your spawn command again" instead const output = consoleErrorSpy.mock.calls.map((call) => call[0]).join("\n"); expect(output).toContain("Run your spawn command again"); @@ -310,6 +316,7 @@ describe("update-check", () => { fetchSpy.mockRestore(); execSyncSpy.mockRestore(); + execFileSyncSpy.mockRestore(); process.argv = originalArgv; }); }); diff --git a/cli/src/update-check.ts b/cli/src/update-check.ts index 72406f7c..593edf6e 100644 --- a/cli/src/update-check.ts +++ b/cli/src/update-check.ts @@ -1,5 +1,5 @@ import "./unicode-detect.js"; // Ensure TERM is set before using symbols -import { execSync as nodeExecSync } from "child_process"; +import { execSync as nodeExecSync, execFileSync as nodeExecFileSync } from "child_process"; import pc from "picocolors"; import pkg from "../package.json" with { type: "json" }; import { RAW_BASE } from "./manifest.js"; @@ -9,6 +9,7 @@ const VERSION = pkg.version; // Internal executor for testability - can be replaced in tests export const executor = { execSync: (cmd: string, options?: any) => nodeExecSync(cmd, options), + execFileSync: (file: string, args: string[], options?: any) => nodeExecFileSync(file, args, options), }; // ── Constants ────────────────────────────────────────────────────────────────── @@ -52,11 +53,6 @@ function compareVersions(current: string, latest: string): boolean { return false; // Versions are equal } -/** Shell-quote a string for safe interpolation into a bash command */ -function shellQuote(s: string): string { - return `'${s.replace(/'/g, "'\\''")}'`; -} - /** Print boxed update banner to stderr */ function printUpdateBanner(latestVersion: string): void { const line1 = `Update available: v${VERSION} -> v${latestVersion}`; @@ -94,13 +90,11 @@ function reExecWithArgs(): void { } const binPath = process.argv[1] || "spawn"; - const cmd = [shellQuote(binPath), ...args.map(shellQuote)].join(" "); console.error(pc.dim(` Rerunning: spawn ${args.join(" ")}`)); console.error(); try { - executor.execSync(cmd, { + executor.execFileSync(binPath, args, { stdio: "inherit", - shell: "/bin/bash", env: { ...process.env, SPAWN_NO_UPDATE_CHECK: "1" }, }); process.exit(0);