Add url_scrubber for redacting URLs #687
Conversation
lib/sentry/plug_context.ex
Outdated
| @@ -152,7 +175,7 @@ defmodule Sentry.PlugContext do | |||
| |> Plug.Conn.fetch_query_params() | |||
|
|
|||
| %{ | |||
| url: Plug.Conn.request_url(conn), | |||
| url: apply_fun_with_conn(conn, url_scrubber), | |||
There was a problem hiding this comment.
Using apply_fun_with_conn is consistent with the other scrubbers. However, passing in nil for url_scrubber will cause %{} to be sent instead of Plug.Conn.request_url(conn).
Does this matter? Should apply_fun_with_conn get a new 3rd default argument that defaults to %{}?
defp apply_fun_with_conn(conn, function, default \\ %{})
defp apply_fun_with_conn(_conn, _function = nil, default), do: default
defp apply_fun_with_conn(conn, {module, fun}, _default), do: apply(module, fun, [conn])
defp apply_fun_with_conn(conn, fun, _default) when is_function(fun, 1), do: fun.(conn)url: apply_fun_with_conn(conn, url_scrubber, Plug.Conn.request_url(conn)),
There was a problem hiding this comment.
Yes let's go with a third argument, but without the \\ clause. Just do apply_fun_with_conn(conn, function, default) and explicitly pass in %{} in the other instances where we call apply_fun_with_conn/2 today.
There was a problem hiding this comment.
Will do! I like the explicitness.
|
|
||
| It can also be passed in as a `{module, fun}` like so: | ||
|
|
||
| plug Sentry.PlugContext, header_scrubber: {MyModule, :scrub_headers} | ||
| plug Sentry.PlugContext, header_scrubber: {MySentryScrubber, :scrub_headers} |
There was a problem hiding this comment.
It's an insignificant detail, but there was some inconsistency with what module names were used here in the docs. Since I was updating a new, related section and wanted to be consistent, I thought it felt clean to set them all to MySentryScrubber.
Happy to revert if this isn't preferred!
|
I think this test failure is a fluke. Cannot reproduce it on my machine. |
lib/sentry/plug_context.ex
Outdated
| @@ -152,7 +175,7 @@ defmodule Sentry.PlugContext do | |||
| |> Plug.Conn.fetch_query_params() | |||
|
|
|||
| %{ | |||
| url: Plug.Conn.request_url(conn), | |||
| url: apply_fun_with_conn(conn, url_scrubber), | |||
There was a problem hiding this comment.
Yes let's go with a third argument, but without the \\ clause. Just do apply_fun_with_conn(conn, function, default) and explicitly pass in %{} in the other instances where we call apply_fun_with_conn/2 today.
test/sentry/plug_context_test.exs
Outdated
| @@ -105,6 +109,13 @@ defmodule Sentry.PlugContextTest do | |||
| assert %{"not-secret" => "not-secret"} == Sentry.Context.get_all().request.cookies | |||
| end | |||
|
|
|||
| test "allows configuring url scrubber" do | |||
There was a problem hiding this comment.
Tiny but mighty 😉
| test "allows configuring url scrubber" do | |
| test "allows configuring URL scrubber" do |
| plug Sentry.PlugContext, cookie_scrubber: &MyCookieScrubber.scrub_cookies/1 | ||
| plug Sentry.PlugContext, cookie_scrubber: &MySentryScrubber.scrub_cookies/1 | ||
|
|
||
| ### Scrubbing URLs |
There was a problem hiding this comment.
We need to say that this has not always been available
| ### Scrubbing URLs | |
| ### Scrubbing URLs | |
| *Available since v10.2.0.* |
There was a problem hiding this comment.
Done! Thanks for the suggestion.
|
@whatyouhide Thank you for the feedback! Addressed all of it in de2fc94. CI is failing in the same way as before; I think this is a fluke, as I cannot reproduce locally. Please let me know if you think I need to adjust the changes in any way! |
|
@paulstatezny yeah CI is unrelated, no worries. Thank you! 💟 |
@whatyouhide indicated here that this PR would be welcome. 🙂
This PR adds support for a
url_scrubberwhich works just likebody_scrubber,header_scrubber, andcookie_scrubber.The "default scrubber" behavior is to not redact anything. (Just runs
Plug.Conn.request_url/1as before.)