From 0e0aee4d63e92ce6c0fc33db2cada0cfbee6620c Mon Sep 17 00:00:00 2001 From: kkumar-gcc Date: Mon, 20 Apr 2026 05:54:09 +0530 Subject: [PATCH 1/6] fix(auth): rotate session on login and invalidate on logout (#925) --- auth/session_guard.go | 9 +++++---- auth/session_guard_test.go | 22 +++++++++++++++++++--- 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/auth/session_guard.go b/auth/session_guard.go index 68b5d46f9..fe9169750 100644 --- a/auth/session_guard.go +++ b/auth/session_guard.go @@ -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) 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) { diff --git a/auth/session_guard_test.go b/auth/session_guard_test.go index 5af5b13dd..ad0162511 100644 --- a/auth/session_guard_test.go +++ b/auth/session_guard_test.go @@ -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) @@ -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() @@ -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) @@ -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() @@ -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() @@ -217,3 +219,17 @@ func (s *SessionGuardTestSuite) Test_InvalidKey() { s.NotNil(err) s.ErrorIs(err, errors.AuthInvalidKey) } + +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) +} + +func (s *SessionGuardTestSuite) TestLogout_InvalidateError() { + s.mockSession.EXPECT().Invalidate().Return(assert.AnError).Once() + + s.ErrorIs(s.sessionGuard.Logout(), assert.AnError) +} From d3b7d50a8711ba5a0e1bfc2be18f964fbbb64705 Mon Sep 17 00:00:00 2001 From: kkumar-gcc Date: Mon, 20 Apr 2026 06:45:39 +0530 Subject: [PATCH 2/6] write session cookie after handler so rotations reach client --- auth/session_guard_test.go | 4 +-- session/middleware/start_session.go | 9 ++++--- session/middleware/start_session_test.go | 32 ++++++++++++++++++++++++ 3 files changed, 39 insertions(+), 6 deletions(-) diff --git a/auth/session_guard_test.go b/auth/session_guard_test.go index ad0162511..3ad339e0d 100644 --- a/auth/session_guard_test.go +++ b/auth/session_guard_test.go @@ -220,7 +220,7 @@ func (s *SessionGuardTestSuite) Test_InvalidKey() { s.ErrorIs(err, errors.AuthInvalidKey) } -func (s *SessionGuardTestSuite) TestLoginUsingID_RegenerateError() { +func (s *SessionGuardTestSuite) Test_LoginUsingID_RegenerateError() { s.mockSession.EXPECT().Regenerate(true).Return(assert.AnError).Once() token, err := s.sessionGuard.LoginUsingID(1) @@ -228,7 +228,7 @@ func (s *SessionGuardTestSuite) TestLoginUsingID_RegenerateError() { s.ErrorIs(err, assert.AnError) } -func (s *SessionGuardTestSuite) TestLogout_InvalidateError() { +func (s *SessionGuardTestSuite) Test_Logout_InvalidateError() { s.mockSession.EXPECT().Invalidate().Return(assert.AnError).Once() s.ErrorIs(s.sessionGuard.Logout(), assert.AnError) diff --git a/session/middleware/start_session.go b/session/middleware/start_session.go index a5b28c33e..ad41507f5 100644 --- a/session/middleware/start_session.go +++ b/session/middleware/start_session.go @@ -39,7 +39,11 @@ func StartSession() http.Middleware { s.Start() req.SetSession(s) - // Set session cookie in response + // Continue processing request + req.Next() + + // Set session cookie in response using the current (post-handler) + // session ID so rotations via Regenerate/Invalidate reach the client. config := session.ConfigFacade ctx.Response().Cookie(http.Cookie{ Name: s.GetName(), @@ -52,9 +56,6 @@ func StartSession() http.Middleware { SameSite: config.GetString("session.same_site"), }) - // Continue processing request - req.Next() - // Save session if err = s.Save(); err != nil { color.Errorf("Error saving session: %s\n", err) diff --git a/session/middleware/start_session_test.go b/session/middleware/start_session_test.go index 86efb9f08..af6d60d1e 100644 --- a/session/middleware/start_session_test.go +++ b/session/middleware/start_session_test.go @@ -82,6 +82,38 @@ func TestStartSession(t *testing.T) { assert.NoError(t, file.Remove("storage")) } +func TestStartSession_RegenerateReachesClient(t *testing.T) { + mockConfig := configmocks.NewConfig(t) + session.ConfigFacade = mockConfig + mockConfig.EXPECT().GetString("session.default", "file").Return("file").Once() + mockConfig.EXPECT().GetString("session.drivers.file.driver").Return("file").Once() + mockConfig.EXPECT().GetInt("session.lifetime", 120).Return(120).Once() + mockConfig.EXPECT().GetInt("session.gc_interval", 30).Return(30).Once() + mockConfig.EXPECT().GetString("session.files").Return(path.Storage("framework/sessions")).Once() + mockConfig.EXPECT().GetString("session.cookie").Return("goravel_session").Once() + + session.SessionFacade = session.NewManager(mockConfig, json.New()) + + var preID, postID string + server := httptest.NewServer(testHttpSessionMiddleware(nethttp.HandlerFunc(func(w nethttp.ResponseWriter, r *nethttp.Request) { + s := r.Context().Value("session").(contractsession.Session) + preID = s.GetID() + require.NoError(t, s.Regenerate(true)) + postID = s.GetID() + }), mockConfig)) + defer server.Close() + + resp, err := (&nethttp.Client{}).Get(server.URL + "/login") + require.NoError(t, err) + require.NotEmpty(t, resp.Cookies()) + + cookie := resp.Cookies()[0] + assert.NotEqual(t, preID, postID) + assert.Equal(t, postID, cookie.Value) + + assert.NoError(t, file.Remove("storage")) +} + type TestContext struct { ctx context.Context next nethttp.Handler From 33254765dffb192874edb6fde2b7166c00f9743a Mon Sep 17 00:00:00 2001 From: kkumar-gcc Date: Mon, 20 Apr 2026 06:51:29 +0530 Subject: [PATCH 3/6] reissue session cookie from guard for gin compatibility --- auth/session_guard.go | 29 +++++++++++++++- auth/session_guard_test.go | 42 ++++++++++++++++++++++++ session/middleware/start_session.go | 9 +++-- session/middleware/start_session_test.go | 32 ------------------ 4 files changed, 74 insertions(+), 38 deletions(-) diff --git a/auth/session_guard.go b/auth/session_guard.go index fe9169750..0fe913b94 100644 --- a/auth/session_guard.go +++ b/auth/session_guard.go @@ -9,6 +9,7 @@ import ( "github.com/goravel/framework/contracts/http" contractsession "github.com/goravel/framework/contracts/session" "github.com/goravel/framework/errors" + "github.com/goravel/framework/support/carbon" ) type SessionGuard struct { @@ -80,6 +81,7 @@ func (r *SessionGuard) LoginUsingID(id any) (token string, err error) { if err := r.session.Regenerate(true); err != nil { return "", err } + r.reissueSessionCookie() r.session.Put(sessionName, key) @@ -87,7 +89,32 @@ func (r *SessionGuard) LoginUsingID(id any) (token string, err error) { } func (r *SessionGuard) Logout() error { - return r.session.Invalidate() + if err := r.session.Invalidate(); err != nil { + return err + } + r.reissueSessionCookie() + + return nil +} + +// reissueSessionCookie writes the current session ID to the response cookie. +// Called after Regenerate/Invalidate so the rotated ID reaches the client +// even on drivers that flush headers when the handler writes the body. +func (r *SessionGuard) reissueSessionCookie() { + if configFacade == nil { + return + } + + 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"), + }) } func (r *SessionGuard) Parse(token string) (*contractsauth.Payload, error) { diff --git a/auth/session_guard_test.go b/auth/session_guard_test.go index 3ad339e0d..97dfb6869 100644 --- a/auth/session_guard_test.go +++ b/auth/session_guard_test.go @@ -4,6 +4,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/suite" "github.com/goravel/framework/contracts/http" @@ -24,6 +25,7 @@ type SessionGuardTestSuite struct { mockCache *mockscache.Cache mockConfig *mocksconfig.Config mockContext http.Context + mockResponse *mockshttp.ContextResponse mockDB *mocksorm.Query mockLog *mockslog.Log mockUserProvider *mocksauth.UserProvider @@ -51,8 +53,10 @@ func (s *SessionGuardTestSuite) SetupTest() { mockRequest := mockshttp.NewContextRequest(s.T()) mockRequest.EXPECT().Session().Return(s.mockSession) + s.mockResponse = mockshttp.NewContextResponse(s.T()) mockContext := mockshttp.NewContext(s.T()) mockContext.EXPECT().Request().Return(mockRequest) + mockContext.EXPECT().Response().Return(s.mockResponse).Maybe() s.mockContext = mockContext @@ -108,6 +112,7 @@ func (s *SessionGuardTestSuite) TestCheck_LoginUsingID_Logout() { s.True(s.sessionGuard.Guest()) s.mockSession.EXPECT().Regenerate(true).Return(nil).Once() + s.expectReissueCookie() s.mockSession.EXPECT().Put("auth_user_id", "1").Return(nil).Once() token, err := s.sessionGuard.LoginUsingID(1) s.Nil(err) @@ -118,6 +123,7 @@ func (s *SessionGuardTestSuite) TestCheck_LoginUsingID_Logout() { s.False(s.sessionGuard.Guest()) s.mockSession.EXPECT().Invalidate().Return(nil).Once() + s.expectReissueCookie() s.NoError(s.sessionGuard.Logout()) s.mockSession.EXPECT().Get("auth_user_id", nil).Return(nil).Once() @@ -135,6 +141,7 @@ func (s *SessionGuardTestSuite) Test_Login() { s.mockUserProvider.EXPECT().GetID(&user).Return("2", nil).Once() s.mockSession.EXPECT().Regenerate(true).Return(nil).Once() + s.expectReissueCookie() s.mockSession.EXPECT().Put("auth_user_id", "2").Return(nil).Once() token, err := s.sessionGuard.Login(&user) s.Nil(err) @@ -145,6 +152,7 @@ func (s *SessionGuardTestSuite) Test_Login() { s.False(s.sessionGuard.Guest()) s.mockSession.EXPECT().Invalidate().Return(nil).Once() + s.expectReissueCookie() s.NoError(s.sessionGuard.Logout()) s.mockSession.EXPECT().Get("auth_user_id", nil).Return(nil).Once() @@ -170,6 +178,7 @@ func (s *SessionGuardTestSuite) Test_LoginFailed() { s.True(s.sessionGuard.Guest()) s.mockSession.EXPECT().Invalidate().Return(nil).Once() + s.expectReissueCookie() s.NoError(s.sessionGuard.Logout()) s.mockSession.EXPECT().Get("auth_user_id", nil).Return(nil).Once() @@ -233,3 +242,36 @@ func (s *SessionGuardTestSuite) Test_Logout_InvalidateError() { s.ErrorIs(s.sessionGuard.Logout(), assert.AnError) } + +func (s *SessionGuardTestSuite) Test_LoginUsingID_ReissuesCookie() { + s.mockSession.EXPECT().Regenerate(true).Return(nil).Once() + s.mockSession.EXPECT().GetName().Return("goravel_session").Once() + s.mockSession.EXPECT().GetID().Return("new-session-id").Once() + s.mockSession.EXPECT().Put("auth_user_id", "1").Return(nil).Once() + + s.mockConfig.EXPECT().GetInt("session.lifetime", 120).Return(120).Once() + s.mockConfig.EXPECT().GetString("session.path").Return("/").Once() + s.mockConfig.EXPECT().GetString("session.domain").Return("").Once() + s.mockConfig.EXPECT().GetBool("session.secure").Return(false).Once() + s.mockConfig.EXPECT().GetBool("session.http_only").Return(true).Once() + s.mockConfig.EXPECT().GetString("session.same_site").Return("").Once() + + s.mockResponse.EXPECT().Cookie(mock.MatchedBy(func(c http.Cookie) bool { + return c.Name == "goravel_session" && c.Value == "new-session-id" + })).Return(s.mockResponse).Once() + + _, err := s.sessionGuard.LoginUsingID(1) + s.Nil(err) +} + +func (s *SessionGuardTestSuite) expectReissueCookie() { + s.mockSession.EXPECT().GetName().Return("goravel_session").Once() + s.mockSession.EXPECT().GetID().Return("session-id").Once() + s.mockConfig.EXPECT().GetInt("session.lifetime", 120).Return(120).Once() + s.mockConfig.EXPECT().GetString("session.path").Return("/").Once() + s.mockConfig.EXPECT().GetString("session.domain").Return("").Once() + s.mockConfig.EXPECT().GetBool("session.secure").Return(false).Once() + s.mockConfig.EXPECT().GetBool("session.http_only").Return(true).Once() + s.mockConfig.EXPECT().GetString("session.same_site").Return("").Once() + s.mockResponse.EXPECT().Cookie(mock.Anything).Return(s.mockResponse).Once() +} diff --git a/session/middleware/start_session.go b/session/middleware/start_session.go index ad41507f5..a5b28c33e 100644 --- a/session/middleware/start_session.go +++ b/session/middleware/start_session.go @@ -39,11 +39,7 @@ func StartSession() http.Middleware { s.Start() req.SetSession(s) - // Continue processing request - req.Next() - - // Set session cookie in response using the current (post-handler) - // session ID so rotations via Regenerate/Invalidate reach the client. + // Set session cookie in response config := session.ConfigFacade ctx.Response().Cookie(http.Cookie{ Name: s.GetName(), @@ -56,6 +52,9 @@ func StartSession() http.Middleware { SameSite: config.GetString("session.same_site"), }) + // Continue processing request + req.Next() + // Save session if err = s.Save(); err != nil { color.Errorf("Error saving session: %s\n", err) diff --git a/session/middleware/start_session_test.go b/session/middleware/start_session_test.go index af6d60d1e..86efb9f08 100644 --- a/session/middleware/start_session_test.go +++ b/session/middleware/start_session_test.go @@ -82,38 +82,6 @@ func TestStartSession(t *testing.T) { assert.NoError(t, file.Remove("storage")) } -func TestStartSession_RegenerateReachesClient(t *testing.T) { - mockConfig := configmocks.NewConfig(t) - session.ConfigFacade = mockConfig - mockConfig.EXPECT().GetString("session.default", "file").Return("file").Once() - mockConfig.EXPECT().GetString("session.drivers.file.driver").Return("file").Once() - mockConfig.EXPECT().GetInt("session.lifetime", 120).Return(120).Once() - mockConfig.EXPECT().GetInt("session.gc_interval", 30).Return(30).Once() - mockConfig.EXPECT().GetString("session.files").Return(path.Storage("framework/sessions")).Once() - mockConfig.EXPECT().GetString("session.cookie").Return("goravel_session").Once() - - session.SessionFacade = session.NewManager(mockConfig, json.New()) - - var preID, postID string - server := httptest.NewServer(testHttpSessionMiddleware(nethttp.HandlerFunc(func(w nethttp.ResponseWriter, r *nethttp.Request) { - s := r.Context().Value("session").(contractsession.Session) - preID = s.GetID() - require.NoError(t, s.Regenerate(true)) - postID = s.GetID() - }), mockConfig)) - defer server.Close() - - resp, err := (&nethttp.Client{}).Get(server.URL + "/login") - require.NoError(t, err) - require.NotEmpty(t, resp.Cookies()) - - cookie := resp.Cookies()[0] - assert.NotEqual(t, preID, postID) - assert.Equal(t, postID, cookie.Value) - - assert.NoError(t, file.Remove("storage")) -} - type TestContext struct { ctx context.Context next nethttp.Handler From 0a868352ac40b80a981ad1dc0030d07c0331fa40 Mon Sep 17 00:00:00 2001 From: kkumar-gcc Date: Mon, 20 Apr 2026 10:32:41 +0530 Subject: [PATCH 4/6] skip middleware cookie write when client already has matching id --- session/middleware/start_session.go | 32 ++++++++++++++---------- session/middleware/start_session_test.go | 22 +++++++++------- 2 files changed, 32 insertions(+), 22 deletions(-) diff --git a/session/middleware/start_session.go b/session/middleware/start_session.go index a5b28c33e..420d2fb72 100644 --- a/session/middleware/start_session.go +++ b/session/middleware/start_session.go @@ -33,24 +33,30 @@ func StartSession() http.Middleware { return } - s.SetID(req.Cookie(s.GetName())) + incomingCookie := req.Cookie(s.GetName()) + s.SetID(incomingCookie) // Start session s.Start() 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"), - }) + // Only write the session cookie when the client doesn't already have + // a matching one. Rotations triggered in the handler (e.g. login / + // logout) reissue the cookie themselves, so this avoids emitting a + // duplicate Set-Cookie header in that flow. + if incomingCookie != s.GetID() { + 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"), + }) + } // Continue processing request req.Next() diff --git a/session/middleware/start_session_test.go b/session/middleware/start_session_test.go index 86efb9f08..171f476cd 100644 --- a/session/middleware/start_session_test.go +++ b/session/middleware/start_session_test.go @@ -28,13 +28,15 @@ 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() + // Cookie-related reads only happen when the middleware emits a new + // Set-Cookie (no matching incoming cookie), so mark them optional. + mockConfig.EXPECT().GetInt("session.lifetime", 120).Return(120).Maybe() + mockConfig.EXPECT().GetString("session.path").Return("/").Maybe() + mockConfig.EXPECT().GetString("session.domain").Return("").Maybe() + mockConfig.EXPECT().GetBool("session.secure").Return(false).Maybe() + mockConfig.EXPECT().GetBool("session.http_only").Return(true).Maybe() + mockConfig.EXPECT().GetString("session.same_site").Return("").Maybe() } func TestStartSession(t *testing.T) { @@ -76,8 +78,10 @@ func TestStartSession(t *testing.T) { resp, err = client.Do(req) require.NoError(t, err) - assert.Equal(t, cookie.Name, resp.Cookies()[0].Name) - assert.Equal(t, cookie.Value, resp.Cookies()[0].Value) + // Incoming cookie already matches the loaded session ID — middleware + // skips emitting another Set-Cookie to avoid duplicating the header + // produced by any in-handler rotation. + assert.Empty(t, resp.Cookies()) assert.NoError(t, file.Remove("storage")) } From ed3267db3d66e548362c3bb6fefe625d1d2c30b8 Mon Sep 17 00:00:00 2001 From: kkumar-gcc Date: Wed, 22 Apr 2026 16:06:29 +0530 Subject: [PATCH 5/6] refactor(session): extract shared cookie writer --- auth/session_guard.go | 32 +++++----------------- auth/session_guard_test.go | 2 ++ session/cookie.go | 41 +++++++++++++++++++++++++++++ session/middleware/start_session.go | 13 +-------- 4 files changed, 50 insertions(+), 38 deletions(-) create mode 100644 session/cookie.go diff --git a/auth/session_guard.go b/auth/session_guard.go index 0fe913b94..93997266f 100644 --- a/auth/session_guard.go +++ b/auth/session_guard.go @@ -9,7 +9,7 @@ import ( "github.com/goravel/framework/contracts/http" contractsession "github.com/goravel/framework/contracts/session" "github.com/goravel/framework/errors" - "github.com/goravel/framework/support/carbon" + "github.com/goravel/framework/session" ) type SessionGuard struct { @@ -23,13 +23,13 @@ func NewSessionGuard(ctx http.Context, name string, userProvider contractsauth.U if ctx == nil { return nil, errors.InvalidHttpContext.SetModule(errors.ModuleAuth) } - session := ctx.Request().Session() - if session == nil { + s := ctx.Request().Session() + if s == nil { return nil, errors.SessionDriverIsNotSet.SetModule(errors.ModuleAuth) } return &SessionGuard{ - session: session, + session: s, ctx: ctx, guard: name, provider: userProvider, @@ -81,7 +81,7 @@ func (r *SessionGuard) LoginUsingID(id any) (token string, err error) { if err := r.session.Regenerate(true); err != nil { return "", err } - r.reissueSessionCookie() + session.WriteCookie(r.ctx) r.session.Put(sessionName, key) @@ -92,31 +92,11 @@ func (r *SessionGuard) Logout() error { if err := r.session.Invalidate(); err != nil { return err } - r.reissueSessionCookie() + session.WriteCookie(r.ctx) return nil } -// reissueSessionCookie writes the current session ID to the response cookie. -// Called after Regenerate/Invalidate so the rotated ID reaches the client -// even on drivers that flush headers when the handler writes the body. -func (r *SessionGuard) reissueSessionCookie() { - if configFacade == nil { - return - } - - 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"), - }) -} - func (r *SessionGuard) Parse(token string) (*contractsauth.Payload, error) { return nil, errors.AuthUnsupportedDriverMethod.Args("session") } diff --git a/auth/session_guard_test.go b/auth/session_guard_test.go index 97dfb6869..feba1498a 100644 --- a/auth/session_guard_test.go +++ b/auth/session_guard_test.go @@ -16,6 +16,7 @@ import ( mockshttp "github.com/goravel/framework/mocks/http" mockslog "github.com/goravel/framework/mocks/log" mockssession "github.com/goravel/framework/mocks/session" + "github.com/goravel/framework/session" "github.com/goravel/framework/support/carbon" ) @@ -62,6 +63,7 @@ func (s *SessionGuardTestSuite) SetupTest() { cacheFacade = s.mockCache configFacade = s.mockConfig + session.ConfigFacade = s.mockConfig sessionGuard, err := NewSessionGuard(s.mockContext, testUserGuard, s.mockUserProvider) s.Require().Nil(err) diff --git a/session/cookie.go b/session/cookie.go new file mode 100644 index 000000000..9645582d4 --- /dev/null +++ b/session/cookie.go @@ -0,0 +1,41 @@ +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. Shared between StartSession middleware and guards that rotate +// the session ID mid-request so the cookie attributes and config keys +// live in one place. 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"), + }) +} diff --git a/session/middleware/start_session.go b/session/middleware/start_session.go index 420d2fb72..9e5f78fd5 100644 --- a/session/middleware/start_session.go +++ b/session/middleware/start_session.go @@ -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" ) @@ -45,17 +44,7 @@ func StartSession() http.Middleware { // logout) reissue the cookie themselves, so this avoids emitting a // duplicate Set-Cookie header in that flow. if incomingCookie != s.GetID() { - 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 From b2739a309c946231fc2f36e213b99842f2880791 Mon Sep 17 00:00:00 2001 From: kkumar-gcc Date: Sun, 26 Apr 2026 16:27:21 +0530 Subject: [PATCH 6/6] session: emit cookie from middleware only --- auth/session_guard.go | 15 +++----- auth/session_guard_test.go | 44 ------------------------ session/cookie.go | 12 ++++--- session/middleware/start_session.go | 12 ++----- session/middleware/start_session_test.go | 20 +++++------ 5 files changed, 22 insertions(+), 81 deletions(-) diff --git a/auth/session_guard.go b/auth/session_guard.go index 93997266f..fe9169750 100644 --- a/auth/session_guard.go +++ b/auth/session_guard.go @@ -9,7 +9,6 @@ import ( "github.com/goravel/framework/contracts/http" contractsession "github.com/goravel/framework/contracts/session" "github.com/goravel/framework/errors" - "github.com/goravel/framework/session" ) type SessionGuard struct { @@ -23,13 +22,13 @@ func NewSessionGuard(ctx http.Context, name string, userProvider contractsauth.U if ctx == nil { return nil, errors.InvalidHttpContext.SetModule(errors.ModuleAuth) } - s := ctx.Request().Session() - if s == nil { + session := ctx.Request().Session() + if session == nil { return nil, errors.SessionDriverIsNotSet.SetModule(errors.ModuleAuth) } return &SessionGuard{ - session: s, + session: session, ctx: ctx, guard: name, provider: userProvider, @@ -81,7 +80,6 @@ func (r *SessionGuard) LoginUsingID(id any) (token string, err error) { if err := r.session.Regenerate(true); err != nil { return "", err } - session.WriteCookie(r.ctx) r.session.Put(sessionName, key) @@ -89,12 +87,7 @@ func (r *SessionGuard) LoginUsingID(id any) (token string, err error) { } func (r *SessionGuard) Logout() error { - if err := r.session.Invalidate(); err != nil { - return err - } - session.WriteCookie(r.ctx) - - return nil + return r.session.Invalidate() } func (r *SessionGuard) Parse(token string) (*contractsauth.Payload, error) { diff --git a/auth/session_guard_test.go b/auth/session_guard_test.go index feba1498a..3ad339e0d 100644 --- a/auth/session_guard_test.go +++ b/auth/session_guard_test.go @@ -4,7 +4,6 @@ import ( "testing" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/mock" "github.com/stretchr/testify/suite" "github.com/goravel/framework/contracts/http" @@ -16,7 +15,6 @@ import ( mockshttp "github.com/goravel/framework/mocks/http" mockslog "github.com/goravel/framework/mocks/log" mockssession "github.com/goravel/framework/mocks/session" - "github.com/goravel/framework/session" "github.com/goravel/framework/support/carbon" ) @@ -26,7 +24,6 @@ type SessionGuardTestSuite struct { mockCache *mockscache.Cache mockConfig *mocksconfig.Config mockContext http.Context - mockResponse *mockshttp.ContextResponse mockDB *mocksorm.Query mockLog *mockslog.Log mockUserProvider *mocksauth.UserProvider @@ -54,16 +51,13 @@ func (s *SessionGuardTestSuite) SetupTest() { mockRequest := mockshttp.NewContextRequest(s.T()) mockRequest.EXPECT().Session().Return(s.mockSession) - s.mockResponse = mockshttp.NewContextResponse(s.T()) mockContext := mockshttp.NewContext(s.T()) mockContext.EXPECT().Request().Return(mockRequest) - mockContext.EXPECT().Response().Return(s.mockResponse).Maybe() s.mockContext = mockContext cacheFacade = s.mockCache configFacade = s.mockConfig - session.ConfigFacade = s.mockConfig sessionGuard, err := NewSessionGuard(s.mockContext, testUserGuard, s.mockUserProvider) s.Require().Nil(err) @@ -114,7 +108,6 @@ func (s *SessionGuardTestSuite) TestCheck_LoginUsingID_Logout() { s.True(s.sessionGuard.Guest()) s.mockSession.EXPECT().Regenerate(true).Return(nil).Once() - s.expectReissueCookie() s.mockSession.EXPECT().Put("auth_user_id", "1").Return(nil).Once() token, err := s.sessionGuard.LoginUsingID(1) s.Nil(err) @@ -125,7 +118,6 @@ func (s *SessionGuardTestSuite) TestCheck_LoginUsingID_Logout() { s.False(s.sessionGuard.Guest()) s.mockSession.EXPECT().Invalidate().Return(nil).Once() - s.expectReissueCookie() s.NoError(s.sessionGuard.Logout()) s.mockSession.EXPECT().Get("auth_user_id", nil).Return(nil).Once() @@ -143,7 +135,6 @@ func (s *SessionGuardTestSuite) Test_Login() { s.mockUserProvider.EXPECT().GetID(&user).Return("2", nil).Once() s.mockSession.EXPECT().Regenerate(true).Return(nil).Once() - s.expectReissueCookie() s.mockSession.EXPECT().Put("auth_user_id", "2").Return(nil).Once() token, err := s.sessionGuard.Login(&user) s.Nil(err) @@ -154,7 +145,6 @@ func (s *SessionGuardTestSuite) Test_Login() { s.False(s.sessionGuard.Guest()) s.mockSession.EXPECT().Invalidate().Return(nil).Once() - s.expectReissueCookie() s.NoError(s.sessionGuard.Logout()) s.mockSession.EXPECT().Get("auth_user_id", nil).Return(nil).Once() @@ -180,7 +170,6 @@ func (s *SessionGuardTestSuite) Test_LoginFailed() { s.True(s.sessionGuard.Guest()) s.mockSession.EXPECT().Invalidate().Return(nil).Once() - s.expectReissueCookie() s.NoError(s.sessionGuard.Logout()) s.mockSession.EXPECT().Get("auth_user_id", nil).Return(nil).Once() @@ -244,36 +233,3 @@ func (s *SessionGuardTestSuite) Test_Logout_InvalidateError() { s.ErrorIs(s.sessionGuard.Logout(), assert.AnError) } - -func (s *SessionGuardTestSuite) Test_LoginUsingID_ReissuesCookie() { - s.mockSession.EXPECT().Regenerate(true).Return(nil).Once() - s.mockSession.EXPECT().GetName().Return("goravel_session").Once() - s.mockSession.EXPECT().GetID().Return("new-session-id").Once() - s.mockSession.EXPECT().Put("auth_user_id", "1").Return(nil).Once() - - s.mockConfig.EXPECT().GetInt("session.lifetime", 120).Return(120).Once() - s.mockConfig.EXPECT().GetString("session.path").Return("/").Once() - s.mockConfig.EXPECT().GetString("session.domain").Return("").Once() - s.mockConfig.EXPECT().GetBool("session.secure").Return(false).Once() - s.mockConfig.EXPECT().GetBool("session.http_only").Return(true).Once() - s.mockConfig.EXPECT().GetString("session.same_site").Return("").Once() - - s.mockResponse.EXPECT().Cookie(mock.MatchedBy(func(c http.Cookie) bool { - return c.Name == "goravel_session" && c.Value == "new-session-id" - })).Return(s.mockResponse).Once() - - _, err := s.sessionGuard.LoginUsingID(1) - s.Nil(err) -} - -func (s *SessionGuardTestSuite) expectReissueCookie() { - s.mockSession.EXPECT().GetName().Return("goravel_session").Once() - s.mockSession.EXPECT().GetID().Return("session-id").Once() - s.mockConfig.EXPECT().GetInt("session.lifetime", 120).Return(120).Once() - s.mockConfig.EXPECT().GetString("session.path").Return("/").Once() - s.mockConfig.EXPECT().GetString("session.domain").Return("").Once() - s.mockConfig.EXPECT().GetBool("session.secure").Return(false).Once() - s.mockConfig.EXPECT().GetBool("session.http_only").Return(true).Once() - s.mockConfig.EXPECT().GetString("session.same_site").Return("").Once() - s.mockResponse.EXPECT().Cookie(mock.Anything).Return(s.mockResponse).Once() -} diff --git a/session/cookie.go b/session/cookie.go index 9645582d4..08029bc77 100644 --- a/session/cookie.go +++ b/session/cookie.go @@ -6,11 +6,13 @@ import ( ) // WriteCookie emits the current session ID as a Set-Cookie header on the -// response. Shared between StartSession middleware and guards that rotate -// the session ID mid-request so the cookie attributes and config keys -// live in one place. Safe to call with a partially initialised context — -// returns without effect if the context, response, session, or config -// facade is nil. +// 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 diff --git a/session/middleware/start_session.go b/session/middleware/start_session.go index 9e5f78fd5..64168547a 100644 --- a/session/middleware/start_session.go +++ b/session/middleware/start_session.go @@ -32,20 +32,14 @@ func StartSession() http.Middleware { return } - incomingCookie := req.Cookie(s.GetName()) - s.SetID(incomingCookie) + s.SetID(req.Cookie(s.GetName())) // Start session s.Start() req.SetSession(s) - // Only write the session cookie when the client doesn't already have - // a matching one. Rotations triggered in the handler (e.g. login / - // logout) reissue the cookie themselves, so this avoids emitting a - // duplicate Set-Cookie header in that flow. - if incomingCookie != s.GetID() { - session.WriteCookie(ctx) - } + // Set session cookie in response + session.WriteCookie(ctx) // Continue processing request req.Next() diff --git a/session/middleware/start_session_test.go b/session/middleware/start_session_test.go index 171f476cd..fea8bcb36 100644 --- a/session/middleware/start_session_test.go +++ b/session/middleware/start_session_test.go @@ -29,14 +29,12 @@ func testHttpSessionMiddleware(next nethttp.Handler, mockConfig *configmocks.Con func mockConfigFacade(mockConfig *configmocks.Config) { mockConfig.EXPECT().GetString("session.default").Return("file").Once() - // Cookie-related reads only happen when the middleware emits a new - // Set-Cookie (no matching incoming cookie), so mark them optional. - mockConfig.EXPECT().GetInt("session.lifetime", 120).Return(120).Maybe() - mockConfig.EXPECT().GetString("session.path").Return("/").Maybe() - mockConfig.EXPECT().GetString("session.domain").Return("").Maybe() - mockConfig.EXPECT().GetBool("session.secure").Return(false).Maybe() - mockConfig.EXPECT().GetBool("session.http_only").Return(true).Maybe() - mockConfig.EXPECT().GetString("session.same_site").Return("").Maybe() + 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) { @@ -78,10 +76,8 @@ func TestStartSession(t *testing.T) { resp, err = client.Do(req) require.NoError(t, err) - // Incoming cookie already matches the loaded session ID — middleware - // skips emitting another Set-Cookie to avoid duplicating the header - // produced by any in-handler rotation. - assert.Empty(t, resp.Cookies()) + assert.Equal(t, cookie.Name, resp.Cookies()[0].Name) + assert.Equal(t, cookie.Value, resp.Cookies()[0].Value) assert.NoError(t, file.Remove("storage")) }