From eabacaf153472c2654f08f86a075e880b9376f93 Mon Sep 17 00:00:00 2001 From: Thomas Lebeau <1926949+thomas-lebeau@users.noreply.github.com> Date: Wed, 18 Feb 2026 09:27:39 +0100 Subject: [PATCH 1/3] chore(session): drop the pre-2020 cookie format migration Mirrors upstream datadog/browser-sdk c2bba2de8 ("Remove old cookie migration", 2026-02-18), same three files, same 121 deletions. The migration read the legacy `_dd` / `_dd_r` / `_dd_l` cookies and folded them into today's single `_dd_s`. Nothing has written those cookies since 2020-04-07 (upstream #342), and this SDK line never did: `_dd_s` is already the store key in v0.0.1. tryOldCookiesMigration is not exported from any package entry, so nothing outside the cookie strategy can reach it. It also mattered for a second reason. It was the only place that set `session.id` without stamping `created`, and since `created` is only written when a new id is generated, a session that came through it could never acquire one -- which is exactly the state the next commit stops trusting. --- .../session/oldCookiesMigration.spec.ts | 76 ------------------- .../src/domain/session/oldCookiesMigration.ts | 42 ---------- .../storeStrategies/sessionInCookie.ts | 3 - 3 files changed, 121 deletions(-) delete mode 100644 packages/core/src/domain/session/oldCookiesMigration.spec.ts delete mode 100644 packages/core/src/domain/session/oldCookiesMigration.ts diff --git a/packages/core/src/domain/session/oldCookiesMigration.spec.ts b/packages/core/src/domain/session/oldCookiesMigration.spec.ts deleted file mode 100644 index 1404733549..0000000000 --- a/packages/core/src/domain/session/oldCookiesMigration.spec.ts +++ /dev/null @@ -1,76 +0,0 @@ -import { getCookie, resetInitCookies, setCookie } from '../../browser/cookie' -import { getSessionState } from '../../../test' -import type { Configuration } from '../configuration' -import { - OLD_LOGS_COOKIE_NAME, - OLD_RUM_COOKIE_NAME, - OLD_SESSION_COOKIE_NAME, - tryOldCookiesMigration, -} from './oldCookiesMigration' -import { SESSION_EXPIRATION_DELAY } from './sessionConstants' -import { initCookieStrategy } from './storeStrategies/sessionInCookie' -import type { SessionStoreStrategy } from './storeStrategies/sessionStoreStrategy' -import { SESSION_STORE_KEY } from './storeStrategies/sessionStoreStrategy' -const DEFAULT_INIT_CONFIGURATION = { trackAnonymousUser: true } as Configuration - -describe('old cookies migration', () => { - let sessionStoreStrategy: SessionStoreStrategy - - beforeEach(() => { - sessionStoreStrategy = initCookieStrategy(DEFAULT_INIT_CONFIGURATION, {}) - resetInitCookies() - }) - - afterEach(() => { - resetInitCookies() - }) - - it('should not touch current cookie', () => { - setCookie(SESSION_STORE_KEY, 'id=abcde&rum=0&logs=1&expire=1234567890', SESSION_EXPIRATION_DELAY) - - tryOldCookiesMigration(sessionStoreStrategy) - - expect(getCookie(SESSION_STORE_KEY)).toBe('id=abcde&rum=0&logs=1&expire=1234567890') - }) - - it('should create new cookie from old cookie values', () => { - setCookie(OLD_SESSION_COOKIE_NAME, 'abcde', SESSION_EXPIRATION_DELAY) - setCookie(OLD_LOGS_COOKIE_NAME, '1', SESSION_EXPIRATION_DELAY) - setCookie(OLD_RUM_COOKIE_NAME, '0', SESSION_EXPIRATION_DELAY) - - tryOldCookiesMigration(sessionStoreStrategy) - - expect(getSessionState(SESSION_STORE_KEY).id).toBe('abcde') - expect(getSessionState(SESSION_STORE_KEY).rum).toBe('0') - expect(getSessionState(SESSION_STORE_KEY).logs).toBe('1') - expect(getSessionState(SESSION_STORE_KEY).expire).toMatch(/\d+/) - }) - - it('should create new cookie from a single old cookie', () => { - setCookie(OLD_RUM_COOKIE_NAME, '0', SESSION_EXPIRATION_DELAY) - - tryOldCookiesMigration(sessionStoreStrategy) - expect(getSessionState(SESSION_STORE_KEY).id).not.toBeDefined() - expect(getSessionState(SESSION_STORE_KEY).rum).toBe('0') - expect(getSessionState(SESSION_STORE_KEY).expire).toMatch(/\d+/) - }) - - it('should not create a new cookie if no old cookie is present', () => { - tryOldCookiesMigration(sessionStoreStrategy) - expect(getCookie(SESSION_STORE_KEY)).toBeUndefined() - }) - - it('should behave correctly when performing the migration multiple times', () => { - setCookie(OLD_SESSION_COOKIE_NAME, 'abcde', SESSION_EXPIRATION_DELAY) - setCookie(OLD_LOGS_COOKIE_NAME, '1', SESSION_EXPIRATION_DELAY) - setCookie(OLD_RUM_COOKIE_NAME, '0', SESSION_EXPIRATION_DELAY) - - tryOldCookiesMigration(sessionStoreStrategy) - tryOldCookiesMigration(sessionStoreStrategy) - - expect(getSessionState(SESSION_STORE_KEY).id).toBe('abcde') - expect(getSessionState(SESSION_STORE_KEY).rum).toBe('0') - expect(getSessionState(SESSION_STORE_KEY).logs).toBe('1') - expect(getSessionState(SESSION_STORE_KEY).expire).toMatch(/\d+/) - }) -}) diff --git a/packages/core/src/domain/session/oldCookiesMigration.ts b/packages/core/src/domain/session/oldCookiesMigration.ts deleted file mode 100644 index da41b75f5a..0000000000 --- a/packages/core/src/domain/session/oldCookiesMigration.ts +++ /dev/null @@ -1,42 +0,0 @@ -import { getInitCookie } from '../../browser/cookie' -import type { SessionStoreStrategy } from './storeStrategies/sessionStoreStrategy' -import { SESSION_STORE_KEY } from './storeStrategies/sessionStoreStrategy' -import type { SessionState } from './sessionState' -import { expandSessionState, isSessionStarted } from './sessionState' - -export const OLD_SESSION_COOKIE_NAME = '_dd' -export const OLD_RUM_COOKIE_NAME = '_dd_r' -export const OLD_LOGS_COOKIE_NAME = '_dd_l' - -// duplicate values to avoid dependency issues -export const RUM_SESSION_KEY = 'rum' -export const LOGS_SESSION_KEY = 'logs' - -/** - * This migration should remain in the codebase as long as older versions are available/live - * to allow older sdk versions to be upgraded to newer versions without compatibility issues. - */ -export function tryOldCookiesMigration(cookieStoreStrategy: SessionStoreStrategy) { - const sessionString = getInitCookie(SESSION_STORE_KEY) - if (!sessionString) { - const oldSessionId = getInitCookie(OLD_SESSION_COOKIE_NAME) - const oldRumType = getInitCookie(OLD_RUM_COOKIE_NAME) - const oldLogsType = getInitCookie(OLD_LOGS_COOKIE_NAME) - const session: SessionState = {} - - if (oldSessionId) { - session.id = oldSessionId - } - if (oldLogsType && /^[01]$/.test(oldLogsType)) { - session[LOGS_SESSION_KEY] = oldLogsType - } - if (oldRumType && /^[012]$/.test(oldRumType)) { - session[RUM_SESSION_KEY] = oldRumType - } - - if (isSessionStarted(session)) { - expandSessionState(session) - cookieStoreStrategy.persistSession(session) - } - } -} diff --git a/packages/core/src/domain/session/storeStrategies/sessionInCookie.ts b/packages/core/src/domain/session/storeStrategies/sessionInCookie.ts index 2c4319e9a3..a6c2736e62 100644 --- a/packages/core/src/domain/session/storeStrategies/sessionInCookie.ts +++ b/packages/core/src/domain/session/storeStrategies/sessionInCookie.ts @@ -2,7 +2,6 @@ import { isChromium } from '../../../tools/utils/browserDetection' import type { CookieOptions } from '../../../browser/cookie' import { getCurrentSite, areCookiesAuthorized, getCookie, setCookie } from '../../../browser/cookie' import type { InitConfiguration, Configuration } from '../../configuration' -import { tryOldCookiesMigration } from '../oldCookiesMigration' import { SESSION_COOKIE_EXPIRATION_DELAY, SESSION_EXPIRATION_DELAY, @@ -38,8 +37,6 @@ export function initCookieStrategy(configuration: Configuration, cookieOptions: ), } - tryOldCookiesMigration(cookieStore) - return cookieStore } From cd5cc88d484619a3bf57a5eb0730260fc57106fb Mon Sep 17 00:00:00 2001 From: Fiona Date: Mon, 31 Aug 2026 03:30:06 -0700 Subject: [PATCH 2/3] fix(session): expire a session that cannot prove it is still within bounds A session was kept alive whenever its stamps were missing, because both comparisons short-circuited to `true` on `undefined`: (created === undefined || dateNow() - Number(created) < SESSION_TIME_OUT_DELAY) && (expire === undefined || dateNow() < Number(expire)) So a stored state holding an id but no `created` cleared the timeout, and one without `expire` cleared the inactivity deadline. `created` is only written in the step that generates a new id, so such a state could never acquire one and the session went on indefinitely -- surviving idle gaps of hours on a page that was never closed, well past both bounds. Both bounds now collapse into one effective deadline, min(expire, created + SESSION_TIME_OUT_DELAY), and a state that cannot produce one is expired rather than assumed young. This also caps a deadline written by a clock that was running ahead, which would otherwise hold a session open until that error had elapsed for real. Taken from upstream DataDog/browser-sdk 5257b52ea ("fix session lifetime bugs for long-lived pages and multi-tab scenarios", #4531). Their SessionManager rewrite makes the commit itself unmergeable here, so only the rule is carried over, with one deliberate difference: upstream stamps every started session, while here only tracked sessions get an id and a `created`, so a session with no id is still judged on `expire` alone. expandOrRenewSessionState now stamps the deadline before returning. processSessionStoreOperations decides whether the session is expired immediately after it runs and only stamped it afterwards, so a session renewed there would be judged with no deadline at all and no session could ever establish itself. Specs that asserted the old contract are updated: a stored session with no stamps used to be treated as live, and fixtures that stood in for an existing session now carry the stamps a real one has. --- .../src/domain/session/sessionManager.spec.ts | 30 +++++--- .../src/domain/session/sessionState.spec.ts | 72 +++++++++++++++++-- .../core/src/domain/session/sessionState.ts | 42 +++++++++-- .../core/src/domain/session/sessionStore.ts | 6 ++ .../session/sessionStoreOperations.spec.ts | 6 +- packages/logs/src/boot/startLogs.spec.ts | 2 +- .../src/domain/logsSessionManager.spec.ts | 14 ++-- .../src/domain/rumSessionManager.spec.ts | 18 ++--- 8 files changed, 151 insertions(+), 39 deletions(-) diff --git a/packages/core/src/domain/session/sessionManager.spec.ts b/packages/core/src/domain/session/sessionManager.spec.ts index 345502f1d6..3ff7d9767a 100644 --- a/packages/core/src/domain/session/sessionManager.spec.ts +++ b/packages/core/src/domain/session/sessionManager.spec.ts @@ -104,7 +104,11 @@ describe('startSessionManager', () => { describe('resume from a frozen tab ', () => { it('when session in store, do nothing', () => { - setCookie(SESSION_STORE_KEY, 'id=abcdef&first=tracked', DURATION) + setCookie( + SESSION_STORE_KEY, + `id=abcdef&first=tracked&created=${Date.now()}&expire=${Date.now() + DURATION}`, + DURATION + ) const sessionManager = startSessionManagerWithDefaults() window.dispatchEvent(createNewEvent(DOM_EVENT.RESUME)) @@ -143,7 +147,11 @@ describe('startSessionManager', () => { }) it('when tracked should keep existing tracking type and session id', () => { - setCookie(SESSION_STORE_KEY, 'id=abcdef&first=tracked', DURATION) + setCookie( + SESSION_STORE_KEY, + `id=abcdef&first=tracked&created=${Date.now()}&expire=${Date.now() + DURATION}`, + DURATION + ) const sessionManager = startSessionManagerWithDefaults() @@ -152,7 +160,7 @@ describe('startSessionManager', () => { }) it('when not tracked should keep existing tracking type', () => { - setCookie(SESSION_STORE_KEY, 'first=not-tracked', DURATION) + setCookie(SESSION_STORE_KEY, `first=not-tracked&expire=${Date.now() + DURATION}`, DURATION) const sessionManager = startSessionManagerWithDefaults({ computeSessionState: () => NOT_TRACKED_SESSION_STATE }) @@ -174,19 +182,19 @@ describe('startSessionManager', () => { }) it('should be called with an invalid value if the cookie has an invalid value', () => { - setCookie(SESSION_STORE_KEY, 'first=invalid', DURATION) + setCookie(SESSION_STORE_KEY, `first=invalid&expire=${Date.now() + DURATION}`, DURATION) startSessionManagerWithDefaults({ computeSessionState: spy }) expect(spy).toHaveBeenCalledWith('invalid') }) it('should be called with TRACKED', () => { - setCookie(SESSION_STORE_KEY, 'first=tracked', DURATION) + setCookie(SESSION_STORE_KEY, `first=tracked&expire=${Date.now() + DURATION}`, DURATION) startSessionManagerWithDefaults({ computeSessionState: spy }) expect(spy).toHaveBeenCalledWith(FakeTrackingType.TRACKED) }) it('should be called with NOT_TRACKED', () => { - setCookie(SESSION_STORE_KEY, 'first=not-tracked', DURATION) + setCookie(SESSION_STORE_KEY, `first=not-tracked&expire=${Date.now() + DURATION}`, DURATION) startSessionManagerWithDefaults({ computeSessionState: spy }) expect(spy).toHaveBeenCalledWith(FakeTrackingType.NOT_TRACKED) }) @@ -347,13 +355,17 @@ describe('startSessionManager', () => { expect(expireSessionSpy).not.toHaveBeenCalled() // the session has not been active from the start }) - it('should not add created date to an existing session from an older versions', () => { + it('should not adopt a stored session that carries no creation date', () => { + // Written by a version that did not stamp `created`. Such a session cannot be shown to sit + // inside SESSION_TIME_OUT_DELAY, so it is renewed rather than trusted -- adopting it is how + // a session stayed alive for days on a page that was never closed. Every release of this + // SDK stamps `created`, so nothing we ship produces this state. setCookie(SESSION_STORE_KEY, 'id=abcde&first=tracked', DURATION) const sessionManager = startSessionManagerWithDefaults() - expect(sessionManager.findSession()!.id).toBe('abcde') - expect(getSessionState(SESSION_STORE_KEY).created).toBeUndefined() + expect(sessionManager.findSession()!.id).not.toBe('abcde') + expect(getSessionState(SESSION_STORE_KEY).created).toBeDefined() }) }) diff --git a/packages/core/src/domain/session/sessionState.spec.ts b/packages/core/src/domain/session/sessionState.spec.ts index 1fe4fa4c12..c9c38936d9 100644 --- a/packages/core/src/domain/session/sessionState.spec.ts +++ b/packages/core/src/domain/session/sessionState.spec.ts @@ -1,8 +1,9 @@ import { dateNow } from '../../tools/utils/timeUtils' -import { SESSION_EXPIRATION_DELAY } from './sessionConstants' +import { SESSION_EXPIRATION_DELAY, SESSION_TIME_OUT_DELAY } from './sessionConstants' import type { SessionState } from './sessionState' import { expandSessionState, + getExpireDate, isSessionInExpiredState, toSessionString, toSessionState, @@ -29,22 +30,83 @@ describe('session state utilities', () => { }) describe('isSessionInExpiredState', () => { + const ONE_DAY = 24 * 60 * 60 * 1000 + function dateNowWithOffset(offset = 0) { return String(dateNow() + offset) } it('should correctly identify a session in expired state', () => { expect(isSessionInExpiredState(EXPIRED_SESSION)).toBe(true) - expect(isSessionInExpiredState({ created: dateNowWithOffset(-1000 * 60 * 60 * 4) })).toBe(true) - expect(isSessionInExpiredState({ expire: dateNowWithOffset(-100) })).toBe(true) + expect( + isSessionInExpiredState({ + created: dateNowWithOffset(-SESSION_TIME_OUT_DELAY), + expire: dateNowWithOffset(1000), + }) + ).toBe(true) + expect(isSessionInExpiredState({ created: dateNowWithOffset(-100), expire: dateNowWithOffset(-100) })).toBe(true) + }) + + it('should expire a session that cannot say when it started or when it lapses', () => { + // A missing stamp used to short-circuit the comparison to `true`, which is how a session + // outlived both bounds and kept running for days on a page that was never closed. + expect(isSessionInExpiredState({ first: 'not-tracked' })).toBe(true) + expect(isSessionInExpiredState({ first: 'tracked' })).toBe(true) + expect(isSessionInExpiredState({ id: '123', first: 'tracked', expire: dateNowWithOffset(1000) })).toBe(true) + expect(isSessionInExpiredState({ id: '123', first: 'tracked', created: dateNowWithOffset(-1000) })).toBe(true) + }) + + it('should cap the sliding deadline at SESSION_TIME_OUT_DELAY from creation', () => { + // An `expire` beyond the cap can only come from a clock that was ahead when it was written, + // and would otherwise hold the session open until that error had elapsed for real. + expect( + isSessionInExpiredState({ + created: dateNowWithOffset(-SESSION_TIME_OUT_DELAY), + expire: dateNowWithOffset(ONE_DAY), + }) + ).toBe(true) }) it('should correctly identify a session in live state', () => { expect(isSessionInExpiredState({ created: dateNowWithOffset(-1000), expire: dateNowWithOffset(1000) })).toBe( false ) - expect(isSessionInExpiredState({ first: 'not-tracked' })).toBe(false) - expect(isSessionInExpiredState({ first: 'tracked' })).toBe(false) + }) + + it('should not consider a session that was never started as expired', () => { + expect(isSessionInExpiredState(NOT_STARTED_SESSION)).toBe(false) + }) + }) + + describe('getExpireDate', () => { + function dateNowWithOffset(offset = 0) { + return String(dateNow() + offset) + } + + it('should return undefined without an expire stamp', () => { + expect(getExpireDate({})).toBeUndefined() + expect(getExpireDate({ created: dateNowWithOffset(-1000) })).toBeUndefined() + }) + + it('should return undefined when a session holding an id has no creation date', () => { + expect(getExpireDate({ id: '123', expire: dateNowWithOffset(1000) })).toBeUndefined() + }) + + it('should fall back to expire alone for a not-tracked session, which is never stamped', () => { + const expire = dateNowWithOffset(1000) + expect(getExpireDate({ first: 'not-tracked', expire })).toBe(Number(expire)) + }) + + it('should return the sliding deadline while it is the earlier of the two', () => { + const expire = dateNowWithOffset(1000) + expect(getExpireDate({ created: dateNowWithOffset(-1000), expire })).toBe(Number(expire)) + }) + + it('should return the creation cap once it is the earlier of the two', () => { + const created = dateNowWithOffset(-SESSION_TIME_OUT_DELAY + 1000) + expect(getExpireDate({ created, expire: dateNowWithOffset(SESSION_TIME_OUT_DELAY) })).toBe( + Number(created) + SESSION_TIME_OUT_DELAY + ) }) }) diff --git a/packages/core/src/domain/session/sessionState.ts b/packages/core/src/domain/session/sessionState.ts index 9734e138a3..4c48a5620d 100644 --- a/packages/core/src/domain/session/sessionState.ts +++ b/packages/core/src/domain/session/sessionState.ts @@ -41,18 +41,48 @@ export function isSessionStarted(session: SessionState) { return !isSessionInNotStartedState(session) } +/** + * The moment the session actually stops being usable: whichever comes first, the sliding + * inactivity deadline or the hard cap counted from when the session was created. + * + * Returns undefined when either stamp is missing, which callers treat as "expired". A session + * that cannot say when it started or when it lapses is not given the benefit of the doubt -- + * letting `undefined` short-circuit those comparisons is what allowed sessions to outlive both + * bounds and stay alive for days on a page that was never closed. + * + * Ported from upstream DataDog/browser-sdk 5257b52ea ("fix session lifetime bugs for long-lived + * pages and multi-tab scenarios", #4531); their SessionManager rewrite makes the commit itself + * unmergeable here, so only the rule is carried over. + */ +export function getExpireDate(state: SessionState): number | undefined { + const expireDate = state.expire && Number(state.expire) + if (!expireDate) { + return + } + const createdDate = state.created && Number(state.created) + if (createdDate) { + return Math.min(expireDate, createdDate + SESSION_TIME_OUT_DELAY) + } + // A session is stamped with `created` in the same step that generates its id, so one holding an + // id but no creation date cannot be shown to sit inside the cap and gets no expiry date at all. + // A session without an id is not tracked and is never stamped here, so `expire` alone bounds it + // -- this is where we part from upstream, which stamps every started session and can therefore + // require both unconditionally. + return state.id === undefined ? expireDate : undefined +} + export function isSessionInExpiredState(session: SessionState) { + if (isSessionInNotStartedState(session)) { + // nothing has been stored yet, so there is no session to consider expired + return false + } return session.isExpired !== undefined || !isActiveSession(session) } // An active session is a session in either `Tracked` or `NotTracked` state function isActiveSession(sessionState: SessionState) { - // created and expire can be undefined for versions which was not storing them - // these checks could be removed when older versions will not be available/live anymore - return ( - (sessionState.created === undefined || dateNow() - Number(sessionState.created) < SESSION_TIME_OUT_DELAY) && - (sessionState.expire === undefined || dateNow() < Number(sessionState.expire)) - ) + const expireDate = getExpireDate(sessionState) + return expireDate ? dateNow() < expireDate : false } export function expandSessionState(session: SessionState) { diff --git a/packages/core/src/domain/session/sessionStore.ts b/packages/core/src/domain/session/sessionStore.ts index 4c0a1a74e5..198230679f 100644 --- a/packages/core/src/domain/session/sessionStore.ts +++ b/packages/core/src/domain/session/sessionStore.ts @@ -9,6 +9,7 @@ import { selectCookieStrategy, initCookieStrategy } from './storeStrategies/sess import type { SessionStoreStrategyType } from './storeStrategies/sessionStoreStrategy' import { getExpiredSessionState, + expandSessionState, isSessionInExpiredState, isSessionInNotStartedState, isSessionStarted, @@ -183,6 +184,11 @@ export function startSessionStore( sessionState.id = generateUUID() sessionState.created = String(dateNow()) } + // Stamp the deadline before returning. The caller decides whether the session is expired + // straight after this runs and only stamps it afterwards, so a session renewed here would be + // judged with no deadline at all -- which now reads as expired and would stop any session + // from ever establishing. Upstream does the same inside its own expandOrRenew. + expandSessionState(sessionState) } function hasSessionInCache() { diff --git a/packages/core/src/domain/session/sessionStoreOperations.spec.ts b/packages/core/src/domain/session/sessionStoreOperations.spec.ts index 9235fc0bb9..78d76b6efe 100644 --- a/packages/core/src/domain/session/sessionStoreOperations.spec.ts +++ b/packages/core/src/domain/session/sessionStoreOperations.spec.ts @@ -5,6 +5,7 @@ import type { Configuration } from '../configuration' import { initCookieStrategy } from './storeStrategies/sessionInCookie' import { initLocalStorageStrategy } from './storeStrategies/sessionInLocalStorage' import type { SessionState } from './sessionState' +import { SESSION_EXPIRATION_DELAY } from './sessionConstants' import { expandSessionState, toSessionString } from './sessionState' import { processSessionStoreOperations, LOCK_MAX_TRIES, LOCK_RETRY_DELAY } from './sessionStoreOperations' import { SESSION_STORE_KEY } from './storeStrategies/sessionStoreStrategy' @@ -39,8 +40,9 @@ const DEFAULT_INIT_CONFIGURATION = { trackAnonymousUser: true } as Configuration beforeEach(() => { sessionStoreStrategy.expireSession(initialSession) - initialSession = { id: '123', created: String(now) } - otherSession = { id: '456', created: String(now + 100) } + // Both stamps are required for a session to read as live, so the fixtures carry them. + initialSession = { id: '123', created: String(now), expire: String(now + SESSION_EXPIRATION_DELAY) } + otherSession = { id: '456', created: String(now + 100), expire: String(now + SESSION_EXPIRATION_DELAY) } processSpy = jasmine.createSpy('process') afterSpy = jasmine.createSpy('after') storage = mockStorage() diff --git a/packages/logs/src/boot/startLogs.spec.ts b/packages/logs/src/boot/startLogs.spec.ts index 2b112aee69..e48fb5cc4d 100644 --- a/packages/logs/src/boot/startLogs.spec.ts +++ b/packages/logs/src/boot/startLogs.spec.ts @@ -264,7 +264,7 @@ describe('logs', () => { }) it('sends logs without session id when the session expires ', async () => { - setCookie(SESSION_STORE_KEY, 'id=foo&logs=1', ONE_MINUTE) + setCookie(SESSION_STORE_KEY, `id=foo&logs=1&created=${Date.now()}&expire=${Date.now() + ONE_MINUTE}`, ONE_MINUTE) ;({ handleLog, stop: stopLogs } = startLogs( initConfiguration, baseConfiguration, diff --git a/packages/logs/src/domain/logsSessionManager.spec.ts b/packages/logs/src/domain/logsSessionManager.spec.ts index e19ce9176b..54ff4a51d7 100644 --- a/packages/logs/src/domain/logsSessionManager.spec.ts +++ b/packages/logs/src/domain/logsSessionManager.spec.ts @@ -53,7 +53,7 @@ describe('logs session manager', () => { }) it('when tracked should keep existing tracking type and session id', () => { - setCookie(SESSION_STORE_KEY, 'id=abcdef&logs=1', DURATION) + setCookie(SESSION_STORE_KEY, `id=abcdef&logs=1&created=${Date.now()}&expire=${Date.now() + DURATION}`, DURATION) startLogsSessionManagerWithDefaults() @@ -62,7 +62,7 @@ describe('logs session manager', () => { }) it('when not tracked should keep existing tracking type', () => { - setCookie(SESSION_STORE_KEY, 'logs=0', DURATION) + setCookie(SESSION_STORE_KEY, `logs=0&expire=${Date.now() + DURATION}`, DURATION) startLogsSessionManagerWithDefaults() @@ -84,19 +84,19 @@ describe('logs session manager', () => { describe('findTrackedSession', () => { it('should return the current active session', () => { - setCookie(SESSION_STORE_KEY, 'id=abcdef&logs=1', DURATION) + setCookie(SESSION_STORE_KEY, `id=abcdef&logs=1&created=${Date.now()}&expire=${Date.now() + DURATION}`, DURATION) const logsSessionManager = startLogsSessionManagerWithDefaults() expect(logsSessionManager.findTrackedSession()!.id).toBe('abcdef') }) it('should return undefined if the session is not tracked', () => { - setCookie(SESSION_STORE_KEY, 'id=abcdef&logs=0', DURATION) + setCookie(SESSION_STORE_KEY, `id=abcdef&logs=0&created=${Date.now()}&expire=${Date.now() + DURATION}`, DURATION) const logsSessionManager = startLogsSessionManagerWithDefaults() expect(logsSessionManager.findTrackedSession()).toBeUndefined() }) it('should not return the current session if it has expired by default', () => { - setCookie(SESSION_STORE_KEY, 'id=abcdef&logs=1', DURATION) + setCookie(SESSION_STORE_KEY, `id=abcdef&logs=1&created=${Date.now()}&expire=${Date.now() + DURATION}`, DURATION) const logsSessionManager = startLogsSessionManagerWithDefaults() clock.tick(10 * ONE_SECOND) expireCookie() @@ -112,10 +112,10 @@ describe('logs session manager', () => { }) it('should return session corresponding to start time', () => { - setCookie(SESSION_STORE_KEY, 'id=foo&logs=1', DURATION) + setCookie(SESSION_STORE_KEY, `id=foo&logs=1&created=${Date.now()}&expire=${Date.now() + DURATION}`, DURATION) const logsSessionManager = startLogsSessionManagerWithDefaults() clock.tick(10 * ONE_SECOND) - setCookie(SESSION_STORE_KEY, 'id=bar&logs=1', DURATION) + setCookie(SESSION_STORE_KEY, `id=bar&logs=1&created=${Date.now()}&expire=${Date.now() + DURATION}`, DURATION) // simulate a click to renew the session document.dispatchEvent(createNewEvent(DOM_EVENT.CLICK)) clock.tick(STORAGE_POLL_DELAY) diff --git a/packages/rum-core/src/domain/rumSessionManager.spec.ts b/packages/rum-core/src/domain/rumSessionManager.spec.ts index 95fa26abd6..dc82b52f8e 100644 --- a/packages/rum-core/src/domain/rumSessionManager.spec.ts +++ b/packages/rum-core/src/domain/rumSessionManager.spec.ts @@ -87,7 +87,7 @@ describe('rum session manager', () => { }) it('when tracked should keep existing session type and id', () => { - setCookie(SESSION_STORE_KEY, 'id=abcdef&rum=1', DURATION) + setCookie(SESSION_STORE_KEY, `id=abcdef&rum=1&created=${Date.now()}&expire=${Date.now() + DURATION}`, DURATION) startRumSessionManagerWithDefaults() @@ -98,7 +98,7 @@ describe('rum session manager', () => { }) it('when not tracked should keep existing session type', () => { - setCookie(SESSION_STORE_KEY, 'rum=0', DURATION) + setCookie(SESSION_STORE_KEY, `rum=0&expire=${Date.now() + DURATION}`, DURATION) startRumSessionManagerWithDefaults() @@ -108,7 +108,7 @@ describe('rum session manager', () => { }) it('should renew on activity after expiration', () => { - setCookie(SESSION_STORE_KEY, 'id=abcdef&rum=1', DURATION) + setCookie(SESSION_STORE_KEY, `id=abcdef&rum=1&created=${Date.now()}&expire=${Date.now() + DURATION}`, DURATION) startRumSessionManagerWithDefaults({ configuration: { sessionSampleRate: 100, sessionReplaySampleRate: 100 } }) @@ -129,13 +129,13 @@ describe('rum session manager', () => { describe('findSession', () => { it('should return the current session', () => { - setCookie(SESSION_STORE_KEY, 'id=abcdef&rum=1', DURATION) + setCookie(SESSION_STORE_KEY, `id=abcdef&rum=1&created=${Date.now()}&expire=${Date.now() + DURATION}`, DURATION) const rumSessionManager = startRumSessionManagerWithDefaults() expect(rumSessionManager.findTrackedSession()!.id).toBe('abcdef') }) it('should return undefined if the session is not tracked', () => { - setCookie(SESSION_STORE_KEY, 'id=abcdef&rum=0', DURATION) + setCookie(SESSION_STORE_KEY, `id=abcdef&rum=0&created=${Date.now()}&expire=${Date.now() + DURATION}`, DURATION) const rumSessionManager = startRumSessionManagerWithDefaults() expect(rumSessionManager.findTrackedSession()).toBe(undefined) }) @@ -148,7 +148,7 @@ describe('rum session manager', () => { }) it('should return session corresponding to start time', () => { - setCookie(SESSION_STORE_KEY, 'id=abcdef&rum=1', DURATION) + setCookie(SESSION_STORE_KEY, `id=abcdef&rum=1&created=${Date.now()}&expire=${Date.now() + DURATION}`, DURATION) const rumSessionManager = startRumSessionManagerWithDefaults() clock.tick(10 * ONE_SECOND) expireCookie() @@ -158,19 +158,19 @@ describe('rum session manager', () => { }) it('should return session TRACKED_WITH_SESSION_REPLAY', () => { - setCookie(SESSION_STORE_KEY, 'id=abcdef&rum=1', DURATION) + setCookie(SESSION_STORE_KEY, `id=abcdef&rum=1&created=${Date.now()}&expire=${Date.now() + DURATION}`, DURATION) const rumSessionManager = startRumSessionManagerWithDefaults() expect(rumSessionManager.findTrackedSession()!.sessionReplay).toBe(SessionReplayState.SAMPLED) }) it('should return session TRACKED_WITHOUT_SESSION_REPLAY', () => { - setCookie(SESSION_STORE_KEY, 'id=abcdef&rum=2', DURATION) + setCookie(SESSION_STORE_KEY, `id=abcdef&rum=2&created=${Date.now()}&expire=${Date.now() + DURATION}`, DURATION) const rumSessionManager = startRumSessionManagerWithDefaults() expect(rumSessionManager.findTrackedSession()!.sessionReplay).toBe(SessionReplayState.OFF) }) it('should update current entity when replay recording is forced', () => { - setCookie(SESSION_STORE_KEY, 'id=abcdef&rum=2', DURATION) + setCookie(SESSION_STORE_KEY, `id=abcdef&rum=2&created=${Date.now()}&expire=${Date.now() + DURATION}`, DURATION) const rumSessionManager = startRumSessionManagerWithDefaults() rumSessionManager.setForcedReplay() From 57613705b3f0054475ced2b59fb7f7c359d8ece0 Mon Sep 17 00:00:00 2001 From: Fiona Date: Tue, 1 Sep 2026 01:15:30 -0700 Subject: [PATCH 3/3] fix(session): cap not-tracked sessions too, and align the legacy bundle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up. The first pass closed the escape for tracked sessions and left it open for the other half. `created` is now stamped on every started session, not only on the one that mints an id. A not-tracked session carried no creation date, so the cap had nothing to count from while the visibility timer pushed its deadline forward once a minute: on a page that stays open it never expired. The practical effect is that a user sampled out of a long-lived page never got to re-run the sampling draw, because every renewal read the stored `rum=0` back. The id-less fallback in getExpireDate stays. Its job is not this case but mixed builds: a bundle that predates the stamp keeps writing state without one, and expiring those on sight would have old and new builds take turns ending each other's sessions. The legacy bundle carried its own copy of the same fail-open check, and its own header promises "same cookie name, same serialisation, same expiration rules". After the first pass that promise was false: the modern bundle stopped trusting a state with no creation date while the legacy one would adopt it and refresh its deadline forever, so any page loading the legacy build could resurrect exactly the sessions this change ends. Its rule now mirrors the modern one, and it stamps what it adopts. Tests: a not-tracked session must be stamped; the legacy build must not adopt a session with no creation date, and must stamp a sampled-out one it adopts. Each was checked against the unfixed source first — all three fail without the change. One existing test is repaired rather than moved: "should renew an existing timed out session" set a fixture with no `expire`, so after the first pass it was passing for the mundane reason of having no deadline at all, and the cap it names was never exercised. --- .../src/domain/session/sessionManager.spec.ts | 18 ++++++++++++- .../src/domain/session/sessionState.spec.ts | 3 +-- .../core/src/domain/session/sessionState.ts | 10 +++---- .../core/src/domain/session/sessionStore.ts | 7 +++++ .../src/domain/sessionStore.spec.ts | 25 +++++++++++++++++ .../rum-legacy/src/domain/sessionStore.ts | 27 ++++++++++++++++--- 6 files changed, 79 insertions(+), 11 deletions(-) diff --git a/packages/core/src/domain/session/sessionManager.spec.ts b/packages/core/src/domain/session/sessionManager.spec.ts index 3ff7d9767a..d709e3b54f 100644 --- a/packages/core/src/domain/session/sessionManager.spec.ts +++ b/packages/core/src/domain/session/sessionManager.spec.ts @@ -146,6 +146,16 @@ describe('startSessionManager', () => { expectTrackingTypeToBe(sessionManager, FIRST_PRODUCT_KEY, FakeTrackingType.NOT_TRACKED) }) + it('should stamp a creation date on a not-tracked session too', () => { + // The creation date is what caps a session at SESSION_TIME_OUT_DELAY. Without one, the + // visibility timer keeps pushing `expire` forward and a sampled-out session on a page that + // stays open never expires -- so those users never get to re-roll the sampling decision. + startSessionManagerWithDefaults({ computeSessionState: () => NOT_TRACKED_SESSION_STATE }) + + expect(getSessionState(SESSION_STORE_KEY).id).toBeUndefined() + expect(getSessionState(SESSION_STORE_KEY).created).toMatch(/^\d+$/) + }) + it('when tracked should keep existing tracking type and session id', () => { setCookie( SESSION_STORE_KEY, @@ -344,7 +354,13 @@ describe('startSessionManager', () => { }) it('should renew an existing timed out session', () => { - setCookie(SESSION_STORE_KEY, `id=abcde&first=tracked&created=${Date.now() - SESSION_TIME_OUT_DELAY}`, DURATION) + // `expire` is still ahead, so the creation cap is what has to end this session -- without it + // the session would expire for the mundane reason of having no deadline at all. + setCookie( + SESSION_STORE_KEY, + `id=abcde&first=tracked&created=${Date.now() - SESSION_TIME_OUT_DELAY}&expire=${Date.now() + SESSION_EXPIRATION_DELAY}`, + DURATION + ) const sessionManager = startSessionManagerWithDefaults() const expireSessionSpy = jasmine.createSpy() diff --git a/packages/core/src/domain/session/sessionState.spec.ts b/packages/core/src/domain/session/sessionState.spec.ts index c9c38936d9..525d90936a 100644 --- a/packages/core/src/domain/session/sessionState.spec.ts +++ b/packages/core/src/domain/session/sessionState.spec.ts @@ -48,8 +48,7 @@ describe('session state utilities', () => { }) it('should expire a session that cannot say when it started or when it lapses', () => { - // A missing stamp used to short-circuit the comparison to `true`, which is how a session - // outlived both bounds and kept running for days on a page that was never closed. + // A missing stamp used to short-circuit the comparison to `true`. See getExpireDate. expect(isSessionInExpiredState({ first: 'not-tracked' })).toBe(true) expect(isSessionInExpiredState({ first: 'tracked' })).toBe(true) expect(isSessionInExpiredState({ id: '123', first: 'tracked', expire: dateNowWithOffset(1000) })).toBe(true) diff --git a/packages/core/src/domain/session/sessionState.ts b/packages/core/src/domain/session/sessionState.ts index 4c48a5620d..4166f31b4b 100644 --- a/packages/core/src/domain/session/sessionState.ts +++ b/packages/core/src/domain/session/sessionState.ts @@ -63,11 +63,11 @@ export function getExpireDate(state: SessionState): number | undefined { if (createdDate) { return Math.min(expireDate, createdDate + SESSION_TIME_OUT_DELAY) } - // A session is stamped with `created` in the same step that generates its id, so one holding an - // id but no creation date cannot be shown to sit inside the cap and gets no expiry date at all. - // A session without an id is not tracked and is never stamped here, so `expire` alone bounds it - // -- this is where we part from upstream, which stamps every started session and can therefore - // require both unconditionally. + // Every session this bundle starts is stamped, so a missing creation date means the state was + // written elsewhere: either by a build that predates the stamp, or by another bundle sharing the + // cookie. A state holding an id is judged strictly -- it cannot be shown to sit inside the cap. + // One without an id is not tracked, carries no identity, and falls back to `expire` alone rather + // than being expired on sight, which would make old and new builds fight over the same cookie. return state.id === undefined ? expireDate : undefined } diff --git a/packages/core/src/domain/session/sessionStore.ts b/packages/core/src/domain/session/sessionStore.ts index 198230679f..c926e79410 100644 --- a/packages/core/src/domain/session/sessionStore.ts +++ b/packages/core/src/domain/session/sessionStore.ts @@ -182,6 +182,13 @@ export function startSessionStore( delete sessionState.isExpired if (isTracked && !sessionState.id) { sessionState.id = generateUUID() + } + // Stamp every started session, tracked or not. The creation date is what caps a session at + // SESSION_TIME_OUT_DELAY, and a not-tracked session that carried none would be held open + // indefinitely by the visibility timer refreshing `expire` -- the same escape this change + // closes for tracked sessions, left open for the sampled-out half. Without the cap those + // users never get to re-roll the sampling decision. + if (!sessionState.created) { sessionState.created = String(dateNow()) } // Stamp the deadline before returning. The caller decides whether the session is expired diff --git a/packages/rum-legacy/src/domain/sessionStore.spec.ts b/packages/rum-legacy/src/domain/sessionStore.spec.ts index 3f770365d3..fdeddb47a4 100644 --- a/packages/rum-legacy/src/domain/sessionStore.spec.ts +++ b/packages/rum-legacy/src/domain/sessionStore.spec.ts @@ -210,6 +210,31 @@ describe('session store', () => { expect(second.id).not.toBe(first.id) }) + it('does not adopt a session that carries no creation date', () => { + // Such a cookie has no cap to count from, so every access would push its deadline forward and + // the session would never end. The modern bundle stops trusting these too -- both builds share + // this cookie, so a difference here would let this one resurrect a session the other ended. + document.cookie = `${SESSION_COOKIE_NAME}=id=abcdef&rum=2&expire=${Date.now() + ONE_MINUTE};path=/` + + const session = createSessionStore(100).getOrCreateSession() + + expect(session.id).not.toBe('abcdef') + expect(readRawCookie()).toMatch(/created=\d+/) + }) + + it('stamps a creation date on a sampled-out session it adopts without one', () => { + // An id-less state is a legitimately sampled-out session, so it is adopted rather than redrawn. + // Adopting it without stamping would leave the cap nothing to count from, and every access + // pushes the deadline forward — the sampled-out user on a page that stays open would then + // never get to re-run the draw. + document.cookie = `${SESSION_COOKIE_NAME}=rum=0&expire=${Date.now() + ONE_MINUTE};path=/` + + createSessionStore(100).getOrCreateSession() + + expect(readRawCookie()).toMatch(/rum=0/) + expect(readRawCookie()).toMatch(/created=\d+/) + }) + describe('sampling', () => { it('tracks the session when the sample rate is 100', () => { expect(createSessionStore(100).getOrCreateSession().isTracked).toBe(true) diff --git a/packages/rum-legacy/src/domain/sessionStore.ts b/packages/rum-legacy/src/domain/sessionStore.ts index 4bc4c53b33..60a870a3a7 100644 --- a/packages/rum-legacy/src/domain/sessionStore.ts +++ b/packages/rum-legacy/src/domain/sessionStore.ts @@ -115,6 +115,13 @@ export function createSessionStore(sessionSampleRate: number) { state.id = generateUUID() } + // Stamp every session, tracked or not, so the cap above always has something to count from. + // A session adopted from a build that did not stamp would otherwise keep having its deadline + // pushed forward on every access and never reach the cap at all. + if (!state.created) { + state.created = String(now) + } + state.expire = String(now + SESSION_EXPIRATION_DELAY) inMemoryState = state lastCookieAccess = now @@ -158,10 +165,24 @@ function extractForeignFields(state: SessionState | undefined): SessionState { return kept } +/** + * Mirrors the modern bundle's rule (`core/domain/session/sessionState.ts`): a session lives until + * whichever comes first — its sliding deadline, or the cap counted from when it was created — and a + * state that cannot produce either is expired rather than assumed young. Both builds share `_dd_s`, + * so any difference here would let one build resurrect a session the other had already ended. + */ function isExpired(state: SessionState, now: number): boolean { - const createdAt = Number(state.created) - const expiresAt = Number(state.expire) - return (createdAt && now - createdAt >= SESSION_TIME_OUT_DELAY) || (expiresAt && now >= expiresAt) ? true : false + const expiresAt = state.expire && Number(state.expire) + if (!expiresAt || !isFinite(expiresAt)) { + return true + } + const createdAt = state.created && Number(state.created) + if (createdAt && isFinite(createdAt)) { + return now >= Math.min(expiresAt, createdAt + SESSION_TIME_OUT_DELAY) + } + // No creation date: written by a build that predates the stamp. A state holding an id cannot be + // shown to sit inside the cap; one without an id is not tracked and falls back to the deadline. + return state.id !== undefined || now >= expiresAt } /**