CodeQL 6: refactor: catch specific exceptions across CTS/RAPS/shared#194
Open
rlorenzo wants to merge 2 commits into
Open
CodeQL 6: refactor: catch specific exceptions across CTS/RAPS/shared#194rlorenzo wants to merge 2 commits into
rlorenzo wants to merge 2 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughThe PR narrows exception handling across 28 files by replacing broad ChangesException Handling Specificity
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Bundle ReportBundle size has no change ✅ |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #194 +/- ##
==========================================
- Coverage 43.02% 43.01% -0.02%
==========================================
Files 881 881
Lines 51437 51437
Branches 4812 4812
==========================================
- Hits 22131 22125 -6
- Misses 28780 28786 +6
Partials 526 526
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
4 tasks
rlorenzo
force-pushed
the
codeql/6-other-exceptions
branch
from
f559040 to
fd88995
Compare
rlorenzo
force-pushed
the
codeql/6-other-exceptions
branch
from
fd88995 to
d888563
Compare
CodeQL cs/catch-of-all-exceptions: same when-filter pattern as #193, applied across the remaining areas (~46 sites in 28 files): - CTS controllers (9 files) - ClinicalScheduler controllers (4 files) - RAPS controllers and services (5 files) - Computing service, shared web/Classes, web/Classes/Utilities (7 files) - web/Services/EmailService.cs, web/ViteProxyHelpers.cs, web/Program.cs Filter is restricted to DbUpdateException, SqlException, InvalidOperationException, OperationCanceledException (plus IOException for ViteProxyHelpers and FormatException/ArgumentException for the MailAddress email validator); anything outside that set now propagates. Two intentional broad catches kept with #pragma warning disable CA1031: - web/Program.cs top-level startup must catch any exception to log fatal before rethrowing. Bare `catch { }` in RoleMembersController (VMACS JSON parse) and BiorenderStudentLookup (MailAddress validation) tightened to the specific exceptions those calls actually throw.
…ndant qualifiers The codeql/6 batch script converted bare 'catch (Exception)' blocks to 'catch (Exception) when (true /* placeholder */)' to satisfy CodeQL. ReSharper rightly flagged that as CS7095 (filter is a constant). Replaced 14 of those with the standard when-filter pattern 'catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)' used in the rest of the PR. Also dropped Microsoft.EntityFrameworkCore. and Microsoft.Data.SqlClient. qualifiers in catch filters across 26 files; added the corresponding 'using' directives where missing.
rlorenzo
force-pushed
the
codeql/6-other-exceptions
branch
from
d888563 to
5e071de
Compare
Contributor
Author
|
@CodeRabbit full review |
Contributor
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Pull request overview
Refactors exception handling across CTS/ClinicalScheduler/RAPS/shared utilities to reduce CodeQL cs/catch-of-all-exceptions alerts by replacing broad catch (Exception)/catch { } blocks with more specific exception filters.
Changes:
- Narrowed many
catch (Exception)blocks usingwhen (ex is …)filters across controllers, middleware, and utility/services. - Tightened previously bare
catch { }blocks to explicit exception types (e.g., JSON parsing, email validation). - Added a top-level startup
CA1031suppression inProgram.csto keep a broad catch at the application boundary.
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| web/ViteProxyHelpers.cs | Narrows proxy/fallback exception handling. |
| web/Services/EmailService.cs | Narrows exception handling in service availability check. |
| web/Program.cs | Narrows several startup/runtime catches; adds CA1031 suppression for top-level startup catch. |
| web/Classes/Utilities/IamApi.cs | Narrows catch in IAM response parsing. |
| web/Classes/Utilities/F5HttpRequest.cs | Narrows catch around HttpClient send/fallback logic. |
| web/Classes/Utilities/ActiveDirectoryService.cs | Narrows catches around AD group membership mutations. |
| web/Classes/UserHelper.cs | Narrows catches around user lookup/caching helpers. |
| web/Classes/SitemapMiddleware.cs | Narrows catch in sitemap generation middleware. |
| web/Classes/ClaimsTransformer.cs | Narrows catch during claims/roles assignment. |
| web/Areas/RAPS/Services/VMACSExport.cs | Narrows catches for VMACS export HTTP call and response parsing. |
| web/Areas/RAPS/Services/OuGroupService.cs | Narrows catch around AD group retrieval. |
| web/Areas/RAPS/Controllers/RoleMembersController.cs | Tightens VMACS JSON parse from bare catch to JsonException. |
| web/Areas/RAPS/Controllers/RolesController.cs | Narrows DB update catch in role creation/update path. |
| web/Areas/RAPS/Controllers/AdGroupsController.cs | Narrows catch when creating AD groups. |
| web/Areas/Computing/Services/BiorenderStudentLookup.cs | Tightens email validation exception handling. |
| web/Areas/ClinicalScheduler/Controllers/CliniciansController.cs | Narrows catches before rethrowing to ApiExceptionFilter. |
| web/Areas/ClinicalScheduler/Controllers/InstructorScheduleController.cs | Narrows action-level catches for schedule mutations/queries. |
| web/Areas/ClinicalScheduler/Controllers/PermissionsController.cs | Narrows catches before rethrowing to ApiExceptionFilter. |
| web/Areas/ClinicalScheduler/Controllers/RotationsController.cs | Narrows catches before rethrowing to ApiExceptionFilter. |
| web/Areas/CTS/Controllers/BundleCompetencyController.cs | Narrows catch during DB mutation. |
| web/Areas/CTS/Controllers/BundleCompetencyGroupController.cs | Narrows catch during DB mutation. |
| web/Areas/CTS/Controllers/BundleController.cs | Narrows catch during DB mutation. |
| web/Areas/CTS/Controllers/CompetencyController.cs | Narrows catch during DB mutation. |
| web/Areas/CTS/Controllers/CourseController.cs | Narrows catch during DB mutation (multiple sites). |
| web/Areas/CTS/Controllers/DomainController.cs | Narrows catch during DB mutation. |
| web/Areas/CTS/Controllers/EpaController.cs | Narrows catch during DB mutation. |
| web/Areas/CTS/Controllers/LevelsController.cs | Narrows catch during DB mutation/transaction. |
| web/Areas/CTS/Controllers/RoleController.cs | Narrows catch during DB mutation. |
Comments suppressed due to low confidence (13)
web/ViteProxyHelpers.cs:376
- HandleProxyError’s fallback-to-static-files catch filter is too narrow/mismatched: filesystem and SendFileAsync paths can throw UnauthorizedAccessException (not an IOException), IOException/DirectoryNotFoundException, etc., but won’t throw DbUpdateException/SqlException. As written, some real fallback failures will now bubble out and skip the intended 502 response/logging. Catch the relevant IO/security exceptions here (and avoid catching OperationCanceledException so request aborts propagate cleanly).
try
{
// Determine the static file path
string? staticPath = null;
if (context.Request.Path.StartsWithSegments("/vue") || context.Request.Path.StartsWithSegments("/2/vue"))
{
staticPath = context.Request.Path.Value?.Replace("/2/vue", "/vue") ?? "/vue";
}
else
{
// Check if it's a Vue app route that should map to index.html
var pathValue = context.Request.Path.Value;
foreach (var appName in vueAppNames)
{
if (string.Equals(pathValue, $"/{appName}", StringComparison.OrdinalIgnoreCase) ||
pathValue?.StartsWith($"/{appName}/", StringComparison.OrdinalIgnoreCase) == true)
{
staticPath = $"/vue/src/{appName}/index.html";
break;
}
}
// If no app match, map as asset file
if (staticPath == null)
{
staticPath = "/vue/src" + context.Request.Path;
}
}
var physicalPath = Path.Join(context.RequestServices.GetRequiredService<IWebHostEnvironment>().WebRootPath,
staticPath.TrimStart('/').Replace('/', Path.DirectorySeparatorChar));
// Prevent directory traversal: ensure the resolved physical path is within WebRootPath
var webRoot = context.RequestServices.GetRequiredService<IWebHostEnvironment>().WebRootPath;
var resolvedPhysical = Path.GetFullPath(physicalPath);
var resolvedWebRoot = Path.GetFullPath(webRoot);
if (!resolvedPhysical.StartsWith(resolvedWebRoot, StringComparison.OrdinalIgnoreCase))
{
// Do not serve files outside the web root
logger.LogWarning("Attempt to access file outside web root: {Path}", resolvedPhysical);
}
else if (File.Exists(resolvedPhysical))
{
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);
return;
}
}
catch (Exception fileEx) when (fileEx is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException or IOException)
{
var safePath = context.Request.Path.ToString().Replace("\r", "").Replace("\n", "");
logger.LogWarning(fileEx, "Failed to serve static file fallback for {Path}", safePath);
}
web/Program.cs:460
- The Vite dev-server proxy catch is filtering for EF/SQL exceptions, but SendAsync/CopyProxyResponse failures will be HttpRequestException/IOException/etc. This means common “Vite not running” errors may no longer fall back to static files. Also, catching OperationCanceledException here can mis-handle request aborts as “Vite unavailable”. Catch the relevant HTTP/network exceptions and let cancellations propagate.
try
{
// Use the registered HttpClient from dependency injection
var httpClientFactory = context.RequestServices.GetRequiredService<IHttpClientFactory>();
var httpClient = httpClientFactory.CreateClient("ViteProxy");
// Build the Vite server URL and try to proxy directly
var viteUrl = ViteProxyHelpers.BuildViteUrl(context.Request.Path, context.Request.QueryString, VueAppNames);
var requestMessage = ViteProxyHelpers.CreateProxyRequest(context, viteUrl);
using var response = await httpClient.SendAsync(requestMessage, HttpCompletionOption.ResponseHeadersRead, context.RequestAborted);
// Copy the response back to the client
await ViteProxyHelpers.CopyProxyResponse(context, response);
return; // Successfully proxied, don't continue to static files
}
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
var logger = context.RequestServices.GetRequiredService<ILogger<Program>>();
logger.LogDebug(ex, "Vite server not available, falling back to static files for {Path}",
Uri.EscapeDataString(context.Request.Path.Value ?? "unknown"));
// Fall through to static file serving
}
web/Program.cs:590
- File.Delete(awsCredentialsFilePath) can throw IOException/UnauthorizedAccessException/DirectoryNotFoundException, but the current filter only matches EF/SQL/InvalidOperation/OperationCanceled. If deletion fails, the exception may now escape and fail startup unexpectedly. Catch the filesystem exceptions that File.Delete actually throws (and avoid catching OperationCanceledException here).
try
{
File.Delete(awsCredentialsFilePath);
}
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
logger.Error(ex, $"COULD NOT DELETE THE AWS CREDENTIALS XML FILE (\"{awsCredentialsFilePath}\"). The file will need to be deleted manually.");
logger.Error(ex, $"COULD NOT DELETE THE AWS CREDENTIALS XML FILE (\"{awsCredentialsFilePath}\"). The file will need to be deleted manually.");
}
web/Program.cs:590
- This catch logs the same error message twice, which can create duplicate alerts/noise. Remove the duplicate logger.Error call (or vary the messages if two distinct log entries are intended).
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
logger.Error(ex, $"COULD NOT DELETE THE AWS CREDENTIALS XML FILE (\"{awsCredentialsFilePath}\"). The file will need to be deleted manually.");
logger.Error(ex, $"COULD NOT DELETE THE AWS CREDENTIALS XML FILE (\"{awsCredentialsFilePath}\"). The file will need to be deleted manually.");
}
web/Areas/RAPS/Services/VMACSExport.cs:144
- The export HTTP call can fail with HttpRequestException/IOException/TaskCanceledException (network/F5 issues), but the current filter only matches EF/SQL exceptions, so failures may now escape without being recorded into the messages list. Catch the relevant HTTP/network exceptions for _f5HttpRequest.Send and let OperationCanceledException propagate if cancellation is supported.
try
{
using HttpResponseMessage response = await _f5HttpRequest.Send(request);
RecordMessage(messages, "Response Status: " + response.StatusCode);
VmacsResponse vmacsResponse = await ParseResponse(response);
RecordMessage(messages, JsonSerializer.Serialize(vmacsResponse));
}
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
HttpHelper.Logger.Log(LogLevel.Warn, ex);
RecordMessage(messages, "Error: " + ex.Message + " " + ex.StackTrace);
if (ex.InnerException != null)
{
RecordMessage(messages, "Error: " + ex.InnerException.Message + " " + ex.InnerException.StackTrace);
}
}
web/Areas/RAPS/Services/VMACSExport.cs:178
- VMACS response parsing errors will typically be JsonException/NotSupportedException from JsonSerializer.Deserialize; the current catch filter won’t match those, so invalid VMACS JSON can now throw instead of returning a structured error response. Catch the JSON parsing exceptions here (and avoid catching OperationCanceledException unless you rethrow it immediately).
private static async Task<VmacsResponse> ParseResponse(HttpResponseMessage response)
{
VmacsResponse? vmacsResponse;
string responseBody = await response.Content.ReadAsStringAsync();
try
{
vmacsResponse = JsonSerializer.Deserialize<VmacsResponse>(responseBody, new JsonSerializerOptions { PropertyNameCaseInsensitive = true });
if (vmacsResponse == null)
{
throw new InvalidOperationException("Failed to deserialize VMACS response.");
}
vmacsResponse.Success = response.IsSuccessStatusCode;
}
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
vmacsResponse = new()
{
Success = false,
ErrorMessage = "Invalid response from VMACS: " + responseBody
};
}
web/Areas/RAPS/Services/OuGroupService.cs:53
- ActiveDirectoryService.GetGroups() failures are likely directory/LDAP exceptions (e.g., PrincipalOperationException/LdapException), not EF/SQL exceptions. With the current filter, those errors will now bypass this handler and can crash the request. Update the filter to the AD/LDAP exception types actually thrown (and don’t catch OperationCanceledException unless rethrowing).
try
{
ActiveDirectoryService.GetGroups();
}
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
Logger logger = LogManager.GetCurrentClassLogger();
logger.Error(ex);
}
web/Classes/ClaimsTransformer.cs:111
- The catch filter excludes RecordNotFoundException, but the catch body explicitly checks for RecordNotFoundException to decide whether to log. As written, RecordNotFoundException will now bypass this handler and can break claims transformation/auth. Include RecordNotFoundException in the filter (and avoid catching OperationCanceledException here so request aborts don’t get logged/handled as 500s).
}
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
bool isProd = false;
if (HttpHelper.Environment != null)
{
isProd = HttpHelper.Environment.IsProduction();
}
// if were on the production system or the error is something other that RecordNotFoundException then log the error
// basically we don't want to be notified of RecordNotFound exception's except on production since users can be missing until the next db refresh
if (isProd || !(ex is RecordNotFoundException))
{
HttpHelper.Logger.Log(LogLevel.Error, ex, ex.Message);
}
}
web/Classes/UserHelper.cs:306
- These UserHelper catches now include OperationCanceledException and return null, which can convert request cancellation into “user not found” behavior. Also, the filter is very narrow for code that touches caching/DI/EF, so unexpected but recoverable exceptions may now escape. Prefer letting OperationCanceledException propagate, and ensure the filter matches the actual exception types you intend to handle for these helper methods.
try
{
AAUDContext? aaudContext = _aaudContext
?? (AAUDContext?)HttpHelper.HttpContext?.RequestServices.GetService(typeof(AAUDContext));
AaudUser? currentUser = GetByLoginId(aaudContext, HttpHelper.HttpContext?.User?.Identity?.Name);
return currentUser;
}
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
HttpHelper.Logger.Error(ex);
return null;
}
web/Areas/ClinicalScheduler/Controllers/RotationsController.cs:90
- Several controller-level catch filters include OperationCanceledException and then rethrow into ApiExceptionFilter, which will log the cancellation as an error and return a 500. Request aborts should generally propagate without being treated as server errors. Remove OperationCanceledException from these filters (or add a dedicated catch (OperationCanceledException) { throw; } before the filtered catch) throughout this controller.
_logger.LogDebug("Retrieved {Count} rotations, filtered to {FilteredCount} based on permissions",
rotations.Count, filteredRotations.Count);
return Ok(filteredRotations);
}
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
// Store context for ApiExceptionFilter to use in logging
SetExceptionContext(new Dictionary<string, object>
{
["ServiceId"] = serviceId,
["Operation"] = "RetrieveRotations"
});
throw; // Let ApiExceptionFilter handle the response
}
web/Areas/ClinicalScheduler/Controllers/PermissionsController.cs:163
- Several controller-level catch filters include OperationCanceledException and then rethrow into ApiExceptionFilter, which will log the cancellation as an error and return a 500. Request aborts should generally propagate without being treated as server errors. Remove OperationCanceledException from these filters (or add a dedicated catch (OperationCanceledException) { throw; } before the filtered catch) throughout this controller.
LogSanitizer.SanitizeId(user.MothraId), serviceId, canEdit);
return Ok(new { canEdit });
}
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
// Store context for ApiExceptionFilter to use in logging
SetExceptionContext("ServiceId", serviceId);
throw; // Let ApiExceptionFilter handle the response
}
}
web/Areas/ClinicalScheduler/Controllers/CliniciansController.cs:583
- Several controller-level catch filters include OperationCanceledException and then rethrow into ApiExceptionFilter, which will log the cancellation as an error and return a 500. Request aborts should generally propagate without being treated as server errors. Remove OperationCanceledException from these filters (or add a dedicated catch (OperationCanceledException) { throw; } before the filtered catch) throughout this controller.
// All other cases (Admin, Manage, EditClnSchedules, rotation view, or service-specific permissions) see all clinicians
return clinicians;
}
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
_logger.LogError(ex, "Error filtering clinicians by permissions. Returning unfiltered list.");
return clinicians; // Return unfiltered list on error to avoid breaking functionality
}
web/Services/EmailService.cs:228
- IsServiceAvailableAsync doesn’t perform EF/SQL operations, so this exception filter is unlikely to ever match real failures and it introduces unnecessary coupling to EF Core/SqlClient. Either remove the try/catch entirely (since IsMailpitAvailableAsync already returns false on failure) or catch the specific networking exceptions that can occur during availability checks.
public async Task<bool> IsServiceAvailableAsync()
{
try
{
// If using Mailpit in development, check if it's running
if (_hostEnvironment.IsDevelopment() && _emailSettings.UseMailpit)
{
return await IsMailpitAvailableAsync();
}
// For production, assume the SMTP server is available
return true;
}
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
_logger.LogError(ex, "Error checking email service availability");
return false;
}
Comment on lines
285
to
292
| try | ||
| { | ||
| context.Response.Headers[header.Key] = header.Value.ToArray(); | ||
| } | ||
| catch (Exception headerEx) | ||
| catch (Exception headerEx) when (headerEx is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException or IOException) | ||
| { | ||
| // Use structured logging for header errors | ||
| var safeHeaderKey = WebUtility.HtmlEncode(header.Key); |
Comment on lines
79
to
93
| try | ||
| { | ||
| // AWS Configurations | ||
| AWSOptions awsOptions = new() | ||
| { | ||
| Region = RegionEndpoint.USWest1 | ||
| }; | ||
| builder.Configuration | ||
| .AddSystemsManager("/" + builder.Environment.EnvironmentName, awsOptions) | ||
| .AddSystemsManager("/Shared", awsOptions); | ||
| } | ||
| catch (Exception ex) | ||
| catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException) | ||
| { | ||
| logger.Fatal(ex, "Failed to get secrets from AWS"); | ||
| } |
Comment on lines
20
to
30
| public async Task<HttpResponseMessage> Send(HttpRequestMessage request, int attemptNumber = 0) | ||
| { | ||
| HttpResponseMessage? response; | ||
| try | ||
| { | ||
| response = await _httpClient.SendAsync(request); | ||
| } | ||
| catch (Exception) | ||
| catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException) | ||
| { | ||
| response = await HandleConnectionFail(request, attemptNumber); | ||
| } |
| } | ||
| } | ||
| catch (Exception ex) | ||
| catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException) |
Comment on lines
211
to
227
| string creds = HttpHelper.GetSetting<string>("Credentials", "svmadgrp") ?? ""; | ||
| try | ||
| { | ||
| using PrincipalContext ad3Pc = new(ContextType.Domain, "ad3.ucdavis.edu", _username, creds); | ||
| using PrincipalContext ouPc = new(ContextType.Domain, "ou.ad3.ucdavis.edu", _username, creds); | ||
| GroupPrincipal? group = GroupPrincipal.FindByIdentity(ouPc, IdentityType.DistinguishedName, groupDn); | ||
| UserPrincipal? user = UserPrincipal.FindByIdentity(ad3Pc, IdentityType.DistinguishedName, userDn); | ||
| if (group != null && user != null) | ||
| { | ||
| group.Members.Add(user); | ||
| group.Save(); | ||
| } | ||
| } | ||
| catch (Exception ex) | ||
| catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException) | ||
| { | ||
| HttpHelper.Logger.Error(ex); | ||
| } |
Comment on lines
+90
to
91
| catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException) | ||
| { |
Comment on lines
+111
to
112
| catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException) | ||
| { |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
web/Areas/ClinicalScheduler/Controllers/CliniciansController.cs (1)
579-583:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail closed on permission-filter exceptions.
Returning
clinicianshere exposes unfiltered data when permission checks fail. Return an empty list (or rethrow) instead of fail-open behavior.Suggested fix
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException) { _logger.LogError(ex, "Error filtering clinicians by permissions. Returning unfiltered list."); - return clinicians; // Return unfiltered list on error to avoid breaking functionality + return new List<ClinicianSummary>(); // Fail closed on permission-evaluation errors }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/Areas/ClinicalScheduler/Controllers/CliniciansController.cs` around lines 579 - 583, The catch block that currently catches DbUpdateException/SqlException/InvalidOperationException/OperationCanceledException and calls _logger.LogError(...) must not fail open by returning the unfiltered variable clinicians; change the behavior to return an empty collection (e.g., Enumerable.Empty<Clinician>() or new List<Clinician>()) or rethrow the exception instead of returning clinicians to avoid exposing unfiltered data—update the catch clause in the CliniciansController method containing the catch (Exception ex) when (...) block and keep the _logger.LogError call but replace return clinicians with a safe empty-list return (or throw) as appropriate for your error-handling policy.web/Classes/ClaimsTransformer.cs (1)
95-107:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFilter breaks existing
RecordNotFoundExceptionhandling path.Line 95 narrows the catch so
RecordNotFoundExceptionis no longer reliably handled, but Lines 104–107 still assume it is. This can now bubble out of role-assignment flow unexpectedly. Add an explicitcatch (RecordNotFoundException)path (or restore intended behavior) before the other specific catches.As per coding guidelines, "
**/*.cs: Prefer specific exception types ... Flag broadExceptioncatches inside service/business logic."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/Classes/ClaimsTransformer.cs` around lines 95 - 107, The current catch in ClaimsTransformer narrows Exception to DbUpdateException/SqlException/InvalidOperationException/OperationCanceledException which prevents the existing RecordNotFoundException handling from running; add an explicit catch (RecordNotFoundException) block (before the broader catches) in the method that contains this try/catch so RecordNotFoundException is handled locally as intended (or restore the original broader catch ordering), then keep the existing production-check/logging logic in the other catch blocks (the block referencing HttpHelper.Environment and the isProd check) to ensure RecordNotFoundException no longer bubbles out of the role-assignment flow.web/Program.cs (2)
454-460:⚠️ Potential issue | 🟠 Major | ⚡ Quick winVite proxy fallback catch is too narrow; it won’t run on common HttpClient proxy failures.
In
web/Program.cs(around theHttpClient.SendAsynccatch), the filter only catchesDbUpdateException/SqlException/InvalidOperationException/OperationCanceledException, so typical network failures from a down/unreachable Vite server (e.g.,HttpRequestException, oftenIOException-wrapped) won’t trigger the “fall back to static files” path.Proposed fix
-catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException) +catch (Exception ex) when (ex is HttpRequestException or TaskCanceledException or OperationCanceledException or IOException or InvalidOperationException) { var logger = context.RequestServices.GetRequiredService<ILogger<Program>>(); logger.LogDebug(ex, "Vite server not available, falling back to static files for {Path}", Uri.EscapeDataString(context.Request.Path.Value ?? "unknown")); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/Program.cs` around lines 454 - 460, The catch block guarding the HttpClient.SendAsync fallback is too narrow and misses common network errors (e.g., HttpRequestException/IOException), so update the catch around the HttpClient.SendAsync call (the existing catch handling and logging using logger from context.RequestServices.GetRequiredService<ILogger<Program>> and the message "Vite server not available, falling back to static files for {Path}") to also catch HttpRequestException and IOException (and/or related network exceptions like SocketException) — or replace the filtered exception clause with a broader network-aware catch — then log the exception the same way and fall through to static file serving as before.
586-590:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix
File.Deletecatch filter inweb/Program.cs.
File.Delete(awsCredentialsFilePath)is caught only for DB/SQL/InvalidOperation/OperationCanceled exceptions, so filesystem failures (IO/permissions/path/security/etc.) can escape the handler. Catch the relevant filesystem exception types instead.Proposed fix
-catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException) +catch (Exception ex) when (ex is IOException or UnauthorizedAccessException or SecurityException or NotSupportedException or PathTooLongException) { logger.Error(ex, $"COULD NOT DELETE THE AWS CREDENTIALS XML FILE (\"{awsCredentialsFilePath}\"). The file will need to be deleted manually."); logger.Error(ex, $"COULD NOT DELETE THE AWS CREDENTIALS XML FILE (\"{awsCredentialsFilePath}\"). The file will need to be deleted manually."); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/Program.cs` around lines 586 - 590, The catch filter currently only checks for DB/SQL/InvalidOperation/OperationCanceled exceptions but the File.Delete(awsCredentialsFilePath) call can throw filesystem-related exceptions; update the when-clause on the catch for the File.Delete block to match filesystem exception types (e.g., IOException, UnauthorizedAccessException, PathTooLongException, NotSupportedException and SecurityException) so those failures are caught and logged by the existing logger.Error calls; locate the catch surrounding File.Delete in Program.cs (the catch with variable ex) and replace the type checks in the when clause accordingly (also remove the duplicate logger.Error if still present).web/Services/EmailService.cs (1)
224-228:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix email availability error handling: outer catch is irrelevant, inner probe swallows all exceptions.
IsServiceAvailableAsynccatchesDbUpdateException/SqlException, but when Mailpit is used the real probe isIsMailpitAvailableAsync, which has a barecatch { return false; }, so network/transport failures are already hidden before the outer filter can help.- Tighten
IsMailpitAvailableAsyncto catch specific exceptions (and log) instead of swallowing everything; then remove or narrow the outercatch (Exception ex) when (...)since it won’t cover the failures that matter.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/Services/EmailService.cs` around lines 224 - 228, IsServiceAvailableAsync's outer filtered catch is ineffective because IsMailpitAvailableAsync currently swallows all exceptions; update IsMailpitAvailableAsync to only catch specific exception types (e.g., HttpRequestException, TaskCanceledException/OperationCanceledException, SocketException or other transport/network exceptions you expect) and log the exception with _logger.LogError/LogWarning before returning false, and then remove or narrow the outer catch in IsServiceAvailableAsync (the catch (Exception ex) when (...) block) so it no longer tries to handle broad/irrelevant exceptions—this ensures network/transport failures from IsMailpitAvailableAsync surface properly and are logged.web/ViteProxyHelpers.cs (2)
372-376:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStatic-file fallback catch misses important file-system/security exceptions.
web/ViteProxyHelpers.cscurrently catchesExceptiononly forDbUpdateException | SqlException | InvalidOperationException | OperationCanceledException | IOException, so failures likeUnauthorizedAccessException,SecurityException, orNotSupportedExceptioncan bypass this handler and avoid the intended fallback response.Proposed fix
-catch (Exception fileEx) when (fileEx is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException or IOException) +catch (Exception fileEx) when (fileEx is IOException or UnauthorizedAccessException or SecurityException or NotSupportedException or PathTooLongException or InvalidOperationException) { var safePath = context.Request.Path.ToString().Replace("\r", "").Replace("\n", ""); logger.LogWarning(fileEx, "Failed to serve static file fallback for {Path}", safePath); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/ViteProxyHelpers.cs` around lines 372 - 376, The catch filter in ViteProxyHelpers (the catch(Exception fileEx) when (...) block) excludes critical filesystem/security exceptions so the static-file fallback can be skipped; update the exception filter to also include UnauthorizedAccessException, System.Security.SecurityException, NotSupportedException (and optionally PathTooLongException) so those exceptions are handled the same way, keeping the existing safePath sanitization and logger.LogWarning(fileEx, "Failed to serve static file fallback for {Path}", safePath) call; ensure you reference the same local variable fileEx and preserve the surrounding logic in the method where the catch block appears.
289-296:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix exception filters in
ViteProxyHelpersto match the header/file operations
- Header copy catch (around line 289) includes
DbUpdateException/SqlExceptionand omits common header-setting failures (e.g., argument/format), so those can bypass the log; drop DB/SQL types and include header-relevant exceptions.- Static-file fallback catch (around line 372) includes DB/SQL types and only filters
IOException/InvalidOperationException; drop DB/SQL types and include filesystem/access exceptions thatSendFileAsync/path resolution can throw.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/ViteProxyHelpers.cs` around lines 289 - 296, Update the exception filters in ViteProxyHelpers: in the header copy catch (the catch block handling headerEx used when setting response headers) remove DbUpdateException and SqlException and instead catch header-relevant errors such as ArgumentException and FormatException plus the existing InvalidOperationException, OperationCanceledException and IOException so header-setting failures are always logged; similarly update the static-file fallback catch (the catch around SendFileAsync/path resolution) to remove DbUpdateException and SqlException and include filesystem/access exceptions like FileNotFoundException, DirectoryNotFoundException, PathTooLongException, UnauthorizedAccessException, ArgumentException and FormatException in addition to IOException/InvalidOperationException so file/path errors are properly handled and logged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/Areas/Computing/Services/BiorenderStudentLookup.cs`:
- Line 98: In BiorenderStudentLookup.cs replace the filtered catch "catch
(Exception ex) when (ex is FormatException or ArgumentException)" with two
explicit catches—first "catch (FormatException ex)" and then "catch
(ArgumentException ex)"—inside the same method in class BiorenderStudentLookup,
keeping the existing error handling logic for both; if both handlers share
identical code, factor shared behavior into a small private helper method to
avoid duplication and ensure the original logging/response behavior remains
unchanged.
In `@web/Areas/CTS/Controllers/CompetencyController.cs`:
- Around line 180-183: The catch block in CompetencyController's delete handler
currently returns a misleading message "Could not remove domain..." when
catching
DbUpdateException/SqlException/InvalidOperationException/OperationCanceledException;
update the return BadRequest(...) call in that catch block to reference the
correct entity (e.g., "Could not remove competency. It may be linked to other
objects.") or use the appropriate localized/message constant so clients and logs
correctly state "competency" instead of "domain".
In `@web/Areas/CTS/Controllers/EpaController.cs`:
- Around line 109-112: The catch block in EpaController.cs that catches
DbUpdateException/SqlException/InvalidOperationException/OperationCanceledException
should not return raw ex.Message to clients; instead log the full exception
server-side (e.g., using the controller's ILogger or existing logging helper)
and return a fixed, non-sensitive BadRequest or Problem response (e.g., "An
error occurred processing your request."). Update the catch in the action method
to call logger.LogError(ex, "...") with contextual information and replace
return BadRequest(ex.Message) with a generic error response message.
In `@web/Areas/RAPS/Controllers/AdGroupsController.cs`:
- Around line 117-120: In the AdGroupsController catch block (the catch
(Exception ex) when (...) that currently returns ValidationProblem(ex.Message)),
stop returning raw exception text to clients: log the full exception via the
controller's logger (e.g., _logger.LogError(ex, "Error creating/updating ad
group")) and replace ValidationProblem(ex.Message) with a generic client-safe
response such as ValidationProblem("An error occurred while processing the
request.") or a ValidationProblemDetails instance containing a non-sensitive
message; preserve the original exception only in server logs for diagnostics.
In `@web/Areas/RAPS/Controllers/RolesController.cs`:
- Around line 226-228: In RolesController's catch block (the catch (Exception
ex) when (...) handler) remove appending ex.InnerException?.Message from the
Problem(...) response so the API returns a generic error like "There was a
problem updating the database."; instead log the full exception server-side
(e.g., _logger.LogError(ex, "Error updating roles") or similar) and return only
the generic Problem(...) call using the existing ex variable and Problem(...)
method to avoid leaking internal details.
In `@web/Areas/RAPS/Services/OuGroupService.cs`:
- Line 49: In OuGroupService replace the filtered catch "catch (Exception ex)
when (ex is DbUpdateException or SqlException or InvalidOperationException or
OperationCanceledException)" with explicit typed catch blocks: add separate
catches for DbUpdateException, SqlException, InvalidOperationException, and
OperationCanceledException in the method where the current catch appears (inside
the OuGroupService class), handling/logging each type as currently done for ex,
and avoid a broad catch(Exception); if a fallback is required, keep it minimal
(log and rethrow) but prefer the specific handlers.
In `@web/Areas/RAPS/Services/VMACSExport.cs`:
- Line 171: The catch filter in ParseResponse around the
JsonSerializer.Deserialize call currently only handles DbUpdateException,
SqlException, InvalidOperationException, and OperationCanceledException; update
that filter to also include JsonException (and optionally NotSupportedException)
so malformed VMACS JSON will be caught and the method can return the fallback
VmacsResponse; locate the JsonSerializer.Deserialize usage and the catch with
the filter in VMACSExport.ParseResponse and add the additional exception types
to the "ex is ..." list.
- Line 136: The catch block around the call to _f5HttpRequest.Send(request) in
VMACSExport.cs currently filters exceptions but omits HttpRequestException;
update the exception filter on the catch (currently "catch (Exception ex) when
(ex is DbUpdateException or SqlException or InvalidOperationException or
OperationCanceledException)") to also include HttpRequestException so
network/DNS/TLS failures are handled by this block (keep
OperationCanceledException for timeouts/cancellation and leave the other
exception types unchanged).
In `@web/Classes/SitemapMiddleware.cs`:
- Around line 90-93: The current catch in SitemapMiddleware that only catches
DbUpdateException/SqlException/InvalidOperationException/OperationCanceledException
prevents other sitemap-generation failures from falling through to
_next(context); change the handler to catch all exceptions at this middleware
boundary (e.g., catch(Exception ex)) so any runtime/reflection/type-loading
failures also await _next(context) and preserve fallback behavior; update the
catch block around the sitemap generation code (where the existing catch is
declared) and optionally add minimal logging inside the broad catch before
calling await _next(context).
In `@web/Classes/UserHelper.cs`:
- Line 275: Replace the filtered broad catch "catch (Exception ex) when (ex is
DbUpdateException or SqlException or InvalidOperationException or
OperationCanceledException)" used in the helper methods with explicit typed
catch blocks: add separate catches for DbUpdateException, SqlException,
InvalidOperationException, and OperationCanceledException (preserving the
exception variable name if used) and handle/log/rethrow each appropriately as
the original code intended; do this for every occurrence of that filtered catch
in UserHelper.cs (the three helper method catch sites) so each exception type is
caught explicitly rather than using a filtered Exception catch.
In `@web/Classes/Utilities/ActiveDirectoryService.cs`:
- Line 224: The catch filters in AddUserToGroup and RemoveUserFromGroup are
using DB/SQL exception types which are irrelevant to the
System.DirectoryServices.AccountManagement/LDAP operations; update the exception
filter to catch AD/LDAP-specific exceptions (e.g., PrincipalOperationException,
DirectoryServicesCOMException, LdapException and general
InvalidOperationException/OperationCanceledException if appropriate) and remove
DbUpdateException and SqlException so AD errors are correctly caught and
logged/handled by those methods.
In `@web/Classes/Utilities/F5HttpRequest.cs`:
- Around line 27-30: The retry catch currently filters
DbUpdateException/SqlException/InvalidOperationException which are irrelevant
for HttpClient; change the filter in the F5HttpRequest retry block so transport
failures trigger HandleConnectionFail by catching HttpRequestException and
keeping OperationCanceledException (to preserve timeout/cancellation behavior)
instead of the DB/SQL exception types — i.e., update the catch expression that
calls HandleConnectionFail(request, attemptNumber) to check for
HttpRequestException or OperationCanceledException.
In `@web/Classes/Utilities/IamApi.cs`:
- Around line 403-406: The exception filter in ParseResponse<T> currently
catches DbUpdateException, SqlException, InvalidOperationException, and
OperationCanceledException but misses JsonException and NotSupportedException so
JSON deserialize failures escape; update the catch in ParseResponse<T> to
include JsonException and NotSupportedException (or replace the filter with a
broader Exception catch for deserialize errors) and ensure the handler sets
r.ErrorMessage = "Invalid response: " + ex.Message + "." so
malformed/unsupported IAM payloads are converted into the error result.
In `@web/Program.cs`:
- Around line 90-93: The catch block that wraps
builder.Configuration.AddSystemsManager(...) is too narrow and lets non-matching
AWS/network exceptions escape; update the catch on the try surrounding
builder.Configuration.AddSystemsManager to either remove the restrictive
exception filter (catch Exception ex) or explicitly include
Amazon.Runtime.AmazonServiceException, Amazon.Runtime.AmazonClientException,
System.Net.Http.HttpRequestException and
TaskCanceledException/OperationCanceledException in the filter, then call
logger.Fatal(ex, "Failed to get secrets from AWS") as before and handle/rethrow
as appropriate; locate the catch block that currently references
DbUpdateException/SqlException and replace the filter with the recommended
exception types or a plain catch to ensure AWS/config/network errors are logged
and handled.
---
Outside diff comments:
In `@web/Areas/ClinicalScheduler/Controllers/CliniciansController.cs`:
- Around line 579-583: The catch block that currently catches
DbUpdateException/SqlException/InvalidOperationException/OperationCanceledException
and calls _logger.LogError(...) must not fail open by returning the unfiltered
variable clinicians; change the behavior to return an empty collection (e.g.,
Enumerable.Empty<Clinician>() or new List<Clinician>()) or rethrow the exception
instead of returning clinicians to avoid exposing unfiltered data—update the
catch clause in the CliniciansController method containing the catch (Exception
ex) when (...) block and keep the _logger.LogError call but replace return
clinicians with a safe empty-list return (or throw) as appropriate for your
error-handling policy.
In `@web/Classes/ClaimsTransformer.cs`:
- Around line 95-107: The current catch in ClaimsTransformer narrows Exception
to
DbUpdateException/SqlException/InvalidOperationException/OperationCanceledException
which prevents the existing RecordNotFoundException handling from running; add
an explicit catch (RecordNotFoundException) block (before the broader catches)
in the method that contains this try/catch so RecordNotFoundException is handled
locally as intended (or restore the original broader catch ordering), then keep
the existing production-check/logging logic in the other catch blocks (the block
referencing HttpHelper.Environment and the isProd check) to ensure
RecordNotFoundException no longer bubbles out of the role-assignment flow.
In `@web/Program.cs`:
- Around line 454-460: The catch block guarding the HttpClient.SendAsync
fallback is too narrow and misses common network errors (e.g.,
HttpRequestException/IOException), so update the catch around the
HttpClient.SendAsync call (the existing catch handling and logging using logger
from context.RequestServices.GetRequiredService<ILogger<Program>> and the
message "Vite server not available, falling back to static files for {Path}") to
also catch HttpRequestException and IOException (and/or related network
exceptions like SocketException) — or replace the filtered exception clause with
a broader network-aware catch — then log the exception the same way and fall
through to static file serving as before.
- Around line 586-590: The catch filter currently only checks for
DB/SQL/InvalidOperation/OperationCanceled exceptions but the
File.Delete(awsCredentialsFilePath) call can throw filesystem-related
exceptions; update the when-clause on the catch for the File.Delete block to
match filesystem exception types (e.g., IOException,
UnauthorizedAccessException, PathTooLongException, NotSupportedException and
SecurityException) so those failures are caught and logged by the existing
logger.Error calls; locate the catch surrounding File.Delete in Program.cs (the
catch with variable ex) and replace the type checks in the when clause
accordingly (also remove the duplicate logger.Error if still present).
In `@web/Services/EmailService.cs`:
- Around line 224-228: IsServiceAvailableAsync's outer filtered catch is
ineffective because IsMailpitAvailableAsync currently swallows all exceptions;
update IsMailpitAvailableAsync to only catch specific exception types (e.g.,
HttpRequestException, TaskCanceledException/OperationCanceledException,
SocketException or other transport/network exceptions you expect) and log the
exception with _logger.LogError/LogWarning before returning false, and then
remove or narrow the outer catch in IsServiceAvailableAsync (the catch
(Exception ex) when (...) block) so it no longer tries to handle
broad/irrelevant exceptions—this ensures network/transport failures from
IsMailpitAvailableAsync surface properly and are logged.
In `@web/ViteProxyHelpers.cs`:
- Around line 372-376: The catch filter in ViteProxyHelpers (the catch(Exception
fileEx) when (...) block) excludes critical filesystem/security exceptions so
the static-file fallback can be skipped; update the exception filter to also
include UnauthorizedAccessException, System.Security.SecurityException,
NotSupportedException (and optionally PathTooLongException) so those exceptions
are handled the same way, keeping the existing safePath sanitization and
logger.LogWarning(fileEx, "Failed to serve static file fallback for {Path}",
safePath) call; ensure you reference the same local variable fileEx and preserve
the surrounding logic in the method where the catch block appears.
- Around line 289-296: Update the exception filters in ViteProxyHelpers: in the
header copy catch (the catch block handling headerEx used when setting response
headers) remove DbUpdateException and SqlException and instead catch
header-relevant errors such as ArgumentException and FormatException plus the
existing InvalidOperationException, OperationCanceledException and IOException
so header-setting failures are always logged; similarly update the static-file
fallback catch (the catch around SendFileAsync/path resolution) to remove
DbUpdateException and SqlException and include filesystem/access exceptions like
FileNotFoundException, DirectoryNotFoundException, PathTooLongException,
UnauthorizedAccessException, ArgumentException and FormatException in addition
to IOException/InvalidOperationException so file/path errors are properly
handled and logged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fac47195-2b26-4625-8406-e8b749f064a0
📒 Files selected for processing (28)
web/Areas/CTS/Controllers/BundleCompetencyController.csweb/Areas/CTS/Controllers/BundleCompetencyGroupController.csweb/Areas/CTS/Controllers/BundleController.csweb/Areas/CTS/Controllers/CompetencyController.csweb/Areas/CTS/Controllers/CourseController.csweb/Areas/CTS/Controllers/DomainController.csweb/Areas/CTS/Controllers/EpaController.csweb/Areas/CTS/Controllers/LevelsController.csweb/Areas/CTS/Controllers/RoleController.csweb/Areas/ClinicalScheduler/Controllers/CliniciansController.csweb/Areas/ClinicalScheduler/Controllers/InstructorScheduleController.csweb/Areas/ClinicalScheduler/Controllers/PermissionsController.csweb/Areas/ClinicalScheduler/Controllers/RotationsController.csweb/Areas/Computing/Services/BiorenderStudentLookup.csweb/Areas/RAPS/Controllers/AdGroupsController.csweb/Areas/RAPS/Controllers/RoleMembersController.csweb/Areas/RAPS/Controllers/RolesController.csweb/Areas/RAPS/Services/OuGroupService.csweb/Areas/RAPS/Services/VMACSExport.csweb/Classes/ClaimsTransformer.csweb/Classes/SitemapMiddleware.csweb/Classes/UserHelper.csweb/Classes/Utilities/ActiveDirectoryService.csweb/Classes/Utilities/F5HttpRequest.csweb/Classes/Utilities/IamApi.csweb/Program.csweb/Services/EmailService.csweb/ViteProxyHelpers.cs
| return addr.Address == trimmed; | ||
| } | ||
| catch | ||
| catch (Exception ex) when (ex is FormatException or ArgumentException) |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use explicit exception catches for email validation.
Line 98 should be split into catch (FormatException) and catch (ArgumentException) instead of filtered catch (Exception).
As per coding guidelines, "**/*.cs: Prefer specific exception types ... Flag broad Exception catches inside service/business logic."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/Areas/Computing/Services/BiorenderStudentLookup.cs` at line 98, In
BiorenderStudentLookup.cs replace the filtered catch "catch (Exception ex) when
(ex is FormatException or ArgumentException)" with two explicit catches—first
"catch (FormatException ex)" and then "catch (ArgumentException ex)"—inside the
same method in class BiorenderStudentLookup, keeping the existing error handling
logic for both; if both handlers share identical code, factor shared behavior
into a small private helper method to avoid duplication and ensure the original
logging/response behavior remains unchanged.
Comment on lines
+180
to
183
| catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException) | ||
| { | ||
| return BadRequest("Could not remove domain. It may be linked to other objects."); | ||
| } |
Contributor
There was a problem hiding this comment.
Fix incorrect entity name in delete error message.
The delete-competency path returns "Could not remove domain...", which is misleading for clients and logs.
Suggested fix
- return BadRequest("Could not remove domain. It may be linked to other objects.");
+ return BadRequest("Could not remove competency. It may be linked to other objects.");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException) | |
| { | |
| return BadRequest("Could not remove domain. It may be linked to other objects."); | |
| } | |
| catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException) | |
| { | |
| return BadRequest("Could not remove competency. It may be linked to other objects."); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/Areas/CTS/Controllers/CompetencyController.cs` around lines 180 - 183,
The catch block in CompetencyController's delete handler currently returns a
misleading message "Could not remove domain..." when catching
DbUpdateException/SqlException/InvalidOperationException/OperationCanceledException;
update the return BadRequest(...) call in that catch block to reference the
correct entity (e.g., "Could not remove competency. It may be linked to other
objects.") or use the appropriate localized/message constant so clients and logs
correctly state "competency" instead of "domain".
Comment on lines
+109
to
112
| catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException) | ||
| { | ||
| return BadRequest(ex.Message); | ||
| } |
Contributor
There was a problem hiding this comment.
Do not expose raw exception messages in API responses.
BadRequest(ex.Message) can leak internal DB/runtime details to clients. Return a fixed message and log server-side details instead.
Suggested fix
- return BadRequest(ex.Message);
+ return BadRequest("Could not delete EPA. It may be linked to other objects.");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException) | |
| { | |
| return BadRequest(ex.Message); | |
| } | |
| catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException) | |
| { | |
| return BadRequest("Could not delete EPA. It may be linked to other objects."); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/Areas/CTS/Controllers/EpaController.cs` around lines 109 - 112, The catch
block in EpaController.cs that catches
DbUpdateException/SqlException/InvalidOperationException/OperationCanceledException
should not return raw ex.Message to clients; instead log the full exception
server-side (e.g., using the controller's ILogger or existing logging helper)
and return a fixed, non-sensitive BadRequest or Problem response (e.g., "An
error occurred processing your request."). Update the catch in the action method
to call logger.LogError(ex, "...") with contextual information and replace
return BadRequest(ex.Message) with a generic error response message.
Comment on lines
+117
to
120
| catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException) | ||
| { | ||
| //Exception message could be indication user is trying to create a group that exists or is invalid. | ||
| return ValidationProblem(ex.Message); |
Contributor
There was a problem hiding this comment.
Do not return raw exception messages to clients.
ValidationProblem(ex.Message) leaks internal details from database/directory failures. Return a generic client message and keep exception details in server logs.
Suggested fix
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
- //Exception message could be indication user is trying to create a group that exists or is invalid.
- return ValidationProblem(ex.Message);
+ // Log details server-side; return a safe client message.
+ return ValidationProblem("Unable to create group. Verify the request and try again.");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException) | |
| { | |
| //Exception message could be indication user is trying to create a group that exists or is invalid. | |
| return ValidationProblem(ex.Message); | |
| catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException) | |
| { | |
| // Log details server-side; return a safe client message. | |
| return ValidationProblem("Unable to create group. Verify the request and try again."); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/Areas/RAPS/Controllers/AdGroupsController.cs` around lines 117 - 120, In
the AdGroupsController catch block (the catch (Exception ex) when (...) that
currently returns ValidationProblem(ex.Message)), stop returning raw exception
text to clients: log the full exception via the controller's logger (e.g.,
_logger.LogError(ex, "Error creating/updating ad group")) and replace
ValidationProblem(ex.Message) with a generic client-safe response such as
ValidationProblem("An error occurred while processing the request.") or a
ValidationProblemDetails instance containing a non-sensitive message; preserve
the original exception only in server logs for diagnostics.
Comment on lines
+226
to
228
| catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException) | ||
| { | ||
| return Problem("There was a problem updating the database. " + ex.InnerException?.Message); |
Contributor
There was a problem hiding this comment.
Remove InnerException text from API error responses.
Appending ex.InnerException?.Message can expose internal SQL/stack details. Return a generic error payload.
Suggested fix
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
- return Problem("There was a problem updating the database. " + ex.InnerException?.Message);
+ return Problem("There was a problem updating the database.");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException) | |
| { | |
| return Problem("There was a problem updating the database. " + ex.InnerException?.Message); | |
| catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException) | |
| { | |
| return Problem("There was a problem updating the database."); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/Areas/RAPS/Controllers/RolesController.cs` around lines 226 - 228, In
RolesController's catch block (the catch (Exception ex) when (...) handler)
remove appending ex.InnerException?.Message from the Problem(...) response so
the API returns a generic error like "There was a problem updating the
database."; instead log the full exception server-side (e.g.,
_logger.LogError(ex, "Error updating roles") or similar) and return only the
generic Problem(...) call using the existing ex variable and Problem(...) method
to avoid leaking internal details.
| } | ||
| } | ||
| catch (Exception ex) | ||
| catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException) |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Replace filtered broad catches with explicit typed catches across helper methods.
Lines 275, 302, and 334 should use explicit catch blocks for the intended types instead of catch (Exception) when (...).
As per coding guidelines, "**/*.cs: Prefer specific exception types ... Flag broad Exception catches inside service/business logic."
Also applies to: 302-302, 334-334
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/Classes/UserHelper.cs` at line 275, Replace the filtered broad catch
"catch (Exception ex) when (ex is DbUpdateException or SqlException or
InvalidOperationException or OperationCanceledException)" used in the helper
methods with explicit typed catch blocks: add separate catches for
DbUpdateException, SqlException, InvalidOperationException, and
OperationCanceledException (preserving the exception variable name if used) and
handle/log/rethrow each appropriately as the original code intended; do this for
every occurrence of that filtered catch in UserHelper.cs (the three helper
method catch sites) so each exception type is caught explicitly rather than
using a filtered Exception catch.
| } | ||
| } | ||
| catch (Exception ex) | ||
| catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="web/Classes/Utilities/ActiveDirectoryService.cs"
echo "=== File exists? ==="
ls -la "$FILE" || exit 1
echo "=== Relevant sections around lines 200-270 ==="
nl -ba "$FILE" | sed -n '180,270p'
echo "=== Find catch blocks with 'catch (Exception' in this file ==="
rg -n "catch\s*\(\s*Exception" "$FILE" || true
echo "=== Find DirectoryServices / Principal / DirectoryEntry usage in this file ==="
rg -n "(DirectoryServices|PrincipalContext|DirectoryEntry|PrincipalSearcher|UserPrincipal|GroupPrincipal|Auth|LDAP|Ldap|System\.DirectoryServices)" "$FILE" || trueRepository: ucdavis/VIPER
Length of output: 284
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="web/Classes/Utilities/ActiveDirectoryService.cs"
echo "=== cat -n available? ==="
command -v cat >/dev/null && echo "cat ok"
command -v awk >/dev/null && echo "awk ok"
command -v rg >/dev/null && echo "rg ok"
echo "=== Relevant sections around ~200-270 (with line numbers via awk) ==="
awk 'NR>=180 && NR<=270 {printf "%6d:%s\n", NR, $0}' "$FILE"
echo "=== Find catch blocks with 'catch (Exception' in this file ==="
rg -n "catch\s*\(\s*Exception" "$FILE" || true
echo "=== Find DirectoryServices / Principal / DirectoryEntry usage in this file ==="
rg -n "(DirectoryServices|PrincipalContext|DirectoryEntry|PrincipalSearcher|UserPrincipal|GroupPrincipal|Ldap|System\.DirectoryServices|LDAP)" "$FILE" || trueRepository: ucdavis/VIPER
Length of output: 8357
🏁 Script executed:
FILE="web/Classes/Utilities/ActiveDirectoryService.cs"
awk 'NR>=300 && NR<=430 {printf "%6d:%s\n", NR, $0}' "$FILE"Repository: ucdavis/VIPER
Length of output: 3524
Fix exception filter mismatch in Active Directory code (DB/SQL exceptions are irrelevant here).
- In
ActiveDirectoryService.AddUserToGroup(line 224) andRemoveUserFromGroup(line 250), thecatch (Exception ex) when (ex is DbUpdateException or SqlException ...)filter doesn’t match theSystem.DirectoryServices.AccountManagement/LDAP operations being performed; update it to AD/LDAP-specific exceptions (e.g.,PrincipalOperationException,DirectoryServicesCOMException,LdapException) and remove the DB/SQL types so AD failures are properly logged/handled.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/Classes/Utilities/ActiveDirectoryService.cs` at line 224, The catch
filters in AddUserToGroup and RemoveUserFromGroup are using DB/SQL exception
types which are irrelevant to the
System.DirectoryServices.AccountManagement/LDAP operations; update the exception
filter to catch AD/LDAP-specific exceptions (e.g., PrincipalOperationException,
DirectoryServicesCOMException, LdapException and general
InvalidOperationException/OperationCanceledException if appropriate) and remove
DbUpdateException and SqlException so AD errors are correctly caught and
logged/handled by those methods.
Comment on lines
+27
to
30
| catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException) | ||
| { | ||
| response = await HandleConnectionFail(request, attemptNumber); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and print the relevant section of the file
FILE="web/Classes/Utilities/F5HttpRequest.cs"
if [ -f "$FILE" ]; then
echo "== File exists: $FILE =="
# Print around the specified line range (27-30) and a bit more context
START=1
END=80
nl -ba "$FILE" | sed -n "${START},${END}p"
else
echo "== File not found: $FILE =="
# Try a broader search by filename
fd -i "F5HttpRequest.cs" . -a || true
fiRepository: ucdavis/VIPER
Length of output: 177
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="web/Classes/Utilities/F5HttpRequest.cs"
echo "== Showing file $FILE (lines 1-140) =="
python3 - <<'PY'
import itertools,sys
path="web/Classes/Utilities/F5HttpRequest.cs"
with open(path,'r',encoding='utf-8') as f:
for i,line in enumerate(f, start=1):
if i>140: break
print(f"{i:4d}:{line.rstrip()}")
PY
echo
echo "== Catch blocks containing 'catch' in $FILE =="
rg -n "catch\s*\(" "$FILE" || true
echo
echo "== Any mention of HttpRequestException / TaskCanceledException in $FILE =="
rg -n "HttpRequestException|TaskCanceledException" "$FILE" || true
echo
echo "== HandleConnectionFail usages in $FILE =="
rg -n "HandleConnectionFail" "$FILE" || trueRepository: ucdavis/VIPER
Length of output: 5101
Retry logic catch block misses HttpClient transport failures (HttpRequestException)
The retry catch filter includes DbUpdateException/SqlException/InvalidOperationException + OperationCanceledException, but not HttpRequestException—so connection/send failures won’t trigger HandleConnectionFail. Catch HttpRequestException for transport errors, and keep OperationCanceledException (already covers TaskCanceledException/HttpClient timeouts if that’s the intent); remove the DB/SQL exception families from this HttpClient path.
Current catch filter
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
response = await HandleConnectionFail(request, attemptNumber);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/Classes/Utilities/F5HttpRequest.cs` around lines 27 - 30, The retry catch
currently filters DbUpdateException/SqlException/InvalidOperationException which
are irrelevant for HttpClient; change the filter in the F5HttpRequest retry
block so transport failures trigger HandleConnectionFail by catching
HttpRequestException and keeping OperationCanceledException (to preserve
timeout/cancellation behavior) instead of the DB/SQL exception types — i.e.,
update the catch expression that calls HandleConnectionFail(request,
attemptNumber) to check for HttpRequestException or OperationCanceledException.
Comment on lines
+403
to
406
| catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException) | ||
| { | ||
| r.ErrorMessage = "Invalid response: " + ex.Message + "."; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify catch filter context vs operations in ParseResponse<T>
rg -n -C4 'ParseResponse<|JsonSerializer\.Deserialize|ReadAsStringAsync|catch \(Exception ex\) when' web/Classes/Utilities/IamApi.csRepository: ucdavis/VIPER
Length of output: 2159
Fix exception filter in ParseResponse<T> to handle JSON parse failures.
ParseResponse<T> calls JsonSerializer.Deserialize(...), but the filtered catch (Exception ex) when (...) at web/Classes/Utilities/IamApi.cs (lines ~403-406) excludes JsonException/NotSupportedException, so malformed/unsupported IAM payloads can escape instead of setting r.ErrorMessage.
Proposed fix
-catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
+catch (Exception ex) when (ex is JsonException or NotSupportedException or InvalidOperationException or OperationCanceledException)
{
r.ErrorMessage = "Invalid response: " + ex.Message + ".";
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException) | |
| { | |
| r.ErrorMessage = "Invalid response: " + ex.Message + "."; | |
| } | |
| catch (Exception ex) when (ex is JsonException or NotSupportedException or InvalidOperationException or OperationCanceledException) | |
| { | |
| r.ErrorMessage = "Invalid response: " + ex.Message + "."; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/Classes/Utilities/IamApi.cs` around lines 403 - 406, The exception filter
in ParseResponse<T> currently catches DbUpdateException, SqlException,
InvalidOperationException, and OperationCanceledException but misses
JsonException and NotSupportedException so JSON deserialize failures escape;
update the catch in ParseResponse<T> to include JsonException and
NotSupportedException (or replace the filter with a broader Exception catch for
deserialize errors) and ensure the handler sets r.ErrorMessage = "Invalid
response: " + ex.Message + "." so malformed/unsupported IAM payloads are
converted into the error result.
Comment on lines
+90
to
93
| catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException) | ||
| { | ||
| logger.Fatal(ex, "Failed to get secrets from AWS"); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify mismatch between catch filter and AWS setup block
rg -n -C5 'AddSystemsManager|catch \(Exception ex\) when' web/Program.csRepository: ucdavis/VIPER
Length of output: 2064
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show surrounding context for the three catch blocks identified in the review
sed -n '60,110p' web/Program.cs
echo "----"
sed -n '420,490p' web/Program.cs
echo "----"
sed -n '560,620p' web/Program.csRepository: ucdavis/VIPER
Length of output: 7294
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the outer catch that wraps the big startup try { ... }
rg -n "try\s*\{|\bcatch\s*\(" web/Program.cs | head -n 50
echo "---- Outer catch area ----"
# Print a larger window near the end of the big try block
tail -n 120 web/Program.csRepository: ucdavis/VIPER
Length of output: 5819
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,80p' web/Program.cs
echo "----"
rg -n "AmazonServiceException|AmazonClientException" web/Program.cs
echo "----"
rg -n "HttpRequestException|TaskCanceledException|TimeoutException" web/Program.csRepository: ucdavis/VIPER
Length of output: 2854
🏁 Script executed:
rg -n "AmazonServiceException|AmazonClientException|Amazon.*Exception" web/Program.cs || true
echo "----"
rg -n "using Amazon\.Runtime" web/Program.cs || true
echo "----"
rg -n "HttpRequestException|TaskCanceledException|TimeoutException" web/Program.cs || true
echo "----"
rg -n "File.Delete\\(" -n web/Program.csRepository: ucdavis/VIPER
Length of output: 170
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the full using block to see whether HttpRequestException / Amazon.* exception types are in scope
sed -n '1,30p' web/Program.cs
# Look for existing usage/imports of these exception types
rg -n "HttpRequestException|TaskCanceledException|using System\.Net\.Http|using System\.Threading\.Tasks|Amazon\.Runtime\.(AmazonServiceException|AmazonClientException)" web/Program.cs || trueRepository: ucdavis/VIPER
Length of output: 981
Broaden the AWS secrets catch filter in web/Program.cs to avoid startup failure
The catch that logs "Failed to get secrets from AWS" is filtering for DB/SQL exceptions (DbUpdateException/SqlException) and other unrelated types, even though it wraps builder.Configuration.AddSystemsManager(...). Any AWS/network/config exception that doesn’t match the filter will fall through to the top-level startup catch and rethrow, stopping the app.
Update the handler to catch the relevant AWS SDK and connectivity/timeout exceptions for Systems Manager (e.g., Amazon.Runtime.AmazonServiceException, Amazon.Runtime.AmazonClientException, System.Net.Http.HttpRequestException, and task/timeout cancellation types) or otherwise remove the mismatched filter and handle appropriately after logging.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/Program.cs` around lines 90 - 93, The catch block that wraps
builder.Configuration.AddSystemsManager(...) is too narrow and lets non-matching
AWS/network exceptions escape; update the catch on the try surrounding
builder.Configuration.AddSystemsManager to either remove the restrictive
exception filter (catch Exception ex) or explicitly include
Amazon.Runtime.AmazonServiceException, Amazon.Runtime.AmazonClientException,
System.Net.Http.HttpRequestException and
TaskCanceledException/OperationCanceledException in the filter, then call
logger.Fatal(ex, "Failed to get secrets from AWS") as before and handle/rethrow
as appropriate; locate the catch block that currently references
DbUpdateException/SqlException and replace the filter with the recommended
exception types or a plain catch to ensure AWS/config/network errors are logged
and handled.
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
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.
Summary
Closes ~44 of 46 remaining
cs/catch-of-all-exceptionsalerts. Same when-filter pattern as #193, applied across the rest of the codebase.Files changed (28)
Filter is
DbUpdateException∨SqlException∨InvalidOperationException∨OperationCanceledExceptionfor most catches, withIOExceptionadded in ViteProxyHelpers (proxy-and-file-fallback) andFormatException/ArgumentExceptionfor the MailAddress email validator.Bare
catch { }inRoleMembersController(VMACS JSON parse) andBiorenderStudentLookup(MailAddress validation) tightened to the specific exceptions those calls actually throw.Why 2 broad catches are kept
web/Program.cs:527- top-level startup must catch any exception to log fatal before rethrowing. Marked#pragma warning disable CA1031.The 4 ClinicalScheduler/Services post-transaction catches kept broad in #193 remain so on this branch. Test files (test/CTS/AssessmentControllerTest.cs helpers, test/ClinicalScheduler/EmailNotificationTest.cs diagnostic catches) are intentionally left as broad catches - they are test infrastructure, not production code.
Context
Sixth in the
CodeQL N:cleanup series (after #189, #190, #191, #192, #193).Test plan
npm run test:backend- 1946 tests passingnpm run verify:build- clean (0 errors)