diff --git a/CHANGELOG.md b/CHANGELOG.md index 18c358e..3b87a61 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,27 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 --- +## [0.6.1] — 2026-05-03 + +### Fixed +- **Markup injection in `accounts` command output** — provider names, account names, environments, summary `DisplayFields`, exception messages, and the `--provider` flag echoed in the "Unknown provider" error now flow through `Markup.Escape` before reaching `MarkupLine` / `Table.AddRow` / Spectre selection prompts. A credential whose name happened to contain `[` or `]` previously corrupted the `accounts list` table rendering, and exception messages with markup-looking content (common on `JsonException` / IO errors) garbled the error line in `CommandErrorReporter`. +- **`accounts delete ` crash** — `accounts delete abc` (or any non-GUID id shorter than 8 chars) previously threw `ArgumentOutOfRangeException` on the `accountId[..8]` slice in the confirm prompt before reaching "not found". A new `CommandFormatting.ShortId` helper safely abbreviates, and `DeleteCredentialAsync` / `SelectCredentialAsync` / `GetCredentialByIdAsync` now validate that `accountId` is a GUID before any filesystem call. Non-GUIDs resolve to deterministic `false` on the mutating APIs and `ArgumentException` on the strict `GetCredentialByIdAsync`. Closes a path-traversal/glob-injection vector where `*` or `..\..\foo` could have flowed into `Path.Combine` and `Directory.GetFiles` patterns. +- **Concurrent `accounts select` lost updates** — `selections.json` was read-modify-written without serialization, so two CLI invocations modifying different providers could lose one provider's update. Reads/writes now happen under a cross-process advisory lock backed by `selections.json.lock` (`FileShare.None` + `FileOptions.DeleteOnClose` + exponential backoff up to ~5s). Atomic rename was already preventing torn files; this closes the lost-update window. The Keychain and libsecret backends are unaffected — both OS stores serialise their own writes. +- **Unbalanced parenthesis in `accounts select` and `accounts delete` choice prompts** — drive-by fix while replacing the unsafe `[..8]` slices. + +### Changed +- **`PackageOutputPath`** — was hard-coded as `C:\nuget-local\`, which on Linux produced surprising `src/NextIteration.SpectreConsole.Auth/C:/nuget-local/…` repo churn. Now repo-relative (`$(MSBuildThisFileDirectory)..\..\artifacts\packages`) and already gitignored. +- **`GeneratePackageOnBuild`** — now scoped to `Release` builds via MSBuild condition. `dotnet test` and Debug builds no longer pack, materially shortening the local test loop. + +### Tests +- 18 new tests; suite now at 145. + - `CommandFormatting.ShortId` across full GUIDs, short strings, exact-8 boundary, null/empty. + - Non-GUID `accountId` resolves to deterministic not-found on file-backend `Delete`/`Select`; throws on `GetCredentialByIdAsync`. + - Concurrent `SelectCredentialAsync` on different providers both persist (regression test for the `selections.json` race). + - `SelectionsLock` uncontended/contended/post-release semantics. + +--- + ## [0.6.0] — 2026-04-18 ### Added @@ -79,6 +100,7 @@ Consumers needed a way to read a specific stored credential's secret at runtime - SourceLink, deterministic builds, embedded symbols, published symbol packages. - `TreatWarningsAsErrors=true`, `AnalysisLevel=latest` — zero-warning public API. +[0.6.1]: https://github.com/StuartMeeks/NextIteration.SpectreConsole.Auth/releases/tag/v0.6.1 [0.5.0]: https://github.com/StuartMeeks/NextIteration.SpectreConsole.Auth/releases/tag/v0.5.0 [0.4.2]: https://github.com/StuartMeeks/NextIteration.SpectreConsole.Auth/releases/tag/v0.4.2 [0.4.1]: https://github.com/StuartMeeks/NextIteration.SpectreConsole.Auth/releases/tag/v0.4.1 diff --git a/src/NextIteration.SpectreConsole.Auth/Commands/AddCredentialCommand.cs b/src/NextIteration.SpectreConsole.Auth/Commands/AddCredentialCommand.cs index babacdb..70b4b9e 100644 --- a/src/NextIteration.SpectreConsole.Auth/Commands/AddCredentialCommand.cs +++ b/src/NextIteration.SpectreConsole.Auth/Commands/AddCredentialCommand.cs @@ -63,7 +63,7 @@ protected override async Task ExecuteAsync(CommandContext context, Settings if (!_collectorsByProvider.TryGetValue(settings.Provider, out var collector)) { - AnsiConsole.MarkupLine($"[red]Unknown provider: {settings.Provider}[/]"); + AnsiConsole.MarkupLine($"[red]Unknown provider: {Markup.Escape(settings.Provider)}[/]"); return 1; } diff --git a/src/NextIteration.SpectreConsole.Auth/Commands/CommandErrorReporter.cs b/src/NextIteration.SpectreConsole.Auth/Commands/CommandErrorReporter.cs index 463bb2a..dc9fcdb 100644 --- a/src/NextIteration.SpectreConsole.Auth/Commands/CommandErrorReporter.cs +++ b/src/NextIteration.SpectreConsole.Auth/Commands/CommandErrorReporter.cs @@ -18,14 +18,19 @@ internal static class CommandErrorReporter /// internal static void Report(Exception ex, string contextMessage, bool verbose) { + // contextMessage is library-internal today, but escape defensively + // so a future caller passing user-derived text can't break the line. + // ex.Message is always external — JSON parse errors, IO errors, etc. + // commonly contain '[' / ']' which Spectre would otherwise interpret + // as malformed markup. if (verbose) { - AnsiConsole.MarkupLine($"[red]{contextMessage}[/]"); + AnsiConsole.MarkupLine($"[red]{Markup.Escape(contextMessage)}[/]"); AnsiConsole.WriteException(ex, ExceptionFormats.ShortenEverything); } else { - AnsiConsole.MarkupLine($"[red]{contextMessage}: {ex.Message}[/]"); + AnsiConsole.MarkupLine($"[red]{Markup.Escape(contextMessage)}: {Markup.Escape(ex.Message)}[/]"); AnsiConsole.MarkupLine("[grey]Run with --verbose for more detail.[/]"); } } diff --git a/src/NextIteration.SpectreConsole.Auth/Commands/CommandFormatting.cs b/src/NextIteration.SpectreConsole.Auth/Commands/CommandFormatting.cs new file mode 100644 index 0000000..006f263 --- /dev/null +++ b/src/NextIteration.SpectreConsole.Auth/Commands/CommandFormatting.cs @@ -0,0 +1,24 @@ +namespace NextIteration.SpectreConsole.Auth.Commands +{ + /// + /// Display helpers shared across the accounts commands. Centralised + /// so an unusually short or malformed account id (e.g. a tampered file on + /// disk, or a user-supplied id that didn't pass GUID parsing) doesn't + /// crash the command via an out-of-range slice. + /// + internal static class CommandFormatting + { + /// + /// Returns a display-friendly abbreviation of an account id. Library + /// account ids are GUIDs (36 chars), but on the user-supplied path + /// we may receive arbitrarily short strings before validation has + /// run — falling back to the full string keeps error messages + /// useful instead of throwing. + /// + internal static string ShortId(string? accountId) + { + if (string.IsNullOrEmpty(accountId)) return string.Empty; + return accountId.Length >= 8 ? accountId[..8] + "..." : accountId; + } + } +} diff --git a/src/NextIteration.SpectreConsole.Auth/Commands/DeleteCredentialCommand.cs b/src/NextIteration.SpectreConsole.Auth/Commands/DeleteCredentialCommand.cs index 26210aa..9540384 100644 --- a/src/NextIteration.SpectreConsole.Auth/Commands/DeleteCredentialCommand.cs +++ b/src/NextIteration.SpectreConsole.Auth/Commands/DeleteCredentialCommand.cs @@ -51,7 +51,7 @@ protected override async Task ExecuteAsync(CommandContext context, Settings } var choices = allCredentials.Select(c => - $"{c.AccountName} ({c.ProviderName} - {c.AccountId[..8]}...").ToArray(); + $"{Markup.Escape(c.AccountName)} ({Markup.Escape(c.ProviderName)} - {CommandFormatting.ShortId(c.AccountId)})").ToArray(); var selectedChoice = await AnsiConsole.PromptAsync( new SelectionPrompt() @@ -63,7 +63,7 @@ protected override async Task ExecuteAsync(CommandContext context, Settings } // Confirm deletion - if (!settings.Force && !await AnsiConsole.ConfirmAsync($"Are you sure you want to delete credential '{accountId[..8]}...'?", cancellationToken: cancellationToken).ConfigureAwait(false)) + if (!settings.Force && !await AnsiConsole.ConfirmAsync($"Are you sure you want to delete credential '{Markup.Escape(CommandFormatting.ShortId(accountId))}'?", cancellationToken: cancellationToken).ConfigureAwait(false)) { AnsiConsole.MarkupLine("[yellow]Deletion cancelled.[/]"); return 0; diff --git a/src/NextIteration.SpectreConsole.Auth/Commands/ListCredentialsCommand.cs b/src/NextIteration.SpectreConsole.Auth/Commands/ListCredentialsCommand.cs index a42e36e..05e6521 100644 --- a/src/NextIteration.SpectreConsole.Auth/Commands/ListCredentialsCommand.cs +++ b/src/NextIteration.SpectreConsole.Auth/Commands/ListCredentialsCommand.cs @@ -74,7 +74,7 @@ private async Task DisplayCredentialsForProvider(string provider) if (credentials.Count == 0) { - AnsiConsole.MarkupLine($"[yellow]No credentials found for provider '{provider}'.[/]"); + AnsiConsole.MarkupLine($"[yellow]No credentials found for provider '{Markup.Escape(provider)}'.[/]"); return; } @@ -94,7 +94,7 @@ private async Task DisplayCredentialsForProvider(string provider) } } - AnsiConsole.MarkupLine($"[bold]{provider}[/]"); + AnsiConsole.MarkupLine($"[bold]{Markup.Escape(provider)}[/]"); var table = new Table(); _ = table.AddColumn("ID"); @@ -109,18 +109,21 @@ private async Task DisplayCredentialsForProvider(string provider) foreach (var credential in credentials) { + // Cells render via Spectre markup, so user/provider-controlled + // values must be escaped — an account name like "[red]X[/]" + // would otherwise be parsed as styling rather than text. var row = new List { - credential.AccountId[..8] + "...", - credential.AccountName, - credential.Environment, + CommandFormatting.ShortId(credential.AccountId), + Markup.Escape(credential.AccountName), + Markup.Escape(credential.Environment), }; foreach (var key in displayFieldKeys) { var value = credential.DisplayFields .FirstOrDefault(kvp => string.Equals(kvp.Key, key, StringComparison.OrdinalIgnoreCase)); - row.Add(value.Value ?? string.Empty); + row.Add(Markup.Escape(value.Value ?? string.Empty)); } row.Add(credential.CreatedAt.ToString("yyyy-MM-dd HH:mm")); diff --git a/src/NextIteration.SpectreConsole.Auth/Commands/SelectCredentialCommand.cs b/src/NextIteration.SpectreConsole.Auth/Commands/SelectCredentialCommand.cs index 2b62967..e8d965e 100644 --- a/src/NextIteration.SpectreConsole.Auth/Commands/SelectCredentialCommand.cs +++ b/src/NextIteration.SpectreConsole.Auth/Commands/SelectCredentialCommand.cs @@ -52,7 +52,7 @@ protected override async Task ExecuteAsync(CommandContext context, Settings } var choices = allCredentials.Select(c => - $"{c.AccountName} ({c.ProviderName} - {c.AccountId[..8]}... {(c.IsSelected ? "[green](selected)[/]" : "")}").ToArray(); + $"{Markup.Escape(c.AccountName)} ({Markup.Escape(c.ProviderName)} - {CommandFormatting.ShortId(c.AccountId)}) {(c.IsSelected ? "[green](selected)[/]" : "")}").ToArray(); var selectedChoice = await AnsiConsole.PromptAsync(new SelectionPrompt() .Title("Select credential to [green]activate[/]:") diff --git a/src/NextIteration.SpectreConsole.Auth/NextIteration.SpectreConsole.Auth.csproj b/src/NextIteration.SpectreConsole.Auth/NextIteration.SpectreConsole.Auth.csproj index c391c22..3dd4bf4 100644 --- a/src/NextIteration.SpectreConsole.Auth/NextIteration.SpectreConsole.Auth.csproj +++ b/src/NextIteration.SpectreConsole.Auth/NextIteration.SpectreConsole.Auth.csproj @@ -17,11 +17,11 @@ NextIteration.SpectreConsole.Auth - 0.6.0 + 0.6.1 Stuart Meeks Credential storage, encryption, and Spectre.Console CLI commands for managing provider credentials in CLI tools. - true - C:\nuget-local\ + true + $(MSBuildThisFileDirectory)..\..\artifacts\packages true MIT README.md diff --git a/src/NextIteration.SpectreConsole.Auth/Persistence/FileCredentialManager.cs b/src/NextIteration.SpectreConsole.Auth/Persistence/FileCredentialManager.cs index 15ef671..9664ad7 100644 --- a/src/NextIteration.SpectreConsole.Auth/Persistence/FileCredentialManager.cs +++ b/src/NextIteration.SpectreConsole.Auth/Persistence/FileCredentialManager.cs @@ -20,6 +20,7 @@ public class FileCredentialManager : ICredentialManager private readonly string _credentialsDirectory; private readonly string _selectionFile; + private readonly string _selectionLockFile; private readonly ICredentialEncryption _encryption; private readonly Dictionary _summaryProviders; @@ -41,6 +42,7 @@ public FileCredentialManager( _encryption = encryption; _credentialsDirectory = credentialsDirectory; _selectionFile = Path.Combine(_credentialsDirectory, "selections.json"); + _selectionLockFile = Path.Combine(_credentialsDirectory, "selections.json.lock"); _summaryProviders = (summaryProviders ?? []) .ToDictionary(p => p.ProviderName, StringComparer.OrdinalIgnoreCase); @@ -144,6 +146,8 @@ await AtomicFile.WriteAllTextAsync( /// public async Task DeleteCredentialAsync(string accountId) { + if (!IsValidAccountId(accountId)) return false; + var found = await FindCredentialByAccountIdAsync(accountId).ConfigureAwait(false); if (found is null) { @@ -153,7 +157,9 @@ public async Task DeleteCredentialAsync(string accountId) var (filePath, credential) = found.Value; File.Delete(filePath); - // Remove the selection entry if it pointed at this credential. + // Hold the selections lock across load+modify+save so a concurrent + // select/delete on a different provider can't lose its update. + using var selectionsLock = await SelectionsLock.AcquireAsync(_selectionLockFile).ConfigureAwait(false); var selections = await LoadSelectionsAsync().ConfigureAwait(false); if (selections.TryGetValue(credential.ProviderName, out var selectedId) && selectedId.Equals(accountId, StringComparison.OrdinalIgnoreCase)) @@ -168,6 +174,8 @@ public async Task DeleteCredentialAsync(string accountId) /// public async Task SelectCredentialAsync(string accountId) { + if (!IsValidAccountId(accountId)) return false; + var found = await FindCredentialByAccountIdAsync(accountId).ConfigureAwait(false); if (found is null) { @@ -175,6 +183,7 @@ public async Task SelectCredentialAsync(string accountId) } var credential = found.Value.Credential; + using var selectionsLock = await SelectionsLock.AcquireAsync(_selectionLockFile).ConfigureAwait(false); var selections = await LoadSelectionsAsync().ConfigureAwait(false); selections[credential.ProviderName] = accountId; await SaveSelectionsAsync(selections).ConfigureAwait(false); @@ -187,8 +196,11 @@ public async Task SelectCredentialAsync(string accountId) ValidateProviderName(providerName); var selections = await LoadSelectionsAsync().ConfigureAwait(false); - if (!selections.TryGetValue(providerName, out var selectedId)) + if (!selections.TryGetValue(providerName, out var selectedId) || !IsValidAccountId(selectedId)) { + // A non-GUID id can only land here via tampered selections.json; + // treat it as "no selection" rather than letting a malformed + // string flow into Path.Combine. return null; } @@ -199,7 +211,7 @@ public async Task SelectCredentialAsync(string accountId) public async Task GetCredentialByIdAsync(string providerName, string accountId) { ValidateProviderName(providerName); - ArgumentException.ThrowIfNullOrWhiteSpace(accountId); + ValidateAccountId(accountId); return await ReadAndDecryptByIdAsync(providerName, accountId).ConfigureAwait(false); } @@ -314,6 +326,31 @@ private static void ValidateProviderName(string providerName) } } + /// + /// Library-generated account ids are always GUIDs. Caller-supplied + /// strings are constrained to that shape so they can't smuggle glob + /// metacharacters (*, ?) or path-traversal segments + /// (..) into the file lookups. + /// + private static bool IsValidAccountId(string? accountId) => + !string.IsNullOrWhiteSpace(accountId) && Guid.TryParse(accountId, out _); + + /// + /// Throwing variant of used at API + /// boundaries where a malformed id is a programmer error rather than + /// a "not found" condition. + /// + private static void ValidateAccountId(string accountId) + { + ArgumentException.ThrowIfNullOrWhiteSpace(accountId); + if (!Guid.TryParse(accountId, out _)) + { + throw new ArgumentException( + $"Account id '{accountId}' is not a valid GUID.", + nameof(accountId)); + } + } + private void EnsureDirectoryExists() => CredentialsDirectory.Ensure(_credentialsDirectory); private async Task> LoadSelectionsAsync() diff --git a/src/NextIteration.SpectreConsole.Auth/Persistence/Keychain/KeychainCredentialManager.cs b/src/NextIteration.SpectreConsole.Auth/Persistence/Keychain/KeychainCredentialManager.cs index c1a70b8..3c2e269 100644 --- a/src/NextIteration.SpectreConsole.Auth/Persistence/Keychain/KeychainCredentialManager.cs +++ b/src/NextIteration.SpectreConsole.Auth/Persistence/Keychain/KeychainCredentialManager.cs @@ -110,6 +110,8 @@ public Task> ListCredentialsAsync(string provider /// public Task DeleteCredentialAsync(string accountId) { + if (!IsValidAccountId(accountId)) return Task.FromResult(false); + var match = FindItemByAccountId(accountId); if (match is null) return Task.FromResult(false); @@ -133,6 +135,8 @@ public Task DeleteCredentialAsync(string accountId) /// public Task SelectCredentialAsync(string accountId) { + if (!IsValidAccountId(accountId)) return Task.FromResult(false); + var match = FindItemByAccountId(accountId); if (match is null) return Task.FromResult(false); @@ -160,7 +164,7 @@ public Task SelectCredentialAsync(string accountId) public Task GetCredentialByIdAsync(string providerName, string accountId) { ValidateProviderName(providerName); - ArgumentException.ThrowIfNullOrWhiteSpace(accountId); + ValidateAccountId(accountId); return Task.FromResult(ReadItemDataById(providerName, accountId)); } @@ -284,6 +288,20 @@ private static void ValidateProviderName(string providerName) } } + private static bool IsValidAccountId(string? accountId) => + !string.IsNullOrWhiteSpace(accountId) && Guid.TryParse(accountId, out _); + + private static void ValidateAccountId(string accountId) + { + ArgumentException.ThrowIfNullOrWhiteSpace(accountId); + if (!Guid.TryParse(accountId, out _)) + { + throw new ArgumentException( + $"Account id '{accountId}' is not a valid GUID.", + nameof(accountId)); + } + } + // ========================= // Keychain operations — each takes ownership of every CF object it // creates and releases in finally. diff --git a/src/NextIteration.SpectreConsole.Auth/Persistence/Libsecret/LibsecretCredentialManager.cs b/src/NextIteration.SpectreConsole.Auth/Persistence/Libsecret/LibsecretCredentialManager.cs index 7be25ab..35c3515 100644 --- a/src/NextIteration.SpectreConsole.Auth/Persistence/Libsecret/LibsecretCredentialManager.cs +++ b/src/NextIteration.SpectreConsole.Auth/Persistence/Libsecret/LibsecretCredentialManager.cs @@ -133,6 +133,8 @@ public Task> ListCredentialsAsync(string provider /// public Task DeleteCredentialAsync(string accountId) { + if (!IsValidAccountId(accountId)) return Task.FromResult(false); + // Find the item so we know its provider (for selection cleanup). var match = SearchItems( new Dictionary(StringComparer.Ordinal) @@ -168,6 +170,8 @@ public Task DeleteCredentialAsync(string accountId) /// public Task SelectCredentialAsync(string accountId) { + if (!IsValidAccountId(accountId)) return Task.FromResult(false); + var match = SearchItems( new Dictionary(StringComparer.Ordinal) { @@ -200,7 +204,7 @@ public Task SelectCredentialAsync(string accountId) public Task GetCredentialByIdAsync(string providerName, string accountId) { ValidateProviderName(providerName); - ArgumentException.ThrowIfNullOrWhiteSpace(accountId); + ValidateAccountId(accountId); return Task.FromResult(LookupCredentialByAccountId(providerName, accountId)); } @@ -308,6 +312,20 @@ private static void ValidateProviderName(string providerName) } } + private static bool IsValidAccountId(string? accountId) => + !string.IsNullOrWhiteSpace(accountId) && Guid.TryParse(accountId, out _); + + private static void ValidateAccountId(string accountId) + { + ArgumentException.ThrowIfNullOrWhiteSpace(accountId); + if (!Guid.TryParse(accountId, out _)) + { + throw new ArgumentException( + $"Account id '{accountId}' is not a valid GUID.", + nameof(accountId)); + } + } + // ========================= // Secret Service operations — each takes ownership of every GHashTable // / GError handle it creates and releases them via try/finally. diff --git a/src/NextIteration.SpectreConsole.Auth/Persistence/SelectionsLock.cs b/src/NextIteration.SpectreConsole.Auth/Persistence/SelectionsLock.cs new file mode 100644 index 0000000..852633f --- /dev/null +++ b/src/NextIteration.SpectreConsole.Auth/Persistence/SelectionsLock.cs @@ -0,0 +1,76 @@ +namespace NextIteration.SpectreConsole.Auth.Persistence +{ + /// + /// Cross-process advisory lock backed by a sentinel file inside the + /// credentials directory. Used to serialise selections.json + /// read-modify-write sequences across concurrent CLI invocations — atomic + /// rename alone prevents torn files but not lost updates. + /// + /// + /// + /// Implemented via on a sentinel file. On + /// POSIX this maps to flock(LOCK_EX | LOCK_NB); on Windows it maps + /// to the share-deny-all mode of CreateFile. In both cases a + /// second acquirer sees an and we retry with + /// exponential backoff. + /// + /// + /// auto-removes the sentinel file + /// when the holder disposes (or its process is killed and the kernel + /// closes the FD), so there is no stale-lock recovery code to maintain. + /// + /// + internal sealed class SelectionsLock : IDisposable + { + // Cumulative backoff ~5.1s. Long enough to ride through a normal + // peer holding the lock for a single read-modify-write, short enough + // to surface a deadlocked or stuck peer instead of hanging forever. + private static readonly int[] _delaysMs = [10, 25, 50, 100, 200, 400, 800, 1500, 2000]; + + private readonly FileStream _stream; + + private SelectionsLock(FileStream stream) => _stream = stream; + + internal static async Task AcquireAsync(string lockPath, CancellationToken ct = default) + { + var options = new FileStreamOptions + { + Mode = FileMode.OpenOrCreate, + Access = FileAccess.ReadWrite, + Share = FileShare.None, + Options = FileOptions.DeleteOnClose, + }; + if (!OperatingSystem.IsWindows()) + { + options.UnixCreateMode = UnixFileMode.UserRead | UnixFileMode.UserWrite; + } + + IOException? last = null; + foreach (var delay in _delaysMs) + { + try + { + return new SelectionsLock(new FileStream(lockPath, options)); + } + catch (IOException ex) + { + last = ex; + await Task.Delay(delay, ct).ConfigureAwait(false); + } + } + + try + { + return new SelectionsLock(new FileStream(lockPath, options)); + } + catch (IOException ex) + { + throw new IOException( + $"Could not acquire selections lock at '{lockPath}' after retrying for ~5s. Another process may be holding it.", + last ?? ex); + } + } + + public void Dispose() => _stream.Dispose(); + } +} diff --git a/tests/NextIteration.SpectreConsole.Auth.Tests/Commands/CommandFormattingTests.cs b/tests/NextIteration.SpectreConsole.Auth.Tests/Commands/CommandFormattingTests.cs new file mode 100644 index 0000000..378cd74 --- /dev/null +++ b/tests/NextIteration.SpectreConsole.Auth.Tests/Commands/CommandFormattingTests.cs @@ -0,0 +1,40 @@ +using NextIteration.SpectreConsole.Auth.Commands; + +using Xunit; + +namespace NextIteration.SpectreConsole.Auth.Tests.Commands; + +public sealed class CommandFormattingTests +{ + [Fact] + public void ShortId_FullGuid_ReturnsFirstEightPlusEllipsis() + { + var id = "12345678-1234-1234-1234-123456789012"; + Assert.Equal("12345678...", CommandFormatting.ShortId(id)); + } + + [Theory] + [InlineData("abc")] // shorter than 8 — no slice attempted + [InlineData("1234567")] // exactly one short + public void ShortId_ShortString_ReturnsFullStringWithoutThrowing(string id) + { + // Regression: previously every site sliced AccountId[..8] which + // throws ArgumentOutOfRangeException for short user-supplied ids + // (e.g. `accounts delete abc`). + Assert.Equal(id, CommandFormatting.ShortId(id)); + } + + [Fact] + public void ShortId_ExactlyEight_ReturnsValuePlusEllipsis() + { + Assert.Equal("12345678...", CommandFormatting.ShortId("12345678")); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + public void ShortId_NullOrEmpty_ReturnsEmpty(string? id) + { + Assert.Equal(string.Empty, CommandFormatting.ShortId(id)); + } +} diff --git a/tests/NextIteration.SpectreConsole.Auth.Tests/Persistence/FileCredentialManagerTests.cs b/tests/NextIteration.SpectreConsole.Auth.Tests/Persistence/FileCredentialManagerTests.cs index da0a3a7..97882b3 100644 --- a/tests/NextIteration.SpectreConsole.Auth.Tests/Persistence/FileCredentialManagerTests.cs +++ b/tests/NextIteration.SpectreConsole.Auth.Tests/Persistence/FileCredentialManagerTests.cs @@ -434,6 +434,72 @@ public async Task Credential_Persists_AcrossManagerInstances() Assert.Equal("{\"key\":\"v\"}", await second.GetSelectedCredentialAsync("Adobe")); } + [Theory] + [InlineData("not-a-guid")] + [InlineData("../etc/passwd")] + [InlineData("*")] + [InlineData("abc")] + public async Task DeleteCredentialAsync_NonGuidId_ReturnsFalseWithoutThrowing(string accountId) + { + using var temp = new TempDir(); + var manager = CreateManager(temp.Path); + _ = await manager.AddCredentialAsync("Adobe", "prod", "Production", "{}"); + + // Malformed ids must not flow into Path.Combine / Directory.GetFiles + // glob — they should resolve to a clean "not found". + var result = await manager.DeleteCredentialAsync(accountId); + + Assert.False(result); + } + + [Theory] + [InlineData("not-a-guid")] + [InlineData("../etc/passwd")] + [InlineData("*")] + public async Task SelectCredentialAsync_NonGuidId_ReturnsFalseWithoutThrowing(string accountId) + { + using var temp = new TempDir(); + var manager = CreateManager(temp.Path); + _ = await manager.AddCredentialAsync("Adobe", "prod", "Production", "{}"); + + var result = await manager.SelectCredentialAsync(accountId); + + Assert.False(result); + } + + [Fact] + public async Task GetCredentialByIdAsync_NonGuidId_Throws() + { + using var temp = new TempDir(); + var manager = CreateManager(temp.Path); + + await Assert.ThrowsAsync( + () => manager.GetCredentialByIdAsync("Adobe", "not-a-guid")); + } + + [Fact] + public async Task SelectCredentialAsync_ConcurrentDifferentProviders_BothPersist() + { + using var temp = new TempDir(); + var manager = CreateManager(temp.Path); + + var adobeId = await manager.AddCredentialAsync("Adobe", "a", "Production", "{}"); + var airtableId = await manager.AddCredentialAsync("Airtable", "b", "Production", "{}"); + + // Without the selections lock both calls would read the same + // pre-write snapshot, mutate their own provider's entry, then both + // save — last writer wins and one update is lost. With the lock + // both updates must be observable afterwards. + await Task.WhenAll( + manager.SelectCredentialAsync(adobeId), + manager.SelectCredentialAsync(airtableId)); + + var adobe = (await manager.ListCredentialsAsync("Adobe")).Single(); + var airtable = (await manager.ListCredentialsAsync("Airtable")).Single(); + Assert.True(adobe.IsSelected); + Assert.True(airtable.IsSelected); + } + /// /// Minimal summary provider used only to verify that /// routes diff --git a/tests/NextIteration.SpectreConsole.Auth.Tests/Persistence/SelectionsLockTests.cs b/tests/NextIteration.SpectreConsole.Auth.Tests/Persistence/SelectionsLockTests.cs new file mode 100644 index 0000000..b7530b0 --- /dev/null +++ b/tests/NextIteration.SpectreConsole.Auth.Tests/Persistence/SelectionsLockTests.cs @@ -0,0 +1,61 @@ +using NextIteration.SpectreConsole.Auth.Persistence; +using NextIteration.SpectreConsole.Auth.Tests.Infrastructure; + +using Xunit; + +namespace NextIteration.SpectreConsole.Auth.Tests.Persistence; + +public sealed class SelectionsLockTests +{ + [Fact] + public async Task Acquire_WhenUncontended_Succeeds() + { + using var temp = new TempDir(); + var lockPath = Path.Combine(temp.Path, "selections.json.lock"); + + using var held = await SelectionsLock.AcquireAsync(lockPath); + + // Successful acquisition is the assertion; we'd hit the IOException + // path below if the contract were broken. + Assert.NotNull(held); + } + + [Fact] + public async Task Acquire_WhenHeld_RetriesUntilReleased() + { + using var temp = new TempDir(); + var lockPath = Path.Combine(temp.Path, "selections.json.lock"); + + var first = await SelectionsLock.AcquireAsync(lockPath); + + // Kick off a contender — it should park inside the backoff loop until + // we dispose the holder, then proceed. + var contender = SelectionsLock.AcquireAsync(lockPath); + + // Give the contender a moment to land inside its retry loop, then + // release. If the lock semantics are broken the contender would have + // already completed by now. + await Task.Delay(50); + Assert.False(contender.IsCompleted, "contender completed while lock was held"); + + first.Dispose(); + + using var second = await contender.WaitAsync(TimeSpan.FromSeconds(5)); + Assert.NotNull(second); + } + + [Fact] + public async Task Acquire_AfterRelease_Succeeds() + { + using var temp = new TempDir(); + var lockPath = Path.Combine(temp.Path, "selections.json.lock"); + + (await SelectionsLock.AcquireAsync(lockPath)).Dispose(); + + // DeleteOnClose should clean up the sentinel file when the holder + // disposes; a fresh acquirer must succeed without seeing a stale + // lock. + using var second = await SelectionsLock.AcquireAsync(lockPath); + Assert.NotNull(second); + } +}