Repository navigation
fix(api): restore only a commit, and answer 500 for one git cannot read - #771
Conversation
The deprecated target_commit_sha skips ref resolution, and resolveTree
peeled it with <sha>^{tree}: a tree id peels to itself, so a request
that named a tree restored that tree as a new commit (200). resolveTree
now peels ^{commit} first, so any id other than a commit's (or a tag's
that points to one) exits 1 and answers 404, like an unknown id. Its
other callers pass commit ids already.
TestRestoreCommit's new case sends a tree id and a blob id: before this
change the tree was restored with 200.
Change-Id: Ief14da6e055713f8871359c1388ee371846ac325
Signed-off-by: Thomas Kosiewski <tk@coder.com>
…nnot read
rev-parse --verify --quiet <target>^{commit}^{tree} exits 1 for a
target that names no commit, and also for a commit whose tree, or a
tag whose commit, git cannot read. restore-commit answered 404
"restore target not found" for both, so a broken repository read as
the client's unknown id.
restoreTargetTree keeps 404 for no object, a tree, a blob, or a tag of
one. For a commit, or a tag chain to one, that rev-parse could not
peel, it follows the tags' headers (which name the type of an object
that may be missing) and returns an error: 500.
TestRestoreTargetTreeTellsMissingFromBroken removes a commit's tree and
a tagged commit from a real repository. TestRestoreCommit's fake git
now also exits 1 on c1, which exists: 500.
Change-Id: I7f0e7aae02c94d835dba40b564fa198556be45b2
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.
Security findingsAdvisory findings (1)ℹ️ 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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7057f81223
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
There was a problem hiding this comment.
🛡️ Codex Security Review
Here are some automated security review suggestions for this pull request.
Reviewed commit: 7057f81223
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
Codex round 1 on #771: restoreTargetTree read each tag with cat-file tag, which buffers the whole tag object, message included, with no bound (P1, also its security lane). It also parsed the headers itself: an uppercase object id answered 500 instead of 404, a tag whose headers git cannot parse answered 404 instead of 500, and a chain of 64 tags hit the walk's own limit. Now git peels the chain: <target>^{} fails only when an object on the way is missing or a tag cannot be parsed (500), and cat-file -t names the object at its end: a commit (its tree is unreadable, 500), or a tree or a blob (404). No git call prints a tag's message. Change-Id: I428dcbd5c235802cfaa15c9356674510c0372064 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 753b987125
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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. |
Codex round 2 on #771: a refs/replace/<tree> that points at a commit, which any pusher can write, made a tree id peel as that commit, so a target_commit_sha naming a tree still restored (200). restoreTargetTree asks git whether the target is a commit with --no-replace-objects, and classifies a failed peel the same way. The tree of a real commit is then read as the rest of the write path reads trees. Change-Id: I60ffcf5c42cfe49bba68ca3ca816ae378bcac82b Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
🛡️ 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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 730ba04dc6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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 round 3 on #771: a refs/replace/<commit> that points at a tree, which any pusher can write, passed the replacement-free commit check, and then the replacement-aware tree read failed, so restoring a valid commit answered 500. restoreTargetTree reads the commit's tree with --no-replace-objects as well, so the type and the tree both come from the target's own objects. Change-Id: I49236be37a1d39ab8706579c5ebddad2ad313c65 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 27f3879c20
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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. |
Codex round 4 on #771: resolveTree's ^{commit} peel made a tip that refs/replace reads as a tree fail commit-pack and diff-commit with 500. Restore no longer reads its target through resolveTree, so resolveTree goes back to main's <commit>^{tree}. Change-Id: I55a8dc2f0b63e251a94b2abfffda6dc0fcf7bdf7 Signed-off-by: Thomas Kosiewski <tk@coder.com>
Change-Id: Iaaf4750c850e4d0e1fd54229d4652ca35c8ea024 Signed-off-by: Thomas Kosiewski <tk@coder.com>
Since #777, gitlimit sets GIT_NO_REPLACE_OBJECTS for every git child, so restore's git calls read each object as itself without a flag of their own. The replacement cases of the restore tests still pass, and fail when the registry does not set the variable. Change-Id: I6dc049a8c9888b97c0143477b2e2a896b77d54de Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. Another round soon, please! 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
POST /repos/restore-commitchecks its target withgit rev-parse --verify --quiet <target>^{tree}. Two cases go wrong:target_commit_shaskips ref resolution. A tree id peels to itself, so a request that named a tree restored that tree as a new commit (200).What changes
restoreTargetTree, not the sharedresolveTree.resolveTree, which commit-pack, diff-commit and merge read a parent's tree with, stays as on main (<commit>^{tree}).restoreTargetTreekeeps 404 for no object, a tree, a blob, or a tag of one. It peels<target>^{commit}first, so an id other than a commit's, or an annotated tag's that points to one, answers 404, like an unknown id. Since fix: run every git child without replacement objects #777, every git child reads each object as itself (GIT_NO_REPLACE_OBJECTSin gitlimit's registry), so arefs/replace/<id>cannot make a tree read as a commit, or a commit as a tree. When rev-parse exits 1 and the object exists, git peels its tag chain (<target>^{}). That peel fails only when an object on the way is missing or a tag cannot be parsed: 500. Otherwisecat-file -tnames the object at the end. A commit there has a tree git cannot read: 500. A tree or a blob: 404. No git call prints a tag's message, so a tag of any size costs the server nothing to classify.docs/reference/http-api.md:target_commit_shais "the 40-hex id of a commit or of an annotated tag of one".The note target keeps its check.
rev-parse --verify --quiet <sha>^{object}does not peel, so it exits 1 only for a missing object. A commit whose tree is missing still gets its note, andgit notes showexits 128 for a broken notes tree, which #760 answers with 500. A revisionobject_refresolves throughgitread.ResolveRef, which answers "not found" for a branch or tag whose commit object is missing. That is the case on every endpoint that resolves refs, and a follow-up PR fixes it.Verification
Red first. Against main at b3add73:
TestRestoreCommit's new case sends a tree id. Before the^{commit}change, the tree was restored (status 200, want 404, on both PostgreSQL and SQLite).TestRestoreTargetTreeTellsMissingFromBrokenbuilds a real repository. It removes one commit's tree and one tagged commit. Then it checks a commit, a tag, a tag of a tag, an unknown id, a tree, a blob, a tag of a tree, the commit without its tree, the tag of the missing commit, and a tag of that tag. With main's rule (exit 1 is "not found"), the three broken cases come back as not found with no error:TestRestoreCommit's fake git now also exits 1 on c1's^{commit}^{tree}. c1 exists, so the answer is 500.Round 2 (Codex round 1): round 1 read each tag with
cat-file tag, which buffered the whole object with no bound, and it parsed the headers itself. The test now also checks a tag of a tree named by an uppercase object id (404), a tag whose headers are out of order (500), and that no git call reads a tag's body (a git that dies on anytagargument). Against round 1:Round 3 (Codex round 2): a tree that no commit has, with
refs/replace/<tree>pointing at c1, is still no commit. Against round 2:restore_target_test.go:100: tree that a replacement makes a commit: tree "20e50a07..." found true err <nil>, want ... found false.Round 4 (Codex round 3): a commit that
refs/replace/<commit>makes read as a tree is still a commit, with its own tree. Against round 3:restore_target_test.go:105: commit that a replacement makes a tree: tree "" found false err restore target c7d6da7f...: commit has no readable tree: ....Round 5 (Codex round 4): round 4's
^{commit}peel in the sharedresolveTreemade a tip thatrefs/replace/<tip>reads as a tree fail commit-pack and diff-commit with 500. This round merges main, which now has #777, and restores main'sresolveTree. It also dropsrestoreTargetTree's per-call--no-replace-objects: the registry's variable covers every git child. The replacement cases ofTestRestoreTargetTreeTellsMissingFromBrokenstill pass. Withregistry.startnot setting the variable, they fail:Lanes on round 5's head, all with
-race: build, vet, golangci-lint (0 issues), the doc-link check and both--fingerprints-onlychecks pass. The restore, commit-pack, diff-commit and write-commit tests ofinternal/api(20 tests) pass on PostgreSQL, SQLite and S3-WAL.Lanes on round 1's head (7057f81), all with
-race:internal/apiandcmd/openstoragepass on PostgreSQL (-tags=openstorage_viewcheck), SQLite and S3-WAL (-tags=openstorage_viewcheck). Build, vet, golangci-lint (0 issues), the doc-link check and both--fingerprints-onlychecks pass. The first PostgreSQL run was killed by a SIGTERM that another workspace sent to everyflockon the host, so it has no result. Its rerun passed. On a merge with main,docs/reference/http-api.mdmerges cleanly and the doc-link check finds 0 broken links. The restore tests (TestRestoreCommit|TestRestoreTargetTreeTellsMissingFromBroken|TestWriteCommit,-race) pass on PostgreSQL, SQLite and S3-WAL at 730ba04 and at 27f3879.Size
223 changed lines: 72 of production Go (15 of them comments), 149 of tests and 2 of docs.
Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:xhigh