|
|
@@ -1,270 +1,242 @@
|
|
|
-# 权限管理系统 —— 深度代码审计报告(第 7 轮)
|
|
|
+# 权限管理系统 —— 深度代码审计报告(第 8 轮)
|
|
|
|
|
|
> **审计范围**:`/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` 的目标侧授权缺失等高风险项。
|
|
|
+> **与第 7 轮对比**:
|
|
|
+> - **已落地(本轮不再复列)**:H-1 `loadPerms` deny fail-open、H-3 `AddMember` 目标侧授权、H-4 JWT HMAC 断言、M-1 `Load` 半成品缓存污染、M-2 `UpdatePassword`/`UpdateStatus` 未校验 `RowsAffected`、M-4 `CreateProduct` 明文密码(切成一次性 `credentialsTicket`)、M-7 gRPC 限流剥端口、L-1 `DeleteDept` 锁序列、L-2 `IncrementTokenVersion` WARN 注释、L-3 `loadPerms` 其余分支 fail-close、L-4 SQL `status = ?` 参数化(`sysUserRole` / `sysUserPerm` / `sysRole` 三处)、L-5 `FindMapByProductCodeUserIds` / `FindMapByProductCode` 非事务版移除、L-6 gRPC 负缓存预污染(TTL=10s + 预写前强一致 `FindOne`)。
|
|
|
+> - **未落地 / 回归**:**H-2 PII 暴露**(第 6~7 轮持续未修),**M-3 残留分支**(`GuardRoleLevelAssignable` 已走 fresh read,但 `CheckManageAccess → checkPermLevel` 仍读缓存 `caller.MinPermsLevel`,降级 admin 的 TOCTOU 窗口只封了"分配角色"一个出口),L-5 `CountActiveAdminsTx` 零调用,L-7 历史账号 `DeptId=0` 兜底。
|
|
|
+> - **新发现**:M-N1 `CreateProduct → Redis 票据写入失败` 导致**产品/管理员孤儿化**;M-N2 `checkPermLevel` 读缓存 `MinPermsLevel` 的 TOCTOU 口子(M-3 未闭合的另一半);M-N3 `SyncPermsError{Code:404}` 在 gRPC 映射里被同化成 `codes.Internal`;L-N1 `sysPermModel` 仍用 `fmt.Sprintf("... status = %d", consts.StatusEnabled)`,与 L-4 修复风格不一致;L-N2 `SetUserPerms` `FindByIds` 校验与 `BatchInsert` 之间的 TOCTOU(影响面轻,列入存档)。
|
|
|
|
|
|
---
|
|
|
|
|
|
## 🚩 核心逻辑漏洞 (High Risk)
|
|
|
|
|
|
-### 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"权限集,**有多少次请求就有多少次越权**。
|
|
|
-- **影响**:
|
|
|
- - **任意 deny-revoke 授权操作在单次瞬时 DB 抖动下 5 分钟内失效**。考虑到 `setUserPermsLogic` 的主要用途就是"临时撤销某用户对敏感权限的访问",这类 deny 往往就是最后一道安全闸。闸被 silently 打开 5 分钟。
|
|
|
- - 攻击者若能制造一次对 `sys_user_perm` 的短时读失败(例如对该表发起 hot-row 争抢使 `denyIds` 的查询 timeout),即可让目标用户的 deny 被旁路。
|
|
|
- - 与 R6 H-3 / H-4 不同,这是一条纯代码路径问题,不依赖配置、不依赖代理头,**出现概率等于 DB 抖动概率**。
|
|
|
-- **修复方案**:把两次查询在错误语义上**对称**处理:
|
|
|
- ```go
|
|
|
- denyIds, err := l.models.SysUserPermModel.FindPermIdsByUserIdAndEffectForProduct(ctx, ud.UserId, consts.PermEffectDeny, ud.ProductCode)
|
|
|
- if err != nil {
|
|
|
- logx.WithContext(ctx).Errorf("userDetailsLoader: load deny perms failed: %v", err)
|
|
|
- return // fail-close:宁可让用户看到 0 perms 让他们刷新,也不能把 deny 旁路
|
|
|
- }
|
|
|
- ```
|
|
|
- 同时**本次加载不要写缓存**,交给下一次 Load 重试,或者把失败信号往上传(见 M-1)。
|
|
|
-
|
|
|
-### H-2. `UserDetailLogic` / `UserListLogic` 仍把 `Email` / `Phone` / `Remark` 暴露给**任意同产品成员**(R6 M-3 未落地)
|
|
|
+### H-1. `UserDetailLogic` / `UserListLogic` 仍把 `Email` / `Phone` / `Remark` 暴露给**任意同产品成员**(第 6 轮 M-3、第 7 轮 H-2,三轮未落地)
|
|
|
- **位置**:
|
|
|
- - `internal/logic/user/userDetailLogic.go:68-70`
|
|
|
- - `internal/logic/user/userListLogic.go:81-83`
|
|
|
-- **描述**:两接口的访问控制是"同产品 → 返回完整资料"。`UserDetail` 仅用 `FindOneByProductCodeUserId` 检查目标与调用方在同一产品内,就返回 `Email`、`Phone`、`Remark`(纯文本,无脱敏)。`UserList` 更严重 —— 一次分页可以批量拿到同产品所有成员的手机号与邮箱。
|
|
|
+ - `internal/logic/user/userDetailLogic.go:64-76`(`types.UserItem` 构造)
|
|
|
+ - `internal/logic/user/userListLogic.go:77-89`
|
|
|
+- **描述**:两接口的访问控制仍然只到"同产品即可看全字段"的粒度:
|
|
|
+ - `UserDetail` 只用 `FindOneByProductCodeUserId` 核对 caller 与 target 是否在同一产品,之后直接塞 `Email / Phone / Remark`(纯文本,无脱敏)。
|
|
|
+ - `UserList` 分页返回同产品所有成员,每条也原样带上 `Email / Phone / Remark`。一次分页可以把整个产品通讯录灌下来。
|
|
|
- **影响**:
|
|
|
- - **同产品最低权限 MEMBER 即可遍历整个产品通讯录**,获取手机 / 邮箱 / 备注(备注里可能含 PII / 内部身份)。
|
|
|
- - 与 H-3(`AddMember` 不做目标侧授权)组合后威力更强:一个产品 ADMIN 可先把想看的任意人(包括其部门树外、不归自己管的用户)强行拉入自己的产品,再通过 `UserDetail` 或 `UserList` 抽走其 PII。
|
|
|
- - 严重违反 GDPR / 《个人信息保护法》最小必要原则。
|
|
|
-- **修复方案**:
|
|
|
- 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
|
|
|
- if targetUser.IsSuperAdmin == consts.IsSuperAdminYes {
|
|
|
- return nil, response.ErrForbidden("无法将超级管理员加入具体产品")
|
|
|
- }
|
|
|
- if err := authHelper.CheckManageAccess(l.ctx, l.svcCtx, req.UserId, req.ProductCode,
|
|
|
- authHelper.WithPrefetchedTarget(targetUser)); err != nil {
|
|
|
- return nil, err
|
|
|
- }
|
|
|
- ```
|
|
|
- 注意:`CheckManageAccess` 内部对 "caller 是 ADMIN" 会短路 `checkDeptHierarchy`,所以这不会让产品 ADMIN 失去管理自己下属 / 旁部门用户的能力,但会拦住"部门树外 + 不归自己管"这类真正的越权路径。
|
|
|
-
|
|
|
-### H-4. JWT 解析三处 `keyfunc` **未显式断言 `*jwt.SigningMethodHMAC`**(R6 M-7 未落地)
|
|
|
+ - **同产品最低权限 MEMBER 即可遍历整个产品通讯录**,获取手机 / 邮箱 / 备注(备注里常常会有内部身份、外部联络人、岗位说明等 PII)。
|
|
|
+ - 叠加风险:
|
|
|
+ - 即便 H-3(第 7 轮,已修)已关掉了"ADMIN 跨部门 AddMember"的入口,现有 ADMIN 仍可通过 `UserList` 一次拿走本产品全员通讯录;
|
|
|
+ - 对于拿到一次性有效 JWT 的外包 / 服务账号,本接口天然就是拖库点。
|
|
|
+ - 违反《个人信息保护法》第 6 / 13 条的最小必要原则与《数据安全法》下的分级保护要求。
|
|
|
+- **修复方案**(与前两轮报告一致,代码未落):
|
|
|
+ 1. 在 `internal/logic/auth/access.go` 新增:
|
|
|
+ ```go
|
|
|
+ // CanViewContact 判定 caller 是否可以看 target 的联系信息字段。
|
|
|
+ // - caller 是 SuperAdmin → true
|
|
|
+ // - caller.UserId == target.Id(看自己) → true
|
|
|
+ // - CheckManageAccess(caller, target) 通过(caller 在 target 的管理链上) → true
|
|
|
+ // - 其余情况一律 false,由 filterPIIForCaller 落地脱敏。
|
|
|
+ func CanViewContact(ctx context.Context, svcCtx *svc.ServiceContext, caller *loaders.UserDetails, target *userModel.SysUser, productCode string) bool
|
|
|
+ ```
|
|
|
+ 2. 新增 `authHelper.MaskEmail` / `MaskPhone`(`138****1234` / `a***@b.com`),在 Logic 构造 DTO 前调用 `filterPIIForCaller(caller, target, &item)` 统一覆盖 `Email / Phone / Remark` 三字段。
|
|
|
+ 3. 单测覆盖四种身份组合:同产品同级 MEMBER 互看、跨部门互看、ADMIN 看下级、看自己。
|
|
|
+- **为什么必须本轮解决**:前两轮已经连续标记为 P0 且提出了完整方案,在 H-3(AddMember)修完之后这条变成"剩下最大的单点 PII 出口",攻击面大、修复工作量小(一个 helper + 两个 Logic 返回前的 hook)。
|
|
|
+
|
|
|
+### H-2. `CheckManageAccess → checkPermLevel` 仍靠 caller 的**缓存 `MinPermsLevel`** 决策,降级 admin 在 5 分钟 TTL 内仍可管辖原本够不到的目标(M-3 只封了一半)
|
|
|
- **位置**:
|
|
|
- - `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,三处统一调用:
|
|
|
+ - `internal/logic/auth/access.go:279-316`(`checkPermLevel`)
|
|
|
+ - 受影响调用方:`internal/logic/member/updateMemberLogic.go`、`internal/logic/member/removeMemberLogic.go`、`internal/logic/user/updateUserLogic.go`、`internal/logic/user/updateUserStatusLogic.go`、`internal/logic/user/setUserPermsLogic.go`
|
|
|
+- **描述**:第 7 轮对 `GuardRoleLevelAssignable` 的修复方式是"在做 role 分配决策前 `FindMinPermsLevelByUserIdAndProductCode` 走一次 NoCache 查 DB"。但**同样类型的 TOCTOU 问题**在更一般的 `CheckManageAccess` 里没修:
|
|
|
```go
|
|
|
- // 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 []byte(secret), nil
|
|
|
- })
|
|
|
+ // access.go:312
|
|
|
+ if caller.MinPermsLevel >= targetLevel {
|
|
|
+ return response.ErrForbidden("无权管理权限级别高于或等于您的用户")
|
|
|
}
|
|
|
```
|
|
|
+ 这里 `caller` 来自 `middleware.GetUserDetails(ctx)`,也就是 `UserDetailsLoader.Load` 缓存的那份;TTL 5 分钟。
|
|
|
+ 攻击路径:
|
|
|
+ 1. T=0:超管把 caller C 从 `MinPermsLevel=10`(总监级)降到 `20`(经理级)。超管调用业务接口时会触发 `UserDetailsLoader.Clean(C.UserId)`,但 Redis 抖动 / 集群主从切换期间,`Clean` 单次失败只会被 `logx.Errorf`,没有重试、没有降级旁路。
|
|
|
+ 2. T=0+δ:C 在另一终端调 `RemoveMember` / `UpdateMember` / `UpdateUser` / `UpdateUserStatus` / `SetUserPerms` 去管理一个 `MinPermsLevel=15` 的目标 D。
|
|
|
+ 3. `checkPermLevel` 读到 C 的缓存 `MinPermsLevel=10`,判定 `10 >= 15` 为 false → 放行。C 的**实际**级别 20,`20 >= 15` 为 true,本应被拦。
|
|
|
+ 4. C 有整整 5 分钟时间把下属统一踢人、改 MemberType、覆盖 UserPerms、冻结状态 —— 审计日志里看起来是合法操作。
|
|
|
+- **与 M-3 修复的对照**:`GuardRoleLevelAssignable` 修的是"C 用旧身份去授角色给别人"的路径;`checkPermLevel` 漏的是"C 用旧身份去直接动别人"的路径。两条路径一个在**出**一个在**改**,安全边界对称才叫完整。
|
|
|
+- **影响**:与 M-3 同一量级,但触达面更广(5 个 Logic 都走 `CheckManageAccess`),且 `RemoveMember` / `UpdateUserStatus` 可以直接产生**不可逆的破坏性操作**(把管理员从产品踢出、冻结账号)。
|
|
|
+- **修复方案**:
|
|
|
+ 1. `checkPermLevel` 对 caller.MinPermsLevel 采用与 `GuardRoleLevelAssignable` 一致的策略:
|
|
|
+ ```go
|
|
|
+ freshCallerLevel, err := svcCtx.SysRoleModel.FindMinPermsLevelByUserIdAndProductCode(ctx, caller.UserId, productCode)
|
|
|
+ if err != nil {
|
|
|
+ if errors.Is(err, sqlx.ErrNotFound) {
|
|
|
+ // caller 当前已无产品角色 → 等同最低等级,对同级管辖一律拒绝
|
|
|
+ return response.ErrForbidden("无权管理权限级别高于或等于您的用户")
|
|
|
+ }
|
|
|
+ return response.NewCodeError(500, "校验权限级别失败,请稍后重试")
|
|
|
+ }
|
|
|
+ if freshCallerLevel >= targetLevel {
|
|
|
+ return response.ErrForbidden("无权管理权限级别高于或等于您的用户")
|
|
|
+ }
|
|
|
+ ```
|
|
|
+ 2. 抽一个共享 helper `loadFreshMinPermsLevel(ctx, svcCtx, userId, productCode) (int64, error)`,`GuardRoleLevelAssignable` 和 `checkPermLevel` 统一调用;同时顺手把两处的 `ErrNotFound` 语义文档化。
|
|
|
+ 3. 长期建议(可选):给 `UserDetailsLoader` 加一个 `LoadFresh(ctx, userId, productCode)` 接口,授权决策点强制 bypass 缓存;普通业务依然走缓存。
|
|
|
|
|
|
---
|
|
|
|
|
|
## ⚠️ 健壮性与性能建议 (Medium / Low)
|
|
|
|
|
|
-### M-1. `UserDetailsLoader.Load` 把 "DB 瞬时故障" 同化为 "用户不存在",副作用是**把半成品 UD 写入 5 分钟缓存**
|
|
|
-- **位置**:`internal/loaders/userDetailsLoader.go:119-165, 284-304`
|
|
|
-- **描述**:
|
|
|
- - `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. `Load` 返回 `(*UserDetails, error)`,让中间件自己区分 "NotFound → 401 用户不存在" 与 "其他错误 → 503 服务暂时不可用"。
|
|
|
- 2. `loadFromDB` 里任何子步骤出错,都**不要写缓存**(让下次 Load 重试)。
|
|
|
- 3. 如果还是想保留无 error 返回,至少在 ud 上加一个 `PartiallyLoaded bool` 字段,Load 写缓存前检查这一位。
|
|
|
-
|
|
|
-### M-2. `SysUserModel.UpdatePassword` / `UpdateStatus` **不校验 `RowsAffected`**,对已删除 / 条件不满足的用户会静默成功
|
|
|
+### M-1. `CreateProduct` 事务已提交后,Redis 票据写入失败会把产品 + admin 用户**永久孤儿化**(第 7 轮 M-4 修复方案遗留的新缺陷)
|
|
|
+- **位置**:`internal/logic/product/createProductLogic.go:109-185`
|
|
|
+- **描述**:第 7 轮把 M-4(响应体明文密码)改成了"DB 事务提交 → 暂存 Redis → 响应 ticket"三段流水线。但流水线的失败语义没有做补偿:
|
|
|
+ 1. L109-149:`SysProductModel.TransactCtx` 里同时 INSERT `sys_product` / `sys_user`(admin 账号)/ `sys_product_member`。成功后 `productId` 已经持久化。
|
|
|
+ 2. L162-169:`generateRandomHex(32)` 生成 ticket —— 理论上 `crypto/rand.Read` 出错概率 ≈ 0,但已不在事务内。
|
|
|
+ 3. L176-180:`json.Marshal(&payload)` —— 对固定结构体几乎不会失败,但落出错分支同样只返回 500。
|
|
|
+ 4. L181-184:`Redis.SetexCtx(ticketKey, …)` —— **这里才是真实风险面**。Redis 短暂不可用 / 超时 / 集群 failover 都会让 Setex 返 err。
|
|
|
+ 落到任一 fail 分支时:
|
|
|
+ - `sys_product` 里新建的产品记录已经落盘;
|
|
|
+ - `admin_<code>` 账号已经落盘,bcrypt 密码是**我们刚刚在内存里生成、随 500 响应丢弃**的那串随机 12 字节;
|
|
|
+ - 没有任何方式可以拿回这个密码:
|
|
|
+ - 仓库里没有 `DeleteProductLogic`(只有 `CreateProduct` / `UpdateProduct` / `ProductList` / `ProductDetail`);
|
|
|
+ - 仓库里没有 "SuperAdmin Reset Password" 类型的接口,只有 `ChangePasswordLogic`,且它要求 `oldPassword` 校验通过;
|
|
|
+ - `CreateProduct` 再试一次会命中 `product.Code` / `admin_<code>` 的 `FindOneByCode` / `FindOneByUsername` 前置判定,直接 `ErrConflict`。
|
|
|
+ 运维只能下场直接 SQL:要么跑一次 `UPDATE sys_user SET password = ? WHERE username = ?` 硬改 admin 密码,要么 `DELETE` 三张表对应数据。**这两种手法都绕过了业务不变式和审计日志。**
|
|
|
+- **影响**:数据完整性问题。概率虽低(依赖 Redis 单次失败 & 在一次创建流程内),但一旦发生是**永久性**的(孤儿产品不会自愈),且需要手工改库,违反最小事故介入原则。
|
|
|
+- **修复方案**(按代价从低到高):
|
|
|
+ 1. **最小成本**:`Redis.SetexCtx` 失败时,在同一 handler 内做补偿 DB 事务——删除刚才创建的 `sys_product` / `sys_user` / `sys_product_member`,回到"从未创建"状态再返 500。为防止补偿事务自己也失败,至少要把 `productId` / `adminUserId` 落一条 `logx.Errorw("createProduct compensation required", ...)` 带有 ERROR 等级 + 结构化字段,方便告警侧接管人工回捞。
|
|
|
+ 2. **更稳**:新增 `SuperAdmin` 专用接口 `RegenerateInitialCredentials(productCode)` —— 找到 `admin_<code>`,重新生成随机密码、bcrypt 后写 DB,再走一次 `SetexCtx` + 返回新的 `credentialsTicket`;与 `ChangePassword` 解耦,有独立审计日志字段 `audit=regenerate_init_cred`。
|
|
|
+ 3. **一步到位**:引入 outbox / 2PC 风格 —— DB 事务 + `sys_outbox` 行同时写;单独 worker 消费 outbox,驱动 Redis 写入;对调用方接口异步化(响应体先给 `ticketId`,轮询拉真值)。落地成本高,按本仓规模不建议。
|
|
|
+- **锁死建议**:无论采纳 (1) 还是 (2),在上线前至少要补齐:`CreateProduct` 集成测试中把 `Redis.Setex` 做成主动注错路径,断言 "产品不存在 / admin 用户不存在(方案 1)" 或 "可以通过 Regenerate 拿回凭证(方案 2)",防止未来重构又漏掉这一路径。
|
|
|
+
|
|
|
+### M-2. `SyncPermsError{Code: 404}` 在 gRPC 映射里被同化成 `codes.Internal`,接口契约泄露 DB 语义
|
|
|
- **位置**:
|
|
|
- - `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 会"后写覆盖先写"且都返回成功,用户拿到的新密码是无法预测的那一份(在已知旧密码共谋场景下影响低)。
|
|
|
-- **建议**:
|
|
|
+ - `internal/logic/pub/syncPermsService.go:75-79`(事务内 `LockByCodeTx` 返 `ErrNotFound` → `SyncPermsError{Code: 404}`)
|
|
|
+ - `internal/server/permserver.go:67-83`(gRPC 层映射)
|
|
|
+- **描述**:gRPC handler 的 `switch se.Code` 只列了 400 / 401 / 403 / 409,其余(含 404 / 500 / 任何异常值)一律落 `default → codes.Internal`。于是:
|
|
|
+ 1. 前置 `FindOneByAppKey` 已经能 hit 到一条 `sys_product`,但事务内 `LockByCodeTx` 又 `ErrNotFound`(极罕见:并发 "DeleteProduct" —— 虽然目前没有 Delete 入口,但一旦加上就中招);
|
|
|
+ 2. 调用方(产品接入方的服务端)拿到 `codes.Internal` + 消息 "产品不存在",但 SDK 侧的重试策略一般对 `Internal` 是 "not retriable" —— 调用方会直接当作永久错误上报,而我们实际希望它是 `codes.NotFound` 让其按 404 处理。
|
|
|
+- **影响**:契约级一致性缺陷。目前无直接安全风险,但一旦未来引入 DeleteProduct / ProductCode 重命名逻辑,会给接入方的调用栈制造误导性错误分类(Internal 会 page 值班,NotFound 不会)。
|
|
|
+- **修复方案**:
|
|
|
```go
|
|
|
- res, err := m.ExecCtx(...)
|
|
|
- if err != nil { return err }
|
|
|
- if n, _ := res.RowsAffected(); n == 0 {
|
|
|
- return ErrNotFound // 或自定义 ErrUpdateConflict
|
|
|
- }
|
|
|
- return nil
|
|
|
+ case 404:
|
|
|
+ return nil, status.Error(codes.NotFound, se.Message)
|
|
|
```
|
|
|
- 另外建议对 `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. `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. 响应里把 `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`(因为这里是存在性判断而不是修改)。
|
|
|
+ 同时把 HTTP 侧的 `response.NewCodeError(se.Code, …)` / gRPC 侧的映射表都抽到一个 `mapSyncPermsErr` helper,避免两边漂移。
|
|
|
|
|
|
-### 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. 给 `IncrementTokenVersion` 加个 `// WARN: 仅限强制全量失效(Logout / 封禁)。Refresh/Rotate 必须使用 IncrementTokenVersionIfMatch。` 的显式 header 注释。
|
|
|
- 2. 最干净的做法:把 `IncrementTokenVersion` 改成 package-private,再在 `logout` 所在 package 用**显式命名的 wrapper**(`ForceRevokeAllSessions`)暴露,新接入者一眼看到红色标签。
|
|
|
-
|
|
|
-### L-3. `loadPerms` 其他分支的错误同样被静默 log(`rolePermIds` / `allowIds` / `FindAllCodesByProductCode`),和 H-1 同宗
|
|
|
-- **位置**:`internal/loaders/userDetailsLoader.go:435-498`
|
|
|
+### M-3. `RefreshToken` CAS 成功后,若 `GenerateAccessToken` / `GenerateRefreshTokenWithExpiry` 失败,tokenVersion 已递增但客户端拿不到新令牌,用户被强制退出
|
|
|
+- **位置**:`internal/server/permserver.go:198-229`
|
|
|
- **描述**:
|
|
|
- - L450-453:`FindPermIdsByRoleIds` 失败 → `rolePermIds` 保持空。若此时 role 权限正常、但查询临时失败,用户的"角色→权限"整块就被丢掉。
|
|
|
- - L435-437:`FindAllCodesByProductCode` 失败 → `ud.Perms = nil`(对 ADMIN / DEV 部门这类"全量权限"角色来说直接降成 0 perm)。
|
|
|
- - L487-498:`FindByIds` 失败 → `ud.Perms = nil`。
|
|
|
-
|
|
|
- 所有这些都会被 `Load` 写入 5 分钟缓存。对 ADMIN 来说是"5 分钟内所有权限消失",对普通成员来说是"5 分钟内权限表不一致"。用户体感就是间歇性 403,定位困难。
|
|
|
-- **建议**:与 M-1 同步修复:loadPerms 内任一子步骤返回 error,**整次 Load 跳过缓存写入**,同时把 error 传给 `Load`,由上层决定是 503 还是 401。
|
|
|
-
|
|
|
-### L-4. Model 层 SQL 中的 `status = 1` 硬编码,与 `consts.StatusEnabled` 脱钩
|
|
|
-- **位置**:
|
|
|
- - `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`)风格一致。
|
|
|
-
|
|
|
-### 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. `Load` 在写入负缓存之前,再通过 `SysUserModel.FindOne(ctx, userId)` 强一致校验一次(绕过 cache),确认真 NotFound 才写哨兵。
|
|
|
- 2. 或者在 `CreateUserLogic` 成功插入之后主动 `Del` 掉 `ud:newId:*` 的负缓存键(需要遍历产品维度,因此成本较高)。
|
|
|
- 3. 最简:`negativeCacheTTL` 从 30 → 10,并加一条 `svc` 级的"新用户创建后 30s 内绕过负缓存"的白名单(按 userId > `watermark` 判定)。
|
|
|
+ - L199:`IncrementTokenVersionIfMatch` 成功把 DB 的 `tokenVersion` 从 N 增到 N+1,返回 `newVersion`。
|
|
|
+ - L206:`UserDetailsLoader.Clean(ctx, claims.UserId)` 清缓存(此时 TokenVersion 在 DB 是 N+1,但在用户手上的 refreshToken 还是 N)。
|
|
|
+ - L208-214:生成新 accessToken 失败(理论上 HMAC signing 几乎不会失败,但 OOM / 奇怪的运行时错误不是 0 概率)。
|
|
|
+ - L216-223:生成新 refreshToken 失败亦然。
|
|
|
+ - 任一处失败就直接 `return nil, status.Error(codes.Internal / codes.Unauthenticated, ...)`。**客户端的老 refreshToken 因为 tokenVersion 对不上,下一次刷新会被 `claims.TokenVersion != ud.TokenVersion` 一刀切成 "登录状态已失效"**。用户必须完整重登,体验上等同于被踢下线。
|
|
|
+- **影响**:可用性 / 数据完整性:签名失败原本是 100% 服务端 bug,用户却要重登。量不大但会污染"非预期登出"告警,淹没真正的会话劫持信号。
|
|
|
+- **修复方案**:
|
|
|
+ 1. 重排顺序:先生成两个新 token(不成功就直接 500,不动 DB),成功后再 CAS 递增 tokenVersion、Clean 缓存。即便递增后 log 层有问题,至少签名成功的 token 一定带回给了客户端。
|
|
|
+ 2. 若坚持"先 CAS 再签名"(语义上更安全:CAS 成功才证明本次 refresh 是唯一 winner),则失败路径需要记一条明确的 `audit=refresh_post_cas_sign_fail userId=X oldVer=N newVer=N+1` 的 ERROR 日志,并把返回消息改成用户可感知的 "登录刷新失败,请重新登录"(保留现有行为但补上 observability)。
|
|
|
|
|
|
-### 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. 运维侧一次性迁移 `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-1. `sysPermModel.go` SQL 仍用 `fmt.Sprintf("... status = %d", consts.StatusEnabled)`,与 L-4 本轮修复风格不一致
|
|
|
+- **位置**:`internal/model/perm/sysPermModel.go:59,102,135`
|
|
|
+ ```go
|
|
|
+ query := fmt.Sprintf("SELECT `code` FROM %s WHERE `productCode` = ? AND `status` = %d", m.table, consts.StatusEnabled)
|
|
|
+ findQuery := fmt.Sprintf("SELECT %s FROM %s WHERE `productCode` = ? AND `status` = %d", sysPermRows, m.table, consts.StatusEnabled)
|
|
|
+ updateQuery := fmt.Sprintf("UPDATE %s SET `status` = %d, `updateTime` = ? WHERE `productCode` = ? AND `status` = %d", m.table, consts.StatusDisabled, consts.StatusEnabled)
|
|
|
+ ```
|
|
|
+- **描述**:第 7 轮 L-4 已经把 `sysUserRole` / `sysUserPerm` / `sysRole` 三处改成了占位符 `?` + 参数传 `consts.StatusEnabled`,但 `sysPermModel` 的三条查询还是走 `fmt.Sprintf` 把 `int` 直接嵌到 SQL 里。问题不是注入(`consts.StatusEnabled` 是编译期常量),而是:
|
|
|
+ 1. **类型契约不稳**:如果未来把 `StatusEnabled` 从 `int` 改成 `int8` / `uint8` / `ActiveStatus` enum,`%d` 可能要改,占位符版本可以稳定。
|
|
|
+ 2. **审计一致性**:L-4 的修复初衷是"SQL 里**不再出现**状态常量字面值,统一走 prepared statement 占位"。`sysPermModel` 的三处仍然把数字编进 SQL 字符串(虽然是间接通过 Sprintf),与 L-4 的意图偏离。
|
|
|
+- **影响**:非安全问题,属于审计口径一致性 / 未来可维护性。
|
|
|
+- **修复方案**:
|
|
|
+ ```go
|
|
|
+ query := fmt.Sprintf("SELECT `code` FROM %s WHERE `productCode` = ? AND `status` = ?", m.table)
|
|
|
+ // 调用处:
|
|
|
+ QueryRowsNoCacheCtx(ctx, &dest, query, productCode, consts.StatusEnabled)
|
|
|
+ ```
|
|
|
+ 三条一致处理;`BatchUpdateWithTx` 里的 `status = ?` 同理。
|
|
|
+
|
|
|
+### L-2. `CountActiveAdminsTx` 零调用仍在接口里公开(第 7 轮 L-5 部分遗留)
|
|
|
+- **位置**:`internal/model/productmember/sysProductMemberModel.go`(接口 + 实现)
|
|
|
+- **描述**:第 7 轮已经把 `FindMapByProductCodeUserIds` / `FindMapByProductCode`(非事务版)从模型接口里干掉。但与之同批引入的 `CountActiveAdminsTx`(不带 `Other`)在业务层的实际调用方是 `CountOtherActiveAdminsTx`;`CountActiveAdminsTx` 自身只在 mock / test 里被引用。
|
|
|
+- **验证方式**:
|
|
|
+ ```shell
|
|
|
+ rg -n 'CountActiveAdminsTx' internal/logic | wc -l # 0
|
|
|
+ rg -n 'CountOtherActiveAdminsTx' internal/logic | wc -l # >0
|
|
|
+ ```
|
|
|
+- **影响**:僵尸接口方法,增加接口 surface area 与未来误用机会("应该用 `CountActiveAdminsTx` 还是 `CountOtherActiveAdminsTx`?"的歧义)。
|
|
|
+- **修复方案**:从 interface / 实现 / mock / 单测里删除 `CountActiveAdminsTx`;保留一条 `CountOtherActiveAdminsTx`(业务语义是"除自己以外还有几个活跃 admin",刚好吻合"不能移除/降级最后一个 admin"的不变式)。
|
|
|
+
|
|
|
+### L-3. `CheckManageAccess` 对 caller `DeptId=0` 且非 ADMIN / SuperAdmin 的历史账号直接 403(第 7 轮 L-7 未消化)
|
|
|
+- **位置**:`internal/logic/auth/access.go`(`checkDeptHierarchy` 内 `caller.DeptId == 0 || caller.DeptPath == ""` 分支)
|
|
|
+- **描述**:历史遗留账号仍有 `DeptId=0` 的 MEMBER / DEVELOPER,即使在自己的产品范围内想做简单的"看自己 / 改自己"操作,也会被 `checkDeptHierarchy` 拒 403(除非上层已短路)。
|
|
|
+- **修复方案**(任选其一,两者都不破坏现有安全边界):
|
|
|
+ 1. 运维侧一次性 SQL:
|
|
|
+ ```sql
|
|
|
+ UPDATE sys_user SET deptId = <DEFAULT_NORMAL_DEPT_ID>
|
|
|
+ WHERE deptId = 0 AND isSuperAdmin = 0 AND (
|
|
|
+ userId IN (SELECT userId FROM sys_product_member WHERE memberType != 'ADMIN')
|
|
|
+ OR userId NOT IN (SELECT userId FROM sys_product_member)
|
|
|
+ );
|
|
|
+ ```
|
|
|
+ 同时 `UserDetailsLoader.CleanByUserIds` 一次性批量清缓存。
|
|
|
+ 2. 代码侧:`CheckManageAccess` 早期追加
|
|
|
+ ```go
|
|
|
+ if caller.UserId == targetUserId {
|
|
|
+ return nil
|
|
|
+ }
|
|
|
+ ```
|
|
|
+ (这一条已经在 L60-69 实现了,但仅对"看自己"生效;真正被 403 的路径是"看同部门的人"——仍需要数据修复)。
|
|
|
+
|
|
|
+### L-4. `SetUserPerms` 在 `FindByIds` 校验与 `BatchInsertWithTx` 之间存在 perm 状态 TOCTOU
|
|
|
+- **位置**:`internal/logic/user/setUserPermsLogic.go:90-130`
|
|
|
+- **描述**:
|
|
|
+ - L95-109:循环检查 `dbPerms` 里每条权限都 `ProductCode == productCode` 且 `Status == StatusEnabled`;
|
|
|
+ - L112-131:在新事务里 `DeleteByUserIdForProductTx` + `BatchInsertWithTx`。
|
|
|
+ - 两段之间若并发一次 `SyncPermissions` 把某个 permId 的 `status` 置成 DISABLED,本次 `SetUserPerms` 依然会把那条 `user_perm` 落库。
|
|
|
+- **影响**:`loadPerms` 在组装缓存时是 `JOIN sys_perm WHERE sys_perm.status = ?`(L-4 修过的部分),失效权限不会真正生效。本漏洞**不造成越权**,但会留下一行"脏"`sys_user_perm`,让审计查询 "X 拥有哪些权限" 与 "X 实际享有哪些权限" 出现微小偏差,是数据一致性噪声。
|
|
|
+- **修复方案**(按实际需要决定是否做):
|
|
|
+ 1. 把 `FindByIds` 挪到事务内,并对 `sys_perm` 这几行加 `FOR SHARE`(`SyncPerms` 已经通过 `LockByCodeTx` 在 sys_product 行上串行化;这里加 `FOR SHARE` 只是为了在事务边界内读到一致的 status,开销很低);
|
|
|
+ 2. 或者最廉价:在 `BatchInsertWithTx` 之后补一条 `COUNT(*)` 校验 —— "我刚才插入的 permId 全部仍是 Enabled",不满足就主动回滚事务。任一方案都能把 TOCTOU 窗口缩到零。
|
|
|
|
|
|
---
|
|
|
|
|
|
## 📋 修复优先级汇总
|
|
|
|
|
|
-| 优先级 | 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) | 历史遗留账号,运维侧补数据或代码兜底"看自己" |
|
|
|
+| 优先级 | finding | 一句话概要 |
|
|
|
+| :----- | :------------------------------------------------------------ | :------------------------------------------------------------------------------------------ |
|
|
|
+| **P0** | **H-1** UserDetail/UserList PII 暴露(3 轮未落地) | 任意同产品 MEMBER 可读全员手机邮箱备注,违反 PIPL 最小必要 |
|
|
|
+| **P0** | **H-2** `checkPermLevel` 仍读缓存 MinPermsLevel(M-3 另一半) | 降级 admin 在 5 分钟 TTL 内仍可 RemoveMember / UpdateMember / SetUserPerms |
|
|
|
+| P1 | **M-1** CreateProduct Redis 写失败 → 产品 + admin 孤儿化 | 一次 Redis 失败 → 永久不可恢复,只能手动 SQL |
|
|
|
+| P1 | **M-2** SyncPermsError 404 → codes.Internal 契约错位 | 接入方 SDK 的错误分类/重试策略失真 |
|
|
|
+| P1 | **M-3** RefreshToken CAS 后签名失败 → 用户被强制登出 | tokenVersion 已推进,客户端必须重登,可用性事件 |
|
|
|
+| P2 | **L-1** `sysPermModel` 仍用 `fmt.Sprintf`(L-4 风格漂移) | 三条 SQL 统一改占位符,接近审计一致性 |
|
|
|
+| P2 | **L-2** `CountActiveAdminsTx` 零调用(L-5 遗留) | 接口层可见的僵尸方法 |
|
|
|
+| P2 | **L-3** `CheckManageAccess` 对 `DeptId=0` 老账号 403(L-7) | 需数据迁移或 "看自己" 短路 |
|
|
|
+| P3 | **L-4** SetUserPerms FindByIds / BatchInsert TOCTOU | 不越权,但会留脏 `sys_user_perm` 行 |
|
|
|
+
|
|
|
+---
|
|
|
|
|
|
## 🛠 建议修复次序
|
|
|
|
|
|
-1. **P0 同批上线**(同一次发版一起修,互相放大):
|
|
|
- - H-1:给 deny-list 查询改 fail-close。
|
|
|
- - H-2:`filterPIIForCaller` 在 UserDetail / UserList 返回前强制走一遍。
|
|
|
- - H-3:`AddMember` 追加 `CheckManageAccess` + 超管判定。
|
|
|
- - H-4:抽 `parseWithHMAC` helper,三处 `keyfunc` 替换。
|
|
|
+1. **本轮必修 P0**(对称封口):
|
|
|
+ - H-1:`filterPIIForCaller` + `CanViewContact` 两个 helper,落到 `UserDetail` / `UserList` 返回前。
|
|
|
+ - H-2:`checkPermLevel` 把 `caller.MinPermsLevel` 换成 `loadFreshMinPermsLevel(...)`;与 `GuardRoleLevelAssignable` 共享同一个 helper。两条 P0 建议同批上线,一起做回归单测。
|
|
|
|
|
|
-2. **P1 紧随**:
|
|
|
- - M-1 + L-3:一起做 "Load/loadPerms 错误模型重构"。接口改成 `(*UserDetails, error)`,半加载不写缓存。
|
|
|
- - M-2:两处 `ExecCtx` 后加 `RowsAffected` 判定。
|
|
|
- - M-3:`GuardRoleLevelAssignable` 改走 fresh read,不靠 UD 缓存。
|
|
|
- - M-4:`CreateProductResp` 换成"一次性展示链接 + 立即改密"流程。
|
|
|
+2. **P1**(稳定性与契约):
|
|
|
+ - M-1:CreateProduct 失败路径补偿(删 `sys_product` + `sys_user` + `sys_product_member`)或新增 SuperAdmin-only `RegenerateInitialCredentials`;集成测试强制注入 Redis fault。
|
|
|
+ - M-2:`SyncPermsError{Code:404}` 补到 switch 里,映射到 `codes.NotFound`;抽 `mapSyncPermsErr` 统一 HTTP / gRPC。
|
|
|
+ - M-3:RefreshToken 重排为"先签新 token 成功才做 CAS";或至少给 post-CAS 失败路径加 audit 字段。
|
|
|
|
|
|
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` "看自己" 短路。
|
|
|
+ - L-1:`sysPermModel` 三条 SQL 改占位符;
|
|
|
+ - L-2:`CountActiveAdminsTx` 删除(接口 + 实现 + mock),`CountOtherActiveAdminsTx` 保留;
|
|
|
+ - L-3:数据迁移脚本批量把历史 `deptId=0` 账号挪到默认部门,然后 `CleanByUserIds` 刷缓存;
|
|
|
+ - L-4:`SetUserPerms` 要么把 `FindByIds` 挪进事务 + `FOR SHARE`,要么事务末补一条 status 复核 `COUNT(*)`。
|
|
|
|
|
|
---
|
|
|
+
|
|
|
+## 🔎 备注:本轮已验证仍在正轨上的机制
|
|
|
+
|
|
|
+下列机制经过完整阅读后认为仍然安全、无需调整,特此备注以免未来误改:
|
|
|
+
|
|
|
+- `UserDetailsLoader`:H-1(deny fail-close)、M-1(半加载不写缓存 + 中间件按 `(ud, err)` 分流 401/503)、L-3(loadPerms 任一子步骤错误都 fail-close)、L-6(负缓存 TTL=10s + 写前 `FindOne`)全部落地。
|
|
|
+- `RefreshToken` / `Logout` 走 `IncrementTokenVersionIfMatch` / `IncrementTokenVersion` 两把刀,前者是 CAS(单会话轮转),后者是"无条件大杀器"(强制全量失效),`L-2` 的 WARN 注释已就位。
|
|
|
+- `DeleteDeptLogic` 的锁序列:`X(target dept)` → `FOR SHARE(children)` → `FOR SHARE(users in dept range)`,AB-BA 死锁风险大幅收敛。
|
|
|
+- `SyncPermissions` 通过 `LockByCodeTx` 把同 product 的 sync 串行化,sys_perm UNIQUE 再撞 1062 会落一条 `audit=mysql_error_1062` ERROR 日志,足以触发告警。
|
|
|
+- `LoginLogic` / `AdminLoginLogic` / `UserInfoLogic` 里的 `Email` / `Phone` 返回属于"看自己"合法场景,本轮不纳入 H-1 修复范围。
|
|
|
+- `ExtractClientIP` 对 `X-Forwarded-For` / `X-Real-IP` 做了严格解析 + `firstValidIP` 过滤,gRPC 侧 `net.SplitHostPort` 剥端口(M-7 已修)。
|
|
|
+- `CheckAddMemberAccess` 对 ADMIN 也强制走部门链校验 + 拒绝 `target.IsSuperAdmin`,H-3 入口已封住。
|