Skip to content

Potential fix for code scanning alert no. 84: Clear-text logging of sensitive information #91

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

Open
intel352 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Potential fix for code scanning alert no. 84: Clear-text logging of sensitive information #91

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
41 changes: 37 additions & 4 deletions logger_decorator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,35 @@
d.logWithLevel("debug", msg, args...)
}

// 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
Comment on lines +275 to +301
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
}

// PrefixLoggerDecorator adds a prefix to all log messages.
// This decorator automatically prepends a configured prefix to every log message.
type PrefixLoggerDecorator struct {
Expand Down Expand Up @@ -300,17 +329,21 @@
}

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

Sensitive data returned by HTTP request headers
Loading
flows to a logging call.

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

Sensitive data returned by HTTP request headers
Loading
flows to a logging call.

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:

  1. Invoke sanitizeLogArgs(args) once at the beginning, saving the result in safeArgs.
  2. Use safeArgs instead of args in all subsequent logging calls inside logWithLevel.

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.

Suggested changeset 1
logger_decorator.go

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/logger_decorator.go b/logger_decorator.go
--- a/logger_decorator.go
+++ b/logger_decorator.go
@@ -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...)
 		}
 	}
 }
EOF
@@ -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...)
}
}
}
Copilot is powered by AI and may make mistakes. Always verify output.
}

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

Sensitive data returned by HTTP request headers
Loading
flows to a logging call.

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’s logDryRunResult, 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 strengthen sanitizeLogArgs so 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.

Suggested changeset 1
logger_decorator.go

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/logger_decorator.go b/logger_decorator.go
--- a/logger_decorator.go
+++ b/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] = "***"
EOF
@@ -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] = "***"
Copilot is powered by AI and may make mistakes. Always verify output.
}

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...)
Comment on lines 331 to +348
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

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

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.

Copilot generated this review using guidance from repository custom instructions.

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information High

Sensitive data returned by HTTP request headers
Loading
flows to a logging call.

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:

  1. Ensure all decorator entry points that accept args ...any sanitize them before logging.
  2. Specifically, update LevelModifierLoggerDecorator.logWithLevel so that it calls sanitizeLogArgs(args) once, and then uses that sanitized slice for all calls to d.inner.Debug/Info/Warn/Error. This is where the current leak exists, because it forwards args unchanged.
  3. Keep PrefixLoggerDecorator as-is, since it already sanitizes args via sanitizeLogArgs.

Concretely:

  • In logger_decorator.go, within LevelModifierLoggerDecorator.logWithLevel, introduce a safeArgs := sanitizeLogArgs(args) at the top and use safeArgs... instead of args... in all logging calls.
  • No changes are needed in modules/reverseproxy/dryrun.go beyond what’s already there, because its responsibility is to build logAttrs; 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.

Suggested changeset 1
logger_decorator.go

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/logger_decorator.go b/logger_decorator.go
--- a/logger_decorator.go
+++ b/logger_decorator.go
@@ -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...)
 		}
 	}
 }
EOF
@@ -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...)
}
}
}
Copilot is powered by AI and may make mistakes. Always verify output.
}
Loading