Skip to content

🛡️ Sentinel: [CRITICAL] Fix SSRF in bulk lookup API #195

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
aicoder2009 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

🛡️ Sentinel: [CRITICAL] Fix SSRF in bulk lookup API #195

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .jules/sentinel.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,7 @@
**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-02-28 - [SSRF via Host Header Loopback in Next.js API]
**Vulnerability:** The bulk lookup API (`src/app/api/lookup/bulk/route.ts`) performed loopback API calls to process individual lookups by dynamically constructing URLs using `request.nextUrl.origin` (e.g., `fetch(`${request.nextUrl.origin}/api/lookup/...`)`). In Next.js, `request.nextUrl.origin` is derived directly from the user-controlled `Host` header. This allows attackers to manipulate the `Host` header and redirect the internal `fetch` call to a malicious server, leading to Server-Side Request Forgery (SSRF).
**Learning:** Never rely on the `Host` header or `request.nextUrl.origin` for internal loopback requests in a Next.js environment, as it is fully controllable by the client. Doing so inherently exposes SSRF risks.
**Prevention:** Rather than performing HTTP `fetch` requests over the network for internal API routes, directly import and invoke the corresponding Next.js Route Handler functions (e.g., `POST(syntheticRequest)`). For required network loopbacks, hardcode the trusted origin (e.g., via a protected environment variable or `localhost:<port>`).
48 changes: 19 additions & 29 deletions src/app/api/lookup/bulk/route.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,13 @@
import { describe, it, expect, vi, beforeEach } from 'vitest';
import { POST } from './route';
import { NextRequest } from 'next/server';
import { NextRequest, NextResponse } from 'next/server';
import { POST as urlLookup } from '@/app/api/lookup/url/route';
import { POST as doiLookup } from '@/app/api/lookup/doi/route';
import { POST as isbnLookup } from '@/app/api/lookup/isbn/route';

vi.mock('@/app/api/lookup/url/route', () => ({ POST: vi.fn() }));
vi.mock('@/app/api/lookup/doi/route', () => ({ POST: vi.fn() }));
vi.mock('@/app/api/lookup/isbn/route', () => ({ POST: vi.fn() }));

global.fetch = vi.fn();

Expand Down Expand Up @@ -54,57 +61,41 @@ describe('Bulk Lookup API', () => {
});

it('routes URLs to /api/lookup/url', async () => {
(global.fetch as ReturnType<typeof vi.fn>).mockResolvedValueOnce({
ok: true,
json: async () => ({ data: { title: 'Example Page' } }),
});
(urlLookup 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(urlLookup).toHaveBeenCalled();
});

it('routes DOIs to /api/lookup/doi', async () => {
(global.fetch as ReturnType<typeof vi.fn>).mockResolvedValueOnce({
ok: true,
json: async () => ({ data: { title: 'Research Article' } }),
});
(doiLookup as ReturnType<typeof vi.fn>).mockResolvedValueOnce(NextResponse.json({ data: { title: 'Research Article' } }, { status: 200 }));
const response = await POST(makeRequest({ items: ['10.1000/xyz123'] }));
const data = await response.json();
expect(data.results[0].success).toBe(true);
const [url] = (global.fetch as ReturnType<typeof vi.fn>).mock.calls[0] as [string];
expect(url).toContain('/api/lookup/doi');
expect(doiLookup).toHaveBeenCalled();
});

it('routes ISBNs to /api/lookup/isbn', async () => {
(global.fetch as ReturnType<typeof vi.fn>).mockResolvedValueOnce({
ok: true,
json: async () => ({ data: { title: 'Book Title' } }),
});
(isbnLookup as ReturnType<typeof vi.fn>).mockResolvedValueOnce(NextResponse.json({ data: { title: 'Book Title' } }, { status: 200 }));
const response = await POST(makeRequest({ items: ['9780316769174'] }));
const data = await response.json();
expect(data.results[0].success).toBe(true);
const [url] = (global.fetch as ReturnType<typeof vi.fn>).mock.calls[0] as [string];
expect(url).toContain('/api/lookup/isbn');
expect(isbnLookup).toHaveBeenCalled();
});

it('marks item as failed when sub-request fails', async () => {
(global.fetch as ReturnType<typeof vi.fn>).mockResolvedValueOnce({
ok: false,
json: async () => ({ error: 'Not found' }),
});
(doiLookup as ReturnType<typeof vi.fn>).mockResolvedValueOnce(NextResponse.json({ error: 'Not found' }, { status: 404 }));
const response = await POST(makeRequest({ items: ['10.1000/nonexistent'] }));
const data = await response.json();
expect(data.results[0].success).toBe(false);
expect(data.results[0].error).toBe('Not found');
});

it('returns summary counts', async () => {
(global.fetch as ReturnType<typeof vi.fn>)
.mockResolvedValueOnce({ ok: true, json: async () => ({ data: { title: 'A' } }) })
.mockResolvedValueOnce({ ok: false, json: async () => ({ error: 'fail' }) });
(urlLookup as ReturnType<typeof vi.fn>).mockResolvedValueOnce(NextResponse.json({ data: { title: 'A' } }, { status: 200 }));
(doiLookup as ReturnType<typeof vi.fn>).mockResolvedValueOnce(NextResponse.json({ error: 'fail' }, { status: 400 }));
const response = await POST(
makeRequest({ items: ['https://success.com', '10.1000/fail'] })
);
Expand All @@ -115,9 +106,8 @@ describe('Bulk Lookup API', () => {
});

it('handles mixed item types in one batch', async () => {
(global.fetch as ReturnType<typeof vi.fn>)
.mockResolvedValueOnce({ ok: true, json: async () => ({ data: { title: 'URL result' } }) })
.mockResolvedValueOnce({ ok: true, json: async () => ({ data: { title: 'DOI result' } }) });
(urlLookup as ReturnType<typeof vi.fn>).mockResolvedValueOnce(NextResponse.json({ data: { title: 'URL result' } }, { status: 200 }));
(doiLookup as ReturnType<typeof vi.fn>).mockResolvedValueOnce(NextResponse.json({ data: { title: 'DOI result' } }, { status: 200 }));
const response = await POST(
makeRequest({ items: ['https://example.com', '10.1000/abc'] })
);
Expand Down
20 changes: 11 additions & 9 deletions src/app/api/lookup/bulk/route.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
import { NextRequest, NextResponse } from "next/server";
import { POST as urlLookup } from "@/app/api/lookup/url/route";
import { POST as doiLookup } from "@/app/api/lookup/doi/route";
import { POST as isbnLookup } from "@/app/api/lookup/isbn/route";

interface LookupResult {
input: string;
Expand All @@ -22,27 +25,26 @@ export async function POST(request: NextRequest) {
}

// Refactored to process lookups concurrently for performance improvement
const baseUrl = request.nextUrl.origin;

const lookupPromises = items.map(async (item) => {
const trimmedItem = item.trim();
Comment on lines 28 to 29

@coderabbitai coderabbitai Bot Jun 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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.

Suggested change
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) {
return { input: item, success: false, error: "Empty input" };
}

try {
let apiEndpoint: string;
// eslint-disable-next-line @typescript-eslint/no-explicit-any
let handler: (req: NextRequest) => Promise<any>;
let body: object;

// Detect input type
if (trimmedItem.match(/^(https?:\/\/|www\.)/i)) {
apiEndpoint = "/api/lookup/url";
handler = urlLookup;
body = { url: trimmedItem };
Comment on lines 40 to 42

@coderabbitai coderabbitai Bot Jun 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.

Suggested change
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.

} else if (trimmedItem.match(/^10\.\d{4,}/)) {
apiEndpoint = "/api/lookup/doi";
handler = doiLookup;
body = { doi: trimmedItem };
} else if (trimmedItem.match(/^(97[89])?\d{9}[\dXx]$/)) {
apiEndpoint = "/api/lookup/isbn";
handler = isbnLookup;
body = { isbn: trimmedItem };
} else {
return {
Expand All @@ -52,13 +54,13 @@ export async function POST(request: NextRequest) {
};
}

// Make the API call
const response = await fetch(`${baseUrl}${apiEndpoint}`, {
// Directly invoke the handler to prevent SSRF via Host header manipulation
const syntheticRequest = new NextRequest(new URL('http://localhost'), {
method: "POST",
headers: { "Content-Type": "application/json" },
body: JSON.stringify(body),
});

const response = await handler(syntheticRequest);
const data = await response.json();

if (response.ok && data.data) {
Expand Down
Loading