long-agent/user-system
3
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity
Files
main
user-system/docs/code-review/SYSTEMATIC_FIX_PLAN.md

722 lines
20 KiB
Markdown
Raw Permalink Normal View History

docs: project docs, scripts, deployment configs, and evidence
2026-04-02 11:22:17 +08:00
# 系统性代码修复计划
**文档版本**: v2.0
**生成日期**: 2026-03-29
**计划状态**: 待确认
**专家审核**: 已通过安全、性能、代码质量三个专家 agent 审核
---
## 一、修复策略概述
### 1.1 修复阶段划分（已更新）
| 阶段 | 名称 | 优先级 | 问题数 | 预计工作量 |
|------|------|--------|--------|------------|
| Phase 0 | 安全紧急修复 | P0 | 6 | 1-2天 |
| Phase 1 | 核心安全修复 | P1 | 9 | 3-5天 |
| Phase 2 | 性能优化 | P2 | 5 | 2-3天 |
| Phase 3 | 代码质量提升 | P3 | 15+ | 5-7天 |
### 1.2 前置条件
1. **必须先合并 sub2api 最新代码** ⚠️
2. 建立代码审查 CI 流程
3. 准备回滚方案
### 1.3 专家审核总结
| 审核类别 | 方案可行性 | 需注意事项 |
|----------|-----------|------------|
| Phase 0 安全修复 | 90% | SEC-03 需数据迁移方案 |
| Phase 1 安全修复 | 85% | 部分需要用户重置流程 |
| Phase 2 性能优化 | 80% | PERF-01 SQL 需修正 |
| Phase 3 代码质量 | 90% | OAuth State 合并需审计调用方 |
---
## 二、Phase 0: 安全紧急修复 (P0)
### 问题清单
| ID | 问题 | 位置 | 严重程度 | 修复方案 | 状态 |
|----|------|------|----------|----------|------|
| SEC-01 | OAuth ValidateToken 始终返回 true | oauth.go:436 | 严重 | 删除或实现真正验证 | 待修复 |
| SEC-02 | 敏感操作验证绕过 | auth.go:1068-1102 | 严重 | 要求必须有密码或TOTP | 待修复 |
| SEC-03 | 恢复码明文存储 | auth.go:1117-1132 | 严重 | 使用 SHA256 哈希存储 | 待修复 |
| NEW-SEC-01 | Webhook SSRF 风险 | webhook.go:181 | 严重 | 添加 URL 验证和内网IP过滤 | 待修复 |
| SEC-05 | X-Forwarded-For IP 伪造 | ip_filter.go:50-78 | 高危 | 可信代理配置 | 待修复 |
| SEC-11 | rand.Read 错误忽略 | oauth_utils.go:30 | 高危 | **升级为P0** 处理错误返回值 | 待修复 |
### 修复步骤
#### Step 0.1: OAuth ValidateToken 修复 ⚠️ 专家建议删除无参数方法
```go
// 文件: internal/auth/oauth.go
// 专家建议: 直接删除无参数的 ValidateToken，只保留 ValidateTokenWithProvider
// 推荐方案: 删除 ValidateToken 方法
// 或改为调用 GetUserInfo 进行实际验证
func (m *DefaultOAuthManager) ValidateToken(token string) (bool, error) {
if len(token) == 0 {
return false, nil
}
// 遍历所有 provider 进行验证
for _, provider := range m.GetEnabledProviders() {
if ok, _ := m.ValidateTokenWithProvider(provider.Type, token); ok {
return true, nil
}
}
return false, nil
}
```
**专家意见**: 原方法注释已说明无法进行真正验证，建议删除或重命名避免调用方误用。
---
#### Step 0.2: 敏感操作验证修复
```go
// 文件: internal/service/auth.go
// 方案: 当用户没有密码也没有TOTP时，禁止执行敏感操作
func (s *AuthService) verifySensitiveAction(...) error {
hasPassword := strings.TrimSpace(user.Password) != ""
hasTOTP := user.TOTPEnabled && strings.TrimSpace(user.TOTPSecret) != ""
// ⚠️ 专家建议: 必须在验证逻辑之前检查
if !hasPassword && !hasTOTP {
return errors.New("请先设置密码或启用两步验证")
}
// 原有验证逻辑...
if password != "" {
if !auth.VerifyPassword(user.Password, password) {
return errors.New("当前密码不正确")
}
return nil
}
if code != "" {
return s.verifyTOTPCodeOrRecoveryCode(ctx, user, code)
}
return errors.New("password or TOTP verification is required")
}
```
**专家意见**: 需要确认 `user.Password` 字段存储方式（明文/哈希），因为 bcrypt 哈希不应该用 `!= ""` 判断是否设置。
---
#### Step 0.3: 恢复码哈希存储 ⚠️ 专家建议使用 SHA256 而非 bcrypt
```go
// 文件: internal/service/auth.go
// 专家建议: 恢复码是一次性使用场景，不适合 bcrypt（适合可重复验证场景）
// 使用 SHA256 HMAC 更适合
import (
"crypto/hmac"
"crypto/sha256"
"encoding/hex"
)
func hashRecoveryCode(code string) (string, error) {
// 恢复码使用 SHA256 哈希（一次性使用场景不需要 cost factor）
h := sha256.Sum256([]byte(code))
return hex.EncodeToString(h[:]), nil
}
func verifyRecoveryCode(code, hashedCode string) bool {
computedHash := sha256.Sum256([]byte(code))
return hmac.Equal(computedHash[:], []byte(hashedCode))
}
// 数据迁移: 需要添加迁移脚本处理已存在的明文恢复码
// 1. 读取所有用户的 TOTPRecoveryCodes
// 2. 逐个哈希并更新
```
**专家意见**: bcrypt 适合密码等需要反复验证的场景，恢复码一次性使用，用 SHA256 HMAC 更合适。
---
#### Step 0.4: Webhook SSRF 防护 ⚠️ 专家建议补充完整检查
```go
// 文件: internal/service/webhook.go
// 专家建议: 需要补充 localhost 检查和更完整的内网域名过滤
func isSafeURL(rawURL string) bool {
u, err := url.Parse(rawURL)
if err != nil || u.Scheme == "" {
return false
}
// 只允许 http/https
if u.Scheme != "http" && u.Scheme != "https" {
return false
}
host := u.Hostname()
// ⚠️ 禁止 localhost
if host == "localhost" || host == "127.0.0.1" || host == "::1" {
return false
}
// 检查内网 IP
if ip := net.ParseIP(host); ip != nil {
if isPrivateIP(ip) {
return false
}
}
// 检查内网域名
if strings.HasSuffix(host, ".internal") ||
strings.HasSuffix(host, ".local") ||
strings.HasSuffix(host, ".corp") ||
strings.HasSuffix(host, ".lan") {
return false
}
// ⚠️ 专家建议添加: 检查 DNS rebinding
// 如果 URL 解析后的 IP 是内网，则拒绝
// ...
return true
}
// 调用位置: webhook.go:181
func (s *WebhookService) sendWebhook(ctx context.Context, task *DeliveryTask) error {
if !isSafeURL(task.URL) {
return errors.New("webhook URL 不安全")
}
// ...
}
```
**测试用例要求**:
- `http://localhost/` ❌
- `http://127.0.0.1/` ❌
- `http://169.254.169.254/` (AWS) ❌
- `http://10.0.0.1/` ❌
- `http://internal.corp/` ❌
---
#### Step 0.5: IP 伪造防护 ⚠️ 专家建议添加可信代理列表
```go
// 文件: internal/api/middleware/ip_filter.go
// 专家建议: 添加可信代理 IP 列表配置
type IPFilterConfig struct {
TrustProxy bool // 是否信任 X-Forwarded-For
TrustedProxies []string // 可信代理 IP 列表
}
func realIP(c *gin.Context, cfg IPFilterConfig) string {
// 不信任代理时，直接用 TCP 连接 IP
if !cfg.TrustProxy {
return c.ClientIP()
}
xff := c.GetHeader("X-Forwarded-For")
if xff == "" {
return c.ClientIP()
}
// 从右到左遍历（最右边的是最后一次代理添加的）
parts := strings.Split(xff, ",")
for i := len(parts) - 1; i >= 0; i-- {
ip := strings.TrimSpace(parts[i])
if ip == "" {
continue
}
// 检查是否在可信代理列表中
if !isTrustedProxy(ip, cfg.TrustedProxies) {
continue // 不是可信代理，跳过
}
// 是可信代理，返回这个 IP
if !isPrivateIP(ip) {
return ip
}
}
// 没有找到可信代理，使用客户端 IP
return c.ClientIP()
}
func isTrustedProxy(ip string, trusted []string) bool {
for _, t := range trusted {
if ip == t {
return true
}
}
return false
}
```
---
#### Step 0.6: rand.Read 错误处理 ⚠️ 升级为 P0
```go
// 文件: internal/auth/oauth_utils.go
// 专家意见: rand.Read 失败时返回空 state 会导致安全问题
func GenerateState() (string, error) {
b := make([]byte, 32)
// ⚠️ 必须处理错误
if _, err := rand.Read(b); err != nil {
return "", fmt.Errorf("generate state failed: %w", err)
}
state := base64.URLEncoding.EncodeToString(b)
// ...
return state, nil
}
```
---
## 三、Phase 1: 核心安全修复 (P1)
### 问题清单
| ID | 问题 | 位置 | 修复方案 | 注意事项 |
|----|------|------|----------|----------|
| SEC-04 | TOTP 使用 SHA1 | totp.go:25 | 改为 SHA256 | ⚠️ 需用户重置流程 |
| SEC-06 | JTI 包含时间戳 | jwt.go:65 | 移除时间戳 | - |
| SEC-07 | OAuth State TOCTOU | oauth_utils.go:43-62 | 统一锁区域 | ⚠️ 先于 PERF-02 |
| SEC-08 | refresh 无限流 | router.go:108 | 添加限流中间件 | - |
| SEC-09 | CSRF 保护缺失 | auth.go:673-683 | 添加来源验证 | - |
| SEC-10 | Cookie 非 HttpOnly | auth.go:117 | 设置 HttpOnly=true | ⚠️ 需确认用途 |
| SEC-14 | Argon2 参数偏弱 | password.go:29 | 增加 iterations | ⚠️ 渐进式调整 |
| NEW-SEC-02 | Webhook context.Background | webhook.go:255 | 使用带超时 context | - |
| NEW-SEC-03 | 邮件发送用已取消 context | auth_email.go:86-90 | 使用独立 context | - |
### 修复步骤
#### Step 1.1: TOTP 改为 SHA256 ⚠️ 需用户重置流程
```go
// 文件: internal/auth/totp.go
const TOTPAlgorithm = otp.AlgorithmSHA256 // 从 SHA1 改为 SHA256
```
**用户重置流程方案**:
1. 添加数据库迁移标记 `totp_algorithm_upgrade = true`
2. 用户下次登录时提示"请重新设置两步验证"
3. 或提供管理员批量重置选项
---
#### Step 1.2: JTI 移除时间戳
```go
// 文件: internal/auth/jwt.go
func generateJTI() (string, error) {
b := make([]byte, 32) // 32字节熵足够
if _, err := cryptorand.Read(b); err != nil {
return "", err
}
return hex.EncodeToString(b), nil // 移除时间戳
}
```
---
#### Step 1.3: OAuth State TOCTOU 修复 ⚠️ 先于 PERF-02
```go
// 文件: internal/auth/oauth_utils.go
// 专家意见: 必须先修复 TOCTOU，再添加清理 goroutine
func ValidateState(state string) bool {
stateStore.mu.Lock()
defer stateStore.mu.Unlock()
expireTime, ok := stateStore.states[state]
if !ok {
return false
}
if time.Now().After(expireTime) {
delete(stateStore.states, state)
return false
}
delete(stateStore.states, state)
return true
}
```
---
#### Step 1.4: refresh 添加限流
```go
// 文件: internal/api/router/router.go
authGroup.POST("/refresh",
r.rateLimitMiddleware.Refresh(), // 添加限流
r.authHandler.RefreshToken)
```
---
#### Step 1.5: Argon2 增加迭代次数 ⚠️ 渐进式调整
```go
// 文件: internal/auth/password.go
// 专家建议: 渐进式增加，先到 4，观察性能影响后再到 5
return &Password{
memory: 64 * 1024,
iterations: 4, // 从 3 先增加到 4（原来是 3，OWASP 建议 >= 5）
parallelism: 2,
saltLength: 16,
keyLength: 32,
}
```
---
## 四、Phase 2: 性能优化 (P2)
### 问题清单 ⚠️ 专家审核后调整
| ID | 问题 | 位置 | 修复方案 | 专家意见 |
|----|------|------|----------|----------|
| PERF-01 | 认证 4 次 DB 查询 | middleware/auth.go:131 | 合并为 JOIN 查询 | ⚠️ SQL 需修正 |
| PERF-02 | OAuth State 无清理 | oauth_utils.go:23 | 添加清理 goroutine | ⚠️ 必须先修 SEC-07 |
| PERF-03 | findUserForLogin 串行查询 | auth_runtime.go:32 | 使用 OR 查询 | 方案可行 |
| PERF-07 | goroutine 无超时 | auth.go:470 | 添加 5s 超时 | 方案可行 |
| PERF-08 | L1Cache 无清理 | l1.go | 添加定期清理 | 方案可行 |
**已移除**:
- PERF-04: 限流清理问题描述不准确，SlidingWindow 已有过期机制
- PERF-09: AnomalyDetector 已有截断机制，不存在无上限
### 修复步骤
#### Step 2.1: 合并认证查询 ⚠️ SQL 需修正
```go
// 文件: internal/repository/user_role.go
// 专家修正: JOIN 顺序有误，需要修正
func (r *UserRoleRepository) GetUserRolesAndPermissions(ctx context.Context, userID int64) ([]*Role, []*Permission, error) {
// ⚠️ 修正后的 SQL
var results []struct {
RoleID int64
RoleName string
RoleCode string
PermissionID int64
PermissionCode string
}
err := r.db.WithContext(ctx).
Raw(`
SELECT DISTINCT r.id as role_id, r.name as role_name, r.code as role_code,
p.id as permission_id, p.code as permission_code
FROM user_roles ur
JOIN roles r ON ur.role_id = r.id
LEFT JOIN role_permissions rp ON r.id = rp.role_id
LEFT JOIN permissions p ON rp.permission_id = p.id
WHERE ur.user_id = ? AND r.status = 1
`, userID).
Scan(&results).Error
if err != nil {
return nil, nil, err
}
// 处理结果，构建 Role 和 Permission 列表...
}
```
---
#### Step 2.2: findUserForLogin OR 查询
```go
// 文件: internal/repository/user.go
// 方案: 单次查询
func (r *UserRepository) FindByAccount(ctx context.Context, account string) (*User, error) {
var user User
err := r.db.WithContext(ctx).
Where("username = ? OR email = ? OR phone = ?", account, account, account).
First(&user).Error
if err != nil {
return nil, err
}
return &user, nil
}
```
---
#### Step 2.3: 添加超时 context
```go
// 文件: internal/service/auth.go
go func() {
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
defer cancel()
if err := s.loginLogRepo.Create(ctx, loginRecord); err != nil {
log.Printf("auth: write login log failed...")
}
}()
```
---
## 五、Phase 3: 代码质量提升 (P3)
### 问题清单
| 类别 | 问题 | 修复方案 | 专家意见 |
|------|------|----------|----------|
| 代码重复 | OAuth State 重复 | 合并到 state.go | ⚠️ 需审计调用方 |
| 代码重复 | Handler 授权函数重复 | 提取到 authz.go | ⚠️ 统一错误处理 |
| 代码重复 | 分页逻辑重复 | 统一 PaginationParams | 方案可行 |
| 代码质量 | 魔法数字 | 定义常量 | 方案可行 |
| 代码质量 | 错误处理不一致 | 统一错误类型 | ⚠️ response.Error 可能存在 bug |
| 代码质量 | 正则重复编译 | 预编译 | 方案可行 |
### 修复步骤
#### Step 3.1: OAuth State 合并 ⚠️ 需审计调用方
```
修复步骤:
1. 审计所有调用方，确定使用的是哪个实现
- 搜索 GenerateState 调用
- 搜索 ValidateState 调用
- 搜索 stateStore 访问
2. 统一使用 state.go 的 StateManager
- 修改 GetStateManager() 初始化
- 确保 package 级别 stateStore 指向 StateManager
3. 删除 oauth_utils.go 中的重复代码
- 删除 stateStore 变量
- 删除重复的 GenerateState/ValidateState
- 保留其他 OAuth 辅助函数
4. 回归测试所有 OAuth 流程
```
---
#### Step 3.2: 分页逻辑统一
```go
// 创建 internal/api/handler/pagination.go
type PaginationParams struct {
Page int
PageSize int
Offset int
}
const (
DefaultPageSize = 20
MaxPageSize = 100
)
func ParsePageParams(c *gin.Context) PaginationParams {
page, _ := strconv.Atoi(c.DefaultQuery("page", "1"))
pageSize, _ := strconv.Atoi(c.DefaultQuery("page_size", strconv.Itoa(DefaultPageSize)))
if page < 1 {
page = 1
}
if pageSize < 1 || pageSize > MaxPageSize {
pageSize = DefaultPageSize
}
return PaginationParams{
Page: page,
PageSize: pageSize,
Offset: (page - 1) * pageSize,
}
}
```
---
#### Step 3.3: 魔法数字定义常量
```go
// 创建 internal/pkg/constants/constants.go
package constants
import "time"
const (
// Password
Argon2Memory = 64 * 1024
Argon2Iterations = 4 // 渐进式调整
Argon2Parallelism = 2
Argon2SaltLength = 16
Argon2KeyLength = 32
// OAuth State
OAuthStateTTL = 10 * time.Minute
OAuthStateCleanupInterval = 5 * time.Minute
// Pagination
DefaultPageSize = 20
MaxPageSize = 100
// Login
MaxLoginAttempts = 5
LoginLockDuration = 15 * time.Minute
// Cache
DefaultUserCacheTTL = 15 * time.Minute
DefaultBlacklistTTL = 1 * time.Hour
)
```
---
#### Step 3.4: 正则预编译
```go
// 文件: internal/security/validator.go
// 在包级别预编译正则表达式
var (
sqlCommentRegex = regexp.MustCompile(`(?i);[\s]*--`)
blockCommentRegex = regexp.MustCompile(`(?i)/\*.*?\*/`)
xpProcRegex = regexp.MustCompile(`(?i)\bxp_\w+`)
execRegex = regexp.MustCompile(`(?i)\bexec[\s\(]`)
unionSelectRegex = regexp.MustCompile(`(?i)\bunion[\s]+select`)
// ... 更多预编译正则
)
func (v *Validator) SanitizeSQL(input string) string {
result := input
// 使用预编译的正则
result = sqlCommentRegex.ReplaceAllString(result, "")
result = blockCommentRegex.ReplaceAllString(result, "")
// ...
return result
}
```
---
## 六、新增安全问题（专家审核发现）
### 遗漏的 P1 问题
| ID | 问题 | 位置 | 严重程度 | 修复方案 |
|----|------|------|----------|----------|
| SEC-NEW-1 | 登录失败无限流 | auth.go | 高 | 添加限流 |
| SEC-NEW-2 | 密码复杂度验证不足 | password_policy.go | 中 | 添加强度检查 |
---
## 七、修复执行计划
### 7.1 时间安排
| 周次 | Phase | 任务 | 里程碑 |
|------|-------|------|---------|
| Week 1 | Phase 0 | 安全紧急修复 | 6 个 P0 问题修复完成 |
| Week 2 | Phase 1 | 核心安全修复 | 9 个 P1 问题修复完成 |
| Week 3 | Phase 2 | 性能优化 | 5 个性能问题修复完成 |
| Week 4-5 | Phase 3 | 代码质量提升 | 代码重复和质量问题修复 |
### 7.2 代码合并流程
```
⚠️ 前置条件: 合并 sub2api 最新代码
Phase 0:
1. git checkout -b fix/security-phase-0
2. 修复 SEC-01, SEC-02, SEC-03, NEW-SEC-01, SEC-05, SEC-11
3. 运行测试: go test ./...
4. 手动安全测试
5. Code Review
6. git merge to main
Phase 1:
1. git checkout -b fix/security-phase-1
2. 修复剩余安全问题
3. 测试 + Review + Merge
Phase 2 & 3: 同上
```
### 7.3 验证清单
每个 Phase 完成后需要验证：
- [ ] 所有单元测试通过 `go test ./...`
- [ ] 集成测试通过
- [ ] 手动安全测试通过
- [ ] 性能测试无退化（基准测试）
- [ ] 回归测试（OAuth、登录、设备管理等核心流程）
---
## 八、关键文件清单
| 文件 | 涉及问题 |
|------|----------|
| `internal/auth/oauth.go` | SEC-01 |
| `internal/service/auth.go` | SEC-02, SEC-03, PERF-07 |
| `internal/service/webhook.go` | NEW-SEC-01, NEW-SEC-02 |
| `internal/api/middleware/ip_filter.go` | SEC-05 |
| `internal/auth/oauth_utils.go` | SEC-07, SEC-11, PERF-02 |
| `internal/auth/totp.go` | SEC-04 |
| `internal/auth/jwt.go` | SEC-06 |
| `internal/auth/password.go` | SEC-14 |
| `internal/api/middleware/auth.go` | PERF-01 |
| `internal/repository/user_role.go` | PERF-01 |
| `internal/repository/user.go` | PERF-03 |
| `internal/cache/l1.go` | PERF-08 |
---
## 九、风险控制
### 9.1 回滚方案
每个修复需要同时提交：
1. 修复代码
2. 对应的单元测试
3. 回滚脚本（如需要）
### 9.2 监控告警
修复后需要监控：
- [ ] 认证失败率异常上升
- [ ] API 响应时间 P99 > 500ms
- [ ] 错误日志中安全相关关键词
- [ ] Webhook 投递失败率
### 9.3 专家审核意见汇总
| 问题 | 审核结论 |
|------|----------|
| SEC-01 | 方案可行，建议删除无参方法 |
| SEC-02 | 方案可行，需确认 Password 字段存储方式 |
| SEC-03 | 方案可行，建议用 SHA256 替代 bcrypt |
| SEC-05 | 方案可行，建议添加可信代理列表 |
| SEC-07 | 方案可行，必须先于 PERF-02 |
| PERF-01 | 方案可行，SQL 需修正 |
| PERF-08 | 方案可行，需添加定期清理 goroutine |
---
*本计划由代码审查系统生成，已通过专家 agent 审核，待确认后执行*
*版本历史: v1.0 初稿, v2.0 专家审核后更新*
Reference in New Issue Copy Permalink