Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some concerns about PII: send_default_pii not being completely used, silent update on transactions capture with potential PII #2438

Closed
pbernery opened this issue Oct 18, 2024 · 6 comments · Fixed by #2443
Assignees

Comments

@pbernery
Copy link

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

@sl0thentr0py
Copy link
Member

@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!

@sl0thentr0py
Copy link
Member

sl0thentr0py commented Oct 22, 2024

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

@pbernery
Copy link
Author

pbernery commented Dec 10, 2024

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.

@sl0thentr0py
Copy link
Member

sl0thentr0py commented Dec 10, 2024

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?

@pbernery
Copy link
Author

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.

@sl0thentr0py
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants