fix : SSRF IPv6 handling, profile avatar SSRF validation, analysis job null check, and webhook signature-first rate limiting#2467
Open
tmdeveloper007 wants to merge 3 commits into
Conversation
…b null check, and webhook signature-first rate limiting - lib/utils/ssrfValidator.ts: Handle IPv6-mapped IPv4 addresses (::ffff:x.x.x.x) to prevent SSRF bypass via IPv6 notation - app/api/users/profile/route.ts: Add SSRF validation for HTTP avatar URLs to prevent probing internal services - lib/services/analysisJobService.ts: Use COALESCE(next_run_at, NOW()) to handle NULL next_run_at in job claiming SQL - app/api/integrations/github/webhook/route.ts: Move signature verification before rate limiting to prevent DoS; return 503 when DLQ write fails
|
@tmdeveloper007 is attempting to deploy a commit to the Nisshchaya's projects Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
|
Warning Review limit reached
More reviews will be available in 45 minutes and 45 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses rolling per-developer review limits. Reviews become available again as older review attempts age out of the rolling limit window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR reorders GitHub webhook signature verification to occur before rate limiting and returns ChangesSecurity Hardening and Bug Fixes
Sequence Diagram(s)sequenceDiagram
participant GitHub
participant WebhookRoute
participant GithubWebhookVerifier
participant RateLimiter
participant DLQ
GitHub->>WebhookRoute: POST webhook event
WebhookRoute->>WebhookRoute: Read raw body
WebhookRoute->>GithubWebhookVerifier: Verify HMAC signature
alt Signature invalid
GithubWebhookVerifier-->>WebhookRoute: failure
WebhookRoute-->>GitHub: 401 Unauthorized
else Signature valid
GithubWebhookVerifier-->>WebhookRoute: ok
WebhookRoute->>RateLimiter: checkRateLimit
alt Rate limit fallback fails and DLQ write fails
WebhookRoute->>DLQ: Persist to DLQ
DLQ-->>WebhookRoute: error
WebhookRoute-->>GitHub: 503 (retry)
else Rate limit exceeded
WebhookRoute-->>GitHub: 429 rateLimitResponse
else Unsupported event type
WebhookRoute-->>GitHub: 200 Event type not handled
else Supported event
WebhookRoute-->>GitHub: 202 Accepted
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Contributor
Author
|
CodeRabbit review: approved. Note: Vercel CI check is failing due to "Authorization required to deploy" - this is a Vercel account permission issue with the fork (not a code problem). CodeRabbit code review has passed. The code changes are ready to merge once Vercel authorization is configured for tmdeveloper007/gitverse-nextjs. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/api/integrations/github/webhook/route.ts (1)
69-80: 🩺 Stability & Availability | 🟠 MajorAdd a payload-size guard before reading the raw body.
Line 69 buffers the entire request without any size limit before signature verification. GitHub webhooks have a 25 MB hard limit, and an oversized payload can still consume memory and CPU for HMAC validation before rejection. Add a pre-read size check on the
content-lengthheader and a post-read guard, matching the pattern used throughout the codebase and GitHub's own constraint.🛡️ Suggested guard
+const MAX_GITHUB_WEBHOOK_BODY_BYTES = 25 * 1024 * 1024; + export async function POST(request: NextRequest) { + const contentLength = request.headers.get("content-length"); + if (contentLength) { + const bytes = Number(contentLength); + if (!Number.isFinite(bytes) || bytes > MAX_GITHUB_WEBHOOK_BODY_BYTES) { + return NextResponse.json({ error: "Payload too large" }, { status: 413 }); + } + } + const rawBody = await request.text(); + if (Buffer.byteLength(rawBody, "utf8") > MAX_GITHUB_WEBHOOK_BODY_BYTES) { + return NextResponse.json({ error: "Payload too large" }, { status: 413 }); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/api/integrations/github/webhook/route.ts` around lines 69 - 80, The rawBody assignment at line 69 reads the entire request body without any size validation, which can lead to memory exhaustion on oversized payloads before signature verification occurs. Add a pre-read size guard by checking the content-length header from request.headers.get("content-length") against GitHub's 25 MB limit and reject requests that exceed this threshold before calling request.text(). Additionally, implement a post-read guard that validates the actual size of rawBody after it is read to catch cases where content-length is missing or invalid. This ensures oversized payloads are rejected efficiently without consuming resources for HMAC validation.
🧹 Nitpick comments (1)
lib/services/analysisJobService.ts (1)
431-431: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPrefer a sargable NULL-aware predicate for
next_run_at.Line 431 is functionally correct, but
COALESCE(a1.next_run_at, NOW()) <= NOW()may block efficient use of a normal index onnext_run_atin this claim hot path. Prefer:
(a1.next_run_at IS NULL OR a1.next_run_at <= NOW()).Suggested SQL change
- WHERE COALESCE(a1.next_run_at, NOW()) <= NOW() + WHERE (a1.next_run_at IS NULL OR a1.next_run_at <= NOW())🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/services/analysisJobService.ts` at line 431, Replace the non-sargable COALESCE predicate in the WHERE clause at line 431 of the analysisJobService.ts file. Change `COALESCE(a1.next_run_at, NOW()) <= NOW()` to `(a1.next_run_at IS NULL OR a1.next_run_at <= NOW())` to allow the database query optimizer to efficiently use an index on the next_run_at column, improving query performance in this hot code path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/api/integrations/github/webhook/route.ts`:
- Around line 91-126: The rate limiting check via checkRateLimit is currently
applied to all valid GitHub webhook deliveries before filtering for supported
event types. This means unsupported events like label, milestone, and ping
consume rate limit quota and can exhaust RATE_LIMITS.GITHUB_WEBHOOK, causing
legitimate pull_request, issues, and push events to be rate-limited with 429
responses. Move the event type validation (the check that returns 200 if the
event is not pull_request, issues, or push) to execute before the checkRateLimit
call. This ensures unsupported events are filtered out without consuming any
rate limit quota.
In `@app/api/users/profile/route.ts`:
- Around line 377-382: The validateSafeUrl call for the avatar URL validation is
occurring after database mutations (deleteMany calls for Google account/session
management) have already been committed, which means a failed avatar validation
can leave the account in a partially corrupted state. Move the avatar validation
using validateSafeUrl to the beginning of the profile update logic, before any
database mutations are executed, to ensure all validations pass before any data
changes are persisted.
In `@lib/utils/ssrfValidator.ts`:
- Around line 21-40: The ipv6MappedMatch regex in the ssrfValidator function
only matches the compressed dotted-decimal format of IPv6-mapped IPv4 addresses
(::ffff:a.b.c.d), but other equivalent representations like
0:0:0:0:0:ffff:a.b.c.d or ::ffff:7f00:1 bypass this check. Refactor the code to
normalize or parse all IPv6-mapped IPv4 address formats first to extract the
underlying IPv4 address, then apply the existing private-range checking logic
(the range checks from lines 31-37) to the extracted IPv4 address instead of
only matching one specific IPv6 format. This ensures all equivalent IPv6-mapped
representations are properly validated against the private IP ranges.
---
Outside diff comments:
In `@app/api/integrations/github/webhook/route.ts`:
- Around line 69-80: The rawBody assignment at line 69 reads the entire request
body without any size validation, which can lead to memory exhaustion on
oversized payloads before signature verification occurs. Add a pre-read size
guard by checking the content-length header from
request.headers.get("content-length") against GitHub's 25 MB limit and reject
requests that exceed this threshold before calling request.text(). Additionally,
implement a post-read guard that validates the actual size of rawBody after it
is read to catch cases where content-length is missing or invalid. This ensures
oversized payloads are rejected efficiently without consuming resources for HMAC
validation.
---
Nitpick comments:
In `@lib/services/analysisJobService.ts`:
- Line 431: Replace the non-sargable COALESCE predicate in the WHERE clause at
line 431 of the analysisJobService.ts file. Change `COALESCE(a1.next_run_at,
NOW()) <= NOW()` to `(a1.next_run_at IS NULL OR a1.next_run_at <= NOW())` to
allow the database query optimizer to efficiently use an index on the
next_run_at column, improving query performance in this hot code path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6ae0eed9-c50c-44bc-af93-ecbd546747c6
📒 Files selected for processing (5)
app/api/integrations/github/webhook/route.tsapp/api/users/profile/route.tslib/services/analysisJobService.tslib/utils/ssrfValidator.tssrc/components/repository/RepositoryInsights.tsx
Comment on lines
+91
to
+126
| * Step 2: Rate limiting | ||
| * Apply per-IP rate limits after signature is validated. | ||
| */ | ||
| const deliveryId = request.headers.get("x-github-delivery") || ""; | ||
|
|
||
| if (event !== "pull_request" && event !== "issues" && event !== "push") { | ||
| return NextResponse.json( | ||
| { ok: true, ignored: true, event }, | ||
| { status: 200 }, | ||
| ); | ||
| } | ||
|
|
||
| let payload: WebhookPayload; | ||
| try { | ||
| payload = JSON.parse(rawBody); | ||
| } catch { | ||
| return NextResponse.json({ error: "Invalid JSON" }, { status: 400 }); | ||
| } | ||
| const ip = getClientIp(request); | ||
| const rl = await checkRateLimit(ip, RATE_LIMITS.GITHUB_WEBHOOK); | ||
|
|
||
| const action = payload.action; | ||
|
|
||
| if (event === "pull_request") { | ||
| if (!shouldHandlePullRequestAction(action)) { | ||
| return NextResponse.json( | ||
| { ok: true, ignored: true, action }, | ||
| { status: 200 }, | ||
| ); | ||
| } | ||
| if (payload.pull_request?.draft && action !== "ready_for_review") { | ||
| return NextResponse.json( | ||
| { ok: true, ignored: true, reason: "draft" }, | ||
| { status: 200 }, | ||
| ); | ||
| } | ||
| } else if (event === "issues") { | ||
| if (!shouldHandleIssueAction(action)) { | ||
| return NextResponse.json( | ||
| { ok: true, ignored: true, action }, | ||
| { status: 200 }, | ||
| ); | ||
| if (rl.fallbackFailed) { | ||
| console.error("[WebhookRoute] Rate limiters completely failed. DLQing webhook."); | ||
| try { | ||
| await prisma.webhookEvent.create({ | ||
| data: { | ||
| event: event || "unknown", | ||
| payload: rawBody, | ||
| status: "dlq", | ||
| error: "Rate limiter and fallback completely failed", | ||
| }, | ||
| }); | ||
| } catch (e) { | ||
| console.error("[WebhookRoute] Failed to write to DLQ!", e); | ||
| // Return 503 so GitHub retries the webhook delivery. | ||
| return NextResponse.json({ error: "Webhook processing failed. Please retry." }, { status: 503 }); | ||
| } | ||
| } else if (event === "push") { | ||
| // We accept all push events — no action filtering needed | ||
| return NextResponse.json({ ok: true, message: "Webhook accepted and queued to DLQ due to severe outages" }, { status: 202 }); | ||
| } | ||
|
|
||
| /* | ||
| * ┌──────────────────────────────────────────────────────────┐ | ||
| * │ 4. Bot filtering │ | ||
| * │ Ignore events sent by GitHub bots (including our own │ | ||
| * │ automation) to prevent feedback loops. │ | ||
| * └──────────────────────────────────────────────────────────┘ | ||
| */ | ||
| if (payload.sender?.type === "Bot") { | ||
| return NextResponse.json( | ||
| { ok: true, ignored: true, reason: "bot" }, | ||
| { status: 200 }, | ||
| ); | ||
| } | ||
| if (!rl.allowed) return rateLimitResponse(rl, "Webhook rate limit exceeded"); | ||
|
|
||
| /* | ||
| * ┌──────────────────────────────────────────────────────────┐ | ||
| * │ 5. Field validation │ | ||
| * │ Ensure the payload contains the minimum required │ | ||
| * │ fields before proceeding to idempotency and enqueue. │ | ||
| * └──────────────────────────────────────────────────────────┘ | ||
| * Step 3: Event routing — only process events we handle. | ||
| * Unsupported event types (label, milestone, etc.) and non-material PR/issue | ||
| * actions get a silent 200. | ||
| */ | ||
| const owner = payload.repository?.owner?.login; | ||
| const repo = payload.repository?.name; | ||
| const number = payload.pull_request?.number || payload.issue?.number; | ||
| const installationId = payload.installation?.id; | ||
|
|
||
| if (!owner || !repo || (!number && event !== "push") || !installationId) { | ||
| return NextResponse.json( | ||
| { | ||
| error: "Missing required fields", | ||
| details: { owner, repo, number, installationId, event }, | ||
| }, | ||
| { status: 400 }, | ||
| ); | ||
| } | ||
| const deliveryId = request.headers.get("x-github-delivery") || ""; | ||
|
|
||
| /* | ||
| * ┌──────────────────────────────────────────────────────────┐ | ||
| * │ 6. Redis-based idempotency │ | ||
| * │ Atomically claim the deliveryId so concurrent │ | ||
| * │ deliveries of the same webhook are deduplicated. │ | ||
| * │ The lock is released if the enqueue fails. │ | ||
| * └──────────────────────────────────────────────────────────┘ | ||
| */ | ||
| let idempotencyKey: string | null = null; | ||
| if (deliveryId) { | ||
| idempotencyKey = generateWebhookKey(deliveryId, event || "unknown", action); | ||
| const acquired = await tryAcquireIdempotency(idempotencyKey); | ||
| if (!acquired) { | ||
| return NextResponse.json( | ||
| { ok: true, ignored: true, reason: "duplicate_delivery" }, | ||
| { status: 200 }, | ||
| ); | ||
| } | ||
| if (event !== "pull_request" && event !== "issues" && event !== "push") { | ||
| return NextResponse.json({ ok: true, message: "Event type not handled" }, { status: 200 }); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Filter unsupported signed events before consuming webhook quota.
Lines 94-96 rate-limit every valid GitHub delivery, but Lines 125-126 later discard unsupported event types. If the webhook receives signed label, milestone, ping, or other ignored events, they can exhaust RATE_LIMITS.GITHUB_WEBHOOK and cause handled PR/issues/push deliveries to get 429s.
🔁 Suggested reorder
/*
- * Step 2: Rate limiting
- * Apply per-IP rate limits after signature is validated.
+ * Step 2: Event routing — only process events we handle.
+ * Unsupported event types (label, milestone, etc.) get a silent 200
+ * without consuming processing quota.
*/
+ if (event !== "pull_request" && event !== "issues" && event !== "push") {
+ return NextResponse.json({ ok: true, message: "Event type not handled" }, { status: 200 });
+ }
+
+ /*
+ * Step 3: Rate limiting
+ * Apply per-IP rate limits after signature is validated and event is relevant.
+ */
const ip = getClientIp(request);
const rl = await checkRateLimit(ip, RATE_LIMITS.GITHUB_WEBHOOK);
@@
if (!rl.allowed) return rateLimitResponse(rl, "Webhook rate limit exceeded");
/*
- * Step 3: Event routing — only process events we handle.
- * Unsupported event types (label, milestone, etc.) and non-material PR/issue
- * actions get a silent 200.
+ * Step 4: Event processing
+ * Non-material PR/issue actions get a silent 200.
*/
const deliveryId = request.headers.get("x-github-delivery") || "";
-
- if (event !== "pull_request" && event !== "issues" && event !== "push") {
- return NextResponse.json({ ok: true, message: "Event type not handled" }, { status: 200 });
- }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| * Step 2: Rate limiting | |
| * Apply per-IP rate limits after signature is validated. | |
| */ | |
| const deliveryId = request.headers.get("x-github-delivery") || ""; | |
| if (event !== "pull_request" && event !== "issues" && event !== "push") { | |
| return NextResponse.json( | |
| { ok: true, ignored: true, event }, | |
| { status: 200 }, | |
| ); | |
| } | |
| let payload: WebhookPayload; | |
| try { | |
| payload = JSON.parse(rawBody); | |
| } catch { | |
| return NextResponse.json({ error: "Invalid JSON" }, { status: 400 }); | |
| } | |
| const ip = getClientIp(request); | |
| const rl = await checkRateLimit(ip, RATE_LIMITS.GITHUB_WEBHOOK); | |
| const action = payload.action; | |
| if (event === "pull_request") { | |
| if (!shouldHandlePullRequestAction(action)) { | |
| return NextResponse.json( | |
| { ok: true, ignored: true, action }, | |
| { status: 200 }, | |
| ); | |
| } | |
| if (payload.pull_request?.draft && action !== "ready_for_review") { | |
| return NextResponse.json( | |
| { ok: true, ignored: true, reason: "draft" }, | |
| { status: 200 }, | |
| ); | |
| } | |
| } else if (event === "issues") { | |
| if (!shouldHandleIssueAction(action)) { | |
| return NextResponse.json( | |
| { ok: true, ignored: true, action }, | |
| { status: 200 }, | |
| ); | |
| if (rl.fallbackFailed) { | |
| console.error("[WebhookRoute] Rate limiters completely failed. DLQing webhook."); | |
| try { | |
| await prisma.webhookEvent.create({ | |
| data: { | |
| event: event || "unknown", | |
| payload: rawBody, | |
| status: "dlq", | |
| error: "Rate limiter and fallback completely failed", | |
| }, | |
| }); | |
| } catch (e) { | |
| console.error("[WebhookRoute] Failed to write to DLQ!", e); | |
| // Return 503 so GitHub retries the webhook delivery. | |
| return NextResponse.json({ error: "Webhook processing failed. Please retry." }, { status: 503 }); | |
| } | |
| } else if (event === "push") { | |
| // We accept all push events — no action filtering needed | |
| return NextResponse.json({ ok: true, message: "Webhook accepted and queued to DLQ due to severe outages" }, { status: 202 }); | |
| } | |
| /* | |
| * ┌──────────────────────────────────────────────────────────┐ | |
| * │ 4. Bot filtering │ | |
| * │ Ignore events sent by GitHub bots (including our own │ | |
| * │ automation) to prevent feedback loops. │ | |
| * └──────────────────────────────────────────────────────────┘ | |
| */ | |
| if (payload.sender?.type === "Bot") { | |
| return NextResponse.json( | |
| { ok: true, ignored: true, reason: "bot" }, | |
| { status: 200 }, | |
| ); | |
| } | |
| if (!rl.allowed) return rateLimitResponse(rl, "Webhook rate limit exceeded"); | |
| /* | |
| * ┌──────────────────────────────────────────────────────────┐ | |
| * │ 5. Field validation │ | |
| * │ Ensure the payload contains the minimum required │ | |
| * │ fields before proceeding to idempotency and enqueue. │ | |
| * └──────────────────────────────────────────────────────────┘ | |
| * Step 3: Event routing — only process events we handle. | |
| * Unsupported event types (label, milestone, etc.) and non-material PR/issue | |
| * actions get a silent 200. | |
| */ | |
| const owner = payload.repository?.owner?.login; | |
| const repo = payload.repository?.name; | |
| const number = payload.pull_request?.number || payload.issue?.number; | |
| const installationId = payload.installation?.id; | |
| if (!owner || !repo || (!number && event !== "push") || !installationId) { | |
| return NextResponse.json( | |
| { | |
| error: "Missing required fields", | |
| details: { owner, repo, number, installationId, event }, | |
| }, | |
| { status: 400 }, | |
| ); | |
| } | |
| const deliveryId = request.headers.get("x-github-delivery") || ""; | |
| /* | |
| * ┌──────────────────────────────────────────────────────────┐ | |
| * │ 6. Redis-based idempotency │ | |
| * │ Atomically claim the deliveryId so concurrent │ | |
| * │ deliveries of the same webhook are deduplicated. │ | |
| * │ The lock is released if the enqueue fails. │ | |
| * └──────────────────────────────────────────────────────────┘ | |
| */ | |
| let idempotencyKey: string | null = null; | |
| if (deliveryId) { | |
| idempotencyKey = generateWebhookKey(deliveryId, event || "unknown", action); | |
| const acquired = await tryAcquireIdempotency(idempotencyKey); | |
| if (!acquired) { | |
| return NextResponse.json( | |
| { ok: true, ignored: true, reason: "duplicate_delivery" }, | |
| { status: 200 }, | |
| ); | |
| } | |
| if (event !== "pull_request" && event !== "issues" && event !== "push") { | |
| return NextResponse.json({ ok: true, message: "Event type not handled" }, { status: 200 }); | |
| /* | |
| * Step 2: Event routing — only process events we handle. | |
| * Unsupported event types (label, milestone, etc.) get a silent 200 | |
| * without consuming processing quota. | |
| */ | |
| if (event !== "pull_request" && event !== "issues" && event !== "push") { | |
| return NextResponse.json({ ok: true, message: "Event type not handled" }, { status: 200 }); | |
| } | |
| /* | |
| * Step 3: Rate limiting | |
| * Apply per-IP rate limits after signature is validated and event is relevant. | |
| */ | |
| const ip = getClientIp(request); | |
| const rl = await checkRateLimit(ip, RATE_LIMITS.GITHUB_WEBHOOK); | |
| if (rl.fallbackFailed) { | |
| console.error("[WebhookRoute] Rate limiters completely failed. DLQing webhook."); | |
| try { | |
| await prisma.webhookEvent.create({ | |
| data: { | |
| event: event || "unknown", | |
| payload: rawBody, | |
| status: "dlq", | |
| error: "Rate limiter and fallback completely failed", | |
| }, | |
| }); | |
| } catch (e) { | |
| console.error("[WebhookRoute] Failed to write to DLQ!", e); | |
| // Return 503 so GitHub retries the webhook delivery. | |
| return NextResponse.json({ error: "Webhook processing failed. Please retry." }, { status: 503 }); | |
| } | |
| return NextResponse.json({ ok: true, message: "Webhook accepted and queued to DLQ due to severe outages" }, { status: 202 }); | |
| } | |
| if (!rl.allowed) return rateLimitResponse(rl, "Webhook rate limit exceeded"); | |
| /* | |
| * Step 4: Event processing | |
| * Non-material PR/issue actions get a silent 200. | |
| */ | |
| const deliveryId = request.headers.get("x-github-delivery") || ""; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/api/integrations/github/webhook/route.ts` around lines 91 - 126, The rate
limiting check via checkRateLimit is currently applied to all valid GitHub
webhook deliveries before filtering for supported event types. This means
unsupported events like label, milestone, and ping consume rate limit quota and
can exhaust RATE_LIMITS.GITHUB_WEBHOOK, causing legitimate pull_request, issues,
and push events to be rate-limited with 429 responses. Move the event type
validation (the check that returns 200 if the event is not pull_request, issues,
or push) to execute before the checkRateLimit call. This ensures unsupported
events are filtered out without consuming any rate limit quota.
Comment on lines
+377
to
+382
| const isSafe = await validateSafeUrl(avatar); | ||
| if (!isSafe) { | ||
| return NextResponse.json( | ||
| { error: "Avatar URL resolves to an untrusted or private network address." }, | ||
| { status: 400 } | ||
| ); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Move this validation before any profile-update mutations.
Line 379 can return 400 after the earlier Google-account/session deleteMany calls have already committed for email-changing linked accounts. An unsafe avatar URL can therefore partially unlink/invalidate the account while the profile update fails. Run all avatar validation before those DB mutations, or make the whole update flow transactional.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/api/users/profile/route.ts` around lines 377 - 382, The validateSafeUrl
call for the avatar URL validation is occurring after database mutations
(deleteMany calls for Google account/session management) have already been
committed, which means a failed avatar validation can leave the account in a
partially corrupted state. Move the avatar validation using validateSafeUrl to
the beginning of the profile update logic, before any database mutations are
executed, to ensure all validations pass before any data changes are persisted.
Comment on lines
+21
to
+40
| const ipv6MappedMatch = ip.match(/^::ffff:(\d+)\.(\d+)\.(\d+)\.(\d+)$/i); | ||
| if (ipv6MappedMatch) { | ||
| const parts = [ | ||
| parseInt(ipv6MappedMatch[1], 10), | ||
| parseInt(ipv6MappedMatch[2], 10), | ||
| parseInt(ipv6MappedMatch[3], 10), | ||
| parseInt(ipv6MappedMatch[4], 10), | ||
| ]; | ||
|
|
||
| if ( | ||
| parts[0] === 10 || // 10.0.0.0/8 | ||
| (parts[0] === 172 && parts[1] >= 16 && parts[1] <= 31) || // 172.16.0.0/12 | ||
| (parts[0] === 192 && parts[1] === 168) || // 192.168.0.0/16 | ||
| parts[0] === 127 || // 127.0.0.0/8 | ||
| (parts[0] === 169 && parts[1] === 254) || // 169.254.0.0/16 | ||
| parts[0] === 0 // 0.0.0.0/8 | ||
| ) { | ||
| return true; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Normalize all IPv6-mapped IPv4 forms before checking ranges.
Line 21 only matches compressed dotted-decimal ::ffff:a.b.c.d. Equivalent mapped forms like 0:0:0:0:0:ffff:127.0.0.1 or ::ffff:7f00:1 still fall through and are treated as public, so private/loopback IPv4 can bypass this validator. Prefer parsing/canonicalizing mapped IPv6 addresses and then reusing one IPv4 private-range predicate.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/utils/ssrfValidator.ts` around lines 21 - 40, The ipv6MappedMatch regex
in the ssrfValidator function only matches the compressed dotted-decimal format
of IPv6-mapped IPv4 addresses (::ffff:a.b.c.d), but other equivalent
representations like 0:0:0:0:0:ffff:a.b.c.d or ::ffff:7f00:1 bypass this check.
Refactor the code to normalize or parse all IPv6-mapped IPv4 address formats
first to extract the underlying IPv4 address, then apply the existing
private-range checking logic (the range checks from lines 31-37) to the
extracted IPv4 address instead of only matching one specific IPv6 format. This
ensures all equivalent IPv6-mapped representations are properly validated
against the private IP ranges.
Contributor
Author
|
PR updated: restored webhook processing logic (bot filtering, field validation, idempotency) + fixed ESLint unused imports. Closes #2469. |
Contributor
Author
|
CI Status: Playwright Tests PASSED, CodeQL PASSED, Prisma Schema Check PASSED. Test Platform and Worker Consistency failures are pre-existing upstream issues unrelated to this PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Four security and reliability fixes bundled into one PR:
1. SSRF Validator IPv6-Mapped Address Handling (lib/utils/ssrfValidator.ts)
The isPrivateIP function did not check IPv6-mapped IPv4 addresses (e.g. ::ffff:127.0.0.1). An attacker could bypass SSRF protection by using IPv6 notation for private IPs.
Fix: Added regex match for ::ffff:x.x.x.x format and validate the embedded IPv4 part.
2. Profile Avatar HTTP URL SSRF Validation (app/api/users/profile/route.ts)
HTTP avatar URLs were stored without SSRF validation, allowing attackers to probe internal services (e.g. AWS metadata at 169.254.169.254).
Fix: Added validateSafeUrl() check for HTTP avatar URLs before storing.
3. Analysis Job Null Next_Run_At (lib/services/analysisJobService.ts)
The job claiming SQL used WHERE next_run_at <= NOW() which returns NULL for NULL values in PostgreSQL, causing jobs with NULL next_run_at to never be picked up.
Fix: Use COALESCE(next_run_at, NOW()) <= NOW() to handle NULL values.
4. Webhook Rate Limit Before Signature (app/api/integrations/github/webhook/route.ts)
Rate limiting happened before signature verification, allowing attackers to exhaust the rate limit for legitimate IPs using forged requests. Also, DLQ write failure returned 202 OK.
Fix: Moved signature verification to step 1 (before rate limiting). Return 503 when DLQ write fails so GitHub retries.
Summary by CodeRabbit
Bug Fixes
New Features