Enable scrubbing options for dom inputs #1383
Conversation
src/browser/telemetry.js
Outdated
| this.maskInputOptions[inputType] | ||
| ) { | ||
| if (this.maskInputFn) { | ||
| value = this.maskInputFn(value, element); |
There was a problem hiding this comment.
If value happens to be masked in the conditionals in line 802:813, you'd be passing the masked value to this.maskInputFn here.
Maybe store the value in some originalValue var and pass that here?
src/browser/telemetry.js
Outdated
| if ( | ||
| domUtil.isMatchingElement(element, this.scrubClasses, this.scrubSelectors) | ||
| ) { | ||
| value = mask; | ||
| } | ||
|
|
||
| if (inputType === 'password') { | ||
| value = mask; | ||
| } |
There was a problem hiding this comment.
Could this override the return value of the custom this.maskInputFn? Is that expected behavior?
There was a problem hiding this comment.
Yes, this is expected. If it's a password field, it is always masked. Making it an early return at the top would make that more obvious.
test/browser.telemetry.test.js
Outdated
| ((options = { scrubFields, autoInstrument: { log: false } }), | ||
| (rollbar = new Rollbar({}))); |
There was a problem hiding this comment.
non-blocking, don't feel strongly, just asking
Is there any reason this is like this and not just
options = { scrubFields, autoInstrument: { log: false } };
rollbar = new Rollbar({});?
There was a problem hiding this comment.
I think prettier did this? Let me see what happens when I undo it.
test/fixtures/html/dom-events.html
Outdated
| <title>Elements for testing DOM event handling</title> | ||
| </head> | ||
| <body> | ||
| <div class="container" id"main-container"> |
There was a problem hiding this comment.
Yikes, there's an = missing between id and "main-container" here!
2 participants
Description of the change
Enables scrubbing of DOM inputs in telemetry.
telemetryScrubberandscrubTelemetryInputsas documented here: https://docs.rollbar.com/docs/rollbarjs-telemetryType of change
Checklists
Development