Commit `8994b7770e`

8994b7770ea48df9f349ddcd0f2c3c72979d7280

parent: 32fb00e679

Verified · cmc ci/build: success ci/test: success

cmc <hello@cleberg.net> · 2026-09-28 22:38 UTC

backup: verify skips commondir; refs/ before packed-refs; tighter stale match

Ref #259

Layout: unified · split

.gitbay/wiki/Admin.org +4 −3

 Verifying an archive of unknown origin: run it as an unprivileged
 user. The connectivity check runs git with =--git-dir= on each
 extracted repository, so a directory that is not a repository fails
-rather than git checking an enclosing one, and =objects/info/alternates=
+rather than git checking an enclosing one. =objects/info/alternates=
-members are not extracted, so an archive cannot point git at object
+and a =commondir= directly in a =*.git= directory are not extracted,
-stores elsewhere on the host. Git still reads each archived
+so an archive cannot use either to have git read another repository's
+objects or refs on the host. Git still reads each archived
 repository's own =config=.
 Archives carry a directory entry for every directory, including an

.gitbay/wiki/Architecture/08-Operations.org +4 −2

 | Offsite (restic)| nightly  | per prune policy | =/var/lib/gitbay= and a staged database copy, to object storage |
 - The database snapshot is taken before repositories are read, and
-  each repository's HEAD, packed-refs and refs/ are archived before its
+  each repository's HEAD, refs/ and packed-refs are archived before its
-  objects, so every archived ref finds the objects it reaches. A push
+  objects, so every archived ref finds the objects it reaches, unless
+  git's own automatic gc after a push repacks during the walk; the
+  archive can then miss objects, and =--verify= reports it. A push
   during the backup is missing or present as unreferenced objects
   (=cmd/gitbayd/backup.go=).
 - Excluded: WAL files, the hook socket, askpass scripts, generated

.gitbay/wiki/Threat-Model.org +3 −1

 - Backups snapshot the database first, and repository deletes and
   moves wait out a full backup. Each repository's refs are archived
   before its objects, so every archived ref finds the objects it
-  reaches. A push during a backup may be missing from the archive, or
+  reaches, unless git's own automatic gc after a push repacks during
+  the walk: the archive can then miss objects, and =--verify= reports
+  it. A push during a backup may be missing from the archive, or
   present as objects no archived ref names, and a repository's refs may
   be newer than the database snapshot (see [[Admin]]).
 - The audit log lives in the database the daemon writes, so anyone with

CHANGELOG.org +8 −4

   backup is running" while it runs. =--verify= now also runs =git
   fsck --connectivity-only= on each archived repository and names any
   that fail. (#259)
-- A full backup archives each repository's HEAD, =packed-refs= and
+- A full backup archives each repository's HEAD, =refs/= and
-  =refs/= before its objects, so a push during the backup cannot leave
+  =packed-refs= before its objects, so a push during the backup cannot
-  an archived ref naming objects the archive lacks. (#259)
+  leave an archived ref naming objects the archive lacks. The exception
+  is git's own automatic gc after a push repacking during the walk: the
+  archive can then miss objects, and =--verify= reports it. (#259)
 - =gitbayd admin gc= and =admin mr prune= refuse with "a backup is
   running" during a full backup. A pack or loose object that git's
   automatic gc removes while the backup walks is skipped instead of
   left beside its archive once they are a day old. (#259)
 - =gitbayd admin backup --verify= runs fsck with =--git-dir=, so a
   directory that is not a repository fails instead of git checking an
-  enclosing one, and does not extract =objects/info/alternates=. (#259)
+  enclosing one, and does not extract =objects/info/alternates= or a
+  repository's =commondir=, so neither can point fsck at another
+  repository on the host. (#259)
 - =gitbayd admin secrets init= and =rotate= hold an flock on =<key
   file>.lock=, so two runs at once serialize. (#273)

cmd/gitbayd/backup.go +46 −21

 	"io"
 	"io/fs"
 	"os"
+	"path"
 	"path/filepath"
+	"regexp"
 	"strings"
 	"time"
 // younger than this.
 const staleAge = 24 * time.Hour
+// tmpArchive is a temporary archive's name: os.CreateTemp's pattern
+// "."+base+".tmp-" followed by the digits it appends.
+var tmpArchive = regexp.MustCompile(`^\..+\.tmp-[0-9]+$`)
 // removeStale removes what a killed run left in dir: snapshot
 // directories and temporary archives last modified before cutoff.
 func removeStale(dir string, cutoff time.Time) {
 	for _, e := range ents {
 		name := e.Name()
 		snap := e.IsDir() && strings.HasPrefix(name, ".gitbay-snap-")
-		tmp := e.Type().IsRegular() && strings.HasPrefix(name, ".") && strings.Contains(name, ".tmp-")
+		tmp := e.Type().IsRegular() && tmpArchive.MatchString(name)
 		if !snap && !tmp {
 			continue
 		}
 // use it to write into the repository at that point.
 var afterRefs = func(repo string) {}
-// addRefs archives HEAD, packed-refs and refs/ of the repository at
+// addRefs archives HEAD, refs/ and packed-refs of the repository at
-// path, whichever exist.
+// path, whichever exist. refs/ is read before packed-refs, the order git
+// reads them in: pack-refs writes packed-refs before deleting the loose
+// refs it packed, so a ref moving between the two is caught in one.
 func addRefs(tw *tar.Writer, path, name string) error {
-	for _, f := range []string{"HEAD", "packed-refs"} {
+	if err := addRegular(tw, path, name, "HEAD"); err != nil {
-		fi, err := os.Lstat(filepath.Join(path, f))
+		return err
-		if errors.Is(err, fs.ErrNotExist) || err == nil && !fi.Mode().IsRegular() {
+	}
-			continue
+	refs := filepath.Join(path, "refs")
-		}
+	if _, err := os.Lstat(refs); err == nil {
-		if err != nil {
+		if err := addTree(tw, path, name, refs); err != nil {
-			return err
-		}
-		if err := addFile(tw, filepath.Join(path, f), name+"/"+f); err != nil {
 			return err
 		}
+	} else if !errors.Is(err, fs.ErrNotExist) {
+		return err
 	}
-	refs := filepath.Join(path, "refs")
+	return addRegular(tw, path, name, "packed-refs")
-	if _, err := os.Lstat(refs); errors.Is(err, fs.ErrNotExist) {
+}
+// addRegular archives the regular file f in the repository at path, if
+// it exists.
+func addRegular(tw *tar.Writer, path, name, f string) error {
+	fi, err := os.Lstat(filepath.Join(path, f))
+	if errors.Is(err, fs.ErrNotExist) || err == nil && !fi.Mode().IsRegular() {
 		return nil
 	}
+	if err != nil {
+		return err
+	}
+	return addFile(tw, filepath.Join(path, f), name+"/"+f)
+}
+// addTree archives the directory refs inside the repository at path.
+func addTree(tw *tar.Writer, path, name, refs string) error {
 	return filepath.WalkDir(refs, func(p string, d fs.DirEntry, err error) error {
 		if err != nil {
 			return err
 			if !filepath.IsLocal(trimmed) {
 				return fmt.Errorf("%s: member %q leaves the archive root", path, h.Name)
 			}
-			if alternates(trimmed) {
+			if borrowsObjects(trimmed) {
 				continue
 			}
 			dest := filepath.Join(tmp, filepath.FromSlash(trimmed))
 	return nil
 }
-// alternates reports an archive member that would point git at object
+// borrowsObjects reports an archive member that would point git at
-// stores outside the extracted repository. gitbay writes none, and one in
+// objects or refs outside the extracted repository: alternates, or a
-// a hostile archive would have fsck read other paths, so verify leaves
+// commondir directly in a *.git directory. gitbay writes none, and one in
-// them out. The comparison ignores case, as a case-insensitive
+// a hostile archive would have fsck read another repository on the host,
-// filesystem would.
+// so verify leaves them out. The comparison ignores case, as a
-func alternates(name string) bool {
+// case-insensitive filesystem would.
+func borrowsObjects(name string) bool {
 	name = strings.ToLower(filepath.ToSlash(filepath.Clean(name)))
+	if dir, base := path.Split(name); base == "commondir" && strings.HasSuffix(strings.TrimSuffix(dir, "/"), ".git") {
+		return true
+	}
 	return strings.HasSuffix(name, "/objects/info/alternates") ||
 		strings.HasSuffix(name, "/objects/info/http-alternates")
 }

cmd/gitbayd/backup_test.go +74 −6

	@@ -18,6 +18,7 @@ import (
18		18
19	"gitbay.org/gitbay/internal/backuplock"	19	"gitbay.org/gitbay/internal/backuplock"
20	"gitbay.org/gitbay/internal/config"	20	"gitbay.org/gitbay/internal/config"
		21	"gitbay.org/gitbay/internal/gitutil"
21	)	22	)
22		23
23	// members lists the archive's entries by name.	24	// members lists the archive's entries by name.
	@@ -679,15 +680,16 @@ func TestBackupRemovesStaleTemporaries(t *testing.T) {
679	}	680	}
680	}	681	}
681	mk(".gitbay-snap-old", true, old)	682	mk(".gitbay-snap-old", true, old)
682	mk(".b.tar.gz.tmp-old", false, old)	683	mk(".b.tar.gz.tmp-123", false, old)
683	mk(".gitbay-snap-new", true, time.Now())	684	mk(".gitbay-snap-new", true, time.Now())
684	mk(".b.tar.gz.tmp-new", false, time.Now())	685	mk(".b.tar.gz.tmp-456", false, time.Now())
685	mk(".keep", false, old)	686	mk(".keep", false, old)
		687	mk(".notes.tmp-draft", false, old)
686	if err := runBackup(cfg, filepath.Join(dir, "b.tar.gz"), true); err != nil {	688	if err := runBackup(cfg, filepath.Join(dir, "b.tar.gz"), true); err != nil {
687	t.Fatal(err)	689	t.Fatal(err)
688	}	690	}
689	got := leftovers(t, dir)	691	got := leftovers(t, dir)
690	want := []string{".b.tar.gz.tmp-new", ".gitbay-snap-new", ".keep"}	692	want := []string{".b.tar.gz.tmp-456", ".gitbay-snap-new", ".keep", ".notes.tmp-draft"}
691	sort.Strings(got)	693	sort.Strings(got)
692	if strings.Join(got, " ") != strings.Join(want, " ") {	694	if strings.Join(got, " ") != strings.Join(want, " ") {
693	t.Errorf("left %v, want %v", got, want)	695	t.Errorf("left %v, want %v", got, want)
	@@ -758,7 +760,7 @@ func TestVerifyIgnoresAlternates(t *testing.T) {
758	}	760	}
759	}	761	}
760		762
761	func TestAlternatesMember(t *testing.T) {	763	func TestBorrowsObjectsMember(t *testing.T) {
762	for name, want := range map[string]bool{	764	for name, want := range map[string]bool{
763	"repos/a/b.git/objects/info/alternates": true,	765	"repos/a/b.git/objects/info/alternates": true,
764	"repos/a/b.git/objects/info/./alternates": true,	766	"repos/a/b.git/objects/info/./alternates": true,
	@@ -766,9 +768,75 @@ func TestAlternatesMember(t *testing.T) {
766	"repos/a/b.git/objects/info/http-alternates": true,	768	"repos/a/b.git/objects/info/http-alternates": true,
767	"repos/a/b.git/objects/info/packs": false,	769	"repos/a/b.git/objects/info/packs": false,
768	"repos/a/b.git/refs/heads/alternates": false,	770	"repos/a/b.git/refs/heads/alternates": false,
		771	"repos/a/b.git/commondir": true,
		772	"repos/a/b.git/CommonDir": true,
		773	"repos/a/b.git/refs/heads/commondir": false,
769	} {	774	} {
770	if got := alternates(name); got != want {	775	if got := borrowsObjects(name); got != want {
771	t.Errorf("alternates(%q) = %v, want %v", name, got, want)	776	t.Errorf("borrowsObjects(%q) = %v, want %v", name, got, want)
772	}	777	}
773	}	778	}
774	}	779	}
		780
		781	// verify does not extract a commondir, so an archived repository with
		782	// none of its own objects cannot pass by pointing git at a repository on
		783	// the host.
		784	func TestVerifyIgnoresCommondir(t *testing.T) {
		785	cfg := testConfig(t)
		786	st, err := openStore(cfg)
		787	if err != nil {
		788	t.Fatal(err)
		789	}
		790	uid, err := st.CreateUser("krz", false)
		791	if err != nil {
		792	t.Fatal(err)
		793	}
		794	if _, err := st.CreateRepo("user", uid, "thing", "public"); err != nil {
		795	t.Fatal(err)
		796	}
		797	st.Close()
		798
		799	work := t.TempDir()
		800	gitIn(t, work, "init", "-q", "-b", "main")
		801	if err := os.WriteFile(filepath.Join(work, "a.txt"), []byte("a\n"), 0o644); err != nil {
		802	t.Fatal(err)
		803	}
		804	gitIn(t, work, "add", "a.txt")
		805	gitIn(t, work, "commit", "-q", "-m", "one")
		806	host := filepath.Join(t.TempDir(), "host.git")
		807	gitIn(t, work, "clone", "-q", "--bare", work, host)
		808	gitIn(t, host, "pack-refs", "--all")
		809
		810	// A repository whose refs are its own and whose objects directory is
		811	// empty, borrowing everything else from host through commondir.
		812	dir := filepath.Join(cfg.Server.Root, "repos", "krz", "thing.git")
		813	for _, d := range []string{"objects", "refs"} {
		814	if err := os.MkdirAll(filepath.Join(dir, d), 0o755); err != nil {
		815	t.Fatal(err)
		816	}
		817	}
		818	packed, err := os.ReadFile(filepath.Join(host, "packed-refs"))
		819	if err != nil {
		820	t.Fatal(err)
		821	}
		822	for name, body := range map[string]string{
		823	"HEAD": "ref: refs/heads/main\n",
		824	"packed-refs": string(packed),
		825	"commondir": host + "\n",
		826	} {
		827	if err := os.WriteFile(filepath.Join(dir, name), []byte(body), 0o644); err != nil {
		828	t.Fatal(err)
		829	}
		830	}
		831	if err := gitutil.FsckConnectivity(dir); err != nil {
		832	t.Fatalf("with commondir on the host the repository should pass: %v", err)
		833	}
		834
		835	archive := filepath.Join(t.TempDir(), "b.tar.gz")
		836	if err := runBackup(cfg, archive, false); err != nil {
		837	t.Fatal(err)
		838	}
		839	if err := verifyBackup(archive, ""); err == nil \|\| !strings.Contains(err.Error(), "connectivity") {
		840	t.Fatalf("verify of a repository whose objects are only in its commondir: %v", err)
		841	}
		842	}

Commit 8994b7770e

Commit `8994b7770e`