Security fix: argument injection via commit hashes and branch names
Every gitexec call that took a hash or branch name as the trailing argument to a git subprocess trusted it as a plain revision. git's own porcelain (branch, checkout -b) refuses a ref name starting with "-" via check-ref-format, but that's client-side only — a raw update-ref or push straight to the bare repo creates one anyway, and git's own option parsers then treat a leading "-" as a flag instead of a revision. Confirmed exploitable unauthenticated, on any public repo, with no push access at all: GET /repo-commit/{repo}?hash=--output=<path> made `git show` write to an attacker-chosen file path (git diff/show both support -o/--output=<file>) — a path to planting a malicious git hook and getting code execution on the next push to any repo gitfed-server can write to. Also reachable, for a write collaborator, via a merge request's source/target branch fields once such a branch exists. Fixed by adding --end-of-options before every such argument in internal/gitexec (ShowCommit, CommitDiff, BranchDiff, branchFiles, commitFiles, the merge-request worktree/merge machinery) — verified to still resolve a legitimately dash-prefixed revision correctly while blocking option reinterpretation, unlike "--" (which would misinterpret it as a pathspec instead). Also fixes a data-integrity bug found while auditing this: CloseMergeRequest didn't check the MR's current status, so closing an already-merged one silently overwrote it to "closed without merging" even though the merge commit was still real in the branch.
5 files changed
+188 −10
M
CHANGELOG.md
+5 −0
M
internal/admin/admin.go
+6 −0
M
internal/admin/admin_test.go
+65 −0
M
internal/gitexec/gitexec.go
+10 −10
M
internal/gitexec/gitexec_test.go
+102 −0
CHANGELOG.md
@@ -1,5 +1,10 @@
# Changelog
+## 0.10.1
+
+- **Security fix (critical): argument injection via commit hashes and branch names.** Every place that passed a hash or branch name as the last argument to a `git` subprocess (`ShowCommit`, `CommitDiff`, `BranchDiff`, `CheckMergeable`, `MergeBranches`, the merge-request worktree/merge machinery) trusted it as a plain revision — but git ref names can start with `-` (client-side `git branch`/`checkout -b` block it, a raw `update-ref` or push doesn't), so a crafted name like `--output=/some/path` got parsed as git's own flag instead. Confirmed exploitable **unauthenticated**, on any public repo, via `/repo-commit/{repo}?hash=--output=<path>` — no push access needed at all — to make `git` write to an arbitrary file path gitfed-server can reach, which is a path to planting a malicious git hook and getting code execution on the next push. Fixed by adding `--end-of-options` before every such argument everywhere in `internal/gitexec`, with a regression test that reproduces the exact exploit and asserts no file gets written.
+- Fixed a data-integrity bug found while auditing the above: closing a merge request didn't check its current status, so closing an already-merged MR silently overwrote its record to "closed without merging" — the merge commit stayed real in the branch, but gitfed's own history of it lied. `CloseMergeRequest` is now a no-op on anything that isn't still open.
+
## 0.10.0
- Added merge requests: propose merging one branch into another within a repo, with a live diff (always computed from the branches' current tips, never a stale snapshot), a discussion thread, and a one-click merge that creates a real merge commit. Conflicts are detected up front and block the merge with the specific files listed, instead of silently failing — resolve locally, push, and the merge request picks up the new state automatically. Merging never touches git's wire protocol: it's an authenticated web action gated by the same write-access check as a `git push`, executed server-side in a throwaway worktree so it can never corrupt a real branch ref, with an atomic compare-and-swap update so a concurrent push can't be silently discarded.
internal/admin/admin.go
@@ -467,6 +467,12 @@ func (a *Admin) CloseMergeRequest(repoName string, number int) error {
if err != nil {
return err
}
+ if mr.Status != store.MROpen {
+ // Already merged or already closed — a no-op, not an error, so a
+ // stale page (or a double-click) can't overwrite a "merged" record
+ // with "closed" and erase the fact that it actually landed.
+ return nil
+ }
mr.Status = store.MRClosed
return a.Store.UpdateMergeRequest(mr)
}
internal/admin/admin_test.go
@@ -154,6 +154,71 @@ func TestCheckAccessRespectsRoles(t *testing.T) {
}
}
+// TestCloseMergeRequestDoesNotOverwriteMerged is a regression test for a
+// real bug: CloseMergeRequest used to set Status = MRClosed unconditionally,
+// so closing an already-merged MR (a stale page, a double-click, or two
+// people acting on the same MR at once) silently erased the fact that it
+// had actually landed — the merge commit stayed real in the branch, but
+// gitfed's own record started claiming "closed without merging".
+func TestCloseMergeRequestDoesNotOverwriteMerged(t *testing.T) {
+ s, err := store.Open(filepath.Join(t.TempDir(), "gitfed.db"))
+ if err != nil {
+ t.Fatalf("open store: %v", err)
+ }
+ defer s.Close()
+ a := New(s, nil, "local.test", t.TempDir(), testCA(t))
+
+ mr, err := s.CreateMergeRequest(store.MergeRequest{
+ Repo: "alice/repo", Title: "t", Author: "alice@local.test",
+ SourceBranch: "feature", TargetBranch: "main",
+ })
+ if err != nil {
+ t.Fatalf("CreateMergeRequest: %v", err)
+ }
+
+ // Simulate what MergeMergeRequest does after a clean gitexec.MergeBranches.
+ mr.Status = store.MRMerged
+ mr.MergedBy = "alice@local.test"
+ mr.MergeCommit = "deadbeef"
+ if err := s.UpdateMergeRequest(mr); err != nil {
+ t.Fatalf("UpdateMergeRequest: %v", err)
+ }
+
+ if err := a.CloseMergeRequest(mr.Repo, mr.Number); err != nil {
+ t.Fatalf("CloseMergeRequest: %v", err)
+ }
+
+ got, err := s.GetMergeRequest(mr.Repo, mr.Number)
+ if err != nil {
+ t.Fatalf("GetMergeRequest: %v", err)
+ }
+ if got.Status != store.MRMerged {
+ t.Fatalf("status = %q after closing an already-merged MR, want %q — the merge record was overwritten", got.Status, store.MRMerged)
+ }
+ if got.MergeCommit != "deadbeef" {
+ t.Fatalf("MergeCommit = %q, want it preserved as %q", got.MergeCommit, "deadbeef")
+ }
+
+ // Closing a genuinely open MR should still work.
+ mr2, err := s.CreateMergeRequest(store.MergeRequest{
+ Repo: "alice/repo", Title: "t2", Author: "alice@local.test",
+ SourceBranch: "feature2", TargetBranch: "main",
+ })
+ if err != nil {
+ t.Fatalf("CreateMergeRequest: %v", err)
+ }
+ if err := a.CloseMergeRequest(mr2.Repo, mr2.Number); err != nil {
+ t.Fatalf("CloseMergeRequest (open MR): %v", err)
+ }
+ got2, err := s.GetMergeRequest(mr2.Repo, mr2.Number)
+ if err != nil {
+ t.Fatalf("GetMergeRequest: %v", err)
+ }
+ if got2.Status != store.MRClosed {
+ t.Fatalf("status = %q, want %q", got2.Status, store.MRClosed)
+ }
+}
+
func validTestKey(t *testing.T) string {
t.Helper()
pub, _, err := ed25519.GenerateKey(rand.Reader)
internal/gitexec/gitexec.go
@@ -346,7 +346,7 @@ const commitDiffMaxLines = 4000
// repoPath.
func ShowCommit(repoPath, hash string) (detail CommitDetail, found bool, err error) {
format := strings.Join([]string{"%H", "%h", "%an", "%ae", "%aI", "%P"}, commitFieldSep) + commitFieldSep + "%B"
- cmd := exec.Command("git", "--git-dir="+repoPath, "show", "--no-patch", "--format="+format, hash)
+ cmd := exec.Command("git", "--git-dir="+repoPath, "show", "--no-patch", "--format="+format, "--end-of-options", hash)
var stdout, stderr bytes.Buffer
cmd.Stdout = &stdout
cmd.Stderr = &stderr
@@ -412,11 +412,11 @@ func statusName(code byte) string {
// invocations walk the same diff in the same deterministic order, so the
// Nth line of each always describes the same file.
func commitFiles(repoPath, hash string) ([]DiffFile, error) {
- statusLines, err := runGitLines(repoPath, "show", "--format=", "--name-status", hash)
+ statusLines, err := runGitLines(repoPath, "show", "--format=", "--name-status", "--end-of-options", hash)
if err != nil {
return nil, err
}
- numLines, err := runGitLines(repoPath, "show", "--format=", "--numstat", hash)
+ numLines, err := runGitLines(repoPath, "show", "--format=", "--numstat", "--end-of-options", hash)
if err != nil {
return nil, err
}
@@ -429,11 +429,11 @@ func commitFiles(repoPath, hash string) ([]DiffFile, error) {
// rather than a single commit's parent.
func branchFiles(repoPath, target, source string) ([]DiffFile, error) {
rangeSpec := target + "..." + source
- statusLines, err := runGitLines(repoPath, "diff", "--name-status", rangeSpec)
+ statusLines, err := runGitLines(repoPath, "diff", "--name-status", "--end-of-options", rangeSpec)
if err != nil {
return nil, err
}
- numLines, err := runGitLines(repoPath, "diff", "--numstat", rangeSpec)
+ numLines, err := runGitLines(repoPath, "diff", "--numstat", "--end-of-options", rangeSpec)
if err != nil {
return nil, err
}
@@ -491,7 +491,7 @@ func runGitLines(repoPath string, args ...string) ([]string, error) {
// capped at commitDiffMaxLines lines. truncated reports whether the cap
// was hit.
func CommitDiff(repoPath, hash string) (diff string, truncated bool, err error) {
- cmd := exec.Command("git", "--git-dir="+repoPath, "show", "--format=", "--no-color", hash)
+ cmd := exec.Command("git", "--git-dir="+repoPath, "show", "--format=", "--no-color", "--end-of-options", hash)
var stdout, stderr bytes.Buffer
cmd.Stdout = &stdout
cmd.Stderr = &stderr
@@ -535,7 +535,7 @@ func BranchDiff(repoPath, target, source string) (files []DiffFile, diff string,
return nil, "", false, false, err
}
- cmd := exec.Command("git", "--git-dir="+repoPath, "diff", "--no-color", target+"..."+source)
+ cmd := exec.Command("git", "--git-dir="+repoPath, "diff", "--no-color", "--end-of-options", target+"..."+source)
var stdout, stderr bytes.Buffer
cmd.Stdout = &stdout
cmd.Stderr = &stderr
@@ -570,7 +570,7 @@ func withScratchWorktree(repoPath, base string, fn func(dir string) error) error
_ = os.RemoveAll(dir)
}()
- addCmd := exec.Command("git", "--git-dir="+repoPath, "worktree", "add", "--detach", "--quiet", dir, base)
+ addCmd := exec.Command("git", "--git-dir="+repoPath, "worktree", "add", "--detach", "--quiet", "--end-of-options", dir, base)
var stderr bytes.Buffer
addCmd.Stderr = &stderr
if err := addCmd.Run(); err != nil {
@@ -600,7 +600,7 @@ func CheckMergeable(repoPath, target, source string) (MergeResult, error) {
var result MergeResult
err := withScratchWorktree(repoPath, target, func(dir string) error {
cmd := exec.Command("git", "-C", dir, "-c", "user.name=gitfed", "-c", "user.email=gitfed@localhost",
- "merge", "--no-commit", "--no-ff", "--quiet", source)
+ "merge", "--no-commit", "--no-ff", "--quiet", "--end-of-options", source)
var stderr bytes.Buffer
cmd.Stderr = &stderr
if runErr := cmd.Run(); runErr != nil {
@@ -633,7 +633,7 @@ func MergeBranches(repoPath, target, source, message, authorName, authorEmail st
err = withScratchWorktree(repoPath, target, func(dir string) error {
cmd := exec.Command("git", "-C", dir,
"-c", "user.name="+authorName, "-c", "user.email="+authorEmail,
- "merge", "--no-ff", "--quiet", "-m", message, source)
+ "merge", "--no-ff", "--quiet", "-m", message, "--end-of-options", source)
var stderr bytes.Buffer
cmd.Stderr = &stderr
if runErr := cmd.Run(); runErr != nil {
internal/gitexec/gitexec_test.go
@@ -428,3 +428,105 @@ func TestMergeBranchesConflict(t *testing.T) {
t.Fatalf("branches after a conflicted merge attempt = %v, err %v — no stray branch/worktree should be left behind", branches, err)
}
}
+
+// TestArgumentInjectionIsBlocked is a regression test for a real,
+// confirmed vulnerability: git's own porcelain commands (checkout -b,
+// branch) refuse to create a ref whose name starts with "-" via
+// check-ref-format, but that's only enforced client-side — a raw
+// `update-ref` (or an equivalent low-level push) against the bare repo
+// happily creates one anyway. Every gitexec function that took a
+// hash/branch name as a trailing command-line argument was passing it
+// straight to `git`, so a ref literally named e.g. "--output=/some/path"
+// got interpreted as git's own -o/--output flag instead of a revision —
+// confirmed to make `git diff`/`git show` write attacker-chosen content
+// to an attacker-chosen path, reachable by an unauthenticated request to
+// the commit-detail page's ?hash= parameter (no push access required at
+// all) or by any write collaborator via a merge request's branch fields.
+// The fix is `--end-of-options` before every such argument; this test
+// proves it holds for every affected entry point.
+func TestArgumentInjectionIsBlocked(t *testing.T) {
+ bare := setupMergeTestRepo(t)
+
+ mainTip, err := revParse(bare, "refs/heads/main")
+ if err != nil {
+ t.Fatal(err)
+ }
+
+ // A ref name that is also a real git flag which writes to a file —
+ // bypasses git's client-side ref-name validation the same way a raw
+ // push directly to the bare repo would.
+ outFile := filepath.Join(t.TempDir(), "should-not-exist.txt")
+ evilRef := "--output=" + outFile
+ run(t, bare, "git", "update-ref", "refs/heads/"+evilRef, mainTip)
+
+ assertNoFile := func(step string) {
+ t.Helper()
+ if _, err := os.Stat(outFile); err == nil {
+ t.Fatalf("%s: attacker-controlled file was created at %s — argument injection succeeded", step, outFile)
+ }
+ }
+
+ // ShowCommit/CommitDiff: the hash comes straight from an HTTP query
+ // parameter with no validation that it even looks like a commit hash.
+ // evilRef happens to also be a syntactically valid ref name (we just
+ // created it), so git resolving it as an ordinary revision is correct,
+ // safe behavior — the only thing that must never happen is the file
+ // write.
+ if _, _, err := CommitDiff(bare, evilRef); err != nil {
+ t.Logf("CommitDiff(evilRef): %v (acceptable)", err)
+ }
+ assertNoFile("CommitDiff")
+
+ if _, _, err := ShowCommit(bare, evilRef); err != nil {
+ t.Logf("ShowCommit(evilRef): %v (acceptable)", err)
+ }
+ assertNoFile("ShowCommit")
+
+ // BranchDiff/CheckMergeable/MergeBranches: the malicious ref is a real
+ // branch (per ListBranches), so these take the "found" path rather
+ // than erroring outright — what matters is still that nothing gets
+ // written to outFile.
+ branches, err := ListBranches(bare)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if !containsBranchName(branches, evilRef) {
+ t.Fatalf("expected %q to be a listed branch (proves check-ref-format was bypassed), got %v", evilRef, branches)
+ }
+
+ if _, _, _, _, err := BranchDiff(bare, "main", evilRef); err != nil {
+ // An error here is fine (git may reject the malformed revision
+ // outright) — a file write is not.
+ t.Logf("BranchDiff with evilRef as source: %v (acceptable)", err)
+ }
+ assertNoFile("BranchDiff (evil source)")
+
+ if _, _, _, _, err := BranchDiff(bare, evilRef, "feature-clean"); err != nil {
+ t.Logf("BranchDiff with evilRef as target: %v (acceptable)", err)
+ }
+ assertNoFile("BranchDiff (evil target)")
+
+ if _, err := CheckMergeable(bare, "main", evilRef); err != nil {
+ t.Logf("CheckMergeable with evilRef as source: %v (acceptable)", err)
+ }
+ assertNoFile("CheckMergeable (evil source)")
+
+ if _, err := CheckMergeable(bare, evilRef, "feature-clean"); err != nil {
+ t.Logf("CheckMergeable with evilRef as target: %v (acceptable)", err)
+ }
+ assertNoFile("CheckMergeable (evil target)")
+
+ if _, _, err := MergeBranches(bare, "main", evilRef, "msg", "Test", "test@example.com"); err != nil {
+ t.Logf("MergeBranches with evilRef as source: %v (acceptable)", err)
+ }
+ assertNoFile("MergeBranches (evil source)")
+}
+
+func containsBranchName(branches []string, name string) bool {
+ for _, b := range branches {
+ if b == name {
+ return true
+ }
+ }
+ return false
+}