fix(windows): grant owner ACE before ACL inheritance removal

[Wibias]

Summary

• Fix #596: Windows ACL hardening could strand atomic-write temps (auth.json / config.json / responses-state.json .tmp) with a protected zero-ACE DACL after /inheritance:r ran and a later icacls step timed out.
• Reorder runIcacls() to grant current-user Full Control first, then /inheritance:r, then broad-SID /remove:g (with existing /findsid verification).
• Invariant: if grant fails, inheritance is untouched; if a later step fails/times out, the writer still has explicit Full Control for scrub/unlink.

Why

/inheritance:r is destructive (drops inherited ACEs immediately). Microsoft/docs + field reports agree an empty DACL denies ordinary access; owner implicit rights are not enough for reliable cleanup. Related: #160 (timeout honesty), #499 / #502 (stale-temp recovery — complementary, cannot open a zero-ACE temp).

Test plan

• bun test tests/windows-secret-acl.test.ts — 28 pass (success-order + remove:g-timeout owner-ACE + grant-first EPERM)
• bun run typecheck
• bun run privacy:scan
• CI green (win/mac/ubuntu)
• CodeRabbit + Codex review threads resolved

Closes #596

Summary by CodeRabbit

• Bug Fixes
  • Improved Windows secret-file permission hardening by granting the current user full control before changing inheritance or removing broad permissions.
  • Prevented subsequent permission changes when the initial access grant fails.
  • Added clearer timeout diagnostics while preserving the owner’s access if later cleanup steps fail.

[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: e3766a32-20c9-4523-ae1b-2a06963804b4

📥 Commits

Reviewing files that changed from the base of the PR and between 285e2bf and 26d4c14.

📒 Files selected for processing (2)

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

---

📝 Walkthrough

Walkthrough

Windows ACL hardening now grants the current user Full Control before disabling inheritance and removing broad grants. Tests cover command ordering, grant failures, and timeout behavior during broad-grant removal.

Changes

Windows ACL hardening

Layer / File(s)	Summary
Owner-first ACL command sequence src/lib/windows-secret-acl.ts	runIcacls grants current-user Full Control before /inheritance:r and broad SID removal, while comments document the updated order.
ACL ordering and failure regressions tests/windows-secret-acl.test.ts	Tests verify owner-first execution, halt after grant failure, and retain explicit owner access when /remove:g times out.

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

Sequence Diagram(s)

sequenceDiagram
  participant runIcacls
  participant icacls
  participant WindowsTempFile
  runIcacls->>icacls: grant current-user Full Control
  icacls->>WindowsTempFile: establish explicit owner ACE
  runIcacls->>icacls: remove inheritance
  icacls->>WindowsTempFile: protect inherited ACL state
  runIcacls->>icacls: remove broad grants and verify with /findsid

Loading

Suggested reviewers: ingwannu, lidge-jun

🚥 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 states the main change: granting owner ACE before removing inheritance on Windows.
Linked Issues check	✅ Passed	The PR matches #596 by changing ACL order, preserving owner Full Control, and adding success/failure regressions.
Out of Scope Changes check	✅ Passed	Changes stay within the ACL hardening fix and its regression tests; no unrelated scope is introduced.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

[Wibias]

@codex review

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Another round soon, please!

Reviewed commit: 26d4c14f2c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[Wibias]

Review verdict: merge-ready

Context

Implements the fix proposed in #596: grant current-user Full Control before /inheritance:r so a mid-sequence icacls timeout cannot leave a protected zero-ACE DACL on atomic-write temps. Matches Windows/icacls guidance that /inheritance:r is destructive and an empty DACL denies ordinary access. Complements #160 (timeout honesty) and #499/#502 (stale-temp recovery cannot open a zero-ACE file).

Issue thread had no separate maintainer comment beyond the detailed reporter proposal (which this PR follows).

Bots

Bot	Result
CodeRabbit	Review completed — no actionable comments
Codex	@codex review → no major issues (26d4c14f)
Open review threads	0

CI

All green: ubuntu / macos / windows, npm-global ×3, react-doctor, enforce-target, label.

Bug review

No shippable defects. Order is grant → inheritance → remove-broad; grant failure leaves inheritance untouched; /remove:g only targets Everyone/Users/Authenticated Users SIDs and cannot strip the owner ACE. Regressions cover success order, remove:g-timeout with owner ACE retained, and grant-first EPERM.

Security review

No medium+ findings. End-state isolation unchanged (same broad SIDs + /findsid verify). Argv-only icacls spawn (no shell). EPERM still fail-closed; timeout soft-fail policy unchanged but partial failure is now safer (explicit owner ACE vs stranded empty DACL). Brief post-grant / pre-inheritance window is same-process and not worse than unhardened baseline.

Local

• bun test tests/windows-secret-acl.test.ts — 28 pass
• bun run typecheck / privacy:scan — clean

Holding merge per request. Ready when you want it squashed.

[chatgpt-codex-connector]

To use Codex here, create an environment for this repo.