fix: error handle cookie operation#2475
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughTop-level cookie-setting attempts are now wrapped in try/catch: each domain try sets a cookie, checks it, clears the cookie and returns the domain on success; exceptions are swallowed and the loop continues. If no domain succeeds the function returns an empty string. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as StoreManager.getTopDomain
participant DomainLoop as DomainLevels (iterable)
participant CookieAPI as Cookie set/check/clear
Caller->>DomainLoop: iterate domain levels
loop per domain
DomainLoop->>CookieAPI: try set cookie on domain
alt set/check succeeds
Note right of CookieAPI#dff0d8: success path
CookieAPI-->>DomainLoop: cookie set & check true
DomainLoop->>CookieAPI: clear cookie
DomainLoop-->>Caller: return domain (done)
else exception thrown
Note right of CookieAPI#fbe8e8: error swallowed
CookieAPI-->>DomainLoop: throws (caught)
DomainLoop-->>DomainLoop: continue next domain (best-effort cleanup attempted)
end
end
DomainLoop-->>Caller: no domain found (returns "")
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks (1 passed, 1 warning, 1 inconclusive)❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull Request Overview
This PR adds error handling to cookie operations during top-level domain detection to prevent SDK initialization failures in environments with cookie access restrictions.
- Wraps cookie setting and checking operations in a try-catch block
- Continues domain level iteration when cookie operations fail instead of crashing
- Improves SDK robustness across different browser security configurations
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![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 (1)
packages/analytics-js/src/services/StoreManager/top-domain/index.ts (1)
59-60: Nit: avoid shadowing the function name withdomainlocalRename the loop variable to improve readability (e.g.,
candidateDomain).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/analytics-js/src/services/StoreManager/top-domain/index.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/002-javascript-typescript.mdc)
Code must work in browsers AND service workers (no Node.js APIs)
Files:
packages/analytics-js/src/services/StoreManager/top-domain/index.ts
packages/*/src/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place all package source code under packages/[package-name]/src/
Files:
packages/analytics-js/src/services/StoreManager/top-domain/index.ts
packages/analytics-js/src/**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use @preact/signals-core for reactive state in the core SDK (analytics-js)
Files:
packages/analytics-js/src/services/StoreManager/top-domain/index.ts
packages/!(analytics-js-common)/**/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Prefer shared TypeScript types from analytics-js-common across packages
Files:
packages/analytics-js/src/services/StoreManager/top-domain/index.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Adhere to the repository’s ESLint configuration for JavaScript/TypeScript code
Files:
packages/analytics-js/src/services/StoreManager/top-domain/index.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1759
File: packages/analytics-js-cookies/README.md:34-44
Timestamp: 2024-06-25T09:00:22.273Z
Learning: User saikumarrs prefers the current state of the documentation in the README.md of the `analytics-js-cookies` package without merging the blockquote sections or adding example usage for error handling as suggested.
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1759
File: packages/analytics-js-cookies/README.md:34-44
Timestamp: 2024-10-08T15:52:59.819Z
Learning: User saikumarrs prefers the current state of the documentation in the README.md of the `analytics-js-cookies` package without merging the blockquote sections or adding example usage for error handling as suggested.
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1754
File: packages/analytics-js-service-worker/README.md:0-0
Timestamp: 2024-10-08T15:52:59.819Z
Learning: saikumarrs prefers simplified language in documentation, avoiding redundant phrases.
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1754
File: packages/analytics-js-service-worker/README.md:0-0
Timestamp: 2024-06-14T09:50:33.511Z
Learning: saikumarrs prefers simplified language in documentation, avoiding redundant phrases.
📚 Learning: 2024-10-28T08:03:12.163Z
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1902
File: packages/analytics-js-plugins/src/utilities/eventsDelivery.ts:0-0
Timestamp: 2024-10-28T08:03:12.163Z
Learning: In `packages/analytics-js-plugins/src/utilities/eventsDelivery.ts`, the issue regarding inconsistent error handling approaches in `getDMTDeliveryPayload` is no longer valid.
Applied to files:
packages/analytics-js/src/services/StoreManager/top-domain/index.ts
📚 Learning: 2024-10-08T15:52:59.819Z
Learnt from: MoumitaM
PR: rudderlabs/rudder-sdk-js#1777
File: packages/analytics-js/src/components/configManager/util/validate.ts:0-0
Timestamp: 2024-10-08T15:52:59.819Z
Learning: The function `isTopLevelDomain` in `validate.ts` was renamed to `isWebpageTopLevelDomain` for clarity.
Applied to files:
packages/analytics-js/src/services/StoreManager/top-domain/index.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Bundle size checks
- GitHub Check: Code quality checks
- GitHub Check: Unit Tests and Lint
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
size-limit report 📦
|
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/analytics-js/src/services/StoreManager/top-domain/index.ts (1)
65-75: Avoid false positives; compare exact unique value and always cleanup in finallySet a unique test value, verify equality, and move deletion to a finally block so cleanup runs on all paths.
- try { - // Set cookie on domain - cookie(cname, 1, opts); - - // If successful - if (cookie(cname)) { - // Remove cookie from domain - cookie(cname, null, opts); - return domain; - } - } catch { - // Silently continue to next domain level if cookie access is restricted or setting fails - // Best-effort cleanup to avoid leaking the test cookie - try { - cookie(cname, null, opts); - } catch { - // Ignore if we are unable to delete the cookie - } - } + try { + // Set a unique test value to avoid pre-existing-cookie false positives + const testValue = `tld_test.${Date.now()}.${Math.random().toString(36).slice(2)}`; + cookie(cname, testValue, opts); + // Verify exact value + if (cookie(cname) === testValue) { + return domain; + } + } catch { + // Silently continue to next domain level if cookie access is restricted or setting fails + } finally { + // Best-effort cleanup + try { cookie(cname, null, opts); } catch {} + }
🧹 Nitpick comments (1)
packages/analytics-js/src/services/StoreManager/top-domain/index.ts (1)
65-83: Explicitly setpath: '/'in cookie opts for consistent set/deleteAdd
path: '/'to theoptsobject inpackages/analytics-js/src/services/StoreManager/top-domain/index.tsto ensure the test cookie is always removed successfully, regardless of the current URL path.const opts = { domain: `${domain.indexOf('localhost') !== -1 ? '' : '.'}${domain}`, + path: '/', // ensure consistent set/delete across browsers };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/analytics-js/src/services/StoreManager/top-domain/index.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/002-javascript-typescript.mdc)
Code must work in browsers AND service workers (no Node.js APIs)
Files:
packages/analytics-js/src/services/StoreManager/top-domain/index.ts
packages/*/src/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place all package source code under packages/[package-name]/src/
Files:
packages/analytics-js/src/services/StoreManager/top-domain/index.ts
packages/analytics-js/src/**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use @preact/signals-core for reactive state in the core SDK (analytics-js)
Files:
packages/analytics-js/src/services/StoreManager/top-domain/index.ts
packages/!(analytics-js-common)/**/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Prefer shared TypeScript types from analytics-js-common across packages
Files:
packages/analytics-js/src/services/StoreManager/top-domain/index.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Adhere to the repository’s ESLint configuration for JavaScript/TypeScript code
Files:
packages/analytics-js/src/services/StoreManager/top-domain/index.ts
🧠 Learnings (4)
📓 Common learnings
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1754
File: packages/analytics-js-service-worker/README.md:0-0
Timestamp: 2024-10-08T15:52:59.819Z
Learning: saikumarrs prefers simplified language in documentation, avoiding redundant phrases.
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1754
File: packages/analytics-js-service-worker/README.md:0-0
Timestamp: 2024-06-14T09:50:33.511Z
Learning: saikumarrs prefers simplified language in documentation, avoiding redundant phrases.
📚 Learning: 2024-10-08T15:52:59.819Z
Learnt from: MoumitaM
PR: rudderlabs/rudder-sdk-js#1777
File: packages/analytics-js/src/components/configManager/util/validate.ts:0-0
Timestamp: 2024-10-08T15:52:59.819Z
Learning: The function `isTopLevelDomain` in `validate.ts` was renamed to `isWebpageTopLevelDomain` for clarity.
Applied to files:
packages/analytics-js/src/services/StoreManager/top-domain/index.ts
📚 Learning: 2024-06-25T09:00:22.273Z
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1759
File: packages/analytics-js-cookies/README.md:34-44
Timestamp: 2024-06-25T09:00:22.273Z
Learning: User saikumarrs prefers the current state of the documentation in the README.md of the `analytics-js-cookies` package without merging the blockquote sections or adding example usage for error handling as suggested.
Applied to files:
packages/analytics-js/src/services/StoreManager/top-domain/index.ts
📚 Learning: 2024-11-08T06:58:29.573Z
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1907
File: packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts:172-174
Timestamp: 2024-11-08T06:58:29.573Z
Learning: The function `onError` in `packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts` is acceptable as currently implemented, and refactoring suggestions are not required unless necessary.
Applied to files:
packages/analytics-js/src/services/StoreManager/top-domain/index.ts
🧬 Code graph analysis (1)
packages/analytics-js/src/services/StoreManager/top-domain/index.ts (1)
packages/analytics-js-common/src/component-cookie/index.ts (1)
cookie(138-138)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Code quality checks
- GitHub Check: Bundle size checks
- GitHub Check: Unit Tests and Lint
🔇 Additional comments (1)
packages/analytics-js/src/services/StoreManager/top-domain/index.ts (1)
76-82: LGTM: descriptive catch and best-effort cleanupClear, minimal comment and safe nested cleanup. Matches the PR goal without adding risk.
saikumarrs
deleted the
feature/sdk-3883-error-handle-cookie-access-during-initialisation
branch
PR Description
I have error handled the cookie test operations that are used to determine the top-level domain of the webpage. Without this fix, the SDK would not initialise properly in environments with security restrictions around cookie access.
Linear task (optional)
https://linear.app/rudderstack/issue/SDK-3883/error-handle-cookie-access-during-initialisation
Cross Browser Tests
Please confirm you have tested for the following browsers:
Sanity Suite
Security
Summary by CodeRabbit