-
Notifications
You must be signed in to change notification settings - Fork 47
feat(session-replay-browser): ugc removal poc #1062
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
Conversation
📝 Documentation updates detected! You can review documentation updates here |
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.
Looks good overall and I think it's pretty close. Just wanted to discuss a few things before shipping this.
ac0aff4
to
27df1bb
Compare
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.
LGTM, one comment though.
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.
One request then I think we're OK
59ca82a
to
d52a4b4
Compare
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.
Bug: UGC Filter Rules Validation Errors
The ugcFilterRules
configuration introduces two related validation bugs:
validateUGCFilterRules
lacks anArray.isArray
check, causing aTypeError
(.every is not a function
) and SDK crash ifugcFilterRules
is not an array.getPageUrl
assumes elements withinugcFilterRules
are objects withselector
andreplacement
properties. RemoteugcFilterRules
are not validated, so if they contain non-objects (e.g., strings), accessingrule.selector
causes aTypeError
inglobToRegex
, crashing event processing.
packages/session-replay-browser/src/helpers.ts#L163-L185
Amplitude-TypeScript/packages/session-replay-browser/src/helpers.ts
Lines 163 to 185 in 0e3fd9e
export const validateUGCFilterRules = (ugcFilterRules: UGCFilterRule[]) => { | |
// validate ugcFilterRules | |
if (!ugcFilterRules.every((rule) => typeof rule.selector === 'string' && typeof rule.replacement === 'string')) { | |
throw new Error('ugcFilterRules must be an array of objects with selector and replacement properties'); | |
} | |
// validate ugcFilterRules are valid globs | |
if (!ugcFilterRules.every((rule) => isValidGlobUrl(rule.selector))) { | |
throw new Error('ugcFilterRules must be an array of objects with valid globs'); | |
} | |
}; | |
export const getPageUrl = (pageUrl: string, ugcFilterRules: UGCFilterRule[]) => { | |
// apply ugcFilterRules, order is important, first rule wins | |
for (const rule of ugcFilterRules) { | |
const regex = globToRegex(rule.selector); | |
if (regex.test(pageUrl)) { | |
return pageUrl.replace(regex, rule.replacement); | |
} | |
} | |
return pageUrl; |
Bug: UGC Filtering Logic Fails Remote Configuration Handling
The logic introduced to preserve local ugcFilterRules
during remote configuration fetching is flawed. If the remote interactionConfig
is absent, any locally provided ugcFilterRules
are discarded, silently disabling UGC filtering. Conversely, if a remote interactionConfig
is present, local ugcFilterRules
incorrectly overwrite remote ugcFilterRules
, violating the intended precedence for remote configuration. This results in either silent disabling of UGC filtering or unexpected masking behavior.
packages/session-replay-browser/src/config/joined-config.ts#L60-L82
Amplitude-TypeScript/packages/session-replay-browser/src/config/joined-config.ts
Lines 60 to 82 in 0e3fd9e
'sessionReplay', | |
'sr_sampling_config', | |
sessionId, | |
); | |
const privacyConfig = await this.remoteConfigFetch.getRemoteConfig( | |
'sessionReplay', | |
'sr_privacy_config', | |
sessionId, | |
); | |
const ugcFilterRules = config.interactionConfig?.ugcFilterRules; | |
// This is intentionally forced to only be set through the remote config. | |
config.interactionConfig = await this.remoteConfigFetch.getRemoteConfig( | |
'sessionReplay', | |
'sr_interaction_config', | |
sessionId, | |
); | |
if (config.interactionConfig && ugcFilterRules) { | |
config.interactionConfig.ugcFilterRules = ugcFilterRules; | |
} | |
// This is intentionally forced to only be set through the remote config. |
Was this report helpful? Give feedback by reacting with 👍 or 👎
[Experimental]
Summary
This pull request introduces a new feature to support User-Generated Content (UGC) filtering in the Session Replay library. It includes changes to add UGC filter rules, apply them to page URLs, and validate their usage. The changes span across configuration, helper methods, hooks, and tests.
UGC Filtering Feature Implementation
Configuration Updates:
UGCFilterRule
type to define UGC filter rules withselector
andreplacement
properties. (packages/session-replay-browser/src/config/types.ts
, packages/session-replay-browser/src/config/types.tsR53-R66)SessionReplayLocalConfig
to include an optionalugcFilterRules
property. (packages/session-replay-browser/src/config/local-config.ts
, [1] [2]Helper Functionality:
getPageUrl
helper to apply UGC filter rules to page URLs, with validation for rule structure and glob patterns. (packages/session-replay-browser/src/helpers.ts
, packages/session-replay-browser/src/helpers.tsR146-R193)Integration in Hooks:
click
andscroll
hooks to usegetPageUrl
for applying UGC filter rules to event data. (packages/session-replay-browser/src/hooks/click.ts
, [1];packages/session-replay-browser/src/hooks/scroll.ts
, [2]Testing Enhancements
Unit Tests for
getPageUrl
:packages/session-replay-browser/test/helpers.test.ts
, packages/session-replay-browser/test/helpers.test.tsR274-R373)Integration Tests for Hooks:
click
andscroll
events correctly apply UGC filter rules in various scenarios. (packages/session-replay-browser/test/hooks/click.test.ts
, [1];packages/session-replay-browser/test/hooks/scroll.test.ts
, [2]These changes collectively enable UGC filtering for improved privacy and customization in session replay data.
Checklist