Merge pull request #570 from seancribbs/sdc/unsafe-atom-creation

Add check for unsafe conversions to atoms

lib/credo/check/warning/unsafe_to_atom.ex

@@ -0,0 +1,86 @@
+defmodule Credo.Check.Warning.UnsafeToAtom do
+  @moduledoc """
+  Creating atoms from unknown or external sources dynamically is a potentially
+  unsafe operation because atoms are not garbage-collected by the runtime.
+
+  Creating an atom from a string or charlist should be done by using
+
+      String.to_existing_atom(string)
+
+  or
+
+      List.to_existing_atom(charlist)
+
+  Module aliases should be constructed using
+
+      Module.safe_concat(prefix, suffix)
+
+  or
+
+      Module.safe_concat([prefix, infix, suffix])
+
+  """
+
+  @explanation [check: @moduledoc]
+
+  use Credo.Check, base_priority: :high, category: :warning
+
+  @doc false
+  def run(source_file, params \\ []) do
+    issue_meta = IssueMeta.for(source_file, params)
+
+    Credo.Code.prewalk(source_file, &traverse(&1, &2, issue_meta))
+  end
+
+  defp traverse({{:., _loc, call}, meta, args} = ast, issues, issue_meta) do
+    case get_forbidden_call(call, args) do
+      {bad, suggestion} ->
+        {ast, issues_for_call(bad, suggestion, meta, issue_meta, issues)}
+
+      nil ->
+        {ast, issues}
+    end
+  end
+
+  defp traverse(ast, issues, _issue_meta) do
+    {ast, issues}
+  end
+
+  defp get_forbidden_call([:erlang, :list_to_atom], [_]) do
+    {":erlang.list_to_atom/1", ":erlang.list_to_existing_atom/1"}
+  end
+
+  defp get_forbidden_call([:erlang, :binary_to_atom], [_, _]) do
+    {":erlang.binary_to_atom/2", ":erlang.binary_to_existing_atom/2"}
+  end
+
+  defp get_forbidden_call([{:__aliases__, _, [:String]}, :to_atom], [_]) do
+    {"String.to_atom/1", "String.to_existing_atom/1"}
+  end
+
+  defp get_forbidden_call([{:__aliases__, _, [:List]}, :to_atom], [_]) do
+    {"List.to_atom/1", "List.to_existing_atom/1"}
+  end
+
+  defp get_forbidden_call([{:__aliases__, _, [:Module]}, :concat], [_]) do
+    {"Module.concat/1", "Module.safe_concat/1"}
+  end
+
+  defp get_forbidden_call([{:__aliases__, _, [:Module]}, :concat], [_, _]) do
+    {"Module.concat/2", "Module.safe_concat/2"}
+  end
+
+  defp get_forbidden_call(_, _) do
+    nil
+  end
+
+  defp issues_for_call(call, suggestion, meta, issue_meta, issues) do
+    options = [
+      message: "Prefer #{suggestion} over #{call} to avoid creating atoms at runtime",
+      trigger: call,
+      line_no: meta[:line]
+    ]
+
+    [format_issue(issue_meta, options) | issues]
+  end
+end

test/credo/check/warning/unsafe_to_atom_test.exs

@@ -0,0 +1,117 @@
+defmodule Credo.Check.Warning.UnsafeToAtomTest do
+  use Credo.TestHelper
+
+  @described_check Credo.Check.Warning.UnsafeToAtom
+
+  #
+  # cases NOT raising issues
+  #
+
+  test "it should NOT report expected code" do
+    """
+    defmodule CredoSampleModule do
+      def convert_module(parameter) do
+        Module.safe_concat(__MODULE__, parameter)
+      end
+
+      def convert_module_2(parameter1, parameter2) do
+        Module.safe_concat([__MODULE__, parameter1, parameter2])
+      end
+
+      def convert_atom(parameter) do
+        String.to_existing_atom(parameter)
+      end
+
+      def convert_atom_2(parameter) do
+        List.to_existing_atom(parameter)
+      end
+
+      def convert_erlang_list(parameter) do
+        :erlang.list_to_existing_atom(parameter)
+      end
+
+      def convert_erlang_binary(parameter) do
+        :erlang.binary_to_existing_atom(parameter, :utf8)
+      end
+    end
+    """
+    |> to_source_file()
+    |> refute_issues(@described_check)
+  end
+
+  #
+  # cases raising issues
+  #
+
+  test "it should report a violation" do
+    """
+    defmodule CredoSampleModule do
+      def some_function(parameter) do
+        String.to_atom(parameter)
+      end
+    end
+    """
+    |> to_source_file()
+    |> assert_issue(@described_check)
+  end
+
+  test "it should report a violation /2" do
+    """
+    defmodule CredoSampleModule do
+      def some_function(parameter) do
+        List.to_atom(parameter)
+      end
+    end
+    """
+    |> to_source_file()
+    |> assert_issue(@described_check)
+  end
+
+  test "it should report a violation /3" do
+    """
+    defmodule CredoSampleModule do
+      def some_function(parameter) do
+        Module.concat(__MODULE__, parameter)
+      end
+    end
+    """
+    |> to_source_file()
+    |> assert_issue(@described_check)
+  end
+
+  test "it should report a violation /4" do
+    """
+    defmodule CredoSampleModule do
+      def some_function(parameter1, parameter2) do
+        Module.concat([__MODULE__, parameter1, parameter2])
+      end
+    end
+    """
+    |> to_source_file()
+    |> assert_issue(@described_check)
+  end
+
+  test "it should report a violation /5" do
+    """
+    defmodule CredoSampleModule do
+      def some_function(parameter) do
+        :erlang.list_to_atom(parameter)
+      end
+    end
+    """
+    |> to_source_file()
+    |> assert_issue(@described_check)
+  end
+
+  test "it should report a violation /6" do
+    """
+    defmodule CredoSampleModule do
+      def some_function(parameter) do
+        :erlang.binary_to_atom(parameter, :utf8)
+      end
+    end
+    """
+    |> to_source_file()
+    |> assert_issue(@described_check)
+  end
+end