Skip to content

Require domain parameter for Intercom cookie deletion #366

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

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

DamonFstr
Copy link
Contributor

@DamonFstr DamonFstr commented Mar 11, 2025

This change makes the domain parameter required in both intercom_shutdown_helper and intercom_shutdown methods to ensure proper cookie deletion. Intercom sets cookies with a domain prefix (e.g. ".domain.com"), and we need to match this format when deleting cookies as Chrome/browsers very particular about cookie attributes. If you don't set a specific domain the cookie doesn't get deleted.

Key changes:

  • Make domain parameter required in both methods
  • Ensure domain has a leading dot to match Intercom's cookie format
  • Handle localhost domain as a special case
  • Update tests to verify correct domain handling
  • Improve documentation with clear parameter requirements
  • Fix typos in comments

This ensures that Intercom cookies are properly cleared during logout.

Note: This will likely be a major version change if we decide to ship it since it's a breaking change.

@DamonFstr DamonFstr requested a review from Copilot March 11, 2025 11:16
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Overview

This PR makes the domain parameter required for Intercom cookie deletion to ensure that cookies are cleared correctly in browsers, especially with Intercom’s domain-prefixed cookies. It also enforces a leading dot in the domain (except for 'localhost') and updates tests and documentation accordingly.

  • Domain parameter is now required in intercom_shutdown_helper and intercom_shutdown.
  • A leading dot is added automatically to the domain if missing, with a special case for 'localhost'.
  • Tests and comments have been updated to reflect and validate these changes.

Reviewed Changes

File Description
lib/intercom-rails/shutdown_helper.rb Updated the shutdown helper methods to require a domain and normalize it with a leading dot.
spec/shutdown_helper_spec.rb Updated tests to cover the new domain parameter requirement and special handling for localhost.

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

lib/intercom-rails/shutdown_helper.rb:14

  • The removal of the type check for ActionDispatch::Cookies::CookieJar (and its fallback for a controller) changes the expected behavior when a non-CookieJar object is passed. Ensure that only a CookieJar instance is accepted or update both the documentation and tests to reflect this stricter requirement.
unless domain == 'localhost'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant