Files
MailUI4Agents/docs/reviews/push-and-gui-review.md
JianFeeeee 3b8204f356 docs(审查): 补上 pi 建议的第二格判据(pi 又改了一次它自己的标注)
上一提交只写进了一格判据,pi 随后把它的建议补上 —— 报告里现在是两格:

  · `TestHMSAccessTokenFailureDoesNotBurnQuota`          钉内部计数器 dayCount==0
  · `TestHMSQuotaSurvivesTokenFailureWithLimitOne`       钉用户看得见的行为

两层都要钉的理由:**计数器对而行为错是可能的** ——
运维会收到「达到每日推送上限」这种**误导性文案**,
而真实原因只是上一次网络抖动。这正是它建议单独加一格的原因。
2026-09-28 09:53:12 +08:00

373 lines
19 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters

This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.

# 系统推送链 与 GUI 审查报告
审查对象:
- **推送链(端到端)**:`client/harmony` 推送侧(`PushService.ets` / `PushContract.ts` / `EntryAbility.ets`)
+ `server/internal/push/*`(`push.go` / `hms.go` / `config.go`)+ `handler/push.go`
+ `repo/push_tokens.go` + `notify/mail.go`
- **GUI(Electron/React)**:`client/electron/src/`(约 14,100 行,10 个 store + 30 个组件)
基线:`node test/run-all.mjs` → 577 判据 / 565 通过 / 3 红(与本报告无关,见上份报告)
Go 侧未能执行 `go test`(本机无 `GOMODCACHE`/`GOPATH`),推送逻辑以**读码 + 对照既有测试**验证。
---
# 第一部分:系统推送
## 一、先说结论:推送链整体是健康的
设计上有几处**明确比一般实现高明**的地方,先列出来避免后续误改:
- **不写死华为**:`Notifier` 接口 + `Factory` 注册表 + `push.json` 配置式接入,
`RegisterType` 一个函数就能加厂商。
- **单项配错不拖垮服务**:`config.go:133-159` 逐项 `continue` 并打明确日志,
网关照常启动 —— 推送是锦上添花,不能变成单点故障。
- **推送是可选旁路且零开销**:`shouldDispatch` 抽成**纯函数**(`push.go:115-117`),
且注释诚实记录了变异验证曾经失败的原因(goroutine 竞态让错误理由也能绿)。
- **所有失败静默**:客户端 `PushService` 每一步 catch 都不打扰用户、不阻塞登录。
- **有效 token 自愈**:华为回 `80300007` / `illegal_tokens` 时自动删表(`hms.go:244-254`),
因为测试额度是**项目级**的,死 token 会白吃额度。
- **动态 `IN (...)` 的方言安全性我已验证**:`repo/push_tokens.go` 手拼 `$1,$2…` 占位符,
而 `db.go:7` 明确「SQLite 也支持 $1/$2,无需改写」⇒ 两种方言都安全,**不是 SQL 注入**
(占位符位置由 `i+1` 生成,值一律走参数绑定)。
## 二、推送链的真实缺陷
### 1.【HIGH】每日额度按 **token 数**扣,且在**实际投递之前**扣
`hms.go:192-194`:
```go
if !h.reserveDaily(len(tokens)) {
return fmt.Errorf("达到每日推送上限 %d 条(HMS_DAILY_LIMIT)", h.DailyLimit)
}
tok, err := h.accessToken(ctx) // ← 额度已经扣了
...
// 网络请求在下面,失败也只记日志
```
两个问题叠在一起:
- **计数单位错**:`reserveDaily(len(tokens))` 记的是 **token 个数**,而注释与变量名
(`dayCount`、`达到每日推送上限 N 条`)都说是「**条**」。华为的测试消息额度是
**按条消息**限的(一次 `messages:send` 一条消息,无论带几个 token)。
⇒ **3 个设备 1 封邮件就吃掉 3 条额度**,实际发出去 1 条。
多设备自部署用户会以约 1/设备数 的速度提前耗尽 1000 条/天。
- **扣在投递前**:`accessToken` 失败、HTTP 失败、华为返回非成功码 —— 这些**一条都没发出去**,
但额度已经扣掉了。而失败只 `log.Printf`(`push.go:174-176`),用户侧完全无感。
⇒ 一次网络抖动会静默烧掉配额。
**修法**:`reserveDaily(1)`(按批次计),并把预留挪到**确认拿到 access_token 之后**、
或失败时归还。
> **修复状态(2026-09-28)**:① 已修(`044a664`,`reserveDaily(1)`,有判据
> `TestHMSDailyLimitCountsMessagesNotTokens`)。
> ② **曾被声称已修但没落地** —— `044a664` 的 commit message 承诺「挪到确认能发之后」,
> diff 里却只改了传参,**调用位置仍在 `accessToken` 之前**,注释与代码自相矛盾。
> 之所以没被当场发现,是因为当时那批判据**造不出「accessToken 失败」这条路**。
> 现已真正落地(预留移到 `accessToken` 成功之后、发请求之前),
> 并补上两格判据:
> - `TestHMSAccessTokenFailureDoesNotBurnQuota` —— 钉**内部计数器**(`dayCount == 0`)
> - `TestHMSQuotaSurvivesTokenFailureWithLimitOne` —— 钉**用户看得见的行为**
> (`DailyLimit=1` 时一次 `accessToken` 失败后第二次仍然要能发出去)
>
> 两层都要钉:计数器对而行为错是可能的 —— 那会让运维收到「达到每日推送上限」这种
> **误导性文案**,真实原因却是上一次网络抖动。
> 变异验证:把预留挪回 `accessToken` 之前,**两格同时红**。
> 抓住这处不一致的是 pi。
### 2.【HIGH】HMS 通知载荷缺 `click_action`,锁屏点击可能带不出 `data`
`hms.go:206-217`:
```go
"message": map[string]any{
"token": tokens,
"notification": map[string]any{ "title": ..., "body": ... },
"data": string(data), // ← data 带了,但没有任何 click 行为声明
}
```
客户端**完全依赖** `data` 里的 `mail_id` 做跳转:
`EntryAbility.onCreate` / `onNewWant` → `PushService.routeFromWant` → `routeFromWant(params, ...)`
读 `parameters['data']`(`EntryAbility.ets:84-86`、`120-122`)。
而载荷里**没有 `click_action`、没有 `intent`、没有 `backgroundMode`**。
HMS v1 在「普通通知」形态下,点击行为由 `click_action` 决定(`1`=打开应用、`2`=打开页面、
`3`=触发 Action);**缺省时不同形态/系统版本对 `data` 是否透传到 `want.parameters` 的行为并不一致**。
项目里 `PushContract.ts:9-10` 自己写着「服务端推送是**至多一次、无幂等键**」,
`EntryAbility.ets:78-82` 也记录了 2026-09-15 为此专门加过冷启处理 ——
说明这条链此前已经因「跳转目标丢失」返工过一次,而**根因(载荷形态)没有变**。
**这正是「App 弹到前台、停在列表」的另一种表现形态。**
**修法**:显式声明点击行为(HMS v1 `click_action` 或按 Action 形态补 `intent`),
并**用真机 token 实测一次**确认 `data` 能进 `want.parameters` ——
`hms.go:50` 自己写着「我**无法在本机验证成功路径**(需要一台真机产出的 token)」,
这个未知项至今没有关闭。
### 3.【MEDIUM】通知标题把**完整邮件主题**写进锁屏
`push.go:36-38` 的设计声明:
> 「字段刻意少:推送只负责『通知栏那一行 + 点进去看哪封信』。
> **正文不进通知**,否则锁屏上就会露出邮件内容。」
但 `hms.go:211` 实际是:
```go
"title": "新邮件:" + truncate(n.Subject, 40), // ← 主题全文前 40 字
"body": n.From,
```
`Subject` 是**邮件主题本身**,标题里就带「报销 3 万走哪个账户」这类内容。
声明说"正文不进通知",实现却把**主题**放进去了 —— 这在锁屏上是可见的。
(`truncate` 限了 40 字,但 40 字足够泄漏一个机密项目名。)
**修法**:要么按声明改成固定文案(如「新邮件」+ 发件人),
要么把 `push.go` 的注释改成与实现一致 —— 现状是**注释在骗人**,这比任一选择都糟。
### 4.【MEDIUM】客户端写死 `provider: 'hms'`,与"配置式多厂商"自相矛盾
`PushContract.ts:13` + `PushService.ets:348,381` 无条件用 `PROVIDER_HMS`。
服务端明确支持一实例多厂商、且 `name` 可覆盖(`config.go:44-47`)。
但客户端把 `hms` 硬编码,`PushService.ts:360-361` 又只是**把** `resp.providers` 打进日志
就丢掉,从不据此选择。⇒ 服务端配了小米通道,鸿蒙客户端永远不会往它登记。
讽刺的是 `PushContract.ts:203-207` 的注释正在讲"不要硬编码只有 hms 合法"这个原则,
而调用处恰恰硬编码了。
### 5.【MEDIUM】`NotificationLedger` 只在 want 层去重,SSE 侧无跨通道去重
`PushContract.ts:9-10` 说「重复保护只能落在客户端」,`EntryAbility` 用
`NotificationLedger(50)` 按 `mail_id` 去重(`EntryAbility.ets:42`)。
但这只覆盖「点通知」这一条路径。用户实际会看到**两次**:
1. App 在前台 → SSE `new_mail` → `MainPage.ets:2911` `showToast('新邮件:…')`
2. HMS 通知同时下发 → 通知栏一条
两条路径**各自独立**、无共享去重台账 ⇒ 前台每次都"toast + 通知栏"双份。
`main.go` 端也没有按在线状态抑制推送的逻辑(`shouldDispatch` 只看 `Enabled()` 和收件人非空)。
### 6.【MEDIUM】`ledger` 是 `EntryAbility` 实例字段,50 条就会开始漏
`EntryAbility.ets:42` `private ledger: NotificationLedger = new NotificationLedger(50)`。
台账按 `mail_id` 去重且**有界(50)**。高流量场景(Agent 批量发信)下,
第 51 封之后旧 `mail_id` 被挤出 ⇒ 同一封邮件若被系统重放 `want`(HMS 在网络重试时确实会)
会**再次触发跳转**。属于可接受但需知情的取舍,`limit=50` 是否够值得确认。
### 7.【LOW】`client` 侧 `PushService` 在 `aboutToAppear` 之前就上报(已知,非新问题)
`EntryAbility.onCreate:106` 用 `ApiClient.getInstance(...)` 上报,而该实例**尚未 `init()`**
(`init()` 只在 `LoginPage.ets:101` 与 `MainPage.ets:3128` 调)⇒ 此刻无 token,服务端回 401。
`LoginPage.ets:168-192` 的 `reportPushToken` 已在登录后补报,**已修复**。
仅记录:`EntryAbility.ets:101` 那句注释仍写「`PushService.isEnabled`,默认关」,
而 `PushContract.DEFAULT_PUSH_ENABLED = true` —— **注释与实现相反**,会误导后续维护。
### 8.【LOW】`SessionApi` 同类问题:`null` 违反本仓约定
`api/SessionApi.ets:48` `proposal: RenameProposal | null = null`。
本仓 ArkTS 约定偏向 `undefined`。服务端用 `null` 表示"没有待处理建议",
客户端需要原样承载 —— 属于**契约强制**,但与 `PushContract.ts` 的 `| undefined` 风格不一致。
---
# 第二部分:GUI(Electron/React)
## 一、先说结论:GUI 的 XSS 面是干净的
我独立复核并确认:
- **全仓零** `dangerouslySetInnerHTML` / `innerHTML` / `insertAdjacentHTML`
- 三个 Markdown 渲染点(`MailView.tsx:146`、`:690`、`ComposePage.tsx:305`)统一用
`<Markdown remarkPlugins={[remarkGfm]}>`,**没有 `rehype-raw`** ⇒ 原始 HTML 被转义
- 唯一的动态 `href` 是 `Attachments.tsx:22` 的 `api.attachmentURL(a.attachment_id)`
(服务端签发的 id + `encodeURIComponent` 过的 token),唯一的动态 `src` 是本地生成的 JPEG data URL
- `electron/main.cjs:47-48`:`contextIsolation: true` / `nodeIntegration: false` ✔
- `main.cjs:47-48` 有 `preload`,未见 `openExternal` / `will-navigate` 缺口
**这条链上 `test/markdown-xss.test.mjs` 的不变量没有被绕过。**
## 二、GUI 的真实缺陷
### 1.【CRITICAL】切账号不清 store ⇒ **A 账号的邮件显示在 B 账号下**
`AccountSwitcher.tsx:56-62`:
```tsx
const pick = async (id: string) => {
setOpen(false);
await setActive(id);
await fetchInbox('all'); // ← 只重取了收件箱
};
```
`setActive`(`accountStore.ts:186-197`)会 `syncAuth(next, id)`,**把 `api/config` 单例的
`API_BASE` 与 bearer 翻到新账号**。此后:
- `useMailStore.sent`、`currentMail`
- `useSessionStore.sessions`、`currentSession`、`currentSessionMails`
- `useContactStore.contacts` / `archivedContacts`
**一个都没复位**。而后续单账号请求(`api.getSent()`、`api.getMail()`、`api.archiveContact()`)
会带上 **B 的凭证**。
后果:A 账号下打开发件箱 → 切到 B → 发件箱**仍是 A 的邮件**;
`MailView.tsx:67` 仍按 A 的会话渲染全文;**从这些陈旧视图点转发/归档/批准,改的是 B**。
grep 确认:只有 `MailList.pick` / `ContactPanel.open` / `PermissionList.pick` 会调
`clearSession()` / `clearCurrentMail()`,**账号切换路径一个都没有**。
**修法**:`setActive` 之后除 `fetchInbox` 外,还要
`clearCurrentMail()` / `clearSession()` / 清 `sent` / 清 contacts。
> 这与鸿蒙侧 `MailStore.clear()` **零调用**(上份报告第 3 条)是**同一个根因的两端**:
> 两个客户端都在"切换身份"时只刷新了主列表,没清其余状态。
### 2.【CRITICAL】`lastUploadedImage` 是模块级单例、不按账号分 ⇒ 第二个账号的壁纸**永不上传**
`appearanceSync.ts:40`:
```ts
let lastUploadedImage = ''; // 模块级,不带账号维度
```
`pull()` 第 87 行写它(从服务端图回填时),`push()` 第 115 行用它做跳过判据:
```ts
if (ok && bg.kind === 'image' && bg.imageDataUrl && bg.imageDataUrl !== lastUploadedImage) {
```
路径:A 账号把壁纸设为 X(上传成功 ⇒ 标记 = X)→ 切到 B 账号,B 的服务端记录是
`saved:false` ⇒ `pull()` 走 `appearanceSync.ts:69-76` 的"**把本地这份推上去**"分支
(`:74` 调 `push()`)⇒ 但跳过判据被 A 留下的标记满足 ⇒
**B 的壁纸永远不传**,而 `:120` 仍把 `status` 置为 `'synced'`。
⇒ B 的壁纸在其**其它所有设备**上都缺失,而界面上显示"已同步"。
讽刺的是该文件头明确把 localStorage 键按账号做了隔离,
却漏了这个变量 —— 它同样需要 `accountId` 维度。
**修法**:`lastUploadedImage` 改成 `Map<accountId, string>`,或直接存进按账号分区的
localStorage(与现有 key 策略一致)。
### 3.【HIGH】权限审批**吞掉错误** ⇒ 失败的"同意"看起来像成功了
`MailView.tsx:746-761`:
```ts
} catch (err) {
console.error(err); // ← 只进控制台
} finally { setBusy(false); }
```
`setDecided(...)`(`:753`)在 `await api.decidePermission` **之后**。
⇒ 请求失败时 `decided` 仍是 `''`,界面回到可点状态,**用户看不到任何提示**,
只会以为"点了没反应"而反复点,而 Agent 那边一直阻塞。
同文件其它提交路径(`ForwardBar.submit:401`、`ReplyBar.send:1046`、`BudgetEditor.commit:252`)
都正确地 `setError` 了 —— 只有这一处是例外。
**修法**:`catch` 里 `setError(...)` 并把错误渲染出来。
### 4.【HIGH】转发有双发窗口,且"关窗"依赖可能失败的刷新
`MailView.tsx:390-408`:
```ts
if (!to.trim() || busy) return;
setBusy(true);
...
await api.forwardMail(...);
await Promise.all([fetchInbox('all'), fetchSent(), fetchSessions(), fetchContacts()]);
onClose();
```
- `busy` 守卫要等 React 提交 `setBusy(true)` 之后才生效 ⇒ **快速双击会发出两个
`forwardMail`**(真转发,不是本地乐观更新)
- 四个刷新里任一 reject ⇒ `onClose()` 被跳过,**尽管转发已经成功** ⇒
用户看到报错、于是**再转一次**
**修法**:`setBusy` 之前用同步 ref 置位;`onClose()` 挪进 `finally` 或改成
`Promise.allSettled`。
### 5.【HIGH】ICS 导出的 object URL **同步 revoke** ⇒ 文件可能 0 字节
`CalendarView.tsx:262-267`:
```ts
const url = URL.createObjectURL(new Blob([ics], ...));
const a = document.createElement('a');
a.href = url; a.download = `...ics`;
a.click();
URL.revokeObjectURL(url); // ← 同一同步块内立即撤销
```
`a` 也从未插入 document。Firefox / WebKit 的下载是**异步**取 blob 的 ⇒
同步撤销会导致**静默失败或 0 字节 .ics**(Chromium 通常容忍,故容易漏测)。
**修法**:`setTimeout(() => URL.revokeObjectURL(url), 0)`,并把 `a` 挂到
document 后再 `remove()`。
### 6.【MEDIUM】日历加载无请求代次守卫 ⇒ 快速翻月时"标题与内容错配"
`CalendarView.tsx:101-106`:
```ts
useEffect(() => { setLoading(true); load(); }, [range.from, range.to]);
```
`load()` 里无条件 `setEvents(r.events ?? [])`。
快速点「下一页/上一页」⇒ 两个请求同时在飞,**后落地的覆盖先落地的** ⇒
标题显示 11 月而网格是 10 月的事件。
对比:`ThreadView.tsx:66-85` 用 `gen.current` / `myGen` **做对了**,
`CalendarView` 没有对应实现。
### 7.【MEDIUM】三个 `setTimeout` 无卸载清理
`ComposePage.tsx:160-163`(900ms 后清提示并关窗)、`AdminUsersPage.tsx:93-96`、
`KeyPanel.tsx:37-40`。`ComposePage` 那条可达(清空按钮在 `sending` 期间未禁用)。
### 8.【MEDIUM】预设缩略图全部渲染同一张自定义图
`BackgroundPicker.tsx:120-122`:
```tsx
className={`bg-preset-${p.id} absolute inset-0 block`}
style={{ backgroundImage: 'var(--bg-image)', backgroundSize: 'cover' }}
```
当 `kind==='image'` 时 `--bg-image` 有值 ⇒ **六个预设格子都显示同一张照片**,
而用户正在这一步里挑选预设。
### 9.【LOW】`api.attachmentURL` 把 token 放进 query
`api/client.ts:385-387` + `:118-122`。服务端**有意支持**
(`main.go:299` 用 `middleware.UserAuthAllowQueryToken`,`handler/events.go:17` 注释说明
这是浏览器 `EventSource` 专用回退)⇒ **不是缺陷**。
但代价是 token 会进浏览器历史/服务端访问日志,是已知的取舍,记录备查。
(鸿蒙侧 `ApiClient.getBytes` 的注释明确写了"不用 `?token=`,那会把密钥写进日志" ——
**两端做法不一致**,值得统一口径。)
---
## 三、本轮明确排除的疑点
- **推送 SQL 注入**:`repo/push_tokens.go` 的 `IN (...)` 占位符由序号生成、值全走绑定,
且两种方言都支持 `$N` ⇒ 安全。
- **Go 推送测试**:23 个测试覆盖了形状、分批、限额、无效 token 清理、配置容错 ——
覆盖面是好的;`TestHMSDailyLimitStopsSending` 用例也说明"按 token 计"是**已固化的行为**,
但**没人质疑这个计数单位**(见缺陷 1)。
- **`resetUI()` 登出不清数据 store**:单独看不可达(无账号时 `AccountSwitcher` 渲染 `null`),
真正的泄露面是**切账号**路径(缺陷 1)。不算独立缺陷。
- **Electron `MonthGrid key={i}`**:父级 `key={${anchor.getTime()}-${scale}}` 使其
每次导航都重挂载,索引不会跨数据变化 ⇒ 安全。
- **`ContactPanel` 双按钮**:非嵌套可交互元素,无点击吞没。
- **GUI 监听器/定时器清理**:`LoginPage.tsx:43-55`、`AddressInput.tsx:195-199`
(`{capture:true}` 成对)、`backgroundStore.ts:379-393`(两条路径都 revoke)⇒ 正确。
---
## 四、修复顺序建议
| 序 | 项 | 位置 | 理由 |
|---|---|---|---|
| 1 | 切账号清 store | `AccountSwitcher.tsx:56-62` | 唯一的**跨账号数据泄露**,改动最小 |
| 2 | 壁纸按账号分 | `appearanceSync.ts:40` | 静默丢数据 + 谎报"已同步" |
| 3 | 审批错误提示 | `MailView.tsx:756-757` | 权限决策失败却无反馈,Agent 一直阻塞 |
| 4 | 每日额度计数单位 | `hms.go:192` | 用户可感知的功能失效(提前耗尽配额) |
| 5 | HMS `click_action` + 真机实测 | `hms.go:206-217` | 需真机 token 才能关闭这个未知项 |
| 6 | ICS revoke 时机 | `CalendarView.tsx:267` | 导出功能在部分内核上失效 |
| 7 | 转发双发 / 关窗 | `MailView.tsx:390-408` | 数据重复 |
| 8 | 锁屏文案 | `hms.go:211` + `push.go:36-38` | 注释与实现不一致(二选一,别留现状) |
| 9 | 日历代次守卫 | `CalendarView.tsx:101` | 照抄 `ThreadView.tsx:66-85` 即可 |
| 10 | provider 硬编码 | `PushContract.ts:13` | 影响"配置式多厂商"这个卖点 |