diff --git a/internal/api/magic_link.go b/internal/api/magic_link.go index 1422b33888..8819228917 100644 --- a/internal/api/magic_link.go +++ b/internal/api/magic_link.go @@ -78,6 +78,21 @@ func (a *API) MagicLink(w http.ResponseWriter, r *http.Request) error { return apierrors.NewInternalServerError("Database error finding user").WithInternalError(err) } } + if user != nil && user.IsBanned() { + // A banned user can't redeem the link, so don't send one. Respond as + // if it was sent so the ban status isn't revealed. Still apply the + // per-user max frequency check the real flow would have applied: + // sendMagicLink uses recovery_sent_at, and an unconfirmed user goes + // through signup, which uses confirmation_sent_at. + sentAt := user.RecoverySentAt + if !user.IsConfirmed() { + sentAt = user.ConfirmationSentAt + } + if err := validateSentWithinFrequencyLimit(sentAt, config.SMTP.MaxFrequency); err != nil { + return err + } + return sendJSON(w, http.StatusOK, make(map[string]string)) + } if user != nil { isNewUser = !user.IsConfirmed() } diff --git a/internal/api/mail.go b/internal/api/mail.go index c29f62e18c..d577d952dd 100644 --- a/internal/api/mail.go +++ b/internal/api/mail.go @@ -728,6 +728,15 @@ func validateSentWithinFrequencyLimit(sentAt *time.Time, frequency time.Duration return nil } +// validateSMSSentWithinFrequencyLimit is the SMS counterpart of +// validateSentWithinFrequencyLimit. +func validateSMSSentWithinFrequencyLimit(sentAt *time.Time, frequency time.Duration) error { + if sentAt != nil && !sentAt.Add(frequency).Before(time.Now()) { + return apierrors.NewTooManyRequestsError(apierrors.ErrorCodeOverSMSSendRateLimit, "%s", generateFrequencyLimitErrorMessage(sentAt, frequency)) + } + return nil +} + var emailLabelPattern = regexp.MustCompile("[+][^@]+@") func (a *API) checkEmailAddressAuthorization(email string) bool { diff --git a/internal/api/otp.go b/internal/api/otp.go index 5f12b0bbe3..3c122227d1 100644 --- a/internal/api/otp.go +++ b/internal/api/otp.go @@ -133,6 +133,17 @@ func (a *API) SmsOtp(w http.ResponseWriter, r *http.Request) error { return apierrors.NewInternalServerError("Database error finding user").WithInternalError(err) } } + if user != nil && user.IsBanned() { + // A banned user can't redeem the OTP, so don't send one. Respond as + // if it was sent so the ban status isn't revealed. Still apply the + // per-user max frequency check sendPhoneConfirmation would have + // applied (confirmed and unconfirmed users both use + // confirmation_sent_at). + if err := validateSMSSentWithinFrequencyLimit(user.ConfirmationSentAt, config.Sms.MaxFrequency); err != nil { + return err + } + return sendJSON(w, http.StatusOK, SmsOtpResponse{}) + } if user != nil { isNewUser = !user.IsPhoneConfirmed() } diff --git a/internal/api/otp_test.go b/internal/api/otp_test.go index adf5030749..96e06bee2f 100644 --- a/internal/api/otp_test.go +++ b/internal/api/otp_test.go @@ -442,3 +442,121 @@ func (ts *OtpTestSuite) TestOtpKeepsConfirmedPassword() { require.NoError(ts.T(), err) require.True(ts.T(), user.HasPassword()) } + +func (ts *OtpTestSuite) TestOtpBannedUser() { + ts.Config.SMTP.MaxFrequency = 0 + ts.Config.Sms.MaxFrequency = 0 + ts.Config.External.Phone.Enabled = true + ts.Config.Sms.TestOTP = map[string]string{"123456789": "000000"} + defer func() { + ts.Config.Sms.TestOTP = nil + }() + + u, err := models.NewUser("123456789", "banned@example.com", "password", ts.Config.JWT.Aud, nil) + require.NoError(ts.T(), err) + require.NoError(ts.T(), ts.API.db.Create(u)) + require.NoError(ts.T(), u.Confirm(ts.API.db)) + require.NoError(ts.T(), u.ConfirmPhone(ts.API.db)) + require.NoError(ts.T(), u.Ban(ts.API.db, time.Hour)) + + cases := []struct { + desc string + path string + body map[string]interface{} + }{ + { + desc: "Magic link via /otp", + path: "/otp", + body: map[string]interface{}{"email": u.GetEmail()}, + }, + { + desc: "Magic link via /magiclink", + path: "/magiclink", + body: map[string]interface{}{"email": u.GetEmail()}, + }, + { + desc: "SMS OTP via /otp", + path: "/otp", + body: map[string]interface{}{"phone": u.GetPhone()}, + }, + } + + for _, c := range cases { + ts.Run(c.desc, func() { + var buffer bytes.Buffer + require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(c.body)) + req := httptest.NewRequest(http.MethodPost, c.path, &buffer) + req.Header.Set("Content-Type", "application/json") + + w := httptest.NewRecorder() + ts.API.handler.ServeHTTP(w, req) + require.Equal(ts.T(), http.StatusOK, w.Code) + require.JSONEq(ts.T(), "{}", w.Body.String()) + + dbUser, err := models.FindUserByID(ts.API.db, u.ID) + require.NoError(ts.T(), err) + require.Empty(ts.T(), dbUser.RecoveryToken) + require.Nil(ts.T(), dbUser.RecoverySentAt) + require.Empty(ts.T(), dbUser.ConfirmationToken) + require.Nil(ts.T(), dbUser.ConfirmationSentAt) + }) + } + + ts.Run("Expired ban sends magic link again", func() { + pastBan := time.Now().Add(-time.Hour) + u.BannedUntil = &pastBan + require.NoError(ts.T(), ts.API.db.UpdateOnly(u, "banned_until")) + + var buffer bytes.Buffer + require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(map[string]interface{}{"email": u.GetEmail()})) + req := httptest.NewRequest(http.MethodPost, "/otp", &buffer) + req.Header.Set("Content-Type", "application/json") + + w := httptest.NewRecorder() + ts.API.handler.ServeHTTP(w, req) + require.Equal(ts.T(), http.StatusOK, w.Code) + + dbUser, err := models.FindUserByID(ts.API.db, u.ID) + require.NoError(ts.T(), err) + require.NotEmpty(ts.T(), dbUser.RecoveryToken) + require.NotNil(ts.T(), dbUser.RecoverySentAt) + }) +} + +func (ts *OtpTestSuite) TestOtpBannedUserMaxFrequency() { + origSMTP, origSms := ts.Config.SMTP.MaxFrequency, ts.Config.Sms.MaxFrequency + ts.Config.SMTP.MaxFrequency = time.Minute + ts.Config.Sms.MaxFrequency = time.Minute + ts.Config.External.Phone.Enabled = true + defer func() { + ts.Config.SMTP.MaxFrequency = origSMTP + ts.Config.Sms.MaxFrequency = origSms + }() + + sentAt := time.Now().Add(-10 * time.Second) + u, err := models.NewUser("987654321", "banned-freq@example.com", "password", ts.Config.JWT.Aud, nil) + require.NoError(ts.T(), err) + u.RecoverySentAt = &sentAt + u.ConfirmationSentAt = &sentAt + require.NoError(ts.T(), ts.API.db.Create(u)) + require.NoError(ts.T(), u.Confirm(ts.API.db)) + require.NoError(ts.T(), u.ConfirmPhone(ts.API.db)) + require.NoError(ts.T(), u.Ban(ts.API.db, time.Hour)) + + for _, c := range []struct { + path string + body map[string]interface{} + }{ + {"/otp", map[string]interface{}{"email": u.GetEmail()}}, + {"/magiclink", map[string]interface{}{"email": u.GetEmail()}}, + {"/otp", map[string]interface{}{"phone": u.GetPhone()}}, + } { + var buffer bytes.Buffer + require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(c.body)) + req := httptest.NewRequest(http.MethodPost, c.path, &buffer) + req.Header.Set("Content-Type", "application/json") + w := httptest.NewRecorder() + ts.API.handler.ServeHTTP(w, req) + require.Equal(ts.T(), http.StatusTooManyRequests, w.Code, c.path) + } +} diff --git a/internal/api/phone.go b/internal/api/phone.go index 75659a3a80..864ba41740 100644 --- a/internal/api/phone.go +++ b/internal/api/phone.go @@ -70,8 +70,8 @@ func (a *API) sendPhoneConfirmation(r *http.Request, tx *storage.Connection, use // intentionally keeping this before the test OTP, so that the behavior // of regular and test OTPs is similar - if sentAt != nil && !sentAt.Add(config.Sms.MaxFrequency).Before(time.Now()) { - return "", apierrors.NewTooManyRequestsError(apierrors.ErrorCodeOverSMSSendRateLimit, "%s", generateFrequencyLimitErrorMessage(sentAt, config.Sms.MaxFrequency)) + if err := validateSMSSentWithinFrequencyLimit(sentAt, config.Sms.MaxFrequency); err != nil { + return "", err } now := time.Now() diff --git a/internal/api/recover.go b/internal/api/recover.go index 7c967aaf00..c31e816be5 100644 --- a/internal/api/recover.go +++ b/internal/api/recover.go @@ -56,6 +56,17 @@ func (a *API) Recover(w http.ResponseWriter, r *http.Request) error { } return apierrors.NewInternalServerError("Unable to process request").WithInternalError(err) } + if user.IsBanned() { + // A banned user can't redeem the recovery link, so don't send one. + // Respond as for an unknown email so the ban status isn't revealed. + // Still apply the per-user max frequency check sendPasswordRecovery + // would have applied, so the 429 behavior is the same as for any + // other user. + if err := validateSentWithinFrequencyLimit(user.RecoverySentAt, config.SMTP.MaxFrequency); err != nil { + return err + } + return sendJSON(w, http.StatusOK, map[string]string{}) + } if isPKCEFlow(flowType) { if _, err := generateFlowState(db, models.Recovery.String(), models.Recovery, params.CodeChallengeMethod, params.CodeChallenge, &(user.ID)); err != nil { return err diff --git a/internal/api/recover_test.go b/internal/api/recover_test.go index a7e655c596..95b69bc79a 100644 --- a/internal/api/recover_test.go +++ b/internal/api/recover_test.go @@ -151,3 +151,79 @@ func (ts *RecoverTestSuite) TestRecover_NoSideChannelLeak() { ts.API.handler.ServeHTTP(w, req) assert.Equal(ts.T(), http.StatusOK, w.Code) } + +func (ts *RecoverTestSuite) TestRecover_BannedUser() { + u, err := models.FindUserByEmailAndAudience(ts.API.db, "test@example.com", ts.Config.JWT.Aud) + require.NoError(ts.T(), err) + require.NoError(ts.T(), u.Ban(ts.API.db, time.Hour)) + + doRecover := func() *httptest.ResponseRecorder { + var buffer bytes.Buffer + require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(map[string]interface{}{ + "email": "test@example.com", + })) + req := httptest.NewRequest(http.MethodPost, "http://localhost/recover", &buffer) + req.Header.Set("Content-Type", "application/json") + w := httptest.NewRecorder() + ts.API.handler.ServeHTTP(w, req) + return w + } + + w := doRecover() + assert.Equal(ts.T(), http.StatusOK, w.Code) + assert.JSONEq(ts.T(), "{}", w.Body.String()) + + u, err = models.FindUserByEmailAndAudience(ts.API.db, "test@example.com", ts.Config.JWT.Aud) + require.NoError(ts.T(), err) + assert.Empty(ts.T(), u.RecoveryToken) + assert.Nil(ts.T(), u.RecoverySentAt) + + // once the ban has expired, the recovery email is sent again + pastBan := time.Now().Add(-time.Hour) + u.BannedUntil = &pastBan + require.NoError(ts.T(), ts.API.db.UpdateOnly(u, "banned_until")) + + w = doRecover() + assert.Equal(ts.T(), http.StatusOK, w.Code) + + u, err = models.FindUserByEmailAndAudience(ts.API.db, "test@example.com", ts.Config.JWT.Aud) + require.NoError(ts.T(), err) + assert.NotEmpty(ts.T(), u.RecoveryToken) + require.NotNil(ts.T(), u.RecoverySentAt) + assert.WithinDuration(ts.T(), time.Now(), *u.RecoverySentAt, 1*time.Second) +} + +func (ts *RecoverTestSuite) TestRecover_BannedUserMaxFrequency() { + origFreq := ts.Config.SMTP.MaxFrequency + ts.Config.SMTP.MaxFrequency = time.Minute + defer func() { ts.Config.SMTP.MaxFrequency = origFreq }() + + u, err := models.FindUserByEmailAndAudience(ts.API.db, "test@example.com", ts.Config.JWT.Aud) + require.NoError(ts.T(), err) + sentAt := time.Now().Add(-10 * time.Second) + u.RecoverySentAt = &sentAt + require.NoError(ts.T(), ts.API.db.UpdateOnly(u, "recovery_sent_at")) + + doRecover := func() int { + var buffer bytes.Buffer + require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(map[string]interface{}{"email": "test@example.com"})) + req := httptest.NewRequest(http.MethodPost, "http://localhost/recover", &buffer) + req.Header.Set("Content-Type", "application/json") + w := httptest.NewRecorder() + ts.API.handler.ServeHTTP(w, req) + return w.Code + } + + // not banned: rate limited + assert.Equal(ts.T(), http.StatusTooManyRequests, doRecover()) + + // banned: indistinguishable from the not banned response + require.NoError(ts.T(), u.Ban(ts.API.db, time.Hour)) + assert.Equal(ts.T(), http.StatusTooManyRequests, doRecover()) + + // banned, but outside of the max frequency window: 200 + old := time.Now().Add(-2 * time.Minute) + u.RecoverySentAt = &old + require.NoError(ts.T(), ts.API.db.UpdateOnly(u, "recovery_sent_at")) + assert.Equal(ts.T(), http.StatusOK, doRecover()) +} diff --git a/internal/api/resend.go b/internal/api/resend.go index 22bac98ece..4207dc1c9a 100644 --- a/internal/api/resend.go +++ b/internal/api/resend.go @@ -96,7 +96,6 @@ func (a *API) Resend(w http.ResponseWriter, r *http.Request) error { } return apierrors.NewInternalServerError("Unable to process request").WithInternalError(err) } - switch params.Type { case mail.SignupVerification: if user.IsConfirmed() { @@ -120,6 +119,29 @@ func (a *API) Resend(w http.ResponseWriter, r *http.Request) error { } } + if user.IsBanned() { + // A banned user can't redeem the token, so don't send one. Respond as + // for an unknown user so the ban status isn't revealed. Still apply the + // per-user max frequency check the real send would have applied, so + // the 429 behavior is the same as for any other user. This is after + // the switch above so the early 200 responses stay identical too. + var freqErr error + switch params.Type { + case mail.SignupVerification: + freqErr = validateSentWithinFrequencyLimit(user.ConfirmationSentAt, config.SMTP.MaxFrequency) + case smsVerification: + freqErr = validateSMSSentWithinFrequencyLimit(user.ConfirmationSentAt, config.Sms.MaxFrequency) + case mail.EmailChangeVerification: + freqErr = validateSentWithinFrequencyLimit(user.EmailChangeSentAt, config.SMTP.MaxFrequency) + case phoneChangeVerification: + freqErr = validateSMSSentWithinFrequencyLimit(user.PhoneChangeSentAt, config.Sms.MaxFrequency) + } + if freqErr != nil { + return freqErr + } + return sendJSON(w, http.StatusOK, map[string]string{}) + } + messageID := "" err = db.Transaction(func(tx *storage.Connection) error { switch params.Type { diff --git a/internal/api/resend_test.go b/internal/api/resend_test.go index 78cd839454..96f034c25c 100644 --- a/internal/api/resend_test.go +++ b/internal/api/resend_test.go @@ -346,3 +346,102 @@ func (ts *ResendTestSuite) TestResendPKCESuccess() { require.True(ts.T(), strings.HasPrefix(dbUser.EmailChangeTokenNew, PKCEPrefix), "expected pkce_ prefix on EmailChangeTokenNew, got: %s", dbUser.EmailChangeTokenNew) }) } + +func (ts *ResendTestSuite) TestResendBannedUser() { + // Avoid max freq limit error + sentAt := time.Now().Add(-1 * time.Minute) + + // disable secure email change + ts.Config.Mailer.SecureEmailChangeEnabled = false + + u, err := models.NewUser("", "banned@example.com", "password", ts.Config.JWT.Aud, nil) + require.NoError(ts.T(), err) + u.ConfirmationToken = "123456" + u.ConfirmationSentAt = &sentAt + u.EmailChange = "new@example.com" + u.EmailChangeTokenNew = "654321" + u.EmailChangeSentAt = &sentAt + require.NoError(ts.T(), ts.API.db.Create(u)) + require.NoError(ts.T(), u.Ban(ts.API.db, time.Hour)) + + cases := []struct { + desc string + params map[string]interface{} + }{ + { + desc: "Resend signup confirmation", + params: map[string]interface{}{"type": "signup", "email": u.GetEmail()}, + }, + { + desc: "Resend email change", + params: map[string]interface{}{"type": "email_change", "email": u.GetEmail()}, + }, + } + + doResend := func(params map[string]interface{}) *httptest.ResponseRecorder { + var buffer bytes.Buffer + require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(params)) + req := httptest.NewRequest(http.MethodPost, "http://localhost/resend", &buffer) + req.Header.Set("Content-Type", "application/json") + w := httptest.NewRecorder() + ts.API.handler.ServeHTTP(w, req) + return w + } + + for _, c := range cases { + ts.Run(c.desc, func() { + w := doResend(c.params) + require.Equal(ts.T(), http.StatusOK, w.Code) + require.JSONEq(ts.T(), "{}", w.Body.String()) + + dbUser, err := models.FindUserByID(ts.API.db, u.ID) + require.NoError(ts.T(), err) + require.Equal(ts.T(), u.ConfirmationToken, dbUser.ConfirmationToken) + require.WithinDuration(ts.T(), sentAt, *dbUser.ConfirmationSentAt, time.Second) + require.Equal(ts.T(), u.EmailChangeTokenNew, dbUser.EmailChangeTokenNew) + require.WithinDuration(ts.T(), sentAt, *dbUser.EmailChangeSentAt, time.Second) + }) + } + + ts.Run("Expired ban resends signup confirmation", func() { + pastBan := time.Now().Add(-time.Hour) + u.BannedUntil = &pastBan + require.NoError(ts.T(), ts.API.db.UpdateOnly(u, "banned_until")) + + w := doResend(map[string]interface{}{"type": "signup", "email": u.GetEmail()}) + require.Equal(ts.T(), http.StatusOK, w.Code) + + dbUser, err := models.FindUserByID(ts.API.db, u.ID) + require.NoError(ts.T(), err) + require.NotEqual(ts.T(), u.ConfirmationToken, dbUser.ConfirmationToken) + require.WithinDuration(ts.T(), time.Now(), *dbUser.ConfirmationSentAt, time.Second) + }) +} + +func (ts *ResendTestSuite) TestResendBannedUserMaxFrequency() { + origFreq := ts.Config.SMTP.MaxFrequency + ts.Config.SMTP.MaxFrequency = time.Minute + defer func() { ts.Config.SMTP.MaxFrequency = origFreq }() + + sentAt := time.Now().Add(-10 * time.Second) + u, err := models.NewUser("", "banned-freq@example.com", "password", ts.Config.JWT.Aud, nil) + require.NoError(ts.T(), err) + u.ConfirmationToken = "123456" + u.ConfirmationSentAt = &sentAt + require.NoError(ts.T(), ts.API.db.Create(u)) + + doResend := func() int { + var buffer bytes.Buffer + require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(map[string]interface{}{"type": "signup", "email": u.GetEmail()})) + req := httptest.NewRequest(http.MethodPost, "http://localhost/resend", &buffer) + req.Header.Set("Content-Type", "application/json") + w := httptest.NewRecorder() + ts.API.handler.ServeHTTP(w, req) + return w.Code + } + + require.Equal(ts.T(), http.StatusTooManyRequests, doResend()) + + require.NoError(ts.T(), u.Ban(ts.API.db, time.Hour)) + require.Equal(ts.T(), http.StatusTooManyRequests, doResend()) +}