fix(sandbox): refcount temporary roots so one holder's cleanup cannot revoke another's

internal/sandbox/scope.go

@@ -19,6 +19,17 @@ type Scope struct {
 	workspaceRoot string
 	readRoots     []string
 	extraRoots    []string
+	// tempReads / tempWrites count how many LIVE temporary grants depend on a
+	// root, so one holder's cleanup cannot revoke another's access.
+	//
+	// Without them a temporary grant was add-then-remove with no notion of who
+	// still needed it: the second caller to ask for a root already present got a
+	// NO-OP undo, and the first caller's cleanup removed the root out from under
+	// it. Two read-only tools in the same parallel batch, both blocked on the
+	// same directory, is exactly that shape — and read-only tools are precisely
+	// the ones the batch runs concurrently.
+	tempReads  map[string]int
+	tempWrites map[string]int
 }
 
 // NewScope builds a scope for workspaceRoot plus the given extra roots. The
@@ -81,15 +92,88 @@ func (s *Scope) Add(path string) (string, error) {
 	}
 	s.mu.Lock()
 	defer s.mu.Unlock()
-	for _, existing := range append([]string{s.workspaceRoot}, s.extraRoots...) {
-		if pathWithinRoot(existing, root) {
+	// COVERED BY WHAT. extraRoots holds temporary write roots alongside
+	// permanent ones, so "something already covers this" was not the question
+	// worth asking: a permanent grant made over a path a temporary holder
+	// happened to cover recorded nothing, and vanished the moment that holder
+	// released. A session-scoped grant outliving the request that prompted it is
+	// the entire difference between Add and AddTemporaryWrite.
+	//
+	// Permanent coverage is looked for FIRST and across every root, because a
+	// path can be covered twice and the temporary cover must not decide the
+	// answer when a permanent one is also present.
+	if s.permanentWriteRootCoversLocked(root) {
+		return root, nil
+	}
+	for _, existing := range s.extraRoots {
+		// The same root, held temporarily: promote it in place. Its holders'
+		// releases become no-ops, which is what permanence means here.
+		if existing == root && s.temporaryWriteLocked(existing) {
+			delete(s.tempWrites, root)
 			return root, nil
 		}
 	}
+	// Either uncovered, or covered only by a BROADER temporary root that is
+	// going to be released. Recording the narrower root in its own right is what
+	// survives that release.
 	s.extraRoots = append(s.extraRoots, root)
 	return root, nil
 }
 
+// temporaryWriteLocked reports whether root is held by a temporary write grant
+// rather than a session-scoped one. Callers must hold the lock.
+func (s *Scope) temporaryWriteLocked(root string) bool {
+	_, temporary := s.tempWrites[root]
+	return temporary
+}
+
+// permanentWriteRootCoversLocked reports whether root sits under write authority
+// that outlives every temporary holder: the workspace root, or a session-scoped
+// grant. Coverage by a TEMPORARY write root deliberately does not count —
+// it is going to be released, so nothing recorded on the strength of it
+// survives. Callers must hold the lock.
+//
+// One helper rather than a copy per caller because Add, AddRead and
+// AddTemporaryRead all turn on the same question, and three copies of "covered,
+// but by what" is how one of them ends up answering it differently.
+// permanentReadRootCoversLocked reports whether root is already readable by a
+// grant that outlives every temporary holder.
+//
+// SLICE ORDER MUST NOT DECIDE THIS. The old check walked readRoots and stopped at
+// the first entry covering the path, so a BROAD TEMPORARY root sitting earlier in
+// the slice shadowed a narrower PERMANENT one later — and the caller was handed a
+// reference on somebody else's grant when it needed no reference at all. Asking
+// the whole question first, over both lists, makes the answer independent of
+// insertion order.
+//
+// Write roots count: a grant that permits writing permits reading the same tree.
+func (s *Scope) permanentReadRootCoversLocked(root string) bool {
+	if s.permanentWriteRootCoversLocked(root) {
+		return true
+	}
+	for _, existing := range s.readRoots {
+		if pathWithinRoot(existing, root) && !s.temporaryReadLocked(existing) {
+			return true
+		}
+	}
+	return false
+}
+
+func (s *Scope) permanentWriteRootCoversLocked(root string) bool {
+	for _, existing := range append([]string{s.workspaceRoot}, s.extraRoots...) {
+		if pathWithinRoot(existing, root) && !s.temporaryWriteLocked(existing) {
+			return true
+		}
+	}
+	return false
+}
+
+// temporaryReadLocked is temporaryWriteLocked for read roots.
+func (s *Scope) temporaryReadLocked(root string) bool {
+	_, temporary := s.tempReads[root]
+	return temporary
+}
+
 // AddRead grants read-only access under path. If the path is already covered by
 // a writable root, no separate read root is stored.
 func (s *Scope) AddRead(path string) (string, error) {
@@ -99,11 +183,19 @@ func (s *Scope) AddRead(path string) (string, error) {
 	}
 	s.mu.Lock()
 	defer s.mu.Unlock()
-	if s.writeRootCoversLocked(root) {
+	// Same rule as Add: coverage that is going to be released is not coverage a
+	// permanent grant can rely on, whichever side of the read/write boundary it
+	// sits on. A permanent read covered only by a temporary WRITE root died with
+	// that root just as surely.
+	if s.permanentWriteRootCoversLocked(root) {
+		return root, nil
+	}
+	if s.permanentReadRootCoversLocked(root) {
 		return root, nil
 	}
 	for _, existing := range s.readRoots {
-		if pathWithinRoot(existing, root) {
+		if existing == root && s.temporaryReadLocked(existing) {
+			delete(s.tempReads, root)
 			return root, nil
 		}
 	}
@@ -118,16 +210,63 @@ func (s *Scope) AddTemporaryRead(path string) (string, func(), error) {
 	}
 	s.mu.Lock()
 	defer s.mu.Unlock()
-	if s.writeRootCoversLocked(root) {
+	// Genuinely permanent — the workspace root, or a session-scoped grant. It
+	// outlives this reader, so there is nothing to release.
+	if s.permanentWriteRootCoversLocked(root) {
 		return root, func() {}, nil
 	}
-	for _, existing := range s.readRoots {
-		if pathWithinRoot(existing, root) {
-			return root, func() {}, nil
-		}
+	// A TEMPORARY write root covering this path is NOT borrowed, and this is the
+	// one place the distinction bites hardest.
+	//
+	// The dependency on that root is real — the write holder's cleanup must not
+	// revoke this reader, which is the defect the refcount exists to close,
+	// reached across the read/write boundary. But taking a reference on the
+	// WRITE root pays for the lifetime with the authority: the reference keeps
+	// the root in extraRoots, extraRoots is what Roots() feeds validate(), and
+	// validate() is WRITE authorization. So a reader outliving its writer went on
+	// writing anywhere under a write grant that had already ended — not merely
+	// under the path it asked to read.
+	//
+	// One reference cannot be both the lifetime and the capability. The lifetime
+	// is kept below, as a temporary READ root for the path this caller actually
+	// asked for. readRoots feeds ReadRoots(), which confers no write authority —
+	// validate() authorises writes from workspaceRoot plus extraRoots, and this
+	// path never enters that list. So the reader survives its writer with read
+	// authority and only read authority.
+	//
+	// That claim is the written justification for the whole design, so it has to
+	// stay exhaustive as the accessors change. It has already been wrong once, in
+	// both directions: an earlier draft said readRoots fed ReadRoots() and nothing
+	// else while ExtraReadRoots() also read it, and that accessor has since been
+	// removed as belonging to a different change.
+	if s.permanentReadRootCoversLocked(root) {
+		return root, func() {}, nil
 	}
+	if s.tempReads == nil {
+		s.tempReads = map[string]int{}
+	}
+	// THE SAME ROOT, ANOTHER HOLDER: one entry, two references. Only an EXACT
+	// match shares — a narrower request must not, which is the whole point below.
+	if _, held := s.tempReads[root]; held {
+		s.tempReads[root]++
+		return root, oncePerHolder(func() { s.releaseTemporaryRead(root) }), nil
+	}
+	// A NARROWER REQUEST RECORDS ITS OWN ROOT, even under a broader temporary
+	// read that currently covers it.
+	//
+	// Borrowing the covering root's reference kept this reader alive by keeping
+	// the BROAD ROOT alive, and readRoots feeds validateRead and the native
+	// sandbox profile. So once the broad reader released, a caller that had asked
+	// to read one subdirectory was left able to read the whole tree its neighbour
+	// had asked for, siblings included. Reported by @jatmn, and it is the read
+	// twin of the write escalation fixed above: the refcount key decides not only
+	// WHEN an entry disappears but WHICH path stays authorised while it lives.
 	s.readRoots = append(s.readRoots, root)
-	return root, func() { s.removeReadRoot(root) }, nil
+	if s.tempReads == nil {
+		s.tempReads = map[string]int{}
+	}
+	s.tempReads[root] = 1
+	return root, oncePerHolder(func() { s.releaseTemporaryRead(root) }), nil
 }
 
 func (s *Scope) AddTemporaryWrite(path string) (string, func(), error) {
@@ -137,32 +276,107 @@ func (s *Scope) AddTemporaryWrite(path string) (string, func(), error) {
 	}
 	s.mu.Lock()
 	defer s.mu.Unlock()
-	if s.writeRootCoversLocked(root) {
+	// A PERMANENT grant is the only cover that needs no bookkeeping: it outlives
+	// every temporary holder, so there is nothing to keep alive and nothing to
+	// release.
+	if s.permanentWriteRootCoversLocked(root) {
 		return root, func() {}, nil
 	}
+	if s.tempWrites == nil {
+		s.tempWrites = map[string]int{}
+	}
+	// THE SAME ROOT, ANOTHER HOLDER: one entry, two references. Only an exact
+	// match shares an entry — a NARROWER request must not, which is the whole
+	// point below.
+	if _, held := s.tempWrites[root]; held {
+		s.tempWrites[root]++
+		return root, oncePerHolder(func() { s.releaseTemporaryWrite(root) }), nil
+	}
+	// A NARROWER REQUEST RECORDS ITS OWN ROOT, even when a broader temporary
+	// grant currently covers it.
+	//
+	// Borrowing a reference on the covering root kept this holder alive, which
+	// was the point, but it kept the COVERING ROOT alive to do it — and
+	// extraRoots is what validate() authorises writes from. So once the broad
+	// holder released, a caller that had asked to write one subdirectory was
+	// left holding write authority over the whole tree its neighbour had asked
+	// for, including siblings it never named. Reported by CodeRabbit, and it is
+	// the write-side twin of the read-side escalation @jatmn found: one
+	// reference cannot be both a lifetime and a capability, whichever side of
+	// the read/write boundary it sits on.
+	//
+	// Recording the requested root separately gives each holder exactly the
+	// authority it asked for and a lifetime of its own. The nesting costs an
+	// extra entry in extraRoots while both are live, which is redundant but not
+	// wrong — the broader root already permits everything the narrower one does.
 	s.extraRoots = append(s.extraRoots, root)
-	return root, func() { s.removeWriteRoot(root) }, nil
+	s.tempWrites[root] = 1
+	return root, oncePerHolder(func() { s.releaseTemporaryWrite(root) }), nil
 }
 
-func (s *Scope) writeRootCoversLocked(root string) bool {
-	for _, existing := range append([]string{s.workspaceRoot}, s.extraRoots...) {
-		if pathWithinRoot(existing, root) {
-			return true
-		}
-	}
-	return false
-}
-
-func (s *Scope) removeReadRoot(root string) {
+// releaseTemporaryRead drops ONE holder's reference and removes the root only
+// when the last one is gone.
+//
+// The count alone cannot make this safe, and an earlier comment here claimed it
+// could — that each undo is called exactly once, and flooring at zero handled
+// the rest. Flooring stops the count going negative; it does not stop one
+// holder's second call consuming a DIFFERENT holder's reference. With two
+// readers, calling the first's undo twice took the count 2 -> 1 -> 0 and removed
+// a root the second was still using. Idempotency belongs to the closure, which
+// is the thing that knows whose reference it is, so every undo handed out is
+// wrapped in oncePerHolder.
+func (s *Scope) releaseTemporaryRead(root string) {
 	s.mu.Lock()
-	defer s.mu.Unlock()
+	remaining, tracked := s.tempReads[root]
+	if !tracked {
+		s.mu.Unlock()
+		return
+	}
+	remaining--
+	if remaining > 0 {
+		s.tempReads[root] = remaining
+		s.mu.Unlock()
+		return
+	}
+	// Both mutations under ONE hold. Dropping the lock between them opened a
+	// window where the root was still in readRoots but no longer in tempReads:
+	// a concurrent AddTemporaryRead landing there reads it as a PERMANENT root,
+	// hands its caller a no-op undo, and then this call strips the root — so
+	// that caller believes it holds access it has already silently lost.
+	delete(s.tempReads, root)
 	s.readRoots = removeScopeRoot(s.readRoots, root)
+	s.mu.Unlock()
 }
 
-func (s *Scope) removeWriteRoot(root string) {
+// releaseTemporaryWrite is releaseTemporaryRead for write roots. Two functions
+// rather than one generic helper because they guard different slices and the
+// generic version would take the slice by name — which is how the wrong one
+// gets passed.
+func (s *Scope) releaseTemporaryWrite(root string) {
 	s.mu.Lock()
-	defer s.mu.Unlock()
+	remaining, tracked := s.tempWrites[root]
+	if !tracked {
+		s.mu.Unlock()
+		return
+	}
+	remaining--
+	if remaining > 0 {
+		s.tempWrites[root] = remaining
+		s.mu.Unlock()
+		return
+	}
+	// Same single-hold rule as releaseTemporaryRead above.
+	delete(s.tempWrites, root)
 	s.extraRoots = removeScopeRoot(s.extraRoots, root)
+	s.mu.Unlock()
+}
+
+// oncePerHolder makes one holder's undo safe to call more than once. The
+// reference it drops is that holder's own, so a second call must do nothing
+// rather than reach into the shared count and take somebody else's.
+func oncePerHolder(release func()) func() {
+	var once sync.Once
+	return func() { once.Do(release) }
 }
 
 func removeScopeRoot(roots []string, root string) []string {

internal/sandbox/scope_extra_read_test.go

@@ -0,0 +1,56 @@
+package sandbox
+
+import (
+	"path/filepath"
+	"testing"
+)
+
+// THE ANTI-ESCALATION PROPERTY. A read grant (AddRead, where --add-read-dir lands)
+// is READABLE but NOT WRITABLE. This is what makes propagating a parent's
+// request_permissions read grant to a child safe: the child can audit the granted
+// path but cannot modify it. Emitting the grant as a WRITE root (--add-dir) would
+// break exactly this.
+func TestAReadGrantIsReadableButNotWritable(t *testing.T) {
+	scope, granted := grantOutsideDefaults(t)
+	root, err := scope.AddRead(granted)
+	if err != nil {
+		t.Fatalf("AddRead: %v", err)
+	}
+	target := filepath.Join(root, "audit-me.go")
+
+	if block := scope.validateRead(target); block != nil {
+		t.Fatalf("a read grant did not allow READING %q: %v — the audit would fail 'outside the workspace'", target, block)
+	}
+	if block := scope.validate(target); block == nil {
+		t.Fatalf("a read grant ALLOWED WRITING %q — a read grant escalated to write", target)
+	}
+}
+
+// grantOutsideDefaults returns a scope and a directory that scope does not
+// already cover, both under test-owned storage.
+//
+// THE CAUSE, NOT THE SYMPTOM. NewScope seeds the system temporary directory as a
+// permanent write root, so a plain t.TempDir() grant is writable before any
+// grant exists and the anti-escalation assertions below prove nothing. The first
+// fix placed the fixture under os.UserHomeDir() to find ground the defaults do
+// not own. That worked and bought two problems: a hermetic runner may expose
+// HOME read-only, so these tests failed at MkdirTemp before asserting anything,
+// and on an ordinary machine they wrote into the developer's real home and left
+// zero-scope-* debris whenever a run was interrupted. Reported by @jatmn.
+//
+// Building the Scope directly removes the seeded defaults, which is what made
+// HOME necessary in the first place. t.TempDir() is then outside by
+// construction, the fixture is hermetic, and nothing touches HOME.
+func grantOutsideDefaults(t *testing.T) (*Scope, string) {
+	t.Helper()
+	workspace := resolvedFixturePath(t, t.TempDir())
+	granted := resolvedFixturePath(t, t.TempDir())
+	scope := &Scope{workspaceRoot: workspace}
+	// PROVED AGAINST THIS SCOPE. The point of the helper is that the grant is
+	// what changes access, so the target has to begin unauthorised — asserted
+	// here once rather than trusted in every caller.
+	if scope.validate(filepath.Join(granted, "probe.txt")) == nil {
+		t.Fatalf("%s is writable before any grant exists; the assertions below would prove nothing", granted)
+	}
+	return scope, granted
+}