🛡️ Sentinel: [CRITICAL] Fix SSRF vulnerability in bulk lookup API#202
🛡️ Sentinel: [CRITICAL] Fix SSRF vulnerability in bulk lookup API#202aicoder2009 wants to merge 1 commit into
Conversation
Removed dangerous dynamic loopback HTTP fetches that relied on the attacker-controllable `request.nextUrl.origin` inside the `src/app/api/lookup/bulk/route.ts` API endpoint. Directly imported and invoked the target Next.js `POST` route handlers using synthetic `NextRequest` objects, entirely eliminating the Server-Side Request Forgery (SSRF) vector while preserving functionality. Tests were updated to reflect and verify this safer internal routing. 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 route is refactored to directly invoke internal URL/DOI/ISBN POST handlers with synthetic NextRequest objects instead of fetching via request.nextUrl.origin, eliminating a loopback fetch pattern. Tests are updated to mock the internal handlers instead of global.fetch. A security documentation entry describes the SSRF risk and fix. ChangesSSRF fix in bulk lookup handler
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant BulkRoute
participant urlPOST
participant doiPOST
participant isbnPOST
Client->>BulkRoute: POST batch of lookup items
loop each item
BulkRoute->>BulkRoute: detect item format (URL/DOI/ISBN)
alt URL item
BulkRoute->>urlPOST: synthetic NextRequest with JSON body
urlPOST-->>BulkRoute: JSON response
else DOI item
BulkRoute->>doiPOST: synthetic NextRequest with JSON body
doiPOST-->>BulkRoute: JSON response
else ISBN item
BulkRoute->>isbnPOST: synthetic NextRequest with JSON body
isbnPOST-->>BulkRoute: JSON response
end
end
BulkRoute-->>Client: aggregated success/failure summary
Related Issues: None referenced in the provided changes. Suggested labels: security, refactor, tests Suggested reviewers: None determinable from the provided information. 🐇 A loopback closed, no more the fetch that strayed, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: lockfile failed supply-chain policy check. Run 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 |
There was a problem hiding this comment.
Pull request overview
This PR mitigates a critical SSRF risk in the bulk lookup API by removing internal loopback fetch(request.nextUrl.origin + ...) calls and instead directly invoking the relevant Next.js route handlers with synthetic NextRequest objects.
Changes:
- Replaced internal loopback
fetch()calls in the bulk lookup route with direct handler invocations (urlPOST,doiPOST,isbnPOST). - Updated bulk lookup tests to mock imported route handlers instead of
global.fetch. - Added a Sentinel entry documenting the SSRF learning/prevention guidance.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/app/api/lookup/bulk/route.ts |
Removes Host-header-influenced loopback fetches by calling internal lookup handlers directly. |
src/app/api/lookup/bulk/route.test.ts |
Switches from mocking fetch to mocking the imported route handlers used by the bulk route. |
.jules/sentinel.md |
Documents the SSRF pattern and the preferred mitigation approach for internal route calls. |
💡 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(); | ||
| vi.mock('../url/route', () => ({ POST: vi.fn() })); |
| let handler: typeof urlPOST; | ||
| let body: object; | ||
|
|
||
| // Detect input type | ||
| if (trimmedItem.match(/^(https?:\/\/|www\.)/i)) { |
| **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-07-01 - Prevent SSRF in Next.js Server Actions |
![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)
63-90: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the synthetic sub-request contract in the routing tests.
These cases only prove that a mock was called. If the bulk route sends the wrong JSON shape or accidentally calls multiple sub-handlers, they still pass because the mocks ignore the request argument. Please assert the selected handler receives the expected
{ url }/{ doi }/{ isbn }body and that the other handlers stay untouched. As per coding guidelines,**/{src/lib/citation/**/*.test.ts,src/app/api/**/*.test.ts}: Write tests for citation formatting and API route logic.Example assertion pattern
expect(data.results[0].success).toBe(true); - expect(urlPOST).toHaveBeenCalled(); + expect(urlPOST).toHaveBeenCalledTimes(1); + expect(doiPOST).not.toHaveBeenCalled(); + expect(isbnPOST).not.toHaveBeenCalled(); + const [requestArg] = (urlPOST as ReturnType<typeof vi.fn>).mock.calls[0]; + expect(await requestArg.json()).toEqual({ url: 'https://example.com' });🤖 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 - 90, The bulk routing tests for POST only verify that a handler was called, so they can miss incorrect sub-request payloads or extra handler invocations. Update the POST tests in route.test.ts to assert the selected mock handler receives the expected request body shape for each case ({ url }, { doi }, { isbn }) using the request argument passed into urlPOST, doiPOST, and isbnPOST. Also verify the other handler mocks remain untouched for each scenario so the routing contract is fully covered.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 @.jules/sentinel.md:
- Around line 9-12: Retitle this note to refer to route handlers instead of
Server Actions, since the issue is in the bulk lookup route handler rather than
an action. Update the heading in the sentinel entry and keep the rest of the
guidance aligned with the route handler pattern described by the bulk lookup
handler symbols such as the lookup route’s POST flow, so readers are pointed at
the correct Next.js surface.
In `@src/app/api/lookup/bulk/route.ts`:
- Around line 29-32: The bulk lookup mapper in the route handler should validate
each entry before calling trim() so a non-string item does not throw outside the
per-item try/catch. Update the lookupPromises logic to check the item type
first, return a validation-style result for invalid entries, and only call
trim() on confirmed strings inside the same item-level handling used by the bulk
route.
---
Nitpick comments:
In `@src/app/api/lookup/bulk/route.test.ts`:
- Around line 63-90: The bulk routing tests for POST only verify that a handler
was called, so they can miss incorrect sub-request payloads or extra handler
invocations. Update the POST tests in route.test.ts to assert the selected mock
handler receives the expected request body shape for each case ({ url }, { doi
}, { isbn }) using the request argument passed into urlPOST, doiPOST, and
isbnPOST. Also verify the other handler mocks remain untouched for each scenario
so the routing contract is fully covered.
🪄 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: e08d1a16-b759-4464-aeb3-cabbb90dc988
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
.jules/sentinel.mdsrc/app/api/lookup/bulk/route.test.tssrc/app/api/lookup/bulk/route.ts
| ## 2025-07-01 - Prevent SSRF in Next.js Server Actions | ||
| **Vulnerability:** The application was previously making loopback HTTP requests to itself using `fetch(request.nextUrl.origin + '/api/...')` in the bulk lookup API. Using `request.nextUrl.origin` inside a server route allows an attacker to control the `Host` header, leading to Server-Side Request Forgery (SSRF) and redirecting internal server traffic to malicious external domains. | ||
| **Learning:** Next.js route handlers are standard async functions. We do not need to use HTTP `fetch()` to call another internal API route from within the server. We can import the route function directly and invoke it. | ||
| **Prevention:** Always directly import and invoke internal Next.js route handlers (e.g. `import { POST } from '../other/route'`) using a synthetic `NextRequest` object rather than relying on `fetch` with dynamically derived hostnames. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Retitle this entry to route handlers, not Server Actions.
Line 9 points readers at the wrong surface: the issue described in this PR came from src/app/api/lookup/bulk/route.ts, which is a route handler, not a Server Action.
Suggested fix
-## 2025-07-01 - Prevent SSRF in Next.js Server Actions
+## 2025-07-01 - Prevent SSRF in Next.js Route Handlers📝 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.
| ## 2025-07-01 - Prevent SSRF in Next.js Server Actions | |
| **Vulnerability:** The application was previously making loopback HTTP requests to itself using `fetch(request.nextUrl.origin + '/api/...')` in the bulk lookup API. Using `request.nextUrl.origin` inside a server route allows an attacker to control the `Host` header, leading to Server-Side Request Forgery (SSRF) and redirecting internal server traffic to malicious external domains. | |
| **Learning:** Next.js route handlers are standard async functions. We do not need to use HTTP `fetch()` to call another internal API route from within the server. We can import the route function directly and invoke it. | |
| **Prevention:** Always directly import and invoke internal Next.js route handlers (e.g. `import { POST } from '../other/route'`) using a synthetic `NextRequest` object rather than relying on `fetch` with dynamically derived hostnames. | |
| ## 2025-07-01 - Prevent SSRF in Next.js Route Handlers | |
| **Vulnerability:** The application was previously making loopback HTTP requests to itself using `fetch(request.nextUrl.origin + '/api/...')` in the bulk lookup API. Using `request.nextUrl.origin` inside a server route allows an attacker to control the `Host` header, leading to Server-Side Request Forgery (SSRF) and redirecting internal server traffic to malicious external domains. | |
| **Learning:** Next.js route handlers are standard async functions. We do not need to use HTTP `fetch()` to call another internal API route from within the server. We can import the route function directly and invoke it. | |
| **Prevention:** Always directly import and invoke internal Next.js route handlers (e.g. `import { POST } from '../other/route'`) using a synthetic `NextRequest` object rather than relying on `fetch` with dynamically derived hostnames. |
🤖 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 @.jules/sentinel.md around lines 9 - 12, Retitle this note to refer to route
handlers instead of Server Actions, since the issue is in the bulk lookup route
handler rather than an action. Update the heading in the sentinel entry and keep
the rest of the guidance aligned with the route handler pattern described by the
bulk lookup handler symbols such as the lookup route’s POST flow, so readers are
pointed at the correct Next.js surface.
| const lookupPromises = items.map(async (item) => { | ||
| const trimmedItem = item.trim(); | ||
| if (!trimmedItem) { | ||
| return { input: item, success: false, error: "Empty input" }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Validate each batch item before calling .trim().
Line 30 assumes every array element is a string, and that happens before the per-item try. A payload like { items: [null, '10.1000/xyz'] } will reject the mapper, make Promise.all(...) fail, and turn the whole batch into a 500 instead of a validation error for that entry.
Suggested fix
- const lookupPromises = items.map(async (item) => {
- const trimmedItem = item.trim();
+ const lookupPromises = items.map(async (item) => {
+ if (typeof item !== "string") {
+ return { input: String(item), success: false, error: "Each item must be a string" };
+ }
+ const trimmedItem = item.trim();📝 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 (!trimmedItem) { | |
| return { input: item, success: false, error: "Empty input" }; | |
| const lookupPromises = items.map(async (item) => { | |
| if (typeof item !== "string") { | |
| return { input: String(item), success: false, error: "Each item must be a string" }; | |
| } | |
| const trimmedItem = item.trim(); | |
| if (!trimmedItem) { | |
| return { input: item, success: false, error: "Empty input" }; |
🤖 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 29 - 32, The bulk lookup
mapper in the route handler should validate each entry before calling trim() so
a non-string item does not throw outside the per-item try/catch. Update the
lookupPromises logic to check the item type first, return a validation-style
result for invalid entries, and only call trim() on confirmed strings inside the
same item-level handling used by the bulk route.
🚨 Severity: CRITICAL
💡 Vulnerability: The application was making loopback HTTP requests to itself using
fetch(request.nextUrl.origin + '/api/...')within the bulk lookup API endpoint. Usingrequest.nextUrl.origininside a server route allows an attacker to control theHostheader.🎯 Impact: This created a Server-Side Request Forgery (SSRF) vulnerability. An attacker could spoof the
Hostheader in their request, causing the internalfetchcall to be redirected to a malicious external server. This could lead to information disclosure or allow an attacker to probe internal services.🔧 Fix: I replaced the vulnerable HTTP loopback
fetch()calls with direct function invocations of the respective Next.js route handlers (urlPOST,doiPOST,isbnPOST). I constructed syntheticNextRequestobjects to pass to these handlers, safely bypassing any network-level routing and host origin dependencies.✅ Verification: The file
src/app/api/lookup/bulk/route.tsnow uses direct handler invocations. The test filesrc/app/api/lookup/bulk/route.test.tshas been updated to mock the imported route handlers instead ofglobal.fetch. Runningpnpm test:runvalidates that all lookups behave correctly.PR created automatically by Jules for task 15885741833350580002 started by @aicoder2009
Summary by CodeRabbit