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}
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.