From 38f72bbd2ff6046dae301f3dab5fa37b51d8560f Mon Sep 17 00:00:00 2001 From: Kalin Ovtcharov Date: Thu, 16 Apr 2026 14:41:12 -0700 Subject: [PATCH] fix: close remaining shell-injection vectors in github-issues and git-diff MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #38 migrated most exec() → execFile() but left four sites with user-controlled input interpolated into shell strings: - /api/workspaces/git-diff — filePath was wrapped in quotes only; shell metacharacters in the query parameter were interpreted. - /api/workspaces/github-issues (GET) — state and assignee from the query string flowed unvalidated into the gh CLI invocation. - /api/workspaces/github-issues (POST) — title and body only had " escaped; backticks, \$(), and backslashes could still inject. - /api/workspaces/github-issues/:num (PATCH) — subcommand path migrated as defense in depth. state is now allowlisted (open|closed|all) and assignee is restricted to @me or a valid GitHub username pattern before reaching gh. Also add .catch() handlers to the post-spawn learnings and git-state promises in task-spawner so a rejection no longer surfaces as an unhandledRejection. Co-Authored-By: Claude Opus 4.7 (1M context) --- backend/src/server.ts | 59 ++++++++++++++++++++++++++++--------- backend/src/task-spawner.ts | 4 +++ 2 files changed, 49 insertions(+), 14 deletions(-) diff --git a/backend/src/server.ts b/backend/src/server.ts index 762c9e6..a4812ee 100644 --- a/backend/src/server.ts +++ b/backend/src/server.ts @@ -3863,13 +3863,30 @@ export async function createApp(basePath?: string) { body: string; } + // Validate state against an allowlist before passing to gh + if (!['open', 'closed', 'all'].includes(state)) { + return res.status(400).json({ error: 'state must be "open", "closed", or "all"' }); + } + // Validate assignee: allow @me or a GitHub username (alphanumerics + dashes, max 39 chars) + if (assignee && assignee !== '@me' && !/^[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,38})$/.test(assignee)) { + return res.status(400).json({ error: 'Invalid assignee' }); + } + let issues: GitHubIssue[] = []; try { - const assigneeArg = assignee ? `--assignee ${assignee}` : ''; - const { stdout } = await execAsync( - `gh issue list --repo ${owner}/${repo} --state ${state} ${assigneeArg} --limit ${limit} --json number,title,state,url,createdAt,updatedAt,closedAt,author,assignees,labels,comments,body`, - { cwd: workspacePath } - ); + const { execFile } = await import('child_process'); + const execFileAsync = (await import('util')).promisify(execFile); + const ghArgs = [ + 'issue', 'list', + '--repo', `${owner}/${repo}`, + '--state', state, + '--limit', String(limit), + '--json', 'number,title,state,url,createdAt,updatedAt,closedAt,author,assignees,labels,comments,body', + ]; + if (assignee) { + ghArgs.push('--assignee', assignee); + } + const { stdout } = await execFileAsync('gh', ghArgs, { cwd: workspacePath }); issues = JSON.parse(stdout); } catch (err) { const errorMsg = err instanceof Error ? err.message : String(err); @@ -3984,11 +4001,20 @@ export async function createApp(basePath?: string) { // Create issue using gh CLI try { + const { execFile } = await import('child_process'); + const execFileAsync = (await import('util')).promisify(execFile); // gh CLI requires --body when running non-interactively, so provide empty string if not given const bodyText = body || ''; // Auto-assign to current user (@me) - const { stdout } = await execAsync( - `gh issue create --repo ${owner}/${repo} --title "${title.replace(/"/g, '\\"')}" --body "${bodyText.replace(/"/g, '\\"')}" --assignee @me`, + const { stdout } = await execFileAsync( + 'gh', + [ + 'issue', 'create', + '--repo', `${owner}/${repo}`, + '--title', title, + '--body', bodyText, + '--assignee', '@me', + ], { cwd: workspacePath } ); // gh issue create returns the URL of the created issue @@ -4099,9 +4125,12 @@ export async function createApp(basePath?: string) { // Close or reopen the issue using gh CLI try { - const stateArg = state === 'closed' ? '--close' : '--reopen'; - await execAsync( - `gh issue ${state === 'closed' ? 'close' : 'reopen'} ${issueNumber} --repo ${owner}/${repo}`, + const { execFile } = await import('child_process'); + const execFileAsync = (await import('util')).promisify(execFile); + const subcommand = state === 'closed' ? 'close' : 'reopen'; + await execFileAsync( + 'gh', + ['issue', subcommand, String(issueNumber), '--repo', `${owner}/${repo}`], { cwd: workspacePath } ); @@ -4245,10 +4274,12 @@ export async function createApp(basePath?: string) { // Get the diff let diff = ''; try { - const command = staged - ? `git diff --cached -- "${filePath}"` - : `git diff -- "${filePath}"`; - const { stdout } = await execAsync(command, { cwd: workspacePath, maxBuffer: 10 * 1024 * 1024 }); + const { execFile } = await import('child_process'); + const execFileAsync = (await import('util')).promisify(execFile); + const args = staged + ? ['diff', '--cached', '--', filePath] + : ['diff', '--', filePath]; + const { stdout } = await execFileAsync('git', args, { cwd: workspacePath, maxBuffer: 10 * 1024 * 1024 }); diff = stdout; } catch (err) { logger.error('Failed to get git diff', { error: err instanceof Error ? err.message : String(err) }); diff --git a/backend/src/task-spawner.ts b/backend/src/task-spawner.ts index 5466823..0c5d1a1 100644 --- a/backend/src/task-spawner.ts +++ b/backend/src/task-spawner.ts @@ -1810,6 +1810,8 @@ export class TaskSpawner extends EventEmitter { }); this.taskLearnings.set(task.id, injectedLearnings.map(l => l.learning.id)); } + }).catch(err => { + logger.warn('Learnings promise rejected (post-spawn)', { taskId: task.id, error: err instanceof Error ? err.message : String(err) }); }); return task; @@ -2073,6 +2075,8 @@ You are running as an agent inside Claudia, a multi-agent orchestrator. You have task.gitStateBefore = gitStateBefore; logger.info(`Git state attached to task`, { taskId: id, commit: gitStateBefore.commitBefore?.substring(0, 7) }); } + }).catch(err => { + logger.warn('Git state promise rejected (post-spawn)', { taskId: id, error: err instanceof Error ? err.message : String(err) }); });