From 5cfde0be520107d8ae127adf24493d0a3443b718 Mon Sep 17 00:00:00 2001 From: Tanvir Ahmed Date: Tue, 21 Jul 2026 02:10:10 +0000 Subject: [PATCH 1/2] fix: Session renewal in hook not rotating token --- plugins/session/plugin.go | 27 ++++-- plugins/session/plugin_test.go | 149 +++++++++++++++++++++++++++++++++ 2 files changed, 169 insertions(+), 7 deletions(-) create mode 100644 plugins/session/plugin_test.go diff --git a/plugins/session/plugin.go b/plugins/session/plugin.go index 17c6b08..60924c9 100644 --- a/plugins/session/plugin.go +++ b/plugins/session/plugin.go @@ -194,20 +194,33 @@ func (p *SessionPlugin) shouldRenewSession(session *models.Session) bool { return timeToExpiry <= p.globalConfig.Session.UpdateAge } -// renewSession extends the session expiration in the database and updates the cookie +// renewSession rotates the session token, deletes the previous session, and creates a new one func (p *SessionPlugin) renewSession(w http.ResponseWriter, r *http.Request, session *models.Session) { - cookie, _ := r.Cookie(p.globalConfig.Session.CookieName) - if cookie == nil { + newToken, err := p.tokenService.Generate() + if err != nil { + p.logger.Error("session renewal failed: token generation error", "error", err) return } - session.ExpiresAt = time.Now().UTC().Add(p.globalConfig.Session.ExpiresIn) - if _, err := p.sessionService.Update(r.Context(), session); err != nil { - p.logger.Error("session renewal failed", "error", err) + hashedToken := p.tokenService.Hash(newToken) + + if err := p.sessionService.Delete(r.Context(), session.ID); err != nil { + p.logger.Error("session renewal failed: delete error", "error", err) return } - p.SetSessionCookie(w, cookie.Value) + newSession, err := p.sessionService.Create(r.Context(), session.UserID, hashedToken, session.IPAddress, session.UserAgent, p.globalConfig.Session.ExpiresIn) + if err != nil { + p.logger.Error("session renewal failed: create error", "error", err) + return + } + + // Update the caller's session pointer so downstream code uses the new session identity + session.ID = newSession.ID + session.Token = hashedToken + session.ExpiresAt = newSession.ExpiresAt + + p.SetSessionCookie(w, newToken) } func (p *SessionPlugin) Close() error { diff --git a/plugins/session/plugin_test.go b/plugins/session/plugin_test.go new file mode 100644 index 0000000..ee47ac9 --- /dev/null +++ b/plugins/session/plugin_test.go @@ -0,0 +1,149 @@ +package session + +import ( + "errors" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + + internaltests "github.com/Authula/authula/internal/tests" + "github.com/Authula/authula/models" +) + +func TestSessionPlugin_RenewSession(t *testing.T) { + t.Parallel() + + now := time.Now().UTC() + clientIP := "192.168.1.1" + userAgent := "test-agent" + + tests := []struct { + name string + setup func(*internaltests.MockTokenService, *internaltests.MockSessionService) + session *models.Session + wantCookie bool + wantToken string + wantSessionID string + }{ + { + name: "rotates token and creates new session", + setup: func(mockTokenSvc *internaltests.MockTokenService, mockSessionSvc *internaltests.MockSessionService) { + mockTokenSvc.On("Generate").Return("new-token", nil).Once() + mockTokenSvc.On("Hash", "new-token").Return("hashed-new-token").Once() + mockSessionSvc.On("Delete", mock.Anything, "session-id").Return(nil).Once() + mockSessionSvc.On("Create", mock.Anything, "user-id", "hashed-new-token", &clientIP, &userAgent, time.Hour*24*7). + Return(&models.Session{ID: "new-session-id", Token: "hashed-new-token", ExpiresAt: now.Add(time.Hour * 24 * 7)}, nil).Once() + }, + session: &models.Session{ + ID: "session-id", + UserID: "user-id", + Token: "hashed-old-token", + ExpiresAt: now.Add(24 * time.Hour), + IPAddress: &clientIP, + UserAgent: &userAgent, + }, + wantCookie: true, + wantToken: "new-token", + wantSessionID: "new-session-id", + }, + { + name: "token generation failure aborts renewal", + setup: func(mockTokenSvc *internaltests.MockTokenService, mockSessionSvc *internaltests.MockSessionService) { + mockTokenSvc.On("Generate").Return("", errors.New("generation failed")).Once() + }, + session: &models.Session{ + ID: "session-id", + Token: "hashed-old-token", + IPAddress: &clientIP, + UserAgent: &userAgent, + }, + wantCookie: false, + }, + { + name: "delete failure aborts renewal", + setup: func(mockTokenSvc *internaltests.MockTokenService, mockSessionSvc *internaltests.MockSessionService) { + mockTokenSvc.On("Generate").Return("new-token", nil).Once() + mockTokenSvc.On("Hash", "new-token").Return("hashed-new-token").Once() + mockSessionSvc.On("Delete", mock.Anything, "session-id").Return(errors.New("delete failed")).Once() + }, + session: &models.Session{ + ID: "session-id", + Token: "hashed-old-token", + IPAddress: &clientIP, + UserAgent: &userAgent, + }, + wantCookie: false, + }, + { + name: "create failure after delete aborts renewal", + setup: func(mockTokenSvc *internaltests.MockTokenService, mockSessionSvc *internaltests.MockSessionService) { + mockTokenSvc.On("Generate").Return("new-token", nil).Once() + mockTokenSvc.On("Hash", "new-token").Return("hashed-new-token").Once() + mockSessionSvc.On("Delete", mock.Anything, "session-id").Return(nil).Once() + mockSessionSvc.On("Create", mock.Anything, "user-id", "hashed-new-token", &clientIP, &userAgent, time.Hour*24*7). + Return(nil, errors.New("create failed")).Once() + }, + session: &models.Session{ + ID: "session-id", + UserID: "user-id", + Token: "hashed-old-token", + IPAddress: &clientIP, + UserAgent: &userAgent, + }, + wantCookie: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mockTokenSvc := new(internaltests.MockTokenService) + mockSessionSvc := new(internaltests.MockSessionService) + + tt.setup(mockTokenSvc, mockSessionSvc) + + plugin := &SessionPlugin{ + globalConfig: &models.Config{ + Session: models.SessionConfig{ + CookieName: "authula.session_token", + ExpiresIn: time.Hour * 24 * 7, + CookieMaxAge: time.Hour * 24 * 7, + HttpOnly: true, + SameSite: "lax", + }, + }, + sessionService: mockSessionSvc, + tokenService: mockTokenSvc, + logger: new(internaltests.MockLogger), + } + + r := httptest.NewRequest(http.MethodGet, "/", nil) + r.AddCookie(&http.Cookie{ + Name: "authula.session_token", + Value: "old-plaintext-token", + }) + w := httptest.NewRecorder() + + plugin.renewSession(w, r, tt.session) + + cookies := w.Result().Cookies() + if tt.wantCookie { + require.Len(t, cookies, 1) + assert.Equal(t, "authula.session_token", cookies[0].Name) + assert.Equal(t, tt.wantToken, cookies[0].Value) + assert.Equal(t, tt.wantSessionID, tt.session.ID, "session ID should be updated") + assert.Equal(t, "hashed-new-token", tt.session.Token, "session token should be updated") + } else { + assert.Empty(t, cookies) + assert.Equal(t, "hashed-old-token", tt.session.Token, "session token should not change on error") + } + + mockTokenSvc.AssertExpectations(t) + mockSessionSvc.AssertExpectations(t) + }) + } +} From 3a355f20d638215882af457f3198b66e62e3bacf Mon Sep 17 00:00:00 2001 From: Tanvir Ahmed Date: Tue, 21 Jul 2026 02:18:33 +0000 Subject: [PATCH 2/2] chore: Removed comments --- plugins/session/hooks.go | 1 - plugins/session/plugin.go | 5 ----- 2 files changed, 6 deletions(-) diff --git a/plugins/session/hooks.go b/plugins/session/hooks.go index 349ee8f..62c8861 100644 --- a/plugins/session/hooks.go +++ b/plugins/session/hooks.go @@ -91,7 +91,6 @@ func (p *SessionPlugin) validateSessionHook(reqCtx *models.RequestContext) error }) reqCtx.Values[models.ContextSessionID.String()] = session.ID - // Optionally renew session if it's past 50% of its max age if p.shouldRenewSession(session) { p.renewSession(reqCtx.ResponseWriter, reqCtx.Request, session) } diff --git a/plugins/session/plugin.go b/plugins/session/plugin.go index 60924c9..6bda0cf 100644 --- a/plugins/session/plugin.go +++ b/plugins/session/plugin.go @@ -91,7 +91,6 @@ func (p *SessionPlugin) AuthMiddleware() func(http.Handler) http.Handler { return } - // Check if session should be renewed (sliding window: <50% life remaining) if p.shouldRenewSession(session) { p.renewSession(w, r, session) } @@ -108,7 +107,6 @@ func (p *SessionPlugin) OptionalAuthMiddleware() func(http.Handler) http.Handler return func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if session, err := p.validateSessionCookie(r); err == nil && session != nil { - // Check if session should be renewed (sliding window: <50% life remaining) if p.shouldRenewSession(session) { p.renewSession(w, r, session) } @@ -187,14 +185,12 @@ func (p *SessionPlugin) ClearSessionCookie(w http.ResponseWriter) { }) } -// shouldRenewSession checks if the session is past 50% of its max age and should be renewed func (p *SessionPlugin) shouldRenewSession(session *models.Session) bool { now := time.Now().UTC() timeToExpiry := session.ExpiresAt.Sub(now) return timeToExpiry <= p.globalConfig.Session.UpdateAge } -// renewSession rotates the session token, deletes the previous session, and creates a new one func (p *SessionPlugin) renewSession(w http.ResponseWriter, r *http.Request, session *models.Session) { newToken, err := p.tokenService.Generate() if err != nil { @@ -215,7 +211,6 @@ func (p *SessionPlugin) renewSession(w http.ResponseWriter, r *http.Request, ses return } - // Update the caller's session pointer so downstream code uses the new session identity session.ID = newSession.ID session.Token = hashedToken session.ExpiresAt = newSession.ExpiresAt