fix: address PR review comments

- Add nonce parameter to terminal navigation to allow reopening same
  worktree multiple times
- Fix shell path escaping in editor.ts using single-quote wrapper
- Add validatePathParams middleware to open-in-external-terminal route
- Remove redundant validation block from createOpenInExternalTerminalHandler
- Remove unused pendingTerminal state and setPendingTerminal action
- Remove unused getTerminalInfo function from editor.ts
This commit is contained in:
Stefan de Vogelaere
2026-01-17 23:09:23 +01:00
parent 111eb24856
commit 9529afbbaa
7 changed files with 36 additions and 81 deletions

View File

@@ -117,7 +117,11 @@ export function createWorktreeRoutes(
router.get('/available-terminals', createGetAvailableTerminalsHandler()); router.get('/available-terminals', createGetAvailableTerminalsHandler());
router.get('/default-terminal', createGetDefaultTerminalHandler()); router.get('/default-terminal', createGetDefaultTerminalHandler());
router.post('/refresh-terminals', createRefreshTerminalsHandler()); router.post('/refresh-terminals', createRefreshTerminalsHandler());
router.post('/open-in-external-terminal', createOpenInExternalTerminalHandler()); router.post(
'/open-in-external-terminal',
validatePathParams('worktreePath'),
createOpenInExternalTerminalHandler()
);
router.post('/init-git', validatePathParams('projectPath'), createInitGitHandler()); router.post('/init-git', validatePathParams('projectPath'), createInitGitHandler());
router.post('/migrate', createMigrateHandler()); router.post('/migrate', createMigrateHandler());

View File

@@ -144,42 +144,20 @@ export function createRefreshTerminalsHandler() {
export function createOpenInExternalTerminalHandler() { export function createOpenInExternalTerminalHandler() {
return async (req: Request, res: Response): Promise<void> => { return async (req: Request, res: Response): Promise<void> => {
try { try {
// worktreePath is validated by validatePathParams middleware
const { worktreePath, terminalId } = req.body as { const { worktreePath, terminalId } = req.body as {
worktreePath: string; worktreePath: string;
terminalId?: string; terminalId?: string;
}; };
if (!worktreePath) { const result = await openInExternalTerminal(worktreePath, terminalId);
res.status(400).json({ res.json({
success: false, success: true,
error: 'worktreePath required', result: {
}); message: `Opened ${worktreePath} in ${result.terminalName}`,
return; terminalName: result.terminalName,
} },
});
// 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;
}
try {
const result = await openInExternalTerminal(worktreePath, terminalId);
res.json({
success: true,
result: {
message: `Opened ${worktreePath} in ${result.terminalName}`,
terminalName: result.terminalName,
},
});
} catch (terminalError) {
// Terminal failed to open
logger.warn(`Failed to open in terminal: ${getErrorMessage(terminalError)}`);
throw terminalError;
}
} catch (error) { } catch (error) {
logError(error, 'Open in external terminal failed'); logError(error, 'Open in external terminal failed');
res.status(500).json({ success: false, error: getErrorMessage(error) }); res.status(500).json({ success: false, error: getErrorMessage(error) });

View File

@@ -131,9 +131,10 @@ export function useWorktreeActions({ fetchWorktrees, fetchBranches }: UseWorktre
(worktree: WorktreeInfo, mode?: 'tab' | 'split') => { (worktree: WorktreeInfo, mode?: 'tab' | 'split') => {
// Navigate to the terminal view with the worktree path and branch name // Navigate to the terminal view with the worktree path and branch name
// The terminal view will handle creating the terminal with the specified cwd // The terminal view will handle creating the terminal with the specified cwd
// Include nonce to allow opening the same worktree multiple times
navigate({ navigate({
to: '/terminal', to: '/terminal',
search: { cwd: worktree.path, branch: worktree.branch, mode }, search: { cwd: worktree.path, branch: worktree.branch, mode, nonce: Date.now() },
}); });
}, },
[navigate] [navigate]

View File

@@ -224,9 +224,11 @@ interface TerminalViewProps {
initialBranch?: string; initialBranch?: string;
/** Mode for opening terminal: 'tab' for new tab, 'split' for split in current tab */ /** Mode for opening terminal: 'tab' for new tab, 'split' for split in current tab */
initialMode?: 'tab' | 'split'; initialMode?: 'tab' | 'split';
/** Unique nonce to allow opening the same worktree multiple times */
nonce?: number;
} }
export function TerminalView({ initialCwd, initialBranch, initialMode }: TerminalViewProps) { export function TerminalView({ initialCwd, initialBranch, initialMode, nonce }: TerminalViewProps) {
const { const {
terminalState, terminalState,
setTerminalUnlocked, setTerminalUnlocked,
@@ -556,9 +558,9 @@ export function TerminalView({ initialCwd, initialBranch, initialMode }: Termina
// Skip if no initialCwd provided // Skip if no initialCwd provided
if (!initialCwd) return; if (!initialCwd) return;
// Skip if we've already handled this exact cwd (prevents duplicate terminals) // Skip if we've already handled this exact request (prevents duplicate terminals)
// Include mode in the key to allow opening same cwd with different modes // Include mode and nonce in the key to allow opening same cwd multiple times
const cwdKey = `${initialCwd}:${initialMode || 'default'}`; const cwdKey = `${initialCwd}:${initialMode || 'default'}:${nonce || 0}`;
if (initialCwdHandledRef.current === cwdKey) return; if (initialCwdHandledRef.current === cwdKey) return;
// Skip if terminal is not enabled or not unlocked // Skip if terminal is not enabled or not unlocked
@@ -632,6 +634,7 @@ export function TerminalView({ initialCwd, initialBranch, initialMode }: Termina
initialCwd, initialCwd,
initialBranch, initialBranch,
initialMode, initialMode,
nonce,
status?.enabled, status?.enabled,
status?.passwordRequired, status?.passwordRequired,
terminalState.isUnlocked, terminalState.isUnlocked,

View File

@@ -6,6 +6,7 @@ const terminalSearchSchema = z.object({
cwd: z.string().optional(), cwd: z.string().optional(),
branch: z.string().optional(), branch: z.string().optional(),
mode: z.enum(['tab', 'split']).optional(), mode: z.enum(['tab', 'split']).optional(),
nonce: z.number().optional(),
}); });
export const Route = createFileRoute('/terminal')({ export const Route = createFileRoute('/terminal')({
@@ -14,6 +15,6 @@ export const Route = createFileRoute('/terminal')({
}); });
function RouteComponent() { function RouteComponent() {
const { cwd, branch, mode } = Route.useSearch(); const { cwd, branch, mode, nonce } = Route.useSearch();
return <TerminalView initialCwd={cwd} initialBranch={branch} initialMode={mode} />; return <TerminalView initialCwd={cwd} initialBranch={branch} initialMode={mode} nonce={nonce} />;
} }

View File

@@ -531,7 +531,6 @@ export interface TerminalState {
lineHeight: number; // Line height multiplier for terminal text lineHeight: number; // Line height multiplier for terminal text
maxSessions: number; // Maximum concurrent terminal sessions (server setting) maxSessions: number; // Maximum concurrent terminal sessions (server setting)
lastActiveProjectPath: string | null; // Last project path to detect route changes vs project switches lastActiveProjectPath: string | null; // Last project path to detect route changes vs project switches
pendingTerminal: { cwd: string; branchName: string } | null; // Pending terminal to create (from "open in terminal" action)
openTerminalMode: 'newTab' | 'split'; // How to open terminals from "Open in Terminal" action openTerminalMode: 'newTab' | 'split'; // How to open terminals from "Open in Terminal" action
} }
@@ -1239,7 +1238,6 @@ export interface AppActions {
setTerminalLineHeight: (lineHeight: number) => void; setTerminalLineHeight: (lineHeight: number) => void;
setTerminalMaxSessions: (maxSessions: number) => void; setTerminalMaxSessions: (maxSessions: number) => void;
setTerminalLastActiveProjectPath: (projectPath: string | null) => void; setTerminalLastActiveProjectPath: (projectPath: string | null) => void;
setPendingTerminal: (pending: { cwd: string; branchName: string } | null) => void;
setOpenTerminalMode: (mode: 'newTab' | 'split') => void; setOpenTerminalMode: (mode: 'newTab' | 'split') => void;
addTerminalTab: (name?: string) => string; addTerminalTab: (name?: string) => string;
removeTerminalTab: (tabId: string) => void; removeTerminalTab: (tabId: string) => void;
@@ -1459,7 +1457,6 @@ const initialState: AppState = {
lineHeight: 1.0, lineHeight: 1.0,
maxSessions: 100, maxSessions: 100,
lastActiveProjectPath: null, lastActiveProjectPath: null,
pendingTerminal: null,
openTerminalMode: 'newTab', openTerminalMode: 'newTab',
}, },
terminalLayoutByProject: {}, terminalLayoutByProject: {},
@@ -2912,9 +2909,6 @@ export const useAppStore = create<AppState & AppActions>()((set, get) => ({
maxSessions: current.maxSessions, maxSessions: current.maxSessions,
// Preserve lastActiveProjectPath - it will be updated separately when needed // Preserve lastActiveProjectPath - it will be updated separately when needed
lastActiveProjectPath: current.lastActiveProjectPath, lastActiveProjectPath: current.lastActiveProjectPath,
// Preserve pendingTerminal - this is set by "open in terminal" action and should
// survive the clearTerminalState() call that happens during project switching
pendingTerminal: current.pendingTerminal,
// Preserve openTerminalMode - user preference // Preserve openTerminalMode - user preference
openTerminalMode: current.openTerminalMode, openTerminalMode: current.openTerminalMode,
}, },
@@ -3008,13 +3002,6 @@ export const useAppStore = create<AppState & AppActions>()((set, get) => ({
}); });
}, },
setPendingTerminal: (pending) => {
const current = get().terminalState;
set({
terminalState: { ...current, pendingTerminal: pending },
});
},
setOpenTerminalMode: (mode) => { setOpenTerminalMode: (mode) => {
const current = get().terminalState; const current = get().terminalState;
set({ set({

View File

@@ -19,6 +19,15 @@ const execFileAsync = promisify(execFile);
const isWindows = process.platform === 'win32'; const isWindows = process.platform === 'win32';
const isMac = process.platform === 'darwin'; const isMac = process.platform === 'darwin';
/**
* Escape a string for safe use in shell commands
* Handles paths with spaces, special characters, etc.
*/
function escapeShellArg(arg: string): string {
// Escape single quotes by ending the quoted string, adding escaped quote, and starting new quoted string
return `'${arg.replace(/'/g, "'\\''")}'`;
}
// Cache with TTL for editor detection // Cache with TTL for editor detection
let cachedEditors: EditorInfo[] | null = null; let cachedEditors: EditorInfo[] | null = null;
let cacheTimestamp: number = 0; let cacheTimestamp: number = 0;
@@ -342,34 +351,6 @@ export async function openInFileManager(targetPath: string): Promise<{ editorNam
return { editorName: fileManager.name }; return { editorName: fileManager.name };
} }
/**
* Get the platform-specific terminal information
*/
function getTerminalInfo(): { name: string; command: string; args: string[] } {
if (isMac) {
// On macOS, use Terminal.app with AppleScript to open in a specific directory
return {
name: 'Terminal',
command: 'open',
args: ['-a', 'Terminal'],
};
} else if (isWindows) {
// On Windows, use Windows Terminal if available, otherwise cmd
return {
name: 'Windows Terminal',
command: 'wt',
args: ['-d'],
};
} else {
// On Linux, try common terminal emulators in order of preference
return {
name: 'Terminal',
command: 'x-terminal-emulator',
args: ['--working-directory'],
};
}
}
/** /**
* Open a terminal in the specified directory * Open a terminal in the specified directory
* *
@@ -386,7 +367,7 @@ export async function openInTerminal(targetPath: string): Promise<{ terminalName
// Use AppleScript to open Terminal.app in the specified directory // Use AppleScript to open Terminal.app in the specified directory
const script = ` const script = `
tell application "Terminal" tell application "Terminal"
do script "cd ${targetPath.replace(/"/g, '\\"').replace(/\$/g, '\\$')}" do script "cd ${escapeShellArg(targetPath)}"
activate activate
end tell end tell
`; `;