chore: Masking user data in mixpanel session recordings #38267
Conversation
|
Warning Rate limit exceeded@hetunandu has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 25 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe pull request introduces a systematic approach to adding the Changes
Assessment against linked issues
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
IDE Pod
hetunandu
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/client/src/pages/Editor/Canvas.tsx (2)
Line range hint
95-101: Review canvas-level masking scopeAdding masking to the entire canvas artboard might be too broad. Consider more granular masking at the widget level for sensitive data.
Consider creating a higher-order component or utility function to handle masking:
const withMasking = (WrappedComponent: React.ComponentType) => { return (props: any) => ( <WrappedComponent {...props} className={`${props.className || ''} mp-mask`} /> ); };
Line range hint
35-36: Document masking strategy and testing requirementsThe implementation adds masking capabilities across multiple components, but requires:
- Documentation of the masking strategy
- Testing plan for session recording masking
- Security review of masked data
Consider creating:
- A masking configuration file defining which components need masking
- Unit tests verifying masking behavior
- Security guidelines for implementing masking in new components
Also applies to: 43-43, 95-101
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/client/src/PluginActionEditor/components/PluginActionResponse/components/Table.tsx(2 hunks)app/client/src/components/editorComponents/CodeEditor/PeekOverlayPopup/PeekOverlayPopup.tsx(1 hunks)app/client/src/components/editorComponents/Debugger/LogItem/LogItem.tsx(3 hunks)app/client/src/components/editorComponents/ReadOnlyEditor.tsx(1 hunks)app/client/src/pages/AppViewer/AppPage/AppPage.tsx(1 hunks)app/client/src/pages/Editor/Canvas.tsx(1 hunks)app/client/src/utils/Analytics/mixpanel.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- app/client/src/pages/AppViewer/AppPage/AppPage.tsx
- app/client/src/PluginActionEditor/components/PluginActionResponse/components/Table.tsx
🔇 Additional comments (3)
app/client/src/utils/Analytics/mixpanel.ts (1)
35-36: Verify masking selector implementation across the application
The selectors .mp-mask and .mp-block are used to control session recording masking. Ensure consistent implementation across all sensitive data components.
Consider documenting the masking strategy in a central location and creating a shared constant for the class names to ensure consistency.
✅ Verification successful
Masking classes are consistently implemented across sensitive components
The search results show that .mp-mask class is properly used across multiple sensitive components:
- Canvas artboard
- App page
- Read-only editor
- Debugger logs and console data
- Code peek overlay
- Plugin action response tables
However, .mp-block class is only defined in the Mixpanel configuration but not used in any components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for components using masking classes
echo "Components using mp-mask:"
rg "className.*mp-mask" --type tsx
echo "\nComponents using mp-block:"
rg "className.*mp-block" --type tsx
Length of output: 258
Script:
#!/bin/bash
# Search for components using masking classes with correct file extensions
echo "Components using mp-mask:"
rg "className.*mp-mask" --type ts --type js --type jsx --type typescript --type javascript
echo -e "\nComponents using mp-block:"
rg "className.*mp-block" --type ts --type js --type jsx --type typescript --type javascript
# Search for any other potential usage of these classes
echo -e "\nOther usages of mp-mask:"
rg "mp-mask" --type ts --type js --type jsx --type typescript --type javascript
echo -e "\nOther usages of mp-block:"
rg "mp-block" --type ts --type js --type jsx --type typescript --type javascript
Length of output: 721
Script:
#!/bin/bash
# Search for components using masking classes without specifying file types
echo "Components using mp-mask:"
rg "className.*mp-mask"
echo -e "\nComponents using mp-block:"
rg "className.*mp-block"
# Search for any other potential usage of these classes
echo -e "\nOther usages of mp-mask:"
rg "mp-mask"
echo -e "\nOther usages of mp-block:"
rg "mp-block"
# Also check for CSS/SCSS files that might define these classes
echo -e "\nCSS definitions:"
rg "\.(mp-mask|mp-block)" --type css --type scss
Length of output: 68323
app/client/src/components/editorComponents/ReadOnlyEditor.tsx (1)
43-43: Verify masking behavior for editor content
The addition of mp-mask class is aligned with the masking strategy, but ensure that it properly masks sensitive data in session recordings.
✅ Verification successful
The previous command failed because we need to specify the correct file type flags. Let's search for Editor components, the mp-mask class usage, and any masking-related code to verify the masking behavior implementation.
Let's try searching without file type restrictions since the tsx file type isn't recognized. This will help us understand the editor components and masking implementation across the codebase.
Let's try using find with grep to locate files containing masking-related code and the ReadOnlyEditor component usage, since the previous commands weren't successful. This will help us verify the masking implementation.
Let's examine the implementation of the mp-mask class in ReadOnlyEditor and related files to understand how the masking is being used and verify its proper implementation for sensitive data.
Masking implementation is correctly applied for session recording privacy
The mp-mask class is properly integrated with Mixpanel's session recording configuration. The implementation:
- Masks sensitive data in the ReadOnlyEditor component
- Is configured in Mixpanel initialization with
record_mask_text_selector: ".mp-mask" - Consistently applied across viewer components like AppPage
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other editor components that might need masking
rg "extends.*Editor|implements.*Editor" --type tsx
Length of output: 80
Script:
#!/bin/bash
# Search for Editor components and their usage
rg "class.*Editor|interface.*Editor" --type ts --type tsx -A 5
# Search for mp-mask class usage
rg "mp-mask" --type ts --type tsx -A 2
# Search for masking related code
rg "mask" --type ts --type tsx -A 2
Length of output: 223
Script:
#!/bin/bash
# Search for Editor components and their usage without file type restrictions
rg "class.*Editor|interface.*Editor"
# Search for mp-mask class usage
rg "mp-mask"
# Search for masking related code
rg "mask.*class|className.*mask"
Length of output: 102064
Script:
#!/bin/bash
# Search for masking-related code and classes in the codebase
find . -type f -exec grep -l "mp-mask\|mask.*class\|className.*mask" {} \;
# Search for ReadOnlyEditor component usage
find . -type f -exec grep -l "ReadOnlyEditor" {} \;
# Look for masking implementation details
find . -type f -exec grep -l "mask.*implementation\|masking.*data" {} \;
Length of output: 857
Script:
#!/bin/bash
# Check the implementation of mp-mask in ReadOnlyEditor.tsx
cat "./app/client/src/components/editorComponents/ReadOnlyEditor.tsx"
# Check for any masking implementation in mixpanel analytics
cat "./app/client/src/utils/Analytics/mixpanel.ts"
# Look for masking usage in AppPage
cat "./app/client/src/pages/AppViewer/AppPage/AppPage.tsx"
Length of output: 7257
app/client/src/components/editorComponents/CodeEditor/PeekOverlayPopup/PeekOverlayPopup.tsx (1)
160-160: LGTM! Verify masking behavior with different data types
The addition of the mp-mask class aligns with the data privacy requirements.
✅ Verification successful
The previous search might have failed because we need to understand the component structure better. Let's check for the actual implementation and usage patterns of both JsonWrapper and the mp-mask class.
Let's look for more context about how the masking class is implemented and used across the codebase to verify if it's being applied correctly in this component.
Based on the search results, I can now provide a final response about the mp-mask class addition:
LGTM! The mp-mask class is correctly applied for data masking
The mp-mask class is consistently used across the codebase for masking sensitive data in various components including tables, JSON viewers, and debug logs. The implementation in PeekOverlayPopup aligns with this pattern, particularly for masking JSON data display.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistent masking implementation across similar components
rg -l "JsonWrapper.*className.*mp-mask"
Length of output: 41
Script:
#!/bin/bash
# Let's first check if JsonWrapper is imported and used in other components
rg -l "JsonWrapper"
# Then check for any other components using mp-mask class
rg -l "mp-mask"
# Look for the JsonWrapper component definition
ast-grep --pattern 'const JsonWrapper = $$$'
Length of output: 89680
Script:
#!/bin/bash
# Let's check how mp-mask is used in the codebase to understand its purpose
rg -A 2 -B 2 "mp-mask"
# Also check for any masking-related functionality in the codebase
rg -l "mask|masking"
Length of output: 9696
| @@ -311,7 +311,7 @@ export function LogItem(props: LogItemProps) { | |||
| })} | |||
| {props.state && ( | |||
| <JsonWrapper | |||
| className="t--debugger-log-state" | |||
| className="t--debugger-log-state mp-mask" | |||
There was a problem hiding this comment.
💡 Codebase verification
Additional sensitive data displays found in ErrorLogs that need masking
The LogCollapseData.tsx component also uses ReactJson to display state data but lacks the mp-mask class. Consider applying consistent masking across all debugger data displays.
app/client/src/components/editorComponents/Debugger/ErrorLogs/components/LogCollapseData.tsx: Addmp-maskclass to theJsonWrappercomponent displaying state data
🔗 Analysis chain
LGTM! Comprehensive masking implementation
The mp-mask class is consistently applied across all data display elements, covering both JSON and text content.
Also applies to: 328-328, 337-337
Let me verify if the masking is consistently implemented across other debugger components.
Let me check if there are any other sensitive data displays in the debugger components that might need masking.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify consistent masking across debugger components
rg -A 2 "className.*debugger.*mp-mask"
Length of output: 1124
Script:
#!/bin/bash
# Search for other debugger components that might need masking
rg -l "className.*debugger" | grep -i "debug"
# Check for any existing mp-mask usage in debugger components
rg "mp-mask" $(rg -l "className.*debugger" | grep -i "debug")
Length of output: 2771
Script:
#!/bin/bash
# Search for JSON or data display components in debugger that might need masking
rg -A 2 "(JsonView|ReactJson|data-value|console\.log)" $(rg -l "className.*debugger" | grep -i "debug")
Length of output: 3035
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
app/client/src/utils/Analytics/mixpanel.ts (2)
25-39: Add input validation for configuration parameters.Consider validating the boolean parameters to ensure they're not undefined.
public async init({ enabled, mask, }: SessionRecordingConfig): Promise<boolean> { + if (typeof enabled !== 'boolean' || typeof mask !== 'boolean') { + log.warn("Invalid session recording configuration"); + return false; + } if (this.mixpanel) { log.warn("Mixpanel is already initialized.");
48-49: Document the CSS classes used for masking.The implementation uses
.mp-blockand.mp-maskCSS classes for masking sensitive data. Consider adding a comment explaining how these classes should be used in the UI components.+ // Use .mp-block to completely block elements from recording + // Use .mp-mask to blur text content in recordings record_block_selector: mask ? ".mp-block" : "", record_mask_text_selector: mask ? ".mp-mask" : "",app/client/src/ce/utils/AnalyticsUtil.tsx (1)
42-42: Consider handling initialization failure.Add error handling for cases where Mixpanel initialization fails.
- await MixpanelSingleton.getInstance().init(sessionRecordingConfig); + const mixpanelInitialized = await MixpanelSingleton.getInstance().init(sessionRecordingConfig); + if (!mixpanelInitialized) { + log.warn("Mixpanel session recording initialization failed"); + }app/client/src/ce/sagas/userSagas.tsx (1)
111-138: Add unit tests for the session recording configuration logic.The function implementation is clean and well-documented. However, given its role in controlling sensitive data masking, it should be thoroughly tested.
Would you like me to help generate unit tests for this function? The tests should cover:
- Kill switch scenario (kill_session_recordings_enabled)
- User-level disable scenario (!config_user_session_recordings_enabled)
- Enabled scenarios with and without masking
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/client/src/ce/entities/FeatureFlag.ts(2 hunks)app/client/src/ce/sagas/userSagas.tsx(2 hunks)app/client/src/ce/utils/AnalyticsUtil.tsx(3 hunks)app/client/src/utils/Analytics/mixpanel.ts(3 hunks)
🔇 Additional comments (5)
app/client/src/ce/entities/FeatureFlag.ts (1)
52-56: LGTM! Feature flags follow naming convention and safe defaults.
The implementation follows the documented naming convention and sets secure defaults:
- Kill switch disabled by default (false)
- Masking enabled by default (true)
- User recording enabled by default (true)
Also applies to: 99-101
app/client/src/utils/Analytics/mixpanel.ts (1)
7-10: LGTM! Clear interface definition for session recording config.
app/client/src/ce/utils/AnalyticsUtil.tsx (1)
9-11: LGTM! Verify impact on existing analytics initialization.
The changes to initialize method are clean and maintain compatibility with existing analytics implementation.
Also applies to: 30-33
✅ Verification successful
Let me search for the specific analytics initialization usage to verify the impact.
Let me check the actual usage in AdminSettings to verify the impact.
Let me check the actual initialize method implementation.
Let me try one final search to locate the initialize implementation.
Changes to MixpanelSingleton are safe and backward compatible
The import changes in AnalyticsUtil.tsx are correct as they reflect the actual implementation. The initialize method has been renamed to init and properly handles the new SessionRecordingConfig interface. The only usage in AdminSettings is for session recording control, which is compatible with these changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any direct initialize() calls that might need updating
# Search for direct initialize calls
rg -l "initialize\(" | xargs -I {} rg -A 2 "initialize\(" {}
Length of output: 4169
Script:
#!/bin/bash
# Search for MixpanelSingleton initialize calls
ast-grep --pattern 'MixpanelSingleton.initialize($$$)'
# Also search for any imports of MixpanelSingleton
rg "import.*MixpanelSingleton"
Length of output: 268
Script:
#!/bin/bash
# Get the context around MixpanelSingleton usage in AdminSettings
rg -B 3 -A 3 "MixpanelSingleton" app/client/src/pages/AdminSettings/index.tsx
# Also check AnalyticsUtil for initialize method implementation
rg -B 3 -A 3 "initialize.*SessionRecordingConfig" app/client/src/ce/utils/AnalyticsUtil.tsx
Length of output: 684
Script:
#!/bin/bash
# Get the initialize method implementation from mixpanel.ts
rg -B 5 -A 10 "initialize.*\(" app/client/src/utils/Analytics/mixpanel.ts
# Also check for any other files that might be using the initialize method
rg -l "MixpanelSingleton.*initialize"
Length of output: 115
Script:
#!/bin/bash
# Search for initialize method in Analytics directory
fd mixpanel.ts -t f | xargs cat
# Backup search in case the file is not found
rg -g "*.ts" -g "*.tsx" "class.*MixpanelSingleton" -A 20
Length of output: 5457
app/client/src/ce/sagas/userSagas.tsx (2)
77-77: LGTM!
The SessionRecordingConfig type import is appropriately placed and well-named.
142-146: Verify AnalyticsUtil.initialize implementation.
The changes look good, but we should verify that AnalyticsUtil.initialize properly handles the new sessionRecordingConfig parameter.
✅ Verification successful
Changes to AnalyticsUtil.initialize are properly implemented
The implementation correctly handles the new sessionRecordingConfig parameter, passing it to MixpanelSingleton's initialization. The changes maintain the existing initialization flow while adding session recording configuration support.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check AnalyticsUtil.initialize implementation for session recording config handling
# Search for AnalyticsUtil.initialize implementation
ast-grep --pattern 'initialize($$$, sessionRecordingConfig$$$) {
$$$
}'
Length of output: 75
Script:
#!/bin/bash
# Let's try a broader search for initialize method in AnalyticsUtil
rg -U "initialize.*\(" -A 10
# Also search for the class/file definition
fd -e ts -e tsx -e js "AnalyticsUtil"
Length of output: 66498
Script:
#!/bin/bash
# Let's search for AnalyticsUtil.initialize implementation in TypeScript/JavaScript files
rg -t ts -t tsx -t js "AnalyticsUtil.*initialize.*\(" -A 10
# Also search for the file containing AnalyticsUtil class
rg -t ts -t tsx -t js "class AnalyticsUtil" -A 10
Length of output: 169
Script:
#!/bin/bash
# Search for AnalyticsUtil.initialize without file type restrictions
rg "AnalyticsUtil.*initialize.*\(" -A 10
# Search for AnalyticsUtil class definition
rg "class AnalyticsUtil" -A 10
# Also try to find the file itself
fd "AnalyticsUtil"
Length of output: 474
Script:
#!/bin/bash
# Let's check the content of both CE and EE AnalyticsUtil files
cat app/client/src/ce/utils/AnalyticsUtil.tsx
cat app/client/src/ee/utils/AnalyticsUtil.tsx
Length of output: 3888
hetunandu
requested review from
ApekshaBhosale and
sagar-qa007
as code owners
| @@ -334,7 +334,7 @@ export function LogItem(props: LogItemProps) { | |||
| ); | |||
| } else { | |||
| return ( | |||
| <span className="debugger-label" key={Math.random()}> | |||
| <span className="debugger-label mp-mask" key={Math.random()}> | |||
There was a problem hiding this comment.
Hope you realize that this random key is your doing now :)
Description
Update masking properties to mask user data and not other UI Elements of Appsmith
Adds feature flagging support to control the recording and masking
Fixes #38249
Fixes #38269
Fixes #38014
Automation
/ok-to-test tags="@tag.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12429717492
Commit: 15f654e
Cypress dashboard.
Tags:
@tag.SanitySpec:
Fri, 20 Dec 2024 11:37:04 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
mp-maskclass to improve visual representation.Bug Fixes
Documentation