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

install sentry #1982

Merged
merged 1 commit into from
Sep 10, 2023
Merged

install sentry #1982

merged 1 commit into from
Sep 10, 2023

Conversation

andrewfader
Copy link
Collaborator

This will enable us to track errors for free.

@codecov
Copy link

codecov bot commented Jun 20, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (2431a2a) 98.03% compared to head (461a4d6) 98.03%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1982   +/-   ##
=======================================
  Coverage   98.03%   98.03%           
=======================================
  Files          53       53           
  Lines        2090     2090           
=======================================
  Hits         2049     2049           
  Misses         41       41           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@andrewfader andrewfader added dependencies Pull requests that update a dependency file ruby Pull requests that update Ruby code labels Jun 20, 2023
@david-a-wheeler
Copy link
Collaborator

Interesting idea.

The big concern of using an external service, especially this kind of service, is that they might reveal something private, e.g., secret keys, private email addresses, etc. This could be by accident or on purpose. We can use external services, we just need to take some steps to minimize the risk of using such services. We'll also need to document them as an external service we use and why we think they're okay, e.g., in our assurance case.

The Crunchbase entry on Sentry says they're a San Francisco firm founded in 2012. I don't see any issues there, there' no obvious location in a sanctioned country (we could get in trouble for that), and they've been around for a while (so it suggests they aren't a fly-by-night scam).

Sentry has a privacy policy - no guarantee of perfection, but it certainly has the look of a reasonable organization.

The Sentry about page suggests others trust them. Of course, this could be false, but I don't see any reason to doubt them.

I looked for discussions about malicious activity and Sentry.io. I did find discussions like this stackexchange.com about malicious domain names and this analysis report on ingest.sentry.io. This led me discussions on easylist and this uBlockOrigin discussion. The service seems to be used by malicious people, and/or they sometimes hide behind it, but that's different than being malicious themselves.

Question: Is the user ever tracked by this, in particular, is the user sent a cookie or anything else that their browser responds to? On a quick look it appears to be no, this is more a way to record errors when they happen internally, but maybe I'm missing something.

@andrewfader
Copy link
Collaborator Author

@david-a-wheeler basically what it does is, from the server side, send up the context and the session information from the error message, so if we want to ensure that no user information ever gets sent to it, we can scrub that data e.g. https://docs.sentry.io/platforms/javascript/data-management/sensitive-data/ (for the JS version but similar process) It's not a user tracking cookie or anything of that nature, but it's possible that user's email would be included in the payload if we don't scrub it. But yeah, the intended use case is for error tracking from the code itself and not to track users or their specific data

@david-a-wheeler
Copy link
Collaborator

How hard would it be to modify their beforeSend in Ruby to filter out email addresses and passwords? Bonus points if we can filter out the IP addresses of the client. Those are the sensitive pieces of information.

@andrewfader
Copy link
Collaborator Author

Yep, we can do that

@andrewfader
Copy link
Collaborator Author

Will reopen

@david-a-wheeler
Copy link
Collaborator

Yep, we can do that

Great! I have no reason to believe sentry would misuse our data, but if we can reduce the risk that'd be even better :-).

Will reopen

No problem, I've done that too!

@david-a-wheeler
Copy link
Collaborator

Oh, we should also use filters to try to remove our secret keys so they're unlikely to be sent to Sentry.

I'm willing to accept them as a trusted service (expecting them to keep things secret we send them), but reducing the trust we need to place on them seems prudent.

Signed-off-by: Andrew Fader <[email protected]>
Copy link
Collaborator

@david-a-wheeler david-a-wheeler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks okay. We should add this to assurance-case.md in our list of trusted services.

@david-a-wheeler david-a-wheeler merged commit 91b3474 into main Sep 10, 2023
6 checks passed
@david-a-wheeler
Copy link
Collaborator

I merged it. Should we add filters, or is that unlikely to be an issue? It's okay to share secret information with trusted orgs (by definition we trust them not to abuse it or share it further), but I want to avoid that where practical.

@andrewfader
Copy link
Collaborator Author

Oh, yeah, I was going to add those filters.

@andrewfader andrewfader deleted the install_sentry branch September 11, 2023 00:15
@david-a-wheeler
Copy link
Collaborator

Please add the filters as a new PR.

@david-a-wheeler david-a-wheeler mentioned this pull request Nov 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file ruby Pull requests that update Ruby code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants