🐛 [RUM-11596] fix cookie domain within pages with empty location (#3866)

* 🐛 [RUM-11596] fix cookie domain within pages with empty location

* ✅♻️ [e2e] rename Server.url to Server.origin

* ✅🔥 [e2e] remove crossOriginUrl

As it's not too much used, let's just use the server origin directly.

* ✅♻️ [e2e] `baseUrl` includes the path

* ✅ [RUM-11596] add e2e tests

Author: BenoitZugmeyer

packages/core/src/browser/cookie.spec.ts

@@ -0,0 +1,53 @@
+import { mockCookies } from '../../test'
+import { getCurrentSite, resetGetCurrentSite } from './cookie'
+
+describe('cookie', () => {
+  describe('getCurrentSite', () => {
+    beforeEach(() => {
+      resetGetCurrentSite()
+    })
+
+    it('returns the eTLD+1 for example.com', () => {
+      mockCookies()
+      expect(getCurrentSite('example.com')).toBe('example.com')
+    })
+
+    it('returns the eTLD+1 for example.co.uk', () => {
+      mockCookies({
+        filter: (cookie) => cookie.domain !== '.co.uk',
+      })
+      expect(getCurrentSite('example.co.uk')).toBe('example.co.uk')
+    })
+
+    it('returns the eTLD+1 for foo.bar.baz.example.com', () => {
+      mockCookies()
+      expect(getCurrentSite('foo.bar.baz.example.com')).toBe('example.com')
+    })
+
+    it('does not left any cookies', () => {
+      const { getCookies } = mockCookies()
+      expect(getCurrentSite('example.com')).toBe('example.com')
+      expect(getCookies()).toEqual([])
+    })
+
+    it('falls back to the referrer when the hostname is empty', () => {
+      mockCookies()
+      expect(getCurrentSite('', 'https://example.com')).toBe('example.com')
+    })
+
+    it('returns undefined when the referrer is empty', () => {
+      mockCookies()
+      expect(getCurrentSite('', '')).toBeUndefined()
+    })
+
+    it('caches the result', () => {
+      const { setter } = mockCookies()
+
+      expect(getCurrentSite('example.com')).toBe('example.com')
+      expect(setter).toHaveBeenCalledTimes(2)
+
+      expect(getCurrentSite('example.com')).toBe('example.com')
+      expect(setter).toHaveBeenCalledTimes(2)
+    })
+  })
+})

packages/core/src/browser/cookie.ts

@@ -1,6 +1,7 @@
 import { display } from '../tools/display'
 import { ONE_MINUTE, ONE_SECOND } from '../tools/utils/timeUtils'
 import { findCommaSeparatedValue, findCommaSeparatedValues, generateUUID } from '../tools/utils/stringUtils'
+import { buildUrl } from '../tools/utils/urlPolyfill'
 
 export interface CookieOptions {
   secure?: boolean
@@ -70,21 +71,37 @@ export function areCookiesAuthorized(options: CookieOptions): boolean {
  * https://web.dev/same-site-same-origin/#site
  */
 let getCurrentSiteCache: string | undefined
-export function getCurrentSite() {
+export function getCurrentSite(hostname = location.hostname, referrer = document.referrer): string | undefined {
   if (getCurrentSiteCache === undefined) {
-    // Use a unique cookie name to avoid issues when the SDK is initialized multiple times during
-    // the test cookie lifetime
-    const testCookieName = `dd_site_test_${generateUUID()}`
-    const testCookieValue = 'test'
+    const defaultHostName = getCookieDefaultHostName(hostname, referrer)
+    if (defaultHostName) {
+      // Use a unique cookie name to avoid issues when the SDK is initialized multiple times during
+      // the test cookie lifetime
+      const testCookieName = `dd_site_test_${generateUUID()}`
+      const testCookieValue = 'test'
 
-    const domainLevels = window.location.hostname.split('.')
-    let candidateDomain = domainLevels.pop()!
-    while (domainLevels.length && !getCookie(testCookieName)) {
-      candidateDomain = `${domainLevels.pop()!}.${candidateDomain}`
-      setCookie(testCookieName, testCookieValue, ONE_SECOND, { domain: candidateDomain })
+      const domainLevels = defaultHostName.split('.')
+      let candidateDomain = domainLevels.pop()!
+      while (domainLevels.length && !getCookie(testCookieName)) {
+        candidateDomain = `${domainLevels.pop()!}.${candidateDomain}`
+        setCookie(testCookieName, testCookieValue, ONE_SECOND, { domain: candidateDomain })
+      }
+      deleteCookie(testCookieName, { domain: candidateDomain })
+      getCurrentSiteCache = candidateDomain
     }
-    deleteCookie(testCookieName, { domain: candidateDomain })
-    getCurrentSiteCache = candidateDomain
   }
+
   return getCurrentSiteCache
 }
+
+function getCookieDefaultHostName(hostname: string, referrer: string) {
+  try {
+    return hostname || buildUrl(referrer).hostname
+  } catch {
+    // Ignore
+  }
+}
+
+export function resetGetCurrentSite() {
+  getCurrentSiteCache = undefined
+}

packages/core/src/domain/session/sessionManager.ts

@@ -238,7 +238,7 @@ async function getSessionCookies(): Promise<{ count: number; domain: string }> {
 
   return {
     count: sessionCookies.length,
-    domain: getCurrentSite(),
+    domain: getCurrentSite() || 'undefined',
     ...sessionCookies,
   }
 }

packages/core/src/domain/session/storeStrategies/sessionInCookie.ts

@@ -16,7 +16,9 @@ import { SESSION_STORE_KEY } from './sessionStoreStrategy'
 
 export function selectCookieStrategy(initConfiguration: InitConfiguration): SessionStoreStrategyType | undefined {
   const cookieOptions = buildCookieOptions(initConfiguration)
-  return areCookiesAuthorized(cookieOptions) ? { type: SessionPersistence.COOKIE, cookieOptions } : undefined
+  return cookieOptions && areCookiesAuthorized(cookieOptions)
+    ? { type: SessionPersistence.COOKIE, cookieOptions }
+    : undefined
 }
 
 export function initCookieStrategy(configuration: Configuration, cookieOptions: CookieOptions): SessionStoreStrategy {
@@ -63,7 +65,7 @@ export function retrieveSessionCookie(): SessionState {
   return sessionState
 }
 
-export function buildCookieOptions(initConfiguration: InitConfiguration) {
+export function buildCookieOptions(initConfiguration: InitConfiguration): CookieOptions | undefined {
   const cookieOptions: CookieOptions = {}
 
   cookieOptions.secure =
@@ -72,7 +74,11 @@ export function buildCookieOptions(initConfiguration: InitConfiguration) {
   cookieOptions.partitioned = !!initConfiguration.usePartitionedCrossSiteSessionCookie
 
   if (initConfiguration.trackSessionAcrossSubdomains) {
-    cookieOptions.domain = getCurrentSite()
+    const currentSite = getCurrentSite()
+    if (!currentSite) {
+      return
+    }
+    cookieOptions.domain = currentSite
   }
 
   return cookieOptions

packages/core/test/cookie.ts

@@ -10,3 +10,99 @@ export function expireCookie() {
 export function getSessionState(sessionStoreKey: string) {
   return toSessionState(getCookie(sessionStoreKey))
 }
+
+interface Cookie {
+  name: string
+  value: string
+  domain?: string
+  path?: string
+  expires?: number
+  secure?: boolean
+  sameSite?: 'strict' | 'lax' | 'none'
+  partitioned?: boolean
+}
+
+export function mockCookies({ filter }: { filter?: (cookie: Cookie) => boolean } = {}) {
+  let cookies: Cookie[] = []
+
+  const documentPrototype =
+    'cookie' in Document.prototype
+      ? Document.prototype
+      : // Firefox 67 doesn't define `cookie` on `Document.prototype`
+        HTMLDocument.prototype
+
+  const getter = spyOnProperty(documentPrototype, 'cookie', 'get').and.callFake(() => {
+    removeExpiredCookies()
+    return cookies.map((cookie) => `${cookie.name}=${cookie.value}`).join(';')
+  })
+
+  const setter = spyOnProperty(documentPrototype, 'cookie', 'set').and.callFake((cookieString) => {
+    const cookie = parseSingleCookieString(cookieString)
+
+    if (filter && !filter(cookie)) {
+      return
+    }
+
+    const matchingCookieIndex = cookies.findIndex(
+      (otherCookie) =>
+        cookie.name === otherCookie.name && cookie.domain === otherCookie.domain && cookie.path === otherCookie.path
+    )
+    if (matchingCookieIndex !== -1) {
+      cookies[matchingCookieIndex] = cookie
+    } else {
+      cookies.push(cookie)
+    }
+    removeExpiredCookies()
+  })
+
+  function removeExpiredCookies() {
+    cookies = cookies.filter((cookie) => cookie.expires && cookie.expires > Date.now())
+  }
+
+  return {
+    getCookies: () => {
+      removeExpiredCookies()
+      return cookies
+    },
+    getter,
+    setter,
+  }
+}
+
+function parseSingleCookieString(cookieString: string) {
+  const parts = cookieString.split(';')
+  const [name, value] = parts.shift()!.split('=')
+  const parsedCookie: Cookie = {
+    name,
+    value,
+  }
+
+  for (const part of parts) {
+    const parsedPart = part.match(/^\s*([^=]*)(?:=(.*))?/)
+    if (!parsedPart) {
+      continue
+    }
+
+    const name = parsedPart[1].toLowerCase()
+    const value: string | undefined = parsedPart[2]
+
+    if (value) {
+      if (name === 'domain') {
+        parsedCookie.domain = value.startsWith('.') ? value : `.${value}`
+      } else if (name === 'path') {
+        parsedCookie.path = value
+      } else if (name === 'expires') {
+        parsedCookie.expires = new Date(value).getTime()
+      } else if (name === 'max-age') {
+        parsedCookie.expires = Date.now() + Number(value) * 1000
+      } else if (name === 'samesite') {
+        parsedCookie.sameSite = value as Cookie['sameSite']
+      }
+    } else if (name === 'partitioned') {
+      parsedCookie.partitioned = true
+    } else if (name === 'secure') {
+      parsedCookie.secure = true
+    }
+  }
+  return parsedCookie
+}

test/e2e/lib/framework/createTest.ts

@@ -18,6 +18,7 @@ import { html, DEFAULT_SETUPS, npmSetup, reactSetup } from './pageSetups'
 import { createIntakeServerApp } from './serverApps/intake'
 import { createMockServerApp } from './serverApps/mock'
 import type { Extension } from './createExtension'
+import { isBrowserStack } from './environment'
 
 interface LogsWorkerOptions {
   importScript?: boolean
@@ -30,7 +31,6 @@ export function createTest(title: string) {
 
 interface TestContext {
   baseUrl: string
-  crossOriginUrl: string
   intakeRegistry: IntakeRegistry
   servers: Servers
   page: Page
@@ -63,6 +63,7 @@ class TestBuilder {
     logsConfiguration?: LogsInitConfiguration
   } = {}
   private useServiceWorker: boolean = false
+  private hostName?: string
 
   constructor(private title: string) {}
 
@@ -153,6 +154,8 @@ class TestBuilder {
 
       const options = isModule ? '{ type: "module" }' : '{}'
 
+      // Service workers require HTTPS or localhost due to browser security restrictions
+      this.hostName = 'localhost'
       this.withBody(html`
         <script>
           if (!window.myServiceWorker && 'serviceWorker' in navigator) {
@@ -167,6 +170,11 @@ class TestBuilder {
     return this
   }
 
+  withHostName(hostName: string) {
+    this.hostName = hostName
+    return this
+  }
+
   run(runner: TestRunner) {
     const setupOptions: SetupOptions = {
       body: this.body,
@@ -185,7 +193,7 @@ class TestBuilder {
       },
       testFixture: this.testFixture,
       extension: this.extension,
-      useServiceWorker: this.useServiceWorker,
+      hostName: this.hostName,
     }
 
     if (this.alsoRunWithRumSlim) {
@@ -238,6 +246,20 @@ function declareTest(title: string, setupOptions: SetupOptions, factory: SetupFa
     addTag('test.browserName', browserName)
     addTestOptimizationTags(test.info().project.metadata as BrowserConfiguration)
 
+    test.skip(
+      !!setupOptions.hostName && setupOptions.hostName.endsWith('.localhost') && isBrowserStack,
+      // Skip those tests on BrowserStack because it doesn't support localhost subdomains. As a
+      // workaround we could use normal domains and use either:
+      // * the BrowserStack proxy capabilities -> not tried, but this sounds more complex because
+      //   we also want to run tests outside of BrowserStack
+      // * the Playwright proxy capabilities -> tried and it seems to fail because of mismatch
+      //   version between Playwright local and BrowserStack versions
+      // * a "ngrok-like" service -> not tried yet (it sounds more complex)
+      //
+      // See https://www.browserstack.com/support/faq/local-testing/local-exceptions/i-face-issues-while-testing-localhost-urls-or-private-servers-in-safari-on-macos-os-x-and-ios
+      'Localhost subdomains are not supported in BrowserStack'
+    )
+
     const title = test.info().titlePath.join(' > ')
     setupOptions.context.test_name = title
 
@@ -268,15 +290,16 @@ function createTestContext(
   browserContext: BrowserContext,
   browserLogsManager: BrowserLogsManager,
   browserName: TestContext['browserName'],
-  { basePath, useServiceWorker }: SetupOptions
+  { basePath, hostName }: SetupOptions
 ): TestContext {
-  const url = servers.base.url
-  const hostname = useServiceWorker ? url.replace(/http:\/\/[^:]+:/, 'http://localhost:') : url
+  const baseUrl = new URL(basePath, servers.base.origin)
+
+  if (hostName) {
+    baseUrl.hostname = hostName
+  }
 
   return {
-    // Service workers require HTTPS or localhost due to browser security restrictions
-    baseUrl: hostname + basePath,
-    crossOriginUrl: servers.crossOrigin.url,
+    baseUrl: baseUrl.href,
     intakeRegistry: new IntakeRegistry(),
     servers,
     page,

test/e2e/lib/framework/environment.ts

@@ -0,0 +1,2 @@
+export const isBrowserStack = Boolean(process.env.BROWSER_STACK)
+export const isContinuousIntegration = Boolean(process.env.CI)

test/e2e/lib/framework/flushEvents.ts

@@ -21,6 +21,6 @@ export async function flushEvents(page: Page) {
   // The issue mainly occurs with local e2e tests (not browserstack), because the network latency is
   // very low (same machine), so the request resolves very quickly. In real life conditions, this
   // issue is mitigated, because requests will likely take a few milliseconds to reach the server.
-  await page.goto(`${servers.base.url}/ok?duration=200`)
+  await page.goto(`${servers.base.origin}/ok?duration=200`)
   await waitForServersIdle()
 }

test/e2e/lib/framework/httpServers.ts

@@ -15,7 +15,7 @@ export type MockServerApp = ServerApp & {
 }
 
 export interface Server<App extends ServerApp> {
-  url: string
+  origin: string
   app: App
   bindServerApp(serverApp: App): void
   waitForIdle(): Promise<void>
@@ -67,7 +67,7 @@ async function createServer<App extends ServerApp>(): Promise<Server<App>> {
       }
       return serverApp
     },
-    url: `http://${address}:${port}`,
+    origin: `http://${address}:${port}`,
     waitForIdle: createServerIdleWaiter(server),
   }
 }