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

Never unmask password inputs #34

Closed
Lms24 opened this issue Jan 30, 2023 · 1 comment · Fixed by #78
Closed

Never unmask password inputs #34

Lms24 opened this issue Jan 30, 2023 · 1 comment · Fixed by #78

Comments

@Lms24
Copy link
Member

Lms24 commented Jan 30, 2023

Currently, users can write custom unmask selectors to unmask passwords, meaning their users' passwords can be visible in clear text if the website for instance provides a "View Password" funcitonality.

IMO, under no circumstances whatsoever, never! (yup, feeling strongly about this) should we allow this to happen.

After chatting a little bit about this, the probably best way to block this type of unmasking, is to change the unmasking behaviour in rrweb. We should investigate how to handle this best.

@Lms24
Copy link
Member Author

Lms24 commented Feb 7, 2023

Putting this here to not loose the Slack conversation:

We could probably get away with a similar approach as shown here: highlight/rrweb@a50aed4#diff-45cc3019725e113400ca8fb1da26625aa1dc4cf507713f452af569d1cf22a257R578-R586

@mydea mydea closed this as completed in #78 Mar 8, 2023
mydea added a commit that referenced this issue Mar 8, 2023
This PR does two things:

1. Ensure you cannot opt-out of masking `type="password"` inputs
2. Ensure inputs that used to have `type=password` and have this type
changed (so e.g. through a "show password" toggle) always keep the
password masked, even afterwards.

We should never record passwords, and e.g. `toggle password` type
buttons are quite common and would easily leak the password into the
replay as of now.

We do this by adding a `rr_is_password` attribute to the HTML input when
the type is changed from `password` to something else. This is IMHO the
easiest solution for this, and since we only add this when the type is
really changed (so not eagerly for all inputs) IMHO it's an acceptable
tradeoff to have this in the DOM.

Closes #34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant