Deprecate decoding using atoms

README.md

@@ -17,15 +17,15 @@ Bridges to Graphql services are defined by `use`ing the `BridgeEx.Graphql` macro
 
 ```elixir
 defmodule MyApp.SomeServiceBridge do
-  use BridgeEx.Graphql, endpoint: "http://some_service.example.com"
+  use BridgeEx.Graphql, endpoint: "http://some_service.example.com", decode_keys: :strings
 
   def my_query(%{} = variables) do
     call("a graphql query or mutation", variables, retry_policy: [max_retries: 1])
   end
 end
 ```
 
-Besides `endpoint`, the following parameters can be optionally set when `use`ing `BridgeEx.Graphql`:
+Besides `endpoint` and `decode_keys`, the following parameters can be optionally set when `use`ing `BridgeEx.Graphql`:
 
 - `auth0`
 - `encode_variables`
@@ -36,6 +36,8 @@ Besides `endpoint`, the following parameters can be optionally set when `use`ing
 - `log_options`
 - `max_attempts` `⚠ Deprecated in favour of retry_options in call method`
 
+The option `decode_keys` determines how JSON keys are decoded. By default it is set to `:atoms` which is the previous behavior and is deprecated since it may raise security concerns. See ["Decoding keys to atoms" in Jason documentation](https://hexdocs.pm/jason/Jason.html#decode/2-decoding-keys-to-atoms) for more information. Other options include `:strings` and `:existing_atoms` which are safer. In the future, `:strings` keys decoder will be used by default.
+
 Refer to [the documentation](https://hexdocs.pm/bridge_ex/BridgeEx.Graphql.html) for more details.
 
 If you need more control on your requests you can use [`BridgeEx.Graphql.Client.call`](https://hexdocs.pm/bridge_ex/BridgeEx.Graphql.Client.html#call/7) directly.
@@ -44,7 +46,7 @@ The library supports preloading queries from external files via the `BridgeEx.Ex
 
 ```elixir
 defmodule MyApp.SomeServiceBridge do
-  use BridgeEx.Graphql, endpoint: "http://some_service.example.com"
+  use BridgeEx.Graphql, endpoint: "http://some_service.example.com", decode_keys: :strings
   use BridgeEx.Extensions.ExternalResources, resources: [my_query: "my_query.graphql"]
 
   def my_query(%{} = variables), do: call(my_query(), variables)
@@ -94,7 +96,7 @@ end
 
 defmodule BridgeWithCustomRetry do
   use BridgeEx.Graphql,
-    endpoint: "http://some_service.example.com/graphql"
+    endpoint: "http://some_service.example.com/graphql", decode_keys: :strings
 end
 
 BridgeWithCustomRetry.call("myquery", %{}, retry_options: [policy: retry_policy, max_retries: 2])
@@ -122,6 +124,7 @@ Then configure your bridge with the audience of the target service:
 ```elixir
 use BridgeEx.Graphql,
   endpoint: "...",
+  decode_keys: :strings,
   auth0: [enabled: true, audience: "target_audience"]
 ```
 

lib/graphql.ex

@@ -15,6 +15,7 @@ defmodule BridgeEx.Graphql do
     * `http_options`: HTTP options to be passed to Telepoison. Defaults to `[timeout: 1_000, recv_timeout: 16_000]`.
     * `log_options`: override global configuration for logging errors. Takes the form of `[log_query_on_error: false, log_response_on_error: false]`
     * `max_attempts`: number of times the request will be retried upon failure. Defaults to `1`. ⚠️ Deprecated: use retry_options instead.
+    * `decode_keys`: selects which key decoder to use for decoding responses: `:atoms`, `:strings`, `:existing_atoms` . Defaults to `atoms` which is deprecated and will be replaced by `strings` in a future version of this lib.
     * `retry_options`: override configuration regarding retries, namely
       * `delay`: meaning depends on `timing`
         * `:constant`: retry ever `delay` ms
@@ -44,6 +45,7 @@ defmodule BridgeEx.Graphql do
       alias BridgeEx.Graphql.Client
       alias BridgeEx.Graphql.Formatter.SnakeCase
       alias BridgeEx.Graphql.Formatter.Adapter
+      alias BridgeEx.Graphql.Utils
 
       # local config
       # mandatory opts
@@ -58,6 +60,7 @@ defmodule BridgeEx.Graphql do
       @max_attempts Keyword.get(unquote(opts), :max_attempts, 1)
       @log_options Keyword.get(unquote(opts), :log_options, [])
       @format_variables Keyword.get(unquote(opts), :format_variables, false)
+      @decode_keys Keyword.get(unquote(opts), :decode_keys, :atoms)
 
       if Keyword.has_key?(unquote(opts), :max_attempts) do
         IO.warn(
@@ -66,6 +69,13 @@ defmodule BridgeEx.Graphql do
         )
       end
 
+      unless Keyword.has_key?(unquote(opts), :decode_keys) do
+        IO.warn(
+          "missing decode_keys option in graphql bridge creation. Currently fallbacks to the discouraged atom keys decoder which may lead to memory leak and raise security concerns. It will be replaced with the safer :strings keys decoder in a future major release. If you want to keep the current behavior and hide this warning, just add `decode_keys: :atoms` to your bridge creation options.",
+          Macro.Env.stacktrace(__ENV__)
+        )
+      end
+
       @doc """
       Run a graphql query or mutation over the configured bridge.
 
@@ -113,7 +123,8 @@ defmodule BridgeEx.Graphql do
             encode_variables: @encode_variables,
             log_options: @log_options,
             retry_options: retry_options,
-            format_variables: @format_variables
+            format_variables: @format_variables,
+            decode_keys: @decode_keys
           )
           |> format_response()
         end

lib/graphql/client.ex

@@ -35,10 +35,10 @@ defmodule BridgeEx.Graphql.Client do
     * `options`: extra HTTP options to be passed to Telepoison.
     * `headers`: extra HTTP headers.
     * `encode_variables`: whether to JSON encode variables or not.
+    * `decode_keys`: how JSON keys are decoded. Valid options are :strings (recommended), :atoms (currently the default, but discouraged due to security concerns - will be changed to :strings in a future version), :existing_atoms (safest, but may crash the application if an unexpected key is received)
     * `retry_options`: configures retry attempts. Takes the form of `[max_retries: 1, timing: :exponential]`
     * `log_options`: configures logging on errors. Takes the form of `[log_query_on_error: false, log_response_on_error: false]`.
   """
-
   @spec call(
           url :: String.t(),
           query :: String.t(),
@@ -54,8 +54,15 @@ defmodule BridgeEx.Graphql.Client do
     encode_variables = Keyword.get(opts, :encode_variables, false)
     http_options = Keyword.merge(@http_options, Keyword.get(opts, :options, []))
     http_headers = Map.merge(@http_headers, Keyword.get(opts, :headers, %{}))
-    log_options = Keyword.merge(log_options(), Keyword.get(opts, :log_options))
+    log_options = Keyword.merge(log_options(), Keyword.get(opts, :log_options, []))
     format_variables = Keyword.get(opts, :format_variables, false)
+    decode_keys = Keyword.get(opts, :decode_keys, :atoms)
+
+    unless Keyword.has_key?(opts, :decode_keys),
+      do:
+        Logger.warning(
+          "BridgeEx.Client.call will decode keys using atoms. This is discouraged and will be changed in a future version. To silence this warning, pass decode_keys: :atoms to the call function."
+        )
 
     retry_options =
       opts
@@ -85,7 +92,7 @@ defmodule BridgeEx.Graphql.Client do
         fn query ->
           url
           |> Telepoison.post(query, http_headers, http_options)
-          |> Utils.decode_http_response(query, log_options)
+          |> Utils.decode_http_response(query, decode_keys, log_options)
           |> Utils.parse_response()
         end,
         retry_options

lib/graphql/utils.ex

@@ -24,16 +24,22 @@ defmodule BridgeEx.Graphql.Utils do
           {:ok, HTTPoison.Response.t() | HTTPoison.AsyncResponse.t()}
           | {:error, HTTPoison.Error.t()},
           String.t(),
+          :strings | :atoms | :existing_atoms,
           Keyword.t()
         ) :: client_response()
-  def decode_http_response({:ok, %HTTPoison.Response{status_code: 200, body: body_string}}, _, _) do
-    Jason.decode(body_string, keys: :atoms)
-  end
+  def decode_http_response(
+        {:ok, %HTTPoison.Response{status_code: 200, body: body_string}},
+        _,
+        decode_keys,
+        _
+      ),
+      do: decode_json(body_string, decode_keys)
 
   def decode_http_response(
         {:ok,
          %HTTPoison.Response{status_code: code, body: body_string, request_url: request_url}},
         query,
+        _,
         log_options
       ) do
     log_response_on_error = Keyword.get(log_options, :log_response_on_error, false)
@@ -52,6 +58,7 @@ defmodule BridgeEx.Graphql.Utils do
   def decode_http_response(
         {:error, %HTTPoison.Error{reason: reason}},
         query,
+        _,
         log_options
       ) do
     log_query_on_error = Keyword.get(log_options, :log_query_on_error, false)
@@ -65,9 +72,17 @@ defmodule BridgeEx.Graphql.Utils do
   def parse_response({:error, error}), do: {:error, error}
 
   def parse_response({:ok, %{errors: errors}}), do: {:error, errors}
+  def parse_response({:ok, %{"errors" => errors}}), do: {:error, errors}
 
   def parse_response({:ok, %{data: data}}), do: {:ok, data}
+  def parse_response({:ok, %{"data" => data}}), do: {:ok, data}
 
   defp prepend_if(list, false, _), do: list
   defp prepend_if(list, true, value), do: [value | list]
+
+  @spec decode_json(String.t(), :strings | :atoms | :existing_atoms) ::
+          {:ok, map()} | {:error, any()}
+  defp decode_json(body, :strings), do: Jason.decode(body)
+  defp decode_json(body, :atoms), do: Jason.decode(body, keys: :atoms)
+  defp decode_json(body, :existing_atoms), do: Jason.decode(body, keys: :atoms!)
 end

test/auth0_authentication_test.exs

@@ -29,7 +29,8 @@ defmodule BridgeEx.Auth0AuthenticationTest do
     defmodule TestBridgeWithAuth0 do
       use BridgeEx.Graphql,
         endpoint: "http://localhost:#{bypass.port}/graphql",
-        auth0: [audience: "my-audience", enabled: true]
+        auth0: [audience: "my-audience", enabled: true],
+        decode_keys: :atoms
     end
 
     assert {:ok, %{key: "value"}} = TestBridgeWithAuth0.call("myquery", %{})
@@ -46,7 +47,8 @@ defmodule BridgeEx.Auth0AuthenticationTest do
     defmodule TestBridgeWithAuth0EnabledOnlyInBridge do
       use BridgeEx.Graphql,
         endpoint: "http://localhost:#{bypass.port}/graphql",
-        auth0: [audience: "my-audience", enabled: true]
+        auth0: [audience: "my-audience", enabled: true],
+        decode_keys: :atoms
     end
 
     catch_exit(TestBridgeWithAuth0EnabledOnlyInBridge.call("myquery", %{}))

test/config_validation_test.exs

@@ -15,7 +15,8 @@ defmodule BridgeEx.ConfigValidationTest do
       defmodule TestBridgeWithAuth0EnabledButNoAudience do
         use BridgeEx.Graphql,
           endpoint: "http://localhost:#{bypass.port}/graphql",
-          auth0: [enabled: true]
+          auth0: [enabled: true],
+          decode_keys: :atoms
       end
     end
   end

test/graphql/client_test.exs

@@ -0,0 +1,38 @@
+defmodule BridgeEx.Graphql.ClientTest do
+  use ExUnit.Case
+
+  alias BridgeEx.Graphql.Client
+
+  alias Bypass
+
+  setup do
+    bypass = Bypass.open(port: 55_000)
+    {:ok, bypass: bypass}
+  end
+
+  test "call using deprecated atom keys decoder", %{bypass: bypass} do
+    Bypass.expect(bypass, "POST", "/", fn conn ->
+      Plug.Conn.resp(conn, 200, ~s[{"data": {"result": "ok"}}])
+    end)
+
+    assert {:ok, %{result: "ok"}} = Client.call("localhost:55000/", "", %{}, decode_keys: :atoms)
+  end
+
+  test "call using safer string keys decoder", %{bypass: bypass} do
+    Bypass.expect(bypass, "POST", "/", fn conn ->
+      Plug.Conn.resp(conn, 200, ~s[{"data": {"result": "ok"}}])
+    end)
+
+    assert {:ok, %{"result" => "ok"}} =
+             Client.call("localhost:55000/", "", %{}, decode_keys: :strings)
+  end
+
+  test "call using new existing atom keys decoder", %{bypass: bypass} do
+    Bypass.expect(bypass, "POST", "/", fn conn ->
+      Plug.Conn.resp(conn, 200, ~s[{"data": {"new_atom": "ok"}}])
+    end)
+
+    assert {:ok, %{new_atom: "ok"}} =
+             Client.call("localhost:55000/", "", %{}, decode_keys: :existing_atoms)
+  end
+end

test/graphql_test.exs

@@ -21,6 +21,7 @@ defmodule BridgeEx.GraphqlTest do
     defmodule TestBridgeDeprecatedOption do
       use BridgeEx.Graphql,
         endpoint: "http://localhost:#{bypass.port}/graphql",
+        decode_keys: :atoms,
         max_attempts: 2
     end
 
@@ -33,12 +34,44 @@ defmodule BridgeEx.GraphqlTest do
     end)
 
     defmodule TestSimpleBridge do
-      use BridgeEx.Graphql, endpoint: "http://localhost:#{bypass.port}/graphql"
+      use BridgeEx.Graphql,
+        endpoint: "http://localhost:#{bypass.port}/graphql",
+        decode_keys: :atoms
     end
 
     assert {:ok, %{key: "value"}} = TestSimpleBridge.call("myquery", %{})
   end
 
+  test "runs graphql queries over the provided endpoint, decoding as strings", %{bypass: bypass} do
+    Bypass.expect(bypass, "POST", "/graphql", fn conn ->
+      Plug.Conn.resp(conn, 200, ~s[{"data": {"key": "value"}}])
+    end)
+
+    defmodule TestSimpleBridgeStrings do
+      use BridgeEx.Graphql,
+        endpoint: "http://localhost:#{bypass.port}/graphql",
+        decode_keys: :strings
+    end
+
+    assert {:ok, %{"key" => "value"}} = TestSimpleBridgeStrings.call("myquery", %{})
+  end
+
+  test "runs graphql queries over the provided endpoint, decoding as existing atoms", %{
+    bypass: bypass
+  } do
+    Bypass.expect(bypass, "POST", "/graphql", fn conn ->
+      Plug.Conn.resp(conn, 200, ~s[{"data": {"key": "value"}}])
+    end)
+
+    defmodule TestSimpleBridgeExistingAtoms do
+      use BridgeEx.Graphql,
+        endpoint: "http://localhost:#{bypass.port}/graphql",
+        decode_keys: :existing_atoms
+    end
+
+    assert {:ok, %{key: "value"}} = TestSimpleBridgeExistingAtoms.call("myquery", %{})
+  end
+
   @tag capture_log: true
   test "retries request on status code != 2XX", %{bypass: bypass} do
     Bypass.expect_once(bypass, "POST", "/graphql", fn conn ->
@@ -51,7 +84,8 @@ defmodule BridgeEx.GraphqlTest do
 
     defmodule TestBridgeRetriesOnBadResponse do
       use BridgeEx.Graphql,
-        endpoint: "http://localhost:#{bypass.port}/graphql"
+        endpoint: "http://localhost:#{bypass.port}/graphql",
+        decode_keys: :atoms
     end
 
     assert {:ok, %{key: "value"}} =
@@ -73,7 +107,8 @@ defmodule BridgeEx.GraphqlTest do
 
     defmodule TestBridgeRetriesOnError do
       use BridgeEx.Graphql,
-        endpoint: "http://localhost:#{bypass.port}/graphql"
+        endpoint: "http://localhost:#{bypass.port}/graphql",
+        decode_keys: :atoms
     end
 
     assert {:ok, %{key: "value"}} =
@@ -88,7 +123,9 @@ defmodule BridgeEx.GraphqlTest do
     end)
 
     defmodule TestBridgeWithCustomHeaders do
-      use BridgeEx.Graphql, endpoint: "http://localhost:#{bypass.port}/graphql"
+      use BridgeEx.Graphql,
+        endpoint: "http://localhost:#{bypass.port}/graphql",
+        decode_keys: :atoms
     end
 
     TestBridgeWithCustomHeaders.call("myquery", %{},
@@ -106,7 +143,9 @@ defmodule BridgeEx.GraphqlTest do
     end)
 
     defmodule TestBridgeForErrors do
-      use BridgeEx.Graphql, endpoint: "http://localhost:#{bypass.port}/graphql"
+      use BridgeEx.Graphql,
+        endpoint: "http://localhost:#{bypass.port}/graphql",
+        decode_keys: :atoms
     end
 
     assert {:error,
@@ -136,7 +175,9 @@ defmodule BridgeEx.GraphqlTest do
     end
 
     defmodule TestBridgePreventsRetryWithCustomPolicy do
-      use BridgeEx.Graphql, endpoint: "http://localhost:#{bypass.port}/graphql"
+      use BridgeEx.Graphql,
+        endpoint: "http://localhost:#{bypass.port}/graphql",
+        decode_keys: :atoms
     end
 
     assert {:error, {:bad_response, _}} =
@@ -166,7 +207,9 @@ defmodule BridgeEx.GraphqlTest do
     end
 
     defmodule TestBridgeRetriesWithGivenPolicy do
-      use BridgeEx.Graphql, endpoint: "http://localhost:#{bypass.port}/graphql"
+      use BridgeEx.Graphql,
+        endpoint: "http://localhost:#{bypass.port}/graphql",
+        decode_keys: :atoms
     end
 
     assert {:ok, %{key: "value"}} =