code-review-security

コードレビューのセキュリティ

使用するタイミング

次のような場合に、このスキルを有効にしてください：

• プルリクエストのセキュリティ脆弱性をレビューするとき
• 認証または認可コードの変更を監査するとき
• ユーザー入力、ファイルアップロード、外部データを扱うコードをレビューするとき
• 新機能に OWASP Top 10 の脆弱性がないか確認するとき
• シークレットがリポジトリにコミットされていないことを検証するとき
• 依存関係に既知の脆弱性がないかスキャンするとき
• 機密データを公開する API エンドポイントをレビューするとき

出力： 重大度、ファイル名と行番号、説明、推奨事項を記載した調査結果を security-review.md に書き込んでください。

次の用途ではこのスキルを使用しないでください：

• デプロイインフラストラクチャのセキュリティ（docker-best-practices を使用してください）
• インシデント対応の手順（incident-response を使用してください）
• セキュリティに焦点を当てない一般的なコード品質レビュー（pre-merge-checklist を使用してください）
• 実装コードの作成（python-backend-expert または react-frontend-expert を使用してください）

手順

OWASP Top 10 チェックリスト

すべてのプルリクエストを OWASP Top 10（2021 年版）に照らしてレビューしてください。以下の各カテゴリには、Python/FastAPI および React のコードベース向けの具体的な確認項目が含まれています。

---

A01：アクセス制御の不備

確認する項目：

• エンドポイントに認可チェックがない
• 所有権を検証せずにオブジェクトを直接参照している
• ロールベースのフィルタリングなしでデータを公開するエンドポイント
• 新しいルートに認証用の Depends() がない

Python/FastAPI の確認項目：

# BAD: No authorization check -- any authenticated user can access any user
@router.get("/users/{user_id}")
async def get_user(user_id: int, db: Session = Depends(get_db)):
    return await user_repo.get(user_id)

# GOOD: Verify the requesting user owns the resource or is admin
@router.get("/users/{user_id}")
async def get_user(
    user_id: int,
    current_user: User = Depends(get_current_user),
    db: Session = Depends(get_db),
):
    if current_user.id != user_id and current_user.role != "admin":
        raise HTTPException(status_code=403, detail="Forbidden")
    return await user_repo.get(user_id)

レビュー用チェックリスト：

• すべてのルートに認証がある（Depends(get_current_user)）
• リソースへのアクセスが、リクエストしたユーザーに照らして検証されている
• 管理者専用のエンドポイントで role == "admin" が確認されている
• リストエンドポイントがユーザーの所有権でフィルタリングされている（管理者を除く）
• IDOR（安全でない直接オブジェクト参照）の脆弱性がない

---

A02：暗号化の失敗

確認する項目：

• パスワードが平文または脆弱なハッシュ方式で保存されている
• ログやエラーメッセージに機密データが含まれている
• シークレット、API キー、トークンがハードコードされている
• JWT の設定が脆弱である

Python の確認項目：

# BAD: Weak password hashing
import hashlib
password_hash = hashlib.md5(password.encode()).hexdigest()

# GOOD: Use bcrypt via passlib
from passlib.context import CryptContext
pwd_context = CryptContext(schemes=["bcrypt"], deprecated="auto")
password_hash = pwd_context.hash(password)

# BAD: Secret in code
SECRET_KEY = "my-super-secret-key-123"

# GOOD: Secret from environment
SECRET_KEY = os.environ["SECRET_KEY"]

レビュー用チェックリスト：

• パスワードが bcrypt でハッシュ化されている（MD5、SHA1、平文は使用しない）
• JWT シークレットがハードコードされず、環境変数から読み込まれている
• 機密データ（パスワード、トークン、PII）がログから除外されている
• すべての外部通信で HTTPS が強制されている
• ソースコードにシークレットがない（.env.example にプレースホルダーのみが記載されていることを確認）

---

A03：インジェクション

確認する項目：

• 文字列補間を使った生の SQL クエリ
• ユーザー入力を使った eval()、exec()、compile()
• shell=True を指定した subprocess 呼び出し
• テンプレートインジェクション

Python の確認項目：

# BAD: SQL injection via string formatting
query = f"SELECT * FROM users WHERE email = '{email}'"
db.execute(text(query))

# GOOD: Parameterized query
db.execute(text("SELECT * FROM users WHERE email = :email"), {"email": email})

# GOOD: SQLAlchemy ORM (always parameterized)
user = db.query(User).filter(User.email == email).first()

# BAD: Command injection
subprocess.run(f"convert {filename}", shell=True)

# GOOD: Pass arguments as a list
subprocess.run(["convert", filename], shell=False)

# BAD: Code execution with user input
result = eval(user_input)

# GOOD: Never eval user input. Use ast.literal_eval for safe parsing.
result = ast.literal_eval(user_input)  # Only for literal structures

レビュー用チェックリスト：

• 文字列補間を使った生の SQL がない（ORM またはパラメーター化クエリを使用する）
• 外部入力を使った eval()、exec()、compile() がない
• 動的な引数を使った subprocess.run(..., shell=True) がない
• 信頼できないデータに対する pickle.loads() がない
• すべてのユーザー入力が、使用前に Pydantic スキーマで検証されている

---

A04：安全でない設計

確認する項目：

• 認証エンドポイントにレート制限がない
• ログイン失敗後のアカウントロックがない
• 公開フォームに CAPTCHA がない
• ビジネスロジックの不備（例：負の金額、ユーザー自身による権限昇格）

レビュー用チェックリスト：

• ログイン、登録、パスワードリセットにレート制限がある
• 5 回以上ログインに失敗した後、アカウントロックまたは指数バックオフが行われる
• ビジネスロジックで制約（正の金額、有効な状態遷移）が検証される
• 機密性の高い操作に再認証が必要である

---

A05：セキュリティ設定ミス

確認する項目：

• 本番環境でデバッグモードが有効になっている
• CORS でワイルドカード * のオリジンが設定されている
• デフォルトの認証情報または管理者アカウントが存在する
• スタックトレースを公開する詳細なエラーメッセージ

Python/FastAPI の確認項目：

# BAD: Wide-open CORS
app.add_middleware(CORSMiddleware, allow_origins=["*"])

# GOOD: Explicit allowed origins
app.add_middleware(
    CORSMiddleware,
    allow_origins=["https://app.example.com"],
    allow_methods=["GET", "POST", "PUT", "DELETE"],
    allow_headers=["Authorization", "Content-Type"],
)

# BAD: Debug mode in production
app = FastAPI(debug=True)

# GOOD: Debug only in development
app = FastAPI(debug=settings.DEBUG)  # DEBUG=False in production

レビュー用チェックリスト：

• CORS のオリジンが明示的に指定されている（本番環境ではワイルドカードを使用しない）
• 本番環境の設定でデバッグモードが無効になっている
• エラーレスポンスにスタックトレースや内部の詳細が含まれていない
• デフォルトの管理者認証情報が変更または削除されている
• セキュリティヘッダーが設定されている（X-Content-Type-Options、X-Frame-Options など）

---

A06：脆弱で古いコンポーネント

レビュー用チェックリスト：

• Python の依存関係に既知の CVE がない（pip-audit または safety check）
• npm の依存関係に既知の CVE がない（npm audit）
• ロックファイルで依存関係のバージョンが固定されている
• 非推奨のパッケージが引き続き使用されていない

---

A07：識別および認証の不備

確認する項目：

• 弱いパスワードポリシー
• 有効期限のないセッショントークン
• 管理者の操作で多要素認証が設定されていない
• 有効期限のない JWT トークン

Python の確認項目：

# BAD: JWT without expiration
token = jwt.encode({"sub": user_id}, SECRET_KEY, algorithm="HS256")

# GOOD: JWT with expiration
token = jwt.encode(
    {"sub": user_id, "exp": datetime.utcnow() + timedelta(minutes=30)},
    SECRET_KEY,
    algorithm="HS256",
)

レビュー用チェックリスト：

• JWT トークンに有効期限（exp クレーム）が設定されている
• リフレッシュトークンが安全に保存され、失効できる
• パスワードポリシーで最小文字数（12 文字以上）と複雑さが求められている
• パスワード変更時またはログアウト時にセッションが無効化される
• ログインエラーメッセージからユーザーを列挙できない

---

A08：ソフトウェアおよびデータの完全性の不備

レビュー用チェックリスト：

• CI/CD パイプラインでアーティファクトの完全性が検証される
• 署名または検証のないパッケージがない
• 信頼できないデータのデシリアライズに安全な方法が使われている（pickle.loads を使用しない）
• データベースのマイグレーションが実行前にレビューされる

---

A09：セキュリティログおよび監視の不備

レビュー用チェックリスト：

• 認証イベント（ログイン、ログアウト、失敗した試行）が記録される
• 認可の失敗がコンテキストとともに記録される
• 機密データ（パスワード、トークン、PII）がログに含まれていない
• ログエントリにタイムスタンプ、ユーザー ID、IP アドレス、操作が含まれている
• 不審なパターン（総当たり攻撃、通常と異なるアクセス）に対するアラートが設定されている

---

A10：サーバーサイドリクエストフォージェリ（SSRF）

確認する項目：

• ユーザーが指定した URL がサーバーサイドリクエストに使用されている
• 任意の URL を受け付けるリダイレクトエンドポイント

Python の確認項目：

# BAD: Fetch arbitrary URL from user input
url = request.query_params["url"]
response = httpx.get(url)  # SSRF: can access internal services

# GOOD: Validate URL against allowlist
ALLOWED_HOSTS = {"api.example.com", "cdn.example.com"}
parsed = urlparse(url)
if parsed.hostname not in ALLOWED_HOSTS:
    raise HTTPException(400, "URL not allowed")
response = httpx.get(url)

レビュー用チェックリスト：

• 検証せずにユーザーが制御する URL へサーバーサイドリクエストを送信していない
• 外部連携に URL の許可リストが使用されている
• エラーメッセージで内部サービスの URL が公開されていない

---

Python 固有のセキュリティチェック

OWASP に加えて、Python コードで次のパターンを確認してください。

パターン	リスク	対策
eval(user_input)	リモートコード実行	削除するか、ast.literal_eval を使用する
pickle.loads(data)	任意のコード実行	JSON または msgpack を使用する
subprocess.run(cmd, shell=True)	コマンドインジェクション	引数をリストで渡し、shell=False にする
yaml.load(data)	コード実行	yaml.safe_load(data) を使用する
os.system(cmd)	コマンドインジェクション	subprocess.run([...]) を使用する
生の SQL 文字列	SQL インジェクション	ORM またはパラメーター化クエリを使用する
hashlib.md5(password)	弱いハッシュ化	passlib 経由で bcrypt を使用する
jwt.decode(token, options={"verify_signature": False})	認証の回避	常に署名を検証する
open(user_path)	パストラバーサル	パスを検証し、pathlib.resolve() を使用する
tempfile.mktemp()	競合状態	tempfile.mkstemp() を使用する

React 固有のセキュリティチェック

パターン	リスク	修正方法
dangerouslySetInnerHTML	XSS	テキストコンテンツを使用するか、DOMPurify でサニタイズします
href 内の javascript:	XSS	URL を検証し、https: のみを許可します
window.location = userInput	オープンリダイレクト	許可リストと照合して検証します
localStorage へのトークンの保存	XSS によるトークン窃取	httpOnly Cookie を使用します
データに基づくインラインイベントハンドラー	XSS	React のイベントハンドラーを使用します
eval() または Function()	コード実行	完全に削除します
ユーザーの HTML のレンダリング	XSS	サニタイズライブラリを使用します

React コードレビュー：

// BAD: XSS via dangerouslySetInnerHTML
<div dangerouslySetInnerHTML={{ __html: userBio }} />

// GOOD: Sanitize first, or use text content
import DOMPurify from "dompurify";
<div dangerouslySetInnerHTML={{ __html: DOMPurify.sanitize(userBio) }} />

// BETTER: Use text content when HTML is not needed
<p>{userBio}</p>

// BAD: javascript: URL
<a href={userLink}>Click</a>  // userLink could be "javascript:alert(1)"

// GOOD: Validate protocol
const safeHref = /^https?:\/\//.test(userLink) ? userLink : "#";
<a href={safeHref}>Click</a>

重大度の分類

優先順位を付けるため、検出事項を重大度で分類します。

重大度	説明	例	SLA
Critical	リモートから悪用可能で、認証が不要なデータ侵害	SQL インジェクション、RCE、認証の回避	マージをブロックし、直ちに修正
High	認証済みで悪用可能な権限昇格	IDOR、アクセス制御の不備、XSS（格納型）	マージをブロックし、リリース前に修正
Medium	悪用には特定の条件が必要	CSRF、XSS（反射型）、オープンリダイレクト	スプリント内に修正
Low	多層防御、情報提供	ヘッダーの欠落、詳細すぎるエラー	都合のよいときに修正
Info	ベストプラクティスの推奨事項	依存関係の更新、コードスタイル	バックログで追跡

検出事項の報告形式

セキュリティ上の検出事項を報告する際は、一貫性を保つために次の形式を使用します。

## Security Finding: [Title]

**Severity:** Critical | High | Medium | Low | Info
**Category:** OWASP A01-A10 or custom category
**File:** path/to/file.py:42
**CWE:** CWE-89 (if applicable)

### Description
Brief description of the vulnerability and its impact.

### Vulnerable Code
```python
# The problematic code
vulnerable_function(user_input)

推奨される修正方法

# The secure alternative
safe_function(sanitize(user_input))

影響

攻撃者がこの脆弱性を悪用して実行できること。

参考資料

• 関連する OWASP ページへのリンク
• 関連する CWE エントリへのリンク

### Automated Scanning

Use `scripts/security-scan.py` to perform AST-based scanning for common vulnerability patterns in Python code. The script scans for:
- `eval()` / `exec()` / `compile()` calls
- `subprocess` with `shell=True`
- `pickle.loads()` on potentially untrusted data
- Raw SQL string construction
- `yaml.load()` without `Loader=SafeLoader`
- Hardcoded secret patterns (API keys, passwords)
- Weak hash functions (MD5, SHA1 for passwords)

Run: `python scripts/security-scan.py --path ./app --output-dir ./security-results`

**Dependency scanning (run separately):**
```bash
# Python dependencies
pip-audit --requirement requirements.txt --output json > dep-audit.json

# npm dependencies
npm audit --json > npm-audit.json

例

レビューコメントの例（Critical）

セキュリティ：SQL インジェクション（Critical、OWASP A03）

ファイル：app/repositories/user_repository.py:47

query = f"SELECT * FROM users WHERE name LIKE '%{search_term}%'"

これは文字列補間を使って生の SQL クエリを構築しているため、SQL インジェクションが可能です。 攻撃者は '; DROP TABLE users; -- を入力して、データを破壊できます。

修正方法： SQLAlchemy ORM のフィルタリングを使用します。

users = db.query(User).filter(User.name.ilike(f"%{search_term}%")).all()

レビューコメントの例（Medium）

セキュリティ：レート制限の欠落（Medium、OWASP A04）

ファイル：app/routes/auth.py:12

/auth/login エンドポイントにはレート制限がありません。攻撃者は無制限の速度で パスワードへの総当たり攻撃を実行できます。

修正方法： レート制限ミドルウェアを追加します。

from slowapi import Limiter
limiter = Limiter(key_func=get_remote_address)

@router.post("/login")
@limiter.limit("5/minute")
async def login(request: Request, ...):

出力ファイル

セキュリティ上の検出事項を security-review.md に書き込みます。

# Security Review: [Feature/PR Name]

## Summary
- Critical: 0 | High: 1 | Medium: 2 | Low: 1

## Findings

### [CRITICAL] SQL Injection in user search
- **File:** app/routes/users.py:45
- **OWASP:** A03 Injection
- **Description:** Raw SQL with string interpolation
- **Recommendation:** Use SQLAlchemy ORM filtering

### [HIGH] Missing authorization check
...

## Passed Checks
- No hardcoded secrets found
- Dependencies up to date