From e53391f6d63162bc22342829d96f2635b24ba67f Mon Sep 17 00:00:00 2001 From: Stuart Meeks Date: Sat, 22 Aug 2026 01:26:45 +0000 Subject: [PATCH] fix: resolve cs/dereferenced-value-may-be-null in ListCredentialsAsync MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The corrected buildless CodeQL analysis (adopted in #33) surfaced a cs/dereferenced-value-may-be-null alert on FileCredentialManager.cs: the provider-match guard was written `credential?.ProviderName.Equals(…) == true`, and CodeQL's flow analysis does not narrow `credential` to non-null through the `?.… == true` idiom, so the dereferences in the block body read as potential null accesses. The code was already safe (a null credential makes the guard false), but the guard is rewritten to `credential is not null && …` so the null state flows into the block — the whole body dereferences `credential` — letting both the compiler and CodeQL prove it safe. Behavior is unchanged. Build clean (0 warnings, TreatWarningsAsErrors on); 334 tests pass on net8.0 and net10.0. Co-Authored-By: Claude Opus 4.8 --- CHANGELOG.md | 7 +++++++ .../Persistence/FileCredentialManager.cs | 6 +++++- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 764a4f9..e2689f6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 every target framework at once, where autobuild could pick a single TFM and analyse half the code. Adopts the revised canonical `codeql.yml` verbatim; no query coverage changes. +- **Rewrote the provider-match guard in `FileCredentialManager.ListCredentialsAsync`** + from `credential?.ProviderName.Equals(…) == true` to an explicit + `credential is not null && …`. Behavior is identical, but the null state now flows into + the block body — which dereferences `credential` throughout — so both the compiler and + CodeQL can prove the accesses safe. Resolves the `cs/dereferenced-value-may-be-null` + alert the corrected buildless analysis surfaced (a genuine code fix, not a dismissal). + ## [1.0.0] — 2026-08-21 First stable release. Headline: whole-store **export/import** to move credentials diff --git a/src/NextIteration.SpectreConsole.Auth/Persistence/FileCredentialManager.cs b/src/NextIteration.SpectreConsole.Auth/Persistence/FileCredentialManager.cs index 751ba67..581845e 100644 --- a/src/NextIteration.SpectreConsole.Auth/Persistence/FileCredentialManager.cs +++ b/src/NextIteration.SpectreConsole.Auth/Persistence/FileCredentialManager.cs @@ -74,7 +74,11 @@ public async Task> ListCredentialsAsync(string pr // Defensive re-check: the glob should only match this // provider's files, but if a stray file sneaks in we want // to ignore it rather than report a mis-attributed row. - if (credential?.ProviderName.Equals(providerName, StringComparison.OrdinalIgnoreCase) == true) + // `is not null` (rather than `?.… == true`) so the null + // state flows into the block — the whole body dereferences + // `credential`, and this lets the analyzer prove it safe. + if (credential is not null && + credential.ProviderName.Equals(providerName, StringComparison.OrdinalIgnoreCase)) { var isSelected = selections.TryGetValue($"{credential.ProviderName}", out var selectedId) && selectedId.Equals(credential.AccountId, StringComparison.OrdinalIgnoreCase);