From 2de3ae69d4522ee3813f90bbfb7d5e1e333f3da3 Mon Sep 17 00:00:00 2001 From: Shirone Date: Sun, 25 Jan 2026 21:02:53 +0100 Subject: [PATCH] fix: Address CodeRabbit security and robustness review comments - Guard against NaN ports from non-numeric env variables in constants.ts - Validate IPC sender before returning API key to prevent leaking to untrusted senders (webviews, additional windows) - Filter dialog properties to maintain file-only intent and prevent renderer from requesting directories via OPEN_FILE - Fix Windows VS Code URL paths by ensuring leading slash after 'file' Co-Authored-By: Claude Opus 4.5 --- apps/ui/src/electron/constants.ts | 7 +++-- apps/ui/src/electron/ipc/auth-handlers.ts | 7 ++++- apps/ui/src/electron/ipc/dialog-handlers.ts | 29 ++++++++++++++------- apps/ui/src/electron/ipc/shell-handlers.ts | 8 +++--- 4 files changed, 34 insertions(+), 17 deletions(-) diff --git a/apps/ui/src/electron/constants.ts b/apps/ui/src/electron/constants.ts index fffca657..7039857b 100644 --- a/apps/ui/src/electron/constants.ts +++ b/apps/ui/src/electron/constants.ts @@ -22,8 +22,11 @@ export const DEFAULT_HEIGHT = 950; // When launched via root init.mjs we pass: // - PORT (backend) // - TEST_PORT (vite dev server / static) -export const DEFAULT_SERVER_PORT = parseInt(process.env.PORT || '3008', 10); -export const DEFAULT_STATIC_PORT = parseInt(process.env.TEST_PORT || '3007', 10); +// Guard against NaN from non-numeric environment variables +const parsedServerPort = Number.parseInt(process.env.PORT ?? '', 10); +const parsedStaticPort = Number.parseInt(process.env.TEST_PORT ?? '', 10); +export const DEFAULT_SERVER_PORT = Number.isFinite(parsedServerPort) ? parsedServerPort : 3008; +export const DEFAULT_STATIC_PORT = Number.isFinite(parsedStaticPort) ? parsedStaticPort : 3007; // ============================================ // File names for userData storage diff --git a/apps/ui/src/electron/ipc/auth-handlers.ts b/apps/ui/src/electron/ipc/auth-handlers.ts index ea05ac18..c10fc94d 100644 --- a/apps/ui/src/electron/ipc/auth-handlers.ts +++ b/apps/ui/src/electron/ipc/auth-handlers.ts @@ -14,7 +14,12 @@ import { state } from '../state'; export function registerAuthHandlers(): void { // Get API key for authentication // Returns null in external server mode to trigger session-based auth - ipcMain.handle(IPC_CHANNELS.AUTH.GET_API_KEY, () => { + // Only returns API key to the main window to prevent leaking to untrusted senders + ipcMain.handle(IPC_CHANNELS.AUTH.GET_API_KEY, (event) => { + // Validate sender is the main window + if (event.sender !== state.mainWindow?.webContents) { + return null; + } if (state.isExternalServerMode) { return null; } diff --git a/apps/ui/src/electron/ipc/dialog-handlers.ts b/apps/ui/src/electron/ipc/dialog-handlers.ts index 1b27ed0f..b5217bde 100644 --- a/apps/ui/src/electron/ipc/dialog-handlers.ts +++ b/apps/ui/src/electron/ipc/dialog-handlers.ts @@ -31,7 +31,7 @@ export function registerDialogHandlers(): void { ? `The selected directory is not allowed. Please select a directory within: ${allowedRoot}` : 'The selected directory is not allowed.'; - await dialog.showErrorBox('Directory Not Allowed', errorMessage); + dialog.showErrorBox('Directory Not Allowed', errorMessage); return { canceled: true, filePaths: [] }; } @@ -41,16 +41,25 @@ export function registerDialogHandlers(): void { }); // Open file dialog - ipcMain.handle(IPC_CHANNELS.DIALOG.OPEN_FILE, async (_, options = {}) => { - if (!state.mainWindow) { - return { canceled: true, filePaths: [] }; + // Filter properties to maintain file-only intent and prevent renderer from requesting directories + ipcMain.handle( + IPC_CHANNELS.DIALOG.OPEN_FILE, + async (_, options: Record = {}) => { + if (!state.mainWindow) { + return { canceled: true, filePaths: [] }; + } + // Ensure openFile is always present and filter out directory-related properties + const inputProperties = (options.properties as string[]) ?? []; + const properties = ['openFile', ...inputProperties].filter( + (p) => p !== 'openDirectory' && p !== 'createDirectory' + ); + const result = await dialog.showOpenDialog(state.mainWindow, { + ...options, + properties: properties as Electron.OpenDialogOptions['properties'], + }); + return result; } - const result = await dialog.showOpenDialog(state.mainWindow, { - properties: ['openFile'], - ...options, - }); - return result; - }); + ); // Save file dialog ipcMain.handle(IPC_CHANNELS.DIALOG.SAVE_FILE, async (_, options = {}) => { diff --git a/apps/ui/src/electron/ipc/shell-handlers.ts b/apps/ui/src/electron/ipc/shell-handlers.ts index 8e852f5b..db079630 100644 --- a/apps/ui/src/electron/ipc/shell-handlers.ts +++ b/apps/ui/src/electron/ipc/shell-handlers.ts @@ -41,10 +41,10 @@ export function registerShellHandlers(): void { // URL encode the path to handle special characters (spaces, brackets, etc.) // Handle both Unix (/) and Windows (\) path separators const normalizedPath = filePath.replace(/\\/g, '/'); - const encodedPath = normalizedPath.startsWith('/') - ? '/' + normalizedPath.slice(1).split('/').map(encodeURIComponent).join('/') - : normalizedPath.split('/').map(encodeURIComponent).join('/'); - let url = `vscode://file${encodedPath}`; + const segments = normalizedPath.split('/').map(encodeURIComponent); + const encodedPath = segments.join('/'); + // VS Code URL format requires a leading slash after 'file' + let url = `vscode://file/${encodedPath}`; if (line !== undefined && line > 0) { url += `:${line}`; if (column !== undefined && column > 0) {