ExHammer / hammer

An Elixir rate-limiter with pluggable backends
https://hexdocs.pm/hammer/
MIT License
747 stars 42 forks source link

Fix bug that allows check_rate to succeed 2x in sequence even if the bucket limit is 1 attempt per hour #99

Open rschef opened 5 months ago

rschef commented 5 months ago

Problem

When check_rate is called for the first time, it's expected that ms_to_next_bucket would be qual to scale_ms, but that's not the case at the moment due to:

  1. Utils.stamp_key/2 is currently creating a variable bucket key for the same ID and scale_ms. The bucket key changes every scale_ms milliseconds.
  2. Redis/ETS backends are not expiring the bucket key after scale_ms, instead they rely that a new key is provided (1)

This bug allows check_rate to be called 2x in sequence even if the bucket limit is 1 attempt per hour. Both calls could receive :allow if timed when the bucket is about to expire (see how to reproduce the bug below).

How to reproduce the bug

# Install hammer

Application.put_env(
  :hammer,
  :backend,
  {Hammer.Backend.ETS,
   [
     ets_table_name: :hammer_backend_ets_buckets,
     expiry_ms: :timer.hours(24),
     cleanup_interval_ms: :timer.hours(24)
   ]}
)

Mix.install([:hammer])

# Reproduce the bug

bucket_key = "bucket_key:#{:rand.uniform()}"
scale_ms = :timer.seconds(20)
max_attempts = 1

ms_about_to_expire = 100

wait_for_timing_fn = fn ->
  {:ok, {0, 1, ms_to_next_bucket, nil, nil}} =
    Hammer.inspect_bucket(bucket_key, scale_ms, max_attempts)

  if ms_to_next_bucket >= ms_about_to_expire do
    :timer.sleep(ms_to_next_bucket - ms_about_to_expire)
  end

  :ready
end

:ready = wait_for_timing_fn.()

{:allow, 1} = Hammer.check_rate(bucket_key, scale_ms, max_attempts)

:timer.sleep(ms_about_to_expire)

{:allow, 1} = Hammer.check_rate(bucket_key, scale_ms, max_attempts)

Proposed solution

This PR fixes this issue by:

  1. Returning a fixed key on Utils.stamp_key/2 for the same bucket ID and scale_ms
  2. Requiring the Redis/ETS backends to expire the bucket after scale_ms. I will open a PR on hammer-backend-redis if this PR is accepted: https://github.com/nash-io/hammer-backend-redis/pull/1