|
@@ -1,530 +1,630 @@
|
|
|
-# 权限管理系统 - 深度代码审计报告
|
|
|
|
|
|
|
+# 权限管理系统 - 深度代码审计报告(第 3 轮)
|
|
|
|
|
|
|
|
-> 审计范围:`/internal` 下全部非测试生产代码(logic、model、middleware、handler、loaders、server、svc、consts、response、util)及入口文件 `perm.go`。
|
|
|
|
|
-> 审计时间:2026-04-18
|
|
|
|
|
-> 审计重点:业务逻辑闭环、跨接口一致性、权限绕过、缓存一致性、并发竞态、资源与性能、僵尸代码、接口契约完整性。
|
|
|
|
|
-> 相对上一轮:H-1(BindRoles 误拦截 ADMIN)、H-2(GetUserPerms 未校验状态)、H-3(DEV 部门绕过)、M-2(批量 DELETE)、M-3/M-4(roleIds 语义)、M-5(UpdateDept 级联)、M-6(Claims.Perms)、M-11(DeleteDept TOCTOU)、L-3(UpdateDept 乐观锁)均已修复。本报告聚焦残留问题与本轮新发现。
|
|
|
|
|
|
|
+> 审计范围:`/internal` 下全部非测试生产代码(logic、model、middleware、handler、loaders、server、svc、consts、response、util)及入口文件 `perm.go`、`perm.api`。
|
|
|
|
|
+> 审计时间:2026-04-19
|
|
|
|
|
+> 审计重点:自我越权、并发竞态、接口 DoS、事务内外读写分裂、缓存一致性、接口契约完整性。
|
|
|
|
|
+> 相对上一轮修复情况:
|
|
|
|
|
+> - **已修复**:H-1(产品禁用联动)、H-2(JwtAuth MemberType 校验)、H-3(ManagementKey 前置)、H-4(最后一个 ADMIN 保护)、M-1(/auth/logout 路由)、M-2(refreshToken 轮转递增 tokenVersion)、M-9(BindRolePerms 事务外用户集查询已改为事务后)、M-11(`FindByPathPrefix`/`FindByParentId` 已清理)、M-12(UpdateUserStatus 重复校验已清理)、M-14(setUserPerms 校验产品启用)、M-15(BindRoles 无 diff 也清缓存)、M-16(用户 perm 查询已加 `p.status=1`)、L-3(非超管禁止下调 permsLevel)、L-4(CreateRole 校验产品启用)、L-5(AddMember 校验产品启用)、L-6(UserDetail 分支语义已明确)。
|
|
|
|
|
+> - **未修复且仍存在**:M-3(`generateRandomHex` 截断 bug)、M-4(`DeptTree` 权限过滤)、M-5(`ProductList`/`ProductDetail` 权限过滤)、M-7(`X-Real-IP` 信任 + 无 XFF)、M-8(缓存失效非原子)、L-1/L-2/L-7~L-10。
|
|
|
|
|
+> - **新增 P0/P1 问题**:H-A(`SetUserPerms` 自我权限越权)、H-B(`IncrementTokenVersion` 读缓存导致返回值错位)、M-A("最后 ADMIN" 校验存在 TOCTOU)、M-B(`/auth/refreshToken` 无限流,DB 热点写 DoS)、M-C(产品登录用户枚举 + 选择性锁定)、M-D(`DeleteRole` 事务内但用非事务连接读用户集)、M-E(多个 `Delete*ForProductTx` 的 "先 SELECT 再 DELETE" 非原子)、M-F(`CountActiveAdmins` 硬编码 'ADMIN' 字面量)。
|
|
|
|
|
|
|
|
---
|
|
---
|
|
|
|
|
|
|
|
## 🚩 核心逻辑漏洞 (High Risk)
|
|
## 🚩 核心逻辑漏洞 (High Risk)
|
|
|
|
|
|
|
|
-### H-1. 禁用产品后,已持有 token 的成员仍可正常使用该产品
|
|
|
|
|
|
|
+### H-A. `SetUserPerms` 对"自己调用自己"直接放行,普通 MEMBER 可自我授予产品内任意权限(自我越权)
|
|
|
|
|
|
|
|
- **位置**:
|
|
- **位置**:
|
|
|
- - `internal/logic/product/updateProductLogic.go:30-63`
|
|
|
|
|
- - `internal/middleware/jwtauthMiddleware.go:45-89`
|
|
|
|
|
- - `internal/loaders/userDetailsLoader.go:280-290`(`loadProduct`)、`348-364`(`loadPerms`)
|
|
|
|
|
- - `internal/server/permserver.go:157-222`(`VerifyToken` / `GetUserPerms`)
|
|
|
|
|
|
|
+ - `internal/logic/user/setUserPermsLogic.go:50`
|
|
|
|
|
+ - `internal/logic/auth/access.go:47`(`CheckManageAccess` 内的 `if caller.UserId == targetUserId { return nil }`)
|
|
|
- **描述**:
|
|
- **描述**:
|
|
|
- `UpdateProduct` 在将 `sys_product.status` 置为 `Disabled` 之后,只做了 `UserDetailsLoader.CleanByProduct(product.Code)`。但:
|
|
|
|
|
-
|
|
|
|
|
- 1. `loadProduct` 从 DB 只取 `ProductName`,**没有把 `product.Status` 写入 `UserDetails`**。
|
|
|
|
|
- 2. `loadPerms` 的"全量权限"短路条件里完全没有引用产品状态,因此哪怕产品被禁用,`IsSuperAdmin / ADMIN / DEVELOPER / DEV 部门` 四类用户重新 `Load` 后仍会拿到完整 `perms`。
|
|
|
|
|
- 3. `jwtauthMiddleware.Handle` 只校验 `ud.Username / ud.Status / claims.TokenVersion`,**没有校验产品状态**。
|
|
|
|
|
- 4. 对外的 `gRPC VerifyToken` 和 `gRPC GetUserPerms` 也没有校验产品状态。
|
|
|
|
|
- 5. 产品登录入口 `ValidateProductLogin` 是唯一校验了 `product.Status != Enabled` 的点;但这仅影响**新登录**,对已经签发的 access / refresh token 无任何影响。
|
|
|
|
|
-
|
|
|
|
|
- 也就是说:管理员把一个产品"冻结"之后,该产品的所有在线用户在整个 `AccessExpire`(甚至通过 `RefreshToken` 可以一直续期到 `RefreshExpire`)窗口内都能继续访问产品端的所有接口,并且接入方通过 gRPC `GetUserPerms` / `VerifyToken` 拿到的权限和"有效"状态也依然是放行的。
|
|
|
|
|
-
|
|
|
|
|
-- **影响**:
|
|
|
|
|
- - "禁用产品"这一核心管控动作**近乎无效**:离线下线、应急止损、合规处置场景下,管理员无法阻断业务。
|
|
|
|
|
- - 攻击面:当某个产品因为安全事件需要临时下线时(例如 appSecret 泄露、业务侧数据异常),除物理删除该产品之外,没有办法收回其成员的访问能力。
|
|
|
|
|
- - 对接入方(产品服务端)的一致性漏洞:管理系统显示"产品禁用"、但对外 RPC 依然告知"这个用户有全部权限"。
|
|
|
|
|
-
|
|
|
|
|
-- **修复方案**:
|
|
|
|
|
- - `userDetailsLoader` 的 `UserDetails` 增加 `ProductStatus int64` 字段,`loadProduct` 赋值。
|
|
|
|
|
- - `loadPerms` 在所有"自动给全量权限"的分支上叠加 `ud.ProductStatus == StatusEnabled` 前置;或者在 `loadPerms` 入口直接:
|
|
|
|
|
-
|
|
|
|
|
|
|
+ - `SetUserPerms` 使用 `CheckManageAccess(ctx, svcCtx, req.UserId, productCode)` 作为唯一的访问控制。
|
|
|
|
|
+ - `CheckManageAccess` 中"自己操作自己"是无条件放行的短路:
|
|
|
```go
|
|
```go
|
|
|
- if ud.ProductCode != "" && ud.ProductStatus != consts.StatusEnabled {
|
|
|
|
|
- ud.Perms = nil
|
|
|
|
|
- return
|
|
|
|
|
|
|
+ if caller.UserId == targetUserId {
|
|
|
|
|
+ return nil
|
|
|
}
|
|
}
|
|
|
```
|
|
```
|
|
|
|
|
+ - 随后逻辑只校验:
|
|
|
|
|
+ 1. 目标是当前产品的有效成员(调用者自己当然是);
|
|
|
|
|
+ 2. 传入的 permIds 都属于当前产品、且 `status=1`;
|
|
|
|
|
+ 3. DELETE 现有 `sys_user_perm`(userId+productCode),然后 `BatchInsert` 新的 (permId, effect) 对。
|
|
|
|
|
+ - 没有校验"调用者本身是否有权授予该 perm"。
|
|
|
|
|
+
|
|
|
|
|
+ 攻击路径(任意 MEMBER 都可完成):
|
|
|
|
|
+
|
|
|
|
|
+ ```http
|
|
|
|
|
+ POST /api/user/setPerms
|
|
|
|
|
+ Authorization: Bearer <自己的 access token>
|
|
|
|
|
+ {
|
|
|
|
|
+ "userId": <自己的 userId>,
|
|
|
|
|
+ "perms": [
|
|
|
|
|
+ {"permId": 1, "effect": "ALLOW"},
|
|
|
|
|
+ {"permId": 2, "effect": "ALLOW"},
|
|
|
|
|
+ ...所有 permId...
|
|
|
|
|
+ ]
|
|
|
|
|
+ }
|
|
|
|
|
+ ```
|
|
|
|
|
|
|
|
- - `jwtauthMiddleware.Handle` 与 `RefreshToken` / `VerifyToken` / `GetUserPerms` 在 `claims.ProductCode != ""` 时统一校验 `ud.ProductStatus == StatusEnabled`,非启用直接 `403 "该产品已被禁用"`。
|
|
|
|
|
- - `UpdateProduct` 在置 `Disabled` 时,**同步把该产品所有成员的 `tokenVersion+1`**(或引入一个 `product_token_epoch`),从而让所有既有 token 立即作废。推荐后者:新增 `sys_product.tokenEpoch`,access token 里带 `productEpoch`,中间件对比。
|
|
|
|
|
-
|
|
|
|
|
----
|
|
|
|
|
-
|
|
|
|
|
-### H-2. JWT 中间件未校验"产品成员是否被禁用",造成已被剔除/禁用成员仍可访问业务接口
|
|
|
|
|
|
|
+ 下一次 `loadPerms` 会走"普通成员"分支:`rolePerms ∪ userAllow − userDeny`。用户自己塞进去的 `userAllow` 全部生效(第 427-431 行),直接获得整个产品的全部 `perms` 集合。中间件侧的 `ud.Perms` 也会包含这些 code;下游产品服务通过 `GetUserPerms` gRPC 拿到的权限同样被污染。
|
|
|
|
|
|
|
|
-- **位置**:`internal/middleware/jwtauthMiddleware.go:73-87`
|
|
|
|
|
-- **描述**:`RefreshToken`(`refreshTokenLogic.go:53`、`permserver.go:125`)、`VerifyToken`(`permserver.go:174`)、`GetUserPerms`(`permserver.go:214`)都会在 `productCode != "" && !IsSuperAdmin` 时校验 `ud.MemberType != ""`——这正是 `loadMembership` 在成员不存在或 `status != Enabled` 时的返回值。
|
|
|
|
|
|
|
+ 该越权对 ADMIN/DEVELOPER/SUPER_ADMIN 无新增危害(这三类本来就有全产品权限),但对 **MEMBER 类型是完全的权限集逃逸**。
|
|
|
|
|
|
|
|
- 但是 **HTTP 主流量入口 `JwtAuth` 中间件**却没有这个校验,只看用户自身 `status`。结果:
|
|
|
|
|
|
|
+- **影响**:
|
|
|
|
|
+ - 产品端的"最小权限"模型**彻底失效**:任何通过 `/auth/login` 登录的普通成员都能自我提权到"全权限"(等价于 ADMIN 级 perms,但不改 `memberType`、不触发 `checkDeptHierarchy` 等 ADMIN 后续护栏——也就是说**绕过权限级别校验、绕过部门护栏**,仅对自身 perms 集合生效)。
|
|
|
|
|
+ - 与 `BindRoles` 形成对比:`BindRoles` 的"自己操作自己"在 `caller.MemberType == MEMBER` 时仍会被 `r.PermsLevel < caller.MinPermsLevel` 拦截(`bindRolesLogic.go:82-88`),避免了自我绑高等级角色;但 `SetUserPerms` **没有任何对应的自我越权护栏**。
|
|
|
|
|
+ - 由于 `ud.Perms` 会被注入到 access token 的 `claims` 之外的 DB 查询结果中(走的是 loader,不是 JWT 内联 perms),攻击者不需要再刷 token,下一次请求就生效。
|
|
|
|
|
|
|
|
- - 管理员执行 `UpdateMember.Status=Disabled` 后,`loader.Del` 清理缓存,但旧 access token 未作废(`tokenVersion` 未变)。
|
|
|
|
|
- - 被禁用成员带着这张 token 继续请求 `/api/dept/tree`、`/api/perm/list`、`/api/user/detail?id=self` 等"只 JwtAuth 不做业务校验"的接口,**全部放行**。
|
|
|
|
|
- - 更严重的是 `loadPerms`:`MemberType == ""` 时会跳过"全量权限"分支,但普通成员分支仍会基于 `sys_user_role` / `sys_user_perm` 返回权限集。也就是被踢出的成员在中间件层不被拒,随后其 `ud.Perms` 还被填充(如果前端仍按 `/api/perm/list` 来做菜单,依然能看到"这个产品下自己之前拥有的权限")。
|
|
|
|
|
|
|
+- **修复方案**(二选一或叠加):
|
|
|
|
|
|
|
|
- `loadPerms` 的 DEV 部门短路已经加了 `ud.MemberType != ""` 这层护栏(第 358 行),但角色/用户权限并没有这层保护,导致普通成员分支依然会生效。
|
|
|
|
|
|
|
+ **方案 A(最小侵入,直接禁止自我 set):**
|
|
|
|
|
|
|
|
-- **影响**:
|
|
|
|
|
- - "禁用成员"的实际效果仅对**登录时刻**生效;对**在线成员**只有间接约束(token 过期后才生效)。
|
|
|
|
|
- - 配合 H-1(禁用产品),这条路径让"访问控制回收"能力在产品/成员两个维度都失灵。
|
|
|
|
|
- - 一致性问题:`RefreshToken` 拒绝了已禁用成员,但 HTTP 业务接口不拒,用户可能看到"业务继续可用 / 但 refresh 已失败"的分裂状态。
|
|
|
|
|
|
|
+ ```go
|
|
|
|
|
+ // internal/logic/user/setUserPermsLogic.go
|
|
|
|
|
+ caller := middleware.GetUserDetails(l.ctx)
|
|
|
|
|
+ if caller != nil && caller.UserId == req.UserId && !caller.IsSuperAdmin {
|
|
|
|
|
+ return response.ErrForbidden("不能为自己设置权限")
|
|
|
|
|
+ }
|
|
|
|
|
+ ```
|
|
|
|
|
|
|
|
-- **修复方案**:在 `jwtauthMiddleware.Handle` 的 403 校验中对齐 `RefreshToken`:
|
|
|
|
|
|
|
+ **方案 B(对齐"高危写操作需 ADMIN"语义,推荐):**
|
|
|
|
|
|
|
|
```go
|
|
```go
|
|
|
- if claims.ProductCode != "" && !ud.IsSuperAdmin && ud.MemberType == "" {
|
|
|
|
|
- httpx.ErrorCtx(r.Context(), w, response.NewCodeError(403, "您已不是该产品的有效成员"))
|
|
|
|
|
- return
|
|
|
|
|
|
|
+ // 全部调用者限定为超管或该产品 ADMIN
|
|
|
|
|
+ if err := authHelper.RequireProductAdminFor(l.ctx, productCode); err != nil {
|
|
|
|
|
+ return err
|
|
|
}
|
|
}
|
|
|
```
|
|
```
|
|
|
|
|
|
|
|
- 同时 `loadPerms` 的普通成员分支也应当在入口加:
|
|
|
|
|
|
|
+ 方案 B 更贴近 API 注释"个性化权限"的管理语义(默认只有管理员能做精细化调权)。保留 `CheckManageAccess` 的层级校验作为纵深:
|
|
|
|
|
|
|
|
```go
|
|
```go
|
|
|
- if ud.ProductCode != "" && !ud.IsSuperAdmin && ud.MemberType == "" {
|
|
|
|
|
- return // 非有效成员,权限置空
|
|
|
|
|
|
|
+ if err := authHelper.RequireProductAdminFor(l.ctx, productCode); err != nil {
|
|
|
|
|
+ return err
|
|
|
|
|
+ }
|
|
|
|
|
+ if err := authHelper.CheckManageAccess(l.ctx, l.svcCtx, req.UserId, productCode); err != nil {
|
|
|
|
|
+ return err
|
|
|
}
|
|
}
|
|
|
```
|
|
```
|
|
|
|
|
|
|
|
- (这条在第 353-358 行的"自动全量"里已有,但需要抽出作用到整函数)
|
|
|
|
|
|
|
+ - 同步排查 `BindRoles`:对 MEMBER 目前通过 permsLevel 拦截了"自我绑低等级角色",这条逻辑是护栏;但 `caller.MemberType == DEVELOPER` 自己绑自己任意角色的路径**仍能过**(DEVELOPER 本身就全权,无新增危害,保留即可)。保留现状是合理的。
|
|
|
|
|
+
|
|
|
|
|
+- **优先级**:P0(立即修复)
|
|
|
|
|
|
|
|
---
|
|
---
|
|
|
|
|
|
|
|
-### H-3. AdminLogin:`UsernameLoginLimit` 在 `ManagementKey` 校验之前计数,允许无凭据 DoS 超管账号
|
|
|
|
|
|
|
+### H-B. `SysUserModel.IncrementTokenVersion` 返回基于**缓存读**的新版本号,并发 refresh/logout 会签发"DB 已作废"的新 token
|
|
|
|
|
|
|
|
-- **位置**:`internal/logic/pub/adminLoginLogic.go:35-45`
|
|
|
|
|
|
|
+- **位置**:`internal/model/user/sysUserModel.go:140-156`
|
|
|
- **描述**:
|
|
- **描述**:
|
|
|
```go
|
|
```go
|
|
|
- if l.svcCtx.UsernameLoginLimit != nil {
|
|
|
|
|
- code, _ := l.svcCtx.UsernameLoginLimit.Take(req.Username)
|
|
|
|
|
- if code == limit.OverQuota {
|
|
|
|
|
- return nil, response.NewCodeError(429, "该账号登录尝试过于频繁,请5分钟后再试")
|
|
|
|
|
|
|
+ func (m *customSysUserModel) IncrementTokenVersion(ctx context.Context, id int64) (int64, error) {
|
|
|
|
|
+ data, err := m.FindOne(ctx, id) // ← 从缓存读
|
|
|
|
|
+ ...
|
|
|
|
|
+ _, err = m.ExecCtx(ctx, func(ctx context.Context, conn sqlx.SqlConn) (sql.Result, error) {
|
|
|
|
|
+ query := fmt.Sprintf("UPDATE %s SET `tokenVersion` = `tokenVersion` + 1, `updateTime` = ? WHERE `id` = ?", m.table)
|
|
|
|
|
+ return conn.ExecCtx(ctx, query, time.Now().Unix(), id)
|
|
|
|
|
+ }, sysUserIdKey, sysUserUsernameKey)
|
|
|
|
|
+ if err != nil {
|
|
|
|
|
+ return 0, err
|
|
|
}
|
|
}
|
|
|
- }
|
|
|
|
|
- if subtle.ConstantTimeCompare([]byte(req.ManagementKey), []byte(l.svcCtx.Config.Auth.ManagementKey)) != 1 {
|
|
|
|
|
- return nil, response.ErrUnauthorized("managementKey无效")
|
|
|
|
|
|
|
+ return data.TokenVersion + 1, nil // ← 基于旧缓存 + 1
|
|
|
}
|
|
}
|
|
|
```
|
|
```
|
|
|
|
|
|
|
|
- `UsernameLoginLimit` 的 key 维度是**纯 username**(`svcCtx.UsernameLoginLimit = limit.NewPeriodLimit(300, 10, rds, ":rl:user")`,没有叠加 IP),5 分钟内全局 10 次。由于 `Take` 发生在 `ManagementKey` 校验之前,攻击者**不需要任何凭据**就能消耗配额。
|
|
|
|
|
|
|
+ 两个严重问题:
|
|
|
|
|
+
|
|
|
|
|
+ 1. **缓存陈旧**:`data.TokenVersion` 来自 `FindOne` 的缓存层(`sqlc.CachedConn.FindOne`)。如果在另一条写链路(`UpdatePassword` / `UpdateStatus` / 另一次 `IncrementTokenVersion`)**DB 已 +1 但 Redis 缓存尚未失效**(M-8 窗口),缓存里读到的 `TokenVersion` 仍是旧值 X;`UPDATE tokenVersion+1` 把 DB 从 X+1 推到 X+2;函数却返回 X+1。随后签发的 access token 里 `tokenVersion=X+1` 与 DB 的 X+2 **不匹配**,**下一次请求直接被 `jwtauthMiddleware` 以 "登录状态已失效" 拒掉**。
|
|
|
|
|
+ 2. **并发自身**:即使不存在外部写入,两条并发 `RefreshToken` 也会命中同一现象——两条流程同时读缓存得 X,`UPDATE x+1` 让 DB 变成 X+2;两条流程都返回 X+1 签发了 access+refresh token 对,两张 refresh token 下一次使用都会因为 `TokenVersion != ud.TokenVersion` 被拒。
|
|
|
|
|
|
|
|
- 攻击场景:
|
|
|
|
|
- 1. 攻击者对 `/auth/adminLogin` 以任意 `managementKey="x"` 但 `username=admin` 打 10 次(单 IP 一分钟内即可完成,AdminLoginRateLimit 是 IP 60s/20)。
|
|
|
|
|
- 2. 真正的超管此后 5 分钟内无法登录管理后台,返回 `429`。
|
|
|
|
|
- 3. 攻击者持续周期性地(每 5 分钟一波)重放,即可**永久锁死**已知超管用户名的管理后台入口。
|
|
|
|
|
|
|
+ 调用方均依赖**返回值**作为新签 token 的 version:
|
|
|
|
|
+ - `internal/logic/pub/refreshTokenLogic.go:66-75`(生成新 access/refresh)
|
|
|
|
|
+ - `internal/server/permserver.go:141-148`(gRPC RefreshToken)
|
|
|
|
|
+
|
|
|
|
|
+ 这不只是理论问题——`RefreshToken` 现在每次都轮转 `tokenVersion`(上一轮 M-2 的修复要求),配合前端"多 tab / SW 并发 / 失败重试 / 双请求"等真实场景,用户会规律性遭遇"刷新一次就全家桶失效、需要完全重新登录"。
|
|
|
|
|
|
|
|
- **影响**:
|
|
- **影响**:
|
|
|
- - 任何已知超管用户名(如 `admin`、`admin_{code}`)可被**无凭据**持续 DoS 锁登录入口;应急响应、事件处置期间超管无法登录管理后台。
|
|
|
|
|
- - 攻击成本极低,IP 级限流(20/min)足以完成锁定。
|
|
|
|
|
|
|
+ - 正常用户会看到**偶发的"刚刷新完 token 就被登出"**(灰度回溯难度高,属于典型的 TOCTOU 竞态)。
|
|
|
|
|
+ - `Logout` 本身对此问题免疫(不读 `data.TokenVersion` 作为返回值),但 `RefreshToken` 严重受影响。
|
|
|
|
|
+ - 间接让 "被盗 refresh token 立即失效" 的安全语义在正常用户身上误伤自己。
|
|
|
|
|
|
|
|
- **修复方案**:
|
|
- **修复方案**:
|
|
|
- - 把 `UsernameLoginLimit.Take` 移动到 `ManagementKey` 校验**之后**、用户名/密码校验之前:
|
|
|
|
|
|
|
|
|
|
- ```go
|
|
|
|
|
- if subtle.ConstantTimeCompare([]byte(req.ManagementKey), []byte(l.svcCtx.Config.Auth.ManagementKey)) != 1 {
|
|
|
|
|
- return nil, response.ErrUnauthorized("managementKey无效")
|
|
|
|
|
- }
|
|
|
|
|
- if l.svcCtx.UsernameLoginLimit != nil {
|
|
|
|
|
- code, _ := l.svcCtx.UsernameLoginLimit.Take(req.Username)
|
|
|
|
|
- ...
|
|
|
|
|
- }
|
|
|
|
|
- ```
|
|
|
|
|
|
|
+ **方案 A(推荐,MySQL 原子自增 + 读取):**
|
|
|
|
|
|
|
|
- - 同时把限流 key 改为 `ip+username` 混合维度(推荐独立新桶),避免单个攻击者永久封锁真实用户。
|
|
|
|
|
- - `ValidateProductLogin`(`loginService.go:33-38`)存在同样的账号锁定 DoS(无凭据即可锁任意用户名),推荐也改用 `ip+username` 混合 key 或增加可绕过的 CAPTCHA 机制。
|
|
|
|
|
|
|
+ ```go
|
|
|
|
|
+ func (m *customSysUserModel) IncrementTokenVersion(ctx context.Context, id int64) (int64, error) {
|
|
|
|
|
+ var newVersion int64
|
|
|
|
|
+ sysUserIdKey := fmt.Sprintf("%s%v", cacheSysUserIdPrefix, id)
|
|
|
|
|
+ // 先查出 username(仅用于缓存 key)
|
|
|
|
|
+ data, err := m.FindOne(ctx, id)
|
|
|
|
|
+ if err != nil {
|
|
|
|
|
+ return 0, err
|
|
|
|
|
+ }
|
|
|
|
|
+ sysUserUsernameKey := fmt.Sprintf("%s%v", cacheSysUserUsernamePrefix, data.Username)
|
|
|
|
|
+ err = m.TransactCtx(ctx, func(ctx context.Context, session sqlx.Session) error {
|
|
|
|
|
+ // LAST_INSERT_ID trick:UPDATE 的同时把新值写入 LAST_INSERT_ID()
|
|
|
|
|
+ upd := fmt.Sprintf("UPDATE %s SET `tokenVersion` = LAST_INSERT_ID(`tokenVersion` + 1), `updateTime` = ? WHERE `id` = ?", m.table)
|
|
|
|
|
+ if _, err := session.ExecCtx(ctx, upd, time.Now().Unix(), id); err != nil {
|
|
|
|
|
+ return err
|
|
|
|
|
+ }
|
|
|
|
|
+ return session.QueryRowCtx(ctx, &newVersion, "SELECT LAST_INSERT_ID()")
|
|
|
|
|
+ })
|
|
|
|
|
+ if err != nil {
|
|
|
|
|
+ return 0, err
|
|
|
|
|
+ }
|
|
|
|
|
+ // 事务外再 purge 两个缓存 key
|
|
|
|
|
+ _, _ = m.DelCacheCtx(ctx, sysUserIdKey, sysUserUsernameKey)
|
|
|
|
|
+ return newVersion, nil
|
|
|
|
|
+ }
|
|
|
|
|
+ ```
|
|
|
|
|
|
|
|
----
|
|
|
|
|
|
|
+ (`DelCacheCtx` 是 `sqlc.CachedConn` 公开接口;若 go-zero 当前版本未暴露,可在事务成功后通过 `m.ExecCtx` 触发一次空 UPDATE 以复用其自动 invalidation,或直接用 `m.rds.Del` 删 Redis key。)
|
|
|
|
|
|
|
|
-### H-4. 移除产品成员 / 降级 MemberType 时未校验"最后一个 ADMIN",可把产品彻底变成无人管理态
|
|
|
|
|
|
|
+ **方案 B(最小改动,用 `SELECT ... FOR UPDATE`):**
|
|
|
|
|
|
|
|
-- **位置**:
|
|
|
|
|
- - `internal/logic/member/removeMemberLogic.go:29-53`
|
|
|
|
|
- - `internal/logic/member/updateMemberLogic.go:30-64`
|
|
|
|
|
-- **描述**:
|
|
|
|
|
- - `RemoveMember` 的访问控制只有 `CheckManageAccess(caller, member.UserId, member.ProductCode)`;`CheckManageAccess` 里 `if caller.UserId == targetUserId { return nil }` 允许自删除,`if caller.MemberType == ADMIN { return nil }`(`checkDeptHierarchy`)后续权限级别比较又用 `callerPri < targetPri` 放行——也就是 **任意一个 ADMIN 可以把另一个 ADMIN(或者自己)移出产品**;
|
|
|
|
|
- - `UpdateMember` 可以把 ADMIN 降级为 `MEMBER`,同样没有"最后一个 ADMIN"检查。
|
|
|
|
|
|
|
+ ```go
|
|
|
|
|
+ err = m.TransactCtx(ctx, func(ctx context.Context, session sqlx.Session) error {
|
|
|
|
|
+ var cur int64
|
|
|
|
|
+ if err := session.QueryRowCtx(ctx, &cur,
|
|
|
|
|
+ fmt.Sprintf("SELECT `tokenVersion` FROM %s WHERE `id` = ? FOR UPDATE", m.table), id); err != nil {
|
|
|
|
|
+ return err
|
|
|
|
|
+ }
|
|
|
|
|
+ newVersion = cur + 1
|
|
|
|
|
+ _, err := session.ExecCtx(ctx,
|
|
|
|
|
+ fmt.Sprintf("UPDATE %s SET `tokenVersion` = ?, `updateTime` = ? WHERE `id` = ?", m.table),
|
|
|
|
|
+ newVersion, time.Now().Unix(), id)
|
|
|
|
|
+ return err
|
|
|
|
|
+ })
|
|
|
|
|
+ ```
|
|
|
|
|
|
|
|
- 假设产品 P 最初由 `CreateProduct` 自动生成 `admin_P`(ADMIN)加入。这个 admin_P 之后通过 `AddMember` 邀请了 admin_Q(ADMIN)。两人随后:
|
|
|
|
|
- - admin_Q 先 `UpdateMember(admin_P → MEMBER)` 或直接 `RemoveMember(admin_P)`;
|
|
|
|
|
- - 之后 admin_Q 再自己 `RemoveMember(admin_Q)`。
|
|
|
|
|
|
|
+ 修复后需同时确认 `ChangePassword` / `UpdateStatus` / `UpdateProfile`(status 变更分支)等"递增"路径都走同一实现,避免一致性漂移。
|
|
|
|
|
|
|
|
- 产品 P 从此**没有任何 ADMIN**。前端路径无法再新增管理员(`AddMember` 需要 ADMIN 或 SUPER_ADMIN 操作;`CheckMemberTypeAssignment` 对新 ADMIN 又要求操作者是更高级别)。虽然超管可以通过 `AddMember` 介入,但该场景下产品运营已经 **必须依赖平台管理员介入**,违背了"产品自治"的设计意图。
|
|
|
|
|
|
|
+- **优先级**:P0(立即修复,生产将周期性出现自伤)
|
|
|
|
|
|
|
|
- 类似的,`admin_P` 可以不小心把自己降级为 `MEMBER`(UpdateMember 允许),产品立刻失去管理员。
|
|
|
|
|
|
|
+---
|
|
|
|
|
|
|
|
-- **影响**:
|
|
|
|
|
- - 真实业务中,产品管理员误操作(自己把自己移除或降级)会直接让该产品进入"需平台救援"状态。
|
|
|
|
|
- - 有意的内部对抗中,一个 ADMIN 可以把其他 ADMIN 全部踢出,事实上独占管理权(已属于越权滥用,但本身合规上应有"至少保留一个 ADMIN"约束)。
|
|
|
|
|
|
|
+## ⚠️ 健壮性与安全建议 (Medium)
|
|
|
|
|
|
|
|
-- **修复方案**:在 `RemoveMember` 与 `UpdateMember`(降级时)增加"最后 ADMIN"保护:
|
|
|
|
|
|
|
+### M-A. `CountActiveAdmins` 校验在事务外,`RemoveMember` / `UpdateMember` "最后 ADMIN" 检查存在 TOCTOU
|
|
|
|
|
|
|
|
|
|
+- **位置**:
|
|
|
|
|
+ - `internal/logic/member/removeMemberLogic.go:41-49`
|
|
|
|
|
+ - `internal/logic/member/updateMemberLogic.go:50-58`
|
|
|
|
|
+ - `internal/model/productmember/sysProductMemberModel.go:49-56`(`CountActiveAdmins`)
|
|
|
|
|
+- **描述**:新加入的"最后一个 ADMIN 保护"机制是:
|
|
|
```go
|
|
```go
|
|
|
- // 伪代码
|
|
|
|
|
- if member.MemberType == consts.MemberTypeAdmin &&
|
|
|
|
|
- (operation == Remove || (operation == Update && req.MemberType != ADMIN)) {
|
|
|
|
|
|
|
+ if member.MemberType == consts.MemberTypeAdmin {
|
|
|
adminCount, _ := svcCtx.SysProductMemberModel.CountActiveAdmins(ctx, member.ProductCode)
|
|
adminCount, _ := svcCtx.SysProductMemberModel.CountActiveAdmins(ctx, member.ProductCode)
|
|
|
if adminCount <= 1 {
|
|
if adminCount <= 1 {
|
|
|
- return response.ErrBadRequest("不能移除/降级该产品的最后一个管理员")
|
|
|
|
|
|
|
+ return response.ErrBadRequest("不能移除该产品的最后一个管理员")
|
|
|
}
|
|
}
|
|
|
}
|
|
}
|
|
|
|
|
+ // …然后进入事务 DELETE
|
|
|
```
|
|
```
|
|
|
|
|
|
|
|
- 需要在 model 层新增 `CountByProductCodeAndMemberType(productCode, MemberTypeAdmin, StatusEnabled)`。同时禁止管理员对自己的 MemberType 做降级(对比 `UpdateUserStatus` 已有的"不能修改自己的状态")。
|
|
|
|
|
|
|
+ `CountActiveAdmins` 用 `QueryRowNoCacheCtx` 读 DB,**但不加锁**,且整个"判断 + DELETE"**不在同一个事务里**。
|
|
|
|
|
|
|
|
----
|
|
|
|
|
|
|
+ 并发路径:
|
|
|
|
|
|
|
|
-## ⚠️ 健壮性与安全建议 (Medium)
|
|
|
|
|
|
|
+ | 时序 | 请求 A(移除 admin_P) | 请求 B(移除 admin_Q) |
|
|
|
|
|
+ |------|------------------------|------------------------|
|
|
|
|
|
+ | T1 | `CountActiveAdmins` = 2 | |
|
|
|
|
|
+ | T2 | | `CountActiveAdmins` = 2 |
|
|
|
|
|
+ | T3 | 进入 Tx → DELETE admin_P | |
|
|
|
|
|
+ | T4 | | 进入 Tx → DELETE admin_Q |
|
|
|
|
|
+ | T5 | 产品剩余 ADMIN = 0 | 产品剩余 ADMIN = 0 |
|
|
|
|
|
|
|
|
-### M-1. 无注销接口,refreshToken 被盗后无法主动吊销
|
|
|
|
|
|
|
+ `UpdateMember`(ADMIN 降级为 MEMBER)存在同样的 TOCTOU。真实触发场景:
|
|
|
|
|
|
|
|
-- **位置**:`internal/handler/routes.go` 路由清单、`internal/middleware/jwtauthMiddleware.go`
|
|
|
|
|
-- **描述**:系统只有登录 (`/auth/login` / `/auth/adminLogin`)、刷新 (`/auth/refreshToken`)、改密 (`/auth/changePassword`) 接口。**没有任何一个接口会主动 `tokenVersion+1`**(除 `UpdatePassword` / `UpdateStatus`),也没有 `/auth/logout` 路由。
|
|
|
|
|
|
|
+ - 超管在管理后台"批量移除"两个 ADMIN;
|
|
|
|
|
+ - 两个 ADMIN 同时在前端点"移除对方";
|
|
|
|
|
+ - 自动化脚本一次性下发两条变更。
|
|
|
|
|
|
|
|
- 后果:
|
|
|
|
|
- - 用户即使在客户端"退出登录",也只是本地清了 token;已签发的 access + refresh 在整个 expire 窗口内仍然可被重放。
|
|
|
|
|
- - 用户怀疑 token 被盗时,唯一的自救手段是**修改密码**(`ChangePassword` 会 `tokenVersion+1` 并 `Clean` 缓存)。对使用 SSO、或不自己设密码(例如 OAuth 接入)的场景不可用。
|
|
|
|
|
|
|
+ 虽然概率不高,但"产品永久无 ADMIN 需平台救援"的代价高。
|
|
|
|
|
|
|
|
-- **建议**:
|
|
|
|
|
- - 新增 `/auth/logout` 接口:鉴权后将该用户的 `tokenVersion+1`(或者维护一个"签出黑名单"集合,带 TTL 到 `RefreshExpire`)。
|
|
|
|
|
- - 相应 gRPC 端也暴露 `RevokeTokens(userId, productCode)`。
|
|
|
|
|
|
|
+- **影响**:绕过了上一轮 H-4 的"最后一个 ADMIN"保护,导致产品进入无人管理态。
|
|
|
|
|
|
|
|
----
|
|
|
|
|
|
|
+- **修复方案**:把 count 检查挪到事务内、并对要变更/删除的那行加行锁即可;对其他 ADMIN 行不需要锁(因为 count 查询会用到 InnoDB 的当前读规则,配合事务隔离级别足够)。
|
|
|
|
|
|
|
|
-### M-2. refreshToken 轮转未消耗原 token,存在"双 token 并存"窗口
|
|
|
|
|
|
|
+ ```go
|
|
|
|
|
+ // RemoveMember 示例(UpdateMember 同理)
|
|
|
|
|
+ return l.svcCtx.SysProductMemberModel.TransactCtx(l.ctx, func(ctx context.Context, session sqlx.Session) error {
|
|
|
|
|
+ // 1) 锁定当前要删的行,避免重复删
|
|
|
|
|
+ var lockedId int64
|
|
|
|
|
+ lockQ := fmt.Sprintf("SELECT `id` FROM %s WHERE `id` = ? FOR UPDATE", l.svcCtx.SysProductMemberModel.TableName())
|
|
|
|
|
+ if err := session.QueryRowCtx(ctx, &lockedId, lockQ, req.Id); err != nil {
|
|
|
|
|
+ return response.ErrNotFound("成员不存在")
|
|
|
|
|
+ }
|
|
|
|
|
+ // 2) 最后一个 ADMIN 校验在事务内
|
|
|
|
|
+ if member.MemberType == consts.MemberTypeAdmin {
|
|
|
|
|
+ var cnt int64
|
|
|
|
|
+ cntQ := fmt.Sprintf(
|
|
|
|
|
+ "SELECT COUNT(*) FROM %s WHERE `productCode`=? AND `memberType`=? AND `status`=? FOR SHARE",
|
|
|
|
|
+ l.svcCtx.SysProductMemberModel.TableName(),
|
|
|
|
|
+ )
|
|
|
|
|
+ if err := session.QueryRowCtx(ctx, &cnt, cntQ, member.ProductCode, consts.MemberTypeAdmin, consts.StatusEnabled); err != nil {
|
|
|
|
|
+ return err
|
|
|
|
|
+ }
|
|
|
|
|
+ if cnt <= 1 {
|
|
|
|
|
+ return response.ErrBadRequest("不能移除该产品的最后一个管理员")
|
|
|
|
|
+ }
|
|
|
|
|
+ }
|
|
|
|
|
+ // 3) 既有的 DeleteByUserIdForProductTx / DeleteWithTx
|
|
|
|
|
+ ...
|
|
|
|
|
+ })
|
|
|
|
|
+ ```
|
|
|
|
|
|
|
|
-- **位置**:`internal/logic/pub/refreshTokenLogic.go:70-77`、`internal/server/permserver.go:141-148`
|
|
|
|
|
-- **描述**:`GenerateRefreshTokenWithExpiry` 的行为是"基于 claims 里原 token 的 `ExpiresAt` 重新签一张新 token"——此时 `tokenVersion` 并未递增,新旧两张 refreshToken 都用同一个 `tokenVersion` 命中 `tokenVersion == ud.TokenVersion` 的校验。
|
|
|
|
|
|
|
+ 备选:保留现有 `CountActiveAdmins` 调用顺序,但在 DELETE 成功**之后**再 count 一次,若 `=0` 则显式 `return fmt.Errorf("rollback: last admin")` 触发回滚。这个实现更简单。
|
|
|
|
|
|
|
|
- 如果攻击者偷到 refreshToken 的同时、真实用户也在使用:
|
|
|
|
|
- 1. 真实用户用原 rt 换 rt1、rt2、rt3……
|
|
|
|
|
- 2. 攻击者用原 rt 换 rt';
|
|
|
|
|
- 3. 两边的 `tokenVersion` 相同,两边都能继续无限续期到 `refreshExpire`。
|
|
|
|
|
|
|
+- **优先级**:P1
|
|
|
|
|
|
|
|
- 标准的 refresh token rotation 语义应当是"已使用的 refresh token 立即一次性失效"(通过 jti 黑名单、或者每次刷新都让 `tokenVersion` 递增)。当前实现不具备这个能力。
|
|
|
|
|
|
|
+---
|
|
|
|
|
|
|
|
-- **影响**:refreshToken 泄露后几乎无法挽回,直到 refreshExpire 自然过期,或用户主动改密。
|
|
|
|
|
|
|
+### M-B. `/auth/refreshToken` 路由无任何限流,可对单用户发起 DB 写热点 DoS 并清空其所有缓存
|
|
|
|
|
|
|
|
-- **建议**:
|
|
|
|
|
- - 方案 1(最小侵入):在 refresh 时让 `sys_user.tokenVersion` 递增,使老 refresh token 立即失效。缺点是多端登录无法共存。
|
|
|
|
|
- - 方案 2:在 Redis 里维护 `refreshJti → userId` 的一次性 map,`RefreshToken` 时 `GETDEL`,不存在即失败。`jti` 存入 `RegisteredClaims`。
|
|
|
|
|
- - 方案 3:缩短 refreshExpire,降低风险窗口。
|
|
|
|
|
|
|
+- **位置**:`internal/handler/routes.go:176-185`(`/auth/refreshToken` 无中间件)、`internal/logic/pub/refreshTokenLogic.go:66-70`
|
|
|
|
|
+- **描述**:
|
|
|
|
|
+ - 上一轮 M-2 修复后,`RefreshToken` 每次都会:
|
|
|
|
|
+ 1. `IncrementTokenVersion`:`UPDATE sys_user SET tokenVersion=tokenVersion+1` + 清两个 cache key;
|
|
|
|
|
+ 2. `UserDetailsLoader.Clean(userId)`:`SMEMBERS` 该用户 index → `DEL` 所有 cache key → `DEL` index key。
|
|
|
|
|
+ - 路由挂载处(`routes.go:176-185`)**没有 `JwtAuth`、没有 IP 限流**,只有签名 + `tokenVersion` 校验。
|
|
|
|
|
+ - 只要攻击者曾经拿到过任意一张有效 refreshToken(或者就是合法用户自己被攻破),就能每秒反复调用:
|
|
|
|
|
+ - 每次一条 `UPDATE sys_user`(写热点,命中行锁);
|
|
|
|
|
+ - 每次一轮 Redis `SMEMBERS` + `DEL`(用户缓存整体失效,后续所有请求都 miss 并穿透到 DB 的 `loadFromDB`,进而扇出 6 条 DB 查询)。
|
|
|
|
|
+ - 单用户持续被刷,等价于"该用户的每次业务请求都穿透 loader,全部冷路径查 DB",且对 `sys_user` 表形成写热点。如果该用户是 ADMIN/超管,其 `loadFromDB` 里 `loadPerms` 会扫 `sys_perm` 全表拉"该产品全量 code",代价更高。
|
|
|
|
|
+
|
|
|
|
|
+ 关键是:**刷新是发出 token 后就能做的,没有任何 IP/账号层面的限流**。`AdminLoginRateLimit` / `ProductLoginRateLimit` 只防登录入口,`SyncRateLimit` 只防 `/perm/sync`。
|
|
|
|
|
+
|
|
|
|
|
+- **影响**:单个凭据即可对权限系统制造持续的 DB 写 + 缓存穿透;且由于每次刷新都轮转 refresh token,攻击者总能拿到下一张有效凭据继续刷。
|
|
|
|
|
+- **修复方案**:
|
|
|
|
|
+ - 在 `/auth/refreshToken` 前挂一条 `refreshLimiter`,key = `user:<userId>` 或 `ip+user`,窗口建议 60s/6 次(用户实际只会在 access 过期前偶发刷新,多端也不太会超过 3 次)。
|
|
|
|
|
+ - 或者给 `refreshTokenLogic` 内部加一层"Redis 计数器":以 `rl:refresh:<userId>` 为 key 做自增 + TTL。
|
|
|
|
|
+ - 超额后不应无条件 429,可选择降级为"返回同一张已经签发的 access token",但这又引入复杂度,保持 429 最简单。
|
|
|
|
|
+
|
|
|
|
|
+- **优先级**:P1
|
|
|
|
|
|
|
|
---
|
|
---
|
|
|
|
|
|
|
|
-### M-3. 产品自动管理员密码实际熵仅 32 bits
|
|
|
|
|
|
|
+### M-C. 产品端登录 `ValidateProductLogin`:存在用户名枚举 & 选择性账号锁定
|
|
|
|
|
|
|
|
-- **位置**:`internal/logic/product/createProductLogic.go:80`、`157-163`
|
|
|
|
|
-- **描述**:
|
|
|
|
|
|
|
+- **位置**:`internal/logic/pub/loginService.go:32-46`
|
|
|
|
|
+- **描述**:当前顺序是:
|
|
|
```go
|
|
```go
|
|
|
- func generateRandomHex(length int) (string, error) {
|
|
|
|
|
- b := make([]byte, length)
|
|
|
|
|
- if _, err := rand.Read(b); err != nil {
|
|
|
|
|
- return "", fmt.Errorf(...)
|
|
|
|
|
|
|
+ u, err := svcCtx.SysUserModel.FindOneByUsername(ctx, username)
|
|
|
|
|
+ if err != nil { // 不存在 → 401
|
|
|
|
|
+ return nil, &LoginError{Code: 401, Message: "用户名或密码错误"}
|
|
|
|
|
+ }
|
|
|
|
|
+ if svcCtx.UsernameLoginLimit != nil {
|
|
|
|
|
+ code, _ := svcCtx.UsernameLoginLimit.Take(username)
|
|
|
|
|
+ if code == limit.OverQuota {
|
|
|
|
|
+ return nil, &LoginError{Code: 429, Message: ...}
|
|
|
}
|
|
}
|
|
|
- return hex.EncodeToString(b)[:length], nil
|
|
|
|
|
}
|
|
}
|
|
|
|
|
+ if u.Status != consts.StatusEnabled { ... }
|
|
|
|
|
+ if err := bcrypt.CompareHashAndPassword(...); err != nil { return 401 }
|
|
|
```
|
|
```
|
|
|
- `rand.Read` 填充 `length` 字节,`hex.EncodeToString` 产生 `2*length` 个 hex 字符,随后**截断取前 `length` 个字符**——也就是实际只保留了前 `length/2` 字节的随机性,相当于 `4*length` bits。
|
|
|
|
|
|
|
|
|
|
- - `generateRandomHex(32)` appKey:实际 128 bits,OK。
|
|
|
|
|
- - `generateRandomHex(64)` appSecret:实际 256 bits,OK。
|
|
|
|
|
- - `generateRandomHex(8)` adminPassword:**只有 32 bits ≈ 4e9 种可能**。虽然 `MustChangePassword=Yes` 会强制首登改密,但这个一次性密码在超管拿到之后到管理员首次登录之前的窗口内暴力破解可达。依赖外层 `UsernameLoginLimit`(5min/10 次)间接保护并不健壮。
|
|
|
|
|
|
|
+ 两个衍生问题:
|
|
|
|
|
|
|
|
-- **建议**:修正截断边界或直接提高长度:
|
|
|
|
|
|
|
+ 1. **用户名枚举**:不存在的 username 直接返回 401、**不消耗任何限流配额**,响应时间也很短(不走 bcrypt);存在的 username 则可能返回 401(密码错)或 429(锁定)或 403(冻结)。攻击者通过"时间 + 响应码"组合能稳定区分 username 是否存在。
|
|
|
|
|
+ 2. **选择性锁定**:攻击者探测出真实 username 后,对该 username 连打 10 次,该 username 立即被 `UsernameLoginLimit`(5min/10 次,key 纯 username)锁定 5 分钟,真正用户同期无法登录。配合 `AdminLoginRateLimit`(IP 60s/30)毫无阻塞。
|
|
|
|
|
|
|
|
- ```go
|
|
|
|
|
- func generateRandomHex(length int) (string, error) {
|
|
|
|
|
- byteLen := (length + 1) / 2
|
|
|
|
|
- b := make([]byte, byteLen)
|
|
|
|
|
- if _, err := rand.Read(b); err != nil {
|
|
|
|
|
- return "", err
|
|
|
|
|
- }
|
|
|
|
|
- return hex.EncodeToString(b)[:length], nil
|
|
|
|
|
- }
|
|
|
|
|
- ```
|
|
|
|
|
|
|
+ `/auth/adminLogin` 已经把 `UsernameLoginLimit.Take` 移到 `ManagementKey` 校验之后,享受了一定保护(攻击者必须知道 ManagementKey 才能触发)。但 `/auth/login` 没有这种护栏。
|
|
|
|
|
|
|
|
- 同时 `adminPassword` 改为 `generateRandomHex(16)`(64 bits)起步。
|
|
|
|
|
|
|
+- **影响**:外部攻击者可在不触发限流的前提下批量探测有效 username;对已知用户可周期性(每 5 分钟一轮)持续锁定。
|
|
|
|
|
+- **修复方案**:
|
|
|
|
|
+ - **限流 key 加 IP 维度**:`UsernameLoginLimit` 的 key 改为 `fmt.Sprintf("%s:%s", ip, username)`,同时保留一个更宽松的"纯 IP 限流"(已有 `ProductLoginRateLimit`),这样攻击者无法通过廉价 IP 锁定目标账号。
|
|
|
|
|
+ - **`FindOneByUsername` 之前先 `Take`**:让 Take 对无效 username 也消耗配额(这样无效 username 也会得 429,不再区分);或者在 username 不存在时直接 `time.Sleep` 一个近似 bcrypt 的耗时,避免时间侧信道。
|
|
|
|
|
+ - **成功登录时重置计数**(go-zero `PeriodLimit` 不支持 reset,可以改用 Redis 自增 + 登录成功 `DEL` key)。
|
|
|
|
|
+
|
|
|
|
|
+- **优先级**:P1
|
|
|
|
|
|
|
|
---
|
|
---
|
|
|
|
|
|
|
|
-### M-4. `DeptTree` 对所有登录用户开放,暴露完整组织架构
|
|
|
|
|
|
|
+### M-D. `DeleteRoleLogic` 的 `FindUserIdsByRoleId` 写在事务回调内,却**没有用 session**,仍是旧连接读
|
|
|
|
|
|
|
|
-- **位置**:`internal/logic/dept/deptTreeLogic.go`、`internal/handler/routes.go:42-69`
|
|
|
|
|
-- **描述**:`/api/dept/tree` 只挂了 `JwtAuth`。`DeptTreeLogic.DeptTree` 自身完全不做权限过滤,**任意 JWT 通过的用户(包括任意产品的普通 MEMBER)**都能拉到全公司组织架构(包括 `deptType=DEV`、部门名称、层级结构、备注)。
|
|
|
|
|
-- **影响**:组织架构常包含内部命名、隐含岗位属性(DEV 部门自动获得全权限),属于内部敏感信息;不应对产品端的普通成员暴露。
|
|
|
|
|
-- **建议**:
|
|
|
|
|
- - 严格版:仅超管可拉全量;其余用户只能拿"自身部门 + 其下级子部门"(`strings.HasPrefix(d.Path, caller.DeptPath)`)。
|
|
|
|
|
- - 宽松版:超管 + ADMIN / DEVELOPER 可见;MEMBER 不可调用该接口。
|
|
|
|
|
|
|
+- **位置**:
|
|
|
|
|
+ - `internal/logic/role/deleteRoleLogic.go:40-50`
|
|
|
|
|
+ - `internal/model/userrole/sysUserRoleModel.go:55-62`(`FindUserIdsByRoleId` 用的是 `m.QueryRowsNoCacheCtx`,不接 `session`)
|
|
|
|
|
+- **描述**:
|
|
|
|
|
+ ```go
|
|
|
|
|
+ if err := l.svcCtx.SysRoleModel.TransactCtx(l.ctx, func(ctx context.Context, session sqlx.Session) error {
|
|
|
|
|
+ affectedUserIds, _ = l.svcCtx.SysUserRoleModel.FindUserIdsByRoleId(ctx, req.Id) // ← 这里没传 session
|
|
|
|
|
+ ...
|
|
|
|
|
+ return l.svcCtx.SysRoleModel.DeleteWithTx(ctx, session, req.Id)
|
|
|
|
|
+ })
|
|
|
|
|
+ ```
|
|
|
|
|
|
|
|
----
|
|
|
|
|
|
|
+ `FindUserIdsByRoleId` 直接用 `m.QueryRowsNoCacheCtx`(非事务连接、非当前 session)查询。结果:
|
|
|
|
|
|
|
|
-### M-5. `ProductList` / `ProductDetail` 对所有登录用户返回产品元数据
|
|
|
|
|
|
|
+ - 这一行读位于事务**之外**,本事务内尚未提交的变更看不到;
|
|
|
|
|
+ - 反之,事务**之外**并发提交的 `BindRoles`(另一 goroutine 给这个 roleId 插入新行)会被这条 SELECT **看到,或看不到**,取决于提交时序;
|
|
|
|
|
+ - 真正的问题是:**在当前事务 COMMIT 之前**,如果另一条已提交的并发事务给 roleId 加了新用户 U',本事务的 DELETE(`DeleteByRoleIdTx`)**会把 U' 也删掉**(因为它用 `DELETE ... WHERE roleId=?`),但 `affectedUserIds` 中不包含 U',所以**U' 的 UserDetails 缓存不会被清理**。
|
|
|
|
|
|
|
|
-- **位置**:`internal/logic/product/productListLogic.go`、`internal/logic/product/productDetailLogic.go`
|
|
|
|
|
-- **描述**:任何 JWT 通过的用户都能遍历系统全部产品(`code`、`name`、`status`、`remark`、`createTime`)。虽然 `AppKey` 只对超管返回,但产品清单本身对所有成员可见。跨产品用户可以探测出系统内其他产品的存在(例如"内部管理后台"、"支付中心")。
|
|
|
|
|
-- **建议**:
|
|
|
|
|
- - 仅超管可列全部;非超管只能看到自己作为成员的产品(join `sys_product_member` 过滤)。
|
|
|
|
|
- - `ProductDetail` 增加同样的归属校验:非超管只能看自己所在产品。
|
|
|
|
|
|
|
+ 同样的问题在 `BindRolePermsLogic`(`bindRolePermsLogic.go:127`)已经通过"事务提交后再查"规避;`UpdateRoleLogic`(`updateRoleLogic.go:73`)也是事务外后查。只剩 `DeleteRole` 这一条路径仍然错位。
|
|
|
|
|
|
|
|
----
|
|
|
|
|
|
|
+- **影响**:`DeleteRole` 并发 `BindRoles` 的小概率场景下,漏清一批用户的缓存;这些用户会继续持有"引用已删角色"的权限集合,直到 300s TTL 自然过期。
|
|
|
|
|
+- **修复方案**:把 `FindUserIdsByRoleId` 挪到事务提交**之后**:
|
|
|
|
|
|
|
|
-### M-6. 产品端登录的用户名锁定 DoS(账号级)
|
|
|
|
|
|
|
+ ```go
|
|
|
|
|
+ if err := l.svcCtx.SysRoleModel.TransactCtx(l.ctx, func(ctx context.Context, session sqlx.Session) error {
|
|
|
|
|
+ if err := l.svcCtx.SysRolePermModel.DeleteByRoleIdTx(ctx, session, req.Id); err != nil { return err }
|
|
|
|
|
+ if err := l.svcCtx.SysUserRoleModel.DeleteByRoleIdTx(ctx, session, req.Id); err != nil { return err }
|
|
|
|
|
+ return l.svcCtx.SysRoleModel.DeleteWithTx(ctx, session, req.Id)
|
|
|
|
|
+ }); err != nil {
|
|
|
|
|
+ return err
|
|
|
|
|
+ }
|
|
|
|
|
+ // 提交之后再读(此时该 roleId 已无任何关联行,但仍能查到曾经的 userId——需要在 DELETE 之前保留)
|
|
|
|
|
+ ```
|
|
|
|
|
|
|
|
-- **位置**:`internal/logic/pub/loginService.go:33-38`
|
|
|
|
|
-- **描述**:`UsernameLoginLimit.Take(username)` 在无任何前置鉴权的情况下被消费,5min/10 次、纯 username 维度。任何外部攻击者只需知道目标用户名,即可以 10 次失败请求锁定该账号 5 分钟;通过定时重放可达到长时间账号级 DoS。
|
|
|
|
|
-- **影响**:
|
|
|
|
|
- - 登录入口对外开放、任意 IP 可触达;
|
|
|
|
|
- - 配合 H-3(管理后台也有同样的问题),造成系统级账号锁定攻击面。
|
|
|
|
|
-- **建议**:与 H-3 同步处理,把 rate limit key 改为 `ip:username`(或对同一 IP 的失败次数独立设桶);对成功登录重置该 username 的计数。
|
|
|
|
|
|
|
+ **注意**:`DELETE ... WHERE roleId=?` 执行后,`sys_user_role` 里 `roleId=?` 的行已没了,提交后再查 `FindUserIdsByRoleId` 得空集。正确做法是在事务内(用 session)先读一次,然后再 DELETE:
|
|
|
|
|
|
|
|
----
|
|
|
|
|
|
|
+ ```go
|
|
|
|
|
+ if err := l.svcCtx.SysRoleModel.TransactCtx(l.ctx, func(ctx context.Context, session sqlx.Session) error {
|
|
|
|
|
+ // 在事务内用 session 读,锁住这些关系行
|
|
|
|
|
+ query := fmt.Sprintf("SELECT `userId` FROM `sys_user_role` WHERE `roleId` = ? FOR UPDATE")
|
|
|
|
|
+ if err := session.QueryRowsCtx(ctx, &affectedUserIds, query, req.Id); err != nil {
|
|
|
|
|
+ return err
|
|
|
|
|
+ }
|
|
|
|
|
+ ...DELETE...
|
|
|
|
|
+ return l.svcCtx.SysRoleModel.DeleteWithTx(ctx, session, req.Id)
|
|
|
|
|
+ })
|
|
|
|
|
+ ```
|
|
|
|
|
|
|
|
-### M-7. `X-Real-IP` 信任策略过简,未支持 `X-Forwarded-For`
|
|
|
|
|
|
|
+ `FOR UPDATE` 让并发 `BindRoles` 的 `INSERT sys_user_role` 被挂起,直到当前事务提交。这样 affectedUserIds 覆盖所有实际被删的行。
|
|
|
|
|
|
|
|
-- **位置**:`internal/middleware/ratelimitMiddleware.go:41-52`
|
|
|
|
|
-- **描述**:
|
|
|
|
|
- 1. 仅识别 `X-Real-IP`,没有兼容多数 ingress / ELB 默认设置的 `X-Forwarded-For`。
|
|
|
|
|
- 2. 只要 `behindProxy=true`,任何 `X-Real-IP` 无条件被信任;如果反向代理没有覆盖客户端原有 header,攻击者可以通过伪造 header 分散限流桶。
|
|
|
|
|
-- **建议**:
|
|
|
|
|
- - 支持 XFF:按最右可信节点取值。
|
|
|
|
|
- - 配置可信代理 CIDR 列表,仅当 `RemoteAddr` 在可信网段内时才信任 header。
|
|
|
|
|
- - 保留目前行为作为降级,优先级:`XFF`(可信时)> `X-Real-IP`(可信时)> `RemoteAddr`。
|
|
|
|
|
|
|
+ 需要在 `SysUserRoleModel` 接口新增一个 `FindUserIdsByRoleIdForUpdateTx(ctx, session, roleId)`,或者 DeleteRole 内直接拼 SQL(如上)。
|
|
|
|
|
+
|
|
|
|
|
+- **优先级**:P2(低概率,但修复成本很小)
|
|
|
|
|
|
|
|
---
|
|
---
|
|
|
|
|
|
|
|
-### M-8. 缓存失效与 DB 事务非原子:`Clean/Del/BatchDel` 失败时静默吞错
|
|
|
|
|
|
|
+### M-E. 所有 `Delete*ForProductTx` / `DeleteByRoleIdTx` / `DeleteByUserIdForProductTx` 的"先 SELECT 再 DELETE"模式非原子
|
|
|
|
|
|
|
|
- **位置**:
|
|
- **位置**:
|
|
|
- - 写路径:`UpdateUser`、`UpdateUserStatus`、`ChangePassword`、`BindRoles`、`SetUserPerms`、`UpdateMember`、`RemoveMember`、`DeleteRole`、`BindRolePerms`、`UpdateRole`、`UpdateProduct`、`ExecuteSyncPerms`、`UpdateDept`
|
|
|
|
|
- - Loader:`internal/loaders/userDetailsLoader.go:138-173`
|
|
|
|
|
-- **描述**:所有写操作都是"DB tx 提交 → Redis 失效"两步,但 `Del / Clean / BatchDel` 内部的 Redis 错误只打日志。Redis 瞬时抖动期间,DB 已提交但缓存未失效,在 `defaultCacheTTL=300s` 之内其他请求命中旧缓存,包括 `tokenVersion / MemberType / Perms` 等安全关键字段。
|
|
|
|
|
- - 对修改密码、冻结账号、删除角色这类"收紧"操作,最大 5 分钟的旧视图继续有效,是不可忽视的安全风险窗口。
|
|
|
|
|
- - 对"放宽"操作(添加角色、启用成员),旧视图是"没权限",用户体验问题,相对安全。
|
|
|
|
|
-- **建议**:
|
|
|
|
|
- - 对安全关键动作(`UpdateUserStatus` 冻结、`ChangePassword`、`RemoveMember`、`UpdateMember Status=Disabled`),`Clean` 失败必须返回 5xx,或把这类 key 的 TTL 收紧到 60s。
|
|
|
|
|
- - 引入"延迟双删"或消息队列补偿:DB 提交后发送缓存失效事件,由消费端重试到成功。
|
|
|
|
|
|
|
+ - `internal/model/userrole/sysUserRoleModel.go:75-107,109-136`
|
|
|
|
|
+ - `internal/model/roleperm/sysRolePermModel.go:73-117`
|
|
|
|
|
+ - `internal/model/userperm/sysUserPermModel.go:43-63`
|
|
|
|
|
+ - `internal/model/perm/sysPermModel.go:94-154`(`DisableNotInCodesWithTx`)
|
|
|
|
|
+- **描述**:统一套路是:
|
|
|
|
|
+ ```go
|
|
|
|
|
+ // 1) 先用 QueryRowsNoCacheCtx 读出要删的行,拼 cache keys
|
|
|
|
|
+ if err := m.QueryRowsNoCacheCtx(ctx, &list, findQuery, ...); err != nil { return err }
|
|
|
|
|
+ keys := buildCacheKeys(list)
|
|
|
|
|
+ // 2) 再 m.ExecCtx 包装 session.ExecCtx,借助 ExecCtx 的 cache-aside invalidate
|
|
|
|
|
+ _, err := m.ExecCtx(ctx, func(ctx, conn) {
|
|
|
|
|
+ return session.ExecCtx(ctx, deleteQuery, ...)
|
|
|
|
|
+ }, keys...)
|
|
|
|
|
+ ```
|
|
|
|
|
|
|
|
----
|
|
|
|
|
|
|
+ 问题:
|
|
|
|
|
+ 1. **SELECT 用的是 `m.QueryRowsNoCacheCtx`(主库默认连接,读未提交前的数据)**,它既不是 `session`,也没有加锁。如果此刻有并发 INSERT 提交(例如 BindRoles 插入新 `sys_user_role`),那条新行的 cache key 不会进入 `keys` 列表。
|
|
|
|
|
+ 2. 紧接着 `DELETE ... WHERE roleId=?` 会一并删掉该新行;新行的 `cacheSysUserRoleIdPrefix:<newId>` 和 `cacheSysUserRoleUserIdRoleIdPrefix:<userId>:<roleId>` 缓存**不会被 invalidate**。
|
|
|
|
|
+ 3. 一般情况下,新行刚插入缓存也没写过(只有 `FindOne` 会写缓存,insert 不写),所以大部分时候这些 key 本来就不存在,问题不显现。但一旦有其它读路径(例如 `FindOne`、或 `DeleteByUserIdAndRoleIdsTx` 里的 list 查询本身)在极短窗口内 cache miss + 回填,就会留下**已 DELETE 但仍在 Redis 的幽灵缓存**,直到 TTL 自然过期。
|
|
|
|
|
|
|
|
-### M-9. `BindRolePerms` / `UpdateRole` / `DeleteRole` 的"受影响用户查询"发生在事务之外,存在漏清缓存的窗口
|
|
|
|
|
|
|
+ 同时 `perm/sysPermModel.go:DisableNotInCodesWithTx` 走的路径里,SELECT 是非事务读,之后的 UPDATE 是 session.ExecCtx,如果并发 `SyncPerms` 插入了新 perm code(不在 `codes` 列表),第二轮 SELECT 不会看到,但 UPDATE 的 `WHERE NOT IN (codes)` **会禁用**它——该 perm 的缓存键不会被清理。
|
|
|
|
|
|
|
|
-- **位置**:
|
|
|
|
|
- - `internal/logic/role/bindRolePermsLogic.go:126-127`
|
|
|
|
|
- - `internal/logic/role/deleteRoleLogic.go:39-53`
|
|
|
|
|
- - `internal/logic/role/updateRoleLogic.go:66-67`
|
|
|
|
|
-- **描述**:三个接口的模式都是"事务外 `FindUserIdsByRoleId` → 事务内写 DB → 事务外 `BatchDel`"。在**事务开始后、事务提交前**,若有另一个 goroutine 通过 `BindRoles` 把新用户加进这个角色(`sys_user_role` 插入并已提交),当前 goroutine 计算 `affectedUserIds` 时没有包含这些新用户。
|
|
|
|
|
- - 事务提交之后,新加入的用户缓存不会被当前流程清理。他们会用"角色权限变更前的 perms 快照"继续工作 5 分钟。
|
|
|
|
|
- - 另一条 `BindRoles` 流程会对它自己绑的用户 `Clean`,但不会感知到本流程对角色权限的改动。
|
|
|
|
|
|
|
+ 这是一类普遍的"缓存 keys ≠ 实际被影响行"的问题。
|
|
|
|
|
|
|
|
- 虽然是低概率双写,但对"删除角色"这种一次性收紧操作,未清掉的那一批用户仍会基于"已删除角色下的权限集"工作(实际上,一旦事务提交,`sys_role_perm` 清空,`loadPerms` 的 role path 自然会走 `FindPermIdsByRoleIds` 得空——但缓存是上次的;只有下次 miss 才会触发)。本质是**缓存永远滞后于 5 分钟**。
|
|
|
|
|
-- **建议**:把 `FindUserIdsByRoleId` 放进事务内,并使用 `SELECT ... FOR UPDATE` 锁住这些用户的绑定关系,避免并发新增;或在事务提交后再 `FindUserIdsByRoleId` 一次(更简单)——这样保证看到的是最新的用户集:
|
|
|
|
|
|
|
+- **影响**:
|
|
|
|
|
+ - 罕见但可触发的"幽灵缓存"问题,表现为"某用户/角色/权限在 DB 已删但 Redis 仍返回老值",最多持续 300s(默认 TTL)+ loader.Load 的 singleflight 覆盖。
|
|
|
|
|
+ - 实际业务影响通常被 `UserDetailsLoader` 的 300s TTL 吸收——`loader.Load` 是以用户维度缓存 `UserDetails`,不直接依赖 `sys_user_role.id` 级 cache key。所以**实际暴露面主要在直接调 `SysXxxModel.FindOne(id)` 的路径**。
|
|
|
|
|
+ - 风险较低,但长期看会积累数据不一致。
|
|
|
|
|
|
|
|
- ```go
|
|
|
|
|
- // 事务提交成功之后
|
|
|
|
|
- affectedUserIds, _ := l.svcCtx.SysUserRoleModel.FindUserIdsByRoleId(l.ctx, req.RoleId)
|
|
|
|
|
- l.svcCtx.UserDetailsLoader.BatchDel(l.ctx, affectedUserIds, role.ProductCode)
|
|
|
|
|
- ```
|
|
|
|
|
|
|
+- **修复方案**:
|
|
|
|
|
+ - **方案一(推荐)**:把 SELECT 改为在 session 内执行,并且 `FOR UPDATE`:
|
|
|
|
|
+
|
|
|
|
|
+ ```go
|
|
|
|
|
+ findQuery := fmt.Sprintf("SELECT ... WHERE `roleId` = ? FOR UPDATE")
|
|
|
|
|
+ if err := session.QueryRowsCtx(ctx, &list, findQuery, roleId); err != nil { return err }
|
|
|
|
|
+ ```
|
|
|
|
|
+
|
|
|
|
|
+ 这样在事务提交前,其他事务无法插入新行到同一 roleId 范围内;SELECT 和 DELETE 看到的是完全一致的集合。
|
|
|
|
|
|
|
|
- 当前是在事务之前拿的,移到事务之后即可显著减少竞态。
|
|
|
|
|
|
|
+ - **方案二**:事务提交后再做一次"宽清理"——直接 `SREM` 掉该 `roleId` / `productCode` 下的全部 cache 键;粒度粗但绝不会漏。
|
|
|
|
|
+ - **方案三(最小成本)**:既然缓存不一致窗口 ≤ 300s 且 `UserDetailsLoader` 层做主业务隔离,可将这类 cache key 的 TTL 降到 60s,限制风险窗口。
|
|
|
|
|
+
|
|
|
|
|
+- **优先级**:P2(风险低、影响面小,但建议与 M-D 合并一次性收敛)
|
|
|
|
|
|
|
|
---
|
|
---
|
|
|
|
|
|
|
|
-### M-10. `FindByPathPrefix` LIKE 转义依赖 MySQL 默认 `\` 转义
|
|
|
|
|
|
|
+### M-F. `CountActiveAdmins` SQL 字面量 `'ADMIN'` 与 `consts.MemberTypeAdmin` 解耦,僵尸常量同步风险
|
|
|
|
|
|
|
|
-- **位置**:`internal/model/dept/sysDeptModel.go:56-64`
|
|
|
|
|
-- **描述**:`strings.NewReplacer("%", "\\%", "_", "\\_").Replace(pathPrefix)` 产出的是 `/xxx/\%yyy/`,在 SQL `WHERE path LIKE ?` 下默认依赖 MySQL 的 `\` 作为 LIKE 转义符。当 `sql_mode` 含 `NO_BACKSLASH_ESCAPES` 时,`\%` 会被当作两个字符,匹配失败或命中预期外数据。
|
|
|
|
|
|
|
+- **位置**:`internal/model/productmember/sysProductMemberModel.go:51`
|
|
|
|
|
+- **描述**:
|
|
|
|
|
+ ```go
|
|
|
|
|
+ query := fmt.Sprintf("SELECT COUNT(*) FROM %s WHERE `productCode` = ? AND `memberType` = 'ADMIN' AND `status` = 1", m.table)
|
|
|
|
|
+ ```
|
|
|
|
|
|
|
|
- 此函数在当前产线代码中**已无调用方**(见 M-11 僵尸代码),但为了避免将来复用踩坑,建议显式 `LIKE ? ESCAPE '!'` 并在应用层用 `!` 作为转义符。
|
|
|
|
|
|
|
+ `'ADMIN'` 和 `1`(StatusEnabled)都是裸字面量,没有引用 `consts.MemberTypeAdmin` / `consts.StatusEnabled`。一旦常量值调整(例如未来新增 `OWNER` 或 `ADMIN` 更名为 `PRODUCT_ADMIN`),Go 调用方的判断和 SQL 查询会**悄悄脱钩**——不会编译报错、不会测试失败(除非有专门的覆盖测试),但"最后一个 ADMIN"保护**逻辑失效**(SQL 找不到任何 ADMIN,count 永远 = 0,所有移除都被拒)。
|
|
|
|
|
+
|
|
|
|
|
+- **影响**:维护风险(契约隐性约定);也违反"同一信息不写两遍"原则,与 `FindAllCodesByProductCode`(`perm/sysPermModel.go:56`)等现有做法不一致(那边用了 `consts.StatusEnabled` 拼到 SQL)。
|
|
|
|
|
+- **修复方案**:
|
|
|
|
|
|
|
|
-- **建议**:
|
|
|
|
|
```go
|
|
```go
|
|
|
- escaped := strings.NewReplacer("!", "!!", "%", "!%", "_", "!_").Replace(pathPrefix)
|
|
|
|
|
- query := fmt.Sprintf("SELECT ... WHERE `path` LIKE ? ESCAPE '!' ORDER BY ...", ...)
|
|
|
|
|
|
|
+ query := fmt.Sprintf(
|
|
|
|
|
+ "SELECT COUNT(*) FROM %s WHERE `productCode` = ? AND `memberType` = ? AND `status` = ?",
|
|
|
|
|
+ m.table,
|
|
|
|
|
+ )
|
|
|
|
|
+ return m.QueryRowNoCacheCtx(ctx, &count, query, productCode, consts.MemberTypeAdmin, consts.StatusEnabled)
|
|
|
```
|
|
```
|
|
|
|
|
|
|
|
|
|
+- **优先级**:P2
|
|
|
|
|
+
|
|
|
---
|
|
---
|
|
|
|
|
|
|
|
-### M-11. 僵尸代码:`FindByPathPrefix` / `FindByParentId` / `FindRoleIdsByUserId` 仅测试引用
|
|
|
|
|
|
|
+### M-G. `UserList` 在有 `productCode` 分支时,JOIN 后又多做了一次 `FindMapByProductCodeUserIds`
|
|
|
|
|
|
|
|
- **位置**:
|
|
- **位置**:
|
|
|
- - `internal/model/dept/sysDeptModel.go:47-64`(`FindByParentId`、`FindByPathPrefix`)
|
|
|
|
|
- - `internal/model/userrole/sysUserRoleModel.go:37-44`(`FindRoleIdsByUserId`,`UserDetailLogic.userDetailLogic.go:55` 理论上会调用,但看下面)
|
|
|
|
|
-- **描述**:经全仓搜索:
|
|
|
|
|
- - `FindByPathPrefix` 和 `FindByParentId`:自上一轮 `DeleteDept` 改为行锁 + 子查询后,两者在生产代码中没有任何调用方,仅测试/mock 保留。
|
|
|
|
|
- - `FindRoleIdsByUserId`(全产品汇总):在 `userDetailLogic.go:55` 的 `else` 分支调用("没有 productCode 上下文时")。该分支仅在超管登录管理后台且未带产品上下文时进入;但前端在"用户详情"页面一般会带产品上下文。调用路径存在但极少。
|
|
|
|
|
|
|
+ - `internal/logic/user/userListLogic.go:51-76`
|
|
|
|
|
+ - `internal/model/user/sysUserModel.go:59-76`(`FindListByProductMembers` 已 INNER JOIN `sys_product_member`)
|
|
|
|
|
+- **描述**:当 `req.ProductCode != ""` 时:
|
|
|
|
|
+ 1. `FindListByProductMembers` 已经 `INNER JOIN sys_product_member pm ON u.id=pm.userId WHERE pm.productCode=?`——此时每行都有对应的 `pm.memberType`;
|
|
|
|
|
+ 2. 代码又发起一次 `FindMapByProductCodeUserIds` 把 `userIds` IN 回 `sys_product_member` 取 `memberType`。
|
|
|
|
|
|
|
|
- 建议清理 `FindByPathPrefix` / `FindByParentId`(或保留但加注释,避免重复发明轮子)。`FindRoleIdsByUserId` 仍有路径保留。
|
|
|
|
|
|
|
+ 两次查询做了完全一样的事。这是一个纯粹的冗余读——第一次 JOIN 丢弃了 `pm.*`,第二次再 IN 取回。
|
|
|
|
|
+- **影响**:产品端用户列表多一次全表/IN 扫描,无功能性问题。
|
|
|
|
|
+- **修复方案**:扩展 `FindListByProductMembers` 返回 `[]struct{ *SysUser; MemberType string }`(或返回 `memberType` 数组),替掉第二次查询。
|
|
|
|
|
|
|
|
-- **建议**:
|
|
|
|
|
- - 删除 `FindByPathPrefix` / `FindByParentId`,或至少加 `// Deprecated` 注释。
|
|
|
|
|
- - `FindRoleIdsByUserId` 保留。
|
|
|
|
|
|
|
+- **优先级**:P2
|
|
|
|
|
|
|
|
---
|
|
---
|
|
|
|
|
|
|
|
-### M-12. `UpdateUserStatus` 的 productCode 归属校验与超管路径重复
|
|
|
|
|
|
|
+### M-3(遗留). 产品初始管理员随机密码熵仍为 32 bits
|
|
|
|
|
|
|
|
-- **位置**:`internal/logic/user/updateUserStatusLogic.go:49-60`
|
|
|
|
|
-- **描述**:
|
|
|
|
|
|
|
+- **位置**:`internal/logic/product/createProductLogic.go:80-81, 158-164`
|
|
|
|
|
+- **状态**:未修复。`generateRandomHex(8)` 依然使用了截断 bug 的路径:
|
|
|
```go
|
|
```go
|
|
|
- if productCode != "" {
|
|
|
|
|
- caller := middleware.GetUserDetails(l.ctx)
|
|
|
|
|
- if caller != nil && !caller.IsSuperAdmin {
|
|
|
|
|
- if _, err := svcCtx.SysProductMemberModel.FindOneByProductCodeUserId(...); err != nil {
|
|
|
|
|
- return response.ErrBadRequest("目标用户不是当前产品的成员")
|
|
|
|
|
- }
|
|
|
|
|
- }
|
|
|
|
|
- }
|
|
|
|
|
- if err := authHelper.CheckManageAccess(l.ctx, l.svcCtx, req.Id, productCode); err != nil {
|
|
|
|
|
- return err
|
|
|
|
|
- }
|
|
|
|
|
|
|
+ b := make([]byte, 8) // 8 字节 = 64 bits
|
|
|
|
|
+ rand.Read(b)
|
|
|
|
|
+ return hex.EncodeToString(b)[:8] // 取前 8 hex 字符 = 4 字节 = 32 bits
|
|
|
```
|
|
```
|
|
|
|
|
+- **影响**:见上一轮审计。虽然 `MustChangePassword=Yes`,但一次性密码暴力破解窗口依然存在。
|
|
|
|
|
+- **修复方案**:同上一轮方案——修正截断边界或直接把 adminPassword 调大到 16 字符(64 bits)。
|
|
|
|
|
+- **优先级**:P1
|
|
|
|
|
|
|
|
- `CheckManageAccess` 内部已经对"非超管且同一产品下非成员"做了 `checkPermLevel` 的目标成员检查(会返回 "目标用户不是当前产品的成员,无法执行管理操作")。外层这段手动检查在非超管场景下和内部检查**重复**,且多了一次 `FindOneByProductCodeUserId` DB 查询。
|
|
|
|
|
|
|
+---
|
|
|
|
|
|
|
|
- - 功能上没问题,但多一次 DB 查询且逻辑位置分散。
|
|
|
|
|
|
|
+### M-4(遗留). `/api/dept/tree` 对所有已登录用户开放,暴露完整组织架构
|
|
|
|
|
|
|
|
-- **建议**:删掉外层的重复校验,全部交给 `CheckManageAccess`。
|
|
|
|
|
|
|
+- **位置**:`internal/logic/dept/deptTreeLogic.go:27-66`
|
|
|
|
|
+- **状态**:未修复。
|
|
|
|
|
+- **建议**:仅超管可拉全量;其他用户按 `caller.DeptPath` 过滤(只返回自身部门及其子树)。
|
|
|
|
|
|
|
|
---
|
|
---
|
|
|
|
|
|
|
|
-### M-13. `UpdateUser` 自修改时能"显式传 `Status=0` + 非 nil `DeptId`"通过第一层但被 `FindOne` 后再补判
|
|
|
|
|
-
|
|
|
|
|
-- **位置**:`internal/logic/user/updateUserLogic.go:39-59`
|
|
|
|
|
-- **描述**:逻辑结构:
|
|
|
|
|
- ```go
|
|
|
|
|
- if caller.UserId == req.Id {
|
|
|
|
|
- if req.DeptId != nil || req.Status != 0 {
|
|
|
|
|
- return 403
|
|
|
|
|
- }
|
|
|
|
|
- } else {
|
|
|
|
|
- if err := CheckManageAccess(...); err != nil { return err }
|
|
|
|
|
- }
|
|
|
|
|
- user, _ := FindOne(req.Id) // 再查一次
|
|
|
|
|
- if caller.UserId != req.Id && user.IsSuperAdmin == Yes {
|
|
|
|
|
- if req.Status != 0 || req.DeptId != nil { return 403 }
|
|
|
|
|
- }
|
|
|
|
|
- ```
|
|
|
|
|
|
|
+### M-5(遗留). `ProductList` / `ProductDetail` 对所有已登录用户返回系统级元数据
|
|
|
|
|
|
|
|
- - 自修改场景下:如果传 `DeptId=&0`(指针非 nil,值为 0),第一层 `req.DeptId != nil` 直接拦下,正确。
|
|
|
|
|
- - 他人修改超管场景下:第二道防线阻断 `Status != 0 || DeptId != nil`。OK。
|
|
|
|
|
|
|
+- **位置**:`internal/logic/product/productListLogic.go`、`productDetailLogic.go`
|
|
|
|
|
+- **状态**:未修复。
|
|
|
|
|
+- **建议**:非超管只能看到自己有有效成员资格的产品。
|
|
|
|
|
|
|
|
- 但是一个隐含风险:**他人修改普通用户**场景下,如果 caller 通过 `UpdateUser` 改对方 `DeptId`,没有校验 caller 对"新目标部门"的权限。例如普通 ADMIN(部门 A)可以把用户 U 从部门 X 挪到部门 Y,哪怕 Y 不在 ADMIN 的管辖范围。`CheckManageAccess` 只校验 caller 能否管理 U(在 caller 自己的部门子树内),**不校验新 DeptId 是否合法**。
|
|
|
|
|
-
|
|
|
|
|
- 对 ADMIN 的判定:ADMIN 在 `checkDeptHierarchy` 里直接放行(第 101-103 行),所以 ADMIN 可以跨部门挪用户,这是设计意图。但 `MemberType=DEVELOPER` 或无角色的 MEMBER 不会走到这里(会被更早拦下)。
|
|
|
|
|
|
|
+---
|
|
|
|
|
|
|
|
- 结论:目前行为是 ADMIN 可跨部门分配,正常;MEMBER/DEVELOPER 不会触发这个路径。但如果未来放宽 `checkDeptHierarchy`,需要补这层目标部门校验。
|
|
|
|
|
|
|
+### M-7(遗留). `ExtractClientIP` 仅识别 `X-Real-IP`,未识别 `X-Forwarded-For`,且无可信代理白名单
|
|
|
|
|
|
|
|
-- **建议**(低优):把"变更 DeptId 时,校验目标部门在 caller 的可管理范围或 caller 是超管/ADMIN"独立出来,避免后续逻辑回归。
|
|
|
|
|
|
|
+- **位置**:`internal/middleware/ratelimitMiddleware.go:41-52`
|
|
|
|
|
+- **状态**:未修复。
|
|
|
|
|
+- **建议**:同上一轮——按"可信代理 CIDR 列表 + XFF 取最右可信值 > X-Real-IP > RemoteAddr"。
|
|
|
|
|
|
|
|
---
|
|
---
|
|
|
|
|
|
|
|
-### M-14. `setUserPermsLogic` 对已被禁用的权限直接拒绝,但缺少对"产品已被禁用"的校验
|
|
|
|
|
|
|
+### M-8(遗留). 缓存失效为 fire-and-forget,"收紧类"安全动作 + Redis 抖动 = 5 分钟内旧视图生效
|
|
|
|
|
|
|
|
-- **位置**:`internal/logic/user/setUserPermsLogic.go`
|
|
|
|
|
-- **描述**:只校验 `perm.ProductCode == productCode && perm.Status == Enabled`,没有校验该产品本身是否被禁用。结合 H-1,管理员在产品被禁用后依然能对该产品的成员设置权限。
|
|
|
|
|
-- **影响**:与 H-1 同源,一旦 H-1 修复(loadPerms 感知产品状态),这里的写入仍然合法;建议一并在管理面增加 `product.Status == Enabled` 的前置校验,作为防御纵深。
|
|
|
|
|
|
|
+- **位置**:所有 `Del/Clean/BatchDel/CleanByProduct` 的调用点。
|
|
|
|
|
+- **状态**:未修复。
|
|
|
|
|
+- **建议**:
|
|
|
|
|
+ - 对 `UpdateUserStatus(Disabled)`、`ChangePassword`、`RemoveMember`、`Logout`、`UpdateMember(Status=Disabled)` 等"收紧"类动作,缓存清理失败必须返回 5xx / 重试;
|
|
|
|
|
+ - 或者在这些路径上同步走 "DB tx 提交 → 消息队列发事件 → 消费方确保删干净",把缓存失效转为幂等补偿。
|
|
|
|
|
+ - 对新加入的 `Logout`,`Clean` 失败目前只打日志,用户虽然 DB 的 `tokenVersion+1` 生效、但缓存里旧 ud.TokenVersion 仍在——中间件对比 claim 里的旧 version vs cache 里的旧 version,**会继续放行** 5 分钟,这和 `Logout` 的"立即失效"语义严重冲突。此项建议升到 P1。
|
|
|
|
|
|
|
|
---
|
|
---
|
|
|
|
|
|
|
|
-### M-15. `BindRoles` 对"重复绑定同一角色"请求,toAdd/toRemove 都为空时直接 `return nil`,不同步缓存
|
|
|
|
|
|
|
+## 📝 低风险 / 遗留问题 (Low)
|
|
|
|
|
+
|
|
|
|
|
+### L-A. `CreateUser` 注释写"仅超管可调用",实际却允许产品 ADMIN 调用
|
|
|
|
|
|
|
|
-- **位置**:`internal/logic/user/bindRolesLogic.go:114-116`
|
|
|
|
|
-- **描述**:当请求的 `req.RoleIds` 与数据库现状完全相等时,直接 return。这是正确的优化,但它意味着:
|
|
|
|
|
- - 如果由于缓存异常,`UserDetails.Roles` 在 Redis 中是"失效但未清"的错值(例如上一次写入失败),调用 `BindRoles` 做一次"无改动 upsert"**不会**触发缓存清理。
|
|
|
|
|
-- **影响**:极低,仅在"上次 Clean 失败 + 当前调用无 diff"的联合场景下出现,属于灰度 / 降级运维场景。
|
|
|
|
|
-- **建议**:保留优化,但在 `return nil` 之前仍然做一次 `UserDetailsLoader.Clean(l.ctx, req.UserId)`,确保本次调用语义是"写读一致"。
|
|
|
|
|
|
|
+- **位置**:`internal/logic/user/createUserLogic.go:38-43`
|
|
|
|
|
+- **描述**:
|
|
|
|
|
+ ```go
|
|
|
|
|
+ // CreateUser 创建用户。新建系统用户账号,可指定部门归属。仅超管可调用。
|
|
|
|
|
+ func (l *CreateUserLogic) CreateUser(req *types.CreateUserReq) (...) {
|
|
|
|
|
+ productCode := middleware.GetProductCode(l.ctx)
|
|
|
|
|
+ if err := authHelper.RequireProductAdminFor(l.ctx, productCode); err != nil { ... }
|
|
|
|
|
+ ```
|
|
|
|
|
+ 产品 ADMIN(登录时带 productCode)可以通过该接口创建**系统级用户**(没有任何产品归属)。该用户不会自动加入任何产品,ADMIN 也只能把他加入自己这一个产品;但**文档契约与行为不一致**,容易在后续变更时导致误解。
|
|
|
|
|
+- **建议**:若意图是"超管专用",改为 `RequireSuperAdmin`;若意图是"产品 ADMIN 也可以",改注释并在 `perm.api` 的接口注释中说明。
|
|
|
|
|
|
|
|
---
|
|
---
|
|
|
|
|
|
|
|
-### M-16. `loadPerms` 普通成员分支对"用户附加 ALLOW / DENY"未过滤权限启用状态
|
|
|
|
|
|
|
+### L-B. `jwtauthMiddleware` 校验顺序:`tokenVersion` 在 `ProductStatus/MemberType` 之后
|
|
|
|
|
|
|
|
-- **位置**:`internal/loaders/userDetailsLoader.go:380-405`
|
|
|
|
|
-- **描述**:`FindPermIdsByUserIdAndEffectForProduct` 仅按 `sys_user_perm.effect + productCode` 过滤,未过滤 `sys_perm.status = 1`。最终在第 412-422 行通过 `FindByIds` + `p.Status == Enabled` 过滤已禁用的 perm code,再投入 `ud.Perms`。
|
|
|
|
|
- - 功能上正确:禁用的 perm 不会进入最终 codes。
|
|
|
|
|
- - 但**每次 Load 都白白查询禁用的 permId**,多传一趟到 `FindByIds`。
|
|
|
|
|
-- **建议**:`FindPermIdsByUserIdAndEffectForProduct` 的 SQL 加 `AND p.status = 1`(对齐 `FindRoleIdsByUserIdForProduct` 已加的 `r.status = 1`)。同时 `FindPermIdsByRoleIds` 也可以加 `INNER JOIN sys_perm p ON rp.permId = p.id AND p.status = 1`。
|
|
|
|
|
|
|
+- **位置**:`internal/middleware/jwtauthMiddleware.go:78-93`
|
|
|
|
|
+- **描述**:顺序是 `Status → ProductStatus → MemberType → TokenVersion`。当用户已 `Logout` 或 `ChangePassword`(`tokenVersion` 应失效),但产品被同步禁用了,会优先返回 "该产品已被禁用" 而不是 "登录已失效"。这会给前端错误的 UX 分支(比如前端见到 "产品禁用" 可能不会清本地 token,而是提示用户"联系管理员恢复产品";实际上该 token 已经作废了)。
|
|
|
|
|
+- **建议**:把 `tokenVersion` 检查提到最前(或仅次于 `Status != Enabled`)。这也符合 "优先拒绝无效凭据,再拒绝业务层禁用" 的错误码语义。
|
|
|
|
|
|
|
|
---
|
|
---
|
|
|
|
|
|
|
|
-## 📝 低风险 / 遗留问题 (Low)
|
|
|
|
|
-
|
|
|
|
|
-### L-1. 响应永远 HTTP 200,业务错误通过 body.code 区分
|
|
|
|
|
|
|
+### L-C. `Logout` 无并发保护,一条 token 可不断自增 `tokenVersion`
|
|
|
|
|
|
|
|
-- **位置**:`internal/response/response.go:45-52`
|
|
|
|
|
-- **描述**:所有业务错误返回 HTTP 200 + body.code=4xx/5xx。这让部分 WAF、CDN、监控工具(期望基于 HTTP 状态码告警)失效。属于已知的 API 契约选择,保留一致性即可,但建议在对外文档中明确。
|
|
|
|
|
|
|
+- **位置**:`internal/logic/auth/logoutLogic.go:30-43`
|
|
|
|
|
+- **描述**:`Logout` 虽然是幂等(多次 +1 不伤害业务),但每次都会:
|
|
|
|
|
+ 1. 发 `UPDATE sys_user tokenVersion+1`(行锁);
|
|
|
|
|
+ 2. 清所有 UserDetails 缓存。
|
|
|
|
|
+ 攻击者拿到一次有效 access token,就能反复调 `/auth/logout`——每次都是 **DB 写 + 全缓存清**,比 M-B 的 refresh 路径更轻量。
|
|
|
|
|
+- **建议**:对 `Logout` 同步加一条 "60s / 6 次" 的 userId 级限流;或者在中间件层面对"高写入路径"统一限流。
|
|
|
|
|
|
|
|
---
|
|
---
|
|
|
|
|
|
|
|
-### L-2. 敏感配置明文提交到仓库
|
|
|
|
|
|
|
+### L-D. `SyncPerms` 通过 appKey 查询,appKey 错误 vs appSecret 错误返回同一文案,但**响应时间**差异明显
|
|
|
|
|
|
|
|
-- **位置**:`etc/perm-api-*.yaml`
|
|
|
|
|
-- **描述**:MySQL/Redis 密码、AccessSecret、RefreshSecret、ManagementKey 等明文存在;即便后续轮换,历史 commit 仍可追溯。建议改用环境变量或密钥管理服务注入。
|
|
|
|
|
|
|
+- **位置**:`internal/logic/pub/syncPermsService.go:37-43`
|
|
|
|
|
+- **描述**:appKey 不存在 → 401,不走 bcrypt(快);appKey 存在但 appSecret 错 → 401,走 bcrypt(慢)。攻击者可据此枚举有效 appKey。
|
|
|
|
|
+- **影响**:低——appKey 对外不暴露,枚举难度大;但如果将来 `/api/perm/sync` 暴露在公网,这是信息泄露。
|
|
|
|
|
+- **建议**:appKey 不存在时也跑一次 dummy bcrypt,或对该接口限流。
|
|
|
|
|
|
|
|
---
|
|
---
|
|
|
|
|
|
|
|
-### L-3. `UpdateRole` 允许产品 ADMIN 任意下调 permsLevel 到 1
|
|
|
|
|
|
|
+### L-E. 保留的 `FindRoleIdsByUserId`(不按 productCode 过滤)在当前业务中几乎不可达
|
|
|
|
|
|
|
|
-- **位置**:`internal/logic/role/updateRoleLogic.go:47-48`
|
|
|
|
|
-- **描述**:产品 ADMIN 可以把一个原本 `permsLevel=500` 的角色改为 `permsLevel=1`,然后把它绑给普通 MEMBER,使其 `MinPermsLevel=1`,进而绕过 `checkPermLevel` 对"同级 MEMBER"的级别约束。由于 ADMIN 本身已是产品最高级别,这并没有扩展其能力范围;但它让普通 MEMBER 能"管理与自己同 MemberType 的更多用户"。
|
|
|
|
|
-- **建议**:要求 `permsLevel` 的修改必须是超管;或要求新 `permsLevel >= 原 permsLevel`(单调不递减)。
|
|
|
|
|
|
|
+- **位置**:`internal/model/userrole/sysUserRoleModel.go:37-44`、`internal/logic/user/userDetailLogic.go:53`
|
|
|
|
|
+- **描述**:仅在 `UserDetail` 的 `productCode == ""` 分支调用。该分支只有超管未带产品上下文时才进入(超管通过 `adminLogin` + 查用户详情页)。虽可达但使用面极窄,且返回**跨全部产品**的 roleIds,对 API 消费方语义模糊(前端只拿到 roleIds 无 productCode 区分,容易误用)。
|
|
|
|
|
+- **建议**:保留但在接口文档里明确"此分支的 roleIds 是跨产品聚合",或改为按 "caller 的产品上下文"(不等价,但更可预测)。
|
|
|
|
|
|
|
|
---
|
|
---
|
|
|
|
|
|
|
|
-### L-4. `CreateRole` 未校验 `req.ProductCode` 是否存在 / 启用
|
|
|
|
|
|
|
+### L-F. `UpdateUser` 允许他人调用者修改目标的 `deptId`,但未校验新 `deptId` 是否在调用者可管理的子树中
|
|
|
|
|
|
|
|
-- **位置**:`internal/logic/role/createRoleLogic.go:33-50`
|
|
|
|
|
-- **描述**:仅通过 `RequireProductAdminFor(req.ProductCode)` 间接校验(超管或该产品 ADMIN)。但如果超管误传 `productCode="not_exist"`,会插入一条挂在无效产品下的角色;该角色因产品不存在不会被任何人使用,但也不会报错。
|
|
|
|
|
-- **建议**:插入前 `FindOneByCode(req.ProductCode)` 校验存在且 `Status == Enabled`。
|
|
|
|
|
|
|
+- **位置**:`internal/logic/user/updateUserLogic.go:99-106`
|
|
|
|
|
+- **描述**:`CheckManageAccess` 只校验"可管理目标用户",不校验"新目标部门"在 caller 的子树中。实际上 `checkDeptHierarchy` 对 ADMIN 直接放行(第 101-103 行),但对 DEVELOPER 会先走 `callerDeptPath` 前缀匹配;DEVELOPER 如果能管理到跨部门的用户(极少数配置下),就能把他调到任意部门。由于 `CheckManageAccess` 的部门校验只限制"目标当前部门",不限制"目标的新部门",DEVELOPER 可以把下属挪出自己的子树。
|
|
|
|
|
+- **建议**:更新 `deptId` 时,新 deptId 若非空,校验 `caller.IsSuperAdmin || caller.MemberType == ADMIN || strings.HasPrefix(newDept.Path, caller.DeptPath)`。
|
|
|
|
|
|
|
|
---
|
|
---
|
|
|
|
|
|
|
|
-### L-5. `AddMember` 未校验产品启用状态
|
|
|
|
|
|
|
+### L-G. `loginService.ValidateProductLogin` 对已冻结用户的密码仍做 bcrypt 比对
|
|
|
|
|
|
|
|
-- **位置**:`internal/logic/member/addMemberLogic.go:33-34`
|
|
|
|
|
-- **描述**:只校验产品存在,不校验 `Status == Enabled`。管理员可以在产品已被禁用的情况下继续加成员。虽然被禁用的产品理论上也不应再有新成员流入,但当前不阻断。
|
|
|
|
|
-- **建议**:增加 `if product.Status != consts.StatusEnabled { return 400 "产品已被禁用" }`。
|
|
|
|
|
|
|
+- **位置**:`internal/logic/pub/loginService.go:48-54`
|
|
|
|
|
+- **描述**:Status 检查在 bcrypt 之前,但中间多了一步 `UsernameLoginLimit.Take` → 已冻结用户仍会消耗配额。累积消耗后,**真正解冻后用户 5 分钟内无法登录**。
|
|
|
|
|
+- **建议**:Status 检查前置到 FindOneByUsername 之后、`UsernameLoginLimit.Take` 之前,冻结即返回 403,不消耗配额。
|
|
|
|
|
|
|
|
---
|
|
---
|
|
|
|
|
|
|
|
-### L-6. `UserDetailLogic` 对"非超管 + productCode==='' + 查他人"的校验语义模糊
|
|
|
|
|
|
|
+### L-H. `ValidateProductLogin` 成功登录后没有重置 `UsernameLoginLimit`
|
|
|
|
|
|
|
|
-- **位置**:`internal/logic/user/userDetailLogic.go:33-43`
|
|
|
|
|
-- **描述**:逻辑上 "非超管 + 无 productCode + 查自己 → OK / 查他人 → 拒绝"。但对 ADMIN/DEVELOPER 而言,他们的 JWT productCode 不会为空;此分支只可能被"非超管 + 无 productCode"触发,当前系统里几乎只有"超管通过 adminLogin 登录"这一路径。
|
|
|
|
|
- - 换言之这段 `if caller.ProductCode == ""` 分支只对"超管自身"有意义,但条件已显式排除超管。形同死代码分支——超管走不到,其它用户也不会 `ProductCode==""`。
|
|
|
|
|
-- **建议**:要么完全删除这段分支,要么明确写成 `if !caller.IsSuperAdmin && caller.ProductCode == "" { return 401 "会话缺少产品上下文" }`,表达"产品端一定有 productCode"的不变式。
|
|
|
|
|
|
|
+- **位置**:同上
|
|
|
|
|
+- **描述**:go-zero 的 `PeriodLimit` 没有 reset API,因此"登录成功"不会归零失败计数。连续多次失败(例如输错 3 次、然后登录成功)后 24 小时内的窗口内若再输错 7 次仍会触发锁定——这对用户体验不友好,也让攻击者可以通过合法登录不打破锁定(因为锁定只和失败计数相关)。
|
|
|
|
|
+- **建议**:改用 Redis 自增 + 登录成功时 `DEL` key,或只在 `bcrypt.CompareHashAndPassword` 失败时才计数。
|
|
|
|
|
|
|
|
---
|
|
---
|
|
|
|
|
|
|
|
-### L-7. `singleflight` 在 `Load` 失败时返回零值 UserDetails 而非 nil
|
|
|
|
|
|
|
+### L-I. `AddMember` 不校验目标用户 `Status`
|
|
|
|
|
|
|
|
-- **位置**:`internal/loaders/userDetailsLoader.go:116-134`
|
|
|
|
|
-- **描述**:`sf.Do` 回调在 `loadFromDB` 返回 `ok=false` 时返回 `(nil, nil)`,外层做了兜底 `return &UserDetails{UserId, ProductCode}`。调用方靠 `ud.Username == ""` 判断"用户不存在"。
|
|
|
|
|
- - 语义上"查不到用户"和"DB 报错"无法区分;
|
|
|
|
|
- - 所有 caller 都按 `ud.Username == ""` 判定,耦合在这个不变式上。
|
|
|
|
|
-- **建议**:保留现有接口,但内部把 "DB error" 与 "不存在" 分开传递,对 5xx 让上层正确返回 500;同时在 `LoadE`(新增)接口里返回 `(*UserDetails, error)`,旧 `Load` 保持兼容。
|
|
|
|
|
|
|
+- **位置**:`internal/logic/member/addMemberLogic.go:41-43`
|
|
|
|
|
+- **描述**:只校验用户存在、不校验 `u.Status == Enabled`。可以把已冻结用户加成员;他们被解冻的瞬间就拥有产品访问权。
|
|
|
|
|
+- **建议**:`if u.Status != consts.StatusEnabled { return ErrBadRequest("用户已被冻结,无法添加为成员") }`。
|
|
|
|
|
|
|
|
---
|
|
---
|
|
|
|
|
|
|
|
-### L-8. `gRPC Login` 的 IP 提取依赖 `peer.Addr`,不识别 XFF
|
|
|
|
|
|
|
+### L-J. `UpdateUser` 允许他人修改**目标用户的 `Status` 值**,与 `UpdateUserStatus` 职责重叠
|
|
|
|
|
|
|
|
-- **位置**:`internal/server/permserver.go:60-72`
|
|
|
|
|
-- **描述**:`GrpcLoginLimiter` 的 key 用 `peer.Addr`。如果 gRPC 入口前面有 gateway/proxy,所有请求的 `peer.Addr` 都是 gateway IP,限流变成"全局 20/min"。不过一般 gRPC 不会直接对外暴露,风险低。
|
|
|
|
|
-- **建议**:若 gRPC 会走网关,应在 metadata 中带上真实客户端 IP(如 `x-real-ip`),取 metadata 作为 key。
|
|
|
|
|
|
|
+- **位置**:`internal/logic/user/updateUserLogic.go:108-125`、`internal/logic/user/updateUserStatusLogic.go`
|
|
|
|
|
+- **描述**:两条接口都能改 `Status`;但 `UpdateUser` 的路径只在"用户维度"`Clean`,而 `UpdateUserStatus` 显式通过 `UpdateStatus`(tokenVersion+1)。`UpdateUser` 里的 `UpdateProfile(statusChanged=true)` 模型代码**也** `tokenVersion+1`,行为一致,所以功能等价;但两条路径分别做校验、很容易在一侧加了新防御一侧漏掉(比如 `UpdateUserStatus` 已经禁止自改自己、不允许改超管;`UpdateUser` 只在 `caller.UserId == req.Id` 时禁 status,漏掉了"不允许把超管冻结"——实际上走 `UpdateUser` 修改超管 Status 会被第 56-60 行的"不能通过此接口修改其他超级管理员的状态和部门"拦下,OK)。
|
|
|
|
|
+- **建议**:要么把 `UpdateUser` 里处理 `Status` 的分支删掉(转嫁给 `UpdateUserStatus`),要么显式文档化两接口的角色差异,避免未来维护者再添新守则时漏一处。
|
|
|
|
|
|
|
|
---
|
|
---
|
|
|
|
|
|
|
|
-### L-9. `BindRoles` 对 `req.RoleIds = []` 的语义是"清空所有绑定",但缺少显式确认
|
|
|
|
|
|
|
+### L-K. `BindRoles` 的 caller 读取在校验之后但使用条件分散,易读性差
|
|
|
|
|
|
|
|
-- **位置**:`internal/logic/user/bindRolesLogic.go:48-58`
|
|
|
|
|
-- **描述**:传空数组时,`toAdd=[]`、`toRemove=existing`,流程会在事务里删光用户在该产品下的所有角色绑定。该语义合理(前端做全量覆盖),但没有任何二次确认或显式参数(`clearAll bool`),容易在前端误传 `[]` 时造成"误删"。
|
|
|
|
|
-- **建议**:在请求体中增加 `Intent: "replace" | "append"` 区分,或前端传 null/omitempty 时禁止清空。可选的 API 契约强化。
|
|
|
|
|
|
|
+- **位置**:`internal/logic/user/bindRolesLogic.go:65-89`
|
|
|
|
|
+- **描述**:`caller := middleware.GetUserDetails(l.ctx)` 在角色数组校验中段读取,caller 可能为 nil(中间件理论上保证非 nil,但代码仍做 `caller != nil && ...`)。这种"半防御"容易让后续维护者以为 caller 可能为 nil,加上无必要的判空。
|
|
|
|
|
+- **建议**:在函数开头统一读取 caller,确认非 nil;后续逻辑直接使用。
|
|
|
|
|
|
|
|
---
|
|
---
|
|
|
|
|
|
|
|
-### L-10. `productmember` 的 `FindOneByProductCodeUserId` 没有按 `status` 过滤
|
|
|
|
|
|
|
+### L-1 / L-2 / L-7 / L-8 / L-9 / L-10(前轮遗留)
|
|
|
|
|
|
|
|
-- **位置**:model 层(`sys_product_member` 的 cached 查询)
|
|
|
|
|
-- **描述**:`loadMembership` 在拿到 member 后校验 `member.Status == Enabled`;`bindRolesLogic.go:44`、`setUserPermsLogic.go:44`、`updateUserStatusLogic.go:53` 等业务层都只检查"是否存在成员记录",没有再检查成员状态。
|
|
|
|
|
- - `bindRoles` 给"已被禁用的成员"重新绑角色:数据上写入,但由于 `jwtauthMiddleware` 不校验 MemberType(见 H-2),会和 H-2 联动放大影响。
|
|
|
|
|
- - `setUserPerms` 同理。
|
|
|
|
|
-- **建议**:在这些业务校验处把 `FindOneByProductCodeUserId` 的结果加 `member.Status == Enabled` 判断,明确拒绝对已禁用成员的权限操作。
|
|
|
|
|
|
|
+- 与上一轮一致,此处不再赘述。保持 P3 跟进。
|
|
|
|
|
|
|
|
---
|
|
---
|
|
|
|
|
|
|
@@ -532,33 +632,40 @@
|
|
|
|
|
|
|
|
| 维度 | 评估 |
|
|
| 维度 | 评估 |
|
|
|
|------|------|
|
|
|------|------|
|
|
|
-| **逻辑一致性** | 新发现:产品禁用未联动 token 失效(H-1)、HTTP 中间件不校验禁用成员(H-2)、最后 ADMIN 保护缺失(H-4)。 |
|
|
|
|
|
-| **并发与竞态** | `BindRolePerms/DeleteRole/UpdateRole` 的"受影响用户"查询发生在事务外,存在并发缺漏(M-9)。其它关键写入已有乐观锁或 `FOR UPDATE`。 |
|
|
|
|
|
-| **资源管理** | go-zero `TransactCtx` 使用规范,`sqlx` 与 Redis 连接由池管理;未见泄漏。 |
|
|
|
|
|
-| **数据完整性** | 核心写路径(createProduct、bindRoles、bindRolePerms、removeMember、deleteRole、syncPerms)均在事务内,缓存失效为 fire-and-forget(M-8)。 |
|
|
|
|
|
-| **安全漏洞** | 产品禁用失效(H-1)、成员禁用不生效于 HTTP 路径(H-2)、管理后台账号锁定 DoS(H-3)、最后 ADMIN 失守(H-4)、产品端账号 DoS(M-6)、adminPassword 熵不足(M-3)、DeptTree / ProductList 暴露过度(M-4、M-5)。 |
|
|
|
|
|
-| **边界处理** | nil / 空串 / 可选字段(指针)处理普遍得当;`UserDetails` 零值语义仍依赖 `Username == ""` 约定(L-7)。 |
|
|
|
|
|
-| **DB 性能** | BindRoles / BindRolePerms / role 删除路径已批量化;其它列表接口采用"批量 IN + map 拼装",无 N+1。`loadPerms` 的 role/user perm 查询可加 `p.status=1` 减少无效数据(M-16)。 |
|
|
|
|
|
-| **僵尸代码** | `SysDeptModel.FindByPathPrefix` / `FindByParentId` 仅测试引用(M-11)。`Claims.Perms` 已清理、`FindRoleIdsByUserId` 仍有调用路径。 |
|
|
|
|
|
-| **接口契约与对象完整性** | `UserDetails` 缺 `ProductStatus` 字段(H-1 所需);`UpdateUserStatus` 有重复归属校验(M-12)。 |
|
|
|
|
|
|
|
+| **逻辑一致性** | 新增重大发现:`SetUserPerms` 自我越权(H-A);"最后 ADMIN" 校验 TOCTOU(M-A);`IncrementTokenVersion` 返回值与 DB 脱钩(H-B)。 |
|
|
|
|
|
+| **并发与竞态** | `DeleteRole` 仍有事务内外分裂(M-D);`Delete*ForProductTx` 的 "先 SELECT 再 DELETE" 模式非原子(M-E)。 |
|
|
|
|
|
+| **资源管理** | 无新增泄漏;`RefreshToken` / `Logout` 无限流造成 DB + Redis 热点(M-B/L-C)。 |
|
|
|
|
|
+| **数据完整性** | 关键写路径事务化;剩余问题都在"读-用-改"时序上。`UpdateMember` 的 `req.Status=0` 分支保留原值,无覆盖风险。 |
|
|
|
|
|
+| **安全漏洞** | H-A(MEMBER 自我提权全权限)属于严重权限越权;H-B 导致用户随机被登出(可用性),也降低 refresh rotation 的安全语义;M-C 仍允许账号枚举 + 选择性锁定。 |
|
|
|
|
|
+| **边界处理** | `SetUserPerms` / `BindRoles` / `RemoveMember` 对空数组、重复值的处理健壮;`UpdateUser` 指针字段的 nil/空区分明确。 |
|
|
|
|
|
+| **DB 性能** | `UserList` 冗余二次查询(M-G);`loadPerms` 已加 `p.status=1`;其他列表接口批量 IN 无 N+1。 |
|
|
|
|
|
+| **僵尸代码** | `FindByPathPrefix`/`FindByParentId` 已清理;残留 `FindRoleIdsByUserId`(L-E)。`CountActiveAdmins` SQL 硬编码常量(M-F)。 |
|
|
|
|
|
+| **接口契约** | `CreateUser` 注释 vs 实现不一致(L-A);`UpdateUser` 与 `UpdateUserStatus` 职责重叠(L-J)。 |
|
|
|
|
|
|
|
|
### 修复优先级建议
|
|
### 修复优先级建议
|
|
|
|
|
|
|
|
-1. **立即修复(P0)**
|
|
|
|
|
- - H-1 产品禁用不生效:加 `ProductStatus` 字段,`loadPerms` / `JwtAuth` / `GetUserPerms` / `VerifyToken` 统一校验,必要时在 `UpdateProduct` 递增成员的 `tokenVersion`。
|
|
|
|
|
- - H-2 中间件不校验 MemberType:与 `RefreshToken` 对齐。
|
|
|
|
|
- - H-3 AdminLogin DoS:ManagementKey 校验前置,rate limit key 加 IP 维度。
|
|
|
|
|
- - H-4 最后 ADMIN 保护:`RemoveMember` / `UpdateMember` 增加 adminCount 前置校验。
|
|
|
|
|
-
|
|
|
|
|
-2. **短期修复(P1)**
|
|
|
|
|
- - M-1 无注销接口 → 补 `/auth/logout`。
|
|
|
|
|
- - M-2 refreshToken 轮转应让旧 token 失效。
|
|
|
|
|
- - M-3 `generateRandomHex` 截断 bug + adminPassword 长度提升。
|
|
|
|
|
- - M-4 DeptTree 权限过滤。
|
|
|
|
|
- - M-5 ProductList/Detail 权限过滤。
|
|
|
|
|
- - M-6 产品登录账号锁定 DoS(与 H-3 同步)。
|
|
|
|
|
- - M-8 缓存失效原子性补偿(最高优先保障"收紧"类安全操作)。
|
|
|
|
|
- - M-9 事务外用户集查询移到事务后。
|
|
|
|
|
-
|
|
|
|
|
-3. **中期修复(P2)**
|
|
|
|
|
- - 其它 M/L 级条目(XFF 支持、LIKE 转义、僵尸代码清理、校验冗余去重、权限查询 `status=1` 过滤)。
|
|
|
|
|
|
|
+**P0(立即修复)**
|
|
|
|
|
+- **H-A**:`SetUserPerms` 加 `RequireProductAdminFor(productCode)` 或至少禁止自我 set。——普通 MEMBER 自我提权到产品全权限,属于可利用的权限越权,需优先拦截。
|
|
|
|
|
+- **H-B**:`IncrementTokenVersion` 改为事务 + `LAST_INSERT_ID` / `FOR UPDATE` 获取真实的新版本号。——否则并发刷新会让正常用户偶发"刷新后即失效",同时削弱 refresh rotation 安全性。
|
|
|
|
|
+
|
|
|
|
|
+**P1(短期修复)**
|
|
|
|
|
+- **M-A**:`RemoveMember` / `UpdateMember` 的"最后 ADMIN"校验挪进事务 + 加锁。
|
|
|
|
|
+- **M-B**:`/auth/refreshToken` 加 userId 级限流,或在 logic 内部做 Redis 计数。
|
|
|
|
|
+- **M-C**:`UsernameLoginLimit` key 增加 IP 维度,冻结账号不再消耗配额;可选加入 bcrypt 假耗时。
|
|
|
|
|
+- **M-3**:`generateRandomHex` 修截断边界 + adminPassword 长度提升到 16。
|
|
|
|
|
+- **M-4 / M-5**:`DeptTree` / `ProductList` / `ProductDetail` 增加归属过滤。
|
|
|
|
|
+- **M-8**:对"收紧类"安全动作的缓存失败策略收紧,尤其是新加入的 `Logout`(失败必须返回 5xx)。
|
|
|
|
|
+- **L-C**:`Logout` 加限流。
|
|
|
|
|
+
|
|
|
|
|
+**P2(中期修复)**
|
|
|
|
|
+- M-D:`DeleteRole` 把 `FindUserIdsByRoleId` 改为 session `FOR UPDATE`。
|
|
|
|
|
+- M-E:所有 `Delete*ForProductTx` 的 SELECT 改为 session 内执行。
|
|
|
|
|
+- M-F:`CountActiveAdmins` 用常量占位符替换硬编码。
|
|
|
|
|
+- M-G:`UserList` 合并 JOIN 与 memberType 查询,去掉二次 IN。
|
|
|
|
|
+- M-7:XFF + 可信代理白名单。
|
|
|
|
|
+- L-B:JWT 中间件调整校验顺序(tokenVersion 前置)。
|
|
|
|
|
+- L-A / L-J / L-F / L-G / L-H / L-I / L-K:按各项说明收敛。
|
|
|
|
|
+
|
|
|
|
|
+**P3(长期)**
|
|
|
|
|
+- L-1 / L-2(返回码选择、敏感配置明文)、L-7(`loader.Load` error 语义)、L-8(gRPC IP 提取)、L-9(`BindRoles` 空数组默认语义)、L-10(`FindOneByProductCodeUserId` 不按 status 过滤)保持跟进。
|
|
|
|
|
+
|