|
@@ -1,524 +1,492 @@
|
|
|
-# 权限管理系统 - 深度代码审计报告(第 4 轮)
|
|
|
|
|
|
|
+# 权限管理系统 - 深度代码审计报告(第 5 轮)
|
|
|
|
|
|
|
|
-> 审计范围:`/internal` 下全部非测试、非 `_gen.go` 生产代码(logic、loaders、model 的 custom 层、middleware、handler、server、svc、consts、response、util)+ 入口 `perm.go` + 接口定义 `perm.api`。
|
|
|
|
|
-> 审计时间:2026-04-19
|
|
|
|
|
|
|
+> 审计范围:同第 4 轮,`/internal` 下全部非测试、非 `_gen.go` 生产代码。
|
|
|
|
|
+> 审计时间:2026-04-19(复盘第 4 轮修复后再度深入)
|
|
|
> 审计重点:
|
|
> 审计重点:
|
|
|
-> - 并发场景下"最后一个 ADMIN"保护的真实可打破性(跨行 TOCTOU、事务内外数据脱钩)
|
|
|
|
|
-> - 状态变更接口的"无变化也强制递增 tokenVersion"导致的不必要踢出
|
|
|
|
|
-> - 级联删除的 TOCTOU(父部门删除 vs 子部门/用户插入)
|
|
|
|
|
-> - 高频写接口的限流盲区(changePassword 等)
|
|
|
|
|
-> - 负缓存缺失 / 缓存索引集合与数据 key 的非原子 SADD/SetEx 导致的漏清理
|
|
|
|
|
-> - 依赖 `strings.Contains(err, "1062")` 的脆弱错误分类
|
|
|
|
|
-> - 僵尸 scaffolded 中间件
|
|
|
|
|
|
|
+> - **令牌刷新链路**的原子性与重放窗口(HTTP + gRPC 两套入口并行审视)
|
|
|
|
|
+> - **垂直/水平越权**的"等级相等放行"死角
|
|
|
|
|
+> - **乐观锁以秒级 `updateTime` 为版本号**在真实同秒并发下的丢失更新
|
|
|
|
|
+> - **软删除用户 / 已删除用户的 JWT 仍有效期内**触发的 DB 重复压栈(DoS 向量)
|
|
|
|
|
+> - 缓存失效链路中残留的"N 次串行 Redis 往返"
|
|
|
|
|
+> - 上轮部分修复不彻底的遗留项(`strings.Contains(errMsg, "uk_code")`)
|
|
|
>
|
|
>
|
|
|
-> 相对上一轮:本轮新发现一批**未被上一轮覆盖**的 P0/P1 问题,都涉及生产环境真实可触发的业务逻辑/数据完整性破坏路径。
|
|
|
|
|
|
|
+> 相对第 4 轮:第 4 轮 H-1/H-2/H-3(最后一个 ADMIN)、H-5(changePassword 限流)、H-4(DeleteDept 存在性锁读)已**实际落地修复**(本轮代码阅读已确认)。本轮新暴露一组围绕**刷新令牌重放**和**等级相等分配**的高危问题,以及若干性能 / 健壮性缺口。
|
|
|
|
|
|
|
|
---
|
|
---
|
|
|
|
|
|
|
|
## 🚩 核心逻辑漏洞 (High Risk)
|
|
## 🚩 核心逻辑漏洞 (High Risk)
|
|
|
|
|
|
|
|
-### H-1. `UpdateMember` 的"最后一个 ADMIN"保护只看 `memberType` 变化,不看 `status` 变化 → **可把最后一个 ADMIN 直接禁用**(产品瞬间"无人管理")
|
|
|
|
|
|
|
+### H-1. `RefreshToken` 先校验 `tokenVersion` 再递增,**并发刷新可被第三方"接管会话"**(HTTP + gRPC 双入口)
|
|
|
|
|
|
|
|
-- **位置**:`internal/logic/member/updateMemberLogic.go:51,54-59,66-73`
|
|
|
|
|
|
|
+- **位置**:
|
|
|
|
|
+ - `internal/logic/pub/refreshTokenLogic.go:64-79`
|
|
|
|
|
+ - `internal/server/permserver.go`(同逻辑的 gRPC `RefreshToken` 实现)
|
|
|
- **描述**:
|
|
- **描述**:
|
|
|
|
|
+ 核心代码片段(HTTP 版本):
|
|
|
```go
|
|
```go
|
|
|
- needAdminCheck := member.MemberType == consts.MemberTypeAdmin && req.MemberType != consts.MemberTypeAdmin
|
|
|
|
|
-
|
|
|
|
|
- member.MemberType = req.MemberType
|
|
|
|
|
- if req.Status != 0 {
|
|
|
|
|
- if req.Status != consts.StatusEnabled && req.Status != consts.StatusDisabled {
|
|
|
|
|
- return response.ErrBadRequest("状态值无效...")
|
|
|
|
|
- }
|
|
|
|
|
- member.Status = req.Status
|
|
|
|
|
|
|
+ if claims.TokenVersion != ud.TokenVersion {
|
|
|
|
|
+ return nil, response.ErrUnauthorized("登录状态已失效,请重新登录")
|
|
|
}
|
|
}
|
|
|
- ...
|
|
|
|
|
- if needAdminCheck {
|
|
|
|
|
- adminCount, _ := ...CountActiveAdminsTx(...)
|
|
|
|
|
- if adminCount <= 1 {
|
|
|
|
|
- return response.ErrBadRequest("不能降级该产品的最后一个管理员")
|
|
|
|
|
- }
|
|
|
|
|
|
|
+ if l.svcCtx.TokenOpLimiter != nil {
|
|
|
|
|
+ code, _ := l.svcCtx.TokenOpLimiter.Take(fmt.Sprintf("refresh:%d", claims.UserId))
|
|
|
|
|
+ ...
|
|
|
}
|
|
}
|
|
|
|
|
+ newVersion, err := l.svcCtx.SysUserModel.IncrementTokenVersion(l.ctx, claims.UserId)
|
|
|
```
|
|
```
|
|
|
- `needAdminCheck` 只在 **memberType 从 ADMIN 改成非 ADMIN** 时为 `true`。如果请求保持 `memberType=ADMIN` 但 **`status` 改为 `StatusDisabled`**,`needAdminCheck=false`,直接跳过计数检查。`CountActiveAdminsTx` 只计 `status=1` 的 ADMIN,所以"禁用最后一个 ADMIN"是在绕过该保护之下合法完成的。
|
|
|
|
|
|
|
|
|
|
- 攻击/误操作路径(任何持有该产品 ADMIN 令牌的账号都可做):
|
|
|
|
|
|
|
+ "check 版本号 → 递增版本号"是**两条独立的 SQL**,中间没有行级锁、也没有条件更新语义。DB 里的 `IncrementTokenVersion` 实际是无条件 `UPDATE ... SET tokenVersion = tokenVersion+1`。
|
|
|
|
|
|
|
|
- ```http
|
|
|
|
|
- POST /api/member/update
|
|
|
|
|
- { "id": <最后一个 ADMIN 的 member 记录 id>,
|
|
|
|
|
- "memberType": "ADMIN",
|
|
|
|
|
- "status": 2 }
|
|
|
|
|
- ```
|
|
|
|
|
|
|
+ **攻击 / 真实泄露场景**:
|
|
|
|
|
+ 1. 攻击者通过设备失窃 / 前端 XSS / localStorage 泄露,拿到受害者一枚**仍在有效期内的 refreshToken**(claims.TokenVersion = V)。单会话轮转的前提:一旦合法用户刷新过一次,旧 token 就作废。这是标准 OAuth2 refresh token rotation 的基线安全属性。
|
|
|
|
|
+ 2. 受害者在某时刻 T 发起合法刷新;同一秒或几毫秒内攻击者也用窃到的 token 发起刷新。
|
|
|
|
|
+ 3. 两个请求分别读到 `ud.TokenVersion = V`,双方都通过 `claims.TokenVersion == ud.TokenVersion` 这一步 → 都进入 `IncrementTokenVersion`。
|
|
|
|
|
+ 4. DB tokenVersion 经两次递增变成 `V+2`。
|
|
|
|
|
+ 5. "最后完成的请求"会得到 `newVersion = V+2`,并以此签发新 accessToken/refreshToken。"先完成的请求"拿到的 `newVersion = V+1`,用户端使用它发起后续业务请求时 Middleware 判定 `V+1 != V+2 → 登录失效`,直接被踢。
|
|
|
|
|
+ 6. **结果**:合法用户被登出;攻击者持有 V+2 的 accessToken 和 refreshToken,静默拿走后续完整会话(并且在攻击者拿到的 refreshToken 自然过期前,可以一直续期、一直维持会话)。
|
|
|
|
|
|
|
|
- 执行结果:DB 里这条 ADMIN 的 `status=2`,`CountActiveAdmins==0`。此后:
|
|
|
|
|
- - 该产品下无人能通过 `RequireProductAdminFor` 的 Admin 校验(除了超管);
|
|
|
|
|
- - 所有需要 ADMIN 的接口(CreateRole/BindRolePerms/SetUserPerms/AddMember/…)都失锁,只能走超管通道;
|
|
|
|
|
- - 该产品上的 ADMIN 自助救援路径不存在,需要联系平台管理员。
|
|
|
|
|
|
|
+ 换句话说,refresh token rotation 的"旧 token 必须在发新 token 的同一瞬间失效"这一原子性前提被打破。在**攻击者 + 合法用户并发一次**的窗口下,会话被直接接管。
|
|
|
|
|
|
|
|
- **影响**:
|
|
- **影响**:
|
|
|
- - 直接破坏"产品始终保留至少一个可用 ADMIN"的业务不变式;
|
|
|
|
|
- - 非超管的恶意 ADMIN 可以"离职前锁死产品";
|
|
|
|
|
- - 普通运维失误也可能造成同样结果,且没有任何回滚机制。
|
|
|
|
|
|
|
+ - 这不是普通的时序抖动,是**会话劫持** / **账号被盗**级别的 P0 问题。
|
|
|
|
|
+ - gRPC 版本因为**根本没有限流**(见 H-2),攻击者可以程序化拉高并发,几乎必然落到 race 窗口里。
|
|
|
|
|
+ - 更恶劣的是:受害者前端只会看到"登录状态已失效"一次,下次重新登录即可,几乎没有任何异常信号可以让风控察觉。
|
|
|
|
|
|
|
|
- **修复方案**:
|
|
- **修复方案**:
|
|
|
- 把 `needAdminCheck` 扩展为"任何会让这条 ADMIN 记录变为非活跃的写入":
|
|
|
|
|
-
|
|
|
|
|
|
|
+ 把 check + increment 合并成**单条带版本条件的原子更新**:
|
|
|
```go
|
|
```go
|
|
|
- // internal/logic/member/updateMemberLogic.go
|
|
|
|
|
- nextType := req.MemberType
|
|
|
|
|
- nextStatus := member.Status
|
|
|
|
|
- if req.Status != 0 {
|
|
|
|
|
- nextStatus = req.Status
|
|
|
|
|
|
|
+ // 新增 model 方法
|
|
|
|
|
+ func (m *customSysUserModel) IncrementTokenVersionIfMatch(ctx context.Context, id, expected int64) (int64, error) {
|
|
|
|
|
+ var newVersion int64
|
|
|
|
|
+ err := m.TransactCtx(ctx, func(ctx context.Context, session sqlx.Session) error {
|
|
|
|
|
+ q := fmt.Sprintf("UPDATE %s SET `tokenVersion`=LAST_INSERT_ID(`tokenVersion`+1), `updateTime`=? WHERE `id`=? AND `tokenVersion`=?", m.table)
|
|
|
|
|
+ res, err := session.ExecCtx(ctx, q, time.Now().Unix(), id, expected)
|
|
|
|
|
+ if err != nil { return err }
|
|
|
|
|
+ affected, _ := res.RowsAffected()
|
|
|
|
|
+ if affected == 0 { return ErrTokenVersionMismatch }
|
|
|
|
|
+ return session.QueryRowCtx(ctx, &newVersion, "SELECT LAST_INSERT_ID()")
|
|
|
|
|
+ })
|
|
|
|
|
+ ...
|
|
|
|
|
+ }
|
|
|
|
|
+ ```
|
|
|
|
|
+ logic 层改为:
|
|
|
|
|
+ ```go
|
|
|
|
|
+ newVersion, err := l.svcCtx.SysUserModel.IncrementTokenVersionIfMatch(l.ctx, claims.UserId, claims.TokenVersion)
|
|
|
|
|
+ if errors.Is(err, userModel.ErrTokenVersionMismatch) {
|
|
|
|
|
+ return nil, response.ErrUnauthorized("登录状态已失效,请重新登录")
|
|
|
}
|
|
}
|
|
|
-
|
|
|
|
|
- willBecomeInactiveAdmin :=
|
|
|
|
|
- member.MemberType == consts.MemberTypeAdmin &&
|
|
|
|
|
- member.Status == consts.StatusEnabled &&
|
|
|
|
|
- (nextType != consts.MemberTypeAdmin || nextStatus != consts.StatusEnabled)
|
|
|
|
|
```
|
|
```
|
|
|
- 并把后续赋值/校验改成基于 `nextType`/`nextStatus`。同时建议把 `needAdminCheck`/"最后一个 ADMIN" 同时在 `RemoveMember` 和 `UpdateMember` 里抽成一个 helper `guardLastAdminTx(session, productCode, excludingId)`,统一用 **行锁 + FOR UPDATE 遍历** 方式做。参考 H-3 的修复方案。
|
|
|
|
|
|
|
+ 这样两个并发请求只有一个能 `WHERE tokenVersion = V` 命中(`affected=1`),另一个 `affected=0`,明确失败并返回 401。HTTP 与 gRPC 两个入口**必须共享这一条原子更新逻辑**,不能只修一边。
|
|
|
|
|
|
|
|
---
|
|
---
|
|
|
|
|
|
|
|
-### H-2. `RemoveMember` 在"是不是 ADMIN"判断上使用了**事务外读到的** `member.MemberType`,而把事务内 `FindOneForUpdateTx` 的返回值扔掉 → 并发下最后一个 ADMIN 会被删掉
|
|
|
|
|
|
|
+### H-2. gRPC `RefreshToken` / `VerifyToken` **完全没有任何限流**
|
|
|
|
|
|
|
|
-- **位置**:`internal/logic/member/removeMemberLogic.go:32,42-53`
|
|
|
|
|
|
|
+- **位置**:`internal/server/permserver.go`(gRPC `RefreshToken`、`VerifyToken` RPC 方法)
|
|
|
- **描述**:
|
|
- **描述**:
|
|
|
- ```go
|
|
|
|
|
- member, err := l.svcCtx.SysProductMemberModel.FindOne(l.ctx, req.Id) // 事务外
|
|
|
|
|
- ...
|
|
|
|
|
- if err := l.svcCtx.SysProductMemberModel.TransactCtx(l.ctx, func(ctx context.Context, session sqlx.Session) error {
|
|
|
|
|
- if _, err := l.svcCtx.SysProductMemberModel.FindOneForUpdateTx(ctx, session, req.Id); err != nil { // <-- 返回值被丢弃
|
|
|
|
|
- return response.ErrNotFound("成员不存在")
|
|
|
|
|
- }
|
|
|
|
|
- if member.MemberType == consts.MemberTypeAdmin { // <-- 用的是事务外的 member
|
|
|
|
|
- adminCount, err := ...CountActiveAdminsTx(...)
|
|
|
|
|
- ...
|
|
|
|
|
- }
|
|
|
|
|
- ...
|
|
|
|
|
- })
|
|
|
|
|
- ```
|
|
|
|
|
- `FindOneForUpdateTx` 仅用来加行锁,但返回的最新值**直接丢掉**,后续判断"这条是不是 ADMIN"仍然走事务外的 `member`。
|
|
|
|
|
-
|
|
|
|
|
- 攻击/误操作路径:
|
|
|
|
|
- - 初始:`productA` 有 ADMIN = [A1, A2];`M` 是 MEMBER。
|
|
|
|
|
- - `T1`:调用 `RemoveMember(member.Id = M)`:
|
|
|
|
|
- - 事务外 FindOne → `member.MemberType = MEMBER`。
|
|
|
|
|
- - `T2` 几乎同时:`UpdateMember` 把 `M` 提升成 ADMIN(事务提交)。
|
|
|
|
|
- - `T1`:进入事务,`FindOneForUpdateTx(M)` 返回 ADMIN(被丢弃)。
|
|
|
|
|
- - `T1`:`if member.MemberType == ADMIN` 用的是旧 MEMBER → **跳过 count 检查**。
|
|
|
|
|
- - `T1`:删除 `M`。
|
|
|
|
|
- - 如果 T2 是因为 A1/A2 之一被先移除而把 `M` 提到 ADMIN 补位(运营流程),T1 就在恢复期内删掉了新晋 ADMIN;极端下可导致 0 active admin。
|
|
|
|
|
|
|
+ HTTP 端的 `/api/auth/refreshToken` 至少在**解析成功 claims 之后**调了 `TokenOpLimiter.Take("refresh:%d")`,做了一层**按用户**的令牌桶限流。但 gRPC 服务同名 RPC 完全没有做这件事:
|
|
|
|
|
+ - 没有 gRPC interceptor 级别的 IP 限流;
|
|
|
|
|
+ - 没有 per-user 限流;
|
|
|
|
|
+ - 也没有 per-refresh-secret 全局限流。
|
|
|
|
|
|
|
|
- 即使不考虑跨线程竞态,这也违背了"同一事务内一次读到一次写之间必须**用事务内的**视图来决策"的原则。
|
|
|
|
|
|
|
+ 业务语义上 gRPC 是内网其他服务向权限中心"换 accessToken / 验证 token"的主链路,**本就应该是最需要限流的地方**(被错误部署 / 服务腔体被打穿时,可以直接把 DB 打爆)。
|
|
|
|
|
|
|
|
-- **影响**:与 H-1 同类,破坏 "至少一个 ADMIN" 不变式。
|
|
|
|
|
|
|
+- **影响**:
|
|
|
|
|
+ - 与 H-1 组合时:攻击者可在 gRPC 通道上对 `RefreshToken` 发起**任意并发**,几乎必然命中 H-1 的 race 窗口,把会话劫持概率从"需要运气"拉到"只要有网络带宽"。
|
|
|
|
|
+ - 即便没有 H-1:攻击者用一枚尚未过期的 refreshToken 可以无限换取 accessToken,作为**持续化 RCE 工具链的身份通道**;也可以对有效 token 进行大量 signature verification,把权限中心 CPU 打满。
|
|
|
|
|
+ - `VerifyToken` 无限流:任何下游被攻破后,可以对权限中心做 token-oracle 爆破。
|
|
|
|
|
|
|
|
- **修复方案**:
|
|
- **修复方案**:
|
|
|
-
|
|
|
|
|
- ```go
|
|
|
|
|
- if err := l.svcCtx.SysProductMemberModel.TransactCtx(l.ctx, func(ctx context.Context, session sqlx.Session) error {
|
|
|
|
|
- locked, err := l.svcCtx.SysProductMemberModel.FindOneForUpdateTx(ctx, session, req.Id)
|
|
|
|
|
- if err != nil {
|
|
|
|
|
- return response.ErrNotFound("成员不存在")
|
|
|
|
|
- }
|
|
|
|
|
- if locked.MemberType == consts.MemberTypeAdmin && locked.Status == consts.StatusEnabled {
|
|
|
|
|
- // 只有当前是"活跃 ADMIN"才需要计数
|
|
|
|
|
- adminCount, err := ...CountActiveAdminsTx(...)
|
|
|
|
|
- if err != nil {
|
|
|
|
|
- return err
|
|
|
|
|
- }
|
|
|
|
|
- if adminCount <= 1 {
|
|
|
|
|
- return response.ErrBadRequest("不能移除该产品的最后一个管理员")
|
|
|
|
|
- }
|
|
|
|
|
- }
|
|
|
|
|
- ...
|
|
|
|
|
- })
|
|
|
|
|
- ```
|
|
|
|
|
- 外层的 `member` 只用来取 `UserId`/`ProductCode` 做 `CheckManageAccess` 和事后清缓存。
|
|
|
|
|
|
|
+ 1. **强制**添加 gRPC unary interceptor,做 IP 粒度的 PeriodLimit(`peer.FromContext` 取对端 IP,失败时按 H-4 的做法 fail-close):
|
|
|
|
|
+ ```go
|
|
|
|
|
+ func GrpcRateLimitInterceptor(limiter *limit.PeriodLimit, quota int, window int) grpc.UnaryServerInterceptor {
|
|
|
|
|
+ return func(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (interface{}, error) {
|
|
|
|
|
+ ip, err := extractClientIP(ctx)
|
|
|
|
|
+ if err != nil {
|
|
|
|
|
+ return nil, status.Error(codes.Unavailable, "peer not identifiable")
|
|
|
|
|
+ }
|
|
|
|
|
+ code, _ := limiter.Take(fmt.Sprintf("grpc:%s:%s", info.FullMethod, ip))
|
|
|
|
|
+ if code == limit.OverQuota {
|
|
|
|
|
+ return nil, status.Error(codes.ResourceExhausted, "rate limited")
|
|
|
|
|
+ }
|
|
|
|
|
+ return handler(ctx, req, info)
|
|
|
|
|
+ }
|
|
|
|
|
+ }
|
|
|
|
|
+ ```
|
|
|
|
|
+ 2. 在 `RefreshToken` / `VerifyToken` 逻辑内部**也加一层 per-user 的 `TokenOpLimiter.Take(fmt.Sprintf("grpc-refresh:%d", claims.UserId))`**,作为 claims 解析成功后的第二道闸。
|
|
|
|
|
+ 3. 把 refresh / verify 的失败结果(`claims.TokenVersion != ud.TokenVersion`、`jwt.ParseWithClaims err`)接入 **IP-level 失败计数器**,连续失败 N 次直接封禁一段时间,避免爆破 refresh secret。
|
|
|
|
|
|
|
|
---
|
|
---
|
|
|
|
|
|
|
|
-### H-3. "最后一个 ADMIN" 保护基于 `SELECT COUNT(*)` 的快照读 → 并发移除/降级两个不同 ADMIN 会同时通过检查(跨行 TOCTOU,业务不变式被打破)
|
|
|
|
|
-
|
|
|
|
|
-- **位置**:
|
|
|
|
|
- - `internal/model/productmember/sysProductMemberModel.go:62-69` (`CountActiveAdminsTx`)
|
|
|
|
|
- - `internal/logic/member/updateMemberLogic.go:66-73`
|
|
|
|
|
- - `internal/logic/member/removeMemberLogic.go:45-52`
|
|
|
|
|
|
|
+### H-3. `BindRoles` 允许**平级自增**:MEMBER 可给其他用户绑定与自己**最小等级相等**的角色,从而让目标等同自己 → 后续再也管不动
|
|
|
|
|
|
|
|
|
|
+- **位置**:`internal/logic/user/bindRolesLogic.go:86-91`
|
|
|
- **描述**:
|
|
- **描述**:
|
|
|
```go
|
|
```go
|
|
|
- func (m *customSysProductMemberModel) CountActiveAdminsTx(ctx context.Context, session sqlx.Session, productCode string) (int64, error) {
|
|
|
|
|
- var count int64
|
|
|
|
|
- query := fmt.Sprintf("SELECT COUNT(*) FROM %s WHERE `productCode` = ? AND `memberType` = ? AND `status` = ?", m.table)
|
|
|
|
|
- if err := session.QueryRowCtx(ctx, &count, query, productCode, consts.MemberTypeAdmin, consts.StatusEnabled); err != nil {
|
|
|
|
|
- return 0, err
|
|
|
|
|
|
|
+ if !authHelper.HasFullProductPerms(caller) {
|
|
|
|
|
+ if caller.MinPermsLevel == math.MaxInt64 || r.PermsLevel < caller.MinPermsLevel {
|
|
|
|
|
+ return response.ErrForbidden("不能分配权限级别高于自身的角色")
|
|
|
}
|
|
}
|
|
|
- return count, nil
|
|
|
|
|
}
|
|
}
|
|
|
```
|
|
```
|
|
|
- - 非锁定读(`session.QueryRowCtx` 不附带 `FOR UPDATE` / `LOCK IN SHARE MODE`)。在 MySQL InnoDB 默认 `REPEATABLE READ` 下,这是 MVCC 快照读,**不会**阻塞也**不会**被其他事务的 INSERT/UPDATE 阻塞。
|
|
|
|
|
- - 事务 T1 锁定并计划降级/移除 ADMIN `m1`;事务 T2 几乎同时锁定并计划降级/移除 ADMIN `m2`(`m1 ≠ m2`,所以行锁不互斥)。
|
|
|
|
|
- - 两个事务同时做 `CountActiveAdmins`,都读到 snapshot=2,都通过 `<=1` 检查,都 commit → 活跃 ADMIN 数变成 0。
|
|
|
|
|
-
|
|
|
|
|
- 这是一个真实存在的**跨行 TOCTOU**,经典情景:
|
|
|
|
|
- - 两位超管同时对不同 ADMIN 做"降级为 DEVELOPER";
|
|
|
|
|
- - 一个 ADMIN 并行点了"退出产品"(自己 RemoveMember)和"降级自己为 MEMBER";
|
|
|
|
|
- - 批处理脚本 + 人工操作的并发。
|
|
|
|
|
|
|
+ 权限模型约定:`PermsLevel` 数字越小表示权限越高。这里的判定是 `r.PermsLevel < caller.MinPermsLevel`,即 **严格小于** 才拒绝。结果:调用者 `MinPermsLevel = 5` 时允许分配 `PermsLevel = 5` 的角色。
|
|
|
|
|
|
|
|
-- **影响**:与 H-1/H-2 累积。"至少一个 ADMIN"是系统的关键不变式,它被三条独立路径共同破坏:
|
|
|
|
|
- - H-1:通过 disable 绕过;
|
|
|
|
|
- - H-2:通过事务内外视图不一致绕过;
|
|
|
|
|
- - H-3:通过快照读+跨行并发绕过。
|
|
|
|
|
|
|
+ 但 `CheckManageAccess` 里 `checkPermLevel` 的判定是:
|
|
|
|
|
+ ```go
|
|
|
|
|
+ if caller.MinPermsLevel >= targetLevel {
|
|
|
|
|
+ return response.ErrForbidden("无权管理权限级别高于或等于您的用户")
|
|
|
|
|
+ }
|
|
|
|
|
+ ```
|
|
|
|
|
+ 即目标用户的最小 PermsLevel **必须严格大于**调用者(即目标权限严格低于调用者)。两边判定口径不一致:
|
|
|
|
|
|
|
|
-- **修复方案**(推荐方案 B):
|
|
|
|
|
|
|
+ - MEMBER A(`MinPermsLevel=5`)调用 `/api/user/bindRoles` 给属于自己部门子树的 MEMBER B(`MinPermsLevel=6`)追加一个 `PermsLevel=5` 的角色 → 通过。
|
|
|
|
|
+ - 此后 B 的 `MinPermsLevel=5`,与 A 平级;A 再想 `UpdateUser/BindRoles/SetUserPerms/UpdateUserStatus` 管理 B,都会在 `checkPermLevel` 里被 `5 >= 5` 拦住 → 管不了了。
|
|
|
|
|
+ - 也就是说 A **合法地**把下属 B 提到自己平级,然后永久失去对 B 的后续管控权。如果此时 A 自己被冻结 / 账号被挤下线,B 也不能被原 A 的同级 MEMBER 回收——要么超管 / 产品 ADMIN 出手,要么永久残留。
|
|
|
|
|
+ - 同样的 gap 在 `CheckMemberTypeAssignment`(`>=` 拦截)与此处(`<` 拦截)之间是不一致的:前者严格高一级才能分配 memberType,这里却允许"同级角色"。
|
|
|
|
|
|
|
|
- **方案 A(局部:对所有活跃 ADMIN 加共享锁)**:
|
|
|
|
|
|
|
+- **影响**:
|
|
|
|
|
+ - **垂直权限泄露**:即便没有恶意,普通配置错误就能制造出"产品里多个同级 owner,互相管不了对方"的死结,业务上得**靠人肉联系平台超管**收拾残局。
|
|
|
|
|
+ - 有恶意的 MEMBER 可以**主动把攻击者账号拉到自己平级**,从此攻击者的权限只能由超管吊销 → **实际上完成了一次越权提升**,攻击者绕过了"MEMBER 只能管下级"的心智模型。
|
|
|
|
|
+ - 与 `UpdateRoleLogic` 的"非超管不能降低 PermsLevel"结合:A 把 B 拉到平级后,如果再找一个 ADMIN 去调整角色 PermsLevel,整个产品里的等级模型会越发混乱。
|
|
|
|
|
|
|
|
|
|
+- **修复方案**:
|
|
|
|
|
+ 把 `<` 改为 `<=`,与 `checkPermLevel` 的 `>=` 对称:
|
|
|
```go
|
|
```go
|
|
|
- func (m *customSysProductMemberModel) LockActiveAdminsTx(ctx context.Context, session sqlx.Session, productCode string) (int64, error) {
|
|
|
|
|
- var ids []int64
|
|
|
|
|
- // 锁定该产品下所有当前活跃的 ADMIN 行;任何其它事务想把这些行变更,都必须等我
|
|
|
|
|
- q := fmt.Sprintf(
|
|
|
|
|
- "SELECT `id` FROM %s WHERE `productCode`=? AND `memberType`=? AND `status`=? FOR UPDATE",
|
|
|
|
|
- m.table)
|
|
|
|
|
- if err := session.QueryRowsCtx(ctx, &ids, q, productCode, consts.MemberTypeAdmin, consts.StatusEnabled); err != nil {
|
|
|
|
|
- return 0, err
|
|
|
|
|
- }
|
|
|
|
|
- return int64(len(ids)), nil
|
|
|
|
|
|
|
+ if caller.MinPermsLevel == math.MaxInt64 || r.PermsLevel <= caller.MinPermsLevel {
|
|
|
|
|
+ return response.ErrForbidden("不能分配权限级别高于或等于您自身的角色")
|
|
|
}
|
|
}
|
|
|
```
|
|
```
|
|
|
- 在所有会让一条 ADMIN 变为非活跃的路径(`UpdateMember` 的 disable/降级 分支、`RemoveMember`)上:**先**调用 `LockActiveAdminsTx`,再判断 `count <= 1`。这样两个并发事务都会尝试锁定 **相同的一组 ADMIN 行**,其中一个会被阻塞;赢的那个先计数 → 允许或拒绝 → 提交后释放锁;输的那个获得锁后再看到准确的 count-1。
|
|
|
|
|
-
|
|
|
|
|
- **方案 B(更稳:对产品行做粗粒度互斥)**:
|
|
|
|
|
- 在所有"可能影响 ADMIN 数量"的事务入口先 `SELECT ... FOR UPDATE` 产品行(或建一张 `sys_product_mutex` 表),让对同一产品的 ADMIN 集合变更天然串行化。业务上这些操作并发率极低(手动运营动作),粗粒度锁不会成为性能瓶颈。
|
|
|
|
|
|
|
+ 同时:
|
|
|
|
|
+ 1. 在 `BindRoles` / `SetUserPerms` 所在文件里**抽一个 `guardRoleLevelAssignable(caller, role)` helper**,强约束"只能分配严格更低等级的角色"——与 `checkPermLevel`(严格更低等级才能管)对齐,后续任何新增绑定场景不会再走错。
|
|
|
|
|
+ 2. 补一条单测:模拟 MEMBER (level=5) 给 MEMBER 绑 level=5 的角色,必须 403。
|
|
|
|
|
|
|
|
---
|
|
---
|
|
|
|
|
|
|
|
-### H-4. `DeleteDept` 子部门/用户计数用非锁定快照读 → `CreateDept` / `CreateUser` / `UpdateUser` 能在 Delete 事务中"偷偷插入"→ 产生孤儿引用
|
|
|
|
|
|
|
+### H-4. `UpdateUserLogic` 的 `DeptId = 0` 分支**跳过部门管辖校验**:下属可以被合法"移出部门"、失去上级管辖
|
|
|
|
|
|
|
|
-- **位置**:`internal/logic/dept/deleteDeptLogic.go:36-63`
|
|
|
|
|
|
|
+- **位置**:`internal/logic/user/updateUserLogic.go:106-120`
|
|
|
- **描述**:
|
|
- **描述**:
|
|
|
```go
|
|
```go
|
|
|
- return l.svcCtx.SysDeptModel.TransactCtx(l.ctx, func(ctx context.Context, session sqlx.Session) error {
|
|
|
|
|
- // 仅锁定要删的那一行
|
|
|
|
|
- lockQuery := fmt.Sprintf("SELECT `id` FROM %s WHERE `id` = ? FOR UPDATE", ...)
|
|
|
|
|
- ...
|
|
|
|
|
- var childCount int64
|
|
|
|
|
- countChildQuery := "SELECT COUNT(*) FROM sys_dept WHERE parentId = ?"
|
|
|
|
|
- session.QueryRowCtx(..., countChildQuery, req.Id) // 快照读,无锁
|
|
|
|
|
-
|
|
|
|
|
- var userCount int64
|
|
|
|
|
- countUserQuery := "SELECT COUNT(*) FROM sys_user WHERE deptId = ?"
|
|
|
|
|
- session.QueryRowCtx(..., countUserQuery, req.Id) // 快照读,无锁
|
|
|
|
|
-
|
|
|
|
|
- return l.svcCtx.SysDeptModel.DeleteWithTx(...)
|
|
|
|
|
- })
|
|
|
|
|
|
|
+ if req.DeptId != nil {
|
|
|
|
|
+ if *req.DeptId > 0 {
|
|
|
|
|
+ newDept, err := l.svcCtx.SysDeptModel.FindOne(l.ctx, *req.DeptId)
|
|
|
|
|
+ ...
|
|
|
|
|
+ if !caller.IsSuperAdmin && caller.MemberType != consts.MemberTypeAdmin &&
|
|
|
|
|
+ caller.DeptPath != "" &&
|
|
|
|
|
+ !strings.HasPrefix(newDept.Path, caller.DeptPath) {
|
|
|
|
|
+ return response.ErrForbidden("无权将用户调入非自己管辖的部门")
|
|
|
|
|
+ }
|
|
|
|
|
+ }
|
|
|
|
|
+ deptId = *req.DeptId
|
|
|
|
|
+ }
|
|
|
```
|
|
```
|
|
|
- - 父部门加了 `FOR UPDATE`,但这只是对 `sys_dept.id=X` 这一行的 X 锁。
|
|
|
|
|
- - 子部门计数走 `sys_dept.parentId=X`。父部门行 X 锁**不会**传递给"引用这个父"的子行,也**不会**在 `parentId` 索引上建立 gap lock(因为 COUNT(*) 不是锁定读)。
|
|
|
|
|
- - 用户计数更离谱:`sys_user.deptId=X` 分属另一张表,跟 `sys_dept` 的行锁没有任何关系。
|
|
|
|
|
- - `CreateDept`(`internal/logic/dept/createDeptLogic.go:46-52`)在事务外 `FindOne` 读父部门,**不加任何锁**,随后 InsertWithTx 写子行。同样 `CreateUser`、`UpdateUser`(改 deptId)都不会锁父部门。
|
|
|
|
|
-
|
|
|
|
|
- 结果:
|
|
|
|
|
- - T1 开始 Delete dept=5:父行加锁,childCount=0、userCount=0,通过。
|
|
|
|
|
- - T2 `CreateDept(parentId=5)`:不需要任何锁,Insert 成功,commit。
|
|
|
|
|
- - T1:DELETE parent,commit。
|
|
|
|
|
- - 数据库里有 `sys_dept` 子行挂在一个**已不存在**的 parentId=5 → `DeptTreeLogic` 的 "parent 不存在则视为 root" 兜底 (`deptTreeLogic.go:60-62`) 只是把异常用户吞掉,组织架构彻底错乱。
|
|
|
|
|
- - 对 `sys_user` 同理:可能生产出 `deptId=5` 但 dept 已删的孤儿用户,`loadDept`→`FindOne` 会失败静默,`DeptPath=""`,进而触发 `CheckManageAccess → checkDeptHierarchy` 的"您的部门信息异常"分支,用户相当于完全失去权限。
|
|
|
|
|
|
|
+ 新部门层级校验放在 `*req.DeptId > 0` 分支内。如果调用者传 `deptId: 0`,直接 `deptId = 0`,**跳过整个层级校验**。由 `access.go:146-150` 可知,`target.DeptId == 0` 会被 `checkDeptHierarchy` 视为"仅超管或产品 ADMIN 可管"。
|
|
|
|
|
+
|
|
|
|
|
+ 真实业务路径:
|
|
|
|
|
+ 1. MEMBER A(部门 `/1/2/`,通过 `CheckManageAccess` 管辖部门 `/1/2/3/` 下的 MEMBER B)。
|
|
|
|
|
+ 2. A 调用 `POST /api/user/update { id: B.id, deptId: 0 }`。第一步校验:A 对 B 有管辖权限(通过,B 确在 A 子树)。后续:`req.DeptId != nil && *req.DeptId == 0` → 跳过新部门 Path 校验 → `deptId = 0`。
|
|
|
|
|
+ 3. DB 里 B 的 `deptId` 被抹成 0。此后 B 的 `DeptPath` 为空,`checkDeptHierarchy` 对任何非 ADMIN 都返回"目标用户未归属部门,仅超管或产品管理员可管"。
|
|
|
|
|
+ 4. **A 自己也管不了 B 了**:因为 B 已经脱离部门体系。等价于 A 把 B "踢出部门树",使后续任何 MEMBER-级别的同级调整都失效,只有超管/产品 ADMIN 能动。
|
|
|
|
|
+ 5. 结合 H-3 的等级平级攻击:A 先把 B 拉到同级,再把 B 踢出部门 → B 变成一个在组织结构里"无归属、无人能管"的幽灵高权账号。
|
|
|
|
|
|
|
|
- **影响**:
|
|
- **影响**:
|
|
|
- - 组织树出现悬空父指针,`DeptTree` 展示异常;
|
|
|
|
|
- - 用户的 deptId 成孤儿,`loadDept` 静默失败,用户被持续推到"无权操作"分支;
|
|
|
|
|
- - 如果外部系统以 `deptPath` 鉴权,会出现"应该有权限但系统说没"或反之的权限紊乱。
|
|
|
|
|
|
|
+ - **这不是简单的一致性问题**,是 MEMBER 可以**合法**破坏组织架构语义 + 失去组织可管控性:账号进入"灰色托孤态",只有平台超管能打捞。
|
|
|
|
|
+ - 从合规角度:任何有"部门树 = 数据隔离边界"的业务(如多租户),本接口可以**越过部门边界丢弃账号的隶属关系**。
|
|
|
|
|
+ - 没有 audit log 配合的情况下,此动作甚至不会立即被察觉。
|
|
|
|
|
|
|
|
- **修复方案**:
|
|
- **修复方案**:
|
|
|
- 在 `DeleteDept` 事务内,对 **parentId 索引** 和 **deptId 索引** 做"存在性锁定读":
|
|
|
|
|
|
|
+ 两种口径任选其一(建议第 2 种,更严格):
|
|
|
|
|
+ 1. **禁止非 ADMIN 把 deptId 改为 0**(把原"只在 >0 时校验"变成"不等于当前 deptId 时都要校验"):
|
|
|
|
|
+ ```go
|
|
|
|
|
+ if req.DeptId != nil && *req.DeptId != user.DeptId {
|
|
|
|
|
+ if *req.DeptId == 0 {
|
|
|
|
|
+ if !caller.IsSuperAdmin && caller.MemberType != consts.MemberTypeAdmin {
|
|
|
|
|
+ return response.ErrForbidden("仅超级管理员或产品管理员可移除用户的部门归属")
|
|
|
|
|
+ }
|
|
|
|
|
+ } else {
|
|
|
|
|
+ newDept, err := l.svcCtx.SysDeptModel.FindOne(l.ctx, *req.DeptId)
|
|
|
|
|
+ if err != nil { return response.ErrBadRequest("部门不存在") }
|
|
|
|
|
+ if !caller.IsSuperAdmin && caller.MemberType != consts.MemberTypeAdmin &&
|
|
|
|
|
+ caller.DeptPath != "" &&
|
|
|
|
|
+ !strings.HasPrefix(newDept.Path, caller.DeptPath) {
|
|
|
|
|
+ return response.ErrForbidden("无权将用户调入非自己管辖的部门")
|
|
|
|
|
+ }
|
|
|
|
|
+ }
|
|
|
|
|
+ deptId = *req.DeptId
|
|
|
|
|
+ }
|
|
|
|
|
+ ```
|
|
|
|
|
+ 2. 如果产品规范里本就不支持"用户无部门",应直接把 `0` 当作非法值:`if *req.DeptId <= 0 { return ErrBadRequest("部门ID无效") }`。
|
|
|
|
|
+
|
|
|
|
|
+---
|
|
|
|
|
+
|
|
|
|
|
+## ⚠️ 健壮性与性能建议 (Medium/Low)
|
|
|
|
|
|
|
|
|
|
+### M-1. `RefreshTokenLogic` **先解析 JWT,再限流**:无效 token 穿透限流,可用于爆破 refreshSecret
|
|
|
|
|
+
|
|
|
|
|
+- **位置**:`internal/logic/pub/refreshTokenLogic.go:40-73`
|
|
|
|
|
+- **描述**:`TokenOpLimiter.Take("refresh:%d")` 的 key 需要 `claims.UserId`,所以必须**先 `ParseRefreshToken` 成功**才能限流。对一批**攻击用的无效 token**(例如在爆破 refresh secret 时构造的伪造签名),全部会走到:
|
|
|
```go
|
|
```go
|
|
|
- // 子部门:如果有任意一行,就失败;并在 parentId 索引上加 gap/next-key 锁,阻塞并发 INSERT
|
|
|
|
|
- var tmp int64
|
|
|
|
|
- q := "SELECT 1 FROM sys_dept WHERE parentId=? LIMIT 1 FOR UPDATE"
|
|
|
|
|
- err := session.QueryRowCtx(ctx, &tmp, q, req.Id)
|
|
|
|
|
- if err == nil {
|
|
|
|
|
- return response.ErrBadRequest("该部门下存在子部门,无法删除")
|
|
|
|
|
- }
|
|
|
|
|
- if !errors.Is(err, sql.ErrNoRows) {
|
|
|
|
|
- return err
|
|
|
|
|
|
|
+ claims, err := authHelper.ParseRefreshToken(tokenStr, l.svcCtx.Config.Auth.RefreshSecret)
|
|
|
|
|
+ if err != nil {
|
|
|
|
|
+ return nil, response.ErrUnauthorized("refreshToken无效或已过期")
|
|
|
}
|
|
}
|
|
|
- // 用户引用:同样用锁定读
|
|
|
|
|
- q = "SELECT 1 FROM sys_user WHERE deptId=? LIMIT 1 FOR UPDATE"
|
|
|
|
|
- err = session.QueryRowCtx(ctx, &tmp, q, req.Id)
|
|
|
|
|
- ...
|
|
|
|
|
```
|
|
```
|
|
|
- `CreateDept` / `CreateUser` / `UpdateUser` 在写 `parentId` / `deptId` 之前,也要 `SELECT 1 FROM sys_dept WHERE id=? FOR SHARE`(S-lock 父行),这样 `DeleteDept` 的 X-lock 会阻塞这些插入,TOCTOU 被彻底消除。
|
|
|
|
|
-
|
|
|
|
|
- 对长期工程:建议直接加 MySQL FK `RESTRICT`;但即使不加 FK,应用层也必须把"存在性检查 + 删除"放在同一事务的锁定读语义下。
|
|
|
|
|
|
|
+ 不进入限流桶。JWT 验签是较重的 HMAC/RS 运算,攻击者可以借此做 CPU-放大 DoS,或持续爆破 refresh secret(离线爆破失败,就改在线爆破,反正被限流也不限流)。
|
|
|
|
|
+- **建议**:在 `Authorization` 头解析(TrimPrefix)成功后,**以 IP 为 key**加一道最外层 PeriodLimit,如 `limiter.Take(fmt.Sprintf("refresh-ip:%s", clientIP))`。路由层已经可以通过 `RefreshTokenRateLimit` 中间件配合来做,**但这个中间件目前没挂载在 `/api/auth/refreshToken`**(路由定义里只有 LoginRateLimit 等),建议补上。
|
|
|
|
|
|
|
|
---
|
|
---
|
|
|
|
|
|
|
|
-### H-5. `ChangePasswordLogic` 无任何限流,JWT 被盗/会话仍在场景下可**暴力爆破当前密码**
|
|
|
|
|
|
|
+### M-2. `UpdateDeptLogic` 对部门成员的 `UserDetailsLoader.Clean` 在 for 循环里**串行同步调用**
|
|
|
|
|
|
|
|
-- **位置**:`internal/logic/auth/changePasswordLogic.go:32-62`
|
|
|
|
|
-- **描述**:
|
|
|
|
|
|
|
+- **位置**:`internal/logic/dept/updateDeptLogic.go`(循环清缓存处)
|
|
|
|
|
+- **描述**:在 deptType / status 变更时,代码形如:
|
|
|
```go
|
|
```go
|
|
|
- func (l *ChangePasswordLogic) ChangePassword(req *types.ChangePasswordReq) error {
|
|
|
|
|
- if msg := util.ValidatePassword(req.NewPassword); msg != "" { ... }
|
|
|
|
|
- userId := middleware.GetUserId(l.ctx)
|
|
|
|
|
- user, err := l.svcCtx.SysUserModel.FindOne(l.ctx, userId)
|
|
|
|
|
- ...
|
|
|
|
|
- if err := bcrypt.CompareHashAndPassword([]byte(user.Password), []byte(req.OldPassword)); err != nil {
|
|
|
|
|
- return response.ErrBadRequest("原密码错误")
|
|
|
|
|
- }
|
|
|
|
|
- ...
|
|
|
|
|
|
|
+ userIds, _ := l.svcCtx.SysUserModel.FindIdsByDeptId(l.ctx, req.Id)
|
|
|
|
|
+ for _, uid := range userIds {
|
|
|
|
|
+ l.svcCtx.UserDetailsLoader.Clean(l.ctx, uid)
|
|
|
}
|
|
}
|
|
|
```
|
|
```
|
|
|
- - 路由层:`/api/auth/changePassword` 只有 `JwtAuth` 中间件,**无 RateLimitMiddleware**。
|
|
|
|
|
- - 逻辑层:`LogoutLogic` 和 `RefreshTokenLogic` 都挂了 `svcCtx.TokenOpLimiter`,但 `ChangePasswordLogic` 没有;也没有 `UsernameLoginLimit` 类的每用户限流。
|
|
|
|
|
- - `bcrypt.CompareHashAndPassword` 大约 ~100ms,已登录用户可以串行每秒 ~10 次对自己当前密码做爆破。
|
|
|
|
|
- - 威胁模型:
|
|
|
|
|
- 1. 攻击者偷到 access token(XSS/设备共用),想"把密码改成自己的"以实现持久化。由于必须知道旧密码,于是他用盗来的 token 不停调 `changePassword` 枚举旧密码。虽然 bcrypt 慢,但没有限流意味着**可以一直尝试**——被害人直到 access token 过期(默认 `AccessExpire`,通常 2h~1d)都无法自救。
|
|
|
|
|
- 2. 更现实:同一台办公机、同一设备同一账号,未离开时间段内 malicious script 执行百万次尝试;每次失败 `ErrBadRequest("原密码错误")`,既不记录日志也不递增 tokenVersion,失败完全"无痕"。
|
|
|
|
|
- - 对比:`LogoutLogic` 做了 10/60s 限流,`RefreshTokenLogic` 也做了;`ChangePasswordLogic` 显然遗漏。
|
|
|
|
|
|
|
+ 单次 `Clean` 内部包含 `SMEMBERS + DEL(批) + DEL(索引键) + SREM`(见 `userDetailsLoader.go:185-199, 221-230`),至少 4 次 Redis 往返。一个 500 人的部门 = 2000 次 Redis 同步 RTT 卡在请求线程里。这会直接阻塞 HTTP handler(默认 go-zero timeout 可能都兜不住)。
|
|
|
|
|
+- **建议**:
|
|
|
|
|
+ 1. loader 层加 `CleanMany(ctx, userIds)`:一次 `SMEMBERS` 取每个索引键,`DEL` 合并所有 `cacheKey`(Redis 支持任意多 key 的批量 DEL),索引键用一次 `DEL` 或 pipeline 批处理。
|
|
|
|
|
+ 2. 或用 `BatchDel(userIds, productCode)`(已经在 loader 里)——不过 `BatchDel` 只清特定产品,对"部门跨多个产品"的场景要多次调用。更清爽的做法是按索引键一次性清:
|
|
|
|
|
+ ```go
|
|
|
|
|
+ func (l *UserDetailsLoader) CleanByUserIds(ctx context.Context, ids []int64) {
|
|
|
|
|
+ idxKeys := make([]string, len(ids))
|
|
|
|
|
+ for i, id := range ids { idxKeys[i] = l.userIndexKey(id) }
|
|
|
|
|
+ // pipelined SMEMBERS + DEL
|
|
|
|
|
+ }
|
|
|
|
|
+ ```
|
|
|
|
|
+ 3. 当部门用户数量超过阈值(如 200)时,**fire-and-forget 到后台 goroutine + 日志**,不要阻塞外部请求。
|
|
|
|
|
|
|
|
-- **影响**:
|
|
|
|
|
- - 提供了"持有短期 access token 后可以 brute-force 当前密码,命中后改密持久化"的攻击路径;
|
|
|
|
|
- - 一旦命中,新密码会触发 `tokenVersion+1`(`UpdatePassword` 里),**踢掉真正的用户**,攻击者用新密码重新登录即可接管(管理后台也同理,因为它用同一套 bcrypt)。
|
|
|
|
|
|
|
+---
|
|
|
|
|
|
|
|
-- **修复方案**:
|
|
|
|
|
|
|
+### M-3. `UserDetailsLoader.Load` 对**已删除用户**不做负缓存:一把有效 JWT + 删除账号 = 每次请求 7 条 DB 查询
|
|
|
|
|
|
|
|
|
|
+- **位置**:`internal/loaders/userDetailsLoader.go:119-144`
|
|
|
|
|
+- **描述**:
|
|
|
```go
|
|
```go
|
|
|
- // internal/logic/auth/changePasswordLogic.go
|
|
|
|
|
- if l.svcCtx.TokenOpLimiter != nil {
|
|
|
|
|
- code, _ := l.svcCtx.TokenOpLimiter.Take(fmt.Sprintf("chpwd:%d", userId))
|
|
|
|
|
- if code == limit.OverQuota {
|
|
|
|
|
- return response.ErrTooManyRequests("操作过于频繁,请稍后再试")
|
|
|
|
|
|
|
+ v, sfErr, _ := l.sf.Do(key, func() (interface{}, error) {
|
|
|
|
|
+ ud, err := l.loadFromDB(ctx, userId, productCode)
|
|
|
|
|
+ ...
|
|
|
|
|
+ if ud.Username == "" {
|
|
|
|
|
+ return nil, nil
|
|
|
}
|
|
}
|
|
|
|
|
+ ...
|
|
|
|
|
+ })
|
|
|
|
|
+ ...
|
|
|
|
|
+ if !ok || ud == nil {
|
|
|
|
|
+ return &UserDetails{UserId: userId, ProductCode: productCode}
|
|
|
}
|
|
}
|
|
|
```
|
|
```
|
|
|
- 建议在 `ValidatePassword` 之后、`FindOne` 之前立即做(失败也计数,避免 "先写合法新密码才触发限流" 的绕过)。同时把失败做一条 `logx.WithContext(l.ctx).Infof("change-password old-password mismatch userId=%d", userId)`,给 SOC 提供可观测性。
|
|
|
|
|
|
|
+ 当用户已被硬删除(`FindOne` 返回 `ErrNotFound`),`loadUser` 把 `ud.Username` 留空,外层判定"空,不缓存",返回一个空壳 `UserDetails`。Middleware 再根据 `ud.Username == ""` 返回 401。
|
|
|
|
|
+
|
|
|
|
|
+ 但"不缓存"这件事意味着:**只要攻击者 / 离线员工仍持有未过期的 accessToken(默认 1~2 小时),每个请求都会触发 `loadFromDB` → 至少 1~7 次 DB SELECT**(loadUser / loadDept / loadProduct / loadMember / loadRoles / loadRolePerms / loadUserPerms)。
|
|
|
|
|
+ 虽然 `sf.Do` 会合并**同时到达**的相同 key,但在真实场景下"攻击者手里只有一张 token,每 100ms 打一次"时,每次请求都是串行命中。
|
|
|
|
|
+
|
|
|
|
|
+ 一个具体的 DoS 放大场景:
|
|
|
|
|
+ - 公司大规模离职,某几个离职员工的 accessToken 还在剩余寿命里(甚至能用 refreshToken 续期——虽然 `claims.TokenVersion != ud.TokenVersion` 会拦住,但 token 失效前的 accessToken 里 `tokenVersion` 对照"空 ud.TokenVersion=0"反而可能通过 `claims.TokenVersion != 0` 拦截)。
|
|
|
|
|
+ - 他们留下的 pipeline 任务 / 前端 polling 以 HTTP 规律重试,相当于**针对权限中心 DB 发起常驻压测**,且**无任何限流**(token 能通过 JWT 签名校验,就不会进入 IP 限流的 login 桶)。
|
|
|
|
|
+
|
|
|
|
|
+- **建议**:
|
|
|
|
|
+ 1. 对"Username 为空"的路径**也写入短 TTL(30~60s)的负缓存 sentinel**:
|
|
|
|
|
+ ```go
|
|
|
|
|
+ if ud.Username == "" {
|
|
|
|
|
+ _ = l.rds.SetexCtx(ctx, key, `{"userId":0}`, 30) // 或带一个 "deleted":true 字段
|
|
|
|
|
+ return nil, nil
|
|
|
|
|
+ }
|
|
|
|
|
+ ```
|
|
|
|
|
+ 加载侧读到 sentinel 立刻返回空 `UserDetails`,不再走 DB。
|
|
|
|
|
+ 2. 更彻底:引入**基于 userId 的全局"已删除"布隆过滤器**,在 `DeleteUser` 时添加;`Load` 读到命中时直接 short-circuit。
|
|
|
|
|
+ 3. `jwtauthMiddleware` 对"用户被删除"的路径打印警告日志 + 对应 token 的 `userId` 加入短期封禁列表(5~10 分钟),避免垃圾 token 长期耗资源。
|
|
|
|
|
|
|
|
---
|
|
---
|
|
|
|
|
|
|
|
-## ⚠️ 健壮性与性能建议 (Medium / Low)
|
|
|
|
|
|
|
+### M-4. `UpdateWithOptLock` / `UpdateProfile` / `UpdatePassword` 的乐观锁**以秒级 `updateTime` 为版本**:同秒并发写会丢更新
|
|
|
|
|
+
|
|
|
|
|
+- **位置**:
|
|
|
|
|
+ - `internal/model/user/sysUserModel.go:99-120`(UpdateProfile,`WHERE id=? AND updateTime=?`)
|
|
|
|
|
+ - `internal/model/dept/sysDeptModel.go:UpdateWithOptLock`
|
|
|
|
|
+ - `internal/model/product/sysProductModel.go:UpdateWithOptLock`
|
|
|
|
|
+ - `internal/model/role/sysRoleModel.go:UpdateWithOptLock`
|
|
|
|
|
+- **描述**:
|
|
|
|
|
+ 乐观锁的 `WHERE updateTime = ?` 约束依赖"版本必然变化"。当前实现把 `updateTime` 写为 `time.Now().Unix()`(秒级)。同一秒内的两笔并发 UPDATE:
|
|
|
|
|
+ - Op1 读到 `updateTime = T`,计算 `new.updateTime = T`(同秒)。
|
|
|
|
|
+ - Op2 读到 `updateTime = T`,计算 `new.updateTime = T`。
|
|
|
|
|
+ - Op1 先 commit:`UPDATE ... SET ...,updateTime=T WHERE id=? AND updateTime=T` → 匹配 1 行(MySQL 默认 `rows_affected` 是"matched rows";如果配置了 `CLIENT_FOUND_ROWS` 是更明显的匹配),**新 updateTime 仍是 T**(没变)。
|
|
|
|
|
+ - Op2 再 commit:同样 `WHERE updateTime=T` 命中,Op1 的变更被 Op2 覆盖;业务**静默丢失**。
|
|
|
|
|
+
|
|
|
|
|
+ `UpdatePassword` / `UpdateStatus` 走的是**无乐观锁**的 `UPDATE ... WHERE id=?`,同秒并发也一样会后写覆盖前写,只不过对"设密码"而言是同一人改两次的幂等场景,影响小;但 `UpdateProfile` 这种带业务字段的操作,**同秒后提交的请求会把前一个合法更改吞掉,且 `RowsAffected=1`**,外层误以为成功。
|
|
|
|
|
+
|
|
|
|
|
+ 同秒并发在本项目不算罕见:
|
|
|
|
|
+ - 管理后台前端批量操作,一次提交里同时改部门下 20 个用户;
|
|
|
|
|
+ - gRPC 下游 API 网关重放;
|
|
|
|
|
+ - 运维脚本并行刷数据。
|
|
|
|
|
+
|
|
|
|
|
+- **建议**:
|
|
|
|
|
+ 1. **引入独立的 `version` 整型列**,每次 `SET version=version+1 WHERE id=? AND version=?`,彻底脱离时间戳。
|
|
|
|
|
+ 2. 过渡方案:在 `UpdateXxxWithOptLock` 里用 `time.Now().UnixNano()` 代替 `Unix()`,并把 `updateTime` 列类型放宽到 `BIGINT`(已经是 bigint);同秒间碰撞概率从 1 降到纳秒级,基本规避。
|
|
|
|
|
+ 3. 或使用 `UPDATE ... WHERE id=? AND updateTime=? AND ... (其他校验)`,让 `affected=0` 时抛 `ErrUpdateConflict` 触发客户端重试(**前提是 updateTime 每次真会变**,上面两种方案任选其一先修)。
|
|
|
|
|
+
|
|
|
|
|
+---
|
|
|
|
|
|
|
|
-### M-1. `UpdateUserStatus` 对"状态无变化"的请求仍然强制递增 `tokenVersion`,把被操作用户不必要地踢下线
|
|
|
|
|
-- **位置**:`internal/logic/user/updateUserStatusLogic.go:31-52`、`internal/model/user/sysUserModel.go:137-150`
|
|
|
|
|
-- **描述**:`UpdateUserStatus` 没有对比 `user.Status` 与 `req.Status`,不管是否真的变更,都调用 `SysUserModel.UpdateStatus`;模型层 SQL 为 `UPDATE … SET status=?, tokenVersion=tokenVersion+1 …` **无条件**递增。结果:管理员点一次"启用"按钮(用户原本就是启用),所有该用户在场的会话全部下线。
|
|
|
|
|
- - 对比 `UpdateUserLogic`(`updateUserLogic.go:122-135`):显式 `if user.Status != req.Status { statusChanged = true }`,只在真正变化时才递增。两处逻辑不一致。
|
|
|
|
|
-- **修复**:进入 logic 时先读当前值,仅当 `user.Status != req.Status` 时调 UpdateStatus;或者在 SQL 上加 `WHERE id=? AND status<>?` 并仅当 `RowsAffected>0` 时认为"确实变更"。
|
|
|
|
|
|
|
+### M-5. `CreateProductLogic` 仍残留 `strings.Contains(errMsg, "uk_code")` 与 `strings.Contains(errMsg, req.Code)` 二级判定
|
|
|
|
|
|
|
|
-### M-2. `generateRandomHex` 长度截断导致 `appSecret` / `adminPassword` 熵减半(上一轮 M-3,仍未修复)
|
|
|
|
|
-- **位置**:`internal/logic/product/createProductLogic.go:158-164`
|
|
|
|
|
|
|
+- **位置**:`internal/logic/product/createProductLogic.go:135-146`
|
|
|
- **描述**:
|
|
- **描述**:
|
|
|
```go
|
|
```go
|
|
|
- func generateRandomHex(length int) (string, error) {
|
|
|
|
|
- b := make([]byte, length)
|
|
|
|
|
- rand.Read(b)
|
|
|
|
|
- return hex.EncodeToString(b)[:length], nil // 😱 截断
|
|
|
|
|
|
|
+ if util.IsDuplicateEntryErr(err) {
|
|
|
|
|
+ errMsg := err.Error()
|
|
|
|
|
+ if strings.Contains(errMsg, "uk_code") || strings.Contains(errMsg, req.Code) { ... }
|
|
|
|
|
+ if strings.Contains(errMsg, "uk_username") || strings.Contains(errMsg, adminUsername) { ... }
|
|
|
|
|
+ return nil, response.ErrConflict("数据冲突,请稍后重试")
|
|
|
}
|
|
}
|
|
|
```
|
|
```
|
|
|
- - `hex.EncodeToString(N bytes) => 2N chars`;`[:length]` 拿到 `length` 个 hex 字符 = `length/2` 字节随机性。
|
|
|
|
|
- - `generateRandomHex(64)` 生成 `appSecret` → **32 字节**熵,不是 64;
|
|
|
|
|
- - `generateRandomHex(32)` 生成 `appKey` → **16 字节** / 128 bit,勉强;
|
|
|
|
|
- - `generateRandomHex(8)` 生成首任管理员初始密码 → **4 字节 = 32 bit**,可在 10 分钟内离线爆破(就算走 bcrypt,超管持有明文返回值且要邮件/IM 明文传给运营方——泄漏风险真实存在)。
|
|
|
|
|
-- **修复**:
|
|
|
|
|
- ```go
|
|
|
|
|
- func generateRandomHex(byteLen int) (string, error) {
|
|
|
|
|
- b := make([]byte, byteLen)
|
|
|
|
|
- if _, err := rand.Read(b); err != nil { return "", err }
|
|
|
|
|
- return hex.EncodeToString(b), nil // 返回 2*byteLen 个 hex 字符
|
|
|
|
|
- }
|
|
|
|
|
- ```
|
|
|
|
|
- 调用方按"字节数"来传;初始管理员密码至少 12 字节随机 → 96 bit 熵,或者直接改用人类可读的 passphrase。
|
|
|
|
|
|
|
+ 第一步正确使用 `IsDuplicateEntryErr`,但第二步"到底是哪张唯一键冲突"又退回 `strings.Contains(errMsg, ...)`。
|
|
|
|
|
+ 问题:
|
|
|
|
|
+ - `strings.Contains(errMsg, req.Code)` 这一回退 fallback 极其脆弱:DB 错误消息未必内嵌值,也可能有类似子串误命中;
|
|
|
|
|
+ - MySQL 5.7/8.0 在错误消息里展示键名时带 prefix `Duplicate entry 'xxx' for key 'sys_product.uk_code'`,把 table 前缀去掉后才是 `uk_code`;不同版本字符串不同;
|
|
|
|
|
+ - 万一后续 DDL 把索引 rename,消息匹配直接失效,代码编译仍通过——**静默退化**到 "数据冲突,请稍后重试",前端无法区分是产品 code 冲突还是用户名冲突,UX 降级。
|
|
|
|
|
+- **建议**:
|
|
|
|
|
+ 改用**预校验 + 插入并发兜底**模式:
|
|
|
|
|
+ 1. 在 tx 开始前(已经有这段)用 `FindOneByCode` / `FindOneByUsername` 做存在性校验,命中直接返回明确错误消息。
|
|
|
|
|
+ 2. 并发插入时命中的重复键错误,类型断言 `*mysql.MySQLError` 后直接返回通用 `ErrConflict("数据冲突,请重试")`,**不要再用字符串匹配区分键**。这等价于:
|
|
|
|
|
+ - 并发场景极少发生(单秒同时建同 code 的产品几乎不会有);
|
|
|
|
|
+ - 一旦发生,用户看到"冲突请重试"是可接受的;
|
|
|
|
|
+ - 非并发场景早被步骤 1 的精确文案拦住了。
|
|
|
|
|
|
|
|
-### M-3. `UserDetailsLoader` 对 "不存在的用户 / 错误的 (userId, productCode)" 没有负缓存 → 每次请求都穿透到 DB
|
|
|
|
|
-- **位置**:`internal/loaders/userDetailsLoader.go:119-144`
|
|
|
|
|
-- **描述**:`loadFromDB` 若 `ud.Username == ""`(用户不存在或已被删),`sf.Do` 返回 `(nil, nil)`,**不写任何缓存**。`Load` 返回一个空 UserDetails。下一次同样请求仍然走一次完整 DB 链路(`FindOne` → 命中模型层 cache miss → DB)。
|
|
|
|
|
-- **影响**:
|
|
|
|
|
- - 账号被删除后,旧 access token 的每次请求都会触发 1 次 `sys_user.FindOne` 未命中的 DB 查询(直到 token 过期,默认可达 1h~24h)。
|
|
|
|
|
- - 攻击者发送垃圾 access token(签名正确但 `userId` 为不存在 id)即可放大 DB 压力;虽然签名伪造需要 secret,但一个离职用户保留的 token 就能让内网攻击者制造 DB 噪声。
|
|
|
|
|
-- **修复**:在 `loadFromDB` 识别到 "用户不存在 / 已删除" 时,写一条 TTL 很短(15~60s)的负缓存标记(例如 cache 一个 `ud.Username=="_not_found_"`),`Load` 检测到该标记直接返回"不存在",无需再次查库。
|
|
|
|
|
-
|
|
|
|
|
-### M-4. `UserDetailsLoader` 的 "index set 添加 (SADD) 与数据 key 写入 (SETEX)" 非原子 → 并发下 `Clean` 可能漏清
|
|
|
|
|
-- **位置**:`internal/loaders/userDetailsLoader.go:127-132,185-199,201-219`
|
|
|
|
|
-- **描述**:写链路顺序是 `SETEX(key, json)` → `SADD(userIdxKey, key)` → `EXPIRE(userIdxKey)`;清链路是 `SMEMBERS(userIdxKey)` → `DEL(keys...)` → `DEL(userIdxKey)`。
|
|
|
|
|
- 交错:
|
|
|
|
|
- 1. Thread A 写入:`SETEX` 完成。
|
|
|
|
|
- 2. Thread B 触发 `Clean(userId)`:`SMEMBERS` 尚未看到新 key → `DEL` 清单中不含它 → `DEL idxKey` 清掉索引。
|
|
|
|
|
- 3. Thread A 继续:`SADD` 把 key 加回到(已被删除的)索引 → 再 `EXPIRE`(这步会重新创建 set,于是孤儿索引);同时数据 key 仍在 Redis 里。
|
|
|
|
|
- 4. 结果:**数据 key 实际仍在**,`Clean` 却已经认为"清完了"。stale 数据持续到 `ttl=300s` 才自然过期。
|
|
|
|
|
- 这在 `BindRoles → Clean`、`UpdateUser → Clean`、`UpdateRole → BatchDel` 等高频写入路径都可复现。
|
|
|
|
|
-- **修复**:
|
|
|
|
|
- - 用 Redis 事务 / 脚本把 `SETEX + SADD + EXPIRE` 打包成单次 `EVAL`;
|
|
|
|
|
- - 清理链路也用 Lua 把 `SMEMBERS + DEL keys + DEL idxKey` 原子化;
|
|
|
|
|
- - 更简洁的替代:弱一致保证下,在每次 `Del/Clean` 后发一条 "延迟二次清除" 消息(比如 `DEL after 1s`),补偿这一窄竞态。
|
|
|
|
|
-
|
|
|
|
|
-### M-5. `UpdateRole` / `BindRolePerms` 在事务提交后读 `FindUserIdsByRoleId` 时**忽略错误**,Redis 抖动时会漏清缓存
|
|
|
|
|
-- **位置**:`internal/logic/role/updateRoleLogic.go:73-75`、`internal/logic/role/bindRolePermsLogic.go:127-128`
|
|
|
|
|
-- **描述**:
|
|
|
|
|
- ```go
|
|
|
|
|
- affectedUserIds, _ := l.svcCtx.SysUserRoleModel.FindUserIdsByRoleId(l.ctx, req.Id)
|
|
|
|
|
- l.svcCtx.UserDetailsLoader.BatchDel(l.ctx, affectedUserIds, role.ProductCode)
|
|
|
|
|
- ```
|
|
|
|
|
- `_,_` 丢 err,`affectedUserIds` 为空即 `BatchDel` 无作为。底层 DB 任何抖动都会让这次变更的缓存失效丢失;用户最多需要 5 分钟才拿到新权限。
|
|
|
|
|
-- **修复**:`return err` 并在调用链把这个错误归为 "已更新但缓存未清" 的 500;或者失败时写进 retry 队列,由后台任务重做。
|
|
|
|
|
|
|
+---
|
|
|
|
|
|
|
|
-### M-6. `UpdateRole` / `UpdateProduct` / `UpdateMember` 没有乐观锁(与 `UpdateUser` / `UpdateDept` 的策略不一致)
|
|
|
|
|
-- **位置**:
|
|
|
|
|
- - `internal/logic/role/updateRoleLogic.go:69`(`SysRoleModel.Update(ctx, role)`)
|
|
|
|
|
- - `internal/logic/product/updateProductLogic.go:58`(`SysProductModel.Update(...)`)
|
|
|
|
|
- - `internal/logic/member/updateMemberLogic.go:75`(`SysProductMemberModel.UpdateWithTx(...)`)
|
|
|
|
|
-- **描述**:`UpdateUserLogic` 和 `UpdateDeptLogic` 用了 `UpdateWithOptLock(expectedUpdateTime)` 防并发覆盖;但相同模式的 `UpdateRole` / `UpdateProduct` / `UpdateMember` 仍是无条件 `UPDATE`:
|
|
|
|
|
- - 两位管理员同时修改同一角色(A 改名字,B 改 permsLevel),后提交者会全字段覆盖先提交者的改动,**丢失字段**。
|
|
|
|
|
-- **修复**:统一给三者加 `UpdateWithOptLock(expectedUpdateTime)` / 或在 SQL WHERE 子句中加 `updateTime = ?`,并在受影响行数为 0 时返回 `ErrUpdateConflict`。
|
|
|
|
|
-
|
|
|
|
|
-### M-7. `AdminLoginLogic` 缺少"不存在账号的 dummy bcrypt",响应时间可用于 username 枚举
|
|
|
|
|
-- **位置**:`internal/logic/pub/adminLoginLogic.go:48-66`
|
|
|
|
|
-- **描述**:`ValidateProductLogin` 在用户不存在时跑一次 `dummyBcryptHash` 做恒时对齐(`loginService.go:53`),但 `AdminLoginLogic` 没有——直接 `return "用户名或密码错误"`。结合错误信息差异("账号已被冻结" vs "仅超级管理员可通过管理后台登录")和响应时间差异(bcrypt ~100ms vs 不调用 bcrypt ~1ms),攻击者可在 `AdminLoginRateLimit(20/min)`、`UsernameLoginLimit(10/5min)` 限额内做较精确枚举。
|
|
|
|
|
-- **修复**:与 `ValidateProductLogin` 对齐:
|
|
|
|
|
- ```go
|
|
|
|
|
- if errors.Is(err, user.ErrNotFound) {
|
|
|
|
|
- bcrypt.CompareHashAndPassword(dummyBcryptHash, []byte(req.Password))
|
|
|
|
|
- return nil, response.ErrUnauthorized("用户名或密码错误")
|
|
|
|
|
- }
|
|
|
|
|
- ```
|
|
|
|
|
- 并把"账号已被冻结" / "仅超级管理员可通过管理后台登录" 这两个分支统一归到 `"用户名或密码错误"`(管理后台侧不暴露任何有效账号的状态信息),让登录失败无法区分。
|
|
|
|
|
|
|
+### M-6. `SyncPermsService.ExecuteSyncPerms`:**读快照在 tx 外、写在 tx 内**,并发同步会撞 `DuplicateEntry`
|
|
|
|
|
|
|
|
-### M-8. `ProductList` / `ProductDetail` / `DeptTree` 无访问控制,普通成员能看全量产品名 / 组织架构(上一轮 M-4/M-5,仍未修复)
|
|
|
|
|
-- **位置**:
|
|
|
|
|
- - `internal/logic/product/productListLogic.go:31-58`
|
|
|
|
|
- - `internal/logic/product/productDetailLogic.go:29-48`
|
|
|
|
|
- - `internal/logic/dept/deptTreeLogic.go:27-67`
|
|
|
|
|
-- **描述**:都只有 `JwtAuth`,没有任何"是当前产品成员 / 当前部门管辖"过滤。`ProductList` 只是把 `appKey` 在非超管时置空,名称/备注仍返回;`ProductDetail` 给任何 id 都能读。`DeptTree` 更是一次性返回所有部门——对外泄漏组织结构和产品清单。
|
|
|
|
|
-- **修复**:
|
|
|
|
|
- - `ProductList` 改为"按 `sys_product_member` inner join `productCode` 过滤成当前调用者所属的产品集合";超管才能看全量;
|
|
|
|
|
- - `ProductDetail` 对非超管校验 `caller.ProductCode == product.Code`;
|
|
|
|
|
- - `DeptTree` 至少要求 `RequireSuperAdmin` 或返回以调用者部门为根的子树。
|
|
|
|
|
-
|
|
|
|
|
-### M-9. `SetUserPermsLogic` 重复执行了同一条 `FindOneByProductCodeUserId` 查询(`CheckManageAccess → checkPermLevel` 内部做一次,随后 `SetUserPerms` 里又做一次)
|
|
|
|
|
-- **位置**:`internal/logic/user/setUserPermsLogic.go:54-64` + `internal/logic/auth/access.go:148-157`
|
|
|
|
|
-- **描述**:`CheckManageAccess(req.UserId, productCode)` 内部 `checkPermLevel` 已经 `FindOneByProductCodeUserId(productCode, targetUserId)`;紧随其后 `SetUserPerms` 又做一次同样的读,再加上一次 `FindOne(user)`、一次 `FindOneByCode(product)`、一次 `FindByIds(perms)`——在写路径上累计 5~6 次 DB read。考虑到模型层都有 `CachedConn`,多数命中缓存,但 `FindMinPermsLevelByUserIdAndProductCode` 是无缓存查询,每次接口调用都会实际打一次 DB。
|
|
|
|
|
-- **修复**:
|
|
|
|
|
- - 让 `CheckManageAccess` 把解析出来的 `targetMember` / `targetRoleLevel` 通过返回值或 context 透出,SetUserPerms 直接复用;
|
|
|
|
|
- - 或把整个权限校验前置到单独的 "AuthContext" 对象,一次加载。
|
|
|
|
|
-
|
|
|
|
|
-### M-10. `VerifyToken` gRPC 对"无效 token"完全不记录日志,也没有调用方识别字段(appKey/serviceName)→ 生产上不可观测
|
|
|
|
|
-- **位置**:`internal/server/permserver.go:173-208`
|
|
|
|
|
-- **描述**:所有失败分支都 `return &pb.VerifyTokenResp{Valid: false}, nil`,不分类、不打日志。线上"用户反馈总是 401"时没法判断是 `TokenVersion` 不符还是 `ProductStatus` 禁用还是别的;更没有"该产品服务在过去 10 分钟发生了 10k 次 VerifyToken 失败"这种告警能力。
|
|
|
|
|
-- **修复**:至少保留 `logx.WithContext(ctx).Infof("verifyToken fail userId=%d reason=%s", claims.UserId, reason)`;`reason` 单独落字段方便日志聚合。
|
|
|
|
|
-
|
|
|
|
|
-### M-11. gRPC `PermServer.Login` 在 `peer.FromContext` 失败时静默跳过限流(fail-open)
|
|
|
|
|
-- **位置**:`internal/server/permserver.go:62-76`
|
|
|
|
|
-- **描述**:`if s.svcCtx.GrpcLoginLimiter != nil { if p, ok := peer.FromContext(ctx); ok { … rate limit … } }`——如果 `ok == false`(走 in-process / socket 无 peer 信息的场景)就**不限流**。配合 ExecuteSyncPerms 同样逻辑(gRPC 不限流,HTTP 层才有),理论上内部调用或错误配置下会绕过保护。
|
|
|
|
|
-- **修复**:`ok == false` 时把 key 当作 `"grpc:login:unknown"` 走限流,更严苛的做法是"没 peer → 直接拒绝"(因为生产环境都在 gRPC over TCP)。
|
|
|
|
|
-
|
|
|
|
|
-### M-12. 三个 scaffolded 中间件文件是僵尸代码,从未注册也没被引用
|
|
|
|
|
-- **位置**:
|
|
|
|
|
- - `internal/middleware/adminloginratelimitMiddleware.go`
|
|
|
|
|
- - `internal/middleware/productloginratelimitMiddleware.go`
|
|
|
|
|
- - `internal/middleware/syncratelimitMiddleware.go`
|
|
|
|
|
-- **描述**:三个文件各自声明一个空壳类型和 `Handle` 方法(直接 passthrough),路由里实际使用的是 `svc.ServiceContext` 里基于 `RateLimitMiddleware` 构造出来的实例。两边同名但互不相干,静态分析工具看来这些 `*Middleware` 类型无任何引用。
|
|
|
|
|
-- **修复**:直接删除这三个文件;或者把它们改成 `// Deprecated. Use ...` 并在下次发版一起删。
|
|
|
|
|
-
|
|
|
|
|
-### M-13. `bindRolesLogic` 的 "role level" 检查对 DEVELOPER 无条件放行
|
|
|
|
|
-- **位置**:`internal/logic/user/bindRolesLogic.go:85-91`
|
|
|
|
|
-- **描述**:
|
|
|
|
|
- ```go
|
|
|
|
|
- if !caller.IsSuperAdmin &&
|
|
|
|
|
- caller.MemberType != consts.MemberTypeAdmin &&
|
|
|
|
|
- caller.MemberType != consts.MemberTypeDeveloper {
|
|
|
|
|
- if caller.MinPermsLevel == math.MaxInt64 || r.PermsLevel < caller.MinPermsLevel {
|
|
|
|
|
- return response.ErrForbidden("不能分配权限级别高于自身的角色")
|
|
|
|
|
|
|
+- **位置**:`internal/logic/pub/syncPermsService.go`
|
|
|
|
|
+- **描述**:代码流程大致是:
|
|
|
|
|
+ ```
|
|
|
|
|
+ existing := FindMapByProductCode(productCode) // tx 外,普通 SELECT
|
|
|
|
|
+ TransactCtx(func(session) {
|
|
|
|
|
+ for code, perm := range req.Perms {
|
|
|
|
|
+ if _, ok := existing[code]; !ok {
|
|
|
|
|
+ InsertWithTx(...)
|
|
|
|
|
+ } else {
|
|
|
|
|
+ UpdateWithTx(...)
|
|
|
|
|
+ }
|
|
|
}
|
|
}
|
|
|
- }
|
|
|
|
|
|
|
+ ... disable 不在请求列表里的旧权限 ...
|
|
|
|
|
+ })
|
|
|
```
|
|
```
|
|
|
- DEVELOPER 绕过 perms-level 校验——**语义正确前提**是"DEVELOPER 已经拿到产品全部权限(`loadPerms` 里走全权分支)",因此让 TA 给 MEMBER 分高阶角色不算越权。但这条业务语义并不显式写在代码里,任何未来把 DEVELOPER 权限收窄的改动都会立刻让这里成为漏洞。
|
|
|
|
|
-- **修复**:在 `access.go` 里统一写一个 `callerIsFullPermInProduct(ud) bool`(条件:SuperAdmin / ADMIN / DEVELOPER / 或 `DeptType==DEV` 且成员启用),所有依赖"caller 已拥有全权"做的短路都复用它,变更只需改一处。`loadPerms` 的判定也统一走它。
|
|
|
|
|
|
|
+ 两次并发 `SyncPermissions`:
|
|
|
|
|
+ - 都读到 `existing` 中没有 code X;
|
|
|
|
|
+ - 都 `InsertWithTx(..., code=X)`;
|
|
|
|
|
+ - 第二次 tx 在 `UNIQUE (productCode, code)` 上撞 1062 → rollback。
|
|
|
|
|
|
|
|
-### M-14. `strings.Contains(err.Error(), "1062")` 脆弱的错误识别(多处)
|
|
|
|
|
-- **位置**:
|
|
|
|
|
- - `internal/logic/user/createUserLogic.go:92-96`
|
|
|
|
|
- - `internal/logic/role/createRoleLogic.go:67-71`
|
|
|
|
|
- - `internal/logic/member/addMemberLogic.go:76-80`
|
|
|
|
|
- - `internal/logic/product/createProductLogic.go:134-144`
|
|
|
|
|
-- **描述**:所有冲突检测都走字符串匹配 `"1062"` / `"Duplicate entry"`。一旦换 driver 版本或 MySQL 升级导致文案变化,这些检测直接失效,逻辑全部变成 500。产品/用户的唯一索引冲突会被当成内部错误吃掉。
|
|
|
|
|
-- **修复**:换成 `mysql.MySQLError` 类型判断:
|
|
|
|
|
- ```go
|
|
|
|
|
- import "github.com/go-sql-driver/mysql"
|
|
|
|
|
- var me *mysql.MySQLError
|
|
|
|
|
- if errors.As(err, &me) && me.Number == 1062 {
|
|
|
|
|
- return response.ErrConflict(...)
|
|
|
|
|
- }
|
|
|
|
|
- ```
|
|
|
|
|
|
|
+ 当前错误处理把 1062 直接 `return err`,外层包装成 500,对客户端来说看起来"同步随机失败",但其实是两个合法同步的竞态。
|
|
|
|
|
+- **建议**:
|
|
|
|
|
+ 1. 把 `FindMapByProductCode` **挪进 tx 内**,并在前面做一次 `SELECT id FROM sys_product WHERE code=? FOR UPDATE` 锁住 product 行(或 `GET_LOCK('sync:'+productCode, 5)`),相当于把同步串行化到每个 product。同步是低频操作,串行化是能接受的。
|
|
|
|
|
+ 2. `InsertWithTx` 改用 `INSERT ... ON DUPLICATE KEY UPDATE`:把插入 / 更新合并成一个语义,天然幂等。
|
|
|
|
|
+ 3. `IsDuplicateEntryErr` 的路径显式返回 `response.ErrConflict("权限同步存在并发冲突,请重试")`,前端据此做重试,不要让客户端无脑 500。
|
|
|
|
|
|
|
|
-### M-15. `ChangePasswordLogic` 在 bcrypt 校验之前不检查 `user.Status` 冻结状态
|
|
|
|
|
-- **位置**:`internal/logic/auth/changePasswordLogic.go:37-45`
|
|
|
|
|
-- **描述**:`FindOne → bcrypt.CompareHashAndPassword`,没有 `if user.Status != StatusEnabled`。JWT 中间件已经对冻结用户拦截,但 (a) 冻结后中间件 loader cache 未及时失效的 race 窗口;(b) 冻结状态的用户理论上仍可调 changePassword(虽然此 JWT 已被拦截)。防御纵深建议双保险。
|
|
|
|
|
-- **修复**:
|
|
|
|
|
- ```go
|
|
|
|
|
- if user.Status != consts.StatusEnabled {
|
|
|
|
|
- return response.ErrForbidden("账号已被冻结")
|
|
|
|
|
- }
|
|
|
|
|
- ```
|
|
|
|
|
|
|
+---
|
|
|
|
|
|
|
|
-### L-1. `SetUserPerms` / `BindRoles` / `BindRolePerms` 的去重写回到了 `req`(副作用入参)
|
|
|
|
|
-- **位置**:
|
|
|
|
|
- - `setUserPermsLogic.go:72-86`
|
|
|
|
|
- - `bindRolesLogic.go:58-68`
|
|
|
|
|
- - `bindRolePermsLogic.go:44-54`
|
|
|
|
|
|
|
+### M-7. gRPC `PermServer.Login` 的**对端 IP 提取 fail-open**
|
|
|
|
|
+
|
|
|
|
|
+- **位置**:`internal/server/permserver.go`(`Login` 方法里对 `peer.FromContext` 和 `net.SplitHostPort` 的错误处理分支,clientIP 回退为 `"unknown"` 或原始 `p.Addr.String()`)
|
|
|
- **描述**:
|
|
- **描述**:
|
|
|
|
|
+ - `clientIP = "unknown"` 意味着**所有**这种失败 case 共享同一个限流桶,正常请求被 pollute 后限流可能集体打满;
|
|
|
|
|
+ - 如果回退成 `p.Addr.String()` 又包含端口号,每个连接端口都是一个新 key,限流完全等价于无限流(端口随每个连接变化)。
|
|
|
|
|
+- **建议**:
|
|
|
|
|
+ - 不能确定 IP 时**直接拒绝**:`return nil, status.Error(codes.Unavailable, "peer not identifiable")`。
|
|
|
|
|
+ - 这个规则同样要套在 gRPC `SyncPermissions` / `RefreshToken` / `VerifyToken` / `GetUserPerms` 的新限流中间件里(H-2 的修复路径)。
|
|
|
|
|
+
|
|
|
|
|
+---
|
|
|
|
|
+
|
|
|
|
|
+### M-8. `ChangePasswordLogic` / `UpdatePassword`:**无乐观锁 + 并发修改密码双方都成功**
|
|
|
|
|
+
|
|
|
|
|
+- **位置**:
|
|
|
|
|
+ - `internal/logic/auth/changePasswordLogic.go`
|
|
|
|
|
+ - `internal/model/user/sysUserModel.go:122-135`(UpdatePassword)
|
|
|
|
|
+- **描述**:两笔同时发起的 ChangePassword 请求(例如用户双窗口操作 + 自动化脚本),都能通过 "旧密码比对",都走 `UpdatePassword`,**后写覆盖前写**。`tokenVersion` 被加 2 次,最终 hash 来自后写者。
|
|
|
|
|
+ 这里不涉及 security escalation,但:
|
|
|
|
|
+ - 如果用户看到第一次"修改成功",过了 1 秒再用新密码登录却失败(因为后写者用的是"旧密码 + 新密码计算出的 hash",最终 hash 属于第二个请求的新密码),用户会以为被盗号。
|
|
|
|
|
+ - 两个 `IncrementTokenVersion` 把后续所有合法会话踢掉两次,UX 破损。
|
|
|
|
|
+- **建议**:在 `UpdatePassword` 里用 `WHERE id=? AND tokenVersion=?`(expected = 改密前读到的 tokenVersion),冲突时走 `ErrUpdateConflict` 返回前端"请刷新后重试"。
|
|
|
|
|
+
|
|
|
|
|
+---
|
|
|
|
|
+
|
|
|
|
|
+### L-1. `CreateUserLogic` 默认 `mustChangePassword = consts.MustChangePasswordNo`
|
|
|
|
|
+
|
|
|
|
|
+- **位置**:`internal/logic/user/createUserLogic.go`
|
|
|
|
|
+- **描述**:管理员为新用户创建账号时,密码是管理员"代填"的,业务上通常应**强制首登改密**。当前默认值是 `No`,意味着管理员口头告诉员工密码后,员工可以长期不改;如果管理员密码库泄露,所有新建账号都通用。
|
|
|
|
|
+- **建议**:把默认改为 `consts.MustChangePasswordYes`;或请求体加 `mustChangePassword *int` 字段,不传时按 `Yes` 处理。
|
|
|
|
|
+
|
|
|
|
|
+---
|
|
|
|
|
+
|
|
|
|
|
+### L-2. `RefreshToken` 限流 key 仅含 `userId`,**不含 IP**
|
|
|
|
|
+
|
|
|
|
|
+- **位置**:`internal/logic/pub/refreshTokenLogic.go:69`
|
|
|
|
|
+- **描述**:`TokenOpLimiter.Take(fmt.Sprintf("refresh:%d", claims.UserId))`。若攻击者从多个 IP 同时使用同一 refreshToken(与 H-1 的 race 攻击配套),限流桶共享,单桶配额耗尽攻击即止;但同一合法用户**自己**在多个 IP 刷新(手机 + 电脑)时也会互相挤占配额。
|
|
|
|
|
+- **建议**:把 IP 加入 key(`refresh:%d:%s`,IP),同时保留 per-user 总量上限(`refresh-u:%d`)。
|
|
|
|
|
+
|
|
|
|
|
+---
|
|
|
|
|
+
|
|
|
|
|
+### L-3. `CreateDeptLogic` 的 `InsertWithTx → FindOneWithTx → UpdateWithTx` 组合:**缓存早于事务提交失效**
|
|
|
|
|
+
|
|
|
|
|
+- **位置**:`internal/logic/dept/createDeptLogic.go:73-95`
|
|
|
|
|
+- **描述**:`UpdateWithTx` 内部调用 `m.ExecCtx`,go-zero 会**在 exec 成功后立即 `DelCacheCtx`**,此时事务尚未 commit。假设另一 goroutine 在 exec 完成与 commit 之间以 deptId 调 `FindOne`,会:
|
|
|
|
|
+ - cache miss → DB 查询 → 事务未提交对其他 session 不可见 → 返回 `ErrNotFound` → go-zero cached conn 把"未找到"的 sentinel 写进 cache(典型 TTL ~1 秒)。
|
|
|
|
|
+ - 事务 commit 之后,真正的行已存在,但 cache 里是"未找到" sentinel,后续请求命中错误缓存。
|
|
|
|
|
+ 现象:创建部门后的前 1 秒,其他查询可能偶发"部门不存在"。生产罕见,属于一致性微抖动。
|
|
|
|
|
+- **建议**:这是 go-zero cache 层面的已知模式,不是本项目独有。如需彻底消除,把"计算 Path + 第二次 UpdateWithTx"改成**在插入阶段就把 Path 算好**(trigger 或 app 层先 `LAST_INSERT_ID()`,但 MyISAM/InnoDB 在 `InsertWithTx` 里就能从 `result.LastInsertId()` 拿到),然后 `UPDATE` 一次就好——其实现在的代码也是一次 UPDATE,只是 `FindOneWithTx` 是多余的(拿完整 SysDept 只为回写 Path,完全可以直接构造 UPDATE 语句)。简化为:
|
|
|
```go
|
|
```go
|
|
|
- uniquePerms := make([]types.UserPermItem, 0, len(req.Perms))
|
|
|
|
|
- ...
|
|
|
|
|
- req.Perms = uniquePerms
|
|
|
|
|
|
|
+ result, err := InsertWithTx(...)
|
|
|
|
|
+ deptId = result.LastInsertId()
|
|
|
|
|
+ path := fmt.Sprintf("%s%d/", parentPath, deptId)
|
|
|
|
|
+ _, err = session.ExecCtx(ctx, "UPDATE sys_dept SET `path`=?, `updateTime`=? WHERE `id`=?", path, now, deptId)
|
|
|
```
|
|
```
|
|
|
- 对外部传入的 `req` 做原地写入会让上层(例如 handler、中间件、单元测试)读到被改过的结构,违背"logic 不修改入参"原则。
|
|
|
|
|
-- **修复**:改成局部变量 `perms := uniquePerms`;后续所有流程使用 `perms`,不再碰 `req.Perms`。
|
|
|
|
|
|
|
+ 同时用 `m.DelCacheCtx(cacheSysDeptIdPrefix+deptId)` 显式在 tx 外做一次二次清理。
|
|
|
|
|
|
|
|
-### L-2. `memberTypePriority("")` 返回 `MaxInt32`,在 `CheckMemberTypeAssignment` 下会与未知类型保持等价判定
|
|
|
|
|
-- **位置**:`internal/logic/auth/access.go:15-28,67-69`
|
|
|
|
|
-- **描述**:若 `caller.MemberType == ""`(例如产品禁用后被 loader 清空),`memberTypePriority("")==MaxInt32`;`if MaxInt32 >= priority(assigned)` 恒真 → `CheckMemberTypeAssignment` 直接拒绝。这里"fail-closed"是正确行为,但逻辑靠的是 sentinel 值而非显式分支,容易在将来扩展 MemberType 时出错。
|
|
|
|
|
-- **修复**:显式判断 `caller.MemberType == "" → return ErrForbidden("缺少产品成员上下文")`。
|
|
|
|
|
|
|
+---
|
|
|
|
|
+
|
|
|
|
|
+### L-4. `CheckManageAccess` / `checkPermLevel` 把 `targetLevel = math.MaxInt64`(查不到角色)当"目标无权限"处理
|
|
|
|
|
|
|
|
-### L-3. `UserDetailsLoader.Clean` 的错误忽略会扩散
|
|
|
|
|
-- **位置**:`internal/loaders/userDetailsLoader.go:150-153,177-179,186-198`
|
|
|
|
|
-- **描述**:Redis 所有操作失败都只 `Errorf` 记日志,然后继续。当 Redis 间歇不可用时,管理员以为"清完了",实际上旧缓存可能至少 5 分钟内生效。
|
|
|
|
|
-- **修复**:对关键变更路径(角色授权、状态变更)做"两次清理 + 失败回退":第一次 `Del` 失败 → 在队列里起一次 retry;或者把 UserDetails 的缓存层 TTL 缩短到 60s 并接受 DB 压力(负载允许前提下)。
|
|
|
|
|
|
|
+- **位置**:`internal/logic/auth/access.go:185-192`
|
|
|
|
|
+- **描述**:目标用户没有任何启用角色时 `FindMinPermsLevelByUserIdAndProductCode` 返回 err 或空 → 代码 fallback 为 `targetLevel = math.MaxInt64`。然后 `caller.MinPermsLevel >= targetLevel`(MaxInt64)永远为 false(除非 caller 也是 MaxInt64),于是 caller "严格高于" target → 允许管辖。
|
|
|
|
|
+ 语义上这是合理的(无角色用户的等级最低),但一旦 `FindMinPermsLevelByUserIdAndProductCode` 因为 DB 抖动真正返回 err,**会被同化成"目标无角色"**,给出错误放行。
|
|
|
|
|
+- **建议**:把"查不到角色 = MaxInt64"与"DB 抖动 = err"的路径分开:`err != nil && !errors.Is(err, sqlx.ErrNotFound)` 时直接返回错误而不是降级放行。
|
|
|
|
|
+
|
|
|
|
|
+---
|
|
|
|
|
|
|
|
-### L-4. `DeptTree` 对"孤儿 parent"只打日志即当 root 并继续返回 → 静默数据异常
|
|
|
|
|
-- **位置**:`internal/logic/dept/deptTreeLogic.go:55-63`
|
|
|
|
|
-- **描述**:如果发生 H-4 的 TOCTOU,树里会出现 "parentId 指向不存在的 id" 的部门,代码会把它"视作 root"继续放回前端。**数据已损坏但用户/管理员无感知**,管理员很可能在之后一通操作里把 children 移到别处却不清楚真正丢失的是哪棵子树。
|
|
|
|
|
-- **修复**:同一批异常记录直接在响应里带一个 `warnings` 字段或在 HTTP 头里 flag,让前端报警;后台做一条 alerting 指标 `dept_orphan_count > 0`。
|
|
|
|
|
|
|
+### L-5. `CreateProductLogic.generateRandomHex` 的 tx 外密钥生成:**事务失败时密钥被泄露到日志 / 响应**
|
|
|
|
|
|
|
|
-### L-5. 多数接口在 DB 错误时直接 `return err` 而不包装为统一 500,生产会透出底层 sqlx/gorm/bcrypt 错误文本
|
|
|
|
|
-- **位置**:几乎所有 logic 文件的尾端
|
|
|
|
|
-- **描述**:`response.Setup` 会把非 `*CodeError` 的错误映射成 `{code: 500, msg: "服务器内部错误"}` 并 `logx.Errorf`。这本身没问题,但很多 logic 在上下文敏感的位置(比如 bcrypt 生成失败)返回原始 err 到调用栈,日志里出现原始 bcrypt 错误文本也是一种信息披露。建议所有 logic 统一包装成 `response.ErrInternal("xxx 失败")` 并把原 err 放入 `logx.Errorf` 里,避免上下层关心如何转换。
|
|
|
|
|
|
|
+- **位置**:`internal/logic/product/createProductLogic.go`
|
|
|
|
|
+- **描述**:`appKey` / `appSecret` / adminPassword 在 tx 前生成;tx 失败时函数直接 `return nil, err`,响应体里不会带走这些值(OK),但:
|
|
|
|
|
+ - `logx.Errorf("internal error: %+v", err)` 有可能在 stack 之外打印 req;
|
|
|
|
|
+ - 如果调用方带 `X-Request-Id` 等 trace header,这次失败生成的 appSecret 不会重试时复用,等于每次重试都把一串密钥丢到熵池里直到 tx 成功。不是安全问题但无意义。
|
|
|
|
|
+- **建议**:把密钥生成挪进 tx 内部(或 tx 成功 commit 后再最后一步),避免失败态的"幽灵密钥";响应返回的明文 `AppSecret` / `AdminPassword` 建议换成一次性下载 URL 或要求创建者当场抄写,别持久化在响应体。
|
|
|
|
|
|
|
|
---
|
|
---
|
|
|
|
|
|
|
|
-## 🧭 总结与本轮优先级
|
|
|
|
|
|
|
+## 结论与修复优先级
|
|
|
|
|
|
|
|
-| 优先级 | 问题 | 关键词 |
|
|
|
|
|
|
|
+| 优先级 | finding | 概要 |
|
|
|
| --- | --- | --- |
|
|
| --- | --- | --- |
|
|
|
-| P0 | H-1 | UpdateMember 禁用最后 ADMIN 绕过 |
|
|
|
|
|
-| P0 | H-2 | RemoveMember 事务内外视图脱钩 |
|
|
|
|
|
-| P0 | H-3 | CountActiveAdmins 跨行 TOCTOU |
|
|
|
|
|
-| P0 | H-4 | DeleteDept 子部门/用户 TOCTOU |
|
|
|
|
|
-| P0 | H-5 | ChangePassword 无限流可暴力破旧密码 |
|
|
|
|
|
-| P1 | M-1 | UpdateUserStatus "无变化也踢下线" |
|
|
|
|
|
-| P1 | M-2 | generateRandomHex 熵减半 |
|
|
|
|
|
-| P1 | M-6 | UpdateRole/Product/Member 缺乐观锁 |
|
|
|
|
|
-| P1 | M-7 | AdminLogin username 枚举 |
|
|
|
|
|
-| P1 | M-8 | ProductList / ProductDetail / DeptTree 无访问控制 |
|
|
|
|
|
-| P2 | M-3/M-4/M-5/M-9/M-10/M-11/M-13/M-14/M-15 | 可观测 / 容错 / 代码一致性 |
|
|
|
|
|
-| P3 | M-12 / L-* | 僵尸代码、副作用入参、错误透传 |
|
|
|
|
|
-
|
|
|
|
|
-### 建议的修复顺序
|
|
|
|
|
-1. **立刻修 H-1 / H-2 / H-3**:三者共同保护"产品至少一个 ADMIN"不变式,彼此独立,必须三条路径一起堵。建议抽一个 `guardLastActiveAdminTx(session, productCode, targetMemberId)` helper,`UpdateMember` 和 `RemoveMember` 都调用它;内部先 `SELECT id ... FOR UPDATE` 锁定所有活跃 ADMIN,再做 count/是否将失活判断。
|
|
|
|
|
-2. **修 H-4**:部门删除用 `SELECT 1 ... FOR UPDATE` 做存在性锁定读;`CreateDept`/`CreateUser`/`UpdateUser` 写 deptId 前对父部门 `SELECT ... FOR SHARE`。短期方案即可生效,长期改 FK。
|
|
|
|
|
-3. **修 H-5**:changePassword 加 `TokenOpLimiter.Take("chpwd:%d")`;补充失败日志。
|
|
|
|
|
-4. **批量修 M-1 / M-6 / M-2 / M-7**:这几条都是一行到十行级的小改,收益很高。
|
|
|
|
|
-5. **有余力再做 M-3/M-4/M-5/M-10/M-14**:涉及缓存层或错误分类,需要更严谨的回归测试。
|
|
|
|
|
-
|
|
|
|
|
-> 注:本轮报告不再列"已在上轮修复"或"已被单测覆盖"的 finding(比如 H-A/H-B、TokenVersion 相关等),见 `test-report.md` 对应条目。
|
|
|
|
|
|
|
+| **P0** | H-1 | RefreshToken TOCTOU 导致会话劫持(HTTP + gRPC) |
|
|
|
|
|
+| **P0** | H-2 | gRPC RefreshToken / VerifyToken 零限流 |
|
|
|
|
|
+| **P0** | H-3 | BindRoles 平级放行破坏管理层级 |
|
|
|
|
|
+| **P0** | H-4 | UpdateUser deptId=0 绕过部门管辖 |
|
|
|
|
|
+| P1 | M-4 | 秒级 updateTime 乐观锁丢更新 |
|
|
|
|
|
+| P1 | M-3 | UserDetailsLoader 无负缓存 → DoS |
|
|
|
|
|
+| P1 | M-2 | UpdateDept 缓存失效串行 Redis |
|
|
|
|
|
+| P1 | M-6 | SyncPermissions 并发 1062 |
|
|
|
|
|
+| P1 | M-1 | refreshToken 先解析再限流 |
|
|
|
|
|
+| P2 | M-5 / M-7 / M-8 | 错误分类脆弱 / IP 提取 / ChangePassword 并发 |
|
|
|
|
|
+| P3 | L-1 ~ L-5 | 默认值 / 限流 key / cache 抖动 / err 降级 / 密钥生命周期 |
|
|
|
|
|
+
|
|
|
|
|
+### 建议的修复次序
|
|
|
|
|
+
|
|
|
|
|
+1. **先修 H-1 + H-2 一起修**:`IncrementTokenVersionIfMatch` + gRPC 限流中间件,一次封住会话劫持通道。这两条必须原子上线,否则单修一边(例如只修 HTTP)攻击者改用 gRPC 就绕过。
|
|
|
|
|
+2. **修 H-3 / H-4**:两条都是一行到十行级别的条件修正,配套写单测"MEMBER 给下属绑同级角色必须 403"、"MEMBER 把下属 deptId 改 0 必须 403"。
|
|
|
|
|
+3. **修 M-4 乐观锁**:先用纳秒级 updateTime 做过渡,同时评估加 `version` 列的 DDL 变更计划。
|
|
|
|
|
+4. **修 M-3 负缓存 + M-2 缓存失效批处理**:两者都是影响线上稳定性的中等问题,配合可观测(`UserDetailsLoader` 每次 `loadFromDB` 打一个 metric)。
|
|
|
|
|
+5. **修 M-6 SyncPermissions**:`FindMapByProductCode` 进 tx,把插入改 `ON DUPLICATE KEY UPDATE`;错误回复改 `ErrConflict`。
|
|
|
|
|
+6. 收尾:M-1 / M-5 / M-7 / M-8 / L-*。
|
|
|
|
|
+
|
|
|
|
|
+> 说明:第 5 轮审计不再重列已在第 4 轮完成修复的 H-1/H-2/H-3(最后一个 ADMIN)、H-5(changePassword TokenOpLimiter)、H-4(DeleteDept 存在性锁读),也不重列已由测试覆盖的 TokenVersion 基本路径。以上 findings 在当前 HEAD 代码中复现无误,均有可触发的真实业务 / 攻击路径。
|