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

feat(sdk): Custom touch tracking property instead of accessibilityLabel #2712

Merged
merged 5 commits into from
Jan 4, 2023
Merged

feat(sdk): Custom touch tracking property instead of accessibilityLabel #2712

merged 5 commits into from
Jan 4, 2023

Conversation

jenskuhrjorgensen
Copy link
Contributor

@jenskuhrjorgensen jenskuhrjorgensen commented Dec 21, 2022

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

This PR removes the default usage of accessibilityLabel for automatic touch tracking due to GDPR issues. As a replacement, the PR adds a custom labelName property to the TouchEventBoundary that can be used to set a custom property name to be used for touch tracking.

💡 Motivation and Context

Currently, the sentry-react-native uses accessibilityLabel for automatic touch tracking. This means that if no sentry-label is provided it falls back to using accessibilityLabel as part of the uploaded breadcrumbs. However, accessibilityLabel often contains PII data that can help accessibility users with e.g. TalkBack or VoiceOver (this be for instance home address, phone number, social security number etc.). This means that whenever a touch event is logged to sentry.io for a component that has an accessibilityLabel and not a sentry-label the PII data will end up at Sentry. This is a GDPR issue for European companies.

I think for a lot of users/companies it is not desirable (or even legal) to have their accessibilityLabel data logged by default. This PR adds a custom labelName property to the TouchEventBoundary that will then be used as fallback in case no sentry-label is present. This can then be set to for instance "accessibilityLabel" for users that would like to continue using their accessibilityLabel in touch tracking, but it will then make sure that it is a conscious choice to log this as it will potentially hold PII data. In my own case, we would like to use our existing testID property for touch tracking (currently, we have an npm patch applied to Sentry that does exactly this because logging accessibilityLabel is definitely not an option, an even though we could get around this by adding a sentry-label to all components that have an accessibilityLabel the default fallback is simply to dangerous for us in case we miss a sentry-label and an accessibilityLabel instead slips through), as it gives us a lot of context information about what interactions the app user performed up to a crash.

I was motivated to create this PR by our support contacts from Sentry, Will C. and Hazid M.

💚 How did you test it?

I replaced the accessibilityLabel tests with custom-sentry-label-name and verified that clicking the "Capture Exception" button is logging the value stored in custom-sentry-label-name in the Metro bundler in "Event beforeSend:" instead of the previously logged accessibilityLabel.

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • All tests passing
  • No breaking changes

🔮 Next steps

This is a breaking change as accessibilityLabel will no longer be logged by default. The migration for existing users is to simply add it to your touchEventBoundaryProps.labelName:

Sentry.wrap(App, {
  touchEventBoundaryProps: {
    labelName: 'accessibilityLabel',
  },
})

or when using the component variant:

<Sentry.TouchEventBoundary
  labelName='accessibilityLabel'
>
  <RestOfTheApp />
</Sentry.TouchEventBoundary>

Jens Kuhr Jørgensen added 2 commits December 21, 2022 12:28
…ing and remove accessibilityLabel being used by default because it can (and often will) contain PII data.

(cherry picked from commit 83d349e)
(cherry picked from commit 53c3d82)
Copy link
Member

@krystofwoldrich krystofwoldrich left a comment

Choose a reason for hiding this comment

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

Hi,
I've checked the code again and it looks good to me,
could you add a changelog (example below) so we can merge this?

### Breaking changes

(...)
- `touchEventBoundaryProps.labelName` property instead of default `accessibilityLabel` fallback ([#2712](https://github.com/getsentry/sentry-react-native/pull/2712))

Jens Kuhr Jørgensen added 2 commits January 4, 2023 12:52
@jenskuhrjorgensen
Copy link
Contributor Author

Hi, I've checked the code again and it looks good to me, could you add a changelog (example below) so we can merge this?

### Breaking changes

(...)
- `touchEventBoundaryProps.labelName` property instead of default `accessibilityLabel` fallback ([#2712](https://github.com/getsentry/sentry-react-native/pull/2712))

@krystofwoldrich thanks for your review. I've added a changelog.

Copy link
Member

@krystofwoldrich krystofwoldrich left a comment

Choose a reason for hiding this comment

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

Looks good. Let's wait for CI and then it can be merged.

Thanks.

@krystofwoldrich krystofwoldrich changed the title Custom touch tracking property instead of accessibilityLabel feat(sdk): Custom touch tracking property instead of accessibilityLabel Jan 4, 2023
@krystofwoldrich krystofwoldrich merged commit 29f7009 into getsentry:5.0.0 Jan 4, 2023
@jenskuhrjorgensen jenskuhrjorgensen deleted the 5.0.0 branch January 4, 2023 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants