Skip to content

Security fixes: Sanitize user input in logs (CodeQL High Severity) #172

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
BryanSoltis merged 1 commit into from
Dec 8, 2025
Merged

Security fixes: Sanitize user input in logs (CodeQL High Severity) #172

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/v5.0.0/V5.0.0_MIGRATION_GUIDE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -493,7 +493,7 @@ sudo systemctl status azurenamingtool
- **Port already in use**: Change port in appsettings.json or use environment variable: `ASPNETCORE_URLS=http://localhost:8080`
- **Permission denied (Linux)**: Ensure proper file permissions: `chmod -R 755 /path/to/azurenamingtool`

> **� CRITICAL:** After deployment completes and the application starts, you **MUST** restart it again before proceeding to Step 5. This ensures all v5.0.0 features are properly initialized.
> **🔴 CRITICAL:** After deployment completes and the application starts, you **MUST** restart it again before proceeding to Step 5. This ensures all v5.0.0 features are properly initialized.
>
> **Restart again now:**
> - **Azure App Service:** Azure Portal → Restart
Expand Down
2 changes: 1 addition & 1 deletion docs/v5.0.0/wiki/V5.0.0_MIGRATION_GUIDE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -493,7 +493,7 @@ sudo systemctl status azurenamingtool
- **Port already in use**: Change port in appsettings.json or use environment variable: `ASPNETCORE_URLS=http://localhost:8080`
- **Permission denied (Linux)**: Ensure proper file permissions: `chmod -R 755 /path/to/azurenamingtool`

> **� CRITICAL:** After deployment completes and the application starts, you **MUST** restart it again before proceeding to Step 5. This ensures all v5.0.0 features are properly initialized.
> **🔴 CRITICAL:** After deployment completes and the application starts, you **MUST** restart it again before proceeding to Step 5. This ensures all v5.0.0 features are properly initialized.
>
> **Restart again now:**
> - **Azure App Service:** Azure Portal → Restart
Expand Down
4 changes: 3 additions & 1 deletion scripts/development/convert-services.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
# Script to help identify service conversion targets
# This helps us understand which services follow similar patterns

$servicesPath = "c:\Projects\AzureNamingTool-DEV\src\Services"
param(
[string]$servicesPath = "..\..\src\Services"
)

# Services that need conversion (excluding already done ones)
$servicesToConvert = @(
Expand Down
50 changes: 34 additions & 16 deletions src/Middleware/ApiLoggingMiddleware.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,23 @@ public class ApiLoggingMiddleware
private readonly RequestDelegate _next;
private readonly ILogger<ApiLoggingMiddleware> _logger;

/// <summary>
/// Sanitizes user input for safe logging by removing newlines and control characters.
/// </summary>
private static string SanitizeForLog(string? input)
{
if (string.IsNullOrEmpty(input))
return string.Empty;

// Remove newlines and carriage returns
var sanitized = input.Replace("\r", "").Replace("\n", "");

// Remove other control characters
Comment on lines +22 to +25

Copilot AI Dec 8, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sanitization logic removes newlines and control characters, but there's a subtle issue with the implementation. After removing \r and \n on line 23, line 26 filters out all control characters except space. However, since \r and \n are control characters, they're already handled by line 26's char.IsControl() check, making line 23 redundant.

More importantly, the current implementation may not handle all attack vectors. Consider these potential issues:

  1. Tab characters (\t) are control characters and will be removed, which is good
  2. The condition || c == ' ' preserves spaces, which is intentional
  3. However, Unicode line separators like \u2028 (Line Separator) and \u2029 (Paragraph Separator) are NOT considered control characters by char.IsControl() but can still be used for log injection

For more robust sanitization, consider:

private static string SanitizeForLog(string? input)
{
    if (string.IsNullOrEmpty(input))
        return string.Empty;
    
    // Remove various line break characters
    var sanitized = input
        .Replace("\r", "")
        .Replace("\n", "")
        .Replace("\u2028", "")  // Unicode Line Separator
        .Replace("\u2029", ""); // Unicode Paragraph Separator
    
    // Remove other control characters except space
    sanitized = new string(sanitized.Where(c => !char.IsControl(c) || c == ' ').ToArray());
    
    return sanitized;
}
Suggested change
// Remove newlines and carriage returns
var sanitized = input.Replace("\r", "").Replace("\n", "");
// Remove other control characters
// Remove Unicode line and paragraph separators
var sanitized = input.Replace("\u2028", "").Replace("\u2029", "");
// Remove control characters except space

Copilot uses AI. Check for mistakes.
sanitized = new string(sanitized.Where(c => !char.IsControl(c) || c == ' ').ToArray());

return sanitized;
}
Comment on lines +17 to +29

Copilot AI Dec 8, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new SanitizeForLog() method lacks test coverage. Given that this is a security-critical function addressing High Severity CodeQL alerts, it should have comprehensive unit tests to verify:

  1. Basic functionality (null, empty string, normal strings)
  2. Security cases (strings with \r, \n, control characters)
  3. Edge cases (Unicode line separators \u2028 and \u2029)
  4. Performance (handling of very long strings)

Consider adding a test file such as tests/AzureNamingTool.UnitTests/Middleware/ApiLoggingMiddlewareTests.cs with tests like:

[Theory]
[InlineData("normal text", "normal text")]
[InlineData("text\nwith\nnewlines", "textwithnewlines")]
[InlineData("text\r\nwith\r\nCRLF", "textwithCRLF")]
[InlineData("text\twith\ttabs", "textwithtabs")]
[InlineData(null, "")]
[InlineData("", "")]
public void SanitizeForLog_ShouldRemoveControlCharacters(string input, string expected)
{
    // Test implementation
}

This is especially important since the project has comprehensive unit test coverage for other services (as seen in tests/AzureNamingTool.UnitTests/Services/).

Copilot uses AI. Check for mistakes.

/// <summary>
/// Initializes a new instance of the <see cref="ApiLoggingMiddleware"/> class.
/// </summary>
Expand Down Expand Up @@ -64,8 +81,8 @@ public async Task InvokeAsync(HttpContext context)

_logger.LogError(ex,
"API request failed. Method: {Method}, Path: {Path}, CorrelationId: {CorrelationId}, Duration: {Duration}ms",
context.Request.Method,
context.Request.Path,
SanitizeForLog(context.Request.Method),
SanitizeForLog(context.Request.Path.ToString()),
correlationId,

Copilot AI Dec 8, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The correlationId is not sanitized here but should be, since it can be controlled by users through the X-Correlation-ID HTTP header (see CorrelationIdMiddleware.cs lines 32-34). An attacker could inject malicious content including newlines to forge log entries.

Change line 86 to:

                    SanitizeForLog(correlationId),

This is consistent with how correlationId is sanitized at line 109 in the request logging.

Suggested change
correlationId,
SanitizeForLog(correlationId),

Copilot uses AI. Check for mistakes.
stopwatch.ElapsedMilliseconds);

Expand All @@ -87,15 +104,16 @@ private async Task LogRequestAsync(HttpContext context, string correlationId)
// Build request details
var requestDetails = new StringBuilder();
requestDetails.AppendLine($"API Request:");
requestDetails.AppendLine($" Method: {request.Method}");
requestDetails.AppendLine($" Path: {request.Path}{request.QueryString}");
requestDetails.AppendLine($" CorrelationId: {correlationId}");
requestDetails.AppendLine($" Method: {SanitizeForLog(request.Method)}");
requestDetails.AppendLine($" Path: {SanitizeForLog(request.Path.ToString())}{SanitizeForLog(request.QueryString.ToString())}");
requestDetails.AppendLine($" CorrelationId: {SanitizeForLog(correlationId)}");

// Log API key info (first few characters only for security)
if (request.Headers.TryGetValue("APIKey", out var apiKey))
{
var maskedKey = apiKey.ToString().Length > 8
? apiKey.ToString().Substring(0, 8) + "..."
var sanitizedKey = SanitizeForLog(apiKey.ToString());
var maskedKey = sanitizedKey.Length > 8
? sanitizedKey.Substring(0, 8) + "..."
: "***";
requestDetails.AppendLine($" APIKey: {maskedKey}");
}
Expand All @@ -114,7 +132,7 @@ private async Task LogRequestAsync(HttpContext context, string correlationId)
var bodyAsText = Encoding.UTF8.GetString(buffer, 0, totalRead);
request.Body.Position = 0; // Reset stream position

requestDetails.AppendLine($" Body: {bodyAsText}");
requestDetails.AppendLine($" Body: {SanitizeForLog(bodyAsText)}");
}

_logger.LogInformation(requestDetails.ToString());
Expand Down Expand Up @@ -154,8 +172,8 @@ private async Task LogResponseAsync(HttpContext context, string correlationId, l
using (_logger.BeginScope(new Dictionary<string, object>
{
["CorrelationId"] = correlationId,

Copilot AI Dec 8, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The correlationId is not sanitized when added to the logging scope dictionary, but it should be since it can be controlled by users through the X-Correlation-ID HTTP header. An attacker could inject malicious content that may appear in structured logs.

Change line 174 to:

                ["CorrelationId"] = SanitizeForLog(correlationId),

This is consistent with how other user-controlled inputs like Method and Path are sanitized in this same scope at lines 175-176.

Suggested change
["CorrelationId"] = correlationId,
["CorrelationId"] = SanitizeForLog(correlationId),

Copilot uses AI. Check for mistakes.
["Method"] = context.Request.Method,
["Path"] = context.Request.Path.ToString(),
["Method"] = SanitizeForLog(context.Request.Method),
["Path"] = SanitizeForLog(context.Request.Path.ToString()),
["StatusCode"] = response.StatusCode,
["DurationMs"] = durationMs
}))
Expand All @@ -164,26 +182,26 @@ private async Task LogResponseAsync(HttpContext context, string correlationId, l
{
_logger.LogError(
"API request completed with error. Method: {Method}, Path: {Path}, StatusCode: {StatusCode}, Duration: {Duration}ms",
context.Request.Method,
context.Request.Path,
SanitizeForLog(context.Request.Method),
SanitizeForLog(context.Request.Path.ToString()),
response.StatusCode,
durationMs);
}
else if (logLevel == LogLevel.Warning)
{
_logger.LogWarning(
"API request completed with client error. Method: {Method}, Path: {Path}, StatusCode: {StatusCode}, Duration: {Duration}ms",
context.Request.Method,
context.Request.Path,
SanitizeForLog(context.Request.Method),
SanitizeForLog(context.Request.Path.ToString()),
response.StatusCode,
durationMs);
}
else if (durationMs > 5000)
{
_logger.LogWarning(
"Slow API request detected. Method: {Method}, Path: {Path}, StatusCode: {StatusCode}, Duration: {Duration}ms",
context.Request.Method,
context.Request.Path,
SanitizeForLog(context.Request.Method),
SanitizeForLog(context.Request.Path.ToString()),
response.StatusCode,
durationMs);
}
Expand Down
10 changes: 5 additions & 5 deletions src/Services/AzureValidationService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,12 +99,12 @@ public async Task<AzureValidationMetadata> ValidateNameAsync(string resourceName
// Use appropriate validation method based on scope
if (isGlobalScope)
{
_logger.LogInformation("Using CheckNameAvailability API for globally unique resource: {ResourceType}", resourceType);
_logger.LogInformation("Using CheckNameAvailability API for globally unique resource: {ResourceType}", SanitizeForLog(resourceType));

Copilot AI Dec 8, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SanitizeForLog() method is being called but is not defined in this class. This will cause a compilation error.

You need to add the same helper method that was added to ApiLoggingMiddleware.cs:

/// <summary>
/// Sanitizes user input for safe logging by removing newlines and control characters.
/// </summary>
private static string SanitizeForLog(string? input)
{
    if (string.IsNullOrEmpty(input))
        return string.Empty;
    
    // Remove newlines and carriage returns
    var sanitized = input.Replace("\r", "").Replace("\n", "");
    
    // Remove other control characters
    sanitized = new string(sanitized.Where(c => !char.IsControl(c) || c == ' ').ToArray());
    
    return sanitized;
}

This should be added near the top of the class, similar to the placement in ApiLoggingMiddleware.cs.

Copilot uses AI. Check for mistakes.
validationResult = await CheckNameAvailabilityAsync(resourceName, resourceType, settings);
}
else
{
_logger.LogInformation("Using Resource Graph query for scoped resource: {ResourceType}", resourceType);
_logger.LogInformation("Using Resource Graph query for scoped resource: {ResourceType}", SanitizeForLog(resourceType));
validationResult = await CheckResourceExistsAsync(resourceName, resourceType, settings);
}

Expand Down Expand Up @@ -580,7 +580,7 @@ private async Task<string> GetClientSecretAsync(AzureValidationSettings settings
var resourceTypeInfo = await GetResourceTypeInfoAsync(resourceType);
if (resourceTypeInfo == null)
{
_logger.LogWarning("Could not find resource type info for {ResourceType}", resourceType);
_logger.LogWarning("Could not find resource type info for {ResourceType}", SanitizeForLog(resourceType));
return (false, new List<string>());
}

Expand Down Expand Up @@ -668,7 +668,7 @@ private async Task<string> GetClientSecretAsync(AzureValidationSettings settings
}
catch (Exception ex)
{
_logger.LogWarning(ex, "Error getting resource type info for {ResourceType}", resourceTypeShortName);
_logger.LogWarning(ex, "Error getting resource type info for {ResourceType}", SanitizeForLog(resourceTypeShortName));
return null;
}
}
Expand All @@ -687,7 +687,7 @@ private async Task<string> GetClientSecretAsync(AzureValidationSettings settings
var resourceTypeInfo = await GetResourceTypeInfoAsync(resourceType);
if (resourceTypeInfo == null)
{
_logger.LogWarning("Could not find resource type info for {ResourceType}, falling back to Resource Graph", resourceType);
_logger.LogWarning("Could not find resource type info for {ResourceType}, falling back to Resource Graph", SanitizeForLog(resourceType));
return await CheckResourceExistsAsync(resourceName, resourceType, settings);
}

Expand Down