466 lines
16 KiB
Markdown
466 lines
16 KiB
Markdown
# 项目严格 Review 报告
|
||
|
||
更新日期：2026-03-29
|
||
审查范围：`D:\project` 当前工作代码、配置、文档
|
||
|
||
---
|
||
|
||
## 0. 审查说明
|
||
|
||
本报告基于两次系统性代码审查：
|
||
1. **Go后端审查** (`internal/` 目录)
|
||
2. **React/TypeScript前端审查** (`frontend/admin/` 目录)
|
||
|
||
审查重点：安全性、并发安全、错误处理、资源管理、业务逻辑、代码规范。
|
||
|
||
---
|
||
|
||
## 1. 后端发现的问题
|
||
|
||
### 1.1 安全问题
|
||
|
||
#### [高] OAuth `ValidateToken` 无实际验证
|
||
|
||
**文件**: `internal/auth/oauth.go`, 行 433-437
|
||
|
||
```go
|
||
func (m *DefaultOAuthManager) ValidateToken(token string) (bool, error) {
|
||
// 各 provider 的 token 验证需要 provider 上下文，此处作为通用 fallback
|
||
return len(token) > 0, nil
|
||
}
|
||
```
|
||
|
||
**问题**: 接口 `OAuthManager.ValidateToken` 的实现仅检查 `len(token) > 0`，对任何非空字符串都返回 `true`。没有任何实际的 token 有效性验证逻辑。
|
||
|
||
**建议**: 移除此 fallback 实现，改为对不支持的 provider 返回明确错误，或通过各 provider 的 userinfo 端点验证 token 有效性。
|
||
|
||
---
|
||
|
||
#### [中] OAuth StateSecret 使用不安全默认值
|
||
|
||
**文件**: `internal/auth/oauth_config.go`, 行 155
|
||
|
||
```go
|
||
StateSecret: getEnv("OAUTH_STATE_SECRET", "default-secret-change-in-production"),
|
||
```
|
||
|
||
**问题**: 当环境变量 `OAUTH_STATE_SECRET` 未配置时，OAuth state 的签名密钥硬编码为默认值。如果生产环境配置缺失且环境变量也未设置，攻击者可以伪造有效的 OAuth state，绕过 CSRF 保护。
|
||
|
||
**建议**: 当配置缺失时显式返回错误，禁止程序以不安全的默认密钥启动。
|
||
|
||
---
|
||
|
||
#### [中] 多处类型断言缺少 ok 检查
|
||
|
||
**文件**: `internal/api/handler/auth.go`, 行 406, 567, 603, 622
|
||
|
||
多处 `userID.(int64)` 直接进行类型断言而未检查 `ok` 值。
|
||
|
||
**问题**: Gin 的 `c.Get()` 返回 `interface{}` 类型，直接进行类型断言在类型不匹配时会触发 panic。
|
||
|
||
**建议**: 添加类型检查：
|
||
```go
|
||
userIDVal, exists := c.Get("user_id")
|
||
if !exists {
|
||
c.JSON(http.StatusUnauthorized, apierrors.New(apierrors.CodeUnauthorized, "unauthorized"))
|
||
return
|
||
}
|
||
userID, ok := userIDVal.(int64)
|
||
if !ok {
|
||
c.JSON(http.StatusUnauthorized, apierrors.New(apierrors.CodeUnauthorized, "invalid user id"))
|
||
return
|
||
}
|
||
```
|
||
|
||
---
|
||
|
||
#### [中] ResendActivationEmail 存在用户枚举风险
|
||
|
||
**文件**: `internal/api/handler/auth.go`, 行 264-277
|
||
|
||
**问题**: 服务端通过 `if the email exists...` 的提示文字，隐式地承认了邮箱是否存在。但通过观察响应是否一致来判断邮箱存在性的攻击者，可以结合响应时间侧信道来枚举有效邮箱。
|
||
|
||
**建议**: 确保整个邮件发送流程的总耗时在两种情况下保持一致（可增加人工延迟），防止时间侧信道攻击。
|
||
|
||
---
|
||
|
||
### 1.2 并发安全
|
||
|
||
#### [高] StateManager 清理 goroutine 无法停止
|
||
|
||
**文件**: `internal/auth/state.go`, 行 68-75
|
||
|
||
```go
|
||
func (sm *StateManager) StartCleanupRoutine() {
|
||
ticker := time.NewTicker(5 * time.Minute)
|
||
go func() {
|
||
for range ticker.C {
|
||
sm.Cleanup()
|
||
}
|
||
}}()
|
||
}
|
||
```
|
||
|
||
**问题**: `StartCleanupRoutine` 启动的后台 goroutine 没有任何停止机制。当程序需要优雅关闭时，无法等待或通知此 goroutine 退出，可能导致 goroutine 泄漏。
|
||
|
||
**建议**: 添加 stop channel：
|
||
```go
|
||
func (sm *StateManager) StartCleanupRoutine(stop <-chan struct{}) {
|
||
ticker := time.NewTicker(5 * time.Minute)
|
||
go func() {
|
||
for {
|
||
select {
|
||
case <-ticker.C:
|
||
sm.Cleanup()
|
||
case <-stop:
|
||
ticker.Stop()
|
||
return
|
||
}
|
||
}
|
||
}()
|
||
}
|
||
```
|
||
|
||
---
|
||
|
||
#### [高] 内存 rate limiter map 无界限增长
|
||
|
||
**文件**: `internal/api/middleware/ratelimit.go`, 行 84-106
|
||
|
||
**问题**: `limiters` map 为每个不同的 IP 地址或用户 ID 创建一个 rate limiter 实例并永久存储，从不清理。随着时间推移和用户增多，该 map 会无限增长，导致内存持续消耗。
|
||
|
||
**建议**: 实施 LRU 淘汰策略或 TTL 机制，定期清理长期未使用的 limiter 条目。
|
||
|
||
---
|
||
|
||
#### [高] L1Cache 无最大容量限制
|
||
|
||
**文件**: `internal/cache/l1.go`, 行 19-46
|
||
|
||
**问题**: `L1Cache` 是一个无界的并发安全 map，既没有最大条目数限制，也没有 LRU/TTL 淘汰策略。
|
||
|
||
**建议**: 为 `L1Cache` 添加最大容量限制和 LRU 淘汰策略。
|
||
|
||
---
|
||
|
||
#### [中] TOTP 恢复码删除非原子
|
||
|
||
**文件**: `internal/service/totp.go`, 行 130-133
|
||
|
||
**问题**: 恢复码在内存中从 slice 中删除后，如果 `UpdateTOTP` 数据库更新失败（`err` 被忽略），该恢复码实际上已从内存中移除，但数据库中的记录并未更新——导致一个恢复码被使用两次的窗口期。
|
||
|
||
**建议**: `UpdateTOTP` 的错误不应被忽略，应回滚内存中的恢复码列表。
|
||
|
||
---
|
||
|
||
### 1.3 资源管理
|
||
|
||
#### [中] 头像上传路径处理
|
||
|
||
**文件**: `internal/api/handler/avatar.go`, 行 28
|
||
|
||
```go
|
||
avatarDir = "./uploads/avatars"
|
||
```
|
||
|
||
**问题**: 头像目录使用相对路径，可能存在路径遍历风险。
|
||
|
||
**建议**: 使用绝对路径并通过配置管理，对文件名进行 UUID 化处理。
|
||
|
||
---
|
||
|
||
#### [低] RSA 私钥文件权限设置过于宽松
|
||
|
||
**文件**: `internal/auth/jwt.go`, 行 212
|
||
|
||
```go
|
||
if err := os.WriteFile(privatePath, privatePEM, 0o644); err != nil {
|
||
```
|
||
|
||
**建议**: 将私钥文件权限改为 `0o600`。
|
||
|
||
---
|
||
|
||
### 1.4 业务逻辑
|
||
|
||
#### [中] TOTP Disable 时恢复码直接清空无审计日志
|
||
|
||
**文件**: `internal/service/totp.go`, 行 81-106
|
||
|
||
**问题**: 当用户通过恢复码成功禁用 2FA 时，所有未使用的恢复码全部被删除，但没有任何记录表明"哪些恢复码被作废"，无法区分是用户主动禁用还是攻击者通过恢复码禁用了账号。
|
||
|
||
**建议**: 在操作日志中记录 2FA 禁用事件（包含操作来源 IP），并考虑对"通过恢复码禁用 2FA"触发安全告警。
|
||
|
||
---
|
||
|
||
#### [低] 密码强度评分对短密码过于宽松
|
||
|
||
**文件**: `internal/service/auth.go`, 行 276-298
|
||
|
||
**问题**: 当 `strict=false` 时，仅要求 `info.Score < 2` 才拒绝，即密码长度 8 位且包含任意一种字符类型（Score=1）也可通过验证。这意味着 `"aaaaaaaa"` 这样极弱的密码可通过验证。
|
||
|
||
**建议**: 调整评分阈值或增强评分逻辑，确保长度 8 位的单类型字符密码被拒绝。
|
||
|
||
---
|
||
|
||
### 1.5 代码规范
|
||
|
||
#### [中] `social_account_repo.go` 使用原生 SQL 而非 GORM
|
||
|
||
**文件**: `internal/repository/social_account_repo.go`
|
||
|
||
**问题**: 整个代码库使用 GORM 作为 ORM，但 `SocialAccountRepository` 使用 `*sql.DB` 的原生 SQL 实现，绕过了 GORM 的事务管理和连接池封装。
|
||
|
||
**建议**: 将 `SocialAccountRepository` 迁移为 GORM 实现，与其他 repository 保持一致。
|
||
|
||
---
|
||
|
||
#### [中] 多处错误被静默忽略
|
||
|
||
多处 `_ = err` 或 `_ = json.Marshal(...)` 的静默错误处理：
|
||
- `internal/service/totp.go` 行 47, 94, 131, 133
|
||
- `internal/api/handler/auth.go` 行 379
|
||
- `internal/api/middleware/operation_log.go` 行 87
|
||
|
||
**问题**: 静默忽略错误使得调试困难，且可能导致数据不一致。
|
||
|
||
**建议**: 对于关键操作的错误，不应忽略，至少记录日志。
|
||
|
||
---
|
||
|
||
#### [低] 命名不一致
|
||
|
||
**文件**: `internal/auth/oauth.go`, 行 18
|
||
|
||
`OProviderGoogle`（大写 O）与其他 `OAuthProviderXXX` 常量命名风格不一致。
|
||
|
||
**建议**: 统一为 `OAuthProviderGoogle`。
|
||
|
||
---
|
||
|
||
## 2. 前端发现的问题
|
||
|
||
### 2.1 安全性
|
||
|
||
#### [高] `uploadAvatar` 字段名可能错误
|
||
|
||
**文件**: `frontend/admin/src/services/profile.ts`, 行 49
|
||
|
||
```typescript
|
||
export function uploadAvatar(userId: number, file: File): Promise<AvatarUploadResponse> {
|
||
const formData = new FormData()
|
||
formData.append('avatar', file) // ← 字段名可能是 'file'
|
||
return post<AvatarUploadResponse>(`/users/${userId}/avatar`, formData)
|
||
}
|
||
```
|
||
|
||
**问题**: 函数签名的第二个参数名为 `file`，FormData 中使用的字段名是 `avatar`，但后端期望的字段名可能是 `file`。这会导致后端无法识别上传的文件字段。
|
||
|
||
**建议**: 确认后端期望的字段名，保持前后端一致。
|
||
|
||
---
|
||
|
||
#### [中] 操作日志字段未经 HTML 转义直接渲染
|
||
|
||
**文件**: `frontend/admin/src/pages/admin/OperationLogsPage/OperationLogDetailDrawer.tsx`, 行 31, 35, 45
|
||
|
||
**问题**: `request_path`、`request_params`、`user_agent` 均来自后端日志数据，如果包含 XSS 可执行脚本，在管理后台中可能造成风险。
|
||
|
||
**建议**: 使用 AntD 的 `text` 属性而非 `children` 来渲染这些用户可控字段。
|
||
|
||
---
|
||
|
||
### 2.2 状态管理
|
||
|
||
#### [中] React 状态与模块状态双重管理
|
||
|
||
**文件**: `frontend/admin/src/app/providers/AuthProvider.tsx`, 行 45-50, 127-181
|
||
|
||
**问题**: `sessionState`（模块级变量）和 React state (`user`, `roles`) 同时保存用户信息。当某处只更新了模块状态而未更新 React 状态时，fallback 机制会掩盖问题。
|
||
|
||
**建议**: 统一状态管理范式，只使用 React state 作为唯一数据源。
|
||
|
||
---
|
||
|
||
#### [中] `onPressEnter` 绑定时未使用 `void`
|
||
|
||
**文件**: `frontend/admin/src/pages/admin/UsersPage/UsersPage.tsx`, 行 403
|
||
|
||
```typescript
|
||
onPressEnter={fetchUsers} // ← fetchUsers 是 async 函数
|
||
```
|
||
|
||
**问题**: `fetchUsers` 是 `async` 函数，返回 Promise，而 `onPressEnter` 期望的是 `React.KeyboardEventHandler`。
|
||
|
||
**建议**: `onPressEnter={() => void fetchUsers()}`
|
||
|
||
---
|
||
|
||
### 2.3 性能
|
||
|
||
#### [高] Webhooks 全量加载后在客户端分页，无服务端分页
|
||
|
||
**文件**: `frontend/admin/src/pages/admin/WebhooksPage/WebhooksPage.tsx`, 行 50-61, 73-82
|
||
|
||
```typescript
|
||
const fetchWebhooks = useCallback(async () => {
|
||
const result = await listWebhooks() // ← 获取全部数据
|
||
setWebhooks(result)
|
||
}, [])
|
||
```
|
||
|
||
**问题**: `listWebhooks()` 无任何参数，后端返回全部 webhook 数据。当 webhook 数量增长时，会导致网络传输大量无用数据、客户端内存占用过高、过滤和分页全在主线程执行。
|
||
|
||
**建议**: 为 `listWebhooks` 添加服务端分页支持（`page`, `page_size`）。
|
||
|
||
---
|
||
|
||
#### [中] ProfileSecurityPage 单组件管理 ~30 个状态变量
|
||
|
||
**文件**: `frontend/admin/src/pages/admin/ProfileSecurityPage/ProfileSecurityPage.tsx`, 行 72-103
|
||
|
||
**问题**: 单个组件管理超过 30 个状态变量，任何一个状态变化都会触发整个组件重新渲染。这个 880 行的巨型组件应该被拆分。
|
||
|
||
**建议**: 拆分为多个子组件：AvatarSection、PasswordSection、TOTPSection、SocialBindingSection、DevicesSection、AuditLogSection。
|
||
|
||
---
|
||
|
||
### 2.4 类型安全
|
||
|
||
#### [中] `ApiResponse.data` 类型为 `T` 而非 `T | null`
|
||
|
||
**文件**: `frontend/admin/src/types/http.ts`, 行 8-15
|
||
|
||
**问题**: 某些后端 API 响应（如 204 No Content）其 `data` 字段可能为 `null` 或 `undefined`，但类型定义为非空。
|
||
|
||
**建议**: `data: T` 改为 `data: T | null`，在访问前做空值检查。
|
||
|
||
---
|
||
|
||
### 2.5 组件设计
|
||
|
||
#### [高] ProfileSecurityPage 未复用已有的 ContactBindingsSection
|
||
|
||
**文件**: `frontend/admin/src/pages/admin/ProfileSecurityPage/ProfileSecurityPage.tsx`, 行 656-663
|
||
|
||
**问题**: `ContactBindingsSection` 组件已在同目录下定义，但 `ProfileSecurityPage` 中的邮箱/手机号绑定逻辑与 `ContactBindingsSection` 存在功能重叠和重复实现。
|
||
|
||
**建议**: 确认 `ContactBindingsSection` 的职责范围，如果它已包含完整的绑定 UI，删除 `ProfileSecurityPage` 中的重复 `ContentCard`。
|
||
|
||
---
|
||
|
||
## 3. 问题汇总
|
||
|
||
### 后端问题统计
|
||
|
||
| 编号 | 严重程度 | 分类 | 文件 | 行号 |
|
||
|------|----------|------|------|------|
|
||
| 1.1 | 高 | 安全 | `auth/oauth.go` | 433-437 |
|
||
| 1.2 | 中 | 安全 | `auth/oauth_config.go` | 155 |
|
||
| 1.3 | 中 | 安全 | `api/handler/auth.go` | 406, 567, 603, 622 |
|
||
| 1.4 | 中 | 安全 | `api/handler/auth.go` | 264-277 |
|
||
| 2.1 | 高 | 并发 | `auth/state.go` | 68-75 |
|
||
| 2.2 | 高 | 并发 | `api/middleware/ratelimit.go` | 84-106 |
|
||
| 2.3 | 高 | 资源 | `cache/l1.go` | 19-46 |
|
||
| 2.4 | 中 | 并发 | `service/totp.go` | 130-133 |
|
||
| 3.1 | 中 | 资源 | `api/handler/avatar.go` | 28 |
|
||
| 3.2 | 低 | 资源 | `auth/jwt.go` | 212 |
|
||
| 4.1 | 中 | 业务 | `service/totp.go` | 81-106 |
|
||
| 4.2 | 低 | 业务 | `service/auth.go` | 293 |
|
||
| 5.1 | 中 | 规范 | `repository/social_account_repo.go` | 全文件 |
|
||
| 5.2 | 中 | 规范 | 多处 | 多行 |
|
||
| 5.3 | 低 | 规范 | `auth/oauth.go` | 18 |
|
||
|
||
**后端总计**: 高危 4 个，中危 8 个，低危 3 个
|
||
|
||
### 前端问题统计
|
||
|
||
| 编号 | 严重程度 | 分类 | 文件 |
|
||
|------|----------|------|------|
|
||
| 1.1 | 高 | 安全 | `services/profile.ts` |
|
||
| 1.2 | 中 | 安全 | `OperationLogDetailDrawer.tsx` |
|
||
| 2.1 | 中 | 状态管理 | `AuthProvider.tsx` |
|
||
| 2.2 | 中 | 状态管理 | `UsersPage.tsx` |
|
||
| 3.1 | 高 | 性能 | `WebhooksPage.tsx` |
|
||
| 3.2 | 中 | 性能 | `ProfileSecurityPage.tsx` |
|
||
| 4.1 | 中 | 类型安全 | `http.ts` |
|
||
| 5.1 | 高 | 组件设计 | `ProfileSecurityPage.tsx` |
|
||
|
||
**前端总计**: 高危 3 个，中危 5 个
|
||
|
||
---
|
||
|
||
## 4. 已确认的良好实践
|
||
|
||
以下方面经审查确认为良好实践，无需修改：
|
||
|
||
### 后端
|
||
1. **JWT JTI 黑名单**: 访问令牌和刷新令牌都包含 JTI，支持基于 JTI 的令牌黑名单，Logout 机制完善
|
||
2. **密码哈希**: 使用 Argon2id（64MB 内存，3 次迭代），bcrypt 作为向后兼容，均使用 `crypto/rand` 生成盐
|
||
3. **SQL 注入防护**: GORM 参数化查询，`escapeLikePattern` 正确处理 LIKE 通配符转义
|
||
4. **CSRF Token**: 使用 `crypto/rand` 生成 16 字节随机数
|
||
5. **TOTP 验证**: 使用 pquerna/otp 库，支持前后各 1 个时间窗口
|
||
6. **OAuth state 管理**: 使用带 TTL 的 in-memory map 存储 state，防止 CSRF
|
||
7. **OAuth return_to 校验**: 验证 URL scheme、origin 白名单，防止开放重定向
|
||
8. **头像上传**: 内容类型白名单检查、图片尺寸解码后验证、文件大小限制
|
||
9. **敏感字段日志脱敏**: `sanitizeParams` 过滤 password、token 等敏感字段
|
||
10. **Rate Limiting**: 支持 Token Bucket、Leaky Bucket、Sliding Window 三种算法
|
||
11. **IP 过滤**: 支持 CIDR 范围、AnomalyDetector 自动封禁
|
||
12. **并发安全**: L1/L2 cache 使用 `sync.RWMutex`，StateManager 使用 `sync.RWMutex`
|
||
13. **Context 超时**: 数据库操作、缓存操作均通过 `context.WithContext` 传递超时
|
||
|
||
### 前端
|
||
1. **认证状态管理**: 内存-only token，不持久化到 localStorage/sessionStorage
|
||
2. **窗口守卫**: `window.alert/confirm/prompt/open` 被阻断并记录为结构化错误
|
||
3. **错误处理**: `AppError` 类封装了完整的错误类型和响应映射
|
||
4. **HTTP 客户端**: 完整的 401 自动刷新机制
|
||
5. **组件测试**: 高覆盖率的组件测试
|
||
|
||
---
|
||
|
||
## 5. 优先级修复建议
|
||
|
||
### 第一优先级（高危，必须修复）
|
||
1. OAuth `ValidateToken` fallback 实现 - 安全漏洞
|
||
2. StateManager goroutine 无法停止 - 资源泄漏
|
||
3. Rate limiter map 无界限增长 - 内存泄漏
|
||
4. L1Cache 无最大容量限制 - 内存泄漏
|
||
5. `uploadAvatar` 字段名可能错误 - 功能性 bug
|
||
6. Webhooks 全量加载无分页 - 性能和可扩展性问题
|
||
7. ProfileSecurityPage 未复用 ContactBindingsSection - 代码重复
|
||
|
||
### 第二优先级（中危，建议修复）
|
||
1. OAuth StateSecret 不安全默认值
|
||
2. 多处类型断言缺少 ok 检查
|
||
3. TOTP 恢复码删除非原子
|
||
4. 多处错误被静默忽略
|
||
5. `social_account_repo.go` 使用原生 SQL 而非 GORM
|
||
6. React 状态与模块状态双重管理
|
||
7. `onPressEnter` 绑定未使用 `void`
|
||
8. ProfileSecurityPage 单组件管理 ~30 个状态变量
|
||
9. 操作日志字段未经 HTML 转义直接渲染
|
||
|
||
### 第三优先级（低危，可选修复）
|
||
1. RSA 私钥文件权限过于宽松
|
||
2. 密码强度评分对短密码过于宽松
|
||
3. 命名不一致 (`OProviderGoogle`)
|
||
4. `ApiResponse.data` 类型定义问题
|
||
|
||
---
|
||
|
||
## 6. 文档一致性
|
||
|
||
### 发现的问题
|
||
|
||
1. **PROJECT_REVIEW_REPORT.md** - 文件编码损坏，需要重新创建为 UTF-8 编码
|
||
2. **DATA_MODEL.md** - 以下表格与实际实现不符：
|
||
- `verification_codes` - 无独立表（内存/Redis管理）
|
||
- `token_blacklist` - 未实现
|
||
- `user_custom_fields` - 未实现
|
||
- `system_configs` - 通过 config.yaml 管理
|
||
- `audit_logs` - 实际表名为 `operation_logs`
|
||
|
||
---
|
||
|
||
*本报告由系统审查生成，审查日期：2026-03-29*
|