chore: resolve open CodeQL code-quality alerts - #26
Merged
Conversation
Genuine fixes across the code-scanning backlog (no suppression hacks): - Path.Combine -> Path.Join repo-wide (src + tests). Path.Join never treats a later segment as rooted, so it can't silently drop earlier arguments — a defence-in-depth improvement for the input-validated credential/keystore path construction. Resolves #24 and clears the cs/path-combine alerts. - cs/linq/missed-where: replaced filter-style loops with idiomatic LINQ — providerName.Any(...) in the three ValidateProviderName checks, Distinct(OrdinalIgnoreCase) for the accounts-list column-key union, and .Where(...) / .FirstOrDefault(predicate) in the Keychain backend. - cs/catch-of-all-exceptions (crypto translation): narrowed DpapiCredentialEncryption and LocalFileCredentialEncryption from catch (Exception) to the specific expected types (CryptographicException, FormatException, IOException, UnauthorizedAccessException). Unexpected exceptions now propagate rather than being masked. The deliberately-broad boundary catches (CLI command handlers, best-effort cleanup) and the two generated-code alerts in the xUnit entry point are handled by dismissal with justification, out of band from this PR. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Works toward the 1.0.0 "ducks in a row" goal by clearing the open code-scanning backlog. Every code change is a genuine fix, not a suppression.
Starting state: 52 open alerts — 29×
cs/path-combine, 15×cs/catch-of-all-exceptions, 6×cs/linq/missed-where, 2×cs/missed-ternary-operator.Fixed in this PR (39 alerts)
cs/path-combine(29) —Path.Combine→Path.Joinacrosssrc+tests(resolves Replace Path.Combine with Path.Join across the repo #24).Path.Joinnever treats a later segment as rooted, so it can't silently drop earlier arguments; defence-in-depth for the already-validated credential/keystore paths.cs/linq/missed-where(6) — filter-loops → idiomatic LINQ:providerName.Any(...)(×3ValidateProviderName),Distinct(OrdinalIgnoreCase)(list column-key union),.Where(...)+.FirstOrDefault(predicate)(Keychain).cs/catch-of-all-exceptions— crypto translation (4) — narrowed DPAPI +LocalFileCredentialEncryptioncatch (Exception)to the specific expected types (CryptographicException,FormatException,IOException,UnauthorizedAccessException). Unexpected exceptions now propagate instead of being masked as a generic failure.Handled by dismissal-with-justification (13 alerts, out of band — agreed approach)
AtomicFile.TryDelete,TempDir.Dispose, test cleanup). Catching everything here is the correct boundary pattern; narrowing would reintroduce crashes.cs/missed-ternary-operatorin the generatedobj/Release/.../XunitAutoGeneratedEntryPoint.cs— not our source, already excluded by the CodeQL config; stale alerts.Verification
334/334 tests pass on net8.0 + net10.0 locally. I'll confirm the 39 alerts flip to
fixedon this PR's CodeQL run before merging, then apply the 13 dismissals.🤖 Generated with Claude Code