fix(sandbox): add platform-specific write lock around grant state file (#752)

[PierrunoYT]

Summary

• Adds platform-specific file-level write locks for the persisted grant state (grants_lock_unix.go, grants_lock_windows.go).
• Wires lockStateFile() into Grant, GrantCommandPrefix, Revoke, RevokePath, and Clear so concurrent processes serialize access to the grant JSON file.
• Adds regression coverage in grants_test.go.
• Adds .gitattributes to enforce LF line endings for Go files so gofmt checks pass on Windows.

Closes #752

Test plan

• gofmt -l . clean
• go vet ./... passes
• go test ./... passes
• go run ./cmd/zero-release build succeeds
• go run ./cmd/zero-release smoke passes
• git diff --check clean

Generated with Devin

Summary by CodeRabbit

• Bug Fixes

  • Improved sandbox grant state handling when multiple processes or instances modify grants simultaneously.
  • Added cross-platform file locking to prevent conflicting updates and reduce the risk of lost or corrupted grant changes.
  • Grant operations now wait briefly for an active update to finish and report an error if the lock cannot be acquired.

• Tests

  • Added coverage for serialized grant updates across separate store instances.

[Copilot]

Pull request overview

This PR addresses a sandbox security race where concurrent zero processes could overwrite each other’s persisted grants file and inadvertently resurrect a revoked permission. It does so by adding an inter-process, file-level write lock around the full read-modify-write transaction for the grants JSON.

Changes:

• Add platform-specific file locking helpers (Unix flock, Windows LockFileEx/UnlockFileEx) and a timed retry loop to acquire the lock.
• Serialize all persisted grant mutations by acquiring the lock in Grant, GrantCommandPrefix, Revoke, RevokePath, and Clear before reading/writing state.
• Add a regression test to ensure concurrent stores block while the lock is held, plus .gitattributes to enforce LF endings for Go/module files.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
internal/sandbox/grants.go	Adds a lock acquisition step (with timeout/retry) around grant state read-modify-write operations.
internal/sandbox/grants_test.go	Adds a concurrency regression test validating that a second store blocks while the lock is held.
internal/sandbox/grants_lock_unix.go	Implements non-blocking exclusive lock/unlock using syscall.Flock for non-Windows builds.
internal/sandbox/grants_lock_windows.go	Implements non-blocking exclusive lock/unlock using Win32 LockFileEx/UnlockFileEx.
.gitattributes	Forces LF line endings for Go/module/sum files to keep formatting checks consistent on Windows.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: bbbeb343-8e79-4eb4-8d22-ab423567420c

📥 Commits

Reviewing files that changed from the base of the PR and between ce4a996 and a60e60c.

📒 Files selected for processing (5)

• .gitattributes
• internal/sandbox/grants.go
• internal/sandbox/grants_lock_unix.go
• internal/sandbox/grants_lock_windows.go
• internal/sandbox/grants_test.go

---

Walkthrough

Changes

Sandbox grant locking

Layer / File(s)	Summary
Platform lock primitives internal/sandbox/grants.go, internal/sandbox/grants_lock_*	Adds lock timing configuration and Unix/Windows implementations for non-blocking exclusive file locks.
Grant transaction locking internal/sandbox/grants.go	Acquires an adjacent lock file around grant read-modify-write and clearing operations, with timeout handling and deferred unlocks.
Serialization test and repository normalization internal/sandbox/grants_test.go, .gitattributes	Tests serialization across stores sharing a grants file and enforces LF endings for Go-related files.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested reviewers: copilot, gnanam1990, kevincodex1

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The PR adds interprocess locking, but the required re-read after acquiring the lock is not evidenced in the change summary.	Re-read the grants file immediately after acquiring the interprocess lock, then mutate and write from that fresh state.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: adding a platform-specific write lock for sandbox grant state.
Out of Scope Changes check	✅ Passed	The .gitattributes normalization and regression test are in scope for the lock fix and do not appear unrelated.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[Vasanthdev2004]

Approve. This closes the #752 lost-update/resurrection hole cleanly, and the Windows path in particular is done right.

Checked on head a60e60c:

• The full read-modify-write is under the inter-process lock: every mutator (Grant, GrantCommandPrefix, Revoke, RevokePath, Clear) does lockStateFile then readState then mutate then writeState, with the read inside the lock, so two processes cannot both read-then-write and clobber a revoke.
• writeState is atomic (temp file in the same dir plus os.Rename), so the unlocked reader / permission-check path never sees a torn file, only the complete old or new one.
• Fail-closed: on a lock error or the 5s timeout the mutators return the error and abort, and the CLI surfaces a Revoke error instead of printing a success line, so a dropped revoke cannot be mistaken for a completed one.
• Windows LockFileEx (grants_lock_windows.go): success is keyed off the BOOL return value (if result != 0), not off the err from LazyProc.Call, which is always the last-error. That is the right way to do it and sidesteps the trap where a failed lock reads as success and silently disables the protection. Flags are LOCKFILE_EXCLUSIVE_LOCK|LOCKFILE_FAIL_IMMEDIATELY with a zeroed OVERLAPPED, which is correct.
• Built and ran internal/sandbox locally: gofmt, vet and build clean, grant tests pass including the cross-store serialization test that exercises the real file lock. CI green.

One optional note, not blocking: grants_lock_unix.go uses //go:build !windows, which nominally includes plan9 and js where syscall.Flock is undefined. It matches the existing convention in the repo (permissions_lock_unix.go and friends), and Zero only ships linux/darwin/windows, so it builds fine for every real target. Flagging only because I hit the same tag question on another lock PR; take it or leave it.

LGTM.

[jatmn]

I found issues that need to be addressed before this is ready.

Findings

• [P2] Rebase this branch onto the current main before merging
  internal/sandbox/grants.go
  The PR head is based on ce4a996, while main is now 96859c9; GitHub reports the PR as CONFLICTING/DIRTY, and the merge has content conflicts in both this file and grants_test.go. Resolve the conflicts against the current state model and rerun the grant-state review/tests rather than selecting either side wholesale. In particular, retain main's schema-v3 and legacy/policy migration behavior, and extend the new interprocess transaction to ConsumeMigrationNotice plus the migrations that currently write through readState; otherwise the rebase would either regress existing grants or leave additional lost-update paths.