Update CSRF migration for KeyLookup #142
Conversation
WalkthroughThe migration logic for CSRF middleware configuration was updated to transform the Changes
Poem
✨ Finishing Touches
🧪 Generate unit tests
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 @ReneWerner87, 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 introduces an important update to the CSRF middleware migration script. Its primary purpose is to automatically convert deprecated KeyLookup configurations within CSRF middleware settings to the new Extractor field, ensuring compatibility with updated API requirements. This change streamlines the migration process for users and includes new tests to validate the conversion logic.
Highlights
- CSRF Configuration Migration Enhancement: The
MigrateCSRFConfigfunction has been updated to handle the conversion ofKeyLookupfields to the newExtractorfield, aligning with updated CSRF middleware requirements. - Dynamic Extractor Generation: The migration now intelligently converts
KeyLookupvalues (e.g.,header:,form:,query:) into correspondingcsrf.FromHeader,csrf.FromForm, orcsrf.FromQueryextractor functions, ensuring proper configuration transformation. - Removal of Unsupported KeyLookups: Any
KeyLookupvalues that are unsupported or deemed insecure (such ascookiebased lookups) are now automatically removed during the migration process. - Comprehensive Test Coverage: A new test case,
Test_MigrateCSRFConfig_KeyLookup, has been added to verify the correct migration ofKeyLookupconfigurations, ensuring the conversion logic works as intended and prevents regressions.
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. ↩
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cmd/internal/migrations/v3/common.go (1)
203-228: Well-implemented KeyLookup migration with good security practices.The migration logic correctly:
- Preserves formatting (indentation, commas, newlines)
- Handles quoted string literals with
strconv.Unquote- Converts supported prefixes to appropriate CSRF extractor functions
- Removes unsupported/insecure values (like cookies) by returning empty string
Consider adding validation for the extracted field name to ensure it's a valid identifier:
fieldName := strings.TrimPrefix(val, "header:") if fieldName == "" { return "" // Invalid: empty field name }This would handle edge cases like
KeyLookup: "header:"with no field name specified.cmd/internal/migrations/v3/common_test.go (1)
404-427: Excellent test coverage for KeyLookup migration.The test properly validates the new functionality by:
- Following established testing patterns in the codebase
- Testing a realistic migration scenario (header prefix)
- Verifying both removal of old field and presence of new extractor
- Checking console output for proper user feedback
Consider adding test cases for other supported prefixes to ensure comprehensive coverage:
func Test_MigrateCSRFConfig_KeyLookup_AllPrefixes(t *testing.T) { // Test cases for form: and query: prefixes // Test case for unsupported prefix (should be removed) }This would provide more thorough validation of the migration logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/internal/migrations/v3/common.go(1 hunks)cmd/internal/migrations/v3/common_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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: 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.
cmd/internal/migrations/v3/common_test.go (14)
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: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: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-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: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: 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/session/middleware_test.go:400-407
Timestamp: 2024-09-25T17:09:03.756Z
Learning: In the `Test_Session_Next` function in `middleware/session/middleware_test.go`, the variable `doNext` is properly synchronized with the `muNext` mutex for both read and write access.
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`.
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#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: luk3skyw4lker
PR: gofiber/storage#1342
File: clickhouse/clickhouse_test.go:138-160
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The `Test_Reset` function in `clickhouse/clickhouse_test.go` already includes a verification step to ensure the storage is empty after a reset operation by checking that a previously set key returns an empty byte slice.
Learnt from: luk3skyw4lker
PR: gofiber/storage#1342
File: clickhouse/clickhouse_test.go:138-160
Timestamp: 2024-07-01T15:48:53.094Z
Learning: The `Test_Reset` function in `clickhouse/clickhouse_test.go` already includes a verification step to ensure the storage is empty after a reset operation by checking that a previously set key returns an empty byte slice.
🧬 Code Graph Analysis (1)
cmd/internal/migrations/v3/common.go (1)
cmd/internal/helpers.go (1)
ChangeFileContent(36-68)
⏰ 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). (1)
- GitHub Check: Build (1.24.x, macos-13)
🔇 Additional comments (1)
cmd/internal/migrations/v3/common.go (1)
197-198: LGTM! Well-structured regex patterns.The regex patterns correctly capture the structure of Go struct fields, with proper handling of whitespace, optional commas, and newlines.
There was a problem hiding this comment.
Code Review
The pull request updates the CSRF migration to handle the KeyLookup field, converting it to the new Extractor format. There is a potential issue with the regular expression used for finding KeyLookup, which could lead to incorrect migrations in some edge cases involving comments.
| replacer := strings.NewReplacer("Expiration:", "IdleTimeout:") | ||
| re := regexp.MustCompile(`\s*SessionKey:\s*[^,]+,?\n`) | ||
| reSession := regexp.MustCompile(`\s*SessionKey:\s*[^,]+,?\n`) | ||
| reKeyLookup := regexp.MustCompile(`(\s*)KeyLookup:\s*([^,\n]+)(,?)(\n?)`) |
There was a problem hiding this comment.
The current regular expression for reKeyLookup is too greedy and can cause issues when a KeyLookup line contains a comment. This could lead to the migration tool incorrectly removing the entire line.
For example, with a line like KeyLookup: "header:X-CSRF-Token" // my token, the ([^,\n]+) part of the regex will match the entire value including the comment ("header:X-CSRF-Token" // my token). This combined string will then fail to be unquoted by strconv.Unquote, causing the migration to fall into the default case which removes the line.
A safer regex would be more specific, for instance, by only matching quoted string literals. This would correctly parse the value and leave the comment untouched.
| reKeyLookup := regexp.MustCompile(`(\s*)KeyLookup:\s*([^,\n]+)(,?)(\n?)`) | |
| reKeyLookup := regexp.MustCompile(`(\s*)KeyLookup:\s*("[^"]*")\s*(,?)(\n?)`) |
Summary
KeyLookuptoExtractorTesting
go test ./...https://chatgpt.com/codex/tasks/task_e_687beb9fd3d4832696f242eccc1d1533
Summary by CodeRabbit
Bug Fixes
KeyLookupfield, ensuring more secure and explicit extractor settings.Tests
KeyLookupfield in CSRF middleware configurations.