-
Notifications
You must be signed in to change notification settings - Fork 22
Fix bug in access attempts. Add burst and daily rate limits #4794
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces dual-tier rate limiting for access attempts by implementing separate burst and daily limits alongside bug fixes for error handling. The changes enhance security by providing both short-term protection against rapid attacks and long-term protection against sustained abuse.
Key changes:
- Adds burst rate limits (5-minute windows) and daily rate limits (24-hour windows) for both user and IP-based access attempts
- Updates error handling to distinguish between
too_many_burst_attemptsandtoo_many_daily_attempts - Enhances email login flow to check IP limits before user creation, preventing user pollution from blocked IPs
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
lib/sanbase/accounts/access_attempt.ex |
Core implementation of dual-tier rate limiting with separate burst and daily limit checking |
lib/sanbase/accounts/email_login_attempt.ex |
Updated configuration with burst/daily limits and added IP-only checking function |
lib/sanbase/accounts/coupon_attempt.ex |
Updated configuration with burst/daily limits and added IP-only checking function |
lib/sanbase/accounts/access_attempt_behaviour.ex |
Updated callback signature to support multiple error types |
lib/sanbase_web/graphql/resolvers/user/auth_resolver.ex |
Enhanced email login flow with IP pre-validation and specific error handling |
lib/sanbase_web/graphql/resolvers/billing_resolver.ex |
Updated error handling for new rate limit error types |
test/sanbase/accounts/access_attempt_test.exs |
Comprehensive test coverage for both burst and daily rate limiting scenarios |
test/sanbase_web/graphql/auth/email_login_api_test.exs |
Updated tests to use dynamic configuration and added IP pollution prevention tests |
test/sanbase_web/graphql/billing/subscribe_api_test.exs |
Updated mock to return new burst limit error type |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| {:error, message: "Too many login attempts, try again tomorrow"} | ||
|
|
||
| {:error, :too_many_attempts} -> |
Copilot
AI
Aug 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The legacy error handler for :too_many_attempts should be removed since the new implementation uses specific error types (:too_many_burst_attempts and :too_many_daily_attempts). This prevents confusion and ensures consistent error handling.
| for i <- 1..(config.allowed_ip_daily_attempts + 1) do | ||
| existing_user = insert(:user, email: "dailyuser#{i}@example.com") | ||
|
|
||
| {:ok, attempt} = | ||
| Sanbase.Accounts.AccessAttempt.create("email_login", existing_user, test_ip) | ||
|
|
||
| from(a in Sanbase.Accounts.AccessAttempt, where: a.id == ^attempt.id) | ||
| |> Sanbase.Repo.update_all(set: [inserted_at: past_time]) | ||
| end | ||
|
|
Copilot
AI
Aug 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test is performing individual database updates for each access attempt record. Consider using a single update_all query with a list of IDs to improve test performance and reduce database load.
| for i <- 1..(config.allowed_ip_daily_attempts + 1) do | |
| existing_user = insert(:user, email: "dailyuser#{i}@example.com") | |
| {:ok, attempt} = | |
| Sanbase.Accounts.AccessAttempt.create("email_login", existing_user, test_ip) | |
| from(a in Sanbase.Accounts.AccessAttempt, where: a.id == ^attempt.id) | |
| |> Sanbase.Repo.update_all(set: [inserted_at: past_time]) | |
| end | |
| attempt_ids = | |
| for i <- 1..(config.allowed_ip_daily_attempts + 1) do | |
| existing_user = insert(:user, email: "dailyuser#{i}@example.com") | |
| {:ok, attempt} = | |
| Sanbase.Accounts.AccessAttempt.create("email_login", existing_user, test_ip) | |
| attempt.id | |
| end | |
| from(a in Sanbase.Accounts.AccessAttempt, where: a.id in ^attempt_ids) | |
| |> Sanbase.Repo.update_all(set: [inserted_at: past_time]) |
| for _i <- 1..(config.allowed_user_daily_attempts + 1) do | ||
| {:ok, attempt} = AccessAttempt.create(type, user, ip) | ||
|
|
||
| Repo.update_all(from(a in AccessAttempt, where: a.id == ^attempt.id), | ||
| set: [inserted_at: past_time] | ||
| ) | ||
| end |
Copilot
AI
Aug 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the email login tests, individual database updates in loops create unnecessary overhead. Consider collecting all attempt IDs and performing a single batch update operation.
| for _i <- 1..(config.allowed_user_daily_attempts + 1) do | |
| {:ok, attempt} = AccessAttempt.create(type, user, ip) | |
| Repo.update_all(from(a in AccessAttempt, where: a.id == ^attempt.id), | |
| set: [inserted_at: past_time] | |
| ) | |
| end | |
| attempt_ids = | |
| for _i <- 1..(config.allowed_user_daily_attempts + 1), into: [] do | |
| {:ok, attempt} = AccessAttempt.create(type, user, ip) | |
| attempt.id | |
| end | |
| Repo.update_all(from(a in AccessAttempt, where: a.id in ^attempt_ids), | |
| set: [inserted_at: past_time] | |
| ) |
| # Check burst limits (short-term) | ||
| too_many_ip_burst? = | ||
| attempts_count(type, remote_ip, :burst) > config.allowed_ip_burst_attempts | ||
|
|
||
| # Check daily limits (long-term) | ||
| too_many_ip_daily? = | ||
| attempts_count(type, remote_ip, :daily) > config.allowed_ip_daily_attempts | ||
|
|
||
| cond do | ||
| too_many_ip_burst? -> | ||
| {:error, :too_many_burst_attempts} | ||
|
|
||
| too_many_ip_daily? -> | ||
| {:error, :too_many_daily_attempts} | ||
|
|
||
| true -> | ||
| :ok |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can call the functions from within cond:
cond do
attempts_count(type, remote_ip, :burst) > config.allowed_ip_burst_attempts ->
{:error, :too_many_burst_attempts}
...
This way it will stop computing the rest if it finds an offence.
Alternatively, we could also probably compute all of the counts in one SQL query to avoid multiple DB calls. It would be a fragment with COUNT(CASE ... THEN 1 ELSE 0 END) like here: https://github.com/santiment/sanbase2/blob/master/lib/sanbase/accounts/interaction/interaction.ex#L166-L172
| # Daily limits (long-term protection) | ||
| # 24 hours | ||
| daily_interval_in_minutes: 24 * 60, | ||
| allowed_user_daily_attempts: 200, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the use case where a user would need 200 login emails per day? That's like one login every 8 minutes.
I think we can push this down to 10?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, this is the coupon limit.. Isn't this also very, very high?
Changes
Ticket
Checklist: