From ed3a9b685730d8b7c53e515c5b5b94deb30b86fa Mon Sep 17 00:00:00 2001 From: knoxiboy Date: Wed, 29 Jul 2026 19:00:05 +0530 Subject: [PATCH 1/2] sec: prevent arbitrary command injection in terminal execution helpers (#3202) --- packages/data/src/hooks/_exec.test.ts | 11 +- packages/data/src/hooks/_exec.ts | 15 +- packages/jsx/src/hooks/useSubprocess.test.ts | 224 ++++++++++--------- packages/jsx/src/hooks/useSubprocess.ts | 7 +- 4 files changed, 146 insertions(+), 111 deletions(-) diff --git a/packages/data/src/hooks/_exec.test.ts b/packages/data/src/hooks/_exec.test.ts index 9ebbeabf6..c48943ce9 100644 --- a/packages/data/src/hooks/_exec.test.ts +++ b/packages/data/src/hooks/_exec.test.ts @@ -27,7 +27,7 @@ describe('execFileAsync', () => { stdout: 'hello stdout', stderr: 'hello stderr', }); - expect(spy).toHaveBeenCalledWith('test-bin', ['arg1'], { cwd: '/tmp' }, expect.any(Function)); + expect(spy).toHaveBeenCalledWith('test-bin', ['arg1'], { cwd: '/tmp', shell: false }, expect.any(Function)); }); it('should reject with error when execution fails', async () => { @@ -42,4 +42,13 @@ describe('execFileAsync', () => { await expect(execFileAsync('test-bin', [])).rejects.toThrow('Spawn failed'); }); + + it('should reject when file path contains null bytes', async () => { + await expect(execFileAsync('test-bin\0malicious', [])).rejects.toThrow('execFileAsync: file path contains null bytes'); + }); + + it('should reject when an argument contains null bytes', async () => { + await expect(execFileAsync('test-bin', ['safe', 'malicious\0arg'])).rejects.toThrow('execFileAsync: argument contains null bytes'); + }); }); + diff --git a/packages/data/src/hooks/_exec.ts b/packages/data/src/hooks/_exec.ts index 4cfbaa6d8..80c262abe 100644 --- a/packages/data/src/hooks/_exec.ts +++ b/packages/data/src/hooks/_exec.ts @@ -1,8 +1,21 @@ import { execFile } from 'node:child_process'; export const execFileAsync = (file: string, args: string[], opts?: any): Promise<{ stdout: string; stderr: string }> => { + if (!file || typeof file !== 'string') { + return Promise.reject(new TypeError('execFileAsync: file path must be a non-empty string')); + } + if (file.includes('\0')) { + return Promise.reject(new Error('execFileAsync: file path contains null bytes')); + } + if (Array.isArray(args)) { + for (const arg of args) { + if (typeof arg === 'string' && arg.includes('\0')) { + return Promise.reject(new Error('execFileAsync: argument contains null bytes')); + } + } + } return new Promise((resolve, reject) => { - execFile(file, args, opts, (err, stdout, stderr) => { + execFile(file, args, { shell: false, ...opts }, (err, stdout, stderr) => { if (err) reject(err); else resolve({ stdout: String(stdout), stderr: String(stderr) }); }); diff --git a/packages/jsx/src/hooks/useSubprocess.test.ts b/packages/jsx/src/hooks/useSubprocess.test.ts index f708ddc98..8c486d63a 100644 --- a/packages/jsx/src/hooks/useSubprocess.test.ts +++ b/packages/jsx/src/hooks/useSubprocess.test.ts @@ -1,108 +1,116 @@ -import { EventEmitter } from 'node:events'; -import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; -import { useSubprocess } from './useSubprocess.js'; -import { setCurrentApp } from '../runtime.js'; -import { spawn } from 'node:child_process'; - -vi.mock('node:child_process', () => ({ - spawn: vi.fn(), -})); - -const mockSpawn = vi.mocked(spawn); - -describe('useSubprocess', () => { - beforeEach(() => { - mockSpawn.mockReset(); - }); - - afterEach(() => { - setCurrentApp(null); - }); - - it('spawns a subprocess with inherited stdio and returns the exit code', async () => { - const proc = new EventEmitter(); - - mockSpawn.mockReturnValue(proc as any); - - const subprocess = useSubprocess(); - const promise = subprocess.run(['git', 'status']); - - proc.emit('close', 7); - - const code = await promise; - - expect(mockSpawn).toHaveBeenCalledWith('git', ['status'], { - stdio: 'inherit', - }); - expect(code).toBe(7); - }); - - it('exits raw mode before spawning and restores the TUI after exit', async () => { - const app = { - terminal: { - exitRawMode: vi.fn(), - enterRawMode: vi.fn(), - }, - screen: { - invalidate: vi.fn(), - }, - requestRender: vi.fn(), - } as any; - - setCurrentApp(app); - - const proc = new EventEmitter(); - mockSpawn.mockReturnValue(proc as any); - - const subprocess = useSubprocess(); - const promise = subprocess.run(['vim', 'file.txt']); - - expect(app.terminal.exitRawMode).toHaveBeenCalledOnce(); - - proc.emit('close', 0); - - const code = await promise; - - expect(app.terminal.enterRawMode).toHaveBeenCalledOnce(); - expect(app.screen.invalidate).toHaveBeenCalledOnce(); - expect(app.requestRender).toHaveBeenCalledOnce(); - expect(code).toBe(0); - }); - - it('restores raw mode and re-renders when the subprocess emits an error', async () => { - const app = { - terminal: { - exitRawMode: vi.fn(), - enterRawMode: vi.fn(), - }, - screen: { - invalidate: vi.fn(), - }, - requestRender: vi.fn(), - } as any; - - setCurrentApp(app); - - const proc = new EventEmitter(); - mockSpawn.mockReturnValue(proc as any); - - const subprocess = useSubprocess(); - const promise = subprocess.run(['bad-command']); - - proc.emit('error', new Error('spawn failed')); - - await expect(promise).rejects.toThrow('spawn failed'); - - expect(app.terminal.enterRawMode).toHaveBeenCalledOnce(); - expect(app.screen.invalidate).toHaveBeenCalledOnce(); - expect(app.requestRender).toHaveBeenCalledOnce(); - }); - - it('throws when command is empty', async () => { - const subprocess = useSubprocess(); - - await expect(subprocess.run([])).rejects.toThrow( - 'useSubprocess.run requires a command', - ); - }); -}); \ No newline at end of file +import { EventEmitter } from 'node:events'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { useSubprocess } from './useSubprocess.js'; +import { setCurrentApp } from '../runtime.js'; +import { spawn } from 'node:child_process'; + +vi.mock('node:child_process', () => ({ + spawn: vi.fn(), +})); + +const mockSpawn = vi.mocked(spawn); + +describe('useSubprocess', () => { + beforeEach(() => { + mockSpawn.mockReset(); + }); + + afterEach(() => { + setCurrentApp(null); + }); + + it('spawns a subprocess with inherited stdio and returns the exit code', async () => { + const proc = new EventEmitter(); + + mockSpawn.mockReturnValue(proc as any); + + const subprocess = useSubprocess(); + const promise = subprocess.run(['git', 'status']); + + proc.emit('close', 7); + + const code = await promise; + + expect(mockSpawn).toHaveBeenCalledWith('git', ['status'], { + stdio: 'inherit', + }); + expect(code).toBe(7); + }); + + it('exits raw mode before spawning and restores the TUI after exit', async () => { + const app = { + terminal: { + exitRawMode: vi.fn(), + enterRawMode: vi.fn(), + }, + screen: { + invalidate: vi.fn(), + }, + requestRender: vi.fn(), + } as any; + + setCurrentApp(app); + + const proc = new EventEmitter(); + mockSpawn.mockReturnValue(proc as any); + + const subprocess = useSubprocess(); + const promise = subprocess.run(['vim', 'file.txt']); + + expect(app.terminal.exitRawMode).toHaveBeenCalledOnce(); + + proc.emit('close', 0); + + const code = await promise; + + expect(app.terminal.enterRawMode).toHaveBeenCalledOnce(); + expect(app.screen.invalidate).toHaveBeenCalledOnce(); + expect(app.requestRender).toHaveBeenCalledOnce(); + expect(code).toBe(0); + }); + + it('restores raw mode and re-renders when the subprocess emits an error', async () => { + const app = { + terminal: { + exitRawMode: vi.fn(), + enterRawMode: vi.fn(), + }, + screen: { + invalidate: vi.fn(), + }, + requestRender: vi.fn(), + } as any; + + setCurrentApp(app); + + const proc = new EventEmitter(); + mockSpawn.mockReturnValue(proc as any); + + const subprocess = useSubprocess(); + const promise = subprocess.run(['bad-command']); + + proc.emit('error', new Error('spawn failed')); + + await expect(promise).rejects.toThrow('spawn failed'); + + expect(app.terminal.enterRawMode).toHaveBeenCalledOnce(); + expect(app.screen.invalidate).toHaveBeenCalledOnce(); + expect(app.requestRender).toHaveBeenCalledOnce(); + }); + + it('throws when command is empty', async () => { + const subprocess = useSubprocess(); + + await expect(subprocess.run([])).rejects.toThrow( + 'useSubprocess.run requires a command', + ); + }); + + it('throws when command contains null bytes', async () => { + const subprocess = useSubprocess(); + + await expect(subprocess.run(['ls', 'dir\0malicious'])).rejects.toThrow( + 'useSubprocess: command contains null bytes', + ); + }); +}); \ No newline at end of file diff --git a/packages/jsx/src/hooks/useSubprocess.ts b/packages/jsx/src/hooks/useSubprocess.ts index 422cb5e30..3c46e807e 100644 --- a/packages/jsx/src/hooks/useSubprocess.ts +++ b/packages/jsx/src/hooks/useSubprocess.ts @@ -21,9 +21,14 @@ function spawnProcess(cmd: string[]): Promise { export function useSubprocess(): UseSubprocessResult { async function run(cmd: string[]): Promise { - if (cmd.length === 0) { + if (!cmd || cmd.length === 0) { throw new Error('useSubprocess.run requires a command'); } + for (const part of cmd) { + if (typeof part === 'string' && part.includes('\0')) { + throw new Error('useSubprocess: command contains null bytes'); + } + } const app = getCurrentApp(); From e4fef258a087d064f6a46fa5dfdedd01f2cc4fb0 Mon Sep 17 00:00:00 2001 From: knoxiboy Date: Wed, 29 Jul 2026 23:21:09 +0530 Subject: [PATCH 2/2] sec: fix shell:false spread order and improve test type safety (#3202) - Fix critical spread order: change { shell: false, ...opts } to { ...opts, shell: false } so callers cannot override the shell: false security invariant - Add regression test that verifies shell: true in opts is always overridden - Replace raw 'any' casts in useSubprocess.test.ts with typed makeAppStub() factory using intentional 'unknown' cast (explained inline) - Replace 'proc as any' with ReturnType cast Addresses review comments from coderabbitai --- packages/data/src/hooks/_exec.test.ts | 16 ++ packages/data/src/hooks/_exec.ts | 2 +- packages/jsx/src/hooks/useSubprocess.test.ts | 233 ++++++++++--------- 3 files changed, 134 insertions(+), 117 deletions(-) diff --git a/packages/data/src/hooks/_exec.test.ts b/packages/data/src/hooks/_exec.test.ts index c48943ce9..84b8b6d3e 100644 --- a/packages/data/src/hooks/_exec.test.ts +++ b/packages/data/src/hooks/_exec.test.ts @@ -50,5 +50,21 @@ describe('execFileAsync', () => { it('should reject when an argument contains null bytes', async () => { await expect(execFileAsync('test-bin', ['safe', 'malicious\0arg'])).rejects.toThrow('execFileAsync: argument contains null bytes'); }); + + it('should always enforce shell: false even if caller passes shell: true in opts', async () => { + // Security invariant: caller must never be able to re-enable shell execution + const spy = vi.spyOn(childProcess, 'execFile').mockImplementation( + (file, args, opts, callback) => { + const cb = typeof opts === 'function' ? opts : callback; + cb(null, '', ''); + return {} as any; + } + ); + + // Even if caller attempts to pass shell: true, it must be overridden to false + await execFileAsync('test-bin', [], { shell: true }); + const calledOpts = spy.mock.calls[0][2] as { shell: boolean }; + expect(calledOpts.shell).toBe(false); + }); }); diff --git a/packages/data/src/hooks/_exec.ts b/packages/data/src/hooks/_exec.ts index 80c262abe..ae82f2b92 100644 --- a/packages/data/src/hooks/_exec.ts +++ b/packages/data/src/hooks/_exec.ts @@ -15,7 +15,7 @@ export const execFileAsync = (file: string, args: string[], opts?: any): Promise } } return new Promise((resolve, reject) => { - execFile(file, args, { shell: false, ...opts }, (err, stdout, stderr) => { + execFile(file, args, { ...opts, shell: false }, (err, stdout, stderr) => { if (err) reject(err); else resolve({ stdout: String(stdout), stderr: String(stderr) }); }); diff --git a/packages/jsx/src/hooks/useSubprocess.test.ts b/packages/jsx/src/hooks/useSubprocess.test.ts index 8c486d63a..2b14a5ef6 100644 --- a/packages/jsx/src/hooks/useSubprocess.test.ts +++ b/packages/jsx/src/hooks/useSubprocess.test.ts @@ -1,116 +1,117 @@ -import { EventEmitter } from 'node:events'; -import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; -import { useSubprocess } from './useSubprocess.js'; -import { setCurrentApp } from '../runtime.js'; -import { spawn } from 'node:child_process'; - -vi.mock('node:child_process', () => ({ - spawn: vi.fn(), -})); - -const mockSpawn = vi.mocked(spawn); - -describe('useSubprocess', () => { - beforeEach(() => { - mockSpawn.mockReset(); - }); - - afterEach(() => { - setCurrentApp(null); - }); - - it('spawns a subprocess with inherited stdio and returns the exit code', async () => { - const proc = new EventEmitter(); - - mockSpawn.mockReturnValue(proc as any); - - const subprocess = useSubprocess(); - const promise = subprocess.run(['git', 'status']); - - proc.emit('close', 7); - - const code = await promise; - - expect(mockSpawn).toHaveBeenCalledWith('git', ['status'], { - stdio: 'inherit', - }); - expect(code).toBe(7); - }); - - it('exits raw mode before spawning and restores the TUI after exit', async () => { - const app = { - terminal: { - exitRawMode: vi.fn(), - enterRawMode: vi.fn(), - }, - screen: { - invalidate: vi.fn(), - }, - requestRender: vi.fn(), - } as any; - - setCurrentApp(app); - - const proc = new EventEmitter(); - mockSpawn.mockReturnValue(proc as any); - - const subprocess = useSubprocess(); - const promise = subprocess.run(['vim', 'file.txt']); - - expect(app.terminal.exitRawMode).toHaveBeenCalledOnce(); - - proc.emit('close', 0); - - const code = await promise; - - expect(app.terminal.enterRawMode).toHaveBeenCalledOnce(); - expect(app.screen.invalidate).toHaveBeenCalledOnce(); - expect(app.requestRender).toHaveBeenCalledOnce(); - expect(code).toBe(0); - }); - - it('restores raw mode and re-renders when the subprocess emits an error', async () => { - const app = { - terminal: { - exitRawMode: vi.fn(), - enterRawMode: vi.fn(), - }, - screen: { - invalidate: vi.fn(), - }, - requestRender: vi.fn(), - } as any; - - setCurrentApp(app); - - const proc = new EventEmitter(); - mockSpawn.mockReturnValue(proc as any); - - const subprocess = useSubprocess(); - const promise = subprocess.run(['bad-command']); - - proc.emit('error', new Error('spawn failed')); - - await expect(promise).rejects.toThrow('spawn failed'); - - expect(app.terminal.enterRawMode).toHaveBeenCalledOnce(); - expect(app.screen.invalidate).toHaveBeenCalledOnce(); - expect(app.requestRender).toHaveBeenCalledOnce(); - }); - - it('throws when command is empty', async () => { - const subprocess = useSubprocess(); - - await expect(subprocess.run([])).rejects.toThrow( - 'useSubprocess.run requires a command', - ); - }); - - it('throws when command contains null bytes', async () => { - const subprocess = useSubprocess(); - - await expect(subprocess.run(['ls', 'dir\0malicious'])).rejects.toThrow( - 'useSubprocess: command contains null bytes', - ); - }); -}); \ No newline at end of file +import { EventEmitter } from 'node:events'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { useSubprocess } from './useSubprocess.js'; +import { setCurrentApp } from '../runtime.js'; +import { spawn } from 'node:child_process'; +import type { App } from '@termuijs/core'; + +vi.mock('node:child_process', () => ({ + spawn: vi.fn(), +})); + +const mockSpawn = vi.mocked(spawn); + +/** + * Create a minimal typed App stub for useSubprocess tests. + * We only stub the three properties that useSubprocess accesses: + * terminal.exitRawMode, terminal.enterRawMode, screen.invalidate, requestRender. + */ +function makeAppStub(): App { + return { + terminal: { + exitRawMode: vi.fn(), + enterRawMode: vi.fn(), + }, + screen: { + invalidate: vi.fn(), + }, + requestRender: vi.fn(), + } as unknown as App; // unknown-first cast is intentional: we supply a typed partial stub +} + +describe('useSubprocess', () => { + beforeEach(() => { + mockSpawn.mockReset(); + }); + + afterEach(() => { + setCurrentApp(null); + }); + + it('spawns a subprocess with inherited stdio and returns the exit code', async () => { + const proc = new EventEmitter(); + + mockSpawn.mockReturnValue(proc as ReturnType); + + const subprocess = useSubprocess(); + const promise = subprocess.run(['git', 'status']); + + proc.emit('close', 7); + + const code = await promise; + + expect(mockSpawn).toHaveBeenCalledWith('git', ['status'], { + stdio: 'inherit', + }); + expect(code).toBe(7); + }); + + it('exits raw mode before spawning and restores the TUI after exit', async () => { + const app = makeAppStub(); + + setCurrentApp(app); + + const proc = new EventEmitter(); + mockSpawn.mockReturnValue(proc as ReturnType); + + const subprocess = useSubprocess(); + const promise = subprocess.run(['vim', 'file.txt']); + + expect(app.terminal.exitRawMode).toHaveBeenCalledOnce(); + + proc.emit('close', 0); + + const code = await promise; + + expect(app.terminal.enterRawMode).toHaveBeenCalledOnce(); + expect(app.screen.invalidate).toHaveBeenCalledOnce(); + expect(app.requestRender).toHaveBeenCalledOnce(); + expect(code).toBe(0); + }); + + it('restores raw mode and re-renders when the subprocess emits an error', async () => { + const app = makeAppStub(); + + setCurrentApp(app); + + const proc = new EventEmitter(); + mockSpawn.mockReturnValue(proc as ReturnType); + + const subprocess = useSubprocess(); + const promise = subprocess.run(['bad-command']); + + proc.emit('error', new Error('spawn failed')); + + await expect(promise).rejects.toThrow('spawn failed'); + + expect(app.terminal.enterRawMode).toHaveBeenCalledOnce(); + expect(app.screen.invalidate).toHaveBeenCalledOnce(); + expect(app.requestRender).toHaveBeenCalledOnce(); + }); + + it('throws when command is empty', async () => { + const subprocess = useSubprocess(); + + await expect(subprocess.run([])).rejects.toThrow( + 'useSubprocess.run requires a command', + ); + }); + + it('throws when command contains null bytes', async () => { + const subprocess = useSubprocess(); + + await expect(subprocess.run(['ls', 'dir\0malicious'])).rejects.toThrow( + 'useSubprocess: command contains null bytes', + ); + }); +}); \ No newline at end of file