Fix CodeQL warning: Log entries created from user input#3726
Fix CodeQL warning: Log entries created from user input#3726RaymondLuong3 merged 1 commit intomasterfrom
Conversation
| projectSFId ??= _projectDoc?.Id ?? "unknown"; | ||
| userId ??= _userSecret?.Id ?? "unknown"; | ||
| _logger.LogInformation($"SyncLog ({projectSFId} {userId}): {message}"); | ||
| _logger.LogInformation($"SyncLog ({projectSFId.Sanitize()} {userId.Sanitize()}): {message}"); |
Check failure
Code scanning / CodeQL
Log entries created from user input High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 days ago
General approach: Ensure that any data that can originate from user input is normalized before being written to logs. For plain-text logs, this typically means removing or replacing control characters that can break the log format, especially \r and \n. We should also avoid changing log message structure so existing analysis still works. Since the vulnerable sink is the Log method in ParatextSyncRunner, we can centralize the fix by sanitizing both the interpolated IDs and the message written to _syncMetrics.Log.
Best concrete fix for this code:
- Keep the existing
Sanitize()calls onprojectSFIdanduserIdin the_logger.LogInformation(...)call; these already reduce risk. - Additionally sanitize the
messageparameter at the start ofLogto remove newline sequences and carriage returns. Use simplestring.Replacecalls, as recommended, to stripEnvironment.NewLine,\r, and\n. - Use this sanitized
messageboth for the structured logger call and when adding to_syncMetrics.Log. This ensures that any content derived from user input cannot inject extra log lines into either the normal log output or the metrics log. - Implement the sanitization locally in
ParatextSyncRunner.Logto minimize impact on existing functionality and avoid broad changes. No new dependencies are required.
Concretely, in src/SIL.XForge.Scripture/Services/ParatextSyncRunner.cs, update Log so that:
- It normalizes
messageinto a new variablesanitizedMessageby stripping newlines. - It uses
sanitizedMessagein_logger.LogInformationand in_syncMetrics.Log.Add(...).
No other files need edits for this specific alert, because the sink is centralized in this logging method.
| @@ -2190,8 +2190,12 @@ | ||
| { | ||
| projectSFId ??= _projectDoc?.Id ?? "unknown"; | ||
| userId ??= _userSecret?.Id ?? "unknown"; | ||
| _logger.LogInformation($"SyncLog ({projectSFId.Sanitize()} {userId.Sanitize()}): {message}"); | ||
| _syncMetrics.Log.Add($"{DateTime.UtcNow:u} {message}"); | ||
| string sanitizedMessage = message | ||
| .Replace(Environment.NewLine, string.Empty) | ||
| .Replace("\r", string.Empty) | ||
| .Replace("\n", string.Empty); | ||
| _logger.LogInformation($"SyncLog ({projectSFId.Sanitize()} {userId.Sanitize()}): {sanitizedMessage}"); | ||
| _syncMetrics.Log.Add($"{DateTime.UtcNow:u} {sanitizedMessage}"); | ||
| } | ||
|
|
||
| private void LogMetric(string message) => Log(message); |
There was a problem hiding this comment.
I'm a bit confused about what the github warning is about; I wonder if it is pointing out a need to sanitize message.
d0e8357 to
f7f04fc
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3726 +/- ##
=======================================
Coverage 81.33% 81.33%
=======================================
Files 620 620
Lines 39037 39037
Branches 6364 6359 -5
=======================================
Hits 31749 31749
Misses 6304 6304
Partials 984 984 ☔ View full report in Codecov by Sentry. |
| projectSFId ??= _projectDoc?.Id ?? "unknown"; | ||
| userId ??= _userSecret?.Id ?? "unknown"; | ||
| _logger.LogInformation($"SyncLog ({projectSFId} {userId}): {message}"); | ||
| _logger.LogInformation($"SyncLog ({projectSFId.Sanitize()} {userId.Sanitize()}): {message}"); |
There was a problem hiding this comment.
I'm a bit confused about what the github warning is about; I wonder if it is pointing out a need to sanitize message.
987f62c to
c5f11b2
Compare
See: https://github.com/sillsdev/web-xforge/security/code-scanning/540
This change is