Proposal fix for #4280 with no breaking changes #4289
Conversation
…ests are passing, Added possibility to dynamically control the cookie name and tokenKey using context variable. Rationale is that sometimes csrf token is overridden .. e.g. using SingleUseToken true , and having more then 1 browser windows/tab opened it may happen that all browser sessions use the same csrf token, but on the first post on any window the csrf will be accepted and changed, the other windows will have to be hard refreshed as their token got invalidated. This fix allows control of csrf token on a per window/tab session
WalkthroughThe CSRF middleware adds per-request dynamic cookie naming. A new ChangesCSRF Dynamic Cookie Name Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
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 |
![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 introduces a CookieNameFn field to the CSRF middleware configuration, enabling dynamic cookie names. The review feedback identifies a potential panic risk when invoking this function with a nil context during configuration validation and suggests refactoring variable initializations into explicit if/else blocks. Furthermore, the reviewer advises against using dynamic keys for context storage, as it breaks the TokenFromContext helper and logging; they recommend maintaining the use of the internal tokenKey and reverting the breaking change to the TokenFromContext signature to ensure backward compatibility.
| cookieName := cfg.CookieName | ||
| if cfg.CookieNameFn != nil { | ||
| cookieName = cfg.CookieNameFn(nil) | ||
| } |
There was a problem hiding this comment.
Following the general rules, variable initializations should be refactored into explicit if/else blocks to avoid overwriting. Additionally, calling CookieNameFn(nil) poses a risk of panicking if the user-provided function does not explicitly handle a nil context. While the example in the PR description handles it, this should be documented or handled more defensively to avoid crashes during middleware initialization.
var cookieName string
if cfg.CookieNameFn != nil {
// Note: CookieNameFn is called with nil context during config validation.
// Users must handle nil context in their implementation to avoid panics at startup.
cookieName = cfg.CookieNameFn(nil)
} else {
cookieName = cfg.CookieName
}References
- Prefer refactoring variable initializations into explicit if/else blocks to avoid using linter suppressions like wastedassign when a variable is conditionally assigned or immediately overwritten.
| var t any | ||
| if cfg.CookieNameFn == nil { | ||
| t = tokenKey | ||
| } else { | ||
| t = cookieName | ||
| } | ||
|
|
||
| // Store the token in the context | ||
| fiber.StoreInContext(c, tokenKey, token) | ||
| fiber.StoreInContext(c, t, token) |
There was a problem hiding this comment.
Storing the CSRF token in the context using a dynamic string key (cookieName) instead of the internal tokenKey breaks the standard TokenFromContext(c) helper and the built-in logger tag (csrf-token). Since the request context is already scoped to a single request, there is no need for a dynamic key here. Always using tokenKey ensures compatibility with existing utilities and logging while still supporting dynamic cookie names on the transport layer.
// Store the token in the context
fiber.StoreInContext(c, tokenKey, token)| func TokenFromContext(ctx any, cookieKey ...string) string { | ||
|
|
||
| // variadic function in order not to introduce breaking changes, if cookie name is not sent it uses tokenKey as default | ||
| var tK any | ||
| if len(cookieKey) == 1 { | ||
| tK = cookieKey[0] | ||
| if tK == "" { | ||
| tK = tokenKey | ||
| } | ||
| } else { | ||
| tK = tokenKey | ||
| } | ||
|
|
||
| if token, ok := fiber.ValueFromContext[string](ctx, tK); ok { |
There was a problem hiding this comment.
Changing the signature of TokenFromContext to be variadic is a breaking change for users who use this function as a value (e.g., passing it to another function). Furthermore, if the token is always stored under tokenKey in the middleware (as suggested), this complexity and the extra allocations from the variadic slice are unnecessary. Reverting this change maintains backward compatibility as promised in the PR title.
func TokenFromContext(ctx any) string {
if token, ok := fiber.ValueFromContext[string](ctx, tokenKey); ok {
return token
}
return ""
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@middleware/csrf/config.go`:
- Around line 191-206: The startup validation calls CookieNameFn with nil which
can panic and misses dynamic values; change the logic so that if
cfg.CookieNameFn != nil you skip the nil-call startup check (do not compute
cookieName from CookieNameFn at construction) and instead perform the
isInsecureCookieExtractor checks at request time after computing cookieName from
the real fiber.Ctx (i.e., in the middleware handler where cookieName is
resolved), and if CookieNameFn is nil keep the existing startup checks using
cfg.CookieName; ensure you still validate both the primary extractor via
isInsecureCookieExtractor(cfg.Extractor, cookieName) and each chained extractor
in cfg.Extractor.Chain at request time when CookieNameFn is used.
In `@middleware/csrf/csrf.go`:
- Around line 227-235: Always store the CSRF token under the canonical tokenKey
so TokenFromContext(c) and the "csrf-token" log tag continue to work: call
fiber.StoreInContext(c, tokenKey, token) unconditionally; if cfg.CookieNameFn is
non-nil also mirror the token under cookieName by calling
fiber.StoreInContext(c, cookieName, token) so keyed lookups that rely on the
cookie name still work; adjust the existing branch that currently stores only
under cookieName (inspect cfg.CookieNameFn, tokenKey, cookieName, token and
fiber.StoreInContext) to implement this unconditional + optional dual-store
behavior.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d49131a5-582a-4afa-af27-bfb73daca773
📒 Files selected for processing (2)
middleware/csrf/config.gomiddleware/csrf/csrf.go
| cookieName := cfg.CookieName | ||
| if cfg.CookieNameFn != nil { | ||
| cookieName = cfg.CookieNameFn(nil) | ||
| } | ||
|
|
||
| // Check primary extractor | ||
| if isInsecureCookieExtractor(cfg.Extractor, cfg.CookieName) { | ||
| panic("CSRF: Extractor reads from the same cookie '" + cfg.CookieName + | ||
| if isInsecureCookieExtractor(cfg.Extractor, cookieName) { | ||
| panic("CSRF: Extractor reads from the same cookie '" + cookieName + | ||
| "' used for token storage. This completely defeats CSRF protection.") | ||
| } | ||
|
|
||
| // Check chained extractors | ||
| for i, extractor := range cfg.Extractor.Chain { | ||
| if isInsecureCookieExtractor(extractor, cfg.CookieName) { | ||
| if isInsecureCookieExtractor(extractor, cookieName) { | ||
| panic(fmt.Sprintf("CSRF: Chained extractor #%d reads from the same cookie '%s' "+ | ||
| "used for token storage. This completely defeats CSRF protection.", i+1, cfg.CookieName)) | ||
| "used for token storage. This completely defeats CSRF protection.", i+1, cookieName)) |
There was a problem hiding this comment.
Don't invoke CookieNameFn with a nil context here.
This runs during middleware construction, so any normal CookieNameFn that reads from c will panic before the app starts. It also only validates one synthetic name, so a dynamic function can still evade the insecure-cookie extractor check for other request-time values. Please move this validation to the request path after cookieName is computed from a real fiber.Ctx, or skip startup validation when CookieNameFn is configured and enforce it later instead.
🤖 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/csrf/config.go` around lines 191 - 206, The startup validation
calls CookieNameFn with nil which can panic and misses dynamic values; change
the logic so that if cfg.CookieNameFn != nil you skip the nil-call startup check
(do not compute cookieName from CookieNameFn at construction) and instead
perform the isInsecureCookieExtractor checks at request time after computing
cookieName from the real fiber.Ctx (i.e., in the middleware handler where
cookieName is resolved), and if CookieNameFn is nil keep the existing startup
checks using cfg.CookieName; ensure you still validate both the primary
extractor via isInsecureCookieExtractor(cfg.Extractor, cookieName) and each
chained extractor in cfg.Extractor.Chain at request time when CookieNameFn is
used.
| var t any | ||
| if cfg.CookieNameFn == nil { | ||
| t = tokenKey | ||
| } else { | ||
| t = cookieName | ||
| } | ||
|
|
||
| // Store the token in the context | ||
| fiber.StoreInContext(c, tokenKey, token) | ||
| fiber.StoreInContext(c, t, token) |
There was a problem hiding this comment.
Keep storing the token under tokenKey as well.
When CookieNameFn is enabled, TokenFromContext(c) still defaults to tokenKey, so existing handlers/templates and the built-in "csrf-token" log tag start returning "". Store the token under tokenKey unconditionally, and optionally mirror it under cookieName if keyed lookup is still needed.
Suggested fix
- var t any
- if cfg.CookieNameFn == nil {
- t = tokenKey
- } else {
- t = cookieName
- }
-
- // Store the token in the context
- fiber.StoreInContext(c, t, token)
+ // Preserve existing lookups.
+ fiber.StoreInContext(c, tokenKey, token)
+ if cfg.CookieNameFn != nil {
+ fiber.StoreInContext(c, cookieName, token)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var t any | |
| if cfg.CookieNameFn == nil { | |
| t = tokenKey | |
| } else { | |
| t = cookieName | |
| } | |
| // Store the token in the context | |
| fiber.StoreInContext(c, tokenKey, token) | |
| fiber.StoreInContext(c, t, token) | |
| // Preserve existing lookups. | |
| fiber.StoreInContext(c, tokenKey, token) | |
| if cfg.CookieNameFn != nil { | |
| fiber.StoreInContext(c, cookieName, token) | |
| } |
🤖 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/csrf/csrf.go` around lines 227 - 235, Always store the CSRF token
under the canonical tokenKey so TokenFromContext(c) and the "csrf-token" log tag
continue to work: call fiber.StoreInContext(c, tokenKey, token) unconditionally;
if cfg.CookieNameFn is non-nil also mirror the token under cookieName by calling
fiber.StoreInContext(c, cookieName, token) so keyed lookups that rely on the
cookie name still work; adjust the existing branch that currently stores only
under cookieName (inspect cfg.CookieNameFn, tokenKey, cookieName, token and
fiber.StoreInContext) to implement this unconditional + optional dual-store
behavior.
|
This is a breaking change in two locations and is unnecessary. |
|
This PR shouldn't be merged. It takes a working, collision-safe design and actively breaks it. The core problemYour original issue was predicated on This patch replaces the safe, typed key with a string key in the critical path: var t any
if cfg.CookieNameFn == nil {
t = tokenKey
} else {
t = cookieName // string key — now collisions are possible
}When API concerns
Summary
Closing this. If you have a concrete use case for dynamic cookie names that cannot be solved by existing configuration, please open a proposal with real-world requirements rather than a patch for a non-existent collision. |
I think it is obvious that when the cookie name is handled through an external function then this becomes the caller's responsabilitty to avoid collisions
True, i mainly tried not to introduce breaking changes ..but i welcome other ideas if any.. i didn't put to much though in this :)
I will open a new proposal then ( i commented in the bug with a real use case, where conflicts happen because the csrf is "static") |
4 participants
Proposal fix for #4280 with no breaking changes
Added possibility to dynamically control the cookie name and tokenKey using context variable. Rationale is that sometimes csrf token is overridden .. e.g. using SingleUseToken true , and having more then 1 browser windows/tab opened it may happen that all browser sessions use the same csrf token, but on the first post on any window the csrf will be accepted and changed, the other windows will have to be hard refreshed as their token got invalidated. This fix allows control of csrf token on a per window/tab session
Description
Added possibility to dynamically control the cookie name and tokenKey using context variable. Rationale is that sometimes csrf token is overridden ..
Fixes # (issue)
e.g. using SingleUseToken true , and having more then 1 browser windows/tab opened it may happen that all browser sessions use the same csrf token, but on the first post on any window the csrf will be accepted and changed, the other windows will have to be hard refreshed as their token got invalidated. This fix allows control of csrf token on a per window/tab session
Changes introduced
Added CookieNameFn .. if this is used then default CookieName and tokenKey will use this function instead of the default. If not used then CookieName will behave similar.
Setting up configuration :
`
csrfConfig := csrf.Config{
Session: store,
Extractor: extractors.FromHeader("X-Csrf-Token"), // In this example, we will be using a hidden input field to store the CSRF token
CookieNameFN: GetCSRFByTabId, // set our function
.................
SingleUseToken: true,
}
return csrfConfig
// Example function that gets the tab id sent from the frontend)
func GetCSRFByTabId(ctx fiber.Ctx) string {
}
`
Type of change
Please delete options that are not relevant.
Checklist
Before you submit your pull request, please make sure you meet these requirements:
/docs/directory for Fiber's documentation.Commit formatting
Please use emojis in commit messages for an easy way to identify the purpose or intention of a commit. Check out the emoji cheatsheet here: CONTRIBUTING.md