🩹 fix(logger): sanitize control characters in user-controlled tags (log injection)#4460
🩹 fix(logger): sanitize control characters in user-controlled tags (log injection)#4460yathboss wants to merge 1 commit into
Conversation
The logger middleware wrote user-controlled values (path, URL, headers, body, query/form/cookie values, locals, error message, ...) straight to the log line without sanitization. An attacker could embed CR/LF in request data to forge additional log entries (log/CRLF injection), deceiving log processors and SIEM systems. This mirrors the existing sanitization already applied to contextual log fields in log/context.go. The shared helpers (WriteSanitized, WriteSanitizedString, IsControlByte) are lifted into the internal logtemplate package — which both the log package and the logger middleware already depend on for Buffer — so there is a single implementation. log/context.go now delegates to it instead of keeping a private copy. ASCII control bytes (CR, LF, NUL, the other C0 bytes and DEL) are replaced with spaces; tab is preserved because operators rely on it for field delimiting, and non-ASCII bytes pass through untouched. The hot path stays alloc-free for already-clean input. Tests cover the helper directly and the affected logger tags end to end (user-agent, referer, custom request header, locals, and the error tag with and without colors), asserting no raw CR/LF survives. Fixes gofiber#4341 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
WalkthroughA shared control-byte sanitization package was added and wired into logger output paths. Logger tag renderers now write request, response, locals, query, form, cookie, and error values through sanitized helpers, with new tests covering control characters and preserved tabs. ChangesLog control-byte sanitization
Sequence Diagram(s)sequenceDiagram
participant Ctx as fiber.Ctx
participant Tags as middleware/logger/tags.go
participant Sanitizer as logtemplate.WriteSanitized*
participant Output as Buffer
Ctx->>Tags: path, headers, body, locals, error text
Tags->>Sanitizer: user-controlled string or []byte
Sanitizer->>Output: sanitized log bytes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
middleware/logger/sanitize_test.go (1)
21-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBroaden E2E coverage for the remaining sanitized tags.
This table covers header-derived tags, while the PR objective also calls out path, URL, query params, cookies, bodies, response bodies, host/IP/XFF, and full request headers. Adding a few representative cases here would catch future regressions where a tag accidentally switches back to direct writes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@middleware/logger/sanitize_test.go` around lines 21 - 59, The sanitize test table currently covers only header-derived tags, but it should also exercise the remaining sanitized placeholders to match the PR goal. Expand the cases in sanitize_test.go within the existing sanitize test table to include representative path, URL, query param, cookie, request body, response body, host/IP/XFF, and full request header tags, using the same setup/want pattern so regressions in the sanitizer logic are caught across all tag sources.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@middleware/logger/sanitize_test.go`:
- Around line 21-59: The sanitize test table currently covers only
header-derived tags, but it should also exercise the remaining sanitized
placeholders to match the PR goal. Expand the cases in sanitize_test.go within
the existing sanitize test table to include representative path, URL, query
param, cookie, request body, response body, host/IP/XFF, and full request header
tags, using the same setup/want pattern so regressions in the sanitizer logic
are caught across all tag sources.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: dc6e405d-e4bb-47f9-b364-aa9de79c31fe
📒 Files selected for processing (6)
internal/logtemplate/sanitize.gointernal/logtemplate/sanitize_test.golog/context.gomiddleware/logger/sanitize_test.gomiddleware/logger/tags.gomiddleware/logger/utils.go
|
All this is doing is moving things from internal/logtemplate/context.go into internal/logtemplate/sanitize.go ? The middleware logger does need this, but why move everything else? |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4460 +/- ##
==========================================
- Coverage 91.77% 91.73% -0.04%
==========================================
Files 138 138
Lines 13486 13470 -16
==========================================
- Hits 12377 12357 -20
- Misses 700 702 +2
- Partials 409 411 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
3 participants
Description
The logger middleware rendered user-controlled values directly into the log line without sanitizing ASCII control characters. An attacker could embed
CR/LF(e.g.GET /admin%0d%0a200 GET /legit) in any request-derived field and forge additional, attacker-chosen log entries — a classic log / CRLF injection that can deceive log processors and SIEM systems and corrupt audit trails.This PR sanitizes every user-controlled logger tag, mirroring the sanitization that is already applied to contextual log fields in
log/context.go.Fixes #4341
Changes introduced
Shared helper, single implementation. Lifted the existing sanitization helpers (
WriteSanitized,WriteSanitizedString,IsControlByte, and theneedsControlSanitize*fast-path checks) out oflog/context.gointo theinternal/logtemplatepackage, which both thelogpackage and the logger middleware already depend on forBuffer.log/context.gonow delegates to it, so there is exactly one copy of the logic.Logger tags sanitized.
middleware/logger/tags.gonow routes all user-controlled tags through the sanitizing writers:path,url,ua,referer,ip,ips,host,body,resBody,reqHeaders,queryParams, the parametricreqHeader:,respHeader:,query:,form:,cookie:,locals:, and theerrortag. Server-controlled tags (method, status, latency, colors, route, pid, time, ...) are left untouched.Behavior.
CR,LF,NUL, the other C0 bytes andDELare replaced with spaces.TABis preserved (operators use it to delimit fields) and non-ASCII bytes pass through unchanged. The hot path stays alloc-free for already-clean input (the common case) via a fast pre-scan.Benchmarks: No new allocations on the clean path — sanitization only allocates a scratch buffer when a control byte is actually present.
Documentation Update: Not required — no public API change. The helpers are in an internal package.
Changelog/What's New: Security fix for log injection in the logger middleware.
API Alignment with Express: N/A (security hardening of existing behavior).
API Longevity: No exported API changes; the new helpers live in
internal/.Examples: N/A.
Type of change
Checklist
internal/logtemplate/sanitize_test.go,middleware/logger/sanitize_test.go).go test ./internal/logtemplate/ ./log/ ./middleware/logger/),go vetandgofmtare clean.