diff --git a/apps/sim/lib/credentials/oauth-chat-attempt.test.ts b/apps/sim/lib/credentials/oauth-chat-attempt.test.ts index 99ae6e78bb1..d88e5231542 100644 --- a/apps/sim/lib/credentials/oauth-chat-attempt.test.ts +++ b/apps/sim/lib/credentials/oauth-chat-attempt.test.ts @@ -4,6 +4,7 @@ */ import { beforeEach, describe, expect, it } from 'vitest' +import type { OAuthChatAttempt, OAuthChatAttemptStatus } from '@/lib/credentials/oauth-chat-attempt' import { addOAuthChatAttemptToAuthorizeUrl, buildOAuthChatCompleteAuthorizeUrl, @@ -11,6 +12,7 @@ import { getOAuthCredentialBaseline, hasOAuthCredentialChanged, OAUTH_CHAT_ATTEMPT_EVENT, + OAUTH_CHAT_ATTEMPT_MAX_AGE_MS, OAUTH_CHAT_ATTEMPT_PARAM, OAUTH_CHAT_COMPLETE_PATH, OAUTH_CHAT_RETURN_TO_PARAM, @@ -154,6 +156,126 @@ describe('OAuth chat attempts', () => { expect(resolveActiveDesktopOAuthChatAttempt('connected')).toBeNull() }) + const SWEEP_INPUT = { + workspaceId: 'workspace-1', + providerId: 'slack', + baseProviderId: 'slack', + displayName: 'Slack', + controlId: 'message-1:0:0', + baselineCredentialIds: [], + } + const SWEEP_LATEST_KEY = 'sim.oauth-chat-latest.workspace-1.slack.message-1%3A0%3A0.' + const RETENTION_MS = 24 * 60 * 60 * 1000 + + /** Storage is index-addressed and its keys are not own-enumerable in jsdom. */ + function storageKeysWithPrefix(prefix: string): string[] { + const keys: string[] = [] + for (let index = 0; index < window.localStorage.length; index++) { + const key = window.localStorage.key(index) + if (key?.startsWith(prefix)) keys.push(key) + } + return keys.sort() + } + + /** Backdates a stored attempt, then starts an unrelated one to trigger the sweep. */ + function ageAttemptThenSweep( + attempt: OAuthChatAttempt, + ageMs: number, + status: OAuthChatAttemptStatus + ): void { + window.localStorage.setItem( + `sim.oauth-chat-attempt.${attempt.id}`, + JSON.stringify({ ...attempt, requestedAt: attempt.requestedAt - ageMs, status }) + ) + createOAuthChatAttempt({ ...SWEEP_INPUT, controlId: 'message-1:9:9' }) + } + + it('sweeps resolved attempts and their latest-pointers past the retention window', () => { + const stale = createOAuthChatAttempt(SWEEP_INPUT) + expect(window.localStorage.getItem(SWEEP_LATEST_KEY)).toBe(stale.id) + + ageAttemptThenSweep(stale, RETENTION_MS + 1, 'connected') + + expect(window.localStorage.getItem(`sim.oauth-chat-attempt.${stale.id}`)).toBeNull() + expect(window.localStorage.getItem(SWEEP_LATEST_KEY)).toBeNull() + }) + + it('keeps a resolved attempt a mounted row still reads past the lookup cutoff', () => { + const settled = createOAuthChatAttempt(SWEEP_INPUT) + + ageAttemptThenSweep(settled, OAUTH_CHAT_ATTEMPT_MAX_AGE_MS + 1, 'connected') + + // A chip recomputes itself from storage on every attempt event, and on the + // reconnect path the record is its only source of connected state — so + // sweeping at the lookup cutoff would revert a row that is still on screen. + expect(readOAuthChatAttempt(settled.id)?.status).toBe('connected') + }) + + it('keeps a pending attempt past the read cutoff so a late verdict still lands', () => { + const parked = createOAuthChatAttempt(SWEEP_INPUT) + + ageAttemptThenSweep(parked, OAUTH_CHAT_ATTEMPT_MAX_AGE_MS + 1, 'pending') + + // The read cutoff already hides it from the latest-pointer lookup, but the + // record itself must survive: a popup parked this long can still return. + expect(readLatestOAuthChatAttempt(SWEEP_INPUT)).toBeNull() + expect(setOAuthChatAttemptStatus(parked.id, 'connected')?.status).toBe('connected') + }) + + it('keeps a late verdict alive after it lands on a long-parked attempt', () => { + const parked = createOAuthChatAttempt(SWEEP_INPUT) + ageAttemptThenSweep(parked, OAUTH_CHAT_ATTEMPT_MAX_AGE_MS + 1, 'pending') + + // The verdict arrives long after the request, so the record is only young + // by resolution. Aging it from requestedAt would make it sweepable at once. + expect(setOAuthChatAttemptStatus(parked.id, 'connected')?.status).toBe('connected') + createOAuthChatAttempt({ ...SWEEP_INPUT, controlId: 'message-1:8:8' }) + + expect(readOAuthChatAttempt(parked.id)?.status).toBe('connected') + }) + + it('sweeps many adjacent stale records in one pass', () => { + const template = createOAuthChatAttempt(SWEEP_INPUT) + const staleIds = [0, 1, 2, 3, 4, 5].map((slot) => `stale-attempt-${slot}`) + // Written directly, so the records land in consecutive storage slots with + // no latest-pointer between them — interleaved keys would mask the skip. + for (const staleId of staleIds) { + window.localStorage.setItem( + `sim.oauth-chat-attempt.${staleId}`, + JSON.stringify({ + ...template, + id: staleId, + status: 'connected', + resolvedAt: template.requestedAt - RETENTION_MS - 1, + }) + ) + } + + createOAuthChatAttempt({ ...SWEEP_INPUT, controlId: 'message-1:9:9' }) + + // Removing entries mid-scan would shift every later key down a slot and + // skip the next one, leaving about half of these behind. + expect(storageKeysWithPrefix('sim.oauth-chat-attempt.stale-attempt-')).toEqual([]) + }) + + it('sweeps a pending attempt once it is past the retention window', () => { + const abandoned = createOAuthChatAttempt(SWEEP_INPUT) + + ageAttemptThenSweep(abandoned, RETENTION_MS + 1, 'pending') + + expect(window.localStorage.getItem(`sim.oauth-chat-attempt.${abandoned.id}`)).toBeNull() + expect(window.localStorage.getItem(SWEEP_LATEST_KEY)).toBeNull() + }) + + it('keeps unexpired attempts across a sweep', () => { + const live = createOAuthChatAttempt(SWEEP_INPUT) + + createOAuthChatAttempt({ ...SWEEP_INPUT, controlId: 'message-1:9:9' }) + + expect(readOAuthChatAttempt(live.id)?.id).toBe(live.id) + expect(window.localStorage.getItem(SWEEP_LATEST_KEY)).toBe(live.id) + }) + it('resolves the exact correlated desktop attempt without consuming a sibling', () => { const first = createOAuthChatAttempt({ workspaceId: 'workspace-1', diff --git a/apps/sim/lib/credentials/oauth-chat-attempt.ts b/apps/sim/lib/credentials/oauth-chat-attempt.ts index 0a6fd6bed3e..85588c9f5a8 100644 --- a/apps/sim/lib/credentials/oauth-chat-attempt.ts +++ b/apps/sim/lib/credentials/oauth-chat-attempt.ts @@ -43,6 +43,8 @@ export interface OAuthChatAttempt { baselineCredentialUpdatedAt?: string requestedAt: number status: OAuthChatAttemptStatus + /** When {@link status} last left 'pending'. Absent on records written before this was tracked. */ + resolvedAt?: number } interface CreateOAuthChatAttemptInput { @@ -136,6 +138,7 @@ function isOAuthChatAttempt(value: unknown): value is OAuthChatAttempt { candidate.status === 'connected' || candidate.status === 'failed') && (candidate.credentialId === undefined || typeof candidate.credentialId === 'string') && + (candidate.resolvedAt === undefined || typeof candidate.resolvedAt === 'number') && Array.isArray(candidate.baselineCredentialIds) && candidate.baselineCredentialIds.every((credentialId) => typeof credentialId === 'string') && (candidate.baselineCredentialUpdatedAt === undefined || @@ -157,11 +160,85 @@ function writeOAuthChatAttempt(attempt: OAuthChatAttempt): void { ) } +/** + * How long a record is kept after its last meaningful activity, whatever its + * status. Deliberately far longer than {@link OAUTH_CHAT_ATTEMPT_MAX_AGE_MS}, + * which only governs what a *lookup* will honour — a record still has readers + * after that cutoff, and removing one out from under them is visible: + * + * - A pending record is a flow whose window may yet return. + * {@link readOAuthChatAttempt} applies no age gate, so a parked popup can + * still publish a verdict through {@link setOAuthChatAttemptStatus}. + * - A resolved record still backs a mounted chip. That row recomputes itself + * from storage on every attempt event, and for a reconnect its connected + * state has no other source, so sweeping the record reverts the row. + * + * Sweeping on the lookup cutoff — as the common OIDC client implementations do + * — breaks both. A day is past any live consent flow or session in which a row + * is still on screen, while still bounding what abandoned flows leave behind. + */ +const OAUTH_CHAT_ATTEMPT_RETENTION_MS = 24 * 60 * 60 * 1000 + +/** + * Drops attempt records that can no longer inform a reader, along with the + * latest-pointers aimed at them, so resolved flows stop accumulating for the + * life of the browser profile. + * + * Runs on create rather than on resolve: a verdict is published by status + * update and the row re-reads the attempt by id immediately afterwards, so + * removing it at that point would race the reader. + */ +function pruneExpiredOAuthChatAttempts(now: number): void { + const expiredAttemptIds = new Set() + const staleKeys: string[] = [] + + // Keys are collected before any removal: localStorage is index-addressed, and + // removing mid-scan shifts every later entry down one slot, skipping it. + for (let index = 0; index < window.localStorage.length; index++) { + const key = window.localStorage.key(index) + if (!key?.startsWith(OAUTH_CHAT_ATTEMPT_KEY_PREFIX)) continue + const attemptId = key.slice(OAUTH_CHAT_ATTEMPT_KEY_PREFIX.length) + const attempt = readOAuthChatAttempt(attemptId) + // An unparseable or malformed record can never be read back, so it is + // collected too rather than left behind forever. + if (attempt) { + // Age from the last thing that happened to the record. Measuring a + // resolved one from `requestedAt` would make a verdict that landed late + // sweepable the instant it arrived. + const lastActivityAt = attempt.resolvedAt ?? attempt.requestedAt + if (now - lastActivityAt <= OAUTH_CHAT_ATTEMPT_RETENTION_MS) continue + } + expiredAttemptIds.add(attemptId) + staleKeys.push(key) + } + + if (expiredAttemptIds.size === 0) return + + for (let index = 0; index < window.localStorage.length; index++) { + const key = window.localStorage.key(index) + if (!key?.startsWith(OAUTH_CHAT_LATEST_KEY_PREFIX)) continue + const pointedAttemptId = window.localStorage.getItem(key) + if (pointedAttemptId && expiredAttemptIds.has(pointedAttemptId)) staleKeys.push(key) + } + + for (const key of staleKeys) { + window.localStorage.removeItem(key) + } +} + export function createOAuthChatAttempt(input: CreateOAuthChatAttemptInput): OAuthChatAttempt { + const requestedAt = Date.now() + if (typeof window !== 'undefined') { + // Same reasoning as the write path: a blocked or full store must not throw + // into the caller, and losing a sweep only defers it to the next attempt. + try { + pruneExpiredOAuthChatAttempts(requestedAt) + } catch {} + } const attempt: OAuthChatAttempt = { ...input, id: generateShortId(24), - requestedAt: Date.now(), + requestedAt, status: 'pending', } writeOAuthChatAttempt(attempt) @@ -200,7 +277,11 @@ export function setOAuthChatAttemptStatus( ): OAuthChatAttempt | null { const attempt = readOAuthChatAttempt(attemptId) if (!attempt) return null - const updated = { ...attempt, status } + const updated: OAuthChatAttempt = { + ...attempt, + status, + resolvedAt: status === 'pending' ? undefined : Date.now(), + } writeOAuthChatAttempt(updated) return updated }