Repository: rrrene/credo
Pull request #730 (2019-12-20 21:30:43)

Improve UnsafeToAtom to recognize Jason.decode using keys: :atoms
## What

This PR improves the UnsafeToAtom check to recognize and warn about the use of `keys: :atoms` in `Jason.decode` and `Jason.decode!`.

## Why

Jason.decode and Jason.decode! take a `:keys` opt which allows you to choose what type of keys your maps are decoded with. Passing `keys: :atoms` will use the unsafe `String.to_atom` under the hood. Passing `keys: :atoms!` will use the safe `String.to_existing_atom`.


Patch:
diff --git a/lib/credo/check/warning/unsafe_to_atom.ex b/lib/credo/check/warning/unsafe_to_atom.ex
index 537d6fab..6df3ca11 100644
--- a/lib/credo/check/warning/unsafe_to_atom.ex
+++ b/lib/credo/check/warning/unsafe_to_atom.ex
@@ -19,6 +19,14 @@ defmodule Credo.Check.Warning.UnsafeToAtom do
 
       Module.safe_concat([prefix, infix, suffix])
 
+  Jason.decode/Jason.decode! should be called using `keys: :atoms!` (*not* `keys: :atoms`):
+
+      Jason.decode(str, keys: :atoms!)
+
+  or `:keys` should be omitted (which defaults to `:strings`):
+
+      Jason.decode(str)
+
   """
 
   @explanation [check: @moduledoc]
@@ -70,6 +78,16 @@ defmodule Credo.Check.Warning.UnsafeToAtom do
     {"Module.concat/2", "Module.safe_concat/2"}
   end
 
+  defp get_forbidden_call([{:__aliases__, _, [:Jason]}, decode], args) when decode in [:decode, :decode!] do
+    args
+    |> Enum.any?(fn arg -> Keyword.keyword?(arg) and Keyword.get(arg, :keys) == :atoms end)
+    |> if do
+      {"Jason.#{decode}(..., keys: :atoms)", "Jason.#{decode}(..., keys: :atoms!)"}
+    else
+      nil
+    end
+  end
+
   defp get_forbidden_call(_, _) do
     nil
   end


Test patch:
diff --git a/test/credo/check/warning/unsafe_to_atom_test.exs b/test/credo/check/warning/unsafe_to_atom_test.exs
index 9d42f1de..439050ca 100644
--- a/test/credo/check/warning/unsafe_to_atom_test.exs
+++ b/test/credo/check/warning/unsafe_to_atom_test.exs
@@ -114,4 +114,153 @@ defmodule Credo.Check.Warning.UnsafeToAtomTest do
     |> to_source_file()
     |> assert_issue(@described_check)
   end
+
+  describe "Jason decode/decode!" do
+    # Keys unspecified
+    test "it should not report a violation on Jason.decode without keys with a pipeline" do
+      """
+      defmodule CredoSampleModule do
+        def some_function(parameter) do
+          parameter |> Jason.decode()
+        end
+      end
+      """
+      |> to_source_file()
+      |> refute_issues(@described_check)
+    end
+
+    test "it should not report a violation on Jason.decode without keys without a pipeline" do
+      """
+      defmodule CredoSampleModule do
+        def some_function(parameter) do
+          Jason.decode(parameter)
+        end
+      end
+      """
+      |> to_source_file()
+      |> refute_issues(@described_check)
+    end
+
+    test "it should not report a violation on Jason.decode! without keys with a pipeline" do
+      """
+      defmodule CredoSampleModule do
+        def some_function(parameter) do
+          parameter |> Jason.decode!()
+        end
+      end
+      """
+      |> to_source_file()
+      |> refute_issues(@described_check)
+    end
+
+    test "it should not report a violation on Jason.decode! without keys without a pipeline" do
+      """
+      defmodule CredoSampleModule do
+        def some_function(parameter) do
+          Jason.decode!(parameter)
+        end
+      end
+      """
+      |> to_source_file()
+      |> refute_issues(@described_check)
+    end
+
+    # keys: :atoms! (safe)
+    test "it should not report a violation on Jason.decode with keys: :atoms! with a pipeline" do
+      """
+      defmodule CredoSampleModule do
+        def some_function(parameter) do
+          parameter |> Jason.decode(keys: :atoms!)
+        end
+      end
+      """
+      |> to_source_file()
+      |> refute_issues(@described_check)
+    end
+
+    test "it should not report a violation on Jason.decode with keys: :atoms! without a pipeline" do
+      """
+      defmodule CredoSampleModule do
+        def some_function(parameter) do
+          Jason.decode(parameter, keys: :atoms!)
+        end
+      end
+      """
+      |> to_source_file()
+      |> refute_issues(@described_check)
+    end
+
+    test "it should not report a violation on Jason.decode! with keys: :atoms! with a pipeline" do
+      """
+      defmodule CredoSampleModule do
+        def some_function(parameter) do
+          parameter |> Jason.decode!(keys: :atoms!)
+        end
+      end
+      """
+      |> to_source_file()
+      |> refute_issues(@described_check)
+    end
+
+    test "it should not report a violation on Jason.decode! with keys: :atoms! without a pipeline" do
+      """
+      defmodule CredoSampleModule do
+        def some_function(parameter) do
+          Jason.decode!(parameter, keys: :atoms!)
+        end
+      end
+      """
+      |> to_source_file()
+      |> refute_issues(@described_check)
+    end
+
+    # keys: :atoms (unsafe)
+    test "it should report a violation on Jason.decode with keys: :atoms!with a pipeline" do
+      """
+      defmodule CredoSampleModule do
+        def some_function(parameter) do
+          parameter |> Jason.decode(keys: :atoms)
+        end
+      end
+      """
+      |> to_source_file()
+      |> assert_issue(@described_check)
+    end
+
+    test "it should report a violation on Jason.decode with keys: :atoms without a pipeline" do
+      """
+      defmodule CredoSampleModule do
+        def some_function(parameter) do
+          Jason.decode(parameter, keys: :atoms)
+        end
+      end
+      """
+      |> to_source_file()
+      |> assert_issue(@described_check)
+    end
+
+    test "it should report a violation on Jason.decode! with keys: :atoms with a pipeline" do
+      """
+      defmodule CredoSampleModule do
+        def some_function(parameter) do
+          parameter |> Jason.decode!(keys: :atoms)
+        end
+      end
+      """
+      |> to_source_file()
+      |> assert_issue(@described_check)
+    end
+
+    test "it should report a violation on Jason.decode! with keys: :atoms without a pipeline" do
+      """
+      defmodule CredoSampleModule do
+        def some_function(parameter) do
+          Jason.decode!(parameter, keys: :atoms)
+        end
+      end
+      """
+      |> to_source_file()
+      |> assert_issue(@described_check)
+    end
+  end
 end