From 220dda522e305f06e060afb57e7849884f894012 Mon Sep 17 00:00:00 2001 From: joel Date: Wed, 17 Apr 2024 18:37:22 +0800 Subject: [PATCH 1/2] fix: calculate seconds left --- internal/api/errors.go | 7 +++++++ internal/api/external.go | 2 +- internal/api/identity.go | 3 ++- internal/api/magic_link.go | 2 +- internal/api/reauthenticate.go | 1 + internal/api/recover.go | 3 ++- internal/api/resend.go | 10 ++++++---- internal/api/signup.go | 15 +++++---------- internal/api/user.go | 2 +- 9 files changed, 26 insertions(+), 19 deletions(-) diff --git a/internal/api/errors.go b/internal/api/errors.go index 2d40a53f4..e821409a7 100644 --- a/internal/api/errors.go +++ b/internal/api/errors.go @@ -6,6 +6,7 @@ import ( "net/http" "os" "runtime/debug" + "time" "github.com/pkg/errors" "github.com/supabase/auth/internal/observability" @@ -312,3 +313,9 @@ func HandleResponseError(err error, w http.ResponseWriter, r *http.Request) { } } } + +func generateFrequencyLimitErrorMessage(timeStamp *time.Time, maxFrequency time.Duration) string { + now := time.Now() + left := timeStamp.Add(maxFrequency).Sub(now) / time.Second + return fmt.Sprintf("For security purposes, you can only request this after %d seconds.", left) +} diff --git a/internal/api/external.go b/internal/api/external.go index a8048fb26..b76e2efe3 100644 --- a/internal/api/external.go +++ b/internal/api/external.go @@ -388,7 +388,7 @@ func (a *API) createAccountFromExternalIdentity(tx *storage.Connection, r *http. if decision.CandidateEmail.Email != "" { if terr = a.sendConfirmation(r, tx, user, models.ImplicitFlow); terr != nil { if errors.Is(terr, MaxFrequencyLimitError) { - return nil, tooManyRequestsError(ErrorCodeOverEmailSendRateLimit, "For security purposes, you can only request this once every minute") + return nil, tooManyRequestsError(ErrorCodeOverEmailSendRateLimit, generateFrequencyLimitErrorMessage(user.ConfirmationSentAt, config.SMTP.MaxFrequency)) } return nil, internalServerError("Error sending confirmation mail").WithInternalError(terr) } diff --git a/internal/api/identity.go b/internal/api/identity.go index 7acf7c25c..ce95f26e6 100644 --- a/internal/api/identity.go +++ b/internal/api/identity.go @@ -106,6 +106,7 @@ func (a *API) LinkIdentity(w http.ResponseWriter, r *http.Request) error { } func (a *API) linkIdentityToUser(r *http.Request, ctx context.Context, tx *storage.Connection, userData *provider.UserProvidedData, providerType string) (*models.User, error) { + config := a.config targetUser := getTargetUser(ctx) identity, terr := models.FindIdentityByIdAndProvider(tx, userData.Metadata.Subject, providerType) if terr != nil { @@ -133,7 +134,7 @@ func (a *API) linkIdentityToUser(r *http.Request, ctx context.Context, tx *stora if !userData.Metadata.EmailVerified { if terr := a.sendConfirmation(r, tx, targetUser, models.ImplicitFlow); terr != nil { if errors.Is(terr, MaxFrequencyLimitError) { - return nil, tooManyRequestsError(ErrorCodeOverSMSSendRateLimit, "For security purposes, you can only request this once every minute") + return nil, tooManyRequestsError(ErrorCodeOverSMSSendRateLimit, generateFrequencyLimitErrorMessage(targetUser.ConfirmationSentAt, config.Sms.MaxFrequency)) } } return nil, storage.NewCommitWithError(unprocessableEntityError(ErrorCodeEmailNotConfirmed, "Unverified email with %v. A confirmation email has been sent to your %v email", providerType, providerType)) diff --git a/internal/api/magic_link.go b/internal/api/magic_link.go index e197d72f6..eeabafd39 100644 --- a/internal/api/magic_link.go +++ b/internal/api/magic_link.go @@ -142,7 +142,7 @@ func (a *API) MagicLink(w http.ResponseWriter, r *http.Request) error { }) if err != nil { if errors.Is(err, MaxFrequencyLimitError) { - return tooManyRequestsError(ErrorCodeOverEmailSendRateLimit, "For security purposes, you can only request this once every 60 seconds") + return tooManyRequestsError(ErrorCodeOverEmailSendRateLimit, generateFrequencyLimitErrorMessage(user.RecoverySentAt, config.SMTP.MaxFrequency)) } return internalServerError("Error sending magic link").WithInternalError(err) } diff --git a/internal/api/reauthenticate.go b/internal/api/reauthenticate.go index cf86d102f..7ff119b4b 100644 --- a/internal/api/reauthenticate.go +++ b/internal/api/reauthenticate.go @@ -64,6 +64,7 @@ func (a *API) Reauthenticate(w http.ResponseWriter, r *http.Request) error { reason = ErrorCodeOverSMSSendRateLimit } + // TODO: convert this return tooManyRequestsError(reason, "For security purposes, you can only request this once every 60 seconds") } return err diff --git a/internal/api/recover.go b/internal/api/recover.go index 0fa9760ae..c02bebb5c 100644 --- a/internal/api/recover.go +++ b/internal/api/recover.go @@ -32,6 +32,7 @@ func (p *RecoverParams) Validate() error { // Recover sends a recovery email func (a *API) Recover(w http.ResponseWriter, r *http.Request) error { ctx := r.Context() + config := a.config db := a.db.WithContext(ctx) params := &RecoverParams{} if err := retrieveRequestParams(r, params); err != nil { @@ -68,7 +69,7 @@ func (a *API) Recover(w http.ResponseWriter, r *http.Request) error { }) if err != nil { if errors.Is(err, MaxFrequencyLimitError) { - return tooManyRequestsError(ErrorCodeOverEmailSendRateLimit, "For security purposes, you can only request this once every 60 seconds") + return tooManyRequestsError(ErrorCodeOverEmailSendRateLimit, generateFrequencyLimitErrorMessage(user.RecoverySentAt, config.SMTP.MaxFrequency)) } return internalServerError("Unable to process request").WithInternalError(err) } diff --git a/internal/api/resend.go b/internal/api/resend.go index b9e16df51..6724fb698 100644 --- a/internal/api/resend.go +++ b/internal/api/resend.go @@ -3,7 +3,6 @@ package api import ( "errors" "net/http" - "time" "github.com/supabase/auth/internal/api/sms_provider" "github.com/supabase/auth/internal/conf" @@ -154,12 +153,15 @@ func (a *API) Resend(w http.ResponseWriter, r *http.Request) error { if err != nil { if errors.Is(err, MaxFrequencyLimitError) { reason := ErrorCodeOverEmailSendRateLimit - if params.Type == smsVerification || params.Type == phoneChangeVerification { + if params.Type == smsVerification { reason = ErrorCodeOverSMSSendRateLimit + return tooManyRequestsError(reason, generateFrequencyLimitErrorMessage(user.ConfirmationSentAt, config.Sms.MaxFrequency)) + } else if params.Type == phoneChangeVerification { + reason = ErrorCodeOverSMSSendRateLimit + return tooManyRequestsError(reason, generateFrequencyLimitErrorMessage(user.PhoneChangeSentAt, config.Sms.MaxFrequency)) } - until := time.Until(user.ConfirmationSentAt.Add(config.SMTP.MaxFrequency)) / time.Second - return tooManyRequestsError(reason, "For security purposes, you can only request this once every %d seconds.", until) + return tooManyRequestsError(reason, generateFrequencyLimitErrorMessage(user.ConfirmationSentAt, config.SMTP.MaxFrequency)) } return internalServerError("Unable to process request").WithInternalError(err) } diff --git a/internal/api/signup.go b/internal/api/signup.go index 75c287cff..2cce3ad81 100644 --- a/internal/api/signup.go +++ b/internal/api/signup.go @@ -2,7 +2,6 @@ package api import ( "context" - "fmt" "net/http" "time" @@ -246,9 +245,7 @@ func (a *API) Signup(w http.ResponseWriter, r *http.Request) error { } if terr = a.sendConfirmation(r, tx, user, flowType); terr != nil { if errors.Is(terr, MaxFrequencyLimitError) { - now := time.Now() - left := user.ConfirmationSentAt.Add(config.SMTP.MaxFrequency).Sub(now) / time.Second - return tooManyRequestsError(ErrorCodeOverEmailSendRateLimit, fmt.Sprintf("For security purposes, you can only request this after %d seconds.", left)) + return tooManyRequestsError(ErrorCodeOverEmailSendRateLimit, generateFrequencyLimitErrorMessage(user.ConfirmationSentAt, config.SMTP.MaxFrequency)) } return internalServerError("Error sending confirmation mail").WithInternalError(terr) } @@ -284,13 +281,11 @@ func (a *API) Signup(w http.ResponseWriter, r *http.Request) error { }) if err != nil { - reason := ErrorCodeOverEmailSendRateLimit - if params.Provider == "phone" { - reason = ErrorCodeOverSMSSendRateLimit - } - if errors.Is(err, MaxFrequencyLimitError) { - return tooManyRequestsError(reason, "For security purposes, you can only request this once every minute") + if params.Provider == "phone" { + return tooManyRequestsError(ErrorCodeOverSMSSendRateLimit, generateFrequencyLimitErrorMessage(user.ConfirmationSentAt, config.Sms.MaxFrequency)) + } + return tooManyRequestsError(ErrorCodeOverEmailSendRateLimit, generateFrequencyLimitErrorMessage(user.ConfirmationSentAt, config.SMTP.MaxFrequency)) } else if errors.Is(err, UserExistsError) { err = db.Transaction(func(tx *storage.Connection) error { if terr := models.NewAuditLogEntry(r, tx, user, models.UserRepeatedSignUpAction, "", map[string]interface{}{ diff --git a/internal/api/user.go b/internal/api/user.go index 33c35aa57..e76b77453 100644 --- a/internal/api/user.go +++ b/internal/api/user.go @@ -214,7 +214,7 @@ func (a *API) UserUpdate(w http.ResponseWriter, r *http.Request) error { } if terr = a.sendEmailChange(r, tx, user, params.Email, flowType); terr != nil { if errors.Is(terr, MaxFrequencyLimitError) { - return tooManyRequestsError(ErrorCodeOverEmailSendRateLimit, "For security purposes, you can only request this once every 60 seconds") + return tooManyRequestsError(ErrorCodeOverEmailSendRateLimit, generateFrequencyLimitErrorMessage(user.EmailChangeSentAt, config.SMTP.MaxFrequency)) } return internalServerError("Error sending change email").WithInternalError(terr) } From 9db8cecef8d7d0b6ccfcf18d1b86509241671570 Mon Sep 17 00:00:00 2001 From: joel Date: Wed, 15 May 2024 07:25:02 +0300 Subject: [PATCH 2/2] fix: show seconds since last retry on all core mail routes --- internal/api/external.go | 2 +- internal/api/identity.go | 3 +-- internal/api/reauthenticate.go | 1 - internal/api/recover.go | 3 +-- internal/api/resend.go | 10 ++++------ internal/api/signup.go | 10 ++++++---- 6 files changed, 13 insertions(+), 16 deletions(-) diff --git a/internal/api/external.go b/internal/api/external.go index b76e2efe3..a8048fb26 100644 --- a/internal/api/external.go +++ b/internal/api/external.go @@ -388,7 +388,7 @@ func (a *API) createAccountFromExternalIdentity(tx *storage.Connection, r *http. if decision.CandidateEmail.Email != "" { if terr = a.sendConfirmation(r, tx, user, models.ImplicitFlow); terr != nil { if errors.Is(terr, MaxFrequencyLimitError) { - return nil, tooManyRequestsError(ErrorCodeOverEmailSendRateLimit, generateFrequencyLimitErrorMessage(user.ConfirmationSentAt, config.SMTP.MaxFrequency)) + return nil, tooManyRequestsError(ErrorCodeOverEmailSendRateLimit, "For security purposes, you can only request this once every minute") } return nil, internalServerError("Error sending confirmation mail").WithInternalError(terr) } diff --git a/internal/api/identity.go b/internal/api/identity.go index ce95f26e6..7acf7c25c 100644 --- a/internal/api/identity.go +++ b/internal/api/identity.go @@ -106,7 +106,6 @@ func (a *API) LinkIdentity(w http.ResponseWriter, r *http.Request) error { } func (a *API) linkIdentityToUser(r *http.Request, ctx context.Context, tx *storage.Connection, userData *provider.UserProvidedData, providerType string) (*models.User, error) { - config := a.config targetUser := getTargetUser(ctx) identity, terr := models.FindIdentityByIdAndProvider(tx, userData.Metadata.Subject, providerType) if terr != nil { @@ -134,7 +133,7 @@ func (a *API) linkIdentityToUser(r *http.Request, ctx context.Context, tx *stora if !userData.Metadata.EmailVerified { if terr := a.sendConfirmation(r, tx, targetUser, models.ImplicitFlow); terr != nil { if errors.Is(terr, MaxFrequencyLimitError) { - return nil, tooManyRequestsError(ErrorCodeOverSMSSendRateLimit, generateFrequencyLimitErrorMessage(targetUser.ConfirmationSentAt, config.Sms.MaxFrequency)) + return nil, tooManyRequestsError(ErrorCodeOverSMSSendRateLimit, "For security purposes, you can only request this once every minute") } } return nil, storage.NewCommitWithError(unprocessableEntityError(ErrorCodeEmailNotConfirmed, "Unverified email with %v. A confirmation email has been sent to your %v email", providerType, providerType)) diff --git a/internal/api/reauthenticate.go b/internal/api/reauthenticate.go index 7ff119b4b..cf86d102f 100644 --- a/internal/api/reauthenticate.go +++ b/internal/api/reauthenticate.go @@ -64,7 +64,6 @@ func (a *API) Reauthenticate(w http.ResponseWriter, r *http.Request) error { reason = ErrorCodeOverSMSSendRateLimit } - // TODO: convert this return tooManyRequestsError(reason, "For security purposes, you can only request this once every 60 seconds") } return err diff --git a/internal/api/recover.go b/internal/api/recover.go index c02bebb5c..0fa9760ae 100644 --- a/internal/api/recover.go +++ b/internal/api/recover.go @@ -32,7 +32,6 @@ func (p *RecoverParams) Validate() error { // Recover sends a recovery email func (a *API) Recover(w http.ResponseWriter, r *http.Request) error { ctx := r.Context() - config := a.config db := a.db.WithContext(ctx) params := &RecoverParams{} if err := retrieveRequestParams(r, params); err != nil { @@ -69,7 +68,7 @@ func (a *API) Recover(w http.ResponseWriter, r *http.Request) error { }) if err != nil { if errors.Is(err, MaxFrequencyLimitError) { - return tooManyRequestsError(ErrorCodeOverEmailSendRateLimit, generateFrequencyLimitErrorMessage(user.RecoverySentAt, config.SMTP.MaxFrequency)) + return tooManyRequestsError(ErrorCodeOverEmailSendRateLimit, "For security purposes, you can only request this once every 60 seconds") } return internalServerError("Unable to process request").WithInternalError(err) } diff --git a/internal/api/resend.go b/internal/api/resend.go index 6724fb698..b9e16df51 100644 --- a/internal/api/resend.go +++ b/internal/api/resend.go @@ -3,6 +3,7 @@ package api import ( "errors" "net/http" + "time" "github.com/supabase/auth/internal/api/sms_provider" "github.com/supabase/auth/internal/conf" @@ -153,15 +154,12 @@ func (a *API) Resend(w http.ResponseWriter, r *http.Request) error { if err != nil { if errors.Is(err, MaxFrequencyLimitError) { reason := ErrorCodeOverEmailSendRateLimit - if params.Type == smsVerification { + if params.Type == smsVerification || params.Type == phoneChangeVerification { reason = ErrorCodeOverSMSSendRateLimit - return tooManyRequestsError(reason, generateFrequencyLimitErrorMessage(user.ConfirmationSentAt, config.Sms.MaxFrequency)) - } else if params.Type == phoneChangeVerification { - reason = ErrorCodeOverSMSSendRateLimit - return tooManyRequestsError(reason, generateFrequencyLimitErrorMessage(user.PhoneChangeSentAt, config.Sms.MaxFrequency)) } - return tooManyRequestsError(reason, generateFrequencyLimitErrorMessage(user.ConfirmationSentAt, config.SMTP.MaxFrequency)) + until := time.Until(user.ConfirmationSentAt.Add(config.SMTP.MaxFrequency)) / time.Second + return tooManyRequestsError(reason, "For security purposes, you can only request this once every %d seconds.", until) } return internalServerError("Unable to process request").WithInternalError(err) } diff --git a/internal/api/signup.go b/internal/api/signup.go index 2cce3ad81..b396178db 100644 --- a/internal/api/signup.go +++ b/internal/api/signup.go @@ -281,11 +281,13 @@ func (a *API) Signup(w http.ResponseWriter, r *http.Request) error { }) if err != nil { + reason := ErrorCodeOverEmailSendRateLimit + if params.Provider == "phone" { + reason = ErrorCodeOverSMSSendRateLimit + } + if errors.Is(err, MaxFrequencyLimitError) { - if params.Provider == "phone" { - return tooManyRequestsError(ErrorCodeOverSMSSendRateLimit, generateFrequencyLimitErrorMessage(user.ConfirmationSentAt, config.Sms.MaxFrequency)) - } - return tooManyRequestsError(ErrorCodeOverEmailSendRateLimit, generateFrequencyLimitErrorMessage(user.ConfirmationSentAt, config.SMTP.MaxFrequency)) + return tooManyRequestsError(reason, "For security purposes, you can only request this once every minute") } else if errors.Is(err, UserExistsError) { err = db.Transaction(func(tx *storage.Connection) error { if terr := models.NewAuditLogEntry(r, tx, user, models.UserRepeatedSignUpAction, "", map[string]interface{}{