fix: cross domain tracking

seeratawan01 commented 2 weeks ago

PR Type

Enhancement, Tests

Description

Modified the cookie setting logic to use the document's protocol to determine the secure flag.
Simplified the domain extraction logic in the CookieManager class and added console logs for debugging.
Added a new HTML example for cross-domain tracking, including SDK and test scripts.
Updated existing HTML examples to include links to the new cross-domain tracking example.

Changes walkthrough 📝

Relevant files

Enhancement

client.ts `Modify cookie setting to use protocol for secure flag` packages/javascript-sdk/src/core/client.ts - Updated cookie setting to consider protocol for secure flag.	+1/-1
cookie.ts `Simplify domain extraction and add debug logs` packages/javascript-sdk/src/utils/cookie.ts Added console logs for debugging cookie settings. Simplified domain extraction logic. Removed redundant domain extraction methods.	+16/-52

Tests

cross-domain.html `Add cross-domain tracking example` packages/javascript-sdk/examples/cross-domain.html Added new example for cross-domain tracking. Included SDK and test scripts for event tracking.	+67/-0
form-tracking.html `Update form tracking example with cross-domain link` packages/javascript-sdk/examples/form-tracking.html - Added link to cross-domain tracking example.	+1/-0
index.html `Update index example with cross-domain link` packages/javascript-sdk/examples/index.html - Added link to cross-domain tracking example.	+1/-0

💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

github-actions[bot] commented 2 weeks ago

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🏅 Score: 82

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review

Secure Cookie Flag
The logic for setting the secure flag on cookies might not correctly handle all scenarios, especially with mixed content (HTTP and HTTPS). Console Logging
Console logging in production code can expose sensitive information and should be removed or replaced with a proper logging framework. Domain Extraction Logic
The new domain extraction logic might not correctly handle all edge cases, such as new or less common top-level domains.

Code feedback:

relevant file	packages/javascript-sdk/src/core/client.ts
suggestion	Consider using a more robust method to determine the secure flag based on page origin rather than protocol alone. This can help avoid issues with mixed content. [important]
relevant line	this.cookieManager?.set(cookieName, id, tenYearsInDays, document.location.protocol !== "http:", false);

relevant file	packages/javascript-sdk/src/utils/cookie.ts
suggestion	Remove console logging or replace it with a more secure logging mechanism that does not expose potentially sensitive information. [important]
relevant line	console.log('Setting cookie', name, value, expirationDays, secure, httpOnly);

relevant file	packages/javascript-sdk/src/utils/cookie.ts
suggestion	Refactor the domain extraction logic to handle new top-level domains and ensure accuracy in extracting the correct domain part. Consider using a well-maintained library for domain parsing. [important]
relevant line	return '.' + domainParts.slice(-2).join('.');

github-actions[bot] commented 2 weeks ago

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
Security	Improve security by ensuring cookies are set with the 'Secure' flag only when the protocol is HTTPS ___ Ensure the `document.location.protocol` check for setting the `secure` flag in cookies is correctly handling edge cases and mixed content scenarios. [packages/javascript-sdk/src/core/client.ts [235]](https://github.com/usermaven/usermaven-js/pull/124/files#diff-cf1b46df259d6f5f7aa4358f3de9b0e4ca7051836cc3f55729ad1c56a5f7ac40R235-R235) ```diff -this.cookieManager?.set(cookieName, id, tenYearsInDays, document.location.protocol !== "http:", false); +this.cookieManager?.set(cookieName, id, tenYearsInDays, document.location.protocol === "https:", false); ``` Suggestion importance[1-10]: 9 Why: This suggestion correctly identifies a security improvement by ensuring the 'Secure' flag is set only when the protocol is HTTPS, which is crucial for preventing cookies from being sent over insecure connections.	9
Best practice	Enhance security and performance by removing console logs ___ Remove console logging from production code to prevent leaking sensitive information and to improve performance. [packages/javascript-sdk/src/utils/cookie.ts [6-10]](https://github.com/usermaven/usermaven-js/pull/124/files#diff-304a2517b554eaf957917f59494746ca14cd003a21f6076e2db28255dcafe5ccR6-R10) ```diff -console.log(this.cookieDomain); -console.log('Setting cookie', name, value, expirationDays, secure, httpOnly); +// console.log(this.cookieDomain); +// console.log('Setting cookie', name, value, expirationDays, secure, httpOnly); ``` Suggestion importance[1-10]: 7 Why: Removing console logs from production code is a good practice to prevent potential information leakage and improve performance, making this a relevant and useful suggestion.	7
Enhancement	Improve domain extraction logic to dynamically handle a wider range of top-level domains ___ Refactor the `extractRoot` method to handle more generic top-level domains dynamically by using a maintained list or library. [packages/javascript-sdk/src/utils/cookie.ts [60-65]](https://github.com/usermaven/usermaven-js/pull/124/files#diff-304a2517b554eaf957917f59494746ca14cd003a21f6076e2db28255dcafe5ccR60-R65) ```diff -const knownCcTLDs = ['co.uk', 'com.au', 'co.jp']; // Add more as needed -const lastTwoParts = domainParts.slice(-2).join('.'); -if (knownCcTLDs.includes(lastTwoParts)) { - return '.' + domainParts.slice(-3).join('.'); -} -return '.' + domainParts.slice(-2).join('.'); +return '.' + this.dynamicTldExtractor(domainParts); ``` Suggestion importance[1-10]: 6 Why: While the suggestion to use a dynamic TLD extractor is valid, it lacks specificity on implementation details. It addresses a potential enhancement but requires further clarification on how to achieve it.	6
Enhancement	Enhance domain parsing to support complex domain structures and new TLDs ___ Ensure that the `extractRoot` method correctly handles domains with more than three segments and potential new top-level domains. [packages/javascript-sdk/src/utils/cookie.ts [58-65]](https://github.com/usermaven/usermaven-js/pull/124/files#diff-304a2517b554eaf957917f59494746ca14cd003a21f6076e2db28255dcafe5ccR58-R65) ```diff if (domainLength > 2) { - const knownCcTLDs = ['co.uk', 'com.au', 'co.jp']; // Add more as needed - const lastTwoParts = domainParts.slice(-2).join('.'); - if (knownCcTLDs.includes(lastTwoParts)) { - return '.' + domainParts.slice(-3).join('.'); - } - return '.' + domainParts.slice(-2).join('.'); + return '.' + this.handleComplexDomains(domainParts); } ``` Suggestion importance[1-10]: 5 Why: The suggestion aims to improve domain parsing logic, which is beneficial for handling complex domain structures. However, it lacks detail on the implementation, reducing its immediate applicability.	5

usermaven / usermaven-js