From afd7ffc86627d19478e939e46a443d1b0845409b Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 8 Aug 2026 18:28:51 -0700 Subject: [PATCH 1/3] improvement(mship): sweep spent OAuth chat attempts out of localStorage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Attempt records were written on every chat OAuth connect and never removed, so they accumulated in the store for the life of the browser profile. Reads already refuse an attempt past the 15m cutoff, so the residue was inert — but unbounded. Collect them on create instead: resolved records go at the read cutoff, and still-pending ones only after a 24h abandoned grace. The grace matters — readOAuthChatAttempt applies no age gate, so a consent window parked past 15m can still publish a verdict, and sweeping on age alone (what the common OIDC clients do) would drop that record and strand the row on 'pending'. --- .../credentials/oauth-chat-attempt.test.ts | 65 +++++++++++++++++ .../sim/lib/credentials/oauth-chat-attempt.ts | 69 ++++++++++++++++++- 2 files changed, 133 insertions(+), 1 deletion(-) diff --git a/apps/sim/lib/credentials/oauth-chat-attempt.test.ts b/apps/sim/lib/credentials/oauth-chat-attempt.test.ts index 99ae6e78bb1..46dced53be2 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,69 @@ 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 PENDING_GRACE_MS = 24 * 60 * 60 * 1000 + + /** 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 expired attempts and their latest-pointers when a new one starts', () => { + const stale = createOAuthChatAttempt(SWEEP_INPUT) + expect(window.localStorage.getItem(SWEEP_LATEST_KEY)).toBe(stale.id) + + ageAttemptThenSweep(stale, OAUTH_CHAT_ATTEMPT_MAX_AGE_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 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('sweeps a pending attempt once it is past the abandoned grace period', () => { + const abandoned = createOAuthChatAttempt(SWEEP_INPUT) + + ageAttemptThenSweep(abandoned, PENDING_GRACE_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..a00fff503e2 100644 --- a/apps/sim/lib/credentials/oauth-chat-attempt.ts +++ b/apps/sim/lib/credentials/oauth-chat-attempt.ts @@ -157,11 +157,78 @@ function writeOAuthChatAttempt(attempt: OAuthChatAttempt): void { ) } +/** + * Grace period before a still-pending attempt is collected. A pending record is + * a flow whose window may yet return: {@link readOAuthChatAttempt} applies no + * age gate, so a popup parked past the read cutoff can still publish a verdict + * through {@link setOAuthChatAttemptStatus}. Sweeping on age alone — as the + * common OIDC client implementations do — would drop that record and strand the + * row on 'pending' with no event to correct it. A day is far past any live + * consent flow while still bounding what an abandoned one can leave behind. + */ +const OAUTH_CHAT_ATTEMPT_PENDING_GRACE_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) { + const age = now - attempt.requestedAt + const maxAge = + attempt.status === 'pending' + ? OAUTH_CHAT_ATTEMPT_PENDING_GRACE_MS + : OAUTH_CHAT_ATTEMPT_MAX_AGE_MS + if (age <= maxAge) 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) From d5a22f4aab89a82ff4b972dde58f1775c144aa6c Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 8 Aug 2026 18:36:32 -0700 Subject: [PATCH 2/3] fix(mship): age a resolved attempt from when it resolved, not when it was requested Review round 1 findings. A pending attempt held by the 24h grace is by definition older than the 15m cutoff, so the moment a late verdict landed on it the record became instantly sweepable and the next create erased it. Every chip re-reads its row on the event create dispatches, so a connected chip dropped straight back to unset -- the grace period was defeating its own purpose. Resolved records now age from resolvedAt, giving the UI the full window to observe a verdict however late it arrives. Legacy records without the field fall back to requestedAt. Also makes the multi-record sweep test a real guard: its records are written directly so they occupy consecutive storage slots. The earlier version interleaved latest-pointers between them, which masked the index shift -- a remove-during-scan regression passed it. --- .../credentials/oauth-chat-attempt.test.ts | 46 +++++++++++++++++++ .../sim/lib/credentials/oauth-chat-attempt.ts | 19 +++++++- 2 files changed, 63 insertions(+), 2 deletions(-) diff --git a/apps/sim/lib/credentials/oauth-chat-attempt.test.ts b/apps/sim/lib/credentials/oauth-chat-attempt.test.ts index 46dced53be2..4f33cac71dc 100644 --- a/apps/sim/lib/credentials/oauth-chat-attempt.test.ts +++ b/apps/sim/lib/credentials/oauth-chat-attempt.test.ts @@ -167,6 +167,16 @@ describe('OAuth chat attempts', () => { const SWEEP_LATEST_KEY = 'sim.oauth-chat-latest.workspace-1.slack.message-1%3A0%3A0.' const PENDING_GRACE_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, @@ -201,6 +211,42 @@ describe('OAuth chat attempts', () => { expect(setOAuthChatAttemptStatus(parked.id, 'connected')?.status).toBe('connected') }) + it('keeps a late verdict alive after it lands on a grace-preserved 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 - OAUTH_CHAT_ATTEMPT_MAX_AGE_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 abandoned grace period', () => { const abandoned = createOAuthChatAttempt(SWEEP_INPUT) diff --git a/apps/sim/lib/credentials/oauth-chat-attempt.ts b/apps/sim/lib/credentials/oauth-chat-attempt.ts index a00fff503e2..3af2cab74ea 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 || @@ -191,7 +194,15 @@ function pruneExpiredOAuthChatAttempts(now: number): void { // An unparseable or malformed record can never be read back, so it is // collected too rather than left behind forever. if (attempt) { - const age = now - attempt.requestedAt + // A resolved record ages from when it was resolved, not from when it was + // requested. Aging it from `requestedAt` would make the verdict on a + // grace-preserved pending record sweepable the instant it landed, and the + // next create would erase it — every chip re-reads its row on the event + // that create dispatches, so the row would drop straight back to unset. + const age = + attempt.status === 'pending' + ? now - attempt.requestedAt + : now - (attempt.resolvedAt ?? attempt.requestedAt) const maxAge = attempt.status === 'pending' ? OAUTH_CHAT_ATTEMPT_PENDING_GRACE_MS @@ -267,7 +278,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 } From 1652f4e18f24c6ea449bd6f51c9402acee5b91ce Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 8 Aug 2026 18:47:04 -0700 Subject: [PATCH 3/3] fix(mship): retain attempt records for a day regardless of status The 15m resolved cutoff was still short enough to pull a record out from under a mounted row. A chip recomputes itself from storage on every attempt event, and on the reconnect path connectedFromWorkspaceChange is forced false, so connected collapses to exactly the stored status. Any connect click 15m after a reconnect swept that record -- via the CustomEvent in the same tab or the storage event these removals now fire in others -- and reverted the row from Connected. Non-reconnect rows kept the label but silently lost their lock. OAUTH_CHAT_ATTEMPT_MAX_AGE_MS governs what a lookup honours, not how long a record has readers. Retention is now one flat window measured from last activity, which also drops the status branch. --- .../credentials/oauth-chat-attempt.test.ts | 25 ++++++++--- .../sim/lib/credentials/oauth-chat-attempt.ts | 43 +++++++++---------- 2 files changed, 39 insertions(+), 29 deletions(-) diff --git a/apps/sim/lib/credentials/oauth-chat-attempt.test.ts b/apps/sim/lib/credentials/oauth-chat-attempt.test.ts index 4f33cac71dc..d88e5231542 100644 --- a/apps/sim/lib/credentials/oauth-chat-attempt.test.ts +++ b/apps/sim/lib/credentials/oauth-chat-attempt.test.ts @@ -165,7 +165,7 @@ describe('OAuth chat attempts', () => { baselineCredentialIds: [], } const SWEEP_LATEST_KEY = 'sim.oauth-chat-latest.workspace-1.slack.message-1%3A0%3A0.' - const PENDING_GRACE_MS = 24 * 60 * 60 * 1000 + 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[] { @@ -190,16 +190,27 @@ describe('OAuth chat attempts', () => { createOAuthChatAttempt({ ...SWEEP_INPUT, controlId: 'message-1:9:9' }) } - it('sweeps resolved expired attempts and their latest-pointers when a new one starts', () => { + 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, OAUTH_CHAT_ATTEMPT_MAX_AGE_MS + 1, 'connected') + 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) @@ -211,7 +222,7 @@ describe('OAuth chat attempts', () => { expect(setOAuthChatAttemptStatus(parked.id, 'connected')?.status).toBe('connected') }) - it('keeps a late verdict alive after it lands on a grace-preserved attempt', () => { + 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') @@ -235,7 +246,7 @@ describe('OAuth chat attempts', () => { ...template, id: staleId, status: 'connected', - resolvedAt: template.requestedAt - OAUTH_CHAT_ATTEMPT_MAX_AGE_MS - 1, + resolvedAt: template.requestedAt - RETENTION_MS - 1, }) ) } @@ -247,10 +258,10 @@ describe('OAuth chat attempts', () => { expect(storageKeysWithPrefix('sim.oauth-chat-attempt.stale-attempt-')).toEqual([]) }) - it('sweeps a pending attempt once it is past the abandoned grace period', () => { + it('sweeps a pending attempt once it is past the retention window', () => { const abandoned = createOAuthChatAttempt(SWEEP_INPUT) - ageAttemptThenSweep(abandoned, PENDING_GRACE_MS + 1, 'pending') + 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() diff --git a/apps/sim/lib/credentials/oauth-chat-attempt.ts b/apps/sim/lib/credentials/oauth-chat-attempt.ts index 3af2cab74ea..85588c9f5a8 100644 --- a/apps/sim/lib/credentials/oauth-chat-attempt.ts +++ b/apps/sim/lib/credentials/oauth-chat-attempt.ts @@ -161,15 +161,23 @@ function writeOAuthChatAttempt(attempt: OAuthChatAttempt): void { } /** - * Grace period before a still-pending attempt is collected. A pending record is - * a flow whose window may yet return: {@link readOAuthChatAttempt} applies no - * age gate, so a popup parked past the read cutoff can still publish a verdict - * through {@link setOAuthChatAttemptStatus}. Sweeping on age alone — as the - * common OIDC client implementations do — would drop that record and strand the - * row on 'pending' with no event to correct it. A day is far past any live - * consent flow while still bounding what an abandoned one can leave behind. + * 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_PENDING_GRACE_MS = 24 * 60 * 60 * 1000 +const OAUTH_CHAT_ATTEMPT_RETENTION_MS = 24 * 60 * 60 * 1000 /** * Drops attempt records that can no longer inform a reader, along with the @@ -194,20 +202,11 @@ function pruneExpiredOAuthChatAttempts(now: number): void { // An unparseable or malformed record can never be read back, so it is // collected too rather than left behind forever. if (attempt) { - // A resolved record ages from when it was resolved, not from when it was - // requested. Aging it from `requestedAt` would make the verdict on a - // grace-preserved pending record sweepable the instant it landed, and the - // next create would erase it — every chip re-reads its row on the event - // that create dispatches, so the row would drop straight back to unset. - const age = - attempt.status === 'pending' - ? now - attempt.requestedAt - : now - (attempt.resolvedAt ?? attempt.requestedAt) - const maxAge = - attempt.status === 'pending' - ? OAUTH_CHAT_ATTEMPT_PENDING_GRACE_MS - : OAUTH_CHAT_ATTEMPT_MAX_AGE_MS - if (age <= maxAge) continue + // 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)