diff --git a/.jules/sentinel.md b/.jules/sentinel.md new file mode 100644 index 0000000000..198fa8132d --- /dev/null +++ b/.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. 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 0000000000..c28c701619 --- /dev/null +++ b/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']) + }) +}) diff --git a/packages/cli-kit/src/public/node/tree-kill.ts b/packages/cli-kit/src/public/node/tree-kill.ts index 1bd7bdee30..b624a77a97 100644 --- a/packages/cli-kit/src/public/node/tree-kill.ts +++ b/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(