getsentry / sentry-cocoa

The official Sentry SDK for iOS, tvOS, macOS, watchOS.
https://sentry.io/for/cocoa/
MIT License
801 stars 316 forks source link

Scrubbing of device UUID not possible #1541

Closed ghost closed 2 years ago

ghost commented 2 years ago

Environment

SaaS (https://sentry.io/)

Version

Cocoa 7.6.1

Steps to Reproduce

To conform with GDPR I try to scrub as many informations as possible from my macOS crash reports.

My configuration looks like this:

        SentrySDK.start { options in

            options.dsn = apiKey

            /*
             * Debug logging on iOS is too verbose
             */
            options.debug = false

            /*
             * Per default Sentry is not sending any identifiable
             * information and we want to ensure that explicit.
             */
            options.sendDefaultPii = false

            options.beforeSend = { event in

                /*
                 * Scrub user info if it was attached.
                 */
                event.user = nil

                /*
                 * Not needed data
                 */
                event.dist = nil

                /*
                 * "serverName" contains the host name of the user which
                 * can contain a persons name. Too sensitive for us.
                 */
                event.serverName = nil

                return event
            }
        }

        /*
         * We don't need device identifiers
         */
        SentrySDK.configureScope { scope in
            scope.removeTag(key: "app.device")
        }

        /*
         * We don't want to track the user in any way.
         */
        SentrySDK.setUser(nil) 

Expected Result

No "app.device" tag in the UI.

I don't know what it is, but it looks like a fingerprint of the users hardware and maybe therefore a PII.

Actual Result

An "app.device" tag is still included in the report.

getsentry-release commented 2 years ago

Routing to @getsentry/workflow for triage. ⏲️

getsentry-release commented 2 years ago

Routing to @getsentry/owners-ingest for triage. ⏲️

wedamija commented 2 years ago

@getsentry/owners-ingest Redirecting to you since I think you can answer this better than workflow can

bruno-garcia commented 2 years ago

You should be able to remove that value through beforeSend: https://docs.sentry.io/platforms/apple/configuration/options/#before-send

You can define a callback and remove any value from the event.

That said, it's worth pointing out that this UUID is a randomly generated ID that's written to disk when the SDK initializes for the first time. We call it "installation id". So it cannot be used to identify the same user across different devices or apps through Sentry or other products. This will only consistently identify that the same crashes came from the same devices (unless the user reinstalls the app, then it'll consider those as different users).

The reason this exists is to be able to give you an idea of impact for a bug. By telling you how many different "users" are affected by it. I'm describing this to really question whether it's necessary to scrub this identifier. Regardless if you are willing to reconsider given the reasoning above, it does make sense for Sentry to allow scrubbing this value.

getsentry-release commented 2 years ago

Routing to @getsentry/team-mobile for triage. ⏲️

marandaneto commented 2 years ago

You should be able to remove that value through beforeSend: docs.sentry.io/platforms/apple/configuration/options/#before-send

You can define a callback and remove any value from the event.

That said, it's worth pointing out that this UUID is a randomly generated ID that's written to disk when the SDK initializes for the first time. We call it "installation id". So it cannot be used to identify the same user across different devices or apps through Sentry or other products. This will only consistently identify that the same crashes came from the same devices (unless the user reinstalls the app, then it'll consider those as different users).

The reason this exists is to be able to give you an idea of impact for a bug. By telling you how many different "users" are affected by it. I'm describing this to really question whether it's necessary to scrub this identifier. Regardless if you are willing to reconsider given the reasoning above, it does make sense for Sentry to allow scrubbing this value.

that's different @bruno-garcia, that comes from contexts.app. device_app_hash, which is a hash of identifierForVendor https://github.com/getsentry/sentry-cocoa/blob/cd561c7b8f5e0396f3510600167c17b2c076deed/Sources/SentryCrash/Recording/Monitors/SentryCrashMonitor_System.m#L378-L425

maybe only report that if defaultPii is enabled or is a hash an acceptable thing? cc @philipphofmann

philipphofmann commented 2 years ago

Good catch, @marandaneto. We shouldn't only send this if defaultPii is enabled. I'm going to move this to the Cocoa repo.

bruno-garcia commented 2 years ago

~If it's a hash it now brings the question if it can be used as PII.~ ~Is there a reason we don't use installation-id as a user-id by default? That we're clear is not an issue with PII.~

Generate a 20 byte SHA1 hash that remains unique across a single device and application.

It's a different thing from installation-id (thanks for pointing out). But it's the same situation. It's not something that can be used to link the user across different apps or services. So my thoughts above should still apply.

If this is being sent now without checking for sendDefautlPii we should just remove it as it's a breaking change (user impact info would disappear). Maybe this requires discussing with other folks. Perhaps we should discuss using installation-id by default?

marandaneto commented 2 years ago

I don't think contexts.app.device_app_hash is used for user impact info unless it's a special case for iOS, this should be based on the user only, this is set on contexts

bruno-garcia commented 2 years ago

I see. Worth checking in ingest pulls this and we index (do we have a tag with this value in sentry?) If we don't we can remove it without much worry. But I still don't understand the reason since this isn't pii

philipphofmann commented 2 years ago

Yes, I agree with you @bruno-garcia. The contexts.app.device_app_hash, which is a hash of identifierForVendor, is not PII. Therefore the app.device tag is not PII and you don't need to scrub it @AshStefanOltmann.

If you want to get rid of it though, something like this should work.

options.beforeSend = { event in

    var context = event.context
    var appContext = context?["app"]
    appContext?.removeValue(forKey: "device_app_hash")
    context?["app"] = appContext
    event.context = context

    return event
}
ghost commented 2 years ago

Thanks for clarification. 👍