From 8a9b84ed0e91748f6053c2be4c0dc61e2995d828 Mon Sep 17 00:00:00 2001 From: Peter Solnica Date: Wed, 9 Sep 2026 08:14:53 +0000 Subject: [PATCH] fix(scrubbing): leave query params the SDK does not redact untouched Co-Authored-By: GPT-5 --- lib/sentry/scrubber.ex | 40 ++++++++++++++++++++++------- test/plug_capture_test.exs | 3 +-- test/sentry/live_view_hook_test.exs | 4 +-- test/sentry/plug_context_test.exs | 16 +++++------- test/sentry/scrubber_test.exs | 35 +++++++++++++++++++++++++ 5 files changed, 75 insertions(+), 23 deletions(-) diff --git a/lib/sentry/scrubber.ex b/lib/sentry/scrubber.ex index 594b7839..673a90fd 100644 --- a/lib/sentry/scrubber.ex +++ b/lib/sentry/scrubber.ex @@ -332,15 +332,37 @@ defmodule Sentry.Scrubber do keys = param_keys(opts) query - |> URI.query_decoder() - |> Enum.map(fn {key, value} -> - cond do - sensitive_key?(key, keys) -> {key, @scrubbed_value} - is_binary(value) and value =~ credit_card_regex() -> {key, @scrubbed_value} - true -> {key, value} - end - end) - |> URI.encode_query() + |> String.split("&") + |> Enum.map_join("&", &scrub_query_pair(&1, keys)) + end + + # Only a redacted pair is rewritten. Everything the SDK keeps is passed + # through exactly as it arrived, so the reported query string still matches + # what the client sent rather than a re-encoding of it. + defp scrub_query_pair(pair, keys) do + case String.split(pair, "=", parts: 2) do + [raw_key, raw_value] -> + if redact_param?(decode_www_form(raw_key), decode_www_form(raw_value), keys) do + raw_key <> "=" <> @scrubbed_value + else + pair + end + + [_without_value] -> + pair + end + end + + defp redact_param?(key, value, keys) do + sensitive_key?(key, keys) or value =~ credit_card_regex() + end + + # Scrubbing runs while an error is already being reported, so malformed + # percent-encoding must not raise. The raw form is still fine to match on. + defp decode_www_form(value) do + URI.decode_www_form(value) + rescue + ArgumentError -> value end @doc """ diff --git a/test/plug_capture_test.exs b/test/plug_capture_test.exs index b538054f..5078a72f 100644 --- a/test/plug_capture_test.exs +++ b/test/plug_capture_test.exs @@ -382,7 +382,6 @@ defmodule Sentry.PlugCaptureTest do @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, @@ -420,7 +419,7 @@ defmodule Sentry.PlugCaptureTest do assert [%{"exception" => [%{"value" => value}]}] = SentryTest.collect_sentry_events(ref, 1) - assert value =~ ~s(query_string: "token=#{@encoded_redacted}") + assert value =~ ~s(query_string: "token=#{@redacted}") end test "redacts a route parameter named like a credential", %{ref: ref} do diff --git a/test/sentry/live_view_hook_test.exs b/test/sentry/live_view_hook_test.exs index 0c5995a6..7f6c6297 100644 --- a/test/sentry/live_view_hook_test.exs +++ b/test/sentry/live_view_hook_test.exs @@ -260,11 +260,11 @@ defmodule Sentry.LiveViewHookTest do params_breadcrumb = Enum.find(context.breadcrumbs, &(&1.category == "web.live_view.params")) refute params_breadcrumb.data.uri =~ "supersecret" - assert params_breadcrumb.data.uri =~ "password=%2A%2A%2A%2A%2A%2A%2A%2A%2A" + assert params_breadcrumb.data.uri =~ "password=#{Sentry.Scrubber.scrubbed_value()}" assert params_breadcrumb.data.uri =~ "visible=ok" refute context.request.url =~ "supersecret" - assert context.request.url =~ "password=%2A%2A%2A%2A%2A%2A%2A%2A%2A" + assert context.request.url =~ "password=#{Sentry.Scrubber.scrubbed_value()}" end test "raises ArgumentError when :scrubber is not an MFA tuple" do diff --git a/test/sentry/plug_context_test.exs b/test/sentry/plug_context_test.exs index 8847257b..232eb283 100644 --- a/test/sentry/plug_context_test.exs +++ b/test/sentry/plug_context_test.exs @@ -120,7 +120,7 @@ defmodule Sentry.PlugContextTest do conn = conn(:get, "/test?password=hunter2") call(conn, []) - assert "http://www.example.com/test?password=#{encoded_scrubbed_value()}" == + assert "http://www.example.com/test?password=#{Sentry.Scrubber.scrubbed_value()}" == Sentry.Context.get_all().request.url end @@ -128,7 +128,7 @@ defmodule Sentry.PlugContextTest do conn = conn(:get, "/test?password=hunter2&hello=world") call(conn, []) - assert "password=#{encoded_scrubbed_value()}&hello=world" == + assert "password=#{Sentry.Scrubber.scrubbed_value()}&hello=world" == Sentry.Context.get_all().request.query_string end @@ -143,10 +143,10 @@ defmodule Sentry.PlugContextTest do conn = conn(:get, "/test?password=hunter2&hello=world") call(conn, url_scrubber: fn _conn -> raise "custom scrubber bug" end) - assert "http://www.example.com/test?password=#{encoded_scrubbed_value()}&hello=world" == + assert "http://www.example.com/test?password=#{Sentry.Scrubber.scrubbed_value()}&hello=world" == Sentry.Context.get_all().request.url - assert "password=#{encoded_scrubbed_value()}&hello=world" == + assert "password=#{Sentry.Scrubber.scrubbed_value()}&hello=world" == Sentry.Context.get_all().request.query_string end @@ -154,10 +154,10 @@ defmodule Sentry.PlugContextTest do conn = conn(:get, "/test?password=hunter2&hello=world") call(conn, url_scrubber: fn _conn -> %{unexpected: "value"} end) - assert "http://www.example.com/test?password=#{encoded_scrubbed_value()}&hello=world" == + assert "http://www.example.com/test?password=#{Sentry.Scrubber.scrubbed_value()}&hello=world" == Sentry.Context.get_all().request.url - assert "password=#{encoded_scrubbed_value()}&hello=world" == + assert "password=#{Sentry.Scrubber.scrubbed_value()}&hello=world" == Sentry.Context.get_all().request.query_string end @@ -296,8 +296,4 @@ defmodule Sentry.PlugContextTest do defp call(conn, opts) do Plug.run(conn, [{Sentry.PlugContext, opts}]) end - - defp encoded_scrubbed_value do - URI.encode_www_form(Sentry.Scrubber.scrubbed_value()) - end end diff --git a/test/sentry/scrubber_test.exs b/test/sentry/scrubber_test.exs index 7a68cc79..66ba443f 100644 --- a/test/sentry/scrubber_test.exs +++ b/test/sentry/scrubber_test.exs @@ -117,6 +117,13 @@ defmodule Sentry.ScrubberTest do assert Scrubber.scrub_url("http://example.com/foo") == "http://example.com/foo" end + test "leaves everything it does not redact exactly as it arrived" do + url = "http://example.com/reset%20me?password=x&keep=hello%20there&flag" + + assert Scrubber.scrub_url(url) == + "http://example.com/reset%20me?password=#{Scrubber.scrubbed_value()}&keep=hello%20there&flag" + end + test "preserves scheme, host, port, and path" do scrubbed = Scrubber.scrub_url("https://example.com:8443/p?secret=x") assert scrubbed =~ "https://example.com:8443/p?" @@ -175,6 +182,34 @@ defmodule Sentry.ScrubberTest do refute scrubbed =~ "hunter2" assert scrubbed =~ "keep=ok" end + + test "leaves the placeholder readable rather than percent-encoding it" do + scrubbed = Scrubber.scrub_query_string("password=hunter2") + + assert scrubbed == "password=#{Scrubber.scrubbed_value()}" + refute scrubbed =~ "%2A" + end + + test "passes through params it keeps byte for byte" do + assert Scrubber.scrub_query_string("greeting=hello%20there") == "greeting=hello%20there" + assert Scrubber.scrub_query_string("greeting=hello+there") == "greeting=hello+there" + assert Scrubber.scrub_query_string("a=1&&b=2") == "a=1&&b=2" + assert Scrubber.scrub_query_string("flag&visible=ok") == "flag&visible=ok" + end + + test "keeps the key as it was sent when redacting its value" do + assert Scrubber.scrub_query_string("Reset%2DToken=abc") == + "Reset%2DToken=#{Scrubber.scrubbed_value()}" + end + + test "does not raise on malformed percent-encoding" do + assert Scrubber.scrub_query_string("a=%ZZ") == "a=%ZZ" + end + + test "still redacts credit-card-shaped values" do + assert Scrubber.scrub_query_string("card=4111+1111+1111+1111&keep=a+b") == + "card=#{Scrubber.scrubbed_value()}&keep=a+b" + end end describe "scrub/1 with no registered scrubber" do