# 第 13 轮深度审计报告

审计对象：`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 全部闭环，未观察到回退。

---

## 🚩 核心逻辑漏洞 (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)

### L-R13-1（Low · 信息泄露 · 枚举信号）· `AddMember` 将权限校验放在读产品 / 读用户之后

**描述**：`internal/logic/member/addMemberLogic.go:33-56` 的校验顺序是：

```
① FindOneByCode(productCode)  → 400/404 "产品不存在/已禁用"
② FindOne(userId)              → 400/404 "用户不存在/已冻结"
③ memberType 字面校验          → 400
④ RequireProductAdminFor       → 403 "无权限"
```

任何登录用户（只要持有有效 JWT，不需要是产品 ADMIN）都可以通过响应码差异做两件事：

- 枚举**存在且启用**的产品（对比 "产品不存在" / "产品已禁用" / "继续推进到 user 校验"）。
- 在已知 `productCode` 前提下，枚举**存在且启用**的 `userId`（对比"用户不存在 / 已冻结 / 通过用户校验" 三态）。

步骤 ④ 把"本用户无管理员权限"的 403 放在最后，意味着非 product-ADMIN 的合法登录用户也能消费到 ①②③ 的信号——这比 R10-10 已经封死的 gRPC `GetUserPerms` 枚举面更宽。

**影响**：

- **数据敏感度**：泄露的是"产品 code 集合是否在线" 和 "userId → 是否存在/已冻结"两条事实。前者在中大型组织里属于内部但不敏感，后者在"通过工号推 userId" 的场景下可以被攻击者用来筛选可用账号（配合 H-2 PII 暴露后的 phone / email 泄露链放大）。
- **可触发门槛**：任何持有有效 access token 的用户（含仅 MEMBER）。
- **概率**：在 JWT 泄露 / 内鬼场景下即时可用，概率非零。

**修复方案**：把 `RequireProductAdminFor(productCode)` 提升到读任何实体之前。由于 `productCode` 来自 `middleware.GetProductCode(ctx)`（权威，不是入参），这一提升零成本：

```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)
    // ... 其余保持不变
}
```

同时建议同路径检查两个兄弟接口：

- `SetUserPerms`（`setUserPermsLogic.go:38-47`）：先 `FindOne(userId)` 再 `RequireProductAdminFor`，同样可枚举 `userId` 存在性。
- `BindRoles`（`bindRolesLogic.go:41-49`）：先 `FindOne(userId)` 再 `CheckManageAccess`，语义上"管理者才能读 target"，但同样走到了未授权调用方的 404 分支。

**结论**：按 `RequireProductAdminFor → 读其它实体` 的顺序统一重排，Medium 以下的"侧信道枚举面"就能大面积收敛。成本极低。

---

### L-R13-2（Low · TOCTOU · 脏写长尾）· `SetUserPerms` 的 memberType 检查点未覆盖到事务内

**描述**：`internal/logic/user/setUserPermsLogic.go:91-97` 对目标用户的 `memberType == ADMIN / DEVELOPER` 做"禁止写 DENY" 的入口拦截，目的是避免写入"永不生效的 DENY 脏行"（L-R10-8）。该检查读的是事务外的 `targetMember.MemberType` 快照。

时序风险：

```
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 ...) 复核通过，事务提交
```

最终：target 现在是 ADMIN（loadPerms 走全权分支），`sys_user_perm` 里留下了永不生效的 DENY 行。这条路径恰好是 L-R10-8 主动防御的语义欺骗（"能写、永不生效" 的脏状态污染审计日志 / 权限推理工具）。

**影响**：

- **运行期安全**：零。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` 闭环：

```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 复核
})
```

需新增 `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
if !caller.IsSuperAdmin &&
    caller.MemberType != consts.MemberTypeAdmin &&
    caller.DeptPath != "" &&               // ← 这条在当前调用链下永远为 true
    !strings.HasPrefix(newDept.Path, caller.DeptPath) {
    return response.ErrForbidden("无权将用户调入非自己管辖的部门")
}
```

走到这段代码有三个前置事实：

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 != ""` 恒成立——这是防御冗余，不是主动保护。

**影响**：

- **运行期**：零。
- **可读性**：中。新维护者看到这行容易以为"某种分支下 caller.DeptPath 可以为空"，进而在 `checkDeptHierarchy` 里放宽约束，把真实的 fail-close 护栏拆掉。R10 ~ R12 对类似误导性注释已经多次浪费工时（L-R12-3）。

**修复方案**：删除该条件，并在上方添加一行注释说明这段依赖 `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("无权将用户调入非自己管辖的部门")
}
```

**优先级**：P3。纯可读性/防回退修复。

---

### L-R13-4（Low · 边界 · 数据卫生）· `CreateUser` 未拒绝负数 `deptId`

**描述**：`internal/logic/user/createUserLogic.go:80-98`

```go
if req.DeptId > 0 {
    // ... 校验部门存在、启用、hierarchy
} else if !caller.IsSuperAdmin {
    return nil, response.ErrBadRequest("必须指定部门")
}
// 落到 Insert 时 deptId 是 req.DeptId 原值；超管可直接 req.DeptId = -1
```

超级管理员以 `req.DeptId = -1`（或其他负数）调用时会直接 `Insert` 一条 `sys_user.deptId = -1` 的脏数据。后续：

- `UserDetailsLoader.loadDept` 有 `if ud.DeptId == 0 { return nil }` 短路，但 `DeptId == -1` 会去 `FindOne(-1)` 命中 `ErrNotFound` → 报错 → `loadOk = false` → 返回 503 Degraded。
- `FindIdsByDeptId(-1)` 永远返 nil；这条用户在部门相关接口里彻底隐形。

即"超管的输入合法域"未被钉死，能构造出永远无法被部门树管理到的僵尸账号。

**影响**：

- **运行期**：非阻断。被影响的仅是该条记录的用户本人在登录时会踩 503（因 `loadDept` 报错被 degrade）。
- **数据完整性**：污染 `sys_user` 的 `deptId` 定义域。

**修复方案**：在入口简单加一条校验：

```go
if req.DeptId < 0 {
    return nil, response.ErrBadRequest("部门ID必须为非负整数")
}
if req.DeptId > 0 {
    // ... 原有逻辑
} else if !caller.IsSuperAdmin {
    return nil, response.ErrBadRequest("必须指定部门")
}
```

同类检查建议同步到 `UpdateUser`（line 111-138 的 `*req.DeptId > 0` 分支外，没有对 `*req.DeptId < 0` 的显式拒绝——但那条路径会落到"deptId=0 且不是 super/admin 则 403"，与 0 等价；超管仍可写入负数）。

**优先级**：P3。

---

### L-R13-5（Low · 资源管理 · 可观测性）· post-commit 缓存失效在 ctx 被取消时静默降级

**描述**：整套代码库的一种常见模式：

```go
if err := transactionBody(...); err != nil { return err }
l.svcCtx.UserDetailsLoader.Clean(l.ctx, userId)   // 或 BatchDel / CleanByUserIds
l.svcCtx.SysUserModel.InvalidateProfileCache(...) // sqlc 低层缓存
```

当 `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 收集样本。

---

## 复核中仍成立的契约（本轮不动）

以下条目是历史审计（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**：已接受契约。

---

## 本轮 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 做缓存失效 |

---

## 复核结论

经过 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` 级别的规则扫出来，成本远低于人工重走全链。