From 44c24b3f1d7adf1ac381f72087bf6480bb0b8364 Mon Sep 17 00:00:00 2001 From: Rex Lorenzo Date: Fri, 7 Aug 2026 19:29:02 -0700 Subject: [PATCH 1/6] fix(cms): bound the download-name regex with a timeout - treat a timed-out match as an unsafe name and fall back to the default, matching the fail-safe path every other rejection in the method takes --- web/Areas/CMS/Services/CmsFilePathSafety.cs | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/web/Areas/CMS/Services/CmsFilePathSafety.cs b/web/Areas/CMS/Services/CmsFilePathSafety.cs index a5d5f4e9e..702b8ff43 100644 --- a/web/Areas/CMS/Services/CmsFilePathSafety.cs +++ b/web/Areas/CMS/Services/CmsFilePathSafety.cs @@ -30,7 +30,9 @@ public static class CmsFilePathSafety "LPT1", "LPT2", "LPT3", "LPT4", "LPT5", "LPT6", "LPT7", "LPT8", "LPT9" }; - private static readonly Regex DisallowedFileNameChars = new(@"[^a-zA-Z0-9._\- ]", RegexOptions.Compiled); + // Bounds matching so a pathological filename cannot pin a request thread. + private static readonly Regex DisallowedFileNameChars = + new(@"[^a-zA-Z0-9._\- ]", RegexOptions.Compiled, TimeSpan.FromSeconds(1)); /// /// Returns a filename safe to use in a Content-Disposition response header. @@ -46,7 +48,17 @@ public static string SanitizeDownloadName(string? userInput) } var fileNamePart = StripPathComponents(userInput); - var filtered = DisallowedFileNameChars.Replace(fileNamePart, string.Empty).Trim(); + + string filtered; + try + { + filtered = DisallowedFileNameChars.Replace(fileNamePart, string.Empty).Trim(); + } + catch (RegexMatchTimeoutException) + { + // Fail safe: an unfiltered name must never reach the response header. + return DefaultDownloadName; + } // Reject names that collapse to only dots/spaces: ".", ".." etc. would // become "..zip" after the suffix step, which is traversal-shaped. From 5fb9338fa8be85cef1c7381830f8c4b19103b82c Mon Sep 17 00:00:00 2001 From: Rex Lorenzo Date: Fri, 7 Aug 2026 19:31:18 -0700 Subject: [PATCH 2/6] chore(analyzer): justify four security-hotspot findings - S2077: the interpolated SQL carries only the generated @jc parameter placeholders; every job code binds through AddWithValue - S5332: the CAS literal is an XML namespace identifier, not an endpoint - S4502: the error page is anonymous, binds one route int, mutates nothing - S4790: widen the existing CA5350 disable to Sonar's equivalent rule --- web/Areas/Effort/Services/ClinicalScheduleService.cs | 4 ++++ web/Areas/RAPS/Services/UinformService.cs | 4 ++-- web/Controllers/HomeController.cs | 9 +++++++++ 3 files changed, 15 insertions(+), 2 deletions(-) diff --git a/web/Areas/Effort/Services/ClinicalScheduleService.cs b/web/Areas/Effort/Services/ClinicalScheduleService.cs index 4b56245cb..e6959ec46 100644 --- a/web/Areas/Effort/Services/ClinicalScheduleService.cs +++ b/web/Areas/Effort/Services/ClinicalScheduleService.cs @@ -150,7 +150,11 @@ private async Task> QueryClinicalScheduleAsync( paramNames.Add(paramName); command.Parameters.AddWithValue(paramName, clinicalJobCodes[i]); } + // The interpolation injects only the "@jc0, @jc1, ..." placeholder names generated + // above; every job code value is bound through AddWithValue, never concatenated. +#pragma warning disable S2077 // Formatting SQL queries is security-sensitive command.CommandText = $"SELECT DISTINCT emplid FROM pps.dbo.vw_personJobPosition WHERE jobcode IN ({string.Join(", ", paramNames)})"; +#pragma warning restore S2077 await using var reader = await command.ExecuteReaderAsync(ct); while (await reader.ReadAsync(ct)) diff --git a/web/Areas/RAPS/Services/UinformService.cs b/web/Areas/RAPS/Services/UinformService.cs index b99d4cf04..03bf17213 100644 --- a/web/Areas/RAPS/Services/UinformService.cs +++ b/web/Areas/RAPS/Services/UinformService.cs @@ -250,9 +250,9 @@ private static string GetAuthSignature(HttpMethod method, string publicKey, int { string toSign = method.Method.ToUpper() + ":" + epochTime + ":" + publicKey; // Legacy API requires HMACSHA1 - third-party system constraint -#pragma warning disable CA5350 // Do Not Use Weak Cryptographic Algorithms +#pragma warning disable CA5350, S4790 // Do Not Use Weak Cryptographic Algorithms using var sha1 = new HMACSHA1(Encoding.ASCII.GetBytes(privateKey)); -#pragma warning restore CA5350 +#pragma warning restore CA5350, S4790 byte[] hashed = sha1.ComputeHash(Encoding.ASCII.GetBytes(toSign)); return Convert.ToBase64String(hashed); } diff --git a/web/Controllers/HomeController.cs b/web/Controllers/HomeController.cs index 65769ae8e..1f7d613b7 100644 --- a/web/Controllers/HomeController.cs +++ b/web/Controllers/HomeController.cs @@ -29,7 +29,11 @@ public class HomeController : AreaController private readonly AAUDContext _aAUDContext; private readonly RAPSContext _rapsContext; private readonly VIPERContext _viperContext; + // An XML namespace identifier, not a network endpoint. The scheme is part of the + // literal CAS responses are namespaced with; changing it stops the elements matching. +#pragma warning disable S5332 // Using http protocol is insecure private readonly XNamespace _ns = "http://www.yale.edu/tp/cas"; +#pragma warning restore S5332 private readonly IHttpClientFactory _clientFactory; private readonly CasSettings _settings; private readonly List _casAttributesToCapture = new() { "authenticationDate", "credentialType" }; @@ -210,7 +214,12 @@ public IActionResult ClearCache() [Route("/[action]")] [Route("/[action]/{statusCode:int}")] [AllowAnonymous] + // Anti-forgery is irrelevant here: the error page is anonymous, binds one int? route + // value, and mutates no state. Requiring a token would break the 404/500 handler, + // which is re-executed on requests that never carried one. +#pragma warning disable S4502 // Disabling CSRF protections is security-sensitive [IgnoreAntiforgeryToken] +#pragma warning restore S4502 [ResponseCache(Duration = 0, Location = ResponseCacheLocation.None, NoStore = true)] [SearchExclude] #pragma warning disable S6967 // Error handler uses simple route parameter, not form data requiring validation From 666f10c861c9d83f3cde440ed21f6429dd2fb7c8 Mon Sep 17 00:00:00 2001 From: Rex Lorenzo Date: Fri, 7 Aug 2026 19:34:53 -0700 Subject: [PATCH 3/6] refactor: fold Where predicates into the following Any Completes c5e5eef0, which folded the First/FirstOrDefault shapes and left every Any shape behind. All five receivers are EF DbSets, so the generated SQL is unchanged. --- web/Areas/CTS/Controllers/BundleCompetencyController.cs | 4 ++-- .../CTS/Controllers/BundleCompetencyGroupController.cs | 7 +++---- web/Areas/Students/Services/StudentList.cs | 3 +-- 3 files changed, 6 insertions(+), 8 deletions(-) diff --git a/web/Areas/CTS/Controllers/BundleCompetencyController.cs b/web/Areas/CTS/Controllers/BundleCompetencyController.cs index a3df4a6dc..27fcee8a2 100644 --- a/web/Areas/CTS/Controllers/BundleCompetencyController.cs +++ b/web/Areas/CTS/Controllers/BundleCompetencyController.cs @@ -22,12 +22,12 @@ public BundleCompetencyController(VIPERContext context) private bool BundleExists(int bundleId) { - return context.Bundles.Where(b => b.BundleId == bundleId).Any(); + return context.Bundles.Any(b => b.BundleId == bundleId); } private bool CompetencyExists(int competencyId) { - return context.Competencies.Where(b => b.CompetencyId == competencyId).Any(); + return context.Competencies.Any(b => b.CompetencyId == competencyId); } [HttpGet] diff --git a/web/Areas/CTS/Controllers/BundleCompetencyGroupController.cs b/web/Areas/CTS/Controllers/BundleCompetencyGroupController.cs index a9ed6330b..580faac87 100644 --- a/web/Areas/CTS/Controllers/BundleCompetencyGroupController.cs +++ b/web/Areas/CTS/Controllers/BundleCompetencyGroupController.cs @@ -22,16 +22,15 @@ public BundleCompetencyGroupController(VIPERContext context) private bool BundleExists(int bundleId) { - return context.Bundles.Where(b => b.BundleId == bundleId).Any(); + return context.Bundles.Any(b => b.BundleId == bundleId); } private bool SameNameExists(int bundleId, string name, int? bundleCompetencyGroupId = null) { return context.BundleCompetencyGroups - .Where(g => g.BundleId == bundleId + .Any(g => g.BundleId == bundleId && g.Name == name - && (bundleCompetencyGroupId == null || bundleCompetencyGroupId != g.BundleCompetencyGroupId)) - .Any(); + && (bundleCompetencyGroupId == null || bundleCompetencyGroupId != g.BundleCompetencyGroupId)); } [HttpGet] diff --git a/web/Areas/Students/Services/StudentList.cs b/web/Areas/Students/Services/StudentList.cs index e320fef73..fd5cc4e28 100644 --- a/web/Areas/Students/Services/StudentList.cs +++ b/web/Areas/Students/Services/StudentList.cs @@ -59,8 +59,7 @@ public async Task> GetStudents(string? classLevel = null, int? cla { //get all students that have a class year entry in this year q = q.Where(q => q.Student != null && _context.StudentClassYears - .Where(anyClassYear => anyClassYear.ClassYear == classYear && anyClassYear.PersonId == q.Student.PersonId) - .Any() + .Any(anyClassYear => anyClassYear.ClassYear == classYear && anyClassYear.PersonId == q.Student.PersonId) ); } From a7d903e96d2e8953efc4dce977796bc449ce48ae Mon Sep 17 00:00:00 2001 From: Rex Lorenzo Date: Fri, 7 Aug 2026 19:38:28 -0700 Subject: [PATCH 4/6] fix: honour request cancellation in streaming and proxy responses - pass context.RequestAborted to the sitemap writes, the Vite proxy body copy, the static-file fallback and the 502 body, so work stops when the client disconnects instead of running to completion - opt the two SSE Task.Run calls out explicitly with CancellationToken.None: a token there only prevents the task starting, which would skip the lambda catch and leave the channel un-completed --- .../Controllers/ClinicalImportController.cs | 8 +++++--- .../Controllers/PercentRolloverController.cs | 8 +++++--- web/Classes/SitemapMiddleware.cs | 11 +++++++++-- web/ViteProxyHelpers.cs | 16 +++++++++++++--- 4 files changed, 32 insertions(+), 11 deletions(-) diff --git a/web/Areas/Effort/Controllers/ClinicalImportController.cs b/web/Areas/Effort/Controllers/ClinicalImportController.cs index cfbdd89fe..7acc85be3 100644 --- a/web/Areas/Effort/Controllers/ClinicalImportController.cs +++ b/web/Areas/Effort/Controllers/ClinicalImportController.cs @@ -131,8 +131,10 @@ public async Task StreamImport( // Create a channel for progress events var channel = Channel.CreateUnbounded(); - // Start the import in a background task. - // Don't pass ct to Task.Run — cancellation is handled cooperatively inside the lambda. + // Start the import in a background task. CancellationToken.None is deliberate: a token + // here only stops the task ever starting, which would skip the lambda's own catch and + // leave the channel un-completed, hanging the SSE reader below. Cancellation is handled + // cooperatively via ct inside the lambda instead. var importTask = Task.Run(async () => { try @@ -160,7 +162,7 @@ public async Task StreamImport( { channel.Writer.TryComplete(); } - }); + }, CancellationToken.None); try { diff --git a/web/Areas/Effort/Controllers/PercentRolloverController.cs b/web/Areas/Effort/Controllers/PercentRolloverController.cs index a411f42c3..a3d91af88 100644 --- a/web/Areas/Effort/Controllers/PercentRolloverController.cs +++ b/web/Areas/Effort/Controllers/PercentRolloverController.cs @@ -91,8 +91,10 @@ public async Task StreamRollover(int year, CancellationToken ct) // Create a channel for progress events var channel = Channel.CreateUnbounded(); - // Start the rollover in a background task. - // Don't pass ct to Task.Run — cancellation is handled cooperatively inside the lambda. + // Start the rollover in a background task. CancellationToken.None is deliberate: a token + // here only stops the task ever starting, which would skip the lambda's own catch and + // leave the channel un-completed, hanging the SSE reader below. Cancellation is handled + // cooperatively via ct inside the lambda instead. var rolloverTask = Task.Run(async () => { try @@ -120,7 +122,7 @@ public async Task StreamRollover(int year, CancellationToken ct) { channel.Writer.TryComplete(); } - }); + }, CancellationToken.None); try { diff --git a/web/Classes/SitemapMiddleware.cs b/web/Classes/SitemapMiddleware.cs index 6c5f00556..0ae70bbb1 100644 --- a/web/Classes/SitemapMiddleware.cs +++ b/web/Classes/SitemapMiddleware.cs @@ -80,11 +80,18 @@ public async Task Invoke(HttpContext context) using (var memoryStream = new MemoryStream()) { var bytes = Encoding.UTF8.GetBytes(sitemapContent.ToString()); - await memoryStream.WriteAsync(bytes.AsMemory()); + await memoryStream.WriteAsync(bytes.AsMemory(), context.RequestAborted); memoryStream.Seek(0, SeekOrigin.Begin); - await memoryStream.CopyToAsync(stream, bytes.Length); + await memoryStream.CopyToAsync(stream, bytes.Length, context.RequestAborted); } } + // A disconnected client is not a generation failure. The response has + // already started by this point, so end the request instead of running + // the rest of the pipeline against it. + catch (OperationCanceledException) when (context.RequestAborted.IsCancellationRequested) + { + // Intentionally no fall-through to _next: the request is over. + } // Middleware boundary: any sitemap-generation failure (DB, IO, // reflection, etc.) must fall through to the pipeline rather than // break the request. diff --git a/web/ViteProxyHelpers.cs b/web/ViteProxyHelpers.cs index a1f8a00d7..81058dce1 100644 --- a/web/ViteProxyHelpers.cs +++ b/web/ViteProxyHelpers.cs @@ -319,7 +319,17 @@ public static async Task CopyProxyResponse(HttpContext context, HttpResponseMess } } - await response.Content.CopyToAsync(context.Response.Body); + try + { + await response.Content.CopyToAsync(context.Response.Body, context.RequestAborted); + } + catch (OperationCanceledException) when (context.RequestAborted.IsCancellationRequested) + { + // The client disconnected mid-stream. Swallow rather than rethrow: the caller + // catches TaskCanceledException as "Vite dev server unavailable" and falls + // through to the static-file branch, which would both log the wrong cause and + // write to a response that has already started. + } } /// @@ -392,7 +402,7 @@ public static async Task HandleProxyError(HttpContext context, Exception ex, ILo var contentType = GetContentType(Path.GetExtension(resolvedPhysical)); // Set content type if not already started context.Response.ContentType = contentType ?? "application/octet-stream"; - await context.Response.SendFileAsync(resolvedPhysical); + await context.Response.SendFileAsync(resolvedPhysical, context.RequestAborted); return; } } @@ -403,7 +413,7 @@ public static async Task HandleProxyError(HttpContext context, Exception ex, ILo } context.Response.StatusCode = 502; - await context.Response.WriteAsync("Vite dev server not running. Please start the frontend development server or build static files as appropriate."); + await context.Response.WriteAsync("Vite dev server not running. Please start the frontend development server or build static files as appropriate.", context.RequestAborted); } } From 3c9f50152ceb61fe9cb1b773b68b8bdf02633439 Mon Sep 17 00:00:00 2001 From: Rex Lorenzo Date: Fri, 7 Aug 2026 19:47:31 -0700 Subject: [PATCH 5/6] refactor: drop null-forgiving operators the compiler no longer needs 41 sites flagged by S8969 across 20 files, 21 production and 20 test. Most are a plain deletion; two spots needed the surrounding code to make the non-null state provable instead: - both clinical-import commits guard on ownsTransaction, which Sonar can follow but the compiler cannot; null-check the transaction itself, which is identical by construction - AddInstructor and SetPrimaryEvaluator capture their [Required] nullable fields into non-null locals behind an explicit guard, which also clears the CodeQL nullable-dereference findings on those lines --- test/Areas/Directory/VMACSServiceTest.cs | 2 +- test/CMS/CMSContentControllerTests.cs | 8 ++-- test/CMS/CMSFilesControllerTests.cs | 2 +- test/CMS/CMSLeftNavControllerTests.cs | 4 +- test/CMS/CMSOptionsControllerTests.cs | 2 +- .../PermissionsControllerTest.cs | 8 ++-- test/Effort/HarvestTimeParserTests.cs | 3 +- .../HealthCheckCollectorTokenHandlerTests.cs | 2 +- test/Scheduler/ScheduledJobRunnerTests.cs | 2 +- .../EmergencyContactControllerTests.cs | 6 +-- test/Students/EmergencyContactServiceTests.cs | 2 +- .../Controllers/BundleCompetencyController.cs | 4 +- .../BundleCompetencyGroupController.cs | 3 +- .../Models/CompetencyBundleAssociationDto.cs | 2 +- .../Controllers/CliniciansController.cs | 2 +- .../InstructorScheduleController.cs | 46 ++++++++++++++----- .../Services/BiorenderStudentLookup.cs | 4 +- .../Effort/Services/ClinicalImportService.cs | 16 ++++--- .../Services/Harvest/ClinicalHarvestPhase.cs | 2 +- .../Services/Harvest/CrestHarvestPhase.cs | 4 +- .../Effort/Services/InstructorService.cs | 4 +- web/Areas/RAPS/Services/OuGroupService.cs | 2 +- 22 files changed, 80 insertions(+), 50 deletions(-) diff --git a/test/Areas/Directory/VMACSServiceTest.cs b/test/Areas/Directory/VMACSServiceTest.cs index 68ccee68a..7039de4f0 100644 --- a/test/Areas/Directory/VMACSServiceTest.cs +++ b/test/Areas/Directory/VMACSServiceTest.cs @@ -61,7 +61,7 @@ public void Deserialize_ReturnsItem_WhenPayloadMatches() Assert.NotNull(query); Assert.NotNull(query.item); - Assert.Equal("Doe, John", query.item!.Name?.Single()); + Assert.Equal("Doe, John", query.item.Name?.Single()); } [Fact] diff --git a/test/CMS/CMSContentControllerTests.cs b/test/CMS/CMSContentControllerTests.cs index badcb4553..f24c807de 100644 --- a/test/CMS/CMSContentControllerTests.cs +++ b/test/CMS/CMSContentControllerTests.cs @@ -150,7 +150,7 @@ public async Task GetContentBlock_ReturnsBlock_WhenFound() var result = await _controller.GetContentBlock(5, TestContext.Current.CancellationToken); Assert.NotNull(result.Value); - Assert.Equal(5, result.Value!.ContentBlockId); + Assert.Equal(5, result.Value.ContentBlockId); } [Fact] @@ -273,7 +273,7 @@ public async Task CreateContentBlock_ReturnsBlock_OnSuccess() var result = await _controller.CreateContentBlock(request, TestContext.Current.CancellationToken); Assert.NotNull(result.Value); - Assert.Equal(7, result.Value!.ContentBlockId); + Assert.Equal(7, result.Value.ContentBlockId); } [Fact] @@ -286,7 +286,7 @@ public async Task CreateContentBlock_ReturnsValidationProblem_OnArgumentExceptio var result = await _controller.CreateContentBlock(request, TestContext.Current.CancellationToken); Assert.IsType(result.Result); - var problem = (ObjectResult)result.Result!; + var problem = (ObjectResult)result.Result; Assert.IsType(problem.Value); } @@ -313,7 +313,7 @@ public async Task UpdateContentBlock_ReturnsBlock_OnSuccess() var result = await _controller.UpdateContentBlock(3, request, TestContext.Current.CancellationToken); Assert.NotNull(result.Value); - Assert.Equal(3, result.Value!.ContentBlockId); + Assert.Equal(3, result.Value.ContentBlockId); } [Fact] diff --git a/test/CMS/CMSFilesControllerTests.cs b/test/CMS/CMSFilesControllerTests.cs index dade1b68e..3974e8c04 100644 --- a/test/CMS/CMSFilesControllerTests.cs +++ b/test/CMS/CMSFilesControllerTests.cs @@ -160,7 +160,7 @@ public async Task GetFile_ReturnsFile_WhenFound() var result = await _controller.GetFile(guid, TestContext.Current.CancellationToken); Assert.NotNull(result.Value); - Assert.Equal(guid, result.Value!.FileGuid); + Assert.Equal(guid, result.Value.FileGuid); } [Fact] diff --git a/test/CMS/CMSLeftNavControllerTests.cs b/test/CMS/CMSLeftNavControllerTests.cs index 5fa6cb2d4..e0c4fc245 100644 --- a/test/CMS/CMSLeftNavControllerTests.cs +++ b/test/CMS/CMSLeftNavControllerTests.cs @@ -56,7 +56,7 @@ public async Task GetMenu_ReturnsMenu_WhenFound() var result = await _controller.GetMenu(5, TestContext.Current.CancellationToken); Assert.NotNull(result.Value); - Assert.Equal(5, result.Value!.LeftNavMenuId); + Assert.Equal(5, result.Value.LeftNavMenuId); } [Fact] @@ -96,7 +96,7 @@ public async Task UpdateMenu_ReturnsMenu_OnSuccess() var result = await _controller.UpdateMenu(5, request, TestContext.Current.CancellationToken); Assert.NotNull(result.Value); - Assert.Equal(5, result.Value!.LeftNavMenuId); + Assert.Equal(5, result.Value.LeftNavMenuId); } [Fact] diff --git a/test/CMS/CMSOptionsControllerTests.cs b/test/CMS/CMSOptionsControllerTests.cs index 44ae5b1f0..6877b6dfd 100644 --- a/test/CMS/CMSOptionsControllerTests.cs +++ b/test/CMS/CMSOptionsControllerTests.cs @@ -70,7 +70,7 @@ public async Task SearchPeople_ReturnsEmpty_WhenSearchBelowMinimumLength(string? var result = await _controller.SearchPeople(search!, TestContext.Current.CancellationToken); Assert.NotNull(result.Value); - Assert.Empty(result.Value!); + Assert.Empty(result.Value); } [Fact] diff --git a/test/ClinicalScheduler/PermissionsControllerTest.cs b/test/ClinicalScheduler/PermissionsControllerTest.cs index 4d4b1b39e..7280cbff3 100644 --- a/test/ClinicalScheduler/PermissionsControllerTest.cs +++ b/test/ClinicalScheduler/PermissionsControllerTest.cs @@ -128,10 +128,10 @@ public async Task GetUserPermissions_WithValidUser_ReturnsOkWithPermissions() var okResult = Assert.IsType(result.Result); dynamic? response = okResult.Value; Assert.Equal(TestUserMothraId, response!.user.mothraId); - Assert.Equal(TestUserDisplayName, response!.user.displayName); - Assert.False(response!.permissions.hasManagePermission); - Assert.Equal(1, response!.permissions.editableServiceCount); - Assert.Single(response!.editableServices); + Assert.Equal(TestUserDisplayName, response.user.displayName); + Assert.False(response.permissions.hasManagePermission); + Assert.Equal(1, response.permissions.editableServiceCount); + Assert.Single(response.editableServices); } [Fact] diff --git a/test/Effort/HarvestTimeParserTests.cs b/test/Effort/HarvestTimeParserTests.cs index 02df15732..0898d1ec0 100644 --- a/test/Effort/HarvestTimeParserTests.cs +++ b/test/Effort/HarvestTimeParserTests.cs @@ -37,8 +37,7 @@ public void ParseTimeString_ValidTimeFormats_ParsesCorrectly(string input, int e { var result = HarvestTimeParser.ParseTimeString(input); - Assert.NotNull(result); - var value = result!.Value; + var value = Assert.IsType(result); Assert.Equal(expectedHour, value.Hours); Assert.Equal(expectedMinute, value.Minutes); } diff --git a/test/HealthChecks/HealthCheckCollectorTokenHandlerTests.cs b/test/HealthChecks/HealthCheckCollectorTokenHandlerTests.cs index 20fd159e9..6e7e201e2 100644 --- a/test/HealthChecks/HealthCheckCollectorTokenHandlerTests.cs +++ b/test/HealthChecks/HealthCheckCollectorTokenHandlerTests.cs @@ -16,7 +16,7 @@ public async Task SendAsync_StampsTokenHeader() Assert.Equal(HttpStatusCode.OK, response.StatusCode); Assert.NotNull(recorder.LastRequest); - var values = recorder.LastRequest!.Headers.GetValues(HealthCheckCollectorAuth.HeaderName).ToList(); + var values = recorder.LastRequest.Headers.GetValues(HealthCheckCollectorAuth.HeaderName).ToList(); Assert.Single(values); Assert.Equal(HealthCheckCollectorAuth.Token, values[0]); } diff --git a/test/Scheduler/ScheduledJobRunnerTests.cs b/test/Scheduler/ScheduledJobRunnerTests.cs index b850d9c44..a35caac40 100644 --- a/test/Scheduler/ScheduledJobRunnerTests.cs +++ b/test/Scheduler/ScheduledJobRunnerTests.cs @@ -48,7 +48,7 @@ public async Task RunAsync_ResolvesJobByIdAndStampsSchedulerActor() Assert.True(capturing.Ran); Assert.NotNull(capturing.CapturedContext); - Assert.Equal(ScheduledJobContext.SchedulerActor, capturing.CapturedContext!.ModBy); + Assert.Equal(ScheduledJobContext.SchedulerActor, capturing.CapturedContext.ModBy); } [Fact] diff --git a/test/Students/EmergencyContactControllerTests.cs b/test/Students/EmergencyContactControllerTests.cs index 2efc6b194..ac8fd9fe6 100644 --- a/test/Students/EmergencyContactControllerTests.cs +++ b/test/Students/EmergencyContactControllerTests.cs @@ -214,7 +214,7 @@ public async Task GetStudentContactDetail_StudentCannotViewOtherRecord_ReturnsFo var result = await _controller.GetStudentContactDetail(999); Assert.IsType(result.Result); - var objectResult = (ObjectResult)result.Result!; + var objectResult = (ObjectResult)result.Result; Assert.Equal(403, objectResult.StatusCode); } @@ -662,7 +662,7 @@ public void GetStudentContactDetail_HasAuthorizeAttributeButNoPermissionGate() var method = typeof(EmergencyContactController).GetMethod(nameof(EmergencyContactController.GetStudentContactDetail)); Assert.NotNull(method); - var permissionAttrs = method!.GetCustomAttributes(typeof(PermissionAttribute), false); + var permissionAttrs = method.GetCustomAttributes(typeof(PermissionAttribute), false); Assert.Empty(permissionAttrs); var authorizeAttrs = method.GetCustomAttributes(typeof(AuthorizeAttribute), false); @@ -676,7 +676,7 @@ public void CanEdit_HasAuthorizeAttributeButNoPermissionGate() var method = typeof(EmergencyContactController).GetMethod(nameof(EmergencyContactController.CanEdit)); Assert.NotNull(method); - var permissionAttrs = method!.GetCustomAttributes(typeof(PermissionAttribute), false); + var permissionAttrs = method.GetCustomAttributes(typeof(PermissionAttribute), false); Assert.Empty(permissionAttrs); var authorizeAttrs = method.GetCustomAttributes(typeof(AuthorizeAttribute), false); diff --git a/test/Students/EmergencyContactServiceTests.cs b/test/Students/EmergencyContactServiceTests.cs index 407f387ee..d21fd43bd 100644 --- a/test/Students/EmergencyContactServiceTests.cs +++ b/test/Students/EmergencyContactServiceTests.cs @@ -1387,7 +1387,7 @@ public async Task GetStudentContactReportAsync_IncludesContactDetails() Assert.Equal("95616", student.Zip); Assert.True(student.ContactPermanent); Assert.NotNull(student.EmergencyContact); - Assert.Equal("Jane Doe", student.EmergencyContact!.Name); + Assert.Equal("Jane Doe", student.EmergencyContact.Name); } } diff --git a/web/Areas/CTS/Controllers/BundleCompetencyController.cs b/web/Areas/CTS/Controllers/BundleCompetencyController.cs index 27fcee8a2..f7c7ecfc3 100644 --- a/web/Areas/CTS/Controllers/BundleCompetencyController.cs +++ b/web/Areas/CTS/Controllers/BundleCompetencyController.cs @@ -22,12 +22,12 @@ public BundleCompetencyController(VIPERContext context) private bool BundleExists(int bundleId) { - return context.Bundles.Any(b => b.BundleId == bundleId); + return context.Bundles.AsNoTracking().Any(b => b.BundleId == bundleId); } private bool CompetencyExists(int competencyId) { - return context.Competencies.Any(b => b.CompetencyId == competencyId); + return context.Competencies.AsNoTracking().Any(b => b.CompetencyId == competencyId); } [HttpGet] diff --git a/web/Areas/CTS/Controllers/BundleCompetencyGroupController.cs b/web/Areas/CTS/Controllers/BundleCompetencyGroupController.cs index 580faac87..2801e4c88 100644 --- a/web/Areas/CTS/Controllers/BundleCompetencyGroupController.cs +++ b/web/Areas/CTS/Controllers/BundleCompetencyGroupController.cs @@ -22,12 +22,13 @@ public BundleCompetencyGroupController(VIPERContext context) private bool BundleExists(int bundleId) { - return context.Bundles.Any(b => b.BundleId == bundleId); + return context.Bundles.AsNoTracking().Any(b => b.BundleId == bundleId); } private bool SameNameExists(int bundleId, string name, int? bundleCompetencyGroupId = null) { return context.BundleCompetencyGroups + .AsNoTracking() .Any(g => g.BundleId == bundleId && g.Name == name && (bundleCompetencyGroupId == null || bundleCompetencyGroupId != g.BundleCompetencyGroupId)); diff --git a/web/Areas/CTS/Models/CompetencyBundleAssociationDto.cs b/web/Areas/CTS/Models/CompetencyBundleAssociationDto.cs index a4bcfa387..e2e77832f 100644 --- a/web/Areas/CTS/Models/CompetencyBundleAssociationDto.cs +++ b/web/Areas/CTS/Models/CompetencyBundleAssociationDto.cs @@ -49,7 +49,7 @@ public CompetencyBundleAssociationDto(Competency c) .Where(bc => bc.Bundle != null) .Select(bc => new BundleInfoDto { - BundleId = bc.Bundle!.BundleId, + BundleId = bc.Bundle.BundleId, Name = bc.Bundle.Name, Clinical = bc.Bundle.Clinical, Assessment = bc.Bundle.Assessment, diff --git a/web/Areas/ClinicalScheduler/Controllers/CliniciansController.cs b/web/Areas/ClinicalScheduler/Controllers/CliniciansController.cs index 69be2975f..b0badf8f1 100644 --- a/web/Areas/ClinicalScheduler/Controllers/CliniciansController.cs +++ b/web/Areas/ClinicalScheduler/Controllers/CliniciansController.cs @@ -289,7 +289,7 @@ public async Task GetClinicianSchedule(string mothraId, [FromQuer isPrimaryEvaluator = schedule.Evaluator }; }) - .OrderBy(r => r!.name) // Sort rotations alphabetically + .OrderBy(r => r.name) // Sort rotations alphabetically .Cast() .ToArray() : Array.Empty(); diff --git a/web/Areas/ClinicalScheduler/Controllers/InstructorScheduleController.cs b/web/Areas/ClinicalScheduler/Controllers/InstructorScheduleController.cs index f38d36146..1c969242d 100644 --- a/web/Areas/ClinicalScheduler/Controllers/InstructorScheduleController.cs +++ b/web/Areas/ClinicalScheduler/Controllers/InstructorScheduleController.cs @@ -76,24 +76,36 @@ public async Task AddInstructor( correlationId)); } + // ValidateRequestAsync above already guarantees both are present. Capturing them + // as non-null locals makes every later use provable rather than asserted. + if (request.RotationId is not { } rotationId || request.GradYear is not { } gradYear) + { + return BadRequest(new ErrorResponse( + ErrorCodes.ValidationError, + "Please check your input and try again.", + correlationId)); + } + // Step 2: Check permissions - include own schedule check - if (!await CheckPermissionsForAddAsync(request.RotationId!.Value, request.MothraId!, correlationId, cancellationToken)) + if (!await CheckPermissionsForAddAsync(rotationId, request.MothraId, correlationId, cancellationToken)) { return Forbid(); } // Step 3: Check for conflicts and build warning message var warningMessage = await BuildConflictWarningAsync( - request.MothraId!, + request.MothraId, request.WeekIds, - request.GradYear!.Value, - request.RotationId!.Value, + gradYear, + rotationId, correlationId, cancellationToken); // Step 4: Add instructor through service layer var response = await ProcessAddInstructorAsync( request, + rotationId, + gradYear, warningMessage, correlationId, cancellationToken); @@ -176,16 +188,18 @@ private async Task CheckPermissionsForAddAsync( private async Task ProcessAddInstructorAsync( AddInstructorRequest request, + int rotationId, + int gradYear, string? warningMessage, string correlationId, CancellationToken cancellationToken) { // Add instructor to schedule var createdSchedules = await _scheduleEditService.AddInstructorAsync( - request.MothraId!, - request.RotationId!.Value, + request.MothraId, + rotationId, request.WeekIds, - request.GradYear!.Value, + gradYear, request.IsPrimaryEvaluator, cancellationToken); @@ -198,7 +212,7 @@ private async Task ProcessAddInstructorAsync( } _logger.LogInformation("Successfully added instructor to rotation {RotationId} for {WeekCount} weeks (CorrelationId: {CorrelationId})", - request.RotationId!.Value, request.WeekIds.Length, correlationId); + rotationId, request.WeekIds.Length, correlationId); return new AddInstructorResponse { @@ -388,8 +402,18 @@ public async Task SetPrimaryEvaluator( correlationId)); } + // [Required] plus the ModelState check above already guarantee this. Capturing it + // as a non-null local makes every later use provable rather than asserted. + if (request.IsPrimary is not { } isPrimary) + { + return BadRequest(new ErrorResponse( + ErrorCodes.ValidationError, + "Please check your input and try again.", + correlationId)); + } + var (success, previousPrimaryName) = await _scheduleEditService.SetPrimaryEvaluatorAsync( - instructorScheduleId, request.IsPrimary!.Value, cancellationToken, request.RequiresPrimaryEvaluator); + instructorScheduleId, isPrimary, cancellationToken, request.RequiresPrimaryEvaluator); if (!success) { @@ -399,14 +423,14 @@ public async Task SetPrimaryEvaluator( correlationId)); } - var action = request.IsPrimary!.Value ? "set as" : "removed as"; + var action = isPrimary ? "set as" : "removed as"; _logger.LogInformation("Successfully {Action} primary evaluator for instructor schedule {ScheduleId} (CorrelationId: {CorrelationId})", action, instructorScheduleId, correlationId); return Ok(new { message = $"Instructor successfully {action} primary evaluator", - isPrimaryEvaluator = request.IsPrimary!.Value, + isPrimaryEvaluator = isPrimary, previousPrimaryName }); } diff --git a/web/Areas/Computing/Services/BiorenderStudentLookup.cs b/web/Areas/Computing/Services/BiorenderStudentLookup.cs index 4eb6b5821..8aa4d111b 100644 --- a/web/Areas/Computing/Services/BiorenderStudentLookup.cs +++ b/web/Areas/Computing/Services/BiorenderStudentLookup.cs @@ -56,8 +56,10 @@ public async Task> GetBiorenderStudentInfo(List e ); } + // GetSingleStudent returns a constructed BiorenderStudent on every path, so there is + // nothing to filter out here. var taskResults = await Task.WhenAll(resultList); - return taskResults.Where(t => t != null).ToList()!; + return taskResults.ToList(); } /// diff --git a/web/Areas/Effort/Services/ClinicalImportService.cs b/web/Areas/Effort/Services/ClinicalImportService.cs index a4dc4b301..10bd7eb54 100644 --- a/web/Areas/Effort/Services/ClinicalImportService.cs +++ b/web/Areas/Effort/Services/ClinicalImportService.cs @@ -128,8 +128,8 @@ public async Task ValidateImportableInstructorsAsync( // (emeritus/recall appointments are excluded from harvest). var titleCodeByMothraId = aaudImportInfo .Where(a => !string.IsNullOrEmpty(a.TitleCode?.Trim()) - && _titleLookup.ContainsKey(a.TitleCode!.Trim()) - && !excludedTitleCodes.Contains(a.TitleCode!.Trim())) + && _titleLookup.ContainsKey(a.TitleCode.Trim()) + && !excludedTitleCodes.Contains(a.TitleCode.Trim())) .GroupBy(a => a.MothraId, StringComparer.OrdinalIgnoreCase) .ToDictionary(g => g.Key, g => g.First().TitleCode!.Trim(), StringComparer.OrdinalIgnoreCase); @@ -265,9 +265,11 @@ public async Task ExecuteImportAsync(int termCode, Clin await _context.SaveChangesAsync(ct); - if (ownsTransaction) + // Null-check the transaction rather than ownsTransaction: identical by construction + // above, but this is the form the compiler can actually prove. + if (transaction != null) { - await transaction!.CommitAsync(ct); + await transaction.CommitAsync(ct); } return result; @@ -341,9 +343,11 @@ public async Task ExecuteImportWithProgressAsync( await _context.SaveChangesAsync(ct); - if (ownsTransaction) + // Null-check the transaction rather than ownsTransaction: identical by construction + // above, but this is the form the compiler can actually prove. + if (transaction != null) { - await transaction!.CommitAsync(ct); + await transaction.CommitAsync(ct); } _logger.LogInformation( diff --git a/web/Areas/Effort/Services/Harvest/ClinicalHarvestPhase.cs b/web/Areas/Effort/Services/Harvest/ClinicalHarvestPhase.cs index fbba63a8a..151a21d3a 100644 --- a/web/Areas/Effort/Services/Harvest/ClinicalHarvestPhase.cs +++ b/web/Areas/Effort/Services/Harvest/ClinicalHarvestPhase.cs @@ -236,7 +236,7 @@ private async Task BuildClinicalInstructorPreviewsAsync( { // Only build previews for importable instructors (those with valid AAUD data + title code) var clinicalMothraIds = clinicalPersonData - .Where(c => !string.IsNullOrEmpty(c.MothraId) && importableMothraIds.Contains(c.MothraId!)) + .Where(c => !string.IsNullOrEmpty(c.MothraId) && importableMothraIds.Contains(c.MothraId)) .Select(c => c.MothraId!) .Distinct() .ToList(); diff --git a/web/Areas/Effort/Services/Harvest/CrestHarvestPhase.cs b/web/Areas/Effort/Services/Harvest/CrestHarvestPhase.cs index f9279c21b..26b4b880f 100644 --- a/web/Areas/Effort/Services/Harvest/CrestHarvestPhase.cs +++ b/web/Areas/Effort/Services/Harvest/CrestHarvestPhase.cs @@ -272,7 +272,7 @@ private static async Task> GetCrestInstructorsAsync( .Select(i => new { i.IdsMothraid, i.IdsPKey }) .ToListAsync(ct); - var pKeys = aaudIds.Where(i => !string.IsNullOrEmpty(i.IdsPKey)).Select(i => i.IdsPKey!).Distinct().ToList(); + var pKeys = aaudIds.Where(i => !string.IsNullOrEmpty(i.IdsPKey)).Select(i => i.IdsPKey).Distinct().ToList(); var employees = await context.AaudContext.Employees .AsNoTracking() @@ -311,7 +311,7 @@ private static async Task> GetCrestInstructorsAsync( var mothraIdToPKey = aaudIds .Where(i => !string.IsNullOrEmpty(i.IdsMothraid) && !string.IsNullOrEmpty(i.IdsPKey)) .GroupBy(i => i.IdsMothraid!, StringComparer.OrdinalIgnoreCase) - .ToDictionary(g => g.Key, g => g.Select(i => i.IdsPKey!).ToList(), StringComparer.OrdinalIgnoreCase); + .ToDictionary(g => g.Key, g => g.Select(i => i.IdsPKey).ToList(), StringComparer.OrdinalIgnoreCase); // Step 4: Build instructor details var instructorDetails = new List(); diff --git a/web/Areas/Effort/Services/InstructorService.cs b/web/Areas/Effort/Services/InstructorService.cs index 301c41333..28c6120e6 100644 --- a/web/Areas/Effort/Services/InstructorService.cs +++ b/web/Areas/Effort/Services/InstructorService.cs @@ -802,7 +802,7 @@ public async Task> BatchResolveDepartmentsAsync( var mothraIdToPKey = idsRecords .Where(i => !string.IsNullOrEmpty(i.IdsMothraid) && !string.IsNullOrEmpty(i.IdsPKey)) .GroupBy(i => i.IdsMothraid!, StringComparer.OrdinalIgnoreCase) - .ToDictionary(g => g.Key, g => g.First().IdsPKey!, StringComparer.OrdinalIgnoreCase); + .ToDictionary(g => g.Key, g => g.First().IdsPKey, StringComparer.OrdinalIgnoreCase); var pKeys = mothraIdToPKey.Values.Distinct().ToList(); @@ -1357,7 +1357,7 @@ public async Task> GetInstructorEffortRecordsAsy .Where(cr => cr.ChildCourse != null) .Select(cr => new ChildCourseDto { - Id = cr.ChildCourse!.Id, + Id = cr.ChildCourse.Id, SubjCode = cr.ChildCourse.SubjCode, CrseNumb = cr.ChildCourse.CrseNumb, SeqNumb = cr.ChildCourse.SeqNumb, diff --git a/web/Areas/RAPS/Services/OuGroupService.cs b/web/Areas/RAPS/Services/OuGroupService.cs index ea5ab6292..e2e10006d 100644 --- a/web/Areas/RAPS/Services/OuGroupService.cs +++ b/web/Areas/RAPS/Services/OuGroupService.cs @@ -251,7 +251,7 @@ private async Task CompareToGroup(string groupName, List members) //Create a lookup of loginids of members that should be in the group Dictionary membersInRoles = members .Where(m => m.LoginId != null) - .Select(m => m.LoginId!) + .Select(m => m.LoginId) .Distinct() .ToDictionary(id => id, _ => true); From f086392db9c5e12ef14346dff56ff8c5e0a19fd0 Mon Sep 17 00:00:00 2001 From: Rex Lorenzo Date: Sat, 8 Aug 2026 10:23:52 -0700 Subject: [PATCH 6/6] perf(students): preload class-year person ids for the student list The Any in GetStudents was correlated to every outer StudentClassYears row. Load the matching person ids once and match with EF.Parameter(...).Contains so the filter becomes a single IN list. --- web/Areas/Students/Services/StudentList.cs | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/web/Areas/Students/Services/StudentList.cs b/web/Areas/Students/Services/StudentList.cs index fd5cc4e28..32228eeeb 100644 --- a/web/Areas/Students/Services/StudentList.cs +++ b/web/Areas/Students/Services/StudentList.cs @@ -57,10 +57,16 @@ public async Task> GetStudents(string? classLevel = null, int? cla //include all class years, as long as at least one of them is the given year else if (classYear != null) { - //get all students that have a class year entry in this year - q = q.Where(q => q.Student != null && _context.StudentClassYears - .Any(anyClassYear => anyClassYear.ClassYear == classYear && anyClassYear.PersonId == q.Student.PersonId) - ); + //get all students that have a class year entry in this year. Pre-loading the ids + //keeps this a single IN list instead of a subquery correlated to every outer row. + var personIdsInClassYear = await _context.StudentClassYears + .AsNoTracking() + .Where(cy => cy.ClassYear == classYear) + .Select(cy => cy.PersonId) + .Distinct() + .ToListAsync(); + q = q.Where(studentClassYear => studentClassYear.Student != null + && EF.Parameter(personIdsInClassYear).Contains(studentClassYear.Student.PersonId)); } if (personId != null)