fix(gitread): answer 500, not 404, for a ref whose commit git cannot read

docs/reference/http-api.md

@@ -34,7 +34,8 @@ are in [`../upstream/openapi.yaml`](../upstream/openapi.yaml).
   token, or a fork's `base_repo.auth.token`, lacks a required scope),
   `repo_name_mismatch` (400: the create body's `repo_name` differs from the token's
   `repo` claim), `branch_not_found` (404), `ref_not_found` (404: a ref, tag, or
-  revision does not resolve), `credential_not_found` (404), `credential_exists` (409),
+  revision does not resolve; an existing ref or object whose commit git cannot
+  read is a 500), `credential_not_found` (404), `credential_exists` (409),
   `repository_deleted` (409: `DELETE /api/v1/repos/delete` on a deleted repository;
   `DELETE /api/repos/{repo_name}` answers that case with 404 `repository_not_found`,
   because its name lookup skips deleted repositories), `ref_guard_failed` (409: a

internal/api/broken_ref_test.go

@@ -0,0 +1,100 @@
+package api
+
+import (
+	"context"
+	"net/http"
+	"os/exec"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"github.com/coder/openstorage/internal/auth"
+	"github.com/coder/openstorage/internal/blob"
+	"github.com/coder/openstorage/internal/ref"
+)
+
+// A tag whose commit is in no pack is an existing ref that git cannot
+// read: each endpoint that resolves a ref answers 500 for it, and for the
+// tag's object id, not 404. A ref that does not exist is still 404.
+func TestReadEndpointsAnswer500ForARefGitCannotRead(t *testing.T) {
+	runBackends(t, func(t *testing.T, e *testEnv) {
+		ctx := context.Background()
+		org := e.newOrg(t)
+		repoURL := "https://example.com/" + org + "/broken-" + randomHex(t, 4)
+		repo, err := e.store.CreateRepo(ctx, org, repoURL, "main")
+		if err != nil {
+			t.Fatal(err)
+		}
+		dir := t.TempDir()
+		coreGit(t, dir, "init", "-q", "-b", "main", ".")
+		coreWriteFile(t, dir, "a.txt", "one\n")
+		coreGit(t, dir, "add", ".")
+		coreGit(t, dir, "commit", "-q", "-m", "c1")
+		c1 := coreRevParse(t, dir, "HEAD")
+		c2 := strings.TrimSpace(coreGit(t, dir, "commit-tree", coreRevParse(t, dir, "HEAD^{tree}"), "-p", c1, "-m", "c2"))
+		coreGit(t, dir, "tag", "-a", "-m", "gone", "gone-tag", c2)
+		tagObj := coreRevParse(t, dir, "refs/tags/gone-tag")
+		coreGit(t, dir, "tag", "-d", "gone-tag")
+		coreSeedStores(t, e, repo.RepoID, dir, map[string]string{"refs/heads/main": c1})
+
+		// A pack with the tag object alone: its commit is in no pack.
+		packDir := t.TempDir()
+		pk := exec.Command("git", "pack-objects", filepath.Join(packDir, "pack"))
+		pk.Dir = dir
+		pk.Stdin = strings.NewReader(tagObj + "\n")
+		out, err := pk.Output()
+		if err != nil {
+			t.Fatal(err)
+		}
+		id := strings.TrimSpace(string(out))
+		key := blob.PackKey(repo.RepoID, id)
+		if err := e.blobStore.UploadFile(ctx, key, filepath.Join(packDir, "pack-"+id+".pack")); err != nil {
+			t.Fatal(err)
+		}
+		if err := e.refStore.RegisterPacks(ctx, repo.RepoID, []ref.Pack{{ID: id, Key: key}}); err != nil {
+			t.Fatal(err)
+		}
+		if won, err := e.refStore.CASRef(ctx, repo.RepoID, "refs/tags/gone-tag", ref.ZeroSHA, tagObj); err != nil || !won {
+			t.Fatalf("CASRef(refs/tags/gone-tag) = %v, %v", won, err)
+		}
+
+		token := e.mint(t, tokenSpec{iss: org, repo: repoURL, scopes: []string{auth.ScopeGitRead}})
+		for _, tc := range []struct {
+			path string
+			want int
+		}{
+			{"/api/v1/repos/commits?ref=gone-tag", http.StatusInternalServerError},
+			{"/api/v1/repos/commits?ref=" + tagObj, http.StatusInternalServerError},
+			{"/api/v1/repos/commits?ref=" + tagObj[:12], http.StatusInternalServerError},
+			{"/api/v1/repos/commit?ref=gone-tag", http.StatusInternalServerError},
+			{"/api/v1/repos/files?ref=gone-tag", http.StatusInternalServerError},
+			{"/api/v1/repos/file?ref=gone-tag&path=a.txt", http.StatusInternalServerError},
+			{"/api/v1/repos/blame?ref=gone-tag&path=a.txt", http.StatusInternalServerError},
+			{"/api/v1/repos/diff?ref=gone-tag", http.StatusInternalServerError},
+			{"/api/v1/repos/commits?ref=main", http.StatusOK},
+			{"/api/v1/repos/commits?ref=nope", http.StatusNotFound},
+		} {
+			resp, body := e.doReq(t, http.MethodGet, tc.path, token, nil, "")
+			if resp.StatusCode != tc.want {
+				t.Errorf("GET %s = %d, want %d: %.200s", tc.path, resp.StatusCode, tc.want, body)
+			}
+		}
+		// create-branch falls back to preferred_base only for a base that
+		// names no ref: a base git cannot read is a 500.
+		writeTok := e.mint(t, tokenSpec{iss: org, repo: repoURL, scopes: []string{auth.ScopeGitWrite}})
+		srv := newWrefServer(t, e)
+		for _, tc := range []struct {
+			base string
+			want int
+		}{
+			{"gone-tag", http.StatusInternalServerError},
+			{"nope", http.StatusOK},
+		} {
+			resp, body := wrefDo(t, srv, http.MethodPost, "/api/v1/repos/branches/create", writeTok,
+				map[string]any{"target_branch": "from-" + tc.base, "base_ref": tc.base, "preferred_base": "main"})
+			if resp.StatusCode != tc.want {
+				t.Errorf("create-branch from %s with preferred_base main = %d, want %d: %v", tc.base, resp.StatusCode, tc.want, body)
+			}
+		}
+	})
+}

internal/api/handlers_fork.go

@@ -236,7 +236,8 @@ func (s *server) handleForkRepo(w http.ResponseWriter, r *http.Request, verifier
 	}
 
 	// Fork point (docs/reference/http-api.md): sha overrides ref; default = the source's
-	// default branch. Everything unresolvable is invalid input (400).
+	// default branch. A fork point that names no ref is invalid input
+	// (400), and one whose commit git cannot read is a 500.
 	point := spec.SHA
 	if point == "" {
 		point = spec.Ref

internal/api/handlers_write_commit.go

@@ -1089,8 +1089,10 @@ func (sc *wcScratch) resolveTree(ctx context.Context, commitSHA string) (string,
 // is false when the target names no commit: no object, a tree, a blob,
 // or a tag of one. A commit whose tree, or a tag whose commit, git cannot
 // read is the repository's failure, an error, and so is any other git
-// failure (a fatal exit, a killed git).
+// failure (a fatal exit, a killed git). target is a full object id, so
+// that cat-file -e exit 1 below means only that no such object exists.
 func (sc *wcScratch) restoreTargetTree(ctx context.Context, target string) (tree string, found bool, err error) {
+	assertf(ref.ValidSHA(target), "restoreTargetTree: malformed sha %q", target)
 	_, kindErr := sc.git(ctx, nil, nil, "rev-parse", "--verify", "--quiet", "--end-of-options", target+"^{commit}")
 	if kindErr == nil {
 		out, err := sc.git(ctx, nil, nil, "rev-parse", "--verify", "--quiet", "--end-of-options", target+"^{commit}^{tree}")

internal/api/handlers_write_refs.go

@@ -262,7 +262,8 @@ type wrefCreateBranchRequest struct {
 	// stay subject to the no-force-push ref-policy op (real ancestry check).
 	Force bool `json:"force"`
 	// PreferredBase is the fallback base: used when no base_ref/base_branch
-	// is given, or when the primary base does not resolve.
+	// is given, or when the primary base names no ref. A primary base
+	// whose commit git cannot read is a 500, not a reason to fall back.
 	PreferredBase string `json:"preferred_base"`
 }
 
@@ -290,7 +291,8 @@ type wrefCreateBranchResponse struct {
 
 // handleCreateBranch resolves the base to a commit (branch, tag — peeled —
 // or hex sha via gitread.ResolveRef; preferred_base is the fallback when
-// the primary base is absent or unresolvable), policy-checks the new ref,
+// the primary base is absent or names no ref, and a base whose commit git
+// cannot read is a 500, with no fallback), policy-checks the new ref,
 // and CAS creates it. With force, an existing target is CAS-recreated at
 // the base — no-force-push policies still deny non-fast-forward moves via a
 // real ancestry check. No new objects are created, so there is nothing to
@@ -333,7 +335,8 @@ func (s *server) handleCreateBranch(w http.ResponseWriter, r *http.Request) {
 		sha, err := gitread.ResolveRef(ctx, repoPath, base, req.BaseIsEphemeral)
 		switch {
 		case errors.Is(err, gitread.ErrNotFound):
-			// Fall through to preferred_base below (when given).
+			// Fall through to preferred_base below (when given). An
+			// unreadable base is an error below, never a fallback.
 		case err != nil:
 			s.internalErr(w, r, "resolve base ref", err)
 			return

internal/api/read_query_test.go

@@ -1144,7 +1144,7 @@ func testQueryGrep(t *testing.T, e *testEnv) {
 				t.Fatal(err)
 			}
 			bin := t.TempDir()
-			wrapper := "#!/bin/sh\ncase \"$*\" in *" + fx.c4 + "^{commit}*) (sleep 5) & ;; esac\nexec " + realGit + " \"$@\"\n"
+			wrapper := "#!/bin/sh\nif [ \"$1\" = rev-parse ]; then for a in \"$@\"; do [ \"$a\" = '" + fx.c4 + "' ] && { (sleep 5) & }; done; fi\nexec " + realGit + " \"$@\"\n"
 			if err := os.WriteFile(filepath.Join(bin, "git"), []byte(wrapper), 0o755); err != nil {
 				t.Fatal(err)
 			}

internal/api/restore_target_test.go

@@ -1,7 +1,10 @@
 package api
 
 import (
+	"bytes"
+	"compress/zlib"
 	"context"
+	"io"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -76,6 +79,46 @@ func TestRestoreTargetTreeTellsMissingFromBroken(t *testing.T) {
 			t.Fatal(err)
 		}
 	}
+	// Commits whose object file is there but whose header git cannot
+	// read: cat-file -e finds them, and rev-parse exits 128 for them.
+	badHeader := strings.TrimSpace(coreGit(t, dir, "commit-tree", tree1, "-p", c1, "-m", "bad header"))
+	tagBadHeader := tag("bad-header", badHeader)
+	notZlib := strings.TrimSpace(coreGit(t, dir, "commit-tree", tree1, "-p", c1, "-m", "not zlib"))
+	rewriteLoose := func(oid string, data func(body []byte) []byte) {
+		t.Helper()
+		p := filepath.Join(dir, ".git", "objects", oid[:2], oid[2:])
+		f, err := os.Open(p)
+		if err != nil {
+			t.Fatal(err)
+		}
+		zr, err := zlib.NewReader(f)
+		if err != nil {
+			t.Fatal(err)
+		}
+		raw, err := io.ReadAll(zr)
+		_ = f.Close()
+		if err != nil {
+			t.Fatal(err)
+		}
+		_, body, ok := bytes.Cut(raw, []byte{0})
+		if !ok {
+			t.Fatalf("loose object %s has no header", oid)
+		}
+		if err := os.Remove(p); err != nil {
+			t.Fatal(err)
+		}
+		if err := os.WriteFile(p, data(body), 0o444); err != nil {
+			t.Fatal(err)
+		}
+	}
+	rewriteLoose(badHeader, func(body []byte) []byte {
+		var z bytes.Buffer
+		zw := zlib.NewWriter(&z)
+		_, _ = zw.Write(append([]byte("commit 12x\x00"), body...))
+		_ = zw.Close()
+		return z.Bytes()
+	})
+	rewriteLoose(notZlib, func([]byte) []byte { return []byte("not zlib data\n") })
 
 	sc, err := wcNewScratch(filepath.Join(dir, ".git"))
 	if err != nil {
@@ -100,6 +143,9 @@ func TestRestoreTargetTreeTellsMissingFromBroken(t *testing.T) {
 		{"commit whose tree is missing", c2, "", false, true},
 		{"tag of a missing commit", tagC3, "", false, true},
 		{"tag of a tag of a missing commit", tagTagC3, "", false, true},
+		{"commit with a header git cannot parse", badHeader, "", false, true},
+		{"tag of a commit with a header git cannot parse", tagBadHeader, "", false, true},
+		{"commit whose object file is not zlib data", notZlib, "", false, true},
 	} {
 		tree, found, err := sc.restoreTargetTree(context.Background(), tc.target)
 		if tree != tc.tree || found != tc.found || (err != nil) != tc.broken {
@@ -112,6 +158,16 @@ func TestRestoreTargetTreeTellsMissingFromBroken(t *testing.T) {
 	if _, err := sc.resolveTree(context.Background(), c4); err != nil {
 		t.Errorf("resolveTree(commit that a replacement makes a tree): %v", err)
 	}
+	// Only a full object id reaches restoreTargetTree: for an abbreviated
+	// one, cat-file -e exit 1 can also mean an ambiguous name.
+	t.Run("abbreviated target", func(t *testing.T) {
+		defer func() {
+			if recover() == nil {
+				t.Fatal("restoreTargetTree(abbreviated id) did not panic")
+			}
+		}()
+		_, _, _ = sc.restoreTargetTree(context.Background(), c1[:12])
+	})
 	// git peels the tags: nothing reads a tag object's message, which
 	// can be as large as the repository allows.
 	t.Run("no tag body read", func(t *testing.T) {
@@ -121,3 +177,38 @@ func TestRestoreTargetTreeTellsMissingFromBroken(t *testing.T) {
 		}
 	})
 }
+
+// A git run in restoreTargetTree that fails other than with its "no such
+// object" exit is the server's failure, an error, never "not found". So
+// is a commit at the end of the chain that rev-parse ^{commit} cannot
+// read.
+func TestRestoreTargetTreeGitFailures(t *testing.T) {
+	dir := t.TempDir()
+	coreGit(t, dir, "init", "-q", "-b", "main", ".")
+	coreWriteFile(t, dir, "a.txt", "one\n")
+	coreGit(t, dir, "add", ".")
+	coreGit(t, dir, "commit", "-q", "-m", "c1")
+	c1 := coreRevParse(t, dir, "HEAD")
+	tree1 := coreRevParse(t, dir, "HEAD^{tree}")
+	sc, err := wcNewScratch(filepath.Join(dir, ".git"))
+	if err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(sc.close)
+	for _, tc := range []struct{ name, target, arg, how string }{
+		{"rev-parse ^{commit} of a tree exits 128", tree1, tree1 + "^{commit}", "128"},
+		{"rev-parse ^{commit} of a tree is killed", tree1, tree1 + "^{commit}", "kill"},
+		{"cat-file -e exits 128", tree1, "-e", "128"},
+		{"cat-file -e is killed", tree1, "-e", "kill"},
+		{"cat-file -t exits 128", tree1, "-t", "128"},
+		{"cat-file -t is killed", tree1, "-t", "kill"},
+		{"rev-parse ^{commit} of a commit exits 1", c1, c1 + "^{commit}", "1"},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			gitFailsOnArg(t, tc.arg, tc.how)
+			if tree, found, err := sc.restoreTargetTree(context.Background(), tc.target); tree != "" || found || err == nil {
+				t.Fatalf("tree %q found %v err %v, want an error", tree, found, err)
+			}
+		})
+	}
+}

internal/gitread/README.md

@@ -21,7 +21,8 @@ Read-side git plumbing for the `git:read` API. It hydrates a cache repo, resolve
 
 ## Invariants
 - `assertf` panics on programming errors such as an empty repo path or a non-SHA commit argument [gitread.go:assertf]
-- `ResolveRef` builds candidates only from `check-ref-format`-valid names and admits raw revisions only if they match `hexRevRe`. Only git's exit 1 makes it answer `ErrNotFound`: every other git failure (a budget error, a kill, a fatal exit) is returned as is. An empty spec, a spec with a NUL byte, and one longer than `maxRefSpecBytes` (4096 bytes) are `ErrNotFound` before git runs [gitread.go:ResolveRef]
+- `ResolveRef` reads each candidate name directly with `rev-parse --verify --quiet` without a peel (so a ref's missing object still resolves), and its full id's type and peels with `cat-file --batch-check`. A hex abbreviation is read as `<abbrev>^{commit}` first, as on main: only the peel picks a commit that shares the abbreviation with a blob or a tree. On git's exit 1 it gets the plain lookup too. No command lists ref directories. git's DWIM rules stay in use, and revision operators and control bytes are refused before git starts: the hex fallback is the only object revision. Only git's exit 1 makes the name unknown: every other git failure (a budget error, a kill, a fatal exit) is returned as is. An empty spec, a spec with a NUL byte, and one longer than `maxRefSpecBytes` (4096 bytes) are `ErrNotFound` before git runs [gitread.go:ResolveRef]
+- `resolveCommitObject` reads three batch-check answers (the id's type, its `^{commit}` peel and its `^{}` peel). A full id that names no object and a tree, blob or tag of one are not found. A ref whose object is missing, a broken tag chain, and a commit git cannot read are errors [gitread.go:resolveCommitObject]
 - `IsBudgetError` errors must not be reported as not-found [budget.go:IsBudgetError]
 - `(*Budget).start` asserts the budget is open and returns `ErrTimeBudget` past the deadline; `limitedBuffer.Write` kills git once output is exhausted [budget.go:Budget.start]
 - `Run` never wraps `ExitError` for limit outcomes [budget.go:Run]