From d835ced6ddb25914bff310ee05a4a26633c16b6a Mon Sep 17 00:00:00 2001 From: bgard68 <30295154+bgard68@users.noreply.github.com> Date: Mon, 24 Aug 2026 12:17:17 -0500 Subject: [PATCH] fix(tests): resolve the five CodeQL findings the coverage push introduced MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The zero-open-alerts state is an invariant now, so the new test code holds to the same bar as the product code. MemoryCache is IDisposable and was constructed per scanner without disposal; the test class now owns one per instance and disposes it. The fake handler built responses inside Task.FromResult, where the analyzer loses the ownership transfer — they are built in methods that return them directly, with the transfer documented. The key-directory cleanup swallowed every exception; only the two a filesystem delete can produce are caught now. 147 tests pass. Co-Authored-By: Claude Opus 5 --- .../GitHubInstallationTokenProviderTests.cs | 13 ++++- .../PublicRepositoryScannerTests.cs | 48 ++++++++++++------- 2 files changed, 43 insertions(+), 18 deletions(-) diff --git a/tests/DevSecOpsSentinel.Infrastructure.Tests/GitHubInstallationTokenProviderTests.cs b/tests/DevSecOpsSentinel.Infrastructure.Tests/GitHubInstallationTokenProviderTests.cs index c835576..35c1b08 100644 --- a/tests/DevSecOpsSentinel.Infrastructure.Tests/GitHubInstallationTokenProviderTests.cs +++ b/tests/DevSecOpsSentinel.Infrastructure.Tests/GitHubInstallationTokenProviderTests.cs @@ -145,6 +145,17 @@ await Assert.ThrowsAsync( public void Dispose() { - try { Directory.Delete(_keyDirectory, recursive: true); } catch { /* best effort */ } + // Best-effort cleanup of the per-test key directory; only the failures a + // filesystem delete can actually produce are worth swallowing. + try + { + Directory.Delete(_keyDirectory, recursive: true); + } + catch (IOException) + { + } + catch (UnauthorizedAccessException) + { + } } } diff --git a/tests/DevSecOpsSentinel.Infrastructure.Tests/PublicRepositoryScannerTests.cs b/tests/DevSecOpsSentinel.Infrastructure.Tests/PublicRepositoryScannerTests.cs index f6a8402..bb633a5 100644 --- a/tests/DevSecOpsSentinel.Infrastructure.Tests/PublicRepositoryScannerTests.cs +++ b/tests/DevSecOpsSentinel.Infrastructure.Tests/PublicRepositoryScannerTests.cs @@ -10,8 +10,12 @@ namespace DevSecOpsSentinel.Infrastructure.Tests; -public sealed class PublicRepositoryScannerTests +public sealed class PublicRepositoryScannerTests : IDisposable { + private readonly MemoryCache _cache = new(new MemoryCacheOptions()); + + public void Dispose() => _cache.Dispose(); + private const string VulnerableWorkflow = """ name: CI on: @@ -133,7 +137,7 @@ public async Task A_repository_with_a_workflows_directory_but_no_yml_files_repor Assert.Equal(PublicScanStatus.NoWorkflows, result.Status); } - private static PublicRepositoryScanner Scanner(FakeGitHub github) => + private PublicRepositoryScanner Scanner(FakeGitHub github) => new( github, new WorkflowAnalysisService( @@ -144,7 +148,7 @@ private static PublicRepositoryScanner Scanner(FakeGitHub github) => RuleDiscovery.All(), new NeverResolvesActionReferenceResolver(), new GitHubOptions())), - new MemoryCache(new MemoryCacheOptions()), + _cache, TimeProvider.System, NullLogger.Instance); @@ -193,27 +197,37 @@ protected override Task SendAsync( HttpRequestMessage request, CancellationToken cancellationToken) { + // Ownership of these responses transfers to the caller: the scanner + // disposes the listing via `using`, and GetStringAsync disposes the raw + // response internally. Built in methods that return them directly so the + // transfer is visible to analysis. string url = request.RequestUri!.ToString(); + return Task.FromResult(url.Contains("/contents/.github/workflows") + ? BuildListingResponse() + : BuildRawResponse(url)); + } - if (url.Contains("/contents/.github/workflows")) + private HttpResponseMessage BuildListingResponse() + { + fake.ListingRequests++; + HttpResponseMessage response = new(fake.ListingStatus) { - fake.ListingRequests++; - HttpResponseMessage response = new(fake.ListingStatus) - { - Content = new StringContent(fake._listingBody, Encoding.UTF8, "application/json") - }; - if (fake.RateLimitRemaining is not null) - { - response.Headers.Add("X-RateLimit-Remaining", fake.RateLimitRemaining); - } - - return Task.FromResult(response); + Content = new StringContent(fake._listingBody, Encoding.UTF8, "application/json") + }; + if (fake.RateLimitRemaining is not null) + { + response.Headers.Add("X-RateLimit-Remaining", fake.RateLimitRemaining); } + return response; + } + + private HttpResponseMessage BuildRawResponse(string url) + { fake.RawRequests.Add(url); - return Task.FromResult(fake._raw.TryGetValue(url, out string? content) + return fake._raw.TryGetValue(url, out string? content) ? new HttpResponseMessage(HttpStatusCode.OK) { Content = new StringContent(content) } - : new HttpResponseMessage(HttpStatusCode.NotFound)); + : new HttpResponseMessage(HttpStatusCode.NotFound); } } }