security: redact sensitive data in logger decorators + httpclient logging#121
Merged
Conversation
…ging Fixes GitHub CodeQL go/clear-text-logging HIGH alerts: 1. logger_decorator.go — strengthen sanitizeLogArgs with a broad case-insensitive sensitive-key list (password, passwd, secret, token, authorization, auth, apikey, api_key, accesskey, access_key, credential, cookie, set-cookie, private_key, privatekey, session, bearer) via isSensitiveKey helper. Apply sanitizeLogArgs in DualWriterLoggerDecorator (both inner + secondary), ValueInjectionLoggerDecorator (on combined args), FilterLoggerDecorator, LevelModifierLoggerDecorator.logWithLevel, and PrefixLoggerDecorator (already had it). BaseLoggerDecorator remains a pure passthrough to preserve the MaskingLogger (logmasker) contract. 2. httpclient/module.go — add isSensitiveHeader + redactHeaders helpers that replace values of Authorization, Proxy-Authorization, Cookie, Set-Cookie, X-Api-Key, X-Auth-Token, and any header containing token/secret/password/ apikey with "***". Apply redactHeaders at both logRequest (~line 695) and logResponse (~line 826) important_headers logging sites. Tests added in logger_decorator_test.go (TestSanitizeLogArgs_BroadSensitiveKeys, TestSanitizeLogArgs_NonSensitivePassThrough, TestDecorators_SensitiveArgsAreMasked) and httpclient/logging_improvements_test.go (TestSensitiveHeadersRedacted). All existing tests continue to pass. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
📋 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: databaseContract diff saved to artifacts/diffs/database.json Module: eventbusContract diff saved to artifacts/diffs/eventbus.json Module: jsonschemaContract diff saved to artifacts/diffs/jsonschema.json Module: letsencryptContract diff saved to artifacts/diffs/letsencrypt.json Module: reverseproxyContract diff saved to artifacts/diffs/reverseproxy.json Artifacts📁 Full contract diffs and JSON artifacts are available in the workflow artifacts. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…king Address adversarial-review findings on the go/clear-text-logging fix: CRITICAL 1 — httpclient detailed-logging path leaked secrets. When LogHeaders/LogBody is enabled, logRequest/logResponse logged the raw httputil dump (Authorization/Cookie/Set-Cookie in clear text) as "details". Added redactDump(string) which scrubs sensitive header values in the dump's header section (up to the first blank line) to "***" while preserving header names, the request/status line, and the body. Applied to both detailed logRequest and logResponse before logging; truncation now operates on the already-redacted string. CRITICAL 2 — over-masking tenantID. "tenant"/"requestid" moved to an exact-match set (sensitiveKeyExact) so tenantID/tenantName/tenantCount (primary observability keys) are no longer masked. IMPORTANT 3+4 — bare auth/token/key substrings over-masked author/authority/authenticated/token_count/primary_key. Replaced the broad Contains list with a precise sensitiveKeySubstrings set (password, passwd, secret, credential, apikey/api_key/api-key, accesskey/access_key, privatekey/private_key, authorization, cookie, bearer, and explicit *_token compound forms). Bare auth/token/key removed. Tests: TestSanitizeLogArgs_SensitiveKeys (precise masks), TestSanitizeLogArgs_NotMasked (over-masking regression guard: tenantID/token_count/author/authority/etc.), TestSensitiveHeadersRedactedInDetailedDump (LogHeaders=true dump redaction), and updated existing detailed-logging assertion to require the Authorization secret VALUE be absent while the header NAME remains. IMPORTANT 5 — residual CodeQL taint through the dump string is expected post-rescan and will be dismissed by the lead; the runtime leak is now structurally redacted. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes GitHub CodeQL go/clear-text-logging HIGH-severity alerts in the root
modularmodule.sanitizeLogArgswith a broad case-insensitive sensitive-key list (password,passwd,secret,token,authorization,auth,apikey,api_key,accesskey,access_key,credential,cookie,set-cookie,private_key,privatekey,session,bearer). AddedisSensitiveKeyhelper for the matching logic. AppliedsanitizeLogArgsin every decorator that forwards raw caller args to a logging sink:DualWriterLoggerDecorator(bothinner+secondary),ValueInjectionLoggerDecorator(on combined args, after injection),FilterLoggerDecorator,LevelModifierLoggerDecorator.logWithLevel, andPrefixLoggerDecorator(already had it).BaseLoggerDecoratorremains a pure passthrough to preserve theMaskingLogger(logmasker subpackage) contract and avoid double-masking.isSensitiveHeaderandredactHeadershelpers. Sensitive headers (Authorization, Proxy-Authorization, Cookie, Set-Cookie, X-Api-Key, X-Auth-Token, and any header whose name containstoken/secret/password/apikey) have their values replaced with***before being logged asimportant_headers. Applied at both thelogRequestandlogResponsesites.What changed
logger_decorator.gosensitiveKeyPatternslist +isSensitiveKeyhelper;sanitizeLogArgsuses contains-match (case-insensitive); sanitization applied inDualWriter,ValueInjection,Filter,LevelModifierdecoratorslogger_decorator_test.goTestSanitizeLogArgs_BroadSensitiveKeys(per-key subtests),TestSanitizeLogArgs_NonSensitivePassThrough,TestDecorators_SensitiveArgsAreMasked(per-decorator subtests)httpclient/module.gosensitiveHeaderPatterns,isSensitiveHeader,redactHeadershelpers; applied at both request and response logging siteshttpclient/logging_improvements_test.goTestSensitiveHeadersRedacted: asserts Authorization/Cookie/Set-Cookie values are masked in loggedimportant_headersTest plan
GOWORK=off go test -race ./...(root) — okcd httpclient && GOWORK=off go test -race ./...— okGOWORK=off go vet ./...— cleanGOWORK=off golangci-lint run ./...(root + httpclient) — 0 issuesgofmt -lon changed files — cleanNotes
LogHeaders=true/LogBody=true, which dumps raw HTTP viahttputil.DumpRequestOut) are out of scope for this PR — those paths intentionally expose full wire content when explicitly enabled by the operator. Those will be triaged separately by the lead.logmaskersubpackage (MaskingLogger) applies its own[REDACTED]strategy before reachingBaseLoggerDecorator, which is whyBaseLoggerDecoratorremains a pure passthrough — adding sanitization there would cause double-masking and break existinglogmaskertests/behavior.🤖 Generated with Claude Code