|
@@ -1,304 +1,232 @@
|
|
|
-# 第 13 轮深度审计报告
|
|
|
|
|
|
|
+# 深度审计报告 · Round 14
|
|
|
|
|
|
|
|
-审计对象:`perms-system/server`(不含测试代码)
|
|
|
|
|
-审计维度:逻辑一致性、并发/竞态、资源管理、数据完整性、安全漏洞、边界、DB 性能、僵尸代码、接口契约
|
|
|
|
|
-业务量级假设:数千级用户 / 数十级产品 / 单产品 < 100 角色 / 单次 SyncPermissions < 1k 权限 / 单用户 10~30 角色 / 峰值 QPS 数百
|
|
|
|
|
-
|
|
|
|
|
----
|
|
|
|
|
-
|
|
|
|
|
-## R12 闭环复核(均已修复,本轮抽检通过)
|
|
|
|
|
-
|
|
|
|
|
-| R12 条目 | 状态 | 落地证据 |
|
|
|
|
|
-| --- | --- | --- |
|
|
|
|
|
-| M-R12-1 `BindRoles`/`DeleteRole` 孤儿 `sys_user_role` | ✅ 已修 | `internal/model/role/sysRoleModel.go:LockRolesForShareTx` + `internal/logic/user/bindRolesLogic.go:122-129` 事务内 S 锁闭环 |
|
|
|
|
|
-| L-R12-1 `*WithTx` 预提交缓存失效 | ✅ 已修 | `internal/model/user/sysUserModel.go:UpdateProfileWithTx` 绕过 `m.ExecCtx`;`InvalidateProfileCache` 由 Logic 层 post-commit 显式调用(`updateUserLogic.go:205`) |
|
|
|
|
|
-| L-R12-2 `CreateDept` 父部门 Status 复核 | ✅ 已修 | `createDeptLogic.go:67-85` 事务内 `FindOneForShareTx` 取完整行并校验 `Status` |
|
|
|
|
|
-| L-R12-3 `UpdateRole` 注释与代码反向 | ✅ 已修 | `updateRoleLogic.go:34-41 / 64-70` 注释已与"数字越小 = 权限越高"钉死,代码语义一致 |
|
|
|
|
|
-| L-R12-4 / L-R12-5 | ✅ 保持接受契约 | 代码位置无回退;风险评估与 R12 一致 |
|
|
|
|
|
-
|
|
|
|
|
-结论:R12 输出的 P1/P2/P3 全部闭环,未观察到回退。
|
|
|
|
|
|
|
+> 基线:R13 审计后的代码库快照。R13 的 5 条 Low 风险项(L-R13-1 ~ L-R13-5)中,L-R13-1/2/3/4 已在对应 logic 文件中看到修复(见文末"R13 回归验证");L-R13-5 仅做到"大部分调用点切换成 `DetachCacheCleanCtx`",仍遗留两处关键路径未改造,本轮升级归入 M-R14-1。
|
|
|
|
|
|
|
|
---
|
|
---
|
|
|
|
|
|
|
|
## 🚩 核心逻辑漏洞 (High Risk)
|
|
## 🚩 核心逻辑漏洞 (High Risk)
|
|
|
|
|
|
|
|
-本轮未发现新的 High Risk 漏洞。
|
|
|
|
|
-
|
|
|
|
|
-R5~R11 期间暴露过的四条主链路(Auth / Token / Dept-Member / Perm-Sync)在代码中均看到锁序、乐观锁、CAS、fail-close 的层叠护栏,且 R12 的孤儿绑定修复把最后一条"锁序缺口"也补齐。抽样覆盖的高影响面:
|
|
|
|
|
-
|
|
|
|
|
-- **密码变更链**(`changePasswordLogic` → `UpdatePassword` 带 `expectedUpdateTime`):H-R11-1 TOCTOU 闭环,未见回退。
|
|
|
|
|
-- **Token 轮转**(`authHelper.RotateRefreshToken` + `IncrementTokenVersionIfMatch` CAS):HTTP / gRPC 两条路径收敛到同一 helper,未观测到新的并发 rotate 路径。
|
|
|
|
|
-- **部门生命周期**(`DeleteDept` 锁序:self.X → children.S → users.S)vs(`CreateDept`、`UpdateUser(deptId)`):锁方向无 AB-BA。
|
|
|
|
|
-- **权限同步 × 设置用户权限**:`LockByCodeTx` × `SetUserPerms` 事务内 COUNT 双重把关,TOCTOU 闭环。
|
|
|
|
|
-- **授角色 × 删角色**:R12 的 `LockRolesForShareTx` 已闭合。
|
|
|
|
|
-
|
|
|
|
|
----
|
|
|
|
|
-
|
|
|
|
|
-## ⚠️ 健壮性与性能建议 (Medium/Low)
|
|
|
|
|
|
|
+### H-R14-1 · 授权漏洞 / 跨产品权限升级 —— 产品 ADMIN 可借 `UpdateUser` 把他人调入 DEV 部门,间接赋予跨产品全权
|
|
|
|
|
|
|
|
-### L-R13-1(Low · 信息泄露 · 枚举信号)· `AddMember` 将权限校验放在读产品 / 读用户之后
|
|
|
|
|
|
|
+**描述**
|
|
|
|
|
|
|
|
-**描述**:`internal/logic/member/addMemberLogic.go:33-56` 的校验顺序是:
|
|
|
|
|
|
|
+`internal/logic/user/updateUserLogic.go` 在处理 `req.DeptId != nil && *req.DeptId > 0` 分支时,对「新部门是否是 DEV 类型」没有任何特殊校验;且第 137-141 行对调用方的部门前缀校验通过 `caller.MemberType != consts.MemberTypeAdmin` **直接短路**:
|
|
|
|
|
|
|
|
|
|
+```137:141:internal/logic/user/updateUserLogic.go
|
|
|
|
|
+if !caller.IsSuperAdmin &&
|
|
|
|
|
+ caller.MemberType != consts.MemberTypeAdmin &&
|
|
|
|
|
+ !strings.HasPrefix(newDept.Path, caller.DeptPath) {
|
|
|
|
|
+ return response.ErrForbidden("无权将用户调入非自己管辖的部门")
|
|
|
|
|
+}
|
|
|
```
|
|
```
|
|
|
-① FindOneByCode(productCode) → 400/404 "产品不存在/已禁用"
|
|
|
|
|
-② FindOne(userId) → 400/404 "用户不存在/已冻结"
|
|
|
|
|
-③ memberType 字面校验 → 400
|
|
|
|
|
-④ RequireProductAdminFor → 403 "无权限"
|
|
|
|
|
|
|
+
|
|
|
|
|
+配合 `internal/loaders/userDetailsLoader.go` 第 540-554 行的全权分支:
|
|
|
|
|
+
|
|
|
|
|
+```540:554:internal/loaders/userDetailsLoader.go
|
|
|
|
|
+if ud.IsSuperAdmin ||
|
|
|
|
|
+ ud.MemberType == consts.MemberTypeAdmin ||
|
|
|
|
|
+ ud.MemberType == consts.MemberTypeDeveloper ||
|
|
|
|
|
+ (ud.MemberType != "" && ud.DeptType == consts.DeptTypeDev && ud.DeptStatus == consts.StatusEnabled) {
|
|
|
|
|
+ codes, err := l.models.SysPermModel.FindAllCodesByProductCode(ctx, ud.ProductCode)
|
|
|
|
|
+ ...
|
|
|
|
|
+ ud.Perms = codes
|
|
|
|
|
+ return nil
|
|
|
|
|
+}
|
|
|
```
|
|
```
|
|
|
|
|
|
|
|
-任何登录用户(只要持有有效 JWT,不需要是产品 ADMIN)都可以通过响应码差异做两件事:
|
|
|
|
|
|
|
+只要 `sys_user.deptId` 指向 `DeptType=DEV` 的部门,该用户在**任何他已加入的产品**下都自动拿到该产品的全量权限。而 `sys_user.deptId` 是全局字段——`UpdateUser` 在 caller 作用域内用 `CheckManageAccess(productCode=caller.ProductCode, ...)` 判权,却会把修改向**所有其他产品**级联。
|
|
|
|
|
+
|
|
|
|
|
+**攻击链(复现路径)**
|
|
|
|
|
|
|
|
-- 枚举**存在且启用**的产品(对比 "产品不存在" / "产品已禁用" / "继续推进到 user 校验")。
|
|
|
|
|
-- 在已知 `productCode` 前提下,枚举**存在且启用**的 `userId`(对比"用户不存在 / 已冻结 / 通过用户校验" 三态)。
|
|
|
|
|
|
|
+1. 攻击者 A 是产品 P1 的 ADMIN(正常业务赋予的权限),同伙 B 是产品 P1 的 MEMBER;B 同时也是产品 P2 的 MEMBER。
|
|
|
|
|
+2. A 调 `UpdateUser(id=B.Id, deptId=<DEV 部门 id>)`:
|
|
|
|
|
+ - `CheckManageAccess` 仅作用在 P1,通过(B 在 P1 的 memberType=MEMBER,ADMIN 绕过 `checkDeptHierarchy`,`checkPermLevel` 也因 callerPri<targetPri 直接放行);
|
|
|
|
|
+ - 第 137-141 行对 ADMIN 短路,允许把 B 调入 P1 作用域之外的 DEV 部门;
|
|
|
|
|
+ - 第 146-148 行 "仅超管 / ADMIN 可移出部门" 也是按 ADMIN 放行。
|
|
|
|
|
+3. 事务提交后 `UserDetailsLoader.Clean(B.Id)` 刷新 UD,B 在产品 P2 下的下一次 `Load` 命中 DEV 全权分支 → `ud.Perms = FindAllCodesByProductCode(P2)`,B 从 P2 的普通成员瞬间升级为**P2 全权**。
|
|
|
|
|
+4. 如需抹痕,A 再把 B 移出 DEV 即可。该窗口期内 B 可调用 P2 的任何接口(含敏感读写)。
|
|
|
|
|
|
|
|
-步骤 ④ 把"本用户无管理员权限"的 403 放在最后,意味着非 product-ADMIN 的合法登录用户也能消费到 ①②③ 的信号——这比 R10-10 已经封死的 gRPC `GetUserPerms` 枚举面更宽。
|
|
|
|
|
|
|
+**影响**
|
|
|
|
|
|
|
|
-**影响**:
|
|
|
|
|
|
|
+- 跨产品权限升级:P1 的 ADMIN 能为 P2 的任意共有成员授予 P2 的全量权限——这是典型的**信任边界穿透**。
|
|
|
|
|
+- 对于多产品共享用户池(同一 `sys_user` 被多个产品加为成员)的部署,这个路径把"ADMIN 只应在自己产品内全权"的不变量彻底打破。
|
|
|
|
|
+- ADMIN 还可以顺带把其他产品的普通员工(B 可能是 P2 的 MEMBER 但在 P1 只是挂名)调入 DEV 后再移回原部门,对 P2 而言几乎不可审计(`sys_user.deptId` 的变更在 P2 日志里看不到是谁发起的)。
|
|
|
|
|
|
|
|
-- **数据敏感度**:泄露的是"产品 code 集合是否在线" 和 "userId → 是否存在/已冻结"两条事实。前者在中大型组织里属于内部但不敏感,后者在"通过工号推 userId" 的场景下可以被攻击者用来筛选可用账号(配合 H-2 PII 暴露后的 phone / email 泄露链放大)。
|
|
|
|
|
-- **可触发门槛**:任何持有有效 access token 的用户(含仅 MEMBER)。
|
|
|
|
|
-- **概率**:在 JWT 泄露 / 内鬼场景下即时可用,概率非零。
|
|
|
|
|
|
|
+**修复方案**
|
|
|
|
|
|
|
|
-**修复方案**:把 `RequireProductAdminFor(productCode)` 提升到读任何实体之前。由于 `productCode` 来自 `middleware.GetProductCode(ctx)`(权威,不是入参),这一提升零成本:
|
|
|
|
|
|
|
+在 `updateUserLogic.go` 的 `*req.DeptId > 0` 分支里加入对「目标新部门是 DEV」的显式护栏,**仅允许超级管理员**把用户调入 DEV;同理对 `deptId=0`(移出部门)保留现状即可,但把 "移入 DEV" 与 "跨越产品范围" 这条路径堵死:
|
|
|
|
|
|
|
|
```go
|
|
```go
|
|
|
-func (l *AddMemberLogic) AddMember(req *types.AddMemberReq) (*types.IdResp, error) {
|
|
|
|
|
- // 先把"无权调此接口"的路径一刀切在任何 DB 查询之前
|
|
|
|
|
- if err := authHelper.RequireProductAdminFor(l.ctx, req.ProductCode); err != nil {
|
|
|
|
|
- return nil, err
|
|
|
|
|
- }
|
|
|
|
|
- product, err := l.svcCtx.SysProductModel.FindOneByCode(l.ctx, req.ProductCode)
|
|
|
|
|
- // ... 其余保持不变
|
|
|
|
|
|
|
+if newDept.DeptType == consts.DeptTypeDev && !caller.IsSuperAdmin {
|
|
|
|
|
+ // DEV 部门承载"加入即全权"的跨产品语义,任何产品 ADMIN / DEVELOPER / MEMBER
|
|
|
|
|
+ // 发起的调入都可能越过其 productCode 作用域,污染其它产品的 loadPerms 全权分支。
|
|
|
|
|
+ // 与之对称,CreateUser 对非超管 + DEV 目标部门已经被 DeptPath 前缀校验天然约束
|
|
|
|
|
+ // (DEV 部门路径通常不在 ADMIN 的 caller.DeptPath 子树里),这里补齐 UpdateUser
|
|
|
|
|
+ // 被 ADMIN 短路掉的缺口。
|
|
|
|
|
+ return response.ErrForbidden("仅超级管理员可将用户调入研发部门")
|
|
|
}
|
|
}
|
|
|
```
|
|
```
|
|
|
|
|
|
|
|
-同时建议同路径检查两个兄弟接口:
|
|
|
|
|
|
|
+同时建议在 `CreateUser` 中显式镜像一份同样的 `newDept.DeptType == DEV` 护栏,避免未来维护者误将 ADMIN 纳入可跨入 DEV 的行列。
|
|
|
|
|
|
|
|
-- `SetUserPerms`(`setUserPermsLogic.go:38-47`):先 `FindOne(userId)` 再 `RequireProductAdminFor`,同样可枚举 `userId` 存在性。
|
|
|
|
|
-- `BindRoles`(`bindRolesLogic.go:41-49`):先 `FindOne(userId)` 再 `CheckManageAccess`,语义上"管理者才能读 target",但同样走到了未授权调用方的 404 分支。
|
|
|
|
|
|
|
+验收测试(对应 `internal/logic/user/updateUserLogic_test.go`)应补:
|
|
|
|
|
|
|
|
-**结论**:按 `RequireProductAdminFor → 读其它实体` 的顺序统一重排,Medium 以下的"侧信道枚举面"就能大面积收敛。成本极低。
|
|
|
|
|
|
|
+1. caller=ADMIN of P1,target=MEMBER of P1(且是 P2 的 MEMBER),`req.DeptId=DEV 部门 id` → 返回 `ErrForbidden("仅超级管理员可将用户调入研发部门")`;`sys_user.deptId` 不变;`UserDetailsLoader.Clean` 不被触发。
|
|
|
|
|
+2. caller=SuperAdmin 同请求 → 正常放行,提交后 `UserDetailsLoader.Clean` 被调用一次。
|
|
|
|
|
+3. caller=ADMIN of P1,`req.DeptId=<普通部门且在 caller.DeptPath 子树>` → 正常放行(与现有行为一致)。
|
|
|
|
|
|
|
|
---
|
|
---
|
|
|
|
|
|
|
|
-### L-R13-2(Low · TOCTOU · 脏写长尾)· `SetUserPerms` 的 memberType 检查点未覆盖到事务内
|
|
|
|
|
|
|
+## ⚠️ 健壮性与性能建议 (Medium / Low)
|
|
|
|
|
|
|
|
-**描述**:`internal/logic/user/setUserPermsLogic.go:91-97` 对目标用户的 `memberType == ADMIN / DEVELOPER` 做"禁止写 DENY" 的入口拦截,目的是避免写入"永不生效的 DENY 脏行"(L-R10-8)。该检查读的是事务外的 `targetMember.MemberType` 快照。
|
|
|
|
|
|
|
+### M-R14-1 · 资源管理 / 可观测性 —— `RotateRefreshToken` 与 `SyncPermsService` 两处 post-commit 缓存失效未接入 `DetachCacheCleanCtx`
|
|
|
|
|
|
|
|
-时序风险:
|
|
|
|
|
|
|
+**位置**
|
|
|
|
|
|
|
|
-```
|
|
|
|
|
-T0: caller A 读 targetMember.MemberType = "MEMBER" -- 入口检查通过
|
|
|
|
|
-T1: caller B(产品 ADMIN)调 UpdateMember,把 target 升为 "ADMIN"(并提交)
|
|
|
|
|
-T2: caller A 进入事务;DeleteByUserIdForProductTx + BatchInsertWithTx(DENY 行) 均成功
|
|
|
|
|
-T3: caller A 事务末 COUNT(sys_perm ...) 复核通过,事务提交
|
|
|
|
|
-```
|
|
|
|
|
|
|
+- `internal/logic/auth/rotateRefreshToken.go:82`
|
|
|
|
|
+ ```go
|
|
|
|
|
+ svcCtx.UserDetailsLoader.Clean(ctx, claims.UserId)
|
|
|
|
|
+ ```
|
|
|
|
|
+- `internal/logic/pub/syncPermsService.go:171`
|
|
|
|
|
+ ```go
|
|
|
|
|
+ svcCtx.UserDetailsLoader.CleanByProduct(ctx, product.Code)
|
|
|
|
|
+ ```
|
|
|
|
|
|
|
|
-最终:target 现在是 ADMIN(loadPerms 走全权分支),`sys_user_perm` 里留下了永不生效的 DENY 行。这条路径恰好是 L-R10-8 主动防御的语义欺骗("能写、永不生效" 的脏状态污染审计日志 / 权限推理工具)。
|
|
|
|
|
|
|
+R13 提出的 Solution A 是"事务已提交的缓存清理必须与请求 ctx 解耦,用 `loaders.DetachCacheCleanCtx` 拿一个 parent-cancel-不穿透 + 3s 硬超时的 ctx"。审计当下,`changePasswordLogic / logoutLogic / updateDeptLogic / addMember / removeMember / updateMember / updateProduct / bindRolePerms / deleteRole / updateRole / bindRoles / setUserPerms / updateUser / updateUserStatus` 均已落地,**唯独这两处仍用原始 `ctx`**。两条路径都是高敏感后果:
|
|
|
|
|
|
|
|
-**影响**:
|
|
|
|
|
|
|
+- `RotateRefreshToken`:CAS 提交新 `tokenVersion` 已落库,此时 client 断连 / HTTP ctx 被 deadline 触发 → `UserDetailsLoader.Clean` 被立刻 canceled → Redis 里 UD 仍缓存旧 `tokenVersion`。最多 5 分钟 TTL 内:
|
|
|
|
|
+ - 旧 access token(tokenVersion=N)仍能通过中间件(因为 UD 缓存里就是 N);
|
|
|
|
|
+ - 客户端拿到新 refresh 失败时自然会重试 refresh,但若 TTL 没过就会命中 `IncrementTokenVersionIfMatch(expected=N)` 失败(DB 已是 N+1),被强制重登录,形成"无故踢出"的 UX 抖动。
|
|
|
|
|
+ - 更坏:如果攻击者的目标是**让用户的老 access token 继续生效**,TTL 5 分钟的窗口是确定可利用的。
|
|
|
|
|
+- `SyncPermsService`:`CleanByProduct` 失败后,被禁用 / 删除的 perm 仍在 5min TTL 内出现在所有该产品成员的 UD 缓存里——被本产品服务端用 `VerifyToken / GetUserPerms` 查询到的 perms 列表里,`checkStillValid` 逻辑会把失效 perm 从结果里剔除,但 `UserDetails.Perms` 缓存本身不会因 miss 而重建,最多延迟至 TTL。即使下游 caller 做了 `checkStillValid`,也依赖"下游每次查询都保留 db round-trip"的前提;若 perms-system 的消费方只看 `GetUserPerms` 返回,仍会命中窗口。
|
|
|
|
|
|
|
|
-- **运行期安全**:零。ADMIN/DEVELOPER 的 loadPerms 分支完全忽略 `sys_user_perm`,DENY 不会意外丢失敏感权限。
|
|
|
|
|
-- **数据卫生**:污染 `sys_user_perm`;未来如果 target 再被 `UpdateMember` 降回 MEMBER 或调 `RemoveMember → AddMember`(后者不清 user_perm),这些 DENY 会"诈尸"生效,运维排查会踩到"为什么这个用户突然少了一条权限"。
|
|
|
|
|
-- **概率**:极低。需要 caller A 与 caller B 在同一产品同一 target 上毫秒级交错。
|
|
|
|
|
|
|
+**修复方案**
|
|
|
|
|
|
|
|
-**修复方案**:把 memberType 检查纳入事务、与 `sys_product_member` 行 `FOR SHARE` 闭环:
|
|
|
|
|
|
|
+两处统一改造为 detach ctx:
|
|
|
|
|
|
|
|
```go
|
|
```go
|
|
|
-if err := l.svcCtx.SysUserPermModel.TransactCtx(l.ctx, func(ctx, session) error {
|
|
|
|
|
- // 事务内复读 member 状态 + 锁
|
|
|
|
|
- member, err := l.svcCtx.SysProductMemberModel.FindOneForShareTx(ctx, session, targetMember.Id)
|
|
|
|
|
- if err != nil {
|
|
|
|
|
- return response.ErrConflict("成员状态已变更,请刷新后重试")
|
|
|
|
|
- }
|
|
|
|
|
- if (member.MemberType == ADMIN || member.MemberType == DEVELOPER) && hasDeny {
|
|
|
|
|
- return response.ErrBadRequest("目标用户是管理员或开发者,DENY 不会生效")
|
|
|
|
|
- }
|
|
|
|
|
- // ... 原有 DeleteByUserIdForProductTx + BatchInsertWithTx + COUNT 复核
|
|
|
|
|
-})
|
|
|
|
|
|
|
+cleanCtx, cancel := loaders.DetachCacheCleanCtx(ctx)
|
|
|
|
|
+defer cancel()
|
|
|
|
|
+svcCtx.UserDetailsLoader.Clean(cleanCtx, claims.UserId)
|
|
|
```
|
|
```
|
|
|
|
|
|
|
|
-需新增 `SysProductMemberModel.FindOneForShareTx`(与现有 `FindOneForUpdateTx` 对称)。`UpdateMember` 走的是 `FOR UPDATE` 路径,会与本 `FOR SHARE` 形成阻塞链。
|
|
|
|
|
-
|
|
|
|
|
-**优先级**:P3。当前代码的 L-R10-8 "入口拦截 + loadPerms 全权分支忽略脏行"已经把运行期风险压到零,本条只是数据卫生层面的补丁。
|
|
|
|
|
-
|
|
|
|
|
----
|
|
|
|
|
-
|
|
|
|
|
-### L-R13-3(Low · 僵尸代码 / 防御冗余)· `UpdateUserLogic` 中的 `caller.DeptPath != ""` 分支已不可达
|
|
|
|
|
-
|
|
|
|
|
-**描述**:`internal/logic/user/updateUserLogic.go:123-128`
|
|
|
|
|
|
|
+和:
|
|
|
|
|
|
|
|
```go
|
|
```go
|
|
|
-if !caller.IsSuperAdmin &&
|
|
|
|
|
- caller.MemberType != consts.MemberTypeAdmin &&
|
|
|
|
|
- caller.DeptPath != "" && // ← 这条在当前调用链下永远为 true
|
|
|
|
|
- !strings.HasPrefix(newDept.Path, caller.DeptPath) {
|
|
|
|
|
- return response.ErrForbidden("无权将用户调入非自己管辖的部门")
|
|
|
|
|
-}
|
|
|
|
|
|
|
+cleanCtx, cancel := loaders.DetachCacheCleanCtx(ctx)
|
|
|
|
|
+defer cancel()
|
|
|
|
|
+svcCtx.UserDetailsLoader.CleanByProduct(cleanCtx, product.Code)
|
|
|
```
|
|
```
|
|
|
|
|
|
|
|
-走到这段代码有三个前置事实:
|
|
|
|
|
|
|
+`rotateRefreshToken.go` 是 helper,被 `internal/logic/pub/refreshTokenLogic.go`、`internal/server/permserver.go` 的 `RefreshToken` 同时调用,改 helper 一处即可覆盖 HTTP / gRPC 两个入口,避免外部调用方再各自 detach 的重复代码。
|
|
|
|
|
|
|
|
-1. 上方 `caller.UserId == req.Id → 拒绝改 deptId`(line 42-45)已经把"caller == target" 分支 cut 掉;
|
|
|
|
|
-2. `CheckManageAccess`(line 57)对 `caller != target` 分支调 `checkDeptHierarchy`,后者在 `caller.DeptId == 0 || caller.DeptPath == ""` 时 fail-close(`access.go:318-324`);
|
|
|
|
|
-3. 走到第 123 行时 `caller` 必然既不是 super、也不是 ADMIN(同一条判定前几句过滤掉)。
|
|
|
|
|
|
|
+回归测试建议:
|
|
|
|
|
|
|
|
-综合三条事实:`caller.DeptPath != ""` 恒成立——这是防御冗余,不是主动保护。
|
|
|
|
|
|
|
+- 模拟 `IncrementTokenVersionIfMatch` 成功后把 `ctx` 显式 cancel,验证 `Clean` 仍能在 detach ctx 下走到 Redis DEL(或在 redis mock 上观察 DEL 触发)。
|
|
|
|
|
+- 模拟 `BatchWriteProductPerms` 事务成功后 cancel ctx,验证 `CleanByProduct` 仍走到 cache invalidation。
|
|
|
|
|
|
|
|
-**影响**:
|
|
|
|
|
|
|
+---
|
|
|
|
|
|
|
|
-- **运行期**:零。
|
|
|
|
|
-- **可读性**:中。新维护者看到这行容易以为"某种分支下 caller.DeptPath 可以为空",进而在 `checkDeptHierarchy` 里放宽约束,把真实的 fail-close 护栏拆掉。R10 ~ R12 对类似误导性注释已经多次浪费工时(L-R12-3)。
|
|
|
|
|
|
|
+### L-R14-1 · 信息泄露 / 枚举信号 —— Role 维度的 `FindOne → RequireProductAdminFor` 顺序泄漏跨产品 roleId 存在性
|
|
|
|
|
|
|
|
-**修复方案**:删除该条件,并在上方添加一行注释说明这段依赖 `CheckManageAccess` 已经 fail-close:
|
|
|
|
|
|
|
+**位置**
|
|
|
|
|
|
|
|
-```go
|
|
|
|
|
-// caller.DeptId == 0 / DeptPath == "" 的 fail-close 已经由 CheckManageAccess
|
|
|
|
|
-// → checkDeptHierarchy 在 access.go:318-324 统一兜底,这里不再重复防御。
|
|
|
|
|
-if !caller.IsSuperAdmin &&
|
|
|
|
|
- caller.MemberType != consts.MemberTypeAdmin &&
|
|
|
|
|
- !strings.HasPrefix(newDept.Path, caller.DeptPath) {
|
|
|
|
|
- return response.ErrForbidden("无权将用户调入非自己管辖的部门")
|
|
|
|
|
-}
|
|
|
|
|
-```
|
|
|
|
|
|
|
+- `internal/logic/role/updateRoleLogic.go`(先 `SysRoleModel.FindOne(req.Id)` → 再 `RequireProductAdminFor(role.ProductCode)`)
|
|
|
|
|
+- `internal/logic/role/deleteRoleLogic.go`(同上)
|
|
|
|
|
+- `internal/logic/role/bindRolePermsLogic.go`(同上)
|
|
|
|
|
|
|
|
-**优先级**:P3。纯可读性/防回退修复。
|
|
|
|
|
|
|
+任何已登录用户(即使只是某产品的普通 MEMBER)都能构造上述三个接口,用顺序 id 扫 `req.RoleId=1..N`:
|
|
|
|
|
|
|
|
----
|
|
|
|
|
|
|
+- 若 `FindOne` 返回 `ErrNotFound` → `响应 404 "角色不存在"`;
|
|
|
|
|
+- 若角色存在且属于其他产品 → `响应 403 "仅超级管理员或该产品的管理员可执行此操作"`。
|
|
|
|
|
+
|
|
|
|
|
+两个响应码不一致,直接把"该 roleId 是否存在以及属于别的产品"漏给了认证用户,和 R13 的 L-R13-1(`addMember / setUserPerms / bindRoles` 的枚举信号)同族。对比已经收敛过的 `roleDetailLogic.go`(M-N3:把"不存在"和"他产品角色"统一回 404),这三个接口属于遗漏。
|
|
|
|
|
|
|
|
-### L-R13-4(Low · 边界 · 数据卫生)· `CreateUser` 未拒绝负数 `deptId`
|
|
|
|
|
|
|
+**修复方案**
|
|
|
|
|
|
|
|
-**描述**:`internal/logic/user/createUserLogic.go:80-98`
|
|
|
|
|
|
|
+在 `FindOne` 之后、`RequireProductAdminFor` 之前,对非超管调用方先做"产品归属比对",把跨产品情况降级成与"不存在"完全一致的 404:
|
|
|
|
|
|
|
|
```go
|
|
```go
|
|
|
-if req.DeptId > 0 {
|
|
|
|
|
- // ... 校验部门存在、启用、hierarchy
|
|
|
|
|
-} else if !caller.IsSuperAdmin {
|
|
|
|
|
- return nil, response.ErrBadRequest("必须指定部门")
|
|
|
|
|
|
|
+role, err := l.svcCtx.SysRoleModel.FindOne(l.ctx, req.Id)
|
|
|
|
|
+if err != nil {
|
|
|
|
|
+ return response.ErrNotFound("角色不存在")
|
|
|
|
|
+}
|
|
|
|
|
+caller := middleware.GetUserDetails(l.ctx)
|
|
|
|
|
+if caller == nil {
|
|
|
|
|
+ return response.ErrUnauthorized("未登录")
|
|
|
|
|
+}
|
|
|
|
|
+// 与 RoleDetailLogic 的 M-N3 收敛口径一致:非超管看到"非本产品角色"一律返回 404,
|
|
|
|
|
+// 杜绝通过 403/404 文案差异枚举跨产品 roleId。
|
|
|
|
|
+if !caller.IsSuperAdmin && role.ProductCode != middleware.GetProductCode(l.ctx) {
|
|
|
|
|
+ return response.ErrNotFound("角色不存在")
|
|
|
|
|
+}
|
|
|
|
|
+if err := authHelper.RequireProductAdminFor(l.ctx, role.ProductCode); err != nil {
|
|
|
|
|
+ return err
|
|
|
}
|
|
}
|
|
|
-// 落到 Insert 时 deptId 是 req.DeptId 原值;超管可直接 req.DeptId = -1
|
|
|
|
|
```
|
|
```
|
|
|
|
|
|
|
|
-超级管理员以 `req.DeptId = -1`(或其他负数)调用时会直接 `Insert` 一条 `sys_user.deptId = -1` 的脏数据。后续:
|
|
|
|
|
|
|
+三个文件统一同一模板,推荐抽成 `authHelper.ResolveOwnRoleOr404(ctx, svcCtx, roleId)` 以避免日后漂移。
|
|
|
|
|
|
|
|
-- `UserDetailsLoader.loadDept` 有 `if ud.DeptId == 0 { return nil }` 短路,但 `DeptId == -1` 会去 `FindOne(-1)` 命中 `ErrNotFound` → 报错 → `loadOk = false` → 返回 503 Degraded。
|
|
|
|
|
-- `FindIdsByDeptId(-1)` 永远返 nil;这条用户在部门相关接口里彻底隐形。
|
|
|
|
|
|
|
+---
|
|
|
|
|
+
|
|
|
|
|
+### L-R14-2 · 信息泄露 —— `BindRoles` 对入参 roleIds 的错误文案区分"不存在 / 跨产品"
|
|
|
|
|
|
|
|
-即"超管的输入合法域"未被钉死,能构造出永远无法被部门树管理到的僵尸账号。
|
|
|
|
|
|
|
+**位置** `internal/logic/user/bindRolesLogic.go`
|
|
|
|
|
|
|
|
-**影响**:
|
|
|
|
|
|
|
+当 `FindByIds(req.RoleIds)` 返回的角色里有跨产品项,返回 `"不能绑定其他产品的角色"`;缺项时返回 `"包含无效的角色ID"`。已通过 `CheckManageAccess` 的调用方即便只是某产品 MEMBER,也可以借此枚举他人产品的 roleId 分布(比 L-R14-1 门槛更低,因为他只需管得了本产品某个下属即可)。
|
|
|
|
|
|
|
|
-- **运行期**:非阻断。被影响的仅是该条记录的用户本人在登录时会踩 503(因 `loadDept` 报错被 degrade)。
|
|
|
|
|
-- **数据完整性**:污染 `sys_user` 的 `deptId` 定义域。
|
|
|
|
|
|
|
+**修复方案**
|
|
|
|
|
|
|
|
-**修复方案**:在入口简单加一条校验:
|
|
|
|
|
|
|
+将"跨产品"与"不存在"折叠成同一个 `ErrBadRequest("包含无效的角色ID")` 文案(日志里保留细分标记供审计分析):
|
|
|
|
|
|
|
|
```go
|
|
```go
|
|
|
-if req.DeptId < 0 {
|
|
|
|
|
- return nil, response.ErrBadRequest("部门ID必须为非负整数")
|
|
|
|
|
|
|
+valid := 0
|
|
|
|
|
+for _, r := range roles {
|
|
|
|
|
+ if r.ProductCode != productCode || r.Status != consts.StatusEnabled {
|
|
|
|
|
+ continue
|
|
|
|
|
+ }
|
|
|
|
|
+ valid++
|
|
|
}
|
|
}
|
|
|
-if req.DeptId > 0 {
|
|
|
|
|
- // ... 原有逻辑
|
|
|
|
|
-} else if !caller.IsSuperAdmin {
|
|
|
|
|
- return nil, response.ErrBadRequest("必须指定部门")
|
|
|
|
|
|
|
+if valid != len(req.RoleIds) {
|
|
|
|
|
+ logx.WithContext(l.ctx).Infow("bind roles: invalid ids",
|
|
|
|
|
+ logx.Field("audit", "bind_roles_invalid_ids"),
|
|
|
|
|
+ logx.Field("requested", req.RoleIds),
|
|
|
|
|
+ logx.Field("productCode", productCode),
|
|
|
|
|
+ )
|
|
|
|
|
+ return response.ErrBadRequest("包含无效的角色ID")
|
|
|
}
|
|
}
|
|
|
```
|
|
```
|
|
|
|
|
|
|
|
-同类检查建议同步到 `UpdateUser`(line 111-138 的 `*req.DeptId > 0` 分支外,没有对 `*req.DeptId < 0` 的显式拒绝——但那条路径会落到"deptId=0 且不是 super/admin 则 403",与 0 等价;超管仍可写入负数)。
|
|
|
|
|
-
|
|
|
|
|
-**优先级**:P3。
|
|
|
|
|
|
|
+不要把产品归属揭露给响应体,只保留到日志里给运营同学排障。
|
|
|
|
|
|
|
|
---
|
|
---
|
|
|
|
|
|
|
|
-### L-R13-5(Low · 资源管理 · 可观测性)· post-commit 缓存失效在 ctx 被取消时静默降级
|
|
|
|
|
|
|
+### L-R14-3 · 可维护性 —— `UpdateUserLogic` 的 ADMIN 分支注释缺少对 DEV 部门语义的声明
|
|
|
|
|
|
|
|
-**描述**:整套代码库的一种常见模式:
|
|
|
|
|
|
|
+即便 H-R14-1 选择保留现有 ADMIN 可调部门的语义(决定不改代码),line 131-141 的注释也仅讨论了"caller.DeptPath 为空"的边界,没有钉出**"此分支允许 ADMIN 把用户调入 DEV 部门 = 跨产品全权赋予"**这一事实。建议至少在注释层面明确写一段:
|
|
|
|
|
|
|
|
-```go
|
|
|
|
|
-if err := transactionBody(...); err != nil { return err }
|
|
|
|
|
-l.svcCtx.UserDetailsLoader.Clean(l.ctx, userId) // 或 BatchDel / CleanByUserIds
|
|
|
|
|
-l.svcCtx.SysUserModel.InvalidateProfileCache(...) // sqlc 低层缓存
|
|
|
|
|
|
|
+```text
|
|
|
|
|
+注意:ADMIN 分支短路 DeptPath 前缀校验,意味着 ADMIN 可以把目标调入任何部门,
|
|
|
|
|
+包括 DeptType=DEV 的部门。DEV 部门在 loadPerms 中等价于"加入任一产品即全权",
|
|
|
|
|
+这条路径相当于把 ADMIN 本地产品外的权限一并下发到目标的其他产品身份上;若产品
|
|
|
|
|
+间信任不对称,请额外增加 `newDept.DeptType == DEV → RequireSuperAdmin` 的护栏
|
|
|
|
|
+(见审计 H-R14-1)。
|
|
|
```
|
|
```
|
|
|
|
|
|
|
|
-当 `l.ctx`(继承自 HTTP 请求 ctx)在 DB 事务提交之后、Clean 执行之前被 client 断连 / server 超时取消时:
|
|
|
|
|
-
|
|
|
|
|
-- `Clean` 内部的 Redis `Smembers + Del + Del` 三步 / `BatchDel` 的 `Del + Pipelined SRem`——只要第一步遇到 `ctx canceled` 就会短路到 logx `Errorf` 并返回,后续步骤不执行。
|
|
|
|
|
-- `InvalidateProfileCache` 对失败是完全静默(`_ = m.DelCacheCtx(ctx, ...)`)。
|
|
|
|
|
-
|
|
|
|
|
-最终:**DB 已改、缓存未清**。用户在 5 分钟 TTL 窗口内会继续看到旧的 `UserDetails`(旧 DeptPath / 旧 MinPermsLevel / 旧冻结状态),直到下一次该用户被 Load 命中 TTL 失效或被别的 Clean 触发。
|
|
|
|
|
-
|
|
|
|
|
-与 `UserDetailsLoader` 自身文档里承诺的"Clean 的失败是 best-effort,TTL 兜底" 是一致的——但这些失败目前**没有任何可观测的 metric**,只进 Errorf 日志。
|
|
|
|
|
-
|
|
|
|
|
-**影响**:
|
|
|
|
|
-
|
|
|
|
|
-- **安全敏感变更**(冻结用户、切换部门、降权):TTL 5 分钟内旧 UD 继续生效。
|
|
|
|
|
-- **用户体验**:改完资料立即刷新看到旧头像 / 昵称,属于次要。
|
|
|
|
|
-- **监控侧**:Errorf 在正常业务里也会零星出现(Redis 抖动),没有区分"ctx 取消" vs "Redis 真的挂了"的 tag,运维难以建告警。
|
|
|
|
|
-
|
|
|
|
|
-**修复方案**:
|
|
|
|
|
-
|
|
|
|
|
-- **方案 A(推荐)**:post-commit 阶段用 `context.WithoutCancel(l.ctx)`(Go 1.21+)或手动 detach 出一个独立的 `context.Background()` + timeout(比如 3s)把缓存失效做完。事务已经提交,这几次 Redis 写是后置补偿,不该随请求取消而丢失。
|
|
|
|
|
-
|
|
|
|
|
- ```go
|
|
|
|
|
- if err := transactionBody(...); err != nil { return err }
|
|
|
|
|
-
|
|
|
|
|
- // post-commit 的缓存失效不应随 HTTP 请求一起被取消
|
|
|
|
|
- cleanCtx, cancel := context.WithTimeout(context.WithoutCancel(l.ctx), 3*time.Second)
|
|
|
|
|
- defer cancel()
|
|
|
|
|
- l.svcCtx.UserDetailsLoader.Clean(cleanCtx, userId)
|
|
|
|
|
- l.svcCtx.SysUserModel.InvalidateProfileCache(cleanCtx, userId, username)
|
|
|
|
|
- return nil
|
|
|
|
|
- ```
|
|
|
|
|
-
|
|
|
|
|
-- **方案 B**:保持现状,但在 `UserDetailsLoader.Clean` / `BatchDel` / `CleanByUserIds` 内部识别 `errors.Is(err, context.Canceled)` 并打一条带 `cache_invalidation_skipped_due_to_ctx_cancel` tag 的 WARN 日志,方便运维基于这个 tag 建看板报警;真正的"Redis 挂了"走原来的 Errorf。
|
|
|
|
|
-
|
|
|
|
|
-两种方案可以叠加。方案 A 治本(让敏感变更的缓存失效脱离请求生命周期),方案 B 治可观测性。
|
|
|
|
|
-
|
|
|
|
|
-**优先级**:P2(方案 B,日志分级)+ P3(方案 A,detach ctx)。方案 A 的行为改动面较大,建议先做方案 B 收集样本。
|
|
|
|
|
|
|
+这样后续维护者能在修改前看到明确的风险披露,避免把 H-R14-1 的推论再次拆掉。
|
|
|
|
|
|
|
|
---
|
|
---
|
|
|
|
|
|
|
|
-## 复核中仍成立的契约(本轮不动)
|
|
|
|
|
-
|
|
|
|
|
-以下条目是历史审计(R5~R12)确认的**业务侧已接受**选择,本轮没有新观测让它们的风险/收益比改变:
|
|
|
|
|
-
|
|
|
|
|
-- **H-1(同产品成员可见彼此 PII)**:业务需求明示保留,不视为漏洞。
|
|
|
|
|
-- **M-4(CreateProduct 的 one-time ticket 机制)**:`internal/logic/product/fetchInitialCredentialsLogic.go` 的 `GetDelCtx` 原子消费 + 超管鉴权仍在位。
|
|
|
|
|
-- **H-2 / M-3(decision-time fresh DB 读)**:`loadFreshMinPermsLevel` 继续覆盖 `GuardRoleLevelAssignable` 与 `checkPermLevel` 两条决策链,TOCTOU 闭环。
|
|
|
|
|
-- **L-R10-8(全权成员的 DENY 入口拦截 + loadPerms 全权分支忽略脏行)**:入口拦截 + loadPerms JOIN status 双层兜底;L-R13-2 仅在数据卫生层进一步收敛。
|
|
|
|
|
-- **L-R10-9(代理 X-Forwarded-For 链一致性)**:依赖部署侧反代 / WAF 硬约束,代码侧 `firstValidIP` + `net.ParseIP` 已尽全力。
|
|
|
|
|
-- **L-R12-4 / L-R12-5**:已接受契约。
|
|
|
|
|
|
|
+## R13 回归验证(附录)
|
|
|
|
|
|
|
|
----
|
|
|
|
|
-
|
|
|
|
|
-## 本轮 Findings 汇总与修复优先级
|
|
|
|
|
-
|
|
|
|
|
-| 优先级 | 条目 | 类型 | 一句话总结 |
|
|
|
|
|
|
|
+| 条目 | 期望修复 | 代码现状 | 判定 |
|
|
|
| --- | --- | --- | --- |
|
|
| --- | --- | --- | --- |
|
|
|
-| P2 | **L-R13-1** | 信息泄露 | `AddMember` / `SetUserPerms` / `BindRoles` 未把 `RequireProductAdminFor` 提到读实体之前,已认证用户可枚举产品 / 用户存在性 |
|
|
|
|
|
-| P2 | L-R13-5 方案 B | 可观测性 | post-commit 缓存失效的 ctx-canceled 失败需要独立 tag,便于告警 |
|
|
|
|
|
-| P3 | L-R13-2 | 数据卫生 | `SetUserPerms` 的 memberType 检查点应纳入事务 + `FOR SHARE` 闭环 |
|
|
|
|
|
-| P3 | L-R13-3 | 僵尸代码 | `UpdateUserLogic` 中冗余的 `caller.DeptPath != ""` 分支删掉 |
|
|
|
|
|
-| P3 | L-R13-4 | 边界 | `CreateUser` 拒绝 `req.DeptId < 0` |
|
|
|
|
|
-| P3 | L-R13-5 方案 A | 资源管理 | post-commit 阶段用 detached ctx 做缓存失效 |
|
|
|
|
|
|
|
+| L-R13-1 枚举信号 | `addMember / setUserPerms / bindRoles` 在 `RequireProductAdminFor` / 早期 caller 校验前不泄漏对象存在性 | `addMemberLogic.go:41` 把 `RequireProductAdminFor` 置于 User / Product `FindOne` 之前;`setUserPermsLogic.go:45` 同;`bindRolesLogic.go:34-49` 新增 `caller.MemberType == ""` 快速 403 | ✅ 已收敛(roleId 维度新出现 L-R14-1/2) |
|
|
|
|
|
+| L-R13-2 `SetUserPerms` TOCTOU | ADMIN/DEVELOPER → DENY 的拦截必须在事务里并锁 `sys_product_member` | `setUserPermsLogic.go` 事务内 `FindOneForShareTx(targetUserId, productCode)` + `memberType` 再检,`sysProductMemberModel.go` 新增 `FindOneForShareTx` | ✅ 已闭合 |
|
|
|
|
|
+| L-R13-3 冗余条件 | 删除 `UpdateUserLogic` 里 `caller.DeptPath != ""` | `updateUserLogic.go:131-136` 已删除并附注释 | ✅ 已收敛 |
|
|
|
|
|
+| L-R13-4 负数 `deptId` | `CreateUser / UpdateUser` 显式 `< 0` 返回 400 | `createUserLogic.go`、`updateUserLogic.go:117-119` 均有 `*req.DeptId < 0 → BadRequest` | ✅ 已收敛 |
|
|
|
|
|
+| L-R13-5 post-commit 缓存失效 | 方案 A `DetachCacheCleanCtx` + 方案 B 审计日志标签 | 主流程已切换(见文件列表);`loaders/cacheCleanCtx.go` 提供 API 与 `logCacheInvalidationErr`;**遗漏 `rotateRefreshToken.go` / `syncPermsService.go` 两处**,本轮按 M-R14-1 升级跟进 | 🟡 部分收敛 |
|
|
|
|
|
|
|
|
---
|
|
---
|
|
|
|
|
|
|
|
-## 复核结论
|
|
|
|
|
-
|
|
|
|
|
-经过 12 轮迭代 + 本轮(R13)独立复核,`perms-system/server` 的核心授权、会话、数据持久化链路进入**尾部收敛**阶段:
|
|
|
|
|
-
|
|
|
|
|
-- **High Risk 本轮为 0**:连续第二轮无新 High;主链路护栏稳定,未观察到历史修复回退。
|
|
|
|
|
-- **Medium 本轮为 0(本体)**:L-R13-1(枚举面)与 L-R13-5(可观测性)可归为"次 Medium"处理,但其影响面都被既有限流 / TTL 兜住,没有形成直接越权路径。
|
|
|
|
|
-- **Low 本轮 5 条**:全部是**数据卫生**、**僵尸代码**、**防御冗余**三类维护性建议,没有会改变系统安全模型的条目。
|
|
|
|
|
-
|
|
|
|
|
-整体观感:R12 的锁链修复(M-R12-1)+ L-R12-1 的缓存失效时机整改后,这一轮只能在"已认证用户的枚举信号"和"超管输入合法域"这类更小的面上找到可改进点——说明核心逻辑已高度收敛。建议把 L-R13-1(性价比最高,纯 reorder 改动)和 L-R13-5 方案 B(可观测性,零行为变动)合并进最近一次 release;其它 P3 条目随代码重构顺手做即可。
|
|
|
|
|
-
|
|
|
|
|
-若后续要把审计频次从"每轮全量"转成"增量 diff + 主链路定向",建议固化以下 invariant 作为 CI 静态扫描规则:
|
|
|
|
|
-
|
|
|
|
|
-1. 所有 `TransactCtx` 的 post-commit cache clean 必须用 detached ctx(L-R13-5)。
|
|
|
|
|
-2. 所有权限 / 管理类 HTTP / gRPC handler 的第一条业务语句必须是 `Require*` / `CheckManageAccess` 之一(L-R13-1)。
|
|
|
|
|
-3. `expectedUpdateTime` 参数必须从"调用方业务读"的快照传入,禁止在 model 层自读自比(H-R11-1 的长期闭环)。
|
|
|
|
|
-
|
|
|
|
|
-这三条都可以用 `ast` / `go/analysis` 级别的规则扫出来,成本远低于人工重走全链。
|
|
|
|
|
|
|
+本轮新增发现一条 High(H-R14-1)、一条 Medium(M-R14-1)、三条 Low(L-R14-1 / L-R14-2 / L-R14-3)。建议优先处置 H-R14-1(跨产品升级,实际可被产品 ADMIN 触发)和 M-R14-1(两处遗留的缓存失效路径,修复代价极小)。
|