diff --git a/src/main/ipc/githubIpc.ts b/src/main/ipc/githubIpc.ts index eef4604b2..17898d384 100644 --- a/src/main/ipc/githubIpc.ts +++ b/src/main/ipc/githubIpc.ts @@ -4,7 +4,7 @@ import { GitHubService } from '../services/GitHubService'; import { worktreeService } from '../services/WorktreeService'; import { githubCLIInstaller } from '../services/GitHubCLIInstaller'; import { databaseService } from '../services/DatabaseService'; -import { exec } from 'child_process'; +import { exec, execFile } from 'child_process'; import { promisify } from 'util'; import * as path from 'path'; import * as fs from 'fs'; @@ -13,6 +13,7 @@ import { homedir } from 'os'; import { quoteShellArg } from '../utils/shellEscape'; const execAsync = promisify(exec); +const execFileAsync = promisify(execFile); const githubService = new GitHubService(); const slugify = (name: string) => @@ -380,7 +381,7 @@ export function registerGithubIpc() { // Find the project root from the worktree path let projectRoot: string; try { - const { stdout } = await execAsync('git rev-parse --show-toplevel', { + const { stdout } = await execFileAsync('git', ['rev-parse', '--show-toplevel'], { cwd: worktreePath, }); projectRoot = stdout.trim(); @@ -398,7 +399,7 @@ export function registerGithubIpc() { // Fetch the base branch to ensure we have the latest try { - await execAsync(`git fetch origin ${quoteShellArg(baseRefName)}`, { cwd: worktreePath }); + await execFileAsync('git', ['fetch', 'origin', baseRefName], { cwd: worktreePath }); } catch { // Best effort — base ref may already be available locally } @@ -409,16 +410,17 @@ export function registerGithubIpc() { let diff: string; try { // Three-dot diff: changes introduced by the PR relative to the merge base - const { stdout } = await execAsync( - `git diff ${quoteShellArg(`origin/${baseRefName}`)}...HEAD`, - { cwd: worktreePath, maxBuffer: 10 * 1024 * 1024 } - ); + const { stdout } = await execFileAsync('git', ['diff', `origin/${baseRefName}...HEAD`], { + cwd: worktreePath, + maxBuffer: 10 * 1024 * 1024, + }); diff = stdout; } catch { // Fallback: two-dot diff try { - const { stdout } = await execAsync( - `git diff ${quoteShellArg(`origin/${baseRefName}`)} HEAD`, + const { stdout } = await execFileAsync( + 'git', + ['diff', `origin/${baseRefName}`, 'HEAD'], { cwd: worktreePath, maxBuffer: 10 * 1024 * 1024 } ); diff = stdout; diff --git a/src/main/services/DatabaseService.ts b/src/main/services/DatabaseService.ts index 9a4534b40..0a4b2942f 100644 --- a/src/main/services/DatabaseService.ts +++ b/src/main/services/DatabaseService.ts @@ -1,4 +1,5 @@ import type sqlite3Type from 'sqlite3'; +import * as crypto from 'crypto'; import { and, asc, desc, eq, inArray, isNull, ne, or, sql } from 'drizzle-orm'; import { readMigrationFiles } from 'drizzle-orm/migrator'; import { resolveDatabasePath, resolveMigrationsPath } from '../db/path'; @@ -626,7 +627,7 @@ export class DatabaseService { .where(eq(conversationsTable.taskId, taskId)); // Create the new conversation - const conversationId = `conv_${Date.now()}_${Math.random().toString(36).substr(2, 9)}`; + const conversationId = `conv_${crypto.randomUUID()}`; const newConversation = { id: conversationId, taskId, @@ -710,7 +711,7 @@ export class DatabaseService { input: Omit ): Promise { if (this.disabled) return ''; - const id = `comment-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`; + const id = `comment-${crypto.randomUUID()}`; const { db } = await getDrizzleClient(); await db.insert(lineCommentsTable).values({ id, @@ -795,7 +796,7 @@ export class DatabaseService { } const { db } = await getDrizzleClient(); - const id = connection.id ?? `ssh_${Date.now()}_${Math.random().toString(36).substr(2, 9)}`; + const id = connection.id ?? `ssh_${crypto.randomUUID()}`; const now = new Date().toISOString(); const result = await db diff --git a/src/main/services/GitHubService.ts b/src/main/services/GitHubService.ts index e0afbc9a3..872bd7667 100644 --- a/src/main/services/GitHubService.ts +++ b/src/main/services/GitHubService.ts @@ -1,4 +1,4 @@ -import { exec, spawn } from 'child_process'; +import { execFile, spawn } from 'child_process'; import { promisify } from 'util'; import * as path from 'path'; import * as fs from 'fs'; @@ -7,7 +7,7 @@ import { getMainWindow } from '../app/window'; import { errorTracking } from '../errorTracking'; import { sortByUpdatedAtDesc } from '../utils/issueSorting'; -const execAsync = promisify(exec); +const execFileAsync = promisify(execFile); export interface GitHubUser { id: number; @@ -431,7 +431,7 @@ export class GitHubService { private async authenticateGHCLI(token: string): Promise { try { // Check if gh CLI is installed first - await execAsync('gh --version'); + await execFileAsync('gh', ['--version']); // Security: Authenticate gh CLI with token via stdin (not shell interpolation) // This prevents command injection if token contains shell metacharacters @@ -461,14 +461,15 @@ export class GitHubService { } /** - * Execute gh command with automatic re-auth on failure + * Execute gh command with automatic re-auth on failure. + * Uses execFile with array args to prevent shell injection. */ private async execGH( - command: string, - options?: any + args: string[], + options?: { cwd?: string } ): Promise<{ stdout: string; stderr: string }> { try { - const result = await execAsync(command, { encoding: 'utf8', ...options }); + const result = await execFileAsync('gh', args, { encoding: 'utf8', ...options }); return { stdout: String(result.stdout), stderr: String(result.stderr), @@ -482,7 +483,7 @@ export class GitHubService { await this.authenticateGHCLI(token); // Retry the command - const result = await execAsync(command, { encoding: 'utf8', ...options }); + const result = await execFileAsync('gh', args, { encoding: 'utf8', ...options }); return { stdout: String(result.stdout), stderr: String(result.stderr), @@ -514,7 +515,16 @@ export class GitHubService { try { const fields = ['number', 'title', 'url', 'state', 'updatedAt', 'assignees', 'labels']; const { stdout } = await this.execGH( - `gh issue list --state open --limit ${safeLimit} --json ${fields.join(',')}`, + [ + 'issue', + 'list', + '--state', + 'open', + '--limit', + String(safeLimit), + '--json', + fields.join(','), + ], { cwd: projectPath } ); const list = JSON.parse(stdout || '[]'); @@ -549,7 +559,18 @@ export class GitHubService { try { const fields = ['number', 'title', 'url', 'state', 'updatedAt', 'assignees', 'labels']; const { stdout } = await this.execGH( - `gh issue list --state open --search ${JSON.stringify(term)} --limit ${safeLimit} --json ${fields.join(',')}`, + [ + 'issue', + 'list', + '--state', + 'open', + '--search', + term, + '--limit', + String(safeLimit), + '--json', + fields.join(','), + ], { cwd: projectPath } ); const list = JSON.parse(stdout || '[]'); @@ -587,7 +608,7 @@ export class GitHubService { 'labels', ]; const { stdout } = await this.execGH( - `gh issue view ${JSON.stringify(String(number))} --json ${fields.join(',')}`, + ['issue', 'view', String(number), '--json', fields.join(',')], { cwd: projectPath } ); const data = JSON.parse(stdout || 'null'); @@ -663,7 +684,7 @@ export class GitHubService { private async isGHCLIAuthenticated(): Promise { try { // gh auth status exits with 0 if authenticated, non-zero otherwise - await execAsync('gh auth status'); + await execFileAsync('gh', ['auth', 'status']); return true; } catch (error) { // Not authenticated or gh CLI not installed @@ -693,7 +714,7 @@ export class GitHubService { userData = await response.json(); } else { // Use gh CLI to get user info as fallback - const { stdout } = await this.execGH('gh api user'); + const { stdout } = await this.execGH(['api', 'user']); userData = JSON.parse(stdout); } @@ -737,9 +758,14 @@ export class GitHubService { async getRepositories(_token: string): Promise { try { // Use gh CLI to get repositories with correct field names - const { stdout } = await this.execGH( - 'gh repo list --limit 100 --json name,nameWithOwner,description,url,defaultBranchRef,isPrivate,updatedAt,primaryLanguage,stargazerCount,forkCount' - ); + const { stdout } = await this.execGH([ + 'repo', + 'list', + '--limit', + '100', + '--json', + 'name,nameWithOwner,description,url,defaultBranchRef,isPrivate,updatedAt,primaryLanguage,stargazerCount,forkCount', + ]); const repos = JSON.parse(stdout); return repos.map((repo: any) => ({ @@ -787,7 +813,7 @@ export class GitHubService { 'headRepository', ]; const { stdout } = await this.execGH( - `gh pr list --state open --limit ${safeLimit} --json ${fields.join(',')}`, + ['pr', 'list', '--state', 'open', '--limit', String(safeLimit), '--json', fields.join(',')], { cwd: projectPath } ); const list = JSON.parse(stdout || '[]'); @@ -824,7 +850,7 @@ export class GitHubService { private async getOpenPullRequestCount(projectPath: string): Promise { try { const { stdout: repoStdout } = await this.execGH( - 'gh repo view --json nameWithOwner --jq .nameWithOwner', + ['repo', 'view', '--json', 'nameWithOwner', '--jq', '.nameWithOwner'], { cwd: projectPath } ); const repoNameWithOwner = repoStdout.trim(); @@ -832,7 +858,7 @@ export class GitHubService { const query = `repo:${repoNameWithOwner} is:pr is:open`; const { stdout } = await this.execGH( - `gh api search/issues --method GET -f q=${JSON.stringify(query)} --jq .total_count`, + ['api', 'search/issues', '--method', 'GET', '-f', `q=${query}`, '--jq', '.total_count'], { cwd: projectPath } ); @@ -860,7 +886,7 @@ export class GitHubService { try { const fields = ['baseRefName', 'headRefName', 'title', 'number', 'url']; const { stdout } = await this.execGH( - `gh pr view ${JSON.stringify(String(prNumber))} --json ${fields.join(',')}`, + ['pr', 'view', String(prNumber), '--json', fields.join(',')], { cwd: projectPath } ); const data = JSON.parse(stdout || 'null'); @@ -892,8 +918,9 @@ export class GitHubService { // Fetch the PR ref directly without checking out (avoids touching the working tree) try { const prRef = `refs/pull/${prNumber}/head`; - await execAsync( - `git fetch origin ${JSON.stringify(prRef)}:${JSON.stringify(`refs/heads/${safeBranch}`)} --force`, + await execFileAsync( + 'git', + ['fetch', 'origin', `${prRef}:refs/heads/${safeBranch}`, '--force'], { cwd: projectPath } ); } catch (fetchError) { @@ -904,7 +931,7 @@ export class GitHubService { ); let previousRef: string | null = null; try { - const { stdout } = await execAsync('git rev-parse --abbrev-ref HEAD', { + const { stdout } = await execFileAsync('git', ['rev-parse', '--abbrev-ref', 'HEAD'], { cwd: projectPath, }); const current = (stdout || '').trim(); @@ -915,7 +942,7 @@ export class GitHubService { try { await this.execGH( - `gh pr checkout ${JSON.stringify(String(prNumber))} --branch ${JSON.stringify(safeBranch)} --force --detach`, + ['pr', 'checkout', String(prNumber), '--branch', safeBranch, '--force', '--detach'], { cwd: projectPath } ); } catch (error) { @@ -924,7 +951,7 @@ export class GitHubService { } finally { if (previousRef && previousRef !== safeBranch) { try { - await execAsync(`git checkout ${JSON.stringify(previousRef)} --force`, { + await execFileAsync('git', ['checkout', previousRef, '--force'], { cwd: projectPath, }); } catch (switchErr) { @@ -1011,7 +1038,7 @@ export class GitHubService { */ async checkRepositoryExists(owner: string, name: string): Promise { try { - await this.execGH(`gh repo view ${owner}/${name}`); + await this.execGH(['repo', 'view', `${owner}/${name}`]); return true; } catch { return false; @@ -1024,7 +1051,7 @@ export class GitHubService { async getOwners(): Promise> { try { // Get current user - const { stdout: userStdout } = await this.execGH('gh api user'); + const { stdout: userStdout } = await this.execGH(['api', 'user']); const user = JSON.parse(userStdout); const owners: Array<{ login: string; type: 'User' | 'Organization' }> = [ @@ -1033,7 +1060,7 @@ export class GitHubService { // Get organizations try { - const { stdout: orgsStdout } = await this.execGH('gh api user/orgs'); + const { stdout: orgsStdout } = await this.execGH(['api', 'user/orgs']); const orgs = JSON.parse(orgsStdout); if (Array.isArray(orgs)) { for (const org of orgs) { @@ -1064,22 +1091,24 @@ export class GitHubService { try { const { name, description, owner, isPrivate } = params; - // Build gh repo create command + // Build gh repo create args as array to prevent shell injection const visibilityFlag = isPrivate ? '--private' : '--public'; - let command = `gh repo create ${owner}/${name} ${visibilityFlag} --confirm`; + const args = ['repo', 'create', `${owner}/${name}`, visibilityFlag, '--confirm']; if (description && description.trim()) { - // Escape description for shell - const desc = JSON.stringify(description.trim()); - command += ` --description ${desc}`; + args.push('--description', description.trim()); } - await this.execGH(command); + await this.execGH(args); // Get repository details - const { stdout } = await this.execGH( - `gh repo view ${owner}/${name} --json name,nameWithOwner,url,defaultBranchRef` - ); + const { stdout } = await this.execGH([ + 'repo', + 'view', + `${owner}/${name}`, + '--json', + 'name,nameWithOwner,url,defaultBranchRef', + ]); const repoInfo = JSON.parse(stdout); return { @@ -1118,15 +1147,15 @@ export class GitHubService { // Initialize git, add files, commit, and push const execOptions = { cwd: localPath }; - // Add and commit - await execAsync('git add README.md', execOptions); - await execAsync('git commit -m "Initial commit"', execOptions); + // Add and commit (use execFileAsync to avoid shell injection) + await execFileAsync('git', ['add', 'README.md'], execOptions); + await execFileAsync('git', ['commit', '-m', 'Initial commit'], execOptions); // Push to origin - await execAsync('git push -u origin main', execOptions).catch(async () => { + await execFileAsync('git', ['push', '-u', 'origin', 'main'], execOptions).catch(async () => { // If main branch doesn't exist, try master try { - await execAsync('git push -u origin master', execOptions); + await execFileAsync('git', ['push', '-u', 'origin', 'master'], execOptions); } catch { // If both fail, let the error propagate throw new Error('Failed to push to remote repository'); @@ -1152,8 +1181,8 @@ export class GitHubService { fs.mkdirSync(dir, { recursive: true }); } - // Clone the repository - await execAsync(`git clone "${repoUrl}" "${localPath}"`); + // Clone the repository (use execFileAsync to avoid shell injection) + await execFileAsync('git', ['clone', repoUrl, localPath]); return { success: true }; } catch (error) { @@ -1171,9 +1200,23 @@ export class GitHubService { async logout(): Promise { // Run both operations in parallel since they're independent await Promise.allSettled([ - // Logout from gh CLI - execAsync('echo Y | gh auth logout --hostname github.com').catch((error) => { - console.warn('Failed to logout from gh CLI (may not be installed or logged in):', error); + // Logout from gh CLI (use spawn to pipe 'Y' via stdin instead of shell) + new Promise((resolve) => { + const child = spawn('gh', ['auth', 'logout', '--hostname', 'github.com'], { + stdio: ['pipe', 'pipe', 'pipe'], + }); + child.stdin.write('Y\n'); + child.stdin.end(); + child.on('close', (code) => { + if (code !== 0) { + console.warn(`gh auth logout exited with code ${code}`); + } + resolve(); + }); + child.on('error', (error) => { + console.warn('Failed to logout from gh CLI (may not be installed or logged in):', error); + resolve(); + }); }), // Clear keychain token (async () => { diff --git a/src/main/services/RepositoryManager.ts b/src/main/services/RepositoryManager.ts index ccd9d8f5b..96b824739 100644 --- a/src/main/services/RepositoryManager.ts +++ b/src/main/services/RepositoryManager.ts @@ -1,7 +1,8 @@ -import { exec } from 'child_process'; +import { execFile } from 'child_process'; import { promisify } from 'util'; +import * as crypto from 'crypto'; -const execAsync = promisify(exec); +const execFileAsync = promisify(execFile); export interface Repo { id: string; @@ -24,10 +25,12 @@ export class RepositoryManager { return []; } - async addRepository(path: string): Promise { + async addRepository(repoPath: string): Promise { try { // Validate that the path is a git repository - const { stdout } = await execAsync(`cd "${path}" && git rev-parse --is-inside-work-tree`); + const { stdout } = await execFileAsync('git', ['rev-parse', '--is-inside-work-tree'], { + cwd: repoPath, + }); if (stdout.trim() !== 'true') { throw new Error('Not a git repository'); @@ -35,13 +38,13 @@ export class RepositoryManager { // Get repository info const [origin, defaultBranch] = await Promise.all([ - this.getOrigin(path), - this.getDefaultBranch(path), + this.getOrigin(repoPath), + this.getDefaultBranch(repoPath), ]); const repo: Repo = { id: this.generateId(), - path, + path: repoPath, origin, defaultBranch, lastActivity: new Date().toISOString(), @@ -54,28 +57,32 @@ export class RepositoryManager { } } - private async getOrigin(path: string): Promise { + private async getOrigin(repoPath: string): Promise { try { - const { stdout } = await execAsync(`cd "${path}" && git remote get-url origin`); + const { stdout } = await execFileAsync('git', ['remote', 'get-url', 'origin'], { + cwd: repoPath, + }); return stdout.trim(); } catch { return 'No origin'; } } - private async getDefaultBranch(path: string): Promise { + private async getDefaultBranch(repoPath: string): Promise { try { - const { stdout } = await execAsync( - `cd "${path}" && git symbolic-ref refs/remotes/origin/HEAD | sed 's@^refs/remotes/origin/@@'` - ); - return stdout.trim() || 'main'; + const { stdout } = await execFileAsync('git', ['symbolic-ref', 'refs/remotes/origin/HEAD'], { + cwd: repoPath, + }); + const ref = stdout.trim(); + const prefix = 'refs/remotes/origin/'; + return ref.startsWith(prefix) ? ref.slice(prefix.length) : ref || 'main'; } catch { return 'main'; } } private generateId(): string { - return Math.random().toString(36).substr(2, 9); + return crypto.randomUUID(); } getRepository(id: string): Repo | undefined { diff --git a/src/test/main/GitHubService.test.ts b/src/test/main/GitHubService.test.ts index 940e94a32..143a38357 100644 --- a/src/test/main/GitHubService.test.ts +++ b/src/test/main/GitHubService.test.ts @@ -1,7 +1,11 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; import { promisify } from 'util'; -const execCalls: string[] = []; +vi.mock('electron', () => ({ + app: { getPath: () => '/tmp/test-emdash' }, +})); + +const execFileCalls: { file: string; args: string[] }[] = []; let issueListStdout = '[]'; let issueSearchStdout = '[]'; let prListStdout = '[]'; @@ -9,9 +13,17 @@ let repoViewStdout = 'generalaction/emdash'; let prCountStdout = '0'; vi.mock('child_process', () => { - const execImpl = (command: string, options?: any, callback?: any) => { - const cb = typeof options === 'function' ? options : callback; - execCalls.push(command); + const execFileImpl = (file: string, args?: string[] | any, options?: any, callback?: any) => { + // execFile can be called as (file, args, cb) or (file, args, options, cb) + const actualArgs = Array.isArray(args) ? args : []; + const cb = + typeof callback === 'function' + ? callback + : typeof options === 'function' + ? options + : undefined; + + execFileCalls.push({ file, args: actualArgs }); const respond = (stdout: string) => { setImmediate(() => { @@ -19,6 +31,8 @@ vi.mock('child_process', () => { }); }; + const command = [file, ...actualArgs].join(' '); + if (command.startsWith('gh auth status')) { respond('github.com\n ✓ Logged in to github.com account test (keyring)\n'); } else if (command.startsWith('gh auth token')) { @@ -33,8 +47,8 @@ vi.mock('child_process', () => { avatar_url: '', }) ); - } else if (command.startsWith('gh issue list')) { - if (command.includes('--search')) { + } else if (command.includes('issue') && command.includes('list')) { + if (actualArgs.includes('--search')) { respond(issueSearchStdout); } else { respond(issueListStdout); @@ -53,9 +67,9 @@ vi.mock('child_process', () => { }; // Avoid TS7022 by annotating via any-cast for the Symbol-based property - (execImpl as any)[promisify.custom] = (command: string, options?: any) => { + (execFileImpl as any)[promisify.custom] = (file: string, args?: string[], options?: any) => { return new Promise<{ stdout: string; stderr: string }>((resolve, reject) => { - execImpl(command, options, (err: any, stdout: string, stderr: string) => { + execFileImpl(file, args, options, (err: any, stdout: string, stderr: string) => { if (err) { reject(err); return; @@ -66,7 +80,15 @@ vi.mock('child_process', () => { }; return { - exec: execImpl, + execFile: execFileImpl, + spawn: vi.fn().mockReturnValue({ + stdin: { write: vi.fn(), end: vi.fn() }, + stdout: { on: vi.fn() }, + stderr: { on: vi.fn() }, + on: vi.fn((event: string, cb: any) => { + if (event === 'close') setImmediate(() => cb(0)); + }), + }), }; }); @@ -91,7 +113,7 @@ import { GitHubService } from '../../main/services/GitHubService'; describe('GitHubService.isAuthenticated', () => { beforeEach(() => { - execCalls.length = 0; + execFileCalls.length = 0; issueListStdout = '[]'; issueSearchStdout = '[]'; prListStdout = '[]'; @@ -108,8 +130,11 @@ describe('GitHubService.isAuthenticated', () => { const result = await service.isAuthenticated(); expect(result).toBe(true); - expect(execCalls.find((cmd) => cmd.startsWith('gh auth status'))).toBeDefined(); - expect(execCalls.find((cmd) => cmd.startsWith('gh auth token'))).toBeUndefined(); + expect( + execFileCalls.find( + (c) => c.file === 'gh' && c.args.includes('auth') && c.args.includes('status') + ) + ).toBeDefined(); expect(setPasswordMock).not.toHaveBeenCalled(); }); @@ -152,8 +177,19 @@ describe('GitHubService.isAuthenticated', () => { expect(result.totalCount).toBe(42); expect(result.prs.map((pr) => pr.number)).toEqual([9, 8]); expect( - execCalls.find((cmd) => cmd.startsWith('gh pr list --state open --limit 10')) + execFileCalls.find( + (c) => + c.file === 'gh' && + c.args.includes('pr') && + c.args.includes('list') && + c.args.includes('--limit') && + c.args.includes('10') + ) + ).toBeDefined(); + expect( + execFileCalls.find( + (c) => c.file === 'gh' && c.args.includes('api') && c.args.includes('search/issues') + ) ).toBeDefined(); - expect(execCalls.find((cmd) => cmd.startsWith('gh api search/issues'))).toBeDefined(); }); });