diff --git a/docs/code-review/FULL_CODE_REVIEW_REPORT_2026-04-17.md b/docs/code-review/FULL_CODE_REVIEW_REPORT_2026-04-17.md new file mode 100644 index 0000000..b032e0c --- /dev/null +++ b/docs/code-review/FULL_CODE_REVIEW_REPORT_2026-04-17.md @@ -0,0 +1,581 @@ +# 🔍 UMS 项目全面代码审查报告 v5.0 + +**报告日期**: 2026-04-17 +**审查专家**: 代码审查专家 Agent +**项目分支**: main +**审查范围**: 全部实现文件(后端 Go 348+ 文件 + 前端 TS/TSX 196 文件) +**标准版本**: CODE_REVIEW_STANDARD_V4.0(8维度评估体系) + +--- + +## 📊 总体印象 + +### 一句话总结 +> **这是一个安全基础扎实、架构设计合理的 IAM 系统,但在并发安全、API 契约一致性和代码组织方面存在需要系统性修复的问题。整体质量从上次审查的 7.63 分有显著提升,但发现了若干新的 P0 级问题需要在上线前解决。** + +### 自动化验证门禁结果 + +| 检查项 | 结果 | 详情 | +|--------|------|------| +| `go build ./cmd/server` | ✅ PASS | 编译通过,0 错误 | +| `go vet ./...` | ✅ PASS | 静态分析通过 | +| `go test ./... -count=1` | ⚠️ FAIL | `internal/service` 规模测试超时(单次21s,5min总限制),单独运行该测试 PASS | +| 覆盖率 | ✅ **69.9%** | 超过 60% 门禁(上次 36.3%) | +| `govulncheck ./...` | ✅ PASS | 无已知 CVE 漏洞 | + +### 8 维度评分对比 + +| 维度 | 权重 | 上次(04-12) | 本次(04-17) | 变化 | 关键原因 | +|------|------|-------------|-------------|------|----------| +| ① 代码质量 | 15% | 7.0 | **7.2** | ↑+0.2 | 覆盖率大幅提升,但新发现并发问题 | +| ② API 契约 | 10% | 6.5 | **6.0** | ↓-0.5 | 响应格式不一致问题比预期严重 | +| ③ 安全强度 | 20% | 8.5 | **7.8** | ↓-0.7 | 新发现 CORS 默认配置 + LIKE 注入 + TOCTOU | +| ④ 前后端集成 | 10% | 8.0 | **8.2** | ↑+0.2 | 前端安全实践优秀,类型定义完整 | +| ⑤ 功能完整性 | 15% | 7.5 | **7.8** | ↑+0.3 | Webhook/Settings/TOTP 等功能已补齐 | +| ⑥ 业务专业性 | 10% | 8.5 | **8.3** | ↓-0.2 | 登录流程缺少 TOTP/设备信任检查步骤 | +| ⑦ 用户体验 | 10% | 8.0 | **8.0** | →持平 | 前端组件质量好,但巨型组件需拆分 | +| ⑧ 运维简洁性 | 10% | 6.5 | **6.5** | →持平 | 连接池硬编码等问题仍存在 | +| **综合得分** | 100% | **7.63** | **7.54** | ↓-0.09 | 新发现的 P0 问题拉低安全分 | + +--- + +## 🔴 P0 — 必须修复(阻塞合并/上线) + +共发现 **8 个 P0 问题**,按紧急程度排序: + +--- + +### P0-01: LIKE 查询 SQL 注入风险(3处) + +**📍 位置**: +- `internal/repository/operation_log.go:105` — Search() +- `internal/repository/device.go:241` — ListAll() +- `internal/repository/device.go:277` — ListAllCursor() + +**问题描述**: +```go +// 当前代码(危险) +search := "%" + params.Keyword + "%" +// ... +query = query.Where("name LIKE ?", search) +``` +LIKE 查询直接拼接用户输入,未转义 `%` 和 `_` 通配符。攻击者可输入包含这些特殊字符的关键词来操纵查询匹配行为。 + +**为什么是 P0**: SQL 注入的一种形式——虽然不是完整 SQL 注入,但属于模式操纵攻击,可被利用进行信息枚举和数据推断。 + +**影响**: 攻击者可构造特殊输入绕过关键词过滤,获取非预期的数据记录;在特定条件下可能影响业务逻辑判断。 + +**建议修复**: 复用已有的 `escapeLikePattern()` 函数(user.go 中已正确实现): +```go +import "strings" +func escapeLikePattern(s string) string { + s = strings.ReplaceAll(s, "\\", "\\\\") + s = strings.ReplaceAll(s, "%", "\\%") + s = strings.ReplaceAll(s, "_", "\\_") + return s +} +search := "%" + escapeLikePattern(params.Keyword) + "%" +``` + +**工作量**: 30 分钟 + +--- + +### P0-02: 登录失败计数器竞态条件(TOCTOU Race) + +**📍 位置**: `internal/service/auth.go:492-508` — incrementFailAttempts() + +**问题描述**: +```go +func (s *AuthService) incrementFailAttempts(ctx context.Context, key string) int { + current := 0 + if value, ok := s.cache.Get(ctx, key); ok { + current = attemptCount(value) + } + current++ // ← 读取后、写入前 + _ = s.cache.Set(ctx, key, current, s.loginLockDuration, s.loginLockDuration) + return current +} +``` + +经典的 **Check-Then-Act (TOCTOU)** 竞态条件。高并发场景下,多个攻击请求可以同时读取到相同的计数值(如都读到 4),各自 +1 后写入 5,但本应在第 5 次就触发锁定。 + +**为什么是 P0**: 暴力破解频率限制可被并发请求完全绕过。登录锁定机制形同虚设。 + +**影响**: 攻击者使用多线程/并发工具可在不触发锁定的情况下暴力破解密码。 + +**建议修复**: 使用原子递增操作: +```go +// 方案 A:在 cache 接口层提供 Increment 原子方法 +newVal, err := s.cache.Increment(ctx, key, 1, s.loginLockDuration) + +// 方案 B:使用 Redis INCR(如果底层是 Redis) +// 方案 C:使用 distributed lock 包装 Get+Set +``` + +**工作量**: 2-4 小时(取决于缓存层改造) + +--- + +### P0-03: Token 刷新黑名单写入失败被静默忽略 + +**📍 位置**: `internal/service/auth.go:786-795` — RefreshToken() + +**问题描述**: +```go +if s.cache != nil { + blacklistKey := tokenBlacklistPrefix + claims.JTI + if claims.ExpiresAt != nil { + remaining := time.Until(claims.ExpiresAt.Time) + if remaining > 0 { + _ = s.cache.Set(ctx, blacklistKey, "1", 5*time.Minute, remaining) + // ↑ 错误被忽略!如果 Set 失败,旧 token 仍然有效 + } + } +} +return s.generateLoginResponse(ctx, user, claims.Remember) +``` + +黑名单写入和新生成 Token 之间没有事务保证。如果 `cache.Set` 失败(网络超时、内存不足等),旧的 refresh token 在其 TTL 内仍然有效,可被重复用于刷新。 + +**为什么是 P0**: Token 泄露后无法可靠撤销。"Token 双花"漏洞——同一 refresh token 可多次使用。 + +**影响**: Token 泄露(如日志记录、中间人攻击)后,攻击者可在黑名单失效窗口内持续获取新的 access token。 + +**建议修复**: 将黑名单写入纳入错误传播链: +```go +if err := s.cache.Set(ctx, blacklistKey, "1", 5*time.Minute, remaining); err != nil { + return nil, fmt.Errorf("token revocation failed: %w", err) +} +return s.generateLoginResponse(ctx, user, claims.Remember) +``` + +**工作量**: 30 分钟 + +--- + +### P0-04: 密码重置验证码 Replay 攻击 + +**📍 位置**: `internal/service/password_reset.go:216-257` — ValidateResetCode / doResetPassword + +**问题描述**: 验证码校验通过后、密码重置完成前的窗口期内,验证码尚未删除: +```go +// 第 225 行:校验通过 +if subtle.ConstantTimeCompare([]byte(code), []byte(req.Code)) != 1 { ... } + +// ... 中间还有用户查询等操作(第 230-248 行)... + +// 第 254 行:才清理验证码 +s.cache.Delete(ctx, codeKey) +s.cache.Delete(ctx, cacheKey) +``` + +**为什么是 P0**: 同一验证码可被多次使用(Replay Attack)。攻击者可在窗口内并发提交多个重置请求。 + +**影响**: 第一次设置攻击者控制的密码,第二次受害者设置的密码——最终状态不可预测。 + +**建议修复**: 采用"验证即消耗"模式: +```go +// 校验通过后立即原子性删除验证码 +deleted := s.cache.Delete(ctx, codeKey) // 应返回是否成功删除 +if !deleted { return errors.New("验证码已被使用或已过期") } +// 再执行密码重置... +``` + +**工作量**: 1 小时 + +--- + +### P0-05: CORS 默认配置允许任意来源 + 凭证 + +**📍 位置**: `internal/api/middleware/cors.go:12-15` + `resolveAllowedOrigin()` + +**问题描述**: +```go +var corsConfig = config.CORSConfig{ + AllowedOrigins: []string{"*"}, // 通配符 + AllowCredentials: true, // 同时启用凭证! +} + +func resolveAllowedOrigin(origin string, ...) (string, bool) { + for _, allowed := range allowedOrigins { + if allowed == "*" { + if allowCredentials { + return origin, true // ← 反射任意 Origin + } + // ... + } + } +} +``` + +默认配置同时设置了通配符和凭证标志。当遇到 `"*"` + `AllowCredentials=true` 时,函数会反射**任何传入的 Origin** 值。 + +**为什么是 P0**: 如果部署时忘记显式配置 CORS 允许域名,任何恶意网站都可以发起跨域请求并携带用户认证凭证(Cookie/Authorization Header)。 + +**影响**: CSRF 类型攻击或数据窃取。结合 XSS 可导致完整的账户劫持。 + +**建议修复**: +1. 默认 `AllowCredentials` 应为 `false` +2. 或默认 `AllowedOrigins` 改为空列表(必须显式配置) +3. 启动时检测到 `*` + Credentials 组合时记录 WARN 日志 + +**工作量**: 1 小时 + +--- + +### P0-06: UpdateUser 缺少所有权检查(IDOR 越权) + +**📍 位置**: `internal/api/handler/user_handler.go:198-209` — UpdateUser + +**问题描述**: `PUT /api/v1/users/:id` 允许任何已认证用户更新**任意**用户信息(只要知道 user id)。路由中没有权限中间件保护,handler 中也没有 self-or-admin 检查。 + +**对比**: `GetUserRoles`(行356-369)正确实现了 self-or-admin 权限检查。 + +**为什么是 P0**: 任意已认证用户可修改系统中任何用户的邮箱和昵称——严重的越权漏洞(IDOR/CVE 级)。 + +**影响**: 信息篡改、钓鱼攻击(修改邮箱后重置密码)。 + +**建议修复**: 添加与 GetUserRoles 相同的权限检查逻辑: +```go +currentUserID := c.GetInt64("user_id") +targetID, _ := strconv.ParseInt(c.Param("id"), 10, 64) +if targetID != currentUserID { + // 检查是否有 user:manage 权限 + if !hasPermission(c, "user:manage") { + c.JSON(http.StatusForbidden, gin.H{"code": 403, "message": "无权限"}) + return + } +} +``` + +**工作量**: 30 分钟 + +--- + +### P0-07: Login 方法绕过 TOTP 和设备信任检查 + +**📍 位置**: `internal/service/auth.go:678-761` — Login() + +**问题描述**: 审查登录流程发现: +1. 密码验证通过后直接签发 Token(第 759 行) +2. **没有**检查设备信任状态 +3. **没有**触发 TOTP 二次验证 +4. 而 `VerifyTOTP` 方法明确提到"设备已信任时跳过 TOTP" + +这意味着纯密码登录完全绕过了 MFA(多因素认证)机制。 + +**为什么是 P0**: 启用了 TOTP 的账户可以通过纯密码登录直接获取 Token,MFA 形同虚设。 + +**影响**: 双因素认证被绕过,降低了账户安全性。 + +**建议修复**: 在密码验证通过后、Token 签发前增加: +1. 设备信任检查(未信任设备 → 要求 TOTP) +2. TOTP 验证(如果用户启用了 TOTP 且设备不受信) +```go +// 伪代码 +if user.TOTPSecret != "" && !isTrustedDevice(deviceID) { + // 不直接返回 token,返回 requires_totp 标识 + return &AuthResult{RequiresTOTP: true, UserID: user.ID} +} +``` + +**工作量**: 4-6 小时(涉及前后端协议变更) + +--- + +### P0-08: ListCursor 游标条件与动态排序字段解耦(数据错乱 BUG) + +**📍 位置**: `internal/repository/user.go:353-417` — ListCursor() + +**问题描述**: 游标分页固定使用 `(created_at < ? OR (created_at = ? AND id < ?))` 作为游标条件,但如果 `sortBy` 不是 `created_at`(例如按 `username` 排序),则游标条件与排序字段不一致。 + +**为什么是 P0**: 当 `sortBy != "created_at"` 时,游标分页会返回**重复或遗漏的数据**。这是一个确定性的逻辑 BUG。 + +**影响**: 用户列表翻页出现数据错乱、重复或丢失。 + +**建议修复**: +- 方案 A(最小改动):限制 ListCursor 只能按 created_at 排序 +- 方案 B(推荐):根据 sortBy 动态选择游标条件列 + +**工作量**: 1-2 小时 + +--- + +## 🟠 P1 — 必须修复(当天) + +共 **16 个 P1 问题**: + +### 安全相关(P1) + +**P1-01**: `internal/api/middleware/error.go:25` — 错误处理中间件泄露内部错误信息 +- 非 ApplicationError 类型的原始 error 直接返回给客户端 +- 可能泄露数据库连接字符串、内部堆栈等信息 +- **建议**: 未知错误返回通用消息 "Internal Server Error",详细错误仅记日志 + +**P1-02**: `internal/auth/oauth.go:212,311` — ExchangeCode / GetUserInfo 使用 context.Background() +- 断开请求上下文链路,取消信号无法传播,无法追踪慢请求 +- **建议**: 重构接口签名添加 context.Context 参数 + +**P1-03**: `internal/api/handler/export_handler.go:66` — 导出功能泄露内部错误详情 +- `"导出失败: " + err.Error()` 直接暴露给客户端 +- **建议**: 返回通用错误消息 + +**P1-04**: `internal/repository/login_log.go:113-116` — CountByResultSince() 错误被静默忽略 +- DB 查询 error 被 discard,返回值可能是错误的 count(0) +- 可能导致安全策略误判(基于失败次数判断是否锁账户) +- **建议**: 返回签名改为 `(int64, error)` 向上传播 + +### 业务逻辑相关(P1) + +**P1-05**: `internal/service/role.go:166-191` — DeleteRole 非事务性级联删除 +- 先删 role_permissions 再删 role,不在同一事务中 +- 如果第二步失败 → 孤立的权限关联数据 +- **建议**: 用数据库事务包裹或用 ON DELETE CASCADE + +**P1-06**: `internal/service/user_service.go:84-145` — ChangePassword 无 Token 失效机制 +- 修改密码后不使其他 session 的 token 失效 +- 已登录的其他设备/session 继续有效 +- **建议**: 密码修改成功后将用户加入 token 版本追踪黑名单 + +**P1-07**: `internal/repository/theme.go:92-98` — SetDefault 操作非原子性 +- 先清除所有默认标记,再设置新默认 → 并发下可能出现双默认或无默认 +- **建议**: 包裹在事务中 + +**P1-08**: `internal/database/db.go:63-66` — 数据库连接池参数硬编码 +- MaxOpenConns=10, MaxIdleConns=5 硬编码,配置文件中的 db_pool 设置无效 +- **建议**: NewDB() 中调用 applyDBPoolSettings(db, cfg) + +**P1-09**: `internal/repository/social_account_repo.go:204-206` — rows.Err() 未检查 +- rows.Next() 循环结束后缺少迭代错误检查 +- **建议**: 循环后添加 `if err := rows.Err(); err != nil { return nil, err }` + +**P1-10**: `internal/repository/user.go:332,407` — ORDER BY 字符串拼接风险 +- 虽然 sortBy 有白名单校验,但 sortOrder 只检查了 "asc" 大小写 +- **建议**: 使用 map 存储合法组合,避免拼接 + +**P1-11**: `internal/domain/announcement.go` — 缺少 GORM 标签 +- 与所有其他 Domain 实体风格不一致 +- **建议**: 补充 gorm 标签或注释说明故意省略的原因 + +### API 设计相关(P1) + +**P1-12 ~ P1-14**: 响应格式不一致(多处) +- `auth_handler.go`: ShouldBindJSON 错误返回 `{error: err.Error()}` 而非标准格式 +- `auth_handler.go:169`: Logout 返回 `{message: "logged out"}` 缺少 code/data +- `auth_handler.go:245`: CSRF Token 返回 `{csrf_token: ""}` 无 code 字段 +- `user_handler.go` 多处同样的问题 +- **建议**: 引入统一的 Response struct 或强化 ResponseWrapper 中间件处理 + +**P1-15**: 分页参数无上限限制(3个 handler) +- `user_handler.go:116`, `device_handler.go:81`, `log_handler.go:45` 的 page_size 参数无最大值约束 +- **建议**: 统一提取分页辅助函数内置 MaxPageSize=100 + +**P1-16**: `frontend/admin/src/app/providers/AuthProvider.tsx:189` — isAuthenticated 双重判断 +- 同时检查 React state (`effectiveUser !== null`) 和模块级状态 (`isAuthenticated()`) +- 异步更新可能出现短暂状态不一致 → UI 闪烁 +- **建议**: 统一单一数据源 + +--- + +## 🟡 P2 — 建议修复(本周) + +共 **18 个 P2 问题**,精选重点: + +| ID | 问题 | 位置 | 影响 | +|----|------|------|------| +| P2-01 | Repository 缺少统一接口抽象(DIP 违反) | internal/repository/ | 架构层面违反依赖倒置原则 | +| P2-02 | UserRepository.DB() 泄露底层 *gorm.DB | repository/user.go:35 | 破坏封装,可绕过 Repo 管理 | +| P2-03 | ProfileSecurityPage 组件 949 行巨型组件 | frontend/.../ProfileSecurityPage.tsx | 维护成本极高,应拆分为子组件 | +| P2-04 | UsersPage 20+ useState 状态爆炸 | frontend/.../UsersPage.tsx:58-91 | 应提取自定义 Hooks | +| P2-05 | AuthProvider 状态双重存储复杂度高 | frontend/.../AuthProvider.tsx:44-51 | React State + 模块级全局状态同步困难 | +| P2-06 | 时间字段未强制 UTC 存储 | domain 层多处 time.Now() | 多服务器部署时时间不一致 | +| P2-07 | Role.GetAncestorIDs N+1 查询 | repository/role.go:183 | 深层角色树性能差 | +| P2-08 | Webhook.Events 用 string 存储 JSON 数组 | domain/webhook.go:37 | 手动序列化容易出错 | +| P2-09 | Domain 层依赖外部 infraerrors 包 | domain/announcement.go:7 | Domain 层不够纯净 | +| P2-10 | ActivateEmail 使用 GET 执行状态变更 | auth_handler.go:141 | 违反 REST 语义,可被预取器触发 | +| P2-11 | ValidateResetToken 用 GET 传 token | password_reset_handler.go:67 | token 出现在 URL/日志中 | +| P2-12 | 静态文件目录直接暴露 /uploads | router.go:123 | 上传文件无需认证即可访问 | +| P2-13 | pagination/cursor.go Encode 忽略 JSON 序列化错误 | cursor.go:29 | 不符合防御性编程 | +| P2-14 | initDefaultData 循环创建权限无错误聚合 | database/db.go:139 | 启动时权限初始化可能静默失败 | +| P2-15 | JWT NewJWT 初始化失败返回损坏对象 | auth/jwt.go:76 | 调用者可能不检查 initErr | +| P2-16 | Webhook 服务 Publish/deliver 0% 覆盖率 | service/webhook.go | 核心投递链路无测试保护 | +| P2-17 | Redis 初始化放在 repository 包 | repository/redis.go | 包职责不清 | +| P2-18 | constants.go 映射表过大(AI平台映射混入) | domain/constants.go:73 | 职责混乱 | + +--- + +## 💙 P3 — 建议改进(Nice-to-have) + +- `repository/device.go:28` Create 事务开销(零值省略问题可用 Select/Omit 替代) +- `domain/custom_field.go:67` parseFloat 重新实现了标准库 strconv.ParseFloat +- `domain/user.go:55` 复合索引 idx_users_status_created_at 是否覆盖实际查询模式 +- 前端 `services/webhooks.ts:51` 使用 `.then()` 链式调用而非 async/await(风格不一致) +- `services/settings.ts:57` 同样使用 .then() 链式调用 + +--- + +## ✅ 做得好的地方 + +### 🏆 安全亮点(值得保持和表扬) + +1. **Argon2id 密码哈希**: 64MB 内存 / 5次迭代 / 4并行 —— 业界最佳实践 ✅ +2. **crypto/rand 全覆盖**: Token/JTI/盐值全部使用加密安全随机数,无 math/rand ✅ +3. **JTI 防枚举设计**: timestamp(8B hex) + random(16B hex),无法被预测或枚举 ✅ +4. **Token 滚动轮换**: refresh_token 每次刷新后旧值失效(虽然黑名单写入需加强)✅ +5. **access_token 内存存储**: 前端完全不使用 localStorage 存 token,防止 XSS 窃取 ✅ +6. **401 并发刷新锁**: 单例 Promise 模式,多个 401 请求共享一次刷新操作 ✅ +7. **CSRF 保护完整**: POST/PUT/DELETE/PATCH 自动注入 CSRF Token ✅ +8. **window 原生弹窗拦截**: alert/confirm/prompt/open 全部被安全拦截 ✅ +9. **常數时间密码比较**: 防时序攻击 ✅ +10. **JWT Secret 弱值检测**: isWeakJWTSecret() + 启动时 Warn 日志 ✅ +11. **Bootstrap 模式安全**: 缺失 JWT Secret 时使用临时随机密钥而非固定弱密钥 ✅ +12. **govulncheck 零漏洞**: 无已知 CVE ✅ +13. **前端零 any 类型**: 全量搜索确认无 `any` / `` / `as any` 使用 ✅ +14. **前端零 dangerouslySetInnerHTML**: 无 XSS 注入点 ✅ +15. **前端零 console.log**: 生产代码无调试日志残留 ✅ + +### 🏆 架构亮点 + +1. **RBAC + 角色继承 + 循环检测**: IAM 最佳实践的完整实现 +2. **密码历史防复用**: ChangePassword + ResetPassword 均接入 +3. **游标分页**: Keyset pagination O(limit),LL P99=53ms +4. **结构化错误分类**: ClassifiedError + ApplicationError 分层清晰 +5. **Webhook 投递系统**: HMAC-SHA256 签名 + 私有 IP 过滤 + 失败重试 +6. **E2E 测试闭环**: Playwright CDP 真实浏览器 7 个核心场景 + +--- + +## 📈 修复路线图 + +### Phase 1: P0 紧急修复(上线前必须完成,预计 2-3 天) + +| 任务 | 工作量 | 依赖 | +|------|--------|------| +| P0-01: LIKE 注入修复(3处) | 30min | 无 | +| P0-06: UpdateUser IDOR 修复 | 30min | 无 | +| P0-03: 黑名单写入错误传播 | 30min | 无 | +| P0-08: ListCursor 游标 BUG 修复 | 1-2h | 无 | +| P0-04: 验证码 Replay 修复 | 1h | 无 | +| P0-05: CORS 默认配置加固 | 1h | 无 | +| P0-02: OAuth context 传播 | 2h | 接口重构 | +| P0-07: Login 流程 TOTP 集成 | 4-6h | 前后端协议变更 | +| P0-02: 登录计数器竞态修复 | 2-4h | 缓存层改造 | + +**Phase 1 完成后预计综合评分: 8.1-8.3** + +### Phase 2: P1 修复(上线后第一周) + +| 任务 | 工作量 | +|------|--------| +| 错误信息泄露修复(3处) | 1h | +| 响应格式统一(引入统一 Response struct) | 4h | +| 分页参数上限统一 | 1h | +| DeleteRole 事务化 | 1h | +| ChangePassword Token 失效 | 2h | +| 连接池配置生效 | 30min | +| rows.Err() 检查补充 | 30min | +| AuthProvider 单一数据源 | 2h | + +### Phase 3: P2 技术债清理(本月内) + +- Repository 接口抽象(DIP 改造) +- 巨型组件拆分(ProfileSecurityPage + UsersPage) +- UTC 时间统一 +- OpenAPI/Swagger 规范完善 +- N+1 查询优化 +- 测试覆盖率提升至 80% + +--- + +## 📋 与上次审查(v4.0)对比 + +### 进步项 ✅ +- 测试覆盖率: 36.3% → **69.9%** (+33.6pp,跨越式提升) +- 新增功能: Webhook/Settings/TOTP/Theme/ImportExport 全部实现 +- 前端安全实践: window guard / CSRF / token storage 全面到位 +- 配置管理: JWT secret bootstrap 模式 / 弱密钥检测 完善 + +### 新发现问题 ⚠️ +- 并发安全问题(首次深入审查 Service 层发现) +- API 契约一致性比文档描述更差(实际代码审查 vs 自评) +- CORS 默认配置安全隐患 +- Login 流程 MFA 绕过 + +### 持续问题 +- Runbook 仍不完整 +- OpenAPI 规范缺失 +- pagination 包无测试 +- staticcheck 死代码 + +--- + +## 🎯 最终结论 + +| 评级 | 结论 | +|------|------| +| **当前评分** | **7.54 / 10** (良好偏上) | +| **能否上线** | ❌ **不建议当前状态上线** — 8 个 P0 必须先修 | +| **P0 修复后预估** | **8.1-8.3 / 10** (优秀,可发布) | +| **全部 P0+P1 修复后** | **8.5-8.7 / 10** (卓越) | +| **代码健康度趋势** | 📈 **上升**(覆盖率大幅提升 + 功能完整性改善 > 新发现问题) | + +**核心建议**: 这是一个**底子很好、安全意识强、但并发安全和 API 契约需要补课**的项目。P0 问题集中在安全敏感路径上(SQL注入变体、竞态条件、越权访问),建议优先修复后再进入生产环境。 + +--- + +*报告生成: 2026-04-17 22:50 CST* +*审查工具: 人工专家 Agent + 5 路并行子代理深度审查* +*下次建议复审: P0 全部修复后* +## 2026-04-18 复核附录 + +当本附录与本报告旧表述冲突时,以本附录基于 2026-04-18 新鲜命令证据和代码核查得到的结论为准。 + +### 最新命令证据 + +| Command | 2026-04-18 结果 | 说明 | +|--------|--------------------|------| +| `go build ./cmd/server` | `PASS` | 退出码 `0` | +| `go vet ./...` | `PASS` | 退出码 `0` | +| `go test ./... -count=1` | `PASS` | 退出码 `0`;总耗时约 `326.8s`;`internal/service` 用时 `316.011s` | +| `cd frontend/admin && npm.cmd run lint` | `FAIL` | 当前工作区在 `src/lib/device-fingerprint.test.ts` 和 `src/lib/http/index.test.ts` 有 5 个 ESLint 错误 | +| `cd frontend/admin && npm.cmd run build` | `PASS` | 退出码 `0` | + +### 报告真实性复核 + +| 项目 | 复核结果 | 结论 | +|------|---------------|-----------| +| 门禁摘要 | 部分过时 | 当前工作区的 `go test ./... -count=1` 已不再是红灯;前端 `lint` 现在转红,所以报告首页的门禁摘要已不再准确反映当前状态 | +| P0-01 LIKE 问题 | 已确认,但需收紧表述 | `internal/repository/operation_log.go` 与 `internal/repository/device.go` 中的问题真实存在,但更准确的表述应是基于 `LIKE` 的通配/模式注入,而不是任意 SQL 文本注入 | +| P0-02 登录失败计数竞态 | 已确认 | `incrementFailAttempts()` 仍是非原子的 `Get` + 自增 + `Set` 序列 | +| P0-03 refresh 黑名单静默失败 | 已确认 | `RefreshToken()` 仍忽略 `cache.Set(...)` 失败,存在 fail-open 风险 | +| P0-04 重置码 replay | 部分确认 | replay 窗口真实存在于手机重置路径 `ResetPasswordByPhone`;报告原始定位过宽,应精确指向短信重置流程 | +| P0-05 CORS 默认配置 | 已确认 | `internal/api/middleware/cors.go` 仍默认 `AllowedOrigins: [\"*\"]` 且 `AllowCredentials: true`,并会反射任意来源 | +| P0-06 UpdateUser IDOR | 已确认 | `PUT /api/v1/users/:id` 仍缺少路由层权限中间件和 handler 层 self-or-admin 授权 | +| P0-07 登录绕过 TOTP/设备信任 | 已确认 | `AuthService.Login()` 在密码验证后仍直接签发 token,没有经过 MFA 门禁 | +| P0-08 cursor/sort 不一致 | 已确认 | `UserRepository.ListCursor()` 仍固定使用 `created_at` 游标过滤,但允许其他排序字段 | + +### 分级任务可行性复核 + +| 任务 | 可行性 | 说明 | +|------|-------------|------| +| P0-01 LIKE 转义 | 高 | 小改动、低风险;应补 `%`、`_`、`\\` 的 repository 回归测试 | +| P0-02 原子失败计数器 | 中 | 可做,但需要扩展 cache API 或走 Redis 原子路径;不是 30 分钟级别改动 | +| P0-03 黑名单写入 fail-closed | 高 | 代码改动小,但需要明确产品决策:当 cache 不可用时,是拒绝 refresh,还是显式降级 | +| P0-04 重置码一次性消费 | 中 | 可做,但当前 cache API 缺少 compare-and-delete 语义;最稳妥的修法可能需要专门的原子消费 helper | +| P0-05 CORS 加固 | 高 | 改动直接;还应补启动期校验,拒绝 `* + credentials` 组合 | +| P0-06 UpdateUser 授权 | 高 | 在 handler/router 层都容易落地;应补 self、admin、未授权三类回归测试 | +| P0-07 MFA 登录门禁 | 中 | 可做,但这是前后端协议级变更;应设计明确的登录状态,而不是硬塞进当前成功响应 | +| P0-08 cursor 契约修复 | 高 | 可以限制 cursor 模式只支持 `created_at`,或改成按排序字段编码游标;最小安全修法是先拒绝不支持的排序 | + +### 路线图修正 + +- Phase 1 里的 `P0-02: OAuth context propagation` 分级挂错了。它对应的是 P1 中 OAuth 代码使用 `context.Background()` 的问题,不是登录失败计数竞态。 +- 在没有新鲜失败命令证据前,不应继续把 `go test ./... -count=1` 写成当前阻塞红灯。 +- 当前工作区 `npm.cmd run lint` 已经变红,因此不应再把前端门禁笼统表述为绿色。 + +### 应补充的后续任务 + +- 为每个确认接受的 P0 修复补回归测试,尤其是 `UpdateUser` 授权、refresh token 轮换失败处理、cursor 排序契约。 +- 将本报告与 `docs/status/REAL_PROJECT_STATUS.md` 对齐,消除 `AssignRoles`、`CreateAdmin/DeleteAdmin`、头像上传历史表述的冲突。 +- 增加一个专门的验证章节,明确区分“报告日期事实”和“当前工作区事实”,防止后续继续漂移。 diff --git a/docs/status/REAL_PROJECT_STATUS.md b/docs/status/REAL_PROJECT_STATUS.md index 739593b..2d86fae 100644 --- a/docs/status/REAL_PROJECT_STATUS.md +++ b/docs/status/REAL_PROJECT_STATUS.md @@ -1,6 +1,6 @@ # REAL PROJECT STATUS -## 2026-04-10 Review Update (TDD修复后) +## 2026-04-10 复核更新(TDD修复后) 本节记录 2026-04-10 TDD修复后的最新状态。 @@ -48,13 +48,13 @@ --- -## 2026-04-10 Review Update (原始) +## 2026-04-10 复核更新(原始) -This section supersedes older status summaries when they conflict with the -fresh 2026-04-10 review evidence in -`docs/code-review/PROJECT_REAL_COMPLETION_REVIEW_2026-04-10.md`. +当本节与更早的状态摘要冲突时,以 +`docs/code-review/PROJECT_REAL_COMPLETION_REVIEW_2026-04-10.md` +中的 2026-04-10 新鲜复核证据为准。 -### Fresh verification snapshot +### 最新验证快照 | Command | Result | Note | |------|------|------| @@ -70,7 +70,7 @@ fresh 2026-04-10 review evidence in | `cd frontend/admin && npm.cmd audit --omit=dev --json --registry=https://registry.npmjs.org/` | `PASS` | production vulnerabilities `0` | | `cd frontend/admin && npm.cmd run e2e:full:win` | `FAIL` | browser E2E wrapper still fails in the backend build/bootstrap stage | -### Current real blockers +### 当前真实阻塞项 - Full backend release-style verification is still red because of the `LL_001` login-log pagination SLA gate. - Browser-level E2E cannot yet be honestly claimed re-verified in the current review environment. @@ -81,15 +81,14 @@ fresh 2026-04-10 review evidence in - `CreateAdmin` still hardcodes admin role ID `1` and skips the stronger validation pattern already used by admin bootstrap. - Avatar upload remains a visible stub on the backend. -### Current honest external statement +### 当前诚实的对外表述 -The project now has a mostly green routine verification baseline, but it still -cannot be presented as fully release-closed. The correct statement is: +项目当前已经具备“大部分常规验证为绿色”的基线,但仍不能表述为“完整发布闭环”。更准确的说法是: -- backend short-path checks, frontend lint/build/tests, dependency audit, and local vuln scan are green -- one full backend SLA gate is still red -- browser-level E2E is still not freshly closed in this review -- RBAC/admin-management hardening and avatar upload remain open items +- 后端短路径检查、前端 lint/build/tests、依赖审计和本地漏洞扫描为绿色 +- 仍有一个完整后端 SLA 门禁为红灯 +- 浏览器级 E2E 在本轮复核中仍不能诚实宣称重新闭环 +- RBAC/管理员治理加固和头像上传相关治理项仍未全部关闭 ## 2026-04-09 二次复核更新(与审查报告对齐) @@ -1437,3 +1436,25 @@ powershell -ExecutionPolicy Bypass -File scripts/ops/validate-secret-boundary.ps - `npm.cmd run test:coverage` still exits successfully but prints one post-summary jsdom `AggregateError` network-noise line. - Evidence: - [`docs/evidence/ops/2026-03-28/quality/COVERAGE_REMEDIATION_20260328-140215.md`](/D:/project/docs/evidence/ops/2026-03-28/quality/COVERAGE_REMEDIATION_20260328-140215.md) +## 2026-04-18 复核附录 + +当本附录与下方旧状态表述冲突时,以本附录基于 2026-04-18 新鲜命令证据和直接代码核查得到的结论为准。 + +### 最新验证快照 + +| Command | Result | Note | +|------|------|------| +| `go build ./cmd/server` | `PASS` | 退出码 `0` | +| `go vet ./...` | `PASS` | 退出码 `0` | +| `go test ./... -count=1` | `PASS` | 退出码 `0`;总耗时约 `326.8s`;`internal/service` 用时 `316.011s` | +| `cd frontend/admin && npm.cmd run lint` | `FAIL` | 当前工作区在 `src/lib/device-fingerprint.test.ts` 与 `src/lib/http/index.test.ts` 有 5 个 ESLint 错误 | +| `cd frontend/admin && npm.cmd run build` | `PASS` | 退出码 `0` | + +### 当前真实情况 + +- `AssignRoles` 已通过 `ReplaceUserRoles(...)` 实现,不再是 stub。 +- `CreateAdmin/DeleteAdmin` 已实现,且具备事务性/保护逻辑,不应再表述为缺失。 +- `UploadAvatar` 已实现;当前剩余问题是 `/uploads` 的公开暴露面,而不是后端 stub。 +- `PUT /api/v1/users/:id` 仍缺少 self-or-admin 授权校验,依然是真实的 IDOR 风险。 +- 密码登录仍绕过 TOTP/设备信任门禁,依然是真实的发布阻塞项。 +- `UserRepository.ListCursor()` 仍允许与 `created_at` 游标谓词不一致的排序字段,依然是真实的正确性缺陷。 diff --git a/docs/team/PROJECT_EXPERIENCE_SUMMARY.md b/docs/team/PROJECT_EXPERIENCE_SUMMARY.md index fdf476c..1958f82 100644 --- a/docs/team/PROJECT_EXPERIENCE_SUMMARY.md +++ b/docs/team/PROJECT_EXPERIENCE_SUMMARY.md @@ -212,3 +212,60 @@ - 完整性检查必须逐项 - 覆盖率必须验证真实测试运行 - 类型引用必须验证定义存在 +## 2026-04-18 从复核到修复的经验 + +本附录记录了 2026-04-17 报告复核和 2026-04-18 文档对齐过程中提炼出的工程经验。 + +### 1. 评审报告不是实时状态页 + +- 一份报告可以在技术上仍然有价值,但它的门禁摘要会很快过时。 +- 团队必须把以下两类事实分开: + - 报告日期的发现 + - 当前工作区的真实门禁状态 +- 如果这两类事实混写,执行顺序和优先级判断会很快漂移。 + +### 2. 新鲜命令证据优先于继承结论 + +- `go test ./... -count=1` 曾在评审材料里被视为红灯,但新鲜执行后在当前工作区已经转绿。 +- 与此同时,前端 `lint` 已经重新变红。 +- 经验: + 在安排修复顺序前,必须先刷新真实门禁。 + +### 3. stub 转 live 会带来第二波风险 + +- `AssignRoles`、`CreateAdmin/DeleteAdmin`、`UploadAvatar` 已经越过了旧的“未实现”阶段。 +- 一旦转为 live,主导风险就会从“功能缺失”切换为: + - 授权边界 + - 事务性 + - 公开暴露面 + - 自操作 / 最后管理员治理 +- 经验: + live 实现必须被当作新的安全与治理面重新复核,不能因为 stub 消失就直接标记为“闭环”。 + +### 4. 发布阻塞往往是策略链断裂,不是没写代码 + +- 密码登录绕过 TOTP/设备信任校验,比很多显眼的“功能缺失”更像真实发布阻塞项。 +- refresh token 吊销 fail-open 也是发布阻塞项,即使代码路径本身已经存在。 +- 经验: + 在认证系统里,“已实现”不等于“完整”,只要安全策略链断了,就是关键缺陷。 + +### 5. 事实成立,不代表措辞可以粗糙 + +- LIKE 搜索问题是真实的,但把它笼统写成通用 SQL 注入,会夸大具体缺陷类型。 +- 密码重置 replay 问题也是真实的,但必须精确指出脆弱路径。 +- 经验: + 严重级别可以保持不变,但措辞必须更精确;精确措辞能加快修复,也能减少无效争论。 + +### 6. 主入口绿灯比局部绿灯更重要 + +- 局部命令成功,不能替代项目正式支持的主命令成功。 +- 包装层失败或顶层命令失败,就是真实项目失败,即使更深层子命令单独能过。 +- 经验: + 所有结论都必须对齐文档中声明的主验收入口。 + +### 7. 文档漂移会制造返工 + +- `REAL_PROJECT_STATUS`、评审报告和团队规范已经开始出现漂移。 +- 这种漂移会把下一轮修复引向过时优先级。 +- 经验: + 文档更新不是交付后的清理工作,而是交付本身的一部分。 diff --git a/docs/team/QUALITY_STANDARD.md b/docs/team/QUALITY_STANDARD.md index a3fbc96..9473153 100644 --- a/docs/team/QUALITY_STANDARD.md +++ b/docs/team/QUALITY_STANDARD.md @@ -302,3 +302,66 @@ npm.cmd run e2e:full:win - 验证方法:`grep -r “type IntegrationRedisSuite” internal/repository/` 必须返回定义位置。 - 禁止:测试文件引用未定义的类型,即使该测试有 `//go:build integration` 标签。 +## 2026-04-18 优化修复前治理基线 + +本附录定义了后续任何优化或修复工作开始前必须遵守的治理基线。若旧章节与本附录冲突,以本附录为准。 + +### 1. 当前门禁真相优先 + +- 任何“当前状态”“已绿”“阻塞中”“可继续”的表述,都必须绑定当前工作区的新鲜命令证据。 +- 报告日期事实与当前工作区事实必须分开书写。 +- 历史绿灯结果不能复用为当前门禁证据。 + +### 2. 当前优化修复的最低门禁 + +在声称一批修复已完成前,必须执行并记录: + +```powershell +go build ./cmd/server +go vet ./... +go test ./... -count=1 + +cd frontend/admin +npm.cmd run lint +npm.cmd run build +``` + +- 若改动涉及认证、会话、路由守卫、导航、`window` 防线或用户主流程,还必须执行: + +```powershell +cd frontend/admin +npm.cmd run e2e:full:win +``` + +- 超时不算通过。 +- 包装脚本失败就是真实失败。 +- 成功摘要后仍有浏览器原生弹窗噪声,不算干净通过。 + +### 3. 安全敏感修复必须 fail closed + +- refresh token 轮换在吊销持久化失败时必须 fail closed。 +- 与 MFA 相关的登录逻辑在 TOTP/设备信任策略完成执行前,不能签发最终 token。 +- CORS 必须拒绝危险默认组合,例如通配来源配合 credentials 开启。 +- 任何由用户可控 ID 定位资源的接口,都必须在路由层或 handler 边界做显式授权检查。 + +### 4. 正确性修复必须遵守契约 + +- cursor pagination 只能支持与游标谓词一致的排序;不支持的排序必须显式拒绝。 +- 多步写操作必须具备事务性,或具备显式回滚逻辑。 +- 基于缓存的安全计数器或一次性验证码必须使用原子语义,不能继续使用 best-effort 的读改写序列。 + +### 5. 文档同步是强制项 + +- 若新鲜验证改变了真实门禁状态,必须在同一批次更新 `docs/status/REAL_PROJECT_STATUS.md`。 +- 若评审改变了长期工程约束,必须在同一批次更新本文和 `docs/team/TECHNICAL_GUIDE.md`。 +- 若评审产出了可复用经验,必须在同一批次更新 `docs/team/PROJECT_EXPERIENCE_SUMMARY.md`。 + +### 6. 强制修复顺序 + +除非有更窄的依赖关系强制改变顺序,否则按以下次序执行: + +1. 刷新当前门禁真相并写入文档。 +2. 先修发布阻塞级别的安全与授权缺陷。 +3. 为每个确认接受的修复补回归测试。 +4. 重新执行受影响的完整门禁。 +5. 只有在以上完成后,才进入结构清理或一般优化。 diff --git a/docs/team/TECHNICAL_GUIDE.md b/docs/team/TECHNICAL_GUIDE.md index 64a8c4c..9ff926e 100644 --- a/docs/team/TECHNICAL_GUIDE.md +++ b/docs/team/TECHNICAL_GUIDE.md @@ -81,3 +81,75 @@ npm.cmd run e2e:full:win - `frontend/admin/scripts/run-playwright-auth-e2e.ps1`:需要优先保证文档支持的 `e2e:full:win` 入口自身稳定,而不是只验证子命令。 - `frontend/admin/src/components/common/ui-consistency.test.tsx`:原生弹窗噪声仍会污染测试结果,应继续清理。 - `internal/api/handler/user_handler.go` 与 `internal/service/user_service.go`:RBAC / 管理员治理逻辑需要持续按越权、事务、自删、最后管理员等维度审查。 +## 2026-04-18 优化修复入口 + +本附录是任何工程师或智能体在当前仓库状态下开启新一轮优化或修复批次时的强制入口。 + +### 1. 改代码前的阅读顺序 + +开始前按以下顺序阅读: + +1. `docs/status/REAL_PROJECT_STATUS.md` +2. `docs/code-review/FULL_CODE_REVIEW_REPORT_2026-04-17.md` +3. `docs/team/QUALITY_STANDARD.md` +4. `docs/team/PROJECT_EXPERIENCE_SUMMARY.md` + +用途: + +- `REAL_PROJECT_STATUS` 告诉你当前已经验证过的工作区真相。 +- `FULL_CODE_REVIEW_REPORT` 告诉你已经复核过的风险清单和任务分级。 +- `QUALITY_STANDARD` 告诉你当前必须遵守的工程约束。 +- `PROJECT_EXPERIENCE_SUMMARY` 告诉你哪些失败模式已经真实消耗过项目时间。 + +### 2. 先执行的新鲜命令 + +在做出任何“当前状态”判断前,先执行: + +```powershell +go build ./cmd/server +go vet ./... +go test ./... -count=1 + +cd frontend/admin +npm.cmd run lint +npm.cmd run build +``` + +如果本轮工作涉及认证、会话、路由守卫、导航、弹窗防线或用户主流程,还要执行: + +```powershell +cd frontend/admin +npm.cmd run e2e:full:win +``` + +### 3. 当前发布阻塞级关注点 + +在一般优化之前,优先处理这些区域: + +- `internal/api/handler/user_handler.go` + `UpdateUser` authorization boundary +- `internal/service/auth.go` + password login MFA/device-trust enforcement +- `internal/service/auth.go` + refresh-token revocation persistence failure handling +- `internal/api/middleware/cors.go` + unsafe default CORS behavior +- `internal/repository/user.go` + cursor/sort mismatch in `ListCursor` +- `internal/service/password_reset.go` + single-use verification code consumption semantics + +### 4. 修复批次工作规则 + +- 不要把历史绿灯当作当前证据。 +- 不要在没有分别验证的情况下,把门禁刷新、安全修复和重构混在同一个“已完成”结论里。 +- 修 bug 或安全问题时,没有对应回归测试,就不要把任务提升为“完成”。 +- 不要让包装脚本掩盖项目正式支持主命令的失败。 + +### 5. 文档更新规则 + +当一轮修复改变了真实结论时,必须在同一批次同步更新: + +- `docs/status/REAL_PROJECT_STATUS.md` +- 规则变化时更新 `docs/team/QUALITY_STANDARD.md` +- 产出可复用经验时更新 `docs/team/PROJECT_EXPERIENCE_SUMMARY.md`