feat: Migrate from UUIDv4 to SecureToken for key generation #3946
Conversation
- Replace UUIDv4 with SecureToken in session, csrf, and requestid middlewares - Optimize requestid middleware to use single SecureToken fallback instead of retry loop - Update test files to use UUIDv4 for predictable test IDs - Update middleware documentation to reflect SecureToken defaults - Update go.mod to use latest utils version with SecureToken support
|
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. WalkthroughRefactors delimiter parsing to use strings.Cut across request handling and middleware, replaces several middleware default ID/token generators from UUID variants to utils.SecureToken, simplifies binder decoder fields to plain function types (removing utils aliases), and updates tests and docs accordingly. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
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)
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 |
Summary of ChangesHello @sixcolors, 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 enhances the security and privacy of key generation within several core middlewares by transitioning from UUID-based identifiers to cryptographically secure tokens. This change impacts Session, CSRF, and RequestID middlewares, making them more robust against potential information leakage. Alongside this, the RequestID middleware's internal logic for ID generation and fallback has been optimized, and various string manipulation operations across the codebase have been refactored for improved readability and efficiency. The update also includes necessary documentation adjustments and a dependency bump to support these new security features. 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
|
![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 is a great initiative to enhance security by migrating from UUIDv4 to the more secure SecureToken for key generation in various middlewares. The changes are well-implemented across the session, CSRF, and request ID middlewares, including necessary documentation and test updates. The code also benefits from several refactorings that improve readability and adopt modern Go idioms, such as using strings.Cut. My main feedback is a minor suggestion to improve the robustness of the fallback logic in the request ID middleware to ensure it aligns perfectly with the documented behavior.
middleware/requestid/requestid.go
Outdated
| if generator != nil { | ||
| for range 3 { | ||
| rid = utils.UUID() | ||
| if isValidRequestID(rid) { | ||
| return rid | ||
| } | ||
| } | ||
| rid = utils.SecureToken() | ||
| return rid | ||
| } |
There was a problem hiding this comment.
The result of utils.SecureToken() is returned directly without validation. While it's highly likely to be a valid request ID, for robustness, it would be better to validate it with isValidRequestID before returning. This would also align with the function's documentation which states it falls back to an empty string if a valid ID cannot be produced. This change ensures that if utils.SecureToken() were to produce an invalid ID, the function would correctly fall back to returning an empty string.
| if generator != nil { | |
| for range 3 { | |
| rid = utils.UUID() | |
| if isValidRequestID(rid) { | |
| return rid | |
| } | |
| } | |
| rid = utils.SecureToken() | |
| return rid | |
| } | |
| if generator != nil { | |
| rid = utils.SecureToken() | |
| if isValidRequestID(rid) { | |
| return rid | |
| } | |
| } |
There was a problem hiding this comment.
@gemini-code-assist The utils.SecureToken() function doesn’t require explicit validation because it panics on any RNG failure, just like the underlying crypto/rand.Read function it relies on. Furthermore, it uses base64.RawURLEncoding.EncodeToString, which always produces valid ASCII characters (0x20-0x7E). This makes additional checks redundant and adds unnecessary overhead. The middleware now relies on SecureToken's internal guarantees for robustness, avoiding 'generally' applied suggestions that don't account for the specific implementation details.
There was a problem hiding this comment.
Thank you for the detailed explanation, @sixcolors. I understand now that utils.SecureToken() is designed to always produce a valid, visible ASCII string due to its use of base64.RawURLEncoding.EncodeToString and its panic-on-failure behavior for RNG. Given these strong guarantees, the isValidRequestID check for the output of utils.SecureToken() is indeed redundant and would add unnecessary overhead. I withdraw my previous suggestion.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3946 +/- ##
==========================================
+ Coverage 91.62% 91.67% +0.05%
==========================================
Files 119 119
Lines 10190 10180 -10
==========================================
- Hits 9337 9333 -4
+ Misses 540 536 -4
+ Partials 313 311 -2
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:
|
There was a problem hiding this comment.
Pull request overview
This PR migrates the Fiber framework from UUID-based token generation to SecureToken for improved security and privacy in Session, CSRF, and RequestID middlewares. The change provides cryptographically secure random tokens instead of potentially predictable UUID formats, reducing information leakage about request patterns.
Key changes include:
- Migration of default key generators from
utils.UUIDv4toutils.SecureTokenacross Session, CSRF, and RequestID middlewares - Refactoring of string operations to use
strings.Cutfor cleaner code - Updated tests to accommodate the new 43-character base64-encoded token format instead of 36-character UUID format
- Comprehensive documentation updates reflecting the new defaults and behavior
Reviewed changes
Copilot reviewed 21 out of 22 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| middleware/session/config.go | Changed default KeyGenerator from UUIDv4 to SecureToken |
| middleware/csrf/config.go | Changed default KeyGenerator from UUIDv4 to SecureToken |
| middleware/requestid/config.go | Changed default Generator from UUID to SecureToken and updated documentation |
| middleware/requestid/requestid.go | Simplified fallback logic to use SecureToken directly instead of multiple UUID retry loops |
| middleware/csrf/csrf.go | Refactored hardcoded "https" string to use schemeHTTPS constant |
| middleware/session/session_test.go | Updated test assertions for new 43-character token length and base64url regex pattern |
| middleware/requestid/requestid_test.go | Updated test assertions for new 43-character token length and added new test cases for fallback behavior |
| middleware/cache/manager_test.go | Changed test key generation from UUID to UUIDv4 for predictable test IDs |
| internal/memory/memory_test.go | Changed benchmark key generation from UUID to UUIDv4 |
| req.go | Refactored string parsing to use strings.Cut instead of strings.Index |
| middleware/basicauth/basicauth.go | Refactored credential parsing to use strings.Cut |
| middleware/cors/utils.go | Refactored scheme matching to use strings.Cut |
| client/core.go | Refactored port checking to use strings.Contains |
| helpers.go | Refactored wildcard checking to use strings.Contains |
| binder/msgpack.go | Simplified type definition by removing utils dependency |
| binder/cbor.go | Simplified type definition by removing utils dependency |
| docs/whats_new.md | Added documentation for KeyGenerator changes in Session and CSRF |
| docs/middleware/session.md | Updated default KeyGenerator documentation |
| docs/middleware/requestid.md | Updated default Generator documentation and fallback behavior description |
| docs/middleware/csrf.md | Updated default KeyGenerator documentation |
| go.mod | Updated utils dependency to v2.0.0-rc.5 |
| go.sum | Updated checksums for new utils version |
![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
🧹 Nitpick comments (2)
internal/memory/memory_test.go (1)
59-83: Consider parallelizing the benchmark.The benchmark creates isolated store instances with no shared state, making it safe to parallelize with
b.Parallel().🔎 Proposed enhancement for benchmark parallelization
func Benchmark_Memory(b *testing.B) { + b.Parallel() keyLength := 1000middleware/requestid/requestid_test.go (1)
194-208: Consider moving the assertion outside the handler.Based on learnings, the linter may not allow
requireassertions from within HTTP handlers. Consider following the pattern used inTest_RequestID_FromContext(lines 184-187), where the value is captured in a variable and asserted outside the handler.🔎 Suggested refactor
func Test_RequestID_FromContext_Empty(t *testing.T) { t.Parallel() app := fiber.New() // No middleware + + var ctxVal string app.Use(func(c fiber.Ctx) error { - ctxVal := FromContext(c) - require.Empty(t, ctxVal) + ctxVal = FromContext(c) return c.SendStatus(fiber.StatusOK) }) _, err := app.Test(httptest.NewRequest(fiber.MethodGet, "/", http.NoBody)) require.NoError(t, err) + require.Empty(t, ctxVal) }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (20)
binder/cbor.go(1 hunks)binder/msgpack.go(1 hunks)client/core.go(1 hunks)docs/middleware/csrf.md(1 hunks)docs/middleware/requestid.md(2 hunks)docs/middleware/session.md(2 hunks)docs/whats_new.md(2 hunks)helpers.go(1 hunks)internal/memory/memory_test.go(1 hunks)middleware/basicauth/basicauth.go(1 hunks)middleware/cache/manager_test.go(1 hunks)middleware/cors/utils.go(1 hunks)middleware/csrf/config.go(2 hunks)middleware/csrf/csrf.go(1 hunks)middleware/requestid/config.go(2 hunks)middleware/requestid/requestid.go(1 hunks)middleware/requestid/requestid_test.go(3 hunks)middleware/session/config.go(2 hunks)middleware/session/session_test.go(2 hunks)req.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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/basicauth/basicauth.gomiddleware/session/session_test.gomiddleware/csrf/csrf.gointernal/memory/memory_test.goreq.gomiddleware/requestid/requestid.gomiddleware/requestid/config.gobinder/cbor.gomiddleware/csrf/config.gomiddleware/cors/utils.gobinder/msgpack.goclient/core.gohelpers.gomiddleware/cache/manager_test.gomiddleware/requestid/requestid_test.gomiddleware/session/config.go
**/*_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/session/session_test.gointernal/memory/memory_test.gomiddleware/cache/manager_test.gomiddleware/requestid/requestid_test.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.mddocs/middleware/session.mddocs/whats_new.mddocs/middleware/csrf.md
**/*.md
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run
make markdownto lint all Markdown files when modifying code
Files:
docs/middleware/requestid.mddocs/middleware/session.mddocs/whats_new.mddocs/middleware/csrf.md
🧠 Learnings (21)
📓 Common learnings
Learnt from: CR
Repo: gofiber/fiber PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T15:07:23.885Z
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
📚 Learning: 2024-06-30T00:38:06.580Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3051
File: middleware/session/session.go:215-216
Timestamp: 2024-06-30T00:38:06.580Z
Learning: Parallel tests for `Session.Save` already exist in the `middleware/session/session_test.go` file, specifically in the `Test_Session_Save` and `Test_Session_Save_Expiration` functions.
Applied to files:
middleware/session/session_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3056
File: middleware/encryptcookie/utils.go:22-25
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The `encryptcookie_test.go` file contains unit tests that validate key lengths for both `EncryptCookie` and `DecryptCookie` functions, ensuring that invalid key lengths raise appropriate errors.
Applied to files:
middleware/session/session_test.go
📚 Learning: 2024-09-25T17:09:03.756Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/middleware_test.go:400-407
Timestamp: 2024-09-25T17:09:03.756Z
Learning: In the `Test_Session_Next` function in `middleware/session/middleware_test.go`, the variable `doNext` is properly synchronized with the `muNext` mutex for both read and write access.
Applied to files:
middleware/session/session_test.go
📚 Learning: 2024-09-25T17:05:06.991Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/middleware_test.go:190-191
Timestamp: 2024-09-25T17:05:06.991Z
Learning: When testing session `IdleTimeout` expiration, it's acceptable to use `time.Sleep` to simulate the passage of time in tests.
Applied to files:
middleware/session/session_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/csrf/csrf_test.go:164-165
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In the `Test_CSRF_WithSession_Middleware` function, calling `session.NewWithStore()` without arguments is acceptable, as the default configuration is sufficient.
Applied to files:
middleware/session/session_test.godocs/middleware/session.md
📚 Learning: 2024-07-01T03:44:03.672Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3056
File: middleware/encryptcookie/utils.go:20-23
Timestamp: 2024-07-01T03:44:03.672Z
Learning: Unit tests for key length enforcement in both `EncryptCookie` and `DecryptCookie` functions have been added to ensure robust validation and prevent potential runtime errors.
Applied to files:
middleware/session/session_test.gomiddleware/requestid/requestid_test.go
📚 Learning: 2024-10-02T23:02:12.306Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/session.go:46-61
Timestamp: 2024-10-02T23:02:12.306Z
Learning: In this codebase, the `sessionPool` only contains `Session` instances, so type assertions without additional checks are acceptable.
Applied to files:
middleware/session/session_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In the Fiber framework tests, using `ctx.Response.Header.Cookie` may not be suitable for parsing cookies from the response header, as it requires a `*Cookie` and fills it rather than returning a string value; thus, manual parsing of the `Set-Cookie` header may be necessary.
Applied to files:
middleware/session/session_test.gomiddleware/requestid/requestid_test.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
📚 Learning: 2025-07-27T17:28:53.403Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3625
File: middleware/session/config.go:57-58
Timestamp: 2025-07-27T17:28:53.403Z
Learning: In the session middleware `Config` struct, the `Extractor` field uses function closures (like `FromCookie(key)`), making it impossible to introspect extractor parameters at runtime for validation purposes without complex reflection techniques.
Applied to files:
docs/middleware/session.mdmiddleware/csrf/config.godocs/middleware/csrf.mdmiddleware/session/config.go
📚 Learning: 2024-09-25T16:17:00.969Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/config.go:16-26
Timestamp: 2024-09-25T16:17:00.969Z
Learning: In the session middleware `Config` struct, `Store` is backed by `fiber.Storage`; they are different entities serving distinct purposes in session management.
Applied to files:
docs/middleware/session.md
📚 Learning: 2024-09-25T16:15:39.392Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/csrf/session_manager.go:30-43
Timestamp: 2024-09-25T16:15:39.392Z
Learning: In the session middleware, `session.FromContext(c)` returns `*session.Middleware`, whereas `m.session.Get(c)` returns `*session.Store`, so they are not directly interchangeable.
Applied to files:
docs/middleware/session.md
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/config.go:26-26
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In the session middleware, the `newStore`, `New`, and `NewWithStore` functions ensure that a `Store` is present even if it is not initialized in `ConfigDefault`.
Applied to files:
docs/middleware/session.md
📚 Learning: 2025-09-14T00:10:40.547Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 0
File: :0-0
Timestamp: 2025-09-14T00:10:40.547Z
Learning: The `session.Save()` method in the `middleware/session` package only persists session data to storage and updates the client cookie. It does NOT return the Session back to `sync.Pool`. The session remains valid and usable after calling Save().
Applied to files:
docs/middleware/session.md
📚 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-09-25T16:18:34.719Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/config.go:122-122
Timestamp: 2024-09-25T16:18:34.719Z
Learning: In `DefaultErrorHandler(c *fiber.Ctx, err error)`, since `c` is a pointer to an interface, we need to dereference `*c` when calling interface methods like `SendStatus`.
Applied to files:
middleware/requestid/requestid_test.go
📚 Learning: 2024-12-13T08:14:22.851Z
Learnt from: efectn
Repo: gofiber/fiber PR: 3162
File: hooks_test.go:228-228
Timestamp: 2024-12-13T08:14:22.851Z
Learning: In Go test files, prefer using the `require` methods from the `testify` package for assertions instead of manual comparisons and calls to `t.Fatal` or `t.Fatalf`.
Applied to files:
middleware/requestid/requestid_test.go
📚 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: 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_test.go
📚 Learning: 2024-11-29T12:37:27.581Z
Learnt from: efectn
Repo: gofiber/fiber PR: 3162
File: app_test.go:893-895
Timestamp: 2024-11-29T12:37:27.581Z
Learning: In the `Test_App_ShutdownWithContext` function in `app_test.go`, the `clientDone` channel is used to synchronize the client's request completion before proceeding, eliminating the need for additional `time.Sleep` calls.
Applied to files:
middleware/requestid/requestid_test.go
⏰ 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)
- GitHub Check: Compare
- GitHub Check: repeated
- GitHub Check: unit (1.25.x, windows-latest)
🔇 Additional comments (30)
client/core.go (1)
35-45: LGTM!The refactoring from an index-based check to
strings.Containsis idiomatic Go and more readable. The function logic is correct and handles all edge cases properly, including IPv6 addresses withnet.JoinHostPort.internal/memory/memory_test.go (2)
12-13: LGTM!Test function properly uses
t.Parallel()for concurrent execution.
62-64: Appropriate use ofutils.UUIDv4()for predictable test keys.The migration from
utils.UUID()toutils.UUIDv4()aligns with the PR objective of using deterministic values in tests while production code switches toSecureToken. This properly isolates test behavior from the new token generation strategy.middleware/cache/manager_test.go (1)
18-18: LGTM! Test updates align with PR strategy.The switch from
utils.UUID()toutils.UUIDv4()for test ID generation is appropriate. Using explicit UUIDv4 calls in tests ensures predictable, deterministic identifiers while production code adopts SecureToken for enhanced security.Also applies to: 24-24
middleware/cors/utils.go (1)
10-12: LGTM! Clean refactor to strings.Cut.The migration from Index-based parsing to
strings.Cutimproves code clarity and safety by explicitly checking whether both domain and pattern contain a scheme delimiter.helpers.go (1)
211-211: LGTM! Acceptable refactor.The change from
strings.IndexByte(spec, '*') != -1tostrings.Contains(spec, "*")is more idiomatic and equally correct for detecting wildcard characters.middleware/basicauth/basicauth.go (1)
94-101: LGTM! Safer credential parsing with strings.Cut.The migration to
strings.Cutprovides cleaner semantics for splitting credentials on the colon delimiter, with explicit validation via theokflag.binder/msgpack.go (1)
5-5: LGTM! Decoupled from utils package types.The change from
utils.MsgPackUnmarshalto a direct function signaturefunc([]byte, any) errorremoves the dependency on utils package types while maintaining identical behavior.binder/cbor.go (1)
5-5: LGTM! Decoupled from utils package types.The change from
utils.CBORUnmarshalto a direct function signaturefunc([]byte, any) errorremoves the dependency on utils package types while maintaining identical behavior. This aligns with the parallel change inbinder/msgpack.go.req.go (3)
324-327: LGTM! Cleaner header parsing with strings.Cut.The migration to
strings.Cutfor parsing theX-Forwarded-Hostheader provides cleaner semantics and explicit validation of the comma delimiter.
648-653: LGTM! Consistent header parsing pattern.The migration to
strings.Cutfor parsing forwarded protocol headers maintains consistency with theHost()method refactor and provides safer parsing.
762-770: LGTM! Enhanced Range parsing with better validation.The migration to
strings.Cutimproves the Range header parsing with:
- Cleaner extraction of type and ranges via
before/after- Additional validation to reject malformed ranges containing multiple "=" delimiters
This makes the parsing more robust against malformed input.
middleware/session/config.go (2)
35-36: LGTM! Documentation accurately reflects the new default.The comment correctly documents the updated default KeyGenerator.
95-95: Useutils.SecureTokenfor session key generation.This aligns with the coding guidelines to prefer
github.com/gofiber/utils/v2helpers. The change upgrades from UUID format (36 chars) to cryptographically secure base64-encoded tokens (43 chars), improving security. However, document the breaking change: existing sessions with UUID-format IDs will not be recognized after upgrade, and systems expecting UUID format may require migration.docs/middleware/requestid.md (2)
44-46: LGTM! Documentation accurately reflects the new fallback behavior.The updated documentation clearly describes the three-attempt mechanism with SecureToken as the final fallback, which aligns with the code changes in the RequestID middleware.
64-75: LGTM! Configuration documentation correctly updated.The table and default config example accurately reflect the new
utils.SecureTokendefault generator, with appropriate security-focused comments.middleware/csrf/config.go (1)
36-37: LGTM! CSRF token generator updated consistently.The change from
utils.UUIDv4toutils.SecureTokenis consistent with the broader PR objective to adopt cryptographically secure tokens across all middlewares.Note: This is a breaking change. CSRF tokens will now be base64-encoded (43 characters) instead of UUID format (36 characters). Systems expecting UUID format tokens may need adjustment.
Also applies to: 136-136
docs/middleware/csrf.md (1)
409-409: LGTM! Configuration documentation updated correctly.The table accurately reflects the new default KeyGenerator value.
middleware/requestid/config.go (1)
17-18: LGTM! RequestID generator updated to SecureToken.The change from
utils.UUIDtoutils.SecureTokenaligns with the PR's security objectives. The comment on line 27 appropriately highlights the privacy and security benefits.Note: Request IDs will now be 43-character base64-encoded tokens instead of 36-character UUIDs. Systems parsing or storing request IDs may need to accommodate the new format.
Also applies to: 27-32
docs/whats_new.md (2)
1566-1567: LGTM! Breaking change clearly documented.The Session middleware section appropriately documents the change from
utils.UUIDv4toutils.SecureTokenas a key change, noting that it produces base64-encoded tokens instead of UUID format.
2788-2789: LGTM! Migration guide includes the breaking change.The CSRF migration section properly documents the default KeyGenerator change, alerting users to the new token format. This helps users understand the impact during migration.
docs/middleware/session.md (1)
457-457: LGTM! Session documentation updated consistently.Both the production configuration example and the Config Properties table correctly reflect
utils.SecureTokenas the new default KeyGenerator. Documentation is accurate and complete.Also applies to: 682-682
middleware/csrf/csrf.go (1)
141-141: Use constant instead of string literal for HTTPS scheme comparison.Replacing the hardcoded
"https"string with theschemeHTTPSconstant improves code maintainability and prevents typos. This follows Go conventions and maintains consistency with the rest of the Fiber codebase, where the constant is already defined and widely used.middleware/session/session_test.go (2)
100-100: LGTM! Session ID length updated for SecureToken.The expected length correctly reflects the new default SecureToken generator (43 characters vs. 36 for UUID).
935-935: LGTM! Cookie regex correctly matches SecureToken format.The regex pattern
[A-Za-z0-9\-_]{43}properly validates base64url-encoded SecureToken output (43 characters with URL-safe charset).middleware/requestid/requestid.go (1)
42-56: LGTM! Fallback logic correctly implements SecureToken as final safeguard.The implementation properly tries the configured generator up to three times before falling back to
utils.SecureToken(), which guarantees a valid ID (base64url-encoded). Based on past review discussions,SecureToken()doesn't require validation as it always produces valid ASCII output or panics on RNG failure.middleware/requestid/requestid_test.go (4)
28-28: LGTM! Request ID length updated for SecureToken.The expected length correctly reflects the new default SecureToken generator (43 characters).
71-71: LGTM! Fallback length expectation is correct.The comment and assertion correctly verify that the fallback produces a 43-character SecureToken.
74-95: LGTM! Excellent test coverage for always-invalid generator.This test properly verifies the fallback behavior when the generator consistently produces invalid output (null byte), confirming the system falls back to SecureToken after 3 attempts.
97-117: LGTM! Good test coverage for custom generator.This test properly verifies that a custom generator's valid output is used directly without triggering the fallback mechanism.
gaby
changed the title
…er/fiber into feature/adopt-secure-token
|
@gaby mergeable ? |
This PR migrates Fiber middlewares from using UUID or UUIDv4 to SecureToken for key generation, providing better security and privacy.
Changes
Code Changes
Documentation Updates
Dependencies
Security Benefits
Breaking Changes
Testing
Note
Waiting on utils release for SecureToken availability in production.
Related
gofiber/utils#175
gofiber/utils#171
gofiber/utils#170
GHSA-m98w-cqp3-qcqr
Fixes #3959