audit-report.md 40 KB

权限管理系统 - 深度代码审计报告(第 6 轮)

审计范围:/internal 下全部非测试、非 _gen.go 生产代码(含 internal/server/permserver.go、HTTP logic / handler / middleware、loaders、model 定制层、svc、util)。 审计时间:2026-04-19(第 5 轮修复复盘 + 深扫一轮新代码面) 审计重点:

  • 账号锁定 / 账号枚举相关的侧信道 —— 第 5 轮未覆盖
  • SyncPermissions 并发修复的真正落地程度(第 5 轮 M-6 给了基础设施 LockByCodeTx/FindMapByProductCodeWithTx,但实际 service 是否真的串行化)
  • UpdateDept 缓存失效批处理:上轮 M-2 给的结论是"建议实现 CleanMany / pipeline",本轮确认其有无落地
  • 读放大路径(logic 层一次请求里 FindOne(targetUserId) 被调用 3~4 次)
  • 水平越权 / 信息泄露:ProductList / ProductDetail / DeptTree / UserDetail 对最低权限用户暴露面是否超额
  • JWT 解析契约:token.Method 的显式校验

相对第 5 轮:H-1(IncrementTokenVersionIfMatch CAS)、H-2(gRPC RefreshToken/VerifyToken 限流)、H-3(GuardRoleLevelAssignable 严格低于自身)、H-4(UpdateUser deptId=0 ADMIN 守门)、L-1(MustChangePasswordYes 默认开启)、L-4(FindMinPermsLevelByUserIdAndProductCode 区分 NotFound / 其它 err)均已实际落地(阅读 HEAD 代码验证)。M-3(negativeCacheMarker 负缓存)、M-5(CreateProduct 移除 strings.Contains 脆弱匹配)、M-7(gRPC Login IP 剥 host:port)也已落地。本轮暴露的是:M-2 未落地(部门用户缓存仍串行 Clean)、M-6 只落地一半(1062 → 409 已做,但 LockByCodeTx 在 service 里还没接上)、以及一组围绕登录侧信道、管理员账号 DoS、info disclosure的新 H/M 级问题。


🚩 核心逻辑漏洞 (High Risk)

H-1. AdminLogin 限流 key 只含 username(不含 IP)——任意远端可永久锁死任意超管账号

  • 位置internal/logic/pub/adminLoginLogic.go:41-46
  • 描述
  if l.svcCtx.UsernameLoginLimit != nil {
      code, _ := l.svcCtx.UsernameLoginLimit.Take(req.Username)   // ← key 只有 username
      if code == limit.OverQuota {
          return nil, response.NewCodeError(429, "该账号登录尝试过于频繁,请5分钟后再试")
      }
  }

UsernameLoginLimit 当前配置是 NewPeriodLimit(300, 10, …)servicecontext.go:45),即每 5 分钟 10 次。同接口的产品端登录用 ip:username 作为 key(见 loginService.go:40),admin 路径完全没有 IP 维度。 攻击链:

  1. 攻击者通过 POST /api/auth/adminLogin 用一个已知的超管用户名(比如 admin_<productCode>,见下面的结构性放大点)连打 10 次错误密码 → 触发 5 分钟封禁;
  2. 攻击者每 5 分钟刷新一次,只需要极低带宽就能让该账号永远没有 5 分钟内的合法登录窗口
  3. 攻击者甚至不用在同一个 IP 上,任何网络都可以维持这个封禁循环;
  4. 合法超管在任何 IP、任何地理位置都无法登录。

结构性放大点(这才是真正让这条变 High 的地方):CreateProductLogic.go:77 里产品管理员账号的用户名是 admin_<productCode>,是可从 ProductList 端点枚举出来的ProductList 不做任何访问控制,见 M-3):任何一枚普通登录账号都可以拉到 code 列表,立刻得到所有产品 admin 账号用户名。

攻击者因此可以:

  • 一次性把全站所有产品 admin 都锁掉adminLogin 不走 JWT 中间件,只需要能到达 /api/auth/adminLogin);
  • 配合"managementKey 即便泄露也仅定位到请求能被送出去"这一事实:整条链不要求攻击者有合法凭证,只要 managementKey 写错即可,每次请求都会在 UsernameLoginLimit.Take(username) 处计数(上面代码是先限流再做 managementKey 校验的顺序吗?——不是,managementKey 校验在第 37 行先做,失败直接 return,不会走到 Take)。
  • 但即便 managementKey 正确,用错密码还是会计数。也就是说:攻击者只要一次 valid managementKey 泄露(或运维错误把它 push 到 git),这个 DoS 立即变成"可长期维持"。

  • 影响

    • 所有产品的 ADMIN 账号被远端静默批量锁死,业务侧显示的只是"登录过于频繁,请 5 分钟后再试",没有任何 IP 信号可被风控切入;
    • 与 L-5(ExtractClientIPbehindProxy=true 时信任 X-Real-IP)叠加:攻击者发假 X-Real-IP 不会绕过 admin 限流(因为 key 根本没用 IP),反而会让产品登录的限流绕过——两条攻击面互补;
    • 根本上违反了 OWASP ASVS 2.2.1"按用户限流必须包含 IP 或设备维度"。
  • 修复方案

    1. 把 key 改成IP + username 双键(与产品登录对齐),并额外保留一个 username 维度但配额更大的桶作为"全局上限":

      // per-IP + per-user:防暴力破解,严格
      perIPUserKey := fmt.Sprintf("admin:%s:%s", clientIP, req.Username)
      if code, _ := l.svcCtx.UsernameLoginLimit.Take(perIPUserKey); code == limit.OverQuota {
       return nil, response.NewCodeError(429, "该账号登录尝试过于频繁,请稍后再试")
      }
      // per-user 全局:防分布式爆破,配额宽松(100/5min)
      if code, _ := l.svcCtx.AdminUserSoftLimit.Take("admin:u:" + req.Username); code == limit.OverQuota {
       // 告警但不强制返回 429,记录可疑事件
       logx.WithContext(l.ctx).Errorf("admin_login suspicious rate u=%s ip=%s", req.Username, clientIP)
      }
      
      1. adminLogin 必须取出 clientIP(目前 handler 里并没有把 clientIP 注入 ctx,需先挂载 RateLimitMiddleware 或手动 middleware.ExtractClientIP)。
      2. 连续失败 N 次后,对 IP 封禁(按 IP 拉黑几分钟),而不是对 username 封禁——永远不要把 DoS 面留给 "攻击者控制的 key"。

      H-2. ValidateProductLogin 在"状态=冻结"分支早于 bcrypt 返回:构造账号存在性 / 冻结态的时序 + 明文错误信息双重信道

      • 位置internal/logic/pub/loginService.go:50-65
      • 描述: 关键顺序: go u, err := svcCtx.SysUserModel.FindOneByUsername(ctx, username) // L50 if err != nil { if errors.Is(err, user.ErrNotFound) { bcrypt.CompareHashAndPassword(dummyBcryptHash, []byte(password)) // L53:等时 dummy return nil, &LoginError{Code: 401, Message: "用户名或密码错误"} } return nil, err } if u.Status != consts.StatusEnabled { return nil, &LoginError{Code: 403, Message: "账号已被冻结"} // L59-60:**没有 bcrypt** } if err := bcrypt.CompareHashAndPassword([]byte(u.Password), []byte(password)); err != nil { return nil, &LoginError{Code: 401, Message: "用户名或密码错误"} // L63-65:有 bcrypt }

第一个分支(用户名不存在)有意做了 dummy bcrypt 等时。但第二个分支(账号被冻结)直接 return,跳过了 bcrypt。三种输入的耗时差异: | 输入 | 耗时 | 响应消息 | code | | ------------------------------- | -------------------------- | ------------------ | ---- | | 用户名不存在 | ≈ bcrypt(dummy) | "用户名或密码错误" | 401 | | 用户名存在但 Status=2(冻结) | ≪ bcrypt(直接跳过) | "账号已被冻结" | 403 | | 用户名存在,密码错 | ≈ bcrypt | "用户名或密码错误" | 401 | | 用户名存在,密码对 | ≈ bcrypt + 后续一串 DB IO | 登录成功 | 200 |

两种独立的侧信道各自就足够用了:

  • 响应消息 + HTTP code403 "账号已被冻结" 是独一无二的(401 "用户名或密码错误" 覆盖了"不存在"和"密码错"两种)——攻击者每次请求都在做一次无条件的账号存在性 + 状态 oracle

实际攻击路径:

  1. 人员离职后,HR 走流程把账号状态置为冻结 → Status=2 驻留在 DB;这时 refreshToken 还没过期(refreshExpire 往往是 7~30 天);
  2. 攻击者用公司常见命名规则批量扫一遍 zhangsan / lisi / admin_xxx,非 403 都排除,只留 403 的一批 → 这一批就是离职但还在 JWT 窗口里的高价值目标
  3. 攻击者用钓鱼 / credentials stuffing / 内部 IM 撬这一批账号的 refreshToken,命中率显著高于随机喷撒。

同样的 pattern 也出现在 adminLoginLogic.go:57-67(有 bcrypt 再检查 status/superAdmin,但错误 message 都归一到"用户名或密码错误"——这一版 adminLogin 做对了)。证明开发团队知道这个 pattern,但 product login 这条没同步修。

  • 影响

    • 账号存在性 / 冻结态单次请求可探测。配合 H-1(admin 账号可从 ProductList 枚举),攻击者可以画出全组织的"谁在,谁离职了,谁被短期冻结了"的状态图谱;
    • 违反 OWASP ASVS V2.1.12 "系统 shall not reveal account status or existence";
    • 离职用户处于"冻结但 JWT 还在期内"的短窗(几天)是最容易被 social engineering 命中的。
  • 修复方案: 把 bcrypt 变成无条件总是执行,状态 / superAdmin 禁令走登录成功之后的统一检查,并且所有用户侧可见的失败消息归一为"用户名或密码错误":

    u, err := svcCtx.SysUserModel.FindOneByUsername(ctx, username)
    var userHash []byte
    if err != nil {
      if errors.Is(err, user.ErrNotFound) {
          userHash = dummyBcryptHash // 等时
      } else {
          return nil, err
      }
    } else {
      userHash = []byte(u.Password)
    }
    // 无论用户是否存在、是否冻结,统一 bcrypt 一次
    bcryptErr := bcrypt.CompareHashAndPassword(userHash, []byte(password))
    if err != nil || bcryptErr != nil {
      return nil, &LoginError{Code: 401, Message: "用户名或密码错误"}
    }
    if u.Status != consts.StatusEnabled {
      // 密码正确的冻结用户,才提示冻结(此时攻击者已经猜中密码,再保留"冻结"已无意义)
      return nil, &LoginError{Code: 403, Message: "账号已被冻结"}
    }
    if u.IsSuperAdmin == consts.IsSuperAdminYes {
      return nil, &LoginError{Code: 403, Message: "超级管理员请使用管理后台登录"}
    }
    

    效果:账号不存在 / 冻结 / 密码错三者对外完全不可区分——响应耗时一致、消息一致、code 一致。只有密码正确后才"奖励性"地暴露后续语义。


    H-3. SyncPermsService 并发 1062 修复只落地一半LockByCodeTx 已实现但没在 service 里调用

    • 位置internal/logic/pub/syncPermsService.go:66-120internal/model/product/sysProductModel.go:56-63
    • 描述: 第 5 轮 M-6 建议的完整修复是"FOR UPDATE 锁 product → tx 内 load existing → tx 内写"。当前 HEAD:
    • sysProductModel.LockByCodeTx 已经实现(sysProductModel.go:56-63);
    • sysPermModel.FindMapByProductCodeWithTx 已实现(sysPermModel.go:98-109);
    • syncPermsService.go 完全没有调用这两个——existingMap 仍然在 tx 外被 FindMapByProductCode 读(L66),tx 内只做写(L106-120)。

    代码里留了一段自认的注释:

    // NOTE(R5-M-6):理想方案是"同 tx 内先 SELECT ... FOR UPDATE 锁 sys_product 行…";
    // 但当前 mock 契约(syncPermsLogic_mock_test.go)把 FindMapByProductCodeWithTx 固定在 tx 外,
    // 为不破坏测试约定,保留了原先的"tx 外预读 + tx 内写入"结构。
    

问题:

  1. 被测试约束反向拽住 = 架构决策被测试约束倒挂。正确做法是修测试 mock,让生产代码符合正确语义,而不是反过来;
  2. "1062 → 409" 的兜底只是缓解症状:调用方看到 409 要重试,极端情况两个并发同步同时在重试仍可能继续撞,形成活锁;
  3. 实际并发:同一个产品多部署实例启动时都会调一次 POST /perm/sync(部署流水线常见),两边都在热启动瞬间命中——409 并不比 500 友好多少,只是重试路径成立了。
  • 影响

    • 同步接口在部署时概率性失败,客户端必须自己做重试(而且当前 SyncPermsError 结构只给 Code/Message,没有 Retry-After 提示);
    • 上一轮的修复已经把基础设施准备好了(LockByCodeTx 是用代码+测试覆盖过的),缺的只是最后接上——这是一条"几十行代码 + 改 mock"可以直接落地的事情,风险收益比极高。
  • 修复方案: 把 mock 测试里的 FindMapByProductCode 预期调用改为 FindMapByProductCodeWithTx,然后把 service 改为:

    err = svcCtx.SysPermModel.TransactCtx(ctx, func(txCtx context.Context, session sqlx.Session) error {
      // 1. 锁 product 行,把同一 product 的 SyncPermissions 串行化
      if _, err := svcCtx.SysProductModel.LockByCodeTx(txCtx, session, product.Code); err != nil {
          if errors.Is(err, sqlx.ErrNotFound) {
              return &SyncPermsError{Code: 404, Message: "产品不存在"}
          }
          return err
      }
      // 2. tx 内读 existing
      existingMap, err := svcCtx.SysPermModel.FindMapByProductCodeWithTx(txCtx, session, product.Code)
      if err != nil { return err }
      // 3. tx 内写(按原逻辑分 insert / update / disable)
      ...
    })
    

    LockByCodeTx 把同一 product 的并发同步串行化后,ON DUPLICATE KEY UPDATE / 1062 兜底都可以不再依赖。


    ⚠️ 健壮性与性能建议 (Medium/Low)

    M-1. UpdateDeptLogic 的 Clean 循环仍未批处理(第 5 轮 M-2 未落地)

    • 位置internal/logic/dept/updateDeptLogic.go:86-94
    • 描述go if deptTypeChanged || statusChanged { userIds, _ := l.svcCtx.SysUserModel.FindIdsByDeptId(l.ctx, req.Id) for _, uid := range userIds { l.svcCtx.UserDetailsLoader.Clean(l.ctx, uid) } ... }

每次 Clean 内部是 SMEMBERS → DEL(批) → DEL(索引)userDetailsLoader.go:204-218),即 3 个 Redis RTT。一个有 300 个成员的部门改 deptType 会串行产生 900 次 RTT,卡在 HTTP handler 里。即使 Redis 0.5ms,300 个用户也至少 450ms——这个窗口内 handler 被 hold 住,其他写入堆积,连带拖慢 UpdateDept 的乐观锁重试。 第 5 轮已把问题点清楚暴露,并给出 CleanByUserIds 的草案,但本轮代码里没有这个方法cleanByIndex 也没升级成 pipeline。

  • 建议

    1. UserDetailsLoader 新增 CleanByUserIds(ctx, ids []int64):合并所有用户的 userIndexKey 一次 SMEMBERS pipeline → 合并所有 cacheKey 一次 DEL → 索引键一次 DEL,RTT 降到 3 次常数;
    2. 如果仍想保留 Clean 单用户语义,在 UpdateDeptLogic 里改成一次 batch:

      if deptTypeChanged || statusChanged {
       userIds, err := l.svcCtx.SysUserModel.FindIdsByDeptId(l.ctx, req.Id)
       if err != nil {
           logx.WithContext(l.ctx).Errorf("clean dept users cache, FindIdsByDeptId failed: %v", err) // 不能再 "_," 把错吞了
       } else if len(userIds) > 0 {
           l.svcCtx.UserDetailsLoader.CleanByUserIds(l.ctx, userIds)
       }
      }
      
      1. 顺手把 _, _ = FindIdsByDeptId(...)错误静默吞掉修掉:当前代码把查询 error 直接丢,会导致"DB 抖动 → 缓存没清 → 旧权限缓存继续生效 5 分钟",对"被禁用的研发部"这种安全敏感变更是静默绕过

      M-2. ProductList / ProductDetail / DeptTree 对任意登录用户暴露全公司清单

      • 位置
      • internal/logic/product/productListLogic.go:29-58(无任何访问控制)
      • internal/logic/product/productDetailLogic.go:29-48(无任何访问控制)
      • internal/logic/dept/deptTreeLogic.go:27-66(无任何访问控制)
      • 描述: 三个接口都挂在 JwtAuth 中间件后面(见 handler/routes.go:47-74, 119-146),但 logic 层里只做了"caller 非超管时遮蔽 AppKey"这种字段级防护,没有行/资源级防护:
      • 一个只属于产品 A 的 MEMBER 级账号,可以从 ProductList 拉到全站所有产品的 code / name / remark / status
      • 同样可以从 ProductDetail 按 ID 逐个拉别的产品的详情(只是看不到 AppKey);
      • DeptTree 直接返回整棵组织架构树,包括 MEMBER 根本无权管的兄弟部门和叔辈部门;

      这几个泄露叠加起来就是 H-1 的"可枚举 admin 用户名"的根源——一个 MEMBER 登录之后:

      1. /api/product/list → 拉到 code 列表 → 拼出 admin_<code> 全量用户名;
      2. /api/dept/tree → 得到对手组织结构 / 研发部节点位置,为 loadPerms 特权判定(DeptType == DEV 自动拥有全权)提供情报——知道哪些部门是 DEV,就知道哪些账号一旦拿下即"产品全权"。
      3. 枚举结果喂给 H-1 的 admin DoS 或针对性的钓鱼。
      • 建议
      • ProductList / ProductDetail非超管只能看自己 caller.ProductCode 对应的那一条。列表直接返回 [自己产品] 或空;详情校验 product.Code == caller.ProductCode
      • DeptTree非超管按 caller.DeptPath 剪枝返回——只返回以 caller.DeptPath 为根的子树 + 与业务需要对齐的上级路径链(通常做法是回传 ancestors 字段而非整棵树)。
      • 如果确实产品要"给所有员工看公司架构"这种展示场景,应有独立的 /api/org/publicTree 端点,返回脱敏后的字段(无 remark、无 status、无 createTime),并显式记为"公开"。

      M-3. UserDetailLogicPhone / Email 返回给任意同产品成员

      • 位置internal/logic/user/userDetailLogic.go:29-77
      • 描述go if !caller.IsSuperAdmin { if _, err := l.svcCtx.SysProductMemberModel.FindOneByProductCodeUserId(l.ctx, caller.ProductCode, req.Id); err != nil { return nil, response.ErrForbidden("无权查看非本产品成员的用户信息") } } ... return &types.UserItem{ ... Email: user.Email, Phone: user.Phone, ... }, nil

判定规则是"同产品成员即可查看"——一个产品里最低权限 MEMBER 可以遍历同产品所有用户(userId 范围内 fuzz)的手机 / 邮箱。同产品有几百上千成员时,这等同于暴露公司通讯录 PII。 这里没有"调用者对目标有管理权"或"看自己"的更细粒度条款。对照 BindRoles / UpdateUser 使用 CheckManageAccess(含部门层级),UserDetail 是最宽松的。

  • 建议
    1. Phone / Email 应仅对满足以下之一的 caller 返回:
      • caller.UserId == user.Id(自己);
      • caller.IsSuperAdmin || caller.MemberType == consts.MemberTypeAdmin
      • CheckManageAccess(...) == nil(即调用者管辖该目标); 其余 caller 只返回脱敏字段(例如 13***1234)或不返回。
    2. 在返回 UserItem 时,加一个 filterPIIForCaller(user, caller) 的 helper,所有返回用户详情 / 列表的 logic 都走同一个过滤器,避免未来再漏。

M-4. BindRolePermsLogic / UpdateRoleLogic写成功后的缓存清理失败时返回 500,客户端会把成功的写误判为失败并重试

  • 位置
    • internal/logic/role/bindRolePermsLogic.go:128-134
    • internal/logic/role/updateRoleLogic.go:79-85
  • 描述: ```go if err := l.svcCtx.SysRolePermModel.TransactCtx(...); err != nil { return err }

affectedUserIds, err := l.svcCtx.SysUserRoleModel.FindUserIdsByRoleId(l.ctx, req.RoleId) if err != nil {

  logx.WithContext(l.ctx).Errorf("角色权限已更新但缓存清理失败 roleId=%d: %v", req.RoleId, err)
  return response.NewCodeError(500, "权限已更新但缓存刷新失败,请稍后手动刷新")

} l.svcCtx.UserDetailsLoader.BatchDel(l.ctx, affectedUserIds, role.ProductCode)

  出问题的细节在三个层面:
  1. `FindUserIdsByRoleId` 是一次**普通 SELECT**,失败通常意味着 DB 抖动或连接池耗尽——此时**绑定权限的事务已经 commit**,DB 事实数据已改。给客户端返回 500 会让上层重试整个 `BindRolePerms`;重试时先 diff 得到 `toAdd/toRemove` 都为空 → `if len(toAdd) == 0 && len(toRemove) == 0 { return nil }`(`bindRolePermsLogic.go:102-104`)→ **静默返回 200,但期间的业务逻辑 / 回调不会再触发**。更糟的变体:`BindRoles`(`user/bindRolesLogic.go:118-121`)的同一 pattern 里有 `UserDetailsLoader.Clean` 但没有 `FindUserIdsByRoleId` 这步,所以 BindRoles 不受此影响,只有 `BindRolePerms` / `UpdateRole` 有;
  2. `BatchDel` 失败只打 log 不改响应——那就让 `FindUserIdsByRoleId` 失败也同样处理才对称;
  3. 业务表达:500 的 "权限已更新但缓存刷新失败" 这段话**既给客户端看又给运维看**,但客户端看到 500 不会知道"我不用重试,只要等 5 分钟缓存 TTL 自然过期即可"——这是文案 / 状态码错配。

- **建议**:
  1. 把这两处改成 "事务成功即视为成功;缓存刷新失败仅 log + 异步重试":
     ```go
     if err := ...TransactCtx(...); err != nil { return err }

     if userIds, err := l.svcCtx.SysUserRoleModel.FindUserIdsByRoleId(l.ctx, req.RoleId); err == nil {
         l.svcCtx.UserDetailsLoader.BatchDel(l.ctx, userIds, role.ProductCode)
     } else {
         logx.WithContext(l.ctx).Errorf("postcommit cache cleanup fetch userIds failed roleId=%d: %v", req.RoleId, err)
         // TODO(可选):入异步队列重试,或记 audit 让运维手工刷新
     }
     return nil
     ```
  2. 真的要让客户端知道 "权限变更成功但部分缓存未刷新",应返回 200 + resp 里带 `cacheRefreshStatus: "degraded"`,而不是 500。

---

### M-5. `UpdateUserStatusLogic` / `BindRolesLogic` / `UpdateUserLogic` 的**同请求重复 `FindOne(targetUserId)`**(缓存命中但仍有 Redis 往返)

- **位置**:
  - `internal/logic/user/updateUserStatusLogic.go:37-50`(3 次)
  - `internal/logic/user/bindRolesLogic.go:40-50`(2 次 `FindOne(userId)` + 2 次 `FindOneByProductCodeUserId`)
  - `internal/logic/user/updateUserLogic.go:47-58`(2 次 `FindOne(userId)`)
- **描述**:
  以 `UpdateUserStatus` 为例:

ValidateStatusChange(...) → SysUserModel.FindOne(targetUserId) CheckManageAccess(...) →

  checkDeptHierarchy(...) → SysUserModel.FindOne(targetUserId)
  checkPermLevel(...) → SysProductMemberModel.FindOneByProductCodeUserId(...)

// 回到 handler user, _ := SysUserModel.FindOne(targetUserId)

  全部通过 go-zero cache 命中,但每次 `FindOne` 还是要 Redis GET(若 Redis 上无 key 会触发 DB)。即便 Redis GET 是 0.5ms,3 次同一 key 就浪费 1.5ms + 3 个连接池 slot;更糟的是:在"`FindOne` → `UpdateProfile` → `FindOne`(下一次)"的时间窗口里,如果上一次 `UpdateProfile` 把 cache invalid 掉了,下一次 `FindOne` 会触发一次 DB 查询。**同一个 request 内部**本不该做这种往返。

- **建议**:
  1. 在 `CheckManageAccess` 签名里加一个可选 `prefetchedTarget *user.SysUser`:调用方已经有目标用户对象时,直接传进去,`checkDeptHierarchy` 复用;否则再查:
     ```go
     func CheckManageAccess(ctx, svcCtx, targetUserId, productCode, opts ...Option) error {
         var target *user.SysUser
         for _, opt := range opts { opt.apply(&target) }
         if target == nil {
             target, _ = svcCtx.SysUserModel.FindOne(ctx, targetUserId)
         }
         ...
     }
     ```
  2. 更激进:在 handler 最外层或 middleware 里做**请求级 cache**(`context.WithValue` 一个小 map),`FindOne`/`FindOneByProductCodeUserId` 走这层再透传。这对所有类似 `UpdateUser + Check*` 的组合都直接受益。

---

### M-6. `ExtractClientIP` 在 `behindProxy=true` 时**只信任 `X-Real-IP`**,没 `X-Forwarded-For` fallback;且**未设头时回落到 proxy 的 RemoteAddr → 全站共享一个桶**

- **位置**:`internal/middleware/ratelimitMiddleware.go:54-65`
- **描述**:
  ```go
  func ExtractClientIP(r *http.Request, behindProxy bool) string {
      if behindProxy {
          if ip := r.Header.Get("X-Real-IP"); ip != "" { return ip }
      }
      host, _, err := net.SplitHostPort(r.RemoteAddr)
      if err != nil { return r.RemoteAddr }
      return host
  }

两个真实业务场景会 bug:

  1. 部分运维 / 反向代理(特别是 K8s Ingress-nginx 默认配置)只设 X-Forwarded-For,不设 X-Real-IP。本实现会完全忽略 X-Forwarded-For,回落到 r.RemoteAddr——那个值是 ingress controller 的 IP → 全部请求共享一个 IP 限流桶
  2. 反向代理忘记把客户端 X-Real-IP 清掉的话,客户端能直接自己伪造 X-Real-IP: 1.1.1.1,每次变换轻松绕过限流。 综合后果:一旦部署姿势稍有偏差,/api/auth/login / /api/auth/refreshToken / /api/perm/sync 上的限流全部等价于"共享一个桶或无限流",配合 H-1/H-2 直接放大。
  • 建议
    1. 按常规反代优先级解析 IP:X-Forwarded-For(取第一段非私网)→ X-Real-IPRemoteAddr
    2. 显式校验解析结果是合法的 IPnet.ParseIP(ip) != nil),非法(空串、乱码)时退化到 RemoteAddr,并打印 warn 日志让运维能尽快发现代理链漏配;
    3. behindProxy 开关和线上部署姿势必须在 README 显式对齐;如果线上实际用的是 ingress-nginx,得 behindProxy=true + 解析 X-Forwarded-For

M-7. JWT 解析三处(HTTP jwtauthMiddleware / gRPC VerifyToken / ParseRefreshToken都没显式检查 token.Method

  • 位置
    • internal/middleware/jwtauthMiddleware.go:59-61
    • internal/server/permserver.go:242-244
    • internal/logic/auth/jwt.go:78-80
  • 描述: 三处 keyfunc 直接返回 []byte(secret),没有做:

    if _, ok := token.Method.(*jwt.SigningMethodHMAC); !ok {
      return nil, fmt.Errorf("unexpected signing method: %v", token.Header["alg"])
    }
    

    jwt/v4 默认对 alg=none 是关闭的(SigningMethodNone 需要显式 jwt.UnsafeAllowNoneSignatureType),所以目前没有立即可利用的漏洞。但:

    1. 如果未来 refresh / access secret 被替换成 RSA / ECDSA 密钥(业务上合理:双密钥轮转、JWKS),keyfunc 返回 []byte 就会被 SigningMethodRSA.Verify 当 public key 解析失败,这本身安全——但业务上会突然全站登录失败,调试困难;
    2. 更重要的是:显式 token.Method 检查是深度防御的行业标准(OWASP JWT Cheat Sheet),任何 review 过 JWT 代码的审计都会拉出来。
    3. 建议:抽一个 parseHS256(tokenStr, secret, claims) 的小 helper,统一在 keyfunc 里断言 *jwt.SigningMethodHMAC,并在 ParseRefreshToken / access token 解析两处调用。

    M-8. IncrementTokenVersion / IncrementTokenVersionIfMatchFindOne 一次只为拿 username 构造 cache key

    • 位置internal/model/user/sysUserModel.go:158-181, 186-214
    • 描述go func (m *customSysUserModel) IncrementTokenVersionIfMatch(ctx context.Context, id, expected int64) (int64, error) { data, err := m.FindOne(ctx, id) // ← 只为下面的 usernameKey if err != nil { return 0, err } sysUserIdKey := fmt.Sprintf("%s%v", cacheSysUserIdPrefix, id) sysUserUsernameKey := fmt.Sprintf("%s%v", cacheSysUserUsernamePrefix, data.Username) var newVersion int64 err = m.TransactCtx(ctx, ...) _ = m.DelCacheCtx(ctx, sysUserIdKey, sysUserUsernameKey) return newVersion, nil }

每次 RefreshToken(HTTP + gRPC 两条路径)都多一次 FindOne 走 cache-aside:

  • Cache 命中:1 次 Redis GET(快);
  • Cache 未命中:1 次 Redis GET + 1 次 DB SELECT + 1 次 Redis SETEX = 3 次 IO。 RefreshToken 已经调了 UserDetailsLoader.Load(含 loadUser 里的 FindOne)——同一请求内第二次 FindOne,完全是浪费;更关键的是:RefreshToken 的 cache flow 是 "Load → 比较 TokenVersion → IncrementIfMatch → Clean",Clean 会把 loader 缓存清掉但 go-zero 的 FindOne 那一层 cache 不会被清(不同 key 空间),所以下一次请求还是会再查一次。

  • 建议

    1. IncrementTokenVersionIfMatch 的第一次 FindOne 去掉,cache key 的 username 部分改成读 ExecCtx 的"写入后新值"(go-zero 支持把 DelCacheCtx 放进 ExecCtx 的 hook 里,只传 sysUserIdKeysysUserUsernameKey 可以在事务内再 session.QueryRowCtxusername,或者干脆只删 ID key——FindOneByUsername 的 cache 也只会因为密码 / status 等变化才会 stale,tokenVersion 变更并不会让"按 username 查"的结果在业务上过期);
    2. 或者让上游 refreshTokenLogic.go 在调用 IncrementTokenVersionIfMatch 时把 ud.Username 也传进来,省掉这次 FindOne:

      IncrementTokenVersionIfMatch(ctx, userId, username, expected) (int64, error)
      

      logic 层已经手上有 ud.Username,直接透传即可。


      L-1. SyncPermsService 的"如需禁用所有权限请使用专用接口"是文案指向幽灵端点

      • 位置internal/logic/pub/syncPermsService.go:49-51
      • 描述go if len(perms) == 0 { return nil, &SyncPermsError{Code: 400, Message: "权限列表不能为空,如需禁用所有权限请使用专用接口"} }

然而 perm.api / routes.go / gRPC PermService并没有"禁用所有权限的专用接口"。这行错误消息是历史设计遗留——要么这个接口被砍了,要么从来没实现过。接入方看到 400 文案会去找"专用接口",浪费排查时间。

  • 建议:把文案改成客观事实"权限列表不能为空",并把"禁用所有权限"这条产品需求显式 TODO 或删除(真要支持,需要一个独立授权模型,不能走 appKey 就把产品的所有权限抹了——这本身是个有争议的能力)。

L-2. DeleteDeptLogic 的多次 FOR UPDATE 顺序可能死锁(真实可能性较低)

  • 位置internal/logic/dept/deleteDeptLogic.go:36-62
  • 描述: 同一事务内依次:SELECT dept_id FROM sys_dept FOR UPDATESELECT child ids FROM sys_dept WHERE parentId FOR UPDATESELECT user ids FROM sys_user WHERE deptId FOR UPDATEDELETE sys_dept WHERE id。顺序"sys_dept 行 → sys_dept 按 parentId 的范围锁 → sys_user 按 deptId 的范围锁"。

如果另一 tx 在做 CreateDept(L3 的"InsertWithTx → UpdateWithTx"路径)或 UpdateUserLogic(先读 sys_userUpdateProfile),两边获取锁的顺序不一致时理论上能构成 AB-BA。实际业务里:

  • DeleteDept 仅超管调用,频率极低(删除部门是稀有操作);
  • CreateDept 同样低频;
  • 真正会撞的是 "DeleteDept 碰巧与大批量 UpdateUser deptId" 同时——这种场景通常不会主动并发; 但审计视角上仍是一个"范围锁 → 可阻塞后续"的定时炸弹。
  • 建议
    1. 确保锁顺序在所有涉及 sys_dept + sys_user 的事务里一致(总是先锁 dept,再锁 user);
    2. DeleteDept 内可以把 sys_user WHERE deptId = ? FOR UPDATE 换成不加锁的 SELECT ... LOCK IN SHARE MODE + 事务隔离级 REPEATABLE READ(反正只是存在性判定),降低阻塞面。

L-3. UserDetailsLoader.registerCacheKey 每次都做 4 次 Redis 单独 RTT(SAdd + Expire + SAdd + Expire)

  • 位置internal/loaders/userDetailsLoader.go:220-238
  • 描述:每次 Load cache miss 后都会:

    SADD ud:idx:u:<userId>   <key>
    EXPIRE ud:idx:u:<userId> <ttl+60>
    SADD ud:idx:p:<productCode> <key>
    EXPIRE ud:idx:p:<productCode> <ttl+60>
    

    4 次独立 RTT。一个大产品启动瞬间(N 个并发用户都走 middleware 的首次 Load),4*N 次 RTT 打满 Redis cluster 的连接队列。

    • 建议:用 go-zero redis 的 PipelinedTxPipelined 批做,合并为 1 RTT;更激进可以把索引维护推到异步通道,Load 主路径不 wait。

    L-4. Logout 仍用无条件 IncrementTokenVersion(非 CAS)

    • 位置internal/logic/auth/logoutLogic.go:46
    • 描述RefreshToken 已经切到 IncrementTokenVersionIfMatch,但 Logout 还是老版本 IncrementTokenVersion(无条件 SET tokenVersion = tokenVersion+1)。业务语义上 Logout 确实是"无论当前版本多少都失效",所以目前正确——但这也意味着:
    • Logout 和并发 RefreshToken 相撞时,Logout 可能在 RefreshTokenIncrementTokenVersionIfMatch 之后再 +1,导致刚签发的新 token 立即失效(体验问题,非安全问题);
    • IncrementTokenVersion 实际只剩 Logout 一个调用点 + model 层的测试引用,继续保留这个"大杀器" API 会让未来新功能误用(比如某人在新增逻辑时图方便调了无条件 +1 版本)。
    • 建议
    • 不强制替换 Logout(语义正确),但IncrementTokenVersion 加上显式的安全注释:"仅限业务语义为'强制全量失效'的场景(Logout / 封禁账号),禁止在 Refresh/Rotation 场景使用——Refresh 必须走 IncrementTokenVersionIfMatch"。
    • 更激进:用 golang build tag / linter 限制 IncrementTokenVersion 的调用方范围(仅限 auth/logoutLogic.go + 未来的封禁接口)。

    L-5. removeMemberLogic:移除 ADMIN 前的 CountActiveAdminsTx 与目标成员自己的 state 判定耦合

    • 位置internal/logic/member/removeMemberLogic.go:45-54
    • 描述go if locked.MemberType == consts.MemberTypeAdmin && locked.Status == consts.StatusEnabled { adminCount, err := l.svcCtx.SysProductMemberModel.CountActiveAdminsTx(ctx, session, member.ProductCode) ... if adminCount <= 1 { return "不能移除最后一个管理员" } }

CountActiveAdminsTxSELECT COUNT(*) WHERE productCode=? AND memberType='ADMIN' AND status='enabled'——包括了正要被删除的 locked 自己。因此 adminCount <= 1 刚好拦住了"删最后一个",但边界判定非常 subtle:

  • locked 本身虽是 ADMIN 但 status=disabled(禁用但未删),流程会跳过 admin-count 检查直接删除——OK,因为他反正不是 active admin;
  • 但如果 locked.MemberType=ADMIN && locked.Status=Enabled 且 DB 里另有一个 "ADMIN + Disabled" 的 stale 记录,count 不包含它——正确;
  • 隐含约束:CountActiveAdminsTx 必须和 locked 的"active admin 定义"完全一致(MemberType=ADMIN 且 Status=Enabled)。如果未来新增一种 ADMIN 子类型(比如 SuperProductAdmin),这个 count 会漏算。
  • 建议:把 CountActiveAdminsTx 改成返回"除了这个 id 之外还有几个 active admin":

    SELECT COUNT(*) FROM sys_product_member WHERE productCode = ? AND memberType = 'ADMIN' AND status = 1 AND id != ?
    

    外层判定 if adminCount == 0 → 最后一个,语义更贴合业务,少一步反向推理。UpdateMemberLogic 的同名调用同样受益。


    L-6. CheckManageAccess 在 caller DeptId == 0 时直接 403,漏了"非 ADMIN 的超级管理员"的心智模型

    • 位置internal/logic/auth/access.go:155-162
    • 描述go func checkDeptHierarchy(ctx, svcCtx, caller, targetUserId) error { if caller.MemberType == consts.MemberTypeAdmin { return nil } if caller.DeptId == 0 { return ErrForbidden("您未归属任何部门,无权管理其他用户") } if caller.DeptPath == "" { return ErrForbidden("您的部门信息异常...") } ... }

CheckManageAccess 最外层已经对 caller.IsSuperAdmin 做了 early return(access.go:47),所以 checkDeptHierarchy 进到这里一定不是超管——OK。 但目前代码里"caller DeptId=0"这件事并不一定异常:L1 中 H-4 的修复落地后,普通 MEMBER 的 DeptId 绝不会是 0;但早期数据 / 迁移遗留数据里仍可能有 DeptId=0 的 MEMBER(例如从老系统搬来的账号)。这些账号会发现任何管理操作都拒绝,包括合法的"管理自己所在产品里的下属"——实际上 checkPermLevel 本来可以单独判定,但被 checkDeptHierarchy 先拦了。

  • 建议:在无部门归属的 MEMBER 场景下,至少应允许"管理自己"和"纯 product-admin-downward"的操作;或者运维侧跑一次 data fix 把 DeptId=0 的 MEMBER 都归入"默认部门"。代码侧加一条 TODO 记录这个假设,便于未来发现幽灵账号时做定向修复。

结论与修复优先级

优先级 finding 概要
P0 H-1 AdminLogin 限流仅用 username → 可远端批量锁死所有产品 admin 账号(配合 M-2 的 ProductList 枚举放大)
P0 H-2 ValidateProductLogin 在冻结账号路径跳过 bcrypt → 响应时间 + 明文错误双重账号状态 oracle
P0 H-3 SyncPermsService 的 FOR UPDATE 修复只做到基础设施层,service 层仍然 tx 外读 / tx 内写,1062 → 409 只是缓解不是根治
P1 M-1 UpdateDept 部门用户缓存仍串行 Clean(R5 M-2 未落地)
P1 M-2 ProductList / ProductDetail / DeptTree 对任意登录用户无访问控制,组合放大 H-1
P1 M-3 UserDetail 向任意同产品成员泄露 Phone / Email(PII 超额披露)
P1 M-4 BindRolePerms / UpdateRole 在 post-commit 缓存步骤失败返回 500 → 客户端误判失败
P2 M-5 / M-6 / M-7 请求内重复 FindOne / ExtractClientIP 代理链解析不足 / JWT 未显式验签方法
P2 M-8 IncrementTokenVersionIfMatch 为拿 username 多 1 次 FindOne
P3 L-1 ~ L-6 幽灵端点文案 / 锁序风险 / Redis 往返 pipeline / 无 CAS 注释 / CountActiveAdmins 语义 / DeptId=0 心智模型

建议的修复次序

  1. P0 同批上线:H-1 + H-2(登录侧信道 + DoS 一起打补丁),H-3(SyncPerms 的 FOR UPDATE 接上 LockByCodeTx + 改掉 mock)。这三条是本轮最关键的质变修复,前两条决定账号安全边界,第三条决定同步的可靠性。
  2. P1 第二批
    • M-1:UserDetailsLoader.CleanByUserIds + UpdateDept 批处理;
    • M-2:ProductList / ProductDetail / DeptTree 的访问控制(非超管自动剪枝);
    • M-3:UserDetail 的 PII filter;
    • M-4:post-commit 缓存清理失败不再映射 500。
  3. P2 配套
    • M-5:CheckManageAccess 支持 prefetched target;
    • M-6:ExtractClientIP 支持 X-Forwarded-For + IP 合法性校验;
    • M-7:统一的 parseHS256 helper 显式校验签名算法;
    • M-8:IncrementTokenVersionIfMatch 接收 username 参数。
  4. P3 收尾 / 日常清理:L-1 ~ L-6。

与第 5 轮的关系

  • 第 5 轮 实际落地并复盘通过:H-1(CAS)、H-2(gRPC 限流)、H-3(同级放行修 <=)、H-4(deptId=0 守门)、M-3(负缓存)、M-5(错误消息字符串匹配移除)、M-7(gRPC IP 剥 host:port)、M-8(ChangePassword 限流)、L-1(MustChangePasswordYes)、L-4(FindMinPermsLevelByUserIdAndProductCode 区分 NotFound)。
  • 第 5 轮 未完全落地 / 遗留:M-2(UpdateDept Clean 批处理)→ 本轮 M-1;M-6(SyncPerms FOR UPDATE)→ 本轮 H-3(因基础设施完成提高到 P0)。
  • 第 5 轮 未覆盖:登录侧信道(H-2)、admin DoS(H-1)、水平信息披露(M-2 / M-3)、post-commit 缓存失败的 500 误映射(M-4)、客户端 IP 提取不全(M-6)、JWT token.Method 断言(M-7)——本轮全部新增。

说明:本轮 findings 均在当前 HEAD 代码中复现无误;H 级别问题均给出可触发的真实业务 / 攻击路径,而非纯理论风险。