Stack trace argument types (#804)

* Use UndefinedFunctionError in stacktrace_test

To test if arguments are corrctly replaced in stack traces, call
String.to_atom/1 with two arguments to raise an actual
UndefinedFunctionError, then assert the arguments are replaced with just
the function arity.

* Add prototype for stacktrace argument types

Instead of stripping out arguments from exceptions like
UndefinedFunctionErrors, this prototype replaces the arguments with
their types.

* Add Appsignal.Utils.Type.of/1

The newly added of/1 function takes a term and converts it to a
typespec-like string representation.

* Add Appsignal.Utils.Type.from/1

The from/1 function returns a Type struct containing the passed term's
type. The struct implents the Inspect protocol, so it returns the type
without inspect-style quotes whenever it's inspected.

* Make Appsignal.Utils.Type.of/1 private

Since the of/1 function will only be used from the from/1 function for
now, make of/1 private and test it through from/1.

* Remove ArgumentType, use Utils.Type instead

Appsignal.Stacktrace switched to using Appsignal.Utils.Type, which
implements the same behavior as the previously used ArgumentType module.

* Add ArgumentCleaner to clean stack trace arguments

Instead of using Appsignal.Utils.Type.of/1 directly from the Stacktrace
module, this patch adds ArgumentCleaner.clean/1, which only runs terms
through Type.of/1 when they potentially hold sensitive data.

In practice, this means booleans, integers, floats, pids, ports,
references and map keys are left as-is, and everything else is converted
to its type.

* Add changeset for arguments patch

"Replace arguments in stack traces with sanitized versions instead of
stripping them out completely"

* Use “types” over “arities” in stack trace tests

We’re no longer replacing stack trace arguments with arities, but
using argument types instead.

Co-authored-by: Luismi Ramírez <luismi.ramirez@protonmail.com>

Co-authored-by: Luismi Ramírez <luismi.ramirez@protonmail.com>

.changesets/replace-arguments-in-stack-traces-with-sanitized-versions-instead-of-stripping-them-out-completely.md

@@ -0,0 +1,6 @@
+---
+bump: "patch"
+type: "change"
+---
+
+Replace arguments in stack traces with sanitized versions instead of stripping them out completely

lib/appsignal/stacktrace.ex

@@ -24,11 +24,15 @@ defmodule Appsignal.Stacktrace do
 
   defp format_stacktrace_entry(entry) when is_binary(entry), do: entry
 
-  defp format_stacktrace_entry({module, function, arity, location}) when is_list(arity) do
-    format_stacktrace_entry({module, function, length(arity), location})
+  defp format_stacktrace_entry({module, function, arguments, location}) when is_list(arguments) do
+    Exception.format_stacktrace_entry({module, function, to_types(arguments), location})
   end
 
   defp format_stacktrace_entry(entry) do
     Exception.format_stacktrace_entry(entry)
   end
+
+  defp to_types(arguments) do
+    Enum.map(arguments, &Appsignal.Utils.ArgumentCleaner.clean/1)
+  end
 end

lib/appsignal/utils/argument_cleaner.ex

@@ -0,0 +1,17 @@
+defmodule Appsignal.Utils.ArgumentCleaner do
+  alias Appsignal.Utils.Type
+
+  def clean(argument)
+      when is_boolean(argument) or is_integer(argument) or is_float(argument) or is_pid(argument) or
+             is_port(argument) or is_reference(argument) do
+    argument
+  end
+
+  def clean(argument) when is_map(argument) do
+    {struct, map} = Map.pop(argument, :__struct__)
+
+    "%#{Appsignal.Utils.module_name(struct)}{#{Enum.map_join(map, ", ", fn {key, value} -> "#{inspect(key)} => #{clean(value)}" end)}}"
+  end
+
+  def clean(argument), do: Type.from(argument)
+end

lib/appsignal/utils/type.ex

@@ -0,0 +1,53 @@
+defmodule Appsignal.Utils.Type do
+  defstruct [:type]
+
+  def from(term) do
+    %__MODULE__{type: of(term)}
+  end
+
+  defp of(term) when is_boolean(term), do: "boolean"
+
+  defp of(nil), do: "nil"
+
+  defp of(term) when is_integer(term), do: "integer"
+
+  defp of(term) when is_float(term), do: "float"
+
+  defp of(term) when is_pid(term), do: "pid"
+
+  defp of(term) when is_atom(term), do: "atom"
+
+  defp of(term) when is_bitstring(term) and not is_binary(term), do: "bitstring"
+
+  defp of(term) when is_binary(term), do: "binary"
+
+  defp of(term) when is_function(term), do: "function"
+
+  defp of(term) when is_port(term), do: "port"
+
+  defp of(term) when is_reference(term), do: "reference"
+
+  defp of(term) when is_tuple(term) do
+    "{#{Enum.map_join(Tuple.to_list(term), ", ", &of/1)}}"
+  end
+
+  defp of(term) when is_list(term) do
+    "[#{Enum.map_join(term, ", ", &of/1)}]"
+  end
+
+  defp of(term) when is_map(term) do
+    {struct, map} = Map.pop(term, :__struct__)
+
+    "%#{Appsignal.Utils.module_name(struct)}{#{Enum.map_join(map, ", ", fn {key, value} -> "#{of(key)} => #{of(value)}" end)}}"
+  end
+
+  defp of(_term), do: "unknown"
+end
+
+defimpl Inspect, for: Appsignal.Utils.Type do
+  def inspect(%Appsignal.Utils.Type{type: type}, _opts), do: type
+end
+
+defimpl String.Chars, for: Appsignal.Utils.Type do
+  def to_string(%Appsignal.Utils.Type{type: type}), do: type
+end

test/appsignal/stacktrace_test.exs

@@ -33,26 +33,24 @@ defmodule Appsignal.StacktraceTest do
       all_location = expected_location ++ [error_info: %{module: Exception}]
       assert Enum.all?(location, &Enum.member?(all_location, &1))
     end
-  end
-
-  test "formats stacktrace lines" do
-    stacktrace =
-      [line] = [
-        {:elixir_translator, :guard_op, 2, [file: 'src/elixir_translator.erl', line: 317]}
-      ]
 
-    assert Stacktrace.format(stacktrace) == [
-             Exception.format_stacktrace_entry(line)
-           ]
+    test "formats stacktrace lines", %{stack: stack} do
+      [line | _] = stack
+      assert Stacktrace.format([line]) == [Exception.format_stacktrace_entry(line)]
+    end
   end
 
-  test "replaces arguments with arities" do
-    stacktrace = [
-      {:erl_internal, :op_type, [:get_stacktrace, 0], [file: 'erl_internal.erl', line: 212]}
-    ]
+  describe "get/0, with an exception with included arguments" do
+    setup do
+      String.to_atom("string", :extra_argument)
+    catch
+      :error, _ -> %{stack: Stacktrace.get()}
+    end
 
-    [line] = Stacktrace.format(stacktrace)
-    assert line =~ ~r{\(stdlib( [\w.-]+)?\) erl_internal.erl:212: :erl_internal.op_type/2}
+    test "replaces arguments with types", %{stack: stack} do
+      [line | _] = Stacktrace.format(stack)
+      assert line =~ ~r{\(elixir( [\w.-]+)?\) String.to_atom\(binary, atom\)}
+    end
   end
 
   test "handles lists of binaries" do

test/appsignal/utils/argument_cleaner_test.exs

@@ -0,0 +1,34 @@
+defmodule NonEmptyStruct do
+  defstruct [:foo]
+end
+
+defmodule Appsignal.Utils.ArgumentCleanerTest do
+  alias Appsignal.Utils.{ArgumentCleaner, Type}
+  use ExUnit.Case
+
+  test "cleaned types" do
+    assert ArgumentCleaner.clean(:foo) == %Type{type: "atom"}
+    assert ArgumentCleaner.clean("bar") == %Type{type: "binary"}
+    assert ArgumentCleaner.clean(<<1::1>>) == %Type{type: "bitstring"}
+    assert ArgumentCleaner.clean(fn -> nil end) == %Type{type: "function"}
+  end
+
+  test "untouched types" do
+    assert ArgumentCleaner.clean(true) == true
+    assert ArgumentCleaner.clean(false) == false
+    assert ArgumentCleaner.clean(1) == 1
+    assert ArgumentCleaner.clean(1.2) == 1.2
+    pid = :erlang.list_to_pid('<0.0.0>')
+    assert ArgumentCleaner.clean(pid) == pid
+    port = Port.open({:spawn, "echo foo"}, [])
+    assert ArgumentCleaner.clean(port) == port
+    reference = make_ref()
+    assert ArgumentCleaner.clean(reference) == reference
+  end
+
+  test "map" do
+    assert ArgumentCleaner.clean(%{foo: 1}) == "%{:foo => 1}"
+    assert ArgumentCleaner.clean(%{foo: "bar"}) == "%{:foo => binary}"
+    assert ArgumentCleaner.clean(%NonEmptyStruct{foo: "bar"}) == "%NonEmptyStruct{:foo => binary}"
+  end
+end

test/appsignal/utils/type_test.exs

@@ -0,0 +1,97 @@
+defmodule EmptyStruct do
+  defstruct []
+end
+
+defmodule NonEmptyStruct do
+  defstruct [:foo]
+end
+
+defmodule Appsignal.Utils.TypeTest do
+  use ExUnit.Case
+  alias Appsignal.Utils.Type
+
+  test "atom" do
+    assert Type.from(:foo).type == "atom"
+  end
+
+  test "booleans" do
+    assert Type.from(true).type == "boolean"
+    assert Type.from(false).type == "boolean"
+  end
+
+  test "nils" do
+    assert Type.from(nil).type == "nil"
+  end
+
+  test "binary" do
+    assert Type.from("bar").type == "binary"
+  end
+
+  test "bitstring" do
+    assert Type.from(<<1::1>>).type == "bitstring"
+  end
+
+  test "function" do
+    assert Type.from(fn -> nil end).type == "function"
+  end
+
+  test "float" do
+    assert Type.from(1.2).type == "float"
+  end
+
+  test "integer" do
+    assert Type.from(1).type == "integer"
+  end
+
+  test "pid" do
+    assert Type.from(:erlang.list_to_pid('<0.0.0>')).type == "pid"
+  end
+
+  test "port" do
+    assert Type.from(Port.open({:spawn, "echo foo"}, [])).type == "port"
+  end
+
+  test "reference" do
+    assert Type.from(make_ref()).type == "reference"
+  end
+
+  test "empty tuple" do
+    assert Type.from({}).type == "{}"
+  end
+
+  test "non-empty tuple" do
+    assert Type.from({:foo, "bar"}).type == "{atom, binary}"
+  end
+
+  test "empty list" do
+    assert Type.from([]).type == "[]"
+  end
+
+  test "non-empty list" do
+    assert Type.from([:foo]).type == "[atom]"
+  end
+
+  test "keyword list" do
+    assert Type.from(foo: "bar").type == "[{atom, binary}]"
+  end
+
+  test "empty map" do
+    assert Type.from(%{}).type == "%{}"
+  end
+
+  test "non-empty map" do
+    assert Type.from(%{foo: "bar"}).type == "%{atom => binary}"
+  end
+
+  test "empty struct" do
+    assert Type.from(%EmptyStruct{}).type == "%EmptyStruct{}"
+  end
+
+  test "non-empty struct" do
+    assert Type.from(%NonEmptyStruct{foo: 1}).type == "%NonEmptyStruct{atom => integer}"
+  end
+
+  test "inspect" do
+    assert inspect(Type.from("string")) == "binary"
+  end
+end