From 426b57ccadd15b1b4a7f95efc1ec3cf7f78841b3 Mon Sep 17 00:00:00 2001 From: Stuart Meeks Date: Tue, 25 Aug 2026 11:24:45 +0800 Subject: [PATCH] test: assert the Windows credentials-directory ACL (#47) CredentialsDirectoryTests skipped its Windows case with the comment "Unix-only: Windows uses ACLs, verified via the file-perm integration path." No such path existed: grepping the test project for GetAccessControl, FileSystemAccessRule and DirectorySecurity returned zero hits. The comment was worse than none -- it told a reader coverage existed. So CredentialsDirectory.CreateWithWindowsAcl was asserted nowhere, and a regression in it -- wrong SID, inheritance left enabled, an ACE dropped -- would ship green on all three platforms. ACL hardening is one of the two reasons CLAUDE.md gives for keeping the Windows CI leg, and it is the stated security boundary for LocalFileCredentialEncryption. The new test asserts AreAccessRulesProtected, and that the rule set is exactly the current user and SYSTEM with FullControl/Allow. Asserting the whole set, rather than that our rule is present, is what catches an extra grant. The Unix test keeps its early-return guard rather than moving to Assert.SkipWhen: that branch is what lets the analyzer narrow the platform for the File.GetUnixFileMode calls below it (CA1416). Noted in place so the inconsistency with the new test does not read as an oversight. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 11 ++++ .../Persistence/CredentialsDirectoryTests.cs | 56 ++++++++++++++++++- 2 files changed, 66 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d8848d8..738adba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **The Windows ACL hardening was verified by no test, behind a comment claiming it was** + (#47). `CredentialsDirectoryTests` skipped its Windows case with *"verified via the file-perm + integration path"*, and no such path existed anywhere in the suite — grepping for + `GetAccessControl`, `FileSystemAccessRule` and `DirectorySecurity` returned nothing. So a + regression in `CredentialsDirectory.CreateWithWindowsAcl` — wrong SID, inheritance left + enabled, an ACE dropped — would have shipped green on all three platforms, while ACL + hardening is one of the two reasons `CLAUDE.md` gives for keeping the Windows CI leg at all. + There is now a test asserting inheritance is disabled and that the rule set is *exactly* the + current user and SYSTEM, each with `FullControl` and `Allow`. Asserting the whole set rather + than the presence of one rule is what catches an extra grant. + - **Replacing an existing selection was never tested on any backend** (#51). Every `SelectCredentialAsync` call in the suite was either a first selection or a negative case, and the only multi-select test used *different* providers — so `KeychainCredentialManager`'s diff --git a/tests/NextIteration.SpectreConsole.Auth.Tests/Persistence/CredentialsDirectoryTests.cs b/tests/NextIteration.SpectreConsole.Auth.Tests/Persistence/CredentialsDirectoryTests.cs index ce55d7e..441e6cc 100644 --- a/tests/NextIteration.SpectreConsole.Auth.Tests/Persistence/CredentialsDirectoryTests.cs +++ b/tests/NextIteration.SpectreConsole.Auth.Tests/Persistence/CredentialsDirectoryTests.cs @@ -1,3 +1,7 @@ +using System.Runtime.Versioning; +using System.Security.AccessControl; +using System.Security.Principal; + using NextIteration.SpectreConsole.Auth.Persistence; using NextIteration.SpectreConsole.Auth.Tests.Infrastructure; @@ -55,7 +59,10 @@ public void Ensure_SetsUnixMode0700_OnFirstCreation() { if (OperatingSystem.IsWindows()) { - return; // Unix-only: Windows uses ACLs, verified via the file-perm integration path. + // Early return rather than Assert.SkipWhen: the analyzer uses this branch to + // narrow the platform for the File.GetUnixFileMode calls below (CA1416). + // The Windows ACL equivalent is Ensure_SetsHardenedAcl_OnFirstCreation. + return; } using var temp = new TempDir(); @@ -94,5 +101,52 @@ public void Ensure_DoesNotChange_ExistingUnixMode() // Should respect consumer-chosen perms on an existing directory. Assert.Equal(originalMode, File.GetUnixFileMode(target)); } + + [Fact] + [SupportedOSPlatform("windows")] + public void Ensure_SetsHardenedAcl_OnFirstCreation() + { + Assert.SkipWhen(!OperatingSystem.IsWindows(), "Windows ACLs; Unix uses mode bits, covered above."); + + using var temp = new TempDir(); + var target = Path.Join(temp.Path, "creds"); + + CredentialsDirectory.Ensure(target); + + // Nothing asserted this before. The comment on the Unix test claimed the ACL was + // "verified via the file-perm integration path", and no such path existed + // anywhere in the suite (#47) -- so a regression here (wrong SID, inheritance + // left on, an ACE dropped) shipped green on all three platforms, while ACL + // hardening is one of the two reasons CLAUDE.md keeps the Windows CI leg. + var security = new DirectoryInfo(target).GetAccessControl(); + + Assert.True( + security.AreAccessRulesProtected, + "inheritance must be disabled, or %USERPROFILE% ACEs still grant access here"); + + var rules = security.GetAccessRules( + includeExplicit: true, includeInherited: true, typeof(SecurityIdentifier)); + + var sids = rules.Cast() + .Select(r => ((SecurityIdentifier)r.IdentityReference).Value) + .Distinct(StringComparer.Ordinal) + .ToList(); + + var currentUser = WindowsIdentity.GetCurrent().User!.Value; + var system = new SecurityIdentifier(WellKnownSidType.LocalSystemSid, null).Value; + + // Exactly the current user and SYSTEM -- no Administrators, no Users, nothing + // inherited. Asserting the whole set, not just that ours is present, is what + // catches an extra ACE being granted. + string[] expected = [.. new[] { currentUser, system }.OrderBy(x => x, StringComparer.Ordinal)]; + string[] actual = [.. sids.OrderBy(x => x, StringComparer.Ordinal)]; + Assert.Equal(expected, actual); + + foreach (FileSystemAccessRule rule in rules) + { + Assert.Equal(AccessControlType.Allow, rule.AccessControlType); + Assert.Equal(FileSystemRights.FullControl, rule.FileSystemRights); + } + } } }