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

コードを読む前に、次の3点を確認してください。

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、新しいファイル操作でのパス処理、デシリアライズ、シークレット、弱い乱数、安全でないデフォルト設定です。

差分から実際の問題の大半を見つけるには、次の2つの質問が役立ちます。

新しい信頼できないデータが、既存のシンクに到達していませんか？ 既存の信頼できないデータが、新しいシンクに到達していませんか？

どちらを確認するにも、差分の外側を読む必要があります。

差分の外側にある文脈

重要なハンクごとに関数全体を読んでから、呼び出し元を確認してください。

# 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

特に、他のコードが依存する不変条件が変更されていないか確認してください。以前はサニタイズしていたのに今はしなくなった関数、意味が反転した戻り値、null 許容から非 null に変わった値、失われた順序保証などです。

付随する設定、インフラ、パイプラインの変更

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

• 新規または更新された依存関係：誰が保守しているか、ロックファイルの変更はマニフェストの変更と一致しているか？（auditing-supply-chain を参照してください。）
• ワークフローの権限拡大、pull_request_target の追加、バージョンが固定されていないアクション
• 開放されたセキュリティグループ／ファイアウォールルール、公開設定されたバケット、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／エージェント機能の変更をレビューする方法
• reporting-security-findings — 指摘が PR の外に持ち越される場合の重大度と報告方法