From 1810f90490271c33c3ab852beb057e85940ab1c4 Mon Sep 17 00:00:00 2001 From: Peter Solnica Date: Mon, 7 Sep 2026 13:48:59 +0000 Subject: [PATCH] fix(scrubbing): apply the url scrubber to conn path and query fields (#1195) --- lib/sentry/plug_capture.ex | 4 ++ lib/sentry/plug_context.ex | 8 +++ lib/sentry/scrubber.ex | 111 ++++++++++++++++++++++++++++------ test/event_test.exs | 22 +++++++ test/plug_capture_test.exs | 77 ++++++++++++++++++++--- test/sentry/scrubber_test.exs | 38 +++++++++++- 6 files changed, 231 insertions(+), 29 deletions(-) diff --git a/lib/sentry/plug_capture.ex b/lib/sentry/plug_capture.ex index 89272401..f49b45c9 100644 --- a/lib/sentry/plug_capture.ex +++ b/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`); diff --git a/lib/sentry/plug_context.ex b/lib/sentry/plug_context.ex index 77b618b6..719d4b89 100644 --- a/lib/sentry/plug_context.ex +++ b/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* diff --git a/lib/sentry/scrubber.ex b/lib/sentry/scrubber.ex index 940b4e88..c4a9a3b7 100644 --- a/lib/sentry/scrubber.ex +++ b/lib/sentry/scrubber.ex @@ -50,17 +50,19 @@ 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 @@ -68,9 +70,15 @@ defmodule Sentry.Scrubber do 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. diff --git a/test/event_test.exs b/test/event_test.exs index e99925c3..9021d600 100644 --- a/test/event_test.exs +++ b/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([]) diff --git a/test/plug_capture_test.exs b/test/plug_capture_test.exs index 51434a88..67b45885 100644 --- a/test/plug_capture_test.exs +++ b/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, []}]) diff --git a/test/sentry/scrubber_test.exs b/test/sentry/scrubber_test.exs index 4e9db2c6..d63db206 100644 --- a/test/sentry/scrubber_test.exs +++ b/test/sentry/scrubber_test.exs @@ -203,7 +203,12 @@ defmodule Sentry.ScrubberTest do phoenix_endpoint: SomeApp.Endpoint, phoenix_controller: SomeApp.PageController }, + scheme: :https, + host: "example.com", + port: 443, request_path: "/users", + path_info: ["users"], + query_string: "page=2&secret=leak", method: "POST" } @@ -261,8 +266,17 @@ defmodule Sentry.ScrubberTest do refute Map.has_key?(scrubbed.private, :guardian_default_claims) end - test "preserves non-sensitive fields", %{scrubbed: scrubbed} do + test "leaves a request_path the url scrubber does not touch unchanged", %{scrubbed: scrubbed} do assert scrubbed.request_path == "/users" + assert scrubbed.path_info == ["users"] + end + + test "scrubs sensitive params out of query_string", %{scrubbed: scrubbed} do + refute scrubbed.query_string =~ "leak" + assert scrubbed.query_string =~ "page=2" + end + + test "preserves the request method", %{scrubbed: scrubbed} do assert scrubbed.method == "POST" end @@ -299,6 +313,28 @@ defmodule Sentry.ScrubberTest do assert scrubbed =~ "token=abc" assert scrubbed =~ "keep=ok" end + + test "keeps the conn's own path when the url scrubber returns a non-binary" do + :ok = Scrubber.put_conn_scrubber(url_scrubber: fn _conn -> :not_a_url end) + + conn = %Plug.Conn{request_path: "/users", path_info: ["users"]} + + scrubbed = Scrubber.scrub(conn) + + assert scrubbed.request_path == "/users" + assert scrubbed.path_info == ["users"] + end + + test "still scrubs sensitive query params when the url scrubber is disabled" do + :ok = Scrubber.put_conn_scrubber(url_scrubber: nil) + + conn = %Plug.Conn{query_string: "password=secret&keep=ok"} + + scrubbed = Scrubber.scrub(conn).query_string + + refute scrubbed =~ "secret" + assert scrubbed =~ "keep=ok" + end end describe "scrub/2 with conn field overrides" do