fix(security): create secret files atomically with owner-only permissions (#1264)

[Chris0Jeky]

Closes #1264 (seeded from the #1263/#1241 review; the Copilot bot round on #1263 flagged the same race).

What

New FirstRunBootstrapper.WriteRestrictedFile creates the file atomically with owner-only permissions and FileShare.None, then writes through that same handle:

• Unix: FileStreamOptions.UnixCreateMode = UserRead | UserWrite — the file is born 0600.
• Windows: the protected owner-only FileSecurity is passed to CreateFile itself via the in-box FileInfo.Create(FileMode, FileSystemRights, FileShare, int, FileOptions, FileSecurity) extension.
• FileMode.CreateNew refuses to adopt a file someone pre-created at the path (and the helper never deletes a file it did not create).

Why

The previous sequence (File.Create → RestrictFileToCurrentUser → write) left a window where the empty temp file existed with the directory's default permissions; a racer could pre-open a read handle that survives the tightened ACL (ACL changes don't revoke open handles) and read the later-written JWT secret / connector key. Unreachable in the single-user deployment, but the claim 'the secret is never in an unprotected file' is now true at every instant.

Call sites

• PersistValue temp file → WriteRestrictedFile(tempPath, payload) (+ existing atomic File.Move).
• PreserveCorruptConfig .corrupt-* backup → atomic restricted create from File.ReadAllBytes (byte-faithful; no copy-then-restrict window), with a recovery-first fallback to the old copy + best-effort restrict so an ACL failure never costs the only copy of a recoverable key.
• RestrictFileToCurrentUser is retained for forward remediation of EXISTING files (RestrictExistingLocalConfigFile, the backup fallback); its Windows DACL construction is extracted into a shared BuildOwnerOnlyFileSecurity.

Tests (42/42 FirstRun-filtered Api + 13/13 Cli, both green locally; each CI OS covers its platform branch)

• WriteRestrictedFile_CreatesFileAtomicallyLockedToCurrentUser — born-restricted (0600 / protected single-SID DACL) + exact content.
• WriteRestrictedFile_RefusesToAdoptAPreExistingFile — CreateNew throws IOException, pre-existing file untouched.
• QuarantineCorruptLocalConfigAt_BackupIsRestrictedToCurrentUser — backup restricted + byte-faithful.
• WriteRestrictedFile_LockdownSurvivesAtomicMove — the fix(security): lock appsettings.local.json secrets to the current user on Windows (#1241) #1263 move-preservation test updated to mirror the new PersistValue sequence.

Out of scope

• CliFirstRunBootstrapper sibling (same gap + Unix TOCTOU) — tracked in CliFirstRunBootstrapper: owner-only Windows ACL + fix write-before-chmod TOCTOU on the connector key file #1262.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[gemini-code-assist]

Code Review

This pull request improves security when writing sensitive configuration files by introducing atomic file creation with owner-only permissions (WriteRestrictedFile), closing a race condition window where files were briefly readable by other local users. Corresponding unit tests were added to verify atomic lockdown, refusal of pre-existing files, and backup restriction. The reviewer suggested a performance improvement to use Encoding.UTF8.GetBytes instead of instantiating a new UTF8Encoding object on every call to avoid unnecessary allocations.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[Chris0Jeky]

Adversarial Code Review

Two independent review agents (filesystem-security API semantics + failure-paths/test-regression), both empirical: they built scratch net8.0 probes (Windows on this box; Unix in WSL) rather than assuming API behavior.

CRITICAL / HIGH

• None. Empirically confirmed sound: the DACL is applied AT creation (with the handle open and before the first write, GetAccessControl already shows the protected single-SID descriptor — a plain File.Create inherits 9 ACEs incl. other users' SIDs, proving this closes a real FirstRunBootstrapper: create secret temp file atomically with owner-only ACL (close empty-file handle race) #1264 window); FileSystemRights.Write suffices for the write (CreateFileW adds SYNCHRONIZE implicitly); the protected DACL / 0600 survive File.Move incl. overwrite onto a default-ACL target; CreateNew never adopts or deletes a pre-existing file (on Linux, O_EXCL even refuses a dangling symlink — a hardening over the old File.Create); the exception surface is strictly NARROWER than before (the old File.Create's raw UnauthorizedAccessException bypassed callers' catch (IOException) — a latent startup-crash path, now normalized); UTF8-no-BOM output is byte-identical; PersistValue's mutex/cleanup/ReplaceFileWithRetry semantics exactly preserved; 39/39 tests passed on the PR head in an isolated worktree.

MEDIUM

1. Fail-closed became fail-open on non-ACL filesystems (both lenses independently). CreateFileW silently IGNORES the supplied security descriptor on FAT32/exFAT/some SMB shares, and open(2)'s mode is ignored on vfat mounts — where the OLD SetAccessControl/SetUnixFileMode calls FAILED (fail-closed: in-memory fallback + the desktop-Production fatal error that literally names FAT32/exFAT). The new code would silently persist the JWT secret/connector key world-readable there (e.g. a portable install on an exFAT USB stick), contradicting the helper's own doc.
2. No test pinned the load-bearing property (probe-proven). The reviewer implemented create-then-restrict in a scratch probe and it PASSED every test in the file — including PersistValue_SetsUnixFileMode_StructuralCheck, which had become vacuous (PersistValue no longer calls SetUnixFileMode; the assertion was satisfied by the unrelated remediation helper).

LOW

3. Corrupt-backup double-failure corner: if the primary restricted create fails mid-write AND its cleanup delete fails (same-user AV/indexer handle), the residue made the fallback File.Copy(overwrite:false) fail on the SAME name — 'could NOT be backed up' while the source was still readable, then the original gets deleted. Also the error label said 'restricted create:' even when the failure came from File.ReadAllBytes.
4. CS0419 ambiguous <see cref="WriteRestrictedFile"/> (now overloaded).
5. Issue CliFirstRunBootstrapper: owner-only Windows ACL + fix write-before-chmod TOCTOU on the connector key file #1262's remediation text is stale: it still prescribes the create-then-restrict sequence this PR just replaced as insufficient.
6. (Informational, probed): UnixCreateMode is umask-masked — can only STRIP bits (0600 is never weakened), but a pathological owner-bit umask could over-tighten below 0600 where the old chmod guaranteed exactness.

Bot Comments Addressed

• None had landed at review time (any subsequent round will be addressed before merge).

Summary

0 CRITICAL / 0 HIGH / 2 MEDIUM / 3 LOW + 1 informational. MEDIUMs merge-blocking until fixed — all fixed, see follow-up.

[Chris0Jeky]

Adversarial Review — Fixes Applied

Finding	Severity	Fix Commit	Verified
Fail-open on non-ACL filesystems	MEDIUM	cf19f552 — Windows: DACL read back through the open handle (opened with Write | ReadPermissions; an unprotected/unreadable descriptor → normalized IOException, own file cleaned up). Unix: exact 0600 pinned via File.SetUnixFileMode(SafeFileHandle) (fchmod — race-free, umask-proof, fails on vfat exactly where the old path failed). Restores fail-closed everywhere the old code was, keeps atomicity on NTFS/POSIX. Also fixes finding 6 (umask over-tightening).	43/43 FirstRun (Api) + 13/13 (Cli); the initially-missing ReadPermissions was itself caught by the behavioral tests (read-back threw UnauthorizedAccessException on a Write-only handle)
Vacuous structural check / no regression pin	MEDIUM	7f547efd — PersistValue_SetsUnixFileMode_StructuralCheck replaced by PersistValue_WritesSecretsViaAtomicRestrictedCreate_StructuralCheck (pins WriteRestrictedFile wiring for temp + backup, creation-time permissions, bans File.Create(tempPath) / RestrictFileToCurrentUser(tempPath)) + WriteRestrictedFile_FailsClosedOnNonAclFilesystems_StructuralCheck (pins both post-create guards).	The probe's create-then-restrict regression now fails 2 structural checks
Backup double-failure residue + misleading label	LOW	cf19f552 — fallback copies to a distinct .fallback name (primary residue can no longer block it; the final 'preserved at' message tracks the right path); label reworded to 'primary backup:' (honestly covers a ReadAllBytes failure too).	FirstRun suite green
CS0419 ambiguous cref	LOW	cf19f552 — <see cref="WriteRestrictedFile(string, string)"/>.	grep -c 'warning CS0419' on a full Release build → 0
#1262 stale remediation text	LOW	Issue #1262 updated (comment) to prescribe the atomic WriteRestrictedFile pattern instead of create-then-restrict.	—

All findings addressed. CI status: PENDING on 7f547efd.