🛡️ Sentinel: [CRITICAL] Fix SSRF in bulk lookup API#192
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 route now dispatches URL, DOI, and ISBN items by calling internal POST handlers directly with synthetic requests. The tests were updated to mock those handlers, and a sentinel note documents the SSRF risk from loopback fetches. ChangesBulk lookup direct handler dispatch
Sequence Diagram(s)sequenceDiagram
participant BulkLookupRoute
participant lookupUrlPOST
participant lookupDoiPOST
participant lookupIsbnPOST
BulkLookupRoute->>BulkLookupRoute: trim input and detect item type
alt URL item
BulkLookupRoute->>lookupUrlPOST: POST synthetic NextRequest
lookupUrlPOST-->>BulkLookupRoute: NextResponse.json(...)
else DOI item
BulkLookupRoute->>lookupDoiPOST: POST synthetic NextRequest
lookupDoiPOST-->>BulkLookupRoute: NextResponse.json(...)
else ISBN item
BulkLookupRoute->>lookupIsbnPOST: POST synthetic NextRequest
lookupIsbnPOST-->>BulkLookupRoute: NextResponse.json(...)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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: 3
🤖 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 10-12: The markdown example in the vulnerability note is using
single backticks around text that already contains a template literal, which
triggers the markdownlint MD038 warning. Update the inline example in the
sentinel content to wrap the whole
`fetch(\`${request.nextUrl.origin}/api/...\`)` snippet in double backticks so
the nested backticks render correctly.
In `@src/app/api/lookup/bulk/route.ts`:
- Around line 30-33: The bulk lookup mapping in the route handler calls
item.trim() before verifying the element is a string, so a non-string entry can
reject the Promise.all batch. Update the lookupPromises logic to validate each
item’s type first in the same map callback, and return a per-item failure result
for invalid values before any trim() call. Keep the fix localized to the
items.map async handler so one bad entry does not fail the entire request.
- Around line 47-49: Broaden the ISBN check in bulk lookup so it matches the
normalization used by lookupIsbnPOST in src/app/api/lookup/isbn/route.ts. The
current trimmedItem regex in the bulk route only accepts pre-cleaned digits/X,
so ISBNs with hyphens or spaces are rejected before reaching the ISBN handler.
Update the branch around handler selection and reqBody assignment to recognize
these common ISBN formats, normalize them the same way cleanISBN() does, and
then pass the cleaned ISBN through to lookupIsbnPOST.
🪄 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: 01ba36a5-d4a3-413d-a0f3-94f77009624a
📒 Files selected for processing (3)
.jules/sentinel.mdsrc/app/api/lookup/bulk/route.test.tssrc/app/api/lookup/bulk/route.ts
| **Vulnerability:** The `/api/lookup/bulk` endpoint invoked local API routes using `fetch(\`\${request.nextUrl.origin}/api/...\`)`. `request.nextUrl.origin` is derived from the user-provided `Host` header. An attacker could send a maliciously crafted `Host` header (e.g., `Host: internal-service.local`), causing the server to make requests to internal services on the attacker's behalf, resulting in a Server-Side Request Forgery (SSRF) vulnerability. | ||
| **Learning:** Relying on the `Host` header or `request.nextUrl.origin` to construct URLs for loopback requests in server-side code is inherently unsafe because these values are client-controlled. | ||
| **Prevention:** When calling internal Next.js API endpoints from within the server (e.g., in a route handler), import and invoke the exported route handler functions (like `POST`) directly using a synthetic request, rather than performing an HTTP `fetch` to a dynamically derived hostname. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Escape the template-literal example with double backticks.
The inline example on Line 10 nests backticks inside a code span, which is what markdownlint is complaining about here. Wrapping the whole example in double backticks fixes the parsing and clears the MD038 warning.
Suggested fix
-**Vulnerability:** The `/api/lookup/bulk` endpoint invoked local API routes using `fetch(\`${request.nextUrl.origin}/api/...\`)`. `request.nextUrl.origin` is derived from the user-provided `Host` header. An attacker could send a maliciously crafted `Host` header (e.g., `Host: internal-service.local`), causing the server to make requests to internal services on the attacker's behalf, resulting in a Server-Side Request Forgery (SSRF) vulnerability.
+**Vulnerability:** The `/api/lookup/bulk` endpoint invoked local API routes using ``fetch(`${request.nextUrl.origin}/api/...`)``. `request.nextUrl.origin` is derived from the user-provided `Host` header. An attacker could send a maliciously crafted `Host` header (e.g., `Host: internal-service.local`), causing the server to make requests to internal services on the attacker's behalf, resulting in a Server-Side Request Forgery (SSRF) vulnerability.📝 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.
| **Vulnerability:** The `/api/lookup/bulk` endpoint invoked local API routes using `fetch(\`\${request.nextUrl.origin}/api/...\`)`. `request.nextUrl.origin` is derived from the user-provided `Host` header. An attacker could send a maliciously crafted `Host` header (e.g., `Host: internal-service.local`), causing the server to make requests to internal services on the attacker's behalf, resulting in a Server-Side Request Forgery (SSRF) vulnerability. | |
| **Learning:** Relying on the `Host` header or `request.nextUrl.origin` to construct URLs for loopback requests in server-side code is inherently unsafe because these values are client-controlled. | |
| **Prevention:** When calling internal Next.js API endpoints from within the server (e.g., in a route handler), import and invoke the exported route handler functions (like `POST`) directly using a synthetic request, rather than performing an HTTP `fetch` to a dynamically derived hostname. | |
| **Vulnerability:** The `/api/lookup/bulk` endpoint invoked local API routes using ``fetch(`${request.nextUrl.origin}/api/...`)``. `request.nextUrl.origin` is derived from the user-provided `Host` header. An attacker could send a maliciously crafted `Host` header (e.g., `Host: internal-service.local`), causing the server to make requests to internal services on the attacker's behalf, resulting in a Server-Side Request Forgery (SSRF) vulnerability. | |
| **Learning:** Relying on the `Host` header or `request.nextUrl.origin` to construct URLs for loopback requests in server-side code is inherently unsafe because these values are client-controlled. | |
| **Prevention:** When calling internal Next.js API endpoints from within the server (e.g., in a route handler), import and invoke the exported route handler functions (like `POST`) directly using a synthetic request, rather than performing an HTTP `fetch` to a dynamically derived hostname. |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 10-10: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 11-11: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 12-12: Spaces inside code span elements
(MD038, no-space-in-code)
🤖 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 10 - 12, The markdown example in the
vulnerability note is using single backticks around text that already contains a
template literal, which triggers the markdownlint MD038 warning. Update the
inline example in the sentinel content to wrap the whole
`fetch(\`${request.nextUrl.origin}/api/...\`)` snippet in double backticks so
the nested backticks render correctly.
Source: Linters/SAST tools
| 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 item types before calling trim().
Line 31 runs item.trim() before any guard. A payload like { items: [42] } will reject the mapped promise, bubble through Promise.all, and turn one bad element into a 500 for the whole batch.
Suggested fix
if (items.length > 20) {
return NextResponse.json({ error: "Maximum 20 items allowed per request" }, { status: 400 });
}
+
+ if (!items.every((item): item is string => typeof item === "string")) {
+ return NextResponse.json({ error: "Each item must be a string" }, { status: 400 });
+ }
const lookupPromises = items.map(async (item) => {
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" }; | |
| if (items.length > 20) { | |
| return NextResponse.json({ error: "Maximum 20 items allowed per request" }, { status: 400 }); | |
| } | |
| if (!items.every((item): item is string => typeof item === "string")) { | |
| return NextResponse.json({ error: "Each item must be a string" }, { status: 400 }); | |
| } | |
| const lookupPromises = items.map(async (item) => { | |
| 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 30 - 33, The bulk lookup
mapping in the route handler calls item.trim() before verifying the element is a
string, so a non-string entry can reject the Promise.all batch. Update the
lookupPromises logic to validate each item’s type first in the same map
callback, and return a per-item failure result for invalid values before any
trim() call. Keep the fix localized to the items.map async handler so one bad
entry does not fail the entire request.
| } else if (trimmedItem.match(/^(97[89])?\d{9}[\dXx]$/)) { | ||
| apiEndpoint = "/api/lookup/isbn"; | ||
| body = { isbn: trimmedItem }; | ||
| handler = lookupIsbnPOST; | ||
| reqBody = { isbn: trimmedItem }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Broaden ISBN detection to match the ISBN handler.
src/app/api/lookup/isbn/route.ts calls cleanISBN() before validation, but this branch only accepts already-normalized digits/X. Inputs like 978-0-316-76917-4 or 0 306 40615 2 get rejected here as “Unrecognized format” instead of reaching the ISBN handler.
Suggested fix
- } else if (trimmedItem.match(/^(97[89])?\d{9}[\dXx]$/)) {
+ } else if (trimmedItem.replace(/[\s-]/g, "").match(/^(97[89])?\d{9}[\dXx]$/)) {
handler = lookupIsbnPOST;
reqBody = { isbn: 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.
| } else if (trimmedItem.match(/^(97[89])?\d{9}[\dXx]$/)) { | |
| apiEndpoint = "/api/lookup/isbn"; | |
| body = { isbn: trimmedItem }; | |
| handler = lookupIsbnPOST; | |
| reqBody = { isbn: trimmedItem }; | |
| } else if (trimmedItem.replace(/[\s-]/g, "").match(/^(97[89])?\d{9}[\dXx]$/)) { | |
| handler = lookupIsbnPOST; | |
| reqBody = { isbn: 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 47 - 49, Broaden the ISBN
check in bulk lookup so it matches the normalization used by lookupIsbnPOST in
src/app/api/lookup/isbn/route.ts. The current trimmedItem regex in the bulk
route only accepts pre-cleaned digits/X, so ISBNs with hyphens or spaces are
rejected before reaching the ISBN handler. Update the branch around handler
selection and reqBody assignment to recognize these common ISBN formats,
normalize them the same way cleanISBN() does, and then pass the cleaned ISBN
through to lookupIsbnPOST.
1 participant
🚨 Severity: CRITICAL
💡 Vulnerability:
The
/api/lookup/bulkendpoint invoked local Next.js API routes (URL, DOI, ISBN) by performing an HTTPfetchto a loopback address derived fromrequest.nextUrl.origin. Because this origin is derived from the user-controlledHostHTTP header, an attacker could craft a malicious request (e.g.,Host: internal-service.local), tricking the server into making arbitrary internal requests on the attacker's behalf, leading to Server-Side Request Forgery (SSRF).🎯 Impact:
An attacker could bypass network restrictions, scan internal networks, or trigger unintended actions on internal microservices that assume requests originating from the backend server are trusted.
🔧 Fix:
Replaced the
fetch()network call with direct invocations of the exportedPOSThandlers from the target route files (lookupUrlPOST,lookupDoiPOST,lookupIsbnPOST). A syntheticNextRequestwith a safe, hardcoded URL is constructed to pass directly to these handlers, entirely bypassing the network layer andHostheader interpretation.✅ Verification:
npx eslint src/app/api/lookup/bulk/route.ts- Code passes linting.global.fetch.pnpm test:run- All test suites pass successfully.PR created automatically by Jules for task 10470547538108334148 started by @aicoder2009
Summary by CodeRabbit