🛡️ Sentinel: [CRITICAL] Fix SSRF vulnerability in bulk lookup#161
🛡️ Sentinel: [CRITICAL] Fix SSRF vulnerability in bulk lookup#161aicoder2009 wants to merge 1 commit into
Conversation
Removed internal HTTP `fetch` loopbacks in `/api/lookup/bulk` which derived the origin from the user-controllable `request.nextUrl.origin` (`Host` header). This resolves a Server-Side Request Forgery (SSRF) vulnerability. The logic has been updated to directly import and invoke the internal Next.js Route Handlers (`urlHandler`, `doiHandler`, `isbnHandler`) using a synthetically constructed `NextRequest` object. Test cases have been appropriately mocked and verified. Co-authored-by: aicoder2009 <127642633+aicoder2009@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe PR fixes an SSRF vulnerability in the bulk lookup API route by replacing loopback HTTP fetch (which uses user-controlled Host headers) with direct invocation of internal sub-route handlers via synthetic ChangesSSRF Mitigation via Direct Handler Invocation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed due to a network error. Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/api/lookup/bulk/route.ts (1)
16-20:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHarden the bulk API contract for non-string
items.
src/app/api/lookup/bulk/route.tsvalidates thatitemsis an array, but not that every element is a string. A payload like{ items: [123] }reachesitem.trim()and turns a client error into a 500. Please reject non-string elements up front insrc/app/api/lookup/bulk/route.ts, and add a regression test insrc/app/api/lookup/bulk/route.test.tsto lock that behavior in.🤖 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 `@src/app/api/lookup/bulk/route.ts` around lines 16 - 20, Validate that every element of the parsed items array is a string before using item.trim(): after const { items } = body... and the existing falsy/empty checks, add a guard that checks Array.isArray(items) && items.every(x => typeof x === "string") and return NextResponse.json({ error: "Items must be an array of strings" }, { status: 400 }) if it fails; update the handler where item.trim() is called to assume elements are strings. Then add a regression test in src/app/api/lookup/bulk/route.test.ts that posts { items: [123] } (and another mixed-type array) and asserts a 400 response and the expected error message to lock this behavior.
🤖 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.
Outside diff comments:
In `@src/app/api/lookup/bulk/route.ts`:
- Around line 16-20: Validate that every element of the parsed items array is a
string before using item.trim(): after const { items } = body... and the
existing falsy/empty checks, add a guard that checks Array.isArray(items) &&
items.every(x => typeof x === "string") and return NextResponse.json({ error:
"Items must be an array of strings" }, { status: 400 }) if it fails; update the
handler where item.trim() is called to assume elements are strings. Then add a
regression test in src/app/api/lookup/bulk/route.test.ts that posts { items:
[123] } (and another mixed-type array) and asserts a 400 response and the
expected error message to lock this behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5504a29f-daff-459f-9395-bedf38ea84e1
📒 Files selected for processing (3)
.jules/sentinel.mdsrc/app/api/lookup/bulk/route.test.tssrc/app/api/lookup/bulk/route.ts
🚨 Severity: CRITICAL
💡 Vulnerability: The
/api/lookup/bulkendpoint usedrequest.nextUrl.originto determine the base URL for internal API calls viafetch(). Because this origin is derived from theHostheader, it could be spoofed by an attacker, leading to Server-Side Request Forgery (SSRF) and allowing the server to proxy requests to arbitrary external or internal network locations.🎯 Impact: Attackers could abuse the bulk lookup proxy to scan internal networks, access private resources, or bypass external IP-based protections.
🔧 Fix: Replaced network-based internal
fetchloopbacks with direct function invocations. The handlers for/api/lookup/url,/api/lookup/doi, and/api/lookup/isbnare now imported and called directly with a synthetically constructedNextRequest.✅ Verification:
pnpm lintandpnpm test:runpass. The tests forsrc/app/api/lookup/bulk/route.test.tswere updated to mock the internal handlers and confirm the direct invocation logic.PR created automatically by Jules for task 15995001700895303358 started by @aicoder2009
Summary by CodeRabbit
Bug Fixes