From e35d2af2e56f4ecb3a4f6d1109d02c8aa1a6d182 Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Wed, 27 Dec 2023 11:59:01 +0100 Subject: [PATCH] Rate limit pre-activation email change separately Changing the email address before any email address is activated should be subject to a different rate limit than the normal activation email resending. If there's only one rate limit for both, then if a newly signed up quickly discovers they gave a wrong email address, they'd have to wait three minutes to change it. With the two separate limits, they don't - but they'll have to wait three minutes before they can change the email address again. The downside of this setup is that a malicious actor can alternate between resending and changing the email address (to something like `user+$idx@domain`, delivered to the same inbox) to effectively halving the rate limit. I do not think there's a better solution, and this feels like such a small attack surface that I'd deem it acceptable. The way the code works after this change is that `ActivatePost` will now check the `MailChangeLimit_user` key rather than `MailResendLimit_user`, and if we're within the limit, it will set `MailChangedJustNow_user`. The `Activate` method - which sends the activation email, whether it is a normal resend, or one following an email change - will check `MailChangedJustNow_user`, and if it is set, it will check the rate limit against `MailChangedLimit_user`, otherwise against `MailResendLimit_user`, and then will delete the `MailChangedJustNow_user` key from the cache. Fixes #2040. Signed-off-by: Gergely Nagy --- routers/web/auth/auth.go | 22 +++++++++++++++++++--- tests/integration/signup_test.go | 9 +++++++++ 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/routers/web/auth/auth.go b/routers/web/auth/auth.go index d28dc62ffb..ee6719649b 100644 --- a/routers/web/auth/auth.go +++ b/routers/web/auth/auth.go @@ -647,13 +647,22 @@ func Activate(ctx *context.Context) { } // Resend confirmation email. if setting.Service.RegisterEmailConfirm { - if ctx.Cache.IsExist("MailResendLimit_" + ctx.Doer.LowerName) { + var cacheKey string + if ctx.Cache.IsExist("MailChangedJustNow_" + ctx.Doer.LowerName) { + cacheKey = "MailChangedLimit_" + if err := ctx.Cache.Delete("MailChangedJustNow_" + ctx.Doer.LowerName); err != nil { + log.Error("Delete cache(MailChangedJustNow) fail: %v", err) + } + } else { + cacheKey = "MailResendLimit_" + } + if ctx.Cache.IsExist(cacheKey + ctx.Doer.LowerName) { ctx.Data["ResendLimited"] = true } else { ctx.Data["ActiveCodeLives"] = timeutil.MinutesToFriendly(setting.Service.ActiveCodeLives, ctx.Locale) mailer.SendActivateAccountMail(ctx.Locale, ctx.Doer) - if err := ctx.Cache.Put("MailResendLimit_"+ctx.Doer.LowerName, ctx.Doer.LowerName, 180); err != nil { + if err := ctx.Cache.Put(cacheKey+ctx.Doer.LowerName, ctx.Doer.LowerName, 180); err != nil { log.Error("Set cache(MailResendLimit) fail: %v", err) } } @@ -696,7 +705,7 @@ func ActivatePost(ctx *context.Context) { } // Change the primary email if setting.Service.RegisterEmailConfirm { - if false && ctx.Cache.IsExist("MailResendLimit_"+ctx.Doer.LowerName) { + if ctx.Cache.IsExist("MailChangeLimit_" + ctx.Doer.LowerName) { ctx.Data["ResendLimited"] = true } else { ctx.Data["ActiveCodeLives"] = timeutil.MinutesToFriendly(setting.Service.ActiveCodeLives, ctx.Locale) @@ -710,6 +719,13 @@ func ActivatePost(ctx *context.Context) { ctx.RenderWithErr(ctx.Tr("auth.change_unconfirmed_email_error", err), TplActivate, nil) return } + if err := ctx.Cache.Put("MailChangeLimit_"+ctx.Doer.LowerName, ctx.Doer.LowerName, 180); err != nil { + log.Error("Set cache(MailChangeLimit) fail: %v", err) + } + if err := ctx.Cache.Put("MailChangedJustNow_"+ctx.Doer.LowerName, ctx.Doer.LowerName, 180); err != nil { + log.Error("Set cache(MailChangedJustNow) fail: %v", err) + } + // Confirmation mail will be re-sent after the redirect to `/user/activate` below. } } else { diff --git a/tests/integration/signup_test.go b/tests/integration/signup_test.go index 9cee944edc..8b0013419f 100644 --- a/tests/integration/signup_test.go +++ b/tests/integration/signup_test.go @@ -124,6 +124,15 @@ func TestSignupEmailChangeForInactiveUser(t *testing.T) { // Verify that the email was updated user = unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: "exampleUserX"}) assert.Equal(t, "fine-email@example.com", user.Email) + + // Try to change the email again + req = NewRequestWithValues(t, "POST", "/user/activate", map[string]string{ + "email": "wrong-again@example.com", + }) + session.MakeRequest(t, req, http.StatusSeeOther) + // Verify that the email was NOT updated + user = unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: "exampleUserX"}) + assert.Equal(t, "fine-email@example.com", user.Email) } func TestSignupEmailChangeForActiveUser(t *testing.T) {