diff --git a/.gitattributes b/.gitattributes new file mode 100644 index 00000000..a6231632 --- /dev/null +++ b/.gitattributes @@ -0,0 +1,3 @@ +*.go text eol=lf +*.mod text eol=lf +*.sum text eol=lf diff --git a/internal/sandbox/grants.go b/internal/sandbox/grants.go index 46b4ca8f..a54eb917 100644 --- a/internal/sandbox/grants.go +++ b/internal/sandbox/grants.go @@ -16,7 +16,11 @@ import ( "github.com/Gitlawb/zero/internal/redaction" ) -const grantSchemaVersion = 3 +const ( + grantSchemaVersion = 3 + grantLockTimeout = 5 * time.Second + grantLockRetry = 10 * time.Millisecond +) type Grant struct { ToolName string `json:"toolName"` @@ -139,7 +143,16 @@ func (store *GrantStore) ConsumeMigrationNotice() (string, error) { } store.mu.Lock() defer store.mu.Unlock() - state, err := store.readState() + // Clearing the pending flag is a read-modify-write like any other mutator, so + // it takes the same interprocess lock: two frontends starting at once must not + // both read NoticePending, both print the notice, and clobber each other's + // write of the rest of the state. + unlock, err := store.lockStateFile() + if err != nil { + return "", err + } + defer unlock() + state, err := store.readStateLocked() if err != nil { return "", err } @@ -161,7 +174,13 @@ func (store *GrantStore) Grant(input GrantInput) (Grant, error) { } store.mu.Lock() defer store.mu.Unlock() - state, err := store.readState() + unlock, err := store.lockStateFile() + if err != nil { + return Grant{}, err + } + defer unlock() + + state, err := store.readStateLocked() if err != nil { return Grant{}, err } @@ -193,7 +212,13 @@ func (store *GrantStore) GrantCommandPrefix(input CommandPrefixInput) (CommandPr } store.mu.Lock() defer store.mu.Unlock() - state, err := store.readState() + unlock, err := store.lockStateFile() + if err != nil { + return CommandPrefixGrant{}, err + } + defer unlock() + + state, err := store.readStateLocked() if err != nil { return CommandPrefixGrant{}, err } @@ -337,7 +362,13 @@ func (store *GrantStore) Revoke(toolName string) (int, error) { } store.mu.Lock() defer store.mu.Unlock() - state, err := store.readState() + unlock, err := store.lockStateFile() + if err != nil { + return 0, err + } + defer unlock() + + state, err := store.readStateLocked() if err != nil { return 0, err } @@ -372,7 +403,13 @@ func (store *GrantStore) RevokePath(toolName string, scopePath string) (int, err } store.mu.Lock() defer store.mu.Unlock() - state, err := store.readState() + unlock, err := store.lockStateFile() + if err != nil { + return 0, err + } + defer unlock() + + state, err := store.readStateLocked() if err != nil { return 0, err } @@ -407,7 +444,13 @@ func (store *GrantStore) RevokePath(toolName string, scopePath string) (int, err func (store *GrantStore) Clear() (int, error) { store.mu.Lock() defer store.mu.Unlock() - state, err := store.readState() + unlock, err := store.lockStateFile() + if err != nil { + return 0, err + } + defer unlock() + + state, err := store.readStateLocked() if err != nil { return 0, err } @@ -537,7 +580,52 @@ func reconcileScope(scope string, kind ScopeKind) (string, ScopeKind) { return scope, kind } +// readState reads the grant file for a caller that does NOT hold the +// interprocess lock (the lookup/list paths). A schema or policy migration has to +// write — a backup plus the migrated state — so it is performed under the lock, +// and the read is retaken there because another process may have migrated the +// file while this one waited. func (store *GrantStore) readState() (grantFile, error) { + state, needsMigration, err := store.readStateFile(false) + if err != nil || !needsMigration { + return state, err + } + unlock, err := store.lockStateFile() + if err != nil { + return grantFile{}, err + } + defer unlock() + return store.readStateLocked() +} + +// readStateLocked reads the grant file for a caller that already holds the +// interprocess lock, so a migration writes through directly instead of +// re-acquiring it (which would deadlock against the caller's own lock). +func (store *GrantStore) readStateLocked() (grantFile, error) { + state, _, err := store.readStateFile(true) + return state, err +} + +// readStateFile decodes the grant file. needsMigration reports that the state on +// disk must be rewritten (legacy schema or a changed policy version) and that the +// caller has to retake this read while holding the interprocess lock; the +// returned state is meaningless in that case. +func (store *GrantStore) readStateFile(locked bool) (grantFile, bool, error) { + state, err := store.decodeState(locked) + if err != nil { + if errors.Is(err, errGrantMigrationNeedsLock) { + return grantFile{}, true, nil + } + return grantFile{}, false, err + } + return state, false, nil +} + +// errGrantMigrationNeedsLock is the internal signal from decodeState that the +// file needs a migration write, which only a lock holder may perform. +var errGrantMigrationNeedsLock = errors.New("sandbox grants file needs a migration write") + +func (store *GrantStore) decodeState(locked bool) (grantFile, error) { data, err := os.ReadFile(store.filePath) if err != nil { if errors.Is(err, os.ErrNotExist) { @@ -559,12 +647,18 @@ func (store *GrantStore) readState() (grantFile, error) { return grantFile{}, store.invalidGrantFile(err) } if head.SchemaVersion == 1 || head.SchemaVersion == 2 { + if !locked { + return grantFile{}, errGrantMigrationNeedsLock + } return store.migrateLegacyState(data, head.SchemaVersion, head.Grants, head.CommandPrefixes) } if head.SchemaVersion != grantSchemaVersion { return grantFile{}, fmt.Errorf("invalid sandbox grants file at %s: unsupported schemaVersion", store.filePath) } if head.PolicyVersion != execution.PolicyVersion { + if !locked { + return grantFile{}, errGrantMigrationNeedsLock + } return store.migrateChangedPolicy(data, head.PolicyVersion, head.Grants, head.CommandPrefixes) } buckets := map[string][]Grant{} @@ -800,6 +894,36 @@ func (store *GrantStore) writeState(state grantFile) error { return nil } +func (store *GrantStore) lockStateFile() (func(), error) { + lockPath := store.filePath + ".lockfile" + if err := os.MkdirAll(filepath.Dir(store.filePath), 0o700); err != nil { + return nil, err + } + file, err := os.OpenFile(lockPath, os.O_CREATE|os.O_RDWR, 0o600) + if err != nil { + return nil, err + } + deadline := time.Now().Add(grantLockTimeout) + for { + locked, err := tryLockGrantFile(file) + if err != nil { + _ = file.Close() + return nil, err + } + if locked { + return func() { + _ = unlockGrantFile(file) + _ = file.Close() + }, nil + } + if time.Now().After(deadline) { + _ = file.Close() + return nil, fmt.Errorf("timed out waiting for sandbox grants lock at %s", lockPath) + } + time.Sleep(grantLockRetry) + } +} + func normalizeStoredGrant(name string, grant Grant) (Grant, error) { if strings.TrimSpace(grant.ToolName) == "" { grant.ToolName = name diff --git a/internal/sandbox/grants_lock_unix.go b/internal/sandbox/grants_lock_unix.go new file mode 100644 index 00000000..ac36b674 --- /dev/null +++ b/internal/sandbox/grants_lock_unix.go @@ -0,0 +1,24 @@ +//go:build !windows + +package sandbox + +import ( + "errors" + "os" + "syscall" +) + +func tryLockGrantFile(file *os.File) (bool, error) { + err := syscall.Flock(int(file.Fd()), syscall.LOCK_EX|syscall.LOCK_NB) + if err == nil { + return true, nil + } + if errors.Is(err, syscall.EWOULDBLOCK) || errors.Is(err, syscall.EAGAIN) { + return false, nil + } + return false, err +} + +func unlockGrantFile(file *os.File) error { + return syscall.Flock(int(file.Fd()), syscall.LOCK_UN) +} diff --git a/internal/sandbox/grants_lock_windows.go b/internal/sandbox/grants_lock_windows.go new file mode 100644 index 00000000..fa56261d --- /dev/null +++ b/internal/sandbox/grants_lock_windows.go @@ -0,0 +1,60 @@ +//go:build windows + +package sandbox + +import ( + "errors" + "os" + "syscall" + "unsafe" +) + +const ( + grantLockfileFailImmediately = 0x00000001 + grantLockfileExclusiveLock = 0x00000002 + grantErrorLockViolation = syscall.Errno(33) + grantErrorSharingViolation = syscall.Errno(32) +) + +var ( + grantKernel32 = syscall.NewLazyDLL("kernel32.dll") + grantProcLockFileEx = grantKernel32.NewProc("LockFileEx") + grantProcUnlockFileEx = grantKernel32.NewProc("UnlockFileEx") +) + +func tryLockGrantFile(file *os.File) (bool, error) { + var overlapped syscall.Overlapped + result, _, err := grantProcLockFileEx.Call( + file.Fd(), + uintptr(grantLockfileExclusiveLock|grantLockfileFailImmediately), + 0, + 1, + 0, + uintptr(unsafe.Pointer(&overlapped)), + ) + if result != 0 { + return true, nil + } + if errors.Is(err, grantErrorLockViolation) || errors.Is(err, grantErrorSharingViolation) { + return false, nil + } + if err == syscall.Errno(0) { + return false, nil + } + return false, err +} + +func unlockGrantFile(file *os.File) error { + var overlapped syscall.Overlapped + result, _, err := grantProcUnlockFileEx.Call( + file.Fd(), + 0, + 1, + 0, + uintptr(unsafe.Pointer(&overlapped)), + ) + if result != 0 || err == syscall.Errno(0) { + return nil + } + return err +} diff --git a/internal/sandbox/grants_test.go b/internal/sandbox/grants_test.go index 271ede30..2bdfba1f 100644 --- a/internal/sandbox/grants_test.go +++ b/internal/sandbox/grants_test.go @@ -1,10 +1,12 @@ package sandbox import ( + "errors" "os" "path/filepath" "strings" "testing" + "time" ) func TestGrantStorePersistsListsRevokesAndClears(t *testing.T) { @@ -70,6 +72,166 @@ func TestGrantStorePersistsListsRevokesAndClears(t *testing.T) { } } +func TestGrantStoreSerializesWritesAcrossStores(t *testing.T) { + path := filepath.Join(t.TempDir(), "sandbox-grants.json") + storeA, err := NewGrantStore(StoreOptions{FilePath: path}) + if err != nil { + t.Fatalf("NewGrantStore A returned error: %v", err) + } + storeB, err := NewGrantStore(StoreOptions{FilePath: path}) + if err != nil { + t.Fatalf("NewGrantStore B returned error: %v", err) + } + + unlock, err := storeA.lockStateFile() + if err != nil { + t.Fatalf("lockStateFile returned error: %v", err) + } + + done := make(chan error, 1) + go func() { + _, err := storeB.Grant(GrantInput{ToolName: "write_file", Decision: GrantAllow}) + done <- err + }() + + select { + case err := <-done: + unlock() + t.Fatalf("Grant completed while another store held the file lock: %v", err) + case <-time.After(100 * time.Millisecond): + // Expected: storeB is waiting for storeA to release the lock. + } + + unlock() + select { + case err := <-done: + if err != nil { + t.Fatalf("Grant returned error after file lock release: %v", err) + } + case <-time.After(2 * time.Second): + t.Fatal("Grant did not complete after file lock release") + } +} + +// TestGrantStoreSerializesMigrationNoticeAcrossStores covers the other +// read-modify-write on the state file: clearing the pending flag. Two frontends +// starting at once must not both read it as pending and clobber each other. +func TestGrantStoreSerializesMigrationNoticeAcrossStores(t *testing.T) { + path := filepath.Join(t.TempDir(), "sandbox-grants.json") + if err := writeText(path, `{"schemaVersion":1,"grants":{"write_file":{"toolName":"write_file","decision":"allow","approvedAt":"2026-06-05T14:30:00Z"}}}`); err != nil { + t.Fatalf("write v1 grants: %v", err) + } + storeA, err := NewGrantStore(StoreOptions{FilePath: path}) + if err != nil { + t.Fatalf("NewGrantStore A returned error: %v", err) + } + storeB, err := NewGrantStore(StoreOptions{FilePath: path}) + if err != nil { + t.Fatalf("NewGrantStore B returned error: %v", err) + } + // Migrate first, so the notice is pending and the lock contention below is + // about consuming it rather than about the migration write. + if _, err := storeA.List(); err != nil { + t.Fatalf("List returned error: %v", err) + } + + unlock, err := storeA.lockStateFile() + if err != nil { + t.Fatalf("lockStateFile returned error: %v", err) + } + done := make(chan error, 1) + go func() { + notice, err := storeB.ConsumeMigrationNotice() + if err == nil && notice == "" { + err = errors.New("expected the pending migration notice") + } + done <- err + }() + + select { + case err := <-done: + unlock() + t.Fatalf("ConsumeMigrationNotice completed while another store held the file lock: %v", err) + case <-time.After(100 * time.Millisecond): + // Expected: storeB is waiting for storeA to release the lock. + } + + unlock() + select { + case err := <-done: + if err != nil { + t.Fatalf("ConsumeMigrationNotice after lock release: %v", err) + } + case <-time.After(2 * time.Second): + t.Fatal("ConsumeMigrationNotice did not complete after file lock release") + } + // The flag write landed, so the notice is not reported twice. + if again, err := storeA.ConsumeMigrationNotice(); err != nil || again != "" { + t.Fatalf("second ConsumeMigrationNotice = %q err=%v, want empty", again, err) + } +} + +// TestGrantStoreMigrationTakesTheFileLock covers the migrations that write +// through readState: a legacy file must not be rewritten (nor its backup written) +// while another process holds the lock, or two processes could both migrate and +// one would clobber the other's result. +func TestGrantStoreMigrationTakesTheFileLock(t *testing.T) { + path := filepath.Join(t.TempDir(), "sandbox-grants.json") + original := `{"schemaVersion":1,"grants":{"write_file":{"toolName":"write_file","decision":"allow","approvedAt":"2026-06-05T14:30:00Z"}}}` + if err := writeText(path, original); err != nil { + t.Fatalf("write v1 grants: %v", err) + } + storeA, err := NewGrantStore(StoreOptions{FilePath: path}) + if err != nil { + t.Fatalf("NewGrantStore A returned error: %v", err) + } + storeB, err := NewGrantStore(StoreOptions{FilePath: path}) + if err != nil { + t.Fatalf("NewGrantStore B returned error: %v", err) + } + + unlock, err := storeA.lockStateFile() + if err != nil { + t.Fatalf("lockStateFile returned error: %v", err) + } + done := make(chan error, 1) + go func() { + _, err := storeB.List() + done <- err + }() + + select { + case err := <-done: + unlock() + t.Fatalf("the migration ran while another store held the file lock: %v", err) + case <-time.After(100 * time.Millisecond): + if raw, err := os.ReadFile(path); err != nil { + unlock() + t.Fatalf("ReadFile grants: %v", err) + } else if string(raw) != original { + unlock() + t.Fatalf("the grant file was rewritten while the lock was held:\n%s", raw) + } + } + + unlock() + select { + case err := <-done: + if err != nil { + t.Fatalf("List after lock release: %v", err) + } + case <-time.After(2 * time.Second): + t.Fatal("the migration did not complete after file lock release") + } + raw, err := os.ReadFile(path) + if err != nil { + t.Fatalf("ReadFile migrated grants: %v", err) + } + if !strings.Contains(string(raw), `"schemaVersion": 3`) { + t.Fatalf("grant file was not migrated after the lock was released:\n%s", raw) + } +} + func TestGrantStoreMigratesExactV1GrantAndReportsOnce(t *testing.T) { root := t.TempDir() path := filepath.Join(root, "sandbox-grants.json")