Skip to content

fix(tgit): shell-quote every gf CLI arg to prevent injection#60

Merged
jeff-r2026 merged 1 commit into
Tencent:mainfrom
hobostay:fix/gf-cli-shell-injection
Jun 28, 2026
Merged

fix(tgit): shell-quote every gf CLI arg to prevent injection#60
jeff-r2026 merged 1 commit into
Tencent:mainfrom
hobostay:fix/gf-cli-shell-injection

Conversation

@hobostay

Copy link
Copy Markdown
Contributor

Summary

gfExec (src/providers/tgit/gf-cli.ts) runs the gf CLI via bash -c "<gfPath> <args.join(' ')'>". Only the MR title and description were shell-quoted (in gfMrCreate); repo, source/target branch, reviewers, and the clone local-path were interpolated raw. A repo or branch value containing shell metacharacters (;, |, $(), backticks, …) was therefore executed by bash -c.

The values originate from user input (--repo, branch names, reviewers), so this is an argument-injection / arbitrary-command-execution vector.

Proof (real bash + fake gf)

gfMrCreate({ repo: "team/repo;echo PWNED", source: "feat", target: "master", title: "my title" })

Before — the constructed command is:

gf mr create -R team/repo;echo PWNED -s feat -T master -t 'my title'

bash -c prints PWNED … (the injected echo runs) and gf receives split args:

PWNED -s feat -T master -t my title      ← stdout (command executed)
ARG:[team/repo]                           ← gf only got the pre-`;` fragment

After — every token is single-quoted:

'/…/gf' 'mr' 'create' '-R' 'team/repo;echo PWNED' '-s' 'feat' '-T' 'master' '-t' 'my title'

nothing extra executes and gf receives the value as one literal argument:

ARG:[team/repo;echo PWNED]                ← single literal arg, no injection

Fix

Shell-quote every token (including the binary path) centrally in gfExec, and drop the now-redundant per-call shellQuote() in gfMrCreate:

const cmd = [shellQuote(gfPath), ...args.map(shellQuote)].join(' ');
// gfMrCreate — title/description are now quoted by gfExec
const args = ['mr', 'create', '-R', opts.repo, '-s', opts.source, '-T', opts.target, '-t', opts.title];
if (opts.description) args.push('-d', opts.description);

This keeps the existing bash -c execution mechanism (chosen for launcher compatibility) and covers every call site at once:

  • The two-segment gf repo clone path (gfExec(['repo','clone', repo, localPath])) is now safe.
  • The multi-segment clone path already used a spawnSync('git', [...]) argv array (no shell) — unchanged.

Tests

  • Added a regression test (gf-cli.test.ts) asserting repo/source/target metacharacters are single-quoted as single tokens, with no unquoted ;/| remaining for bash -c to interpret. Fails on the old code (team/repo;echo PWNED was unquoted).
  • Existing gf-cli.test.ts cases (newline preservation in description, single-quote escaping, MR-URL extraction) still pass — the title/description quoting result is unchanged.
  • npm test → 1440 passed; tsc --noEmit → clean.

Co-Authored-By: Claude noreply@anthropic.com

gfExec built `bash -c "<gfPath> <args.join(' ')>"`. Only the MR title and
description were shell-quoted (in gfMrCreate); repo, source/target branch,
reviewers, and the clone local-path were interpolated raw. A repo or
branch value containing shell metacharacters (`;`, `|`, `$()`, …) was
therefore executed by `bash -c`.

Example — gfMrCreate({ repo: "team/repo;echo PWNED", ... }) produced the
command:
  gf mr create -R team/repo;echo PWNED -s feat ...
which runs `echo PWNED` when gfExec spawns it. Verified with a real bash
invocation + fake gf: the old cmd executes the injected `echo` and gf
receives split args; the fixed cmd passes `team/repo;echo PWNED` as a
single literal argument and nothing extra runs.

Fix: shell-quote every token (including the binary path) centrally in
gfExec, and drop the now-redundant per-call shellQuote() in gfMrCreate.
The two-segment `gf repo clone` path through gfExec is covered by the
same change; the multi-segment git-clone path already used a spawnSync
argv array.

Added a regression test asserting repo/source/target metacharacters are
single-quoted as single tokens with no unquoted `;`/`|` left for bash.
Existing gf-cli tests (newline preservation, single-quote escaping, URL
extraction) still pass. `npm test` 1440 passed; `tsc --noEmit` clean.

Co-Authored-By: Claude <noreply@anthropic.com>
@jeff-r2026 jeff-r2026 merged commit 7e0a07e into Tencent:main Jun 28, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants