Skip to content

CodeQL 6: refactor: catch specific exceptions across CTS/RAPS/shared #194

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
rlorenzo wants to merge 2 commits into
base: main
Choose a base branch
from
Open

CodeQL 6: refactor: catch specific exceptions across CTS/RAPS/shared #194

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2b57b4c
refactor: catch specific exceptions across CTS/RAPS/shared
rlorenzo May 13, 2026
5e071de
chore(resharper): replace placeholder when-true filters and drop redu…
rlorenzo May 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion web/Areas/CTS/Controllers/BundleCompetencyController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using Viper.Classes.SQLContext;
using Viper.Models.CTS;
using Web.Authorization;
using Microsoft.Data.SqlClient;

namespace Viper.Areas.CTS.Controllers
{
Expand Down Expand Up @@ -158,7 +159,7 @@ public async Task<ActionResult<BundleCompetencyDto>> DeleteBundleCompetency(int
await context.SaveChangesAsync();
AdjustBundleCompetencyOrders(bundleComp);
}
catch (Exception)
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
return BadRequest("Cannot delete this bundle competency.");
}
Expand Down
3 changes: 2 additions & 1 deletion web/Areas/CTS/Controllers/BundleCompetencyGroupController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using Viper.Classes.SQLContext;
using Viper.Models.CTS;
using Web.Authorization;
using Microsoft.Data.SqlClient;

namespace Viper.Areas.CTS.Controllers
{
Expand Down Expand Up @@ -116,7 +117,7 @@ public async Task<ActionResult<BundleCompetencyGroupDto>> DeleteGroup(int bundle
await context.SaveChangesAsync();
AdjustGroupOrders(group);
}
catch (Exception)
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
return BadRequest("Cannot delete group. Competencies must be removed from the group, and the group cannot have been used to document a student competency.");
}
Expand Down
3 changes: 2 additions & 1 deletion web/Areas/CTS/Controllers/BundleController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using Viper.Classes.SQLContext;
using Viper.Models.CTS;
using Web.Authorization;
using Microsoft.Data.SqlClient;

namespace Viper.Areas.CTS.Controllers
{
Expand Down Expand Up @@ -139,7 +140,7 @@ public async Task<ActionResult<BundleDto>> DeleteBundle(int bundleId)
await context.SaveChangesAsync();
await trans.CommitAsync();
}
catch (Exception)
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
return BadRequest("Could not delete bundle. If this bundle has been used, it cannot be deleted.");
}
Expand Down
3 changes: 2 additions & 1 deletion web/Areas/CTS/Controllers/CompetencyController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using Microsoft.AspNetCore.Mvc;
using Microsoft.Data.SqlClient;
using Microsoft.EntityFrameworkCore;
using Viper.Areas.CTS.Models;
using Viper.Classes;
Expand Down Expand Up @@ -176,7 +177,7 @@ public async Task<ActionResult<CompetencyDto>> DeleteCompetency(int competencyId
{
await context.SaveChangesAsync();
}
catch (Exception)
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.");
}
Comment on lines +180 to 183
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

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".

Expand Down
5 changes: 3 additions & 2 deletions web/Areas/CTS/Controllers/CourseController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using Microsoft.AspNetCore.Mvc;
using Microsoft.Data.SqlClient;
using Microsoft.EntityFrameworkCore;
using Viper.Areas.CTS.Models;
using Viper.Classes;
Expand Down Expand Up @@ -105,7 +106,7 @@ public async Task<ActionResult<List<RoleDto>>> SetCourseRoles(int courseId, List
await context.SaveChangesAsync();
await trans.CommitAsync();
}
catch (Exception)
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
return BadRequest("Could not set roles.");
}
Expand Down Expand Up @@ -287,7 +288,7 @@ public async Task<ActionResult<List<SessionCompetencyDto>>> UpdateSessionCompete
await context.SaveChangesAsync();
await trans.CommitAsync();
}
catch (Exception)
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
return BadRequest("Could not update levels.");
}
Expand Down
3 changes: 2 additions & 1 deletion web/Areas/CTS/Controllers/DomainController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using Viper.Classes.SQLContext;
using Viper.Models.CTS;
using Web.Authorization;
using Microsoft.Data.SqlClient;

namespace Viper.Areas.CTS.Controllers
{
Expand Down Expand Up @@ -91,7 +92,7 @@ public async Task<ActionResult<DomainDto>> DeleteDomain(int domainId)
{
await context.SaveChangesAsync();
}
catch (Exception)
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.");
}
Expand Down
3 changes: 2 additions & 1 deletion web/Areas/CTS/Controllers/EpaController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using Microsoft.AspNetCore.Mvc;
using Microsoft.Data.SqlClient;
using Microsoft.EntityFrameworkCore;
using Viper.Classes;
using Viper.Classes.SQLContext;
Expand Down Expand Up @@ -105,7 +106,7 @@ public async Task<ActionResult<Epa>> GetEpa(int epaId)
context.Entry(epa).State = EntityState.Deleted;
await context.SaveChangesAsync();
}
catch (Exception ex)
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
return BadRequest(ex.Message);
}
Comment on lines +109 to 112
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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.

Expand Down
3 changes: 2 additions & 1 deletion web/Areas/CTS/Controllers/LevelsController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using Viper.Classes.SQLContext;
using Viper.Models.CTS;
using Web.Authorization;
using Microsoft.Data.SqlClient;

namespace Viper.Areas.CTS.Controllers
{
Expand Down Expand Up @@ -151,7 +152,7 @@ public async Task<ActionResult> DeleteLevel(int levelId)
AdjustLevelOrders(existing);
await trans.CommitAsync();
}
catch (Exception)
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
return BadRequest("Could not delete level. If this level has been used in an assessment, it cannot be deleted.");
}
Expand Down
3 changes: 2 additions & 1 deletion web/Areas/CTS/Controllers/RoleController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using Viper.Classes;
using Viper.Classes.SQLContext;
using Web.Authorization;
using Microsoft.Data.SqlClient;

namespace Viper.Areas.CTS.Controllers
{
Expand Down Expand Up @@ -84,7 +85,7 @@ public async Task<ActionResult<RoleDto>> DeleteRole(int roleId)
context.Entry(role).State = EntityState.Deleted;
await context.SaveChangesAsync();
}
catch (Exception)
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
return BadRequest("Could not delete role. If this role has been added to a bundle, it cannot be deleted.");
}
Expand Down
9 changes: 5 additions & 4 deletions web/Areas/ClinicalScheduler/Controllers/CliniciansController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using Microsoft.AspNetCore.Mvc;
using Microsoft.Data.SqlClient;
using Microsoft.EntityFrameworkCore;
using Viper.Areas.ClinicalScheduler.Services;
using Viper.Areas.Curriculum.Services;
Expand Down Expand Up @@ -122,7 +123,7 @@
[ProducesResponseType(StatusCodes.Status200OK)]
[ProducesResponseType(StatusCodes.Status403Forbidden)]
[ProducesResponseType(StatusCodes.Status500InternalServerError)]
public async Task<IActionResult> GetClinicianSchedule(string mothraId, [FromQuery] int? year = null)

Check warning on line 126 in web/Areas/ClinicalScheduler/Controllers/CliniciansController.cs

View workflow job for this annotation

GitHub Actions / Backend Tests 

'GetClinicianSchedule' has a cyclomatic complexity of '29'. Rewrite or refactor the code to decrease its complexity below '26'. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1502)
{
var correlationId = HttpContext.TraceIdentifier ?? Guid.NewGuid().ToString();

Expand Down Expand Up @@ -334,7 +335,7 @@
schedules.Count, LogSanitizer.SanitizeId(mothraId), LogSanitizer.SanitizeYear(targetYear));
return Ok(result);
}
catch (Exception)
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
// Store context for ApiExceptionFilter to use in logging
SetExceptionContext("MothraId", mothraId);
Expand Down Expand Up @@ -381,7 +382,7 @@
_logger.LogDebug("Found {RotationCount} unique rotations for clinician {MothraId}", rotations.Count, LogSanitizer.SanitizeId(mothraId));
return Ok(rotations);
}
catch (Exception)
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
// Store context for ApiExceptionFilter to use in logging
SetExceptionContext("MothraId", mothraId);
Expand Down Expand Up @@ -575,7 +576,7 @@
// All other cases (Admin, Manage, EditClnSchedules, rotation view, or service-specific permissions) see all clinicians
return clinicians;
}
catch (Exception ex)
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
Expand Down Expand Up @@ -653,7 +654,7 @@
LogSanitizer.SanitizeId(currentUser.MothraId), LogSanitizer.SanitizeId(targetMothraId));
return false;
}
catch (Exception ex)
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
_logger.LogError(ex, "Error checking clinician schedule view permission for target {TargetMothraId}", LogSanitizer.SanitizeId(targetMothraId));
return false; // Deny access on error
Expand Down
12 changes: 7 additions & 5 deletions web/Areas/ClinicalScheduler/Controllers/InstructorScheduleController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
using Microsoft.AspNetCore.Mvc;
using Microsoft.Data.SqlClient;
using Microsoft.EntityFrameworkCore;
using Viper.Areas.ClinicalScheduler.Constants;
using Viper.Areas.ClinicalScheduler.Extensions;
using Viper.Areas.ClinicalScheduler.Models;
Expand Down Expand Up @@ -106,7 +108,7 @@ public async Task<IActionResult> AddInstructor(
{
return HandleInvalidOperation(ex, correlationId);
}
catch (Exception ex)
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
Comment on lines +111 to 112
return HandleSystemError(ex, request.RotationId!.Value, correlationId);
}
Expand Down Expand Up @@ -335,7 +337,7 @@ public async Task<IActionResult> RemoveInstructor(
userMessage,
correlationId));
}
catch (Exception ex)
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
_logger.LogError(ex, "Error removing instructor schedule {ScheduleId} (CorrelationId: {CorrelationId})",
instructorScheduleId, correlationId);
Expand Down Expand Up @@ -414,7 +416,7 @@ public async Task<IActionResult> SetPrimaryEvaluator(
instructorScheduleId, correlationId);
return Forbid();
}
catch (Exception ex)
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
_logger.LogError(ex, "Error setting primary evaluator for instructor schedule {ScheduleId} (CorrelationId: {CorrelationId})",
instructorScheduleId, correlationId);
Expand Down Expand Up @@ -507,7 +509,7 @@ public async Task<IActionResult> CheckScheduleConflicts(

return Ok(response);
}
catch (Exception ex)
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
_logger.LogError(ex, "Error checking schedule conflicts for {MothraId}", LogSanitizer.SanitizeId(mothraId));
return StatusCode(500, "An error occurred while checking for schedule conflicts");
Expand All @@ -532,7 +534,7 @@ public async Task<IActionResult> GetAuditHistory(
var auditHistory = await _auditService.GetInstructorScheduleAuditHistoryAsync(instructorScheduleId, cancellationToken);
return Ok(auditHistory);
}
catch (Exception ex)
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
_logger.LogError(ex, "Error retrieving audit history for instructor schedule {ScheduleId}", instructorScheduleId);
return StatusCode(500, "An error occurred while retrieving audit history");
Expand Down
7 changes: 4 additions & 3 deletions web/Areas/ClinicalScheduler/Controllers/PermissionsController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using Microsoft.AspNetCore.Mvc;
using Microsoft.Data.SqlClient;
using Microsoft.EntityFrameworkCore;
using Viper.Areas.ClinicalScheduler.Services;
using Viper.Classes.SQLContext;
Expand Down Expand Up @@ -153,7 +154,7 @@ public async Task<ActionResult<object>> CanEditService(int serviceId)

return Ok(new { canEdit });
}
catch (Exception)
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
// Store context for ApiExceptionFilter to use in logging
SetExceptionContext("ServiceId", serviceId);
Expand Down Expand Up @@ -215,7 +216,7 @@ public async Task<ActionResult<object>> CanEditRotation(int rotationId)

return Ok(new { canEdit });
}
catch (Exception)
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
// Store context for ApiExceptionFilter to use in logging
SetExceptionContext("RotationId", rotationId);
Expand Down Expand Up @@ -277,7 +278,7 @@ public async Task<ActionResult<object>> CanEditOwnSchedule(int instructorSchedul

return Ok(new { canEditOwn });
}
catch (Exception)
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
// Store context for ApiExceptionFilter to use in logging
SetExceptionContext("InstructorScheduleId", instructorScheduleId);
Expand Down
9 changes: 5 additions & 4 deletions web/Areas/ClinicalScheduler/Controllers/RotationsController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using Viper.Models.ClinicalScheduler;
using Web.Authorization;
using Person = Viper.Models.ClinicalScheduler.Person;
using Microsoft.Data.SqlClient;

namespace Viper.Areas.ClinicalScheduler.Controllers
{
Expand Down Expand Up @@ -77,7 +78,7 @@ public async Task<ActionResult<IEnumerable<RotationDto>>> GetRotations(int? serv
rotations.Count, filteredRotations.Count);
return Ok(filteredRotations);
}
catch (Exception)
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>
Expand Down Expand Up @@ -138,7 +139,7 @@ public async Task<ActionResult<object>> GetRotation(int id)
_logger.LogInformation("Retrieved rotation via RotationService: {RotationName}", rotation.Name);
return Ok(response);
}
catch (Exception)
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
// Store context for ApiExceptionFilter to use in logging
SetExceptionContext("RotationId", id);
Expand Down Expand Up @@ -240,7 +241,7 @@ public async Task<ActionResult<object>> GetRotationSchedule(int id, [FromQuery]

return Ok(BuildRotationScheduleResponse(rotation, targetYear, groupedSchedules, recentCliniciansList));
}
catch (Exception)
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
// Store context for ApiExceptionFilter to use in logging
SetExceptionContext("RotationId", id);
Expand Down Expand Up @@ -305,7 +306,7 @@ join w in _context.Weeks on i.WeekId equals w.WeekId
_logger.LogInformation("Retrieved {Count} rotations with scheduled weeks for year {Year} (filtered to {FilteredCount})", rotationsWithSchedules.Count, LogSanitizer.SanitizeYear(targetYear), filteredRotations.Count);
return Ok(filteredRotations);
}
catch (Exception)
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
{
// Store context for ApiExceptionFilter to use in logging
SetExceptionContext("Year", year?.ToString() ?? "null");
Expand Down
2 changes: 1 addition & 1 deletion web/Areas/Computing/Services/BiorenderStudentLookup.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ static private bool IsValidEmail(string email)
var addr = new MailAddress(email);
return addr.Address == trimmed;
}
catch
catch (Exception ex) when (ex is FormatException or ArgumentException)
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

{
return false;
}
Expand Down
3 changes: 2 additions & 1 deletion web/Areas/RAPS/Controllers/AdGroupsController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
using Viper.Classes.Utilities;
using Viper.Models.RAPS;
using Web.Authorization;
using Microsoft.Data.SqlClient;

namespace Viper.Areas.RAPS.Controllers
{
Expand Down Expand Up @@ -113,7 +114,7 @@ public async Task<ActionResult<OuGroup>> CreateGroup(GroupAddEdit group)
OuGroup newOuGroup = await _ouGroupService.CreateRapsGroup(group.Name, group.Description);
return CreatedAtAction("CreateGroup", new { id = newOuGroup.OugroupId }, newOuGroup);
}
catch (Exception ex)
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);
Comment on lines +117 to 120
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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.

Expand Down
2 changes: 1 addition & 1 deletion web/Areas/RAPS/Controllers/RoleMembersController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,7 @@ private static void UpdateTblRoleMemberWithDto(TblRoleMember tblRoleMember, Role
return vmacsResponse;
}
}
catch
catch (JsonException)
{
return new VmacsResponse
{
Expand Down
3 changes: 2 additions & 1 deletion web/Areas/RAPS/Controllers/RolesController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using Viper.Classes;
using Viper.Classes.SQLContext;
using Viper.Models.RAPS;
using Microsoft.Data.SqlClient;

namespace Viper.Areas.RAPS.Controllers
{
Expand Down Expand Up @@ -222,7 +223,7 @@ public async Task<ActionResult<TblRole>> PostTblRole(string instance, RoleCreate
{
return Problem("The record was not updated because it was locked. " + ex.InnerException?.Message);
}
catch (Exception ex)
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);
Comment on lines +226 to 228
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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.

}
Expand Down
3 changes: 2 additions & 1 deletion web/Areas/RAPS/Services/OuGroupService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using Viper.Classes.Utilities;
using Viper.Models.RAPS;
using static Viper.Areas.RAPS.Services.RAPSAuditService;
using Microsoft.Data.SqlClient;

namespace Viper.Areas.RAPS.Services
{
Expand Down Expand Up @@ -45,7 +46,7 @@ public async Task<List<Group>> GetAllGroups(string? search)
{
ActiveDirectoryService.GetGroups();
}
catch (Exception ex)
catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException)
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Replace filtered catch (Exception) with explicit typed catches.

Line 49 still uses a broad Exception catch in service logic. Use explicit catches (DbUpdateException, SqlException, InvalidOperationException, OperationCanceledException) instead of a filtered 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/RAPS/Services/OuGroupService.cs` at 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.

{
Logger logger = LogManager.GetCurrentClassLogger();
logger.Error(ex);
Expand Down
Loading
Loading