Merge pull request #7430 from Shopify/sentinel/fix-tree-kill-command-injection-13735669651940213355

[Security] Fix command injection in treeKill utility

Author: gonzaloriestra

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'])
+  })
+})

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,