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); + } + } } }