From 0cb3e56c8f60f6de8d3e07b4e7144bc8a2aef59e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Greg=20Berg=C3=A9?= Date: Fri, 11 Sep 2026 16:03:07 +0200 Subject: [PATCH] fix(core): prevent git option injection via CI branch names (GHSA-v58q-fvq4-9vh4) The git helpers pass the branch and commit of the CI environment to git as positional arguments. Git parses any argument starting with a dash as an option, so a pull request from a branch named `--upload-pack=` (a valid, pushable ref name) made `git fetch` run `` through a shell on ssh:// and file:// origins whenever the merge base is resolved locally, i.e. for projects without a connected Git provider. The fix for GHSA-4x45-gxvp-6283 removed the shell but left this option parsing. Pass every CI-provided value after `--end-of-options` (git 2.24+) in the fetch, merge-base, rev-parse, log and rev-list invocations, so git always treats it as a ref. The fetch helper now assembles the argument list itself so no caller can leave the separator out. Add regression tests feeding the advisory's payload through the merge base, the ancestor listing and the commit parents, plus one checking that a branch legitimately named with a leading dash is still fetched. Co-Authored-By: Claude Fable 5.1 --- packages/core/src/ci-environment/git.test.ts | 83 ++++++++++++++++++-- packages/core/src/ci-environment/git.ts | 74 +++++++++++------ 2 files changed, 125 insertions(+), 32 deletions(-) diff --git a/packages/core/src/ci-environment/git.test.ts b/packages/core/src/ci-environment/git.test.ts index 7e44fd98..387659c6 100644 --- a/packages/core/src/ci-environment/git.test.ts +++ b/packages/core/src/ci-environment/git.test.ts @@ -35,34 +35,59 @@ describe.skip("#getRepositoryURL", () => { }); }); -describe("#getMergeBaseCommitSha (command injection)", () => { +describe("injection through the branch or commit of the CI environment", () => { let cwd: string; + let root: string; let repoDir: string; let markerFile: string; + /** SHA of the tip of `main`, pushed to origin. */ + let mainSha: string; + let warn: ReturnType; + + const git = (...args: string[]) => + execFileSync("git", ["-C", repoDir, ...args]) + .toString() + .trim(); beforeEach(() => { cwd = process.cwd(); - const root = mkdtempSync(join(tmpdir(), "argos-git-test-")); + root = mkdtempSync(join(tmpdir(), "argos-git-injection-test-")); repoDir = join(root, "repo"); markerFile = join(root, "pwned"); // A bare repo acts as the "origin" remote so that git fetch has a - // reachable target. + // reachable target. A local path goes through git's file transport, which + // runs `--upload-pack` through a shell like the ssh transport does. const bareDir = join(root, "origin.git"); execFileSync("git", ["init", "--bare", bareDir]); execFileSync("git", ["init", repoDir]); - const git = (...args: string[]) => - execFileSync("git", ["-C", repoDir, ...args]); git("remote", "add", "origin", bareDir); + git("config", "user.email", "test@argos-ci.com"); + git("config", "user.name", "Argos Test"); + git("commit", "--allow-empty", "-m", "initial"); + git("branch", "-M", "main"); + git("push", "origin", "main"); + mainSha = git("rev-parse", "HEAD"); + warn = vi.spyOn(console, "warn").mockImplementation(() => {}); process.chdir(repoDir); }); afterEach(() => { process.chdir(cwd); - rmSync(join(repoDir, ".."), { recursive: true, force: true }); + warn.mockRestore(); + rmSync(root, { recursive: true, force: true }); }); + /** + * Mirrors the GHSA-v58q-fvq4-9vh4 PoC: a ref name starting with a dash is + * accepted by `git check-ref-format` and can be pushed, but once in a + * positional argument git parses it as an option. `--upload-pack` makes git + * run the given program through a shell on ssh:// and file:// origins. + */ + const getOptionInjection = () => + `--upload-pack=touch\${IFS}${markerFile};true`; + it("does not execute shell metacharacters in the branch name", async () => { // Mirrors the GHSA-4x45-gxvp-6283 PoC: a ref containing $() command // substitution must be passed to git as a literal argument, never @@ -79,6 +104,52 @@ describe("#getMergeBaseCommitSha (command injection)", () => { expect(existsSync(markerFile)).toBe(false); }); + + it("does not let a branch name starting with a dash inject a git option", async () => { + const malicious = getOptionInjection(); + + const sha = await getMergeBaseCommitSha({ base: "main", head: malicious }); + + expect(existsSync(markerFile)).toBe(false); + // The ref exists neither on origin nor locally. + expect(sha).toBe(null); + }); + + it("does not let a reference commit starting with a dash inject a git option", async () => { + const malicious = getOptionInjection(); + + const ancestors = await listAncestorCommits({ sha: malicious, limit: 10 }); + + expect(existsSync(markerFile)).toBe(false); + expect(ancestors).toEqual([]); + }); + + it("does not let a commit starting with a dash inject a git option", async () => { + const malicious = getOptionInjection(); + + const parents = await getCommitParents(malicious); + + expect(existsSync(markerFile)).toBe(false); + expect(parents).toBe(null); + }); + + it("fetches a branch whose name starts with a dash as a ref", async () => { + // `git branch` refuses such a name but `git push` accepts it as a refspec + // destination, so it can reach the CI environment. It is only on origin: + // the local history has no ref to fall back to. + git("checkout", "-q", "-b", "tmp"); + git("commit", "--allow-empty", "-m", "dashed commit"); + const dashedSha = git("rev-parse", "HEAD"); + git("push", "origin", "tmp:refs/heads/-dashed"); + git("checkout", "-q", "main"); + git("branch", "-D", "tmp"); + + const sha = await getMergeBaseCommitSha({ base: "main", head: "-dashed" }); + + expect(sha).toBe(mainSha); + expect(git("rev-parse", "--verify", "argos/-dashed")).toBe(dashedSha); + expect(warn).not.toHaveBeenCalled(); + }); }); describe("#getMergeBaseCommitSha (refs missing on origin)", () => { diff --git a/packages/core/src/ci-environment/git.ts b/packages/core/src/ci-environment/git.ts index 9cbb9948..f9259c6e 100644 --- a/packages/core/src/ci-environment/git.ts +++ b/packages/core/src/ci-environment/git.ts @@ -5,6 +5,16 @@ import { debug, isDebugEnabled } from "../debug"; const execFileAsync = promisify(execFile); +// The branches and commits passed to git come from the CI environment, where +// anyone opening a pull request names a branch. Two rules keep them from being +// interpreted as anything but a ref: +// - git is spawned without a shell (`execFile`), so shell metacharacters are +// never evaluated (GHSA-4x45-gxvp-6283); +// - they are always passed after `--end-of-options` (git 2.24+), as git parses +// any argument starting with a dash as an option: a branch named +// `--upload-pack=` would otherwise make git run `` on +// ssh:// and file:// origins (GHSA-v58q-fvq4-9vh4). + /** * Check if the current directory is a git repository. */ @@ -68,7 +78,12 @@ export function getRepositoryURL() { */ function gitMergeBase(input: { base: string; head: string }) { try { - return execFileSync("git", ["merge-base", input.head, input.base]) + return execFileSync("git", [ + "merge-base", + "--end-of-options", + input.head, + input.base, + ]) .toString() .trim(); } catch (error) { @@ -111,39 +126,45 @@ function checkIsGitLockError(error: unknown): boolean { } /** - * Run `git fetch` with the given arguments. + * Run `git fetch` from origin with the given options and refs (or refspecs). * * Retries on lock contention (`.git/shallow.lock` "File exists") with an * exponential backoff, since this is usually a transient conflict with another * git process and resolves once that process releases the lock. */ -function runGitFetch(args: string[]) { - return pRetry(() => execFileAsync("git", ["fetch", ...args]), { - retries: 3, - minTimeout: 500, - shouldRetry: ({ error }) => checkIsGitLockError(error), - onFailedAttempt: ({ error, retriesLeft, retryDelay }) => { - if (checkIsGitLockError(error) && retriesLeft > 0) { - debug( - `git fetch failed on lock contention, retrying in ${retryDelay}ms (${retriesLeft} left)`, - ); - } +function runGitFetch(input: { options: string[]; refs: string[] }) { + return pRetry( + () => + execFileAsync("git", [ + "fetch", + ...input.options, + "--end-of-options", + "origin", + ...input.refs, + ]), + { + retries: 3, + minTimeout: 500, + shouldRetry: ({ error }) => checkIsGitLockError(error), + onFailedAttempt: ({ error, retriesLeft, retryDelay }) => { + if (checkIsGitLockError(error) && retriesLeft > 0) { + debug( + `git fetch failed on lock contention, retrying in ${retryDelay}ms (${retriesLeft} left)`, + ); + } + }, }, - }); + ); } /** * Run git fetch with a specific ref and depth. */ async function gitFetch(input: { ref: string; depth: number; target: string }) { - await runGitFetch([ - "--force", - "--update-head-ok", - "--depth", - String(input.depth), - "origin", - `${input.ref}:${input.target}`, - ]); + await runGitFetch({ + options: ["--force", "--update-head-ok", "--depth", String(input.depth)], + refs: [`${input.ref}:${input.target}`], + }); } /** @@ -180,6 +201,7 @@ function checkLocalRefExists(ref: string): boolean { "rev-parse", "--verify", "--quiet", + "--end-of-options", `${ref}^{commit}`, ]); return true; @@ -316,7 +338,7 @@ function listShas(path: string, maxCount?: number): string[] { if (maxCount) { args.push(`--max-count=${maxCount}`); } - args.push(path); + args.push("--end-of-options", path); const raw = execFileSync("git", args); const shas = raw.toString().trim().split("\n"); return shas; @@ -338,7 +360,7 @@ export async function listAncestorCommits(input: { // Fetch one extra commit since the commit itself is excluded from the result. const depth = input.limit + 1; try { - await runGitFetch([`--depth=${depth}`, "origin", input.sha]); + await runGitFetch({ options: [`--depth=${depth}`], refs: [input.sha] }); } catch (error) { debug( `Failed to deepen history for ${input.sha}, using local history`, @@ -371,7 +393,7 @@ export async function getCommitParents(sha: string): Promise { // Fetch the commit and its parents, so the parents stop being hidden by the // boundary of a shallow history. try { - await runGitFetch(["--depth=2", "origin", sha]); + await runGitFetch({ options: ["--depth=2"], refs: [sha] }); } catch (error) { debug( `Failed to deepen history for ${sha}, using local history`, @@ -393,7 +415,7 @@ function readCommitParents(sha: string): string[] | null { try { const raw = execFileSync( "git", - ["rev-list", "--parents", "-n", "1", sha, "--"], + ["rev-list", "--parents", "-n", "1", "--end-of-options", sha, "--"], { stdio: ["ignore", "pipe", "pipe"] }, ); const [, ...parents] = raw.toString().trim().split(" ");