From f8c6ee8fc7caade9a555bc74118c3836c0cc59ae Mon Sep 17 00:00:00 2001 From: majiayu000 <1835304752@qq.com> Date: Sun, 4 Jan 2026 11:13:29 +0800 Subject: [PATCH 1/3] fix: clean up expired sessions in SessionMiddleware MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, expired sessions were detected but not removed from the database, causing them to accumulate indefinitely. Now when an expired session is encountered, it is deleted from the database before continuing with the request. Fixes #3 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- internal/oauth/middleware.go | 6 ++++-- internal/oauth/middleware_test.go | 12 ++++++++++-- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/internal/oauth/middleware.go b/internal/oauth/middleware.go index b444dec..e9004c5 100644 --- a/internal/oauth/middleware.go +++ b/internal/oauth/middleware.go @@ -38,8 +38,10 @@ func SessionMiddleware(storage *Storage) echo.MiddlewareFunc { // Check if session is expired if session.ExpiresAt.Before(time.Now()) { - // Expired session - continue without user - // TODO: Consider cleaning up expired session here + // Clean up expired session from database + if err := storage.DeleteSession(c.Request().Context(), cookie.Value); err != nil { + c.Logger().Errorf("Failed to delete expired session: %v", err) + } return next(c) } diff --git a/internal/oauth/middleware_test.go b/internal/oauth/middleware_test.go index a11d007..3bc0f2e 100644 --- a/internal/oauth/middleware_test.go +++ b/internal/oauth/middleware_test.go @@ -95,7 +95,7 @@ func TestSessionMiddleware(t *testing.T) { assert.Equal(t, "did:plc:test123", capturedUser.DID) }) - t.Run("expired session - sets nil user in context", func(t *testing.T) { + t.Run("expired session - sets nil user in context and deletes session", func(t *testing.T) { // Create an expired session e := echo.New() setupReq := httptest.NewRequest(http.MethodGet, "/", nil) @@ -103,13 +103,17 @@ func TestSessionMiddleware(t *testing.T) { setupCtx := e.NewContext(setupReq, setupRec) session := OAuthSession{ - ID: "expired-session", + ID: "expired-session-cleanup", DID: "did:plc:expired", ExpiresAt: time.Now().Add(-1 * time.Hour), } err := storage.CreateSession(setupCtx.Request().Context(), session) require.NoError(t, err) + // Verify session exists before middleware call + _, err = storage.GetSessionByID(setupCtx.Request().Context(), session.ID) + require.NoError(t, err, "Session should exist before middleware call") + // Make request with expired session cookie req := httptest.NewRequest(http.MethodGet, "/", nil) req.AddCookie(&http.Cookie{ @@ -128,6 +132,10 @@ func TestSessionMiddleware(t *testing.T) { err = handler(c) require.NoError(t, err) assert.Nil(t, capturedUser) + + // Verify session was deleted from database + _, err = storage.GetSessionByID(setupCtx.Request().Context(), session.ID) + assert.Error(t, err, "Expired session should be deleted from database") }) } From e9ad0a824e8cd8cd8745c56b3dfa8bd0d14f00a6 Mon Sep 17 00:00:00 2001 From: majiayu000 <1835304752@qq.com> Date: Sun, 4 Jan 2026 14:11:03 +0800 Subject: [PATCH 2/3] test: add unit tests for session cleanup --- internal/oauth/middleware.go | 9 +- internal/oauth/middleware_unit_test.go | 137 +++++++++++++++++++++++++ 2 files changed, 145 insertions(+), 1 deletion(-) create mode 100644 internal/oauth/middleware_unit_test.go diff --git a/internal/oauth/middleware.go b/internal/oauth/middleware.go index e9004c5..20493ee 100644 --- a/internal/oauth/middleware.go +++ b/internal/oauth/middleware.go @@ -1,6 +1,7 @@ package oauth import ( + "context" "database/sql" "time" @@ -12,9 +13,15 @@ type User struct { DID string } +// SessionStore defines the session operations needed by the middleware. +type SessionStore interface { + GetSessionByID(ctx context.Context, id string) (*OAuthSession, error) + DeleteSession(ctx context.Context, id string) error +} + // SessionMiddleware creates middleware that reads the session cookie // and adds the user to the context if the session is valid -func SessionMiddleware(storage *Storage) echo.MiddlewareFunc { +func SessionMiddleware(storage SessionStore) echo.MiddlewareFunc { return func(next echo.HandlerFunc) echo.HandlerFunc { return func(c echo.Context) error { // Try to get session cookie diff --git a/internal/oauth/middleware_unit_test.go b/internal/oauth/middleware_unit_test.go new file mode 100644 index 0000000..c21c6a5 --- /dev/null +++ b/internal/oauth/middleware_unit_test.go @@ -0,0 +1,137 @@ +package oauth + +import ( + "context" + "database/sql" + "errors" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/labstack/echo/v4" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +type stubSessionStore struct { + sessions map[string]*OAuthSession + deleteErr error + deleteCalls []string +} + +func (s *stubSessionStore) GetSessionByID(ctx context.Context, id string) (*OAuthSession, error) { + session, ok := s.sessions[id] + if !ok { + return nil, sql.ErrNoRows + } + return session, nil +} + +func (s *stubSessionStore) DeleteSession(ctx context.Context, id string) error { + s.deleteCalls = append(s.deleteCalls, id) + if s.deleteErr != nil { + return s.deleteErr + } + delete(s.sessions, id) + return nil +} + +func TestSessionMiddlewareExpiredSessionDeletes(t *testing.T) { + store := &stubSessionStore{ + sessions: map[string]*OAuthSession{ + "expired-session": { + ID: "expired-session", + DID: "did:plc:expired", + ExpiresAt: time.Now().Add(-1 * time.Minute), + }, + }, + } + + e := echo.New() + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.AddCookie(&http.Cookie{Name: "session", Value: "expired-session"}) + rec := httptest.NewRecorder() + c := e.NewContext(req, rec) + + var capturedUser *User + nextCalled := false + handler := SessionMiddleware(store)(func(c echo.Context) error { + nextCalled = true + capturedUser = GetUser(c) + return c.String(http.StatusOK, "ok") + }) + + err := handler(c) + require.NoError(t, err) + assert.True(t, nextCalled) + assert.Nil(t, capturedUser) + assert.Equal(t, []string{"expired-session"}, store.deleteCalls) + _, exists := store.sessions["expired-session"] + assert.False(t, exists) +} + +func TestSessionMiddlewareDeleteErrorDoesNotBlock(t *testing.T) { + store := &stubSessionStore{ + sessions: map[string]*OAuthSession{ + "expired-session": { + ID: "expired-session", + DID: "did:plc:expired", + ExpiresAt: time.Now().Add(-1 * time.Minute), + }, + }, + deleteErr: errors.New("delete failed"), + } + + e := echo.New() + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.AddCookie(&http.Cookie{Name: "session", Value: "expired-session"}) + rec := httptest.NewRecorder() + c := e.NewContext(req, rec) + + var capturedUser *User + nextCalled := false + handler := SessionMiddleware(store)(func(c echo.Context) error { + nextCalled = true + capturedUser = GetUser(c) + return c.String(http.StatusOK, "ok") + }) + + err := handler(c) + require.NoError(t, err) + assert.True(t, nextCalled) + assert.Nil(t, capturedUser) + assert.Equal(t, []string{"expired-session"}, store.deleteCalls) + _, exists := store.sessions["expired-session"] + assert.True(t, exists) +} + +func TestSessionMiddlewareValidSessionDoesNotDelete(t *testing.T) { + store := &stubSessionStore{ + sessions: map[string]*OAuthSession{ + "valid-session": { + ID: "valid-session", + DID: "did:plc:valid", + ExpiresAt: time.Now().Add(1 * time.Hour), + }, + }, + } + + e := echo.New() + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.AddCookie(&http.Cookie{Name: "session", Value: "valid-session"}) + rec := httptest.NewRecorder() + c := e.NewContext(req, rec) + + var capturedUser *User + handler := SessionMiddleware(store)(func(c echo.Context) error { + capturedUser = GetUser(c) + return c.String(http.StatusOK, "ok") + }) + + err := handler(c) + require.NoError(t, err) + require.NotNil(t, capturedUser) + assert.Equal(t, "did:plc:valid", capturedUser.DID) + assert.Empty(t, store.deleteCalls) +} From 6bbf439663605bce34bdce25667624c3660021aa Mon Sep 17 00:00:00 2001 From: lif <1835304752@qq.com> Date: Sun, 4 Jan 2026 22:52:03 +0800 Subject: [PATCH 3/3] fix: clear session cookie when expired session is detected MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address reviewer feedback: when detecting an expired session, now also clear the browser cookie by setting MaxAge=-1. This prevents the client from sending stale session cookies on subsequent requests. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- internal/oauth/middleware.go | 9 +++++++++ internal/oauth/middleware_test.go | 12 ++++++++++++ 2 files changed, 21 insertions(+) diff --git a/internal/oauth/middleware.go b/internal/oauth/middleware.go index 20493ee..cbd776d 100644 --- a/internal/oauth/middleware.go +++ b/internal/oauth/middleware.go @@ -3,6 +3,7 @@ package oauth import ( "context" "database/sql" + "net/http" "time" "github.com/labstack/echo/v4" @@ -49,6 +50,14 @@ func SessionMiddleware(storage SessionStore) echo.MiddlewareFunc { if err := storage.DeleteSession(c.Request().Context(), cookie.Value); err != nil { c.Logger().Errorf("Failed to delete expired session: %v", err) } + // Clear the session cookie from browser + c.SetCookie(&http.Cookie{ + Name: "session", + Value: "", + Path: "/", + MaxAge: -1, + HttpOnly: true, + }) return next(c) } diff --git a/internal/oauth/middleware_test.go b/internal/oauth/middleware_test.go index 3bc0f2e..30a8c38 100644 --- a/internal/oauth/middleware_test.go +++ b/internal/oauth/middleware_test.go @@ -136,6 +136,18 @@ func TestSessionMiddleware(t *testing.T) { // Verify session was deleted from database _, err = storage.GetSessionByID(setupCtx.Request().Context(), session.ID) assert.Error(t, err, "Expired session should be deleted from database") + + // Verify session cookie was cleared in response + cookies := rec.Result().Cookies() + var sessionCookie *http.Cookie + for _, cookie := range cookies { + if cookie.Name == "session" { + sessionCookie = cookie + break + } + } + require.NotNil(t, sessionCookie, "Session cookie should be set in response") + assert.Equal(t, -1, sessionCookie.MaxAge, "Session cookie MaxAge should be -1 to clear it") }) }