buildkite / buildkite/agent

gitFetch shellword-splits every refspec, corrupting legal ref names built from BUILDKITE_BRANCH

Open
#4,212 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
1.1k
Forks
378
Avg merge
2d 6h
Merged PRs (30d)
74

Description

## Summary

`gitFetch` runs `shellwords.Split` on **every** refspec it is given (internal/job/git.go). That is the right contract for `BUILDKITE_REFSPEC`, which is documented to allow multiple space-separated refspecs — but it is applied indiscriminately to single refspecs built from externally controlled values, corrupting ref names that are legal in git.

Quotes and backslashes are valid in git refnames (`git check-ref-format` allows them). Passed through `shellwords.Split`:

- a branch named `he'll'o` becomes `hello` — the fetch silently targets a **different ref**
- a branch with an unbalanced quote (`it's`) makes `shellwords.Split` error and fails the job
- a backslash-containing refname is unescaped into something else

## Affected call sites (on main)

- **`updateGitMirror` warm-path branch fetch** — `RefSpecs: []string{e.Branch}`. Fixed in #4210, which adds a `RawRefSpecs` field to `gitFetchArgs` (appended verbatim, no splitting) and fetches by explicit refspec.
- **`fetchSource` `refspecBranch` case** (checkout_fetch.go, the `commit == "HEAD"` path) — `RefSpecs: []string{e.Branch}`. Still split-mangled. This site also inherits the bare-name resolution problem from #4211: with a same-named tag, the fetch resolves the tag, so FETCH_HEAD — which this path checks out — points at the **tag tip instead of the branch head**.
- `refspecCustom` (`BUILDKITE_REFSPEC`) — splitting is intentional here; not a bug, but the only site that actually wants it.
- PR refspecs, commit-sha fetches, `gitFetchWithFallback` — constructed from PR numbers / SHAs / git config; unaffected in practice but still pass through the splitter.

`checkCommitOnBranch` (commit_verification.go) already builds its fetch argv directly, with a comment documenting exactly this hazard — evidence the footgun is known and being dodged ad hoc.

## Suggested fix

Invert the default: pass refspecs verbatim everywhere, and reserve word-splitting for the one place whose contract requires it (`BUILDKITE_REFSPEC`). #4210's `RawRefSpecs` field is the mechanism; the remaining work is migrating the `refspecBranch` fetch (ideally to an explicit `+refs/heads/:` form, which also fixes its #4211-style tag shadowing) and then confining `shellwords.Split` to the custom-refspec path.

Related: #4211 (bare-name fetch resolving `refs/tags/` before `refs/heads/`), #4210 (fixes the mirror call site).

Contributor guide

Open the contributing guide

Research direction

Start in internal/job/git.go, then trace the refspecBranch case in checkout_fetch.go and compare it with the RawRefSpecs handling from #4210. Check the existing direct-fetch logic in commit_verification.go for the documented refname hazard. Done means branch refspecs preserve legal quotes and backslashes, avoid tag shadowing, and shellword splitting remains confined to BUILDKITE_REFSPEC.

Written by the indexing model from the issue text.

Assessment

Tech stack
git, go
Domain
ci-cd, tooling
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
66/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.