From d7df8a4b4b32dbbbd65ce790091f7143176f5949 Mon Sep 17 00:00:00 2001 From: xarmian Date: Thu, 17 Sep 2026 23:14:20 -0400 Subject: [PATCH] fix(web): fence child components' post-await sends against identity change (BUG-3095) (#1400) * wip(BUG-3095): fence rows 1-8 + driven leg (pre-counterfactual checkpoint) * wip(BUG-3095): population guard + gap list (pre-mutation checkpoint) * fix(web): fence child components' post-await requests against identity change (BUG-3095) TimelineVersionCard and ChildItems issue their own requests after awaiting a parent-provided callback or a previous iteration of their own write loop. None of the population held an identity fence: every guard present compares the workspace/item slugs, which do not move when the signed-in USER changes on the same workspace and item. Fixes the eight await-then-send paths in those two files, adds a driven leg for the known member, and lands an AST guard over the whole child population with the remaining six paths enumerated as known gaps. Claude-Session: https://claude.ai/code/session_01JoHaYR8C9wtRnb589CQGC8 * test(web): resolve one-hop local-helper sends in the population guard (BUG-3095) Three independent hand audits of the same population found await-then-send paths this guard reported as having no send at all: the commonest post-await send here is not an api.* call but a call to the component's own loader (loadTimeline, loadChildren). Matching api.* syntactically missed every one -- a false negative, the direction that stays silent. Adds one hop of local-helper resolution, which surfaces eight further paths (ItemTimeline's six comment/reaction handlers, CopyItemDialog.runPreflight, PushToAgentDialog.handleSend); all are enumerated in KNOWN_UNFENCED. Declares depth, commits and non-await suspension as gaps (c)(d)(e). Claude-Session: https://claude.ai/code/session_01JoHaYR8C9wtRnb589CQGC8 * fix(web): fence the attachment delete confirmation against identity change (BUG-3095 row 9) Claude-Session: https://claude.ai/code/session_01JoHaYR8C9wtRnb589CQGC8 --------- Co-authored-by: xarmian --- web/src/lib/components/ChildItems.svelte | 99 ++- .../items/ItemAttachmentStrip.svelte | 28 +- ...tachmentDeleteIdentityFence.svelte.test.ts | 243 ++++++++ .../childIdentityFencePopulation.test.ts | 575 ++++++++++++++++++ .../items/itemDetailIdentityFenceAst.test.ts | 28 +- .../timeline/TimelineVersionCard.svelte | 48 +- ...ineVersionCardIdentityFence.svelte.test.ts | 243 ++++++++ 7 files changed, 1230 insertions(+), 34 deletions(-) create mode 100644 web/src/lib/components/items/attachmentDeleteIdentityFence.svelte.test.ts create mode 100644 web/src/lib/components/items/childIdentityFencePopulation.test.ts create mode 100644 web/src/lib/components/timeline/timelineVersionCardIdentityFence.svelte.test.ts diff --git a/web/src/lib/components/ChildItems.svelte b/web/src/lib/components/ChildItems.svelte index dab814028..2d471b868 100644 --- a/web/src/lib/components/ChildItems.svelte +++ b/web/src/lib/components/ChildItems.svelte @@ -6,6 +6,7 @@ import { syncService } from '$lib/services/sync.svelte'; import { collectionStore } from '$lib/stores/collections.svelte'; import { workspaceStore } from '$lib/stores/workspace.svelte'; + import { authStore } from '$lib/stores/auth.svelte'; import { toastStore } from '$lib/stores/toast.svelte'; import type { Item, Collection, PaneTarget } from '$lib/types'; import { parseFields, parseSchema, formatItemRef } from '$lib/types'; @@ -171,13 +172,31 @@ .filter((i: any) => !i[SHADOW_ITEM_MARKER_PROPERTY_NAME]) .map((item, index) => ({ id: item.id, sort_order: index })); + // IDENTITY fence (BUG-3095) + the workspace this reorder is about, + // both captured before the loop. `wsSlug` was read LIVE inside the loop + // before this change, so a workspace switch mid-loop addressed the + // remaining PATCHes at the new workspace. + const isSameIdentity = authStore.identityFence(); + const reqWs = wsSlug; try { for (const { id, sort_order } of updates) { // HT-2176 Option A (TASK-2172): NO per-PATCH freeze recheck. The // reorder was INITIATED before peeking (the top guard blocks a NEW // one); breaking mid-loop would persist it only partially, leaving // inconsistent sort_orders. Let the initiated reorder finish. - await api.items.update(wsSlug, id, { sort_order }); + // + // IDENTITY is checked per iteration even though FREEZE is not + // (BUG-3095), and the two are not in tension. Option A's argument + // is that a half-applied reorder is worse than a fully-applied one + // the user no longer has permission to make — true while the + // SENDER is the same person throughout. It says nothing about a + // DIFFERENT user: every iteration after the first sends after the + // previous iteration's await, so a sign-out mid-loop would issue + // the remaining writes on the next user's cookie. A partial + // reorder is the lesser harm there, and it is the only outcome + // that keeps the writes attributable to whoever started them. + if (!isSameIdentity()) return; + await api.items.update(reqWs, id, { sort_order }); } } catch (e) { console.error('Failed to persist reorder:', e); @@ -204,11 +223,18 @@ groupData[status] = reordered.map((it, idx) => ({ ...it, sort_order: idx })); const updates = reorderGroup(grp, child.id, dir); + // IDENTITY fence + captured workspace (BUG-3095) — see handleFinalize + // for why the loop needs a per-iteration check and why that does not + // reopen Option A's partial-reorder argument. + const isSameIdentity = authStore.identityFence(); + const reqWs = wsSlug; try { for (const u of updates) { // Option A (TASK-2172): no per-PATCH freeze recheck — a reorder - // initiated pre-pane finishes fully (see handleFinalize). - await api.items.update(wsSlug, u.item.id, { sort_order: u.sort_order }); + // initiated pre-pane finishes fully (see handleFinalize). Identity + // IS checked per iteration even though freeze is not (BUG-3095). + if (!isSameIdentity()) return; + await api.items.update(reqWs, u.item.id, { sort_order: u.sort_order }); } } catch (e) { console.error('Failed to persist reorder:', e); @@ -225,18 +251,30 @@ let loadSeq = 0; async function loadChildren() { - // Capture the request identity (item + workspace) + sequence BEFORE the - // await. ItemDetail reuses this panel across a no-{#key} item switch (its - // `itemSlug` prop just changes), so a slower A load must NOT overwrite - // B's children — nor fire onChildrenChange with A's data into the - // parent's childItemIds / progress overrides (PLAN-2105 / TASK-2112). + // NAVIGATION fence: capture the REQUEST identity (item + workspace) + + // sequence BEFORE the await. ItemDetail reuses this panel across a + // no-{#key} item switch (its `itemSlug` prop just changes), so a slower A + // load must NOT overwrite B's children — nor fire onChildrenChange with + // A's data into the parent's childItemIds / progress overrides + // (PLAN-2105 / TASK-2112). const reqSlug = itemSlug; const reqWs = wsSlug; const seq = ++loadSeq; + // IDENTITY fence (BUG-3095): a different SIGNED-IN USER on the same + // workspace and item moves none of the three values above, so without + // this the commit below paints one identity's children into the next + // identity's pane and pushes them into the parent through + // onChildrenChange. + const isSameIdentity = authStore.identityFence(); // `destroyed` guards the {#key itemSlug} teardown → same-slug remount // case: a late load from the old instance must not push stale children // through onChildrenChange into the freshly-mounted parent (Codex). - const stale = () => destroyed || seq !== loadSeq || reqSlug !== itemSlug || reqWs !== wsSlug; + const stale = () => + destroyed || + !isSameIdentity() || + seq !== loadSeq || + reqSlug !== itemSlug || + reqWs !== wsSlug; // BUG-2871: only tear the list down when there is nothing valid to show // for THIS item — a first load or an item switch. A same-item REFRESH // keeps the rendered rows mounted, whether it succeeds or fails. @@ -547,12 +585,23 @@ const title = createTitle.trim(); const collSlug = createCollSlug; if (!title || !collSlug || creating) return; - // DR-6b: capture identity BEFORE the await; bail after it if the - // instance was destroyed (item switch) or the identity moved. + // DR-6b: capture the REQUEST identity (workspace + item) BEFORE the + // await; bail after it if the instance was destroyed or the pane + // navigated. This is a NAVIGATION fence — it was described as an + // identity fence, which it is not (BUG-3095). const reqWs = wsSlug; const reqSlug = itemSlug; const reqId = itemId; - const stale = () => destroyed || reqWs !== wsSlug || reqSlug !== itemSlug || reqId !== itemId; + // IDENTITY fence (BUG-3095): the signed-in USER, which none of the three + // captures above can see. Guards the post-await `loadChildren()` refresh, + // which is a fresh request issued after the create settles. + const isSameIdentity = authStore.identityFence(); + const stale = () => + destroyed || + !isSameIdentity() || + reqWs !== wsSlug || + reqSlug !== itemSlug || + reqId !== itemId; creating = true; try { // DR-1: parent goes ONLY through the `fields` JSON `parent` key. @@ -625,13 +674,22 @@ linkSearching = false; return; } - // DR-6b: per-query sequence + identity fence. + // DR-6b: per-query sequence + NAVIGATION fence (workspace + item). Was + // described as an identity fence; it is not one (BUG-3095). const seq = ++searchSeq; const reqWs = wsSlug; const reqSlug = itemSlug; const reqId = itemId; + // IDENTITY fence (BUG-3095): search results are user-scoped, so a swap + // mid-query would render the previous identity's matches to the next one. + const isSameIdentity = authStore.identityFence(); const stale = () => - destroyed || seq !== searchSeq || reqWs !== wsSlug || reqSlug !== itemSlug || reqId !== itemId; + destroyed || + !isSameIdentity() || + seq !== searchSeq || + reqWs !== wsSlug || + reqSlug !== itemSlug || + reqId !== itemId; linkSearching = true; try { const res = await api.search(q, { workspace: reqWs }); @@ -656,11 +714,20 @@ const cand = confirmCandidate; // Freeze guard (TASK-2172): mirror submitCreate — no reparent while frozen. if (!cand || linking || frozen) return; - // DR-6b: capture identity BEFORE the await. + // DR-6b: capture the REQUEST identity (workspace + item) BEFORE the + // await — a NAVIGATION fence, not an identity one (BUG-3095). const reqWs = wsSlug; const reqSlug = itemSlug; const reqId = itemId; - const stale = () => destroyed || reqWs !== wsSlug || reqSlug !== itemSlug || reqId !== itemId; + // IDENTITY fence (BUG-3095): guards the post-await `loadChildren()` + // refresh, a fresh request issued after the link write settles. + const isSameIdentity = authStore.identityFence(); + const stale = () => + destroyed || + !isSameIdentity() || + reqWs !== wsSlug || + reqSlug !== itemSlug || + reqId !== itemId; linking = true; try { // SOURCE = candidate, TARGET = current item id. Address the source diff --git a/web/src/lib/components/items/ItemAttachmentStrip.svelte b/web/src/lib/components/items/ItemAttachmentStrip.svelte index ab61d8cd2..de169456f 100644 --- a/web/src/lib/components/items/ItemAttachmentStrip.svelte +++ b/web/src/lib/components/items/ItemAttachmentStrip.svelte @@ -40,6 +40,7 @@ */ import { onDestroy, untrack } from 'svelte'; import { api, PadApiError } from '$lib/api/client'; + import { authStore } from '$lib/stores/auth.svelte'; import type { AttachmentListItem } from '$lib/types'; import { iconForAttachment, @@ -217,6 +218,13 @@ att: StripAttachment; anchor: HTMLElement | null; prompt: string; + /** + * IDENTITY fence (BUG-3095 row 9), captured when the menu OPENED and + * checked at the point that sends. It rides on the pending record rather + * than on a component field because it belongs to THIS confirmation: a + * second `×` click replaces the record and must capture afresh. + */ + isSameIdentity: () => boolean; } | null>(null); const uid = $props.id(); @@ -1060,6 +1068,10 @@ att, anchor, prompt: attachmentDeletePrompt(att.filename, referencedIds().has(att.id)), + // Captured HERE, when the menu opens, so the check at confirm time + // compares against the identity that asked for the deletion — not + // against whoever is signed in when the button is pressed. + isSameIdentity: authStore.identityFence(), }; } @@ -1091,12 +1103,26 @@ * returned; an in-app confirmation does NOT block, and the user can switch * item or workspace while it is up. So the fence — and the permission — are * re-checked HERE, at the point that actually sends the request. + * + * THREE checks, answering three different questions (BUG-3095 row 9 added + * the third). `canDelete` is the PERMISSION. `paint.isCurrent()` is the + * NAVIGATION fence: is the strip still showing the workspace and item this + * tile was painted for. `pending.isSameIdentity()` is the IDENTITY fence: is + * the same USER still signed in. + * + * The third is not implied by the other two, and this is the site that shows + * why most sharply. A logout and a different login while the confirmation + * menu is open leaves the workspace and item unchanged, so the navigation + * fence passes — and the DELETE below would go out on the NEW user's cookie, + * for a deletion the previous user asked for. There is no `await` anywhere in + * that window, so no post-await guard anywhere in this file could have caught + * it; the suspension is the user reading a prompt. */ function confirmDelete() { const pending = pendingDelete; pendingDelete = null; if (!pending) return; - if (!canDelete || !paint.isCurrent()) return; + if (!canDelete || !paint.isCurrent() || !pending.isSameIdentity()) return; void performDelete(pending.att); } diff --git a/web/src/lib/components/items/attachmentDeleteIdentityFence.svelte.test.ts b/web/src/lib/components/items/attachmentDeleteIdentityFence.svelte.test.ts new file mode 100644 index 000000000..24902b165 --- /dev/null +++ b/web/src/lib/components/items/attachmentDeleteIdentityFence.svelte.test.ts @@ -0,0 +1,243 @@ +/** + * BUG-3095 row 9 — the DRIVEN leg for `ItemAttachmentStrip`'s delete + * confirmation. + * + * WHY THIS ONE NEEDED ITS OWN LEG, and why the population guard could never + * have found it. Every other member of this surface suspends on an `await`. + * This one suspends on a HUMAN: `requestDelete` opens a non-blocking in-app + * confirmation menu and returns, and `confirmDelete` sends the DELETE when the + * user presses the destructive row. There is no `await` anywhere between those + * two points, so no post-await analysis — the guard's whole model — applies. + * + * The site already understood the shape and got the quantity wrong. Its comment + * reasons it out correctly ("`window.confirm` blocked the thread … so the fence + * — and the permission — are re-checked HERE, at the point that actually sends + * the request"), but the re-check was `paint.isCurrent()`, which compares + * `{ws, item}`. A logout and a different login while the menu is open moves + * NEITHER, so the navigation fence passes and the DELETE goes out on the new + * user's cookie — a destructive call, for a deletion the previous user asked + * for. + * + * The auth double is local to this file rather than the family's shared one, + * because the sibling suite `ItemAttachmentStrip.svelte.test.ts` deliberately + * does NOT mock `authStore`: it uses the real store, whose epoch never moves + * under test, so `identityFence()` is inert there and every existing assertion + * is unaffected. Mocking it here keeps that true. + */ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { flushSync, mount, unmount } from 'svelte'; +import type { AttachmentListResponse } from '$lib/types'; + +const listMock = + vi.fn<(ws: string, filters: Record) => Promise>(); +const deleteMock = vi.fn<(ws: string, id: string) => Promise>(); + +// Declared via `vi.hoisted` rather than as a top-level class: `vi.mock`'s +// factory is hoisted above every module-scope binding, so a plain `class` +// referenced from inside it throws `Cannot access 'FakeApiError' before +// initialization` at mock time. +const { FakeApiError } = vi.hoisted(() => ({ + FakeApiError: class extends Error { + code: string; + constructor(code: string) { + super(code); + this.code = code; + } + }, +})); + +vi.mock('$lib/api/client', () => ({ + PadApiError: FakeApiError, + api: { + attachments: { + list: (ws: string, filters: Record) => listMock(ws, filters), + downloadUrl: (ws: string, id: string, variant?: string) => + `/api/v1/workspaces/${ws}/attachments/${id}${variant ? `?variant=${variant}` : ''}`, + delete: (ws: string, id: string) => deleteMock(ws, id), + }, + }, +})); + +const auth = vi.hoisted(() => { + let epoch = 0; + return { + get identityEpoch() { + return epoch; + }, + get userId() { + return 'u1'; + }, + get user() { + return { id: 'u1', name: 'A' }; + }, + get authenticated() { + return true; + }, + /** What the real `notifyIdentityChange` does to the epoch. */ + moveIdentity() { + epoch += 1; + }, + reset() { + epoch = 0; + }, + identityFence() { + const captured = epoch; + return () => epoch === captured; + }, + /** + * Required because `toast.svelte.ts` subscribes at MODULE LOAD, and the + * strip imports it. A double missing this throws before any test runs — + * so the double has to model the store's subscription surface, not just + * the one method under test. + */ + onIdentityChange(_fn: (previousUserId: string) => void) { + return () => {}; + }, + }; +}); +vi.mock('$lib/stores/auth.svelte', () => ({ authStore: auth })); + +import ItemAttachmentStrip from './ItemAttachmentStrip.svelte'; + +const ATT = { + id: 'att-1', + filename: 'notes.pdf', + mime_type: 'application/pdf', + size_bytes: 1024, + created_at: '2026-07-20T00:00:00Z', + item_id: 'item-1', +}; + +let target: HTMLElement; +let instance: ReturnType | null = null; + +function deleteButtons(): HTMLButtonElement[] { + return Array.from(target.querySelectorAll('.att-delete')); +} +function confirmRow(label: string): HTMLElement | undefined { + return Array.from(document.querySelectorAll('[role="menu"] [role="menuitem"]')).find( + (el) => el.querySelector('.mi-label')?.textContent?.trim() === label + ); +} + +async function settle() { + await Promise.resolve(); + await Promise.resolve(); + flushSync(); +} + +beforeEach(() => { + listMock.mockReset(); + deleteMock.mockReset(); + deleteMock.mockResolvedValue(undefined); + auth.reset(); + listMock.mockResolvedValue({ + attachments: [ATT], + total: 1, + } as unknown as AttachmentListResponse); + target = document.body.appendChild(document.createElement('div')); +}); + +afterEach(() => { + if (instance) unmount(instance); + instance = null; + target.remove(); + document.querySelectorAll('[role="menu"]').forEach((n) => n.remove()); +}); + +async function mountAndOpenConfirm() { + instance = mount(ItemAttachmentStrip, { + target, + props: { + wsSlug: 'ws', + username: 'dave', + itemId: 'item-1', + canDelete: true, + itemContent: null, + liveContent: null, + hostToken: 'host-1', + parentArchived: false, + mutationsEnabled: true, + } as Record, + }); + flushSync(); + await settle(); + const btns = deleteButtons(); + // PRECONDITION for every absence assertion below: the delete control is + // actually reachable. Without this, a strip that rendered no tile at all + // would make "no DELETE was sent" pass against the defect. + expect(btns.length).toBeGreaterThan(0); + btns[0].click(); + flushSync(); + expect(confirmRow('Delete file')).toBeTruthy(); +} + +describe('ItemAttachmentStrip delete confirmation identity fence (BUG-3095 row 9)', () => { + it('CONTROL: confirming with the identity unchanged DOES send the DELETE', async () => { + await mountAndOpenConfirm(); + confirmRow('Delete file')!.click(); + flushSync(); + await settle(); + expect(deleteMock).toHaveBeenCalledWith('ws', 'att-1'); + }); + + it('an identity change while the confirmation is OPEN refuses the DELETE', async () => { + await mountAndOpenConfirm(); + + // The window: the menu is up, the user is replaced, and then the + // destructive row is pressed. + auth.moveIdentity(); + flushSync(); + + confirmRow('Delete file')!.click(); + flushSync(); + await settle(); + + expect(deleteMock).not.toHaveBeenCalled(); + }); + + it('the refusal is the IDENTITY fence, not the navigation fence', async () => { + // Neither the workspace nor the item moves in this test, so + // `paint.isCurrent()` is true throughout. If the DELETE is still refused, + // the only check that could have refused it is the identity one. Without + // this leg, a green above would be consistent with the pre-existing + // navigation fence having done the work — which would leave the real bug + // open, since that fence provably cannot see a user swap. + await mountAndOpenConfirm(); + + const epochBefore = auth.identityEpoch; + auth.moveIdentity(); + flushSync(); + expect(auth.identityEpoch).not.toBe(epochBefore); + + confirmRow('Delete file')!.click(); + flushSync(); + await settle(); + + expect(deleteMock).not.toHaveBeenCalled(); + }); + + it('the fence is captured per confirmation, not once per component', async () => { + // A second `×` click replaces the pending record and must capture afresh. + // If the predicate were a component-level field captured at mount, an + // identity change would poison every later confirmation for the life of + // the strip — including legitimate ones by the new user. + await mountAndOpenConfirm(); + auth.moveIdentity(); + flushSync(); + confirmRow('Delete file')!.click(); + flushSync(); + await settle(); + expect(deleteMock).not.toHaveBeenCalled(); + + // Now the NEW identity opens its own confirmation and presses it. + const btns = deleteButtons(); + expect(btns.length).toBeGreaterThan(0); + btns[0].click(); + flushSync(); + confirmRow('Delete file')!.click(); + flushSync(); + await settle(); + expect(deleteMock).toHaveBeenCalledWith('ws', 'att-1'); + }); +}); diff --git a/web/src/lib/components/items/childIdentityFencePopulation.test.ts b/web/src/lib/components/items/childIdentityFencePopulation.test.ts new file mode 100644 index 000000000..f95b70602 --- /dev/null +++ b/web/src/lib/components/items/childIdentityFencePopulation.test.ts @@ -0,0 +1,575 @@ +/** + * BUG-3095 — the CHILD POPULATION guard (surface 8 of the BUG-3084 family). + * + * ItemDetail's own fences are gated by `itemDetailIdentityFenceAst.test.ts`. + * That guard stops at ItemDetail's file boundary, which is the whole reason + * this surface exists: ItemDetail hands callbacks down, and a child that awaits + * one and then issues its OWN request is invisible from inside the parent. + * + * WHAT THIS GUARD ASSERTS, in two independent halves. + * + * 1. THE POPULATION IS THE ONE WE REVIEWED. `POPULATION` below is checked + * against the components ItemDetail actually imports. This half exists + * because the hand-written population table on BUG-3095 was probed at + * `ea0d0848` and had silently drifted by the time it was worked: six + * children had been added to ItemDetail that the table did not list, and one + * row named a component ItemDetail no longer mounts. A table nobody can tell + * is stale is worse than no table, because it is read as coverage. A child + * added to ItemDetail now fails HERE, naming itself, before anyone has to + * notice. + * + * 2. EVERY AWAIT-THEN-REQUEST PATH HOLDS AN IDENTITY FENCE. For each population + * file, every async function (and every `.then` continuation) in which a + * REQUEST is reachable after an `await` must contain a call to a predicate + * bound from `authStore.identityFence()`, positioned between them. + * + * WHY A SECOND, SIMPLER ANALYZER RATHER THAN REUSING THE AID MODULE. + * `src/test/identityFenceAst.ts` is a 1500-line flow analysis tuned to + * ItemDetail's specific vocabulary — its generation counters, its capture + * helpers, its fence booleans. Pointing it at fifteen other components would + * mean teaching it fifteen more vocabularies, and the last six windows on this + * family all went the same way: the review rounds are spent on the GUARD's + * defects, not the component's. So this one is deliberately small and answers a + * narrower question, and the rest is declared below rather than discovered in + * review. + * + * WHAT IT CANNOT SEE — the declared gaps, stated up front (lead ruling on + * checkpoint 1b). These are not suspicions; each is a construct this guard + * provably does not model: + * + * (a) PROVENANCE. It matches a CALL to a name bound from + * `authStore.identityFence()` in the same function. A predicate arriving + * as a prop, or returned by a helper module, reads as unfenced. Accepted + * as blind: closing it means tracking values across module boundaries, + * which is the aid module's job and its size. + * (b) BRANCH OUTCOME. It checks that the predicate is CALLED between the + * await and the send, not that the branch it guards actually returns. A + * fence whose body is empty passes here. The driven legs are what cover + * that — `timelineVersionCardIdentityFence.svelte.test.ts` presses the + * real component through the real window. + * (c) DEPTH. A send reached through a LOCAL helper is modelled to exactly ONE + * hop (`loadTimeline()`, `loadChildren()` — see `requestingHelperNames`), + * and a fence reached through a local helper likewise (`stale()`). A + * second hop in either direction is not modelled. For sends that is + * fail-OPEN, which is why it is stated here rather than implied. + * A send inside a callback passed to a helper that is not itself awaited + * here also reads as having no await before it. + * (d) COMMITS. This guard covers REQUESTS (`api.*`, `fetch`, and one-hop + * local helpers around them). A post-await assignment to component state + * is equally capable of showing one identity's data to the next, and is + * NOT modelled. The two commit rows fixed in this unit + * (`TimelineVersionCard.ensureResolved`, `ChildItems.loadChildren`) are + * covered by their own fences and by the driven legs, not by this guard. + * `ItemPicker.invokeCreate` and `ItemAttachmentStrip`'s post-confirmation + * delete are known members of this gap, found by hand. + * (e) NON-AWAIT SUSPENSION. `ItemAttachmentStrip.confirmDelete` re-checks on + * the far side of a NON-BLOCKING in-app confirmation menu — structurally + * the right move, on the wrong quantity (`paint.isCurrent()` compares + * `{ws, item}`). There is no `await` there at all, so nothing in this + * guard's model applies: a logout and different login while that menu is + * open passes the check and the DELETE goes out as the new user. Found by + * hand, not by this guard, and it is the clearest evidence that this + * guard is a floor and not a ceiling. + * + * FAIL-CLOSED, which is the property that makes the gaps survivable: anything + * this guard cannot classify is a FAILURE, never a skip. A file it cannot parse + * fails. A send whose enclosing function it cannot resolve fails. It never + * concludes "safe" from not having understood something — so the way this guard + * goes wrong is by asking to be taught, which is visible, rather than by going + * quiet, which is not. + * + * KNOWN GAPS ARE ENUMERATED, NOT TOLERATED IN BULK (lead ruling). The paths + * this unit did not fix are listed in `KNOWN_UNFENCED` by exact + * `file::function` path. Each listed path is EXPECTED to be unfenced: a path + * that gets fixed without being removed from the list ALSO fails, so the list + * cannot rot into a permanent exemption. Anything unfenced and unlisted fails. + * That is what makes this guard mergeable green while the follow-up unit is + * still open. + */ +import { describe, it, expect } from 'vitest'; +import { readFileSync } from 'node:fs'; +import { parseComponent, walk, type AstSource, type Node } from '../../../test/identityFenceAst'; + +const ROOT = new URL('../../../', import.meta.url).pathname; // src/ + +/** + * The child components ItemDetail mounts. Half 1 checks this against the real + * import list, so this array cannot silently drift from the component. + * + * `BottomSheet` is deliberately absent: ItemDetail imports it but mounts it + * nowhere (zero `::`. The follow-up unit's contract is + * exactly this list; removing an entry without fixing it, or fixing one without + * removing it, fails. + */ +const KNOWN_UNFENCED: string[] = [ + // ItemTimeline.loadMore: paginates the timeline; the second page request is + // issued after the first page's await resolves. + 'lib/components/timeline/ItemTimeline.svelte::loadMore', + // QuickActionsMenu: both send after awaiting an earlier request in the same + // handler; handleSaveNewAction is a read-modify-write across three calls. + 'lib/components/common/QuickActionsMenu.svelte::handleAction', + 'lib/components/common/QuickActionsMenu.svelte::handleSaveNewAction', + // EditCollectionModal.handleArchive: delete → list → delete, each after the + // previous await. The worst-shaped of the six: two destructive calls. + 'lib/components/collections/EditCollectionModal.svelte::handleArchive', + // CopyItemDialog.handleConfirm: re-runs the preflight after an earlier await. + // Note the copy call itself is NOT retried (no idempotency key) — the fence + // here must refuse, never re-send. + 'lib/components/items/CopyItemDialog.svelte::handleConfirm', + // ItemTimeline's six comment/reaction handlers: each writes, then calls + // `loadTimeline()` — a local helper that issues `api.timeline.list`. The + // write goes out as user A; if the identity moves during that round trip, + // the refresh GET goes out on the next user's cookie. These are only + // visible through the one-hop helper resolution in `requestingHelperNames`; + // a guard matching `api.*` syntactically reports them as having no send. + 'lib/components/timeline/ItemTimeline.svelte::submitComment', + 'lib/components/timeline/ItemTimeline.svelte::handleReply', + 'lib/components/timeline/ItemTimeline.svelte::handleEdit', + 'lib/components/timeline/ItemTimeline.svelte::handleDelete', + 'lib/components/timeline/ItemTimeline.svelte::handleReaction', + 'lib/components/timeline/ItemTimeline.svelte::handleRemoveReaction', + // CopyItemDialog.runPreflight: the trailing re-dispatch in its `finally` + // re-enters the function, issuing a preflight after the previous await with + // no guard at all — deliberately, per its own comment, because the trailing + // run re-reads the CURRENT destination. That argument is about which + // destination, not about who is asking. + 'lib/components/items/CopyItemDialog.svelte::runPreflight', + // PushToAgentDialog.handleSend: pushes, then refreshes presence after the + // await. Guarded by `!destroyed && gen === presenceGen` — instance liveness + // plus an opening generation, neither of which sees a user change. + 'lib/components/items/PushToAgentDialog.svelte::handleSend', + // Editor's onMount callback: `api.server.capabilities()` fire-and-forget. + // FLAGGED BUT PROBABLY NOT A DEFECT — the endpoint is public and carries no + // user-scoped data (its own comment says it works pre-login on shared-item + // preview surfaces), so a cross-identity issue leaks nothing. Listed rather + // than silently excluded because "this endpoint is public" is a judgement + // this guard cannot make, and the follow-up should record the decision + // instead of re-deriving it. + 'lib/components/editor/Editor.svelte::', +]; + + +function calleeText(src: AstSource, n: Node): string { + return src.text(n.callee).replace(/\s+/g, '').replace(/\?\./g, '.'); +} + +/** A DIRECT REQUEST: an `api.*` call or a bare `fetch(`. */ +function isDirectRequest(src: AstSource, n: Node): boolean { + if (n.type !== 'CallExpression') return false; + const c = calleeText(src, n); + return c === 'fetch' || c.startsWith('api.') || c.startsWith('apiClient.'); +} + +/** + * Script-level function names whose body issues a direct request — so a CALL to + * one of them is itself a send. + * + * WHY THIS EXISTS, and it was a genuine hole rather than a refinement. The + * commonest post-await send in this codebase is not an `api.*` call; it is a + * call to the component's own loader: `await api.comments.create(…)` then + * `await loadTimeline()`, `await api.items.create(…)` then `loadChildren()`. + * Matching only `api.*` syntactically inside the handler misses every one of + * them, and the miss is a FALSE NEGATIVE — the guard reports such a path as + * having no send at all and stays silent. + * + * Found by three independent readers auditing the same population by hand while + * this guard was already green; `ItemTimeline` alone has six such handlers, none + * of which this guard saw. Recorded here rather than quietly fixed because the + * lesson is the guard's, not the codebase's: a green source scanner had not been + * shown able to go red for the shape that dominates the population. + * + * ONE hop, matching `fenceHelperNames`: a helper calling a helper that requests + * is not modelled and reads as no-send, which is the fail-open direction and is + * therefore declared as gap (c) in the header rather than left implicit. + */ +function requestingHelperNames(src: AstSource): Set { + const names = new Set(); + walk(src.script, (n) => { + let fn: Node | null = null; + let name = ''; + if (n.type === 'FunctionDeclaration' && n.id?.type === 'Identifier') { + fn = n; + name = n.id.name; + } else if (n.type === 'VariableDeclarator' && isFnNode(n.init) && n.id?.type === 'Identifier') { + fn = n.init; + name = n.id.name; + } + if (!fn || !name) return; + let requests = false; + walk(fn, (m) => { + if (isDirectRequest(src, m)) requests = true; + }); + if (requests) names.add(name); + }); + return names; +} + +const isFnNode = (n: Node | undefined | null): n is Node => + !!n && + (n.type === 'ArrowFunctionExpression' || + n.type === 'FunctionExpression' || + n.type === 'FunctionDeclaration'); + +/** + * A STABLE name for a function, used as the key in `KNOWN_UNFENCED`. + * + * The fallback deliberately avoids byte offsets. An offset-keyed entry changes + * whenever anything ABOVE it in the file is edited, so the gap list would fail + * on edits that have nothing to do with it — noise that trains a reader to + * re-paste the key without re-reading the path, which is the one thing a + * known-gaps list must not teach. Naming an anonymous callback after the call + * it is an argument to (`onMount`, `$effect`) is stable under unrelated edits + * and still unique in practice; a genuine collision between two anonymous + * callbacks of the same callee in one file would merge their entries, which + * fails closed (both must be fixed to clear one). + */ +function fnName(src: AstSource, n: Node, ancestors: Node[]): string { + if (n.id?.name) return n.id.name; + const parent = ancestors[ancestors.length - 1]; + if (parent?.type === 'VariableDeclarator' && parent.id?.type === 'Identifier') return parent.id.name; + if (parent?.type === 'Property' && parent.key?.type === 'Identifier') return parent.key.name; + if (parent?.type === 'CallExpression' && parent.callee) { + return ``; + } + for (let i = ancestors.length - 1; i >= 0; i--) { + const a = ancestors[i]; + if (a.type === 'CallExpression' && a.callee) return ``; + if (isFnNode(a) && a.id?.name) return ``; + } + return ''; +} + +/** + * Names bound from `authStore.identityFence()` inside this function. + * Provenance gap (a): only a direct call to that member expression counts. + */ +function fenceNamesIn(src: AstSource, fn: Node): Set { + const names = new Set(); + walk(fn, (n) => { + if (n.type !== 'VariableDeclarator' || !n.init) return; + if (n.init.type !== 'CallExpression') return; + if (calleeText(src, n.init) !== 'authStore.identityFence') return; + if (n.id?.type === 'Identifier') names.add(n.id.name); + }); + return names; +} + +/** + * Names of local helpers whose body CALLS one of `names` — the `stale()` shape. + * + * ChildItems does not call its predicate at the send; it folds it into a + * `const stale = () => destroyed || !isSameIdentity() || …` declared in the + * same function, and calls `stale()` after each await. Without this hop the + * guard reports every one of those paths as unfenced, which is how it first + * behaved — it flagged the six paths this unit had just fixed. + * + * ONE hop only, and that bound is deliberate rather than lazy: a helper calling + * a helper calling the predicate is not modelled and reads as unfenced, which + * fails closed. Widening it is how a source scanner acquires an unbounded tail. + */ +function fenceHelperNames(src: AstSource, fn: Node, names: Set): Set { + const helpers = new Set(); + if (names.size === 0) return helpers; + walk(fn, (n) => { + if (n.type !== 'VariableDeclarator' || !isFnNode(n.init)) return; + if (n.id?.type !== 'Identifier') return; + let calls = false; + walk(n.init, (m) => { + if (m.type === 'CallExpression' && names.has(calleeText(src, m))) calls = true; + }); + if (calls) helpers.add(n.id.name); + }); + return helpers; +} + +/** Offsets at which the predicate — or a one-hop helper around it — is CALLED. */ +function fenceCallOffsets(src: AstSource, fn: Node, names: Set): number[] { + const out: number[] = []; + if (names.size === 0) return out; + const helpers = fenceHelperNames(src, fn, names); + walk(fn, (n) => { + if (n.type !== 'CallExpression') return; + const c = calleeText(src, n); + if (names.has(c) || helpers.has(c)) out.push(n.start); + }); + return out; +} + +/** + * Enclosing loop whose body contains an `await`, if any. + * + * THE BACK-EDGE CASE, encoded deliberately. `for (const x of xs) { await + * send(x); }` has its send TEXTUALLY BEFORE every await in the function, so a + * naive "is there an await above this send" reading calls it safe. It is not: + * every iteration after the first runs after the previous iteration's await. + * ChildItems' two reorder loops are exactly this shape and were the least + * guarded paths in the population, so a guard that reads them as fine would + * have certified the worst rows. + */ +/** + * True when `a` and `b` sit in DIFFERENT branches of the same `if` or ternary, + * and therefore never both run. + * + * Without this the guard reports a false positive on the common shape + * `const res = itemId ? await api.listItem(…) : await api.listCollection(…)`: + * the first branch's await is textually before the second branch's send, so a + * position-only reading calls the second "after an await". It is not — the two + * are alternatives. + * + * A false positive is fail-closed and would be survivable on its own. It is not + * survivable HERE: this guard's findings are the follow-up unit's contract, and + * an entry naming a path that is not a defect sends the next reader to fence + * something that cannot race. A wrong gap list is worse than a short one. + * + * `try`/`catch` is deliberately NOT treated as exclusive: a catch block really + * does run after the try block's await, which is a genuine await-then-send. + */ +function branchExclusive(fn: Node, a: Node, b: Node): boolean { + const contains = (n: Node | null | undefined, x: Node) => !!n && n.start <= x.start && n.end >= x.end; + let exclusive = false; + walk(fn, (n) => { + if (exclusive) return; + if (n.type === 'ConditionalExpression') { + if ( + (contains(n.consequent, a) && contains(n.alternate, b)) || + (contains(n.alternate, a) && contains(n.consequent, b)) + ) { + exclusive = true; + } + } else if (n.type === 'IfStatement') { + if ( + (contains(n.consequent, a) && contains(n.alternate, b)) || + (contains(n.alternate, a) && contains(n.consequent, b)) + ) { + exclusive = true; + } + } + }); + return exclusive; +} + +function awaitBearingLoopAncestor(ancestors: Node[]): Node | null { + const LOOPS = new Set(['ForStatement', 'ForOfStatement', 'ForInStatement', 'WhileStatement', 'DoWhileStatement']); + for (let i = ancestors.length - 1; i >= 0; i--) { + const a = ancestors[i]; + if (!LOOPS.has(a.type)) continue; + let hasAwait = false; + walk(a.body ?? a, (n) => { + if (n.type === 'AwaitExpression') hasAwait = true; + }); + if (hasAwait) return a; + } + return null; +} + +interface Finding { + path: string; + fn: string; + line: number; + callee: string; + reason: string; +} + +function analyseFile(path: string): { findings: Finding[]; fenced: string[] } { + let code: string; + try { + code = readFileSync(ROOT + path, 'utf8'); + } catch (e) { + // FAIL CLOSED, and cleanly: a POPULATION entry naming a file that is not + // there is a finding, not a crash. Raised while mutating this guard — an + // ENOENT escaping here reads as a broken test rather than as the guard + // reporting a stale population, which is the same confusion the + // population check exists to prevent. + return { + findings: [{ path, fn: '', line: 0, callee: '', reason: `missing file: ${String(e)}` }], + fenced: [], + }; + } + let src: AstSource; + try { + src = parseComponent(code); + } catch (e) { + // FAIL CLOSED: a file we cannot parse is a failure, never a skip. + return { + findings: [{ path, fn: '', line: 0, callee: '', reason: `unparseable: ${String(e)}` }], + fenced: [], + }; + } + + const findings: Finding[] = []; + const fenced: string[] = []; + const helperSends = requestingHelperNames(src); + const isSend = (n: Node) => + isDirectRequest(src, n) || + (n.type === 'CallExpression' && helperSends.has(calleeText(src, n))); + + walk(src.script, (fn, fnAncestors) => { + if (!isFnNode(fn)) return; + // Only functions that can suspend are in scope. + let hasAwait = false; + walk(fn, (n) => { + if (n.type === 'AwaitExpression') hasAwait = true; + }); + if (!hasAwait) return; + + const name = fnName(src, fn, fnAncestors); + const names = fenceNamesIn(src, fn); + const fenceOffsets = fenceCallOffsets(src, fn, names); + + // Every await NODE in this function. Nodes, not offsets, because a send + // is usually ITSELF awaited (`const x = await api.foo()`), and that + // wrapping AwaitExpression starts before the call it contains. Counting + // it as "an await preceding this send" leaves no room for any fence and + // reports every awaited request as unfenced — which is exactly how this + // guard first behaved, flagging the six paths this unit had just fixed. + // A send's own await is not something it can be fenced against. + const awaitNodes: Node[] = []; + walk(fn, (n) => { + if (n.type === 'AwaitExpression') awaitNodes.push(n); + }); + + walk(fn, (n, ancestors) => { + if (!isSend(n)) return; + // A nested function's sends belong to that function, not this one. + for (const a of ancestors) { + if (a !== fn && isFnNode(a)) return; + } + + const loop = awaitBearingLoopAncestor([...ancestors, n]); + // An await counts as PRECEDING this send only if it does not contain + // it — a send's own `await` is not a suspension point it could have + // been fenced after. + const preceding = awaitNodes.filter( + (a) => a.start < n.start && a.end < n.end && !branchExclusive(fn, a, n) + ); + const afterAwait = preceding.length > 0 || loop !== null; + if (!afterAwait) return; + + // The fence must be called between the await and the send. In the loop + // case there is no "before" in the textual sense — the back edge is the + // await — so the requirement is a fence call inside the loop body, + // textually above the send. + const lowerBound = loop ? loop.start : Math.max(...preceding.map((a) => a.start)); + const ok = fenceOffsets.some((o) => o > lowerBound && o < n.start); + + const label = `${path}::${name}`; + if (ok) { + fenced.push(label); + } else { + findings.push({ + path, + fn: name, + line: src.line(n.start), + callee: calleeText(src, n), + reason: + names.size === 0 + ? 'no authStore.identityFence() bound in this function' + : 'fence bound but not called between the await and the send', + }); + } + }); + }); + + return { findings, fenced }; +} + +describe('BUG-3095 — child population identity fence', () => { + it('the reviewed population is the population ItemDetail actually mounts', () => { + const detail = readFileSync(ROOT + 'lib/components/items/ItemDetail.svelte', 'utf8'); + const imported = new Set(); + for (const m of detail.matchAll(/import\s+(\w+)\s+from\s+'(\$lib\/[^']+\.svelte)'/g)) { + const [, local, spec] = m; + // Mounted, not merely imported — a dead import is not a child. + if (!new RegExp(`<${local}\\b`).test(detail)) continue; + imported.add(spec.replace('$lib/', 'lib/')); + } + // Relative imports (`./ItemPicker.svelte`) resolve against the items dir. + for (const m of detail.matchAll(/import\s+(\w+)\s+from\s+'(\.\/[^']+\.svelte)'/g)) { + const [, local, spec] = m; + if (!new RegExp(`<${local}\\b`).test(detail)) continue; + imported.add('lib/components/items/' + spec.replace('./', '')); + } + + const reviewed = new Set(POPULATION.filter((p) => !NOT_DIRECTLY_IMPORTED.has(p))); + const missing = [...imported].filter((p) => !reviewed.has(p)).sort(); + const stale = [...reviewed].filter((p) => !imported.has(p)).sort(); + + expect( + { missing, stale }, + 'ItemDetail mounts a child this guard has never reviewed, or POPULATION names one it no longer mounts. ' + + 'Add it to POPULATION (and review its await-then-request paths) or remove it.' + ).toEqual({ missing: [], stale: [] }); + }); + + it('every await-then-request path in the population holds an identity fence', () => { + const findings: Finding[] = []; + for (const path of POPULATION) findings.push(...analyseFile(path).findings); + const unlisted = findings + .map((f) => ({ ...f, label: `${f.path}::${f.fn}` })) + .filter((f) => !KNOWN_UNFENCED.includes(f.label)); + + expect( + unlisted.map((f) => `${f.label} @${f.line} → ${f.callee} (${f.reason})`), + 'An await-then-request path with no identity fence. Fence it with ' + + 'authStore.identityFence(), or — if it belongs to the follow-up unit — ' + + 'add its exact file::function path to KNOWN_UNFENCED.' + ).toEqual([]); + }); + + it('every KNOWN_UNFENCED entry is still unfenced, so the list cannot rot into an exemption', () => { + const findings: Finding[] = []; + for (const path of new Set(KNOWN_UNFENCED.map((k) => k.split('::')[0]))) { + findings.push(...analyseFile(path).findings); + } + const stillUnfenced = new Set(findings.map((f) => `${f.path}::${f.fn}`)); + const fixed = KNOWN_UNFENCED.filter((k) => !stillUnfenced.has(k)).sort(); + expect( + fixed, + 'These paths are listed as known-unfenced but are now fenced (or gone). ' + + 'Remove them from KNOWN_UNFENCED — a stale entry is a permanent hole.' + ).toEqual([]); + }); +}); + +/** Exported for the guard's own tests below. */ +export { analyseFile, POPULATION, KNOWN_UNFENCED }; diff --git a/web/src/lib/components/items/itemDetailIdentityFenceAst.test.ts b/web/src/lib/components/items/itemDetailIdentityFenceAst.test.ts index cea84ee81..a0d4e6e10 100644 --- a/web/src/lib/components/items/itemDetailIdentityFenceAst.test.ts +++ b/web/src/lib/components/items/itemDetailIdentityFenceAst.test.ts @@ -196,13 +196,27 @@ function codeOf(src: AstSource, fn: Node): string { } /** - * A separator that cannot occur in the source: the reviewed text is built by - * JOINING slices, so the join has to be unambiguous or two different programs - * render the same text. Round 10's ASI finding is exactly that — `return` - * followed by an expression on the next line is TWO statements whose slices, - * joined by a space, are byte-identical to the ONE statement spanning the - * same two lines, and `identityHeld` returning `undefined` hashed the same as - * `identityHeld` returning the comparison. + * A separator that cannot occur at a STATEMENT BOUNDARY — which is the only + * place the join ever puts it. The reviewed text is built by JOINING slices, + * so the join has to be unambiguous or two different programs render the same + * text. Round 10's ASI finding is exactly that — `return` followed by an + * expression on the next line is TWO statements whose slices, joined by a + * space, are byte-identical to the ONE statement spanning the same two lines, + * and `identityHeld` returning `undefined` hashed the same as `identityHeld` + * returning the comparison. + * + * The boundary wording is load-bearing and replaces an earlier claim that this + * byte "cannot occur in the source" (BUG-3095). That was false: a NUL is legal + * inside a string literal, so it CAN occur in the source. What it cannot do is + * appear between two statements, because a statement boundary is not inside a + * literal — and a slice join only ever lands there. The weaker, true claim is + * the one the collision argument actually needs. + * + * CAUTION for anyone searching this file: the NUL below makes every byte- + * oriented tool classify it as BINARY. The harness `grep` runs with `-I`, so + * it returns exit 1 and NO output for patterns that are present here — a + * silent false negative indistinguishable from a real absence. Use + * `/bin/grep -a … | cat -v` (which renders this byte as `^@`) instead. */ const SEP = ''; diff --git a/web/src/lib/components/timeline/TimelineVersionCard.svelte b/web/src/lib/components/timeline/TimelineVersionCard.svelte index 132755f23..9ff3c44b8 100644 --- a/web/src/lib/components/timeline/TimelineVersionCard.svelte +++ b/web/src/lib/components/timeline/TimelineVersionCard.svelte @@ -1,6 +1,7 @@