🛡️ Sentinel: [CRITICAL] Fix SSRF in bulk lookup API#138
Conversation
Refactored `/api/lookup/bulk/route.ts` to execute internal Next.js Route Handlers (`POST` methods for DOI, URL, ISBN) directly by constructing a synthetic `NextRequest` instead of performing a loopback `fetch` using the user-controllable `request.nextUrl.origin`. This closes a critical SSRF vulnerability where an attacker could have supplied a custom `Host` header to bypass authorization or redirect traffic over the internet. Also updated Vitest tests to directly mock the internal route handler exports (`POST`) rather than mocking `global.fetch`. 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.
|
📝 WalkthroughWalkthroughBulk lookup API refactored to invoke sub-route handlers directly instead of making HTTP requests, eliminating SSRF risk from internal fetch patterns. Security guidance added to sentinel documentation. Tests updated to mock per-route handlers instead of global fetch. ChangesBulk lookup SSRF mitigation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR fixes an SSRF risk in the bulk lookup API by replacing Host-header-derived loopback fetch calls with direct invocation of local lookup route handlers.
Changes:
- Imports URL, DOI, and ISBN lookup
POSThandlers directly in the bulk lookup route. - Replaces dynamic-origin internal HTTP requests with synthetic
NextRequestobjects. - Updates bulk lookup tests to mock route handlers instead of
global.fetch, and adds a Sentinel learning entry.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/app/api/lookup/bulk/route.ts |
Removes request.nextUrl.origin usage and calls local lookup handlers directly. |
src/app/api/lookup/bulk/route.test.ts |
Updates tests to mock imported lookup handlers. |
.jules/sentinel.md |
Documents the SSRF prevention lesson for future development. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| **Vulnerability:** The application was using the `marked` library to parse Markdown content into HTML (in `src/app/docs/changelog/page.tsx` and `src/lib/docs.ts`) and subsequently rendering it using `dangerouslySetInnerHTML` without proper sanitization. | ||
| **Learning:** `marked` does not sanitize HTML by default. While this may seem safe for trusted inputs (like internal docs or GitHub releases), if malicious input manages to enter these sources, it leads directly to an XSS vulnerability. | ||
| **Prevention:** The output of `marked` (or any markdown parser) must always be wrapped with `DOMPurify.sanitize()` (using `isomorphic-dompurify` for SSR) before being passed to `dangerouslySetInnerHTML`. | ||
| ## 2025-05-31 - Prevent SSRF via Internal Fetch Loops |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/app/api/lookup/bulk/route.test.ts (1)
62-72: ⚡ Quick winConsider verifying request body contents in handler assertions.
The tests verify that the correct handlers are called (
mockLookupUrl,mockLookupDoi,mockLookupIsbn) but don't assert that they receive the expected request bodies. Adding assertions for the request body would strengthen test coverage.🧪 Example: Assert request body for URL handler
it('routes URLs to /api/lookup/url', async () => { (mockLookupUrl as unknown as ReturnType<typeof vi.fn>).mockResolvedValueOnce({ ok: true, json: async () => ({ data: { title: 'Example Page' } }), }); const response = await POST(makeRequest({ items: ['https://example.com'] })); const data = await response.json(); expect(data.results[0].success).toBe(true); expect(data.results[0].data.title).toBe('Example Page'); expect(mockLookupUrl).toHaveBeenCalled(); + const callArg = (mockLookupUrl as unknown as ReturnType<typeof vi.fn>).mock.calls[0][0]; + const body = await callArg.json(); + expect(body).toEqual({ url: 'https://example.com' }); });Also applies to: 74-83, 85-94
🤖 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 62 - 72, The test currently checks that mockLookupUrl/mockLookupDoi/mockLookupIsbn were called but not what they were called with; update the tests in src/app/api/lookup/bulk/route.test.ts (the cases that call POST(makeRequest(...))) to assert the request body passed to the handler mocks by inspecting their call args (e.g., expect(mockLookupUrl).toHaveBeenCalledWith(expect.objectContaining({ body: /* expected payload or parsed JSON */ })) or asserting the parsed body arg directly), and add equivalent assertions for mockLookupDoi and mockLookupIsbn so each handler is verified to receive the exact items payload sent by makeRequest.src/app/api/lookup/bulk/route.ts (1)
35-35: 💤 Low valueAdd type annotation to
handlervariable.The
handlervariable lacks a type annotation, reducing type safety. Consider adding an explicit type to ensure only valid route handlers are assigned.📝 Suggested type annotation
- let handler; + let handler: (req: NextRequest) => Promise<NextResponse>; let body: object;🤖 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` at line 35, The local variable handler in src/app/api/lookup/bulk/route.ts is missing an explicit type; add a type annotation such as a request-to-response async function signature (for example, (request: Request) => Promise<Response>) or the project's route handler type so only valid handlers can be assigned to handler, and update any assigned functions to conform to that signature (ensure imports/types like Request/Response or NextRequest/NextResponse are used consistently with existing route handlers).
🤖 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.
Nitpick comments:
In `@src/app/api/lookup/bulk/route.test.ts`:
- Around line 62-72: The test currently checks that
mockLookupUrl/mockLookupDoi/mockLookupIsbn were called but not what they were
called with; update the tests in src/app/api/lookup/bulk/route.test.ts (the
cases that call POST(makeRequest(...))) to assert the request body passed to the
handler mocks by inspecting their call args (e.g.,
expect(mockLookupUrl).toHaveBeenCalledWith(expect.objectContaining({ body: /*
expected payload or parsed JSON */ })) or asserting the parsed body arg
directly), and add equivalent assertions for mockLookupDoi and mockLookupIsbn so
each handler is verified to receive the exact items payload sent by makeRequest.
In `@src/app/api/lookup/bulk/route.ts`:
- Line 35: The local variable handler in src/app/api/lookup/bulk/route.ts is
missing an explicit type; add a type annotation such as a request-to-response
async function signature (for example, (request: Request) => Promise<Response>)
or the project's route handler type so only valid handlers can be assigned to
handler, and update any assigned functions to conform to that signature (ensure
imports/types like Request/Response or NextRequest/NextResponse are used
consistently with existing route handlers).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0c439ccd-c300-4cee-81ec-cb84ab495e22
📒 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 application was using
fetch(request.nextUrl.origin + endpoint)to multiplex concurrent metadata lookups for citations. Because the Next.jsrequest.nextUrl.origininherits theHostheader from the client request, an attacker could spoof theHostheader, forcing the application to execute a Server-Side Request Forgery (SSRF) against an arbitrary host.🎯 Impact: Allowed unauthenticated external routing, possible internal network mapping, and authorization bypasses.
🔧 Fix: Replaced the loopback
fetchcalls with direct function invocation of the imported localPOSTroute handlers, passing a safe, statically generatedNextRequestobject (http://localhost).✅ Verification: Unit tests in
src/app/api/lookup/bulk/route.test.tshave been rewritten to mock local functions instead ofglobal.fetch. All 652 tests pass successfully.PR created automatically by Jules for task 4832378979654159316 started by @aicoder2009
Summary by CodeRabbit
Documentation
Bug Fixes & Improvements