From f1cea4792b7542fa2af754a7a65b173c7d96ef36 Mon Sep 17 00:00:00 2001 From: Baker B Date: Tue, 5 May 2026 21:08:55 -0400 Subject: [PATCH] fix(security): use runArgv in enqueuePullRequestViaGraphQL to eliminate shell injection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The enqueuePullRequestViaGraphQL helper interpolated 'repo' and 'prId' values into shell command strings via execSync. While 'prId' from the GitHub API is opaque base64 in practice, no validation enforced this contract — and 'repo' flowed through from caller args without adapter-level shell-safety guarantees on this code path. Replace string-template interpolation with runArgv (argv-array form, shell-escapes each element). This is the established pattern in lib/adapters/ subprocess code (label-create-github.ts, pr-diff-gitlab.ts, etc.) and removes the shell-injection surface entirely. Also adds a regression test that injects a node_id containing '"; echo PWNED; #' and verifies the dangerous value remains contained within a single shell-quoted argv token (never escapes to bare shell). --- lib/adapters/pr-merge-github.test.ts | 68 ++++++++++++++++++++++++++-- lib/adapters/pr-merge-github.ts | 43 ++++++++++++------ 2 files changed, 93 insertions(+), 18 deletions(-) diff --git a/lib/adapters/pr-merge-github.test.ts b/lib/adapters/pr-merge-github.test.ts index 58d7fff..d63cf08 100644 --- a/lib/adapters/pr-merge-github.test.ts +++ b/lib/adapters/pr-merge-github.test.ts @@ -461,15 +461,77 @@ describe('prMergeGithub — subprocess boundary', () => { expect(result.data.queue_position).toBe(3); // Verify both --auto and GraphQL calls were made const autoCall = execCalls.find( - (c) => c.includes('gh pr merge 284') && c.includes('--auto'), + (c) => unquote(c).includes('gh pr merge 284') && unquote(c).includes('--auto'), ); expect(autoCall).toBeDefined(); - const nodeIdCall = execCalls.find((c) => c.includes('gh api repos/org/repo/pulls/284')); + const nodeIdCall = execCalls.find((c) => + unquote(c).includes('gh api repos/org/repo/pulls/284'), + ); expect(nodeIdCall).toBeDefined(); const graphqlCall = execCalls.find( - (c) => c.includes('gh api graphql') && c.includes('enqueuePullRequest'), + (c) => + unquote(c).includes('gh api graphql') && unquote(c).includes('enqueuePullRequest'), + ); + expect(graphqlCall).toBeDefined(); + }); + + test('pr-merge-github — GraphQL fallback shell-escapes repo and prId (security)', async () => { + // Regression: enqueuePullRequestViaGraphQL must use argv-array form + // (runArgv) so that special characters in `repo` or `prId` cannot break + // out of the shell command. We verify by stubbing a node_id with chars + // that would terminate a string-template'd command. + on( + 'enqueuePullRequest', + JSON.stringify({ + data: { enqueuePullRequest: { mergeQueueEntry: { position: 7 } } }, + }), + ); + stubNoQueue(); + on('gh pr merge 286 --squash --delete-branch', () => { + throw mergeQueueError(); + }); + on('gh pr merge 286 --squash --delete-branch --auto', () => { + const err = new Error('Auto merge is not allowed for this repository') as ThrowableError; + err.stderr = 'Auto merge is not allowed for this repository\n'; + throw err; + }); + // node_id contains characters that WOULD break out of a quoted shell value + on( + 'gh api repos/org/repo/pulls/286', + JSON.stringify({ node_id: `PR_kw"; echo PWNED; #` }), + ); + on( + 'gh pr view 286 --json state,url,mergeCommit', + JSON.stringify({ + state: 'OPEN', + url: 'https://github.com/org/repo/pull/286', + mergeCommit: null, + }), + ); + + const result = await prMergeGithub({ number: 286, repo: 'org/repo' }); + expectOk(result); + // The dangerous prId value should appear inside single-quoted argv + // elements, never as bare shell text. With shell-escape, every `'` in + // the value is replaced by `'\''` and the value is wrapped in `'...'`. + const graphqlCall = execCalls.find( + (c) => c.includes('gh') && c.includes('graphql') && c.includes('PWNED'), ); expect(graphqlCall).toBeDefined(); + // Shell-escape contract: every argv element is wrapped in `'...'` and + // any embedded single quote is rewritten as `'\''`. The dangerous value + // (including spaces and `; echo PWNED; #`) must be contained inside one + // single-quoted region. We verify via a regex that captures the entire + // single-quoted prId argv element. + const prIdMatch = graphqlCall!.match(/'(prId=[^']*)'/); + expect(prIdMatch).not.toBeNull(); + expect(prIdMatch![1]).toContain('PWNED'); + // Confirm there are no UNQUOTED `;` chars (which would let the shell run + // the rest as a separate command). Strip all single-quoted regions and + // the residual must not contain `;`. + const residual = graphqlCall!.replace(/'[^']*'/g, ''); + expect(residual).not.toContain(';'); + expect(residual).not.toContain('echo'); }); test('pr-merge-github — direct merge success has graphql_fallback:false', async () => { diff --git a/lib/adapters/pr-merge-github.ts b/lib/adapters/pr-merge-github.ts index 1076593..4f2bdd1 100644 --- a/lib/adapters/pr-merge-github.ts +++ b/lib/adapters/pr-merge-github.ts @@ -21,6 +21,7 @@ import { execSync } from 'child_process'; import { writeFileSync } from 'fs'; import { detectMergeQueue, type MergeQueueInfo } from '../merge_queue_detect.js'; import { parseRepoSlug } from '../shared/parse-repo-slug.js'; +import { runArgv } from '../shared/error-norm.js'; import { getAdapter } from './index.js'; import type { AdapterResult, @@ -81,35 +82,47 @@ function enqueuePullRequestViaGraphQL( number: number, repo: string | undefined, ): number | null { - // Step 1: fetch the PR's node_id. When repo is undefined, gh uses the - // current directory's remote. When repo is provided, we must include it in - // both the API path and --repo flag. - let nodeIdCmd: string; + // Use runArgv (argv-array form, each element shell-escaped) instead of + // string-template interpolation to eliminate shell-injection surface on + // `repo` and `prId` values. Both flow from external sources (caller-supplied + // repo arg, GitHub API response) and must not be trusted as shell-safe. + const cwd = process.env.CLAUDE_PROJECT_DIR ?? process.cwd(); + + // Step 1: fetch the PR's node_id. + let nodeIdArgv: string[]; if (repo !== undefined) { - nodeIdCmd = `gh api repos/${repo}/pulls/${number} --repo ${repo} --jq '.node_id'`; + nodeIdArgv = ['gh', 'api', `repos/${repo}/pulls/${number}`, '--repo', repo, '--jq', '.node_id']; } else { // When repo is undefined, resolve via the slug parser (same logic as - // detectMergeQueue). If that fails, let gh api infer from cwd. + // detectMergeQueue). If that fails, fall back to `gh pr view` which infers + // the repo from cwd's remote. const slug = parseRepoSlug(); if (slug !== null) { - nodeIdCmd = `gh api repos/${slug}/pulls/${number} --jq '.node_id'`; + nodeIdArgv = ['gh', 'api', `repos/${slug}/pulls/${number}`, '--jq', '.node_id']; } else { - // Last resort: gh api without explicit repo path — will fail if cwd isn't - // a repo, but that's a caller error (same as other gh invocations). - nodeIdCmd = `gh pr view ${number} --json id --jq '.id'`; + nodeIdArgv = ['gh', 'pr', 'view', String(number), '--json', 'id', '--jq', '.id']; } } - const prId = exec(nodeIdCmd).trim(); + const nodeIdResult = runArgv(nodeIdArgv, cwd); + if (nodeIdResult.exitCode !== 0) { + throw new Error(`gh node_id lookup failed: ${nodeIdResult.stderr || nodeIdResult.stdout}`); + } + const prId = nodeIdResult.stdout.trim(); // Step 2: invoke the enqueuePullRequest mutation const mutation = `mutation($prId:ID!){enqueuePullRequest(input:{pullRequestId:$prId}){mergeQueueEntry{position}}}`; - const repoFlag = repo !== undefined ? `--repo ${repo}` : ''; - const graphqlCmd = `gh api graphql -f query='${mutation}' -f prId="${prId}" ${repoFlag}`.trim(); - const response = exec(graphqlCmd); + const graphqlArgv = ['gh', 'api', 'graphql', '-f', `query=${mutation}`, '-f', `prId=${prId}`]; + if (repo !== undefined) { + graphqlArgv.push('--repo', repo); + } + const graphqlResult = runArgv(graphqlArgv, cwd); + if (graphqlResult.exitCode !== 0) { + throw new Error(`gh graphql enqueue failed: ${graphqlResult.stderr || graphqlResult.stdout}`); + } // Parse the response to extract the queue position try { - const data = JSON.parse(response); + const data = JSON.parse(graphqlResult.stdout); return data?.data?.enqueuePullRequest?.mergeQueueEntry?.position ?? null; } catch { return null;