[Security] Fix command injection in treeKill on Windows

.jules/sentinel.md

@@ -0,0 +1,10 @@
+# Sentinel Security Learnings
+
+## Vulnerability: Command Injection via Shell Execution
+
+**Learning:** Using `child_process.exec` with user-controlled input (even if partially validated) can lead to command injection if the input contains shell metacharacters. In this case, `Number.isNaN()` was incorrectly used on a string, which does not perform type coercion and can return `false` for malicious strings like `"123; calc.exe"`.
+
+**Prevention:**
+1. Use `child_process.spawn` instead of `child_process.exec` whenever possible, as it does not spawn a shell by default and arguments are passed as an array.
+2. Perform strict input validation using regular expressions (e.g., `/^\d+$/` for numeric IDs) to ensure the input matches the expected format exactly.
+3. Always handle the `error` event on spawned processes to prevent unhandled exceptions.

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

@@ -0,0 +1,65 @@
+import {treeKill} from './tree-kill.js'
+import {exec, spawn} from 'child_process'
+import {describe, expect, test, vi, beforeEach, afterEach} from 'vitest'
+
+vi.mock('child_process', () => ({
+  exec: vi.fn(),
+  spawn: vi.fn(() => {
+    const mockProcess: any = {
+      stdout: {
+        on: vi.fn(),
+      },
+      on: vi.fn(),
+    }
+    mockProcess.on.mockReturnValue(mockProcess)
+    return mockProcess
+  }),
+}))
+
+describe('treeKill', () => {
+  let originalPlatform: string
+
+  beforeEach(() => {
+    originalPlatform = process.platform
+  })
+
+  afterEach(() => {
+    Object.defineProperty(process, 'platform', {
+      value: originalPlatform,
+    })
+    vi.clearAllMocks()
+  })
+
+  test('raises an error if the pid is not a number string', () => {
+    // Given
+    const pid = '123; calc.exe'
+    const callback = vi.fn()
+
+    // When
+    treeKill(pid, 'SIGTERM', true, callback)
+
+    // Then
+    expect(callback).toHaveBeenCalledWith(
+      expect.objectContaining({
+        message: 'pid must be a number',
+      }),
+    )
+    expect(exec).not.toHaveBeenCalled()
+    expect(spawn).not.toHaveBeenCalled()
+  })
+
+  test('on windows, it calls taskkill with the correct pid', () => {
+    // Given
+    Object.defineProperty(process, 'platform', {
+      value: 'win32',
+    })
+    const pid = 123
+    const callback = vi.fn()
+
+    // When
+    treeKill(pid, 'SIGTERM', true, callback)
+
+    // Then
+    expect(spawn).toHaveBeenCalledWith('taskkill', ['/pid', '123', '/T', '/F'])
+  })
+})

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

@@ -52,7 +52,7 @@ function adaptedTreeKill(
 ): void {
   const rootPid = typeof pid === 'number' ? pid.toString() : pid
 
-  if (Number.isNaN(rootPid)) {
+  if (!/^\d+$/.test(rootPid)) {
     if (callback) {
       callback(new Error('pid must be a number'))
       return
@@ -73,7 +73,19 @@ function adaptedTreeKill(
   switch (process.platform) {
     case 'win32':
       // @ts-ignore
-      exec(`taskkill /pid ${pid} /T /F`, callback)
+      spawn('taskkill', ['/pid', rootPid, '/T', '/F'])
+        .on('close', (code) => {
+          if (callback) {
+            if (code === 0) {
+              callback()
+            } else {
+              callback(new Error(`taskkill exited with code ${code}`))
+            }
+          }
+        })
+        .on('error', (err) => {
+          if (callback) callback(err)
+        })
       break
     case 'darwin':
       buildProcessTree(