Fix proper sentry initialization#10116
Conversation
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR moves the remote-server daemon through the full app initialization path and adds protocol plumbing for Sentry identity and crash-reporting preference updates.
Security
- The client-side crash-reporting preference stored for future daemon handshakes is only initialized once, so later connects can send a stale opt-in/opt-out state.
Concerns
- Re-enabling daemon crash reporting after it was disabled initializes Sentry without reapplying the client-supplied user identity, despite the new protocol fields.
Verdict
Found: 0 critical, 2 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| auth_token: Option<String>, | ||
| /// User ID from the most recent `Initialize` handshake (Firebase UID). | ||
| #[cfg(feature = "crash_reporting")] | ||
| sentry_user_id: String, |
There was a problem hiding this comment.
Why is this prefixed with sentry_? It's really just the user_id, no?
| sentry_user_id: String::new(), | ||
| #[cfg(feature = "crash_reporting")] | ||
| sentry_user_email: String::new(), |
There was a problem hiding this comment.
if there's no user, this should be unset instead of an empty string
| fn apply_sentry_user_id(&self, ctx: &mut warpui::AppContext) { | ||
| if !self.auth.sentry_user_id.is_empty() { | ||
| crate::crash_reporting::set_user_id( | ||
| crate::auth::UserUid::new(&self.auth.sentry_user_id), | ||
| if self.auth.sentry_user_email.is_empty() { | ||
| None | ||
| } else { | ||
| Some(self.auth.sentry_user_email.clone()) | ||
| }, | ||
| ctx, | ||
| ); |
There was a problem hiding this comment.
Can we not directly populdate the AuthManager / AuthStateProvider upon receiving the initialize message? I doubt this is the only model that we need for remote code that reads those two models
| // privacy settings so that future daemon handshakes send the | ||
| // current value rather than a stale snapshot. | ||
| let privacy_settings = PrivacySettings::handle(ctx); |
There was a problem hiding this comment.
It's not just future daemon handshakes thuogh...we'll need to send an explicit message to the server when the user changes their crash reporting setting right?
2 participants
Description
Tech spec here: https://github.com/warpdotdev/warp/blob/70a0bed2f31a126495f8190e9cb5252cb9fe3ee6/specs/daemon-sentry-initialization/TECH.md
Testing
Tested locally and confirmed sentry event was produced: https://warpdotdev.sentry.io/issues/7459771029/?project=5660375&query=is%3Aunresolved&referrer=issue-stream