test(sandbox): add worktree edge-case and Linux integration tests#44
Open
nhuelstng wants to merge 7 commits into
Open
test(sandbox): add worktree edge-case and Linux integration tests#44nhuelstng wants to merge 7 commits into
nhuelstng wants to merge 7 commits into
Conversation
`omac sandbox run` grants the workdir read+write. For a plain clone that
covers `.git` (it lives inside the workdir), but a linked worktree splits
repo state: the workdir is `<repo>/.worktrees/<name>/` while the git-admin
dir is `<repo>/.git/worktrees/<name>/` — a sibling outside the workdir. So
`git add`/`commit` hit EPERM writing the object store, refs, reflogs and
per-worktree index/HEAD, and the agent can't commit from any worktree.
Detect a linked worktree (`.git` is a file), resolve the shared common dir
via the admin dir's `commondir`, and restore plain-clone parity by granting
the commit-relevant subdirs at the workdir's access level:
- allow (rw): objects, refs, logs, packed-refs, worktrees/<name>
- read-only: config, info (blocks core.hooksPath / credential.helper)
- deny: hooks/ (a writable hooks dir would run un-sandboxed
as the host user on the next host-side commit)
No-op for a plain clone or a submodule (whose admin dir has no `commondir`).
Verified below the process boundary: a real-`git worktree` pipeline test
drives ResolveGrants -> SBPL and bwrap argv generation and asserts the
object store is bound read+write, config read-only, and hooks masked.
Signed-off-by: Sajjad Ahmad <sajjad.ahmad@tngtech.com>
The worktree grant paths are derived from in-workdir file content (`.git`, `<admin>/commondir`) that a prior sandboxed session could have tampered with, and both backends symlink-canonicalize every grant into a kernel rule. A malicious agent with rw workdir could plant a crafted `.git` + admin/`commondir` chain whose resolved common dir it controls, with `objects` a symlink to a sensitive dir (e.g. ~/.ssh). Because SBPL emits write-allows AFTER protected-path denies (last match wins), the symlink-resolved write grant would punch through the ~/.ssh denial — read+write escape on the next `omac sandbox run` in that workdir. Confine the grant: - Enforce git's structural invariant: admin must be <common>/worktrees/<name>, rejecting a commondir aimed at an unrelated location. - Admit only subdirs whose symlink-resolved path physically lives inside the resolved common dir; drop any entry that escapes (pathWithinRoot). Adds regression tests proving a symlinked `objects`/`refs` never widens a grant to the target, and a spoofed commondir yields no worktree grants. Signed-off-by: Sajjad Ahmad <sajjad.ahmad@tngtech.com>
End-to-end testing under real Seatbelt showed the earlier "make packed-refs writable" change was both ineffective and unnecessary: - git 2.x's ref transaction tries to create <common>/packed-refs.LOCK in the common-dir root on every ref update. That root is deliberately not granted (it holds config and hooks), so the lock EPERMs — but git falls back to the loose-ref path and the commit succeeds (verified: exit 0, fsck clean, refs correct). Granting the packed-refs *file* rw cannot silence this: the blocker is the lock sibling in the ungranted root. - Granting the common root rw to allow the lock would, on macOS, let a config write slip through (SBPL emits write-allows last, after the protected denies), defeating the core.hooksPath guard — the exact vector #30 closes. So the harmless lock noise is the accepted cost. A normal add/commit only ever writes loose refs under refs/; packed-refs is only rewritten by gc/pack-refs, which need that ungranted lock anyway. So packed-refs is correctly read-only (git still reads it for status/log and for committing onto a packed ref). Documented the limitation on gitWorktreeGrants. Signed-off-by: Sajjad Ahmad <sajjad.ahmad@tngtech.com>
Flip the linked-worktree shared hooks dir from a hard deny to read-only: readable+runnable so host-authored commit hooks (prepare-commit-msg, commit-msg, pre-commit) execute inside the sandbox, but never writable so the agent still cannot plant a hook that would run un-sandboxed on the host's next commit (the #30 persistence vector). The read grant stays inside the contained() symlink-containment check, since unlike a deny it would otherwise follow a planted hooks->secret symlink to the target. Add a real-Seatbelt e2e (rooted under $HOME, not $TMPDIR, so the baseline temp-write grant doesn't mask the read-only hooks grant) proving the hook runs during a sandboxed commit while the hooks dir stays unwritable. Signed-off-by: Sajjad Ahmad <sajjad.ahmad@tngtech.com>
Signed-off-by: Sajjad Ahmad <sajjad.ahmad@tngtech.com>
Adds 7 tests covering gaps identified during PR #31 review: Unit tests (grants_test.go): - Submodule exclusion: submodule admin dir has no commondir, so no worktree grants leak into the parent common dir. - AccessWrite worktree: write-only workdir broadens worktree subdirs to read+write (git must read objects/refs to commit); config/info/ packed-refs/hooks stay read-only. - Bare-repo worktree: worktree from a bare repo resolves through the git-invariant check (common dir is the bare repo dir itself). - Concurrent worktrees isolation: two worktrees sharing one common dir each get grants for their OWN admin dir only, never the sibling's. Linux integration tests (integration_worktree_linux_test.go, new file): - Worktree hooks run but hooks dir not writable (Linux counterpart of the darwin test). - Symlink escape blocked under real bwrap (objects -> secret). - Known limitations: branch -d fails cleanly (ref preserved), gc no-ops, commit succeeds despite packed-refs.lock EPERM, fsck clean. - Landlock + worktree combined path: commit succeeds (fs binds) AND network stays blocked (Landlock holds) in the same sandbox. Signed-off-by: Niclas Hülsmann <niclas.huelsmann@tngtech.com>
staticcheck SA4006: out never used after branch -d call. Replace out, code with _, code. Fixes lint failure in #44. Signed-off-by: Niclas Hülsmann <niclas.huelsmann@tngtech.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Supplements PR #31 with tests for edge cases and the Linux backend identified during review.
What
Adds 7 tests covering gaps found in the PR #31 review:
Unit tests (
grants_test.go)TestResolveGrantsSubmoduleNoWorktreeGrantscommondir→ no worktree grants leak into parent common dir. Pins the PR body's submodule exclusion claim.TestResolveGrantsWorktreeAccessWriteAccessWrite(write-only) broadens worktree subdirs to read+write (git must read to commit); config/info/packed-refs/hooks stay read-only.TestResolveGrantsBareRepoWorktreeTestResolveGrantsConcurrentWorktreesIsolationLinux integration tests (
integration_worktree_linux_test.go, new file)TestIntegrationWorktreeHooksRunButNotWritableTestIntegrationWorktreeSymlinkEscapeobjects -> secretsymlink blocked under real bwrap.TestIntegrationWorktreeKnownLimitationsbranch -dfails cleanly (ref preserved),gcno-ops, commit succeeds despitepacked-refs.lockEPERM,fsckclean.TestIntegrationWorktreeLandlockCombinedWhy
The PR shipped a darwin-only integration test and unit tests covering
AccessReadWrite/AccessRead/AccessNone. The review found:AccessWriteuntested — the worktree path broadens it to read+write, but no test covered this.Verification
All tests pass on Linux (bwrap 0.11.0, git 2.53.0, Landlock ABI 6):
Build and vet clean. No regressions in the existing suite.
Review findings from #31 still outstanding
Notes for the PR #31 author, not fixed here (out of scope for a test-only PR):
deny: hooks/but code makes hooks read-only (readable, not writable, not hard-denied). The code is better than described — update the description.denyAdds/worktreeDenyingitWorktreeGrantsis alwaysnil; the third return value and theprotected = append(protected, worktreeDeny...)line can be removed.