Revalidate cached stack leaves before directory reuse#2559
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63192ce900
ℹ️ 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 (@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 (@codex) address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR addresses a checkout path-stack validation gap by ensuring cached “leaf” components are revalidated when later reused as parent directories, and by improving stack state restoration on certain failure paths.
Changes:
- Update
gix_fs::Stack::make_relative_path_current()to re-run delegate validation when a cached leaf becomes a directory, and adjust delegate call ordering/state restoration. - Extend
gix-fsstack tests to cover failure recovery around leaf→directory transitions and update expected delegate call counts. - Add
gix-worktreeregression tests covering symlink collisions when a previously cached leaf is later used as a directory (forced vs non-forced behavior).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
gix-worktree/tests/worktree/stack/create_directory.rs |
Adds regression tests for revalidating cached leaves when encountering symlink collisions during directory creation. |
gix-fs/tests/fs/stack.rs |
Adds failure-mode tests for leaf→directory transitions and updates expected delegate call counts. |
gix-fs/src/stack.rs |
Implements leaf revalidation via delegate calls and improves rollback behavior for some delegate failure paths. |
63192ce to
1d2f69e
Compare
1d2f69e to
74e4b20
Compare
Add a gix integration fixture that constructs a malformed tree with a directory/file conflict on the same path: - `a` as a symlink to `.git/hooks` - `a` as a tree containing `post-checkout -> ../../payload` - `payload` as an executable file The tree is created with `git hash-object --literally` so the duplicate root entries are preserved in the fixture repository. Add a clone checkout regression test that clones this repository and asserts that checkout does not write `.git/hooks/post-checkout` through the attacker-controlled `a` symlink prefix. The test also checks that the payload itself is checked out and remains executable. This currently reproduces the vulnerability: delayed symlink checkout creates `a` first, then reuses the cached `a` prefix when processing `a/post-checkout`, allowing the final symlink creation to resolve through `a` into `.git/hooks/post-checkout`. The fixture is Unix-only at the test level because the exploit depends on Unix symlink behavior. Co-authored-by: Sebastian Thiel <sebastian.thiel@icloud.com>
74e4b20 to
83628bf
Compare
GHSA-f89h-2fjh-2r9q reports that checkout could process a later path beneath a previously cached leaf without replaying the delegate validation and directory-creation path for that reused component. The generic gix_fs::Stack now promotes cached leaves through delegate.push(false, ...) before entering directory state, so worktree checkout re-runs collision handling and path validation before using the component as a parent. The new error handling restores the prior leaf or directory state if delegate setup or a child push fails, keeping reusable stack state balanced after recoverable errors. Regression coverage exercises both overwrite modes in gix-worktree and the gix-fs delegate callback/error-state transitions. Git baseline checked in /Users/byron/dev/github.com/git/git at 94f057755b: symlinks.c check_leading_path() uses lstat-based leading-path checks, and entry.c unlink_entry() consults that path check before removing entries.
6c41c25 to
cedcb63
Compare
Add a Criterion benchmark for `gix_index::State::from_tree()` to track the cost of constructing an index from in-memory tree objects. The benchmark covers the same broad input shapes used by the existing `to_tree` benchmark: - a flat tree with 10k files - a wide/deep tree with 100 directories and 100 files each - a sparse-directory-like tree with 10k tree entries The benchmark builds each input once with `gix_object::tree::Editor`, stores serialized tree objects in an in-memory object database, and then measures only `State::from_tree()` over those prebuilt trees. Throughput is reported in logical entries, so regressions from extra sorting, conflict handling, or tree traversal changes are easier to compare across shapes. This is especially useful while `from_tree()` performs post-collection normalization for file/directory conflicts. The desired long-term state is to preserve the same normalized index invariant while moving closer to Git’s insertion-time conflict handling, avoiding unnecessary whole-index work when traversal already produces entries in order. Co-authored-by: Sebastian Thiel <sebastian.thiel@icloud.com>
cedcb63 to
f7bae0f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7bae0f2c5
ℹ️ 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 (@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 (@codex) address that feedback".
This is currently not easily exploitable as anything that interacts with the worktree goes through a symlink check, which catches this issue naturally. The plan is to get closer to how Git keeps the sort order, which detects such issues more naturally. Co-authored-by: Sebastian Thiel <sebastian.thiel@icloud.com>
f7bae0f to
2facc6f
Compare
|
Bypassed check of 'changelog' commit because it just changed some version numbers and a changelog. |
Tasks
gix_index::State::from_tree()gix-index regression
Index-reading is about 20% slower at least. But it has to be for now.
This PR was created by Codex on behalf of Byron. Byron will review it before it is merged.
Advisory
Advisory URL: GHSA-f89h-2fjh-2r9q
GHSA ID: GHSA-f89h-2fjh-2r9q
Advisory summary
This fixes a checkout path-stack validation gap without reproducing exploit details from the unpublished advisory. Cached leaf path components are now revalidated before being reused as parent directories, and stack state is restored correctly if delegate validation or directory setup fails.
Git baseline
Checked /Users/byron/dev/github.com/git/git at 94f057755b. Git uses lstat-based leading-path checks in symlinks.c and consults check_leading_path() from entry.c before removing entries.
Validation
Note: a later codex review --commit attempt was started, produced continuous exploratory output, and was terminated after running unreasonably long without a final verdict.