Skip already-hardened config backups in the startup ACL sweep (#2093)

[erikdarlingdata]

Field shape (ghauan, #2093): the #1816 backup-hardening sweep re-attempts its ACL rewrite unconditionally on every start, and the rewrite needs ownership — which the error message's own icacls remediation doesn't transfer. So following the instructions closed the exposure but the ERROR kept firing every start about an already-secure file (the independent READABLE-by-Users CRITICAL check stayed silent — the honest witness).

Fix: gate the sweep on IsReadableByOrdinaryUsers — exposure already closed means skip entirely (nothing to do, nothing to report); the CRITICAL check remains the witness for still-exposed backups. Test pins the skip with a canary ACE that a HardenFile rewrite would strip: its survival proves the descriptor was never touched. Existing sweep tests (exposed backups get hardened, live file untouched, decoys ignored) unchanged and still passing semantics.

🤖 Generated with Claude Code

[claude]

IsReadableByOrdinaryUsers returns false in two different situations, and this gate treats them identically:

1. The exposure is genuinely closed (the intended case for this fix).
2. The DACL itself couldn't be read — GetAccessControl() threw IOException/UnauthorizedAccessException/InvalidOperationException/PlatformNotSupportedException and was swallowed (DarlingFileSecurity.cs ~line 213).

Case 2 is exactly the scenario the method's own doc comment was written for — but only for the existing call site, after a HardenFile attempt: "Returns false when the DACL itself cannot be read — the harden attempt the caller just made already logs loudly on failure, and a Critical raised on an unreadable DACL would be noise, not signal."

That assumption no longer holds here, because this new gate runs before HardenFile is ever attempted. Previously, a backup whose ACL couldn't even be inspected would fall through to HardenFile, which would very likely fail for the same reason and land in the catch below, logging the actionable icacls remediation. Now it hits continue here instead, and since the CRITICAL check further down (line 683) uses the same IsReadableByOrdinaryUsers(backup) call and also gets false, nothing is logged at all.

Net effect: a backup carrying the same DPAPI-protected credentials as the live config, whose permissions are broken enough that the ACL can't even be read, now fails completely silently instead of producing the loud, actionable error this file was designed to always emit. Worth distinguishing "confirmed already-secure" from "couldn't determine" before skipping — e.g. have IsReadableByOrdinaryUsers surface the read failure (or add a small wrapper) so this gate can fall through to the existing attempt/log path instead of swallowing it.

[claude]

Reviewed the diff (CHANGELOG.md, DarlingWorker.cs, DarlingFileSecurityTests.cs).

Scope check: this is Darling-only. Lite hardens its config directory's ACL once, differently (Lite/App.xaml.cs), and has no equivalent per-start "existing backups" sweep, so there's no Lite counterpart that needs the same idempotence gate — no parity drift here.

Fix logic: gating TryHardenConfigBackups's per-file rewrite on IsReadableByOrdinaryUsers(backup) correctly stops it from re-attempting (and erroring on) an already-hardened backup, matching the #2093 field report. Left one inline comment on a real edge case in that gate: IsReadableByOrdinaryUsers returns false both when the exposure is genuinely closed and when the DACL couldn't be read at all (a swallowed exception), and the new gate can't tell those apart. That collapses a case that used to produce a loud, actionable icacls remediation log into a silent skip — for code that exists specifically to protect DPAPI-encrypted credentials, that's worth tightening.

Test: ConfigBackupSweep_AlreadyHardenedBackup_IsSkippedUntouched is a solid regression test — the canary ACE approach convincingly proves the descriptor was never touched, and it's consistent with the existing sweep tests' style.

No SQL changed in this PR, so no T-SQL style-guide or missing-index concerns apply. No injection/secrets/network issues spotted — this is local ACL manipulation on paths derived from the resolved config path, not external input.