reviewing-code-changes

审查代码变更

审查差异并不等于对代码库做一次缩小版审计。差异会展示发生了哪些变化，却会隐藏这些变化的含义——现在接收不可信数据的调用方、以前会执行而现在不再执行的检查、不再成立的假设。只审查新增行，是最容易漏掉漏洞的常见做法。

适用场景

• 审查拉取请求、分支或提交范围的安全性
• 提交或合并前检查自己的变更
• 评估依赖项升级或配置变更的风险
• 发布前审查 AI 生成的变更

不适用场景

• 整个代码库的审计——使用 auditing-code-for-vulnerabilities
• 对已部署变更进行运行时测试——使用相关测试技能
• 以依赖项／CI 变更为主要审查对象——使用 auditing-supply-chain

先获取真实差异

# Against the actual merge base, not against whatever HEAD happens to be
BASE=$(git merge-base HEAD origin/main)
git diff --stat $BASE...HEAD
git diff $BASE...HEAD

# Rename and whitespace noise hides real changes
git diff -M -C -w $BASE...HEAD

# Files changed most, and by whom — where to spend attention
git diff --numstat $BASE...HEAD | sort -rn | head -20

阅读代码前，先确认三件事：

1. 这项变更要实现什么？ 阅读 PR 描述和测试。不清楚变更意图就做安全审查，只是在对照模式。
2. 差异是否与描述相符？ 未作解释的文件——例如随功能一同提交的配置变更、新依赖或工作流编辑——是审查中最值得关注的信号。
3. 这项变更的暴露面有多大？ 对未认证请求路径的变更，值得投入的关注度应比内部重构高一个数量级。

分诊：决定审查重点

并非所有差异都值得花同样多的时间。根据代码块涉及的内容确定优先级：

优先级	信号
严重	身份验证／授权逻辑、会话处理、加密、在不可信边界解析输入、SQL／命令／模板构造、文件路径处理、反序列化、CI/CD 工作流、IAM／安全组配置、添加依赖项
高	新增端点或路由、新增工具／RPC 接口、安全路径上的错误处理、记录用户数据、租户范围限定、缓存键、速率限制
中	使用已验证输入的业务逻辑、移动安全相关代码的重构
低	测试、文档、格式调整、注释——但要阅读测试的删除部分

# Fast triage of a large diff
git diff $BASE...HEAD --name-only | rg 'auth|login|session|crypto|password|token|admin|permission|\.github/workflows|Dockerfile|terraform|policy'
git diff $BASE...HEAD -U10 | rg -n '^\+.*(eval\(|exec\(|system\(|innerHTML|pickle|yaml\.load|Sprintf.*SELECT|shell=True|verify=False|InsecureSkipVerify)'

审查重点

删除与弱化——先看减号行

最危险的差异往往是删除内容。请专门搜索被删除的行：

git diff $BASE...HEAD | rg '^-' | rg -i 'auth|verify|valid|check|sanitiz|escape|permission|assert|csrf|limit'

• 因为“多余”而删除验证调用
• 重构时移除授权装饰器
• 删除测试而不是修复测试
• 从未完成、如今却即将随代码发布的 TODO: add authz
• 移除超时、大小上限或速率限制
• 放宽 strict 模式、安全标头或 CSP 指令

新增内容——结合上下文审查常见问题类别

按照 auditing-code-for-vulnerabilities 中的漏洞类别排查方法进行审查，但只关注变更涉及的路径：注入接收点、新对象查找的授权检查、新出站调用中的 SSRF、新文件操作中的路径处理、反序列化、机密信息、弱随机性、不安全的默认值。

差异中最容易发现实际问题的两个问题：

新的不可信数据是否会到达旧的接收点？ 旧的不可信数据是否会到达新的接收点？

回答这两个问题都需要阅读差异之外的代码。

差异之外的上下文

阅读每个非简单代码块所在的整个函数，然后检查调用方：

# Who calls the changed function, and do they still satisfy its assumptions?
rg -n 'changedFunctionName\s*\(' --type <lang>
# What else in the codebase uses the pattern this change introduced?
git log -1 --format=%H $BASE  # anchor, then compare behaviour before/after

具体来说，要检查这项变更是否改变了其他代码所依赖的不变量：原本会清理数据、现在却不再清理的函数；含义发生反转的返回值；从可空变为非空的值；被取消的顺序保证。

顺带提交的配置、基础设施和流水线变更

git diff $BASE...HEAD -- '*.yml' '*.yaml' '*.tf' 'Dockerfile*' '.github/**' '*.json'

• 新增或升级的依赖项：由谁维护？锁文件变更是否与清单变更一致？（参见 auditing-supply-chain。）
• 扩大工作流权限、添加 pull_request_target、未固定版本的 Actions
• 开放安全组／防火墙规则、将存储桶设为公开、IAM "*"
• 调试标志、详细记录请求正文、禁用 TLS 验证
• 默认开启的功能标志

AI 生成内容和大规模机械性变更

机器编写的差异有一些典型问题：看似合理、实际上并未起约束作用的验证逻辑；吞掉安全相关错误的错误处理；悄无声息地无效执行的虚构 API 用法；以及应用在周围假设不同场景中的复制模式。对于大型生成式差异，应抽查安全相关代码块，并根据真实的库行为进行验证，而不是只看起来是否合理。

报告前先验证

差异审查比其他任何审查方式都更容易产生误报，因为缺失的上下文恰恰会让某个代码块看起来有问题。在提出问题前：

1. 阅读相关函数及其调用方。 你认为缺失的检查，往往就在上两层调用中。
2. 检查框架的行为。 许多关于“缺少转义”的问题，实际上已由模板引擎处理。
3. 确认所需调用方权限下是否可达。
4. 查找代码库其他位置是否存在相同模式。 如果该模式到处都有，且早于此 PR，那么这是代码库层面的问题，而不是 PR 的阻断项——请明确说明，并单独提交。

然后用具体的失败场景描述问题：输入、路径和结果。“这可能不安全”不是审查意见，而是让别人代你完成分析。

输出

区分哪些问题会阻止合并，哪些不会：

## Security review — PR #482

**Blocking**
1. `api/reports.go:214` — export handler drops the tenant predicate present on
   the list path. Any authenticated user can export another tenant's report by
   ID. Add `AND tenant_id = ?` to the lookup.

**Non-blocking, should fix**
2. `api/reports.go:190` — report IDs are sequential, making enumeration
   trivial once (1) is fixed. Consider opaque IDs.

**Notes / pre-existing**
3. The `findByID` pattern without scoping appears in 14 other handlers and
   predates this PR. Filed as ISSUE-991 rather than blocking here.

**Reviewed but clear**
- New `/webhooks/stripe` route: signature verified before body parse, replay
  window enforced. No finding.
- Dependency bump `lodash 4.17.20 → 4.17.21`: patch for CVE-2021-23337, no
  API change.

**Not covered**
- The React changes under `web/` were not reviewed for XSS in depth; the diff
  touches 40 components.

说明你审查了哪些内容，以及哪些内容没有问题，与指出问题同样有用——这能告诉下一位审查者哪些地方无需花时间，也能确保审查覆盖范围的说明属实。

应拒绝的托词

• “差异看起来没问题。” 你审查了新增行，也要阅读删除行和调用方。
• “CI 通过了。” CI 只运行针对他人预想到的行为所编写的测试。
• “这只是小改动。” 删除一个装饰器就是小改动。
• “作者是资深工程师。” 审查代码。
• “这只是一次重构。” 相比功能开发，重构更常将安全相关代码移过信任边界。
• “这是已有的问题。” 没错，也值得说明——但要确认它确实是已有问题，而不是想当然，并将其记录下来，而不是置之不理。
• “我得了解整个系统才能确定。” 那就说明你无法确定什么。坦诚说明不确定之处，是可供审查的陈述；保持沉默则会被视为认可。

参考资料

• auditing-code-for-vulnerabilities — 完整的审计方法和漏洞类别
• auditing-supply-chain — 差异中的依赖项和工作流变更
• securing-ai-systems — 审查 LLM/Agent 功能变更
• reporting-security-findings — 问题离开 PR 后的严重性评估和报告撰写