Skip to content

feat: Better masking of option/radio/checkbox values#1164

Open
mydea wants to merge 1 commit intorrweb-io:masterfrom
getsentry:fn/mask-select-option
Open

feat: Better masking of option/radio/checkbox values#1164
mydea wants to merge 1 commit intorrweb-io:masterfrom
getsentry:fn/mask-select-option

Conversation

@mydea
Copy link
Copy Markdown
Contributor

@mydea mydea commented Mar 2, 2023

This PR improves masking via maskAllInputs for:

  • <option> elements as part of <select>
  • <input type="checkbox">
  • <input type="radio">

What is masked

For radio/checkbox, we only mask the value="xxx" attribute. So for example:

<input type="radio" value="my-val">

becomes

<input type="radio" value="******">

If no value is specified, nothing is added.

For option tags, we mask both the value attribute on them (so they also match the value of the select), as well as the content of the option - this is IMHO more in line with text input masking than to leave the displayed text.

<select value="val1">
  <option value="val1">Value 1</option>
  <option value="val2">Value 2</option>
</select>

becomes

<select value="****">
  <option value="****">*******</option>
  <option value="****">*******</option>
</select>

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Mar 2, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: b310e6f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages
Name Type
rrweb-snapshot Minor
rrweb Minor
rrdom Minor
rrdom-nodejs Minor
rrweb-player Minor
@rrweb/types Minor
@rrweb/web-extension Minor
rrvideo Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@mydea mydea mentioned this pull request Mar 2, 2023
Merged
Juice10
Juice10 reviewed Mar 3, 2023
View reviewed changes
packages/rrweb-snapshot/src/utils.ts
Comment on lines +171 to +188
// Handle `option` like `select
if (tagName.toLowerCase() === 'option') {
tagName = 'select';
}

Copy link
Copy Markdown
Member

@Juice10 Juice10 Mar 3, 2023

Choose a reason for hiding this comment

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

This method always gets an upper case tagName, keeping it upper case would save us one toLowerCase call, slightly improving performance

Suggested change
// Handle `option` like `select
if (tagName.toLowerCase() === 'option') {
tagName = 'select';
}
// Handle `option` like `select
if (tagName === 'OPTION') {
tagName = 'SELECT';
}

Copy link
Copy Markdown
Contributor Author

@mydea mydea Mar 6, 2023

Choose a reason for hiding this comment

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

Yeah, I wasn't quite sure here myself, as sometimes tagNames are passed as lowercase, sometimes as uppercase strings 😅

Copy link
Copy Markdown
Contributor Author

@mydea mydea Mar 6, 2023

Choose a reason for hiding this comment

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

Oops, actually it is not always lowercase (which nicely the tests caught!).
I've actually changed the type to use Uppercase to ensure that the correct case is passed in. This is supported since TS 4.1, is this OK?

YunFeng0817

This comment was marked as outdated.

Sign in to view

YunFeng0817
YunFeng0817 reviewed Mar 19, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@YunFeng0817 YunFeng0817 left a comment

Choose a reason for hiding this comment

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

Sorry for the late review. There are so many changes to the Jest snapshot files so I took some time to check them.
I may find an issue with masking radio/checkbox elements. It would be nice to check them. I'm willing to merge this PR after then.

packages/rrweb/test/__snapshots__/integration.test.ts.snap Outdated
\\"data\\": {
\\"source\\": 5,
\\"text\\": \\"off\\",
\\"text\\": \\"radio-on\\",
Copy link
Copy Markdown
Member

@YunFeng0817 YunFeng0817 Mar 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I may find a bug for masking radio input. In this test, the value "radio-on" is masked in the full snapshot however its actual value is exposed in the input mutation event here.

Copy link
Copy Markdown
Contributor Author

@mydea mydea Mar 20, 2023

Choose a reason for hiding this comment

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

found the culprit and fixed it! 🎉

packages/rrweb/test/__snapshots__/integration.test.ts.snap Outdated
\\"type\\": 3,
\\"data\\": {
\\"source\\": 5,
\\"text\\": \\"radio-off\\",
Copy link
Copy Markdown
Member

@YunFeng0817 YunFeng0817 Mar 19, 2023

Choose a reason for hiding this comment

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

Same here.

@mydea mydea mentioned this pull request Mar 20, 2023
Merged
@mydea mydea force-pushed the fn/mask-select-option branch from 5026175 to f36b26c Compare March 20, 2023 14:23
@mydea mydea force-pushed the fn/mask-select-option branch from 4a07be3 to 9aa9fbf Compare July 7, 2023 09:38
@mydea
Copy link
Copy Markdown
Contributor Author

mydea commented Jul 7, 2023

I rebased & updated this PR on latest master!

@billyvg billyvg force-pushed the fn/mask-select-option branch from 9aa9fbf to b310e6f Compare July 17, 2023 20:55
@pauldambra pauldambra mentioned this pull request Mar 24, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Juice10 Juice10 Juice10 left review comments

@YunFeng0817 YunFeng0817 YunFeng0817 requested changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mydea @Juice10 @YunFeng0817