Skip to content

Commit e69d67a

Browse files
gomitrahgomitrah
committed
fix(auth): clear stale id_token when session regenerates without one
1 parent 45e8628 commit e69d67a

6 files changed

Lines changed: 158 additions & 21 deletions

File tree

‎routers/web/auth/auth.go‎

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -119,9 +119,13 @@ func autoSignIn(ctx *context.Context) (bool, error) {
119119

120120
ctx.SetSiteCookie(setting.CookieRememberName, nt.ID+":"+token, setting.LogInRememberDays*timeutil.Day)
121121

122+
// clear even if unset: a stale OAuth2 sign-in method/id_token from an earlier session
123+
// on this browser must not survive an auto sign-in from the remember-me cookie
122124
if err := regenerateSession(ctx, map[string]any{
123125
session.KeyUID: u.ID,
124126
session.KeyUserHasTwoFactorAuth: userHasTwoFactorAuth,
127+
session.KeySignInMethod: "",
128+
session.KeyOIDCIDToken: "",
125129
}); err != nil {
126130
return false, fmt.Errorf("unable to updateSession: %w", err)
127131
}
@@ -339,7 +343,12 @@ func SignInPost(ctx *context.Context) {
339343
return
340344
}
341345

342-
handleTwoFactorRequired(ctx, u, form.Remember, nil)
346+
// clear even if unset: a stale OAuth2 sign-in method/id_token from an earlier session
347+
// on this browser must not survive into a password-initiated 2FA flow
348+
handleTwoFactorRequired(ctx, u, form.Remember, map[string]any{
349+
session.KeySignInMethod: "",
350+
session.KeyOIDCIDToken: "",
351+
})
343352
}
344353

345354
func handleTwoFactorRequired(ctx *context.Context, u *user_model.User, remember bool, extra map[string]any) {
@@ -387,10 +396,22 @@ func handleSignInFull(ctx *context.Context, u *user_model.User, remember bool) {
387396
return
388397
}
389398

399+
// a pending 2FA (twofaUid set) means handleTwoFactorRequired already resolved
400+
// KeySignInMethod/KeyOIDCIDToken for this flow (real OIDC values, or explicitly
401+
// cleared) — carry that forward. Otherwise this is a direct, non-2FA sign-in, so
402+
// any leftover value from an unrelated earlier session must not survive.
403+
var signInMethod, idToken string
404+
if ctx.Session.Get("twofaUid") != nil {
405+
signInMethod, _ = ctx.Session.Get(session.KeySignInMethod).(string)
406+
idToken, _ = ctx.Session.Get(session.KeyOIDCIDToken).(string)
407+
}
408+
390409
auth_service.ClearSessionKeysForSignIn(ctx.Session)
391410
if err := regenerateSession(ctx, map[string]any{
392411
session.KeyUID: u.ID,
393412
session.KeyUserHasTwoFactorAuth: userHasTwoFactorAuth,
413+
session.KeySignInMethod: signInMethod,
414+
session.KeyOIDCIDToken: idToken,
394415
}); err != nil {
395416
ctx.ServerError("RegenerateSession", err)
396417
return

‎routers/web/auth/auth_test.go‎

Lines changed: 111 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import (
1717
"gitea.dev/modules/setting"
1818
"gitea.dev/modules/test"
1919
"gitea.dev/modules/util"
20+
auth_service "gitea.dev/services/auth"
2021
"gitea.dev/services/auth/source/oauth2"
2122
"gitea.dev/services/contexttest"
2223

@@ -218,7 +219,7 @@ func TestWebAuthOAuth2(t *testing.T) {
218219
ctx.SetPathParamRaw("provider", "oidc-auth-source")
219220
SignInOAuthCallback(ctx)
220221
require.Equal(t, http.StatusSeeOther, resp.Code)
221-
assert.Nil(t, ctx.Session.Get(session.KeyOIDCIDToken))
222+
assert.Empty(t, ctx.Session.Get(session.KeyOIDCIDToken))
222223

223224
ctx, resp = contexttest.MockContext(t, "/user/logout", mockOpt)
224225
ctx.Doer = oauthUser
@@ -229,6 +230,91 @@ func TestWebAuthOAuth2(t *testing.T) {
229230
assert.Empty(t, u.Query().Get("id_token_hint"))
230231
})
231232

233+
t.Run("OAuth2CallbackClearsStaleIDTokenOnRegen", func(t *testing.T) {
234+
// simulates a user who signed in before with an id_token (stale value already
235+
// in the session), then signs in again via a callback that yields no id_token
236+
mockOpt := contexttest.MockContextOption{SessionStore: session.NewMockMemStore("dummy-sid-oidc-stale-token")}
237+
ctx, resp := contexttest.MockContext(t, "/user/oauth2/..../callback?code=dummy-code", mockOpt)
238+
require.NoError(t, ctx.Session.Set(session.KeyOIDCIDToken, "stale-id-token-from-prior-login"))
239+
ctx.Doer = oauthUser
240+
handleOAuth2SignIn(ctx, authSource, oauthUser, goth.User{Provider: "oidc-auth-source", UserID: "oauth-user"})
241+
require.Equal(t, http.StatusSeeOther, resp.Code)
242+
assert.Empty(t, ctx.Session.Get(session.KeyOIDCIDToken), "stale id_token from a prior session must not survive regenerateSession")
243+
244+
ctx, resp = contexttest.MockContext(t, "/user/logout", mockOpt)
245+
ctx.Doer = oauthUser
246+
SignOut(ctx)
247+
assert.Equal(t, http.StatusSeeOther, resp.Code)
248+
u, err := url.Parse(test.RedirectURL(resp))
249+
require.NoError(t, err)
250+
assert.Empty(t, u.Query().Get("id_token_hint"), "logout must not reuse a stale id_token as id_token_hint")
251+
})
252+
253+
t.Run("OAuth2CallbackClearsStaleIDTokenOnRegenWith2FA", func(t *testing.T) {
254+
// same as OAuth2CallbackClearsStaleIDTokenOnRegen, but through the 2FA-required
255+
// branch in handleOAuth2SignIn, a separate call site from the !needs2FA one
256+
tfa := &auth_model.TwoFactor{UID: oauthUser.ID}
257+
require.NoError(t, auth_model.NewTwoFactor(t.Context(), tfa))
258+
t.Cleanup(func() { _ = auth_model.DeleteTwoFactorByID(t.Context(), tfa.ID, oauthUser.ID) })
259+
260+
mockOpt := contexttest.MockContextOption{SessionStore: session.NewMockMemStore("dummy-sid-oidc-stale-token-2fa")}
261+
ctx, resp := contexttest.MockContext(t, "/user/oauth2/..../callback?code=dummy-code", mockOpt)
262+
require.NoError(t, ctx.Session.Set(session.KeyOIDCIDToken, "stale-id-token-from-prior-login"))
263+
ctx.Doer = oauthUser
264+
handleOAuth2SignIn(ctx, authSource, oauthUser, goth.User{Provider: "oidc-auth-source", UserID: "oauth-user"})
265+
require.Equal(t, http.StatusSeeOther, resp.Code)
266+
assert.Equal(t, "/user/two_factor", test.RedirectURL(resp))
267+
assert.Empty(t, ctx.Session.Get(session.KeyOIDCIDToken), "stale id_token must not survive the 2FA-required regeneration branch")
268+
})
269+
270+
t.Run("PasswordSignInAfterOIDCClearsStaleIDTokenAndMethod", func(t *testing.T) {
271+
// a session that previously authenticated via OIDC (KeySignInMethod + KeyOIDCIDToken
272+
// set) must not keep redirecting to end_session_endpoint with a stale hint once the
273+
// same browser session re-authenticates via password: handleSignInFull's regenerateSession
274+
// call doesn't pass either key, so ClearSessionKeysForSignIn must be the one clearing them
275+
mockOpt := contexttest.MockContextOption{SessionStore: session.NewMockMemStore("dummy-sid-password-after-oidc")}
276+
ctx, _ := contexttest.MockContext(t, "/user/login", mockOpt)
277+
require.NoError(t, ctx.Session.Set(session.KeySignInMethod, session.SignInMethodOAuth2))
278+
require.NoError(t, ctx.Session.Set(session.KeyOIDCIDToken, "stale-id-token-from-prior-oidc-login"))
279+
280+
handleSignInFull(ctx, oauthUser, false)
281+
assert.NotEqual(t, session.SignInMethodOAuth2, ctx.Session.Get(session.KeySignInMethod), "password sign-in must not leave a stale OAuth2 sign-in method behind")
282+
assert.Empty(t, ctx.Session.Get(session.KeyOIDCIDToken), "password sign-in must not leave a stale id_token behind")
283+
284+
ctx, resp := contexttest.MockContext(t, "/user/logout", mockOpt)
285+
ctx.Doer = oauthUser
286+
SignOut(ctx)
287+
assert.Equal(t, http.StatusSeeOther, resp.Code)
288+
assert.Equal(t, "/", test.RedirectURL(resp), "must not redirect to the OIDC end_session_endpoint after a password sign-in")
289+
})
290+
291+
t.Run("OIDCIDTokenSurvives2FACompletion", func(t *testing.T) {
292+
// counterpart to PasswordSignInAfterOIDCClearsStaleIDTokenAndMethod: an OIDC sign-in
293+
// that required 2FA sets KeySignInMethod/KeyOIDCIDToken via handleTwoFactorRequired
294+
// *before* the 2FA step; handleSignIn (called on successful 2FA) must preserve them,
295+
// not just blindly clear on every sign-in completion
296+
mockOpt := contexttest.MockContextOption{SessionStore: session.NewMockMemStore("dummy-sid-oidc-2fa-completion")}
297+
ctx, _ := contexttest.MockContext(t, "/user/oauth2/..../callback?code=dummy-code", mockOpt)
298+
handleTwoFactorRequired(ctx, oauthUser, false, map[string]any{
299+
session.KeySignInMethod: session.SignInMethodOAuth2,
300+
session.KeyOIDCIDToken: "real-id-token-pending-2fa",
301+
})
302+
303+
// simulates what TwoFactorPost does on a correct passcode
304+
ctx, _ = contexttest.MockContext(t, "/user/twofa", mockOpt)
305+
handleSignIn(ctx, oauthUser, false)
306+
assert.Equal(t, session.SignInMethodOAuth2, ctx.Session.Get(session.KeySignInMethod), "completing 2FA must not lose the OAuth2 sign-in method recorded before the 2FA step")
307+
assert.Equal(t, "real-id-token-pending-2fa", ctx.Session.Get(session.KeyOIDCIDToken), "completing 2FA must not lose the id_token recorded before the 2FA step")
308+
309+
ctx, resp := contexttest.MockContext(t, "/user/logout", mockOpt)
310+
ctx.Doer = oauthUser
311+
SignOut(ctx)
312+
assert.Equal(t, http.StatusSeeOther, resp.Code)
313+
u, err := url.Parse(test.RedirectURL(resp))
314+
require.NoError(t, err)
315+
assert.Equal(t, "real-id-token-pending-2fa", u.Query().Get("id_token_hint"), "logout after 2FA completion must still include the id_token_hint from the original OIDC sign-in")
316+
})
317+
232318
t.Run("PasswordSignInSkipsOIDC", func(t *testing.T) {
233319
// OAuth2-linked account signed in via password form must not hit end_session_endpoint.
234320
mockOpt := contexttest.MockContextOption{SessionStore: session.NewMockMemStore("dummy-sid-password")}
@@ -247,12 +333,36 @@ func TestOpenIDRequireTwoFactor(t *testing.T) {
247333

248334
user32 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 32}) // has a webauthn credential
249335
ctx, resp := contexttest.MockContext(t, "/user/openid/connect", mockOpt)
336+
require.NoError(t, ctx.Session.Set(session.KeySignInMethod, session.SignInMethodOAuth2))
337+
require.NoError(t, ctx.Session.Set(session.KeyOIDCIDToken, "stale-id-token-from-prior-oidc-login"))
250338
openIDRequireTwoFactor(ctx, user32, false, "https://example.com/id")
251339
assert.Equal(t, "/user/webauthn", test.RedirectURL(resp))
252340
unittest.AssertNotExistsBean(t, &user_model.UserOpenID{UID: user32.ID}) // not attached before the key answered
341+
assert.Empty(t, ctx.Session.Get(session.KeyOIDCIDToken), "legacy OpenID 2FA flow must not carry forward a stale id_token from an earlier OIDC session")
342+
assert.NotEqual(t, session.SignInMethodOAuth2, ctx.Session.Get(session.KeySignInMethod))
253343

254344
user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
255345
ctx, _ = contexttest.MockContext(t, "/user/openid/connect", mockOpt)
256346
openIDRequireTwoFactor(ctx, user2, false, "https://example.com/id")
257347
assert.False(t, ctx.Written())
258348
}
349+
350+
func TestAutoSignInClearsStaleOIDCState(t *testing.T) {
351+
require.NoError(t, unittest.PrepareTestDatabase())
352+
user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
353+
354+
nt, token, err := auth_service.CreateAuthTokenForUserID(t.Context(), user2.ID)
355+
require.NoError(t, err)
356+
357+
mockOpt := contexttest.MockContextOption{SessionStore: session.NewMockMemStore("dummy-sid-autologin")}
358+
ctx, _ := contexttest.MockContext(t, "/user/login", mockOpt)
359+
require.NoError(t, ctx.Session.Set(session.KeySignInMethod, session.SignInMethodOAuth2))
360+
require.NoError(t, ctx.Session.Set(session.KeyOIDCIDToken, "stale-id-token-from-prior-oidc-login"))
361+
ctx.Req.AddCookie(&http.Cookie{Name: setting.CookieRememberName, Value: nt.ID + ":" + token})
362+
363+
succeeded, err := autoSignIn(ctx)
364+
require.NoError(t, err)
365+
require.True(t, succeeded)
366+
assert.Empty(t, ctx.Session.Get(session.KeyOIDCIDToken), "auto sign-in via remember-me cookie must not carry forward a stale id_token")
367+
assert.NotEqual(t, session.SignInMethodOAuth2, ctx.Session.Get(session.KeySignInMethod))
368+
}

‎routers/web/auth/linkaccount.go‎

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -168,14 +168,11 @@ func oauth2LinkAccount(ctx *context.Context, u *user_model.User, linkAccountData
168168
return
169169
}
170170

171-
extra := map[string]any{
171+
handleTwoFactorRequired(ctx, u, remember, map[string]any{
172172
"linkAccount": true,
173173
session.KeySignInMethod: session.SignInMethodOAuth2,
174-
}
175-
if linkAccountData.GothUser.IDToken != "" {
176-
extra[session.KeyOIDCIDToken] = linkAccountData.GothUser.IDToken
177-
}
178-
handleTwoFactorRequired(ctx, u, remember, extra)
174+
session.KeyOIDCIDToken: linkAccountData.GothUser.IDToken, // set even if "": clears any stale token regenerateSession would otherwise carry over
175+
})
179176
}
180177

181178
// LinkAccountPostRegister handle the creation of a new account for an external account using signUp

‎routers/web/auth/oauth.go‎

Lines changed: 7 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -429,15 +429,12 @@ func handleOAuth2SignIn(ctx *context.Context, authSource *auth.Source, u *user_m
429429
return
430430
}
431431

432-
sessionData := map[string]any{
432+
if err := regenerateSession(ctx, map[string]any{
433433
session.KeyUID: u.ID,
434434
session.KeyUserHasTwoFactorAuth: userHasTwoFactorAuth,
435435
session.KeySignInMethod: session.SignInMethodOAuth2,
436-
}
437-
if gothUser.IDToken != "" {
438-
sessionData[session.KeyOIDCIDToken] = gothUser.IDToken
439-
}
440-
if err := regenerateSession(ctx, sessionData); err != nil {
436+
session.KeyOIDCIDToken: gothUser.IDToken, // set even if "": clears any stale token regenerateSession would otherwise carry over
437+
}); err != nil {
441438
ctx.ServerError("updateSession", err)
442439
return
443440
}
@@ -458,11 +455,10 @@ func handleOAuth2SignIn(ctx *context.Context, authSource *auth.Source, u *user_m
458455
}
459456
}
460457

461-
extra := map[string]any{session.KeySignInMethod: session.SignInMethodOAuth2}
462-
if gothUser.IDToken != "" {
463-
extra[session.KeyOIDCIDToken] = gothUser.IDToken
464-
}
465-
handleTwoFactorRequired(ctx, u, false, extra)
458+
handleTwoFactorRequired(ctx, u, false, map[string]any{
459+
session.KeySignInMethod: session.SignInMethodOAuth2,
460+
session.KeyOIDCIDToken: gothUser.IDToken, // set even if "": see regenerateSession call above
461+
})
466462
}
467463

468464
// OAuth2UserLoginCallback attempts to handle the callback from the OAuth2 provider and if successful

‎routers/web/auth/openid.go‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import (
1212
user_model "gitea.dev/models/user"
1313
"gitea.dev/modules/auth/openid"
1414
"gitea.dev/modules/log"
15+
"gitea.dev/modules/session"
1516
"gitea.dev/modules/setting"
1617
"gitea.dev/modules/templates"
1718
"gitea.dev/modules/util"
@@ -37,7 +38,13 @@ func openIDRequireTwoFactor(ctx *context.Context, u *user_model.User, remember b
3738
if !hasTwoFactor {
3839
return
3940
}
40-
handleTwoFactorRequired(ctx, u, remember, map[string]any{"openidPendingURI": pendingURI})
41+
// clear even if unset: a stale OAuth2 sign-in method/id_token from an earlier session
42+
// on this browser must not survive a legacy-OpenID-initiated 2FA flow
43+
handleTwoFactorRequired(ctx, u, remember, map[string]any{
44+
"openidPendingURI": pendingURI,
45+
session.KeySignInMethod: "",
46+
session.KeyOIDCIDToken: "",
47+
})
4148
}
4249

4350
func openIDConnectFromContext(ctx *context.Context, u *user_model.User) error {

‎routers/web/auth/password.go‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import (
1212
"gitea.dev/modules/auth/password"
1313
"gitea.dev/modules/log"
1414
"gitea.dev/modules/optional"
15+
"gitea.dev/modules/session"
1516
"gitea.dev/modules/setting"
1617
"gitea.dev/modules/templates"
1718
"gitea.dev/modules/timeutil"
@@ -246,7 +247,12 @@ func ResetPasswdPost(ctx *context.Context) {
246247
return
247248
}
248249
if hasWebAuthn {
249-
handleTwoFactorRequired(ctx, u, remember, nil)
250+
// clear even if unset: a stale OAuth2 sign-in method/id_token from an earlier
251+
// session on this browser must not survive a password-reset-initiated 2FA flow
252+
handleTwoFactorRequired(ctx, u, remember, map[string]any{
253+
session.KeySignInMethod: "",
254+
session.KeyOIDCIDToken: "",
255+
})
250256
return
251257
}
252258
}

0 commit comments

Comments
 (0)