long-agent/user-system
3
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity
Files
e49865df11e69f9cffced3dc8fa646175c26e596
user-system/docs/code-review/FULL_CODE_REVIEW_REPORT_2026-04-20.md
long-agent 3f3bb82f1d fix: v6 code review P0 auth/IDOR fixes + frontend regression patches 
Backend fixes:
- auth_handler: P0 认证逻辑修复
- ratelimit: 限速中间件增强 + 新增单元测试
- auth_service: 认证服务逻辑完善 + 新增测试
- server: server 配置增强 + 新增测试
- handler_test: 新增 handler 层集成测试
- auth_bootstrap_test: bootstrap 路径测试

Frontend patches:
- LoginPage/RegisterPage: CSRF + 表单交互修复
- BootstrapAdminPage: 引导流程修复
- DevicesPage: 设备管理页修复
- auth/social-accounts/users/webhooks services: 类型修正
- csrf.ts: CSRF token 处理修正
- E2E 脚本: CDP smoke + auth e2e 增强

Docs:
- FULL_CODE_REVIEW_REPORT_2026-04-20
- report-v6 执行计划
- REAL_PROJECT_STATUS 更新
- .gitignore: 新增 .gocache-*/config.yaml 排除

验证: go build/vet 0错误, go test 42/42 PASS, 0 FAIL
2026-04-23 07:14:12 +08:00

341 lines
14 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.
# UMS 项目全面代码复核报告 v6.0
**报告日期**: 2026-04-20
**审查范围**: 当前 `main` 工作区全部实现代码、旧报告未闭环问题、自动化门禁、系统化静态审查结果
**基线说明**: 本报告按日期拆分，作为 [FULL_CODE_REVIEW_REPORT_2026-04-17.md](./FULL_CODE_REVIEW_REPORT_2026-04-17.md) 的后续复核报告。凡与旧报告或旧附录冲突之处，以本报告基于 2026-04-20 新鲜命令证据和当前代码实现得到的结论为准。
---
## 一句话结论
项目在 2026-04-17 报告中的多数首轮 P0 缺陷已经被修复，但当前代码仍存在新的认证与授权断层，且旧报告中的一部分未修复问题仍未真正闭环。当前状态不适合宣称“全部问题已修完”或“可直接上线”。
---
## 2026-04-20 新鲜验证证据
| 项目 | 命令 | 结果 | 说明 |
|---|---|---|---|
| 后端构建 | `go build ./cmd/server` | PASS | 2026-04-20 23:07:51 +08:00 实跑通过 |
| 后端静态检查 | `go vet ./...` | PASS | 实跑通过 |
| 后端测试 | `go test ./... -count=1` | PASS | 全量通过，`internal/service` 仍是主要耗时段 |
| 前端 Lint | `cd frontend/admin && npm.cmd run lint` | PASS | 与 2026-04-18 红灯状态相比已恢复 |
| 前端构建 | `cd frontend/admin && npm.cmd run build` | PASS | 实跑通过 |
| 系统化静态检查 | `staticcheck ./...` | FAIL | 发现测试代码 `nil context`、潜在空指针、死代码等问题 |
| 安全静态检查 | `gosec ./internal/... ./cmd/...` | FAIL | 有真实问题，也有大量误报/高噪音结果，需要人工过滤 |
---
## 当前阻塞级问题
### P0-01: `TOTP` 二次验证链路缺少首因子绑定，形成独立登录入口
**位置**
- `internal/api/handler/auth_handler.go:151`
- `internal/service/auth.go:125`
- `internal/service/auth.go:811`
**问题**
- `/api/v1/auth/login/totp-verify` 只要求 `user_id + code + device_id`
- 服务端 `VerifyTOTPAfterPasswordLogin()` 只校验用户状态与 `TOTP` 码，然后直接签发完整 token
- 代码里虽然保留了 `TempToken` 字段，但当前登录闭环并未使用任何临时登录态或 challenge 票据
**影响**
- “密码登录后第二步验证”被降级成“知道用户 ID 且拿到有效 TOTP 即可直接登录”
- 这不是旧 P0-07 的原样复现，但本质上仍然属于 MFA 闭环未正确实现
**结论**
- 旧报告 P0-07 不能标记为“已完全修复”，应迁移为“修复方向已变化，但认证闭环仍未完成”
### P0-02: 设备接口存在成组 `IDOR`
**位置**
- `internal/api/handler/device_handler.go:114`
- `internal/api/handler/device_handler.go:147`
- `internal/api/handler/device_handler.go:183`
- `internal/api/handler/device_handler.go:214`
- `internal/api/handler/device_handler.go:392`
- `internal/api/handler/device_handler.go:474`
- `internal/service/device.go:121`
- `internal/service/device.go:158`
- `internal/service/device.go:163`
- `internal/service/device.go:181`
- `internal/service/device.go:204`
- `internal/service/device.go:236`
**问题**
- `GET/PUT/DELETE /devices/:id`
- `PUT /devices/:id/status`
- `POST/DELETE /devices/:id/trust`
这些接口的 handler 没有 owner/admin 校验，service 层也没有按 `user_id` 兜底约束，只按设备主键直接读写删除。
**影响**
- 任意已登录用户只要知道设备 ID，就可以读取、修改、删除、信任或取消信任他人设备
**结论**
- 这是本轮新增发现，严重程度等同发布阻塞
### P0-03: 修改密码接口缺少“本人或管理员”授权校验
**位置**
- `internal/api/handler/user_handler.go:275`
- `internal/service/user_service.go:84`
**问题**
- `PUT /api/v1/users/:id/password` 直接使用路径里的 `id`
- handler 没有 self-or-admin 校验
- service 只验证目标用户旧密码是否正确
**影响**
- 普通用户在知道目标用户旧密码时可直接修改目标用户密码
- 管理员也没有单独的安全重置路径，权限模型与接口语义混杂
**结论**
- 这是一条真实的授权缺口，应纳入 P0
### P0-04: 上下文协议漂移导致多处管理员路径失效
**位置**
- `internal/api/middleware/auth.go:90`
- `internal/api/middleware/auth.go:91`
- `internal/api/handler/user_handler.go:191`
- `internal/api/handler/user_handler.go:374`
- `internal/api/handler/avatar_handler.go:74`
**问题**
- 认证中间件当前只写入 `role_codes` / `permission_codes`
- 多个 handler 仍读取旧的 `user_roles`
**影响**
- 管理员跨用户更新资料
- 管理员查看他人角色
- 管理员代传头像
这些路径都会被错误判定为无权限。
**结论**
- 旧 P0-06 已做过一轮修复，但当前实现没有真正闭环，应以“部分修复后回归失效”迁移进新报告
### P0-05: OAuth handler 仍返回“200 假成功”占位响应
**位置**
- `internal/api/handler/auth_handler.go:316`
- `internal/api/handler/auth_handler.go:329`
- `internal/api/handler/auth_handler.go:342`
- `internal/api/handler/auth_handler.go:353`
- `internal/service/auth.go:939`
- `internal/service/auth.go:946`
- `internal/service/auth.go:1492`
**问题**
- handler 仍直接返回 `OAuth not configured` 或空 provider 列表
- service 层实际上已经存在 `OAuthLogin` / `OAuthCallback` / `GetEnabledOAuthProviders` 逻辑
**影响**
- API 层向前端暴露假成功语义
- 与仓库“禁止 fake success / fail closed”的运行时规则冲突
**结论**
- 这不是旧报告中的原编号问题，但属于当前实现真实性问题，应纳入高优先级修复
### P0-06: 游标分页与动态排序的契约仍未真正闭环
**位置**
- `internal/repository/user.go:353`
**问题**
- 当前实现只在 `sortBy == created_at` 时应用游标条件
- 其他排序字段下并不会报错，只是静默忽略游标条件
**影响**
- 前端如果带着非 `created_at` 排序继续请求下一页，得到的不是严格意义上的“下一页”
- 旧报告的“数据错乱”主因已经被收敛，但 API 契约仍然是不闭合的，容易出现重复页或错误分页预期
**结论**
- 旧 P0-08 不应从报告中移除，应以下降风险后的“残留契约缺口”形式迁移
---
## 从旧报告迁移的未闭环问题
下表只迁移“当前仍未真正闭环”的旧问题；已经明确修复完成的问题不再重复记为未完成。
| 旧编号 | 当前状态 | 新报告结论 |
|---|---|---|
| P0-06 UpdateUser IDOR | 部分修复后再次失效 | 迁移为 P0-04，上下文协议漂移导致管理员授权逻辑失效 |
| P0-07 Login 绕过 TOTP | 修复方向变化，但未闭环 | 迁移为 P0-01，`totp-verify` 未绑定首因子 |
| P0-08 ListCursor / sort | 风险下降但契约未闭合 | 迁移为 P0-06，非 `created_at` 排序下游标被静默忽略 |
| P1-12 ~ P1-14 响应格式不一致 | 仍未修复 | 保留为 P1，`auth_handler`、`password_reset_handler` 等多处仍返回非统一响应格式 |
| P2-12 `/uploads` 直接暴露 | 仍未修复 | 保留为 P2，`router.Setup()` 仍静态暴露上传目录 |
---
## 已确认修复完成的旧问题
以下问题在当前代码中已具备明确修复证据，不再迁移为“未修复项”：
| 旧编号 | 当前状态 | 证据 |
|---|---|---|
| P0-01 LIKE 通配/模式注入 | 已修复 | `internal/repository/operation_log.go`、`internal/repository/device.go`、`internal/repository/user.go` 已统一使用 `escapeLikePattern()` |
| P0-02 登录失败计数竞态 | 主路径已修复 | `internal/service/auth.go:492` 已改用 `cache.Increment()`；但降级 fallback 仍保留非原子路径，见“残留风险” |
| P0-03 refresh 黑名单 fail-open | 已修复 | `internal/service/auth.go` 中黑名单写入失败已向上返回错误 |
| P0-04 手机重置 replay | 基本修复 | `internal/service/password_reset.go` 在验证码校验通过后先删除 key 再继续流程 |
| P0-05 CORS 默认危险组合 | 已修复 | `internal/api/middleware/cors.go` 默认值已改为空 origins + `AllowCredentials=false` |
| P1-01 错误处理中间件泄露内部错误 | 已修复 | `internal/api/middleware/error.go` 对未知错误返回通用消息 |
| P1-03 导出接口泄露内部错误 | 已修复 | `internal/api/handler/export_handler.go` 已改为通用错误文案 |
| P1-04 CountByResultSince 静默忽略错误 | 已修复 | `internal/repository/login_log.go` 已返回 `(int64, error)` |
| P1-07 Theme SetDefault 非原子 | 已修复 | `internal/repository/theme.go` 已改用事务 |
| P1-08 数据库连接池硬编码 | 已修复 | `internal/database/db.go` 已使用配置参数 |
| P1-15 分页参数无上限 | 大体修复 | `user_handler.go`、`device_handler.go`、`log_handler.go` 均已限制 `page_size <= 100` |
---
## 仍需保留的中高优先级问题
### P1-01: API 响应格式仍然不统一
**位置**
- `internal/api/handler/auth_handler.go`
- `internal/api/handler/password_reset_handler.go`
- `internal/api/handler/user_handler.go`
**问题**
- 同一套 API 中同时存在 `{error: ...}`、`{message: ...}`、`{code,message,data}` 等多种响应结构
- `Logout`、`CSRF`、认证错误分支、参数绑定错误分支的格式仍不一致
**影响**
- 前端错误处理成本高
- 自动化契约测试难写
- 文档与真实行为容易继续漂移
### P1-02: 登录失败计数器仍保留非原子降级路径
**位置**
- `internal/service/auth.go:492`
**问题**
- 主路径已使用 `cache.Increment()`
- 但 `Increment` 出错时仍回退到 `Get + current++ + Set`
**影响**
- 在缓存不支持原子递增或运行时出错场景下，旧竞态仍可能重现
**结论**
- 不再按 P0 处理，但仍是必须收尾的 P1
### P1-03: CLI/初始化路径存在权限与类型转换告警
**系统化工具证据**
- `cmd/ums/cmd/init.go:306` `gosec G115`
- `cmd/ums/cmd/init.go:341` `gosec G301`
- `cmd/ums/cmd/init.go:446` `gosec G306`
**人工判断**
- `int(os.Stdin.Fd())` 在 Windows 常见运行路径下不一定形成真实高危，但应改成更明确的受控转换
- 初始化命令写目录/文件权限偏宽，适合作为 P1/P2 收敛项
### P2-01: 上传目录仍被直接公开暴露
**位置**
- `internal/api/router/router.go`
**问题**
- `r.engine.Static("/uploads", "./uploads")` 仍直接公开暴露上传目录
**影响**
- 上传内容默认可被匿名访问
- 一旦上传内容策略控制不足，容易扩大文件暴露面
---
## 系统化工具补充审查
### `staticcheck ./...` 结果摘要
人工过滤后，当前值得保留的信号主要有三类：
1. **测试代码错误用法**
- `internal/api/handler/captcha_handler_test.go`
- `internal/service/auth_capabilities_test.go`
存在 `SA1012`，测试里向需要 `context.Context` 的调用传了 `nil`。
2. **测试代码潜在空指针**
- `internal/service/sms_provider_test.go`
- `internal/service/user_roles_test.go`
- `internal/service/webhook_service_test.go`
存在 `SA5011`，说明部分测试断言路径缺少空值保护。
3. **仓库内死代码/遗留辅助代码**
- `internal/api/middleware/auth.go`
- `internal/monitoring/slo.go`
- `internal/repository/sql_scan.go`
- `internal/repository/pagination.go`
存在 `U1000`，说明最近几轮修复后有未清理的遗留函数或字段。
### `gosec ./internal/... ./cmd/...` 结果摘要
`gosec` 本轮输出噪音较大，尤其把 OAuth URL、常量名、header 名、token URL 大量误判为“硬编码凭证”。人工过滤后，建议保留的结果如下：
1. **真实可收敛问题**
- `internal/api/handler/avatar_handler.go:147` `G301`
- `internal/api/handler/avatar_handler.go:159` `G306`
- `internal/service/password_reset.go:237`
- `internal/service/password_reset.go:252`
前者是目录/文件权限偏宽，后者是关键删除操作忽略返回错误。
2. **低风险但建议修整**
- `internal/service/captcha.go:164` `G404`
这里使用 `math/rand` 仅用于验证码图片背景色随机化，不直接影响验证码秘密值，但可以考虑改为更明确的非安全随机用途注释，或避免被安全扫描反复报警。
3. **高噪音误报，不建议直接据此立项**
- OAuth token URL / auth URL
- Header 名称
- 非凭证字符串常量
这些不应直接写进缺陷列表，否则会污染修复优先级。
---
## 当前建议修复顺序
### 第一批：立即处理
1. 修复 `totp-verify` 登录闭环，要求必须携带首因子验证后的临时态
2. 为设备接口补全 owner/admin 校验，并在 service 层增加按 `user_id` 的兜底约束
3. 为 `/users/:id/password` 增加 self-or-admin 授权，并区分“本人修改密码”和“管理员重置密码”语义
4. 统一 handler 上下文字段，彻底移除 `user_roles` 旧协议
5. 去掉 OAuth handler 的假成功返回，改成真实能力分发或显式 fail closed
### 第二批：本周内收口
1. 统一 API 响应结构
2. 清理登录失败计数器 fallback 竞态
3. 清理 `staticcheck` 暴露的测试错误与死代码
4. 收敛 `gosec` 中目录/文件权限与关键错误忽略问题
---
## 对旧报告的处理建议
1. 保留旧报告作为历史记录，不删除
2. 明确以本报告作为后续复核基线
3. 旧报告中“2026-04-18 修复完成附录”的“全部问题已修复完成”说法不再可信，后续对外引用时应停止使用该表述
---
## 最终判断
| 维度 | 结论 |
|---|---|
| 当前是否全部修复完成 | 否 |
| 当前是否适合直接上线 | 否 |
| 是否比 2026-04-17 更接近可上线 | 是，门禁更绿，旧 P0 多数已修，但出现新的授权/认证断层 |
| 当前最真实的状态 | “旧高危问题大部分已修，当前仍有新的 P0 授权与认证问题待收口，系统化静态审查还暴露出测试与遗留代码清理不足” |
Reference in New Issue View Git Blame Copy Permalink