docs(audit): Wave 2 audit — 28 new findings across 4 domains (N97–N128)

Security & data persistence (N97–N100): plaintext access token storage detail, normal logout wiping user prefs via localStorage.clear(), sync ERROR freezing the loading screen, unrestricted CSS classes on <pre>. PWA/SW/notifications (N105–N109): missing SW notificationclick + push handlers, decrypted E2EE message body leaked to OS notification center, missing maskable PWA icon, auth media URLs producing 401 in notification icon/badge fetches. Lotus feature internals (N113–N120, N128): reminder read-modify-write race, fire-and-forget removeReminder silently drops on network failure, setInterval restart on every reminder state change, useCallSpeakers rebuilds speaker set from mutation batch only (drops current speakers), static NodeList misses mid-call tile additions, CDN outage silently wipes decoration catalog, CDN URL drift between two source files, patch-folds silent exit-0 when patch target not found. Call system & noise suppression (N122–N127): setMediaState Promise hangs forever if EC omits DeviceMute echo, focusCameraParticipant drops tile click if spotlight isn't ready in 2 rAFs, denoise cleanup() leaks AudioWorklet gateNode, postMessage wildcard '*' origin, PiP position NaN on corrupt localStorage, denoise shim inactive in vite dev. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2026-06-27 20:57:45 -04:00
parent 91c6f2f091
commit af58f7a32c
1 changed files with 326 additions and 0 deletions
@@ -508,3 +508,329 @@ This document tracks identified bugs, edge cases, and architectural discrepancie
 - **Issue:** The `Retry` and `Leave` buttons in the call error overlay both executed the exact same `dismiss` function (`setCallEmbed(undefined)`), returning the user to the prescreen. "Retry" falsely implied it would automatically re-attempt joining the call. A true retry would require threading the previous `CallPreferences` into this component (via props or a Jotai atom) — a non-trivial change.
 - **Root Cause:** Two identically-wired buttons with misleading labels; the simpler recovery path is a single honest label.
 - **Fix Applied:** Removed the "Retry" button. Renamed "Leave" → "Back". One button, one clear action: returns to the prescreen where the user can manually click Join again to retry. Updated the code comment to match.
 ---
 ## 📱 PWA, Service Worker & Notifications Audit (Wave 2)
 > Scope: `src/sw.ts`, `src/app/pages/client/ClientNonUIFeatures.tsx`, `vite.config.js`, `public/manifest.json`, `src/app/utils/callSounds.ts`, `src/app/hooks/useCallJoinLeaveSounds.ts`.
 > Numbers N105–N109. Items already open (asset caching, `manifest: false`, `new Notification()` vs `showNotification()`) are NOT re-listed.
 ---
 **N105 — Missing SW `notificationclick` handler: notification clicks broken when tab is closed**
 - **File:** `src/sw.ts` (handler entirely absent); `src/app/pages/client/ClientNonUIFeatures.tsx`, lines 151–155 (`InviteNotifications`) and 277–284 (`MessageNotifications`)
 - **Status:** **OPEN** [Claude_Found]
 - **Issue:** All notification click handling is wired via `noti.onclick` in the main thread (`noti.onclick = () => { navigate(...); noti.close(); }`). This callback only fires while the originating tab is open and its JavaScript is running. When the browser has no open tabs for the app (or the tab is suspended/backgrounded), clicking an OS notification does nothing — there is no SW `notificationclick` handler to focus an existing window or open a new one and navigate to the correct room.
 - **Root Cause:** Notifications were built entirely in the main thread without a corresponding SW `notificationclick` event listener. The SW is registered and active but has zero notification-lifecycle handlers.
 - **Fix:** Add a `notificationclick` handler to `src/sw.ts` that calls `event.waitUntil(clients.matchAll({ type: 'window', includeUncontrolled: true }).then(list => { const win = list[0]; if (win) return win.focus(); return clients.openWindow(event.notification.data?.url ?? '/'); }))`. Pass the target room URL via `data: { url: roomPath }` in the `Notification` constructor so the SW can navigate correctly.
 ---
 **N106 — Decrypted E2EE message plaintext leaked to OS notification center**
 - **File:** `src/app/pages/client/ClientNonUIFeatures.tsx`, line 343
 - **Status:** **OPEN** [Claude_Found]
 - **Issue:** The `MessageNotifications` component passes `mEvent.getContent().body` directly as the notification body: `body: (mEvent.getContent().body as string | undefined) ?? ''`. By the time `RoomEvent.Timeline` fires, `matrix-js-sdk` has already decrypted the event in memory. The fully decrypted plaintext is then handed to `new window.Notification()`, which stores it in the OS notification center. This plaintext is visible on the device lock screen (if notification previews are enabled), in the OS notification history, and may be read by any app with `READ_NOTIFICATIONS` permission (e.g., accessibility services, backup apps) — even when the room uses end-to-end encryption. The 120-character slice (`slice(0, 120)`) does not mitigate this.
 - **Root Cause:** No distinction is made between encrypted and unencrypted rooms when constructing notification bodies. There is no check such as `mEvent.isEncrypted()` or `room.hasEncryptionStateEvent()` that would substitute a generic body.
 - **Fix:** Check whether the room is encrypted before populating the body. For encrypted rooms, use a generic string (e.g., `"New encrypted message"`) as the body instead of the decrypted content. If message previews in notifications are intentionally desired by the user, gate them behind an explicit opt-in setting that warns about OS-level plaintext exposure.
 ---
 **N107 — SW has no `push` event handler: Web Push delivery is completely broken**
 - **File:** `src/sw.ts` (handler entirely absent)
 - **Status:** **OPEN** [Claude_Found]
 - **Issue:** The service worker never registers a `push` event listener. If a Matrix push gateway (e.g., Sygnal) is ever configured and sends a Web Push notification, the SW silently discards the push event — no notification is shown, no in-app routing occurs. The absence of a `push` handler means the entire background-notification path (i.e., notifications when no tab is open) is non-functional, which is one of the primary requirements for a PWA.
 - **Root Cause:** The SW was written exclusively to proxy authenticated Matrix media requests. No background notification plumbing was ever added.
 - **Fix:** Add a `push` event listener to `src/sw.ts` that reads the push payload (`event.data.json()`), then calls `self.registration.showNotification(title, { body, data: { url } })`. Pair with the `notificationclick` fix from N105. On the app-registration side, wire `PushManager.subscribe()` to a Matrix push gateway so the server can actually deliver pushes.
 ---
 **N108 — No maskable icon in PWA manifest: Android adaptive icons display incorrectly**
 - **File:** `public/manifest.json`, lines 12–57
 - **Status:** **OPEN** [Claude_Found]
 - **Issue:** The manifest lists nine `android-chrome-*.png` icons (36 × 36 through 512 × 512) but none include `"purpose": "maskable"`. Android 8+ adaptive icons apply a platform-defined shape mask (circle, squircle, teardrop, etc.) to PWA home-screen icons. Without a maskable-purpose icon, the OS either adds a white square background to prevent clipping or applies the mask directly to the regular icon, typically cropping the Lotus logo in a visually incorrect way.
 - **Root Cause:** Icons were added from a standard Android icon set without adding a `maskable` variant. The `"purpose"` field defaults to `"any"`, which tells the OS the icon is not designed for safe-area masking.
 - **Fix:** Create a variant of the Lotus icon with sufficient padding (at least 10% safe zone on all sides so the center artwork survives any clip shape) and add it as a separate manifest entry with `"purpose": "maskable"`, e.g.: `{ "src": "./res/android/android-chrome-512x512-maskable.png", "sizes": "512x512", "type": "image/png", "purpose": "maskable" }`. One maskable icon at 512 × 512 is sufficient; keep the existing `"any"` entries.
 ---
 **N109 — Authenticated media URLs passed to `Notification` icon/badge: OS cannot fetch them (produces 401)**
 - **File:** `src/app/pages/client/ClientNonUIFeatures.tsx`, lines 333–339 and 270–273
 - **Status:** **OPEN** [Claude_Found]
 - **Issue:** When the homeserver requires authenticated media (Matrix spec v1.11+, path `/_matrix/client/v1/media/download/...`), `mxcUrlToHttp(mx, avatarMxc, useAuthentication, 96, 96, 'crop')` returns an authenticated URL. That URL is then passed directly as `icon` and `badge` to `new window.Notification()`. The OS/browser notification subsystem fetches `icon` and `badge` URLs directly — outside the page's JavaScript context — so the service worker's `fetch` handler never fires for them (the SW only intercepts fetches with a valid `event.clientId`, which these OS-initiated fetches lack). The homeserver returns HTTP 401, and the notification shows no icon or badge.
 - **Root Cause:** The SW auth-header injection is designed for page-initiated `/_matrix/client/v1/media/` fetches. It does not (and cannot) intercept fetches made by the OS notification subsystem. Room avatar URLs are passed to `Notification` without first converting them to an auth-agnostic form.
 - **Fix:** Before creating a `Notification`, fetch the avatar URL in-page (via the existing authenticated fetch path where the SW can inject headers), convert the response to a Blob URL (`URL.createObjectURL(blob)`), and pass the Blob URL as `icon`/`badge`. Alternatively, skip the avatar for notifications entirely and use the static app logo (already done for invite notifications via `LogoSVG`) to avoid the authenticated-media complexity.
 ## 🌸 Lotus Feature Internals Audit (Wave 2)
 > Deep audit of Lotus-specific hook internals, build scripts, and the avatar-decoration pipeline. All findings below are **[Claude_Found]**.
 ---
 **N113 — `addReminder`/`removeReminder` Read-Modify-Write Race Condition**
 - **File:** `src/app/hooks/useReminders.ts`, lines 52–68
 - **Status:** **OPEN** [Claude_Found]
 - **Issue:** Both `addReminder` and `removeReminder` call `readReminders(mx)` — a synchronous read from the Matrix client's local account-data cache — and then fire `setAccountData` asynchronously. If two calls overlap before either write has committed and the local cache updated (e.g. a user quickly adds two reminders, or adds one while a removal is in flight), both calls read the same stale baseline and the second write silently overwrites the first. Example: adding R1 and R2 in quick succession → both calls read `[]`, write `[R1]` and `[R2]` respectively → only R2 survives, R1 is lost.
 - **Root Cause:** No optimistic locking, no serial queue, and the read source (`mx.getAccountData()`) does not reflect uncommitted in-flight writes.
 - **Fix:** Use the React `reminders` state (passed as a parameter or captured in a `useRef`) as the source of truth for mutations instead of re-reading from the client cache. Alternatively, serialize writes through a promise queue so each `addReminder`/`removeReminder` awaits the previous `setAccountData` before computing the next state.
 ---
 **N114 — `ReminderMonitor` Calls `removeReminder` Fire-and-Forget; Network Failure Silently Drops the Reminder**
 - **File:** `src/app/pages/client/ClientNonUIFeatures.tsx`, lines 399, 413–414
 - **Status:** **OPEN** [Claude_Found]
 - **Issue:** Inside `ReminderMonitor.check()`, when a reminder fires the code immediately does `firedRef.current.add(key)` and then calls `removeReminder(r.eventId, r.timestamp)` without `await` and without a `.catch()` handler. If `removeReminder` fails (network error, 429 rate-limit, homeserver down), the reminder remains in account data but is permanently blocked from re-firing this session because its key is already in `firedRef`. The user's reminder is silently swallowed for the rest of the session; only a full page reload recovers it.
 - **Root Cause:** The promise returned by `removeReminder` is discarded. There is no error path that rolls back `firedRef.current` or reschedules the reminder for retry.
 - **Fix:** Make `check` an `async` function (or add a `.catch()` on the call), and only add to `firedRef` after `removeReminder` succeeds. On failure, omit the `firedRef` add so the reminder retries on the next poll tick.
 ---
 **N115 — `ReminderMonitor` 30 s Poll Interval Is Reset on Every `reminders` State Change, Delaying Near-Due Reminders**
 - **File:** `src/app/pages/client/ClientNonUIFeatures.tsx`, lines 394–428
 - **Status:** **OPEN** [Claude_Found]
 - **Issue:** `reminders` is listed in the `useEffect` dependency array (`}, [mx, reminders, setToast, removeReminder, mDirects]`). Every time a reminder is added, removed, or synced back from the server, React tears down the effect (clearing `setInterval`) and re-creates it, resetting the 30 s countdown from zero. A reminder due 1 s from now will not fire for up to 30 s if a reminder state change occurs 0.5 s before the due time — for instance, when the server's account-data echo arrives and updates `reminders`. In the worst case, rapid add/remove cycles can continuously defer the poll indefinitely (as long as new mutations keep arriving faster than 30 s).
 - **Root Cause:** `check()` closes over `reminders`, requiring it as a dependency; but the interval itself does not need to be recreated on every reminder change — only the closure does.
 - **Fix:** Store the latest `reminders` value in a `useRef` updated on each render, and read from the ref inside `check()`. Remove `reminders` from the `useEffect` dependency array. The interval is then created once per `mx`/handler change, and `check()` always sees the current snapshot via the ref.
 ---
 **N116 — `useCallSpeakers` Speaker Set Rebuilt From Mutation Batch Only — All Other Speaking Participants Are Dropped**
 - **File:** `src/app/hooks/useCallSpeakers.ts`, lines 20–44
 - **Status:** **OPEN** [Claude_Found]
 - **Issue:** The `MutationObserver` callback builds a fresh `Set<string>` from only the tiles present in the current mutation batch, then calls `setSpeakers(s)`. If participant A has been speaking for 10 s but their tile has not mutated recently, and participant B's tile mutates for an unrelated reason (e.g. a class change), the batch contains only B's tile. Even if B is not speaking, `s` is empty and `setSpeakers(s)` replaces the entire state — A disappears from the speakers set despite still speaking. The result is a constantly-flickering or always-empty speakers indicator.
 - **Root Cause:** Speaker state is derived from the delta (mutation batch) rather than the full current DOM state. Compare with `useRemoteAllMuted.syncState()` in the same file, which correctly re-scans all `[data-muted]` elements on every mutation rather than looking only at the mutated ones.
 - **Fix:** Replace the per-batch iteration with a full re-scan of all observed tiles on each callback: iterate all elements in `videoContainers`, check each for the `::before` speaking indicator, and build the new `Set` from currently-speaking tiles — not just the mutated ones.
 ---
 **N117 — `useCallSpeakers` Static `querySelectorAll` NodeList Misses Video Tiles Added to EC DOM Mid-Call**
 - **File:** `src/app/hooks/useCallSpeakers.ts`, lines 14–17
 - **Status:** **OPEN** [Claude_Found]
 - **Issue:** `callEmbed.document?.querySelectorAll('[data-video-fit]')` returns a static `NodeList` snapshot at the instant the `useMemo` evaluates. When a new participant joins mid-call and EC renders their video tile, that tile is not in the captured list. No `MutationObserver` is ever attached to the new tile, so the new participant can never be detected as a speaker for the remainder of the call. `callMembers` is a memo dependency and does update on join/leave, but there is a timing gap: `callMembers` may change before EC has finished rendering the new tile inside the iframe, so `querySelectorAll` at that moment still does not find the new tile.
 - **Root Cause:** Observing a static snapshot of tiles does not compose with EC's dynamically-updating DOM. `useRemoteAllMuted` avoids this entirely by watching `doc.body` with `{ subtree: true, childList: true }`, which automatically picks up new tiles without re-querying.
 - **Fix:** Replace the static-NodeList + per-tile-observer approach with a single body-level observer (same as `useRemoteAllMuted`), and re-scan all `[data-video-fit]` tiles on each relevant mutation.
 ---
 **N118 — `useCallSpeakers` Relies on Three Layers of Undocumented EC Internal APIs**
 - **File:** `src/app/hooks/useCallSpeakers.ts`, lines 15, 28–35
 - **Status:** **OPEN** [Claude_Found]
 - **Issue:** Speaker detection depends on three private Element Call implementation details that are not part of any stable EC API contract and can silently break on any EC version bump:
  1. **`[data-video-fit]`** — selector for video tile wrapper elements (internal EC data attribute).
  2. **`getComputedStyle(el, '::before').getPropertyValue('background-image') !== 'none'`** — speaking state is inferred from a `::before` pseudo-element's `background-image`. Any EC refactor of the speaking indicator (e.g. switching to a CSS class, `data-speaking` attribute, or canvas overlay) silently breaks detection with no error.
  3. **`el.querySelector('[aria-label]')?.getAttribute('aria-label')`** — assumes the first child with an `aria-label` carries the Matrix user ID; EC could equally label that element with a display name or a button description.
  When these internals change, `speakers` silently stays empty with no runtime error.
 - **Root Cause:** There is no stable programmatic API exposed by the EC iframe for speaker state; the implementation reverse-engineers EC's internal DOM/CSS.
 - **Fix:** Prefer EC's `postMessage` protocol if it exposes speaker events. At minimum, add a build-time assertion that pins the EC package version this mechanism was validated against (e.g. in `lotusDenoise` or a separate CI check), and file an upstream EC issue requesting a stable `data-speaking` attribute — which would match the pattern already used by `[data-muted]` in `useRemoteAllMuted`.
 ---
 **N119 — `syncDecorations.mjs` Treats Network Errors the Same as 404 — CDN Outage Silently Wipes Entire Catalog**
 - **File:** `scripts/syncDecorations.mjs`, lines 39–46, 56–65
 - **Status:** **OPEN** [Claude_Found]
 - **Issue:** `headCheck` catches all fetch exceptions (DNS failure, timeout, CORS error, TLS failure) and returns `{ ok: false, status: 0 }`. This is structurally identical to an HTTP 404 (`{ ok: false, status: 404 }`). The script classifies all non-ok results as "missing" and removes them from `avatarDecorations.ts`. If `drive.lotusguild.org` is temporarily unreachable when a developer runs `npm run sync:decorations`, every single decoration fails the HEAD check with `status: 0`, is marked missing, and is removed. The script writes an empty `avatarDecorations.ts`, logs "Done. Removed N entries from the catalog.", and exits 0 — permanently destroying the catalog in source control with no warning.
 - **Root Cause:** The `catch` block does not distinguish transient network failures from confirmed HTTP 404 responses.
 - **Fix:** Return a distinct value for network errors (e.g. `{ slug, ok: false, status: 0, networkError: true }`). Before writing the updated catalog, abort with `process.exit(1)` if any result has `networkError: true` — the CDN may be unreachable and removing all entries would be data loss. Only entries with a confirmed `status: 404` (file genuinely absent from the CDN) should be removed.
 ---
 **N120 — CDN URL Hard-Coded Separately in `syncDecorations.mjs` and `avatarDecorations.ts` — Can Drift**
 - **File:** `scripts/syncDecorations.mjs`, line 24; `src/app/features/lotus/avatarDecorations.ts`, lines 1–2
 - **Status:** **OPEN** [Claude_Found]
 - **Issue:** The Nextcloud CDN base URL (including the embedded share token `bHswJ9pNKp2t26N`) is defined twice: as `const CDN` in the sync script and as `export const DECORATION_CDN` in the runtime catalog. If the CDN is migrated (new provider, new Nextcloud share, rotated token), a developer must update both files. Missing one means the sync script probes the old URL while the runtime client fetches from the new one (or vice versa), silently producing a catalog that references unreachable assets. There is no test or lint check that enforces parity.
 - **Root Cause:** `syncDecorations.mjs` is a plain `.mjs` script that cannot directly `import` from a `.ts` source file at runtime, so the constant was copied instead of shared.
 - **Fix:** Extract the CDN URL into a shared `.mjs` config file (e.g. `scripts/decorationConfig.mjs`) that `syncDecorations.mjs` imports directly. Have `avatarDecorations.ts` read the same value at build time (via a Vite define/import, or by making the script write the constant into `avatarDecorations.ts` rather than hardcoding it). Alternatively, add a CI step that `grep`s both files and fails if the URLs differ.
 ---
 **N128 — `patch-folds.mjs` Emits `console.warn` Instead of `process.exit(1)` When Patch Target Is Not Found**
 - **File:** `scripts/patch-folds.mjs`, lines 21–23
 - **Status:** **OPEN** [Claude_Found]
 - **Issue:** When the target string `children: src(filled)` is not found in `node_modules/folds/dist/index.js` — because folds shipped an update that renamed or restructured this code path — the script logs `Warning: folds Icon patch target not found - may need updating.` and exits with code 0. The `postinstall` npm hook considers the install successful. The production build then ships the unpatched folds, where passing a non-function as `src` to `<Icon>` causes a runtime `TypeError: src is not a function` at any call site that relies on the guard. The failure is invisible at build and install time; it manifests only when the affected UI is rendered in production.
 - **Root Cause:** The mismatch branch uses `console.warn` (exit 0) rather than `process.exit(1)`, treating a broken build pre-requisite as a non-fatal advisory.
 - **Fix:** Replace the `console.warn(...)` + implicit exit-0 with `console.error(...)` followed by `process.exit(1)`. This causes `npm install` (and CI) to fail loudly, forcing the developer to update the patch target string before the build can proceed. The "already applied" branch (line 15) correctly exits 0 and does not need to change.
 ---
 ## 🔐 Security & Data Persistence Audit (Wave 2)
 > Deep audit of five files: `src/app/state/sessions.ts`, `src/client/initMatrix.ts`, `src/app/pages/client/ClientRoot.tsx`, `src/app/state/settings.ts`, `src/app/utils/sanitize.ts`. Findings N97–N100. Items already tracked elsewhere in this file are noted as FALSE POSITIVEs below.
 ---
 **N97 — `setFallbackSession()` stores the full Matrix access token in plaintext `localStorage` with zero mitigations**
 - **File:** `src/app/state/sessions.ts`, lines 32–68
 - **Status:** **OPEN** [Claude_Found]
 - **Issue:** `setFallbackSession()` persists four credentials to plaintext `localStorage` under fixed, predictable keys with no encryption, no `httpOnly`-cookie alternative, and no `sessionStorage` (which would at least not survive a browser restart). The four keys and their threat value:
  - `cinny_access_token` — the raw Matrix Bearer token; **sufficient alone** to fully impersonate the user with the homeserver: send/read messages, download E2E media, change account settings, join/leave rooms
  - `cinny_device_id` — the E2E device identifier; lets an attacker narrow the cross-signing key set needed to read encrypted history
  - `cinny_user_id` — the Matrix ID (`@user:server`)
  - `cinny_hs_base_url` — homeserver origin
  Any XSS payload executing in this origin can exfiltrate all four with four `localStorage.getItem()` calls. There is no Content-Security-Policy in the nginx/Caddy config files (existing open finding) that would limit script injection. `getFallbackSession()` (lines 49–68) also re-reads all four keys from `localStorage` on every boot — there is no in-memory cache that would allow the token to be removed from storage after the first load, so the credential window is permanent until logout.
  Additionally, `setFallbackSession()` performs **four sequential, non-atomic `localStorage.setItem()` calls** (lines 38–41). If the process is killed or the browser crashes between calls 1 and 3, `cinny_access_token` will be written to storage but the session will be incomplete; `getFallbackSession()` will return `undefined` (requires all four keys), leaving a stranded, fully-valid access token in `localStorage` that is never used or cleaned up.
 - **Root Cause:** The original multi-account Cinny path (now commented out) used an `atomWithLocalStorage` abstraction layer. The current single-account "fallback" path bypasses all abstraction and writes directly to raw `localStorage` with no protection.
 - **Fix:** Replace the four `setItem` calls with a single atomic write: serialize all four fields as one JSON object under a single key (`cinny_session`). This eliminates the partial-write window. For the XSS-resistance problem: migrate the access token to `sessionStorage` as a minimum (does not survive browser restart, limiting the exposure window on shared devices). For stronger protection: derive a per-device encryption key via `crypto.subtle.generateKey` and store it in `IndexedDB` (which already holds E2E keys via `IndexedDBCryptoStore`); encrypt the access token before writing to `localStorage`. The OIDC token-rotation flow (short-lived access tokens, refresh-token-only persistence) is the architecturally cleanest long-term fix.
 ---
 **N98 — Normal logout (`logoutClient` / `handleLogout`) calls `window.localStorage.clear()`, permanently wiping user preferences and unsent drafts**
 - **File:** `src/client/initMatrix.ts`, line 78 (`logoutClient`); `src/app/pages/client/ClientRoot.tsx`, line 133 (`handleLogout` inside `useLogoutListener`)
 - **Status:** **OPEN** [Claude_Found]
 - **Issue:** Both logout code paths call `window.localStorage.clear()`, which removes **every key** for the origin — not just the session credentials. Keys destroyed on every normal logout include:
  - `settings` — theme, notification preferences, keyboard shortcuts (`pttKey`, `deafenKey`), toolbar configuration, noise-suppression mode, accessibility settings, and all other `Settings` interface fields
  - `draft-msg-{roomId}` (one key per room) — unsent composer drafts for every room the user had open at logout time
  - `pip-position` — saved PiP window position
  - `status_msg_{userId}` / `status_expiry_{userId}` — persisted presence status message and auto-clear timestamp
  - `afterLoginRedirectPath` — post-login redirect
  A user who logs out and back in on the same device starts with a factory-reset app. This violates the standard expectation that app preferences persist across sessions (every comparable Matrix client and messaging app preserves preferences across logout). The `clearLoginData()` function (the explicit "wipe all data" reset path, surfaced in the UI as "Clear local data and reload") also calls `localStorage.clear()` — that usage is appropriate and expected — but `logoutClient()` / `handleLogout` should not share this behavior.
 - **Root Cause:** `localStorage.clear()` was chosen as a one-line logout implementation rather than selectively removing only the four session credential keys. No distinction is made between "end the session" and "factory reset."
 - **Fix:** Replace `window.localStorage.clear()` in both `logoutClient` (line 78) and `handleLogout` (line 133) with targeted removal of only the session credential keys:
  ```typescript
  ['cinny_access_token', 'cinny_device_id', 'cinny_user_id', 'cinny_hs_base_url'].forEach(k =>
    window.localStorage.removeItem(k)
  );
  ```
  Leave `settings`, draft keys, and all other preference keys intact. Reserve `window.localStorage.clear()` for the `clearLoginData()` path only.
 ---
 **N99 — `useSyncState` callback in `ClientRoot.tsx` only handles `PREPARED`; a sync `ERROR` before first sync completion freezes the app on the loading screen with contradictory UI**
 - **File:** `src/app/pages/client/ClientRoot.tsx`, lines 179–186; `src/app/hooks/useSyncState.ts`, lines 1–14
 - **Status:** **OPEN** [Claude_Found]
 - **Issue:** The `useSyncState` callback in `ClientRoot` only calls `setLoading(false)` for `state === 'PREPARED'`. The Matrix JS SDK can emit `SyncState.Error` before ever reaching `PREPARED` — for example when the device is offline at startup, the homeserver is unreachable, or the first `/sync` request returns a non-retryable server error. When this happens:
  1. `loading` remains `true` (never set to `false`)
  2. `<ClientRootLoading />` renders indefinitely, showing the "Heating up" spinner
  3. `<SyncStatus mx={mx} />` — rendered unconditionally **above** the loading conditional at line 191 — fires its own `useSyncState` listener and shows a "Connection Lost!" red banner simultaneously
  4. The user sees contradictory messages ("Connection Lost!" + "Heating up") with no recovery action visible from the loading screen. The only escape is the `ClientRootOptions` ⋮ menu (lines 192–125), which is a small icon button with Logout / Clear Cache — not discoverable without prior knowledge.
  Note: This is **distinct from the existing race-condition finding** (which concerns the listener missing PREPARED because it registers too late). Here the listener registers correctly and fires, but it fires with `ERROR` instead of `PREPARED`, and the callback ignores it.
 - **Root Cause:** The `useSyncState` callback is designed around a single happy-path terminal state (`PREPARED`). `SyncStatus` handles error states for the **post-PREPARED** reconnection UX, but does not replace the loading screen.
 - **Fix:** Extend the `useSyncState` callback to handle `SyncState.Error` and `SyncState.Stopped` by setting a separate `syncError` state, then render a dedicated error splash (parallel to the existing `loadState`/`startState` error dialog at lines 193–238) that shows a descriptive message and a Retry button that calls `startMatrix(mx)`:
  ```tsx
  useSyncState(mx, useCallback((state) => {
    if (state === 'PREPARED') setLoading(false);
    else if (state === 'ERROR' || state === 'STOPPED') setSyncError(true);
  }, []));
  ```
 ---
 **N100 — `sanitize.ts` allows unrestricted CSS class names on `<pre>` elements; `allowedClasses` not configured for `pre`**
 - **File:** `src/app/utils/sanitize.ts`, lines 69 and 156–163
 - **Status:** **OPEN** [Claude_Found]
 - **Issue:** `permittedTagToAttributes` includes `pre: ['data-md', 'class']` (line 69), permitting the `class` attribute on `<pre>` elements in Matrix `formatted_body` messages. However, `allowedClasses` (lines 156–163) restricts class names only for `code` elements (`language-*` patterns for Prism syntax highlighting). Per `sanitize-html` documentation: when `class` is listed in `allowedAttributes` for a tag but that tag has no entry in `allowedClasses`, **all class names are permitted** on that element. This allows a remote message sender to inject arbitrary class names onto `<pre>` blocks — e.g. `<pre class="some-cinny-class admin-notice">` — which could activate site-specific or folds-generated CSS rules keyed to those class names, override visual styling, or trigger `::before`/`::after` pseudo-element content defined in any loaded stylesheet. By contrast, the `code` element (which is typically the inner child of `<pre>`) is correctly restricted to `language-*` only, making the `pre` oversight inconsistent.
 - **Root Cause:** When Prism syntax-highlighting class support was added for `<code>`, the `<pre>` element was given a `class` passthrough (to allow `<pre class="language-python">` wrappers) but no corresponding `allowedClasses` whitelist entry was added for it.
 - **Fix:** Add `pre` to `allowedClasses` with the same `language-*` pattern already used for `code`:
  ```typescript
  allowedClasses: {
    code: ['language-*'],
    pre:  ['language-*'],
  },
  ```
 ---
 ### Wave 2 Security Audit — FALSE POSITIVES (re-examined, correctly handled)
 - **`setMaxListeners(150)` in `initMatrix.ts`** — already tracked as OPEN in the Infrastructure table above. Not duplicated here.
 - **`useSyncState` PREPARED race condition** — already tracked as OPEN in the Architectural Resilience table. N99 above is the distinct ERROR-before-PREPARED case, not a duplicate of the existing race-condition entry.
 - **`pushSessionToSW()` called without `await` in `logoutClient()`** — `pushSessionToSW` is synchronous; `postMessage` is fire-and-forget by design and requires no `await`. FALSE POSITIVE.
 - **`mx.initRustCrypto()` uncaught rejection in `initMatrix.ts` line 48** — the rejection propagates out of the async `initClient()` function and is caught by `useAsyncCallback` in `ClientRoot.tsx`, surfaced as `loadState.status === AsyncStatus.Error` with an error dialog and Retry button. FALSE POSITIVE.
 - **`style` attribute on `<font>` and `<span>` in `sanitize.ts`** — `transformFontTag` and `transformSpanTag` overwrite `style` entirely: the spread `...attribs` is followed by an explicit `style:` key that replaces any attacker-supplied value with a computed-safe string derived from regex-validated `data-mx-color`/`data-mx-bg-color` only. `allowedStyles` then further validates the result. FALSE POSITIVE.
 - **`href` allowing `javascript:` URLs** — `allowedSchemes: ['https', 'http', 'ftp', 'mailto', 'magnet']` plus `allowProtocolRelative: false` and `allowedSchemesAppliedToAttributes: ['href']` correctly block `javascript:`. FALSE POSITIVE.
 - **`<img src="...">` without scheme checking** — `transformImgTag` converts all non-`mxc://` `src` values to `<a href="...">`, at which point the href is scheme-checked; `javascript:` and `data:` are both rejected. `mxc://` images are correctly passed through. FALSE POSITIVE.
 - **`mentionHighlightColor` missing whitelist in `getSettings()`** — the value is consumed only via `document.documentElement.style.setProperty()` (CSS custom property), which cannot execute JavaScript regardless of value. FALSE POSITIVE.
 - **`dangerouslySetInnerHTML` / `innerHTML` XSS chain via `data-mx-maths`** — a full codebase grep confirms zero uses of `dangerouslySetInnerHTML` or direct `innerHTML` assignment in `src/app/`. Sanitized HTML is rendered via `html-react-parser`'s `parse()`, which produces React elements via `createElement`, not raw HTML injection. FALSE POSITIVE.
 - **`removeFallbackSession()` key-ordering issue** — `removeFallbackSession` is dead code in all active paths; it is only referenced in the commented-out multi-account migration block within `sessions.ts` itself. Active logout goes through `window.localStorage.clear()`. FALSE POSITIVE for the ordering concern; the broader `localStorage.clear()` behavior is tracked in N98.
 - **Settings atom contains sensitive data** — the `Settings` interface stores only UI preferences (theme, notification flags, keyboard shortcuts, toolbar config). No access tokens, cryptographic keys, or private message content are stored in the `settings` localStorage key. FALSE POSITIVE.
 ---
 ## 📞 Call System & Noise Suppression Audit (Wave 2)
 > Scope: `src/app/plugins/call/CallControl.ts`, `src/app/plugins/call/CallEmbed.ts`, `src/app/hooks/useCallSpeakers.ts`, `src/app/components/CallEmbedProvider.tsx`, `build/lotus-denoise.js`, `vite.config.js`.
 > Numbers N122–N127. N116–N118 already document `useCallSpeakers` speaker-detection fragility; findings below cover distinct issues not captured there.
 ---
 **N122 — `setMediaState` promise hangs permanently when EC omits a `DeviceMute` state-echo**
 - **File:** `src/app/plugins/call/CallControl.ts`, lines 185–193
 - **Status:** **OPEN** [Claude_Found]
 - **Issue:** The Promise returned by `setMediaState` can never resolve if EC does not emit a `DeviceMute` `fromWidget` state-update event in response to the host's mute command. After `await this.call.transport.send(ElementWidgetActions.DeviceMute, state)` resolves (EC has ACK'd the command), the function creates an inner Promise whose resolver is stored in `this.mediaStatePromiseResolver` — a field consumed only by `onMediaState` or by the NEXT call to `setMediaState`. If EC ACKs the command but does not subsequently fire a `DeviceMute` state-report back (the most likely trigger: the requested state already matches EC's current state and EC elides the echo, or EC is shutting down before broadcasting), the inner Promise is stranded forever. `applyState()` awaits this Promise at line 118 (`await this.setMediaState({...})`); the subsequent `this.setSound(this.sound)` and `this.emitStateUpdate()` calls at lines 122–123 are never reached. Because `forceState` (which calls `applyState`) is invoked fire-and-forget from `onCallJoined`, the practical result is that the initial deafen state and the first `StateUpdate` event emission are silently skipped on every call join when EC batches or omits the echo.
 - **Root Cause:** The single-slot `mediaStatePromiseResolver` architecture gates the mute operation's completion on an EC-originated event that is not guaranteed to fire for every host-initiated command.
 - **Fix:** Resolve the inner Promise directly when `transport.send()` returns — EC having replied already confirms the command was received and applied. Drop the `new Promise(...)` wrapper and return `data` immediately after `await transport.send()`. Keep `onMediaState` as the authoritative state-sync path (updating `this.state` and calling `emitStateUpdate`) but remove the `mediaStatePromiseResolver` field and its invocation from that handler entirely.
 ---
 **N123 — `focusCameraParticipant` tile click silently drops when EC spotlight layout isn't ready in 2 animation frames**
 - **File:** `src/app/plugins/call/CallControl.ts`, lines 396–401
 - **Status:** **OPEN** [Claude_Found]
 - **Issue:** After clicking `spotlightButton` to enter spotlight mode, `focusCameraParticipant` waits exactly two `requestAnimationFrame` callbacks (~32 ms at 60 fps) before querying the EC document for the target tile. If EC's React tree has not committed new spotlight tile nodes within that window — which occurs regularly on slower devices, during animated layout transitions, or when EC is simultaneously decoding video streams — `findTile()` returns `undefined` and the focus action is silently dropped. The user sees EC switch to spotlight mode but the requested participant is never pinned. There is no retry, no surfaced error, and the only signal is a DEV-only `console.warn`.
 - **Root Cause:** The double-rAF heuristic is a timing approximation, not a DOM-readiness guarantee. EC's React reconciliation and layout commit can exceed 32 ms.
 - **Fix:** Replace the double-rAF with a `MutationObserver` on `this.document.body` (childList + subtree) that waits for a `[data-testid="videoTile"]` element to appear, then calls `applyFocus()` and disconnects. Add a 600 ms hard-timeout fallback that calls `applyFocus()` and disconnects regardless, so the click is always attempted at least once even when tile rendering is slow.
 ---
 **N124 — Denoise shim `cleanup()` leaks the noise gate `AudioWorkletNode` processor thread when `USE_GATE=true`**
 - **File:** `build/lotus-denoise.js`, lines 235–244 and 267–281
 - **Status:** **OPEN** [Claude_Found]
 - **Issue:** When the noise gate is active (`USE_GATE=true`), `processStream` creates a `gateNode` (`AudioWorkletNode`) and wires it as `source → gateNode → mlNode → dest`. The `cleanup()` closure inside the inner `.then()` callback calls `source.disconnect()` and `mlNode.disconnect()` but never `gateNode.disconnect()`. `gateNode` is declared with `var` inside the outer `if (USE_GATE)` block — hoisted via `var` to the enclosing `.then()` function scope — and IS accessible in the inner callback via closure, but is simply absent from `cleanup()`. The AudioWorklet processor thread for the orphaned gate node continues running on the audio rendering thread until the EC iframe is destroyed. If EC's LiveKit client calls `getUserMedia` more than once within a session (e.g., a device switch mid-call), a new orphaned gate processor accumulates on each call, each consuming audio-thread CPU indefinitely.
 - **Root Cause:** `gateNode` is in closure scope but missing from the `cleanup()` body.
 - **Fix:** Add to `cleanup()`:
  ```javascript
  try { if (gateNode) gateNode.disconnect(); } catch (e) {}
  ```
 ---
 **N125 — Denoise shim `postMessage` uses wildcard `'*'` target origin**
 - **File:** `build/lotus-denoise.js`, lines 294–306 and 317–320
 - **Status:** **OPEN** [Claude_Found]
 - **Issue:** Both `lotus-denoise-status` `postMessage` calls use `'*'` as the `targetOrigin` argument, broadcasting the message to any frame that currently contains the EC iframe as a child regardless of its origin. If the Lotus EC widget URL is ever embedded by a third-party page (possible since it is same-origin and publicly routable), that page receives the denoise status payload (`{ type, active, model, nativeNS, gate }`). Using `'*'` violates the MDN/W3C `postMessage` security recommendation.
 - **Root Cause:** The shim has no reference to the parent origin at the point these calls are made. The `parentUrl` widget URL parameter — already present in `window.location.search` and parsed into `params` at line 27 — provides the correct target origin.
 - **Fix:** Extract `parentUrl` from `params` and use it as the target origin:
  ```javascript
  var targetOrigin = params.get('parentUrl') || window.location.origin;
  window.parent.postMessage({ ... }, targetOrigin);
  ```
 ---
 **N126 — PiP position restored from `localStorage` without type validation, silently producing `NaN` coordinates on corrupt data**
 - **File:** `src/app/components/CallEmbedProvider.tsx`, line 723
 - **Status:** **OPEN** [Claude_Found]
 - **Issue:** The saved PiP position is cast without runtime validation:
  ```typescript
  const savedPos = saved ? (JSON.parse(saved) as { left: number; top: number }) : null;
  ```
  If `localStorage['pip-position']` contains a corrupted value (from a prior bug, a different app version's format, or a developer edit), `JSON.parse` may succeed but return an object where `.left`/`.top` are `undefined`, strings, or non-finite numbers. `Math.max(0, Math.min(undefined, window.innerWidth - 280))` evaluates to `NaN`, yielding `el.style.left = 'NaN px'` — an invalid CSS value the browser silently ignores — and the PiP appears at an undefined position with no error surfaced.
 - **Root Cause:** TypeScript `as` casts do not validate at runtime; the parsed value's shape is never checked.
 - **Fix:** Add an explicit shape-and-finite guard:
  ```typescript
  const raw = saved ? (() => { try { return JSON.parse(saved); } catch { return null; } })() : null;
  const savedPos =
    raw != null &&
    typeof raw.left === 'number' && isFinite(raw.left) &&
    typeof raw.top  === 'number' && isFinite(raw.top)
      ? (raw as { left: number; top: number })
      : null;
  ```
 ---
 **N127 — ML noise suppression shim is never injected in `vite dev` mode; the ML feature is silently inactive during development**
 - **File:** `vite.config.js`, `lotusDenoise` plugin, lines 72–193
 - **Status:** **OPEN** [Claude_Found]
 - **Issue:** The `lotusDenoise` plugin only defines a `closeBundle` Rollup/Vite build hook, which executes only during `vite build`. In `vite dev`, `closeBundle` is never invoked: `lotus-denoise.js` is never copied, and EC's `index.html` is never modified to include the shim `<script>` tag. EC loads its original entry from `node_modules/@element-hq/element-call-embedded/dist/` without modification. When a developer enables ML noise suppression in Settings and joins a call, the `lotusDenoise=ml` URL parameter is correctly appended to the EC widget URL, but no shim intercepts `getUserMedia` inside the iframe and the mic is never routed through the ML pipeline. No error, warning, or status indicator surfaces this discrepancy; the `lotus-denoise-status` postMessage the shim would send never arrives, leaving any status display silently blank.
 - **Root Cause:** The plugin has no `configureServer` hook for the dev-server path; `viteStaticCopy` serves the original EC assets from `node_modules` without modification in dev mode.
 - **Fix:** Add a `configureServer` hook to `lotusDenoise` that installs two express middlewares: one serving `build/lotus-denoise.js` at `/public/element-call/lotus-denoise.js`, and one intercepting GET requests for `/public/element-call/index.html` that reads the original from `node_modules/@element-hq/element-call-embedded/dist/index.html`, injects the `<script src="./lotus-denoise.js"></script>` tag (mirroring the production replacement regex), and returns the patched HTML. This makes dev and production consistent for ML noise suppression testing.