|
|
@@ -1,301 +1,280 @@
|
|
|
-# 权限系统深度代码审计报告
|
|
|
+# 权限系统 (perms-system-server) 代码审计报告
|
|
|
|
|
|
-> 审计范围:`internal/logic`、`internal/model`、`internal/middleware`、`internal/loaders`、`internal/server` 全部非测试业务代码
|
|
|
-> 审计时间:2026-04-17
|
|
|
+> 审计范围:`internal/` 下所有非测试 `.go` 文件、`perm.sql`、`perm.api`、`pb/` gRPC 服务
|
|
|
+> 审计日期:2026-04-18
|
|
|
+> 审计维度:逻辑一致性、并发竞态、资源管理、数据完整性、安全漏洞、边界崩溃
|
|
|
|
|
|
---
|
|
|
|
|
|
## 🚩 核心逻辑漏洞 (High Risk)
|
|
|
|
|
|
-### H1. UpdateUser 修改状态时未递增 tokenVersion,导致冻结用户可能仍持有有效令牌
|
|
|
+### H1. gRPC `GetUserPerms` 接口无任何鉴权
|
|
|
|
|
|
-- **位置**:`internal/logic/user/updateUserLogic.go` 第 82-93 行
|
|
|
-- **描述**:`UpdateUser` 接口允许通过 `req.Status` 将用户冻结(`status=2`),但内部调用的是通用 `SysUserModel.Update()`,该方法**不会递增 `tokenVersion`**。而专用接口 `UpdateUserStatus` 正确调用了 `SysUserModel.UpdateStatus()`,其中执行了 `tokenVersion = tokenVersion + 1`。两个接口实现同一个业务操作(冻结用户),安全保障强度却不同。
|
|
|
-- **影响**:
|
|
|
- 1. 通过 `UpdateUser` 冻结用户后,`tokenVersion` 不变,被冻结用户的 RefreshToken 在 Redis 缓存未命中的情况下(如 Redis 故障、缓存过期后重新加载前的竞态窗口)仍可用于换取新 AccessToken。
|
|
|
- 2. 虽然 `UserDetailsLoader.Clean()` 提供了即时保护(清缓存后下次请求会从 DB 加载到 `status=2`),但若 Redis 操作失败(网络抖动等),旧缓存最长可存活 5 分钟(`defaultCacheTTL=300`),期间用户不受冻结影响。
|
|
|
- 3. 与 `UpdateUserStatus` 的行为不一致,容易让维护者产生误解。
|
|
|
-- **修复方案**:在 `UpdateUser` 中,当 `status` 发生变更时,改用 `UpdateStatus` 或手动递增 `tokenVersion`:
|
|
|
+- **位置**:`internal/server/permserver.go:186-197`
|
|
|
+- **描述**:`GetUserPerms` 方法直接调用 `UserDetailsLoader.Load(ctx, req.UserId, req.ProductCode)`,没有任何身份校验逻辑——不要求 JWT、不校验调用方服务身份、不验证 mTLS。任何能连接 gRPC 端口的客户端,只需知道 `userId` 和 `productCode` 即可获取该用户的 `MemberType` 与完整权限列表。
|
|
|
+- **影响**:攻击者可枚举用户 ID 和产品编码,批量拉取所有用户的权限数据,属于严重信息泄露与越权。
|
|
|
+- **修复方案**:
|
|
|
+ - 方案 A:为 gRPC 服务增加 Interceptor,校验调用方携带的内部服务 Token(如 gRPC Metadata 中传递一个共享密钥或签名)。
|
|
|
+ - 方案 B:部署层面使用 mTLS,确保只有可信服务节点能连接 gRPC 端口。
|
|
|
+ - 方案 C:将 `GetUserPerms` 改为要求传入一个有效的 AccessToken,在方法内部验证 token 后,仅返回该 token 对应用户的权限。
|
|
|
|
|
|
```go
|
|
|
-// updateUserLogic.go — 在 status 变更分支中
|
|
|
-if req.Status == consts.StatusEnabled || req.Status == consts.StatusDisabled {
|
|
|
- if user.Status != req.Status {
|
|
|
- // status 发生了实际变更,走 UpdateStatus 以递增 tokenVersion
|
|
|
- if err := l.svcCtx.SysUserModel.UpdateStatus(l.ctx, req.Id, req.Status); err != nil {
|
|
|
- return err
|
|
|
- }
|
|
|
- user.Status = req.Status // 同步内存值,后续 Update 不会覆盖回旧值
|
|
|
+// 建议在 GetUserPerms 中增加调用方鉴权
|
|
|
+func (s *PermServer) GetUserPerms(ctx context.Context, req *pb.GetUserPermsReq) (*pb.GetUserPermsResp, error) {
|
|
|
+ // 校验内部调用凭证
|
|
|
+ if err := s.verifyInternalCaller(ctx); err != nil {
|
|
|
+ return nil, status.Error(codes.Unauthenticated, "未授权的调用方")
|
|
|
}
|
|
|
+ // ... 原有逻辑
|
|
|
}
|
|
|
```
|
|
|
|
|
|
-或者更彻底的方案:移除 `UpdateUser` 中的 `status` 字段支持,强制状态变更只能通过 `updateUserStatus` 接口。
|
|
|
-
|
|
|
---
|
|
|
|
|
|
-### H2. UserDetail 接口返回的 roleIds 未按产品隔离,存在跨产品信息泄露
|
|
|
+### H2. `AddMember` 缺乏产品归属校验,可跨产品添加成员
|
|
|
|
|
|
-- **位置**:`internal/logic/user/userDetailLogic.go` 第 44 行
|
|
|
-- **描述**:`FindRoleIdsByUserId` 查询 `sys_user_role` 时没有关联产品过滤,返回了用户在**所有产品**下的全部角色 ID。非超管的产品 A 成员查看某用户详情时,能看到该用户在产品 B 下的角色 ID 列表。
|
|
|
-- **影响**:虽然角色 ID 本身只是数字,但结合角色列表接口可反推出目标用户在其他产品中的角色配置,构成越权信息泄露。
|
|
|
-- **修复方案**:在查询时加入产品维度的过滤:
|
|
|
+- **位置**:`internal/logic/member/addMemberLogic.go:46`
|
|
|
+- **描述**:`AddMember` 使用 `CheckManageAccess(ctx, svcCtx, req.UserId, req.ProductCode)` 进行鉴权。然而 `CheckManageAccess`(`access.go:47`)对"操作自己"无条件放行(`caller.UserId == targetUserId → return nil`),**不校验** `req.ProductCode` 是否等于 `caller.ProductCode`。这意味着:
|
|
|
+ 1. 用户 A(产品 X 的 ADMIN)可以将**自己**添加为产品 Y 的成员(只要 `req.UserId == caller.UserId`)。
|
|
|
+ 2. `CheckMemberTypeAssignment` 只校验 `caller.MemberType` 的优先级,**不区分产品上下文**——A 产品的 ADMIN 身份被用来判断是否可分配 B 产品的 MEMBER 类型。
|
|
|
+- **影响**:用户可将自己"自举"到不属于自己的产品中,绕过产品隔离边界。
|
|
|
+- **修复方案**:在 `AddMember` 中增加显式的产品归属校验:
|
|
|
|
|
|
```go
|
|
|
-productCode := middleware.GetProductCode(l.ctx)
|
|
|
-var roleIds []int64
|
|
|
-if productCode != "" {
|
|
|
- roleIds, err = l.svcCtx.SysUserRoleModel.FindRoleIdsByUserIdForProduct(l.ctx, user.Id, productCode)
|
|
|
-} else {
|
|
|
- roleIds, err = l.svcCtx.SysUserRoleModel.FindRoleIdsByUserId(l.ctx, user.Id)
|
|
|
+func (l *AddMemberLogic) AddMember(req *types.AddMemberReq) (resp *types.IdResp, err error) {
|
|
|
+ // 新增:要求操作者必须是目标产品的管理员(或超管)
|
|
|
+ if err := authHelper.RequireProductAdminFor(l.ctx, req.ProductCode); err != nil {
|
|
|
+ return nil, err
|
|
|
+ }
|
|
|
+ // ... 其余逻辑保持不变
|
|
|
}
|
|
|
```
|
|
|
|
|
|
-需要在 `SysUserRoleModel` 中新增 `FindRoleIdsByUserIdForProduct` 方法,关联 `sys_role` 表按 `productCode` 过滤。
|
|
|
-
|
|
|
---
|
|
|
|
|
|
-### H3. CreateUser 未校验 DeptId 是否存在,可写入不存在的部门关联
|
|
|
+### H3. `UpdateUserStatus` 缺少"目标用户属于当前产品"的校验
|
|
|
|
|
|
-- **位置**:`internal/logic/user/createUserLogic.go` 第 69-80 行
|
|
|
-- **描述**:`CreateUser` 直接将 `req.DeptId` 写入数据库,未校验该部门是否存在。而 `UpdateUser` 在修改 `DeptId` 时正确地进行了存在性校验。
|
|
|
-- **影响**:创建用户时传入不存在的 `deptId`,用户记录会关联到一个"幽灵部门"。后续 `UserDetailsLoader.loadDept` 会查询失败并静默跳过(`DeptPath` 为空),导致该用户在部门层级校验 `checkDeptHierarchy` 中行为异常——具体表现为 `caller.DeptPath == ""`,触发 `"您的部门信息异常"` 错误,无法管理任何其他用户。
|
|
|
-- **修复方案**:
|
|
|
+- **位置**:`internal/logic/user/updateUserStatusLogic.go:32-60`
|
|
|
+- **描述**:`UpdateUserStatus` 仅调用 `CheckManageAccess` 校验管理权限,**没有**像 `SetUserPerms` / `BindRoles` 那样先验证 `SysProductMemberModel.FindOneByProductCodeUserId`(目标用户必须是当前产品的成员)。而 `CheckManageAccess` 中的 `checkPermLevel` 在目标无成员记录时,`targetMemberType=""` → `memberTypePriority=MaxInt32` → `callerPri < targetPri` → **直接放行**。
|
|
|
+- **影响**:产品 A 的 ADMIN 可以修改**不属于产品 A 的全局用户**的账号状态(启用/冻结),因为 `SysUser.Status` 是全局字段,冻结后影响用户在**所有产品**下的登录。
|
|
|
+- **修复方案**:增加产品成员校验,与 `SetUserPerms` / `BindRoles` 保持一致:
|
|
|
|
|
|
```go
|
|
|
-if req.DeptId > 0 {
|
|
|
- if _, err := l.svcCtx.SysDeptModel.FindOne(l.ctx, req.DeptId); err != nil {
|
|
|
- return nil, response.ErrBadRequest("部门不存在")
|
|
|
+func (l *UpdateUserStatusLogic) UpdateUserStatus(req *types.UpdateUserStatusReq) error {
|
|
|
+ // ... 现有校验 ...
|
|
|
+
|
|
|
+ productCode := middleware.GetProductCode(l.ctx)
|
|
|
+ // 新增:确保目标用户是当前产品的成员
|
|
|
+ if _, err := l.svcCtx.SysProductMemberModel.FindOneByProductCodeUserId(l.ctx, productCode, req.Id); err != nil {
|
|
|
+ return response.ErrBadRequest("目标用户不是当前产品的成员")
|
|
|
}
|
|
|
+
|
|
|
+ if err := authHelper.CheckManageAccess(l.ctx, l.svcCtx, req.Id, productCode); err != nil {
|
|
|
+ return err
|
|
|
+ }
|
|
|
+ // ...
|
|
|
}
|
|
|
```
|
|
|
|
|
|
---
|
|
|
|
|
|
-### H4. 成员类型管理层级与 SQL 设计文档不一致
|
|
|
+### H4. `UpdateUser` 中 `Update` + `UpdateStatus` 非原子操作
|
|
|
|
|
|
-- **位置**:`internal/logic/auth/access.go` `memberTypePriority` 函数 vs `perm.sql` 第 151-155 行注释
|
|
|
-- **描述**:`perm.sql` 中明确标注管理层级顺序为 `超级管理员 > DEVELOPER > ADMIN > MEMBER`,但代码中 `memberTypePriority` 的实现为:
|
|
|
+- **位置**:`internal/logic/user/updateUserLogic.go:111-118`
|
|
|
+- **描述**:当修改用户信息且包含状态变更时,代码先调用 `SysUserModel.Update(ctx, user)` 更新所有字段(包括 `Status`),若状态确实变更又额外调用 `SysUserModel.UpdateStatus(ctx, req.Id, req.Status)`。这两次写操作没有在同一事务中:
|
|
|
+ 1. 第一次 `Update` 已经将 `Status` 写入了 DB,第二次 `UpdateStatus` 是冗余的(其主要目的是递增 `tokenVersion` 使旧 token 失效)。
|
|
|
+ 2. 如果第一次 `Update` 成功但第二次 `UpdateStatus` 失败,用户状态已被冻结但 `tokenVersion` **未递增**,导致用户的旧 token 仍然有效——**被冻结的用户仍可继续访问系统**直到 token 过期。
|
|
|
+- **影响**:冻结用户操作可能只部分生效,导致被冻结的用户仍可在 token 有效期内继续使用系统。
|
|
|
+- **修复方案**:将两步操作合并到一个事务中,或直接只调用 `UpdateStatus`(它内部已经包含了 `tokenVersion` 递增逻辑):
|
|
|
|
|
|
-| 类型 | 代码优先级 | SQL 文档预期 |
|
|
|
-|------|-----------|-------------|
|
|
|
-| SUPER_ADMIN | 0 (最高) | 最高 |
|
|
|
-| ADMIN | 1 | 2 |
|
|
|
-| DEVELOPER | 2 | 1 |
|
|
|
-| MEMBER | 3 (最低) | 最低 |
|
|
|
+```go
|
|
|
+// 方案:移除冗余的双重写入,在 Update 中统一处理
|
|
|
+if statusChanged {
|
|
|
+ // 使用事务确保 status 变更和 tokenVersion 递增的原子性
|
|
|
+ if err := l.svcCtx.SysUserModel.TransactCtx(l.ctx, func(ctx context.Context, session sqlx.Session) error {
|
|
|
+ if err := l.svcCtx.SysUserModel.UpdateWithTx(ctx, session, user); err != nil {
|
|
|
+ return err
|
|
|
+ }
|
|
|
+ return l.svcCtx.SysUserModel.UpdateStatusWithTx(ctx, session, req.Id, req.Status)
|
|
|
+ }); err != nil {
|
|
|
+ return err
|
|
|
+ }
|
|
|
+} else {
|
|
|
+ if err := l.svcCtx.SysUserModel.Update(l.ctx, user); err != nil {
|
|
|
+ return err
|
|
|
+ }
|
|
|
+}
|
|
|
+```
|
|
|
+
|
|
|
+---
|
|
|
|
|
|
- 代码中 ADMIN(1) > DEVELOPER(2),而文档描述 DEVELOPER > ADMIN。
|
|
|
-- **影响**:如果文档是正确的设计意图,那么当前实现存在以下问题:
|
|
|
- - DEVELOPER 无法管理同产品下的 ADMIN 用户(被 `checkPermLevel` 拦截)
|
|
|
- - ADMIN 可以管理 DEVELOPER 用户(不应被允许)
|
|
|
- - `CheckMemberTypeAssignment` 中 DEVELOPER 无法分配 ADMIN 类型(若按文档应该可以)
|
|
|
+### H5. `checkPermLevel` 对非产品成员目标默认放行
|
|
|
|
|
|
- 如果代码是正确的、文档是过时的,则应更新 SQL 注释以避免后续维护者误解。
|
|
|
-- **修复方案**:确认真实的业务层级意图,统一代码与文档。若 DEVELOPER 应高于 ADMIN:
|
|
|
+- **位置**:`internal/logic/auth/access.go:147-177`
|
|
|
+- **描述**:当目标用户**不是**当前产品的成员时,`FindOneByProductCodeUserId` 返回错误,`targetMemberType` 保持为空字符串 `""`。`memberTypePriority("")` 返回 `math.MaxInt32`(即优先级最低)。接下来比较 `callerPri < targetPri`(例如 ADMIN 的 1 < MaxInt32)→ **直接 return nil 放行**。这意味着:任何低级别管理者都可以"管理"一个不属于本产品的用户。
|
|
|
+- **影响**:与 H3 联动,扩大了越权操作的范围。非产品成员的目标被视为"权限最低的人",反而最容易被操作。
|
|
|
+- **修复方案**:当目标用户不是当前产品成员时,应拒绝而非放行:
|
|
|
|
|
|
```go
|
|
|
-func memberTypePriority(memberType string) int {
|
|
|
- switch memberType {
|
|
|
- case consts.MemberTypeSuperAdmin:
|
|
|
- return 0
|
|
|
- case consts.MemberTypeDeveloper:
|
|
|
- return 1
|
|
|
- case consts.MemberTypeAdmin:
|
|
|
- return 2
|
|
|
- case consts.MemberTypeMember:
|
|
|
- return 3
|
|
|
- default:
|
|
|
- return math.MaxInt32
|
|
|
+func checkPermLevel(ctx context.Context, svcCtx *svc.ServiceContext, caller *loaders.UserDetails, targetUserId int64, productCode string) error {
|
|
|
+ if productCode == "" {
|
|
|
+ return response.ErrBadRequest("缺少产品上下文,无法进行权限级别判定")
|
|
|
+ }
|
|
|
+
|
|
|
+ targetMember, err := svcCtx.SysProductMemberModel.FindOneByProductCodeUserId(ctx, productCode, targetUserId)
|
|
|
+ if err != nil {
|
|
|
+ // 目标不是当前产品成员,应拒绝操作而非放行
|
|
|
+ return response.ErrForbidden("目标用户不是当前产品的成员,无法执行管理操作")
|
|
|
}
|
|
|
+ targetMemberType := targetMember.MemberType
|
|
|
+ // ... 后续比较逻辑不变
|
|
|
}
|
|
|
```
|
|
|
|
|
|
-若代码实现是正确的(ADMIN > DEVELOPER),则更新 `perm.sql` 注释为:`超级管理员 > ADMIN > DEVELOPER > MEMBER`。
|
|
|
-
|
|
|
---
|
|
|
|
|
|
-### H5. SyncPerms 传入空权限列表会禁用产品全部权限,缺乏防护
|
|
|
+### H6. gRPC Reflection 在生产环境中开启
|
|
|
|
|
|
-- **位置**:`internal/logic/pub/syncPermsLogic.go` 及 `internal/model/perm/sysPermModel.go` `DisableNotInCodesWithTx`
|
|
|
-- **描述**:当 `SyncPerms` 请求中 `perms` 数组为空时,`codes` 也为空,`DisableNotInCodesWithTx` 会执行:
|
|
|
- ```sql
|
|
|
- UPDATE sys_perm SET status=2 WHERE productCode=? AND status=1
|
|
|
- ```
|
|
|
- 一次性禁用该产品下**所有启用的权限**。
|
|
|
-- **影响**:客户端代码 bug(如序列化异常导致 perms 为空数组)、网络问题(请求被截断)都可能触发全量权限禁用,影响该产品下所有用户的访问。这是一个潜在的可用性灾难。
|
|
|
-- **修复方案**:在 `SyncPerms` 入口处增加空数组保护:
|
|
|
+- **位置**:`perm.go:38`
|
|
|
+- **描述**:`reflection.Register(grpcServer)` 无条件开启了 gRPC Server Reflection。攻击者可使用 `grpcurl` 等工具发现所有 RPC 方法、请求/响应结构,大幅降低攻击门槛。
|
|
|
+- **影响**:结合 H1(`GetUserPerms` 无鉴权),攻击者可自动化发现并利用无保护接口。
|
|
|
+- **修复方案**:仅在开发/测试环境开启反射:
|
|
|
|
|
|
```go
|
|
|
-if len(req.Perms) == 0 {
|
|
|
- return nil, response.ErrBadRequest("权限列表不能为空,如需禁用所有权限请使用专用接口")
|
|
|
+if c.Mode == "dev" {
|
|
|
+ reflection.Register(grpcServer)
|
|
|
}
|
|
|
```
|
|
|
|
|
|
-gRPC 端的 `SyncPermissions` 也需要同步添加此校验。
|
|
|
-
|
|
|
---
|
|
|
|
|
|
-## ⚠️ 健壮性与性能建议 (Medium/Low)
|
|
|
+## ⚠️ 健壮性与性能建议 (Medium / Low)
|
|
|
|
|
|
-### M1. RefreshToken 未实现轮转,被盗令牌在有效期内可无限复用
|
|
|
+### M1. `RequireProductAdmin` 未绑定具体产品(Medium)
|
|
|
|
|
|
-- **位置**:`internal/logic/pub/refreshTokenLogic.go` 第 75 行
|
|
|
-- **级别**:Medium
|
|
|
-- **描述**:`RefreshToken` 接口刷新后直接原样返回旧的 `refreshToken`(`RefreshToken: tokenStr`)。这意味着 refresh token 在整个有效期内是静态不变的,一旦泄露,攻击者可以持续用它换取新的 access token,直到 refresh token 过期或用户主动修改密码(触发 tokenVersion 递增)。
|
|
|
-- **建议**:实现 Refresh Token Rotation:每次刷新时签发新的 refresh token 并使旧的失效(可通过在 Redis 中维护一个 token 黑名单或版本号实现)。
|
|
|
+- **位置**:`internal/logic/auth/access.go:86-98`
|
|
|
+- **描述**:`RequireProductAdmin` 只检查 `caller.MemberType == MemberTypeAdmin`,**不校验** `caller.ProductCode` 是否与目标操作的产品一致。虽然目前代码中大多数场景使用的是 `RequireProductAdminFor`(带产品校验),但如果未来有开发者误用 `RequireProductAdmin`,将产生跨产品越权。
|
|
|
+- **建议**:标记 `RequireProductAdmin` 为 deprecated,或重构为必须传入 `targetProductCode`。
|
|
|
|
|
|
---
|
|
|
|
|
|
-### M2. HTTP 与 gRPC 的 SyncPermissions 逻辑重复,存在不同步风险
|
|
|
+### M2. `UserDetail` 对 `ProductCode=""` 的非超管跳过成员校验(Medium)
|
|
|
|
|
|
-- **位置**:`internal/logic/pub/syncPermsLogic.go` vs `internal/server/permserver.go` `SyncPermissions` 方法
|
|
|
-- **级别**:Medium
|
|
|
-- **描述**:HTTP 端走 `SyncPermsLogic`,gRPC 端在 `permserver.go` 中内联实现了几乎相同的逻辑。两处代码维护相同的业务语义(认证、去重、事务批量更新、缓存清理),但互相独立。
|
|
|
-- **建议**:将核心逻辑抽取为共享的 service 函数(类似 `ValidateProductLogin` 的做法),让 HTTP Logic 和 gRPC Server 都调用同一份代码。
|
|
|
+- **位置**:`internal/logic/user/userDetailLogic.go:35-38`
|
|
|
+- **描述**:仅当 `!caller.IsSuperAdmin && caller.ProductCode != ""` 时才校验目标是否为本产品成员。如果运行时出现 `ProductCode == ""` 的非超管用户(例如直接通过 adminLogin 且未指定产品),则可读取任意用户的基础信息。
|
|
|
+- **建议**:非超管用户在 `ProductCode` 为空时应直接拒绝访问其他用户详情。
|
|
|
|
|
|
---
|
|
|
|
|
|
-### M3. RequireProductAdmin 函数未绑定具体产品,存在跨产品越权隐患
|
|
|
+### M3. 超管 `UserList` 与 `ProductCode` 筛选逻辑不一致(Medium)
|
|
|
|
|
|
-- **位置**:`internal/logic/auth/access.go` `RequireProductAdmin` 函数
|
|
|
-- **级别**:Medium
|
|
|
-- **描述**:`RequireProductAdmin` 只检查 `caller.MemberType == ADMIN`,不验证操作者是否是**目标产品**的管理员。如果产品 A 的 ADMIN 调用了使用此函数鉴权的接口来操作产品 B 的数据,会被错误放行。
|
|
|
-- **现状**:当前所有业务代码使用的是带产品校验的 `RequireProductAdminFor`,`RequireProductAdmin` 实际未被引用。
|
|
|
-- **建议**:删除 `RequireProductAdmin` 函数或标记为 deprecated,避免后续开发者误用。
|
|
|
+- **位置**:`internal/logic/user/userListLogic.go:55-62`
|
|
|
+- **描述**:超管无论是否传了 `ProductCode`,都走 `FindListByPage`(全量分页),而不是按产品筛选。但后续又用 `ProductCode` 去查 `MemberType` 并附加到每条记录上。用户在前端选择按产品过滤时,列表仍然是全库数据,仅是 `MemberType` 字段有值,**与"按产品筛选"的语义不一致**。
|
|
|
+- **建议**:超管也应支持 `ProductCode` 作为筛选条件,按产品成员过滤列表。
|
|
|
|
|
|
---
|
|
|
|
|
|
-### M4. CreateProduct 事务中管理员用户名可能冲突导致整体回滚
|
|
|
+### M4. `loadPerms` 静默忽略数据库错误(Medium)
|
|
|
|
|
|
-- **位置**:`internal/logic/product/createProductLogic.go` 第 87 行
|
|
|
-- **级别**:Medium
|
|
|
-- **描述**:创建产品时自动生成管理员用户名为 `admin_{productCode}`。如果系统中已存在同名用户(如手动创建或之前产品删除后遗留),`InsertWithTx` 会因唯一索引冲突报错,导致整个事务回滚——产品也不会被创建,且错误信息为底层数据库错误,对调用方不友好。
|
|
|
-- **建议**:在事务开始前先检查用户名是否已存在,给出明确的业务错误提示:
|
|
|
+- **位置**:`internal/loaders/userDetailsLoader.go:373-379`
|
|
|
+- **描述**:`allowIds, _ := l.models.SysUserPermModel.FindPermIdsByUserIdAndEffectForProduct(...)` 忽略了错误。如果数据库查询失败,用户的 ALLOW 权限列表为空,最终计算出的权限集合会**比实际少**——用户会被"静默降权"而非收到错误提示。
|
|
|
+- **建议**:DB 错误应向上传递或记录日志并返回错误,避免静默降权:
|
|
|
|
|
|
```go
|
|
|
-if _, err := l.svcCtx.SysUserModel.FindOneByUsername(l.ctx, adminUsername); err == nil {
|
|
|
- return nil, response.ErrConflict(fmt.Sprintf("用户名 %s 已存在,无法自动创建管理员账号", adminUsername))
|
|
|
+allowIds, err := l.models.SysUserPermModel.FindPermIdsByUserIdAndEffectForProduct(...)
|
|
|
+if err != nil {
|
|
|
+ logx.WithContext(ctx).Errorf("load allow perms failed: %v", err)
|
|
|
+ return // 或给 ud.Perms 设置默认空值并标记加载失败
|
|
|
}
|
|
|
```
|
|
|
|
|
|
---
|
|
|
|
|
|
-### M5. BindRolePerms / SetUserPerms 未校验权限的启用状态
|
|
|
-
|
|
|
-- **位置**:`internal/logic/role/bindRolePermsLogic.go` 第 58-64 行、`internal/logic/user/setUserPermsLogic.go` 第 65-74 行
|
|
|
-- **级别**:Low
|
|
|
-- **描述**:绑定权限时只校验了权限 ID 存在且属于同一产品,但未检查权限是否处于启用状态(`status=1`)。已被 `SyncPerms` 禁用的权限仍可被绑定到角色或用户。
|
|
|
-- **实际影响有限**:`loadPerms` 在计算最终权限时会过滤掉 `status != 1` 的权限,所以被禁用的权限不会生效。但绑定关系的存在可能造成管理界面上的困惑。
|
|
|
-- **建议**:在校验循环中增加状态检查:
|
|
|
+### M5. 密码策略偏弱(Medium)
|
|
|
|
|
|
-```go
|
|
|
-for _, p := range perms {
|
|
|
- if p.ProductCode != role.ProductCode {
|
|
|
- return response.ErrBadRequest("不能绑定其他产品的权限")
|
|
|
- }
|
|
|
- if p.Status != consts.StatusEnabled {
|
|
|
- return response.ErrBadRequest(fmt.Sprintf("权限 %s 已被禁用,无法绑定", p.Code))
|
|
|
- }
|
|
|
-}
|
|
|
-```
|
|
|
+- **位置**:`internal/logic/user/createUserLogic.go:48-54`、`internal/logic/auth/changePasswordLogic.go`
|
|
|
+- **描述**:密码最低长度仅 6 字符,无大小写混合、数字、特殊字符等复杂度要求。对于后台管理系统,安全性偏弱。
|
|
|
+- **建议**:至少增加「包含大小写字母和数字」要求,或将最低长度提高至 8 位。
|
|
|
|
|
|
---
|
|
|
|
|
|
-### M6. ProductDetail / ProductList 缺少产品维度的访问控制
|
|
|
+### M6. 登录仅 IP 维度限流,无账户级锁定(Medium)
|
|
|
|
|
|
-- **位置**:`internal/logic/product/productDetailLogic.go`、`productListLogic.go`
|
|
|
-- **级别**:Low
|
|
|
-- **描述**:任何已登录用户(包括非超管的普通产品成员)都可以查看系统中**所有产品**的列表和详情(除 AppKey 外)。虽然敏感字段 `AppKey` 仅对超管可见、`AppSecret` 不返回,但产品编码、名称等信息对非本产品成员可见。
|
|
|
-- **建议**:评估业务需求。如果产品信息确实应该对所有登录用户可见(如产品选择页面),则当前实现合理。否则应增加 `caller.ProductCode` 校验或只返回用户所属产品的列表。
|
|
|
+- **位置**:`internal/svc/servicecontext.go` 中 `LoginRateLimit` 为 60秒/IP/20次
|
|
|
+- **描述**:限流仅基于 IP,攻击者可通过分布式 IP 绕过。对同一用户名的暴力破解尝试没有账户级锁定机制。
|
|
|
+- **建议**:增加按用户名维度的登录失败计数,连续 N 次失败后临时锁定账户或要求验证码。
|
|
|
|
|
|
---
|
|
|
|
|
|
-### M7. DeptTree 接口无任何权限过滤
|
|
|
+### M7. 数据库无外键约束,完全依赖应用层维护引用完整性(Low)
|
|
|
|
|
|
-- **位置**:`internal/logic/dept/deptTreeLogic.go`
|
|
|
-- **级别**:Low
|
|
|
-- **描述**:`DeptTree` 加载并返回系统中**全部部门**的树形结构,无论调用者的身份和所属产品。任何已登录用户都能看到完整的组织架构。
|
|
|
-- **建议**:如果部门树只应由超管可见,增加 `RequireSuperAdmin` 校验。如果需要按层级裁剪(只看本部门及子部门),应根据 `caller.DeptPath` 过滤。
|
|
|
+- **位置**:`perm.sql`
|
|
|
+- **描述**:所有表之间通过 `userId`、`roleId`、`permId`、`productCode` 等字段关联,均**无 FOREIGN KEY**。如果应用层代码遗漏了级联清理(如删除产品时忘记清理成员表),会产生孤儿数据。
|
|
|
+- **当前状态**:`DeleteRole` 有事务内级联清理 `role_perm` 和 `user_role`;`RemoveMember` 有事务内清理 `user_role` 和 `user_perm`。但**无删除产品**和**删除用户**的接口,若未来添加需注意。
|
|
|
+- **建议**:至少通过定期数据校验脚本排查孤儿行,或在关键关联字段上加外键。
|
|
|
|
|
|
---
|
|
|
|
|
|
-### M8. loadRoles 全量加载后内存过滤,存在轻微效率损失
|
|
|
+### M8. `sys_user_role` 表缺少 `roleId` 的单独索引(Low)
|
|
|
|
|
|
-- **位置**:`internal/loaders/userDetailsLoader.go` `loadRoles` 方法
|
|
|
-- **级别**:Low
|
|
|
-- **描述**:`FindRoleIdsByUserId` 返回用户在所有产品下的全部角色 ID,再通过 `FindByIds` 批量查询角色详情,最后在内存中按 `ProductCode` 和 `Status` 过滤。对于只加入了 1-2 个产品的普通用户不会有问题,但逻辑上可以在 SQL 层面就做好过滤。
|
|
|
-- **建议**:新增按产品过滤的查询方法:
|
|
|
+- **位置**:`perm.sql` 中 `sys_user_role` 表唯一索引为 `(userId, roleId)`
|
|
|
+- **描述**:`FindUserIdsByRoleId`(删除角色、更新角色时批量清除缓存用到)按 `roleId` 单列查询,复合索引左前缀为 `userId`,无法高效利用。在角色关联用户数较多时可能影响查询性能。
|
|
|
+- **建议**:增加 `KEY idx_role (roleId)` 索引。
|
|
|
|
|
|
-```go
|
|
|
-func FindRoleIdsByUserIdForProduct(ctx context.Context, userId int64, productCode string) ([]int64, error)
|
|
|
-```
|
|
|
+---
|
|
|
|
|
|
-通过 JOIN `sys_role` 表在查询时过滤 `productCode`,减少不必要的数据传输和内存开销。
|
|
|
+### M9. `CreateProduct` 中 TOCTOU 竞态(Low)
|
|
|
+
|
|
|
+- **位置**:`internal/logic/product/createProductLogic.go:53-56`、`72-75`
|
|
|
+- **描述**:先 `FindOneByCode` 检查产品编码是否存在、再 `FindOneByUsername` 检查管理员用户名是否存在,最后在事务中执行插入。预检与插入之间存在时间窗口,极低概率下两个并发请求可同时通过预检,但实际上 DB 层唯一约束会兜底(返回 1062 错误),不会导致数据损坏。
|
|
|
+- **影响**:极低风险,唯一约束可兜底,但错误信息可能不够友好(返回原始 DB 错误而非"产品编码已存在")。
|
|
|
+- **建议**:在事务内的 Insert 错误中也做 `Duplicate entry` 判断并返回友好错误信息。
|
|
|
|
|
|
---
|
|
|
|
|
|
-### M9. DeleteRole 缓存失效存在极小时间窗口遗漏
|
|
|
+### M10. `BindRoles` 中 `MinPermsLevel == 0` 时绕过角色级别约束(Low)
|
|
|
|
|
|
-- **位置**:`internal/logic/role/deleteRoleLogic.go` 第 40-42 行
|
|
|
-- **级别**:Low
|
|
|
-- **描述**:`affectedUserIds` 在事务执行**之前**查询。如果在查询之后、事务执行之前有新的用户被绑定到该角色,这些用户的缓存不会被主动清理(但会在 5 分钟后自然过期)。
|
|
|
-- **实际影响极低**:这个时间窗口极短(微秒级),且角色删除事务内已清除所有 user-role 绑定关系,新绑定的用户在缓存过期后会自动生效。
|
|
|
-- **建议**:可接受当前实现。如需极致一致性,可在事务内查询 affectedUserIds。
|
|
|
+- **位置**:`internal/logic/user/bindRolesLogic.go:78-80`
|
|
|
+- **描述**:约束条件为 `caller.MinPermsLevel > 0 && r.PermsLevel < caller.MinPermsLevel`。当调用者的 `MinPermsLevel` 为 0(即无角色绑定或角色 `PermsLevel` 为 0)时,整个条件不生效,允许绑定任意 `PermsLevel` 的角色。
|
|
|
+- **影响**:取决于业务设计——如果 `PermsLevel=0` 意味着"无限制"则合理;否则应补充边界处理。
|
|
|
+- **建议**:明确 `MinPermsLevel == 0` 的业务语义。如果 0 不是合法值,应在 `loadRoles` 或角色创建时强制 `PermsLevel >= 1`(目前 `CreateRole` 已做 1-999 校验,故此问题风险较低)。
|
|
|
|
|
|
---
|
|
|
|
|
|
-### M10. CreateUser 缺少用户名格式校验
|
|
|
-
|
|
|
-- **位置**:`internal/logic/user/createUserLogic.go`
|
|
|
-- **级别**:Low
|
|
|
-- **描述**:`CreateUser` 仅校验了用户名长度(最大 64 字符),未校验格式。用户名可以包含空格、特殊字符、中文等,可能导致:
|
|
|
- - 与自动生成的 `admin_{code}` 格式冲突
|
|
|
- - 登录时的编码问题
|
|
|
- - 日志可读性降低
|
|
|
-- **建议**:增加用户名格式校验(如只允许字母数字下划线):
|
|
|
+### M11. Redis 缓存索引集合与数据键 TTL 不同步(Low)
|
|
|
|
|
|
-```go
|
|
|
-if !regexp.MustCompile(`^[a-zA-Z0-9_]{2,64}$`).MatchString(req.Username) {
|
|
|
- return nil, response.ErrBadRequest("用户名只能包含字母、数字和下划线,长度2-64个字符")
|
|
|
-}
|
|
|
-```
|
|
|
+- **位置**:`internal/loaders/userDetailsLoader.go:190-207`
|
|
|
+- **描述**:缓存数据键 TTL 为 300s,索引集合 TTL 为 360s。在 300-360s 之间的时间窗口内,索引集合中引用的数据键已过期但索引还在,`cleanByIndex` 会尝试删除已不存在的键(不会报错但产生无效 DEL 命令)。反向情况:若 `Clean` 在数据键写入后、索引 `SADD` 前被调用(极低概率),则数据键不会被清理直到自然过期。
|
|
|
+- **影响**:极低风险,不影响正确性,仅可能导致少量无效 Redis 操作。
|
|
|
|
|
|
---
|
|
|
|
|
|
-### M11. gRPC GetUserPerms 无鉴权保护
|
|
|
+### M12. 部门树对异常数据的静默处理(Low)
|
|
|
|
|
|
-- **位置**:`internal/server/permserver.go` `GetUserPerms` 方法
|
|
|
-- **级别**:Low(取决于部署方式)
|
|
|
-- **描述**:gRPC 端的 `GetUserPerms` 接口没有任何认证或授权校验,任何能访问 gRPC 端口的客户端都可以查询任意用户的权限列表。
|
|
|
-- **现状评估**:如果 gRPC 仅在内网(如 K8s 集群内部)暴露给可信的下游服务,这是合理的设计。但如果 gRPC 端口意外暴露到公网,则构成严重的信息泄露。
|
|
|
-- **建议**:确保 gRPC 端口不暴露到外部网络。如有需要,可增加 gRPC 拦截器进行 mTLS 或 token 校验。
|
|
|
+- **位置**:`internal/logic/dept/deptTreeLogic.go`
|
|
|
+- **描述**:构建部门树时,如果子节点的 `ParentId` 对应的父节点不在查询结果中(数据不一致),该节点会被当作根节点挂到 `roots`,而不是报错。这会掩盖数据损坏问题。
|
|
|
+- **建议**:对 `ParentId != 0` 但找不到父节点的情况,记录告警日志。
|
|
|
|
|
|
---
|
|
|
|
|
|
-### M12. UpdateMember / UpdateRole / UpdateDept 对无效 status 值静默忽略
|
|
|
+### M13. `response.Setup` 中内部错误信息可能通过日志泄露(Low)
|
|
|
|
|
|
-- **位置**:`updateMemberLogic.go`、`updateRoleLogic.go`、`updateDeptLogic.go` 多处
|
|
|
-- **级别**:Low
|
|
|
-- **描述**:这些接口在处理 `status` 字段时采用"白名单匹配"模式——只有 1 和 2 才会赋值,其他值(如 3、-1)被静默忽略。调用方传入非法值时不会收到任何错误提示,可能导致前端 bug 难以排查。
|
|
|
-- **建议**:对非 0 的非法 status 值返回明确错误:
|
|
|
-
|
|
|
-```go
|
|
|
-if req.Status != 0 {
|
|
|
- if req.Status != consts.StatusEnabled && req.Status != consts.StatusDisabled {
|
|
|
- return response.ErrBadRequest("状态值无效,仅支持 1(启用) 和 2(冻结)")
|
|
|
- }
|
|
|
- role.Status = req.Status
|
|
|
-}
|
|
|
-```
|
|
|
+- **位置**:`internal/response/response.go:46`
|
|
|
+- **描述**:`logx.WithContext(ctx).Errorf("internal error: %+v", err)` 使用 `%+v` 打印了完整的错误堆栈到服务端日志。虽然不会返回给客户端(客户端只收到"服务器内部错误"),但日志中可能包含 SQL 语句、表结构等敏感信息。
|
|
|
+- **建议**:确保日志收集系统有适当的访问控制;考虑对 SQL 相关错误做脱敏处理。
|
|
|
|
|
|
---
|
|
|
|
|
|
-## 总结
|
|
|
+## 审计总结
|
|
|
|
|
|
-| 级别 | 数量 | 关键发现 |
|
|
|
-|------|------|---------|
|
|
|
-| 🚩 High | 5 | token 失效不一致、跨产品信息泄露、层级定义矛盾、数据完整性、无保护的全量禁用 |
|
|
|
-| ⚠️ Medium | 4 | RefreshToken 无轮转、gRPC/HTTP 逻辑重复、函数越权隐患、用户名冲突 |
|
|
|
-| 💡 Low | 8 | 无效权限绑定、产品访问控制、部门树全量暴露、效率优化、格式校验等 |
|
|
|
+| 维度 | 评估 |
|
|
|
+|------|------|
|
|
|
+| **逻辑一致性** | `SetUserPerms`/`BindRoles` 与 `UpdateUserStatus`/`AddMember` 在产品成员校验上**不一致**(H2/H3),是主要风险点 |
|
|
|
+| **并发与竞态** | `CreateProduct` 存在 TOCTOU 但有唯一约束兜底(M9);`UpdateUser` 的双重写入有部分失败风险(H4) |
|
|
|
+| **资源管理** | go-zero 框架层面管理连接池和 Redis,未发现泄漏;`singleflight` 有效防止缓存穿透 |
|
|
|
+| **数据完整性** | 关键写操作使用了事务(角色删除、成员删除、权限同步);**无外键**依赖应用层级联(M7) |
|
|
|
+| **安全漏洞** | gRPC `GetUserPerms` 无鉴权是**最高优先级**修复项(H1);跨产品成员添加(H2)和状态越权修改(H3)次之 |
|
|
|
+| **边界崩溃** | 整体处理较好,`FindOne` 错误统一返回 `ErrNotFound`;`loadPerms` 静默忽略错误有隐患(M4) |
|
|
|
+| **SQL 注入** | 所有自定义 SQL 均使用参数化查询,LIKE 做了通配符转义,**未发现注入风险** |
|
|
|
|
|
|
-**整体评价**:项目的架构设计合理,go-zero 框架的使用规范,核心安全机制(JWT tokenVersion、bcrypt、参数化 SQL、限流)实现到位。主要风险集中在**同一业务操作的多个入口未保持一致性**(如 UpdateUser vs UpdateUserStatus)以及**数据隔离在产品边界上的不完整**。建议优先修复 H1(token 失效不一致)和 H5(空列表全量禁用),这两个问题在生产环境中最有可能造成实际影响。
|
|
|
+**建议修复优先级**:H1 > H2 = H3 = H5 > H4 > H6 > M1 ~ M6
|