🐛 fix: CSRF Cookie is removed when using Proxy Middleware #3390
Conversation
WalkthroughThe changes update the CSRF middleware to capture errors using a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CSRF_Middleware
participant Next_Middleware
participant Proxy_Handler
participant Target_Server
Client->>CSRF_Middleware: Sends request with potential token
CSRF_Middleware->>CSRF_Middleware: Store token in context
CSRF_Middleware->>Next_Middleware: Call c.Next()
Next_Middleware-->>CSRF_Middleware: Return control (token now in context)
CSRF_Middleware-->>CSRF_Middleware: Check token & update CSRF cookie, set Vary header
alt Proxy scenario
CSRF_Middleware->>Proxy_Handler: Forward request
Proxy_Handler->>Target_Server: Proxy request
Target_Server-->>Proxy_Handler: Return OK response
Proxy_Handler-->>CSRF_Middleware: Pass response back
end
CSRF_Middleware->>Client: Returns response, with captured error if any
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (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 ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3390 +/- ##
==========================================
+ Coverage 83.93% 83.95% +0.01%
==========================================
Files 119 119
Lines 11904 11914 +10
==========================================
+ Hits 9992 10002 +10
Misses 1484 1484
Partials 428 428
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:
|
gaby
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue where the CSRF cookie was removed when using the proxy middleware by updating the CSRF middleware logic and adding a dedicated test.
- Introduces Test_CSRF_With_Proxy_Middleware to verify that the CSRF cookie is correctly set after proxy handling.
- Adjusts the CSRF middleware to store the token before calling the next middleware and updates the cookie after c.Next() returns.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| middleware/csrf/csrf_test.go | Adds a new test to verify proper CSRF cookie behavior when using the proxy middleware. |
| middleware/csrf/csrf.go | Modifies the CSRF middleware to update the cookie after the next handler returns. |
| // Store the token in the context | ||
| c.Locals(tokenKey, token) | ||
| // Retrieve the final token from the context, if it was set. | ||
| finalToken, ok := c.Locals(tokenKey).(string) |
There was a problem hiding this comment.
Consider checking if err is non-nil immediately after c.Next() before updating the CSRF cookie with the final token. This may prevent unintended cookie updates when subsequent middleware fails.
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
There was a problem hiding this comment.
@gaby The logic of the code now disregards unexpected cookie updates on subsequent middleware failures, and I tried something like the following:
// Execute the next middleware or handler in the stack.
err = c.Next()
// If the next handler returned an error, return it immediately.
// Do not proceed to update the CSRF cookie if the request failed downstream.
if err != nil {
return err
}
// Retrieve the final token from the context, if it was set.
finalToken, ok := c.Locals(tokenKey).(string)
// Check if the token exists and is not empty.
if ok && finalToken != "" { // Ensure token exists
// Update the CSRF cookie in the response with the final token.
updateCSRFCookie(c, cfg, finalToken)
// Add the Vary: Cookie header to indicate that the response may differ
// based on the Cookie header, which is important for caching mechanisms.
// Tell the browser that a new header value is generated
c.Vary(fiber.HeaderCookie)
}
// Return any error that occurred during the execution of the next handlers.
// Continue stack
return err
Do not proceed to update the CSRF cookie if the request failed downstream. Will result in a large number of errors in existing unitary functions, Such an approach may require careful consideration
There was a problem hiding this comment.
To summarize, I'm going to backtrack on the handling of err. If we need to change the err logic here, we should open an additional separate issue
There was a problem hiding this comment.
Yeah remove it. Try adding a route that returns an error and see if the response still has csrf
There was a problem hiding this comment.
No need to fetch token from ctx.Locals after c.Next() — it's local and not mutated outside the package.
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue where the CSRF cookie was removed when requests passed through the proxy middleware. Key changes include the addition of two tests verifying that the CSRF cookie (and the session cookie) are correctly set in proxy scenarios and adjustments to the CSRF middleware logic to update the cookie after executing subsequent handlers.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| middleware/csrf/csrf_test.go | Adds tests to ensure that the CSRF (and session) cookies are correctly set via proxy. |
| middleware/csrf/csrf.go | Adjusts middleware logic to update the CSRF cookie after downstream handlers succeed. |
Comments suppressed due to low confidence (2)
middleware/csrf/csrf_test.go:1644
- [nitpick] The session cookie name is hard-coded to 'session_id'. Consider using a constant or configuration value from the session middleware to ensure consistency if the default name ever changes.
sessionCookieName := "session_id" // Default name for session.NewWithStore() unless configured otherwise
middleware/csrf/csrf_test.go:1624
- It would be beneficial to add a test case covering the scenario where the downstream handler (c.Next) returns an error, ensuring that the CSRF cookie is not updated when an error occurs.
require.NoError(t, err, "app.Test failed")
|
Will review tomorrow |
| // Store the token in the context | ||
| c.Locals(tokenKey, token) | ||
| // Retrieve the final token from the context, if it was set. | ||
| finalToken, ok := c.Locals(tokenKey).(string) |
There was a problem hiding this comment.
No need to fetch token from ctx.Locals after c.Next() — it's local and not mutated outside the package.
There was a problem hiding this comment.
❓ I'm not sure I see the point of adding CSRF protection at the proxy level. CSRF is meant to protect user-initiated browser requests to your app — applying it to a proxied backend call doesn't really make sense from a security perspective.
Can you clarify the rationale behind this?
hi, there. One can look at the scenario described by the user who initiated the issue : #3387 (comment) |
Description
This update introduces a new test,
Test_CSRF_With_Proxy_Middleware, to ensure that the CSRF cookie is correctly set even when requests are handled by the proxy middleware. Additionally, the CSRF middleware logic has been adjusted to update the CSRF cookie after the next handler is executed, improving its reliability in proxy scenarios.Fixes #3387