fix(sandbox): resolve git metadata carveouts for worktrees - #805
Conversation
.git is a file (gitdir: ...) in worktrees/submodules, not a dir. The hooks/config write-deny carveout assumed a directory and pointed at <root>/.git/hooks, which has no mkdir-able parent under the .git file. bwrap then failed 'Can't mkdir parents ... Not a directory', blocking every sandboxed tool in worktree checkouts. Resolve the real (common) git dir via the gitdir pointer + commondir so worktrees deny the shared hooks/config like a plain clone. Plain checkouts (.git dir) and .git-absent roots keep the literal paths.
Walkthrough
ChangesGit metadata carveout resolution
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/sandbox/profile.go`:
- Around line 72-75: Update the gitDir resolution flow around resolveGitDir so
an unresolved non-directory .git file does not retain the literal root/.git
fallback; return no carveouts for stale, malformed, or unreadable gitdir
pointers. Preserve the literal fallback only when .git is absent, and add a
regression test covering a stale gitdir: pointer.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a4a360b2-9b5c-48d9-b5ae-4d6f0af98017
📒 Files selected for processing (2)
internal/sandbox/profile.gointernal/sandbox/profile_worktree_test.go
| if info, err := os.Stat(gitDir); err == nil && !info.IsDir() { | ||
| if real := resolveGitDir(root); real != "" { | ||
| gitDir = resolveGitCommonDir(real) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Return no carveouts when a .git file cannot be resolved.
If resolveGitDir returns "" for a stale, malformed, or unreadable .git file, this leaves gitDir as <root>/.git and returns descendants of that file. That recreates the documented bwrap “Not a directory” failure and blocks sandboxed tools. Keep the literal fallback only when .git is absent; return no carveouts for an unresolved non-directory .git, with a regression test for a stale gitdir: pointer.
Proposed fix
if info, err := os.Stat(gitDir); err == nil && !info.IsDir() {
- if real := resolveGitDir(root); real != "" {
- gitDir = resolveGitCommonDir(real)
+ real := resolveGitDir(root)
+ if real == "" {
+ return nil
}
+ gitDir = resolveGitCommonDir(real)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if info, err := os.Stat(gitDir); err == nil && !info.IsDir() { | |
| if real := resolveGitDir(root); real != "" { | |
| gitDir = resolveGitCommonDir(real) | |
| } | |
| if info, err := os.Stat(gitDir); err == nil && !info.IsDir() { | |
| real := resolveGitDir(root) | |
| if real == "" { | |
| return nil | |
| } | |
| gitDir = resolveGitCommonDir(real) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/sandbox/profile.go` around lines 72 - 75, Update the gitDir
resolution flow around resolveGitDir so an unresolved non-directory .git file
does not retain the literal root/.git fallback; return no carveouts for stale,
malformed, or unreadable gitdir pointers. Preserve the literal fallback only
when .git is absent, and add a regression test covering a stale gitdir: pointer.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Thanks for this, and sorry it took a while to get a human on it. The bug you are fixing is real: a worktree or submodule keeps .git as a file, so the old hardcoded <root>/.git/hooks carveout points at a path with no parent directory, and bwrap fails with "Can't mkdir parents" and takes every sandboxed tool down with it. That needed fixing and the direction is right.
I am requesting changes because of what following the pointer opens up, which I verified rather than reasoned about.
The resolved git dir is not bounded to the workspace
resolveGitDir accepts an absolute gitdir: target verbatim and only validates it with os.Stat. resolveGitCommonDir then applies the same trust to whatever commondir contains. I ran three probes against your head 0c7ae90a:
gitdir: <absolute path outside the workspace>
carveouts = [<outside>/hooks <outside>/config]
gitdir: ../elsewhere (no absolute path needed)
carveouts = [<outside>/hooks <outside>/config]
gitdir: .git-real + .git-real/commondir -> <outside>
carveouts = [<outside>/hooks <outside>/config]
The third one matters on its own: even if you bound gitdir, commondir gives a second independent redirect.
Why that is a problem rather than a curiosity
The carveout is the thing keeping .git/hooks and .git/config write-denied, and your own comment says why: hooks auto-execute, and config carries core.hooksPath and credential.helper. Move the carveout and the real ones become writable.
Two facts make that reachable:
PermissionProfileFromPolicyis called atrunner.go:203for every command plan, so the pointer is re-read on each command, not resolved once at startup.ProtectedMetadataNamesis only.zeroand.agents. The.gitfile itself is not protected, and only<gitDir>/hooksand<gitDir>/configare read-only subpaths.
So in exactly the layout this PR is about, where .git is a file, a sandboxed command can rewrite that file, and the next command's profile puts the write-deny somewhere else. The command has then removed its own restriction on hooks and config.
To be fair about scope: this is not a pre-existing hole you widened. Before this change gitDir was hardcoded, so there was no pointer to poison. It is new surface that arrives with the fix, which is why I would rather sort it now than after it lands.
What I would like
- Bound the resolved directory. After resolving
gitdirandcommondir, require the result to sit inside the workspace (or inside the write roots) and otherwise refuse to use it. - Decide deliberately what happens when it does escape, and say so in a comment. Silently falling back to
""still leaves the real hooks and config unprotected, so "collapse to a no-op" is not automatically the safe answer. My instinct is that an escaping pointer should be treated as untrusted input rather than as a valid layout. - Consider protecting the
.gitfile itself. If the pointer cannot be rewritten, following it is a much smaller question. That may be the cleaner fix and it would let you keep the resolution simple.
Tests
TestGitMetadataWriteCarveoutsResolvesWorktree is genuinely load-bearing. I reverted the production change and kept your tests, and it failed, which is what it should do:
--- FAIL: TestGitMetadataWriteCarveoutsResolvesWorktree
--- PASS: TestGitMetadataWriteCarveoutsPlainCheckout
TestGitMetadataWriteCarveoutsPlainCheckout passing there is fine, it is a regression guard for the path you did not change, and I do not want it removed. Worth knowing though that it means one test covers the new logic.
The gap is that nothing covers the cases above. Whatever bounding you choose, please add a test for an escaping gitdir, for the relative ../ form, and for an escaping commondir with an in-workspace gitdir, since that last one fails differently from the other two. Also worth a case for the commondir read failing, since the fallback there is currently unexercised: swapping return gitDir for return "" leaves both tests green.
Happy to look again quickly once the bounding is in. The underlying fix is one we want.
2 participants
Problem
Worktree/submodule checkouts store
.gitas a file (gitdir: <path>), not a directory. The sandbox's git metadata write-deny carveout (gitMetadataWriteCarveoutsininternal/sandbox/profile.go) hardcoded<root>/.git/hooks+<root>/.git/config, assuming.gitis a directory.In a worktree that path has no mkdir-able parent (
.gitis a file), so the Linux bwrap backend's--tmpfs <root>/.git/hooksfails:→ every sandboxed tool is blocked in a worktree checkout (bash, file tools, etc.).
Fix
.gitis a file → resolve the real gitdir from thegitdir:pointer, followcommondir, and deny the sharedhooks/config(a worktree is protected like a plain clone)..gitis a dir or absent → unchanged (<root>/.git/{hooks,config}).resolveGitDir,resolveGitCommonDir.Tests
internal/sandbox/profile_worktree_test.go: worktree resolves the common dir + never points under the.gitfile; plain checkout keeps literal paths.TestSeatbeltProfileAllowsGitWritesExceptHooksAndConfigstill passes.Verification
<worktree>/.git/hooksbwrap: Can't mkdir parents ... Not a directory<common>/.git/hooksgit rev-parseruns fineSummary by CodeRabbit
.gitis a file.