diff --git a/electron.vite.config.ts b/electron.vite.config.ts index 80cbf26c1..24b3bb22d 100644 --- a/electron.vite.config.ts +++ b/electron.vite.config.ts @@ -5,7 +5,13 @@ import tailwindcss from '@tailwindcss/vite' export default defineConfig({ main: {}, - preload: {}, + preload: { + build: { + externalizeDeps: { + exclude: ['@electron-toolkit/preload'] + } + } + }, renderer: { resolve: { alias: { diff --git a/src/main/github/client.test.ts b/src/main/github/client.test.ts index 869e896cf..37b6709cc 100644 --- a/src/main/github/client.test.ts +++ b/src/main/github/client.test.ts @@ -1,5 +1,17 @@ import { describe, expect, it } from 'vitest' -import { deriveCheckStatus } from './client' +import { deriveCheckStatus, mapPRState } from './client' + +describe('mapPRState', () => { + it('returns draft when an open PR is marked as draft', () => { + expect(mapPRState('OPEN', true)).toBe('draft') + }) + + it('preserves merged and closed states', () => { + expect(mapPRState('MERGED', true)).toBe('merged') + expect(mapPRState('CLOSED', true)).toBe('closed') + expect(mapPRState('OPEN')).toBe('open') + }) +}) describe('deriveCheckStatus', () => { it('returns neutral when no checks are present', () => { @@ -8,12 +20,7 @@ describe('deriveCheckStatus', () => { }) it('returns failure when a failed check is present', () => { - expect( - deriveCheckStatus([ - { status: 'QUEUED' }, - { conclusion: 'FAILURE' } - ]) - ).toBe('failure') + expect(deriveCheckStatus([{ status: 'QUEUED' }, { conclusion: 'FAILURE' }])).toBe('failure') }) it('returns pending when checks are still running', () => { @@ -21,11 +28,8 @@ describe('deriveCheckStatus', () => { }) it('returns success when all checks complete without failures', () => { - expect( - deriveCheckStatus([ - { conclusion: 'SUCCESS' }, - { conclusion: 'SKIPPED' } - ]) - ).toBe('success') + expect(deriveCheckStatus([{ conclusion: 'SUCCESS' }, { conclusion: 'SKIPPED' }])).toBe( + 'success' + ) }) }) diff --git a/src/main/github/client.ts b/src/main/github/client.ts index 28bd50971..2bac7ba95 100644 --- a/src/main/github/client.ts +++ b/src/main/github/client.ts @@ -41,7 +41,13 @@ export async function getPRForBranch(repoPath: string, branch: string): Promise< const branchName = branch.replace(/^refs\/heads\//, '') const { stdout } = await execFileAsync( 'gh', - ['pr', 'view', branchName, '--json', 'number,title,state,url,statusCheckRollup,updatedAt'], + [ + 'pr', + 'view', + branchName, + '--json', + 'number,title,state,url,statusCheckRollup,updatedAt,isDraft' + ], { cwd: repoPath, encoding: 'utf-8' @@ -51,7 +57,7 @@ export async function getPRForBranch(repoPath: string, branch: string): Promise< return { number: data.number, title: data.title, - state: mapPRState(data.state), + state: mapPRState(data.state, data.isDraft), url: data.url, checksStatus: deriveCheckStatus(data.statusCheckRollup), updatedAt: data.updatedAt @@ -216,7 +222,7 @@ function mapCheckConclusion(state: string): PRCheckDetail['conclusion'] { return null } -export function mapPRState(state: string): PRInfo['state'] { +export function mapPRState(state: string, isDraft?: boolean): PRInfo['state'] { const s = state?.toUpperCase() if (s === 'MERGED') { return 'merged' @@ -224,7 +230,9 @@ export function mapPRState(state: string): PRInfo['state'] { if (s === 'CLOSED') { return 'closed' } - // gh CLI returns isDraft separately, but state field is OPEN for drafts too + if (isDraft) { + return 'draft' + } return 'open' } diff --git a/src/main/ipc/filesystem-auth.ts b/src/main/ipc/filesystem-auth.ts new file mode 100644 index 000000000..9e8563789 --- /dev/null +++ b/src/main/ipc/filesystem-auth.ts @@ -0,0 +1,125 @@ +import { realpath } from 'fs/promises' +import { resolve, relative, sep, dirname, basename } from 'path' +import type { Store } from '../persistence' +import { listWorktrees } from '../git/worktree' + +export const PATH_ACCESS_DENIED_MESSAGE = + 'Access denied: path resolves outside allowed directories. If this blocks a legitimate workflow, please file a GitHub issue.' + +/** + * Check whether resolvedTarget is equal to or a descendant of resolvedBase. + * Uses relative() so it works with both `/` (Unix) and `\` (Windows) separators. + */ +export function isDescendantOrEqual(resolvedTarget: string, resolvedBase: string): boolean { + if (resolvedTarget === resolvedBase) { + return true + } + const rel = relative(resolvedBase, resolvedTarget) + // rel must not start with ".." and must not be an absolute path (e.g. different drive on Windows) + return ( + rel !== '' && + !rel.startsWith('..') && + !rel.startsWith(sep + sep) && + resolve(resolvedBase, rel) === resolvedTarget + ) +} + +export function getAllowedRoots(store: Store): string[] { + const roots = store.getRepos().map((repo) => resolve(repo.path)) + const workspaceDir = store.getSettings().workspaceDir + if (workspaceDir) { + roots.push(resolve(workspaceDir)) + } + return roots +} + +export function isPathAllowed(targetPath: string, store: Store): boolean { + const resolvedTarget = resolve(targetPath) + return getAllowedRoots(store).some((root) => isDescendantOrEqual(resolvedTarget, root)) +} + +/** + * Returns true if the error is an ENOENT (file-not-found) error. + */ +export function isENOENT(error: unknown): boolean { + return ( + error instanceof Error && 'code' in error && (error as NodeJS.ErrnoException).code === 'ENOENT' + ) +} + +export async function resolveAuthorizedPath(targetPath: string, store: Store): Promise { + const resolvedTarget = resolve(targetPath) + if (!isPathAllowed(resolvedTarget, store)) { + throw new Error(PATH_ACCESS_DENIED_MESSAGE) + } + + try { + const realTarget = await realpath(resolvedTarget) + if (!isPathAllowed(realTarget, store)) { + throw new Error(PATH_ACCESS_DENIED_MESSAGE) + } + return realTarget + } catch (error) { + if (!isENOENT(error)) { + throw error + } + + const realParent = await realpath(dirname(resolvedTarget)) + const candidateTarget = resolve(realParent, basename(resolvedTarget)) + if (!isPathAllowed(candidateTarget, store)) { + throw new Error(PATH_ACCESS_DENIED_MESSAGE) + } + return candidateTarget + } +} + +async function normalizeExistingPath(targetPath: string): Promise { + try { + return await realpath(targetPath) + } catch { + return resolve(targetPath) + } +} + +export async function resolveRegisteredWorktreePath( + worktreePath: string, + store: Store +): Promise { + const resolvedPath = await resolveAuthorizedPath(worktreePath, store) + + for (const repo of store.getRepos()) { + const normalizedRepoPath = await normalizeExistingPath(repo.path) + + if (resolvedPath === normalizedRepoPath) { + return resolvedPath + } + + const worktrees = await listWorktrees(repo.path) + for (const worktree of worktrees) { + const normalizedWorktreePath = await normalizeExistingPath(worktree.path) + if (resolvedPath === normalizedWorktreePath) { + return resolvedPath + } + } + } + + throw new Error('Access denied: unknown repository or worktree path') +} + +export function validateGitRelativeFilePath(worktreePath: string, filePath: string): string { + if (!filePath || filePath.includes('\0') || resolve(filePath) === filePath) { + throw new Error('Access denied: invalid git file path') + } + + const resolvedFilePath = resolve(worktreePath, filePath) + if (!isDescendantOrEqual(resolvedFilePath, worktreePath)) { + throw new Error('Access denied: git file path escapes the selected worktree') + } + + const normalizedRelativePath = relative(worktreePath, resolvedFilePath) + if (!normalizedRelativePath) { + throw new Error('Access denied: invalid git file path') + } + + return normalizedRelativePath +} diff --git a/src/main/ipc/filesystem.test.ts b/src/main/ipc/filesystem.test.ts new file mode 100644 index 000000000..fa47fbff8 --- /dev/null +++ b/src/main/ipc/filesystem.test.ts @@ -0,0 +1,178 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const handlers = new Map Promise | unknown>() +const { + handleMock, + readdirMock, + readFileMock, + writeFileMock, + statMock, + realpathMock, + lstatMock, + getStatusMock, + getDiffMock, + stageFileMock, + unstageFileMock, + discardChangesMock, + listWorktreesMock +} = vi.hoisted(() => ({ + handleMock: vi.fn(), + readdirMock: vi.fn(), + readFileMock: vi.fn(), + writeFileMock: vi.fn(), + statMock: vi.fn(), + realpathMock: vi.fn(), + lstatMock: vi.fn(), + getStatusMock: vi.fn(), + getDiffMock: vi.fn(), + stageFileMock: vi.fn(), + unstageFileMock: vi.fn(), + discardChangesMock: vi.fn(), + listWorktreesMock: vi.fn() +})) + +vi.mock('electron', () => ({ + ipcMain: { + handle: handleMock + } +})) + +vi.mock('fs/promises', () => ({ + readdir: readdirMock, + readFile: readFileMock, + writeFile: writeFileMock, + stat: statMock, + realpath: realpathMock, + lstat: lstatMock +})) + +vi.mock('../git/status', () => ({ + getStatus: getStatusMock, + getDiff: getDiffMock, + stageFile: stageFileMock, + unstageFile: unstageFileMock, + discardChanges: discardChangesMock +})) + +vi.mock('../git/worktree', () => ({ + listWorktrees: listWorktreesMock +})) + +import { registerFilesystemHandlers } from './filesystem' + +describe('registerFilesystemHandlers', () => { + const store = { + getRepos: () => [ + { + id: 'repo-1', + path: '/workspace/repo', + displayName: 'repo', + badgeColor: '#000', + addedAt: 0 + } + ], + getSettings: () => ({ + workspaceDir: '/workspace' + }) + } + + beforeEach(() => { + handlers.clear() + handleMock.mockReset() + readdirMock.mockReset() + readFileMock.mockReset() + writeFileMock.mockReset() + statMock.mockReset() + realpathMock.mockReset() + lstatMock.mockReset() + getStatusMock.mockReset() + getDiffMock.mockReset() + stageFileMock.mockReset() + unstageFileMock.mockReset() + discardChangesMock.mockReset() + listWorktreesMock.mockReset() + + handleMock.mockImplementation((channel, handler) => { + handlers.set(channel, handler) + }) + + realpathMock.mockImplementation(async (targetPath: string) => targetPath) + listWorktreesMock.mockResolvedValue([ + { path: '/workspace/repo-feature', head: 'abc', branch: '', isBare: false } + ]) + statMock.mockResolvedValue({ size: 10, isDirectory: () => false, mtimeMs: 123 }) + lstatMock.mockRejectedValue(Object.assign(new Error('missing'), { code: 'ENOENT' })) + }) + + it('rejects readFile when the real path escapes allowed roots', async () => { + realpathMock.mockImplementation(async (targetPath: string) => { + if (targetPath === '/workspace/repo/link.txt') { + return '/private/secret.txt' + } + return targetPath + }) + + registerFilesystemHandlers(store as never) + + await expect( + handlers.get('fs:readFile')!(null, { filePath: '/workspace/repo/link.txt' }) + ).rejects.toThrow('Access denied: path resolves outside allowed directories') + + expect(readFileMock).not.toHaveBeenCalled() + }) + + it('rejects writes to directories', async () => { + lstatMock.mockResolvedValue({ isDirectory: () => true }) + + registerFilesystemHandlers(store as never) + + await expect( + handlers.get('fs:writeFile')!(null, { + filePath: '/workspace/repo/folder', + content: 'data' + }) + ).rejects.toThrow('Cannot write to a directory') + + expect(writeFileMock).not.toHaveBeenCalled() + }) + + it('normalizes repo worktree paths and keeps git file paths relative', async () => { + stageFileMock.mockResolvedValue(undefined) + + registerFilesystemHandlers(store as never) + + await handlers.get('git:stage')!(null, { + worktreePath: '/workspace/repo-feature', + filePath: './src/../src/file.ts' + }) + + expect(stageFileMock).toHaveBeenCalledWith('/workspace/repo-feature', 'src/file.ts') + }) + + it('rejects git file paths that escape the selected worktree', async () => { + registerFilesystemHandlers(store as never) + + await expect( + handlers.get('git:discard')!(null, { + worktreePath: '/workspace/repo-feature', + filePath: '../outside.txt' + }) + ).rejects.toThrow('Access denied: git file path escapes the selected worktree') + + expect(discardChangesMock).not.toHaveBeenCalled() + }) + + it('rejects git operations for unknown worktrees', async () => { + listWorktreesMock.mockResolvedValue([]) + + registerFilesystemHandlers(store as never) + + await expect( + handlers.get('git:status')!(null, { + worktreePath: '/workspace/repo-feature' + }) + ).rejects.toThrow('Access denied: unknown repository or worktree path') + + expect(getStatusMock).not.toHaveBeenCalled() + }) +}) diff --git a/src/main/ipc/filesystem.ts b/src/main/ipc/filesystem.ts index 8cec5bcc9..d1d0e0f4b 100644 --- a/src/main/ipc/filesystem.ts +++ b/src/main/ipc/filesystem.ts @@ -1,6 +1,6 @@ import { ipcMain } from 'electron' -import { readdir, readFile, writeFile, stat } from 'fs/promises' -import { resolve, relative } from 'path' +import { readdir, readFile, writeFile, stat, lstat } from 'fs/promises' +import { relative } from 'path' import { spawn } from 'child_process' import type { Store } from '../persistence' import type { @@ -12,41 +12,15 @@ import type { SearchFileResult } from '../../shared/types' import { getStatus, getDiff, stageFile, unstageFile, discardChanges } from '../git/status' +import { + resolveAuthorizedPath, + resolveRegisteredWorktreePath, + validateGitRelativeFilePath, + isENOENT +} from './filesystem-auth' const MAX_FILE_SIZE = 5 * 1024 * 1024 // 5MB -/** - * Validate that a path is within a known worktree directory. - */ -function isPathAllowed(targetPath: string, store: Store): boolean { - const resolvedTarget = resolve(targetPath) - const repos = store.getRepos() - - for (const repo of repos) { - // Allow paths within the repo itself - if ( - resolvedTarget.startsWith(`${resolve(repo.path)}/`) || - resolvedTarget === resolve(repo.path) - ) { - return true - } - } - - // Also check the workspace directory from settings - const settings = store.getSettings() - if (settings.workspaceDir) { - const resolvedWorkspace = resolve(settings.workspaceDir) - if ( - resolvedTarget.startsWith(`${resolvedWorkspace}/`) || - resolvedTarget === resolvedWorkspace - ) { - return true - } - } - - return false -} - /** * Check if a buffer appears to be binary (contains null bytes in first 8KB). */ @@ -63,11 +37,8 @@ function isBinaryBuffer(buffer: Buffer): boolean { export function registerFilesystemHandlers(store: Store): void { // ─── Filesystem ───────────────────────────────────────── ipcMain.handle('fs:readDir', async (_event, args: { dirPath: string }): Promise => { - if (!isPathAllowed(args.dirPath, store)) { - throw new Error('Access denied: path is outside allowed directories') - } - - const entries = await readdir(args.dirPath, { withFileTypes: true }) + const dirPath = await resolveAuthorizedPath(args.dirPath, store) + const entries = await readdir(dirPath, { withFileTypes: true }) return entries .map((entry) => ({ name: entry.name, @@ -86,18 +57,15 @@ export function registerFilesystemHandlers(store: Store): void { ipcMain.handle( 'fs:readFile', async (_event, args: { filePath: string }): Promise<{ content: string; isBinary: boolean }> => { - if (!isPathAllowed(args.filePath, store)) { - throw new Error('Access denied: path is outside allowed directories') - } - - const stats = await stat(args.filePath) + const filePath = await resolveAuthorizedPath(args.filePath, store) + const stats = await stat(filePath) if (stats.size > MAX_FILE_SIZE) { throw new Error( `File too large: ${(stats.size / 1024 / 1024).toFixed(1)}MB exceeds ${MAX_FILE_SIZE / 1024 / 1024}MB limit` ) } - const buffer = await readFile(args.filePath) + const buffer = await readFile(filePath) if (isBinaryBuffer(buffer)) { return { content: '', isBinary: true } } @@ -109,11 +77,20 @@ export function registerFilesystemHandlers(store: Store): void { ipcMain.handle( 'fs:writeFile', async (_event, args: { filePath: string; content: string }): Promise => { - if (!isPathAllowed(args.filePath, store)) { - throw new Error('Access denied: path is outside allowed directories') + const filePath = await resolveAuthorizedPath(args.filePath, store) + + try { + const fileStats = await lstat(filePath) + if (fileStats.isDirectory()) { + throw new Error('Cannot write to a directory') + } + } catch (error) { + if (!isENOENT(error)) { + throw error + } } - await writeFile(args.filePath, args.content, 'utf-8') + await writeFile(filePath, args.content, 'utf-8') } ) @@ -123,11 +100,8 @@ export function registerFilesystemHandlers(store: Store): void { _event, args: { filePath: string } ): Promise<{ size: number; isDirectory: boolean; mtime: number }> => { - if (!isPathAllowed(args.filePath, store)) { - throw new Error('Access denied: path is outside allowed directories') - } - - const stats = await stat(args.filePath) + const filePath = await resolveAuthorizedPath(args.filePath, store) + const stats = await stat(filePath) return { size: stats.size, isDirectory: stats.isDirectory(), @@ -138,9 +112,7 @@ export function registerFilesystemHandlers(store: Store): void { // ─── Search ──────────────────────────────────────────── ipcMain.handle('fs:search', async (_event, args: SearchOptions): Promise => { - if (!isPathAllowed(args.rootPath, store)) { - throw new Error('Access denied: path is outside allowed directories') - } + const rootPath = await resolveAuthorizedPath(args.rootPath, store) const maxResults = args.maxResults ?? 10000 @@ -179,7 +151,7 @@ export function registerFilesystemHandlers(store: Store): void { } } - rgArgs.push('--', args.query, args.rootPath) + rgArgs.push('--', args.query, rootPath) const fileMap = new Map() let totalMatches = 0 @@ -213,7 +185,7 @@ export function registerFilesystemHandlers(store: Store): void { const data = msg.data const absPath: string = data.path.text - const relPath = relative(args.rootPath, absPath) + const relPath = relative(rootPath, absPath) let fileResult = fileMap.get(absPath) if (!fileResult) { @@ -275,7 +247,8 @@ export function registerFilesystemHandlers(store: Store): void { ipcMain.handle( 'git:status', async (_event, args: { worktreePath: string }): Promise => { - return getStatus(args.worktreePath) + const worktreePath = await resolveRegisteredWorktreePath(args.worktreePath, store) + return getStatus(worktreePath) } ) @@ -285,28 +258,36 @@ export function registerFilesystemHandlers(store: Store): void { _event, args: { worktreePath: string; filePath: string; staged: boolean } ): Promise => { - return getDiff(args.worktreePath, args.filePath, args.staged) + const worktreePath = await resolveRegisteredWorktreePath(args.worktreePath, store) + const filePath = validateGitRelativeFilePath(worktreePath, args.filePath) + return getDiff(worktreePath, filePath, args.staged) } ) ipcMain.handle( 'git:stage', async (_event, args: { worktreePath: string; filePath: string }): Promise => { - await stageFile(args.worktreePath, args.filePath) + const worktreePath = await resolveRegisteredWorktreePath(args.worktreePath, store) + const filePath = validateGitRelativeFilePath(worktreePath, args.filePath) + await stageFile(worktreePath, filePath) } ) ipcMain.handle( 'git:unstage', async (_event, args: { worktreePath: string; filePath: string }): Promise => { - await unstageFile(args.worktreePath, args.filePath) + const worktreePath = await resolveRegisteredWorktreePath(args.worktreePath, store) + const filePath = validateGitRelativeFilePath(worktreePath, args.filePath) + await unstageFile(worktreePath, filePath) } ) ipcMain.handle( 'git:discard', async (_event, args: { worktreePath: string; filePath: string }): Promise => { - await discardChanges(args.worktreePath, args.filePath) + const worktreePath = await resolveRegisteredWorktreePath(args.worktreePath, store) + const filePath = validateGitRelativeFilePath(worktreePath, args.filePath) + await discardChanges(worktreePath, filePath) } ) } diff --git a/src/main/ipc/github.test.ts b/src/main/ipc/github.test.ts new file mode 100644 index 000000000..a375e52d1 --- /dev/null +++ b/src/main/ipc/github.test.ts @@ -0,0 +1,92 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { handleMock, getPRForBranchMock, getIssueMock, listIssuesMock } = vi.hoisted(() => ({ + handleMock: vi.fn(), + getPRForBranchMock: vi.fn(), + getIssueMock: vi.fn(), + listIssuesMock: vi.fn() +})) + +vi.mock('electron', () => ({ + ipcMain: { + handle: handleMock + } +})) + +vi.mock('../github/client', () => ({ + getPRForBranch: getPRForBranchMock, + getIssue: getIssueMock, + listIssues: listIssuesMock +})) + +import { registerGitHubHandlers } from './github' + +type HandlerMap = Record unknown> + +describe('registerGitHubHandlers', () => { + const handlers: HandlerMap = {} + const store = { + getRepos: () => [ + { + id: 'repo-1', + path: '/workspace/repo', + displayName: 'repo', + badgeColor: '#000', + addedAt: 0 + } + ] + } + + beforeEach(() => { + handleMock.mockReset() + getPRForBranchMock.mockReset() + getIssueMock.mockReset() + listIssuesMock.mockReset() + for (const key of Object.keys(handlers)) { + delete handlers[key] + } + + handleMock.mockImplementation((channel, handler) => { + handlers[channel] = handler + }) + }) + + it('normalizes registered repo paths before invoking github clients', async () => { + getPRForBranchMock.mockResolvedValue({ number: 42 }) + + registerGitHubHandlers(store as never) + + await handlers['gh:prForBranch'](null, { + repoPath: '/workspace/repo/../repo', + branch: 'feature/test' + }) + + expect(getPRForBranchMock).toHaveBeenCalledWith('/workspace/repo', 'feature/test') + }) + + it('rejects unknown repository paths', async () => { + registerGitHubHandlers(store as never) + + expect(() => + handlers['gh:issue'](null, { + repoPath: '/workspace/other', + number: 7 + }) + ).toThrow('Access denied: unknown repository path') + + expect(getIssueMock).not.toHaveBeenCalled() + }) + + it('forwards listIssues for registered repositories', async () => { + listIssuesMock.mockResolvedValue([]) + + registerGitHubHandlers(store as never) + + await handlers['gh:listIssues'](null, { + repoPath: '/workspace/repo', + limit: 5 + }) + + expect(listIssuesMock).toHaveBeenCalledWith('/workspace/repo', 5) + }) +}) diff --git a/src/main/ipc/github.ts b/src/main/ipc/github.ts index 147f40bf9..7b1a5ba29 100644 --- a/src/main/ipc/github.ts +++ b/src/main/ipc/github.ts @@ -1,27 +1,43 @@ import { ipcMain } from 'electron' +import { resolve } from 'path' +import type { Store } from '../persistence' import { getPRForBranch, getIssue, listIssues, getPRChecks, updatePRTitle } from '../github/client' -export function registerGitHubHandlers(): void { +function assertRegisteredRepoPath(repoPath: string, store: Store): string { + const resolvedRepoPath = resolve(repoPath) + const registeredRepo = store.getRepos().find((repo) => resolve(repo.path) === resolvedRepoPath) + if (!registeredRepo) { + throw new Error('Access denied: unknown repository path') + } + return registeredRepo.path +} + +export function registerGitHubHandlers(store: Store): void { ipcMain.handle('gh:prForBranch', (_event, args: { repoPath: string; branch: string }) => { - return getPRForBranch(args.repoPath, args.branch) + const repoPath = assertRegisteredRepoPath(args.repoPath, store) + return getPRForBranch(repoPath, args.branch) }) ipcMain.handle('gh:issue', (_event, args: { repoPath: string; number: number }) => { - return getIssue(args.repoPath, args.number) + const repoPath = assertRegisteredRepoPath(args.repoPath, store) + return getIssue(repoPath, args.number) }) ipcMain.handle('gh:listIssues', (_event, args: { repoPath: string; limit?: number }) => { - return listIssues(args.repoPath, args.limit) + const repoPath = assertRegisteredRepoPath(args.repoPath, store) + return listIssues(repoPath, args.limit) }) ipcMain.handle('gh:prChecks', (_event, args: { repoPath: string; prNumber: number }) => { - return getPRChecks(args.repoPath, args.prNumber) + const repoPath = assertRegisteredRepoPath(args.repoPath, store) + return getPRChecks(repoPath, args.prNumber) }) ipcMain.handle( 'gh:updatePRTitle', (_event, args: { repoPath: string; prNumber: number; title: string }) => { - return updatePRTitle(args.repoPath, args.prNumber, args.title) + const repoPath = assertRegisteredRepoPath(args.repoPath, store) + return updatePRTitle(repoPath, args.prNumber, args.title) } ) } diff --git a/src/main/ipc/register-core-handlers.test.ts b/src/main/ipc/register-core-handlers.test.ts new file mode 100644 index 000000000..8ca93b905 --- /dev/null +++ b/src/main/ipc/register-core-handlers.test.ts @@ -0,0 +1,80 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { + registerGitHubHandlersMock, + registerSettingsHandlersMock, + registerShellHandlersMock, + registerSessionHandlersMock, + registerUIHandlersMock, + registerFilesystemHandlersMock, + registerClipboardHandlersMock, + registerUpdaterHandlersMock +} = vi.hoisted(() => ({ + registerGitHubHandlersMock: vi.fn(), + registerSettingsHandlersMock: vi.fn(), + registerShellHandlersMock: vi.fn(), + registerSessionHandlersMock: vi.fn(), + registerUIHandlersMock: vi.fn(), + registerFilesystemHandlersMock: vi.fn(), + registerClipboardHandlersMock: vi.fn(), + registerUpdaterHandlersMock: vi.fn() +})) + +vi.mock('./github', () => ({ + registerGitHubHandlers: registerGitHubHandlersMock +})) + +vi.mock('./settings', () => ({ + registerSettingsHandlers: registerSettingsHandlersMock +})) + +vi.mock('./shell', () => ({ + registerShellHandlers: registerShellHandlersMock +})) + +vi.mock('./session', () => ({ + registerSessionHandlers: registerSessionHandlersMock +})) + +vi.mock('./ui', () => ({ + registerUIHandlers: registerUIHandlersMock +})) + +vi.mock('./filesystem', () => ({ + registerFilesystemHandlers: registerFilesystemHandlersMock +})) + +vi.mock('../window/attach-main-window-services', () => ({ + registerClipboardHandlers: registerClipboardHandlersMock, + registerUpdaterHandlers: registerUpdaterHandlersMock +})) + +import { registerCoreHandlers } from './register-core-handlers' + +describe('registerCoreHandlers', () => { + beforeEach(() => { + registerGitHubHandlersMock.mockReset() + registerSettingsHandlersMock.mockReset() + registerShellHandlersMock.mockReset() + registerSessionHandlersMock.mockReset() + registerUIHandlersMock.mockReset() + registerFilesystemHandlersMock.mockReset() + registerClipboardHandlersMock.mockReset() + registerUpdaterHandlersMock.mockReset() + }) + + it('passes the store through to handler registrars that need it', () => { + const store = { marker: 'store' } + + registerCoreHandlers(store as never) + + expect(registerGitHubHandlersMock).toHaveBeenCalledWith(store) + expect(registerSettingsHandlersMock).toHaveBeenCalledWith(store) + expect(registerSessionHandlersMock).toHaveBeenCalledWith(store) + expect(registerUIHandlersMock).toHaveBeenCalledWith(store) + expect(registerFilesystemHandlersMock).toHaveBeenCalledWith(store) + expect(registerShellHandlersMock).toHaveBeenCalled() + expect(registerClipboardHandlersMock).toHaveBeenCalled() + expect(registerUpdaterHandlersMock).toHaveBeenCalled() + }) +}) diff --git a/src/main/ipc/register-core-handlers.ts b/src/main/ipc/register-core-handlers.ts index f7388094b..617586622 100644 --- a/src/main/ipc/register-core-handlers.ts +++ b/src/main/ipc/register-core-handlers.ts @@ -12,7 +12,7 @@ import { } from '../window/attach-main-window-services' export function registerCoreHandlers(store: Store): void { - registerGitHubHandlers() + registerGitHubHandlers(store) registerSettingsHandlers(store) registerShellHandlers() registerSessionHandlers(store) diff --git a/src/main/ipc/worktrees.ts b/src/main/ipc/worktrees.ts index 2274837b1..3c4017678 100644 --- a/src/main/ipc/worktrees.ts +++ b/src/main/ipc/worktrees.ts @@ -1,6 +1,6 @@ import type { BrowserWindow } from 'electron' import { ipcMain } from 'electron' -import { join, basename } from 'path' +import { join, basename, resolve, relative, isAbsolute } from 'path' import type { Store } from '../persistence' import type { Worktree, WorktreeMeta } from '../../shared/types' import { listWorktrees, addWorktree, removeWorktree } from '../git/worktree' @@ -58,9 +58,7 @@ export function registerWorktreeHandlers(mainWindow: BrowserWindow, store: Store const settings = store.getSettings() const requestedName = args.name - // Sanitize name for use in branch names and directory paths - // (git branch names cannot contain spaces; collapse runs of spaces to a single hyphen) - const sanitizedName = args.name.replace(/\s+/g, '-') + const sanitizedName = sanitizeWorktreeName(args.name) // Compute branch name with prefix let branchName = sanitizedName @@ -83,6 +81,7 @@ export function registerWorktreeHandlers(mainWindow: BrowserWindow, store: Store } else { worktreePath = join(settings.workspaceDir, sanitizedName) } + worktreePath = ensurePathWithinWorkspace(worktreePath, settings.workspaceDir) // Determine base branch const baseBranch = args.baseBranch || repo.worktreeBaseRef || getDefaultBaseRef(repo.path) @@ -196,6 +195,34 @@ function mergeWorktree( } } +function sanitizeWorktreeName(input: string): string { + const sanitized = input + .trim() + .replace(/[\\/]+/g, '-') + .replace(/\s+/g, '-') + .replace(/[^A-Za-z0-9._-]+/g, '-') + .replace(/-+/g, '-') + .replace(/^[.-]+|[.-]+$/g, '') + + if (!sanitized || sanitized === '.' || sanitized === '..') { + throw new Error('Invalid worktree name') + } + + return sanitized +} + +function ensurePathWithinWorkspace(targetPath: string, workspaceDir: string): string { + const resolvedWorkspaceDir = resolve(workspaceDir) + const resolvedTargetPath = resolve(targetPath) + const rel = relative(resolvedWorkspaceDir, resolvedTargetPath) + + if (isAbsolute(rel) || rel.startsWith('..')) { + throw new Error('Invalid worktree path') + } + + return resolvedTargetPath +} + function parseWorktreeId(worktreeId: string): { repoId: string; worktreePath: string } { const sepIdx = worktreeId.indexOf('::') if (sepIdx === -1) { diff --git a/src/main/window/attach-main-window-services.test.ts b/src/main/window/attach-main-window-services.test.ts new file mode 100644 index 000000000..7576b86a7 --- /dev/null +++ b/src/main/window/attach-main-window-services.test.ts @@ -0,0 +1,87 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { + onMock, + removeAllListenersMock, + setPermissionRequestHandlerMock, + registerRepoHandlersMock, + registerWorktreeHandlersMock, + registerPtyHandlersMock, + setupAutoUpdaterMock +} = vi.hoisted(() => ({ + onMock: vi.fn(), + removeAllListenersMock: vi.fn(), + setPermissionRequestHandlerMock: vi.fn(), + registerRepoHandlersMock: vi.fn(), + registerWorktreeHandlersMock: vi.fn(), + registerPtyHandlersMock: vi.fn(), + setupAutoUpdaterMock: vi.fn() +})) + +vi.mock('electron', () => ({ + app: {}, + clipboard: {}, + ipcMain: { + on: onMock, + removeAllListeners: removeAllListenersMock, + removeHandler: vi.fn(), + handle: vi.fn() + } +})) + +vi.mock('../ipc/repos', () => ({ + registerRepoHandlers: registerRepoHandlersMock +})) + +vi.mock('../ipc/worktrees', () => ({ + registerWorktreeHandlers: registerWorktreeHandlersMock +})) + +vi.mock('../ipc/pty', () => ({ + registerPtyHandlers: registerPtyHandlersMock +})) + +vi.mock('../updater', () => ({ + checkForUpdates: vi.fn(), + getUpdateStatus: vi.fn(), + quitAndInstall: vi.fn(), + setupAutoUpdater: setupAutoUpdaterMock +})) + +import { attachMainWindowServices } from './attach-main-window-services' + +describe('attachMainWindowServices', () => { + beforeEach(() => { + onMock.mockReset() + removeAllListenersMock.mockReset() + setPermissionRequestHandlerMock.mockReset() + registerRepoHandlersMock.mockReset() + registerWorktreeHandlersMock.mockReset() + registerPtyHandlersMock.mockReset() + setupAutoUpdaterMock.mockReset() + }) + + it('only allows the explicit permission allowlist', () => { + const mainWindow = { + webContents: { + session: { + setPermissionRequestHandler: setPermissionRequestHandlerMock + } + } + } + const store = { flush: vi.fn() } + + attachMainWindowServices(mainWindow as never, store as never) + + expect(setPermissionRequestHandlerMock).toHaveBeenCalledTimes(1) + const permissionHandler = setPermissionRequestHandlerMock.mock.calls[0][0] + const callback = vi.fn() + + permissionHandler(null, 'media', callback) + permissionHandler(null, 'fullscreen', callback) + permissionHandler(null, 'pointerLock', callback) + permissionHandler(null, 'clipboard-read', callback) + + expect(callback.mock.calls).toEqual([[true], [true], [true], [false]]) + }) +}) diff --git a/src/main/window/attach-main-window-services.ts b/src/main/window/attach-main-window-services.ts index 93bc8dcce..f9b6aaad9 100644 --- a/src/main/window/attach-main-window-services.ts +++ b/src/main/window/attach-main-window-services.ts @@ -13,14 +13,10 @@ export function attachMainWindowServices(mainWindow: BrowserWindow, store: Store registerFileDropRelay(mainWindow) setupAutoUpdater(mainWindow, { onBeforeQuit: () => store.flush() }) + const allowedPermissions = new Set(['media', 'fullscreen', 'pointerLock']) mainWindow.webContents.session.setPermissionRequestHandler( (_webContents, permission, callback) => { - if (permission === 'clipboard-read' || permission === 'clipboard-sanitized-write') { - callback(false) - return - } - - callback(true) + callback(allowedPermissions.has(permission)) } ) } diff --git a/src/main/window/createMainWindow.test.ts b/src/main/window/createMainWindow.test.ts new file mode 100644 index 000000000..78fef32b7 --- /dev/null +++ b/src/main/window/createMainWindow.test.ts @@ -0,0 +1,73 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { browserWindowMock, openExternalMock } = vi.hoisted(() => ({ + browserWindowMock: vi.fn(), + openExternalMock: vi.fn() +})) + +vi.mock('electron', () => ({ + BrowserWindow: browserWindowMock, + nativeTheme: { shouldUseDarkColors: false }, + shell: { openExternal: openExternalMock } +})) + +vi.mock('@electron-toolkit/utils', () => ({ + is: { dev: false } +})) + +vi.mock('../../../resources/icon.png?asset', () => ({ + default: 'icon' +})) + +vi.mock('../../../resources/icon-dev.png?asset', () => ({ + default: 'icon-dev' +})) + +import { createMainWindow } from './createMainWindow' + +describe('createMainWindow', () => { + beforeEach(() => { + browserWindowMock.mockReset() + openExternalMock.mockReset() + }) + + it('enables renderer sandboxing and only opens http(s) URLs externally', () => { + const windowHandlers: Record void> = {} + const webContents = { + on: vi.fn((event, handler) => { + windowHandlers[event] = handler + }), + setZoomLevel: vi.fn(), + setWindowOpenHandler: vi.fn((handler) => { + windowHandlers.windowOpen = handler + }), + send: vi.fn() + } + const browserWindowInstance = { + webContents, + on: vi.fn(), + maximize: vi.fn(), + show: vi.fn(), + loadFile: vi.fn(), + loadURL: vi.fn() + } + browserWindowMock.mockImplementation(function () { + return browserWindowInstance + }) + + createMainWindow(null) + + expect(browserWindowMock).toHaveBeenCalledWith( + expect.objectContaining({ + webPreferences: expect.objectContaining({ sandbox: true }) + }) + ) + + expect(windowHandlers.windowOpen({ url: 'https://example.com' })).toEqual({ action: 'deny' }) + expect(windowHandlers.windowOpen({ url: 'file:///etc/passwd' })).toEqual({ action: 'deny' }) + expect(windowHandlers.windowOpen({ url: 'not a url' })).toEqual({ action: 'deny' }) + + expect(openExternalMock).toHaveBeenCalledTimes(1) + expect(openExternalMock).toHaveBeenCalledWith('https://example.com') + }) +}) diff --git a/src/main/window/createMainWindow.ts b/src/main/window/createMainWindow.ts index c151dc388..e8378d1cb 100644 --- a/src/main/window/createMainWindow.ts +++ b/src/main/window/createMainWindow.ts @@ -19,7 +19,7 @@ export function createMainWindow(store: Store | null): BrowserWindow { icon: is.dev ? devIcon : icon, webPreferences: { preload: join(__dirname, '../preload/index.js'), - sandbox: false + sandbox: true } }) @@ -33,7 +33,14 @@ export function createMainWindow(store: Store | null): BrowserWindow { }) mainWindow.webContents.setWindowOpenHandler((details) => { - shell.openExternal(details.url) + try { + const parsed = new URL(details.url) + if (parsed.protocol === 'https:' || parsed.protocol === 'http:') { + shell.openExternal(details.url) + } + } catch { + // ignore malformed URLs + } return { action: 'deny' } }) diff --git a/src/renderer/src/store/slices/editor.test.ts b/src/renderer/src/store/slices/editor.test.ts new file mode 100644 index 000000000..7c2e18426 --- /dev/null +++ b/src/renderer/src/store/slices/editor.test.ts @@ -0,0 +1,60 @@ +import { createStore, type StoreApi } from 'zustand/vanilla' +import { describe, expect, it } from 'vitest' +import { createEditorSlice } from './editor' +import type { AppState } from '../types' + +function createEditorStore(): StoreApi { + // Only the editor slice + activeWorktreeId are needed for these tests. + // eslint-disable-next-line @typescript-eslint/no-explicit-any + return createStore()((...args: any[]) => ({ + activeWorktreeId: 'wt-1', + ...createEditorSlice(...(args as Parameters)) + })) as unknown as StoreApi +} + +describe('createEditorSlice openDiff', () => { + it('keeps staged and unstaged diffs in separate tabs', () => { + const store = createEditorStore() + + store.getState().openDiff('wt-1', '/repo/file.ts', 'file.ts', 'typescript', false) + store.getState().openDiff('wt-1', '/repo/file.ts', 'file.ts', 'typescript', true) + + expect(store.getState().openFiles.map((file) => file.id)).toEqual([ + '/repo/file.ts::unstaged', + '/repo/file.ts::staged' + ]) + }) + + it('repairs an existing diff tab entry to the correct mode and staged state', () => { + const store = createEditorStore() + + store.setState({ + openFiles: [ + { + id: '/repo/file.ts::staged', + filePath: '/repo/file.ts', + relativePath: 'file.ts', + worktreeId: 'wt-1', + language: 'typescript', + isDirty: false, + mode: 'edit' + } + ], + activeFileId: null, + activeFileIdByWorktree: {}, + activeTabTypeByWorktree: {}, + activeTabType: 'terminal' + }) + + store.getState().openDiff('wt-1', '/repo/file.ts', 'file.ts', 'typescript', true) + + expect(store.getState().openFiles).toEqual([ + expect.objectContaining({ + id: '/repo/file.ts::staged', + mode: 'diff', + diffStaged: true + }) + ]) + expect(store.getState().activeFileId).toBe('/repo/file.ts::staged') + }) +}) diff --git a/src/renderer/src/store/slices/editor.ts b/src/renderer/src/store/slices/editor.ts index a651734b5..1abda2086 100644 --- a/src/renderer/src/store/slices/editor.ts +++ b/src/renderer/src/store/slices/editor.ts @@ -279,10 +279,18 @@ export const createEditorSlice: StateCreator = (s openDiff: (worktreeId, filePath, relativePath, language, staged) => set((s) => { - const id = `${filePath}${staged ? '::staged' : ''}` + const id = `${filePath}::${staged ? 'staged' : 'unstaged'}` const existing = s.openFiles.find((f) => f.id === id) if (existing) { + // Ensure mode and diffStaged are up-to-date (e.g. if a plain edit tab + // previously occupied this id before the suffix scheme changed). + const needsUpdate = existing.mode !== 'diff' || existing.diffStaged !== staged return { + openFiles: needsUpdate + ? s.openFiles.map((f) => + f.id === id ? { ...f, mode: 'diff' as const, diffStaged: staged } : f + ) + : s.openFiles, activeFileId: id, activeTabType: 'editor', activeFileIdByWorktree: { ...s.activeFileIdByWorktree, [worktreeId]: id },