Repository navigation
fix(gitread): run blame and grep without the repository's attributes - #781
Conversation
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 <tk@coder.com>
Change-Id: If4af6fcfc730cf5a666ff7a451c2c42c5afad672 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…ne 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 <tk@coder.com>
…s and git's stderr Change-Id: Ib5521da28e2249608ccfc4121a0d489d54889a8e Signed-off-by: Thomas Kosiewski <tk@coder.com>
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 <tk@coder.com>
…sEnv 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 <tk@coder.com>
|
@codex review |
|
@codex security review |
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
…rpen 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 <tk@coder.com>
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
…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 <tk@coder.com>
…TTR_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 <tk@coder.com>
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
Why
Blame and grep read git's stderr to classify a failure (#760, #772). A repository's own
.gitattributesfiles can put repository text into that stderr. A directory whose name holds a newline, with a crafted.gitattributesin it, makes git print a warning that quotes the path unescaped. That path can spell a whole lineerror: '<name>': unable to read <oid>, and grep then reads a good repository as unreadable and answers 500. git reads tree attributes in a work tree, in a bare repository withattr.treeorGIT_ATTR_SOURCEset, and in a bare repository by default on git 2.43.0 to 2.45.2. The shipped image does not read them by default. The root verifier of #772 found this.This PR removes the repository text from git's stderr. It adds no text pattern.
What changes
BlameandGreprun git withGIT_ATTR_SOURCEset to the empty tree (noTreeAttrsEnvininternal/gitread/gitread.go). git then reads no.gitattributesfrom a tree or a work tree. It still reads$GIT_DIR/info/attributesand the global and system attribute files, which a push cannot write. git 2.41 and later honor the variable. An older git ignores it, and an older git reads no tree attributes in a bare repository.Runstill setsGIT_NO_REPLACE_OBJECTSfor these children: gitlimit adds it to anyCmd.Env.-Ibinary detection and blame's textconv. Where git read them (a work tree,attr.treeorGIT_ATTR_SOURCE=HEADin the server's environment, a bare repository on git 2.43.0 to 2.45.2), a text file markedbinaryor-diffis now searched, and a file with a NUL byte and thediffattribute is skipped. A bare repository on git before 2.43.0, or from 2.45.3 on, as the server runs, read no tree attributes before, so nothing changes there.lfsscan's three git children start outside gitlimit's registry and disabled replacements per call with--no-replace-objects, which the new rule bans. They now getGIT_NO_REPLACE_OBJECTSthroughgitlimit.NoReplaceObjects.ErrBadPatternat the gitread level.AGENTS.mdwith a## Code Review Rulessection. Replacement objects: every git child runs withGIT_NO_REPLACE_OBJECTS=1, with no per-call handling. Git's stderr: do not add or widen a text pattern that catches forged lines, because repository text such as a path with a newline can spell any line. Keep repository text out of git's stderr or decide by object state instead. Updating a classifier for git's own message changes, or replacing one with an object-state check, is fine.Verification
TestBlameAndGrepReadNoTreeAttributesbuilds a repository with two such directories (one forges grep's read-error line, one forges blame'sfatal: -Lline). It runs grep and blame in a work tree, and in a bare repository withGIT_ATTR_SOURCE=HEADand withattr.tree=HEAD. Grep finds its matches and answers no match. Blame of a missing path isErrNotFound, and blame past the end isErrBadRange.TestBlameAndGrepSetTheAttributeSourcerecords theGIT_ATTR_SOURCEthat each git child gets, and wants the empty tree for blame and for grep. The work-tree case ofTestBlameAndGrepReadNoTreeAttributesskips on git before 2.41, which ignores the variable.t.txt binary,m.txt -diffandb.dat diff(b.dat holds a NUL byte) greps-Itob.datandplain.txtwith the attributes, and tom.txt,plain.txtandt.txtwithGIT_ATTR_SOURCEset to the empty tree.TestGrepBadPatternIsOnlyGitsHeadpins that only a last stderr line that starts withfatal: -e option, 'isErrBadPattern: not that text later in the line, and not on an earlier line.TestReachableLFSOIDs_IgnoresReplacementRefs(lfsscan) fails withoutGIT_NO_REPLACE_OBJECTSon the three children: "replacement ref hid a reachable pointer OID". It replaces a commit, which onlyrev-listreads.TestReachableLFSOIDs_IgnoresReplacedBlobsreplaces the pointer blob by text, whichcat-file --batchwould read, and by an empty blob, whichcat-file --batch-checkwould report as size 0. With the variable on only one or two of the three children, each of the four such mutants fails one of the two tests.noTreeAttrsEnv:attr_source_test.go:48("grep one = 0 files ... could not read the repository").noTreeAttrsEnv:attr_source_test.go:86(blame's git gets noGIT_ATTR_SOURCE). Blame's classified failures read no attributes, so only the recording test catches this one.':prefix, or keeping only the head of a long write:grep_test.go:238.runGrepreturning a bad pattern's error withoutErrBadPattern:grep_test.go:136.strings.Contains, against the whole stderr, or on any line:grep_test.go:279.gitlimit.NoReplaceObjects:scan_test.go:299.-race: build, vet, golangci-lint (0 issues), the doc-link check and both--fingerprints-onlychecks pass, andinternal/gitread/...andinternal/lfsscan/...pass. 05a9f93 and 2aa7081 change only docs, comments and lfsscan tests after ccca7a5.-race: build, vet (alsoGOOS=darwin go vetofinternal/gitreadandinternal/lfsscan), golangci-lint (0 issues), the doc-link check and both--fingerprints-onlychecks pass.internal/gitread/...andinternal/lfsscan/...pass, andinternal/jobs, which runs the LFS walk, passes on PostgreSQL.noTreeAttrsEnv. Build, vet, golangci-lint (0 issues), the doc-link check and both--fingerprints-onlychecks pass.internal/gitread/...passes.internal/apiandcmd/openstoragepass on PostgreSQL (-tags=openstorage_viewcheck), SQLite and S3-WAL. The first PostgreSQL run ofcmd/openstoragestopped before it reached the code (the postgres container exited at start), and its rerun at 48cce16 passes.Size
220 changed lines: 37 of production Go, 172 of tests and 11 of docs.
Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:xhigh