🔥 feat: Validate HTTP headers in RequestID middleware #3919
Conversation
Summary of ChangesHello @gaby, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the robustness of the "requestid" middleware by introducing explicit validation and sanitization for request IDs. The primary goal is to ensure that all request IDs conform to visible ASCII standards, thereby enhancing RFC-compliance for HTTP headers. This is achieved through new helper functions that check and, if necessary, regenerate IDs, along with updated documentation to guide users on this new behavior. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
gaby
added
the
📜 RFC Compliance
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3919 +/- ##
==========================================
- Coverage 91.66% 91.57% -0.09%
==========================================
Files 119 119
Lines 10096 10120 +24
==========================================
+ Hits 9254 9267 +13
- Misses 534 542 +8
- Partials 308 311 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
gaby
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds documentation and logic for sanitizing and validating request IDs. The changes ensure that request IDs conform to visible ASCII characters, with fallbacks to generate a new valid ID if needed.
My review focuses on the new implementation in middleware/requestid/requestid.go. I've found a couple of areas where the logic can be simplified and made more robust by removing redundant code and checks. The new documentation and tests look good and accurately reflect the intended changes.
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive documentation for request ID validation and sanitization helpers in the requestid middleware. The changes introduce validation logic to ensure request IDs contain only visible ASCII characters and implement proper fallback mechanisms when invalid IDs are encountered.
- Implements
isValidRequestIDandsanitizeRequestIDhelper functions with visible ASCII validation - Adds test coverage for invalid header values, invalid generated values, and ASCII validation
- Updates middleware documentation to explain the validation and regeneration behavior
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| middleware/requestid/requestid.go | Introduces request ID validation helpers (sanitizeRequestID and isValidRequestID) and integrates them into the middleware handler to ensure RFC-compliant headers |
| middleware/requestid/requestid_test.go | Adds tests for invalid request ID handling, ASCII validation, and obs-text rejection to cover the new sanitization logic |
| docs/middleware/requestid.md | Documents the visible ASCII validation requirement and regeneration behavior when invalid IDs are detected |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughRequestID middleware now validates incoming IDs (visible ASCII only) and sanitizes them. If invalid, it attempts up to three values from the configured generator, then up to three UUID attempts before leaving the value empty. Documentation and tests were updated to reflect this behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)**/*_test.go📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.go📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (2)📚 Learning: 2024-10-16T12:12:30.506ZApplied to files:
📚 Learning: 2025-10-16T07:19:52.418ZApplied to files:
🧬 Code graph analysis (1)middleware/requestid/requestid_test.go (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (2)
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.
Actionable comments posted: 0
♻️ Duplicate comments (2)
middleware/requestid/requestid.go (2)
48-75: Redundant nil checks and duplicate UUID attempts.Since
configDefault()ensurescfg.Generatoris nevernil(defaulting toutils.UUID), the nil check on line 54 is dead code, and the condition on line 65 is always true. When the default generator is used, this results in tryingutils.UUIDup to 6 times (3 + 3) redundantly.Consider simplifying:
func sanitizeRequestID(rid string, generator func() string) string { if isValidRequestID(rid) { return rid } - generatorFn := generator - if generatorFn == nil { - generatorFn = utils.UUID - } - for range 3 { - rid = generatorFn() + rid = generator() if isValidRequestID(rid) { return rid } } - if generator != nil { - for range 3 { - rid = utils.UUID() - if isValidRequestID(rid) { - return rid - } + // Fallback to UUID only if a custom generator was provided and failed + if generator != utils.UUID { + for range 3 { + rid = utils.UUID() + if isValidRequestID(rid) { + return rid + } } } return "" }Note: Comparing function pointers (
generator != utils.UUID) may not work reliably in Go. An alternative is to pass a flag indicating whether a custom generator was used, or accept the minor redundancy for simplicity.
28-31: Redundant UUID fallback after exhausted retries.If
sanitizeRequestIDreturns an empty string, it means all generation attempts (up to 3 from the generator + 3 fromutils.UUID()) have failed. Adding another unvalidatedutils.UUID()call here is both redundant and inconsistent since this final ID bypassesisValidRequestIDvalidation.Consider removing the fallback or at minimum validating the result:
rid := sanitizeRequestID(c.Get(cfg.Header), cfg.Generator) - if rid == "" { - rid = utils.UUID() - }If an empty request ID is unacceptable, logging a warning would be more appropriate than an unvalidated fallback.
🧹 Nitpick comments (2)
middleware/requestid/requestid_test.go (2)
67-71: Consider strengthening UUID fallback verification.The assertions verify the result is non-empty and lacks CRLF characters, but don't confirm the fallback produces a valid UUID. Adding a length check would better validate the UUID fallback behavior:
rid := resp.Header.Get(fiber.HeaderXRequestID) require.NotEmpty(t, rid) require.NotContains(t, rid, "\r") require.NotContains(t, rid, "\n") + require.Len(t, rid, 36, "Fallback should produce a UUID")
73-83: Consider adding explicit boundary rejection tests.The tests cover valid characters including boundaries (space
0x20and tilde0x7E), but explicit tests for rejection of adjacent invalid characters (0x1Fand0x7F) would strengthen confidence in the boundary conditions:func Test_isValidRequestID_RejectsBoundaries(t *testing.T) { t.Parallel() require.False(t, isValidRequestID("valid\x1f"), "0x1F should be rejected") require.False(t, isValidRequestID("valid\x7f"), "0x7F (DEL) should be rejected") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/middleware/requestid.md(1 hunks)middleware/requestid/requestid.go(3 hunks)middleware/requestid/requestid_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
When adding Go tests, always invoke
t.Parallel()at the start of each test and subtest to maximize concurrency
Files:
middleware/requestid/requestid_test.go
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Prefer
github.com/gofiber/utils/v2helpers (for example,utils.Trim) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code
Files:
middleware/requestid/requestid_test.gomiddleware/requestid/requestid.go
docs/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Review and update the contents of the
docsfolder if necessary when modifying code
Files:
docs/middleware/requestid.md
**/*.md
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run
make markdownto lint all Markdown files when modifying code
Files:
docs/middleware/requestid.md
🧠 Learnings (6)
📚 Learning: 2024-10-16T12:12:30.506Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3170
File: ctx_test.go:1721-1724
Timestamp: 2024-10-16T12:12:30.506Z
Learning: In the Go unit tests in `ctx_test.go`, it is acceptable to use invalid CIDR notation such as `"0.0.0.1/31junk"` for testing purposes.
Applied to files:
middleware/requestid/requestid_test.go
📚 Learning: 2025-10-16T07:19:52.418Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:19:52.418Z
Learning: In the Fiber codebase, the linter does not allow `require` assertions from within HTTP handlers (including net/http-style handlers). Use `t.Fatalf`, `t.Errorf`, or similar `testing.T` methods for error handling inside handler functions instead.
Applied to files:
middleware/requestid/requestid_test.go
📚 Learning: 2024-11-10T23:44:13.704Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-10T23:44:13.704Z
Learning: In the `middleware/adaptor/adaptor.go` file of the Fiber framework, when updating context handling, replacing `c.Context()` with `c.RequestCtx()` is appropriate to access the `fasthttp.RequestCtx`.
Applied to files:
middleware/requestid/requestid.go
📚 Learning: 2024-11-08T04:10:42.990Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3193
File: middleware/cache/cache_test.go:897-897
Timestamp: 2024-11-08T04:10:42.990Z
Learning: In the Fiber framework, `Context()` is being renamed to `RequestCtx()`, and `UserContext()` to `Context()` to improve clarity and align with Go's context conventions.
Applied to files:
middleware/requestid/requestid.go
📚 Learning: 2025-11-26T13:34:08.100Z
Learnt from: CR
Repo: gofiber/fiber PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T13:34:08.100Z
Learning: Applies to **/*.go : Prefer `github.com/gofiber/utils/v2` helpers (for example, `utils.Trim`) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code
Applied to files:
middleware/requestid/requestid.go
📚 Learning: 2025-09-28T17:39:26.644Z
Learnt from: arturmelanchyk
Repo: gofiber/fiber PR: 3768
File: client/request.go:162-166
Timestamp: 2025-09-28T17:39:26.644Z
Learning: In fasthttp-based code like gofiber, using utils.UnsafeString(key) instead of string(key) is acceptable when values are used immediately within the same call stack and not stored for later use, as it avoids allocations and aligns with fasthttp's zero-allocation design philosophy.
Applied to files:
middleware/requestid/requestid.go
🧬 Code graph analysis (1)
middleware/requestid/requestid_test.go (3)
middleware/requestid/requestid.go (1)
New(18-42)middleware/requestid/config.go (1)
Config(9-24)constants.go (1)
HeaderXRequestID(281-281)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Compare
- GitHub Check: lint
- GitHub Check: unit (1.25.x, macos-latest)
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: repeated
🔇 Additional comments (4)
docs/middleware/requestid.md (1)
42-46: LGTM!The documentation clearly explains the new validation and fallback behavior, including the visible ASCII requirement and the multi-attempt generation strategy.
middleware/requestid/requestid.go (2)
5-5: LGTM!Import aligns with coding guidelines to prefer
github.com/gofiber/utils/v2helpers.
77-92: LGTM!The validation logic correctly enforces visible ASCII characters (0x20–0x7E) with efficient byte-by-byte iteration.
middleware/requestid/requestid_test.go (1)
39-47: LGTM!Test correctly validates that an invalid header value triggers the generator fallback.
Add a test to ensure the request ID is a valid UUID.
Summary