✨[REPLAY-336] Privacy by Default: MVP scaffold #914
Conversation
Codecov Report
@@ Coverage Diff @@
## main #914 +/- ##
==========================================
- Coverage 89.10% 88.93% -0.17%
==========================================
Files 81 81
Lines 3836 3859 +23
Branches 855 860 +5
==========================================
+ Hits 3418 3432 +14
- Misses 418 427 +9
Continue to review full report at Codecov.
|
jagracey
force-pushed
the
john.gracey/privacy-by-default-mvp
branch
from
a452f7d to
3ad2e85
Compare
| @@ -13,6 +13,7 @@ export const DEFAULT_CONFIGURATION = { | |||
| silentMultipleInit: false, | |||
| trackInteractions: false, | |||
| trackViewsManually: false, | |||
| censorshipLevel: 'PUBLIC', | |||
There was a problem hiding this comment.
- It could be interesting to pass recording options to the
startSessionReplayRecordingfunction instead ofinit, so they are scoped and can be changed every time the recording is restarted - I'm not found of
censorship: I think it is a bit negatively connoted. What about something likeprivacyLevelormaskMode? - We should have an object constants (ex: similar to
StatusType) to avoid hardcoding values
There was a problem hiding this comment.
startSessionReplayRecording is interesting. 🤔
| @@ -139,6 +142,10 @@ export function buildConfiguration(userConfiguration: UserConfiguration, buildEn | |||
| configuration.trackViewsManually = !!userConfiguration.trackViewsManually | |||
| } | |||
|
|
|||
| if ('censorshipLevel' in userConfiguration) { | |||
| configuration.censorshipLevel = userConfiguration.censorshipLevel ?? configuration.censorshipLevel | |||
There was a problem hiding this comment.
I don't think we should diverge from the current configuration handling here. Just keep whatever value the user provides.
| configuration.censorshipLevel = userConfiguration.censorshipLevel ?? configuration.censorshipLevel | |
| configuration.censorshipLevel = userConfiguration.censorshipLevel |
| @@ -89,3 +91,5 @@ function isElement(node: Node): node is Element { | |||
| function isInputElement(elem: Element): elem is HTMLInputElement { | |||
| return elem.tagName === 'INPUT' | |||
| } | |||
|
|
|||
| export const censorText = (text: string) => text.replace(/[^\s]/g, MASKING_CHAR) | |||
There was a problem hiding this comment.
This is equivalent:
| export const censorText = (text: string) => text.replace(/[^\s]/g, MASKING_CHAR) | |
| export const censorText = (text: string) => text.replace(/\S/g, MASKING_CHAR) |
| @@ -125,5 +126,27 @@ export function getElementInputValue(element: Element, ancestorInputPrivacyMode? | |||
| } | |||
|
|
|||
| export function maskValue(value: string) { | |||
| if (isFlagEnabled('privacy-by-default-poc')) { | |||
| return value.replace(/.+/, PRIVACY_INPUT_MASK) | |||
There was a problem hiding this comment.
FMU, you are trying to replace the value with PRIVACY_INPUT_MASK, right? So the value length doesn't leak.
First, what about:
| return value.replace(/.+/, PRIVACY_INPUT_MASK) | |
| return PRIVACY_INPUT_MASK |
This is not enough to avoid leaking input length, because we are capturing every keystroke, so we will generate one IncrementalSnapshot.Input record for each character entered by the user. We might want to adjust things around here.
But, I think we should not try to implement this in the same PR.
There was a problem hiding this comment.
Agreed. More to discuss, probably making a distinction between block and mask (or other synonyms) whereby mask censors (or at least throttles) JS events. I imagine block makes more sense for simple stuff like text nodes (textContent).
return PRIVACY_INPUT_MASK yeah in hindsight....😳
| @@ -47,3 +51,7 @@ export function trackViewEndRecord(lifeCycle: LifeCycle, addRawRecord: (record: | |||
| }) | |||
| }) | |||
| } | |||
|
|
|||
| export function getRumRecorderConfig(): Configuration | undefined { | |||
There was a problem hiding this comment.
We will speak about this a bit further. I don't think it will be that useful to have a singleton for the configuration, because the "censorship level" depends on the DOM tree, not only the user configuration.
There was a problem hiding this comment.
Naming is going to need to change as we progress through the POC.
In this case, this variable represents the default censorship mode.
The name should be changed reflect that (along with a few other vars + consts)
| @@ -153,7 +153,7 @@ export function doStartSegmentCollection( | |||
| state = { | |||
| status: SegmentCollectionStatus.SegmentPending, | |||
| segment, | |||
| expirationTimeoutId: setTimeout( | |||
| expirationTimeoutId: window.setTimeout( | |||
There was a problem hiding this comment.
This shouldn't be needed. We'll have to audit which files are included during e2e tests and avoid including too many files in nodejs.
Motivation
Part one of a series of PRs aimed at implementing privacy by default (REPLAY-336).
censorshipLevelconfig property torum-corefor use withrum-recorderisFlagEnabledandgetCensorshipLevelcensorTextfor HTML text censorship that preserves whitespacePRIVACY_INPUT_MASKandFORM_PRIVATE_TAG_NAMES.Changes
Testing
I have gone over the contributing documentation.