-
Notifications
You must be signed in to change notification settings - Fork 351
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
Email rate limit is triggered even in scenarios where an email doesn't end up being sent #1236
Comments
@makeusabrew Indeed. It seems the Signup API is using the the so called By design, at least I think it would be interesting if we could move the limiter from a before-middleware to an after-middleware and trigger the rate limiter depending on return value of the API handler, as it returns an error if something went wrong. This implies a change of the design, but it would be cool. |
We'll get around to this eventually. Please feel free to submit a PR instead! |
In response to the suggestion by @LautaroJayat to move the rate limiter from a before-middleware to an after-middleware, I tried implementing changes in the rate limiting middleware as I feel it is a great suggestion. My goal was to increment the rate limit only if the API handler executed successfully (HTTP status OK). Updated Middleware Flow in middleware.go on my branch:
Issue Encountered: Double CountingMy updated implementation works, but leads to a double counting issue due to the way Tollbooth works. As far as I can tell after diving into the implementation of Tollbooth, there is no way to check if the rate limit has exceeded without incrementing the count as a side effect:
Potential SolutionsI am considering several alternatives to address this issue:
Request for InputAny insights, suggestions, or recommendations regarding this issue would be highly appreciated, especially if I overlooked a way on Tollbooth to check if the rate limit has exceeded without incrementing the count. |
Please open a PR, it's very difficult for us to review files without seeing what is changing and provide meaningful reviews. |
Any updates? Can't develop locally without using a custom SMTP without this, sadly :( |
## 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.
@brunobely we just merged in a fix for this #1748 which should be rolled out soon |
Bug report
Describe the bug
Failed signups still count towards the email rate limit even though no user record is created and no email ends up being sent, leading to unwarranted
AuthApiError: Email rate limit exceeded
errors. Misuse of the registration process (by well-intentioned would-be users or deliberate bad actors) can very quickly lead to a denial of service whereby nobody can sign up for an account, even if no emails have been sent.I've tried my best to report this in the right repository tracing through the calls I think the
/signup
flow makes starting with the Supabase Auth TypeScript helpers at the top of the stack. Apologies if this isn't the right place after all.To Reproduce
Steps to reproduce the behavior, please provide code snippets or a repository:
Not the best reproduction steps here, sorry. In effect, using the built-in SMTP testing rate limits just by way of example, calling something like
await supabase.auth.signUp({email: "a@b.com", password: "x"})
5 times in an hour with default minimum password length restrictions in place will result in error responses as follows:Of course, users are encouraged not to use the test SMTP endpoint - I'm only doing so as an example here. Even if users provide their own, they're encouraged to set sensible rate limits to mitigate abuse. The point is that no emails are actually being sent here.
Expected behavior
In the scenario above I would not expect to ever hit the "email rate limit exceeded" error, since no emails had been sent and no new users had been added to the database.
Screenshots
I don't think this helps much as you can't see the full implementation, but here's the tail of some logs of me deliberately entering a short password until hitting the rate limit (I'd previously been testing while writing this bug, hence why I only managed two "password should be..." errors before hitting the limit again).
System information
"@supabase/auth-helpers-nextjs": "^0.7.3"
"@supabase/supabase-js": "^2.31.0"
Additional context
I don't know Go at all, but a quick glance at
internal/api/api.go
around line 107/108 within this repo made me wonder if the rate limiter middleware is always invoked before the actual call, regardless of whether that call does the thing it's expected to or not.The text was updated successfully, but these errors were encountered: