Skip to content

Commit

Permalink
fix: apply shared limiters before email / sms is sent (#1748)
Browse files Browse the repository at this point in the history
## What kind of change does this PR introduce?
* Fixes #1236
* Reduces the number of false positives arising from validation errors
when counting rate limits for emails / sms sent
* This change applies the shared rate limiter for email and phone
functions before the actual email / sms is being sent out rather than at
the start of the request
* The `limitEmailOrPhoneSentHandler()` now initialises the rate limiters
and sets it in the request context so we can subsequently retrieve it
right before the email / sms is sent

## What is the current behavior?

Please link any relevant issues here.

## What is the new behavior?

Feel free to include screenshots if it includes visual changes.

## Additional context

Add any other context or screenshots.
  • Loading branch information
kangmingtay authored Aug 28, 2024
1 parent d03a54e commit bf276ab
Show file tree
Hide file tree
Showing 15 changed files with 158 additions and 165 deletions.
19 changes: 19 additions & 0 deletions internal/api/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"net/url"

"github.com/didip/tollbooth/v5/limiter"
jwt "github.com/golang-jwt/jwt/v5"
"github.com/supabase/auth/internal/models"
)
Expand Down Expand Up @@ -31,6 +32,7 @@ const (
ssoProviderKey = contextKey("sso_provider")
externalHostKey = contextKey("external_host")
flowStateKey = contextKey("flow_state_id")
sharedLimiterKey = contextKey("shared_limiter")
)

// withToken adds the JWT token to the context.
Expand Down Expand Up @@ -241,3 +243,20 @@ func getExternalHost(ctx context.Context) *url.URL {
}
return obj.(*url.URL)
}

type SharedLimiter struct {
EmailLimiter *limiter.Limiter
PhoneLimiter *limiter.Limiter
}

func withLimiter(ctx context.Context, limiter *SharedLimiter) context.Context {
return context.WithValue(ctx, sharedLimiterKey, limiter)
}

func getLimiter(ctx context.Context) *SharedLimiter {
obj := ctx.Value(sharedLimiterKey)
if obj == nil {
return nil
}
return obj.(*SharedLimiter)
}
6 changes: 1 addition & 5 deletions internal/api/external.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package api

import (
"context"
"errors"
"fmt"
"net/http"
"net/url"
Expand Down Expand Up @@ -383,10 +382,7 @@ func (a *API) createAccountFromExternalIdentity(tx *storage.Connection, r *http.
emailConfirmationSent := false
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, internalServerError("Error sending confirmation mail").WithInternalError(terr)
return nil, terr
}
emailConfirmationSent = true
}
Expand Down
5 changes: 1 addition & 4 deletions internal/api/identity.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (
"github.com/fatih/structs"
"github.com/go-chi/chi/v5"
"github.com/gofrs/uuid"
"github.com/pkg/errors"
"github.com/supabase/auth/internal/api/provider"
"github.com/supabase/auth/internal/models"
"github.com/supabase/auth/internal/storage"
Expand Down Expand Up @@ -133,9 +132,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, terr
}
return nil, storage.NewCommitWithError(unprocessableEntityError(ErrorCodeEmailNotConfirmed, "Unverified email with %v. A confirmation email has been sent to your %v email", providerType, providerType))
}
Expand Down
2 changes: 1 addition & 1 deletion internal/api/invite.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ func (a *API) Invite(w http.ResponseWriter, r *http.Request) error {
}

if err := a.sendInvite(r, tx, user); err != nil {
return internalServerError("Error inviting user").WithInternalError(err)
return err
}
return nil
})
Expand Down
6 changes: 1 addition & 5 deletions internal/api/magic_link.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package api
import (
"bytes"
"encoding/json"
"errors"
"fmt"
"io"
"net/http"
Expand Down Expand Up @@ -141,10 +140,7 @@ func (a *API) MagicLink(w http.ResponseWriter, r *http.Request) error {
return a.sendMagicLink(r, tx, user, flowType)
})
if err != nil {
if errors.Is(err, MaxFrequencyLimitError) {
return tooManyRequestsError(ErrorCodeOverEmailSendRateLimit, generateFrequencyLimitErrorMessage(user.RecoverySentAt, config.SMTP.MaxFrequency))
}
return internalServerError("Error sending magic link").WithInternalError(err)
return err
}

return sendJSON(w, http.StatusOK, make(map[string]string))
Expand Down
Loading

0 comments on commit bf276ab

Please sign in to comment.