fix: セキュリティ強化(認証・SSRF防止・レート制限)#46
Conversation
- GET /api/rooms に認証チェック追加(未ログインで 401) - loadReferenceImage の HTTP/HTTPS フェッチを削除(SSRF防止) - src/lib/rate-limit.ts を新設(インメモリ Map ベース) - 画像生成エンドポイントに 5回/10分/ユーザー のレート制限を追加 - ユーザー作成エンドポイントに 20回/時間/IP のレート制限を追加 - VISION.md・LESSONS.md を新規追加、PLAN.md を更新 Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request introduces several security and architectural improvements, including authentication for room listings, SSRF prevention in image loading, and a new in-memory rate limiting utility. Documentation for the project's vision and development lessons was also added. Key feedback includes addressing a potential DoS vulnerability where users with unidentified IPs share a rate limit, implementing a cleanup mechanism for the rate limiter to prevent memory leaks, and replacing magic numbers with named constants for better maintainability.
| const ip = req.headers.get("x-forwarded-for") ?? "unknown"; | ||
| if (!checkRateLimit("user-create", ip, 20, 60 * 60 * 1000)) { | ||
| return NextResponse.json({ error: "Rate limit exceeded" }, { status: 429 }); | ||
| } |
There was a problem hiding this comment.
IPアドレスの取得で ?? "unknown" を使用すると、x-forwarded-for ヘッダーを持たないすべてのユーザーが同じ "unknown" というキーでレート制限を共有してしまいます。これにより、無関係なユーザーが互いのリクエストによってブロックされる可能性があり、小規模なDoS攻撃につながる恐れがあります。
IPが特定できない場合は、エラーを返すのがより安全です。
| const ip = req.headers.get("x-forwarded-for") ?? "unknown"; | |
| if (!checkRateLimit("user-create", ip, 20, 60 * 60 * 1000)) { | |
| return NextResponse.json({ error: "Rate limit exceeded" }, { status: 429 }); | |
| } | |
| const ip = req.headers.get("x-forwarded-for"); | |
| if (!ip) { | |
| return NextResponse.json({ error: "Could not determine IP address" }, { status: 400 }); | |
| } | |
| if (!checkRateLimit("user-create", ip, 20, 60 * 60 * 1000)) { | |
| return NextResponse.json({ error: "Rate limit exceeded" }, { status: 429 }); | |
| } |
| export function checkRateLimit( | ||
| limiterId: string, | ||
| key: string, | ||
| maxRequests: number, | ||
| windowMs: number | ||
| ): boolean { | ||
| if (!stores.has(limiterId)) stores.set(limiterId, new Map()); | ||
| const store = stores.get(limiterId)!; | ||
| const now = Date.now(); | ||
| const entry = store.get(key); | ||
|
|
||
| if (!entry || now > entry.resetAt) { | ||
| store.set(key, { count: 1, resetAt: now + windowMs }); | ||
| return true; | ||
| } | ||
| if (entry.count >= maxRequests) return false; | ||
| entry.count++; | ||
| return true; | ||
| } |
There was a problem hiding this comment.
このインメモリレートリミッターの実装は、エントリをクリーンアップする仕組みがないため、時間とともにメモリリークを引き起こす可能性があります。storesマップは、新しいキー(ユーザーIDやIPアドレス)が追加されるたびに増大し続けます。期限切れのエントリは、再度アクセスされるまでメモリから削除されません。
この問題を解決するために、期限切れのエントリを定期的に削除するクリーンアップ処理を実装することをお勧めします。例えば、setInterval を使用して、数分ごとに stores マップを走査し、now > entry.resetAt となったエントリを削除する方法が考えられます。
// クリーンアップ処理の例(別途実装が必要)
const CLEANUP_INTERVAL_MS = 5 * 60 * 1000; // 5分ごと
setInterval(() => {
const now = Date.now();
for (const store of stores.values()) {
for (const [key, entry] of store.entries()) {
if (now > entry.resetAt) {
store.delete(key);
}
}
}
}, CLEANUP_INTERVAL_MS);| if (!checkRateLimit("image-gen", userId, 5, 10 * 60 * 1000)) { | ||
| return NextResponse.json({ error: "Rate limit exceeded" }, { status: 429 }); | ||
| } |
There was a problem hiding this comment.
レートリミットのパラメータにマジックナンバー(5 と 10 * 60 * 1000)が直接使用されています。可読性とメンテナンス性を向上させるため、これらの値をファイルの上部や別の設定ファイルで名前付き定数として定義することを推奨します。
例:
const IMAGE_GEN_RATE_LIMIT_MAX_REQUESTS = 5;
const IMAGE_GEN_RATE_LIMIT_WINDOW_MS = 10 * 60 * 1000; // 10 minutes
// ...
if (!checkRateLimit("image-gen", userId, IMAGE_GEN_RATE_LIMIT_MAX_REQUESTS, IMAGE_GEN_RATE_LIMIT_WINDOW_MS)) {
// ...
}|
|
||
| export async function POST(req: NextRequest) { | ||
| const ip = req.headers.get("x-forwarded-for") ?? "unknown"; | ||
| if (!checkRateLimit("user-create", ip, 20, 60 * 60 * 1000)) { |
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 169170c626
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const ip = req.headers.get("x-forwarded-for") ?? "unknown"; | ||
| if (!checkRateLimit("user-create", ip, 20, 60 * 60 * 1000)) { |
There was a problem hiding this comment.
Use trusted client identity for rate-limit key
POST /api/users uses the raw x-forwarded-for header as the limiter key, which is attacker-controllable in common setups (for example direct Node deployments or proxies that append to an existing header). In those environments, sending a different header value per request bypasses the new 20/hour limit completely. Use a trusted client address source (or strictly parse/validate trusted hops) before calling checkRateLimit.
Useful? React with 👍 / 👎.
| if (!entry || now > entry.resetAt) { | ||
| store.set(key, { count: 1, resetAt: now + windowMs }); | ||
| return true; |
There was a problem hiding this comment.
Prune stale limiter buckets to bound memory growth
The new in-memory limiter never evicts old keys from stores; it only overwrites entries when the same key is seen again. With endpoints that accept arbitrary caller keys (notably /api/users), repeated requests using unique keys will make this map grow indefinitely even after windows expire, which can become a memory-exhaustion path over time. Add expiration cleanup (sweep or bounded cache) so stale buckets are removed.
Useful? React with 👍 / 👎.
- run/route.ts: rate limit 超過時に expansion を FAILED にしてロックを解放してから 429 を返す (QUEUED のまま残るとセルが UI でブロックされ続ける問題を修正) - users/route.ts: x-forwarded-for が取れない環境 (ip=unknown) ではレート制限をスキップ (プロキシなし環境で全ユーザーが同一キーになりグローバルブロックが起きる問題を修正) Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
- rate-limit.ts: setInterval で期限切れエントリを5分ごとに削除(メモリリーク防止) - run/route.ts: IMAGE_GEN_RATE_LIMIT_MAX / WINDOW_MS を定数化 - users/route.ts: USER_CREATE_RATE_LIMIT_MAX / WINDOW_MS を定数化 スキップした指摘: - Gemini「IP不明時は400返却」: ローカル開発環境でサインアップが全失敗するため不採用 - Codex P1「x-forwarded-for attacker-controllable」: インフラレベルの問題のため現状維持 Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Summary
GET /api/roomsに認証チェックがなく未ログインユーザーが全ルーム一覧を取得できた問題を修正loadReferenceImage()の HTTP/HTTPS フェッチを削除(SSRF防止)。tile.imageUrlは常にローカルパスのみsrc/lib/rate-limit.tsを新設し、画像生成(5回/10分/ユーザー)・ユーザー作成(20回/時間/IP)にレート制限を追加対応しなかった項目
laxはリンク遷移UXを維持する適切な選択のため変更なしTest plan
npx tsc --noEmit通過npm run test全テスト通過GET /api/rooms→ 401/api/expansions/:id/runを 6 回連続呼び出し → 6 回目が 429🤖 Generated with Claude Code