From 76053ac31afccefc0335977a131089e696b46eab Mon Sep 17 00:00:00 2001 From: zeripath Date: Sun, 17 May 2020 13:43:29 +0100 Subject: [PATCH] Whenever the ctx.Session is updated, release it to save it before sending the redirect (#11456) Signed-off-by: Andrew Thornton Co-authored-by: Lauris BH --- routers/install.go | 5 ++ routers/user/auth.go | 113 ++++++++++++++----------- routers/user/auth_openid.go | 28 +++--- routers/user/oauth.go | 10 +++ routers/user/setting/security_twofa.go | 96 +++++++++++++-------- routers/user/setting/security_u2f.go | 16 ++-- 6 files changed, 168 insertions(+), 100 deletions(-) diff --git a/routers/install.go b/routers/install.go index e18adfea1..d4f270c9c 100644 --- a/routers/install.go +++ b/routers/install.go @@ -387,6 +387,11 @@ func InstallPost(ctx *context.Context, form auth.InstallForm) { ctx.RenderWithErr(ctx.Tr("install.save_config_failed", err), tplInstall, &form) return } + + if err = ctx.Session.Release(); err != nil { + ctx.RenderWithErr(ctx.Tr("install.save_config_failed", err), tplInstall, &form) + return + } } log.Info("First-time run install finished!") diff --git a/routers/user/auth.go b/routers/user/auth.go index f00f349a0..e1a885480 100644 --- a/routers/user/auth.go +++ b/routers/user/auth.go @@ -82,14 +82,18 @@ func AutoSignIn(ctx *context.Context) (bool, error) { } isSucceed = true - err = ctx.Session.Set("uid", u.ID) - if err != nil { + + // Set session IDs + if err := ctx.Session.Set("uid", u.ID); err != nil { return false, err } - err = ctx.Session.Set("uname", u.Name) - if err != nil { + if err := ctx.Session.Set("uname", u.Name); err != nil { return false, err } + if err := ctx.Session.Release(); err != nil { + return false, err + } + ctx.SetCookie(setting.CSRFCookieName, "", -1, setting.AppSubURL, setting.SessionConfig.Domain, setting.SessionConfig.Secure, true) return true, nil } @@ -204,14 +208,16 @@ func SignInPost(ctx *context.Context, form auth.SignInForm) { } // User needs to use 2FA, save data and redirect to 2FA page. - err = ctx.Session.Set("twofaUid", u.ID) - if err != nil { - ctx.ServerError("UserSignIn", err) + if err := ctx.Session.Set("twofaUid", u.ID); err != nil { + ctx.ServerError("UserSignIn: Unable to set twofaUid in session", err) return } - err = ctx.Session.Set("twofaRemember", form.Remember) - if err != nil { - ctx.ServerError("UserSignIn", err) + if err := ctx.Session.Set("twofaRemember", form.Remember); err != nil { + ctx.ServerError("UserSignIn: Unable to set twofaRemember in session", err) + return + } + if err := ctx.Session.Release(); err != nil { + ctx.ServerError("UserSignIn: Unable to save session", err) return } @@ -408,10 +414,14 @@ func U2FChallenge(ctx *context.Context) { ctx.ServerError("u2f.NewChallenge", err) return } - if err = ctx.Session.Set("u2fChallenge", challenge); err != nil { - ctx.ServerError("UserSignIn", err) + if err := ctx.Session.Set("u2fChallenge", challenge); err != nil { + ctx.ServerError("UserSignIn: unable to set u2fChallenge in session", err) return } + if err := ctx.Session.Release(); err != nil { + ctx.ServerError("UserSignIn: unable to store session", err) + } + ctx.JSON(200, challenge.SignRequest(regs.ToRegistrations())) } @@ -495,13 +505,14 @@ func handleSignInFull(ctx *context.Context, u *models.User, remember bool, obeyR _ = ctx.Session.Delete("twofaRemember") _ = ctx.Session.Delete("u2fChallenge") _ = ctx.Session.Delete("linkAccount") - err := ctx.Session.Set("uid", u.ID) - if err != nil { - log.Error(fmt.Sprintf("Error setting session: %v", err)) + if err := ctx.Session.Set("uid", u.ID); err != nil { + log.Error("Error setting uid %d in session: %v", u.ID, err) } - err = ctx.Session.Set("uname", u.Name) - if err != nil { - log.Error(fmt.Sprintf("Error setting session: %v", err)) + if err := ctx.Session.Set("uname", u.Name); err != nil { + log.Error("Error setting uname %s session: %v", u.Name, err) + } + if err := ctx.Session.Release(); err != nil { + log.Error("Unable to store session: %v", err) } // Language setting of the user overwrites the one previously set @@ -594,9 +605,11 @@ func handleOAuth2SignIn(u *models.User, gothUser goth.User, ctx *context.Context if u == nil { // no existing user is found, request attach or new account - err = ctx.Session.Set("linkAccountGothUser", gothUser) - if err != nil { - log.Error(fmt.Sprintf("Error setting session: %v", err)) + if err := ctx.Session.Set("linkAccountGothUser", gothUser); err != nil { + log.Error("Error setting linkAccountGothUser in session: %v", err) + } + if err := ctx.Session.Release(); err != nil { + log.Error("Error storing session: %v", err) } ctx.Redirect(setting.AppSubURL + "/user/link_account") return @@ -611,13 +624,14 @@ func handleOAuth2SignIn(u *models.User, gothUser goth.User, ctx *context.Context return } - err = ctx.Session.Set("uid", u.ID) - if err != nil { - log.Error(fmt.Sprintf("Error setting session: %v", err)) + if err := ctx.Session.Set("uid", u.ID); err != nil { + log.Error("Error setting uid in session: %v", err) } - err = ctx.Session.Set("uname", u.Name) - if err != nil { - log.Error(fmt.Sprintf("Error setting session: %v", err)) + if err := ctx.Session.Set("uname", u.Name); err != nil { + log.Error("Error setting uname in session: %v", err) + } + if err := ctx.Session.Release(); err != nil { + log.Error("Error storing session: %v", err) } // Clear whatever CSRF has right now, force to generate a new one @@ -646,13 +660,14 @@ func handleOAuth2SignIn(u *models.User, gothUser goth.User, ctx *context.Context } // User needs to use 2FA, save data and redirect to 2FA page. - err = ctx.Session.Set("twofaUid", u.ID) - if err != nil { - log.Error(fmt.Sprintf("Error setting session: %v", err)) + if err := ctx.Session.Set("twofaUid", u.ID); err != nil { + log.Error("Error setting twofaUid in session: %v", err) } - err = ctx.Session.Set("twofaRemember", false) - if err != nil { - log.Error(fmt.Sprintf("Error setting session: %v", err)) + if err := ctx.Session.Set("twofaRemember", false); err != nil { + log.Error("Error setting twofaRemember in session: %v", err) + } + if err := ctx.Session.Release(); err != nil { + log.Error("Error storing session: %v", err) } // If U2F is enrolled -> Redirect to U2F instead @@ -821,17 +836,17 @@ func LinkAccountPostSignIn(ctx *context.Context, signInForm auth.SignInForm) { } // User needs to use 2FA, save data and redirect to 2FA page. - err = ctx.Session.Set("twofaUid", u.ID) - if err != nil { - log.Error(fmt.Sprintf("Error setting session: %v", err)) + if err := ctx.Session.Set("twofaUid", u.ID); err != nil { + log.Error("Error setting twofaUid in session: %v", err) } - err = ctx.Session.Set("twofaRemember", signInForm.Remember) - if err != nil { - log.Error(fmt.Sprintf("Error setting session: %v", err)) + if err := ctx.Session.Set("twofaRemember", signInForm.Remember); err != nil { + log.Error("Error setting twofaRemember in session: %v", err) } - err = ctx.Session.Set("linkAccount", true) - if err != nil { - log.Error(fmt.Sprintf("Error setting session: %v", err)) + if err := ctx.Session.Set("linkAccount", true); err != nil { + log.Error("Error setting linkAccount in session: %v", err) + } + if err := ctx.Session.Release(); err != nil { + log.Error("Error storing session: %v", err) } // If U2F is enrolled -> Redirect to U2F instead @@ -1200,14 +1215,16 @@ func Activate(ctx *context.Context) { log.Trace("User activated: %s", user.Name) - err = ctx.Session.Set("uid", user.ID) - if err != nil { - log.Error(fmt.Sprintf("Error setting session: %v", err)) + if err := ctx.Session.Set("uid", user.ID); err != nil { + log.Error(fmt.Sprintf("Error setting uid in session: %v", err)) } - err = ctx.Session.Set("uname", user.Name) - if err != nil { - log.Error(fmt.Sprintf("Error setting session: %v", err)) + if err := ctx.Session.Set("uname", user.Name); err != nil { + log.Error(fmt.Sprintf("Error setting uname in session: %v", err)) } + if err := ctx.Session.Release(); err != nil { + log.Error("Error storing session: %v", err) + } + ctx.Flash.Success(ctx.Tr("auth.account_activated")) ctx.Redirect(setting.AppSubURL + "/") return diff --git a/routers/user/auth_openid.go b/routers/user/auth_openid.go index bd05538ad..ba2c8be8c 100644 --- a/routers/user/auth_openid.go +++ b/routers/user/auth_openid.go @@ -128,9 +128,12 @@ func SignInOpenIDPost(ctx *context.Context, form auth.SignInOpenIDForm) { url += "&openid.sreg.optional=nickname%2Cemail" log.Trace("Form-passed openid-remember: %t", form.Remember) - err = ctx.Session.Set("openid_signin_remember", form.Remember) - if err != nil { - log.Error("SignInOpenIDPost: Could not set session: %v", err.Error()) + + if err := ctx.Session.Set("openid_signin_remember", form.Remember); err != nil { + log.Error("SignInOpenIDPost: Could not set openid_signin_remember in session: %v", err) + } + if err := ctx.Session.Release(); err != nil { + log.Error("SignInOpenIDPost: Unable to save changes to the session: %v", err) } ctx.Redirect(url) @@ -227,23 +230,22 @@ func signInOpenIDVerify(ctx *context.Context) { } } - err = ctx.Session.Set("openid_verified_uri", id) - if err != nil { - log.Error("signInOpenIDVerify: Could not set session: %v", err.Error()) + if err := ctx.Session.Set("openid_verified_uri", id); err != nil { + log.Error("signInOpenIDVerify: Could not set openid_verified_uri in session: %v", err) } - - err = ctx.Session.Set("openid_determined_email", email) - if err != nil { - log.Error("signInOpenIDVerify: Could not set session: %v", err.Error()) + if err := ctx.Session.Set("openid_determined_email", email); err != nil { + log.Error("signInOpenIDVerify: Could not set openid_determined_email in session: %v", err) } if u != nil { nickname = u.LowerName } - err = ctx.Session.Set("openid_determined_username", nickname) - if err != nil { - log.Error("signInOpenIDVerify: Could not set session: %v", err.Error()) + if err := ctx.Session.Set("openid_determined_username", nickname); err != nil { + log.Error("signInOpenIDVerify: Could not set openid_determined_username in session: %v", err) + } + if err := ctx.Session.Release(); err != nil { + log.Error("signInOpenIDVerify: Unable to save changes to the session: %v", err) } if u != nil || !setting.Service.EnableOpenIDSignUp { diff --git a/routers/user/oauth.go b/routers/user/oauth.go index d4dcb857f..a9e089b39 100644 --- a/routers/user/oauth.go +++ b/routers/user/oauth.go @@ -229,6 +229,11 @@ func AuthorizeOAuth(ctx *context.Context, form auth.AuthorizationForm) { }, form.RedirectURI) return } + // Here we're just going to try to release the session early + if err := ctx.Session.Release(); err != nil { + // we'll tolerate errors here as they *should* get saved elsewhere + log.Error("Unable to save changes to the session: %v", err) + } case "": break default: @@ -287,6 +292,11 @@ func AuthorizeOAuth(ctx *context.Context, form auth.AuthorizationForm) { log.Error(err.Error()) return } + // Here we're just going to try to release the session early + if err := ctx.Session.Release(); err != nil { + // we'll tolerate errors here as they *should* get saved elsewhere + log.Error("Unable to save changes to the session: %v", err) + } ctx.HTML(200, tplGrantAccess) } diff --git a/routers/user/setting/security_twofa.go b/routers/user/setting/security_twofa.go index 5aa951a9b..ed87f0043 100644 --- a/routers/user/setting/security_twofa.go +++ b/routers/user/setting/security_twofa.go @@ -15,6 +15,7 @@ import ( "code.gitea.io/gitea/models" "code.gitea.io/gitea/modules/auth" "code.gitea.io/gitea/modules/context" + "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" "github.com/pquerna/otp" @@ -28,18 +29,22 @@ func RegenerateScratchTwoFactor(ctx *context.Context) { t, err := models.GetTwoFactorByUID(ctx.User.ID) if err != nil { - ctx.ServerError("SettingsTwoFactor", err) + if models.IsErrTwoFactorNotEnrolled(err) { + ctx.Flash.Error(ctx.Tr("setting.twofa_not_enrolled")) + ctx.Redirect(setting.AppSubURL + "/user/settings/security") + } + ctx.ServerError("SettingsTwoFactor: Failed to GetTwoFactorByUID", err) return } token, err := t.GenerateScratchToken() if err != nil { - ctx.ServerError("SettingsTwoFactor", err) + ctx.ServerError("SettingsTwoFactor: Failed to GenerateScratchToken", err) return } if err = models.UpdateTwoFactor(t); err != nil { - ctx.ServerError("SettingsTwoFactor", err) + ctx.ServerError("SettingsTwoFactor: Failed to UpdateTwoFactor", err) return } @@ -54,12 +59,21 @@ func DisableTwoFactor(ctx *context.Context) { t, err := models.GetTwoFactorByUID(ctx.User.ID) if err != nil { - ctx.ServerError("SettingsTwoFactor", err) + if models.IsErrTwoFactorNotEnrolled(err) { + ctx.Flash.Error(ctx.Tr("setting.twofa_not_enrolled")) + ctx.Redirect(setting.AppSubURL + "/user/settings/security") + } + ctx.ServerError("SettingsTwoFactor: Failed to GetTwoFactorByUID", err) return } if err = models.DeleteTwoFactorByID(t.ID, ctx.User.ID); err != nil { - ctx.ServerError("SettingsTwoFactor", err) + if models.IsErrTwoFactorNotEnrolled(err) { + // There is a potential DB race here - we must have been disabled by another request in the intervening period + ctx.Flash.Success(ctx.Tr("settings.twofa_disabled")) + ctx.Redirect(setting.AppSubURL + "/user/settings/security") + } + ctx.ServerError("SettingsTwoFactor: Failed to DeleteTwoFactorByID", err) return } @@ -74,7 +88,7 @@ func twofaGenerateSecretAndQr(ctx *context.Context) bool { if uri != nil { otpKey, err = otp.NewKeyFromURL(uri.(string)) if err != nil { - ctx.ServerError("SettingsTwoFactor: NewKeyFromURL: ", err) + ctx.ServerError("SettingsTwoFactor: Failed NewKeyFromURL: ", err) return false } } @@ -87,7 +101,7 @@ func twofaGenerateSecretAndQr(ctx *context.Context) bool { AccountName: ctx.User.Name, }) if err != nil { - ctx.ServerError("SettingsTwoFactor", err) + ctx.ServerError("SettingsTwoFactor: totpGenerate Failed", err) return false } } @@ -95,27 +109,33 @@ func twofaGenerateSecretAndQr(ctx *context.Context) bool { ctx.Data["TwofaSecret"] = otpKey.Secret() img, err := otpKey.Image(320, 240) if err != nil { - ctx.ServerError("SettingsTwoFactor", err) + ctx.ServerError("SettingsTwoFactor: otpKey image generation failed", err) return false } var imgBytes bytes.Buffer if err = png.Encode(&imgBytes, img); err != nil { - ctx.ServerError("SettingsTwoFactor", err) + ctx.ServerError("SettingsTwoFactor: otpKey png encoding failed", err) return false } ctx.Data["QrUri"] = template.URL("data:image/png;base64," + base64.StdEncoding.EncodeToString(imgBytes.Bytes())) - err = ctx.Session.Set("twofaSecret", otpKey.Secret()) - if err != nil { - ctx.ServerError("SettingsTwoFactor", err) + + if err := ctx.Session.Set("twofaSecret", otpKey.Secret()); err != nil { + ctx.ServerError("SettingsTwoFactor: Failed to set session for twofaSecret", err) return false } - err = ctx.Session.Set("twofaUri", otpKey.String()) - if err != nil { - ctx.ServerError("SettingsTwoFactor", err) + + if err := ctx.Session.Set("twofaUri", otpKey.String()); err != nil { + ctx.ServerError("SettingsTwoFactor: Failed to set session for twofaUri", err) return false } + + // Here we're just going to try to release the session early + if err := ctx.Session.Release(); err != nil { + // we'll tolerate errors here as they *should* get saved elsewhere + log.Error("Unable to save changes to the session: %v", err) + } return true } @@ -126,12 +146,14 @@ func EnrollTwoFactor(ctx *context.Context) { t, err := models.GetTwoFactorByUID(ctx.User.ID) if t != nil { - // already enrolled - ctx.ServerError("SettingsTwoFactor", err) + // already enrolled - we should redirect back! + log.Warn("Trying to re-enroll %-v in twofa when already enrolled", ctx.User) + ctx.Flash.Error(ctx.Tr("setting.twofa_is_enrolled")) + ctx.Redirect(setting.AppSubURL + "/user/settings/security") return } if err != nil && !models.IsErrTwoFactorNotEnrolled(err) { - ctx.ServerError("SettingsTwoFactor", err) + ctx.ServerError("SettingsTwoFactor: GetTwoFactorByUID", err) return } @@ -150,11 +172,12 @@ func EnrollTwoFactorPost(ctx *context.Context, form auth.TwoFactorAuthForm) { t, err := models.GetTwoFactorByUID(ctx.User.ID) if t != nil { // already enrolled - ctx.ServerError("SettingsTwoFactor", err) + ctx.Flash.Error(ctx.Tr("setting.twofa_is_enrolled")) + ctx.Redirect(setting.AppSubURL + "/user/settings/security") return } if err != nil && !models.IsErrTwoFactorNotEnrolled(err) { - ctx.ServerError("SettingsTwoFactor", err) + ctx.ServerError("SettingsTwoFactor: Failed to check if already enrolled with GetTwoFactorByUID", err) return } @@ -181,30 +204,37 @@ func EnrollTwoFactorPost(ctx *context.Context, form auth.TwoFactorAuthForm) { } err = t.SetSecret(secret) if err != nil { - ctx.ServerError("SettingsTwoFactor", err) + ctx.ServerError("SettingsTwoFactor: Failed to set secret", err) return } token, err := t.GenerateScratchToken() if err != nil { - ctx.ServerError("SettingsTwoFactor", err) + ctx.ServerError("SettingsTwoFactor: Failed to generate scratch token", err) return } + // Now we have to delete the secrets - because if we fail to insert then it's highly likely that they have already been used + // If we can detect the unique constraint failure below we can move this to after the NewTwoFactor + if err := ctx.Session.Delete("twofaSecret"); err != nil { + // tolerate this failure - it's more important to continue + log.Error("Unable to delete twofaSecret from the session: Error: %v", err) + } + if err := ctx.Session.Delete("twofaUri"); err != nil { + // tolerate this failure - it's more important to continue + log.Error("Unable to delete twofaUri from the session: Error: %v", err) + } + if err := ctx.Session.Release(); err != nil { + // tolerate this failure - it's more important to continue + log.Error("Unable to save changes to the session: %v", err) + } + if err = models.NewTwoFactor(t); err != nil { - ctx.ServerError("SettingsTwoFactor", err) + // FIXME: We need to handle a unique constraint fail here it's entirely possible that another request has beaten us. + // If there is a unique constraint fail we should just tolerate the error + ctx.ServerError("SettingsTwoFactor: Failed to save two factor", err) return } - err = ctx.Session.Delete("twofaSecret") - if err != nil { - ctx.ServerError("SettingsTwoFactor", err) - return - } - err = ctx.Session.Delete("twofaUri") - if err != nil { - ctx.ServerError("SettingsTwoFactor", err) - return - } ctx.Flash.Success(ctx.Tr("settings.twofa_enrolled", token)) ctx.Redirect(setting.AppSubURL + "/user/settings/security") } diff --git a/routers/user/setting/security_u2f.go b/routers/user/setting/security_u2f.go index b733467b8..7e32b4aae 100644 --- a/routers/user/setting/security_u2f.go +++ b/routers/user/setting/security_u2f.go @@ -10,6 +10,7 @@ import ( "code.gitea.io/gitea/models" "code.gitea.io/gitea/modules/auth" "code.gitea.io/gitea/modules/context" + "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" "github.com/tstranex/u2f" @@ -26,9 +27,8 @@ func U2FRegister(ctx *context.Context, form auth.U2FRegistrationForm) { ctx.ServerError("NewChallenge", err) return } - err = ctx.Session.Set("u2fChallenge", challenge) - if err != nil { - ctx.ServerError("Session.Set", err) + if err := ctx.Session.Set("u2fChallenge", challenge); err != nil { + ctx.ServerError("Unable to set session key for u2fChallenge", err) return } regs, err := models.GetU2FRegistrationsByUID(ctx.User.ID) @@ -42,11 +42,15 @@ func U2FRegister(ctx *context.Context, form auth.U2FRegistrationForm) { return } } - err = ctx.Session.Set("u2fName", form.Name) - if err != nil { - ctx.ServerError("", err) + if err := ctx.Session.Set("u2fName", form.Name); err != nil { + ctx.ServerError("Unable to set session key for u2fName", err) return } + // Here we're just going to try to release the session early + if err := ctx.Session.Release(); err != nil { + // we'll tolerate errors here as they *should* get saved elsewhere + log.Error("Unable to save changes to the session: %v", err) + } ctx.JSON(200, u2f.NewWebRegisterRequest(challenge, regs.ToRegistrations())) }