fix: Session renewal in hook not rotating token

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

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,27 +185,37 @@ 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 extends the session expiration in the database and updates the cookie
 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
+	}
+
+	session.ID = newSession.ID
+	session.Token = hashedToken
+	session.ExpiresAt = newSession.ExpiresAt
+
+	p.SetSessionCookie(w, newToken)
 }
 
 func (p *SessionPlugin) Close() error {

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)
+		})
+	}
+}