fix: address open code scanning alerts (log forging, missed Where)#254
fix: address open code scanning alerts (log forging, missed Where)#254rlorenzo wants to merge 2 commits into
Conversation
CodeQL flags string.Join(",", weekIds) as log forging (alert 714)
each time this file changes, even though int[] cannot carry control
characters. Wrap all four occurrences with LogSanitizer.SanitizeString
so the taint path is closed for good instead of re-dismissing.
Resolves CodeQL cs/linq/missed-where (alert 703) by moving the CRN guard from an if inside the loop into the Where clause.
Bundle ReportBundle size has no change ✅ |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #254 +/- ##
=======================================
Coverage 44.67% 44.67%
=======================================
Files 897 897
Lines 51894 51893 -1
Branches 4869 4868 -1
=======================================
+ Hits 23184 23185 +1
Misses 28130 28130
+ Partials 580 578 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
There was a problem hiding this comment.
Pull request overview
This PR addresses two CodeQL alerts by (1) sanitizing weekIds values before logging to permanently close a log-forging taint path, and (2) rewriting a foreach guard-pattern into a LINQ Where() filter to satisfy the “missed where” analyzer without changing behavior.
Changes:
- Sanitize
string.Join(",", weekIds)viaLogSanitizer.SanitizeString(...)at all affected log sites inScheduleEditService. - Refactor
ApplyDirectorCustodialDeptFallbackto push CRN presence + dictionary membership checks into the.Where()clause and keep the loop body as a single assignment.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| web/Areas/Effort/Services/Harvest/NonCrestHarvestPhase.cs | Refactors a foreach guard if into a stronger LINQ filter and simplifies the loop body to an assignment. |
| web/Areas/ClinicalScheduler/Services/ScheduleEditService.cs | Sanitizes logged week-id lists with LogSanitizer.SanitizeString to resolve CodeQL log-forging alerts. |
Summary
Fixes both open CodeQL code scanning alerts:
string.Join(",", weekIds)flowing into a log entry. Whileint[]values cannot actually carry control characters, this line was previously dismissed as a false positive (alert build(deps): bump the npm_and_yarn group across 2 directories with 2 updates #117) and CodeQL re-flagged it as a new alert when the surrounding catch block changed in fix(clinical): detect duplicate schedules by SQL error number #229. Wrapping withLogSanitizer.SanitizeString()closes the taint path permanently. Applied to all four occurrences of the pattern in the file (lines 129, 248, 557, 584) so the parallel sites with dismissed alerts (Upgrade to LTS versions of .NET 10 and NodeJS 24 #120, VPR-104 fix(a11y): ClinicalScheduler, CMS, Computing, CAHFS accessibility (PR 6 of 6) #146) do not re-flag either.ApplyDirectorCustodialDeptFallbackforeach whose body was a single guardingif. Moved the CRN presence and dictionary membership checks into the existing.Where()clause; the loop body now only performs the assignment. Behavior is unchanged.Testing
npm run linton both files: passes (remaining CA1502 complexity warnings are pre-existing on an untouched method)npm run test:backend: all 2134 tests pass, includingApplyDirectorCustodialDeptFallback_InheritsDirectorDept_OnlyForNonAcademicCourses