Potential fix for code scanning alert no. 4: Log entries created from user input#3
Potential fix for code scanning alert no. 4: Log entries created from user input#3AlahmadiQ8 wants to merge 1 commit intomainfrom
Conversation
… user input Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses a security vulnerability (code scanning alert #4) by sanitizing user-controlled data before logging to prevent log injection attacks. The fix adds newline character removal (\r and \n) to validation error messages that could contain malicious content from image data URIs.
Key Changes
- Added sanitization to log output by removing carriage return and newline characters from validation error messages
- Added explicit
using System.Linq;directive to support the.Select()LINQ method used in sanitization
| using OpenAI.Responses; | ||
| using System.Runtime.CompilerServices; | ||
| using WebApp.Api.Models; | ||
| using System.Linq; |
There was a problem hiding this comment.
The using System.Linq; directive is redundant because the project has <ImplicitUsings>enable</ImplicitUsings> in the csproj file (line 6), which automatically includes System.Linq. While harmless, it's unnecessary.
| using System.Linq; |
| "Image attachment validation failed: {Errors}", | ||
| string.Join("; ", validationErrors.Select(e => e.Replace("\r", "").Replace("\n", ""))) | ||
| ); | ||
| throw new ArgumentException($"Invalid image attachments: {string.Join(", ", validationErrors)}"); |
There was a problem hiding this comment.
The sanitization applied to the log statement should also be applied to the exception message on line 384. The same validationErrors list contains user-controlled data (MIME types from data URIs) that could include newline characters, creating a log injection vulnerability when the exception is logged by the caller.
Consider applying the same sanitization:
throw new ArgumentException($"Invalid image attachments: {string.Join(", ", validationErrors.Select(e => e.Replace("\r", "").Replace("\n", "")))}");| throw new ArgumentException($"Invalid image attachments: {string.Join(", ", validationErrors)}"); | |
| throw new ArgumentException($"Invalid image attachments: {string.Join(", ", validationErrors.Select(e => e.Replace("\r", "").Replace("\n", "")))}"); |
Potential fix for https://github.com/AlahmadiQ8/foundry-agent-webapp/security/code-scanning/4
To fix this issue, all entries originating from user-provided data (such as those in
errorsthat flow to log contents) should be sanitized before being logged. Since the log entry is plain text, the primary concern is removing any characters that could cause log forging: newline (\n,\r), and other characters that could disrupt log format (such as control characters). This can be achieved by sanitizing each string output viaString.Replacefor\rand\n, or potentially filtering out other non-printable characters if necessary. The best practice is to sanitize each element ofvalidationErrorsbefore sending to the log, so that maliciously crafted image URIs or MIME types do not trigger new lines or spoof log entries.Specifically, update the log statement on line 379 to sanitize each error string in
validationErrorsby replacing/removing\rand\n.You'll need to use LINQ's
.Select()for mapping, so ensureusing System.Linq;is present.Only the affected snippet in
backend/WebApp.Api/Services/AzureAIAgentService.csneeds updating.Suggested fixes powered by Copilot Autofix. Review carefully before merging.