fix(callbacks): wrap plug and phoenix callbacks

lib/sentry.ex

@@ -155,13 +155,18 @@ defmodule Sentry do
 
   ## Crashing Callbacks
 
-  If a `:before_send`, `:after_send_event`, `:filter`, `:before_send_log`,
-  `:before_send_metric`, or `:traces_sampler` callback raises, throws, or exits, Sentry catches
-  the failure and logs it at the `:error` level instead of letting it reach the code that was
-  reporting the event. The log carries the `:sentry` logger domain, so the SDK never reports
-  its own callback failure as an event.
+  When a callback you configure raises, throws, or exits, Sentry catches the failure and logs
+  it at the `:error` level instead of letting it reach the code that was reporting the event
+  or serving the request. The log carries the `:sentry` logger domain, so the SDK never
+  reports its own callback failure as an event.
+
+  What happens next depends on where the callback runs.
 
-  The item being handled is then dropped:
+  ### Event Callbacks
+
+  If a `:before_send`, `:after_send_event`, `:filter`, `:before_send_log`,
+  `:before_send_metric`, or `:traces_sampler` callback fails, the item being handled is
+  dropped:
 
     * A `:before_send` callback that crashes is treated like one that returned `false`.
       The event or transaction is not sent, and the capture function returns `:excluded`.
@@ -184,6 +189,31 @@ defmodule Sentry do
   dropped and the child spans of that trace inherit that decision instead of calling the
   failing sampler again.
 
+  ### Request Callbacks
+
+  The callbacks that `Sentry.PlugContext`, `Sentry.PlugCapture`, and `Sentry.LiveViewHook`
+  accept run inside your request or LiveView process, where a failure of theirs would break
+  your application rather than just the report. It cannot: the request is served, and the
+  LiveView keeps running, exactly as they would have without Sentry. `Sentry.PlugCapture`
+  additionally re-raises your application's original exception unchanged, whatever fails
+  while it is capturing it.
+
+  Nothing is dropped either. The event is still reported, and only the field the failing
+  callback was responsible for degrades:
+
+  | Callback | Value reported after a crash |
+  | --- | --- |
+  | `Sentry.PlugContext`'s `:body_scrubber`, `:header_scrubber`, `:cookie_scrubber`, or `:url_scrubber` | the SDK's own default scrubber for that field |
+  | `Sentry.PlugContext`'s `:remote_address_reader` | the address the SDK's default reader produces |
+  | `Sentry.PlugCapture`'s `:scrubber` | the connection scrubbed by `Sentry.Scrubber.scrub/1` |
+  | `Sentry.LiveViewHook`'s `:scrubber` | redacted breadcrumb data, that is, an empty map |
+
+  > #### A crashed scrubber reports more, not less {: .warning}
+  >
+  > Apart from `Sentry.LiveViewHook`, which redacts the data outright, falling back to the
+  > SDK's default scrubber means that data only your custom scrubber was dropping is sent to
+  > Sentry for as long as that scrubber keeps failing. The error-level log is the only signal.
+
   ## Reporting Source Code
 
   Sentry supports reporting the source code of (and around) the line that

lib/sentry/callback.ex

@@ -4,7 +4,7 @@ defmodule Sentry.Callback do
   alias Sentry.ClientReport
   alias Sentry.LoggerUtils
 
-  @type spec() :: (... -> term()) | {module(), atom()}
+  @type spec() :: (... -> term()) | {module(), atom()} | {module(), atom(), [term()]}
 
   @spec run(atom(), (-> result), result, keyword()) :: result when result: var
   def run(name, fun, fallback, opts \\ []) when is_list(opts) do
@@ -19,14 +19,16 @@ defmodule Sentry.Callback do
   end
 
   @spec run(atom(), (-> result)) :: {:ok, result} | :failed when result: var
-  def run(name, fun) when is_atom(name) and is_function(fun, 0) do
+  def run(name, fun) when is_atom(name) do
+    guard("#{inspect(name)} callback failed", fun)
+  end
+
+  @spec guard(String.t(), (-> result)) :: {:ok, result} | :failed when result: var
+  def guard(description, fun) when is_binary(description) and is_function(fun, 0) do
     {:ok, fun.()}
   catch
     kind, reason ->
-      LoggerUtils.error(
-        "#{inspect(name)} callback failed: " <>
-          Exception.format(kind, reason, __STACKTRACE__)
-      )
+      LoggerUtils.error(description <> ": " <> Exception.format(kind, reason, __STACKTRACE__))
 
       :failed
   end
@@ -40,6 +42,9 @@ defmodule Sentry.Callback do
       {mod, fun} when is_atom(mod) and is_atom(fun) ->
         fn -> apply(mod, fun, args) end
 
+      {mod, fun, extra_args} when is_atom(mod) and is_atom(fun) and is_list(extra_args) ->
+        fn -> apply(mod, fun, args ++ extra_args) end
+
       other ->
         raise ArgumentError,
               "#{inspect(name)} must be an anonymous function or a {module, function} tuple, " <>

lib/sentry/live_view_hook.ex

@@ -73,12 +73,22 @@ if Code.ensure_loaded?(Phoenix.LiveView) do
     The scrubber is resolved once at `on_mount` time and applies to every
     breadcrumb recorded for the lifetime of the LiveView process.
 
+    ## Crashing Callbacks
+
+    The `:scrubber` runs in the LiveView process, where a failure of its own
+    would crash the LiveView. It cannot: if it raises, throws, exits, or returns
+    anything other than a map, Sentry catches the failure and logs it at the
+    `:error` level with the `:sentry` logger domain, so the SDK never reports its
+    own callback failure as an event. The breadcrumb is then recorded with
+    redacted data - an empty map - rather than with data that was never scrubbed.
+
     """
 
     @moduledoc since: "10.5.0"
 
     import Phoenix.LiveView, only: [attach_hook: 4, get_connect_info: 2]
 
+    alias Sentry.Callback
     alias Sentry.Context
     alias Sentry.LoggerUtils
 
@@ -129,34 +139,16 @@ if Code.ensure_loaded?(Phoenix.LiveView) do
     end
 
     defp scrub(data) when is_map(data) do
-      {mod, fun, args} =
-        Process.get(@scrubber_pdict_key, {__MODULE__, :default_scrubber, []})
-
-      try do
-        case apply(mod, fun, [data | args]) do
-          result when is_map(result) ->
-            result
-
-          other ->
-            LoggerUtils.error(
-              "Sentry.LiveViewHook scrubber returned non-map value: #{inspect(other)}; " <>
-                "falling back to redacted data",
-              event_source: :logger
-            )
-
-            %{}
-        end
-      catch
-        # We must NEVER raise an error in a hook, as it will crash the LiveView process
-        # and we don't want Sentry to be responsible for that.
-        kind, reason ->
-          LoggerUtils.error(
-            "Sentry.LiveViewHook scrubber raised an error: #{Exception.format(kind, reason)}; " <>
-              "falling back to redacted data",
-            event_source: :logger
-          )
-
-          %{}
+      scrubber = Process.get(@scrubber_pdict_key, {__MODULE__, :default_scrubber, []})
+
+      # We must NEVER raise an error in a hook, as it will crash the LiveView process
+      # and we don't want Sentry to be responsible for that.
+      with {:ok, scrubbed} <-
+             Callback.run(:scrubber, Callback.to_fun(:scrubber, scrubber, [data])),
+           {:ok, scrubbed} <- Callback.validate(:scrubber, scrubbed, &is_map/1, "a map") do
+        scrubbed
+      else
+        _ -> %{}
       end
     end
 

lib/sentry/plug_capture.ex

@@ -73,7 +73,9 @@ defmodule Sentry.PlugCapture do
       will be invoked to scrub sensitive data from `Plug.Conn` structs. The
       `Plug.Conn` struct is prepended to `args` before invoking the function,
       so that the final function will be called as `apply(module, function, [conn | args])`.
-      The function must return a `Plug.Conn` struct. By default, the built-in
+      The function must return a `Plug.Conn` struct; if it returns anything else,
+      or if it crashes, scrubbing falls back to the built-in scrubber (see
+      *Crashing Callbacks* below). By default, the built-in
       scrubber delegates to `Sentry.Scrubber.scrub/1`, which honors any
       `:body_scrubber`, `:header_scrubber`, `:cookie_scrubber`, or
       `:url_scrubber` opts configured on `Sentry.PlugContext` for the current
@@ -95,6 +97,32 @@ defmodule Sentry.PlugCapture do
         everything else (notably the decoded session under `:plug_session`);
         configurable via the `scrubber: [conn_private_allow_list: ...]` option
 
+  ## Crashing Callbacks
+
+  This module captures the application's exception from inside `c:Plug.call/2`,
+  where a failure of its own would replace the error the application raised. It
+  cannot: if anything in the capture path raises, throws, or exits - the
+  `:scrubber` callback, the scrubbing of the exception, or the reporting
+  itself - Sentry catches the failure and re-raises **the application's
+  original exception, unchanged**. The failure is logged at the `:error` level
+  with the `:sentry` logger domain, so the SDK never reports its own failure as
+  an event.
+
+  Only the reporting degrades, and only as far as the failure forces:
+
+  | Failure | What Sentry still reports |
+  | --- | --- |
+  | The `:scrubber` crashes, or returns something other than a `Plug.Conn` | The event, with the conn scrubbed by the built-in scrubber, `Sentry.Scrubber.scrub/1` |
+  | Scrubbing a `Phoenix.ActionClauseError` fails for any other reason | The event, with each of the exception's arguments scrubbed on its own, without mirroring the conn's scrubbed params onto the action's params argument |
+  | Capturing the event itself fails | Nothing - the log is the only record of the error |
+
+  > #### A crashed scrubber reports more, not less {: .warning}
+  >
+  > The fallback redacts the keys listed in `Sentry.Scrubber.default_param_keys/0`
+  > and `Sentry.Scrubber.default_header_keys/0`, and nothing more. Data that only
+  > a custom `:scrubber` was dropping is sent to Sentry for as long as that
+  > scrubber keeps failing, and the error-level log is the only signal.
+
   """
   defmacro __using__(opts) do
     quote do
@@ -142,7 +170,7 @@ defmodule Sentry.PlugCapture do
           kind, reason ->
             message = "Uncaught #{kind} - #{inspect(reason)}"
             stack = __STACKTRACE__
-            _ = Sentry.capture_message(message, stacktrace: stack, event_source: :plug)
+            :ok = Sentry.PlugCapture.__capture_message__(message, stack)
             :erlang.raise(kind, reason, stack)
         end
       end
@@ -151,50 +179,83 @@ defmodule Sentry.PlugCapture do
 
   @doc false
   def __capture_exception__(exception, stacktrace, scrubber) do
-    # `Phoenix.ActionClauseError` is the one error whose args we know the shape of —
-    # a controller action is invoked as `apply(controller, action, [conn, conn.params])`.
-    # We handle it explicitly: `StacktraceScrubber` does the generic per-arg scrubbing,
-    # and we instruct it (via the callback) to scrub the conn through the configured
-    # `:scrubber` and mirror the conn's scrubbed params onto the standalone params arg.
-    exception =
-      if is_struct(exception, Phoenix.ActionClauseError) do
-        Sentry.Scrubber.StacktraceScrubber.scrub(
-          exception,
-          &scrub_action_clause_args(&1, scrubber)
+    _ =
+      Sentry.Callback.guard("Sentry failed to capture an exception from Plug", fn ->
+        Sentry.capture_exception(scrub_exception(exception, scrubber),
+          stacktrace: stacktrace,
+          event_source: :plug,
+          handled: false
         )
-      else
-        exception
-      end
+      end)
 
+    :ok
+  end
+
+  @doc false
+  def __capture_message__(message, stacktrace) do
     _ =
-      Sentry.capture_exception(exception,
-        stacktrace: stacktrace,
-        event_source: :plug,
-        handled: false
-      )
+      Sentry.Callback.guard("Sentry failed to capture a message from Plug", fn ->
+        Sentry.capture_message(message, stacktrace: stacktrace, event_source: :plug)
+      end)
 
     :ok
   end
 
+  # `Phoenix.ActionClauseError` is the one error whose args we know the shape of -
+  # a controller action is invoked as `apply(controller, action, [conn, conn.params])`.
+  # We handle it explicitly: `StacktraceScrubber` does the generic per-arg scrubbing,
+  # and we instruct it (via the callback) to scrub the conn through the configured
+  # `:scrubber` and mirror the conn's scrubbed params onto the standalone params arg.
+  defp scrub_exception(exception, scrubber) do
+    if is_struct(exception, Phoenix.ActionClauseError) do
+      case Sentry.Callback.guard("Sentry failed to scrub a Phoenix.ActionClauseError", fn ->
+             Sentry.Scrubber.StacktraceScrubber.scrub(
+               exception,
+               &scrub_action_clause_args(&1, scrubber)
+             )
+           end) do
+        {:ok, scrubbed} -> scrubbed
+        :failed -> Sentry.Scrubber.StacktraceScrubber.scrub(exception)
+      end
+    else
+      exception
+    end
+  end
+
   defp scrub_action_clause_args(args, scrubber) do
-    conn = Enum.find(args, &is_struct(&1, Plug.Conn))
-    scrubbed_conn = apply_scrubber(conn, scrubber)
-    params = conn.params
-
-    Enum.map(args, fn
-      ^conn -> scrubbed_conn
-      ^params -> scrubbed_conn.params
-      other -> Sentry.Scrubber.scrub(other)
-    end)
+    case Enum.find(args, &is_struct(&1, Plug.Conn)) do
+      nil ->
+        Sentry.Scrubber.StacktraceScrubber.scrub_args(args)
+
+      conn ->
+        scrubbed_conn = apply_scrubber(conn, scrubber)
+        params = conn.params
+
+        Enum.map(args, fn
+          ^conn -> scrubbed_conn
+          ^params -> scrubbed_conn.params
+          other -> Sentry.Scrubber.scrub(other)
+        end)
+    end
   end
 
   @doc false
   def default_scrubber(conn), do: Sentry.Scrubber.scrub(conn)
 
-  defp apply_scrubber(conn, {mod, fun, args} = _scrubber) do
-    case apply(mod, fun, [conn | args]) do
-      conn when is_struct(conn, Plug.Conn) -> conn
-      other -> raise ":scrubber function must return a Plug.Conn struct, got: #{inspect(other)}"
+  defp apply_scrubber(conn, scrubber) do
+    invocation = Sentry.Callback.to_fun(:scrubber, scrubber, [conn])
+
+    with {:ok, scrubbed} <- Sentry.Callback.run(:scrubber, invocation),
+         {:ok, scrubbed} <-
+           Sentry.Callback.validate(
+             :scrubber,
+             scrubbed,
+             &is_struct(&1, Plug.Conn),
+             "a Plug.Conn struct"
+           ) do
+      scrubbed
+    else
+      _ -> default_scrubber(conn)
     end
   end
 end