🛡️ Sentinel: [CRITICAL] Fix SSRF in bulk lookup API#195
Conversation
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 bulk lookup API route is refactored to import and directly invoke the individual lookup sub-route handlers (URL, DOI, ISBN) using a synthetic ChangesSSRF Fix: Direct Handler Dispatch in Bulk Lookup
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/app/api/lookup/bulk/route.test.ts (1)
64-85: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAssert the synthetic request contract, not just that a handler was called.
These tests would be stronger if they passed a hostile incoming origin and asserted the mocked handler receives
http://localhostplus the expected JSON body.Example test shape
- (urlLookup as ReturnType<typeof vi.fn>).mockResolvedValueOnce(NextResponse.json({ data: { title: 'Example Page' } }, { status: 200 })); - const response = await POST(makeRequest({ items: ['https://example.com'] })); + (urlLookup as ReturnType<typeof vi.fn>).mockImplementationOnce(async (req: NextRequest) => { + expect(req.nextUrl.origin).toBe('http://localhost'); + await expect(req.json()).resolves.toEqual({ url: 'https://example.com' }); + return NextResponse.json({ data: { title: 'Example Page' } }, { status: 200 }); + }); + const response = await POST(new NextRequest('http://attacker.test/api/lookup/bulk', { + method: 'POST', + body: JSON.stringify({ items: ['https://example.com'] }), + }));As per coding guidelines, "
**/{src/lib/citation/**/*.test.ts,src/app/api/**/*.test.ts}: Write tests for citation formatting and API route logic."🤖 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.test.ts` around lines 64 - 85, The bulk lookup route tests only verify that the lookup handlers are called, but they should also assert the synthetic request contract used by POST in route.test.ts. Update the tests around POST/makeRequest so they pass a hostile incoming origin and confirm the mocked handler receives a request with http://localhost as the origin plus the expected JSON body for each item type. Use the existing symbols POST, makeRequest, urlLookup, doiLookup, and isbnLookup to locate the assertions and strengthen them to validate the forwarded request shape, not just invocation.Source: Coding guidelines
🤖 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 `@src/app/api/lookup/bulk/route.ts`:
- Around line 40-42: The bulk lookup URL branch in the route handler accepts
values starting with www. but passes them directly to urlLookup, which later
parses them with new URL and fails. Update the URL-handling logic in the bulk
route to normalize www. inputs by prefixing https:// before assigning the body,
while keeping existing http/https URLs unchanged.
- Around line 28-29: The bulk lookup handler in route.ts should validate each
element in items before calling trim() inside lookupPromises. Update the item
processing logic to reject non-string entries early, returning a client error
instead of letting item.trim() throw and bubble into a 500. Use the existing
bulk request flow around lookupPromises and the item handling path to add a
per-item type check before trimming and lookup.
---
Nitpick comments:
In `@src/app/api/lookup/bulk/route.test.ts`:
- Around line 64-85: The bulk lookup route tests only verify that the lookup
handlers are called, but they should also assert the synthetic request contract
used by POST in route.test.ts. Update the tests around POST/makeRequest so they
pass a hostile incoming origin and confirm the mocked handler receives a request
with http://localhost as the origin plus the expected JSON body for each item
type. Use the existing symbols POST, makeRequest, urlLookup, doiLookup, and
isbnLookup to locate the assertions and strengthen them to validate the
forwarded request shape, not just invocation.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e49eb0a4-1643-4a26-a78a-6b385c0d2022
📒 Files selected for processing (3)
.jules/sentinel.mdsrc/app/api/lookup/bulk/route.test.tssrc/app/api/lookup/bulk/route.ts
| const lookupPromises = items.map(async (item) => { | ||
| const trimmedItem = item.trim(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Validate item element types before trimming.
items is only checked as an array, so a payload like { "items": [123] } makes item.trim() reject and turns the whole bulk request into a 500.
Proposed fix
if (!items || !Array.isArray(items) || items.length === 0) {
return NextResponse.json({ error: "Items array is required" }, { status: 400 });
}
+
+ if (!items.every((item): item is string => typeof item === "string")) {
+ return NextResponse.json({ error: "All items must be strings" }, { status: 400 });
+ }📝 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.
| const lookupPromises = items.map(async (item) => { | |
| const trimmedItem = item.trim(); | |
| if (!items.every((item): item is string => typeof item === "string")) { | |
| return NextResponse.json({ error: "All items must be strings" }, { status: 400 }); | |
| } | |
| const lookupPromises = items.map(async (item) => { | |
| const trimmedItem = item.trim(); |
🤖 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 28 - 29, The bulk lookup
handler in route.ts should validate each element in items before calling trim()
inside lookupPromises. Update the item processing logic to reject non-string
entries early, returning a client error instead of letting item.trim() throw and
bubble into a 500. Use the existing bulk request flow around lookupPromises and
the item handling path to add a per-item type check before trimming and lookup.
| if (trimmedItem.match(/^(https?:\/\/|www\.)/i)) { | ||
| apiEndpoint = "/api/lookup/url"; | ||
| handler = urlLookup; | ||
| body = { url: trimmedItem }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Normalize www. URLs before dispatching.
This branch accepts www.example.com, but the URL handler calls new URL(url), so it returns “Invalid URL format.” Prefix https:// before passing the body.
Proposed fix
if (trimmedItem.match(/^(https?:\/\/|www\.)/i)) {
handler = urlLookup;
- body = { url: trimmedItem };
+ body = { url: trimmedItem.match(/^www\./i) ? `https://${trimmedItem}` : trimmedItem };📝 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.
| if (trimmedItem.match(/^(https?:\/\/|www\.)/i)) { | |
| apiEndpoint = "/api/lookup/url"; | |
| handler = urlLookup; | |
| body = { url: trimmedItem }; | |
| if (trimmedItem.match(/^(https?:\/\/|www\.)/i)) { | |
| handler = urlLookup; | |
| body = { url: trimmedItem.match(/^www\./i) ? `https://${trimmedItem}` : trimmedItem }; |
🤖 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 40 - 42, The bulk lookup URL
branch in the route handler accepts values starting with www. but passes them
directly to urlLookup, which later parses them with new URL and fails. Update
the URL-handling logic in the bulk route to normalize www. inputs by prefixing
https:// before assigning the body, while keeping existing http/https URLs
unchanged.
1 participant
🚨 Severity: CRITICAL
💡 Vulnerability: The application was vulnerable to Server-Side Request Forgery (SSRF) in the bulk lookup API (
src/app/api/lookup/bulk/route.ts). Loopbackfetchcalls were constructed dynamically usingrequest.nextUrl.origin, which relies on the client-controlledHostheader. An attacker could manipulate this header to force the internal API to make requests to an arbitrary external server.🎯 Impact: An attacker could perform network scanning or access sensitive internal services via the server, potentially exposing metadata or bypassing external firewall restrictions.
🔧 Fix: Replaced the loopback
fetchmechanism with direct invocations of the corresponding Next.js Route Handlers (urlLookup,doiLookup,isbnLookup), using a hardcoded, synthetic internal URL request object. This entirely bypasses the network layer and ignores external Host headers, inherently eliminating the SSRF. Updated Vitest suite appropriately.✅ Verification: Verify that
pnpm test:runpasses. Thebulk/route.test.tsfile now asserts against direct handler calls usingvi.mockinstead of mockingglobal.fetch.PR created automatically by Jules for task 7184815631816819964 started by @aicoder2009
Summary by CodeRabbit
Security Fixes
Tests