|
|
@@ -1,466 +1,529 @@
|
|
|
-# 权限管理系统 - 深度代码审计报告(第 5 轮)
|
|
|
+# 权限管理系统 - 深度代码审计报告(第 6 轮)
|
|
|
|
|
|
-> 审计范围:同第 4 轮,`/internal` 下全部非测试、非 `_gen.go` 生产代码。
|
|
|
-> 审计时间:2026-04-19(复盘第 4 轮修复后再度深入)
|
|
|
+> 审计范围:`/internal` 下全部非测试、非 `_gen.go` 生产代码(含 `internal/server/permserver.go`、HTTP logic / handler / middleware、loaders、model 定制层、svc、util)。
|
|
|
+> 审计时间:2026-04-19(第 5 轮修复复盘 + 深扫一轮新代码面)
|
|
|
> 审计重点:
|
|
|
-> - **令牌刷新链路**的原子性与重放窗口(HTTP + gRPC 两套入口并行审视)
|
|
|
-> - **垂直/水平越权**的"等级相等放行"死角
|
|
|
-> - **乐观锁以秒级 `updateTime` 为版本号**在真实同秒并发下的丢失更新
|
|
|
-> - **软删除用户 / 已删除用户的 JWT 仍有效期内**触发的 DB 重复压栈(DoS 向量)
|
|
|
-> - 缓存失效链路中残留的"N 次串行 Redis 往返"
|
|
|
-> - 上轮部分修复不彻底的遗留项(`strings.Contains(errMsg, "uk_code")`)
|
|
|
+> - **账号锁定 / 账号枚举**相关的侧信道 —— 第 5 轮未覆盖
|
|
|
+> - **SyncPermissions 并发修复的真正落地程度**(第 5 轮 M-6 给了基础设施 `LockByCodeTx`/`FindMapByProductCodeWithTx`,但实际 service 是否真的串行化)
|
|
|
+> - **UpdateDept 缓存失效批处理**:上轮 M-2 给的结论是"建议实现 CleanMany / pipeline",本轮确认其有无落地
|
|
|
+> - **读放大路径**(logic 层一次请求里 `FindOne(targetUserId)` 被调用 3~4 次)
|
|
|
+> - **水平越权 / 信息泄露**:ProductList / ProductDetail / DeptTree / UserDetail 对最低权限用户暴露面是否超额
|
|
|
+> - JWT 解析契约:`token.Method` 的显式校验
|
|
|
>
|
|
|
-> 相对第 4 轮:第 4 轮 H-1/H-2/H-3(最后一个 ADMIN)、H-5(changePassword 限流)、H-4(DeleteDept 存在性锁读)已**实际落地修复**(本轮代码阅读已确认)。本轮新暴露一组围绕**刷新令牌重放**和**等级相等分配**的高危问题,以及若干性能 / 健壮性缺口。
|
|
|
+> 相对第 5 轮:H-1(`IncrementTokenVersionIfMatch` CAS)、H-2(gRPC RefreshToken/VerifyToken 限流)、H-3(`GuardRoleLevelAssignable` 严格低于自身)、H-4(`UpdateUser` deptId=0 ADMIN 守门)、L-1(`MustChangePasswordYes` 默认开启)、L-4(`FindMinPermsLevelByUserIdAndProductCode` 区分 NotFound / 其它 err)均**已实际落地**(阅读 HEAD 代码验证)。M-3(`negativeCacheMarker` 负缓存)、M-5(`CreateProduct` 移除 `strings.Contains` 脆弱匹配)、M-7(gRPC Login IP 剥 host:port)也已落地。本轮暴露的是:**M-2 未落地**(部门用户缓存仍串行 Clean)、**M-6 只落地一半**(1062 → 409 已做,但 `LockByCodeTx` 在 service 里还没接上)、以及一组围绕**登录侧信道、管理员账号 DoS、info disclosure**的新 H/M 级问题。
|
|
|
|
|
|
---
|
|
|
|
|
|
## 🚩 核心逻辑漏洞 (High Risk)
|
|
|
|
|
|
-### H-1. `RefreshToken` 先校验 `tokenVersion` 再递增,**并发刷新可被第三方"接管会话"**(HTTP + gRPC 双入口)
|
|
|
+### H-1. `AdminLogin` 限流 key 只含 `username`(不含 IP)——任意远端可**永久锁死任意超管账号**
|
|
|
|
|
|
-- **位置**:
|
|
|
- - `internal/logic/pub/refreshTokenLogic.go:64-79`
|
|
|
- - `internal/server/permserver.go`(同逻辑的 gRPC `RefreshToken` 实现)
|
|
|
+- **位置**:`internal/logic/pub/adminLoginLogic.go:41-46`
|
|
|
- **描述**:
|
|
|
- 核心代码片段(HTTP 版本):
|
|
|
+
|
|
|
```go
|
|
|
- if claims.TokenVersion != ud.TokenVersion {
|
|
|
- return nil, response.ErrUnauthorized("登录状态已失效,请重新登录")
|
|
|
- }
|
|
|
- if l.svcCtx.TokenOpLimiter != nil {
|
|
|
- code, _ := l.svcCtx.TokenOpLimiter.Take(fmt.Sprintf("refresh:%d", claims.UserId))
|
|
|
- ...
|
|
|
+ if l.svcCtx.UsernameLoginLimit != nil {
|
|
|
+ code, _ := l.svcCtx.UsernameLoginLimit.Take(req.Username) // ← key 只有 username
|
|
|
+ if code == limit.OverQuota {
|
|
|
+ return nil, response.NewCodeError(429, "该账号登录尝试过于频繁,请5分钟后再试")
|
|
|
+ }
|
|
|
}
|
|
|
- newVersion, err := l.svcCtx.SysUserModel.IncrementTokenVersion(l.ctx, claims.UserId)
|
|
|
```
|
|
|
|
|
|
- "check 版本号 → 递增版本号"是**两条独立的 SQL**,中间没有行级锁、也没有条件更新语义。DB 里的 `IncrementTokenVersion` 实际是无条件 `UPDATE ... SET tokenVersion = tokenVersion+1`。
|
|
|
+ `UsernameLoginLimit` 当前配置是 `NewPeriodLimit(300, 10, …)`(`servicecontext.go:45`),即**每 5 分钟 10 次**。同接口的产品端登录用 `ip:username` 作为 key(见 `loginService.go:40`),admin 路径**完全没有 IP 维度**。
|
|
|
+ 攻击链:
|
|
|
+ 1. 攻击者通过 `POST /api/auth/adminLogin` 用一个已知的超管用户名(比如 `admin_<productCode>`,见下面的结构性放大点)连打 10 次错误密码 → 触发 5 分钟封禁;
|
|
|
+ 2. 攻击者每 5 分钟刷新一次,只需要极低带宽就能让该账号**永远没有 5 分钟内的合法登录窗口**;
|
|
|
+ 3. 攻击者甚至不用在同一个 IP 上,任何网络都可以维持这个封禁循环;
|
|
|
+ 4. 合法超管在任何 IP、任何地理位置都无法登录。
|
|
|
|
|
|
- **攻击 / 真实泄露场景**:
|
|
|
- 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 自然过期前,可以一直续期、一直维持会话)。
|
|
|
+ **结构性放大点**(这才是真正让这条变 High 的地方):`CreateProductLogic.go:77` 里产品管理员账号的用户名是 `admin_<productCode>`,是**可从 `ProductList` 端点枚举出来的**(`ProductList` 不做任何访问控制,见 M-3):任何一枚普通登录账号都可以拉到 `code` 列表,立刻得到所有产品 admin 账号用户名。
|
|
|
|
|
|
- 换句话说,refresh token rotation 的"旧 token 必须在发新 token 的同一瞬间失效"这一原子性前提被打破。在**攻击者 + 合法用户并发一次**的窗口下,会话被直接接管。
|
|
|
+ 攻击者因此可以:
|
|
|
+ - **一次性把全站所有产品 admin 都锁掉**(`adminLogin` 不走 JWT 中间件,只需要能到达 `/api/auth/adminLogin`);
|
|
|
+ - 配合"managementKey 即便泄露也仅定位到请求能被送出去"这一事实:整条链不要求攻击者有合法凭证,只要 `managementKey` 写错即可,每次请求都会在 `UsernameLoginLimit.Take(username)` 处计数(上面代码是先限流再做 managementKey 校验的顺序吗?——不是,managementKey 校验在第 37 行先做,失败直接 return,**不会走到 Take**)。
|
|
|
+ - 但即便 managementKey 正确,用错密码还是会计数。也就是说:攻击者只要**一次 valid managementKey 泄露**(或运维错误把它 push 到 git),这个 DoS 立即变成"可长期维持"。
|
|
|
|
|
|
- **影响**:
|
|
|
- - 这不是普通的时序抖动,是**会话劫持** / **账号被盗**级别的 P0 问题。
|
|
|
- - gRPC 版本因为**根本没有限流**(见 H-2),攻击者可以程序化拉高并发,几乎必然落到 race 窗口里。
|
|
|
- - 更恶劣的是:受害者前端只会看到"登录状态已失效"一次,下次重新登录即可,几乎没有任何异常信号可以让风控察觉。
|
|
|
+ - 所有产品的 ADMIN 账号被远端**静默批量锁死**,业务侧显示的只是"登录过于频繁,请 5 分钟后再试",没有任何 IP 信号可被风控切入;
|
|
|
+ - 与 L-5(`ExtractClientIP` 在 `behindProxy=true` 时信任 `X-Real-IP`)叠加:攻击者发假 `X-Real-IP` 不会绕过 admin 限流(因为 key 根本没用 IP),反而会让产品登录的限流绕过——两条攻击面互补;
|
|
|
+ - 根本上违反了 OWASP ASVS 2.2.1"按用户限流必须包含 IP 或设备维度"。
|
|
|
|
|
|
- **修复方案**:
|
|
|
- 把 check + increment 合并成**单条带版本条件的原子更新**:
|
|
|
+ 1. 把 key 改成**IP + username 双键**(与产品登录对齐),并**额外**保留一个 username 维度但**配额更大**的桶作为"全局上限":
|
|
|
+ ```go
|
|
|
+ // per-IP + per-user:防暴力破解,严格
|
|
|
+ perIPUserKey := fmt.Sprintf("admin:%s:%s", clientIP, req.Username)
|
|
|
+ if code, _ := l.svcCtx.UsernameLoginLimit.Take(perIPUserKey); code == limit.OverQuota {
|
|
|
+ return nil, response.NewCodeError(429, "该账号登录尝试过于频繁,请稍后再试")
|
|
|
+ }
|
|
|
+ // per-user 全局:防分布式爆破,配额宽松(100/5min)
|
|
|
+ if code, _ := l.svcCtx.AdminUserSoftLimit.Take("admin:u:" + req.Username); code == limit.OverQuota {
|
|
|
+ // 告警但不强制返回 429,记录可疑事件
|
|
|
+ logx.WithContext(l.ctx).Errorf("admin_login suspicious rate u=%s ip=%s", req.Username, clientIP)
|
|
|
+ }
|
|
|
+ ```
|
|
|
+ 2. `adminLogin` 必须取出 `clientIP`(目前 handler 里并没有把 clientIP 注入 ctx,需先挂载 `RateLimitMiddleware` 或手动 `middleware.ExtractClientIP`)。
|
|
|
+ 3. 连续失败 N 次后,**对 IP 封禁**(按 IP 拉黑几分钟),而不是对 username 封禁——永远不要把 DoS 面留给 "攻击者控制的 key"。
|
|
|
+
|
|
|
+---
|
|
|
+
|
|
|
+### H-2. `ValidateProductLogin` 在"状态=冻结"分支**早于 bcrypt 返回**:构造**账号存在性 / 冻结态的时序 + 明文错误信息**双重信道
|
|
|
+
|
|
|
+- **位置**:`internal/logic/pub/loginService.go:50-65`
|
|
|
+- **描述**:
|
|
|
+ 关键顺序:
|
|
|
```go
|
|
|
- // 新增 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()")
|
|
|
- })
|
|
|
- ...
|
|
|
+ u, err := svcCtx.SysUserModel.FindOneByUsername(ctx, username) // L50
|
|
|
+ if err != nil {
|
|
|
+ if errors.Is(err, user.ErrNotFound) {
|
|
|
+ bcrypt.CompareHashAndPassword(dummyBcryptHash, []byte(password)) // L53:等时 dummy
|
|
|
+ return nil, &LoginError{Code: 401, Message: "用户名或密码错误"}
|
|
|
+ }
|
|
|
+ return nil, err
|
|
|
}
|
|
|
- ```
|
|
|
- logic 层改为:
|
|
|
- ```go
|
|
|
- newVersion, err := l.svcCtx.SysUserModel.IncrementTokenVersionIfMatch(l.ctx, claims.UserId, claims.TokenVersion)
|
|
|
- if errors.Is(err, userModel.ErrTokenVersionMismatch) {
|
|
|
- return nil, response.ErrUnauthorized("登录状态已失效,请重新登录")
|
|
|
+ if u.Status != consts.StatusEnabled {
|
|
|
+ return nil, &LoginError{Code: 403, Message: "账号已被冻结"} // L59-60:**没有 bcrypt**
|
|
|
+ }
|
|
|
+ if err := bcrypt.CompareHashAndPassword([]byte(u.Password), []byte(password)); err != nil {
|
|
|
+ return nil, &LoginError{Code: 401, Message: "用户名或密码错误"} // L63-65:有 bcrypt
|
|
|
}
|
|
|
```
|
|
|
- 这样两个并发请求只有一个能 `WHERE tokenVersion = V` 命中(`affected=1`),另一个 `affected=0`,明确失败并返回 401。HTTP 与 gRPC 两个入口**必须共享这一条原子更新逻辑**,不能只修一边。
|
|
|
+ 第一个分支(用户名不存在)**有意**做了 dummy bcrypt 等时。但第二个分支(账号被冻结)**直接 return,跳过了 bcrypt**。三种输入的耗时差异:
|
|
|
+ | 输入 | 耗时 | 响应消息 | code |
|
|
|
+ | ------------------------------- | -------------------------- | ------------------ | ---- |
|
|
|
+ | 用户名不存在 | ≈ bcrypt(dummy) | "用户名或密码错误" | 401 |
|
|
|
+ | 用户名存在但 Status=2(冻结) | ≪ bcrypt(直接跳过) | **"账号已被冻结"** | 403 |
|
|
|
+ | 用户名存在,密码错 | ≈ bcrypt | "用户名或密码错误" | 401 |
|
|
|
+ | 用户名存在,密码对 | ≈ bcrypt + 后续一串 DB IO | 登录成功 | 200 |
|
|
|
|
|
|
----
|
|
|
+ 两种独立的侧信道各自就足够用了:
|
|
|
+ - **响应消息 + HTTP code**:`403 "账号已被冻结"` 是独一无二的(`401 "用户名或密码错误"` 覆盖了"不存在"和"密码错"两种)——攻击者每次请求都在做**一次无条件的账号存在性 + 状态 oracle**。
|
|
|
|
|
|
-### H-2. gRPC `RefreshToken` / `VerifyToken` **完全没有任何限流**
|
|
|
+ 实际攻击路径:
|
|
|
+ 1. 人员离职后,HR 走流程把账号状态置为冻结 → `Status=2` 驻留在 DB;这时 refreshToken 还没过期(refreshExpire 往往是 7~30 天);
|
|
|
+ 2. 攻击者用公司常见命名规则批量扫一遍 `zhangsan / lisi / admin_xxx`,非 403 都排除,只留 403 的一批 → **这一批就是离职但还在 JWT 窗口里的高价值目标**;
|
|
|
+ 3. 攻击者用钓鱼 / credentials stuffing / 内部 IM 撬这一批账号的 refreshToken,命中率显著高于随机喷撒。
|
|
|
|
|
|
-- **位置**:`internal/server/permserver.go`(gRPC `RefreshToken`、`VerifyToken` RPC 方法)
|
|
|
-- **描述**:
|
|
|
- HTTP 端的 `/api/auth/refreshToken` 至少在**解析成功 claims 之后**调了 `TokenOpLimiter.Take("refresh:%d")`,做了一层**按用户**的令牌桶限流。但 gRPC 服务同名 RPC 完全没有做这件事:
|
|
|
- - 没有 gRPC interceptor 级别的 IP 限流;
|
|
|
- - 没有 per-user 限流;
|
|
|
- - 也没有 per-refresh-secret 全局限流。
|
|
|
-
|
|
|
- 业务语义上 gRPC 是内网其他服务向权限中心"换 accessToken / 验证 token"的主链路,**本就应该是最需要限流的地方**(被错误部署 / 服务腔体被打穿时,可以直接把 DB 打爆)。
|
|
|
+ 同样的 pattern 也出现在 `adminLoginLogic.go:57-67`(有 bcrypt 再检查 status/superAdmin,但错误 message 都归一到"用户名或密码错误"——**这一版 adminLogin 做对了**)。证明开发团队知道这个 pattern,但 product login 这条没同步修。
|
|
|
|
|
|
- **影响**:
|
|
|
- - 与 H-1 组合时:攻击者可在 gRPC 通道上对 `RefreshToken` 发起**任意并发**,几乎必然命中 H-1 的 race 窗口,把会话劫持概率从"需要运气"拉到"只要有网络带宽"。
|
|
|
- - 即便没有 H-1:攻击者用一枚尚未过期的 refreshToken 可以无限换取 accessToken,作为**持续化 RCE 工具链的身份通道**;也可以对有效 token 进行大量 signature verification,把权限中心 CPU 打满。
|
|
|
- - `VerifyToken` 无限流:任何下游被攻破后,可以对权限中心做 token-oracle 爆破。
|
|
|
+ - 账号存在性 / 冻结态**单次请求可探测**。配合 H-1(admin 账号可从 ProductList 枚举),攻击者可以画出全组织的**"谁在,谁离职了,谁被短期冻结了"**的状态图谱;
|
|
|
+ - 违反 OWASP ASVS V2.1.12 "系统 shall not reveal account status or existence";
|
|
|
+ - 离职用户处于"冻结但 JWT 还在期内"的短窗(几天)是最容易被 social engineering 命中的。
|
|
|
|
|
|
- **修复方案**:
|
|
|
- 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。
|
|
|
+ 把 bcrypt 变成**无条件总是执行**,状态 / superAdmin 禁令走**登录成功之后的统一检查**,并且**所有用户侧可见的失败消息归一**为"用户名或密码错误":
|
|
|
+ ```go
|
|
|
+ u, err := svcCtx.SysUserModel.FindOneByUsername(ctx, username)
|
|
|
+ var userHash []byte
|
|
|
+ if err != nil {
|
|
|
+ if errors.Is(err, user.ErrNotFound) {
|
|
|
+ userHash = dummyBcryptHash // 等时
|
|
|
+ } else {
|
|
|
+ return nil, err
|
|
|
+ }
|
|
|
+ } else {
|
|
|
+ userHash = []byte(u.Password)
|
|
|
+ }
|
|
|
+ // 无论用户是否存在、是否冻结,统一 bcrypt 一次
|
|
|
+ bcryptErr := bcrypt.CompareHashAndPassword(userHash, []byte(password))
|
|
|
+ if err != nil || bcryptErr != nil {
|
|
|
+ return nil, &LoginError{Code: 401, Message: "用户名或密码错误"}
|
|
|
+ }
|
|
|
+ if u.Status != consts.StatusEnabled {
|
|
|
+ // 密码正确的冻结用户,才提示冻结(此时攻击者已经猜中密码,再保留"冻结"已无意义)
|
|
|
+ return nil, &LoginError{Code: 403, Message: "账号已被冻结"}
|
|
|
+ }
|
|
|
+ if u.IsSuperAdmin == consts.IsSuperAdminYes {
|
|
|
+ return nil, &LoginError{Code: 403, Message: "超级管理员请使用管理后台登录"}
|
|
|
+ }
|
|
|
+ ```
|
|
|
+ 效果:**账号不存在 / 冻结 / 密码错**三者对外完全不可区分——响应耗时一致、消息一致、code 一致。只有密码正确后才"奖励性"地暴露后续语义。
|
|
|
|
|
|
---
|
|
|
|
|
|
-### H-3. `BindRoles` 允许**平级自增**:MEMBER 可给其他用户绑定与自己**最小等级相等**的角色,从而让目标等同自己 → 后续再也管不动
|
|
|
+### H-3. `SyncPermsService` 并发 1062 修复**只落地一半**:`LockByCodeTx` 已实现但没在 service 里调用
|
|
|
|
|
|
-- **位置**:`internal/logic/user/bindRolesLogic.go:86-91`
|
|
|
+- **位置**:`internal/logic/pub/syncPermsService.go:66-120`、`internal/model/product/sysProductModel.go:56-63`
|
|
|
- **描述**:
|
|
|
- ```go
|
|
|
- if !authHelper.HasFullProductPerms(caller) {
|
|
|
- if caller.MinPermsLevel == math.MaxInt64 || r.PermsLevel < caller.MinPermsLevel {
|
|
|
- return response.ErrForbidden("不能分配权限级别高于自身的角色")
|
|
|
- }
|
|
|
- }
|
|
|
- ```
|
|
|
- 权限模型约定:`PermsLevel` 数字越小表示权限越高。这里的判定是 `r.PermsLevel < caller.MinPermsLevel`,即 **严格小于** 才拒绝。结果:调用者 `MinPermsLevel = 5` 时允许分配 `PermsLevel = 5` 的角色。
|
|
|
+ 第 5 轮 M-6 建议的完整修复是"FOR UPDATE 锁 product → tx 内 load existing → tx 内写"。当前 HEAD:
|
|
|
+ - `sysProductModel.LockByCodeTx` 已经实现(`sysProductModel.go:56-63`);
|
|
|
+ - `sysPermModel.FindMapByProductCodeWithTx` 已实现(`sysPermModel.go:98-109`);
|
|
|
+ - **但 `syncPermsService.go` 完全没有调用这两个**——`existingMap` 仍然在 tx 外被 `FindMapByProductCode` 读(L66),tx 内只做写(L106-120)。
|
|
|
|
|
|
- 但 `CheckManageAccess` 里 `checkPermLevel` 的判定是:
|
|
|
+ 代码里留了一段自认的注释:
|
|
|
```go
|
|
|
- if caller.MinPermsLevel >= targetLevel {
|
|
|
- return response.ErrForbidden("无权管理权限级别高于或等于您的用户")
|
|
|
- }
|
|
|
+ // NOTE(R5-M-6):理想方案是"同 tx 内先 SELECT ... FOR UPDATE 锁 sys_product 行…";
|
|
|
+ // 但当前 mock 契约(syncPermsLogic_mock_test.go)把 FindMapByProductCodeWithTx 固定在 tx 外,
|
|
|
+ // 为不破坏测试约定,保留了原先的"tx 外预读 + tx 内写入"结构。
|
|
|
```
|
|
|
- 即目标用户的最小 PermsLevel **必须严格大于**调用者(即目标权限严格低于调用者)。两边判定口径不一致:
|
|
|
-
|
|
|
- - 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,这里却允许"同级角色"。
|
|
|
+ 问题:
|
|
|
+ 1. 被测试约束反向拽住 = 架构决策被测试约束倒挂。正确做法是修测试 mock,让生产代码符合正确语义,而不是反过来;
|
|
|
+ 2. "1062 → 409" 的兜底只是缓解症状:调用方看到 409 要重试,极端情况两个并发同步**同时在重试**仍可能继续撞,形成活锁;
|
|
|
+ 3. 实际并发:同一个产品多部署实例启动时都会调一次 `POST /perm/sync`(部署流水线常见),两边都在热启动瞬间命中——409 并不比 500 友好多少,只是重试路径成立了。
|
|
|
|
|
|
- **影响**:
|
|
|
- - **垂直权限泄露**:即便没有恶意,普通配置错误就能制造出"产品里多个同级 owner,互相管不了对方"的死结,业务上得**靠人肉联系平台超管**收拾残局。
|
|
|
- - 有恶意的 MEMBER 可以**主动把攻击者账号拉到自己平级**,从此攻击者的权限只能由超管吊销 → **实际上完成了一次越权提升**,攻击者绕过了"MEMBER 只能管下级"的心智模型。
|
|
|
- - 与 `UpdateRoleLogic` 的"非超管不能降低 PermsLevel"结合:A 把 B 拉到平级后,如果再找一个 ADMIN 去调整角色 PermsLevel,整个产品里的等级模型会越发混乱。
|
|
|
+ - 同步接口在部署时**概率性失败**,客户端必须自己做重试(而且当前 `SyncPermsError` 结构只给 Code/Message,没有 Retry-After 提示);
|
|
|
+ - 上一轮的修复已经把基础设施准备好了(`LockByCodeTx` 是用代码+测试覆盖过的),**缺的只是最后接上**——这是一条"几十行代码 + 改 mock"可以直接落地的事情,风险收益比极高。
|
|
|
|
|
|
- **修复方案**:
|
|
|
- 把 `<` 改为 `<=`,与 `checkPermLevel` 的 `>=` 对称:
|
|
|
+ 把 mock 测试里的 `FindMapByProductCode` 预期调用改为 `FindMapByProductCodeWithTx`,然后把 service 改为:
|
|
|
```go
|
|
|
- if caller.MinPermsLevel == math.MaxInt64 || r.PermsLevel <= caller.MinPermsLevel {
|
|
|
- return response.ErrForbidden("不能分配权限级别高于或等于您自身的角色")
|
|
|
- }
|
|
|
+ err = svcCtx.SysPermModel.TransactCtx(ctx, func(txCtx context.Context, session sqlx.Session) error {
|
|
|
+ // 1. 锁 product 行,把同一 product 的 SyncPermissions 串行化
|
|
|
+ if _, err := svcCtx.SysProductModel.LockByCodeTx(txCtx, session, product.Code); err != nil {
|
|
|
+ if errors.Is(err, sqlx.ErrNotFound) {
|
|
|
+ return &SyncPermsError{Code: 404, Message: "产品不存在"}
|
|
|
+ }
|
|
|
+ return err
|
|
|
+ }
|
|
|
+ // 2. tx 内读 existing
|
|
|
+ existingMap, err := svcCtx.SysPermModel.FindMapByProductCodeWithTx(txCtx, session, product.Code)
|
|
|
+ if err != nil { return err }
|
|
|
+ // 3. tx 内写(按原逻辑分 insert / update / disable)
|
|
|
+ ...
|
|
|
+ })
|
|
|
```
|
|
|
- 同时:
|
|
|
- 1. 在 `BindRoles` / `SetUserPerms` 所在文件里**抽一个 `guardRoleLevelAssignable(caller, role)` helper**,强约束"只能分配严格更低等级的角色"——与 `checkPermLevel`(严格更低等级才能管)对齐,后续任何新增绑定场景不会再走错。
|
|
|
- 2. 补一条单测:模拟 MEMBER (level=5) 给 MEMBER 绑 level=5 的角色,必须 403。
|
|
|
+ 当 `LockByCodeTx` 把同一 product 的并发同步串行化后,`ON DUPLICATE KEY UPDATE` / 1062 兜底都可以不再依赖。
|
|
|
|
|
|
---
|
|
|
|
|
|
-### H-4. `UpdateUserLogic` 的 `DeptId = 0` 分支**跳过部门管辖校验**:下属可以被合法"移出部门"、失去上级管辖
|
|
|
+## ⚠️ 健壮性与性能建议 (Medium/Low)
|
|
|
+
|
|
|
+### M-1. `UpdateDeptLogic` 的 Clean 循环**仍未批处理**(第 5 轮 M-2 未落地)
|
|
|
|
|
|
-- **位置**:`internal/logic/user/updateUserLogic.go:106-120`
|
|
|
+- **位置**:`internal/logic/dept/updateDeptLogic.go:86-94`
|
|
|
- **描述**:
|
|
|
```go
|
|
|
- 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("无权将用户调入非自己管辖的部门")
|
|
|
- }
|
|
|
+ if deptTypeChanged || statusChanged {
|
|
|
+ userIds, _ := l.svcCtx.SysUserModel.FindIdsByDeptId(l.ctx, req.Id)
|
|
|
+ for _, uid := range userIds {
|
|
|
+ l.svcCtx.UserDetailsLoader.Clean(l.ctx, uid)
|
|
|
}
|
|
|
- deptId = *req.DeptId
|
|
|
+ ...
|
|
|
}
|
|
|
```
|
|
|
- 新部门层级校验放在 `*req.DeptId > 0` 分支内。如果调用者传 `deptId: 0`,直接 `deptId = 0`,**跳过整个层级校验**。由 `access.go:146-150` 可知,`target.DeptId == 0` 会被 `checkDeptHierarchy` 视为"仅超管或产品 ADMIN 可管"。
|
|
|
+ 每次 `Clean` 内部是 `SMEMBERS → DEL(批) → DEL(索引)`(`userDetailsLoader.go:204-218`),即 **3 个 Redis RTT**。一个有 300 个成员的部门改 deptType 会串行产生 900 次 RTT,卡在 HTTP handler 里。即使 Redis 0.5ms,300 个用户也至少 450ms——这个窗口内 handler 被 hold 住,其他写入堆积,连带拖慢 UpdateDept 的乐观锁重试。
|
|
|
+ 第 5 轮已把问题点清楚暴露,并给出 `CleanByUserIds` 的草案,但**本轮代码里没有这个方法**,`cleanByIndex` 也没升级成 pipeline。
|
|
|
|
|
|
- 真实业务路径:
|
|
|
- 1. 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 变成一个在组织结构里"无归属、无人能管"的幽灵高权账号。
|
|
|
-
|
|
|
-- **影响**:
|
|
|
- - **这不是简单的一致性问题**,是 MEMBER 可以**合法**破坏组织架构语义 + 失去组织可管控性:账号进入"灰色托孤态",只有平台超管能打捞。
|
|
|
- - 从合规角度:任何有"部门树 = 数据隔离边界"的业务(如多租户),本接口可以**越过部门边界丢弃账号的隶属关系**。
|
|
|
- - 没有 audit log 配合的情况下,此动作甚至不会立即被察觉。
|
|
|
-
|
|
|
-- **修复方案**:
|
|
|
- 两种口径任选其一(建议第 2 种,更严格):
|
|
|
- 1. **禁止非 ADMIN 把 deptId 改为 0**(把原"只在 >0 时校验"变成"不等于当前 deptId 时都要校验"):
|
|
|
+- **建议**:
|
|
|
+ 1. 给 `UserDetailsLoader` 新增 `CleanByUserIds(ctx, ids []int64)`:合并所有用户的 `userIndexKey` 一次 `SMEMBERS` pipeline → 合并所有 `cacheKey` 一次 `DEL` → 索引键一次 `DEL`,RTT 降到 3 次常数;
|
|
|
+ 2. 如果仍想保留 `Clean` 单用户语义,在 `UpdateDeptLogic` 里改成一次 batch:
|
|
|
```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("无权将用户调入非自己管辖的部门")
|
|
|
- }
|
|
|
+ if deptTypeChanged || statusChanged {
|
|
|
+ userIds, err := l.svcCtx.SysUserModel.FindIdsByDeptId(l.ctx, req.Id)
|
|
|
+ if err != nil {
|
|
|
+ logx.WithContext(l.ctx).Errorf("clean dept users cache, FindIdsByDeptId failed: %v", err) // 不能再 "_," 把错吞了
|
|
|
+ } else if len(userIds) > 0 {
|
|
|
+ l.svcCtx.UserDetailsLoader.CleanByUserIds(l.ctx, userIds)
|
|
|
}
|
|
|
- deptId = *req.DeptId
|
|
|
}
|
|
|
```
|
|
|
- 2. 如果产品规范里本就不支持"用户无部门",应直接把 `0` 当作非法值:`if *req.DeptId <= 0 { return ErrBadRequest("部门ID无效") }`。
|
|
|
+ 3. 顺手把 `_, _ = FindIdsByDeptId(...)` 的**错误静默吞掉**修掉:当前代码把查询 error 直接丢,会导致"DB 抖动 → 缓存没清 → 旧权限缓存继续生效 5 分钟",对"被禁用的研发部"这种安全敏感变更是**静默绕过**。
|
|
|
|
|
|
---
|
|
|
|
|
|
-## ⚠️ 健壮性与性能建议 (Medium/Low)
|
|
|
+### M-2. `ProductList` / `ProductDetail` / `DeptTree` **对任意登录用户暴露全公司清单**
|
|
|
+
|
|
|
+- **位置**:
|
|
|
+ - `internal/logic/product/productListLogic.go:29-58`(无任何访问控制)
|
|
|
+ - `internal/logic/product/productDetailLogic.go:29-48`(无任何访问控制)
|
|
|
+ - `internal/logic/dept/deptTreeLogic.go:27-66`(无任何访问控制)
|
|
|
+- **描述**:
|
|
|
+ 三个接口都挂在 `JwtAuth` 中间件后面(见 `handler/routes.go:47-74, 119-146`),但 logic 层里只做了"caller 非超管时遮蔽 AppKey"这种**字段级**防护,没有行/资源级防护:
|
|
|
+ - 一个只属于产品 A 的 MEMBER 级账号,可以从 `ProductList` 拉到全站所有产品的 `code / name / remark / status`;
|
|
|
+ - 同样可以从 `ProductDetail` 按 ID 逐个拉别的产品的详情(只是看不到 AppKey);
|
|
|
+ - `DeptTree` 直接返回整棵组织架构树,包括 MEMBER 根本无权管的兄弟部门和叔辈部门;
|
|
|
|
|
|
-### M-1. `RefreshTokenLogic` **先解析 JWT,再限流**:无效 token 穿透限流,可用于爆破 refreshSecret
|
|
|
+ 这几个泄露叠加起来就是 H-1 的"可枚举 admin 用户名"的根源——一个 MEMBER 登录之后:
|
|
|
+ 1. `/api/product/list` → 拉到 `code` 列表 → 拼出 `admin_<code>` 全量用户名;
|
|
|
+ 2. `/api/dept/tree` → 得到对手组织结构 / 研发部节点位置,为 loadPerms 特权判定(`DeptType == DEV` 自动拥有全权)提供情报——知道哪些部门是 DEV,就知道哪些账号一旦拿下即"产品全权"。
|
|
|
+ 3. 枚举结果喂给 H-1 的 admin DoS 或针对性的钓鱼。
|
|
|
|
|
|
-- **位置**:`internal/logic/pub/refreshTokenLogic.go:40-73`
|
|
|
-- **描述**:`TokenOpLimiter.Take("refresh:%d")` 的 key 需要 `claims.UserId`,所以必须**先 `ParseRefreshToken` 成功**才能限流。对一批**攻击用的无效 token**(例如在爆破 refresh secret 时构造的伪造签名),全部会走到:
|
|
|
+- **建议**:
|
|
|
+ 1. `ProductList` / `ProductDetail`:**非超管只能看自己 `caller.ProductCode` 对应的那一条**。列表直接返回 `[自己产品]` 或空;详情校验 `product.Code == caller.ProductCode`。
|
|
|
+ 2. `DeptTree`:**非超管按 `caller.DeptPath` 剪枝返回**——只返回以 caller.DeptPath 为根的子树 + 与业务需要对齐的上级路径链(通常做法是回传 `ancestors` 字段而非整棵树)。
|
|
|
+ 3. 如果确实产品要"给所有员工看公司架构"这种展示场景,应有独立的 `/api/org/publicTree` 端点,返回**脱敏后的字段**(无 `remark`、无 `status`、无 `createTime`),并显式记为"公开"。
|
|
|
+
|
|
|
+---
|
|
|
+
|
|
|
+### M-3. `UserDetailLogic` 把 `Phone` / `Email` 返回给**任意同产品成员**
|
|
|
+
|
|
|
+- **位置**:`internal/logic/user/userDetailLogic.go:29-77`
|
|
|
+- **描述**:
|
|
|
```go
|
|
|
- claims, err := authHelper.ParseRefreshToken(tokenStr, l.svcCtx.Config.Auth.RefreshSecret)
|
|
|
- if err != nil {
|
|
|
- return nil, response.ErrUnauthorized("refreshToken无效或已过期")
|
|
|
+ if !caller.IsSuperAdmin {
|
|
|
+ if _, err := l.svcCtx.SysProductMemberModel.FindOneByProductCodeUserId(l.ctx, caller.ProductCode, req.Id); err != nil {
|
|
|
+ return nil, response.ErrForbidden("无权查看非本产品成员的用户信息")
|
|
|
+ }
|
|
|
}
|
|
|
+ ...
|
|
|
+ return &types.UserItem{
|
|
|
+ ...
|
|
|
+ Email: user.Email,
|
|
|
+ Phone: user.Phone,
|
|
|
+ ...
|
|
|
+ }, nil
|
|
|
```
|
|
|
- 不进入限流桶。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 等),建议补上。
|
|
|
+ 判定规则是"同产品成员即可查看"——一个产品里最低权限 MEMBER 可以遍历同产品所有用户(userId 范围内 fuzz)的手机 / 邮箱。同产品有几百上千成员时,这等同于**暴露公司通讯录 PII**。
|
|
|
+ 这里没有"**调用者对目标有管理权**"或"**看自己**"的更细粒度条款。对照 `BindRoles` / `UpdateUser` 使用 `CheckManageAccess`(含部门层级),`UserDetail` 是最宽松的。
|
|
|
+
|
|
|
+- **建议**:
|
|
|
+ 1. `Phone` / `Email` 应仅对满足以下之一的 caller 返回:
|
|
|
+ - `caller.UserId == user.Id`(自己);
|
|
|
+ - `caller.IsSuperAdmin || caller.MemberType == consts.MemberTypeAdmin`;
|
|
|
+ - `CheckManageAccess(...) == nil`(即调用者管辖该目标);
|
|
|
+ 其余 caller 只返回脱敏字段(例如 `13***1234`)或不返回。
|
|
|
+ 2. 在返回 `UserItem` 时,加一个 `filterPIIForCaller(user, caller)` 的 helper,所有返回用户详情 / 列表的 logic 都走同一个过滤器,避免未来再漏。
|
|
|
|
|
|
---
|
|
|
|
|
|
-### M-2. `UpdateDeptLogic` 对部门成员的 `UserDetailsLoader.Clean` 在 for 循环里**串行同步调用**
|
|
|
+### M-4. `BindRolePermsLogic` / `UpdateRoleLogic` 在**写成功后的缓存清理失败**时返回 500,客户端会把成功的写误判为失败并重试
|
|
|
|
|
|
-- **位置**:`internal/logic/dept/updateDeptLogic.go`(循环清缓存处)
|
|
|
-- **描述**:在 deptType / status 变更时,代码形如:
|
|
|
+- **位置**:
|
|
|
+ - `internal/logic/role/bindRolePermsLogic.go:128-134`
|
|
|
+ - `internal/logic/role/updateRoleLogic.go:79-85`
|
|
|
+- **描述**:
|
|
|
```go
|
|
|
- userIds, _ := l.svcCtx.SysUserModel.FindIdsByDeptId(l.ctx, req.Id)
|
|
|
- for _, uid := range userIds {
|
|
|
- l.svcCtx.UserDetailsLoader.Clean(l.ctx, uid)
|
|
|
+ if err := l.svcCtx.SysRolePermModel.TransactCtx(...); err != nil { return err }
|
|
|
+
|
|
|
+ affectedUserIds, err := l.svcCtx.SysUserRoleModel.FindUserIdsByRoleId(l.ctx, req.RoleId)
|
|
|
+ if err != nil {
|
|
|
+ logx.WithContext(l.ctx).Errorf("角色权限已更新但缓存清理失败 roleId=%d: %v", req.RoleId, err)
|
|
|
+ return response.NewCodeError(500, "权限已更新但缓存刷新失败,请稍后手动刷新")
|
|
|
}
|
|
|
+ l.svcCtx.UserDetailsLoader.BatchDel(l.ctx, affectedUserIds, role.ProductCode)
|
|
|
```
|
|
|
- 单次 `Clean` 内部包含 `SMEMBERS + DEL(批) + DEL(索引键) + SREM`(见 `userDetailsLoader.go:185-199, 221-230`),至少 4 次 Redis 往返。一个 500 人的部门 = 2000 次 Redis 同步 RTT 卡在请求线程里。这会直接阻塞 HTTP handler(默认 go-zero timeout 可能都兜不住)。
|
|
|
+ 出问题的细节在三个层面:
|
|
|
+ 1. `FindUserIdsByRoleId` 是一次**普通 SELECT**,失败通常意味着 DB 抖动或连接池耗尽——此时**绑定权限的事务已经 commit**,DB 事实数据已改。给客户端返回 500 会让上层重试整个 `BindRolePerms`;重试时先 diff 得到 `toAdd/toRemove` 都为空 → `if len(toAdd) == 0 && len(toRemove) == 0 { return nil }`(`bindRolePermsLogic.go:102-104`)→ **静默返回 200,但期间的业务逻辑 / 回调不会再触发**。更糟的变体:`BindRoles`(`user/bindRolesLogic.go:118-121`)的同一 pattern 里有 `UserDetailsLoader.Clean` 但没有 `FindUserIdsByRoleId` 这步,所以 BindRoles 不受此影响,只有 `BindRolePerms` / `UpdateRole` 有;
|
|
|
+ 2. `BatchDel` 失败只打 log 不改响应——那就让 `FindUserIdsByRoleId` 失败也同样处理才对称;
|
|
|
+ 3. 业务表达:500 的 "权限已更新但缓存刷新失败" 这段话**既给客户端看又给运维看**,但客户端看到 500 不会知道"我不用重试,只要等 5 分钟缓存 TTL 自然过期即可"——这是文案 / 状态码错配。
|
|
|
+
|
|
|
- **建议**:
|
|
|
- 1. loader 层加 `CleanMany(ctx, userIds)`:一次 `SMEMBERS` 取每个索引键,`DEL` 合并所有 `cacheKey`(Redis 支持任意多 key 的批量 DEL),索引键用一次 `DEL` 或 pipeline 批处理。
|
|
|
- 2. 或用 `BatchDel(userIds, productCode)`(已经在 loader 里)——不过 `BatchDel` 只清特定产品,对"部门跨多个产品"的场景要多次调用。更清爽的做法是按索引键一次性清:
|
|
|
+ 1. 把这两处改成 "事务成功即视为成功;缓存刷新失败仅 log + 异步重试":
|
|
|
```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
|
|
|
+ if err := ...TransactCtx(...); err != nil { return err }
|
|
|
+
|
|
|
+ if userIds, err := l.svcCtx.SysUserRoleModel.FindUserIdsByRoleId(l.ctx, req.RoleId); err == nil {
|
|
|
+ l.svcCtx.UserDetailsLoader.BatchDel(l.ctx, userIds, role.ProductCode)
|
|
|
+ } else {
|
|
|
+ logx.WithContext(l.ctx).Errorf("postcommit cache cleanup fetch userIds failed roleId=%d: %v", req.RoleId, err)
|
|
|
+ // TODO(可选):入异步队列重试,或记 audit 让运维手工刷新
|
|
|
}
|
|
|
+ return nil
|
|
|
```
|
|
|
- 3. 当部门用户数量超过阈值(如 200)时,**fire-and-forget 到后台 goroutine + 日志**,不要阻塞外部请求。
|
|
|
+ 2. 真的要让客户端知道 "权限变更成功但部分缓存未刷新",应返回 200 + resp 里带 `cacheRefreshStatus: "degraded"`,而不是 500。
|
|
|
|
|
|
---
|
|
|
|
|
|
-### M-3. `UserDetailsLoader.Load` 对**已删除用户**不做负缓存:一把有效 JWT + 删除账号 = 每次请求 7 条 DB 查询
|
|
|
+### M-5. `UpdateUserStatusLogic` / `BindRolesLogic` / `UpdateUserLogic` 的**同请求重复 `FindOne(targetUserId)`**(缓存命中但仍有 Redis 往返)
|
|
|
|
|
|
-- **位置**:`internal/loaders/userDetailsLoader.go:119-144`
|
|
|
+- **位置**:
|
|
|
+ - `internal/logic/user/updateUserStatusLogic.go:37-50`(3 次)
|
|
|
+ - `internal/logic/user/bindRolesLogic.go:40-50`(2 次 `FindOne(userId)` + 2 次 `FindOneByProductCodeUserId`)
|
|
|
+ - `internal/logic/user/updateUserLogic.go:47-58`(2 次 `FindOne(userId)`)
|
|
|
- **描述**:
|
|
|
- ```go
|
|
|
- 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}
|
|
|
- }
|
|
|
+ 以 `UpdateUserStatus` 为例:
|
|
|
```
|
|
|
- 当用户已被硬删除(`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 桶)。
|
|
|
+ ValidateStatusChange(...) → SysUserModel.FindOne(targetUserId)
|
|
|
+ CheckManageAccess(...) →
|
|
|
+ checkDeptHierarchy(...) → SysUserModel.FindOne(targetUserId)
|
|
|
+ checkPermLevel(...) → SysProductMemberModel.FindOneByProductCodeUserId(...)
|
|
|
+ // 回到 handler
|
|
|
+ user, _ := SysUserModel.FindOne(targetUserId)
|
|
|
+ ```
|
|
|
+ 全部通过 go-zero cache 命中,但每次 `FindOne` 还是要 Redis GET(若 Redis 上无 key 会触发 DB)。即便 Redis GET 是 0.5ms,3 次同一 key 就浪费 1.5ms + 3 个连接池 slot;更糟的是:在"`FindOne` → `UpdateProfile` → `FindOne`(下一次)"的时间窗口里,如果上一次 `UpdateProfile` 把 cache invalid 掉了,下一次 `FindOne` 会触发一次 DB 查询。**同一个 request 内部**本不该做这种往返。
|
|
|
|
|
|
- **建议**:
|
|
|
- 1. 对"Username 为空"的路径**也写入短 TTL(30~60s)的负缓存 sentinel**:
|
|
|
+ 1. 在 `CheckManageAccess` 签名里加一个可选 `prefetchedTarget *user.SysUser`:调用方已经有目标用户对象时,直接传进去,`checkDeptHierarchy` 复用;否则再查:
|
|
|
```go
|
|
|
- if ud.Username == "" {
|
|
|
- _ = l.rds.SetexCtx(ctx, key, `{"userId":0}`, 30) // 或带一个 "deleted":true 字段
|
|
|
- return nil, nil
|
|
|
+ func CheckManageAccess(ctx, svcCtx, targetUserId, productCode, opts ...Option) error {
|
|
|
+ var target *user.SysUser
|
|
|
+ for _, opt := range opts { opt.apply(&target) }
|
|
|
+ if target == nil {
|
|
|
+ target, _ = svcCtx.SysUserModel.FindOne(ctx, targetUserId)
|
|
|
+ }
|
|
|
+ ...
|
|
|
}
|
|
|
```
|
|
|
- 加载侧读到 sentinel 立刻返回空 `UserDetails`,不再走 DB。
|
|
|
- 2. 更彻底:引入**基于 userId 的全局"已删除"布隆过滤器**,在 `DeleteUser` 时添加;`Load` 读到命中时直接 short-circuit。
|
|
|
- 3. `jwtauthMiddleware` 对"用户被删除"的路径打印警告日志 + 对应 token 的 `userId` 加入短期封禁列表(5~10 分钟),避免垃圾 token 长期耗资源。
|
|
|
+ 2. 更激进:在 handler 最外层或 middleware 里做**请求级 cache**(`context.WithValue` 一个小 map),`FindOne`/`FindOneByProductCodeUserId` 走这层再透传。这对所有类似 `UpdateUser + Check*` 的组合都直接受益。
|
|
|
|
|
|
---
|
|
|
|
|
|
-### M-4. `UpdateWithOptLock` / `UpdateProfile` / `UpdatePassword` 的乐观锁**以秒级 `updateTime` 为版本**:同秒并发写会丢更新
|
|
|
+### M-6. `ExtractClientIP` 在 `behindProxy=true` 时**只信任 `X-Real-IP`**,没 `X-Forwarded-For` fallback;且**未设头时回落到 proxy 的 RemoteAddr → 全站共享一个桶**
|
|
|
|
|
|
-- **位置**:
|
|
|
- - `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`
|
|
|
+- **位置**:`internal/middleware/ratelimitMiddleware.go:54-65`
|
|
|
- **描述**:
|
|
|
- 乐观锁的 `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 网关重放;
|
|
|
- - 运维脚本并行刷数据。
|
|
|
+ ```go
|
|
|
+ func ExtractClientIP(r *http.Request, behindProxy bool) string {
|
|
|
+ if behindProxy {
|
|
|
+ if ip := r.Header.Get("X-Real-IP"); ip != "" { return ip }
|
|
|
+ }
|
|
|
+ host, _, err := net.SplitHostPort(r.RemoteAddr)
|
|
|
+ if err != nil { return r.RemoteAddr }
|
|
|
+ return host
|
|
|
+ }
|
|
|
+ ```
|
|
|
+ 两个真实业务场景会 bug:
|
|
|
+ 1. 部分运维 / 反向代理(特别是 K8s Ingress-nginx 默认配置)只设 `X-Forwarded-For`,不设 `X-Real-IP`。本实现会完全忽略 `X-Forwarded-For`,回落到 `r.RemoteAddr`——那个值是 ingress controller 的 IP → **全部请求共享一个 IP 限流桶**;
|
|
|
+ 2. 反向代理忘记把客户端 `X-Real-IP` 清掉的话,客户端能直接自己伪造 `X-Real-IP: 1.1.1.1`,每次变换轻松绕过限流。
|
|
|
+ 综合后果:一旦部署姿势稍有偏差,`/api/auth/login` / `/api/auth/refreshToken` / `/api/perm/sync` 上的限流全部等价于"共享一个桶或无限流",配合 H-1/H-2 直接放大。
|
|
|
|
|
|
- **建议**:
|
|
|
- 1. **引入独立的 `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 每次真会变**,上面两种方案任选其一先修)。
|
|
|
+ 1. 按常规反代优先级解析 IP:`X-Forwarded-For`(取第一段非私网)→ `X-Real-IP` → `RemoteAddr`;
|
|
|
+ 2. 显式校验解析结果是**合法的 IP**(`net.ParseIP(ip) != nil`),非法(空串、乱码)时退化到 `RemoteAddr`,并**打印 warn 日志**让运维能尽快发现代理链漏配;
|
|
|
+ 3. `behindProxy` 开关和线上部署姿势必须在 README 显式对齐;如果线上实际用的是 ingress-nginx,得 `behindProxy=true` + 解析 `X-Forwarded-For`。
|
|
|
|
|
|
---
|
|
|
|
|
|
-### M-5. `CreateProductLogic` 仍残留 `strings.Contains(errMsg, "uk_code")` 与 `strings.Contains(errMsg, req.Code)` 二级判定
|
|
|
+### M-7. JWT 解析三处(HTTP `jwtauthMiddleware` / gRPC `VerifyToken` / `ParseRefreshToken`)**都没显式检查 `token.Method`**
|
|
|
|
|
|
-- **位置**:`internal/logic/product/createProductLogic.go:135-146`
|
|
|
+- **位置**:
|
|
|
+ - `internal/middleware/jwtauthMiddleware.go:59-61`
|
|
|
+ - `internal/server/permserver.go:242-244`
|
|
|
+ - `internal/logic/auth/jwt.go:78-80`
|
|
|
- **描述**:
|
|
|
+ 三处 `keyfunc` 直接返回 `[]byte(secret)`,没有做:
|
|
|
```go
|
|
|
- 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("数据冲突,请稍后重试")
|
|
|
+ if _, ok := token.Method.(*jwt.SigningMethodHMAC); !ok {
|
|
|
+ return nil, fmt.Errorf("unexpected signing method: %v", token.Header["alg"])
|
|
|
}
|
|
|
```
|
|
|
- 第一步正确使用 `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 的精确文案拦住了。
|
|
|
+ `jwt/v4` 默认对 `alg=none` 是关闭的(`SigningMethodNone` 需要显式 `jwt.UnsafeAllowNoneSignatureType`),所以**目前没有立即可利用的漏洞**。但:
|
|
|
+ 1. 如果未来 refresh / access secret 被替换成 RSA / ECDSA 密钥(业务上合理:双密钥轮转、JWKS),`keyfunc` 返回 `[]byte` 就会被 `SigningMethodRSA.Verify` 当 public key 解析失败,这本身安全——但业务上会突然全站登录失败,调试困难;
|
|
|
+ 2. 更重要的是:显式 `token.Method` 检查是深度防御的行业标准(OWASP JWT Cheat Sheet),任何 review 过 JWT 代码的审计都会拉出来。
|
|
|
+- **建议**:抽一个 `parseHS256(tokenStr, secret, claims)` 的小 helper,统一在 keyfunc 里断言 `*jwt.SigningMethodHMAC`,并在 `ParseRefreshToken` / access token 解析两处调用。
|
|
|
|
|
|
---
|
|
|
|
|
|
-### M-6. `SyncPermsService.ExecuteSyncPerms`:**读快照在 tx 外、写在 tx 内**,并发同步会撞 `DuplicateEntry`
|
|
|
+### M-8. `IncrementTokenVersion` / `IncrementTokenVersionIfMatch` 都**先 `FindOne` 一次**只为拿 `username` 构造 cache key
|
|
|
|
|
|
-- **位置**:`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 不在请求列表里的旧权限 ...
|
|
|
- })
|
|
|
+- **位置**:`internal/model/user/sysUserModel.go:158-181, 186-214`
|
|
|
+- **描述**:
|
|
|
+ ```go
|
|
|
+ func (m *customSysUserModel) IncrementTokenVersionIfMatch(ctx context.Context, id, expected int64) (int64, error) {
|
|
|
+ data, err := m.FindOne(ctx, id) // ← 只为下面的 usernameKey
|
|
|
+ if err != nil { return 0, err }
|
|
|
+ sysUserIdKey := fmt.Sprintf("%s%v", cacheSysUserIdPrefix, id)
|
|
|
+ sysUserUsernameKey := fmt.Sprintf("%s%v", cacheSysUserUsernamePrefix, data.Username)
|
|
|
+ var newVersion int64
|
|
|
+ err = m.TransactCtx(ctx, ...)
|
|
|
+ _ = m.DelCacheCtx(ctx, sysUserIdKey, sysUserUsernameKey)
|
|
|
+ return newVersion, nil
|
|
|
+ }
|
|
|
```
|
|
|
- 两次并发 `SyncPermissions`:
|
|
|
- - 都读到 `existing` 中没有 code X;
|
|
|
- - 都 `InsertWithTx(..., code=X)`;
|
|
|
- - 第二次 tx 在 `UNIQUE (productCode, code)` 上撞 1062 → rollback。
|
|
|
+ 每次 `RefreshToken`(HTTP + gRPC 两条路径)都多一次 `FindOne` 走 cache-aside:
|
|
|
+ - Cache 命中:1 次 Redis GET(快);
|
|
|
+ - Cache 未命中:1 次 Redis GET + 1 次 DB SELECT + 1 次 Redis SETEX = 3 次 IO。
|
|
|
+ `RefreshToken` 已经调了 `UserDetailsLoader.Load`(含 `loadUser` 里的 `FindOne`)——同一请求内第二次 `FindOne`,完全是浪费;更关键的是:`RefreshToken` 的 cache flow 是 "Load → 比较 TokenVersion → IncrementIfMatch → Clean",`Clean` 会把 loader 缓存清掉但 **go-zero 的 `FindOne` 那一层 cache 不会被清**(不同 key 空间),所以下一次请求还是会再查一次。
|
|
|
|
|
|
- 当前错误处理把 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。
|
|
|
+ 1. 把 `IncrementTokenVersionIfMatch` 的第一次 `FindOne` 去掉,cache key 的 username 部分**改成读 ExecCtx 的"写入后新值"**(go-zero 支持把 `DelCacheCtx` 放进 `ExecCtx` 的 hook 里,只传 `sysUserIdKey`;`sysUserUsernameKey` 可以在事务内再 `session.QueryRowCtx` 拿 `username`,或者**干脆只删 ID key**——`FindOneByUsername` 的 cache 也只会因为密码 / status 等变化才会 stale,tokenVersion 变更并不会让"按 username 查"的结果在业务上过期);
|
|
|
+ 2. 或者让上游 `refreshTokenLogic.go` 在调用 `IncrementTokenVersionIfMatch` 时把 `ud.Username` 也传进来,省掉这次 FindOne:
|
|
|
+ ```go
|
|
|
+ IncrementTokenVersionIfMatch(ctx, userId, username, expected) (int64, error)
|
|
|
+ ```
|
|
|
+ logic 层已经手上有 `ud.Username`,直接透传即可。
|
|
|
|
|
|
---
|
|
|
|
|
|
-### M-7. gRPC `PermServer.Login` 的**对端 IP 提取 fail-open**
|
|
|
+### L-1. `SyncPermsService` 的"如需禁用所有权限请使用专用接口"是**文案指向幽灵端点**
|
|
|
|
|
|
-- **位置**:`internal/server/permserver.go`(`Login` 方法里对 `peer.FromContext` 和 `net.SplitHostPort` 的错误处理分支,clientIP 回退为 `"unknown"` 或原始 `p.Addr.String()`)
|
|
|
+- **位置**:`internal/logic/pub/syncPermsService.go:49-51`
|
|
|
- **描述**:
|
|
|
- - `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 的修复路径)。
|
|
|
+ ```go
|
|
|
+ if len(perms) == 0 {
|
|
|
+ return nil, &SyncPermsError{Code: 400, Message: "权限列表不能为空,如需禁用所有权限请使用专用接口"}
|
|
|
+ }
|
|
|
+ ```
|
|
|
+ 然而 `perm.api` / `routes.go` / gRPC `PermService` 里**并没有"禁用所有权限的专用接口"**。这行错误消息是历史设计遗留——要么这个接口被砍了,要么从来没实现过。接入方看到 400 文案会去找"专用接口",浪费排查时间。
|
|
|
+- **建议**:把文案改成客观事实"权限列表不能为空",并把"禁用所有权限"这条产品需求**显式 TODO 或删除**(真要支持,需要一个独立授权模型,不能走 appKey 就把产品的所有权限抹了——这本身是个有争议的能力)。
|
|
|
|
|
|
---
|
|
|
|
|
|
-### M-8. `ChangePasswordLogic` / `UpdatePassword`:**无乐观锁 + 并发修改密码双方都成功**
|
|
|
+### L-2. `DeleteDeptLogic` 的多次 `FOR UPDATE` 顺序**可能死锁**(真实可能性较低)
|
|
|
|
|
|
-- **位置**:
|
|
|
- - `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` 返回前端"请刷新后重试"。
|
|
|
+- **位置**:`internal/logic/dept/deleteDeptLogic.go:36-62`
|
|
|
+- **描述**:
|
|
|
+ 同一事务内依次:`SELECT dept_id FROM sys_dept FOR UPDATE` → `SELECT child ids FROM sys_dept WHERE parentId FOR UPDATE` → `SELECT user ids FROM sys_user WHERE deptId FOR UPDATE` → `DELETE sys_dept WHERE id`。顺序"sys_dept 行 → sys_dept 按 parentId 的范围锁 → sys_user 按 deptId 的范围锁"。
|
|
|
+
|
|
|
+ 如果另一 tx 在做 `CreateDept`(L3 的"`InsertWithTx → UpdateWithTx`"路径)或 `UpdateUserLogic`(先读 `sys_user` 再 `UpdateProfile`),两边获取锁的顺序不一致时理论上能构成 AB-BA。实际业务里:
|
|
|
+ - DeleteDept 仅超管调用,频率极低(删除部门是稀有操作);
|
|
|
+ - CreateDept 同样低频;
|
|
|
+ - 真正会撞的是 "DeleteDept 碰巧与大批量 UpdateUser deptId" 同时——这种场景通常不会主动并发;
|
|
|
+ 但审计视角上仍是一个"范围锁 → 可阻塞后续"的定时炸弹。
|
|
|
+- **建议**:
|
|
|
+ 1. 确保锁顺序在所有涉及 `sys_dept + sys_user` 的事务里一致(总是先锁 dept,再锁 user);
|
|
|
+ 2. `DeleteDept` 内可以把 `sys_user WHERE deptId = ? FOR UPDATE` 换成不加锁的 `SELECT ... LOCK IN SHARE MODE` + 事务隔离级 REPEATABLE READ(反正只是存在性判定),降低阻塞面。
|
|
|
|
|
|
---
|
|
|
|
|
|
-### L-1. `CreateUserLogic` 默认 `mustChangePassword = consts.MustChangePasswordNo`
|
|
|
+### L-3. `UserDetailsLoader.registerCacheKey` **每次都做 4 次 Redis 单独 RTT**(SAdd + Expire + SAdd + Expire)
|
|
|
|
|
|
-- **位置**:`internal/logic/user/createUserLogic.go`
|
|
|
-- **描述**:管理员为新用户创建账号时,密码是管理员"代填"的,业务上通常应**强制首登改密**。当前默认值是 `No`,意味着管理员口头告诉员工密码后,员工可以长期不改;如果管理员密码库泄露,所有新建账号都通用。
|
|
|
-- **建议**:把默认改为 `consts.MustChangePasswordYes`;或请求体加 `mustChangePassword *int` 字段,不传时按 `Yes` 处理。
|
|
|
+- **位置**:`internal/loaders/userDetailsLoader.go:220-238`
|
|
|
+- **描述**:每次 `Load` cache miss 后都会:
|
|
|
+ ```go
|
|
|
+ SADD ud:idx:u:<userId> <key>
|
|
|
+ EXPIRE ud:idx:u:<userId> <ttl+60>
|
|
|
+ SADD ud:idx:p:<productCode> <key>
|
|
|
+ EXPIRE ud:idx:p:<productCode> <ttl+60>
|
|
|
+ ```
|
|
|
+ 4 次独立 RTT。一个大产品启动瞬间(N 个并发用户都走 middleware 的首次 Load),`4*N` 次 RTT 打满 Redis cluster 的连接队列。
|
|
|
+- **建议**:用 go-zero redis 的 `Pipelined` 或 `TxPipelined` 批做,合并为 1 RTT;更激进可以把索引维护推到**异步通道**,Load 主路径不 wait。
|
|
|
|
|
|
---
|
|
|
|
|
|
-### L-2. `RefreshToken` 限流 key 仅含 `userId`,**不含 IP**
|
|
|
+### L-4. `Logout` 仍用**无条件 `IncrementTokenVersion`**(非 CAS)
|
|
|
|
|
|
-- **位置**:`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`)。
|
|
|
+- **位置**:`internal/logic/auth/logoutLogic.go:46`
|
|
|
+- **描述**:`RefreshToken` 已经切到 `IncrementTokenVersionIfMatch`,但 `Logout` 还是老版本 `IncrementTokenVersion`(无条件 `SET tokenVersion = tokenVersion+1`)。业务语义上 `Logout` 确实是"无论当前版本多少都失效",**所以目前正确**——但这也意味着:
|
|
|
+ 1. 在 `Logout` 和并发 `RefreshToken` 相撞时,`Logout` 可能在 `RefreshToken` 的 `IncrementTokenVersionIfMatch` 之后再 +1,导致刚签发的新 token 立即失效(体验问题,非安全问题);
|
|
|
+ 2. `IncrementTokenVersion` 实际只剩 `Logout` 一个调用点 + model 层的测试引用,继续保留这个"大杀器" API 会让未来新功能误用(比如某人在新增逻辑时图方便调了无条件 +1 版本)。
|
|
|
+- **建议**:
|
|
|
+ 1. 不强制替换 `Logout`(语义正确),但**把 `IncrementTokenVersion` 加上显式的安全注释**:"仅限业务语义为'强制全量失效'的场景(Logout / 封禁账号),**禁止**在 Refresh/Rotation 场景使用——Refresh 必须走 `IncrementTokenVersionIfMatch`"。
|
|
|
+ 2. 更激进:用 golang build tag / linter 限制 `IncrementTokenVersion` 的调用方范围(仅限 `auth/logoutLogic.go` + 未来的封禁接口)。
|
|
|
|
|
|
---
|
|
|
|
|
|
-### L-3. `CreateDeptLogic` 的 `InsertWithTx → FindOneWithTx → UpdateWithTx` 组合:**缓存早于事务提交失效**
|
|
|
+### L-5. `removeMemberLogic`:移除 ADMIN 前的 `CountActiveAdminsTx` **与目标成员自己的 state 判定耦合**
|
|
|
|
|
|
-- **位置**:`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 语句)。简化为:
|
|
|
+- **位置**:`internal/logic/member/removeMemberLogic.go:45-54`
|
|
|
+- **描述**:
|
|
|
```go
|
|
|
- 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)
|
|
|
+ if locked.MemberType == consts.MemberTypeAdmin && locked.Status == consts.StatusEnabled {
|
|
|
+ adminCount, err := l.svcCtx.SysProductMemberModel.CountActiveAdminsTx(ctx, session, member.ProductCode)
|
|
|
+ ...
|
|
|
+ if adminCount <= 1 { return "不能移除最后一个管理员" }
|
|
|
+ }
|
|
|
```
|
|
|
- 同时用 `m.DelCacheCtx(cacheSysDeptIdPrefix+deptId)` 显式在 tx 外做一次二次清理。
|
|
|
-
|
|
|
----
|
|
|
-
|
|
|
-### L-4. `CheckManageAccess` / `checkPermLevel` 把 `targetLevel = math.MaxInt64`(查不到角色)当"目标无权限"处理
|
|
|
-
|
|
|
-- **位置**:`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)` 时直接返回错误而不是降级放行。
|
|
|
+ `CountActiveAdminsTx` 是 `SELECT COUNT(*) WHERE productCode=? AND memberType='ADMIN' AND status='enabled'`——包括了**正要被删除的 locked 自己**。因此 `adminCount <= 1` 刚好拦住了"删最后一个",但边界判定非常 subtle:
|
|
|
+ - 若 `locked` 本身虽是 ADMIN 但 `status=disabled`(禁用但未删),流程会**跳过 admin-count 检查直接删除**——OK,因为他反正不是 active admin;
|
|
|
+ - 但如果 `locked.MemberType=ADMIN && locked.Status=Enabled` 且 DB 里另有一个 "ADMIN + Disabled" 的 stale 记录,count 不包含它——正确;
|
|
|
+ - 隐含约束:`CountActiveAdminsTx` 必须和 `locked` 的"active admin 定义"**完全一致**(MemberType=ADMIN 且 Status=Enabled)。如果未来新增一种 ADMIN 子类型(比如 SuperProductAdmin),这个 count 会漏算。
|
|
|
+- **建议**:把 `CountActiveAdminsTx` 改成返回"除了这个 id 之外还有几个 active admin":
|
|
|
+ ```sql
|
|
|
+ SELECT COUNT(*) FROM sys_product_member WHERE productCode = ? AND memberType = 'ADMIN' AND status = 1 AND id != ?
|
|
|
+ ```
|
|
|
+ 外层判定 `if adminCount == 0 → 最后一个`,语义更贴合业务,少一步反向推理。`UpdateMemberLogic` 的同名调用同样受益。
|
|
|
|
|
|
---
|
|
|
|
|
|
-### L-5. `CreateProductLogic.generateRandomHex` 的 tx 外密钥生成:**事务失败时密钥被泄露到日志 / 响应**
|
|
|
+### L-6. `CheckManageAccess` 在 caller `DeptId == 0` 时直接 403,漏了"**非 ADMIN 的超级管理员**"的心智模型
|
|
|
|
|
|
-- **位置**:`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 或要求创建者当场抄写,别持久化在响应体。
|
|
|
+- **位置**:`internal/logic/auth/access.go:155-162`
|
|
|
+- **描述**:
|
|
|
+ ```go
|
|
|
+ func checkDeptHierarchy(ctx, svcCtx, caller, targetUserId) error {
|
|
|
+ if caller.MemberType == consts.MemberTypeAdmin { return nil }
|
|
|
+ if caller.DeptId == 0 { return ErrForbidden("您未归属任何部门,无权管理其他用户") }
|
|
|
+ if caller.DeptPath == "" { return ErrForbidden("您的部门信息异常...") }
|
|
|
+ ...
|
|
|
+ }
|
|
|
+ ```
|
|
|
+ `CheckManageAccess` 最外层已经对 `caller.IsSuperAdmin` 做了 early return(`access.go:47`),所以 `checkDeptHierarchy` 进到这里一定不是超管——OK。
|
|
|
+ 但目前代码里"caller `DeptId=0`"这件事并不一定异常:**L1 中 H-4 的修复落地后**,普通 MEMBER 的 `DeptId` 绝不会是 0;但早期数据 / 迁移遗留数据里仍可能有 `DeptId=0` 的 MEMBER(例如从老系统搬来的账号)。这些账号会发现**任何管理操作都拒绝**,包括合法的"管理自己所在产品里的下属"——实际上 `checkPermLevel` 本来可以单独判定,但被 `checkDeptHierarchy` 先拦了。
|
|
|
+- **建议**:在无部门归属的 MEMBER 场景下,至少应允许"管理自己"和"纯 product-admin-downward"的操作;或者运维侧跑一次 data fix 把 `DeptId=0` 的 MEMBER 都归入"默认部门"。代码侧加一条 TODO 记录这个假设,便于未来发现幽灵账号时做定向修复。
|
|
|
|
|
|
---
|
|
|
|
|
|
@@ -468,25 +531,36 @@
|
|
|
|
|
|
| 优先级 | finding | 概要 |
|
|
|
| --- | --- | --- |
|
|
|
-| **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 降级 / 密钥生命周期 |
|
|
|
+| **P0** | H-1 | AdminLogin 限流仅用 username → 可远端批量锁死所有产品 admin 账号(配合 M-2 的 ProductList 枚举放大) |
|
|
|
+| **P0** | H-2 | `ValidateProductLogin` 在冻结账号路径跳过 bcrypt → 响应时间 + 明文错误双重账号状态 oracle |
|
|
|
+| **P0** | H-3 | SyncPermsService 的 FOR UPDATE 修复只做到基础设施层,service 层仍然 tx 外读 / tx 内写,1062 → 409 只是缓解不是根治 |
|
|
|
+| P1 | M-1 | UpdateDept 部门用户缓存仍串行 Clean(R5 M-2 未落地) |
|
|
|
+| P1 | M-2 | ProductList / ProductDetail / DeptTree 对任意登录用户无访问控制,组合放大 H-1 |
|
|
|
+| P1 | M-3 | UserDetail 向任意同产品成员泄露 Phone / Email(PII 超额披露) |
|
|
|
+| P1 | M-4 | BindRolePerms / UpdateRole 在 post-commit 缓存步骤失败返回 500 → 客户端误判失败 |
|
|
|
+| P2 | M-5 / M-6 / M-7 | 请求内重复 FindOne / ExtractClientIP 代理链解析不足 / JWT 未显式验签方法 |
|
|
|
+| P2 | M-8 | IncrementTokenVersionIfMatch 为拿 username 多 1 次 FindOne |
|
|
|
+| P3 | L-1 ~ L-6 | 幽灵端点文案 / 锁序风险 / Redis 往返 pipeline / 无 CAS 注释 / CountActiveAdmins 语义 / DeptId=0 心智模型 |
|
|
|
|
|
|
### 建议的修复次序
|
|
|
|
|
|
-1. **先修 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 代码中复现无误,均有可触发的真实业务 / 攻击路径。
|
|
|
+1. **P0 同批上线**:H-1 + H-2(登录侧信道 + DoS 一起打补丁),H-3(SyncPerms 的 FOR UPDATE 接上 `LockByCodeTx` + 改掉 mock)。**这三条是本轮最关键的质变修复,前两条决定账号安全边界,第三条决定同步的可靠性。**
|
|
|
+2. **P1 第二批**:
|
|
|
+ - M-1:`UserDetailsLoader.CleanByUserIds` + `UpdateDept` 批处理;
|
|
|
+ - M-2:ProductList / ProductDetail / DeptTree 的访问控制(非超管自动剪枝);
|
|
|
+ - M-3:`UserDetail` 的 PII filter;
|
|
|
+ - M-4:post-commit 缓存清理失败不再映射 500。
|
|
|
+3. **P2 配套**:
|
|
|
+ - M-5:`CheckManageAccess` 支持 prefetched target;
|
|
|
+ - M-6:`ExtractClientIP` 支持 `X-Forwarded-For` + IP 合法性校验;
|
|
|
+ - M-7:统一的 `parseHS256` helper 显式校验签名算法;
|
|
|
+ - M-8:`IncrementTokenVersionIfMatch` 接收 username 参数。
|
|
|
+4. **P3 收尾 / 日常清理**:L-1 ~ L-6。
|
|
|
+
|
|
|
+### 与第 5 轮的关系
|
|
|
+
|
|
|
+- 第 5 轮 **实际落地并复盘通过**:H-1(CAS)、H-2(gRPC 限流)、H-3(同级放行修 `<=`)、H-4(deptId=0 守门)、M-3(负缓存)、M-5(错误消息字符串匹配移除)、M-7(gRPC IP 剥 host:port)、M-8(ChangePassword 限流)、L-1(MustChangePasswordYes)、L-4(FindMinPermsLevelByUserIdAndProductCode 区分 NotFound)。
|
|
|
+- 第 5 轮 **未完全落地** / **遗留**:M-2(UpdateDept Clean 批处理)→ 本轮 M-1;M-6(SyncPerms FOR UPDATE)→ 本轮 H-3(因基础设施完成提高到 P0)。
|
|
|
+- 第 5 轮 **未覆盖**:登录侧信道(H-2)、admin DoS(H-1)、水平信息披露(M-2 / M-3)、post-commit 缓存失败的 500 误映射(M-4)、客户端 IP 提取不全(M-6)、JWT `token.Method` 断言(M-7)——本轮全部新增。
|
|
|
+
|
|
|
+> 说明:本轮 findings 均在当前 HEAD 代码中复现无误;H 级别问题均给出可触发的真实业务 / 攻击路径,而非纯理论风险。
|