From 2c2afb10b6771309910f2378e0f9ab5d116516a2 Mon Sep 17 00:00:00 2001 From: Stuart Meeks Date: Sat, 22 Aug 2026 00:26:18 +0000 Subject: [PATCH] fix: resolve open CodeQL code-scanning alerts with genuine fixes Address every open alert from the security-and-quality pack without suppressions or dismissals. No consumer-visible behaviour changes. - cs/path-combine: Path.Combine -> Path.Join at all 8 sites (Path.Join concatenates unconditionally, so a rooted later segment can't silently drop the base directory). - cs/catch-of-all-exceptions: best-effort cleanup catches (AtomicFile.TryDelete, SettingsStore.TryBackupCorruptFile, test TempDir.Dispose) now catch only IOException/UnauthorizedAccessException; the intentionally broad catches (SettingsBase fire-and-forget safety net, the two settings-command boundaries) keep routing operational failures to their handler via a when-filter but let a process-fatal OutOfMemoryException propagate. - cs/linq/missed-where: ResetInstanceToDefaults filters with .Where(...). - cs/missed-using-statement: the debounce task scopes its CancellationTokenSource with a using block, preserving the existing idempotent disposal semantics. - cs/local-not-disposed: the test CLI harness disposes its TestConsole. Build clean (0 warnings, TreatWarningsAsErrors); 64/64 tests pass on net8.0 and net10.0. Co-Authored-By: Claude Opus 4.8 --- CHANGELOG.md | 24 +++++++- .../Commands/ListSettingsCommand.cs | 6 +- .../Commands/ResetSettingsCommand.cs | 6 +- .../Persistence/AtomicFile.cs | 2 +- .../Persistence/SettingsStore.cs | 13 ++-- .../ServiceCollectionExtensions.cs | 2 +- .../SettingsBase.cs | 59 +++++++++++-------- .../CommandFlowTests.cs | 2 +- .../Infrastructure/CliHarness.cs | 2 +- .../Infrastructure/TempDir.cs | 4 +- .../Persistence/AtomicFileTests.cs | 8 +-- .../SettingsStoreTests.cs | 2 +- 12 files changed, 85 insertions(+), 45 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index de48c49..f8a5ee9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,29 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] -_Nothing yet._ +### Fixed + +- Resolved every open CodeQL code-scanning alert from the `security-and-quality` + pack with a genuine code change rather than a suppression (no consumer-visible + behaviour changes): + - `cs/path-combine`: switched every `Path.Combine` to `Path.Join` (one in + `ServiceCollectionExtensions`, the rest in test infrastructure). `Path.Join` + concatenates unconditionally, so it cannot silently discard the base + directory if a later segment ever looks rooted. + - `cs/catch-of-all-exceptions`: the two best-effort cleanup catches + (`AtomicFile.TryDelete`, `SettingsStore.TryBackupCorruptFile`) and the test + `TempDir.Dispose` now catch only the `IOException`/`UnauthorizedAccessException` + they expect, so an unexpected exception surfaces instead of being swallowed. + The three intentionally broad catches — the fire-and-forget persistence + safety net (`SettingsBase`) and the two `settings` command boundaries — keep + routing all operational failures to their handler but now let a process-fatal + `OutOfMemoryException` propagate rather than mislabel it. + - `cs/linq/missed-where`: `SettingsStore.ResetInstanceToDefaults` filters the + settable properties with `.Where(...)` instead of an `if` inside the loop. + - `cs/missed-using-statement`: the debounce task now scopes its + `CancellationTokenSource` with a `using` block instead of a manual + `finally`-dispose, preserving the existing (idempotent) disposal semantics. + - `cs/local-not-disposed`: the test CLI harness now disposes its `TestConsole`. ## [1.0.0] — 2026-08-21 diff --git a/src/NextIteration.SpectreConsole.Settings/Commands/ListSettingsCommand.cs b/src/NextIteration.SpectreConsole.Settings/Commands/ListSettingsCommand.cs index 868a1c3..59f9563 100644 --- a/src/NextIteration.SpectreConsole.Settings/Commands/ListSettingsCommand.cs +++ b/src/NextIteration.SpectreConsole.Settings/Commands/ListSettingsCommand.cs @@ -50,8 +50,12 @@ protected override Task ExecuteAsync(CommandContext context, Settings setti return Task.FromResult(0); } - catch (Exception ex) + catch (Exception ex) when (ex is not OutOfMemoryException) { + // Top-level boundary: turn any operational failure into a clean + // message and a non-zero exit code rather than an unhandled + // stack trace. A process-fatal OutOfMemoryException is left to + // propagate. CommandErrorReporter.Report(ex, "Error listing settings", settings.Verbose); return Task.FromResult(1); } diff --git a/src/NextIteration.SpectreConsole.Settings/Commands/ResetSettingsCommand.cs b/src/NextIteration.SpectreConsole.Settings/Commands/ResetSettingsCommand.cs index 1b18965..c8598aa 100644 --- a/src/NextIteration.SpectreConsole.Settings/Commands/ResetSettingsCommand.cs +++ b/src/NextIteration.SpectreConsole.Settings/Commands/ResetSettingsCommand.cs @@ -100,8 +100,12 @@ protected override async Task ExecuteAsync(CommandContext context, Settings AnsiConsole.MarkupLine($"[green]Reset '{Markup.Escape(registration.Name)}' to defaults.[/]"); return 0; } - catch (Exception ex) + catch (Exception ex) when (ex is not OutOfMemoryException) { + // Top-level boundary: turn any operational failure into a clean + // message and a non-zero exit code rather than an unhandled + // stack trace. A process-fatal OutOfMemoryException is left to + // propagate. CommandErrorReporter.Report(ex, "Error resetting settings", settings.Verbose); return 1; } diff --git a/src/NextIteration.SpectreConsole.Settings/Persistence/AtomicFile.cs b/src/NextIteration.SpectreConsole.Settings/Persistence/AtomicFile.cs index 029c49a..e6d3e76 100644 --- a/src/NextIteration.SpectreConsole.Settings/Persistence/AtomicFile.cs +++ b/src/NextIteration.SpectreConsole.Settings/Persistence/AtomicFile.cs @@ -101,7 +101,7 @@ private static void TryDelete(string path) File.Delete(path); } } - catch + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) { // Best-effort cleanup; a stray ".tmp" file is harmless — it // doesn't match the "{ClassName}.json" name the loader reads. diff --git a/src/NextIteration.SpectreConsole.Settings/Persistence/SettingsStore.cs b/src/NextIteration.SpectreConsole.Settings/Persistence/SettingsStore.cs index 189fcc1..1b82c29 100644 --- a/src/NextIteration.SpectreConsole.Settings/Persistence/SettingsStore.cs +++ b/src/NextIteration.SpectreConsole.Settings/Persistence/SettingsStore.cs @@ -141,7 +141,7 @@ private static void TryBackupCorruptFile(string filePath) { File.Copy(filePath, filePath + ".bak", overwrite: true); } - catch + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) { // Best-effort: a failed backup must not prevent falling back to // defaults. The original file is left untouched (the copy, not a @@ -158,12 +158,13 @@ private static void ResetInstanceToDefaults(SettingsBase instance, SettingsTypeD instance.SuspendNotifications(); try { - foreach (var property in descriptor.SettingsType.GetProperties(BindingFlags.Public | BindingFlags.Instance)) + var settableProperties = descriptor.SettingsType + .GetProperties(BindingFlags.Public | BindingFlags.Instance) + .Where(property => property.CanRead && property.CanWrite && property.GetIndexParameters().Length == 0); + + foreach (var property in settableProperties) { - if (property.CanRead && property.CanWrite && property.GetIndexParameters().Length == 0) - { - property.SetValue(instance, property.GetValue(defaults)); - } + property.SetValue(instance, property.GetValue(defaults)); } } finally diff --git a/src/NextIteration.SpectreConsole.Settings/ServiceCollectionExtensions.cs b/src/NextIteration.SpectreConsole.Settings/ServiceCollectionExtensions.cs index fff9a72..f72f2ba 100644 --- a/src/NextIteration.SpectreConsole.Settings/ServiceCollectionExtensions.cs +++ b/src/NextIteration.SpectreConsole.Settings/ServiceCollectionExtensions.cs @@ -53,7 +53,7 @@ public static IServiceCollection AddSettings( { SettingsType = typeof(T), Name = name, - FilePath = Path.Combine(options.SettingsDirectory, name + ".json"), + FilePath = Path.Join(options.SettingsDirectory, name + ".json"), PersistenceMode = options.PersistenceMode, DebounceInterval = options.DebounceInterval, ErrorHandler = options.ErrorHandler ?? SettingsSerialization.DefaultErrorHandler, diff --git a/src/NextIteration.SpectreConsole.Settings/SettingsBase.cs b/src/NextIteration.SpectreConsole.Settings/SettingsBase.cs index 3f7c605..fa5ec77 100644 --- a/src/NextIteration.SpectreConsole.Settings/SettingsBase.cs +++ b/src/NextIteration.SpectreConsole.Settings/SettingsBase.cs @@ -198,39 +198,44 @@ private async Task DebounceAndPersistAsync( TimeSpan interval, CancellationTokenSource cts) { - try + // This task owns the CTS for the rest of its life: the using + // disposes it on every exit path (cancelled or not). A superseding + // change or Save disposes its own reference too, but CTS.Dispose is + // idempotent, so the double-dispose is harmless. + using (cts) { - if (interval > TimeSpan.Zero) + try { - await Task.Delay(interval, cts.Token).ConfigureAwait(false); + if (interval > TimeSpan.Zero) + { + await Task.Delay(interval, cts.Token).ConfigureAwait(false); + } + else + { + // Let the current synchronous call stack unwind so a burst + // of setters all schedule-then-cancel and only the last + // survives to write — coalescing with a zero interval too. + await Task.Yield(); + cts.Token.ThrowIfCancellationRequested(); + } } - else + catch (OperationCanceledException) { - // Let the current synchronous call stack unwind so a burst - // of setters all schedule-then-cancel and only the last - // survives to write — coalescing with a zero interval too. - await Task.Yield(); - cts.Token.ThrowIfCancellationRequested(); + return; // superseded by a newer change or an explicit Save. } - } - catch (OperationCanceledException) - { - return; // superseded by a newer change or an explicit Save. - } - finally - { - lock (_gate) + finally { - if (ReferenceEquals(_debounceCts, cts)) + lock (_gate) { - _debounceCts = null; + if (ReferenceEquals(_debounceCts, cts)) + { + _debounceCts = null; + } } } - cts.Dispose(); + await PersistGuardedAsync(persister).ConfigureAwait(false); } - - await PersistGuardedAsync(persister).ConfigureAwait(false); } private async Task PersistGuardedAsync(ISettingsPersister persister) @@ -239,10 +244,14 @@ private async Task PersistGuardedAsync(ISettingsPersister persister) { await persister.PersistAsync(this).ConfigureAwait(false); } - catch (Exception ex) + catch (Exception ex) when (ex is not OutOfMemoryException) { - // Never swallow: route to the configured handler (default - // writes to stderr). This is the fire-and-forget safety net. + // Route any write failure to the configured handler (default + // writes to stderr): this is the fire-and-forget safety net, so + // the persister's exception (whatever type it is) must not be + // lost. A process-fatal OutOfMemoryException is the exception — + // reporting it as a settings-write failure would be misleading, + // so it propagates instead. _errorHandler(ex); } } diff --git a/tests/NextIteration.SpectreConsole.Settings.Tests/CommandFlowTests.cs b/tests/NextIteration.SpectreConsole.Settings.Tests/CommandFlowTests.cs index fe6eccd..a606645 100644 --- a/tests/NextIteration.SpectreConsole.Settings.Tests/CommandFlowTests.cs +++ b/tests/NextIteration.SpectreConsole.Settings.Tests/CommandFlowTests.cs @@ -13,7 +13,7 @@ namespace NextIteration.SpectreConsole.Settings.Tests public sealed class CommandFlowTests { private static string FileFor(string directory) => - Path.Combine(directory, typeof(T).Name + ".json"); + Path.Join(directory, typeof(T).Name + ".json"); private static void Register(string directory, IServiceCollection services) => services.AddSettings(o => o.SettingsDirectory = directory); diff --git a/tests/NextIteration.SpectreConsole.Settings.Tests/Infrastructure/CliHarness.cs b/tests/NextIteration.SpectreConsole.Settings.Tests/Infrastructure/CliHarness.cs index 6ccaf6d..90da33a 100644 --- a/tests/NextIteration.SpectreConsole.Settings.Tests/Infrastructure/CliHarness.cs +++ b/tests/NextIteration.SpectreConsole.Settings.Tests/Infrastructure/CliHarness.cs @@ -37,7 +37,7 @@ public static async Task RunAsync( var app = new CommandApp(new TypeRegistrar(services)); app.Configure(config => config.AddSettingsBranch()); - var console = new TestConsole().Interactive(); + using var console = new TestConsole().Interactive(); foreach (var line in consoleInput) { console.Input.PushTextWithEnter(line); diff --git a/tests/NextIteration.SpectreConsole.Settings.Tests/Infrastructure/TempDir.cs b/tests/NextIteration.SpectreConsole.Settings.Tests/Infrastructure/TempDir.cs index 7e31ee8..7a3fe9b 100644 --- a/tests/NextIteration.SpectreConsole.Settings.Tests/Infrastructure/TempDir.cs +++ b/tests/NextIteration.SpectreConsole.Settings.Tests/Infrastructure/TempDir.cs @@ -9,7 +9,7 @@ namespace NextIteration.SpectreConsole.Settings.Tests.Infrastructure internal sealed class TempDir : IDisposable { public string Path { get; } = - System.IO.Path.Combine(System.IO.Path.GetTempPath(), "ni.scs.tests." + Guid.NewGuid().ToString("N")); + System.IO.Path.Join(System.IO.Path.GetTempPath(), "ni.scs.tests." + Guid.NewGuid().ToString("N")); public TempDir() { @@ -25,7 +25,7 @@ public void Dispose() Directory.Delete(Path, recursive: true); } } - catch + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) { // Best-effort cleanup. Stray scratch dirs in %TEMP% aren't a // problem — the OS reclaims temp eventually. diff --git a/tests/NextIteration.SpectreConsole.Settings.Tests/Persistence/AtomicFileTests.cs b/tests/NextIteration.SpectreConsole.Settings.Tests/Persistence/AtomicFileTests.cs index 619e8a7..250dcdf 100644 --- a/tests/NextIteration.SpectreConsole.Settings.Tests/Persistence/AtomicFileTests.cs +++ b/tests/NextIteration.SpectreConsole.Settings.Tests/Persistence/AtomicFileTests.cs @@ -11,7 +11,7 @@ public sealed class AtomicFileTests public async Task WriteAllTextAsync_WritesExpectedContent() { using var temp = new TempDir(); - var target = Path.Combine(temp.Path, "file.txt"); + var target = Path.Join(temp.Path, "file.txt"); await AtomicFile.WriteAllTextAsync(target, "hello", TestContext.Current.CancellationToken); @@ -22,7 +22,7 @@ public async Task WriteAllTextAsync_WritesExpectedContent() public async Task WriteAllTextAsync_NoTempFileLeftBehindAfterSuccess() { using var temp = new TempDir(); - var target = Path.Combine(temp.Path, "file.txt"); + var target = Path.Join(temp.Path, "file.txt"); await AtomicFile.WriteAllTextAsync(target, "hello", TestContext.Current.CancellationToken); @@ -34,7 +34,7 @@ public async Task WriteAllTextAsync_NoTempFileLeftBehindAfterSuccess() public async Task WriteAllTextAsync_OverwritesExisting() { using var temp = new TempDir(); - var target = Path.Combine(temp.Path, "file.txt"); + var target = Path.Join(temp.Path, "file.txt"); await File.WriteAllTextAsync(target, "original", TestContext.Current.CancellationToken); await AtomicFile.WriteAllTextAsync(target, "replaced", TestContext.Current.CancellationToken); @@ -46,7 +46,7 @@ public async Task WriteAllTextAsync_OverwritesExisting() public async Task WriteAllTextAsync_ConcurrentWriters_OneWinsNoStragglers() { using var temp = new TempDir(); - var target = Path.Combine(temp.Path, "file.txt"); + var target = Path.Join(temp.Path, "file.txt"); await Task.WhenAll( AtomicFile.WriteAllTextAsync(target, "writer-a", TestContext.Current.CancellationToken), diff --git a/tests/NextIteration.SpectreConsole.Settings.Tests/SettingsStoreTests.cs b/tests/NextIteration.SpectreConsole.Settings.Tests/SettingsStoreTests.cs index 970094d..3efb70e 100644 --- a/tests/NextIteration.SpectreConsole.Settings.Tests/SettingsStoreTests.cs +++ b/tests/NextIteration.SpectreConsole.Settings.Tests/SettingsStoreTests.cs @@ -24,7 +24,7 @@ private static ServiceProvider BuildProvider(string directory, PersistenceMode m .BuildServiceProvider(); private static string FileFor(string directory) => - Path.Combine(directory, typeof(T).Name + ".json"); + Path.Join(directory, typeof(T).Name + ".json"); [Fact] public void AddSettings_WithoutSettingsDirectory_Throws()