Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 24 additions & 4 deletions internal/cli/agent.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ type agentSandboxLaunch struct {
// empty anchor would silently drop every home-anchored credential deny while
// still reporting the run as sandboxed (post-merge review, PR #107).
// `--sandbox off` is the explicit escape hatch.
func agentSandboxSpec(worktreeDir, perRunTmp, logDir string, launch agentSandboxLaunch, runID string) (platform.SandboxSpec, error) {
func agentSandboxSpec(worktreeDir, perRunTmp, logDir string, gitDirs []string, launch agentSandboxLaunch, runID string) (platform.SandboxSpec, error) {
userHome, err := os.UserHomeDir()
if err != nil {
return platform.SandboxSpec{}, fmt.Errorf("resolve user home for sandbox credential denies (use --sandbox off to run unconfined): %w", err)
Expand All @@ -74,6 +74,7 @@ func agentSandboxSpec(worktreeDir, perRunTmp, logDir string, launch agentSandbox
ViolationTag: sandboxViolationTag(runID),
ReadConfine: launch.readConfine,
ReadAllowExtra: launch.readAllow,
GitDirs: gitDirs,
}, nil
}

Expand Down Expand Up @@ -352,7 +353,23 @@ func newAgentRunCommand(stdout io.Writer, opts *options) *cobra.Command {
return err
}
runStart := time.Now()
commandErr := runAgentProcess(cmd.Context(), wt, run, agentCommand, stdout, sandboxLaunch)
// Resolve the worktree's git storage dirs here (we hold *options),
// only when the sandbox is active — otherwise the grant is unused
// and the two rev-parse forks are wasted. Best-effort: a resolution
// failure leaves the grant empty rather than blocking the run
// (P7-SANDBOX-01).
var sandboxGitDirs []string
if sandboxLaunch.enabled {
sandboxGitDirs, _ = gitRunner(opts).WorktreeSandboxWriteDirs(cmd.Context(), wt.Path)
if len(sandboxGitDirs) == 0 {
// A real fresh worktree always resolves; an empty grant here
// means the resolver hit an unexpected git error, and the
// agent's `git commit` would be EPERM'd inside the sandbox.
// Warn rather than fail silently (review: P7-SANDBOX-01).
_, _ = fmt.Fprintln(cmd.ErrOrStderr(), "warning: could not resolve the worktree's git dirs for the sandbox; git commits inside the run may be blocked (use --sandbox off if the agent must commit)")
}
}
commandErr := runAgentProcess(cmd.Context(), wt, run, agentCommand, stdout, sandboxLaunch, sandboxGitDirs)
collectSandboxViolations(cmd.Context(), cmd.ErrOrStderr(), store, run, sandboxLaunch, runStart)
diffSummary := agentDiffSummary(cmd.Context(), wt.Path, wt.BaseSHA)
status := "complete"
Expand Down Expand Up @@ -766,7 +783,7 @@ func pathWithin(root, path string) bool {
return rel == "." || (rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator)))
}

func runAgentProcess(ctx context.Context, wt state.Worktree, run state.AgentRun, args []string, stdout io.Writer, sandboxLaunch agentSandboxLaunch) error {
func runAgentProcess(ctx context.Context, wt state.Worktree, run state.AgentRun, args []string, stdout io.Writer, sandboxLaunch agentSandboxLaunch, gitDirs []string) error {
if err := os.MkdirAll(filepath.Dir(run.LogPath), 0o700); err != nil {
return fmt.Errorf("create agent log dir: %w", err)
}
Expand All @@ -793,7 +810,10 @@ func runAgentProcess(ctx context.Context, wt state.Worktree, run state.AgentRun,
}
defer func() { _ = os.RemoveAll(perRunTmp) }()
envOverrides["TMPDIR"] = perRunTmp
spec, err := agentSandboxSpec(wt.Path, perRunTmp, filepath.Dir(run.LogPath), sandboxLaunch, run.ID)
// gitDirs (resolved by the caller, which holds *options) grants the
// linked worktree's git storage dirs so the agent's `git add`/`commit`
// are not EPERM'd by the sandbox (P7-SANDBOX-01).
spec, err := agentSandboxSpec(wt.Path, perRunTmp, filepath.Dir(run.LogPath), gitDirs, sandboxLaunch, run.ID)
if err != nil {
return err
}
Expand Down
4 changes: 2 additions & 2 deletions internal/cli/agent_sandbox_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -98,15 +98,15 @@ func (p passthroughSandbox) Command(_ context.Context, spec platform.SandboxSpec
// silently vanished.
func TestAgentSandboxSpecFailsClosedWithoutUserHome(t *testing.T) {
t.Setenv("HOME", "")
if _, err := agentSandboxSpec("/wt", "/tmp/run", "/log", agentSandboxLaunch{devstrapHome: "/dsh"}, "arun_test"); err == nil {
if _, err := agentSandboxSpec("/wt", "/tmp/run", "/log", nil, agentSandboxLaunch{devstrapHome: "/dsh"}, "arun_test"); err == nil {
t.Fatal("agentSandboxSpec succeeded without a resolvable user home; want fail-closed error")
}
}

func TestAgentSandboxSpecAnchorsRealUserHome(t *testing.T) {
home := t.TempDir()
t.Setenv("HOME", home)
spec, err := agentSandboxSpec("/wt", "/tmp/run", "/log", agentSandboxLaunch{devstrapHome: "/dsh", denyNetwork: true}, "arun_test")
spec, err := agentSandboxSpec("/wt", "/tmp/run", "/log", nil, agentSandboxLaunch{devstrapHome: "/dsh", denyNetwork: true}, "arun_test")
if err != nil {
t.Fatalf("agentSandboxSpec: %v", err)
}
Expand Down
69 changes: 69 additions & 0 deletions internal/git/git.go
Original file line number Diff line number Diff line change
Expand Up @@ -545,6 +545,75 @@ func (r Runner) IsSquashMerged(ctx context.Context, dir, branch, baseRef string)
return strings.TrimSpace(merged) == strings.TrimSpace(baseTree), nil
}

// WorktreeSandboxWriteDirs returns the absolute git storage paths a linked
// worktree must be able to WRITE for `git add`/`git commit` to succeed under an
// OS sandbox: the shared object store, refs, and reflogs in the git-common-dir,
// plus the per-worktree admin dir (index/HEAD/COMMIT_EDITMSG/logs). It
// deliberately EXCLUDES the common dir itself (and thus hooks/ and config) —
// granting those would let a sandboxed agent plant a hook or config that
// executes UNSANDBOXED on a later git operation (P7-SANDBOX-01). Paths are
// symlink-resolved. A nil slice with no error is returned when dir is not inside
// a git worktree, so callers can grant nothing without special-casing.
func (r Runner) WorktreeSandboxWriteDirs(ctx context.Context, dir string) ([]string, error) {
common, err := r.Run(ctx, dir, "rev-parse", "--git-common-dir")
if err != nil {
return nil, nil // not a git worktree — nothing to grant
}
gitDir, err := r.Run(ctx, dir, "rev-parse", "--git-dir")
if err != nil {
return nil, nil
}
abs := func(p string) string {
p = strings.TrimSpace(p)
if p == "" {
return ""
}
if !filepath.IsAbs(p) {
p = filepath.Join(dir, p)
}
if resolved, rerr := filepath.EvalSymlinks(p); rerr == nil {
return resolved
}
return filepath.Clean(p)
}
commonAbs := abs(common)
gitDirAbs := abs(gitDir)
if commonAbs == "" || gitDirAbs == "" {
return nil, nil
}
// Resolve each storage subpath too: <common>/objects can itself be a
// symlink (git alternates / a relocated object store), and Seatbelt matches
// the kernel-real path — so grant the resolved target, not the alias.
// EvalSymlinks fails on a not-yet-created path (e.g. no reflog); fall back
// to the clean join, which git creates under the already-resolved commonAbs.
evalJoin := func(elem string) string {
p := filepath.Join(commonAbs, elem)
if resolved, rerr := filepath.EvalSymlinks(p); rerr == nil {
return resolved
}
return p
}
dirs := []string{
evalJoin("objects"),
evalJoin("refs"),
evalJoin("logs"),
}
// Add the per-worktree admin dir only for a genuine LINKED worktree, and
// only when it is actually under <common>/worktrees/. gitDir == common is
// the main checkout (objects/refs/logs already cover its writes; never add
// the whole common dir — hooks/ and config live there). A --git-dir that
// resolves ELSEWHERE under the common dir (a malformed gitfile/commondir
// could point at <common>/hooks or a sibling) is refused rather than
// granted, keeping the hooks/config exclusion robust against hostile setups.
if gitDirAbs != commonAbs {
worktreesRoot := filepath.Join(commonAbs, "worktrees")
if rel, rerr := filepath.Rel(worktreesRoot, gitDirAbs); rerr == nil && rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator)) {
dirs = append(dirs, gitDirAbs)
}
}
return dirs, nil
}

func (r Runner) WorktreeAdd(ctx context.Context, dir, path, branch, base string) error {
if !safeBranchName(branch) {
return fmt.Errorf("invalid git branch name %q", branch)
Expand Down
98 changes: 98 additions & 0 deletions internal/git/sandbox_writedirs_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
package git

import (
"context"
"os"
"os/exec"
"path/filepath"
"strings"
"testing"
"time"
)

// TestWorktreeSandboxWriteDirs proves the resolver grants exactly the git
// storage a linked worktree writes for add/commit — objects, refs, logs, and
// the per-worktree admin dir — and NEVER the common dir itself or its
// hooks/config (which would be a sandbox escape, P7-SANDBOX-01).
func TestWorktreeSandboxWriteDirs(t *testing.T) {
repo, r := initSquashMergeRepo(t) // real local repo on branch main with one commit
ctx := context.Background()

wt := filepath.Join(t.TempDir(), "wt")
if err := r.WorktreeAdd(ctx, repo, wt, "agent/x", "main"); err != nil {
t.Fatalf("WorktreeAdd: %v", err)
}

dirs, err := r.WorktreeSandboxWriteDirs(ctx, wt)
if err != nil {
t.Fatalf("WorktreeSandboxWriteDirs: %v", err)
}
if len(dirs) != 4 {
t.Fatalf("want 4 grant dirs (objects/refs/logs + per-worktree admin), got %d: %v", len(dirs), dirs)
}

commonReal := mustEval(t, filepath.Join(repo, ".git"))
var bases []string
sawWorktreeAdmin := false
for _, d := range dirs {
// Security invariants: never the common dir root, never hooks/, never config.
if d == commonReal {
t.Errorf("grant includes the common dir root %q — would expose hooks/config (sandbox escape)", d)
}
if strings.Contains(d, string(os.PathSeparator)+"hooks") || strings.HasSuffix(d, string(os.PathSeparator)+"config") {
t.Errorf("grant includes a hooks/config path %q", d)
}
bases = append(bases, filepath.Base(d))
if strings.Contains(d, string(os.PathSeparator)+"worktrees"+string(os.PathSeparator)) {
sawWorktreeAdmin = true
}
}
for _, want := range []string{"objects", "refs", "logs"} {
if !contains(bases, want) {
t.Errorf("missing grant for %s; bases=%v", want, bases)
}
}
if !sawWorktreeAdmin {
t.Errorf("missing the per-worktree admin dir (…/worktrees/<name>); dirs=%v", dirs)
}
}

// TestWorktreeSandboxWriteDirsNonRepo returns (nil, nil) outside a git worktree
// so the caller grants nothing without special-casing.
func TestWorktreeSandboxWriteDirsNonRepo(t *testing.T) {
r := Runner{Bin: gitBinOrSkip(t), Timeout: 5 * time.Second}
dirs, err := r.WorktreeSandboxWriteDirs(context.Background(), t.TempDir())
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if dirs != nil {
t.Fatalf("want nil grant outside a git worktree, got %v", dirs)
}
}

func mustEval(t *testing.T, p string) string {
t.Helper()
r, err := filepath.EvalSymlinks(p)
if err != nil {
return filepath.Clean(p)
}
return r
}

func contains(s []string, v string) bool {
for _, x := range s {
if x == v {
return true
}
}
return false
}

func gitBinOrSkip(t *testing.T) string {
t.Helper()
bin, err := exec.LookPath("git")
if err != nil {
t.Skip("git not installed")
}
return bin
}
Comment on lines +73 to +98

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
rg -n --type=go '^func (contains|mustEval|gitBinOrSkip)\(' internal/git

Repository: Reederey87/DevStrap

Length of output: 414


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== package files =="
git ls-files internal/git/*.go

echo
echo "== helper name search across internal/git =="
rg -n --type=go '^func (contains|mustEval|gitBinOrSkip)\(' internal/git

echo
echo "== tests around sandbox_writedirs =="
ast-grep outline internal/git/sandbox_writedirs_test.go --view expanded || true

echo
echo "== implementation branch references =="
rg -n 'gitDirAbs == commonAbs|commonAbs|linked-worktree|sandbox_writedirs' internal/git

Repository: Reederey87/DevStrap

Length of output: 1353


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== internal/git/sandbox_writedirs_test.go (relevant sections) =="
sed -n '1,170p' internal/git/sandbox_writedirs_test.go

echo
echo "== internal/git/git.go (relevant section) =="
sed -n '560,630p' internal/git/git.go

Repository: Reederey87/DevStrap

Length of output: 5566


Cover the plain-checkout branch too Add a case for the gitDirAbs == commonAbs path in a regular checkout; the current tests only cover linked worktrees and non-repos.

🤖 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/git/sandbox_writedirs_test.go` around lines 73 - 98, Add test
coverage for the plain-checkout branch where gitDirAbs equals commonAbs in a
normal repository checkout; the current `mustEval`, `contains`, and
`gitBinOrSkip` helpers are fine, but the sandbox write-dir tests need a new case
that exercises the regular checkout path in addition to the existing linked
worktree and non-repo scenarios. Update `internal/git/sandbox_writedirs_test.go`
to set up or use a standard checkout and assert the `gitDirAbs == commonAbs`
branch is reached.

11 changes: 11 additions & 0 deletions internal/platform/sandbox.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,17 @@ type SandboxSpec struct {
// ReadAllowExtra are additional absolute read roots (repeatable
// --read-allow) folded into the read-confinement allow-list.
ReadAllowExtra []string
// GitDirs are absolute git storage paths a linked agent worktree must be
// able to WRITE for `git add`/`git commit` to succeed: the shared object
// store, refs, and reflogs in the clone's git-common-dir, plus the
// per-worktree admin dir (index/HEAD/COMMIT_EDITMSG). A DevStrap agent
// worktree is a git *linked* worktree whose .git lives in the parent clone,
// so without these the child's commit is EPERM'd and `agent pr` has nothing
// to push (P7-SANDBOX-01). The common dir's hooks/ and config are
// deliberately EXCLUDED — granting them would let the child plant a hook or
// config that executes UNSANDBOXED on a later git operation. Also folded
// into the read-confinement allow-list so git reads work under --read-confine.
GitDirs []string
}

// Sandbox wraps an agent argv in an OS-enforced confinement (AGEN-03 /
Expand Down
8 changes: 8 additions & 0 deletions internal/platform/sandbox_bwrap_args.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,14 @@ func bwrapArgs(spec SandboxSpec, maskDirs, maskFiles []string, opts bwrapOptions
if spec.TmpDir != "" {
args = append(args, "--bind", spec.TmpDir, spec.TmpDir)
}
// The linked worktree's git storage dirs (objects/refs/logs/per-worktree
// admin) so `git commit` writes succeed — NOT the common dir's hooks/config
// (P7-SANDBOX-01). --bind-try tolerates an absent dir (e.g. no reflog).
for _, dir := range spec.GitDirs {
if dir != "" {
args = append(args, "--bind-try", dir, dir)
}
}
// Mount ops are processed sequentially and later mounts override earlier
// ones, so credential masks MUST come after the read-write binds. Under
// read confinement the credential paths are already outside the exposed
Expand Down
65 changes: 65 additions & 0 deletions internal/platform/sandbox_gitdirs_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
package platform

import (
"slices"
"strings"
"testing"
)

// gitStorageDirs are the kind of paths WorktreeSandboxWriteDirs yields: the
// linked worktree's git storage, never the common dir root or hooks/config.
var gitStorageDirs = []string{
"/home/dev/clone/.git/objects",
"/home/dev/clone/.git/refs",
"/home/dev/clone/.git/logs",
"/home/dev/clone/.git/worktrees/agent-x",
}

// TestSBPLProfileGrantsGitDirs proves the Seatbelt write allow-list includes
// each git storage dir so an agent `git commit` in a linked worktree is not
// EPERM'd (P7-SANDBOX-01) — the platform where the default-on sandbox bites.
func TestSBPLProfileGrantsGitDirs(t *testing.T) {
spec := SandboxSpec{WorktreeDir: "/wt", TmpDir: "/tmp", GitDirs: gitStorageDirs}
profile := sbplProfile(spec, nil, nil)
for _, d := range gitStorageDirs {
if !strings.Contains(profile, `(subpath "`+d+`")`) {
t.Errorf("SBPL write allow-list missing git dir %q\n%s", d, profile)
}
}
// The common dir root must never be granted wholesale (hooks/config escape).
if strings.Contains(profile, `(subpath "/home/dev/clone/.git")`) {
t.Error("SBPL grants the whole common dir — hooks/config sandbox escape")
}
}

// TestBwrapArgsGrantsGitDirs proves the bubblewrap write binds include each git
// storage dir (via --bind-try, tolerating an absent reflog).
func TestBwrapArgsGrantsGitDirs(t *testing.T) {
spec := SandboxSpec{WorktreeDir: "/wt", TmpDir: "/tmp", GitDirs: gitStorageDirs}
args := bwrapArgs(spec, nil, nil, bwrapOptions{})
for _, d := range gitStorageDirs {
if indexSequence(args, "--bind-try", d, d) == -1 {
t.Errorf("bwrap args missing --bind-try %s %s\n%v", d, d, args)
}
}
if slices.Contains(args, "/home/dev/clone/.git") {
t.Error("bwrap binds the whole common dir — hooks/config sandbox escape")
}
}

// TestReadConfineRootsIncludesGitDirs proves git storage stays readable under
// --read-confine (readonly policy) so git read ops work; the RW grant already
// makes them writable elsewhere.
func TestReadConfineRootsIncludesGitDirs(t *testing.T) {
roots := readConfineRoots(SandboxSpec{
WorktreeDir: "/wt",
TmpDir: "/tmp",
ReadConfine: true,
GitDirs: gitStorageDirs,
})
for _, d := range gitStorageDirs {
if !slices.Contains(roots, d) {
t.Errorf("read-confine roots missing git dir %q: %v", d, roots)
}
}
}
14 changes: 14 additions & 0 deletions internal/platform/sandbox_landlock.go
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,20 @@ func applyLandlockPolicy(spec SandboxSpec) error {
// such renames inside the worktree.
rules = append(rules, landlock.RWDirs(rw...).WithRefer())
}
// The linked worktree's git storage dirs (objects/refs/logs/per-worktree
// admin) so `git commit` succeeds — NOT the common dir's hooks/config
// (P7-SANDBOX-01). Separate rule so a rare absent dir (no reflog yet) is
// skipped rather than failing the whole ruleset; WithRefer for git's
// tmp-object -> objects/xx rename promotion.
var gitRW []string
for _, dir := range spec.GitDirs {
if dir != "" {
gitRW = append(gitRW, dir)
}
}
if len(gitRW) > 0 {
rules = append(rules, landlock.RWDirs(gitRW...).WithRefer().IgnoreIfMissing())
}
// Shell plumbing (`> /dev/null`) and pty allocation; IgnoreIfMissing
// because minimal containers lack some nodes. Wider than bwrap's fresh
// --dev (these nodes are machine-shared), named in landlockLimitations.
Expand Down
5 changes: 4 additions & 1 deletion internal/platform/sandbox_profile.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,10 @@ func sbplProfile(spec SandboxSpec, denyReadDirs, denyReadFiles []string) string
b.WriteString(")\n")
}
b.WriteString("(allow file-write*\n")
for _, dir := range []string{spec.WorktreeDir, spec.TmpDir} {
// WorktreeDir + TmpDir, plus the linked worktree's git storage dirs
// (objects/refs/logs/per-worktree-admin) so `git commit` works — but NOT
// the common dir's hooks/config (P7-SANDBOX-01).
for _, dir := range append([]string{spec.WorktreeDir, spec.TmpDir}, spec.GitDirs...) {
if dir == "" {
continue
}
Expand Down
Loading
Loading