fix: don't send OTPs and links to banned users

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()
 	}

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 {

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()
 	}

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)
+	}
+}

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()

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

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())
+}

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 {