feat(sandbox): give each workspace an offline and an online principal

[Vasanthdev2004]

Stacked on #808, and targets that branch rather than main so the diff is only the new work. #808 should land first.

What this fixes

#808 shipped with the principal backend standing down whenever the network was denied. Deny is the default, so opting in left the restricted-token path doing all the work in ordinary use and the new identity only engaged for approved network commands. That was the honest choice at the time, but it made the feature close to inert.

The cause: network denial is enforced by block filters keyed to the offline-marker SID, and a principal token cannot carry it. LogonUser builds a token from an account's real group memberships, and the marker is a synthetic capability SID with no account behind it.

How

A real local group. ZeroSandboxOffline is created by setup, the block filters name its SID alongside the marker, and a principal is denied the network by being a member of it. Each workspace gets two accounts that differ only in that membership, and the command's network mode picks between them.

A group rather than each principal's own SID because principals here are per workspace: one filter set covers every offline principal on the machine, instead of needing a filter per workspace and a setup re-run whenever a workspace appears.

Both principals are provisioned together even though a setup run only sees one profile. Setup needs elevation and commands do not, so provisioning lazily would mean an unelevated command discovering it needs an account it cannot create. They also get identical filesystem access, so an approved network command sees the same filesystem as an ordinary one.

Two orderings that are load bearing

The network plan is now built after provisioning. The group it keys to is created there. Planning first installed filters naming only the marker and left every offline principal with an open network while the machine looked correctly set up. That is the same class of silent-downgrade bug this PR exists to remove, so it is worth naming.

The role tag sits before the workspace hash in the account name. Truncation at the 20 character limit then eats hash characters rather than the tag. A lost tag would collide the two roles onto one account, on exactly the workspaces most likely to truncate, and the dangerous direction of that collision is an ordinary command silently getting the online principal.

Anything that is not an explicit allow maps to the offline principal, so an unrecognised mode loses the network rather than keeps it.

Setup-marker compatibility

The filter identity set is resolved, not assumed, and stays absent until the group exists. A machine that never provisions principals computes exactly the plan it computed before. This matters because the plan is hashed into the setup marker and re-derived on every command: an identity set that differed between setup and the command path would fail every command with "setup is out of date". There is a test for the hash changing when the group is present and for a lookup failure propagating rather than silently degrading to "no group".

Verification, and what is not verified

gofmt, go vet, GOOS=windows go vet, and builds for linux, darwin and windows are clean.

Before I added the last three tests, the full internal/sandbox suite passed on this branch with the single expected failure: the test asserting the old stand-down, which this replaces.

The three new tests have not been run locally. Smart App Control on this machine is enforcing and started refusing every freshly built unsigned test binary partway through, including via go test, and turning it off is not something I am going to do to get a green local run. CI is the check for those. They are TestPrincipalRoleFollowsNetworkMode, TestWindowsSandboxUserNameSeparatesRoles, and the two TestNetworkInfraPlan... cases, the last of which are portable and will run on the Linux and macOS jobs too.

Also unchanged from #808: the privileged half is still unexercised here, so LogonUser and the LSA rights need a clean elevated box.

Cost worth stating

This doubles the sandbox accounts, to two per workspace. Given the concern already raised about local account creation being visible to endpoint protection and enterprise policy, that is a real increase and a deliberate trade for making network denial work under a separate identity.

Summary by CodeRabbit

• New Features
  • Added separate offline and online Windows sandbox principals with role-specific account naming.
  • Windows network planning can now include an additional offline-group identity SID for coverage checks.
• Bug Fixes
  • Windows sandbox principal eligibility is now controlled solely by explicit opt-in, with unknown/empty network modes failing closed.
  • Improved Windows sandbox setup/teardown sequencing and hardened role-aware cleanup, including safer rollback and validation behavior.
• Tests
  • Updated and expanded coverage for opt-in gating, network-to-role mapping, role naming separation, offline-group SID behavior, and dual-role rollback scenarios.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

Windows sandbox identity handling now provisions separate offline and online principals, selects them by network mode, applies offline-group network enforcement, and builds and validates network infrastructure after principal provisioning.

Changes

Windows identity and network enforcement

Layer / File(s)	Summary
Role-aware identity provisioning internal/sandbox/windows_identity_windows.go, internal/sandbox/windows_identity_windows_test.go, internal/sandbox/windows_identity_rollback_windows_test.go, internal/sandbox/windows_identity_policy_windows_test.go	Introduces role-specific account names, offline-group membership, ownership checks, role-aware lookup and provisioning, teardown handling, and updated provisioning tests.
Runtime role selection and ACL setup internal/sandbox/windows_identity_runtime_windows.go, internal/sandbox/windows_identity_runtime_windows_test.go, internal/sandbox/windows_dualrole_rollback_windows_test.go	Makes eligibility depend only on opt-in, maps network modes to roles, provisions both roles, applies per-role ACLs, and validates rollback behavior for adopted and newly created principals.
Network plan and setup ordering internal/sandbox/windows_network.go, internal/sandbox/windows_network_test.go, internal/sandbox/windows_online_offline_test.go, internal/sandbox/windows_setup_windows.go	Adds offline-group SID coverage to network plans, propagates lookup errors, validates principal coverage, and builds the plan after provisioning with rollback handling.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant runWindowsSandboxSetup
  participant setupWindowsSandboxPrincipal
  participant BuildWindowsNetworkInfraPlan
  participant WindowsNetworkPlanCoversPrincipals
  runWindowsSandboxSetup->>setupWindowsSandboxPrincipal: provision offline and online principals
  setupWindowsSandboxPrincipal-->>runWindowsSandboxSetup: return role-specific identities
  runWindowsSandboxSetup->>BuildWindowsNetworkInfraPlan: build network plan
  BuildWindowsNetworkInfraPlan-->>runWindowsSandboxSetup: return identity SIDs and filters
  runWindowsSandboxSetup->>WindowsNetworkPlanCoversPrincipals: verify principal coverage

Loading

Possibly related PRs

• Gitlawb/zero#808: Refactors Windows principal eligibility and runtime token selection around opt-in and offline/online roles.

Suggested reviewers: gnanam1990, anandh8x

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the core change: each workspace now gets separate offline and online principals.
Docstring Coverage	✅ Passed	Docstring coverage is 97.44% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/windows-sandbox-offline-online

---

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

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

internal/sandbox/windows_identity_windows.go (1)

179-198: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

No migration path for accounts provisioned before role tags existed. Adding the role tag to windowsSandboxUserName makes every previously provisioned account name unreachable: nothing deletes it, and nothing finds it. On an existing opt-in machine that means a stale local account plus its secret and LSA logon rights linger forever, while commands silently drop to the restricted-token backend with no indication that setup needs re-running.

• internal/sandbox/windows_identity_windows.go#L179-L198: delete or adopt the legacy untagged name (windowsSandboxUserPrefix + cleaned) during provisioning, so re-running setup reclaims the old account instead of abandoning it.
• internal/sandbox/windows_identity_runtime_windows.go#L96-L136: when the role lookup misses but a legacy untagged account exists, emit a one-line stderr hint pointing at zero sandbox setup rather than falling back mutely.

🤖 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/sandbox/windows_identity_windows.go` around lines 179 - 198, The new
role-tagged naming in windowsSandboxUserName needs a migration path for legacy
accounts. In internal/sandbox/windows_identity_windows.go lines 179-198, during
provisioning detect and delete or adopt the legacy untagged name formed from
windowsSandboxUserPrefix and cleaned before using the role-tagged name; in
internal/sandbox/windows_identity_runtime_windows.go lines 96-136, when role
lookup misses but that legacy account exists, emit a one-line stderr hint
directing the user to zero sandbox setup instead of silently falling back.

🧹 Nitpick comments (3)

internal/sandbox/windows_identity_windows_test.go (1)

396-425: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift

Exercise the online role through provisioning, not only name generation.

This regression test proves that offline and online names differ, but the privileged provisioning, logon-rights, token, and lookup tests cover only windowsSandboxRoleOffline. Add an online-role round-trip case (or parameterize the existing integration test) so a role-specific provisioning or lookup regression cannot pass unnoticed.

As per coding guidelines: “**/*_test.go: Keep tests beside their source files, add regression tests for behavior changes, and run affected concurrent code under the race detector.”

🤖 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/sandbox/windows_identity_windows_test.go` around lines 396 - 425,
Add an integration round-trip test for windowsSandboxRoleOnline, covering
provisioning, logon-right assignment, token creation, and account lookup
alongside the existing offline-role coverage. Prefer parameterizing the existing
role-specific integration test so both roles execute the same assertions, while
preserving the current offline case and running the affected concurrent test
with the race detector.

Source: Coding guidelines

internal/sandbox/windows_network_test.go (1)

128-129: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Global hook mutation is safe only while no test in this package is parallel.

Save/restore via t.Cleanup is right, but resolveWindowsSandboxOfflineGroupSIDHook is package-global. The moment someone adds t.Parallel() to any test that calls BuildWindowsNetworkInfraPlan, this becomes a data race that -race will flag. A short comment stating these tests must stay serial would save the next person the debug.

As per coding guidelines, "run affected concurrent code under the race detector".

🤖 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/sandbox/windows_network_test.go` around lines 128 - 129, Document
near the save/restore of resolveWindowsSandboxOfflineGroupSIDHook that tests
using this package-global hook, including BuildWindowsNetworkInfraPlan callers,
must remain serial and must not use t.Parallel; preserve the existing t.Cleanup
restoration.

Source: Coding guidelines

internal/sandbox/windows_setup_windows.go (1)

57-73: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Ordering fix is correct. The offline group only exists after provisioning, so building the plan here is what makes the filters actually name it.

Optional: this block, the applyWindowsNetworkPlan block, and the marker-write block are now three byte-identical rollback-and-report branches. A small failSetup(stderr, err, rollback) int helper would collapse ~24 lines and guarantee the next failure path can't forget the rollback.

🤖 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/sandbox/windows_setup_windows.go` around lines 57 - 73, The setup
failure handling is duplicated across the plan-building,
applyWindowsNetworkPlan, and marker-write branches. Add a failSetup helper that
accepts stderr, the setup error, and rollback, performs the existing
rollback-and-report behavior, and returns the failure status; replace all three
duplicated branches with calls to this helper while preserving their current
error messages and rollback semantics.

🤖 Prompt for all review comments with 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.

Inline comments:
In `@internal/sandbox/windows_identity_windows_test.go`:
- Around line 214-216: Update the privileged tests around
provisionWindowsSandboxIdentity to stop unconditionally removing deterministic
accounts via removeWindowsSandboxIdentity. Use a unique per-test account key, or
track and remove only identities created by the current test, preserving
legitimate pre-existing accounts in both affected test cases.

In `@internal/sandbox/windows_network.go`:
- Around line 84-86: Update the groupSID handling in the identity SID collection
to append the trimmed value produced by strings.TrimSpace, while retaining the
non-empty guard. Ensure IdentitySIDs contains canonical SID strings so
applyWindowsNetworkPlan can consume them directly.

---

Outside diff comments:
In `@internal/sandbox/windows_identity_windows.go`:
- Around line 179-198: The new role-tagged naming in windowsSandboxUserName
needs a migration path for legacy accounts. In
internal/sandbox/windows_identity_windows.go lines 179-198, during provisioning
detect and delete or adopt the legacy untagged name formed from
windowsSandboxUserPrefix and cleaned before using the role-tagged name; in
internal/sandbox/windows_identity_runtime_windows.go lines 96-136, when role
lookup misses but that legacy account exists, emit a one-line stderr hint
directing the user to zero sandbox setup instead of silently falling back.

---

Nitpick comments:
In `@internal/sandbox/windows_identity_windows_test.go`:
- Around line 396-425: Add an integration round-trip test for
windowsSandboxRoleOnline, covering provisioning, logon-right assignment, token
creation, and account lookup alongside the existing offline-role coverage.
Prefer parameterizing the existing role-specific integration test so both roles
execute the same assertions, while preserving the current offline case and
running the affected concurrent test with the race detector.

In `@internal/sandbox/windows_network_test.go`:
- Around line 128-129: Document near the save/restore of
resolveWindowsSandboxOfflineGroupSIDHook that tests using this package-global
hook, including BuildWindowsNetworkInfraPlan callers, must remain serial and
must not use t.Parallel; preserve the existing t.Cleanup restoration.

In `@internal/sandbox/windows_setup_windows.go`:
- Around line 57-73: The setup failure handling is duplicated across the
plan-building, applyWindowsNetworkPlan, and marker-write branches. Add a
failSetup helper that accepts stderr, the setup error, and rollback, performs
the existing rollback-and-report behavior, and returns the failure status;
replace all three duplicated branches with calls to this helper while preserving
their current error messages and rollback semantics.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: a1ae6364-8744-4d97-a882-357b313596ad

📥 Commits

Reviewing files that changed from the base of the PR and between fbe340b and aa861df.

📒 Files selected for processing (7)

• internal/sandbox/windows_identity_runtime_windows.go
• internal/sandbox/windows_identity_runtime_windows_test.go
• internal/sandbox/windows_identity_windows.go
• internal/sandbox/windows_identity_windows_test.go
• internal/sandbox/windows_network.go
• internal/sandbox/windows_network_test.go
• internal/sandbox/windows_setup_windows.go

[Vasanthdev2004]

CI is green on all five checks, which closes the gap I flagged in the description.

The three tests I could not run locally have now run: TestPrincipalRoleFollowsNetworkMode and TestWindowsSandboxUserNameSeparatesRoles on the Windows job, and the two TestNetworkInfraPlan cases on Windows, Linux and macOS since they carry no build tag. None of them skip, so green means they executed rather than quietly opting out.

Still unexercised, unchanged from #808: the privileged calls. LogonUser, the LSA rights, and now the offline-group membership that actually carries the network denial all need a clean elevated box. Membership is the whole mechanism here, so that is the thing most worth proving on real hardware before this leaves draft.

[Vasanthdev2004]

Both handled in f551a44, though the second one differently than suggested.

Deleting accounts by derived name. Right, and the fix belongs deeper than the tests. removeWindowsSandboxIdentity is always called with a name we derive, so the production teardown path had the same hazard: the only thing separating "our account" from "somebody else's account that happens to match" was the name matching a pattern we generate ourselves. Making only the fixtures safe would have left that in place.

Ownership is now read back from the comment provisioning stamps on the account, and an account that is not ours is left alone rather than deleted. The gated tests get it for free, since their pre-clean goes through the same helper, so they keep the deterministic key and still converge after a crashed run.

The test asserts against Administrator, Guest and DefaultAccount, which exist on any install and are definitively not ours. It needs no privilege, because it only has to establish that they are not classified as managed; NetUserDel is never reached for them. Classifying everything as managed makes it fail, so it is load bearing.

The untrimmed append. Taken, but the stated consequence does not hold and I would rather say so than quietly accept it. applyWindowsNetworkPlan does not consume IdentitySIDs raw: it goes through newWindowsWFPUserCondition, whose first statement is canonicalWindowsNetworkSIDs, which trims. A padded SID would have been trimmed before reaching StringToSid, so the filters could not have come out narrower than the plan. On top of that the resolver returns SID.String(), which never carries whitespace. So this is defensive tidying, not a fix, and worth recording as such in case someone later reads the diff as evidence of a bug that existed.

Local runs are working again, so everything on this branch is now verified here rather than only in CI: the full internal/sandbox suite passes, including the four tests I had to leave to CI in the description.

[github-actions]

Zero automated PR review

Verdict: No blockers found

Blockers

• None found.

Validation

• [pass] Diff hygiene: git diff --check
• [pass] Tests: go test ./...
• [pass] Build: go run ./cmd/zero-release build
• [pass] Smoke build: go run ./cmd/zero-release smoke

Scope

Head: b88ac4693839
Changed files (16): internal/sandbox/windows_dualrole_rollback_windows_test.go, internal/sandbox/windows_identity_policy_windows_test.go, internal/sandbox/windows_identity_privilege_recheck_windows_test.go, internal/sandbox/windows_identity_rollback_windows_test.go, internal/sandbox/windows_identity_runtime_windows.go, internal/sandbox/windows_identity_runtime_windows_test.go, internal/sandbox/windows_identity_windows.go, internal/sandbox/windows_identity_windows_test.go, internal/sandbox/windows_network.go, internal/sandbox/windows_network_test.go, internal/sandbox/windows_offline_group_ownership_windows_test.go, internal/sandbox/windows_offline_membership_windows_test.go, and 4 more

This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality.

[gnanam1990]

Verdict

Changes requested — on branch state, not on the design.

Reviewed at 6c3037fe2c17, base feat/windows-sandbox-identity, re-confirmed live before posting. This is no longer marked draft, so I am reviewing it as a real pull request rather than leaving it alone as I did earlier.

The blocking point is small and mechanical: this branch is behind the branch it targets.

#808 is at 832f53a; this is at 6c3037f, and git merge-base --is-ancestor pr808 pr812 is false. The commit you are missing is 832f53a "fix(sandbox): drop the stored secret whenever provisioning fails", which is not incidental to this change — it touches provisionWindowsSandboxIdentity, the same function this branch edits, and #808 changed its signature to return an extra bool. A textual auto-merge that reports no conflict is not sufficient evidence here; this repository has already produced a clean merge that did not build. Rebase onto the current #808 head and confirm the merged tree still cross-compiles for Windows before this is ready.

I checked each branch independently: GOOS=windows go vet ./internal/sandbox/... and GOOS=windows go test -c both pass on #808 at 832f53a and on this branch at 6c3037f. I did not get a clean run of the two merged together, which is exactly the gap the rebase closes.

On the substance, which I think is right.

The diagnosis is the strongest part. #808 shipped with the principal backend standing down whenever the network was denied, deny is the default, so the feature was close to inert in ordinary use — and the reason is not visible from the code. LogonUser builds a token from real group memberships and the offline marker is a synthetic capability SID with no account behind it, so the two mechanisms could never meet. A real local group is the right answer, and keying one filter set to it rather than one per workspace is the right trade.

Building the network plan after provisioning is load bearing and I would keep it as prominent as you have it. Planning first installed filters naming only the marker and left every offline principal with an open network while the machine reported itself correctly configured — a security control that reports success while enforcing nothing. That deserves a test asserting the ordering directly rather than only the end state, because it is exactly the kind of thing a later refactor reverses silently.

Proving ownership before deleting is the same predicate #808 now uses before adopting, correctly applied to the other end. Good.

One non-blocking observation on that path. When the account is not ours, removal returns nil. Idempotence makes that defensible — there is no principal of ours, so the goal state holds — but it also means a stranger's account squatting the derived name produces a silent success, and the operator is told cleanup completed when a name they may care about was left in place. A diagnostic, or a distinguishable return, would cost little.

Worth settling alongside #808 rather than here. Two accounts per workspace instead of one doubles what an operator sees in net user, and endpoint protection and enterprise policy were already an open product question on the parent. That is a conversation for the stack, not a defect in this change.

Limitations. No Windows host, no elevated session. Everything above rests on reading and cross-compilation; none of the privileged paths this adds have been executed by me.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

internal/sandbox/windows_identity_windows_test.go (2)

95-96: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win

Do not print generated passwords.

A failed assertion writes the generated credential into test and CI logs. Redact it.

Proposed fix

-		t.Fatalf("password %q lacks a required character class", first)
+		t.Fatal("generated password lacks a required character class")

🤖 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/sandbox/windows_identity_windows_test.go` around lines 95 - 96,
Update the failure message in the password character-class assertion to avoid
interpolating the generated password variable first. Keep the assertion and
required-character-class diagnostic, but report only a non-sensitive message so
generated credentials never appear in test or CI logs.

---

217-274: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift

Add an offline-group membership assertion to the elevated provisioning test.
This round-trip covers provisioning and batch logon, but it still doesn’t check that the offline principal was added to ZeroSandboxOffline, which is the part that actually drives network denial.

🤖 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/sandbox/windows_identity_windows_test.go` around lines 217 - 274,
Extend the elevated provisioning test after the second provision to verify that
the identity from provisionWindowsSandboxIdentity belongs to the
ZeroSandboxOffline group. Use the existing Windows group-membership lookup or
assertion helper, and fail the test if the provisioned SID is not a member while
preserving the current logon and cleanup checks.

Source: Coding guidelines

🤖 Prompt for all review comments with 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.

Outside diff comments:
In `@internal/sandbox/windows_identity_windows_test.go`:
- Around line 95-96: Update the failure message in the password character-class
assertion to avoid interpolating the generated password variable first. Keep the
assertion and required-character-class diagnostic, but report only a
non-sensitive message so generated credentials never appear in test or CI logs.
- Around line 217-274: Extend the elevated provisioning test after the second
provision to verify that the identity from provisionWindowsSandboxIdentity
belongs to the ZeroSandboxOffline group. Use the existing Windows
group-membership lookup or assertion helper, and fail the test if the
provisioned SID is not a member while preserving the current logon and cleanup
checks.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: ae325643-a3d0-46ab-88da-b3072102d409

📥 Commits

Reviewing files that changed from the base of the PR and between 6c3037f and 4fc1bfa.

📒 Files selected for processing (7)

• internal/sandbox/windows_identity_runtime_windows.go
• internal/sandbox/windows_identity_runtime_windows_test.go
• internal/sandbox/windows_identity_windows.go
• internal/sandbox/windows_identity_windows_test.go
• internal/sandbox/windows_network.go
• internal/sandbox/windows_network_test.go
• internal/sandbox/windows_setup_windows.go

🚧 Files skipped from review as they are similar to previous changes (6)

• internal/sandbox/windows_network_test.go
• internal/sandbox/windows_identity_runtime_windows_test.go
• internal/sandbox/windows_network.go
• internal/sandbox/windows_setup_windows.go
• internal/sandbox/windows_identity_windows.go
• internal/sandbox/windows_identity_runtime_windows.go

[Vasanthdev2004]

Your blocking point is fixed, and your reason for insisting on it was correct in a way I would not have predicted.

Rebased. This is now on #808 at 99fefdc4, which itself moved: #808 has been rebased onto current main, so both branches sit on a live base rather than an old one. git merge-base --is-ancestor pr808 pr812 is true.

And you were right to refuse a clean auto-merge as evidence. The rebase reported no conflict and did not build. #808's new line resolves the secret path via windowsSandboxSecretPath(config.SandboxHome, windowsSandboxUserName(key)), and this branch had made windowsSandboxUserName take a role. The two edits never touch the same lines, so git merged them happily and produced a call with the wrong arity. go vet caught it; nothing about the rebase output suggested anything was wrong. That is the second time in this repository, and it is exactly the case you named.

The evidence you asked for, on the merged tree:

GOOS=windows go vet ./internal/sandbox/...   PASS
GOOS=windows go test -c                       PASS
808 head is an ancestor of 812                PASS

Plus gofmt, go build for linux, darwin and windows, and the sandbox suite.

On the ordering, I took your suggestion and went a step further. You asked for a test asserting the ordering directly rather than the end state. A test alone would not have helped much, because the thing that breaks is the sequence inside runWindowsSandboxSetup and a unit test cannot easily observe it. So setup now checks the plan it is about to install actually names the offline group, and refuses otherwise. Moving the plan build back before provisioning now fails loudly at setup time rather than producing filters that name only the marker.

The predicate is separate so it can be asserted on its own: marker-only reads as uncovered, marker-plus-group as covered, case differences do not read as missing, and a host with no group provisioned is not refused, since there is no principal for the filters to miss. Making coverage always return true fails that test.

The silent success on removal is fixed too. It returns a distinguishable typed error now, and teardown treats it as success, since "no principal of ours under this name" is the goal state whether we deleted one or declined to. You were right that a bare nil tells an operator cleanup completed when a name they may care about was deliberately left in place.

On the account count, agreed it belongs on the stack rather than here. It is two per workspace now, and the local-account visibility question was already open on #808. I have flagged it in both descriptions rather than treating it as settled.

Your limitation note applies to me too, more so: the privileged paths in this change have not been executed anywhere, and the offline-group membership is the mechanism that carries the network denial, so it is the thing I would most want proven on real hardware before this ships.

[Vasanthdev2004]

@gnanam1990 your changes-requested here is on 6c3037fe; the branch is now at 011b26d8, rebased onto #808 at 99fefdc4, which itself moved onto current main. Your blocking point is addressed, along with both of the non-blocking ones.

Worth repeating the part you called: you refused to accept a clean auto-merge as evidence, and you were right. The rebase reported no conflict and did not build. #808 resolves the secret path through windowsSandboxUserName(key) while this branch had made that function take a role; the two edits never touch the same lines, so git merged them happily and produced a call with the wrong arity. go vet caught it, nothing in the rebase output suggested a problem.

Evidence on the merged tree, as you asked:

GOOS=windows go vet ./internal/sandbox/...   PASS
GOOS=windows go test -c                       PASS
808 head is an ancestor of 812                PASS

On the ordering, I went further than a test. A unit test cannot easily observe a sequence inside runWindowsSandboxSetup, so setup now checks the plan it is about to install actually names the offline group and refuses otherwise. Moving the plan build back before provisioning fails loudly rather than installing filters that name only the marker. The predicate is asserted separately, and making it always report coverage fails that test.

Removal no longer reports plain success when it declines to delete an account Zero did not create; it returns a distinguishable typed error, and teardown treats it as success since the goal state holds either way.

Full detail is in my earlier comment. Your limitation note applies to me more than to you: the privileged paths here have been executed by nobody, and offline-group membership is the mechanism that carries the network denial, so that is the thing most worth proving on real hardware before this ships.

[gnanam1990]

Verdict

Approve.

Reviewed at 011b26d8ff3c, base feat/windows-sandbox-identity (99fefdc), re-confirmed live before posting. This withdraws my changes-requested.

The blocking point is resolved. This branch is no longer behind the one it targets: git merge-base --is-ancestor pr808 pr812 is now true and there are zero commits on #808 missing here, including 99fefdc, the one that changed provisionWindowsSandboxIdentity's signature and was my specific concern. I re-checked the merged state rather than relying on the ancestry alone — at this head go build ./..., go vet ./... and ./internal/sandbox all pass, and GOOS=windows go vet ./internal/sandbox/... plus GOOS=windows go test -c both succeed, so the stacked tree type-checks for Windows.

My non-blocking observation is also addressed, and better than I framed it. Removal no longer returns a bare nil when the account is not Zero's. errWindowsSandboxForeignAccountRetained is distinguishable, teardown paths treat it as success, and the comment explains the distinction precisely: leaving the account alone is correct, but reporting plain success would tell an operator cleanup completed when a name they may care about was deliberately left in place. That is exactly the right resolution — it keeps idempotence for callers who want the goal state while making the refusal visible to anyone who cares which.

The new assertion that the filters cover principals is worth having for the same reason the ordering is: it pins a property that is invisible from the outside when it breaks.

On the substance, which I said before and still think. The diagnosis is the strongest part of this change. #808 shipped with the principal backend standing down whenever the network was denied, deny is the default, so the feature was close to inert in ordinary use — and nothing in the code reveals why. LogonUser builds a token from real group memberships while the offline marker is a synthetic capability SID with no account behind it, so the two could never meet. A real local group is the right answer, and keying one filter set to it rather than one per workspace is the right trade.

Building the network plan after provisioning remains the load-bearing ordering, since planning first left every offline principal with an open network while the machine reported itself correctly configured. Proving ownership before deleting is the same predicate #808 uses before adopting, correctly applied to the other end.

One thing that belongs to the stack rather than to this PR. Two accounts per workspace instead of one doubles what an operator sees in net user, and endpoint protection and enterprise policy were already an open product question on the parent. Worth settling there before either lands.

Limitations. No Windows host, no elevated session. Everything above rests on reading and cross-compilation; none of the privileged paths this adds have been executed by me, and the same caveat that made #808 approvable applies here — the surface is behind an opt-in flag that is off by default.

CodeRabbit's changes-requested from earlier is separate from this and still outstanding.

Merge order is #808 then this, and merge itself is kevin's call per the program gate.

[anandh8x]

Review at 011b26d

PR #812 — give each workspace an offline and an online principal. Stacked on #808 (targets that branch). 7 files, +520/-97 incremental over #808's head. 3 commits.

Verdict: approve. The design closes the biggest gap in #808 (network denial was inert under the principal backend), and the load-bearing orderings are pinned by tests.

What this fixes

#808 shipped with the principal backend standing down whenever the network was denied. Deny is the default, so opting in left the restricted-token path doing all the work — the feature was close to inert. The cause: WFP block filters key to the offline-marker SID (a synthetic capability SID), and LogonUser builds a token from an account's real group memberships, so a principal token can't carry the marker.

How

A real local group (ZeroSandboxOffline). The block filters name its SID alongside the marker. A principal is denied the network by being a member of it. Each workspace gets two accounts that differ only in that membership; the command's network mode picks between them. A group rather than per-principal SIDs because one filter set covers every offline principal on the machine — no per-workspace filter and no setup re-run when a workspace appears.

What's good

• The diagnosis is the strongest part. The PR body explains why #808's principal was inert (LogonUser vs. synthetic SID) and why a group is the right fix (one filter set, no per-workspace filters). That's the kind of reasoning that makes the change safe to review without a Windows box.
• Fail-closed for unknown modes. Anything that isn't an explicit NetworkAllow maps to the offline principal. An unrecognized mode loses the network rather than keeps it. windowsSandboxRoleForNetwork is a clean 3-line function.
• Role tag before the hash in the account name. zero-sbx-d<hash> (offline) vs zero-sbx-n<hash> (online). Truncation at the 20-char limit eats hash characters, never the tag. TestWindowsSandboxUserNameSeparatesRoles proves the two roles never collide, including under truncation — the dangerous collision is an ordinary command silently getting the online principal.
• Both roles provisioned together. Setup needs elevation; commands don't. Provisioning lazily would mean an unelevated command discovering it needs an account it can't create. Both get identical filesystem access — an approved network command sees the same filesystem as an ordinary one.
• Network plan built AFTER provisioning. The group it keys to is created there. Planning first installed filters naming only the marker, leaving every offline principal with an open network while the machine reported correctly set up. The PR calls this out explicitly as "the same class of silent-downgrade bug this PR exists to remove."
• WindowsNetworkPlanCoversPrincipals pins the ordering. This predicate is asserted in setup before installing anything. Moving the plan build back ahead of provisioning fails loudly. TestNetworkPlanCoverageDetectsPrincipalsLeftUncovered proves a marker-only plan is detected as not covering principals, while a marker-plus-group plan passes. Case-insensitive SID comparison. Unprovisioned host (no group) is a no-op.
• Setup-marker compatibility. The filter identity set is resolved, not assumed, and stays absent until the group exists. A machine that never provisions principals computes exactly the plan it computed before. TestNetworkInfraPlanIncludesOfflineGroupIdentity proves the hash changes when the group appears (so setup and command path can't disagree silently) and the marker SID stays at position 0.
• Lookup failure propagates. TestNetworkInfraPlanPropagatesOfflineGroupLookupFailure proves a failed group lookup returns an error rather than silently degrading to "no group" (which would produce filters that cover no principal).
• Ownership-before-delete. removeWindowsSandboxIdentity now calls windowsSandboxUserIsManaged before deleting. A foreign account returns errWindowsSandboxForeignAccountRetained (distinguishable, not bare nil). Teardown paths treat it as success; operators see the refusal. This is gnanam's non-blocking finding from #808, addressed better than asked — the comment explains precisely why bare nil would be wrong.
• Rollback is multi-step and ordered. setupWindowsSandboxPrincipal provisions both roles in a loop, appending removal and ACL-revert closures to an undo slice. Rollback unwinds in reverse, keeps going after a failure, reports the first error. ACEs reverted before accounts deleted (orphaned-residue prevention).

Verification

• GOOS=windows go vet ./internal/sandbox/... — clean
• GOOS=windows go test -c — compiles (type-checks all Windows surface)
• go build ./internal/sandbox/... (Linux) — clean
• go test ./internal/sandbox/ (Linux, from non-/tmp path) — pass
• go vet ./internal/sandbox/... — clean
• Stacked on #808: git merge-base --is-ancestor pr808 pr812 confirmed by gnanam; zero #808 commits missing.

Cost noted in the PR

Two accounts per workspace instead of one. Doubles what an operator sees in net user. The PR explicitly says this is a deliberate trade for making network denial work under a separate identity, and that the endpoint-protection/enterprise-policy product question belongs to the stack (both #808 and this) rather than to this PR alone.

What's not verified (same caveats as #808)

The privileged half is still unexercised: LogonUser, NetUserAdd, LsaAddAccountRights need an elevated Windows box. Three new tests haven't run locally (Smart App Control blocked the test binary). CI is the check. The feature is behind ZERO_WINDOWS_SANDBOX_IDENTITY=1, off by default.

Verdict

Approve. The design closes the network-denial gap in #808 correctly, the two load-bearing orderings (plan after provisioning, role tag before hash) are pinned by tests, and the ownership-before-delete check is the right resolution to gnanam's #808 finding. Merge order is #808 then this.

Stale — superseded by later APPROVED reviews on the latest head (011b26d). gnanam1990 APPROVED at 15:39:03Z; anandh8x APPROVED at 15:42:35Z. Merge state is CLEAN.

[anandh8x]

Reviewed the latest stacked head 011b26d against #808. The offline/online split and WFP group design are sound, but this branch adds a blocking rollback hole.

[P2] internal/sandbox/windows_identity_windows.go:537-540 — after a new offline account has been created and added to the managed group, failure while adding it to ZeroSandboxOffline returns created=true with an empty identity. provisionWindowsSandboxPrincipalForSetup then runs its cleanup with identity.Username empty, so the account this run just created is left behind. This is especially relevant here because group membership is the enforcement boundary and can fail under local policy. Return enough identity for rollback or clean up within provisionWindowsSandboxIdentity, and add a failure-path test. This should be resolved together with the analogous post-creation paths noted on #808.

[Vasanthdev2004]

Taken, in 5db0e6ef. You were right that this and the #808 paths are the same bug, so I fixed it there and rebased this branch on top rather than patching the two independently.

The offline-group step now returns the account name with its error, same as the other post-creation failures. It is the one that matters most here for the reason you gave: membership IS the network denial, local policy can refuse a group join, and the rollback deleting "" would have left behind a principal that has network access and no offline membership. That is a worse residue than an ordinary orphan.

Covered by an injected failure alongside the other two paths. Reverting just that return fails only the offline case:

--- FAIL: .../offline_group_attachment_fails
    identity carries no username, so the rollback deletes "" and strands the account

One thing the rebase turned up that is worth flagging, because it would not have been obvious in review. windows_identity_runtime_windows.go merged cleanly and did not compile: it still called windowsSandboxUserName(key) while this branch makes the name role-aware. Had it compiled it would have been the quieter bug, since the secret path would have been derived from the wrong role's account name, so the undo would have removed the other principal's secret. Fixed as part of the rebase, and the build catches it.

Also seamed ensureWindowsSandboxOfflineGroup here. Without it the new test never reached the offline step on an unelevated machine and passed for the wrong reason, which the assertion caught.

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

internal/sandbox/windows_identity_rollback_windows_test.go (1)

145-164: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Assert that the read ACL remains a grant.

mode is discarded, so DENY_ACCESS or an empty mask would still pass as long as no write bits are present. Assert mode == windows.GRANT_ACCESS and at least one contractually required read bit.

🤖 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/sandbox/windows_identity_rollback_windows_test.go` around lines 145
- 164, Update TestWindowsACLAllowReadGrantsNoDelete to retain the mode returned
by windowsACLAccess and assert it equals windows.GRANT_ACCESS. Also assert that
the mask contains at least one contractually required read-access bit, while
preserving the existing checks that forbid delete and write bits.

🤖 Prompt for all review comments with 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.

Inline comments:
In `@internal/sandbox/windows_identity_rollback_windows_test.go`:
- Around line 18-26: Update the test setup and cleanup around the
pre-existing-account regression test to save, stub, and restore
windowsSandboxUserIsManaged and resetWindowsSandboxUserPassword alongside the
existing seams. Configure the stubs so execution reaches the injected group
failure, then assert that injected error is returned, ensuring the test is
deterministic and does not modify real account state.

---

Outside diff comments:
In `@internal/sandbox/windows_identity_rollback_windows_test.go`:
- Around line 145-164: Update TestWindowsACLAllowReadGrantsNoDelete to retain
the mode returned by windowsACLAccess and assert it equals windows.GRANT_ACCESS.
Also assert that the mask contains at least one contractually required
read-access bit, while preserving the existing checks that forbid delete and
write bits.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: bd266c61-0a6f-4795-88c7-7d55ece45da1

📥 Commits

Reviewing files that changed from the base of the PR and between 011b26d and 5db0e6e.

📒 Files selected for processing (8)

• internal/sandbox/windows_identity_rollback_windows_test.go
• internal/sandbox/windows_identity_runtime_windows.go
• internal/sandbox/windows_identity_runtime_windows_test.go
• internal/sandbox/windows_identity_windows.go
• internal/sandbox/windows_identity_windows_test.go
• internal/sandbox/windows_network.go
• internal/sandbox/windows_network_test.go
• internal/sandbox/windows_setup_windows.go

🚧 Files skipped from review as they are similar to previous changes (7)

• internal/sandbox/windows_identity_runtime_windows.go
• internal/sandbox/windows_network.go
• internal/sandbox/windows_setup_windows.go
• internal/sandbox/windows_network_test.go
• internal/sandbox/windows_identity_runtime_windows_test.go
• internal/sandbox/windows_identity_windows.go
• internal/sandbox/windows_identity_windows_test.go

[Vasanthdev2004]

@coderabbitai review

Rebased onto the updated #808 head. The offline-group failure path now returns the account name for rollback, and ensureWindowsSandboxOfflineGroup is seamed so the failure path is actually reachable in a test.

[coderabbitai]

@Vasanthdev2004 I’ll review the rebased changes, with particular attention to rollback ownership, offline-group membership failure handling, and the test seam’s coverage of that path.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[jatmn]

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

Findings

• [P2] Scope dual-role setup rollback to accounts this run created
  internal/sandbox/windows_identity_runtime_windows.go:242-266
provisionWindowsSandboxPrincipalForSetup already avoids deleting a pre-existing principal on failure (created=false in its inner undo), but setupWindowsSandboxPrincipal appends an unconditional removeWindowsSandboxPrincipalForSetup for every role that completed before a later step fails. If offline provisioning succeeds on an account that already existed and online provisioning or ACL application then fails, the outer rollback deletes the working offline principal anyway. That turns a partial re-setup failure into loss of a principal that was fine before the run. The single-principal path on #808 had a similar outer-rollback shape; dual-role provisioning makes this bite more often. Please mirror the created contract in the outer rollback, or only append removal closures for principals this run actually created.

• [P3] Update the mode-independent infra-plan test for the offline group
  internal/sandbox/windows_online_offline_test.go:59
TestBuildWindowsNetworkInfraPlanIsModeIndependent still requires exactly one identity SID. On a Windows host where ZeroSandboxOffline already exists, BuildWindowsNetworkInfraPlan now legitimately returns two SIDs, so this test fails even though the plan is correct. CI stays green because Linux/macOS jobs leave the hook nil and fresh Windows runners usually have no group yet. Please stub resolveWindowsSandboxOfflineGroupSIDHook the way the new network tests do, or relax the assertion to match the new contract.

Needs maintainer decision

• Machine-global offline group and per-workspace setup markers
  internal/sandbox/windows_network.go:79-87 and internal/sandbox/windows_setup.go:182-265
ZeroSandboxOffline is machine-global by design, and BuildWindowsNetworkInfraPlan folds its SID into NetworkInfraHash whenever the group exists. That means identity-enabled setup in one workspace changes the expected infra hash for every other sandbox home on the machine, so their markers read as out of date until each re-runs elevated setup, even if they never opted into the principal backend. That is a predictable consequence of the chosen design, not an accidental bug. Please confirm this cross-workspace coupling is acceptable for the experimental flag, and document it if so; otherwise keep the infra hash independent of a group another workspace created.

[jatmn]

Re-reviewed at c4edd17 on feat/windows-sandbox-offline-online, stacked on feat/windows-sandbox-identity at 71ad0f1. Merge is clean and the branch now contains the base fixes I asked for in the earlier round (a1a0edc logon-rights guard, secret permission fail-soft, dual-role rollback scoped to created, host-independent infra-plan assertions). I still have one hardening item I would like addressed; the rest are compatibility or polish.

What I withdraw from my earlier review

Merge/rebase blocker. GitHub now reports mergeable: true / mergeable_state: clean, and 71ad0f1 is an ancestor of this head. The seam conflicts I called out against the older base are resolved here.

Outer dual-role rollback scoped to created. setupWindowsSandboxPrincipal only appends removal for principals this run created, and TestDualRoleSetupRollbackSparesAdoptedPrincipals exercises the adopted-offline / created-online failure path I cared about.

Mode-independent infra-plan test. The offline-group hook is pinned, group-present SIDs are asserted by value, and the cross-workspace hash coupling is documented in-test rather than hidden.

#808 rollback regressions. rightsAttempted && created is present in provisionWindowsSandboxPrincipalForSetup's undo(), and readWindowsSandboxSecret maps os.IsPermission to errWindowsSandboxIdentityUnavailable.

Coverage guard on groupErr as a P2. I rechecked this: BuildWindowsNetworkInfraPlan and the setup guard both call the same resolver, and provisioning creates the group immediately before the plan is built. A lookup failure at the guard after a successful plan build would need a transient error between two back-to-back calls. The groupErr == nil gate is a real defensive gap in the source, but not a practical failure mode on the happy path. I am keeping it as P3 below.

Findings

• [P2] An adopted offline principal can survive provisioning without ZeroSandboxOffline membership
  internal/sandbox/windows_identity_windows.go:711-714
internal/sandbox/windows_identity_runtime_windows.go:344-346
  If offline provisioning adopts an existing managed account and addWindowsSandboxUserToOfflineGroup then fails, provisioning returns created=false, so neither the inner nor outer rollback removes the account. That is the right data-loss tradeoff, but deny-mode commands can still log on as that principal when a readable secret remains from a prior run. The token is not in the offline group the WFP filters key to, so network denial may not apply. Membership is the enforcement boundary in this design.
  This is a narrow edge case, not the common path: first-time create plus a failed offline join still deletes the account (created=true), and re-setup usually succeeds the group join because addWindowsSandboxUserToOfflineGroup treats "already a member" as success. The scenario that bites is re-setup adopting an existing offline account, a policy refusal on the offline-group join, and a secret that still authenticates. Please either refuse to leave an offline-role principal without offline-group membership after provisioning, or fail closed at command time when the offline principal is not a member of ZeroSandboxOffline.

• [P3] No migration path from #808 single-principal account names
  internal/sandbox/windows_identity_windows.go:189-206
internal/sandbox/windows_identity_runtime_windows.go:107-112
  This is an intentional rename, not an accidental regression: #808 derived zero-sbx-<hash> and this branch derives zero-sbx-d<hash> / zero-sbx-n<hash>. There is no adopt/rename/cleanup of the legacy untagged name. Stack testers who upgrade without elevated re-setup will miss both role-tagged lookups and silently fall back to the restricted-token backend; re-setup then creates two new accounts while the old one and its secret remain orphaned. That is real compatibility debt for in-flight testers, but not a logic bug and not a main release concern yet. A migration path or a one-time stderr hint when a legacy account exists but role-tagged lookup misses would be enough if you want to support people already on the stacked branch.

• [P3] Setup skips the principal-coverage guard when offline-group lookup errors
  internal/sandbox/windows_setup_windows.go:80-90
  After principals are provisioned, resolveWindowsSandboxOfflineGroupSID() errors are ignored and WindowsNetworkPlanCoversPrincipals is not run, yet applyWindowsNetworkPlan still proceeds. On the identity-enabled path the plan was just built with the same resolver, so this branch should not fire in normal operation. It is still worth failing setup when groupErr != nil here, or reusing the group SID already folded into networkPlan, rather than silently skipping the last assertion.

• [P3] Partial dual-role setup leaves allow-mode commands on the restricted-token path without warning
  internal/sandbox/windows_identity_runtime_windows.go:329-334
internal/sandbox/windows_identity_runtime_windows.go:109-112
  If setup fails after the offline role completes but before the online role is provisioned, allow-mode commands look up the online principal, get errWindowsSandboxIdentityUnavailable, and fail-soft to the restricted-token backend with no stderr warning (unlike the missing-secret path). First-time setup failures usually leave no marker, so commands should not run anyway; this mostly matters on re-setup or other inconsistent machine state. Consider warning once when opt-in is set, the offline principal resolves, but the online principal does not.

• [P3] Teardown treats a name collision as a hard error after the secret is already removed
  internal/sandbox/windows_identity_runtime_windows.go:399-404
internal/sandbox/windows_identity_runtime_windows.go:410-412
  When lookupWindowsSandboxIdentity returns errWindowsSandboxNameCollision, removeWindowsSandboxPrincipalForSetup aborts before removeWindowsSandboxIdentity, which would return errWindowsSandboxForeignAccountRetained and is already treated as success downstream. The secret is gone but rollback/teardown reports failure for correctly declining to delete somebody else's account. Please fall through to removeWindowsSandboxIdentity (or treat collision like foreign-retained) after secret removal.

Needs maintainer decision

• Machine-global ZeroSandboxOffline changes every sandbox home's NetworkInfraHash
internal/sandbox/windows_network.go:79-86
internal/sandbox/windows_setup.go:264-265
  This is deliberate, documented behavior rather than drift: the test asserts the hash changes when the offline group appears. I am not asking you to resolve it in this PR. @kevincodex1 still needs to decide whether cross-workspace marker invalidation is acceptable for the experimental flag and document it for operators, or whether the infra hash should stay independent of a group another workspace created. I do not think it should block the rest of this change.

[Vasanthdev2004]

Rebased onto the updated #808 (08ae5915, base 2ce52aa5). Conflicting → mergeable again, base is a clean ancestor, five commits replayed.

Five hunks needed resolving and three were more than textual, so recording what I decided:

The command-path lookup. #808 added a privileged-group re-check on the path that mints the token; this branch added the role parameter. Both survive — lookupWindowsSandboxPrincipalForCommand(key, role) now carries the role through to lookupWindowsSandboxIdentity. Neither change is worth much without the other: the re-check would gate the wrong principal, or the role would skip the check.

The ACL step inside the role loop. #808 replaced the bare applyWindowsACLPlan with applyWindowsPrincipalACLs, which revokes this trustee's existing ACEs before applying. I kept that inside the loop rather than reverting to the plain apply, so the revocation is per role — the two principals are separate trustees and retiring one must not disturb the other's ACEs.

A duplicate seam. Both branches independently added applyWindowsACLPlanFn. I removed this branch's copy and kept #808's, in the var block with the other identity seams. Two declarations of one seam is the kind of thing that lets a test stub one while production uses the other.

One test needed a real change, not a mechanical one. TestDualRoleSetupRollbackSparesAdoptedPrincipals injected its ACL failure on the second call. applyWindowsPrincipalACLs makes two calls per role now (revoke, then grant), so that fired on the first role's grant instead of the second role — the suite went red and, more to the point, would have been asserting something other than what the test is named for. It now counts grant plans and skips revocations, which is the "second ROLE fails" case it was written for.

gofmt, go vet ./..., go build ./... clean; internal/sandbox green. @jatmn @anandh8x this is ready for another look whenever #808 settles — the six findings from your last round on #808 are answered over there.

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (2)

internal/sandbox/windows_identity_runtime_windows.go (1)

396-417: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Teardown still hard-fails on a name-collision lookup instead of falling through to the foreign-account path.

This exact code and line range were flagged in a prior review round and the comment here has no "resolved" marker — tracing the current code confirms it's still present:

if identity, err := lookupWindowsSandboxIdentity(key, role); err == nil {
    ...
} else if !errors.Is(err, errWindowsSandboxIdentityUnavailable) {
    return err
}

lookupWindowsSandboxIdentity returns errWindowsSandboxNameCollision (a distinct sentinel from errWindowsSandboxIdentityUnavailable) when the derived account name belongs to someone else. That's exactly the case removeWindowsSandboxIdentity a few lines below is designed to absorb gracefully via errWindowsSandboxForeignAccountRetained. As written, teardown returns the collision error immediately and never reaches that graceful path, contradicting this PR's own stated goal that teardown "leaves unmanaged or foreign accounts untouched" as a successful cleanup outcome.

🐛 Proposed fix

-	} else if !errors.Is(err, errWindowsSandboxIdentityUnavailable) {
+	} else if !errors.Is(err, errWindowsSandboxIdentityUnavailable) && !errors.Is(err, errWindowsSandboxNameCollision) {
 		return err
 	}

🤖 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/sandbox/windows_identity_runtime_windows.go` around lines 396 - 417,
Update the teardown lookup error handling around lookupWindowsSandboxIdentity so
errWindowsSandboxNameCollision follows the same fall-through path as
errWindowsSandboxIdentityUnavailable instead of being returned. Preserve
returning all other lookup errors, allowing removeWindowsSandboxIdentity to
handle the foreign-account case through errWindowsSandboxForeignAccountRetained.

internal/sandbox/windows_setup_windows.go (1)

80-90: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Offline-group SID lookup error is still silently swallowed here.

This is the same code and line range flagged in a prior review round, and the comment carries no "resolved" marker — the current code confirms it's unchanged:

if groupSID, groupErr := resolveWindowsSandboxOfflineGroupSID(); groupErr == nil {
    if !WindowsNetworkPlanCoversPrincipals(networkPlan, groupSID) { ... }
}

Every other failure in this function (ACL plan build/apply, principal provisioning, plan build, marker write) reports the error and rolls back. Here, if groupErr != nil, the coverage assertion is skipped entirely and setup proceeds straight to applyWindowsNetworkPlan without ever checking that the just-installed filters actually cover the just-provisioned offline principal. BuildWindowsNetworkInfraPlan resolved this same SID successfully a few lines above via the same underlying call through the hook, so the window is narrow, but a transient failure on the second call should fail setup loudly — consistent with every other step here — rather than silently downgrading to "unchecked."

🐛 Proposed fix

-	if groupSID, groupErr := resolveWindowsSandboxOfflineGroupSID(); groupErr == nil {
-		if !WindowsNetworkPlanCoversPrincipals(networkPlan, groupSID) {
-			err := errors.New("network block filters do not name the sandbox offline group, so they would not apply to any sandbox principal; the network plan must be built after principals are provisioned")
-			if rollbackErr := rollback(); rollbackErr != nil {
-				fmt.Fprintf(stderr, "%s: %v; rollback failed: %v\n", WindowsSandboxSetupName, err, rollbackErr)
-				return 1
-			}
-			fmt.Fprintln(stderr, WindowsSandboxSetupName+": "+err.Error())
-			return 1
-		}
-	}
+	groupSID, err := resolveWindowsSandboxOfflineGroupSID()
+	if err != nil {
+		if rollbackErr := rollback(); rollbackErr != nil {
+			fmt.Fprintf(stderr, "%s: %v; rollback failed: %v\n", WindowsSandboxSetupName, err, rollbackErr)
+			return 1
+		}
+		fmt.Fprintln(stderr, WindowsSandboxSetupName+": "+err.Error())
+		return 1
+	}
+	if !WindowsNetworkPlanCoversPrincipals(networkPlan, groupSID) {
+		err := errors.New("network block filters do not name the sandbox offline group, so they would not apply to any sandbox principal; the network plan must be built after principals are provisioned")
+		if rollbackErr := rollback(); rollbackErr != nil {
+			fmt.Fprintf(stderr, "%s: %v; rollback failed: %v\n", WindowsSandboxSetupName, err, rollbackErr)
+			return 1
+		}
+		fmt.Fprintln(stderr, WindowsSandboxSetupName+": "+err.Error())
+		return 1
+	}

🤖 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/sandbox/windows_setup_windows.go` around lines 80 - 90, Handle
errors returned by resolveWindowsSandboxOfflineGroupSID in the Windows sandbox
setup flow instead of silently skipping coverage validation. When the lookup
fails, report the error to stderr, invoke rollback, and return failure
consistently with the other setup steps; only call
WindowsNetworkPlanCoversPrincipals after a successful SID lookup.

🤖 Prompt for all review comments with 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.

Inline comments:
In `@internal/sandbox/windows_identity_windows.go`:
- Around line 259-294: Update ensureWindowsSandboxOfflineGroup and the
underlying ensureWindowsLocalGroup flow to verify that an existing
ZeroSandboxOffline group has the expected windowsSandboxOfflineGroupComment
ownership marker before adopting it. Reject name collisions with unrelated
groups, while preserving creation and successful reuse of groups that carry the
matching ownership comment.

---

Duplicate comments:
In `@internal/sandbox/windows_identity_runtime_windows.go`:
- Around line 396-417: Update the teardown lookup error handling around
lookupWindowsSandboxIdentity so errWindowsSandboxNameCollision follows the same
fall-through path as errWindowsSandboxIdentityUnavailable instead of being
returned. Preserve returning all other lookup errors, allowing
removeWindowsSandboxIdentity to handle the foreign-account case through
errWindowsSandboxForeignAccountRetained.

In `@internal/sandbox/windows_setup_windows.go`:
- Around line 80-90: Handle errors returned by
resolveWindowsSandboxOfflineGroupSID in the Windows sandbox setup flow instead
of silently skipping coverage validation. When the lookup fails, report the
error to stderr, invoke rollback, and return failure consistently with the other
setup steps; only call WindowsNetworkPlanCoversPrincipals after a successful SID
lookup.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 0c448553-a210-41db-a54a-d1392dbf9b16

📥 Commits

Reviewing files that changed from the base of the PR and between c4edd17 and 08ae591.

📒 Files selected for processing (12)

• internal/sandbox/windows_dualrole_rollback_windows_test.go
• internal/sandbox/windows_identity_policy_windows_test.go
• internal/sandbox/windows_identity_privilege_recheck_windows_test.go
• internal/sandbox/windows_identity_rollback_windows_test.go
• internal/sandbox/windows_identity_runtime_windows.go
• internal/sandbox/windows_identity_runtime_windows_test.go
• internal/sandbox/windows_identity_windows.go
• internal/sandbox/windows_identity_windows_test.go
• internal/sandbox/windows_network.go
• internal/sandbox/windows_network_test.go
• internal/sandbox/windows_online_offline_test.go
• internal/sandbox/windows_setup_windows.go

[jatmn]

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

Findings

• [P1] Restore an adopted principal's pre-existing ACLs when setup rolls back
  internal/sandbox/windows_identity_runtime_windows.go:361
applyWindowsPrincipalACLs first removes every existing ACE for the principal, but deliberately discards that revoke operation's snapshot. This branch now retains adopted principals on a later setup failure: if the offline role is adopted, its ACLs are refreshed here, and provisioning or ACL application for the online role then fails, rollback() runs only revertACL, whose snapshot is already after the revocation. The account and secret survive but the ACEs that made the previously working principal usable do not, so subsequent deny-mode commands log on and then lose their workspace/runtime access. Preserve and restore the pre-refresh ACL state for retained principals (or defer changing an adopted principal until the full dual-role setup can succeed).

• [P1] Reject a pre-existing offline group that is not owned by Zero
  internal/sandbox/windows_identity_windows.go:262
ensureWindowsSandboxOfflineGroup delegates to ensureWindowsLocalGroup, which accepts NERR_GroupExists/ERROR_ALIAS_EXISTS without checking the group's comment or other ownership marker. Setup then resolves that arbitrary group's SID and installs it on the persistent WFP deny filters. If another tool or policy already owns ZeroSandboxOffline, every existing member of that group can abruptly lose outbound access, and the sandbox principal also inherits that group's permissions. Verify the expected managed-group marker before reusing the group and fail on a foreign same-name group.

• [P2] Fail closed when the selected offline principal is no longer in the offline group
  internal/sandbox/windows_identity_runtime_windows.go:106
  Network denial now depends entirely on ZeroSandboxOffline membership, but the command path only validates the account's ownership and privilege groups before logging it on. If an existing offline account loses that membership through local policy or administrative drift (and a re-setup then cannot re-add it), the prior secret and setup marker remain usable; a NetworkDeny command selects the account and its token no longer matches the WFP group condition, so it gets egress. Recheck the selected role's required membership at token minting and refuse/fall back to the restricted-token path when it is absent; cover the failed re-setup case.

[Vasanthdev2004]

@jatmn all three addressed in 7498e9d1, 396fce78, e87133b2. Each has a regression test I confirmed fails without its fix.

P1, adopted principal's ACEs on rollback. You were right about the premise: the "discard the revocation's restore" comment justified itself on "the only failure path from here removes the principal outright", and dual-role setup is exactly what broke that — an adopted principal is deliberately retained. revokeWindowsPrincipalACEs now returns its restore and applyWindowsPrincipalACLs hands back both, revocation first. Setup appends the revocation restore before the grant restore so the reverse unwind runs grant-then-revocation; since each snapshot is a whole DACL, that ordering is what walks a path back one operation at a time rather than clobbering the other role's ACEs. A grant failure undoes its own revocation inline, since the caller never gets a handle to it. Teardown still discards — there the account really is going away.

P1, foreign offline group. The add now reports its raw status and the already-exists branch verifies the managed marker before adopting, failing with an actionable message otherwise. A lookup error fails closed. I put NetLocalGroupAdd and the ownership lookup behind seams so the branch is reachable without an elevated machine, and the marker compared is the group's own — the principals group and the offline group can't be confused for each other.

P2, membership at token minting. Agreed this is the sharp one: membership is the enforcement, since LogonUser builds the token from the account's real memberships and the filters match the group SID. The offline role now confirms membership before the secret is read. I went with fall-back rather than refuse — the restricted token carries the offline marker the same filters match, so egress stays blocked and only read confinement is lost, which seemed better than failing the command outright. A failed lookup surfaces instead of downgrading. The online role isn't checked, since it's not in that group by design. Warning is once-per-process like its sibling.

Two seams I added along the way that are worth a look: readWindowsSandboxSecretFn and lookupWindowsSandboxPrincipalForCommandFn, purely so the command path's gates are testable without a provisioned machine.

gofmt, go vet and the full internal/sandbox suite are clean on Windows here. Ready for another pass.

[jatmn]

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

Merge conflict

This PR is currently marked CONFLICTING with its target branch (feat/windows-sandbox-identity). Please rebase it onto the current target and rerun the Windows build/tests on the merged tree; this stack has already demonstrated that a clean textual rebase can leave a broken call site.

Findings

• [P1] Do not grant FILE_DELETE_CHILD to write roots
  internal/sandbox/windows_acl_apply_windows.go:244
  A WindowsACLAllowWrite ACE is inherited from a writable root, while the protected Git/config and hooks carve-outs are denied on children. FILE_DELETE_CHILD on the parent authorizes deleting a child regardless of that child's DACL, so the sandbox principal can delete .git/config or .git/hooks, recreate it under the inherited allow ACE, and then set credential.helper or core.hooksPath. This bypasses the control-plane restriction; the base implementation deliberately omitted this bit for exactly that reason. Keep DELETE for ordinary allowed-file replacement, but do not grant FILE_DELETE_CHILD from the parent.

• [P1] Restore the ancestor reparse-point check before elevated ACL writes
  internal/sandbox/windows_acl_apply_windows.go:180
FILE_FLAG_OPEN_REPARSE_POINT only prevents following the final path component. Removing verifyWindowsACLTargetNotRedirected means a workspace owner can turn an ancestor such as .git into a junction before elevated setup; opening .git\\hooks then lands on an external non-reparse target and setup rewrites that target's DACL as Administrator. The deleted helper and test covered this exact junction-ancestor path, so retain an equivalent final-handle/canonical-path verification.

• [P1] Keep fallback runtime roots out of the principal ACL path
  internal/sandbox/runtime_state.go:48
  When the user cache is inside the workspace, this now calls fallbackSandboxRuntimeRoot, which uses MkdirTemp and only memoizes the result in-process. Elevated setup grants the principals access to temporary root A, but a later command is a new process and chooses root B for HOME and caches, where the principal has no ACE; ordinary cache writes then fail with ACCESS_DENIED. Teardown also creates and targets a third temporary directory rather than the real one. Restore the side-effect-free deterministic derivation (and omit the random fallback where it cannot be named reliably) instead of using the command-time fallback resolver for setup/teardown.

[Vasanthdev2004]

@jatmn rebased onto the current #808 head — new head is 90e2556c, and the CONFLICTING state is gone. Worth re-reviewing against this rather than the old head, because two of your three P1s are closed by the rebase itself rather than by new code.

Closed without a code change:

• FILE_DELETE_CHILD granted to write roots — windows_acl_apply_windows.go:229 on the old head read "DELETE and FILE_DELETE_CHILD are part of the grant". It now reads "FILE_DELETE_CHILD is deliberately NOT granted", because that fix already lived on feat(sandbox): Windows sandbox principals (foundation for #662, does not close it) #808 and this branch had forked three commits before it.
• Missing ancestor reparse check — verifyWindowsACLTargetNotRedirected was absent here and is present now, same reason. feat(sandbox): Windows sandbox principals (foundation for #662, does not close it) #808 also gained dd3520a6 since your review, which fixes the ordering half you flagged there: materializeWindowsACLTarget no longer uses os.MkdirAll, so nothing is created before the ancestors are verified.

So both were real on the head you reviewed, and both were artifacts of a stale base — which is exactly the failure mode you called out, and the reason you asked for the rebase first.

Still open and mine to fix: the runtime-root P1 (runtime_state.go:48). Your read is right — fallbackSandboxRuntimeRoot uses MkdirTemp memoized only in-process, so elevated setup grants an ACE on temp root A and a later command, being a new process, picks root B and gets ACCESS_DENIED on ordinary cache writes. I have not fixed it yet; I want to restore a deterministic side-effect-free derivation rather than paper over it, and I would rather do that carefully than quickly.

On the mechanics, in case it is useful for the rest of the stack: I did this as a per-commit rebase rather than a merge. The merge produced 77 conflict hunks across 11 files with neither side a clean winner; the rebase produced four small conflicts. One of them was informative — my commit 7498e9d1 (restoring an adopted principal's ACEs on rollback) turned out to duplicate a fix already on #808 that composes both restores into a single returned func, so I dropped mine and kept yours. Same bug, fixed twice on two branches.

Verified rather than assumed, since you warned that a clean textual rebase can still leave a broken call site — and it did: a test stub kept the old arity and only go vet caught it. Current state has zero commits of #808 or main missing, all five load-bearing guards confirmed present, and internal/sandbox green locally. CI is still running as I write this.

[Vasanthdev2004]

@jatmn the third one is done as well — head is now b88ac469, so all three P1s on this PR are closed.

Runtime roots (runtime_state.go:48). Your diagnosis was exactly right, including the detail that teardown was targeting a third directory. The cause was that windowsSandboxRuntimeRootPath resolved through sandboxRuntimeRootFor, which falls back to os.MkdirTemp and memoizes only in-process, while windowsPrincipalTeardownPaths already used deterministicSandboxRuntimeRoot. Three processes, three different answers, for a value whose own doc comment says both callers have to agree exactly.

Setup now uses that same deterministic derivation, and when it is unusable it reports no runtime root instead of inventing one. That is the part worth your eye: I took "omit the random fallback where it cannot be named reliably" literally rather than trying to make the fallback stable. The principal then gets no ACE on the runtime tree, which is a degraded sandbox — but a root only the granting process can name is worse, because the sandbox looks provisioned while every later command fails on a bare ACCESS_DENIED.

One thing I want to flag rather than let you find: I inverted an existing test. TestTeardownPathDerivationCreatesNothing asserted that "setup's resolver should still fall back to a usable tree", which is precisely the assumption your finding overturns. Changing a test so a fix passes is normally a smell, so the reasoning is recorded in the test body and the commit message, and I added TestSetupAndTeardownDeriveTheSameRuntimeRoot to pin the ordinary case — otherwise "report none" could quietly become the answer everywhere and nothing would notice.

Both directions are pinned: restoring the fallback fails with the invented path in the message (...\Temp\zero-runtime-829337130\runtime) and a count of the temp entries it created.

Where the other two landed, since they were closed by the rebase rather than by code: FILE_DELETE_CHILD is no longer granted, and verifyWindowsACLTargetNotRedirected is present again — both were live on the head you reviewed and both were artifacts of this branch having forked three commits before those fixes landed on #808.

internal/sandbox is green locally and gofmt/vet are clean. CI has four jobs still running and nothing failing yet. Ready for another pass whenever you have time.

[jatmn]

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

Findings

• [P1] Keep the principal write jail
  internal/sandbox/windows_command_runner_windows.go:93
  This now executes the raw LogonUser token after deleting the CreateRestrictedToken/WRITE_RESTRICTED wrapping. The principal ACL plan only adds ACEs for configured paths; it cannot remove writes inherited through ambient memberships such as BUILTIN\\Users and batch logon. Consequently an identity-enabled sandbox whose only WriteRoot is its workspace can write to an unrelated user-writable location such as C:\\Users\\Public\\Documents. The deleted windows_principal_jail_windows_test.go exercised exactly this boundary. Restore an equivalent write restriction (while retaining the principal SID needed for granted roots) and cover a real principal-token attempt outside the configured roots.

Maintainer Decision

internal/sandbox/windows_network.go:67

Creating ZeroSandboxOffline for workspace A adds its SID to NetworkInfraHash. On the next command in every already-initialized workspace B, ValidateWindowsSandboxSetupMarker recomputes the now-different hash and rejects the command as “network infrastructure changed,” requiring elevated setup for each unrelated workspace. The test intentionally pins this behavior and the author has explicitly left it for maintainer resolution. Please decide whether that availability break is acceptable for the experimental flag, or whether the global group must be made compatible with existing per-home markers.

• [P2] Fail setup when the post-provisioning group lookup fails
  internal/sandbox/windows_setup_windows.go:80
  The second resolveWindowsSandboxOfflineGroupSID call is used to enforce the load-bearing coverage assertion, but its error is silently ignored. This contradicts the stated fail-closed behavior: a transient lookup failure, group deletion, or replacement after planning lets setup install filters and record success without confirming that they cover the just-provisioned offline principal. Roll back and report groupErr before applying the network plan; this is also an unresolved CodeRabbit request on the current head.

• [P3] Let foreign-name collisions reach the retained-account cleanup path
  internal/sandbox/windows_identity_runtime_windows.go:447
lookupWindowsSandboxIdentity returns errWindowsSandboxNameCollision when the derived name belongs to a foreign account, but this branch returns that error before removeWindowsSandboxIdentity can classify the same state as the explicitly tolerated errWindowsSandboxForeignAccountRetained. Thus the claimed “teardown treats it as success” behavior does not hold for a collision: rollback/teardown fails instead of converging after correctly leaving the foreign account untouched. Treat the collision like an unavailable identity for the LSA-revocation step and continue to the ownership-checked removal path; this is also an unresolved CodeRabbit request on the current head.