CodeQL 12: chore: style sweep for remaining CodeQL alerts#198
Open
rlorenzo wants to merge 3 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughPull request modernizes code across test data, controllers, and helpers by adopting contemporary C# patterns: DateTime arithmetic precision adjustments, ternary expressions replacing if/else blocks, implicit ToString() calls in string formatting, parameter renames in HttpHelper, and null-coalescing consolidation in UserHelper. ChangesCode Style Modernization Refactoring
🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 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 #198 +/- ##
==========================================
- Coverage 43.21% 43.20% -0.01%
==========================================
Files 894 894
Lines 51851 51847 -4
Branches 4844 4843 -1
==========================================
- Hits 22406 22402 -4
Misses 28899 28899
Partials 546 546
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
2 tasks
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![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: 2
🤖 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/RAPS/Controllers/RAPSController.cs`:
- Line 315: Add a deny-access helper on RAPSController to centralize logging and
403 rendering: implement a private method (e.g., DenyAccess(string reason) or
DenyAccessResult(string reason)) in RAPSController that logs the reason via the
controller logger, sets Response.StatusCode = 403, and returns View("403") (or
an appropriate IActionResult). Replace duplicated denial code paths in methods
like any access checks inside RAPSController to call this helper instead of
repeating log + status + view logic, keeping the same log message format and
ensuring the helper returns IActionResult to be used with return statements.
In `@web/Controllers/HomeController.cs`:
- Line 315: The code currently logs raw CAS XML via
HttpHelper.Logger.Log(NLog.LogLevel.Warn, "No username. CAS response: " + doc),
which may leak user identifiers; sanitize/redact sensitive fields from the CAS
response before logging by using the LogSanitizer utilities (e.g.,
LogSanitizer.SanitizeString or SanitizeId) on the variable doc or on the
extracted attributes, then pass the sanitized string to HttpHelper.Logger.Log
(keep the "No username. CAS response:" prefix but replace doc with the sanitized
output).
🪄 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: f890a93f-65d0-470c-876d-651c0bddcba5
📒 Files selected for processing (12)
test/ClinicalScheduler/EmailNotificationTest.cstest/ClinicalScheduler/TestDataBuilder.csweb/Areas/CMS/Controllers/CMSController.csweb/Areas/Curriculum/Services/TermCodeService.csweb/Areas/Directory/Models/LdapUserContact.csweb/Areas/Effort/Services/VerificationService.csweb/Areas/RAPS/Controllers/RAPSController.csweb/Areas/RAPS/Services/UinformService.csweb/Areas/Students/Services/GradYearClassLevel.csweb/Classes/HttpHelper.csweb/Classes/UserHelper.csweb/Controllers/HomeController.cs
rlorenzo
force-pushed
the
codeql/12-style-sweep
branch
from
a0528cd to
a67ec40
Compare
There was a problem hiding this comment.
Pull request overview
This PR continues the CodeQL cleanup series by addressing remaining straightforward C# style alerts (e.g., unnecessary ToString() usage, missed ternaries/null-coalescing, and loss-of-precision warnings) across web and test projects.
Changes:
- Sanitizes a CAS XML response before logging, reducing log-injection risk when troubleshooting missing usernames.
- Simplifies conditional assignments/formatting (e.g., ternary for due-date selection, removing redundant
ToString()instring.Format). - Adjusts ClinicalScheduler test date math to use
doublearithmetic forDateTime.AddDays(double).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| web/Controllers/HomeController.cs | Sanitizes CAS response string before logging when username is missing. |
| web/Classes/HttpHelper.cs | Fixes a documentation typo in the Configure XML doc comment. |
| web/Areas/Effort/Services/VerificationService.cs | Converts due-date selection to a ternary assignment. |
| web/Areas/Curriculum/Services/TermCodeService.cs | Removes redundant ToString() in string.Format call. |
| test/ClinicalScheduler/TestDataBuilder.cs | Uses double math for AddDays offset calculation. |
| test/ClinicalScheduler/EmailNotificationTest.cs | Uses double math for AddDays offset calculation in test seed data. |
rlorenzo
force-pushed
the
codeql/12-style-sweep
branch
2 times, most recently
from
0cbb8ac to
6cf8c1d
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
- UserHelper.cs: capture user into 'fallbackUser' local before
GetOrCreate lambda so ReSharper AccessToModifiedClosure is silenced
- HttpHelper.cs: drop stale '<param name="memoryCache">' XML tag now
that the other params don't have matching tags (was creating 5
InvalidXmlDocComment warnings since the rename)
- test/ClinicalScheduler/{EmailNotificationTest,TestDataBuilder}.cs:
replace '(double)(-7 * x)' with '-7.0 * x' — same intent (force the
multiplication into double per cs/loss-of-precision) but no
RedundantCast warning from ReSharper
The fallback log path for a CAS validation response with no username embedded the raw XML in the message. CAS responses come from an external source and can contain user identifiers and claim attributes, so per CLAUDE.md the value must pass through LogSanitizer before hitting the log. Wraps the doc.ToString() in LogSanitizer.SanitizeString.
rlorenzo
force-pushed
the
codeql/12-style-sweep
branch
from
6cf8c1d to
92f13f0
Compare
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
Final slice of the CodeQL style sweep. Most alerts originally scoped here have since landed on
mainthrough other PRs in the series; this PR carries the remaining fixes plus a CAS log-injection hardening that surfaced while touching the same line.Closes
cs/useless-tostring-call (2):
TermCodeService.cs:39- drop redundantyear.ToString()insidestring.FormatHomeController.cs:315- redundantdoc.ToString()in string concat; the log line now routes throughLogSanitizer(see below)cs/missed-ternary-operator (1):
VerificationService.cs:773-dueDateif/else assignment collapsed to a ternarycs/loss-of-precision (3):
test/ClinicalScheduler/EmailNotificationTest.cs:122-123andTestDataBuilder.cs:179--7 * (...)→-7.0 * (...)so the week-offset arithmetic flowing intoDateTime.AddDays(double)is computed indoubleAlso in this PR
HomeControllerCAS auth-failure log now sanitizes the raw CAS XML viaLogSanitizer.SanitizeString()before loggingHttpHelper.Configuredoc summary no longer calls the static configuration method a "constructor" (also fixes the "memeory" typo)HomeControllerstale "uncomment this line" comment replaced with one describing the now-active, sanitized diagnostic logVerificationServicedue-date comment now references the configurableVerificationReplyDaysinstead of hard-coding "7 days"Not addressed
cs/complex-conditionand similar subjective-rewrite alerts remain open; converting them would hurt readability. Other items from the original sweep scope (multi-statement ternary candidates, complex-block, the test-file implicit conversions) were picked up by later PRs in the series and are no longer part of this branch.Context
Twelfth in the
CodeQL N:cleanup series.Test plan