From 39a3610e6828ee035d320fd3340e2c26c133fbbd Mon Sep 17 00:00:00 2001 From: Rex Lorenzo Date: Tue, 12 May 2026 23:04:34 -0700 Subject: [PATCH 1/3] fix: close CodeQL correctness alerts Three rule families consolidated into one PR because each fix needs per-site judgment rather than mechanical replacement. cs/virtual-call-in-constructor (5): seal CMSFile and StudentClassYear - neither is inherited from in C#, neither is mocked, and the project has no UseLazyLoadingProxies so EF does not need them non-sealed. Drops `virtual` on properties newly declared inside the now-sealed classes. For RoleTemplateSimplified the same alert is closed by dropping `virtual` from the Roles property, which is a smaller change than sealing and keeps the public constructor surface intact. cs/unsafe-year-construction (4): in PercentRolloverService, bound the HTTP-derived year parameter with ArgumentOutOfRangeException and use .AddYears(1) instead of `new DateTime(year+1, ...)` for the target end date. In AcademicYearHelper.GetAcademicYearStart, build the July date first then .AddYears(-1) instead of subtracting from .Year before constructing. cs/constant-condition (21): remove redundant null-propagation that the analyzer can already prove dead - `?.` chains after `!= null` guards, an unreachable CompetencyId range check after the FindAsync record-existence check, and a duplicated `RegionEndpoint` element lookup in SetAwsCredentials. RAPSController null-flow was restructured to lift the path null check up. The SetAwsCredentials region lookup also gets hardened to truly close the alert: trim the value, look it up with BindingFlags.IgnoreCase, and fall back to USWest1 when the name is non-empty but does not match any known region (previously left profile.Region as null). --- web/Areas/CTS/Models/AuditRow.cs | 6 +++--- .../Effort/Services/PercentRolloverService.cs | 6 +++++- web/Areas/Effort/Services/PercentageService.cs | 3 ++- web/Areas/RAPS/Controllers/RAPSController.cs | 16 ++++++++++------ .../RAPS/Controllers/RoleTemplatesController.cs | 4 ++-- web/Areas/RAPS/Services/RAPSAuditService.cs | 6 +++--- web/Classes/Utilities/AcademicYearHelper.cs | 4 ++-- web/Controllers/HomeController.cs | 3 ++- web/Models/Students/StudentClassYear.cs | 10 +++++----- 9 files changed, 34 insertions(+), 24 deletions(-) diff --git a/web/Areas/CTS/Models/AuditRow.cs b/web/Areas/CTS/Models/AuditRow.cs index 240f5da35..536a6ea37 100644 --- a/web/Areas/CTS/Models/AuditRow.cs +++ b/web/Areas/CTS/Models/AuditRow.cs @@ -22,10 +22,10 @@ public AuditRow(CtsAudit dbAudit) Timestamp = dbAudit.TimeStamp; ModifiedById = dbAudit.ModifiedBy; ModifiedByName = dbAudit.Modifier.LastName + ", " + dbAudit.Modifier.FirstName; - if (dbAudit?.Encounter?.Student != null) + if (dbAudit.Encounter?.Student != null) { - ModifiedPersonId = dbAudit.Encounter?.StudentUserId; - ModifiedPersonName = dbAudit.Encounter?.Student?.LastName + ", " + dbAudit.Encounter?.Student?.FirstName; + ModifiedPersonId = dbAudit.Encounter.StudentUserId; + ModifiedPersonName = dbAudit.Encounter.Student.LastName + ", " + dbAudit.Encounter.Student.FirstName; } } diff --git a/web/Areas/Effort/Services/PercentRolloverService.cs b/web/Areas/Effort/Services/PercentRolloverService.cs index 3fe56d08a..4e4ab1d79 100644 --- a/web/Areas/Effort/Services/PercentRolloverService.cs +++ b/web/Areas/Effort/Services/PercentRolloverService.cs @@ -30,6 +30,10 @@ public PercentRolloverService( public async Task GetRolloverPreviewAsync(int year, CancellationToken ct = default) { + // Bound year for DateTime constructions below (year and year+1 must be valid years). + ArgumentOutOfRangeException.ThrowIfLessThan(year, 1); + ArgumentOutOfRangeException.ThrowIfGreaterThan(year, 9998); + var result = new PercentRolloverPreviewDto(); result.SourceAcademicYear = year; @@ -42,7 +46,7 @@ public async Task GetRolloverPreviewAsync(int year, C var july1Start = new DateTime(year, 7, 1, 0, 0, 0, DateTimeKind.Local); result.OldEndDate = june30Start; result.NewStartDate = july1Start; - result.NewEndDate = new DateTime(year + 1, 6, 30, 0, 0, 0, DateTimeKind.Local); + result.NewEndDate = june30Start.AddYears(1); // Find assignments ending on June 30 of source year (any time on that day) var assignments = await _context.Percentages diff --git a/web/Areas/Effort/Services/PercentageService.cs b/web/Areas/Effort/Services/PercentageService.cs index 82c708650..2050c2ccc 100644 --- a/web/Areas/Effort/Services/PercentageService.cs +++ b/web/Areas/Effort/Services/PercentageService.cs @@ -306,7 +306,8 @@ public async Task ValidatePercentageAsync( .Where(p => !string.Equals(p.PercentAssignType.Class, LeaveTypeClass, StringComparison.OrdinalIgnoreCase)) .Sum(p => (decimal)EffortConstants.ToDisplayPercent(p.PercentageValue)); - if (isNewActive && type != null && !string.Equals(type.Class, LeaveTypeClass, StringComparison.OrdinalIgnoreCase)) + // type is guaranteed non-null here: early-return above when result.IsValid==false (set when type==null). + if (isNewActive && !string.Equals(type!.Class, LeaveTypeClass, StringComparison.OrdinalIgnoreCase)) { var newTotal = activeNonLeaveTotal + Math.Round(request.PercentageValue, EffortConstants.PercentDisplayDecimals); if (newTotal > 100) diff --git a/web/Areas/RAPS/Controllers/RAPSController.cs b/web/Areas/RAPS/Controllers/RAPSController.cs index ae29f6609..71b1cb743 100644 --- a/web/Areas/RAPS/Controllers/RAPSController.cs +++ b/web/Areas/RAPS/Controllers/RAPSController.cs @@ -46,14 +46,18 @@ public override async Task OnActionExecutionAsync(ActionExecutingContext context List? path = HttpContext?.Request?.Path.ToString().Split("/").ToList(); int? rapsIdx = path?.FindIndex(p => p.Equals("raps", StringComparison.OrdinalIgnoreCase)); string instance = "VIPER"; - if (rapsIdx != null && rapsIdx > -1 && path?.Count > rapsIdx + 1) - { - instance = path[(int)rapsIdx + 1]; - } string page = ""; - if (rapsIdx != null && rapsIdx > -1 && path?.Count > rapsIdx + 2) + // rapsIdx is non-null iff path is non-null (it's derived from path?.FindIndex). + if (rapsIdx is int idx && idx > -1) { - page = path[(int)rapsIdx + 2]; + if (path!.Count > idx + 1) + { + instance = path[idx + 1]; + } + if (path.Count > idx + 2) + { + page = path[idx + 2]; + } } ViewData["ViperLeftNav"] = await Nav(roleIdValid ? roleId : null, permIdValid ? permissionId : null, diff --git a/web/Areas/RAPS/Controllers/RoleTemplatesController.cs b/web/Areas/RAPS/Controllers/RoleTemplatesController.cs index 19de64b30..32559dc6b 100644 --- a/web/Areas/RAPS/Controllers/RoleTemplatesController.cs +++ b/web/Areas/RAPS/Controllers/RoleTemplatesController.cs @@ -185,8 +185,8 @@ public async Task> RoleTemplateApply(stri return new RoleTemplateApplyPreview { - DisplayName = user?.DisplayFullName ?? "User not found", - MemberId = user?.MothraId ?? "", + DisplayName = user.DisplayFullName ?? "User not found", + MemberId = user.MothraId ?? "", Roles = rolesToApply }; } diff --git a/web/Areas/RAPS/Services/RAPSAuditService.cs b/web/Areas/RAPS/Services/RAPSAuditService.cs index eab5d7ef8..8515adf37 100644 --- a/web/Areas/RAPS/Services/RAPSAuditService.cs +++ b/web/Areas/RAPS/Services/RAPSAuditService.cs @@ -137,11 +137,11 @@ public async Task> GetMemberRolesAndPermissionHistory(string inst Dictionary> actionsPerformedOnObject = new(); foreach (AuditLog auditLog in auditEntries) { - if (auditLog?.RoleId != null || auditLog?.PermissionId != null) + if (auditLog.RoleId != null || auditLog.PermissionId != null) { - string key = auditLog?.RoleId != null + string key = auditLog.RoleId != null ? "role-" + auditLog.RoleId - : "permission-" + auditLog!.PermissionId; + : "permission-" + auditLog.PermissionId; if (actionsPerformedOnObject.ContainsKey(key)) { List moreRecentActions = actionsPerformedOnObject[key]; diff --git a/web/Classes/Utilities/AcademicYearHelper.cs b/web/Classes/Utilities/AcademicYearHelper.cs index 921e298ac..321beeeff 100644 --- a/web/Classes/Utilities/AcademicYearHelper.cs +++ b/web/Classes/Utilities/AcademicYearHelper.cs @@ -52,8 +52,8 @@ public static List GetTermCodesForAcademicYear(IEnumerable allTermCode /// public static DateTime GetAcademicYearStart(DateTime date) { - var year = date.Month < 7 ? date.Year - 1 : date.Year; - return new DateTime(year, 7, 1, 0, 0, 0, DateTimeKind.Local); + var julyOfYear = new DateTime(date.Year, 7, 1, 0, 0, 0, DateTimeKind.Local); + return date.Month < 7 ? julyOfYear.AddYears(-1) : julyOfYear; } /// diff --git a/web/Controllers/HomeController.cs b/web/Controllers/HomeController.cs index bb91938f5..a31956319 100644 --- a/web/Controllers/HomeController.cs +++ b/web/Controllers/HomeController.cs @@ -320,7 +320,8 @@ private async Task AuthenticateCasLogin(string? ticket, string? r { var claimsIdentity = new ClaimsIdentity(new[] { new Claim(ClaimTypes.Name, validatedUserName), new Claim(ClaimTypes.NameIdentifier, validatedUserName), new Claim(ClaimTypes.AuthenticationMethod, "CAS") }, CookieAuthenticationDefaults.AuthenticationScheme); - XElement? attributesNode = successNode?.Element(_ns + "attributes"); + // successNode is guaranteed non-null here: validatedUserName is derived from successNode?.Element(user)?.Value. + XElement? attributesNode = successNode!.Element(_ns + "attributes"); if (attributesNode != null) { foreach (string attributeName in _casAttributesToCapture) diff --git a/web/Models/Students/StudentClassYear.cs b/web/Models/Students/StudentClassYear.cs index 6c77638f2..71020ab09 100644 --- a/web/Models/Students/StudentClassYear.cs +++ b/web/Models/Students/StudentClassYear.cs @@ -3,7 +3,7 @@ namespace Viper.Models.Students { - public class StudentClassYear + public sealed class StudentClassYear { public int StudentClassYearId { get; set; } @@ -20,10 +20,10 @@ public class StudentClassYear public int? UpdatedBy { get; set; } public string? Comment { get; set; } - public virtual Person? Student { get; set; } - public virtual ClassYearLeftReason? ClassYearLeftReason { get; set; } - public virtual Person? AddedByPerson { get; set; } - public virtual Person? UpdatedByPerson { get; set; } + public Person? Student { get; set; } + public ClassYearLeftReason? ClassYearLeftReason { get; set; } + public Person? AddedByPerson { get; set; } + public Person? UpdatedByPerson { get; set; } [NotMapped] public string? LeftReasonText From 7e6cb31e75a91aad93bded84674c64d536b141c4 Mon Sep 17 00:00:00 2001 From: Rex Lorenzo Date: Wed, 13 May 2026 13:57:27 -0700 Subject: [PATCH 2/3] chore(resharper): silence PR-scoped gate warnings introduced by codeql/3 These were flagged by the ReSharper PR-scoped gate as new warnings on lines my CodeQL 3 fix touched: - CMSFile.cs: drop redundant 'partial' modifier (PartialTypeWithSinglePart) - RAPSController.cs: 'is int idx' on int? -> 'is { } idx' (ConvertTypeCheckPatternToNullCheck) - RoleTemplatesController.cs: drop '??' fallbacks; both properties are declared non-null (NullCoalescingConditionIsAlwaysNotNullAccordingToAPIContract) Also folds in the controller cleanup from CodeRabbit on the same lines: replace foreach + Add with .Select(...).ToList() to match the project's LINQ conventions. --- web/Areas/RAPS/Controllers/RAPSController.cs | 2 +- .../RAPS/Controllers/RoleTemplatesController.cs | 13 +++++-------- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/web/Areas/RAPS/Controllers/RAPSController.cs b/web/Areas/RAPS/Controllers/RAPSController.cs index 71b1cb743..19f717b1f 100644 --- a/web/Areas/RAPS/Controllers/RAPSController.cs +++ b/web/Areas/RAPS/Controllers/RAPSController.cs @@ -48,7 +48,7 @@ public override async Task OnActionExecutionAsync(ActionExecutingContext context string instance = "VIPER"; string page = ""; // rapsIdx is non-null iff path is non-null (it's derived from path?.FindIndex). - if (rapsIdx is int idx && idx > -1) + if (rapsIdx is { } idx && idx > -1) { if (path!.Count > idx + 1) { diff --git a/web/Areas/RAPS/Controllers/RoleTemplatesController.cs b/web/Areas/RAPS/Controllers/RoleTemplatesController.cs index 32559dc6b..3e28f77b1 100644 --- a/web/Areas/RAPS/Controllers/RoleTemplatesController.cs +++ b/web/Areas/RAPS/Controllers/RoleTemplatesController.cs @@ -41,12 +41,9 @@ public async Task>> GetRoleTemp .OrderBy(rt => rt.TemplateName) .ToListAsync(); - List roleTemplates = new(); - foreach (var rt in dbRoleTemplates) - { - roleTemplates.Add(new RoleTemplateSimplified(rt)); - } - return roleTemplates; + return dbRoleTemplates + .Select(rt => new RoleTemplateSimplified(rt)) + .ToList(); } // GET: RoleTemplates/5 @@ -185,8 +182,8 @@ public async Task> RoleTemplateApply(stri return new RoleTemplateApplyPreview { - DisplayName = user.DisplayFullName ?? "User not found", - MemberId = user.MothraId ?? "", + DisplayName = user.DisplayFullName, + MemberId = user.MothraId, Roles = rolesToApply }; } From df971c61d9e62b75601d66838d15f0adc240da80 Mon Sep 17 00:00:00 2001 From: Rex Lorenzo Date: Wed, 13 May 2026 13:57:51 -0700 Subject: [PATCH 3/3] test(raps): cover RoleTemplateSimplified constructor mapping Two unit tests for the constructor that drives the GetRoleTemplates projection: - Constructor_MapsScalarsAndFlattensRoles: verifies the scalar properties and the nested RoleTemplateRoles -> Roles flattening via Role.RoleId / Role.FriendlyName. - Constructor_EmptyRoles_ReturnsEmptyCollection: ensures the Roles initializer does not depend on a non-empty source collection. Pins the constructor's mapping behavior so the cs/virtual-call-in- constructor fix (dropping `virtual` from Roles) does not silently regress the projection. --- test/RAPS/RoleTemplateSimplifiedTests.cs | 59 ++++++++++++++++++++++++ 1 file changed, 59 insertions(+) create mode 100644 test/RAPS/RoleTemplateSimplifiedTests.cs diff --git a/test/RAPS/RoleTemplateSimplifiedTests.cs b/test/RAPS/RoleTemplateSimplifiedTests.cs new file mode 100644 index 000000000..aad8a1061 --- /dev/null +++ b/test/RAPS/RoleTemplateSimplifiedTests.cs @@ -0,0 +1,59 @@ +using Viper.Areas.RAPS.Models; +using Viper.Models.RAPS; + +namespace Viper.test.RAPS +{ + public class RoleTemplateSimplifiedTests + { + [Fact] + public void Constructor_MapsScalarsAndFlattensRoles() + { + // arrange + var rt = new RoleTemplate + { + RoleTemplateId = 42, + TemplateName = "Test Template", + Description = "Desc", + RoleTemplateRoles = new List + { + new() { Role = new TblRole { RoleId = 1, Role = "Alpha", DisplayName = "Alpha" } }, + new() { Role = new TblRole { RoleId = 2, Role = "Beta", DisplayName = "Beta" } } + } + }; + + // act + var dto = new RoleTemplateSimplified(rt); + + // assert + Assert.Equal(42, dto.RoleTemplateId); + Assert.Equal("Test Template", dto.TemplateName); + Assert.Equal("Desc", dto.Description); + var roles = dto.Roles.ToList(); + Assert.Equal(2, roles.Count); + Assert.Equal(1, roles[0].RoleId); + Assert.Equal("Alpha", roles[0].FriendlyName); + Assert.Equal(2, roles[1].RoleId); + Assert.Equal("Beta", roles[1].FriendlyName); + } + + [Fact] + public void Constructor_EmptyRoles_ReturnsEmptyCollection() + { + // arrange + var rt = new RoleTemplate + { + RoleTemplateId = 7, + TemplateName = "No Roles", + Description = "", + RoleTemplateRoles = new List() + }; + + // act + var dto = new RoleTemplateSimplified(rt); + + // assert + Assert.Equal(7, dto.RoleTemplateId); + Assert.Empty(dto.Roles); + } + } +}