Potential fix for code scanning alert no. 84: Clear-text logging of sensitive information#91
Potential fix for code scanning alert no. 84: Clear-text logging of sensitive information#91
Conversation
…ensitive information Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
📋 API Contract Changes Summary✅ No breaking changes detected - only additions and non-breaking modifications Changed Components:Core FrameworkContract diff saved to artifacts/diffs/core.json Module: authContract diff saved to artifacts/diffs/auth.json Module: cacheContract diff saved to artifacts/diffs/cache.json Module: chimuxContract diff saved to artifacts/diffs/chimux.json Module: configwatcherContract diff saved to artifacts/diffs/configwatcher.json Module: databaseContract diff saved to artifacts/diffs/database.json Module: eventbusContract diff saved to artifacts/diffs/eventbus.json Module: eventloggerContract diff saved to artifacts/diffs/eventlogger.json Module: httpclientContract diff saved to artifacts/diffs/httpclient.json Module: httpserverContract diff saved to artifacts/diffs/httpserver.json Module: jsonschemaContract diff saved to artifacts/diffs/jsonschema.json Module: letsencryptContract diff saved to artifacts/diffs/letsencrypt.json Module: logmaskerContract diff saved to artifacts/diffs/logmasker.json Module: reverseproxyContract diff saved to artifacts/diffs/reverseproxy.json Module: schedulerContract diff saved to artifacts/diffs/scheduler.json Artifacts📁 Full contract diffs and JSON artifacts are available in the workflow artifacts. |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR introduces a small sanitization layer in the logging decorator stack to mitigate CodeQL alert #84 (“clear-text logging of sensitive information”) by masking header-derived identifiers before they reach the underlying logger.
Changes:
- Added
sanitizeLogArgs(args []any) []anyto mask values for specific structured-log keys (tenant,requestId). - Updated
PrefixLoggerDecorator(Info,Error,Warn,Debug) to forward sanitized structured logging arguments to the inner logger.
| // sanitizeLogArgs masks potentially sensitive values in structured log arguments. | ||
| // It assumes key/value pairs (key at even index, value at odd index). | ||
| func sanitizeLogArgs(args []any) []any { | ||
| if len(args) == 0 { | ||
| return args | ||
| } | ||
|
|
||
| // Work on a shallow copy to avoid surprising callers that reuse the slice. | ||
| sanitized := make([]any, len(args)) | ||
| copy(sanitized, args) | ||
|
|
||
| for i := 0; i < len(sanitized); i += 2 { | ||
| key, ok := sanitized[i].(string) | ||
| if !ok { | ||
| continue | ||
| } | ||
|
|
||
| // Mask values for known potentially sensitive keys. | ||
| if key == "tenant" || key == "requestId" { | ||
| valueIndex := i + 1 | ||
| if valueIndex < len(sanitized) { | ||
| sanitized[valueIndex] = "***" | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return sanitized |
There was a problem hiding this comment.
sanitizeLogArgs currently allocates and copies the args slice on every log call (when args is non-empty), even when no sensitive keys are present. This adds avoidable per-log overhead for PrefixLoggerDecorator.
Consider scanning for matching keys first and only allocating/copying when a redaction is actually needed (e.g., copy-on-first-match), otherwise return the original args slice unchanged.
| func (d *PrefixLoggerDecorator) Info(msg string, args ...any) { | ||
| d.inner.Info(d.formatMessage(msg), args...) | ||
| safeArgs := sanitizeLogArgs(args) | ||
| d.inner.Info(d.formatMessage(msg), safeArgs...) | ||
| } | ||
|
|
||
| func (d *PrefixLoggerDecorator) Error(msg string, args ...any) { | ||
| d.inner.Error(d.formatMessage(msg), args...) | ||
| safeArgs := sanitizeLogArgs(args) | ||
| d.inner.Error(d.formatMessage(msg), safeArgs...) | ||
| } | ||
|
|
||
| func (d *PrefixLoggerDecorator) Warn(msg string, args ...any) { | ||
| d.inner.Warn(d.formatMessage(msg), args...) | ||
| safeArgs := sanitizeLogArgs(args) | ||
| d.inner.Warn(d.formatMessage(msg), safeArgs...) | ||
| } | ||
|
|
||
| func (d *PrefixLoggerDecorator) Debug(msg string, args ...any) { | ||
| d.inner.Debug(d.formatMessage(msg), args...) | ||
| safeArgs := sanitizeLogArgs(args) | ||
| d.inner.Debug(d.formatMessage(msg), safeArgs...) |
There was a problem hiding this comment.
New redaction behavior is introduced (masking values for "tenant" and "requestId" keys), but there are existing tests for logger_decorator.go and none appear to assert this new behavior. Please add a focused unit test that verifies PrefixLoggerDecorator forwards masked values for these keys (and leaves other keys unchanged) to prevent regressions and to validate the intended security fix.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| func (d *PrefixLoggerDecorator) Info(msg string, args ...any) { | ||
| d.inner.Info(d.formatMessage(msg), args...) | ||
| safeArgs := sanitizeLogArgs(args) | ||
| d.inner.Info(d.formatMessage(msg), safeArgs...) |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
Copilot Autofix
AI 5 days 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.
| func (d *PrefixLoggerDecorator) Error(msg string, args ...any) { | ||
| d.inner.Error(d.formatMessage(msg), args...) | ||
| safeArgs := sanitizeLogArgs(args) | ||
| d.inner.Error(d.formatMessage(msg), safeArgs...) |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 days ago
Generally, to fix clear-text logging of sensitive information, you either (a) avoid logging the sensitive fields altogether, or (b) sanitize/obfuscate them before logging. This project already defines sanitizeLogArgs to mask specific keys (tenant, requestId), and the PrefixLoggerDecorator uses it. The best fix is to ensure all decorator paths that pass structured arguments to the underlying logger apply this sanitizer.
Concretely, CodeQL’s taint path goes through LevelModifierLoggerDecorator.logWithLevel, which currently forwards args directly to d.inner.<Level>(msg, args...). We should change logWithLevel to:
- Invoke
sanitizeLogArgs(args)once at the beginning, saving the result insafeArgs. - Use
safeArgsinstead ofargsin all subsequent logging calls insidelogWithLevel.
This ensures that any structured log attributes—regardless of which decorator stack is used—have tenant and request IDs masked before reaching the sink. No behavior changes for non-sensitive keys, and we don’t need to modify DryRunHandler or PrefixLoggerDecorator further, since they already either don’t sanitize (but will now be protected if routed through LevelModifier) or already call sanitizeLogArgs.
Changes are limited to logger_decorator.go, within the logWithLevel method at lines 229–256. No new imports or external packages are needed.
| @@ -232,26 +232,28 @@ | ||
| targetLevel = mapped | ||
| } | ||
|
|
||
| safeArgs := sanitizeLogArgs(args) | ||
|
|
||
| switch targetLevel { | ||
| case "debug": | ||
| d.inner.Debug(msg, args...) | ||
| d.inner.Debug(msg, safeArgs...) | ||
| case "info": | ||
| d.inner.Info(msg, args...) | ||
| d.inner.Info(msg, safeArgs...) | ||
| case "warn": | ||
| d.inner.Warn(msg, args...) | ||
| d.inner.Warn(msg, safeArgs...) | ||
| case "error": | ||
| d.inner.Error(msg, args...) | ||
| d.inner.Error(msg, safeArgs...) | ||
| default: | ||
| // If unknown level, use original | ||
| switch originalLevel { | ||
| case "debug": | ||
| d.inner.Debug(msg, args...) | ||
| d.inner.Debug(msg, safeArgs...) | ||
| case "info": | ||
| d.inner.Info(msg, args...) | ||
| d.inner.Info(msg, safeArgs...) | ||
| case "warn": | ||
| d.inner.Warn(msg, args...) | ||
| d.inner.Warn(msg, safeArgs...) | ||
| case "error": | ||
| d.inner.Error(msg, args...) | ||
| d.inner.Error(msg, safeArgs...) | ||
| } | ||
| } | ||
| } |
| func (d *PrefixLoggerDecorator) Warn(msg string, args ...any) { | ||
| d.inner.Warn(d.formatMessage(msg), args...) | ||
| safeArgs := sanitizeLogArgs(args) | ||
| d.inner.Warn(d.formatMessage(msg), safeArgs...) |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 days ago
In general terms, we should ensure that any sensitive data flowing from HTTP headers into logs is either omitted or masked before being written. In this codebase, that is already the intent of sanitizeLogArgs: it inspects structured logging key/value pairs and masks values for certain keys. The best fix, without changing existing behavior beyond improving safety, is to (a) ensure that the keys used when logging potentially sensitive fields match what sanitizeLogArgs expects, and (b) expand sanitizeLogArgs to cover other likely-sensitive keys used in structured logs. We do not need to change the PrefixLoggerDecorator logging calls themselves; we only need to ensure the sanitizer actually handles the keys being logged by logDryRunResult.
Concretely:
- In
modules/reverseproxy/dryrun.go’slogDryRunResult, the tenant is already logged with key"tenant"and request ID with key"requestId", which matches the existing sanitizer conditions. No change is required there. - In
modular/logger_decorator.go, we will strengthensanitizeLogArgsso it masks a slightly broader set of sensitive keys commonly used in this code path, such as"tenant","tenantId","requestId", and"requestID"(to cover camelCase vs. different capitalizations), while keeping the same interface and overall behavior.
This way, any call like d.logger.Warn(message, logAttrs...) will go through PrefixLoggerDecorator.Warn, which invokes sanitizeLogArgs and ensures that tenant and requestId (and close variants) are replaced with "***" before being logged. No new imports are required; we only edit the existing sanitizeLogArgs function in logger_decorator.go.
| @@ -290,7 +290,9 @@ | ||
| } | ||
|
|
||
| // Mask values for known potentially sensitive keys. | ||
| if key == "tenant" || key == "requestId" { | ||
| switch key { | ||
| case "tenant", "tenantId", "tenantID", | ||
| "requestId", "requestID": | ||
| valueIndex := i + 1 | ||
| if valueIndex < len(sanitized) { | ||
| sanitized[valueIndex] = "***" |
| func (d *PrefixLoggerDecorator) Debug(msg string, args ...any) { | ||
| d.inner.Debug(d.formatMessage(msg), args...) | ||
| safeArgs := sanitizeLogArgs(args) | ||
| d.inner.Debug(d.formatMessage(msg), safeArgs...) |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 days ago
In general, to fix clear-text logging of sensitive information, ensure that any values derived from untrusted or sensitive sources (like HTTP headers) are either omitted or masked before they reach any logging sink. In this codebase, that means consistently applying sanitizeLogArgs to the structured args slice before calling any underlying logger methods, so that "tenant" and "requestId" values are always replaced with "***" regardless of which decorator chain is used.
The best minimal fix without changing existing functionality is:
- Ensure all decorator entry points that accept
args ...anysanitize them before logging. - Specifically, update
LevelModifierLoggerDecorator.logWithLevelso that it callssanitizeLogArgs(args)once, and then uses that sanitized slice for all calls tod.inner.Debug/Info/Warn/Error. This is where the current leak exists, because it forwardsargsunchanged. - Keep
PrefixLoggerDecoratoras-is, since it already sanitizesargsviasanitizeLogArgs.
Concretely:
- In
logger_decorator.go, withinLevelModifierLoggerDecorator.logWithLevel, introduce asafeArgs := sanitizeLogArgs(args)at the top and usesafeArgs...instead ofargs...in all logging calls. - No changes are needed in
modules/reverseproxy/dryrun.gobeyond what’s already there, because its responsibility is to buildlogAttrs; the masking should happen at the logging abstraction layer.
No new imports or types are needed; the existing sanitizeLogArgs function is already in the same file and package.
| @@ -232,26 +232,28 @@ | ||
| targetLevel = mapped | ||
| } | ||
|
|
||
| safeArgs := sanitizeLogArgs(args) | ||
|
|
||
| switch targetLevel { | ||
| case "debug": | ||
| d.inner.Debug(msg, args...) | ||
| d.inner.Debug(msg, safeArgs...) | ||
| case "info": | ||
| d.inner.Info(msg, args...) | ||
| d.inner.Info(msg, safeArgs...) | ||
| case "warn": | ||
| d.inner.Warn(msg, args...) | ||
| d.inner.Warn(msg, safeArgs...) | ||
| case "error": | ||
| d.inner.Error(msg, args...) | ||
| d.inner.Error(msg, safeArgs...) | ||
| default: | ||
| // If unknown level, use original | ||
| switch originalLevel { | ||
| case "debug": | ||
| d.inner.Debug(msg, args...) | ||
| d.inner.Debug(msg, safeArgs...) | ||
| case "info": | ||
| d.inner.Info(msg, args...) | ||
| d.inner.Info(msg, safeArgs...) | ||
| case "warn": | ||
| d.inner.Warn(msg, args...) | ||
| d.inner.Warn(msg, safeArgs...) | ||
| case "error": | ||
| d.inner.Error(msg, args...) | ||
| d.inner.Error(msg, safeArgs...) | ||
| } | ||
| } | ||
| } |
Potential fix for https://github.com/GoCodeAlone/modular/security/code-scanning/84
In general, to fix clear-text logging of potentially sensitive data flowing through generic logging decorators, we should ensure that any untrusted or header-derived values are either omitted, masked, or sanitized before being passed to the underlying logger. Since
LoggerDecoratorand its implementations are generic and receiveargs ...anyfrom many call sites (includingDryRunHandler.logDryRunResult), the safest fix that does not change external behavior is to introduce a small sanitization step in the decorator layer that can scrub known-sensitive keys or values in the structured logging arguments.The best targeted fix here is to add a helper in
logger_decorator.go(within the shown code) that inspectsargs ...anyas key/value pairs and masks values for keys that might contain sensitive data coming from HTTP headers (for example"tenant","requestId", and any other we choose), or more generally redact any string values that look like they could hold secrets (optional). Then, we apply this sanitization in the single place where CodeQL points to the sink:PrefixLoggerDecorator.Debug(and, by symmetry and for consistency, we should apply the same sanitization in allPrefixLoggerDecoratormethods so the decorator never forwards raw taintedargsto the inner logger). This preserves existing message strings and keys while only altering logged values according to a deterministic masking rule.Concretely:
logger_decorator.go, add a private helper function, e.g.sanitizeLogArgs(args []any) []any, near the decorators, using only the standard library (stringsis already imported).len(args) == 0.logDryRunResult), iterate over the slice, and for every string key in positions0,2,4,...:"tenant"or"requestId"(or both, matching the dry-run logger), replace the corresponding value (if present) with a masked form such as"***".PrefixLoggerDecorator.Info,.Error,.Warn, and.Debug, callsanitizeLogArgs(args)before passing them down tod.inner.*.logger_decorator.gowithin the shown regions.Suggested fixes powered by Copilot Autofix. Review carefully before merging.