From e15b730ea6e6b7171b44c1d69321f4805cdc0969 Mon Sep 17 00:00:00 2001 From: Baker B Date: Tue, 5 May 2026 21:15:46 -0400 Subject: [PATCH] fix(security): shell-escape repo arg in buildGithubMergeCommand MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sibling fix to PR #403. The original critical finding noted both repoFlag in enqueuePullRequestViaGraphQL AND the repo arg in buildGithubMergeCommand. PR #403 fixed only the former (via runArgv); this PR closes the buildGithubMergeCommand path with shellEscape() — matching the existing pattern for squashMessage and tempFile two lines above. Adds a regression test that injects 'sec/repo'\''; echo PWNED; #' as the repo arg and verifies the dangerous chars only appear inside single-quoted argv tokens (or inside the '\\'' escape sequence). Updates two existing tests that asserted the unquoted form to use unquote() / .replace(/'/g, ''). --- lib/adapters/pr-merge-github.test.ts | 43 ++++++++++++++++++++++++++-- lib/adapters/pr-merge-github.ts | 2 +- tests/pr_merge.test.ts | 6 ++-- 3 files changed, 46 insertions(+), 5 deletions(-) diff --git a/lib/adapters/pr-merge-github.test.ts b/lib/adapters/pr-merge-github.test.ts index d63cf08..dfd306e 100644 --- a/lib/adapters/pr-merge-github.test.ts +++ b/lib/adapters/pr-merge-github.test.ts @@ -305,14 +305,53 @@ describe('prMergeGithub — subprocess boundary', () => { }); expectOk(result); const mergeCall = findCall('gh pr merge 42'); - expect(mergeCall).toContain('--repo Wave-Engineering/mcp-server-sdlc'); + expect(unquote(mergeCall!)).toContain('--repo Wave-Engineering/mcp-server-sdlc'); const viewCall = findCall('gh pr view 42'); - expect(viewCall).toContain('--repo Wave-Engineering/mcp-server-sdlc'); + expect(unquote(viewCall!)).toContain('--repo Wave-Engineering/mcp-server-sdlc'); const graphqlCall = findCall('gh api graphql'); expect(graphqlCall).toContain('-F owner=Wave-Engineering'); expect(graphqlCall).toContain('-F name=mcp-server-sdlc'); }); + test('pr-merge-github — buildGithubMergeCommand shell-escapes repo (security)', async () => { + // Defence-in-depth: even though the schema layer rejects shell metachars + // in `repo`, the adapter must shell-escape before joining argv into a + // shell command. The test passes a value that bypasses schema validation + // (we call prMergeGithub directly, not the handler) and verifies the + // resulting command contains the dangerous chars only inside a single + // shell-quoted token. + on('git remote get-url origin', 'https://github.com/cwd-org/cwd-repo.git\n'); + stubNoQueue(); + on('gh pr merge 99', ''); + on( + 'gh pr view 99', + JSON.stringify({ + state: 'MERGED', + url: 'https://github.com/sec/repo/pull/99', + mergeCommit: { oid: 'abc' }, + }), + ); + + // Hostile repo value with shell-injection characters + const hostileRepo = `sec/repo'; echo PWNED; #`; + await prMergeGithub({ number: 99, repo: hostileRepo }); + + const mergeCall = execCalls.find((c) => c.includes('gh pr merge 99')); + expect(mergeCall).toBeDefined(); + // shellEscape wraps each argv element in `'...'` and rewrites embedded + // single quotes as `'\''` (close + escape + reopen). The hostile value + // becomes `'sec/repo'\''; echo PWNED; #'` — three safe shell tokens + // joined by an escaped quote, treated as ONE argv element by the shell. + // Strip both `'\''` sequences and `'...'` regions; no shell-active chars + // should remain in the residual. + const stripped = mergeCall! + .replace(/'\\''/g, '') // remove escaped-quote sequences + .replace(/'[^']*'/g, ''); // remove single-quoted regions + expect(stripped).not.toContain(';'); + expect(stripped).not.toContain('echo'); + expect(stripped).not.toContain('PWNED'); + }); + // ========================================================================= // Bug #280 / Story 2.0 (#294) — skip_train + queue-strategy error fallback // ========================================================================= diff --git a/lib/adapters/pr-merge-github.ts b/lib/adapters/pr-merge-github.ts index 4f2bdd1..47bf708 100644 --- a/lib/adapters/pr-merge-github.ts +++ b/lib/adapters/pr-merge-github.ts @@ -158,7 +158,7 @@ function buildGithubMergeCommand( } } if (repo !== undefined) { - parts.push('--repo', repo); + parts.push('--repo', shellEscape(repo)); } return parts.join(' '); } diff --git a/tests/pr_merge.test.ts b/tests/pr_merge.test.ts index 2aef088..8940cad 100644 --- a/tests/pr_merge.test.ts +++ b/tests/pr_merge.test.ts @@ -632,9 +632,11 @@ describe('pr_merge handler — aggregate response (#225)', () => { expect(data.ok).toBe(true); const mergeCall = execCalls.find((c) => c.startsWith('gh pr merge 42')) ?? ''; - expect(mergeCall).toContain('--repo Wave-Engineering/mcp-server-sdlc'); + // shellEscape wraps the repo arg in single quotes; strip them for the + // contains check. + expect(mergeCall.replace(/'/g, '')).toContain('--repo Wave-Engineering/mcp-server-sdlc'); const viewCall = execCalls.find((c) => c.startsWith('gh pr view 42')) ?? ''; - expect(viewCall).toContain('--repo Wave-Engineering/mcp-server-sdlc'); + expect(viewCall.replace(/'/g, '')).toContain('--repo Wave-Engineering/mcp-server-sdlc'); // Queue detection should also use the explicit repo, not the cwd remote. const graphqlCall = execCalls.find((c) => c.includes('gh api graphql')) ?? ''; expect(graphqlCall).toContain('-F owner=Wave-Engineering');