fix(scrubbing): apply the url scrubber to conn path and query fields …

lib/sentry/plug_capture.ex

@@ -86,6 +86,10 @@ defmodule Sentry.PlugCapture do
         (defaulting to the sensitive params `password`, `passwd`, `secret`; a
         `nil` `body_scrubber` empties both), and scrubs the same sensitive params
         in `query_params`
+      * derives `request_path`, `path_info` and `query_string` from the URL the
+        configured `url_scrubber` returns, so a scrubber that redacts a path
+        segment redacts it here too; `query_string` is scrubbed against the
+        sensitive params either way
       * clears `assigns` (where auth libraries store user structs and tokens)
       * reduces `private` to an allow-list of framework metadata, dropping
         everything else (notably the decoded session under `:plug_session`);

lib/sentry/plug_context.ex

@@ -117,6 +117,14 @@ defmodule Sentry.PlugContext do
 
       plug Sentry.PlugContext, url_scrubber: {MySentryScrubber, :scrub_url}
 
+  The `:url_scrubber` governs more than the reported request URL: wherever the
+  connection itself is reported — a `%Plug.Conn{}` inspected into a stacktrace
+  frame variable, or a `Phoenix.ActionClauseError` captured by
+  `Sentry.PlugCapture` — its `request_path`, `path_info` and `query_string` are
+  derived from the scrubbed URL, so the example above redacts the token in all
+  of them. Setting `:url_scrubber` to `nil` opts out; `query_string` is still
+  scrubbed against the sensitive parameter keys.
+
   ## Including Request Identifiers
 
   If you're using Phoenix, `Plug.RequestId`, or any other method to set a *request ID*

lib/sentry/scrubber.ex

@@ -50,27 +50,35 @@ defmodule Sentry.Scrubber do
       default `scrub(conn, field)` clause when none is registered, or
     * a fixed tag — `:clear` replaces the field with `%{}`, `:params` scrubs the
       field as a params-shaped map, `:query_string` redacts sensitive params from
-      a raw query string, and `:private_allow_list` keeps only the registered
-      allow-listed keys of the field (see `default_private_allow_list/0` and the
-      `:private_allow_list` option of `put_conn_scrubber/1`), dropping everything
-      else.
+      a raw query string, `:url_scrubbed` derives the field from the URL the
+      registered `:url_scrubber` returns, and `:private_allow_list` keeps only
+      the registered allow-listed keys of the field (see
+      `default_private_allow_list/0` and the `:private_allow_list` option of
+      `put_conn_scrubber/1`), dropping everything else.
 
   By default `scrub/1` redacts `cookies`, `req_headers`, `params`, and
   `body_params` (the configurable fields — `body_params` shares the
   `:body_scrubber` with `params`, so it honors the same registered scrubber and
   is emptied when `body_scrubber` is `nil`), clears `req_cookies` and `assigns`
-  to `%{}`, scrubs `query_params` as a params-shaped map, and reduces `private`
-  to its allow-listed keys (`default_private_allow_list/0`). `assigns` is cleared
+  to `%{}`, scrubs `query_params` as a params-shaped map, derives `request_path`,
+  `path_info` and `query_string` from the scrubbed URL, and reduces `private` to
+  its allow-listed keys (`default_private_allow_list/0`). `assigns` is cleared
   wholesale because auth libraries (Guardian, Pow, Coherence) routinely store
   decoded tokens, full user structs, and session data there, where no key-based
   heuristic redacts safely. `private` keeps only the allow-listed framework
   metadata and drops everything else (notably `:plug_session`).
 
   The defaults can be overridden per call with `scrub(conn, overrides)`, where
   `overrides` is a `field: strategy` keyword list merged over the attribute —
-  for example `scrub(conn, assigns: :clear)`. The request URL is not a conn
-  field, so callers fetch the registered `:url_scrubber` with `get/1` and apply
-  it to the conn.
+  for example `scrub(conn, assigns: :clear)`.
+
+  The request URL itself is not a conn field, so callers that report it (such as
+  `Sentry.PlugContext`) fetch the registered `:url_scrubber` with `get/1` and
+  apply it to the conn. The conn's own URL-derived fields are covered here: a
+  custom `:url_scrubber` that redacts a path segment redacts it in
+  `request_path` and `path_info` too, wherever the conn itself is reported.
+  Registering `url_scrubber: nil` opts out of that, though `query_string` is
+  still scrubbed against the sensitive key list.
   """
 
   @moduledoc since: "13.1.0"
@@ -105,6 +113,7 @@ defmodule Sentry.Scrubber do
   # scrubber struct-key (resolved per process via `get/1`) or a fixed tag:
   # `:clear` -> `%{}`, `:params` -> params-shaped scrub (Unfetched-safe),
   # `:query_string` -> redact sensitive params from the raw query string,
+  # `:url_scrubbed` -> derive from the URL the registered `:url_scrubber` returns,
   # `:private_allow_list` -> keep only the registered allow-listed keys.
   # Add an entry to make a new conn field scrubbed by default.
   #
@@ -121,7 +130,9 @@ defmodule Sentry.Scrubber do
     params: :body_scrubber,
     body_params: :body_scrubber,
     query_params: :params,
-    query_string: :query_string,
+    query_string: :url_scrubbed,
+    request_path: :url_scrubbed,
+    path_info: :url_scrubbed,
     assigns: :clear,
     private: :private_allow_list
   ]
@@ -389,8 +400,10 @@ defmodule Sentry.Scrubber do
   Given a `%Plug.Conn{}`, scrubs each field listed in `@scrubbable_conn_fields`
   according to its strategy — see the "Scrubbing a `%Plug.Conn{}`" section in
   the module docs and `scrub/2` for the per-field defaults and how to override
-  them per call. The request URL is not a conn field; callers scrub it
-  separately by applying the `:url_scrubber` from `get/1` (whose default is
+  them per call. This includes `request_path`, `path_info` and `query_string`,
+  which are derived from the URL the registered `:url_scrubber` returns. The
+  reported request URL is not a conn field; callers scrub that separately by
+  applying the `:url_scrubber` from `get/1` (whose default is
   `scrub(conn, :url)`).
 
   Given a plain map, recursively scrubs it with the default sensitive keys —
@@ -461,8 +474,10 @@ defmodule Sentry.Scrubber do
   Behaves like `scrub/1` but merges the `field: strategy` keyword `overrides`
   over the `@scrubbable_conn_fields` defaults, so a caller can scrub additional
   fields or change a field's strategy for that call. Strategies are a
-  configurable scrubber struct-key, `:clear` (replace with `%{}`), or `:params`
-  (params-shaped scrub of that field):
+  configurable scrubber struct-key, `:clear` (replace with `%{}`), `:params`
+  (params-shaped scrub of that field), `:query_string` (redact sensitive params
+  from that raw query string), `:url_scrubbed` (derive that field from the
+  scrubbed URL), or `:private_allow_list` (keep only the allow-listed keys):
 
       Sentry.Scrubber.scrub(conn, assigns: :clear, query_params: :params)
   """
@@ -482,10 +497,12 @@ defmodule Sentry.Scrubber do
   end
 
   def scrub(conn, overrides) when is_struct(conn, Plug.Conn) and is_list(overrides) do
-    @scrubbable_conn_fields
-    |> Keyword.merge(overrides)
-    |> Enum.reduce(conn, fn {field, strategy}, acc ->
-      Map.replace(acc, field, normalize(field, scrub_conn_field(conn, field, strategy)))
+    fields = Keyword.merge(@scrubbable_conn_fields, overrides)
+
+    uri = if url_scrubbed?(fields), do: scrubbed_uri(conn)
+
+    Enum.reduce(fields, conn, fn {field, strategy}, acc ->
+      Map.replace(acc, field, normalize(field, scrub_conn_field(conn, field, strategy, uri)))
     end)
   end
 
@@ -534,6 +551,13 @@ defmodule Sentry.Scrubber do
   #     string) via `scrub_query_string/1`
   #   * `:private_allow_list` — keeps only the registered allow-listed keys of
   #     THIS field (a map), dropping everything else
+  #   * `:url_scrubbed` — derives THIS field from the URL produced by the
+  #     registered `:url_scrubber`, so a custom scrubber governs the conn's own
+  #     path as well as the reported request URL
+  defp scrub_conn_field(conn, field, :url_scrubbed, uri), do: url_scrubbed(conn, field, uri)
+
+  defp scrub_conn_field(conn, field, strategy, _uri), do: scrub_conn_field(conn, field, strategy)
+
   defp scrub_conn_field(conn, _field, scrubber_key) when scrubber_key in @scrubber_names,
     do: get(scrubber_key).(conn)
 
@@ -548,6 +572,57 @@ defmodule Sentry.Scrubber do
   defp scrub_conn_field(conn, field, :private_allow_list),
     do: Map.take(Map.fetch!(conn, field), scrubber().private_allow_list)
 
+  defp url_scrubbed?(fields), do: Enum.any?(fields, &match?({_field, :url_scrubbed}, &1))
+
+  # Applies the registered `:url_scrubber` and parses the result. Returns `nil`
+  # when the scrubber hands back a non-binary, in which case every
+  # `:url_scrubbed` field keeps the conn's own value: scrubbing runs while an
+  # error is already being reported, so a broken user scrubber must not take the
+  # report down with it. `URI.parse/1` needs no such guard — it returns a `%URI{}`
+  # for any binary, and unparseable input lands in `:path`, which over-redacts
+  # rather than under-redacts.
+  defp scrubbed_uri(conn) do
+    case get(:url_scrubber).(conn) do
+      url when is_binary(url) -> URI.parse(url)
+      _other -> nil
+    end
+  end
+
+  defp url_scrubbed(conn, :request_path, uri), do: scrubbed_path(conn, uri)
+
+  defp url_scrubbed(conn, :path_info, uri) do
+    case scrubbed_path(conn, uri) do
+      path when path == conn.request_path ->
+        conn.path_info
+
+      path ->
+        path |> split_path() |> Enum.drop(length(conn.script_name))
+    end
+  end
+
+  defp url_scrubbed(conn, :query_string, uri) do
+    conn |> scrubbed_query(uri) |> scrub_query_string()
+  end
+
+  defp url_scrubbed(conn, field, _uri), do: Map.fetch!(conn, field)
+
+  # `URI.parse/1` yields `path: nil` for a URL without one, and `nil` is not a
+  # valid `:request_path` — it makes `Plug.Conn.request_url/1` raise on the
+  # scrubbed conn. A scrubber that collapsed the URL to a bare host meant to
+  # redact the path, so that becomes "" rather than the original path.
+  defp scrubbed_path(conn, nil), do: conn.request_path
+  defp scrubbed_path(_conn, %URI{path: path}) when is_binary(path), do: path
+  defp scrubbed_path(_conn, %URI{}), do: ""
+
+  defp scrubbed_query(conn, nil), do: conn.query_string
+  defp scrubbed_query(_conn, %URI{query: query}) when is_binary(query), do: query
+  defp scrubbed_query(_conn, %URI{}), do: ""
+
+  # Splits a request path into `:path_info` segments the way Plug adapters do.
+  # Deliberately does not percent-decode: adapters store `path_info` encoded and
+  # `Plug.Router.Utils.decode_path_info!/1` decodes at match time.
+  defp split_path(path), do: for(segment <- String.split(path, "/"), segment != "", do: segment)
+
   # Scrubs a params-shaped value with the default sensitive keys, leaving
   # `%Plug.Conn.Unfetched{}` (and any non-plain-map) untouched. Shared by the
   # `:body` default clause and the `:params` strategy.

test/event_test.exs

@@ -161,6 +161,28 @@ defmodule Sentry.EventTest do
     assert vars["arg1"] =~ "fine"
   end
 
+  test "scrubs a credential in the request path from stacktrace frame vars" do
+    put_test_config(enable_source_code_context: false)
+
+    :ok =
+      Sentry.Scrubber.put_conn_scrubber(
+        url_scrubber: fn conn ->
+          conn |> Plug.Conn.request_url() |> String.replace("leaky-token", "redacted")
+        end
+      )
+
+    conn = %Plug.Conn{request_path: "/reset/leaky-token", path_info: ["reset", "leaky-token"]}
+    stack = [{SomeMod, :some_fun, [conn], [file: ~c"x.ex", line: 1]}]
+
+    exception = %FunctionClauseError{module: SomeMod, function: :some_fun, arity: 1}
+    event = Event.transform_exception(exception, stacktrace: stack)
+
+    %{vars: vars} = hd(hd(event.exception).stacktrace.frames)
+
+    refute vars["arg0"] =~ "leaky-token"
+    assert vars["arg0"] =~ "redacted"
+  end
+
   describe "create_event/1" do
     test "uses all the right defaults when called without options" do
       assert %Event{} = event = Event.create_event([])

test/plug_capture_test.exs

@@ -24,6 +24,7 @@ defmodule Sentry.PlugCaptureTest do
     get "/throw_route", PhoenixController, :throw
     get "/action_clause_error", PhoenixController, :action_clause_error
     get "/assigns_route", PhoenixController, :assigns
+    get "/reset_password/:token", PhoenixController, :action_clause_error
   end
 
   defmodule PhoenixEndpoint do
@@ -72,6 +73,30 @@ defmodule Sentry.PlugCaptureTest do
     plug PhoenixRouter
   end
 
+  defmodule PathUrlScrubber do
+    def scrub_url(conn) do
+      conn
+      |> Plug.Conn.request_url()
+      |> Sentry.Scrubber.scrub_url(keys: ["token"])
+      |> String.replace(
+        ~r{/reset_password/[^/?]+},
+        "/reset_password/#{Sentry.Scrubber.scrubbed_value()}"
+      )
+    end
+  end
+
+  defmodule PhoenixEndpointWithUrlScrubber do
+    use Sentry.PlugCapture
+    use Phoenix.Endpoint, otp_app: :sentry
+    use Plug.Debugger, otp_app: :sentry
+
+    json_mod = if Code.ensure_loaded?(JSON), do: JSON, else: Jason
+
+    plug Plug.Parsers, parsers: [:json], pass: ["*/*"], json_decoder: json_mod
+    plug Sentry.PlugContext, url_scrubber: {PathUrlScrubber, :scrub_url}
+    plug PhoenixRouter
+  end
+
   setup do
     SentryTest.setup_sentry()
   end
@@ -199,16 +224,9 @@ defmodule Sentry.PlugCaptureTest do
       assert exception.type == "Phoenix.ActionClauseError"
       assert exception.value =~ ~s(params: %{"password" => "*********"})
 
-      # conn.query_string must be scrubbed too. The "query_string:" prefix isolates
-      # this from the conn's query_params map, which is not broadened into the
-      # scrubbed fields on this branch.
       refute exception.value =~ ~s(query_string: "password=secret"),
              "query_string leaked into exception value: #{exception.value}"
 
-      # The action's second argument is the raw params map, a separate arg from
-      # the conn. It must be scrubbed too. Isolate the "# 2" argument block so
-      # this assertion is not confounded by the conn's query_params, which is not
-      # broadened into the scrubbed fields on this branch.
       assert [_arg1, arg2] = String.split(exception.value, ~r/#\s*2\s*\n/, parts: 2)
       refute arg2 =~ "secret", "non-conn params arg leaked into exception value: #{arg2}"
     end
@@ -297,9 +315,6 @@ defmodule Sentry.PlugCaptureTest do
 
       assert [exception] = event.exception
 
-      # Isolate the action's second argument (the raw params map). The conn's own
-      # query_params is not broadened into the scrubbed fields on this branch, so
-      # a global assertion would be confounded by it.
       assert [_arg1, arg2] = String.split(exception.value, ~r/#\s*2\s*\n/, parts: 2)
 
       assert arg2 =~ ~s("scrubbed_by" => "custom_body_scrubber"),
@@ -360,6 +375,48 @@ defmodule Sentry.PlugCaptureTest do
     end
   end
 
+  describe "credentials in a conn captured into stacktrace frame vars" do
+    @describetag :capture_log
+
+    @token "SEKRIT-TOKEN-VALUE"
+    @redacted Sentry.Scrubber.scrubbed_value()
+    @encoded_redacted URI.encode_www_form(Sentry.Scrubber.scrubbed_value())
+
+    setup %{bypass: bypass} do
+      Application.put_env(:sentry, PhoenixEndpointWithUrlScrubber,
+        render_errors: [view: Sentry.ErrorView, accepts: ~w(html)]
+      )
+
+      pid = start_supervised!(PhoenixEndpointWithUrlScrubber)
+      Process.link(pid)
+
+      %{ref: SentryTest.setup_bypass_envelope_collector(bypass, type: "event")}
+    end
+
+    test "redacts a path segment the url scrubber redacts", %{ref: ref} do
+      assert_raise Phoenix.ActionClauseError, fn ->
+        conn(:get, "/reset_password/#{@token}")
+        |> Plug.run([{PhoenixEndpointWithUrlScrubber, []}])
+      end
+
+      assert [%{"exception" => [%{"value" => value}]}] = SentryTest.collect_sentry_events(ref, 1)
+
+      assert value =~ ~s(request_path: "/reset_password/#{@redacted}")
+      assert value =~ ~s(path_info: ["reset_password", "#{@redacted}"])
+    end
+
+    test "redacts a query parameter the url scrubber redacts", %{ref: ref} do
+      assert_raise Phoenix.ActionClauseError, fn ->
+        conn(:get, "/reset_password/whatever?token=#{@token}")
+        |> Plug.run([{PhoenixEndpointWithUrlScrubber, []}])
+      end
+
+      assert [%{"exception" => [%{"value" => value}]}] = SentryTest.collect_sentry_events(ref, 1)
+
+      assert value =~ ~s(query_string: "token=#{@encoded_redacted}")
+    end
+  end
+
   defp call_plug_app(conn), do: Plug.run(conn, [{Sentry.ExamplePlugApplication, []}])
 
   defp call_phoenix_endpoint(conn), do: Plug.run(conn, [{PhoenixEndpoint, []}])