golang-code-review

# Golang Code Review Skill ## 概述 本 skill 用于对 Golang 项目做安全评审，聚焦以下三类问题： 1. **安全风险**：panic、未判断 error、数组越界、nil 指针解引用 2. **GORM SQL 改动**：提取所有使用 GORM 框架的 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 -- '*.go' # 如果 master 不存在，尝试 main git diff main...HEAD -- '*.go' # 获取变更的文件列表 git diff --name-only master...HEAD -- '*.go' ``` 如果仓库没有 master/main，改用： ```bash git log --oneline -1 # 确认 HEAD git diff HEAD~1 HEAD -- '*.go' ``` ### Step 3：获取上下文代码 对于 diff 中涉及的每个文件，获取完整文件内容以理解上下文： ```bash # 获取变更文件的完整内容（当前版本） git show HEAD:<filepath> # 或直接读取工作区文件 cat <filepath> ``` **重点关注：** + diff 所在函数的完整实现 + 被修改代码调用的函数签名 + 结构体定义（判断字段是否可为 nil） + 错误处理链路 **读取文件时使用带行号的命令，为后续行号定位做准备：** ```bash # 带行号读取，方便后续 grep 验证 cat -n <filepath> # 或只看关键函数所在区域 grep -n "func \|\.Where\|\.Find\|\.Create\|\.Update\|\.Delete" <filepath> ``` ### Step 4：安全问题分析 逐一检查以下风险类别： #### 3.1 Panic 风险 | 场景 | 检查点 | | --- | --- | | nil 指针解引用 | 指针/接口在使用前是否判空 | | 类型断言 | `x.(T)` 是否使用双返回值 `v, ok := x.(T)` | | map 并发读写 | 是否有并发场景未加锁 | | channel 操作 | 向关闭的 channel 发送数据 | | 空接口转换 | interface{} 断言未处理 ok | #### 3.2 未判断 Error | 场景 | 检查点 | | --- | --- | | 函数返回 error | `_, err` 中 err 是否被 `if err != nil` 检查 | | GORM 操作 | `.Error` 字段是否被检查 | | 文件操作 | os.Open/Read/Write 的 error | | JSON 解析 | json.Unmarshal 的 error | | 类型转换 | strconv 系列函数的 error | #### 3.3 数组/切片越界 | 场景 | 检查点 | | --- | --- | | 直接下标访问 | `arr[i]` 前是否判断 `i < len(arr)` | | 切片操作 | `s[a:b]` 中 b 是否可能超出 len | | 空切片取第一个 | `s[0]` 前是否判断 `len(s) > 0` | | for range 后取值 | 循环结束后直接取值可能越界 | #### 4.4 其他常见问题 + goroutine 泄露（context 未传递或未取消） + 锁未释放（defer mu.Unlock() 是否正确） + 资源未关闭（rows.Close()、file.Close()） ### Step 5：提取 GORM SQL 改动 识别所有 GORM 操作并还原原始 SQL。 #### 5.1 识别 GORM 操作模式 ```plain // 查询类 db.Where(...).Find(&result) db.Where(...).First(&result) db.Raw("SELECT ...").Scan(&result) db.Table("...").Select("...").Where(...).Find(...) // 写入类 db.Create(&obj) db.Save(&obj) db.Model(&obj).Update(field, value) db.Model(&obj).Updates(map/struct) // 删除类 db.Where(...).Delete(&obj) db.Unscoped().Where(...).Delete(&obj) // 执行类 db.Exec("UPDATE ...") ``` #### 5.2 SQL 还原规则 | GORM 代码 | 对应 SQL | | --- | --- | | `.Where("name = ?", "tom")` | `WHERE name = 'tom'` | | `.Where("age > ? AND status = ?", 18, 1)` | `WHERE age > 18 AND status = 1` | | `.Where(User{Name: "tom"})` | `WHERE name = 'tom'` | | `.Find(&users)` | `SELECT * FROM users` | | `.Select("id, name")` | `SELECT id, name FROM ...` | | `.Limit(10).Offset(20)` | `LIMIT 10 OFFSET 20` | | `.Order("created_at DESC")` | `ORDER BY created_at DESC` | | `.Joins("LEFT JOIN orders ON ...")` | `LEFT JOIN orders ON ...` | | `.Create(&user)` | `INSERT INTO users (...) VALUES (...)` | | `.Save(&user)` | `INSERT ... ON DUPLICATE KEY UPDATE` 或 `UPDATE users SET ... WHERE id=?` | | `.Updates(map)` | `UPDATE users SET col1=?, col2=? WHERE ...` | | `.Delete(&user)` | `DELETE FROM users WHERE id=?`（软删除：`UPDATE users SET deleted_at=? WHERE id=?`） | | `.Unscoped().Delete()` | `DELETE FROM users WHERE id=?`（硬删除） | **还原步骤：** 1. 确定操作表名（Model/Table 参数或结构体名转 snake_case） 2. 拼接 SELECT 子句 3. 拼接 JOIN 子句 4. 拼接 WHERE 条件，将 `?` 替换为实际值（如能推断） 5. 拼接 ORDER BY / LIMIT / OFFSET 6. 对写操作，列出所有被修改的字段 --- ## 行号确认（重要） `git diff` 输出的 `@@` 行号是 diff 上下文行号，**不能直接用于报告**，必须通过当前分支的实际文件确认真实行号。 对每一处需要标注行号的问题代码，执行以下步骤： ```bash # 用问题代码的关键字在当前文件中定位真实行号 grep -n "关键代码片段" <filepath> # 如果关键字不唯一，结合函数名缩小范围 grep -n "func FunctionName" <filepath> # 多行 GORM 链式调用，用链式起始行定位 grep -n "db\.Where\|db\.Model\|db\.Table" <filepath> ``` **规则：** + 报告中的行号必须来自 `grep -n` 的实际输出，不得使用 diff 中的 `@@` 行号 + 对于多行链式调用（如 GORM），行号指向该调用的**第一行** + 若同一代码片段出现多次，结合函数上下文取正确的那一处 --- ## 输出格式 ### 一、安全风险清单 按严重程度排列（🔴 高危 / 🟡 中危 / 🟢 低危）： ```plain 🔴 [高危] 文件: internal/service/user.go, 行: 42 问题: nil 指针解引用 —— user 可能为 nil，但直接访问 user.Name 改动前: （原始代码） 改动后: （新代码） 建议: 在使用 user 前添加 if user == nil { return ... } 🟡 [中危] 文件: internal/repo/order.go, 行: 88 问题: GORM 查询结果未判断 Error 改动后: result := db.Where(...).Find(&orders) 建议: 添加 if result.Error != nil { return result.Error } ``` ### 二、GORM SQL 改动清单 ```plain 【新增】internal/repo/order.go, 行: 55 GORM 代码: db.Where("user_id = ? AND status IN ?", userID, []int{1, 2}). Order("created_at DESC"). Limit(10). Find(&orders) 原始 SQL: SELECT * FROM orders WHERE user_id = <userID> AND status IN (1, 2) ORDER BY created_at DESC LIMIT 10 备注: 如 Order 有 deleted_at 字段（软删除），实际 SQL 还会附加 AND deleted_at IS NULL --- 【修改】internal/repo/product.go, 行: 120 GORM 代码（改动前）: db.Where("id = ?", id).First(&product) GORM 代码（改动后）: db.Where("id = ? AND shop_id = ?", id, shopID).First(&product) 原始 SQL（改动后）: SELECT * FROM products WHERE id = <id> AND shop_id = <shopID> ORDER BY id LIMIT 1 ``` --- ## 注意事项 + 若项目使用了 GORM v1（`github.com/jinzhu/gorm`）和 v2（`gorm.io/gorm`），软删除行为略有差异，请在备注中注明版本 + 对于 `db.Raw()` 和 `db.Exec()`，直接提取 SQL 字符串，无需转换 + 链式调用跨多行时，需完整拼接所有条件 + Scopes 函数（`db.Scopes(ActiveUser)`）需追踪 Scope 定义来还原条件