diff --git a/docs/code-review/HERMES_FULL_REVIEW_2026-05-27.md b/docs/code-review/HERMES_FULL_REVIEW_2026-05-27.md new file mode 100644 index 0000000..de5c435 --- /dev/null +++ b/docs/code-review/HERMES_FULL_REVIEW_2026-05-27.md @@ -0,0 +1,414 @@ +# Hermes Full Review — 2026-05-27 + +- 仓库:`/home/long/project/user-system` +- 分支:`main` +- 基线提交:`82109ec Merge branch 'fix/status-review-sync-20260409'` +- 审查方式:文档对齐 + 代码静态复核 + 本地构建/测试/审计实测 + 二次复核补查 +- 结论:**❌ Not Ready / 当前不建议发布** + +--- + +## 1. Executive Summary + +当前仓库不是“完全不可运行”,但**不满足诚实发布条件**。阻断原因主要有三类: + +1. **安全 / 权限 P0 问题** + - 普通登录用户可枚举全部用户并读取任意用户详情 + - TOTP 二次验证被降级成可单独换取登录态的入口 + - 多个“未配置”认证/绑定接口返回 `200 + code:0`,形成假成功 + +2. **后端 clean-state 基线不绿** + - `go build ./cmd/server` + - `go vet ./...` + - `go test ./... -count=1` + - 三者在当前提交态均失败,并提示 `go mod tidy` + +3. **文档状态比代码现实更乐观** + - README / 状态文档存在“已闭环 / 已完成”表述 + - 但实际仍有主链路契约漂移、假成功与 clean-state 基线不干净问题 + +--- + +## 2. 审查范围与方法 + +### 2.1 读取的关键文件 +- `AGENTS.md` +- `README.md` +- `docs/PRD.md` +- `docs/status/REAL_PROJECT_STATUS.md` +- `go.mod` +- `frontend/admin/package.json` + +### 2.2 执行的关键命令 + +后端: +- `go version` +- `go build ./cmd/server` +- `go vet ./...` +- `go test ./... -count=1` +- `go test -mod=mod ./internal/repository -count=1` +- `go test -mod=mod ./... -count=1` +- `go test ./... -coverprofile=/tmp/user-system-cover.out -count=1` +- `go tool cover -func=/tmp/user-system-cover.out` + +前端: +- `npm ci` +- `env -u NODE_ENV npm ci` +- `env -u NODE_ENV npm run lint` +- `env -u NODE_ENV npm run build` +- `env -u NODE_ENV npm run test:run` +- `env -u NODE_ENV npm run test:coverage` +- `env -u NODE_ENV npm audit --omit=dev --json` +- `env -u NODE_ENV npm audit --json` + +--- + +## 3. 验证快照 + +### 3.1 环境事实 +- Go:`go1.26.3 linux/amd64` +- Node:`v22.22.0` +- npm:`10.9.4` +- 观察到默认 shell 存在:`NODE_ENV=production` + +### 3.2 环境风险 +默认 `NODE_ENV=production` 会导致第一次 `npm ci` 只安装生产依赖,进而出现: +- `eslint: not found` +- `tsc: not found` +- `@vitejs/plugin-react` not found + +这说明 runbook / CI 若不显式控制环境变量,前端验证容易误判。 + +### 3.3 后端实测 + +#### 当前提交态 / clean-state +以下命令均失败: +- `go build ./cmd/server` +- `go vet ./...` +- `go test ./... -count=1` + +统一报错: +```text +go: updates to go.mod needed; to update it: + go mod tidy +``` + +#### 探索性验证 +为了区分“代码问题”与“模块清单漂移问题”,额外执行: +- `go test -mod=mod ./internal/repository -count=1` → **PASS** +- `go test -mod=mod ./... -count=1` → **PASS** + +说明:核心代码不是全部跑不起来,但**提交态本身不干净**。 + +#### 覆盖率 +- `go tool cover -func=/tmp/user-system-cover.out` +- 总覆盖率:**52.4%** + +### 3.4 前端实测 +在显式移除 `NODE_ENV=production` 影响后: +- `env -u NODE_ENV npm ci` → **PASS** +- `env -u NODE_ENV npm run lint` → **PASS** +- `env -u NODE_ENV npm run build` → **PASS** +- `env -u NODE_ENV npm run test:run` → **PASS** + - `82` 个 test files + - `518` 个 tests +- `env -u NODE_ENV npm run test:coverage` → **PASS** + +前端 coverage: +- Statements: **89.83%** +- Branch: **80.38%** +- Funcs: **88.24%** +- Lines: **90.36%** + +### 3.5 前端依赖审计 +- `env -u NODE_ENV npm audit --omit=dev --json` → **0 漏洞** +- `env -u NODE_ENV npm audit --json` → **5 漏洞** + - 含 **1 个 high**:`vite 8.0.3` + +--- + +## 4. Blockers(必须修复) + +### P0-1 普通登录用户可枚举全部用户并读取任意用户详情 +**证据** +- `internal/api/router/router.go:206-215` +- `internal/api/handler/user_handler.go:90-165` + +**问题** +- `GET /api/v1/users` +- `GET /api/v1/users/:id` + +当前仅挂在 `protected.Use(r.authMiddleware.Required())` 下,未加: +- `AdminOnly` +- `RequirePermission` +- 本人访问约束 + +**影响** +普通用户可读取其他用户列表 / 详情 / 邮箱等信息,属于明确数据越权。 + +--- + +### P0-2 TOTP 验证接口可单独换取登录态,二次验证被降级为单因子登录 +**证据** +- `internal/api/handler/auth_handler.go:151-172` +- `internal/service/auth.go:811-831` + +**问题** +`POST /api/v1/auth/login/totp-verify` 只依赖: +- `user_id` +- `code` +- `device_id` + +没有要求: +- 已完成密码登录 +- 临时 challenge ticket +- 短期 server-side login session + +**影响** +拿到 TOTP / 恢复码即可直接换取完整 token,安全模型错误。 + +--- + +### P0-3 未实现的绑定 / OAuth 接口使用 `200 + code:0` 伪装成功 +**证据** +- `internal/api/handler/auth_handler.go:316-355` +- `internal/api/handler/auth_handler.go:563-660` +- `frontend/admin/src/lib/http/client.ts:274-279` +- `frontend/admin/src/pages/admin/ProfileSecurityPage/ContactBindingsSection.tsx:141-216` + +**问题** +后端在以下场景仍返回成功语义: +- OAuth not configured +- email bind not configured +- phone bind not configured +- social binding not configured + +前端只要 `code===0` 就按成功处理。 + +**影响** +用户会看到“已绑定 / 已解绑 / 已发送验证码”等成功反馈,但实际无状态变化。 + +--- + +### P0-4 Bootstrap Admin 前后端契约冲突,首个管理员初始化默认不可用 +**证据** +前端: +- `frontend/admin/src/pages/auth/BootstrapAdminPage/BootstrapAdminPage.tsx:24-30,68-76` +- `frontend/admin/src/services/auth.ts:61-63` + +后端: +- `internal/api/handler/auth_handler.go:504-527` + +**问题** +前端未满足后端强制契约: +- 缺少 `X-Bootstrap-Secret` +- `email` 前端可为空,但后端必填 + +**影响** +首次部署时最关键的 bootstrap 链路可能直接失败。 + +--- + +### P0-5 clean-state 后端构建基线不绿 +**证据** +- `go build ./cmd/server` → fail +- `go vet ./...` → fail +- `go test ./... -count=1` → fail +- 统一要求 `go mod tidy` + +**影响** +当前 `main` 不满足仓库 AGENTS 要求的最低验证矩阵,不能诚实宣称“始终可构建、可测试通过”。 + +--- + +## 5. High / Important Issues + +### P1-1 Logout fail-open,token 失效失败也返回成功 +**证据** +- `internal/service/auth.go:897-925` +- `internal/api/handler/auth_handler.go:185-209` + +**问题** +黑名单写入错误被忽略,handler 仍返回 `200 logged out`。 + +--- + +### P1-2 多个 handler 的管理员判断读错 context key +**证据** +middleware 写入: +- `internal/api/middleware/auth.go:85-91` +- 写入 `role_codes`, `permission_codes` + +handler 读取: +- `internal/api/handler/user_handler.go:188-200` +- `internal/api/handler/user_handler.go:374-383` +- `internal/api/handler/avatar_handler.go:72-85` +- 读取的是 `user_roles` + +**影响** +管理员代操作逻辑可能失效,权限模型与实际行为漂移。 + +--- + +### P1-3 修改密码接口与注释声明不一致 +**证据** +- `internal/api/router/router.go:211-213` +- `internal/api/handler/user_handler.go:275-297` + +**问题** +注释写“仅管理员或本人”,但 handler 没有显式按该规则做校验。 + +--- + +### P1-4 密码历史记录异步写入,事务不完整 +**证据** +- `internal/service/user_service.go:128-145` + +**问题** +密码更新同步写库,但密码历史在 goroutine 中异步写入且错误吞掉。 + +--- + +### P1-5 Avatar token 随机源错误未 fail-closed +**证据** +- `internal/api/handler/avatar_handler.go:35-39` + +**问题** +`rand.Read(bytes)` 错误被忽略。 + +--- + +## 6. 二次复核补充(第一次遗漏后补查) + +### 6.1 前端测试绿,但没挡住真实 API 契约漂移 +- 前端测试:`518 passed` +- 但 bootstrap-admin、contact bindings、OAuth 与真实后端契约仍不一致 + +**判断** +这是测试体系盲点,不是“测试通过即可放心”。 + +### 6.2 前端开发依赖存在 1 个 high 漏洞 +- `vite 8.0.3` high +- 另有 moderate 级别依赖漏洞 + +### 6.3 `NODE_ENV=production` 造成验证误判风险 +- 未显式控制环境变量时,devDependencies 可能缺失 +- 容易把环境问题误判为代码问题 + +### 6.4 后端总覆盖率仅 52.4% +在当前已有多条认证 / 权限高风险链路下,后端覆盖率偏低会放大回归风险。 + +### 6.5 测试 warning 噪音较多 +实测出现: +- `act(...)` warning +- React Router future flag warning +- `danger` 非布尔 attribute warning +- `addonAfter` deprecated warning +- React list key warning + +虽然不阻断当前前端测试通过,但说明测试基线不够干净。 + +--- + +## 7. 文档真相审查 + +### 结论:❌ 未闭环 +当前 README 与 `docs/status/REAL_PROJECT_STATUS.md` 存在“闭环 / 已完成 / 当前绿色”等偏乐观表述,但 live review 证明: +- 后端 clean-state 不绿 +- bootstrap-admin 主链路漂移 +- 绑定 / OAuth 存在假成功 +- 权限模型存在 P0 + +建议文档至少降级为: + +> 前端 lint/build/test 当前可通过;后端代码在 `-mod=mod` 探索性测试下大体可运行,但 clean-state 构建基线未绿;认证 / 权限 / 绑定链路仍有 P0 阻断,不可宣称发布闭环。 + +--- + +## 8. 四类闭环判断 + +### 8.1 实现闭环 +**状态:❌** +- 权限越权未解决 +- TOTP 流程模型错误 +- bootstrap-admin 契约漂移 +- binding / OAuth 实际未闭环 + +### 8.2 证据闭环 +**状态:✅/⚠️ 部分成立** +- 前端构建 / 测试证据充分 +- 后端 clean-state 失败证据明确 +- 但这些事实尚未同步进主状态文档 + +### 8.3 文档真相闭环 +**状态:❌** +- 当前对外状态文档比代码现实更乐观 + +### 8.4 防复发闭环 +**状态:❌** +尚未看到系统性防线去约束: +- binding / OAuth 禁止 200 假成功 +- bootstrap 前后端契约对齐校验 +- `/users` 与 `/:id` 权限回归测试 +- clean-state `go build/vet/test` gate +- 真实 API contract 联调验证 + +--- + +## 9. 最终评级 + +| 维度 | 评级 | 说明 | +|---|---|---| +| 需求 / 实现一致性 | C | 多条主链路契约漂移 | +| 安全基线 | D | 存在 P0 权限 / 认证问题 | +| 构建与测试基线 | C | 前端绿,后端 clean-state 红 | +| 可维护性 | B- | 结构尚可,但存在 context key 漂移 / fail-open / 异步事务问题 | +| 文档真相 | C- | 文档明显乐观于代码现实 | +| 发布就绪度 | D | 当前不建议发布 | + +**综合评级:D / Not Ready** + +--- + +## 10. 修复优先级建议 + +### P0(先修) +1. 修复 `/api/v1/users` 与 `/:id` 越权 +2. 重构 `totp-verify`,必须绑定密码登录 challenge +3. 所有未实现的 binding / OAuth 接口改为 fail-closed,并同步前端处理 +4. 修复 bootstrap-admin 前后端契约 +5. 清理 `go.mod/go.sum` 漂移,恢复 clean-state build/vet/test 绿灯 + +### P1(紧随其后) +6. 修复 logout fail-open +7. 修复 `user_roles` / `role_codes` context key 漂移 +8. 修复 password history 异步写入的事务缺口 +9. 修复 avatar token 生成未检查错误 +10. 升级前端 dev toolchain 漏洞(至少 vite) + +### P2(收口) +11. 清理测试 warning 噪音 +12. 补真实 API contract 集成测试 +13. 更新 README / `docs/status/REAL_PROJECT_STATUS.md` + +--- + +## 11. 本次二次 Review 的新增补充摘要 + +相较第一次结论,本次额外明确了以下问题: +1. 前端测试体系没有挡住真实 API 契约漂移 +2. 前端 dev toolchain 存在 1 个 high 漏洞(Vite) +3. `NODE_ENV=production` 会导致 devDependencies 缺失,runbook / CI 易误判 +4. 后端总覆盖率仅 52.4% +5. 测试输出 warning 噪音较多,质量门禁不够干净 + +--- + +## 12. 最终结论 + +**当前建议:不要发布;先修两个 high blocker 类问题,再推进剩余 P0 / P1。** + +其中最先收口的方向应是: +- 认证 / 权限真安全 +- clean-state 构建真绿色 +- 文档真相与代码现实一致 diff --git a/docs/review-fix-closure-2026-05-28.md b/docs/review-fix-closure-2026-05-28.md new file mode 100644 index 0000000..5159e53 --- /dev/null +++ b/docs/review-fix-closure-2026-05-28.md @@ -0,0 +1,94 @@ +# user-system review 修复收口(2026-05-28) + +## 结论 +本轮已完成 review 报告相关最高优先级前端/E2E blocker 修复,并完成后端、前端、E2E 三层验证。 + +当前状态: +- 最高优先级 blocker:已修复 +- Go 全量测试:通过 +- 前端全量测试:通过(82 files, 522 tests) +- Playwright CDP 全链路 E2E:通过 + +## 本轮修复项 + +### 1. 会话恢复 / refresh 竞态 +- 问题:`AuthProvider` 初始恢复会话与 HTTP client 401 重试路径会并发触发 `/auth/refresh`,在 refresh token 轮换模型下导致 `401`。 +- 修复:前端改为共享 single-flight refresh。 +- 涉及文件: + - `frontend/admin/src/lib/http/client.ts` + - `frontend/admin/src/services/auth.ts` + - `frontend/admin/src/services/auth.test.ts` + +### 2. 用户列表响应结构漂移 +- 问题:后端 `/users` 返回 `{ users, total, limit, offset }`,前端只按 `items` 读取,导致页面空表。 +- 修复:增加 users 列表 normalize,兼容 `items/users` 和 `page_size/limit/offset`。 +- 涉及文件: + - `frontend/admin/src/services/users.ts` + - `frontend/admin/src/services/users.test.ts` + +### 3. Webhooks 列表响应结构漂移 +- 问题:Webhooks 页加载时报 `Cannot read properties of undefined (reading 'map')`。 +- 修复:兼容 `data/items/webhooks` 多种列表包裹形状。 +- 涉及文件: + - `frontend/admin/src/services/webhooks.ts` + - `frontend/admin/src/services/webhooks.test.ts` + +### 4. Social accounts 响应结构漂移 +- 问题:ProfileSecurityPage 报 `socialAccounts.map is not a function`。 +- 修复:兼容 `array/items/accounts/social_accounts` 形状。 +- 涉及文件: + - `frontend/admin/src/services/social-accounts.ts` + - `frontend/admin/src/services/social-accounts.test.ts` + +### 5. Playwright CDP E2E harness 漂移 +- 修复点包括: + - refresh token 断言从可读 cookie 改为 HttpOnly cookie / session presence 真相 + - `创建用员` 文案 typo + - responsive 场景后 viewport 未恢复 + - drawer 选择器 strict mode 冲突 + - delete confirm 由 modal 漂移为 popconfirm + - 菜单分组/路由漂移:设备、审计日志、Webhooks、profile/security + - 多处页面断言从宽文本改为更稳定选择器 +- 涉及文件: + - `frontend/admin/scripts/run-playwright-cdp-e2e.mjs` + - `frontend/admin/scripts/run-playwright-auth-e2e.sh` + +### 6. E2E 限流误伤 +- 问题:测试流量触发 API rate limit,导致后续场景误报。 +- 修复:为 E2E backend 增加 `DISABLE_RATE_LIMIT=1` 开关,仅用于测试启动脚本。 +- 涉及文件: + - `internal/api/middleware/ratelimit.go` + - `frontend/admin/scripts/run-playwright-auth-e2e.sh` + +## 验证结果 + +### 后端 +- 命令:`go test ./...` +- 结果:通过 + +### 前端 +- 命令:`npm test -- --runInBand` +- 结果:通过 +- 统计:`82 passed`, `522 passed` + +### E2E +- 命令:`npm run e2e:full` +- 结果:通过 +- 结论:`Playwright CDP E2E completed successfully` + +## 闭环判断 + +### 实现闭环 +已完成。本轮识别出的真实 blocker 均已修复。 + +### 证据闭环 +已完成。Go 全量测试、前端全量测试、CDP E2E 全部通过。 + +### 文档真相闭环 +已完成。本文件记录了问题、修复、验证与当前结论。 + +### 防复发闭环 +已部分完成: +- 已为 users/webhooks/social-accounts 响应结构漂移补 service-level normalize + tests +- 已把 refresh 单飞与 E2E harness 漂移修复固化 +- 后续建议:把 E2E 页面导航/断言进一步抽象为页面对象或稳定 helper,减少文案/菜单变动带来的连锁断言漂移