Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| if (_logger.IsEnabled(LogLevel.Information)) | ||
| { | ||
| _logger.LogInformation("Auto-starting browser automation session for '{StartUrl}' because no active session was found for the default alias.", bootstrapUrl); |
Check failure
Code scanning / CodeQL
Log entries created from user input High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 days ago
To fix the problem, we should ensure that user-controlled data (bootstrapUrl) is sanitized before being written to the log. Since the log message appears to be plain text and we do not have context that it will be safely HTML-encoded later, the conservative approach is to remove newline characters and other problematic control characters from the string before logging it. This mitigates log forging by preventing attackers from injecting extra log lines or confusing content.
The best minimally invasive fix is to introduce a small, private helper method inside BrowserAutomationService that “log-sanitizes” strings: replacing \r and \n with empty strings (and optionally trimming). Then, at the log site at line 378, call this helper on bootstrapUrl and log the sanitized value instead of the raw one. This keeps existing functionality intact (the URL content is otherwise unchanged) while addressing all CodeQL variants that flag this logging statement.
Concretely:
- In
src/Modules/CrestApps.OrchardCore.AI.Agent/Services/BrowserAutomationService.cs, add a new private static method, e.g.SanitizeForLog(string value), near the other private static helpers (TryGetAbsoluteHttpUrlis a good reference point). - Implement
SanitizeForLogusingstring.Replaceto strip\rand\n(and possibly\t) characters, and return the result (or empty string if null). - Modify the logging call at line 378 to use
SanitizeForLog(bootstrapUrl)instead ofbootstrapUrl. No new external dependencies or using directives are required.
| @@ -375,7 +375,8 @@ | ||
|
|
||
| if (_logger.IsEnabled(LogLevel.Information)) | ||
| { | ||
| _logger.LogInformation("Auto-starting browser automation session for '{StartUrl}' because no active session was found for the default alias.", bootstrapUrl); | ||
| var sanitizedBootstrapUrl = SanitizeForLog(bootstrapUrl); | ||
| _logger.LogInformation("Auto-starting browser automation session for '{StartUrl}' because no active session was found for the default alias.", sanitizedBootstrapUrl); | ||
| } | ||
|
|
||
| var snapshot = await CreateSessionAsync( | ||
| @@ -473,6 +474,19 @@ | ||
| await context.AddCookiesAsync(cookies); | ||
| } | ||
|
|
||
| private static string SanitizeForLog(string value) | ||
| { | ||
| if (string.IsNullOrEmpty(value)) | ||
| { | ||
| return string.Empty; | ||
| } | ||
|
|
||
| // Remove newline characters to prevent log forging. | ||
| return value | ||
| .Replace("\r", string.Empty, StringComparison.Ordinal) | ||
| .Replace("\n", string.Empty, StringComparison.Ordinal); | ||
| } | ||
|
|
||
| private static bool TryGetAbsoluteHttpUrl(string value, out string normalizedUrl) | ||
| { | ||
| normalizedUrl = null; |
|
This pull request has merge conflicts. Please resolve those before requesting a review. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
This pull request has merge conflicts. Please resolve those before requesting a review. |
1 participant
No description provided.