From a63ce34ae98128034aca878ad24d945ea0470b55 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sat, 18 Dec 2021 12:26:35 +0000 Subject: [PATCH 1/6] Reset Session ID on login When logging in the SessionID should be reset and the session cleaned up. Signed-off-by: Andrew Thornton --- routers/web/user/auth.go | 85 ++++++++++++++++++++++++++++++++-------- 1 file changed, 68 insertions(+), 17 deletions(-) diff --git a/routers/web/user/auth.go b/routers/web/user/auth.go index 0f1ede85a73a0..7e1b9d7c2b404 100644 --- a/routers/web/user/auth.go +++ b/routers/web/user/auth.go @@ -90,14 +90,23 @@ func AutoSignIn(ctx *context.Context) (bool, error) { isSucceed = true + // Save the session first + _ = ctx.Session.Release() + + // Then regenerate the ID - which should copy the previous data + newSess, _ := ctx.Session.RegenerateID(ctx.Resp, ctx.Req) + + // Then flush the old session to delete it + _ = ctx.Session.Flush() + // Set session IDs - if err := ctx.Session.Set("uid", u.ID); err != nil { + if err := newSess.Set("uid", u.ID); err != nil { return false, err } - if err := ctx.Session.Set("uname", u.Name); err != nil { + if err := newSess.Set("uname", u.Name); err != nil { return false, err } - if err := ctx.Session.Release(); err != nil { + if err := newSess.Release(); err != nil { return false, err } @@ -233,26 +242,35 @@ func SignInPost(ctx *context.Context) { return } + // Save the current session + _ = ctx.Session.Release() + + // Regenerate the session - this will copy the data from the old session + newSess, _ := ctx.Session.RegenerateID(ctx.Resp, ctx.Req) + + // Delete the old session + _ = ctx.Session.Flush() + // User will need to use 2FA TOTP or U2F, save data - if err := ctx.Session.Set("twofaUid", u.ID); err != nil { + if err := newSess.Set("twofaUid", u.ID); err != nil { ctx.ServerError("UserSignIn: Unable to set twofaUid in session", err) return } - if err := ctx.Session.Set("twofaRemember", form.Remember); err != nil { + if err := newSess.Set("twofaRemember", form.Remember); err != nil { ctx.ServerError("UserSignIn: Unable to set twofaRemember in session", err) return } if hasTOTPtwofa { // User will need to use U2F, save data - if err := ctx.Session.Set("totpEnrolled", u.ID); err != nil { + if err := newSess.Set("totpEnrolled", u.ID); err != nil { ctx.ServerError("UserSignIn: Unable to set u2fEnrolled in session", err) return } } - if err := ctx.Session.Release(); err != nil { + if err := newSess.Release(); err != nil { ctx.ServerError("UserSignIn: Unable to save session", err) return } @@ -526,6 +544,7 @@ func handleSignInFull(ctx *context.Context, u *user_model.User, remember bool, o setting.CookieRememberName, u.Name, days) } + // Delete the openid, 2fa and linkaccount data _ = ctx.Session.Delete("openid_verified_uri") _ = ctx.Session.Delete("openid_signin_remember") _ = ctx.Session.Delete("openid_determined_email") @@ -534,13 +553,25 @@ func handleSignInFull(ctx *context.Context, u *user_model.User, remember bool, o _ = ctx.Session.Delete("twofaRemember") _ = ctx.Session.Delete("u2fChallenge") _ = ctx.Session.Delete("linkAccount") - if err := ctx.Session.Set("uid", u.ID); err != nil { + + // Save the session + _ = ctx.Session.Release() + + // Regenerate the session copying the old data to the new one + newSess, _ := ctx.Session.RegenerateID(ctx.Resp, ctx.Req) + + // delete the old session + _ = ctx.Session.Flush() + + // Now set our login data + if err := newSess.Set("uid", u.ID); err != nil { log.Error("Error setting uid %d in session: %v", u.ID, err) } - if err := ctx.Session.Set("uname", u.Name); err != nil { + if err := newSess.Set("uname", u.Name); err != nil { log.Error("Error setting uname %s session: %v", u.Name, err) } - if err := ctx.Session.Release(); err != nil { + + if err := newSess.Release(); err != nil { log.Error("Unable to store session: %v", err) } @@ -799,13 +830,23 @@ func handleOAuth2SignIn(ctx *context.Context, source *login.Source, u *user_mode // If this user is enrolled in 2FA and this source doesn't override it, // we can't sign the user in just yet. Instead, redirect them to the 2FA authentication page. if !needs2FA { - if err := ctx.Session.Set("uid", u.ID); err != nil { + // save the current session + _ = ctx.Session.Release() + + // regenerate the session - copying data from the old session to the new. + newSess, _ := ctx.Session.RegenerateID(ctx.Resp, ctx.Req) + + // delete the old session + ctx.Session.Flush() + + // Set session IDs + if err := newSess.Set("uid", u.ID); err != nil { log.Error("Error setting uid in session: %v", err) } - if err := ctx.Session.Set("uname", u.Name); err != nil { + if err := newSess.Set("uname", u.Name); err != nil { log.Error("Error setting uname in session: %v", err) } - if err := ctx.Session.Release(); err != nil { + if err := newSess.Release(); err != nil { log.Error("Error storing session: %v", err) } @@ -1199,7 +1240,7 @@ func LinkAccountPostRegister(ctx *context.Context) { return } - ctx.Redirect(setting.AppSubURL + "/user/login") + handleSignIn(ctx, u, false) } // HandleSignOut resets the session and sets the cookies @@ -1563,13 +1604,23 @@ func handleAccountActivation(ctx *context.Context, user *user_model.User) { log.Trace("User activated: %s", user.Name) - if err := ctx.Session.Set("uid", user.ID); err != nil { + // save the old session + _ = ctx.Session.Release() + + // Regenerate the session copying the old data to the new session + newSess, _ := ctx.Session.RegenerateID(ctx.Resp, ctx.Req) + + // delete the old session + ctx.Session.Flush() + + // Set session IDs + if err := newSess.Set("uid", user.ID); err != nil { log.Error("Error setting uid in session[%s]: %v", ctx.Session.ID(), err) } - if err := ctx.Session.Set("uname", user.Name); err != nil { + if err := newSess.Set("uname", user.Name); err != nil { log.Error("Error setting uname in session[%s]: %v", ctx.Session.ID(), err) } - if err := ctx.Session.Release(); err != nil { + if err := newSess.Release(); err != nil { log.Error("Error storing session[%s]: %v", ctx.Session.ID(), err) } From 6c63eb0f120e75adf211a0f1ca34c906c9cc15b8 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sat, 18 Dec 2021 19:14:23 +0000 Subject: [PATCH 2/6] with new session.RegenerateID function Signed-off-by: Andrew Thornton --- modules/session/store.go | 11 ++ routers/web/user/auth.go | 119 ++++++++++----------- services/auth/auth.go | 9 +- vendor/gitea.com/go-chi/session/session.go | 22 +++- 4 files changed, 95 insertions(+), 66 deletions(-) diff --git a/modules/session/store.go b/modules/session/store.go index 529187d3be874..08c3369233a36 100644 --- a/modules/session/store.go +++ b/modules/session/store.go @@ -4,9 +4,20 @@ package session +import ( + "net/http" + + "gitea.com/go-chi/session" +) + // Store represents a session store type Store interface { Get(interface{}) interface{} Set(interface{}, interface{}) error Delete(interface{}) error } + +func RegenerateSession(resp http.ResponseWriter, req *http.Request) (Store, error) { + s, err := session.RegenerateSession(resp, req) + return s, err +} diff --git a/routers/web/user/auth.go b/routers/web/user/auth.go index 7e1b9d7c2b404..5506087d9b989 100644 --- a/routers/web/user/auth.go +++ b/routers/web/user/auth.go @@ -22,6 +22,7 @@ import ( "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/password" "code.gitea.io/gitea/modules/recaptcha" + "code.gitea.io/gitea/modules/session" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/timeutil" "code.gitea.io/gitea/modules/web" @@ -90,23 +91,18 @@ func AutoSignIn(ctx *context.Context) (bool, error) { isSucceed = true - // Save the session first - _ = ctx.Session.Release() - - // Then regenerate the ID - which should copy the previous data - newSess, _ := ctx.Session.RegenerateID(ctx.Resp, ctx.Req) - - // Then flush the old session to delete it - _ = ctx.Session.Flush() + if _, err := session.RegenerateSession(ctx.Resp, ctx.Req); err != nil { + return false, fmt.Errorf("unable to RegenerateSession: Error: %w", err) + } // Set session IDs - if err := newSess.Set("uid", u.ID); err != nil { + if err := ctx.Session.Set("uid", u.ID); err != nil { return false, err } - if err := newSess.Set("uname", u.Name); err != nil { + if err := ctx.Session.Set("uname", u.Name); err != nil { return false, err } - if err := newSess.Release(); err != nil { + if err := ctx.Session.Release(); err != nil { return false, err } @@ -242,35 +238,31 @@ func SignInPost(ctx *context.Context) { return } - // Save the current session - _ = ctx.Session.Release() - - // Regenerate the session - this will copy the data from the old session - newSess, _ := ctx.Session.RegenerateID(ctx.Resp, ctx.Req) - - // Delete the old session - _ = ctx.Session.Flush() + if _, err := session.RegenerateSession(ctx.Resp, ctx.Req); err != nil { + ctx.ServerError("UserSignIn: Unable to set regenerate session", err) + return + } // User will need to use 2FA TOTP or U2F, save data - if err := newSess.Set("twofaUid", u.ID); err != nil { + if err := ctx.Session.Set("twofaUid", u.ID); err != nil { ctx.ServerError("UserSignIn: Unable to set twofaUid in session", err) return } - if err := newSess.Set("twofaRemember", form.Remember); err != nil { + if err := ctx.Session.Set("twofaRemember", form.Remember); err != nil { ctx.ServerError("UserSignIn: Unable to set twofaRemember in session", err) return } if hasTOTPtwofa { // User will need to use U2F, save data - if err := newSess.Set("totpEnrolled", u.ID); err != nil { + if err := ctx.Session.Set("totpEnrolled", u.ID); err != nil { ctx.ServerError("UserSignIn: Unable to set u2fEnrolled in session", err) return } } - if err := newSess.Release(); err != nil { + if err := ctx.Session.Release(); err != nil { ctx.ServerError("UserSignIn: Unable to save session", err) return } @@ -415,6 +407,9 @@ func TwoFactorScratchPost(ctx *context.Context) { handleSignInFull(ctx, u, remember, false) ctx.Flash.Info(ctx.Tr("auth.twofa_scratch_used")) + if ctx.Written() { + return + } ctx.Redirect(setting.AppSubURL + "/user/settings/security") return } @@ -521,6 +516,9 @@ func U2FSign(ctx *context.Context) { } } redirect := handleSignInFull(ctx, user, remember, false) + if ctx.Written() { + return + } if redirect == "" { redirect = setting.AppSubURL + "/" } @@ -533,7 +531,11 @@ func U2FSign(ctx *context.Context) { // This handles the final part of the sign-in process of the user. func handleSignIn(ctx *context.Context, u *user_model.User, remember bool) { - handleSignInFull(ctx, u, remember, true) + redirect := handleSignInFull(ctx, u, remember, true) + if ctx.Written() { + return + } + ctx.Redirect(redirect) } func handleSignInFull(ctx *context.Context, u *user_model.User, remember bool, obeyRedirect bool) string { @@ -544,6 +546,11 @@ func handleSignInFull(ctx *context.Context, u *user_model.User, remember bool, o setting.CookieRememberName, u.Name, days) } + if _, err := session.RegenerateSession(ctx.Resp, ctx.Req); err != nil { + ctx.ServerError("RegenerateSession", err) + return setting.AppSubURL + "/" + } + // Delete the openid, 2fa and linkaccount data _ = ctx.Session.Delete("openid_verified_uri") _ = ctx.Session.Delete("openid_signin_remember") @@ -553,25 +560,13 @@ func handleSignInFull(ctx *context.Context, u *user_model.User, remember bool, o _ = ctx.Session.Delete("twofaRemember") _ = ctx.Session.Delete("u2fChallenge") _ = ctx.Session.Delete("linkAccount") - - // Save the session - _ = ctx.Session.Release() - - // Regenerate the session copying the old data to the new one - newSess, _ := ctx.Session.RegenerateID(ctx.Resp, ctx.Req) - - // delete the old session - _ = ctx.Session.Flush() - - // Now set our login data - if err := newSess.Set("uid", u.ID); err != nil { + if err := ctx.Session.Set("uid", u.ID); err != nil { log.Error("Error setting uid %d in session: %v", u.ID, err) } - if err := newSess.Set("uname", u.Name); err != nil { + if err := ctx.Session.Set("uname", u.Name); err != nil { log.Error("Error setting uname %s session: %v", u.Name, err) } - - if err := newSess.Release(); err != nil { + if err := ctx.Session.Release(); err != nil { log.Error("Unable to store session: %v", err) } @@ -580,7 +575,7 @@ func handleSignInFull(ctx *context.Context, u *user_model.User, remember bool, o if len(u.Language) == 0 { u.Language = ctx.Locale.Language() if err := user_model.UpdateUserCols(db.DefaultContext, u, "language"); err != nil { - log.Error(fmt.Sprintf("Error updating user language [user: %d, locale: %s]", u.ID, u.Language)) + ctx.ServerError("UpdateUserCols Language", fmt.Errorf("Error updating user language [user: %d, locale: %s]", u.ID, u.Language)) return setting.AppSubURL + "/" } } @@ -830,23 +825,19 @@ func handleOAuth2SignIn(ctx *context.Context, source *login.Source, u *user_mode // If this user is enrolled in 2FA and this source doesn't override it, // we can't sign the user in just yet. Instead, redirect them to the 2FA authentication page. if !needs2FA { - // save the current session - _ = ctx.Session.Release() - - // regenerate the session - copying data from the old session to the new. - newSess, _ := ctx.Session.RegenerateID(ctx.Resp, ctx.Req) - - // delete the old session - ctx.Session.Flush() + if _, err := session.RegenerateSession(ctx.Resp, ctx.Req); err != nil { + ctx.ServerError("RegenerateSession", err) + return + } // Set session IDs - if err := newSess.Set("uid", u.ID); err != nil { + if err := ctx.Session.Set("uid", u.ID); err != nil { log.Error("Error setting uid in session: %v", err) } - if err := newSess.Set("uname", u.Name); err != nil { + if err := ctx.Session.Set("uname", u.Name); err != nil { log.Error("Error setting uname in session: %v", err) } - if err := newSess.Release(); err != nil { + if err := ctx.Session.Release(); err != nil { log.Error("Error storing session: %v", err) } @@ -1383,7 +1374,7 @@ func SignUpPost(ctx *context.Context) { } ctx.Flash.Success(ctx.Tr("auth.sign_up_successful")) - handleSignInFull(ctx, u, false, true) + handleSignIn(ctx, u, false) } // createAndHandleCreatedUser calls createUserInContext and @@ -1604,23 +1595,20 @@ func handleAccountActivation(ctx *context.Context, user *user_model.User) { log.Trace("User activated: %s", user.Name) - // save the old session - _ = ctx.Session.Release() - - // Regenerate the session copying the old data to the new session - newSess, _ := ctx.Session.RegenerateID(ctx.Resp, ctx.Req) - - // delete the old session - ctx.Session.Flush() + if _, err := session.RegenerateSession(ctx.Resp, ctx.Req); err != nil { + log.Error("Unable to regenerate session for user: %-v with email: %s: %v", user, user.Email, err) + ctx.ServerError("ActivateUserEmail", err) + return + } // Set session IDs - if err := newSess.Set("uid", user.ID); err != nil { + if err := ctx.Session.Set("uid", user.ID); err != nil { log.Error("Error setting uid in session[%s]: %v", ctx.Session.ID(), err) } - if err := newSess.Set("uname", user.Name); err != nil { + if err := ctx.Session.Set("uname", user.Name); err != nil { log.Error("Error setting uname in session[%s]: %v", ctx.Session.ID(), err) } - if err := newSess.Release(); err != nil { + if err := ctx.Session.Release(); err != nil { log.Error("Error storing session[%s]: %v", ctx.Session.ID(), err) } @@ -1880,11 +1868,14 @@ func ResetPasswdPost(ctx *context.Context) { handleSignInFull(ctx, u, remember, false) ctx.Flash.Info(ctx.Tr("auth.twofa_scratch_used")) + if ctx.Written() { + return + } ctx.Redirect(setting.AppSubURL + "/user/settings/security") return } - handleSignInFull(ctx, u, remember, true) + handleSignIn(ctx, u, remember) } // MustChangePassword renders the page to change a user's password diff --git a/services/auth/auth.go b/services/auth/auth.go index e53691221f5f8..f5a576eabdcc6 100644 --- a/services/auth/auth.go +++ b/services/auth/auth.go @@ -15,6 +15,7 @@ import ( "code.gitea.io/gitea/models/db" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/log" + "code.gitea.io/gitea/modules/session" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/web/middleware" ) @@ -106,6 +107,12 @@ func isGitRawReleaseOrLFSPath(req *http.Request) bool { // handleSignIn clears existing session variables and stores new ones for the specified user object func handleSignIn(resp http.ResponseWriter, req *http.Request, sess SessionStore, user *user_model.User) { + // We need to regenerate the session... + sess, err := session.RegenerateSession(resp, req) + if err != nil { + log.Error(fmt.Sprintf("Error regenerating session: %v", err)) + } + _ = sess.Delete("openid_verified_uri") _ = sess.Delete("openid_signin_remember") _ = sess.Delete("openid_determined_email") @@ -114,7 +121,7 @@ func handleSignIn(resp http.ResponseWriter, req *http.Request, sess SessionStore _ = sess.Delete("twofaRemember") _ = sess.Delete("u2fChallenge") _ = sess.Delete("linkAccount") - err := sess.Set("uid", user.ID) + err = sess.Set("uid", user.ID) if err != nil { log.Error(fmt.Sprintf("Error setting session: %v", err)) } diff --git a/vendor/gitea.com/go-chi/session/session.go b/vendor/gitea.com/go-chi/session/session.go index 5c2afedc934b5..be10674133575 100644 --- a/vendor/gitea.com/go-chi/session/session.go +++ b/vendor/gitea.com/go-chi/session/session.go @@ -260,7 +260,7 @@ func Sessioner(options ...Options) func(next http.Handler) http.Handler { return } - if err = sess.Release(); err != nil { + if err = s.RawStore.Release(); err != nil { panic("session(release): " + err.Error()) } }) @@ -274,6 +274,26 @@ func GetSession(req *http.Request) Store { return sess } +// RegenerateSession +func RegenerateSession(resp http.ResponseWriter, req *http.Request) (Store, error) { + sess, ok := GetSession(req).(*store) + if !ok { + return nil, fmt.Errorf("no session in request context") + } + + oldRawStore := sess.RawStore + if err := oldRawStore.Release(); err != nil { + return nil, err + } + + store, err := sess.RegenerateID(resp, req) + if err != nil { + return nil, err + } + sess.RawStore = store + return sess, nil +} + // Provider is the interface that provides session manipulations. type Provider interface { // Init initializes session provider. From d15a3e5d85a93bda94ae5cbe94cfd4a6f9ffba13 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sat, 18 Dec 2021 22:21:28 +0000 Subject: [PATCH 3/6] update go-chi/session Signed-off-by: Andrew Thornton --- go.mod | 2 +- go.sum | 4 ++-- vendor/modules.txt | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/go.mod b/go.mod index a597958b5b46d..6e34c6f512cdd 100644 --- a/go.mod +++ b/go.mod @@ -9,7 +9,7 @@ require ( gitea.com/go-chi/binding v0.0.0-20211013065440-d16dc407c2be gitea.com/go-chi/cache v0.0.0-20211013020926-78790b11abf1 gitea.com/go-chi/captcha v0.0.0-20211013065431-70641c1a35d5 - gitea.com/go-chi/session v0.0.0-20211013065435-7d334f340c09 + gitea.com/go-chi/session v0.0.0-20211218221615-e3605d8b28b8 gitea.com/lunny/levelqueue v0.4.1 github.com/Microsoft/go-winio v0.5.0 // indirect github.com/NYTimes/gziphandler v1.1.1 diff --git a/go.sum b/go.sum index 6891abd6b380e..acd9b7447fa16 100644 --- a/go.sum +++ b/go.sum @@ -48,8 +48,8 @@ gitea.com/go-chi/cache v0.0.0-20211013020926-78790b11abf1 h1:Z7DcvTkxt8ovcENgPsQ gitea.com/go-chi/cache v0.0.0-20211013020926-78790b11abf1/go.mod h1:k2V/gPDEtXGjjMGuBJiapffAXTv76H4snSmlJRLUhH0= gitea.com/go-chi/captcha v0.0.0-20211013065431-70641c1a35d5 h1:J/1i8u40TbcLP/w2w2KCkgW2PelIqYkD5UOwu8IOvVU= gitea.com/go-chi/captcha v0.0.0-20211013065431-70641c1a35d5/go.mod h1:hQ9SYHKdOX968wJglb/NMQ+UqpOKwW4L+EYdvkWjHSo= -gitea.com/go-chi/session v0.0.0-20211013065435-7d334f340c09 h1:1+K+6uOXrnSfLHXvu8jpuoNEWA3/NULOlLSRZwaij+c= -gitea.com/go-chi/session v0.0.0-20211013065435-7d334f340c09/go.mod h1:fc/pjt5EqNKgqQXYzcas1Z5L5whkZHyOvTA7OzWVJck= +gitea.com/go-chi/session v0.0.0-20211218221615-e3605d8b28b8 h1:tJQRXgZigkLeeW9LPlps9G9aMoE6LAmqigLA+wxmd1Q= +gitea.com/go-chi/session v0.0.0-20211218221615-e3605d8b28b8/go.mod h1:fc/pjt5EqNKgqQXYzcas1Z5L5whkZHyOvTA7OzWVJck= gitea.com/lunny/levelqueue v0.4.1 h1:RZ+AFx5gBsZuyqCvofhAkPQ9uaVDPJnsULoJZIYaJNw= gitea.com/lunny/levelqueue v0.4.1/go.mod h1:HBqmLbz56JWpfEGG0prskAV97ATNRoj5LDmPicD22hU= gitea.com/xorm/sqlfiddle v0.0.0-20180821085327-62ce714f951a h1:lSA0F4e9A2NcQSqGqTOXqu2aRi/XEQxDCBwM8yJtE6s= diff --git a/vendor/modules.txt b/vendor/modules.txt index 6f3acc56274ef..02461cbf82e4c 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -18,7 +18,7 @@ gitea.com/go-chi/cache/memcache # gitea.com/go-chi/captcha v0.0.0-20211013065431-70641c1a35d5 ## explicit gitea.com/go-chi/captcha -# gitea.com/go-chi/session v0.0.0-20211013065435-7d334f340c09 +# gitea.com/go-chi/session v0.0.0-20211218221615-e3605d8b28b8 ## explicit gitea.com/go-chi/session gitea.com/go-chi/session/couchbase From aa93f00eeee52e4fb461a8965191016a0828ee2b Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sat, 18 Dec 2021 22:48:19 +0000 Subject: [PATCH 4/6] Ensure that session id is changed after oauth data is set and between account linking pages too Signed-off-by: Andrew Thornton --- routers/web/user/auth.go | 15 +++++++++++++++ routers/web/user/auth_openid.go | 6 ++++++ services/auth/source/oauth2/store.go | 6 ++++++ 3 files changed, 27 insertions(+) diff --git a/routers/web/user/auth.go b/routers/web/user/auth.go index 5506087d9b989..7479b5a0c0200 100644 --- a/routers/web/user/auth.go +++ b/routers/web/user/auth.go @@ -782,6 +782,11 @@ func getUserName(gothUser *goth.User) string { } func showLinkingLogin(ctx *context.Context, gothUser goth.User) { + if _, err := session.RegenerateSession(ctx.Resp, ctx.Req); err != nil { + ctx.ServerError("RegenerateSession", err) + return + } + if err := ctx.Session.Set("linkAccountGothUser", gothUser); err != nil { log.Error("Error setting linkAccountGothUser in session: %v", err) } @@ -882,6 +887,11 @@ func handleOAuth2SignIn(ctx *context.Context, source *login.Source, u *user_mode } } + if _, err := session.RegenerateSession(ctx.Resp, ctx.Req); err != nil { + ctx.ServerError("RegenerateSession", err) + return + } + // User needs to use 2FA, save data and redirect to 2FA page. if err := ctx.Session.Set("twofaUid", u.ID); err != nil { log.Error("Error setting twofaUid in session: %v", err) @@ -1094,6 +1104,11 @@ func linkAccount(ctx *context.Context, u *user_model.User, gothUser goth.User, r return } + if _, err := session.RegenerateSession(ctx.Resp, ctx.Req); err != nil { + ctx.ServerError("RegenerateSession", err) + return + } + // User needs to use 2FA, save data and redirect to 2FA page. if err := ctx.Session.Set("twofaUid", u.ID); err != nil { log.Error("Error setting twofaUid in session: %v", err) diff --git a/routers/web/user/auth_openid.go b/routers/web/user/auth_openid.go index 884ec29c212c4..55dd8f8d09a87 100644 --- a/routers/web/user/auth_openid.go +++ b/routers/web/user/auth_openid.go @@ -16,6 +16,7 @@ import ( "code.gitea.io/gitea/modules/hcaptcha" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/recaptcha" + "code.gitea.io/gitea/modules/session" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/util" "code.gitea.io/gitea/modules/web" @@ -232,6 +233,11 @@ func signInOpenIDVerify(ctx *context.Context) { } } + if _, err := session.RegenerateSession(ctx.Resp, ctx.Req); err != nil { + ctx.ServerError("RegenerateSession", err) + return + } + if err := ctx.Session.Set("openid_verified_uri", id); err != nil { log.Error("signInOpenIDVerify: Could not set openid_verified_uri in session: %v", err) } diff --git a/services/auth/source/oauth2/store.go b/services/auth/source/oauth2/store.go index 4026abb6ecdef..f00264f997d27 100644 --- a/services/auth/source/oauth2/store.go +++ b/services/auth/source/oauth2/store.go @@ -55,6 +55,7 @@ func (st *SessionsStore) getOrNew(r *http.Request, name string, override bool) ( } } + session.IsNew = override session.ID = chiStore.ID() // Simply copy the session id from the chi store return session, chiStore.Set(name, session) @@ -64,6 +65,11 @@ func (st *SessionsStore) getOrNew(r *http.Request, name string, override bool) ( func (st *SessionsStore) Save(r *http.Request, w http.ResponseWriter, session *sessions.Session) error { chiStore := chiSession.GetSession(r) + if session.IsNew { + _, _ = chiSession.RegenerateSession(w, r) + session.IsNew = false + } + if err := chiStore.Set(session.Name(), session); err != nil { return err } From 2cbf150d80aff14927db2595f7914875646f8112 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sun, 19 Dec 2021 09:08:36 +0000 Subject: [PATCH 5/6] placate lint Signed-off-by: Andrew Thornton --- modules/session/store.go | 1 + services/auth/auth.go | 2 +- services/auth/reverseproxy.go | 2 +- services/auth/sspi_windows.go | 2 +- 4 files changed, 4 insertions(+), 3 deletions(-) diff --git a/modules/session/store.go b/modules/session/store.go index 08c3369233a36..8c5d7d82eb463 100644 --- a/modules/session/store.go +++ b/modules/session/store.go @@ -17,6 +17,7 @@ type Store interface { Delete(interface{}) error } +// RegenerateSession regenerates the underlying session and returns the new store func RegenerateSession(resp http.ResponseWriter, req *http.Request) (Store, error) { s, err := session.RegenerateSession(resp, req) return s, err diff --git a/services/auth/auth.go b/services/auth/auth.go index f5a576eabdcc6..ea87f88f4fbb6 100644 --- a/services/auth/auth.go +++ b/services/auth/auth.go @@ -106,7 +106,7 @@ func isGitRawReleaseOrLFSPath(req *http.Request) bool { } // handleSignIn clears existing session variables and stores new ones for the specified user object -func handleSignIn(resp http.ResponseWriter, req *http.Request, sess SessionStore, user *user_model.User) { +func handleSignIn(resp http.ResponseWriter, req *http.Request, user *user_model.User) { // We need to regenerate the session... sess, err := session.RegenerateSession(resp, req) if err != nil { diff --git a/services/auth/reverseproxy.go b/services/auth/reverseproxy.go index 3e44d8b8639ca..cc03af16c1181 100644 --- a/services/auth/reverseproxy.go +++ b/services/auth/reverseproxy.go @@ -75,7 +75,7 @@ func (r *ReverseProxy) Verify(req *http.Request, w http.ResponseWriter, store Da // Make sure requests to API paths, attachment downloads, git and LFS do not create a new session if !middleware.IsAPIPath(req) && !isAttachmentDownload(req) && !isGitRawReleaseOrLFSPath(req) { if sess != nil && (sess.Get("uid") == nil || sess.Get("uid").(int64) != user.ID) { - handleSignIn(w, req, sess, user) + handleSignIn(w, req, user) } } store.GetData()["IsReverseProxy"] = true diff --git a/services/auth/sspi_windows.go b/services/auth/sspi_windows.go index 19f2349122437..5726db81a8984 100644 --- a/services/auth/sspi_windows.go +++ b/services/auth/sspi_windows.go @@ -145,7 +145,7 @@ func (s *SSPI) Verify(req *http.Request, w http.ResponseWriter, store DataStore, // Make sure requests to API paths and PWA resources do not create a new session if !middleware.IsAPIPath(req) && !isAttachmentDownload(req) { - handleSignIn(w, req, sess, user) + handleSignIn(w, req, user) } log.Trace("SSPI Authorization: Logged in user %-v", user) From 1b95303e0f6f4339a410ff19432a3a934e6cc54a Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sun, 19 Dec 2021 16:56:44 +0000 Subject: [PATCH 6/6] as per review Signed-off-by: Andrew Thornton --- routers/web/user/auth.go | 2 +- services/auth/auth.go | 6 ++++-- services/auth/reverseproxy.go | 2 +- services/auth/sspi_windows.go | 2 +- 4 files changed, 7 insertions(+), 5 deletions(-) diff --git a/routers/web/user/auth.go b/routers/web/user/auth.go index 7479b5a0c0200..0c3bd629cb619 100644 --- a/routers/web/user/auth.go +++ b/routers/web/user/auth.go @@ -406,10 +406,10 @@ func TwoFactorScratchPost(ctx *context.Context) { } handleSignInFull(ctx, u, remember, false) - ctx.Flash.Info(ctx.Tr("auth.twofa_scratch_used")) if ctx.Written() { return } + ctx.Flash.Info(ctx.Tr("auth.twofa_scratch_used")) ctx.Redirect(setting.AppSubURL + "/user/settings/security") return } diff --git a/services/auth/auth.go b/services/auth/auth.go index ea87f88f4fbb6..ceb9f4c5d87db 100644 --- a/services/auth/auth.go +++ b/services/auth/auth.go @@ -106,11 +106,13 @@ func isGitRawReleaseOrLFSPath(req *http.Request) bool { } // handleSignIn clears existing session variables and stores new ones for the specified user object -func handleSignIn(resp http.ResponseWriter, req *http.Request, user *user_model.User) { +func handleSignIn(resp http.ResponseWriter, req *http.Request, sess SessionStore, user *user_model.User) { // We need to regenerate the session... - sess, err := session.RegenerateSession(resp, req) + newSess, err := session.RegenerateSession(resp, req) if err != nil { log.Error(fmt.Sprintf("Error regenerating session: %v", err)) + } else { + sess = newSess } _ = sess.Delete("openid_verified_uri") diff --git a/services/auth/reverseproxy.go b/services/auth/reverseproxy.go index cc03af16c1181..3e44d8b8639ca 100644 --- a/services/auth/reverseproxy.go +++ b/services/auth/reverseproxy.go @@ -75,7 +75,7 @@ func (r *ReverseProxy) Verify(req *http.Request, w http.ResponseWriter, store Da // Make sure requests to API paths, attachment downloads, git and LFS do not create a new session if !middleware.IsAPIPath(req) && !isAttachmentDownload(req) && !isGitRawReleaseOrLFSPath(req) { if sess != nil && (sess.Get("uid") == nil || sess.Get("uid").(int64) != user.ID) { - handleSignIn(w, req, user) + handleSignIn(w, req, sess, user) } } store.GetData()["IsReverseProxy"] = true diff --git a/services/auth/sspi_windows.go b/services/auth/sspi_windows.go index 5726db81a8984..19f2349122437 100644 --- a/services/auth/sspi_windows.go +++ b/services/auth/sspi_windows.go @@ -145,7 +145,7 @@ func (s *SSPI) Verify(req *http.Request, w http.ResponseWriter, store DataStore, // Make sure requests to API paths and PWA resources do not create a new session if !middleware.IsAPIPath(req) && !isAttachmentDownload(req) { - handleSignIn(w, req, user) + handleSignIn(w, req, sess, user) } log.Trace("SSPI Authorization: Logged in user %-v", user)