gitFetch shellword-splits every refspec, corrupting legal ref names built from BUILDKITE_BRANCH
- 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
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