Skip to content

Remove CSRF ContextKey and streamline TokenFromContext usage #169

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ReneWerner87 merged 1 commit into from
Aug 23, 2025
Merged

Remove CSRF ContextKey and streamline TokenFromContext usage #169

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions cmd/internal/migrations/v3/csrfconfig.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,14 @@ import (
func MigrateCSRFConfig(cmd *cobra.Command, cwd string, _, _ *semver.Version) error {
reConfig := regexp.MustCompile(`csrf\.Config{[^}]*}`)
reSession := regexp.MustCompile(`\s*SessionKey:\s*[^,]+,?\n`)
reContextKey := regexp.MustCompile(`\s*ContextKey:\s*[^,]+,?\n`)
reKeyLookup := regexp.MustCompile(`(\s*)KeyLookup:\s*([^,\n]+)(,?)(\n?)`)
changed, err := internal.ChangeFileContent(cwd, func(content string) string {
content = reConfig.ReplaceAllStringFunc(content, func(s string) string {
return strings.ReplaceAll(s, "Expiration:", "IdleTimeout:")
})
content = reSession.ReplaceAllString(content, "")
content = reContextKey.ReplaceAllString(content, "")

content = reKeyLookup.ReplaceAllStringFunc(content, func(s string) string {
sub := reKeyLookup.FindStringSubmatch(s)
Expand Down
2 changes: 2 additions & 0 deletions cmd/internal/migrations/v3/csrfconfig_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
var _ = csrf.New(csrf.Config{
Expiration: 10 * time.Minute,
SessionKey: "csrf",
ContextKey: "csrf",
})`)

var buf bytes.Buffer
Expand All @@ -36,6 +37,7 @@ var _ = csrf.New(csrf.Config{
assert.Contains(t, content, "IdleTimeout:")
assert.NotContains(t, content, "Expiration:")
assert.NotContains(t, content, "SessionKey")
assert.NotContains(t, content, "ContextKey")
assert.Contains(t, buf.String(), "Migrating CSRF middleware configs")
}

Expand Down
7 changes: 7 additions & 0 deletions cmd/internal/migrations/v3/middleware_locals.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,13 @@ func MigrateMiddlewareLocals(cmd *cobra.Command, cwd string, _, _ *semver.Versio
for _, r := range replacements {
content = r.re.ReplaceAllString(content, r.repl)
}

reTypeAssert := regexp.MustCompile(`([\w\.]+FromContext\([^\)]+\))\.\([^\)]+\)`)
content = reTypeAssert.ReplaceAllString(content, "$1")

reComma := regexp.MustCompile(`(\w+)\s*,\s*\w+\s*:=\s*([\w\.]+FromContext\([^\)]+\))`)
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Aug 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Suggested change
reComma := regexp.MustCompile(`(\w+)\s*,\s*\w+\s*:=\s*([\w\.]+FromContext\([^\)]+\))`)
reComma := regexp.MustCompile("(\\w+)\\s*,\\s*_\\s*:=\\s*([\\w\\.]+FromContext\\([^\\)]+\\))")

content = reComma.ReplaceAllString(content, "$1 := $2")

Comment on lines +31 to +36
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Aug 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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.

return content
})
if err != nil {
Expand Down
10 changes: 8 additions & 2 deletions cmd/internal/migrations/v3/middleware_locals_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,12 @@ func Test_MigrateMiddlewareLocals(t *testing.T) {
file := writeTempFile(t, dir, `package main
import "github.com/gofiber/fiber/v2"
func handler(c fiber.Ctx) error {
id := c.Locals("requestid")
id := c.Locals("requestid").(string)
csrfToken, _ := c.Locals("csrf").(string)
token := c.Locals("token").(string)
_ = id
_ = csrfToken
_ = token
return nil
}`)

Expand All @@ -31,6 +35,8 @@ func handler(c fiber.Ctx) error {
require.NoError(t, v3.MigrateMiddlewareLocals(cmd, dir, nil, nil))

content := readFile(t, file)
assert.Contains(t, content, `requestid.FromContext(c)`)
assert.Contains(t, content, `id := requestid.FromContext(c)`)
assert.Contains(t, content, `csrfToken := csrf.TokenFromContext(c)`)
assert.Contains(t, content, `token := keyauth.TokenFromContext(c)`)
assert.Contains(t, buf.String(), "Migrating middleware locals")
}
Loading