Files
pure-note/docs/review-round2.md
T
wangairnan 412821684b docs: 新增第 2 轮评审报告、D36 决策记录与 serve 旧词收尾
review-round2.md 全量修复清单;decisions.md 澄清 D6、新增 D36;design/acceptance 残留 serve → start(P2-14)。
2026-09-08 17:32:57 +08:00

129 lines
13 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.
# 《Pure Note v1.1 实现》审评报告(第 2 轮)
> 审评对象:docs/design.md v1.1 的完整实现(Go 后端 + React 前端 + 构建/部署链),含二进制/子命令改名(pure-note → pn)后的全量代码
> 审评日期:2026-09-08
> 审评方法:三路并行代码评审(store/数据层、httpapi/中间件、前端),关键发现全部人工复核源码与文档(decisions.md D6 等)确认后成稿
> 基线:docs/review-round1.md(v1.0 设计评审)——其 P0/P1 修复要求已在 v1.1 设计中落地,本轮核对实现是否兑现
---
## 一、总体结论
**无 P0。** 默认配置下未发现远程可利用的越权或注入漏洞:SQL 全部参数化、可见性矩阵(列表/详情/标签/RSS/sitemap/图片/meta 注入)落实到位、CSRF 双保险(token + Origin/Referer 严格模式,覆盖 login/logout)、`__Host-` Cookie、PHC 口令哈希与会话摘要、meta 注入经 html/template 转义——round1 的 4 个 P0 均已在实现层封死。
但存在 **5 个 P1 功能缺陷**:其中 1 个导致前端功能完全不可用却被测试漏过(图片上传)、1 个邻接导航查询逻辑写反、1 个文档承诺(D6)与实现矛盾(升级/回滚场景备份不可用)。另有 10 项 P2 加固项。
| 级别 | 定义 | 数量 |
| --- | --- | --- |
| P0 | 远程可利用的越权/注入/私密内容泄露 | **0** |
| P1 | 功能缺陷/契约破坏/文档承诺与实现矛盾 | **5** |
| P2 | 加固与健壮性 | **10**(表内合并条目) |
---
## 二、发现汇总
| # | 级别 | 位置 | 问题一句话 |
| --- | --- | --- | --- |
| 1 | P1 | internal/store/notes.go:188-210 | 上一篇/下一篇邻接查询 tie-break 写反且忽略 pinned,同秒更新漏链/错链,无测试覆盖 |
| 2 | P1 | web/src/lib/api.ts:39-41 | api() 对 FormData 强制 `Content-Type: application/json`,前端图片上传完全不可用 |
| 3 | P1 | internal/httpapi/server.go:102 | `/api/admin/*` 与 `/api/me` 缺 `Cache-Control: no-store`(中间件注释宣称已覆盖) |
| 4 | P1 | web/src/pages/AdminEdit.tsx:113 | 自动保存成功后 invalidate 连详情查询一起失效,refetch 回滚正在输入的内容并抑制下一轮自动保存 |
| 5 | P1 | cmd/pn/main.go:241/316/340 | 维护子命令总是迁移且带守卫、`--allow-newer` 被解析但忽略,与 D6 矛盾;回滚场景连备份都无法执行 |
| 6 | P2 | store/maint.go:36-98 | GC SELECT 与 DELETE 之间竞态:恢复/新引用的活数据会被物理删除 |
| 7 | P2 | store/maint.go:109-119 | 备份先以默认 umask 创建后 chmod 0600,私有内容短暂暴露窗口 |
| 8 | P2 | auth/auth.go:57-68 | VerifyPassword 不校验 PHC 参数上下界,畸形哈希可 panic/OOM |
| 9 | P2 | httpapi/public.go:161-174 | 图片数据行竞态缺失时 404 携带 public immutable 一年 |
| 10 | P2 | httpapi/feed.go:52-75 | 无公开笔记时 lastBuildDate 输出公元 1 年(非法 RSS 日期) |
| 11 | P2 | middleware/ratelimit.go:41 | sameOrigin 只比 host 不比 scheme |
| 12 | P2 | httpapi/admin.go:344-349 | image.Decode 无尺寸上限,<5MB 解压炸弹可 OOM 整站 |
| 13 | P2 | store/settings.go:64-67 | Sscanf("%d") 部分解析("10abc"→10),脏 page_size 被静默采用 |
| 14 | P2 | docs(design:567/590/628、acceptance:53、decisions D6/D28) | 改名后残留 5 处 `serve` 旧词;D6 内容与实现矛盾(见 P1-5) |
| 15 | P2 | web/src 多处 | Home 硬编码 page_size 使站点设置失效;auth.tsx 登录竞态覆盖 CSRF;401 无全局跳转;公共缓存失效缺失等 |
---
## 三、P1 详细说明
### P1-1 上一篇/下一篇邻接查询 tie-break 写反(store/notes.go:188-210)
**问题**:公开列表序为 `ORDER BY pinned DESC, updated_at DESC, id DESC`(notes.go:169)。邻接语义应在 (updated_at, id) 字典序上取紧邻项,但:
- prev(列表中更早一篇)写 `(updated_at > ? OR (updated_at = ? AND id < ?)) ORDER BY updated_at ASC, id DESC`——`id` 比较方向与列表序(id DESC)相反;同秒组内 `id DESC` 取到的是组内顶端而非紧邻项;
- next 同理(`id > ?` + `id ASC`)。
**后果**:同一秒有两篇以上笔记更新/创建(个人站点连续保存极常见,updated_at 为秒级粒度)时 prev/next 漏链或错链;且查询完全不参与 pinned 排序,置顶笔记附近跳错条目。调用方 public.go:103-104 用 `_` 吞掉错误,静默失效。该函数**无任何测试覆盖**(已确认)。
**修复**:prev 改为 `(updated_at > ? OR (updated_at = ? AND id > ?)) ORDER BY updated_at ASC, id ASC LIMIT 1`;next 改为 `(updated_at < ? OR (updated_at = ? AND id < ?)) ORDER BY updated_at DESC, id DESC LIMIT 1`;补表驱动测试(含同秒多篇、置顶参与排序的语义确认,必要时把 pinned 纳入邻接定义)。
### P1-2 前端图片上传完全不可用(web/src/lib/api.ts:39-41)
**问题**:`api()` 对所有带 body 的请求无条件补 `Content-Type: application/json`。Editor.tsx:22 的 `uploadImage` 用 FormData(注释明确「让浏览器自动设置 Content-Type(含 boundary)」),被此逻辑覆盖为 application/json → 后端 `ParseMultipartForm` 失败 → 400,粘贴/拖拽上传功能不可用。
**为什么测试全绿**:Go 集成测试直连 API 手工构造正确 multipart 头;验收浏览器走查未覆盖上传动作;前端无该路径的测试。
**修复**:`if (options.body && !(options.body instanceof FormData) && !headers.has('Content-Type'))`;补一个前端测试或至少把上传动作纳入浏览器走查清单。
### P1-3 /api/admin/* 与 /api/me 缺 no-store(server.go:102、public.go:21-28)
**问题**:middleware.go:129-136 注释宣称 NoStore「为 /api/admin/* 与 /api/auth/* 响应统一附加」,实际接线仅 `/api/auth/`(server.go:86)挂了 NoStore,`mux.Handle("/api/admin/", s.requireAdmin(adminMux))`(server.go:102)没有。`/api/me`(已认证时返回 csrf_token)同样无。
**后果**:登出后经 bfcache/历史回退(共用设备场景)可回看管理数据(笔记全文、回收站列表、settings、CSRF token);与设计 §9.2 目标直接冲突。
**修复**:`mux.Handle("/api/admin/", middleware.NoStore(s.requireAdmin(adminMux)))`;`/api/me` 已认证分支响应前补 `Cache-Control: no-store`(或对 /api/me 整路由统一 no-store)。
### P1-4 自动保存竞态回滚用户输入(AdminEdit.tsx:108-116、62-76)
**问题**:保存成功 `qc.invalidateQueries({ queryKey: ['admin'] })` 前缀匹配连当前编辑中的详情查询(key 形如 `['admin','note',id]`)一起失效 → refetch 返回服务端快照 → `useEffect([existing])` 执行 `setForm(...)` 把保存期间用户继续输入的内容回滚,并把 `dirtyRef.current = false`,抑制下一轮自动保存。每次自动保存都经历一次「输入被吞」窗口。
**修复**:onSuccess 仅 invalidate 列表键(如 `['admin','notes']`)与回收站键,不失效当前详情;或 refetch 落地时比较 `updated_at`/内容未变才允许重置 form。
### P1-5 维护子命令与 D6 矛盾:迁移与守卫始终执行、--allow-newer 被忽略(cmd/pn/main.go:241/316/340)
**问题**:decisions.md D6 承诺「backup/gc 不执行迁移、不做 user_version 守卫(纯数据操作)」,但实现中 passwd/backup/gc 一律 `store.Open(cfg.DBPath(), false)`,而 `store.Open` 无条件 `migrate()`(store.go:105)且 `false` 关闭 `--allow-newer`。`config.ParseMaint` 解析了 `--allow-newer` 却从未使用。
**后果**:
1. 用户升级到更高 schema 后回滚旧二进制,**`pn backup` 直接被守卫拒绝**——与「升级 SOP 第一步先备份」(review-round1 P1-13 修复)的语义冲突;
2. 错误 `--data-dir` 会静默创建并迁移出一个新库(维护命令不再是无副作用的纯数据操作);
3. 传入 `--allow-newer` 无任何效果且无提示。
**修复**:按 D6 拆分「打开 + 迁移(serve/init)」与「仅数据操作打开(passwd/backup/gc,跳过迁移与守卫)」,或至少让三个子命令真正透传 `cfg.AllowNewer`;同步修订 D6/代码注释,消除文档与实现二选一的漂移。
---
## 四、P2 简述
| # | 位置 | 修复建议 |
| --- | --- | --- |
| 6 | store/maint.go:36-98 | GC commit 阶段 DELETE 带原条件复查:笔记 `WHERE id=? AND deleted_at < ?`,图片 `DELETE ... WHERE id=? AND NOT EXISTS(SELECT 1 FROM image_refs WHERE image_id=?)`(或整体单事务)——防 SELECT 与 DELETE 之间恢复/新引用导致活数据被物理删除 |
| 7 | store/maint.go:109-119 | 备份文件以 0600 创建(先 `os.OpenFile(O_CREATE|O_EXCL, 0600)` 占位或临时目录生成后 rename),消除「先以 umask 创建后 chmod」的暴露窗口与存在性竞态 |
| 8 | auth/auth.go:57-68 | 解析 PHC 后校验 `1 ≤ t ≤ 10`、`1 ≤ p ≤ 8`、`m ≤ 1<<20`(KiB)、`len(want) == 32`,不合法直接返回 false——防 t/p=0 触发 argon2 panic、m 巨大 OOM(库被篡改场景) |
| 9 | httpapi/public.go:161-174 | 缓存头移到 `GetImageData` 成功之后设置;数据行缺失的 404 不带 public immutable(防恢复后仍被缓存 404 一年) |
| 10 | httpapi/feed.go:52-75 | 无公开笔记时 lastBuildDate 用当前时间或省略字段,不输出 `Mon, 01 Jan 0001` |
| 11 | middleware/ratelimit.go:41 | `sameOrigin` 同时比较 scheme(或至少把 http/https 视为不同源);与 HSTS 形成双层防护 |
| 12 | httpapi/admin.go:344-349 | 解码校验前先 `image.DecodeConfig` 校验宽高/像素总数上限(如 ≤8192×8192 或总像素 ≤2^26),防 <5MB 高压缩图解压炸弹 OOM |
| 13 | store/settings.go:64-67 | `Sscanf("%d")` 后校验「串已完整消费」(如 `%d%c` 探尾或 strconv.Atoi 全文),拒 "10abc" 类脏值 |
| 14 | docs 多处 | 改名收尾:design.md:567/590/628、acceptance.md:53 的 `serve` → `start`;decisions.md D6/D28 同步(D28 的 `serve` 改 `start`;D6 待 P1-5 修复后按新行为重写) |
| 15 | web/src 多处 | Home.tsx:10 改用 `/api/site` 的 page_size;auth.tsx:26-45 给 refresh 加请求序号/登录态变化判定,丢弃迟到匿名响应(StrictMode 双挂载放大窗口);api.ts 401 回调接入 AuthContext 触发跳转;写操作成功后同步 invalidate `['notes']`/`['tags']`/`['note']` 公共键;TagPage 补 error 分支;上传路径加前端测试 |
---
## 五、核实通过(维持不动)
1. **可见性单一可信点**:列表/标签/RSS/sitemap 统一走 `PublicNoteFilter`(store/notes.go:152),详情/图片/meta 出口 status + deleted_at 双层裁决,私有与不存在统一 404 防枚举,id 越界/负数均 404;
2. **SQL 注入面**:全部 `?` 参数化;仅 `VACUUM INTO` 路径经 `escapeSQLString`(引号加倍,SQLite 语义正确)与内部常量 PRAGMA 两处拼接,均无注入面;
3. **认证与 CSRF**:`__Host-` + Secure + HttpOnly + Lax、登录重建会话防固定、Origin/Referer 严格模式(缺头即拒)覆盖 login/logout、token 仅内存、改密校验旧口令且会话保持;
4. **口令与令牌**:Argon2id PHC 串(参数随哈希走)、会话库存 SHA-256 摘要、常量时间比较;
5. **上传防线**:声明类型白名单 → 魔数(SVG 拒绝)→ PNG/JPEG/GIF 再解码 → ≤5MB 哨兵 → sha256 去重;
6. **输出转义**:meta 注入经 html/template、JSON 默认 HTML 转义、RSS 经 encoding/xml + goldmark(非 unsafe) + bluemonday 双层清洗;
7. **迁移与并发**:user_version 上界守卫、仅追加式、DDL 与版本号同事务幂等;WAL + busy_timeout(5000) + synchronous(NORMAL) + foreign_keys(1) + `SetMaxOpenConns(1)`;
8. **构建/部署链**:Vite `base:'/'` + sync-assets + 指纹缓存 + 冒烟脚本(19 项断言)全链路打通;
9. **重命名**:pure-note → pn 的代码/脚本/deploy/README 残留已清理干净,本轮仅剩 docs 中 5 处 `serve` 旧词(P2-14);
10. **测试资产**:可见性矩阵、CSRF、限流、上传、回收站/gc、slug、设置白名单、meta 转义、迁移守卫均有自动化用例;`go vet`/`go test`/vitest/`make smoke` 全绿。
---
## 六、结论
v1.1 实现的骨架与安全设计经受住了第二轮对抗性评审,round1 的全部 P0 修复要求已在实现层兑现,无新增 P0。需优先处理的是 5 个 P1:其中 P1-1(邻接导航)与 P1-2(图片上传)是用户可直接感知的功能性缺陷,P1-3(no-store)与 P1-5(维护命令契约)是安全/运维承诺的缺口,P1-4(自动保存竞态)影响核心编辑体验。P1 修复后建议随本轮把 P2 清单中低成本项(8/9/10/13/14)一并落地,并补上 P1-1 的邻接查询测试。