diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 00000000..b3191f88 --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,8 @@ +# AGENTS.md + +Rules for agents and reviewers in this repository. [CONTRIBUTING.md](CONTRIBUTING.md) has the build, test, lint and pull request conventions. + +## Code Review Rules + +- Replacement objects (`refs/replace/*`): every git child that the server starts runs with `GIT_NO_REPLACE_OBJECTS=1`, so git reads each object as itself. gitlimit's registry sets it, and a runner outside the registry adds the variable to its environment. Do not handle replacements per call instead, such as `--no-replace-objects` on one command or a replacement-aware case in one handler. Refusing a push to `refs/replace/*` is not per-call handling. +- Git's stderr: do not add or widen a text pattern whose job is to catch a forged line in git's stderr. Repository text, such as a path with a newline, can spell any line, so such a pattern never covers every case. Keep the repository's text out of git's stderr instead (for example, `GIT_ATTR_SOURCE` set to the empty tree), or decide by object state. Updating a classifier when git changes its own message is fine, and so is replacing a classifier with an object-state check. diff --git a/internal/gitread/README.md b/internal/gitread/README.md index 1c77b257..9626a289 100644 --- a/internal/gitread/README.md +++ b/internal/gitread/README.md @@ -35,6 +35,7 @@ Read-side git plumbing for the `git:read` API. It hydrates a cache repo, resolve - `LastCommits` fails with `ErrTooManyLastCommits` past `maxDistinct`; spool lines over 1 MiB or spool over 256 MiB fail with `ErrOutputLimit` [lastcommits.go:spoolWriter] - `Note` maps git exit 1 to `ErrNotFound`; `PeeledTags` skips non-commit tags [notes.go:Note] - A failed `Blame` reads only the exit status and fixed parts of git's stderr, never the path, which git rewrites in its messages. With ranges, exit 129, or exit 128 with stderr that starts `fatal: -L `, or that starts `fatal: file ` and ends `has only N line(s)`, read from the first line that starts `fatal: `, is `ErrBadRange`: git prints no other fatal line before that message, and a path in it comes after the fixed head. Otherwise `ls-tree` decides: a path that names no file (not one blob or symlink, or entries under `/`) is `ErrNotFound`, and any other failure is returned [blame.go:Blame] +- `Blame` and `Grep` run git with `GIT_ATTR_SOURCE` set to the empty tree, so that git reads no `.gitattributes` of the repository's own (git 2.41 and later honor it); those files make git print warnings that quote their paths unescaped. So the repository's attributes do not steer grep's `-I` binary detection or blame's textconv either: where git read them (a work tree, `attr.tree` or `GIT_ATTR_SOURCE=HEAD` in the server's environment, a bare repository on git 2.43.0 to 2.45.2), a text file marked `binary` or `-diff` is searched, and a file with a NUL byte and the `diff` attribute is skipped. A bare repository on git before 2.43.0, or from 2.45.3 on, read none before either [gitread.go:noTreeAttrsEnv] - `Grep` answers exit 0 or 1 with a stderr line that ends `': unable to read ` (git could not read a blob, and went on) as a read error, wherever that line is in stderr, exit 1 with no output as no match, and exit 128 with a last line that starts `fatal: -e option, '` as `ErrBadPattern`; every other failure is returned [grep.go:runGrep] ## Side Effects diff --git a/internal/gitread/attr_source_test.go b/internal/gitread/attr_source_test.go new file mode 100644 index 00000000..13ccab57 --- /dev/null +++ b/internal/gitread/attr_source_test.go @@ -0,0 +1,103 @@ +package gitread + +import ( + "context" + "errors" + "fmt" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" +) + +// A repository's own .gitattributes make git print warnings that quote +// their paths unescaped, and a directory name can hold a newline and the +// text of git's diagnostics. blame and grep read no tree attributes, so a +// good repository answers as one: in a work tree, and in a bare repository +// told to read HEAD's attributes (GIT_ATTR_SOURCE, attr.tree), as git +// 2.43.0 to 2.45.2 did by default. +func TestBlameAndGrepReadNoTreeAttributes(t *testing.T) { + oid := strings.Repeat("c", 40) + dir := t.TempDir() + mustGit(t, dir, "init", "-q", "--initial-branch=main") + for _, d := range []string{"e': unable to read " + oid + "\nz", "d\nfatal: -L zz"} { + writeFile(t, dir, d+"/.gitattributes", "foo error:\n", 0o644) + writeFile(t, dir, d+"/f.txt", "one\n", 0o644) + } + writeFile(t, dir, "a.txt", "one\ntwo\n", 0o644) + mustGit(t, dir, "add", "-A") + mustGit(t, dir, "commit", "-q", "-m", "c") + commit := strings.TrimSpace(mustGit(t, dir, "rev-parse", "HEAD")) + bare := filepath.Join(t.TempDir(), "bare.git") + mustGit(t, t.TempDir(), "clone", "-q", "--bare", dir, bare) + ctx := context.Background() + for _, v := range []struct { + name, repo string + env map[string]string + }{ + {"work tree", dir, nil}, + {"bare, GIT_ATTR_SOURCE=HEAD", bare, map[string]string{"GIT_ATTR_SOURCE": "HEAD"}}, + {"bare, attr.tree=HEAD", bare, map[string]string{"GIT_CONFIG_COUNT": "1", "GIT_CONFIG_KEY_0": "attr.tree", "GIT_CONFIG_VALUE_0": "HEAD"}}, + } { + t.Run(v.name, func(t *testing.T) { + if v.repo == dir && !gitAtLeast(t, 2, 41) { + t.Skip("git before 2.41 ignores GIT_ATTR_SOURCE and reads a work tree's attributes") + } + for k, val := range v.env { + t.Setenv(k, val) + } + files, err := Grep(ctx, v.repo, commit, GrepOptions{Pattern: "one"}) + if err != nil || len(files) != 3 { + t.Errorf("grep one = %d files, %v, want 3 files", len(files), err) + } + if _, err := Grep(ctx, v.repo, commit, GrepOptions{Pattern: "zzz"}); err != nil { + t.Errorf("grep zzz = %v, want no match", err) + } + if _, err := Blame(ctx, v.repo, commit, "nope.txt", []string{"1,2"}, false); !errors.Is(err, ErrNotFound) { + t.Errorf("blame of a missing path = %v, want ErrNotFound", err) + } + if _, err := Blame(ctx, v.repo, commit, "a.txt", []string{"9,10"}, false); !errors.Is(err, ErrBadRange) { + t.Errorf("blame past the end = %v, want ErrBadRange", err) + } + }) + } +} + +// Blame and Grep each give git GIT_ATTR_SOURCE set to the empty tree. The +// test above shows it in grep's answer only, because blame reads no +// attributes for the failures it classifies; this pins both. +func TestBlameAndGrepSetTheAttributeSource(t *testing.T) { + f := newFixture(t) + real, err := exec.LookPath("git") + if err != nil { + t.Fatal(err) + } + log := filepath.Join(t.TempDir(), "attr-source") + fakeGit(t, "printf '%s\\n' \"${GIT_ATTR_SOURCE-unset}\" >> '"+log+"'\nexec '"+real+"' \"$@\"") + ctx := context.Background() + if _, err := Blame(ctx, f.dir, f.c1, "README.md", nil, false); err != nil { + t.Fatalf("blame README.md: %v", err) + } + if _, err := Grep(ctx, f.dir, f.c1, GrepOptions{Pattern: "line"}); err != nil { + t.Fatalf("grep line: %v", err) + } + got, err := os.ReadFile(log) + if err != nil { + t.Fatal(err) + } + if want := EmptyTreeSHA + "\n" + EmptyTreeSHA + "\n"; string(got) != want { + t.Fatalf("GIT_ATTR_SOURCE of blame's and grep's git = %q, want the empty tree for both", got) + } +} + +// gitAtLeast reports whether the git on PATH is major.minor or later. +func gitAtLeast(t *testing.T, major, minor int) bool { + t.Helper() + out := strings.TrimSpace(mustGit(t, t.TempDir(), "version")) + var maj, min int + if _, err := fmt.Sscanf(strings.TrimPrefix(out, "git version "), "%d.%d", &maj, &min); err != nil { + t.Fatalf("parse %q: %v", out, err) + } + return maj > major || (maj == major && min >= minor) +} diff --git a/internal/gitread/blame.go b/internal/gitread/blame.go index fdd0beb6..b3eada77 100644 --- a/internal/gitread/blame.go +++ b/internal/gitread/blame.go @@ -45,7 +45,7 @@ func Blame(ctx context.Context, repoPath, commit, path string, ranges []string, args = append(args, "-M") } args = append(args, commit, "--", path) - out, err := runGit(ctx, Heavy, repoPath, args...) + out, err := Run(ctx, Cmd{Class: Heavy, Dir: repoPath, Env: noTreeAttrsEnv(), Args: args}) if err == nil { return parseBlamePorcelain(out) } diff --git a/internal/gitread/gitread.go b/internal/gitread/gitread.go index ee2b30f9..20f59855 100644 --- a/internal/gitread/gitread.go +++ b/internal/gitread/gitread.go @@ -18,6 +18,7 @@ import ( "context" "errors" "fmt" + "os" "os/exec" "regexp" "strings" @@ -52,6 +53,28 @@ func runGit(ctx context.Context, class Class, repoPath string, args ...string) ( return Run(ctx, Cmd{Class: class, Dir: repoPath, Args: args}) } +// noTreeAttrsEnv is the server's environment with GIT_ATTR_SOURCE set to +// the empty tree, so that git reads no .gitattributes from a tree or a +// work tree: only $GIT_DIR/info/attributes and the global and system +// attribute files, which a push cannot write. A repository's own +// attribute files make git print warnings that quote their paths +// unescaped, and a path can hold a newline, so blame and grep, which read +// git's stderr, run with it. git 2.41 and later honor the variable (git +// 2.43.0 to 2.45.2 read HEAD's attributes in a bare repository without +// it). An older git ignores it and reads no tree attributes in a bare +// repository. +// +// The repository's attributes also stop steering grep's -I binary +// detection and blame's textconv. Where git read them (a work tree, +// attr.tree or GIT_ATTR_SOURCE=HEAD in the server's environment, a bare +// repository on git 2.43.0 to 2.45.2), a text file marked binary or +// -diff is now searched, and a file with a NUL byte and the diff +// attribute is skipped. A bare repository, as the server runs, on git +// before 2.43.0 or from 2.45.3 on read no tree attributes before either. +func noTreeAttrsEnv() []string { + return append(os.Environ(), "GIT_ATTR_SOURCE="+EmptyTreeSHA) +} + // Materialize rebuilds the cache repo for repo from the ref/blob stores // (githttp.Hydrate: ensure repo, hydrate packs, sync refs from the DB // authority, point HEAD at defaultBranch) and returns its bare-repo path. diff --git a/internal/gitread/grep.go b/internal/gitread/grep.go index 41163d75..c0e13faa 100644 --- a/internal/gitread/grep.go +++ b/internal/gitread/grep.go @@ -120,7 +120,7 @@ func runGrep(ctx context.Context, repoPath, commit string, opts GrepOptions, ext } var stderr gitlimit.TailBuffer var unreadable grepUnreadableWatch - out, err := Run(ctx, Cmd{Class: Heavy, Dir: repoPath, Args: args, Stderr: io.MultiWriter(&stderr, &unreadable)}) + out, err := Run(ctx, Cmd{Class: Heavy, Dir: repoPath, Env: noTreeAttrsEnv(), Args: args, Stderr: io.MultiWriter(&stderr, &unreadable)}) var exitErr *exec.ExitError noMatch := errors.As(err, &exitErr) && exitErr.ExitCode() == 1 // git grep that cannot read a blob prints "error: '': unable to diff --git a/internal/gitread/grep_test.go b/internal/gitread/grep_test.go index fa11fc62..1679d080 100644 --- a/internal/gitread/grep_test.go +++ b/internal/gitread/grep_test.go @@ -132,8 +132,8 @@ func TestGrepBadPattern(t *testing.T) { f := newFixture(t) _, err := Grep(context.Background(), f.dir, f.c2, GrepOptions{Pattern: "["}) var exitErr *exec.ExitError - if !errors.As(err, &exitErr) || exitErr.ExitCode() != 128 { - t.Fatalf("bad-pattern grep err = %v, want exit status 128", err) + if !errors.Is(err, ErrBadPattern) || !errors.As(err, &exitErr) || exitErr.ExitCode() != 128 { + t.Fatalf("bad-pattern grep err = %v, want ErrBadPattern with exit status 128", err) } } @@ -226,6 +226,9 @@ func TestGrepReadErrorIsGitsUnreadableObjectLine(t *testing.T) { {"unreadable blob", "error: 'HEAD:b.txt': unable to read " + oid, true}, {"unreadable blob, then 70 KiB", "error: 'HEAD:b.txt': unable to read " + oid + "\n" + noise, true}, {"unreadable blob with a newline in its path", "warning: a\nerror: 'HEAD:x\ny.txt': unable to read " + oid + "\n", true}, + {"unreadable blob with a 64-hex (SHA-256) id", "error: 'HEAD:b.txt': unable to read " + strings.Repeat("ce", 32) + "\n", true}, + {"unreadable blob with a 300-byte path, in one write", "error: 'HEAD:" + strings.Repeat("d/", 150) + "b.txt': unable to read " + oid + "\n", true}, + {"unable to read without the quoted name", "error: unable to read " + oid + "\n", false}, {"attribute name", "error: is not a valid attribute name: .gitattributes:2\n", false}, {"diagnostic text inside a line", "warning: ': unable to read " + oid + " and more\n", false}, } { @@ -238,8 +241,10 @@ func TestGrepReadErrorIsGitsUnreadableObjectLine(t *testing.T) { } } -// A repository whose .gitattributes git complains about ("error: is not a -// valid attribute name") still greps: those lines are not read errors. +// A repository's own .gitattributes, even one that git complains about +// ("error: is not a valid attribute name"), does not stop grep: git 2.41 +// and later read no such file (noTreeAttrsEnv), and an older git's +// complaint is not a read error. func TestGrepAttributeErrorsAreNotReadErrors(t *testing.T) { dir := t.TempDir() mustGit(t, dir, "init", "-q", "--initial-branch=main") @@ -254,6 +259,28 @@ func TestGrepAttributeErrorsAreNotReadErrors(t *testing.T) { } } +// Only a last stderr line that starts with git's "fatal: -e option, '" is +// ErrBadPattern: the same text later in that line, after a path or other +// request text, or on an earlier line, is another failure. +func TestGrepBadPatternIsOnlyGitsHead(t *testing.T) { + f := newFixture(t) + for _, tc := range []struct { + name, stderr string + bad bool + }{ + {"git's head", "fatal: -e option, '[': Unmatched [, [^, [:, [., or [=\n", true}, + {"git's head after a warning", "warning: x\nfatal: -e option, '[': Unmatched [\n", true}, + {"the head inside the last line", "fatal: unable to read 'x fatal: -e option, '[': y'\n", false}, + {"the head on an earlier line", "fatal: -e option, '[': y\nfatal: bad object\n", false}, + } { + failingBlameGit(t, 128, tc.stderr, "grep") + _, err := Grep(context.Background(), f.dir, f.c1, GrepOptions{Pattern: "zzz"}) + if err == nil || errors.Is(err, ErrBadPattern) != tc.bad { + t.Errorf("%s: err = %.200v, want ErrBadPattern: %v", tc.name, err, tc.bad) + } + } +} + // grepUnreadableWatch reads a line that git writes in pieces. func TestGrepUnreadableWatchJoinsWrites(t *testing.T) { var w grepUnreadableWatch diff --git a/internal/lfsscan/README.md b/internal/lfsscan/README.md index 9001d1e4..43db4c15 100644 --- a/internal/lfsscan/README.md +++ b/internal/lfsscan/README.md @@ -14,7 +14,7 @@ Lenient Git LFS pointer decoder plus a bounded walk that runs three `git` subpro - `ParseLFSPointer` rejects data over `LFSPointerMaxSize` or containing NUL, needs a known version line and an `oid sha256:` line in any order, and needs a 64-char lowercase hex OID; it never panics on arbitrary input [pointer.go:ParseLFSPointer] - Each object charges 1 + size/4096 units; exceeding the budget returns `ErrLFSScanBudget` [scan.go:ReachableLFSOIDs] - An invalid rev-list OID, a `missing` batch-check line, a malformed size or header, or a failed subprocess fails the walk with no OID set [scan.go:ReachableLFSOIDs] -- Every git call uses `--no-replace-objects`, and rev-list runs unfiltered so every object is charged [scan.go:ReachableLFSOIDs] +- Every git call runs with `GIT_NO_REPLACE_OBJECTS=1` (`gitlimit.NoReplaceObjects`), and rev-list runs unfiltered so every object is charged [scan.go:ReachableLFSOIDs] ## Side Effects - Runs `git rev-list --objects --no-object-names --all`, `git cat-file --batch-check`, and `git cat-file --batch` in repoPath, each holding the repository's `gitlimit` lease until it exits [scan.go:ReachableLFSOIDs] [lease_test.go:TestReachableLFSOIDsLeaseFollowsEveryStage] diff --git a/internal/lfsscan/scan.go b/internal/lfsscan/scan.go index c001ad75..5e7a6a9e 100644 --- a/internal/lfsscan/scan.go +++ b/internal/lfsscan/scan.go @@ -73,7 +73,8 @@ func ReachableLFSOIDs(ctx context.Context, repoPath string, budget int64) (map[s defer lease.Close() // Every subprocess runs with object replacement disabled (Codex - // security round 1): refs/replace/* substitutions are writable by any + // security round 1), through GIT_NO_REPLACE_OBJECTS, which the start + // loop below sets: refs/replace/* substitutions are writable by any // git:write pusher WITHOUT moving the protected refs they shadow — an // attacker could replace a commit on main with a doctored twin whose // tree omits the pointer files, hiding their OIDs from the walk and @@ -90,7 +91,7 @@ func ReachableLFSOIDs(ctx context.Context, repoPath string, budget int64) (map[s // budget cancels gctx and kills the pipeline mid-walk — and the // batch-check stage below discards oversized blobs before any content // is read. - revList := exec.CommandContext(gctx, "git", "--no-replace-objects", "rev-list", "--objects", "--no-object-names", "--all") + revList := exec.CommandContext(gctx, "git", "rev-list", "--objects", "--no-object-names", "--all") revList.Dir = repoPath var revErr gitlimit.TailBuffer // the errors below reach the log revList.Stderr = &revErr @@ -99,7 +100,7 @@ func ReachableLFSOIDs(ctx context.Context, repoPath string, budget int64) (map[s return nil, 0, fmt.Errorf("rev-list stdout: %w", err) } - check := exec.CommandContext(gctx, "git", "--no-replace-objects", "cat-file", "--batch-check=%(objecttype) %(objectsize) %(objectname)", "--buffer") + check := exec.CommandContext(gctx, "git", "cat-file", "--batch-check=%(objecttype) %(objectsize) %(objectname)", "--buffer") check.Dir = repoPath var checkErr gitlimit.TailBuffer check.Stderr = &checkErr @@ -112,7 +113,7 @@ func ReachableLFSOIDs(ctx context.Context, repoPath string, budget int64) (map[s return nil, 0, fmt.Errorf("batch-check stdout: %w", err) } - read := exec.CommandContext(gctx, "git", "--no-replace-objects", "cat-file", "--batch", "--buffer") + read := exec.CommandContext(gctx, "git", "cat-file", "--batch", "--buffer") read.Dir = repoPath var readErr gitlimit.TailBuffer read.Stderr = &readErr @@ -127,6 +128,7 @@ func ReachableLFSOIDs(ctx context.Context, repoPath string, budget int64) (map[s started := make([]*exec.Cmd, 0, 3) for _, cmd := range []*exec.Cmd{revList, check, read} { + cmd.Env = gitlimit.NoReplaceObjects(cmd.Env) lease.Attach(cmd) if err := cmd.Start(); err != nil { // Kill the stages already running (Codex round 6): left diff --git a/internal/lfsscan/scan_test.go b/internal/lfsscan/scan_test.go index d0b18ad4..14388f35 100644 --- a/internal/lfsscan/scan_test.go +++ b/internal/lfsscan/scan_test.go @@ -300,6 +300,40 @@ func TestReachableLFSOIDs_IgnoresReplacementRefs(t *testing.T) { } } +// A refs/replace/ can also swap the pointer file's content +// for a blob that is no pointer, which cat-file --batch would read, or for +// an empty blob, which cat-file --batch-check would report as size 0, so +// that the walk never reads it. Every stage reads the true blob. +func TestReachableLFSOIDs_IgnoresReplacedBlobs(t *testing.T) { + for _, tc := range []struct{ name, content string }{ + {"by text", "not a pointer\n"}, + {"by an empty blob", ""}, + } { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + mustGit(t, dir, "init", "-q", "--initial-branch=main") + writeFile(t, dir, "a.bin", lfsPointer(oidA, 100), 0o644) + writeFile(t, dir, "other.txt", tc.content, 0o644) + mustGit(t, dir, "add", "-A") + mustGit(t, dir, "commit", "-q", "-m", "pointer commit") + pointerBlob := strings.TrimSpace(mustGit(t, dir, "rev-parse", "HEAD:a.bin")) + other := strings.TrimSpace(mustGit(t, dir, "rev-parse", "HEAD:other.txt")) + mustGit(t, dir, "replace", pointerBlob, other) + // Sanity: a replacement-honoring read gets the other content. + if got := mustGit(t, dir, "cat-file", "-p", pointerBlob); got != tc.content { + t.Fatalf("fixture broken: replacement did not swap the pointer blob (%d bytes)", len(got)) + } + oids, _, err := ReachableLFSOIDs(context.Background(), dir, 1_000) + if err != nil { + t.Fatal(err) + } + if _, ok := oids[oidA]; !ok { + t.Fatalf("a replaced pointer blob hid its OID: %v", oids) + } + }) + } +} + func TestReachableLFSOIDs_EmptyRepo(t *testing.T) { dir := t.TempDir() mustGit(t, dir, "init", "-q", "--initial-branch=main")