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

Replay: Make unmasking of detectable sensitive fields impossible #10258

Closed
bruno-garcia opened this issue Jan 18, 2024 · 2 comments · Fixed by getsentry/rrweb#166 or #10472
Closed

Replay: Make unmasking of detectable sensitive fields impossible #10258

bruno-garcia opened this issue Jan 18, 2024 · 2 comments · Fixed by getsentry/rrweb#166 or #10472

Comments

@bruno-garcia
Copy link
Member

bruno-garcia commented Jan 18, 2024

Sentry Replay by default masks all text. But users are able to unmask things by opt-ing out of unmasking.

An additional safety net is to skip the unmasking process for some field types, like type='password':

Some sensitive fields are not well defined but somehow integrations like 1password can pre-fill credit card and CVV info.
Would be great to also avoid recording those altogether. To make sure someone who accidently (or due to malice) removed masking does not capture that data.

@mydea
Copy link
Member

mydea commented Jan 19, 2024

An additional safety net is to skip the unmasking process for some field types, like type='password' (I believe we do this already but couldn't find it on the docs)

Yes, we force masking of password inputs in rrweb itself!

Some sensitive fields are not well defined but somehow integrations like 1password can pre-fill credit card and CVV info.
Would be great to also avoid recording those altogether. To make sure someone who accidently (or due to malice) removed masking does not capture that data.

There are ways to infer if something is a credit card field etc. I think I've written this down somewhere already at some point, but no idea where, but the easiest approach is to look at this: https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/autocomplete

The following could be considered un-unmaskable:

  • [autocomplete=current-password]
  • [autocomplete=new-password]
  • [autocomplete=cc-number]
  • [autocomplete=cc-exp]
  • [autocomplete=cc-exp-month]
  • [autocomplete=cc-exp=year]
  • [autocomplete=cc-csc]

this obv. only works if these are set, but they are also very reliable and we can pretty much rule out false positives there, I think.

@bruno-garcia
Copy link
Member Author

bruno-garcia commented Jan 19, 2024

Yes, we force masking of password inputs in rrweb itself!

We had an issue where a replay that was explicitly unmasking everything had some input=password show its content. We'll investigate further.

First look from @mydea had him say:

maybe the problem is that the value is inlined in the html, we mostly handle this in Dom listeners . Not sure but will look into it next week!

Additional context we received in how this situation can happen:

Some additional context; the password was originally masked but upon clicking the Password Reveal Button it was unmasked.

@bruno-garcia bruno-garcia changed the title Replay: Make unmasking of detectable sensitive fields not possible Replay: Make unmasking of detectable sensitive fields impossible Jan 19, 2024
mydea added a commit to getsentry/rrweb that referenced this issue Feb 1, 2024
This tests masking of initial & updated values for password & credit
card fields.

You can see that for now credit card data is not masked, which
demonstrates the current behavior.
In a future PR we can then verify that our fixed masking is working.

ref getsentry/sentry-javascript#10258
mydea added a commit to getsentry/rrweb that referenced this issue Feb 1, 2024
This is on top of #165, actually
fixing the behavior so that certain fields cannot be unmasked.

This is a pretty straightforward fix, a bit "hacky" but should work well
enough.

Fixes getsentry/sentry-javascript#10258

---------

Co-authored-by: mydea <mydea@users.noreply.github.com>
mydea added a commit that referenced this issue Feb 2, 2024
This bumps our rrweb-fork to 2.11.0, which mainly includes an
improvement to avoid capturing credit card inputs.

See: https://github.com/getsentry/rrweb/releases/tag/2.11.0

Fixes #10258

I also added a test in replay itself to verify that this works as
expected!
onurtemizkan pushed a commit that referenced this issue Feb 4, 2024
This bumps our rrweb-fork to 2.11.0, which mainly includes an
improvement to avoid capturing credit card inputs.

See: https://github.com/getsentry/rrweb/releases/tag/2.11.0

Fixes #10258

I also added a test in replay itself to verify that this works as
expected!
billyvg pushed a commit to getsentry/rrweb that referenced this issue Apr 26, 2024
This tests masking of initial & updated values for password & credit
card fields.

You can see that for now credit card data is not masked, which
demonstrates the current behavior.
In a future PR we can then verify that our fixed masking is working.

ref getsentry/sentry-javascript#10258
billyvg pushed a commit to getsentry/rrweb that referenced this issue Apr 26, 2024
This is on top of #165, actually
fixing the behavior so that certain fields cannot be unmasked.

This is a pretty straightforward fix, a bit "hacky" but should work well
enough.

Fixes getsentry/sentry-javascript#10258

---------

Co-authored-by: mydea <mydea@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants