Skip to content

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.

Sign up for GitHub

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

Open
tspenov wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix bug in access attempts. Add burst and daily rate limits #4794

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 53 additions & 10 deletions lib/sanbase/accounts/access_attempt.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,51 @@ defmodule Sanbase.Accounts.AccessAttempt do

def check_attempt_limit(type, user, remote_ip) do
config = get_config(type)
too_many_user_attempts? = attempts_count(type, user) > config.allowed_user_attempts
too_many_ip_attempts? = attempts_count(type, remote_ip) > config.allowed_ip_attempts

if too_many_user_attempts? or too_many_ip_attempts? do
{:error, :too_many_attempts}
else
:ok
# Check burst limits (short-term)
too_many_user_burst? = attempts_count(type, user, :burst) > config.allowed_user_burst_attempts

too_many_ip_burst? =
attempts_count(type, remote_ip, :burst) > config.allowed_ip_burst_attempts

# Check daily limits (long-term)
too_many_user_daily? = attempts_count(type, user, :daily) > config.allowed_user_daily_attempts

too_many_ip_daily? =
attempts_count(type, remote_ip, :daily) > config.allowed_ip_daily_attempts

cond do
too_many_user_burst? or too_many_ip_burst? ->
{:error, :too_many_burst_attempts}

too_many_user_daily? or too_many_ip_daily? ->
{:error, :too_many_daily_attempts}

true ->
:ok
end
end

def check_ip_attempt_limit(type, remote_ip) do
config = get_config(type)

# 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
Comment on lines +44 to +60

@IvanIvanoff IvanIvanoff Aug 26, 2025

Copy link
Copy Markdown
Member

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

end
end

Expand All @@ -44,9 +82,10 @@ defmodule Sanbase.Accounts.AccessAttempt do
|> foreign_key_constraint(:user_id)
end

defp attempts_count(type, remote_ip) when is_binary(remote_ip) do
defp attempts_count(type, remote_ip, limit_type) when is_binary(remote_ip) do
config = get_config(type)
interval_limit = Timex.shift(Timex.now(), minutes: -config.interval_in_minutes)
interval_minutes = get_interval_minutes(config, limit_type)
interval_limit = Timex.shift(Timex.now(), minutes: -interval_minutes)

from(attempt in __MODULE__,
where:
Expand All @@ -57,9 +96,10 @@ defmodule Sanbase.Accounts.AccessAttempt do
|> Repo.aggregate(:count, :id)
end

defp attempts_count(type, %{id: user_id}) do
defp attempts_count(type, %{id: user_id}, limit_type) do
config = get_config(type)
interval_limit = Timex.shift(Timex.now(), minutes: -config.interval_in_minutes)
interval_minutes = get_interval_minutes(config, limit_type)
interval_limit = Timex.shift(Timex.now(), minutes: -interval_minutes)

from(attempt in __MODULE__,
where:
Expand All @@ -70,6 +110,9 @@ defmodule Sanbase.Accounts.AccessAttempt do
|> Repo.aggregate(:count, :id)
end

defp get_interval_minutes(config, :burst), do: config.burst_interval_in_minutes
defp get_interval_minutes(config, :daily), do: config.daily_interval_in_minutes

defp get_config(type) do
case type do
"email_login" -> Sanbase.Accounts.EmailLoginAttempt.config()
Expand Down
2 changes: 1 addition & 1 deletion lib/sanbase/accounts/access_attempt_behaviour.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ defmodule Sanbase.Accounts.AccessAttemptBehaviour do
@callback type() :: String.t()

@callback check_attempt_limit(user :: term(), remote_ip :: String.t()) ::
:ok | {:error, :too_many_attempts}
:ok | {:error, atom()}

@callback create(user :: term(), remote_ip :: String.t()) ::
{:ok, term()} | {:error, term()}
Expand Down
17 changes: 14 additions & 3 deletions lib/sanbase/accounts/coupon_attempt.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,16 @@ defmodule Sanbase.Accounts.CouponAttempt do
@impl true
def config do
%{
interval_in_minutes: 10,
allowed_user_attempts: 30,
allowed_ip_attempts: 60
# Burst limits (short-term protection)
burst_interval_in_minutes: 5,
allowed_user_burst_attempts: 30,
allowed_ip_burst_attempts: 60,

# Daily limits (long-term protection)
# 24 hours
daily_interval_in_minutes: 24 * 60,
allowed_user_daily_attempts: 200,

@IvanIvanoff IvanIvanoff Aug 26, 2025

Copy link
Copy Markdown
Member

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?

@IvanIvanoff IvanIvanoff Aug 26, 2025

Copy link
Copy Markdown
Member

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?

allowed_ip_daily_attempts: 500
}
end

Expand All @@ -20,6 +27,10 @@ defmodule Sanbase.Accounts.CouponAttempt do
AccessAttempt.check_attempt_limit(type(), user, remote_ip)
end

def check_ip_attempt_limit(remote_ip) do
AccessAttempt.check_ip_attempt_limit(type(), remote_ip)
end

@impl true
def create(user, remote_ip) do
AccessAttempt.create(type(), user, remote_ip)
Expand Down
17 changes: 14 additions & 3 deletions lib/sanbase/accounts/email_login_attempt.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,16 @@ defmodule Sanbase.Accounts.EmailLoginAttempt do
@impl true
def config do
%{
interval_in_minutes: 5,
allowed_user_attempts: 5,
allowed_ip_attempts: 20
# Burst limits (short-term protection)
burst_interval_in_minutes: 5,
allowed_user_burst_attempts: 5,
allowed_ip_burst_attempts: 10,

# Daily limits (long-term protection)
# 24 hours
daily_interval_in_minutes: 24 * 60,
allowed_user_daily_attempts: 20,
allowed_ip_daily_attempts: 100
}
end

Expand All @@ -19,6 +26,10 @@ defmodule Sanbase.Accounts.EmailLoginAttempt do
AccessAttempt.check_attempt_limit(type(), user, remote_ip)
end

def check_ip_attempt_limit(remote_ip) do
AccessAttempt.check_ip_attempt_limit(type(), remote_ip)
end

@impl true
def create(user, remote_ip) do
AccessAttempt.create(type(), user, remote_ip)
Expand Down
5 changes: 4 additions & 1 deletion lib/sanbase_web/graphql/resolvers/billing_resolver.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,9 +197,12 @@ defmodule SanbaseWeb.Graphql.Resolvers.BillingResolver do
amount_off: amount_off
}}
else
{:error, :too_many_attempts} ->
{:error, :too_many_burst_attempts} ->
{:error, "Too many coupon attempts. Please try again later."}

{:error, :too_many_daily_attempts} ->
{:error, "Too many coupon attempts. Please try again tomorrow"}

{:error, %Stripe.Error{message: message} = reason} ->
log_error("Error checking coupon", reason)
{:error, message}
Expand Down
17 changes: 17 additions & 0 deletions lib/sanbase_web/graphql/resolvers/user/auth_resolver.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,11 @@ defmodule SanbaseWeb.Graphql.Resolvers.AuthResolver do

with true <- allowed_email_domain?(email),
true <- allowed_origin?(origin_host_parts, origin_url),
# Check IP limits before creating/finding user
:ok <- EmailLoginAttempt.check_ip_attempt_limit(remote_ip),
{:ok, %{first_login: first_login} = user} <-
User.find_or_insert_by(:email, email, %{username: args[:username]}),
# Check user-specific limits after user creation
:ok <- EmailLoginAttempt.check_attempt_limit(user, remote_ip),
{:ok, user} <- User.Email.update_email_token(user, args[:consent]),
{:ok, _res} <- User.Email.send_login_email(user, first_login, origin_host_parts, args),
Expand All @@ -106,6 +109,20 @@ defmodule SanbaseWeb.Graphql.Resolvers.AuthResolver do

{:error, message: message}

{:error, :too_many_burst_attempts} ->
Logger.info(
"Login failed: too many burst attempts. Email: #{email}, IP Address: #{remote_ip}, Origin URL: #{origin_url}"
)

{:error, message: "Too many login attempts, try again after a few minutes"}

{:error, :too_many_daily_attempts} ->
Logger.info(
"Login failed: too many daily attempts. Email: #{email}, IP Address: #{remote_ip}, Origin URL: #{origin_url}"
)

{:error, message: "Too many login attempts, try again tomorrow"}

{:error, :too_many_attempts} ->

Copilot AI Aug 26, 2025

Copy link

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.

Copilot uses AI. Check for mistakes.
Logger.info(
"Login failed: too many login attempts. Email: #{email}, IP Address: #{remote_ip}, Origin URL: #{origin_url}"
Expand Down
Loading