From 5d68e75541e7f753c10731f81da12c29b3089919 Mon Sep 17 00:00:00 2001 From: Stefan de Vogelaere Date: Sun, 18 Jan 2026 15:38:54 +0100 Subject: [PATCH] fix: address PR review security and validation issues - Add runtime type check for worktreePath in open-in-terminal handler - Fix Windows Terminal detection using commandExists before spawn - Fix xterm shell injection by using sh -c with escapeShellArg - Use loose equality for null/undefined in useEffectiveDefaultTerminal - Consolidate duplicate imports from open-in-terminal.js --- apps/server/src/routes/worktree/index.ts | 2 +- .../worktree/routes/open-in-terminal.ts | 4 +- .../hooks/use-available-terminals.ts | 4 +- libs/platform/src/editor.ts | 52 ++++++++++--------- libs/platform/src/terminal.ts | 7 ++- 5 files changed, 39 insertions(+), 30 deletions(-) diff --git a/apps/server/src/routes/worktree/index.ts b/apps/server/src/routes/worktree/index.ts index 7074691d..854e5c60 100644 --- a/apps/server/src/routes/worktree/index.ts +++ b/apps/server/src/routes/worktree/index.ts @@ -29,8 +29,8 @@ import { createGetAvailableEditorsHandler, createRefreshEditorsHandler, } from './routes/open-in-editor.js'; -import { createOpenInTerminalHandler } from './routes/open-in-terminal.js'; import { + createOpenInTerminalHandler, createGetAvailableTerminalsHandler, createGetDefaultTerminalHandler, createRefreshTerminalsHandler, diff --git a/apps/server/src/routes/worktree/routes/open-in-terminal.ts b/apps/server/src/routes/worktree/routes/open-in-terminal.ts index 37ba5480..069325f3 100644 --- a/apps/server/src/routes/worktree/routes/open-in-terminal.ts +++ b/apps/server/src/routes/worktree/routes/open-in-terminal.ts @@ -32,10 +32,10 @@ export function createOpenInTerminalHandler() { worktreePath: string; }; - if (!worktreePath) { + if (!worktreePath || typeof worktreePath !== 'string') { res.status(400).json({ success: false, - error: 'worktreePath required', + error: 'worktreePath required and must be a string', }); return; } diff --git a/apps/ui/src/components/views/board-view/worktree-panel/hooks/use-available-terminals.ts b/apps/ui/src/components/views/board-view/worktree-panel/hooks/use-available-terminals.ts index c89573e1..b719183d 100644 --- a/apps/ui/src/components/views/board-view/worktree-panel/hooks/use-available-terminals.ts +++ b/apps/ui/src/components/views/board-view/worktree-panel/hooks/use-available-terminals.ts @@ -82,8 +82,8 @@ export function useEffectiveDefaultTerminal(terminals: TerminalInfo[]): Terminal const defaultTerminalId = useAppStore((s) => s.defaultTerminalId); return useMemo(() => { - // If user hasn't set a preference (null), they prefer integrated terminal - if (defaultTerminalId === null) { + // If user hasn't set a preference (null/undefined), they prefer integrated terminal + if (defaultTerminalId == null) { return null; } diff --git a/libs/platform/src/editor.ts b/libs/platform/src/editor.ts index 7f960747..5fd2a756 100644 --- a/libs/platform/src/editor.ts +++ b/libs/platform/src/editor.ts @@ -374,8 +374,9 @@ export async function openInTerminal(targetPath: string): Promise<{ terminalName await execFileAsync('osascript', ['-e', script]); return { terminalName: 'Terminal' }; } else if (isWindows) { - // Try Windows Terminal first - try { + // Try Windows Terminal first - check if it exists before trying to spawn + const hasWindowsTerminal = await commandExists('wt'); + if (hasWindowsTerminal) { return await new Promise((resolve, reject) => { const child: ChildProcess = spawn('wt', ['-d', targetPath], { shell: true, @@ -384,33 +385,32 @@ export async function openInTerminal(targetPath: string): Promise<{ terminalName }); child.unref(); - child.on('error', () => { - reject(new Error('Windows Terminal not available')); - }); - - setTimeout(() => resolve({ terminalName: 'Windows Terminal' }), 100); - }); - } catch { - // Fall back to cmd - return await new Promise((resolve, reject) => { - const child: ChildProcess = spawn( - 'cmd', - ['/c', 'start', 'cmd', '/k', `cd /d "${targetPath}"`], - { - shell: true, - stdio: 'ignore', - detached: true, - } - ); - child.unref(); - child.on('error', (err) => { reject(err); }); - setTimeout(() => resolve({ terminalName: 'Command Prompt' }), 100); + setTimeout(() => resolve({ terminalName: 'Windows Terminal' }), 100); }); } + // Fall back to cmd + return await new Promise((resolve, reject) => { + const child: ChildProcess = spawn( + 'cmd', + ['/c', 'start', 'cmd', '/k', `cd /d "${targetPath}"`], + { + shell: true, + stdio: 'ignore', + detached: true, + } + ); + child.unref(); + + child.on('error', (err) => { + reject(err); + }); + + setTimeout(() => resolve({ terminalName: 'Command Prompt' }), 100); + }); } else { // Linux: Try common terminal emulators in order const terminals = [ @@ -425,7 +425,11 @@ export async function openInTerminal(targetPath: string): Promise<{ terminalName command: 'xfce4-terminal', args: ['--working-directory', targetPath], }, - { name: 'xterm', command: 'xterm', args: ['-e', `cd "${targetPath}" && $SHELL`] }, + { + name: 'xterm', + command: 'xterm', + args: ['-e', 'sh', '-c', `cd ${escapeShellArg(targetPath)} && $SHELL`], + }, { name: 'x-terminal-emulator', command: 'x-terminal-emulator', diff --git a/libs/platform/src/terminal.ts b/libs/platform/src/terminal.ts index 17ee5ad6..4bbe120a 100644 --- a/libs/platform/src/terminal.ts +++ b/libs/platform/src/terminal.ts @@ -556,7 +556,12 @@ async function executeTerminalCommand(terminal: TerminalInfo, targetPath: string case 'xterm': // XTerm: uses -e to run a shell in the directory - await spawnDetached(command, ['-e', `cd "${targetPath}" && $SHELL`]); + await spawnDetached(command, [ + '-e', + 'sh', + '-c', + `cd ${escapeShellArg(targetPath)} && $SHELL`, + ]); break; default: