security: validate client-supplied trace IDs#26
Conversation
Add validateTraceID helper that sanitizes trace IDs from gRPC metadata and proto fields: max 128 chars, printable ASCII only (0x20-0x7E). Non-printable/control/unicode bytes are stripped. If all characters are invalid, falls back to generating a new UUID. Called in SetTraceIdWithValue (after metadata extraction) and UpdateTraceId (before storing). Protects logs, Sentry/Rollbar, and OTEL span attributes from injection attacks and DoS via oversized trace IDs.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 12 minutes and 20 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughTrace ID sanitization and configurable validation were added to the notifier package: trace IDs are normalized (printable ASCII only, max 128 chars) and sanitized before use; a package-level validator can be overridden or disabled. README docs were updated and unit tests added to cover the behavior. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
This PR hardens trace ID handling in the notifier package by sanitizing client-supplied trace IDs before they’re propagated into structured logs, error reporters (Sentry/Rollbar/Airbrake), and OTEL span attributes.
Changes:
- Added
validateTraceIDhelper (printable ASCII only, max 128 chars) and applied it to metadata-derived IDs inSetTraceIdWithValueand to caller-supplied IDs inUpdateTraceId. - Added notifier unit tests covering sanitization, truncation, and fallback behavior.
- Regenerated README docs (function index/anchors) to reflect current code.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| README.md | Updates generated doc links/anchors after code movement. |
| notifier/README.md | Updates generated notifier docs, including new/updated trace ID APIs and line anchors. |
| notifier/notifier.go | Introduces validateTraceID and applies sanitization to trace ID resolution/update paths. |
| notifier/notifier_test.go | Adds table-driven tests for trace ID sanitization and integration tests for metadata/update flows. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
notifier/notifier_test.go (1)
242-253: Add an explicit unicode-stripping test case.You validate control-byte stripping, but there’s no direct unicode input assertion yet.
➕ Suggested test case
{ {"spaces preserved", "abc def", "abc def"}, {"printable special chars preserved", "abc!@#$%^&*()def", "abc!@#$%^&*()def"}, + {"unicode stripped", "abcé漢def", "abcdef"}, {"only non-printable returns empty", "\x00\x01\x02", ""}, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notifier/notifier_test.go` around lines 242 - 253, Add a new table test row to explicitly assert how multibyte Unicode is handled: insert an entry alongside the existing cases (e.g., next to "printable special chars preserved") such as {"unicode preserved", "héllø—你好", "héllø—你好"} to verify the sanitizer preserves non-ASCII printable Unicode characters; add or adjust expected output according to the behavior of the sanitizer function used in these tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@notifier/notifier.go`:
- Around line 637-640: UpdateTraceId currently only checks traceID == "" after
validateTraceID, which lets whitespace-only IDs be stored; update it to use the
same "empty after trimming" fallback as SetTraceIdWithValue by trimming the
validated traceID (e.g., strings.TrimSpace(traceID)) and if the result is empty
call SetTraceId(ctx) instead of storing the whitespace-only value; reference
functions: UpdateTraceId, validateTraceID, SetTraceId, SetTraceIdWithValue.
---
Nitpick comments:
In `@notifier/notifier_test.go`:
- Around line 242-253: Add a new table test row to explicitly assert how
multibyte Unicode is handled: insert an entry alongside the existing cases
(e.g., next to "printable special chars preserved") such as {"unicode
preserved", "héllø—你好", "héllø—你好"} to verify the sanitizer preserves non-ASCII
printable Unicode characters; add or adjust expected output according to the
behavior of the sanitizer function used in these tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e3a723b6-cf3c-4b87-b7a6-f14604d53b4a
📒 Files selected for processing (4)
README.mdnotifier/README.mdnotifier/notifier.gonotifier/notifier_test.go
Add SetTraceIDValidator(func(string) string) to allow custom validation or disable it entirely (set to nil). Follows the existing init-only setter pattern. Default behavior unchanged (max 128 chars, printable ASCII only).
Address review feedback: - Add non-ASCII UTF-8 test case (accented, CJK, emoji) to validateTraceID - Use strings.TrimSpace in UpdateTraceId to match SetTraceIdWithValue behavior — whitespace-only trace IDs now fall back to generation
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
notifier/notifier.go:565
- PR description states that when all characters are invalid the logic "falls back to generating a new UUID", but the code paths can fall back to OTEL span trace ID (and
UpdateTraceIdcan fall back toSetTraceId, which may also pick up incoming gRPC metadata). Please update the PR description (or adjust behavior) so the documented fallback matches what ships.
// Fall back to OTEL span trace ID.
if strings.TrimSpace(traceID) == "" && hasSpan {
traceID = span.SpanContext().TraceID().String()
}
// Last resort: generate UUID.
if strings.TrimSpace(traceID) == "" {
u, err := uuid.NewRandom()
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ution flow, not direct generation
There was a problem hiding this comment.
🧹 Nitpick comments (2)
notifier/notifier_test.go (2)
286-293: Unnecessary pre-initialization of trace ID.Line 287 pre-sets an empty trace ID, but
UpdateTraceIdunconditionally overwrites it viaoptions.AddToOptions. This setup is redundant and could be simplified.♻️ Simplified test
func TestUpdateTraceId_ValidatesInput(t *testing.T) { - ctx := options.AddToOptions(context.Background(), tracerID, "") - ctx = UpdateTraceId(ctx, "good-id\x00\nbad") + ctx := UpdateTraceId(context.Background(), "good-id\x00\nbad") got := GetTraceId(ctx) if got != "good-idbad" { t.Errorf("expected sanitized trace ID %q, got %q", "good-idbad", got) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notifier/notifier_test.go` around lines 286 - 293, The test TestUpdateTraceId_ValidatesInput redundantly pre-initializes the trace ID using options.AddToOptions before calling UpdateTraceId, which always overwrites the value; remove the initial options.AddToOptions call and create the context by calling UpdateTraceId directly (e.g., ctx := UpdateTraceId(context.Background(), "good-id\x00\nbad")), leaving the subsequent GetTraceId assertion unchanged to verify sanitization.
303-334: Document that these tests must not run in parallel.These tests modify the package-level
traceIDValidatorwithout synchronization (as documented: "Must be called during init — not safe for concurrent use"). While they currently don't uset.Parallel(), a future contributor adding it would introduce a data race.Consider adding a brief comment at the top of the first validator test to prevent accidental parallelization.
📝 Add clarifying comment
+// TestSetTraceIDValidator_* tests modify package-level state and must not +// run in parallel with each other or with tests that call SetTraceId/UpdateTraceId. func TestSetTraceIDValidator_Custom(t *testing.T) { old := traceIDValidator🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notifier/notifier_test.go` around lines 303 - 334, These tests mutate the package-level traceIDValidator via SetTraceIDValidator (used in TestSetTraceIDValidator_Custom and TestSetTraceIDValidator_Nil) and are not concurrency-safe; add a short comment at the top of the validator tests (e.g., at the start of TestSetTraceIDValidator_Custom, and mirror in TestSetTraceIDValidator_Nil) stating that the tests modify traceIDValidator and must not be run in parallel (do not call t.Parallel()), so future contributors don't accidentally parallelize them.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@notifier/notifier_test.go`:
- Around line 286-293: The test TestUpdateTraceId_ValidatesInput redundantly
pre-initializes the trace ID using options.AddToOptions before calling
UpdateTraceId, which always overwrites the value; remove the initial
options.AddToOptions call and create the context by calling UpdateTraceId
directly (e.g., ctx := UpdateTraceId(context.Background(), "good-id\x00\nbad")),
leaving the subsequent GetTraceId assertion unchanged to verify sanitization.
- Around line 303-334: These tests mutate the package-level traceIDValidator via
SetTraceIDValidator (used in TestSetTraceIDValidator_Custom and
TestSetTraceIDValidator_Nil) and are not concurrency-safe; add a short comment
at the top of the validator tests (e.g., at the start of
TestSetTraceIDValidator_Custom, and mirror in TestSetTraceIDValidator_Nil)
stating that the tests modify traceIDValidator and must not be run in parallel
(do not call t.Parallel()), so future contributors don't accidentally
parallelize them.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 90fdc3c4-2931-442d-a9af-cf4fec8beb27
📒 Files selected for processing (3)
notifier/README.mdnotifier/notifier.gonotifier/notifier_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- notifier/notifier.go
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
validateTraceIDhelper: max 128 chars, printable ASCII only (0x20-0x7E), strips control/unicode bytesSetTraceIdWithValue(after gRPC metadata extraction) andUpdateTraceId(before storing)Test plan
TestValidateTraceID— table-driven: normal, truncation, stripping, edge casesTestSetTraceId_ValidatesMetadataTraceID— dirty metadata sanitizedTestSetTraceId_TruncatesLongMetadataTraceID— 200 chars → 128TestUpdateTraceId_ValidatesInput— dirty input sanitizedTestUpdateTraceId_AllInvalidFallsBackToGenerated— generates new UUIDmake test,make lint,make docall passSummary by CodeRabbit
New Features
Improvements
Documentation
Tests