Some concerns about PII: send_default_pii not being completely used, silent update on transactions capture with potential PII
Closed this issue · 6 comments
Issue Description
I report some concerns about PII, after discovering some data on Sentry that I didn't know was captured.
I enabled Sentry Transaction/Performance 2 years ago. At this time, only the controller action were reported if I am not mistaken.
Some month ago, I upgraded from 5.7.0 to 5.16.1. I usually read the changelog to check for any important changes, particularly around PII.
I discovered some weeks ago that the params of the controller actions are now captured. Some investigation on my side showed that this is coming from #1973, which was reported as "Fix sentry-rails' controller span nesting", without mentioning this update on capture if I am not mistaken. This PR was introduced in 5.8.0.
Also, the send_default_pii option (https://docs.sentry.io/platforms/ruby/configuration/options/#send-default-pii) is presented as is:
When its value is false (the default), sensitive information like:
- user ip
- user cookie
- request body
- query string in the url
won't be sent to Sentry.
which is not true here, as params is the body of the request. I expect this option to hide params in transactions, and probably in other parts.
What's your point of view?
Did I miss something in the release notes or any other news channel?
Reproduction Steps
- upgrade from 5.7.0 to 5.16.1,
- params are added as span tags,
- content is displayed on Sentry interface.
Expected Behavior
- changelog mentioning any changes that may impact PII or any other data captured,
- see these changes as breaking changes, and thus upgrading the version accordingly.
Actual Behavior
- changelog not mentioning this critical change in captured data,
- version not increased accordingly.
Ruby Version
3.3.4
SDK Version
5.21.0
Integration and Its Version
No response
Sentry Config
No response
@pbernery thanks for the report, you're right that this was an oversight on our part.
Will gate params behind config.send_default_pii.
Apologies for the inconvenience!
okay I see that rails was using filtered_parameters and filtered_path before, so I will fall back to that behavior when pii is off, and send the raw ones if pii is on.
https://github.com/rails/rails/blob/dfd1e951aa1aeef06c39fffb2994db8a8fa1914f/actionpack/lib/action_controller/metal/instrumentation.rb#L66
I believe this line of code has the same issue: it will leak the whole payload, for instance the whole body of a POST, to Sentry while send_default_pii is set to false.
And I don't know much ActiveStorage, but maybe this line could cause the same issue.
I believe this line of code has the same issue:
That subscriber is deprecated and will be removed in the next major. It is no longer used within the SDK.
The active storage link is pointing to the same as above. Can you fix the link or explain what would leak in the active storage case?
I believe this line of code has the same issue:
That subscriber is deprecated and will be removed in the next major. It is no longer used within the SDK.
But it is still pushing data to Sentry, at least in my version (5.21.0), seen in the "Trace Details" section of an issue on Sentry Interface.
It was expected to me that a trace is filtered using before_send_transaction, but from my understanding before_send must also be implemented to ensure that a issue won't leak identifying data, to filter the same content.
The active storage link is pointing to the same as above. Can you fix the link or explain what would leak in the active storage case?
Oops, I've updated the link in my original message.
But it is still pushing data to Sentry, at least in my version (5.21.0), seen in the "Trace Details" section of an issue on Sentry Interface.
We're not using that older subscriber anymore. Can you link me to an event in Sentry where you see the whole payload? If you don't want to paste it here publicly on github, you can send it to me via email on neel dot shah at sentry.io.
Regarding active storage, I'm opening a new issue to track.