From c89b5788b260c31045394f77991e77eaa82eb12e Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Mon, 4 Nov 2024 14:38:44 +0100 Subject: [PATCH 1/4] fix: rotate session id proactively --- src/sessionid.ts | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/src/sessionid.ts b/src/sessionid.ts index b2e81a413..8b03c5b3c 100644 --- a/src/sessionid.ts +++ b/src/sessionid.ts @@ -29,6 +29,9 @@ export class SessionIdManager { private _sessionIdChangedHandlers: SessionIdChangedCallback[] = [] private readonly _sessionTimeoutMs: number + // we track activity so we can end the session proactively when it has passed the idle timeout + private _enforceIdleTimeout: ReturnType | undefined + constructor( config: Partial, persistence: PostHogPersistence, @@ -162,14 +165,14 @@ export class SessionIdManager { if (this._sessionId && this._sessionActivityTimestamp && this._sessionStartTimestamp) { return [this._sessionActivityTimestamp, this._sessionId, this._sessionStartTimestamp] } - const sessionId = this.persistence.props[SESSION_ID] + const sessionIdInfo = this.persistence.props[SESSION_ID] - if (isArray(sessionId) && sessionId.length === 2) { + if (isArray(sessionIdInfo) && sessionIdInfo.length === 2) { // Storage does not yet have a session start time. Add the last activity timestamp as the start time - sessionId.push(sessionId[0]) + sessionIdInfo.push(sessionIdInfo[0]) } - return sessionId || [0, null, 0] + return sessionIdInfo || [0, null, 0] } // Resets the session id by setting it to null. On the subsequent call to checkAndGetSessionAndWindowId, @@ -212,7 +215,7 @@ export class SessionIdManager { const timestamp = _timestamp || new Date().getTime() // eslint-disable-next-line prefer-const - let [lastTimestamp, sessionId, startTimestamp] = this._getSessionId() + let [lastActivityTimestamp, sessionId, startTimestamp] = this._getSessionId() let windowId = this._getWindowId() const sessionPastMaximumLength = @@ -222,7 +225,7 @@ export class SessionIdManager { let valuesChanged = false const noSessionId = !sessionId - const activityTimeout = !readOnly && Math.abs(timestamp - lastTimestamp) > this.sessionTimeoutMs + const activityTimeout = !readOnly && Math.abs(timestamp - lastActivityTimestamp) > this.sessionTimeoutMs if (noSessionId || activityTimeout || sessionPastMaximumLength) { sessionId = this._sessionIdGenerator() windowId = this._windowIdGenerator() @@ -238,11 +241,20 @@ export class SessionIdManager { valuesChanged = true } - const newTimestamp = lastTimestamp === 0 || !readOnly || sessionPastMaximumLength ? timestamp : lastTimestamp + const newActivityTimestamp = + lastActivityTimestamp === 0 || !readOnly || sessionPastMaximumLength ? timestamp : lastActivityTimestamp const sessionStartTimestamp = startTimestamp === 0 ? new Date().getTime() : startTimestamp this._setWindowId(windowId) - this._setSessionId(sessionId, newTimestamp, sessionStartTimestamp) + this._setSessionId(sessionId, newActivityTimestamp, sessionStartTimestamp) + + if (!readOnly) { + clearTimeout(this._enforceIdleTimeout) + this._enforceIdleTimeout = setTimeout(() => { + // enforce idle timeout a little after the session timeout to ensure the session is reset even without activity + this.resetSessionId() + }, this.sessionTimeoutMs * 1.1) + } if (valuesChanged) { this._sessionIdChangedHandlers.forEach((handler) => @@ -259,7 +271,7 @@ export class SessionIdManager { windowId, sessionStartTimestamp, changeReason: valuesChanged ? { noSessionId, activityTimeout, sessionPastMaximumLength } : undefined, - lastActivityTimestamp: lastTimestamp, + lastActivityTimestamp: lastActivityTimestamp, } } } From bf840aac9d4fb73a86700dcad4cec2ec9e316a8a Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Tue, 5 Nov 2024 10:30:23 +0100 Subject: [PATCH 2/4] rename some test files --- src/__tests__/{decide.ts => decide.test.ts} | 0 src/__tests__/{featureflags.ts => featureflags.test.ts} | 0 .../{posthog-core.loaded.ts => posthog-core.loaded.test.ts} | 2 +- src/sessionid.ts | 2 +- 4 files changed, 2 insertions(+), 2 deletions(-) rename src/__tests__/{decide.ts => decide.test.ts} (100%) rename src/__tests__/{featureflags.ts => featureflags.test.ts} (100%) rename src/__tests__/{posthog-core.loaded.ts => posthog-core.loaded.test.ts} (98%) diff --git a/src/__tests__/decide.ts b/src/__tests__/decide.test.ts similarity index 100% rename from src/__tests__/decide.ts rename to src/__tests__/decide.test.ts diff --git a/src/__tests__/featureflags.ts b/src/__tests__/featureflags.test.ts similarity index 100% rename from src/__tests__/featureflags.ts rename to src/__tests__/featureflags.test.ts diff --git a/src/__tests__/posthog-core.loaded.ts b/src/__tests__/posthog-core.loaded.test.ts similarity index 98% rename from src/__tests__/posthog-core.loaded.ts rename to src/__tests__/posthog-core.loaded.test.ts index 1dc667f4a..1e5382fa5 100644 --- a/src/__tests__/posthog-core.loaded.ts +++ b/src/__tests__/posthog-core.loaded.test.ts @@ -65,7 +65,7 @@ describe('loaded() with flags', () => { // we should call _reloadFeatureFlagsRequest for `group` only after the initial load // because it ought to be paused until decide returns - expect(instance._send_request).toHaveBeenCalledTimes(1) + expect(instance._send_request).toHaveBeenCalledWith('wat') expect(instance.featureFlags._reloadFeatureFlagsRequest).toHaveBeenCalledTimes(0) jest.runOnlyPendingTimers() diff --git a/src/sessionid.ts b/src/sessionid.ts index 8b03c5b3c..9c65d83eb 100644 --- a/src/sessionid.ts +++ b/src/sessionid.ts @@ -252,7 +252,7 @@ export class SessionIdManager { clearTimeout(this._enforceIdleTimeout) this._enforceIdleTimeout = setTimeout(() => { // enforce idle timeout a little after the session timeout to ensure the session is reset even without activity - this.resetSessionId() + // this.resetSessionId() }, this.sessionTimeoutMs * 1.1) } From 5a10768b18dc10dea5bc8ee96014615a5c1880c4 Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Thu, 28 Nov 2024 12:01:02 +0000 Subject: [PATCH 3/4] inch forwards --- src/__tests__/sessionid.test.ts | 41 +++++++++++++++++++++++++++++++++ src/sessionid.ts | 15 ++++++++---- 2 files changed, 51 insertions(+), 5 deletions(-) diff --git a/src/__tests__/sessionid.test.ts b/src/__tests__/sessionid.test.ts index 720e908af..e08baa263 100644 --- a/src/__tests__/sessionid.test.ts +++ b/src/__tests__/sessionid.test.ts @@ -43,6 +43,7 @@ describe('Session ID manager', () => { disabled: false, } ;(sessionStore.is_supported as jest.Mock).mockReturnValue(true) + // @ts-expect-error - TS gets confused about the types here jest.spyOn(global, 'Date').mockImplementation(() => new originalDate(now)) ;(uuidv7 as jest.Mock).mockReturnValue('newUUID') ;(uuid7ToTimestampMs as jest.Mock).mockReturnValue(timestamp) @@ -370,4 +371,44 @@ describe('Session ID manager', () => { expect(console.warn).toBeCalledTimes(3) }) }) + + describe('proactive idle timeout', () => { + it('starts a timer', () => { + expect(sessionIdMgr(persistence)['_enforceIdleTimeout']).toBeDefined() + }) + + it('sets a new timer when checking session id', () => { + const sessionIdManager = sessionIdMgr(persistence) + const originalTimer = sessionIdManager['_enforceIdleTimeout'] + sessionIdManager.checkAndGetSessionAndWindowId(undefined, timestamp) + expect(sessionIdManager['_enforceIdleTimeout']).toBeDefined() + expect(sessionIdManager['_enforceIdleTimeout']).not.toEqual(originalTimer) + }) + + it('does not set a new timer when read only checking session id', () => { + const sessionIdManager = sessionIdMgr(persistence) + const originalTimer = sessionIdManager['_enforceIdleTimeout'] + sessionIdManager.checkAndGetSessionAndWindowId(true, timestamp) + expect(sessionIdManager['_enforceIdleTimeout']).toBeDefined() + expect(sessionIdManager['_enforceIdleTimeout']).toEqual(originalTimer) + }) + + /** timer doesn't advance and fire this? */ + it.skip('resets session id despite no activity after timeout', () => { + ;(uuidv7 as jest.Mock).mockImplementationOnce(() => 'originalUUID') + + const sessionIdManager = sessionIdMgr(persistence) + const { sessionId: originalSessionId } = sessionIdManager.checkAndGetSessionAndWindowId( + undefined, + timestamp + ) + expect(originalSessionId).toBeDefined() + + jest.advanceTimersByTime(DEFAULT_SESSION_IDLE_TIMEOUT_SECONDS * 1.1 + 1) + + const { sessionId: finalSessionId } = sessionIdManager.checkAndGetSessionAndWindowId(undefined, timestamp) + expect(finalSessionId).toBeDefined() + expect(finalSessionId).not.toEqual(originalSessionId) + }) + }) }) diff --git a/src/sessionid.ts b/src/sessionid.ts index eecccbccc..749ed0871 100644 --- a/src/sessionid.ts +++ b/src/sessionid.ts @@ -61,6 +61,7 @@ export class SessionIdManager { ) * 1000 instance.register({ $configured_session_timeout_ms: this._sessionTimeoutMs }) + this.resetIdleTimer() this._window_id_storage_key = 'ph_' + persistenceName + '_window_id' this._primary_window_exists_storage_key = 'ph_' + persistenceName + '_primary_window_exists' @@ -253,11 +254,7 @@ export class SessionIdManager { this._setSessionId(sessionId, newActivityTimestamp, sessionStartTimestamp) if (!readOnly) { - clearTimeout(this._enforceIdleTimeout) - this._enforceIdleTimeout = setTimeout(() => { - // enforce idle timeout a little after the session timeout to ensure the session is reset even without activity - // this.resetSessionId() - }, this.sessionTimeoutMs * 1.1) + this.resetIdleTimer() } if (valuesChanged) { @@ -278,4 +275,12 @@ export class SessionIdManager { lastActivityTimestamp: lastActivityTimestamp, } } + + private resetIdleTimer() { + clearTimeout(this._enforceIdleTimeout) + this._enforceIdleTimeout = setTimeout(() => { + // enforce idle timeout a little after the session timeout to ensure the session is reset even without activity + this.resetSessionId() + }, this.sessionTimeoutMs * 1.1) + } } From 9663464002ed32ebdc4a9a6cbcd0db78d6c18ae8 Mon Sep 17 00:00:00 2001 From: Paul D'Ambra Date: Mon, 16 Dec 2024 11:16:07 +0000 Subject: [PATCH 4/4] fix --- src/__tests__/decide.test.ts | 0 1 file changed, 0 insertions(+), 0 deletions(-) delete mode 100644 src/__tests__/decide.test.ts diff --git a/src/__tests__/decide.test.ts b/src/__tests__/decide.test.ts deleted file mode 100644 index e69de29bb..000000000