fix(windows): grant secret ACLs to effective token SID

[luvs01]

Summary

• resolve the effective Windows token SID instead of constructing the ACL owner from USERDOMAIN\\USERNAME;
• pass the validated numeric principal to icacls as *S-1-...:(F);
• use trusted System32 PowerShell with hidden, non-interactive sync and async runners bounded by the existing ACL deadline;
• process-cache only a successful lookup and keep identity lookup failures separate from icacls timeouts;
• add WORKGROUP, failure-propagation, cache, async-deduplication, and real Windows token regression coverage.

Fixes #1149.

Why

On workgroup Windows hosts, USERDOMAIN may be the literal WORKGROUP while the effective token belongs to COMPUTER\\user. The previous currentWindowsUser() therefore asked icacls to grant an unmappable WORKGROUP\\user principal and received exit code 52. Required ACL hardening then failed closed even though NTFS and the effective account were valid.

The token SID is locale-independent and is already the authority used by the Codex coordinator identity work from #998. This PR extracts a low-level ACL-specific resolver rather than importing the higher-level coordinator module.

This is independent of #1130/#1135: those handle one retry after an ACL timeout; this fixes principal construction before icacls can succeed.

Security boundary

• PowerShell is resolved from the trusted Windows system directory through GetSystemDirectoryW; PATH and SystemRoot do not select it.
• Both runners use windowsHide: true, -WindowStyle Hidden, -NonInteractive, and -NoProfile.
• Only strict S-1-... output is accepted; raw output, paths, and account identifiers are not included in diagnostics.
• Lookup failure is EACLIDENTITY, not ETIMEDOUT, so it cannot poison the destination ACL timeout memo.
• Failed and invalid lookups are not cached. Concurrent async callers share one owned lookup.

Verification

Post-rebase at 6d04574d:

• Bun 1.3.14: principal + affected ACL suites 166/166 passed.
• Bun 1.4.0-canary.1 (b22e0e6d0): the same suites 166/166 passed.
• tests/server-management-auth.test.ts exceeded its fixed 5-second per-test cap on this host. The unchanged dev baseline reproduced the same six timeouts plus one unrelated unhandled assertion, while this branch had no patch-specific correctness failure. This result is reported as baseline timing instability, not as a pass.
• bun x tsc --noEmit: passed on both runtimes.
• bun scripts/privacy-scan.ts: passed.
• git diff --check: passed.
• Frontend-only lint and doctor gates do not apply because no frontend files changed.
• Independent read-only review found no remaining P0/P1 issue after trusted-executable and identity-failure coverage were added.

Checklist

• Scope stays focused and avoids unrelated cleanup.
• Docs or release notes were updated when needed. (No user-facing configuration or command changed.)
• Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

Draft pending upstream CI and the repository-required human maintainer security review.

Review readiness checklist

This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:

• All CI tests are green on my local testing.

• I pushed my PR to the latest dev commit.

• I resolved all correct Codex and CodeRabbit findings.

• My PR is ready for review.

Summary by CodeRabbit

• Bug Fixes

  • Improved Windows access-control handling by identifying the effective account directly, rather than relying on potentially misleading environment values.
  • Added safer failure handling when account identity cannot be resolved, including clearer diagnostics and prevention of unintended permission changes.
  • Improved consistency and reliability for both synchronous and asynchronous permission operations.

• Tests

  • Added coverage for identity resolution, caching, timeouts, invalid results, and concurrent lookups.
  • Added integration tests verifying correct permissions and fail-safe behavior.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 6c345aba-0c32-4447-b684-d83753dd3be7

📥 Commits

Reviewing files that changed from the base of the PR and between 6d04574 and df6989c.

📒 Files selected for processing (4)

• src/lib/windows-secret-acl.ts
• src/lib/windows-user-principal.ts
• tests/windows-secret-acl.test.ts
• tests/windows-user-principal.test.ts

---

📝 Walkthrough

Walkthrough

Changes

Windows ACL hardening now derives the effective principal from the Windows token SID. A new resolver runs trusted PowerShell synchronously or asynchronously, validates SID output, caches successful results, shares concurrent lookups, enforces timeouts, and reports EACLIDENTITY failures.

Windows ACL identity

Layer / File(s)	Summary
Effective principal resolution src/lib/windows-user-principal.ts, tests/windows-user-principal.test.ts	The new resolver executes WindowsIdentity.GetCurrent().User.Value, normalizes valid SIDs, applies timeout handling, caches successful results, shares asynchronous lookups, and exposes test seams.
SID-based ACL hardening src/lib/windows-secret-acl.ts	Synchronous and asynchronous ACL grants use the resolved SID instead of USERDOMAIN and USERNAME. POSIX Windows-platform tests retain a synthetic SID. Identity failures produce sanitized EACLIDENTITY diagnostics.
ACL integration validation tests/windows-secret-acl.test.ts	Tests cover SID grants, required and optional failure paths, sanitized diagnostics, resolver call counts, timeout-cache behavior, and cleanup.

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

Sequence Diagram(s)

sequenceDiagram
  participant ACLHardening
  participant PrincipalResolver
  participant PowerShell
  participant icacls
  ACLHardening->>PrincipalResolver: Resolve effective Windows SID
  PrincipalResolver->>PowerShell: Run WindowsIdentity.GetCurrent().User.Value
  PowerShell-->>PrincipalResolver: Return SID output
  PrincipalResolver-->>ACLHardening: Return normalized *S-1-... principal
  ACLHardening->>icacls: Apply /grant:r using SID
  icacls-->>ACLHardening: Return ACL result

Loading

Possibly related PRs

• lidge-jun/opencodex#601: Both changes modify Windows ACL hardening and its tests, but this change addresses principal resolution rather than ACL operation ordering.

Suggested reviewers: lidge-jun, wibias

🚥 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 and concisely describes the primary change: granting Windows secret ACLs to the effective token SID.
Linked Issues check	✅ Passed	The changes resolve the effective token SID, validate it, use it in icacls grants, separate identity failures, and add required coverage for issue [#1149].
Out of Scope Changes check	✅ Passed	The implementation and tests remain focused on effective Windows principal resolution and ACL hardening required by issue [#1149].
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Create stacked PR
• Commit on current branch

🧪 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.}

[github-actions]

⚠️ Deterministic hygiene checks failed.

• unsponsored_surface — This changes an authentication, workflow, release-automation, or dependency surface. MAINTAINERS.md requires security review for these; ask a maintainer to apply maintainer-sponsored once they have reviewed it. Paths: src/lib/windows-secret-acl.ts.

[github-actions]

⏳ DRAFT

• review readiness checklist open (1/4 boxes ticked).

What to do

• Tick all four boxes in the PR description once you're done (currently 1/4).

Review readiness checklist

• ⬜ All CI tests are green on my local testing.
• ✅ I pushed my PR to the latest dev commit.
• ⬜ I resolved all correct Codex and CodeRabbit findings.
• ⬜ My PR is ready for review.

1/4 boxes ticked.

This PR stays in draft until every box above is ticked.

[luvs01]

@coderabbitai review

[coderabbitai]

✅ 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.

[lidge-jun]

Thank you — this is adopted, and the design decisions in it held up under review.

Your commit is the first commit of #1216, cherry-picked unchanged with Co-authored-by preserved. I kept every structural choice: extracting a low-level SID resolver rather than reusing the coordinator identity module, reusing resolveTrustedWindowsPowerShellExe() instead of writing a third System32 resolver, keeping the identity failure in its own error class so it cannot poison the icacls timeout memo, and caching only successful lookups. Each of those was independently re-derived here before I saw your PR, and yours got there first.

Three things I layered on top, all of which came out of an independent audit rather than a stylistic preference:

The two most important tests never ran outside Windows. FORCED_NON_WINDOWS_TEST_PRINCIPAL was selected before the injected runner, which made a lookup failure unreachable on POSIX — so the fail-closed case and the timedOutPaths isolation case carried if (process.platform !== "win32") return; and counted as passing while executing nothing. I verified this: they report (pass) on macOS with an empty body. Moving the synthetic value into the resolver as its own seam, with selection order explicit > synthetic > default, lets an injected failure win and both guards come off.

EACLIDENTITY never reached the caller. You added the case to sanitizeDiagnostics, but sanitizedAclError re-attaches only allow-listed codes and that one is not on the list, so a required: true harden threw with the cause in the message and error.code === undefined. The test matched /EACLIDENTITY/ against the message, which hid it.

I tried to add back a qualified name fallback for the optional path and was wrong. My reasoning was that DOMAIN\\User is already qualified so it avoids the ambiguity problem. The audit rejected it: shape is not evidence of the token's subject, both variables are attacker-writable, and runIcacls grants Full Control before removing inheritance — so a wrong principal either leaves another account holding the secret or strands the file with no usable ACE. Your original decision to decline entirely was the correct one, and it is what shipped.

Closing in favor of #1216. The credit for the fix is yours.