diff --git a/CHANGELOG.md b/CHANGELOG.md index 74f5ff09..d2e41c6f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,38 @@ auto-generated per-PR notes; this file is the curated, human-readable history. flip above a row near the viewport bottom, and oversized menus scroll within the viewport instead of placing Add/Cancel off-screen. +### Fixed +- **An involuntary auth loss (expired/invalid token, failed refresh, or a + concurrent first-contact authentication failure) now runs the same + mandatory authenticated-session teardown as an explicit sign-out** (#502). + Previously only `app.signOut` cancelled the in-flight Workbench + request (and issued its server-side `KILL QUERY`), cancelled the schema + graph and both export modes, invalidated the catalog, and closed the + documentation pane before rendering login; the `onAuthLost` path skipped + all of it, so a mid-flight query/export/lineage stream — or a still-completing + request that would go on to record History, bound parameters, or a schema + reload — could survive past the login screen. The teardown is now a single + idempotent `teardownAuthenticatedSession()` helper shared by both paths. + Making this actually work required deeper changes to + `connection-session.ts`: `getToken()` no longer clears credentials itself + on a failed proactive refresh (it did, previously — before its caller ever + got a chance to run the teardown), and `chCtx.onSignedOut` now (a) + notifies the teardown *before* clearing credentials, not after, freezing + `getToken` for that window to resolve the still-retained credential (even + one within the proactive-refresh skew, or already past real expiry — a + best-effort `KILL QUERY` costs nothing extra either way) rather than + discarding it, so the teardown's `KILL QUERY`/export-cancellation requests + reach the server instead of silently no-op'ing on a token that looks gone; + (b) latches once it starts handling a dead session so the teardown's own + concurrent authenticated calls rediscovering the same dead token can't + recursively re-run it, resetting only on a genuine fresh sign-in; and (c) + still clears credentials even if the injected `onAuthLost` callback + throws. A detached/expanded Data Pane view's own refresh preflight + (`src/ui/results.ts`) also now reports auth loss the same way every other + caller does — previously it neither cleared nor reported it, so a dead + token could linger un-torn-down indefinitely if that was a user's only + live activity. + ### Removed - **The unused saved-query repair planner has been removed** (#429 phase 6 / #500). Direct, ownership-safe Panel and Dashboard trash actions are the diff --git a/src/application/connection-session.ts b/src/application/connection-session.ts index 185608fa..a229f6f3 100644 --- a/src/application/connection-session.ts +++ b/src/application/connection-session.ts @@ -124,6 +124,14 @@ export function createConnectionSession(deps: ConnectionSessionDeps): Connection let token: string | null = ss.getItem('oauth_id_token'); let refreshTok: string | null = ss.getItem('oauth_refresh_token'); let authMode: 'oauth' | 'basic' = ss.getItem('ch_basic_auth') ? 'basic' : 'oauth'; + // #502: latches once `chCtx.onSignedOut` starts handling a dead session, so + // concurrent/nested reports of the SAME auth loss (e.g. the teardown's own + // fire-and-forget KILL QUERY/export-cancel calls independently discovering + // the same expired-and-unrefreshable token) can't re-run the teardown, + // re-render login, or re-attempt `refresh()` against the token endpoint. + // Reset only by a genuine fresh sign-in (`setTokens`/`connectBasic`), so a + // LATER, distinct auth-loss event in the new session is handled normally. + let signedOutHandled = false; const basicCreds = (): string | null => ss.getItem('ch_basic_auth'); const basicUser = (): string => ss.getItem('ch_basic_user') || ''; const originHost = (o: string): string => { try { return new URL(o).host; } catch { return ''; } }; @@ -182,6 +190,9 @@ export function createConnectionSession(deps: ConnectionSessionDeps): Connection // tokens. (The refresh path also lands here; they're already gone → no-op.) ss.removeItem('oauth_verifier'); ss.removeItem('oauth_state'); + // #502: a fresh/refreshed token means this is a live session again — a + // LATER auth loss is a distinct event `onSignedOut` must handle again. + signedOutHandled = false; } function clearTokens(): void { token = null; @@ -244,7 +255,14 @@ export function createConnectionSession(deps: ConnectionSessionDeps): Connection if (!token) return null; if (!isTokenExpired(token)) return token; if (await refresh()) return token; - clearTokens(); + // #502: deliberately NOT `clearTokens()` here. Every caller of `getToken` + // already does `if (!(await getToken())) { }` — that path's authenticated teardown (KILL QUERY, + // export cancellation) needs the still-current `token`/`authMode` this + // closure holds to actually reach the server; clearing it here, before + // any of those callers ever get a chance to run, was making every one of + // them silently no-op on an already-missing token. `onSignedOut` clears + // once its teardown has run. return null; } @@ -278,9 +296,56 @@ export function createConnectionSession(deps: ConnectionSessionDeps): Connection authHeader, // detail is set when CH rejects a *valid* login (authorization denial); the // no-arg calls (no token / expired + refresh failed) fall back to expiry. + // #502: the shell-owned teardown `deps.onAuthLost` runs (aborting the + // workbench/graph/export requests and KILLing their server-side queries) + // must authenticate its own fire-and-forget calls with the credential + // this dying session held — even one `getToken()`'s 60s skew calls + // "expiring soon", or one already past real expiry: a best-effort KILL + // QUERY costs nothing extra either way, and it's likely still accepted. + // The plain `ctx.getToken()` can't be reused as-is for this: it treats a + // failed *proactive* refresh as terminal and returns null regardless of + // whether the retained token is still good, so every nested + // `authedFetch` inside the teardown would see `!token` and bail before + // ever reaching the server. `chCtx.getToken` is frozen to resolve this + // exact retained token — computed and captured BEFORE the teardown + // runs — for the synchronous duration of `deps.onAuthLost`, then + // restored. This works because `authedFetch` reads+calls `ctx.getToken` + // synchronously, before its own first `await` — every nested call + // captures the frozen value before this function's `finally` below ever + // runs (fire-and-forget calls only kick off their promises here; they + // don't settle until well after this function returns). + // + // `ctx.authHeader`/`ctx.refresh`, in contrast, are read by `authedFetch` + // only in the continuation AFTER its own `await ctx.getToken()` — i.e. + // in a later microtask (or later still, `ctx.refresh` only fires on an + // actual 401 response, after a real network round-trip) — by which + // point this function has already returned and restored/cleared + // everything. Freezing them here would be a no-op that only creates a + // false sense of protection, so this deliberately does NOT try: for a + // Basic-mode session specifically, this means a stray retry can still + // build the wrong Authorization scheme once `authMode` has been reset + // (tracked separately as `inbox` #520, not fixed here — a real fix needs + // the fire-and-forget calls to be awaited, or their credentials captured + // and threaded through explicitly, before this function ever restores or + // clears anything). + // + // `signedOutHandled` guards re-entrancy: the teardown's own frozen calls, + // if they still fail, report auth loss again but that's a no-op (no + // second render/teardown) until a fresh sign-in (`setTokens`/ + // `connectBasic`) resets it. The `finally` guarantees credentials are + // still cleared even if the injected `onAuthLost` throws. onSignedOut: (detail?: string) => { - clearTokens(); - deps.onAuthLost(detail || 'Your session expired — please sign in again.'); + if (signedOutHandled) return; + signedOutHandled = true; + const frozenToken = authMode === 'basic' ? basicCreds() : token; + const realGetToken = chCtx.getToken; + chCtx.getToken = async () => frozenToken; + try { + deps.onAuthLost(detail || 'Your session expired — please sign in again.'); + } finally { + chCtx.getToken = realGetToken; + clearTokens(); + } }, }; @@ -326,6 +391,8 @@ export function createConnectionSession(deps: ConnectionSessionDeps): Connection ss.setItem('ch_basic_user', user); ss.setItem('ch_basic_origin', target); chCtx.origin = target; + // #502: a fresh Basic session is live again — see `setTokens`'s own reset. + signedOutHandled = false; } // --- dashboard (#149 D1) ------------------------------------------------- diff --git a/src/ui/app.ts b/src/ui/app.ts index cc7fa66f..8084017a 100644 --- a/src/ui/app.ts +++ b/src/ui/app.ts @@ -455,6 +455,28 @@ export function createApp(env: CreateAppEnv = {}): App { disposeShell(); renderLogin(app as App & { root: Element }, msg); }; + // #502: cancel/tear down every in-flight authenticated operation BEFORE + // credentials are cleared and login renders — required for both an + // explicit sign-out AND an involuntary auth loss (expired/invalid token, + // failed refresh, or a first-contact auth failure via `onAuthLost` below), + // so a mid-flight query/export/lineage stream can never land (or repaint) + // after the login screen is showing; invalidating the catalog means a + // later sign-in (possibly a different server) never sees stale + // schema/reference caches. Idempotent: several concurrent requests may + // each report auth loss. The workbench session stays reusable after + // destroy() — the next renderApp re-attaches its shell effects. + const teardownAuthenticatedSession = (): void => { + workbench.destroy(); + // Plain abort (no clearResult settle) — the login render replaces the + // whole DOM next, so settling the visible result would be a wasted paint. + graph.cancel(); + exportService.cancelExport(); + exportService.cancelExportScript(); + catalog.invalidate(); + // #313: pane content must never survive a connection change — closed + // alongside the catalog reset, before the login screen renders. + closeDocPane(app); + }; // The auth + config + ClickHouse connection lifecycle (#276 Phase 2) — OAuth // PKCE login/refresh, Basic probing, and IdP config resolution live in // `application/connection-session.ts`, @@ -464,7 +486,12 @@ export function createApp(env: CreateAppEnv = {}): App { const conn = createConnectionSession({ fetch: fetchFn, storage: ss, location: loc, crypto: cryptoObj, queryJson: ch.queryJson, - onAuthLost: (detail) => renderLoginApp(detail), + // #502: onAuthLost is the involuntary counterpart of signOut() below — + // same mandatory teardown ordering, before login replaces the DOM. + onAuthLost: (detail) => { + teardownAuthenticatedSession(); + renderLoginApp(detail); + }, }); app.conn = conn; // THE single live ClickHouse context — owned by the session, aliased locally @@ -478,28 +505,14 @@ export function createApp(env: CreateAppEnv = {}): App { // comment) — no flat `App` delegates (#276 Phase 5 deleted them). // `showLogin`/`signOut` stay app.ts-owned: they compose rendering, not // pure forwards. - // Sign-out is the one real end-of-session event in a single-route tab — - // the first production wiring of the sessions' teardown surfaces (#276 - // Phase 5). Order matters: cancel/tear down every in-flight operation - // BEFORE clearing credentials and rendering login, so a mid-flight - // query/export/lineage stream can never land (or repaint) after the login - // screen is showing; invalidate the catalog so a later sign-in (possibly a - // different server) never sees stale schema/reference caches. The - // workbench session stays reusable after destroy(): the next renderApp - // re-attaches its shell effects. + // Sign-out is the one real explicit end-of-session event in a + // single-route tab (#276 Phase 5); `onAuthLost` above is its involuntary + // counterpart, sharing the same `teardownAuthenticatedSession()` (#502) — + // both must tear down before clearing credentials and rendering login. app.signOut = () => { app.closeShortcutDialog(); resetShortcutChord(app); - workbench.destroy(); - // Plain abort (no clearResult settle) — the login render replaces the - // whole DOM next, so settling the visible result would be a wasted paint. - graph.cancel(); - exportService.cancelExport(); - exportService.cancelExportScript(); - catalog.invalidate(); - // #313: pane content must never survive a connection change — closed - // alongside the catalog reset, before the login screen renders. - closeDocPane(app); + teardownAuthenticatedSession(); conn.signOut(); // #425: the Dashboard teardown, the surface-generation bump and the // main-surface reset live in `renderLoginApp` — the one place that knows the diff --git a/src/ui/results.ts b/src/ui/results.ts index 024f4950..489a6a2e 100644 --- a/src/ui/results.ts +++ b/src/ui/results.ts @@ -161,7 +161,7 @@ export interface ResultsApp { now(): number; elapsedMs(): number; wallNow(): number; - conn: Pick; + conn: Pick & { chCtx: Pick }; /** The shared request/stream/normalize service (#276 Phase 1) — this module * only ever needs `executeRead` (the detached Data view's own re-run). */ exec: Pick; @@ -1008,6 +1008,11 @@ export function expandDataPane(app: ResultsApp, r: QueryResult): DetachedView { if (refreshBtn) refreshBtn.disabled = true; if (!(await app.conn.ensureFreshToken())) { if (myGen === gen && !closed) settle('Not signed in'); + // #522: unlike a dashboard tile's own preflight (`onAuthFailed`), + // this path never reported the auth loss at all — dead credentials + // would otherwise linger un-torn-down until some other action + // happened to rediscover the same dead token. + app.conn.chCtx.onSignedOut(); return; } const execution = panelExecution(savedPanel, mergedSourceSql(src, source.sql), { diff --git a/tests/unit/app.test.ts b/tests/unit/app.test.ts index 63eb28df..2c342de0 100644 --- a/tests/unit/app.test.ts +++ b/tests/unit/app.test.ts @@ -2142,6 +2142,88 @@ describe('query run', () => { expect(result(app.activeTab()).rows).toEqual([['ok']]); expect(app.state.running.value).toBe(false); }); + // #502: `onAuthLost` (a 401/expired-token/failed-refresh — routed here via + // `chCtx.onSignedOut`) is the involuntary counterpart of `signOut()` + // above and must run the same mandatory teardown before login renders, + // not just the Dashboard-surface reset `renderLoginApp` already owns. + it('chCtx.onSignedOut() (involuntary auth loss) tears down the authenticated session exactly like signOut(), and the request that resolves afterward records nothing', async () => { + let resolveRunFetch!: (value: FakeResponse | Promise) => void; + const fetch = asFetch(vi.fn((_url: string, init?: { body?: string }) => { + const sql = (init && init.body) || ''; + if (/CREATE TABLE t\b/.test(sql)) return new Promise((res) => { resolveRunFetch = res; }); + return Promise.resolve(resp({ json: { data: [] } })); // version/schema/reference background loads + })); + const { app, e } = appForRun([], { fetch }); + const invalidateSpy = vi.spyOn(app.catalog, 'invalidate'); + const loadSchemaSpy = vi.spyOn(app.catalog, 'loadSchema'); + const graphCancelSpy = vi.spyOn(app.graph, 'cancel'); + const cancelExportSpy = vi.spyOn(app.exports, 'cancelExport'); + const cancelExportScriptSpy = vi.spyOn(app.exports, 'cancelExportScript'); + const recordBoundParamsSpy = vi.spyOn(app.params, 'recordBoundParams'); + app.openDocEntry({ kind: 'function', name: 'sum' }); // pane opens (lookup resolves unavailable — irrelevant here) + expect(document.querySelector('[role="complementary"]')).not.toBeNull(); + // A schema-mutating statement (#502's own example) so a stray schema + // reload after resolution would show up as a spy call. + app.activeTab().sqlDraft = 'CREATE TABLE t (x UInt8) ENGINE=Memory'; + const pending = app.actions.run(); + await new Promise((r) => setTimeout(r)); + expect(app.state.running.value).toBe(true); + const runCall = asMock(fetch).mock.calls.find((c) => c[1] && /CREATE TABLE t\b/.test(c[1].body || ''))!; + app.conn.chCtx.onSignedOut(); // involuntary auth loss — NOT app.signOut() + // Teardown fired before login rendered, exactly as signOut()'s does. + expect((runCall[1].signal as AbortSignal).aborted).toBe(true); + expect(invalidateSpy).toHaveBeenCalled(); + expect(graphCancelSpy).toHaveBeenCalled(); + expect(cancelExportSpy).toHaveBeenCalled(); + expect(cancelExportScriptSpy).toHaveBeenCalled(); + expect(document.querySelector('[role="complementary"]')).toBeNull(); // doc pane closed + expect(qs(app.root!, '.login-card')).toBeTruthy(); + await new Promise((r) => setTimeout(r)); + const kill = asMock(e.fetch!).mock.calls.find((c) => /KILL QUERY/.test((c[1] && c[1].body) || '')); + expect(kill).toBeTruthy(); + resolveRunFetch(Promise.reject(Object.assign(new Error('aborted'), { name: 'AbortError' }))); + await pending; + // Resolving afterward must record nothing: no History entry, no bound + // params, no schema reload for the DDL that was in flight. + expect(app.state.history.length).toBe(0); + expect(recordBoundParamsSpy).not.toHaveBeenCalled(); + expect(loadSchemaSpy).not.toHaveBeenCalled(); + // Idempotent: a second concurrent report of auth loss must not throw. + expect(() => app.conn.chCtx.onSignedOut()).not.toThrow(); + }); + // #502: the "failed proactive refresh" trigger specifically — getToken() + // treats a token within its 60s skew as needing refresh, and (unlike a + // hard-expired token) discards it on a failed refresh attempt UNLESS the + // teardown uses the frozen snapshot connection-session.ts now captures. + // Without that, workbench.destroy()'s KILL QUERY would never reach the + // server at all for this trigger — it would see a null token and bail. + it('KILL QUERY still reaches the server when the retained token needed (and failed) a proactive refresh', async () => { + const expiringSoonToken = jwt({ email: 'soon@example.com', exp: Math.floor(Date.now() / 1000) + 30 }); + let resolveRunFetch!: (value: FakeResponse | Promise) => void; + const fetch = makeFetch([ + [(u) => /config\.json/.test(u), resp({ json: { issuer: 'https://accounts.google.com', client_id: 'cid' } })], + [(u) => /openid-configuration/.test(u), resp({ json: { authorization_endpoint: 'https://a', token_endpoint: 'https://t' } })], + [(u) => u === 'https://t', resp({ ok: false, status: 401, text: '{}' })], // refresh fails + [(u, sql) => /SELECT 1\b/.test(sql), () => new Promise((res) => { resolveRunFetch = res; })], + ]); + const { app } = appForRun([], { fetch }); // default sessionStorage: a valid (non-expiring) token — run()'s own preflight needs no refresh + app.activeTab().sqlDraft = 'SELECT 1'; + const pending = app.actions.run(); + await new Promise((r) => setTimeout(r)); + expect(app.state.running.value).toBe(true); + const runCall = asMock(fetch).mock.calls.find((c) => c[1] && /SELECT 1\b/.test(c[1].body || ''))!; + // The live token is now this one (e.g. rotated by an earlier refresh + // elsewhere) — expiring soon, and its own refresh attempt fails. + app.conn.setTokens(expiringSoonToken, 'rt0'); + expect(await app.conn.getToken()).toBeNull(); // mirrors any real caller's preflight + app.conn.chCtx.onSignedOut(); // mirrors what that caller does next + expect((runCall[1].signal as AbortSignal).aborted).toBe(true); + await new Promise((r) => setTimeout(r)); + const kill = asMock(fetch).mock.calls.find((c) => /KILL QUERY/.test((c[1] && c[1].body) || '')); + expect(kill).toBeTruthy(); // reached the server using the retained, expiring-soon token + resolveRunFetch(Promise.reject(Object.assign(new Error('aborted'), { name: 'AbortError' }))); + await pending; + }); it('surfaces a query error', async () => { const { app } = appForRun([ [(u, sql) => /bad/.test(sql), resp({ ok: false, status: 500, text: '{"exception":"DB::Exception: nope"}' })], @@ -3340,7 +3422,7 @@ describe('auth flows', () => { expect(ok).toBe(true); expect(app.conn.token()).toBe(validToken); }); - it('getToken returns null + clears when refresh fails', async () => { + it('getToken returns null when refresh fails', async () => { const e = env({ sessionStorage: memSession({ oauth_id_token: jwt({ exp: 1 }) }), fetch: makeFetch([ @@ -3351,16 +3433,29 @@ describe('auth flows', () => { const app = createApp(e); expect(await app.conn.chCtx.getToken()).toBeNull(); }); - it('onSignedOut shows the given message, else a session-expired default', async () => { + // #502: `chCtx.onSignedOut` latches once it starts handling a dead + // session (concurrent/nested reports of the SAME auth loss must not + // re-render login with a second message) — a fresh `app` per case is the + // distinct-session boundary the latch actually gates on. + it('onSignedOut shows the given detail — an authorization denial', async () => { const app = createApp(env()); app.renderApp(); - // authorization denial: CH-supplied message is shown verbatim on the login screen app.conn.chCtx.onSignedOut('ClickHouse denied your account (HTTP 403). Server: nope'); expect(qs(app.root, '.login-error').textContent).toContain('denied your account'); - // genuine expiry: no detail → the reworded default + }); + it('onSignedOut falls back to a session-expired default with no detail', async () => { + const app = createApp(env()); + app.renderApp(); app.conn.chCtx.onSignedOut(); expect(qs(app.root, '.login-error').textContent).toContain('session expired'); }); + it('a second onSignedOut report for the same dead session does not re-render login with a new message', async () => { + const app = createApp(env()); + app.renderApp(); + app.conn.chCtx.onSignedOut('ClickHouse denied your account (HTTP 403). Server: nope'); + app.conn.chCtx.onSignedOut(); // e.g. a concurrent request's own report — no-op + expect(qs(app.root, '.login-error').textContent).toContain('denied your account'); + }); it('login(idp, origin) stashes oauth_origin for a cross-origin cluster; sign-out clears it', async () => { const loc = { host: 'ch', origin: 'https://ch', pathname: '/sql', search: '', hash: '', href: 'https://ch/sql' } as Location; const e = env({ diff --git a/tests/unit/connection-session.test.ts b/tests/unit/connection-session.test.ts index 716ec854..4ee73361 100644 --- a/tests/unit/connection-session.test.ts +++ b/tests/unit/connection-session.test.ts @@ -260,7 +260,15 @@ describe('refresh (via getToken)', () => { await expect(session.getToken()).resolves.toBe('YWJj'); }); - it('clears everything when the token endpoint rejects the refresh', async () => { + // #502: getToken() itself must NOT clear credentials on a failed refresh — + // its caller (every one of them already does + // `if (!(await getToken())) { }`) needs the still- + // current token/authMode readable when it reports auth loss, so its own + // authenticated teardown (KILL QUERY, export cancellation) can actually + // reach the server instead of finding a token that's already gone. + // Clearing is now `chCtx.onSignedOut`'s job alone (see the 'chCtx.onSignedOut' + // describe block below). + it('leaves credentials in place when the token endpoint rejects the refresh — clearing is onSignedOut\'s job', async () => { const { session, storage } = setup({ storage: memStorage({ oauth_id_token: expiredToken, oauth_refresh_token: 'r0', oauth_idp: 'g' }), routes: [(url) => (url.endsWith('/token') ? jsonResponse(401, {}) : null)], @@ -268,15 +276,11 @@ describe('refresh (via getToken)', () => { session.chCtx.authConfirmed = true; const tok = await session.getToken(); expect(tok).toBeNull(); - expect(session.token()).toBeNull(); - expect(session.refreshToken()).toBeNull(); - expect(session.idpId()).toBeNull(); - expect(session.authMode()).toBe('oauth'); - expect(session.chCtx.authConfirmed).toBe(false); - for (const k of [ - 'oauth_id_token', 'oauth_refresh_token', 'oauth_verifier', 'oauth_state', 'oauth_idp', 'oauth_origin', - 'ch_basic_auth', 'ch_basic_user', 'ch_basic_origin', - ]) expect(storage.getItem(k)).toBeNull(); + expect(session.token()).toBe(expiredToken); + expect(session.refreshToken()).toBe('r0'); + expect(session.idpId()).toBe('g'); + expect(session.chCtx.authConfirmed).toBe(true); + expect(storage.getItem('oauth_id_token')).toBe(expiredToken); }); it('returns false when the token endpoint yields no usable bearer', async () => { @@ -487,6 +491,86 @@ describe('chCtx.onSignedOut', () => { session.chCtx.onSignedOut(); expect(onAuthLost).toHaveBeenCalledWith('Your session expired — please sign in again.'); }); + + // #502: onAuthLost must see the still-current credential — its own + // authenticated teardown needs it — and only THEN does the token clear. + it('notifies onAuthLost while the credential is still current, and only clears once it returns', async () => { + const storage = memStorage({ oauth_id_token: expiredToken, oauth_refresh_token: 'r0', oauth_idp: 'g' }); + let idTokenDuringTeardown: string | null | undefined; + const { session } = setup({ + storage, + routes: [(url) => (url.endsWith('/token') ? jsonResponse(401, {}) : null)], + onAuthLost: vi.fn(() => { idTokenDuringTeardown = storage.getItem('oauth_id_token'); }), + }); + // Mirrors authedFetch's own `if (!(await getToken())) { ctx.onSignedOut(); }`. + if (!(await session.getToken())) session.chCtx.onSignedOut(); + expect(idTokenDuringTeardown).toBe(expiredToken); + expect(session.token()).toBeNull(); + expect(storage.getItem('oauth_id_token')).toBeNull(); + }); + + // #502: the plain `getToken()` can't be reused as-is for the teardown's own + // authenticated calls — it would treat an already-expired-or-expiring-soon + // token as needing another refresh and return null on failure, discarding + // a token that might still be accepted. `onSignedOut` freezes `getToken` + // to resolve the retained credential for its own synchronous duration — + // this works because `authedFetch` (ch-client.ts) reads+calls `ctx.getToken` + // synchronously, before its own first `await`, so a nested fire-and-forget + // call captures the frozen value before this function's `finally` restores + // it. (`ctx.authHeader`/`ctx.refresh` are read later, in `authedFetch`'s own + // async continuation — well after this function has already returned — so + // freezing THEM would be a no-op; not attempted here. See #520 for the + // narrower, separately-tracked Basic-mode consequence.) + it('freezes getToken for the teardown window: a nested call gets the still-current (even already-expired) credential', async () => { + let session!: ReturnType['session']; + let getTokenResult: string | null | undefined; + const storage = memStorage({ oauth_id_token: expiredToken, oauth_refresh_token: 'r0' }); + const setupResult = setup({ + storage, + // Mirrors what the teardown's own fire-and-forget authedFetch calls + // do: call getToken() WHILE onSignedOut is still synchronously running. + onAuthLost: () => { + void session.chCtx.getToken().then((t) => { getTokenResult = t; }); + }, + }); + session = setupResult.session; + session.chCtx.onSignedOut(); + await Promise.resolve(); + await Promise.resolve(); + expect(getTokenResult).toBe(expiredToken); // the retained credential, not null + expect(setupResult.fetchMock.calls.some((u) => u.endsWith('/token'))).toBe(false); // no real refresh attempted + }); + + // #502: idempotency — several concurrent requests can each independently + // report the same dead session (including the teardown's own fire-and- + // forget calls rediscovering the same expired-and-unrefreshable token). + it('a second report for the same dead session is a no-op', () => { + const { session, onAuthLost } = setup({ storage: memStorage({ oauth_id_token: validToken }) }); + session.chCtx.onSignedOut('first report'); + session.chCtx.onSignedOut('second report'); + expect(onAuthLost).toHaveBeenCalledTimes(1); + expect(onAuthLost).toHaveBeenCalledWith('first report'); + }); + + it('a fresh sign-in resets the latch, so a later distinct auth loss is handled again', () => { + const { session, onAuthLost } = setup({ storage: memStorage({ oauth_id_token: validToken }) }); + session.chCtx.onSignedOut('first loss'); + session.setTokens(validToken); // fresh sign-in + session.chCtx.onSignedOut('second loss'); + expect(onAuthLost).toHaveBeenCalledTimes(2); + expect(onAuthLost).toHaveBeenNthCalledWith(2, 'second loss'); + }); + + it('still clears every credential even when the injected onAuthLost throws', () => { + const { session, storage } = setup({ + storage: memStorage({ oauth_id_token: validToken, oauth_refresh_token: 'r' }), + onAuthLost: () => { throw new Error('shell blew up'); }, + }); + expect(() => session.chCtx.onSignedOut()).toThrow('shell blew up'); + expect(session.token()).toBeNull(); + expect(storage.getItem('oauth_id_token')).toBeNull(); + expect(storage.getItem('oauth_refresh_token')).toBeNull(); + }); }); // ── ensureFreshToken ───────────────────────────────────────────────────────── diff --git a/tests/unit/results.test.ts b/tests/unit/results.test.ts index ad4cb7b2..31b14e17 100644 --- a/tests/unit/results.test.ts +++ b/tests/unit/results.test.ts @@ -1039,7 +1039,8 @@ describe('expandDataPane', () => { // `exec.executeRead` fixture in this suite uses. const executeRead = vi.fn(async (result: QueryResult, _opts: ExecuteReadOpts = {} as ExecuteReadOpts) => result); const ensureFreshToken = vi.fn(async () => false); - const app = makeApp({ conn: { ensureFreshToken }, exec: { executeRead } }); + const onSignedOut = vi.fn(); + const app = makeApp({ conn: { ensureFreshToken }, chCtx: { onSignedOut }, exec: { executeRead } }); app.state.varValues.level = 'X'; expandDataPane(app, paramResult()); const overlay = qs(document, '.graph-overlay'); @@ -1048,6 +1049,10 @@ describe('expandDataPane', () => { expect(app.exec.executeRead).not.toHaveBeenCalled(); expect(qs(overlay, '.detached-status').textContent).toBe('Not signed in'); expect(refreshBtn(overlay)!.disabled).toBe(false); // re-enabled after the blocked attempt + // #522: unlike before, this failure now reports auth loss too — dead + // credentials must not linger un-torn-down until some other action + // happens to rediscover them. + expect(onSignedOut).toHaveBeenCalled(); }); it('shows a status and keeps the previous result when the rerun errors', async () => {