From 196b33a3dd085e38a4e0bc245062de7bc397fabe Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Sun, 8 Nov 2026 03:19:13 +0000 Subject: [PATCH 1/9] fix(gitread): run blame and grep without the repository's attributes GIT_ATTR_SOURCE, attr.tree, or a bare repository on git 2.43 to 2.45), a .gitattributes under a directory whose name holds a newline makes git print a warning that quotes the path unescaped, which can forge any stderr line. blame and grep, which read git's stderr, now run with GIT_ATTR_SOURCE set to the empty tree, so git reads no tree attributes. git 2.41 and later honor the variable; an older git ignores it and reads no tree attributes in a bare repository. Change-Id: Icfb04e9ac6156e05ce73cf023f49f61987cc6ec2 Signed-off-by: Thomas Kosiewski --- internal/gitread/README.md | 1 + internal/gitread/attr_source_test.go | 59 ++++++++++++++++++++++++++++ internal/gitread/blame.go | 2 +- internal/gitread/gitread.go | 18 +++++++++ internal/gitread/grep.go | 2 +- 5 files changed, 80 insertions(+), 2 deletions(-) create mode 100644 internal/gitread/attr_source_test.go diff --git a/internal/gitread/README.md b/internal/gitread/README.md index 1c77b257..aed66f6e 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 [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..74945a53 --- /dev/null +++ b/internal/gitread/attr_source_test.go @@ -0,0 +1,59 @@ +package gitread + +import ( + "context" + "errors" + "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 +// to 2.45 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) { + 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) + } + }) + } +} 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..94ddf21c 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,23 @@ 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, which the server writes. 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 to 2.45 read HEAD's attributes in a bare repository +// without it). An older git ignores it and reads no tree attributes in a +// bare repository. +func noTreeAttrsEnv() []string { + return append(os.Environ(), "GIT_ATTR_SOURCE="+emptyTreeID) +} + +// emptyTreeID is the id of the empty tree, which git knows in every +// repository without storing it. +const emptyTreeID = "4b825dc642cb6eb9a060e54bf8d69288fbee4904" + // 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 From fdd928403bb54bd6d6a449b8431c819f55d122f2 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Sun, 8 Nov 2026 03:33:45 +0000 Subject: [PATCH 2/9] refactor(gitread): use EmptyTreeSHA for the attribute source Change-Id: If4af6fcfc730cf5a666ff7a451c2c42c5afad672 Signed-off-by: Thomas Kosiewski --- internal/gitread/gitread.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/internal/gitread/gitread.go b/internal/gitread/gitread.go index 94ddf21c..88501b1f 100644 --- a/internal/gitread/gitread.go +++ b/internal/gitread/gitread.go @@ -63,13 +63,9 @@ func runGit(ctx context.Context, class Class, repoPath string, args ...string) ( // without it). An older git ignores it and reads no tree attributes in a // bare repository. func noTreeAttrsEnv() []string { - return append(os.Environ(), "GIT_ATTR_SOURCE="+emptyTreeID) + return append(os.Environ(), "GIT_ATTR_SOURCE="+EmptyTreeSHA) } -// emptyTreeID is the id of the empty tree, which git knows in every -// repository without storing it. -const emptyTreeID = "4b825dc642cb6eb9a060e54bf8d69288fbee4904" - // 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. From 9ca419239d78a997e9d2aee2eac357a54d058d13 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Sun, 8 Nov 2026 11:53:53 +0000 Subject: [PATCH 3/9] test(gitread): pin grep's read-error line for a SHA-256 id, a long line and its quoted name The tests now send git grep's unreadable-object line with a 64-hex id, with a 300-byte path in one write, and without the quoted name (not a read error), and check that a bad pattern is ErrBadPattern. Change-Id: I688b50fe977874406a711c569bfe5102a052001a Signed-off-by: Thomas Kosiewski --- internal/gitread/grep_test.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/internal/gitread/grep_test.go b/internal/gitread/grep_test.go index fa11fc62..5cc4d251 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}, } { From 233b0e22f90d9cb9f0ade29ce7b3cc2567778376 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Sun, 8 Nov 2026 11:54:06 +0000 Subject: [PATCH 4/9] docs: add AGENTS.md with the code review rules for replacement objects and git's stderr Change-Id: Ib5521da28e2249608ccfc4121a0d489d54889a8e Signed-off-by: Thomas Kosiewski --- AGENTS.md | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 AGENTS.md diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 00000000..b672febc --- /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/*`): keep `GIT_NO_REPLACE_OBJECTS=1` set for every git child, in gitlimit's registry (`registry.start`) and in the few runners outside it (`gitlimit.NoReplaceObjects`, the sync worker's `hermeticGitEnv`); do not add per-call handling, such as `--no-replace-objects` on one command or a replacement-aware case in one handler. +- Git's stderr: keep the existing classifiers of git's stderr (blame's range errors, grep's unreadable-object line, git apply's lines) as they are, and keep repository content out of git's stderr (for example `GIT_ATTR_SOURCE` set to the empty tree), or decide by git object state; do not add another text pattern that recognizes or rejects a forged stderr line. From 522659be42ae5c99d0005ea5780bf31775116267 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Sun, 8 Nov 2026 12:06:06 +0000 Subject: [PATCH 5/9] test(gitread): pin GIT_ATTR_SOURCE on blame's and grep's git Blame reads no attributes for the failures it classifies, so only grep's answer shows the variable. A fake git now records it for both. Change-Id: I9dddaccc0a6b76e709dfcd127ec46fc8fd414a6a Signed-off-by: Thomas Kosiewski --- internal/gitread/attr_source_test.go | 29 ++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/internal/gitread/attr_source_test.go b/internal/gitread/attr_source_test.go index 74945a53..28186db5 100644 --- a/internal/gitread/attr_source_test.go +++ b/internal/gitread/attr_source_test.go @@ -3,6 +3,8 @@ package gitread import ( "context" "errors" + "os" + "os/exec" "path/filepath" "strings" "testing" @@ -57,3 +59,30 @@ func TestBlameAndGrepReadNoTreeAttributes(t *testing.T) { }) } } + +// 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) + } +} From 48cce16ad951abd7d6792ae11fe38ddbdcdf9b0d Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Sun, 8 Nov 2026 12:33:43 +0000 Subject: [PATCH 6/9] docs(gitread): name the attribute files git still reads in noTreeAttrsEnv The server writes no info/attributes file. Say which files git still reads with GIT_ATTR_SOURCE set to the empty tree, and that a push cannot write them. Change-Id: Ia48fbfd730732921d6730b0dff892e665a00f822 Signed-off-by: Thomas Kosiewski --- internal/gitread/gitread.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/internal/gitread/gitread.go b/internal/gitread/gitread.go index 88501b1f..6d885b22 100644 --- a/internal/gitread/gitread.go +++ b/internal/gitread/gitread.go @@ -55,10 +55,11 @@ func runGit(ctx context.Context, class Class, repoPath string, args ...string) ( // 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, which the server writes. 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 +// 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 to 2.45 read HEAD's attributes in a bare repository // without it). An older git ignores it and reads no tree attributes in a // bare repository. From ccca7a5f8d0e40d80d5b46b24076b85e2f1797ee Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Sun, 8 Nov 2026 14:44:26 +0000 Subject: [PATCH 7/9] fix(lfsscan): run the walk's git with GIT_NO_REPLACE_OBJECTS, and sharpen the review rules Round 1 items on #781: - AGENTS.md: the git stderr rule bans only adding or widening a text pattern that catches forged lines, says why, and allows updating a classifier for git's own message changes or replacing one with an object-state check. Both rules state outcomes, not function lists. - lfsscan's three git children start outside gitlimit's registry and disabled replacements per call with --no-replace-objects. They now get GIT_NO_REPLACE_OBJECTS through gitlimit.NoReplaceObjects, and TestReachableLFSOIDs_IgnoresReplacementRefs goes red without it. - noTreeAttrsEnv's comment and the gitread README say that grep's -I binary detection and blame's textconv no longer read the repository's attributes either. - TestBlameAndGrepReadNoTreeAttributes skips its work-tree case on git before 2.41, which ignores GIT_ATTR_SOURCE, and the comment of TestGrepAttributeErrorsAreNotReadErrors says why grep still works. - TestGrepBadPatternIsOnlyGitsHead pins that only a last line starting with git's "fatal: -e option, '" is ErrBadPattern. Change-Id: I37aa13b9b8f4c671fb78f0937cbc86156c67a01b Signed-off-by: Thomas Kosiewski --- AGENTS.md | 4 ++-- internal/gitread/README.md | 2 +- internal/gitread/attr_source_test.go | 15 +++++++++++++++ internal/gitread/gitread.go | 7 +++++++ internal/gitread/grep_test.go | 28 ++++++++++++++++++++++++++-- internal/lfsscan/README.md | 2 +- internal/lfsscan/scan.go | 10 ++++++---- 7 files changed, 58 insertions(+), 10 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index b672febc..a1118220 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -4,5 +4,5 @@ Rules for agents and reviewers in this repository. [CONTRIBUTING.md](CONTRIBUTIN ## Code Review Rules -- Replacement objects (`refs/replace/*`): keep `GIT_NO_REPLACE_OBJECTS=1` set for every git child, in gitlimit's registry (`registry.start`) and in the few runners outside it (`gitlimit.NoReplaceObjects`, the sync worker's `hermeticGitEnv`); do not add per-call handling, such as `--no-replace-objects` on one command or a replacement-aware case in one handler. -- Git's stderr: keep the existing classifiers of git's stderr (blame's range errors, grep's unreadable-object line, git apply's lines) as they are, and keep repository content out of git's stderr (for example `GIT_ATTR_SOURCE` set to the empty tree), or decide by git object state; do not add another text pattern that recognizes or rejects a forged stderr line. +- 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 sets it with `gitlimit.NoReplaceObjects`. Do not handle replacements per call instead, such as `--no-replace-objects` on one command or a replacement-aware case in one handler. +- 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 aed66f6e..3796f9af 100644 --- a/internal/gitread/README.md +++ b/internal/gitread/README.md @@ -35,7 +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 [gitread.go:noTreeAttrsEnv] +- `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`, a bare repository on git 2.43 to 2.45), 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 2.46 or later 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 index 28186db5..0fbe4a6b 100644 --- a/internal/gitread/attr_source_test.go +++ b/internal/gitread/attr_source_test.go @@ -3,6 +3,7 @@ package gitread import ( "context" "errors" + "fmt" "os" "os/exec" "path/filepath" @@ -40,6 +41,9 @@ func TestBlameAndGrepReadNoTreeAttributes(t *testing.T) { {"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) } @@ -86,3 +90,14 @@ func TestBlameAndGrepSetTheAttributeSource(t *testing.T) { 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/gitread.go b/internal/gitread/gitread.go index 6d885b22..16643c4f 100644 --- a/internal/gitread/gitread.go +++ b/internal/gitread/gitread.go @@ -63,6 +63,13 @@ func runGit(ctx context.Context, class Class, repoPath string, args ...string) ( // variable (git 2.43 to 2.45 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, a bare repository on git 2.43 to 2.45), 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 on git 2.46 or later, as +// the server runs, read no tree attributes before either. func noTreeAttrsEnv() []string { return append(os.Environ(), "GIT_ATTR_SOURCE="+EmptyTreeSHA) } diff --git a/internal/gitread/grep_test.go b/internal/gitread/grep_test.go index 5cc4d251..1679d080 100644 --- a/internal/gitread/grep_test.go +++ b/internal/gitread/grep_test.go @@ -241,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") @@ -257,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 From 05a9f93f6f1196eebcfcf4363fdd43e58825288f Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Sun, 8 Nov 2026 15:58:30 +0000 Subject: [PATCH 8/9] docs: name the exact git versions that read a bare repository's HEAD attributes A bare repository reads HEAD's attributes by default only on git 2.43.0 to 2.45.2 (attr.c sets the default at v2.45.2, not at v2.45.3). The noTreeAttrsEnv comment, the gitread README and the test comment said 2.43 to 2.45, and that 2.46 and later read none. AGENTS.md's replacement rule states the outcome for a runner outside the registry (it adds the variable to its environment), and says that refusing a push to refs/replace/* is not per-call handling. Change-Id: Ica52e8db9d55ff0bb6d5fff02c1f0ef62a601df3 Signed-off-by: Thomas Kosiewski --- AGENTS.md | 2 +- internal/gitread/README.md | 2 +- internal/gitread/attr_source_test.go | 4 ++-- internal/gitread/gitread.go | 17 +++++++++-------- 4 files changed, 13 insertions(+), 12 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index a1118220..b3191f88 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -4,5 +4,5 @@ Rules for agents and reviewers in this repository. [CONTRIBUTING.md](CONTRIBUTIN ## 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 sets it with `gitlimit.NoReplaceObjects`. Do not handle replacements per call instead, such as `--no-replace-objects` on one command or a replacement-aware case in one handler. +- 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 3796f9af..55e16f8d 100644 --- a/internal/gitread/README.md +++ b/internal/gitread/README.md @@ -35,7 +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`, a bare repository on git 2.43 to 2.45), 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 2.46 or later read none before either [gitread.go:noTreeAttrsEnv] +- `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`, 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 index 0fbe4a6b..13ccab57 100644 --- a/internal/gitread/attr_source_test.go +++ b/internal/gitread/attr_source_test.go @@ -15,8 +15,8 @@ import ( // 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 -// to 2.45 did by default. +// 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() diff --git a/internal/gitread/gitread.go b/internal/gitread/gitread.go index 16643c4f..e270124f 100644 --- a/internal/gitread/gitread.go +++ b/internal/gitread/gitread.go @@ -59,17 +59,18 @@ func runGit(ctx context.Context, class Class, repoPath string, args ...string) ( // 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 to 2.45 read HEAD's attributes in a bare repository -// without it). An older git ignores it and reads no tree attributes in a -// bare repository. +// 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, a bare repository on git 2.43 to 2.45), 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 on git 2.46 or later, as -// the server runs, read no tree attributes before either. +// attr.tree, 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) } From 2aa70817fb194ba2ddc3294bf01d4f80bee5f8da Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Sun, 8 Nov 2026 16:25:25 +0000 Subject: [PATCH 9/9] test(lfsscan): a replaced pointer blob hides no OID; docs: name GIT_ATTR_SOURCE=HEAD The #781 verifier's gap: TestReachableLFSOIDs_IgnoresReplacementRefs replaces only a commit, so GIT_NO_REPLACE_OBJECTS on rev-list alone passed it. TestReachableLFSOIDs_IgnoresReplacedBlobs replaces the pointer blob by text, which cat-file --batch would read, and by an empty blob, which cat-file --batch-check would report as size 0. Leaving the variable off any one of the three children fails a test. The noTreeAttrsEnv comment and the gitread README also list GIT_ATTR_SOURCE=HEAD in the server's environment among the setups whose answers change. Change-Id: I6028bee8bbc2b43353e09cae07fadbdc53a77969 Signed-off-by: Thomas Kosiewski --- internal/gitread/README.md | 2 +- internal/gitread/gitread.go | 10 +++++----- internal/lfsscan/scan_test.go | 34 ++++++++++++++++++++++++++++++++++ 3 files changed, 40 insertions(+), 6 deletions(-) diff --git a/internal/gitread/README.md b/internal/gitread/README.md index 55e16f8d..9626a289 100644 --- a/internal/gitread/README.md +++ b/internal/gitread/README.md @@ -35,7 +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`, 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] +- `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/gitread.go b/internal/gitread/gitread.go index e270124f..20f59855 100644 --- a/internal/gitread/gitread.go +++ b/internal/gitread/gitread.go @@ -66,11 +66,11 @@ func runGit(ctx context.Context, class Class, repoPath string, args ...string) ( // // 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, 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. +// 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) } 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")