From f4de6ef1ab398e18446955d27019b7bb8bc08b6b Mon Sep 17 00:00:00 2001
From: Gonzalo Riestra <gonzalo.riestra@shopify.com>
Date: Thu, 30 Apr 2026 09:54:57 +0200
Subject: [PATCH] =?UTF-8?q?=F0=9F=9B=A1=EF=B8=8F=20Sentinel:=20[CRITICAL]?=
 =?UTF-8?q?=20Fix=20command=20injection=20in=20treeKill=20utility?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

🚨 Severity: CRITICAL
💡 Vulnerability: Command injection on Windows systems.
🎯 Impact: An attacker could execute arbitrary commands by providing a malicious PID string.
🔧 Fix:
- Replaced the broken `Number.isNaN` PID validation with a robust regex check (`/^\d+$/`).
- Replaced `child_process.exec` with `child_process.spawn` for the Windows `taskkill` command to prevent shell interpretation of arguments.
- Added security comments explaining the fixes.
✅ Verification:
- Added a new unit test file `packages/cli-kit/src/public/node/tree-kill.test.ts` to verify the fix.
- Ran `pnpm lint` and `pnpm type-check` for `@shopify/cli-kit`.

Made-with: Cursor
---
 .../cli-kit/src/public/node/tree-kill.test.ts | 58 +++++++++++++++++++
 packages/cli-kit/src/public/node/tree-kill.ts | 21 +++++--
 2 files changed, 74 insertions(+), 5 deletions(-)
 create mode 100644 packages/cli-kit/src/public/node/tree-kill.test.ts

diff --git a/packages/cli-kit/src/public/node/tree-kill.test.ts b/packages/cli-kit/src/public/node/tree-kill.test.ts
new file mode 100644
index 00000000000..e70e3df2d7f
--- /dev/null
+++ b/packages/cli-kit/src/public/node/tree-kill.test.ts
@@ -0,0 +1,58 @@
+/* eslint-disable no-restricted-imports */
+import {treeKill} from './tree-kill.js'
+import {vi, describe, test, expect, afterEach} from 'vitest'
+import {spawn} from 'child_process'
+
+vi.mock('child_process', async () => {
+  const actual: any = await vi.importActual('child_process')
+  return {
+    ...actual,
+    spawn: vi.fn(),
+  }
+})
+
+describe('treeKill', () => {
+  afterEach(() => {
+    vi.unstubAllGlobals()
+  })
+
+  test('calls the callback with an error if the PID is not a number (string with digits)', async () => {
+    const maliciousPid = '1234; calc.exe'
+
+    await new Promise<void>((resolve) => {
+      // Correct signature for treeKill is (pid, signal, killRoot, callback)
+      treeKill(maliciousPid, 'SIGTERM', true, (err) => {
+        expect(err?.message).toBe('pid must be a number')
+        resolve()
+      })
+    })
+
+    expect(spawn).not.toHaveBeenCalled()
+  })
+
+  test('works with a valid numeric PID as string', () => {
+    const pid = '1234'
+    vi.mocked(spawn).mockReturnValue({
+      on: vi.fn(),
+      stdout: {on: vi.fn()},
+    } as any)
+    vi.stubGlobal('process', {...process, platform: 'win32'})
+
+    treeKill(pid)
+
+    expect(spawn).toHaveBeenCalledWith('taskkill', ['/pid', '1234', '/T', '/F'])
+  })
+
+  test('works with a valid numeric PID as number', () => {
+    const pid = 1234
+    vi.mocked(spawn).mockReturnValue({
+      on: vi.fn(),
+      stdout: {on: vi.fn()},
+    } as any)
+    vi.stubGlobal('process', {...process, platform: 'win32'})
+
+    treeKill(pid)
+
+    expect(spawn).toHaveBeenCalledWith('taskkill', ['/pid', '1234', '/T', '/F'])
+  })
+})
diff --git a/packages/cli-kit/src/public/node/tree-kill.ts b/packages/cli-kit/src/public/node/tree-kill.ts
index 1bd7bdee30d..30b8e86ce20 100644
--- a/packages/cli-kit/src/public/node/tree-kill.ts
+++ b/packages/cli-kit/src/public/node/tree-kill.ts
@@ -3,7 +3,7 @@
 /* eslint-disable no-restricted-imports */
 
 import {outputDebug} from './output.js'
-import {exec, spawn} from 'child_process'
+import {spawn} from 'child_process'
 
 type ProcessTree = Record<string, string[]>
 
@@ -52,7 +52,8 @@ function adaptedTreeKill(
 ): void {
   const rootPid = typeof pid === 'number' ? pid.toString() : pid
 
-  if (Number.isNaN(rootPid)) {
+  // Security: Validate that the PID is a number to prevent command injection
+  if (!/^\d+$/.test(rootPid)) {
     if (callback) {
       callback(new Error('pid must be a number'))
       return
@@ -71,10 +72,20 @@ function adaptedTreeKill(
 
   // eslint-disable-next-line @typescript-eslint/switch-exhaustiveness-check -- default handles all Unix-like platforms
   switch (process.platform) {
-    case 'win32':
-      // @ts-ignore
-      exec(`taskkill /pid ${pid} /T /F`, callback)
+    case 'win32': {
+      // Security: Use spawn instead of exec to avoid shell injection when killing processes on Windows
+      const taskkill = spawn('taskkill', ['/pid', rootPid, '/T', '/F'])
+      taskkill.on('close', (code: number) => {
+        if (callback) {
+          if (code === 0) {
+            callback()
+          } else {
+            callback(new Error(`taskkill exited with code ${code}`))
+          }
+        }
+      })
       break
+    }
     case 'darwin':
       buildProcessTree(
         rootPid,