From 57820422a2fe8da7aeb730ec7f266c28a2e62816 Mon Sep 17 00:00:00 2001 From: xarmian Date: Thu, 10 Sep 2026 19:42:26 -0400 Subject: [PATCH] fix(web): four singleton stores kept the previous user's data across an identity change (BUG-3005) (#1325) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(web): an ORDERED identity epoch, so a store can tell "still me" from "me again" (BUG-3005, unit 1) Four singleton stores hold per-user state and none of them can currently ask the question they need to ask when a response comes back: was this issued under the identity that is signed in now. The tree's existing answer is a captured user id compared at settle (`workspace.svelte.ts`). That answers "is this a DIFFERENT user", which is a weaker question and comes apart on sign out of A, in as B, back in as A: a request issued during the first A session settles, compares equal, and commits data from a session that has ended. An ordinal cannot be confused that way — the epoch it captured is strictly lower than the one it settles into. `authStore.identityEpoch` bumps on exactly the transitions `onIdentityChange` already fires on, and `identityFence()` is the capture-and-compare helper, so four stores do not hand-roll it four ways. That is not a hypothetical: this store and `workspace.svelte.ts` hand-rolled the same three parts of a single-flight loader and drifted three times in one afternoon, which is why `singleFlight.ts` exists. Two orderings are load-bearing and each has a test: - 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 existed has nothing stale to refuse. - Bumped BEFORE the listeners, not after. A listener's expected shape is "drop state, reload it", and that reload captures a fence while the listener is still running — under the old epoch it would refuse its own response. Distinct from `generation`, 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 it, so it cannot serve as the epoch. Five tests. Mutants: fence on the user id instead of the epoch (2 fail, including the A->B->A straggler this exists for), bump after the listeners (1 fail, alone), bump on the baseline too (3 fail). Full web suite 2416 passed. No consumer yet — the stores are unit 2. Nothing changes behaviour on its own. * fix(web): starredStore drops the previous user's stars, and refuses a settle from a session that has ended (BUG-3005, unit 2a) `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, because they fail independently: - an `authStore.onIdentityChange` subscription clears the store when nothing is racing at all, which is the common case; - an identity fence on every settle, for the case where something is. Both arms of `load` commit — the catch arm publishes an empty set and flips `loaded` — so fencing only the success path would leave the store telling B that B's stars are loaded. The navigation fence stays and is not subsumed: a workspace switch under one identity moves no epoch, and an account swap moves no workspace. FIVE TESTS, AND ONE OF THEM TOOK THREE TRIES TO BE WORTH ANYTHING. The toggle-revert mutant SURVIVED twice: - first because the identity-change reset blanks `currentWs`, so the workspace fence was doing the work — fixed by having B sign back into the SAME workspace before the revert fires; - then because the test reverted a failed STAR, which is a delete, and a delete is invisible in an empty set. The write that actually leaks is the revert of a failed UNSTAR, which is an ADD into the next user's set. Both are recorded in the test's own comment, because "this assertion is pointed the wrong way" is not visible from a green run. Mutants now: drop the subscription (2 fail), drop the fence on load's success arm (1), on its catch arm (1), on the toggle revert (1, and only against the corrected test). All 291 store tests pass. * fix(web): collectionStore drops the previous user's collections and items, and fences every settle (BUG-3005, unit 2b) `collections`, `items`, `itemsWorkspace`, `collectionsWorkspace` and `activeItem` all belong to a user and none of them 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 — Roles, the item picker and the editor's link gate then render A's collection names and item titles inside B's session. The store's only fence was `isLatest`, which is a NAVIGATION fence and cannot see this: a same-route account swap supersedes nothing, so A's load is still the latest one for that workspace and commits. Both halves, as with starredStore: - an `onIdentityChange` subscription clearing the store, for the ordinary case where nothing is racing; - an identity fence captured before the first await on `loadCollections`, `loadItems` (both arms — they assign through different branches and stamp `itemsWorkspace` differently) and `loadItem`. `loadItem` returns NULL rather than the fetched item when the identity moved. 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 with `activeItem` untouched. The FRESHNESS STAMPS clear with the arrays, and that is load-bearing rather than tidiness. Emptying `collections` while leaving `collectionsWorkspace` set makes `collectionsAreFreshFor(ws)` answer true for an empty list — the one answer that stops TASK-2200's recovery from re-fetching, so the new user's sidebar would stay permanently empty. It has its own test and its own mutant. `loadItems` and `loadItem` have no production callers today (the item list is maintained through addItem/updateItemInList/removeItem from SSE, and `activeItem` through setActiveItem). They are fenced anyway: they are public API and this item names exactly that state as retained. Seven tests, six mutants, five of them dying to exactly one test: drop the subscription (2 fail), drop the fence on loadCollections (1), on each items arm (1 each), make loadItem return the item anyway (1), leave the stamps set in clear() (1). Full web suite 2428 passed. Claude-Session: https://claude.ai/code/session_0115vpkjH5mmFdjRdRmZRuZt * fix(web): localIndex and localSearch drop every workspace when the signed-in user changes (BUG-3005, unit 2c) `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 previous user's rows in RAM — and `localSearch`, whose only reset callers live inside `localIndex`, kept a full-text index built from their item titles and bodies. That fourth store is not named on the item; the enumeration this unit re-ran found it, which is what CONVE-18 asks for. The per-workspace shape is the other half. `reset(ws)` names one slug, so the sign-out path dropped the workspace the layout happened to be looking at and a tab that had visited two kept the other. `localIndex.resetAll()` sweeps every workspace it holds and then calls `localSearch.resetAll()`. The second call is not redundant with the per- workspace resets it just performed: the two maps are keyed independently and nothing keeps them in step, so an index whose workspace state is already gone survives a sweep driven from localIndex's keys alone. It has its own test and its own mutant. 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 nothing changed within this tab so nothing fired. Four tests, three mutants: drop the subscription (3 fail), sweep only the first workspace (1, alone), drop the localSearch.resetAll belt (1, alone). Full web suite 2432 passed; svelte-check 0 errors. Claude-Session: https://claude.ai/code/session_0115vpkjH5mmFdjRdRmZRuZt * fix(web): the store fix routed the starred PAGE around its own filter, and three more singletons held user state (BUG-3005, codex round 1) Round 1's largest finding is that the four stores were not the population, and the sharpest instance is one my own fix made worse. THE STARRED PAGE. `items` is `starredStore.loaded ? filter(fetchedItems) : fetchedItems` — an unfiltered fallback for the pre-load window. The store's identity reset sets `loaded = false`, so the fix routed the page around its only filter and handed B the full `fetchedItems` array it had fetched as A. Its load effect is keyed on the workspace slug, so a same-route swap started no reload to correct it. The page now drops its own copy on the identity signal, invalidates any load in flight, and reloads; its settle carries an identity fence beside the `seq` one, which is a navigation fence and does not move on an account swap. THREE MORE STORES, none of which looks like per-user state to a sweep for per-user FIELDS: - `adminStore.stats` — instance-wide DATA, but authorization-scoped: only an admin can read it, and it must not outlive the admin who did. - `titleStore` — its parts are retired by a PATH stamp, and an account swap moves no path, so A's workspace name and item ref stayed in the browser tab. Cosmetic, and cheap enough that leaving one member of a class unfixed costs more than fixing it. - `editorStore` — `dirty` and `saveStatus` are read by GUARDS, not displayed: A's dirty editor makes B's unsaved-draft prompt and SSE archive suppression act on a document B never opened. Needs a WIDER clear than `resetForDoc`, which keeps `saveStatus` on purpose because it handles a document change within one session; an identity change ends the session. Its own test. AND ONE FALSE CLAIM OF MINE. Unit 2b's message said every post-await write in collectionStore was fenced. `finally { loading = false }` was not, so A's load settling under B's in-flight one cleared B's flag and could flash empty-state mid-load. Fenced, with `clear()` now owning the flag so nothing is stuck true. THREE THINGS DOCUMENTED RATHER THAN CHANGED, each with the reasoning in the code: - The baseline exemption's residual. A request issued before `/auth/session` resolves settles under a fence that still says current, so a cross-tab identity change inside that window commits. It cannot be closed by refusing pre-baseline settles: the root layout issues the workspace list CONCURRENTLY with the session — that concurrency is why `identityEstablished` exists — so refusing them empties the app on every cold start. Nothing inside the tab can tell the two cases apart. - The contract bumping-before-listeners places on listeners: a fence captured inside a listener passes, so a listener must reload for the identity signed in NOW. Every listener here clears and either stops or reloads from the current route. - The durable wipe `resetAll` inherits from `reset`. Not needed for isolation (the cache is namespaced per user+workspace) and it costs a returning user their warm cache, but it is what sign-out has done since TASK-1360 and leaving a signed-out user's item titles on a shared browser's disk is the wrong default to adopt silently. What this widens is the pre-existing race between a fast re-login's hydrate and a wipe still in flight; that is a persistence-layer fix, not a change to this sweep. Six new tests. Five mutants, each dying to exactly one: unfence `loading`, drop the admin subscription, drop the title subscription, use `resetForDoc` instead of `clear` for the editor, drop the starred page's subscription. The admin test loads REAL stats through a stubbed fetch first — asserting null before and after would have passed against a store with no clear at all, which is the failure this branch has already produced once. Full web suite 2438 passed; svelte-check 0 errors. Claude-Session: https://claude.ai/code/session_0115vpkjH5mmFdjRdRmZRuZt * fix(web): a reset that dropped the data but not the in-flight slot, plus the layout that outlived the identity (BUG-3005, codex round 2) Round 2 found four defects, one of which my round-1 reset CREATED, and the lead's three folded rows land with them. THE SINGLE-FLIGHT SLOT — the reset was actively harmful. `clear()` dropped the collections but left the previous identity's promise published, so 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 there to prevent. `invalidate()` on the keyed single-flight abandons the slot AND bumps the sequence; `workspaceStore` gets the same line, because it is the same defect one file away and BUG-2991 did not have to be filed for it to be true there too. THE WORKSPACE LAYOUT outlived the identity. Its per-workspace effect is keyed on the slug, so a same-route swap re-ran nothing and the layout kept every connection it made for the previous user. Two halves: - the SSE stream is a leak rather than staleness. `sseService.connect` is idempotent per workspace, so B's tab kept the EventSource A opened — 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. Explicit `disconnect()` first. BUG-3011 tracks the server half. - fourteen leaf route components own user-scoped state and key their reloads on the slug. `{#key authStore.identityEpoch}` remounts them structurally rather than fixing fourteen pages and leaving the fifteenth to be remembered. AND THE REMOUNT SHIPPED BROKEN FIRST. `identityEpoch` was a plain `let`, so the template never saw it change and the block never re-ran. Nothing about the code looked wrong; a test that counted mounts is what found it. It is `$state` now, with the reason in the comment, because it is the only value in that file read from a template. Also from round 2: `adminStore.loadStats` had no fence, so A's statistics landed after the clear; `clear()` set `loading = true` and the admin layout loads only on mount, which pinned the console on "Loading admin data…" forever; and the starred page reloaded its private copy but not the shared store, so every OTHER view showed all items unstarred. MY BASELINE COMMENT WAS FACTUALLY WRONG and codex was right to call it. I wrote that the root layout issues the workspace list concurrently with /auth/session — taken from the neighbouring comment rather than read from the layout. `loadAll()` is gated on `authReady`, and no children render until then, so nothing can issue a fenced request before the baseline. The residual is therefore currently UNREACHABLE rather than narrow, and the exemption stays for a different reason: the bump and the listener notification are one event, and firing listeners on the baseline is a live hazard. Corrected in place, with what is actually true. The lead's three folded rows: SSE (above), `toastStore` (the tray is a readable log of A's actions — toast text names items), and the editor's attachment-metadata memo, cleared from the ROOT layout rather than by self-subscribing, because that module is deliberately rune-free and importing the auth store breaks every `node`-project test with "$state is not defined" before an assertion runs. Eight new tests. Mutants, each dying to exactly one test: no flight invalidation, invalidate without the sequence bump (pinned by an A->B->A leg in `workspaceStore`, where the captured user id AGREES and the bump is the only guard left), unfenced `loadStats`, toast history kept, the starred page not reloading the store, no SSE disconnect, no remount. Full web suite 2446 passed; svelte-check 0 errors. Claude-Session: https://claude.ai/code/session_0115vpkjH5mmFdjRdRmZRuZt * fix(web): stop enumerating surfaces — an identity change clears what a reload cannot drop, then reloads (BUG-3005, codex round 3 + lead ruling) 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. The lead's ruling, and it is the right one: reload. `reloadForIdentityChange` clears the state a reload does NOT drop — the palette's recent searches in localStorage, collab cursors in sessionStorage, the attachment-metadata memo — and then reloads. What remains an enumeration is bounded by STORAGE KEYS rather than by every module in the app, and a grep for the storage APIs can check it. The clears run FIRST and that ordering has its own test: reloading first tears the page down before they execute. REMOVED, because one mechanism is the point: the `{#key}` remount added in round 2, and the starred page's own listener. The starred page had three reload paths on a single identity change. The remount's absence is pinned by a test rather than left implicit, so re-adding it has to be deliberate. KEPT, because none of it is redundant with the reload: the epoch, the four store fences, and the SSE disconnect. 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. The SSE close bounds that window to zero for a stream that would otherwise stay authorized — signing in as B replaces the cookie without destroying A's session row, which is the case BUG-3007's server-side close cannot see (BUG-3011). THE TWO WRITES NEED THEIR OWN RULE, AND THE RELOAD MAKES THEM WORSE rather than better. A teardown that PERSISTS must discard when the identity moved, because the reload's unload fires those handlers under the NEW cookie: - `ItemDetail`'s raw-markdown saver flushes pending edits with `keepalive: true` from three sites, a request deliberately built to outlive the page. It now refuses when the epoch differs from the one captured when the item was LOADED — the identity whose typing the editor contains. - `openChildrenDialog` holds the fetched 409 child list AND continuations that perform the mutation on confirm. It is mounted in the root layout, outside everything the workspace layout controls. All pending requests now resolve FALSE, so the producer takes its cancel path; rejecting would surface unhandled rejections at call sites that only ever expected a boolean. Grepped the class (`keepalive: true`): three sites, all funnelling through the one saver callback that now carries the guard. MY BASELINE COMMENT WAS WRONG A SECOND TIME. Round 2 corrected it to "no child renders before authReady"; round 3 found the share-page branch sets `authReady` WITHOUT calling `authStore.load()`, and the auth-failure path proceeds deliberately. So the residual is narrow, not unreachable. Corrected again, and this time the exceptions are named rather than the rule restated. Nine tests across the new mechanism; every mutant dies to exactly one: reload before clear, no palette clear, a collab clear that takes the whole of sessionStorage, the dialog abandoning only its active entry, no SSE disconnect, a reinstated remount. Full web suite 2450 passed; svelte-check 0 errors. Claude-Session: https://claude.ai/code/session_0115vpkjH5mmFdjRdRmZRuZt * fix(web): the identity reload was firing the teardown writes it was meant to outrun (BUG-3005, codex round 4) Round 4 found two P1s, and both are defects the RELOAD created — which is the shape of this whole unit by now: each fix's own mechanism becomes the next round's hazard. THE UNLOAD HANDLER WAS PERSISTING UNDER THE NEW COOKIE. A reload fires `beforeunload`, and both persistence paths there use `keepalive` — a request deliberately built to outlive the page. Round 3's guard covered the raw saver only. The collab flush was unguarded, and MY GREP FOR THE CLASS MISSED IT: I grepped `keepalive: true`, and that call passes the flag positionally as `flushNow(ctx, true)`. An enumeration by literal, in the unit whose entire lesson is that enumerations by shape are wrong. THE UNLOAD HANDLER COULD ALSO CANCEL THE RELOAD. `preventDefault()` on a dirty editor raises the native "unsaved changes" prompt, and "Stay" cancels the navigation. The identity has already changed and the stores are already cleared, so the answer would be the new user sitting in the previous user's page shell — with the `{#key}` remount removed in round 3, nothing left to rebuild it. The prompt protects the author's unsaved work, and after an identity change the author is not the one sitting there. Both close the same way: when the epoch differs from the one captured at LOAD, the handler returns before it flushes or prompts. The $effect cleanup is a third door into the same write and is gated too. Also from round 4: `pad-last-route-` and `pad-last-scroll--` are keyed by WORKSPACE with no user in them. The scroll key embeds the pathname in the KEY, so a private item slug is legible from a key listing without reading a single value. Both are cleared now, and the test asserts an ordinary UI preference SURVIVES, because a clear that took all of localStorage would be a different and undocumented change. ACCEPTED, NOT FIXED, with the reasoning: `persistWipe` is fire-and-forget, so an IndexedDB database can outlive the reload if its delete has not landed. It is residual data on disk rather than a cross-user read — the databases are named `pad-local-index-{userId}-{workspace}`, so the next user cannot open the previous one's — and making the reload await an unbounded IDB delete trades a privacy-hygiene residual for a tab that may not reload at all. Six tests. The ItemDetail ones are SOURCE guards, the pattern this directory already uses for that file (~7,900 lines of collab, SSE and pane wiring), and the header says what a source guard cannot do rather than leaving it implied. Mutants: remove the beforeunload guard, remove the collab cleanup gate, remove the route/scroll clear — each dies to exactly one test. Full web suite 2456 passed; svelte-check 0 errors. Claude-Session: https://claude.ai/code/session_0115vpkjH5mmFdjRdRmZRuZt * fix(web): the reload now happens even if a clear throws, and a test that passed for the wrong reason (BUG-3005, codex round 5) No P1s. Two P2s and a P3, all addressed. THE RELOAD IS EXCEPTION-SAFE NOW. `clearPersistentIdentityState()` ran outside any try/finally, so a throw on the way through would have skipped `location.reload()` entirely — leaving 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. Latent today (the only uncaught call is a `Map.clear()`), and the ordering is now structural rather than incidental. A TEST THAT REPORTED PASSED WHILE THE PAGE CRASHED. The starred-page leg used `mockReturnValueOnce` with no default, so a later call resolved `undefined`, `items.map` threw, and vitest showed a green test with an unhandled error beside it — the assertion was reading a page that had crashed rather than a fence that had refused. Every mock carries a default now, `vi.resetModules()` is gone (it handed the test a different authStore instance than the component closed over), and there is a counterfactual leg where the identity holds still and the data DOES render. AND THE SAME LEG'S MUTANT SURVIVES, which is in the file rather than left for the next reader to discover. Removing the page's identity fence leaves it green: the page's own `$effect` re-runs during the transition and advances `loadSeq`, so the NAVIGATION fence refuses A's settle first and the identity fence never decides anything there. The leg stands as 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. Recorded because a surviving mutant filed as a passing test is exactly how this file already shipped one assertion pointed the wrong way. P3: a trailing blank line `git diff --check` flagged. Full web suite 2457 passed, no unhandled errors; svelte-check 0 errors; `git diff --check` clean. Claude-Session: https://claude.ai/code/session_0115vpkjH5mmFdjRdRmZRuZt * fix(web): a sign-in from an unauthenticated tab must not reload (BUG-3005, E2E failure + lead ruling) E2E failed on 3aebbb58 and the suite was right. `web/e2e/lib/collab-helpers.ts` signs in by fetching `/api/v1/auth/login` FROM INSIDE a mounted page, which is an identity change from `''` to a real user — and the reload took the page out from under Playwright. The narrowing is correct on its own terms, which is why it lands whatever the log finally says. The reload exists so one user's data cannot be visible to the next, so which transitions need it is a question about WHOSE data is being dropped: an anonymous baseline holds nobody's private data. `user -> user` and `user -> ''` reload; `'' -> user` does not. It also takes a full page load out of the primary login path, where it was racing the login flow's own navigation. That cost was never intended and would have shipped unnoticed if the E2E suite had not been driving a real login. `onIdentityChange` listeners now receive the PREVIOUS established user id. Every other listener ignores it and drops state as before; only the root layout needs to tell a sign-in from a sign-out or a swap, and reading it from the signal is better than the alternative of a second store field that can disagree with the one the notification was computed from. Three legs, and the mutant that removes the guard kills all three — the swap and sign-out ones on the exact CALL COUNT rather than on an absence, because `afterEach` clears the store so every test starts from an established empty identity and its own setup performs a sign-in. That is written into the test file, since a leg failing for a reason other than its name is how a suite stops meaning what it says. Full web suite 2460 passed; svelte-check 0 errors. Claude-Session: https://claude.ai/code/session_0115vpkjH5mmFdjRdRmZRuZt * fix(web): sign-out clears and lets its own navigation leave; only a swap reloads (BUG-3005, E2E account-delete failure) The E2E failure was `account-delete.spec.ts:147` with `net::ERR_ABORTED; maybe frame was detached?` and `Navigation to /console/settings is interrupted by another navigation to /login`. A NAVIGATION RACE, not a rule about identity transitions — and not the sign-in I had guessed at before the log was readable. Recording that because the previous commit's narrowing was proposed on a wrong diagnosis; it is right on its own terms, and it was not this. Both sign-out sites already navigate away by themselves. Account deletion does `authStore.clear()` then `window.location.href = '/login'`, deliberately and with a comment saying so. My listener fires inside `clear()` and started a reload of the current URL; the delete flow then set `location.href`. Two navigations, one aborts. So the split, per the lead's ruling: - SWAP (user -> user): reload. Nothing else navigates, so the reload is the whole mechanism. - SIGN-OUT (user -> ''): run the persistent clears and return. The clears still have to happen — a hard navigation drops the tab's memory, not localStorage, sessionStorage or IndexedDB — but the reload is redundant with a navigation that is already underway, and racing it is what broke the flow. - and the console logout now navigates HARD. `goto('/login')` is an SPA navigation that leaves every singleton, component and cache where it was, which is this bug's original defect arriving through the sign-out path itself. Account deletion has always done this; the two sites agree now. The property is unchanged: every transition away from a real user drops that user's in-tab state, by reload on a swap and by hard navigation on a sign-out. Four legs in the root-layout test (swap reloads and does NOT take the clear-only branch; sign-out clears and does NOT reload; sign-in does neither) and a source guard on the console logout, since jsdom does not perform an assignment to `window.location`. Mutants: sign-out reloading again, sign-out doing neither, and the logout reverting to `goto` — each dies. Full web suite 2464 passed; svelte-check 0 errors; `git diff --check` clean. Claude-Session: https://claude.ai/code/session_0115vpkjH5mmFdjRdRmZRuZt --- web/src/lib/collab/wsProvider.svelte.ts | 5 +- .../components/editor/attachment-metadata.ts | 34 ++++ .../lib/components/items/ItemDetail.svelte | 53 ++++- ...tailIdentityDiscardsTeardownWrites.test.ts | 81 ++++++++ .../timeline/ItemTimeline.svelte.test.ts | 12 +- web/src/lib/stores/admin.svelte.ts | 39 +++- web/src/lib/stores/auth.svelte.ts | 136 ++++++++++++- .../stores/authIdentityEpoch.svelte.test.ts | 117 +++++++++++ web/src/lib/stores/collections.svelte.ts | 83 +++++++- .../collectionsIdentityReset.svelte.test.ts | 191 ++++++++++++++++++ web/src/lib/stores/editor.svelte.ts | 23 +++ .../lib/stores/identityReload.svelte.test.ts | 132 ++++++++++++ web/src/lib/stores/identityReload.svelte.ts | 110 ++++++++++ .../stores/identityResetRound1.svelte.test.ts | 167 +++++++++++++++ .../stores/identityResetRound2.svelte.test.ts | 188 +++++++++++++++++ web/src/lib/stores/localIndex.svelte.ts | 52 +++++ .../localIndexIdentityReset.svelte.test.ts | 130 ++++++++++++ web/src/lib/stores/localSearch.svelte.ts | 16 ++ .../lib/stores/openChildrenDialog.svelte.ts | 37 +++- web/src/lib/stores/singleFlight.ts | 22 ++ web/src/lib/stores/starred.svelte.ts | 35 +++- .../starredIdentityReset.svelte.test.ts | 155 ++++++++++++++ web/src/lib/stores/title.svelte.ts | 10 + web/src/lib/stores/toast.svelte.ts | 26 ++- web/src/lib/stores/workspace.svelte.ts | 8 + web/src/routes/+layout.svelte | 46 ++++- .../[username]/[workspace]/+layout.svelte | 21 ++ .../[workspace]/starred/+page.svelte | 15 +- .../starredPageIdentityReset.svelte.test.ts | 150 ++++++++++++++ .../workspaceLayoutIdentitySse.svelte.test.ts | 158 +++++++++++++++ web/src/routes/console/+layout.svelte | 9 +- .../consoleLogoutHardNavigates.test.ts | 44 ++++ ...utWorkspaceReloadOnIdentity.svelte.test.ts | 93 +++++++++ 33 files changed, 2374 insertions(+), 24 deletions(-) create mode 100644 web/src/lib/components/items/itemDetailIdentityDiscardsTeardownWrites.test.ts create mode 100644 web/src/lib/stores/authIdentityEpoch.svelte.test.ts create mode 100644 web/src/lib/stores/collectionsIdentityReset.svelte.test.ts create mode 100644 web/src/lib/stores/identityReload.svelte.test.ts create mode 100644 web/src/lib/stores/identityReload.svelte.ts create mode 100644 web/src/lib/stores/identityResetRound1.svelte.test.ts create mode 100644 web/src/lib/stores/identityResetRound2.svelte.test.ts create mode 100644 web/src/lib/stores/localIndexIdentityReset.svelte.test.ts create mode 100644 web/src/lib/stores/starredIdentityReset.svelte.test.ts create mode 100644 web/src/routes/[username]/[workspace]/starred/starredPageIdentityReset.svelte.test.ts create mode 100644 web/src/routes/[username]/[workspace]/workspaceLayoutIdentitySse.svelte.test.ts create mode 100644 web/src/routes/console/consoleLogoutHardNavigates.test.ts 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(); + }); +});