diff --git a/docs/reviews/electron-gui-review.md b/docs/reviews/electron-gui-review.md new file mode 100644 index 0000000..d8d41eb --- /dev/null +++ b/docs/reviews/electron-gui-review.md @@ -0,0 +1,100 @@ +### CRITICAL (security / data leak / data loss) + +- **`stores/mailStore.ts` / `stores/sessionStore.ts` / `stores/contactStore.ts` are never cleared on account switch — cross-account mail disclosure.** + `AccountSwitcher.tsx:64-67` — the entire account-switch side effect is: + ```tsx + const pick = async (id: string) => { + setOpen(false); + await setActive(id); + await fetchInbox('all'); // ← only the inbox is refetched + }; + ``` + `setActive` (`stores/accountStore.ts:190-201`) calls `syncAuth()` which flips the `api/config` singleton (`API_BASE` + bearer) to the new account. But `useMailStore.sent`, `useMailStore.currentMail`, `useSessionStore.sessions`, `useSessionStore.currentSession`, `useSessionStore.currentSessionMails`, `useContactStore.contacts` and `.archivedContacts` are **never reset**, and every subsequent single-account request (`api.getSent()`, `api.getMail()`, `api.getSessionDetail()`, `api.archiveContact()`) now carries the *new* account's credentials. + Concretely: open Sent while on account A → switch to B → the Sent list still shows A's mail, and `MailView`/`ThreadView` render and forward A's bodies while the user believes they are in B. Archiving or approving from that stale view mutates B. `MailView.tsx:67` (`if (currentSession && currentSessionMails.length > 0)`) will *still* show A's whole session under account B. Nothing in `AccountSwitcher`, `accountStore`, or `App.tsx` calls `clearSession()`/`clearCurrentMail()` on switch (verified by grep: only `MailList.pick` / `ContactPanel.open` / `PermissionList.pick` do). + +- **`stores/appearanceSync.ts:40` — `lastUploadedImage` is a module-level singleton never keyed by account; an account's wallpaper is treated as already-uploaded under another account.** + ```ts + let lastUploadedImage = ''; + ``` + It is assigned in `pull()` (`:86`) and read in `push()` (`:110`). The wallpaper upload is guarded by `if (ok && bg.kind === 'image' && bg.imageDataUrl && bg.imageDataUrl !== lastUploadedImage)`. Sequence: account A sets wallpaper X (uploaded, `lastUploadedImage = X`) → switch to account B whose server record is `saved:false` → `pull()` takes the "server has no record" branch and calls `push()` with B's *local* background cache → the guard is satisfied by A's marker, so **B's wallpaper is never uploaded to B's server record** and `status` is set to `'synced'`. The wallpaper also survives on any device that syncs B. The file's own header comment enumerates three invariants and checks the account-scoped localStorage key, but this one account-blind module variable defeats invariant 2 for image uploads specifically. + +### HIGH (real bug hit in production) + +- **`components/MailView.tsx:746-758` — `PermissionPanel.submit` swallows the error entirely; an approval/rejection that failed leaves the UI claiming it was processed.** + ```ts + } catch (err) { + console.error(err); + } finally { + setBusy(false); + } + ``` + `setDecided(...)` runs *before* `await fetchInbox`, but a network/401/409 thrown by `api.decidePermission` skips it and lands in the `console.error` branch — no `setStaleWarning`, no error UI, and `decided` stays `''`, so the panel looks untouched and the user can re-click forever while the Agent waits. Every sibling call site in the same file (`ForwardBar.submit` :401-404, `ReplyBar.send` :1046-1051, `BudgetEditor.commit` :252) surfaces the message; this one is the odd branch and it is the only one that gates a security decision. + +- **`components/MailView.tsx:390-404` and `:404-408` — `ForwardBar.submit` has a double-submit hole combined with a failed refresh leaving the panel open.** + ```ts + const submit = async () => { + if (!to.trim() || busy) return; + setBusy(true); + ... + await api.forwardMail(...); + await Promise.all([fetchInbox('all'), fetchSent(), fetchSessions(), fetchContacts()]); + onClose(); + ``` + The `busy` guard only closes the window *after* React commits `setBusy(true)`. A fast double-click (or Enter-style rapid activation) issues two `forwardMail` POSTs — the second one is a duplicate forward. Worse, if any of the four follow-up `fetch*` calls rejects, the `Promise.all` rejects into the `catch`, `onClose()` never runs, the forward *did* succeed, and the user sees an error and will press Forward again. The refreshes are the wrong thing to gate the close on. + +- **`components/CalendarView.tsx:263-265` — ICS object URL revoked before the click download has consumed it.** + ```ts + a.click(); + URL.revokeObjectURL(url); + ``` + `revokeObjectURL` is called synchronously immediately after `a.click()` and with no anchor appended to the document. Firefox/WebKit start the download asynchronously; revoking in the same task invalidates the blob and the export silently produces a 0-byte or failed download. Needs the `setTimeout(revoke, 0)` (or a tick later) pattern used elsewhere in the repo. + +### MEDIUM (correctness or robustness gap) + +- **`components/CalendarView.tsx:101-106` — stale-while-revalidate race in `load()`: a slower earlier month can overwrite a newer one.** + ```tsx + useEffect(() => { setLoading(true); load(); }, [range.from, range.to]); + ``` + `load()` closes over `range` and unconditionally does `setEvents(r.events ?? [])` with no request-generation guard. Rapidly tapping 下一页/上一页 (or a swipe) issues overlapping `listCalendarEvents`; the responses can settle out of order and the grid shows a different month's events than the title says, with no error. `ThreadView.tsx:66-85` gets this right (`gen.current` + `myGen`); `CalendarView` has no equivalent. + +- **`components/ComposePage.tsx:160-163` — `setTimeout` after unmount and after `cancelCompose()`.** + ```tsx + setTimeout(() => { setOkMsg(null); cancelCompose(); }, 900); + ``` + No ref, no cleanup on unmount. Within the 900 ms the user can hit 取消/返回/清空/切换视图, unmounting the component; React logs an update-after-unmount on the setState and the pending timer keeps a closure over the whole compose page alive. Combined with the "清空" button (`:206-217`) which is not disabled while `sending`, this is reachable. + +- **`components/AdminUsersPage.tsx:93-96` and `components/KeyPanel.tsx:37-40` — same uncleared-timeout pattern** (`flash()` and the clipboard "copied" reset), both set state on an unmounted `AdminUsersPage` / `NewKeyBanner` after navigation or account switch. + +- **`components/MailView.tsx:820-846` — approval chips do not visually block while the request is in flight.** + ```tsx + disabled={busy} + className={... disabled:opacity-40 ...} + ``` + `disabled` on `