🔥 feat: Add context.Context support to CSRF middleware#3340
🔥 feat: Add context.Context support to CSRF middleware#3340JIeJaitt wants to merge 9 commits intogofiber:mainfrom
Conversation
WalkthroughThe changes update the CSRF middleware to support a generic context parameter. The functions Changes
Sequence Diagram(s)sequenceDiagram
participant Req as Request
participant CSRF as CSRF Middleware
participant Ctx as Context (fiber.Ctx / context.Context)
participant Log as Logger
Req->>CSRF: Incoming Request
CSRF->>Ctx: Retrieve token and handler
alt Valid fiber.Ctx or context.Context
Ctx-->>CSRF: Return valid token and handler
else Unsupported type
CSRF->>Log: Log error message
Log-->>CSRF: Error logged
end
CSRF->>Req: Process request with retrieved token/handler
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
Note 🎁 Summarized by CodeRabbit FreeYour organization has reached its limit of developer seats under the Pro Plan. For new users, CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please add seats to your subscription by visiting https://app.coderabbit.ai/login.If you believe this is a mistake and have available seats, please assign one to the pull request author through the subscription management page using the link above. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3340 +/- ##
==========================================
- Coverage 83.61% 83.61% -0.01%
==========================================
Files 118 118
Lines 11727 11746 +19
==========================================
+ Hits 9806 9821 +15
- Misses 1491 1495 +4
Partials 430 430
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:
|
|
@ReneWerner87 @gaby hi there. how can i fix this lint error? |
|
@JIeJaitt I'm fixing it in a branch. It randomly broke without us updating the version |
| ctx := context.WithValue(c.Context(), tokenKey, token) | ||
| ctx = context.WithValue(ctx, handlerKey, handler) |
There was a problem hiding this comment.
I'm unsure I understand the benefit of retaining the references in both c.Context and c.Locals. Given that the UserContext used is always linked to the ctx.
There was a problem hiding this comment.
When the developer controller layer will fiber.Ctx converted to context.Context, in the service layer will have a lot of inconvenience when using, for example, requestid can not be directly inherited from the ctx, can only rely on the external incoming ... This is the initial idea, other middleware, perhaps to maintain the consistency of the middleware fiber framework it
There was a problem hiding this comment.
I’m still not sure I understand. Could you perhaps provide a code example demonstrating the issue you are seeking to address?
There was a problem hiding this comment.
Fiber's fiber.Ctx is specific to the Fiber framework. If we only store CSRF information in fiber.Ctx.Locals, it will not be accessible when we switch to Go's native context.Context in the service layer or other parts of the application.
Go's native context.Context is widely used across libraries and frameworks. By injecting CSRF information into the native context, we ensure that it can be accessed seamlessly in the service layer or any other part of the application that relies on context.Context.
code example like this:
.
├── main.go
├── controllers
│ └── user_controller.go
├── services
│ └── user_service.go
├── middlewares
│ └── csrf_middleware.go
└── utils
└── context_utils.go
Controller Layer (controllers/user_controller.go)
package controllers
import (
"context"
"github.com/gofiber/fiber/v2"
"your_project/services"
)
type UserController struct {
userService *services.UserService
}
func NewUserController(userService *services.UserService) *UserController {
return &UserController{userService: userService}
}
func (uc *UserController) CreateUser(c *fiber.Ctx) error {
// Convert Fiber context to native context
ctx := c.Context()
// Call service layer with native context
err := uc.userService.CreateUser(ctx)
if err != nil {
return c.Status(fiber.StatusInternalServerError).JSON(fiber.Map{
"error": err.Error(),
})
}
return c.Status(fiber.StatusOK).JSON(fiber.Map{
"message": "User created successfully",
})
}package services
import (
"context"
"fmt"
)
type UserService struct{}
func NewUserService() *UserService {
return &UserService{}
}
func (us *UserService) CreateUser(ctx context.Context) error {
// Retrieve CSRF token from context
csrfToken := ctx.Value("csrf").(string)
// Use CSRF token in the service logic
fmt.Println("CSRF Token:", csrfToken)
// Business logic here...
return nil
}If we don't inject a csrf into the context, then we can't get the csrf information at the service level unless we pass it in as a parameter manually, but this is obviously not that elegant.
There was a problem hiding this comment.
The assumption here is incorrect—Fiber’s CSRF token is not stored in context.Context using a string key like "csrf". Instead, it is stored in c.Locals with an unexported key, and the correct way to retrieve it is:
csrfToken := csrf.TokenFromContext(c)Even in the PR proposed approach, the token is still being redundantly stored in context.Context using an unexported key. This means the service layer must be aware of the Fiber middleware and use csrf.TokenFromContext to retrieve the token—making the change ineffective in addressing the issue.
sixcolors
left a comment
There was a problem hiding this comment.
Thanks for your contribution! However, I do not support merging this change due to the reasons outlined in issue #3358.
Fiber is designed to be lightweight and high-performance, and modifying context.Context within middleware introduces unnecessary overhead.
If you need context.Context integration, you can implement a custom middleware to copy values into context.Context for your specific use case without affecting Fiber’s default behavior.
For more details, see: this comment.
I recommend closing this PR. Thanks again! 🚀
thinks，I've already understood the whole working process and I will close this pr |
In order to maintain the consistency of the fiber middleware, this pr is proposed to add the corresponding function mentioned in #3212 to the CSRF middleware.