fix(credentials): restore Windows ACL inheritance to fix EPERM on read

[aythin]

What does this PR do?

ensureWindowsRestrictiveAcl previously called:

icacls credentials /inheritance:r /grant:r USERNAME:F

/inheritance:r strips all inherited ACEs and leaves the file protected solely by the USERNAME env-var grant. On Windows with Microsoft Accounts or domain-joined machines, USERNAME may not resolve to the same SID that owns the file — the resulting ACL locks out the file owner, causing every subsequent credential read to fail with EPERM: operation not permitted.

Fix: call icacls /reset first (re-enables inheritance from the parent directory), then /grant:r USERNAME:F as an explicit belt-and-suspenders Full Control entry.

Verified on Windows 11 Pro:

• testsprite setup --from-env --yes + testsprite doctor previously failed immediately with EPERM
• After this fix, testsprite doctor reports all checks passed ✅

Related issue

None — small bug fix, no issue required per CONTRIBUTING.md.

Type of change

• Bug fix (non-breaking change that fixes an issue)

Checklist

• PR targets the main branch.
• Commits follow Conventional Commits (fix(credentials): ...).
• npm run lint and npm run format:check pass.
• npm run typecheck passes.
• npm test passes and coverage stays at or above the 80% gate.
• New behavior is covered by unit tests (updated credentials.test.ts to expect two icacls calls).
• No secrets, API keys, internal endpoints, or personal data are included.
• User-facing changes are reflected in README.md / DOCUMENTATION.md where relevant. (N/A — internal Windows ACL fix)

Notes for reviewers

Only two files changed: src/lib/credentials.ts (the fix) and src/lib/credentials.test.ts (updated test assertions to match the new two-call sequence: /reset then /grant:r).

Summary by CodeRabbit

Bug Fixes

• Improved Windows credential file permissions by resetting inherited access before applying restrictive access controls.
• Strengthened protection for stored credentials by granting access through Windows OWNER RIGHTS rather than relying on the logged-in username.
• Added clearer warning handling when permission updates fail or return unsuccessful results.

Tests

• Expanded Windows permission checks to cover reset and access-grant failures.
• Verified the expected command sequence and secure execution behavior.

Closes #304

[github-actions]

✅ This PR is linked to an issue assigned to @aythin — thanks! The needs-issue label has been removed.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 11b63197-cdad-40bf-b7da-d478e71aff1e

📥 Commits

Reviewing files that changed from the base of the PR and between 4c10aed and f3816d2.

📒 Files selected for processing (2)

• src/lib/credentials.test.ts
• src/lib/credentials.ts

Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.

---

Walkthrough

Windows credential ACL hardening now resets permissions before granting Full Control to the OWNER RIGHTS SID. The implementation checks reset and grant failures. Tests cover command order, secure spawn options, exit statuses, and spawn errors.

Changes

Windows ACL hardening

Layer / File(s)	Summary
Reset and grant ACL permissions src/lib/credentials.ts, src/lib/credentials.test.ts	Windows ACL handling runs icacls /reset, then grants Full Control with /inheritance:r /grant:r *S-1-3-4:F. The username-based grant and env option were removed.
ACL failure validation src/lib/credentials.test.ts	Tests verify warnings and invocation counts when either icacls command fails or exits with a non-zero status.

Estimated code review effort: 2 (Simple) | ~15 minutes

Merge Risk: 🔵 Low · up to f3816

On Windows, ACL enforcement now uses a reset followed by a tightening grant. If the second operation fails or is interrupted, inherited permissions may remain on the credential file until a later write retries the fix, potentially exposing credentials to principals allowed by the parent directory. The change is localized and mergeable with explicit owner awareness and follow-up to make this transition fail closed or validate the resulting ACL.

Suggested reviewers: ruili-testsprite

🚥 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 identifies the Windows ACL change and its purpose: fixing credential-file EPERM read errors.
Linked Issues check	✅ Passed	The changes address issue #304 by resetting inherited ACLs, applying restrictive permissions to the OWNER RIGHTS SID, checking both icacls operations, and adding failure-path tests. These changes targ…
Out of Scope Changes check	✅ Passed	The implementation and tests remain within the issue scope. The ACL changes, error handling, argument-order verification, and removal of the unused environment option all support the Windows credentia…
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files.

Full details: Linked Issues check

Explanation

The changes address issue #304 by resetting inherited ACLs, applying restrictive permissions to the OWNER RIGHTS SID, checking both icacls operations, and adding failure-path tests. These changes target the Windows credentials-file unreadability and preserve owner access.

Full details: Out of Scope Changes check

Explanation

The implementation and tests remain within the issue scope. The ACL changes, error handling, argument-order verification, and removal of the unused environment option all support the Windows credentials-file fix.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 1

🤖 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 `@src/lib/credentials.ts`:
- Around line 242-246: The ACL update flow around run('icacls', [path,
'/reset']) must validate the reset before issuing /grant:r. Capture the /reset
result, handle both an execution error and a non-zero status by reporting the
failure and stopping the ACL transition, and add tests covering each failure
form.

🪄 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 Plus

Run ID: 6de32c2f-8823-409f-9734-67e69f459548

📥 Commits

Reviewing files that changed from the base of the PR and between fe07bc9 and 4010631.

📒 Files selected for processing (2)

• src/lib/credentials.test.ts
• src/lib/credentials.ts

[zeshi-du]

Thanks for this — the diagnosis is exactly right, and it's a regression we introduced: an earlier Windows ACL hardening change runs icacls <path> /inheritance:r /grant:r %USERNAME%:F with no /reset, so on Microsoft-Account or domain-joined Windows the explicit USERNAME grant may not resolve to the SID that owns the file and the owner gets locked out of their own credentials. /reset before the grant is the right shape.

Your CI had never been approved to run (first-time-contributor gate — our fault, not yours). I've approved it, and now that it has run there's one thing to fix:

• Lint & Format fails. Run npm run lint:fix && npm run format and commit the result.

Everything else is green, including the Windows leg, which is the one that matters here.

One request while you're in there: please make sure there's a unit test that pins the icacls argument order — the existing spawnSync mock in credentials.test.ts gives you the seam. This code path can only fail on real Windows with a non-local account, so the test is the only thing that will stop the next person from re-introducing /inheritance:r without /reset.

Push those and I'll merge. This is a customer-visible break for Windows users, so it's going out in the next release.

[zeshi-du]

Correcting my own steer from 8/13: "/reset before the grant is the right shape" was true as far as it went, but I said "push those and I'll merge" without checking what /reset + /grant:r actually leaves behind — and it doesn't leave the file restrictive. That gap is mine, not yours. Here's what actually needs to change before I can merge.

What /reset + /grant:r alone does, and why it isn't equivalent to the /inheritance:r it replaces:

• icacls <path> /reset (credentials.ts:242) clears the "protected" flag and re-enables inheritance from the parent directory. It adds no ACE of its own.
• icacls <path> /grant:r <user>:F (credentials.ts:248) replaces only <user>'s explicit ACE. It doesn't strip the inherited ACEs /reset just turned back on, and it doesn't restore the protected flag.
• Net effect: the file is no longer decoupled from its parent directory's ACL — it inherits whatever the parent grants, and will keep tracking any future change to that parent ACL. That's the opposite of what this function's own docstring promises ("Windows hosts use ACL tightening via icacls", credentials.ts:207), and it's now a real asymmetry with the POSIX branch (credentials.ts:216), which still does chmod(0600).
• On a standard single-user Windows profile this is a modest gap — inherited ACEs there are typically just owner + SYSTEM + Administrators. On a domain-joined or GPO-managed machine it's wider, since inherited ACEs routinely include broader groups there. This is a hardening regression, not a live exploit — but it's exactly the thing ensureWindowsRestrictiveAcl exists to prevent, so I can't take it as-is.

Root cause — not walking this back: your diagnosis is correct. %USERNAME% doesn't reliably resolve to the SID that owns the file on Microsoft-Account or domain-joined machines, so the old /inheritance:r /grant:r %USERNAME%:F could strip every inherited ACE and then grant to a SID nobody has, leaving an ACL nobody can read — exactly the EPERM in #304. /reset first is the right move for unwinding that broken state. It just can't be the last step.

What I need: keep /reset as the cleanup, then re-tighten with a grant target that's guaranteed to resolve. Pick whichever of these — I'll take a working version of any of them:

(a) Grant to the well-known OWNER RIGHTS SID instead of a username. It always resolves and always means "whoever owns this file":

icacls <path> /reset
icacls <path> /inheritance:r /grant:r *S-1-3-4:F

This is closest to what the docstring already claims, and it removes the dependency on USERNAME resolving to anything — which is the actual root cause. My preference, but not a requirement.

(b) Resolve the real owning SID first (e.g. [System.Security.Principal.WindowsIdentity]::GetCurrent().User.Value or whoami /user) and grant to that SID literal instead of the USERNAME string.

(c) Grant first, then /inheritance:r, then read the resulting ACL back and confirm the intended principal has F before returning — rolling back to the inherited state (re-/reset) if the tightening left the file unreadable, instead of leaving it silently broader.

Two more things while you're in there:

1. The first run('icacls', [path, '/reset'], ...) call (credentials.ts:242) discards its return value — only the second call's result is checked. CodeRabbit flagged exactly this on this PR on 2026-08-03 and it's still open on the current head: if /reset fails, execution falls straight through to the grant with no warning. Check result.error / result.status on both calls the same way.
2. There's no test for /reset failing, or /reset succeeding while /grant:r fails. The existing "warns on Windows when credentials ACL tightening cannot run" test only covers a missing USERNAME env var, where spawnSync is never called at all — it doesn't exercise the icacls-failure path either call could actually hit.

Separate, factual, not something to fix in this PR: mutateCredentialsFile's mkdirSync(dirname(path), { recursive: true, mode: 0o700 }) (credentials.ts:283) — the mode option is a documented no-op on Windows, so the parent directory was never actually tightened by us on this platform either. Just context for why the file-level ACL is the only real control point here; not blocking.

On CI: your 2026-08-18 push was a pure Prettier reflow, no logic change. Full CI had never actually run on that head — the fork-approval gate re-arms on every push from a first-time contributor, so your checks tab was empty through no fault of yours. I've approved the run now. Two failures in it are ours, not yours: Dependency Review (org-level dependency graph is off org-wide, fails on every PR) and ESLint Security (changed files) (pre-existing lint backlog; the fix ships in our source repo on the next release). Unit Tests (Windows) also failed on the first attempt, but on an unrelated timeout in test.test.ts (runFailureGet > in-place rewrite removes stale top-level files — Test timed out in 5000ms, in a filesystem-heavy test whose sibling took 2562ms on the same runner). Your credentials.test.ts passed clean in that same run (25 passed, 1 skipped, 0 failed), and this PR touches nothing that test reaches. I re-ran the Windows job to be sure rather than asserting it — it passed, so that was a runner-speed flake, not your change. Everything except the two infrastructure checks above is now green.

Push the ACL fix plus the two follow-ups above and I'll take another look.

[aythin]

Hi @zeshi-du, thank you for the exceptionally thorough review — and for owning the gap in the earlier steer. No apology needed at all; your analysis of what /reset + /grant:r leaves behind made the fix straightforward to land.

I went with option (a), since it addresses the root cause directly:

• icacls <path> /reset — unwinds any broken inheritance state from the previous /inheritance:r attempt (the EPERM from bug: fix(credentials): Windows credentials file unreadable after setup — EPERM on read #304)
• icacls <path> /inheritance:r /grant:r *S-1-3-4:F — decouples the file from the parent ACL and grants Full Control to the well-known OWNER RIGHTS SID, removing the dependency on %USERNAME% resolving to the owner's SID entirely

Both follow-ups are also addressed:

1. Both icacls calls now check result.error and result.status, with distinct warning messages for the /reset and grant phases (the first call previously discarded its result — thanks to CodeRabbit for flagging that originally).
2. credentials.test.ts now pins the exact argument order of both calls, and adds coverage for all four failure paths: /reset erroring, /reset exiting non-zero, and the grant failing (error / non-zero) after a successful /reset.

I also removed the now-unused env field from RestrictiveModeOptions, since nothing reads %USERNAME% anymore.

Noted on mkdirSync's mode being a no-op on Windows — thanks for the context; happy to look at that separately if it'd be useful.