Victor_Jay
55c408d86c
- 安全：移除 CSP 中 esm.sh/cdnjs.cloudflare.com，highlight.js 主题已本地化 73 个文件 - 安全：StatusBanned 分支补 dummy hash 防时序攻击 - 安全：手写 constantTimeEq 替换为 crypto/subtle.ConstantTimeCompare - Bug：锁定操作消息已删除→已锁定 - YAGNI：删除 12 个空预留模板目录 - KISS：删除 common.CheckPassword 薄封装，统一用 bcrypt 调用 - KISS：删除 tokenCtrl 别名字段，api.go 统一用 authCtrl - RateLimiter：check()+recordFail 闭包模式重构为 try() 原子操作，消除竞态 - RateLimiter：新增 ClearIP() 方法，注册成功时清除 IP 计数 - 文档：修正 audit_service.go 注释编号跳跃(3→5→4) - 文档：修正 deps_core.go 过清理→过期清理
427 lines
16 KiB
Markdown
427 lines
16 KiB
Markdown
# MetaLab 项目全量代码审计报告
|
||
|
||
**审计日期**: 2025-05-31
|
||
**审计范围**: `lab.metazone.cc-GO/` 全部 Go 源码、模板、配置
|
||
**项目状态**: 未发布，无需考虑旧版兼容
|
||
**审查标准**: DRY/KISS/YAGNI/LoD/SOLID + 最佳实践
|
||
|
||
---
|
||
|
||
## 问题清单
|
||
|
||
---
|
||
|
||
### 问题 #1: 【严重】CSP 安全头引用已废弃的 Tiptap CDN（esm.sh）
|
||
|
||
**类型**: 死代码 / 残留配置
|
||
**位置**: `internal/middleware/security.go:12-21`
|
||
**违反原则**: KISS（引用了不存在的依赖）
|
||
|
||
```go
|
||
// script-src: 本站 + esm.sh CDN (Tiptap ESM 模块) + cdnjs (highlight.js)
|
||
"script-src 'self' 'unsafe-inline' https://esm.sh https://cdnjs.cloudflare.com; "+
|
||
"style-src 'self' 'unsafe-inline' https://esm.sh https://cdnjs.cloudflare.com; "+
|
||
"connect-src 'self' https://esm.sh"
|
||
```
|
||
|
||
**问题**: 项目已迁移到 Vditor 编辑器，但 CSP 头仍保留 Tiptap 时代的 esm.sh CDN 白名单。三个指令（script-src、style-src、connect-src）都包含未使用的 `https://esm.sh`。
|
||
|
||
**风险**:
|
||
- 扩大了不必要的 CSP 白名单，引入额外信任域
|
||
- connect-src 允许到 esm.sh 的连接，可能泄露页面信息
|
||
|
||
**建议**: 移除所有 `https://esm.sh` 引用，highlight.js 主题已本地化为 73 个静态文件（`static/vditor/dist/js/highlight.js/styles/`），CSP 可完全收紧为 `'self'`。
|
||
|
||
---
|
||
|
||
### 问题 #2: 【严重】Login() 中封禁状态检查缺少防时序攻击保护
|
||
|
||
**类型**: 安全缺陷
|
||
**位置**: `internal/service/auth_service.go:151-154`
|
||
**违反原则**: 最佳安全实践
|
||
|
||
```go
|
||
// 封禁
|
||
if user.Status == model.StatusBanned {
|
||
return nil, common.ErrUserBanned // ← 没有 bcrypt dummy hash 比对
|
||
}
|
||
```
|
||
|
||
**问题**: 其他分支（维护模式、用户不存在、密码错误、StatusLocked）都有 `_ = bcrypt.CompareHashAndPassword(dummyHash, ...)` 防时序攻击，唯独 StatusBanned 分支缺失。攻击者可以通过响应时间差异判断被封禁的账号是否存在。
|
||
|
||
**建议**: 在返回 `ErrUserBanned` 前增加 dummy hash 比对：
|
||
```go
|
||
if user.Status == model.StatusBanned {
|
||
_ = bcrypt.CompareHashAndPassword(dummyHash, []byte(req.Password))
|
||
return nil, common.ErrUserBanned
|
||
}
|
||
```
|
||
|
||
---
|
||
|
||
### 问题 #3: 【严重】自定义 constantTimeEq 不如 crypto/subtle 安全
|
||
|
||
**类型**: 安全缺陷 / 最佳实践
|
||
**位置**: `internal/middleware/csrf_token.go:44-53`（被 `csrf.go:47` 调用）
|
||
**违反原则**: 安全最佳实践
|
||
|
||
```go
|
||
// constantTimeEq 恒定时间字符串比较（防时序攻击）
|
||
func constantTimeEq(a, b string) bool {
|
||
if len(a) != len(b) { // ← 长度不等时提前返回，泄露长度信息
|
||
return false
|
||
}
|
||
var result byte
|
||
for i := 0; i < len(a); i++ {
|
||
result |= a[i] ^ b[i]
|
||
}
|
||
return result == 0
|
||
}
|
||
```
|
||
|
||
**问题**:
|
||
1. `constantTimeEq` 在同包内被 `csrf.go:47` 调用，**非死代码**
|
||
2. 但其手写实现存在两个安全隐患：
|
||
- **长度提前返回泄露信息**：`len(a) != len(b)` 时立即返回 false，攻击者可通过响应时间判断 CSRF token 长度
|
||
- **无编译器优化防护**：`crypto/subtle.ConstantTimeCompare` 内部使用了特殊的编译器屏障防止被优化，手写版本可能被 Go 编译器优化掉 XOR 结果检查
|
||
3. 函数定义在 `csrf_token.go`（token 生成文件）而非 `csrf.go`（校验文件），逻辑归属不当
|
||
|
||
**建议**: 删除 `constantTimeEq`，改用 `crypto/subtle.ConstantTimeCompare([]byte(cookieToken), []byte(headerToken)) == 1`。这也是 Go 官方推荐的做法。
|
||
|
||
---
|
||
|
||
### 问题 #4: 【高】Admin 控制器锁定操作消息显示错误
|
||
|
||
**类型**: Bug
|
||
**位置**: `internal/controller/admin/admin_controller.go:93-96`
|
||
**违反原则**: 无（纯 bug）
|
||
|
||
```go
|
||
action := "封禁"
|
||
if req.Status == model.StatusActive {
|
||
action = "解封"
|
||
} else if req.Status == model.StatusLocked {
|
||
action = "已删除" // ← BUG: 应该是 "已锁定"
|
||
}
|
||
```
|
||
|
||
**问题**: 当管理员执行锁定操作时，成功提示消息显示"已删除成功"，而不是"已锁定成功"。Locked 与 Deleted 是两个完全不同的状态。
|
||
|
||
**建议**: 改为 `action = "已锁定"`
|
||
|
||
---
|
||
|
||
### 问题 #5: 【高】Login() 中密码验证方式不一致
|
||
|
||
**类型**: 代码一致性问题
|
||
**位置**: `internal/service/auth_service.go:138,152`
|
||
**违反原则**: KISS（同一逻辑用了两种实现）
|
||
|
||
```go
|
||
// StatusDeleted 分支:
|
||
if !common.CheckPassword(req.Password, user.PasswordHash) { ... }
|
||
|
||
// StatusBanned 之后的主路径:
|
||
if !common.CheckPassword(req.Password, user.PasswordHash) { ... }
|
||
```
|
||
|
||
而 `CheckPassword` 内部只是封装了单行：
|
||
```go
|
||
func CheckPassword(password, hash string) bool {
|
||
err := bcrypt.CompareHashAndPassword([]byte(hash), []byte(password))
|
||
return err == nil
|
||
}
|
||
```
|
||
|
||
**问题**: 虽然功能上没有 bug，但在同一函数中既有 `common.CheckPassword()` 调用，也有其他分支使用 `_ = bcrypt.CompareHashAndPassword(dummyHash, ...)`（直接调用 bcrypt）。风格不统一，且 `CheckPassword` 的封装价值极低（仅包装一行标准库调用，不如直接使用 bcrypt 调用更直观）。
|
||
|
||
**建议**:
|
||
- 选项A: 删除 `common.CheckPassword`，统一使用 `bcrypt.CompareHashAndPassword`
|
||
- 选项B: 在 `CheckPassword` 中增加防时序的一致性包装，统一入口
|
||
|
||
---
|
||
|
||
### 问题 #6: 【中】大量空模板目录（YAGNI 违规）
|
||
|
||
**类型**: YAGNI 违规
|
||
**位置**:
|
||
- `templates/MetaLab-2026/html/post/`（空）
|
||
- `templates/MetaLab-2026/html/comment/`（空）
|
||
- `templates/MetaLab-2026/html/partials/`（空）
|
||
- `templates/MetaLab-2026/html/search/`（空）
|
||
- `templates/MetaLab-2026/html/error/`（空）
|
||
- `templates/MetaLab-2026/html/admin/`（空）
|
||
- `templates/system/email/`（空）
|
||
|
||
**问题**: 7 个空目录，标注为"预留"。项目尚未发布，这些目录的创建时机应该和实际功能开发同步，而非提前占位。
|
||
|
||
**建议**: 删除所有空目录。需要时随功能一起创建。
|
||
|
||
---
|
||
|
||
### 问题 #7: 【中】Shortcode 预留类型（poll/resource）无后端实现
|
||
|
||
**类型**: YAGNI 违规
|
||
**位置**: `internal/model/shortcode.go:37-38`, `internal/service/shortcode_service.go:126-137`
|
||
**违反原则**: YAGNI
|
||
|
||
```go
|
||
ShortcodePoll ShortcodeType = "poll" // ※预留（后端API未实现）
|
||
ShortcodeResource ShortcodeType = "resource" // ※预留（后端API未实现）
|
||
```
|
||
|
||
**问题**: poll 和 resource 两个 shortcode 类型的后端 API 未实现，前端也未实现（shortcode.js 中可能也未实现对应渲染），但代码中已注册了完整的解析和占位 HTML 生成逻辑。用户实际上可以使用 `[zone:poll:xxx]` 语法，但会得到一个永远"加载中..."的卡片。
|
||
|
||
**状态**: 已排期开发，保留（不删除）。首次审计时误删，已恢复。前端渲染逻辑将随 API 同步实现。
|
||
|
||
|
||
---
|
||
|
||
### 问题 #8: 【中】redis_store.go 全注释的"预留实现"
|
||
|
||
**类型**: YAGNI / 死代码
|
||
**位置**: `internal/session/redis_store.go`
|
||
**违反原则**: YAGNI
|
||
|
||
**问题**: 整个文件是注释掉的代码，没有实际可执行逻辑。如果未来需要 Redis 支持，到时再创建即可。
|
||
|
||
**状态**: 已排期开发，保留（不删除）。首次审计时误删，已恢复。Redis 支持将在后续迭代中实现。
|
||
|
||
---
|
||
|
||
### 问题 #9: 【中】tokenCtrl 是 authCtrl 的无意义别名
|
||
|
||
**类型**: 不必要的字段重复
|
||
**位置**: `internal/router/deps_core.go:30`, `internal/router/deps_extra.go:54`
|
||
**违反原则**: KISS
|
||
|
||
```go
|
||
// deps_core.go
|
||
type dependencies struct {
|
||
// ...
|
||
tokenCtrl *controller.AuthController // ← 与 authCtrl 类型完全相同
|
||
}
|
||
|
||
// deps_extra.go
|
||
return &dependencies{
|
||
// ...
|
||
tokenCtrl: authCtrl, // ← 赋的是同一个对象
|
||
}
|
||
```
|
||
|
||
**问题**: `tokenCtrl` 和 `authCtrl` 指向同一个 `*controller.AuthController` 实例，`tokenCtrl` 仅在 `api.go` 的路由中使用（`d.tokenCtrl.CheckEmail/Register/Login/...`）。这个别名不带来任何好处，反而增加理解成本。
|
||
|
||
**建议**: 删除 `tokenCtrl` 字段，`api.go` 中直接使用 `d.authCtrl`。
|
||
|
||
---
|
||
|
||
### 问题 #10: 【中】RateLimiter.check() 存在竞态条件
|
||
|
||
**类型**: 并发缺陷
|
||
**位置**: `internal/middleware/ratelimit_core.go:34-81`
|
||
|
||
```go
|
||
func (rl *RateLimiter) check(...) (RateLimitResult, func()) {
|
||
rl.mu.Lock()
|
||
defer rl.mu.Unlock()
|
||
// ... 读取 state ...
|
||
|
||
recordFail := func() {
|
||
rl.mu.Lock() // ← 重新获取锁
|
||
defer rl.mu.Unlock()
|
||
// ... 修改 state ...
|
||
}
|
||
return RateLimitResult{Blocked: false}, recordFail
|
||
}
|
||
```
|
||
|
||
**问题**: `check()` 持锁检查后释放锁，返回的 `recordFail` 闭包在**锁外**执行，重新获取锁后再修改状态。
|
||
|
||
**修复方案**: 将 `check()` + `recordFail` 闭包模式重构为 `try()` 原子操作模式——在持锁状态下一次性完成检查+递增，消除竞态窗口。API 改为 `AllowAccount() RateLimitResult` / `AllowIP() RateLimitResult`（不再返回闭包）。调用方在失败时不再需要显式调用 `recordFail()`，成功时仍调用 `Clear()` 清除计数。
|
||
|
||
---
|
||
|
||
### 问题 #11: 【低】audit_service.go review() 注释编号跳跃
|
||
|
||
**类型**: 文档瑕疵
|
||
**位置**: `internal/service/audit_service.go:152-166`
|
||
|
||
```go
|
||
// 3. 查审核人信息
|
||
reviewer, err := s.userRepo.FindByID(reviewerID)
|
||
// ...
|
||
|
||
// 5. 标记审核结果 ← 跳过了 4
|
||
submission.ReviewedBy = &reviewerID
|
||
```
|
||
|
||
**问题**: 注释编号从"3."直接跳到"5."，缺少"4."。
|
||
|
||
**建议**: 修正编号为连续递增。
|
||
|
||
---
|
||
|
||
### 问题 #12: 【低】编译产物未纳入 .gitignore（误报，实际不存在）
|
||
|
||
**类型**: 仓库整洁性
|
||
**位置**: `server`（根目录）, `cmd/server/server`
|
||
**状态**: 误报。经核实，`.gitignore` 已配置 `server` 规则且从未被 git 跟踪，`git rm --cached` 实际为空操作。此项已从修复表中移除。
|
||
|
||
---
|
||
|
||
### 问题 #13: 【低】项目零测试覆盖
|
||
|
||
**类型**: 质量保障缺失
|
||
**位置**: 整个项目（`*_test.go` 搜索结果: 0）
|
||
|
||
**问题**: 项目没有任何单元测试或集成测试。对于包含认证、权限、审核、数据持久化等复杂逻辑的系统，零测试意味着每次重构和修改都有回归风险。
|
||
|
||
**建议**: 至少为核心模块添加测试：
|
||
1. `model/user.go` - HasMinRole/CanOperateRole（纯函数，易测）
|
||
2. `service/auth_service.go` - 注册/登录/密码验证逻辑
|
||
3. `service/admin_service.go` - checkAndOperate 权限矩阵
|
||
4. `middleware/ratelimit_core.go` - 限流逻辑
|
||
|
||
---
|
||
|
||
### 问题 #14: 【低】全项目使用 log.Printf 无结构化日志
|
||
|
||
**类型**: 可维护性 / 可观测性
|
||
**位置**: 10 个文件，约 17 处 `log.Printf` 调用
|
||
|
||
**问题**: 项目大量使用标准库 `log.Printf`，无日志级别、无结构化字段、无上下文追踪。在生产环境中排查问题困难，无法按级别过滤日志。
|
||
|
||
**建议**: 引入轻量结构化日志库（如 `slog`，Go 1.21+ 标准库），按级别区分 Info/Warn/Error，关键路径添加 trace/request ID。
|
||
|
||
---
|
||
|
||
### 问题 #15: 【低】err != nil 返回时上下文信息丢失
|
||
|
||
**类型**: 可调试性
|
||
**位置**: 多处，例如 `internal/repository/user_repo.go`
|
||
|
||
```go
|
||
func (r *UserRepo) Create(user *model.User) error {
|
||
return r.db.Create(user).Error // ← 无上下文
|
||
}
|
||
```
|
||
|
||
**问题**: 数据库操作失败时，调用方只知道"出错了"，无法快速定位是哪个操作、哪个实体、哪个 ID 导致的失败。
|
||
|
||
**建议**: 使用 `fmt.Errorf("创建用户失败: %w", err)` 包装错误，在保持错误链的同时添加操作上下文。
|
||
|
||
---
|
||
|
||
### 问题 #16: 【低】common.CheckPassword 封装价值极低
|
||
|
||
**类型**: KISS 违规
|
||
**位置**: `internal/common/crypto.go:12-15`
|
||
|
||
```go
|
||
func CheckPassword(password, hash string) bool {
|
||
err := bcrypt.CompareHashAndPassword([]byte(hash), []byte(password))
|
||
return err == nil
|
||
}
|
||
```
|
||
|
||
**问题**: 仅包装一行标准库调用，无额外逻辑。与同一文件中封装了 bcrypt 成本参数的 `HashPassword` 不同，`CheckPassword` 没有提供抽象价值。反而因为隐藏了 `bcrypt.CompareHashAndPassword` 的调用，在需要 `dummyHash` 比对时（如 auth_service.go 的时序攻击防护）不得不绕过它直接调用 bcrypt。
|
||
|
||
**建议**:
|
||
- 如果保留 `CheckPassword`，将 dummy hash 比对也内置进去
|
||
- 或者删除此函数，直接在各处显式调用 `bcrypt.CompareHashAndPassword`
|
||
|
||
---
|
||
|
||
### 问题 #17: 【低】deps_core.go 中文注释错别字
|
||
|
||
**类型**: 文档瑕疵
|
||
**位置**: `internal/router/deps_core.go:45`
|
||
|
||
```go
|
||
// 启动后台过清理 goroutine
|
||
```
|
||
|
||
**问题**: "过清理"应为"过期清理"，少了一个"期"字。
|
||
|
||
**建议**: 修正为 `启动后台过期清理 goroutine`
|
||
|
||
---
|
||
|
||
## 汇总统计
|
||
|
||
| 严重程度 | 数量 | 问题编号 |
|
||
|---------|------|---------|
|
||
| 严重 | 3 | #1, #2, #3 |
|
||
| 高 | 2 | #4, #5 |
|
||
| 中 | 5 | #6, #7, #8, #9, #10 |
|
||
| 低 | 7 | #11, #12, #13, #14, #15, #16, #17 |
|
||
|
||
**总计: 17 个问题**
|
||
|
||
### 按原则分类
|
||
|
||
| 原则 | 问题编号 |
|
||
|------------|---------|
|
||
| 安全 | #1, #2, #3 |
|
||
| YAGNI | #6, #7, #8 |
|
||
| KISS | #5, #9, #16 |
|
||
| Bug | #4, #10 |
|
||
| 质量/可维护性 | #12, #13, #14, #15 |
|
||
| 文档 | #11, #17 |
|
||
|
||
---
|
||
|
||
## 整体评价
|
||
|
||
项目的分层架构设计合理，严格遵循单向依赖（router→controller→service→repository→model），接口隔离原则（ISP）执行到位，每层都通过最小接口依赖下层。依赖注入清晰，无循环依赖。
|
||
|
||
主要问题集中在三个方面：
|
||
1. **安全防护需加强** - CSP 配置残留、时序攻击防护不完整
|
||
2. **代码清理不及时** - 存在死代码、预留目录、未使用函数
|
||
3. **工程基础设施薄弱** - 零测试、无结构化日志、错误上下文丢失
|
||
|
||
建议优先处理严重级别问题（#1~#3），然后按批次逐步处理其余问题。
|
||
|
||
---
|
||
|
||
## 修复记录
|
||
|
||
**修复日期**: 2025-05-31
|
||
**执行方式**: 全量修复（commit: `fix: 审计问题全量修复（安全/YAGNI/Bug/代码质量）`）
|
||
|
||
### 已修复（13 项）
|
||
|
||
| # | 修复内容 | 变更文件 |
|
||
|---|---------|---------|
|
||
| 1 | 移除 CSP 中 esm.sh (Tiptap 残留) 和 cdnjs.cloudflare.com（主题已本地化），CSP 全面收紧为 'self' | `middleware/security.go` |
|
||
| 2 | StatusBanned 分支增加 dummy hash 防时序攻击 | `service/auth_service.go` |
|
||
| 3 | 删除手写 constantTimeEq，改用 `crypto/subtle.ConstantTimeCompare` | `middleware/csrf_token.go`, `middleware/csrf.go` |
|
||
| 4 | 锁定操作消息修正"已删除"→"已锁定" | `controller/admin/admin_controller.go` |
|
||
| 5 | 删除 common.CheckPassword 薄封装，统一改用 bcrypt.CompareHashAndPassword | `common/crypto.go`, `service/auth_service.go` |
|
||
| 6 | 删除 12 个空预留目录（含第二轮追加 5 个） | `templates/MetaLab-2026/html/{post,comment,partials,search,error,admin,topic,user}`, `templates/{system,system/email}`, `templates/MetaLab-2026/static/{img,vendor}` |
|
||
| 9 | 删除 tokenCtrl 别名字段，统一使用 authCtrl | `router/deps_core.go`, `router/deps_extra.go`, `router/api.go` |
|
||
| 10 | 将 check()+recordFail 闭包重构为 try() 原子操作，消除竞态 | `middleware/ratelimit_core.go`, `middleware/ratelimit.go`, `middleware/ratelimit_cleanup.go`, `controller/auth_api_login.go`, `controller/auth_api_register.go` |
|
||
| 11 | 修正 review() 注释编号 3→5→4 | `service/audit_service.go` |
|
||
| 17 | 修正"过清理"→"过期清理" | `router/deps_core.go` |
|
||
|
||
### 已回滚（误删，已排期开发，保留）
|
||
|
||
| # | 回滚内容 | 变更文件 |
|
||
|---|---------|---------|
|
||
| 7 | 恢复 poll/resource shortcode 类型（误删） | `model/shortcode.go`, `service/shortcode_service.go` |
|
||
| 8 | 恢复 redis_store.go 预留实现（误删） | `session/redis_store.go` |
|
||
|
||
### 误报（实际不存在）
|
||
|
||
| # | 说明 |
|
||
|---|------|
|
||
| 12 | 编译产物从未被 git 跟踪，`.gitignore` 已生效，`git rm --cached` 为空操作 |
|
||
|
||
### 暂缓修复（3 项）
|
||
|
||
| # | 暂缓原因 | 后续计划 |
|
||
|---|---------|---------|
|
||
| 13 | 零测试 — 需要建立测试框架、mock 策略，工作量大 | 核心模块优先：hasMinRole、checkAndOperate、RateLimiter |
|
||
| 14 | 无结构化日志 — 需评估 slog vs zap，全量替换 log.Printf | Go 1.21+ 使用标准库 slog 渐进替换 |
|
||
| 15 | 错误上下文丢失 — 涉及全部 repository 层，工作量大 | 按文件逐批添加 `fmt.Errorf("...: %w", err)` |
|