ExHammer / hammer

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

Fix: Backend config initializer to_existing_atom calls fixed #87

Closed njwest closed 7 months ago

njwest commented 7 months ago

Fixes #85 , tests passing locally.

The bug comes from this code in the supervisor:

  # Multiple backends
  def init(config) when is_list(config) do
    children =
      Enum.map(config, fn {k, c} ->
        "hammer_backend_#{k}_pool"
        |> String.to_existing_atom()
        |> to_pool_spec(c)
      end)

    Supervisor.init(children, strategy: :one_for_one)
  end

String.to_existing_atom/1 is trying to create atoms that do not exist at runtime. Replacing this with String.to_atom/1 does the trick.

String.to_atom/1 is naughty in cases in which dynamic user input is being atomized or when a number of values being atomized are dynamic or large for some other reason because we have a finite number of atoms available to us on the BEAM, but this is neither of those cases.

Because the atom being created in this code is from user configuration, String.to_existing_atom/1 isn't necessary here because it is highly unlikely someone will DDoS their atom memory from their own user configuration of Hammer, as that would require passing a list of hundreds of thousands to millions of backends to their Hammer configuration, an unlikely case that indicates a bigger problem outside of the code.

An alternative solution to this would be instantiating a list of possible atoms prior to this and using them in a way in which they don't get swept away, which would mean updating the list of usable backends in Hammer's source whenever someone develops a new one, a rather limiting condition.

njwest commented 7 months ago

this now also fixes #88

epinault commented 7 months ago

So the tests are failing. We will need to disable that rule