Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(config): add rate limit configuration for emails in toml #3206

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from

Conversation

avallete
Copy link
Member

@avallete avallete commented Feb 25, 2025

Allow to configure rate limits via the config.toml

Comes with the docs PR here: supabase/supabase#33830

@avallete avallete requested a review from a team as a code owner February 25, 2025 05:32
Copy link

linear bot commented Feb 25, 2025

@coveralls
Copy link

coveralls commented Feb 25, 2025

Pull Request Test Coverage Report for Build 13535289202

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 61 of 61 (100.0%) changed or added relevant lines in 2 files are covered.
  • 5 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.2%) to 58.408%

Files with Coverage Reduction New Missed Lines %
internal/gen/keys/keys.go 5 12.9%
Totals Coverage Status
Change from base Build 13513651492: 0.2%
Covered Lines: 7850
Relevant Lines: 13440

💛 - Coveralls

Comment on lines 130 to 138
# Rate limit for unauthenticated requests per minute
rate_limit_anonymous_users = 30
# Rate limit for token refresh requests per minute
rate_limit_token_refresh = 150
# Rate limit for OTP generation requests per minute
rate_limit_otp = 30
# Rate limit for verification requests per minute
rate_limit_verify = 30

Copy link
Contributor

@sweatybridge sweatybridge Feb 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's worth moving them to a new struct so it aligns with our dashboard. For eg.

[auth.rate_limits]
# Number of emails that can be sent per hour. Requires auth.email.smtp to be enabled.
emails_sent = 2
# Number of SMS messages that can be sent per hour. Requires auth.sms to be enabled.
sms_sent = 30
# Number of sessions that can be refreshed in a 5 minute interval per IP address.
token_refreshes = 150
# Number of OTP / Magic link verifications that can be made in a 5 minute interval per IP address.
token_verifications = 30
# Number of anonymous sign-ins that can be made per hour per IP address. Requires enable_anonymous_sign_ins = true.
anonymous_users = 30
# Number of sign up and sign-in requests that can be made in a 5 minute interval per IP address (excludes anonymous users).
sign_in_sign_ups = 30

Btw it's really confusing to me why rate_limit_otp maps to number of sign-in and sign-ups https://github.com/supabase/supabase/blob/master/apps/studio/components/interfaces/Auth/RateLimits/RateLimits.tsx#L417

Copy link
Member Author

@avallete avallete Feb 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regrouped it into the same struct, had to update a bunch of test snapshot because of it tough 😅

Comment on lines +132 to +134
# email_sent = 2
# Number of SMS messages that can be sent per hour. Requires auth.sms to be enabled.
# sms_sent = 30
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# email_sent = 2
# Number of SMS messages that can be sent per hour. Requires auth.sms to be enabled.
# sms_sent = 30
email_sent = 2
# Number of SMS messages that can be sent per hour. Requires auth.sms to be enabled.
sms_sent = 30

It's a bit annoying that we can't leave these uncommented for consistency. Can update api to support configuring values < shared quota?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how much of a side effect this could have looking at those lines on API side:

https://github.com/supabase/infrastructure/blob/43c6d1a97c73eda0c171990ecd4e4ede5bafd185/api/src/common/projects/project-gotrue-config.service.ts#L359-L366

I think it might break some other behaviors.

SignInSignUps *uint `toml:"sign_in_sign_ups"`
TokenVerifications *uint `toml:"token_verifications"`
EmailSent *uint `toml:"email_sent"`
SmsSent *uint `toml:"sms_sent"`
Copy link
Contributor

@sweatybridge sweatybridge Feb 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any objection to make these non-pointers? We are setting default values for these fields https://github.com/supabase/cli/pull/3206/files#diff-ec116054b1a1deee8fc9bfb3c95419fcdae3c4d5a2c0b7a25c7c55ac1cfb135dR130-R142 so only the commented out EmailSent and SmsSent are nil pointers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants