Remove CSRF ContextKey and streamline TokenFromContext usage#169
Conversation
WalkthroughAdds CSRF v3 migration to also remove ContextKey lines alongside SessionKey, keeps IdleTimeout rename and KeyLookup-to-Extractor conversions. Updates corresponding tests. Enhances middleware locals v3 migration with regex post-processing to strip type assertions on FromContext calls and reduce multi-var short declarations; updates tests accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer Code
participant Mig as v3 Migration
participant CSRF as CSRF Config
participant MW as Middleware Locals
Dev->>Mig: Run migration
Mig->>CSRF: Load config text
CSRF-->>Mig: Content
Mig->>CSRF: Replace Expiration→IdleTimeout
Mig->>CSRF: Convert KeyLookup→Extractor(...)
Mig->>CSRF: Remove SessionKey line
Mig->>CSRF: Remove ContextKey line (new)
CSRF-->>Mig: Migrated config
Mig->>MW: Load middleware source
MW-->>Mig: Content
Mig->>MW: Apply Locals→FromContext replacements
Mig->>MW: Strip .(Type) after FromContext (new)
Mig->>MW: Simplify multi-var := FromContext (new)
MW-->>Mig: Migrated source
Mig-->>Dev: Write changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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 refines existing migration scripts to align with updated API practices and improve code cleanliness. It specifically addresses the removal of a deprecated ContextKey from CSRF configurations and simplifies how values are retrieved from context in middleware by eliminating unnecessary type assertions and streamlining variable assignments. These changes are supported by new tests to ensure the migration processes are accurate and effective.
Highlights
- CSRF Configuration Migration: The migration script for CSRF configurations now includes logic to remove the deprecated
ContextKeyfield, ensuring configurations are updated to the latest standard. - Middleware Locals Simplification: The middleware local variable migration has been enhanced to streamline code by automatically removing redundant type assertions and simplifying variable assignments from functions like
TokenFromContextto a single return value. - Improved Test Coverage: New test cases have been added for both CSRF configuration and middleware locals migrations, providing increased confidence in the correctness and robustness of the updated migration logic.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request refactors the v3 migration logic for Fiber applications. It removes the deprecated ContextKey from the CSRF middleware configuration and simplifies the usage of ...FromContext functions by removing redundant type assertions and handling changes in return values. The changes are accompanied by corresponding updates to the test suite.
My main feedback is on the implementation of the middleware locals migration. The regular expression used to simplify assignments from ...FromContext functions is too broad and could silently introduce bugs by removing named variables from two-value assignments. I've suggested a more specific regex to make the migration safer. Otherwise, the changes look good and improve the migration process.
| reTypeAssert := regexp.MustCompile(`([\w\.]+FromContext\([^\)]+\))\.\([^\)]+\)`) | ||
| content = reTypeAssert.ReplaceAllString(content, "$1") | ||
|
|
||
| reComma := regexp.MustCompile(`(\w+)\s*,\s*\w+\s*:=\s*([\w\.]+FromContext\([^\)]+\))`) |
There was a problem hiding this comment.
The regex reComma is a bit too broad. Using \w+ for the second variable in a two-value assignment will match not only the blank identifier _ but also any named variable (e.g., ok). This could lead to silent bugs if the named variable was used later in the code. For example, val, ok := ... would be changed to val := ..., removing the ok variable without warning.
It would be safer to only handle cases where the second variable is explicitly the blank identifier _. This ensures you're only changing assignments where the second return value is intentionally discarded. If a named variable is used, the migration will not touch it, and the Go compiler will flag it as an error after the function signatures change, which is a much safer failure mode.
| reComma := regexp.MustCompile(`(\w+)\s*,\s*\w+\s*:=\s*([\w\.]+FromContext\([^\)]+\))`) | |
| reComma := regexp.MustCompile("(\\w+)\\s*,\\s*_\\s*:=\\s*([\\w\\.]+FromContext\\([^\\)]+\\))") |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
cmd/internal/migrations/v3/csrfconfig.go (2)
18-18: Make ContextKey removal resilient to single-line literals and end-of-struct casesThe regex requires a trailing newline and won’t match when ContextKey appears on a single line or right before
}. Make it multiline-safe and tolerant of}.Apply:
- reContextKey := regexp.MustCompile(`\s*ContextKey:\s*[^,]+,?\n`) + reContextKey := regexp.MustCompile(`(?m)\s*ContextKey:\s*[^,\n}]+,?\s*`)Optional (consistency): mirror the same robustness for SessionKey.
- reSession := regexp.MustCompile(`\s*SessionKey:\s*[^,]+,?\n`) + reSession := regexp.MustCompile(`(?m)\s*SessionKey:\s*[^,\n}]+,?\s*`)
24-26: Scope field removals to csrf.Config blocks to avoid false positives elsewhereRight now, both SessionKey and ContextKey removals run over the whole file and can hit unrelated structs that coincidentally have these field names. Keep the operation confined within matched
csrf.Config{...}blocks for safety.Minimal change that keeps your existing structure:
- content = reConfig.ReplaceAllStringFunc(content, func(s string) string { - return strings.ReplaceAll(s, "Expiration:", "IdleTimeout:") - }) - content = reSession.ReplaceAllString(content, "") - content = reContextKey.ReplaceAllString(content, "") + content = reConfig.ReplaceAllStringFunc(content, func(s string) string { + s = strings.ReplaceAll(s, "Expiration:", "IdleTimeout:") + s = reSession.ReplaceAllString(s, "") + s = reContextKey.ReplaceAllString(s, "") + return s + })If you want to go further, you can also move the
reKeyLookuptransform inside the sameReplaceAllStringFuncto keep all CSRF config edits localized.cmd/internal/migrations/v3/csrfconfig_test.go (1)
41-41: Assertion looks right; consider adding a single-line config case
assert.NotContains(..., "ContextKey")correctly verifies removal. To guard against single-line configs (where the current regex was previously brittle), add a test like:func Test_MigrateCSRFConfig_InlineFields(t *testing.T) { t.Parallel() dir, err := os.MkdirTemp("", "mcsrf-inline") require.NoError(t, err) defer func() { require.NoError(t, os.RemoveAll(dir)) }() file := writeTempFile(t, dir, `package main import "github.com/gofiber/fiber/v2/middleware/csrf" var _ = csrf.New(csrf.Config{ContextKey: "csrf", KeyLookup: "query:_token"})`) var buf bytes.Buffer cmd := newCmd(&buf) require.NoError(t, v3.MigrateCSRFConfig(cmd, dir, nil, nil)) content := readFile(t, file) assert.NotContains(t, content, "ContextKey") assert.NotContains(t, content, "KeyLookup") assert.Contains(t, content, `Extractor: csrf.FromQuery("_token")`) }cmd/internal/migrations/v3/middleware_locals_test.go (1)
38-40: Expectations align; add a guard test for non-targetFromContextusageAssertions reflect the intended output. To prevent regressions from overbroad regexes, add a case that must remain untouched (e.g., gRPC peer context):
func Test_MigrateMiddlewareLocals_DoesNotTouchOtherFromContext(t *testing.T) { t.Parallel() dir, err := os.MkdirTemp("", "mlocals-negative") require.NoError(t, err) defer func() { require.NoError(t, os.RemoveAll(dir)) }() file := writeTempFile(t, dir, `package main import ( "github.com/gofiber/fiber/v2" peer "google.golang.org/grpc/peer" ) func handler2(c fiber.Ctx) error { p, ok := peer.FromContext(c.Context()) _ = p; _ = ok return nil }`) var buf bytes.Buffer cmd := newCmd(&buf) require.NoError(t, v3.MigrateMiddlewareLocals(cmd, dir, nil, nil)) content := readFile(t, file) assert.Contains(t, content, "p, ok := peer.FromContext") assert.NotContains(t, content, "p :=") // ensure we didn't collapse bindings }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
cmd/internal/migrations/v3/csrfconfig.go(1 hunks)cmd/internal/migrations/v3/csrfconfig_test.go(2 hunks)cmd/internal/migrations/v3/middleware_locals.go(1 hunks)cmd/internal/migrations/v3/middleware_locals_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/internal/migrations/v3/csrfconfig.go (1)
cmd/internal/helpers.go (1)
ChangeFileContent(38-79)
⏰ 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.25.x, windows-latest)
🔇 Additional comments (2)
cmd/internal/migrations/v3/csrfconfig_test.go (1)
29-30: Good addition: exercising ContextKey removal pathAdding
ContextKey: "csrf",makes the test reflect real-world usage and validates the new migration step.cmd/internal/migrations/v3/middleware_locals_test.go (1)
24-30: Covers both type-assert removal and multi-assign collapseThese inputs exercise the two post-processors well:
.(string)gets stripped andcsrfToken, _ := ...is collapsed to a single binding.
| reTypeAssert := regexp.MustCompile(`([\w\.]+FromContext\([^\)]+\))\.\([^\)]+\)`) | ||
| content = reTypeAssert.ReplaceAllString(content, "$1") | ||
|
|
||
| reComma := regexp.MustCompile(`(\w+)\s*,\s*\w+\s*:=\s*([\w\.]+FromContext\([^\)]+\))`) | ||
| content = reComma.ReplaceAllString(content, "$1 := $2") | ||
|
|
There was a problem hiding this comment.
Regex is overbroad; it may break unrelated FromContext usages (e.g., grpc/peer.FromContext)
Both post-processors target any X.FromContext(...), which risks deleting type assertions or collapsing v, ok := bindings for non-Fiber middlewares. Constrain matches to the specific packages/functions you just introduced (requestid, csrf, session, basicauth, keyauth), and keep a little whitespace tolerance.
Apply:
- reTypeAssert := regexp.MustCompile(`([\w\.]+FromContext\([^\)]+\))\.\([^\)]+\)`)
- content = reTypeAssert.ReplaceAllString(content, "$1")
-
- reComma := regexp.MustCompile(`(\w+)\s*,\s*\w+\s*:=\s*([\w\.]+FromContext\([^\)]+\))`)
- content = reComma.ReplaceAllString(content, "$1 := $2")
+ allowed := `(?:requestid|csrf|session|basicauth|keyauth)`
+ reTypeAssert := regexp.MustCompile(fmt.Sprintf(
+ `((?:%s)\.(?:TokenFromContext|HandlerFromContext|FromContext|UsernameFromContext|PasswordFromContext)\([^)]+\))\s*\.\([^)]+\)`,
+ allowed,
+ ))
+ content = reTypeAssert.ReplaceAllString(content, "$1")
+
+ reComma := regexp.MustCompile(fmt.Sprintf(
+ `(\w+)\s*,\s*\w+\s*:=\s*((?:%s)\.(?:TokenFromContext|HandlerFromContext|FromContext|UsernameFromContext|PasswordFromContext)\([^)]+\))`,
+ allowed,
+ ))
+ content = reComma.ReplaceAllString(content, "$1 := $2")Follow-up: Add a negative test to ensure peer.FromContext (gRPC) or other third-party FromContext usages aren’t altered.
📝 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.
| reTypeAssert := regexp.MustCompile(`([\w\.]+FromContext\([^\)]+\))\.\([^\)]+\)`) | |
| content = reTypeAssert.ReplaceAllString(content, "$1") | |
| reComma := regexp.MustCompile(`(\w+)\s*,\s*\w+\s*:=\s*([\w\.]+FromContext\([^\)]+\))`) | |
| content = reComma.ReplaceAllString(content, "$1 := $2") | |
| allowed := `(?:requestid|csrf|session|basicauth|keyauth)` | |
| reTypeAssert := regexp.MustCompile(fmt.Sprintf( | |
| `((?:%s)\.(?:TokenFromContext|HandlerFromContext|FromContext|UsernameFromContext|PasswordFromContext)\([^)]+\))\s*\.\([^)]+\)`, | |
| allowed, | |
| )) | |
| content = reTypeAssert.ReplaceAllString(content, "$1") | |
| reComma := regexp.MustCompile(fmt.Sprintf( | |
| `(\w+)\s*,\s*\w+\s*:=\s*((?:%s)\.(?:TokenFromContext|HandlerFromContext|FromContext|UsernameFromContext|PasswordFromContext)\([^)]+\))`, | |
| allowed, | |
| )) | |
| content = reComma.ReplaceAllString(content, "$1 := $2") |
🤖 Prompt for AI Agents
In cmd/internal/migrations/v3/middleware_locals.go around lines 31 to 36, the
two regex replacements are too broad and may alter unrelated FromContext uses
(e.g., grpc/peer.FromContext); restrict the patterns to only match the specific
Fiber-related keys/functions you added (requestid, csrf, session, basicauth,
keyauth) by anchoring the regex to those identifiers (allowing minor whitespace
variations), so the first regex only strips type assertions when the expression
is like "(requestid|csrf|session|basicauth|keyauth)FromContext(...)" and the
second only collapses "v, ok := <same>FromContext(...)" for those names; update
both regexes accordingly and add a negative unit test that asserts
peer.FromContext (and another third-party FromContext) remains unchanged after
the transform.
1 participant
Summary
ContextKeyfrom CSRF config migrationTokenFromContextTesting
make lint(fails: the Go language version (go1.23) used to build golangci-lint is lower than the targeted Go version (1.25))make testhttps://chatgpt.com/codex/tasks/task_e_68a9d2363da083269901679cf359c323
Summary by CodeRabbit
• New Features
• Improved v3 migration produces cleaner CSRF configs by removing deprecated fields and standardizing timeouts and key lookups.
• Bug Fixes
• Ensures outdated CSRF settings are fully removed during migration, avoiding leftover, unused options.
• Refactor
• Post-processing now strips unnecessary type assertions and simplifies assignments in migrated middleware code for better readability and compatibility.
• Tests
• Expanded migration tests to validate CSRF field removals, timeout renaming, key lookup normalization, and middleware code cleanup.