fix(scripts): remove shell and argument injection from git calls (API-511) - #6909
Merged
Conversation
Runs git through execa without a shell, so callers pass untrusted values as argv elements rather than interpolating them into a command string. Returns stdout only; run() returns stdout and stderr merged, which would splice git diagnostics into any value read through it.
The codegen commit built `git commit -m "<message>"` as a shell string from
attacker-controlled git metadata (commit subject, author name/email and
Co-authored-by trailers). It escaped only " and `, leaving $(...) and ${...}
live, and a backslash before a backtick defeated the backtick escape.
Read the subject, author and trailers as separate values via git(), assemble
the message in JS, and commit through gitCommit(). Branch-name interpolations
in the same function are converted at the same time.
The resulting commit message is byte-identical for benign input, on both main
and generated PR branches.
Argv form stops bash from reinterpreting untrusted git metadata, but git itself still reads a leading dash as an option. A branch named -evil is creatable via git update-ref and pushable to a remote, and passing it bare to a git command turns it into a flag: --output=<file> writes an arbitrary file, --upload-pack=<cmd> executes a command. Validate the checked-out branch once, and pass --end-of-options ahead of the revision in each git show. --end-of-options does not cover git pull, which re-parses its arguments in an internal git fetch, so the validation is what protects that path. A dash-leading branch now fails the job rather than being processed. Such a name cannot be created by git branch, only by plumbing.
The diff check interpolated an attacker-controlled branch name into a shell
pipeline with no escaping at all, and it runs before the commit path.
Compose the command in JS instead. Four behaviour notes:
- the revision is omitted rather than passed empty. Argv form makes an empty
string a hard error ("bad revision"), where the shell collapsed it to
nothing; three call sites pass { head: null } with no branch.
- path now accepts a list. The shell was word-splitting a space-joined string
back into separate pathspecs, so isBaseChanged passes its array through
directly. Joining it would have silently broken pathspec scoping in both
directions, depending on whether the entry leads with an exclusion.
- a failing git diff now throws instead of returning 0. The old pipeline took
its exit status from tr, so any git failure was read as "no changes" and
the push was skipped quietly.
- the return value is still 0 or 1, matching wc -l; git diff --shortstat
emits a single summary line regardless of how many files changed.
getLastReleasedTag() returns a tag name read out of the repo, and both of its
call sites interpolated it into a shell string with no escaping at all. Tag
names follow the same rules as branch names, so a tag matching the "released*"
prefix can carry a command substitution:
git tag 'released-2026$(touch${IFS}/tmp/mark)'
git describe returns that verbatim, and both interpolations then execute it.
This path runs in the release pipeline, which holds the broadest token.
Read the tag through argv-form git(), validate it, and pass --end-of-options
ahead of the revision.
Also routes the prepare-release commit through gitCommit(), which the ticket
asks for. That line was never exploitable, since its message is a constant
plus a date, but it was the last `git commit -m` string interpolation left.
It needs CI=true carried into the child process rather than written as a
shell prefix: that variable is what suppresses the pre-commit hook that
unstages generated files, so dropping it would silently produce release
commits missing every generated file.
Adds errorMessage to git() and env to gitCommit(), each for one call site.
Contributor
No code generatedIf you believe code should've been generated, please, report the issue. 📊 Benchmark resultsBenchmarks performed on the method using a mock server, the results might not reflect the real-world performance.
|
Summary by Aikido
⚡ Enhancements
🔧 Refactors
|
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
These assert on the argv arrays rather than on output, because where each value lands is the whole point of the fix. - git() calls execa with no shell option, and returns stdout rather than the merged stdout+stderr stream - assertSafeRef returns ordinary refs unchanged and rejects option-shaped ones - pushGeneratedCode commits through gitCommit() with subject, author and trailers as separate values, so a $(...) payload survives verbatim - getNbGitDiff omits an empty revision rather than passing '', and spreads a pathspec list into separate argv elements Mutation-checked, each against the restored tree: dropping the assertSafeRef guard fails 1 test, bypassing gitCommit fails 4, always passing the revision fails 3, joining the pathspecs fails 1.
…g (API-511) The pushGeneratedCode tests ran the real configureGitHubAuthor, writing the bot identity into .git/config on every run; it is now mocked, and the obsolete run mock is dropped. Also from review: - assertSafeRef rejects empty refs, as returned for a detached HEAD - git() logs the failure output under verbose even with allowFailure, matching the old `|| true` behaviour - prepareGitEnvironment pulls via the git() helper instead of a shell substitution
eric-zaharia
force-pushed
the
fix/api-511-shell-injection
branch
from
August 17, 2026 13:20
aef8b9f to
340d51c
Compare
…PI-511) From review: - git() with errorMessage logs the failure output under verbose before throwing, like the allowFailure path - the co-author trailers are read with unfold and filtered on the Co-authored-by: prefix, so a folded trailer cannot become a malformed entry in the commit message - tests cover the pushGeneratedCode early returns (no changes, behind origin), the release-commit message override, cwd forwarding in getNbGitDiff, and the add-before-diff ordering
MarioAlexandruDan
approved these changes
Aug 18, 2026
eric-zaharia
enabled auto-merge (squash)
August 18, 2026 06:51
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.
🧭 What and Why
🎟 JIRA Ticket: API-511
pushGeneratedCodebuiltgit commit -m "<message>"as a bash string from attacker-controlled git metadata (commit subject, author name/email,Co-authored-bytrailers), escaping only"and`— so$(...)and${...}executed on the runner. Two other paths interpolate a branch or tag name into a shell with no escaping at all. Argv form alone is not enough either: git reads a ref beginning with-as an option, andrefs/heads/-evilis creatable viagit update-refand pushable —--output=then writes an arbitrary file,--upload-pack=runs a command.Changes included:
git()helper. The codegen commit goes through the existinggitCommit(), with subject, author and trailers read as separate values and the message assembled in JS instead of by git.assertSafeRefrejects empty refs and refs starting with-, and--end-of-optionsprecedes untrusted revisions.git pullre-parses its arguments in an internalgit fetch, so only the validation protects that path.getNbGitDiffno longer uses a shell. Itspathnow takes a list, because the shell was word-splitting a joined string into separate pathspecs;isBaseChangedpasses its array through.getLastReleasedTag()'s twocreateReleasePRcall sites are validated and shell-free, as is thegit pull origin $(git branch --show-current)inprepareGitEnvironment. A tag matchingreleased*could carry$(...), and that path runs in the release job.gitCommit({ env: { CI: 'true' } }). That variable suppresses the pre-commit hook that unstages generated files, so it has to reach the child process rather than stay a shell prefix.Shell-form git calls that only interpolate values from this repo's own config (language and repository names) — in
pushToRepository,spreadGenerationand the clone helpers — are unchanged.Two behaviour changes worth a look:
git diffnow throws instead of returning0. The old pipeline took its exit status fromtr, so git failures read as "no changes" and jobs were skipped silently — including on the zero SHAgithub.event.beforereports after a force-push.git branch/git tag, only by plumbing.🧪 Test
assertSafeReffails 1 test, bypassinggitCommitfails 4, always passing the revision fails 3, joining the pathspecs fails 1.mainand generated branches. With a payload, the old path executed 3 of 4 injections and blanked the author name; the new one stores all four verbatim.--end-of-optionsneutralises--output=, andCI=truekeeps generated files in the commit.