|
|
@@ -1,183 +1,355 @@
|
|
|
-# 权限管理系统 —— 深度代码审计报告(第 9 轮)
|
|
|
-
|
|
|
-> **审计范围**:`/internal` 下全部非测试、非 `_gen.go` 生产代码(含 `internal/server/permserver.go`、HTTP logic / handler / middleware、loaders、model 定制层、svc、util、consts)。
|
|
|
-> **审计时间**:2026-04-19
|
|
|
-> **审计维度**:逻辑一致性 / 并发与 RMW / 资源管理 / 数据完整性 / 安全漏洞 / 边界坍塌 / DB 性能 / 僵尸代码 / 接口契约与对象完整性。
|
|
|
-> **与第 8 轮对比**:
|
|
|
-> - **已落地(本轮复核通过,不再复列修复细节)**:
|
|
|
-> - **H-2**(第 8 轮)`checkPermLevel` 读缓存 `MinPermsLevel` 的 TOCTOU —— `internal/logic/auth/access.go:302-347` 现统一调用 `loadFreshMinPermsLevel` 走 DB 复核,`sqlx.ErrNotFound` 降级为"0 roles"、其他 DB 错误向上冒出。
|
|
|
-> - **M-3**(第 7~8 轮)RefreshToken CAS 在签发失败后脏升级 `tokenVersion` —— `internal/logic/pub/refreshTokenLogic.go:90-110` 与 `internal/server/permserver.go:215-230` 均已把 `Generate*Token` 前置到 `IncrementTokenVersionIfMatch` 之前,CAS 只在两份 token 都成功生成后执行。
|
|
|
-> - **M-N1**(第 8 轮)CreateProduct Redis 票据写失败孤儿化 —— `internal/logic/product/createProductLogic.go:178-283` 已落地 `compensateCreatedRows`:ticket Setex 任一步失败都会按 admin / member / product 倒序硬删,并附 WARN 日志便于对账。
|
|
|
-> - **M-N3**(第 8 轮)gRPC `SyncPermsError{Code:404}` 被吞成 `codes.Internal` —— `internal/server/permserver.go:61-90` 已加 `errors.As` 显式映射到 `codes.NotFound`,并保留具体错误文案。
|
|
|
-> - **L-2**(第 7~8 轮)`CountActiveAdminsTx` 僵尸方法 —— `internal/model/productmember/sysProductMemberModel.go` 中仅保留 `CountOtherActiveAdminsTx`,接口与实现两端均已删除旧签名。
|
|
|
-> - **L-N1**(第 8 轮)`sysPermModel` 仍拼接 `fmt.Sprintf("... status = %d", ...)` —— 现统一用 `?` 占位(`FindAllCodesByProductCode:57-67`、`DisableNotInCodesWithTx:100-154`)。
|
|
|
-> - **L-N2**(第 8 轮)`SetUserPerms` 的 `FindByIds` → `BatchInsert` TOCTOU —— `internal/logic/user/setUserPermsLogic.go:148-165` 已在事务内对目标权限集做 `COUNT(*) WHERE status=?` 复核,并要求计数等于入参长度,否则 rollback。
|
|
|
-> - **产品契约接受、不列入风险清单**:
|
|
|
-> - **H-1 同产品成员 PII 互见**(第 6 → 7 → 8 轮累计列入 P0)—— 产品确认"内部系统 + 通讯录互见"为业务契约,不修。该条从本轮起归档,不再进入后续审计复核列表。
|
|
|
-> - **未落地 / 回归**:
|
|
|
-> - **L-3 历史 `DeptId=0` 账号的 `CheckManageAccess` 兜底**(需数据迁移,不属纯代码问题,延续存档)。
|
|
|
-> - **本轮新发现(权重较高的 5 条)**:
|
|
|
-> - **M-N1(新)`userDetailsLoader.Load` 的 `loadOk=false` 契约错位** —— 基础设施故障被同化为业务 403 / "产品已禁用",前端侧不重试且 SOC 无故障信号。
|
|
|
-> - **M-N2(新)`BatchDel` 中 `unregisterCacheKey` 走串行 N×2 次 SREM** —— 绑/解绑/改角色链路退化为 O(N) Redis RTT,属 N+1。
|
|
|
-> - **M-N3(新)`RoleDetail` 枚举 oracle** —— 跨产品访问 403 vs 404 可区分,可用于穷举合法 roleId。
|
|
|
-> - **M-N4(新)`CreateUser` 未做 caller→`req.DeptId` 的部门链校验** —— 产品 ADMIN 可为**非自己管辖的部门**预埋用户名并占坑,绕过 AddMember 侧部门链防护。
|
|
|
-> - **L-N1(新)`ParseWithHMAC` helper 未统一使用** —— `jwtauthMiddleware.go`、gRPC `VerifyToken` 仍用内联 `token.Method.(*jwt.SigningMethodHMAC)` 断言,三处拷贝导致审计面错位。
|
|
|
-> - **本轮复现的低风险残点**:L-N2 `UpdateUser` 调部门未校验 `dept.Status`;L-N3 `AdminLogin` `IsSuperAdmin` 判断在 bcrypt 之后;L-N4 `sysUserModel.UpdateStatus` 缺乐观锁字段(由上层短路保护)。
|
|
|
+# 第 11 轮深度审计报告
|
|
|
+
|
|
|
+审计对象:`perms-system/server`(不含测试代码)
|
|
|
+审计维度:逻辑一致性、并发/竞态、资源管理、数据完整性、安全漏洞、边界、DB 性能、僵尸代码、接口契约
|
|
|
+说明:本轮基于真实业务量级(数千用户 / 数十产品 / 单产品 <100 role / 一次 SyncPerms < 1k perm / 单 user 10~30 role)做判定。对前 10 轮已闭环条目(H-1~H-4、M-1~M-R10-5、L-1~L-R10-10 等)不重复列举,仅追踪**本轮新发现或重新归类**的风险点。
|
|
|
|
|
|
---
|
|
|
|
|
|
## 🚩 核心逻辑漏洞 (High Risk)
|
|
|
|
|
|
-**本轮无 High Risk 项。**
|
|
|
-- 第 8 轮列示的 H-2(`checkPermLevel` TOCTOU)已通过 `loadFreshMinPermsLevel` 闭环,见上方"已落地"小节。
|
|
|
-- 第 6~8 轮列示的 H-1(同产品 PII 互见)经产品确认为系统对内使用场景下的既定契约,自本轮起归档、不再纳入审计清单。
|
|
|
-- 其余安全类待办均已降级至 Medium / Low,见下节。
|
|
|
+### H-R11-1(High · 数据完整性/竞态) · `UpdatePassword` 内部 `FindOne` 把"外层校验过的状态"洗掉,乐观锁自我对齐 → TOCTOU
|
|
|
+
|
|
|
+**描述**:`internal/model/user/sysUserModel.go:128-152` 的 `UpdatePassword` 不接受外部 `expectedUpdateTime`,而是在内部自己 `FindOne` 再取 `data.UpdateTime` 作为乐观锁 WHERE:
|
|
|
+
|
|
|
+```128:152:internal/model/user/sysUserModel.go
|
|
|
+func (m *customSysUserModel) UpdatePassword(ctx context.Context, id int64, password string, mustChangePassword int64) error {
|
|
|
+ data, err := m.FindOne(ctx, id)
|
|
|
+ if err != nil {
|
|
|
+ return err
|
|
|
+ }
|
|
|
+
|
|
|
+ sysUserIdKey := fmt.Sprintf("%s%v", cacheSysUserIdPrefix, id)
|
|
|
+ sysUserUsernameKey := fmt.Sprintf("%s%v", cacheSysUserUsernamePrefix, data.Username)
|
|
|
+ // 乐观锁:WHERE 叠加 updateTime 与 FindOne 拿到的一致。避免 FindOne → Exec 之间并发改密把
|
|
|
+ // 本次写盖成"最后一写赢"、或目标行被删除后仍返回成功造成语义欺骗(见审计 M-2)。
|
|
|
+ expectedUpdateTime := data.UpdateTime
|
|
|
+ res, err := m.ExecCtx(ctx, func(ctx context.Context, conn sqlx.SqlConn) (sql.Result, error) {
|
|
|
+ query := fmt.Sprintf("UPDATE %s SET `password` = ?, `mustChangePassword` = ?, `tokenVersion` = `tokenVersion` + 1, `updateTime` = ? WHERE `id` = ? AND `updateTime` = ?", m.table)
|
|
|
+ return conn.ExecCtx(ctx, query, password, mustChangePassword, time.Now().Unix(), id, expectedUpdateTime)
|
|
|
+ }, sysUserIdKey, sysUserUsernameKey)
|
|
|
+```
|
|
|
+
|
|
|
+调用方 `ChangePasswordLogic`(`internal/logic/auth/changePasswordLogic.go:50-81`)早已经自己 `FindOne` 读到 `user`、校验 `user.Password` + `user.Status != Enabled`,然后把 `userId` 透传进来。此处 `UpdatePassword` 又打一次 `FindOne`,内层 CAS 对齐的是**内层 FindOne 的时间戳**——而不是**外层校验旧密码所依赖的那个时间戳**。
|
|
|
+
|
|
|
+真实并发场景(两个并存会话):
|
|
|
+
|
|
|
+```text
|
|
|
+T0: Device A 发起改密 (oldPass=P0, newPass=P1)
|
|
|
+ ChangePasswordLogic.FindOne → user{password=H(P0), updateTime=T0}
|
|
|
+ bcrypt.CompareHashAndPassword(H(P0), P0) → OK
|
|
|
+ bcrypt.GenerateFromPassword(P1) → H(P1)
|
|
|
+T1: Device B 独立完成改密到 P2
|
|
|
+ UpdatePassword: FindOne → user{updateTime=T0} → UPDATE ... WHERE updateTime=T0
|
|
|
+ 提交成功:password=H(P2), updateTime=T1, tokenVersion+1
|
|
|
+T2: Device A 的 UpdatePassword 开始执行
|
|
|
+ 内部 FindOne → user{updateTime=T1, password=H(P2)} ← 被 B 的写"刷新"
|
|
|
+ expectedUpdateTime=T1
|
|
|
+ UPDATE ... WHERE updateTime=T1 → 匹配,提交成功
|
|
|
+ 最终 DB:password=H(P1), Device B 的新密码 P2 被覆盖
|
|
|
+```
|
|
|
+
|
|
|
+等价结论:内层"自 FindOne-自 Update"的 CAS 等于没有 CAS。调用方看似"带乐观锁",实际语义已退化为 **last-write-wins on password**,而且连"外层校验的旧密码还是有效旧密码"都不再成立(A 验证的是 P0,应用出去的是把 P2 改回到 P1)。
|
|
|
+
|
|
|
+这条 TOCTOU 并非纸面理论:
|
|
|
+
|
|
|
+- 一个用户因安全事件在 Device B 上紧急改密为 P2(本意:立刻让 Device A 的旧会话失效+密码改掉);
|
|
|
+- 但 Device B 提交的瞬间,Device A 正好在点"修改密码 P0 → P1"。A 的 middleware 已经通过 token 鉴权并取到 userId,logic 执行没有依赖 Device B 的 tokenVersion 递增结果;
|
|
|
+- 最终 P2 被 P1 覆盖,Device B 用户将以为密码是 P2,尝试登录失败;而 Device A 并没有"知道 P2"——也就是说,**一个原本没有权限修改最新密码的会话,成功把密码改掉了**。
|
|
|
+
|
|
|
+此外:`UpdatePassword` 里 `tokenVersion = tokenVersion + 1` 是累计的,所以两次成功的 UPDATE 会连续 +2,把刚刚因 B 改密正准备下线的 A 的旧 token(version 已经对不上)再把 B 的所有新会话也踢掉,用户本次密码修改后的新登录也会被强制登出。
|
|
|
+
|
|
|
+**影响**:
|
|
|
+
|
|
|
+- **数据完整性**:密码这一核心凭证可被"被凭证泄露 / 旧会话持有"的攻击者用自己知道的旧密码,把管理员紧急修改过的密码盖回去——会话劫持的时间窗虽小但影响直接。
|
|
|
+- **安全合规**:信息安全审计侧若执行"强制改密"流程,这条 TOCTOU 是一条可以让强制改密语义悄悄失效的旁路。
|
|
|
+- **可观测**:审计链路上会出现"用户短时间内 password 连续变化 + tokenVersion 连加两次"的奇怪模式,排障成本高。
|
|
|
+
|
|
|
+**修复方案**:把 `expectedUpdateTime` 改由调用方显式传入,彻底消除内部二次 FindOne 造成的"自对齐":
|
|
|
+
|
|
|
+```go
|
|
|
+// sysUserModel.go
|
|
|
+UpdatePassword(ctx context.Context, id int64, username, password string,
|
|
|
+ mustChangePassword, expectedUpdateTime int64) error
|
|
|
+
|
|
|
+func (m *customSysUserModel) UpdatePassword(ctx context.Context, id int64,
|
|
|
+ username, password string, mustChangePassword, expectedUpdateTime int64) error {
|
|
|
+ sysUserIdKey := fmt.Sprintf("%s%v", cacheSysUserIdPrefix, id)
|
|
|
+ sysUserUsernameKey := fmt.Sprintf("%s%v", cacheSysUserUsernamePrefix, username)
|
|
|
+ res, err := m.ExecCtx(ctx, func(ctx context.Context, conn sqlx.SqlConn) (sql.Result, error) {
|
|
|
+ query := fmt.Sprintf(
|
|
|
+ "UPDATE %s SET `password` = ?, `mustChangePassword` = ?, `tokenVersion` = `tokenVersion` + 1, `updateTime` = ? WHERE `id` = ? AND `updateTime` = ?",
|
|
|
+ m.table)
|
|
|
+ return conn.ExecCtx(ctx, query, password, mustChangePassword, time.Now().Unix(), id, expectedUpdateTime)
|
|
|
+ }, sysUserIdKey, sysUserUsernameKey)
|
|
|
+ ...
|
|
|
+}
|
|
|
+```
|
|
|
+
|
|
|
+调用方 `ChangePasswordLogic` 把已经持有的 `user.UpdateTime` / `user.Username` 透传(与 `IncrementTokenVersionIfMatch(id, username, expected)` 的签名风格对齐)。这样 CAS 的 expected 就是"外层用来校验旧密码的那一份快照";只要 DB 里 updateTime 发生过任何变化(并发改密 / 改资料 / 冻结/解冻),都会 CAS 失败返回 `ErrUpdateConflict`,调用方按 409 映射返回"密码已被其他会话修改,请刷新后重试",并提示用户重新登录确认。
|
|
|
+
|
|
|
+同时收益:
|
|
|
+
|
|
|
+- 外层的 `FindOne` 不再浪费(以前只用于校验旧密码,然后内层再打一次);
|
|
|
+- `AdminResetPassword` 之类未来想复用本方法的路径,也必须显式承诺"基于某个观察到的 updateTime 改",语义更清晰。
|
|
|
|
|
|
---
|
|
|
|
|
|
-## ⚠️ 健壮性与性能建议 (Medium / Low)
|
|
|
-
|
|
|
-### M-N1. `userDetailsLoader.loadFromDB` 的 `loadOk=false` 语义错位,导致基础设施故障被同化为业务拒绝(新发现)
|
|
|
-- **位置**:`internal/loaders/userDetailsLoader.go:138-204`、`:321-574`(`loadFromDB`、`loadDept`、`loadProduct`、`loadPerms` 等子加载)。
|
|
|
-- **描述**:
|
|
|
- - `loadFromDB` 约定:
|
|
|
- - `(ud, true, nil)` — 全量成功,写 5 分钟缓存;
|
|
|
- - `(ud, false, nil)` — 子段(dept / product / membership / roles / perms)中某段失败,**返回残缺 ud、仅跳过缓存写**;
|
|
|
- - `(nil, _, err)` — 主体加载失败,`Load` 向上传 error。
|
|
|
- - `Load(:180-183)` 在 `loadOk=false` 时 `return ud, nil`。调用方拿到的是 `Username≠""`、但 `DeptPath=""` / `Perms=nil` / `ProductStatus=0` 的"半成品":
|
|
|
- - `jwtauthMiddleware.go` 只校 `tokenVersion` 与 `IsSuperAdmin || MemberType!=""`,**放行**;
|
|
|
- - `refreshTokenLogic.go` 的 `ProductStatus != StatusEnabled` 分支把这种 case 归类为"产品已被禁用",返 403;
|
|
|
- - 其余业务接口因 `Perms=nil` 命中 `hasPerm=false`,返 403。
|
|
|
- - 结果:一次 Redis / MySQL 抖动对外就是 403 "无权 / 产品禁用",而真正应该是 503 "上游暂不可用,请重试"。**前端不会重试**,SOC 也不会出现任何异常信号,故障被静默。
|
|
|
-- **影响**:
|
|
|
- - 故障可观测性塌陷;SLO 把"基础设施降级"错报成"正常业务拒绝"。
|
|
|
- - 用户体验上,一次瞬时 DB 抖动会对全体在线用户抛"产品已被禁用" / "无权限"——比直接 500 更容易引起误解和 P1 工单。
|
|
|
-- **修复方案**:
|
|
|
- 1. 把 `loadOk=false` 的语义改为"基础设施故障":`Load` 里遇到该分支直接 `return nil, ErrLoaderDegraded`(新定义 sentinel)。
|
|
|
- 2. HTTP 中间件 & gRPC AuthInterceptor 对 `ErrLoaderDegraded` 统一返 503(或 `codes.Unavailable`),并附 `retry-after`。
|
|
|
- 3. 保留 `(ud, false, nil)` 作为内部观测用途时,**改为 panic-safe 的诊断日志聚合**而非对外返回。
|
|
|
- 4. 配套:`refreshTokenLogic` 里的 "产品已被禁用" 分支前增加 `if ud.ProductCode != "" && ud.DeptStatus == 0 { return ErrLoaderDegraded }` 兜底。
|
|
|
- 5. 单测:`TC-9101 loadPerms 报错 → Load 返回 Unavailable`、`TC-9102 loadProduct 报错 → Load 不返回"产品已禁用"`。
|
|
|
-
|
|
|
-### M-N2. `UserDetailsLoader.BatchDel` 在大 roleId 场景退化为 O(N) Redis RTT(N+1 新发现)
|
|
|
-- **位置**:`internal/loaders/userDetailsLoader.go:257-272`,调用方 `internal/logic/role/updateRoleLogic.go`、`internal/logic/role/bindRolePermsLogic.go`、`internal/logic/user/bindRolesLogic.go`。
|
|
|
-- **描述**:
|
|
|
- - `BatchDel` 先用一次 `DelCtx(keys...)` 批量删主 key(OK),但紧跟着 **for-range 调用 `unregisterCacheKey`,每个用户串行 2 次 `SremCtx`**(userIndex + productIndex)。
|
|
|
- - 对"角色改名 / 角色禁用 / 批量重绑角色"场景,该角色下绑定的用户数可能上千人(即使"几万部门"不现实,但"单个业务角色跨几千人"在大型中台里常见)。意味着一次普通的 `UpdateRole` 会对 Redis 发出 **2N 次** 串行往返。
|
|
|
- - 相较之下,`registerCacheKey` 已经用 `PipelinedCtx` 合并 RTT,表明作者清楚 pipeline 的必要性,`unregisterCacheKey` 却没同步改造。
|
|
|
-- **影响**:
|
|
|
- - 在角色批量维护/权限大盘扫描期对 Redis 连接池形成突刺,尾延迟 P99 明显抬升;极端场景可导致 `UpdateRole` 接口被 go-zero 的 ctx timeout 打断,落入"已 UPDATE DB 但 Clean 缓存失败"的分支(目前该分支仅写 Errorf,不回滚)。
|
|
|
- - 当 Redis 集群跨机房时,2N 串行比一次 pipeline 多出 (N-1)×RTT,个位数 ms 的 RTT 在 N=1000 时就是秒级延迟。
|
|
|
-- **修复方案**:
|
|
|
- 1. 把 `unregisterCacheKey` 的 per-user 逻辑合入一次 `PipelinedCtx`:对每个 key 发 `pipe.SRem(userIdxKey, cacheKey)` / `pipe.SRem(productIdxKey, cacheKey)`,一次 Exec。
|
|
|
- 2. 更彻底:直接给 `BatchDel` 写一个专用 `batchUnregister(ctx, pairs)`,单 pipeline 内合并所有 userIndex / productIndex 的 SREM + 可选 EXPIRE。
|
|
|
- 3. 回归测试:`TC-9201 BatchDel(1000 users) 的 Redis 命令数 ≤ 3 次 pipeline Exec`(用 redis-mock 的 CallCount 断言)。
|
|
|
-
|
|
|
-### M-N3. `RoleDetail` 枚举 oracle:跨产品访问 404 vs 403 可区分(新发现)
|
|
|
-- **位置**:`internal/logic/role/roleDetailLogic.go:29-58`。
|
|
|
-- **描述**:
|
|
|
- - 现状流程:
|
|
|
- 1. 先 `SysRoleModel.FindOne(req.Id)`:找不到返 **404 "角色不存在"**;
|
|
|
- 2. 再判 `!IsSuperAdmin && caller.ProductCode != role.ProductCode`:返 **403 "无权访问该产品的数据"**。
|
|
|
- - 非超管攻击者遍历 `req.Id`,即可通过响应码精确区分"该 id 不存在" vs "存在于别的产品",从而绘制跨产品的 role id 分布图,为后续定向攻击(社工、横向越权尝试)提供素材。
|
|
|
- - 同一脚本在 `ProductDetailLogic.productDetail` 也存在(先 `FindOneByCode`,再做成员检查);`RoleList`/`ProductList` 因天然按 `caller.ProductCode` 过滤不泄漏。
|
|
|
-- **影响**:
|
|
|
- - 边界信息泄露,属于"先查后授权"反模式;在任何以 id 为目标的枚举场景都会被利用。
|
|
|
- - 与第 7 轮把 `AdminLogin` 非法用户名的 bcrypt 时序补齐的精神不一致。
|
|
|
-- **修复方案**:
|
|
|
- 1. 合并成"授权失败即返 404"的统一契约:先用 `caller.ProductCode` 做过滤(或查询时把 `productCode = ?` 塞进 WHERE),未命中或跨产品一律返 `ErrNotFound("角色不存在")`。
|
|
|
- 2. `ProductDetailLogic` 同步处理:对非超管直接用 `FindByCodeWithMemberCheck` 或先查 `sys_product_member` 命中后再读 `sys_product`,省掉"存在性差异"泄漏。
|
|
|
- 3. 单测:`TC-9301 非超管请求别产品 roleId → 404`、`TC-9302 非超管请求不存在 roleId → 404`,两者响应体必须完全一致。
|
|
|
-
|
|
|
-### M-N4. `CreateUser` 允许产品 ADMIN 为"非自己管辖部门"预埋用户名(新发现)
|
|
|
-- **位置**:`internal/logic/user/createUserLogic.go:37-100`。
|
|
|
-- **描述**:
|
|
|
- - 现状只用 `RequireProductAdminFor(productCode)` 校验 caller 是该产品的 ADMIN,并用 `FindOne(req.DeptId)` 核对部门存在,**未校验 caller 的 `DeptPath` 是否覆盖 `newDept.Path`**。
|
|
|
- - 产品 ADMIN 可以为任意存在的部门创建用户,随后:
|
|
|
- 1. 占坑关键用户名(`admin_*`、`ops_*`、`sre_*` 等易被运营/运维复用的账号);
|
|
|
- 2. 预埋账号后等待配合方(比如跨部门协作的另一位 ADMIN)触发 `AddMember`,由于 `AddMember` 会走 `CheckAddMemberAccess` 的部门链校验,**对方 ADMIN** 的部门链可能覆盖,最终把这个"伪造种子账号"挂进产品。
|
|
|
- - 与 `UpdateUserLogic` 的设计(调部门时严格校验 `caller.DeptPath` 前缀,见 `updateUserLogic.go:116-120`)**不一致**:同样的敏感位,创建时无校验、修改时严校验,出现"先创建后改部门"的绕路。
|
|
|
-- **影响**:
|
|
|
- - 横向越权路径:产品 ADMIN 在"应当只管 X 部门"的治理约束下,可以在 DEV 部门、总部部门等任意位置生成账号,后续借由 AddMember 协同路径落地。
|
|
|
- - 用户名命名空间被占用,正常部门 ADMIN 新建时收到"用户名已存在",排查困难。
|
|
|
-- **修复方案**:
|
|
|
- 1. 与 `UpdateUserLogic` 保持对称:`if req.DeptId > 0 && !caller.IsSuperAdmin && caller.MemberType != Admin || caller.DeptPath != "" && !strings.HasPrefix(newDept.Path, caller.DeptPath) { return ErrForbidden }`。
|
|
|
- 2. 若业务上允许产品 ADMIN "跨部门拉新人",则至少要求 `req.DeptId` 必须是 caller 所在产品已有成员的部门集合之一(查 `sys_product_member` join `sys_user` 得到部门白名单)。
|
|
|
- 3. 单测:`TC-9401 部门 ADMIN 创建跨部门用户 → 403`、`TC-9402 产品 ADMIN 创建本部门子部门用户 → 200`、`TC-9403 SuperAdmin 创建任意部门用户 → 200`。
|
|
|
-
|
|
|
-### L-N1. JWT 解析三处重复:`ParseWithHMAC` helper 未统一使用
|
|
|
-- **位置**:
|
|
|
- - 定义:`internal/logic/auth/jwt.go:16-31`;
|
|
|
- - 内联实现 1:`internal/middleware/jwtauthMiddleware.go:62-66`;
|
|
|
- - 内联实现 2:`internal/server/permserver.go` 的 `VerifyToken`(`jwt.ParseWithClaims(... keyfunc {...})`)。
|
|
|
-- **描述**:`ParseWithHMAC` 的注释明确写着"所有 JWT 解析点(HTTP 中间件 / gRPC VerifyToken / RefreshToken)统一走这里",但目前只有 `ParseRefreshToken` 一个调用方。另两处自己又写了一遍 `token.Method.(*jwt.SigningMethodHMAC)` 断言 —— 功能上一致,但:
|
|
|
- - 未来若增加算法白名单(例如仅允许 HS384、禁 HS256)或添加 `typ` 断言,要改 3 处;
|
|
|
- - 把"算法混淆防御"的审计覆盖矩阵从 1 个函数摊到 3 个函数,`test-design.md` 的 TC-0951~0960 只覆盖了 `ParseRefreshToken` 一条路径。
|
|
|
-- **影响**:安全属性正确,但代码一致性差、改动风险高。属于"长期隐患"。
|
|
|
-- **修复方案**:
|
|
|
- 1. `jwtauthMiddleware.go` 改用 `authHelper.ParseWithHMAC(tokenStr, secret, &UserClaims{})`,移除内联 keyfunc;
|
|
|
- 2. gRPC `VerifyToken` 同样替换;
|
|
|
- 3. 把 TC-0951~0960 复用到这两条路径(直接 table-driven 调三个入口)。
|
|
|
-
|
|
|
-### L-N2. `UpdateUserLogic` 调部门时未校验目标 `dept.Status`
|
|
|
-- **位置**:`internal/logic/user/updateUserLogic.go:110-131`。
|
|
|
-- **描述**:`FindOne(*req.DeptId)` 仅判存在,不判 `Status`。产品 ADMIN 可把用户调入已 `Disabled` 的部门,随后该用户在 `loadPerms` 里会命中"普通成员 + DeptStatus!=Enabled"分支,其 DEV 全权特权被撤销。若业务意图是"停用部门=冻结该部门所有活动",该路径破坏了不变量。
|
|
|
-- **影响**:中低优。主要是产品语义一致性("停用部门"的含义被稀释),不构成越权。
|
|
|
-- **修复方案**:`if newDept.Status != StatusEnabled { return ErrBadRequest("目标部门已停用") }`,与 `UpdateDept` 禁用时的数据流闭环。
|
|
|
-
|
|
|
-### L-N3. `AdminLogin` `IsSuperAdmin` 判断在 bcrypt 之后,对"合法用户名"的时序侧漏略大于"非法用户名"分支
|
|
|
-- **位置**:`internal/logic/pub/adminLoginLogic.go:55-85`。
|
|
|
-- **描述**:流程是"查用户 → bcrypt 校验 → 校 `IsSuperAdmin` → 校 status"。对于一个**存在但非超管**的账号,攻击者即便密码随机也会触发 bcrypt 计算(约 60~100ms),与"存在且密码错"分支耗时相近;而"不存在"分支走 `dummyBcryptHash` 也有类似耗时兜底 —— 单凭这点难以区分。但若攻击者能获取大量样本,`IsSuperAdmin` 这一步的耗时(sql 比较)理论上可让"存在但非超管"比"存在是超管且密码错"略快(无 token 签发路径),仍可能形成<10ms 级的统计差。
|
|
|
-- **影响**:属学术级别时序泄漏,实战价值低。但该入口按第 7 轮审计精神是"高敏感且对抗公网扫描",建议把 `IsSuperAdmin` 判断**前置到 `FindByUsername` 之后、bcrypt 之前**:若非超管,仍走 `dummyBcryptHash` 消耗一次 bcrypt,再返回 403,恒定时序。
|
|
|
-- **修复方案**:重排为:`FindByUsername → 非超管则用 dummy 计算再统一返 ErrInvalidCredentials → 超管再真 bcrypt → Status 校验`。
|
|
|
-
|
|
|
-### L-N4. `sysUserModel.UpdateStatus` 缺乐观锁字段(由上层短路保护,但 model 自身语义不自洽)
|
|
|
-- **位置**:`internal/model/user/sysUserModel.go:154-175`。
|
|
|
-- **描述**:`UpdateStatus` 的 `WHERE id=?` 无 `updateTime` / `tokenVersion` 比较;与同文件里 `UpdateProfile` 使用乐观锁、`IncrementTokenVersionIfMatch` 使用 CAS 的风格不一致。上层 `UpdateUserStatusLogic` 做了"状态相同则短路"、`UserDetailsLoader` 5 分钟 TTL 也提供事实一致性,所以实际故障概率低。
|
|
|
-- **影响**:不会造成越权,但并发改状态会出现"我改了 Enabled 你覆成 Disabled"的 last-write-wins。
|
|
|
-- **修复方案**:把 `UpdateStatus` 改为 `WHERE id=? AND updateTime=?`,接口加入 `expectedUpdateTime` 参数,语义与 `UpdateProfile` 对齐。
|
|
|
+## ⚠️ 健壮性与性能建议 (Medium/Low)
|
|
|
+
|
|
|
+### M-R11-1(Medium · 安全/限流) · gRPC `SyncPermissions` / `GetUserPerms` 未挂 gRPC 入口限流
|
|
|
+
|
|
|
+**描述**:HTTP 侧 `/api/perm/sync` 已挂 `serverCtx.SyncRateLimit`(`internal/handler/routes.go:195-206`),而 gRPC 的 `PermServer.SyncPermissions`(`internal/server/permserver.go:60-92`)与 `PermServer.GetUserPerms`(`:331-369`)既不做 IP 维度限流、也不做按 `appKey` 维度的限流:
|
|
|
+
|
|
|
+```60:66:internal/server/permserver.go
|
|
|
+func (s *PermServer) SyncPermissions(ctx context.Context, req *pb.SyncPermissionsReq) (*pb.SyncPermissionsResp, error) {
|
|
|
+ items := make([]pub.SyncPermItem, len(req.Perms))
|
|
|
+ for i, p := range req.Perms {
|
|
|
+ items[i] = pub.SyncPermItem{Code: p.Code, Name: p.Name, Remark: p.Remark}
|
|
|
+ }
|
|
|
+
|
|
|
+ result, err := pub.ExecuteSyncPerms(ctx, s.svcCtx, req.AppKey, req.AppSecret, items)
|
|
|
+```
|
|
|
+
|
|
|
+`Login / RefreshToken / VerifyToken` 都各自持有 `GrpcLoginLimiter / GrpcRefreshLimiter / GrpcVerifyLimiter`,口径完整;唯独"产品服务端调用"这两个接口是裸调。
|
|
|
+
|
|
|
+**影响**:
|
|
|
+
|
|
|
+- `SyncPermissions` 内部要走"tx + LockByCodeTx 的 X 锁";`appSecret` 一旦泄露,恶意方在没有限流兜底时可持续对同一 product 打高频同步请求,`LockByCodeTx` 会串行化但前置 `bcrypt.Compare(appSecret)` 的 CPU 开销(cost=10 默认 ~100ms)与 DB 短事务都会被放大,单点产品同步的尾延迟会被显著拉高。
|
|
|
+- `GetUserPerms` 会触发 `UserDetailsLoader.Load`,缓存未命中时回源多张表;同样的泄露凭证 + 枚举 `userId` 可打爆 DB。
|
|
|
+
|
|
|
+本条未能被"HTTP 层 SyncRateLimit"兜住,因为 gRPC 是独立监听端口(不同服务进程入口)。
|
|
|
+
|
|
|
+**修复**:
|
|
|
+
|
|
|
+- 在 `servicecontext.go` 增设 `GrpcSyncLimiter` / `GrpcGetUserPermsLimiter`(配额取决于真实产品数和 QPS,例如单 product 每分钟 60 次同步 / 1k 次 perm 查询),按 `fmt.Sprintf("grpc:sync:%s", req.AppKey)` / `fmt.Sprintf("grpc:perms:%s", req.AppKey)` 为 key,避免按 IP(产品后端多实例共享 egress IP 时会误伤);
|
|
|
+- `GetUserPerms` 可以同时叠加按 IP 维度,防止同一合法产品多个后端实例在 DDoS 场景下集体被耗尽配额。
|
|
|
|
|
|
---
|
|
|
|
|
|
-## ✅ 本轮复核通过、认定安全的机制(仅挑敏感点列示)
|
|
|
-
|
|
|
-| 机制 | 位置 | 关键保护点 |
|
|
|
-| --- | --- | --- |
|
|
|
-| `checkPermLevel` fresh read | `logic/auth/access.go:302-347`、`loadFreshMinPermsLevel:339-343` | caller.MinPermsLevel 每次走 DB,降级 admin 后续的跨级分配被立即拒绝(第 8 轮 H-2 已闭环) |
|
|
|
-| CreateProduct 补偿 | `logic/product/createProductLogic.go:178-283` | Redis 票据写入 `SetexCtx` 失败调用 `compensateCreatedRows` 级联删除 admin / member / product |
|
|
|
-| RefreshToken CAS 顺序 | `logic/pub/refreshTokenLogic.go:90-110`、`server/permserver.go:215-230` | 先生成 access+refresh 再 `IncrementTokenVersionIfMatch`,签名失败不会吞掉旧 refresh |
|
|
|
-| DeleteDept AB-BA 防护 | `logic/dept/deleteDeptLogic.go` | 自身 `FOR UPDATE`,子部门 / 成员 `FOR SHARE`,锁顺序与 CreateDept 对齐 |
|
|
|
-| 负缓存投毒防御 | `loaders/userDetailsLoader.go:161-178` | 写哨兵前再 `FindOne` 强一致复核,避免 Insert→Load 并发把新用户哨兵掉 |
|
|
|
-| AddMember 部门链二次校验 | `logic/auth/access.go:CheckAddMemberAccess`、`logic/member/addMemberLogic.go:76-78` | 产品 ADMIN 也要过部门链,切断第 6 轮 H-3 |
|
|
|
-| RemoveMember 末位守卫 | `logic/member/removeMemberLogic.go:49-53` | `CountOtherActiveAdminsTx` 事务内排除自己并 lock row,杜绝并发撤 admin 导致 0 admin |
|
|
|
-| GuardRoleLevelAssignable | `logic/user/bindRolesLogic.go:86-88` | caller 的 min-level fresh read,避免缓存 admin 权级绑定高于自己的角色 |
|
|
|
-| SetUserPerms 事务内复核 | `logic/user/setUserPermsLogic.go:148-165` | `BatchInsertWithTx` 前后 `COUNT(*) WHERE status=?` 复核,并发禁用权限的 TOCTOU 闭环 |
|
|
|
-| gRPC SyncPerms 错误分级 | `server/permserver.go:61-90` | `SyncPermsError{Code:404}` → `codes.NotFound`,不再同化为 Internal |
|
|
|
-| gRPC 限流剥端口 | `server/permserver.go` / `ratelimit` | PeriodLimit key 用客户端 IP 而非 `host:port`,防单连接多端口绕限 |
|
|
|
-| JWT HMAC 断言本身 | `logic/auth/jwt.go:16-31` | `alg=none` / RS256 / ES256 全拒;L-N1 指出的是"调用点未统一",算法防御自身 OK |
|
|
|
+### M-R11-2(Medium · DB 性能) · `UpdateStatus` / `IncrementTokenVersion` 只为构造 cache key 而多打一次 `FindOne`
|
|
|
+
|
|
|
+**描述**:与 H-R11-1 同属"内部 FindOne 冗余"族,但这两处只影响性能而不破坏正确性:
|
|
|
+
|
|
|
+```161-182:internal/model/user/sysUserModel.go
|
|
|
+func (m *customSysUserModel) UpdateStatus(ctx context.Context, id int64, status int64, expectedUpdateTime int64) error {
|
|
|
+ data, err := m.FindOne(ctx, id)
|
|
|
+ if err != nil {
|
|
|
+ return err
|
|
|
+ }
|
|
|
+
|
|
|
+ sysUserIdKey := fmt.Sprintf("%s%v", cacheSysUserIdPrefix, id)
|
|
|
+ sysUserUsernameKey := fmt.Sprintf("%s%v", cacheSysUserUsernamePrefix, data.Username)
|
|
|
+ ...
|
|
|
+```
|
|
|
+
|
|
|
+```190-221:internal/model/user/sysUserModel.go
|
|
|
+func (m *customSysUserModel) IncrementTokenVersion(ctx context.Context, id int64) (int64, error) {
|
|
|
+ data, err := m.FindOne(ctx, id)
|
|
|
+ if err != nil {
|
|
|
+ return 0, err
|
|
|
+ }
|
|
|
+
|
|
|
+ sysUserIdKey := fmt.Sprintf("%s%v", cacheSysUserIdPrefix, id)
|
|
|
+ sysUserUsernameKey := fmt.Sprintf("%s%v", cacheSysUserUsernamePrefix, data.Username)
|
|
|
+```
|
|
|
+
|
|
|
+两处 `FindOne` 的唯一作用都是取 `username` 构造 `cacheSysUserUsernamePrefix` 键;真正的并发安全依赖外层 `expectedUpdateTime` 或 `IncrementTokenVersion` 自己的 `RowsAffected==0 → ErrUpdateConflict` 兜底,`data` 对象其他字段并没有参与逻辑。
|
|
|
+
|
|
|
+- `UpdateStatus` 的调用方 `UpdateUserStatusLogic` 事前已经从 `ValidateStatusChange` 拿到 `sysUser`——`sysUser.Username` 就在手里;
|
|
|
+- `IncrementTokenVersion` 的唯一调用方 `LogoutLogic` 从 middleware 拿到 `userId`,没有 username,但只要上游 `ud, _ := UserDetailsLoader.Load(...)` 已经拉过用户详情,一样可以透传。
|
|
|
+
|
|
|
+**影响**:
|
|
|
+
|
|
|
+- 每次 Logout / 冻结解冻各多一次 cache/DB round-trip;Logout 通常叠加 `TokenOpLimiter`,调用量不大;
|
|
|
+- 但"冗余 FindOne"会占一个连接池槽位,在登录/登出高峰(比如统一挂维护后全员重新登录)会放大尾延迟。
|
|
|
+
|
|
|
+**修复**:把 username 显式提到函数签名,与 `IncrementTokenVersionIfMatch(id, username, expected)` 口径对齐:
|
|
|
+
|
|
|
+```go
|
|
|
+UpdateStatus(ctx, id, username, status, expectedUpdateTime int64) error
|
|
|
+IncrementTokenVersion(ctx, id, username int64) (int64, error)
|
|
|
+```
|
|
|
+
|
|
|
+调用方:
|
|
|
+
|
|
|
+- `UpdateUserStatusLogic` 传 `user.Username`;
|
|
|
+- `LogoutLogic` 在限流已经通过的前提下,先 `ud := UserDetailsLoader.Load(...)`(Logout 本就会走到 Load),把 `ud.Username` 传进来。
|
|
|
+
|
|
|
+顺带把"内部 FindOne → ErrUpdateConflict 能正确触发"这条隐性依赖显式化,未来有人重构把内部 FindOne 挪走也不会把 CAS 语义改坏。
|
|
|
|
|
|
---
|
|
|
|
|
|
-## 🎯 修复建议优先级与落地顺序
|
|
|
+### M-R11-3(Medium · 数据完整性/竞态) · `DeleteDept` 与 `UpdateUser.deptId` 之间的 write skew
|
|
|
+
|
|
|
+**描述**:`internal/logic/dept/deleteDeptLogic.go:36-69` 的策略是:
|
|
|
+
|
|
|
+```
|
|
|
+DeleteDept tx:
|
|
|
+ ① SELECT sys_dept WHERE id=? FOR UPDATE -- X 锁目标部门
|
|
|
+ ② SELECT sys_dept WHERE parentId=? FOR SHARE -- S 锁确认无子部门
|
|
|
+ ③ SELECT sys_user WHERE deptId=? FOR SHARE -- S 锁确认无关联用户
|
|
|
+ ④ DELETE sys_dept WHERE id=?
|
|
|
+```
|
|
|
+
|
|
|
+`UpdateUser` 在修改 `deptId` 为目标部门时,只对 `sys_dept` 做不加锁的 `FindOne` 验证"目标部门存在且 Enabled",然后在自己的事务里对 `sys_user` 行取 X 锁写新 `deptId`(`internal/logic/user/updateUserLogic.go:110-137`、`internal/model/user/sysUserModel.go:105-126`)。
|
|
|
+
|
|
|
+两个事务交错:
|
|
|
+
|
|
|
+```
|
|
|
+T1: DeleteDept
|
|
|
+ ①②③ 都通过:sys_user.deptId=X 为空
|
|
|
+ (尚未 commit)
|
|
|
+T2: UpdateUser.deptId=X
|
|
|
+ 读 sys_dept[X] 的 RR 快照:Status=Enabled(T1 尚未 commit)
|
|
|
+ X 锁 sys_user[userY],写 deptId=X
|
|
|
+ T2 提交
|
|
|
+T1: ④ DELETE sys_dept[X],提交
|
|
|
+最终:sys_user[userY].deptId=X, sys_dept[X] 已删除 → 悬挂 deptId
|
|
|
+```
|
|
|
+
|
|
|
+T1 的 `FOR SHARE ON sys_user WHERE deptId=X` 对"将要变成 X 但目前不是 X"的行没有锁效力:InnoDB 的 gap lock 只覆盖 `deptId` 列现有的索引范围,`userY` 当前 `deptId=Y` 不在 T1 的锁范围。反向 T2 对 `sys_dept[X]` 是无锁读,看不到 T1 的 X 锁。这是典型的 `write skew`。
|
|
|
+
|
|
|
+**影响**:
|
|
|
+
|
|
|
+- 只要并发删部门 + 调整用户部门同时发生就会留下"deptId 指向已删除部门"的 orphan 行;
|
|
|
+- 后续 `DeptTree` / `UserList` 渲染时,这些用户找不到 dept path,会落回"default / 空 path"分支——所有非超管/非产品 ADMIN 的管辖判定全部对该用户失效(管他们的人发现人没了,被管的人发现管理员找不到自己);
|
|
|
+- 真实业务概率:极低(单日内 DeleteDept + 把某人加进这个部门同时点提交的窗口只有毫秒级)。但一旦触发修复成本高(只能靠运维手 SQL 清洗)。
|
|
|
+
|
|
|
+**修复方案**:二选一。
|
|
|
+
|
|
|
+1. **对 `sys_user.deptId` 用 FOR UPDATE 并补 gap lock**(推荐):把 `FOR SHARE` 改成 `FOR UPDATE`,并在 `sys_dept.id` 列上(以及 `deptId` 外键索引)依赖 next-key lock。这会把"向这个 deptId 写入的 UpdateUser"阻塞到 DeleteDept 提交或回滚。代价:DeleteDept 持锁时间变长,但 DeleteDept 极其低频。
|
|
|
+2. **在 `UpdateUser` 里对目标 `sys_dept` 加 `FOR SHARE`**:UpdateUser 事务内先 `SELECT sys_dept WHERE id=? FOR SHARE`,然后再做 `sys_user` 的 X 锁写。这样 DeleteDept 的 X 锁会把 UpdateUser 的 S 读阻塞,形成一致锁链。代价:UpdateUser 的一次查询变成锁读,但 dept 查询本来就走缓存,打穿到 DB 的比例很低。
|
|
|
+
|
|
|
+两种方案等价化解 write skew。推荐 2(把锁链约束放在"新行写入方"上更自然,也与 `CreateDept` 在插子部门前对 `parentId FOR SHARE` 的现有策略口径一致)。
|
|
|
+
|
|
|
+---
|
|
|
+
|
|
|
+### L-R11-1(Low · 逻辑一致性/契约) · `UpdateMember` 对 `memberType` 空串直接 400,丧失"只改 status"语义
|
|
|
+
|
|
|
+此条在前轮 M-4 / L-5 系列修复中被反向推导过,R10 未列;本次复核明确为**接口契约问题**。
|
|
|
+
|
|
|
+**描述**:`UpdateMember` 的 `types` 中 `MemberType` / `Status` 都是非指针必传字段。如果 admin 想只改 member 状态(冻结其产品成员资格),必须重传一份完整的 `memberType`;前端直接传空会被拦 400。这一约束与 HTTP API 的"部分更新"直觉不符。
|
|
|
+
|
|
|
+**影响**:前端要么自己维护"原 memberType"在内存(多一个状态源),要么多打一次 member.detail 接口;属于接口易用性问题,不涉及安全。
|
|
|
+
|
|
|
+**修复**:把 `memberType` / `status` 改成 `*string` / `*int64`,为 nil 时表示不改该字段;logic 侧按 nil/非 nil 分支分别处理校验,和 `UpdateUserReq` 的可选字段风格对齐。
|
|
|
+
|
|
|
+---
|
|
|
+
|
|
|
+### L-R11-2(Low · DB 性能) · `DisableNotInCodesWithTx` 与 `DeleteBy...Tx` 族把"整行 SELECT"当作"取缓存 key"的手段
|
|
|
+
|
|
|
+**描述**:`internal/model/perm/sysPermModel.go:100-164`、`internal/model/userrole/sysUserRoleModel.go:105-166`、`internal/model/roleperm/sysRolePermModel.go:86-130`、`internal/model/userperm/sysUserPermModel.go:45-66` 都遵循同一结构:
|
|
|
+
|
|
|
+```
|
|
|
+SELECT <全部列> FROM ... WHERE ... FOR UPDATE
|
|
|
+拼 cache keys
|
|
|
+UPDATE/DELETE ... WHERE <同样条件>
|
|
|
+```
|
|
|
+
|
|
|
+这里"SELECT 整行"的唯一用途是取 `id` 与构成缓存 key 的两三个字段(`Code / ProductCode / UserId / RoleId / PermId`)。真正需要的列最多 3 个,却把 `Name / Remark / Status / CreateTime / UpdateTime` 等字段全部搬运到应用层再丢弃。
|
|
|
+
|
|
|
+**影响**:
|
|
|
+
|
|
|
+- 对单次 SyncPerms(涉及 <1k 条 perm)或单次 BindRoles(<30 role)影响很小;
|
|
|
+- 在 `DeleteByRoleIdTx` 场景(`DeleteRole` 会级联删除所有 `sys_user_role WHERE roleId=?`,后续会对"受影响用户列表"做批量 cache 失效),被"关联成百上千用户"的角色删除时会显著增加 goroutine 临时内存与网络 I/O;
|
|
|
+- 另外 `session.QueryRowsCtx(list)` + `len(list)==0` 提前 return 的"先查后写"模式,每次删除都付出一次"完整行读回"的成本,哪怕是单行删。
|
|
|
+
|
|
|
+**修复**:把所有"只是为构 cache key"的 SELECT 精简到只取必要列;或者只取 `id / (user, role) / (role, perm)` 等 key 组件,省略业务字段。例如:
|
|
|
+
|
|
|
+```go
|
|
|
+// 只取 id + (productCode, code)
|
|
|
+var rows []struct {
|
|
|
+ Id int64 `db:"id"`
|
|
|
+ ProductCode string `db:"productCode"`
|
|
|
+ Code string `db:"code"`
|
|
|
+}
|
|
|
+```
|
|
|
+
|
|
|
+对"单行删除"路径(如 `DeleteByRoleIdAndPermIdsTx` 只有一个 permId)甚至可以直接由调用方传 key,不再反查。
|
|
|
+
|
|
|
+---
|
|
|
+
|
|
|
+### L-R11-3(Low · 边界 / 缓存一致性) · `UpdateProfile` 不支持改 `username`,但签名暴露 `username` 参数,易被后续误用
|
|
|
+
|
|
|
+**描述**:`internal/model/user/sysUserModel.go:105-126`
|
|
|
+
|
|
|
+```go
|
|
|
+func (m *customSysUserModel) UpdateProfile(ctx context.Context, id int64, username string,
|
|
|
+ nickname, email, phone, remark string, deptId, newStatus int64,
|
|
|
+ statusChanged bool, expectedUpdateTime int64) error {
|
|
|
+ sysUserIdKey := fmt.Sprintf("%s%v", cacheSysUserIdPrefix, id)
|
|
|
+ sysUserUsernameKey := fmt.Sprintf("%s%v", cacheSysUserUsernamePrefix, username)
|
|
|
+ ...
|
|
|
+ // SET 语句里没有 `username`=?
|
|
|
+```
|
|
|
+
|
|
|
+入参有 `username`、但 UPDATE 里不写这个列;它只被用作"构造旧缓存 key"。未来若某人认为"签名已经带了 username,那 UpdateProfile 应该也能顺手改 username"并往 SET 里加上 `username=?`,会立刻出现:
|
|
|
+
|
|
|
+- 新 username 还未在缓存键 `cacheSysUserUsernamePrefix` 删除旧值,stale 缓存残留;
|
|
|
+- 且没有处理 `sys_user.username` UNIQUE 约束违反的 1062 回滚。
|
|
|
+
|
|
|
+**影响**:当前代码功能正确,属于"签名 / 语义鸿沟"。若维护同学出于"方便"扩展此函数支持改 username 就会踩坑。
|
|
|
+
|
|
|
+**修复**:
|
|
|
+
|
|
|
+- 文档化:在 `UpdateProfile` 的 Go doc 里显式说明"本方法不负责修改 `username`;`username` 参数仅用于旧缓存键构造";
|
|
|
+- 或更保险:把这个函数拆成"不关心 username"的纯写入 + 调用方自己按 (id, oldUsername) 做缓存失效;
|
|
|
+- 未来若真要支持改 username,做成独立的 `UpdateUsernameTx`,内部处理"新/旧 username 两份缓存键清理 + 1062 捕获"。
|
|
|
+
|
|
|
+---
|
|
|
+
|
|
|
+### L-R11-4(Low · 资源管理) · `UserDetailsLoader.CleanByProduct` 扫描缓存前缀的放大效应
|
|
|
+
|
|
|
+**描述**:`SyncPerms` 成功后调用 `UserDetailsLoader.CleanByProduct(ctx, product.Code)`(`internal/logic/pub/syncPermsService.go:163`)。该方法(见 `loaders/userDetailsLoader.go` 实现)通常基于前缀批量失效该产品名下所有用户的详情缓存。
|
|
|
+
|
|
|
+在"单 product <1k 活跃成员"的真实业务量下没有问题;但 `SyncPerms` 属于高频事件(产品每次部署都会触发,一天数十次),且 `CleanByProduct` 会把该产品**所有**活跃用户的缓存(含各自的 `Perms / Member / Role` 计算结果)一次性清掉,使得紧随其后的大量在线请求都会打穿缓存到 DB。
|
|
|
+
|
|
|
+**影响**:
|
|
|
+
|
|
|
+- 正常 SyncPerms(新增一两条 perm)其实只影响"已经引用到这些新 perm 的角色的用户",却连同未动过的 `loadPerms` 缓存一并清空;
|
|
|
+- 产品发版时很容易出现"发版后 5 分钟 TPS 打到 DB,缓存命中率陡降"的尾部抖动。
|
|
|
+
|
|
|
+**修复**:
|
|
|
+
|
|
|
+- 方案一:只在 `Added/Updated/Disabled` 里对"发生状态变化的 code 集合"构建失效名单(按 role → user 反推),代价是扫描关联表(小量级 OK);
|
|
|
+- 方案二:保留当前实现但在 `CleanByProduct` 里做 jitter(比如按 userId 哈希分批清,而非一次性全清),缓解"雪崩清空"。
|
|
|
+
|
|
|
+---
|
|
|
+
|
|
|
+### L-R11-5(Low · 僵尸代码 / 契约一致性) · `RefreshToken` 两条路径的 `newVersion != predictedVersion` 分支实际不可达(重申 L-R10-4)
|
|
|
+
|
|
|
+R10-4 已记录本点。本轮复核确认 HTTP 与 gRPC 两条 RefreshToken 路径(`internal/logic/pub/refreshTokenLogic.go:117-135`、`internal/server/permserver.go:240-251`)的 forensic 分支仍然保留,没有被收敛到一个共享 helper。
|
|
|
+
|
|
|
+**影响**:零运行期影响;纯代码重复 + 维护负担。
|
|
|
+
|
|
|
+**修复**:把"试签 → CAS → Clean → 对比 newVersion"整段抽成 `authHelper.RotateRefreshToken(ctx, svcCtx, claims, ud) (access, refresh string, err error)`,让 HTTP / gRPC 只负责前置校验与错误映射。这样未来 CAS 语义若要微调(例如把预签下沉到 tx 内),两条路径只改一处。
|
|
|
+
|
|
|
+---
|
|
|
+
|
|
|
+## 本轮复核中仍成立的契约(不再修)
|
|
|
+
|
|
|
+列出以下事项作为已定档契约,审计不再要求整改:
|
|
|
|
|
|
-| 优先级 | 议题 | 预估工作量 | 风险 |
|
|
|
-| --- | --- | --- | --- |
|
|
|
-| P0 | **M-N1 Loader 半成品降级** | 1 天:新增 sentinel 错误 + 中间件映射 + refreshToken 分支修正 + 2 条新单测 | 中:需验证 5xx 监控告警路径 |
|
|
|
-| P1 | **M-N3 RoleDetail 枚举 oracle** | 半天:`RoleDetail` + `ProductDetail` 合并成"404 或 200" | 低 |
|
|
|
-| P1 | **M-N4 CreateUser 部门链校验** | 半天:对称复用 `UpdateUserLogic` 的校验代码 | 低 |
|
|
|
-| P2 | **M-N2 BatchDel pipeline 化** | 半天:改 `unregisterCacheKey` 支持批量 + 回归压测 | 低 |
|
|
|
-| P2 | **L-N1 ParseWithHMAC 统一** | 半天:替换 2 处调用 + table-driven 用例复用 | 低 |
|
|
|
-| P3 | **L-N2 UpdateUser 目标部门 Status** | 1 小时 | 低 |
|
|
|
-| P3 | **L-N3 AdminLogin IsSuperAdmin 前置** | 1 小时 | 低 |
|
|
|
-| P3 | **L-N4 UpdateStatus 乐观锁** | 2 小时(需调上层一处调用) | 低 |
|
|
|
-| Backlog | **L-3 `DeptId=0` 历史账号迁移** | 需 DBA 配合 | — |
|
|
|
+- **H-1 / R10 复核**:`UserDetail` / `MemberList` 同产品成员可见彼此 `email / phone / remark` —— 产品业务需求已确认保留。
|
|
|
+- **M-4 / R10 复核**:`CreateProduct` 响应体只返回一次性 ticket,真实 `appSecret / adminPassword` 通过 `/fetchInitialCredentials`(超管鉴权 + `GetDelCtx` 原子消费)领取。
|
|
|
+- **M-3 / H-2 / R10 复核**:授角色、管辖决策点 100% 走 NoCache DB 读(`loadFreshMinPermsLevel`),caller 的 `MinPermsLevel` 缓存不参与决策;TTL 不影响越权闭环。
|
|
|
+- **L-R10-4**:RefreshToken 的 `newVersion != predictedVersion` 分支保留 forensic 兜底,本轮新建议(L-R11-5)仅涉及"把两处重复抽象成一处",不改变契约。
|
|
|
+- **L-R10-7**:`PermList` / `RoleList` 对同产品成员可见全量定义。属业务默认约定。
|
|
|
+- **L-R10-8**:`loadPerms` 对 SUPER / ADMIN / DEVELOPER 忽略 DENY 的语义已在 `SetUserPerms` 入口拦截;`DeptType` 动态变动导致旧 DENY 失效的长尾遗留。
|
|
|
+- **L-R10-9**:代理层 X-Forwarded-For 链一致性由运维侧在反代/WAF 上硬约束。
|
|
|
|
|
|
---
|
|
|
|
|
|
-## 📎 审计方法论与覆盖率说明
|
|
|
+## 修复优先级
|
|
|
|
|
|
-- 本轮审计共读取并分析 27 个 `.go` 文件(不含 `_test.go` / `_gen.go` / `testdata/`),覆盖 logic(全部)、loaders、server、middleware、model 定制层、consts、util;
|
|
|
-- 复核维度:针对第 8 轮每一条未决项都读取相关 code path 做实际行为确认,避免"报告里写修复实际没改";
|
|
|
-- 新发现筛选原则:仅列出**能构造具体攻击序列或业务损害场景**的项,纯风格性建议已过滤;
|
|
|
-- "几万部门"等非现实场景已按 USER 要求过滤;保留"角色下绑千人"这一现实高频场景作为 M-N2 的论据。
|
|
|
+| 优先级 | 条目 | 理由 |
|
|
|
+| ---- | ---- | ---- |
|
|
|
+| P0 | **H-R11-1** | 涉及密码本身的 last-write-wins;修复即放弃一条会话劫持旁路。下一迭代必修。 |
|
|
|
+| P1 | M-R11-1 | gRPC 入口限流缺口;产品接入方越多风险敞口越大,建议排入下迭代 |
|
|
|
+| P1 | M-R11-3 | write skew 罕见但数据无法自愈,连带修 `UpdateUser` 的锁链建议一次做完 |
|
|
|
+| P2 | M-R11-2 | 性能与观测性改进;顺带提升 `UpdateStatus / IncrementTokenVersion` 的语义清晰度 |
|
|
|
+| P2 | L-R11-1 | 前端易用性;可并到"接口契约梳理"专项 |
|
|
|
+| P3 | L-R11-2 ~ L-R11-5 | 代码质量/性能优化;触及相关文件时顺手处理即可 |
|
|
|
|
|
|
+整体代码质量在 10 轮迭代后已高度收敛,本轮只发现**一条** High(H-R11-1,存在可复现的 TOCTOU)与**三条** Medium;核心授权 / 会话 / 数据持久化三条链路的主干逻辑仍然稳健,历史修复契约未发生回退。
|