fix: add CSRF protection for cookie-based auth endpoints (#148)#220
fix: add CSRF protection for cookie-based auth endpoints (#148)#220Itsssaksham wants to merge 2 commits into
Conversation
|
Warning Review limit reached
More reviews will be available in 46 minutes and 32 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses rolling per-developer review limits. Reviews become available again as older review attempts age out of the rolling limit window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a custom CSRF protection system to the Gin-based auth server. A new ChangesCSRF Protection Implementation
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser (Cookie Client)
participant Routes as Gin Router + CSRFMiddleware
participant SetCSRFTokenHandler
participant AuthHandler
Browser->>Routes: GET /api/auth/csrf-token
Routes->>SetCSRFTokenHandler: bypass CSRF (safe method)
SetCSRFTokenHandler-->>Browser: Set-Cookie: csrf_token=<token>, body: {csrf_token}
Browser->>Routes: POST /api/auth/login (Cookie: csrf_token, X-CSRF-Token: <token>)
Routes->>Routes: CSRFMiddleware: cookie present, compare header
alt tokens match
Routes->>AuthHandler: c.Next()
AuthHandler->>AuthHandler: SetCookie auth_token
AuthHandler->>AuthHandler: RotateCSRFToken → new csrf_token cookie
AuthHandler-->>Browser: 200 login success
else tokens mismatch or header absent
Routes-->>Browser: 403 Forbidden
end
Browser->>Routes: POST /api/auth/logout (Cookie: csrf_token, X-CSRF-Token: <token>)
Routes->>Routes: CSRFMiddleware: validate
Routes->>AuthHandler: c.Next()
AuthHandler->>AuthHandler: RotateCSRFToken → new csrf_token cookie
AuthHandler-->>Browser: 200 logout success
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
All contributors have signed the CLA. ✅ Thank you! |
|
I have read the CLA and agree to its terms. |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/handler/auth_handler.go (1)
346-358: 🔒 Security & Privacy | 🟠 MajorAdd CSRF token rotation and session cookie to LoginMFA after successful MFA verification.
The
LoginMFAhandler (line 992) returns a success response without rotating the CSRF token or setting the session cookie. This creates a security gap: after the user completes MFA and receives new tokens, the CSRF token should be rotated (consistent with theLoginhandler's non-MFA branch at line 358) and the session cookie should be set. Addc.SetCookie("auth_token", ...)andmiddleware.RotateCSRFToken(c)before the final success response inLoginMFA.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/handler/auth_handler.go` around lines 346 - 358, The LoginMFA handler is missing critical security operations that are present in the Login handler. In the LoginMFA handler (around line 992), before returning the final success response, add two operations: first, call c.SetCookie to set the auth_token session cookie with the same parameters used in the Login handler (auth_token, loginResp.AccessToken, 7*24*3600 for MaxAge, "/" for path, empty domain, false for secure flag, and true for httpOnly), and second, call middleware.RotateCSRFToken(c) to rotate the CSRF token after successful MFA verification, matching the security posture of the non-MFA login flow.
🧹 Nitpick comments (2)
internal/middleware/csrf_test.go (2)
25-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding tests for HEAD and OPTIONS methods for completeness.
The middleware allows HEAD and OPTIONS as safe methods alongside GET (per the implementation), but only GET is tested. Adding tests for these methods would provide complete coverage of the safe-method pass-through behavior.
📋 Example tests to add
func TestCSRFMiddleware_HEADMethodAllowed(t *testing.T) { r := setupCSRFRouter() w := httptest.NewRecorder() req, _ := http.NewRequest(http.MethodHead, "/test", nil) r.ServeHTTP(w, req) assert.Equal(t, http.StatusOK, w.Code) } func TestCSRFMiddleware_OPTIONSMethodAllowed(t *testing.T) { r := setupCSRFRouter() w := httptest.NewRecorder() req, _ := http.NewRequest(http.MethodOptions, "/test", nil) r.ServeHTTP(w, req) assert.Equal(t, http.StatusOK, w.Code) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/middleware/csrf_test.go` around lines 25 - 31, The test file only covers GET method for safe method pass-through in TestCSRFMiddleware_SafeMethodsAllowed, but the middleware implementation also allows HEAD and OPTIONS methods as safe. Add two new test functions following the same pattern as TestCSRFMiddleware_SafeMethodsAllowed: create TestCSRFMiddleware_HEADMethodAllowed that uses http.MethodHead and TestCSRFMiddleware_OPTIONSMethodAllowed that uses http.MethodOptions, each verifying that requests with these methods are allowed (return http.StatusOK) by the CSRF middleware without requiring a token.
12-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding tests for PUT and DELETE methods to match acceptance criteria.
The PR objectives explicitly state: "All cookie-based state-changing requests using POST, PUT, or DELETE methods must include a valid
X-CSRF-Tokenheader." Current tests only cover POST scenarios. While the middleware logic is method-agnostic for non-safe methods, adding explicit tests for PUT and DELETE would:
- Validate the acceptance criteria directly
- Prevent future regressions if method-specific logic is added
- Improve documentation of intended behavior
📋 Example tests to add
func TestCSRFMiddleware_PUTRequiresToken(t *testing.T) { gin.SetMode(gin.TestMode) r := gin.New() r.Use(CSRFMiddleware()) r.PUT("/test", func(c *gin.Context) { c.JSON(http.StatusOK, gin.H{"status": "ok"}) }) // Missing token w := httptest.NewRecorder() req, _ := http.NewRequest(http.MethodPut, "/test", nil) req.AddCookie(&http.Cookie{Name: "csrf_token", Value: "token123"}) r.ServeHTTP(w, req) assert.Equal(t, http.StatusForbidden, w.Code) // Valid token w = httptest.NewRecorder() req, _ = http.NewRequest(http.MethodPut, "/test", nil) req.AddCookie(&http.Cookie{Name: "csrf_token", Value: "token123"}) req.Header.Set("X-CSRF-Token", "token123") r.ServeHTTP(w, req) assert.Equal(t, http.StatusOK, w.Code) } func TestCSRFMiddleware_DELETERequiresToken(t *testing.T) { gin.SetMode(gin.TestMode) r := gin.New() r.Use(CSRFMiddleware()) r.DELETE("/test", func(c *gin.Context) { c.JSON(http.StatusOK, gin.H{"status": "ok"}) }) // Missing token w := httptest.NewRecorder() req, _ := http.NewRequest(http.MethodDelete, "/test", nil) req.AddCookie(&http.Cookie{Name: "csrf_token", Value: "token123"}) r.ServeHTTP(w, req) assert.Equal(t, http.StatusForbidden, w.Code) // Valid token w = httptest.NewRecorder() req, _ = http.NewRequest(http.MethodDelete, "/test", nil) req.AddCookie(&http.Cookie{Name: "csrf_token", Value: "token123"}) req.Header.Set("X-CSRF-Token", "token123") r.ServeHTTP(w, req) assert.Equal(t, http.StatusOK, w.Code) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/middleware/csrf_test.go` around lines 12 - 79, Add two new test functions following the existing pattern to cover PUT and DELETE HTTP methods, which are mentioned in the acceptance criteria but currently not tested. Create TestCSRFMiddleware_PUTRequiresToken and TestCSRFMiddleware_DELETERequiresToken that each register the respective HTTP method in the router using setupCSRFRouter or similar setup, then verify both the failure case (request with csrf_token cookie but missing X-CSRF-Token header returns StatusForbidden) and the success case (request with matching csrf_token cookie and X-CSRF-Token header returns StatusOK), mirroring the structure of existing POST-based tests like TestCSRFMiddleware_MissingHeader and TestCSRFMiddleware_ValidToken.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/handler/auth_handler.go`:
- Around line 354-355: The auth_token cookie has two security issues that need
to be fixed. First, in the SetCookie call for auth_token, change the secure
parameter from false to true to match the CSRF cookie security policy. Second,
the Logout function and LogoutAll function both fail to clear the auth_token
cookie when logging out, leaving it in the browser as a security risk. In both
the Logout and LogoutAll functions, add SetCookie calls to explicitly clear the
auth_token by setting MaxAge=-1 and ensuring the path is "/" (matching where it
was set), domain is "" (empty string), and secure is true (matching the updated
setting from the first fix).
In `@internal/middleware/csrf.go`:
- Around line 13-17: The CSRF validation logic around line 72 currently treats a
missing csrf_token cookie as a sign of bearer flow authentication and skips CSRF
validation. However, since the auth_token cookie has a longer lifetime than the
24-hour CSRF cookie expiration defined in csrfCookieMaxAge, cookie-authenticated
requests can bypass CSRF protection once the CSRF token expires. Modify the CSRF
validation logic to check if the auth_token cookie is present and, if so,
require CSRF token validation regardless of whether the csrf_token cookie is
missing. Only skip CSRF validation when using true bearer token authentication,
not when the request is authenticated via the auth_token cookie.
- Around line 99-104: The RotateCSRFToken function silently returns when token
generation fails, allowing callers to proceed without knowing the rotation
failed. Change the function signature to return an error type, then return the
error when generateCSRFToken fails instead of silently returning, and ensure
callers of RotateCSRFToken check the returned error and fail the response
appropriately before reporting the auth state change as successful.
In `@internal/routes/routes.go`:
- Line 86: The CSRFMiddleware function currently allows state-changing requests
without CSRF tokens when the csrf_token is absent, which creates a security
vulnerability for cookie-authenticated requests. Modify the CSRFMiddleware logic
to only bypass CSRF checks for actual Bearer token authorization (check for
Authorization header with Bearer prefix), and require CSRF token validation for
all state-changing requests when an auth_token session cookie is present. This
ensures that cookie-based authentication cannot bypass CSRF protection.
---
Outside diff comments:
In `@internal/handler/auth_handler.go`:
- Around line 346-358: The LoginMFA handler is missing critical security
operations that are present in the Login handler. In the LoginMFA handler
(around line 992), before returning the final success response, add two
operations: first, call c.SetCookie to set the auth_token session cookie with
the same parameters used in the Login handler (auth_token,
loginResp.AccessToken, 7*24*3600 for MaxAge, "/" for path, empty domain, false
for secure flag, and true for httpOnly), and second, call
middleware.RotateCSRFToken(c) to rotate the CSRF token after successful MFA
verification, matching the security posture of the non-MFA login flow.
---
Nitpick comments:
In `@internal/middleware/csrf_test.go`:
- Around line 25-31: The test file only covers GET method for safe method
pass-through in TestCSRFMiddleware_SafeMethodsAllowed, but the middleware
implementation also allows HEAD and OPTIONS methods as safe. Add two new test
functions following the same pattern as TestCSRFMiddleware_SafeMethodsAllowed:
create TestCSRFMiddleware_HEADMethodAllowed that uses http.MethodHead and
TestCSRFMiddleware_OPTIONSMethodAllowed that uses http.MethodOptions, each
verifying that requests with these methods are allowed (return http.StatusOK) by
the CSRF middleware without requiring a token.
- Around line 12-79: Add two new test functions following the existing pattern
to cover PUT and DELETE HTTP methods, which are mentioned in the acceptance
criteria but currently not tested. Create TestCSRFMiddleware_PUTRequiresToken
and TestCSRFMiddleware_DELETERequiresToken that each register the respective
HTTP method in the router using setupCSRFRouter or similar setup, then verify
both the failure case (request with csrf_token cookie but missing X-CSRF-Token
header returns StatusForbidden) and the success case (request with matching
csrf_token cookie and X-CSRF-Token header returns StatusOK), mirroring the
structure of existing POST-based tests like TestCSRFMiddleware_MissingHeader and
TestCSRFMiddleware_ValidToken.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 683e2f9e-6a32-4d79-9e8a-ad5a08f5d7a8
📒 Files selected for processing (4)
internal/handler/auth_handler.gointernal/middleware/csrf.gointernal/middleware/csrf_test.gointernal/routes/routes.go
| // MaxAge is 7 days (matching refresh token) | ||
| c.SetCookie("auth_token", loginResp.AccessToken, 7*24*3600, "/", "", false, true) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect all auth_token cookie writers for secure flag consistency and logout clearing.
# Expect: login uses a secure/configured cookie policy, and Logout/LogoutAll expire auth_token with matching attributes.
rg -n -C4 'SetCookie\("auth_token"' internal
rg -n -C8 'func \(h \*AuthHandler\) Logout|func \(h \*AuthHandler\) LogoutAll' internal/handler/auth_handler.goRepository: roshankumar0036singh/auth-server
Length of output: 1864
🏁 Script executed:
#!/bin/bash
# Get the complete auth_handler.go file to see all cookie operations
# First, let's see the file size
wc -l internal/handler/auth_handler.go
# Read lines 403-428 (full Logout function)
sed -n '403,428p' internal/handler/auth_handler.go
# Read lines 437-465 (full LogoutAll function)
sed -n '437,470p' internal/handler/auth_handler.goRepository: roshankumar0036singh/auth-server
Length of output: 1827
🏁 Script executed:
#!/bin/bash
# Search for RotateCSRFToken implementation to understand CSRF cookie secure flag
rg -n 'RotateCSRFToken|SetCookie.*csrf' internal/middleware/Repository: roshankumar0036singh/auth-server
Length of output: 548
🏁 Script executed:
#!/bin/bash
# Search for any cookie helper functions or middleware
rg -n 'SetCookie' internal --type go | head -20Repository: roshankumar0036singh/auth-server
Length of output: 1141
Clear and secure the auth_token cookie consistently across login and logout.
The auth_token cookie is set with secure=false (line 355), while the CSRF cookie uses secure=true (csrf.go:104). Additionally, both Logout (line 403+) and LogoutAll (line 437+) only rotate the CSRF token and do not clear the auth_token cookie—leaving it in the browser after logout, a security risk.
Set secure=true on auth_token to match CSRF policy, and expire auth_token on both logout paths using MaxAge=-1 with matching path (/), domain (""), and secure flag attributes.
Suggested fixes
Line 355 – Set auth_token with secure flag:
- c.SetCookie("auth_token", loginResp.AccessToken, 7*24*3600, "/", "", false, true)
+ c.SetCookie("auth_token", loginResp.AccessToken, 7*24*3600, "/", "", true, true)Logout function – Clear auth_token on logout:
// Rotate CSRF token on logout
+ c.SetCookie("auth_token", "", -1, "/", "", true, true)
middleware.RotateCSRFToken(c)LogoutAll function – Clear auth_token on logout-all:
// Rotate CSRF token on logout-all
+ c.SetCookie("auth_token", "", -1, "/", "", true, true)
middleware.RotateCSRFToken(c)📝 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.
| // MaxAge is 7 days (matching refresh token) | |
| c.SetCookie("auth_token", loginResp.AccessToken, 7*24*3600, "/", "", false, true) | |
| // MaxAge is 7 days (matching refresh token) | |
| c.SetCookie("auth_token", loginResp.AccessToken, 7*24*3600, "/", "", true, true) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/handler/auth_handler.go` around lines 354 - 355, The auth_token
cookie has two security issues that need to be fixed. First, in the SetCookie
call for auth_token, change the secure parameter from false to true to match the
CSRF cookie security policy. Second, the Logout function and LogoutAll function
both fail to clear the auth_token cookie when logging out, leaving it in the
browser as a security risk. In both the Logout and LogoutAll functions, add
SetCookie calls to explicitly clear the auth_token by setting MaxAge=-1 and
ensuring the path is "/" (matching where it was set), domain is "" (empty
string), and secure is true (matching the updated setting from the first fix).
| const ( | ||
| csrfTokenLength = 32 | ||
| csrfCookieName = "csrf_token" | ||
| csrfHeaderName = "X-CSRF-Token" | ||
| csrfCookieMaxAge = int(24 * time.Hour / time.Second) // 24 hours |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Require CSRF when the auth cookie is present, even if the CSRF cookie is missing.
Line 72 currently treats a missing csrf_token as bearer flow. But the browser session cookie is auth_token, and it lives longer than this 24-hour CSRF cookie, so cookie-authenticated POST/PUT/DELETE requests can bypass CSRF validation once csrf_token is absent.
🛡️ Proposed fix
const (
+ authCookieName = "auth_token"
csrfTokenLength = 32
csrfCookieName = "csrf_token"
csrfHeaderName = "X-CSRF-Token"
csrfCookieMaxAge = int(24 * time.Hour / time.Second) // 24 hours
)
@@
- // Only enforce CSRF for cookie-based sessions.
- // Bearer token requests skip this middleware.
- cookieToken, err := c.Cookie(csrfCookieName)
- if err != nil || cookieToken == "" {
- // No cookie present — Bearer token flow, skip
+ // Only enforce CSRF for cookie-based sessions.
+ // Bearer token requests skip this middleware.
+ authToken, err := c.Cookie(authCookieName)
+ if err != nil || authToken == "" {
+ // No auth cookie present — Bearer token flow, skip
c.Next()
return
}
+
+ cookieToken, err := c.Cookie(csrfCookieName)
+ if err != nil || cookieToken == "" {
+ c.AbortWithStatusJSON(http.StatusForbidden, utils.ForbiddenResponse(
+ "Missing CSRF token cookie",
+ ))
+ return
+ }Also applies to: 69-75
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/middleware/csrf.go` around lines 13 - 17, The CSRF validation logic
around line 72 currently treats a missing csrf_token cookie as a sign of bearer
flow authentication and skips CSRF validation. However, since the auth_token
cookie has a longer lifetime than the 24-hour CSRF cookie expiration defined in
csrfCookieMaxAge, cookie-authenticated requests can bypass CSRF protection once
the CSRF token expires. Modify the CSRF validation logic to check if the
auth_token cookie is present and, if so, require CSRF token validation
regardless of whether the csrf_token cookie is missing. Only skip CSRF
validation when using true bearer token authentication, not when the request is
authenticated via the auth_token cookie.
| func RotateCSRFToken(c *gin.Context) { | ||
| token, err := generateCSRFToken() | ||
| if err != nil { | ||
| return | ||
| } | ||
| c.SetCookie(csrfCookieName, token, csrfCookieMaxAge, "/", "", true, false) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not silently skip CSRF rotation failures.
RotateCSRFToken fails open when token generation errors, so login/logout handlers can return success without rotating the CSRF token. Return the error and have callers fail the response before reporting the auth state change as successful.
🛡️ Proposed fix
-func RotateCSRFToken(c *gin.Context) {
+func RotateCSRFToken(c *gin.Context) error {
token, err := generateCSRFToken()
if err != nil {
- return
+ return err
}
c.SetCookie(csrfCookieName, token, csrfCookieMaxAge, "/", "", true, false)
+ return nil
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/middleware/csrf.go` around lines 99 - 104, The RotateCSRFToken
function silently returns when token generation fails, allowing callers to
proceed without knowing the rotation failed. Change the function signature to
return an error type, then return the error when generateCSRFToken fails instead
of silently returning, and ensure callers of RotateCSRFToken check the returned
error and fail the response appropriately before reporting the auth state change
as successful.
| // Apply global middleware | ||
| router.Use(middleware.CORSMiddleware(cfg)) | ||
| router.Use(middleware.SecurityMiddleware()) | ||
| router.Use(middleware.CSRFMiddleware()) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Enforce CSRF based on the auth cookie, not the CSRF cookie.
CSRFMiddleware currently skips state-changing requests when csrf_token is absent. Since login sets an auth_token cookie, a cookie-auth POST/PUT/DELETE without csrf_token would pass this global middleware instead of failing the required X-CSRF-Token check. Gate the bearer bypass on an actual Authorization: Bearer ... request, and require CSRF whenever the session cookie is present.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/routes/routes.go` at line 86, The CSRFMiddleware function currently
allows state-changing requests without CSRF tokens when the csrf_token is
absent, which creates a security vulnerability for cookie-authenticated
requests. Modify the CSRFMiddleware logic to only bypass CSRF checks for actual
Bearer token authorization (check for Authorization header with Bearer prefix),
and require CSRF token validation for all state-changing requests when an
auth_token session cookie is present. This ensures that cookie-based
authentication cannot bypass CSRF protection.
1 participant
Closes #148
Summary by CodeRabbit
New Features
Tests