fix: braze anonymousId tracking with alias details #1994
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
CodeRabbit Configuration File (
|
![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
🔭 Outside diff range comments (1)
packages/analytics-js-integrations/src/integrations/Braze/browser.js (1)
Implement PII protection mechanisms for error logging and data handling
The code has multiple security vulnerabilities that need to be addressed:
Error Logging:
- Replace
stringifyWithoutCircularV1(error, true)with a sanitized error logging that excludes PII- Add a dedicated error sanitizer utility to redact sensitive data before logging
Data Handling:
- Add input validation for all user traits before processing
- Implement PII filtering for sensitive fields like email, phone, address
- Create a dedicated sanitization layer for user data
Storage:
- Review and validate encryption implementation for stored user data
- Add data minimization to only store required PII fields
🔗 Analysis chain
Line range hint
1-290: Verify security considerations for user data handlingPlease ensure the following security aspects are addressed:
PII Handling:
- Verify that sensitive user attributes are not logged in debug/error messages
- Consider implementing a PII filter in
handleReservedPropertiesInput Sanitization:
- Add validation for event names and property values before sending to Braze
- Consider implementing maximum length limits for custom attributes
Run this script to check for potential PII logging:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for logging statements that might expose PII rg -n "logger\.(debug|error|info|warn).*(\{|user|email|phone|address)" . # Check for direct access to sensitive user properties ast-grep --pattern 'context?.traits?.email' . ast-grep --pattern 'context?.traits?.phone' .Length of output: 80017
Script:
#!/bin/bash # Search for stringifyWithoutCircularV1 usage in logging rg -n "stringifyWithoutCircularV1.*logger" . # Look for input validation/sanitization functions ast-grep --pattern 'function $name($_) { $$$ sanitize $$$ }' # Check for other potential PII logging rg -n "logger\.(error|debug|info).*(\{.*traits|\{.*user|\{.*email|\{.*phone)" .Length of output: 65734
Script:
#!/bin/bash # Search for input validation/sanitization functions ast-grep --pattern 'function $name($param) { $$$ validate $$$ }' # Look for other potential PII exposures in error handling rg -n "error.*(\{.*user|\{.*email|\{.*phone|\{.*traits)" . # Check for sanitization functions rg -n "(sanitize|redact|mask|filter).*(\{.*user|\{.*email|\{.*phone)" .Length of output: 65743
🧹 Nitpick comments (2)
packages/analytics-js-integrations/src/integrations/Braze/browser.js (2)
Line range hint
266-273: Add defensive programming to event handlingWhile the simplified logic is cleaner, consider adding some safeguards:
if (eventName) { + // Normalize event name to handle null/undefined properties + const normalizedEventName = String(eventName).trim(); + if (!normalizedEventName) { + logger.debug('Skipping track call - empty event name'); + return; + } - if (eventName.toLowerCase() === 'order completed') { + if (normalizedEventName.toLowerCase() === 'order completed') { handlePurchase(properties, userId); } else { + // Ensure properties is an object + properties = properties && typeof properties === 'object' ? properties : {}; properties = handleReservedProperties(properties); - window.braze.logCustomEvent(eventName, properties); + window.braze.logCustomEvent(normalizedEventName, properties); } }
Line range hint
279-290: Align page tracking with track method improvementsFor consistency and robustness, apply similar defensive programming patterns:
if (this.isHybridModeEnabled) { return; } const eventName = rudderElement.message.name; let { properties } = rudderElement.message; + // Ensure properties is an object + properties = properties && typeof properties === 'object' ? properties : {}; properties = handleReservedProperties(properties); + // Normalize event name + const normalizedEventName = eventName ? String(eventName).trim() : ''; - if (eventName) { + if (normalizedEventName) { - window.braze.logCustomEvent(eventName, properties); + window.braze.logCustomEvent(normalizedEventName, properties); } else { window.braze.logCustomEvent('Page View', properties); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/analytics-js-integrations/src/integrations/Braze/browser.js(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Security and code quality checks
- GitHub Check: Bundle size checks
- GitHub Check: Unit Tests and Lint
- GitHub Check: Analyze (javascript-typescript)
packages/analytics-js-integrations/src/integrations/Braze/browser.js
Outdated
Show resolved
Hide resolved
size-limit report 📦
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## hotfix/integrations-braze-indentity-issue-fix #1994 +/- ##
=================================================================================
+ Coverage 61.27% 61.30% +0.02%
=================================================================================
Files 484 484
Lines 16616 16626 +10
Branches 3340 3356 +16
=================================================================================
+ Hits 10182 10192 +10
+ Misses 5213 5167 -46
- Partials 1221 1267 +46 ☔ View full report in Codecov by Sentry. |
saikumarrs
reviewed
Jan 20, 2025
packages/analytics-js-integrations/__tests__/integrations/Braze/browser.test.js
Outdated
Show resolved
Hide resolved
packages/analytics-js-integrations/__tests__/integrations/Braze/browser.test.js
Show resolved
Hide resolved
packages/analytics-js-integrations/src/integrations/Braze/browser.js
Outdated
Show resolved
Hide resolved
krishna2020
reviewed
Jan 20, 2025
utsabc
changed the base branch from
develop
to
hotfix/integrations-braze-indentity-issue-fix
|
saikumarrs
approved these changes
Jan 21, 2025
krishna2020
reviewed
Jan 21, 2025
krishna2020
approved these changes
Jan 21, 2025
utsabc
merged commit 7215304
into
hotfix/integrations-braze-indentity-issue-fix
10 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Description
Anonymous User Identification
Profile Management
We achieve the above by making the following Key Changes
Linear Ticket
Cross Browser Tests
Please confirm you have tested for the following browsers:
Sanity Suite
Security
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
addAliasmethod in the test suite to enhance testing coverage.