fix(scrubbing): leave query params the SDK does not redact untouched - #1200
Conversation
e392227 to
ed80cfe
Compare
| end | ||
| end) | ||
| |> URI.encode_query() | ||
| |> String.split("&") |
There was a problem hiding this comment.
Are you not using URI.decode_query/1 here (and below) because it'd lose ordering?
There was a problem hiding this comment.
Yes we want to preserve original state, order included, and also prevent cases like ending up with ********* placeholder being converted to %2A%2A%2A%2A%2A%2A%2A%2A%2A.
| defp decode_www_form(value) do | ||
| URI.decode_www_form(value) | ||
| rescue | ||
| ArgumentError -> value |
There was a problem hiding this comment.
When does this raise?
There was a problem hiding this comment.
Good question - in my testing nowhere, but BugBot pointed out that if it did, we'd have an unwanted crash. It is very defensive, I have to admit - do you think it's 100% safe to remove that rescue?
whatyouhide
left a comment
whatyouhide
left a comment
There was a problem hiding this comment.
Just some comments here.
ed80cfe to
a22eaab
Compare
Co-Authored-By: GPT-5 <noreply@anthropic.com>
a22eaab to
8a9b84e
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| 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 |
There was a problem hiding this comment.
Bug: The scrub_query_string implementation calls String.downcase on keys that may be non-UTF-8 binaries, causing a UnicodeConversionError and crashing the scrubber.
Severity: HIGH
Suggested Fix
The call to String.downcase within sensitive_key? should be wrapped in a try/rescue block to handle potential UnicodeConversionError. When an error is caught, the function should treat the key as a non-sensitive binary and proceed without attempting to downcase it, preventing the crash.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: lib/sentry/scrubber.ex#L345
Potential issue: The `scrub_query_string` function processes query strings by decoding
them with `decode_www_form`. If a key contains percent-encoded non-UTF-8 bytes (e.g.,
`%FF%FE`), `URI.decode_www_form` produces a non-UTF-8 binary. This binary is then passed
to `sensitive_key?`, which calls `String.downcase`. `String.downcase` raises a
`UnicodeConversionError` on non-UTF-8 input. The existing `rescue` block only catches
`ArgumentError`, so the `UnicodeConversionError` is unhandled and will crash the
scrubber process. This will cause a test at `test/sentry/scrubber_test.exs:176` to fail.
Did we get this right? 👍 / 👎 to inform future reviews.
3 participants
Now
scrub_query_string/2rewrites a parameter only when it redacts its value, so we won't end up with%2A, that would violate DC specs.Before
query_string— the one redacted parameter readspassword=%2A%2A%2A%2A%2A%2A%2A%2A%2AAfter
query_string— the placeholder intact and everything the SDK keeps passed through