From 2c6aac715ff14b2ac38409c01fa3ec271a7d6344 Mon Sep 17 00:00:00 2001 From: Abd Alrahaman Date: Sat, 10 Oct 2026 19:49:07 +0300 Subject: [PATCH] fix(mfa): preserve OAuth 2.1 client sessions during AAL step-up (closes #2801) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit InvalidateSessionsWithAALLessThan (sessions.go:353) is called from all four MFA step-up paths — verifyTOTPFactor, verifyPhoneFactor, verifyWebAuthnFactor (mfa.go:732/873/993), and recoveryCodesVerify (recovery_codes.go:437). Its current DELETE sweeps every session for the user with AAL below the requested level, which is correct for first-party sessions that need to be forced into re-authentication. OAuth 2.1 client sessions are born at AAL1 (NewSession hard-codes it; sessions.go:261) and never get the chance to step up to AAL2. The MFA sweep has therefore been silently wiping every MCP / OAuth integration a user has authorized the moment they verify a factor in a first-party surface: 1. User authorizes an OAuth / MCP client (session with oauth_client_id, AAL=aal1). 2. User later verifies TOTP / phone / WebAuthn / a recovery code in the web console or admin surface. 3. InvalidateSessionsWithAALLessThan runs. The OAuth client session matches `aal < AAL2` and is deleted. 4. The OAuth client's next POST /oauth/token refresh returns refresh_token_not_found. The integration disconnects silently. The model already distinguishes OAuth client sessions from first-party ones via Session.OAuthClientID (sessions.go:96), and already has a dedicated cleanup path for them via RevokeOAuthSessions (sessions.go:373, keyed on user_id AND oauth_client_id). The MFA delete path simply ignored this distinction. The fix is a single additional AND clause: oauth_client_id IS NULL. All four MFA callers want the same thing — invalidate first-party low-AAL sessions, leave OAuth client grants alone. OAuth session invalidation remains the responsibility of RevokeOAuthSessions. - internal/models/sessions.go * Add `AND oauth_client_id IS NULL` to the DELETE query. * Comment the function to explain the invariant for the next reader. - internal/models/sessions_test.go * New regression test TestInvalidateSessionsWithAALLessThan_PreservesOAuthClientSessions covering four scenarios: (1) aal1 OAuth session survives, (2) first-party aal1 session is still removed, (3) aal2 OAuth session also survives (the filter is defense in depth — ownership is by oauth_client_id, not AAL), (4) RevokeOAuthSessions still works as the sole authoritative cleanup. Confirmed call-site intent on all four MFA step-up callers; no other callers exist. Users affected and independently reproduced the trace: @breken-ai, @jhurleyai, @baptistesx, @kut237 on #2801. --- internal/models/sessions.go | 13 ++++++- internal/models/sessions_test.go | 60 ++++++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 1 deletion(-) diff --git a/internal/models/sessions.go b/internal/models/sessions.go index 92580d365a..50471ab5e8 100644 --- a/internal/models/sessions.go +++ b/internal/models/sessions.go @@ -350,8 +350,19 @@ func updateFactorAssociatedSessions(tx *storage.Connection, userID, factorID uui return tx.RawQuery("UPDATE "+(&pop.Model{Value: Session{}}).TableName()+" set aal = ?, factor_id = ? WHERE user_id = ? AND factor_id = ?", aal, nil, userID, factorID).Exec() } +// InvalidateSessionsWithAALLessThan deletes a user's sessions whose AAL is +// below the given level. All four callers are MFA step-up paths +// (verifyTOTPFactor / verifyPhoneFactor / verifyWebAuthnFactor / recoveryCodesVerify) +// and want to force re-authentication on the user's other first-party sessions +// after the step-up succeeds. +// +// OAuth 2.1 client sessions (identified by a non-NULL oauth_client_id) are +// explicitly preserved: they represent third-party-client consent grants whose +// lifecycle is managed independently via RevokeOAuthSessions. Including them +// in the MFA sweep breaks every MCP / OAuth integration the user has authorized +// the moment they verify a factor in a first-party surface (issue #2801). func InvalidateSessionsWithAALLessThan(tx *storage.Connection, userID uuid.UUID, level string) error { - return tx.RawQuery("DELETE FROM "+(&pop.Model{Value: Session{}}).TableName()+" WHERE user_id = ? AND (aal IS NULL OR aal < ?)", userID, level).Exec() + return tx.RawQuery("DELETE FROM "+(&pop.Model{Value: Session{}}).TableName()+" WHERE user_id = ? AND (aal IS NULL OR aal < ?) AND oauth_client_id IS NULL", userID, level).Exec() } // Logout deletes all sessions for a user. diff --git a/internal/models/sessions_test.go b/internal/models/sessions_test.go index 03fb82a570..1df70d98c9 100644 --- a/internal/models/sessions_test.go +++ b/internal/models/sessions_test.go @@ -87,6 +87,66 @@ func (ts *SessionsTestSuite) TestInvalidateSessionsWithAALLessThan() { require.Equal(ts.T(), aal2Session.ID, found.ID) } +// Regression test for #2801 — MFA step-up must not delete OAuth client sessions. +// Covers four scenarios: +// 1. aal1 OAuth client session survives (previously wiped — the user-visible bug) +// 2. aal1 first-party session alongside it is still removed (MFA still enforces step-up) +// 3. aal2 OAuth client session also survives (ownership is by oauth_client_id, not AAL) +// 4. RevokeOAuthSessions remains the sole authoritative path to drop OAuth sessions +func (ts *SessionsTestSuite) TestInvalidateSessionsWithAALLessThan_PreservesOAuthClientSessions() { + u, err := FindUserByEmailAndAudience(ts.db, "test@example.com", ts.Config.JWT.Aud) + require.NoError(ts.T(), err) + + oauthClientID := uuid.Must(uuid.NewV4()) + + // (1) OAuth client session at aal1 — should SURVIVE the MFA sweep. + oauthAAL1 := &Session{ + ID: uuid.Must(uuid.NewV4()), + UserID: u.ID, + AAL: AAL1.PointerString(), + OAuthClientID: &oauthClientID, + } + require.NoError(ts.T(), ts.db.Create(oauthAAL1)) + + // (2) First-party aal1 session — should be removed. + firstPartyAAL1, err := NewSession(u.ID, nil) + require.NoError(ts.T(), err) + require.NoError(ts.T(), ts.db.Create(firstPartyAAL1)) + + // (3) OAuth client session with AAL already at aal2 — SURVIVES unconditionally. + oauthAAL2 := &Session{ + ID: uuid.Must(uuid.NewV4()), + UserID: u.ID, + AAL: AAL2.PointerString(), + OAuthClientID: &oauthClientID, + } + require.NoError(ts.T(), ts.db.Create(oauthAAL2)) + + require.NoError(ts.T(), InvalidateSessionsWithAALLessThan(ts.db, u.ID, AAL2.String())) + + // (1) OAuth aal1 preserved. + foundOAuth1, err := FindSessionByID(ts.db, oauthAAL1.ID, false) + require.NoError(ts.T(), err, "OAuth client session at aal1 must survive MFA step-up (#2801)") + require.Equal(ts.T(), oauthAAL1.ID, foundOAuth1.ID) + + // (2) First-party aal1 removed. + _, err = FindSessionByID(ts.db, firstPartyAAL1.ID, false) + require.ErrorIs(ts.T(), err, SessionNotFoundError{}, + "first-party aal1 session must still be invalidated by MFA step-up") + + // (3) OAuth aal2 preserved. + foundOAuth2, err := FindSessionByID(ts.db, oauthAAL2.ID, false) + require.NoError(ts.T(), err) + require.Equal(ts.T(), oauthAAL2.ID, foundOAuth2.ID) + + // (4) RevokeOAuthSessions remains the authoritative cleanup for OAuth sessions. + require.NoError(ts.T(), RevokeOAuthSessions(ts.db, u.ID, oauthClientID)) + _, err = FindSessionByID(ts.db, oauthAAL1.ID, false) + require.ErrorIs(ts.T(), err, SessionNotFoundError{}) + _, err = FindSessionByID(ts.db, oauthAAL2.ID, false) + require.ErrorIs(ts.T(), err, SessionNotFoundError{}) +} + func (ts *SessionsTestSuite) AddClaimAndReloadSession(session *Session, claim AuthenticationMethod) *Session { err := AddClaimToSession(ts.db, session.ID, claim) require.NoError(ts.T(), err)