MailUI4Agents/docs/reviews/electron-gui-review.md

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:

  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.

  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.

  } 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.

  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.

  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.

  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().

  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.

  disabled={busy}
  className={... disabled:opacity-40 ...}
  

  disabled on <button> does block clicks, so this is not a real double-submit — but setBusy(true) is only committed on the next render, and the panel keeps rendering the full chip set plus (below) the answer field, so a fast double activation within one frame can still fire two decidePermission calls. PermissionPanel has no synchronous in-flight ref; ThreadView.loadMore uses exactly that pattern (busy.current) for the same class of bug.

• components/BackgroundPicker.tsx:120-122 — preset thumbnails always paint var(--bg-image) on top of the preset gradient.

  className={`bg-preset-${p.id} absolute inset-0 block`}
  style={{ backgroundImage: 'var(--bg-image)', backgroundSize: 'cover' }}
  

  When a custom image background is active, --bg-image is set (backgroundStore.ts:262), so all six preset thumbnails render the image and none of them show their own gradient — the picker then shows six identical tiles while the user is choosing a preset. Should be omitted when kind === 'image', or the thumbnail span should be omitted.

• components/Attachments.tsx:96-100 — attachment list built with a stale items closure when uploads overlap. handleFiles reads items from the render closure and calls onChange([...items, ...added]); the picker is only blocked by uploading !== null on the button (:112), not on the <input type="file"> (:105-112), and the disabled prop passed by ComposePage (:320) is sending only. Rapid re-picks can drop an earlier selection. Same shape in ReplyBar.

FALSE POSITIVES YOU RULED OUT

• No dangerouslySetInnerHTML / innerHTML / insertAdjacentHTML anywhere in client/electron/src/ (grep returned zero hits). All three Markdown render sites (MailView.tsx:146, :690, ComposePage.tsx:305) use <Markdown remarkPlugins={[remarkGfm]}> with no rehype-raw, so raw HTML stays escaped and defaultUrlTransform neutralises javascript:/data: — exactly the invariant test/markdown-xss.test.mjs guards. Not a finding.
• No attacker-controlled href/src scheme. The only dynamic href is Attachments.tsx:22 api.attachmentURL(a.attachment_id), built from base() + a server-issued attachment_id + encodeURIComponent'd token; the only dynamic src is BackgroundPicker.tsx:155 with a locally produced image:/JPEG data URL. withToken (api/config.ts:118-122) percent-encodes the token.
• api/sse.ts EventSource URL is withToken(\${API_BASE}/events/stream`); API_BASE comes from the user's own account gateway (normalizeGatewayinlib/accounts.ts:55-67only prependshttp(s)://). Self-inflicted, not remote-supplied — I deliberately did not report the plaintext-http://` gateway default.
• MonthGrid/WeekGrid use key={i} (CalendarView.tsx:534, :609) — I checked and this is safe: the parent is key={${anchor.getTime()}-${scale}} (:390), so the entire grid remounts on every navigation and index identity never crosses a data change. Not a stale-row bug.
• ContactPanel ContactRow nests two buttons (the row <button onClick={onOpen}> at :230 and the .reveal action buttons at :243-258) — valid HTML, not nested-interactive, and the actions are not descendants of the row button, so no click swallowing. Not reported.
• MailList / PermissionList use onClick on <button> throughout (interactive elements, correct); useAccountStore(activeAccount) in AccountList.tsx:28 is a stable module-level selector, not an inline closure, so it is not a zustand v5 infinite-render trap.
• LoginPage.tsx:43-55 setInterval is cleared both in the effect cleanup and inside the setCountdown updater — no leak.
• AddressInput.tsx:195-199 removes scroll with matching { capture: true } — listener removal is correct.
• backgroundStore.loadImage (backgroundStore.ts:379-393) revokes the object URL in both onload and onerror — no blob leak.
• The LEGACY_BACKUP_KEY permanent residue and the legacy→per-account migration ordering in readStored are explicitly documented decisions (the file says "别再问这键谁写的") — not a finding.
• PermissionList.tsx:57-71 expanded ?? autoOpen freeze — the pendingKey effect deliberately re-opens newly arrived pending sessions; the comment explains why. Deliberate, not reported.
• App.tsx:105-109 resetUI() on logout clears the view state only. The underlying data stores are not cleared — I am not reporting that as a separate leak, because with no accounts configured the AccountSwitcher renders null and the store contents are unreachable without re-selecting the same gateway, so it is a cleanliness issue rather than a fresh disclosure on its own. (The real disclosure is the account-switch path above.)

Areas not reached

components/icons.tsx, ModelScopePanel.tsx, KeyPanel.tsx (beyond the snippet above), QuotaPanel.tsx (beyond line 120), ModelScopeTab tail, AccountPage.tsx lines 90-200, Sidebar.tsx, NarrowOnly.tsx, BackButton.tsx, PermissionChip.tsx, ConnectionIndicator.tsx, lib/lunar.ts (327 lines), lib/addressSuggest.ts body, types/index.ts. client/electron/test/CRITERIA.md was read only as far as the sections on structural criteria; I did not audit the test suite itself.