Infrastructure/forgejo
0
Fork 0
mirror of https://codeberg.org/forgejo/forgejo.git synced 2024-12-22 23:33:15 -05:00
Code Issues Activity

Rate limit pre-activation email change separately

Browse source 
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 <forgejo@gergo.csillger.hu>
This commit is contained in:
Gergely Nagy 2023-12-27 11:59:01 +01:00
parent 43ded8ee30
commit e35d2af2e5
No known key found for this signature in database
2 changed files with 28 additions and 3 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

22
routers/web/auth/auth.go
View file

@ -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 {

9
tests/integration/signup_test.go
View file

@ -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) {