From 33dd9ae34719fc48eb5de76b8a7e37ee6fc2f037 Mon Sep 17 00:00:00 2001 From: Stefan de Vogelaere Date: Sun, 11 Jan 2026 16:37:05 +0100 Subject: [PATCH] fix: address nitpick feedback from PR #423 ## Security Fix (Command Injection) - Use `execFile` with argument arrays instead of string interpolation - Add `safeOpenInEditor` helper that properly handles `open -a` commands - Validate that worktreePath is an absolute path before execution - Prevents shell metacharacter injection attacks ## Shared Type Definition - Move `EditorInfo` interface to `@automaker/types` package - Server and UI now import from shared package to prevent drift - Re-export from use-available-editors.ts for convenience ## Remove Unused Code - Remove unused `defaultEditorName` prop from WorktreeActionsDropdown - Remove prop from WorktreeTab component interface - Remove useDefaultEditor hook usage from WorktreePanel - Export new hooks from hooks/index.ts Co-Authored-By: Claude Opus 4.5 --- .../routes/worktree/routes/open-in-editor.ts | 47 ++++++++++++++----- .../components/worktree-actions-dropdown.tsx | 2 - .../components/worktree-tab.tsx | 3 -- .../board-view/worktree-panel/hooks/index.ts | 2 +- .../hooks/use-available-editors.ts | 7 ++- .../worktree-panel/worktree-panel.tsx | 5 -- libs/types/src/editor.ts | 13 +++++ libs/types/src/index.ts | 3 ++ 8 files changed, 54 insertions(+), 28 deletions(-) create mode 100644 libs/types/src/editor.ts diff --git a/apps/server/src/routes/worktree/routes/open-in-editor.ts b/apps/server/src/routes/worktree/routes/open-in-editor.ts index 43053692..25652efd 100644 --- a/apps/server/src/routes/worktree/routes/open-in-editor.ts +++ b/apps/server/src/routes/worktree/routes/open-in-editor.ts @@ -4,18 +4,15 @@ */ import type { Request, Response } from 'express'; -import { exec } from 'child_process'; +import { exec, execFile } from 'child_process'; import { promisify } from 'util'; import { homedir } from 'os'; +import { isAbsolute } from 'path'; +import type { EditorInfo } from '@automaker/types'; import { getErrorMessage, logError } from '../common.js'; const execAsync = promisify(exec); - -// Editor detection with caching -interface EditorInfo { - name: string; - command: string; -} +const execFileAsync = promisify(execFile); let cachedEditor: EditorInfo | null = null; let cachedEditors: EditorInfo[] | null = null; @@ -177,6 +174,21 @@ export function createGetDefaultEditorHandler() { }; } +/** + * Safely execute an editor command with a path argument + * Uses execFile to prevent command injection + */ +async function safeOpenInEditor(command: string, targetPath: string): Promise { + // Handle 'open -a "AppPath"' style commands (macOS) + if (command.startsWith('open -a ')) { + const appPath = command.replace('open -a ', '').replace(/"/g, ''); + await execFileAsync('open', ['-a', appPath, targetPath]); + } else { + // Simple commands like 'code', 'cursor', 'zed', etc. + await execFileAsync(command, [targetPath]); + } +} + export function createOpenInEditorHandler() { return async (req: Request, res: Response): Promise => { try { @@ -193,6 +205,15 @@ export function createOpenInEditorHandler() { return; } + // Security: Validate that worktreePath is an absolute path + if (!isAbsolute(worktreePath)) { + res.status(400).json({ + success: false, + error: 'worktreePath must be an absolute path', + }); + return; + } + // Use specified editor command or detect default let editor: EditorInfo; if (editorCommand) { @@ -205,7 +226,7 @@ export function createOpenInEditorHandler() { } try { - await execAsync(`${editor.command} "${worktreePath}"`); + await safeOpenInEditor(editor.command, worktreePath); res.json({ success: true, result: { @@ -216,21 +237,21 @@ export function createOpenInEditorHandler() { } catch (editorError) { // If the detected editor fails, try opening in default file manager as fallback const platform = process.platform; - let openCommand: string; + let fallbackCommand: string; let fallbackName: string; if (platform === 'darwin') { - openCommand = `open "${worktreePath}"`; + fallbackCommand = 'open'; fallbackName = 'Finder'; } else if (platform === 'win32') { - openCommand = `explorer "${worktreePath}"`; + fallbackCommand = 'explorer'; fallbackName = 'Explorer'; } else { - openCommand = `xdg-open "${worktreePath}"`; + fallbackCommand = 'xdg-open'; fallbackName = 'File Manager'; } - await execAsync(openCommand); + await execFileAsync(fallbackCommand, [worktreePath]); res.json({ success: true, result: { diff --git a/apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx b/apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx index 8d404341..59fd8c6b 100644 --- a/apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx +++ b/apps/ui/src/components/views/board-view/worktree-panel/components/worktree-actions-dropdown.tsx @@ -34,7 +34,6 @@ import { getEditorIcon } from '@/components/icons/editor-icons'; interface WorktreeActionsDropdownProps { worktree: WorktreeInfo; isSelected: boolean; - defaultEditorName: string; aheadCount: number; behindCount: number; isPulling: boolean; @@ -60,7 +59,6 @@ interface WorktreeActionsDropdownProps { export function WorktreeActionsDropdown({ worktree, isSelected, - defaultEditorName, aheadCount, behindCount, isPulling, diff --git a/apps/ui/src/components/views/board-view/worktree-panel/components/worktree-tab.tsx b/apps/ui/src/components/views/board-view/worktree-panel/components/worktree-tab.tsx index 37fe23f4..353ca6b4 100644 --- a/apps/ui/src/components/views/board-view/worktree-panel/components/worktree-tab.tsx +++ b/apps/ui/src/components/views/board-view/worktree-panel/components/worktree-tab.tsx @@ -17,7 +17,6 @@ interface WorktreeTabProps { isActivating: boolean; isDevServerRunning: boolean; devServerInfo?: DevServerInfo; - defaultEditorName: string; branches: BranchInfo[]; filteredBranches: BranchInfo[]; branchFilter: string; @@ -58,7 +57,6 @@ export function WorktreeTab({ isActivating, isDevServerRunning, devServerInfo, - defaultEditorName, branches, filteredBranches, branchFilter, @@ -315,7 +313,6 @@ export function WorktreeTab({ ([]); diff --git a/apps/ui/src/components/views/board-view/worktree-panel/worktree-panel.tsx b/apps/ui/src/components/views/board-view/worktree-panel/worktree-panel.tsx index e0030d09..5df30c50 100644 --- a/apps/ui/src/components/views/board-view/worktree-panel/worktree-panel.tsx +++ b/apps/ui/src/components/views/board-view/worktree-panel/worktree-panel.tsx @@ -9,7 +9,6 @@ import { useDevServers, useBranches, useWorktreeActions, - useDefaultEditor, useRunningFeatures, } from './hooks'; import { WorktreeTab } from './components'; @@ -76,8 +75,6 @@ export function WorktreePanel({ fetchBranches, }); - const { defaultEditorName } = useDefaultEditor(); - const { hasRunningFeatures } = useRunningFeatures({ runningFeatureIds, features, @@ -192,7 +189,6 @@ export function WorktreePanel({ isActivating={isActivating} isDevServerRunning={isDevServerRunning(mainWorktree)} devServerInfo={getDevServerInfo(mainWorktree)} - defaultEditorName={defaultEditorName} branches={branches} filteredBranches={filteredBranches} branchFilter={branchFilter} @@ -247,7 +243,6 @@ export function WorktreePanel({ isActivating={isActivating} isDevServerRunning={isDevServerRunning(worktree)} devServerInfo={getDevServerInfo(worktree)} - defaultEditorName={defaultEditorName} branches={branches} filteredBranches={filteredBranches} branchFilter={branchFilter} diff --git a/libs/types/src/editor.ts b/libs/types/src/editor.ts new file mode 100644 index 00000000..c74e146c --- /dev/null +++ b/libs/types/src/editor.ts @@ -0,0 +1,13 @@ +/** + * Editor types for the "Open In" functionality + */ + +/** + * Information about an available code editor + */ +export interface EditorInfo { + /** Display name of the editor (e.g., "VS Code", "Cursor") */ + name: string; + /** CLI command or open command to launch the editor */ + command: string; +} diff --git a/libs/types/src/index.ts b/libs/types/src/index.ts index 259e251d..d9fb4eab 100644 --- a/libs/types/src/index.ts +++ b/libs/types/src/index.ts @@ -204,6 +204,9 @@ export type { // Port configuration export { STATIC_PORT, SERVER_PORT, RESERVED_PORTS } from './ports.js'; +// Editor types +export type { EditorInfo } from './editor.js'; + // Ideation types export type { IdeaCategory,