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. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
📝 WalkthroughWalkthroughLog statements in ScheduleEditService now sanitize joined week ID strings via LogSanitizer before logging. NonCrestHarvestPhase's director custodial department fallback loop incorporates the CRN key-existence check into its LINQ filter, replacing the TryGetValue guard with direct dictionary indexing. ChangesLog Sanitization and Filter Refactor
Estimated code review effort: 1 (Trivial) | ~5 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/Effort/Services/Harvest/NonCrestHarvestPhase.cs`:
- Around line 431-438: The course update loop in NonCrestHarvestPhase is doing a
double lookup on directorDeptByCrn by filtering with ContainsKey and then
indexing again in the body. Keep the existing Where predicate for the CodeQL
missed-where requirement, but switch the assignment inside the foreach over
courses to use TryGetValue on course.Crn so you fetch the director department
once and only assign when a value is returned.
🪄 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: 922c0e9a-d70b-4ec2-b7fc-59f34399322f
📒 Files selected for processing (2)
web/Areas/ClinicalScheduler/Services/ScheduleEditService.csweb/Areas/Effort/Services/Harvest/NonCrestHarvestPhase.cs
| foreach (var course in courses | ||
| .Where(c => c.Source == EffortConstants.SourceNonCrest && !academicDepts.Contains(c.CustDept))) | ||
| .Where(c => c.Source == EffortConstants.SourceNonCrest | ||
| && !academicDepts.Contains(c.CustDept) | ||
| && !string.IsNullOrWhiteSpace(c.Crn) | ||
| && directorDeptByCrn.ContainsKey(c.Crn))) | ||
| { | ||
| if (!string.IsNullOrWhiteSpace(course.Crn) && | ||
| directorDeptByCrn.TryGetValue(course.Crn, out var iorDept)) | ||
| { | ||
| course.CustDept = iorDept; | ||
| } | ||
| course.CustDept = directorDeptByCrn[course.Crn]; | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Double dictionary lookup — use TryGetValue instead of ContainsKey + indexer.
The filter checks directorDeptByCrn.ContainsKey(c.Crn) and the loop body then does a separate indexer lookup directorDeptByCrn[course.Crn]. That's two lookups per item where one would do. Since course.Crn is already known non-null/non-whitespace by the time you reach the loop body, you can fold the check and fetch into a single TryGetValue inside the loop.
♻️ Proposed fix
- foreach (var course in courses
- .Where(c => c.Source == EffortConstants.SourceNonCrest
- && !academicDepts.Contains(c.CustDept)
- && !string.IsNullOrWhiteSpace(c.Crn)
- && directorDeptByCrn.ContainsKey(c.Crn)))
- {
- course.CustDept = directorDeptByCrn[course.Crn];
- }
+ foreach (var course in courses
+ .Where(c => c.Source == EffortConstants.SourceNonCrest
+ && !academicDepts.Contains(c.CustDept)
+ && !string.IsNullOrWhiteSpace(c.Crn)
+ && directorDeptByCrn.ContainsKey(c.Crn)))
+ {
+ if (directorDeptByCrn.TryGetValue(course.Crn, out var dept))
+ {
+ course.CustDept = dept;
+ }
+ }Note: the Where still needs the ContainsKey (or equivalent) predicate to satisfy the CodeQL "missed-where" alert this PR is fixing, so the TryGetValue in the loop body is a defensive single-lookup rather than a strict guard removal.
📝 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.
| foreach (var course in courses | |
| .Where(c => c.Source == EffortConstants.SourceNonCrest && !academicDepts.Contains(c.CustDept))) | |
| .Where(c => c.Source == EffortConstants.SourceNonCrest | |
| && !academicDepts.Contains(c.CustDept) | |
| && !string.IsNullOrWhiteSpace(c.Crn) | |
| && directorDeptByCrn.ContainsKey(c.Crn))) | |
| { | |
| if (!string.IsNullOrWhiteSpace(course.Crn) && | |
| directorDeptByCrn.TryGetValue(course.Crn, out var iorDept)) | |
| { | |
| course.CustDept = iorDept; | |
| } | |
| course.CustDept = directorDeptByCrn[course.Crn]; | |
| } | |
| foreach (var course in courses | |
| .Where(c => c.Source == EffortConstants.SourceNonCrest | |
| && !academicDepts.Contains(c.CustDept) | |
| && !string.IsNullOrWhiteSpace(c.Crn) | |
| && directorDeptByCrn.ContainsKey(c.Crn))) | |
| { | |
| if (directorDeptByCrn.TryGetValue(course.Crn, out var dept)) | |
| { | |
| course.CustDept = dept; | |
| } | |
| } |
🤖 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/Effort/Services/Harvest/NonCrestHarvestPhase.cs` around lines 431 -
438, The course update loop in NonCrestHarvestPhase is doing a double lookup on
directorDeptByCrn by filtering with ContainsKey and then indexing again in the
body. Keep the existing Where predicate for the CodeQL missed-where requirement,
but switch the assignment inside the foreach over courses to use TryGetValue on
course.Crn so you fetch the director department once and only assign when a
value is returned.
Source: Coding guidelines
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