|
@@ -1,566 +1,270 @@
|
|
|
-# 权限管理系统 - 深度代码审计报告(第 6 轮)
|
|
|
|
|
|
|
+# 权限管理系统 —— 深度代码审计报告(第 7 轮)
|
|
|
|
|
|
|
|
-> 审计范围:`/internal` 下全部非测试、非 `_gen.go` 生产代码(含 `internal/server/permserver.go`、HTTP logic / handler / middleware、loaders、model 定制层、svc、util)。
|
|
|
|
|
-> 审计时间:2026-04-19(第 5 轮修复复盘 + 深扫一轮新代码面)
|
|
|
|
|
-> 审计重点:
|
|
|
|
|
-> - **账号锁定 / 账号枚举**相关的侧信道 —— 第 5 轮未覆盖
|
|
|
|
|
-> - **SyncPermissions 并发修复的真正落地程度**(第 5 轮 M-6 给了基础设施 `LockByCodeTx`/`FindMapByProductCodeWithTx`,但实际 service 是否真的串行化)
|
|
|
|
|
-> - **UpdateDept 缓存失效批处理**:上轮 M-2 给的结论是"建议实现 CleanMany / pipeline",本轮确认其有无落地
|
|
|
|
|
-> - **读放大路径**(logic 层一次请求里 `FindOne(targetUserId)` 被调用 3~4 次)
|
|
|
|
|
-> - **水平越权 / 信息泄露**:ProductList / ProductDetail / DeptTree / UserDetail 对最低权限用户暴露面是否超额
|
|
|
|
|
-> - JWT 解析契约:`token.Method` 的显式校验
|
|
|
|
|
->
|
|
|
|
|
-> 相对第 5 轮:H-1(`IncrementTokenVersionIfMatch` CAS)、H-2(gRPC RefreshToken/VerifyToken 限流)、H-3(`GuardRoleLevelAssignable` 严格低于自身)、H-4(`UpdateUser` deptId=0 ADMIN 守门)、L-1(`MustChangePasswordYes` 默认开启)、L-4(`FindMinPermsLevelByUserIdAndProductCode` 区分 NotFound / 其它 err)均**已实际落地**(阅读 HEAD 代码验证)。M-3(`negativeCacheMarker` 负缓存)、M-5(`CreateProduct` 移除 `strings.Contains` 脆弱匹配)、M-7(gRPC Login IP 剥 host:port)也已落地。本轮暴露的是:**M-2 未落地**(部门用户缓存仍串行 Clean)、**M-6 只落地一半**(1062 → 409 已做,但 `LockByCodeTx` 在 service 里还没接上)、以及一组围绕**登录侧信道、管理员账号 DoS、info disclosure**的新 H/M 级问题。
|
|
|
|
|
|
|
+> **审计范围**:`/internal` 下全部非测试、非 `_gen.go` 生产代码(含 `internal/server/permserver.go`、HTTP logic / handler / middleware、loaders、model 定制层、svc、util、consts)。
|
|
|
|
|
+> **审计时间**:2026-04-19
|
|
|
|
|
+> **审计维度**:逻辑一致性 / 并发与 RMW / 资源管理 / 数据完整性 / 安全漏洞 / 边界坍塌 / DB 性能 / 僵尸代码 / 接口契约与对象完整性。
|
|
|
|
|
+> **与上一轮对比**:第 6 轮的 H-1 / H-2 / H-3 / M-1 / M-2 / M-4 / M-5 / M-6 / M-8 / L-1 / L-3 / L-5 均已在 HEAD 代码中落地。本轮聚焦**第 6 轮未修复项(M-3 / M-7 / L-2 / L-4 / L-6)**和**这一轮深挖出来的新漏洞**,尤其是`UserDetailsLoader.loadPerms` 中 **deny-list fail-open**、`AddMemberLogic` 的目标侧授权缺失等高风险项。
|
|
|
|
|
|
|
|
---
|
|
---
|
|
|
|
|
|
|
|
## 🚩 核心逻辑漏洞 (High Risk)
|
|
## 🚩 核心逻辑漏洞 (High Risk)
|
|
|
|
|
|
|
|
-### H-1. `AdminLogin` 限流 key 只含 `username`(不含 IP)——任意远端可**永久锁死任意超管账号**
|
|
|
|
|
-
|
|
|
|
|
-- **位置**:`internal/logic/pub/adminLoginLogic.go:41-46`
|
|
|
|
|
-- **描述**:
|
|
|
|
|
-
|
|
|
|
|
- ```go
|
|
|
|
|
- if l.svcCtx.UsernameLoginLimit != nil {
|
|
|
|
|
- code, _ := l.svcCtx.UsernameLoginLimit.Take(req.Username) // ← key 只有 username
|
|
|
|
|
- if code == limit.OverQuota {
|
|
|
|
|
- return nil, response.NewCodeError(429, "该账号登录尝试过于频繁,请5分钟后再试")
|
|
|
|
|
- }
|
|
|
|
|
- }
|
|
|
|
|
- ```
|
|
|
|
|
-
|
|
|
|
|
- `UsernameLoginLimit` 当前配置是 `NewPeriodLimit(300, 10, …)`(`servicecontext.go:45`),即**每 5 分钟 10 次**。同接口的产品端登录用 `ip:username` 作为 key(见 `loginService.go:40`),admin 路径**完全没有 IP 维度**。
|
|
|
|
|
- 攻击链:
|
|
|
|
|
- 1. 攻击者通过 `POST /api/auth/adminLogin` 用一个已知的超管用户名(比如 `admin_<productCode>`,见下面的结构性放大点)连打 10 次错误密码 → 触发 5 分钟封禁;
|
|
|
|
|
- 2. 攻击者每 5 分钟刷新一次,只需要极低带宽就能让该账号**永远没有 5 分钟内的合法登录窗口**;
|
|
|
|
|
- 3. 攻击者甚至不用在同一个 IP 上,任何网络都可以维持这个封禁循环;
|
|
|
|
|
- 4. 合法超管在任何 IP、任何地理位置都无法登录。
|
|
|
|
|
-
|
|
|
|
|
- **结构性放大点**(这才是真正让这条变 High 的地方):`CreateProductLogic.go:77` 里产品管理员账号的用户名是 `admin_<productCode>`,是**可从 `ProductList` 端点枚举出来的**(`ProductList` 不做任何访问控制,见 M-3):任何一枚普通登录账号都可以拉到 `code` 列表,立刻得到所有产品 admin 账号用户名。
|
|
|
|
|
-
|
|
|
|
|
- 攻击者因此可以:
|
|
|
|
|
- - **一次性把全站所有产品 admin 都锁掉**(`adminLogin` 不走 JWT 中间件,只需要能到达 `/api/auth/adminLogin`);
|
|
|
|
|
- - 配合"managementKey 即便泄露也仅定位到请求能被送出去"这一事实:整条链不要求攻击者有合法凭证,只要 `managementKey` 写错即可,每次请求都会在 `UsernameLoginLimit.Take(username)` 处计数(上面代码是先限流再做 managementKey 校验的顺序吗?——不是,managementKey 校验在第 37 行先做,失败直接 return,**不会走到 Take**)。
|
|
|
|
|
- - 但即便 managementKey 正确,用错密码还是会计数。也就是说:攻击者只要**一次 valid managementKey 泄露**(或运维错误把它 push 到 git),这个 DoS 立即变成"可长期维持"。
|
|
|
|
|
-
|
|
|
|
|
|
|
+### H-1. `UserDetailsLoader.loadPerms` 在 **deny 列表查询失败时 fail-open**,且把"少了 deny"的权限集写入 5 分钟缓存 —— 单次 DB 抖动 → 用户越权
|
|
|
|
|
+- **位置**:`internal/loaders/userDetailsLoader.go:456-499`
|
|
|
|
|
+- **描述**:普通成员的权限集计算顺序是:
|
|
|
|
|
+ 1. `FindPermIdsByUserIdAndEffectForProduct(allow)` —— 失败时 `return`,ud.Perms 保持 nil(fail-close,OK)。
|
|
|
|
|
+ 2. `FindPermIdsByUserIdAndEffectForProduct(deny)` —— 失败时**只 log,然后继续往下跑**,`denyIds` 为 nil,`denySet` 为空。
|
|
|
|
|
+ 3. 往 `permIdSet` 里塞 `rolePermIds + allowIds`,然后**直接把这个未经 deny 过滤的集合作为最终权限写回缓存**(缓存 TTL 5 分钟)。
|
|
|
|
|
+
|
|
|
|
|
+ `FindPermIdsByUserIdAndEffectForProduct` 走 `QueryRowsNoCacheCtx`,任何瞬时 DB 错误(连接池耗尽、slow query 触发 context deadline、主从漂移等)都会让 deny 查询失败。结果就是:
|
|
|
|
|
+ - 一个原本被 `effect=DENY` 显式撤销权限的用户,**立刻拿回这条被撤销的权限**;
|
|
|
|
|
+ - 并且这个"多出来的权限"被 `json.Marshal` 进 UD JSON 后写入 `ud:userId:productCode` Redis key,**持续 5 分钟**;
|
|
|
|
|
+ - 5 分钟内该用户所有请求(HTTP middleware / gRPC `GetUserPerms`)读取的都是这份"无 deny"权限集,**有多少次请求就有多少次越权**。
|
|
|
- **影响**:
|
|
- **影响**:
|
|
|
- - 所有产品的 ADMIN 账号被远端**静默批量锁死**,业务侧显示的只是"登录过于频繁,请 5 分钟后再试",没有任何 IP 信号可被风控切入;
|
|
|
|
|
- - 与 L-5(`ExtractClientIP` 在 `behindProxy=true` 时信任 `X-Real-IP`)叠加:攻击者发假 `X-Real-IP` 不会绕过 admin 限流(因为 key 根本没用 IP),反而会让产品登录的限流绕过——两条攻击面互补;
|
|
|
|
|
- - 根本上违反了 OWASP ASVS 2.2.1"按用户限流必须包含 IP 或设备维度"。
|
|
|
|
|
-
|
|
|
|
|
-- **修复方案**:
|
|
|
|
|
- 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`
|
|
|
|
|
-- **描述**:
|
|
|
|
|
- 关键顺序:
|
|
|
|
|
|
|
+ - **任意 deny-revoke 授权操作在单次瞬时 DB 抖动下 5 分钟内失效**。考虑到 `setUserPermsLogic` 的主要用途就是"临时撤销某用户对敏感权限的访问",这类 deny 往往就是最后一道安全闸。闸被 silently 打开 5 分钟。
|
|
|
|
|
+ - 攻击者若能制造一次对 `sys_user_perm` 的短时读失败(例如对该表发起 hot-row 争抢使 `denyIds` 的查询 timeout),即可让目标用户的 deny 被旁路。
|
|
|
|
|
+ - 与 R6 H-3 / H-4 不同,这是一条纯代码路径问题,不依赖配置、不依赖代理头,**出现概率等于 DB 抖动概率**。
|
|
|
|
|
+- **修复方案**:把两次查询在错误语义上**对称**处理:
|
|
|
```go
|
|
```go
|
|
|
- u, err := svcCtx.SysUserModel.FindOneByUsername(ctx, username) // L50
|
|
|
|
|
|
|
+ denyIds, err := l.models.SysUserPermModel.FindPermIdsByUserIdAndEffectForProduct(ctx, ud.UserId, consts.PermEffectDeny, ud.ProductCode)
|
|
|
if err != nil {
|
|
if err != nil {
|
|
|
- if errors.Is(err, user.ErrNotFound) {
|
|
|
|
|
- bcrypt.CompareHashAndPassword(dummyBcryptHash, []byte(password)) // L53:等时 dummy
|
|
|
|
|
- return nil, &LoginError{Code: 401, Message: "用户名或密码错误"}
|
|
|
|
|
- }
|
|
|
|
|
- return nil, err
|
|
|
|
|
- }
|
|
|
|
|
- if u.Status != consts.StatusEnabled {
|
|
|
|
|
- return nil, &LoginError{Code: 403, Message: "账号已被冻结"} // L59-60:**没有 bcrypt**
|
|
|
|
|
- }
|
|
|
|
|
- if err := bcrypt.CompareHashAndPassword([]byte(u.Password), []byte(password)); err != nil {
|
|
|
|
|
- return nil, &LoginError{Code: 401, Message: "用户名或密码错误"} // L63-65:有 bcrypt
|
|
|
|
|
|
|
+ logx.WithContext(ctx).Errorf("userDetailsLoader: load deny perms failed: %v", err)
|
|
|
|
|
+ return // fail-close:宁可让用户看到 0 perms 让他们刷新,也不能把 deny 旁路
|
|
|
}
|
|
}
|
|
|
```
|
|
```
|
|
|
- 第一个分支(用户名不存在)**有意**做了 dummy bcrypt 等时。但第二个分支(账号被冻结)**直接 return,跳过了 bcrypt**。三种输入的耗时差异:
|
|
|
|
|
- | 输入 | 耗时 | 响应消息 | code |
|
|
|
|
|
- | ------------------------------- | -------------------------- | ------------------ | ---- |
|
|
|
|
|
- | 用户名不存在 | ≈ bcrypt(dummy) | "用户名或密码错误" | 401 |
|
|
|
|
|
- | 用户名存在但 Status=2(冻结) | ≪ bcrypt(直接跳过) | **"账号已被冻结"** | 403 |
|
|
|
|
|
- | 用户名存在,密码错 | ≈ bcrypt | "用户名或密码错误" | 401 |
|
|
|
|
|
- | 用户名存在,密码对 | ≈ bcrypt + 后续一串 DB IO | 登录成功 | 200 |
|
|
|
|
|
-
|
|
|
|
|
- 两种独立的侧信道各自就足够用了:
|
|
|
|
|
- - **响应消息 + HTTP code**:`403 "账号已被冻结"` 是独一无二的(`401 "用户名或密码错误"` 覆盖了"不存在"和"密码错"两种)——攻击者每次请求都在做**一次无条件的账号存在性 + 状态 oracle**。
|
|
|
|
|
-
|
|
|
|
|
- 实际攻击路径:
|
|
|
|
|
- 1. 人员离职后,HR 走流程把账号状态置为冻结 → `Status=2` 驻留在 DB;这时 refreshToken 还没过期(refreshExpire 往往是 7~30 天);
|
|
|
|
|
- 2. 攻击者用公司常见命名规则批量扫一遍 `zhangsan / lisi / admin_xxx`,非 403 都排除,只留 403 的一批 → **这一批就是离职但还在 JWT 窗口里的高价值目标**;
|
|
|
|
|
- 3. 攻击者用钓鱼 / credentials stuffing / 内部 IM 撬这一批账号的 refreshToken,命中率显著高于随机喷撒。
|
|
|
|
|
-
|
|
|
|
|
- 同样的 pattern 也出现在 `adminLoginLogic.go:57-67`(有 bcrypt 再检查 status/superAdmin,但错误 message 都归一到"用户名或密码错误"——**这一版 adminLogin 做对了**)。证明开发团队知道这个 pattern,但 product login 这条没同步修。
|
|
|
|
|
|
|
+ 同时**本次加载不要写缓存**,交给下一次 Load 重试,或者把失败信号往上传(见 M-1)。
|
|
|
|
|
|
|
|
|
|
+### H-2. `UserDetailLogic` / `UserListLogic` 仍把 `Email` / `Phone` / `Remark` 暴露给**任意同产品成员**(R6 M-3 未落地)
|
|
|
|
|
+- **位置**:
|
|
|
|
|
+ - `internal/logic/user/userDetailLogic.go:68-70`
|
|
|
|
|
+ - `internal/logic/user/userListLogic.go:81-83`
|
|
|
|
|
+- **描述**:两接口的访问控制是"同产品 → 返回完整资料"。`UserDetail` 仅用 `FindOneByProductCodeUserId` 检查目标与调用方在同一产品内,就返回 `Email`、`Phone`、`Remark`(纯文本,无脱敏)。`UserList` 更严重 —— 一次分页可以批量拿到同产品所有成员的手机号与邮箱。
|
|
|
- **影响**:
|
|
- **影响**:
|
|
|
- - 账号存在性 / 冻结态**单次请求可探测**。配合 H-1(admin 账号可从 ProductList 枚举),攻击者可以画出全组织的**"谁在,谁离职了,谁被短期冻结了"**的状态图谱;
|
|
|
|
|
- - 违反 OWASP ASVS V2.1.12 "系统 shall not reveal account status or existence";
|
|
|
|
|
- - 离职用户处于"冻结但 JWT 还在期内"的短窗(几天)是最容易被 social engineering 命中的。
|
|
|
|
|
-
|
|
|
|
|
|
|
+ - **同产品最低权限 MEMBER 即可遍历整个产品通讯录**,获取手机 / 邮箱 / 备注(备注里可能含 PII / 内部身份)。
|
|
|
|
|
+ - 与 H-3(`AddMember` 不做目标侧授权)组合后威力更强:一个产品 ADMIN 可先把想看的任意人(包括其部门树外、不归自己管的用户)强行拉入自己的产品,再通过 `UserDetail` 或 `UserList` 抽走其 PII。
|
|
|
|
|
+ - 严重违反 GDPR / 《个人信息保护法》最小必要原则。
|
|
|
- **修复方案**:
|
|
- **修复方案**:
|
|
|
- 把 bcrypt 变成**无条件总是执行**,状态 / superAdmin 禁令走**登录成功之后的统一检查**,并且**所有用户侧可见的失败消息归一**为"用户名或密码错误":
|
|
|
|
|
|
|
+ 1. 新增 `authHelper.CanViewContact(caller *UserDetails, target *SysUser, targetMember) bool`,只在以下任一条件时返回 `true`:
|
|
|
|
|
+ - caller.IsSuperAdmin;
|
|
|
|
|
+ - target.UserId == caller.UserId(看自己);
|
|
|
|
|
+ - caller 对 target 的 `CheckManageAccess` 通过(即 caller 在管理链上)。
|
|
|
|
|
+ 2. 两个 Logic 返回 DTO 前统一走 `filterPIIForCaller`,其余情况把 `Email / Phone / Remark` 置空或做掩码(如 `138****1234`)。
|
|
|
|
|
+ 3. 单元测试覆盖"同产品同级成员互看"、"跨部门互看"、"ADMIN 看下级"、"看自己"四条。
|
|
|
|
|
+
|
|
|
|
|
+### H-3. `AddMemberLogic` 缺失**目标侧 `CheckManageAccess` + 超管防御**,产品 ADMIN 可把部门树外 / 超管用户拉入自己产品
|
|
|
|
|
+- **位置**:`internal/logic/member/addMemberLogic.go:41-75`
|
|
|
|
|
+- **描述**:`AddMember` 仅做了三件事:
|
|
|
|
|
+ - `RequireProductAdminFor(req.ProductCode)`:caller 是该产品 ADMIN 或超管。
|
|
|
|
|
+ - `CheckMemberTypeAssignment(req.MemberType)`:caller 允许分配这种 MemberType。
|
|
|
|
|
+ - `FindOneByProductCodeUserId` 排查重复加入。
|
|
|
|
|
+
|
|
|
|
|
+ 缺失的两道防线:
|
|
|
|
|
+ 1. **对 `req.UserId` 目标的 `CheckManageAccess`**:没有任何基于 `DeptPath` 的部门链校验,也没有任何基于 `MinPermsLevel` 的权限级校验。产品 ADMIN 可以把**自己部门树之外的用户**(例如 HR 部、财务部员工)强行拉入自己的产品。
|
|
|
|
|
+ 2. **对 `targetUser.IsSuperAdmin` 的显式拒绝**:如果系统中存在"超管但未主动加入任何产品"的账号,产品 ADMIN 可通过 `AddMember` 把这个超管拉入自己产品成为 MEMBER。虽然 `loadMembership` 会在 `ud.IsSuperAdmin == true` 时把 `MemberType` 固定为 `SuperAdmin`(实际权限没被限制),但这在审计日志里会留下一条"product_admin 把 super_admin 纳入自己产品"的假成员关系,**为后续权限推理工具 / 审计系统制造混淆**,也是第一步社工放大点。
|
|
|
|
|
+- **影响**:
|
|
|
|
|
+ - 与 H-2(PII 暴露)组合:ADMIN 随意从部门树外拉人入产品,然后读全员手机邮箱。**这是实际上最容易被滥用的越权路径**。
|
|
|
|
|
+ - 与 `UpdateMemberLogic` 组合:拉入后还可以赋予任何允许的 `MemberType`,制造跨部门的管理权扩张。
|
|
|
|
|
+- **修复方案**:`RequireProductAdminFor` / `CheckMemberTypeAssignment` 之后、`Insert` 之前,追加如下两段:
|
|
|
```go
|
|
```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 targetUser.IsSuperAdmin == consts.IsSuperAdminYes {
|
|
|
|
|
+ return nil, response.ErrForbidden("无法将超级管理员加入具体产品")
|
|
|
}
|
|
}
|
|
|
- if u.Status != consts.StatusEnabled {
|
|
|
|
|
- // 密码正确的冻结用户,才提示冻结(此时攻击者已经猜中密码,再保留"冻结"已无意义)
|
|
|
|
|
- return nil, &LoginError{Code: 403, Message: "账号已被冻结"}
|
|
|
|
|
- }
|
|
|
|
|
- if u.IsSuperAdmin == consts.IsSuperAdminYes {
|
|
|
|
|
- return nil, &LoginError{Code: 403, Message: "超级管理员请使用管理后台登录"}
|
|
|
|
|
|
|
+ if err := authHelper.CheckManageAccess(l.ctx, l.svcCtx, req.UserId, req.ProductCode,
|
|
|
|
|
+ authHelper.WithPrefetchedTarget(targetUser)); err != nil {
|
|
|
|
|
+ return nil, err
|
|
|
}
|
|
}
|
|
|
```
|
|
```
|
|
|
- 效果:**账号不存在 / 冻结 / 密码错**三者对外完全不可区分——响应耗时一致、消息一致、code 一致。只有密码正确后才"奖励性"地暴露后续语义。
|
|
|
|
|
|
|
+ 注意:`CheckManageAccess` 内部对 "caller 是 ADMIN" 会短路 `checkDeptHierarchy`,所以这不会让产品 ADMIN 失去管理自己下属 / 旁部门用户的能力,但会拦住"部门树外 + 不归自己管"这类真正的越权路径。
|
|
|
|
|
|
|
|
----
|
|
|
|
|
-
|
|
|
|
|
-### H-3. `SyncPermsService` 并发 1062 修复**只落地一半**:`LockByCodeTx` 已实现但没在 service 里调用
|
|
|
|
|
-
|
|
|
|
|
-- **位置**:`internal/logic/pub/syncPermsService.go:66-120`、`internal/model/product/sysProductModel.go:56-63`
|
|
|
|
|
-- **描述**:
|
|
|
|
|
- 第 5 轮 M-6 建议的完整修复是"FOR UPDATE 锁 product → tx 内 load existing → tx 内写"。当前 HEAD:
|
|
|
|
|
- - `sysProductModel.LockByCodeTx` 已经实现(`sysProductModel.go:56-63`);
|
|
|
|
|
- - `sysPermModel.FindMapByProductCodeWithTx` 已实现(`sysPermModel.go:98-109`);
|
|
|
|
|
- - **但 `syncPermsService.go` 完全没有调用这两个**——`existingMap` 仍然在 tx 外被 `FindMapByProductCode` 读(L66),tx 内只做写(L106-120)。
|
|
|
|
|
-
|
|
|
|
|
- 代码里留了一段自认的注释:
|
|
|
|
|
- ```go
|
|
|
|
|
- // NOTE(R5-M-6):理想方案是"同 tx 内先 SELECT ... FOR UPDATE 锁 sys_product 行…";
|
|
|
|
|
- // 但当前 mock 契约(syncPermsLogic_mock_test.go)把 FindMapByProductCodeWithTx 固定在 tx 外,
|
|
|
|
|
- // 为不破坏测试约定,保留了原先的"tx 外预读 + tx 内写入"结构。
|
|
|
|
|
- ```
|
|
|
|
|
- 问题:
|
|
|
|
|
- 1. 被测试约束反向拽住 = 架构决策被测试约束倒挂。正确做法是修测试 mock,让生产代码符合正确语义,而不是反过来;
|
|
|
|
|
- 2. "1062 → 409" 的兜底只是缓解症状:调用方看到 409 要重试,极端情况两个并发同步**同时在重试**仍可能继续撞,形成活锁;
|
|
|
|
|
- 3. 实际并发:同一个产品多部署实例启动时都会调一次 `POST /perm/sync`(部署流水线常见),两边都在热启动瞬间命中——409 并不比 500 友好多少,只是重试路径成立了。
|
|
|
|
|
-
|
|
|
|
|
-- **影响**:
|
|
|
|
|
- - 同步接口在部署时**概率性失败**,客户端必须自己做重试(而且当前 `SyncPermsError` 结构只给 Code/Message,没有 Retry-After 提示);
|
|
|
|
|
- - 上一轮的修复已经把基础设施准备好了(`LockByCodeTx` 是用代码+测试覆盖过的),**缺的只是最后接上**——这是一条"几十行代码 + 改 mock"可以直接落地的事情,风险收益比极高。
|
|
|
|
|
-
|
|
|
|
|
-- **修复方案**:
|
|
|
|
|
- 把 mock 测试里的 `FindMapByProductCode` 预期调用改为 `FindMapByProductCodeWithTx`,然后把 service 改为:
|
|
|
|
|
|
|
+### H-4. JWT 解析三处 `keyfunc` **未显式断言 `*jwt.SigningMethodHMAC`**(R6 M-7 未落地)
|
|
|
|
|
+- **位置**:
|
|
|
|
|
+ - `internal/middleware/jwtauthMiddleware.go:59-61`(HTTP access token)
|
|
|
|
|
+ - `internal/server/permserver.go:242-244`(gRPC access token)
|
|
|
|
|
+ - `internal/logic/auth/jwt.go:78-80`(refresh token)
|
|
|
|
|
+- **描述**:三处都是直接 `return []byte(secret), nil`,不检查 `token.Method` 类型。当前使用 `jwt/v4` 且 `accessSecret` / `refreshSecret` 都是对称密钥,不受 `alg=none` 攻击,但这是**深度防御盲区**:
|
|
|
|
|
+ - 如果未来迁移到 RSA/ECDSA 非对称密钥,而 `keyfunc` 仍然把 `[]byte` 塞进去,攻击者可以用**把公钥当 HMAC 密钥**的经典手法伪造 token —— 这在 jwt-go 历史上是实打实出过 CVE 的(CVE-2016-10555 同类问题)。
|
|
|
|
|
+ - 即使密钥不换,线上一旦因为误配生成出 `alg=HS512` 的 token,也不会被明确拒绝,而是当作 HS256 尝试解析,带来噪音和潜在被动兼容。
|
|
|
|
|
+- **影响**:当前配置下不构成直接漏洞,但违反 OWASP JWT Cheat Sheet、RFC 8725(JWT Best Current Practice)对 `alg` 白名单的强制要求。
|
|
|
|
|
+- **修复方案**:抽出一个通用 helper,三处统一调用:
|
|
|
```go
|
|
```go
|
|
|
- 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: "产品不存在"}
|
|
|
|
|
|
|
+ // internal/logic/auth/jwt.go
|
|
|
|
|
+ func parseWithHMAC(tokenStr, secret string, claims jwt.Claims) (*jwt.Token, error) {
|
|
|
|
|
+ return jwt.ParseWithClaims(tokenStr, claims, func(token *jwt.Token) (interface{}, error) {
|
|
|
|
|
+ if _, ok := token.Method.(*jwt.SigningMethodHMAC); !ok {
|
|
|
|
|
+ return nil, fmt.Errorf("unexpected signing method: %v", token.Header["alg"])
|
|
|
}
|
|
}
|
|
|
- return err
|
|
|
|
|
- }
|
|
|
|
|
- // 2. tx 内读 existing
|
|
|
|
|
- existingMap, err := svcCtx.SysPermModel.FindMapByProductCodeWithTx(txCtx, session, product.Code)
|
|
|
|
|
- if err != nil { return err }
|
|
|
|
|
- // 3. tx 内写(按原逻辑分 insert / update / disable)
|
|
|
|
|
- ...
|
|
|
|
|
- })
|
|
|
|
|
|
|
+ return []byte(secret), nil
|
|
|
|
|
+ })
|
|
|
|
|
+ }
|
|
|
```
|
|
```
|
|
|
- 当 `LockByCodeTx` 把同一 product 的并发同步串行化后,`ON DUPLICATE KEY UPDATE` / 1062 兜底都可以不再依赖。
|
|
|
|
|
|
|
|
|
|
---
|
|
---
|
|
|
|
|
|
|
|
-## ⚠️ 健壮性与性能建议 (Medium/Low)
|
|
|
|
|
-
|
|
|
|
|
-### M-1. `UpdateDeptLogic` 的 Clean 循环**仍未批处理**(第 5 轮 M-2 未落地)
|
|
|
|
|
|
|
+## ⚠️ 健壮性与性能建议 (Medium / Low)
|
|
|
|
|
|
|
|
-- **位置**:`internal/logic/dept/updateDeptLogic.go:86-94`
|
|
|
|
|
|
|
+### M-1. `UserDetailsLoader.Load` 把 "DB 瞬时故障" 同化为 "用户不存在",副作用是**把半成品 UD 写入 5 分钟缓存**
|
|
|
|
|
+- **位置**:`internal/loaders/userDetailsLoader.go:119-165, 284-304`
|
|
|
- **描述**:
|
|
- **描述**:
|
|
|
- ```go
|
|
|
|
|
- if deptTypeChanged || statusChanged {
|
|
|
|
|
- userIds, _ := l.svcCtx.SysUserModel.FindIdsByDeptId(l.ctx, req.Id)
|
|
|
|
|
- for _, uid := range userIds {
|
|
|
|
|
- l.svcCtx.UserDetailsLoader.Clean(l.ctx, uid)
|
|
|
|
|
- }
|
|
|
|
|
- ...
|
|
|
|
|
- }
|
|
|
|
|
- ```
|
|
|
|
|
- 每次 `Clean` 内部是 `SMEMBERS → DEL(批) → DEL(索引)`(`userDetailsLoader.go:204-218`),即 **3 个 Redis RTT**。一个有 300 个成员的部门改 deptType 会串行产生 900 次 RTT,卡在 HTTP handler 里。即使 Redis 0.5ms,300 个用户也至少 450ms——这个窗口内 handler 被 hold 住,其他写入堆积,连带拖慢 UpdateDept 的乐观锁重试。
|
|
|
|
|
- 第 5 轮已把问题点清楚暴露,并给出 `CleanByUserIds` 的草案,但**本轮代码里没有这个方法**,`cleanByIndex` 也没升级成 pipeline。
|
|
|
|
|
-
|
|
|
|
|
|
|
+ - `loadFromDB` 在 `loadUser` 失败(非 NotFound)时返回 `ud, err`;在 `Load` 的 singleflight 闭包里转化为 `(nil, err)`。
|
|
|
|
|
+ - 但是 `Load` 最后的 `if !ok || ud == nil` 分支会**构造一个空 UD 返回**。HTTP `jwtauthMiddleware` 看到 `ud.Username == ""` 就直接 401 "用户不存在或已被删除"。
|
|
|
|
|
+ - 用户体验:**一次 DB 抖动 → 全站在线用户被踢出,客户端清 token 重新登录 → 登录又打 DB → 进一步加重 DB → 雪崩**。
|
|
|
|
|
+ - 更隐蔽的是:`loadDept / loadProduct / loadMembership / loadRoles / loadPerms` 这五个子步骤里的**任何错误都是 log + 静默继续**。然后 `Load` 在 singleflight 成功分支里照常 `json.Marshal(ud)` 并 `SetexCtx` 写入缓存。于是当 dept 表抖动时,用户的 `DeptPath / DeptType` 变空白,`checkDeptHierarchy` 直接 403(除非 caller 是 ADMIN/超管),这份"半残" UD 还会被缓存 5 分钟。
|
|
|
|
|
+- **影响**:
|
|
|
|
|
+ - 雪崩风险:单次 DB 抖动 → 全站强制退出登录,客户端反复重试。
|
|
|
|
|
+ - 半加载缓存污染:用户在 5 分钟内会遇到"莫名其妙的 403",且运维看监控是绿的(因为错误被 log 吞了)。
|
|
|
|
|
+ - 与 H-1 叠加:`loadPerms` 的 deny 失败同样落入这个"半加载也写缓存"的通道。
|
|
|
- **建议**:
|
|
- **建议**:
|
|
|
- 1. 给 `UserDetailsLoader` 新增 `CleanByUserIds(ctx, ids []int64)`:合并所有用户的 `userIndexKey` 一次 `SMEMBERS` pipeline → 合并所有 `cacheKey` 一次 `DEL` → 索引键一次 `DEL`,RTT 降到 3 次常数;
|
|
|
|
|
- 2. 如果仍想保留 `Clean` 单用户语义,在 `UpdateDeptLogic` 里改成一次 batch:
|
|
|
|
|
- ```go
|
|
|
|
|
- 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)
|
|
|
|
|
- }
|
|
|
|
|
- }
|
|
|
|
|
- ```
|
|
|
|
|
- 3. 顺手把 `_, _ = FindIdsByDeptId(...)` 的**错误静默吞掉**修掉:当前代码把查询 error 直接丢,会导致"DB 抖动 → 缓存没清 → 旧权限缓存继续生效 5 分钟",对"被禁用的研发部"这种安全敏感变更是**静默绕过**。
|
|
|
|
|
-
|
|
|
|
|
----
|
|
|
|
|
-
|
|
|
|
|
-### M-2. `ProductList` / `ProductDetail` / `DeptTree` **对任意登录用户暴露全公司清单**
|
|
|
|
|
|
|
+ 1. `Load` 返回 `(*UserDetails, error)`,让中间件自己区分 "NotFound → 401 用户不存在" 与 "其他错误 → 503 服务暂时不可用"。
|
|
|
|
|
+ 2. `loadFromDB` 里任何子步骤出错,都**不要写缓存**(让下次 Load 重试)。
|
|
|
|
|
+ 3. 如果还是想保留无 error 返回,至少在 ud 上加一个 `PartiallyLoaded bool` 字段,Load 写缓存前检查这一位。
|
|
|
|
|
|
|
|
|
|
+### M-2. `SysUserModel.UpdatePassword` / `UpdateStatus` **不校验 `RowsAffected`**,对已删除 / 条件不满足的用户会静默成功
|
|
|
- **位置**:
|
|
- **位置**:
|
|
|
- - `internal/logic/product/productListLogic.go:29-58`(无任何访问控制)
|
|
|
|
|
- - `internal/logic/product/productDetailLogic.go:29-48`(无任何访问控制)
|
|
|
|
|
- - `internal/logic/dept/deptTreeLogic.go:27-66`(无任何访问控制)
|
|
|
|
|
-- **描述**:
|
|
|
|
|
- 三个接口都挂在 `JwtAuth` 中间件后面(见 `handler/routes.go:47-74, 119-146`),但 logic 层里只做了"caller 非超管时遮蔽 AppKey"这种**字段级**防护,没有行/资源级防护:
|
|
|
|
|
- - 一个只属于产品 A 的 MEMBER 级账号,可以从 `ProductList` 拉到全站所有产品的 `code / name / remark / status`;
|
|
|
|
|
- - 同样可以从 `ProductDetail` 按 ID 逐个拉别的产品的详情(只是看不到 AppKey);
|
|
|
|
|
- - `DeptTree` 直接返回整棵组织架构树,包括 MEMBER 根本无权管的兄弟部门和叔辈部门;
|
|
|
|
|
-
|
|
|
|
|
- 这几个泄露叠加起来就是 H-1 的"可枚举 admin 用户名"的根源——一个 MEMBER 登录之后:
|
|
|
|
|
- 1. `/api/product/list` → 拉到 `code` 列表 → 拼出 `admin_<code>` 全量用户名;
|
|
|
|
|
- 2. `/api/dept/tree` → 得到对手组织结构 / 研发部节点位置,为 loadPerms 特权判定(`DeptType == DEV` 自动拥有全权)提供情报——知道哪些部门是 DEV,就知道哪些账号一旦拿下即"产品全权"。
|
|
|
|
|
- 3. 枚举结果喂给 H-1 的 admin DoS 或针对性的钓鱼。
|
|
|
|
|
-
|
|
|
|
|
|
|
+ - `internal/model/user/sysUserModel.go:128-140`(`UpdatePassword`)
|
|
|
|
|
+ - `internal/model/user/sysUserModel.go:143-155`(`UpdateStatus`)
|
|
|
|
|
+- **描述**:两处都是 `m.ExecCtx(..., conn.ExecCtx(...))` 然后直接 `return err`,**不读 `sql.Result.RowsAffected`**。如果 `FindOne` 到 `ExecCtx` 之间用户被另一会话删除,或者主从延迟导致 `WHERE id = ?` 命中 0 行,调用方拿到的是 nil err,以为"更新成功"——实际 DB 里没有变化,但 `DelCacheCtx(sysUserIdKey, sysUserUsernameKey)` 已经执行了。
|
|
|
|
|
+- **影响**:
|
|
|
|
|
+ - `ChangePassword` 对已删除用户会返回 200 "成功",客户端以为改密成功,用户下次登录发现密码没变。
|
|
|
|
|
+ - `UpdateUserStatus` 对跨进程并发删除的用户会"假装成功",上层以为冻结生效。
|
|
|
|
|
+ - 更重要的:`UpdatePassword` / `UpdateStatus` 自身没有乐观锁(`UpdateProfile` 有 `updateTime` CAS),两次并发 ChangePassword 会"后写覆盖先写"且都返回成功,用户拿到的新密码是无法预测的那一份(在已知旧密码共谋场景下影响低)。
|
|
|
- **建议**:
|
|
- **建议**:
|
|
|
- 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
|
|
```go
|
|
|
- if !caller.IsSuperAdmin {
|
|
|
|
|
- if _, err := l.svcCtx.SysProductMemberModel.FindOneByProductCodeUserId(l.ctx, caller.ProductCode, req.Id); err != nil {
|
|
|
|
|
- return nil, response.ErrForbidden("无权查看非本产品成员的用户信息")
|
|
|
|
|
- }
|
|
|
|
|
|
|
+ res, err := m.ExecCtx(...)
|
|
|
|
|
+ if err != nil { return err }
|
|
|
|
|
+ if n, _ := res.RowsAffected(); n == 0 {
|
|
|
|
|
+ return ErrNotFound // 或自定义 ErrUpdateConflict
|
|
|
}
|
|
}
|
|
|
- ...
|
|
|
|
|
- return &types.UserItem{
|
|
|
|
|
- ...
|
|
|
|
|
- Email: user.Email,
|
|
|
|
|
- Phone: user.Phone,
|
|
|
|
|
- ...
|
|
|
|
|
- }, nil
|
|
|
|
|
|
|
+ return nil
|
|
|
```
|
|
```
|
|
|
- 判定规则是"同产品成员即可查看"——一个产品里最低权限 MEMBER 可以遍历同产品所有用户(userId 范围内 fuzz)的手机 / 邮箱。同产品有几百上千成员时,这等同于**暴露公司通讯录 PII**。
|
|
|
|
|
- 这里没有"**调用者对目标有管理权**"或"**看自己**"的更细粒度条款。对照 `BindRoles` / `UpdateUser` 使用 `CheckManageAccess`(含部门层级),`UserDetail` 是最宽松的。
|
|
|
|
|
-
|
|
|
|
|
|
|
+ 另外建议对 `UpdatePassword` 加上 `AND updateTime = ?` 的乐观锁,语义上与 `UpdateProfile` 对齐,避免并发改密"最后一写赢"的隐式行为。
|
|
|
|
|
+
|
|
|
|
|
+### M-3. `GuardRoleLevelAssignable` 的授权依据是 caller 的**缓存 `MinPermsLevel`**,被降级的调用者在 5 分钟缓存 TTL 内仍可分配原等级角色
|
|
|
|
|
+- **位置**:`internal/logic/auth/access.go`(`GuardRoleLevelAssignable`);调用方 `internal/logic/user/bindRolesLogic.go`。
|
|
|
|
|
+- **描述**:`BindRoles` 的授权判断是"caller 的 `MinPermsLevel` 必须**严格小于**被分配角色的 `PermsLevel`",而 `caller` 是从 `UserDetailsLoader.Load(callerUserId, productCode)` 取的,缓存 TTL 5 分钟。攻击窗口:
|
|
|
|
|
+ 1. T=0:超管把 caller C 从"总监级角色 (permsLevel=10)"降到"普通员工 (permsLevel=500)"。超管调用 `BindRoles` 改 C 的角色(`Clean(C.UserId)`)。
|
|
|
|
|
+ 2. T=0+δ:C 自己在其他机器上调用 `BindRoles` 给下属 X 分配"总监级角色 (permsLevel=10)"。
|
|
|
|
|
+ 3. C 的 JWT token 里没有 `MinPermsLevel`,它要靠 UD 缓存。如果 C 的 UD 缓存在 T=0 被 Clean,第二次读会打 DB 拿到新级别 → 授权失败。**但只要 Clean 因为 Redis 抖动失败了一次**,C 的 UD 缓存还在,`GuardRoleLevelAssignable` 读到 10,判定"严格小于 10 即可"不通过,判定 `10 >= 10` → 授权失败(这条实际 OK)。
|
|
|
|
|
+ 4. 真正的问题:如果 C 的角色从 10 降到 20(不是 500),C 要分配 15 级角色:`caller.MinPermsLevel(缓存=10) >= 15` 不成立 → 允许。实际 DB 里 C 是 20,`20 >= 15` 成立 → 应该拒绝。**C 用缓存越权分配了自己现在够不到的角色**。
|
|
|
|
|
+ 5. 5 分钟窗口足够 C 把一整组下属 bulk 升到 15 级。
|
|
|
|
|
+- **影响**:
|
|
|
|
|
+ - 典型的"TOCTOU + 缓存失效延迟"叠加漏洞。Clean 失败只被 log,没有重试,也没有降级成"缓存读写直通 DB"的 fallback。
|
|
|
|
|
+ - 触发条件依赖"超管降级某 admin"的时序,实际被动利用概率低,但**主动制造**(admin 预见自己要被降级,抢 5 分钟窗口)可能性不能排除。
|
|
|
- **建议**:
|
|
- **建议**:
|
|
|
- 1. `Phone` / `Email` 应仅对满足以下之一的 caller 返回:
|
|
|
|
|
- - `caller.UserId == user.Id`(自己);
|
|
|
|
|
- - `caller.IsSuperAdmin || caller.MemberType == consts.MemberTypeAdmin`;
|
|
|
|
|
- - `CheckManageAccess(...) == nil`(即调用者管辖该目标);
|
|
|
|
|
- 其余 caller 只返回脱敏字段(例如 `13***1234`)或不返回。
|
|
|
|
|
- 2. 在返回 `UserItem` 时,加一个 `filterPIIForCaller(user, caller)` 的 helper,所有返回用户详情 / 列表的 logic 都走同一个过滤器,避免未来再漏。
|
|
|
|
|
-
|
|
|
|
|
----
|
|
|
|
|
-
|
|
|
|
|
-### M-4. `BindRolePermsLogic` / `UpdateRoleLogic` 在**写成功后的缓存清理失败**时返回 500,客户端会把成功的写误判为失败并重试
|
|
|
|
|
-
|
|
|
|
|
-- **位置**:
|
|
|
|
|
- - `internal/logic/role/bindRolePermsLogic.go:128-134`
|
|
|
|
|
- - `internal/logic/role/updateRoleLogic.go:79-85`
|
|
|
|
|
-- **描述**:
|
|
|
|
|
- ```go
|
|
|
|
|
- if err := l.svcCtx.SysRolePermModel.TransactCtx(...); err != nil { return err }
|
|
|
|
|
-
|
|
|
|
|
- affectedUserIds, err := l.svcCtx.SysUserRoleModel.FindUserIdsByRoleId(l.ctx, req.RoleId)
|
|
|
|
|
- if err != nil {
|
|
|
|
|
- logx.WithContext(l.ctx).Errorf("角色权限已更新但缓存清理失败 roleId=%d: %v", req.RoleId, err)
|
|
|
|
|
- return response.NewCodeError(500, "权限已更新但缓存刷新失败,请稍后手动刷新")
|
|
|
|
|
- }
|
|
|
|
|
- l.svcCtx.UserDetailsLoader.BatchDel(l.ctx, affectedUserIds, role.ProductCode)
|
|
|
|
|
- ```
|
|
|
|
|
- 出问题的细节在三个层面:
|
|
|
|
|
- 1. `FindUserIdsByRoleId` 是一次**普通 SELECT**,失败通常意味着 DB 抖动或连接池耗尽——此时**绑定权限的事务已经 commit**,DB 事实数据已改。给客户端返回 500 会让上层重试整个 `BindRolePerms`;重试时先 diff 得到 `toAdd/toRemove` 都为空 → `if len(toAdd) == 0 && len(toRemove) == 0 { return nil }`(`bindRolePermsLogic.go:102-104`)→ **静默返回 200,但期间的业务逻辑 / 回调不会再触发**。更糟的变体:`BindRoles`(`user/bindRolesLogic.go:118-121`)的同一 pattern 里有 `UserDetailsLoader.Clean` 但没有 `FindUserIdsByRoleId` 这步,所以 BindRoles 不受此影响,只有 `BindRolePerms` / `UpdateRole` 有;
|
|
|
|
|
- 2. `BatchDel` 失败只打 log 不改响应——那就让 `FindUserIdsByRoleId` 失败也同样处理才对称;
|
|
|
|
|
- 3. 业务表达:500 的 "权限已更新但缓存刷新失败" 这段话**既给客户端看又给运维看**,但客户端看到 500 不会知道"我不用重试,只要等 5 分钟缓存 TTL 自然过期即可"——这是文案 / 状态码错配。
|
|
|
|
|
-
|
|
|
|
|
|
|
+ 1. `GuardRoleLevelAssignable` 里,对 caller 的 `MinPermsLevel` 额外做一次"旁路缓存直查 DB"校验(只在这条授权点,不影响其他用 UD 缓存的路径)。
|
|
|
|
|
+ 2. 或者 `Clean` 走 Redis pipeline + 一次 Lua script 保证原子,失败时 retry 2~3 次,失败后把 userId 入一个短 ttl 的降级黑名单,命中就强制走 DB。
|
|
|
|
|
+ 3. `UserDetailsLoader` 加个 `LoadFresh(ctx, userId, productCode)` 方法专供授权点使用,bypass cache。
|
|
|
|
|
+
|
|
|
|
|
+### M-4. `CreateProductLogic` **响应体里明文返回初始 admin 密码**,穿过任何响应日志 / 监控都会落盘
|
|
|
|
|
+- **位置**:`internal/logic/product/createProductLogic.go`(返回 `types.CreateProductResp.AdminPassword` 明文)、`internal/types/types.go:47-54`
|
|
|
|
|
+- **描述**:`CreateProductResp` 包含 `AdminPassword string`,`go-zero` 默认响应序列化走 httpx,**响应体默认不自动打日志**,但在以下三个常见运维情况下会落盘:
|
|
|
|
|
+ - API 网关 / Nginx access log 关掉了 body redaction;
|
|
|
|
|
+ - APM / OpenTelemetry 开了 "response body" 采样;
|
|
|
|
|
+ - 前端 console 或者集成测试截图留在了代码仓库 / 工单系统。
|
|
|
|
|
+- **影响**:密码泄漏;`IsSuperAdmin` 产品默认密码一旦落到长期存储就需要紧急全量改密。
|
|
|
- **建议**:
|
|
- **建议**:
|
|
|
- 1. 把这两处改成 "事务成功即视为成功;缓存刷新失败仅 log + 异步重试":
|
|
|
|
|
- ```go
|
|
|
|
|
- if err := ...TransactCtx(...); err != nil { return err }
|
|
|
|
|
-
|
|
|
|
|
- if userIds, err := l.svcCtx.SysUserRoleModel.FindUserIdsByRoleId(l.ctx, req.RoleId); err == nil {
|
|
|
|
|
- l.svcCtx.UserDetailsLoader.BatchDel(l.ctx, userIds, role.ProductCode)
|
|
|
|
|
- } else {
|
|
|
|
|
- logx.WithContext(l.ctx).Errorf("postcommit cache cleanup fetch userIds failed roleId=%d: %v", req.RoleId, err)
|
|
|
|
|
- // TODO(可选):入异步队列重试,或记 audit 让运维手工刷新
|
|
|
|
|
- }
|
|
|
|
|
- return nil
|
|
|
|
|
- ```
|
|
|
|
|
- 2. 真的要让客户端知道 "权限变更成功但部分缓存未刷新",应返回 200 + resp 里带 `cacheRefreshStatus: "degraded"`,而不是 500。
|
|
|
|
|
-
|
|
|
|
|
----
|
|
|
|
|
-
|
|
|
|
|
-### M-5. `UpdateUserStatusLogic` / `BindRolesLogic` / `UpdateUserLogic` 的**同请求重复 `FindOne(targetUserId)`**(缓存命中但仍有 Redis 往返)
|
|
|
|
|
-
|
|
|
|
|
-- **位置**:
|
|
|
|
|
- - `internal/logic/user/updateUserStatusLogic.go:37-50`(3 次)
|
|
|
|
|
- - `internal/logic/user/bindRolesLogic.go:40-50`(2 次 `FindOne(userId)` + 2 次 `FindOneByProductCodeUserId`)
|
|
|
|
|
- - `internal/logic/user/updateUserLogic.go:47-58`(2 次 `FindOne(userId)`)
|
|
|
|
|
-- **描述**:
|
|
|
|
|
- 以 `UpdateUserStatus` 为例:
|
|
|
|
|
- ```
|
|
|
|
|
- ValidateStatusChange(...) → SysUserModel.FindOne(targetUserId)
|
|
|
|
|
- CheckManageAccess(...) →
|
|
|
|
|
- checkDeptHierarchy(...) → SysUserModel.FindOne(targetUserId)
|
|
|
|
|
- checkPermLevel(...) → SysProductMemberModel.FindOneByProductCodeUserId(...)
|
|
|
|
|
- // 回到 handler
|
|
|
|
|
- user, _ := SysUserModel.FindOne(targetUserId)
|
|
|
|
|
- ```
|
|
|
|
|
- 全部通过 go-zero cache 命中,但每次 `FindOne` 还是要 Redis GET(若 Redis 上无 key 会触发 DB)。即便 Redis GET 是 0.5ms,3 次同一 key 就浪费 1.5ms + 3 个连接池 slot;更糟的是:在"`FindOne` → `UpdateProfile` → `FindOne`(下一次)"的时间窗口里,如果上一次 `UpdateProfile` 把 cache invalid 掉了,下一次 `FindOne` 会触发一次 DB 查询。**同一个 request 内部**本不该做这种往返。
|
|
|
|
|
-
|
|
|
|
|
|
|
+ 1. 响应里把 `AdminPassword` 字段标记为 "**一次性展示**",并在文档里强制要求立刻改密。
|
|
|
|
|
+ 2. 更稳的方案:响应只返回 `adminUser`,密码随后走**带 nonce 的一次性链接**(Redis 中存 5 分钟、一次消费后删除),新产品 owner 登录后自己重置。
|
|
|
|
|
+ 3. 至少在 `response.Middleware` 中把 `AdminPassword` 字段加入日志脱敏白名单。
|
|
|
|
|
+
|
|
|
|
|
+### L-1. `DeleteDeptLogic` 事务内多段 `FOR UPDATE` 锁序列仍保留 AB-BA 死锁理论风险(R6 L-2 未消化)
|
|
|
|
|
+- **位置**:`internal/logic/dept/deleteDeptLogic.go`
|
|
|
|
|
+- **描述**:一个事务内依次 `FOR UPDATE` 了 `sys_dept(row)` → `sys_dept(children range)` → `sys_user(dept range)`。如果另一事务(比如 `UpdateUser` 要改 `DeptId`,同时 `CreateDept` 插一个子部门)以不同顺序抢锁,理论上存在 AB-BA 交叉死锁。实际频率低,但 MySQL 死锁 retry 会被 go-zero 上浮成 500。
|
|
|
|
|
+- **建议**:全局统一 "先锁部门再锁用户"、"先锁父部门再锁子部门" 的顺序协议,并在 `UpdateUserLogic` 涉及 `DeptId` 变更的时候显式 `SELECT ... FOR SHARE` 一下新旧部门。或把 `DeleteDept` 中排查子部门 / 关联用户的 `FOR UPDATE` 降成 `FOR SHARE`(因为这里是存在性判断而不是修改)。
|
|
|
|
|
+
|
|
|
|
|
+### L-2. `SysUserModel.IncrementTokenVersion` 仍是"无条件大杀器",缺安全注释 / 调用点约束(R6 L-4 未消化)
|
|
|
|
|
+- **位置**:`internal/model/user/sysUserModel.go`(`IncrementTokenVersion`),调用方 `internal/logic/auth/logoutLogic.go:46`。
|
|
|
|
|
+- **描述**:`RefreshToken` 已经切到 `IncrementTokenVersionIfMatch`(CAS 语义正确),`Logout` 还在用老的 `IncrementTokenVersion`(业务语义正确,"强制所有会话失效")。风险是这个 API 现在对整个仓库可见,**未来任何改 Refresh / Rotate 场景的开发者都可能误调它**,退回到 R5 以前的会话劫持窗口。
|
|
|
- **建议**:
|
|
- **建议**:
|
|
|
- 1. 在 `CheckManageAccess` 签名里加一个可选 `prefetchedTarget *user.SysUser`:调用方已经有目标用户对象时,直接传进去,`checkDeptHierarchy` 复用;否则再查:
|
|
|
|
|
- ```go
|
|
|
|
|
- func CheckManageAccess(ctx, svcCtx, targetUserId, productCode, opts ...Option) error {
|
|
|
|
|
- var target *user.SysUser
|
|
|
|
|
- for _, opt := range opts { opt.apply(&target) }
|
|
|
|
|
- if target == nil {
|
|
|
|
|
- target, _ = svcCtx.SysUserModel.FindOne(ctx, targetUserId)
|
|
|
|
|
- }
|
|
|
|
|
- ...
|
|
|
|
|
- }
|
|
|
|
|
- ```
|
|
|
|
|
- 2. 更激进:在 handler 最外层或 middleware 里做**请求级 cache**(`context.WithValue` 一个小 map),`FindOne`/`FindOneByProductCodeUserId` 走这层再透传。这对所有类似 `UpdateUser + Check*` 的组合都直接受益。
|
|
|
|
|
-
|
|
|
|
|
----
|
|
|
|
|
|
|
+ 1. 给 `IncrementTokenVersion` 加个 `// WARN: 仅限强制全量失效(Logout / 封禁)。Refresh/Rotate 必须使用 IncrementTokenVersionIfMatch。` 的显式 header 注释。
|
|
|
|
|
+ 2. 最干净的做法:把 `IncrementTokenVersion` 改成 package-private,再在 `logout` 所在 package 用**显式命名的 wrapper**(`ForceRevokeAllSessions`)暴露,新接入者一眼看到红色标签。
|
|
|
|
|
|
|
|
-### M-6. `ExtractClientIP` 在 `behindProxy=true` 时**只信任 `X-Real-IP`**,没 `X-Forwarded-For` fallback;且**未设头时回落到 proxy 的 RemoteAddr → 全站共享一个桶**
|
|
|
|
|
-
|
|
|
|
|
-- **位置**:`internal/middleware/ratelimitMiddleware.go:54-65`
|
|
|
|
|
|
|
+### L-3. `loadPerms` 其他分支的错误同样被静默 log(`rolePermIds` / `allowIds` / `FindAllCodesByProductCode`),和 H-1 同宗
|
|
|
|
|
+- **位置**:`internal/loaders/userDetailsLoader.go:435-498`
|
|
|
- **描述**:
|
|
- **描述**:
|
|
|
- ```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 直接放大。
|
|
|
|
|
|
|
+ - L450-453:`FindPermIdsByRoleIds` 失败 → `rolePermIds` 保持空。若此时 role 权限正常、但查询临时失败,用户的"角色→权限"整块就被丢掉。
|
|
|
|
|
+ - L435-437:`FindAllCodesByProductCode` 失败 → `ud.Perms = nil`(对 ADMIN / DEV 部门这类"全量权限"角色来说直接降成 0 perm)。
|
|
|
|
|
+ - L487-498:`FindByIds` 失败 → `ud.Perms = nil`。
|
|
|
|
|
|
|
|
-- **建议**:
|
|
|
|
|
- 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-7. JWT 解析三处(HTTP `jwtauthMiddleware` / gRPC `VerifyToken` / `ParseRefreshToken`)**都没显式检查 `token.Method`**
|
|
|
|
|
|
|
+ 所有这些都会被 `Load` 写入 5 分钟缓存。对 ADMIN 来说是"5 分钟内所有权限消失",对普通成员来说是"5 分钟内权限表不一致"。用户体感就是间歇性 403,定位困难。
|
|
|
|
|
+- **建议**:与 M-1 同步修复:loadPerms 内任一子步骤返回 error,**整次 Load 跳过缓存写入**,同时把 error 传给 `Load`,由上层决定是 503 还是 401。
|
|
|
|
|
|
|
|
|
|
+### L-4. Model 层 SQL 中的 `status = 1` 硬编码,与 `consts.StatusEnabled` 脱钩
|
|
|
- **位置**:
|
|
- **位置**:
|
|
|
- - `internal/middleware/jwtauthMiddleware.go:59-61`
|
|
|
|
|
- - `internal/server/permserver.go:242-244`
|
|
|
|
|
- - `internal/logic/auth/jwt.go:78-80`
|
|
|
|
|
-- **描述**:
|
|
|
|
|
- 三处 `keyfunc` 直接返回 `[]byte(secret)`,没有做:
|
|
|
|
|
- ```go
|
|
|
|
|
- if _, ok := token.Method.(*jwt.SigningMethodHMAC); !ok {
|
|
|
|
|
- return nil, fmt.Errorf("unexpected signing method: %v", token.Header["alg"])
|
|
|
|
|
- }
|
|
|
|
|
- ```
|
|
|
|
|
- `jwt/v4` 默认对 `alg=none` 是关闭的(`SigningMethodNone` 需要显式 `jwt.UnsafeAllowNoneSignatureType`),所以**目前没有立即可利用的漏洞**。但:
|
|
|
|
|
- 1. 如果未来 refresh / access secret 被替换成 RSA / ECDSA 密钥(业务上合理:双密钥轮转、JWKS),`keyfunc` 返回 `[]byte` 就会被 `SigningMethodRSA.Verify` 当 public key 解析失败,这本身安全——但业务上会突然全站登录失败,调试困难;
|
|
|
|
|
- 2. 更重要的是:显式 `token.Method` 检查是深度防御的行业标准(OWASP JWT Cheat Sheet),任何 review 过 JWT 代码的审计都会拉出来。
|
|
|
|
|
-- **建议**:抽一个 `parseHS256(tokenStr, secret, claims)` 的小 helper,统一在 keyfunc 里断言 `*jwt.SigningMethodHMAC`,并在 `ParseRefreshToken` / access token 解析两处调用。
|
|
|
|
|
-
|
|
|
|
|
----
|
|
|
|
|
-
|
|
|
|
|
-### M-8. `IncrementTokenVersion` / `IncrementTokenVersionIfMatch` 都**先 `FindOne` 一次**只为拿 `username` 构造 cache key
|
|
|
|
|
-
|
|
|
|
|
-- **位置**:`internal/model/user/sysUserModel.go:158-181, 186-214`
|
|
|
|
|
-- **描述**:
|
|
|
|
|
- ```go
|
|
|
|
|
- func (m *customSysUserModel) IncrementTokenVersionIfMatch(ctx context.Context, id, expected int64) (int64, error) {
|
|
|
|
|
- data, err := m.FindOne(ctx, id) // ← 只为下面的 usernameKey
|
|
|
|
|
- if err != nil { return 0, err }
|
|
|
|
|
- sysUserIdKey := fmt.Sprintf("%s%v", cacheSysUserIdPrefix, id)
|
|
|
|
|
- sysUserUsernameKey := fmt.Sprintf("%s%v", cacheSysUserUsernamePrefix, data.Username)
|
|
|
|
|
- var newVersion int64
|
|
|
|
|
- err = m.TransactCtx(ctx, ...)
|
|
|
|
|
- _ = m.DelCacheCtx(ctx, sysUserIdKey, sysUserUsernameKey)
|
|
|
|
|
- return newVersion, nil
|
|
|
|
|
- }
|
|
|
|
|
- ```
|
|
|
|
|
- 每次 `RefreshToken`(HTTP + gRPC 两条路径)都多一次 `FindOne` 走 cache-aside:
|
|
|
|
|
- - Cache 命中:1 次 Redis GET(快);
|
|
|
|
|
- - Cache 未命中:1 次 Redis GET + 1 次 DB SELECT + 1 次 Redis SETEX = 3 次 IO。
|
|
|
|
|
- `RefreshToken` 已经调了 `UserDetailsLoader.Load`(含 `loadUser` 里的 `FindOne`)——同一请求内第二次 `FindOne`,完全是浪费;更关键的是:`RefreshToken` 的 cache flow 是 "Load → 比较 TokenVersion → IncrementIfMatch → Clean",`Clean` 会把 loader 缓存清掉但 **go-zero 的 `FindOne` 那一层 cache 不会被清**(不同 key 空间),所以下一次请求还是会再查一次。
|
|
|
|
|
-
|
|
|
|
|
-- **建议**:
|
|
|
|
|
- 1. 把 `IncrementTokenVersionIfMatch` 的第一次 `FindOne` 去掉,cache key 的 username 部分**改成读 ExecCtx 的"写入后新值"**(go-zero 支持把 `DelCacheCtx` 放进 `ExecCtx` 的 hook 里,只传 `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`,直接透传即可。
|
|
|
|
|
-
|
|
|
|
|
----
|
|
|
|
|
-
|
|
|
|
|
-### L-1. `SyncPermsService` 的"如需禁用所有权限请使用专用接口"是**文案指向幽灵端点**
|
|
|
|
|
-
|
|
|
|
|
-- **位置**:`internal/logic/pub/syncPermsService.go:49-51`
|
|
|
|
|
-- **描述**:
|
|
|
|
|
- ```go
|
|
|
|
|
- if len(perms) == 0 {
|
|
|
|
|
- return nil, &SyncPermsError{Code: 400, Message: "权限列表不能为空,如需禁用所有权限请使用专用接口"}
|
|
|
|
|
- }
|
|
|
|
|
- ```
|
|
|
|
|
- 然而 `perm.api` / `routes.go` / gRPC `PermService` 里**并没有"禁用所有权限的专用接口"**。这行错误消息是历史设计遗留——要么这个接口被砍了,要么从来没实现过。接入方看到 400 文案会去找"专用接口",浪费排查时间。
|
|
|
|
|
-- **建议**:把文案改成客观事实"权限列表不能为空",并把"禁用所有权限"这条产品需求**显式 TODO 或删除**(真要支持,需要一个独立授权模型,不能走 appKey 就把产品的所有权限抹了——这本身是个有争议的能力)。
|
|
|
|
|
-
|
|
|
|
|
----
|
|
|
|
|
-
|
|
|
|
|
-### L-2. `DeleteDeptLogic` 的多次 `FOR UPDATE` 顺序**可能死锁**(真实可能性较低)
|
|
|
|
|
|
|
+ - `internal/model/userrole/sysUserRoleModel.go:51` —— `r.status = 1`
|
|
|
|
|
+ - `internal/model/userperm/sysUserPermModel.go:35` —— `p.status = 1`
|
|
|
|
|
+ - `internal/model/role/sysRoleModel.go`(`FindMinPermsLevelByUserIdAndProductCode`)—— `r.status = 1`
|
|
|
|
|
+- **描述**:`consts.StatusEnabled` 当前定义为 1,但三处 SQL 把它写死。一旦运维 / 迁移脚本把 `StatusEnabled` 的语义改掉,或者业务加出 "status=2 已归档" 之类的新状态,这几条查询会默默返回错误数据集,没有编译期 / 单元测试期信号。
|
|
|
|
|
+- **建议**:改成 `... AND r.status = ?` + 参数传 `consts.StatusEnabled`,与其他同类查询(如 `sysProductMemberModel.CountOtherActiveAdminsTx`)风格一致。
|
|
|
|
|
|
|
|
-- **位置**:`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" 同时——这种场景通常不会主动并发;
|
|
|
|
|
- 但审计视角上仍是一个"范围锁 → 可阻塞后续"的定时炸弹。
|
|
|
|
|
|
|
+### L-5. `internal/model/productmember/sysProductMemberModel.go` 中两个僵尸接口方法
|
|
|
|
|
+- **位置**:
|
|
|
|
|
+ - `FindMapByProductCodeUserIds`(定义在接口和实现中)
|
|
|
|
|
+ - `CountActiveAdmins`(非事务版)
|
|
|
|
|
+- **描述**:`rg` 扫下来这两个方法仅在 `testutil/mocks/mock_productmember_model.go` 与 `sysProductMemberModel_test.go` 中被引用,**整个 `/internal/logic/**` 没有一处调用**。实现里还包含手写 SQL,是维护负担与误用风险源。
|
|
|
|
|
+ 同样地,`internal/model/perm/sysPermModel.go` 中的 `FindMapByProductCode`(非事务版)也仅在 mock 与 test 中出现,`syncPermsService` 已切到 `FindMapByProductCodeWithTx`。
|
|
|
|
|
+- **建议**:确认无残留调用后从接口 / 实现 / mock 里移除,避免接口 surface area 膨胀。
|
|
|
|
|
+
|
|
|
|
|
+### L-6. gRPC `GetUserPerms` 可被有效 AppKey/AppSecret 拥有者用作**负缓存预污染**工具
|
|
|
|
|
+- **位置**:`internal/server/permserver.go`(`GetUserPerms`);结合 `internal/loaders/userDetailsLoader.go:134-154` 的负缓存写入路径。
|
|
|
|
|
+- **描述**:`GetUserPerms` 接受任意 `req.UserId`,内部会调用 `UserDetailsLoader.Load`。若攻击者拥有有效的产品凭证(如被泄漏的 `appKey`/`appSecret`),可批量请求未来将分配的自增 ID(`userId = maxUserId+1 ... maxUserId+N`):
|
|
|
|
|
+ - 每个未命中查询会落一条 `negativeCacheMarker`(TTL 30s);
|
|
|
|
|
+ - 当一个新用户在这 30s 内被 `CreateUser` 自增到这个 ID,他的 UD 缓存键已被占用为负缓存;
|
|
|
|
|
+ - 新用户自身一旦被 `Load`,直接命中 `_NOT_FOUND_`,`Username` 返回空,**JWT middleware 判定"用户已被删除"**,登录 / 使用失败。
|
|
|
|
|
+- **影响**:条件依赖 AppKey 泄漏 + CreateUser 时机,概率低,但这是唯一一条"外部可写负缓存"的通道。
|
|
|
- **建议**:
|
|
- **建议**:
|
|
|
- 1. 确保锁顺序在所有涉及 `sys_dept + sys_user` 的事务里一致(总是先锁 dept,再锁 user);
|
|
|
|
|
- 2. `DeleteDept` 内可以把 `sys_user WHERE deptId = ? FOR UPDATE` 换成不加锁的 `SELECT ... LOCK IN SHARE MODE` + 事务隔离级 REPEATABLE READ(反正只是存在性判定),降低阻塞面。
|
|
|
|
|
-
|
|
|
|
|
----
|
|
|
|
|
|
|
+ 1. `Load` 在写入负缓存之前,再通过 `SysUserModel.FindOne(ctx, userId)` 强一致校验一次(绕过 cache),确认真 NotFound 才写哨兵。
|
|
|
|
|
+ 2. 或者在 `CreateUserLogic` 成功插入之后主动 `Del` 掉 `ud:newId:*` 的负缓存键(需要遍历产品维度,因此成本较高)。
|
|
|
|
|
+ 3. 最简:`negativeCacheTTL` 从 30 → 10,并加一条 `svc` 级的"新用户创建后 30s 内绕过负缓存"的白名单(按 userId > `watermark` 判定)。
|
|
|
|
|
|
|
|
-### L-3. `UserDetailsLoader.registerCacheKey` **每次都做 4 次 Redis 单独 RTT**(SAdd + Expire + SAdd + Expire)
|
|
|
|
|
-
|
|
|
|
|
-- **位置**:`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-4. `Logout` 仍用**无条件 `IncrementTokenVersion`**(非 CAS)
|
|
|
|
|
-
|
|
|
|
|
-- **位置**:`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 版本)。
|
|
|
|
|
|
|
+### L-7. `CheckManageAccess` 对 caller `DeptId=0` 且非 ADMIN 的历史账号直接 403(R6 L-6 未消化)
|
|
|
|
|
+- **位置**:`internal/logic/auth/access.go`(`checkDeptHierarchy`)
|
|
|
|
|
+- **描述**:H-4(R6 时已修复)之后新建 MEMBER/DEVELOPER 不再是 `DeptId=0`,但**存量数据中有遗留**的 `DeptId=0` MEMBER 账号。这类账号即便通过 `checkPermLevel` 校验也会因为 `caller.DeptPath == ""` 在 `checkDeptHierarchy` 被直接 403。
|
|
|
- **建议**:
|
|
- **建议**:
|
|
|
- 1. 不强制替换 `Logout`(语义正确),但**把 `IncrementTokenVersion` 加上显式的安全注释**:"仅限业务语义为'强制全量失效'的场景(Logout / 封禁账号),**禁止**在 Refresh/Rotation 场景使用——Refresh 必须走 `IncrementTokenVersionIfMatch`"。
|
|
|
|
|
- 2. 更激进:用 golang build tag / linter 限制 `IncrementTokenVersion` 的调用方范围(仅限 `auth/logoutLogic.go` + 未来的封禁接口)。
|
|
|
|
|
-
|
|
|
|
|
----
|
|
|
|
|
-
|
|
|
|
|
-### L-5. `removeMemberLogic`:移除 ADMIN 前的 `CountActiveAdminsTx` **与目标成员自己的 state 判定耦合**
|
|
|
|
|
-
|
|
|
|
|
-- **位置**:`internal/logic/member/removeMemberLogic.go:45-54`
|
|
|
|
|
-- **描述**:
|
|
|
|
|
- ```go
|
|
|
|
|
- if locked.MemberType == consts.MemberTypeAdmin && locked.Status == consts.StatusEnabled {
|
|
|
|
|
- adminCount, err := l.svcCtx.SysProductMemberModel.CountActiveAdminsTx(ctx, session, member.ProductCode)
|
|
|
|
|
- ...
|
|
|
|
|
- if adminCount <= 1 { return "不能移除最后一个管理员" }
|
|
|
|
|
- }
|
|
|
|
|
- ```
|
|
|
|
|
- `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` 的同名调用同样受益。
|
|
|
|
|
|
|
+ 1. 运维侧一次性迁移 `UPDATE sys_user SET deptId = <default_dept_id> WHERE deptId = 0 AND isSuperAdmin = 0 AND memberType NOT IN ('ADMIN')`。
|
|
|
|
|
+ 2. 代码侧把"看自己"场景短路,在 `CheckManageAccess` 最上面加 `if callerUserId == targetUserId && productCode == caller.ProductCode { return nil }` 避免纯看自己的操作被部门树误伤。
|
|
|
|
|
|
|
|
---
|
|
---
|
|
|
|
|
|
|
|
-### L-6. `CheckManageAccess` 在 caller `DeptId == 0` 时直接 403,漏了"**非 ADMIN 的超级管理员**"的心智模型
|
|
|
|
|
-
|
|
|
|
|
-- **位置**:`internal/logic/auth/access.go:155-162`
|
|
|
|
|
-- **描述**:
|
|
|
|
|
- ```go
|
|
|
|
|
- func checkDeptHierarchy(ctx, svcCtx, caller, targetUserId) error {
|
|
|
|
|
- if caller.MemberType == consts.MemberTypeAdmin { return nil }
|
|
|
|
|
- if caller.DeptId == 0 { return ErrForbidden("您未归属任何部门,无权管理其他用户") }
|
|
|
|
|
- if caller.DeptPath == "" { return ErrForbidden("您的部门信息异常...") }
|
|
|
|
|
- ...
|
|
|
|
|
- }
|
|
|
|
|
- ```
|
|
|
|
|
- `CheckManageAccess` 最外层已经对 `caller.IsSuperAdmin` 做了 early return(`access.go:47`),所以 `checkDeptHierarchy` 进到这里一定不是超管——OK。
|
|
|
|
|
- 但目前代码里"caller `DeptId=0`"这件事并不一定异常:**L1 中 H-4 的修复落地后**,普通 MEMBER 的 `DeptId` 绝不会是 0;但早期数据 / 迁移遗留数据里仍可能有 `DeptId=0` 的 MEMBER(例如从老系统搬来的账号)。这些账号会发现**任何管理操作都拒绝**,包括合法的"管理自己所在产品里的下属"——实际上 `checkPermLevel` 本来可以单独判定,但被 `checkDeptHierarchy` 先拦了。
|
|
|
|
|
-- **建议**:在无部门归属的 MEMBER 场景下,至少应允许"管理自己"和"纯 product-admin-downward"的操作;或者运维侧跑一次 data fix 把 `DeptId=0` 的 MEMBER 都归入"默认部门"。代码侧加一条 TODO 记录这个假设,便于未来发现幽灵账号时做定向修复。
|
|
|
|
|
|
|
+## 📋 修复优先级汇总
|
|
|
|
|
+
|
|
|
|
|
+| 优先级 | finding | 一句话概要 |
|
|
|
|
|
+| :----- | :------------------------------------------------------------------------- | :------------------------------------------------------------------------- |
|
|
|
|
|
+| **P0** | **H-1** `loadPerms` deny-list fail-open | DB 抖动一次 → 用户越权 5 分钟,纯代码路径,修最简单 |
|
|
|
|
|
+| **P0** | **H-2** UserDetail/UserList PII 暴露(R6 M-3 未落地) | 任意同产品 MEMBER 可读全员手机邮箱 |
|
|
|
|
|
+| **P0** | **H-3** AddMember 缺 CheckManageAccess + 超管防御 | 产品 ADMIN 可拉跨部门 / 超管用户入产品,直接放大 H-2 |
|
|
|
|
|
+| **P0** | **H-4** JWT keyfunc 未断言 HMAC(R6 M-7 未落地) | 深度防御盲区,未来密钥体系迁移的定时炸弹 |
|
|
|
|
|
+| P1 | **M-1** Load 把 DB 故障同化为用户不存在;半加载也写缓存 | 单点 DB 抖动触发雪崩 + 5 分钟半残缓存污染 |
|
|
|
|
|
+| P1 | **M-2** UpdatePassword/UpdateStatus 不校验 RowsAffected | 对已删除用户静默成功,语义欺骗客户端 |
|
|
|
|
|
+| P1 | **M-3** GuardRoleLevelAssignable 依赖缓存 MinPermsLevel | TOCTOU + Clean 失败 → 降级 admin 在 5 分钟内仍能授出原等级 |
|
|
|
|
|
+| P1 | **M-4** CreateProduct 响应里带明文初始密码 | 穿过日志 / APM 就落盘,需紧急改密 |
|
|
|
|
|
+| P2 | **L-1** DeleteDept 多段 FOR UPDATE 锁序列(R6 L-2) | AB-BA 死锁理论风险 |
|
|
|
|
|
+| P2 | **L-2** IncrementTokenVersion 无安全注释(R6 L-4) | 易被未来改 Refresh 的开发者误用 |
|
|
|
|
|
+| P2 | **L-3** loadPerms 其余分支错误同样静默 | 和 H-1 同宗,应作为一个修复包一起上 |
|
|
|
|
|
+| P2 | **L-4** SQL 中 `status = 1` 硬编码 | 统一改成 `consts.StatusEnabled` 占位参数 |
|
|
|
|
|
+| P2 | **L-5** `FindMapByProductCodeUserIds` / `CountActiveAdmins`(非 Tx)等僵尸 | 仅 mock/test 引用,清理 |
|
|
|
|
|
+| P3 | **L-6** gRPC GetUserPerms 负缓存预污染 | 依赖 AppKey 泄漏 + 自增 ID 命中,概率低但可行 |
|
|
|
|
|
+| P3 | **L-7** CheckManageAccess caller DeptId=0 时 403(R6 L-6) | 历史遗留账号,运维侧补数据或代码兜底"看自己" |
|
|
|
|
|
+
|
|
|
|
|
+## 🛠 建议修复次序
|
|
|
|
|
+
|
|
|
|
|
+1. **P0 同批上线**(同一次发版一起修,互相放大):
|
|
|
|
|
+ - H-1:给 deny-list 查询改 fail-close。
|
|
|
|
|
+ - H-2:`filterPIIForCaller` 在 UserDetail / UserList 返回前强制走一遍。
|
|
|
|
|
+ - H-3:`AddMember` 追加 `CheckManageAccess` + 超管判定。
|
|
|
|
|
+ - H-4:抽 `parseWithHMAC` helper,三处 `keyfunc` 替换。
|
|
|
|
|
+
|
|
|
|
|
+2. **P1 紧随**:
|
|
|
|
|
+ - M-1 + L-3:一起做 "Load/loadPerms 错误模型重构"。接口改成 `(*UserDetails, error)`,半加载不写缓存。
|
|
|
|
|
+ - M-2:两处 `ExecCtx` 后加 `RowsAffected` 判定。
|
|
|
|
|
+ - M-3:`GuardRoleLevelAssignable` 改走 fresh read,不靠 UD 缓存。
|
|
|
|
|
+ - M-4:`CreateProductResp` 换成"一次性展示链接 + 立即改密"流程。
|
|
|
|
|
+
|
|
|
|
|
+3. **P2 / P3 收尾**:
|
|
|
|
|
+ - L-1 统一 FOR UPDATE 锁序列。
|
|
|
|
|
+ - L-2 加红色注释 + 考虑 package-private。
|
|
|
|
|
+ - L-4 `status = 1` 批量改占位参数。
|
|
|
|
|
+ - L-5 清掉僵尸接口方法 + 其 mock。
|
|
|
|
|
+ - L-6 `Load` 写负缓存前再跑一次 fresh FindOne,或者缩 TTL 到 10s。
|
|
|
|
|
+ - L-7 数据迁移 + `CheckManageAccess` "看自己" 短路。
|
|
|
|
|
|
|
|
---
|
|
---
|
|
|
-
|
|
|
|
|
-## 结论与修复优先级
|
|
|
|
|
-
|
|
|
|
|
-| 优先级 | finding | 概要 |
|
|
|
|
|
-| --- | --- | --- |
|
|
|
|
|
-| **P0** | H-1 | AdminLogin 限流仅用 username → 可远端批量锁死所有产品 admin 账号(配合 M-2 的 ProductList 枚举放大) |
|
|
|
|
|
-| **P0** | H-2 | `ValidateProductLogin` 在冻结账号路径跳过 bcrypt → 响应时间 + 明文错误双重账号状态 oracle |
|
|
|
|
|
-| **P0** | H-3 | SyncPermsService 的 FOR UPDATE 修复只做到基础设施层,service 层仍然 tx 外读 / tx 内写,1062 → 409 只是缓解不是根治 |
|
|
|
|
|
-| P1 | M-1 | UpdateDept 部门用户缓存仍串行 Clean(R5 M-2 未落地) |
|
|
|
|
|
-| P1 | M-2 | ProductList / ProductDetail / DeptTree 对任意登录用户无访问控制,组合放大 H-1 |
|
|
|
|
|
-| P1 | M-3 | UserDetail 向任意同产品成员泄露 Phone / Email(PII 超额披露) |
|
|
|
|
|
-| P1 | M-4 | BindRolePerms / UpdateRole 在 post-commit 缓存步骤失败返回 500 → 客户端误判失败 |
|
|
|
|
|
-| P2 | M-5 / M-6 / M-7 | 请求内重复 FindOne / ExtractClientIP 代理链解析不足 / JWT 未显式验签方法 |
|
|
|
|
|
-| P2 | M-8 | IncrementTokenVersionIfMatch 为拿 username 多 1 次 FindOne |
|
|
|
|
|
-| P3 | L-1 ~ L-6 | 幽灵端点文案 / 锁序风险 / Redis 往返 pipeline / 无 CAS 注释 / CountActiveAdmins 语义 / DeptId=0 心智模型 |
|
|
|
|
|
-
|
|
|
|
|
-### 建议的修复次序
|
|
|
|
|
-
|
|
|
|
|
-1. **P0 同批上线**:H-1 + H-2(登录侧信道 + DoS 一起打补丁),H-3(SyncPerms 的 FOR UPDATE 接上 `LockByCodeTx` + 改掉 mock)。**这三条是本轮最关键的质变修复,前两条决定账号安全边界,第三条决定同步的可靠性。**
|
|
|
|
|
-2. **P1 第二批**:
|
|
|
|
|
- - M-1:`UserDetailsLoader.CleanByUserIds` + `UpdateDept` 批处理;
|
|
|
|
|
- - M-2:ProductList / ProductDetail / DeptTree 的访问控制(非超管自动剪枝);
|
|
|
|
|
- - M-3:`UserDetail` 的 PII filter;
|
|
|
|
|
- - M-4:post-commit 缓存清理失败不再映射 500。
|
|
|
|
|
-3. **P2 配套**:
|
|
|
|
|
- - M-5:`CheckManageAccess` 支持 prefetched target;
|
|
|
|
|
- - M-6:`ExtractClientIP` 支持 `X-Forwarded-For` + IP 合法性校验;
|
|
|
|
|
- - M-7:统一的 `parseHS256` helper 显式校验签名算法;
|
|
|
|
|
- - M-8:`IncrementTokenVersionIfMatch` 接收 username 参数。
|
|
|
|
|
-4. **P3 收尾 / 日常清理**:L-1 ~ L-6。
|
|
|
|
|
-
|
|
|
|
|
-### 与第 5 轮的关系
|
|
|
|
|
-
|
|
|
|
|
-- 第 5 轮 **实际落地并复盘通过**:H-1(CAS)、H-2(gRPC 限流)、H-3(同级放行修 `<=`)、H-4(deptId=0 守门)、M-3(负缓存)、M-5(错误消息字符串匹配移除)、M-7(gRPC IP 剥 host:port)、M-8(ChangePassword 限流)、L-1(MustChangePasswordYes)、L-4(FindMinPermsLevelByUserIdAndProductCode 区分 NotFound)。
|
|
|
|
|
-- 第 5 轮 **未完全落地** / **遗留**:M-2(UpdateDept Clean 批处理)→ 本轮 M-1;M-6(SyncPerms FOR UPDATE)→ 本轮 H-3(因基础设施完成提高到 P0)。
|
|
|
|
|
-- 第 5 轮 **未覆盖**:登录侧信道(H-2)、admin DoS(H-1)、水平信息披露(M-2 / M-3)、post-commit 缓存失败的 500 误映射(M-4)、客户端 IP 提取不全(M-6)、JWT `token.Method` 断言(M-7)——本轮全部新增。
|
|
|
|
|
-
|
|
|
|
|
-> 说明:本轮 findings 均在当前 HEAD 代码中复现无误;H 级别问题均给出可触发的真实业务 / 攻击路径,而非纯理论风险。
|
|
|