Refactor CSRF configuration removing KeyLookup, and remove FromCookie Extractor#3596
Refactor CSRF configuration removing KeyLookup, and remove FromCookie Extractor#3596sixcolors wants to merge 9 commits into
KeyLookup, and remove FromCookie Extractor#3596Conversation
…tractor usage docs: simplify and improve
|
Warning Rate limit exceeded@sixcolors has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 31 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe CSRF middleware documentation and implementation were updated to remove the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FiberApp
participant CSRF Middleware
Client->>FiberApp: Sends HTTP request (with token in header/form/query)
FiberApp->>CSRF Middleware: Processes request
CSRF Middleware->>CSRF Middleware: Uses configured Extractor(s) (possibly via Chain)
alt Token found and valid
CSRF Middleware->>FiberApp: Passes request to handler
FiberApp->>Client: Returns response
else Token missing/invalid
CSRF Middleware->>Client: Returns 403 Forbidden
end
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 significantly refactors the CSRF middleware by streamlining its configuration around a more flexible and explicit extractor system. This change enhances customization capabilities, particularly for defining route-specific token extraction, and is accompanied by a comprehensive update to the documentation to improve clarity and guide users through various application types and security considerations.
Highlights
- Refactored CSRF Configuration: The
KeyLookupoption has been removed from the CSRF middleware configuration. Token extraction is now solely managed by theExtractorfield, which is now a required configuration property, simplifying and standardizing how CSRF tokens are located in requests. - Enhanced Extractor System: Introduced a more flexible extractor system, including built-in extractors (
FromHeader,FromForm,FromQuery,FromParam,FromCookie). Support for route-specific CSRF extractors has been added, allowing different token extraction methods for different route groups. A newChainextractor enables combining multiple extraction methods as fallbacks. - Improved Documentation: The CSRF middleware documentation (
docs/middleware/csrf.md) has been significantly overhauled. It now includes clearer quick-start guides, configuration examples tailored for Server-Side Rendered (SSR) apps and Single Page Applications (SPAs), detailed explanations of token extractors (including custom and chained examples), and updated sections on security patterns, token management, and best practices.
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 is currently in preview and 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 to provide feedback.
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.
Code Review
The code changes introduce a refactoring of the CSRF middleware configuration, replacing the KeyLookup option with a more flexible Extractor system. The documentation has been improved for clarity. There are a few issues in the tests that need to be addressed due to the refactoring, and a minor clarification for the configuration comments.
| csrfGroup := app.Group("/", New(Config{ | ||
| Extractor: FromCookie("csrf"), | ||
| })) |
There was a problem hiding this comment.
This test will fail after the refactoring. The KeyLookup logic previously also set the CookieName to match the cookie name in the extractor. Since this coupling has been removed, you now need to explicitly set CookieName to match the name used in FromCookie for this test to pass.
| csrfGroup := app.Group("/", New(Config{ | |
| Extractor: FromCookie("csrf"), | |
| })) | |
| csrfGroup := app.Group("/", New(Config{ | |
| Extractor: FromCookie("csrf"), | |
| CookieName: "csrf", | |
| })) |
| cfg := configDefault(Config{ | ||
| KeyLookup: "cookie:csrf", | ||
| Extractor: FromCookie("csrf"), | ||
| CookieSameSite: "None", | ||
| }) |
There was a problem hiding this comment.
This change from KeyLookup to Extractor causes a test failure on a subsequent line (require.Equal(t, "csrf", cfg.CookieName) on line 1595), which is outside the diff. The old KeyLookup logic would set CookieName to csrf, but the new implementation doesn't. cfg.CookieName will now be the default "csrf_". To fix the test, you should explicitly set CookieName in the test config.
| cfg := configDefault(Config{ | |
| KeyLookup: "cookie:csrf", | |
| Extractor: FromCookie("csrf"), | |
| CookieSameSite: "None", | |
| }) | |
| cfg := configDefault(Config{ | |
| Extractor: FromCookie("csrf"), | |
| CookieName: "csrf", | |
| CookieSameSite: "None", | |
| }) |
| // handle error | ||
| } | ||
| handler.DeleteToken(c) | ||
| } |
There was a problem hiding this comment.
The DeleteToken function returns an error which should be handled in this example. Ignoring potential errors can lead to subtle bugs, and the documentation should promote best practices by showing how to handle them.
| handler.DeleteToken(c) | |
| handler := csrf.HandlerFromContext(c) | |
| if handler != nil { | |
| if err := handler.DeleteToken(c); err != nil { | |
| // handle error, e.g. log it | |
| } | |
| } |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/middleware/csrf.md(3 hunks)middleware/csrf/config.go(2 hunks)middleware/csrf/csrf_test.go(6 hunks)middleware/csrf/extractors.go(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: ReneWerner87
PR: gofiber/fiber#0
File: :0-0
Timestamp: 2024-12-01T10:28:36.011Z
Learning: Feature request #3224 has been created to add support for square bracket notation and comma-separated values in multipart form data in Fiber, while maintaining binary data transfer capabilities. This would bring parity with the existing form-urlencoded functionality.
Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:51-54
Timestamp: 2024-07-01T03:33:22.283Z
Learning: Unit tests for key length enforcement in `DecryptCookie` have been added to ensure consistency and security in the encryption processes.
Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:51-54
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests for key length enforcement in `DecryptCookie` have been added to ensure consistency and security in the encryption processes.
Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:20-23
Timestamp: 2024-10-08T19:06:06.583Z
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.
Learnt from: gaby
PR: gofiber/fiber#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.
Learnt from: dave-gray101
PR: gofiber/fiber#3027
File: docs/api/middleware/keyauth.md:222-222
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The `KeyLookup` field in the `keyauth` middleware does not support pipe-delimited keys. Instead, it specifies a single source and name for key extraction, with additional sources specified in the `FallbackKeyLookups` field.
Learnt from: dave-gray101
PR: gofiber/fiber#3027
File: docs/api/middleware/keyauth.md:222-222
Timestamp: 2024-06-09T18:50:02.075Z
Learning: The `KeyLookup` field in the `keyauth` middleware does not support pipe-delimited keys. Instead, it specifies a single source and name for key extraction, with additional sources specified in the `FallbackKeyLookups` field.
middleware/csrf/config.go (9)
Learnt from: dave-gray101
PR: gofiber/fiber#3027
File: docs/api/middleware/keyauth.md:222-222
Timestamp: 2024-06-09T18:50:02.075Z
Learning: The `KeyLookup` field in the `keyauth` middleware does not support pipe-delimited keys. Instead, it specifies a single source and name for key extraction, with additional sources specified in the `FallbackKeyLookups` field.
Learnt from: dave-gray101
PR: gofiber/fiber#3027
File: docs/api/middleware/keyauth.md:222-222
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The `KeyLookup` field in the `keyauth` middleware does not support pipe-delimited keys. Instead, it specifies a single source and name for key extraction, with additional sources specified in the `FallbackKeyLookups` field.
Learnt from: gaby
PR: gofiber/fiber#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`.
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.
Learnt from: sixcolors
PR: gofiber/fiber#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.
Learnt from: ReneWerner87
PR: gofiber/fiber#0
File: :0-0
Timestamp: 2024-12-01T10:28:36.011Z
Learning: Fiber currently supports square bracket notation in form data with 'application/x-www-form-urlencoded' content type, but this feature is not available for multipart form data. The implementation for form-urlencoded is in ctx.go (BodyParser method), while multipart form data handling needs enhancement to support this feature.
Learnt from: sixcolors
PR: gofiber/fiber#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`.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/config.go:122-122
Timestamp: 2024-10-08T19:06:06.583Z
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`.
Learnt from: ReneWerner87
PR: gofiber/fiber#3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the `Test` method in `app.go`.
middleware/csrf/csrf_test.go (17)
Learnt from: gaby
PR: gofiber/fiber#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.
Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:22-25
Timestamp: 2024-07-02T13:29:56.992Z
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.
Learnt from: gaby
PR: gofiber/fiber#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.
Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:20-23
Timestamp: 2024-10-08T19:06:06.583Z
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.
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.
Learnt from: dave-gray101
PR: gofiber/fiber#3027
File: docs/api/middleware/keyauth.md:222-222
Timestamp: 2024-06-09T18:50:02.075Z
Learning: The `KeyLookup` field in the `keyauth` middleware does not support pipe-delimited keys. Instead, it specifies a single source and name for key extraction, with additional sources specified in the `FallbackKeyLookups` field.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/csrf/csrf_test.go:164-165
Timestamp: 2024-09-25T17:08:07.693Z
Learning: In the `Test_CSRF_WithSession_Middleware` function, calling `session.NewWithStore()` without arguments is acceptable, as the default configuration is sufficient.
Learnt from: dave-gray101
PR: gofiber/fiber#3027
File: docs/api/middleware/keyauth.md:222-222
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The `KeyLookup` field in the `keyauth` middleware does not support pipe-delimited keys. Instead, it specifies a single source and name for key extraction, with additional sources specified in the `FallbackKeyLookups` field.
Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:51-54
Timestamp: 2024-07-01T03:33:22.283Z
Learning: Unit tests for key length enforcement in `DecryptCookie` have been added to ensure consistency and security in the encryption processes.
Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:51-54
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests for key length enforcement in `DecryptCookie` have been added to ensure consistency and security in the encryption processes.
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.
Learnt from: sixcolors
PR: gofiber/fiber#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.
Learnt from: gaby
PR: gofiber/fiber#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`.
Learnt from: sixcolors
PR: gofiber/fiber#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`.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/config.go:122-122
Timestamp: 2024-10-08T19:06:06.583Z
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`.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/config.go:16-26
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In the session middleware `Config` struct, `Store` is backed by `fiber.Storage`; they are different entities serving distinct purposes in session management.
Learnt from: sixcolors
PR: gofiber/fiber#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.
docs/middleware/csrf.md (7)
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.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/csrf/csrf_test.go:164-165
Timestamp: 2024-09-25T17:08:07.693Z
Learning: In the `Test_CSRF_WithSession_Middleware` function, calling `session.NewWithStore()` without arguments is acceptable, as the default configuration is sufficient.
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.
Learnt from: sixcolors
PR: gofiber/fiber#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.
Learnt from: gaby
PR: gofiber/fiber#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`.
Learnt from: sixcolors
PR: gofiber/fiber#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.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/config.go:16-26
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In the session middleware `Config` struct, `Store` is backed by `fiber.Storage`; they are different entities serving distinct purposes in session management.
🧬 Code Graph Analysis (2)
middleware/csrf/extractors.go (2)
ctx_interface_gen.go (1)
Ctx(17-395)middleware/csrf/csrf.go (1)
ErrTokenNotFound(16-16)
middleware/csrf/config.go (3)
middleware/session/config.go (1)
ConfigDefault(109-115)middleware/session/session.go (1)
Session(17-25)log/fiberlog.go (1)
Warn(19-21)
🪛 GitHub Check: markdownlint
docs/middleware/csrf.md
[failure] 272-272: Lists should be surrounded by blank lines
docs/middleware/csrf.md:272 MD032/blanks-around-lists Lists should be surrounded by blank lines [Context: "- Stores tokens in user sessio..."] https://github.com/DavidAnson/markdownlint/blob/v0.38.0/doc/md032.md
[failure] 271-271: Headings should be surrounded by blank lines
docs/middleware/csrf.md:271 MD022/blanks-around-headings Headings should be surrounded by blank lines [Expected: 1; Actual: 0; Below] [Context: "### Synchronizer Token (with Session)"] https://github.com/DavidAnson/markdownlint/blob/v0.38.0/doc/md022.md
[failure] 267-267: Lists should be surrounded by blank lines
docs/middleware/csrf.md:267 MD032/blanks-around-lists Lists should be surrounded by blank lines [Context: "- Stores tokens in memory/data..."] https://github.com/DavidAnson/markdownlint/blob/v0.38.0/doc/md032.md
[failure] 266-266: Headings should be surrounded by blank lines
docs/middleware/csrf.md:266 MD022/blanks-around-headings Headings should be surrounded by blank lines [Expected: 1; Actual: 0; Below] [Context: "### Double Submit Cookie (Default)"] https://github.com/DavidAnson/markdownlint/blob/v0.38.0/doc/md022.md
[failure] 222-222: Headings should be surrounded by blank lines
docs/middleware/csrf.md:222 MD022/blanks-around-headings Headings should be surrounded by blank lines [Expected: 1; Actual: 0; Below] [Context: "### Custom Extractor"] https://github.com/DavidAnson/markdownlint/blob/v0.38.0/doc/md022.md
[failure] 205-205: Headings should be surrounded by blank lines
docs/middleware/csrf.md:205 MD022/blanks-around-headings Headings should be surrounded by blank lines [Expected: 1; Actual: 0; Below] [Context: "#### Using Route-Specific Extractors"] https://github.com/DavidAnson/markdownlint/blob/v0.38.0/doc/md022.md
[failure] 203-203: Lists should be surrounded by blank lines
docs/middleware/csrf.md:203 MD032/blanks-around-lists Lists should be surrounded by blank lines [Context: "- `csrf.FromCookie("csrf_token..."] https://github.com/DavidAnson/markdownlint/blob/v0.38.0/doc/md032.md
[failure] 199-199: Lists should be surrounded by blank lines
docs/middleware/csrf.md:199 MD032/blanks-around-lists Lists should be surrounded by blank lines [Context: "- `csrf.FromQuery("csrf_token"..."] https://github.com/DavidAnson/markdownlint/blob/v0.38.0/doc/md032.md
[failure] 195-195: Lists should be surrounded by blank lines
docs/middleware/csrf.md:195 MD032/blanks-around-lists Lists should be surrounded by blank lines [Context: "- `csrf.FromHeader("X-Csrf-Tok..."] https://github.com/DavidAnson/markdownlint/blob/v0.38.0/doc/md032.md
🪛 GitHub Actions: markdownlint
docs/middleware/csrf.md
[error] 195-195: markdownlint MD032: Lists should be surrounded by blank lines. See https://github.com/DavidAnson/markdownlint/blob/v0.38.0/doc/md032.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: repeated
- GitHub Check: Compare
🔇 Additional comments (16)
middleware/csrf/extractors.go (1)
72-82: LGTM! Well-implemented Chain extractor function.The
Chainfunction correctly implements fallback behavior between multiple extractors. The logic properly checks both conditions (err == nil && token != "") before returning success, and appropriately returnsErrTokenNotFoundwhen all extractors fail.middleware/csrf/config.go (4)
43-45: Clear documentation of the new required Extractor field.The comment properly indicates that
Extractoris now required with a sensible default, and lists the available extractor functions. This makes the API change clear to users.
109-109: Appropriate default extractor choice.Using
FromHeader(HeaderName)as the default extractor is a good security-conscious choice, as header-based extraction is more secure than other methods.
143-145: Correct default handling for the new Extractor field.The configuration properly sets the default extractor when none is provided, maintaining backwards compatibility while requiring explicit configuration.
147-155: isFromCookie implementation exists and is correctThe
isFromCookiefunction is defined inmiddleware/csrf/csrf.go(lines 278–280) and compares the pointer of the provided extractor to theFromCookieextractor. No missing references—no changes needed here.middleware/csrf/csrf_test.go (6)
358-358: Correct test update for header extractor.The test properly replaces the KeyLookup configuration with the explicit
FromHeader("X-Csrf-Token")extractor call, maintaining the same test behavior.
461-461: Correct test update for form extractor.The test properly replaces
KeyLookup: "form:_csrf"withExtractor: FromForm("_csrf"), maintaining equivalent functionality.
498-498: Correct test update for query extractor.The test properly replaces
KeyLookup: "query:_csrf"withExtractor: FromQuery("_csrf"), maintaining equivalent functionality.
536-536: Correct test update for param extractor.The test properly replaces
KeyLookup: "param:csrf"withExtractor: FromParam("csrf"), maintaining equivalent functionality.
574-576: Correct test update for cookie extractor.The test properly replaces
KeyLookup: "cookie:csrf"withExtractor: FromCookie("csrf"), maintaining equivalent functionality.
591-591: Correct test update for cookie extractor warning test.The test properly updates the configuration to use the new
Extractor: FromCookie("csrf")format, which will properly trigger the cookie extractor warning validation.docs/middleware/csrf.md (5)
7-7: Excellent concise description of CSRF middleware functionality.The updated description clearly explains what the middleware does (validates tokens on unsafe methods) and what happens when attacks are detected (403 Forbidden response).
17-29: Great practical examples for development vs production.The quick start section provides clear, actionable examples showing the difference between development and production configurations, with appropriate security settings highlighted.
67-69: Excellent addition of practical recipes.The links to specific recipes for different patterns (Double Submit Cookie vs Synchronizer Token) will help users choose the appropriate approach for their use case.
248-258: Excellent security warning for Chain extractor.The documentation appropriately warns about the security risks of chaining extractors while still providing the functionality for edge cases. The danger callout is well-placed.
360-364: Excellent security-focused conclusion.The production requirements callout clearly summarizes the critical security settings needed for production deployments.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
docs/middleware/csrf.md (6)
38-45: Repeat theCookiePathfix for the SSR exampleSame reasoning as above – set the path to
/to keep the__Host-cookie valid.CookieName: "__Host-csrf", CookieSecure: true, + CookiePath: "/",
52-59: SPA example also needs theCookiePathattributeWithout it the cookie won’t be stored by modern browsers.
CookieName: "__Host-csrf", CookieSecure: true, + CookiePath: "/",
92-100: Pointer receiver required inapiHandleras well-func apiHandler(c fiber.Ctx) error { +func apiHandler(c *fiber.Ctx) error {
235-248: Fix parameter type in custom extractor-func BearerTokenExtractor(c fiber.Ctx) (string, error) { +func BearerTokenExtractor(c *fiber.Ctx) (string, error) {
306-311: Error-handler closure must take*fiber.Ctx-ErrorHandler: func(c fiber.Ctx, err error) error { +ErrorHandler: func(c *fiber.Ctx, err error) error {
336-341: Still ignoringDeleteTokenerrorPrevious review already pointed this out. Propagate or at least log the error to avoid silently swallowing failures.
if handler != nil { - handler.DeleteToken(c) + if err := handler.DeleteToken(c); err != nil { + // TODO: handle/log error + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Makefile(1 hunks)docs/middleware/csrf.md(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- Makefile
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ReneWerner87
PR: gofiber/fiber#0
File: :0-0
Timestamp: 2024-12-01T10:28:36.011Z
Learning: Feature request #3224 has been created to add support for square bracket notation and comma-separated values in multipart form data in Fiber, while maintaining binary data transfer capabilities. This would bring parity with the existing form-urlencoded functionality.
Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:51-54
Timestamp: 2024-07-01T03:33:22.283Z
Learning: Unit tests for key length enforcement in `DecryptCookie` have been added to ensure consistency and security in the encryption processes.
Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:51-54
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests for key length enforcement in `DecryptCookie` have been added to ensure consistency and security in the encryption processes.
Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:20-23
Timestamp: 2024-10-08T19:06:06.583Z
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.
Learnt from: gaby
PR: gofiber/fiber#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.
Learnt from: dave-gray101
PR: gofiber/fiber#3027
File: docs/api/middleware/keyauth.md:222-222
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The `KeyLookup` field in the `keyauth` middleware does not support pipe-delimited keys. Instead, it specifies a single source and name for key extraction, with additional sources specified in the `FallbackKeyLookups` field.
Learnt from: dave-gray101
PR: gofiber/fiber#3027
File: docs/api/middleware/keyauth.md:222-222
Timestamp: 2024-06-09T18:50:02.075Z
Learning: The `KeyLookup` field in the `keyauth` middleware does not support pipe-delimited keys. Instead, it specifies a single source and name for key extraction, with additional sources specified in the `FallbackKeyLookups` field.
docs/middleware/csrf.md (13)
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.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/csrf/csrf_test.go:164-165
Timestamp: 2024-09-25T17:08:07.693Z
Learning: In the `Test_CSRF_WithSession_Middleware` function, calling `session.NewWithStore()` without arguments is acceptable, as the default configuration is sufficient.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/config.go:122-122
Timestamp: 2024-10-08T19:06:06.583Z
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`.
Learnt from: sixcolors
PR: gofiber/fiber#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`.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/session.go:272-293
Timestamp: 2024-10-02T15:16:39.868Z
Learning: In the `middleware/session/session.go` file, the `saveSession()` method returns either `nil` or an error, so additional error wrapping in the `Save()` method is unnecessary.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/session.go:272-293
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In the `middleware/session/session.go` file, the `saveSession()` method returns either `nil` or an error, so additional error wrapping in the `Save()` method is unnecessary.
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.
Learnt from: sixcolors
PR: gofiber/fiber#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.
Learnt from: hcancelik
PR: gofiber/fiber#3036
File: docs/middleware/cache.md:103-103
Timestamp: 2024-06-15T19:26:06.401Z
Learning: There are no hard tabs in the lines 100 to 105 of the `docs/middleware/cache.md` file. Future comments about formatting should accurately reflect the actual content.
Learnt from: hcancelik
PR: gofiber/fiber#3036
File: docs/middleware/cache.md:103-103
Timestamp: 2024-10-08T19:06:06.583Z
Learning: There are no hard tabs in the lines 100 to 105 of the `docs/middleware/cache.md` file. Future comments about formatting should accurately reflect the actual content.
Learnt from: gaby
PR: gofiber/fiber#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`.
Learnt from: sixcolors
PR: gofiber/fiber#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.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/config.go:16-26
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In the session middleware `Config` struct, `Store` is backed by `fiber.Storage`; they are different entities serving distinct purposes in session management.
⏰ 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: repeated
- GitHub Check: Compare
| func formHandler(c fiber.Ctx) error { | ||
| token := csrf.TokenFromContext(c) | ||
| if token == "" { | ||
| return c.Status(fiber.StatusInternalServerError) | ||
| } | ||
|
|
||
| // Note: Make sure this matches the KeyLookup configured in your middleware | ||
| // Example: If you configured csrf.Config{KeyLookup: "form:_csrf"} | ||
| formKey := "_csrf" | ||
|
|
||
| // Create a form with the CSRF token | ||
| tmpl := fmt.Sprintf(`<form action="/post" method="POST"> | ||
| <input type="hidden" name="%s" value="%s"> | ||
| <input type="text" name="message"> | ||
| <input type="submit" value="Submit"> | ||
| </form>`, formKey, token) | ||
|
|
||
| c.Set("Content-Type", "text/html") | ||
| return c.SendString(tmpl) | ||
| return c.SendString(fmt.Sprintf(` | ||
| <form method="POST" action="/submit"> | ||
| <input type="hidden" name="_csrf" value="%s"> | ||
| <input type="text" name="message" required> | ||
| <button type="submit">Submit</button> | ||
| </form> | ||
| `, token)) | ||
| } |
There was a problem hiding this comment.
Use *fiber.Ctx, not fiber.Ctx, in handler signature
All Fiber handlers receive a pointer. Using a value type will not compile.
-func formHandler(c fiber.Ctx) error {
+func formHandler(c *fiber.Ctx) error {📝 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.
| func formHandler(c fiber.Ctx) error { | |
| token := csrf.TokenFromContext(c) | |
| if token == "" { | |
| return c.Status(fiber.StatusInternalServerError) | |
| } | |
| // Note: Make sure this matches the KeyLookup configured in your middleware | |
| // Example: If you configured csrf.Config{KeyLookup: "form:_csrf"} | |
| formKey := "_csrf" | |
| // Create a form with the CSRF token | |
| tmpl := fmt.Sprintf(`<form action="/post" method="POST"> | |
| <input type="hidden" name="%s" value="%s"> | |
| <input type="text" name="message"> | |
| <input type="submit" value="Submit"> | |
| </form>`, formKey, token) | |
| c.Set("Content-Type", "text/html") | |
| return c.SendString(tmpl) | |
| return c.SendString(fmt.Sprintf(` | |
| <form method="POST" action="/submit"> | |
| <input type="hidden" name="_csrf" value="%s"> | |
| <input type="text" name="message" required> | |
| <button type="submit">Submit</button> | |
| </form> | |
| `, token)) | |
| } | |
| func formHandler(c *fiber.Ctx) error { | |
| token := csrf.TokenFromContext(c) | |
| return c.SendString(fmt.Sprintf(` | |
| <form method="POST" action="/submit"> | |
| <input type="hidden" name="_csrf" value="%s"> | |
| <input type="text" name="message" required> | |
| <button type="submit">Submit</button> | |
| </form> | |
| `, token)) | |
| } |
🤖 Prompt for AI Agents
In docs/middleware/csrf.md around lines 76 to 86, the handler function
formHandler uses fiber.Ctx as a value parameter, but Fiber requires a pointer
receiver *fiber.Ctx for handlers. Change the function signature to accept
*fiber.Ctx instead of fiber.Ctx to fix the compilation error.
| CookieName: "__Host-csrf_", | ||
| CookieSecure: true, | ||
| CookieHTTPOnly: true, // false for SPAs | ||
| CookieSameSite: "Lax", | ||
| CookieSessionOnly: true, | ||
| Extractor: csrf.FromHeader("X-Csrf-Token"), | ||
| Session: sessionStore, |
There was a problem hiding this comment.
__Host- cookie spec not fully satisfied – add CookiePath: "/" (and consider name)
The __Host- prefix is only valid when all three conditions are met:
Secureflag set (✓ already),- NO
Domainattribute (default""– ✓), Pathexplicitly set to/.
Because CookiePath defaults to "", the example violates the spec and browsers will silently drop the cookie.
While touching this, consider dropping the trailing underscore in __Host-csrf_ – it isn’t required and may confuse readers.
app.Use(csrf.New(csrf.Config{
CookieName: "__Host-csrf",
CookieSecure: true,
+ CookiePath: "/", // required for __Host- prefix
CookieHTTPOnly: true, // false for SPAsCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In docs/middleware/csrf.md around lines 22 to 28, the cookie configuration uses
the __Host- prefix but does not set CookiePath to "/", which violates the
__Host- cookie specification and causes browsers to drop the cookie. Fix this by
explicitly adding CookiePath: "/" to the configuration and remove the trailing
underscore from the CookieName to avoid confusion.
|
@copilot can you verify that the CSRF logic is secure for both double submit and synchronizer patterns? |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the CSRF middleware to replace the string-based KeyLookup configuration with a more flexible function-based Extractor system. The changes introduce route-specific CSRF extractor support and implement a new mechanism for tracking cookie extractors.
- Removes
KeyLookupconfiguration option and replaces with directExtractorfunctions - Implements cookie extractor tracking using reflection and
sync.Map - Adds new
Chainextractor for combining multiple extraction methods - Updates documentation with comprehensive examples and security guidance
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| middleware/csrf/extractors.go | Adds cookie extractor tracking, Chain function, and isCookieExtractor helper |
| middleware/csrf/csrf_test.go | Updates tests to use new Extractor configuration instead of KeyLookup |
| middleware/csrf/csrf.go | Removes reflection import and updates cookie detection logic |
| middleware/csrf/config.go | Removes KeyLookup field and related parsing logic, simplifies configuration |
| docs/middleware/csrf.md | Comprehensive documentation rewrite with examples and security guidance |
Comments suppressed due to low confidence (1)
middleware/csrf/extractors.go:93
- The function
isCookieExtractoris unexported but is used in other files. This should be exported asIsCookieExtractorto match Go naming conventions for public functions.
func isCookieExtractor(extractor any) bool {
| func FromCookie(param string) func(c fiber.Ctx) (string, error) { | ||
| return func(c fiber.Ctx) (string, error) { | ||
| extractor := func(c fiber.Ctx) (string, error) { | ||
| token := c.Cookies(param) | ||
| if token == "" { | ||
| return "", ErrMissingCookie | ||
| } | ||
| return token, nil | ||
| } | ||
|
|
||
| // Register this extractor as a cookie extractor | ||
| cookieExtractors.Store(reflect.ValueOf(extractor).Pointer(), true) | ||
|
|
||
| return extractor |
There was a problem hiding this comment.
Using reflection to store function pointers for tracking cookie extractors is complex and potentially fragile. Consider using a simpler approach like returning a tagged struct or using a type assertion interface.
| cookieExtractors sync.Map | ||
| ) | ||
|
|
There was a problem hiding this comment.
The global cookieExtractors variable creates a shared state that could lead to memory leaks or unexpected behavior. Function pointers stored in this map will never be garbage collected, and the map grows indefinitely.
| cookieExtractors sync.Map | |
| ) | |
| ) | |
| type CookieExtractor struct { | |
| Extractor func(c fiber.Ctx) (string, error) | |
| } |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3596 +/- ##
==========================================
- Coverage 90.97% 90.96% -0.02%
==========================================
Files 111 111
Lines 11208 11197 -11
==========================================
- Hits 10197 10185 -12
- Misses 759 760 +1
Partials 252 252
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.
Actionable comments posted: 1
♻️ Duplicate comments (2)
docs/middleware/csrf.md (2)
22-28: Fix cookie configuration for__Host-prefix compliance.The
__Host-prefix requiresCookiePath: "/"to be explicitly set. Without this, browsers will drop the cookie silently, breaking CSRF protection.app.Use(csrf.New(csrf.Config{ CookieName: "__Host-csrf_", CookieSecure: true, + CookiePath: "/", CookieHTTPOnly: true, // false for SPAs
76-76: Fix handler function signature.Fiber handlers require a pointer receiver
*fiber.Ctx, not a value type.-func formHandler(c fiber.Ctx) error { +func formHandler(c *fiber.Ctx) error {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docs/middleware/csrf.md(3 hunks)middleware/csrf/config.go(2 hunks)middleware/csrf/csrf.go(2 hunks)middleware/csrf/csrf_test.go(4 hunks)middleware/csrf/extractors.go(2 hunks)middleware/csrf/extractors_test.go(0 hunks)
💤 Files with no reviewable changes (1)
- middleware/csrf/extractors_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- middleware/csrf/csrf.go
- middleware/csrf/csrf_test.go
- middleware/csrf/config.go
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: ReneWerner87
PR: gofiber/fiber#0
File: :0-0
Timestamp: 2024-12-01T10:28:36.011Z
Learning: Feature request #3224 has been created to add support for square bracket notation and comma-separated values in multipart form data in Fiber, while maintaining binary data transfer capabilities. This would bring parity with the existing form-urlencoded functionality.
Learnt from: gaby
PR: gofiber/fiber#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.
Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:20-23
Timestamp: 2024-10-08T19:06:06.583Z
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.
Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:51-54
Timestamp: 2024-07-01T03:33:22.283Z
Learning: Unit tests for key length enforcement in `DecryptCookie` have been added to ensure consistency and security in the encryption processes.
Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:51-54
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests for key length enforcement in `DecryptCookie` have been added to ensure consistency and security in the encryption processes.
Learnt from: dave-gray101
PR: gofiber/fiber#3027
File: docs/api/middleware/keyauth.md:222-222
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The `KeyLookup` field in the `keyauth` middleware does not support pipe-delimited keys. Instead, it specifies a single source and name for key extraction, with additional sources specified in the `FallbackKeyLookups` field.
Learnt from: dave-gray101
PR: gofiber/fiber#3027
File: docs/api/middleware/keyauth.md:222-222
Timestamp: 2024-06-09T18:50:02.075Z
Learning: The `KeyLookup` field in the `keyauth` middleware does not support pipe-delimited keys. Instead, it specifies a single source and name for key extraction, with additional sources specified in the `FallbackKeyLookups` field.
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.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/csrf/csrf_test.go:164-165
Timestamp: 2024-09-25T17:08:07.693Z
Learning: In the `Test_CSRF_WithSession_Middleware` function, calling `session.NewWithStore()` without arguments is acceptable, as the default configuration is sufficient.
Learnt from: hcancelik
PR: gofiber/fiber#3036
File: docs/middleware/cache.md:103-103
Timestamp: 2024-06-15T19:26:06.401Z
Learning: There are no hard tabs in the lines 100 to 105 of the `docs/middleware/cache.md` file. Future comments about formatting should accurately reflect the actual content.
Learnt from: hcancelik
PR: gofiber/fiber#3036
File: docs/middleware/cache.md:103-103
Timestamp: 2024-10-08T19:06:06.583Z
Learning: There are no hard tabs in the lines 100 to 105 of the `docs/middleware/cache.md` file. Future comments about formatting should accurately reflect the actual content.
middleware/csrf/extractors.go (4)
Learnt from: sixcolors
PR: gofiber/fiber#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.
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.
Learnt from: sixcolors
PR: gofiber/fiber#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.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/csrf/session_manager.go:30-43
Timestamp: 2024-10-08T19:06:06.583Z
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.
docs/middleware/csrf.md (18)
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.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/csrf/csrf_test.go:164-165
Timestamp: 2024-09-25T17:08:07.693Z
Learning: In the `Test_CSRF_WithSession_Middleware` function, calling `session.NewWithStore()` without arguments is acceptable, as the default configuration is sufficient.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/config.go:122-122
Timestamp: 2024-10-08T19:06:06.583Z
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`.
Learnt from: sixcolors
PR: gofiber/fiber#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`.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/session.go:272-293
Timestamp: 2024-10-02T15:16:39.868Z
Learning: In the `middleware/session/session.go` file, the `saveSession()` method returns either `nil` or an error, so additional error wrapping in the `Save()` method is unnecessary.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/session.go:272-293
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In the `middleware/session/session.go` file, the `saveSession()` method returns either `nil` or an error, so additional error wrapping in the `Save()` method is unnecessary.
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.
Learnt from: sixcolors
PR: gofiber/fiber#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.
Learnt from: hcancelik
PR: gofiber/fiber#3036
File: docs/middleware/cache.md:103-103
Timestamp: 2024-10-08T19:06:06.583Z
Learning: There are no hard tabs in the lines 100 to 105 of the `docs/middleware/cache.md` file. Future comments about formatting should accurately reflect the actual content.
Learnt from: hcancelik
PR: gofiber/fiber#3036
File: docs/middleware/cache.md:103-103
Timestamp: 2024-06-15T19:26:06.401Z
Learning: There are no hard tabs in the lines 100 to 105 of the `docs/middleware/cache.md` file. Future comments about formatting should accurately reflect the actual content.
Learnt from: gaby
PR: gofiber/fiber#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`.
Learnt from: gaby
PR: gofiber/fiber#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.
Learnt from: ReneWerner87
PR: gofiber/fiber#0
File: :0-0
Timestamp: 2024-12-01T10:28:36.011Z
Learning: Fiber currently supports square bracket notation in form data with 'application/x-www-form-urlencoded' content type, but this feature is not available for multipart form data. The implementation for form-urlencoded is in ctx.go (BodyParser method), while multipart form data handling needs enhancement to support this feature.
Learnt from: mdelapenya
PR: gofiber/fiber#3434
File: docs/api/services.md:39-43
Timestamp: 2025-05-07T13:07:33.899Z
Learning: When documenting Go interface methods in the Fiber project, avoid showing method signatures with the interface type as the receiver (e.g., `func (d *Service) Method()`) since interfaces cannot be used as receivers in Go. Instead, show just the method signature without a receiver or use a placeholder implementation name.
Learnt from: ReneWerner87
PR: gofiber/fiber#3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the `Test` method in `app.go`.
Learnt from: sigmundxia
PR: gofiber/fiber#3151
File: middleware/adaptor/adaptor_test.go:226-230
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In Fiber, the `c.Set` method does not return an error.
Learnt from: sixcolors
PR: gofiber/fiber#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.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/config.go:16-26
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In the session middleware `Config` struct, `Store` is backed by `fiber.Storage`; they are different entities serving distinct purposes in session management.
🧬 Code Graph Analysis (1)
middleware/csrf/extractors.go (2)
ctx_interface_gen.go (1)
Ctx(17-395)middleware/csrf/csrf.go (1)
ErrTokenNotFound(15-15)
🪛 GitHub Check: lint
middleware/csrf/extractors.go
[failure] 19-19:
var cookieExtractorRegistry is unused (unused)
🪛 GitHub Actions: golangci-lint
middleware/csrf/extractors.go
[error] 19-19: golangci-lint: var cookieExtractorRegistry is unused (unused)
⏰ 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.24.x, windows-latest)
- GitHub Check: repeated
- GitHub Check: Compare
- GitHub Check: Analyse
🔇 Additional comments (5)
middleware/csrf/extractors.go (1)
65-75: Chain extractor implementation looks solid.The
Chainfunction provides a clean way to try multiple extractors sequentially. The implementation correctly returns the first successful token and falls back toErrTokenNotFoundif all extractors fail.docs/middleware/csrf.md (4)
226-243: Excellent security guidance on cookie extraction vulnerability.The warning about never extracting CSRF tokens from cookies using the same
CookieNameis crucial for security. This clearly explains the Double Submit Cookie pattern and why extracting from the same source defeats the protection.
271-282: Appropriate security warning for Chain extractor.The documentation correctly warns about the security risks of chaining extractors and recommends using single extractors for most use cases. This aligns with security best practices.
354-358: Good practice: Error handling in documentation example.The example correctly demonstrates error handling when deleting CSRF tokens, which promotes best practices.
372-374: Review BREACH mitigation recommendations for accuracyThe note in docs/middleware/csrf.md (lines 372–374) states that “the CSRF token is sent as a header on every request,” but in most CSRF middleware implementations tokens are only included on state-changing (unsafe) requests (e.g. POST, PUT, DELETE), not on GET. Please verify the actual behavior of your CSRF middleware and update the guidance to reflect whether tokens are indeed sent on all requests or only on unsafe methods.
- Location: docs/middleware/csrf.md (lines 372–374)
sixcolors
changed the title
KeyLookup, and remove FromCookie Extractor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
docs/middleware/csrf.md (2)
20-29: AddCookiePath: "/"to satisfy__Host-cookie requirements.The
__Host-prefix requires the path to be explicitly set to "/" along with the Secure flag. Without this, browsers will silently drop the cookie.app.Use(csrf.New(csrf.Config{ CookieName: "__Host-csrf_", CookieSecure: true, + CookiePath: "/", CookieHTTPOnly: true, // false for SPAs CookieSameSite: "Lax", CookieSessionOnly: true, Extractor: csrf.FromHeader("X-Csrf-Token"), Session: sessionStore, }))
76-86: Fix function signature to use*fiber.Ctx.Fiber handlers require pointer receivers.
-func formHandler(c fiber.Ctx) error { +func formHandler(c *fiber.Ctx) error {
🧹 Nitpick comments (4)
middleware/csrf/csrf_test.go (4)
1714-1813: Addt.Parallel()to subtests for better test performance.The parameterized test structure is excellent, but each subtest should call
t.Parallel()for concurrent execution.for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { + t.Parallel() app := fiber.New()
1813-1813: Fix formatting with gofumpt.Run
gofumpt -w middleware/csrf/csrf_test.goto fix the formatting issue.
1814-1871: Addt.Parallel()to subtests.for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { + t.Parallel() app := fiber.New()
1905-1947: Addt.Parallel()to subtests for concurrent execution.The error type testing is comprehensive and correct.
for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { + t.Parallel() token, err := tc.extractor(ctx)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/middleware/csrf.md(3 hunks)middleware/csrf/csrf.go(3 hunks)middleware/csrf/csrf_test.go(5 hunks)middleware/csrf/extractors.go(2 hunks)middleware/csrf/token.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- middleware/csrf/token.go
🚧 Files skipped from review as they are similar to previous changes (2)
- middleware/csrf/csrf.go
- middleware/csrf/extractors.go
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: ReneWerner87
PR: gofiber/fiber#0
File: :0-0
Timestamp: 2024-12-01T10:28:36.011Z
Learning: Feature request #3224 has been created to add support for square bracket notation and comma-separated values in multipart form data in Fiber, while maintaining binary data transfer capabilities. This would bring parity with the existing form-urlencoded functionality.
Learnt from: gaby
PR: gofiber/fiber#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.
Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:20-23
Timestamp: 2024-10-08T19:06:06.583Z
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.
Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:51-54
Timestamp: 2024-07-01T03:33:22.283Z
Learning: Unit tests for key length enforcement in `DecryptCookie` have been added to ensure consistency and security in the encryption processes.
Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:51-54
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests for key length enforcement in `DecryptCookie` have been added to ensure consistency and security in the encryption processes.
Learnt from: dave-gray101
PR: gofiber/fiber#3027
File: docs/api/middleware/keyauth.md:222-222
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The `KeyLookup` field in the `keyauth` middleware does not support pipe-delimited keys. Instead, it specifies a single source and name for key extraction, with additional sources specified in the `FallbackKeyLookups` field.
Learnt from: dave-gray101
PR: gofiber/fiber#3027
File: docs/api/middleware/keyauth.md:222-222
Timestamp: 2024-06-09T18:50:02.075Z
Learning: The `KeyLookup` field in the `keyauth` middleware does not support pipe-delimited keys. Instead, it specifies a single source and name for key extraction, with additional sources specified in the `FallbackKeyLookups` field.
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.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/csrf/csrf_test.go:164-165
Timestamp: 2024-09-25T17:08:07.693Z
Learning: In the `Test_CSRF_WithSession_Middleware` function, calling `session.NewWithStore()` without arguments is acceptable, as the default configuration is sufficient.
Learnt from: hcancelik
PR: gofiber/fiber#3036
File: docs/middleware/cache.md:103-103
Timestamp: 2024-06-15T19:26:06.401Z
Learning: There are no hard tabs in the lines 100 to 105 of the `docs/middleware/cache.md` file. Future comments about formatting should accurately reflect the actual content.
Learnt from: hcancelik
PR: gofiber/fiber#3036
File: docs/middleware/cache.md:103-103
Timestamp: 2024-10-08T19:06:06.583Z
Learning: There are no hard tabs in the lines 100 to 105 of the `docs/middleware/cache.md` file. Future comments about formatting should accurately reflect the actual content.
docs/middleware/csrf.md (18)
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.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/csrf/csrf_test.go:164-165
Timestamp: 2024-09-25T17:08:07.693Z
Learning: In the `Test_CSRF_WithSession_Middleware` function, calling `session.NewWithStore()` without arguments is acceptable, as the default configuration is sufficient.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/config.go:122-122
Timestamp: 2024-10-08T19:06:06.583Z
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`.
Learnt from: sixcolors
PR: gofiber/fiber#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`.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/session.go:272-293
Timestamp: 2024-10-02T15:16:39.868Z
Learning: In the `middleware/session/session.go` file, the `saveSession()` method returns either `nil` or an error, so additional error wrapping in the `Save()` method is unnecessary.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/session.go:272-293
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In the `middleware/session/session.go` file, the `saveSession()` method returns either `nil` or an error, so additional error wrapping in the `Save()` method is unnecessary.
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.
Learnt from: sixcolors
PR: gofiber/fiber#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.
Learnt from: hcancelik
PR: gofiber/fiber#3036
File: docs/middleware/cache.md:103-103
Timestamp: 2024-10-08T19:06:06.583Z
Learning: There are no hard tabs in the lines 100 to 105 of the `docs/middleware/cache.md` file. Future comments about formatting should accurately reflect the actual content.
Learnt from: hcancelik
PR: gofiber/fiber#3036
File: docs/middleware/cache.md:103-103
Timestamp: 2024-06-15T19:26:06.401Z
Learning: There are no hard tabs in the lines 100 to 105 of the `docs/middleware/cache.md` file. Future comments about formatting should accurately reflect the actual content.
Learnt from: gaby
PR: gofiber/fiber#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`.
Learnt from: gaby
PR: gofiber/fiber#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.
Learnt from: ReneWerner87
PR: gofiber/fiber#0
File: :0-0
Timestamp: 2024-12-01T10:28:36.011Z
Learning: Fiber currently supports square bracket notation in form data with 'application/x-www-form-urlencoded' content type, but this feature is not available for multipart form data. The implementation for form-urlencoded is in ctx.go (BodyParser method), while multipart form data handling needs enhancement to support this feature.
Learnt from: mdelapenya
PR: gofiber/fiber#3434
File: docs/api/services.md:39-43
Timestamp: 2025-05-07T13:07:33.899Z
Learning: When documenting Go interface methods in the Fiber project, avoid showing method signatures with the interface type as the receiver (e.g., `func (d *Service) Method()`) since interfaces cannot be used as receivers in Go. Instead, show just the method signature without a receiver or use a placeholder implementation name.
Learnt from: ReneWerner87
PR: gofiber/fiber#3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the `Test` method in `app.go`.
Learnt from: sigmundxia
PR: gofiber/fiber#3151
File: middleware/adaptor/adaptor_test.go:226-230
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In Fiber, the `c.Set` method does not return an error.
Learnt from: sixcolors
PR: gofiber/fiber#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.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/config.go:16-26
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In the session middleware `Config` struct, `Store` is backed by `fiber.Storage`; they are different entities serving distinct purposes in session management.
middleware/csrf/csrf_test.go (25)
Learnt from: gaby
PR: gofiber/fiber#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.
Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:20-23
Timestamp: 2024-10-08T19:06:06.583Z
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.
Learnt from: gaby
PR: gofiber/fiber#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.
Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:22-25
Timestamp: 2024-07-02T13:29:56.992Z
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.
Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:51-54
Timestamp: 2024-07-01T03:33:22.283Z
Learning: Unit tests for key length enforcement in `DecryptCookie` have been added to ensure consistency and security in the encryption processes.
Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:51-54
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests for key length enforcement in `DecryptCookie` have been added to ensure consistency and security in the encryption processes.
Learnt from: sixcolors
PR: gofiber/fiber#3051
File: middleware/session/session.go:215-216
Timestamp: 2024-10-08T19:06:06.583Z
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.
Learnt from: sixcolors
PR: gofiber/fiber#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.
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.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/csrf/csrf_test.go:164-165
Timestamp: 2024-09-25T17:08:07.693Z
Learning: In the `Test_CSRF_WithSession_Middleware` function, calling `session.NewWithStore()` without arguments is acceptable, as the default configuration is sufficient.
Learnt from: sixcolors
PR: gofiber/fiber#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.
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.
Learnt from: dave-gray101
PR: gofiber/fiber#3027
File: docs/api/middleware/keyauth.md:222-222
Timestamp: 2024-06-09T18:50:02.075Z
Learning: The `KeyLookup` field in the `keyauth` middleware does not support pipe-delimited keys. Instead, it specifies a single source and name for key extraction, with additional sources specified in the `FallbackKeyLookups` field.
Learnt from: dave-gray101
PR: gofiber/fiber#3027
File: docs/api/middleware/keyauth.md:222-222
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The `KeyLookup` field in the `keyauth` middleware does not support pipe-delimited keys. Instead, it specifies a single source and name for key extraction, with additional sources specified in the `FallbackKeyLookups` field.
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.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests in this project use testify require.
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.
Learnt from: sixcolors
PR: gofiber/fiber#2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The project uses the testify/assert package for assertions in unit tests.
Learnt from: gaby
PR: gofiber/fiber#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`.
Learnt from: sixcolors
PR: gofiber/fiber#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`.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/config.go:122-122
Timestamp: 2024-10-08T19:06:06.583Z
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`.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/config.go:16-26
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In the session middleware `Config` struct, `Store` is backed by `fiber.Storage`; they are different entities serving distinct purposes in session management.
Learnt from: sixcolors
PR: gofiber/fiber#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.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/middleware_test.go:190-191
Timestamp: 2024-10-12T10:01:44.206Z
Learning: When testing session `IdleTimeout` expiration, it's acceptable to use `time.Sleep` to simulate the passage of time in tests.
Learnt from: sixcolors
PR: gofiber/fiber#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.
🪛 GitHub Check: lint
middleware/csrf/csrf_test.go
[failure] 1913-1913:
fieldalignment: struct with 40 pointer bytes could be 32 (govet)
[failure] 1905-1905:
Test_CSRF_Extractors_ErrorTypes's subtests should call t.Parallel (tparallel)
[failure] 1817-1817:
fieldalignment: struct with 24 pointer bytes could be 16 (govet)
[failure] 1814-1814:
Test_CSRF_Param_Extractor's subtests should call t.Parallel (tparallel)
[failure] 1813-1813:
File is not properly formatted (gofumpt)
[failure] 1717-1717:
fieldalignment: struct with 32 pointer bytes could be 24 (govet)
[failure] 1714-1714:
Test_CSRF_All_Extractors's subtests should call t.Parallel (tparallel)
🪛 GitHub Actions: golangci-lint
middleware/csrf/csrf_test.go
[error] 1714-1714: golangci-lint: Test_CSRF_All_Extractors's subtests should call t.Parallel (tparallel)
⏰ 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.24.x, windows-latest)
- GitHub Check: repeated
- GitHub Check: modernize
- GitHub Check: Compare
🔇 Additional comments (11)
middleware/csrf/csrf_test.go (7)
354-356: LGTM!The migration from
KeyLookuptoExtractoris correctly implemented.
458-458: LGTM!Correct migration from
KeyLookuptoExtractorfor form-based token extraction.
495-495: LGTM!Proper conversion from
KeyLookuptoExtractorfor query parameter extraction.
533-533: Verify if CookieName needs to be set explicitly.The migration to
Extractorlooks correct. However, based on past review comments, the oldKeyLookuplogic would automatically setCookieNameto match the extraction source. With the new approach, you may need to explicitly setCookieNameif the test expects a specific cookie name.
1567-1640: Excellent test coverage for the new Chain extractor!The test comprehensively covers all scenarios including precedence order, fallback behavior, and error cases.
1642-1672: Good edge case coverage!Testing the empty chain extractor ensures predictable behavior when no extractors are provided.
1873-1903: Good edge case testing!Properly tests the scenario where the route parameter is missing entirely.
docs/middleware/csrf.md (4)
103-129: Excellent JavaScript example with proper error handling!The example correctly demonstrates CSRF token extraction from cookies and includes comprehensive error handling in the fetch request.
351-362: Good example with error handling guidance!The token management example properly shows error handling with a comment guiding users to handle errors appropriately.
364-390: Excellent security guidance and best practices!The documentation provides comprehensive security features overview and clear production requirements. The danger box effectively highlights critical security settings.
226-243: Excellent security warning about cookie extraction!The danger box clearly explains why extracting tokens from cookies defeats CSRF protection, with a concrete example of what to avoid. This will help prevent a critical security mistake.
| app.Use(csrf.New(csrf.Config{ | ||
| KeyLookup: "header:X-Csrf-Token", | ||
| CookieName: "csrf_", | ||
| CookieSameSite: "Lax", | ||
| IdleTimeout: 30 * time.Minute, | ||
| KeyGenerator: utils.UUIDv4, | ||
| Extractor: func(c fiber.Ctx) (string, error) { ... }, | ||
| CookieName: "__Host-csrf_", | ||
| CookieSecure: true, | ||
| CookieHTTPOnly: false, // Required for JavaScript access to tokens | ||
| CookieSameSite: "Lax", | ||
| CookieSessionOnly: true, | ||
| Extractor: csrf.FromHeader("X-Csrf-Token"), | ||
| Session: sessionStore, | ||
| })) |
There was a problem hiding this comment.
Add CookiePath: "/" for __Host- prefix compliance.
app.Use(csrf.New(csrf.Config{
CookieName: "__Host-csrf_",
CookieSecure: true,
+ CookiePath: "/",
CookieHTTPOnly: false, // Required for JavaScript access to tokens
CookieSameSite: "Lax",
CookieSessionOnly: true,
Extractor: csrf.FromHeader("X-Csrf-Token"),
Session: sessionStore,
}))📝 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.
| app.Use(csrf.New(csrf.Config{ | |
| KeyLookup: "header:X-Csrf-Token", | |
| CookieName: "csrf_", | |
| CookieSameSite: "Lax", | |
| IdleTimeout: 30 * time.Minute, | |
| KeyGenerator: utils.UUIDv4, | |
| Extractor: func(c fiber.Ctx) (string, error) { ... }, | |
| CookieName: "__Host-csrf_", | |
| CookieSecure: true, | |
| CookieHTTPOnly: false, // Required for JavaScript access to tokens | |
| CookieSameSite: "Lax", | |
| CookieSessionOnly: true, | |
| Extractor: csrf.FromHeader("X-Csrf-Token"), | |
| Session: sessionStore, | |
| })) | |
| app.Use(csrf.New(csrf.Config{ | |
| CookieName: "__Host-csrf_", | |
| CookieSecure: true, | |
| CookiePath: "/", // required for __Host- prefix compliance | |
| CookieHTTPOnly: false, // Required for JavaScript access to tokens | |
| CookieSameSite: "Lax", | |
| CookieSessionOnly: true, | |
| Extractor: csrf.FromHeader("X-Csrf-Token"), | |
| Session: sessionStore, | |
| })) |
🤖 Prompt for AI Agents
In docs/middleware/csrf.md around lines 51 to 59, the CSRF cookie configuration
is missing the CookiePath field, which is required for cookies with the __Host-
prefix to be valid. Add CookiePath: "/" to the csrf.Config struct to comply with
the __Host- cookie prefix rules.
| app.Use(csrf.New(csrf.Config{ | ||
| CookieName: "__Host-csrf_", | ||
| CookieSecure: true, | ||
| CookieHTTPOnly: true, // Secure - blocks JavaScript | ||
| CookieSameSite: "Lax", | ||
| CookieSessionOnly: true, | ||
| Extractor: csrf.FromForm("_csrf"), | ||
| Session: sessionStore, | ||
| })) | ||
| ``` |
There was a problem hiding this comment.
Add CookiePath: "/" for __Host- prefix compliance.
app.Use(csrf.New(csrf.Config{
CookieName: "__Host-csrf_",
CookieSecure: true,
+ CookiePath: "/",
CookieHTTPOnly: true, // Secure - blocks JavaScript
CookieSameSite: "Lax",
CookieSessionOnly: true,
Extractor: csrf.FromForm("_csrf"),
Session: sessionStore,
}))📝 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.
| app.Use(csrf.New(csrf.Config{ | |
| CookieName: "__Host-csrf_", | |
| CookieSecure: true, | |
| CookieHTTPOnly: true, // Secure - blocks JavaScript | |
| CookieSameSite: "Lax", | |
| CookieSessionOnly: true, | |
| Extractor: csrf.FromForm("_csrf"), | |
| Session: sessionStore, | |
| })) | |
| ``` | |
| app.Use(csrf.New(csrf.Config{ | |
| CookieName: "__Host-csrf_", | |
| CookieSecure: true, | |
| CookiePath: "/", // required for __Host- prefix | |
| CookieHTTPOnly: true, // Secure - blocks JavaScript | |
| CookieSameSite: "Lax", | |
| CookieSessionOnly: true, | |
| Extractor: csrf.FromForm("_csrf"), | |
| Session: sessionStore, | |
| })) |
🤖 Prompt for AI Agents
In docs/middleware/csrf.md around lines 37 to 46, the CSRF cookie configuration
is missing the CookiePath field, which is required for cookies with the __Host-
prefix to be valid. Add CookiePath: "/" to the csrf.Config struct to ensure
compliance with the __Host- cookie prefix rules.
Enhance CSRF configuration by removing the KeyLookup option and implementing a more flexible extractor system. Introduce support for route-specific CSRF extractors, allowing for better customization and usage. Remove
FromCookieExtractoras it is a foot-gun. Improve documentation for clarity.Supersedes #3589