From 5d7a2d76ab8dbdb24bf3c3a4ff7d66dcf011c9b4 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Tue, 20 Oct 2026 09:16:23 +0100 Subject: [PATCH] fix(auth): reject sessions issued before the user's last password change A password change, reset or profile password update deletes the user's sessions in a separate, best-effort step after the new hash commits. If that delete failed, every old session stayed valid until it expired. A login that verified the old password could also insert its session after the delete had run, and that session survived the rotation too. API keys already close this gap with users.password_version (migration 000103, issue #402). Sessions now use the same counter: migration 000138 adds sessions.password_version, createSession stores the version of the user row whose password the login verified, and ValidateSession returns ErrSessionPasswordRotated when it differs from the user's current version. The trigger bumps the version in the password write itself, so revocation no longer depends on the delete succeeding. Existing sessions are backfilled with their owner's current version so the deploy does not sign everyone out. Closes #398 Co-Authored-By: Claude Opus 5.5 --- internal/auth/errors.go | 4 + internal/auth/service.go | 31 +++- internal/auth/service_helpers.go | 3 + internal/auth/service_helpers_test.go | 58 +++++++ internal/auth/service_password.go | 5 +- .../auth/service_session_rotation_db_test.go | 150 ++++++++++++++++++ internal/auth/store_postgres.go | 8 +- internal/auth/types.go | 3 + .../000138_session_password_version.down.sql | 3 + .../000138_session_password_version.up.sql | 19 +++ .../000138_session_password_version_test.go | 58 +++++++ 11 files changed, 330 insertions(+), 12 deletions(-) create mode 100644 internal/auth/service_session_rotation_db_test.go create mode 100644 internal/database/postgres/migrations/000138_session_password_version.down.sql create mode 100644 internal/database/postgres/migrations/000138_session_password_version.up.sql create mode 100644 internal/database/postgres/migrations/000138_session_password_version_test.go diff --git a/internal/auth/errors.go b/internal/auth/errors.go index af2753395..7d29ea85c 100644 --- a/internal/auth/errors.go +++ b/internal/auth/errors.go @@ -153,4 +153,8 @@ var ( // ErrAPIKeyPasswordRotated: the owner's password changed after the key was // minted (issue #402). ErrAPIKeyPasswordRotated = errors.New("API key predates the owner's last password change") + + // ErrSessionPasswordRotated: the user's password changed after the session + // was issued (issue #398). + ErrSessionPasswordRotated = errors.New("session predates the user's last password change") ) diff --git a/internal/auth/service.go b/internal/auth/service.go index edaa3795d..83c681f35 100644 --- a/internal/auth/service.go +++ b/internal/auth/service.go @@ -343,14 +343,8 @@ func (s *Service) ValidateSession(ctx context.Context, token string) (*Session, return nil, fmt.Errorf("session expired") } - // UpdateUser revokes sessions on deactivation; this rejects any session that - // outlives that (a failed delete, or Active flipped another way). Issue #89. - user, err := s.loadUser(ctx, session.UserID) - if err != nil { - return nil, fmt.Errorf("failed to load session user: %w", err) - } - if !user.Active { - return nil, fmt.Errorf("session not found") + if err := s.checkSessionUser(ctx, session); err != nil { + return nil, err } // Return a copy of the session with the original token (not the hash) for client use. @@ -360,6 +354,27 @@ func (s *Service) ValidateSession(ctx context.Context, token string) (*Session, return &result, nil } +// checkSessionUser rejects a session whose user can no longer use it. +func (s *Service) checkSessionUser(ctx context.Context, session *Session) error { + // UpdateUser revokes sessions on deactivation; this rejects any session that + // outlives that (a failed delete, or Active flipped another way). Issue #89. + user, err := s.loadUser(ctx, session.UserID) + if err != nil { + return fmt.Errorf("failed to load session user: %w", err) + } + if !user.Active { + return fmt.Errorf("session not found") + } + // A password change deletes sessions only best-effort, and a login that + // verified the old password can insert its session after that delete. + // The version bump in the password write itself is what revokes them + // (issue #398). + if session.PasswordVersion != user.PasswordVersion { + return ErrSessionPasswordRotated + } + return nil +} + // ValidateCSRFToken validates the CSRF token for a session. // // The expected CSRF token is derived as HMAC-SHA256(csrfKey, rawSessionToken), diff --git a/internal/auth/service_helpers.go b/internal/auth/service_helpers.go index 35eeaad34..19df575e5 100644 --- a/internal/auth/service_helpers.go +++ b/internal/auth/service_helpers.go @@ -94,6 +94,9 @@ func (s *Service) createSession(ctx context.Context, user *User, userAgent, ipAd UserAgent: userAgent, IPAddress: ipAddress, CSRFToken: csrfToken, // MAC stored for diagnostics only; not used in validation + // The version of the row whose password this login verified, so a + // rotation that commits after that read still revokes the session. + PasswordVersion: user.PasswordVersion, } if err := s.store.CreateSession(ctx, storedSession); err != nil { diff --git a/internal/auth/service_helpers_test.go b/internal/auth/service_helpers_test.go index 0f021b929..fc2c7bfdb 100644 --- a/internal/auth/service_helpers_test.go +++ b/internal/auth/service_helpers_test.go @@ -1,9 +1,12 @@ package auth import ( + "context" "testing" + "time" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" ) @@ -93,3 +96,58 @@ func TestContainsAny(t *testing.T) { assert.True(t, containsAny(allowed, requested)) }) } + +// A session records the password version of the user row its login verified +// (issue #398). +func TestCreateSession_RecordsPasswordVersion(t *testing.T) { + ctx := context.Background() + mockStore := new(MockStore) + service := createTestService(mockStore, new(MockEmailSender)) + + var stored *Session + mockStore.On("CreateSession", ctx, mock.AnythingOfType("*auth.Session")). + Run(func(args mock.Arguments) { stored = args.Get(1).(*Session) }).Return(nil).Once() + + _, err := service.createSession(ctx, &User{ID: "user-123", Email: "test@example.com", PasswordVersion: 3}, "", "") + require.NoError(t, err) + require.NotNil(t, stored) + assert.Equal(t, int64(3), stored.PasswordVersion) + mockStore.AssertExpectations(t) +} + +// ValidateSession rejects a session issued under another password version, +// whether or not the best-effort session delete ran (issue #398). +func TestValidateSession_PasswordVersion(t *testing.T) { + ctx := context.Background() + for _, tc := range []struct { + name string + sessionVersion int64 + userVersion int64 + wantErr error + }{ + {name: "same version", sessionVersion: 2, userVersion: 2}, + {name: "password changed after login", sessionVersion: 1, userVersion: 2, wantErr: ErrSessionPasswordRotated}, + {name: "session newer than the user row", sessionVersion: 3, userVersion: 2, wantErr: ErrSessionPasswordRotated}, + } { + t.Run(tc.name, func(t *testing.T) { + mockStore := new(MockStore) + service := createTestService(mockStore, new(MockEmailSender)) + hashed := hashSessionToken("raw-token") + mockStore.On("GetSession", ctx, hashed).Return(&Session{ + Token: hashed, UserID: "user-123", ExpiresAt: time.Now().Add(time.Hour), PasswordVersion: tc.sessionVersion, + }, nil).Once() + mockStore.On("GetUserByID", ctx, "user-123"). + Return(&User{ID: "user-123", Active: true, PasswordVersion: tc.userVersion}, nil).Once() + + session, err := service.ValidateSession(ctx, "raw-token") + if tc.wantErr != nil { + require.ErrorIs(t, err, tc.wantErr) + assert.Nil(t, session) + } else { + require.NoError(t, err) + assert.Equal(t, "raw-token", session.Token) + } + mockStore.AssertExpectations(t) + }) + } +} diff --git a/internal/auth/service_password.go b/internal/auth/service_password.go index 3be2d3027..b4557bb1b 100644 --- a/internal/auth/service_password.go +++ b/internal/auth/service_password.go @@ -215,7 +215,10 @@ func (s *Service) checkCommonPasswords(password string) error { // attacker out (issue #102). // // Both cleanups are best-effort: failures are logged, not returned, because -// the new password has already taken effect by the time this runs. Shared +// the new password has already taken effect by the time this runs. They are +// not what enforces the rotation: the password write bumps +// users.password_version, and ValidateSession and ValidateUserAPIKey reject +// any credential issued under an older version (issues #398, #402). Shared // by ChangePassword, ConfirmPasswordReset and UpdateUserProfile so the two // checks live in one place instead of three. func (s *Service) invalidateUserCredentialsBestEffort(ctx context.Context, userID, opName string) { diff --git a/internal/auth/service_session_rotation_db_test.go b/internal/auth/service_session_rotation_db_test.go new file mode 100644 index 000000000..142f0e1d5 --- /dev/null +++ b/internal/auth/service_session_rotation_db_test.go @@ -0,0 +1,150 @@ +//go:build integration + +package auth + +import ( + "context" + "errors" + "sync" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +// failingInvalidationStore makes both best-effort cleanups after a password +// change fail, as a transient database error would (issue #398). +type failingInvalidationStore struct{ failingKeyListStore } + +func (failingInvalidationStore) DeleteUserSessions(context.Context, string) error { + return errors.New("synthetic session delete failure") +} + +// sessionReadHook runs onRead once, right after the first GetSession returns. +type sessionReadHook struct { + *PostgresStore + onRead func() +} + +func (s *sessionReadHook) GetSession(ctx context.Context, token string) (*Session, error) { + session, err := s.PostgresStore.GetSession(ctx, token) + if hook := s.onRead; hook != nil { + s.onRead = nil + hook() + } + return session, err +} + +func loginRotationUser(t *testing.T, svc *Service, email, password string) string { + t.Helper() + resp, err := svc.Login(t.Context(), LoginRequest{Email: email, Password: password}) + require.NoError(t, err) + return resp.Token +} + +// rotatePassword changes the user's password through each service entry point +// that rotates it, using svc for the write. +var rotatePassword = map[string]func(t *testing.T, svc *Service, store *PostgresStore, user *User){ + "change-password": func(t *testing.T, svc *Service, _ *PostgresStore, user *User) { + require.NoError(t, svc.ChangePassword(t.Context(), user.ID, + ChangePasswordRequest{CurrentPassword: rotationPassword, NewPassword: rotationNewPassword})) + }, + "update-profile": func(t *testing.T, svc *Service, _ *PostgresStore, user *User) { + require.NoError(t, svc.UpdateUserProfile(t.Context(), user.ID, user.Email, rotationPassword, rotationNewPassword)) + }, + "reset-confirm": func(t *testing.T, svc *Service, store *PostgresStore, user *User) { + token := "session-rotation-reset-" + user.ID + _, err := store.db.Exec(t.Context(), `UPDATE users SET password_reset_token = $2, + password_reset_expiry = NOW() + interval '1 hour' WHERE id = $1`, user.ID, hashSessionToken(token)) + require.NoError(t, err) + require.NoError(t, svc.ConfirmPasswordReset(t.Context(), PasswordResetConfirm{Token: token, NewPassword: rotationNewPassword})) + }, +} + +func TestIntegration_SessionRejectedAfterPasswordRotation(t *testing.T) { + store := NewPostgresStore(setupAuthTestDB(t)) + svc := newRotationService(store) + ctx := t.Context() + change := ChangePasswordRequest{CurrentPassword: rotationPassword, NewPassword: rotationNewPassword} + expiry := time.Now().Add(time.Hour) + perms := []Permission{{Action: ActionView, Resource: ResourceRecommendations}} + + for name, rotate := range rotatePassword { + t.Run("invalidation-fails/"+name, func(t *testing.T) { + user := createRotationUser(t, svc, store, "session-invalidation-"+name+"@example.com") + session := loginRotationUser(t, svc, user.Email, rotationPassword) + apiKey, _, err := svc.CreateAPIKey(ctx, user.ID, "ci", rotationPassword, perms, &expiry) + require.NoError(t, err) + rotate(t, newRotationService(failingInvalidationStore{failingKeyListStore{store}}), store, user) + + _, err = svc.ValidateSession(ctx, session) + require.ErrorIs(t, err, ErrSessionPasswordRotated) + _, _, err = svc.ValidateUserAPIKey(ctx, apiKey) + require.ErrorIs(t, err, ErrAPIKeyPasswordRotated, "already enforced by migration 000103 (issue #402)") + + fresh := loginRotationUser(t, svc, user.Email, rotationNewPassword) + _, err = svc.ValidateSession(ctx, fresh) + require.NoError(t, err, "a session issued after the change must stay valid") + }) + } + + t.Run("login-across-rotation", func(t *testing.T) { + user := createRotationUser(t, svc, store, "session-login-race@example.com") + racing := &credentialReadBarrier{StoreInterface: store, afterRead: func(ctx context.Context, _ *User) { + require.NoError(t, svc.ChangePassword(ctx, user.ID, change)) + }} + session := loginRotationUser(t, newRotationService(racing), user.Email, rotationPassword) + + _, err := svc.ValidateSession(ctx, session) + require.ErrorIs(t, err, ErrSessionPasswordRotated, + "the login verified the password it read before the rotation, after the session sweep ran") + }) + + t.Run("session-use-across-rotation", func(t *testing.T) { + user := createRotationUser(t, svc, store, "session-use-race@example.com") + session := loginRotationUser(t, svc, user.Email, rotationPassword) + racing := &sessionReadHook{PostgresStore: store, onRead: func() { + require.NoError(t, svc.ChangePassword(ctx, user.ID, change)) + }} + + _, err := newRotationService(racing).ValidateSession(ctx, session) + require.ErrorIs(t, err, ErrSessionPasswordRotated) + }) + + t.Run("concurrent-logins", func(t *testing.T) { + user := createRotationUser(t, svc, store, "session-concurrent-logins@example.com") + var ( + mu sync.Mutex + tokens []string + wg sync.WaitGroup + ) + started := make(chan struct{}) + var once sync.Once + // Each client logs in with the old password until the rotation makes + // it fail, so some logins straddle the rotation's commit and sweep. + for range 16 { + wg.Go(func() { + for range 1000 { + resp, err := svc.Login(ctx, LoginRequest{Email: user.Email, Password: rotationPassword}) + if err != nil { + return + } + mu.Lock() + tokens = append(tokens, resp.Token) + mu.Unlock() + once.Do(func() { close(started) }) + } + }) + } + <-started + require.NoError(t, svc.ChangePassword(ctx, user.ID, change)) + wg.Wait() + + // A session minted before the sweep is deleted; one minted after it + // survives the sweep and must be rejected by its password version. + for _, token := range tokens { + _, err := svc.ValidateSession(ctx, token) + require.Error(t, err, "every session from the old password must be rejected") + } + }) +} diff --git a/internal/auth/store_postgres.go b/internal/auth/store_postgres.go index 052ac8131..9f8909aa2 100644 --- a/internal/auth/store_postgres.go +++ b/internal/auth/store_postgres.go @@ -641,8 +641,8 @@ func (s *PostgresStore) CreateSession(ctx context.Context, session *Session) err query := ` INSERT INTO sessions ( token, user_id, email, expires_at, created_at, - user_agent, ip_address, csrf_token - ) VALUES ($1, $2, $3, $4, $5, $6, $7, $8) + user_agent, ip_address, csrf_token, password_version + ) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9) ` _, err := s.db.Exec(ctx, query, @@ -654,6 +654,7 @@ func (s *PostgresStore) CreateSession(ctx context.Context, session *Session) err session.UserAgent, session.IPAddress, session.CSRFToken, + session.PasswordVersion, ) if err != nil { @@ -667,7 +668,7 @@ func (s *PostgresStore) CreateSession(ctx context.Context, session *Session) err func (s *PostgresStore) GetSession(ctx context.Context, token string) (*Session, error) { query := ` SELECT token, user_id, email, expires_at, created_at, - user_agent, ip_address, csrf_token + user_agent, ip_address, csrf_token, password_version FROM sessions WHERE token = $1 AND expires_at > NOW() ` @@ -682,6 +683,7 @@ func (s *PostgresStore) GetSession(ctx context.Context, token string) (*Session, &session.UserAgent, &session.IPAddress, &session.CSRFToken, + &session.PasswordVersion, ) if err != nil { diff --git a/internal/auth/types.go b/internal/auth/types.go index 898465075..9c8f60863 100644 --- a/internal/auth/types.go +++ b/internal/auth/types.go @@ -259,6 +259,9 @@ type Session struct { UserAgent string `json:"user_agent,omitempty" dynamodbav:"UserAgent"` IPAddress string `json:"ip_address,omitempty" dynamodbav:"IPAddress"` CSRFToken string `json:"csrf_token,omitempty" dynamodbav:"CSRFToken"` + // PasswordVersion is the user's User.PasswordVersion read by the login that + // issued the session; ValidateSession rejects the session once they differ. + PasswordVersion int64 `json:"-" dynamodbav:"PasswordVersion"` } // LoginRequest represents a login attempt. diff --git a/internal/database/postgres/migrations/000138_session_password_version.down.sql b/internal/database/postgres/migrations/000138_session_password_version.down.sql new file mode 100644 index 000000000..ac15b71d1 --- /dev/null +++ b/internal/database/postgres/migrations/000138_session_password_version.down.sql @@ -0,0 +1,3 @@ +-- Reverse 000138: sessions are no longer bound to the password version, and a +-- password change again relies on deleting them (issue #398). +ALTER TABLE sessions DROP COLUMN IF EXISTS password_version; diff --git a/internal/database/postgres/migrations/000138_session_password_version.up.sql b/internal/database/postgres/migrations/000138_session_password_version.up.sql new file mode 100644 index 000000000..54c83cd58 --- /dev/null +++ b/internal/database/postgres/migrations/000138_session_password_version.up.sql @@ -0,0 +1,19 @@ +-- Migration 000138: bind sessions to the user's password (issue #398). +-- +-- A password change deletes the user's sessions in a separate, best-effort +-- step after the new hash commits. A session survived that change when the +-- delete failed, or when a login that had verified the old password inserted +-- its session after the delete ran. A session now records the +-- users.password_version (migration 000103) its login read, and +-- ValidateSession rejects it once the two differ, so the trigger's bump in the +-- password write itself revokes every earlier session. +-- +-- Existing sessions take their owner's current version, so the deploy does not +-- sign everyone out. Any that outlived a failed delete before this migration +-- stay valid until they expire (24 hours by default). +ALTER TABLE sessions ADD COLUMN password_version BIGINT NOT NULL DEFAULT 0; + +UPDATE sessions s +SET password_version = u.password_version +FROM users u +WHERE u.id = s.user_id; diff --git a/internal/database/postgres/migrations/000138_session_password_version_test.go b/internal/database/postgres/migrations/000138_session_password_version_test.go new file mode 100644 index 000000000..8586c1068 --- /dev/null +++ b/internal/database/postgres/migrations/000138_session_password_version_test.go @@ -0,0 +1,58 @@ +//go:build integration + +package migrations_test + +import ( + "context" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/migrations" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" + "github.com/stretchr/testify/require" +) + +// TestMigration_SessionPasswordVersion covers 000138 up, the backfill and down (#398). +func TestMigration_SessionPasswordVersion(t *testing.T) { + ctx := context.Background() + container := testhelpers.RequirePostgresContainer(ctx, t) + t.Cleanup(func() { require.NoError(t, container.Cleanup(context.Background())) }) + pool, path := container.DB.Pool(), getMigrationsPath() + + hasColumn := func() bool { + var n int + require.NoError(t, pool.QueryRow(ctx, `SELECT count(*) FROM information_schema.columns + WHERE table_name = 'sessions' AND column_name = 'password_version'`).Scan(&n)) + return n == 1 + } + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, previousMigrationVersion(t, 138))) + require.False(t, hasColumn()) + + var userID string + require.NoError(t, pool.QueryRow(ctx, `INSERT INTO users (email, password_hash, salt, group_ids) + VALUES ('session-version@example.com', 'hash-1', '', ARRAY[$1::uuid]) RETURNING id`, + adminGroupIDForMinAdminTest).Scan(&userID)) + _, err := pool.Exec(ctx, `UPDATE users SET password_hash = 'hash-2' WHERE id = $1`, userID) + require.NoError(t, err) + _, err = pool.Exec(ctx, `UPDATE users SET password_hash = 'hash-3' WHERE id = $1`, userID) + require.NoError(t, err) + _, err = pool.Exec(ctx, `INSERT INTO sessions (token, user_id, email, expires_at) + VALUES ('existing-session', $1, 'session-version@example.com', NOW() + interval '1 hour')`, userID) + require.NoError(t, err) + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, 138)) + var version int64 + require.NoError(t, pool.QueryRow(ctx, `SELECT password_version FROM sessions WHERE token = 'existing-session'`).Scan(&version)) + require.Equal(t, int64(2), version, "an existing session takes its owner's current version") + + _, err = pool.Exec(ctx, `INSERT INTO sessions (token, user_id, email, expires_at) + VALUES ('writer-without-version', $1, 'session-version@example.com', NOW() + interval '1 hour')`, userID) + require.NoError(t, err) + require.NoError(t, pool.QueryRow(ctx, `SELECT password_version FROM sessions WHERE token = 'writer-without-version'`).Scan(&version)) + require.Zero(t, version, "a writer that omits the version gets 0, which fails closed once the user has rotated") + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, previousMigrationVersion(t, 138))) + require.False(t, hasColumn()) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, 138)) + require.True(t, hasColumn()) +}