docs(todo): add Discovery pass 2 — PERF/SEC/COR findings (agent-surveyed)
Three agents swept fresh lenses (performance, security/privacy, correctness in under-covered subsystems). 16 verified items filed: PERF-1..6, SEC-1..5 (no exploitable XSS found — sanitization surface is hardened), COR-1..6. Single-pass, not yet TPVR'd. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -67,6 +67,36 @@ Built and gate-green; verify per [LOTUS_TESTING.md](./LOTUS_TESTING.md), then gr
|
||||
|
||||
Agent-surveyed + TPVR-verified correctness / a11y / tech-debt fixes (DP1–DP18) are implemented, review-fixed, and gate-green (tsc + eslint + 737 tests + build). Verify per [LOTUS_TESTING.md](./LOTUS_TESTING.md) §R, then remove this note. Full detail in git history — commits `8eb961b6` `db864326` `6cf18c3b` `165714e1` `8c0e2b42` `e545706c` `b1ee3ada` `4fc3f7a3` `101e4116` `4fa4327a` `c2598d21`.
|
||||
|
||||
### 🔍 Discovery pass 2 (2026-07) — perf / security / correctness (agent-surveyed, single-pass — NOT yet TPVR'd)
|
||||
|
||||
Three agents swept fresh lenses (performance, security/privacy, correctness in under-covered subsystems); each verified its own findings against the code + SDK. `Sev`/`Eff` = S/M/L. Confirm before fixing (no independent TPVR yet).
|
||||
|
||||
**Performance / rendering:**
|
||||
|
||||
- [ ] **PERF-1 — Presence: 3 global client listeners registered PER avatar (Sev High · Eff M).** `hooks/useUserPresence.ts:43-53` (via `PresenceRingAvatar.tsx:19`, `MembersDrawer.tsx:142`) — every timeline/member avatar adds `mx.on` for `Presence`/`CurrentlyActive`/`LastPresenceTs` → 100–250 global listeners; each presence event fans out across all, and fast scroll churns add/remove per Message mount. Fix: one shared presence store (context + `useSyncExternalStore` or a jotai `atomFamily`) with 3 listeners total; components select by userId.
|
||||
- [ ] **PERF-2 — `#`-mention autocomplete re-sorts AND mutates the room list every keystroke (Sev Med · Eff S).** `components/editor/autocomplete/RoomMentionAutocomplete.tsx:83` — `useAtomValue(allRoomsAtom).sort(factoryRoomIdByActivity(mx))` is unmemoized (O(N log N) `mx.getRoom` per compare, N=all joined rooms, per keypress) **and `.sort()` mutates the shared `allRoomsAtom` array in place.** Fix: `useMemo(() => [...rooms].sort(...), [rooms, mx])`.
|
||||
- [ ] **PERF-3 — Read-receipt rows attach per-instance global `RoomStateEvent.Members` listeners (Sev Med · Eff M).** `components/read-receipt-avatars/ReadReceiptAvatars.tsx:74` + `hooks/useMemberAvatar.ts:44` — up to ~6 global `Members` listeners per receipt row; fires on any membership/name/avatar change in any room. Fix: one shared room-member-change subscription fanning out by roomId+userId (shares with PERF-1).
|
||||
- [ ] **PERF-4 — Message-search room filter re-sorts full room list every render (Sev Low–Med · Eff S).** `features/message-search/SearchFilters.tsx:155` — `Array.from(...).sort(factoryRoomIdByAtoZ(mx))` outside `useMemo` (re-sorts as the user types). Fix: `useMemo`.
|
||||
- [ ] **PERF-5 — DM-preview hook subscribes a global `Decrypted` listener for EVERY nav item incl. non-DMs (Sev Low–Med · Eff S).** `hooks/useRoomLatestRenderedEvent.ts:64-67` (called at `RoomNavItem.tsx:681`) — preview is only used for `direct` rooms but the hook + its `MatrixEventEvent.Decrypted` listener run for all; wasteful, and fans out across all during bulk decryption. Fix: gate the hook/subscription on `direct`.
|
||||
- [ ] **PERF-6 — Avatar-decoration profile-fetch on first render of large member/timeline views (Sev Low · Eff M, likely leave).** `hooks/useAvatarDecoration.ts` — one `/profile/{userId}` per new user (well-guarded: module cache + in-flight dedupe + backoff). Network/HS-load note only; batch/skip only if it proves costly.
|
||||
|
||||
**Security / privacy:** _(no exploitable XSS found — `sanitize.ts` strips `javascript:`, forces mxc `<img>` + `rel=noopener` `<a>`; embeds use host-allowlist + origin-scoped `postMessage`; KaTeX `trust:false`. These are storage-hygiene / URL items.)_
|
||||
|
||||
- [ ] **SEC-1 — Scheduled-message plaintext persists in `localStorage`, survives logout (Sev Med · Eff S).** `state/scheduledMessages.ts:12-19` — full `IContent` (incl. E2EE cleartext `body`) under `cinny_scheduled_messages_v1`; normal logout (`removeFallbackSession`, N98) keeps it. Leak on shared/kiosk browser. Fix: clear on logout (add to `removeFallbackSession`) — or encrypt-at-rest with N97.
|
||||
- [ ] **SEC-2 — Recent search terms persist in `localStorage`, survive logout (Sev Low · Eff S).** `state/recentSearches.ts:4-12` — last 10 queries (often PII) under `cinny_recent_searches_v1`, survive normal logout. Fix: clear on logout.
|
||||
- [ ] **SEC-3 — `window.open(url, '_blank')` without `'noopener'` → reverse tab-nabbing (Sev Low · Eff S).** `components/user-profile/UserChips.tsx:117` (+ `SSOStage.tsx:23`, `settings/devices/Verification.tsx:276`, `OtherDevices.tsx:36,49`, `OidcManageAccount.tsx:23`) — `window.open` doesn't null `window.opener`. UserChips opens a user/room-derived server URL. Fix: pass `'noopener,noreferrer'` (or set `w.opener = null`).
|
||||
- [ ] **SEC-4 — `/acl` has no `*`/self-lockout guard or glob validation (Sev Low · Eff S–M).** `hooks/useCommands.ts:489-538` — `/acl -d *` writes `deny:['*']` (can brick the room); empty/whitespace globs pushed unvalidated into `m.room.server_acl`. PL-gated + matches upstream, but a destructive footgun. Fix: reject a `*`/empty deny matching the local server, validate glob syntax, confirm on `*`.
|
||||
- [ ] **SEC-5 — Embeds get `allow-popups-to-escape-sandbox` (Sev Low, informational · Eff M).** `components/url-preview/UrlPreviewCard.tsx:590-591` (`EMBED_SANDBOX`) — a hijacked provider script could spawn an un-sandboxed popup (phishing). Main-app hijack already prevented (no `allow-top-navigation`); popups arguably needed for "open in provider." Fix (if desired): drop the escape flag + verify per-provider.
|
||||
|
||||
**Correctness (under-covered subsystems):**
|
||||
|
||||
- [ ] **COR-1 — Space-child UNLINK nukes a room's OTHER parents + orphans descendants (Sev Med · Eff M).** `state/room/roomToParents.ts:102-103` (DELETE reducer `:53-66`) — removing one `m.space.child` fires the same `DELETE` used for "room left," wiping ALL of the child's parents and stripping it as a parent everywhere. Remove C from space A only → C also loses parent B, and C's own children lose C, until a full resync. Fix: a targeted "remove one parent from one child" action. (Reducer inherited from upstream.)
|
||||
- [ ] **COR-2 — `useCallJoined` stays `true` across a direct embed swap (answering a 2nd call) (Sev Med · Eff S).** `hooks/useCallEmbed.ts:140-158` — the reset effect only clears `joined` when `!embed`; answering call B from the banner swaps embed A→B directly, so `joined` stays true → B renders as live/joined before EC actually joins, skipping the loading/watchdog UI. Fix: `useEffect(() => setJoined(embed?.joined ?? false), [embed])`.
|
||||
- [ ] **COR-3 — Incoming-call lifetime guard only corrects FUTURE clock-skew, not past (Sev Low · Eff S).** `components/CallEmbedProvider.tsx:476-477` — distrusts `sender_ts` only when >20 s ahead of `origin_server_ts`; a caller clock that's behind leaves `sender_ts` in the past → `remaining ≤ 0` → ring auto-dismisses/never shows for a fresh invite. Fix: `Math.abs(...) > 20000` or just use `event.getTs()`.
|
||||
- [ ] **COR-4 — Shared notify-dedupe slot double-notifies a re-fired main message after an interleaving thread reply (Sev Low · Eff S).** `pages/client/ClientNonUIFeatures.tsx:521,530` — `lastNotifiedEventRef` is one `Map<roomId,eventId>` slot shared by the main + thread paths; a thread reply overwrites it, so a re-emitted main message (decrypt/edit re-fire, common in E2EE) mis-dedupes and notifies again. Fix: separate dedupe keys per path (or key by `roomId|eventId`).
|
||||
- [ ] **COR-5 — Upload cancel during retry back-off is silently ignored (upload resurrects) (Sev Low · Eff M).** `utils/matrix.ts:204-238` + `state/upload.ts:125-129` — during the 1–30 s back-off the atom holds the previous (rejected) promise, so `cancelUpload` is a no-op and the loop wakes to start the next attempt. Fix: thread an abort flag into the retry loop.
|
||||
- [ ] **COR-6 — `CallControl.forceState` resets `screenshareAudioMuted` to false (Sev Low · Eff S).** `plugins/call/CallControl.ts:155-162` — `forceState` passes only 5 args so the 6th defaults false; low impact today (runs at join, no screenshare yet) but silently discards the field. Fix: pass `this.screenshareAudioMuted`.
|
||||
|
||||
### ✅ Unread/read-receipt flakiness (reported 2026-07) — FIXED (pending prod QA)
|
||||
|
||||
Room unread dots were inconsistent: reading a message sometimes cleared the dot, sometimes left it stuck, sometimes it resurrected. Root cause (confirmed by tracing + diffing upstream cinny `dev`): **our own "N4" change.** `handleReceipt` recomputed via `getUnreadInfo`, which reads `room.getUnreadNotificationCount()` — server-computed and **stale on the synchronous synthetic receipt echo** (SDK only zeroes it immediately when the last event is your own message) → it PUT the stale non-zero count back → stuck/resurrecting. Compounded by `hasUnread = !!unread` lighting the dot on any present map entry, incl. phantom `{0,0}` PUTs from our `UnreadNotifications` listener. Plus a Mark-as-Unread (MSC2867) flag that never cleared on opening an already-read room (no receipt → no auto-clear).
|
||||
|
||||
Reference in New Issue
Block a user