-
Notifications
You must be signed in to change notification settings - Fork 142
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-336] Privacy by Default: MVP scaffold #914
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -13,6 +13,7 @@ export const DEFAULT_CONFIGURATION = { | |||||
silentMultipleInit: false, | ||||||
trackInteractions: false, | ||||||
trackViewsManually: false, | ||||||
censorshipLevel: 'PUBLIC', | ||||||
|
||||||
/** | ||||||
* arbitrary value, byte precision not needed | ||||||
|
@@ -54,6 +55,7 @@ export interface UserConfiguration { | |||||
trackViewsManually?: boolean | ||||||
proxyHost?: string | ||||||
beforeSend?: BeforeSendCallback | ||||||
censorshipLevel?: string | ||||||
|
||||||
service?: string | ||||||
env?: string | ||||||
|
@@ -81,6 +83,7 @@ export type Configuration = typeof DEFAULT_CONFIGURATION & | |||||
|
||||||
service?: string | ||||||
beforeSend?: BeforeSendCallback | ||||||
censorshipLevel?: string | ||||||
|
||||||
isEnabled: (feature: string) => boolean | ||||||
} | ||||||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we should diverge from the current configuration handling here. Just keep whatever value the user provides.
Suggested change
|
||||||
} | ||||||
|
||||||
return configuration | ||||||
} | ||||||
|
||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,13 +6,16 @@ import { startSegmentCollection } from '../domain/segmentCollection' | |
import { send } from '../transport/send' | ||
import { RawRecord, RecordType } from '../types' | ||
|
||
let configurationProviderRef: Configuration | undefined | ||
|
||
export function startRecording( | ||
lifeCycle: LifeCycle, | ||
applicationId: string, | ||
configuration: Configuration, | ||
session: RumSession, | ||
parentContexts: ParentContexts | ||
) { | ||
configurationProviderRef = configuration | ||
const { addRecord, stop: stopSegmentCollection } = startSegmentCollection( | ||
lifeCycle, | ||
applicationId, | ||
|
@@ -36,6 +39,7 @@ export function startRecording( | |
stop: () => { | ||
stopRecording() | ||
stopSegmentCollection() | ||
configurationProviderRef = undefined | ||
}, | ||
} | ||
} | ||
|
@@ -47,3 +51,7 @@ export function trackViewEndRecord(lifeCycle: LifeCycle, addRawRecord: (record: | |
}) | ||
}) | ||
} | ||
|
||
export function getRumRecorderConfig(): Configuration | undefined { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Naming is going to need to change as we progress through the POC. |
||
return configurationProviderRef | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -15,6 +15,8 @@ import { | |||||
// to obfuscate. | ||||||
const PRIVACY_INPUT_TYPES_TO_IGNORE = ['email', 'password', 'tel'] | ||||||
|
||||||
const MASKING_CHAR = '᙮' | ||||||
|
||||||
// Returns true if the given DOM node should be hidden. Ancestors | ||||||
// are not checked. | ||||||
export function nodeShouldBeHidden(node: Node): boolean { | ||||||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is equivalent:
Suggested change
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,5 +1,6 @@ | ||||||
import { buildUrl } from '@datadog/browser-core' | ||||||
import { InputPrivacyMode } from '../../constants' | ||||||
import { CensorshipLevel, InputPrivacyMode, PRIVACY_INPUT_MASK } from '../../constants' | ||||||
import { getRumRecorderConfig } from '../../boot/startRecording' | ||||||
import { getNodeInputPrivacyMode, getNodeOrAncestorsInputPrivacyMode } from './privacy' | ||||||
import { SerializedNodeWithId } from './types' | ||||||
|
||||||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FMU, you are trying to replace the value with First, what about:
Suggested change
This is not enough to avoid leaking input length, because we are capturing every keystroke, so we will generate one But, I think we should not try to implement this in the same PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. More to discuss, probably making a distinction between
|
||||||
} | ||||||
return value.replace(/./g, '*') | ||||||
} | ||||||
|
||||||
export function isFlagEnabled(feature: string): boolean { | ||||||
const configuration = getRumRecorderConfig() | ||||||
if (!configuration) { | ||||||
return false | ||||||
} | ||||||
return configuration.isEnabled(feature) | ||||||
} | ||||||
|
||||||
export function getCensorshipLevel(): CensorshipLevel { | ||||||
const configuration = getRumRecorderConfig() | ||||||
if (!configuration) { | ||||||
// Should never happen. Default to private | ||||||
return CensorshipLevel.PRIVATE | ||||||
} | ||||||
// PENDING review from core package, core defines `censorshipLevel` as any string. | ||||||
const level: CensorshipLevel = configuration.censorshipLevel as CensorshipLevel | ||||||
return level | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -153,7 +153,7 @@ export function doStartSegmentCollection( | |
state = { | ||
status: SegmentCollectionStatus.SegmentPending, | ||
segment, | ||
expirationTimeoutId: setTimeout( | ||
expirationTimeoutId: window.setTimeout( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
monitor(() => { | ||
flushSegment('max_duration') | ||
}), | ||
|
+4 −1 | samples/action.json | |
+4 −1 | samples/app_start.json | |
+4 −1 | samples/error.json | |
+4 −1 | samples/long_task.json | |
+4 −1 | samples/resource.json | |
+4 −1 | samples/view.json | |
+12 −0 | schemas/_common-schema.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
startSessionReplayRecording
function instead ofinit
, so they are scoped and can be changed every time the recording is restartedcensorship
: I think it is a bit negatively connoted. What about something likeprivacyLevel
ormaskMode
?StatusType
) to avoid hardcoding valuesThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
startSessionReplayRecording
is interesting. 🤔