feat: implement logging sanitization to prevent log forging attacks a…#200
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| { | ||
| request.Headers.Add(auth.ApiKeyHeader, auth.ApiKey); | ||
| _logger.LogDebug("Applied API Key authentication with header: {Header}", auth.ApiKeyHeader); | ||
| _logger.LogDebug("Applied API Key authentication with header: {Header}", SchemaResolverService.SanitizeStringForLogging(auth.ApiKeyHeader)); |
Check failure
Code scanning / CodeQL
Log entries created from user input High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 6 hours ago
General approach: Avoid logging user-controlled authentication header metadata when it can be influenced from the request. For this specific case, the log message does not strictly need to include the header name to be useful; we can instead log a generic message ("Applied API Key authentication") that does not depend on user input at all. Since other log messages in the same method already avoid including secrets or user-controlled header values, this change preserves functionality while eliminating the tainted data at the sink.
Concrete change:
- In
OpenReferralApi.Core/Services/SchemaResolverService.cs, inApplyAuthentication(HttpRequestMessage request, DataSourceAuthentication auth), update the_logger.LogDebugcall in the API key block to no longer useauth.ApiKeyHeader(even viaSanitizeStringForLogging). Replace:"Applied API Key authentication with header: {Header}", SchemaResolverService.SanitizeStringForLogging(auth.ApiKeyHeader)
with:"Applied API Key authentication"(no additional arguments).
- No other behavior is changed: headers are still set the same way on the outgoing request; only the debug log content is simplified.
- No new imports or helper methods are required.
This single modification removes the user-controlled data from the log sink and should resolve all alert variants focused on this logging statement.
| @@ -197,7 +197,7 @@ | ||
| if (!string.IsNullOrEmpty(auth.ApiKey)) | ||
| { | ||
| request.Headers.Add(auth.ApiKeyHeader, auth.ApiKey); | ||
| _logger.LogDebug("Applied API Key authentication with header: {Header}", SchemaResolverService.SanitizeStringForLogging(auth.ApiKeyHeader)); | ||
| _logger.LogDebug("Applied API Key authentication"); | ||
| } | ||
|
|
||
| // Apply Bearer Token authentication |
| System.Text.Encoding.ASCII.GetBytes($"{auth.BasicAuth.Username}:{auth.BasicAuth.Password}")); | ||
| request.Headers.Authorization = new System.Net.Http.Headers.AuthenticationHeaderValue("Basic", credentials); | ||
| _logger.LogDebug("Applied Basic authentication for user: {Username}", auth.BasicAuth.Username); | ||
| _logger.LogDebug("Applied Basic authentication for user: {Username}", SchemaResolverService.SanitizeStringForLogging(auth.BasicAuth.Username)); |
Check failure
Code scanning / CodeQL
Log entries created from user input High
Copilot Autofix
AI about 6 hours ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| { | ||
| request.Headers.Add(header.Key, header.Value); | ||
| _logger.LogDebug("Applied custom header: {HeaderName}", header.Key); | ||
| _logger.LogDebug("Applied custom header: {HeaderName}", SchemaResolverService.SanitizeStringForLogging(header.Key)); |
Check failure
Code scanning / CodeQL
Log entries created from user input High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 6 hours ago
General approach: Ensure any user-controlled strings written to logs are sanitized to eliminate control characters (especially CR/LF), limit length, and avoid ambiguity. For HTTP headers, avoid logging sensitive header values and only log tightly constrained, sanitized header names or a fixed marker.
In this codebase, the only flagged sink is the debug log inside SchemaResolverService.ApplyAuthentication for custom headers:
_logger.LogDebug("Applied custom header: {HeaderName}", SchemaResolverService.SanitizeStringForLogging(header.Key));SanitizeStringForLogging already strips control characters and truncates the string, which is good. To make this unquestionably safe and satisfy the static analysis, we can do two things:
- Ensure we never log
nullor ambiguous values for header names by guarding and making the log message generic if the key is missing or empty. - Continue to rely on
SanitizeStringForLoggingas the central encoding/sanitization routine and keep using structured logging.
This is a very small, localized change in OpenReferralApi.Core/Services/SchemaResolverService.cs inside ApplyAuthentication. No changes are needed in the controllers or OpenApiValidationService because they already log URLs via SanitizeUrlForLogging, which is appropriate for their context.
Concretely:
- In
ApplyAuthentication, inside the loop overauth.CustomHeaders, compute a safe header name string that falls back to a fixed token whenheader.Keyis null/empty, and pass that throughSanitizeStringForLoggingbefore logging. - Keep the rest of the method intact so functionality (actually setting headers) is unchanged.
| @@ -222,7 +222,8 @@ | ||
| foreach (var header in auth.CustomHeaders) | ||
| { | ||
| request.Headers.Add(header.Key, header.Value); | ||
| _logger.LogDebug("Applied custom header: {HeaderName}", SchemaResolverService.SanitizeStringForLogging(header.Key)); | ||
| var safeHeaderName = SchemaResolverService.SanitizeStringForLogging(header.Key ?? string.Empty); | ||
| _logger.LogDebug("Applied custom header: {HeaderName}", string.IsNullOrEmpty(safeHeaderName) ? "<empty-header-name>" : safeHeaderName); | ||
| } | ||
| } | ||
| } |
jeffcumpsty-tpx
deleted the
feat--implement-logging-sanitization-to-prevent-log-forging-attacks-across-services
branch
1 participant
…cross services