Skip to content

fix(security): replace shell exec with execFile to prevent command injection #1363

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
bhaktofmahakal wants to merge 8 commits into
base: main
Choose a base branch
from
Open

fix(security): replace shell exec with execFile to prevent command injection #1363

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0e27509
Initial plan
Copilot Mar 9, 2026
efb1e7e
fix(security): replace shell exec with execFile to prevent command in…
Copilot Mar 9, 2026
1da1111
fix(security): replace shell exec with execFile to prevent command in…
Copilot Mar 9, 2026
61c9e14
fix(security): check exit code in logout spawn handler per PR review …
Copilot Mar 9, 2026
1b255e6
fix(security): check exit code in logout spawn handler per PR review …
Copilot Mar 9, 2026
c168d3f
merge: resolve conflicts with upstream/main
Copilot Mar 10, 2026
9e2e9fa
fix(security): convert getPullRequestBaseDiff handler from exec to ex…
Copilot Mar 10, 2026
ab891c1
Merge remote-tracking branch 'origin/bhaktofmahakal/fix-shell-injecti…
Copilot Mar 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions src/main/services/DatabaseService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -710,7 +711,7 @@ export class DatabaseService {
input: Omit<LineCommentInsert, 'id' | 'createdAt' | 'updatedAt'>
): Promise<string> {
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,
Expand Down Expand Up @@ -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
Expand Down
131 changes: 84 additions & 47 deletions src/main/services/GitHubService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -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;
Expand Down Expand Up @@ -426,7 +426,7 @@ export class GitHubService {
private async authenticateGHCLI(token: string): Promise<void> {
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
Expand Down Expand Up @@ -456,14 +456,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),
Expand All @@ -477,7 +478,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),
Expand Down Expand Up @@ -509,7 +510,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 || '[]');
Expand Down Expand Up @@ -544,7 +554,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 || '[]');
Expand Down Expand Up @@ -582,7 +603,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');
Expand Down Expand Up @@ -658,7 +679,7 @@ export class GitHubService {
private async isGHCLIAuthenticated(): Promise<boolean> {
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
Expand Down Expand Up @@ -688,7 +709,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);
}

Expand Down Expand Up @@ -732,9 +753,14 @@ export class GitHubService {
async getRepositories(_token: string): Promise<GitHubRepo[]> {
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) => ({
Expand Down Expand Up @@ -776,9 +802,10 @@ export class GitHubService {
'headRepositoryOwner',
'headRepository',
];
const { stdout } = await this.execGH(`gh pr list --state open --json ${fields.join(',')}`, {
cwd: projectPath,
});
const { stdout } = await this.execGH(
['pr', 'list', '--state', 'open', '--json', fields.join(',')],
{ cwd: projectPath }
);
const list = JSON.parse(stdout || '[]');

if (!Array.isArray(list)) return [];
Expand Down Expand Up @@ -815,7 +842,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();
Expand All @@ -825,17 +852,16 @@ export class GitHubService {
}

try {
await this.execGH(
`gh pr checkout ${JSON.stringify(String(prNumber))} --branch ${JSON.stringify(safeBranch)} --force`,
{ cwd: projectPath }
);
await this.execGH(['pr', 'checkout', String(prNumber), '--branch', safeBranch, '--force'], {
cwd: projectPath,
});
} catch (error) {
console.error('Failed to checkout pull request branch via gh:', error);
throw error;
} finally {
if (previousRef && previousRef !== safeBranch) {
try {
await execAsync(`git checkout ${JSON.stringify(previousRef)}`, { cwd: projectPath });
await execFileAsync('git', ['checkout', previousRef], { cwd: projectPath });
} catch (switchErr) {
console.warn('Failed to restore previous branch after PR checkout:', switchErr);
}
Expand Down Expand Up @@ -919,7 +945,7 @@ export class GitHubService {
*/
async checkRepositoryExists(owner: string, name: string): Promise<boolean> {
try {
await this.execGH(`gh repo view ${owner}/${name}`);
await this.execGH(['repo', 'view', `${owner}/${name}`]);
return true;
} catch {
return false;
Expand All @@ -932,7 +958,7 @@ export class GitHubService {
async getOwners(): Promise<Array<{ login: string; type: 'User' | 'Organization' }>> {
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' }> = [
Expand All @@ -941,7 +967,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) {
Expand Down Expand Up @@ -972,22 +998,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 {
Expand Down Expand Up @@ -1026,15 +1054,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');
Expand All @@ -1060,8 +1088,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) {
Expand All @@ -1079,9 +1107,18 @@ export class GitHubService {
async logout(): Promise<void> {
// 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<void>((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', () => 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 () => {
Expand Down
Loading
Loading