java-code-review

# Java Code Review Skill ## 概述 本 skill 用于对 Java 项目做安全评审，聚焦以下三类问题： 1. **安全风险**：NullPointerException、未捕获异常、数组/集合越界、类型转换异常、线程安全、资源泄漏 2. **MyBatis/MyBatis-Plus SQL 改动**：提取所有使用 MyBatis 框架的 SQL 操作，还原原始 SQL 语句 --- ## 执行步骤 ### Step 1：分支来源检查 在做代码评审之前，先验证当前分支的来源是否符合规范。 ```bash # 获取当前分支名 git branch --show-current # 检查当前分支是否从 master 切出（找到与 master 的分叉点） git merge-base HEAD master # 查看从分叉点到现在的所有 merge commit，检查是否有 develop 分支合入 git log $(git merge-base HEAD master)..HEAD --merges --oneline # 查看完整提交历史（含合并），找出所有合入的分支名 git log $(git merge-base HEAD master)..HEAD --oneline --graph ``` **判断逻辑：** 1. **验证是否从 master 切出** ```bash # 找分叉点 FORK_POINT=$(git merge-base HEAD master) # 检查分叉点是否在 master 的提交历史中 git branch --contains $FORK_POINT | grep -E '^\*?\s*master$' ``` - ✅ 分叉点存在于 master 历史 → 分支来源正常 - ❌ 分叉点不在 master 历史 → 说明不是从 master 切出，输出警告 2. **检查是否有 develop 分支代码合入** ```bash # 查找 merge commit 中来自 develop 的记录 git log $(git merge-base HEAD master)..HEAD --merges --oneline | grep -i develop # 或通过引用日志确认 git log $(git merge-base HEAD master)..HEAD --oneline --graph | grep -i develop ``` - ✅ 无 develop 相关 merge commit → 分支干净 - ❌ 发现 develop 分支合入记录 → 输出高危警告，列出具体 merge commit **输出格式（在最终报告最前面展示）：** ```plain 【分支来源检查】 当前分支: feature/xxx 分叉自: master (commit: a1b2c3d) ✅ 来源正常：当前分支从 master 切出 ❌ 警告：检测到 develop 分支代码合入！ Merge commit: f3e2d1c Merge branch 'develop' into feature/xxx 合入时间: 2024-01-15 14:32 风险说明: develop 分支可能包含未经测试或未上线的功能，混入当前分支可能导致意外代码上线 ``` > 如果 develop 分支有代码合入，应在评审报告顶部用 🔴 标注，提醒评审人重点关注。 ### Step 2：获取 Git Diff ```bash # 获取当前分支与 master 的完整 diff（含上下文行） git diff master...HEAD -- '*.java' '*.xml' # 如果 master 不存在，尝试 main git diff main...HEAD -- '*.java' '*.xml' # 获取变更的文件列表 git diff --name-only master...HEAD -- '*.java' '*.xml' ``` > 注意：Java 项目的 SQL 不仅在 `.java` 文件中，MyBatis 的 XML Mapper 文件（`*.xml`）也是 SQL 的重要来源，需要一并获取。 如果仓库没有 master/main，改用： ```bash git log --oneline -1 # 确认 HEAD git diff HEAD~1 HEAD -- '*.java' '*.xml' ``` ### Step 3：获取上下文代码 对于 diff 中涉及的每个文件，获取完整文件内容以理解上下文： ```bash # 获取变更文件的完整内容（当前版本） git show HEAD:<filepath> # 或直接读取工作区文件 cat <filepath> ``` **重点关注：** + diff 所在方法的完整实现 + 被修改代码调用的方法签名与返回类型 + 实体类/DTO 定义（判断字段是否可能为 null） + 异常处理链路（try-catch-finally 是否完整） + Spring Bean 注入方式和生命周期 + MyBatis Mapper 接口与对应 XML 映射文件 **读取文件时使用带行号的命令，为后续行号定位做准备：** ```bash # 带行号读取，方便后续 grep 验证 cat -n <filepath> # 查看关键注解和 SQL 相关代码 grep -n "@Select\|@Insert\|@Update\|@Delete\|@Transactional\|@Param\|QueryWrapper\|LambdaQueryWrapper" <filepath> ``` ### Step 4：安全问题分析 逐一检查以下风险类别： #### 4.1 NullPointerException 风险 | 场景 | 检查点 | | --- | --- | | 方法返回值为 null | 调用方法后是否判空再使用（特别是 `findById`、`getOne`、`selectOne` 等查询方法） | | 集合操作 | `list.get(0)`、`list.stream()` 前是否判断 `list != null && !list.isEmpty()` | | Map 取值 | `map.get(key)` 返回可能为 null，链式调用前是否判空 | | 自动拆箱 | `Integer`/`Long`/`Boolean` 等包装类型拆箱为基本类型时，值为 null 会 NPE | | Optional 误用 | `Optional.get()` 前是否调用 `isPresent()` 或使用 `orElse`/`orElseThrow` | | 链式调用 | `a.getB().getC().getName()` 中间任一环节为 null 即 NPE | | Spring 注入 | `@Autowired` 字段在非 Spring 管理的对象中为 null | #### 4.2 未捕获/未处理异常 | 场景 | 检查点 | | --- | --- | | Checked Exception | 方法签名中 throws 的异常是否被调用方正确处理 | | Runtime Exception | 数据库操作、JSON 解析、远程调用等是否有 try-catch | | 空 catch 块 | `catch (Exception e) {}` 吞掉异常不处理不记录 | | catch 范围过大 | `catch (Exception e)` 覆盖了不该被忽略的异常类型 | | @Transactional 回滚 | 默认只回滚 RuntimeException，checked exception 不回滚，需要 `rollbackFor = Exception.class` | | CompletableFuture | 异步任务中的异常是否被 `exceptionally`/`handle` 处理 | #### 4.3 数组/集合越界 | 场景 | 检查点 | | --- | --- | | 数组下标访问 | `arr[i]` 前是否判断 `i >= 0 && i < arr.length` | | List.get(index) | 是否判断 `index < list.size()` | | subList 操作 | `list.subList(from, to)` 中 to 是否可能超出 size | | 空集合取首元素 | `list.get(0)` 或 `list.iterator().next()` 前是否判空 | | Stream 操作 | `findFirst().get()` 未处理空 Optional | | 字符串截取 | `str.substring(a, b)` 中 b 是否可能越界 | #### 4.4 类型转换风险 | 场景 | 检查点 | | --- | --- | | 强制类型转换 | `(TargetType) obj` 前是否使用 `instanceof` 检查 | | 泛型擦除 | 运行时类型不匹配导致 ClassCastException | | JSON 反序列化 | `JSON.parseObject` / `ObjectMapper.readValue` 类型不匹配 | | 数值类型转换 | `Long` 强转 `Integer` 可能溢出，`BigDecimal` 与 `double` 精度丢失 | #### 4.5 线程安全问题 | 场景 | 检查点 | | --- | --- | | 共享可变状态 | 成员变量在多线程环境下是否有同步保护 | | SimpleDateFormat | 非线程安全，是否作为成员变量共享（应使用 `DateTimeFormatter`） | | HashMap 并发写 | 多线程写 HashMap 导致死循环或数据丢失，应使用 ConcurrentHashMap | | 双重检查锁定 | 单例模式中变量是否声明为 `volatile` | | Spring Bean 默认单例 | Controller/Service 中的成员变量在并发请求下共享 | | 线程池参数 | 核心线程数、队列容量、拒绝策略是否合理 | #### 4.6 资源泄漏 | 场景 | 检查点 | | --- | --- | | IO 流未关闭 | InputStream/OutputStream/Reader/Writer 是否使用 try-with-resources | | 数据库连接 | 手动获取的 Connection/Statement/ResultSet 是否关闭 | | HTTP 连接 | HttpClient/OkHttp 的 Response/ResponseBody 是否关闭 | | 线程池未关闭 | ExecutorService 在应用关闭时是否调用 shutdown | | Redis 连接 | Jedis/Lettuce 连接是否归还连接池 | #### 4.7 其他常见问题 + **SQL 注入**：MyBatis 中使用 `${}` 拼接用户输入（应使用 `#{}`） + **Spring 事务失效**：同类内部方法调用 `@Transactional` 方法不会走代理 + **序列化安全**：敏感字段（密码、token）是否标记 `@JsonIgnore` 或 `transient` + **日志信息泄漏**：日志中是否打印了敏感信息（密码、身份证号、手机号） + **硬编码**：密码、密钥、API Key 是否硬编码在代码中 + **equals/hashCode**：重写 `equals` 是否同时重写 `hashCode` ### Step 5：提取 MyBatis/MyBatis-Plus SQL 改动 识别所有 MyBatis 操作并还原原始 SQL。 #### 5.1 识别 MyBatis 操作模式 **注解方式（Mapper 接口）：** ```java // 查询类 @Select("SELECT * FROM user WHERE id = #{id}") User selectById(@Param("id") Long id); @Select("<script>SELECT * FROM user WHERE status = #{status} " + "<if test='name != null'>AND name LIKE CONCAT('%', #{name}, '%')</if>" + "</script>") List<User> selectByCondition(@Param("status") Integer status, @Param("name") String name); // 写入类 @Insert("INSERT INTO user(name, age) VALUES(#{name}, #{age})") int insert(User user); // 更新类 @Update("UPDATE user SET name = #{name} WHERE id = #{id}") int updateById(User user); // 删除类 @Delete("DELETE FROM user WHERE id = #{id}") int deleteById(@Param("id") Long id); ``` **XML Mapper 方式：** ```xml <!-- 查询 --> <select id="selectByCondition" resultType="User"> SELECT * FROM user <where> <if test="name != null">AND name = #{name}</if> <if test="age != null">AND age > #{age}</if> </where> ORDER BY id DESC LIMIT #{offset}, #{limit} </select> <!-- 写入 --> <insert id="batchInsert" parameterType="list"> INSERT INTO user(name, age) VALUES <foreach collection="list" item="item" separator=","> (#{item.name}, #{item.age}) </foreach> </insert> <!-- 更新 --> <update id="updateSelective"> UPDATE user <set> <if test="name != null">name = #{name},</if> <if test="age != null">age = #{age},</if> </set> WHERE id = #{id} </update> <!-- 删除 --> <delete id="deleteByIds"> DELETE FROM user WHERE id IN <foreach collection="ids" item="id" open="(" close=")" separator=","> #{id} </foreach> </delete> ``` **MyBatis-Plus 方式（Java 代码）：** ```java // 查询类 QueryWrapper<User> wrapper = new QueryWrapper<>(); wrapper.eq("status", 1).like("name", keyword).orderByDesc("created_at"); userMapper.selectList(wrapper); LambdaQueryWrapper<User> lambdaWrapper = new LambdaQueryWrapper<>(); lambdaWrapper.eq(User::getStatus, 1).last("LIMIT 10"); userMapper.selectOne(lambdaWrapper); // 写入类 userMapper.insert(user); saveBatch(userList); // 更新类 UpdateWrapper<User> updateWrapper = new UpdateWrapper<>(); updateWrapper.eq("id", id).set("status", 2); userMapper.update(null, updateWrapper); LambdaUpdateWrapper<User> lambdaUpdate = new LambdaUpdateWrapper<>(); lambdaUpdate.eq(User::getId, id).set(User::getStatus, 2); userMapper.update(null, lambdaUpdate); // 删除类 userMapper.deleteById(id); userMapper.deleteBatchIds(idList); ``` #### 5.2 SQL 还原规则 | MyBatis/MyBatis-Plus 代码 | 对应 SQL | | --- | --- | | `#{param}` | 预编译占位符 `?`（安全） | | `${param}` | 直接字符串拼接（有 SQL 注入风险！） | | `<if test="name != null">` | 条件成立时拼接该 SQL 片段 | | `<where>` | 自动添加 WHERE 并去除多余 AND/OR | | `<set>` | 自动添加 SET 并去除末尾逗号 | | `<foreach collection="list">` | 展开为 `(?, ?, ?)` 形式 | | `<choose><when><otherwise>` | 类似 switch-case 选择 SQL 片段 | | `<trim>` | 自定义前缀后缀和去除规则 | | `wrapper.eq("col", val)` | `WHERE col = val` | | `wrapper.ne("col", val)` | `WHERE col <> val` | | `wrapper.like("col", val)` | `WHERE col LIKE '%val%'` | | `wrapper.likeRight("col", val)` | `WHERE col LIKE 'val%'` | | `wrapper.in("col", list)` | `WHERE col IN (v1, v2, ...)` | | `wrapper.between("col", v1, v2)` | `WHERE col BETWEEN v1 AND v2` | | `wrapper.isNull("col")` | `WHERE col IS NULL` | | `wrapper.orderByDesc("col")` | `ORDER BY col DESC` | | `wrapper.last("LIMIT 10")` | 在末尾追加 `LIMIT 10` | | `wrapper.select("id", "name")` | `SELECT id, name` | | `wrapper.groupBy("col")` | `GROUP BY col` | | `wrapper.having("count > {0}", 5)` | `HAVING count > 5` | | `updateWrapper.set("col", val)` | `SET col = val` | | `updateWrapper.setSql("col = col + 1")` | `SET col = col + 1` | **还原步骤：** 1. 确定操作表名（`@TableName` 注解、XML 中的表名、或实体类名转 snake_case） 2. 拼接 SELECT 子句（select 指定字段或 `*`） 3. 拼接 JOIN 子句（如有关联查询） 4. 拼接 WHERE 条件，区分 `#{}` (安全) 和 `${}` (风险)，标注动态条件 5. 拼接 ORDER BY / LIMIT / GROUP BY 6. 对写操作，列出所有被修改的字段 7. 对动态 SQL（`<if>`、`<choose>` 等），列出所有可能的 SQL 变体 --- ## 行号确认（重要） `git diff` 输出的 `@@` 行号是 diff 上下文行号，**不能直接用于报告**，必须通过当前分支的实际文件确认真实行号。 对每一处需要标注行号的问题代码，执行以下步骤： ```bash # 用问题代码的关键字在当前文件中定位真实行号