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: Mask page URLs in session recordings #811

Merged
merged 1 commit into from
Sep 27, 2023

Conversation

dzsibi
Copy link
Contributor

@dzsibi dzsibi commented Sep 26, 2023

Changes

  • Meta events generated by rrweb have a href attribute that is now masked by maskNetworkRequestFn
  • Virtual $pageview events have a href attribute that is now masked by maskNetworkRequestFn

Notes

Right now, properties on events can be filtered using sanitize_properties to redact any sensitive information in URLs, and maskNetworkRequestFn does the same for captured network events. But rrweb has a Meta event that leaks the full page URL, and there is also a mechanism that translates $pageview events into rrweb custom events, that captures window.location.href directly. I passed these through maskNetworkRequestFn to redact sensitive information as necessary. Without this patch, neither of these values can be sanitized by the currently available options.

To be honest, I don't know why window.location.href is used directly when $pageview events are translated into rrweb custom events - there is a perfectly good $current_url property that has already passed through sanitize_properties on the event object. As an alternative to passing that through maskNetworkRequestFn one could also just use that value (it might also be more "correct" in the sense that I don't know if there is a guarantee that window.location.href would always have the desired value when it is used here). But that still leaves the Meta event, which captures window.location.href mercilessly.

I couldn't find any test infrastructure for events coming from / forwarded to rrweb, so I don't really know how to add tests for this without doing a lot of groundwork first.

Checklist

@benjackwhite benjackwhite changed the title Mask page URLs in session recordings feat: Mask page URLs in session recordings Sep 27, 2023
Copy link
Collaborator

@benjackwhite benjackwhite left a comment

Choose a reason for hiding this comment

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

This totally makes sense.

We use this value because we aren't guaranteed to have pageview events (some people turn them off for whatever reason) so we do need it in the payload. Re-using the maskNetworkRequestFn makes total sense.

We're likely to be extending this network code later to be able to track more things so we will take care of proper tests at that point 👍

@benjackwhite benjackwhite added the bump patch Bump patch version when this PR gets merged label Sep 27, 2023
@benjackwhite benjackwhite merged commit 152c0b2 into PostHog:master Sep 27, 2023
9 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bump patch Bump patch version when this PR gets merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants