mirror of
https://github.com/AutoMaker-Org/automaker.git
synced 2026-01-30 06:12:03 +00:00
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 <noreply@anthropic.com>
This commit is contained in:
@@ -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<void> {
|
||||
// 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<void> => {
|
||||
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: {
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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({
|
||||
<WorktreeActionsDropdown
|
||||
worktree={worktree}
|
||||
isSelected={isSelected}
|
||||
defaultEditorName={defaultEditorName}
|
||||
aheadCount={aheadCount}
|
||||
behindCount={behindCount}
|
||||
isPulling={isPulling}
|
||||
|
||||
@@ -2,5 +2,5 @@ export { useWorktrees } from './use-worktrees';
|
||||
export { useDevServers } from './use-dev-servers';
|
||||
export { useBranches } from './use-branches';
|
||||
export { useWorktreeActions } from './use-worktree-actions';
|
||||
export { useDefaultEditor } from './use-default-editor';
|
||||
export { useRunningFeatures } from './use-running-features';
|
||||
export { useAvailableEditors, useEffectiveDefaultEditor } from './use-available-editors';
|
||||
|
||||
@@ -2,13 +2,12 @@ import { useState, useEffect, useCallback, useMemo } from 'react';
|
||||
import { createLogger } from '@automaker/utils/logger';
|
||||
import { getElectronAPI } from '@/lib/electron';
|
||||
import { useAppStore } from '@/store/app-store';
|
||||
import type { EditorInfo } from '@automaker/types';
|
||||
|
||||
const logger = createLogger('AvailableEditors');
|
||||
|
||||
export interface EditorInfo {
|
||||
name: string;
|
||||
command: string;
|
||||
}
|
||||
// Re-export EditorInfo for convenience
|
||||
export type { EditorInfo };
|
||||
|
||||
export function useAvailableEditors() {
|
||||
const [editors, setEditors] = useState<EditorInfo[]>([]);
|
||||
|
||||
@@ -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}
|
||||
|
||||
Reference in New Issue
Block a user