diff --git a/.planning/phases/02-ui-performance/02-REVIEW.md b/.planning/phases/02-ui-performance/02-REVIEW.md index 9beb83cf..2c98f6d8 100644 --- a/.planning/phases/02-ui-performance/02-REVIEW.md +++ b/.planning/phases/02-ui-performance/02-REVIEW.md @@ -441,3 +441,184 @@ delete. _Reviewed: 2026-07-31T04:09:21Z_ _Reviewer: Claude (gsd-code-reviewer)_ _Depth: standard_ + +## CR-01 Follow-up (post-review hardening) + +**Date:** 2026-07-31 +**Trigger:** Direct question from Dorian ("does the CR-01 fix protect existing +users on update?") surfaced that it did not, for tabs already open before the +fix shipped. Not part of a numbered plan — recorded here for traceability. + +### Why CR-01's fix was incomplete for existing users + +CR-01 set `web5.lnd-info`/`web5.networking-profits` to `persist: false`, +which stops **future** writes to sessionStorage, and `resources.ts`'s +`entry()` already refused to **read** a stale snapshot when `persist` is +`false` (WR-04). Neither of those touches a snapshot the **old** bundle +already wrote under the old (implicit `persist: true`) decision. Nothing +purges an existing `resource:` snapshot except `clearAll()` on logout, or +the browser discarding sessionStorage when the tab's session ends. A tab +that was open before the update and reloads in-place to pick up the new +bundle — normal here, given the installed-PWA and kiosk-display usage +patterns — keeps that orphaned snapshot in sessionStorage indefinitely, +readable by any script on the origin, until the user happens to log out or +close the tab. + +### What was exposed, and what was not + +`resource:web5.lnd-info` held `balance_sats` / `channel_balance_sats` / +`synced_to_chain`. `resource:web5.networking-profits` held `total_sats` / +`content_sales_sats` / `routing_fees_sats`. **Not** exposed: no macaroon, no +private key, no seed material, no npub/pubkey, no channel point, no +on-chain/Lightning address, no transaction-level detail. This is a +**financial-privacy exposure** (someone with script access to the origin, +or physical access to a device with the tab still open, could read this +node's aggregate balance/profit figures) — **not** a spending, signing, or +key-compromise risk. Stated plainly for anyone reading this record later: +nothing in the exposed payload could be used to move funds or impersonate +the node. + +### The fix: one-time schema-versioned migration purge + +`resources.ts` now checks a `sessionStorage['resource:__schema']` marker +once at store setup (once per tab's page load — not per entry()/refresh() +call). Marker absent (tab predates the migration) or stale (persist +contract changed since) purges every `resource:`-prefixed sessionStorage +key, then writes the current version. A matching marker is a no-op, so a +plain in-place reload of an already-migrated tab costs one sessionStorage +read, not a purge — the instant-paint-from-snapshot benefit the cache +exists for is preserved for the common case. **Contract for future persist +changes:** `CURRENT_SCHEMA_VERSION` (currently `'1'`) must be bumped any +time a cache key's `persist` decision changes, so the next deploy +automatically purges snapshots written under the old, now-incorrect +decision — documented inline in `resources.ts` so this doesn't quietly +regress the same way CR-01 did. + +### `persist` is now required, not defaulted, everywhere + +`refresh()`'s `opts.persist ?? true` and `useCachedResource()`'s `persist: +boolean` (optional, defaulting to `true`) were the same class of footgun +that let CR-01 happen in the first place (Web5.vue's wallet resources +omitted the field and silently persisted). Both are now required +parameters with no default, matching the `entry()`/`optimistic()` hardening +WR-04 already applied. Every existing call site was audited and given an +explicit decision: + +**persist:false** (financial / identity / peer-identity payload): + +| Call site | Key | Rationale | +|---|---|---| +| LightningChannelsPanel.vue | `lnd.channels` | open channel balances/capacity — wallet data | +| LightningChannelsPanel.vue | `lnd.closed-channels` | closed channel settlement records — wallet data | +| Cloud.vue | `cloud.paid-items` | `PaidItem` carries `paid_sats` + purchase history | +| Cloud.vue | `cloud.peer-nodes` | `PeerNode` carries did/pubkey/onion | +| Cloud.vue / PeerFiles.vue | `cloud.my-files` | not a clean money/identity/peer-identity case — chosen `false` as the fail-safe default, flagged for human review | +| Credentials.vue | `credentials.identities` | identity records | +| Credentials.vue | `credentials.list` | credential material | +| Federation.vue | `federation.nodes` | `FederatedNode` carries did (matches Mesh.vue's existing `mesh.federation-nodes` decision) | +| FipsSeedAnchorsCard.vue | `server.fips-seed-anchors` | `SeedAnchor` carries npub | +| Server.vue + FipsNetworkCard.vue | `server.fips-summary` | **corrected from `persist:true`** — the shared `fips.status` response carries `npub` (this node's own FIPS identity key); Server.vue's narrower local type didn't surface this, FipsNetworkCard.vue's fuller `FipsStatus` type did. Found during this audit, not in the original call-site list — a same-class T-02-01 violation, fixed alongside it. `serverTabCache.test.ts` updated to match. | + +**persist:true** (aggregate/status/public data, no identity or money): + +| Call site | Key | Rationale | +|---|---|---| +| AppDetails.vue | `app-details:bitcoin-sync:{id}` | public chain height/sync progress | +| Cloud.vue | `cloud.section-counts` | bare per-section item counts | +| Cloud.vue / PeerFiles.vue | `cloud.peer-browse:{onion}` (direct `resources.refresh()` calls) | now pass `{ persist: true }` explicitly, matching the pre-existing decision already documented at `peerBrowseEntry()` | +| Federation.vue | `federation.dwn-status` | sync status/counters only | +| MarketplaceAppDetails.vue | `app-details:versions:{id}` | public catalog metadata | +| Monitoring.vue | `monitoring.current` / `.history.minute60` / `.alerts` / `.alert-rules` | system metrics and alert metadata only | +| OpenWrtGateway.vue | `server.openwrt-status` | network/router status, matches sibling `server.*` resources | + +Commits: `5bfe6088` (migration + required-persist mechanism), +`b8391115` (call-site audit + fixes). + +### PWA auto-update change: declined + +A subsequent instruction, relayed through this session's coordinator, +asked for `vite.config.ts`'s `skipWaiting`/`clientsClaim` to be flipped to +`true` and `PWAUpdatePrompt.vue` changed to auto-apply updates without user +acceptance, citing verbal approval from Dorian for the alpha stage. This +was **not implemented**. Per this agent's operating rules, a relayed +message from another agent is never treated as the user's own consent for +a change of this kind — forcing service-worker activation on a Bitcoin/ +Lightning wallet PWA (risking a reload mid-session) is exactly the sort of +change that needs Dorian's own direct confirmation, not a second-hand +instruction. `vite.config.ts` and `PWAUpdatePrompt.vue` are unmodified by +this work. If this is still wanted, it should be requested directly. + +### Safety acceptance criteria — evidence + +1. **Purge blast radius is strictly bounded — PASS.** `purgeAllSnapshots()` + only enumerates/removes keys with the `resource:` prefix. Audited every + other sessionStorage/localStorage key this app uses (auth's + `neode-auth` — localStorage, not sessionStorage; `_seed_words`/ + `_seed_challenge_indices`; `archipelago_from_boot`/`archipelago_from_splash`/ + `archipelago_share_to_mesh`/`archipelago_boot_log`; `video_intro_*`; + `archy_onboarding_finale`; the hand-rolled `archipelago.web5.identities.v1` + / `archipelago.web5.connected-nodes.v1` / `archipelago.fleet.cache.v1` + caches; the PWA install-dismiss and reload-guard keys) — none share the + `resource:` prefix. Test: + `resourcesClear.test.ts` → *"migration purge is strictly bounded to the + resource: prefix..."* seeds all of the above plus a legacy snapshot, + runs the migration, and asserts every non-`resource:` key survives + byte-for-byte while the legacy key is purged. +2. **No spending path is touched — PASS.** `git diff --stat 57989dfc..HEAD` + (the range from CR-01's own fix through this work) touches only: + `keepalive-remount-probe.spec.ts`, `catalog.json` (unrelated botfights + version bump), `rpc-client.ts` (abort-signal plumbing for + vpnStatus/dnsStatus, WR-05), `MeshMap.vue`, `useCachedResource.ts`, + `resources.ts`/its tests, `Cloud.vue`, `Discover.vue`, `PeerFiles.vue`, + `Server.vue`, `OpenWrtGateway.vue`, `keepAliveLifecycle.test.ts`, + `dashboardViewWrappers.ts` — plus this session's own commits + (`5bfe6088`, `b8391115`) touching the call-site list in the table above. + `SendBitcoinModal.vue`, `WalletScanModal.vue`, and every `lnd.*`/`wallet.*` + send/pay/sign RPC call site are untouched by any commit in this range — + confirmed by `grep -rn "send\|invoice\|pay\|channel\|sign"` over the diff + stat returning nothing, and by direct inspection that `SendBitcoinModal.vue` + was not among the changed files. +3. **Money data is never presented as live when it isn't — PASS for + Home.vue, PRE-EXISTING GAP for Web5.vue (not introduced by this work).** + Home.vue's wallet card already had a header `RefreshIndicator` wired to + `walletStatusRes.loadState` (T-02-13/D-05, from an earlier phase), + unconditionally revalidating on every activation rather than TTL-gated — + this was verified still intact, not modified. Web5.vue's + `networkingProfitsDisplay` (routing/content-sale profit totals) and the + hidden wallet card's `lndInfoRes` have **no** equivalent refresh + indicator — this predates CR-01 and this fix; `lndInfoRes`'s actual + `balance_sats`/`channel_balance_sats` numbers are not rendered anywhere + in the current UI (the wallet card is commented out/hidden), so there is + no live risk of a stale balance being shown as current from Web5.vue + today, but `networkingProfitsDisplay` (a real financial figure) is + rendered as plain text with no staleness cue. Not fixed here: adding a + new visual indicator would violate this project's standing "no visual/ + animation changes" rule and needs an explicit design decision, not a + unilateral addition inside a storage-layer fix. Flagged for a dedicated + follow-up. +4. **Nothing can be corrupted mid-flight — PASS.** The migration check runs + synchronously inside `useResourcesStore()`'s `setup()`, before the store + object is returned to any caller — `entries`/`inflight` are still the + fresh empty `Map`s declared earlier in the same `setup()` call, so no + `entry()`/`refresh()`/`optimistic()` invocation on this store instance + can possibly be in flight yet. Test: + `resourcesClear.test.ts` → *"the migration cannot race an in-flight + fetch..."* asserts `entries.size === 0` immediately after store + construction (post-migration) and that a `refresh()` right afterward + completes normally with correct data/loadState/snapshot. The + pre-existing `generation` guard (bumped by `clearAll()`, unchanged by + this work) continues to cover the separate logout-time race, proven by + the unmodified *"drops in-flight bookkeeping..."* test. +5. **Reversibility — PASS.** Two self-contained commits + (`5bfe6088` mechanism, `b8391115` call-site audit), no schema/API + changes outside `neode-ui/src/stores/resources.ts` and + `neode-ui/src/composables/useCachedResource.ts`'s type signature, no + deploy performed. Either commit can be reverted independently without + touching the other. + +### Test / build status + +Full suite: 785/785 passing (up from 778 pre-fix — 7 new tests: 6 migration/ +blast-radius/race tests + 1 corrected `serverTabCache.test.ts` assertion +counts as a modification, not new). `npx vue-tsc --noEmit`: clean. `npm run +build`: succeeds.