Skip to content

Commit b4e55c2

Browse files
solnicclaude
andauthored
feat(scrubbing): implement PII scrubbing for stacktrace args (#1068)
* fix(event): scrub sensitive data from stacktrace frame vars Frame vars in event payloads are built by `inspect/1`-ing each arg, so a Plug.Conn or plain map passed to a function that raises leaks its contents into the event - authorization headers, cookies, password params, etc. `Sentry.Event` now scrubs each frame arg with `Sentry.Scrubber.scrub/1` before inspecting it, redacting `Plug.Conn` and map values (honoring any registered conn scrubber) while leaving everything else untouched. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * test(plug): ensure custom scrubber is used for stacktraces * refa(plug): introduce stacktrace scrubber This unifies dealing with exceptions and scrubbing. Previously it would special-case function clause errors from phoenix which was not sufficient. Now we scrub plug conn consistently when reporting exceptions. * test(plug): cover generic FunctionClauseError arg scrubbing Add a Phoenix integration endpoint that fabricates a plain FunctionClauseError (not a Phoenix.ActionClauseError) and assert that its captured stacktrace frame vars are scrubbed by Sentry.Event via StacktraceScrubber, independently of PlugCapture's ActionClauseError handling. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(plug): fix tests changes in master surfaced this but it will be restored in the follow-up scrubb-broadening branch * fix(scrubber): handle atom struct keys too --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
1 parent f45e81f commit b4e55c2

13 files changed

Lines changed: 451 additions & 12 deletions

File tree

lib/sentry/event.ex

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -468,9 +468,12 @@ defmodule Sentry.Event do
468468
# String.slice/3 would, while keeping inspect from materializing huge terms.
469469
inspect_opts = [printable_limit: max_length, limit: max(div(max_length, 3), 1)]
470470

471-
for {arg, index} <- Enum.with_index(args), into: %{} do
471+
args
472+
|> Sentry.Scrubber.StacktraceScrubber.scrub_args()
473+
|> Enum.with_index()
474+
|> Map.new(fn {arg, index} ->
472475
{"arg#{index}", String.slice(inspect(arg, inspect_opts), 0, max_length)}
473-
end
476+
end)
474477
end
475478

476479
defp arity_to_integer(arity) when is_list(arity), do: Enum.count(arity)

lib/sentry/plug_capture.ex

Lines changed: 30 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -55,11 +55,17 @@ defmodule Sentry.PlugCapture do
5555
out if there are `Plug.Conn` structs to scrub. Right now, the strategy we
5656
use follows these steps:
5757
58-
1. if the error is `Phoenix.ActionClauseError`, we scrub the `Plug.Conn` structs
59-
from the `args` field of that exception
58+
1. if the error is `Phoenix.ActionClauseError`, we scrub the `Plug.Conn` in the
59+
`args` field of that exception, and mirror that conn's scrubbed params onto the
60+
action's standalone params argument so both are redacted consistently
61+
62+
Scrubbing goes through the same `Sentry.Scrubber` implementation as
63+
`Sentry.PlugContext`, so it honors the per-field scrubbers (`:body_scrubber`,
64+
`:header_scrubber`, `:cookie_scrubber`, `:url_scrubber`) configured on
65+
`Sentry.PlugContext` for the current request.
6066
6167
Otherwise, we don't perform any scrubbing. To configure scrubbing, you can use the
62-
`:scrubbing` option (see below).
68+
`:scrubber` option (see below).
6369
6470
## Options
6571
@@ -134,14 +140,17 @@ defmodule Sentry.PlugCapture do
134140

135141
@doc false
136142
def __capture_exception__(exception, stacktrace, scrubber) do
137-
# We can't pattern match here, because we're not guaranteed to have
138-
# Phoenix available.
143+
# `Phoenix.ActionClauseError` is the one error whose args we know the shape of —
144+
# a controller action is invoked as `apply(controller, action, [conn, conn.params])`.
145+
# We handle it explicitly: `StacktraceScrubber` does the generic per-arg scrubbing,
146+
# and we instruct it (via the callback) to scrub the conn through the configured
147+
# `:scrubber` and mirror the conn's scrubbed params onto the standalone params arg.
139148
exception =
140149
if is_struct(exception, Phoenix.ActionClauseError) do
141-
update_in(exception, [Access.key!(:args), Access.all()], fn
142-
conn when is_struct(conn, Plug.Conn) -> apply_scrubber(conn, scrubber)
143-
other -> other
144-
end)
150+
Sentry.Scrubber.StacktraceScrubber.scrub(
151+
exception,
152+
&scrub_action_clause_args(&1, scrubber)
153+
)
145154
else
146155
exception
147156
end
@@ -156,6 +165,18 @@ defmodule Sentry.PlugCapture do
156165
:ok
157166
end
158167

168+
defp scrub_action_clause_args(args, scrubber) do
169+
conn = Enum.find(args, &is_struct(&1, Plug.Conn))
170+
scrubbed_conn = apply_scrubber(conn, scrubber)
171+
params = conn.params
172+
173+
Enum.map(args, fn
174+
^conn -> scrubbed_conn
175+
^params -> scrubbed_conn.params
176+
other -> Sentry.Scrubber.scrub(other)
177+
end)
178+
end
179+
159180
@doc false
160181
def default_scrubber(conn), do: Sentry.Scrubber.scrub(conn)
161182

lib/sentry/scrubber.ex

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -413,7 +413,7 @@ defmodule Sentry.Scrubber do
413413
keys = Keyword.get(opts, :keys, @default_scrubbed_param_keys)
414414

415415
Map.new(map, fn {key, value} ->
416-
{key, if(key in keys, do: @scrubbed_value, else: scrub(value, opts))}
416+
{key, if(sensitive_key?(key, keys), do: @scrubbed_value, else: scrub(value, opts))}
417417
end)
418418
end
419419

@@ -496,5 +496,11 @@ defmodule Sentry.Scrubber do
496496

497497
defp normalize(_field, value), do: value
498498

499+
# Matches a map key against the configured sensitive-key list. The list is
500+
# string-based (HTTP params), but maps built from structs via `Map.from_struct/1`
501+
# have atom keys, so atoms are also compared by their string form.
502+
defp sensitive_key?(key, keys),
503+
do: key in keys or (is_atom(key) and Atom.to_string(key) in keys)
504+
499505
defp credit_card_regex, do: ~r/^(?:\d[ -]*?){13,16}$/
500506
end
Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
defmodule Sentry.Scrubber.StacktraceScrubber do
2+
@moduledoc false
3+
4+
# Scrubs args captured into error data — a stacktrace frame's args (see
5+
# `Sentry.Event`) or an exception's `:args` field (see `Sentry.PlugCapture`).
6+
#
7+
# A `%Plug.Conn{}` and any non-struct term are scrubbed with
8+
# `Sentry.Scrubber.scrub/1`. Any other struct has its fields scrubbed but keeps
9+
# its type: these args are rendered with `inspect/2`, so a scrubbed `%Mod{...}`
10+
# is far more useful in a frame var than the bare map `Sentry.Scrubber.scrub/1`
11+
# would produce (that map shape is right for the JSON request payload, but
12+
# garbles structs when inspected). This module is framework-agnostic and makes
13+
# no assumptions about the relationships between args; a caller that knows more
14+
# can pass its own args-scrubber callback to `scrub/2` — for example
15+
# `Sentry.PlugCapture`, which knows a `Phoenix.ActionClauseError` carries
16+
# `[conn, conn.params]` and mirrors the scrubbed conn's params onto the params arg.
17+
18+
@doc """
19+
Scrubs a list of args, redacting each element.
20+
21+
A `%Plug.Conn{}` and any non-struct term go through `Sentry.Scrubber.scrub/1`.
22+
Any other struct has its fields scrubbed while keeping its type, so it inspects
23+
as a scrubbed `%Mod{...}` rather than a bare map in the frame var.
24+
"""
25+
@spec scrub_args([term()]) :: [term()]
26+
def scrub_args(args) when is_list(args), do: Enum.map(args, &scrub_arg/1)
27+
28+
defp scrub_arg(conn) when is_struct(conn, Plug.Conn), do: Sentry.Scrubber.scrub(conn)
29+
30+
defp scrub_arg(struct) when is_struct(struct),
31+
do: struct(struct, struct |> Map.from_struct() |> Sentry.Scrubber.scrub())
32+
33+
defp scrub_arg(other), do: Sentry.Scrubber.scrub(other)
34+
35+
@doc """
36+
Scrubs an exception's `:args` and returns the updated exception.
37+
38+
`args_scrubber` is a 1-arity function applied to the exception's args list; it
39+
defaults to `scrub_args/1`. Callers that know the args' shape can pass a custom
40+
callback. Exceptions without a list `:args` field are returned unchanged.
41+
"""
42+
@spec scrub(Exception.t(), ([term()] -> [term()])) :: Exception.t()
43+
def scrub(exception, args_scrubber \\ &scrub_args/1) do
44+
case exception do
45+
%{args: args} when is_list(args) -> %{exception | args: args_scrubber.(args)}
46+
_ -> exception
47+
end
48+
end
49+
end

test/event_test.exs

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,39 @@ defmodule Sentry.EventTest do
128128
assert event.extra == %{extra_data: "data"}
129129
end
130130

131+
test "scrubs sensitive data from Plug.Conn and map args in stacktrace frame vars" do
132+
put_test_config(enable_source_code_context: false)
133+
134+
conn = %Plug.Conn{
135+
req_headers: [
136+
{"authorization", "Bearer leaky-token"},
137+
{"cookie", "session=leaky-cookie"},
138+
{"x-request-id", "ok-to-keep"}
139+
],
140+
cookies: %{"session" => "leaky-cookie"},
141+
params: %{"password" => "leaky-password", "name" => "Alice"}
142+
}
143+
144+
stack = [
145+
{SomeMod, :some_fun, [conn, %{"password" => "another-leak", "ok" => "fine"}],
146+
[file: ~c"x.ex", line: 1]}
147+
]
148+
149+
exception = %FunctionClauseError{module: SomeMod, function: :some_fun, arity: 2}
150+
event = Event.transform_exception(exception, stacktrace: stack)
151+
152+
%{vars: vars} = hd(hd(event.exception).stacktrace.frames)
153+
154+
refute vars["arg0"] =~ "leaky-token"
155+
refute vars["arg0"] =~ "leaky-cookie"
156+
refute vars["arg0"] =~ "leaky-password"
157+
assert vars["arg0"] =~ "x-request-id"
158+
assert vars["arg0"] =~ "Alice"
159+
160+
refute vars["arg1"] =~ "another-leak"
161+
assert vars["arg1"] =~ "fine"
162+
end
163+
131164
describe "create_event/1" do
132165
test "uses all the right defaults when called without options" do
133166
assert %Event{} = event = Event.create_event([])

test/plug_capture_test.exs

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -204,6 +204,13 @@ defmodule Sentry.PlugCaptureTest do
204204
# scrubbed fields on this branch.
205205
refute exception.value =~ ~s(query_string: "password=secret"),
206206
"query_string leaked into exception value: #{exception.value}"
207+
208+
# The action's second argument is the raw params map, a separate arg from
209+
# the conn. It must be scrubbed too. Isolate the "# 2" argument block so
210+
# this assertion is not confounded by the conn's query_params, which is not
211+
# broadened into the scrubbed fields on this branch.
212+
assert [_arg1, arg2] = String.split(exception.value, ~r/#\s*2\s*\n/, parts: 2)
213+
refute arg2 =~ "secret", "non-conn params arg leaked into exception value: #{arg2}"
207214
end
208215

209216
test "scrubs Phoenix.ActionClauseError using PlugContext-configured body_scrubber" do
@@ -236,6 +243,40 @@ defmodule Sentry.PlugCaptureTest do
236243
"""
237244
end
238245

246+
test "applies the PlugContext body_scrubber to the non-conn params arg too" do
247+
Application.put_env(:sentry, PhoenixEndpointWithCustomPlugContext,
248+
render_errors: [view: Sentry.ErrorView, accepts: ~w(html)]
249+
)
250+
251+
pid = start_supervised!(PhoenixEndpointWithCustomPlugContext)
252+
Process.link(pid)
253+
254+
# "ssn" is not a default-sensitive key, so only the user-configured
255+
# body_scrubber (which replaces the params wholesale) hides it. Scrubbing
256+
# the standalone params arg with the default key list would leak it.
257+
assert_raise Phoenix.ActionClauseError, fn ->
258+
conn(:get, "/action_clause_error?ssn=123-45-6789")
259+
|> Plug.run([{PhoenixEndpointWithCustomPlugContext, []}])
260+
end
261+
262+
event =
263+
assert_sentry_report(:event,
264+
culprit: "Sentry.PlugCaptureTest.PhoenixController.action_clause_error/2"
265+
)
266+
267+
assert [exception] = event.exception
268+
269+
# Isolate the action's second argument (the raw params map). The conn's own
270+
# query_params is not broadened into the scrubbed fields on this branch, so
271+
# a global assertion would be confounded by it.
272+
assert [_arg1, arg2] = String.split(exception.value, ~r/#\s*2\s*\n/, parts: 2)
273+
274+
assert arg2 =~ ~s("scrubbed_by" => "custom_body_scrubber"),
275+
"expected the non-conn params arg to honor the configured body_scrubber, got: #{arg2}"
276+
277+
refute arg2 =~ "123-45-6789", "ssn leaked through the non-conn params arg: #{arg2}"
278+
end
279+
239280
test "can render feedback form in Phoenix ErrorView" do
240281
conn = conn(:get, "/error_route")
241282

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,90 @@
1+
defmodule Sentry.Scrubber.StacktraceScrubberTest.Card do
2+
@moduledoc false
3+
defstruct [:card_number, :name, :secret]
4+
end
5+
6+
defmodule Sentry.Scrubber.StacktraceScrubberTest do
7+
use ExUnit.Case, async: true
8+
9+
alias Sentry.Scrubber.StacktraceScrubber
10+
alias Sentry.Scrubber.StacktraceScrubberTest.Card
11+
12+
describe "scrub_args/1" do
13+
test "scrubs each arg with Sentry.Scrubber.scrub/1" do
14+
conn = %Plug.Conn{
15+
req_headers: [{"authorization", "Bearer secret"}, {"x-keep", "yes"}],
16+
params: %{"password" => "secret", "name" => "Alice"}
17+
}
18+
19+
args = [conn, %{"password" => "another", "ok" => "fine"}, "plain", 42]
20+
21+
assert [scrubbed_conn, scrubbed_map, "plain", 42] = StacktraceScrubber.scrub_args(args)
22+
23+
# the conn is scrubbed as a conn
24+
assert scrubbed_conn.params == %{"password" => "*********", "name" => "Alice"}
25+
assert scrubbed_conn.req_headers == [{"x-keep", "yes"}]
26+
27+
# a plain map is key-scrubbed
28+
assert scrubbed_map == %{"password" => "*********", "ok" => "fine"}
29+
end
30+
31+
test "scrubs a non-Plug.Conn struct's fields but keeps its type" do
32+
args = [%Card{card_number: "4242424242424242", name: "Alice", secret: "top-secret"}]
33+
34+
assert [scrubbed] = StacktraceScrubber.scrub_args(args)
35+
36+
# The struct keeps its type (so it inspects as %Card{...} in the frame var,
37+
# not a bare map)...
38+
assert is_struct(scrubbed, Card)
39+
# ...while its fields are scrubbed by value (credit-card heuristic) and by
40+
# name (the atom key :secret matches the sensitive-key list).
41+
assert scrubbed.card_number == "*********"
42+
assert scrubbed.secret == "*********"
43+
assert scrubbed.name == "Alice"
44+
end
45+
46+
test "scrubs each arg independently, with no conn/params mirroring" do
47+
# A registered body_scrubber only governs the conn's params field; the standalone
48+
# params arg is scrubbed independently with the default keys (no mirror).
49+
Sentry.Scrubber.put_conn_scrubber(body_scrubber: fn _conn -> %{"marker" => "scrubbed"} end)
50+
51+
conn = %Plug.Conn{params: %{"password" => "secret", "ssn" => "123-45-6789"}}
52+
args = [conn, conn.params]
53+
54+
assert [scrubbed_conn, scrubbed_params] = StacktraceScrubber.scrub_args(args)
55+
56+
# conn's params field goes through the registered body_scrubber
57+
assert scrubbed_conn.params == %{"marker" => "scrubbed"}
58+
59+
# the standalone params arg is scrubbed independently (default keys only): the
60+
# "password" value is redacted, but "ssn" (not a default key) is left intact —
61+
# proving the conn's scrubbed params are NOT mirrored onto it.
62+
assert scrubbed_params == %{"password" => "*********", "ssn" => "123-45-6789"}
63+
end
64+
end
65+
66+
describe "scrub/2" do
67+
test "scrubs an exception's :args with the default per-arg scrubber" do
68+
conn = %Plug.Conn{params: %{"password" => "secret", "name" => "Alice"}}
69+
exception = %FunctionClauseError{module: Foo, function: :bar, arity: 2, args: [conn, "x"]}
70+
71+
assert %FunctionClauseError{args: [scrubbed_conn, "x"]} =
72+
StacktraceScrubber.scrub(exception)
73+
74+
assert scrubbed_conn.params == %{"password" => "*********", "name" => "Alice"}
75+
end
76+
77+
test "applies a custom args_scrubber callback to the exception's args" do
78+
exception = %FunctionClauseError{module: Foo, function: :bar, arity: 2, args: [1, 2, 3]}
79+
80+
assert %FunctionClauseError{args: [2, 4, 6]} =
81+
StacktraceScrubber.scrub(exception, fn args -> Enum.map(args, &(&1 * 2)) end)
82+
end
83+
84+
test "leaves an exception without a list :args field unchanged" do
85+
exception = %RuntimeError{message: "boom"}
86+
87+
assert StacktraceScrubber.scrub(exception) == exception
88+
end
89+
end
90+
end

test/sentry/scrubber_test.exs

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,11 @@ defmodule Sentry.ScrubberTest do
4242
%{"password" => "*********", "ok" => 1}
4343
end
4444

45+
test "redacts sensitive keys given as atoms (e.g. struct fields)" do
46+
assert Scrubber.scrub(%{password: "x", ok: 1}) ==
47+
%{password: "*********", ok: 1}
48+
end
49+
4550
test "recurses into nested maps" do
4651
assert Scrubber.scrub(%{"outer" => %{"secret" => "shh"}}) ==
4752
%{"outer" => %{"secret" => "*********"}}
Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
defmodule PhoenixApp.Billing do
2+
@moduledoc """
3+
A small payments context, standing in for the kind of billing code a real app
4+
has. `charge/3` is guarded by the set of currencies the processor supports.
5+
"""
6+
7+
alias PhoenixApp.Billing.CreditCard
8+
9+
@supported_currencies ~w(USD EUR GBP)
10+
11+
@doc """
12+
Charges a card for `amount` (in minor units) in a supported currency.
13+
14+
An unsupported currency matches no clause and raises `FunctionClauseError`. The
15+
`%CreditCard{}` passed in then rides along in that frame's stacktrace args.
16+
"""
17+
@spec charge(CreditCard.t(), pos_integer(), String.t()) :: {:ok, map()}
18+
def charge(%CreditCard{} = card, amount, currency)
19+
when currency in @supported_currencies do
20+
{:ok, %{card: card, amount: amount, currency: currency}}
21+
end
22+
end
Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
defmodule PhoenixApp.Billing.CreditCard do
2+
@moduledoc """
3+
A plain value struct for an in-flight card payment.
4+
5+
It is deliberately a plain struct, not an `Ecto.Schema`, and uses no `redact:`
6+
option — which is typical for ad-hoc value objects that are never persisted
7+
(storing a raw PAN would be a PCI violation). As a result its default `Inspect`
8+
implementation renders every field, including the card number.
9+
"""
10+
11+
@type t :: %__MODULE__{
12+
cardholder: String.t() | nil,
13+
number: String.t() | nil,
14+
cvv: String.t() | nil
15+
}
16+
17+
defstruct [:cardholder, :number, :cvv]
18+
end

0 commit comments

Comments
 (0)