fix(security): eliminate shell injection in pr_merge GraphQL fallback#403
Merged
bakeb7j0 merged 1 commit intoMay 6, 2026
Merged
Conversation
…te shell injection 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).
bakeb7j0
added a commit
that referenced
this pull request
May 6, 2026
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, ''). Co-authored-by: Baker B <bakerb@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Trust-score gate hotfix for kahuna/390-bug-drain. Eliminates a shell-injection surface in
enqueuePullRequestViaGraphQL(introduced in PR #394 / issue #284).The function previously interpolated
repoandprIdvalues into shell command strings viaexecSync. WhileprId(from the GitHub API) is opaque base64 in practice, no validation enforced this — andrepoflowed from caller args without adapter-level shell-safety guarantees.Changes
lib/adapters/pr-merge-github.ts— replaced string-template interpolation withrunArgv(argv-array form, shell-escapes each element). This matches the established pattern inlib/adapters/(label-create-github.ts, pr-diff-gitlab.ts, etc.).lib/adapters/pr-merge-github.test.ts— added regression test injecting a maliciousnode_idcontaining"; echo PWNED; #and verifying the dangerous value stays contained within a single shell-quoted argv token.Tests
pr-merge-github.test.ts(existing 14 + 2 new — the GraphQL fallback test was updated to use the test'sunquotehelper for argv-form matching)./scripts/ci/validate.shgreen (76/76 tools)Trust-score gate context
This is the only critical finding from the kahuna→main code-reviewer pass. The 3 important findings (work_item_update inline schema, plan_load_dod missing schema, wave_init Step 4 atomicity gap) are tracked as separate follow-up issues per the plan-A path.
Closes #390 (gate hotfix)