fix(http): redact credentials in request logs (CodeQL clear-text logging) - #328
Conversation
|
|
|
Warning Review limit reached
Next review available in: 42 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Gavel summary
Totals: 424 passed · 0 failed · 3 skipped · 16.2s |
…ing) The HTTP logging middleware wrote request URLs and transport errors to logs verbatim. When a URL carries userinfo (https://user:pass@host) or a transport error wraps such a URL, the embedded password was logged in clear text — the "Clear-text logging of sensitive information" alert at http/middlewares/logger.go. - Log req.URL via URL.Redacted(), which masks the userinfo password. - accessURL() now returns URL.Redacted() as well. - Run transport error strings through logger.StripSecrets before logging. Also document why MD5 is retained in http/digest.go: HTTP Digest auth (RFC 7616) mandates MD5 as the default algorithm and the server selects it, so it cannot be removed without breaking interoperability. SHA-256 is already used whenever the server offers it. The weak-hash alert there is a known false positive for digest authentication.
62859de to
24f1f3b
Compare
Go 1.25.12 changed os.Root.MkdirAll to reject paths with a trailing slash, returning "mkdirat <path>: no such file or directory". Unarchive passed raw tar/zip entry names (e.g. "subdir/", "./subdir3/") straight to root.MkdirAll, so directory extraction failed on that toolchain — while 1.25.1 through 1.25.11 accepted the same paths. Normalize the entry name with filepath.Clean before creating the directory. The value recorded in Archive.Directories is left unchanged so the reported path format is preserved. Verified against go1.25.1 and go1.25.12 (all files specs pass on both). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013n3Z8YwzdNdmCPCVHhiwgo
3 participants
Summary
Addresses the two open CodeQL code-scanning alerts on
master:http/middlewares/logger.gohttp/digest.go#41 — Clear-text logging (fixed)
The HTTP logging middleware wrote request URLs and transport errors verbatim. When a URL carries userinfo (
https://user:pass@host) — or a transport error wraps such a URL — the embedded password was written to the logs in clear text.req.URLviaURL.Redacted(), which masks the userinfo password, at every log sink injsonLogger.accessURL()now returnsURL.Redacted()too (it previously only stripped the query/fragment, leaving userinfo intact).logger.StripSecrets(...)before logging, so credentials embedded in a wrappedurl.Errorare redacted.The header/body/query/form values reaching these log calls were already sanitized (
SanitizeHeaders,StripSecretsFromMap,IsSensitiveKey); the URL and error paths were the remaining gap.#43 — MD5 in Digest auth (won't fix)
The flagged
md5.New()is HTTP Digest Authentication (RFC 7616), where MD5 is the protocol-mandated default and the algorithm is chosen by the server, not the client. Many servers only offer MD5, so removing it would break authentication against them. SHA-256 is already used whenever the server advertises it.This is a known false positive for digest auth. Rather than change behavior, this PR adds a doc comment on
hashStrexplaining why MD5 is retained. The alert should be dismissed in the Security tab as Won't fix / used as required by protocol.Testing
go build ./...andgo vet ./http/...pass.TestDigestTransport,TestParseWWWAuthenticate,TestDigestAuthString, …) and the fullloggerpackage tests pass. The remainingTestHTTPfailures are unrelated — they require outbound network access to external hosts, which is blocked in the CI sandbox.Note (out of scope)
http/middlewares/trace.go:254also setsreq.URL.String()as an OpenTelemetry span attribute, which has the same userinfo-leak shape but was not flagged by CodeQL. Happy to redact it in a follow-up if desired.🤖 Generated with Claude Code
https://claude.ai/code/session_013n3Z8YwzdNdmCPCVHhiwgo
Generated by Claude Code