fix(session): expire a session that cannot prove it is still within bounds

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))
@@ -142,8 +146,22 @@ 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, '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 +170,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 +192,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)
     })
@@ -336,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()
@@ -347,13 +371,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()
     })
   })
 

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,82 @@ 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`. 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)
+      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
+      )
     })
   })
 

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)
+  }
+  // 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
+}
+
 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) {