credo icon indicating copy to clipboard operation
credo copied to clipboard

`Credo.Check.Warning.UnsafeToAtom` warning on compile time created atom

Open Wigny opened this issue 1 year ago • 1 comments

Environment

  • Credo version (mix credo -v): 1.7.7-ref.dependabot-hex-gettext-0.26.1.58e9c1b08
  • Erlang/Elixir version (elixir -v): Elixir 1.17.2 (compiled with Erlang/OTP 26)
  • Operating system: Ubuntu 22.04.3 LTS

What were you trying to do?

Running the Credo.Check.Warning.UnsafeToAtom check (mix credo -c Credo.Check.Warning.UnsafeToAtom) is reporting right now issues for dynamically created atoms passed to unquote().

Expected outcome

AFAIK dynamically created atoms passed to unquote() are generated in compile time and thus should not be reported by this check, given they cannot be exploited in runtime. Thus the following code should not emit the check warning:

defmodule Test do
  for n <- 1..4 do
    def unquote(:"fun_#{n}")(), do: unquote(n)
  end
end

Actual outcome

Running the check on the code above returns

Prefer :erlang.binary_to_existing_atom/2 over :erlang.binary_to_atom/2 to avoid creating atoms at runtime.

Wigny avatar Aug 26 '24 19:08 Wigny

Rewriting the code to the following avoids the warning, but I'm unsure if that is the right solution.

defmodule Test do
  for n <- 1..4 do
    def unquote(:erlang.binary_to_atom("fun_#{n}"))(), do: unquote(n)
  end
end

Wigny avatar Aug 26 '24 19:08 Wigny

@Wigny Thanks for reporting this 😀 Sorry for the delay. It is now fixed on master.

You can try this by setting the Credo dep to

{:credo, github: "rrrene/credo"}

Please report back if your issue is solved! 👍

rrrene avatar Sep 22 '24 09:09 rrrene

Yes, it worked great, thanks!

The only case the change didn't cover in our codebase was

unquote(context).unquote(:"get_#{type}_by")(id: id)

which I believe could have been written as this instead

apply(unquote(context), unquote(:"get_#{type}_by"), [[id: id]])

So it is all good IMO. Many thanks for solving it!

Wigny avatar Sep 23 '24 13:09 Wigny

@Wigny Patched the check to cover that case as well. Thx for the input 👍

rrrene avatar Sep 23 '24 16:09 rrrene

@Wigny This is part of Credo 1.7.8. Thx! 🎉

rrrene avatar Sep 26 '24 17:09 rrrene