[Security] Fix command injection in treeKill utility

.jules/sentinel.md

@@ -0,0 +1,6 @@
+# Sentinel Learnings
+
+## Command Injection in Windows `taskkill`
+**Vulnerability:** Use of `child_process.exec` with user-controlled (or improperly validated) input for `taskkill` on Windows allowed potential command injection.
+**Learning:** `child_process.exec` spawns a shell, making it susceptible to shell injection. `Number.isNaN()` does not perform type coercion on strings, so `Number.isNaN("abc")` is `false`, failing as a numeric validator.
+**Prevention:** Always use `child_process.spawn` with an arguments array to avoid shell interpretation. Use strict regex like `/^\d+$/` for numeric validation of strings. Always handle the `'error'` event on `spawn` to prevent process crashes.

packages/cli-kit/src/public/node/tree-kill.test.ts

@@ -0,0 +1,130 @@
+/* eslint-disable no-restricted-imports */
+import {treeKill} from './tree-kill.js'
+import * as output from './output.js'
+import {describe, expect, test, vi, afterEach} from 'vitest'
+import {spawn} from 'child_process'
+
+vi.mock('./output.js')
+vi.mock('child_process', () => ({
+  spawn: vi.fn(() => ({
+    stdout: {on: vi.fn()},
+    on: vi.fn(),
+  })),
+}))
+
+describe('treeKill', () => {
+  const originalPlatform = process.platform
+
+  afterEach(() => {
+    Object.defineProperty(process, 'platform', {
+      value: originalPlatform,
+    })
+  })
+
+  test('logs an error if pid is not a number and no callback is provided', () => {
+    // When
+    treeKill('abc')
+
+    // Then
+    expect(output.outputDebug).toHaveBeenCalledWith(
+      expect.stringContaining('Failed to kill process abc: Error: pid must be a number'),
+    )
+  })
+
+  test('calls callback with error if pid is not a number and callback is provided', () => {
+    // Given
+    const callback = vi.fn()
+
+    // When
+    treeKill('abc', 'SIGTERM', true, callback)
+
+    // Then
+    expect(callback).toHaveBeenCalledWith(expect.any(Error))
+    expect(callback.mock.calls[0]![0].message).toBe('pid must be a number')
+  })
+
+  test('calls taskkill on Windows', () => {
+    // Given
+    Object.defineProperty(process, 'platform', {
+      value: 'win32',
+    })
+    const pid = 1234
+    const callback = vi.fn()
+    const mockOn = vi.fn()
+    vi.mocked(spawn).mockReturnValue({
+      on: mockOn.mockReturnValue({
+        on: mockOn,
+      }),
+    } as any)
+
+    // When
+    treeKill(pid, 'SIGTERM', true, callback)
+
+    // Then
+    expect(spawn).toHaveBeenCalledWith('taskkill', ['/pid', '1234', '/T', '/F'])
+    expect(mockOn).toHaveBeenCalledWith('error', expect.any(Function))
+    expect(mockOn).toHaveBeenCalledWith('close', expect.any(Function))
+  })
+
+  test('calls callback with error if taskkill fails to spawn', () => {
+    // Given
+    Object.defineProperty(process, 'platform', {
+      value: 'win32',
+    })
+    const pid = 1234
+    const callback = vi.fn()
+    const mockOn = vi.fn()
+    vi.mocked(spawn).mockReturnValue({
+      on: mockOn.mockImplementation((event, cb) => {
+        if (event === 'error') {
+          cb(new Error('spawn ENOENT'))
+        }
+        return {on: mockOn}
+      }),
+    } as any)
+
+    // When
+    treeKill(pid, 'SIGTERM', true, callback)
+
+    // Then
+    expect(callback).toHaveBeenCalledWith(expect.objectContaining({message: 'spawn ENOENT'}))
+  })
+
+  test('calls pgrep on Darwin', () => {
+    // Given
+    Object.defineProperty(process, 'platform', {
+      value: 'darwin',
+    })
+    const pid = 1234
+    const mockSpawn = vi.mocked(spawn)
+    mockSpawn.mockReturnValue({
+      stdout: {on: vi.fn()},
+      on: vi.fn(),
+    } as any)
+
+    // When
+    treeKill(pid)
+
+    // Then
+    expect(spawn).toHaveBeenCalledWith('pgrep', ['-lfP', '1234'])
+  })
+
+  test('calls ps on other platforms', () => {
+    // Given
+    Object.defineProperty(process, 'platform', {
+      value: 'linux',
+    })
+    const pid = 1234
+    const mockSpawn = vi.mocked(spawn)
+    mockSpawn.mockReturnValue({
+      stdout: {on: vi.fn()},
+      on: vi.fn(),
+    } as any)
+
+    // When
+    treeKill(pid)
+
+    // Then
+    expect(spawn).toHaveBeenCalledWith('ps', ['-o', 'pid command', '--no-headers', '--ppid', '1234'])
+  })
+})

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,13 +52,10 @@ function adaptedTreeKill(
 ): void {
   const rootPid = typeof pid === 'number' ? pid.toString() : pid
 
-  if (Number.isNaN(rootPid)) {
-    if (callback) {
-      callback(new Error('pid must be a number'))
-      return
-    } else {
-      throw new Error('pid must be a number')
-    }
+  // PID must be strictly numeric to prevent command injection
+  if (!/^\d+$/.test(rootPid)) {
+    callback(new Error('pid must be a number'))
+    return
   }
 
   // A map from parent pid to an array of children pids
@@ -72,8 +69,19 @@ function adaptedTreeKill(
   // eslint-disable-next-line @typescript-eslint/switch-exhaustiveness-check -- default handles all Unix-like platforms
   switch (process.platform) {
     case 'win32':
+      // We use spawn instead of exec to avoid shell injection vulnerabilities.
       // @ts-ignore
-      exec(`taskkill /pid ${pid} /T /F`, callback)
+      spawn('taskkill', ['/pid', rootPid, '/T', '/F'])
+        .on('error', (err) => {
+          callback(err)
+        })
+        .on('close', (code) => {
+          if (code !== 0 && code !== null) {
+            callback(new Error(`taskkill exited with code ${code}`))
+          } else {
+            callback()
+          }
+        })
       break
     case 'darwin':
       buildProcessTree(