I agree that PII can be leaked in this way and completely agree that there need to be paranoid defaults.

But I don't think it should be the only option. This is the Ruby/Rails SDK and Rails devs have a good understanding of REST, CRUD and HTTP verbs and most wouldn't put anything sensitive or non-stateless in GETs. Our company code reviews every PR, we use automated security linters, have independent security audits, and a lawyer written privacy policy.

Reset tokens are a valid exception, but they're short lived and expire once used so not a huge threat. Nonetheless since sentry-ruby already has various integrations and defaults it could strip out Devise and Clearance stuff automatically to be safe. I realise it's like whack-a-mole and you can't cover every library, but as it would be an opt-in choice so I think that's ok.

The name send_default_pii is quite unclear to me too. What does default include, or not include? What does PII stand for? Does personally identifiable information mean just personal stuff like phone numbers that would violate GDPR but wouldn't be an issue in all countries, or for B2B companies, or companies with a comprehensive privacy policy? Or does PII mean security information like passwords, one time tokens, credit cards? Really it's both but the name "personal information" really understates it and doesn't convey the gravity of what would be leaked if you disabled it (or rather turn it on).

The bigger issue I have with this setting is you effectively only offer 2 modes:

false (default): send nothing and Sentry is practically useless; a glorified stacktrace.
true: leak all possible sensitive data to Sentry.

Sentry really is close to useless without parameters. How am I supposed to debug anything without IDs? I've got an undefined method on nil error so I'm left guessing whether the database record really is missing, or is our code just broken? I can't look up the record because I don't have the ID. And I can't even replicate because, well, I've got no IDs.

I have seen the scrubbing and filtering docs. The first example in both is filtering out a ZeroDivisionError which is something nobody would ever ask how to do. The params |event, hint| aren't explained so I'll have to go digging with puts and binding.pry to figure out what they are. The other scrubbing examples all say "The platform or SDK you've selected either does not support this functionality, or the code sample is missing from our documentation" and they're about adding extra information not scrubbing it anyway. The filtering docs also don't have any scrubbing examples. So no actual docs on scrubbing despite the page title. Ok fine I'll look in sentry-ruby/examples... nope no examples at all using config.before_send. But surely the sentry-rails/example has it? No! In fact it actually sets send_default_pii to true! Is that what you're recommending? Crazy. The migration guide just says "for scrubbing sensitive data, please use Sentry's Advanced Data Scrubbing feature" which is server side scrubbing but we really don't want to be sending the data in the first place (that guide also needs updating to mention GET params now being included under send_default_pii). The gem doesn't even have rubydoc API documentation. So I'm left trawling through the gem's code to figure out what to do.

Unless I'm missing something (I hope I am) the options you're giving are:

Move back to Rollbar because while their UX is terrible at least it does something useful.
Turn on send_default_pii which is what your example code does and I bet what most of your users end up doing out of confusion and desperation.
Spend hours writing a custom before_send without any documentation on how to actually do that, and no defaults or examples to start from, as a non security expert who's likely to make mistakes.

Short term please at least provide a code example of how to write a sensibly strict before_send that scrubs most things. I can find some guidance in the code itself but it's far too brute force. I assume it'll need to:

Remove headers except for those in an allowlist (is there a safe default list provided anywhere in the SDK or Rails itself?)
Remove params in a blocklist (again is there one built in?), and perhaps remove any that match patterns like email addresses just in case.
Remove request body entirely from POST, PUT, DELETE etc.
Does the local variables feature need scrubbing separately? How?
Do breadcrumbs need scrubbing separately? I'm not clear on what they might even contain or how to test that.

And then there's the risk that if the SDK's API changes, or the way Rails handles things changes, the custom before_send may silently stop scrubbing sensitive information and we'd never know about it. So I guess we have to write a comprehensive test suite too. Whereas if you maintained a small number of sensible presets then Sentry and the community could keep them up to date and every customer would benefit.

We use Sentry as a hosted solution because historically we've been blown away by your top notch UX, design, and customer support. I hope you can find a better long term solution that gets Sentry back to that standard.

Uh oh!

Uh oh!

send_default_pii should also remove query params #1301

Description

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions