Skip to content
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

fix: rotate session id proactively #1512

Merged
merged 8 commits into from
Dec 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions src/__tests__/sessionid.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
})
})
})
35 changes: 26 additions & 9 deletions src/sessionid.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,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<typeof setTimeout> | undefined

constructor(instance: PostHog, sessionIdGenerator?: () => string, windowIdGenerator?: () => string) {
if (!instance.persistence) {
throw new Error('SessionIdManager requires a PostHogPersistence instance')
Expand Down Expand Up @@ -60,6 +63,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'
Expand Down Expand Up @@ -168,14 +172,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])
}
Comment on lines +177 to 180
Copy link
Member Author

Choose a reason for hiding this comment

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

NB we can probably remove this upgrade code now but I won't do it here, a lot of time of passed since we added this


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,
Expand Down Expand Up @@ -218,7 +222,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 =
Expand All @@ -228,7 +232,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()
Expand All @@ -244,11 +248,16 @@ 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) {
this.resetIdleTimer()
}

if (valuesChanged) {
this._sessionIdChangedHandlers.forEach((handler) =>
Expand All @@ -265,7 +274,15 @@ export class SessionIdManager {
windowId,
sessionStartTimestamp,
changeReason: valuesChanged ? { noSessionId, activityTimeout, sessionPastMaximumLength } : undefined,
lastActivityTimestamp: lastTimestamp,
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)
}
}
4 changes: 2 additions & 2 deletions src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ export const knownUnsafeEditableEvent = [
*
* Some features of PostHog rely on receiving 100% of these events
*/
export type KnownUnsafeEditableEvent = typeof knownUnsafeEditableEvent[number]
export type KnownUnsafeEditableEvent = (typeof knownUnsafeEditableEvent)[number]

/**
* These are known events PostHog events that can be processed by the `beforeCapture` function
Expand Down Expand Up @@ -769,7 +769,7 @@ export type ErrorMetadata = {
// and to avoid relying on a frequently changing @sentry/types dependency
// but provided as an array of literal types, so we can constrain the level below
export const severityLevels = ['fatal', 'error', 'warning', 'log', 'info', 'debug'] as const
export declare type SeverityLevel = typeof severityLevels[number]
export declare type SeverityLevel = (typeof severityLevels)[number]

export interface ErrorProperties {
$exception_type: string
Expand Down
Loading