Skip to content

fix(auth): rotate session on login and invalidate on logout (#925) #1447

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

Draft
krishankumar01 wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

fix(auth): rotate session on login and invalidate on logout (#925) #1447

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0e0aee4
fix(auth): rotate session on login and invalidate on logout (#925)
krishankumar01 Apr 20, 2026
d3b7d50
write session cookie after handler so rotations reach client
krishankumar01 Apr 20, 2026
3325476
reissue session cookie from guard for gin compatibility
krishankumar01 Apr 20, 2026
0a86835
skip middleware cookie write when client already has matching id
krishankumar01 Apr 20, 2026
ed3267d
refactor(session): extract shared cookie writer
krishankumar01 Apr 22, 2026
b2739a3
session: emit cookie from middleware only
krishankumar01 Apr 26, 2026
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
9 changes: 5 additions & 4 deletions auth/session_guard.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,16 +77,17 @@ func (r *SessionGuard) LoginUsingID(id any) (token string, err error) {
return "", errors.AuthInvalidKey
}

if err := r.session.Regenerate(true); err != nil {
return "", err
}

r.session.Put(sessionName, key)

Comment on lines +80 to 85
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

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

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).

Copilot uses AI. Check for mistakes.
return "", nil
}

func (r *SessionGuard) Logout() error {
sessionName := r.getSessionName()
r.session.Forget(sessionName)

return nil
return r.session.Invalidate()
}

func (r *SessionGuard) Parse(token string) (*contractsauth.Payload, error) {
Expand Down
22 changes: 19 additions & 3 deletions auth/session_guard_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ func (s *SessionGuardTestSuite) TestCheck_LoginUsingID_Logout() {
s.False(s.sessionGuard.Check())
s.True(s.sessionGuard.Guest())

s.mockSession.EXPECT().Regenerate(true).Return(nil).Once()
s.mockSession.EXPECT().Put("auth_user_id", "1").Return(nil).Once()
token, err := s.sessionGuard.LoginUsingID(1)
s.Nil(err)
Expand All @@ -116,7 +117,7 @@ func (s *SessionGuardTestSuite) TestCheck_LoginUsingID_Logout() {
s.True(s.sessionGuard.Check())
s.False(s.sessionGuard.Guest())

s.mockSession.EXPECT().Forget("auth_user_id").Return(nil).Once()
s.mockSession.EXPECT().Invalidate().Return(nil).Once()
s.NoError(s.sessionGuard.Logout())

s.mockSession.EXPECT().Get("auth_user_id", nil).Return(nil).Once()
Expand All @@ -133,6 +134,7 @@ func (s *SessionGuardTestSuite) Test_Login() {
user.Name = "Goravel"

s.mockUserProvider.EXPECT().GetID(&user).Return("2", nil).Once()
s.mockSession.EXPECT().Regenerate(true).Return(nil).Once()
s.mockSession.EXPECT().Put("auth_user_id", "2").Return(nil).Once()
token, err := s.sessionGuard.Login(&user)
s.Nil(err)
Expand All @@ -142,7 +144,7 @@ func (s *SessionGuardTestSuite) Test_Login() {
s.True(s.sessionGuard.Check())
s.False(s.sessionGuard.Guest())

s.mockSession.EXPECT().Forget("auth_user_id").Return(nil).Once()
s.mockSession.EXPECT().Invalidate().Return(nil).Once()
s.NoError(s.sessionGuard.Logout())

s.mockSession.EXPECT().Get("auth_user_id", nil).Return(nil).Once()
Expand All @@ -167,7 +169,7 @@ func (s *SessionGuardTestSuite) Test_LoginFailed() {
s.False(s.sessionGuard.Check())
s.True(s.sessionGuard.Guest())

s.mockSession.EXPECT().Forget("auth_user_id").Return(nil).Once()
s.mockSession.EXPECT().Invalidate().Return(nil).Once()
s.NoError(s.sessionGuard.Logout())

s.mockSession.EXPECT().Get("auth_user_id", nil).Return(nil).Once()
Expand Down Expand Up @@ -217,3 +219,17 @@ func (s *SessionGuardTestSuite) Test_InvalidKey() {
s.NotNil(err)
s.ErrorIs(err, errors.AuthInvalidKey)
}

func (s *SessionGuardTestSuite) Test_LoginUsingID_RegenerateError() {
s.mockSession.EXPECT().Regenerate(true).Return(assert.AnError).Once()

token, err := s.sessionGuard.LoginUsingID(1)
s.Empty(token)
s.ErrorIs(err, assert.AnError)
}

func (s *SessionGuardTestSuite) Test_Logout_InvalidateError() {
s.mockSession.EXPECT().Invalidate().Return(assert.AnError).Once()

s.ErrorIs(s.sessionGuard.Logout(), assert.AnError)
}
43 changes: 43 additions & 0 deletions session/cookie.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
package session

import (
"github.com/goravel/framework/contracts/http"
"github.com/goravel/framework/support/carbon"
)

// WriteCookie emits the current session ID as a Set-Cookie header on the
// response, using the standard session.* config keys for cookie
// attributes. The StartSession middleware uses it to issue the session
// cookie, and application code can call it after rotating the session
// ID (for example, after auth.Login or auth.Logout) so the rotated ID
// reaches the client. Safe to call with a partially initialised
// context — returns without effect if the context, response, session,
// or config facade is nil.
func WriteCookie(ctx http.Context) {
if ctx == nil || ConfigFacade == nil {
return
}
req := ctx.Request()
if req == nil {
return
}
s := req.Session()
if s == nil {
return
}
resp := ctx.Response()
if resp == nil {
return
}

resp.Cookie(http.Cookie{
Name: s.GetName(),
Value: s.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"),
})
}
13 changes: 1 addition & 12 deletions session/middleware/start_session.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package middleware
import (
"github.com/goravel/framework/contracts/http"
"github.com/goravel/framework/session"
"github.com/goravel/framework/support/carbon"
"github.com/goravel/framework/support/color"
)

Expand Down Expand Up @@ -40,17 +39,7 @@ func StartSession() http.Middleware {
req.SetSession(s)

// Set session cookie in response
config := session.ConfigFacade
ctx.Response().Cookie(http.Cookie{
Name: s.GetName(),
Value: s.GetID(),
Expires: carbon.Now().AddMinutes(config.GetInt("session.lifetime", 120)).StdTime(),
Path: config.GetString("session.path"),
Domain: config.GetString("session.domain"),
Secure: config.GetBool("session.secure"),
HttpOnly: config.GetBool("session.http_only"),
SameSite: config.GetString("session.same_site"),
})
session.WriteCookie(ctx)

// Continue processing request
req.Next()
Expand Down
14 changes: 7 additions & 7 deletions session/middleware/start_session_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,13 @@ func testHttpSessionMiddleware(next nethttp.Handler, mockConfig *configmocks.Con
}

func mockConfigFacade(mockConfig *configmocks.Config) {
mockConfig.On("GetString", "session.default").Return("file").Once()
mockConfig.On("GetInt", "session.lifetime", 120).Return(120).Once()
mockConfig.On("GetString", "session.path").Return("/").Once()
mockConfig.On("GetString", "session.domain").Return("").Once()
mockConfig.On("GetBool", "session.secure").Return(false).Once()
mockConfig.On("GetBool", "session.http_only").Return(true).Once()
mockConfig.On("GetString", "session.same_site").Return("").Once()
mockConfig.EXPECT().GetString("session.default").Return("file").Once()
mockConfig.EXPECT().GetInt("session.lifetime", 120).Return(120).Once()
mockConfig.EXPECT().GetString("session.path").Return("/").Once()
mockConfig.EXPECT().GetString("session.domain").Return("").Once()
mockConfig.EXPECT().GetBool("session.secure").Return(false).Once()
mockConfig.EXPECT().GetBool("session.http_only").Return(true).Once()
mockConfig.EXPECT().GetString("session.same_site").Return("").Once()
}

func TestStartSession(t *testing.T) {
Expand Down
Loading