🛡️ Sentinel: [HIGH] Fix Host-Header SSRF in bulk lookup API#140
🛡️ Sentinel: [HIGH] Fix Host-Header SSRF in bulk lookup API#140aicoder2009 wants to merge 1 commit into
Conversation
Refactored `src/app/api/lookup/bulk/route.ts` to directly invoke internal `POST` handlers (`urlHandler`, `doiHandler`, `isbnHandler`) by constructing a synthetic `NextRequest` object instead of using a loopback `fetch` call with `request.nextUrl.origin`. This prevents Host-Header SSRF where an attacker could manipulate the Host header to route requests to unintended internal networks, while also marginally improving performance by eliminating network round-trip overhead. Tests were also updated to mock the imported route handlers directly instead of 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.
|
📝 WalkthroughWalkthroughThis PR refactors the bulk lookup endpoint to prevent Host-Header SSRF attacks by replacing HTTP fetch calls with direct invocation of internal route handlers using synthetic localhost requests. Security documentation, handler imports, request construction logic, and test assertions are updated accordingly. ChangesHost-Header SSRF Mitigation via Direct Handler Invocation
Sequence DiagramsequenceDiagram
participant BulkRoute as /api/lookup/bulk
participant InputValidation as Input Type Detection
participant URLHandler as /api/lookup/url Handler
participant DOIHandler as /api/lookup/doi Handler
participant ISBNHandler as /api/lookup/isbn Handler
BulkRoute->>InputValidation: Regex match item type
InputValidation->>BulkRoute: Handler + endpoint selected
BulkRoute->>BulkRoute: Create synthetic NextRequest<br/>(localhost URL + JSON body)
BulkRoute->>URLHandler: handler(syntheticRequest)
URLHandler->>BulkRoute: NextResponse result
BulkRoute->>DOIHandler: handler(syntheticRequest)
DOIHandler->>BulkRoute: NextResponse result
BulkRoute->>ISBNHandler: handler(syntheticRequest)
ISBNHandler->>BulkRoute: NextResponse result
BulkRoute->>BulkRoute: Aggregate per-item results<br/>into final JSON response
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 mitigates a Host-header SSRF vector in the /api/lookup/bulk endpoint by removing loopback fetch(request.nextUrl.origin + ...) calls and instead invoking internal lookup route handlers directly with synthetic NextRequest objects.
Changes:
- Replaced loopback
fetchcalls in the bulk lookup handler with direct invocation of/api/lookup/url,/api/lookup/doi, and/api/lookup/isbnhandlers. - Updated bulk lookup tests to mock and assert direct handler invocation rather than mocking
global.fetch. - Added a Sentinel security learning entry documenting the Host-header SSRF pattern and prevention approach.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/app/api/lookup/bulk/route.ts | Removes loopback network calls and calls lookup handlers directly via a synthetic NextRequest. |
| src/app/api/lookup/bulk/route.test.ts | Switches tests from mocking fetch to mocking the individual lookup route handlers. |
| .jules/sentinel.md | Documents the Host-header SSRF issue and recommended mitigation pattern. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { describe, it, expect, vi, beforeEach } from 'vitest'; | ||
| import { POST } from './route'; | ||
| import { NextRequest } from 'next/server'; | ||
| import { NextRequest, NextResponse } from 'next/server'; | ||
|
|
||
| global.fetch = vi.fn(); | ||
| // Mock the individual route handlers | ||
| vi.mock('../url/route', () => ({ POST: vi.fn() })); | ||
| vi.mock('../doi/route', () => ({ POST: vi.fn() })); | ||
| vi.mock('../isbn/route', () => ({ POST: vi.fn() })); | ||
|
|
||
| import { POST as mockUrlHandler } from '../url/route'; | ||
| import { POST as mockDoiHandler } from '../doi/route'; | ||
| import { POST as mockIsbnHandler } from '../isbn/route'; |
| **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-06-01 - Prevent Host-Header SSRF in Next.js Loopback API Calls |
| let handler: (req: NextRequest) => Promise<NextResponse>; | ||
| let apiEndpoint: string; | ||
| let body: object; | ||
| let requestBody: object; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/app/api/lookup/bulk/route.test.ts (1)
100-108: ⚡ Quick winAdd test case for handler exceptions.
The error handling in
route.ts(lines 79-85) catches exceptions from handlers and converts them to failed results, but this behavior is not covered by tests. Currently, tests only verify the case where handlers return error responses with non-OK status. Add a test where a handler throws an exception to ensure the try-catch block works as expected.🧪 Proposed test case
it('handles handler exceptions gracefully', async () => { (mockDoiHandler as ReturnType<typeof vi.fn>).mockRejectedValueOnce( new Error('Handler crashed') ); const response = await POST(makeRequest({ items: ['10.1000/error'] })); const data = await response.json(); expect(data.results[0].success).toBe(false); expect(data.results[0].error).toBe('Handler crashed'); });🤖 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 100 - 108, Add a test that verifies the route's try-catch converts thrown handler exceptions into failed results: mock the DOI handler to reject (use mockDoiHandler.mockRejectedValueOnce(new Error('Handler crashed'))), call POST(makeRequest({ items: ['10.1000/error'] })), parse the JSON and assert data.results[0].success is false and data.results[0].error equals the thrown error message; this covers the exception path exercised by the try-catch in route.ts (the handler call surrounding lines handling returned responses).
🤖 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.test.ts`:
- Around line 63-74: The test currently only asserts req.url contains
"/api/lookup/url" which would miss Host-Header SSRF regressions; change the
assertion to verify the full URL begins with the expected localhost origin (the
same origin used by makeRequest) plus "/api/lookup/url" (e.g., assert
req.url.startsWith("<localhost-origin>/api/lookup/url") or equivalent), and
apply the same stronger full-origin check to the DOI and ISBN tests that call
mockDoiHandler and mockIsbnHandler; locate these by the symbols mockUrlHandler,
mockDoiHandler, mockIsbnHandler, POST, and makeRequest in route.test.ts.
---
Nitpick comments:
In `@src/app/api/lookup/bulk/route.test.ts`:
- Around line 100-108: Add a test that verifies the route's try-catch converts
thrown handler exceptions into failed results: mock the DOI handler to reject
(use mockDoiHandler.mockRejectedValueOnce(new Error('Handler crashed'))), call
POST(makeRequest({ items: ['10.1000/error'] })), parse the JSON and assert
data.results[0].success is false and data.results[0].error equals the thrown
error message; this covers the exception path exercised by the try-catch in
route.ts (the handler call surrounding lines handling returned responses).
🪄 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: 31349e85-9203-4eba-b47b-e9541821e9c7
📒 Files selected for processing (3)
.jules/sentinel.mdsrc/app/api/lookup/bulk/route.test.tssrc/app/api/lookup/bulk/route.ts
| it('routes URLs to /api/lookup/url', async () => { | ||
| (global.fetch as ReturnType<typeof vi.fn>).mockResolvedValueOnce({ | ||
| ok: true, | ||
| json: async () => ({ data: { title: 'Example Page' } }), | ||
| }); | ||
| (mockUrlHandler as ReturnType<typeof vi.fn>).mockResolvedValueOnce( | ||
| NextResponse.json({ data: { title: 'Example Page' } }, { status: 200 }) | ||
| ); | ||
| 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'); | ||
| const [url] = (global.fetch as ReturnType<typeof vi.fn>).mock.calls[0] as [string]; | ||
| expect(url).toContain('/api/lookup/url'); | ||
| expect(mockUrlHandler).toHaveBeenCalled(); | ||
| const req = (mockUrlHandler as ReturnType<typeof vi.fn>).mock.calls[0][0] as NextRequest; | ||
| expect(req.url).toContain('/api/lookup/url'); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Strengthen assertion to verify localhost origin and prevent SSRF regression.
The test verifies that req.url contains /api/lookup/url, but this assertion would pass even if the implementation reverted to using request.nextUrl.origin + '/api/lookup/url', which would reintroduce the Host-Header SSRF vulnerability. To prevent security regression, verify the complete URL including the localhost origin.
🔒 Proposed fix to strengthen security assertion
expect(mockUrlHandler).toHaveBeenCalled();
const req = (mockUrlHandler as ReturnType<typeof vi.fn>).mock.calls[0][0] as NextRequest;
- expect(req.url).toContain('/api/lookup/url');
+ expect(req.url).toBe('http://localhost/api/lookup/url');Apply the same pattern to the DOI test (line 85) and ISBN test (line 97):
- expect(req.url).toContain('/api/lookup/doi');
+ expect(req.url).toBe('http://localhost/api/lookup/doi');- expect(req.url).toContain('/api/lookup/isbn');
+ expect(req.url).toBe('http://localhost/api/lookup/isbn');📝 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.
| it('routes URLs to /api/lookup/url', async () => { | |
| (global.fetch as ReturnType<typeof vi.fn>).mockResolvedValueOnce({ | |
| ok: true, | |
| json: async () => ({ data: { title: 'Example Page' } }), | |
| }); | |
| (mockUrlHandler as ReturnType<typeof vi.fn>).mockResolvedValueOnce( | |
| NextResponse.json({ data: { title: 'Example Page' } }, { status: 200 }) | |
| ); | |
| 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'); | |
| const [url] = (global.fetch as ReturnType<typeof vi.fn>).mock.calls[0] as [string]; | |
| expect(url).toContain('/api/lookup/url'); | |
| expect(mockUrlHandler).toHaveBeenCalled(); | |
| const req = (mockUrlHandler as ReturnType<typeof vi.fn>).mock.calls[0][0] as NextRequest; | |
| expect(req.url).toContain('/api/lookup/url'); | |
| }); | |
| it('routes URLs to /api/lookup/url', async () => { | |
| (mockUrlHandler as ReturnType<typeof vi.fn>).mockResolvedValueOnce( | |
| NextResponse.json({ data: { title: 'Example Page' } }, { status: 200 }) | |
| ); | |
| 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(mockUrlHandler).toHaveBeenCalled(); | |
| const req = (mockUrlHandler as ReturnType<typeof vi.fn>).mock.calls[0][0] as NextRequest; | |
| expect(req.url).toBe('http://localhost/api/lookup/url'); | |
| }); |
🤖 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 63 - 74, The test
currently only asserts req.url contains "/api/lookup/url" which would miss
Host-Header SSRF regressions; change the assertion to verify the full URL begins
with the expected localhost origin (the same origin used by makeRequest) plus
"/api/lookup/url" (e.g., assert
req.url.startsWith("<localhost-origin>/api/lookup/url") or equivalent), and
apply the same stronger full-origin check to the DOI and ISBN tests that call
mockDoiHandler and mockIsbnHandler; locate these by the symbols mockUrlHandler,
mockDoiHandler, mockIsbnHandler, POST, and makeRequest in route.test.ts.
🛡️ Sentinel: [HIGH] Fix Host-Header SSRF in
/api/lookup/bulk🚨 Severity: HIGH
💡 Vulnerability: The bulk lookup endpoint used a loopback
fetchcombiningrequest.nextUrl.originto sequentially request internal API routes (/api/lookup/url, etc). This pattern is inherently vulnerable to Host-Header Server-Side Request Forgery (SSRF). An attacker could submit a maliciousHostheader to route these internal "lookups" to arbitrary destinations within the server's internal network.🎯 Impact: Allowed internal network scanning and potential SSRF bypass against internal services.
🔧 Fix: Removed the loopback
fetch. The endpoint now imports the respective route handlers directly and invokes them sequentially using a constructed syntheticNextRequestobject, completely bypassing the network layer and nullifying the Host header vector.✅ Verification: Run
pnpm test src/app/api/lookup/bulk/route.test.tsto confirm the handler functions process the mocked items locally without triggering a network fetch.PR created automatically by Jules for task 3133156799941850281 started by @aicoder2009
Summary by CodeRabbit
Security
Tests