refactor: update CSRF middleware to use shared extractors #3746
Conversation
- Replaced custom extractor implementations with the new shared extractors github.com/gofiber/fiber/v3/extractors. - Updated Config struct to use extractors for CSRF token extraction. - Modified tests to align with the new extractor methods and error handling. - Removed obsolete extractors.go and extractors_test.go files as their functionality is now encapsulated in the extractors package. - Enhanced documentation for available extractors and their security implications.
|
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. WalkthroughMigrates CSRF middleware to use the shared Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant CSRF as CSRF Middleware
participant Ext as extractors.Extractor
participant Handler as Next Handler
Client->>CSRF: HTTP Request
CSRF->>Ext: Extract(token)
alt token found
CSRF->>Handler: Call next
Handler-->>CSRF: Response
CSRF-->>Client: Response
else token missing (extractors.ErrNotFound)
CSRF-->>Client: 403 / CSRF error (ErrTokenNotFound)
else extraction error
CSRF-->>Client: handler(error)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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.
Summary of Changes
Hello @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 refactors the CSRF middleware to leverage a newly introduced shared extractors package. This change aims to consolidate and standardize token extraction mechanisms across the Fiber framework, promoting code reuse and maintainability. By migrating to shared extractors, the CSRF middleware benefits from a more consistent and robust approach to handling token retrieval, while also simplifying its own codebase and improving documentation clarity.
Highlights
- Shared Extractors Integration: The CSRF middleware has been refactored to utilize a new, shared
extractorspackage (github.com/gofiber/fiber/v3/extractors) for all token extraction logic, centralizing and standardizing this functionality across the framework. - Configuration Update: The
Configstruct within the CSRF middleware has been updated to use the new sharedextractors.Extractortype, ensuring consistency with the new modular approach. - Codebase Simplification: Obsolete custom extractor implementations and their corresponding test files (
extractors.goandextractors_test.go) have been removed, reducing redundancy and simplifying the middleware's internal structure. - Documentation Enhancement: The CSRF middleware documentation (
docs/middleware/csrf.md) has been significantly updated to reflect the use of the shared extractors, providing clearer guidance on available extractors, security considerations, and usage patterns. - Test Alignment: Existing tests for the CSRF middleware have been modified to align with the new shared extractor methods and updated error handling, ensuring continued correctness and reliability.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Pull Request Overview
This pull request refactors the CSRF middleware to use the shared extractors package from github.com/gofiber/fiber/v3/extractors instead of custom implementations. The changes align the middleware with the new extractors architecture while maintaining the same security protections.
Key Changes
- Migrated from custom
csrf.Extractortoextractors.Extractor - Updated all extractor function calls to use the extractors package (
FromHeader,FromForm, etc.) - Replaced custom error types with standardized
extractors.ErrNotFound
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| middleware/csrf/storage_manager_msgp.go | Updated generated msgp code to properly handle return values |
| middleware/csrf/csrf_test.go | Updated tests to use extractors package and new error types |
| middleware/csrf/config_test.go | Updated configuration tests with extractors import and type changes |
| middleware/csrf/config.go | Updated config to use extractors package and updated documentation |
| docs/middleware/csrf.md | Updated documentation to reference extractors package and new usage patterns |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3746 +/- ##
==========================================
- Coverage 92.03% 92.03% -0.01%
==========================================
Files 115 114 -1
Lines 11632 11715 +83
==========================================
+ Hits 10706 10782 +76
- Misses 671 676 +5
- Partials 255 257 +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.
Code Review
This pull request effectively refactors the CSRF middleware to use the new shared extractors package, which is a great move for code reuse and consistency across the Fiber framework. The changes are well-executed, including the removal of the local extractor implementation and updating all tests to align with the new shared methods. The documentation has also been largely updated to reflect these changes.
I've found a couple of minor inconsistencies in the documentation's code examples where references to old csrf package types and errors remain. I've left specific comments on how to fix them.
Additionally, the changes in storage_manager_msgp.go to use explicit returns instead of naked returns are a nice improvement for code clarity.
Overall, this is a solid refactoring. Once the documentation is polished, it will be ready to merge.
![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 (10)
middleware/csrf/config.go (2)
79-90: Docs comment now points to shared extractors: double‑check linkThe inline link to the Extractors Guide is great; verify the path is correct in the published docs site to avoid 404s.
194-197: Also warn for query/param extractors inside ChainYou warn only on the primary extractor. Consider warning for any chained extractors that read from URL locations as well.
Apply this diff:
func validateExtractorSecurity(cfg Config) { @@ // Additional security warnings (non-fatal) - if cfg.Extractor.Source == extractors.SourceQuery || cfg.Extractor.Source == extractors.SourceParam { - log.Warnf("[CSRF WARNING] Using %v extractor - URLs may be logged", cfg.Extractor.Source) - } + if cfg.Extractor.Source == extractors.SourceQuery || cfg.Extractor.Source == extractors.SourceParam { + log.Warnf("[CSRF WARNING] Using %v extractor - URLs may be logged", cfg.Extractor.Source) + } + for i, ex := range cfg.Extractor.Chain { + if ex.Source == extractors.SourceQuery || ex.Source == extractors.SourceParam { + log.Warnf("[CSRF WARNING] Using %v extractor in chain position %d - URLs may be logged", ex.Source, i+1) + } + } }middleware/csrf/config_test.go (2)
158-187: Chain metadata assertions may be brittle vs upstreamAsserting Chain().Source/Key behavior couples tests to implementation details of extractors. Consider asserting only that Chain[0] and Chain[1] metadata match inputs and that Extract tries in order, not the derived Source/Key of the chain wrapper.
Would you like me to rework these to focus on behavior (extraction order) rather than metadata fields to reduce flakiness if extractors evolves?
196-207: Prefer robust cookie parsing in testsAvoid manual Set‑Cookie parsing; use fasthttp.Cookie to reduce brittleness.
Apply this diff:
- token := string(ctx.Response.Header.Peek(fiber.HeaderSetCookie)) - token = strings.Split(strings.Split(token, ";")[0], "=")[1] + ck := fasthttp.AcquireCookie() + ck.SetKey(ConfigDefault.CookieName) + require.True(t, ctx.Response.Header.Cookie(ck)) + token := string(ck.Value()) + fasthttp.ReleaseCookie(ck)Also applies to: 219-239
docs/middleware/csrf.md (6)
229-242: Clarify cookie warning textThe last sentence is ambiguous; it should warn about panic only when names match.
Apply this diff:
-If you do this, set the extractor’s `Source` to `SourceCookie` and allow the middleware to check that the cookie name is different from your CSRF cookie. It will panic if this is the case. +If you do this, set the extractor’s `Source` to `SourceCookie` and ensure the cookie name differs from your CSRF cookie. The middleware will panic if the extractor’s cookie name matches the configured CSRF cookie name.
270-284: Use extractors.Extractor in examples (not csrf.Extractor)Update types/constants to the shared package.
Apply this diff:
-badExtractor := csrf.Extractor{ +badExtractor := extractors.Extractor{ @@ - Source: csrf.SourceCustom, + Source: extractors.SourceCustom,
295-313: BearerTokenExtractor: fix Source constant packageKeep ErrTokenNotFound in csrf, but Source* now lives in extractors.
Apply this diff:
- Source: csrf.SourceCustom, + Source: extractors.SourceCustom,
327-344: JSONBodyExtractor: fix Source constant packageSame package swap as above.
Apply this diff:
- Source: csrf.SourceCustom, + Source: extractors.SourceCustom,
460-461: Config table default should reference shared constructorUse the fully qualified default to avoid confusion.
Apply this diff:
-| Extractor | `extractors.Extractor` | Token extraction method with metadata | `FromHeader("X-Csrf-Token")` | +| Extractor | `extractors.Extractor` | Token extraction method with metadata | `extractors.FromHeader("X-Csrf-Token")` |
487-496: Remove/relocate Source constants from CSRF docs*Source* now belong to the extractors package; duplicating here can mislead users.
Apply this diff to drop the Source constants block:
const ( HeaderName = "X-Csrf-Token" ) - - -// Source types for extractor metadata -const ( - SourceHeader Source = iota // 0 - Most secure - SourceForm // 1 - Secure - SourceQuery // 2 - Less secure - SourceParam // 3 - Less secure - SourceCookie // 4 - Not recommended for CSRF, no built-in extractor for this source - SourceCustom // 5 - Security depends on implementation -)Optionally add a note referencing
extractors.Sourceif needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
docs/middleware/csrf.md(16 hunks)middleware/csrf/config.go(4 hunks)middleware/csrf/config_test.go(12 hunks)middleware/csrf/csrf_test.go(19 hunks)middleware/csrf/extractors.go(0 hunks)middleware/csrf/extractors_test.go(0 hunks)middleware/csrf/storage_manager_msgp.go(8 hunks)
💤 Files with no reviewable changes (2)
- middleware/csrf/extractors_test.go
- middleware/csrf/extractors.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Format Go code using gofumpt (enforced viamake format)
Ensure code passes golangci-lint checks (enforced viamake lint)
Optimize struct field alignment using betteralign (enforced viamake betteralign)
Modernize Go code using gopls modernize (enforced viamake modernize)
Files:
middleware/csrf/config_test.gomiddleware/csrf/csrf_test.gomiddleware/csrf/config.gomiddleware/csrf/storage_manager_msgp.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/csrf.md
🧠 Learnings (7)
📓 Common learnings
Learnt from: sixcolors
PR: gofiber/fiber#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.
📚 Learning: 2024-10-02T23:03:31.727Z
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-02T23:03:31.727Z
Learning: Unit tests in this project use testify require.
Applied to files:
middleware/csrf/config_test.go
📚 Learning: 2024-07-26T21:00:12.902Z
Learnt from: sixcolors
PR: gofiber/fiber#2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-07-26T21:00:12.902Z
Learning: The project uses the testify/assert package for assertions in unit tests.
Applied to files:
middleware/csrf/config_test.go
📚 Learning: 2024-12-13T08:14:22.851Z
Learnt from: efectn
PR: gofiber/fiber#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/csrf/config_test.go
📚 Learning: 2025-07-27T17:28:53.403Z
Learnt from: sixcolors
PR: gofiber/fiber#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:
middleware/csrf/config_test.gomiddleware/csrf/csrf_test.gomiddleware/csrf/config.godocs/middleware/csrf.md
📚 Learning: 2024-09-25T15:57:10.221Z
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-09-25T15:57:10.221Z
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/csrf/config_test.gomiddleware/csrf/csrf_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
PR: gofiber/fiber#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:
docs/middleware/csrf.md
🧬 Code graph analysis (3)
middleware/csrf/config_test.go (2)
ctx_interface_gen.go (1)
Ctx(18-430)extractors/extractors.go (1)
ErrNotFound(64-64)
middleware/csrf/csrf_test.go (3)
middleware/csrf/csrf.go (1)
New(45-189)middleware/csrf/config.go (1)
Config(16-121)extractors/extractors.go (1)
ErrNotFound(64-64)
middleware/csrf/config.go (1)
log/fiberlog.go (1)
Warnf(54-56)
⏰ 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). (4)
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: unit (1.25.x, macos-latest)
- GitHub Check: repeated
- GitHub Check: Compare
🔇 Additional comments (14)
middleware/csrf/storage_manager_msgp.go (2)
10-36: Explicit error propagation in msgp methods: LGTMReturning explicit errors improves clarity and appeases linters; behavior remains unchanged.
Since this file is generated (“DO NOT EDIT”), please confirm these changes come from re‑running msgp with an updated template or generator flags. If edited manually, they’ll be lost on the next regen—consider adding/adjusting a go:generate step.
Also applies to: 38-56, 58-86, 88-92
95-121: Same improvements applied to storageManager: LGTMConsistent error returns across Encode/Decode/Marshal/Unmarshal and Msgsize.
Also applies to: 124-132, 134-141, 144-171, 174-177
middleware/csrf/config.go (2)
133-134: Default extractor switched to shared header extractor: LGTMMatches the new API surface, no behavior change.
200-215: Cookie extractor safety check: LGTMExact‑match panic and case‑insensitive warning are sensible defaults.
middleware/csrf/config_test.go (5)
22-27: Secure extractor configs: LGTMGood coverage across header/form/query/param and chained header→form.
42-58: Insecure cookie extractor panic case: LGTMAccurately exercises the exact‑name match guard.
60-84: Chained insecure cookie extractor panic: LGTMValidates chained extractor inspection.
246-286: Extractor error types: LGTMDirectly asserting extractors.ErrNotFound across header/form/query/param is appropriate.
390-408: Case‑insensitive cookie name warning path: LGTMMatches config.go’s warning (no panic) path.
middleware/csrf/csrf_test.go (5)
356-357: Defaulting extractor to shared header extractor in multi‑use test: LGTM
459-460: Extractor migrations (form/query/param): LGTMTests now reflect shared extractors API.
Also applies to: 496-497, 534-535
572-585: Custom extractor test updated to extractors.Extractor: LGTMError path uses extractors.ErrNotFound as expected.
Also applies to: 624-630
1594-1598: Chain extractor tests: LGTMCovers multi, empty, and single‑element chains.
If extractors.Chain metadata semantics change upstream, these tests may fail. OK to keep, but consider asserting behavior (extraction order) rather than metadata if flakes arise.
Also applies to: 1670-1671, 1702-1703
1743-1805: All extractors + error types coverage: LGTMGood matrix across header/form/query and error cases.
Also applies to: 1873-1874, 1906-1907, 1943-1969
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/middleware/csrf.md (1)
265-287: Update the “never do this” example to the shared extractors API.This snippet still uses
csrf.Extractorandcsrf.SourceCustom. It should demonstrate the shared extractors package (and it’s clearer to show the bad case viaFromCookie).-// ❌ NEVER DO THIS - Completely defeats CSRF protection -badExtractor := csrf.Extractor{ - Extract: func(c fiber.Ctx) (string, error) { - return c.Cookies("csrf_"), nil // Always passes validation! - }, - Source: csrf.SourceCustom, // See extractors.SourceCustom in shared package - Key: "csrf_", -} +// ❌ NEVER DO THIS - Completely defeats CSRF protection +// Reading from the same cookie name as CookieName will panic and would nullify protection. +badExtractor := extractors.FromCookie("csrf_") // ✅ DO THIS - Extract from different source than cookie app.Use(csrf.New(csrf.Config{ CookieName: "csrf_", - Extractor: extractors.FromHeader("X-Csrf-Token"), // Header vs cookie comparison + Extractor: extractors.FromHeader(csrf.HeaderName), // Header vs cookie comparison }))
🧹 Nitpick comments (3)
docs/middleware/csrf.md (3)
29-31: Prefer using the publiccsrf.HeaderNameconstant in examples to avoid drift.Hardcoding "X-Csrf-Token" duplicates the package constant. Use
csrf.HeaderNamein Go snippets.- Extractor: extractors.FromHeader("X-Csrf-Token"), + Extractor: extractors.FromHeader(csrf.HeaderName),- Extractor: extractors.FromHeader("X-Csrf-Token"), + Extractor: extractors.FromHeader(csrf.HeaderName),- Extractor: extractors.FromHeader("X-Csrf-Token"), + Extractor: extractors.FromHeader(csrf.HeaderName),- extractors.FromHeader("X-Csrf-Token"), + extractors.FromHeader(csrf.HeaderName),Also applies to: 43-45, 80-83, 94-97, 252-257, 259-263, 301-308
218-229: Clarify extractor constants location with an explicit example.Consider adding one inline example (e.g.,
extractors.SourceHeader) to remove any ambiguity.
232-245: Fix ambiguity around the panic condition for cookie extractors.The sentence at Line 241 is ambiguous (“It will panic if this is the case”). Spell out that the panic occurs when the extractor reads from a cookie with the same name as
CookieName. Also prefix the source constant with theextractorspackage for consistency.-If you do this, set the extractor’s `Source` to `SourceCookie` and allow the middleware to check that the cookie name is different from your CSRF cookie. It will panic if this is the case. +If you do this, set the extractor’s `Source` to `extractors.SourceCookie` and ensure the cookie name differs from your CSRF cookie’s `CookieName`. The middleware will panic if the names match, since that configuration nullifies CSRF protection.Verify the implementation indeed panics on same‑name cookie extractors to keep docs accurate.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/middleware/csrf.md(13 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
docs/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Review and update the contents of the
docsfolder if necessary when modifying code
Files:
docs/middleware/csrf.md
🧠 Learnings (7)
📓 Common learnings
Learnt from: sixcolors
PR: gofiber/fiber#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.
📚 Learning: 2025-07-27T17:28:53.403Z
Learnt from: sixcolors
PR: gofiber/fiber#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/csrf.md
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
PR: gofiber/fiber#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:
docs/middleware/csrf.md
📚 Learning: 2025-07-19T18:04:19.891Z
Learnt from: ReneWerner87
PR: gofiber/fiber#3598
File: docs/middleware/csrf.md:364-366
Timestamp: 2025-07-19T18:04:19.891Z
Learning: Both `*Session` and `*Middleware` in the session package have `Destroy()` methods with the signature `func Destroy() error` that take no arguments. The method is called directly on the session middleware instance without any parameters.
Applied to files:
docs/middleware/csrf.md
📚 Learning: 2025-07-19T18:04:19.891Z
Learnt from: ReneWerner87
PR: gofiber/fiber#3598
File: docs/middleware/csrf.md:364-366
Timestamp: 2025-07-19T18:04:19.891Z
Learning: The session middleware's `Destroy()` method signature is `func (m *Middleware) Destroy() error` and takes no arguments. It is called on the session middleware instance, not on a session store, and does not require a fiber.Ctx parameter.
Applied to files:
docs/middleware/csrf.md
📚 Learning: 2025-09-14T00:10:40.547Z
Learnt from: sixcolors
PR: gofiber/fiber#0
File: :0-0
Timestamp: 2025-09-14T00:10:40.547Z
Learning: The `session.Release()` method in the `middleware/session` package is responsible for returning the Session back to `sync.Pool` via `sessionPool.Put(s)`. It also calls Reset() to clear session data. Users must call Release() when done with the session, even after calling Save().
Applied to files:
docs/middleware/csrf.md
📚 Learning: 2025-09-14T00:10:40.547Z
Learnt from: sixcolors
PR: gofiber/fiber#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/csrf.md
⏰ 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). (2)
- GitHub Check: Compare
- GitHub Check: repeated
🔇 Additional comments (3)
docs/middleware/csrf.md (3)
296-312: LGTM on chained extractors and warning.Chaining via
extractors.Chainand the cautionary note read correctly.
370-372: Session destroy call aligns with session middleware API.
session.Destroy()with no args matches the documented signature and prior guidance.
405-410: Config.Extractor type and default are correct post‑migration.Good move to
extractors.ExtractorwithFromHeader(csrf.HeaderName)as default.
…rn statements in storage manager
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
middleware/csrf/csrf_test.go (3)
147-152: Fix: invalid use of strings.SplitSeq and range value
strings.SplitSeqdoes not exist in the stdlib, and ranging with a single identifier yields indexes, not the split values. This won’t compile and/or will misbehave.Apply:
- for header := range strings.SplitSeq(token, ";") { - if strings.Split(utils.Trim(header, ' '), "=")[0] == ConfigDefault.CookieName { - token = strings.Split(header, "=")[1] - break - } - } + for _, header := range strings.Split(token, ";") { + if strings.Split(utils.Trim(header, ' '), "=")[0] == ConfigDefault.CookieName { + token = strings.Split(header, "=")[1] + break + } + }
319-324: Same SplitSeq issue hereRepeat the same fix as above for this occurrence.
1067-1077: Pair every app.AcquireCtx(...) with app.ReleaseCtx(...) in testsAcquireCtx is used without a matching ReleaseCtx at these locations — avoid calling AcquireCtx inline (e.g., HandlerFromContext(app.AcquireCtx(...))); assign to a variable and release it (c := app.AcquireCtx(ctx); t.Cleanup(func(){ app.ReleaseCtx(c) }) or defer app.ReleaseCtx(c)):
- middleware/csrf/csrf_test.go:1072, 1075
- middleware/csrf/csrf_test.go:1093, 1095
- middleware/csrf/csrf_test.go:1163, 1165
docs/middleware/csrf.md (1)
274-286: Fix code sample to use the shared extractors packageSample still references
csrf.Extractor/csrf.SourceCustom. Update toextractors.*.- badExtractor := csrf.Extractor{ - Extract: func(c fiber.Ctx) (string, error) { - return c.Cookies("csrf_"), nil // Always passes validation! - }, - Source: csrf.SourceCustom, // See extractors.SourceCustom in shared package - Key: "csrf_", - } + badExtractor := extractors.Extractor{ + Extract: func(c fiber.Ctx) (string, error) { + return c.Cookies("csrf_"), nil // Always passes validation! + }, + Source: extractors.SourceCustom, // For illustration; do not use cookies for CSRF tokens + Key: "csrf_", + }
🧹 Nitpick comments (1)
middleware/csrf/csrf_test.go (1)
146-151: Optional: prefer cookie API over manual Set-Cookie parsingWhere feasible, prefer
fasthttp.Cookiehelpers (as you already do in other tests) over splittingSet-Cookiemanually to reduce parsing fragility when multiple cookies are set.Also applies to: 318-324, 652-655, 1400-1404
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/middleware/csrf.md(13 hunks)middleware/csrf/csrf.go(2 hunks)middleware/csrf/csrf_test.go(19 hunks)middleware/csrf/storage_manager_msgp.go(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- middleware/csrf/storage_manager_msgp.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Format Go code using gofumpt (enforced viamake format)
Ensure code passes golangci-lint checks (enforced viamake lint)
Optimize struct field alignment using betteralign (enforced viamake betteralign)
Modernize Go code using gopls modernize (enforced viamake modernize)
Files:
middleware/csrf/csrf.gomiddleware/csrf/csrf_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/csrf.md
🧠 Learnings (7)
📚 Learning: 2025-07-27T17:28:53.403Z
Learnt from: sixcolors
PR: gofiber/fiber#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/csrf.mdmiddleware/csrf/csrf_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
PR: gofiber/fiber#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:
docs/middleware/csrf.md
📚 Learning: 2025-07-19T18:04:19.891Z
Learnt from: ReneWerner87
PR: gofiber/fiber#3598
File: docs/middleware/csrf.md:364-366
Timestamp: 2025-07-19T18:04:19.891Z
Learning: Both `*Session` and `*Middleware` in the session package have `Destroy()` methods with the signature `func Destroy() error` that take no arguments. The method is called directly on the session middleware instance without any parameters.
Applied to files:
docs/middleware/csrf.md
📚 Learning: 2025-07-19T18:04:19.891Z
Learnt from: ReneWerner87
PR: gofiber/fiber#3598
File: docs/middleware/csrf.md:364-366
Timestamp: 2025-07-19T18:04:19.891Z
Learning: The session middleware's `Destroy()` method signature is `func (m *Middleware) Destroy() error` and takes no arguments. It is called on the session middleware instance, not on a session store, and does not require a fiber.Ctx parameter.
Applied to files:
docs/middleware/csrf.md
📚 Learning: 2025-09-14T00:10:40.547Z
Learnt from: sixcolors
PR: gofiber/fiber#0
File: :0-0
Timestamp: 2025-09-14T00:10:40.547Z
Learning: The `session.Release()` method in the `middleware/session` package is responsible for returning the Session back to `sync.Pool` via `sessionPool.Put(s)`. It also calls Reset() to clear session data. Users must call Release() when done with the session, even after calling Save().
Applied to files:
docs/middleware/csrf.md
📚 Learning: 2025-09-14T00:10:40.547Z
Learnt from: sixcolors
PR: gofiber/fiber#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/csrf.md
📚 Learning: 2024-09-25T15:57:10.221Z
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-09-25T15:57:10.221Z
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/csrf/csrf_test.go
🧬 Code graph analysis (2)
middleware/csrf/csrf.go (1)
app.go (1)
ErrorHandler(59-59)
middleware/csrf/csrf_test.go (2)
middleware/csrf/csrf.go (2)
New(46-194)ErrTokenNotFound(16-16)middleware/csrf/config.go (1)
Config(16-121)
⏰ 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). (6)
- GitHub Check: unit (1.25.x, macos-13)
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: unit (1.25.x, macos-latest)
- GitHub Check: Compare
- GitHub Check: unit (1.25.x, ubuntu-latest)
- GitHub Check: repeated
🔇 Additional comments (7)
middleware/csrf/csrf.go (2)
11-11: Good mapping of extractor errors to CSRF domain errorsImporting shared extractors and translating
extractors.ErrNotFoundtoErrTokenNotFoundis correct and keeps error semantics stable for callers. No further issues here.Also applies to: 138-142
135-147: Defensive guard for empty tokens from custom extractorsYou correctly treat
""asErrTokenNotFound. This covers custom extractors that mistakenly return empty strings without errors and matches existing behavior.middleware/csrf/csrf_test.go (2)
624-630: Nice: explicit empty-string extractor testThis validates the middleware’s fallback to
ErrTokenNotFound. Good coverage.
355-358: Tests migrated to shared extractors look correctUsing
extractors.FromHeader/Form/Query/Paramandextractors.Chain(...)aligns with the new API and exercises the mapping in middleware.Also applies to: 459-460, 496-497, 534-535, 1701-1704
docs/middleware/csrf.md (3)
301-308: Good example: chaining header then form with shared extractorsAccurately reflects
extractors.Chainbehavior and intended precedence.
29-46: Docs align with shared extractors usageImports and configuration samples correctly reference
github.com/gofiber/fiber/v3/extractorsand use the new API.Also applies to: 80-83, 94-97, 252-263
232-245: Clarify cookie-extractor panic wording and conditionsPanic occurs at configuration time when an extractor’s Source == extractors.SourceCookie and its Key exactly equals the CSRF CookieName. Chained extractors are checked and will also panic if any read the same cookie; case-insensitive matches only emit a warning.
- If you do this, set the extractor’s `Source` to `SourceCookie` and allow the middleware to check that the cookie name is different from your CSRF cookie. It will panic if this is the case. + If you implement a cookie-based extractor, set its `Source` to `extractors.SourceCookie` and `Key` to the cookie name. The CSRF middleware will panic if the extractor’s cookie name equals your CSRF `CookieName`, because that configuration disables CSRF protection.
| // With session middleware | ||
| session.Destroy() // Also deletes CSRF token | ||
| // Destroying the session will also remove the CSRF token if using session-based CSRF. | ||
| session.Destroy() | ||
| ``` |
There was a problem hiding this comment.
Correct session-destruction example and lifecycle
session.Destroy() as a bare call is unclear. Show destruction via context and recommend releasing the session.
- // Destroying the session will also remove the CSRF token if using session-based CSRF.
- session.Destroy()
+ // Destroying the session will also remove the CSRF token if using session-based CSRF.
+ sess := session.FromContext(c)
+ if err := sess.Destroy(); err != nil {
+ // handle error
+ }
+ sess.Release() // return to poolCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In docs/middleware/csrf.md around lines 369 to 372, the example uses a bare
session.Destroy() which is ambiguous; update the example to show destroying the
session with the request context (e.g., session.Destroy(ctx)) and demonstrate
releasing/saving the session afterwards (call the session's Release/Save/Close
method as appropriate) so readers see the full lifecycle: obtain session from
context, destroy using the same context, and then release the session to avoid
resource leaks.
Uh oh!
There was an error while loading. Please reload this page.