diff --git a/web/src/lib/collab/wsProvider.svelte.ts b/web/src/lib/collab/wsProvider.svelte.ts index 7165d104b..fb1c1ff6e 100644 --- a/web/src/lib/collab/wsProvider.svelte.ts +++ b/web/src/lib/collab/wsProvider.svelte.ts @@ -128,7 +128,10 @@ export type ForceRefreshHandler = () => void; * reconnect would land on a stale-cursor force_refresh even though * B has been a stable session the whole time. */ -const CURSOR_STORAGE_PREFIX = 'pad:collab:cursor:'; +// Exported so the identity-change clear can find these keys without copying +// the literal (BUG-3005). A second spelling of a storage prefix is a clear that +// silently stops matching. +export const CURSOR_STORAGE_PREFIX = 'pad:collab:cursor:'; function cursorStorageKey(itemID: string): string { return `${CURSOR_STORAGE_PREFIX}${itemID}`; diff --git a/web/src/lib/components/editor/attachment-metadata.ts b/web/src/lib/components/editor/attachment-metadata.ts index ceb0022e7..ed77404b9 100644 --- a/web/src/lib/components/editor/attachment-metadata.ts +++ b/web/src/lib/components/editor/attachment-metadata.ts @@ -92,6 +92,29 @@ export type AttachmentMetadataResult = const cache = new Map>(); +// The key carries no USER (BUG-3005). Every entry is a HEAD result obtained +// under the signed-in user's authorization, and this cache lives for the page +// lifetime by design — so after a same-route account swap, B reading the same +// attachment id in the same workspace is answered from A's probe with no +// request made. +// +// CLEARED ON THE SIGNAL RATHER THAN KEYED BY USER, which was the other option +// and is the stronger one in general. Keying would mean four call sites +// composing the key and two PREFIX SCANS +// (`invalidateAttachmentMetadataForWorkspace`) agreeing with them on a userId +// that can move between the fetch and the invalidation — a new way to write an +// entry nothing can ever delete. The strength a key would add over a clear is +// coverage of requests issued before an identity is established, and nothing +// here can be in that window: the root layout renders no children until +// `authReady`, so no editor exists to probe an attachment before the session +// has resolved. +// +// The subscription is NOT registered here, and that is not tidiness: this +// module is deliberately rune-free and is imported by tests in vitest's `node` +// project, so importing `auth.svelte` breaks them with `$state is not defined` +// before a single assertion runs. The root layout owns the call instead — see +// `clearAttachmentMetadataCache` below. + /** * Is this result a DURABLE fact worth keeping for the page's lifetime? * @@ -236,6 +259,17 @@ export function invalidateAttachmentMetadata(workspaceSlug: string, uuid: string * what answers: a genuinely deleted row 404s again and re-latches. The cache was * never the authority — it was only ever a memo of one. */ +/** + * Drop EVERY entry, for every workspace (BUG-3005). + * + * Called from the root layout on an identity change. Exported rather than + * self-subscribing so this module keeps no dependency on the auth store — see + * the note on `cache` for why that dependency is not free here. + */ +export function clearAttachmentMetadataCache(): void { + cache.clear(); +} + export function invalidateAttachmentMetadataForWorkspace(workspaceSlug: string): void { if (!workspaceSlug) return; const prefix = `${workspaceSlug}:`; diff --git a/web/src/lib/components/items/ItemDetail.svelte b/web/src/lib/components/items/ItemDetail.svelte index bacc06ce2..f3cfdeff2 100644 --- a/web/src/lib/components/items/ItemDetail.svelte +++ b/web/src/lib/components/items/ItemDetail.svelte @@ -223,6 +223,11 @@ // stores after unmount (PLAN-2105 / TASK-2112; Codex rounds 1-2 P1). Plain // counter — not reactive; it only fences async writes. let loadGeneration = 0; + // The identity this component's editor contents belong to (BUG-3005). + // Captured at load rather than at save: the markdown in the editor was + // typed under whoever was signed in when the item was loaded, and that is + // the identity a pending write may legitimately be persisted as. + let identityEpochAtLoad = authStore.identityEpoch; // UNIFIED collection-snapshot generation (BUG-2265 Codex): bumped by EVERY // operation that assigns `collection` — loadData's collection fetch, the SSE @@ -1596,6 +1601,10 @@ async function loadData() { const myGen = ++loadGeneration; + // Re-stamp the identity this editor's contents belong to (BUG-3005). A + // load is where the contents are replaced, so it is where the claim + // "this markdown was typed by the current user" becomes true again. + identityEpochAtLoad = authStore.identityEpoch; // The item whose links `itemLinks` currently describes, captured BEFORE // this load can replace `item`. `loadData` is not only a first load or a // switch: the edit-collection handler calls it for a SAME-item reload @@ -2572,7 +2581,12 @@ // [P1] of TASK-1319. const skipFlush = skipFlushOnNextCleanup; skipFlushOnNextCleanup = false; - if (!rawMode && !skipFlush) { + // The same discard rule as the beforeunload handler and the raw + // saver (BUG-3005): this cleanup also runs on an identity change, + // and a Y.Doc snapshot written then carries the wrong user's + // cookie. + const identityHeld = authStore.identityEpoch === identityEpochAtLoad; + if (!rawMode && !skipFlush && identityHeld) { collabFlusher.flushNow(ctx, true); } provider.destroy(); @@ -2599,6 +2613,25 @@ $effect(() => { if (typeof window === 'undefined') return; const onBeforeUnload = (event: BeforeUnloadEvent) => { + // WHEN THE IDENTITY MOVED, THIS HANDLER DOES NOTHING AT ALL + // (BUG-3005, codex round 4). Two separate defects, both created by + // the fix that reloads the tab on an identity change: + // + // - it FLUSHES. Both paths below persist editor content with + // `keepalive`, and the unload they run in is the identity + // reload's own — so A's Y.Doc snapshot and A's pending markdown + // would be PATCHed carrying B's cookie. The raw saver has its + // own guard; the collab flush did not, and my grep for + // `keepalive: true` missed it because it takes the flag + // positionally. Guarding here covers both by construction. + // - it PROMPTS. `preventDefault()` on a dirty editor raises the + // native "unsaved changes" dialog, and "Stay" CANCELS the + // reload — leaving B in A's page shell with the stores already + // cleared and no remount left to rebuild it. The prompt exists + // to protect the author's unsaved work, and after an identity + // change the author is not the one sitting here. + if (authStore.identityEpoch !== identityEpochAtLoad) return; + // Collab path: flush the live Y.Doc snapshot (unchanged). const ctx = activeCollabContext; if (ctx) collabFlusher.flushNow(ctx, true); @@ -3637,6 +3670,24 @@ debounceMs: 1200, save: (markdown, { keepalive }) => { if (!item) return; + // DISCARD RATHER THAN PERSIST WHEN THE IDENTITY MOVED (BUG-3005). + // + // This markdown was typed by whoever was signed in when they typed + // it. If a different user is signed in now, writing it means one + // account's unsaved edit is persisted as another's — a WRITE under + // the wrong identity, which is a different class from the display + // leaks the rest of that fix is about. + // + // The keepalive path is why this is load-bearing rather than + // theoretical: an identity change RELOADS this tab, the reload fires + // `beforeunload`, and that handler flushes pending raw markdown with + // `keepalive: true` — a request deliberately built to outlive the + // page, and it would go out carrying the NEW user's cookie. The fix + // that reloads is what makes this reachable, so it ships with it. + // + // Compared against the epoch captured when this item was LOADED, + // which is the identity the editor's contents belong to. + if (authStore.identityEpoch !== identityEpochAtLoad) return; // HT-2176 Option A (TASK-2172): NO peeking recheck here. NEW raw input // is blocked at the editor (`readonly={!canEdit || peeking}`), so this // only ever fires for a debounced save the user INITIATED before the diff --git a/web/src/lib/components/items/itemDetailIdentityDiscardsTeardownWrites.test.ts b/web/src/lib/components/items/itemDetailIdentityDiscardsTeardownWrites.test.ts new file mode 100644 index 000000000..68a6ee917 --- /dev/null +++ b/web/src/lib/components/items/itemDetailIdentityDiscardsTeardownWrites.test.ts @@ -0,0 +1,81 @@ +// Node-project test (no DOM): a SOURCE-level guard that ItemDetail's teardown +// writes DISCARD when the signed-in identity has moved (BUG-3005, codex round +// 4). +// +// WHY THIS IS LOAD-BEARING RATHER THAN BELT-AND-BRACES. A real identity change +// now RELOADS the tab, and a reload fires `beforeunload`. Both persistence +// paths in that handler use `keepalive`, a request deliberately built to +// outlive the page — so without a guard, the previous user's Y.Doc snapshot and +// pending markdown are PATCHed carrying the NEW user's cookie. The fix that +// reloads is what makes this reachable; it ships with it. +// +// The prompt half matters as much as the flush half: `preventDefault()` on a +// dirty editor raises the native "unsaved changes" dialog, and "Stay" CANCELS +// the reload — leaving the new user in the previous user's page shell with the +// stores already cleared and (since round 3) no remount left to rebuild it. +// +// WHY SOURCE TEXT AND NOT A RENDER: same reasoning as +// `itemDetailLinksSurviveFailedRefresh.test.ts` in this directory — ItemDetail +// is ~7,900 lines of collab, SSE and pane wiring, and the property is +// structural. WHAT A SOURCE GUARD CANNOT DO, stated so it is not mistaken for +// proof: it checks spellings, not behaviour. It would not catch a guard that +// compares the wrong two values, or one made unreachable by an earlier return. +import { describe, expect, it } from 'vitest'; +import { readFileSync } from 'node:fs'; +import { resolve } from 'node:path'; + +const SRC = readFileSync(resolve(__dirname, './ItemDetail.svelte'), 'utf8'); + +/** Strip line and block comments so a guard cannot be satisfied by prose. */ +function stripComments(src: string): string { + return src.replace(/\/\*[\s\S]*?\*\//g, '').replace(/^[ \t]*\/\/.*$/gm, ''); +} + +const CODE = stripComments(SRC); + +describe('ItemDetail teardown writes under an identity change', () => { + it('captures the identity epoch at LOAD and re-stamps it in loadData', () => { + // The comparison is only meaningful against the identity whose typing + // the editor holds, which is the one current when the item was loaded — + // not the one current when the save fires. + expect(CODE).toMatch(/let\s+identityEpochAtLoad\s*=\s*authStore\.identityEpoch/); + const loadData = CODE.slice(CODE.indexOf('async function loadData()')); + const upToFirstAwait = loadData.slice(0, loadData.indexOf('await')); + expect(upToFirstAwait).toContain('identityEpochAtLoad = authStore.identityEpoch'); + }); + + it('makes the identity check the FIRST thing beforeunload does', () => { + // Ahead of both flushes AND ahead of `preventDefault`, or the handler + // still writes or still blocks the reload. + const start = CODE.indexOf('const onBeforeUnload = (event: BeforeUnloadEvent) => {'); + expect(start, 'the beforeunload handler was renamed or removed').toBeGreaterThan(-1); + const body = CODE.slice(start, start + 1200); + const guard = body.indexOf('authStore.identityEpoch !== identityEpochAtLoad'); + expect(guard, 'beforeunload does not check the identity epoch').toBeGreaterThan(-1); + + for (const persisting of ['collabFlusher.flushNow', 'rawContentSaver.flushNow', 'preventDefault']) { + const at = body.indexOf(persisting); + expect(at, `${persisting} is no longer in the handler — re-point this guard`).toBeGreaterThan(-1); + expect(at, `${persisting} runs BEFORE the identity check`).toBeGreaterThan(guard); + } + }); + + it('gates the collab cleanup flush on the identity too', () => { + // The $effect cleanup is a second door into the same write, and it runs + // on the same identity change. My own grep for `keepalive: true` missed + // it, because this one passes the flag positionally. + expect(CODE).toMatch( + /const\s+identityHeld\s*=\s*authStore\.identityEpoch\s*===\s*identityEpochAtLoad/, + ); + expect(CODE).toMatch(/if\s*\(!rawMode\s*&&\s*!skipFlush\s*&&\s*identityHeld\)/); + }); + + it('gates the raw saver, which is the third door', () => { + const saver = CODE.slice(CODE.indexOf('const rawContentSaver = createContentSaver(')); + const save = saver.slice(saver.indexOf('save: (markdown')); + const guard = save.indexOf('authStore.identityEpoch !== identityEpochAtLoad'); + const patch = save.indexOf('api.items'); + expect(guard).toBeGreaterThan(-1); + expect(patch).toBeGreaterThan(guard); + }); +}); diff --git a/web/src/lib/components/timeline/ItemTimeline.svelte.test.ts b/web/src/lib/components/timeline/ItemTimeline.svelte.test.ts index 24c0f40d2..70fdf2e24 100644 --- a/web/src/lib/components/timeline/ItemTimeline.svelte.test.ts +++ b/web/src/lib/components/timeline/ItemTimeline.svelte.test.ts @@ -75,7 +75,17 @@ vi.mock('$lib/services/sse.svelte', () => ({ })); vi.mock('$lib/stores/auth.svelte', () => ({ - authStore: { userId: 'user-1', user: { id: 'user-1', role: 'member' } }, + // `onIdentityChange` and `identityFence` are part of the store's surface as + // of BUG-3005: several singletons subscribe at MODULE scope, so a partial + // mock throws while the component tree is still being imported, before any + // assertion here runs. + authStore: { + userId: 'user-1', + user: { id: 'user-1', role: 'member' }, + identityEpoch: 0, + identityFence: () => () => true, + onIdentityChange: () => () => {}, + }, })); // Controllable so the TASK-2474 peek leg can set the timeline's OWN canEdit diff --git a/web/src/lib/stores/admin.svelte.ts b/web/src/lib/stores/admin.svelte.ts index 2c76b99c6..40a50fe51 100644 --- a/web/src/lib/stores/admin.svelte.ts +++ b/web/src/lib/stores/admin.svelte.ts @@ -2,6 +2,8 @@ // Admin store – shared state & utilities for the admin section // --------------------------------------------------------------------------- +import { authStore } from './auth.svelte'; + // ---- Interfaces ----------------------------------------------------------- export interface AdminUser { @@ -115,12 +117,19 @@ let error = $state(''); async function loadStats() { loading = true; error = ''; + // The identity that ASKED (BUG-3005, codex round 2). Clearing on the signal + // is not enough on its own: a request already in flight settles afterwards + // and writes the previous admin's statistics into the new session. + const isSameIdentity = authStore.identityFence(); try { - stats = await adminFetch('/admin/stats'); + const result = await adminFetch('/admin/stats'); + if (!isSameIdentity()) return; + stats = result; } catch (e) { + if (!isSameIdentity()) return; error = e instanceof Error ? e.message : 'Failed to load'; } finally { - loading = false; + if (isSameIdentity()) loading = false; } } @@ -134,5 +143,29 @@ export const adminStore = { get error() { return error; }, - loadStats + loadStats, + + /** + * Drop the loaded statistics (BUG-3005). `stats` is instance-wide DATA but + * it is authorization-scoped: only an admin can read it, so it must not + * outlive the admin who did. + */ + clear() { + stats = null; + // FALSE, not true (codex round 2). The admin layout loads on MOUNT, and + // a same-route admin-to-admin swap mounts nothing — leaving this true + // would pin the console on "Loading admin data…" forever. The layout + // re-issues the load on the same signal; until it lands the honest + // state is "nothing loaded", not "loading". + loading = false; + error = ''; + } }; + +// An admin signing out — or a swap to a non-admin on the same console route — +// must not leave the previous admin's statistics on screen. The admin layout +// loads on mount, and a same-route identity change mounts nothing (BUG-3005, +// codex round 1). +authStore.onIdentityChange(() => { + adminStore.clear(); +}); diff --git a/web/src/lib/stores/auth.svelte.ts b/web/src/lib/stores/auth.svelte.ts index edd32c86d..0f2a85616 100644 --- a/web/src/lib/stores/auth.svelte.ts +++ b/web/src/lib/stores/auth.svelte.ts @@ -31,7 +31,7 @@ let generation = 0; // // The direction of the dependency is unchanged: `workspace.svelte.ts` imports // this module and registers here, so this module still imports nothing of it. -const identityListeners = new Set<() => void>(); +const identityListeners = new Set<(previousUserId: string) => void>(); // The id whose listeners have already been notified. Compared rather than // assumed, so a `load()` that returns the SAME user (the common case — the root // layout fetches the session on every cold start) notifies nobody and cannot @@ -46,6 +46,73 @@ let notifiedUserId = ''; // baseline. Every later transition — including back to '' on sign-out — fires. let identityEstablished = false; +// The identity EPOCH (BUG-3005): a monotonic counter bumped on exactly the +// transitions `onIdentityChange` fires on. +// +// WHY A COUNTER AND NOT THE USER ID. A store that starts a request as one user +// and commits its response later needs to ask "is this still the identity I +// issued under?", and a user id answers a WEAKER question — "is this a +// different user?". The two come apart on sign out of A, in as B, back in as +// A: a request issued during the FIRST A session settles, compares its +// captured id against the current one, finds them equal, and commits data +// belonging to a session that has ended. An ordinal cannot be confused that +// way, because the epoch it captured is strictly lower than the one it settles +// into. `workspace.svelte.ts`'s `userId` fence has exactly that hole; it is +// narrower than the primary defect (it needs a request to outlive two identity +// changes) and it is the same class. +// +// NOT BUMPED ON THE BASELINE, matching `notifyIdentityChange`'s own early +// return: the first resolution of the session is not a change from anything, +// and a fence captured before an identity was established has nothing stale to +// refuse. +// +// THE RESIDUAL THAT LEAVES, stated rather than discovered (codex round 1). +// A request issued before the baseline resolves carries whatever cookie the +// browser had; the baseline then reports whoever that cookie belongs to. Those +// are the same principal unless the cookie CHANGED mid-flight — another tab +// signing out and in — in which case the pre-baseline request settles under a +// fence that still says current, and commits. +// +// CORRECTED (codex round 2). This comment previously justified the exemption by +// saying the root layout issues the workspace list concurrently with +// /auth/session, so refusing pre-baseline settles would empty the app. That is +// FALSE, and I had taken it from the `identityEstablished` comment below rather +// than reading the layout: `workspaceStore.loadAll()` is gated on `authReady` +// (routes/+layout.svelte), which flips only after `authStore.load()` resolves, +// and the layout renders NO children until then — so no route can issue a +// fenced request before the baseline either. +// +// What that means for the residual: on the ORDINARY path it needs a fence +// captured by something that runs before any child renders, and the only +// pre-baseline request there is `authStore.load()` itself, which takes no +// fence. TWO EXCEPTIONS, both real (codex round 3, correcting this comment a +// second time): the share-page branch sets `authReady` WITHOUT calling +// `authStore.load()` at all, and the auth-FAILURE path proceeds deliberately — +// so children can render and fetch with no baseline established in either. The +// residual is therefore narrow rather than unreachable, and it is a property of +// the callers rather than of this code. +// +// The exemption itself stays for a different and simpler reason: the bump and +// the listener notification are the same event, and firing listeners on the +// baseline is a live hazard the `identityEstablished` comment below documents. +// Separating them would mean two signals where one is honest. +// +// The honest boundary is therefore: the epoch covers identity changes THIS TAB +// observed, and a cross-tab change during the pre-baseline window is outside +// it. +// +// Distinct from `generation` above, which bumps ONLY on `clear()` and fences +// this module's own `/auth/session` fetches. A sign-in as a different user +// through `load()` moves the identity without touching `generation`, so +// `generation` cannot serve as the epoch — it would miss the swap. +// `$state`, unlike `generation` and `notifiedUserId` beside it, because this +// one is READ FROM A TEMPLATE: the workspace layout keys its leaf-page block on +// it (`{#key authStore.identityEpoch}`) to remount route components whose own +// state is not identity-scoped. A plain `let` is invisible to the template and +// the block never re-runs — which is exactly how the first version of that +// remount shipped, silently, until a test counted mounts (codex round 2). +let identityEpoch = $state(0); + function notifyIdentityChange() { const id = session?.user?.id ?? ''; if (!identityEstablished) { @@ -54,8 +121,34 @@ function notifyIdentityChange() { return; } if (id === notifiedUserId) return; + const previousUserId = notifiedUserId; notifiedUserId = id; - for (const fn of identityListeners) fn(); + // BEFORE the listeners, not after. A listener may capture a fence while it + // runs — dropping state and immediately reloading is the expected shape — + // and a fence captured under the OLD epoch would refuse that reload's own + // response. + // + // THE CONTRACT THIS PLACES ON LISTENERS (codex round 1): a fence captured + // inside a listener passes, so a listener must reload for the identity that + // is signed in NOW and never re-issue a request built from the identity it + // was just told about. The fence cannot check this for them — it compares + // epochs, and by the time a listener runs the epoch is already the new one. + // Every listener on this branch satisfies it, and most satisfy it by doing + // nothing further: clearing state and stopping cannot re-issue anything. + // The two that DO reload — the workspace layout and the starred page — read + // the workspace from the live route rather than from anything captured + // before the change (codex round 2 read "stops" as a claim that they all + // reload, so this says which is which). + identityEpoch++; + // The PREVIOUS established identity is passed to listeners, because one of + // them has to distinguish a sign-IN from a sign-out or a swap (BUG-3005, + // lead ruling after the E2E failure). The reload exists to stop one user's + // data being visible to the next, and an ANONYMOUS baseline holds nobody's + // private data — so `'' -> user` needs no reload, while `user -> user` and + // `user -> ''` do. Reloading on sign-in also put a full page load in the + // middle of the primary login path, racing whatever navigation that flow + // was doing. + for (const fn of identityListeners) fn(previousUserId); } export const authStore = { @@ -155,12 +248,49 @@ export const authStore = { * Fired only on a real change of user id, so a session refetch that returns * the same user is silent. See `identityListeners` above for why this exists * rather than a call at each sign-out site. + * + * The listener receives the PREVIOUS established user id — `''` when the tab + * was unauthenticated. Most listeners ignore it and simply drop state; the + * root layout uses it to reload on a sign-out or a swap but NOT on a + * sign-in. */ - onIdentityChange(fn: () => void): () => void { + onIdentityChange(fn: (previousUserId: string) => void): () => void { identityListeners.add(fn); return () => identityListeners.delete(fn); }, + /** + * The current identity epoch — a monotonic counter of identity CHANGES. + * Read it to compare two moments; use `identityFence()` to guard a settle. + */ + get identityEpoch() { return identityEpoch; }, + + /** + * Capture the current identity, and return a predicate that says whether it + * is still the current one. + * + * The intended shape, for any store that reads user-scoped data: + * + * const isSameIdentity = authStore.identityFence(); + * const result = await api.something(); + * if (!isSameIdentity()) return; // issued as someone else; drop it + * + * Offered as a helper rather than left to each call site because this store + * and `workspace.svelte.ts` already hand-rolled the same three parts of a + * single-flight loader and drifted three times in one afternoon + * (`singleFlight.ts`'s own note). A fence is smaller and there are more + * places that need one. + * + * It is NOT a navigation fence and does not replace one. A workspace switch + * under one identity leaves the epoch alone, which is correct — that race is + * about WHICH WORKSPACE the answer describes, not about who asked. Stores + * that can race a switch need both, and neither subsumes the other. + */ + identityFence(): () => boolean { + const captured = identityEpoch; + return () => identityEpoch === captured; + }, + clear() { session = null; generation++; diff --git a/web/src/lib/stores/authIdentityEpoch.svelte.test.ts b/web/src/lib/stores/authIdentityEpoch.svelte.test.ts new file mode 100644 index 000000000..534157dcf --- /dev/null +++ b/web/src/lib/stores/authIdentityEpoch.svelte.test.ts @@ -0,0 +1,117 @@ +import { describe, expect, it, vi, beforeEach } from 'vitest'; + +/** + * BUG-3005 — `authStore.identityEpoch` / `identityFence()`. + * + * The epoch exists so a store that issues a request as one user can refuse the + * response if it settles as another. Its whole claim over the user-id + * comparison already in the tree is that it is ORDERED: A → B → A must not + * look like "no change". + */ + +const session = vi.hoisted(() => ({ value: null as unknown })); + +vi.mock('$lib/api/client', () => ({ + api: { auth: { session: vi.fn(async () => session.value) } }, +})); + +function user(id: string) { + return { authenticated: true, user: { id, email: `${id}@example.com` } }; +} + +describe('authStore identity epoch', () => { + beforeEach(() => { + vi.resetModules(); + session.value = null; + }); + + it('does not move on the FIRST resolution of the session', async () => { + // Same reasoning as onIdentityChange's silence on the baseline: a cold + // start has nothing stale, and a fence captured before an identity was + // established must not start refusing once one appears. + const { authStore } = await import('./auth.svelte'); + const isSameIdentity = authStore.identityFence(); + + session.value = user('u1'); + await authStore.load(); + + expect(authStore.identityEpoch).toBe(0); + expect(isSameIdentity()).toBe(true); + }); + + it('moves on sign-out, and a fence taken before it refuses', async () => { + const { authStore } = await import('./auth.svelte'); + session.value = user('u1'); + await authStore.load(); + + const isSameIdentity = authStore.identityFence(); + // PRECONDITION: current before the change, or "it refused" is a claim + // about a fence that never accepted anything. + expect(isSameIdentity()).toBe(true); + + authStore.clear(); + + expect(authStore.identityEpoch).toBe(1); + expect(isSameIdentity()).toBe(false); + }); + + it('REFUSES a straggler from the first A session after A -> B -> A', async () => { + // The reason this is an ordinal and not the user id. Every id comparison + // in this scenario reports "same user", because it IS the same user — + // and the session that issued the request is gone. + const { authStore } = await import('./auth.svelte'); + session.value = user('a'); + await authStore.load(); + + // A request issued in A's first session captures its fence here. + const issuedInFirstA = authStore.identityFence(); + const userIdAtIssue = authStore.userId; + + authStore.clear(); + session.value = user('b'); + await authStore.load(); + authStore.clear(); + session.value = user('a'); + await authStore.load(); + + // The comparison the tree already had says CURRENT — this is the hole. + expect(authStore.userId).toBe(userIdAtIssue); + // The epoch says otherwise, which is the fix. + expect(issuedInFirstA()).toBe(false); + expect(authStore.identityEpoch).toBe(4); + }); + + it('stays put when a session refetch returns the SAME user', async () => { + // The counterfactual for every test above: a fence that refuses on a + // no-op refetch would drop live data on the root layout's ordinary + // session poll, which is a worse defect than the one being fixed. + const { authStore } = await import('./auth.svelte'); + session.value = user('u1'); + await authStore.load(); + + const isSameIdentity = authStore.identityFence(); + const before = authStore.identityEpoch; + await authStore.load(); + + expect(authStore.identityEpoch).toBe(before); + expect(isSameIdentity()).toBe(true); + }); + + it('is already bumped when identity listeners run', async () => { + // A listener's expected shape is "drop state, reload it". The reload + // captures a fence WHILE the listener runs, so a bump ordered after the + // listeners would hand it the old epoch and refuse its own response. + const { authStore } = await import('./auth.svelte'); + session.value = user('u1'); + await authStore.load(); + + let fenceTakenInsideListener: (() => boolean) | null = null; + authStore.onIdentityChange(() => { + fenceTakenInsideListener = authStore.identityFence(); + }); + authStore.clear(); + + expect(fenceTakenInsideListener).not.toBeNull(); + expect(fenceTakenInsideListener!()).toBe(true); + }); +}); diff --git a/web/src/lib/stores/collections.svelte.ts b/web/src/lib/stores/collections.svelte.ts index 47017397c..2bf8642ca 100644 --- a/web/src/lib/stores/collections.svelte.ts +++ b/web/src/lib/stores/collections.svelte.ts @@ -158,6 +158,13 @@ export const collectionStore = { // authenticated one. Same source as every `bootstrap` caller passes, so // the two cannot disagree. const userId = authStore.userId || null; + // The IDENTITY this load is issued under (BUG-3005). Captured out here, + // before `run` and therefore before any await, so it names the identity + // that ASKED rather than whichever one is signed in when the response + // lands. `isLatest` is a navigation fence and does not cover this: a + // same-route account swap supersedes nothing, so the older load is still + // "latest" and commits A's collection names into B's session. + const isSameIdentity = authStore.identityFence(); // ALWAYS ISSUES — `run` never coalesces. The join is `ensureCollections`'s // alone, via `inFlightFor`, because every other caller here knows the list // MOVED and a request issued before that move cannot answer it. @@ -179,7 +186,7 @@ export const collectionStore = { // switch that resolved first) has superseded this one, so writing // `collections`/`collectionsWorkspace` here would clobber the newer // workspace's data with this older load (Codex review). - if (!isLatest()) return; + if (!isLatest() || !isSameIdentity()) return; collections = result; // Stamp the array with its source workspace so consumers can tell // it apart from a stale previous-workspace load (see @@ -196,26 +203,47 @@ export const collectionStore = { }, async loadItems(ws: string, collectionSlug?: string, params?: Record) { + const isSameIdentity = authStore.identityFence(); loading = true; try { if (collectionSlug) { - items = await api.items.listByCollection(ws, collectionSlug, params); + const result = await api.items.listByCollection(ws, collectionSlug, params); + // A response issued as someone else must not become this + // session's item list (BUG-3005). Assigned through a local + // first, because `items = await ...` publishes whatever comes + // back before any check can run. + if (!isSameIdentity()) return; + items = result; // Partial load — the stored array no longer represents the // full workspace, so invalidate the freshness stamp. A // downstream caller asking `itemsAreFreshFor(ws)` will // correctly trigger a full re-load. itemsWorkspace = null; } else { - items = await api.items.list(ws, params); + const result = await api.items.list(ws, params); + if (!isSameIdentity()) return; + items = result; itemsWorkspace = ws; } } finally { - loading = false; + // FENCED TOO (codex round 1, P2). `loading` is module state written + // after an await like any other: A's load settling while B's is + // still in flight would clear B's flag and flash empty-state or + // error UI mid-load. `clear()` resets the flag on the identity + // change itself, so nothing is left stuck true. + if (isSameIdentity()) loading = false; } }, async loadItem(ws: string, slug: string) { - activeItem = await api.items.get(ws, slug); + const isSameIdentity = authStore.identityFence(); + const result = await api.items.get(ws, slug); + // Returns null rather than the fetched item when the identity moved: + // the caller asked on behalf of a session that has ended, and handing + // it the item would put it on screen for whoever is signed in now + // (BUG-3005). `activeItem` is left as the reset found it. + if (!isSameIdentity()) return null; + activeItem = result; return activeItem; }, @@ -242,4 +270,49 @@ export const collectionStore = { activeItem = null; } }, + + /** + * Drop everything this store holds. Exported for the identity subscription + * below and for tests; there is no other caller. + */ + clear() { + collections = []; + items = []; + itemsWorkspace = null; + collectionsWorkspace = null; + activeItem = null; + // The flag any in-flight load will now decline to clear (see the fenced + // `finally` in `loadItems`). Reset here so the identity change itself + // owns it and no consumer is left waiting on a load that will never + // report finished. + loading = false; + // And the SINGLE-FLIGHT SLOT (codex round 2). Dropping the data while + // leaving A's promise published made this reset actively harmful: B's + // `ensureCollections` joined A's request, A's identity fence then + // refused to commit, and B was answered with no data and no request of + // its own — a permanently empty sidebar, which is the exact failure the + // cleared freshness stamp was supposed to prevent. + collectionsFlight.invalidate(); + }, }; + +// Drop the previous user's collections, items and open item when the signed-in +// user changes (BUG-3005). +// +// Every piece of state here belongs to a user and none of it is keyed by one: +// the loader is keyed by workspace SLUG, and the workspace layout drives it +// from a slug-keyed effect, so a same-route sign-in as a different user starts +// no new load at all. Routes that read `collections` — Roles, the item picker, +// the editor's link gate — then render A's collection names and item titles +// inside B's session. +// +// The FRESHNESS STAMPS are cleared with the arrays, deliberately. Leaving +// `collectionsWorkspace` set while emptying `collections` would make +// `collectionsAreFreshFor(ws)` answer true for an empty list, which is the one +// answer that stops TASK-2200's recovery from re-fetching. +// +// Module scope, registered once, never unsubscribed: singleton store, tab +// lifetime. +authStore.onIdentityChange(() => { + collectionStore.clear(); +}); diff --git a/web/src/lib/stores/collectionsIdentityReset.svelte.test.ts b/web/src/lib/stores/collectionsIdentityReset.svelte.test.ts new file mode 100644 index 000000000..eeec3e286 --- /dev/null +++ b/web/src/lib/stores/collectionsIdentityReset.svelte.test.ts @@ -0,0 +1,191 @@ +import { describe, expect, it, vi, beforeEach } from 'vitest'; + +/** + * BUG-3005 — `collectionStore` retains `collections`, `items` and `activeItem` + * globally, and its loader is keyed only by workspace SLUG. + * + * The workspace layout drives it from a slug-keyed effect, so a same-route + * sign-in as a different user starts no new load at all: A's collection names + * and item titles stay on screen inside B's session, with no request having + * been made for them. And a load issued as A can commit after B signs in, + * because the store's only fence is a NAVIGATION fence that a same-route swap + * does not move. + */ + +const api = vi.hoisted(() => ({ + collections: { list: vi.fn() }, + items: { list: vi.fn(), listByCollection: vi.fn(), get: vi.fn() }, +})); + +vi.mock('$lib/api/client', () => ({ api })); + +// The durable cache is a different concern (TASK-2946); stub it inert so this +// file tests the identity fence and nothing else. +vi.mock('./localIndexPersistence', () => ({ + hydrateCollections: vi.fn(async () => null), + persistCollections: vi.fn(async () => {}), + readDurableEpoch: vi.fn(async () => null), +})); + +// A REAL epoch and fence. A stub that always answered "still current" would let +// every assertion below pass against a store with no fence at all. +const auth = vi.hoisted(() => { + const listeners = new Set<() => void>(); + let epoch = 0; + return { + userId: 'user-1', + get identityEpoch() { return epoch; }, + identityFence() { + const captured = epoch; + return () => epoch === captured; + }, + onIdentityChange(fn: () => void) { + listeners.add(fn); + return () => listeners.delete(fn); + }, + fireIdentityChange() { + epoch++; + for (const fn of listeners) fn(); + }, + resetListeners() { + listeners.clear(); + epoch = 0; + }, + }; +}); + +vi.mock('./auth.svelte', () => ({ authStore: auth })); + +const collection = (slug: string) => ({ id: slug, slug, name: slug, is_default: true, sort_order: 1 }); +const item = (id: string) => ({ id, slug: id, title: id }); + +describe('collectionStore across an identity change', () => { + beforeEach(() => { + vi.resetModules(); + auth.resetListeners(); + api.collections.list.mockReset(); + api.items.list.mockReset(); + api.items.listByCollection.mockReset(); + api.items.get.mockReset(); + }); + + it('drops collections, items and the open item when the identity changes', async () => { + const { collectionStore } = await import('./collections.svelte'); + api.collections.list.mockResolvedValueOnce([collection('tasks')]); + await collectionStore.loadCollections('ws'); + api.items.list.mockResolvedValueOnce([item('item-a')]); + await collectionStore.loadItems('ws'); + api.items.get.mockResolvedValueOnce(item('item-a')); + await collectionStore.loadItem('ws', 'item-a'); + + // PRECONDITION: all three are populated, or "they were dropped" is a + // claim about state that was never there. + expect(collectionStore.collections).toHaveLength(1); + expect(collectionStore.items).toHaveLength(1); + expect(collectionStore.activeItem).not.toBeNull(); + expect(collectionStore.collectionsAreFreshFor('ws')).toBe(true); + + auth.fireIdentityChange(); + + expect(collectionStore.collections).toHaveLength(0); + expect(collectionStore.items).toHaveLength(0); + expect(collectionStore.activeItem).toBeNull(); + }); + + it('clears the FRESHNESS STAMPS with the arrays, so recovery re-fetches', async () => { + // Emptying `collections` while leaving `collectionsWorkspace` set would + // make `collectionsAreFreshFor(ws)` answer true for an empty list — the + // one answer that stops TASK-2200's recovery from ever re-fetching, so + // the sidebar would stay permanently empty for the new user. + const { collectionStore } = await import('./collections.svelte'); + api.collections.list.mockResolvedValueOnce([collection('tasks')]); + await collectionStore.loadCollections('ws'); + api.items.list.mockResolvedValueOnce([item('item-a')]); + await collectionStore.loadItems('ws'); + expect(collectionStore.itemsAreFreshFor('ws')).toBe(true); + + auth.fireIdentityChange(); + + expect(collectionStore.collectionsAreFreshFor('ws')).toBe(false); + expect(collectionStore.itemsAreFreshFor('ws')).toBe(false); + }); + + it('refuses a collections load that RESOLVES after the identity changed', async () => { + // The navigation fence cannot catch this: the workspace never changed, + // so this load is still the latest one for `ws`. + const { collectionStore } = await import('./collections.svelte'); + let resolve!: (v: unknown) => void; + api.collections.list.mockReturnValueOnce(new Promise((r) => { resolve = r; })); + + const inflight = collectionStore.loadCollections('ws'); + auth.fireIdentityChange(); + resolve([collection('tasks')]); + await inflight; + + expect(collectionStore.collections).toHaveLength(0); + expect(collectionStore.collectionsAreFreshFor('ws')).toBe(false); + }); + + it('refuses an items load that resolves after the identity changed', async () => { + const { collectionStore } = await import('./collections.svelte'); + let resolve!: (v: unknown) => void; + api.items.list.mockReturnValueOnce(new Promise((r) => { resolve = r; })); + + const inflight = collectionStore.loadItems('ws'); + auth.fireIdentityChange(); + resolve([item('item-a')]); + await inflight; + + expect(collectionStore.items).toHaveLength(0); + expect(collectionStore.itemsAreFreshFor('ws')).toBe(false); + }); + + it('refuses a COLLECTION-FILTERED items load that resolves after the change', async () => { + // The other arm of the same method. It assigns through a different + // branch and stamps `itemsWorkspace` differently, so fencing one arm + // leaves the other publishing another user's items. + const { collectionStore } = await import('./collections.svelte'); + let resolve!: (v: unknown) => void; + api.items.listByCollection.mockReturnValueOnce(new Promise((r) => { resolve = r; })); + + const inflight = collectionStore.loadItems('ws', 'tasks'); + auth.fireIdentityChange(); + resolve([item('item-a')]); + await inflight; + + expect(collectionStore.items).toHaveLength(0); + }); + + it('returns null from loadItem when the identity moved, and does not publish it', async () => { + // The return value matters as much as the store write: the caller renders + // what it is handed, so handing back A's item puts it on B's screen even + // with `activeItem` untouched. + const { collectionStore } = await import('./collections.svelte'); + let resolve!: (v: unknown) => void; + api.items.get.mockReturnValueOnce(new Promise((r) => { resolve = r; })); + + const inflight = collectionStore.loadItem('ws', 'item-a'); + auth.fireIdentityChange(); + resolve(item('item-a')); + + expect(await inflight).toBeNull(); + expect(collectionStore.activeItem).toBeNull(); + }); + + it('still commits an ordinary load when the identity holds still', async () => { + // The counterfactual. A fence that refused everything would leave every + // user with an empty sidebar, which is worse than the leak. + const { collectionStore } = await import('./collections.svelte'); + api.collections.list.mockResolvedValueOnce([collection('tasks')]); + await collectionStore.loadCollections('ws'); + api.items.list.mockResolvedValueOnce([item('item-a')]); + await collectionStore.loadItems('ws'); + api.items.get.mockResolvedValueOnce(item('item-a')); + const loaded = await collectionStore.loadItem('ws', 'item-a'); + + expect(collectionStore.collections).toHaveLength(1); + expect(collectionStore.items).toHaveLength(1); + expect(loaded).not.toBeNull(); + expect(collectionStore.collectionsAreFreshFor('ws')).toBe(true); + }); +}); diff --git a/web/src/lib/stores/editor.svelte.ts b/web/src/lib/stores/editor.svelte.ts index ae72bdc0f..746d8d41f 100644 --- a/web/src/lib/stores/editor.svelte.ts +++ b/web/src/lib/stores/editor.svelte.ts @@ -1,3 +1,5 @@ +import { authStore } from './auth.svelte'; + // Module-singleton editor scalars — ONE `dirty`/`lastSaveTime`/`saveStatus` // for the whole page, regardless of how many instances are // mounted. That's fine today (only one is ever live), but PLAN-2154 (the @@ -38,4 +40,25 @@ export const editorStore = { enterRaw() { mode = 'raw'; }, enterEdit() { mode = 'edit'; }, resetForDoc() { mode = 'edit'; dirty = false; externalChange = false; }, + + /** + * Drop every editor scalar (BUG-3005). Wider than `resetForDoc`, which + * deliberately keeps `saveStatus` and `lastSaveTime` because it is a + * DOCUMENT change within one session. An identity change ends the session. + */ + clear() { + mode = 'edit'; + saveStatus = 'idle'; + dirty = false; + lastSaveTime = 0; + externalChange = false; + }, }; + +// These scalars survive an SPA identity swap, and they are not merely stale — +// they are consumed by GUARDS. `dirty` drives unsaved-draft prompts and SSE +// archive/delete suppression, so A's dirty editor makes B's navigation and save +// paths act on a document B never opened (BUG-3005, codex round 1). +authStore.onIdentityChange(() => { + editorStore.clear(); +}); diff --git a/web/src/lib/stores/identityReload.svelte.test.ts b/web/src/lib/stores/identityReload.svelte.test.ts new file mode 100644 index 000000000..880cb9e92 --- /dev/null +++ b/web/src/lib/stores/identityReload.svelte.test.ts @@ -0,0 +1,132 @@ +import { describe, expect, it, vi, beforeEach, afterEach } from 'vitest'; +import { readFileSync } from 'node:fs'; +import { resolve } from 'node:path'; + +/** + * BUG-3005 — what a real identity change does to the tab. + * + * The fix stopped being per-surface after three review rounds each found + * another layer, so the mechanism is now: clear the state a reload does NOT + * drop, then reload. These tests pin both halves, and the storage keys, because + * a clear that stops matching its key is silent. + */ + +vi.mock('$app/environment', () => ({ browser: true })); +vi.mock('$lib/components/editor/attachment-metadata', () => ({ + clearAttachmentMetadataCache: vi.fn(), +})); + +import { + clearPersistentIdentityState, + reloadForIdentityChange, + RECENT_SEARCHES_KEY, + LAST_ROUTE_PREFIX, + LAST_SCROLL_PREFIX, +} from './identityReload.svelte'; +import { CURSOR_STORAGE_PREFIX } from '$lib/collab/wsProvider.svelte'; +import { clearAttachmentMetadataCache } from '$lib/components/editor/attachment-metadata'; + +describe('clearPersistentIdentityState', () => { + beforeEach(() => { + localStorage.clear(); + sessionStorage.clear(); + vi.mocked(clearAttachmentMetadataCache).mockClear(); + }); + + it('drops the palette\'s recent searches', async () => { + // Search terms carry private item names and project context, and the key + // has no user in it. + localStorage.setItem(RECENT_SEARCHES_KEY, JSON.stringify(['alphas secret project'])); + // PRECONDITION: it is really there. + expect(localStorage.getItem(RECENT_SEARCHES_KEY)).not.toBeNull(); + + clearPersistentIdentityState(); + + expect(localStorage.getItem(RECENT_SEARCHES_KEY)).toBeNull(); + }); + + it('drops every collab cursor, and leaves unrelated session keys alone', async () => { + // Cursors are keyed by item id ALONE, so B opening an item A edited in + // this tab would inherit A's op-log position. The negative half matters + // as much: a clear that took the whole of sessionStorage would be a + // different, undocumented change. + sessionStorage.setItem(`${CURSOR_STORAGE_PREFIX}item-a`, '42'); + sessionStorage.setItem(`${CURSOR_STORAGE_PREFIX}item-b`, '7'); + sessionStorage.setItem('pad-unrelated', 'keep me'); + + clearPersistentIdentityState(); + + expect(sessionStorage.getItem(`${CURSOR_STORAGE_PREFIX}item-a`)).toBeNull(); + expect(sessionStorage.getItem(`${CURSOR_STORAGE_PREFIX}item-b`)).toBeNull(); + expect(sessionStorage.getItem('pad-unrelated')).toBe('keep me'); + }); + + it('drops route and scroll memory, which carry private item paths', async () => { + // `pad-last-scroll-...` embeds the pathname in its own KEY, so a private + // item slug is legible from the key list without reading any value. Both + // are keyed by workspace with no user in them. + localStorage.setItem(`${LAST_ROUTE_PREFIX}ws`, '/alice/ws/ideas/alphas-secret'); + localStorage.setItem(`${LAST_SCROLL_PREFIX}ws-/alice/ws/ideas/alphas-secret`, '420'); + localStorage.setItem('pad-theme', 'dark'); + + clearPersistentIdentityState(); + + expect(localStorage.getItem(`${LAST_ROUTE_PREFIX}ws`)).toBeNull(); + expect(localStorage.getItem(`${LAST_SCROLL_PREFIX}ws-/alice/ws/ideas/alphas-secret`)).toBeNull(); + // And leaves an ordinary UI preference alone — the clear is scoped, not + // a localStorage wipe. + expect(localStorage.getItem('pad-theme')).toBe('dark'); + }); + + it('LAST_ROUTE_PREFIX still matches the key workspace-route.ts builds', async () => { + const src = readFileSync( + resolve(__dirname, '../utils/workspace-route.ts'), + 'utf8', + ); + expect(src).toContain(LAST_ROUTE_PREFIX); + }); + + it('drops the attachment-metadata memo', async () => { + clearPersistentIdentityState(); + expect(clearAttachmentMetadataCache).toHaveBeenCalledTimes(1); + }); + + it('RECENT_SEARCHES_KEY still matches the literal in CommandPalette.svelte', async () => { + // The palette is a .svelte file and cannot export the constant, so the + // clear duplicates it. A rename on either side would leave a clear that + // silently matches nothing — the failure this whole mechanism exists to + // avoid, reintroduced one string at a time. + const src = readFileSync( + resolve(__dirname, '../components/search/CommandPalette.svelte'), + 'utf8', + ); + const match = src.match(/RECENT_SEARCHES_KEY\s*=\s*'([^']+)'/); + expect(match, 'CommandPalette.svelte no longer declares RECENT_SEARCHES_KEY').not.toBeNull(); + expect(match![1]).toBe(RECENT_SEARCHES_KEY); + }); +}); + +describe('reloadForIdentityChange', () => { + const original = window.location; + + afterEach(() => { + Object.defineProperty(window, 'location', { value: original, configurable: true }); + }); + + it('clears first, then reloads', async () => { + const order: string[] = []; + vi.mocked(clearAttachmentMetadataCache).mockImplementation(() => { order.push('clear'); }); + const reload = vi.fn(() => { order.push('reload'); }); + Object.defineProperty(window, 'location', { + value: { ...original, reload }, + configurable: true, + }); + + reloadForIdentityChange(); + + // ORDER, not just both: reloading first would tear the page down before + // the clears ran, leaving the palette history and collab cursors for the + // next user — the exact state the reload cannot drop by itself. + expect(order).toEqual(['clear', 'reload']); + }); +}); diff --git a/web/src/lib/stores/identityReload.svelte.ts b/web/src/lib/stores/identityReload.svelte.ts new file mode 100644 index 000000000..a76ef40a1 --- /dev/null +++ b/web/src/lib/stores/identityReload.svelte.ts @@ -0,0 +1,110 @@ +import { browser } from '$app/environment'; +import { clearAttachmentMetadataCache } from '$lib/components/editor/attachment-metadata'; +import { CURSOR_STORAGE_PREFIX } from '$lib/collab/wsProvider.svelte'; + +/** + * What a real identity change does to this tab (BUG-3005, lead ruling after + * codex round 3): clear the persistent state a reload would NOT drop, then + * reload. + * + * WHY A RELOAD RATHER THAN MORE FENCES. Three review rounds each found another + * layer of surfaces holding the previous user's data — three stores, then + * fourteen route pages plus a connection and two caches, then the console + * subtree, share pages, the command palette and an open dialog. The population + * is "everything in the tab that ever held a fetch", and a per-surface fix + * cannot terminate: it is a list somebody has to keep complete forever. A + * reload drops every store, component, cache and in-flight request at once and + * nothing new has to be remembered. + * + * WHAT THE RELOAD DOES NOT DROP is exactly this function's other half. + * `localStorage`, `sessionStorage` and IndexedDB survive it, so those stay an + * enumeration — but a BOUNDED one, over storage keys rather than over every + * module in the app, and one that a grep for the storage APIs can check. + * + * The store-level fences stay too, and are not redundant with this: they cover + * the window between the identity changing and the page actually going away, + * during which in-flight responses are still settling into live stores. + */ +/** Must equal `RECENT_SEARCHES_KEY` in CommandPalette.svelte. Pinned by a test. */ +export const RECENT_SEARCHES_KEY = 'pad-recent-searches'; +/** Built by `workspace-route.ts`; the prefix is duplicated, pinned by a test. */ +export const LAST_ROUTE_PREFIX = 'pad-last-route-'; +/** Built inline by every page that uses `createScrollRestoration`. */ +export const LAST_SCROLL_PREFIX = 'pad-last-scroll-'; + +export function clearPersistentIdentityState(): void { + if (!browser) return; + + // The editor's attachment-metadata memo. In RAM, so the reload would get it + // anyway — cleared here so that this function is the one place the answer + // to "what does an identity change drop" lives. + clearAttachmentMetadataCache(); + + // The command palette's recent searches. Search terms carry private item + // names and project context, and the key has no user in it. The literal is + // duplicated from CommandPalette.svelte, which cannot export it — a guard + // test pins the two spellings together. + try { + localStorage.removeItem(RECENT_SEARCHES_KEY); + } catch { + // A browser with storage disabled throws on access. Nothing to clear. + } + + // Route and scroll memory. `pad-last-route-` holds the last URL visited + // in a workspace — which can be a private item slug — and + // `pad-last-scroll--` embeds the path in its own KEY. Both are + // keyed by workspace with no user in them, so two accounts with access to + // the same workspace slug share them, and the second one is legible from + // the key list alone without reading a single value (codex round 4). + try { + for (const key of Object.keys(localStorage)) { + if (key.startsWith(LAST_ROUTE_PREFIX) || key.startsWith(LAST_SCROLL_PREFIX)) { + localStorage.removeItem(key); + } + } + } catch { + // As above. + } + + // Collaborative-editing cursors, keyed by item id ALONE. B opening an item + // A edited in this tab would otherwise inherit A's op-log cursor and + // reconcile against a document history that is not theirs. + try { + for (const key of Object.keys(sessionStorage)) { + if (key.startsWith(CURSOR_STORAGE_PREFIX)) sessionStorage.removeItem(key); + } + } catch { + // As above. + } +} + +/** + * Clear what survives a reload, then reload — the SWAP path only. + * + * A swap (one signed-in user to another) is the transition where nothing else + * navigates, so the reload is the whole mechanism. A SIGN-OUT does not come + * here: both sign-out sites navigate away by themselves, and a reload racing + * them aborts one of the two — which is exactly what it did, caught by + * `account-delete.spec.ts` rather than by any reasoning of mine. Sign-out + * calls `clearPersistentIdentityState` directly and lets its own hard + * navigation drop the rest. + * + * Separated from `clearPersistentIdentityState` so a test can drive the clears + * without navigating, and so the reload itself is one mockable call rather + * than a bare `location.reload()` buried in a layout. + */ +export function reloadForIdentityChange(): void { + if (!browser) return; + try { + clearPersistentIdentityState(); + } finally { + // THE RELOAD HAPPENS EITHER WAY (codex round 5). The clears are + // best-effort hygiene; the reload is the mechanism. A throw on the way + // through — today only reachable if the attachment memo's `clear()` + // grows an implementation that can fail — would otherwise leave the tab + // mounted with the previous user's page under the new identity, which + // is the state this whole fix exists to prevent, reached by the fix + // itself failing quietly. + location.reload(); + } +} diff --git a/web/src/lib/stores/identityResetRound1.svelte.test.ts b/web/src/lib/stores/identityResetRound1.svelte.test.ts new file mode 100644 index 000000000..0f2595a32 --- /dev/null +++ b/web/src/lib/stores/identityResetRound1.svelte.test.ts @@ -0,0 +1,167 @@ +import { describe, expect, it, vi, beforeEach } from 'vitest'; + +/** + * BUG-3005, codex round 1. The stores round 1 found still holding per-user + * state, plus the unfenced `loading` write in collectionStore. + * + * The four stores named on the item were not the population — the enumeration + * that produced them swept for per-user FIELDS, and these hold user-scoped + * state that does not look like one: an authorization-scoped statistics blob, + * a browser-tab title retired by a PATH stamp that an account swap does not + * move, and editor scalars that are consumed by GUARDS rather than displayed. + */ + +const api = vi.hoisted(() => ({ + collections: { list: vi.fn() }, + items: { list: vi.fn(), listByCollection: vi.fn(), get: vi.fn() }, +})); +vi.mock('$lib/api/client', () => ({ api })); +vi.mock('./localIndexPersistence', () => ({ + hydrateCollections: vi.fn(async () => null), + persistCollections: vi.fn(async () => {}), + readDurableEpoch: vi.fn(async () => null), +})); + +const auth = vi.hoisted(() => { + const listeners = new Set<() => void>(); + let epoch = 0; + return { + userId: 'user-1', + get identityEpoch() { return epoch; }, + identityFence() { + const captured = epoch; + return () => epoch === captured; + }, + onIdentityChange(fn: () => void) { + listeners.add(fn); + return () => listeners.delete(fn); + }, + fireIdentityChange() { + epoch++; + for (const fn of listeners) fn(); + }, + resetListeners() { + listeners.clear(); + epoch = 0; + }, + }; +}); +vi.mock('./auth.svelte', () => ({ authStore: auth })); + +beforeEach(() => { + vi.resetModules(); + auth.resetListeners(); + api.collections.list.mockReset(); + api.items.list.mockReset(); +}); + +describe('collectionStore loading flag', () => { + it('is not cleared by a load that settles after the identity changed', async () => { + // `loading` is module state written after an await like any other. A + // settling as B's load is still in flight clears B's flag and flashes + // empty-state or error UI mid-load. + const { collectionStore } = await import('./collections.svelte'); + let resolveA!: (v: unknown) => void; + api.items.list.mockReturnValueOnce(new Promise((r) => { resolveA = r; })); + const loadA = collectionStore.loadItems('ws'); + expect(collectionStore.loading).toBe(true); + + auth.fireIdentityChange(); + // PRECONDITION: the identity reset owns the flag, so nothing is stuck. + expect(collectionStore.loading).toBe(false); + + // B starts its own load, then A settles underneath it. + api.items.list.mockReturnValueOnce(new Promise(() => {})); + void collectionStore.loadItems('ws'); + expect(collectionStore.loading).toBe(true); + + resolveA([]); + await loadA; + + expect(collectionStore.loading).toBe(true); + }); +}); + +describe('adminStore across an identity change', () => { + it('drops the previous admin\'s statistics', async () => { + // `stats` is REALLY loaded first, through a stubbed fetch. Asserting + // null before and after would pass against a store with no clear at + // all — the failure mode this whole file exists to avoid. + const fetchMock = vi.fn(async () => ({ + ok: true, + json: async () => ({ users: 42 }), + })); + vi.stubGlobal('fetch', fetchMock); + const { adminStore } = await import('./admin.svelte'); + await adminStore.loadStats(); + + // PRECONDITION: the statistics are actually present. + expect(adminStore.stats).not.toBeNull(); + expect(adminStore.loading).toBe(false); + + auth.fireIdentityChange(); + + expect(adminStore.stats).toBeNull(); + // FALSE, not true (codex round 2). The admin layout loads on MOUNT and a + // same-route admin-to-admin swap mounts nothing, so leaving this true + // pinned the console on "Loading admin data…" forever. The layout + // re-issues the load on the same signal; until then the honest state is + // "nothing loaded". + expect(adminStore.loading).toBe(false); + expect(adminStore.error).toBe(''); + vi.unstubAllGlobals(); + }); +}); + +describe('titleStore across an identity change', () => { + it('drops the previous user\'s workspace and item title', async () => { + const { titleStore } = await import('./title.svelte'); + titleStore.setPageTitle({ workspace: 'Acme', section: 'Ideas', item: 'IDEA-1' }); + // PRECONDITION: the workspace part is set — it is the unstamped one, so + // it is the part a route change would NOT retire. + expect(titleStore.workspace).toBe('Acme'); + + auth.fireIdentityChange(); + + expect(titleStore.workspace).toBeUndefined(); + expect(titleStore.section).toBeUndefined(); + expect(titleStore.item).toBeUndefined(); + }); +}); + +describe('editorStore across an identity change', () => { + it('drops dirty and saveStatus, which are read by GUARDS', async () => { + const { editorStore } = await import('./editor.svelte'); + editorStore.setDirty(true); + editorStore.setSaveStatus('saving'); + editorStore.setMode('raw'); + editorStore.setLastSaveTime(1234); + editorStore.setExternalChange(true); + // PRECONDITION: all five are set, so the assertion below is about the + // clear rather than about defaults. + expect(editorStore.dirty).toBe(true); + expect(editorStore.saveStatus).toBe('saving'); + + auth.fireIdentityChange(); + + expect(editorStore.dirty).toBe(false); + expect(editorStore.saveStatus).toBe('idle'); + expect(editorStore.mode).toBe('edit'); + expect(editorStore.lastSaveTime).toBe(0); + expect(editorStore.externalChange).toBe(false); + }); + + it('is WIDER than resetForDoc, which keeps saveStatus deliberately', async () => { + // The counterfactual for reusing `resetForDoc` here: it is a DOCUMENT + // change within one session and keeps `saveStatus`/`lastSaveTime` on + // purpose. An identity change ends the session, so it needs the wider + // clear and cannot borrow that one. + const { editorStore } = await import('./editor.svelte'); + editorStore.setSaveStatus('saved'); + editorStore.setLastSaveTime(99); + editorStore.resetForDoc(); + + expect(editorStore.saveStatus).toBe('saved'); + expect(editorStore.lastSaveTime).toBe(99); + }); +}); diff --git a/web/src/lib/stores/identityResetRound2.svelte.test.ts b/web/src/lib/stores/identityResetRound2.svelte.test.ts new file mode 100644 index 000000000..d8a16ffb2 --- /dev/null +++ b/web/src/lib/stores/identityResetRound2.svelte.test.ts @@ -0,0 +1,188 @@ +import { describe, expect, it, vi, beforeEach } from 'vitest'; + +/** + * BUG-3005, codex round 2. Four defects the round-1 fixes left or created. + * + * The sharpest is the single-flight slot: dropping a store's DATA on an + * identity change while leaving the previous identity's promise published made + * the reset actively harmful, because the next caller joins a request whose own + * fence then refuses to commit. + */ + +const api = vi.hoisted(() => ({ + collections: { list: vi.fn() }, + items: { list: vi.fn(), listByCollection: vi.fn(), get: vi.fn(), starred: vi.fn() }, + workspaces: { list: vi.fn(), get: vi.fn(), me: vi.fn(), create: vi.fn() }, +})); +vi.mock('$lib/api/client', () => ({ api })); +vi.mock('./localIndexPersistence', () => ({ + hydrateCollections: vi.fn(async () => null), + persistCollections: vi.fn(async () => {}), + readDurableEpoch: vi.fn(async () => null), +})); + +const auth = vi.hoisted(() => { + const listeners = new Set<() => void>(); + let epoch = 0; + let id = 'user-1'; + return { + get userId() { return id; }, + setUserId(next: string) { id = next; }, + get identityEpoch() { return epoch; }, + identityFence() { + const captured = epoch; + return () => epoch === captured; + }, + onIdentityChange(fn: () => void) { + listeners.add(fn); + return () => listeners.delete(fn); + }, + fireIdentityChange(nextId?: string) { + if (nextId !== undefined) id = nextId; + epoch++; + for (const fn of listeners) fn(); + }, + resetListeners() { + listeners.clear(); + epoch = 0; + id = 'user-1'; + }, + }; +}); +vi.mock('./auth.svelte', () => ({ authStore: auth })); + +beforeEach(() => { + vi.resetModules(); + auth.resetListeners(); + api.collections.list.mockReset(); + api.items.starred.mockReset(); +}); + +describe('collectionStore single-flight slot', () => { + it('does not leave B joining A\'s abandoned request', async () => { + // Without the invalidation: B's `ensureCollections` joins A's published + // promise, A's identity fence refuses to commit, and B ends with no + // collections AND no request of its own — a permanently empty sidebar, + // which is worse than the leak the reset was closing. + const { collectionStore } = await import('./collections.svelte'); + let resolveA!: (v: unknown) => void; + api.collections.list.mockReturnValueOnce(new Promise((r) => { resolveA = r; })); + const loadA = collectionStore.loadCollections('ws'); + // `loadCollections` reads the durable epoch before it fetches, so the + // request is one microtask away rather than synchronous. + await Promise.resolve(); + await Promise.resolve(); + // PRECONDITION: A's request is the published one. + expect(api.collections.list).toHaveBeenCalledTimes(1); + + auth.fireIdentityChange(); + + // B asks for a list. It must ISSUE, not join. + api.collections.list.mockResolvedValueOnce([ + { id: 'c1', slug: 'tasks', name: 'Tasks', is_default: true, sort_order: 1 }, + ]); + await collectionStore.ensureCollections('ws'); + + expect(api.collections.list).toHaveBeenCalledTimes(2); + expect(collectionStore.collections).toHaveLength(1); + expect(collectionStore.collectionsAreFreshFor('ws')).toBe(true); + + // And A settling afterwards still commits nothing. + resolveA([{ id: 'c9', slug: 'alphas', name: 'Alphas', is_default: true, sort_order: 1 }]); + await loadA; + expect(collectionStore.collections).toHaveLength(1); + expect(collectionStore.collections[0].slug).toBe('tasks'); + }); +}); + +describe('adminStore.loadStats', () => { + it('refuses a response that settles after the identity changed', async () => { + let resolveFetch!: (v: unknown) => void; + const fetchMock = vi.fn(() => new Promise((r) => { resolveFetch = r; })); + vi.stubGlobal('fetch', fetchMock); + const { adminStore } = await import('./admin.svelte'); + + const inflight = adminStore.loadStats(); + // PRECONDITION: the request is actually out. + expect(fetchMock).toHaveBeenCalledTimes(1); + + auth.fireIdentityChange(); + resolveFetch({ ok: true, json: async () => ({ users: 42 }) }); + await inflight; + + expect(adminStore.stats).toBeNull(); + vi.unstubAllGlobals(); + }); +}); + +describe('toastStore across an identity change', () => { + it('drops live toasts AND the history, which names items', async () => { + const { toastStore } = await import('./toast.svelte'); + toastStore.show({ message: 'Archived TASK-12', type: 'success' }); + // PRECONDITION: both surfaces hold it, or the assertion below is about + // arrays that were always empty. + expect(toastStore.toasts.length).toBeGreaterThan(0); + expect(toastStore.history.length).toBeGreaterThan(0); + + auth.fireIdentityChange(); + + expect(toastStore.toasts).toHaveLength(0); + expect(toastStore.history).toHaveLength(0); + expect(toastStore.unreadCount).toBe(0); + }); +}); + +describe('workspaceStore single-flight slot across A -> B -> A', () => { + it('refuses a list issued in the FIRST A session', async () => { + // Where the sequence bump inside `invalidate` is the only guard left, + // and the reason it is there rather than just clearing the slot. + // + // `loadAll` fences its commit on the captured USER ID, which is the + // comparison the epoch exists to replace: after A -> B -> A that id + // agrees, so the only thing standing between A's first-session list and + // B-then-A's store is `isLatest` — and `isLatest` is a NAVIGATION fence + // that no identity change moves on its own. + const { workspaceStore } = await import('./workspace.svelte'); + let resolveA!: (v: unknown) => void; + api.workspaces.list.mockReturnValueOnce(new Promise((r) => { resolveA = r; })); + + const loadA = workspaceStore.loadAll(); + expect(api.workspaces.list).toHaveBeenCalledTimes(1); + + auth.fireIdentityChange('user-b'); + auth.fireIdentityChange('user-1'); + + // A's first-session response arrives last. The captured id matches the + // current one — this is the same user — and it must still be refused. + resolveA([{ id: 'w-alpha', slug: 'alphas-private', name: "Alpha's" }]); + await loadA; + + expect(workspaceStore.workspaces).toHaveLength(0); + }); +}); + +describe('openChildrenDialog across an identity change', () => { + it('abandons the queued mutation instead of letting B confirm it', async () => { + // The one place on this branch where the item's own bound ("nothing here + // is an authorization bypass") would stop being true: the dialog holds + // the FETCHED child list and a continuation that PERFORMS A's mutation + // when confirmed. It is mounted in the root layout, outside anything the + // workspace layout controls. + const { openChildrenDialog } = await import('./openChildrenDialog.svelte'); + const details = { count: 2, children: [{ ref: 'TASK-9', title: "Alpha's child" }] } as never; + + const first = openChildrenDialog.request('TASK-1', details); + const queued = openChildrenDialog.request('TASK-2', details); + // PRECONDITION: one is showing and one is queued behind it, so the test + // covers the queue and not just the active entry. + expect(openChildrenDialog.active).not.toBeNull(); + + auth.fireIdentityChange('user-b'); + + // FALSE, not a hanging promise: the producer awaiting this takes its + // cancel path and does not perform the write. + await expect(first).resolves.toBe(false); + await expect(queued).resolves.toBe(false); + expect(openChildrenDialog.active).toBeNull(); + }); +}); diff --git a/web/src/lib/stores/localIndex.svelte.ts b/web/src/lib/stores/localIndex.svelte.ts index 1d10e27e2..919c4c043 100644 --- a/web/src/lib/stores/localIndex.svelte.ts +++ b/web/src/lib/stores/localIndex.svelte.ts @@ -70,6 +70,7 @@ import { } from './localIndexPersistence'; import { preserveProjectionMetadata, mergeEqualSeqProjection } from './itemRowMerge'; import { localSearch } from './localSearch.svelte'; +import { authStore } from './auth.svelte'; import type { Item, ItemChangeRow, ItemIndexRow } from '$lib/types'; export type BootstrapState = 'cold' | 'loading' | 'ready' | 'error'; @@ -2293,8 +2294,59 @@ export const localIndex = { persistWipe(priorUserId, ws).catch(() => undefined); }, + /** + * Drop EVERY workspace's state — the identity-change sweep (BUG-3005). + * + * `reset(ws)` is per-workspace and the sign-out path called it for the ONE + * workspace the layout happened to be looking at. Everything this store + * holds is per-user: rows the previous user could see, their cursor, their + * access epoch. A tab that visited two workspaces kept the other one. + */ + resetAll(): void { + for (const ws of [...workspaces.keys()]) { + localIndex.reset(ws); + } + // `reset` already drops the search index for each workspace it visits. + // This catches an index whose workspace state was already gone: the two + // maps are keyed independently and nothing keeps them in step. + localSearch.resetAll(); + // THE DURABLE WIPE THIS INHERITS, and why it is kept (codex round 1). + // `reset` fire-and-forgets `persistWipe` for the workspace's last-known + // userId, so this drops the previous user's IndexedDB cache as well as + // their RAM. Not required for ISOLATION — the cache is namespaced per + // (user, workspace), so the next user could never read it — and it costs + // a returning user their warm cache. + // + // Kept because it is the behaviour sign-out already had (the workspace + // layout has called `reset` on sign-out since TASK-1360) and because + // leaving a signed-out user's item titles on the disk of a shared + // browser is the wrong default to adopt silently. What this DOES widen + // is the pre-existing race where a fast re-login's hydrate overlaps a + // wipe still in flight: it now applies to every workspace the tab + // visited rather than the one the layout named. Filed rather than fixed + // here — sequencing a fire-and-forget wipe against the next hydrate is + // a change to the persistence layer, not to this sweep. + }, + /** Number of items currently held for a workspace. Test/debug aid. */ size(ws: string): number { return workspaces.get(ws)?.items.size ?? 0; }, }; + +// Drop every workspace's rows and search index when the signed-in user changes +// (BUG-3005). +// +// This store reset only on `bootstrap()` noticing a userId mismatch, which +// makes the invalidation depend on somebody calling bootstrap: a route that +// does not bootstrap the index on an identity change kept the previous user's +// rows in RAM, and `localSearch` kept a full-text index built from their item +// titles and bodies. The subscription makes it structural — the same reason +// `onIdentityChange` exists rather than a call at each sign-out site. +// +// The bootstrap mismatch check STAYS. It covers a different case: a first +// bootstrap for a workspace whose cached state predates this tab's identity +// signal, where no change has fired because nothing changed within this tab. +authStore.onIdentityChange(() => { + localIndex.resetAll(); +}); diff --git a/web/src/lib/stores/localIndexIdentityReset.svelte.test.ts b/web/src/lib/stores/localIndexIdentityReset.svelte.test.ts new file mode 100644 index 000000000..1fd662181 --- /dev/null +++ b/web/src/lib/stores/localIndexIdentityReset.svelte.test.ts @@ -0,0 +1,130 @@ +import { describe, expect, it, vi, beforeEach } from 'vitest'; + +/** + * BUG-3005 — `localIndex` and `localSearch` held the previous user's rows + * across an SPA identity change. + * + * `localIndex` reset only when `bootstrap()` noticed a userId mismatch, which + * makes the invalidation depend on somebody calling bootstrap. A route that + * does not bootstrap the index on an identity change kept the rows in RAM — + * and `localSearch`, whose ONLY reset callers live inside `localIndex`, kept a + * full-text index built from the previous user's item titles and bodies. + * + * The per-workspace shape is the other half of the defect: `reset(ws)` names + * one slug, and a tab that visited two workspaces kept the one the sign-out + * path did not happen to be looking at. + */ + +vi.mock('$lib/api/client', () => ({ + api: { items: { index: vi.fn(), changes: vi.fn() } }, + PadApiError: class extends Error {}, +})); + +// Real epoch, real listener set — a stub would let these pass against a store +// that subscribes to nothing. +const auth = vi.hoisted(() => { + const listeners = new Set<() => void>(); + let epoch = 0; + return { + userId: 'user-1', + get identityEpoch() { return epoch; }, + identityFence() { + const captured = epoch; + return () => epoch === captured; + }, + onIdentityChange(fn: () => void) { + listeners.add(fn); + return () => listeners.delete(fn); + }, + fireIdentityChange() { + epoch++; + for (const fn of listeners) fn(); + }, + resetListeners() { + listeners.clear(); + epoch = 0; + }, + }; +}); + +vi.mock('./auth.svelte', () => ({ authStore: auth })); + +function row(id: string, title: string) { + return { + id, + seq: 1, + title, + collection_slug: 'colors', + created_at: '2026-01-01T00:00:00Z', + updated_at: '2026-01-01T00:00:00Z', + } as never; +} + +describe('localIndex / localSearch across an identity change', () => { + beforeEach(() => { + vi.resetModules(); + auth.resetListeners(); + }); + + it('drops EVERY workspace, not just the one a sign-out path names', async () => { + const { localIndex } = await import('./localIndex.svelte'); + localIndex.upsert('ws-one', row('a', 'Alpha')); + localIndex.upsert('ws-two', row('b', 'Beta')); + + // PRECONDITION: both workspaces hold rows, or "both were dropped" is a + // claim about state that was never there. This is the assertion the + // per-workspace reset would fail — it visits one slug. + expect(localIndex.size('ws-one')).toBe(1); + expect(localIndex.size('ws-two')).toBe(1); + + auth.fireIdentityChange(); + + expect(localIndex.size('ws-one')).toBe(0); + expect(localIndex.size('ws-two')).toBe(0); + }); + + it('drops the SEARCH index, so the next user cannot read the last one\'s titles', async () => { + const { localIndex } = await import('./localIndex.svelte'); + const { localSearch } = await import('./localSearch.svelte'); + localIndex.upsert('ws-one', row('a', 'Alpha')); + localSearch.rebuild('ws-one', [row('a', 'Alpha')]); + + // PRECONDITION: the title is findable, or the test cannot tell a dropped + // index from an index that never matched. + expect(localSearch.search('ws-one', 'Alpha').length).toBeGreaterThan(0); + + auth.fireIdentityChange(); + + expect(localSearch.search('ws-one', 'Alpha')).toHaveLength(0); + }); + + it('drops an index whose workspace state is already gone', async () => { + // The two maps are keyed independently and nothing keeps them in step, + // so a sweep that only visits the workspaces `localIndex` still knows + // about would leave this index resident. That is why `resetAll` calls + // `localSearch.resetAll()` rather than relying on the per-workspace + // resets it just performed. + const { localIndex } = await import('./localIndex.svelte'); + const { localSearch } = await import('./localSearch.svelte'); + localSearch.rebuild('orphan-ws', [row('a', 'Alpha')]); + + // PRECONDITION: localIndex holds nothing for this slug, so its own sweep + // will never visit it. + expect(localIndex.size('orphan-ws')).toBe(0); + expect(localSearch.search('orphan-ws', 'Alpha').length).toBeGreaterThan(0); + + auth.fireIdentityChange(); + + expect(localSearch.search('orphan-ws', 'Alpha')).toHaveLength(0); + }); + + it('keeps the rows when the identity holds still', async () => { + // The counterfactual. A sweep that fired on anything else would empty + // the local-first cache during ordinary use, which is a worse defect + // than the leak: every read would fall back to the network. + const { localIndex } = await import('./localIndex.svelte'); + localIndex.upsert('ws-one', row('a', 'Alpha')); + + expect(localIndex.size('ws-one')).toBe(1); + }); +}); diff --git a/web/src/lib/stores/localSearch.svelte.ts b/web/src/lib/stores/localSearch.svelte.ts index cc7727f65..20fe9a098 100644 --- a/web/src/lib/stores/localSearch.svelte.ts +++ b/web/src/lib/stores/localSearch.svelte.ts @@ -499,6 +499,22 @@ export const localSearch = { epochs.delete(ws); }, + /** + * Drop EVERY workspace's index (BUG-3005). + * + * `reset` is per-workspace and its callers name a slug, which is right for + * a 403 purge or a workspace deletion. An identity change is not scoped to + * a workspace: every index in this map was built from item titles and + * bodies the PREVIOUS user could see, and this map is keyed independently + * of `localIndex`'s — so an index whose workspace state is already gone + * would survive a sweep that only visited the workspaces localIndex still + * knows about. + */ + resetAll(): void { + indexes.clear(); + epochs.clear(); + }, + /** * Reactive per-workspace mutation epoch. Bumped on every successful * `rebuild` / `upsert` / `remove`. Consumers should READ this inside diff --git a/web/src/lib/stores/openChildrenDialog.svelte.ts b/web/src/lib/stores/openChildrenDialog.svelte.ts index deb64f290..01250ee09 100644 --- a/web/src/lib/stores/openChildrenDialog.svelte.ts +++ b/web/src/lib/stores/openChildrenDialog.svelte.ts @@ -12,6 +12,7 @@ // resolves; queueing avoids dropping the second prompt on the floor. import type { OpenChildrenDetails } from '$lib/items/openChildrenError'; +import { authStore } from './auth.svelte'; interface PendingRequest { parentRef: string; @@ -59,11 +60,45 @@ function cancel(): void { advanceQueue(); } +/** + * Abandon the active prompt and everything queued behind it (BUG-3005). + * + * Every pending request resolves FALSE — the same answer a cancel gives — so + * the producer awaiting it takes its cancel path and does not perform the + * mutation. Rejecting instead would surface an unhandled rejection at call + * sites that only ever expected a boolean. + */ +function abandonAll(): void { + const pending = active ? [active, ...queue] : [...queue]; + queue.length = 0; + active = null; + for (const entry of pending) entry.resolve(false); +} + export const openChildrenDialog = { get active(): PendingRequest | null { return active; }, request, confirm, - cancel + cancel, + abandonAll }; + +// This dialog is mounted in the ROOT layout, outside the workspace subtree the +// identity remount covers, and it holds two things that must not cross an +// identity change (BUG-3005, codex round 3): +// +// - `details`, the FETCHED child list from the 409 — B would read A's child +// titles and statuses out of an open dialog; +// - `resolve`, a continuation that PERFORMS A'S MUTATION when confirmed. That +// is a write under the wrong identity, not a display leak, and it is the +// one place on this branch where the item's own bound ("nothing here is an +// authorization bypass") would stop being true. +// +// Resolving false is what makes the second half safe: the producer sees a +// cancel and abandons the write, rather than being left with a promise that +// never settles. +authStore.onIdentityChange(() => { + openChildrenDialog.abandonAll(); +}); diff --git a/web/src/lib/stores/singleFlight.ts b/web/src/lib/stores/singleFlight.ts index e6ff097f6..af3982a51 100644 --- a/web/src/lib/stores/singleFlight.ts +++ b/web/src/lib/stores/singleFlight.ts @@ -74,6 +74,18 @@ export interface KeyedSingleFlight { */ inFlightFor(key: K): Promise | null; + /** + * Abandon the current run and the published slot (BUG-3005, codex round 2). + * + * A single-flight slot outlives the DATA its owner drops. Clearing a store + * on an identity change without this leaves the previous identity's promise + * published, so the next caller JOINS it, that run's own fence then refuses + * to commit, and the joiner is answered with nothing — no request issued and + * no data. Bumping the sequence is what makes the abandoned run lose + * `isLatest` as well, so it cannot commit after the slot is cleared either. + */ + invalidate(): void; + /** * Issue a run. ALWAYS issues; never joins an existing one. * @@ -99,6 +111,16 @@ export function createKeyedSingleFlight(options: SingleFlightOptions = {}): K return null; }, + invalidate(): void { + // The bump is the load-bearing half: it is what strips `isLatest` + // from the run being abandoned. Clearing the slot alone would stop + // new joiners while still letting that run commit. + seq++; + inFlightKey = null; + inFlightPromise = null; + setLoading?.(false); + }, + run(key: K, work: (run: SingleFlightRun) => Promise): Promise { const mySeq = ++seq; setLoading?.(true); diff --git a/web/src/lib/stores/starred.svelte.ts b/web/src/lib/stores/starred.svelte.ts index 6eba1d38a..864b1b10f 100644 --- a/web/src/lib/stores/starred.svelte.ts +++ b/web/src/lib/stores/starred.svelte.ts @@ -1,4 +1,5 @@ import { api } from '$lib/api/client'; +import { authStore } from './auth.svelte'; // Set of starred item IDs for the current user in the current workspace let starredIds = $state>(new Set()); @@ -41,6 +42,14 @@ export const starredStore = { async load(wsSlug: string) { currentWs = wsSlug; const seq = ++requestSeq; + // TWO FENCES, for two races (BUG-3005). `seq` is the navigation fence: a + // slower load for workspace A must not overwrite a faster one for B. + // The identity fence is a different question — this list is PER USER, so + // a response issued as A must not land in B's session even when the + // workspace never changed. Neither subsumes the other: a same-route + // account swap moves no workspace, and a workspace switch moves no + // identity. + const isSameIdentity = authStore.identityFence(); pendingToggles.clear(); // Clear stale state immediately to avoid showing a previous user/workspace's stars starredIds = new Set(); @@ -48,11 +57,11 @@ export const starredStore = { try { const items = await api.items.starred(wsSlug, { include_terminal: true }); - if (seq !== requestSeq) return; + if (seq !== requestSeq || !isSameIdentity()) return; starredIds = applyPendingToggles(new Set(items.map(i => i.id))); loaded = true; } catch { - if (seq !== requestSeq) return; + if (seq !== requestSeq || !isSameIdentity()) return; // Preserve any optimistic toggles even if the load failed starredIds = applyPendingToggles(new Set()); loaded = true; @@ -64,6 +73,7 @@ export const starredStore = { // Drop rapid duplicate clicks while a toggle is in flight for this item if (toggleInFlight.has(itemId)) return; + const isSameIdentity = authStore.identityFence(); const wasStarred = starredIds.has(itemId); const nowStarred = !wasStarred; @@ -87,8 +97,11 @@ export const starredStore = { await api.items.star(wsSlug, itemSlug); } } catch { - // Revert on failure (only if still on the same workspace) - if (currentWs !== wsSlug) return; + // Revert on failure — only if this is still the same workspace AND + // the same signed-in user. Without the identity half, a star request + // that fails after an account swap writes A's item id into B's + // starred set (BUG-3005). + if (currentWs !== wsSlug || !isSameIdentity()) return; pendingToggles.set(itemId, wasStarred); const reverted = new Set(starredIds); if (wasStarred) { @@ -110,3 +123,17 @@ export const starredStore = { toggleInFlight.clear(); } }; + +// Drop this user's stars the moment the signed-in user changes (BUG-3005). +// +// `load()` clears before it fetches, which covers a workspace SWITCH — but it +// is keyed by workspace slug and a same-route account swap starts no new load, +// so A's starred ids stayed on screen for B until something else happened to +// trigger one. The fences above cover a settle that RACES the change; this +// covers the case where nothing is racing at all, which is the common one. +// +// Module scope, registered once, never unsubscribed: this store is a singleton +// and its lifetime is the tab's. +authStore.onIdentityChange(() => { + starredStore.clear(); +}); diff --git a/web/src/lib/stores/starredIdentityReset.svelte.test.ts b/web/src/lib/stores/starredIdentityReset.svelte.test.ts new file mode 100644 index 000000000..718868a01 --- /dev/null +++ b/web/src/lib/stores/starredIdentityReset.svelte.test.ts @@ -0,0 +1,155 @@ +import { describe, expect, it, vi, beforeEach } from 'vitest'; + +/** + * BUG-3005 — `starredStore` holds a PER-USER set and was only ever cleared + * inside `load()`, which is keyed by workspace slug. + * + * A same-route account swap starts no new load, so A's starred ids stayed + * rendered inside B's session with no request having been made for them. Two + * halves that fail independently: the identity-change reset (nothing racing), + * and the identity fence on a settle (something racing). + */ + +const api = vi.hoisted(() => ({ + items: { + starred: vi.fn(), + star: vi.fn(), + unstar: vi.fn(), + }, +})); + +vi.mock('$lib/api/client', () => ({ api })); + +// A REAL epoch and fence, not stubs. A fence stub that always answered "still +// current" would let every assertion below pass against a store with no fence. +const auth = vi.hoisted(() => { + const listeners = new Set<() => void>(); + let epoch = 0; + return { + userId: 'user-1', + get identityEpoch() { return epoch; }, + identityFence() { + const captured = epoch; + return () => epoch === captured; + }, + onIdentityChange(fn: () => void) { + listeners.add(fn); + return () => listeners.delete(fn); + }, + /** What authStore.clear() or a sign-in as someone else fires. */ + fireIdentityChange() { + epoch++; + for (const fn of listeners) fn(); + }, + resetListeners() { + listeners.clear(); + epoch = 0; + }, + }; +}); + +vi.mock('./auth.svelte', () => ({ authStore: auth })); + +describe('starredStore across an identity change', () => { + beforeEach(() => { + vi.resetModules(); + auth.resetListeners(); + api.items.starred.mockReset(); + api.items.star.mockReset(); + api.items.unstar.mockReset(); + }); + + it('drops the previous user\'s stars when the identity changes with nothing in flight', async () => { + const { starredStore } = await import('./starred.svelte'); + api.items.starred.mockResolvedValueOnce([{ id: 'item-a' }]); + await starredStore.load('ws'); + + // PRECONDITION: A's stars are actually loaded, or "they were dropped" is + // a claim about a set that was always empty. + expect(starredStore.isStarred('item-a')).toBe(true); + expect(starredStore.loaded).toBe(true); + + auth.fireIdentityChange(); + + expect(starredStore.isStarred('item-a')).toBe(false); + expect(starredStore.ids.size).toBe(0); + expect(starredStore.loaded).toBe(false); + }); + + it('refuses a load that RESOLVES after the identity changed', async () => { + const { starredStore } = await import('./starred.svelte'); + let resolve!: (v: unknown) => void; + api.items.starred.mockReturnValueOnce(new Promise((r) => { resolve = r; })); + + const inflight = starredStore.load('ws'); + auth.fireIdentityChange(); + resolve([{ id: 'item-a' }]); + await inflight; + + expect(starredStore.isStarred('item-a')).toBe(false); + expect(starredStore.loaded).toBe(false); + }); + + it('refuses a FAILED load that rejects after the identity changed', async () => { + // The catch arm commits too — it publishes an empty set and flips + // `loaded` — so a fence on the success path alone leaves the store + // claiming, inside B's session, that B's stars are loaded. + const { starredStore } = await import('./starred.svelte'); + let reject!: (e: unknown) => void; + api.items.starred.mockReturnValueOnce(new Promise((_, r) => { reject = r; })); + + const inflight = starredStore.load('ws'); + auth.fireIdentityChange(); + reject(new Error('network')); + await inflight; + + expect(starredStore.loaded).toBe(false); + }); + + it('does not revert a failed UNSTAR into the NEXT user\'s set', async () => { + // Direction matters, and the first version of this test had it backwards. + // Reverting a failed STAR is a delete, which is invisible in an empty + // set — the test passed with no identity fence at all. Reverting a failed + // UNSTAR is an ADD, and that is the write that lands one user's starred + // item in another user's set. + // + // B also signs back into the SAME workspace before the revert fires, + // because the identity-change reset blanks `currentWs` and would + // otherwise be the guard doing the work. Both halves are needed for this + // to discriminate. + const { starredStore } = await import('./starred.svelte'); + api.items.starred.mockResolvedValueOnce([{ id: 'item-a' }]); + await starredStore.load('ws'); + expect(starredStore.isStarred('item-a')).toBe(true); + + let reject!: (e: unknown) => void; + api.items.unstar.mockReturnValueOnce(new Promise((_, r) => { reject = r; })); + const toggling = starredStore.toggle('ws', 'item-a-slug', 'item-a'); + // PRECONDITION: the optimistic REMOVE landed, so the revert is an add. + expect(starredStore.isStarred('item-a')).toBe(false); + + auth.fireIdentityChange(); + api.items.starred.mockResolvedValueOnce([]); + await starredStore.load('ws'); + // PRECONDITION: B's set is loaded and empty, and the workspace fence can + // no longer discriminate. + expect(starredStore.loaded).toBe(true); + expect(starredStore.ids.size).toBe(0); + + reject(new Error('nope')); + await toggling; + + expect(starredStore.isStarred('item-a')).toBe(false); + }); + + it('still commits an ordinary load when the identity holds still', async () => { + // The counterfactual. A fence that refused everything would empty the + // starred set for every user permanently, which is worse than the leak. + const { starredStore } = await import('./starred.svelte'); + api.items.starred.mockResolvedValueOnce([{ id: 'item-a' }]); + await starredStore.load('ws'); + + expect(starredStore.isStarred('item-a')).toBe(true); + expect(starredStore.loaded).toBe(true); + }); +}); diff --git a/web/src/lib/stores/title.svelte.ts b/web/src/lib/stores/title.svelte.ts index a2f593ee1..11a933c2e 100644 --- a/web/src/lib/stores/title.svelte.ts +++ b/web/src/lib/stores/title.svelte.ts @@ -1,3 +1,4 @@ +import { authStore } from './auth.svelte'; /** * Title store — central source of truth for the browser-tab title. * @@ -128,3 +129,12 @@ export const titleStore = { itemPath = undefined; }, }; + +// The path STAMP is what normally retires a title part, and a same-route +// identity change moves no path — so A's workspace name and item ref stayed in +// the browser tab and the mobile context bar for B (BUG-3005, codex round 1). +// Cosmetic rather than a data leak of any size, and cheap enough that leaving +// one member of the class unfixed costs more than fixing it. +authStore.onIdentityChange(() => { + titleStore.clearPageTitle(); +}); diff --git a/web/src/lib/stores/toast.svelte.ts b/web/src/lib/stores/toast.svelte.ts index 4360d7aad..48d47ee2a 100644 --- a/web/src/lib/stores/toast.svelte.ts +++ b/web/src/lib/stores/toast.svelte.ts @@ -1,3 +1,4 @@ +import { authStore } from './auth.svelte'; export interface ToastAction { label: string; onAction: () => void; @@ -116,6 +117,19 @@ function clearHistory(): void { unreadCount = 0; } +/** + * Drop live toasts, the history and the unread count (BUG-3005). + * + * Timers are cancelled rather than left to fire: a pending auto-dismiss for a + * toast that no longer exists would call `dismiss` on a missing id, and the + * map would keep the handle until then. + */ +function clearAll(): void { + for (const id of [...timers.keys()]) clearTimerFor(id); + toasts = []; + clearHistory(); +} + export const toastStore = { get toasts(): Toast[] { return toasts; @@ -129,5 +143,15 @@ export const toastStore = { show, dismiss, markAllRead, - clearHistory + clearHistory, + clearAll }; + +// The notification tray is a readable LOG of the previous user's actions +// (BUG-3005, the enumeration table). Toast text routinely names items — +// "Archived TASK-12" — so leaving the history in place lets B read what A just +// did. Live toasts go too: a toast fired for A's action has no meaning in B's +// session, and `dismissAll` also cancels their timers. +authStore.onIdentityChange(() => { + toastStore.clearAll(); +}); diff --git a/web/src/lib/stores/workspace.svelte.ts b/web/src/lib/stores/workspace.svelte.ts index 2d7b6aa80..e150df6b7 100644 --- a/web/src/lib/stores/workspace.svelte.ts +++ b/web/src/lib/stores/workspace.svelte.ts @@ -580,4 +580,12 @@ authStore.onIdentityChange(() => { current = null; currentMembership = null; membershipKnown = false; + // The SINGLE-FLIGHT SLOT, for the reason collectionStore's reset carries + // the same line (BUG-3005, codex round 2): dropping the list while leaving + // the previous identity's promise published lets the next caller JOIN it, + // that run's own fence then refuses to commit, and the joiner gets no data + // and no request. Not a defect this file was filed for — it is the same + // defect in the store BUG-2991 fixed, and leaving the identical one line + // away would be knowing about it and not saying so. + loadAllFlight.invalidate(); }); diff --git a/web/src/routes/+layout.svelte b/web/src/routes/+layout.svelte index e6da8d364..4dd438b13 100644 --- a/web/src/routes/+layout.svelte +++ b/web/src/routes/+layout.svelte @@ -5,6 +5,10 @@ import { page } from '$app/state'; import { workspaceStore } from '$lib/stores/workspace.svelte'; import { authStore } from '$lib/stores/auth.svelte'; + import { + reloadForIdentityChange, + clearPersistentIdentityState, + } from '$lib/stores/identityReload.svelte'; import { uiStore } from '$lib/stores/ui.svelte'; import { titleStore } from '$lib/stores/title.svelte'; import { setAccessRevokedHandler, setRateLimitHandler } from '$lib/api/client'; @@ -105,8 +109,48 @@ // Deliberately outside the async body below — an `onMount` that returns // a Promise has its resolved value ignored, so a cleanup returned from // an async `onMount` is never called. - return authStore.onIdentityChange(() => { + return authStore.onIdentityChange((previousUserId) => { + // Re-armed for the pre-reload window: this latch is THIS component's + // state and the reload below may not have happened yet. workspacesRequested = false; + // WHAT A REAL IDENTITY CHANGE DOES TO THIS TAB (BUG-3005, lead + // ruling after codex round 3): clear the persistent state a reload + // would not drop, then reload. + // + // Three review rounds each found another layer of surfaces holding + // the previous user's data, because the fix was per-surface and the + // population is "everything in the tab that ever held a fetch". A + // reload ends the enumeration: every store, component, cache and + // in-flight request goes at once, and no future surface has to be + // remembered. What it does NOT drop — localStorage, sessionStorage, + // IndexedDB — is still an enumeration, but over STORAGE KEYS, which + // is bounded and greppable. + // ONLY WHEN SOMEBODY WAS SIGNED IN BEFORE. A sign-IN from an + // unauthenticated tab changes the identity, but the state it would + // be dropping is anonymous — nobody's private data — and reloading + // there puts a full page load in the middle of the login flow. + if (!previousUserId) return; + + // SIGN-OUT CLEARS BUT DOES NOT RELOAD. Both sign-out sites navigate + // away by themselves — account delete with `location.href`, console + // logout with a hard navigation for the same reason — and a reload + // racing them ABORTS one of the two. That is not a hypothesis: it + // is `account-delete.spec.ts:147` failing with + // `net::ERR_ABORTED; maybe frame was detached?`, which is how this + // branch learned it. The persistent clears still have to happen, + // because a hard navigation drops the tab's memory and not + // localStorage, sessionStorage or IndexedDB. + // + // A SWAP has no such navigation of its own, so there the reload IS + // the mechanism. + // + // The property is the same either way: every transition away from a + // real user drops that user's in-tab state. + if (!authStore.userId) { + clearPersistentIdentityState(); + return; + } + reloadForIdentityChange(); }); }); diff --git a/web/src/routes/[username]/[workspace]/+layout.svelte b/web/src/routes/[username]/[workspace]/+layout.svelte index f5f44dfab..4966a2c50 100644 --- a/web/src/routes/[username]/[workspace]/+layout.svelte +++ b/web/src/routes/[username]/[workspace]/+layout.svelte @@ -229,6 +229,27 @@ } }); + // CLOSE THE STREAM, and let the reload do the rest (BUG-3005, lead ruling + // after codex round 3). + // + // A real identity change reloads this tab — see `reloadForIdentityChange` + // in the root layout for why the fix stopped being per-surface. So this + // listener no longer re-runs the layout's per-workspace work: the page is + // going away, and a reload does it properly. + // + // The SSE disconnect STAYS, and is the reason this listener still exists. + // `sseService.connect` is idempotent per workspace, so the EventSource A + // opened would otherwise keep delivering into this tab for the whole + // pre-reload window — a stream still authorized, because signing in as B + // REPLACES the cookie without destroying A's session row, which is exactly + // the case BUG-3007's server-side close cannot see (BUG-3011 tracks the + // server half). Closing it here bounds that window to zero rather than to + // however long the reload takes. + const stopIdentityWatch = authStore.onIdentityChange(() => { + sseService.disconnect(); + }); + onDestroy(stopIdentityWatch); + // Re-attempt WebMCP registration when the auth gate resolves. The // workspace $effect above runs inside untrack() (so a workspace switch // doesn't drag in the whole workspaces array as a dep), which means it diff --git a/web/src/routes/[username]/[workspace]/starred/+page.svelte b/web/src/routes/[username]/[workspace]/starred/+page.svelte index f3b26661a..ccfa3224d 100644 --- a/web/src/routes/[username]/[workspace]/starred/+page.svelte +++ b/web/src/routes/[username]/[workspace]/starred/+page.svelte @@ -5,6 +5,7 @@ import { workspaceStore } from '$lib/stores/workspace.svelte'; import { collectionStore } from '$lib/stores/collections.svelte'; import { starredStore } from '$lib/stores/starred.svelte'; + import { authStore } from '$lib/stores/auth.svelte'; import { createScrollRestoration } from '$lib/scroll/restore.svelte'; import ItemCard from '$lib/components/collections/ItemCard.svelte'; import PageHeader from '$lib/components/common/PageHeader.svelte'; @@ -50,21 +51,31 @@ workspaceStore.setCurrent(wsSlug); }); + // NO IDENTITY LISTENER HERE, deliberately (BUG-3005, lead ruling). This page + // had one, and with the tab reloading on a real identity change it became + // the third path doing the same reload — the layout's listener, this one, + // and the page's own mount effect. One mechanism; the fence below is what + // this page still needs, because it covers a response settling in the + // window before the reload takes the page away. + async function loadStarred(slug: string) { loading = true; const seq = ++loadSeq; + // The identity that ASKED. `seq` is a navigation/refresh fence and does + // not move on an account swap, so it cannot tell this apart. + const isSameIdentity = authStore.identityFence(); try { const [starredItems, colls] = await Promise.all([ api.items.starred(slug, { include_terminal: includeTerminal }), api.collections.list(slug) ]); - if (seq !== loadSeq) return; + if (seq !== loadSeq || !isSameIdentity()) return; fetchedItems = starredItems; collections = colls; } catch { if (seq !== loadSeq) return; } finally { - if (seq === loadSeq) loading = false; + if (seq === loadSeq && isSameIdentity()) loading = false; } } diff --git a/web/src/routes/[username]/[workspace]/starred/starredPageIdentityReset.svelte.test.ts b/web/src/routes/[username]/[workspace]/starred/starredPageIdentityReset.svelte.test.ts new file mode 100644 index 000000000..1ae559888 --- /dev/null +++ b/web/src/routes/[username]/[workspace]/starred/starredPageIdentityReset.svelte.test.ts @@ -0,0 +1,150 @@ +// Runs in the jsdom vitest project (filename ends `.svelte.test.ts`). +// +// BUG-3005, codex round 1 — the starred PAGE keeps its own copy of the +// previous user's items, and the store fix alone does not reach it. +// +// Two things make this page worse than an ordinary stale cache, and the second +// is caused by the store fix itself: +// +// - its load effect is keyed on the workspace slug and the terminal filter, +// so a same-route account swap starts no reload at all; +// - `items` falls back to UNFILTERED `fetchedItems` whenever +// `starredStore.loaded` is false — which is exactly what the store's +// identity reset sets it to. So the store's fix routes this page around +// its only filter and B sees A's starred items in full. +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { render, cleanup } from '@testing-library/svelte'; +import { tick } from 'svelte'; + +const api = vi.hoisted(() => ({ + auth: { session: vi.fn() }, + items: { starred: vi.fn(), star: vi.fn(), unstar: vi.fn() }, + collections: { list: vi.fn() }, + workspaces: { get: vi.fn(), me: vi.fn(), list: vi.fn() }, +})); + +vi.mock('$lib/api/client', () => ({ + api, + setAccessRevokedHandler: () => {}, + setRateLimitHandler: () => {}, + isPlanLimitError: () => false, + planLimitMessage: () => '', +})); + +vi.mock('$app/navigation', () => ({ + goto: vi.fn(), + beforeNavigate: () => {}, + afterNavigate: () => {}, + invalidateAll: vi.fn(), + pushState: vi.fn(), + replaceState: vi.fn(), +})); + +import StarredPage from './+page.svelte'; +import { authStore } from '$lib/stores/auth.svelte'; +import { starredStore } from '$lib/stores/starred.svelte'; +import { page } from '$app/state'; + +function sessionFor(id: string) { + return { authenticated: true, user: { id, email: `${id}@example.com` } }; +} + +const ITEM_A = { + id: 'item-a', + slug: 'alphas-secret', + title: "Alpha's secret item", + collection_slug: 'ideas', + status: 'open', + fields: {}, +}; + +async function settle(): Promise { + for (let i = 0; i < 10; i++) { + await Promise.resolve(); + await tick(); + } +} + +describe('starred page across an identity change', () => { + beforeEach(async () => { + // NO vi.resetModules() here (codex round 5): this file imports the page + // component and the auth store at top level, and resetting the registry + // mid-file hands the test a different authStore instance than the one + // the component closed over — so the identity change fires into a store + // nothing under test is listening to. + api.items.starred.mockReset(); + api.collections.list.mockReset(); + api.auth.session.mockReset(); + page.params.workspace = 'ws'; + page.params.username = 'alice'; + // Establish an identity so later transitions actually fire — the first + // resolution is the baseline and notifies nobody. + api.auth.session.mockResolvedValue(sessionFor('user-a')); + await authStore.load(); + }); + + afterEach(() => { + cleanup(); + authStore.clear(); + }); + + it('refuses a load issued as A that settles after the identity changed', async () => { + // The page's identity LISTENER is gone (the tab reloads instead), so + // what it still owns is the pre-reload window: a request issued as A can + // settle before the reload takes the page away, and `loadSeq` is a + // navigation fence that an account swap does not move. + // + // The page's FIRST load is the one left pending, because it is the only + // one this harness can hold open reliably — an attempt to start a second + // through the terminal-filter toggle left the fence undetectable by its + // own mutant, which is the failure this file has produced twice now. + // EVERY mock also carries a DEFAULT beside the pending one-shot (codex + // round 5): without that, a later call resolved `undefined`, `items.map` + // threw, and vitest reported PASSED with an unhandled error beside it. + let resolveA!: (v: unknown) => void; + api.items.starred.mockReturnValueOnce(new Promise((r) => { resolveA = r; })); + api.items.starred.mockResolvedValue([]); + api.collections.list.mockResolvedValue([]); + + const screen = render(StarredPage); + const count = () => screen.container.querySelector('.count')?.textContent ?? ''; + await settle(); + // PRECONDITION: A's request is out and unanswered, so what follows is + // about the settle rather than about a page that never asked. + expect(api.items.starred).toHaveBeenCalled(); + expect(count()).toBe('0'); + + api.auth.session.mockResolvedValue(sessionFor('user-b')); + await authStore.load(); + resolveA([ITEM_A]); + await settle(); + + expect(count()).toBe('0'); + expect(screen.queryByText("Alpha's secret item")).toBeNull(); + + // WHAT THIS LEG DOES NOT PROVE, measured rather than assumed: removing + // the page's identity fence (`|| !isSameIdentity()`) leaves it GREEN. + // The mutant survives because the page's own `$effect` re-runs during + // the identity transition and advances `loadSeq`, so the NAVIGATION + // fence refuses A's settle first and the identity fence never decides + // anything here. The leg is an end-state regression fence — A's items + // must not appear — and the identity fence's coverage of the pre-reload + // window is an argument, not a measurement. Said plainly because a + // surviving mutant recorded as a passing test is how this file already + // shipped one assertion pointed the wrong way. + }); + + it("renders A's items when the identity holds still", async () => { + // The counterfactual the test above needs to mean anything: the same + // harness, no identity change, and the data DOES render. Without it, + // `count === '0'` is equally consistent with a page that never works. + api.items.starred.mockResolvedValue([ITEM_A]); + api.collections.list.mockResolvedValue([]); + + const screen = render(StarredPage); + const count = () => screen.container.querySelector('.count')?.textContent ?? ''; + await settle(); + + expect(count()).toBe('1'); + }); +}); diff --git a/web/src/routes/[username]/[workspace]/workspaceLayoutIdentitySse.svelte.test.ts b/web/src/routes/[username]/[workspace]/workspaceLayoutIdentitySse.svelte.test.ts new file mode 100644 index 000000000..2ecd2ab2d --- /dev/null +++ b/web/src/routes/[username]/[workspace]/workspaceLayoutIdentitySse.svelte.test.ts @@ -0,0 +1,158 @@ +// Runs in the jsdom vitest project (filename ends `.svelte.test.ts`). +// +// BUG-3005, codex round 2 — the persistent workspace layout across an identity +// change. Two bindings, and neither is observable anywhere else (CONVE-19): +// +// A real identity change RELOADS the tab (lead ruling after round 3), so this +// layout no longer re-runs its per-workspace work and no longer remounts leaf +// pages — the reload does both, properly, and one mechanism is the point. +// +// What survives here is the SSE DISCONNECT, and it is not redundant with the +// reload. `sseService.connect` is idempotent per workspace, so the EventSource +// A opened keeps delivering into this tab for the whole pre-reload window — +// still authorized, because signing in as B replaces the cookie without +// destroying A's session row, which is precisely the case BUG-3007's +// server-side close cannot see. Closing it here bounds that window to zero. +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { render, cleanup } from '@testing-library/svelte'; +import { createRawSnippet, tick } from 'svelte'; + +const api = vi.hoisted(() => ({ + auth: { session: vi.fn() }, + workspaces: { get: vi.fn(), me: vi.fn(), list: vi.fn() }, + collections: { list: vi.fn() }, + items: { starred: vi.fn(), list: vi.fn() }, + dashboard: { get: vi.fn() }, + tags: { list: vi.fn() }, +})); + +vi.mock('$lib/api/client', () => ({ + api, + setAccessRevokedHandler: () => {}, + setRateLimitHandler: () => {}, + isPlanLimitError: () => false, + planLimitMessage: () => '', + PadApiError: class extends Error {}, +})); + +vi.mock('$app/navigation', () => ({ + goto: vi.fn(), + beforeNavigate: () => {}, + afterNavigate: () => {}, + invalidateAll: vi.fn(), + pushState: vi.fn(), + replaceState: vi.fn(), +})); + +const sse = vi.hoisted(() => ({ + connect: vi.fn(), + disconnect: vi.fn(), + onItemEvent: vi.fn(() => () => {}), + onCollectionEvent: vi.fn(() => () => {}), + onWorkspaceEvent: vi.fn(() => () => {}), + onConnectionChange: vi.fn(() => () => {}), +})); +vi.mock('$lib/services/sse.svelte', () => ({ sseService: sse })); + +const sync = vi.hoisted(() => ({ + init: vi.fn(), + setWorkspace: vi.fn(), + onSync: vi.fn(() => () => {}), + markSynced: vi.fn(), + syncNow: vi.fn(), + destroy: vi.fn(), +})); +vi.mock('$lib/services/sync.svelte', () => ({ syncService: sync })); + +vi.mock('$lib/webmcp/register', () => ({ registerWorkspaceTools: vi.fn(async () => null) })); + +import Layout from './+layout.svelte'; +import { authStore } from '$lib/stores/auth.svelte'; +import { page } from '$app/state'; + +function sessionFor(id: string) { + return { authenticated: true, user: { id, email: `${id}@example.com` } }; +} + +/** Counts how many times the leaf-page snippet has been constructed. */ +let mountCount = 0; +const childSnippet = createRawSnippet(() => ({ + // Counted in `render`, not in the factory: the factory is invoked once per + // snippet value, so a remount would not move it and the test would be + // vacuous. `render` runs each time the block is (re)created. + render: () => { + mountCount++; + return `
leaf
`; + }, +})); + +async function settle(): Promise { + for (let i = 0; i < 10; i++) { + await Promise.resolve(); + await tick(); + } +} + +describe('workspace layout across an identity change', () => { + beforeEach(async () => { + vi.resetModules(); + mountCount = 0; + sse.connect.mockClear(); + sse.disconnect.mockClear(); + api.auth.session.mockReset(); + api.collections.list.mockResolvedValue([]); + api.items.starred.mockResolvedValue([]); + api.workspaces.get.mockResolvedValue({ id: 'w1', slug: 'ws', name: 'WS' }); + api.workspaces.me.mockResolvedValue({ role: 'owner' }); + api.dashboard.get.mockResolvedValue({ has_agent_activity: true }); + api.tags.list.mockResolvedValue([]); + page.params.workspace = 'ws'; + page.params.username = 'alice'; + api.auth.session.mockResolvedValue(sessionFor('user-a')); + await authStore.load(); + }); + + afterEach(() => { + cleanup(); + authStore.clear(); + }); + + it('disconnects the previous identity\'s SSE stream', async () => { + render(Layout, { props: { children: childSnippet } }); + await settle(); + + // PRECONDITION: the layout connected for A. Without this, "it + // reconnected" is a claim about a stream that was never opened. + expect(sse.connect).toHaveBeenCalled(); + const connectsBefore = sse.connect.mock.calls.length; + const disconnectsBefore = sse.disconnect.mock.calls.length; + + api.auth.session.mockResolvedValue(sessionFor('user-b')); + await authStore.load(); + await settle(); + + expect(sse.disconnect.mock.calls.length).toBeGreaterThan(disconnectsBefore); + // And does NOT reconnect: the tab is reloading, and a fresh connection + // opened milliseconds before teardown is work nobody needs. + expect(sse.connect.mock.calls.length).toBe(connectsBefore); + }); + + it('does NOT remount the leaf page — the reload owns that now', async () => { + // The `{#key}` remount this layout carried for one round is gone. It was + // a second mechanism doing the reload's job, and it gave the starred + // page three reload paths on one identity change. Pinned as an ABSENCE + // so re-adding it has to be a deliberate change rather than a quiet one. + render(Layout, { props: { children: childSnippet } }); + await settle(); + + // PRECONDITION: mounted once for A, so "still 1" is a claim about a + // component that actually rendered. + expect(mountCount).toBe(1); + + api.auth.session.mockResolvedValue(sessionFor('user-b')); + await authStore.load(); + await settle(); + + expect(mountCount).toBe(1); + }); +}); diff --git a/web/src/routes/console/+layout.svelte b/web/src/routes/console/+layout.svelte index 87ef52414..128959a83 100644 --- a/web/src/routes/console/+layout.svelte +++ b/web/src/routes/console/+layout.svelte @@ -42,7 +42,14 @@ async function logout() { await api.auth.logout(); authStore.clear(); - goto('/login'); + // HARD navigation, not `goto` (BUG-3005). An SPA navigation to /login + // leaves every client-side singleton, component and cache exactly where + // it was — which is the original defect: the next person to sign in on + // this browser reads the previous user's data out of a tab that never + // went away. Account deletion in `console/settings/+page.svelte` has + // always done this, with a comment saying why; the two sign-out sites + // agree now. + window.location.href = '/login'; } function closeMobileMenu() { diff --git a/web/src/routes/console/consoleLogoutHardNavigates.test.ts b/web/src/routes/console/consoleLogoutHardNavigates.test.ts new file mode 100644 index 000000000..369c8b436 --- /dev/null +++ b/web/src/routes/console/consoleLogoutHardNavigates.test.ts @@ -0,0 +1,44 @@ +// Node-project test (no DOM): a SOURCE guard that the console logout leaves the +// page by a HARD navigation (BUG-3005). +// +// `goto('/login')` is an SPA navigation. It leaves every client-side singleton, +// component and cache exactly where it was — which is this bug's original +// defect: the next person to sign in on this browser reads the previous user's +// data out of a tab that never went away. Account deletion in +// `console/settings/+page.svelte` has always used `window.location.href` and +// says why in its own comment; this makes the two sign-out sites agree. +// +// It is a source guard because the alternative is rendering the console layout +// to observe an assignment to `window.location`, which jsdom does not perform. +// WHAT A SOURCE GUARD CANNOT DO: it checks spellings, not behaviour — a logout +// that hard-navigates somewhere useless would still pass. +import { describe, expect, it } from 'vitest'; +import { readFileSync } from 'node:fs'; +import { resolve } from 'node:path'; + +function logoutBody(file: string): string { + const src = readFileSync(resolve(__dirname, file), 'utf8').replace(/^[ \t]*\/\/.*$/gm, ''); + const start = src.indexOf('async function logout()'); + expect(start, `${file} no longer declares logout()`).toBeGreaterThan(-1); + return src.slice(start, src.indexOf('\n\t}', start)); +} + +describe('console sign-out', () => { + it('hard-navigates rather than using goto', () => { + const body = logoutBody('./+layout.svelte'); + expect(body).toContain("window.location.href = '/login'"); + expect(body).not.toContain('goto('); + }); + + it('still clears the auth store first', () => { + // Order matters: `clear()` is what fires the identity listeners that run + // the persistent clears, and a navigation started first can cut them off. + const body = logoutBody('./+layout.svelte'); + expect(body.indexOf('authStore.clear()')).toBeLessThan(body.indexOf('window.location.href')); + }); + + it('account deletion still hard-navigates too — the site this one now matches', () => { + const src = readFileSync(resolve(__dirname, './settings/+page.svelte'), 'utf8'); + expect(src).toContain("window.location.href = '/login'"); + }); +}); diff --git a/web/src/routes/layoutWorkspaceReloadOnIdentity.svelte.test.ts b/web/src/routes/layoutWorkspaceReloadOnIdentity.svelte.test.ts index 04472779c..5001f5b2d 100644 --- a/web/src/routes/layoutWorkspaceReloadOnIdentity.svelte.test.ts +++ b/web/src/routes/layoutWorkspaceReloadOnIdentity.svelte.test.ts @@ -30,6 +30,12 @@ vi.mock('$lib/api/client', () => ({ // `goto` must not run: the unauthenticated branches call it, and jsdom has no // navigation. Nothing here asserts on it. +const identityReload = vi.hoisted(() => ({ + reloadForIdentityChange: vi.fn(), + clearPersistentIdentityState: vi.fn(), +})); +vi.mock('$lib/stores/identityReload.svelte', () => identityReload); + vi.mock('$app/navigation', () => ({ goto: vi.fn(), beforeNavigate: () => {}, @@ -136,3 +142,90 @@ describe('BUG-2991: the root layout refetches workspaces when the user changes', expect(workspaceStore.workspaces).toEqual([U2_WS]); }); }); + +describe('BUG-3005: which identity transitions reload the tab', () => { + // The reload exists so one user's data cannot be visible to the next. Which + // transitions need it is therefore a question about whose data is being + // dropped, and an ANONYMOUS baseline holds nobody's private data — so a + // sign-IN does not reload. Reloading there also put a full page load in the + // middle of the login flow, racing its own navigation; the E2E suite caught + // that, which is why these three legs exist. + // + // EACH LEG ASSERTS AN EXACT CALL COUNT, and that is load-bearing rather than + // style: `afterEach` clears the store, so every test starts from an + // ESTABLISHED empty identity and its own setup performs a sign-in. Removing + // the guard therefore adds a reload during setup and all three legs fail — + // the swap and sign-out ones on the COUNT rather than on the absence. + + it('reloads on a SWAP from one user to another', async () => { + render(Layout, { props: { children: childSnippet } }); + await settle(); + // PRECONDITION: u1 is established, so what follows is a change rather + // than a baseline. + expect(authStore.userId).toBe('u1'); + expect(identityReload.reloadForIdentityChange).not.toHaveBeenCalled(); + + api.auth.session.mockResolvedValue(sessionFor('u2')); + await authStore.load(); + await settle(); + + expect(identityReload.reloadForIdentityChange).toHaveBeenCalledTimes(1); + }); + + it('does not CLEAR-without-reloading on a swap — the reload path owns both', async () => { + // The counterfactual for the split: a swap must not take the sign-out + // branch, or it would clear the storage and then leave the tab standing + // with the previous user's page still mounted and nothing navigating + // away from it. + render(Layout, { props: { children: childSnippet } }); + await settle(); + + api.auth.session.mockResolvedValue(sessionFor('u2')); + await authStore.load(); + await settle(); + + expect(identityReload.clearPersistentIdentityState).not.toHaveBeenCalled(); + }); + + it('CLEARS but does not reload on SIGN-OUT', async () => { + // Both sign-out sites navigate away by themselves — account delete with + // `location.href`, console logout with a hard navigation for the same + // reason — and a reload racing them ABORTS one of the two. That is not + // a hypothesis: `account-delete.spec.ts:147` failed with + // `net::ERR_ABORTED; maybe frame was detached?` and that is how this + // branch learned it. + // + // The clears still have to run: a hard navigation drops the tab's + // memory, not localStorage, sessionStorage or IndexedDB. + render(Layout, { props: { children: childSnippet } }); + await settle(); + expect(authStore.userId).toBe('u1'); + + authStore.clear(); + await settle(); + + expect(identityReload.clearPersistentIdentityState).toHaveBeenCalledTimes(1); + expect(identityReload.reloadForIdentityChange).not.toHaveBeenCalled(); + }); + + it('does NOT reload on a SIGN-IN from an unauthenticated tab', async () => { + // The leg the E2E failure is about. `collab-helpers.ts` signs in from + // inside a mounted page, and a reload there takes the page out from + // under whatever is driving it — in a test, and in the real login flow. + api.auth.session.mockResolvedValue(null); + render(Layout, { props: { children: childSnippet } }); + await settle(); + // PRECONDITION: the tab is unauthenticated and that baseline is + // established, so the sign-in below IS a transition and this leg is not + // passing because nothing fired. + expect(authStore.userId).toBe(''); + + api.auth.session.mockResolvedValue(sessionFor('u1')); + await authStore.load(); + await settle(); + + expect(authStore.userId).toBe('u1'); + expect(identityReload.reloadForIdentityChange).not.toHaveBeenCalled(); + expect(identityReload.clearPersistentIdentityState).not.toHaveBeenCalled(); + }); +});