别让 null 短路了你的权限校验：一次 25% 覆盖率的 Code Review 复盘

知守观
38 阅读9分钟

老炮踩坑录 · FIN-F03 · 翻车现场系列

基于「协作金融项目」真实源码，复盘我在 review 里放行的第三处

关键词：越权 · null 短路 · 权限校验 · fail closed · 自助提权

👋 欢迎阅读

f26.png 🏠个人主页：知守观
📘我的专栏： 老炮踩坑录
💻当前内容：一次 review 的复盘

引言

写完 内网项目，密码明文放 yml 怎么了 那篇数据源密码，我回到同一个文件：AccountController.java。

这个类的代码总共 136 行。一次让我后背发凉的 Code Review：我为什么放行了默认密码 123456？ 讲的是 37 行的 add，这篇讲 96 行的 resetPwd。同一个文件，同一次 review，我点的同一个通过。

动笔之前我以为这篇的主角是 resetPwd 里那行权限判断。读完整个类的代码才发现，resetPwd 算写得好的，它至少写了校验。真正的主角在 70 行 edit 方法，一行权限判断都没有，还能把任何人的超管标志改成 1。

下面我们按顺序来讲。

现场：resetPwd 的权限校验长什么样

原文没动，代码如下：

/** 重置密码 */
@PutMapping("/{accCode}/password")
public Result<Void> resetPwd(@PathVariable String accCode,
                             @RequestBody Map<String, Object> body,
                             HttpServletRequest request) {
    LoginUser user = (LoginUser) request.getAttribute(AuthInterceptor.CURRENT_USER_ATTR);
    String pwd = str(body.get("password"));
    if (pwd == null || pwd.isBlank()) {
        throw new BizException(400, "密码不能为空");
    }
    // 非超管只能改自己密码
    if (user != null && !"1".equals(user.getAdministrator()) && !accCode.equals(user.getAccCode())) {
        throw new BizException(403, "无权修改他人密码");
    }
    systemMapper.updateAccountPwd(accCode, Md5Util.md5(pwd.trim()), user != null ? user.getAccCode() : "system");
    return Result.success("密码重置成功", null);
}

登录人从 request 属性里取，这个属性是 AuthInterceptor 放进去的。校验意图写在注释里：非超管只能改自己密码。

问题在那行 if 的条件判断：user != null &&。

&& 是短路运算。user 为 null 的时候，整个条件直接判 false，403 不会抛，代码继续往下走，updateAccountPwd 照常被调用。这个接口的权限判断，对 "user 取不到值" 这种请求是完全开放的。

更值得注意是的三元表达式：

user != null ? user.getAccCode() : "system"

写代码的人知道 user 可能是 null。他做了处理，处理方式是放行，顺手把操作人写成 "system"。也就是说，真有这么一个请求进来改了密码，数据库的审计字段里留下的操作人是 system。事后追查，追到一个不存在的操作员头上。

这个洞现在触发不了，盖着它的是拦截器

如实说，这个接口今天在生产上大概率是安全的，原因在这份代码之外。

WebConfig 里的注册：

registry.addInterceptor(authInterceptor)
        .addPathPatterns("/api/**")
        .excludePathPatterns(
                "/api/auth/login",
                "/api/auth/captcha",
                "/error"
        );

AuthInterceptor.preHandle 里，token 校验不过直接返回 401，请求根本到不了 Controller。resetPwd 在 /api/** 覆盖范围内，正常请求走到它的时候，user 不会是 null。

所以这行 user != null && 平时触发不到。它开始生效的场景有三个：

  1. excludePathPatterns 的白名单哪天加了一行，或者路径模式改错，拦截器对这个接口失效
  2. 单元测试用 standaloneSetup 起 MockMvc，拦截器不在容器里，user 稳定为 null。测试能跑过，恰恰因为权限校验没生效，还会留下"这个接口测试通过"的记录
  3. 这段校验写法被复制到别的 Controller，那个 Controller 恰好不在拦截器覆盖范围

三个场景都不需要谁做错什么大的决定，都是日常改动。权限判断的正确性，压在一个它控制不了的上游配置上。

同一份文件里更大的洞：edit 没有校验

resetPwd 至少写了校验，只是写漏了一个分支。70 行的 edit 方法，一个权限判断都没有：

/** 修改操作员 */
@PutMapping
public Result<Void> edit(@RequestBody Map<String, Object> body) {
    // 参数校验、登录名查重……
    Map<String, Object> a = new HashMap<>();
    a.put("accCode", accCode);
    a.put("accLoginName", accLoginName);
    a.put("administrator", str(body.getOrDefault("administrator", "0")));
    a.put("enabled", str(body.getOrDefault("enabled", "1")));
    // ……
    systemMapper.updateAccount(a);
}

administrator 字段直接从请求体搬进更新语句，没有任何 "只有超管能改" 的判断。也就是说，任何一个登录态的账号，调一次这个接口，把自己的 administrator 改成 "1"，就是超管了。改别人的也行。

这是这份文件里实际危害最大的一处，而且它是活的。拦截器只校验登录，不校验权限，登录之后的每一步它都不管。

顺手把剩下几个接口也盘一遍：

  • toggleEnabled 启用停用，无权限判断，操作人写死 "system"；
  • remove 停用操作员，只校验了"目标不是超管"，没校验调用方是谁，操作人同样写死 "system"；
  • add 新增操作员， 讲了它的默认密码，它同样没有权限判断。

四个写接口，权限校验的覆盖率是四分之一，而且这唯一的四分之一还有个 null 分支。

我当时为什么放行：有校验的比没校验的更容易过审

一次让我后背发凉的 Code Review：我为什么放行了默认密码 123456？ 的 add方法，我栽在 "有注释" 上。一次让我后背发凉的 Code Review：我为什么放行了默认密码 123456？ 的 yml，我栽在"配置不用审"上。这个文件里的权限问题，我栽在第三个地方：有校验的代码，比没校验的更容易过审。

  • 看 resetPwd 的时候，我的注意力在"有没有权限校验"。有，user != null && 非超管 && 非本人，三件套齐全，看起来很严谨。至于 user 为什么是 null、null 的时候往哪走，我没追问。null 防护在普通代码里是好习惯，我看到 user != null 的第一反应是"作者考虑周到"，但是没有多想一层：在权限判断里，null 防护就是放行。
  • 看 edit 的时候，问题更简单，它长得就像一段纯 CRUD：参数校验、查重、组装、更新。我对这类代码的审法就是看字段对不对，权限这件事压根没进我的检查项。

四个接口的权限覆盖率是 25%，这个数字我在点通过的时候没算过。review 的检查项里有"有没有权限校验"，没有"这份文件的权限校验覆盖率是多少"。缺的不是某一行代码，是覆盖率这个概念本身。

改法：权限判断，取不到值就拒绝

resetPwd 的正确写法，核心就一条：user 为 null 的时候返回 401，不是放行。

@PutMapping("/{accCode}/password")
public Result<Void> resetPwd(@PathVariable String accCode,
                             @RequestBody Map<String, Object> body,
                             HttpServletRequest request) {
    LoginUser user = (LoginUser) request.getAttribute(AuthInterceptor.CURRENT_USER_ATTR);
    if (user == null) {
        throw new BizException(401, "未登录");
    }
    String pwd = str(body.get("password"));
    if (pwd == null || pwd.isBlank()) {
        throw new BizException(400, "密码不能为空");
    }
    boolean isAdmin = "1".equals(user.getAdministrator());
    boolean isSelf = accCode.equals(user.getAccCode());
    if (!isAdmin && !isSelf) {
        throw new BizException(403, "无权修改他人密码");
    }
    systemMapper.updateAccountPwd(accCode, Md5Util.md5(pwd.trim()), user.getAccCode());
    return Result.success("密码重置成功", null);
}

安全设计里有一条通行的规则：鉴权链路任何一环取不到值，结果必须是不通过。这段代码的现状是反着来的：取不到 user，默认你有权。

  • edit 的修法也直接：修改他人资料要求超管；administrator 字段的变更单独卡一道，只允许超管发起，收到非超管请求里带这个字段直接 403，不是默默忽略，要让调用方知道这个动作被拒绝了。
  • 审计字段立一条规矩：web 层的代码里不允许出现 "system" 这种字面量当操作人。操作人必须来自登录态。真正的系统任务有自己的通道，不跟用户请求混用一张表一个字段。

拦截器那层保持现状就好，它没有错。错的是业务代码把权限判断的正确性托付给了它。两层各管各的，拦截器管登录，方法里管授权，任何一层失守，另一层还能挡。

自查清单：五个能直接搜的信号

image.png

自查打卡：全局搜一下 if (user != null &&。如果命中出现在权限判断的位置，评论区报个数，我看看这种"考虑周到"的放行有多少。

老炮点评

协作金融项目这三篇，盘的是同一个项目的三块短板：

  1. 内网项目，密码明文放 yml 怎么了 是存储，密码怎么落库；
  2. 一次让我后背发凉的 Code Review：我为什么放行了默认密码 123456？ 是配置，口令放在哪；
  3. 《别让 null 短路了你的权限校验：一次 25% 覆盖率的 Code Review 复盘》 是授权，谁能改。

三块各缺一角，缺的方式一模一样：每一处都有个"看起来像防护"的东西在场，add 有注释，yml 有"如密码不同请修改"，resetPwd 有三件套权限条件。

有个东西看起来像防护，检查的人就停在这里，没有再往下问它到底防没防住。

我作为 review 的人，三次都在场，三次都点了通过。

这个系列的文章写到这里，责任也说清了：写代码的人一份，点通过的我一份。谁也别替谁扛。

最后问一句：你们项目的写接口，权限校验覆盖率是多少？注意不是"有没有做权限"，是"逐个接口数一遍，有校验的占几成"。算过的扣 1，没算过的扣 2，我先坦白，我当年没算过。

相关阅读：

如果本文对你有帮助，欢迎：

👍 点赞 ｜ ⭐ 收藏 ｜ 👤 关注 ｜ 💬 留言

我是老炮，18 年 Java 老兵，仍在一线。关注「Java老炮踩坑录」，不错过每一篇真实案例，少踩坑。

老炮的排雷服务： 权限校验（越权漏洞）是等保测评和安全审计的重灾区。如果你现在的系统也缺乏系统的权限管理，或者需要做老代码的安全审计、重构与排查，欢迎加我个人微信：javalaopao（备注“公众号”秒过）。18 年老兵，帮你排掉那些看不见的雷。

下期预告：《改一条促销，价签全城重算》

下期我们换个其它的企业真实项目来聊。营销后台点一下保存，一条 MQ 广播飞向全城门店的价签服务，批量重算就此开始。一次配置变更的影响面有多大，下一篇拿促销系统的真实代码量给你看。

avatar
文章
阅读
粉丝