Email rate limit is triggered even in scenarios where an email doesn't end up being sent

[makeusabrew]

Bug report

• I confirm this is a bug with Supabase, not with my own application.
• I confirm I have searched the Docs, GitHub Discussions, and Discord.

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:

AuthApiError: Password should be at least 6 characters
AuthApiError: Password should be at least 6 characters
AuthApiError: Password should be at least 6 characters
AuthApiError: Password should be at least 6 characters
AuthApiError: Email rate limit exceeded

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

• OS: macOS, but don't think it's relevant
• Browser (if applies): N/A
• Version of supabase-js:
  "@supabase/auth-helpers-nextjs": "^0.7.3"
  "@supabase/supabase-js": "^2.31.0"
• Version of Node.js: 20.5.1

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.

[LautaroJayat]

@makeusabrew Indeed.

It seems the Signup API is using the the so called sharedLimiter, which is a middleware that wraps a third party tool named tollboth.
This middleware calls tollbooth.LimitByKeys to check if the limit was exceeded or not.
If the limit was exceeded, it returns an early response. If not, it delegates the control to the next handler in the chain.

By design, at least /invite, /signup, /recover, /resend /magiclink and /otp share the same limiting strategy. So this same problem might happen in all of those endpoints.

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.

[hf]

We'll get around to this eventually. Please feel free to submit a PR instead!

[MarcusTXK]

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:

• Limiter Initialization : Created separate limiters for email and phone with specified rates and bursts.
• Pre-Processing Checks : Before processing the request, checked if the rate limit was exceeded using emailLimiter.LimitReached and phoneLimiter.LimitReached. If exceeded, return http.StatusTooManyRequests to the user, otherwise process the request.
• Post-Processing Increment : After the request was processed, attempted to increment the rate limit counters using tollbooth.LimitByKeys only if the HTTP response status was OK.

Issue Encountered: Double Counting

My 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:

• Initial Rate Limit Check : When checking if the rate limit is exceeded (currently I did so using LimitReached), Tollbooth internally increments the counter as a side effect. This means that merely checking the rate limit also counts as a request.
• Post-Processing Increment : If the request is successful, I increment the counter again using LimitByKeys. This results in the same request being counted twice.

Potential Solutions

I am considering several alternatives to address this issue:

1. Design Change : Directly increment the limiter in the handlers before sending the email. This could involve:

   • Context Passing : Sending the increment function via context (though this is unconventional in Go), set in the middleware.
   • Global Access : Allowing global access to the limiter, which simplifies access but might impact testability and code separation.

2. Alternative Libraries : Evaluate other rate limiting libraries that allow for a check without increment. I feel this should be a last resort as it will be a big change for a non-pressing bug.

Request for Input

Any 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.

[hf]

Please open a PR, it's very difficult for us to review files without seeing what is changing and provide meaningful reviews.