fix(auth): rotate session on login and invalidate on logout (#925)#1447
fix(auth): rotate session on login and invalidate on logout (#925)#1447krishankumar01 wants to merge 6 commits intomasterfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1447 +/- ##
========================================
Coverage 69.15% 69.15%
========================================
Files 362 364 +2
Lines 28315 28491 +176
========================================
+ Hits 19581 19703 +122
- Misses 7870 7916 +46
- Partials 864 872 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR aims to improve session-based authentication security by rotating the session on login and invalidating it on logout (addressing goravel/goravel#225).
Changes:
- Regenerate the session (destroying the old ID) during
SessionGuard.LoginUsingID. - Invalidate the session during
SessionGuard.Logout. - Update/add unit tests to assert
Regenerate(true)andInvalidate()are called and to cover error paths.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
auth/session_guard.go |
Regenerates session on login and invalidates session on logout. |
auth/session_guard_test.go |
Updates expectations for session rotation/invalidation and adds error-path tests. |
| if err := r.session.Regenerate(true); err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| r.session.Put(sessionName, key) | ||
|
|
There was a problem hiding this comment.
LoginUsingID regenerates the session ID, but the session cookie is currently written in session/middleware/start_session.go before req.Next() (StartSession sets the cookie from the pre-regenerate s.GetID()). That means the response will still contain the old session ID after Regenerate(true), so the rotated ID won’t reach the client (login may not persist, and the fixation mitigation is ineffective). Consider updating the session middleware to set/overwrite the session cookie after req.Next() (or otherwise ensure the cookie value is refreshed whenever Regenerate is called).
| func (r *SessionGuard) Logout() error { | ||
| sessionName := r.getSessionName() | ||
| r.session.Forget(sessionName) | ||
|
|
||
| return nil | ||
| return r.session.Invalidate() | ||
| } |
There was a problem hiding this comment.
Logout now calls session.Invalidate(), which also regenerates the session ID. With the current StartSession middleware writing the session cookie before req.Next(), the client may continue to receive/send the pre-invalidate session ID, so the new ID from Invalidate() won’t be propagated. This can lead to inconsistent logout behavior and undermines the intended invalidation. Align cookie-writing with post-handler session ID (e.g., write/overwrite the cookie after req.Next()).
| func (s *SessionGuardTestSuite) TestLoginUsingID_RegenerateError() { | ||
| s.mockSession.EXPECT().Regenerate(true).Return(assert.AnError).Once() | ||
|
|
||
| token, err := s.sessionGuard.LoginUsingID(1) | ||
| s.Empty(token) | ||
| s.ErrorIs(err, assert.AnError) | ||
| } |
There was a problem hiding this comment.
Test name TestLoginUsingID_RegenerateError breaks the file’s prevailing naming pattern (Test_<Something> with underscores, e.g. Test_Login, Test_InvalidKey). Renaming it to match the existing convention will keep the suite consistent and easier to scan/filter.
| if configFacade == nil { | ||
| return | ||
| } | ||
|
|
||
| r.ctx.Response().Cookie(http.Cookie{ |
There was a problem hiding this comment.
reissueSessionCookie() unconditionally calls r.ctx.Response().Cookie(...). If Response() can be nil (e.g., in custom/test contexts), this will panic. Consider guarding by assigning resp := r.ctx.Response() and returning early when resp == nil before calling resp.Cookie(...) (similar for r.ctx if needed).
| if configFacade == nil { | |
| return | |
| } | |
| r.ctx.Response().Cookie(http.Cookie{ | |
| if configFacade == nil || r.ctx == nil { | |
| return | |
| } | |
| resp := r.ctx.Response() | |
| if resp == nil { | |
| return | |
| } | |
| resp.Cookie(http.Cookie{ |
| r.ctx.Response().Cookie(http.Cookie{ | ||
| Name: r.session.GetName(), | ||
| Value: r.session.GetID(), | ||
| Expires: carbon.Now().AddMinutes(configFacade.GetInt("session.lifetime", 120)).StdTime(), | ||
| Path: configFacade.GetString("session.path"), | ||
| Domain: configFacade.GetString("session.domain"), | ||
| Secure: configFacade.GetBool("session.secure"), | ||
| HttpOnly: configFacade.GetBool("session.http_only"), | ||
| SameSite: configFacade.GetString("session.same_site"), | ||
| }) |
There was a problem hiding this comment.
Cookie construction here duplicates the logic in session/middleware/start_session.go (same config keys and fields). To reduce drift (e.g., if new cookie attributes are added later), consider factoring cookie-building into a shared helper (likely in the session package) and reusing it from both places.
📑 Description
Closes goravel/goravel#925
@coderabbitai summary
✅ Checks